Update systemdspawner from version 0.17.* to >=1.0.1,<2 #915
Conversation
consideRatio
changed the title
consideRatio
force-pushed
the
pr/update-systemdspawner
branch
from
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
journalctland 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.
consideRatio
force-pushed
the
pr/update-systemdspawner
branch
from
2db8ca1
to
c5eae33
Compare
consideRatio
changed the title
|
Tests are passing, this works now! Thank you @minrk for working this in systemdspawner! |
The breaking changes in SystemdSpawner are listed as:
I think only the
disable_user_sudo=Trueas 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.