fix(e2e): distinguish a HelmRelease teardown from a retried failure and cover the tenant addons by lexfrei · Pull Request #3594 · cozystack/cozystack

Aleksei Sviridkin (lexfrei) · 2026-08-07T04:49:39Z

What this PR does

The remediation guard read a failed and an uninstalled Snapshot in .status.history as one signal. That holds under Flux's default RemediateOnFailure, where a failed install is remediated by uninstalling the release, and it is wrong under RetryOnFailure, where the failed attempt keeps its manifests applied and is retried as an upgrade: there nothing was removed. The aggregated apiserver stamps RetryOnFailure on the HelmRelease of every Application it serves (pkg/registry/apps/application/rest.go), on install and upgrade alike, and the tenant cilium and csi releases set it in the chart, so the reading reported a cycle on releases that recover by design, and it would have done the same to every addon the guard was about to cover.

helmrelease_has_teardown is the narrow reading: an uninstalled Snapshot, which is proof the release was removed and put back. helmrelease_has_remediation_cycle keeps its meaning and its breadth, since it also catches the upgrade remediation that rolls back rather than uninstalls, at the price of not proving anything was removed. Neither helper takes the release's configuration at all; the caller reads it and decides what the evidence is worth, which keeps each helper answering one question about one list of statuses.

With that distinction in place the coverage reaches past the parent release. cozy_guard_helmrelease judges one release; cozy_guard_addon_helmreleases runs it over the addon HelmReleases the parent installs, because the parent's own history says nothing about them: a teardown of the tenant CNI or CSI otherwise surfaces only as whatever downstream deadline it eventually breaks. The parent block goes through the same function, so its reads are covered by the same unit tests instead of sitting inline in run_kubernetes_test, where a revert of them stays green. The addons are read from the cluster rather than from a copy of the chart's addon set, so a release added later is covered without touching the script, and a prefix that matches nothing is an error rather than a quiet pass over nothing.

What fails a run

One rule, and it takes two fields to state: the release was removed although nothing in its own configuration would remove it to recover. An uninstalled Snapshot is proof of removal. The install strategy is the first field, because RetryOnFailure retries in place and never uninstalls. The retry budget is the second, and it reaches the releases the strategy does not: the default strategy uninstalls only between install attempts it is allowed to take, since InstallRemediation.RetriesExhausted is true once the failure count passes .spec.install.remediation.retries and MustRemediateLastFailure defaults to false, so at a budget of zero no attempt and therefore no uninstall is ever made. An uninstalled Snapshot on such a release is exactly as unexplained as one on RetryOnFailure. The field is omitempty on an int, so a release configured with 0 and a release with no remediation block are the same bytes on the wire and mean the same thing here. No release of this chart is in that shape today, since all twenty addon releases set retries: -1, and the budget is read anyway because the addons are enumerated from the cluster precisely so a release added later is covered without editing the script.

Everything else is printed and named without failing: an uninstalled Snapshot on a release whose default strategy and non-zero budget explain it, and a failed Snapshot anywhere. Nothing in the suite measures how often those addons take their configured recovery path, and failing a 25-minute bringup on a release doing what it is configured to do is how a guard gets switched off. Those addons are configured differently from cilium and csi, which were moved to RetryOnFailure because their teardown removed something the cluster depended on; bringing the rest in line is a separate change from this one.

One read fails the run on the parent alone. .spec.install.strategy.name is read with kubectl -o jsonpath, which prints nothing and exits 0 for a path that stops resolving, so a rename of that field upstream would fail soft into the lenient branch and report every teardown, a real removal of the tenant CNI included, as an install remediation on a green run. An empty value is not by itself evidence of that, because eighteen of the chart's twenty addon releases set no strategy, but the parent always carries one, so an empty strategy there is the read rather than the object. Only the parent is asked for it, and that is enough: the same rename empties the field on every release at once.

The paths where the guard cannot run

The guard sits after the assertions, and every deadline whose breach it exists to explain, a node that never turns Ready, a HelmRelease wait that expires, a PVC that never binds, ends the script under errexit well before it. So on the runs where a teardown of the tenant CNI or CSI was the cause, the guard had nothing to say: the failure surfaced as the downstream deadline and the removal that preceded it was named nowhere. The node-join collector prints each release's Ready message rather than its .status.history, so that path was as blind as before the guard existed.

cozy_report_helmrelease_remediation classifies the same releases on those paths, prints the verdicts and fails nothing. It runs from the node-join collector and from the EXIT handler that covers every other failure. It fails nothing deliberately: returning non-zero from a collector on a failing path would replace the caller's own exit status, and the tenant crust-gather snapshot hangs off that status. It runs once per run, so the two call sites and the guard itself cannot classify one release twice. The enumeration is shared with the guard rather than copied, so a release the chart adds later appears in both readings or in neither.

Inside the node-join collector the classification runs after the guest serial console rather than before it, and the ordering rule is the evidence rather than the price. The console is the only artefact covering a worker that never reached apid, which is the dominant shape of that failure and one nothing else in the block describes; its measured worst case takes most of the phase budget, so anything ahead of it competes with it on exactly the slow runs it exists for. The classification reads objects on the management cluster that outlive the run, so a walk the budget declines can still be answered from those objects afterwards. Both suites' spend-order comments list it in that position, and the two guards over that order match calls by position inside the block rather than by name, since the same collector is also called from the EXIT handler, which belongs to no phase.

The handler is armed at the Kubernetes CR apply rather than once the tenant answers. Everything between those two points is a management-cluster wait on the parent HelmRelease or on what it installs: the KamajiControlPlane, the control-plane Deployments, the MachineDeployment, the admin-kubeconfig Secret. Those waits carry the largest budgets in the run, and that stretch is the window the guard was written for, since a helm-wait budget expiring while admin-kubeconfig is still being provisioned is what triggers the uninstall remediation the guard reads. Armed after those waits, the handler was absent from exactly the window it exists to cover. The parent HelmRelease exists from the apply onwards, rendered from the CR by the aggregated apiserver, so there is something to read the whole way, and arming this early costs the snapshot nothing: _tenant_snapshot_on_fail returns on an unset CURRENT_TENANT_KC, which is still assigned where the kubeconfig becomes usable.

The parent readiness wait the guard depends on now names what it timed out on. On elapse kubectl printed its own timeout line and nothing about the release, and the guard below it never ran, so the run reported a readiness timeout with no statement of what the release was stuck on. That branch collects the HelmRelease table, the parent's own detail and the namespace events, bounded like every other read on a failure path, before the exit that triggers the tenant snapshot. The exit handler classifies .status.history from there as well, but a release still mid-install has a Ready message and events that no history carries.

How the reads are made

Every probe keeps its exit status, because a read that failed and a release with no history both print nothing on stdout, and folding them together lets an API timeout pass for the latter. Neither folds kubectl's stderr into the value it captures, because kubectl writes warnings there while exiting 0: such a line inside the history capture turns an empty history into a populated one and skips the readiness branch, and inside the strategy capture it stops the value matching RetryOnFailure and downgrades a real teardown to a note.

Every read is bounded, by a --request-timeout and by a wall clock, and both are needed. The guard runs around minute 25 of the bringup on the passing path, and against a wedged apiserver an unbounded read there does not lose only itself: it holds the Chainsaw op until the op is killed, so the tenant snapshot the caller's exit triggers is lost rather than truncated. --request-timeout bounds one HTTP request, while a client retrying against an apiserver that keeps answering slowly stays inside it indefinitely, which is what the wall clock catches. Neither adds a retry. The read is issued once, and one that did not finish stays a failed read, which the callers report and fail on rather than folding into "no history". The failure-path classification carries a budget of its own on top, checked before the addon listing and before each release, because the listing is the wrong read to spend past a deadline: it buys a set of names for a walk with no time left to make. A release the budget cut off is named rather than dropped.

