fix(nemo-deployments): observe one-shot Docker status at create time - #925
Conversation
|
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:
📝 WalkthroughWalkthroughDocker deployment creation now observes one-shot containers immediately, centralizes exited-container status mapping, handles wait failures and cleanup, and adds configurable observation timeouts with policy-specific tests. ChangesDocker one-shot status handling
Sequence Diagram(s)sequenceDiagram
participant create_deployment
participant DockerContainer
participant _observe_one_shot_primary_after_create
participant _status_from_exited_container
create_deployment->>DockerContainer: start primary container
create_deployment->>_observe_one_shot_primary_after_create: observe restart policy
_observe_one_shot_primary_after_create->>DockerContainer: wait or reload state
_observe_one_shot_primary_after_create->>_status_from_exited_container: map exited state
_status_from_exited_container-->>create_deployment: BackendStatusUpdate
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py (1)
607-609: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate exit-code parsing.
_wait_for_exit'sStatusCodeextraction duplicates_run_and_wait(lines 429-432). Extract a shared_exit_code_from_wait_result(result)helper.♻️ Proposed refactor
+ `@staticmethod` + def _exit_code_from_wait_result(result: dict[str, Any] | int) -> int: + return int(result.get("StatusCode", 1)) if isinstance(result, dict) else int(result) + def _run_and_wait() -> int: container = self._client.containers.run(**run_kwargs) result = container.wait(timeout=self._executor_config.docker_timeout) - exit_code = int(result.get("StatusCode", 1)) if isinstance(result, dict) else int(result) + exit_code = self._exit_code_from_wait_result(result)🤖 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/backend.py` around lines 607 - 609, Extract the duplicated wait-result parsing from _wait_for_exit and _run_and_wait into a shared _exit_code_from_wait_result(result) helper. Update both callers to use this helper while preserving the existing StatusCode default of 1 and integer conversion for non-dictionary results.
🤖 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/src/nemo_deployments_plugin/backends/docker/backend.py`:
- Around line 607-609: Extract the duplicated wait-result parsing from
_wait_for_exit and _run_and_wait into a shared
_exit_code_from_wait_result(result) helper. Update both callers to use this
helper while preserving the existing StatusCode default of 1 and integer
conversion for non-dictionary results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 50ed0d25-22e8-4422-aaf7-58e0d6f0bd36
📒 Files selected for processing (4)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/docker/test_idempotency.py
|
benmccown
left a comment
benmccown
left a comment
There was a problem hiding this comment.
Docker SDK timeout is 600 seconds by default. If you have a longer running one-shot container (like the model weights puller) this is going to block the reconciler loop and it also might take longer than that 600 second timeout.
|
@benjamin-mccown Good catch — you're right that using Pushed a follow-up: added |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py (1)
73-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate backend-construction boilerplate.
_docker_backend_with_observe_timeoutre-implements the same threepatch(...)context managers asdocker_backend()(lines 35-45), differing only by the extra config key. Consider havingdocker_backend()/a shared helper accept optional**config_overridesto avoid drift between the two.♻️ Example consolidation
-def docker_backend() -> DockerDeploymentBackend: +def _build_docker_backend(**config_overrides: Any) -> DockerDeploymentBackend: mock_entities = AsyncMock() mock_sdk = MagicMock() with ( patch("nemo_deployments_plugin.backends.docker.backend.AsyncEntitiesResource"), patch("nemo_deployments_plugin.backends.docker.backend.NemoEntitiesClient", return_value=mock_entities), patch("nemo_deployments_plugin.backends.docker.backend.get_shared_gpu_pool", return_value=None), ): - backend = DockerDeploymentBackend(mock_sdk, {"pull_images": True}) + backend = DockerDeploymentBackend(mock_sdk, {"pull_images": True, **config_overrides}) backend._entities = mock_entities return backend + + +def docker_backend() -> DockerDeploymentBackend: + return _build_docker_backend() + + +def _docker_backend_with_observe_timeout(*, oneshot_observe_timeout_seconds: int) -> DockerDeploymentBackend: + return _build_docker_backend(oneshot_observe_timeout_seconds=oneshot_observe_timeout_seconds)🤖 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/integration/backends/docker/test_docker_backend.py` around lines 73 - 93, Consolidate the duplicate backend setup by updating the existing docker_backend helper, or introducing a shared construction helper, to accept optional config overrides. Refactor _docker_backend_with_observe_timeout to reuse that helper while supplying oneshot_observe_timeout_seconds, preserving the existing patches and default configuration.
🤖 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/tests/integration/backends/docker/test_docker_backend.py`:
- Around line 168-180: The timing assertion around create_deployment in the
observe-wait integration test incorrectly includes image-pull latency. Adjust
the test setup or measurement so create_elapsed isolates the observe-wait
behavior while retaining the status assertions, and avoid enforcing the 3-second
threshold against uncached alpine image pulls.
---
Nitpick comments:
In
`@plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py`:
- Around line 73-93: Consolidate the duplicate backend setup by updating the
existing docker_backend helper, or introducing a shared construction helper, to
accept optional config overrides. Refactor _docker_backend_with_observe_timeout
to reuse that helper while supplying oneshot_observe_timeout_seconds, preserving
the existing patches and default configuration.
🪄 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: 0ca5b071-6ddc-4dcd-9922-fc8150407447
📒 Files selected for processing (1)
plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py
1a7e6ac to
8cdd674
Compare
Never jobs wait for exit and return SUCCEEDED/FAILED immediately; OnFailure inspects after create. Avoids the fast-exit race without die-event replay or e2e sleeps. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Use DockerExecutorConfig.oneshot_observe_timeout_seconds (default 5s) instead of docker_timeout for create-time Never container.wait so long pullers do not block the serial reconciler loop. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Add a real-Docker integration test with oneshot_observe_timeout_seconds=1 and alpine sleep 5 so create returns STARTING quickly and read_status later reports SUCCEEDED — the long puller path. Signed-off-by: Tyler Bray <tbray@nvidia.com>
The observe-wait integration test timed the whole create_deployment call with pull_images enabled, so an uncached alpine pull could push it past the 3s bound for reasons unrelated to the observe timeout. Warm the image cache first and bound the assertion by the configured observe timeout. Also dedupe container.wait() exit-code parsing into _exit_code_from_wait_result and fold the two integration backend builders into one helper that takes config overrides. Signed-off-by: Tyler Bray <tbray@nvidia.com>
8cdd674 to
a1c2d4c
Compare
The Docker host-port allocator could hand out a port another container was already publishing, so `create_deployment` failed with "port is already allocated". Three defects combined: - The free-port probe bound `127.0.0.1` with `SO_REUSEADDR`, which reports a port free even while another process holds `0.0.0.0:<port>`. It now binds the wildcard address Docker publishes on, without `SO_REUSEADDR`. - The container scan filtered on the platform's managed-by label, so containers the platform does not own (a test-fixture ClickHouse on 9000, say) were invisible. Every container on the daemon competes for host ports. - Docker's own reservations are not observable until a publish is attempted, so a concurrent create can still win the race. That publish failure is now retried on a different port, and the container left behind by the failed start is removed first (`containers.run` creates then starts). This was surfacing as an integration-test flake: the deployments Docker tests allocate from the 9000-9100 default while other suites publish ClickHouse on 9000. Those tests now use a range nothing else in CI claims. Signed-off-by: Tyler Bray <tbray@nvidia.com>
|
Added one more commit ( What was breaking. Three defects, all in the allocator:
Fix 1 is what actually closes the observed failure; 2 and 3 close the remaining windows. Verification. With a container publishing The Docker integration tests pass with 9000 occupied. Unit coverage added for the wildcard probe, the foreign-container scan, and all three retry outcomes (reallocate and succeed, exhaust attempts, don't retry unrelated failures. Separately, the deployments Docker integration tests now use 21000-21100 instead of the 9000-9100 product default, so they don't contend with ClickHouse even when the allocator is right. The product default is unchanged. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Keep the corrected no-SO_REUSEADDR behavior while avoiding a wildcard bind that CodeQL flags as externally exposed. A Docker wildcard publisher still blocks the loopback probe. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Avoid collisions with well-known service ports by allocating Docker deployments from the IANA dynamic/private range by default. Signed-off-by: Tyler Bray <tbray@nvidia.com>
|
Follow-up to the host-port allocator fix: after discussing the product default with Ben, commit This avoids the standing ClickHouse/native-port collision by default while preserving roughly the same capacity. The allocator scan/retry fixes remain necessary for arbitrary collisions. Updated the executor default, platform/e2e configs, docs, and default-value test; existing deployments and explicit executor overrides are unaffected. Validation: focused executor-config tests passed, and the full pre-commit/pre-push hooks passed. |
The port-conflict retry tests hardcoded the old 9000-based default, so they broke when the default range moved. Read the configured range start instead. Signed-off-by: Tyler Bray <tbray@nvidia.com>
3 participants
Summary
restart_policy=Never, wait for the primary container to exit at create time and returnSUCCEEDED/FAILEDwith the exit code (bounded bydocker_timeout).OnFailure, immediately inspect after create; map already-exited containers with the same backoff logic asread_status._status_from_exited_containerso create and read stay aligned; no die-event replay and no e2e sleep workaround.Addresses the fast-exit flake discussed for
e2e/test_nemo_deployments_docker.py(and related to the approach in #868) by observing live container state instead of reconstructing status from Docker events.Test plan
plugins/nemo-deployments/tests/unit/backends/docker/(73 passed)plugins/nemo-deployments/tests/integration/backends/docker/(3 passed)test_docker_job_deployment_reaches_succeededwith the existing fast alpine workload (no sleep)Summary by CodeRabbit
SUCCEEDED/FAILEDimmediately after container creation when the container has already exited.oneshot_observe_timeout_seconds(default 5, min 1) to limit the initial observe window before deferring to later status polling.Never/OnFailurestatus mapping using exit code, restart count, and backoff-related retry decisions.