fix(models): make docker weights volume writable for the HF weight-puller - #853
Merged
Conversation
Contributor
|
The deployments-plugin docker backend created the model weights volume as a plain docker named volume, which the daemon creates root-owned (0755). The HF weight-puller runs as its image's default non-root user (e.g. `nvs`, uid 1000), so `hf download --local-dir /model-store` failed creating /model-store/.cache with PermissionError and the deployment errored with 'Prerequisite <dep>-puller failed'. Docker has no fs_group equivalent (that is k8s-only), so the volume stayed unwritable. This blocked all docker vLLM/NIM deployments that pull weights from the files service. Fix (docker analogue of a k8s fsGroup, keeping the puller non-root): - entities: add DockerVolumeConfig.init_chmod / init_image. - docker backend create_volume: when init_chmod is set, run a one-shot busybox container that chmods the freshly created volume so non-root workloads can write it (idempotent, safe to re-run on reconcile). - models compiler: on the docker runtime only, set the weights volume's docker backend config to initChmod=0777 with the configured busybox image. On k8s the volume is unchanged and pod securityContext/fs_group continues to handle it. Verified in a docker-backend dev deployment: the puller runs as its default non-root user and succeeds, the model reaches READY, and gateway chat-completion works for both vLLM and NIM engines. Signed-off-by: Ben McCown <bmccown@nvidia.com>
benmccown
force-pushed
the
models-e2e-cases
branch
from
July 23, 2026 15:57
c582d86 to
e8181b4
Compare
benmccown
changed the title
benmccown
marked this pull request as ready for review
July 23, 2026 16:05
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughDocker volume configuration now supports optional post-creation permission initialization. Docker weighted-puller compilation emits ChangesDocker volume initialization
Sequence Diagram(s)sequenceDiagram
participant DeploymentCompiler
participant DockerDeploymentBackend
participant DockerVolumeOperations
participant DockerEngine
DeploymentCompiler->>DockerDeploymentBackend: Emit Docker volume initialization settings
DockerDeploymentBackend->>DockerVolumeOperations: Forward init_chmod and init_image
DockerVolumeOperations->>DockerEngine: Create or reuse named volume
DockerVolumeOperations->>DockerEngine: Run helper container with chmod on /vol
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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
🧹 Nitpick comments (2)
services/core/models/tests/unit/controllers/backends/deployments_plugin/test_compiler.py (1)
42-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact helper image.
assert docker_cfg.init_imagewould pass for an incorrect image. Assert the configured BusyBox image value to protect this cross-layer contract.🤖 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/deployments_plugin/test_compiler.py` around lines 42 - 51, Update test_docker_weights_volume_requests_init_chmod to assert docker_cfg.init_image equals the configured BusyBox image value, rather than only checking that it is truthy; preserve the existing init_chmod assertion.plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py (1)
97-103: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUpdate this assertion with the shell-free fix.
The test currently locks in
sh -c. After hardening production code, assertentrypoint == ["chmod"]andcommand == ["0777", "/vol"].🤖 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 97 - 103, Update the assertions for mock_docker_client.containers.run in the relevant test to verify the shell-free invocation: assert entrypoint equals ["chmod"] and command equals ["0777", "/vol"]. Remove the existing command assertion that expects ["chmod 0777 /vol"], while preserving the remove and volume-mount assertions.
🤖 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/volumes.py`:
- Around line 48-53: Remove shell evaluation from volume initialization in
volumes.py around client.containers.run: validate init_chmod as an allowed chmod
mode, invoke chmod directly instead of through sh -c, and pass the mode and /vol
as separate arguments. Update test_backend_mocked.py to assert the direct chmod
entrypoint and argument list.
---
Nitpick comments:
In `@plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py`:
- Around line 97-103: Update the assertions for
mock_docker_client.containers.run in the relevant test to verify the shell-free
invocation: assert entrypoint equals ["chmod"] and command equals ["0777",
"/vol"]. Remove the existing command assertion that expects ["chmod 0777 /vol"],
while preserving the remove and volume-mount assertions.
In
`@services/core/models/tests/unit/controllers/backends/deployments_plugin/test_compiler.py`:
- Around line 42-51: Update test_docker_weights_volume_requests_init_chmod to
assert docker_cfg.init_image equals the configured BusyBox image value, rather
than only checking that it is truthy; preserve the existing init_chmod
assertion.
🪄 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: e74901a1-0714-4eff-900e-2b1af018eebb
📒 Files selected for processing (6)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/volumes.pyplugins/nemo-deployments/src/nemo_deployments_plugin/entities.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/compiler.pyservices/core/models/tests/unit/controllers/backends/deployments_plugin/test_compiler.py
mckornfield
approved these changes
Jul 23, 2026
tylersbray
approved these changes
Jul 23, 2026
tylersbray
left a comment
tylersbray
left a comment
Contributor
There was a problem hiding this comment.
+1 approving, see DM for loose ends. Thanks!
- Invoke chmod directly (entrypoint=["chmod"], command=[mode, "/vol"]) instead of via `sh -c`, so a mode value is never shell-interpolated (defense in depth even though the only caller passes a hardcoded '0777'). - Only run the init-chmod container on a fresh volume create, not on reuse, so reconciles don't spin up a helper container for an already-initialized volume. - Add a comment explaining why chmod (uid-agnostic) is used over chown. - Shorten the DockerVolumeConfig field descriptions and drop 'one-shot' wording. - Tests: assert the shell-free chmod invocation, the exact busybox init image, and that init-chmod is skipped when the volume already exists. Signed-off-by: Ben McCown <bmccown@nvidia.com>
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 23, 2026
AnuradhaKaruppiah
pushed a commit
to AnuradhaKaruppiah/nemo-platform
that referenced
this pull request
Jul 24, 2026
…ller (NVIDIA-NeMo#853) * fix(models): make docker weights volume writable for the HF puller The deployments-plugin docker backend created the model weights volume as a plain docker named volume, which the daemon creates root-owned (0755). The HF weight-puller runs as its image's default non-root user (e.g. `nvs`, uid 1000), so `hf download --local-dir /model-store` failed creating /model-store/.cache with PermissionError and the deployment errored with 'Prerequisite <dep>-puller failed'. Docker has no fs_group equivalent (that is k8s-only), so the volume stayed unwritable. This blocked all docker vLLM/NIM deployments that pull weights from the files service. Fix (docker analogue of a k8s fsGroup, keeping the puller non-root): - entities: add DockerVolumeConfig.init_chmod / init_image. - docker backend create_volume: when init_chmod is set, run a one-shot busybox container that chmods the freshly created volume so non-root workloads can write it (idempotent, safe to re-run on reconcile). - models compiler: on the docker runtime only, set the weights volume's docker backend config to initChmod=0777 with the configured busybox image. On k8s the volume is unchanged and pod securityContext/fs_group continues to handle it. Verified in a docker-backend dev deployment: the puller runs as its default non-root user and succeeds, the model reaches READY, and gateway chat-completion works for both vLLM and NIM engines. Signed-off-by: Ben McCown <bmccown@nvidia.com> * refactor(models): address PR review on docker volume init-chmod - Invoke chmod directly (entrypoint=["chmod"], command=[mode, "/vol"]) instead of via `sh -c`, so a mode value is never shell-interpolated (defense in depth even though the only caller passes a hardcoded '0777'). - Only run the init-chmod container on a fresh volume create, not on reuse, so reconciles don't spin up a helper container for an already-initialized volume. - Add a comment explaining why chmod (uid-agnostic) is used over chown. - Shorten the DockerVolumeConfig field descriptions and drop 'one-shot' wording. - Tests: assert the shell-free chmod invocation, the exact busybox init image, and that init-chmod is skipped when the volume already exists. Signed-off-by: Ben McCown <bmccown@nvidia.com> --------- Signed-off-by: Ben McCown <bmccown@nvidia.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.
Summary
The deployments-plugin docker backend created the model weights volume as a plain docker named volume, which the daemon creates root-owned (0755). The HF weight-puller runs as its image's default non-root user (e.g.
nvs, uid 1000), sohf download --local-dir /model-storefailed creating/model-store/.cachewithPermissionError [Errno 13], and the deployment errored withPrerequisite '<dep>-puller' failed.Docker has no
fs_groupequivalent (that is k8s-only), so the volume stayed unwritable. This blocked all docker vLLM/NIM deployments that pull weights from the files service.Fix
Docker analogue of a k8s
fsGroup, keeping the puller non-root (least privilege):entities.py— addDockerVolumeConfig.init_chmod/init_image.backends/docker/volumes.py+backend.py— increate_volume, wheninit_chmodis set, run a one-shot busybox container thatchmods the freshly created volume so non-root workloads can write it. Idempotent, so re-running on a reconcile is harmless.deployments_plugin/compiler.py— on the docker runtime only, set the weights volume's docker backend config toinitChmod=0777with the configured busybox image. On k8s the volume is unchanged and podsecurityContext/fs_groupcontinues to handle it.busybox is already a first-class dependency of this plugin (used by the existing
lora-cache-initcontainer), so this introduces no new concept.Tests
New unit tests (full deployments-plugin unit suite: 273 passed; models deployments_plugin suite green):
test_create_volume_runs_init_chmod_container,test_create_volume_without_init_chmod_skips_containertest_docker_weights_volume_requests_init_chmod,test_k8s_weights_volume_has_no_docker_init_chmodManual verification (dev pod, docker backend)
nvs) and exits 0 (all files downloaded) → server READY (http probe 200) → gateway chat-completion works. Previously blocked.k8s vLLM/NIM (base + LoRA) were already working and are unaffected (no
initChmodemitted on k8s).Notes
Summary by CodeRabbit
New Features
initChmod/initImagealiases).Bug Fixes
Tests