feat(kubernetes): support PVC subPath driver config

[mjamiv]

Summary

Add Kubernetes driver-config support for mounting existing PVCs into the agent container with optional sub_path values. This gives deployments a narrow driver-owned storage topology for durable per-user PVC data without broad PodSpec overrides.

Related Issue

Fixes #2033

Changes

• Adds driver_config.kubernetes.volumes[] for PVC-backed pod volumes.
• Adds driver_config.kubernetes.containers.agent.volume_mounts[] for agent PVC mounts with optional sub_path.
• Defaults user-supplied PVC volumes and mounts to read-only unless read_only: false is explicit.
• Validates duplicate names, unknown volume references, protected mount paths, invalid sub_path values, and attempts to weaken read-only PVC volumes.
• Skips the default /sandbox workspace PVC injection when an explicit Kubernetes driver-config mount targets a path below /sandbox/.
• Documents the schema and a single-PVC multi-subPath example in the Kubernetes driver README and compute-driver reference docs.

Testing

• mise run pre-commit passes
  • Not completed: this environment initially lacked mise; after installing it, mise run pre-commit refused to run until the repo config was trusted. I did not run mise trust because it persistently changes local trust settings.
  • Ran direct underlying checks instead:
    • cargo fmt --all -- --check
    • cargo test -p openshell-driver-kubernetes
    • cargo clippy -p openshell-driver-kubernetes --all-targets -- -D warnings
    • npx --yes markdownlint-cli2@0.22.0 crates/openshell-driver-kubernetes/README.md docs/reference/sandbox-compute-drivers.mdx
    • UV_CACHE_DIR=.cache/uv uv run python scripts/update_license_headers.py --check
• Unit tests added/updated
  • Added positive validation coverage for a read-write PVC with multiple read-write subPath mounts.
• E2E tests added/updated (if applicable)
  • Not run locally; a k3d or Kubernetes cluster proof with a pre-existing PVC should verify actual subPath mounts and pod recreation behavior.

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)
• Architecture docs updated (if applicable)
  • Published docs and the Kubernetes driver README are updated; no top-level architecture doc was needed for this driver-local config addition.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[austin-shih]

Thanks for this — the caller-owned-PVC-by-claim_name + subPath model is exactly what disposable-pod / durable-per-user-storage deployments need. A few test-scope observations from the perspective of someone planning to run this pattern:

1. Positive validation coverage. The render test exercises a writable PVC, but it goes through sandbox_to_k8s_spec, which doesn't run validate(). The from_template tests are all rejection cases, so there's no happy-path assertion that a writable config is actually accepted. A test that from_template returns Ok for a read-write PVC with several RW subPath mounts (the real per-user shape: workspace, memory, sessions) would lock that path in.

2. Missing-PVC behavior is untested. When a referenced claim_name doesn't exist, behavior currently falls to the kubelet's FailedMount. Worth covering (ideally in e2e) so the failure surfaces clearly rather than as a stuck pod.

3. E2E is where the core claims live. The unit tests prove the pod template renders; they can't prove that a pre-existing PVC actually mounts at the subPaths, that the agent can read/write, that the default /sandbox injection is correctly skipped, and — most importantly — that data persists across pod recreation against the same PVC. The PR notes e2e wasn't run; a k3d-based e2e (pre-create PVC → create sandbox with this driver_config → verify mounts + RW → recreate referencing the same PVC → data persists) would de-risk the durability story.

Happy to help validate — we're deploying this exact pattern and can share e2e findings.

[mjamiv]

Thanks for the test-scope review. I added positive from_template validation coverage for a read-write PVC with multiple read-write sub_path mounts (workspace, memory, sessions) and also normalized the example names in the docs/tests to keep the contribution generic.

I agree the missing-PVC and durability claims need cluster-level coverage: kubelet FailedMount, actual subPath mount behavior, read/write checks, default /sandbox injection being skipped, and data surviving pod recreation all belong in k3d or Kubernetes e2e rather than unit tests.

[elezar]

