Skip to content

Commit 699555c

Browse files
scooby87scooby87
committed
fix(fluxcd): give each release its own share of the hook's retries
The pre-delete hook's retry budget was one window spent in listing order, so a release that could never be read or unfenced, listed first, took all of it and every release after it got a single attempt. The same transient failure then released a fence or not depending on where its release sorted. Each release now gets its own share of the window, capped at the window's end, and the steps that cover every release keep the whole of it. Two comments said more than holds. The retry window is measured from the script's start, not the Job's, so scheduling and the image pull come out of its one-minute reserve rather than being covered by it. And the five-minute conflict retry inside kubectl scale never runs here, since without a precondition it sends a merge patch with no resourceVersion to conflict on. helm-controller's finalizer, which the hook reads to tell whether a release being deleted is still held, is now tied to the constant the controller uses, as the fence already was. Assisted-by: LLM Signed-off-by: Alexey Artamonov <[email protected]>
1 parent af50c4a commit 699555c

3 files changed

Lines changed: 111 additions & 7 deletions

File tree

‎hack/flux-plunger-fence-hook_test.bats‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -497,6 +497,63 @@ run_hook() {
497497
rm -rf "$tmp"
498498
}
499499

500+
@test "a release that can never be read does not leave the ones listed after it a single attempt" {
501+
tmp=$(mktemp -d)
502+
prepare "$tmp"
503+
# Listed first, never readable: it must not spend the whole retry window.
504+
add_release "$STATE" tenant-a app-a "$FENCE"
505+
echo 9999 > "$STATE/[email protected]"
506+
# Listed after it, and its fence comes off only on the third try.
507+
add_release "$STATE" tenant-z app-z "$FENCE"
508+
echo 2 > "$STATE/[email protected]"
509+
510+
run_hook "$tmp"
511+
512+
[ "$status" -ne 0 ]
513+
grep -q '^NOT released tenant-a/app-a: it could not be read' "$tmp/out"
514+
grep -q -x "released $FENCE from tenant-z/app-z" "$tmp/out"
515+
[ "$(cat "$CLOCK")" -lt "$DEADLINE" ]
516+
rm -rf "$tmp"
517+
}
518+
519+
@test "releases swept after the retry window closed get one attempt each, so the report is still written" {
520+
tmp=$(mktemp -d)
521+
prepare "$tmp"
522+
# A slow scale-down and the stuck pod's wait take the window to its end; a
523+
# share of it per release must not reach past the window for these.
524+
echo $((WINDOW - 10)) > "$STATE/scale.slow"
525+
touch "$STATE/pods.stuck"
526+
for n in a b c; do
527+
add_release "$STATE" "tenant-$n" "app-$n" "$FENCE"
528+
echo 9999 > "$STATE/tenant-$n@app-$n.ownedread.fail"
529+
done
530+
531+
run_hook "$tmp"
532+
533+
[ "$status" -ne 0 ]
534+
grep -q -x 'released 0 of 3 fenced HelmReleases, 0 without an uninstall' "$tmp/out"
535+
[ "$(cat "$CLOCK")" -lt "$DEADLINE" ]
536+
rm -rf "$tmp"
537+
}
538+
539+
@test "the listing after the sweep gets the rest of the window, not the last release's share" {
540+
tmp=$(mktemp -d)
541+
prepare "$tmp"
542+
# The pod stays, so the fences are listed again; that listing fails for longer
543+
# than one release's share before it answers.
544+
echo 1 > "$STATE/wait.fail"
545+
touch "$STATE/pods.stuck"
546+
add_release "$STATE" tenant-a app-a "$FENCE"
547+
echo 20 > "$STATE/relist.fail"
548+
549+
run_hook "$tmp"
550+
551+
[ "$status" -eq 0 ]
552+
[ "$(fences_left "$STATE")" -eq 0 ]
553+
grep -q -x 'released 1 of 1 fenced HelmReleases, 0 without an uninstall' "$tmp/out"
554+
rm -rf "$tmp"
555+
}
556+
500557
@test "the wait for helm-controller ends early enough that the last fences still get their retries" {
501558
tmp=$(mktemp -d)
502559
prepare "$tmp"

‎internal/controller/fluxplunger/hook_contract_test.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"strings"
99
"testing"
1010
"time"
11+
12+
helmv2 "github.com/fluxcd/helm-controller/api/v2"
1113
)
1214

1315
// The chart's pre-delete hook restates two of this package's names in shell and
@@ -184,3 +186,28 @@ func TestHookTemplate_UsesTheControllersNames(t *testing.T) {
184186
}
185187
}
186188
}
189+
190+
// The hook tells a release helm-controller has stopped holding by
191+
// helm-controller's own finalizer. A rename on the flux side that the hook did
192+
// not follow would read every release being deleted as handled, and the hook
193+
// would unfence it without waiting for the uninstall.
194+
func TestHookTemplate_UsesHelmControllersFinalizer(t *testing.T) {
195+
raw, err := os.ReadFile(hookTemplate)
196+
if err != nil {
197+
t.Fatalf("read %s: %v", hookTemplate, err)
198+
}
199+
hook := string(raw)
200+
if want := `"` + helmv2.HelmReleaseFinalizer + `"`; !strings.Contains(hook, want) {
201+
t.Fatalf("hook does not name helm-controller's finalizer %s", want)
202+
}
203+
// Every other fluxcd.io literal in quotes, the RBAC API group aside, is that
204+
// finalizer, whole.
205+
for _, l := range regexp.MustCompile(`"[^"\s]*fluxcd\.io[^"\s]*"`).FindAllString(hook, -1) {
206+
if l == `"`+helmv2.GroupVersion.Group+`"` {
207+
continue
208+
}
209+
if l != `"`+helmv2.HelmReleaseFinalizer+`"` {
210+
t.Errorf("hook template names %s, helm-controller's finalizer is %q", l, helmv2.HelmReleaseFinalizer)
211+
}
212+
}
213+
}

