Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make docker build multistage #1234

Closed
wants to merge 3 commits into from

Conversation

nilsnolde
Copy link
Contributor

@nilsnolde nilsnolde commented Nov 30, 2022

Fixes #1232

Multistaging the ORS image brings down the uncompressed size from 1.06 GB to 365 GB.

EDIT: actually the most important improvement I forgot😅. Before, the Java code was compiled for every container. Now it’s compiled in the image, and a container starts building a graph instantly, instead of first compiling the project for 10 mins.

I also tried for along time to keep the logic with USER etc, but eventually it seems rather like it was more redundant and didn't really do what I'd have expected it to do, even before the change. So I removed most of that which makes it a bit easier to maintain I guess.

Basically, before this change, the paths which are mounted as volumes in the docker-compose, were never really owned by whatever the UID was. They were always owned by root. If I actually provided a user ID in docker-compose.yml that matched my host one (1000), it wasn't able to create any files in those directories. AFAIU it's happening because after the image is created, those paths are mounted on the host machine, which makes them root. I tried fixing that as well, but couldn't in any reasonable time. TBH, I do that exact thing on our Valhalla image and it works. I tried replicating exactly that for 2 hours with this image and I couldn't get it to work. In the end the effect is that nothing changes compared to before, other than having 1/3 of the image size and a MUCH quicker container startup time.

More comments in the changes.

Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@rabidllama
Copy link
Contributor

Seems OK to me, the main thing though that I am not sure about is the user stuff as from what I recall, that was added to fix problems people were having with the container not being able to write to the folders

@nilsnolde
Copy link
Contributor Author

The way I read the code before these changes was that you'd provide a host's UID & GID per env vars which are then used for the container-internal ors user, and default to 0, ie root:

user: "${ORS_UID:-0}:${ORS_GID:-0}"

The idea behind that is usually that you'd like to avoid having to use sudo on the host for whatever the container creates and is mapped back to the host (plus the docker internal user is not root which is also nice when exposing as host volumes). However, I don't think that was ever properly working and my attempt to make it work wasn't very successful.

Anyways, you can take whatever you think is nice from this PR and trash the rest. I'm only using this image to test some of our routing py/js libs and I'm happy with our custom image, likely for years to come.

@MichaelsJP MichaelsJP self-assigned this Jan 30, 2023
@MichaelsJP
Copy link
Member

@nilsnolde Thanks for your contribution. More than welcome. I left you some comments. Just leave me a little comment if you want to provide the changes, else we'd happily just take your commits and do the rest ourselves. :)

@MichaelsJP
Copy link
Member

Cherrypicked your commits into a new branch and made a new PR: #1272

Will be merged soon.

@MichaelsJP MichaelsJP closed this Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multi-stage ORS image
3 participants