JupyterHub: update jupyterhub component default version to 5.2.1
#493
Conversation
github-actions
bot
added
component/jupyterhub
There was a problem hiding this comment.
The changelog should describe that changing to this version can break everything depending on the OS the server is running on, since the whole Jenkins CI fiasco originates from this.
Link to Ouranosinc/PAVICS-e2e-workflow-tests#137
It is not necessarily a seamless change.
| c.Authenticator.blacklist = blocked_users # v0.9+ | ||
| c.Authenticator.blocked_users = blocked_users # v1.2+ | ||
|
|
||
| c.Authenticator.allow_all = True # v5.0+ |
There was a problem hiding this comment.
This combination is extremely confusing and prone to misinterpretation.
Please provide the doc reference and some comment.
Even after reading the doc details, it feels config to "allow all" after an explicit set of blocked users.
There was a problem hiding this comment.
This is to maintain the previous behaviour which is what is mentioned a few times both in the link you provided and the upgrade guide (https://jupyterhub.readthedocs.io/en/latest/howto/upgrading-v5.html#authenticator-allow-all-and-allow-existing-users).
We use Magpie as the source of truth for who our users are so we do not want jupyterhub to filter/block those users further.
Can you clarify what the confusion is please? That would help me draft a better comment to help.
There was a problem hiding this comment.
Just curious, I vaguely remember in Magpie, there was a way to only allow Jupyterhub login to some subset of users (using group membership) so not all authenticated users are automatically granted Jupyterhub access. Is that true or I am just imagining things?
There was a problem hiding this comment.
Yes that is possible. But again, that's using Magpie for the source of truth. This is just saying that any user that can authenticate should be allowed.
This is the current behaviour, the configuration changes for JupyterHub just means that we have to be explicit about it now.
There was a problem hiding this comment.
I agree with all mentioned points.
It is potentially confusing for new users since the Magpie auth is not that obvious from that place (c.MagpieAuthenticator is not the same base as c.Authenticator, so it's a stretch to figure out they work together, not to say it is way higher in the config).
Also, reading c.Authenticator.blocked_users followed by c.Authenticator.allow_all immediately raised a "hold on, what, why?" flag for me. I had to deep-dive into Jupyterhub docs to (partially) understand their convoluted management of pre/post user-login and accessible operations. The naming of the parameters are not intuitive. "Allow all" in the context of auth would be understood by pretty much anyone as open access, but "access all [what]" (users, resources, actions, notebooks, all-of-the-above) is not obvious.
There was a problem hiding this comment.
Allow all" in the context of auth would be understood by pretty much anyone as open access
But we are not the one controlling and choosing the naming. Adding more documentation comments to help the reader?
There was a problem hiding this comment.
Exactly. This is why I ask for comments to mitigate the bad naming misinterpretation.
mishaschwartz
changed the title
jupyterhub component default version to [5.2.1](https://github.com/Ouranosinc/jupyterhub/releases/tag/5.2.1-20241114)jupyterhub component default version to 5.2.1
There was a problem hiding this comment.
I'd like the breaking not backward compatible change standout much better in the changelog.
Also given this is a major version upgrade, could you also refresh this file https://github.com/bird-house/birdhouse-deploy/blob/master/birdhouse/components/jupyterhub/jupyterhub_config.py.original_generated by using jupyterhub --generate-config -f /jupyterhub_config.py.original_generated.
@fmigneault You are confusing the Jupyter env running, which breaks recently, and this one, which is JupyterHub, which only launch and manage the runtime. I don't think the previous fiasco will happen again here. That said, for me this is still a backward-incompatible change since the sys admin has to manually alter this |
Why do we need to keep a copy of this in our source code? Why not just add a comment with a link to the documentation like we do everywhere else? https://jupyterhub.readthedocs.io/en/5.2.1/reference/config-reference.html#jupyterhub-configuration |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3037/Result ❌ FAILUREBIRDHOUSE_DEPLOY_BRANCH : upgrade-jupyterhub-v5 DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-91.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1750/NOTEBOOK TEST RESULTS |
Ah true, I got to explain the reasoning: to have a nice diff of what changed directly in our code. If just for reference, then yes a link is fine. But the reason for the commit is to have the diff. Things like |
So do you want the settings in jupyterhub_config.py.original_generated to be in the same order as jupyterhub_config.py.template? Otherwise a diff between these two files won't be very useful |
|
@mishaschwartz @tlvu |
|
Given that the jupyter version can be overridden, the original config that got generated a while ago might not even but up to date with latest definitions as well. I'm in favor to drop it since it doesn't add relevant information, which can be retrieved by commits anyway if need be. |
|
run tests |
Sorry I meant a diff of
Same could be said for the actual config. We can override the Jupyter image version but it still has to be compatible with the config in
Open to change that to a commit if shows cleanly and easily the diff to the configs with each version. Otherwise, it's more direct and visual to see the original config change, then our "adaptation" to follow it, all in the same PR. |
This is fantastic. Will you preserve the previous login for each of us to go check the full build logs and to be able to manually launch some builds? Same hostname as before? |
Same hostname. It's already migrated. Users are not yet migrated, but we are aware of it and planing to address it (@perronld FYI) There are still some errors happening that we must investigate, such as the report not commented automatically on this PR, and some weird partial test outputs. This is the results we got from #493 (comment) so far: |
|
@tlvu I'm investigating the weird partial output from pytest + nbval + xdist and landed on computationalmodelling/nbval#204 😅 Are you seeing a similar result to #493 (comment) on your end? Our Jenkins logs clearly shows that all tests notebooks received the substitution to use the test instance URI, and the |
|
@tlvu @fmigneault |
|
test |
Comments from Jenkins to github should now be fixed! |
Overview
This implements all changes between JupyterHub version
4.1.6 and 5.2.1.
This update requires the following manual upgrade steps:
c.DockerSpawner.image_whitelistconfig option in theJUPYTERHUB_ENABLE_MULTI_NOTEBOOKSenvironnment variable. Changec.DockerSpawner.image_whitelistto
c.DockerSpawner.allowed_images.jupyterhub_data_persistencevolume if you're not storing any custominformation there and if you haven't manually set
c.Authenticator.allow_alltoFalse(ensure thatthe stack is stopped and all jupyterlab containers have been stopped and removed first).
If you have changed any of the default
jupyterhubsettings you may need to consult the JupyterHub upgrade guide to see if any of those settings have been changed.Changes
Non-breaking changes
Breaking changes
Related Issue / Discussion
Additional Information
CI Operations
birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false