Conversation
Signed-off-by: Dmitry Shmulevich <17212177+dmitsh@users.noreply.github.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughTopograph-owned Kubernetes labels and annotations migrate to the ChangesMetadata domain migration
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🟠 High · up to This change migrates Topograph metadata keys, but upgrades may leave existing NFD objects with the old keys that are no longer cleaned up, allowing stale topology information to remain active. Merge should be blocked until cleanup supports both domains or explicitly removes pre-migration objects. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR migrates Topograph-owned Kubernetes labels and annotations from
Confidence Score: 3/5This PR should not merge until NFD can clean up legacy managed resources and the unintended exported Go API removal is restored or explicitly handled. The new NFD ownership selectors cannot discover obsolete resources created under the previous domain, and removing Files Needing Attention: pkg/engines/nfd/objects.go, pkg/topology/topology.go, CHANGELOG.md Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Producers[Broker, providers, and KWOK] -->|topograph.run node annotations| Nodes[(Kubernetes Nodes)]
Nodes --> Consumers[Engines and accelerator discovery]
NFD[NFD engine] -->|topograph.run ownership labels| Objects[(NodeFeature resources)]
Cleanup[NFD cleanup] -->|new-domain selector| Objects
Legacy[(Pre-upgrade NFD resources)] -. old-domain labels not selected .-> Cleanup
Reviews (1): Last reviewed commit: "feat(topology): migrate metadata keys to..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| labelManagedBy = "app.kubernetes.io/managed-by" | ||
| labelEngine = "topograph.nvidia.com/engine" | ||
| labelResource = "topograph.nvidia.com/resource" | ||
| labelGroupType = "topograph.nvidia.com/group-type" | ||
| labelEngine = "topograph.run/engine" | ||
| labelResource = "topograph.run/resource" |
There was a problem hiding this comment.
When upgrading a cluster with existing NFD resources, cleanup selects only objects carrying the new topograph.run ownership labels. Obsolete resources that retain topograph.nvidia.com labels are excluded from cleanup and continue publishing stale node features or topology groups.
Knowledge Base Used: Engines: Topology Generation
| KeyNodeInstance = "topograph.run/instance" | ||
| KeyNodeRegion = "topograph.run/region" | ||
| KeyGpuClusterID = "topograph.run/cluster-id" |
There was a problem hiding this comment.
When a downstream Go package references topology.KeyNvidiaGPUProduct, upgrading to this revision fails compilation because the exported constant has been removed. The key is NVIDIA-owned rather than Topograph-owned, so this removal is unrelated to the metadata-domain migration and has no documented replacement.
Knowledge Base Used: Topology Model
| @@ -71,7 +72,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). | |||
|
|
|||
| ### Removed | |||
|
|
|||
There was a problem hiding this comment.
This deletes the unreleased breaking-change notice for topology.KeyNvidiaGPUClique even though the constant remains removed. Consumers reviewing the release notes consequently lose the applicable migration guidance to configure accelerator discovery through provider.params.accelerator.kubernetesLabel.key.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #465 +/- ##
==========================================
+ Coverage 72.15% 77.13% +4.97%
==========================================
Files 89 99 +10
Lines 5689 7212 +1523
==========================================
+ Hits 4105 5563 +1458
+ Misses 1382 1359 -23
- Partials 202 290 +88 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/kwok/nodes_test.go`:
- Line 113: Expand the migration assertions in the affected test around the
manifest checks: retain the positive assertion for topograph.run/instance, add a
negative assertion that topograph.nvidia.com/instance is absent, and assert the
renamed topograph.run/region key using the fixture’s expected value.
In `@pkg/engines/nfd/objects.go`:
- Around line 50-59: Update cleanupResource to discover and delete stale
NodeFeature and NodeFeatureGroup objects using both the current topograph.run
ownership labels and legacy topograph.nvidia.com ownership labels during
migration. Preserve existing cleanup behavior for new labels, and add regression
coverage for a stale object carrying only the legacy ownership labels.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c0bc3f10-3459-4266-ab20-2e0e982b1d36
📒 Files selected for processing (17)
CHANGELOG.mdcharts/topograph/Chart.yamldemos/test-nfd/demo.shdocs/design/nfd-engine-sdd.mddocs/engines/graph.mddocs/engines/nfd.mddocs/engines/slinky.mddocs/modeling.mddocs/providers/dra.mddocs/providers/infiniband.mddocs/providers/lambdai.mddocs/reference/node-labels.mdinternal/kwok/nodes_test.gopkg/engines/graph/engine_test.gopkg/engines/nfd/objects.gopkg/topology/graph_test.gopkg/topology/topology.go
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: oci-sim / slinky
- GitHub Check: check
- GitHub Check: govulncheck
- GitHub Check: build
- GitHub Check: test
- GitHub Check: k8s / gcp-sim
- GitHub Check: k8s / aws-sim
- GitHub Check: k8s / test
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (11)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Do not publicly disclose suspected security vulnerabilities; report them privately through NVIDIA PSIRT using the channels documented inSECURITY.md.
Every commit must include aSigned-off-by:trailer for DCO compliance.
Use Conventional Commits with an allowed type such asfeat,fix,docs,chore,refactor,test,build, orci.
Before pushing, runmake qualify; all CI checks, including Go build/test/lint, Codecov, and DCO, must be green before merge.
Changes to provider, engine, chart, API, configuration schema, labels, annotations, or user-facing behavior must include the corresponding documentation updates described in the documentation impact table.
Files:
demos/test-nfd/demo.shdocs/engines/graph.mdinternal/kwok/nodes_test.godocs/design/nfd-engine-sdd.mddocs/providers/dra.mddocs/engines/slinky.mddocs/reference/node-labels.mddocs/providers/lambdai.mdpkg/topology/topology.godocs/engines/nfd.mdpkg/topology/graph_test.godocs/modeling.mddocs/providers/infiniband.mdpkg/engines/nfd/objects.gopkg/engines/graph/engine_test.gocharts/topograph/Chart.yamlCHANGELOG.md
docs/engines/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
New, changed, or removed engines require updates to the corresponding engine documentation.
Files:
docs/engines/graph.mddocs/engines/slinky.mddocs/engines/nfd.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Rungo fmt ./...; Go formatting is authoritative and code should not be hand-formatted.
Every new Go file must include the NVIDIA copyright header followed by the matching Apache 2.0 boilerplate.
Files:
internal/kwok/nodes_test.gopkg/topology/topology.gopkg/topology/graph_test.gopkg/engines/nfd/objects.gopkg/engines/graph/engine_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
New or changed public behavior and new code paths should be covered by tests.
Files:
internal/kwok/nodes_test.gopkg/topology/graph_test.gopkg/engines/graph/engine_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Flag missing negative, cancellation, concurrency, malformed-input,
boundary, and regression cases. Check that assertions would fail if
the behavior under test were broken.
Files:
internal/kwok/nodes_test.gopkg/topology/graph_test.gopkg/engines/graph/engine_test.go
{cmd,pkg,internal}/**/*.go
⚙️ CodeRabbit configuration file
{cmd,pkg,internal}/**/*.go: Focus on correctness, robustness, and failure paths:
- Check error handling and propagation, including whether callers
receive enough context to diagnose failures.- Verify context propagation, cancellation, timeouts, resource cleanup,
and goroutine lifecycle.- Look for races, deadlocks, unsafe shared state, leaks, and partial
updates that can leave state inconsistent.- Check nil, empty, malformed, duplicate, boundary, and partial inputs.
- Verify deterministic and idempotent behavior where operations may be
retried or repeated.- Preserve public contracts and the architectural boundaries documented
in AGENTS.md.- Flag behavior changes that lack meaningful regression coverage.
Files:
internal/kwok/nodes_test.gopkg/topology/topology.gopkg/topology/graph_test.gopkg/engines/nfd/objects.gopkg/engines/graph/engine_test.go
docs/providers/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Provider additions or changes require the corresponding provider documentation, including prerequisites, credentials, parameters, operation, and verification.
Files:
docs/providers/dra.mddocs/providers/lambdai.mddocs/providers/infiniband.md
pkg/topology/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do not change
Graph, theVertextree, or topology constants casually; changes require discussion because every provider and engine depends on them.
Files:
pkg/topology/topology.gopkg/topology/graph_test.go
pkg/engines/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Engines only translate the canonical topology and must not discover or read the physical fabric.
Files:
pkg/engines/nfd/objects.gopkg/engines/graph/engine_test.go
charts/topograph/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Do not enable both
ingress.enabledandgatewayAPI.enabledin one Helm release; the routing resources are mutually exclusive.
Files:
charts/topograph/Chart.yaml
charts/topograph/**
⚙️ CodeRabbit configuration file
charts/topograph/**: - Check RBAC least privilege and Kubernetes API compatibility.
- ingress.enabled and gatewayAPI.enabled must remain mutually exclusive.
- HTTPRoute must contain only portable Gateway API v1 fields.
- Flag changes where values, schema, templates, NOTES, tests,
snapshots, documentation, or changelog become inconsistent.
Files:
charts/topograph/Chart.yaml
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Record applicable user-facing features, fixes, breaking changes, or Helm migrations under
[Unreleased].
Files:
CHANGELOG.md
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to pkg/engines/k8s/**/*.go : Do not invent Kubernetes label keys in provider code; use canonical graph values and the configured closest-first `fabricLabels` and fixed accelerator sub-domain key.
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to charts/topograph/tests/**/*.{yaml,yml} : Run Helm chart tests when changing `charts/topograph/`; review snapshot changes before committing updates.
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to **/* : Changes to provider, engine, chart, API, configuration schema, labels, annotations, or user-facing behavior must include the corresponding documentation updates described in the documentation impact table.
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to pkg/topology/**/*.go : Do not change `Graph`, the `Vertex` tree, or topology constants casually; changes require discussion because every provider and engine depends on them.
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to pkg/providers/**/*.go : When using `ClusterTopology`, populate fabric tiers closest-first, preserve optional accelerator domain and sub-domain fields, and call `ToGraph`.
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to docs/engines/**/*.md : New, changed, or removed engines require updates to the corresponding engine documentation.
Applied to files:
docs/engines/graph.md
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to pkg/engines/k8s/**/*.go : Do not invent Kubernetes label keys in provider code; use canonical graph values and the configured closest-first `fabricLabels` and fixed accelerator sub-domain key.
Applied to files:
docs/design/nfd-engine-sdd.mddocs/reference/node-labels.mddocs/providers/lambdai.mdpkg/topology/topology.godocs/engines/nfd.mdpkg/topology/graph_test.godocs/providers/infiniband.mdpkg/engines/nfd/objects.gopkg/engines/graph/engine_test.goCHANGELOG.md
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to **/* : Changes to provider, engine, chart, API, configuration schema, labels, annotations, or user-facing behavior must include the corresponding documentation updates described in the documentation impact table.
Applied to files:
docs/engines/slinky.mddocs/reference/node-labels.mddocs/providers/lambdai.mddocs/engines/nfd.mddocs/providers/infiniband.mdcharts/topograph/Chart.yamlCHANGELOG.md
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to pkg/topology/**/*.go : Do not change `Graph`, the `Vertex` tree, or topology constants casually; changes require discussion because every provider and engine depends on them.
Applied to files:
docs/reference/node-labels.mdpkg/topology/topology.gopkg/topology/graph_test.gopkg/engines/nfd/objects.gopkg/engines/graph/engine_test.gocharts/topograph/Chart.yamlCHANGELOG.md
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to pkg/providers/**/*.go : When using `ClusterTopology`, populate fabric tiers closest-first, preserve optional accelerator domain and sub-domain fields, and call `ToGraph`.
Applied to files:
pkg/topology/topology.godocs/providers/infiniband.mdpkg/engines/nfd/objects.goCHANGELOG.md
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to pkg/providers/**/*.go : Providers discover topology and return the canonical `topology.Graph`; they must not emit scheduler-specific output.
Applied to files:
pkg/topology/topology.godocs/providers/infiniband.mdpkg/engines/graph/engine_test.go
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to pkg/providers/**/*.go : Providers must return a `*topology.Graph` and `*httperr.Error`; plain `error` is not acceptable at the provider interface boundary.
Applied to files:
pkg/topology/topology.go
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to pkg/engines/**/*.go : Engines only translate the canonical topology and must not discover or read the physical fabric.
Applied to files:
pkg/engines/nfd/objects.gopkg/engines/graph/engine_test.go
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to pkg/providers/*/provider.go : A provider must expose `func NamedLoader() (string, providers.Loader)` and be registered in `pkg/registry/registry.go`.
Applied to files:
pkg/engines/graph/engine_test.go
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to charts/topograph/tests/**/*.{yaml,yml} : Run Helm chart tests when changing `charts/topograph/`; review snapshot changes before committing updates.
Applied to files:
charts/topograph/Chart.yamlCHANGELOG.md
📚 Learning: 2026-08-08T10:57:01.397Z
Learnt from: CR
Repo: NVIDIA/topograph PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T10:57:01.397Z
Learning: Applies to CHANGELOG.md : Record applicable user-facing features, fixes, breaking changes, or Helm migrations under `[Unreleased]`.
Applied to files:
CHANGELOG.md
🔇 Additional comments (15)
CHANGELOG.md (1)
31-31: LGTM!Also applies to: 48-48, 59-65, 74-75
charts/topograph/Chart.yaml (1)
15-15: 📐 Maintainability & Code QualityRun the required chart validation before merge.
Because
charts/topograph/Chart.yamlchanged, run the Helm chart tests andmake qualify. Review any chart snapshots produced by the test run.As per coding guidelines: “Before pushing, run
make qualify; all CI checks, including Go build/test/lint, Codecov, and DCO, must be green before merge.”Based on learnings: “Run Helm chart tests when changing
charts/topograph/; review snapshot changes before committing updates.”Sources: Coding guidelines, Learnings
pkg/topology/topology.go (1)
35-37: LGTM!Also applies to: 46-51
demos/test-nfd/demo.sh (1)
30-30: LGTM!docs/design/nfd-engine-sdd.md (1)
93-96: LGTM!docs/engines/nfd.md (1)
124-124: LGTM!Also applies to: 150-154
docs/engines/slinky.md (1)
166-171: LGTM!Also applies to: 187-192
docs/modeling.md (1)
60-66: LGTM!pkg/topology/graph_test.go (1)
14-14: LGTM!Also applies to: 186-186, 202-202
pkg/engines/graph/engine_test.go (1)
19-19: LGTM!Also applies to: 111-111, 123-123
docs/providers/dra.md (1)
114-117: LGTM!docs/providers/infiniband.md (1)
94-101: LGTM!Also applies to: 132-132
docs/providers/lambdai.md (1)
239-240: LGTM!docs/reference/node-labels.md (1)
154-167: LGTM!docs/engines/graph.md (1)
24-24: LGTM!
| require.Contains(t, manifest, "kind: List") | ||
| require.Contains(t, manifest, "name: i21") | ||
| require.Contains(t, manifest, "topograph.nvidia.com/instance: i-I21") | ||
| require.Contains(t, manifest, "topograph.run/instance: i-I21") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add negative migration coverage.
require.Contains proves that the new instance key appears, but it also passes if the old key is emitted. Add an assertion that the manifest does not contain topograph.nvidia.com/instance. Also cover the renamed topograph.run/region key with the fixture's expected value.
As per path instructions, changed tests must cover regression cases for metadata migrations.
Proposed test addition
require.Contains(t, manifest, "topograph.run/instance: i-I21")
+ require.NotContains(t, manifest, "topograph.nvidia.com/instance")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/kwok/nodes_test.go` at line 113, Expand the migration assertions in
the affected test around the manifest checks: retain the positive assertion for
topograph.run/instance, add a negative assertion that
topograph.nvidia.com/instance is absent, and assert the renamed
topograph.run/region key using the fixture’s expected value.
Source: Path instructions
| labelEngine = "topograph.run/engine" | ||
| labelResource = "topograph.run/resource" | ||
| labelGroupType = "topograph.run/group-type" | ||
|
|
||
| managedByTopograph = "topograph" | ||
| resourceNodeFeature = "nodefeature" | ||
| resourceNodeFeatureGroup = "nodefeaturegroup" | ||
| annotationNodeName = "topograph.nvidia.com/node-name" | ||
| annotationTopologyLabelKey = "topograph.nvidia.com/label-key" | ||
| annotationTopologyValue = "topograph.nvidia.com/label-value" | ||
| annotationNodeName = "topograph.run/node-name" | ||
| annotationTopologyLabelKey = "topograph.run/label-key" | ||
| annotationTopologyValue = "topograph.run/label-value" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve cleanup of pre-migration NFD objects.
After an upgrade, existing NodeFeature and NodeFeatureGroup objects can still carry topograph.nvidia.com/... ownership labels. cleanupResource at Lines 347-350 selects only the new topograph.run/... labels. An old object that is no longer desired is therefore not listed or deleted, so stale NFD topology can remain active.
Support both label domains during the migration window, or add an explicit migration cleanup for the old domain. Add a regression test for a stale object with the old ownership labels.
As per path instructions, pkg Go changes must preserve public contracts and flag behavior changes that lack meaningful regression coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/engines/nfd/objects.go` around lines 50 - 59, Update cleanupResource to
discover and delete stale NodeFeature and NodeFeatureGroup objects using both
the current topograph.run ownership labels and legacy topograph.nvidia.com
ownership labels during migration. Preserve existing cleanup behavior for new
labels, and add regression coverage for a stale object carrying only the legacy
ownership labels.
Source: Path instructions
1 participant
Replace Topograph-owned Kubernetes label and annotation domains with
topograph.run across engines, tests, demos, and documentation.