Skip to content

TL/CUDA: fix NVLS on fabric cluster UUID#1308

Merged
janjust merged 2 commits into
openucx:masterfrom
ikryukov:fix/nvls-cluster-uuid-topo-check
Jul 2, 2026
Merged

TL/CUDA: fix NVLS on fabric cluster UUID#1308
janjust merged 2 commits into
openucx:masterfrom
ikryukov:fix/nvls-cluster-uuid-topo-check

Conversation

@ikryukov

@ikryukov ikryukov commented May 4, 2026

Copy link
Copy Markdown
Collaborator

What

Fix silent multinode NVLS crash on standalone DGX nodes.

Why

NVML cliqueId/partitionId are unique only within a fabric cluster, so unrelated DGX nodes can collide on these values, falsely pass ucc_topo_is_single_nvlink_domain, and crash later in cuMulticastAddDevice.

How

Read clusterUuid (globally unique) from nvmlGpuFabricInfo_v2_t into ucc_gpu_info_t.fabric_cluster_uuid and require it to be non-all-zero and matching across ranks.

@ikryukov ikryukov self-assigned this May 4, 2026
@ikryukov ikryukov requested review from Sergei-Lebedev, janjust and nsarka and removed request for Sergei-Lebedev May 4, 2026 12:49
@ikryukov

ikryukov commented May 4, 2026

Copy link
Copy Markdown
Collaborator Author

/build

@greptile-apps

greptile-apps Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a silent crash when running multinode NVLS on standalone DGX nodes by reading the globally-unique clusterUuid from nvmlGpuFabricInfoV_t/nvmlGpuFabricInfo_t and requiring it to be non-zero and matching across all ranks before enabling NVLS — preventing false positives caused by cliqueId/partitionId collision across unrelated fabric clusters.

  • ucc_proc_info.h: adds fabric_cluster_uuid[16] to ucc_gpu_info_t and an ucc_gpu_fabric_cluster_uuid_is_valid() helper that returns true for any non-all-zero UUID.
  • ucc_sysinfo_cuda.c: zero-initialises the new field, uses UCC_STATIC_ASSERT to confirm sizeof(fabric_info.clusterUuid) == 16 at compile time for both NVML API paths, then copies the UUID on a completed fabric state.
  • ucc_topo.c: inserts UUID validity and equality checks ahead of the existing clique_id/partition_id comparisons in ucc_topo_is_single_nvlink_domain.

Confidence Score: 5/5

Safe to merge — the change is a targeted guard that only makes ucc_topo_is_single_nvlink_domain more conservative; it cannot cause a false negative on a properly-connected fabric cluster.

The logic is well-layered: the new globally-unique UUID check is added before the existing within-cluster checks, zero-init ensures a deterministic sentinel for nodes that never query fabric info, and the compile-time assertion catches any future NVML header ABI drift on both the versioned and non-versioned API paths. Both nvmlGpuFabricInfo_t and nvmlGpuFabricInfoV_t include clusterUuid, so the else branch compiles cleanly. No correctness issues were found.

No files require special attention.

Important Files Changed

Filename Overview
src/utils/ucc_proc_info.h Adds UCC_GPU_FABRIC_CLUSTER_UUID_LEN, fabric_cluster_uuid[16] field to ucc_gpu_info_t, and ucc_gpu_fabric_cluster_uuid_is_valid() inline helper; also fixes the stray space before ucc_nic_info.
src/components/topo/cuda/ucc_sysinfo_cuda.c Zero-initialises fabric_cluster_uuid, adds a compile-time size assertion against NVML's clusterUuid, and copies it on a successful fabric-state query; extends the debug log to print the UUID in standard UUID format.
src/components/topo/ucc_topo.c Adds cluster-UUID validity and cross-rank equality checks before the existing clique_id/partition_id comparisons, correctly blocking NVLS activation when UUIDs are all-zero or mismatched.
src/components/topo/ucc_topo.h Doc-comment update for ucc_topo_is_single_nvlink_domain to reflect the new cluster-UUID requirement alongside the existing clique_id/partition_id checks.

