-
Notifications
You must be signed in to change notification settings - Fork 345
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
Update systemdspawner from version 0.17.* to >=1.0.1,<2 #915
Update systemdspawner from version 0.17.* to >=1.0.1,<2 #915
Conversation
621cb35
to
2db8ca1
Compare
I found logs finally, this is what happens.
Is this a path issue perhaps? Oh! Why is jupyterhub-singleuser started from the hub env and not the user env?
@minrk do you have a guess about this? |
Is the path to the user env definitely passed to the spawner (either using PATH, or passing the full path to jupyterhub-singleuser)? Do the logs contain the SystemdSpawner/UserCreatingSpawner configuration? |
Definitely a PATH issue, because the command should be I believe the fix should be for the Spawner to ensure Can you dump the systemd unit file? |
Hmmm hmmm, the user servers are started as "transient units", not having a unit registered etc i think. I'm not sure how to do things. @minrk the logs was found from #916 in https://github.com/jupyterhub/the-littlest-jupyterhub/actions/runs/5194421002/jobs/9366061173#step:5:319 (not in the "show logs" step, but from the in-the-middle-of-the-test run step where the created user is known) |
I think it's worth adding some debug logging to systemdspawner. I was able to run some local tests to do some debugging, and I believe it comes down to the fact that systemd itself resolves executables to absolute paths before loading EnvironmentFile, so systemd-run asserts this as well:
so I'm not quite sure why it behaves weird instead of failing because the condition isn't met. It also appears to be using the calling process's $PATH, because it's finding the executable in tljh/hub/bin, rather than raising command not found. I Options include:
All of these should be done in systemdspawner. I'll have a look at implementing the first option, which I think makes the most sense. |
fwiw, this is how I did my debug setup: .github/integration-test.py build-image
export BOOTSTRAP_PIP_SPEC=git+https://github.com/jupyterhub/the-littlest-jupyterhub.git@refs/pull/915/merge
.github/integration-test.py run-test basic-tests --bootstrap-pip-spec "$BOOTSTRAP_PIP_SPEC" test_hub.py # (once)
# start an interactive shell in the container
docker exec -it basic-tests bash
# on host, copy systemdspawner into container
docker cp -r . basic-tests:/srv/src/systemdspawner
# back in container
cd srv/src
/opt/hub/bin/pip install -e ./systemdspawner
tljh-config reload hub
/opt/hub/bin/pytest -vsx ./integration-tests/test_hub.py
# check logs
journalctl and repeat the |
Having upgraded systemdspawner to 1.0.0, its configuration option `disable_user_sudo` now defaults to True. This would be a breaking unwanted change for our jupyterhub admin users who are configured with passwordless sudo. Its unlikeley a breaking change for other users, but could be if they are granted sudo rights without being a jupyterhub admin. But, if they are, then they could grant themself such rights anyhow so its reasonable to assume jupyterhub admins only should have sudo rights in a TLJH installation.
2db8ca1
to
c5eae33
Compare
Tests are passing, this works now! Thank you @minrk for working this in systemdspawner! |
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.
👍 assuming it works!
The breaking changes in SystemdSpawner are listed as:
I think only the
disable_user_sudo=True
as default change is the only possibly breaking change. I've added a commit to make admin users get it set to False and non-admin users get it set to True explicitly in TLJH now. This leads to the following breaking change:If a non-admin jupyterhub user has been granted sudo permissions, without being granted jupyterhub admin status, then the user would then not be able to sudo any more.
I suggest that we suggest in the changelog that such users should also be granted jupyterhub admin status. This is something the user could acquire with sudo anyhow.