fix(docker,jobs): scope container cleanup to owner labels and preserve pause/cancel status - #586
Merged
Merged
Conversation
Contributor
|
🌿 Preview your docs: https://nvidia-preview-deploy-race-rsadler.docs.buildwithfern.com/nemo-platform |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds model-label propagation and label-scoped Docker cleanup/reconciliation, updates Kubernetes object creation to carry extra model labels, preserves paused or cancelled job states when task errors appear, and switches one intake test base timestamp to a computed recent value. ChangesDocker and Kubernetes label scoping
Kubernetes terminal-state handling
Intake test timestamp update
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (1)
services/core/inference-gateway/tests/integration/conftest.py (1)
231-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fixture logic with
services/core/models/tests/integration/conftest.py.
docker_owner_labels,models_controller_container_cleanup, anddocker_backend_config's label wiring are near-identical across both integration conftest files. Sincenmp_testingalready hosts shared Docker test helpers, consider movingdocker_owner_labels(and possibly the cleanup fixture) there to avoid drift between services.🤖 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 `@services/core/inference-gateway/tests/integration/conftest.py` around lines 231 - 263, The Docker test fixture setup is duplicated across integration conftest files, so the label generation and cleanup logic can drift. Move the shared `docker_owner_labels` helper into the common test utilities in `nmp_testing`, and have `models_controller_container_cleanup` and `docker_backend_config` reuse that shared fixture instead of defining near-identical label wiring locally. Keep the service-specific fixtures only for settings that truly differ, and reference the existing `cleanup_model_deployment_containers` and `get_worker_port_range` usage when wiring the shared helper in.
🤖 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 `@services/core/inference-gateway/tests/integration/conftest.py`:
- Around line 231-263: The Docker test fixture setup is duplicated across
integration conftest files, so the label generation and cleanup logic can drift.
Move the shared `docker_owner_labels` helper into the common test utilities in
`nmp_testing`, and have `models_controller_container_cleanup` and
`docker_backend_config` reuse that shared fixture instead of defining
near-identical label wiring locally. Keep the service-specific fixtures only for
settings that truly differ, and reference the existing
`cleanup_model_deployment_containers` and `get_worker_port_range` usage when
wiring the shared helper in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d4c08a7c-64b5-4267-b8e2-b7cf10cbbcd0
📒 Files selected for processing (10)
docs/set-up/config-reference.mdxpackages/nmp_testing/src/nmp/testing/docker.pypackages/nmp_testing/tests/unit/test_docker.pyservices/core/inference-gateway/tests/integration/conftest.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/config.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/creation_reconciler.pyservices/core/models/tests/integration/conftest.pyservices/core/models/tests/integration/test_models_controller.pyservices/core/models/tests/unit/controllers/test_docker_backend.py
Contributor
|
ironcommit
changed the title
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)
services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/vllm_k8s_compiler.py (1)
85-101: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
common_labelsletsextra/model_labelsclobber reserved controller-identity labels — opposite precedence vs. the other two label-merge implementations in this PR.
common_labels()buildsMODEL_MANAGED_BY_LABEL,DEPLOYMENT_WORKSPACE_LABEL,DEPLOYMENT_NAME_LABEL,enginefirst, then doeslabels.update(extra)— soextra(now wired asself._backend_config.model_labelsfromk8s.py) wins on key conflict.compile_deployment's manualpod_labels.update(extra_labels)has the same issue.Compare to
nimservice_compiler._merge_model_labels(base wins) and Docker's_managed_container_labels(controller labels applied after model_labels) — both correctly protect controller labels. Here it's reversed.If
model_labelsever contains a colliding key (typo, copy/paste), the Deployment/PVC/Job/Service lose their correctDEPLOYMENT_WORKSPACE_LABEL/DEPLOYMENT_NAME_LABEL/MODEL_MANAGED_BY_LABEL, whichk8s.py'slist_managed_deployment_names()relies on for orphan reconciliation — exactly the mechanism this PR is trying to make safe.🐛 Proposed fix — controller labels take precedence
def common_labels( workspace: str, name: str, engine: str, *, extra: Optional[dict[str, str]] = None, ) -> dict[str, str]: """Labels stamped on every emitted object for management + orphan listing.""" - labels = { - MODEL_MANAGED_BY_LABEL: MODEL_MANAGED_BY_MODELS_CONTROLLER, - DEPLOYMENT_WORKSPACE_LABEL: workspace, - DEPLOYMENT_NAME_LABEL: name, - "nmp.nvidia.com/engine": engine, - } - if extra: - labels.update(extra) - return labels + labels = dict(extra or {}) + labels.update( + { + MODEL_MANAGED_BY_LABEL: MODEL_MANAGED_BY_MODELS_CONTROLLER, + DEPLOYMENT_WORKSPACE_LABEL: workspace, + DEPLOYMENT_NAME_LABEL: name, + "nmp.nvidia.com/engine": engine, + } + ) + return labelsAnd in
compile_deployment, apply the same precedence topod_labels:- selector_labels = {"app": resource_name} - pod_labels = { - **selector_labels, - **common_labels(workspace, name, engine), - } - if extra_labels: - pod_labels.update(extra_labels) + selector_labels = {"app": resource_name} + pod_labels = dict(extra_labels or {}) + pod_labels.update(selector_labels) + pod_labels.update(common_labels(workspace, name, engine))Also applies to: 156-156, 169-169, 199-199, 214-214, 328-334, 397-397, 418-425
🤖 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 `@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/vllm_k8s_compiler.py` around lines 85 - 101, `common_labels` and the `compile_deployment` label merge currently let `extra`/`model_labels` override reserved controller labels, which is the wrong precedence. Update `common_labels` so `MODEL_MANAGED_BY_LABEL`, `DEPLOYMENT_WORKSPACE_LABEL`, `DEPLOYMENT_NAME_LABEL`, and the engine label always win over `extra`, and apply the same controller-first merge order in `compile_deployment` for `pod_labels`. Use `common_labels` and `compile_deployment` as the touchpoints, and make the label behavior consistent with `_merge_model_labels` and `_managed_container_labels`.
🧹 Nitpick comments (1)
services/core/models/tests/unit/controllers/backends/test_vllm_k8s_compiler.py (1)
48-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage for the happy path. Consider adding a case where
extra_labelscollides with a reserved key (e.g.nmp.nvidia.com/deployment-workspace) to catch the precedence bug flagged invllm_k8s_compiler.py.🤖 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 `@services/core/models/tests/unit/controllers/backends/test_vllm_k8s_compiler.py` around lines 48 - 101, Add a test in the vllm_k8s_compiler controller suite that covers a collision between extra_labels and a reserved label key such as nmp.nvidia.com/deployment-workspace. Update the assertions around compile_pvc, compile_puller_job, compile_deployment, and compile_service to verify the reserved key keeps the compiler-defined value and is not overridden by extra_labels, while still preserving non-reserved labels.
🤖 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
`@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/vllm_k8s_compiler.py`:
- Around line 85-101: `common_labels` and the `compile_deployment` label merge
currently let `extra`/`model_labels` override reserved controller labels, which
is the wrong precedence. Update `common_labels` so `MODEL_MANAGED_BY_LABEL`,
`DEPLOYMENT_WORKSPACE_LABEL`, `DEPLOYMENT_NAME_LABEL`, and the engine label
always win over `extra`, and apply the same controller-first merge order in
`compile_deployment` for `pod_labels`. Use `common_labels` and
`compile_deployment` as the touchpoints, and make the label behavior consistent
with `_merge_model_labels` and `_managed_container_labels`.
---
Nitpick comments:
In
`@services/core/models/tests/unit/controllers/backends/test_vllm_k8s_compiler.py`:
- Around line 48-101: Add a test in the vllm_k8s_compiler controller suite that
covers a collision between extra_labels and a reserved label key such as
nmp.nvidia.com/deployment-workspace. Update the assertions around compile_pvc,
compile_puller_job, compile_deployment, and compile_service to verify the
reserved key keeps the compiler-defined value and is not overridden by
extra_labels, while still preserving non-reserved labels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 612e4f01-f98a-4fe6-a4ae-2a0cf2492108
📒 Files selected for processing (15)
docs/set-up/config-reference.mdxservices/core/inference-gateway/tests/integration/conftest.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/config.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/creation_reconciler.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/config.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/nimservice_compiler.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/k8s.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/vllm_k8s_compiler.pyservices/core/models/tests/integration/conftest.pyservices/core/models/tests/integration/test_models_controller.pyservices/core/models/tests/unit/controllers/backends/test_vllm_k8s_compiler.pyservices/core/models/tests/unit/controllers/test_backend_config_fields.pyservices/core/models/tests/unit/controllers/test_docker_backend.py
✅ Files skipped from review due to trivial changes (1)
- docs/set-up/config-reference.mdx
🚧 Files skipped from review as they are similar to previous changes (7)
- services/core/models/tests/integration/conftest.py
- services/core/models/tests/integration/test_models_controller.py
- services/core/models/src/nmp/core/models/controllers/backends/docker/backend.py
- services/core/inference-gateway/tests/integration/conftest.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py
- services/core/models/tests/unit/controllers/test_docker_backend.py
- services/core/models/src/nmp/core/models/controllers/backends/docker/creation_reconciler.py
ironcommit
force-pushed
the
deploy-race/rsadler
branch
from
July 7, 2026 14:54
17d89d6 to
922910b
Compare
maxdubrinsky
approved these changes
Jul 7, 2026
…t-run owner labels Add models_docker_container_labels config field to DockerBackendConfig so containers created by the models controller can carry arbitrary labels. Integration test fixtures use this to stamp containers with test-run and test-worker identity labels, scoping teardown cleanup and orphan reconciliation to the owning pytest-xdist worker. Also pass ignore_removed=True on Docker container list calls to prevent races when another worker removes a container during enumeration. Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
ironcommit
force-pushed
the
deploy-race/rsadler
branch
from
July 7, 2026 16:35
922910b to
499117d
Compare
ironcommit
enabled auto-merge
July 7, 2026 16:36
mckornfield
approved these changes
Jul 7, 2026
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
…t-run owner labels (#586) Add models_docker_container_labels config field to DockerBackendConfig so containers created by the models controller can carry arbitrary labels. Integration test fixtures use this to stamp containers with test-run and test-worker identity labels, scoping teardown cleanup and orphan reconciliation to the owning pytest-xdist worker. Also pass ignore_removed=True on Docker container list calls to prevent races when another worker removes a container during enumeration. Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.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
3 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.
Docker: scope container cleanup and orphan reconciliation to owner labels
Add
models_docker_container_labelsconfig field toDockerBackendConfigso containers created by the models controller carry arbitrary labels. Integration test fixtures use this to stamp containers with test-run and test-worker identity labels, scoping teardown cleanup and orphan reconciliation to the owning pytest-xdist worker._managed_container_labels()helper on the creation reconciler centralises label construction for NIM, sidecar, and puller containerscleanup_model_deployment_containers()accepts optional owner labels and only removes matching containersignore_removed=Trueon Docker container list calls to prevent races when another worker removes a container during enumerationJobs: preserve pause/cancel status when pods report task errors
When a Kubernetes job is pausing, paused, cancelling, or cancelled, pod-level task errors (e.g. SIGTERM exit code 137) no longer override the job status to ERROR. This prevents user-initiated pause/cancel from incorrectly surfacing as a failure.
Tests: fix intake ATIF ingest timestamp
Replace the frozen
_BASE_TIME(2026-01-15) with a dynamic timestamp 7 days in the past so ingested spans stay within ClickHouse's 90-day TTL and tests don't start failing as time passes.Summary by CodeRabbit
model_labelssupport for applying custom labels to model resources across Docker and Kubernetes backends.PAUSED/CANCELLEDwhen task errors occur during terminal transitions.