-
Notifications
You must be signed in to change notification settings - Fork 67
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
basehub: default to nfs.enabled / nfs.pv.enabled to false #3666
basehub: default to nfs.enabled / nfs.pv.enabled to false #3666
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This change is a systematic change, so it can quickly get out of sync and needs to be updated/reviewed/merged in quick succession. I'd like to update this PR to be mergabe once the PR is reviewed in outs outdated form, where focus is on the kind of changes, and the retroactive update is focused on making sure the changes are systematic once again. |
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.
LGTM. Thanks @consideRatio. Ping me after when I need to take another look after the rebase.
e2bdb07
to
605e9f6
Compare
nfs: | ||
enabled: true | ||
enabled: false | ||
dirsizeReporter: | ||
enabled: true | ||
shareCreator: | ||
enabled: true | ||
tolerations: [] | ||
pv: | ||
enabled: true | ||
enabled: false |
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.
Since this switches these from true to false, I'm updating all cluster config explicitly configuring nfs to enable nfs stuff.
Thank you @GeorgianaElena! I've now rebased and verified the change is sound twice by searching for |
Actially I think any mistakes here will be caught by the health check tests, because a user server will be started with storage mounted - storage that can't be mounted unless nfs stuff has been enabled. I'll go for a merge before new hub PRs are being created as they will be soon. |
🎉🎉🎉🎉 Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/8432986231 |
Closes #3654.
We are still not following the best practice of providing a chart (basehub) that works out of the box with its default config, because at least we still have
jupyterhub.singleuser.extraVolumeMounts
andjupyterhub.custom.singleuserAdmin.extraVolumeMounts
that mounts NFS folders even ifnfs.enabled
andnfs.pv.enabled
aren't true, which would make startup fail.This PR does not revert #3656 because I think many clusters will have hubs enabling and configuring
nfs
stuff in a common.values.yaml, while individual hubs may then go back to opting out of it.Overall, this is something I consider an improvement, but it isn't as clearcut as I initially thought.