fix(deployments): docker backend delivers config_files; delete agents docker env bodge - #1216
Closed
maxdubrinsky wants to merge 1 commit into
Closed
fix(deployments): docker backend delivers config_files; delete agents docker env bodge#1216maxdubrinsky wants to merge 1 commit into
maxdubrinsky wants to merge 1 commit into
Conversation
… docker env bodge (AIRCORE-955) The docker deployments backend ignored DeploymentConfig.config_files, so the agents compiler smuggled the NAT config through NAT_CONFIG_YAML and reconstituted it with a shell one-liner. That bodge inherited docker's config-delivery behavior on every other substrate, 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-smuggle 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). Items 1 and 2 (openshell config_files delivery and readiness gating) already landed in #1145 and #1139. Item 3 (NAT on PATH inside openshell) is a separable openshell-specific gap tracked separately. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Contributor
Author
|
Closing to revise commit history; will reopen as a new draft. |
Contributor
|
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
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 smuggled the NAT config through aNAT_CONFIG_YAMLenv var and reconstituted it with a shell one-liner. That bodge 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-smuggle 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. Items 1 (openshell
config_filesdelivery) and 2 (openshell readiness gating) already landed in #1145 and #1139. Item 3 (NAT on PATH inside openshell) is a separable openshell-specific gap tracked separately.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-smuggle 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.