Skip to content

Commit 42a2756

Browse files
scooby87lexfrei
scooby87
authored and
lexfrei
committed
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 <[email protected]>
1 parent 52f542c commit 42a2756

23 files changed

Lines changed: 5835 additions & 121 deletions

‎.github/workflows/pull-requests.yaml‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,17 @@ jobs:
353353
sudo "$bats_source/install.sh" /usr/local
354354
fi
355355
bats --version | grep -qx "Bats $BATS_VERSION"
356+
# setup-envtest and the kube-apiserver and etcd it fetches, so a pull
357+
# request downloads them only when the versions pinned in the Makefile
358+
# change, and an outage at their sources cannot fail unrelated changes.
359+
# The envtest target runs the controller suites a fake client cannot
360+
# stand in for.
361+
- name: Cache envtest binaries
362+
if: needs.plan.outputs.code == 'true'
363+
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
364+
with:
365+
path: _out/envtest
366+
key: envtest-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('Makefile') }}
356367
# -k is required, not cosmetic: unit-tests and test-controllers are two
357368
# independent goals on the same command line, and without -k make stops
358369
# handing out new work the moment any unit-tests prerequisite fails —
@@ -361,7 +372,7 @@ jobs:
361372
# goals keep running; the job still fails overall on any red target.
362373
- name: Run unit and controller tests
363374
if: needs.plan.outputs.code == 'true'
364-
run: make unit-tests test-controllers -j4 -k --output-sync=target
375+
run: make unit-tests test-controllers test-controllers-envtest -j4 -k --output-sync=target
365376
- name: Run Bats unit tests for docs-only changes
366377
if: needs.plan.outputs.code == 'false'
367378
run: make bats-unit-tests

‎Makefile‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
.PHONY: manifests assets prepare-env prepare-env-container unit-tests helm-unit-tests bats-unit-tests bats-unit-files-check bats-posix-compat-tests print-bats-unit-files print-bats-posix-compat-files print-bats-jobs rd-presets-check migrations-target-check test test-controllers test-backport-audit preflight
1+
.PHONY: manifests assets prepare-env prepare-env-container unit-tests helm-unit-tests bats-unit-tests bats-unit-files-check bats-posix-compat-tests print-bats-unit-files print-bats-posix-compat-files print-bats-jobs rd-presets-check migrations-target-check test test-controllers test-controllers-envtest test-backport-audit preflight
22

33
include hack/common-envs.mk
44

@@ -163,6 +163,29 @@ go-module-tests:
163163
test-controllers:
164164
go test ./internal/... -count=1
165165

166+
# Controller suites that need a real apiserver (build tag `envtest`): server-side
167+
# apply ownership, apply preconditions and finalizer handling, which a fake client
168+
# does not implement. setup-envtest fetches the kube-apiserver and etcd release
169+
# pinned by ENVTEST_K8S_VERSION into _out/envtest; the HelmRelease CRD comes from
170+
# the in-tree Flux manifests (envtest skips their non-CRD objects). CI runs it
171+
# next to test-controllers. Only the packages listed below are compiled, so a
172+
# package that gains an envtest suite has to be added here.
173+
#
174+
# setup-envtest itself is built into _out/envtest too, and binaries already there
175+
# are used without asking the release index, so a warm _out/envtest (CI caches
176+
# it) needs no network at all.
177+
ENVTEST_K8S_VERSION ?= 1.35.0
178+
SETUP_ENVTEST_VERSION ?= v0.0.0-20260305142021-f9589b9f2b9d
179+
ENVTEST_DIR = $(CURDIR)/_out/envtest
180+
SETUP_ENVTEST = $(ENVTEST_DIR)/bin/setup-envtest-$(SETUP_ENVTEST_VERSION)
181+
$(SETUP_ENVTEST):
182+
GOBIN=$(ENVTEST_DIR)/bin go install sigs.k8s.io/controller-runtime/tools/setup-envtest@$(SETUP_ENVTEST_VERSION)
183+
mv $(ENVTEST_DIR)/bin/setup-envtest $@
184+
test-controllers-envtest: $(SETUP_ENVTEST)
185+
KUBEBUILDER_ASSETS="$$($(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --installed-only --bin-dir $(ENVTEST_DIR) --print path 2>/dev/null || $(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(ENVTEST_DIR) --print path)" \
186+
HELMRELEASE_CRD_DIR=$(CURDIR)/internal/fluxinstall/manifests \
187+
go test -tags envtest -run '^TestEnvtest' ./internal/controller/fluxplunger/... -count=1
188+
166189
# Black-box golden test for cmd/check-readiness. Builds the binary and runs it
167190
# against a mock kubectl (fixtures under test/check-readiness/testdata),
168191
# diffing stdout/stderr/exit against golden files. Regenerate goldens with:

