feat: Dual-level accelerated domain support - Design and Prototype

[ravisoundar]

Description

Design and Prototype for the dual level accelerated domain support.
Addresses #415

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.
• All commits are signed off per DCO (git commit -s).

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[ravisoundar]

/ok-to-test 24e80de

[codecov]

Codecov Report

❌ Patch coverage is 88.93617% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.51%. Comparing base (8468abe) to head (a778d23).
⚠️ Report is 37 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/translate/block_tree.go	72.88%	10 Missing and 6 partials ⚠️
pkg/topology/domain.go	95.91%	2 Missing and 2 partials ⚠️
pkg/providers/oci/provider_imds.go	0.00%	3 Missing ⚠️
pkg/topology/graph.go	88.23%	1 Missing and 1 partial ⚠️
pkg/engines/k8s/labeler.go	95.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #416      +/-   ##
==========================================
+ Coverage   72.15%   74.51%   +2.35%     
==========================================
  Files          89       94       +5     
  Lines        5689     6536     +847     
==========================================
+ Hits         4105     4870     +765     
- Misses       1382     1421      +39     
- Partials      202      245      +43     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[greptile-apps]

Greptile Summary

This PR introduces dual-level accelerated domain support, allowing a cluster topology to express a two-tier accelerator hierarchy (domain → sub-domain, e.g., NVL domain → rack) alongside the existing single-level model. It also renames the network.topology.nvidia.com/accelerator label to xclr.topology.nvidia.com/domain with a new companion xclr.topology.nvidia.com/sub-domain label, and includes a legacy-label cleanup path so existing nodes are migrated on the first reconcile.

• pkg/topology/domain.go: Introduces BlockVertex (embedding Vertex + domain metadata) and GetDomainTree / setDesiredCountByLevel / getDesiredCountByLevel which build a one- or two-level padded block tree from a DomainMap, replacing the former flat domain-packing logic in block_tree.go.
• pkg/translate/block_tree.go: Replaces packDomainNodes / packAggregateNodes with a single recursive convert function that walks the BlockVertex tree produced by GetDomainTree, using an LCM-bounded padding loop to fill empty domain or sub-domain slots.
• All providers: AcceleratorID / ParentAcceleratorID fields in InstanceTopology are replaced by XclrDomainID + XclrSubDomainID; OCI's convertHost now sets XclrSubDomainID = domain + \".\" + rack when rack info is present.

Confidence Score: 5/5

Safe to merge; the dual-level tree construction and label rename are well-tested with unit and integration tests, and the legacy-label cleanup path handles in-place upgrades correctly.

All core algorithm changes are backed by focused unit tests and a full integration test using the new dual-level.yaml model. The label rename includes a legacyXclrDomainLabel cleanup so nodes are migrated on first reconcile without operator intervention. Both flagged observations are about future maintainability rather than current runtime defects.

Files Needing Attention: pkg/topology/domain.go — the unsafe.Pointer cast in asBlockVertex relies on an invariant that the exported Vertices field (inherited from embedded Vertex) cannot enforce at compile time; worth revisiting if BlockVertex usage is extended.

Important Files Changed

