Files
cozystack/hack/downstream-trigger-map.bats
Alexey Artamonov 42a27567ce fix(fluxcd): keep Helm uninstall working during flux-plunger recovery
flux-plunger recovers a "has no deployed releases" HelmRelease by
briefly suspending it, deleting the stale storage Secret and
unsuspending. helm-controller v1.5.x skips the uninstall and drops its
finalizer when it handles a delete on a suspended HelmRelease, and it
wakes on the same delete event flux-plunger does, so a delete inside
that window orphaned the release's DataVolume, PVCs, RBAC and storage
Secret. Lifting the suspension on the delete event cannot win that race
reliably.

Hold the release with a flux-plunger finalizer while it is suspended.
The finalizer is applied together with the suspension in one server-side
apply. On a delete flux-plunger lifts its suspension and keeps the
finalizer, helm-controller reconciles the unsuspended generation and
runs the uninstall, and flux-plunger removes the finalizer once
helm-controller's own is gone and that generation is observed. This
holds whichever controller handles the delete first. A completed
recovery lifts the suspension and the finalizer together, and a
terminating release never begins recovery.

Decide ownership of a suspension by which field manager owns
spec.suspend in metadata.managedFields rather than by an annotation. The
apiserver transfers the field away the moment an operator, cozyhr resume
or a backup controller writes a different value to it, so a later
suspension is never misread as flux-plunger's, and one set through the
shared flux-client-side-apply manager (platform migrations, the tenant
delete hook, a manual kubectl apply) is never claimed. A release deleted
under another manager's suspension is left to that manager's decision.

Acquire applies without forcing first and, on a conflict, re-fetches and
forces only if spec.suspend is still false: a backup controller owns the
field at false through a dynamic-client Update, which a non-forced apply
can never take, while true is an active suspension to leave alone. Every
apply is preconditioned on the resourceVersion and uid of the read that
decided on it. Without the resourceVersion, the unforced apply of true
over a foreign true is accepted as shared ownership, a window the
optimistic-locked merge patch this replaces had closed, and lifting the
suspension and the finalizer from a release read as live would delete it
without an uninstall if helm-controller had meanwhile handled a delete.
Without the uid, an apply to a release that has gone would create an
empty one. The apply carries only spec.suspend and the finalizer, so it
cannot leak the required spec.interval.

Read the release, every re-check and the storage Secret list from the
apiserver rather than the informer cache. A lagging cache would let the
processed-version guard delete the revision below the one just
reclaimed, and the ClusterRole grants secrets get and list but not the
watch an informer needs.

Emit a Warning when flux-plunger holds a suspension it failed to clear,
and Normal events when it leaves an external suspension alone or defers
a recovery. An external suspension on a release flux-plunger has
recovered before is a Warning instead: it is most likely one an earlier
flux-plunger left under the shared manager, which nothing lifts unless
an operator does. A release deleted under another manager's suspension
is a Normal event when that was a migration's choice, and a Warning when
the release carries flux-plunger's processed-version annotation, since
then the skipped uninstall is an accident.

Keep a single writer with leader election. A second process able to act
under the same field manager could clear a suspension the first is still
recovering, and no rollout strategy prevents one: the next pod starts as
soon as the old one is terminating, and an eviction or a node drain
replaces a pod the same way. The old pod keeps its Lease, unreleased,
through its stop and drain, and the Lease outlasts both, so the next pod
acts only after the old one is done; the price is that a new pod waits
for the Lease to run out. The strategy sets maxSurge 0 with
maxUnavailable 1 and stays RollingUpdate because existing installs carry
the apiserver-defaulted rollingUpdate block and helm-controller applies
server-side, where type Recreate over it is rejected on upgrade.

Give the finalizer a way out when flux-plunger itself goes. Once the
manager has stopped, within a bounded time, and with it every reconcile,
flux-plunger drains its finalizers under a deadline that, with the stop,
fits inside the pod's termination grace period. A stop that overran its
bound is drained too and still reported, and only a pod that led drains.
The drain writes its events straight to the apiserver, since the
manager's recorder stops with the manager. A recovery in progress is
aborted, the suspension and the finalizer lifted together for the next
flux-plunger to start again, and a release being deleted has the
suspension lifted and keeps the finalizer until helm-controller has run
the uninstall. A release helm-controller does not handle in time keeps
it. The waiting lives in the controller, where the envtest suites hold
it against a real apiserver's generations and finalizers, rather than in
a shell script against a model of them.

