fix(vllm): accept /run/docker.sock as a default socket for disk-space check - #6864
Closed
yanyunl1991 wants to merge 1 commit into
Closed
fix(vllm): accept /run/docker.sock as a default socket for disk-space check#6864yanyunl1991 wants to merge 1 commit into
yanyunl1991 wants to merge 1 commit into
Conversation
… check The managed-vLLM disk-space guard read `DOCKER_HOST=unix:///run/docker.sock` as a non-default socket and, in non-interactive express mode, aborted onboard at step [3/8] before verifying free space — even with ample disk. On modern systemd Linux `/var/run` is a symlink to `/run`, so `/run/docker.sock` and `/var/run/docker.sock` address the same daemon socket; the check only accepted the `/var/run` spelling. Accept both `/run/docker.sock` and `/var/run/docker.sock` (and their `unix://` forms) as the default local socket via a new `isDefaultDockerSocket` helper, mirroring the socket candidates already probed in platform.ts. A genuinely non-default or remote socket is still rejected. Regression introduced in v0.0.82 when this guard was added; v0.0.81 had no such check. Fixes #6858 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
Contributor
📝 WalkthroughWalkthroughDocker locality validation now recognizes ChangesDocker socket locality
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
![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
🤖 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 `@src/lib/inference/vllm-storage.test.ts`:
- Around line 301-317: Add a rejection regression test alongside the existing
probeDockerHostLocality coverage to verify /tmp/forwarded-remote.sock remains
rejected. Prefer converting the supported-socket test into a table-driven case
covering /run/docker.sock and /var/run/docker.sock, each with and without the
unix:// prefix, while preserving the existing native Docker setup and expected {
ok: true } result for supported forms.
🪄 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: 4eb1d781-d475-46e3-8251-5bc28f7d4db1
📒 Files selected for processing (2)
src/lib/inference/vllm-storage.test.tssrc/lib/inference/vllm-storage.ts
Comment on lines
+301
to
+317
| it("accepts /run/docker.sock as a default socket (systemd /var/run -> /run symlink) (#6858)", () => { | ||
| // Regression: DOCKER_HOST=unix:///run/docker.sock is the same daemon socket | ||
| // as /var/run/docker.sock on systemd Linux, but was read as "non-default" and | ||
| // aborted express managed-vLLM disk-space verification. | ||
| vi.stubEnv("DOCKER_HOST", "unix:///run/docker.sock"); | ||
| vi.stubEnv("DOCKER_CONTEXT", "default"); | ||
|
|
||
| expect( | ||
| probeDockerHostLocality({ | ||
| clientContainerized: false, | ||
| dockerInfo: () => nativeDockerInfo(), | ||
| osRelease: nativeHost.osRelease, | ||
| platform: nativeHost.platform, | ||
| dockerSocketPeerSharesMountNamespace: () => true, | ||
| }), | ||
| ).toEqual({ ok: true }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the required rejection regression case.
This only proves unix:///run/docker.sock is accepted. Add coverage for /tmp/forwarded-remote.sock remaining rejected, and preferably table-test the supported /run and /var/run forms with and without unix://.
🤖 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 `@src/lib/inference/vllm-storage.test.ts` around lines 301 - 317, Add a
rejection regression test alongside the existing probeDockerHostLocality
coverage to verify /tmp/forwarded-remote.sock remains rejected. Prefer
converting the supported-socket test into a table-driven case covering
/run/docker.sock and /var/run/docker.sock, each with and without the unix://
prefix, while preserving the existing native Docker setup and expected { ok:
true } result for supported forms.
Contributor
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Collaborator
|
Currently I am testing to make sure that I can repro the issue first. Will hang the PR a little bit |
wscurran
added
area: install
21 tasks
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
On v0.0.82 the managed-vLLM disk-space guard reads
DOCKER_HOST=unix:///run/docker.sockas a "non-default socket" and, in non-interactive express mode, aborts onboarding at step[3/8]before verifying free space — even with ~1.5 TB free./run/docker.sockand/var/run/docker.sockare the same daemon socket on systemd Linux (/var/run→/run). This PR accepts both spellings.Closes #6858.
Reproduction
Real compiled guard exercised on our DGX Spark aarch64 test host (GB10 GPU) against the real Docker daemon, with the reporter's
DOCKER_HOST.Environment
main(v0.0.82)Observed on
main(before fix)Observed on
fix/...(after fix)A genuinely non-default socket (e.g.
unix:///tmp/forwarded-remote.sock) still fails closed.Analysis
nativeDockerHostProbleminsrc/lib/inference/vllm-storage.tscompared theDOCKER_HOSTendpoint only against/var/run/docker.sock(and itsunix://form). On modern systemd Linux/var/runis a symlink to/run, so aDOCKER_HOSTofunix:///run/docker.sockaddresses the identical socket but failed the equality check, returning "non-default socket". Both the image check (probeDockerStorage→resolveDockerStorageLocations) and the model-cache check (probeDockerHostLocality) route throughnativeDockerHostProblem, so the failure blocks the whole disk-space verification; in non-interactive express modestorageWarningAcceptedthen returns false and onboarding exits before the pull.src/lib/platform.tsalready treats both socket paths as valid daemon-socket candidates, so this check was the inconsistent outlier. Regression: this guard was added in v0.0.82 (v0.0.81 had no such check).Fix
Add an
isDefaultDockerSockethelper that normalizes the optionalunix://prefix and accepts both/var/run/docker.sockand/run/docker.sock, and use it innativeDockerHostProblem. A remote (ssh://…) or genuinely non-default socket is still rejected. A regression test locks in that/run/docker.sockis accepted while/tmp/forwarded-remote.sockstill fails closed.Changes
src/lib/inference/vllm-storage.ts: accept/run/docker.sockas a default socket viaisDefaultDockerSocket.src/lib/inference/vllm-storage.test.ts: regression test for/run/docker.sock.Type of Change
Verification
npm testpasses (touched files)AI Disclosure
Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Summary by CodeRabbit
/var/run/docker.sockand/run/docker.sock, includingunix://formats, as valid default local endpoints.