fix(topology): canonicalize compute instance ordering

[teerthsharma]

Description

Closes #421.

Go map iteration is unspecified, but several discovery paths converted maps to ordered provider inputs without sorting. The same logical ComputeInstances set could therefore change:

• region/API fan-out order;
• AWS InstanceIds order;
• InfiniBand probe-host order; and
• which error is returned first when multiple regions fail.

This is observable before final serialization in retries, logs, metrics, probes, and error precedence. It does not claim that every successful run produced different topology.conf bytes.

Changes

• Add a shared, non-mutating CanonicalComputeInstances total order.
• Apply it at the request, Kubernetes, Slinky, Slurm, and AWS boundaries.
• Sort AWS explicit instance IDs and host-oriented node lists.
• Replace order-insensitive assertions with exact traces across explicit permutations.
• Record the fix in CHANGELOG.md.

The canonical topology.Graph shape and provider/engine responsibilities are unchanged.

Validation

Fresh on PR head 969fad9 after merging current main:

• go vet on all 7 affected packages: pass
• focused go test on all 7 affected packages: pass
• git diff --check: pass
• exact tests cover K8s, Slinky, Slurm, AWS, InfiniBand, deterministic first-error precedence, and caller-input non-mutation

NVIDIA runner workflows are pending the repository's external-contributor validation step. Security validation found a reliability/reproducibility defect, not a security vulnerability: membership, call count, command content, privileges, and exposed data are unchanged.

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.

[greptile-apps]

Greptile Summary

This PR fixes non-deterministic compute-instance ordering that originated from Go map iteration being unspecified. It introduces a shared CanonicalComputeInstances helper (region-sorted, non-mutating via slices.Clone) and applies it at every fan-out boundary: the server request path, K8s/Slinky/Slurm engine outputs, AWS region iteration and instance-ID list, OCI node and instance-ID lists, DSX node ID list, and the K8s label-apply loop.

• topology.CanonicalComputeInstances is the single source of truth for region order; it performs a shallow clone so caller slices and maps are not mutated, verified explicitly in new tests.
• Every affected package adds or upgrades tests to assert exact ordering across input permutations, replacing ElementsMatch with Equal where applicable.
• The InfiniBand probe-host order is determined by sorted node names (via GetNodeNameList), while all other fan-out boundaries use region-sorted CanonicalComputeInstances; the new InfiniBand test data (region-b→node-a, region-a→node-b) correctly distinguishes the two orderings.

Confidence Score: 5/5

Safe to merge — the changes are purely additive determinism fixes with no behavioral changes on the happy path and thorough permutation tests.

Every fan-out boundary is covered, the canonical helper is non-mutating by construction (slices.Clone), the double-sort on the AWS server path is harmless and documented in a comment, and new tests verify exact ordering across input permutations for all affected packages. No data loss, no auth changes, no schema changes.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename	Overview
pkg/topology/request.go	Introduces CanonicalComputeInstances (non-mutating, region-sorted clone) and slices.Sort for node names in GetNodeNameList; also replaces sort.Strings with slices.Sort for consistency.
pkg/server/engine.go	Refactors getComputeInstances to canonicalize output unconditionally across all three code paths (requested, provider-provided, engine-provided).
pkg/providers/aws/instance_topology.go	Adds inner CanonicalComputeInstances call to generateInstanceTopology (self-contained for direct callers) and sorts InstanceIds using slices.Sorted(maps.Keys(...)) instead of unordered map iteration.
pkg/providers/aws/instance_topology_test.go	New tests verify region order and instance-ID sort across both permutations of inputs, and test deterministic first-error selection using a recording client stub.
pkg/providers/infiniband/common_test.go	Adds TestGetIbTreeUsesCanonicalNodeOrder with intentionally cross-mapped regions/nodes (region-b→node-a, region-a→node-b) so the expected order would fail if sorting were done by region instead of node name.
pkg/providers/oci/provider_imds.go	Sorts node names before the getHostTopology API call and iterates instance IDs in sorted key order when building the output, making both the API request and topo-append order deterministic.
pkg/providers/dsx/instance_topology.go	Adds slices.Sort(nodeIDs) after collecting instance IDs so the single cross-region API call receives a stable, deterministic list.
pkg/engines/k8s/labeler.go	Iterates nodeMap in sorted key order via slices.Sorted(maps.Keys(...)) so label-apply calls are deterministic.
internal/k8s/utils.go	Returns topology.CanonicalComputeInstances(cis) instead of the raw map-iteration-ordered slice from GetComputeInstances.
pkg/engines/slinky/engine.go	Returns topology.CanonicalComputeInstances(cis) from getComputeInstances, matching the same fix applied to the Kubernetes and Slurm engines.
pkg/engines/slurm/slurm.go	Returns topology.CanonicalComputeInstances(cis) from aggregateComputeInstances, completing the Slurm engine determinism fix.
pkg/server/engine_test.go	New test verifies getComputeInstances returns a canonicalized slice without mutating the caller-provided requested slice.
pkg/topology/request_test.go	Promotes GetNodeNameList assertion from ElementsMatch to Equal (exact order), adds four-entry out-of-order input, and adds a full CanonicalComputeInstances round-trip test including non-mutation assertion.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Server as pkg/server/engine.go
    participant Engine as Engine (k8s/slinky/slurm)
    participant Provider as Provider (aws/oci/dsx)
    participant Topo as pkg/topology

    Client->>Server: processTopologyRequest(tr)
    Server->>Server: getComputeInstances(requested)
    alt requested non-empty
        Server->>Topo: CanonicalComputeInstances(requested)
    else provider implements GetComputeInstances
        Server->>Provider: GetComputeInstances(ctx)
        Provider-->>Server: raw cis
        Server->>Topo: CanonicalComputeInstances(cis)
    else
        Server->>Engine: GetComputeInstances(ctx, prv)
        Note over Engine: k8s/slinky/slurm already return<br/>CanonicalComputeInstances internally
        Engine-->>Server: canonical cis
        Server->>Topo: CanonicalComputeInstances(cis) [idempotent]
    end
    Topo-->>Server: canonical []ComputeInstances
    Server->>Provider: GenerateTopologyConfig(ctx, cis)
    Note over Provider: AWS: CanonicalComputeInstances again + slices.Sorted(maps.Keys)<br/>OCI: slices.Sort(nodes) + sorted instanceIDs<br/>DSX: slices.Sort(nodeIDs)
    Provider-->>Server: topology.Graph
    Server-->>Client: engine output

