Skip to content

Commit bf5ad40

Browse files
lexfreilexfrei
committed
test(e2e): guard the tenant addon HelmReleases against teardown
The remediation guard ran over the parent kubernetes-<test> HelmRelease only, and the parent's .status.history says nothing about the addon releases it installs. A teardown of one of them therefore surfaced as whatever downstream deadline it eventually broke - a node that never turns Ready, a PVC that never binds - with nothing in the run naming the teardown that preceded it. The CNI and CSI releases are the sharp cases, because their teardown removes a cluster-wide precondition rather than churning a single component. cozy_guard_helmrelease reads one release's history and judges it; cozy_guard_addon_helmreleases runs it over the addon releases the parent installs. Both the parent block and the addon loop go through the same function, so the parent's reads are exercised by the same unit tests rather than sitting inline in run_kubernetes_test, where a revert of them stays green. The addon list comes 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. One rule decides the verdict: fail when the release was REMOVED although it is configured never to remove itself to recover. An uninstalled Snapshot is proof of removal, and two configurations write it: the default strategy's install remediation, and an upgrade remediation set to strategy: uninstall, which nothing under packages/ uses. So on a RetryOnFailure release, which is the tenant cilium and csi and every Application HelmRelease, that Snapshot has no explanation inside the release and fails the run. On a release still using the default strategy the same Snapshot is that strategy doing its documented job, paired with retries: -1, and it is reported instead. So is a failed Snapshot, which under RetryOnFailure is not even remediation. 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 ends up switched off; that the configuration is itself a defect is filed separately and is not this guard's to assert. Both probes keep their exit status and neither folds kubectl's stderr into the value it captures. A read that failed and a release with no history both print nothing on stdout, so folding those together would let an API timeout pass for the latter; and kubectl writes warnings to stderr while exiting 0, which inside the history capture turns an empty history into a populated one, and inside the strategy capture stops the value matching RetryOnFailure and downgrades a real teardown to a note. An empty history is judged against that release's own Ready condition rather than the parent's: the parent carries DisableWait on both actions, from the annotation its ApplicationDefinition sets, so it 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, which is reported as not inspected rather than failed or passed over in silence. Every addon is inspected even after one of them fails, because the notes the others print - a rollback here, a teardown the default strategy performed there - are the context that explains the failure, and stopping at the first one hides what a reader would compare it against. It adds no wait and no retry, and both calls run before the tenant-snapshot trap is disarmed, so a teardown either one finds still captures the tenant cluster. A test pins that wiring by reading the script: with the guards behind functions, deleting the two calls or moving them past the trap would otherwise leave every case in the file green while the run asserted nothing. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
1 parent 5e24cff commit bf5ad40

3 files changed

Lines changed: 743 additions & 37 deletions

File tree

