Uh oh!
There was an error while loading. Please reload this page.
feat(installer): refuse to install when the nodes can't see the host data dir (backend#2422) - #817
Conversation
…data dir (backend#2422) In hostpath mode every chart PV is a hostPath onto /tracebloc/<release>/..., and /tracebloc is the k3d bind mount of HOST_DATA_DIR. When that mount is not in effect, NOTHING FAILS: kubelet's DirectoryOrCreate fabricates the directory inside the node's own filesystem, the PVC Binds, the pod Runs, MySQL initialises a brand-new empty datadir and the dataset dir reads as zero rows. No event, no warning, no failed probe -- the operator sees a healthy install that has quietly stopped using their data, and on the next `cluster delete` it goes with the node. Found during the backend#2422 PV-rebinding rehearsal. It is not hypothetical on the laptops this epic targets: a HOST_DATA_DIR outside Docker Desktop's shared paths produces exactly this, as does a cluster recreated by hand without -v. WHY NOT THE OBVIOUS CHART FIX. Flipping the two data PVs to `type: Directory` so kubelet refuses does not work: spec.persistentvolumesource is IMMUTABLE after creation, so it is rejected on any release that already has PVs. Measured on a real v1.36.3+k3s1 cluster -- `helm upgrade` fails with "spec.persistentvolumesource is immutable after creation" and leaves the release in `failed`, which is then what the fleet auto-upgrade CronJob retries. That would break the next upgrade of every existing hostpath install to close a silent-data bug. So the check goes in the installer, before helm runs, where being wrong costs an error message instead of a broken upgrade. The probe writes a token under HOST_DATA_DIR and reads it back from inside every node container. Content, not presence: a mount pointed at the WRONG directory still shows a file of that name from an earlier run. Fails CLOSED. An unreadable marker, a node that cannot be exec'd, and a node list we cannot obtain all block the install -- "cannot tell" is a finding, since proceeding anyway is the exact behaviour this exists to end. Skipped in node-local mode (RFC-0003 Option C), which deliberately has no host mount. EVERY node, not just the server: AGENTS defaults to 1 and agents run kubelets, so a training pod can land on an agent -- the same @all-vs-@server trap as the cgroup v1 flag in #806. k3d's -serverlb is excluded; it is a proxy, not a kubelet, and probing it would fail every install. Both installers, because a guard in one language leaves the other half of the fleet with the silent mode -- and Windows/Docker Desktop is where the unshared- path cause is MOST likely. A twin-presence test asserts both exist AND are wired in, defined-but-never-called being the likeliest regression. Mutation-proved, 10 anchors, each reddening only the test that owns it: never refusing; presence instead of token match; empty node list failing open; server-only probe; head -1 (first node only); node-local not skipped; probe file left behind; and the three PowerShell equivalents. Two of the bats tests were found VACUOUS during this -- a bad mock made every case take the "cannot list nodes" branch, which is also non-zero, so the refusal tests passed while exercising the wrong refusal. They now assert their own message, and the mock uses globals (bash captures no closure, so the locals were unset by call time). Verified: cluster.bats 126/126, Pester install-k8s 739 passed / 0 failed (develop baseline 733/0, +6 here), assess.bats 71/71, hostpath-prep / bats-hygiene / check-style / check-facts / gen-manifest / copy-catalog / check-drift all green, shellcheck -S warning -x at develop's baseline count (2), bash -n clean, manifest regenerated. The first placement of the PowerShell half was wrong and the suite caught it: in Install-ClientHelm it fired inside unit tests that mock docker away, and one Err exit cascaded into 583 failures. It now sits at the end of New-K3dCluster, which is also the correct parity with the bash twin's cluster path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
bugbot run |
…ount-probe refusal (backend#2422) The style guard bans "workspace" in user-facing text (STYLE.md) and it caught both twins. Manifest regenerated for the copy change; `make drift` is now 18/18. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Reviewed at high effort — this is a solid twin-guard. I verified the two things a fail-closed data probe most needs to get right and both hold:
- Adopted/reused clusters are probed too.
Assert-NodesSeeHostDatasits at the end ofNew-K3dClusterand the$clusterExistsreuse branch has no earlyreturn, so it runs on reuse — matching the bash twin's deliberate placement after_handle_existing_cluster. (The healthy "nothing to do" fast-path in main does skip it, but that's the pre-existing shortcut where the mount was already proven at first install.) - The tests are mutation-proof, each refusal test asserting its specific message rather than a bare non-zero status, and the twin-presence test checks both installers define and call the guard (≥2 refs).
Not blocking-from-me, but the PR can't merge yet:
- CI is red on the style guard —
check-style.shflags the banned term "workspace" (STYLE.md → "secure environment") atinstall-k8s.ps1:766andcluster.sh:105(same sentence in both). That's the immediate blocker. - One low-severity edge (inline):
docker ps --filter name=k3d-<cluster>-is an unanchored substring, so a same-prefixed sibling cluster (k3d-tracebloc-dev-*) could be probed and cause a false refusal. Cheap to anchor; details inline on both installers. - Bugbot hasn't run yet (pending) — will re-check once it and the unit/bats jobs report.
Holding my verdict until CI is green and Bugbot is clean.
Uh oh!
There was an error while loading. Please reload this page.
# Conflicts: # scripts/manifest.sha256
…lls, drop the culture-sensitive mint (backend#2422) Addresses every finding on #817 -- two from Bugbot, one from @saqlainsyed007. 1. NODE SELECTION BY LABEL, NOT NAME (@saqlainsyed007). `name=k3d-<cluster>-` is an unanchored SUBSTRING match, so it also lists a same-prefixed sibling cluster's nodes (k3d-tracebloc-dev-server-0). If that sibling was created against a different HOST_DATA_DIR its nodes cannot see this token, so the probe would refuse THIS install while naming a node that is not ours -- a false refusal, the one failure mode a fail-closed guard most has to avoid. Fixed one step further than the review suggested: rather than anchoring a name REGEX, select on k3d's own labels. `label=k3d.cluster=<name>` is an exact value match (verified: `--filter label=k3d.cluster=tb` returns nothing for cluster `tb-copyreview`), and `k3d.role` says what each container IS -- so the load balancer is excluded because it is a `loadbalancer`, not because its name happens to end in `-serverlb`. Node names are no longer parsed at all. 2. BOUNDED DOCKER CALLS (Bugbot, Medium, reported twice). A WEDGED as opposed to stopped daemon never returns from a bare `docker`, which would freeze a headless install right at this probe with no further output -- the exact failure the guard exists to replace with a clear refusal. Now `_bounded` in bash and `Invoke-DockerCli` in PowerShell, the house patterns, both at 10s. 3. CULTURE-SENSITIVE TOKEN MINT (Bugbot, High). Replaced `[int][double]::Parse((Get-Date -UFormat %s))` with `[DateTimeOffset]::UtcNow.ToUnixTimeSeconds()` -- an integer, so nothing is parsed and no culture is involved. Worth recording precisely, because the finding does NOT reproduce on the machine I tested on: under PowerShell 7 `%s` emits a bare integer ("1787575411"), which [double]::Parse accepts in en-US, de-DE and fr-FR alike -- measured all three. But this installer declares `#Requires -Version 5.1` and is invoked via powershell.exe (see its own note at install-k8s.ps1:1896), and Windows PowerShell 5.1 emits %s WITH a fractional part. In de-DE "." is the GROUP separator, so that string either throws FormatException or parses to a wildly wrong number. Bugbot is right about the platform that matters. TESTS. The de-DE round-trip test I wrote first was VACUOUS -- it passed with the bug still in place, exactly because pwsh 7 emits no decimal. It is now a source guard asserting the mint does no culture-sensitive parsing, which is the property, is checkable here, and reddens under the mutation the round-trip could not see. That guard in turn had to strip comment lines, or it tripped on the comment that EXPLAINS the ban (it names both banned constructs) -- same reason k3s-components-agreement.sh reads the installer with comments removed. Two further test defects fixed while doing this: * The Pester mocks returned Output as an ARRAY. Invoke-BoundedProcess always builds it as ONE string ($outTask.Result + $errTask.Result), so the mocks were exercising a shape production never produces -- testing a copy of the code instead of the code. They now use the real single-string form. * Two new bats tests recorded into VARIABLES from inside `$(docker ps …)`, i.e. a command-substitution subshell, so the parent never saw them. They record into files now, like the suite's own `record` helper. Mutation-proved, 7 new anchors on top of the existing 10: name-substring filter restored (2 tests redden), `docker ps` unbounded, `docker exec` unbounded, role filter dropped so the lb is probed, and the three PowerShell equivalents (culture mint, substring filter, bare docker). Every one reddens only the tests that own it. Verified: cluster.bats 129/129, Pester install-k8s 747 passed / 0 failed / 13 skipped (develop baseline 733/0/13, +14 here), `make drift` 18/18 including check-style and gen-manifest --check, shellcheck -S warning -x back at develop's baseline count of 2 (the label refactor left `role` unused -- removed), bash -n clean, manifest regenerated. Also merges origin/develop: the only conflict was scripts/manifest.sha256, which is regenerated rather than hand-resolved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
bugbot run |
saqlainsyed007
left a comment
There was a problem hiding this comment.
Re-review of 3bcf834 — both of my inline notes are addressed, and well beyond a minimal patch:
- The unanchored-substring node filter is fixed. Node selection now uses
--filter "label=k3d.cluster=${CLUSTER_NAME}"(an exact label-value match) plus ak3d.roleserver/agent filter, replacingname=k3d-<name>-. A same-prefixed sibling cluster can no longer leak in and trigger a false refusal, and the load balancer is excluded because its role isloadbalancer, not because its name ends in-serverlb. Same fix in both installers. - Bonus hardening I didn't ask for: the docker calls are wrapped in
_bounded, so a wedged (not merely stopped) daemon surfaces a clear refusal instead of freezing a headless install. - The style-guard CI failure is resolved — the banned "workspace" wording is gone from the user-facing refusal text, and
Source-of-truth driftnow passes.
Everything I raised is closed and verified in code (threads resolved). Holding the approval only on the green gate: CI is still pending (Prereqs / Unit tests / bats) and Cursor Bugbot hasn't re-run on this head yet. I'll approve as soon as it's green and Bugbot is clean.
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
Really good PR — the fail-closed reasoning, the token-not-presence check, and the tests that each assert their own refusal message rather than just non-zero are all right. Manifest checks out (20/20) and I verified the ordering: HOST_DATA_DIR is created and validated well before create_cluster, so the write probe can't false-refuse a fresh install.
One blocker, the open Bugbot thread on install-k8s.ps1:793 — it's real. Invoke-BoundedProcess concatenates stdout and stderr into .Output (:2068), so docker stderr chatter corrupts the token compare and you get a false refusal after the cluster is already up. Details and why the suggested "first non-empty line" fix isn't enough are on that thread.
Non-blocking: :3730 enumerates the places that are correct only because Windows is hostpath-only (Invoke-LeftoverDataGuard, the unconditional local-storage disable). Assert-NodesSeeHostData is now a third and isn't named there — worth adding, since that comment is what someone adding a Windows node-local path will read.
Happy to approve as soon as the stderr one lands.
…an't forge a miss (backend#2422) @saadqbal's blocker on #817, and it is real. Invoke-BoundedProcess returns `Output = ($outTask.Result + $errTask.Result)` -- a plain string concatenation -- so any client-side docker warning lands in the same string the probe compares against the mount token. The result is a FALSE REFUSAL: the install aborts with "cannot see your data directory" on a machine where the mount is fine, after the cluster is already up. That is the single worst outcome for this guard. WHY THE OBVIOUS FIX DOES NOT WORK. The marker is written -NoNewline, so `cat` emits the token with NO trailing newline and stderr glues onto it INSIDE THE SAME LINE. "Take the first non-empty line" therefore still yields `<token>WARNING: ...`. Saqlain called this out explicitly; the three options he listed were isolating stderr in the helper, comparing a prefix, or giving the marker a trailing newline. Took the first, opt-in: Invoke-BoundedProcess and Invoke-DockerCli gain a `-StdoutOnly` switch, and both probe calls use it. Opt-in is the design, not laziness -- the merged Output is LOAD-BEARING for most callers (Get-GpuBuildFailureReason classifies a docker build by matching stderr text), so isolating globally would break the diagnosis those callers exist to produce. Only the success path is isolated; the failure/timeout paths keep their merged or synthetic text, which is pure diagnostics, and every caller checks .Code before reading .Output for a value. Same root cause, second-order, on the `docker ps` parse above it: with no separator inserted, a stdout lacking its trailing newline would glue a warning onto the LAST node's role field ("serverWARNING: ..."), dropping that node from the list so a single-node cluster falls into "Couldn't list the nodes". docker's --format does terminate its output, so it was latent -- and it goes away with the same switch. TESTED AGAINST A REAL PROCESS, not a mock of the call whose output shape IS the bug: a child writes to both streams, and the test asserts the default still merges them with no separator (`tokWARNING:chatter` -- the precise mechanism) while -StdoutOnly returns `tok` alone. A second test captures what Assert-NodesSeeHostData actually requests, so the wiring cannot silently regress to the merged form. Mutation-proved three ways, each reddening only what it should: reverting -StdoutOnly on the exec call (the original bug), neutering the switch inside the helper, and isolating ALWAYS -- that last one reddens a pre-existing GPU test, which is the proof that the opt-in design is protected rather than merely intended. BASH IS IMMUNE BY CONSTRUCTION, and the test that says so had to be corrected. `$( )` captures stdout only, so the twin's `2>/dev/null` merely keeps the terminal quiet. My first bats test claimed to guard that redirect and was VACUOUS -- removing it changed nothing capturable and the test stayed green. It now documents the real mechanism and pins the regression that CAN break this side: someone adding `2>&1` to capture diagnostics into the variable. Mutation-checked -- with `2>&1` it fails with the exact false refusal. Non-blocking review point also addressed: install-k8s.ps1's enumeration of the places that are correct only because Windows is hostpath-only now names Assert-NodesSeeHostData as the third, since that comment is what someone adding a Windows node-local path will read. Verified: cluster.bats 130/130, Pester install-k8s 749 passed / 0 failed / 13 skipped, `make drift` 18/18, shellcheck -S warning -x at develop's baseline count (2), manifest regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
bugbot run |
saqlainsyed007
left a comment
There was a problem hiding this comment.
Re-review of the new commit e23f902 ("isolate stdout in the mount probe"). From my side everything remains settled: my two earlier findings (the unanchored node filter) are still fixed, and this commit doesn't touch that path.
The new fix is a good one, and correctly scoped. Invoke-BoundedProcess merges stdout+stderr with no separator, and because the marker is written -NoNewline, a docker stderr warning would glue straight onto the token within the same line (<token>WARNING: …) — so "take the first non-empty line" wouldn't save it, and the result is a false refusal on a healthy cluster, the worst outcome for this guard. The -StdoutOnly switch is opt-in rather than global precisely because the merged output is load-bearing elsewhere (e.g. Get-GpuBuildFailureReason classifies a docker build by its stderr text), and only the success path is isolated while failure/timeout keep their diagnostics — that's the right altitude. Applied to both the docker ps and docker exec cat calls, and the bash twin was already isolated via 2>/dev/null, so the asymmetry is correct.
I'm not the blocker here: this addresses @saadqbal's standing change request, so the approve is theirs to give once they've re-reviewed, and CI is still pending. No objection from me.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
The stderr fix is right and complete — -StdoutOnly on both call sites including the docker ps one, and importantly $errTask is still awaited, so it isolates stdout without reintroducing the redirect deadlock the surrounding comments warn about. Tests in both suites. That one's settled from my side.
Both of Bugbot's new findings are real though, and I verified each:
The High on :782 is the serious one. $psi.Arguments quotes whitespace-bearing args without escaping inner quotes, so the --format value's quotes get consumed by Windows command-line parsing and docker's Go template fails to parse k3d.role — the probe then throws Couldn't list the nodes on every Windows hostpath install, after the cluster is up. Bugbot's mechanism is slightly off (one token with quotes eaten, not several tokens) and the detail matters for the fix; it's on the thread. Bash is unaffected.
The Medium on the wiring test is mine to own — I read that test two passes ago and called it adequate without counting. Delete the real call and three mentions remain (definition plus two comments), so -ge 2 holds and the guard passes with the wiring gone. The bash half only escapes this by having no comments naming the function yet. k3s-components-agreement.sh already strips comments before matching — same trick fixes both halves for good.
Still a good change; it's the Windows quoting that would bite a real operator.
…and count call sites not mentions (backend#2422) Two findings from @saadqbal's review, both confirmed and both real. He corrected Bugbot's mechanism on the first in a way that changed the fix, and owned the second. 1. HIGH -- WINDOWS ARGV ATE THE FORMAT STRING. Invoke-BoundedProcess joins the args into one command line and quotes any whitespace-bearing value as '"' + $_ + '"' with NO escaping of inner quotes. The single query --format "{{.Names}} {{.Label `"k3d.role`"}}" has both a space AND quotes, so it went out with its own quotes intact and CommandLineToArgvW toggled in and out of quoting to hand docker ONE token with the inner quotes CONSUMED: `{{.Names}} {{.Label k3d.role}}`. text/template then cannot parse k3d.role as an identifier, docker exits non-zero, $nodes stays empty, and the probe throws "Couldn't list the nodes" -- a FALSE REFUSAL on every Windows hostpath install, after the cluster is already up. Bugbot said it splits into several argv tokens; Asad measured that it does not, and that detail is the fix: one intact argument with broken quoting, not fragments. Fixed by removing the need for quotes at all: ONE QUERY PER ROLE, letting docker AND two label filters. `{{.Names}}` has no space and `label=k3d.role=server` has neither, so no argument reaches the quoting branch. It also drops the awk/PowerShell role parsing, and the load balancer is now excluded BY CONSTRUCTION -- its role is `loadbalancer`, which is simply never queried. Applied to BOTH twins even though bash was never exposed (it passes an array and never re-joins). Keeping both halves on the shape the constrained one requires is what keeps them diffable; a divergence here is a twin gap nobody notices until Windows breaks. Asad's general alternative -- escape inner quotes in the shared quoting branch -- would cover every other caller too, and is the better long-term fix. Deliberately NOT done here: it changes command-line semantics for every caller in a signed installer bootstrap, on a platform I cannot test from this machine. Filed separately rather than smuggled into a data-loss guard. Test: the quoting lives BELOW the Invoke-DockerCli mock, so no Pester case could ever reach it -- which is exactly why it shipped. The property is asserted at the mock boundary instead: no argument may contain a quote or whitespace, and both roles plus the exact cluster label must be queried. Mutation-proved by restoring the old combined format (7 tests redden). 2. MEDIUM -- THE WIRING GUARD WAS VACUOUS ON ITS OWN REGRESSION. It counted MENTIONS (`grep -c … -ge 2`), and comments naming the function keep the count up. Measured: deleting the real call left THREE mentions in install-k8s.ps1 -- definition plus two comments -- so it stayed green with the wiring gone. The mutation output in this change shows both numbers side by side: 3 mentions (old check passes) vs 1 code mention (new check fails). The bash half was sound only by luck at 2 occurrences, and would have gone vacuous the moment anyone wrote a comment naming the function -- precisely what happened on the ps1 side. So a threshold bump would paper over it; the count has to be of CALL SITES. Both halves now strip comment lines before counting, the technique k3s-components-agreement.sh already uses. Mutation-proved on both. A third gap surfaced while mutation-testing my own fix, and it was mine, not the review's: the new PER-ROLE fail-closed branch was untested. A fail-open mutation of it stayed green, because an errored query and an empty one both reach the same final error. The distinguishing case is one role answering while the other ERRORS -- we cannot tell whether there are agents to probe, so refusing is the only safe answer. Added on both sides, plus its opposite (an EMPTY agent list is legitimate on AGENTS=0 and must not be refused), which pins the branch to exit status rather than emptiness from both directions. Both mutation-proved. Verified: cluster.bats 132/132, Pester install-k8s 752 passed / 0 failed / 13 skipped (this base's develop baseline is 736/0/13; +16 here), `make drift` 18/18, check-style clean, shellcheck -S warning -x at develop's baseline count (2), bash -n clean, manifest regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
bugbot run |
LukasWodka
commented
Aug 24, 2026
Filed the general quoting fix as backend#2455 — escaping inner quotes in |
There was a problem hiding this comment.
✅ 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 9ebdef2. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Re-reviewed at the current head (9ebdef2). From my side this is fully clear:
- My two original findings (the unanchored node filter) remain fixed — the current per-role query keeps the exact
label=k3d.cluster=<name>match and excludes the load balancer by construction. - The new commit is a sound fix for a real Windows-only false refusal: PowerShell's
$psi.Argumentsre-joins args and quotes whitespace-bearing values without escaping inner quotes, so--format "{{.Names}} {{.Label "k3d.role"}}"arrived with its quotes consumed and docker's Go template failed to parse → empty node list → refusal on every Windows hostpath install. Querying one role at a time with two ANDed label filters and an unquoted{{.Names}}removes the quoted argument entirely, and fails closed per role. Good fix, and the bash twin is kept on the same shape for diffability. - CI is green (Cursor Bugbot pass), and there are no open review threads.
I'm not the blocker: @saadqbal's change request is still standing (on the prior commit e23f902, which 9ebdef2 addresses), so the approve is theirs to give once they've re-reviewed the current head. No objection from me.
saadqbal
left a comment
There was a problem hiding this comment.
Both of last round's findings are properly fixed, and I verified each rather than taking the commit message for it.
The Windows quoting is gone at the root of the problem: one query per role, --format "{{.Names}}", so no argument carries a space or a quote at all. Keeping the bash half on the same shape even though bash could safely use the quoted format — so the twins stay diffable by eye — is a good call, and the per-role split drops the role parsing and excludes the load balancer by construction. Both halves keep the total-empty guard, so "no nodes at all" still refuses.
The wiring test is genuinely non-vacuous now. I ran its own logic against call-deleted copies of both files: real = 2 non-comment mentions (pass), call deleted = 1 (fail), on each side. Reusing k3s-components-agreement.sh's comment-stripping is the right technique.
One new thing though, on the thread at cluster.sh:127: the per-role fail-closed branch can't run in the real installer. Under set -euo pipefail (install-k8s.sh:44) a failing out=$(…) exits at the assignment, so st=$? and the if (( st != 0 )) block below it are dead — the old shape's || true was what kept set -e off it. The test doesn't catch it because run suppresses set -e; I measured both call shapes and production uses the bare one. The operator gets the ERR trap's generic record instead of the refusal, and the marker is left behind.
Small fix, and the last thing from me.
…set -e (backend#2422) @saadqbal's finding, and he is right on both the mechanism and on why my test could not see it. install-k8s.sh runs under `set -euo pipefail` and shell options are global to the sourcing shell. A bare `out=$(docker ps …)` is a simple command whose status is the substitution's, so when docker errors set -e exits AT THE ASSIGNMENT -- and everything below it is dead code: the fail-closed branch AND the `rm -f` of the probe marker. The previous shape survived only because it ended in `| awk … || true`, which is exactly what kept set -e off it; my per-role rewrite dropped that without replacing it. What the operator would have got: the ERR trap's generic `_record_err` naming `docker ps` instead of the curated refusal, and the probe marker left behind in HOST_DATA_DIR. Precisely the opaque failure this guard exists to replace. Fixed with `st=0` then `out=$(…) || st=$?` -- the `||` context suppresses set -e and preserves the status. WHY THE EXISTING TESTS WERE BLIND, which is the part worth recording: `run` captures the status, and that SUPPRESSES set -e. Production calls this function BARE (create_cluster -> install-k8s.sh:272), so `run`-based tests exercise a different shape than production and pass either way. Measured against Asad's exact pre-fix shape (bare assignment + `st=$?` on the next line): the two per-role tests stay GREEN while the new test reddens. So the note I had left on those tests -- "a fail-OPEN mutation stays green (measured)" -- was covering the logic axis and claiming the reachability one. Corrected in place to say what each test does and does not cover. The new test reproduces production instead of `run`: a subshell that sets the same options and calls the function bare, with the outer `|| st=$?` on the substitution rather than inside it. It asserts three things, because the bug breaks all three -- non-zero status, the curated message actually reached, and the marker cleaned up. Checked the whole function for the same class rather than just the reported line, and pinned the result: the `printf … || error`, the `$( … || true )` exec capture, the `[[ -n "$out" ]] && nodes+=…` append (an AND-list failure does NOT trip set -e -- verified empirically, so this one is safe as written) and the `rm -f … || true` are all fine; only the per-role assignment was not. Two extra tests keep the SUCCESS path and the node-local early return honest under set -e too, since an abort on either would fail every install rather than merely skip a guard. This class was already known in this suite -- there is a #424 Bugbot test for errexit-safety on the CA resolve capture. I should have applied that existing pattern when I introduced a new command substitution; the fix is the same shape it uses. The PowerShell twin is unaffected: `throw` is not conditional on shell options. Verified: cluster.bats 135/135, Pester install-k8s 752 passed / 0 failed / 13 skipped (unchanged -- ps1 untouched this round), `make drift` 18/18, check-style clean, shellcheck -S warning -x at develop's baseline count (2), bash -n clean, manifest regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
bugbot run |
saadqbal
left a comment
There was a problem hiding this comment.
Fixed, and the response is better than the fix alone.
st=0 then || st=$? is the right shape and I verified it under set -euo pipefail with the bare call: the branch is reached, the refusal prints, and the script carries on.
The part worth calling out is the new test. It reproduces the production call shape — a subshell that sets the same options and calls the function bare, with the || st=$? outside the substitution — and asserts all three consequences: non-zero status, the curated refusal actually present in the output, and the marker cleaned up. Revert to the bare assignment and the middle one reddens, which is exactly the axis the existing run-based tests structurally cannot see. And you corrected the old test's comment to say so rather than leaving a claim standing that I'd shown was too strong.
Nothing outstanding from me. Held only on CI — ten checks still running.
There was a problem hiding this comment.
✅ 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 d7d64dd. Configure here.
LukasWodka
commented
Aug 24, 2026
Everything on my side is done and CI is now fully green — 44 pass, 0 pending, 0 fail, Bugbot clean on One mechanical thing though, @saadqbal, because I don't think it's visible from your side: your change request is still active. Your last comment reads as a clear-out, but it was submitted as so Nothing for me to do about either — it needs an explicit Approve from you (which both dismisses the change request and satisfies the required review). Flagging it only so it doesn't sit here waiting on something nobody is waiting for. |
saadqbal
left a comment
There was a problem hiding this comment.
Green, no open threads, everything addressed — approving.
Worth saying this one improved a lot under review, and in the right direction each time: the stderr fix isolated stdout rather than parsing around it, the Windows quoting was solved by removing the quotes rather than escaping them, and the set -e fix came with a test that reproduces the production call shape instead of one that only passes. Three of the four findings were cases where a guard existed but couldn't fire — which is the failure mode this whole PR is about, so it's fitting.
The twin-parity discipline is the part I'd keep: bash held to the shape PowerShell requires even where bash didn't need it, so the two stay diffable by eye.
Uh oh!
There was an error while loading. Please reload this page.
…#817 probe under the flip Merges origin/develop (#809/#817). #817 adds _verify_nodes_see_host_data (a hostpath-only host-mount probe) to both installers. Review/Bugbot fixes on top: - cluster.sh guard_leftover_data: the "a fresh install would silently adopt it" warning is hostpath-only. Under node-local (the default) a fresh install does NOT adopt host data — it's stranded — so the lead line contradicted the very next node-local line. Make the lead mode-aware (Bugbot Medium, client#456). - cluster.sh:78: flip the one _verify_nodes_see_host_data fallback #817 added after the sweep — ${TB_STORAGE_MODE:-hostpath} -> :-node-local — so the whole tree carries a single default value again. - cluster.bats: pin TB_STORAGE_MODE=hostpath in #817's 17 probe tests (they exercise the hostpath-only probe; the sourced default is now node-local, which correctly skips it) and add two leftover-guard tests for the contradiction fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…der (backend#2455) (#845) * fix(installer): escape inner quotes in Invoke-BoundedProcess arg builder (backend#2455) $psi.Arguments is one flat command line, so each arg has to survive CommandLineToArgvW re-splitting it back into argv. The old joiner wrapped whitespace-bearing args in quotes but never escaped an inner `"`, so any arg carrying BOTH a space and a quote (and even a quote with no space, which took the raw pass-through branch) reached the child with its quotes silently consumed and merged into adjacent tokens. #817 dodged this for one call site by never passing a quoted arg; this fixes the general helper. - Add ConvertTo-Win32Arg, which follows the exact CommandLineToArgvW/MSVCRT rules: escape `"` as \", double a run of backslashes before a quote (2N+1) and a trailing run before the close quote (2N), and leave a safe arg untouched. Invoke-BoundedProcess now delegates every arg to it. - Drop the fragile `^".*"$` "already-quoted, leave alone" escape hatch and its one dependent call site: Set-NodeGpuCapacity now passes $patchFile raw and lets the helper quote it (a spaced temp path was the only reason it self-quoted). Swept all ~27 call sites; the env-derived docker-login username is the other arg that can now carry a quote safely. - Replace the source-guard tests that pinned the buggy behavior with golden encodings plus a round-trip test (whitespace, embedded quote, whitespace + quote, empty string, backslashes-before-quote, trailing backslash) that re-splits via a from-spec CommandLineToArgvW parser and, on Windows, the real shell32 API — asserting each arg comes back as one original token. - Regenerate scripts/manifest.sha256 for the edited installer. Verified locally with pwsh 7.6.5 + Pester 6.1.0: install-k8s suite 765 passed / 0 failed / 14 skipped; installer-parity/install/telemetry 158/0; gen-manifest + check-drift + installer-parity bats green; manifest drift gate clean. Found by @saadqbal during client#817 review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(installer): fix the inert shell32 cross-check -- flat string[] cases (backend#2455) The Windows-only "encoder agrees with real shell32!CommandLineToArgvW" test built its cases as @((,@(...)), ...) (comma-separated), which nests each case one level: $argv iterated as an Object[]-of-Object[], so ConvertTo-Win32Arg was handed an array and threw ParameterBindingArgumentTransformationException before any comparison ran. macOS skips the block, so a local "765 passed" hid it while windows-latest went red (939 passed / 1 failed) and this cross-check -- the one that breaks the circularity of the from-spec reimplementation -- never actually verified the encoder (LukasWodka on #845). Switch to the newline-separated ,@(...) shape the round-trip test already uses so each $argv is a flat [string[]]. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(installer): make the shell32 cross-check preserve empty args (backend#2455) With the case array fixed, the Windows-only real-shell32 oracle finally ran and caught a harness bug: realArgv returned ,@($r) and the caller piped it through | Select-Object -Skip 1, which dropped a trailing empty argument -- so the empty """" case saw 0 recovered tokens instead of 1 (windows-latest: 939 passed / 1 failed). Collect into a List[string] and return via the ,$arr idiom, then assign (not pipe) and slice off argv[0] by index, which preserves empty and 0/1-element results. Verified the array logic on the empty and mid-empty cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(installer): dump real shell32 output in the cross-check (backend#2455) The Windows-only real-shell32 oracle disagrees on the space+quote case in a way not reproducible on macOS (no shell32), and "got a" alone is not debuggable. Surface arg / encoded line / real shell32 tokens in the failure -Because and a Write-Host so the next windows-latest run shows exactly what CommandLineToArgvW returned. Diagnostic only -- no change to the encoder or the assertions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(installer): drop argv[0] with an index loop, not a range slice (backend#2455) The real-shell32 diagnostic confirmed the encoder is correct: CommandLineToArgvW returns ["prog.exe","a b\"c"] exactly as intended (shell32=[prog.exe|a b\"c] n=2). The remaining failure was the harness -- $full[1..($full.Count-1)] collapses to a SCALAR string when it selects a single element under Windows PowerShell, so $got[$k] then indexed into that string chars ("got a" for "a b\"c"). Replace the range slice with an explicit index loop that keeps $got a real array on every host; verified it returns whole tokens (incl. the empty arg) for 1- and multi-arg cases. Keeps a concise -Because dumping the real shell32 output for future debug. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): append arg chars as [string] for PS 5.1 (backend#2455) ConvertTo-Win32Arg appended $Arg[$i] (a [char] from string indexing) to the StringBuilder. Under Windows PowerShell 5.1 -- the host the installer relaunches into -- the Append overload binder can bind a [char] to a numeric overload and write the code point instead of the character, corrupting a quoted arg (e.g. a spaced --patch-file); pwsh-7 CI and the golden tests do not see it. Cast to [string] so Append(string) is selected unambiguously on every host. Output is byte-identical on pwsh 7 (golden vectors unchanged). (Bugbot High on #845.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(installer): source guard matches the call, not a comment (backend#2455) The Invoke-BoundedProcess source guard matched the bare name ConvertTo-Win32Arg, which a comment in the same function body also contains -- so deleting the actual call would still pass. Match the call expression (ForEach-Object { ConvertTo-Win32Arg $_) so the guard can detect its own removal. (Bugbot Medium on #845.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
What
In hostpath mode every chart PV is a hostPath onto
/tracebloc/<release>/…, and/traceblocis the k3d bind mount ofHOST_DATA_DIR. When that mount is not in effect, nothing fails. kubelet'sDirectoryOrCreatefabricates the directory inside the node's own filesystem, the PVC Binds, the pod Runs, MySQL initialises a brand-new empty datadir and the dataset dir reads as zero rows. No event, no warning, no failed probe — the operator sees a healthy install that has quietly stopped using their data, and on the nextcluster deleteit goes with the node.This adds a probe, in both installers, that refuses the install when a node can't see the host tree.
Found during the k8s 1.36 PV-rebinding rehearsal (backend#2422). Not hypothetical on the laptops this epic targets: a
HOST_DATA_DIRoutside Docker Desktop's shared paths produces exactly this, as does a cluster recreated by hand without-v.Why not the obvious chart fix
The first thing I tried was flipping the two data PVs to
type: Directoryso kubelet refuses. That cannot ship.spec.persistentvolumesourceis immutable after creation, so it is rejected on any release that already has PVs. Measured on a realv1.36.3+k3s1cluster:…and the release is left in
failed, which is then what the fleet auto-upgrade CronJob retries. That would break the next upgrade of every existing hostpath install in order to close a silent-data bug. So the check belongs in the installer, before helm runs, where being wrong costs an error message instead of a broken upgrade.How it works
Writes a token under
HOST_DATA_DIR, reads it back from inside every node container, compares, removes it.AGENTSdefaults to 1 and agents run kubelets, so a training pod can land on an agent — the same@all-vs-@servertrap as the cgroup v1 flag in feat(installer): set fail-cgroupv1=false from k3s 1.31 so cgroup v1 hosts still start (backend#2422) #806.-serverlbis excluded: it's a proxy, not a kubelet, and probing it would fail every install.Test plan
10 mutation anchors, each reddening only the test that owns it:
if false)docker psfilterhead -1(first node only)Two bats tests were found vacuous while doing this. A bad mock (bash captures no closure, so the helper's
locals were unset by call time) made every case take the "cannot list nodes" branch — which is also non-zero, so the refusal tests passed while exercising the wrong refusal entirely. They now assert their own message, and the mock uses globals.Verified:
cluster.bats126/126install-k8s739 passed / 0 failed (develop baseline 733/0, +6 here)assess.bats71/71hostpath-prep,bats-hygiene,check-style,check-facts,gen-manifest,copy-catalog,check-driftall greenshellcheck -S warning -xat develop's baseline count (2),bash -ncleanThe first placement of the PowerShell half was wrong and the suite caught it: inside
Install-ClientHelmit fired in unit tests that mock docker away, and oneErrexit cascaded into 583 failures. It now sits at the end ofNew-K3dCluster— which is also the correct parity with the bash twin's cluster path.Type
Bug fix (silent data loss) + guard
Refs backend#2422, epic backend#664. No chart change, so no version bump.
🤖 Generated with Claude Code
Note
Medium Risk
Installer-only fail-closed guard on cluster create; a false refusal aborts after the cluster is up, but it does not change auth, charts, or PV specs.
Overview
Stops a silent data-loss path in hostpath installs: if the k3d bind of
HOST_DATA_DIRat/traceblocis missing, kubelet’sDirectoryOrCreatestill succeeds and MySQL/datasets live inside the node untilcluster delete.Both installers now write a unique probe token on the host,
docker execit from every server and agent (not the load balancer), and fail closed if the token is missing, stale, or unverifiable. The check runs at the end of cluster setup, before helm. Bash skips it innode-localmode; Windows is hostpath-only so it always runs.PowerShell adds opt-in
-StdoutOnlyonInvoke-BoundedProcessso docker stderr cannot glue onto the token and false-refuse a good install. Node listing uses exactk3d.cluster/k3d.rolefilters with no quoted--format, avoiding Windows command-line quoting bugs. Extensive bats/Pester coverage pins fail-closed, set -e, sibling clusters, and twin wiring.Reviewed by Cursor Bugbot for commit d7d64dd. Bugbot is set up for automated code reviews on this repo. Configure here.