feat(dstack-util): warn when the NVLink fabric never comes up - #1167
Open
kvinwang wants to merge 1 commit into
Open
feat(dstack-util): warn when the NVLink fabric never comes up#1167kvinwang wants to merge 1 commit into
kvinwang wants to merge 1 commit into
Conversation
A multi-GPU CVM can boot with every surface signal green and no working
NVLink between its GPUs. Links are trained by ALI in hardware whatever the
fabric is doing, so `nvidia-smi nvlink -s` reports all of them Active either
way; Fabric Manager logs its success line; containers start and CUDA runs.
The only symptom is that collectives fall back to PCIe and the tenant pays
full price for a fraction of the bandwidth. Nothing errors.
Check it at the point that already gates GPUs. After attestation passes and
before the ready state is set, poll every GPU's fabric registration and, once
they are all registered, ask whether peer-to-peer actually works.
Three things are read, and the second is the reason this is worth doing:
state must reach NVML_GPU_FABRIC_STATE_COMPLETED
status must be NVML_SUCCESS -- Completed with a failed status is a real
observed condition, and NCCL logs state and never status, so it is
invisible in every NCCL log while P2P capability is gone
cliqueId must be the same for every GPU, or they registered into different
partitions and cannot reach each other however healthy each looks
Then nvmlDeviceGetP2PStatus for both directions of every pair, which is the
call behind NCCL's own "P2P is disabled between NVLINK connected GPUs"
warning and the one that catches a fabric that registered cleanly but still
cannot move traffic.
Advisory, not a gate. Every one of these failures is host-side -- a partition
that was never activated, a stale Fabric Manager, GPUs landing in different
cliques -- and none of it is something the guest can fix or should refuse to
boot over. The message says as much and points at `fmpm -l` and
fabricmanager.log rather than leaving someone to debug inside the guest.
Polled rather than sampled once: registration is asynchronous, so a single
read right after the driver initialises legitimately catches IN_PROGRESS, and
a check that fails intermittently is worse than no check. The elapsed time is
logged so the 30s bound can be tuned from deployments instead of guessed at
again.
Skipped entirely when the driver reports no fabric, which covers single-GPU
tenants and every host without NVSwitch, so nothing has to be configured for
the common case.
nvml-wrapper 0.12 binds neither entry point, so both go through
nvml-wrapper-sys using the handle and loaded library the safe wrapper already
owns. This is the first raw NVML call in the tree; the unsafe blocks carry
their justification.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/robustness issues in the new fabric polling path (early skip on partial NOT_SUPPORTED and silently ignored JoinError) that can hide failures or drop diagnostics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an advisory NVLink fabric health check to dstack-util’s GPU attestation/readying path to surface cases where GPUs appear healthy but NVLink P2P is effectively unavailable (causing NCCL to fall back to PCIe with no hard error).
Changes:
- Introduces a polling NVML-based fabric readiness check (fabric state/status + clique consistency + P2P status per GPU pair) and logs actionable warnings when the fabric never becomes usable.
- Hooks the advisory check into system setup after GPU attestation and before setting the GPU “ready” state (runs in
spawn_blockingdue to polling). - Adds
nvml-wrapper-sysas a workspace dependency to access raw NVML entry points not exposed bynvml-wrapper.
File summaries
| File | Description |
|---|---|
| dstack/dstack-util/src/system_setup.rs | Implements fabric + P2P checks via raw NVML calls and adds unit tests for the pure predicates. |
| dstack/dstack-util/Cargo.toml | Adds nvml-wrapper-sys dependency for the util crate. |
| dstack/Cargo.toml | Adds workspace dependency pin for nvml-wrapper-sys. |
| dstack/Cargo.lock | Locks the new dependency in the workspace. |
Review details
- Files reviewed: 3/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1699
to
+1710
| let mut infos = Vec::with_capacity(expected_devices as usize); | ||
| for index in 0..expected_devices { | ||
| match fabric_info(&nvml, index)? { | ||
| // The driver says this platform has no fabric at all, so | ||
| // there is nothing to wait for and nothing to warn about. | ||
| None => { | ||
| info!("no NVLink fabric on this platform; skipping fabric readiness check"); | ||
| return Ok(()); | ||
| } | ||
| Some(info) => infos.push(info), | ||
| } | ||
| } |
Comment on lines
+2431
to
+2433
| let fabric_devices = expected_devices; | ||
| let _ = tokio::task::spawn_blocking(move || gpu::warn_unless_fabric_ready(fabric_devices)) | ||
| .await; |
Comment on lines
+1639
to
+1647
| let mut blocked = Vec::new(); | ||
| for a in 0..devices { | ||
| for b in 0..devices { | ||
| if a == b { | ||
| continue; | ||
| } | ||
| let from = nvml.device_by_index(a)?; | ||
| let to = nvml.device_by_index(b)?; | ||
| for (name, index) in [ |
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.
The failure this reports
A multi-GPU CVM boots. Every surface signal is green:
nvidia-smilists all GPUsnvidia-smi nvlink -sshows all 18 links Active at 53.125 GB/s — because ALI trains links in hardware regardless of fabric stateSuccessfully configured all the available GPUs and NVSwitchesAnd NVLink P2P is dead. Collectives fall back to PCIe. The workload is 10–100× slower, nothing errors, and the tenant pays full price for a fraction of the bandwidth.
Not hypothetical:
State: CompletedandStatusmixingSuccessandUnknown Erroracross GPUs, undetected by health monitoringWhat it checks
At the point that already gates GPUs — after attestation passes, before the ready state is set.
stateNVML_GPU_FABRIC_STATE_COMPLETEDstatusNVML_SUCCESSCompleted+ failed status is a real observed condition, and NCCL logsstateand neverstatus, so it is invisible in every NCCL log while P2P capability is gonecliqueIdThen
nvmlDeviceGetP2PStatusfor both directions of every pair. That is the call behind NCCL's own "P2P is disabled between NVLINK connected GPUs" warning (src/graph/paths.cc), and it is what catches a fabric that registered cleanly but still cannot move traffic — the B300 case above.Advisory, not a gate
Every one of these failures is host-side: a partition that was never activated, a stale Fabric Manager, GPUs landing in different cliques. None of it is something the guest can fix or should refuse to boot over.
So it warns, and the message points where the problem actually is:
It runs after attestation so a fabric problem can never be mistaken for a security one, and on a blocking thread since it polls.
Polled, not sampled once
Registration is asynchronous — the GPU probes Fabric Manager over NVLink inband once the driver initialises it — so a single read right after boot legitimately catches
IN_PROGRESS. A check that fails intermittently is worse than no check.The elapsed time is logged, so the 30 s bound can be tuned from real deployments instead of guessed at a second time.
Zero configuration for the common case
Skipped entirely when the driver reports no fabric (
NVML_GPU_FABRIC_STATE_NOT_SUPPORTED), which covers single-GPU tenants and every host without NVSwitch. The driver decides, not a device table or a GPU count — same approach as the module-option generator in #1157.Implementation note
nvml-wrapper0.12 binds neithernvmlDeviceGetGpuFabricInfonornvmlDeviceGetP2PStatus, so both go throughnvml-wrapper-sysusing the handle and loaded library the safe wrapper already owns (Nvml::lib(),Device::handle()). This is the first raw NVML call in the tree; the unsafe blocks carry their justification.Testing
cargo test -p dstack-util— 109 passed, including two new tests over the pure predicates:registered()accepts onlyCompletedandSUCCESStogether, rejecting theCompleted+ failed-status pairing that NCCL cannot seeshared_clique()returnsNonewhen two successfully-registered GPUs report different cliquesclippy -D warningsandfmt --checkclean.Not verified on hardware. No GPU was available, so the NVML paths are unexercised. The one thing worth confirming before relying on this is that a guest actually sees fabric state when NVSwitches stay on the host (Blackwell MPT CC) — the mechanism says yes, since
nvmlDeviceGetGpuFabricInfois a per-GPU query and the shared-NVSwitch model requires guest P2P to work, but NVIDIA's integration guide never shows a guest-side example. One command in any multi-GPU CVM settles it:If that section is absent or reports
Not Supportedon a multi-GPU tenant, this check will simply skip and the design needs to move host-side.