A pre-delete hook on the chart is the backstop for what the drain could
not finish, so uninstalling the component never leaves a release that
tenant teardown waits on forever. It stops flux-plunger, waits out its
grace period, then lifts any remaining suspension and finalizer without
waiting again, naming each release it lets go of before helm-controller
uninstalled it. It lifts a suspension only where flux-plunger alone
holds it, with a JSON patch preconditioned on the resourceVersion and
uid it read: kubectl apply --server-side on a release last written by a
client-side apply moves that apply's fields to the applying manager and
strips every one the body leaves out. The Job's deadline sits under the
300s helm-controller gives a pre-delete hook by default. A Deployment
that cannot be stopped or a listing that never succeeds ends the hook
before any finalizer is touched, and a finalizer it cannot remove ends
it after the sweep; either way it exits non-zero: a failed pre-delete
hook leaves the release untouched and helm-controller retries the
uninstall, hook included, from a state it can still converge from, where
exiting 0 would remove the component and strand the finalizer. A
flux-plunger pod still present after its grace period, typically one
stuck Terminating on a NotReady node, is only reported, since failing on
it would fail every retry until the node returns; because it may still
be setting finalizers, they are listed again after the sweep and any
found fails the hook. The Job runs once, since helm-controller's retry
is the one that starts afresh, and is kept after success so that its
report stays readable; helm.sh/hook-output-log-policy would make
helm-controller call a nil output writer (fluxcd/helm-controller#1562).
A delete still held after ten minutes raises a Warning, and the held
state is re-checked on a timer instead of only on helm-controller's next
status write. The chart README documents the finalizer and how to clear
it.

Run the envtest suites on every pull request through a new make target
that fetches a kube-apiserver and etcd with a pinned setup-envtest and
takes the HelmRelease CRD from the in-tree Flux manifests: the unit
tests drive a fake client, and everything the finalizer rests on (apply
preconditions, the finalizer set-merge, real managedFields) only holds
against a real apiserver. CI caches those binaries, and the target uses
them without asking the release index, so a pull request does not depend
on their sources being reachable; the control plane gets a longer start
timeout because it starts beside the other test goals.

Assisted-by: LLM
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
2026-09-15 23:34:52 +03:00

155 lines
7.9 KiB
Bash

#!/usr/bin/env bats
# Guards the "Downstream Repositories" trigger map in docs/agents/contributing.md.
#
# The map tells a contributor which change in this repo forces a follow-up in a
# satellite repository. Each trigger names a file on this side: hack/update-crd.sh,
# packages/core/platform/values.yaml, and so on. Rename one of those and the map
# keeps confidently pointing at a path that no longer exists, from inside the PR
# template, to every reader. The repository list is deliberately not restated here
# — it would be one more copy to drift; the map and the PR template are the two
# sources, and the tests below hold them to each other.
#
# The three tests pin, in order: every in-repo path the map cites to a file that
# exists; the workflow guarantee that Bats contracts run on every docs-only PR;
# and the map and the PR-template checklist to the same repository list, so a
# repository cannot be listed in one and forgotten in the other.
#
# WHAT THIS DOES NOT CHECK, so nobody mistakes a green tick for a correct map:
#
# * Whether a trigger is TRUE. "Change X here and Y breaks over there" is a
# claim about a satellite repo. Verifying it needs that repo checked out at
# a known ref, which this suite deliberately does not do: a unit test must
# not depend on the network or on six sibling clones. A trigger can cite a
# path that exists here and still describe a coupling that is nonsense.
# * Anything on the satellite side. A file named there (its Makefile, its
# scripts/, its playbooks) is not reachable from this repo and is not
# validated. Those names are exactly where the map is most likely to rot.
# * Whether the map is COMPLETE. Deleting a whole trigger, or a whole block of
# them, passes: the repository-list test compares section headings, not bodies.
# * A path written without backticks. Extraction keys off `code spans`, which
# is the section's convention; a lone unquoted path slips through. Stripping
# the backticks wholesale does fail, via the anti-vacuum guard below.
#
# In short: this catches a rename on THIS side, which is the cheap half. The
# expensive half — is the coupling real, and is it still real over there — stays
# a human's job, and reviewers should not assume CI did it for them.
#
# A docs-only PR skips builds and E2E, but the `checks` job still runs the Bats
# lane. Otherwise editing this map, or another document audited by a unit
# contract, would exempt the changed input from its own guard.
load test_helper
REPO_ROOT="$(cd "$(dirname "${BATS_TEST_FILENAME:-$0}")/.." && pwd)"
MAP_FILE="$REPO_ROOT/docs/agents/contributing.md"
TEMPLATE_FILE="$REPO_ROOT/.github/PULL_REQUEST_TEMPLATE.md"
# Body of the "## Downstream Repositories" section, up to the next h2.
map_section() {
awk '/^## Downstream Repositories$/ { inside = 1; next }
/^## / { inside = 0 }
inside' "$MAP_FILE"
}
@test "every in-repo path cited by the downstream trigger map exists" {
[ -f "$MAP_FILE" ] || { echo "missing $MAP_FILE" >&2; exit 1; }
section="$(map_section)"
[ -n "$section" ] || { echo "Could not locate the '## Downstream Repositories' section in $MAP_FILE" >&2; exit 1; }
# Backticked tokens rooted at a top-level directory of THIS repo. Satellite
# paths cited by the map (scripts/package.mk, content/en/docs/...) do not match
# these roots and so fall out. Note this is a heuristic, not a guarantee: a
# satellite repo has its own packages/ tree, so a backticked path from over
# there that happens to share a root would be checked against this tree.
# A trailing slash is prose, not a distinct path: `packages/apps/` and
# `packages/apps` are the same anchor and would otherwise both inflate the count.
paths="$(printf '%s\n' "$section" \
| grep -o '`[^`]*`' \
| tr -d '`' \
| grep -E '^(hack|packages|cmd|api|internal|pkg|\.github)/' \
| sed 's#/$##' \
| sort -u)"
# Guard against a vacuous pass: if the extraction breaks, or the section is
# reworded so it no longer cites paths, this test must fail loudly rather than
# silently verify nothing. The floor sits well under the real count (28 as of
# writing) because the map legitimately shrinks too, and a red build should not
# send the reader hunting for a bug that is really a deleted section.
count="$(printf '%s\n' "$paths" | grep -c . || true)"
[ "$count" -ge 20 ] || {
echo "Extracted only $count in-repo path(s) from the trigger map; expected at least 20." >&2
echo "Either the extraction broke and this test is verifying nothing, or the map genuinely" >&2
echo "shrank - if a downstream repository went away, lower this floor deliberately." >&2
exit 1
}
missing=""
for path in $paths; do
# A placeholder segment (<name>) stands for any package, so match it as a glob.
pattern="$(printf '%s' "$path" | sed 's/<[^>]*>/*/g')"
# Unquoted on purpose: the shell must expand the glob. No match => ls fails.
if ! (cd "$REPO_ROOT" && ls -d $pattern) >/dev/null 2>&1; then
missing="$missing $path"
fi
done
if [ -n "$missing" ]; then
echo "The downstream trigger map in docs/agents/contributing.md cites paths that do not exist:" >&2
for path in $missing; do echo " - $path" >&2; done
echo "Fix: update the map to the new path, or drop the trigger if it no longer applies." >&2
echo "The map is read by contributors from inside the PR template; a stale path there sends them to a dead end." >&2
exit 1
fi
echo "All $count in-repo path(s) cited by the trigger map exist"
}
@test "the workflow runs Bats contracts for docs-only changes" {
WORKFLOW="$REPO_ROOT/.github/workflows/pull-requests.yaml"
[ -f "$WORKFLOW" ] || { echo "missing $WORKFLOW" >&2; exit 1; }
job_if=$(yq -r '.jobs.checks.if' "$WORKFLOW")
code_if=$(yq -r '.jobs.checks.steps[] | select(.name == "Run unit and controller tests") | .if' "$WORKFLOW")
docs_if=$(yq -r '.jobs.checks.steps[] | select(.name == "Run Bats unit tests for docs-only changes") | .if' "$WORKFLOW")
docs_run=$(yq -r '.jobs.checks.steps[] | select(.name == "Run Bats unit tests for docs-only changes") | .run' "$WORKFLOW")
code_run=$(yq -r '.jobs.checks.steps[] | select(.name == "Run unit and controller tests") | .run' "$WORKFLOW")
[ "$job_if" = "!contains(github.event.pull_request.labels.*.name, 'release')" ]
[ "$code_if" = "needs.plan.outputs.code == 'true'" ]
[ "$docs_if" = "needs.plan.outputs.code == 'false'" ]
[ "$docs_run" = 'make bats-unit-tests' ]
[ "$code_run" = 'make unit-tests test-controllers test-controllers-envtest -j4 -k --output-sync=target' ]
}
@test "the trigger map and the PR-template checklist list the same repositories" {
[ -f "$TEMPLATE_FILE" ] || { echo "missing $TEMPLATE_FILE" >&2; exit 1; }
# Checklist lines look like: - [ ] [cozystack/website](https://...) - follow-up:
template_repos="$(grep -oE '^- \[ \] \[cozystack/[a-z0-9.-]+\]' "$TEMPLATE_FILE" \
| grep -oE 'cozystack/[a-z0-9.-]+' \
| sort -u)"
# Map headings look like: ### cozystack/website
map_repos="$(map_section \
| grep -oE '^### cozystack/[a-z0-9.-]+' \
| grep -oE 'cozystack/[a-z0-9.-]+' \
| sort -u)"
template_count="$(printf '%s\n' "$template_repos" | grep -c . || true)"
[ "$template_count" -ge 1 ] || { echo "Found no cozystack/* checklist entries in $TEMPLATE_FILE - extraction is broken" >&2; exit 1; }
if [ "$template_repos" != "$map_repos" ]; then
echo "The PR-template checklist and the trigger map disagree on which repositories are downstream." >&2
echo "In the checklist (.github/PULL_REQUEST_TEMPLATE.md):" >&2
printf '%s\n' "$template_repos" | sed 's/^/ /' >&2
echo "In the map (docs/agents/contributing.md, '### cozystack/<repo>' headings):" >&2
printf '%s\n' "$map_repos" | sed 's/^/ /' >&2
echo "Fix: add the repository to both, or remove it from both." >&2
echo "A repository listed in only one of them gets ticked with no guidance, or documented with no box to tick." >&2
exit 1
fi
echo "Checklist and trigger map agree on $template_count repositories"
}