feat: Dual-level accelerated domain support - Design and Prototype - #416
feat: Dual-level accelerated domain support - Design and Prototype#416ravisoundar wants to merge 10 commits into
Conversation
|
/ok-to-test 24e80de |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #416 +/- ##
==========================================
+ Coverage 72.15% 75.93% +3.77%
==========================================
Files 89 95 +6
Lines 5689 6739 +1050
==========================================
+ Hits 4105 5117 +1012
+ Misses 1382 1355 -27
- Partials 202 267 +65 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR implements dual-level accelerated domain support ("XCLR") in topograph. It renames the single
Confidence Score: 5/5Safe to merge; all providers, engines, and the block translate layer are updated consistently and the label migration path for existing nodes is covered. The rename from AcceleratorID to XclrDomainID/XclrSubDomainID is applied uniformly across all six providers and both engines. The legacy label constant is added to the managed set so old nodes are cleaned up on the next reconcile. The new three-strategy block packer is backed by a comprehensive suite of unit and integration tests including the dual-level simulation model. The only finding is a cosmetic string-description mismatch in Validate(). Files Needing Attention: pkg/engines/k8s/labeler.go — minor description-string issue in Validate(); no functional impact. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Provider: InstanceTopology\nXclrDomainID / XclrSubDomainID] --> B[ClusterTopology.ToGraph]
B --> C[DomainMap\nmap domain → hostName → HostInfo\nHostInfo.SubDomain set when XclrSubDomainID present]
C --> D[GetDomainTree]
D --> E{Hosts have\nSubDomain?}
E -- No --> F[One-level BlockVertex\nLeaf: domain → Hosts]
E -- Yes --> G[Two-level BlockVertex\nDomain → SubDomain → Hosts]
F --> H[toDomainAggregate]
G --> H
H --> I{Strategy\nselection}
I -- Hosts != nil Leaf --> J[Strategy 1:\npackHostsIntoAggregate]
I -- MaxChildNodeCount\n≤ baseBlockSize/2 --> K[Strategy 2:\ncombineChildHostsIntoAggregate]
I -- MaxChildNodeCount\n> baseBlockSize/2 --> L[Strategy 3:\nrecurseChildrenIntoAggregate]
J --> M[toRootAggregate\nGCD-based padding to blockSizes-last]
K --> M
L --> M
M --> N[complementBlocks → flat blockInfo list]
C --> O[getDomainLabels]
O --> P[K8s node labels:\nxclr.topology.nvidia.com/domain\nxclr.topology.nvidia.com/sub-domain]
Reviews (35): Last reviewed commit: "Fix - root level padding when the domain..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
24e80de to
507afeb
Compare
|
/ok-to-test 507afeb |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-416.docs.buildwithfern.com/topograph |
507afeb to
664c001
Compare
dmitsh
changed the title
664c001 to
1411514
Compare
|
/ok-to-test 1411514 |
1411514 to
efa3725
Compare
|
/ok-to-test efa3725 |
efa3725 to
fae6059
Compare
|
/ok-to-test fae6059 |
fae6059 to
bdd65e8
Compare
|
/ok-to-test bdd65e8 |
bdd65e8 to
84b8b2c
Compare
|
/ok-to-test 84b8b2c |
dmitsh
left a comment
dmitsh
left a comment
There was a problem hiding this comment.
Correctness Issues
1. SDD documents the wrong label name
docs/design/dual-level-topology-sdd.md (Data Model / Simulation model YAML section) states:
network.topology.nvidia.com/group → HostInfo.SubDomain (parent domain)
But pkg/models/model.go defines:
LabelTopologySubDomain = "network.topology.nvidia.com/sub-domain"The SDD name (/group) and the implementation name (/sub-domain) don't match. Providers reading the SDD to opt in will use the wrong label.
2. convert: nil return value not guarded (latent panic)
In the interior-node path of convert (pkg/translate/block_tree.go):
for _, name := range slices.Sorted(maps.Keys(src.Children)) {
converted := convert(src.Children[name], baseBlockSize)
target.children = append(target.children, converted)
target.nodeCount += converted.nodeCount // panics if converted == nilconvert returns nil when src == nil. src.Children[name] can't be nil today (all values are set by GetDomainTree), but this is a latent panic for any future caller. A guard if converted == nil { continue } eliminates the footgun at zero cost.
3. childCapacity can stay zero, silently suppressing padding
childCapacity := 0
for _, name := range slices.Sorted(maps.Keys(src.Children)) {
converted := convert(src.Children[name], baseBlockSize)
...
if childCapacity == 0 {
childCapacity = converted.nodeCount
}
}
for target.nodeCount < src.DesiredNodeCount && childCapacity > 0 {
...
}childCapacity is sampled only from the first child. If that child has nodeCount == 0 (e.g. its DesiredNodeCount was never set because blockSizes was empty, or it's a leaf domain with zero hosts), childCapacity stays 0 and the padding loop never fires. Absent base domains are silently omitted from the output rather than getting placeholder slots. The fix is to sample childCapacity from the first child with nodeCount > 0, not just the first child unconditionally.
4. domainsForBlocks interaction with SubDomain (pre-existing, but worth noting)
complementBlocks calls domainsForBlocks(nt.domains, blocks) which filters nt.domains by matching b.name (block name) against domain map keys. In the single-level case b.name is the accelerator domain name, so the lookup works. In the dual-level case b.name will be the parent domain name (domain-01, etc.) which IS the key in DomainMap, so it still works — but only because the test fixture uses the accelerator domain as the outer key.
If a future provider populates the block name as the sub-domain (base rack name) instead of the parent domain, all[b.name] will return nil and hosts will be silently dropped. The pairing between blockInfo.name and DomainMap keys is an undocumented contract worth making explicit (or enforcing in tests).
5. Placeholder positions diverge from alphabetical slot positions
rack-1-03 and rack-1-13 are alphabetically positions 3 and 13 within domain-01, but their placeholder blocks appear as block015 and block016 (the last two slots). The SDD documents this as a known limitation, but it means Slurm's position-based aggregate inference will be wrong for those specific slots — a real operational impact. The limitation callout in the SDD should be elevated to the PR description so reviewers and operators are not surprised.
1b32609 to
785bc8d
Compare
|
/ok-to-test 785bc8d |
e8209f9 to
cf7f184
Compare
22de1b7 to
a778d23
Compare
|
/ok-to-test a778d23 |
0941487 to
902bbd6
Compare
|
/ok-to-test 0fbdffe |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Found 6 issues. The first two are worth settling before merge; the rest are docs and API-surface fixes.
- The padding loop steps by
childCapacityand stops at>= targetCount, so it lands on a multiple ofdesiredNodeCountonly when every child has the same capacity. Dual-level domains break that: withblockSizes=[18,144,1152], domainaaa(72 hosts, 8 sub-domains) converts to nodeCount 576 whilebbb(144 hosts, 2 sub-domains) converts to 144, so the root goes 720 -> 1296 and stops, emitting 72 base blocks where 1296 % 1152 = 144. A sweep of 784 two-domain shapes over that list hits it in 22, and the output then declaresBlockSizes=18,144,1152over a block list that does not tile into 1152. It needs unequal sub-domain counts per domain, sotests/models/dual-level.yamldoes not catch it on any block-size list I tried - but that is the shape a real OCI cluster with uneven racks per fabric domain produces. (pkg/translate/block_tree.go:299) buildNFDObjectsskips bothxclr-domainandxclr-sub-domainwhennvidia.com/gpu.cliqueis present, so a provider-supplied sub-domain attribute is not published. This PR's ownTestBuildNFDObjectsSuppressesSubDomainWhenGPUCliqueExistsasserts that suppression, anddocs/reference/node-labels.mddescribes it correctly, so this line is the outlier. (docs/engines/nfd.md:152)- This location string is the Go field name, while the other two are operator-facing config keys (
fabricLabels[N]andacceleratorLabel). PointingacceleratorLabelat the sub-domain key yieldsconfigured for both acceleratorLabel and XclrSubDomain, which names a parameter operators cannot set. (pkg/engines/k8s/labeler.go:61) - The comment says each depth receives the smallest
blockSize >= max actualNodeCount, butgetDesiredCountByLevelcallspow2GroupCapacity, which returns the smallest2^n x base. WithblockSizes=[18,144]and a depth max of 20 that is 36, not 144, so the documented rule and the code disagree on a value that drives block padding. The same claim is repeated onsetDesiredCountByLevel. (pkg/topology/domain.go:140) - This entry records what simulation models moved away from, and that key was
network.topology.nvidia.com/accelerator-xclr.topology.nvidia.com/domaindid not exist when that change landed. The rename swept up a historical reference, so the entry now describes models leaving a key they never used. (CHANGELOG.md:32) asBlockVertexis sound only while every*VertexinVerticesis&child.Vertex, butVerticesis an exported field on an exported type, so nothing outside this file enforces that. Achildren map[string]*BlockVertexfield, or an unexported child map withChildAtas the only reader, would let the compiler hold the invariant instead of a doc comment. (pkg/topology/domain.go:60)
adfa820 to
ec96faf
Compare
|
/ok-to-test acb00d4 |
acb00d4 to
8742ee0
Compare
|
/ok-to-test 8742ee0 |
8742ee0 to
76d262d
Compare
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
…6 domain Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Signed-off-by: Ravi Shankar <ravish@nvidia.com> Signed-off-by: Dmitry Shmulevich <dshmulevich@nvidia.com>
…rID is present Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Signed-off-by: Dmitry Shmulevich <dshmulevich@nvidia.com>
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
…nodes Signed-off-by: Ravi Shankar <ravish@nvidia.com> Signed-off-by: Dmitry Shmulevich <dshmulevich@nvidia.com>
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
76d262d to
8c36950
Compare
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
7169038 to
9c7ec47
Compare
|
/ok-to-test 9c7ec47 |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Went through the graph conversion, the labeler upgrade path, and the block packing. The dual-level model itself holds up — I verified the three ToGraph cases and confirmed no host is dropped or duplicated across even, uneven, tiny-sub-domain, and one-level layouts. One blocker: the new sub-domain combining and the new blockName.format feature are mutually incompatible.
- [must-fix] Strategy 2 packs hosts from several sub-domains into one base block and names it with a
+-joined compound, butformatBlockNameerrors when nodes in one block derive different names. Both features are new in this release, so a dual-level cluster that setsblockName.nodeNameRegexp/formatfails topology generation outright. Reproduced with one domain, four racks of two hosts,blockSizes=[8,16], and format$rack: block001 came out asr1+r2+r3andformatBlockNamesreturnednodes in block "block001" (domain "r1+r2+r3") produce different block names "r1" and "r2". Either skip the combining when a block-name formatter is configured, or make the formatter tolerate a combined block. (pkg/translate/block_tree.go:411) - [should-fix] The flush fires once the already-accumulated count passes
baseBlockSize/2rather than looking ahead at the next child, so a block closes early. The docstring above says it flushes "whenever the running host count reaches baseBlockSize", and the SDD says "No host slots are wasted" — neither matches. Concretely, four sub-domains of two hosts withbaseBlockSize=8produce two blocks (6 hosts, then 2) where one full block would do. Worth either fixing the threshold or correcting both descriptions. (pkg/translate/block_tree.go:409) - [should-fix] This entry announces
kubeQPSandkubeBurstparameters on the DRA provider and Slinky engine, but no such parameters exist — the only matches in the tree are theKUBE_QPS/KUBE_BURSTenv vars ininternal/k8s/client.goand the HelmkubeClient.qps/kubeClient.burstvalues. The entry five lines above already describes the real mechanism correctly, so this one looks like it should just be dropped. (CHANGELOG.md:16) - [should-fix] This warning sits inside the per-host loop, so a partially-racked domain logs one line per node on every regeneration — and the node observer retriggers generation on node and pod changes. The SDD names partially-racked OCI domains as an expected condition, so this is normal operation rather than an anomaly. Consider aggregating to one line per domain with a count. (pkg/topology/domain.go:140)
- [should-fix] Both cases here carry a rack (
rack01,rack12), so the rackless branch ofconvertComputeHostis never exercised. Deleting theif rack != ""guard ininstance_topology.goleavesgo test ./pkg/providers/oci/green — I checked. That guard implements documented behavior:docs/providers/oci.mdsays hosts without rack metadata keep single-level topology, and without it every rackless host gets the sub-domain<GpuMemoryFabricId>.with a trailing dot. A third case with an empty rack assertingXclrSubDomainIDstays empty would cover it. (pkg/providers/oci/provider_sim_test.go:258) - [consider] This BREAKING entry is a verbatim duplicate of the one five lines above, so the simulation-model annotation change is listed twice in the same Changed section. (CHANGELOG.md:35)
- [consider] A few comments still describe an earlier block-tree design. This one says buildBlockTree "assigns DesiredNodeCount to every node via a BFS pass" — there is no
DesiredNodeCountfield anywhere andGetDomainTreecomputesActualNodeCount/MaxChildNodeCountinline, which its own docstring says avoids a separate BFS pass. Same category: the strategy-3 docstring says the recursion passeschild.MaxChildNodeCount()whenrecurseChildrenIntoAggregatepasses the parent's value, and theplacedcounter is described as excluding unplaced hosts when the fallback branch places every host, so it always equalslen(hosts). (pkg/translate/block_complement.go:22; also pkg/translate/block_tree.go:272, pkg/topology/domain.go:152) - [consider] The first argument to
validateis a human-readable config location — the other call sites passfabricLabels[0]andacceleratorLabel— but this one passes the label key itself. SettingfabricLabels: ["xclr.topology.nvidia.com/sub-domain"]yields:topology label key "xclr.topology.nvidia.com/sub-domain" is configured for both fabricLabels[0] and xclr.topology.nvidia.com/sub-domain. The collision is still caught; only the message is confusing. (pkg/engines/k8s/labeler.go:61)
| // Flush pending hosts into a base block once their count exceeds half the base block size. | ||
| if len(pendingHosts) > baseBlockSize/2 { | ||
| blockName := strings.Join(pendingNames, "+") | ||
| bb := newBaseBlock(blockName, pendingHosts, baseBlockSize) |
There was a problem hiding this comment.
Strategy 2 packs hosts from several sub-domains into one base block and names it with a +-joined compound, but formatBlockName errors when nodes in one block derive different names. Both features are new in this release, so a dual-level cluster that sets blockName.nodeNameRegexp/format fails topology generation outright. Reproduced with one domain, four racks of two hosts, blockSizes=[8,16], and format $rack: block001 came out as r1+r2+r3 and formatBlockNames returned nodes in block "block001" (domain "r1+r2+r3") produce different block names "r1" and "r2". Either skip the combining when a block-name formatter is configured, or make the formatter tolerate a combined block.
| sortHostsByName(childHosts) | ||
|
|
||
| // Flush pending hosts into a base block once their count exceeds half the base block size. | ||
| if len(pendingHosts) > baseBlockSize/2 { |
There was a problem hiding this comment.
The flush fires once the already-accumulated count passes baseBlockSize/2 rather than looking ahead at the next child, so a block closes early. The docstring above says it flushes "whenever the running host count reaches baseBlockSize", and the SDD says "No host slots are wasted" — neither matches. Concretely, four sub-domains of two hosts with baseBlockSize=8 produce two blocks (6 hosts, then 2) where one full block would do. Worth either fixing the threshold or correcting both descriptions.
| - The NFD engine now publishes separate `xclr-domain` and `xclr-sub-domain` attributes and groups. | ||
| - The graph engine now includes `xclr.topology.nvidia.com/sub-domain` in instance labels when supplied alongside an XCLR domain. | ||
| - Exported Go constant `topology.KeyTopologyXclrSubDomain` for the `xclr.topology.nvidia.com/sub-domain` label key. | ||
| - DRA provider and Slinky engine `kubeQPS` and `kubeBurst` parameters for tuning their independent Kubernetes client rate limits on large clusters. |
There was a problem hiding this comment.
This entry announces kubeQPS and kubeBurst parameters on the DRA provider and Slinky engine, but no such parameters exist — the only matches in the tree are the KUBE_QPS/KUBE_BURST env vars in internal/k8s/client.go and the Helm kubeClient.qps/kubeClient.burst values. The entry five lines above already describes the real mechanism correctly, so this one looks like it should just be dropped.
| // info). Place it in a fallback sub-domain vertex keyed by the | ||
| // accelerator domain name so the host is always emitted rather than | ||
| // silently dropped. | ||
| klog.Warningf("domain %q: host %q has no SubDomain; placing in fallback sub-domain %q", domain, host.HostName, domain) |
There was a problem hiding this comment.
This warning sits inside the per-host loop, so a partially-racked domain logs one line per node on every regeneration — and the node observer retriggers generation on node and pod changes. The SDD names partially-racked OCI domains as an expected condition, so this is normal operation rather than an anomaly. Consider aggregating to one line per domain with a count.
| require.Equal(t, tc.parentDomain, instanceTopology.ParentAcceleratorID) | ||
| require.Equal(t, tc.parentDomain+"."+tc.rack, instanceTopology.AcceleratorID) | ||
| require.Equal(t, tc.domain, instanceTopology.XclrDomainID) | ||
| require.Equal(t, tc.domain+"."+tc.rack, instanceTopology.XclrSubDomainID) |
There was a problem hiding this comment.
Both cases here carry a rack (rack01, rack12), so the rackless branch of convertComputeHost is never exercised. Deleting the if rack != "" guard in instance_topology.go leaves go test ./pkg/providers/oci/ green — I checked. That guard implements documented behavior: docs/providers/oci.md says hosts without rack metadata keep single-level topology, and without it every rackless host gets the sub-domain <GpuMemoryFabricId>. with a trailing dot. A third case with an empty rack asserting XclrSubDomainID stays empty would cover it.
3 participants
Description
Design and Prototype for the dual level accelerated domain support.
Addresses #415
Checklist
git commit -s).