fix(execd): skip chmod/chown for pre-existing dirs in MakeDir#1035
fix(execd): skip chmod/chown for pre-existing dirs in MakeDir#1035ashishpatel26 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
When createDirectories is called on a path whose parent components already exist (e.g. /tmp), MakeDir was unconditionally calling ChmodFile on the target after os.MkdirAll, even if the directory was not newly created. For system directories the process does not own this causes 'chmod: operation not permitted'. Fix: stat the target before MkdirAll; if it already exists, skip ChmodFile entirely. Only newly-created directories receive the requested permission. This matches POSIX mkdir -p --mode semantics. Fixes opensandbox-group#1024
ashishpatel26
requested review from
Generalwin,
Pangjiping,
hittyt,
jwx0925 and
ninan-nn
as code owners
|
Changed directories: components、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.
This PR improves Kubernetes volume handling (especially PVC readonly behavior and RO/RW mixing) and adjusts directory creation utilities to avoid permission/ownership changes on pre-existing directories.
Changes:
- Update
apply_volumes_to_pod_specto key PVC volumes by(claim_name, read_only)and propagatereadOnlyinto the PVC volume spec when needed. - Add/extend tests to validate PVC readonly behavior, subPath forwarding, and mixed RO/RW mounts for the same PVC.
- Change
MakeDir(Unix + Windows) to skip chmod/chown when the target directory already exists, with new tests for idempotency.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/opensandbox_server/services/k8s/volume_helper.py | Support separate PVC entries for RO vs RW and set PVC spec readOnly; adjust logging. |
| server/tests/k8s/test_volume_helper.py | New unit tests covering PVC readOnly, mixed RO/RW behavior, host readonly, conflicts, and subPath. |
| server/tests/k8s/test_batchsandbox_provider.py | Extend provider test to assert PVC volume spec has readOnly: true. |
| components/execd/pkg/web/controller/utils.go | Skip chmod/chown when MakeDir target already exists. |
| components/execd/pkg/web/controller/utils_windows.go | Same MakeDir behavior change for Windows build. |
| components/execd/pkg/web/controller/utils_test.go | Add tests for MakeDir creation and idempotency behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -73,7 +80,9 @@ def apply_volumes_to_pod_spec( | |||
| mounts.append(mount) | |||
|
|
|||
| 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, | |||
| ) | |||
| 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] = {} |
| // Record whether the target directory already exists before MkdirAll so | ||
| // we can skip chmod/chown on pre-existing directories (including system | ||
| // dirs like /tmp that the sandbox user does not own). | ||
| _, statErr := os.Stat(abs) | ||
| alreadyExisted := statErr == nil | ||
|
|
||
| if err = os.MkdirAll(abs, os.ModePerm); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if alreadyExisted { | ||
| return nil | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbdd752b0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pvc_key = (pvc_claim_name, vol.read_only) | ||
|
|
||
| if pvc_key not in pvc_to_volume_name: |
There was a problem hiding this comment.
When a sandbox request mounts the same PVC once read-only and once read-write, including vol.read_only in this cache key bypasses the existing claim de-duplication and appends two persistentVolumeClaim volumes for the same claim. The existing test_apply_volumes_to_pod_spec_same_pvc_multiple_mounts documents that this de-duplication avoids CSI driver issues from duplicate PVC volume definitions; this mixed RO/RW case now reintroduces that failure mode on those clusters. Keep a single PVC volume for the claim and rely on per-volumeMount readOnly (or reject mixed modes) instead of emitting duplicate claim entries.
Useful? React with 👍 / 👎.
|
Duplicate of #1025 |
Summary
MakeDirincomponents/execd/pkg/web/controller/utils.go(and the Windows counterpartutils_windows.go) unconditionally calledChmodFileafteros.MkdirAll, even when the target directory already existed before the call./tmp,/var), this causedchmod: operation not permitted, breakingcreateDirectoriesfor any path whose components already exist.os.MkdirAll; if it already existed, return early without callingChmodFile. Only newly-created directories receive the requested permission. This matches POSIXmkdir -p --modesemantics.Fixes #1024.
Test plan
TestMakeDirCreatesNewDirectory— new dir is created successfullyTestMakeDirIsIdempotentOnExistingDirectory— callingMakeDiron a pre-existing dir returns no error and does not alter its permissionsTestMakeDirCreatesNestedDirectoriesWithoutChmodingParents— creating/parent/childwhen/parentalready exists does not chmod/parentgo test ./pkg/web/controller/...): 67/67