Loading

_{Reviews (2): Last reviewed commit: "fix(topology): address maintainer feedba..." | Re-trigger Greptile}

[ArangoGutierrez]

Thanks for the thorough writeup. The audit in #421 is accurate: every path you list really does derive an ordered slice from an unordered source, and the line references check out. The permutation cases plus the non-mutation assertions in pkg/server/engine_test.go and pkg/topology/request_test.go are the right technique for this class of property; asserting against an exact recorded trace, rather than re-running a map loop and hoping the runtime reorders, is how you actually test it. You also scoped the claim carefully in the PR body instead of asserting that successful runs emit different bytes, which is the version that would not have survived.

I traced each of the five paths to its terminus before writing this.

Paths 1, 2, 3 (slurm aggregateComputeInstances, internal/k8s.GetComputeInstances, slinky getComputeInstances) all feed prv.GenerateTopologyConfig, and for every ClusterTopology-based provider that ends in ToGraph at pkg/topology/graph.go:87. ToGraph inserts each vertex into maps keyed by ID: forest at graph.go:93, nodes[tierKey] at graph.go:98 and graph.go:134-142, treeRoot.Vertices via maps.Copy at graph.go:169-172. No append position survives that.

Path 4, pdsh half. provider_bm.go:36 hands the list to exec.Pdsh, which passes it through cluset.Compact at internal/exec/exec.go:63. Compact sorts prefixes at internal/cluset/cluset.go:54 and numeric suffixes at cluset.go:60-62. The -w argument is canonical before it reaches pdsh, so the sort.Strings you added at request.go:159 is erased one call downstream.

Path 4, IB half. Probe order is genuinely observable: getIbTree at pkg/providers/infiniband/common.go:32 contacts the first unvisited host. But the result lands in rootMap, read back in map order at common.go:58, and NewMerger re-sorts by vertex ID at pkg/topology/merger.go:33-35. A failed probe is klog.Warningf and continue at common.go:35-37, never an error.

Path 5 (AWS InstanceIds). This one does change bytes on the wire. The response is then selected by map lookup at instance_topology.go:87 and appended to ClusterTopology.Instances, terminating in the same ToGraph. Worth noting AWS passes normalize=false at pkg/providers/aws/provider.go:191, so the sort in Normalize (graph.go:218-236) never runs on this path, and the output is stable regardless.

I checked that empirically rather than by reading alone. On main at edb20aa, without this PR, permuting both the region fan-out order and the within-region append order produces a deep-equal *topology.Graph and byte-identical topology.conf under topology/tree and topology/block, at both normalize=false and normalize=true. Since GenerateOutput takes only the graph and its params (pkg/engines/engines.go:31), an identical Graph means identical output for slurm, k8s, nfd, and slinky alike.

That leaves the three claims that survive the byte-identical objection, which are the interesting ones. First-error precedence is real: generateInstanceTopology returns on the first failing region at instance_topology.go:38-41, so with two broken regions the message names an arbitrary one. The IB probe order and the AWS request order are as described above.

