feat(deployments): K8s PVC volumes (AIRCORE-757 phase 2) - #536
Merged
Conversation
…ase 2) Add volumes.py for create/read/delete PersistentVolumeClaims with phase mapping (Pending/Bound/Lost). Wire K8sDeploymentBackend volume methods and pass entity backend_config through volume reconciler read/delete so custom namespaces resolve correctly. Signed-off-by: Tyler Bray <tbray@nvidia.com>
…ating Reject 409 idempotency when an existing PVC lacks plugin identity labels. Report DELETING when metadata.deletionTimestamp is set even if phase is Bound. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds optional ChangesVolume backend_config wiring and Kubernetes PVC implementation
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/nemo-deployments/tests/unit/backends/k8s/test_volumes.py (1)
58-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant client wiring.
mock_k8s_clients.request_timeout = 60(line 60) is set but the actualclientsobject passed tocreate_volume(line 65-67) is a separateMagicMock(spec=KubernetesClients)with its ownrequest_timeout = 60. Themock_k8s_clients.request_timeoutassignment is dead here since onlycore_v1is shared.🧹 Suggested cleanup
async def test_create_volume_conflict_reads_existing_pvc(mock_k8s_clients: MagicMock) -> None: - mock_k8s_clients.request_timeout = 60 existing = MagicMock()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-deployments/tests/unit/backends/k8s/test_volumes.py` around lines 58 - 80, Remove the redundant request_timeout setup on mock_k8s_clients in test_create_volume_conflict_reads_existing_pvc, since create_volume is called with the separate clients MagicMock that already has request_timeout set. Keep the shared core_v1 mock wiring for KubernetesClients, and clean up the test setup so only the object actually passed into volume_ops.create_volume carries the timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/volumes.py`:
- Around line 76-97: Wrap the calls to resolve_volume_namespace and
resolve_storage_class in create_volume, read_volume_status, and delete_volume
with the same try/except path used for other volume failures so pydantic
ValidationError from K8sVolumeConfig.model_validate is converted into a
VolumeStatusUpdate(status="FAILED", ...). In create_volume, read_volume_status,
and delete_volume, ensure malformed backend_config["k8s"] never escapes
uncaught, and consolidate the two resolver calls per function so the K8s section
is validated only once.
---
Nitpick comments:
In `@plugins/nemo-deployments/tests/unit/backends/k8s/test_volumes.py`:
- Around line 58-80: Remove the redundant request_timeout setup on
mock_k8s_clients in test_create_volume_conflict_reads_existing_pvc, since
create_volume is called with the separate clients MagicMock that already has
request_timeout set. Keep the shared core_v1 mock wiring for KubernetesClients,
and clean up the test setup so only the object actually passed into
volume_ops.create_volume carries the timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7b11558d-7b72-4fdb-919f-a5096b947e33
📒 Files selected for processing (8)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/base.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/volumes.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/volume_reconciler.pyplugins/nemo-deployments/tests/unit/backends/k8s/conftest.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_volumes.pyplugins/nemo-deployments/tests/unit/reconciler/conftest.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/nemo-deployments/tests/unit/backends/k8s/test_volumes.py (1)
118-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant dict construction.
{MANAGED_BY_KEY: labels[MANAGED_BY_KEY], **labels}—labelsalready containsMANAGED_BY_KEY; the explicit key is redundant.Simplify
- pvc.metadata.labels = {MANAGED_BY_KEY: labels[MANAGED_BY_KEY], **labels} + pvc.metadata.labels = labels🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-deployments/tests/unit/backends/k8s/test_volumes.py` around lines 118 - 128, The test setup in test_status_from_pvc_deleting_reports_deleting builds pvc.metadata.labels with a redundant MANAGED_BY_KEY entry even though volume_identity_labels already includes it. Simplify the fixture by assigning labels directly (or by constructing only the needed overrides) so the test remains clear and avoids duplicated dict keys in the status_from_pvc setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/nemo-deployments/tests/unit/backends/k8s/test_volumes.py`:
- Around line 118-128: The test setup in
test_status_from_pvc_deleting_reports_deleting builds pvc.metadata.labels with a
redundant MANAGED_BY_KEY entry even though volume_identity_labels already
includes it. Simplify the fixture by assigning labels directly (or by
constructing only the needed overrides) so the test remains clear and avoids
duplicated dict keys in the status_from_pvc setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0ea16cf3-f65c-49b9-9a77-aac59a65721f
📒 Files selected for processing (2)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/volumes.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_volumes.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/volumes.py
Contributor
|
Consolidate K8sVolumeConfig validation into resolve_volume_config and run it inside the try blocks in create/read/delete so pydantic ValidationError surfaces as VolumeStatusUpdate instead of crashing the reconciler. Address CodeRabbit test nitpicks. Signed-off-by: Tyler Bray <tbray@nvidia.com>
1 task
mckornfield
approved these changes
Jul 1, 2026
Drop del backend_config stubs in favor of a clarifying comment on Docker, and rewrite resolve_volume_namespace with a positive and guard. Signed-off-by: Tyler Bray <tbray@nvidia.com>
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
* feat(deployments): implement K8s PVC volume lifecycle (AIRCORE-757 phase 2) Add volumes.py for create/read/delete PersistentVolumeClaims with phase mapping (Pending/Bound/Lost). Wire K8sDeploymentBackend volume methods and pass entity backend_config through volume reconciler read/delete so custom namespaces resolve correctly. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): verify PVC identity labels on 409 and handle terminating Reject 409 idempotency when an existing PVC lacks plugin identity labels. Report DELETING when metadata.deletionTimestamp is set even if phase is Bound. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): catch malformed k8s volume backend_config as FAILED Consolidate K8sVolumeConfig validation into resolve_volume_config and run it inside the try blocks in create/read/delete so pydantic ValidationError surfaces as VolumeStatusUpdate instead of crashing the reconciler. Address CodeRabbit test nitpicks. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): address mckornfield review on volume backend_config Drop del backend_config stubs in favor of a clarifying comment on Docker, and rewrite resolve_volume_namespace with a positive and guard. Signed-off-by: Tyler Bray <tbray@nvidia.com> --------- Signed-off-by: Tyler Bray <tbray@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
create_volume/read_volume_status/delete_volumeforK8sDeploymentBackendviabackends/k8s/volumes.pyPending→PENDING,Bound→BOUND,Lost→FAILEDK8sVolumeConfig.storageClassandnamespaceon create; extend ABC + volume reconciler to passbackend_configon read/delete so custom namespaces workTest plan
uv run pytest plugins/nemo-deployments/tests/unit -v(186 passed)tests/unit/backends/k8s/test_volumes.py— emission, status mapping, 409 idempotency, namespace on readLinear
AIRCORE-757 — Phase 2 of 7
Summary by CodeRabbit