test(e2e): add nemo-deployments plugin e2e tests for docker and k8s backends#766
Open
benmccown wants to merge 2 commits into
Open
test(e2e): add nemo-deployments plugin e2e tests for docker and k8s backends#766benmccown wants to merge 2 commits into
benmccown wants to merge 2 commits into
Conversation
…ackends Exercise the nemo-deployments plugin's own public API (DeploymentConfig / Deployment / Volume CRUD) end to end through the reconcile controller on both the Docker and Kubernetes executor backends, mirroring the existing nemo-agents deployment e2e structure. - deployments_helpers.py: backend-agnostic scenario cores driven via sdk._client against /apis/deployments/v2/... (service->READY, job->SUCCEEDED, and a volume provision+mount+read-back round-trip). Workload image refs default to docker.io/library/alpine|nginx and are env-overridable, matching the POSTGRES_IMAGE/BUSYBOX_IMAGE knobs the kind e2e install already exposes. - test_nemo_deployments_docker.py: subprocess harness + docker executor (subprocess_only), skips cleanly without a reachable Docker daemon. - test_nemo_deployments_k8s.py: external kind cluster + k8s executor (container_only), wider timeouts for pod scheduling / PVC binding. - configs/local-docker-deployments.yaml: subprocess platform wired with a docker deployments executor (pull_images enabled, tightened reconcile loop). Signed-off-by: Ben McCown <bmccown@nvidia.com>
Contributor
|
…mer) Manual testing in a kind cluster surfaced a hard deadlock in the k8s volume round-trip: DeploymentReconciler gates a deployment's create on every mounted Volume already being BOUND (volume_mounts_ready), but kind's default local-path StorageClass binds WaitForFirstConsumer, so the PVC only binds once a consumer pod is scheduled — which never happens while the deployment is gated. The Kind CPU e2e job uses this same storage class, so the test would have hung until timeout in CI. This mirrors a known limitation the k8s reconcile integration test already documents and sidesteps (tests/integration/test_reconcile_k8s.py). - Remove the volume scenario from test_nemo_deployments_k8s.py (keeps service->READY and job->SUCCEEDED, both verified passing against a kind Helm platform). - Keep the volume round-trip in the docker module, where volumes bind eagerly (verified passing); tighten its volume-status wait to require BOUND rather than tolerating PENDING. - Document the storage-binding portability constraint in both the k8s module and the run_volume_deployment_round_trip helper. Signed-off-by: Ben McCown <bmccown@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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded shared REST and lifecycle helpers for the deployments plugin, a local Docker E2E configuration, and Docker/Kubernetes tests covering services, jobs, and volume round trips. ChangesDeployments E2E coverage
Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant NeMoDeploymentsAPI
participant ReconcileController
participant Backend
E2ETest->>NeMoDeploymentsAPI: Create deployment resources
NeMoDeploymentsAPI->>ReconcileController: Reconcile resources
ReconcileController->>Backend: Provision workload or volume
E2ETest->>NeMoDeploymentsAPI: Poll status and validate result
E2ETest->>NeMoDeploymentsAPI: Delete resources
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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 `@e2e/deployments_helpers.py`:
- Around line 444-485: Move the create_volume, wait_for_volume_status, and
create_deployment_config calls into the existing try block so every resource
creation and polling operation is protected by teardown. Keep the existing
cleanup logic and deployment configuration unchanged.
🪄 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: 2f6944f3-bb15-4656-ae97-c6ee01f1e9b5
📒 Files selected for processing (4)
e2e/configs/local-docker-deployments.yamle2e/deployments_helpers.pye2e/test_nemo_deployments_docker.pye2e/test_nemo_deployments_k8s.py
Comment on lines
+444
to
+485
| create_volume( | ||
| sdk, | ||
| workspace=workspace, | ||
| name=volume_name, | ||
| size="1Gi", | ||
| access_modes=["ReadWriteOnce"], | ||
| backend_config=volume_backend_config, | ||
| ) | ||
|
|
||
| # A freshly-created volume must at least leave the initial state; lazy-binding | ||
| # k8s storage classes keep it PENDING until first mount, so accept either. | ||
| wait_for_volume_status( | ||
| sdk, | ||
| workspace=workspace, | ||
| name=volume_name, | ||
| target_statuses=("BOUND", "PENDING"), | ||
| timeout_seconds=120, | ||
| ) | ||
|
|
||
| create_deployment_config( | ||
| sdk, | ||
| workspace=workspace, | ||
| name=config_name, | ||
| restart_policy="Never", | ||
| volume_mounts=[{"name": volume_name, "mountPath": mount_path}], | ||
| containers=[ | ||
| { | ||
| "name": "main", | ||
| "image": ALPINE_IMAGE, | ||
| "command": ["sh", "-c"], | ||
| "args": [ | ||
| # Write a sentinel to the mounted volume then read it back and | ||
| # assert its content, exiting non-zero (=> FAILED) on mismatch. | ||
| f"set -e; echo {sentinel} > {sentinel_file}; grep -q {sentinel} {sentinel_file}; " | ||
| f"echo mount-verified", | ||
| ], | ||
| "volumeMounts": [{"name": volume_name, "mountPath": mount_path}], | ||
| } | ||
| ], | ||
| ) | ||
|
|
||
| try: |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Include all resource creation within the teardown-protected block.
Volume polling or config creation can fail before entering try, bypassing cleanup and leaking resources. Move Lines 444-483 inside the existing try.
🤖 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 `@e2e/deployments_helpers.py` around lines 444 - 485, Move the create_volume,
wait_for_volume_status, and create_deployment_config calls into the existing try
block so every resource creation and polling operation is protected by teardown.
Keep the existing cleanup logic and deployment configuration unchanged.
tylersbray
approved these changes
Jul 17, 2026
tylersbray
left a comment
tylersbray
left a comment
Contributor
There was a problem hiding this comment.
Approving modulo knocking out code rabbit comments.
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.
What
Adds e2e coverage for the nemo-deployments plugin's own public API (DeploymentConfig / Deployment / Volume CRUD) end to end through the reconcile controller, on both the Docker and Kubernetes executor backends. Mirrors the existing nemo-agents deployment e2e structure (
test_nemo_agents_docker.py/test_nemo_agents_k8s.py+ shared helpers).Files
e2e/deployments_helpers.py— backend-agnostic scenario cores driven viasdk._clientagainst/apis/deployments/v2/...:restart_policy=Alwaysnginx) → READY with a routable endpointrestart_policy=Neveralpine) → SUCCEEDED, exit 0e2e/test_nemo_deployments_docker.py— subprocess harness + docker executor (subprocess_only); skips cleanly without a reachable Docker daemon.e2e/test_nemo_deployments_k8s.py— external kind cluster + k8s executor (container_only); wider timeouts for pod scheduling / PVC binding.e2e/configs/local-docker-deployments.yaml— subprocess platform wired with a docker deployments executor (pull_imagesenabled, tightened reconcile loop).Notes
docker.io/library/alpine|nginxand are env-overridable (NMP_E2E_DEPLOYMENTS_ALPINE_IMAGE/_NGINX_IMAGE), matching thePOSTGRES_IMAGE/BUSYBOX_IMAGEknobs the kind e2e install already exposes. Pulling publicdocker.io/library/*at cluster runtime is the same pattern the kind CPU e2e job already relies on for postgres/busybox/cloud-provider-kind (no pull-through cache is configured today).kind-cpu-e2ejob (wholee2e/dir), gated bycontainer_only. Docker tests aresubprocess_only.Local:
ruff check,ruff format --check, andty checkall pass; fulle2e/collects.Summary by CodeRabbit
New Features
Tests