Conversation
Signed-off-by: Dmitry Shmulevich <dshmulevich@nvidia.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #411 +/- ##
==========================================
+ Coverage 72.15% 73.36% +1.20%
==========================================
Files 89 94 +5
Lines 5689 6269 +580
==========================================
+ Hits 4105 4599 +494
- Misses 1382 1429 +47
- Partials 202 241 +39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pull-request-411.docs.buildwithfern.com/topograph |
dmitsh
force-pushed
the
ds-tiers
branch
2 times, most recently
from
July 19, 2026 12:34
9976bcd to
9464cd1
Compare
dmitsh
force-pushed
the
ds-tiers
branch
7 times, most recently
from
July 20, 2026 04:41
6ee51c0 to
7f0812e
Compare
dmitsh
force-pushed
the
ds-tiers
branch
3 times, most recently
from
July 20, 2026 12:26
33b2487 to
6f4ba01
Compare
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
Contributor
There was a problem hiding this comment.
Found 5 issues.
- isManagedLevelLabel only recognizes the configured keys and the new tier-N/domain-N prefixes, so the retired accelerator/leaf/spine/core labels are never removed from nodes labeled by a pre-upgrade Topograph -- the legacy-core case in kubernetes_test.go pins that behavior in. Since KAI Scheduler, NVSentinel, and Kueue consume those keys, frozen pre-upgrade values could silently mislead them; special-casing the four legacy defaults for removal (or documenting manual cleanup in the changelog) would close the gap. (pkg/engines/k8s/kubernetes.go:67)
- Hardcoding NewTopologyLabelKeys(nil, nil) drops the operator key customization the NFD engine honored via CurrentTopologyLabelKeys() until #402, and nfd Params has no fabricLabels/acceleratedLabels equivalent -- customized keys silently revert to defaults after upgrade. Plumbing the same params through the nfd engine (or calling the loss out in the changelog) would make the break explicit. (pkg/engines/nfd/engine.go:124)
- When a node carries both a provider-derived accelerated-domain-0 value and a differing nvidia.com/gpu.clique label, the first pass has already recorded the provider value in groupValues and this clique branch only overwrites elements -- producing a NodeFeatureGroup that no NodeFeature ever matches, which cleanupResource never removes because its name stays in objectNames. Resolving the clique override before recording groupValues (as the pre-refactor single pass did) avoids the dangling CR. (pkg/engines/nfd/objects.go:104)
- This reshapes the canonical Graph (Domains -> AcceleratedDomains) and retires the four fixed label keys, but no issue is linked from the PR; the contribution docs gate pkg/topology changes on prior issue discussion. Linking the discussion (or opening one) would satisfy that gate. (pkg/topology/topology.go:64)
- The label families this PR adds are tier-N and domain-N; there is no level-N label, so following this line to grep node labels matches nothing. Same for the accelerated level-0 phrasing here. (docs/providers/lambdai.md:183)
dmitsh
changed the title
dmitsh
force-pushed
the
ds-tiers
branch
2 times, most recently
from
July 21, 2026 05:43
e84c7ca to
1d01174
Compare
Contributor
Greptile SummaryThis PR replaces the fixed three-tier (leaf/spine/core) fabric topology model with a variable-depth, closest-first tier system (
Confidence Score: 5/5Safe to merge. All previously raised blocking issues have been addressed, and the new variable-tier logic is well-tested with mixed-depth and shared-root scenarios. Every issue flagged in prior review rounds is resolved: the empty-node-name guard is restored in both getTierLabels and getDomainLabels; the nil-graph panic in TopologyBlock translate is fixed and covered by new test cases; model.go only assigns Domains when non-empty; forest collision for mixed-depth paths is handled by the level-keyed nodes map plus mergeVertices. The new TopologyLabelKeys is per-engine-instance rather than a process-wide global, and is validated at startup. Test coverage was substantially extended for all changed paths. No files require special attention. The intentional non-removal of legacy leaf/spine/core Kubernetes labels during upgrades is confirmed by the test expectations and acknowledged as a documented breaking change. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Provider API] -->|FabricTiers slice| B[InstanceTopology]
B --> C[ClusterTopology.ToGraph]
C -->|level-keyed nodes map| D[forest map]
D -->|mergeVertices for shared roots| E[topology.Graph]
E --> F{Engine}
F --> G[K8s Engine]
F --> H[NFD Engine]
G --> G1[BuildNodeLabels with configured keys]
G1 --> G2[MergeNodeLabels - removeManagedTopologyLabels]
G2 --> G3[K8s Node Labels tier-0 ... tier-N]
H --> H1[BuildNodeLabels with default keys]
H1 --> H2[buildNFDObjects - topologyKind]
H2 --> H3[NFD NodeFeature and NodeFeatureGroup]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Provider API] -->|FabricTiers slice| B[InstanceTopology]
B --> C[ClusterTopology.ToGraph]
C -->|level-keyed nodes map| D[forest map]
D -->|mergeVertices for shared roots| E[topology.Graph]
E --> F{Engine}
F --> G[K8s Engine]
F --> H[NFD Engine]
G --> G1[BuildNodeLabels with configured keys]
G1 --> G2[MergeNodeLabels - removeManagedTopologyLabels]
G2 --> G3[K8s Node Labels tier-0 ... tier-N]
H --> H1[BuildNodeLabels with default keys]
H1 --> H2[buildNFDObjects - topologyKind]
H2 --> H3[NFD NodeFeature and NodeFeatureGroup]
Reviews (16): Last reviewed commit: "feat(topology): support variable network..." | Re-trigger Greptile |
Comment on lines
69
to
72
| case topology.TopologyBlock: | ||
| if graph == nil || graph.Domains == nil { | ||
| if graph.Domains == nil { | ||
| return fmt.Errorf("missing block topology for topology %q", topo) | ||
| } |
Contributor
There was a problem hiding this comment.
TopologyBlock
The graph == nil guard was removed from the TopologyBlock branch of Validate (both the per-partition path at ~line 69 and the cluster-wide path at ~line 88), but it was kept for TopologyTree. If a caller passes graph = nil with a block-topology config, graph.Domains dereferences a nil pointer and panics at runtime. The old code returned a clean "missing block topology" error in this case.
The guard needs to be restored to match the TopologyTree treatment: if graph == nil || graph.Domains == nil.
dmitsh
force-pushed
the
ds-tiers
branch
2 times, most recently
from
July 21, 2026 08:36
9878c1b to
ce6885a
Compare
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
Contributor
There was a problem hiding this comment.
Found 4 issues.
- When every member of an accelerator domain carries gpu.clique, the provider value recorded into groupValues in the first pass is never removed, so a NodeFeatureGroup gets emitted that no NodeFeature publishes and cleanup keeps it forever. Mirroring the k8s engine's skip-when-clique guard would close it - same point as my comment on the previous revision. (pkg/engines/nfd/objects.go:110)
- Legacy leaf/spine/core keys aren't recognized here, so upgraded nodes keep them frozen alongside the new tier-N labels - consumers still keyed on the old names read stale topology, and Case 4 in the test pins that behavior. Configurability covers new deployments, not values already on nodes; either strip the three well-known legacy keys during relabel or add a manual-cleanup step to the CHANGELOG migration note. (pkg/engines/k8s/kubernetes.go:67)
- Swapping this case away from acceleratorLabel == nvidia.com/gpu.clique removes the only coverage of the clique-preservation branches; either guard could now be deleted without a failing test, and the config is operator-reachable. Worth keeping the custom-label case and re-adding a clique case alongside. (pkg/engines/k8s/kubernetes_test.go:134)
- Still hardcoded to the defaults, so the new fabricLabels/acceleratorLabel customization stops at the k8s engine - NFD operators can't use it and the limitation isn't documented. Plumbing the params through or a line in the CHANGELOG would both settle it; re-raising from the previous revision. (pkg/engines/nfd/engine.go:124)
Signed-off-by: Dmitry Shmulevich <dshmulevich@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
2 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.
Replace the fixed leaf, spine, and core topology model with variable-depth, closest-first fabric tiers.
This change:
Replaces ToThreeTierGraph with arbitrary-depth ToGraph.
Represents per-instance fabric topology using:
Uses tier 0 for the switch closest to the compute node, increasing outward.
Publishes Kubernetes topology labels as:
Updates the Kubernetes and NFD engines to emit any number of discovered fabric tiers.
Retains the single AcceleratorID and Graph.Domains representation for Slurm block-topology compatibility.
Configures label names through Kubernetes engine parameters:
Allows users to reproduce the previous accelerator, leaf, spine, and core label set through custom label configuration.
Prevents mixed-depth paths sharing an outer switch ID from overwriting one another.
Updates providers, simulation models, documentation, Helm configuration, tests, and release notes.
This is a breaking change for consumers using the previous fixed leaf, spine, and core Kubernetes label keys or the process-wide Helm/CLI label overrides.
Addresses #412
Fixes #408