fix(api): keep a suspended HelmRelease suspended across an app update - #4411
Conversation
Update rebuilds the HelmRelease from the Application and writes it back whole, and the Application carries no suspension, so every edit made through the API resumed a release that an operator or a controller had suspended. The CNPG restore driver depends on that suspension. It suspends the target release, patches the Postgres app through this API, purges the Cluster, and resumes only once the purge is done, so the chart's next render lands bootstrap.recovery on an empty namespace. Resumed by the patch instead, helm-controller upgrades straight away: the chart can render the recovery Cluster while the purge is still running, the driver then deletes it as the Cluster being replaced, and nothing renders it again because the values have not changed since. The RestoreJob sits in Running until its deadline. On main this has been masked by timing rather than prevented. The same write also dropped the Flux finalizer, so helm-controller's first pass after it only put the finalizer back and requeued, which held the upgrade back by at least its 750ms minimum retry delay. A branch that keeps the finalizer removes that delay, and the backup round-trip failed on both of its runs there. The suspension is re-read on a conflict retry as well, since the write that caused the conflict may be the suspend itself. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <[email protected]>
github-actions
Bot
added
area/api
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesApplication update
Priority: ⬆️ High Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Application updates are described as preserving HelmRelease suspension, including after a conflicting write. No specific issue remains that would prevent merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
Carrying spec.suspend across the rebuild is right, and both new tests hold it under mutation. What breaks is the field next to it. The same write still rebuilds metadata.annotations from scratch, and one of the annotations it throws away is the only thing that can resume the release afterwards.
Findings
- [MAJOR]
pkg/registry/apps/application/rest.go:605, an app edit during a flux-plunger operation leaves the release suspended with nothing left to resume it
- [MAJOR]
pkg/registry/apps/application/rest.go:605, an edit against a suspended release is accepted, dropped, and never reported
- [MINOR]
pkg/registry/apps/application/rest.go:628, the retry refreshes suspend but not the shard label
- [MINOR]
pkg/registry/apps/application/rest.go:564, the "live HelmRelease" this now reads suspension from is a cache read
Caveats
E2E (in-tree)hasn't reported on 5df219b, so thepostgres-2-backup-roundtripevidence the PR rests on is unreproduced here.- Pre-existing and not a finding against this PR, but adjacent to it: the CNPG restore driver resumes best-effort on its failure paths and discards the error (
cnpgstrategy_controller.go:883and:887) right before the job goes terminal, so a failed restore can already strand a suspension. Until now an app edit was the accidental way out. - Whether the dashboard surfaces a suspended release is unchecked. That lives out of tree.
| // bootstrap.recovery on an empty namespace. Resumed early, the release | ||
| // renders while the purge is still running, and the purge then deletes | ||
| // what it rendered. | ||
| helmRelease.Spec.Suspend = cur.Spec.Suspend |
There was a problem hiding this comment.
[MAJOR] an app edit during a flux-plunger operation leaves the release suspended with nothing left to resume it
ConvertApplicationToHelmRelease builds metadata.annotations purely from the Application's apps.cozystack.io- prefixed ones (rest.go:1657), and Update merges nothing back from cur the way it does for the shard label at rest.go:592. flux-plunger suspends a release (flux_plunger.go:258), deletes the newest Helm secret, then writes flux-plunger.cozystack.io/last-processed-version (flux_plunger.go:297). Its resume hangs entirely on reading that annotation back: flux_plunger.go:68-74 is the only route to unsuspendHelmRelease, and without the annotation line 87 logs HelmRelease is suspended by external process, skipping and returns on every pass. It cannot be rewritten either, because the write at :297 sits past the hr.Spec.Suspend early return at :45, and the watch predicate at :320 stops enqueueing the object once the annotation is gone.
Before this change an app edit in that window sent suspend=false. Wrong for the restore driver, which is what this PR fixes, but the release came back and reconciled. Now it stays suspended with no annotation, and the enumeration below is the whole set of writers: only flux-plunger:278 and the CNPG restore driver's helper ever resume an app HelmRelease, and neither will. A tenant cannot patch their way out, because no access-tier ClusterRole grants helm.toolkit.fluxcd.io at all. The window is wider than the seconds between the annotation write and the next reconcile: the predicate's own comment calls the annotated-and-suspended state crash recovery, so it also spans however long the plunger is down.
$ grep -rn --include='*.go' 'Spec\.Suspend' internal/ pkg/ | grep -v _test.go
internal/controller/fluxplunger/flux_plunger.go:45: if hr.Spec.Suspend {
internal/controller/fluxplunger/flux_plunger.go:253: if latestHR.Spec.Suspend {
internal/controller/fluxplunger/flux_plunger.go:258: latestHR.Spec.Suspend = true
internal/controller/fluxplunger/flux_plunger.go:273: if !latestHR.Spec.Suspend {
internal/controller/fluxplunger/flux_plunger.go:278: latestHR.Spec.Suspend = false
internal/controller/fluxplunger/flux_plunger.go:319: if hr.Spec.Suspend && hr.Annotations != nil {
internal/operator/package_reconciler.go:454: hr.Spec.Suspend = existing.Spec.Suspend
pkg/registry/apps/application/rest.go:605: helmRelease.Spec.Suspend = cur.Spec.Suspend
pkg/registry/apps/application/rest.go:628: helmRelease.Spec.Suspend = cur.Spec.Suspend
$ grep -rn 'annotationLastProcessedVersion' internal/controller/fluxplunger/flux_plunger.go
21: annotationLastProcessedVersion = "flux-plunger.cozystack.io/last-processed-version"
68: if processedVersionStr, exists := hr.Annotations[annotationLastProcessedVersion]; exists {
119: if processedVersionStr, exists := hr.Annotations[annotationLastProcessedVersion]; exists {
297: latestHR.Annotations[annotationLastProcessedVersion] = strconv.Itoa(version)
320: if _, exists := hr.Annotations[annotationLastProcessedVersion]; exists {
$ grep -rn 'helm.toolkit.fluxcd.io' packages/system/cozystack-basics/templates/clusterroles.yaml
$
Annotations want the same treatment labels already get a few lines above: keep the live ones that the Application does not own, and let the prefixed ones come from the Application. That leaves the write surface where it is today, since a tenant can still only set apps.cozystack.io- annotations through this API and still cannot touch anything else on the HelmRelease. A regression test in the shape of the two you added: seed flux-plunger.cozystack.io/last-processed-version on the live release, run Update, assert it survives. It is red today.
| // bootstrap.recovery on an empty namespace. Resumed early, the release | ||
| // renders while the purge is still running, and the purge then deletes | ||
| // what it rendered. | ||
| helmRelease.Spec.Suspend = cur.Spec.Suspend |
There was a problem hiding this comment.
[MAJOR] an edit against a suspended release is accepted, dropped, and never reported
Suspend appears in rest.go only at 605, 618 and 628, so it never reaches ConvertHelmReleaseToApplication: the response and every later GET show the caller their new spec, with status.version and the Ready/Released conditions frozen at whatever they were before the suspension, because helm-controller stops writing status while suspended. Meanwhile the chart keeps running the old values. Before this change the edit landed. The in-repo shape for exactly this is already two thousand lines down, in warnRemovedKubernetesFields (rest.go:1998): an AddWarning telling the caller the write had no effect beats silently giving them nothing, and it costs a few lines here.
$ grep -n 'Suspend' pkg/registry/apps/application/rest.go
605: helmRelease.Spec.Suspend = cur.Spec.Suspend
618: // live object and retry. Suspend is refreshed with it, because the write
628: helmRelease.Spec.Suspend = cur.Spec.Suspend
| return getErr | ||
| } | ||
| helmRelease.SetResourceVersion(cur.GetResourceVersion()) | ||
| helmRelease.Spec.Suspend = cur.Spec.Suspend |
There was a problem hiding this comment.
[MINOR] the retry refreshes suspend but not the shard label
The new comment's own sentence, that the write which caused the conflict may be the one that suspended the release, reads exactly the same way for the flux-shard-operator's label that rest.go:587-591 says must not revert to the ApplicationDefinition default. The retry re-sends the pre-conflict value. Pre-existing, and the placement controller repairs it, so this is a transient shard bounce rather than lost state, but the asymmetry is new to the eye now that the line below it does the right thing.
| // Fetch the live HelmRelease: it backs the ResourceVersion when the | ||
| // converted object carries none, and runtime-managed labels are carried | ||
| // over from it below. | ||
| // converted object carries none, and runtime-managed labels and the |
There was a problem hiding this comment.
[MINOR] the "live HelmRelease" this now reads suspension from is a cache read
r.c is mgr.GetClient() (pkg/apiserver/apiserver.go:203) with a HelmRelease informer registered at apiserver.go:183, so the Get is served from the indexer and the Raw: &metav1.GetOptions{} is ignored. It fails closed, since a stale read carries a stale resourceVersion and 409s into the retry, but the retry's own Get goes through the same cache, so a lagging informer can burn the whole backoff and hand the caller a 409. The comment right above the line now leans on that read being live, which it is not.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
My earlier REQUEST_CHANGES on this PR was wrong on severity, and this review replaces it. Nothing below blocks the merge.
What I graded wrong
I filed two of the four findings as blockers. Neither one is.
The second, that an edit against a suspended release is accepted and never applied, is not a defect at all. That is what spec.suspend means. A missing AddWarning is a suggestion about ergonomics, and I should have filed it as one.
The first, the wiped flux-plunger.cozystack.io/last-processed-version annotation, has a real mechanism and I stand behind the trace. The grading around it was still wrong twice over. The precondition is narrow: the release has to already be in the has no deployed releases state that flux-plunger exists for, and an app edit has to land inside the window before the next reconcile. Recovery is one kubectl patch by a platform admin, and nothing is lost. The wipe itself also predates this PR. metadata.annotations was already rebuilt from scratch on every Update, and only its consequence moves here.
I called that consequence a regression too. It isn't. The behaviour it replaces, an app edit accidentally resuming the release, is the bug this PR fixes, and losing an accidental side effect of a defect is not a regression. I applied that rule by its letter instead of its meaning.
The fix itself is correct, and the PR improves the path it targets.
What is left
-
Merge this PR.
Nothing in my review holds it. -
Decide where the annotation wipe gets fixed: in this PR, or in its own.
It is pre-existing and it reaches further than suspension, so either answer is defensible. Items 3 and 4 are what each one costs you. -
Fix it here: carry the live annotations the Application does not own over from
cur, the way the shard label already is at rest.go:592.
A test in the shape of the two you wrote: seed the annotation on the live release, run Update, assert it survives. Red today. -
Fix it separately: say so on this PR, and I won't raise it again next round.
-
Add an
AddWarningwhen the target release is suspended, or drop the idea.
warnRemovedKubernetesFields(rest.go:1998) is the shape. Your call entirely, I'm not asking for it. -
Ignore the two minor comments at rest.go:628 and rest.go:564, or fix them.
Both are pre-existing, and both either self-heal or fail closed. The inline comments stay for the record.
2 participants
What this PR does
This PR makes
REST.Updatekeep livespec.suspendof the HelmRelease, on first write and on conflict retry. Update rebuilds HelmRelease from Application and writes it back whole, Application has no suspension field, so every edit through aggregated API resumed a release that operator or controller had suspended.Caller that depends on it is cnpg restore driver. It suspends target release, patches Postgres app through this API, purges Cluster and resumes only after purge is done, so next chart render lands
bootstrap.recoveryon empty namespace. When the patch resumes release, helm-controller upgrades right away, chart can render recovery Cluster while purge is still running, then driver deletes it as the Cluster being replaced and nothing renders it again, because values do not change anymore. RestoreJob staysRunninguntil its deadline.On main it works only because of timing. Same write also drops
finalizers.fluxcd.io, and helm-controller v1.5.0 spends its first pass after that on putting finalizer back and requeue (750ms minimum retry delay) before it upgrades, this delay usually lets purge finish first. #3426 keeps controller finalizers across this write, so delay is gone, andpostgres-2-backup-roundtripfailed on both its runs there while it passes on other PRs. In 35714873620 stuck restore ispg-src-to-pg-target-pitr:startedAt11:40:37, helm upgrade v3 at 11:40:37, new Cluster created at 11:40:38 and gone by 11:40:47, no render after that. 35652855170 has same picture on first restore, all inside 22:10:51.Tests are in
rest_suspend_test.go: edit leavesspec.suspendas it was (both values) with values still taken from Application, and suspend that lands between read and write survives conflict retry. Removing either line fails its test.Downstream repositories
Change is in how aggregated API writes HelmRelease back, no downstream repo restates that. Rollback guide on website (
guides/concepts.md) tells operators to suspend HelmRelease while they work, with this fix that suspension holds across app edit, so docs need no change.Release note
Summary by CodeRabbit