Skip to content

Commit 7b84919

Browse files
scooby87scooby87
committed
fix(fluxcd): warn about a foreign suspension only on a stuck release
flux-plunger raised its SuspensionNotOwned and TerminatingWhileSuspended Warnings on a suspension it does not own when the release carried its processed-version annotation. That annotation is never removed, so every later suspension of such a release, by a migration, a backup restore or an operator, drew a Warning: one telling the reader to resume it, which mid-restore is the one thing nobody wants, and one calling a migration's deliberate suspend-then-delete an accident. Both also missed the leftover they are for when the first recovery stopped before writing the annotation. A suspension an earlier flux-plunger left mid-recovery freezes the release in the failure recovery started from, since helm-controller rewrites no condition of a suspended release. Both Warnings now key on that state instead, and the README's candidate query with them. Two comments are qualified where they claimed more than holds: a stop that overran the manager's grace period can still let a reconcile set a fence after the drain's last listing, and a process stopped before controller-runtime closes Elected holds the Lease yet does not drain. Assisted-by: LLM Signed-off-by: Alexey Artamonov <[email protected]>
1 parent caeaa6b commit 7b84919

4 files changed

Lines changed: 93 additions & 66 deletions

File tree

‎internal/controller/fluxplunger/drain.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,10 @@ type drainingManager interface {
179179
// informer cache is frozen and would show fences already gone. Only a
180180
// flux-plunger that led drains: one that never did stops while the leader goes
181181
// on, and the fences are the leader's. Without leader election Elected is closed
182-
// from the start.
182+
// from the start. controller-runtime closes Elected only once its
183+
// leader-election runnables have started, so a process stopped in between holds
184+
// the Lease yet takes the never-led path; it has run no reconcile, so the
185+
// fences it skips are ones it inherited, and the next leader takes them over.
183186
func RunManager(ctx context.Context, mgr drainingManager, drain time.Duration) error {
184187
return RunThenDrain(ctx, mgr.Start, func(dctx context.Context) (int, error) {
185188
select {

‎internal/controller/fluxplunger/flux_plunger.go‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -180,17 +180,24 @@ func (r *FluxPlunger) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Res
180180
// the holder of the leader Lease reconciles, a stopping instance keeps that
181181
// Lease through its drain (ApplyShutdownTimings), and controller-runtime
182182
// serialises reconciles per key within the one process, so there is no
183-
// concurrent owner to race. Releasing it un-sticks the release and lets
184-
// error-recovery start over. A suspension flux-plunger does not own belongs to
185-
// an operator or another controller and must be left untouched.
183+
// concurrent owner to race. The one exception is a stop that overran the
184+
// manager's grace period: Start then returns without joining its workers,
185+
// and a reconcile still in flight can set a fence after the drain's last
186+
// listing, which the next leader or the pre-delete hook then releases.
187+
// Releasing it un-sticks the release and lets error-recovery start over. A
188+
// suspension flux-plunger does not own belongs to an operator or another
189+
// controller and must be left untouched.
186190
if !r.ownsSuspension(hr) {
187191
logger.Info("HelmRelease is suspended by external process, skipping")
188-
// On a release flux-plunger recovered before, the suspension is most
189-
// likely one an earlier flux-plunger left mid-recovery under the shared
190-
// flux-client-side-apply manager, which no one will lift unless told.
191-
if _, recovered := hr.Annotations[annotationLastProcessedVersion]; recovered {
192+
// A release frozen in the failure recovery starts from is most likely
193+
// one an earlier flux-plunger left suspended mid-recovery under the
194+
// shared flux-client-side-apply manager, which no one will lift unless
195+
// told: helm-controller rewrites no condition of a suspended release.
196+
// The processed-version annotation is no sign of it, since it stays on
197+
// every release flux-plunger ever recovered.
198+
if hasNoDeployedReleasesError(hr) {
192199
r.recordEvent(hr, corev1.EventTypeWarning, "SuspensionNotOwned",
193-
"Left a suspension flux-plunger does not own untouched on a release it has recovered before; if an earlier flux-plunger left it, resume the release (see the flux-plunger chart README)")
200+
"Left a suspension flux-plunger does not own untouched on a release stuck with no deployed releases; if an earlier flux-plunger left it mid-recovery, resume the release (see the flux-plunger chart README)")
194201
} else {
195202
r.recordEvent(hr, corev1.EventTypeNormal, "SuspensionNotOwned",
196203
"Left an externally-owned suspension untouched; flux-plunger only clears suspensions it set")
@@ -406,13 +413,14 @@ func (r *FluxPlunger) reconcileTerminating(ctx context.Context, hr *helmv2.HelmR
406413
// Another manager's suspension: helm-controller skips the uninstall for it
407414
// and leaves the release's resources in place, which a migration that
408415
// suspends then deletes relies on. That is not flux-plunger's decision to
409-
// override, and not a release its fence should hold. A release that
410-
// carries flux-plunger's processed-version annotation was recovered before,
411-
// so the suspension that skips its uninstall was not a migration's choice (an
412-
// operator's hand, or one an earlier flux-plunger release left behind): that
413-
// is the orphaning this controller exists to prevent, and a Warning.
416+
// override, and not a release its fence should hold. A release frozen in
417+
// the failure recovery starts from is most likely suspended by an earlier
418+
// flux-plunger that stopped mid-recovery, not by a migration's choice: that
419+
// is the orphaning this controller exists to prevent, and a Warning. The
420+
// processed-version annotation is no sign of it, since it stays on every
421+
// release flux-plunger ever recovered.
414422
logger.Info("HelmRelease is being deleted while suspended by another manager; helm-controller skips the uninstall")
415-
if _, recovered := hr.Annotations[annotationLastProcessedVersion]; recovered {
423+
if hasNoDeployedReleasesError(hr) {
416424
r.recordEvent(hr, corev1.EventTypeWarning, "TerminatingWhileSuspended",
417425
"HelmRelease is being deleted while suspended by another manager; helm-controller skips the Helm uninstall for a suspended release, so its resources are left behind")
418426
} else {

‎internal/controller/fluxplunger/flux_plunger_test.go‎

Lines changed: 65 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1193,25 +1193,36 @@ func TestReconcile_EmitsEventOnExternalSuspension(t *testing.T) {
11931193
}
11941194
}
11951195

1196-
// A suspension flux-plunger does not own on a release flux-plunger has recovered
1197-
// before is most likely one an earlier flux-plunger left mid-recovery under the
1198-
// shared field manager: it stays suspended for good unless someone acts, so it
1199-
// is a Warning, while one on a release flux-plunger never touched stays Normal.
1200-
func TestReconcile_ExternalSuspensionOnARecoveredReleaseWarns(t *testing.T) {
1196+
// A suspension flux-plunger does not own is most likely one an earlier
1197+
// flux-plunger left mid-recovery, under the shared field manager, when the
1198+
// release is frozen in the failure recovery starts from: helm-controller
1199+
// rewrites no condition of a suspended release, so a release suspended
1200+
// mid-recovery keeps "has no deployed releases" for good unless someone acts,
1201+
// and that is a Warning. The processed-version annotation is no sign of it: it
1202+
// stays on every release flux-plunger ever recovered, so a later restore,
1203+
// migration or operator suspension of a healthy release would be told to resume
1204+
// it, and a first recovery left mid-flight had not written it yet.
1205+
func TestReconcile_ExternalSuspensionOnAStuckReleaseWarns(t *testing.T) {
12011206
for _, tc := range []struct {
12021207
name string
12031208
recovered bool
1209+
stuck bool
12041210
want string
12051211
}{
1206-
{"recovered before", true, corev1.EventTypeWarning},
1207-
{"never recovered", false, corev1.EventTypeNormal},
1212+
{"stuck, recovered before", true, true, corev1.EventTypeWarning},
1213+
{"stuck, first recovery", false, true, corev1.EventTypeWarning},
1214+
{"healthy, recovered before", true, false, corev1.EventTypeNormal},
1215+
{"healthy, never recovered", false, false, corev1.EventTypeNormal},
12081216
} {
12091217
t.Run(tc.name, func(t *testing.T) {
12101218
hr := noDeployedReleasesHR()
12111219
hr.Spec.Suspend = true
12121220
if tc.recovered {
12131221
hr.Annotations = map[string]string{annotationLastProcessedVersion: "3"}
12141222
}
1223+
if !tc.stuck {
1224+
hr.Status.Conditions = []metav1.Condition{{Type: "Ready", Status: metav1.ConditionTrue, Reason: "InstallSucceeded", Message: "Helm install succeeded"}}
1225+
}
12151226
c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr).
12161227
WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build()
12171228
rec := record.NewFakeRecorder(8)
@@ -1750,6 +1761,7 @@ func TestReconcile_LiftsFenceOnlyOnceUninstallReached(t *testing.T) {
17501761
func TestReconcile_TerminatingForeignSuspensionReleasesFence(t *testing.T) {
17511762
hr := terminatingFixture(fencedFixture(noDeployedReleasesHR()))
17521763
hr.Spec.Suspend = true // not flux-plunger's
1764+
hr.Status.Conditions = []metav1.Condition{{Type: "Ready", Status: metav1.ConditionTrue, Reason: "UpgradeSucceeded", Message: "Helm upgrade succeeded"}}
17531765

17541766
c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr).
17551767
WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build()
@@ -1778,6 +1790,52 @@ func TestReconcile_TerminatingForeignSuspensionReleasesFence(t *testing.T) {
17781790
}
17791791
}
17801792

1793+
// A release deleted under a suspension flux-plunger does not own loses its
1794+
// resources either way; that is an accident, and a Warning, when the suspension
1795+
// is most likely one an earlier flux-plunger left mid-recovery, which freezes
1796+
// the release in the failure recovery starts from. A migration that suspends
1797+
// then deletes a healthy release meant it, whatever annotation an earlier
1798+
// recovery left on it.
1799+
func TestReconcile_TerminatingForeignSuspensionWarnsOnlyOnAStuckRelease(t *testing.T) {
1800+
for _, tc := range []struct {
1801+
name string
1802+
recovered bool
1803+
stuck bool
1804+
want string
1805+
}{
1806+
{"stuck, recovered before", true, true, corev1.EventTypeWarning},
1807+
{"stuck, first recovery", false, true, corev1.EventTypeWarning},
1808+
{"healthy, recovered before", true, false, corev1.EventTypeNormal},
1809+
{"healthy, never recovered", false, false, corev1.EventTypeNormal},
1810+
} {
1811+
t.Run(tc.name, func(t *testing.T) {
1812+
hr := terminatingFixture(fencedFixture(noDeployedReleasesHR()))
1813+
hr.Spec.Suspend = true
1814+
if tc.recovered {
1815+
hr.Annotations = map[string]string{annotationLastProcessedVersion: "3"}
1816+
}
1817+
if !tc.stuck {
1818+
hr.Status.Conditions = []metav1.Condition{{Type: "Ready", Status: metav1.ConditionTrue, Reason: "UpgradeSucceeded", Message: "Helm upgrade succeeded"}}
1819+
}
1820+
c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr).
1821+
WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build()
1822+
rec := record.NewFakeRecorder(8)
1823+
r := &FluxPlunger{Client: c, owns: testOwns, Recorder: rec}
1824+
1825+
reconcile(t, r)
1826+
1827+
select {
1828+
case e := <-rec.Events:
1829+
if !strings.HasPrefix(e, tc.want+" TerminatingWhileSuspended ") {
1830+
t.Fatalf("want a %s TerminatingWhileSuspended event, got %q", tc.want, e)
1831+
}
1832+
default:
1833+
t.Fatal("no event")
1834+
}
1835+
})
1836+
}
1837+
}
1838+
17811839
// A fenced, terminating, unsuspended release that helm-controller has not handled
17821840
// yet is held by flux-plunger's fence. That hold must not be silent: the reconcile
17831841
// requeues itself so it is re-checked without waiting for an event, and once the
@@ -1835,48 +1893,6 @@ func TestReconcile_HeldFenceRequeuesAndWarnsPastTheDeadline(t *testing.T) {
18351893
}
18361894
}
18371895

1838-
// The same terminating-under-a-foreign-suspension state is two different things.
1839-
// A migration that suspends then deletes chose to keep the resources, so it is
1840-
// Normal. A release flux-plunger has recovered before (it carries the
1841-
// processed-version annotation: an operator suspended it by hand and deleted it,
1842-
// or an earlier flux-plunger release left it suspended) is losing its uninstall by
1843-
// accident, the failure this controller exists to prevent, so it is a Warning.
1844-
func TestReconcile_TerminatingForeignSuspensionWarnsOnlyForAPreviouslyRecoveredRelease(t *testing.T) {
1845-
cases := []struct {
1846-
name string
1847-
annotated bool
1848-
wantType string
1849-
}{
1850-
{"never recovered by flux-plunger (migration)", false, corev1.EventTypeNormal},
1851-
{"recovered by flux-plunger before", true, corev1.EventTypeWarning},
1852-
}
1853-
for _, tc := range cases {
1854-
t.Run(tc.name, func(t *testing.T) {
1855-
hr := terminatingFixture(noDeployedReleasesHR())
1856-
hr.Spec.Suspend = true // not flux-plunger's
1857-
if tc.annotated {
1858-
hr.Annotations = map[string]string{annotationLastProcessedVersion: "3"}
1859-
}
1860-
1861-
c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithObjects(hr).
1862-
WithInterceptorFuncs(interceptor.Funcs{Patch: ssaModelApply}).Build()
1863-
rec := record.NewFakeRecorder(8)
1864-
r := &FluxPlunger{Client: c, owns: testOwns, Recorder: rec}
1865-
1866-
reconcile(t, r)
1867-
1868-
select {
1869-
case e := <-rec.Events:
1870-
if !strings.HasPrefix(e, tc.wantType+" TerminatingWhileSuspended ") {
1871-
t.Fatalf("expected a %s TerminatingWhileSuspended event, got %q", tc.wantType, e)
1872-
}
1873-
default:
1874-
t.Fatalf("expected a TerminatingWhileSuspended event, got none")
1875-
}
1876-
})
1877-
}
1878-
}
1879-
18801896
// A fence left without a suspension on a live release (another writer set
18811897
// spec.suspend=false and took the field), or a fence whose suspension another
18821898
// manager took over, guards nothing and must be lifted, without starting a recovery.

‎packages/system/flux-plunger/README.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,10 @@ kubectl get helmreleases.helm.toolkit.fluxcd.io --all-namespaces --output go-tem
2222

2323
## Suspensions left by an earlier flux-plunger
2424

25-
Earlier flux-plunger releases set `spec.suspend` through the shared `flux-client-side-apply` manager, so a suspension one of them left behind cannot be told apart from a migration's or an operator's and is treated as external: it is not cleared automatically. Such a leftover exists only if the old pod stopped in the middle of a recovery, and flux-plunger raises a `SuspensionNotOwned` Warning on it. To list candidates, suspended releases carrying the flux-plunger annotation whose `spec.suspend` is not owned by flux-plunger's apply:
25+
Earlier flux-plunger releases set `spec.suspend` through the shared `flux-client-side-apply` manager, so a suspension one of them left behind cannot be told apart from a migration's or an operator's and is treated as external: it is not cleared automatically. Such a leftover exists only if the old pod stopped in the middle of a recovery, and since helm-controller rewrites no condition of a suspended release, it stays frozen in the failure recovery started from: `Ready=False` with `has no deployed releases`. flux-plunger raises a `SuspensionNotOwned` Warning on a release in that state, and a Normal event on any other suspension it does not own; the processed-version annotation is no sign of a leftover, since it stays on every release flux-plunger ever recovered. To list candidates, suspended releases in that state whose `spec.suspend` is not owned by flux-plunger's apply:
2626

2727
```bash
28-
kubectl get helmreleases.helm.toolkit.fluxcd.io --all-namespaces --output json | jq --raw-output '.items[] | select(.spec.suspend == true) | select(.metadata.annotations["flux-plunger.cozystack.io/last-processed-version"] != null) | select([.metadata.managedFields[]? | select(.manager == "flux-plunger" and .operation == "Apply" and .fieldsV1["f:spec"]["f:suspend"] != null)] | length == 0) | "\(.metadata.namespace)/\(.metadata.name)"'
28+
kubectl get helmreleases.helm.toolkit.fluxcd.io --all-namespaces --output json | jq --raw-output '.items[] | select(.spec.suspend == true) | select([.status.conditions[]? | select(.type == "Ready" and .status == "False" and (.message // "" | contains("has no deployed releases")))] | length > 0) | select([.metadata.managedFields[]? | select(.manager == "flux-plunger" and .operation == "Apply" and .fieldsV1["f:spec"]["f:suspend"] != null)] | length == 0) | "\(.metadata.namespace)/\(.metadata.name)"'
2929
```
3030

3131
Clearing one is an operator action: confirm nothing else (a migration, a restore) is holding it, then resume it with `flux resume helmrelease NAME --namespace NAMESPACE`.

0 commit comments

Comments
 (0)