MAAP staging: Define group shared folders volumes and volume mounts as dict #6102
Conversation
|
Merging this PR will trigger the following deployment actions. Support deploymentsNo support upgrades will be triggered Staging deployments
Production deployments
|
sunu
force-pushed
the
maap-staging-group-shared-directories-2
branch
2 times, most recently
from
e218550 to
25eae5a
Compare
| # so that the user can't see the contents of other groups' folders | ||
| # that the user is not a member of | ||
| name: shared-group-placeholder | ||
| mountPath: /home/jovyan/shared-group |
There was a problem hiding this comment.
Not sure if this the best approach to fix visibility of stale group shared folders. Please let me know if there's a better way to do this.
The concern is: If a user previously had a shared group folder mounted (e.g., to /home/jovyan/shared-group/<group-folder>), their underlying home PVC (<home-pvc>/<user>/) might now contain a persistent directory at shared-group/<group-folder> that was created by Kubernetes as a mount point.
Later, if the user is no longer in that group, the specific mount for the actual shared data is removed. However, the shared-group/<group-folder> directory within their personal PVC home will still be visible (as part of the main /home/jovyan mount). This can lead to confusion, showing outdated or empty folders for groups they no longer have access to.
sunu
force-pushed
the
maap-staging-group-shared-directories-2
branch
from
74a6779 to
e008806
Compare
|
Edit: I misread that -- |
|
@sunu hmm, can this be intentionally set to an empty object or something else to test? |
|
@yuvipanda yea, my bad, I misread the schema. |
The upstream JupyterHub Helm chart has added support for dict values for volumes and volume mounts in version 4.3.0. This allows us to easily override the config values we need to without having to specify the entire list again.
|
I've updated the PR to take advantage of support for dict values in z2jh 4.3.0. Converting to draft so that we don't merge it before #6915 is fixed and z2jh 4.3.0 is rolled out across all hubs. |
sunu
changed the title
|
note to self: Add docs from #6070 to this PR and address any pending feedback from that PR here. |
|
@sunu, we can now upgrade the z2jh version of just a hub. Do you want to give it a go and try it out on maap so you're not blocked on #6915 ? The docs are at https://infrastructure.2i2c.org/howto/upgrade-cluster/sub-chart-version/#bumping-the-version-of-a-helm-sub-chart-for-a-specific-hub Let me know how it goes and if you have any feedback about it or the docs 🚀 |
There was a problem hiding this comment.
@sunu, I've tried to deployed this to another hub where we need to deploy per groups shared directories.
When using the latest z2jh 4.3.1, I'm getting the following warnings when coalescing the charts.
coalesce.go:223: warning: destination for basehub.jupyterhub.singleuser.storage.extraVolumeMounts is a table. Ignoring non-table value ([map[mountPath:/home/jovyan/shared name:home readOnly:true subPath:_shared] map[mountPath:/dev/shm name:dev-shm] map[mountPath:/home/rstudio name:home subPath:{escaped_username}] map[mountPath:/home/rstudio/shared name:home readOnly:true subPath:_shared]])
coalesce.go:223: warning: destination for basehub.jupyterhub.singleuser.storage.extraVolumes is a table. Ignoring non-table value ([map[emptyDir:map[medium:Memory] name:dev-shm]])
coalesce.go:220: warning: cannot overwrite table with non table for basehub.jupyterhub.singleuser.storage.extraVolumeMounts (map[])
coalesce.go:220: warning: cannot overwrite table with non table for basehub.jupyterhub.singleuser.storage.extraVolumes (map[])
coalesce.go:223: warning: destination for basehub.jupyterhub.singleuser.storage.extraVolumeMounts is a table. Ignoring non-table value ([map[mountPath:/home/jovyan/shared name:home readOnly:true subPath:_shared] map[mountPath:/dev/shm name:dev-shm] map[mountPath:/home/rstudio name:home subPath:{escaped_username}] map[mountPath:/home/rstudio/shared name:home readOnly:true subPath:_shared]])
coalesce.go:223: warning: destination for basehub.jupyterhub.singleuser.storage.extraVolumes is a table. Ignoring non-table value ([map[emptyDir:map[medium:Memory] name:dev-shm]])
I tracked this down as coming from https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/0a3b1bad94d3e0fd31c3d1e207588b7e81bda4aa/jupyterhub/values.yaml#L412-L413, where we default them to being the empty map.
And we're defining them as arrays in our sub-chart, in basehub/values.yaml. And even though we're re-defining as a map in the hub's own values.yaml file, the coalescence happens before the merge, so it still errors out.
This means that the change in z2jh is actually a breaking change and it doesn't work for setups that still use arrays. Wondering is using null is acceptable to keep it backwards compatible.
Either way, we need to at least mark it as a breaking change in the changelog of z2jh I believe.
Cc-ing @agoose77 because we're reviewed jupyterhub/zero-to-jupyterhub-k8s#3690 together and haven't catch this.
|
@GeorgianaElena from a cursory glance, is the problem that the chart coalescence happens first, and the base-hub has a different type (array) to the hub values in this PR (dict)? If so, I would say that the change in the z2jh chart wasn't a breaking change, but it does mean that we can't roll it out unless we do it for all of our hubs at the same time (so that we can change the base-hub itself). The z2jh PR is a breaking change for people modifying the |
Kind of. It'd complain of any type diff along the config
Yes, exactly my concern. I wonder if porting the basehub config into the hub's values file as a map would work. When I tried it, it still threw coalescence warnings, but I haven't checked if it actually did the trick.
I am confused. Isn't it an issue that in z2jh schema we say that For me this reads as: It's true it won't error out when rendering the templates, but it leads to an unexpected behaviour. |
|
From a quick look, I think I agree with @GeorgianaElena that Even though the schema description says it can be either I think we need to set the default to |
I missed the z2jh chart values.yaml. Yes! |
4 participants
Trying out suggestions from #6010 (comment)Haven't tried this out live yet, because I'm having some trouble logging in to the MAAP staging hub.But if I understand correctly, we will run into issues without upstream changes because of howextraFilesis handled. This expects theKubeSpawner.volumesandKubeSpawner.volume_mountsto be lists. SimilarlyextraVolumes/extraVolumeMountshandling also expectsKubeSpawner.volumesandKubeSpawner.volume_mountsto be lists.I have tried this out on MAAP staging and it seems to work as expected.
related to NASA-IMPACT/veda-jupyterhub#66
Update
z2jh version 4.3.0 now supports dict based values for all supported fields and this PR now takes advantage of that. I've tested this on MAAP staging hub manually. But to merge this in, we need to wait for #6915 to roll out z2jh 4.3.0 for all hubs.