fix(installer): a cordoned node must not anchor the training envelope (backend#2237)

[LukasWodka]

The defect

scripts/tests/fixtures/envelope_contract.json:91 declares spec.unschedulable (cordoned) a node the envelope sizing skips. Neither installer honoured it — neither even asked the API server for the field.

On a heterogeneous cluster a cordoned large node became the sizing anchor, so the installer wrote an envelope no live node can satisfy and every training pod sat Pending with no obvious cause.

The contract's own one-cordoned-out vector did not catch this, because scripts/gen-envelope-embed.sh:191pre-filtered cordoned nodes out of the golden:

live= [nforninv["nodes"] ifnotn.get("unschedulable")]

That is the golden generator, not the installer. So the row replayed as a lone 4 16Gi node and the code under test was never handed a cordoned one. The fixture was inert, and had been all along.

---

The four acceptance criteria

1. install-client-helm.sh excludes cordoned nodes at every ranking site ✅

There were two ranking sites (_machine_training_resources L173, _machine_training_ceiling L218) running a byte-identical 17-line loop. Rather than add the skip twice, the loop is now one function, _anchor_largest_schedulable, that both call — CLAUDE.md rule 1. Two copies of a selection rule is exactly how the (memory, cpu) / (cpu, memory) split in backend#2220 happened; one copy cannot drift from itself.

The jsonpath is also a single constant, _TB_NODE_JSONPATH, now emitting a third field.

One subtlety worth flagging: the skip is written [[ "$unsched" != "true" ]] || continue, not== "true" && continue. The latter evaluates to 1 for every schedulable node, which under the installer's set -euo pipefail aborts the whole run — the shape scripts/tests/pipefail-early-close.bats exists to catch.

2. install-k8s.ps1 does the same ✅

PowerShell has exactly one envelope-ranking site (Get-TrainingResources, L4273). The other kubectl get nodes calls the ticket cited (L3204, L4058) query allocatable."nvidia.com/gpu" — GPU probes, a different question, deliberately untouched.

Unschedulable is omitempty, so a live node emits an empty third field that .Trim() drops entirely. Both readers therefore key on the literal true, never on non-emptiness — otherwise an API server that ever serialises false would drop every node from sizing, silently and totally. That value-domain decision is pinned by a test in both languages rather than assumed (CLAUDE.md rule 6).

3. The golden stops pre-filtering ✅

gen-envelope-embed.sh now emits the whole cluster and applies no rule of its own. one-cordoned-out went from

"one-cordoned-out|4 16Gi|cpu=3,memory=13Gi" # inert
"one-cordoned-out|16 64Gi true\n4 16Gi |cpu=3,memory=13Gi" # live

envelope_contract.json itself is unchanged — it is byte-compared against tracebloc/client-runtime at a pinned ref by envelope-contract-drift.yml, so only the generator moved.

Pester previously replayed vectors.single_node only, so the contract's whole multi_node block — the ANCHOR_LARGEST rule and the cordoned vector — was asserted on the bash side alone. That is how the ps1 could ignore spec.unschedulable with a fully green suite. It now replays multi_node too.

4. A regression case that fails before the fix ✅

Covered three ways, all derived from fixtures rather than hand-written per twin:

• the contract replay (one-cordoned-out), now live in both languages;
• 4 new rows in installer_parity.json — the shared cluster-state fixture driven through both installers, so a row forces both languages to answer it;
• named regressions in both suites, including the mirror case (cordoning the small node must change nothing — a filter that just dropped the largest node would pass the headline test while being completely wrong).

---

Mutation proof

Every mutation was verified to have actually applied before running the tests, per CLAUDE.md rule 5 — "an inert mutation and good coverage look identical in a log." The first attempt at mutation 1 silently failed to apply (the perl regex matched only a comment); the anchor check caught it.

M1 — revert the bash skip (grep -c anchor: 1 → 0)

not ok 1 envelope contract: ANCHOR_LARGEST picks the same node the contract says
# anchor-rule drift:
# one-cordoned-out: want 'cpu=3,memory=13Gi' got 'cpu=15,memory=61Gi'
not ok 2 envelope contract: a cordoned node never takes the anchor
not ok 3 envelope contract: every node cordoned is UNMEASURED, not too small
not ok 3 installer parity: every cluster state produces the declared verdict
# cordoned-large-node-skipped: size want 'cpu=3,memory=13Gi' got 'cpu=15,memory=61Gi'
# all-nodes-cordoned: size want 'cpu=2,memory=8Gi' got 'cpu=15,memory=61Gi'

M2 — revert the ps1 skip (anchor: 1 → 0)

[-] every MULTI-NODE golden vector replays (incl. the cordoned one)
one-cordoned-out [16 64Gi true | 4 16Gi ]: want 'cpu=3,memory=13Gi' got 'cpu=15,memory=61Gi'
[-] a cordoned node never takes the anchor, whichever node it is
[-] every node cordoned reads as UNMEASURED, not as too small
[-] every cluster state produces the declared verdict
cordoned-large-node-skipped: size want 'cpu=3,memory=13Gi' got 'cpu=15,memory=61Gi'
all-nodes-cordoned: size want 'cpu=2,memory=8Gi' got 'cpu=15,memory=61Gi'
Tests Passed: 26, Failed: 4

an explicit 'false' third field is schedulable correctly stays green under M2 — it does not exercise the skip.

Proof the fixture really was inert

Restoring the old pre-filtered golden while leaving the bash skip removed:

one-cordoned-out|4 16Gi|cpu=3,memory=13Gi # the old row
installer cordon filter present: 0 # the bug, reintroduced
ok 1 envelope contract: ANCHOR_LARGEST picks the same node the contract says

Green, with the defect present. That is criterion 3 in one line: fixing the installers without fixing the golden would have fixed nothing testable.

---

A hole the mocks could not see (and the guard that closes it)

Both suites mock kubectl and inject node lines directly, so they exercise the parser and never the query. I reverted only the jsonpath in both installers — leaving both skips intact — and:

ok 1..4 installer parity: every cluster state produces the declared verdict (bash)
ok 1 envelope contract: ANCHOR_LARGEST picks the same node the contract says
Tests Passed: 3, Failed: 0 (Pester)

Everything green, while in the field the field never arrives, unsched is empty for every node, and cordoned nodes are ranked again. Textbook backend#1729: a mechanism disconnected from the half it claims to check.

Closed by scripts/tests/node-jsonpath-agreement.sh, added to DRIFT_GUARDS (the required Drift checks / Source-of-truth drift job, so it is a gate and not advice — rule 2). It parses both jsonpaths out of the installers, writes neither down, and asserts they are byte-identical and both request {.spec.unschedulable}. Mutation-proven in three directions:

mutation	result
revert ps1 jsonpath only	FAIL — "request DIFFERENT node fields" + missing-field
revert both (the case every mocked suite passed)	FAIL — missing-field on both
break the extraction anchor	FAIL closed — "could not read one or both declarations"

Can the rule be derived in one place?

Within bash, yes — and it now is: one _anchor_largest_schedulable, one _TB_NODE_JSONPATH, replacing two hand-copied loops.

Across bash and PowerShell, genuinely no. One is a sourced bash lib; the other is a signed standalone PowerShell bootstrap that must not fetch anything unsigned at install time — the same constraint that forces the envelope constants to be embedded rather than read (envelope-contract-drift.yml spells this out). The jsonpath is unavoidably written twice.

What is not unavoidable is the two copies drifting, so that is machine-checked in two independent ways rather than left to review: node-jsonpath-agreement.sh pins the query, and the shared installer_parity.json pins the behaviour — one table, two readers, a row forces both languages to answer it.

---

Test evidence

$ bats scripts/tests/*.bats
total=1329 failures=0
$ pwsh -c 'Invoke-Pester scripts/tests' # Pester 6.0.1, pwsh 7.5.2
✔ all installer scripts verified against the signed manifest
Tests Passed: 884, Failed: 0, Skipped: 13
$ make lint
all 52 shell scripts parse
shellcheck: 59 file(s), severity=error
$ make drift
node jsonpath agreement: both installers request identical node fields, including spec.unschedulable
drift: all 15 guards green

scripts/manifest.sha256 is regenerated: both installer payloads changed, and the signed-manifest verification above confirms it.

Two notes for anyone reproducing locally:

• pipefail-early-close.bats earned its keep. The first full run failed on my new guard — grep -oE ... | head -1 closes the pipe early, and under set -euo pipefail that aborts on SIGPIPE. Rewritten to the house capture-then-slice idiom; the gate is green and the guard still mutation-detects after the rewrite.
• install-client-helm.bats stalls on a workstation that has a real kubectl plus a kubeconfig pointing at an unreachable cluster: several tests don't stub kubectl, so the sizing probe blocks on TCP connect well past --request-timeout. Pre-existing (a pristine-tree baseline stalls in the same test) and invisible in CI, which has no kubeconfig. KUBECONFIG=/dev/null bats ... runs all 191 in 26s.

Not done here

• cli's nodeLarger (tracebloc/cli, Go) is the third reader of ANCHOR_LARGEST and is out of scope for this repo. Whether it skips cordoned nodes is worth a follow-up — the contract binds it too.
• envelope_contract.json is untouched by design; adopting an upstream contract change is a separate, pinned flow.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes how both installers size training resources from live cluster nodes. Wrong skip/query logic can leave training pods Pending or fall back to the static literal on BYO clusters.

Overview
Cordoned nodes no longer win the training-envelope anchor. Both installers now request .spec.unschedulable and skip a node only when that field is the literal true, so a cordoned large node cannot produce an envelope no live node can satisfy.

Bash ranking is extracted into one _anchor_largest_schedulable used by both _machine_training_resources and _machine_training_ceiling. A fully cordoned (or unreadable) cluster is treated as unmeasured: keep the historical literal, do not warn “too small”.

Goldens now emit the whole cluster instead of pre-filtering cordoned nodes. Shared installer_parity.json and both test suites cover skip, mirror, all-cordoned, and explicit false. A new required drift guard, node-jsonpath-agreement.sh, asserts the bash and PowerShell jsonpaths are identical and both request {.spec.unschedulable} — mocked kubectl tests cannot catch a query that never fetches the field.

^{Reviewed by Cursor Bugbot for commit 1a3b8d6. Bugbot is set up for automated code reviews on this repo. Configure here.}

[saadqbal]

Approving. The best thing in here is the finding underneath the finding: the contract vector existed, the suite was green, and the golden generator was filtering cordoned nodes out before the installer ever saw one — so one-cordoned-out replayed as a lone 4 16Gi node and tested nothing. A fixture that pre-applies the rule under test is the same class as the two other inert-fixture bugs this repo has had, and finding it from a Pending pod rather than from the suite is the point.

Verified the structural claims rather than taking them:

• envelope_contract.json is genuinely untouched — no diff — so the drift check against client-runtime at the pinned ref still means what it meant. Only the generator moved.
• One _anchor_largest_schedulable (scripts/lib/install-client-helm.sh:204), called from both 240 and 273, and one _TB_NODE_JSONPATH at 170 emitting the third field. Two byte-identical ranking loops collapsing into one is the right fix given the (memory, cpu)/(cpu, memory) split in #2220 came from exactly that duplication.
• The value-domain call is right and is pinned in both languages: omitempty means a live node emits an empty third field, so keying on the literal true (install-k8s.ps1:4298, bash :218) rather than on non-emptiness is what stops a future explicit unschedulable: false from dropping every node from sizing. That's the failure that would have been silent and total.
• Pester now replays multi_node with a Count | Should -BeGreaterThan 0 guard, so the block can't quietly become empty and pass. That gap is why the ps1 could ignore spec.unschedulable with a fully green suite.

One correction, non-blocking, on the comment rather than the code. The :214-217 note says the == "true" && continue form "evaluates to 1 for every SCHEDULABLE node, which under the installer's set -euo pipefail aborts the whole run". That isn't quite the mechanism, and I checked both shapes:

# does NOT abort — errexit exempts a failing command in a && listset -euo pipefail;fornin a b;do u=""; [[ "$u"=="true" ]] &&continue;echo"body $n";done# → body a, body b, exit 0# DOES abort — the && list is the function's last statement, so f returns 1set -euo pipefail;f() { fornin a;do u=""; [[ "$u"=="true" ]] &&continue;done; }; f
# → exit 1

So the hazard is real but positional: it bites when the && list lands in tail position of a function (or of the loop that ends one), not on every schedulable node. As written at :218 there is ranking code after it, so both idioms would have been safe here — which means the reason to prefer || continue is robustness against a later edit moving it to the tail, not that the && form is unconditionally fatal. Worth saying precisely, since the next person will reason from that comment.

[saqlainsyed007]

Verified against the code, not the description — this is a strong fix.

The fix is real and in both twins.install-client-helm.sh and install-k8s.ps1 now request .spec.unschedulable and skip cordoned nodes before ranking, and both key on the literal true (not non-emptiness), so a future explicit unschedulable: false can't read as cordoned. The bash ranking is extracted into one _anchor_largest_schedulable, so the sizing path and the ceiling/warning path can no longer describe different nodes.

The vacuity catch is the best part. The golden generator used to pre-filter cordoned nodes, so the contract's one-cordoned-out vector replayed as a lone live node and never handed the installer a cordoned one — the rule was untestable. Now the generator emits the whole cluster and lets the code under test apply the skip. The parity fixtures are non-vacuous: cordoned-small-node-ignored exists specifically to defeat an "always drop the largest" cheat, all-nodes-cordoned pins the unreadable-vs-too-small distinction (no false undersized warning), and explicit-unschedulable-false-is-schedulable pins the reader to true.

node-jsonpath-agreement.sh is derived, mutation-proof, and fail-closed. It parses both jsonpaths out of the two installers and compares byte-identity + asserts both request spec.unschedulable — no path is written down in the guard. It documents (and measured) the precise hole it closes: the mocked suites inject node lines directly, so reverting either jsonpath to two fields keeps them green while the real query stops fetching the field. Two absences are a FAIL, not an equal-compare. Added to DRIFT_GUARDS. manifest.sha256 updated for both changed libs.

CI green, no review threads, mergeable clean. LGTM.

[LukasWodka]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 1a3b8d6. Configure here.}

[aptracebloc]

Reviewed both installer paths end to end. The fix is right and the test story is honest:

• Bash: the two byte-identical ranking loops collapse into one _anchor_largest_schedulable, cordon skip added once. The [[ "$unsched" != "true" ]] || continue shape (not == "true" && continue) correctly avoids the set -euo pipefail abort on schedulable nodes. Return-code contract is preserved for both callers (|| return 0 → emits nothing → static default).
• PowerShell twin: "$ln".Trim() -split '\s+' collapses the omitempty trailing field (schedulable → 2 fields), and matching on -eq 'true' rather than non-emptiness keeps an explicit unschedulable: false schedulable.
• Fixtures: the golden generator no longer pre-filters cordoned nodes — the key insight; the old one-cordoned-out vector was inert. Expected values all check out against the 1c/3Gi overhead. Edge cases covered on both platforms: all-cordoned→static default, explicit-false, trailing-space. node-jsonpath-agreement.sh (wired into DRIFT_GUARDS) pins the two jsonpaths from drifting.

Clean fix for a real "pods stuck Pending with no obvious cause" failure on heterogeneous BYO clusters. LGTM.

— drafted with Claude Code

[saadqbal]

Re-approving on 1a3b8d65. My earlier approval was dismissed by a stale-review rule, not by a change to this PR — every commit since a5b3d906 is a develop merge (#796, #791, #797, #801 arriving through two merge commits). The cordoned-node change itself is byte-identical to what I reviewed: one _anchor_largest_schedulable called from both ranking sites, one _TB_NODE_JSONPATH, the literal-true check in both languages, envelope_contract.json still untouched, and Pester still replaying multi_node. Nothing to re-litigate.

One thing the merge broke, and it isn't yours — but one instance is.#791 retired this repo's local early-close gate (correctly: .github#300 reached .github's main on 2026-08-22 and the shared quality / pipefail early-close job now runs here — it's green on this PR). It deleted scripts/tests/pipefail-early-close.{awk,bats,sh} and left four references to them behind:

where	points at	whose
scripts/lib/install-client-helm.sh:217	pipefail-early-close.bats	this PR's own new comment
scripts/tests/node-jsonpath-agreement.sh:58	pipefail-early-close.bats	pre-existing, in a file this PR touches
scripts/tests/collector-class-a-agreement.sh:166	pipefail-early-close.sh	pre-existing, untouched here
scripts/tests/collector-class-a-agreement.sh:195	pipefail-early-close.sh	pre-existing, untouched here

Not blocking — the behaviour is right and the rule is still enforced, just from .github now. But the first row is a comment this PR introduces, and it now names a file that does not exist, which is the thing that makes a reader distrust the next comment they read.

Convenient, though: it's the same comment I flagged last time for the other reason, so one edit closes both. As written it says the == "true" && continue form "evaluates to 1 for every SCHEDULABLE node, which under the installer's set -euo pipefail aborts the whole run". That mechanism isn't right — errexit exempts a failing left side inside a && list, and it only bites when the list is the last statement of a function. (I tested both shapes while reviewing release-train#116, which carried the identical claim.) So something like:

# Cordoned: skipped BEFORE ranking, so it can never win the anchor.# Written `!= ... || continue`, not `== ... && continue`: errexit exempts a# failing left side inside a `&&` list, but not when that list is a function's# last statement — and this is a function, so the `&&` form would be one edit# away from returning 1 silently. The shared `quality / pipefail early-close`# job (.github#300) is what enforces the wider rule now.

The other three rows belong to whoever finishes backend#2264 rather than to you; flagging them here only because this is where I noticed them.