Filename	Overview
pkg/topology/domain.go	Introduces BlockVertex (embedding Vertex + domain metadata) and the GetDomainTree / setDesiredCountByLevel / getDesiredCountByLevel tree-building pipeline; the unsafe.Pointer cast in asBlockVertex is technically sound but fragile due to no compiler enforcement of the safety invariant on the exported Vertices field.
pkg/translate/block_tree.go	Replaces packDomainNodes/packAggregateNodes with a single recursive convert function that walks the BlockVertex tree; padding uses an LCM-bounded loop (blockTreeGCD) to handle non-divisor block sizes; logic and edge-case handling are well-reasoned and covered by tests.
pkg/translate/block_complement.go	Removes groupSizeFromDomains in favour of GetDomainTree-based sizing; complementBlocks is simplified to delegate tree construction to convert; the added contract comment for domainsForBlocks is a useful guard for future providers.
pkg/engines/k8s/kubernetes.go	Renames skipAcceleratorLabelWhenGPUCliqueExists to skipXclrLabelsWhenGPUCliqueExists and adds filtering of the new sub-domain label; adds legacyXclrDomainLabel cleanup so old nodes are migrated on first reconcile; the early-delete asymmetry is safe but could be confusing.
pkg/engines/k8s/labeler.go	Replaces single Accelerator label key with XclrDomain + XclrSubDomain pair; getDomainLabels now iterates over HostInfo values to conditionally write the sub-domain label only when SubDomain is non-empty; clean and correct.
pkg/topology/graph.go	Renames AcceleratorID/ParentAcceleratorID to XclrDomainID/XclrSubDomainID in InstanceTopology; ToGraph and toInstance are updated to populate both fields correctly, including the guard that prevents XclrSubDomain from being written if XclrDomain is not present.
pkg/providers/oci/instance_topology.go	convertHost now sets XclrDomainID from gpuMemoryFabricID and XclrSubDomainID = domain + rack when rack is non-empty, cleanly separating the domain and sub-domain identifiers that were previously conflated in AcceleratorID.
tests/models/dual-level.yaml	New simulation model for the dual-level integration test; intentional gaps (rack-1-03, rack-1-13 absent; rack-2-11 absent; rack-2-15 partial) exercise padding logic correctly.
pkg/topology/domain_test.go	Comprehensive new tests for GetDomainTree covering single-level, two-level, max-driven BFS sizing, single-blockSize rounding, unsorted blockSizes contract, and the fallback sub-domain placement for hosts without a SubDomain.
pkg/engines/nfd/objects.go	Renames topologyTypeAccelerator to topologyTypeXclrDomain and adds topologyTypeXclrSubDomain; topologyKind now uses strings.CutPrefix and maps both new XCLR label keys to their NFD feature types.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Provider: InstanceTopology\nXclrDomainID + XclrSubDomainID] --> B[ClusterTopology.ToGraph]
    B --> C[DomainMap.AddHostInfo\nHostInfo with Domain + SubDomain]
    C --> D[DomainMap.GetDomainTree\nblockSizes]
    D --> E{Has SubDomain?}
    E -- No --> F[Single-level\nDomain leaf BlockVertex\nwith all hosts]
    E -- Yes --> G[Dual-level\nDomain interior BlockVertex\nSub-domain leaf BlockVertex]
    F --> H[setDesiredCountByLevel BFS\nmax-driven per depth]
    G --> H
    H --> I[convert root BlockVertex\nrecursive aggregateBlockNode]
    I --> J{Leaf node?\nhosts != nil}
    J -- Yes --> K[splitIntoBaseBlocks\npad to groupSize base blocks]
    J -- No --> L[recurse into children\npad to LCM desiredNodeCount]
    K --> M[flat base block list]
    L --> M
    M --> N[Slurm block topology output]
    C --> O[labeler.getDomainLabels]
    O --> P[xclr domain + sub-domain node labels]

Loading

_{Reviews (27): Last reviewed commit: "incorporated review comments" | Re-trigger Greptile}

[ravisoundar]

/ok-to-test 507afeb

[github-actions]

🌿 Preview your docs: https://nvidia-preview-pull-request-416.docs.buildwithfern.com/topograph

[ravisoundar]

/ok-to-test 1411514

[ravisoundar]

/ok-to-test efa3725

[ravisoundar]

/ok-to-test fae6059

[ravisoundar]

/ok-to-test bdd65e8

[ravisoundar]

/ok-to-test 84b8b2c

[dmitsh]

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 == nil

convert 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.

[ravisoundar]

/ok-to-test 785bc8d

[ravisoundar]

/ok-to-test b6bb7ee

[dmitsh]

/ok-to-test 654a715

[ArangoGutierrez]

Solid docs discipline here — every breaking change is in the CHANGELOG and AGENTS.md/.claude/CLAUDE.md are updated in the same PR. The main concern is the block-tree rewrite: running the old and new generators side by side on identical single-level DomainMaps, the output diverges in both directions, which the SDD says should stay identical.