Why are we redefining what should be shared across drivers (see driver_mounts::RESERVED_MOUNT_TARGETS)? Note that the openshell_core::driver_utils::SUPERVISOR_CONTAINER_* constants added to the list later would already be covered by /opt/openshell which is included in the shared list.

[elezar]

Review: PVC subPath driver config

Thanks for this — the direction is good and it correctly routes subpath/source/target validation through the shared openshell_core::driver_mounts module. Two themes below: code reuse (so the K8s driver doesn't become a fourth divergent copy of mount logic) and implementation issues (a few I'd gate merge on).

Code reuse / overlap with other drivers

All container drivers already converge on openshell_core::driver_mounts (Docker lib.rs:1779-1909, Podman container.rs:546-688), so subpath rules, NUL/empty checks, .. rejection, and the /sandbox + reserved-target base list are already unified. This PR reuses most of that — good. Remaining duplication worth consolidating:

1. path_is_or_under is an exact copy of the private fn in driver_mounts.rs:99-104. Make the shared one pub and import it instead of re-declaring.
2. default_true() is now defined a third time (already in docker/lib.rs:342 and podman/container.rs:136). Along with the #[serde(default = "default_true")] read_only: bool idiom, this belongs in the shared module.
3. Duplicate mount-target detection is identical in three places — Docker (lib.rs:1857-1917), Podman (container.rs:625-681), and now here. A shared validate_unique_mount_targets(&[...], driver_name) would collapse all three.
4. Protected-path list overlaps and risks drift. KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS repeats /etc/openshell, /etc/openshell-tls, /run/netns (already in driver_mounts::RESERVED_MOUNT_TARGETS) and redundantly lists SUPERVISOR_CONTAINER_DIR / SUPERVISOR_CONTAINER_BINARY, both already under /opt/openshell which the shared list covers. Since validate_kubernetes_mount_path calls validate_container_mount_target first, keep only the K8s-unique entries (/var/run/secrets/openshell, /spiffe-workload-api) locally and let the shared list cover the rest — otherwise the two lists will silently diverge.
5. Consider promoting the stricter path checks into the shared validator. validate_kubernetes_mount_path adds rejection of internal empty segments (/a//b), . segments, and surrounding whitespace — checks validate_container_mount_target lacks today (Docker/Podman currently accept /a//b). If these are correctness improvements, hoist them so every driver benefits and the K8s wrapper shrinks to just the protected-list check.

Correctly kept driver-local (don't hoist): the reserved volume-name list (PVC names are K8s-only), the PVC config structs, and the SPIFFE runtime-conflict check.

Note for reviewers: subpath support is intentionally inconsistent across drivers — Docker supports it on volume mounts, Podman rejects it (reject_subpath). This PR adding subpath for PVC mounts matches Docker; it's deliberate, not an oversight.

Implementation issues

Gate on these:

1. Validation is split across three sites with inconsistent handling of the same check. validate_kubernetes_mount_path is used in validate() (returns Err), in kubernetes_driver_volume_mount_to_k8s (.expect() — panics), and in has_explicit_sandbox_data_mount (.ok() — silently ignores). In the normal flow create_sandbox validates before rendering, so the panic is currently unreachable — but this PR widens the panic surface, since from_template() now fails on semantic checks (duplicate/reserved names, bad subpath, protected paths, read-only weakening), not just serde errors. Any future render path not preceded by validate_driver_config_for_sandbox will panic the driver instead of returning InvalidArgument. Render functions should take an already-validated, normalized config rather than re-parsing and .expect()-ing.

2. Re-validation at render exists only to recompute the normalized string. kubernetes_driver_volume_mount_to_k8s re-runs validate_kubernetes_mount_path / validate_mount_subpath purely to get the normalized mountPath/subPath. Validate once, store the normalized values on the struct, render from those — removes the redundant work and the panic in (1).

3. Reserved volume-name list duplicates pod-spec string literals. KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES hardcodes "openshell-client-tls" and "openshell-sa-token"; the pod spec hardcodes the same literals (driver.rs:1465,1478). They match today, but if either managed volume is renamed the guard silently stops matching and a user volume could collide. Extract both to named constants used in both places (the other three already use constants).

Design — worth discussing:

4. Any subpath mount under /sandbox disables all default workspace persistence. has_explicit_sandbox_data_mount flips inject_workspace off for any mount at-or-under /sandbox, so the documented example (/sandbox/.openshell/workspace, /sandbox/.openshell/memory) makes /sandbox itself ephemeral image content while only the two leaf subpaths persist. Kubernetes supports nested volumeMounts, so disabling the whole workspace isn't strictly required — it's a coarse simplification that silently drops persistence for everything else under /sandbox. At minimum this needs a prominent warning in the docs/README; better, reconsider whether the default workspace should coexist with narrow user subpath mounts.

5. Double read-only opt-in is easy to trip. Both the PVC volume and the mount default to read_only: true, and the validator rejects a RW mount on a RO volume — so RW requires read_only: false in two places. Consider defaulting the mount's read_only from the volume's value to halve the opt-in.

Minor:

6. validate_kubernetes_reference_name and the claim_name check don't enforce DNS-1123 label syntax, so invalid-but-printable names pass driver validation and fail later at the API server.
7. validate_kubernetes_driver_runtime_mounts re-runs validate_kubernetes_mount_path on every mount already validated by validate() (~3× per create) — folds away once (1)/(2) land.

Checked and not issues: the reserved volume-name list does currently cover all five managed volume names; protected-path coverage of managed mounts (TLS, sa-token, supervisor) is complete via the protected list + shared RESERVED_MOUNT_TARGETS; mount_path_conflicts_with_protected_path correctly also blocks mounting a parent of a reserved path.

Net: approve the direction; I'd like 1–3 (and ideally reuse items 1–4) addressed before merge.

[elezar]

Review: PVC subPath driver config

Thanks for this — the direction is good and it correctly routes subpath/source/target validation through the shared openshell_core::driver_mounts module. Two themes below: code reuse (so the K8s driver doesn't become a fourth divergent copy of mount logic) and implementation issues (a few I'd gate merge on).

Code reuse / overlap with other drivers

All container drivers already converge on openshell_core::driver_mounts (Docker lib.rs:1779-1909, Podman container.rs:546-688), so subpath rules, NUL/empty checks, .. rejection, and the /sandbox + reserved-target base list are already unified. This PR reuses most of that — good. Remaining duplication worth consolidating:

1. path_is_or_under is an exact copy of the private fn in driver_mounts.rs:99-104. Make the shared one pub and import it instead of re-declaring.
2. default_true() is now defined a third time (already in docker/lib.rs:342 and podman/container.rs:136). Along with the #[serde(default = "default_true")] read_only: bool idiom, this belongs in the shared module.
3. Duplicate mount-target detection is identical in three places — Docker (lib.rs:1857-1917), Podman (container.rs:625-681), and now here. A shared validate_unique_mount_targets(&[...], driver_name) would collapse all three.
4. Protected-path list overlaps and risks drift. KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS repeats /etc/openshell, /etc/openshell-tls, /run/netns (already in driver_mounts::RESERVED_MOUNT_TARGETS) and redundantly lists SUPERVISOR_CONTAINER_DIR / SUPERVISOR_CONTAINER_BINARY, both already under /opt/openshell which the shared list covers. Since validate_kubernetes_mount_path calls validate_container_mount_target first, keep only the K8s-unique entries (/var/run/secrets/openshell, /spiffe-workload-api) locally and let the shared list cover the rest — otherwise the two lists will silently diverge.
5. Consider promoting the stricter path checks into the shared validator. validate_kubernetes_mount_path adds rejection of internal empty segments (/a//b), . segments, and surrounding whitespace — checks validate_container_mount_target lacks today (Docker/Podman currently accept /a//b). If these are correctness improvements, hoist them so every driver benefits and the K8s wrapper shrinks to just the protected-list check.

Correctly kept driver-local (don't hoist): the reserved volume-name list (PVC names are K8s-only), the PVC config structs, and the SPIFFE runtime-conflict check.

Note for reviewers: subpath support is intentionally inconsistent across drivers — Docker supports it on volume mounts, Podman rejects it (reject_subpath). This PR adding subpath for PVC mounts matches Docker; it's deliberate, not an oversight.

Implementation issues

Gate on these:

1. Validation is split across three sites with inconsistent handling of the same check. validate_kubernetes_mount_path is used in validate() (returns Err), in kubernetes_driver_volume_mount_to_k8s (.expect() — panics), and in has_explicit_sandbox_data_mount (.ok() — silently ignores). In the normal flow create_sandbox validates before rendering, so the panic is currently unreachable — but this PR widens the panic surface, since from_template() now fails on semantic checks (duplicate/reserved names, bad subpath, protected paths, read-only weakening), not just serde errors. Any future render path not preceded by validate_driver_config_for_sandbox will panic the driver instead of returning InvalidArgument. Render functions should take an already-validated, normalized config rather than re-parsing and .expect()-ing.

2. Re-validation at render exists only to recompute the normalized string. kubernetes_driver_volume_mount_to_k8s re-runs validate_kubernetes_mount_path / validate_mount_subpath purely to get the normalized mountPath/subPath. Validate once, store the normalized values on the struct, render from those — removes the redundant work and the panic in (1).

3. Reserved volume-name list duplicates pod-spec string literals. KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES hardcodes "openshell-client-tls" and "openshell-sa-token"; the pod spec hardcodes the same literals (driver.rs:1465,1478). They match today, but if either managed volume is renamed the guard silently stops matching and a user volume could collide. Extract both to named constants used in both places (the other three already use constants).

Design — worth discussing:

4. Any subpath mount under /sandbox disables all default workspace persistence. has_explicit_sandbox_data_mount flips inject_workspace off for any mount at-or-under /sandbox, so the documented example (/sandbox/.openshell/workspace, /sandbox/.openshell/memory) makes /sandbox itself ephemeral image content while only the two leaf subpaths persist. Kubernetes supports nested volumeMounts, so disabling the whole workspace isn't strictly required — it's a coarse simplification that silently drops persistence for everything else under /sandbox. At minimum this needs a prominent warning in the docs/README; better, reconsider whether the default workspace should coexist with narrow user subpath mounts.

5. Double read-only opt-in is easy to trip. Both the PVC volume and the mount default to read_only: true, and the validator rejects a RW mount on a RO volume — so RW requires read_only: false in two places. Consider defaulting the mount's read_only from the volume's value to halve the opt-in.

Minor:

6. validate_kubernetes_reference_name and the claim_name check don't enforce DNS-1123 label syntax, so invalid-but-printable names pass driver validation and fail later at the API server.
7. validate_kubernetes_driver_runtime_mounts re-runs validate_kubernetes_mount_path on every mount already validated by validate() (~3× per create) — folds away once (1)/(2) land.

Checked and not issues: the reserved volume-name list does currently cover all five managed volume names; protected-path coverage of managed mounts (TLS, sa-token, supervisor) is complete via the protected list + shared RESERVED_MOUNT_TARGETS; mount_path_conflicts_with_protected_path correctly also blocks mounting a parent of a reserved path.

Net: approve the direction; I'd like 1–3 (and ideally reuse items 1–4) addressed before merge.

[mjamiv]

@austin-shih PR updated and ready for review.

[austin-shih]

Re-reviewed 3e86e520 — the gating items look addressed: normalization moved into validate() so the render helpers read normalized values (no re-parse/.expect()), reserved volume names are now shared constants, and path_is_or_under / default_true / validate_unique_mount_targets live in driver_mounts with Docker + Podman switched over. DNS-1123 validation and the /sandbox-injection doc note are in too. (One note: #5 — defaulting the mount's read_only from the volume — reads as documented rather than implemented; the double opt-in remains, which is fine by us, just flagging.)

Remaining gap is the cluster-level coverage we discussed: missing-PVC FailedMount and data-persisting-across-pod-recreation. We're set up to run that as a k3d e2e against this branch and will share results, so the durability claim has a proof.