So the question I would put back to you, for each of those three: what does an operator see that is different, and what would they do differently as a result? For first-error precedence, the API returns 502 either way, and processRequestWithRetries at pkg/server/engine.go:54-76 retries the whole request (502 is retryable at internal/httpreq/httpreq.go:44) up to maxRetries = 5 at engine.go:36. There is no per-region retry whose behavior depends on which region failed first. For the other two I could not find the symptom at all. I also went looking for the historical cost, since a property like this usually leaves a trail: the only ordering work in this repo is #207 (fix(slurm): fix block order) and #13, both fixed in pkg/translate at the serializer, neither traceable to discovery order. No flaky-test issue, no support issue, no retry logic keyed on order.

On cost, in maintainer terms. The rule here is that added complexity has to buy something a user or operator can observe, and this lands a comparator in pkg/topology, the one package we deliberately do not change casually because every provider and engine depends on its shape. Two specifics. The tie-break half of compareComputeInstances (request.go:65-76, everything after the Region compare) is unreachable through every in-tree producer: slurm keys by region at slurm.go:132, k8s at internal/k8s/utils.go:108 with 122-123, slinky at slinky/engine.go:270 with 290-291. None can emit two entries sharing a Region, so that code is exercised only by its own unit test. And converting ElementsMatch to Equal at slurm_test.go:82 and request_test.go:185 prices in every future ordering change; those assertions were not masking a bug, they were encoding that order is not part of the contract.

There is also a consistency gap worth naming: the change does not fully close the property it is named after. The k8s engine's outbound writes still go out in map order at pkg/engines/k8s/labeler.go:105, and pkg/providers/dsx/instance_topology.go:26-30 and pkg/providers/oci/provider_imds.go:71-74 still build request arguments straight from map iteration. Closing those would roughly double the surface, and by the argument above it would not buy an observable symptom either. (Greptile's note about the second CanonicalComputeInstances at instance_topology.go:40 is correct as well; the slice is already canonical from pkg/server/engine.go:146.)

Where that leaves me: I think the right call for the project is to close #422 and relabel #421 as a feature request we keep on file. If a real reproducibility requirement shows up later (a byte-stable audit trail of outbound API calls, or a replay harness), the audit you wrote is exactly the document we would start from, and it will still be accurate then. That is worth more parked and correct than merged and unexercised. You reached the "feature request, not bug" read yourself in #421; this is the same conclusion applied to the code.

If you want something here with clear value, #386 is open and unassigned and would suit you: engine/provider-gate the main Topograph ClusterRole. Today the api-server holds cluster-wide nodes:update plus pods and daemonsets reads regardless of the configured engine and provider, including combinations that make zero Kubernetes API calls. The issue already carries the per-verb call-site table, an acceptance-criteria list, and the one real design decision (omit the ClusterRole and its binding entirely for zero-rule combos, rather than rendering empty rules), so there is nothing ambiguous left to negotiate. It stacks on #384, which is merged, so you have a worked example of the same gating pattern in the same template. Scope is charts/topograph/templates/rbac.yaml plus helm-unittest cases, and the benefit is one an operator can see in kubectl auth can-i. I will review it promptly.

Say the word and it is yours. Either way, send the second PR.

[teerthsharma]

Thanks for the thorough writeup. The audit in #421 is accurate: every path you list really does derive an ordered slice from an unordered source, and the line references check out. The permutation cases plus the non-mutation assertions in pkg/server/engine_test.go and pkg/topology/request_test.go are the right technique for this class of property; asserting against an exact recorded trace, rather than re-running a map loop and hoping the runtime reorders, is how you actually test it. You also scoped the claim carefully in the PR body instead of asserting that successful runs emit different bytes, which is the version that would not have survived.

I traced each of the five paths to its terminus before writing this.

Paths 1, 2, 3 (slurm aggregateComputeInstances, internal/k8s.GetComputeInstances, slinky getComputeInstances) all feed prv.GenerateTopologyConfig, and for every ClusterTopology-based provider that ends in ToGraph at pkg/topology/graph.go:87. ToGraph inserts each vertex into maps keyed by ID: forest at graph.go:93, nodes[tierKey] at graph.go:98 and graph.go:134-142, treeRoot.Vertices via maps.Copy at graph.go:169-172. No append position survives that.

Path 4, pdsh half. provider_bm.go:36 hands the list to exec.Pdsh, which passes it through cluset.Compact at internal/exec/exec.go:63. Compact sorts prefixes at internal/cluset/cluset.go:54 and numeric suffixes at cluset.go:60-62. The -w argument is canonical before it reaches pdsh, so the sort.Strings you added at request.go:159 is erased one call downstream.

