feat: configure shared memory for singleuser pods - #246
Keerthirajan58 wants to merge 3 commits into
Conversation
pmeier
left a comment
There was a problem hiding this comment.
Thanks @Keerthirajan58 for the PR. I've added some high level comments that we need to address before going any further.
| # avoids the container runtime's 64 MiB default, which can make Ray fall back | ||
| # to disk and can crash multiprocessing PyTorch DataLoader workers. | ||
| sharedMemory: | ||
| enabled: true |
There was a problem hiding this comment.
When would someone disable this?
There was a problem hiding this comment.
An operator might already define a /dev/shm mount through their profile's
volume overrides. Turning this off lets them keep that mount without the chart
replacing it. I've added a test for that case and explained it in the docs.
Without a custom mount, disabling it leaves the runtime's default in place.
| # RWO claim, which the pod-affinity rule below keeps to a single node. | ||
| c.KubeSpawner.pvc_name_template = "claim-{username}" | ||
| c.KubeSpawner.volumes = [ | ||
| c.KubeSpawner.volumes = list(get_config("singleuser.extraVolumes", []) or []) |
There was a problem hiding this comment.
Where does the singleuser.extraVolumes key come from? I can't any mention of it in the values. I don't understand why we can't just add the /dev/shm volume to the existing list. Same for the volume mounts below.
There was a problem hiding this comment.
You're right, adding the shared-memory entries to the existing lists is enough
here. I checked the pinned JupyterHub chart and also found that I had the key
wrong: it's jupyterhub.singleuser.storage.extraVolumes. I've removed that extra
handling and now append the volume and mount as you suggested.
|
@pmeier, I've pushed the changes discussed above and brought the branch up to date I also tested it in kind. The notebook wrote 128 MiB successfully, while the same |
The small
/dev/shmdescribed in #54 can cause PyTorch DataLoader workers tocrash and Ray to fall back to disk. This adds a memory-backed mount for singleuser
pods, with an 8 GiB cap that operators can change through
singleuser.sharedMemory.Profiles can set their own limit with
shm_size_limit.I've kept the existing home and config volumes unchanged and added the
shared-memory entries to those lists. The
enabledswitch lets operators keepthe runtime default or use a mount they already define through profile overrides.
The profile handling also avoids adding duplicate entries when KubeSpawner applies
the overrides more than once.
Testing
I ran the shared-memory test in kind using the chart's notebook image. It
successfully wrote a 128 MiB shared-memory segment. A separate pod using the same
image without the new mount had the usual 64 MiB
/dev/shmand failed the samewrite with SIGBUS.
That run caught a mistake in my original test: I expected the filesystem to be
exactly 8 GiB, but Kubernetes capped it at the smaller node's memory capacity.
The test now checks the configured cap separately from the filesystem capacity
and still requires the full 128 MiB write.
All 184 unit tests pass on Python 3.10, including checks that render the Helm
values and generate pods through KubeSpawner. Ruff, Helm lint, the nine docs
tests, the docs build, and internal links also pass.
For the local kind run, I disabled unrelated NFS shared storage because its test
image is AMD64-only and this machine is ARM64. I ran only the shared-memory e2e
test locally; the full suite still needs to run in CI.
Closes #54.