Reviews (3): Last reviewed commit: "TL/CUDA: apply review fixes for NVLS UUI..." | Re-trigger Greptile

Comment thread src/utils/ucc_proc_info.h Outdated
@dpressle

Copy link
Copy Markdown
Collaborator

👀 Investigating Lint (codestyle)

@dpressle

Copy link
Copy Markdown
Collaborator

🤖 CI Triage AgentLint (codestyle) · commit a71bf3cd

The logs are clear and the root cause is immediately evident. No further investigation is needed.


Summary: Codestyle/lint check failed because one commit in PR #1308 has a non-conforming title: "Apply suggestions from code review"

Root cause: The codestyle CI job enforces a strict commit title convention for all commits in the PR range (remotes/origin/master..HEAD). Every commit title must begin with one of the approved component prefixes (e.g. TL/CUDA:, CORE:, TEST:, etc.) followed by a space and a word character. The commit at [REDACTED:Hex High Entropy String] has the title "Apply suggestions from code review" — a GitHub-generated title used when accepting review suggestions — which does not match the required pattern and therefore fails the check_title() regex check:

Wrong header
Bad commit title: 'Apply suggestions from code review'

The other commit in the PR (TL/CUDA: fix NVLS on fabric cluster UUID) passes just fine.

Implicated commit: [REDACTED:Hex High Entropy String] — "Apply suggestions from code review" (author visible in PR #1308)

File: .github/workflows/codestyle.yamlcheck_title() function (the policy that is being enforced, not the code under review)

Suggested fix: Amend (or interactively rebase) the offending commit to give it a conforming title. Since the substantive change is a CUDA NVLS topology fix, an appropriate title would be something like:

TL/CUDA: apply review suggestions for NVLS cluster UUID topo check

Then force-push the branch:

git rebase -i HEAD~2   # mark the 'Apply suggestions...' commit as 'reword'
# Change the title to e.g. "TL/CUDA: apply review suggestions for NVLS cluster UUID topo check"
git push --force-with-lease origin fix/nvls-cluster-uuid-topo-check

The new title must start with one of the allowed prefixes (CODESTYLE|REVIEW|CORE|UTIL|TEST|API|DOCS|TOOLS|BUILD|MC|EC|SCHEDULE|TOPO|CI|CL/|TL/|MC/|EC/|UCP|SHM|NCCL|SHARP|BASIC|HIER|DOCA_UROM|CUDA|CPU|EE|RCCL|ROCM|SELF|MLX5) followed by : and at least one word character.

Related: PR #1308 (fix/nvls-cluster-uuid-topo-check)

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 9d226cbb-2995-4964-ac43-4c74efa42bf5 in the triage console for the audit trail.

ikryukov and others added 2 commits July 1, 2026 13:48
NVML cliqueId/partitionId are unique only within a fabric cluster, so
unrelated standalone DGX nodes can report the same cliqueId/partitionId
with different fabrics and silently pass ucc_topo_is_single_nvlink_domain.
Multinode NVLS init then crashes late at cuMulticastAddDevice and wedges
team creation.

Read clusterUuid (nvmlGpuFabricInfo_v2_t, the globally-unique fabric
identifier) into ucc_gpu_info_t.fabric_cluster_uuid and require it to be
non-all-zero and identical across ranks before approving a single NVLink
domain. Verified on gaia DGX H100 where NVML reports all-zero clusterUuid
with state=COMPLETED: the topo guard now correctly disables multinode
NVLS instead of failing late.

Signed-off-by: Ilya Kryukov <ikryukov@nvidia.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@ikryukov ikryukov force-pushed the fix/nvls-cluster-uuid-topo-check branch from a71bf3c to 25f6351 Compare July 1, 2026 13:50
@ikryukov

ikryukov commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

/build

@janjust janjust enabled auto-merge (squash) July 2, 2026 13:24
@janjust janjust merged commit 1f09e66 into openucx:master Jul 2, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants