fix(deployments): docker backend delivers config_files; unify agents config delivery - #1217
Merged
maxdubrinsky merged 4 commits intoAug 11, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe change moves agent configuration to ChangesDeployment configuration and Docker execution
Sequence Diagram(s)sequenceDiagram
participant DeploymentsBackend
participant DockerBackend
participant DockerContainer
DeploymentsBackend->>DockerBackend: create deployment container
DockerBackend->>DockerBackend: build ConfigFile tar archive
DockerBackend->>DockerContainer: put_archive configuration files
DockerBackend->>DockerContainer: start container
Possibly related PRs
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/backend.py (1)
594-602: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove failed init containers.
If
start()orwait()raises,container.remove()is skipped. The failed init container remains until another deployment attempt removes it. Move cleanup intofinallyafter creation.Proposed fix
def _run_and_wait() -> int: create_kwargs = {k: v for k, v in run_kwargs.items() if k != "detach"} container = self._client.containers.create(**create_kwargs) - container.start() - result = container.wait(timeout=self._executor_config.docker_timeout) - exit_code = self._exit_code_from_wait_result(result) try: + container.start() + result = container.wait(timeout=self._executor_config.docker_timeout) + return self._exit_code_from_wait_result(result) + finally: container.remove(force=True) except Exception: logger.warning("Failed to remove init container %s", init_name, exc_info=True) - return exit_code🤖 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 594 - 602, Ensure the init container created in this deployment flow is removed even when container.start() or container.wait() raises. Wrap the start/wait and exit-code handling in a try/finally, keeping container.remove(force=True) and its warning handling in the finally block after successful creation.
🧹 Nitpick comments (1)
plugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.py (1)
380-381: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign staged-file fixtures with the Fabric server path.
The affected tests mock
/workspace/agent.yaml, but the default Fabric server reads/tmp/nemo/agent.yaml. Use/tmp/nemoin both fixtures and assert thatagent.yamlmatches the server argument.🤖 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/src/nemo_agents_plugin/runner/deployments_backend.py` around lines 380 - 381, The staged-file fixtures in plugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.py:380-381 and plugins/nemo-agents/tests/unit/test_runner_deployments.py:978-982 must use /tmp/nemo instead of /workspace for agent.yaml. Update both fixtures and make the tests assert that agent.yaml matches the server argument used by the Fabric deployment path.
🤖 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 452-460: The container cleanup flow in the Docker backend must not
remove a container by name when containers.create fails. In the deployment
creation path around _remove_container_by_name, only clean up the container
object after creation succeeds and _deliver_config_files or container.start
fails; preserve the create-failure path without name-based removal. Update
plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py lines
149-165 to assert removal of server_container rather than a separately returned
leftover container.
---
Outside diff comments:
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py`:
- Around line 594-602: Ensure the init container created in this deployment flow
is removed even when container.start() or container.wait() raises. Wrap the
start/wait and exit-code handling in a try/finally, keeping
container.remove(force=True) and its warning handling in the finally block after
successful creation.
---
Nitpick comments:
In `@plugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.py`:
- Around line 380-381: The staged-file fixtures in
plugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.py:380-381
and plugins/nemo-agents/tests/unit/test_runner_deployments.py:978-982 must use
/tmp/nemo instead of /workspace for agent.yaml. Update both fixtures and make
the tests assert that agent.yaml matches the server argument used by the Fabric
deployment path.
🪄 Autofix
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: ecda3dd9-9ea0-46ef-b4cd-35f4037d2073
📒 Files selected for processing (7)
plugins/nemo-agents/src/nemo_agents_plugin/config.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.pyplugins/nemo-agents/tests/unit/test_runner_deployments.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/docker/test_idempotency.pyplugins/nemo-deployments/tests/unit/backends/docker/test_image_pull_auth.py
Contributor
|
maxdubrinsky
added a commit
that referenced
this pull request
Aug 10, 2026
…E-955) Address CodeRabbit review on #1217: - _run_server_container: only remove the container object returned by containers.create; never name-lookup after a create failure, which could force-remove another deployment's container that won the name. - _run_init_container: move container.remove into a finally so a start/wait raise still cleans up the init container. - Fabric create-deployment test fixtures: align staged-file paths with the /tmp/nemo default config_mount_path. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
…config delivery (AIRCORE-955) The docker deployments backend ignored DeploymentConfig.config_files, so the agents compiler delivered the NAT config through a NAT_CONFIG_YAML env var and reconstituted it with a shell one-liner. That docker-specific path made every other substrate inherit docker's config-delivery behavior, breaking OpenShell-backed deployments (nvbugs 6581239). Docker backend now delivers config_files via create + put_archive + start: the file is written into the container's own filesystem before the server command reads it, with no host-path coupling (correct when nmp-api itself runs in a container). Init and sidecar containers move to create + start for a uniform API; the volume-chmod docker run --rm stays on run. The agents mode == docker env-delivery branch is deleted; build_deployment_config always emits the direct server command and carries config_files on the DeploymentConfig, matching the k8s path. config_mount_path default moves from /workspace/config.yaml to /tmp/nemo/config.yaml: /tmp is the writable intersection of the docker container user, the openshell sandbox user (uid 999, cannot write /workspace), and k8s ConfigMap subPath (read-only). Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
…E-955) Address CodeRabbit review on #1217: - _run_server_container: only remove the container object returned by containers.create; never name-lookup after a create failure, which could force-remove another deployment's container that won the name. - _run_init_container: move container.remove into a finally so a start/wait raise still cleans up the init container. - Fabric create-deployment test fixtures: align staged-file paths with the /tmp/nemo default config_mount_path. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
maxdubrinsky
force-pushed
the
aircore-955-docker-config-files-delivery/mdubrinsky
branch
from
August 10, 2026 20:58
14e1b71 to
73da8d3
Compare
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 `@plugins/nemo-agents/tests/unit/test_runner_deployments.py`:
- Line 220: Update the default config_mount_path assertion in
DeploymentsRunnerConfig to use exact equality with the documented path
“/tmp/nemo/config.yaml” instead of a prefix check.
🪄 Autofix
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: 7a483116-a334-4dc4-9fb1-bbcf53037c50
📒 Files selected for processing (3)
plugins/nemo-agents/tests/unit/test_runner_deployments.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py
- plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py
A prefix check accepts an incorrect filename under /tmp/nemo/. Use exact equality so the test catches a wrong default. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
mckornfield
approved these changes
Aug 10, 2026
Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
maxdubrinsky
enabled auto-merge
August 11, 2026 16:37
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 (2)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py (2)
118-141: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPreserve
ConfigFile.modein the archive.The archive builder assigns mode
0644to every file.ConfigFile.modeis part of the deployment contract. A file declared0600becomes world-readable, and an executable file loses its execute bit. Set each file header fromconfig_file.modeand add tests for non-default modes.🤖 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 118 - 141, Update _config_files_tar to assign each file TarInfo.mode from the corresponding ConfigFile.mode instead of hardcoding 0644, preserving declared permissions including restrictive and executable modes. Add tests covering non-default ConfigFile.mode values and verify those modes are retained in the generated archive.
499-501: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winContain primary-container cleanup failures.
If
created.remove()fails,_run_server_containerraises beforecreate_deploymentreleases the allocated GPU. Catch cleanup failures, log them, and return the startup error. TreatNotFoundas already removed. Do not retry a port conflict when cleanup fails.🤖 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 499 - 501, Update the exception cleanup in _run_server_container to contain failures from created.remove: treat NotFound as already removed, log other cleanup errors, and return the original startup error instead of replacing it. Ensure create_deployment still releases the allocated GPU and does not retry a port conflict when cleanup fails.Source: MCP tools
🤖 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/backend.py`:
- Around line 118-141: Update _config_files_tar to assign each file TarInfo.mode
from the corresponding ConfigFile.mode instead of hardcoding 0644, preserving
declared permissions including restrictive and executable modes. Add tests
covering non-default ConfigFile.mode values and verify those modes are retained
in the generated archive.
- Around line 499-501: Update the exception cleanup in _run_server_container to
contain failures from created.remove: treat NotFound as already removed, log
other cleanup errors, and return the original startup error instead of replacing
it. Ensure create_deployment still releases the allocated GPU and does not retry
a port conflict when cleanup fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 77efc8f0-1428-418c-b85d-d37401a4eba4
📒 Files selected for processing (1)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py
maxdubrinsky
deleted the
aircore-955-docker-config-files-delivery/mdubrinsky
branch
August 11, 2026 17:08
This was referenced Aug 11, 2026
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.
Summary
The docker deployments backend ignored
DeploymentConfig.config_files, so the agents compiler delivered the NAT config through aNAT_CONFIG_YAMLenv var and reconstituted it with a shell one-liner. That docker-specific path made every other substrate inherit docker's config-delivery behavior, breaking OpenShell-backed deployments (nvbugs 6581239). Docker now deliversconfig_filesviacreate+put_archive+start, the agentsmode == "docker"env-delivery branch is deleted, andconfig_mount_pathmoves to/tmp/nemo/config.yaml(the writable intersection of all three substrates).Related Issue
Part of AIRCORE-955 / AIRCORE-1016.
Changes
docker/backend.py: new_config_files_tar()builds an in-memory tar (with dir entries) fromConfigFilelist._run_server_containernow doescontainers.create->put_archive("/", tar)->container.start(). Config is written into the container's own filesystem before the server command reads it, with no host-path coupling (correct when nmp-api itself runs in a container). Init and sidecar containers move tocreate+startfor a uniform API; the volume-chmoddocker run --rmstays onrun.runner/deployments_backend.py: deleted themode == "docker"env-delivery branch (NAT_CONFIG_YAML/STAGED_CONFIG_FILES_B64_JSON/sh -cmaterialization).build_deployment_configalways emits the direct server command and carriesconfig_fileson theDeploymentConfig, matching the k8s path. Removed dead helpers and unused imports (base64,json,shlex).config.py:config_mount_pathdefault/workspace/config.yaml->/tmp/nemo/config.yaml./tmpis writable by the docker container user, the openshell sandbox user (uid 999, cannot write/workspace), and k8s ConfigMap subPath (read-only).containers.create+container.startinstead ofcontainers.run.Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
uv run pre-commit run --files <changed paths>— all hooks pass (ruff, ruff format, ty, copyright, merge-conflict).uv run --frozen pytest plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py plugins/nemo-deployments/tests/unit/backends/docker/test_idempotency.py plugins/nemo-deployments/tests/unit/backends/docker/test_image_pull_auth.py plugins/nemo-agents/tests/unit/test_runner_deployments.py -q— 106 passed.uv run --frozen ty checkon changed backend files — all checks passed.Pre-existing failures confirmed identical on origin/main (not caused by this change): port-allocation tests (environment-dependent real port binding),
test_cli_list_output(timestamp substring), openshell integration test (AsyncEntitiesResourcemock), andtest_backend.pybasename collection conflict between k8s and openshell dirs.Summary by CodeRabbit