fix(k8s): propagate readOnly flag to PVC volume spec in pod manifest#1033
fix(k8s): propagate readOnly flag to PVC volume spec in pod manifest#1033ashishpatel26 wants to merge 2 commits into
Conversation
When a Volume with pvc backend and readOnly=True is mounted, only the VolumeMount readOnly field was set. Some CSI drivers (e.g. mountpoint-s3-csi-driver) require readOnly on the PersistentVolumeClaim volume source as well, so the mount remained writable despite the flag. Fix: also set persistentVolumeClaim.readOnly=true in the pod volumes list when the volume is declared read-only. Additionally change the dedup key from claim_name alone to (claim_name, read_only) so the same PVC can be mounted both read-only and read-write within the same pod without one mount silently overriding the other. Fixes opensandbox-group#545
ashishpatel26
requested review from
Generalwin,
Pangjiping,
hittyt,
jwx0925 and
ninan-nn
as code owners
|
Changed directories: server. 📋 Recommended labels (based on changed files):
Other available labels:
💡 Tip: Use |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support and test coverage for correctly handling read-only PVC mounts in Kubernetes pod specs, including allowing the same PVC to be mounted both read-only and read-write.
Changes:
- Update
apply_volumes_to_pod_specto key PVC volume entries by(claim_name, read_only)and propagatereadOnlyinto the PVC volume spec. - Improve structured logging for added volumes.
- Add focused unit tests for PVC/host volume readOnly behavior, conflicts, and subPath forwarding.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| server/opensandbox_server/services/k8s/volume_helper.py | Updates PVC volume mapping to support both RO/RW mounts and sets persistentVolumeClaim.readOnly when applicable. |
| server/tests/k8s/test_volume_helper.py | New unit tests validating PVC/host readOnly propagation, RO+RW PVC behavior, conflicts, and subPath. |
| server/tests/k8s/test_batchsandbox_provider.py | Extends an existing test to assert persistentVolumeClaim.readOnly is set for read-only PVCs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if pvc_key not in pvc_to_volume_name: | ||
| pvc_volume: Dict[str, Any] = { | ||
| "claimName": pvc_claim_name, | ||
| } | ||
| if vol.read_only: | ||
| pvc_volume["readOnly"] = True | ||
| pod_volumes.append({ | ||
| "name": vol_name, | ||
| "persistentVolumeClaim": { | ||
| "claimName": pvc_claim_name, | ||
| }, | ||
| "persistentVolumeClaim": pvc_volume, | ||
| }) | ||
| pvc_to_volume_name[pvc_claim_name] = vol_name | ||
| pvc_to_volume_name[pvc_key] = vol_name | ||
| existing_volume_names.add(vol_name) | ||
|
|
||
| mount = { | ||
| "name": pvc_to_volume_name[pvc_claim_name], | ||
| "name": pvc_to_volume_name[pvc_key], | ||
| "mountPath": vol.mount_path, | ||
| "readOnly": vol.read_only, | ||
| } |
| logger.info( | ||
| f"Added PVC volume '{vol_name}' (claim: {pvc_claim_name}) mounted at '{vol.mount_path}' for sandbox" | ||
| "Added PVC volume '%s' (claim: %s, readOnly: %s) " | ||
| "mounted at '%s' for sandbox", | ||
| vol_name, pvc_claim_name, vol.read_only, vol.mount_path, | ||
| ) |
| if pvc_claim_name not in pvc_to_volume_name: | ||
| pvc_key = (pvc_claim_name, vol.read_only) | ||
|
|
||
| if pvc_key not in pvc_to_volume_name: |
| pod_volumes.append({ | ||
| "name": vol_name, | ||
| "persistentVolumeClaim": { | ||
| "claimName": pvc_claim_name, | ||
| }, | ||
| "persistentVolumeClaim": pvc_volume, | ||
| }) | ||
| pvc_to_volume_name[pvc_claim_name] = vol_name | ||
| pvc_to_volume_name[pvc_key] = vol_name | ||
| existing_volume_names.add(vol_name) | ||
|
|
||
| mount = { | ||
| "name": pvc_to_volume_name[pvc_claim_name], | ||
| "name": pvc_to_volume_name[pvc_key], | ||
| "mountPath": vol.mount_path, | ||
| "readOnly": vol.read_only, | ||
| } |
| v.get("name") for v in pod_volumes if isinstance(v, dict) | ||
| } | ||
| # Key: (claim_name, read_only) so the same PVC can be mounted both RO and RW. | ||
| pvc_to_volume_name: Dict[tuple, str] = {} |
| ro_mount = _get_mount(pod_spec, "/mnt/ro") | ||
| rw_mount = _get_mount(pod_spec, "/mnt/rw") | ||
|
|
||
| assert ro_mount["name"] == ro_entry["name"] | ||
| assert ro_mount["readOnly"] is True | ||
| assert rw_mount["name"] == rw_entry["name"] | ||
| assert rw_mount["readOnly"] is False |
|
Thanks for the reviews. Addressing feedback: Copilot – Log emitted for every iteration, not just new entries: Valid. The Copilot – Duplicate Copilot – Type annotation too generic: Valid. Will change Copilot – Missing |
Summary
volume_helper.pysetreadOnlyon theVolumeMountbut not on thepersistentVolumeClaimvolume source in the pod specmountpoint-s3-csi-driverenforce read-only at the volume-source level, so the mount stayed writable even whenreadOnly=Truewas declaredclaim_namealone, so mounting the same PVC as both read-only and read-write in one pod would silently reuse the wrong volume entryChanges
persistentVolumeClaim.readOnly: truein the pod volumes list whenvol.read_onlyisTrueclaim_nameto(claim_name, read_only)to allow independent RO/RW mounts of the same PVCserver/tests/k8s/test_volume_helper.pycovering RW (no readOnly key), RO (both flags set), mixed RO+RW same PVC (two volume entries), host volume RO, subPath, and name conflicttest_create_workload_with_pvc_volume_readonlyto also assertpersistentVolumeClaim.readOnlyTest plan
uv run pytest server/tests/k8s/test_volume_helper.py— 6 new tests, all passuv run pytest server/tests/k8s/test_batchsandbox_provider.py -k readonly— existing test passes with new assertionFixes #545