1. Single-level domains now pad to desiredNodeCount / baseBlockSize base blocks, where desiredNodeCount is the smallest configured BlockSize >= the largest domain. Whenever blockSizes[0] < maxDomainHosts < blockSizes[last], every domain gets padded out to a full top-tier block. A/B against the merge base on identical no-SubDomain DomainMaps: 8 domains x 18 hosts with BlockSizes=9,144 gives 16 blocks / 0 empty before and 128 / 112 after; 6 x 16 with 8,64 goes 16/4 to 48/36; 3 x 72 with 18,144 goes 16/4 to 24/12. The SDD goal states no-SubDomain domains must produce output identical to the pre-change behavior. (pkg/translate/block_tree.go:257)
2. The padding loop stops as soon as target.nodeCount reaches desiredNodeCount instead of rounding up to a multiple of it, so an incomplete top-tier group is no longer complemented. Same A/B, opposite direction: 5 single-level domains of 3 hosts with BlockSizes=4,16 emits 8 blocks before (5 real + 3 placeholders completing the second 16-node aggregate) and 5 after; 9 domains goes 12 to 9. The removed packAggregateNodes/newAggregateBlock padded every aggregate to a full groupSize, which is what protects Slurm's position-based aggregate inference. (pkg/translate/block_tree.go:287)
3. A host with an empty SubDomain inside a domain where others carry one is skipped entirely, so it never reaches a block and vanishes from topology.conf with only a klog warning. Verified: 8 racked hosts plus 1 without a SubDomain emits Nodes=node[0001-0008] and drops the ninth. OCI's convertComputeHost leaves rack empty whenever additionalData.locationDetails.rack is absent, so a partial GetComputeHost response silently removes those compute nodes. Before this change such a host landed in its own single-level domain and was always emitted. (pkg/topology/domain.go:161)
4. isManagedLevelLabel now matches only the new xclr keys, so network.topology.nvidia.com/accelerator is no longer treated as managed and removeManagedTopologyLabels never strips it. Nodes labelled by a previous release keep that key with a frozen value forever, alongside the new one. That string now appears nowhere in the tree except this CHANGELOG entry, so there is no cleanup path and no migration note — and CLAUDE.md names KAI Scheduler, NVSentinel and Kueue as consumers of it. Keeping the legacy key in the managed set would let one reconcile clean it up. (pkg/engines/k8s/kubernetes.go:68)
5. The gpu.clique shadow check skips only topologyTypeXclrDomain, so when a clique is present the domain is replaced by the clique value while the provider sub-domain is still published. TestBuildNFDObjectsGroupsXclrSubDomainsAndDoesNotGroupShadowedDomains encodes the result: domain gpu-clique-a together with sub-domain provider-domain-a.rack-a, nested under a domain that was never published. The k8s engine's skipXclrLabelsWhenGPUCliqueExists drops both levels, and node-labels.md says the sub-domain is written only when the domain is. Adding topologyTypeXclrSubDomain to the same continue would line them up. (pkg/engines/nfd/objects.go:95)
6. GetDomainTree, setDesiredCountByLevel and BlockVertex add roughly 120 lines of exported behavior to the load-bearing topology package with no direct unit test — no *_test.go in the tree references any of the three, and domain_test.go still holds only TestDomainMapAddHost. The BFS level sizing, the empty-SubDomain skip path and the documented "blockSizes need not be sorted" contract are reached only indirectly through translate's rendered-output assertion, and the SDD test plan lists them as covered by pkg/topology tests. (pkg/topology/domain.go:127)
7. topology.KeyTopologyAcceleratorSubDomain never existed — git log -S finds it only in this PR's own commit, and the base declares just KeyTopologyAccelerator. KeyTopologyXclrSubDomain is a new constant, so this reads better under Added than as a BREAKING rename. (CHANGELOG.md:29)
8. This says retaining a provider-supplied XCLR sub-domain when nvidia.com/gpu.clique is present matches the k8s engine, but skipXclrLabelsWhenGPUCliqueExists deletes both the domain and the sub-domain. Worth correcting in whichever direction the NFD behavior gets resolved. (docs/design/nfd-engine-sdd.md:234)
9. This delete is redundant with removeManagedTopologyLabels, which already treats the sub-domain key as managed. Removing the line keeps pkg/engines, pkg/topology and pkg/translate green, so nothing pins the intended behavior — "Case 8: remove stale accelerator sub-domain label" exercises the removeManagedTopologyLabels path instead. (pkg/engines/k8s/kubernetes.go:102)

[ArangoGutierrez]

The gpu.clique shadow check skips only topologyTypeXclrDomain, so when a clique is present the domain is replaced by the clique value while the provider sub-domain is still published. TestBuildNFDObjectsGroupsXclrSubDomainsAndDoesNotGroupShadowedDomains encodes the result: domain gpu-clique-a together with sub-domain provider-domain-a.rack-a, nested under a domain that was never published. The k8s engine's skipXclrLabelsWhenGPUCliqueExists drops both levels, and node-labels.md says the sub-domain is written only when the domain is. Adding topologyTypeXclrSubDomain to the same continue would line them up.

[ravisoundar]

@dmitsh, not sure if this is the desired behavior. I assume sub-domain information may still be required.

[ArangoGutierrez]

This says retaining a provider-supplied XCLR sub-domain when nvidia.com/gpu.clique is present matches the k8s engine, but skipXclrLabelsWhenGPUCliqueExists deletes both the domain and the sub-domain. Worth correcting in whichever direction the NFD behavior gets resolved.

[ravisoundar]

/ok-to-test 22de1b7

[ravisoundar]

/ok-to-test a778d23