‎packages/system/flux-plunger/templates/uninstall-fence-cleanup.yaml‎

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -185,11 +185,23 @@ spec:
185185
scope="${HR_SCOPE:---all-namespaces}"
186186
# The pod is given its grace period, and a little more, to drain.
187187
pod_wait={{ $podWait }}
188-
# Retries stop here, a minute short of activeDeadlineSeconds, which
189-
# counts from the Job's start, so the report is still written when
190-
# scheduling and the image pull took part of it. The pod wait is cut
191-
# to what is left of this window too.
188+
# Retries stop here, a minute short of activeDeadlineSeconds. That
189+
# deadline counts from the Job's start and this window from the
190+
# script's, so scheduling and the image pull come out of the minute:
191+
# one that takes longer gets the Job killed before its report, which
192+
# fails the uninstall for helm-controller to retry. The pod wait is
193+
# cut to what is left of this window too.
192194
retry_until=$(( $(date +%s) + {{ $retryWindow }} ))
195+
# Retries stop at $stop_at: the whole window for the steps that cover
196+
# every release, and a share of it for each release, so one that
197+
# can never be read or unfenced does not leave the releases listed
198+
# after it a single attempt.
199+
stop_at=$retry_until
200+
per_release=30
201+
begin_release() {
202+
stop_at=$(( $(date +%s) + per_release ))
203+
[ "$stop_at" -le "$retry_until" ] || stop_at=$retry_until
204+
}
193205
# Each fenced release once, however many copies of the fence it
194206
# carries: the apiserver accepts a finalizer listed twice.
195207
listing='{{`{{range .items}}{{$fenced := false}}{{range .metadata.finalizers}}{{if eq . "flux-plunger.cozystack.io/uninstall-fence"}}{{$fenced = true}}{{end}}{{end}}{{if $fenced}}{{.metadata.namespace}}/{{.metadata.name}}{{"\n"}}{{end}}{{end}}`}}'
@@ -209,7 +221,7 @@ spec:
209221
printf '%s\n' "$out"
210222
return 0
211223
fi
212-
[ "$(date +%s)" -lt "$retry_until" ] || return 1
224+
[ "$(date +%s)" -lt "$stop_at" ] || return 1
213225
sleep 2
214226
done
215227
}
@@ -245,7 +257,7 @@ spec:
245257
--patch "[{\"op\":\"test\",\"path\":\"/metadata/resourceVersion\",\"value\":\"$ls_rv\"},{\"op\":\"test\",\"path\":\"/metadata/uid\",\"value\":\"$ls_uid\"},{\"op\":\"replace\",\"path\":\"/spec/suspend\",\"value\":false}]"; then
246258
return 0
247259
fi
248-
[ "$(date +%s)" -lt "$retry_until" ] || return 1
260+
[ "$(date +%s)" -lt "$stop_at" ] || return 1
249261
sleep 2
250262
done
251263
}
@@ -274,7 +286,7 @@ spec:
274286
fi ;;
275287
esac
276288
fi
277-
[ "$(date +%s)" -lt "$retry_until" ] || return 1
289+
[ "$(date +%s)" -lt "$stop_at" ] || return 1
278290
sleep 2
279291
done
280292
}
@@ -292,6 +304,10 @@ spec:
292304
fi
293305
if [ -z "$present" ]; then
294306
echo "no $deploy Deployment in $NAMESPACE; nothing to stop"
307+
# kubectl scale retries a conflict for up to five minutes on its own,
308+
# but without --current-replicas or --resource-version it sends a
309+
# merge patch of the scale subresource, which carries no
310+
# resourceVersion to conflict on, so that retry never runs here.
295311
elif ! retry kubectl --namespace "$NAMESPACE" scale deployment "$deploy" --replicas=0; then
296312
echo "could not scale $deploy down; no fence was touched" >&2
297313
exit 1
@@ -380,6 +396,7 @@ spec:
380396
total=$((total + 1))
381397
hns=${ref%%/*}
382398
name=${ref#*/}
399+
begin_release
383400
lift_suspension "$hns" "$name"
384401
case $? in
385402
2)
@@ -409,14 +426,17 @@ spec:
409426
if [ "$state" = unhandled ]; then
410427
still="$still $ref"
411428
else
429+
begin_release
412430
report_release "$ref" "$state"
413431
fi
414432
done
415433
pending=$still
416434
done
417435
for ref in $pending; do
436+
begin_release
418437
report_release "$ref" unhandled
419438
done
439+
stop_at=$retry_until
420440
421441
echo "released $released of $total fenced HelmReleases, $unhandled without an uninstall"
422442
# A flux-plunger still running could have fenced a release after the

0 commit comments

Comments
 (0)