Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change replaces hard-coded GPU-clique handling with configurable ChangesAccelerator-domain source configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EngineConfig
participant KubernetesNodes
participant TopologyEngine
participant TopologyOutput
EngineConfig->>TopologyEngine: configure acceleratorDomainSourceLabel
TopologyEngine->>KubernetesNodes: read configured label values
KubernetesNodes-->>TopologyEngine: source values and provider metadata
TopologyEngine->>TopologyOutput: publish accelerator domains and topology labels
Possibly related PRs
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #460 +/- ##
==========================================
+ Coverage 72.15% 76.99% +4.84%
==========================================
Files 89 99 +10
Lines 5689 7169 +1480
==========================================
+ Hits 4105 5520 +1415
+ Misses 1382 1364 -18
- Partials 202 285 +83 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile SummaryThe PR makes accelerator-domain source labels configurable across the Kubernetes, NFD, and Slinky engines and removes implicit GPU-clique handling.
Confidence Score: 3/5The PR is not yet safe to merge because the NFD identifier migration breaks existing consumers and Slinky can still emit incomplete block topology. The current code still replaces the published NFD attribute and group identifiers while deleting their old managed objects, and it still accepts a nonempty label-backed Slinky domain map after skipping selected nodes with missing metadata. Files Needing Attention: pkg/engines/nfd/objects.go and pkg/engines/slinky/engine.go Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
N[Kubernetes Node labels] --> K[Kubernetes engine]
N --> F[NFD engine]
N --> S[Slinky engine]
P[Provider topology] --> K
P --> F
P --> S
K --> KL[Node topology labels]
F --> NF[NodeFeature and NodeFeatureGroup resources]
S --> ST[Slurm block topology]
Reviews (5): Last reviewed commit: "feat(engines): make accelerator domain s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 29-33: The commented documentation for
acceleratorDomainSourceLabel must state that the k8s engine rejects any
non-empty acceleratorLabel when this parameter is set, including explicitly
setting the default value; replace the narrower “customized” restriction in the
acceleratorDomainSourceLabel guidance without changing the configuration
behavior.
In `@docs/api.md`:
- Line 102: Update the acceleratorDomainSourceLabel documentation to separate
Slinky behavior from other engines: state that Slinky replaces provider domains
with label-backed domains, excludes mapped nodes lacking a non-empty source
label, and errors only when no usable domains remain. Remove the generic claim
that unlabeled nodes retain provider-domain behavior, while preserving the
existing k8s and nfd behavior descriptions.
In `@docs/reference/node-labels.md`:
- Around line 54-61: Update docs/reference/node-labels.md lines 54-61 to replace
the claim that label values always come from provider discovery with the
configured acceleratorDomainSourceLabel override behavior. Update
docs/design/nfd-engine-sdd.md lines 231-239 to add acceleratorDomainSourceLabel
to the Engine Parameters section, documenting that a non-empty label value
overrides the provider accelerator domain and suppresses the provider
sub-domain.
In `@pkg/engines/k8s/kubernetes.go`:
- Around line 175-177: Update getParameters to reject configurations where
acceleratorDomainSourceLabel matches any entry in fabricLabels, in addition to
the existing acceleratorLabel validation. Add parameter-validation and
reconciliation regression cases covering this collision, ensuring externally
owned source labels are never overwritten by generated fabric labels.
In `@pkg/engines/nfd/engine_test.go`:
- Around line 394-419: Add a regression case to
TestBuildNFDObjectsUsesProviderAcceleratorDomainWhenSourceIsOmitted by labeling
the node with nvidia.com/gpu.clique, then assert the generated feature still
contains provider-domain-a and provider-sub-domain-a. Keep
acceleratorDomainSourceValues invoked with an omitted source so the test
verifies GPU-clique is not implicitly used as a fallback.
In `@pkg/engines/slinky/engine.go`:
- Line 204: Update the instance-loading condition around getClusterNodes so
supplied non-empty instances are returned without Kubernetes node access unless
UseDynamicNodes requires cluster data; do not use AcceleratorDomainSourceLabel
as a trigger for loading nodes, since it is only needed for block topology
handling later. Add regression coverage for a configured source label, supplied
instances, non-block topology, and a client whose node listing fails, asserting
the request still succeeds with the supplied instances.
🪄 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: 5b243208-4829-4081-b657-f54d6f7f718f
📒 Files selected for processing (28)
CHANGELOG.mdcharts/topograph/templates/_validation.tplcharts/topograph/tests/validation_test.yamlcharts/topograph/values.schema.jsoncharts/topograph/values.slinky.block-example.yamlcharts/topograph/values.slinky.ib.block-example.yamlcharts/topograph/values.slinky.partition-example.yamlcharts/topograph/values.yamldemos/dra-slinky/values.dra-slinky.kwok.yamldocs/api.mddocs/design/nfd-engine-sdd.mddocs/engines/k8s.mddocs/engines/nfd.mddocs/engines/slinky.mddocs/reference/node-labels.mdinternal/k8s/utils.gointernal/k8s/utils_test.gopkg/engines/k8s/engine.gopkg/engines/k8s/engine_test.gopkg/engines/k8s/kubernetes.gopkg/engines/k8s/kubernetes_test.gopkg/engines/k8s/labeler.gopkg/engines/nfd/engine.gopkg/engines/nfd/engine_test.gopkg/engines/nfd/objects.gopkg/engines/slinky/engine.gopkg/engines/slinky/engine_test.gotests/ci/values.slinky-dra-block.yaml
💤 Files with no reviewable changes (2)
- demos/dra-slinky/values.dra-slinky.kwok.yaml
- tests/ci/values.slinky-dra-block.yaml
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Greptile Review
- GitHub Check: k8s / test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: build
- GitHub Check: govulncheck
🧰 Additional context used
📓 Path-based instructions (11)
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/values.slinky.block-example.yamlcharts/topograph/values.slinky.ib.block-example.yamlcharts/topograph/values.slinky.partition-example.yamlcharts/topograph/tests/validation_test.yamlcharts/topograph/values.yaml
**/*
📄 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:
charts/topograph/values.slinky.block-example.yamlcharts/topograph/values.schema.jsoncharts/topograph/values.slinky.ib.block-example.yamlcharts/topograph/values.slinky.partition-example.yamlcharts/topograph/tests/validation_test.yamlpkg/engines/k8s/engine_test.godocs/api.mdinternal/k8s/utils.gocharts/topograph/values.yamlinternal/k8s/utils_test.godocs/engines/k8s.mdcharts/topograph/templates/_validation.tplCHANGELOG.mdpkg/engines/k8s/labeler.gopkg/engines/k8s/engine.godocs/design/nfd-engine-sdd.mddocs/reference/node-labels.mddocs/engines/nfd.mdpkg/engines/k8s/kubernetes_test.gopkg/engines/nfd/objects.gopkg/engines/k8s/kubernetes.gopkg/engines/slinky/engine_test.gopkg/engines/nfd/engine.gopkg/engines/nfd/engine_test.godocs/engines/slinky.mdpkg/engines/slinky/engine.go
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/values.slinky.block-example.yamlcharts/topograph/values.schema.jsoncharts/topograph/values.slinky.ib.block-example.yamlcharts/topograph/values.slinky.partition-example.yamlcharts/topograph/tests/validation_test.yamlcharts/topograph/values.yamlcharts/topograph/templates/_validation.tpl
charts/topograph/tests/**/*.{yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Run Helm chart tests when changing
charts/topograph/; review snapshot changes before committing updates.
Files:
charts/topograph/tests/validation_test.yaml
**/*.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/engines/k8s/engine_test.gointernal/k8s/utils.gointernal/k8s/utils_test.gopkg/engines/k8s/labeler.gopkg/engines/k8s/engine.gopkg/engines/k8s/kubernetes_test.gopkg/engines/nfd/objects.gopkg/engines/k8s/kubernetes.gopkg/engines/slinky/engine_test.gopkg/engines/nfd/engine.gopkg/engines/nfd/engine_test.gopkg/engines/slinky/engine.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/k8s/engine_test.gopkg/engines/k8s/labeler.gopkg/engines/k8s/engine.gopkg/engines/k8s/kubernetes_test.gopkg/engines/nfd/objects.gopkg/engines/k8s/kubernetes.gopkg/engines/slinky/engine_test.gopkg/engines/nfd/engine.gopkg/engines/nfd/engine_test.gopkg/engines/slinky/engine.go
pkg/engines/k8s/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do not invent Kubernetes label keys in provider code; use canonical graph values and the configured closest-first
fabricLabelsand fixed accelerator sub-domain key.
Files:
pkg/engines/k8s/engine_test.gopkg/engines/k8s/labeler.gopkg/engines/k8s/engine.gopkg/engines/k8s/kubernetes_test.gopkg/engines/k8s/kubernetes.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
New or changed public behavior and new code paths should be covered by tests.
Files:
pkg/engines/k8s/engine_test.gointernal/k8s/utils_test.gopkg/engines/k8s/kubernetes_test.gopkg/engines/slinky/engine_test.gopkg/engines/nfd/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:
pkg/engines/k8s/engine_test.gointernal/k8s/utils_test.gopkg/engines/k8s/kubernetes_test.gopkg/engines/slinky/engine_test.gopkg/engines/nfd/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:
pkg/engines/k8s/engine_test.gointernal/k8s/utils.gointernal/k8s/utils_test.gopkg/engines/k8s/labeler.gopkg/engines/k8s/engine.gopkg/engines/k8s/kubernetes_test.gopkg/engines/nfd/objects.gopkg/engines/k8s/kubernetes.gopkg/engines/slinky/engine_test.gopkg/engines/nfd/engine.gopkg/engines/nfd/engine_test.gopkg/engines/slinky/engine.go
docs/engines/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
New, changed, or removed engines require updates to the corresponding engine documentation.
Files:
docs/engines/k8s.mddocs/engines/nfd.mddocs/engines/slinky.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Record applicable user-facing features, fixes, breaking changes, or Helm migrations under
[Unreleased].
Files:
CHANGELOG.md
🪛 LanguageTool
docs/design/nfd-engine-sdd.md
[style] ~235-~235: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...re removed when cleanup is enabled. - Verify an empty generated object set returns a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (21)
charts/topograph/templates/_validation.tpl (1)
15-22: LGTM!internal/k8s/utils.go (1)
17-17: LGTM!Also applies to: 54-62
pkg/engines/k8s/engine.go (1)
10-10: LGTM!Also applies to: 44-46, 89-96
pkg/engines/k8s/labeler.go (1)
14-14: LGTM!Also applies to: 42-43
charts/topograph/values.slinky.block-example.yaml (1)
22-22: LGTM!charts/topograph/values.slinky.ib.block-example.yaml (1)
42-45: LGTM!pkg/engines/slinky/engine.go (1)
94-96: LGTM!Also applies to: 153-157, 309-309, 321-324, 335-358, 373-374, 455-460
charts/topograph/tests/validation_test.yaml (1)
64-73: LGTM!internal/k8s/utils_test.go (1)
2-2: LGTM!Also applies to: 84-112
CHANGELOG.md (1)
14-14: LGTM!Also applies to: 30-30, 46-46
docs/engines/k8s.md (1)
17-22: LGTM!Also applies to: 71-91, 162-172
pkg/engines/k8s/kubernetes.go (1)
68-73: LGTM!Also applies to: 214-237
pkg/engines/k8s/kubernetes_test.go (1)
350-480: LGTM!pkg/engines/slinky/engine_test.go (1)
42-42: LGTM!Also applies to: 228-255, 402-665
charts/topograph/values.schema.json (1)
52-59: LGTM!charts/topograph/values.slinky.partition-example.yaml (1)
47-47: LGTM!docs/engines/nfd.md (1)
11-20: LGTM!Also applies to: 72-72, 85-85, 133-134, 170-177
pkg/engines/nfd/engine.go (1)
48-50: LGTM!Also applies to: 103-107, 141-145, 167-179
pkg/engines/nfd/engine_test.go (1)
81-190: LGTM!Also applies to: 331-392, 421-430
pkg/engines/nfd/objects.go (1)
42-46: LGTM!Also applies to: 77-156
docs/engines/slinky.md (1)
93-126: LGTM!
|
🌿 Preview your docs: https://nvidia-preview-pull-request-460.docs.buildwithfern.com/topograph |
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/engines/slinky/engine.go`:
- Around line 342-350: Update the annotation validation in the node-processing
flow before domains.AddHost to treat instance values that are empty or contain
only whitespace as missing, preserving the existing missingAnnotation warning
and continue behavior. Add a regression test covering a whitespace-only
topology.KeyNodeInstance annotation and verify no HostInfo is created with an
empty InstanceID.
🪄 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: dd3fd2cb-2fa9-4d1c-bf77-267265666afd
📒 Files selected for processing (5)
pkg/engines/slinky/engine.gopkg/engines/slinky/engine_test.gopkg/topology/graph.gopkg/topology/graph_test.gopkg/topology/instances.go
💤 Files with no reviewable changes (1)
- pkg/topology/instances.go
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Greptile Review
- GitHub Check: check
- GitHub Check: govulncheck
- GitHub Check: build
- GitHub Check: test
- GitHub Check: k8s / gcp-sim
- GitHub Check: k8s / aws-sim
- GitHub Check: oci-sim / slinky
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/topology/graph.gopkg/topology/graph_test.gopkg/engines/slinky/engine.gopkg/engines/slinky/engine_test.go
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/graph.gopkg/topology/graph_test.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/topology/graph.gopkg/topology/graph_test.gopkg/engines/slinky/engine.gopkg/engines/slinky/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:
pkg/topology/graph.gopkg/topology/graph_test.gopkg/engines/slinky/engine.gopkg/engines/slinky/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:
pkg/topology/graph_test.gopkg/engines/slinky/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:
pkg/topology/graph_test.gopkg/engines/slinky/engine_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/slinky/engine.gopkg/engines/slinky/engine_test.go
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/engines/slinky/engine.go`:
- Around line 455-460: Add direct GenerateOutput tests for both tree and flat
plugins when AcceleratorDomainSourceLabel is set, UseDynamicNodes is disabled,
and selector-based partition topology is absent. Configure fake-client reactors
to fail or record any Node and Pod list requests, then assert GenerateOutput
succeeds without issuing either request while preserving the expected output.
🪄 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: 130773d2-5733-4610-9dcc-bc7c28801bd4
📒 Files selected for processing (9)
CHANGELOG.mdcharts/topograph/values.schema.jsoncharts/topograph/values.yamldocs/api.mddocs/design/nfd-engine-sdd.mddocs/reference/node-labels.mdpkg/engines/nfd/engine_test.gopkg/engines/slinky/engine.gopkg/engines/slinky/engine_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: Greptile Review
- GitHub Check: build
- GitHub Check: govulncheck
- GitHub Check: test
- GitHub Check: check
- GitHub Check: k8s / test
- GitHub Check: k8s / aws-sim
- GitHub Check: Fern Check
- GitHub Check: k8s / gcp-sim
- GitHub Check: oci-sim / slinky
🧰 Additional context used
📓 Path-based instructions (8)
**/*.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/engines/nfd/engine_test.gopkg/engines/slinky/engine.gopkg/engines/slinky/engine_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/engine_test.gopkg/engines/slinky/engine.gopkg/engines/slinky/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:
pkg/engines/nfd/engine_test.gopkg/engines/slinky/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:
pkg/engines/nfd/engine_test.gopkg/engines/slinky/engine_test.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/engines/nfd/engine_test.gocharts/topograph/values.schema.jsondocs/api.mdcharts/topograph/values.yamlpkg/engines/slinky/engine.godocs/design/nfd-engine-sdd.mdpkg/engines/slinky/engine_test.goCHANGELOG.mddocs/reference/node-labels.md
{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/engines/nfd/engine_test.gopkg/engines/slinky/engine.gopkg/engines/slinky/engine_test.go
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/values.schema.jsoncharts/topograph/values.yaml
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/values.yaml
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Record applicable user-facing features, fixes, breaking changes, or Helm migrations under
[Unreleased].
Files:
CHANGELOG.md
🪛 LanguageTool
docs/api.md
[style] ~102-~102: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...abel-backed domains can be constructed. For k8s, this parameter cannot be set tog...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/design/nfd-engine-sdd.md
[style] ~240-~240: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...re removed when cleanup is enabled. - Verify an empty generated object set returns a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (16)
CHANGELOG.md (1)
14-14: LGTM!Also applies to: 30-30, 46-46, 61-61
docs/reference/node-labels.md (1)
13-13: LGTM!Also applies to: 24-24, 54-68, 76-83, 113-118
charts/topograph/values.yaml (1)
29-35: Correct the k8s restriction statement.
acceleratorLabelmust be absent whenacceleratorDomainSourceLabelis set. The k8s engine rejects any non-emptyacceleratorLabel, including an explicit default value.pkg/engines/slinky/engine.go (3)
94-96: LGTM!Also applies to: 153-157
204-204: LGTM!
309-374: LGTM!charts/topograph/values.schema.json (1)
52-59: LGTM!docs/design/nfd-engine-sdd.md (1)
12-12: LGTM!Also applies to: 21-21, 77-78, 122-126, 139-140, 199-205, 236-244
docs/api.md (1)
102-102: LGTM!pkg/engines/nfd/engine_test.go (3)
85-98: LGTM!
130-190: LGTM!
338-430: LGTM!pkg/engines/slinky/engine_test.go (4)
42-42: LGTM!Also applies to: 228-255
380-403: LGTM!
406-570: LGTM!
572-679: LGTM!
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/engines/slinky/engine_test.go`:
- Around line 647-679: Add failing list reactors for nodes and pods to the fake
client in TestGenerateOutputUsesProviderDomainsWhenSourceIsOmitted, so any
unexpected Kubernetes list access fails the test. After GenerateOutput
completes, assert that client.Actions() is empty while preserving the existing
provider-domain output assertions.
🪄 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: 8eb37fd1-c410-4d72-89fe-55e50bf9696c
📒 Files selected for processing (1)
pkg/engines/slinky/engine_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: Greptile Review
- GitHub Check: k8s / aws-sim
- GitHub Check: k8s / test
- GitHub Check: k8s / gcp-sim
- GitHub Check: test
- GitHub Check: govulncheck
- GitHub Check: check
- GitHub Check: build
- GitHub Check: oci-sim / slinky
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/engines/slinky/engine_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/slinky/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:
pkg/engines/slinky/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:
pkg/engines/slinky/engine_test.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/engines/slinky/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:
pkg/engines/slinky/engine_test.go
🔇 Additional comments (1)
pkg/engines/slinky/engine_test.go (1)
42-43: LGTM!Also applies to: 228-255, 380-390, 406-435, 470-470, 482-482, 507-554, 566-587, 605-616, 625-629, 681-738
Signed-off-by: Dmitry Shmulevich <17212177+dmitsh@users.noreply.github.com>
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/engines/slinky/engine_test.go`:
- Around line 655-657: Update the fallback provider fixture near providerDomains
to use AddHostInfo with HostInfo values containing provider sub-domains instead
of AddHost. Extend the test assertions to verify the generated block topology
preserves the two-level provider structure when AcceleratorDomainSourceLabel is
unset.
🪄 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: d6d94b20-b585-4cdd-96dc-a8046281db89
📒 Files selected for processing (1)
pkg/engines/slinky/engine_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Greptile Review
- GitHub Check: build
- GitHub Check: check
- GitHub Check: govulncheck
- GitHub Check: test
- GitHub Check: k8s / gcp-sim
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/engines/slinky/engine_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/slinky/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:
pkg/engines/slinky/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:
pkg/engines/slinky/engine_test.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/engines/slinky/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:
pkg/engines/slinky/engine_test.go
🔇 Additional comments (1)
pkg/engines/slinky/engine_test.go (1)
42-43: LGTM!Also applies to: 228-255, 380-390, 406-470, 482-570, 572-645, 688-745
1 participant
Add acceleratorDomainSourceLabel to the k8s, NFD, and Slinky engines.
Use configured Node label values as authoritative accelerator domains and
suppress provider-derived accelerator sub-domains when overridden.
Remove implicit nvidia.com/gpu.clique handling and the Slinky
useGpuCliqueLabel parameter. Prevent the k8s engine from combining
acceleratorDomainSourceLabel with a customized acceleratorLabel.
closes #459