Readiness travels in the same read as the strategy and the retry budget, and all three are read before the history, because the other order observes them the wrong way round. The controller writes the Snapshot and flips Ready in one status patch, so a release is never Ready before its first Snapshot exists; read the history first and an addon that finishes installing in the round-trip between the two reads presents as Ready with an empty history, which the guard fails the run on as a status shape it can no longer read. Nothing waits on most of these releases, so that window would be open on every run. What the order buys is the truncation half of that: neither truncation variant in helm-controller/api can empty a non-empty history, since Truncate returns early below two Snapshots and TruncateIgnoringPreviousSnapshots cuts only above five, so which of them the controller picks does not matter here. It does not close the window entirely. HelmReleaseStatus.ClearHistory() drops history outright, and where the controller calls it is in a module this tree does not carry, so a call landing between the two reads would still present as Ready with an empty history. The claim is about truncation and not about the read order being safe. It also drops the guard from three reads per release to two.

Each note says only what was read. The retry budget is quoted in the teardown note, which is the note it explains, and not in the rollback one. Readiness decides the tense of the failed-Snapshot note, because a release that is not Ready carries its failure live rather than behind it, and a note claiming a recovery would send a reader after a second cause while the first one is still the answer.

An empty history is judged against that release's own Ready condition rather than the parent's, because the parent cannot stand in for it: the Kubernetes ApplicationDefinition sets release.cozystack.io/helm-install-disable-wait and the apiserver turns that into DisableWait on both actions, so the parent reaches Ready without waiting for anything it applied, and the suite waits on some addons by name while never naming others. Ready with no history is a status shape the helper can no longer read and fails the run; not-Ready with no history is a release that never completed an action, and it is reported as not inspected rather than failed or passed over in silence.

The guard adds no wait and no retry, and it runs before the tenant-snapshot trap is disarmed, so a teardown it finds still captures the tenant cluster. A test pins that wiring by reading the script, because with the guards behind functions, deleting the calls or moving them past the trap would otherwise leave every case in the file green while the run asserted nothing. cozy_guard_all_helmreleases collects the parent's verdict and the addons', because as two bare commands under errexit a failing parent ended the script before any addon was read, while the addon loop one level down keeps inspecting after a failure since the other notes explain it.

Two smaller corrections ride along in the same library. Both helpers assigned statuses without scoping it, and the library is sourced rather than executed, so calling either one left the argument behind in the caller's scope; nothing in this tree keeps release history under that name, which makes this hardening rather than the repair of an observed clobber, and is also why the scoping gets a case of its own instead of riding on the existing ones. And the comment said a failed or uninstalled Snapshot survives a later successful reconcile and stopped there, which reads as unconditional: the controller truncates history on every in-sync reconcile, keeping the newest five Snapshots on a release whose strategy is a retry and, on any other, those down to the previous deployed or superseded one. Nothing in the guard changes, because a single run applies its release once and never upgrades it afterwards, but a caller reading history further from the action is reading a window and the text says so now.

Tests and docs

hack/remediation-guard.bats gains cases for the two readings and their scoping. hack/run-kubernetes-remediation_test.bats is new, with kubectl answered from fixtures, covering the enumeration, the literal prefix match, the read order, the readiness branch, both fatal conditions and the wording of every note, the probe-failure paths, the arming point of the EXIT handler and the status it hands to the snapshot, the failure-path classification and its budget, the one-classification-per-run flag, the diagnostics on the parent readiness wait, and the wiring of the guard calls. The classification budget is a knob, so it joins the two pins the file's other knobs already have: the loop in hack/run-kubernetes-node-join_test.bats that rejects a unit suffix and a leading zero, and a case of its own for the zero that would decline the walk rather than bound it, at both the environment and the post-source path. The wiring pin requires the matched line to end at the call, because the verdict reaches the run through errexit rather than an exit of its own and a substring match would stay green against an appended || true; indentation is left free so that wrapping the call in a block stays a refactor. The ordering pin in hack/run-kubernetes-serial-console_test.bats moves onto the call it anchors on, off the text of its arguments, which name library locals this change renames, and onto the control-plane wait by name rather than the file's first kubectl_wait_retry, since a file-global match would compare two unrelated line numbers as soon as a wait is added to any function defined above.

docs/agents/e2e-testing.md is updated where it described the guard as parent-only and the footprint as surviving indefinitely, where it stated the teardown rule as the strategy alone rather than the two fields it is, and where it placed the EXIT handler at the point it used to be armed. The bullet answering which releases can red a run now names every condition that does, not the teardown rule alone. Item 8 names where the policy lives as well as where the predicates do.

Two open issues affected without being closed

The global assignment reported in #3774 goes away with the local added to both helpers here; the sweep that issue also asks for over the other _lib/ helpers is not done, and etcd-cleanup.sh, etcd-probe.sh, ghcr-mirror.sh, pod-label-census.sh and talos-image-cache.sh are not touched by this change at all.

#3773 has two halves. The self-refuting comment it quotes is rewritten in full, dropping the opening sentence claiming a non-zero installFailures/upgradeFailures means something is gone, and what replaces it says those counters are exactly what the guard does not use. Of the other half, the kubectl wait calls in the file with no event-driven backstop, this change reaches one: the parent HelmRelease readiness wait the guard depends on, which now dumps the release state before exiting. Six such waits remain, and the issue stays open for them.

Closes #3553.

Open questions

Two things the reviews disagreed on. Not decided in this PR.

The parent's failed Snapshot is a note now instead of a red run, with no measurement behind the change. The mechanism is right: under RetryOnFailure a failed Snapshot is a retried attempt that kept its manifests. What nobody showed is that the old check ever fired falsely. The apiserver has stamped RetryOnFailure on Application HelmReleases since 5e148f7 (2026-05-06) and the fatal-on-failed guard landed in the Chainsaw suite in e22d84f (2026-06-08); no run in those two months was named where it fired. That is the same unmeasured pass condition this PR refuses for the addons. One review suggested splitting the rule by release class: failed fatal on the Application-served parent, a note on the chart-rendered addons. Not done here.

Fail-fast API reads went from one to one listing plus two per release. Each kubectl get reds the whole run on any non-zero exit, and they land around minute 25 of the bringup. They are bounded now, by a request timeout and a wall clock, but no retry is added: docs/agents/e2e-testing.md limits retry to steps with no product or test logic, and a HelmRelease status read is not that.

A test outside this change

One file in this diff is not part of the guard. hack/run-kubernetes-node-join_test.bats already carried a sweep that greps hack/ and docs/ for every minute figure written beside a node-join referent and fails when one of them is not the 29m the wait actually gives. Three of its referents, become Ready, became Ready and Ready within, name no subject, and a HelmRelease, a LoadBalancer and a PVC are each said to do exactly that on a deadline of its own. This branch adds a line reporting that a parent HelmRelease did not become Ready within 5m; the sweep read it as a node-join deadline quoting the wrong figure and went red over a sentence that is correct about both waits and wrong about neither.

The referents are narrowed to the ones that name a node, rather than the new line being added as an exception. The set of things that become Ready on a clock is open and grows with the suite, so an exception list needs an entry every time one is added; the ways to write the node-join are finite, so an allow-list stays right on its own. Coverage did not drop for it: the pattern matches 21 lines across six files against the floor of 18 the test asserts, and exactly one line leaves the sweep, the 5m HelmRelease message that caused the red.

The same sweep also counted files that are not part of the tree, and that turned out to matter more than the referents. It grepped the directory, so a backup left beside a swept file, run-kubernetes.sh.bak from a sed -i, was read as another source of quotations. Such a copy does not redden the guard, since it holds the same lines: it inflates the count. The floor is set just under real coverage precisely to catch a quotation drifting out of the pattern, and an extra copy pushes the count the other way, so a forgotten backup masks the loss the floor exists to find. Reproduced in three states: five quotations pushed out of the pattern take the count to 16 and the guard reds, and one stray backup on top of that takes it to 25 and the guard goes green while real coverage is still 16. The sweep now runs through git grep, which reads tracked files by construction and closes the class rather than an enumerated list of suffixes; on a clean tree its output is byte-identical to the old one.

Screenshots

Not a UI change.

Downstream repositories