Path 4, IB half. Probe order is genuinely observable: getIbTree at pkg/providers/infiniband/common.go:32 contacts the first unvisited host. But the result lands in rootMap, read back in map order at common.go:58, and NewMerger re-sorts by vertex ID at pkg/topology/merger.go:33-35. A failed probe is klog.Warningf and continue at common.go:35-37, never an error.

Path 5 (AWS InstanceIds). This one does change bytes on the wire. The response is then selected by map lookup at instance_topology.go:87 and appended to ClusterTopology.Instances, terminating in the same ToGraph. Worth noting AWS passes normalize=false at pkg/providers/aws/provider.go:191, so the sort in Normalize (graph.go:218-236) never runs on this path, and the output is stable regardless.

I checked that empirically rather than by reading alone. On main at edb20aa, without this PR, permuting both the region fan-out order and the within-region append order produces a deep-equal *topology.Graph and byte-identical topology.conf under topology/tree and topology/block, at both normalize=false and normalize=true. Since GenerateOutput takes only the graph and its params (pkg/engines/engines.go:31), an identical Graph means identical output for slurm, k8s, nfd, and slinky alike.

That leaves the three claims that survive the byte-identical objection, which are the interesting ones. First-error precedence is real: generateInstanceTopology returns on the first failing region at instance_topology.go:38-41, so with two broken regions the message names an arbitrary one. The IB probe order and the AWS request order are as described above.

So the question I would put back to you, for each of those three: what does an operator see that is different, and what would they do differently as a result? For first-error precedence, the API returns 502 either way, and processRequestWithRetries at pkg/server/engine.go:54-76 retries the whole request (502 is retryable at internal/httpreq/httpreq.go:44) up to maxRetries = 5 at engine.go:36. There is no per-region retry whose behavior depends on which region failed first. For the other two I could not find the symptom at all. I also went looking for the historical cost, since a property like this usually leaves a trail: the only ordering work in this repo is #207 (fix(slurm): fix block order) and #13, both fixed in pkg/translate at the serializer, neither traceable to discovery order. No flaky-test issue, no support issue, no retry logic keyed on order.

On cost, in maintainer terms. The rule here is that added complexity has to buy something a user or operator can observe, and this lands a comparator in pkg/topology, the one package we deliberately do not change casually because every provider and engine depends on its shape. Two specifics. The tie-break half of compareComputeInstances (request.go:65-76, everything after the Region compare) is unreachable through every in-tree producer: slurm keys by region at slurm.go:132, k8s at internal/k8s/utils.go:108 with 122-123, slinky at slinky/engine.go:270 with 290-291. None can emit two entries sharing a Region, so that code is exercised only by its own unit test. And converting ElementsMatch to Equal at slurm_test.go:82 and request_test.go:185 prices in every future ordering change; those assertions were not masking a bug, they were encoding that order is not part of the contract.

There is also a consistency gap worth naming: the change does not fully close the property it is named after. The k8s engine's outbound writes still go out in map order at pkg/engines/k8s/labeler.go:105, and pkg/providers/dsx/instance_topology.go:26-30 and pkg/providers/oci/provider_imds.go:71-74 still build request arguments straight from map iteration. Closing those would roughly double the surface, and by the argument above it would not buy an observable symptom either. (Greptile's note about the second CanonicalComputeInstances at instance_topology.go:40 is correct as well; the slice is already canonical from pkg/server/engine.go:146.)

Where that leaves me: I think the right call for the project is to close #422 and relabel #421 as a feature request we keep on file. If a real reproducibility requirement shows up later (a byte-stable audit trail of outbound API calls, or a replay harness), the audit you wrote is exactly the document we would start from, and it will still be accurate then. That is worth more parked and correct than merged and unexercised. You reached the "feature request, not bug" read yourself in #421; this is the same conclusion applied to the code.

If you want something here with clear value, #386 is open and unassigned and would suit you: engine/provider-gate the main Topograph ClusterRole. Today the api-server holds cluster-wide nodes:update plus pods and daemonsets reads regardless of the configured engine and provider, including combinations that make zero Kubernetes API calls. The issue already carries the per-verb call-site table, an acceptance-criteria list, and the one real design decision (omit the ClusterRole and its binding entirely for zero-rule combos, rather than rendering empty rules), so there is nothing ambiguous left to negotiate. It stacks on #384, which is merged, so you have a worked example of the same gating pattern in the same template. Scope is charts/topograph/templates/rbac.yaml plus helm-unittest cases, and the benefit is one an operator can see in kubectl auth can-i. I will review it promptly.

Say the word and it is yours. Either way, send the second PR.

I will be working as told ,I have fixed almost every caveat of this PR , hoping this will provide good reference for the feature request .I have also started working on the issue told thanks for your time