fix(trtllm): restore deps, default CMD, and reduce layer count after upstream base switch - #9889
Conversation
WalkthroughThis PR adds openssh-server and RDMA-related OS packages ( ChangesDocker Runtime Packages
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR #9654 cleared upstream's ENTRYPOINT (/opt/nvidia/nvidia_entrypoint.sh) to let derived runtimes execute arbitrary commands directly, but did not set a CMD. Upstream tensorrt-llm/release has no CMD either, so bare `docker run <image>` now fails with "no command specified" — breaking downstream pipelines that smoke-test the image without passing an explicit command. K8s deploy manifests are unaffected: they set `command:` and `args:` explicitly, which override CMD. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The runtime stage previously had nine sequential COPY commands
(tests, examples, deploy, dev, four components subdirs, lib) — each
its own image layer. Stacked on the upstream tensorrt-llm/release
base (226 layers) plus other Dynamo adds, the resulting image was at
~265 layers, and Dockerfile.test's test_distroless stage pushed past
the BuildKit/overlayfs depth limit ("max depth exceeded") in
downstream CI.
Gather all nine sources in a scratch transport stage and pull them
into the runtime stage with a single cross-stage COPY. Drops the
runtime image from 265 to 258 layers; test_distroless from ~272 to
268.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Builds on the earlier workspace-COPY collapse (commit a39970f). Same goal: reduce overlayfs depth on top of the 226-layer upstream tensorrt-llm/release base so downstream Dockerfile.test builds don't trip the runner's max_lowerdir limit. Six low-risk merges in the runtime stage: 1. Combine the five ENVs (DYNAMO_HOME, HOME, PATH, LD_PRELOAD, NIXL_PLUGIN_DIR) and the trailing DYNAMO_COMMIT_SHA ENV into one multi-key ENV. Move ARG DYNAMO_COMMIT_SHA up alongside the other three ARGs. 2. Fold the LD_PRELOAD sanity assert, the ldconfig conf RUN, and the `rm -f /usr/local/bin/etcd` RUN into a single root-context RUN. 3. Introduce a dynamo_base_export scratch transport stage (same pattern as workspace_files) so the three dynamo_base COPYs become one cross-stage `COPY / /`. Note: place uv/uvx at /usr/bin rather than /bin because upstream is usrmerged and BuildKit can't cross- stage COPY through the /bin symlink. 4. Append the venv creation onto the useradd RUN under the same non-dev Jinja gate. 5. Move the workspace_files COPY before the USER dynamo switch and merge the two launch-screen RUNs (sed + chmod + echo) into one root-context RUN, eliminating two USER flips. Runtime image: 258 → 245 layers (-13). Dockerfile.test test_distroless target: 268 → 255 layers (-13). All checks still pass: sshd, nats-server, etcd, uv/uvx on PATH; librdmacm.so.1 loadable via ldconfig; workspace tests/examples/ deploy/lib/components subdirs all present and owned dynamo:0; launch_screen wired into /etc/bash.bashrc; final user is dynamo:0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
NVIDIA's `jet` CI runner is a PyInstaller-packaged binary that bundles its own libstdc++.so.6 from GCC 10 or earlier into its _MEI extraction dir. Pre-#9654 our trtllm image shipped a matching libstdc++; post- #9654 the upstream nvcr.io/nvidia/tensorrt-llm/release:1.3.0rc14 base is Ubuntu 24.04 + GCC 13.2, and its libnixl/libnixl_build/libserdes/ libnixl_common need GLIBCXX_3.4.{29,30,32} and CXXABI_1.3.13 — symbols jet's bundled libstdc++ lacks. PyInstaller prepends _MEI/ to LD_LIBRARY_PATH so the older bundled copy wins by default, leading to a flood of "version `GLIBCXX_3.4.32' not found" errors when jet dlopens any TRT-LLM / NVDA NIXL library. Force-load the system libstdc++ via LD_PRELOAD. Once it's resolved in the process, the SONAME is satisfied and subsequent dlopens of libnixl etc. find the newer symbols already there; jet's _MEI copy is silently shadowed. - Add a stable arch-independent symlink /opt/dynamo/libstdc++.so.6 -> /usr/lib/${ARCH_ALT}-linux-gnu/libstdc++.so.6 in the existing ldconfig RUN (zero new layers, ARCH_ALT already in scope there). - Prepend that path to the LD_PRELOAD entry in the unified ENV. - Drop the test -f "${LD_PRELOAD}" assert (now multi-path); test the libnixl.so absolute path directly instead. Long-term fix is on the jet side: rebuild against a newer libstdc++ or stop bundling it so jet inherits from the host. This is a container-side workaround until that lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collapse the 4-item numbered comment above the ldconfig RUN into a single 5-line block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Issues / Suggestions
1. Apt cache mount only covers /var/cache/apt, missing /var/lib/apt
The new RUN block (diff lines for the apt install):
RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \
apt-get update && ...
/var/cache/apt caches downloaded .deb files; the package index lives at /var/lib/apt/lists and is re-downloaded by apt-get update every build.
Adding a second mount:
RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \
--mount=type=cache,target=/var/lib/apt,sharing=locked \
apt-get update && ...
avoids the index re-download on iterative builds (and you can keep the rm -rf /var/lib/apt/lists/* at the end since it operates on a cache-mounted
path that doesn't persist into the layer anyway). Minor optimization, but free.
2. librdmacm1 is a transitive dep of rdma-core
apt-cache depends rdma-core on Ubuntu 24.04 (the upstream TRT-LLM 1.3.x base) lists librdmacm1 as a dependency. Specifying both is harmless but
redundant — could drop librdmacm1 from the explicit list. Pedantic; leave it if you prefer the explicit documentation.
3. COPY --chmod=775 dev /dev in scratch stage is technically fine but visually loaded
/dev in a FROM scratch is just a regular dirname (scratch has no /dev devfs convention), but a reader skimming the Dockerfile will pause on it.
Consider staging under a non-special prefix:
FROM scratch AS workspace_files
COPY --chmod=775 tests /workspace_src/tests
...
COPY --chmod=775 dev /workspace_src/dev
and copying with COPY --from=workspace_files /workspace_src/ /workspace/. Tradeoff is one extra path component for clarity. Style call.
4. Inline {% if %} at end of RUN-block continuation
&& echo 'umask 002' > /etc/profile.d/00-umask.sh{% if target not in ("dev", "local-dev") %} \
&& python3 -m venv --system-site-packages /opt/dynamo/venv \
&& ln -sf /usr/bin/uv /opt/dynamo/venv/bin/uv{% endif %}
Mixing Jinja conditionals at the very end of a shell-continuation line is harder to read than the original "separate {% if %} RUN ... {% endif %}
block". Functionally identical — but the original separation made it obvious that venv creation is a non-dev-only step. Maybe worth keeping the venv
RUN as its own conditional block (one extra layer for non-dev only, but improves grep-ability).
5. COPY --from=workspace_files / /workspace/ is implicit-include-by-default
Any future addition to the workspace_files scratch stage will land in /workspace/ of the runtime, whether intended or not. Mostly fine, but worth a
one-line comment in the scratch stage to flag it ("everything copied into this stage ends up in /workspace/ — keep paths minimal"). Adjacent to the
existing transport-stage comment.
6. Verify dev/local-dev still builds
The workspace_files and dynamo_base_export scratch stages run for every target including dev/local-dev. They will build, but the runtime stage's
flow has been rearranged: the workspace COPY now happens at a fixed point regardless of target. Worth a manual build check for target=dev and
target=local-dev to confirm no breakage (the test plan covers standard CI but not dev images explicitly).
Security
- openssh-server in a container image is non-trivial. It installs /usr/sbin/sshd and (per the PR description) generates host keys via postinst. This
is intentional and matches the multi-node MPI use case, but it does introduce a daemon binary that some org-level container scanners flag (e.g., a
future SBOM/CVE scan on the runtime image will now also scan openssh server-side bits). Not a blocker — call it out so reviewers don't get surprised
later by scanner output. Upstream tensorrt-llm/release deliberately ships only openssh-client; we're re-broadening attack surface here in exchange
for MPI capability.
- librdmacm1 / rdma-core are RDMA userland — no new exposure for normal workloads.
Performance
- Net −7 runtime layers / −4 test_distroless layers per the PR description, which matches my read of the diff (9 workspace COPYs → 1, 3 dynamo_base
COPYs → 1, 3 prep RUNs → 1, two USER toggles eliminated). The arithmetic checks out.
- Apt cache mount is good; adding /var/lib/apt cache (point #1) would close the loop.
…che-friendly SHA placement - Apt cache mount: add /var/lib/apt cache target so apt-get update can reuse the index between builds. Drop the now-redundant rm -rf /var/lib/apt/lists/* since the cache mount unmounts the path before the layer is finalized. - Move ARG DYNAMO_COMMIT_SHA + ENV DYNAMO_COMMIT_SHA to just before ENTRYPOINT. SHA changes per commit; putting it at the top busted every downstream layer's cache (the unified ENV referenced it). At the bottom only the trailing 2 layers invalidate per commit, so the apt/wheel/workspace COPYs stay cached. +1 layer for a major cache-hit-rate improvement. - workspace_files scratch stage: prefix sources with /workspace_src/ instead of root paths. /dev at scratch is technically fine but visually loaded; runtime now pulls /workspace_src/ explicitly. - Trim comment volume on the transport stages, the useradd RUN, and the bottom SHA block. Keep the load-bearing NIXL/jet/ldconfig context. Runtime image: 245 → 246 layers. Trade is intentional for cache reuse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dynamo-ops
left a comment
There was a problem hiding this comment.
Previous review comments have been addressed. Approving.
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Inline notes on the new commits — see the separate top-level comment for non-line-anchored concerns.
Remaining concerns from the new commits (not line-anchored)Follow-up on the four new commits since the last pass ( Concerns addressed since prior review
Outstanding non-line concerns1.
|
|
@dmitry-tokarev-nv Reverting the squash. We believe a change on dynamo-ci should bring the layer counts just under threshold. |
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Follow-up inline notes after the squash revert — concerns are tracked in the parent comment thread with bucket labels.
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
🔴 Blocking — verify or fix before merge
- Wheel persistence for dev/local-dev — possible regression (see inline comment on line 105)
- Pre-PR the wheel COPY was unconditional; now the cp is inside the non-dev Jinja guard.
- test_kvbm_imports.py greps the wheelhouse — needs confirmation it runs runtime-only.
- Dev/local-dev/arm64 build verification (not line-anchored — belongs in test plan)
- Just three checkboxes added and exercised.
🟡 Recommended — fix in this PR
-
libstdc++ symlink target not asserted (see inline comment on line 79, with suggestion)
-
PR description is stale (not line-anchored — PR body refresh)
-
jet lacks in-file context (see inline comment on lines 57–58, with suggestion)
🟢 Nits — take or leave
-
librdmacm1 listed alongside rdma-core (prior inline comment on line 64 still applies — not re-posted)
-
chmod -R g+w /workspace is a no-op (see inline comment on line 146, with suggestion)
-
Inline {% if %} in shell-continuation (lines 96–98) (no inline — too stylistic for a clean suggestion)
Co-authored-by: Dmitry Tokarev <dtokarev@nvidia.com> Signed-off-by: Tanmay Verma <tanmay2592@gmail.com>
Co-authored-by: Dmitry Tokarev <dtokarev@nvidia.com> Signed-off-by: Tanmay Verma <tanmay2592@gmail.com>
Co-authored-by: Dmitry Tokarev <dtokarev@nvidia.com> Signed-off-by: Tanmay Verma <tanmay2592@gmail.com>
Add a `FROM ${RUNTIME_IMAGE} AS runtime` stage (target=runtime only)
that overlays the layered `runtime_full` filesystem as a single COPY
layer. The image inherits upstream's full ENV/WORKDIR/USER/CMD config
via FROM, so only Dynamo-specific env needs redeclaring. A preceding
`rm -rf /workspace /home/ubuntu /usr/local/bin/etcd` whiteouts the
paths runtime_full deletes, since COPY can't represent deletions
(overlay shadowing would otherwise leak upstream's content).
Dev/local-dev targets keep the layered runtime (no rebase), so dev
iteration speed is unaffected.
Result: 108 → 104 fs layers in the production runtime image; upstream
env (OPAL_PREFIX, CUDA_HOME, …) inherits via FROM with no redeclaration.
Verified locally with the disagg smoke test on Qwen3-0.6B — prefill→
decode handoff fires and /v1/chat/completions returns 200.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Major concerns addressed! Non-critical items below:
Of my prior eight buckets, four are now closed (inline suggestions all accepted). Status of the remaining four:
┌─────┬──────────────────────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ # │ Item │ Status │
├─────┼──────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ 2 │ Dev/local-dev/arm64 build │ Still missing from test plan. Worth one explicit checklist round before merge — the dev path is now │
│ │ verification │ meaningfully different from the runtime path (dev keeps layered, runtime gets rebased). │
├─────┼──────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ 4 │ PR description stale │ Now even more stale — needs an "Architecture" paragraph mentioning the runtime_full + rebase split. │
├─────┼──────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ 6 │ librdmacm1 redundant with │ Pedantic, ignored as expected. │
│ │ rdma-core │ │
├─────┼──────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ 8 │ Inline {% if %} in shell │ Still present (lines 104–106). Stylistic. │
│ │ continuation │ │
└─────┴──────────────────────────────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────┘
…upstream base switch (ai-dynamo#9889) Signed-off-by: Tanmay Verma <tanmay2592@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Dmitry Tokarev <dtokarev@nvidia.com>
…upstream base switch (#9889) Signed-off-by: Tanmay Verma <tanmay2592@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Dmitry Tokarev <dtokarev@nvidia.com>
Three regressions from PR #9654 (TRT-LLM upstream base switch).
1. Restore missing system packages (
openssh-server,librdmacm1,rdma-core)#9654 dropped Dynamo's explicit apt block on the assumption upstream
nvcr.io/nvidia/tensorrt-llm/releasehad everything we need. Three packages turned out to be missing:openssh-server— upstream shipsopenssh-clientbut nosshd. Breaks multi-node MPI-over-SSH (kimi-k2.5 K8s benchmark).librdmacm1+rdma-core— RDMA Connection Manager userspace; needed for NCCL IB transport and UCX rdmacm mode.Verified empirically against
tensorrt-llm/release:1.3.0rc14. Everything else from the pre-#9654 apt block (build-essential,python3.12-dev,python3-pip,jq,numactl,libibverbs1,libnuma1,libcudnn9-cuda-13,libnvshmem3-cuda-13,libzmq3-dev,openssh-client,curl,git, etc.) is correctly provided by upstream and intentionally not re-added.After local rebuild:
sshdis on PATH at/usr/sbin/sshd, host keys generated by postinst,librdmacm.so.1loadable via ldconfig,rdma-corepackage installed.2. Set default
CMD ["/bin/bash"]#9654 cleared upstream's
ENTRYPOINT(/opt/nvidia/nvidia_entrypoint.sh) to let derived runtimes execute arbitrary commands directly, but did not set a CMD. Upstreamtensorrt-llm/releasehas no CMD either, so baredocker run <image>now fails withError response from daemon: no command specified— breaking downstream pipelines that smoke-test the image without passing an explicit command.K8s deploy manifests are unaffected: they set
command:andargs:explicitly, which override CMD.3. Reduce layer count to clear overlay2's 128-layer cap
Downstream wrapper images (
tensorrt-llm_dynamo_trtllm-runtime-with-k8s-wrapper-arm64-benchmarks) were hittingdocker: failed to register layer: max depth exceededat pull time because the runtime image (~111 layers post-#9654) plus the wrapper's additions (~20 layers) plus the upstream PyTorch base (101 layers underneath) was sitting near or above overlay2's 128-layer cap on some hosts.Architecture: runtime_full + rebase split
The template now produces two different stage chains depending on the build target:
target=runtime(production image): a layeredruntime_fullstage does all the build work (apt install, ldconfig, wheels, workspace_src cp), then a finalFROM ${RUNTIME_IMAGE} AS runtimestage rebases byCOPY --from=runtime_full / /overlaying the entire filesystem as a single layer. Upstream image config (ENV/WORKDIR/USER/CMD — ~40 NVIDIA/CUDA/MPI vars) inherits automatically viaFROM upstream; only Dynamo-specific env (DYNAMO_HOME,LD_PRELOAD,NIXL_PLUGIN_DIR,VIRTUAL_ENV,PATH) is redeclared. ARUN rm -rf /workspace /home/ubuntu /usr/local/bin/etcdbefore the COPY whiteouts upstream paths thatruntime_fulldeleted (COPY can't represent deletions).target=dev/target=local-dev: the original layeredruntimestage stays as-is;dev/dynamo_toolsbuild directlyFROM runtimefor fast iteration. No rebase. Wheelhouse COPY is unconditional sotests/dependencies/test_kvbm_imports.pyfinds the wheels.Net effect: runtime image drops from 108 → 104 fs layers, with upstream env inheritance preserved (no redeclaration of OPAL_PREFIX, CUDA_HOME, NVIDIA_REQUIRE_CUDA, ~37 other vars).
In-Dockerfile cleanup also included: collapsed transport stages (
workspace_files,dynamo_base_export) for one-COPY workspace + dynamo_base imports, consolidated metadata + setup RUNs, restored unconditional wheel COPY (fix for dev/local-dev wheelhouse regression), assertlibstdc++.so.6target exists before symlinking, factor LIBSTDCPP path into a variable.Test plan
docker run nvcr.io/nvidia/ai-dynamo/tensorrtllm-runtime:<tag>no longer errors with "no command specified" — drops into bash.test_distrolessstage no longer fails withmax depth exceeded.which sshdreturns/usr/sbin/sshd,ldconfig -p | grep librdmacmshows the lib,dpkg -s rdma-coreshows installed,/workspace/{tests,examples,deploy,lib,components/src/dynamo/{common,frontend,trtllm,mocker}}all present and owned bydynamo:0.Layer-count fix verification (new for §3)
docker inspect dynamo:<runtime-tag> --format '{{len .RootFS.Layers}}'returns ≤104 on amd64.--platform=linux/arm64).docker run --rm <runtime-tag> env | grep -E '^(OPAL_PREFIX|CUDA_HOME|NVIDIA_REQUIRE_CUDA|LD_LIBRARY_PATH)='returns upstream's values (inheritance via FROM works).run_trtllm_disagg.shwith Qwen3-0.6B):POST /v1/chat/completionsreturns 200; prefill→decode handoff fires with matching request_id.docker run --rm <runtime-tag> ls /workspaceshows only Dynamo content (no upstreamtensorrt_llm/leak from overlay shadowing).docker run --rm <runtime-tag> ls /opt/dynamo/wheelhouse/showskvbm,ai_dynamo_runtime,ai_dynamo,gpu_memory_servicewheels.--target=devand--target=local-devbuilds complete successfully (use the layeredruntimestage, not the rebased one — dev iteration speed must remain unaffected).--target=devarm64 build also succeeds (QEMU emulation OK for verification).tests/dependencies/test_kvbm_imports.py::test_kvbm_wheel_exists_trtllmpasses on the dev test image (wheelhouse populated).🤖 Generated with Claude Code