fix(validator): disclose cordoned RDMA nodes in expected-resources - #1981
fix(validator): disclose cordoned RDMA nodes in expected-resources#1981njhensley wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRDMA readiness now enumerates GPU nodes, validates only schedulable Mellanox nodes, and reports cordoned nodes in coverage totals. Probe results preserve coverage on success, failure, cancellation, and empty-node paths. Structured evidence emits Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@validators/deployment/expected_resources.go`:
- Around line 1128-1160: Update the listCtx.Done() branch in the node-scanning
loop to return the accumulated coverage variable instead of a new empty
rdmaFabricCoverage value, while preserving the timeout error and cancellation
details. Keep the existing accumulation of cordoned and schedulable nodes
unchanged.
- Around line 1112-1120: Update rdmaFabricProbeCoverage to return the existing
err from helper.FindGpuNodes directly when node discovery fails, rather than
wrapping it with errors.Wrap, so ErrCodeTimeout and other structured error codes
are preserved.
🪄 Autofix (Beta)
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: 1d25fab5-25b3-4f5f-85a3-a183c8fbc694
📒 Files selected for processing (4)
docs/contributor/validator.mdpkg/evidence/redact/redact_test.govalidators/deployment/expected_resources.govalidators/deployment/expected_resources_test.go
a19fb0e to
12a2c0c
Compare
|
Addressed both CodeRabbit findings. (1) Cancellation mid-scan now returns the accumulated coverage (cordoned nodes already seen) with schedulable set from the scanned cohort, instead of an empty partition — matching the function contract and all other error paths. (2) The FindGpuNodes error path: verified helper.FindGpuNodes returns a coded StructuredError (ErrCodeInternal on the node-List failure — the realistic path — and ErrCodeTimeout only on ctx cancel, which the in-loop listCtx.Done() branch surfaces on the accumulated coverage), so the ErrCodeInternal wrap adds gate context without clobbering the code; kept with a clarifying comment. Lint + go test -race ./validators/deployment/... green. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
validators/deployment/expected_resources.go (1)
1112-1126: 🎯 Functional Correctness | 🟠 MajorStill clobbers
ErrCodeTimeoutwithErrCodeInternalon cancellation insideFindGpuNodes.
helper.FindGpuNodesruns its ownctx.Done()check in its internal loop overnodeList.Itemsand can return anErrCodeTimeout-coded error from that loop, before this function's own loop at line 1135 ever starts. The new comment at Lines 1118-1123 states that "the listCtx.Done() branch below already surfaces as ErrCodeTimeout on the accumulated coverage," but that branch only fires for cancellation during this function's loop overgpuNodes, not for cancellation insideFindGpuNodesitself. WhenFindGpuNodesreturns anErrCodeTimeout-coded error,errors.Wrap(errors.ErrCodeInternal, ...)still overwrites that code, discarding the caller's actual cancellation classification.The coding guidelines state to not double-wrap errors that already carry the correct structured code. Use
errors.PropagateOrWrapso an existingStructuredErrorcode fromFindGpuNodes(includingErrCodeTimeout) passes through unchanged, while a plain error still getsErrCodeInternaland the added context.🐛 Proposed fix to preserve the structured error code
gpuNodes, err := helper.FindGpuNodes(listCtx, ctx.Clientset) if err != nil { - // helper.FindGpuNodes already returns a coded *errors.StructuredError — - // ErrCodeInternal on the realistic node-List failure (the same code used - // here, so this wrap adds gate context without clobbering it) and - // ErrCodeTimeout only on ctx cancellation, which the listCtx.Done() branch - // below already surfaces as ErrCodeTimeout on the accumulated coverage. - // Wrapping mirrors the pre-existing FindSchedulableGpuNodes gate path. - return rdmaFabricCoverage{}, errors.Wrap(errors.ErrCodeInternal, - "failed to list nodes for the RDMA fabric readiness gate", err) + // helper.FindGpuNodes may already return a coded *errors.StructuredError + // (ErrCodeInternal on the List failure, ErrCodeTimeout if cancellation + // interrupts its own internal node scan). PropagateOrWrap preserves that + // code unchanged instead of clobbering it with ErrCodeInternal. + return rdmaFabricCoverage{}, errors.PropagateOrWrap(err, errors.ErrCodeInternal, + "failed to list nodes for the RDMA fabric readiness gate") }To confirm
PropagateOrWrap's exact propagation behavior in this repository's currentpkg/errorspackage:#!/bin/bash # Description: Inspect PropagateOrWrap and Wrap implementations for code-preservation semantics. rg -n -A 20 'func PropagateOrWrap\(' pkg/errors rg -n -A 15 'func Wrap\(' pkg/errors🤖 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 `@validators/deployment/expected_resources.go` around lines 1112 - 1126, In rdmaFabricProbeCoverage, replace the errors.Wrap call handling helper.FindGpuNodes failures with errors.PropagateOrWrap, preserving existing StructuredError codes such as ErrCodeTimeout while assigning ErrCodeInternal and the existing contextual message to plain errors. Remove or update the misleading explanatory comment so it matches the corrected propagation behavior.Source: Coding guidelines
🤖 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 `@validators/deployment/expected_resources_test.go`:
- Around line 1471-1528: Add a test for rdmaFabricProbeCoverage that cancels its
scan context after at least one cordoned node has been collected, then verifies
the returned error reflects cancellation while the partial coverage still
includes that cordoned node and counts it in total(). Exercise the
listCtx.Done() branch directly, using the existing RDMA node helpers and test
patterns from TestRDMAFabricProbeCoverage_DisclosesCordoned and
TestRDMAFabricProbeCoverage_CountsCordonedOnFailClosed.
---
Duplicate comments:
In `@validators/deployment/expected_resources.go`:
- Around line 1112-1126: In rdmaFabricProbeCoverage, replace the errors.Wrap
call handling helper.FindGpuNodes failures with errors.PropagateOrWrap,
preserving existing StructuredError codes such as ErrCodeTimeout while assigning
ErrCodeInternal and the existing contextual message to plain errors. Remove or
update the misleading explanatory comment so it matches the corrected
propagation behavior.
🪄 Autofix (Beta)
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: 231ab9d9-a451-400b-abb5-1172526ebe40
📒 Files selected for processing (4)
docs/contributor/validator.mdpkg/evidence/redact/redact_test.govalidators/deployment/expected_resources.govalidators/deployment/expected_resources_test.go
| // TestRDMAFabricProbeCoverage_DisclosesCordoned is the end-to-end proof of #1952: | ||
| // a cordoned Mellanox RDMA GPU node is enumerated (via helper.FindGpuNodes) and | ||
| // surfaced in the coverage partition — visible and counted — while still being | ||
| // excluded from the validated cohort. Under the pre-fix code path | ||
| // (FindSchedulableGpuNodes) the cordoned node vanished entirely, so this test | ||
| // fails without the production change. | ||
| func TestRDMAFabricProbeCoverage_DisclosesCordoned(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| clientset := k8sfake.NewClientset( | ||
| rdmaGPUNode("rdma-gpu-0", 8, 1000), // schedulable, fabric ready → validated cohort | ||
| cordon(rdmaGPUNode("rdma-drain-0", 8, -1)), // cordoned RDMA node → disclosed, not dropped | ||
| ) | ||
| ctx := &validators.Context{Ctx: context.Background(), Clientset: clientset} | ||
|
|
||
| cov, err := rdmaFabricProbeCoverage(ctx) | ||
| if err != nil { | ||
| t.Fatalf("rdmaFabricProbeCoverage() error = %v, want nil (the one schedulable RDMA node carries the fabric)", err) | ||
| } | ||
| if cov.schedulable != 1 { | ||
| t.Errorf("schedulable cohort = %d, want 1 (cordoned node excluded from validation)", cov.schedulable) | ||
| } | ||
| if len(cov.cordoned) != 1 || cov.cordoned[0] != "rdma-drain-0" { | ||
| t.Errorf("cordoned = %v, want [rdma-drain-0] (must be disclosed, not silently dropped)", cov.cordoned) | ||
| } | ||
| if got := cov.total(); got != 2 { | ||
| t.Errorf("total() = %d, want 2 (schedulable + cordoned, never narrowed)", got) | ||
| } | ||
| } | ||
|
|
||
| // TestRDMAFabricProbeCoverage_CountsCordonedOnFailClosed proves the cordoned | ||
| // disclosure survives the fail-closed paths too: when the sole schedulable RDMA | ||
| // node has not finished rolling out the fabric, the probe returns an error AND | ||
| // still reports the cordoned node in the coverage so the terminal disclosure can | ||
| // name it. | ||
| func TestRDMAFabricProbeCoverage_CountsCordonedOnFailClosed(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| clientset := k8sfake.NewClientset( | ||
| rdmaGPUNode("rdma-gpu-0", 8, -1), // schedulable but fabric absent → not ready | ||
| cordon(rdmaGPUNode("rdma-drain-0", 8, -1)), // cordoned RDMA node → still disclosed | ||
| ) | ||
| ctx := &validators.Context{Ctx: context.Background(), Clientset: clientset} | ||
|
|
||
| cov, err := rdmaFabricProbeCoverage(ctx) | ||
| if err == nil { | ||
| t.Fatal("expected a fail-closed error while the fabric is absent, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "not yet allocatable") { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(cov.cordoned) != 1 || cov.cordoned[0] != "rdma-drain-0" { | ||
| t.Errorf("cordoned = %v, want [rdma-drain-0] even on the fail-closed path", cov.cordoned) | ||
| } | ||
| if got := cov.total(); got != 2 { | ||
| t.Errorf("total() = %d, want 2 (cordoned counted even on failure)", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add a test for cancellation mid-scan with partial coverage.
TestRDMAFabricProbeCoverage_DisclosesCordoned and TestRDMAFabricProbeCoverage_CountsCordonedOnFailClosed cover the cordoned-disclosure and fail-closed paths, but no test exercises the listCtx.Done() branch in rdmaFabricProbeCoverage (production file, Lines 1138-1149) with cordoned nodes already collected before cancellation. This is the exact regression the earlier review flagged and the fix addressed; a dedicated test locks in that behavior.
🤖 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 `@validators/deployment/expected_resources_test.go` around lines 1471 - 1528,
Add a test for rdmaFabricProbeCoverage that cancels its scan context after at
least one cordoned node has been collected, then verifies the returned error
reflects cancellation while the partial coverage still includes that cordoned
node and counts it in total(). Exercise the listCtx.Done() branch directly,
using the existing RDMA node helpers and test patterns from
TestRDMAFabricProbeCoverage_DisclosesCordoned and
TestRDMAFabricProbeCoverage_CountsCordonedOnFailClosed.
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
12a2c0c to
bb384ad
Compare
|
Applied CodeRabbit fix: FindGpuNodes error path now uses errors.PropagateOrWrap, so an inner ErrCodeTimeout (FindGpuNodes cancelling in its own node scan before this loop) propagates unchanged while a plain List failure still gets ErrCodeInternal. Updated TestRDMAFabricProbe_FailsClosedOnListError to assert the propagated ErrCodeInternal code + fail-closed behavior instead of the removed gate-context message string. Lint + go test -race ./validators/deployment/... green. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
validators/deployment/expected_resources_rdma_test.go (1)
208-234: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for
ErrCodeTimeoutpropagation throughPropagateOrWrap.This test only exercises the plain-error branch of
PropagateOrWrap(aList()failure, whichhelper.FindGpuNodesitself already wraps asErrCodeInternal). The PR's stated purpose is preserving an inner structured code that differs from the fallback — specificallyErrCodeTimeoutwhen cancellation interruptsFindGpuNodes' own node scan. No test drives that path and confirmsrdmaFabricProbeCoveragereturnsErrCodeTimeoutunchanged instead of overwriting it withErrCodeInternal.Add a test that creates at least one node, pre-cancels the context passed to
rdmaFabricProbeCoverage, and assertsstderrors.Is(err, errors.New(errors.ErrCodeTimeout, "")).🤖 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 `@validators/deployment/expected_resources_rdma_test.go` around lines 208 - 234, Add a test alongside TestRDMAFabricProbe_FailsClosedOnListError that creates at least one node, passes an already-canceled context to rdmaFabricProbeCoverage, and verifies the returned error matches ErrCodeTimeout via stderrors.Is with errors.New(errors.ErrCodeTimeout, ""). Ensure the test exercises cancellation during FindGpuNodes’ node-scan path and confirms PropagateOrWrap preserves the inner timeout code rather than replacing it with ErrCodeInternal.
🤖 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.
Outside diff comments:
In `@validators/deployment/expected_resources_rdma_test.go`:
- Around line 208-234: Add a test alongside
TestRDMAFabricProbe_FailsClosedOnListError that creates at least one node,
passes an already-canceled context to rdmaFabricProbeCoverage, and verifies the
returned error matches ErrCodeTimeout via stderrors.Is with
errors.New(errors.ErrCodeTimeout, ""). Ensure the test exercises cancellation
during FindGpuNodes’ node-scan path and confirms PropagateOrWrap preserves the
inner timeout code rather than replacing it with ErrCodeInternal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3eaea061-99df-4340-b440-e708e55a35ef
📒 Files selected for processing (5)
docs/contributor/validator.mdpkg/evidence/redact/redact_test.govalidators/deployment/expected_resources.govalidators/deployment/expected_resources_rdma_test.govalidators/deployment/expected_resources_test.go
|
Thanks @coderabbitai — verified all three inline comments on `bb384ad6` against the current code. All three are non-actionable:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -a '^expected_resources(\.go|_test\.go|_rdma_test\.go)$' validators/deployment
ast-grep outline validators/deployment/expected_resources.go --items all --match 'rdmaFabricProbeCoverage|FindGpuNodes|PropagateOrWrap' || true
rg -n -C 8 'rdmaFabricProbeCoverage|FindGpuNodes\s*\(|PropagateOrWrap|listCtx\.Done\(\)' \
validators/deployment/expected_resources.go
rg -n -C 6 'func\s+FindGpuNodes\b|FindGpuNodes\s*=|FindGpuNodes\b' \
--glob '*.go' .
rg -n -C 5 'func\s+PropagateOrWrap\b|PropagateOrWrap\b' \
--glob '*.go' pkg validators || true
rg -n -C 4 'TestRDMAFabricProbeCoverage_DisclosesCordoned|TestRDMAFabricProbeCoverage_CountsCordonedOnFailClosed|FailsClosedOnListError' \
validators/deploymentLength of output: 211309
The The public path does not provide a deterministic point to cancel after 🐇 ✅ ✏️ Learnings added
You are interacting with an AI system. |
yuanchen8911
left a comment
There was a problem hiding this comment.
The fail-closed disclosure path has a gap: on a never-ready poll, the pod is killed before the terminal disclosure runs.
The catalog's 8m timeout feeds both the Job's activeDeadlineSeconds and AICR_CHECK_TIMEOUT with no margin (pkg/validator/v1/job_plan.go L229/L273/L303), and LoadContext starts its 8m context at process start (validators/context.go L100). Since GPUReadinessTimeout is also 8m, an exhausted poll returns at ~processStart+8m — after the Job deadline at jobStart+8m. With no SIGTERM handler, the process dies before printLines/EmitExtra execute, losing the enumeration and nodesValidated/nodesTotal exactly where a cordoned node narrowed coverage. Reachable in supported states: all RDMA nodes cordoned for maintenance, or a rollout slower than 8m.
Scope: the kill race predates this PR — every exhausted pollUntilStable gate already loses its timeout diagnostic this way, so the general no-margin defect should be a follow-up issue. But the every-terminal-outcome disclosure contract is new here, so the disclosure gap belongs in this PR.
Suggested fix: also emit the coverage disclosure once eagerly after the first probe observation, keeping the terminal emit. parseExtraSentinels keeps the last valid sentinel, so a clean exit's emit wins and a deadline kill still leaves the early one in the logs — no per-tick spam, no envelope changes.
bb384ad to
d57b74d
Compare
|
@yuanchen8911 thanks — good catch on the mid-poll kill race. Addressed in The gap: on a never-ready poll, the terminal Fix (your suggested approach): Scoping notes:
Tests: added |
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 `@validators/deployment/expected_resources.go`:
- Around line 1127-1134: Remove the unused rdmaFabricProbe wrapper and update
all test callers to use rdmaFabricProbeCoverage directly, adapting assertions to
its coverage result as needed. Keep production polling unchanged and do not
retain a duplicate test-only wrapper.
🪄 Autofix (Beta)
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: 8ab04663-9d4d-4cc1-9e96-bd83465c2153
📒 Files selected for processing (5)
docs/contributor/validator.mdpkg/evidence/redact/redact_test.govalidators/deployment/expected_resources.govalidators/deployment/expected_resources_rdma_test.govalidators/deployment/expected_resources_test.go
d57b74d to
03cab57
Compare
|
Follow-up in Removed the now-dead The other three CodeRabbit comments on this commit are re-anchored duplicates of the first-round items already resolved/explained:
|
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 `@docs/contributor/validator.md`:
- Around line 532-534: Update the stale rdmaFabricProbe reference in the
expected-resources documentation to rdmaFabricProbeCoverage, matching the
current production function name and preserving the surrounding explanation.
🪄 Autofix (Beta)
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: 658910a2-7af4-4fc9-a1ac-bb558ee58f6b
📒 Files selected for processing (5)
docs/contributor/validator.mdpkg/evidence/redact/redact_test.govalidators/deployment/expected_resources.govalidators/deployment/expected_resources_rdma_test.govalidators/deployment/expected_resources_test.go
rdmaFabricProbe now enumerates nodes via FindGpuNodes and discloses cordoned Mellanox RDMA nodes: they are reported as skipped (cordoned) and counted in nodesTotal, and the coverage is surfaced through EmitExtra so it survives redaction (following the NVIDIA#1951 pattern). Reuses the existing nodesValidated/nodesTotal allowlist keys, so PolicyVersion stays at v2. The two performance call sites (nccl_all_reduce_bw, inference_perf) were deliberately audited and left unchanged: they assert workload placement/sizing, not node-coverage, so cordoned-node disclosure does not apply. Fixes NVIDIA#1952 Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
03cab57 to
60280cc
Compare
|
Fixed the stale |
Summary
rdmaFabricProbenow enumerates RDMA GPU nodes viahelper.FindGpuNodesand explicitly discloses cordoned Mellanox RDMA nodes (reported asskipped (cordoned)and counted innodesTotal) instead of silently narrowing scope to the schedulable subset. Coverage is surfaced throughEmitExtraso it survives redaction.Motivation / Context
#1668/#1936fixed the same undisclosed narrowing incheck-nvidia-smi.rdmaFabricProbe(reached fromcheckExpectedResources) had the identical spuriously-narrowed-pass shape: it built its RDMA cohort fromhelper.FindSchedulableGpuNodesand printed a count over the schedulable subset only, so a cordoned RDMA-capable node was silently excluded from both the cohort and the printed count with no disclosure. This applies the#1936disclosure pattern tordmaFabricProbeand completes the audit called for in the issue.Fixes: #1952
Related: #1936, #1668, #1951
Type of Change
Component(s) Affected
pkg/validator)docs/,examples/)Implementation Notes
rdmaFabricProbenow enumerates every RDMA GPU node viahelper.FindGpuNodes, validates only the schedulable subset, and discloses cordoned nodes asskipped (cordoned)while counting them innodesTotal. The coverage is emitted viaEmitExtra(nodesValidated/nodesTotal) so it survives redaction, following the#1951pattern.nodesValidated/nodesTotalallowlist keys, so no new redaction-allowlist entries are introduced andPolicyVersionstays atv2.nccl_all_reduce_bwandinference_perfalso callhelper.FindSchedulableGpuNodes, but they were audited and intentionally not modified — they assert workload placement/sizing, not node-coverage, so cordoned-node disclosure does not apply to them.Testing
go test -race(pass) on both affected packages.validators/deployment/expected_resources_test.gocover the cordoned-RDMA-node disclosure path (skipped (cordoned)+nodesTotalaccounting);pkg/evidence/redact/redact_test.goconfirms the emitted coverage keys survive redaction.make qualifyintentionally not run for this validator + docs change (e2e disproportionately heavy); the CI gate will run it.Risk Assessment
Rollout notes: No schema or PolicyVersion change (reuses existing v2 allowlist keys); backwards compatible. N/A migration.
Checklist
make testwith-race)make lint)git commit -S)