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

Add user in script #16

Closed
wants to merge 4 commits into from
Closed

Add user in script #16

wants to merge 4 commits into from

Conversation

costerwi
Copy link
Contributor

I wanted to be able to specify the UID of the jovyan user so I could mount host volumes and have the ownership make sense. I found this was most flexible if the useradd happened inside a CMD script instead of being built into the docker image.

The start-notebook.sh script changes the write permission of the conda root environment so there is no need to swap USERs back-and-forth in the Dockerfiles.

It was also convenient to launch the jupyter notebook at the end of the script, rather than using supervisord. The server may still be restarted automatically using Docker restart policies.

@parente
Copy link
Member

parente commented Aug 26, 2015

Thanks for the UID configuration change. @rgbkrk had a question about supervisord as well back on PR #1. I responded there about why I thought it was important to have it #1 (comment). If there's a way to handle those two concerns with a pure script plus Docker commands, I'm all for it.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 26, 2015

Oooooh, thank you for the UID specifying. We've ran into that trouble before with JupyterHub deployments which had lots of users (and we were mounting from NFS).

/cc @jhamrick

@jhamrick
Copy link
Member

Yes, +1 on the uid specifying!

Used to relaunch notebook and collect logging data.
@costerwi
Copy link
Contributor Author

I think your first concern about preserving a modified jupyter config between container restarts might be handled by assigning it to a persistent docker volume.

The absence of init within Docker is a complicated problem, thanks for the reference! It seems that supervisord is often used to launch multiple programs within containers. It addresses some of the init problems but opinions are mixed on its reaping capability and this isn't a recommended use in the supervisord manual.

However, I don't know anything better and that's not the main thrust of this PR so I've restored supervisord to PID1 and parameterized its config file a little more. I couldn't get it to expand user=%(ENV_NB_USER)s for some reason so you must use jovyan, as is currently the case anyway.

@parente
Copy link
Member

parente commented Aug 28, 2015

Perfect! You beat me to the punch about separating the UID work out from the init process work. I'll run a few tests here on the sub-images and then merge it in.

I'm happy to open a separate issue about supervisord. Maybe we can find a solution over time. For what it's worth, it's specified as a CMD in the Dockerfile so it's entirely possible to override it and just launch the notebook server as part of the docker run command, like:

docker run -P jupyter/minimal-notebook jupyter notebook --ip='*'

@parente
Copy link
Member

parente commented Aug 29, 2015

Some problems:

  • Their Dockerfiles switch back and forth between root and jovyan. No longer needed. Can remove the USER switching.
  • Kernel spec installation winds up putting the spec files in the root user home directory since that's the active user at the time of build. Need to install in system path, not home path.
  • I know of folks already using these images. A switch of the working directory from /home/jovyan/work to /notebooks is going to be a surprise for anyone that pulls the new images. Can't easily resolve this by keeping the path as-is because if it's host mounted with docker run -v the script will fail to copy the skeleton files for the new jovyan user on start. We'll have to break compat, but that's probably OK given how new this repo is and how many issues still getting ironed out.
  • @rgbkrk wanted to try basing the docker-demo-image on minimal-notebook. The above path change will also require a change in that image's assumptions.

I pushed a copy of your PR with one additional commit to parente@9562116. It has all the above updates except but retains /home/jovyan/work as the notebook path. However, it's currently busted when volume mounted. I'm looking for a way around that.

@parente
Copy link
Member

parente commented Aug 29, 2015

Also, the start-notebook.sh script re-runs on container stop/start. Takes a bit to recursively set all the perms on the conda dir each time. Wonder if there's a way around that too.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 29, 2015

Ah, whoops I should have read feedback here before #21!

@parente
Copy link
Member

parente commented Aug 30, 2015

Merged in #21. Thanks @costerwi

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.

4 participants