chore: harden container labeling and e2e tests - #918
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughChangesThe PR adds resource-scope isolation for E2E Docker deployments, improves anonymizer E2E diagnostics and typing, and makes deployment deletion retry through concurrent updates and transient disappearance. ChangesDocker resource scoping
Anonymizer E2E diagnostics
Deployment deletion retries
Sequence Diagram(s)sequenceDiagram
participant E2EConfig
participant DockerBackend
participant DockerDaemon
E2EConfig->>DockerBackend: provide resource_scope
DockerBackend->>DockerDaemon: create labeled resources
DockerBackend->>DockerDaemon: query matching scope
DockerDaemon-->>DockerBackend: return scoped resources
sequenceDiagram
participant Client
participant DeleteAPI
participant EntityClient
Client->>DeleteAPI: delete deployment
DeleteAPI->>EntityClient: fetch and mark deleting
EntityClient-->>DeleteAPI: success, conflict, or not found
DeleteAPI->>EntityClient: retry on conflict
Possibly related PRs
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: 2
🧹 Nitpick comments (2)
plugins/nemo-agents/tests/unit/test_deployments_api.py (1)
123-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for exhausted-retry → 409 path.
Tests cover success, single-retry, and disappear-during-retry, but not the case where
updateraisesNemoEntityConflictErroron all_DELETE_MARK_ATTEMPTSattempts (deployments.py lines 236-240), which should return409. That's the most novel logic added in this cohort and currently has no coverage.✅ Suggested additional test
def test_delete_returns_409_when_conflicts_exhausted(self) -> None: mock_entity_client = AsyncMock() mock_entity_client.get = AsyncMock( side_effect=[_make_deployment(status="pending") for _ in range(3)] ) mock_entity_client.update = AsyncMock(side_effect=NemoEntityConflictError("conflict")) client = _test_client(mock_entity_client) resp = client.delete("/apis/agents/v2/workspaces/default/deployments/fabric-dep") assert resp.status_code == 409 assert mock_entity_client.update.await_count == 3🤖 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-agents/tests/unit/test_deployments_api.py` around lines 123 - 168, Add a test to TestDeleteDeployment covering NemoEntityConflictError on every _DELETE_MARK_ATTEMPTS attempt. Configure get and update mocks for all retries, call the deployment delete endpoint, and assert it returns 409 with update awaited once per attempt.e2e/test_nemo_agents.py (1)
133-145: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider handling
409like500here.The delete endpoint now returns
409once concurrency-conflict retries are exhausted (seedeployments.py_DELETE_MARK_ATTEMPTSlogic). Sincedelete_deployment's docstring indicates a controller reconciles/updates the entity concurrently, a409here is plausible, not just theoretical — but this helper only tolerates404/500, so a409will fail the test even if the deployment is genuinely gone or about to be reconciled away.♻️ Proposed fix
except httpx.HTTPStatusError as exc: if exc.response.status_code == 404: return - if exc.response.status_code == 500: + if exc.response.status_code in (500, 409): try: sdk.agents.deployments.get(name, workspace=workspace) except httpx.HTTPStatusError as get_exc: if get_exc.response.status_code == 404: return raise🤖 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/test_nemo_agents.py` around lines 133 - 145, Update _delete_deployment_if_exists to handle HTTP 409 the same way as HTTP 500: after either status, call sdk.agents.deployments.get and return when that verification responds with 404; otherwise preserve the existing exception propagation behavior.
🤖 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/test_anonymizer_plugin.py`:
- Around line 301-306: Update the timeout handling around job.get_logs() so
log-retrieval failures cannot replace the intended TimeoutError. Catch failures
from get_logs, fall back to an empty or unavailable log tail, and still raise
TimeoutError with the job status and last_poll_error diagnostics preserved.
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py`:
- Around line 698-711: Apply _container_matches_deployment_group to every
direct-name containers.get result, including the force-removal path for
same-name init containers, read_status, and get_logs. Treat any container
failing the workspace, deployment name, managed-by, or resource-scope checks as
absent, preserving the existing behavior for genuinely matching containers and
preventing foreign containers from being inspected or removed.
---
Nitpick comments:
In `@e2e/test_nemo_agents.py`:
- Around line 133-145: Update _delete_deployment_if_exists to handle HTTP 409
the same way as HTTP 500: after either status, call sdk.agents.deployments.get
and return when that verification responds with 404; otherwise preserve the
existing exception propagation behavior.
In `@plugins/nemo-agents/tests/unit/test_deployments_api.py`:
- Around line 123-168: Add a test to TestDeleteDeployment covering
NemoEntityConflictError on every _DELETE_MARK_ATTEMPTS attempt. Configure get
and update mocks for all retries, call the deployment delete endpoint, and
assert it returns 409 with update awaited once per attempt.
🪄 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: 902de918-0c73-45b2-b7eb-590bbbc29cc0
📒 Files selected for processing (11)
e2e/services_pool.pye2e/test_anonymizer_plugin.pye2e/test_nemo_agents.pypackages/nmp_testing/tests/unit/test_e2e_harness.pyplugins/nemo-agents/src/nemo_agents_plugin/api/v2/deployments.pyplugins/nemo-agents/tests/unit/test_deployments_api.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/config.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/docker/test_labels.py
mckornfield
commented
Jul 27, 2026
Contributor
|
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
mckornfield
force-pushed
the
harden-labeling-test-fixes/mck
branch
from
July 27, 2026 19:06
56d4fe7 to
b3b63e7
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/volumes.py (1)
30-42: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent cross-scope volume reuse.
docker_volume_name(workspace, name)omitsresource_scope, and an existing volume is accepted without checking its labels. Two scopes with the same workspace/name will share data. Namespace volume names by scope or reject volumes whose scope label differs; apply the same scope rule to read/delete operations.🤖 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/src/nemo_deployments_plugin/backends/docker/volumes.py` around lines 30 - 42, The volume lifecycle around _create must prevent cross-scope reuse: incorporate resource_scope into docker_volume_name or validate the existing volume’s scope label against resource_scope before returning it. Apply the same scoped naming or label-validation rule consistently to the corresponding read and delete operations.
🤖 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.
Outside diff comments:
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/volumes.py`:
- Around line 30-42: The volume lifecycle around _create must prevent
cross-scope reuse: incorporate resource_scope into docker_volume_name or
validate the existing volume’s scope label against resource_scope before
returning it. Apply the same scoped naming or label-validation rule consistently
to the corresponding read and delete operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1f5dcd12-2ffe-4982-9f66-452870f28718
📒 Files selected for processing (15)
e2e/services_pool.pye2e/test_anonymizer_plugin.pye2e/test_nemo_agents.pypackages/nmp_testing/tests/unit/test_e2e_harness.pyplugins/nemo-agents/src/nemo_agents_plugin/api/v2/deployments.pyplugins/nemo-agents/tests/unit/test_deployments_api.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/config.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/volumes.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/docker/test_executor_config.pyplugins/nemo-deployments/tests/unit/backends/docker/test_idempotency.pyplugins/nemo-deployments/tests/unit/backends/docker/test_labels.py
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e/test_nemo_agents.py
- plugins/nemo-deployments/tests/unit/backends/docker/test_labels.py
- plugins/nemo-agents/tests/unit/test_deployments_api.py
- plugins/nemo-agents/src/nemo_agents_plugin/api/v2/deployments.py
- e2e/test_anonymizer_plugin.py
- e2e/services_pool.py
svvarom
self-requested a review
July 27, 2026 23:53
svvarom
approved these changes
Jul 28, 2026
anastasia-nesterenko
approved these changes
Jul 28, 2026
mckornfield
enabled auto-merge
July 28, 2026 22:03
ngoncharenko
pushed a commit
that referenced
this pull request
Jul 29, 2026
Signed-off-by: Matt Kornfield <mkornfield@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
4 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 by CodeRabbit
New Features
Bug Fixes