-
Notifications
You must be signed in to change notification settings - Fork 48
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
Introduce a new rayleigh-devel container to fix UID mapping issues #292
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. To make testing easier I have build and pushed the new docker image to the geodynamics docker hub already. But I agree I would like to see someone using MacOS and Windows testing this before we merge it.
I noticed a few small problems:
|
I tried to fix all the points. There is now a script |
I haven't had time to look at these results in detail but here's what I get on various machines (spread out over multiple comments so I can copy and paste). On ubuntu focal:
and I don't get put into a docker container at all, which is odd as if I then run:
I am in the docker container and all looks good. |
On my mac (macOS 10.15 Catalina), I get:
So some warnings/errors and something presumably related to my .bashrc that docker is now trying to interpret because we've mounted all of $HOME. But I do at least end up in a running docker container. |
It looks like the environment variables |
The error occurs because the group exists already locally. This is because macOS uses low GIDs for the users, which are system GIDs on Linux. I changed the script to only create the group if it doesn't exist already. |
Not sure if this is a sufficient test for that but this would suggest that isn't the reason:
|
Realize this is moving target now and I should pull the latest changes but I've just got the older version sort of working on windows. Haven't worked out how to run the script directly yet so am just copying and pasting the command. One thing is clear so far - environment variables (especially HOME) need to be in curly brackets. More on this when I figure out how to run it through the script. |
With curly brackets, copy and pasting the command, and an old version of this pr, I get (on windows 10 pro):
So I do at least seem to end up in the docker container. |
I don't know much about the PowerShell syntax, but be probably need a separate script for PS. Windows doesn't know the |
OK, interesting. Does it work with the new version of the script calling |
Yes, that seems to work beautifully now on ubuntu. On mac I still get some errors caused by the docker container sourcing my .bash_profile or .bashrc, which may be a problem with mounting the entirety of $HOME more generally. I'm trying windows through a bash shell but still not having much luck there. It would be nice if it were possible to get the docker-devel script to recognize when id isn't available and then have it fall back on not setting the environment and launching as root as you suggested. |
This uses the buildenv as a basis, but creates a user in the container to make sure the UID/GID in the mounted home directory matches with the user in the container. Before that files the container created files owned by root on Linux host systems.
This now displays a warning if the container is run without the necessary environemnt variables.
Containers apparently do not inherit the environment from their base containers.
This had a different behavior depending on the shell being used to call the script.
In particular on macOS with low user GIDs the group might exist already.
This prevents warnings and errors on systems with incompatible commands in ~/.bashrc file.
The script is already doing that. It's just that the container on Docker Hub hasn't been updated to the latest version. If the any of the calls to ID doesn't work, the environment variable will be empty and the container will start as root, displaying a warning. Maybe you can build the container yourself for testing. As to the errors caused by options in your |
This also add the NOUIDWARN option that suppressed the warning when running as root in the container. This is not an issue on Windows, so it is better not to confuse the users.
I added a batch script |
Last time I checked all worked as expected for me. Maybe someone with a Windows system should give it another spin, especially the |
Apologies for the long silence on this. On linux (ubuntu 20.04 focal) this still works beautifully:
On my mac (macOS 11.6 big sur) I still get an error about the group id (low numbers indeed... gid=20 and I end up in the group
On windows (windows 10 pro, using power shell) I don't get in at all:
As linux is the main target of this -- docker desktop on macs already deals with uid issues and I'm not aware of any windows users -- I'd be happy to approve this if there aren't quick fixes. Though possibly then we should drop the .bat version in that case and be explicit that windows isn't really supported? |
I'm good with Cian's suggestion. Though to be fair, I ONLY use Docker in Windows :) That said, I'm still experimenting with a Rayleigh environment build. |
Ah, oops! Didn't know that :-) Does |
Just for clarification |
Oh, I'm sorry @tukss , hadn't realized that. Running in the command prompt I get the same error:
|
I'm a little rusty with Docker, so it might be a bit before I can try this out on my laptop. It's really only something I use in a pinch because this particular laptop isn't set up for dual boot. Unless @tukss can fix the .bat file, I think it would be fine to remove it for now and add Windows Docker support after 1.0. |
Ah right. |
Some progress there, I do now get into a docker container but I'm root and there's an error along the way regarding the group and user add commands...
Similar story in both the command prompt and power shell. |
Thanks for testing. Looking at the relevant If the problem persists after rebuilding, please send me the output of |
On Windows the expected behavior is that you are root inside the container. That doesn't affect the UID of the files created in your home directory, so it shouldn't be a problem. |
@cianwilson I don't want to rush you. Just checking if GitHub actually sent you my previous comments on this PR. |
No worries about rushing me. I could do with a little bit of that :-) I saw the comments just haven't had a chance to sit in front of that computer yet. Will do tonight. Thanks! |
Is the current version of the docker container you want me to build in this branch or should I build it off master? Building off the one in this branch leads to an error. My complete workflow (in command prompt on windows):
I confirmed that that docker build worked on an ubuntu box. |
Scratch that. I see there is no |
OK. That is really strange. It tells us |
Can you try running it like this? Effectively you're replacing If this works, then it's just the build process on Windows, that's broken. |
Yup, looks like that worked...
So, a new docker image just needs to be built and pushed to dockerhub? |
Yes, I think pushing the new image will solve the problem. And independently of this PR we should find out why the build on Windows doesn't include the |
Great! Happy to test again when that happens but will be offline for a few days until at least next week. |
@gassmoeller What's our normal procedure here? Do we first merge to master and update the image on Docker Hub then or the other way round? Thank you! |
The order doesnt really matter in this case. Ultimately we want to set it up to build the image automatically once a PR is merged. For now I have built it locally and pushed to geodynamics/rayleigh-devel-bionic. Give it a try and let me know. |
Yup, that seems to work now...
Thanks @gassmoeller and @tukss ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
This uses the buildenv as a basis, but creates a user in the container
to make sure the UID/GID in the mounted home directory matches with the
user in the container. Before that files the container created files
owned by root on Linux host systems.
There is an alternative using the bindfs FUSE filesystem, but that comes with a host of other problems involving the use of FUSE inside a container.
For the commands in the documentation to work we still need to upload this new container to the geodynamics Docker hub account. For testing you can just build the container yourself.
It would be good if people could if this still works on macOS and Windows.