‎docs/agents/e2e-testing.md‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,15 @@ The install test fails if **any** HelmRelease is not Ready. A toothless gate (ba
5858
- Use a single `kubectl wait hr --all -A`, then an outcome-based re-list (covers HRs created after the snapshot), and `exit 1` on any non-Ready HR.
5959
- Dump the full Ready-condition message per non-Ready HR so the real error is in the test output. See `hack/e2e-install-cozystack.bats`.
6060

61-
### 6. Assert the parent HelmRelease did not remediate — via `status.history`
61+
### 6. Assert no HelmRelease was torn down against its own configuration — via `status.history`
6262

63-
A parent HelmRelease that hit its wait timeout, uninstalled, and reinstalled is a silent race we want to catch. **Do not** check `.status.installFailures` / `.status.upgradeFailures`: Flux's `ClearFailures()` zeroes those on every successful reconcile, so checking them after the HR is Ready is **vacuous** and passes against a reverted fix.
63+
A HelmRelease that hit its wait timeout, uninstalled, and reinstalled is a silent race we want to catch — and on the releases that no longer recover that way, it is a race with no explanation inside the release at all. **Do not** check `.status.installFailures` / `.status.upgradeFailures`: Flux's `ClearFailures()` zeroes those on every successful reconcile, so checking them after the HR is Ready is **vacuous** and passes against a reverted fix.
6464

65-
- Inspect `.status.history` instead — a `failed` or `uninstalled` Snapshot survives a later successful reconcile. Use the shared helper in `hack/e2e-chainsaw/_lib/remediation-guard.sh` (`helmrelease_has_remediation_cycle`) from a `script` step.
65+
- Inspect `.status.history` instead — a `failed` or `uninstalled` Snapshot survives a later successful reconcile. Use the shared helpers in `hack/e2e-chainsaw/_lib/remediation-guard.sh` from a `script` step.
66+
- **Fail on a teardown the release is configured not to perform; report everything else.** `helmrelease_has_teardown` reports an `uninstalled` Snapshot, which is proof the release was removed and put back. Two configurations write it — the default strategy's install remediation, and an upgrade remediation set to `strategy: uninstall`, which nothing under `packages/` uses — so on a `RetryOnFailure` release with neither — the tenant `cilium` and `csi`, and every Application HelmRelease, which the aggregated apiserver stamps that way (`pkg/registry/apps/application/rest.go`) — an `uninstalled` Snapshot has no explanation inside the release and fails the run. On a release still using the default strategy the same Snapshot is that strategy doing its documented job, and it is printed instead.
67+
- **Report `failed` rather than failing on it.** `helmrelease_has_remediation_cycle` is the broader `failed`-or-`uninstalled` reading; it catches the upgrade remediation that rolls back rather than uninstalls, but it is not proof anything was removed, since a retried install under `RetryOnFailure` leaves `failed` behind with its manifests still applied.
68+
- **Do not put an unmeasured pass condition on the gate.** The tenant addons that set no strategy pair the default one with `remediation.retries: -1`, a configuration that declares repeated uninstall-and-reinstall their recovery path. Nothing in the suite measures how often they take it, and failing a 25-minute bringup on a release doing what it is configured to do is how a guard ends up switched off. That the configuration is itself a defect is a separate matter from what the guard asserts. Print the finding, name the release, leave the run green, and tighten when a measured baseline or a strategy change makes the condition knowable.
69+
- **Do not infer a child release's state from the parent's.** The Kubernetes `ApplicationDefinition` sets `release.cozystack.io/helm-install-disable-wait`, which the apiserver turns 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. An empty `.status.history` is therefore judged against that release's own Ready condition: 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 is reported as not inspected.
6670

6771
### 7. Test-Impact Analysis (TIA), default-on
6872

@@ -126,7 +130,7 @@ The suite is pinned to Chainsaw **v0.2.15** (the latest release as of May 2026);
126130
5. Controller-created artifacts the test cannot reclaim are pruned explicitly; nested tenants delete child → parent with a wait-for-deletion between.
127131
6. Standard HR-Ready assert timeout is **5–6m**; longer waits (harbor 10m, NFS 10m, VM image pulls, platform-wide install 15m) are justified in-line.
128132
7. Failure path attaches scoped diagnostics via a `catch:` block, never a silent pass.
129-
8. If it touches parent-HR behavior, add the `status.history` remediation guard (`hack/e2e-chainsaw/_lib/remediation-guard.sh`).
133+
8. If it touches HelmRelease install/upgrade behavior, add the `status.history` guard (`hack/e2e-chainsaw/_lib/remediation-guard.sh`): fail the run on a teardown of a `RetryOnFailure` release, report every other footprint without failing. On an Application-served HelmRelease the broad `failed` reading is the only footprint the release's own configuration can produce, and it is informational; an `uninstalled` Snapshot there comes from outside the release and fails the run.
130134

131135
## In-flight direction (not yet the merged standard)
132136

‎hack/e2e-chainsaw/_lib/run-kubernetes.sh‎

Lines changed: 177 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -627,6 +627,154 @@ cozy_capture_tenant_talos() (
627627
done <"${workdir}/vmis.rows"
628628
)
629629

630+
# Assert that one HelmRelease was not torn down in a way its own configuration
631+
# says cannot happen. $1 is the namespace, $2 the release name. Returns non-zero
632+
# on such a teardown or on a read that failed, so the caller's errexit fails the
633+
# run.
634+
#
635+
# One rule decides the verdict: fail when the release was REMOVED although it is
636+
# configured never to remove itself to recover. An "uninstalled" Snapshot is
637+
# proof of removal, and two configurations can write it - the default strategy's
638+
# install remediation, which uninstalls before retrying, and an upgrade
639+
# remediation set to strategy: uninstall, which nothing under packages/ sets. So
640+
# on a release running RetryOnFailure, where neither applies, an "uninstalled"
641+
# Snapshot has no explanation inside the release and fails the run: that is the
642+
# tenant CNI and CSI (moved to RetryOnFailure precisely to take the teardown
643+
# away) and every Application HelmRelease, which the aggregated apiserver stamps
644+
# the same way.
645+
#
646+
# Everything else is reported and left green, and that includes an "uninstalled"
647+
# Snapshot on a release still using the default strategy. Those addons pair it
648+
# with remediation.retries: -1, which is a configuration that declares
649+
# uninstall-and-reinstall their recovery path; the run has no measurement of how
650+
# often they take it, and failing a 25-minute bringup on a release doing what it
651+
# is configured to do is how a guard ends up switched off. That the configuration
652+
# is itself the defect is true and is not this guard's to assert - it is filed
653+
# separately. A "failed" Snapshot is reported for the same reason and one more:
654+
# under RetryOnFailure it is not even remediation, just an attempt that kept its
655+
# manifests and was retried.
656+
#
657+
# Both probes keep their exit status. A failed read and a field the object does
658+
# not carry both print nothing, so folding them together (`|| true` on the whole
659+
# substitution) would let an API timeout or an RBAC error pass for a release with
660+
# no history - the same misreading cozy_tenant_drained guards against with its
661+
# "err" sentinel. `if ! var=$(…)` keeps the non-zero status from tripping the
662+
# errexit the caller runs under.
663+
#
664+
# kubectl's stderr is deliberately NOT folded into the values either. kubectl
665+
# writes warnings there while exiting 0 - a deprecation notice, or the discovery
666+
# noise a degraded aggregated APIService produces - and such a line landing in
667+
# the history capture makes an empty history look populated, which skips the
668+
# readiness branch below, while in the strategy capture it stops the value
669+
# matching RetryOnFailure and silently downgrades a teardown to a note. Left
670+
# unredirected, those lines go to the script's own stderr, which is where a
671+
# reader wants them anyway.
672+
#
673+
# Unit-tested in hack/run-kubernetes-remediation_test.bats.
674+
cozy_guard_helmrelease() {
675+
local _ns="$1"
676+
local _hr="$2"
677+
local _strategy _history _ready
678+
if ! _strategy=$(kubectl get hr -n "$_ns" "${_hr}" \
679+
-o jsonpath='{.spec.install.strategy.name}'); then
680+
echo "Reading .spec.install.strategy.name of ${_hr} failed - kubectl's error is above." >&2
681+
return 1
682+
fi
683+
if ! _history=$(kubectl get hr -n "$_ns" "${_hr}" \
684+
-o jsonpath='{range .status.history[*]}{.status}{"\n"}{end}'); then
685+
echo "Reading .status.history of ${_hr} failed - kubectl's error is above." >&2
686+
return 1
687+
fi
688+
# Always emit the raw values, so a silent future-Flux field rename shows up in
689+
# the CI log as an empty history on a Ready release rather than vanishing.
690+
echo "HelmRelease ${_hr} (install strategy ${_strategy:-<unset>}) history statuses:"
691+
printf '%s\n' "${_history:-<empty>}"
692+
# An empty history is read against this release's own readiness. A Ready
693+
# HelmRelease has a completed helm action behind it and the controller keeps a
694+
# Snapshot of every one, so Ready with no history is the Flux status shape
695+
# having moved under the guard - skipping that would leave the release
696+
# unchecked while the run stayed green, which is the failure mode this guard
697+
# exists to remove rather than reproduce. A release that is not Ready may
698+
# simply never have completed an action: it has no teardown to find, and
699+
# failing on it would be a red run blamed on a Flux API change for a release
700+
# that is still installing.
701+
if [ -z "${_history}" ]; then
702+
if ! _ready=$(kubectl get hr -n "$_ns" "${_hr}" \
703+
-o jsonpath='{.status.conditions[?(@.type=="Ready")].status}'); then
704+
echo "Reading the Ready condition of ${_hr} failed - kubectl's error is above." >&2
705+
return 1
706+
fi
707+
if [ "${_ready}" = "True" ]; then
708+
echo "Unexpected empty .status.history on Ready HelmRelease ${_hr} - Flux API shape may have changed." >&2
709+
kubectl -n "$_ns" describe hr "${_hr}" >&2
710+
return 1
711+
fi
712+
echo "» ${_hr} is not Ready and has no release history - no completed helm action to judge, not inspected"
713+
return 0
714+
fi
715+
if helmrelease_has_teardown "${_history}"; then
716+
if [ "${_strategy}" = "RetryOnFailure" ]; then
717+
echo "HelmRelease ${_hr} was uninstalled and reinstalled, though its install strategy is RetryOnFailure, which does not uninstall to recover. The other configuration that uninstalls is an upgrade remediation with strategy: uninstall - check whether one was added before looking outside the release." >&2
718+
kubectl -n "$_ns" describe hr "${_hr}" >&2
719+
return 1
720+
fi
721+
echo "» NOTE: ${_hr} was uninstalled and reinstalled by its own install remediation, which is what the default strategy does with retries: -1. Not failing the run; read this as the cause if something downstream did fail."
722+
return 0
723+
fi
724+
if helmrelease_has_remediation_cycle "${_history}"; then
725+
echo "» NOTE: ${_hr} carries a failed Snapshot - it was remediated in place and recovered. Not failing the run; read this as the cause if something downstream did fail."
726+
fi
727+
return 0
728+
}
729+
730+
# Run that guard over the tenant addon HelmReleases a parent release installs.
731+
# $1 is the namespace, $2 the name prefix the addons share ("kubernetes-<test>-").
732+
#
733+
# The parent's own .status.history says nothing about its children, so a
734+
# remediation cycle on one of them surfaces only as whatever downstream deadline
735+
# it eventually breaks -- a node that never turns Ready, a PVC that never binds
736+
# -- with nothing in the run naming the teardown that preceded it. The CNI and
737+
# CSI releases are the sharp cases, because their teardown removes a
738+
# cluster-wide precondition rather than churning one component.
739+
#
740+
# The addons are read from the cluster rather than listed here, so a release the
741+
# chart adds later is covered without touching this script.
742+
#
743+
# Adds no wait, and does not assume one happened. The parent HelmRelease carries
744+
# DisableWait on both actions (the Kubernetes ApplicationDefinition sets
745+
# release.cozystack.io/helm-install-disable-wait, deliberately, because its addon
746+
# releases cannot go Ready before worker nodes exist), so the parent reaching
747+
# Ready says nothing about them; and the suite waits on some of them by name
748+
# while never naming others, metrics-server and prometheus-operator-crds among
749+
# them. That is why each release is judged on what its own object says, readiness
750+
# included.
751+
cozy_guard_addon_helmreleases() {
752+
local _ns="$1"
753+
local _prefix="$2"
754+
local _all_hrs _addon_hrs _addon_hr _failed
755+
if ! _all_hrs=$(kubectl get hr -n "$_ns" \
756+
-o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}'); then
757+
echo "Listing HelmReleases in ${_ns} failed - kubectl's error is above." >&2
758+
return 1
759+
fi
760+
# grep exits 1 on no match, which the empty-selection check below reports.
761+
_addon_hrs=$(printf '%s\n' "${_all_hrs}" | grep "^${_prefix}" || true)
762+
if [ -z "${_addon_hrs}" ]; then
763+
echo "No addon HelmReleases matched ${_prefix}* in ${_ns}. The parent is Ready, so either this prefix no longer selects its addon releases, or the chart stopped rendering them - they are gated on _namespace.etcd in packages/apps/kubernetes/templates/helmreleases/." >&2
764+
printf '%s\n' "${_all_hrs}" >&2
765+
return 1
766+
fi
767+
# Every addon is inspected even after one of them fails. The notes the others
768+
# print - a rollback here, a teardown the default strategy performed there -
769+
# are the context that explains the failure, and stopping at the first one
770+
# hides exactly the releases a reader would compare it against.
771+
_failed=0
772+
for _addon_hr in ${_addon_hrs}; do
773+
cozy_guard_helmrelease "$_ns" "${_addon_hr}" || _failed=1
774+
done
775+
return "${_failed}"
776+
}
777+
630778
run_kubernetes_test() {
631779
local version_expr="$1"
632780
local test_name="$2"
@@ -1448,41 +1596,37 @@ EOF
14481596

14491597
# Wait for the parent kubernetes-${test_name} HR to be Ready before the
14501598
# remediation guard runs. The guard reads `.status.history`, which is empty
1451-
# until the helm install action completes — under Flux v2.8 kstatus the
1452-
# parent's helm install can still be "Running 'install'" after every child
1453-
# HR (cilium, coredns, csi, vsnap-crd, ingress-nginx) is already Ready,
1454-
# because kstatus walks all applied resources before flipping the parent
1455-
# Ready.
1599+
# until the helm install action completes, and the parent's install can still
1600+
# be "Running 'install'" after every child HR (cilium, coredns, csi,
1601+
# vsnap-crd, ingress-nginx) is already Ready. Not because it waits for them:
1602+
# this release carries DisableWait on both actions, from the
1603+
# release.cozystack.io/helm-install-disable-wait annotation on its
1604+
# ApplicationDefinition, so it waits for nothing it applied. It settles late
1605+
# for its own reasons - the chart's own main-phase work - which is exactly why
1606+
# the wait is here rather than inferred from the children.
14561607
kubectl wait hr -n tenant-test "kubernetes-${test_name}" --timeout=5m --for=condition=ready
14571608

1458-
# Guard: parent HelmRelease must not have entered an install/upgrade remediation cycle.
1459-
# A non-zero installFailures/upgradeFailures indicates the helm-wait budget expired while
1460-
# admin-kubeconfig was still being provisioned, which would trigger uninstall remediation
1461-
# and churn the Cluster CR.
1462-
# Flux helm-controller v2 retains per-revision release Snapshots in
1463-
# .status.history; each Snapshot's .status reflects the Helm release
1464-
# state (deployed/superseded/failed/uninstalled). A remediation cycle
1465-
# leaves a "failed" or "uninstalled" entry behind that survives a later
1466-
# successful reinstall, unlike the installFailures/upgradeFailures
1467-
# counters (which ClearFailures zeroes on every successful reconcile).
1468-
# The shape is pinned by hack/remediation-guard.bats; the upstream
1469-
# types are github.com/fluxcd/helm-controller/api v2 Snapshot.
1470-
history_statuses=$(kubectl get hr -n tenant-test "kubernetes-${test_name}" \
1471-
-ojsonpath='{range .status.history[*]}{.status}{"\n"}{end}')
1472-
# Always emit the raw value so a silent future-Flux field rename shows
1473-
# up as "empty history on a Ready HR" in CI logs rather than vanishing.
1474-
echo "Parent HelmRelease history statuses:"
1475-
printf '%s\n' "${history_statuses:-<empty>}"
1476-
if [ -z "${history_statuses}" ]; then
1477-
echo "Unexpected empty .status.history on a Ready HelmRelease - Flux API shape may have changed." >&2
1478-
kubectl -n tenant-test describe hr "kubernetes-${test_name}" >&2
1479-
exit 1
1480-
fi
1481-
if helmrelease_has_remediation_cycle "${history_statuses}"; then
1482-
echo "Parent HelmRelease entered remediation cycle." >&2
1483-
kubectl -n tenant-test describe hr "kubernetes-${test_name}" >&2
1484-
exit 1
1485-
fi
1609+
# Guard: neither the parent HelmRelease nor the addon releases it installs may
1610+
# have been torn down and reinstalled. Both go through cozy_guard_helmrelease,
1611+
# which reads .status.history rather than
1612+
# .status.installFailures/.status.upgradeFailures -- ClearFailures zeroes those
1613+
# on every successful reconcile, so reading them after the release is Ready is
1614+
# vacuous, while the release Snapshots survive. The Snapshot shape is pinned by
1615+
# hack/remediation-guard.bats against github.com/fluxcd/helm-controller/api v2.
1616+
#
1617+
# On this parent, a "failed" Snapshot is not even evidence of remediation: the
1618+
# aggregated apiserver stamps RetryOnFailure on the HelmRelease of every
1619+
# Application it serves (pkg/registry/apps/application/rest.go), on install and
1620+
# upgrade alike, so a failed attempt here keeps its manifests and is retried.
1621+
# What stays detectable is a teardown the release did not perform on itself,
1622+
# plus the empty-history check inside the guard, which is what reports a Flux
1623+
# status shape the helper can no longer read.
1624+
#
1625+
# Both calls run before the EXIT trap is disarmed, so a teardown either one
1626+
# finds still captures the tenant snapshot. That ordering is pinned by
1627+
# hack/run-kubernetes-remediation_test.bats, along with the calls themselves.
1628+
cozy_guard_helmrelease tenant-test "kubernetes-${test_name}"
1629+
cozy_guard_addon_helmreleases tenant-test "kubernetes-${test_name}-"
14861630

14871631
# Success: disarm the tenant-snapshot trap so it doesn't fire on the clean exit.
14881632
trap - EXIT

0 commit comments

Comments
 (0)