Walked the trigger map against the diff: it touches hack/e2e-chainsaw/_lib/run-kubernetes.sh and remediation-guard.sh, adds one hack/*.bats, extends two others, re-anchors a pin in a fourth, adds one line to each of the two kubernetes suites' spend-order comments, and edits docs/agents/e2e-testing.md. Nothing under hack/ is moved or renamed, no make target changes what it does, and no package, schema, CRD or user-facing document is touched, so none of the listed repositories is reached.

Release note

fix(e2e): the remediation guard now covers the tenant addon HelmReleases as well as the parent release, and classifies them on the failure paths where the guard itself cannot run.

It fails a run only when a release was uninstalled with nothing in its own configuration to explain the removal, and reports every other remediation footprint without failing.

coderabbitai · 2026-08-07T04:50:18Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

The E2E remediation checks distinguish teardown from failed remediation, validate parent and addon HelmReleases, handle API and history states, and keep the failure snapshot trap active during validation.

Changes

HelmRelease remediation guards

Layer / File(s)	Summary
Remediation history classification `hack/e2e-chainsaw/_lib/remediation-guard.sh`, `hack/remediation-guard.bats`	Adds teardown detection for `uninstalled` snapshots and tests it separately from `failed` remediation history.
Parent and addon guard flow `hack/e2e-chainsaw/_lib/run-kubernetes.sh`	Validates parent and matching addon HelmReleases using strategy, history, and Ready state. The Kubernetes test preserves the failure snapshot trap until all checks complete.
Guard validation and release coverage `hack/run-kubernetes-remediation_test.bats`, `docs/agents/e2e-testing.md`	Tests failures, empty histories, filtering, warnings, API errors, guard ordering, and remediation outcomes. The guidance requires history guards for HelmRelease install and upgrade changes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant KubernetesTest
  participant cozy_guard_all_helmreleases
  participant cozy_guard_helmrelease
  participant kubectl
  KubernetesTest->>cozy_guard_all_helmreleases: validate parent and addon HelmReleases
  cozy_guard_all_helmreleases->>cozy_guard_helmrelease: validate each release
  cozy_guard_helmrelease->>kubectl: read strategy, history, and Ready state
  kubectl-->>cozy_guard_helmrelease: return release state or read error
  cozy_guard_helmrelease-->>cozy_guard_all_helmreleases: return release result
  cozy_guard_all_helmreleases-->>KubernetesTest: preserve aggregate failures

Possibly related issues

tenant: monitoring and gateway releases lose telemetry and the ACME account to install remediation #3624: Adds E2E guards for unexpected HelmRelease uninstall remediation under RetryOnFailure.

Possibly related PRs

cozystack/cozystack#3552: Adds guards and tests for RetryOnFailure HelmRelease remediation.
cozystack/cozystack#3581: Changes the HelmRelease strategy validated by these remediation guards.
cozystack/cozystack#3621: Changes HelmRelease RetryOnFailure behavior covered by these checks.

Suggested labels: area/kubernetes

Suggested reviewers: myasnikovdaniil

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes separate teardown detection from remediation-cycle detection and extend guards to tenant addon HelmReleases as required by issue `#3553`.
Out of Scope Changes check	✅ Passed	The implementation, tests, and documentation changes directly support issue `#3553` and the stated PR objectives.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main changes: distinguishing HelmRelease teardowns from retried failures and extending coverage to tenant addon HelmReleases.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/remediation-guard-distinguish-teardown

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Aleksei Sviridkin (lexfrei) · 2026-08-07T07:44:32Z

Reopening to pick up the multus fix on main.

IvanHunters

LGTM. Verified statically and hermetically (no cluster): the fix premise checks out against the code — the aggregated apiserver unconditionally stamps RetryOnFailure on every Application HR (rest.go:1601-1611), cilium/csi set it explicitly, and addons without a strategy fall back to the default, which matches the guard's teardown-vs-retry logic. The guard aborts the run for real (set -eu in the chainsaw step, not decorative), there's no local-assignment status masking, and the empty-selection path is fail-closed, not fail-open. 30/30 unit tests pass; non-vacuity confirmed by two mutations.

Non-blocking notes:

The guard's three kubectl reads are not wrapped in the existing kubectl_wait_retry helper. In the etcd-noisy sandbox a single transient "leader changed" would redden the whole run. Fail-closed is the right default here; wrapping the reads in a bounded retry (distinguishing a transient error from a missing field) would remove a new false-red source.
Addon-guard coverage is bounded by DisableWait (an addon torn down after the check point won't be seen) — best-effort, already acknowledged.
grep "^${_prefix}" treats the prefix as a regex; input is controlled so no practical risk, but grep -F would be stricter.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

hack/e2e-chainsaw/_lib/run-kubernetes.sh (1)
1659-1659: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add diagnostics to the parent readiness wait.

If the parent HelmRelease does not become Ready in 5m, kubectl wait fails and errexit ends the run. The only output is kubectl's timeout message. The guard below prints describe hr on its own failure paths, but this wait prints nothing about the release state, so the stuck release is not named. Every other long wait in this function (node join, backend readiness, scheduling gate) dumps scoped diagnostics before exit 1.

Note that the tenant snapshot trap captures the tenant cluster, not the management-cluster HelmRelease objects, so it does not cover this gap.
♻️ Proposed diagnostics on the wait path
-  kubectl wait hr -n tenant-test "kubernetes-${test_name}" --timeout=5m --for=condition=ready
+  if ! kubectl wait hr -n tenant-test "kubernetes-${test_name}" --timeout=5m --for=condition=ready; then
+    echo "=== parent HelmRelease kubernetes-${test_name} did not become Ready within 5m — diagnostics follow ===" >&2
+    kubectl -n tenant-test get hr >&2 || true
+    kubectl -n tenant-test describe hr "kubernetes-${test_name}" >&2 || true
+    exit 1
+  fi
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh` at line 1659, Update the parent
HelmRelease readiness wait around kubectl wait for kubernetes-${test_name} to
handle timeout failure explicitly, print scoped diagnostics for that HelmRelease
with kubectl describe hr in the relevant namespace, then exit with failure while
preserving the existing 5-minute wait and success path.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh`:
- Around line 740-751: Source remediation-guard.sh in the runtime path before
the cozy_guard_all_helmreleases helper is invoked. Update
hack/e2e-chainsaw/_lib/run-kubernetes.sh so helmrelease_has_teardown and
helmrelease_has_remediation_cycle are defined before this guard logic executes.

---

Nitpick comments:
In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh`:
- Line 1659: Update the parent HelmRelease readiness wait around kubectl wait
for kubernetes-${test_name} to handle timeout failure explicitly, print scoped
diagnostics for that HelmRelease with kubectl describe hr in the relevant
namespace, then exit with failure while preserving the existing 5-minute wait
and success path.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 525b4c53-2d96-4269-8447-f1b4043c1a48

📥 Commits

Reviewing files that changed from the base of the PR and between bf5ad40 and 904285f.

📒 Files selected for processing (4)

docs/agents/e2e-testing.md
hack/e2e-chainsaw/_lib/remediation-guard.sh
hack/e2e-chainsaw/_lib/run-kubernetes.sh
hack/run-kubernetes-remediation_test.bats

🚧 Files skipped from review as they are similar to previous changes (2)

hack/run-kubernetes-remediation_test.bats
hack/e2e-chainsaw/_lib/remediation-guard.sh

coderabbitai

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/agents/e2e-testing.md (1)
46-47: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Remove the EXIT-trap exceptions from this convention.

The supplied path instruction requires no EXIT or RETURN traps. Lines 46-47 and 157 currently permit EXIT traps inside script steps and BATS subshells. Keep cleanup explicit, or update the governing instruction and all related examples together.

As per path instructions, use no EXIT or RETURN traps.

Also applies to: 157-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/agents/e2e-testing.md` around lines 46 - 47, Update the convention in
this document to prohibit all EXIT and RETURN traps, removing the exceptions for
single script steps and explicit BATS subshells. Revise the related guidance and
examples, including the references to hack/e2e-test-openapi.bats and sourced
shell libraries, so cleanup is performed explicitly without either trap type.
Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/agents/e2e-testing.md`:
- Line 65: Revise the guidance around .status.history to state that
helm-controller/api is only a pinned dependency and does not contain this
repository’s controller behavior for reading or truncating history. Keep claims
about retention and strategy-specific truncation explicitly attributed to the
upstream helm-controller source, while preserving the existing e2e timing and
shared remediation-guard helper guidance.
- Line 66: Update the paragraph describing the helmrelease_has_teardown rule to
match cozy_guard_helmrelease’s implemented behavior: document that an
uninstalled Snapshot is treated as unexplained only when
.spec.install.strategy.name is RetryOnFailure. Remove claims about
remediation.retries or upgrade remediation strategy unless the guard is expanded
to read and handle those fields.

In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh`:
- Around line 1001-1010: Bound every kubectl read in the guard helpers: wrap the
strategy and history reads near lines 1001-1010, the Ready read near lines
1025-1029, and the HelmRelease list read near lines 1103-1107 in timeout -k 5
30, adding --request-timeout=30s to each kubectl invocation.
- Around line 1995-2005: Wrap the parent HelmRelease wait for
“kubernetes-${test_name}” in a failure branch that, when kubectl wait fails,
prints the HelmRelease YAML/status conditions and relevant namespace events,
then exits immediately with failure. Keep the existing 5-minute readiness wait
and ensure the remediation guard does not run after a timeout.

---

Outside diff comments:
In `@docs/agents/e2e-testing.md`:
- Around line 46-47: Update the convention in this document to prohibit all EXIT
and RETURN traps, removing the exceptions for single script steps and explicit
BATS subshells. Revise the related guidance and examples, including the
references to hack/e2e-test-openapi.bats and sourced shell libraries, so cleanup
is performed explicitly without either trap type.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 179fb467-b311-4da5-8636-7508a55bb033

📥 Commits

Reviewing files that changed from the base of the PR and between 904285f and ed56999.

📒 Files selected for processing (2)

docs/agents/e2e-testing.md
hack/e2e-chainsaw/_lib/run-kubernetes.sh

IvanHunters

This PR splits the old single helmrelease_has_remediation_cycle reading into a narrow helmrelease_has_teardown (only an uninstalled Snapshot) and the existing broad reading, and uses .spec.install.strategy.name to decide which one is fatal. It also extends the guard from the parent HelmRelease to the tenant addon HelmReleases via cozy_guard_addon_helmreleases/cozy_guard_all_helmreleases in hack/e2e-chainsaw/_lib/run-kubernetes.sh. I verified the wiring, the ordering guarantee, and the test coverage directly against the PR head. I agree with the design. LGTM with a couple of non-blocking notes.

What I verified

cozy_guard_all_helmreleases tenant-test "kubernetes-${test_name}" is called at hack/e2e-chainsaw/_lib/run-kubernetes.sh:2029, and the tenant-snapshot trap - EXIT is at line 2048 — the guard genuinely runs before the trap is disarmed, matching the ordering claim in the new hack/run-kubernetes-remediation_test.bats test.
helmrelease_has_remediation_cycle in hack/e2e-chainsaw/_lib/remediation-guard.sh gains a missing local statuses declaration alongside the new helmrelease_has_teardown, closing a real global-variable leak — a genuine correctness fix, not just a comment change.
The new hack/run-kubernetes-remediation_test.bats fixture-driven kubectl stub distinguishes "read failed" (*.FAILS) from "field absent" (missing fixture) and from "kubectl warns on stderr but exits 0" (*.WARNS), and there are dedicated tests proving a stray stderr warning doesn't silently downgrade a real teardown or mask an empty history. That's the right level of paranoia for this kind of guard.
The ordering-pin test in hack/run-kubernetes-serial-console_test.bats was re-anchored from grep -n 'helmrelease_has_remediation_cycle "${history_statuses}"' to grep -n '^ *cozy_guard_all_helmreleases ', i.e. on the call site rather than on the callee's now-renamed local variables — this correctly avoids brittleness from the refactor.

Non-blocking / notes

One of the open CodeRabbit threads (hack/e2e-chainsaw/_lib/run-kubernetes.sh, originally line 751) claims remediation-guard.sh "is not sourced from the current repository and is not referenced by run-kubernetes.sh." That's incorrect — line 3 of run-kubernetes.sh at the very commit CodeRabbit reviewed and at current head both read . hack/e2e-chainsaw/_lib/remediation-guard.sh. No action needed on the code, but worth resolving/dismissing the thread explicitly so it doesn't linger as a false "unaddressed Major finding."
The new guard's kubectl reads (cozy_guard_helmrelease's strategy/history/Ready reads, cozy_guard_addon_helmreleases's listing) have no --request-timeout/timeout wrapper, unlike other reads added elsewhere in this file. Both a prior human review and a later CodeRabbit pass flagged this independently: a wedged apiserver at guard time (which runs late, near the tenant-snapshot trap) could stall and, per the guard's own doc comments, cost part of the diagnostic budget. Worth a follow-up.
docs/agents/e2e-testing.md's new bullet describes two configurations that can produce an uninstalled Snapshot (default-strategy install remediation, and an upgrade remediation set to strategy: uninstall), but cozy_guard_helmrelease only reads .spec.install.strategy.name — it doesn't check .spec.install.remediation.retries or .spec.upgrade.remediation.strategy. This gap is honestly self-documented in the code's own comment block ("the rule implemented is narrower than the rule stated..."), and no current addon is in the affected shape (retries: -1 everywhere), so it's a real but currently-inert limitation rather than a bug.
The plain kubectl wait hr -n tenant-test "kubernetes-${test_name}" --timeout=5m --for=condition=ready call (unchanged by this diff, only the surrounding comment was rewritten) still prints nothing but kubectl's own timeout message on failure. CodeRabbit raised this twice; it predates this PR and is arguably out of scope for a remediation-guard fix, but it's a reasonable follow-up since the guard right below it depends on this wait having succeeded.
Per the metadata, this PR is CONFLICTING against main and needs a rebase before it can merge — noted as a mergeability item, not a logic concern; nothing in the diff suggests the conflict hides a real semantic clash.

Overall: the core reasoning (RetryOnFailure is stamped on every Application-served HelmRelease, so a failed Snapshot there is not evidence of a teardown) is correctly implemented and heavily unit-tested. The open notes above are worth acting on before or shortly after merge, but none of them constitute a regression or a broken assertion.

myasnikovdaniil

The classification is right and the tests are the best part of this PR. Requesting changes on where the guard is wired in, and on the field that arms it.

On the tests first, because they deserve saying. I mutated 23 ways, gate removed, gate inverted, teardown predicate widened to failed, || true on each of the two reads, stderr folded into each capture, the addon loop swallowing verdicts, the composite dropping either verdict, read order swapped, index() traded for an anchored grep, the call deleted, the call suffixed with || true. Every one turns a test red, each with a case that exists for it. Both suites behave identically under bats and under hack/cozytest.sh, 24 of 24 and 12 of 12. Every repo fact the change rests on holds too: the parent gets RetryOnFailure on both install and upgrade, cilium and csi set it, all 19 addons set retries: -1, nothing under packages/ sets an uninstalling upgrade remediation, and both truncation variants behave as the comments say.

The guard cannot run in the case it was written for. Its own comment says a child remediation surfaces only as whatever downstream deadline it eventually breaks, a node that never turns Ready, a PVC that never binds, with nothing in the run naming the teardown that preceded it. The call is at run-kubernetes.sh:2520 and every one of those deadlines is upstream of it, the node-Ready wait at :1942 with exit 1 at :1954, the csi HelmRelease wait at :2156, then the LB and NFS checks. Under set -eu the script ends there.

This PR's own CI is the demonstration. Tenant cilium never went Ready, coredns, csi, ingress-nginx, metrics-server and ouroboros all sat on dependency '...-cilium' is not ready, the run died at the node-join deadline, and cozy_guard_all_helmreleases was never reached. Had cilium been torn down instead of retried the output would have been identical and the teardown still unnamed. cozy_report_node_join_failure prints each HelmRelease's Ready message and not .status.history, so the failing path is exactly as blind as before this PR. Calling a print-only classification from cozy_report_node_join_failure, or from _tenant_snapshot_on_fail at :1915, closes it. If the happy-path-only placement is deliberate then say so in the comment and in the docs bullet, because both currently claim more.

The field that arms the guard fails soft and nothing pins it. The fatal verdict is a string equality against a value read by kubectl -o jsonpath, and with --allow-missing-template-keys at its default a path that stops resolving prints nothing and exits 0, so empty takes the soft branch. That is the same class of silent failure the empty-history check exists to catch, applied to .status.history and not to the new read. Bump helm-controller/api past a rename of spec.install.strategy and every teardown, including a real removal of the tenant CNI, becomes a NOTE about its own install remediation, green run, all 24 unit tests still passing because they feed the strategy from a fixture rather than through the jsonpath. The printed evidence does not help either, (install strategy <unset>) is the expected line for 17 of the 19 addons. Worth knowing the existing pin does not cover this shape, remediation-guard.bats:143 pins .status.history[].status against a hand-written YAML fixture rather than against the module in go.mod, so the docs claim that the shape is pinned against helm-controller/api v2 is stronger than what that test does. Cheapest fix is a canary, the parent is RetryOnFailure unconditionally so an empty strategy on the parent should be fatal.

Smaller things, none blocking alone. The retries: 0 gap you document at :1596-1605 is one more field in the same jsonpath, appending @{.spec.install.remediation.retries} and treating either condition as fatal closes it and deletes ten lines explaining why it is open. The enumerate-from-the-cluster design was chosen precisely so a release added later is covered without editing this file, and that is the shape where it does not hold. The read-order argument at :1543-1546 says no truncation variant can empty a non-empty history, which is true, but the controller also calls Status.ClearHistory() outright on an unmanaged release, so if that lands between the two reads the guard sees Ready true and empty history and fails the run with the wrong explanation. Tiny window, but the claim should be the narrower one. And it is worth naming in the docs that three releases out of twenty can actually fail the run on history content, since for the other seventeen every history shape is a note. That is a defensible design, it just is not what covering the tenant addon HelmReleases reads as.

On the two failure directions. A real failure read as a fine teardown is reachable and you relaxed it deliberately, failed is now a note everywhere, and the justification is correct against the controller since an active retry returns an upgrade rather than uninstall remediation. But this run shows the limit of recovered by design, cilium logged UpgradeFailed ... stalled resources: [Deployment/cozy-cilium/cilium-operator status: 'Failed'] eleven times over ten minutes and never recovered, and in that state the guard prints its failed-Snapshot note and returns 0. Right call for this guard, the run is already red, but retried and wedged in a retry loop are the same output and the wording is the only difference. The opposite direction is effectively unreachable, a release pairing RetryOnFailure with an uninstalling upgrade remediation would trip the fatal branch on a removal it is configured to perform, and nothing under packages/ does that.

The red E2E is not yours. Both suites fail at the node-join deadline behind tenant cilium stuck on cilium-operator, #3780 which only moves documentation fails identically, #3790 shows the same cilium signature, and nightlies have been red seven days running. Everything this PR touches in the run lives below the line where those runs die, which is also why this code has never executed on a cluster, and why the placement point above matters more than it otherwise would.

myasnikovdaniil · 2026-08-14T07:46:23Z

+  # cozy_guard_all_helmreleases judges the parent and the addons and reports
+  # both verdicts rather than stopping at the parent's; see its comment for why
+  # that matters under errexit.
+  cozy_guard_all_helmreleases tenant-test "kubernetes-${test_name}"


This is the placement I am blocking on. The function's own comment says a child remediation surfaces only as whatever downstream deadline it eventually breaks, a node that never turns Ready, a PVC that never binds. All of those are upstream of this line: the node-Ready wait at :1942 with its exit 1 at :1954, the csi HelmRelease wait at :2156, then LB and NFS. Under set -eu the script ends at the first of them.

So on this PR's own run the guard never executed. Tenant cilium never reached Ready, the addons sat on dependency '...-cilium' is not ready, and the run died at the node-join deadline. A teardown would have produced the same output and still gone unnamed, because cozy_report_node_join_failure prints Ready messages and not .status.history.

A print-only classification called from cozy_report_node_join_failure, or from _tenant_snapshot_on_fail at :1915, would give the failing path the coverage the comment promises. If happy-path-only is deliberate, the comment and the docs bullet should say so.

You are right, and it took two changes. The print-only classification you suggested is cozy_report_helmrelease_remediation, called from cozy_report_node_join_failure and from the EXIT handler that covers every other failure. It reuses the guard's own enumeration rather than a second copy of the selection, so the two readings cannot drift apart.

The second change your comment led me to and I had missed. The EXIT handler was armed only once the tenant LB answered, so even with the collector in place it would have been absent from the whole stretch between the CR apply and that point, which is management-cluster waits on the parent HelmRelease and on what it installs. That stretch is the window the guard exists for: a helm-wait budget expiring while admin-kubeconfig is still being provisioned is what triggers the uninstall remediation it reads. It arms at the apply now, in 91cbb69, and the snapshot half stays a no-op until the tenant kubeconfig exists.

Inside the node-join collector the classification runs after the serial console rather than before it (98f7d8c). The console is the only artefact covering a worker that never reached apid, and its worst case takes most of the phase budget, so anything ahead of it competes with it on the slow runs it exists for. What the classification reads outlives the run and can be read again afterwards; the console cannot. Both walks are bounded by a request timeout and a wall clock checked before the listing and before each release, and a release the budget cut off is named rather than dropped.

Head is 98f7d8c, rebased onto current main. The classification, its budget, the arming point, the collector order and the wiring are pinned in hack/run-kubernetes-remediation_test.bats and in the two order guards in hack/run-kubernetes-node-join_test.bats.

Rebased since my last reply, so the line numbers in your comment and the SHAs in mine no longer resolve. Current positions on the head you would read now.

The EXIT handler arms at 5357, right after the tenant CR apply, instead of once the tenant LB answers. Everything you named sits below that point: the node-Ready wait and its failure path at 5523-5540, the csi HelmRelease wait at 5789. Under errexit each of them now enters _tenant_failure_on_exit rather than ending the script above the classification.

cozy_report_helmrelease_remediation is at 5122 and is reached from two places: line 412 inside the EXIT handler, ahead of the tenant snapshot, and line 4422 inside cozy_report_node_join_failure, after the serial console capture at 4398. Print-only holds the way the comment promises: every cozy_guard_helmrelease call inside it is || true, it returns 0 on all paths including the budget cut-offs, and both call sites are || true as well.

Your 1942, 1954 and 2156 are from before the rebase and the file was rewritten around them.

Rebased onto current main. The file was rewritten under the branch, so the line numbers in my earlier reply no longer point anywhere.

The wiring you blocked on is pinned by tests now. cozy_report_helmrelease_remediation is reached from cozy_report_node_join_failure and from the EXIT handler, and the handler is armed at the tenant CR apply, above every wait you named.

Nothing covered the assignment itself. The handler classifies whatever CURRENT_TENANT_PARENT_HR holds, so deleting that one line leaves it empty: the collector returns on its empty-parent gate, every failure path goes quiet, and the suite stays green. There is a test for that now, and for the assignment landing before the arming.

myasnikovdaniil · 2026-08-15T08:11:08Z

Re-read this against head while my block was standing, and found one thing I missed the first time that I think changes the approach rather than the diff.

The condition this makes fatal cannot happen. In helm-controller v1.5.1, internal/reconcile/atomic_release.go under case ReleaseStatusFailed: returns NewUpgrade(...) as soon as GetActiveRetry() != nil, which is before GetActiveRemediation() and before the remediation switch is reached at all. So a release carrying RetryOnFailure never remediates and never lands in uninstalled. Meanwhile failed, which the guard stops treating as fatal, was reachable. Net in-product detection on parent, cilium and csi goes to zero, which is the opposite of what the PR is for.

Two smaller things while I was in there. The guard call still sits after every failure exit in the file, so on the failure path its own comment names as the motivation it does not run, which is the block I already have on that line and it is unaddressed at head. And the new reads at :1547, :1554 and :1655 carry no --request-timeout in a file that uses one on 31 other reads, so that is 8 to 25 unbounded unretried gets added to the tail of a 50m op.

The discriminator itself is fine, for the record. ^uninstalled$ against ^(failed|uninstalled)$ genuinely takes different branches, and the test suite around it is real: I mutated it seventeen ways, including inverting the discriminator, the strategy gate, the read order and the || true on the wired call, and each one was caught by a named test, with declared equal to executed on all four bats files.

IvanHunters

LGTM.

The teardown-vs-retry distinction is verified against upstream helm-controller/api v1.5.1, not against the PR narrative: .spec.install.strategy.name is a real field, the apiserver stamps it, and cilium/csi set RetryOnFailure while the other addons use retries: -1. The flake is not masked, the opposite: the parent readiness wait is kept and hardened, and addon teardown (cilium/csi) now fails the run. The only constructible misclassification yields a false RED (fail-safe), never a false GREEN. Tests are 39 cases, non-vacuous, and BATS-correct.

[MINOR] run-kubernetes.sh teardown branch does not read .spec.upgrade.remediation.strategy, a latent false RED if someone adds upgrade-remediation uninstall on a RetryOnFailure release. Does not fire today, already commented.

IvanHunters

Verdict

LGTM

The teardown-vs-retry split is right, the failure-path coverage myasnikovdaniil asked for is genuinely there, and the tests are the strongest part of the change. Three non-blocking notes and two nits below, two of which my earlier pass on this same head missed. None of them is a fail-open, a product regression, or a test that cannot fail, so none of them holds the review.

The branch cannot merge as it stands, which is a separate matter from the verdict and is listed under "Before merge".

Resolving the stale CHANGES_REQUESTED

myasnikovdaniil reviewed 9ca986f on 2026-08-14. The branch was force-pushed on 2026-08-20 and GitHub still reports his verdict against code that no longer exists, so before reading anything I checked whether the contribution actually moved or whether this was a rebase that left the substance alone.

It moved. Against the shared merge-base 68d83cb, the two revisions' contributions differ on 8 of 10 files, including both shell files that carry the logic. The revision he reviewed had 2 commits; the head has 6. His review is stale on the merits, not just on the SHA.

Point by point.

The blocking one, "the guard cannot run in the case it was written for" (run-kubernetes.sh:2520 then, :6006 now), is addressed. cozy_report_helmrelease_remediation at hack/e2e-chainsaw/_lib/run-kubernetes.sh:4999 is the print-only classification he suggested, called from the node-join collector at :4417 and from the EXIT handler at :390. It fails nothing on purpose, so it cannot displace the caller's exit status that the tenant snapshot hangs off, and it shares the guard's own enumeration (cozy_addon_helmrelease_names, :4848) rather than a second copy of the selection.

His comment also produced a fix he did not ask for. The EXIT handler used to arm only once the tenant answered; it now arms at the Kubernetes CR apply, :5217, which brings the whole management-cluster wait stretch under it: KamajiControlPlane, control-plane Deployments, MachineDeployment, admin-kubeconfig. That stretch is where a helm-wait budget expiring produces the uninstall remediation the guard reads, so it was the wrong window to be missing.

"The field that arms the guard fails soft and nothing pins it" is addressed. :4754-4758 fails the run when .spec.install.strategy.name reads back empty on the parent, and the message names the read as the suspect rather than the object. Only the parent carries the check, which is the right call: most of the chart's addons legitimately set no strategy, while the parent is stamped RetryOnFailure unconditionally at pkg/registry/apps/application/rest.go:1601-1611. Case 26 pins it; delete the branch and it goes red.

The retries: 0 gap is closed. The retry budget rides in the same jsonpath as the strategy and readiness (:4720), and :4816 fails a teardown on an empty-or-zero budget. Ten lines of "why this is open" are gone, and case 25 pins what replaced them.

The ClearHistory() objection landed too. :4707-4713 now says the read order buys the truncation half and nothing else, and names ClearHistory() as the window it cannot close.

So did the point about remediation-guard.bats:143. :5986-5990 states that the case covers a hand-written document rather than the module in go.mod, and ends with "Do not read it as a pin against upstream."

He asked for the docs to say that three releases out of twenty can actually red a run. docs/agents/e2e-testing.md goes further: it separates the teardown rule (parent, cilium, csi) from the four other conditions that red a run, and explains why the denominator is not fixed.

One of his observations is simply no longer true. "This code has never executed on a cluster": E2E (in-tree) is green at 54a914a, the current head, over a 2h24m run.

And one thing he raised is deliberately not done. failed is a note everywhere with no measurement behind the relaxation. It sits in the PR body under "Open questions" as an unresolved disagreement instead of being quietly settled, which is the honest way to carry it.

CodeRabbit, for completeness. The unbounded-reads finding is closed by _cozy_guard_kubectl (:4627) and pinned by case 24. The readiness-wait backstop is closed at :5970-5979. The docs/implementation mismatch is closed by the guard now reading both fields. The "remediation-guard.sh is not sourced" thread was wrong when filed (run-kubernetes.sh:3 sources it) and is resolved.

My own two earlier non-blocking notes are closed as well: the global statuses leak (remediation-guard.sh:73, :86) and the regex-vs-literal prefix, where :4865 now uses awk 'index($0, p) == 1' with case 39 pinning it.

What I checked

51 of 51 cases green across hack/remediation-guard.bats and hack/run-kubernetes-remediation_test.bats.

Eleven mutations, every one caught: guard call deleted; guard call suffixed with || true; teardown predicate widened to failed; parent strategy canary removed; zero-budget fatal branch removed; empty-history-on-Ready branch defanged; node-join classification call deleted; EXIT-handler classification call deleted; early trap arming reverted; the report's own deadline check removed; the once-per-run gate removed. No vacuous negatives either, every "not detected" case has a positive twin.

No fail-open path reaches green on a real teardown. The one construction that would, a spec.install.strategy rename downgrading every teardown to a note, is closed by the parent canary, and I confirmed both facts it rests on: the apiserver stamps the strategy unconditionally, and only cilium.yaml and csi.yaml set one in the chart. Every other misreading I could build produces a false red.

No hidden caps. No head -n, no top-N, no sampling anywhere in the guard or the report. The addon loop keeps inspecting after a failure, and the report names what its budget cut off instead of dropping it.

Before merge

The branch does not merge and main moved under exactly this file, so this is not a routine rebase. mergeable_state is dirty. Seven of the ten files were also touched on main since the merge-base, hack/e2e-chainsaw/_lib/run-kubernetes.sh among them, and the main-side commits are not incidental to this change: 7cca3d75e removed the FluxCD tenant addon, ad62a8d62 softened the node-join path and moved its deadline from 18m to 29m while re-deriving the phase budget, and f5b957ee2 (#3315) restructured the worker-pool section. This PR re-anchors two order pins and adds a third, all of which read line positions in that file, and it hardcodes 18m at :681, :4119 and :5363 where main now says 29m. Expect to re-derive the pins rather than just resolve hunks, and the stale counts below want fixing in the same pass. Worth a second look at the rebased result, since what lands is not what has been reviewed.

Findings

[MINOR] hack/run-kubernetes-runner-canary_test.bats:1575, the re-anchored pin is weaker than the sentence above it. The anchor moved from trap '_tenant_snapshot_on_fail' EXIT, which sat one line after CURRENT_TENANT_KC= in main, to trap '_tenant_failure_on_exit' EXIT, which now sits at the CR apply. In head that is line 5217 against 5313 for the kubeconfig assignment, so the bound at :1649 admits the whole window 5218 to 5312: the namespace wait, the KamajiControlPlane wait, the 10m kubeconfig wait, the 4m control-plane wait, the 5m MachineDeployment wait, the LB wait and the /healthz wait. The comment at :1644-1648 says the point is to reject a sample "taken before the tenant is even up", and the message at :1650 says such a sample would "describe the run before the cluster it is meant to characterise exists". Neither is true of what the anchor now enforces. Nothing violates it today, so this is latent. Re-anchoring on ^ CURRENT_TENANT_KC= restores the bound the comment describes. The re-anchor itself was forced, since the old string is gone, so this is about which of two candidates got picked.

[MINOR] Four counts in the new comments and docs are already wrong against main. Main carries 18 addon HelmRelease documents in 18 files, all with retries: -1, and only cilium.yaml and csi.yaml set a strategy. So :4746-4748, "18 of the 20 addon releases of this chart set no strategy ... (20 rather than 19 files: fluxcd.yaml declares two)", should read 16 of 18, and fluxcd.yaml no longer exists at all. :4805-4806, "all 20 addon releases set retries: -1", should read 18. docs/agents/e2e-testing.md, "the other eighteen addon releases the chart declares", should read sixteen. The "three releases can red a run on a teardown" claim survives (parent, cilium, csi), so only the denominators move. These were true when written, and the rebase is where they get fixed.

[MINOR] The guard's fail-fast read count went from 1 to 39, unretried, at minute 25. One listing plus two reads per release, over the parent and 18 addons, each bounded by --request-timeout=20s and a 20s wall clock, and each reddening the whole run on any non-zero exit (:4721, :4732, :4856). One transient Error from server on any of the 39 is a red run, and the suite's documented dominant flake is a degraded management apiserver, which is the same failure mode. The direction is fail-safe and the bound is what keeps a wedged apiserver from eating the tenant snapshot, and docs/agents/e2e-testing.md does limit retry to steps with no test logic. But a read that failed and a field that is absent are already told apart here by exit status, so retrying only the former is not the same thing as retrying over a verdict. This deserves a decision rather than another cycle as an open question.

[NIT] hack/run-kubernetes-node-join_test.bats:1668, the in-block filter has no upper bound. [ "$line" -gt "$block" ] || continue selects everything below cozy_report_node_join_failure() at :4097, which includes run_kubernetes_test and the guard functions. The comment claims "Only the calls inside the diagnostics block" and "a second call added INSIDE the block still has to appear in the list", which needs an end-of-function bound to be true. It errs strict, since an outside call would be pulled into the expected spend order and red the test, so it is safe as written. Just not what it says.

[NIT] A rename of the Ready-condition jsonpath silently retires the empty-history canary. :4720 reads .status.conditions[?(@.type=="Ready")].status through the same soft-failing jsonpath as the strategy. Empty means not-Ready, and not-Ready with an empty history returns green at :4774 as "not inspected". So a selector that stops resolving turns off the "Ready with no history is a status shape I can no longer read" check across every release, with nothing to notice. The parent canary at :4754 covers the strategy half of that read and not this half. Second-order: a release with a real teardown always has a non-empty history, so the teardown verdict itself is unaffected.

Caveats

Static review, no cluster. I ran the two guard suites in full (51/51), the four node-join pins this PR touches or extends, and the two re-anchored pins in hack/run-kubernetes-serial-console_test.bats and hack/run-kubernetes-runner-canary_test.bats. All green. I did not run the remaining cases in those three suites to completion.

The reason is worth recording separately, because it is not this PR's doing. cozy_diag_read and _cozy_guard_kubectl both invoke kubectl through timeout, which is an external binary, so the kubectl() { :; } shell stub at hack/run-kubernetes-node-join_test.bats:911 is bypassed and the real kubectl from PATH runs instead. On a machine with a live kubeconfig that one case takes over sixteen minutes. I counted 187 real kubectl invocations out of it, of which 9 belong to the reads this PR adds and the rest are pre-existing cozy_diag_read calls. Not a finding against this change, but the unit lane is reaching whatever context the developer has current, and a PATH-based stub would end that.

Phase 5b (existing-customer upgrade, fresh install) is N/A here: nothing under packages/, no chart, no migration, no RBAC, no CRD, no user-facing schema.

The sweep that keeps every quoted copy of the node-join deadline equal to the wait counted "become Ready", "became Ready" and "Ready within" as referents. None of them names the node-join. They are what a HelmRelease, a LoadBalancer or a PVC is also said to do, each on a deadline of its own, so any readiness message carrying a minute figure was read as a stale copy of the 29m wait -- correct about both waits and a defect in neither. The referents are enumerated rather than the exceptions, because the set of other things that become Ready on a clock is open and grows with the suite, while the ways this tree spells the node-join are not. The one line that relied on the bare spelling asserts the subject along with the figure now. It reads the warning annotation, which says "fewer than 2 tenant nodes Ready within 29m", so naming the nodes is both a tighter assertion and the thing that keeps the line inside the sweep. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

A `failed` and an `uninstalled` Snapshot in .status.history were read as one signal. That holds under Flux's default RemediateOnFailure, where a failed install is remediated by uninstalling the release, and it is wrong under RetryOnFailure, where the failed attempt keeps its manifests applied and is retried as an upgrade: there nothing was removed. helmrelease_has_teardown is the narrow reading, an "uninstalled" Snapshot, which is proof the release was removed and put back. helmrelease_has_remediation_cycle keeps its meaning and its breadth, since it also catches the upgrade remediation that rolls back rather than uninstalls, at the price of not proving anything was removed. Neither takes the release's configuration: the history says what happened, the configuration says what the release could have done about it, and weighing the two is the caller's decision. Both helpers now scope "statuses". The library is sourced rather than executed, so calling either one left the argument behind in the caller's scope. The header also said a footprint survives a later successful reconcile and stopped there, which reads as unconditional. The controller truncates history on every in-sync reconcile, so a caller reading it further from the action is reading a window. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

…not reach The guard judged the parent release alone. A teardown of the tenant CNI or CSI surfaced only as whatever downstream deadline it eventually broke, with the removal that preceded it named nowhere. cozy_guard_helmrelease judges one release and the addon walk runs it over the releases the parent installs, enumerated from the cluster so a release the chart adds later is covered without editing this file. One rule decides the verdict: the release was removed although nothing in its own configuration would remove it to recover. That is two fields, the install strategy and the retry budget, because the default strategy uninstalls only between the attempts its budget allows and performs none at a budget of zero. The guard sits after the assertions, and every deadline whose breach it explains ends the script under errexit well before it, so on exactly the runs a teardown caused, it never ran. cozy_report_helmrelease_remediation is the same classification with no verdict, called from the node-join collector and from the exit handler, once per run. The handler is armed at the CR apply rather than once the tenant answers: everything between is a management-cluster wait on the parent release, and that is the window a helm-wait budget expiring produces. Every read is bounded by a request timeout and a wall clock, and keeps its exit status, because a read that failed and a release with no history both print nothing. Readiness and the configuration are read before the history, or a release completing its install between the two reads presents as Ready with an empty history. A read the wall clock cut off is named as such rather than pointed at a kubectl error that was never printed. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

kubectl is answered from fixtures, so the cases cover the wiring the guard depends on without a cluster: the prefix that selects the addons read literally rather than as a regex, readiness interpreting an empty history in both directions, the read order via a fixture that turns a release Ready between the two reads, both fatal teardown branches, the wording of every note, and the probe-failure paths kept apart from a field the object does not carry. The failure paths get their own cases: the classification runs once per run from any of its three call sites, stops at its budget naming the release it stopped at, and does not spend that one classification on a call that printed nothing. The exit handler is pinned at both ends, the status it captures and the status the snapshot receives, since stubbing either end hides a regression in the other. Four lexical pins hold the wiring itself, because with the guards behind functions, deleting the calls or moving them past the trap would leave every behavioural case green while the run asserted nothing. The serial-console pin moves onto the call it anchors on, off the text of arguments this change renames. Stubs go on PATH rather than into shell functions: a hyphen is legal in a command name and not in a function name, and this file is executed by two runners, one of which sources it into /bin/sh. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

The conventions described the guard as parent-only, the footprint as surviving indefinitely, and the teardown rule as the install strategy alone. All three moved: the guard reads every tenant addon release, the controller truncates history on every in-sync reconcile, and the rule weighs the retry budget beside the strategy. Breadth of reading and breadth of gate are separate numbers and are now separate sentences: three releases can fail a run on a teardown, while every release is read and classified. The failure paths get their own bullet, since a guard that runs after the assertions is absent from the runs it exists for unless a no-verdict twin covers them. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

The classification ran ahead of the guest serial console in the node-join block, on the rule that cheap reads precede collectors costing minutes. That rule is wrong here, and the ordering follows the evidence instead. The console is the only artefact covering a worker that never reached apid, which is the dominant shape of this failure and one nothing else in the block describes. Its measured worst case is most of the phase budget, so anything ahead of it competes with it on exactly the slow runs it exists for. The classification reads objects on the management cluster that outlive the run, so a walk the budget declines can still be answered afterwards. Irreplaceable evidence first, whichever is cheaper. Its section letter moves to (e): the block already had a (d). Both suites' spend-order comments list it in its new position, and the two guards over that order now match calls by position inside the block rather than by name. The same collector is called from the exit handler, which belongs to no phase, and by-name matching either mistook that call for one in the block or would have had to exempt the name outright, which would also have exempted a future call genuinely placed ahead of the console. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

…dler The guard bounds the pre-wait canary sample below by the line that installs the tenant failure handler, and grepped for that line under the snapshot function's name. The snapshot is now reached through the handler rather than installed directly, so the anchor read empty and the guard refused its input. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

The sweep read the directory, so a backup sitting beside the file it shadows -- what `sed -i.bak` and most editors leave behind -- supplied a second copy of every quotation that file holds. Those copies carry the right figure and pass the staleness check; what they move is the count the floor tests. A stray file lifted the count over the floor while real coverage sat under it, hiding the one failure the floor is there to catch. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

Each of these could be deleted or weakened without a single test changing its verdict, which left the behaviour they carry resting on review alone. The parent release name is the sharpest. The exit handler classifies whatever CURRENT_TENANT_PARENT_HR holds, so dropping that assignment leaves it empty, the collector returns on its empty-parent gate, and every failure path goes quiet while the suite stays green. The assignment is now pinned as present and as preceding the arming, since a failure landing between the two lines enters the handler with the name still unset. The 137 arm of the read-failure note had no fixture that could reach it: the kubectl stub produced 124 and 1 and nothing else, so the sentence separating a killed read from one kubectl explained was unreachable. A KILLED sentinel gives it one. The collector's suppression was covered where the parent carries the verdict but not where an addon does, because the existing case leaves its addon a note rather than a verdict. And the addon prefix was pinned as literal and as excluding foreign releases, neither of which separates a prefix reading from a substring one: both listings stay selected either way, so a name merely embedding the prefix would have been pulled in unnoticed. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

myasnikovdaniil · 2026-09-10T12:00:16Z

My objection to making failed non-fatal still blocks this PR. I accepted the classification in my first review, then reconsidered the approach after checking the controller path.

For parent, cilium and csi, RetryOnFailure retries through upgrade without uninstalling, so failing only on uninstalled drops the reachable failure signal. I didn't ask for a blanket failed or uninstalled rule across every addon. Please keep the check able to fail on these failed attempts and resolve this before merge, the Open questions section leaves my objection unaddressed.

Aleksei Sviridkin (lexfrei) requested a review from myasnikovdaniil as a code owner August 7, 2026 04:49

github-actions Bot added size/XL This PR changes 500-999 lines, ignoring generated files area/testing Issues or PRs related to testing (e2e, bats, unit tests) kind/bug Categorizes issue or PR as related to a bug labels Aug 7, 2026

Aleksei Sviridkin (lexfrei) closed this Aug 7, 2026

Aleksei Sviridkin (lexfrei) reopened this Aug 7, 2026

IvanHunters previously approved these changes Aug 7, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) dismissed IvanHunters’s stale review via bf5ad40 August 7, 2026 09:43

Aleksei Sviridkin (lexfrei) force-pushed the fix/remediation-guard-distinguish-teardown branch from e85e915 to bf5ad40 Compare August 7, 2026 09:43

Aleksei Sviridkin (lexfrei) closed this Aug 7, 2026

Aleksei Sviridkin (lexfrei) reopened this Aug 7, 2026

Aleksei Sviridkin (lexfrei) force-pushed the fix/remediation-guard-distinguish-teardown branch from bf5ad40 to 904285f Compare August 7, 2026 12:38

github-actions Bot added size/XXL This PR changes 1000+ lines, ignoring generated files and removed size/XL This PR changes 500-999 lines, ignoring generated files labels Aug 7, 2026

coderabbitai Bot reviewed Aug 7, 2026

View reviewed changes

Comment thread hack/e2e-chainsaw/_lib/run-kubernetes.sh

coderabbitai Bot mentioned this pull request Aug 7, 2026

test(e2e): pin the release-history read against the upstream type #3607

Merged

11 tasks

Aleksei Sviridkin (lexfrei) force-pushed the fix/remediation-guard-distinguish-teardown branch from 904285f to ed56999 Compare August 9, 2026 01:29

coderabbitai Bot reviewed Aug 9, 2026

View reviewed changes

Comment thread docs/agents/e2e-testing.md

Comment thread docs/agents/e2e-testing.md Outdated

Comment thread hack/e2e-chainsaw/_lib/run-kubernetes.sh Outdated

Comment thread hack/e2e-chainsaw/_lib/run-kubernetes.sh Outdated

IvanHunters reviewed Aug 10, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) force-pushed the fix/remediation-guard-distinguish-teardown branch from 8cd6277 to 9ca986f Compare August 12, 2026 18:50

Aleksei Sviridkin (lexfrei) requested review from Andrei Kvapil (kvaps) and Timofei Larkin (lllamnyp) as code owners August 12, 2026 18:50

myasnikovdaniil requested changes Aug 14, 2026

View reviewed changes

coderabbitai Bot mentioned this pull request Aug 15, 2026

test(e2e): hold the tenant-cluster install wait to an ordering rule #3802

Merged

Aleksei Sviridkin (lexfrei) force-pushed the fix/remediation-guard-distinguish-teardown branch 3 times, most recently from 4e822f1 to f6262a6 Compare August 16, 2026 18:03

Aleksei Sviridkin (lexfrei) force-pushed the fix/remediation-guard-distinguish-teardown branch 2 times, most recently from dfa549a to 2aaa149 Compare August 17, 2026 00:10

Aleksei Sviridkin (lexfrei) self-assigned this Aug 17, 2026

Aleksei Sviridkin (lexfrei) force-pushed the fix/remediation-guard-distinguish-teardown branch from 2aaa149 to d8a2836 Compare August 20, 2026 08:46

IvanHunters approved these changes Aug 21, 2026

View reviewed changes

IvanHunters previously approved these changes Sep 3, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) dismissed IvanHunters’s stale review via fb9b8c5 September 7, 2026 01:29

Aleksei Sviridkin (lexfrei) force-pushed the fix/remediation-guard-distinguish-teardown branch from 54a914a to fb9b8c5 Compare September 7, 2026 01:29

Aleksei Sviridkin (lexfrei) requested review from IvanHunters and myasnikovdaniil September 7, 2026 01:33

Aleksei Sviridkin (lexfrei) mentioned this pull request Sep 7, 2026

e2e: 50 kubectl reads inside the armed EXIT-handler window carry no time bound #4111

Open

Aleksei Sviridkin (lexfrei) added 9 commits September 7, 2026 23:38

Aleksei Sviridkin (lexfrei) force-pushed the fix/remediation-guard-distinguish-teardown branch from c2d3dfa to 8c3fce1 Compare September 7, 2026 22:36

Aleksei Sviridkin (lexfrei) requested a review from Timur Tukaev (tym83) as a code owner September 7, 2026 22:36

Aleksei Sviridkin (lexfrei) mentioned this pull request Sep 8, 2026

check-rd-presets reports a present preset as missing when printf loses a SIGPIPE race under pipefail #4153

Closed

Conversation

Aleksei Sviridkin (lexfrei) commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

What fails a run

The paths where the guard cannot run

How the reads are made

Tests and docs

Two open issues affected without being closed

Open questions

A test outside this change

Screenshots

Downstream repositories

Release note

Uh oh!

coderabbitai Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

Aleksei Sviridkin (lexfrei) commented Aug 7, 2026

Uh oh!

IvanHunters left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IvanHunters left a comment

Choose a reason for hiding this comment

Uh oh!

myasnikovdaniil left a comment

Choose a reason for hiding this comment

Uh oh!

myasnikovdaniil Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Aleksei Sviridkin (lexfrei) Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

Aleksei Sviridkin (lexfrei) Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

Aleksei Sviridkin (lexfrei) Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

myasnikovdaniil commented Aug 15, 2026

Uh oh!

IvanHunters left a comment

Choose a reason for hiding this comment

Uh oh!

IvanHunters left a comment

Choose a reason for hiding this comment

Verdict

Resolving the stale CHANGES_REQUESTED

What I checked

Before merge

Findings

Caveats

Uh oh!

myasnikovdaniil commented Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Aleksei Sviridkin (lexfrei) commented Aug 7, 2026 •

edited

Loading

coderabbitai Bot commented Aug 7, 2026 •

edited

Loading