Skip to content

fix(installer): the Windows installer never waited out the metrics-server race (#553) - #757

Merged
LukasWodka merged 5 commits into
developfrom
fix/553-windows-metrics-apiservice-wait
Aug 19, 2026
Merged

fix(installer): the Windows installer never waited out the metrics-server race (#553)#757
LukasWodka merged 5 commits into
developfrom
fix/553-windows-metrics-apiservice-wait

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

k3s applies its bundled metrics-server — and the v1beta1.metrics.k8s.io APIService with it — shortly after the API server reports ready, and k3d cluster create --wait gates on node/serverlb readiness, not on bundled addons. client/templates/resource-monitor-daemonset.yaml (lines 9-15) lookups that APIService at render time and calls {{ fail }} when it is absent — which aborts the entire helm release, not just the DaemonSet.

The bash installer has had a bounded wait for that window since #553 (lib/install-client-helm.sh::_wait_for_metrics_apiservice). install-k8s.ps1 had none — it went from k3d cluster create straight to helm install. So the gap sat on Windows/WSL2: the slowest host we support, and therefore the one most likely to render inside the window.

What this does

Ports the wait, matching the bash semantics:

  • poll kubectl get apiservice v1beta1.metrics.k8s.io every 3s up to TB_METRICS_WAIT_S (default 120s)
  • then a best-effort kubectl wait --for=condition=Available — the template only needs the APIService to exist at render time, so a non-zero here changes nothing
  • then fall through non-fatally, so a genuinely absent metrics-server still reaches the chart's render-time guard and gets its actionable error (install metrics-server, or resourceMonitor: false) instead of a vague stall here

Three deliberate calls worth reviewing:

  • Same knob name as bash. One support instruction — "set TB_METRICS_WAIT_S=300 and re-run" — has to work on either host. The parse keeps the local TB_CREATE_TIMEOUT_MIN idiom (regex-validated integer) but stays in seconds, since 120s is the budget and minutes cannot express it. It also caps the digit count: [int] on a 20-digit string throws, and a typo'd knob must not take the install down.
  • Above both helm paths. The adopted --reuse-values reconcile re-renders the chart too, so it hits the same fail.
  • Announces once, only after a probe has actually missed. bash runs its loop behind spin_cmd_bounded; silence here would be a two-minute frozen terminal on exactly the host this exists for (RFC-0002 §2). The fast path stays quiet.

Test plan

23 Pester cases covering the parse over its whole input domain (rule 6 — every way a human mistypes the knob lands on the default, never on a silent 0), the poll loop, the best-effort Available wait, the non-fatal fall-through, the bound itself, the kubectl-absent branch, the progress line, and the wiring into Install-ClientHelm on both helm paths.

The polled APIService name is derived from the chart template, not restated (rule 1) — a chart-side rename would otherwise leave the installer politely waiting on a name nobody looks up, which is indistinguishable from a slow cluster.

Mutation-proved (rule 5 — anchors asserted applied, so no inert mutations): 11 mutations each redden their own guard, and the restored control is green.

#MutationResult
M1poll loop to single probe (the pre-#553 "--wait is enough" behaviour)0P/1F
M2fall-through made fatal (Err)0P/1F
M3call site deleted from Install-ClientHelm1P/2F
M4call site moved after the helm render0P/1F
M5polls a stale APIService name the chart no longer looks up0P/1F
M6the bound stops bounding (deadline x30)0P/1F
M7garbage knob silently becomes 0 (wait switched off)0P/1F
M8Has kubectl guard removed0P/1F
M9wait made silent — no progress on a two-minute freeze0P/1F
M10announced per poll instead of once0P/1F
M11announces on the fast path too0P/1F
restored control21P/0F

Gates run locally (pwsh 7.5.2, Pester 6.0.1):

  • Invoke-Pester scripts/tests733 passed, 0 failed, 13 skipped (baseline on develop was 712, so +21 new)
  • Invoke-ScriptAnalyzer scripts/install-k8s.ps1 -Severity Error0 errors; no new warnings (Get-MetricsWaitSeconds / Wait-MetricsApiService are not flagged)
  • bash scripts/check-style.sh — clean
  • bash scripts/tests/check-drift.sh — no drift

No bash, chart, or docs changes: docs/SECURITY.md line 103 already describes the chart failing up front, which is still exactly what happens when metrics-server is genuinely absent.

🤖 Generated with Claude Code


Follow-ups on this PR

addcc6d — the wait budget is one cross-OS fact (Bugbot). Both installers advertise the same TB_METRICS_WAIT_S knob, but each held its own literal 120 (bash had it twice — the function and the spinner bound). Moved to scripts/spec/facts.env as METRICS_WAIT_TIMEOUT, alongside READY_TIMEOUT, stamped into both consumers by check-facts.sh. Four more mutations: either side drifting from the spec turns check-facts --check red; either side reverting to its own literal turns Pester red.

2651c12 — a bare $ anchor cannot match a CRLF checkout.Pester (windows-latest) went red on addcc6d while Pester (ubuntu-latest) stayed green — the asymmetry that job exists to catch. The new facts guard used '(?m)^METRICS_WAIT_TIMEOUT=\d+$'; the Windows runner checks these files out CRLF, and .NET's multiline $ matches before \n but not before \r\n, so the anchor missed a line that was plainly there. Now anchored \r?$. Reproduced against a CRLF fixture, which also shows why an LF-only run could never have caught it:

bare $\r?$ (fix)
LF (ubuntu runner)23P/0F — green, hides the bug23P/0F
CRLF (windows runner)22P/1F — the exact CI failure23P/0F

A note on this PR's checks

Between c46329c and the merge, this PR stopped producing any workflow runs — pushes, label changes and a draft toggle all fired nothing. I first read that as GitHub dropping pull_request events and worked around it with workflow_dispatch; that diagnosis was wrong.

The real cause: #756 merged to develop at 15:27 and put this branch into a conflicting state. GitHub does not create pull_request workflow runs for a PR whose merge ref it cannot compute, so every later push produced zero checks, and mergeStateStatus: DIRTY was saying "merge conflict" the whole time. Merging develop in restored every check immediately, including version-bump-gate / version-check, which has no workflow_dispatch trigger and could not be recovered any other way.

Worth knowing for next time: workflow_dispatch runs attach their check-runs to the commit but do not join the PR's check suite, so the commit-level API reported green while the PR page showed nothing had run. When those two disagree, the PR page is right.

…rver race (#553)
k3s applies its bundled metrics-server -- and the v1beta1.metrics.k8s.io
APIService with it -- shortly AFTER the API server reports ready, and
`k3d cluster create --wait` gates on node/serverlb readiness, not on bundled
addons. client/templates/resource-monitor-daemonset.yaml `lookup`s that
APIService at RENDER time and calls `fail` when it is absent, which aborts the
ENTIRE helm release rather than just the DaemonSet.
The bash installer has had a bounded wait for that window since #553
(lib/install-client-helm.sh::_wait_for_metrics_apiservice). install-k8s.ps1 had
none -- it went from `k3d cluster create` straight to `helm install` -- so the
gap sat on Windows/WSL2, the slowest host we support and therefore the one most
likely to render inside the window.
Port it, matching the bash semantics: poll `kubectl get apiservice` every 3s up
to TB_METRICS_WAIT_S (default 120s), then a best-effort
`kubectl wait --for=condition=Available`, then fall through NON-FATALLY so a
genuinely absent metrics-server still reaches the chart's render-time guard and
gets its actionable error instead of a vague stall here.
Same knob name as bash on purpose: one support instruction ("set
TB_METRICS_WAIT_S=300 and re-run") has to work on either host. The parse keeps
the local TB_CREATE_TIMEOUT_MIN idiom, and caps the digit count -- `[int]` on a
20-digit string throws, and a typo'd knob must not take the install down.
Runs above BOTH helm paths: the adopted `--reuse-values` reconcile re-renders
the chart too, so it hits the same `fail`.
Announces once, only after a probe has actually missed -- bash runs its loop
behind spin_cmd_bounded, and silence here would be a two-minute frozen terminal
on exactly the host this exists for (RFC-0002 §2).
Tests: 23 Pester cases -- the parse over its whole input domain, the poll loop,
the best-effort Available wait, the non-fatal fall-through, the bound itself,
the kubectl-absent branch, the progress line, and the wiring into
Install-ClientHelm on both paths. The polled APIService name is DERIVED from the
chart template rather than restated, so a chart-side rename can't leave the
installer waiting on a name nobody looks up.
Mutation-proved: 11 mutations (single-probe loop, fatal fall-through, deleted
call site, call site moved after helm, stale APIService name, unbounded
deadline, garbage-knob-becomes-0, removed kubectl guard, silent wait, announce
per poll, announce on the fast path) each redden their guard; restored control
is 733/0 green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 19, 2026
…ange
The bootstrap verifies each sub-script against scripts/manifest.sha256 before
running the privileged steps, so a changed sub-script with a stale manifest
would ship a manifest that rejects its own scripts.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@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 1 potential issue.

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 c46329c. Configure here.

Comment threadscripts/install-k8s.ps1
… copies (Bugbot #757)
Bugbot on #757: both installers now advertise the same TB_METRICS_WAIT_S knob,
but each carried its own literal 120s default -- bash in
_wait_for_metrics_apiservice (twice, actually: the function and the spinner
bound at the call site) and PowerShell in Get-MetricsWaitSeconds. A default
raised on one OS would leave the two waiting different lengths while the docs
and the support instruction still named one knob. That is the "restate instead
of derive" class the facts.env mechanism exists for.
Move it to scripts/spec/facts.env as METRICS_WAIT_TIMEOUT, alongside
READY_TIMEOUT -- the existing precedent for a cross-OS timeout budget stamped
into both a bash lib and install-k8s.ps1 and gated by check-facts.sh. Both
installers now read the stamped constant; neither holds a number of its own.
check-facts.bats seeds the two new consumers so the gate's own tests keep
exercising the real script, and the Pester assertions now PARSE the expected
default out of facts.env instead of restating 120 -- a test carrying its own
copy would agree with itself while the installers diverged.
Mutation-proved, 4 more on top of the 11 in the parent commit:
- bash default drifts from facts.env (120 -> 90) -> check-facts --check RED
- PowerShell default drifts from facts.env -> check-facts --check RED
- PowerShell reverts to its own literal default -> Pester RED
- bash reverts to its own literal default -> Pester RED
Restored controls green: check-facts --check rc=0, Pester 23/0, and the parent
commit's 11 mutations all still redden.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Bugbot: fixed in addcc6d, not dismissed — the finding was right.

Both installers advertise the same TB_METRICS_WAIT_S knob, and each was carrying its own literal 120 (bash had it twice: the function and the spinner bound at the call site). A default raised on one OS would have left the two waiting different lengths while one documented knob named both.

Moved to scripts/spec/facts.env as METRICS_WAIT_TIMEOUT, next to READY_TIMEOUT — the existing precedent for a cross-OS timeout budget stamped into a bash lib andinstall-k8s.ps1 and gated by check-facts.sh. Two new rows in the consumer table; check-facts.bats seeds both so the gate keeps testing the real script; and the Pester assertions now parse the expected default out of facts.env rather than restating 120.

Mutation-proved, 4 more on top of the 11 already in the PR body:

MutationResult
bash default drifts from facts.env (120 -> 90)check-facts --check RED
PowerShell default drifts from facts.envcheck-facts --check RED
PowerShell reverts to its own literal defaultPester RED
bash reverts to its own literal defaultPester RED

Restored controls green: check-facts --check rc=0, Pester 23/0 on the new Describe, 735/0 for the whole suite, and the 11 original mutations all still redden.

@LukasWodkaLukasWodka added the gate-nudge Toggled by the release train to (re-)fire the fr-gate label Aug 19, 2026
@LukasWodka
LukasWodka marked this pull request as draft August 19, 2026 14:08
@LukasWodka
LukasWodka marked this pull request as ready for review August 19, 2026 14:08
@LukasWodkaLukasWodka added gate-nudge Toggled by the release train to (re-)fire the fr-gate and removed gate-nudge Toggled by the release train to (re-)fire the fr-gate labels Aug 19, 2026
… windows-latest)
Pester (windows-latest) went red on the previous commit while
Pester (ubuntu-latest) stayed green -- the exact asymmetry that job exists to
catch. The new facts.env guard asserted
$script:BashSrc | Should -Match '(?m)^METRICS_WAIT_TIMEOUT=\d+$'
and the Windows runner checks these files out CRLF. .NET's multiline `$` matches
the position before \n but NOT before \r\n, so the anchor failed on a line that
was plainly present -- the failure output printed the matching line back.
Anchor with \r?$ instead, in both this guard and the facts.env parse above it.
Reproduced and mutation-proved against a CRLF fixture (the same tree with
install-client-helm.sh / facts.env / install-k8s.ps1 converted to CRLF), which
also shows why a LF-only run could never have caught it:
bare $ \r?$
LF (ubuntu) 23P/0F 23P/0F
CRLF (windows) 22P/1F 23P/0F
Whole suite on LF: 735/0/13.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka added gate-nudge Toggled by the release train to (re-)fire the fr-gate and removed gate-nudge Toggled by the release train to (re-)fire the fr-gate labels Aug 19, 2026
…trics-apiservice-wait
# Conflicts:
#	scripts/manifest.sha256
@LukasWodka
LukasWodka merged commit c238e5f into developAug 19, 2026
47 checks passed
@LukasWodka
LukasWodka deleted the fix/553-windows-metrics-apiservice-wait branch August 19, 2026 15:11
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.

3 participants

@LukasWodka@aptracebloc@saadqbal