fix(nemo-deployments): race condition in controller#868
Conversation
📝 WalkthroughWalkthroughDocker status handling now recovers exit codes from recent Docker ChangesDocker status recovery
Sequence Diagram(s)sequenceDiagram
participant DockerDeploymentBackend
participant DockerAPI
participant map_exited_status
DockerDeploymentBackend->>DockerAPI: Query recent die events
DockerAPI-->>DockerDeploymentBackend: Return exitCode and restart attributes
DockerDeploymentBackend->>map_exited_status: Map recovered exitCode
map_exited_status-->>DockerDeploymentBackend: Return status update
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py (1)
124-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for
restart_policy="OnFailure"removed-container recovery.The new recovery logic in
backend.pytriggers for both"Never"and"OnFailure"(Line 290:if restart_policy in ("Never", "OnFailure")), but both test suites only exercise"Never".
plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py#L124-L188: add a mocked test withrestart_policy="OnFailure"and a non-zero recovered exit code to verify the mapped status (and surface the backoff-bypass behavior flagged inbackend.py).plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py#L119-L162: add an OnFailure-policy variant of the removed-container integration test using a failing command.🤖 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/docker/test_backend_mocked.py` around lines 124 - 188, Extend the mocked tests around test_read_status_recovers_removed_never_container_exit_code and test_read_status_failed_when_missing_never_has_no_exit_event in plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py (lines 124-188) with an OnFailure case using a non-zero recovered Docker die-event exit code and assertions for its mapped status and exit code. Add the corresponding OnFailure removed-container integration-test variant using a failing command in plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py (lines 119-162).
🤖 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/docker/backend.py`:
- Around line 399-418: The _read_removed_container_status method must preserve
OnFailure retry behavior instead of directly using map_exited_status for
non-zero exits. Apply the same restart_count/backoff_limit handling used for
in-place container exits so eligible failures return STARTING and retries
continue, while exhausted retries retain the terminal FAILED status.
- Around line 399-453: The current _latest_removed_container_exit_code method
depends on Docker’s bounded one-hour event replay and can miss recent container
exits. Capture and retain each container’s exit code when NotFound is first
detected during read_status, or maintain an equivalent live per-container
die-event cache, then have _read_removed_container_status use that recorded
value before falling back to missing_container_status; preserve existing status
mapping and recovery metadata.
---
Nitpick comments:
In `@plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py`:
- Around line 124-188: Extend the mocked tests around
test_read_status_recovers_removed_never_container_exit_code and
test_read_status_failed_when_missing_never_has_no_exit_event in
plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py
(lines 124-188) with an OnFailure case using a non-zero recovered Docker
die-event exit code and assertions for its mapped status and exit code. Add the
corresponding OnFailure removed-container integration-test variant using a
failing command in
plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py
(lines 119-162).
🪄 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: 2a4007df-5e5c-4fdd-a755-a09492bea6f1
📒 Files selected for processing (3)
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.py
|
Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
affb68f to
fd3772a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/docker/backend.py`:
- Around line 633-682: Update _read_removed_container_status to accept a dep_key
parameter and release the GPU via self._gpu_pool.release_gpu(dep_key) whenever
recovered exit handling reaches a terminal status, including success and
non-retryable failure, while preserving the retry/STARTING path without release.
Pass deployment_key(workspace, name) from the read_status call site.
- Around line 656-671: Update the OnFailure handling in backend.py around the
restart_policy check to read RestartCount from cached inspect state or another
persisted pre-removal source rather than die-event attrs, preserving the
backoff-limit behavior. In test_backend_mocked.py lines 507-543 and 546-581,
update both unit cases to provide/assert the real persisted restart-count signal
instead of placing restartCount in event attributes.
🪄 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: 098f3f89-e0e0-4944-aa9a-0515eea3c3e6
📒 Files selected for processing (3)
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.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py
| async def _read_removed_container_status( | ||
| self, | ||
| *, | ||
| container_name: str, | ||
| restart_policy: RestartPolicy, | ||
| ) -> BackendStatusUpdate | None: | ||
| attrs = await asyncio.to_thread(self._latest_removed_container_attrs, container_name) | ||
| if attrs is None: | ||
| return None | ||
|
|
||
| exit_code = _int_attr(attrs, "exitCode") | ||
| if exit_code is None: | ||
| raw_exit_code = attrs.get("exitCode") | ||
| if raw_exit_code is not None: | ||
| logger.warning( | ||
| "Docker die event for removed container %s has invalid exitCode %r", | ||
| container_name, | ||
| raw_exit_code, | ||
| ) | ||
| return None | ||
| logger.warning("Docker die event for removed container %s has no exitCode", container_name) | ||
| return None | ||
|
|
||
| if restart_policy == "OnFailure" and exit_code != 0: | ||
| restart_count = _int_attr(attrs, "RestartCount", "restartCount", default=0) | ||
| backoff_limit = _int_attr(attrs, BACKOFF_LIMIT_LABEL, default=6) | ||
| if restart_count < backoff_limit: | ||
| return BackendStatusUpdate( | ||
| status="STARTING", | ||
| status_message=( | ||
| f"Container exited (code {exit_code}) before it was removed; " | ||
| f"retry {restart_count}/{backoff_limit}" | ||
| ), | ||
| exit_code=exit_code, | ||
| error_details={ | ||
| "expected_container_name": container_name, | ||
| "recovered_from": "docker_die_event", | ||
| }, | ||
| ) | ||
|
|
||
| status = map_exited_status(exit_code, restart_policy) | ||
| return BackendStatusUpdate( | ||
| status=status, | ||
| status_message=f"Container exited with code {exit_code} before it was removed", | ||
| exit_code=exit_code, | ||
| error_details={ | ||
| "expected_container_name": container_name, | ||
| "recovered_from": "docker_die_event", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
GPU never released when recovery reaches a terminal state.
The in-place exited path (lines 536-575) explicitly calls self._gpu_pool.release_gpu(dep_key) on every terminal outcome (success, non-retryable failure). _read_removed_container_status mirrors that terminal-state logic (recovered exit code → SUCCEEDED/FAILED) but never releases the GPU. A Never/OnFailure deployment whose container is removed and recovered as terminal will hold its GPU allocation until delete_deployment is eventually called, unlike the in-place path which frees it immediately on detecting the terminal state.
_read_removed_container_status doesn't receive workspace/name, but both are available at the call site in read_status (lines 466-472).
🔧 Proposed fix: release GPU on recovered terminal states
async def _read_removed_container_status(
self,
*,
container_name: str,
restart_policy: RestartPolicy,
+ dep_key: str | None = None,
) -> BackendStatusUpdate | None:
attrs = await asyncio.to_thread(self._latest_removed_container_attrs, container_name)
if attrs is None:
return None
@@
if restart_policy == "OnFailure" and exit_code != 0:
restart_count = _int_attr(attrs, "RestartCount", "restartCount", default=0)
backoff_limit = _int_attr(attrs, BACKOFF_LIMIT_LABEL, default=6)
if restart_count < backoff_limit:
return BackendStatusUpdate(
status="STARTING",
...
)
+ if dep_key is not None and self._gpu_pool is not None:
+ self._gpu_pool.release_gpu(dep_key)
status = map_exited_status(exit_code, restart_policy)And pass dep_key=deployment_key(workspace, name) from the read_status call site.
🤖 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 633 - 682, Update _read_removed_container_status to accept a
dep_key parameter and release the GPU via self._gpu_pool.release_gpu(dep_key)
whenever recovered exit handling reaches a terminal status, including success
and non-retryable failure, while preserving the retry/STARTING path without
release. Pass deployment_key(workspace, name) from the read_status call site.
| if restart_policy == "OnFailure" and exit_code != 0: | ||
| restart_count = _int_attr(attrs, "RestartCount", "restartCount", default=0) | ||
| backoff_limit = _int_attr(attrs, BACKOFF_LIMIT_LABEL, default=6) | ||
| if restart_count < backoff_limit: | ||
| return BackendStatusUpdate( | ||
| status="STARTING", | ||
| status_message=( | ||
| f"Container exited (code {exit_code}) before it was removed; " | ||
| f"retry {restart_count}/{backoff_limit}" | ||
| ), | ||
| exit_code=exit_code, | ||
| error_details={ | ||
| "expected_container_name": container_name, | ||
| "recovered_from": "docker_die_event", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding code.
ast-grep outline plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py --view expanded || true
sed -n '620,700p' plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py
echo
echo "---- tests ----"
sed -n '480,610p' plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py
echo
echo "---- restart count references ----"
rg -n "RestartCount|restartCount|BACKOFF_LIMIT_LABEL|die event|events.return_value" plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker plugins/nemo-deployments/tests/unit/backends/docker -SRepository: NVIDIA-NeMo/nemo-platform
Length of output: 12888
🌐 Web query:
Docker die event RestartCount event attributes docker events inspect RestartCount
💡 Result:
In Docker, the RestartCount is not an attribute directly available in the event stream for a die event [1]. Instead, it is a property tracked within the container's state, which can be retrieved by inspecting the container [1][2][3]. Docker Events and RestartCount The die event signifies that a container has stopped [4]. While die events do carry attributes—such as exitCode and execDuration [5]—they do not natively include the current RestartCount [1][5]. As of recent updates (circa 2025), a restarting attribute may be added to die events to indicate that a container is scheduled to restart [6], but this differs from providing the numerical restart count itself. To track restarts via events, you must monitor the stream and maintain a counter externally, or use the docker inspect command to query the current state [1]. Retrieving RestartCount via docker inspect The RestartCount is a field within the container's low-level configuration, which you can access using the docker inspect command [1][3][7]. You can retrieve the current restart count for a container using the --format flag: docker inspect --format '{{.RestartCount}}' <container_id_or_name> You can also include it in a formatted list of multiple containers [1]: docker inspect --format "ID: {{.ID}} | Name: {{.Name}} | Restarts: {{.RestartCount}}" $(docker ps -aq) Note that the RestartCount is a volatile counter managed by the Docker daemon [1]. If the Docker daemon restarts or the host reboots, the RestartCount for existing containers is typically reset to 0 [1].
Citations:
- 1: FEATURE REQUEST: Get container restart count moby/moby#25859
- 2: https://github.com/moby/moby/blob/d74daf1a/api/types/container/container.go
- 3: https://manpages.ubuntu.com/manpages/plucky/man1/docker-inspect.1.html
- 4: https://docs.docker.com/reference/cli/docker/system/events/
- 5: https://github.com/moby/moby/blob/master/daemon/monitor.go
- 6: Add
restartingattribute ondieevents moby/moby#50908 - 7: https://manpages.ubuntu.com/manpages/jammy/man1/docker-inspect.1.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '520,575p' plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py
echo
sed -n '684,750p' plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py
echo
sed -n '450,640p' plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyRepository: NVIDIA-NeMo/nemo-platform
Length of output: 11889
Read RestartCount from inspect state, not removed-container event data
die events don’t include restartCount, so this path falls back to 0 on a real daemon and keeps OnFailure containers in STARTING instead of failing once the backoff limit is hit. The two unit tests also fake restartCount in event attributes, so they don’t exercise the real payload.
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py#L656-L671: source the count from cached inspect state or another persisted source before removal.plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py#L507-L581: update both cases to use the real signal.
📍 Affects 2 files
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py#L656-L671(this comment)plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py#L507-L543plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py#L546-L581
🤖 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 656 - 671, Update the OnFailure handling in backend.py around the
restart_policy check to read RestartCount from cached inspect state or another
persisted pre-removal source rather than die-event attrs, preserving the
backoff-limit behavior. In test_backend_mocked.py lines 507-543 and 546-581,
update both unit cases to provide/assert the real persisted restart-count signal
instead of placing restartCount in event attributes.
1 participant
Summary by CodeRabbit
dieevents.exitCodedata.