diff --git a/docs/contributor/validator.md b/docs/contributor/validator.md index 76de6b5de..8390d2da3 100644 --- a/docs/contributor/validator.md +++ b/docs/contributor/validator.md @@ -525,16 +525,52 @@ tagged with its cordon state, and: `RESULT:` prefix makes the coverage figure visible during a live `aicr validate` run regardless of redaction, but it is not guaranteed to survive into the artifact a downstream consumer - verifies by default. See #1951 for carrying this kind of outcome - data in a structured field that survives redaction instead. - -This pattern is not yet applied everywhere it could be. Cluster-aggregate -checks that assert on an operator's aggregate status -(`gpu-operator-health`) are unaffected — DaemonSet operands ignore -cordons — but `expected-resources`' `rdmaFabricProbe` is itself -node-scoped (it calls `helper.FindSchedulableGpuNodes` to build its -RDMA-capable cohort) and has the same undisclosed narrowing; it has not -been updated to this pattern. See #1952. + verifies by default. That is why the same counts are ALSO emitted + through `validators.EmitExtra` (#1951) — see **Structured coverage + survives redaction** below. + +`expected-resources`' `rdmaFabricProbeCoverage` is itself node-scoped and now +follows this pattern too (#1952). It enumerates every GPU node via +`helper.FindGpuNodes`, validates only the schedulable Mellanox +RDMA-capable cohort for uniform allocatable fabric, but discloses each +cordoned RDMA-capable node explicitly (`: skipped (cordoned)`), +counts it in `nodesTotal`, and never narrows the printed total. Because +the probe is re-run on every poll iteration +(`verifyRDMAFabricReady`/`pollUntilStable`), the stdout +enumeration/`RESULT:` line is printed **exactly once at the settled +terminal outcome** (ready or fail-closed), never per tick. The structured +Extra is emitted twice: an **eager floor** on the first observation that +enumerates any RDMA-candidate node (with `nodesValidated=0` — nothing is +certified mid-poll) and again at the terminal outcome. +`parseExtraSentinels` keeps the last valid sentinel, so the terminal emit +wins on a clean exit; the floor exists only so a cordoned-node narrowing +still reaches the signed bundle if the Job's `activeDeadlineSeconds` +SIGKILLs the process at the no-margin poll budget before the terminal emit +runs (#1952). The gate stays fail-closed: "could not observe the fabric" +reports `0` validated and never reads as ready. + +Unlike `check-nvidia-smi`, the RDMA gate never *skips* — it either +certifies the cohort or fails closed — so it mints no `skipReason` +enum. Its coverage rides the existing `nodesValidated`/`nodesTotal` +allowlist keys unchanged (see below), so the redaction +`PolicyVersion` stays `v2`. + +Cluster-aggregate checks that assert on an operator's aggregate status +(`gpu-operator-health`) remain unaffected — DaemonSet operands ignore +cordons. + +**Structured coverage survives redaction.** The `RESULT:` stdout line +is echoed to the live CLI but is stripped from a signed bundle by the +default (`minimal`) redaction policy (`pkg/evidence/redact`), so both +`check-nvidia-smi` and the RDMA gate ALSO emit the coverage through +`validators.EmitExtra` as low-cardinality counts +(`nodesValidated`/`nodesTotal`) or a closed-set `skipReason` code. Those +keys are the only ones that clear the fail-closed `ctrfExtraAllowlist` +(a value that structurally looks like a node name or IP is dropped even +under an allowed key), so a signed bundle records reduced coverage — +e.g. a cordoned RDMA node narrowing the fabric cohort — without shipping +any operator-identifying text. Node names appear only in the redacted +stdout enumeration, never in the Extra channel. See #1951/#1952. For *deliberate*, durable exclusion of a node from GPU service (as opposed to transient cordon-for-maintenance), use the GPU Operator's diff --git a/pkg/evidence/redact/redact_test.go b/pkg/evidence/redact/redact_test.go index fd1148d10..f72fc2aa7 100644 --- a/pkg/evidence/redact/redact_test.go +++ b/pkg/evidence/redact/redact_test.go @@ -419,6 +419,15 @@ func TestCTRFAllowlistsExtra(t *testing.T) { in: map[string]string{"nodesValidated": "1", "nodesTotal": "2", "podName": "nvidia-smi-verify-ip-10-0-0-5"}, want: map[string]string{"nodesValidated": "1", "nodesTotal": "2"}, }, + { + // The RDMA fabric gate (#1952) reuses these same count keys to + // disclose a cordoned RDMA node narrowing its cohort (validated < total). + // No new key or skipReason is minted, so the allowlist is unchanged and + // the coverage survives redaction into the signed bundle verbatim. + name: "rdma cordoned-narrowed coverage survives", + in: map[string]string{"nodesValidated": "1", "nodesTotal": "2"}, + want: map[string]string{"nodesValidated": "1", "nodesTotal": "2"}, + }, { name: "valid skip reason enum survives", in: map[string]string{"skipReason": "no-gpu-nodes"}, diff --git a/validators/deployment/expected_resources.go b/validators/deployment/expected_resources.go index 219741511..eba0f73e9 100644 --- a/validators/deployment/expected_resources.go +++ b/validators/deployment/expected_resources.go @@ -20,6 +20,7 @@ import ( "fmt" "log/slog" "regexp" + "strconv" "strings" "time" @@ -971,43 +972,183 @@ func recipeDeclaresRDMAFabric(ref recipe.ComponentRef) bool { // transient, self-healing partial rollout, mirroring the DRA kubelet-plugin and // Nodewright "stable ≥window" treatment above. // -// Why *this* node set: the probe scopes to schedulable GPU nodes that carry the +// Why *this* node set: the gate validates schedulable GPU nodes that carry the // NicClusterPolicy's own nodeAffinity label (helper.PCIMellanoxPresentLabel) — -// exactly the cohort the fabric can land on and the NCCL check runs on. A -// cordoned/draining node, or a GPU node in a non-RDMA (non-Mellanox) pool, never advertises the -// resource; including it would wedge the gate on a node the workload excludes. +// exactly the cohort the fabric can land on and the NCCL check runs on. A GPU +// node in a non-RDMA (non-Mellanox) pool never advertises the resource; including +// it would wedge the gate on a node the workload excludes. +// +// Cordoned RDMA-capable nodes: like check-nvidia-smi (#1668/#1936), a cordoned +// Mellanox RDMA GPU node is excluded from the *validated* cohort (the NCCL +// workload will not land on it) but is NOT silently dropped. It is enumerated via +// helper.FindGpuNodes, disclosed explicitly as "skipped (cordoned)" in stdout, +// counted in nodesTotal, and the coverage is emitted through validators.EmitExtra +// so it survives the default redaction policy into the signed bundle (#1951/#1952) — +// a cordoned node narrowing the fabric cohort can no longer hide behind a +// stdout-only line the publisher strips. func verifyRDMAFabricReady(ctx *validators.Context) error { - var nodeCount int - return pollUntilStable(ctx, + // Production emit seam: publish the structured coverage as an EmitExtra + // sentinel. verifyRDMAFabricReadyEmit injects it so tests can record the eager + // floor and terminal disclosures without capturing the EmitExtra stdout + // transport (which lives in the validators package). + return verifyRDMAFabricReadyEmit(ctx, func(validated, total int) { + emitExtraOrWarn(rdmaFabricCoverageExtra(validated, total)) + }) +} + +// verifyRDMAFabricReadyEmit is verifyRDMAFabricReady with the structured +// coverage emit injected. See verifyRDMAFabricReady for the gate contract. +func verifyRDMAFabricReadyEmit(ctx *validators.Context, emitCoverage func(validated, total int)) error { + var coverage rdmaFabricCoverage + // emittedEarly gates the eager disclosure floor to exactly one emit. + var emittedEarly bool + // onStable is nil: the success line and the *terminal* coverage disclosure + // are printed once at the single seam below (rdmaFabricProbeCoverage runs every + // poll iteration, so emitting the human enumeration there would repeat it on + // each tick — the settled disclosure must land exactly once, at the final + // outcome). + err := pollUntilStable(ctx, fmt.Sprintf("RDMA shared-device fabric (%s) across RDMA GPU nodes", helper.AKSRdmaSharedResource), func() error { - count, probeErr := rdmaFabricProbe(ctx) - nodeCount = count + cov, probeErr := rdmaFabricProbeCoverage(ctx) + coverage = cov + // Eager disclosure floor: emit the structured coverage once, on the + // first observation that actually enumerated an RDMA-candidate node, + // so a cordoned node narrowing the cohort survives even if the Job's + // activeDeadlineSeconds SIGKILLs the process mid-poll before the + // terminal emit runs. The catalog timeout feeds both the Job deadline + // and this poll budget with no margin (pkg/validator/v1/job_plan.go), + // so an exhausted never-ready poll (every RDMA node cordoned for + // maintenance, or a rollout slower than the budget) can be killed at + // the deadline with no terminal emit. parseExtraSentinels keeps the + // LAST valid sentinel, so a clean exit's terminal emit wins and a + // deadline kill leaves this floor as the disclosure of record. + // validated=0: nothing is certified mid-poll. Only the structured + // Extra is emitted eagerly (not the stdout enumeration) — the Extra is + // the piece that survives redaction into the signed bundle (#1951/ + // #1952), and duplicating stdout would spam divergent counts. The + // broader no-margin kill race predates this gate and is tracked + // separately; this closes only the gate's own every-terminal-outcome + // coverage contract. + if !emittedEarly && cov.total() > 0 { + emittedEarly = true + emitCoverage(0, cov.total()) + } return probeErr }, - func() { - fmt.Printf(" RDMA fabric (%s): allocatable (uniform) on all %d RDMA GPU node(s) (stable ≥%s)\n", - helper.AKSRdmaSharedResource, nodeCount, gpuReadinessStabilityWindow) - }) + nil) + + // Single terminal disclosure — printed/emitted exactly once after the poll + // settles, on BOTH the ready and the fail-closed path, reflecting the final + // observation. validated is the schedulable cohort size only when the gate + // certified it uniform+ready; a fail-closed exit (transient List error, no + // cohort observed, partial rollout, skew, or timeout) reports 0 validated so + // a narrowed-scope failure is never conflated with a full pass. + validated := 0 + if err == nil { + validated = coverage.schedulable + } + printLines(coverage.enumerationLines()...) + printLines(coverage.coverageLine(validated)) + // nodesValidated/nodesTotal are reused verbatim from the existing + // ctrfExtraAllowlist (see pkg/evidence/redact): their semantics fit exactly — + // validated = schedulable RDMA nodes with uniform allocatable fabric, total = + // all RDMA-candidate nodes incl cordoned. No new key or skipReason enum is + // minted (the RDMA gate never "skips" — it fails closed), so the redaction + // PolicyVersion stays v2. + emitCoverage(validated, coverage.total()) + + if err == nil { + fmt.Printf(" RDMA fabric (%s): allocatable (uniform) on all %d schedulable RDMA GPU node(s) (stable ≥%s)\n", + helper.AKSRdmaSharedResource, coverage.schedulable, gpuReadinessStabilityWindow) + } + return err } -// rdmaFabricProbe does one readiness pass over the Mellanox RDMA-capable GPU cohort: -// schedulable GPU nodes (via helper.FindSchedulableGpuNodes — cordoned nodes and -// nodes not yet advertising nvidia.com/gpu are excluded) that also carry the -// NicClusterPolicy nodeAffinity label helper.PCIMellanoxPresentLabel. It returns -// nil — plus the cohort size — only when every such node advertises +// rdmaFabricCoverage partitions the Mellanox RDMA-capable GPU nodes the fabric +// gate discloses: the schedulable nodes it actually validates and the cordoned +// RDMA-capable nodes it must reveal (never silently omit from the total). It +// exists so the disclosure text and the coverage counts are a pure, independently +// testable function of the partition rather than interleaved fmt.Printf calls — +// the #1668/#1936 node-scope disclosure pattern applied to the RDMA gate (#1952). +type rdmaFabricCoverage struct { + schedulable int // schedulable Mellanox RDMA-capable GPU nodes in the gated cohort + cordoned []string // cordoned Mellanox RDMA-capable GPU nodes: excluded from the cohort but disclosed +} + +// total is every RDMA-candidate node the gate saw — the schedulable cohort plus +// the cordoned nodes it excluded but must still count (nodesTotal). +func (c rdmaFabricCoverage) total() int { return c.schedulable + len(c.cordoned) } + +// enumerationLines renders the RDMA-candidate listing: the total/schedulable/ +// cordoned counts, and each cordoned node explicitly marked "skipped (cordoned)" +// rather than omitted from the total. Node names appear ONLY here (stdout), +// never in the structured Extra. +func (c rdmaFabricCoverage) enumerationLines() []string { + total := c.total() + if total == 0 { + return []string{"Found 0 Mellanox RDMA-capable GPU node(s)."} + } + lines := make([]string, 0, 1+len(c.cordoned)) + lines = append(lines, fmt.Sprintf( + "Found %d Mellanox RDMA-capable GPU node(s), %d schedulable, %d cordoned:", + total, c.schedulable, len(c.cordoned))) + for _, name := range c.cordoned { + lines = append(lines, fmt.Sprintf(" %s: skipped (cordoned)", name)) + } + return lines +} + +// coverageLine renders the nodesValidated disclosure for the RDMA gate. The +// "RESULT: " prefix is the validator runtime's convention (pkg/validator/ +// validator.go resultSummaryPrefix) for echoing a stdout line into live CLI +// output; it is not guaranteed to survive redaction, which is why the same +// counts are also emitted structurally via EmitExtra. +func (c rdmaFabricCoverage) coverageLine(validated int) string { + if len(c.cordoned) == 0 { + return fmt.Sprintf("RESULT: nodesValidated: %d/%d", validated, c.total()) + } + return fmt.Sprintf("RESULT: nodesValidated: %d/%d (%d cordoned, skipped)", + validated, c.total(), len(c.cordoned)) +} + +// rdmaFabricCoverageExtra builds the structured coverage disclosure carried +// through the redaction boundary: how many schedulable RDMA nodes the gate +// certified (validated) out of every RDMA-candidate node incl. cordoned (total). +// Values are counts only — never node names or IPs (those live in the stdout +// enumeration lines). The keys mirror check-nvidia-smi's coverage Extra and the +// existing ctrfExtraAllowlist entries. +func rdmaFabricCoverageExtra(validated, total int) map[string]string { + return map[string]string{ + "nodesValidated": strconv.Itoa(validated), + "nodesTotal": strconv.Itoa(total), + } +} + +// rdmaFabricProbeCoverage does one readiness pass over the Mellanox RDMA-capable +// GPU nodes. It enumerates every GPU node via helper.FindGpuNodes (NOT +// FindSchedulableGpuNodes) so cordoned RDMA nodes stay VISIBLE in the coverage, +// then validates only the schedulable cohort: nodes carrying the NicClusterPolicy +// nodeAffinity label helper.PCIMellanoxPresentLabel. It returns nil — plus the +// coverage partition — only when every schedulable such node advertises // helper.AKSRdmaSharedResource in a uniform, positive count. It fails closed on a -// List error and when no RDMA GPU node is observed yet: "could not observe the -// fabric" must never read as "fabric ready". The returned error rides the poll's -// dwell reset like any other unhealthy sample. -func rdmaFabricProbe(ctx *validators.Context) (int, error) { +// List error and when no schedulable RDMA GPU node is observed yet: "could not +// observe the fabric" must never read as "fabric ready". The returned error rides +// the poll's dwell reset like any other unhealthy sample; the coverage is +// returned alongside every error so the terminal disclosure can still name the +// cordoned nodes it saw. +func rdmaFabricProbeCoverage(ctx *validators.Context) (rdmaFabricCoverage, error) { listCtx, cancel := ctx.Timeout(defaults.ResourceVerificationTimeout) defer cancel() - gpuNodes, err := helper.FindSchedulableGpuNodes(listCtx, ctx.Clientset) + gpuNodes, err := helper.FindGpuNodes(listCtx, ctx.Clientset) if err != nil { - return 0, errors.Wrap(errors.ErrCodeInternal, - "failed to list nodes for the RDMA fabric readiness gate", err) + // FindGpuNodes may return a coded *errors.StructuredError (ErrCodeTimeout if + // cancellation interrupts its own node scan, before this function's loop). + // PropagateOrWrap preserves that code, wrapping only a plain error with + // ErrCodeInternal + gate context. + return rdmaFabricCoverage{}, errors.PropagateOrWrap(err, errors.ErrCodeInternal, + "failed to list nodes for the RDMA fabric readiness gate") } fabric := corev1.ResourceName(helper.AKSRdmaSharedResource) @@ -1016,28 +1157,45 @@ func rdmaFabricProbe(ctx *validators.Context) (int, error) { count int64 } var cohort []rdmaNode + var coverage rdmaFabricCoverage for i := range gpuNodes { // Honor cancellation while walking a potentially large node list, per // repo CLAUDE.md "Always check ctx.Done() in long-running operations". select { case <-listCtx.Done(): - return 0, errors.Wrap(errors.ErrCodeTimeout, + // Return the coverage accumulated so far, not an empty partition: + // the function's contract (and every other error path below) hands + // back the cordoned nodes already seen so the terminal disclosure can + // still name them. Set schedulable from the cohort scanned before the + // cancellation so a partially-walked cohort count is not lost. + coverage.schedulable = len(cohort) + return coverage, errors.Wrap(errors.ErrCodeTimeout, "canceled while scanning nodes for the RDMA fabric readiness gate", listCtx.Err()) default: } - node := &gpuNodes[i] + node := &gpuNodes[i].Node + // Only Mellanox RDMA-capable GPU nodes are fabric candidates; a non-RDMA + // GPU node never advertises the shared resource. if node.Labels[helper.PCIMellanoxPresentLabel] != "true" { continue } + // A cordoned RDMA-capable node is excluded from the validated cohort (the + // NCCL workload will not land on it) but is disclosed, not dropped — the + // spuriously-narrowed pass #1668/#1936 fixed, applied here (#1952). + if gpuNodes[i].Cordoned { + coverage.cordoned = append(coverage.cordoned, node.Name) + continue + } var count int64 if q, ok := node.Status.Allocatable[fabric]; ok { count = q.Value() } cohort = append(cohort, rdmaNode{name: node.Name, count: count}) } + coverage.schedulable = len(cohort) if len(cohort) == 0 { - return 0, errors.New(errors.ErrCodeNotFound, + return coverage, errors.New(errors.ErrCodeNotFound, fmt.Sprintf("RDMA fabric gate: no schedulable Mellanox RDMA-capable GPU nodes observed yet (label %s=true)", helper.PCIMellanoxPresentLabel)) } @@ -1051,7 +1209,7 @@ func rdmaFabricProbe(ctx *validators.Context) (int, error) { } } if len(notReady) > 0 { - return len(cohort), errors.New(errors.ErrCodeInternal, + return coverage, errors.New(errors.ErrCodeInternal, fmt.Sprintf("%s not yet allocatable on %d of %d RDMA GPU node(s): %s "+ "(network operator MOFED / rdma-shared-device-plugin still rolling out)", helper.AKSRdmaSharedResource, len(notReady), len(cohort), formatNames(notReady))) @@ -1068,11 +1226,11 @@ func rdmaFabricProbe(ctx *validators.Context) (int, error) { } } if len(skew) > 0 { - return len(cohort), errors.New(errors.ErrCodeInternal, + return coverage, errors.New(errors.ErrCodeInternal, fmt.Sprintf("%s allocatable count is non-uniform across %d RDMA GPU node(s) (want all == %d): %s", helper.AKSRdmaSharedResource, len(cohort), want, formatNames(skew))) } - return len(cohort), nil + return coverage, nil } func formatNames(names []string) string { diff --git a/validators/deployment/expected_resources_rdma_test.go b/validators/deployment/expected_resources_rdma_test.go index 6afdd7b80..62fcb510a 100644 --- a/validators/deployment/expected_resources_rdma_test.go +++ b/validators/deployment/expected_resources_rdma_test.go @@ -21,6 +21,7 @@ import ( "sync/atomic" "testing" + "github.com/NVIDIA/aicr/pkg/errors" "github.com/NVIDIA/aicr/pkg/recipe" "github.com/NVIDIA/aicr/validators" "github.com/NVIDIA/aicr/validators/helper" @@ -202,6 +203,82 @@ func TestVerifyRDMAFabricReady_Poll(t *testing.T) { } } +// TestVerifyRDMAFabricReady_EagerDisclosureFloor locks the every-terminal-outcome +// coverage contract against the mid-poll SIGKILL race: the catalog timeout feeds +// both the Job's activeDeadlineSeconds and this poll's budget with no margin, so +// a never-ready poll can be killed at the deadline before the terminal emit runs. +// The eager floor must have already emitted the structured coverage — including +// the cordoned node that narrowed the cohort — on the first observation, with +// validated=0 (nothing is certified mid-poll). parseExtraSentinels keeps the last +// valid sentinel, so on a clean exit the terminal emit overwrites the floor. +func TestVerifyRDMAFabricReady_EagerDisclosureFloor(t *testing.T) { + t.Parallel() + + type emitCall struct{ validated, total int } + + tests := []struct { + name string + schedRDMA int64 // allocatable fabric on the schedulable node (-1 => absent) + wantErr bool + wantTerminalV int // validated on the terminal (settled) emit + }{ + { + // The schedulable node's fabric never appears → the gate never + // certifies and times out. The eager floor must still have disclosed + // the 2-node total (incl. the cordoned node) so a deadline kill leaves + // the coverage in the logs instead of nothing. + name: "never ready fails closed but floor disclosed the cordoned total", + schedRDMA: -1, + wantErr: true, + wantTerminalV: 0, + }, + { + // Fabric present+uniform → the gate certifies. The eager floor emits + // validated=0 first; the terminal emit reflects the certified cohort. + name: "ready certifies after floor", + schedRDMA: 1000, + wantErr: false, + wantTerminalV: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + clientset := k8sfake.NewClientset( + rdmaGPUNode("rdma-gpu-0", 8, tt.schedRDMA), // schedulable RDMA node + cordon(rdmaGPUNode("rdma-drain-0", 8, -1)), // cordoned RDMA node → disclosed, not dropped + ) + ctx := &validators.Context{Ctx: context.Background(), Clientset: clientset} + + // pollUntilStable runs the probe synchronously in this goroutine, so + // the injected emit is never called concurrently — no lock needed. + var calls []emitCall + err := verifyRDMAFabricReadyEmit(ctx, func(validated, total int) { + calls = append(calls, emitCall{validated, total}) + }) + + if (err != nil) != tt.wantErr { + t.Fatalf("verifyRDMAFabricReadyEmit() error = %v, wantErr %v", err, tt.wantErr) + } + if len(calls) < 2 { + t.Fatalf("expected an eager floor emit AND a terminal emit, got %d: %+v", len(calls), calls) + } + // The FIRST emit is the eager floor: validated=0 (nothing certified + // mid-poll) and total=2 (the schedulable node + the disclosed cordoned + // node). This is the sentinel a deadline SIGKILL would leave behind. + if want := (emitCall{validated: 0, total: 2}); calls[0] != want { + t.Errorf("eager floor emit = %+v, want %+v (cordoned node disclosed before the terminal emit)", calls[0], want) + } + // The terminal emit reflects the settled outcome. + if want := (emitCall{validated: tt.wantTerminalV, total: 2}); calls[len(calls)-1] != want { + t.Errorf("terminal emit = %+v, want %+v", calls[len(calls)-1], want) + } + }) + } +} + // TestRDMAFabricProbe_FailsClosedOnListError proves a node-list failure is // surfaced (not read as "fabric ready"), so the poll fails closed and retries. func TestRDMAFabricProbe_FailsClosedOnListError(t *testing.T) { @@ -213,15 +290,23 @@ func TestRDMAFabricProbe_FailsClosedOnListError(t *testing.T) { }) ctx := &validators.Context{Ctx: context.Background(), Clientset: clientset} - count, err := rdmaFabricProbe(ctx) + cov, err := rdmaFabricProbeCoverage(ctx) if err == nil { t.Fatal("expected an error when listing nodes fails, got nil (must fail closed)") } - if count != 0 { - t.Fatalf("expected 0 nodes on list error, got %d", count) + if cov.schedulable != 0 { + t.Fatalf("expected 0 nodes on list error, got %d", cov.schedulable) } - if !strings.Contains(err.Error(), "failed to list nodes for the RDMA fabric readiness gate") { - t.Fatalf("unexpected error: %v", err) + // FindGpuNodes' error path now flows through errors.PropagateOrWrap: a plain + // List failure carries no code, so it is wrapped ErrCodeInternal (a coded + // inner error — e.g. ErrCodeTimeout from a canceled node scan — would instead + // propagate unchanged). Assert the propagated code, not the removed + // gate-context message, so the fail-closed contract is pinned to the code. + if !stderrors.Is(err, errors.New(errors.ErrCodeInternal, "")) { + t.Fatalf("expected ErrCodeInternal on plain list failure, got %v", err) + } + if !strings.Contains(err.Error(), "failed to list nodes") { + t.Fatalf("expected the underlying list-failure context, got %v", err) } } @@ -238,12 +323,12 @@ func TestRDMAFabricProbe_FailsClosedWithoutRDMANodes(t *testing.T) { ) ctx := &validators.Context{Ctx: context.Background(), Clientset: clientset} - count, err := rdmaFabricProbe(ctx) + cov, err := rdmaFabricProbeCoverage(ctx) if err == nil { t.Fatal("expected an error when no RDMA GPU nodes are present, got nil (must fail closed)") } - if count != 0 { - t.Fatalf("expected 0 cohort nodes, got %d", count) + if cov.schedulable != 0 { + t.Fatalf("expected 0 cohort nodes, got %d", cov.schedulable) } if !strings.Contains(err.Error(), "no schedulable Mellanox RDMA-capable GPU nodes observed yet") { t.Fatalf("unexpected error: %v", err) @@ -267,12 +352,12 @@ func TestRDMAFabricProbe_ExcludesCordonedNonRDMAAndCPU(t *testing.T) { ) ctx := &validators.Context{Ctx: context.Background(), Clientset: clientset} - count, err := rdmaFabricProbe(ctx) + cov, err := rdmaFabricProbeCoverage(ctx) if err != nil { - t.Fatalf("rdmaFabricProbe() error = %v, want nil (only the schedulable RDMA GPU node is required to carry the fabric)", err) + t.Fatalf("rdmaFabricProbeCoverage() error = %v, want nil (only the schedulable RDMA GPU node is required to carry the fabric)", err) } - if count != 1 { - t.Fatalf("rdmaFabricProbe() cohort size = %d, want 1 (cordoned/non-RDMA/zero-GPU/CPU excluded)", count) + if cov.schedulable != 1 { + t.Fatalf("rdmaFabricProbeCoverage() cohort size = %d, want 1 (cordoned/non-RDMA/zero-GPU/CPU excluded)", cov.schedulable) } } @@ -288,12 +373,12 @@ func TestRDMAFabricProbe_NonUniformCountFails(t *testing.T) { ) ctx := &validators.Context{Ctx: context.Background(), Clientset: clientset} - count, err := rdmaFabricProbe(ctx) + cov, err := rdmaFabricProbeCoverage(ctx) if err == nil { t.Fatal("expected an error on non-uniform fabric counts, got nil") } - if count != 2 { - t.Fatalf("expected cohort size 2, got %d", count) + if cov.schedulable != 2 { + t.Fatalf("expected cohort size 2, got %d", cov.schedulable) } if !strings.Contains(err.Error(), "non-uniform") || !strings.Contains(err.Error(), "rdma-gpu-1=500") { t.Fatalf("unexpected error: %v", err) @@ -311,12 +396,12 @@ func TestRDMAFabricProbe_PassesWhenUniform(t *testing.T) { ) ctx := &validators.Context{Ctx: context.Background(), Clientset: clientset} - count, err := rdmaFabricProbe(ctx) + cov, err := rdmaFabricProbeCoverage(ctx) if err != nil { - t.Fatalf("rdmaFabricProbe() error = %v, want nil (fabric uniform on all RDMA GPU nodes)", err) + t.Fatalf("rdmaFabricProbeCoverage() error = %v, want nil (fabric uniform on all RDMA GPU nodes)", err) } - if count != 2 { - t.Fatalf("rdmaFabricProbe() cohort size = %d, want 2", count) + if cov.schedulable != 2 { + t.Fatalf("rdmaFabricProbeCoverage() cohort size = %d, want 2", cov.schedulable) } } diff --git a/validators/deployment/expected_resources_test.go b/validators/deployment/expected_resources_test.go index 43003539a..15d7fc346 100644 --- a/validators/deployment/expected_resources_test.go +++ b/validators/deployment/expected_resources_test.go @@ -1354,3 +1354,175 @@ func nodewrightWithStatus(name, status string) *unstructured.Unstructured { }, } } + +// TestRDMAFabricCoverage_Disclosure exercises the headline behavior of #1952 as a +// pure function of the partition: a cordoned Mellanox RDMA node must be listed +// "skipped (cordoned)", counted in nodesTotal, and never omitted — the same +// spuriously-narrowed-pass guard check-nvidia-smi got in #1668/#1936, applied to +// the RDMA fabric gate. It also pins the two zero-cordoned/zero-total phrasings. +func TestRDMAFabricCoverage_Disclosure(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + cov rdmaFabricCoverage + validated int + wantTotal int + wantEnumeration []string + wantCoverageLine string + }{ + { + name: "cordoned RDMA node is disclosed and counted", + cov: rdmaFabricCoverage{schedulable: 1, cordoned: []string{"rdma-drain-0"}}, + validated: 1, + wantTotal: 2, + wantEnumeration: []string{ + "Found 2 Mellanox RDMA-capable GPU node(s), 1 schedulable, 1 cordoned:", + " rdma-drain-0: skipped (cordoned)", + }, + wantCoverageLine: "RESULT: nodesValidated: 1/2 (1 cordoned, skipped)", + }, + { + name: "fail-closed exit reports zero validated but still counts cordoned", + cov: rdmaFabricCoverage{schedulable: 2, cordoned: []string{"rdma-drain-0", "rdma-drain-1"}}, + validated: 0, + wantTotal: 4, + wantEnumeration: []string{ + "Found 4 Mellanox RDMA-capable GPU node(s), 2 schedulable, 2 cordoned:", + " rdma-drain-0: skipped (cordoned)", + " rdma-drain-1: skipped (cordoned)", + }, + wantCoverageLine: "RESULT: nodesValidated: 0/4 (2 cordoned, skipped)", + }, + { + name: "no cordoned nodes omits the parenthetical", + cov: rdmaFabricCoverage{schedulable: 3}, + validated: 3, + wantTotal: 3, + wantEnumeration: []string{"Found 3 Mellanox RDMA-capable GPU node(s), 3 schedulable, 0 cordoned:"}, + wantCoverageLine: "RESULT: nodesValidated: 3/3", + }, + { + name: "zero total nodes gets a plain sentence", + cov: rdmaFabricCoverage{}, + validated: 0, + wantTotal: 0, + wantEnumeration: []string{"Found 0 Mellanox RDMA-capable GPU node(s)."}, + wantCoverageLine: "RESULT: nodesValidated: 0/0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := tt.cov.total(); got != tt.wantTotal { + t.Errorf("total() = %d, want %d", got, tt.wantTotal) + } + gotEnum := tt.cov.enumerationLines() + if len(gotEnum) != len(tt.wantEnumeration) { + t.Fatalf("enumerationLines() = %v, want %v", gotEnum, tt.wantEnumeration) + } + for i, want := range tt.wantEnumeration { + if gotEnum[i] != want { + t.Errorf("enumerationLines()[%d] = %q, want %q", i, gotEnum[i], want) + } + } + if got := tt.cov.coverageLine(tt.validated); got != tt.wantCoverageLine { + t.Errorf("coverageLine(%d) = %q, want %q", tt.validated, got, tt.wantCoverageLine) + } + }) + } +} + +// TestRDMAFabricCoverageExtra proves the structured coverage disclosure carries +// exactly the two allowlisted count keys (nodesValidated/nodesTotal) as decimal +// strings and nothing else — no node names or IPs leak into the Extra channel. +func TestRDMAFabricCoverageExtra(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + validated int + total int + wantValidated string + wantTotal string + }{ + {"full cohort, one cordoned excluded", 1, 2, "1", "2"}, + {"uniform cohort no cordoned", 2, 2, "2", "2"}, + {"fail-closed zero validated", 0, 3, "0", "3"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + extra := rdmaFabricCoverageExtra(tt.validated, tt.total) + if extra["nodesValidated"] != tt.wantValidated { + t.Errorf("nodesValidated = %q, want %q", extra["nodesValidated"], tt.wantValidated) + } + if extra["nodesTotal"] != tt.wantTotal { + t.Errorf("nodesTotal = %q, want %q", extra["nodesTotal"], tt.wantTotal) + } + if len(extra) != 2 { + t.Errorf("coverage extra must carry exactly the two count keys, got %v", extra) + } + }) + } +} + +// 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) + } +}