Skip to content

feat(installer): set fail-cgroupv1=false from k3s 1.31 so cgroup v1 hosts still start (backend#2422) - #806

Merged
LukasWodka merged 2 commits into
developfrom
fix/2422-fail-cgroupv1-installer
Aug 24, 2026
Merged

feat(installer): set fail-cgroupv1=false from k3s 1.31 so cgroup v1 hosts still start (backend#2422)#806
LukasWodka merged 2 commits into
developfrom
fix/2422-fail-cgroupv1-installer

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Kubernetes 1.35 flipped the kubelet's failCgroupV1 default to true, so from k3s 1.35 the kubelet refuses to start on a cgroup v1 or hybrid host. That is not an exotic case for us:

  • WSL2 defaults to hybrid cgroups — the Windows laptop path (AMIGO, UZ Gent).
  • RHEL 8 / CentOS 7 / Ubuntu 20.04 are cgroup v1 by default — hospital Linux boxes.

k3s never sets the field, so the upstream default applies, and k3s documents none of this — an operator would see only a bare upstream kubelet message with no hint an override exists. This sets the override proactively so the refusal is never reached.

Follow-up from the backend#2422 spike. On a cgroup v2 host — every current install — it is a no-op.

Type

Feature (pre-emptive fix; inert until the k3s pin moves).

The gate is load-bearing, not a nicety

--fail-cgroupv1 was added in kubelet 1.31. Passing it to the 1.29.4 kubelet we pin today would be an unknown flag and the kubelet would not start — so an ungated version of this line breaks every install. That is why it ships with four tests rather than one, and why the flag is emitted only from 1.31.0.

Practically: this is inert on the current pin and arms itself when the pin moves, so the migration doesn't have to remember it.

Verified on a real cluster

During the spike, on v1.36.3+k3s1:

Running kubelet … --fail-cgroupv1=false --config-dir=/var/lib/rancher/k3s/agent/etc/kubelet.conf.d …
node: Ready v1.36.3+k3s1
pod: Succeeded

k3s passes it through verbatim, no parse complaint, and ps aux in the node confirms the kubelet process carries it. This settles a point two of our own investigations disagreed on: it is a real CLI flag reachable via --kubelet-arg, not a KubeletConfiguration-only field.

Why _version_lt moved to common.sh

cluster.sh needs it, and assess.sh — where it lived — is sourced conditionally by install-k8s.sh ([[ -f ]], for stale checkouts) while common.sh is not. Depending on it from cluster.sh would have failed open and silently: a missing function makes the if fail and the flag quietly not get added. One implementation, two consumers, rather than a second copy.

assess.sh's own callers are unaffected — install-k8s.sh sources common.sh first, and the bats helper's load_lib chains common.sh.

The trap this could have shipped with

_version_lt reads a leading v as a non-numeric component, i.e. 0 — so _version_lt v1.36.3 1.31.0 is TRUE and the comparison inverts. A gate written as _version_lt "$K8S_VERSION" "1.31.0" would therefore silently stop emitting the flag on exactly the versions that need it, while looking correct and passing a naive test.

Hence ${K8S_VERSION#v}, and hence a mutation anchor: dropping the strip reddens both flag-asserting tests while the two negative tests stay green. Applied and reverted the mutation to confirm the coverage is real rather than vacuous:

mutation applied → not ok 2, not ok 3 (ok 1, ok 4 still pass)
mutation reverted → all 4 ok

The trap is also pinned directly in assess.bats, documenting the behaviour rather than the intent — a gate that silently stops firing is worse than one that never worked.

Test plan

  • bats scripts/tests/cluster.bats114/114 (4 new)
  • bats scripts/tests/assess.bats71/71 (1 new; confirms the helper move didn't break its callers)
  • shellcheck -S warning on all three touched libs — at baseline count, diffed against origin/develop rather than eyeballed:
    cluster.sh 2→2 · common.sh 15→15 · assess.sh 0→0
  • scripts/gen-manifest.sh re-run and scripts/manifest.sha256 committed (Static analysis R8 gate)
  • PowerShell twin updated in lockstep, using native [version] comparison; the semver strip verified against all four pin shapes (v1.29.4-k3s1, v1.31.0-k3s1, v1.36.3-k3s1, 1.35.7+k3s1)

What this does not prove

That the flag rescues an actual cgroup v1 host. That needs a WSL2 or RHEL 8 machine; it is not reproducible on macOS, where the Docker VM kernel is cgroup v2 (linuxkit.unified_cgroup_hierarchy=1). What is proven is that the flag is accepted and plumbed through, and that KEP-5573's documented purpose is exactly this override. Setting it by default is what makes the unproven half not matter: a customer never reaches the refusal.

Checklist

  • Targets develop
  • Self-assigned
  • bats + shellcheck green, no regression vs baseline
  • Manifest regenerated
  • Mutation-proved
  • One reviewer

🤖 Generated with Claude Code


Note

Medium Risk
Touches k3d cluster create on both Unix and Windows installers. A wrong version gate or node filter would prevent kubelets from starting, though the current 1.29.4 pin leaves the flag unemitted.

Overview
Prevents kubelet refusal on cgroup v1/hybrid hosts once k3s reaches 1.35 (WSL2 hybrid, RHEL 8 / Ubuntu 20.04). Cluster create now passes --kubelet-arg=fail-cgroupv1=false@all when K8S_VERSION is latest or ≥ 1.31.

The gate is required: the flag did not exist before kubelet 1.31, so emitting it on the current v1.29.4 pin would fail every install. Scoped to @all because agents run kubelets too. Empty and digest-only pins skip.

_version_lt moves from assess.sh to common.sh so cluster.sh can use it (assess is sourced only conditionally). Callers must strip a leading v or the comparison inverts. PowerShell mirrors the same gate with a shape-checked [version] cast.

Inert on today’s pin; arms when the pin or latest default crosses 1.31/1.35.

Reviewed by Cursor Bugbot for commit 137899f. Bugbot is set up for automated code reviews on this repo. Configure here.

…osts still start (backend#2422)
Kubernetes 1.35 flipped the kubelet's failCgroupV1 default to true, so from
k3s 1.35 the kubelet REFUSES TO START on a cgroup v1 or hybrid host. That is
not an exotic case for us: WSL2 defaults to HYBRID cgroups, and RHEL 8 /
CentOS 7 / Ubuntu 20.04 are cgroup v1 by default -- the Windows laptops and
hospital Linux boxes we install on. k3s never sets the field, so the upstream
default applies, and k3s documents none of it: an operator would see only a
bare upstream kubelet message with no hint that an override exists.
Set the override proactively rather than discovering it in the field. Verified
on a real v1.36.3+k3s1 cluster during the backend#2422 spike: k3s passes it
through verbatim ("Running kubelet ... --fail-cgroupv1=false ..."), the node
comes up Ready, and a pod schedules. On a cgroup v2 host -- every current
install -- it is a no-op.
THE GATE IS LOAD-BEARING. --fail-cgroupv1 was ADDED in kubelet 1.31, so
passing it to the 1.29.4 kubelet we pin today would be an unknown flag and the
kubelet would not start. An ungated version of this line breaks every install,
which is why it ships with four tests rather than one.
_version_lt moves from assess.sh to common.sh. cluster.sh needs it and
assess.sh is sourced CONDITIONALLY by install-k8s.sh ([[ -f ]], for stale
checkouts) while common.sh is not -- so depending on it from cluster.sh would
fail open silently. One implementation, two consumers, rather than a second
copy.
Mutation-proved the leading-v strip: _version_lt reads a leading "v" as a
non-numeric component (0), so `_version_lt v1.36.3 1.31.0` is TRUE and the
gate inverts. Dropping ${K8S_VERSION#v} reddens both flag-asserting tests
while the negative ones stay green -- confirmed by applying and reverting the
mutation. The trap is also pinned directly in assess.bats, because a gate that
silently stops firing is worse than one that never worked.
Verified: cluster.bats 114/114, assess.bats 71/71, shellcheck -S warning at
baseline count on all three touched libs (no regression), manifest
regenerated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 24, 2026

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 10773b9. Configure here.

Comment threadscripts/lib/cluster.sh
Comment threadscripts/install-k8s.ps1
Comment threadscripts/install-k8s.ps1 Outdated
Comment threadscripts/lib/cluster.sh Outdated

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good PR — the gate reasoning, the _version_lt relocation rationale, and the leading-v mutation
anchor are all sound, and I verified the anchor does what it claims (_version_lt v1.36.3 1.31.0 is
indeed TRUE, so dropping the #v strip reddens the 1.36 test while the other three stay green).

Requesting changes on one thing: [version]$k8sSemver throws on K8S_VERSION=latest, and it
runs before the latest branch 14 lines below that exists to honour it. Confirmed under pwsh, not
inferred. The bash twin returns "don't emit" for the same input and cluster.bats pins that for the
empty case, so the suite asserts a behaviour the PowerShell half crashes on — under a comment that
says keep this in lockstep with the bash twin. This file already guards that exact pair at
:3107 and :6653.

Second comment on cluster.sh is a judgement call, not a blocker: latest/empty read as
below-1.31 and skip the flag, which is fail-closed and defensible — but that's the unpinned path,
and your own comment records it as how a client landed on v1.35.5. Worth deciding explicitly rather
than by parse accident; K3D_VERSION is pinned at v5.9.0, so the pre-1.31 hazard can't be reached
there anyway. Fine either way as long as it's stated.

…dated version (backend#2422)
Two findings on #806, one of them a real bug in the original change.
@ALL, not @server:* (Bugbot, High). AGENTS defaults to 1 and an agent runs a
kubelet too, so scoping the override to the server would have left the agent
kubelet refusing to start on a cgroup v1 host -- `--wait` fails or the cluster
sits half-ready, which is the exact refusal this change exists to prevent. I
copied the nodefilter from the `--disable=` args beside it; those are
`@server:*` because addon deployment is a server-only concern, and a kubelet
arg is not. The two must not be copied from each other. Pinned by a test that
asserts @ALL AND rejects @server, because this is easy to "tidy" back.
Guard the PowerShell cast (Asad + Bugbot). `[version]$k8sSemver` threw before
the `latest` branch 14 lines below that exists to honour that value. Verified
under pwsh, and it is broader than reported: the unguarded cast throws on
`latest`, on EMPTY, and on a digest-only pin -- three inputs, and cluster.bats
pins empty->skip for the bash half, so the suite asserted a behaviour the
PowerShell half crashed on. Now uses this file's own explicit-guard pattern
(Test-K3sVersionDrift, the GPU gate) plus a shape check before casting.
`latest` now EMITS, decided rather than fallen into (Asad's second comment).
It is the unsupported opt-out where k3d chooses the k3s version and we cannot
read it, so the trade is a flag harmless from 1.31 against a refusal fatal
from 1.35 -- and `latest` is the path that produced the v1.35.5 drift
incident. k3d is pinned at v5.9.0, whose default k3s is 1.32: above the flag's
introduction, below the refusal. Empty and unparseable still skip.
Verified: cluster.bats 117/117 (7 on this gate), assess.bats 71/71,
shellcheck -S warning at baseline on all three libs, bash -n clean, ps1 parses
with 0 errors, and the gate logic re-run under pwsh across all six inputs.
Manifest regenerated (also resolved its merge conflict with develop).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Both addressed in 137899f — and one of them was a real bug, not a nitpick.

Bugbot's High was right, and I'd copied the wrong precedent

@server:* would have left the agent kubelet refusing to start. AGENTS defaults to 1 (common.sh:849), an agent runs a kubelet, so on a cgroup v1 host --wait fails or the cluster sits half-ready — the exact refusal this change exists to prevent.

The cause is worth naming: I took the nodefilter from the --disable= args sitting directly above, which are @server:*because addon deployment is a server-only concern. A kubelet arg is not. Those two lines look identical and mean different things, so the comment now says they must not be copied from each other, and a test asserts @alland rejects @server — this is precisely the kind of line someone "tidies" into consistency with its neighbours.

Your [version] finding — confirmed, and broader than reported

You were right that it throws on latest and runs before the branch meant to honour it. Verified under pwsh, and the unguarded cast throws on three inputs:

[latest] old=THROW: RuntimeException new=emit @all
[] old=THROW: RuntimeException new=skip
[sha256:0123abc] old=THROW: RuntimeException new=skip
[v1.29.4-k3s1] old=skip new=skip
[v1.36.3-k3s1] old=emit new=emit @all

The empty case sharpens your point: cluster.bats pins empty→skip for the bash half, so the suite asserted a behaviour the PowerShell half crashed on — under a comment saying keep the two in lockstep. And a digest-only pin isn't dotted-numeric either, which :3105 already anticipates elsewhere in the file.

Fixed with this file's own pattern rather than a new one: explicit -eq "latest" first, as Test-K3sVersionDrift and the GPU gate do, then a ^\d+\.\d+ shape check before casting.

Your second comment — decided, and it changed my answer

You said it was fine either way as long as it's stated. Having to state it made me change it: latest now emits.

The reasoning, now in both twins: latest is the unsupported opt-out where k3d chooses the k3s version and we cannot read it, so the trade is a flag that is harmless from 1.31 against a refusal that is fatal from 1.35 — and latest is the very path that produced the v1.35.5 drift my own comment cites. As you noted, K3D_VERSION is pinned at v5.9.0, whose default k3s is 1.32 — above the flag's introduction, below the refusal. So emitting is safe today and becomes correct the moment k3d's default crosses 1.35, which is the direction it only ever moves.

Empty and unparseable still skip: common.sh defaults K8S_VERSION to the pin, so empty only occurs in tests.

Verification

  • cluster.bats117/117 — 7 on this gate: pinned-1.29 skips, 1.31 emits, 1.36 emits (the leading-v mutation anchor), empty skips, latest emits, digest skips, and the @all nodefilter
  • assess.bats71/71
  • shellcheck -S warning at baseline count on all three libs, diffed against origin/develop rather than eyeballed
  • bash -n clean; install-k8s.ps1 parses with 0 errors; gate logic re-run under pwsh across all six inputs
  • Manifest regenerated — which also resolved its conflict with develop

bugbot run

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Both of my threads are addressed and I re-ran the matrix on both engines rather than
reading the replies — the twins now agree on every input, which was the actual defect:

 pwsh bash
v1.29.4-k3s1 False false
v1.31.0-k3s1 True true
v1.35.5-k3s1 True true
v1.36.3-k3s1 True true
latest True true
(empty) False false
sha256:0123abc False false

No throws on any input, so the latest opt-out reaches its own warning instead of dying on a cast.

You changed my mind back on the second one, which is the right outcome and better reasoned than my
comment: a flag that is harmless from 1.31 against a refusal that is fatal from 1.35 is not a
symmetric trade, and latest is the path that actually produced the v1.35.5 drift. Emitting is safe
under k3d v5.9.0 and becomes correct as its default moves — the only direction it moves. Using this
file's existing -eq "latest" idiom rather than a new shape check is also the better call.

The seven tests each pin a distinct decision — 1.29 skips, 1.31 emits, 1.36 pins the #v strip,
empty skips, latest emits, a digest skips, and @all is asserted against@server:* rather than
just for itself.

And credit where it's due: Bugbot's @server:*@all was the sharper catch and I missed it. An
agent kubelet on a cgroup v1 host would have refused to start while the server came up, so --wait
hangs or the cluster sits half-ready — the exact failure this block exists to prevent, on the WSL2
path it was written for.

One claim I could not check locally, flagged rather than doubted: that k3d v5.9.0's default k3s is
1.32. It's the load-bearing fact for emitting on latest, and it's in a comment where a future reader
will trust it — worth a link to the k3d release notes beside it.

@LukasWodka
LukasWodka merged commit cc2e4bf into developAug 24, 2026
48 checks passed
@LukasWodka
LukasWodka deleted the fix/2422-fail-cgroupv1-installer branch August 24, 2026 10:15
LukasWodka added a commit that referenced this pull request Aug 24, 2026
…data dir (backend#2422) (#817)
* feat(installer): refuse to install when the nodes can't see the host 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>
* style(installer): say "secure environment", not "workspace", in the mount-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>
* fix(installer): select probe nodes by k3d label, bound both docker calls, 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>
* fix(installer): isolate stdout in the mount probe, so docker stderr can'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>
* fix(installer): query nodes per role so no argument carries a quote, 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>
* fix(installer): keep the per-role fail-closed branch reachable under 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>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LukasWodka@saadqbal