flux-plunger: follow-ups from the #4293 review (hook report, drain timeout and events, suspension Warning) #4782

Describe the bug

Follow-up to #4293, which closes #4060 with an uninstall fence, a shutdown drain under leader election, and a pre-delete hook as the backstop. The last review round on that PR raised the points below; both reviewers agreed none of them blocks the merge, and they are collected here so they are fixed rather than lost. Each is grouped by where it lives and carries the reviewer's severity, with the second opinion where one was given.

Pre-delete hook (packages/system/flux-plunger/templates/uninstall-fence-cleanup.yaml)

• The re-listing after the sweep runs only when the pod wait failed (raised as MAJOR, second opinion: hardening). kubectl wait --for=delete on a selector that matches nothing succeeds at once, so a flux-plunger the selector cannot see (a second install under another release name, a relabelled pod, a binary run out of cluster) passes as gone, and a fence it sets after the listing survives a hook that exits 0. Counter-argument recorded in review: such a process was never stopped by the hook, still owns its fences and drains them when it stops, and an unconditional re-list does not close the class either, while with two installs it would fail the uninstall for as long as the other one recovers anything. Decide between an unconditional re-list and documenting the boundary.
• A deleting release whose suspension another manager co-owns is reported as cleanly released (raised as MAJOR, second opinion: MINOR). lift_suspension correctly leaves such a suspension, so no generation bump happens and the unhandled predicate stays silent, and the line reads released ... while the controller records a Warning for the same state. Add a branch to the state template that names a release being deleted under another manager's suspension, and a bats fixture combining deleting with co-owned. The same review asked for resourceVersion/uid preconditions on the fence removal; the second opinion is that they add nothing there, because the hook takes no decision on what it read and the test op already pins the index.
• The retry budget is global and spent in listing order (MINOR). One release that can never be read takes the whole window, and every release listed after it gets a single attempt. A per-release floor of attempts would make the first uninstall attempt as good as the last.
• The retry window starts from the script's clock, not the Job's (MINOR). activeDeadlineSeconds counts from the Job's startTime; the one-minute margin is a fixed offset, so a slow schedule and image pull come out of it. Correct the comment, or read the Job's start. kubectl scale also retries conflicts for up to five minutes on its own, a bound none of the chart's timings accounts for.
• finalizers.fluxcd.io is a literal in the hook (NIT), vouched for only by a copy in the test fake. Tie it to helmv2.HelmReleaseFinalizer in hook_contract_test.go, whose regex currently matches only cozystack.io/ literals.

Shutdown drain (internal/controller/fluxplunger/drain.go)

• A non-positive --shutdown-drain-timeout makes the drain do nothing (raised as MAJOR, second opinion: follow-up). The contract test ties the chart's subtraction to managerStopTimeout + killMargin, but not the 40..140 range guard, so raising the Go constant and the subtraction leaves the floor at 40 and renders a negative timeout there. Refuse a non-positive value at startup in cmd/flux-plunger/main.go, and derive the chart's floor from the same constants rather than writing 40; README.md repeats the numbers in prose.
• Drain Events are not folded (MINOR). apiEventRecorder creates a new Event per call, so a release the drain revisits every second gets one object per pass (about 65 at the default drain, 115 at the maximum), while recordEvent's comment promises a correlator. Each write also takes its own 5s deadline outside the drain's budget; derive it from the drain's context with a cap.
• The drain's outcome is one Info line (MINOR). Fences left behind log at Info only, can print the held: -1 sentinel when no listing ever succeeded, and a process that never led logs "Released every uninstall fence" next to "Never became leader". No Event is emitted for fences left behind.
• Three drain mutations survive the unit and envtest suites (MINOR): removing the outcome log, making a per-release failure end the drain instead of being retried (drain.go:78), and removing the wait between passes, which turns the drain into a tight cluster-wide LIST loop. Add tests that hold each, including one that counts listings over a fixed window.
• Two comments claim more than the code holds (NIT). After a stop that overran its grace period, an in-flight reconcile can still land a fence after the drain's last listing, so the "no concurrent owner" comment at flux_plunger.go:182 should say so. RunManager gates on a non-blocking read of mgr.Elected(), which controller-runtime closes only after its leader-election runnables start, so a process stopped inside that call holds the Lease but takes the never-led branch.

Controller (internal/controller/fluxplunger/flux_plunger.go)

• The SuspensionNotOwned Warning keys on a permanent marker (MINOR). flux-plunger.cozystack.io/last-processed-version is never removed, so every later suspension on a recovered release, by a migration, a backup restore or an operator, raises a Warning that suggests resuming it, which is the harmful action during a restore. It needs a different key; the annotation itself cannot be cleared, because it also carries the already-processed guard.
• A restore that starts while flux-plunger holds its own suspension does not take the field (MINOR, predates fix(fluxcd): keep Helm uninstall working during flux-plunger recovery #4293). The etcd and CNPG restore helpers skip the write when the value already matches, so flux-plunger's relinquish resets spec.suspend to false under a running restore. The window is the few calls between acquire and relinquish; at least add it to the PR's Remaining limitations and the chart README.

Environment

• Cozystack version: main after fix(fluxcd): keep Helm uninstall working during flux-plunger recovery #4293
• Provider: any

To Reproduce

Each item above names the file and the case; the review threads on #4293 carry a reproduction for each.

Expected behaviour

Every fence is accounted for in the hook's report, a misconfigured drain timeout is refused at startup, the drain neither floods Events nor spins, and the suspension Warning does not point an operator at a restore.

Actual behaviour

As listed per item.

Logs

See the review threads on #4293.

Screenshots

Not applicable.

Additional context

None of these blocks #4293: both reviewers approved it with these points left as follow-up.

Checklist

• I have checked the documentation
• I have searched for similar issues
• I have included all required information
• I have provided clear steps to reproduce
• I have included relevant logs