Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughInfiniBand providers now support configurable accelerator discovery through NVIDIA SMI, Kubernetes labels, or no discovery. NVIDIA SMI output is normalized into accelerator assignments used to build topology domains. The node data broker now receives provider configuration through a mounted YAML file. ChangesInfiniBand accelerator discovery
Estimated code review effort: 4 (Complex) | ~45 minutes 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 SummaryThe PR separates InfiniBand fabric discovery from accelerator-domain discovery and introduces configurable NVIDIA SMI, Kubernetes-label, and disabled accelerator sources.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the scope of this follow-up review. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Config[provider.params.accelerator] --> Source{Accelerator source}
Source -->|nvidia-smi| SMI[NVIDIA SMI discovery]
Source -->|kubernetes-label| Label[Kubernetes Node label]
Source -->|none or omitted| Disabled[No accelerator assignments]
SMI --> Assignments[Accelerator assignments]
Label --> Assignments
Disabled --> Assignments
IB[ibnetdiscover fabric topology] --> Provider[InfiniBand provider]
Assignments --> Provider
Provider --> Graph[Canonical topology graph]
Reviews (7): Last reviewed commit: "fix(infiniband): make accelerator discov..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/providers/infiniband/common.go`:
- Around line 30-40: Treat rows whose parsed ClusterUUID or CliqueId is the
NVIDIA-SMI unavailable sentinel “N/A” as explicitly unavailable in
parseNVLPartitionID, and update bm.go’s setID flow and k8s.go’s partitions
collection to skip those results before creating domains or annotations. In
pkg/providers/infiniband/bm_test.go at lines 18-57, add unavailable-only and
valid-plus-unavailable regression cases; in pkg/providers/infiniband/k8s_test.go
at lines 86-111, add an unavailable-only case ensuring N/A.N/A is never emitted,
and include malformed-input coverage as required.
🪄 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: 2cb97d13-16af-4b41-af83-b4caf0b9c939
📒 Files selected for processing (8)
CHANGELOG.mddocs/providers/infiniband.mdpkg/providers/infiniband/bm.gopkg/providers/infiniband/bm_test.gopkg/providers/infiniband/common.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/k8s_test.gopkg/providers/infiniband/provider_bm.go
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: govulncheck
- GitHub Check: check
- GitHub Check: k8s / gcp-sim
- GitHub Check: k8s / aws-sim
- GitHub Check: oci-sim / slinky
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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:
pkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/common.gopkg/providers/infiniband/k8s_test.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/bm_test.gopkg/providers/infiniband/bm.go
pkg/providers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/providers/**/*.go: Providers must return a*topology.Graphand*httperr.Error; plainerroris not acceptable at the provider interface boundary.
Providers discover topology and return the canonicaltopology.Graph; they must not emit scheduler-specific output.
When usingClusterTopology, populate fabric tiers closest-first, preserve optional accelerator domain and sub-domain fields, and callToGraph.
Files:
pkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/common.gopkg/providers/infiniband/k8s_test.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/bm_test.gopkg/providers/infiniband/bm.go
**/*
📄 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:
pkg/providers/infiniband/provider_bm.goCHANGELOG.mddocs/providers/infiniband.mdpkg/providers/infiniband/common.gopkg/providers/infiniband/k8s_test.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/bm_test.gopkg/providers/infiniband/bm.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:
pkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/common.gopkg/providers/infiniband/k8s_test.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/bm_test.gopkg/providers/infiniband/bm.go
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Record applicable user-facing features, fixes, breaking changes, or Helm migrations under
[Unreleased].
Files:
CHANGELOG.md
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/infiniband.md
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
New or changed public behavior and new code paths should be covered by tests.
Files:
pkg/providers/infiniband/k8s_test.gopkg/providers/infiniband/bm_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:
pkg/providers/infiniband/k8s_test.gopkg/providers/infiniband/bm_test.go
🔇 Additional comments (6)
pkg/providers/infiniband/common.go (1)
21-21: LGTM!pkg/providers/infiniband/k8s.go (1)
13-13: LGTM!Also applies to: 73-76
docs/providers/infiniband.md (1)
49-49: LGTM!CHANGELOG.md (1)
29-29: LGTM!pkg/providers/infiniband/bm.go (1)
22-32: LGTM!Also applies to: 85-90
pkg/providers/infiniband/provider_bm.go (1)
40-40: LGTM!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #457 +/- ##
==========================================
+ Coverage 72.15% 76.90% +4.74%
==========================================
Files 89 99 +10
Lines 5689 7149 +1460
==========================================
+ Hits 4105 5498 +1393
+ Misses 1382 1365 -17
- Partials 202 286 +84 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🌿 Preview your docs: https://nvidia-preview-pull-request-457.docs.buildwithfern.com/topograph |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 @.claude/CLAUDE.md:
- Around line 27-30: Add the text language identifier to the opening
repository-map code fence in .claude/CLAUDE.md at lines 27-30 and apply the same
change to the corresponding fence in AGENTS.md at lines 27-30; leave the
repository-map contents unchanged.
In `@charts/topograph/templates/_validation.tpl`:
- Around line 23-43: Normalize the persisted provider configuration’s
accelerator source to lowercase, not only the local $source validation value.
Update the provider serialization used by nodeObserver/configmap.yml so
accelerator.source values such as NVIDIA-SMI are emitted as lowercase constants
accepted by accelerator.Config.Validate(), while preserving the existing
validation behavior.
In `@charts/topograph/tests/node-data-broker_rbac_test.yaml`:
- Around line 116-147: Expand both negative RBAC test cases for the absent
accelerator section and the kubernetes-label accelerator source to assert that
rules also exclude daemonsets:get and pods:list, in addition to
pods/exec:create. Apply the same complete absence checks to the additional case
referenced after this diff, preserving the existing test structure and
least-privilege expectations.
In `@docs/providers/infiniband.md`:
- Around line 121-156: Update docs/providers/infiniband.md lines 121-156 to
explicitly describe the breaking migration from
provider.params.useGpuCliqueLabel to accelerator.source: kubernetes-label with
accelerator.kubernetesLabel.key, including that omitted accelerator
configuration defaults to none. Update CHANGELOG.md line 11 to mark this change
as breaking and include the same migration mapping.
- Around line 121-132: Update the accelerator documentation to distinguish
topology request parameters from Helm deployment settings: move or relabel
accelerator.nvidiaSmi.gpuOperatorNamespace and devicePluginDaemonSet as
Helm-only broker configuration, state that accelerator.source in requests must
match the broker’s rendered source, and note that changing broker settings
requires restarting the node-data-broker.
In `@pkg/accelerator/accelerator_test.go`:
- Around line 81-105: Extend TestNvidiaSMIDiscoverer with a canceled-context
case using a command runner that inspects the context passed to Run and asserts
ctx.Err() is context.Canceled. Cancel the context before calling Discover, then
verify the runner observes that cancellation and the discovery path returns the
expected error. Keep the existing success and command-error cases unchanged.
In `@pkg/providers/infiniband/provider_bm.go`:
- Around line 42-57: Preserve or explicitly migrate the prior default
accelerator discovery behavior: update accelerator configuration handling in
pkg/providers/infiniband/provider_bm.go (lines 42-57) and
pkg/providers/infiniband/provider_k8s.go (lines 35-39 and 65-72) so absent
accelerator settings do not silently become SourceNone, or reject legacy
useGpuCliqueLabel with a clear migration error; translate it to the
corresponding accelerator.source if supported. Add regression coverage in
pkg/providers/infiniband/provider_bm_test.go (line 24) and
pkg/providers/infiniband/provider_k8s_test.go (line 30) for legacy configuration
and the selected compatibility policy.
🪄 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: dd77cd4b-292f-4fbc-b6a2-4efffb74de84
⛔ Files ignored due to path filters (1)
charts/topograph/tests/__snapshot__/render_snapshot_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (32)
.claude/CLAUDE.mdAGENTS.mdCHANGELOG.mdcharts/topograph/templates/NOTES.txtcharts/topograph/templates/_validation.tplcharts/topograph/templates/nodeDataBroker/_helpers.tplcharts/topograph/templates/nodeDataBroker/daemonset.yamlcharts/topograph/templates/nodeDataBroker/rbac.yamlcharts/topograph/tests/node-data-broker_rbac_test.yamlcharts/topograph/tests/node-data-broker_test.yamlcharts/topograph/tests/validation_test.yamlcharts/topograph/values.k8s.ib-example.yamlcharts/topograph/values.slinky.ib.block-example.yamlcharts/topograph/values.yamldocs/api.mddocs/architecture.mddocs/engines/k8s.mddocs/providers/infiniband.mddocs/reference/node-labels.mdpkg/accelerator/accelerator.gopkg/accelerator/accelerator_test.gopkg/accelerator/nvidia_smi.gopkg/engines/slinky/engine.gopkg/providers/infiniband/bm.gopkg/providers/infiniband/bm_test.gopkg/providers/infiniband/common.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/k8s_test.gopkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/provider_k8s.gopkg/providers/infiniband/provider_k8s_test.go
💤 Files with no reviewable changes (2)
- pkg/engines/slinky/engine.go
- pkg/providers/infiniband/common.go
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Greptile Review
- GitHub Check: k8s / gcp-sim
- GitHub Check: k8s / aws-sim
- GitHub Check: k8s / test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: build
- GitHub Check: oci-sim / slinky
🧰 Additional context used
📓 Path-based instructions (11)
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/templates/NOTES.txtcharts/topograph/tests/validation_test.yamlcharts/topograph/values.yamlcharts/topograph/values.slinky.ib.block-example.yamlcharts/topograph/tests/node-data-broker_test.yamlcharts/topograph/templates/nodeDataBroker/daemonset.yamlcharts/topograph/templates/nodeDataBroker/_helpers.tplcharts/topograph/templates/_validation.tplcharts/topograph/values.k8s.ib-example.yamlcharts/topograph/tests/node-data-broker_rbac_test.yamlcharts/topograph/templates/nodeDataBroker/rbac.yaml
charts/topograph/tests/**/*.{yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Run Helm lint and helm-unittest suites when changing the Helm chart; review snapshot updates before committing.
Files:
charts/topograph/tests/validation_test.yamlcharts/topograph/tests/node-data-broker_test.yamlcharts/topograph/tests/node-data-broker_rbac_test.yaml
charts/topograph/**/*.{yaml,yml,tpl}
📄 CodeRabbit inference engine (AGENTS.md)
Changes to chart templates or values schemas require corresponding documentation updates, including values comments,
NOTES.txt, and affected engine documentation.
Files:
charts/topograph/tests/validation_test.yamlcharts/topograph/values.yamlcharts/topograph/values.slinky.ib.block-example.yamlcharts/topograph/tests/node-data-broker_test.yamlcharts/topograph/templates/nodeDataBroker/daemonset.yamlcharts/topograph/templates/nodeDataBroker/_helpers.tplcharts/topograph/templates/_validation.tplcharts/topograph/values.k8s.ib-example.yamlcharts/topograph/tests/node-data-broker_rbac_test.yamlcharts/topograph/templates/nodeDataBroker/rbac.yaml
docs/engines/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Adding, changing, or removing an engine requires updating the corresponding engine documentation.
Files:
docs/engines/k8s.md
charts/topograph/templates/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Do not enable both
ingress.enabledandgatewayAPI.enabledin the same Helm release; the routing resources are mutually exclusive.
Files:
charts/topograph/templates/nodeDataBroker/daemonset.yamlcharts/topograph/templates/nodeDataBroker/rbac.yaml
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Record applicable user-facing features, fixes, breaking changes, or Helm migrations under
[Unreleased].
Files:
CHANGELOG.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Rungo fmt ./...; Go formatting is authoritative and code must not be hand-formatted.
Every new Go file must include the NVIDIA copyright header followed by the matching Apache 2.0 boilerplate.
Files:
pkg/accelerator/accelerator.gopkg/providers/infiniband/bm.gopkg/providers/infiniband/provider_bm_test.gopkg/accelerator/nvidia_smi.gopkg/providers/infiniband/k8s_test.gopkg/accelerator/accelerator_test.gopkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/provider_k8s.gopkg/providers/infiniband/bm_test.gopkg/providers/infiniband/provider_k8s_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:
pkg/accelerator/accelerator.gopkg/providers/infiniband/bm.gopkg/providers/infiniband/provider_bm_test.gopkg/accelerator/nvidia_smi.gopkg/providers/infiniband/k8s_test.gopkg/accelerator/accelerator_test.gopkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/provider_k8s.gopkg/providers/infiniband/bm_test.gopkg/providers/infiniband/provider_k8s_test.go
pkg/providers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/providers/**/*.go: Providers must discover topology and return a canonical*topology.Graph; they must not emit scheduler-specific output.
At the provider interface boundary, return*httperr.Error; plainerroris not acceptable because the API server must propagate the correct HTTP status.
Providers usingClusterTopologymust populate fabric tiers closest-first, set optional accelerator domain and sub-domain IDs, and callToGraph.
Files:
pkg/providers/infiniband/bm.gopkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/k8s_test.gopkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/provider_k8s.gopkg/providers/infiniband/bm_test.gopkg/providers/infiniband/provider_k8s_test.go
**/*_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:
pkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/k8s_test.gopkg/accelerator/accelerator_test.gopkg/providers/infiniband/bm_test.gopkg/providers/infiniband/provider_k8s_test.go
docs/providers/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Adding, changing, or removing a provider requires updating its provider documentation and the provider list and scenario table in
docs/overview.md.
Files:
docs/providers/infiniband.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T16:04:26.698Z
Learning: Every commit must carry a `Signed-off-by:` DCO trailer.
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T16:04:26.698Z
Learning: Potential security vulnerabilities must be reported privately through NVIDIA PSIRT rather than public issues or PRs.
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T16:04:26.698Z
Learning: Breaking changes to the config schema, label keys, or `Vertex` shape require prior discussion in an issue.
🪛 LanguageTool
docs/api.md
[grammar] ~79-~79: Ensure spelling is correct
Context: ...d by Kubernetes collection. Defaults to nvidia-device-plugin-daemonset. - engine: (optional) Selects the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.23.2)
.claude/CLAUDE.md
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
AGENTS.md
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (25)
pkg/providers/infiniband/bm.go (1)
19-83: LGTM!pkg/providers/infiniband/bm_test.go (1)
18-57: LGTM!pkg/providers/infiniband/k8s.go (1)
70-127: LGTM!pkg/providers/infiniband/k8s_test.go (1)
74-125: LGTM!pkg/accelerator/nvidia_smi.go (1)
53-57: 🗄️ Data Integrity & IntegrationKeep the full NVL partition identifier in
DomainID.InfiniBand NVIDIA SMI discovery defines the accelerator domain as
ClusterUUID.CliqueId. The parser and current tests correctly preserve that value. Splitting it intoDomainIDandSubDomainIDwould change the documented contract.> Likely an incorrect or invalid review comment.charts/topograph/templates/NOTES.txt (1)
45-49: LGTM!charts/topograph/templates/nodeDataBroker/_helpers.tpl (1)
56-69: LGTM!charts/topograph/templates/nodeDataBroker/daemonset.yaml (2)
3-5: LGTM!
46-55: 🗄️ Data Integrity & IntegrationNo change needed. The node-data-broker passes
--setvalues toinfiniband.GetNodeAnnotations, which consumes all three exact keys and validates the selected source.docs/engines/k8s.md (1)
80-80: LGTM!docs/providers/infiniband.md (1)
20-20: LGTM!Also applies to: 44-54, 65-76, 94-101
docs/reference/node-labels.md (1)
43-44: LGTM!Also applies to: 54-54
.claude/CLAUDE.md (1)
21-26: LGTM!AGENTS.md (1)
21-26: LGTM!CHANGELOG.md (2)
11-11: 📐 Maintainability & Code QualityVerify the DCO trailer before merge.
Confirm that every commit in this PR contains a
Signed-off-by:trailer.Based on learnings: “Every commit must carry a
Signed-off-by:DCO trailer.”Source: Learnings
30-30: LGTM!charts/topograph/templates/nodeDataBroker/rbac.yaml (1)
2-21: LGTM!charts/topograph/tests/node-data-broker_rbac_test.yaml (1)
74-115: LGTM!charts/topograph/tests/node-data-broker_test.yaml (1)
107-109: LGTM!Also applies to: 146-173
charts/topograph/tests/validation_test.yaml (1)
80-99: LGTM!charts/topograph/values.k8s.ib-example.yaml (1)
4-7: LGTM!charts/topograph/values.slinky.ib.block-example.yaml (1)
15-20: LGTM!Also applies to: 43-43
charts/topograph/values.yaml (1)
9-19: LGTM!docs/api.md (1)
75-79: LGTM!docs/architecture.md (1)
29-29: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@charts/topograph/tests/validation_test.yaml`:
- Around line 80-112: Extend the accelerator validation tests with an
infiniband-bm provider using source kubernetes-label, asserting the
corresponding rejection message. Add malformed-input cases where accelerator is
null and where it is a non-map value, asserting each expected template failure;
keep the existing infiniband-k8s validation cases unchanged.
In `@docs/api.md`:
- Around line 75-79: Update the accelerator documentation to remove
nvidiaSmi.gpuOperatorNamespace and nvidiaSmi.devicePluginDaemonSet from
topology-request parameters, document them instead as Helm deployment settings
for infiniband-k8s, and state that the request source must match the
node-data-broker source.
In `@pkg/providers/infiniband/k8s.go`:
- Around line 70-75: Add regression coverage for GetGpuClusterID’s Kubernetes
execution path: verify k8s.ExecInPod receives the expected pod, namespace, and
strings.Fields(accelerator.NvidiaSMICommand) arguments, validate parsing of
duplicate-row output, and add a case asserting invalid CSV output returns an
error. Keep TestParseNvidiaSMIOutput focused on parsing and restore a direct
TestGetGPUClusterID-style test for this path.
In `@pkg/providers/infiniband/provider_bm_test.go`:
- Around line 42-70: Extend the LoaderBM test loop to inspect
ProviderBM.accelerator for successful cases: assert the nvidia-smi case uses a
non-none accelerator discoverer, and assert the none case uses
accelerator.NewNoneDiscoverer(). Keep the existing error assertions and provider
type checks unchanged.
In `@pkg/providers/infiniband/provider_bm.go`:
- Around line 78-83: Add a focused test for ProviderBM.GenerateTopologyConfig
that injects a failing accelerator.Discoverer, without invoking IB discovery,
and verifies the returned error has HTTP status 500 and includes the “failed to
discover accelerator domains: ...” message.
🪄 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: 5c97ca2f-7b28-435c-84f5-82a4af1afaca
📒 Files selected for processing (17)
CHANGELOG.mdcharts/topograph/templates/_validation.tplcharts/topograph/templates/nodeDataBroker/_helpers.tplcharts/topograph/tests/node-data-broker_rbac_test.yamlcharts/topograph/tests/node-data-broker_test.yamlcharts/topograph/tests/validation_test.yamlcharts/topograph/values.yamldocs/api.mddocs/providers/infiniband.mdpkg/accelerator/accelerator.gopkg/accelerator/accelerator_test.gopkg/providers/infiniband/common.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/provider_k8s.gopkg/providers/infiniband/provider_k8s_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Greptile Review
- GitHub Check: k8s / test
- GitHub Check: k8s / gcp-sim
- GitHub Check: k8s / aws-sim
- GitHub Check: check
- GitHub Check: govulncheck
- GitHub Check: test
- GitHub Check: oci-sim / slinky
🧰 Additional context used
📓 Path-based instructions (10)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Usego fmt ./...as the authoritative formatting standard for Go files; fix applicablegolangci-lintwarnings in code being touched.
Add the required NVIDIA copyright header and Apache 2.0 boilerplate to every new Go file.
Files:
pkg/providers/infiniband/k8s.gopkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/common.gopkg/providers/infiniband/provider_k8s_test.gopkg/accelerator/accelerator.gopkg/providers/infiniband/provider_k8s.gopkg/accelerator/accelerator_test.gopkg/providers/infiniband/provider_bm.go
pkg/providers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/providers/**/*.go: Providers must implementGenerateTopologyConfigwith the defined signature and return*httperr.Error, not plainerror, at the API boundary.
Providers discover topology and return the canonical*topology.Graph; they must not emit scheduler-specific output.
Keep network-fabric discovery and accelerator-domain discovery independently composable throughpkg/accelerator, while the provider combines both into the canonical graph.
Do not read the fabric inside an engine or emit scheduler-specific output from a provider.
Files:
pkg/providers/infiniband/k8s.gopkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/common.gopkg/providers/infiniband/provider_k8s_test.gopkg/providers/infiniband/provider_k8s.gopkg/providers/infiniband/provider_bm.go
**/*.{go,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
New or changed public behavior must have test coverage; run the relevant Go or Helm tests.
Files:
pkg/providers/infiniband/k8s.gocharts/topograph/tests/node-data-broker_test.yamlpkg/providers/infiniband/provider_bm_test.gocharts/topograph/tests/validation_test.yamlpkg/providers/infiniband/common.gocharts/topograph/tests/node-data-broker_rbac_test.yamlcharts/topograph/values.yamlpkg/providers/infiniband/provider_k8s_test.gopkg/accelerator/accelerator.gopkg/providers/infiniband/provider_k8s.gopkg/accelerator/accelerator_test.gopkg/providers/infiniband/provider_bm.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:
pkg/providers/infiniband/k8s.gopkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/common.gopkg/providers/infiniband/provider_k8s_test.gopkg/accelerator/accelerator.gopkg/providers/infiniband/provider_k8s.gopkg/accelerator/accelerator_test.gopkg/providers/infiniband/provider_bm.go
charts/topograph/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Do not enable both
ingress.enabledandgatewayAPI.enabledin the same Helm release.
Files:
charts/topograph/tests/node-data-broker_test.yamlcharts/topograph/tests/validation_test.yamlcharts/topograph/tests/node-data-broker_rbac_test.yamlcharts/topograph/values.yaml
charts/topograph/**/*
📄 CodeRabbit inference engine (AGENTS.md)
When changing chart templates or values, update the corresponding chart documentation and tests/snapshots as applicable; run
make chart-test.
Files:
charts/topograph/tests/node-data-broker_test.yamlcharts/topograph/tests/validation_test.yamlcharts/topograph/tests/node-data-broker_rbac_test.yamlcharts/topograph/templates/_validation.tplcharts/topograph/values.yamlcharts/topograph/templates/nodeDataBroker/_helpers.tpl
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/tests/node-data-broker_test.yamlcharts/topograph/tests/validation_test.yamlcharts/topograph/tests/node-data-broker_rbac_test.yamlcharts/topograph/templates/_validation.tplcharts/topograph/values.yamlcharts/topograph/templates/nodeDataBroker/_helpers.tpl
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Record applicable user-facing features, fixes, breaking changes, or Helm migrations under
[Unreleased].
Files:
CHANGELOG.md
**/*_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:
pkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/provider_k8s_test.gopkg/accelerator/accelerator_test.go
docs/providers/*.md
📄 CodeRabbit inference engine (AGENTS.md)
A new provider requires provider documentation covering prerequisites, credentials, parameters, operation, and verification, plus updates to the overview provider list and scenario table.
Files:
docs/providers/infiniband.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T17:21:12.221Z
Learning: Every commit must include a `Signed-off-by:` DCO trailer.
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T17:21:12.221Z
Learning: Before pushing, run `make qualify`; all CI checks must be green before merge.
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T17:21:12.221Z
Learning: Report suspected security vulnerabilities privately through the NVIDIA PSIRT process rather than public issues or PRs.
🪛 LanguageTool
docs/api.md
[grammar] ~79-~79: Ensure spelling is correct
Context: ...d by Kubernetes collection. Defaults to nvidia-device-plugin-daemonset. - engine: (optional) Selects the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (15)
docs/providers/infiniband.md (1)
121-130: Separate request parameters from Helm deployment settings.The node-data-broker settings still appear as request parameters. This repeats the existing review finding.
CHANGELOG.md (2)
11-11: Document the breaking Kubernetes migration.The changelog still omits the migration from
useGpuCliqueLabeltoaccelerator.source: kubernetes-labelwithaccelerator.kubernetesLabel.key. This repeats the existing review finding.
30-30: LGTM!pkg/accelerator/accelerator.go (1)
18-153: LGTM!pkg/accelerator/accelerator_test.go (1)
20-163: LGTM!pkg/providers/infiniband/k8s.go (1)
20-27: LGTM!Also applies to: 84-127
pkg/providers/infiniband/provider_k8s_test.go (1)
17-130: LGTM!pkg/providers/infiniband/provider_bm.go (1)
13-77: LGTM!pkg/providers/infiniband/common.go (1)
12-12: LGTM!Also applies to: 25-33
pkg/providers/infiniband/provider_k8s.go (1)
21-39: LGTM!Also applies to: 65-79, 87-98, 119-132
charts/topograph/values.yaml (1)
9-21: LGTM!charts/topograph/templates/nodeDataBroker/_helpers.tpl (1)
56-73: LGTM!charts/topograph/templates/_validation.tpl (1)
23-56: LGTM!charts/topograph/tests/node-data-broker_rbac_test.yaml (1)
74-82: LGTM!Also applies to: 116-226
charts/topograph/tests/node-data-broker_test.yaml (1)
107-109: LGTM!Also applies to: 146-187
dmitsh
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@charts/topograph/values.yaml`:
- Around line 9-21: Remove the obsolete useGpuCliqueLabel example and any
related engine parameter documentation from the chart values comments. Ensure
the comments instead reference only the supported accelerator configuration,
including the accelerator source and kubernetesLabel settings shown in the diff.
In `@cmd/node-data-broker/main_test.go`:
- Around line 68-104: Extend TestNewNodeDataBrokerConfig with YAML cases where
accelerator is omitted and where accelerator is an empty map; load each
configuration through newNodeDataBrokerConfig, construct the broker, and call
broker.getAnnotations with a node name. Assert both calls succeed and return no
accelerator annotations, covering disabled discovery through YAML decoding and
provider-parameter forwarding.
In `@pkg/accelerator/accelerator_test.go`:
- Around line 252-277: Add negative constructor tests to
TestKubernetesNodeDiscovererWithoutCollection for the nvidia-smi path with a
non-nil Kubernetes client and nil restConfig, asserting “Kubernetes REST config
is required for nvidia-smi discovery”; also add a NewNvidiaSMIDiscoverer test
with a nil command runner, asserting “nvidia-smi command runner is required.”
🪄 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: 48722e73-6553-4cea-aaaf-68301be61a03
⛔ Files ignored due to path filters (1)
charts/topograph/tests/__snapshot__/render_snapshot_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
charts/topograph/templates/nodeDataBroker/configmap.yamlcharts/topograph/templates/nodeDataBroker/daemonset.yamlcharts/topograph/tests/node-data-broker_configmap_test.yamlcharts/topograph/tests/node-data-broker_test.yamlcharts/topograph/values.schema.jsoncharts/topograph/values.yamlcmd/node-data-broker/main.gocmd/node-data-broker/main_test.godocs/providers/infiniband.mdpkg/accelerator/accelerator.gopkg/accelerator/accelerator_test.gopkg/accelerator/kubernetes.gopkg/accelerator/nvidia_smi.gopkg/providers/infiniband/common.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/k8s_test.gopkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/provider_k8s.gopkg/providers/infiniband/provider_k8s_test.go
💤 Files with no reviewable changes (2)
- charts/topograph/values.schema.json
- pkg/providers/infiniband/common.go
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Greptile Review
- GitHub Check: check
- GitHub Check: test
- GitHub Check: build
- GitHub Check: k8s / gcp-sim
- GitHub Check: k8s / test
- GitHub Check: k8s / aws-sim
- GitHub Check: oci-sim / slinky
🧰 Additional context used
📓 Path-based instructions (9)
charts/topograph/templates/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Do not enable both
ingress.enabledandgatewayAPI.enabledin one Helm release.
Files:
charts/topograph/templates/nodeDataBroker/configmap.yamlcharts/topograph/templates/nodeDataBroker/daemonset.yaml
charts/topograph/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Helm chart changes must be validated with Helm lint and helm-unittest tests; review snapshot updates before committing them.
Files:
charts/topograph/templates/nodeDataBroker/configmap.yamlcharts/topograph/tests/node-data-broker_configmap_test.yamlcharts/topograph/values.yamlcharts/topograph/tests/node-data-broker_test.yamlcharts/topograph/templates/nodeDataBroker/daemonset.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/templates/nodeDataBroker/configmap.yamlcharts/topograph/tests/node-data-broker_configmap_test.yamlcharts/topograph/values.yamlcharts/topograph/tests/node-data-broker_test.yamlcharts/topograph/templates/nodeDataBroker/daemonset.yaml
pkg/providers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/providers/**/*.go: Providers must discover topology and return a canonical*topology.Graph; they must not emit scheduler-specific output. At the interface boundary, return*httperr.Error, not plainerror.
When usingClusterTopology, populate fabric tiers closest-first, optional accelerator domain and sub-domain IDs, and callToGraph; providers must combine fabric and accelerator discovery into the canonical graph.
Files:
pkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/provider_k8s_test.gopkg/providers/infiniband/k8s.gopkg/providers/infiniband/provider_k8s.gopkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/k8s_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Rungo fmt ./...as the authoritative formatter and fix new golangci-lint warnings in touched code.
Every new Go file must contain the NVIDIA copyright header followed by the matching Apache 2.0 boilerplate.
Files:
pkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/provider_k8s_test.gopkg/accelerator/kubernetes.gopkg/providers/infiniband/k8s.gopkg/accelerator/accelerator.gopkg/accelerator/nvidia_smi.gopkg/accelerator/accelerator_test.gocmd/node-data-broker/main.gocmd/node-data-broker/main_test.gopkg/providers/infiniband/provider_k8s.gopkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/k8s_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:
pkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/provider_k8s_test.gopkg/accelerator/kubernetes.gopkg/providers/infiniband/k8s.gopkg/accelerator/accelerator.gopkg/accelerator/nvidia_smi.gopkg/accelerator/accelerator_test.gocmd/node-data-broker/main.gocmd/node-data-broker/main_test.gopkg/providers/infiniband/provider_k8s.gopkg/providers/infiniband/provider_bm.gopkg/providers/infiniband/k8s_test.go
**/*_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:
pkg/providers/infiniband/provider_bm_test.gopkg/providers/infiniband/provider_k8s_test.gopkg/accelerator/accelerator_test.gocmd/node-data-broker/main_test.gopkg/providers/infiniband/k8s_test.go
charts/topograph/values.yaml
📄 CodeRabbit inference engine (AGENTS.md)
When changing the chart values schema, update comments,
NOTES.txt, and documentation referencing those values.
Files:
charts/topograph/values.yaml
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Provider, engine, API, configuration, label, and user-facing behavior changes must update the corresponding documentation; user-facing changes should also update
CHANGELOG.mdunder[Unreleased]when applicable.
Files:
docs/providers/infiniband.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T18:30:53.488Z
Learning: Every commit must carry a `Signed-off-by:` trailer; GPG signing is optional.
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T18:30:53.488Z
Learning: Potential security vulnerabilities must be reported privately through NVIDIA PSIRT channels, not public issues or PRs.
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T18:30:53.488Z
Learning: Run `make qualify` before pushing, and ensure new or changed public behavior has test coverage.
🪛 YAMLlint (1.37.1)
charts/topograph/templates/nodeDataBroker/configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (20)
docs/providers/infiniband.md (1)
121-156: Separate topology-request parameters from broker deployment settings.
accelerator.nvidiaSmi.gpuOperatorNamespaceanddevicePluginDaemonSetcannot reconfigure an existing node-data-broker Pod from a topology request. Document them as Helm-only broker settings. State that request-sideaccelerator.source: nvidia-smirequires matching broker configuration and that broker configuration changes require a restart.charts/topograph/templates/nodeDataBroker/configmap.yaml (1)
1-14: LGTM!charts/topograph/templates/nodeDataBroker/daemonset.yaml (1)
16-17: LGTM!Also applies to: 41-42, 83-85, 98-101
charts/topograph/tests/node-data-broker_configmap_test.yaml (1)
8-48: LGTM!charts/topograph/tests/node-data-broker_test.yaml (1)
80-125: LGTM!cmd/node-data-broker/main.go (1)
26-38: 📐 Maintainability & Code QualityRun the required Go validation.
Run
go fmt ./...and fix newgolangci-lintwarnings in touched code before merge. Runmake qualifybefore pushing. As per coding guidelines, “Rungo fmt ./...as the authoritative formatter and fix new golangci-lint warnings in touched code.” Based on learnings, “Runmake qualifybefore pushing.”Sources: Coding guidelines, Learnings
pkg/providers/infiniband/k8s.go (2)
56-65: The Kubernetes nvidia-smi execution path still has no regression test.The pod lookup, exec, and output parsing moved to
kubernetesNvidiaSMIRunner.Runinpkg/accelerator/kubernetes.go.pkg/accelerator/accelerator_test.goasserts only the constructed runner fields. No test asserts the pod selection, the exec arguments, or the zero-pod and multi-pod branches. Add a test with the fake clientset for that runner.As per path instructions,
{cmd,pkg,internal}/**/*.gorequires meaningful regression coverage for behavior changes.Source: Path instructions
50-55: LGTM!Also applies to: 66-69
pkg/providers/infiniband/provider_bm.go (2)
46-51: The accelerator discovery failure path still has no test.
pkg/providers/infiniband/provider_bm_test.gocovers onlyLoaderBM. No test injects a failingaccelerator.DiscovererintoProviderBMand asserts the 500 status and thefailed to discover accelerator domains: ...message.As per path instructions,
{cmd,pkg,internal}/**/*.gorequires meaningful regression coverage for behavior changes.Source: Path instructions
21-39: LGTM!pkg/providers/infiniband/provider_bm_test.go (1)
65-75: Assert the selected discoverer for the success cases.The
nvidia-smicase and thenonecase produce identical assertions. The test passes ifLoaderBMalways installs the none discoverer. Add an expected-discoverer field to the table and assertProviderBM.acceleratorfor each success case.As per path instructions,
**/*_test.gorequires assertions that fail if the behavior under test is broken.Source: Path instructions
pkg/accelerator/accelerator.go (2)
76-102: LGTM!Also applies to: 104-127
1-4: 📐 Maintainability & Code QualityUse the repository-standard SPDX header.
The short NVIDIA copyright and
SPDX-License-Identifier: Apache-2.0header matches all files inpkg/accelerator. No change is required.> Likely an incorrect or invalid review comment.pkg/accelerator/nvidia_smi.go (2)
58-79: LGTM!Also applies to: 113-136
81-94: 🎯 Functional CorrectnessNo change required
The module and CI declare Go 1.26.5, which supports
strings.Lines.> Likely an incorrect or invalid review comment.pkg/accelerator/kubernetes.go (1)
23-48: LGTM!Also applies to: 57-87
pkg/accelerator/accelerator_test.go (1)
299-328: LGTM!Also applies to: 330-358
pkg/providers/infiniband/k8s_test.go (1)
18-41: LGTM!pkg/providers/infiniband/provider_k8s.go (1)
28-33: LGTM!Also applies to: 52-57, 70-74, 102-115
pkg/providers/infiniband/provider_k8s_test.go (1)
15-50: LGTM!
1cf76f5 to
f555835
Compare
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 `@charts/topograph/tests/node-data-broker_configmap_test.yaml`:
- Around line 42-55: Extend the nodeDataBroker.providerConfig render tests with
cases for an omitted accelerator section, an empty accelerator object, and
source none. Assert each rendered node-data-broker-config.yaml excludes
gpuOperatorNamespace and devicePluginDaemonSet, while preserving the existing
nvidia-smi default assertions.
In `@cmd/node-data-broker/main_test.go`:
- Around line 138-141: Add a separate negative healthzPort fixture and assertion
in the newNodeDataBrokerConfig test, using a negative value and verifying it
returns the same “must specify a positive healthzPort” error; retain the
existing omitted-port case to cover zero.
🪄 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: fe24a0a7-3018-4b80-9c90-a6536f5ba142
📒 Files selected for processing (8)
CHANGELOG.mdcharts/topograph/templates/nodeDataBroker/_helpers.tplcharts/topograph/templates/nodeDataBroker/configmap.yamlcharts/topograph/tests/node-data-broker_configmap_test.yamlcharts/topograph/values.yamlcmd/node-data-broker/main_test.godocs/providers/infiniband.mdpkg/accelerator/kubernetes.go
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: Greptile Review
- GitHub Check: oci-sim / slinky
- GitHub Check: build
- GitHub Check: test
- GitHub Check: k8s / test
- GitHub Check: k8s / aws-sim
- GitHub Check: k8s / gcp-sim
🧰 Additional context used
📓 Path-based instructions (8)
charts/topograph/templates/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Do not enable both
ingress.enabledandgatewayAPI.enabledin the same Helm release.
Files:
charts/topograph/templates/nodeDataBroker/configmap.yaml
**/*.{go,yaml,yml,md}
📄 CodeRabbit inference engine (AGENTS.md)
When changing a documented contract or repository surface, update the corresponding documentation in the same change, including provider, engine, chart, API, label, configuration, and changelog documentation as applicable.
Files:
charts/topograph/templates/nodeDataBroker/configmap.yamlcharts/topograph/values.yamlcharts/topograph/tests/node-data-broker_configmap_test.yamlCHANGELOG.mddocs/providers/infiniband.mdcmd/node-data-broker/main_test.gopkg/accelerator/kubernetes.go
charts/topograph/**/*.{yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Run Helm chart tests when changing the Topograph chart.
Files:
charts/topograph/templates/nodeDataBroker/configmap.yamlcharts/topograph/values.yamlcharts/topograph/tests/node-data-broker_configmap_test.yaml
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: All CI checks must be green before merge, including Go build/test/lint, Codecov, and DCO checks.
Every commit must include aSigned-off-by:DCO trailer.
Use Conventional Commits with an allowed type and include a scope and short description.
Use branch prefixes matching the change type:feat/,fix/,docs/,chore/,refactor/, ortest/.
Report suspected security vulnerabilities privately through NVIDIA PSIRT rather than public issues or PRs.
Discuss changes topkg/topology/, configuration schema, label keys, orVertexshape before implementation.
Do not modify an AGENTS.md-described surface without updating bothAGENTS.mdand.claude/CLAUDE.mdin the same change.
Run and review coverage checks: project target 60%, patch target 50%, with no unacceptable coverage drop.
Evaluate documentation impact for every pull request and update applicable documentation for contract or user-facing changes.
Do not skip DCO sign-off or defer it until later.
Files:
charts/topograph/templates/nodeDataBroker/configmap.yamlcharts/topograph/values.yamlcharts/topograph/tests/node-data-broker_configmap_test.yamlCHANGELOG.mddocs/providers/infiniband.mdcmd/node-data-broker/main_test.gopkg/accelerator/kubernetes.gocharts/topograph/templates/nodeDataBroker/_helpers.tpl
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/templates/nodeDataBroker/configmap.yamlcharts/topograph/values.yamlcharts/topograph/tests/node-data-broker_configmap_test.yamlcharts/topograph/templates/nodeDataBroker/_helpers.tpl
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Rungo fmt ./...; do not hand-format Go code.
Add the specified NVIDIA Apache 2.0 copyright header to every new Go file.
New or changed public behavior should be covered by tests.
Runmake qualifybefore pushing; it performs formatting, vetting, linting, and tests.
Files:
cmd/node-data-broker/main_test.gopkg/accelerator/kubernetes.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:
cmd/node-data-broker/main_test.gopkg/accelerator/kubernetes.go
**/*_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:
cmd/node-data-broker/main_test.go
🪛 YAMLlint (1.37.1)
charts/topograph/templates/nodeDataBroker/configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (6)
cmd/node-data-broker/main_test.go (1)
84-102: Cover an omitted accelerator section through the broker path.This test covers
accelerator: {}, but it does not cover a provider with noacceleratorkey. Add anodeBroker.getAnnotationscase withinfiniband-k8sand omitted accelerator parameters. Assert that it returns only the base annotations.docs/providers/infiniband.md (1)
121-132: Separate request parameters from broker deployment settings.
accelerator.nvidiaSmi.gpuOperatorNamespaceandaccelerator.nvidiaSmi.devicePluginDaemonSetconfigure the Helm-rendered node-data-broker ConfigMap. A topology request cannot reconfigure a running broker. Move these fields out of the request-parameter table. State that a request usingnvidia-smimust align with the broker-rendered source and configuration.pkg/accelerator/kubernetes.go (1)
20-87: LGTM!charts/topograph/templates/nodeDataBroker/configmap.yaml (1)
1-14: LGTM!charts/topograph/templates/nodeDataBroker/_helpers.tpl (1)
56-98: LGTM!charts/topograph/values.yaml (1)
9-23: LGTM!
Signed-off-by: Dmitry Shmulevich <17212177+dmitsh@users.noreply.github.com>
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 `@pkg/accelerator/accelerator_test.go`:
- Around line 1-4: Update the file header above the package declaration in
accelerator_test.go by adding the repository-standard Apache 2.0 boilerplate
immediately after the NVIDIA copyright line, while preserving the existing SPDX
identifier and copyright text.
- Around line 330-358: Extend TestParseNvidiaSMIOutput with a malformed CSV case
using an extra field such as “uuid, 7, extra”, and assert the parser returns the
expected field-count error. Ensure ParseNvidiaSMIOutput rejects rows unless they
contain exactly the ClusterUUID and CliqueId fields, while preserving existing
handling for valid and under-specified rows.
🪄 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: 1773536f-26a0-4e12-8c24-42b49380d79f
📒 Files selected for processing (5)
charts/topograph/tests/node-data-broker_configmap_test.yamlcmd/node-data-broker/main_test.godocs/api.mddocs/providers/infiniband.mdpkg/accelerator/accelerator_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Greptile Review
- GitHub Check: oci-sim / slinky
- GitHub Check: k8s / gcp-sim
- GitHub Check: check
- GitHub Check: build
- GitHub Check: test
- GitHub Check: k8s / aws-sim
- GitHub Check: k8s / test
🧰 Additional context used
📓 Path-based instructions (7)
charts/topograph/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Do not enable both
ingress.enabledandgatewayAPI.enabledin the same Helm release.
Files:
charts/topograph/tests/node-data-broker_configmap_test.yaml
charts/topograph/**/*.{yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
charts/topograph/**/*.{yaml,yml}: Runmake chart-testwhen changingcharts/topograph/; Helm lint and helm-unittest tests must pass.
Chart changes must include appropriate updates to chart tests or snapshots when behavior changes.
Files:
charts/topograph/tests/node-data-broker_configmap_test.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/tests/node-data-broker_configmap_test.yaml
docs/providers/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Adding, changing, or removing a provider requires updating its provider documentation and the provider list and scenario table in
docs/overview.md.
Files:
docs/providers/infiniband.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.
Changes to Go code should passgo vet,golangci-lint, and race-enabled tests.
Files:
cmd/node-data-broker/main_test.gopkg/accelerator/accelerator_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:
cmd/node-data-broker/main_test.gopkg/accelerator/accelerator_test.go
**/*_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:
cmd/node-data-broker/main_test.gopkg/accelerator/accelerator_test.go
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T19:02:41.756Z
Learning: Every commit must include a `Signed-off-by:` DCO trailer.
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T19:02:41.756Z
Learning: Potential security vulnerabilities must be reported privately through NVIDIA PSIRT channels, not public issues or pull requests.
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T19:02:41.756Z
Learning: Use Conventional Commits with an allowed type such as `feat`, `fix`, `docs`, `chore`, `refactor`, `test`, `build`, or `ci`.
Learnt from: CR
Repo: NVIDIA/topograph
Timestamp: 2026-08-11T19:02:41.756Z
Learning: Breaking changes to the configuration schema, label keys, or `Vertex` shape require discussion in an issue first.
🔇 Additional comments (9)
pkg/accelerator/accelerator_test.go (2)
252-277: Cover the remaining constructor error branches.The tests still omit the nil
rest.Configcase forNewKubernetesNodeDiscovererand the nil runner case forNewNvidiaSMIDiscoverer.Also applies to: 299-328
20-328: LGTM!cmd/node-data-broker/main_test.go (3)
84-102: Add omitted accelerator configuration coverage.The test covers
accelerator: {}but does not cover an omittedacceleratorsection through the node-data-broker path.
13-82: LGTM!Also applies to: 105-146
25-25: 📐 Maintainability & Code QualityDo not retain
TestGetExtrascoverage. ThegetExtrasfunction and--setoption are not present, so this coverage is not applicable.> Likely an incorrect or invalid review comment.docs/providers/infiniband.md (2)
121-143: Document theuseGpuCliqueLabelmigration.State that the InfiniBand
provider.params.useGpuCliqueLabelsetting was removed. Document the replacementaccelerator.source: kubernetes-labelandaccelerator.kubernetesLabel.keyconfiguration.
20-20: LGTM!Also applies to: 44-50, 65-75, 94-101, 145-170
charts/topograph/tests/node-data-broker_configmap_test.yaml (1)
8-104: LGTM!docs/api.md (1)
75-78: LGTM!
| /* | ||
| * Copyright 2026 NVIDIA CORPORATION | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required Apache 2.0 boilerplate.
Lines 1-4 include an SPDX identifier but do not include the Apache 2.0 boilerplate required for new Go files. Copy the repository-standard header below the NVIDIA copyright line.
As per coding guidelines, “Every new Go file must include the NVIDIA copyright header followed by the matching Apache 2.0 boilerplate.”
🤖 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/accelerator/accelerator_test.go` around lines 1 - 4, Update the file
header above the package declaration in accelerator_test.go by adding the
repository-standard Apache 2.0 boilerplate immediately after the NVIDIA
copyright line, while preserving the existing SPDX identifier and copyright
text.
Source: Coding guidelines
| func TestParseNvidiaSMIOutput(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| output string | ||
| partition string | ||
| err string | ||
| }{ | ||
| {name: "duplicates", output: "uuid, 7\nuuid , 7\n", partition: "uuid.7"}, | ||
| {name: "missing", err: "missing NVL partition ID"}, | ||
| {name: "missing UUID", output: ", 7", err: "missing ClusterUUID"}, | ||
| {name: "missing clique", output: "uuid, ", err: "missing CliqueId"}, | ||
| {name: "malformed CSV", output: "uuid", err: `expected ClusterUUID and CliqueId CSV fields, got "uuid"`}, | ||
| {name: "N/A UUID", output: "N/A, 7", err: "ClusterUUID is N/A"}, | ||
| {name: "N/A clique", output: "uuid, N/A", err: "CliqueId is N/A"}, | ||
| {name: "ambiguous", output: "uuid, 7\nuuid, 8", err: "ambiguous NVL partition IDs: uuid.7, uuid.8"}, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| partition, err := ParseNvidiaSMIOutput(test.output) | ||
| if test.err != "" { | ||
| require.EqualError(t, err, test.err) | ||
| return | ||
| } | ||
| require.NoError(t, err) | ||
| require.Equal(t, test.partition, partition) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject CSV rows with extra fields.
Add a malformed-input case such as uuid, 7, extra. The current tests only prove rejection of rows with too few fields. The parser must reject rows that do not contain exactly ClusterUUID and CliqueId.
As per path instructions, “Flag missing negative, cancellation, concurrency, malformed-input, boundary, and regression cases.”
🤖 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/accelerator/accelerator_test.go` around lines 330 - 358, Extend
TestParseNvidiaSMIOutput with a malformed CSV case using an extra field such as
“uuid, 7, extra”, and assert the parser returns the expected field-count error.
Ensure ParseNvidiaSMIOutput rejects rows unless they contain exactly the
ClusterUUID and CliqueId fields, while preserving existing handling for valid
and under-specified rows.
Source: Path instructions
2 participants
Decouple accelerator-domain discovery from InfiniBand fabric discovery.
Support nvidia-smi, Kubernetes Node labels, and disabled discovery through
provider.params.accelerator.
Query NVL partition IDs using the nvidia-smi CSV interface, deduplicate
per-GPU results, reject unavailable fields, and normalize IDs as
ClusterUUID.CliqueId. Grant node-data-broker GPU Operator permissions only
when nvidia-smi discovery is enabled.
BREAKING CHANGE: remove useGpuCliqueLabel. Omitting accelerator, using an
empty accelerator section, or selecting source: none disables accelerator
discovery.
closes #458