‎cmd/flux-plunger/main.go‎

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"crypto/tls"
2121
"flag"
2222
"os"
23+
"time"
2324

2425
// Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.)
2526
// to ensure that exec-entrypoint and run can make use of them.
@@ -56,6 +57,7 @@ func main() {
5657
var probeAddr string
5758
var secureMetrics bool
5859
var enableHTTP2 bool
60+
var shutdownDrainTimeout time.Duration
5961
var tlsOpts []func(*tls.Config)
6062

6163
flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+
@@ -68,6 +70,9 @@ func main() {
6870
"If set, the metrics endpoint is served securely via HTTPS. Use --metrics-secure=false to use HTTP instead.")
6971
flag.BoolVar(&enableHTTP2, "enable-http2", false,
7072
"If set, HTTP/2 will be enabled for the metrics server")
73+
flag.DurationVar(&shutdownDrainTimeout, fluxplunger.ShutdownDrainTimeoutFlag, 75*time.Second,
74+
"How long to spend giving up the uninstall fences flux-plunger holds before it exits. "+
75+
"Keep it inside the pod's termination grace period.")
7176

7277
opts := zap.Options{
7378
Development: false,
@@ -102,31 +107,25 @@ func main() {
102107
TLSOpts: tlsOpts,
103108
}
104109

105-
mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
110+
mgrOpts := ctrl.Options{
106111
Scheme: scheme,
107112
Metrics: metricsServerOptions,
108113
HealthProbeBindAddress: probeAddr,
109114
LeaderElection: enableLeaderElection,
110115
LeaderElectionID: "flux-plunger.cozystack.io",
111-
// LeaderElectionReleaseOnCancel defines if the leader should step down voluntarily
112-
// when the Manager ends. This requires the binary to immediately end when the
113-
// Manager is stopped, otherwise, this setting is unsafe. Setting this significantly
114-
// speeds up voluntary leader transitions as the new leader don't have to wait
115-
// LeaseDuration time first.
116-
//
117-
// In the default scaffold provided, the program ends immediately after
118-
// the manager stops, so would be fine to enable this option. However,
119-
// if you are doing or is intended to do any operation such as perform cleanups
120-
// after the manager stops then its usage might be unsafe.
121-
// LeaderElectionReleaseOnCancel: true,
122-
})
116+
}
117+
// The Lease is held, not released, through the drain after the manager stops.
118+
fluxplunger.ApplyShutdownTimings(&mgrOpts, shutdownDrainTimeout)
119+
mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), mgrOpts)
123120
if err != nil {
124121
setupLog.Error(err, "unable to create manager")
125122
os.Exit(1)
126123
}
127124

128125
if err = (&fluxplunger.FluxPlunger{
129-
Client: mgr.GetClient(),
126+
Client: mgr.GetClient(),
127+
APIReader: mgr.GetAPIReader(),
128+
Recorder: mgr.GetEventRecorderFor("flux-plunger"),
130129
}).SetupWithManager(mgr); err != nil {
131130
setupLog.Error(err, "unable to create controller", "controller", "FluxPlunger")
132131
os.Exit(1)
@@ -144,7 +143,7 @@ func main() {
144143
}
145144

146145
setupLog.Info("starting manager")
147-
if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {
146+
if err := fluxplunger.RunManager(ctrl.SetupSignalHandler(), mgr, shutdownDrainTimeout); err != nil {
148147
setupLog.Error(err, "problem running manager")
149148
os.Exit(1)
150149
}

‎hack/bats-runner-contract.bats‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ brc_write_parallel_stub() {
7575
code_command=$(yq -r '.jobs.checks.steps[] | select(.name == "Run unit and controller tests") | .run' "$BRC_WORKFLOW")
7676
docs_command=$(yq -r '.jobs.checks.steps[] | select(.name == "Run Bats unit tests for docs-only changes") | .run' "$BRC_WORKFLOW")
7777
[ -z "$lint_skip" ]
78-
[ "$code_command" = 'make unit-tests test-controllers -j4 -k --output-sync=target' ]
78+
[ "$code_command" = 'make unit-tests test-controllers test-controllers-envtest -j4 -k --output-sync=target' ]
7979
[ "$docs_command" = 'make bats-unit-tests' ]
8080
}
8181

‎hack/downstream-trigger-map.bats‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ map_section() {
119119
[ "$code_if" = "needs.plan.outputs.code == 'true'" ]
120120
[ "$docs_if" = "needs.plan.outputs.code == 'false'" ]
121121
[ "$docs_run" = 'make bats-unit-tests' ]
122-
[ "$code_run" = 'make unit-tests test-controllers -j4 -k --output-sync=target' ]
122+
[ "$code_run" = 'make unit-tests test-controllers test-controllers-envtest -j4 -k --output-sync=target' ]
123123
}
124124

125125
@test "the trigger map and the PR-template checklist list the same repositories" {

0 commit comments

Comments
 (0)