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

Unset singleuser.cmd, previously jupyterhub-singleuser, to instead rely on the image's CMD by default #2449

Merged
merged 2 commits into from Oct 25, 2021

Conversation

minrk
Copy link
Member

@minrk minrk commented Oct 21, 2021

As discussed in #2386

Leaving singleuser.cmd unset means relying on the image for the default command to run. This matches the default in KubeSpawner itself, which sets the default cmd to None, which is interpreted as using the CMD form the image definition. This simplifies things as there are now only two places for the command-to-run to come from:

  1. the image itself (chosen by the user), or
  2. singleuser.cmd in the user's values.yaml

In 1.x, the chart's own default was jupyterhub-singleuser

  • Pro: jupyterhub-singleuser means it will always launch something that works without additional user config
  • Con: If the image does have a custom CMD, it must be duplicated into singleuser.cmd because the image has been overridden by the chart defaults

The main con of the change is that one line of config is required for images that don't have a CMD that will launch jupyterhub-singleuser, which seems fine.

closes #776 which should already be closed by the upgrade to jupyterhub 2.0, which launches lab by default (if available) itself in jupyterhub-singleuser.

This is a reissue of #2138 which was withdrawn from 1.x because of the complications around passing cli args by default. The "full command" to launch issingleuser.cmd + Spawner.get_args(). This isn't an issue for DockerSpawner, which can inspect the image and extract its CMD for the default value if unspecified, KubeSpawner cannot assume this capability. So what it does is ignore cli args if the image's CMD is used, meaning singleuser.cmd must be specified if KubeSpawner is to pass any CLI args. This was a deal breaker, because JupyterHub 1.x sets common options like ip, port, and defaultUrl via CLI args. JupyterHub 2.0 promises to never communicate with itself via cli args to jupyterhub-singleuser, only environment variables. This makes it much safer to ignore cli args in common cases, as CLI args will always come from user config, which can in turn specify singleuser.cmd to match.

Removes inconsistent defaults across z2jh, kubespawner, image.

KubeSpawner already sets `cmd = None` by default,
which means using the CMD from the image.

With JupyterHub 2.0, that means lab by default, running on jupyter-server.
Now that we respect the image by default,
more custom images will need `singleuser.cmd` to be set.
@minrk minrk added the breaking label Oct 21, 2021
@minrk minrk mentioned this pull request Oct 21, 2021
15 tasks
@consideRatio
Copy link
Member

consideRatio commented Oct 21, 2021

@minrk does this mean that https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/main/images/singleuser-sample/Dockerfile should be updated to set CMD to jupyterhub-singleuser then? We are deriving it from jupyter/docker-stacks images that installs jupyterhub but doesn't startup as if jupyterhub is assumed.

From jupyter/docker-stacks' base-notebook Dockerfile

# Configure container startup
ENTRYPOINT ["tini", "-g", "--"]
CMD ["start-notebook.sh"]

@minrk
Copy link
Member Author

minrk commented Oct 21, 2021

@consideRatio not necessary, because start-notebook.sh already calls start-singleuser.sh if JupyterHub environment variables are detected.

Comment on lines +246 to +250
# set the default command of the image,
# if the parent image will not launch a jupyterhub singleuser server.
# The JupyterHub "Docker stacks" do not need to be overridden.
# Set either here or in `singleuser.cmd` in your values.yaml
# CMD ["jupyterhub-singleuser"]
Copy link
Member

@consideRatio consideRatio Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this to be such a tricky subject that motivates some thorough explanation, if not to help us maintainers get less questions to answer manually, and to help questions be raised where they belong (z2jh / jupyterhub / docker-stacks).

Suggested change
# set the default command of the image,
# if the parent image will not launch a jupyterhub singleuser server.
# The JupyterHub "Docker stacks" do not need to be overridden.
# Set either here or in `singleuser.cmd` in your values.yaml
# CMD ["jupyterhub-singleuser"]
# NOTE: JupyterHub's user servers must in the end start the jupyterhub-singleuser
# script, but there are options on how to go about it.
#
# 1. Let the image define a ENTRYPOINT / CMD combination that results in
# jupyterhub-singleuser being started.
#
# The https://github.com/jupyter/docker-stacks derived images, such as
# jupyter/minimal-notebook, are already already configured to start
# jupyterhub-singleuser via another startup script declared in
# their Dockerfile's CMD field.
#
# If you write a Dockerfile from scratch, install the jupyterhub pip
# package or the jupyterhub-base conda-forge package and then declare
# CMD like this:
#
# CMD ["jupyterhub-singleuser"]
#
# 2. Let the Helm chart configuration override the CMD part of what was
# defined in the image via `singleuser.cmd`.

Copy link
Member Author

@minrk minrk Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good representation of this information, but too much for an inline comment in the sample Dockerfile (this one comment would be the majority of the whole Dockerfile). Can you move this to the "setting the command" section I added below and add a 'see below for details' type comment here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! I want to get this merged, I can do that in a separate PR! I opened #2452 to represent this for later.

@consideRatio consideRatio merged commit 6fba03e into jupyterhub:main Oct 25, 2021
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Oct 25, 2021
@consideRatio
Copy link
Member

Thanks @minrk and @manics for pushing through handling this complexity!!! ❤️ 🎉

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/cannot-use-sudo-have-root-access-using-jupyterhub-with-kubernetes/12548/5

@consideRatio
Copy link
Member

If someone wants to help with changelog writing, writing up a changelog for this is especially complicated in my mind. This relates to that far more people will now experience the bug that default_url etc are now going to be silently ignored. See jupyterhub/kubespawner#493.

@consideRatio consideRatio changed the title use image for singleuser.cmd by default Unset singleuser.cmd, previously jupyterhub-singleuser, to instead rely on the image's CMD by default Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JupyterLab - the default?
4 participants