Repository navigation
fix(postgres): read the live Cluster when deciding the WAL-archive prefix - #4299
Conversation
mattia-eleuteri
added
the
kind/backport
github-actions
Bot
added
size/L
|
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 (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe backup controller now reads CNPG Clusters through a dynamic client and converts the live objects to the typed Cluster model. Tests verify that backup application uses the live Cluster’s server name and UID. ChangesCNPG live Cluster reads
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change makes in-place PostgreSQL restores use the live Cluster's server name and UID, which fixes the reported failure. No merge-blocking risk was found. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The live read reduces the risk of a backup using a deleted Cluster’s archive prefix or ownership identity. The review found no new access path or material security exposure, but the multi-step update can still race with Cluster replacement. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
mattia-eleuteri NOT LGTM, only for the misplaced doc comments below. The fix itself is right.
Business context: a BackupJob reconcile can read a stale cached Cluster while an in-place restore re-renders it, write the source's serverName back over bootstrap.newServerName, and own the ObjectStore by a UID that no longer exists.
The change is still needed after #4544 and #4443. The hold from #4544 gates only on StartedAt == nil, and reconcileCNPG calls applyClusterPluginBackup on every poll until the backup finishes. A run that started before the restore keeps re-applying the plugin through the whole purge and re-render window, so the stale read is still reachable. The ObjectStore owner UID part is independent of the hold anyway.
I merged the head onto current main locally (clean, both files auto-merge) and go test ./internal/backupcontroller/... passes. Swapping the dynamic read back to r.Get fails TestApplyClusterPluginBackup_ReadsTheLiveClusterNotTheCache on the serverName assertion, so the test pins the fix.
Blocker
The new declarations are inserted between an existing doc comment and the symbol it documents, in both files. Go attaches the whole comment block to the next declaration, so the docs end up on the wrong symbols.
- In the controller, the first two paragraphs of the
applyClusterPluginBackupdoc ("wires the templated strategy...", "Returns an apierrors.IsNotFound error...") now belong tocnpgClusterGVR.go doc -u ./internal/backupcontroller cnpgClusterGVRprints them, and the function keeps only its second half. - In the test,
testCNPGScheme's doc comment now opens the doc ofTestApplyClusterPluginBackup_ReadsTheLiveClusterNotTheCache, andtestCNPGSchemehas none.
Fix: move cnpgClusterGVR above the applyClusterPluginBackup doc block (or below the function), and move the new test and cnpgDynamicFor above the testCNPGScheme comment.
Non-blocking
- The same rationale is repeated in the
cnpgClusterGVRdoc, the long comment inside the function, the test doc and the commit body. One short comment at the read is enough, something like "live read: the cached Cluster can predate a restore re-render, and both serverName and UID below must come from the current object". "This controller watches BackupJob and nothing else" will go stale the day someone adds a watch. E2E Testsis red on a run from 2026-09-16, before the fork e2e lane was fixed on 2026-09-27. It's not caused by this diff, but there is no e2e result for it yet. A rebase onto main gives it a real run.
| // hard validation error (CNPG's Cluster CRD has many required fields the | ||
| // driver does not set), so we surface the precondition explicitly and let | ||
| // the caller treat it as a retryable wait. | ||
| // cnpgClusterGVR reads Clusters through the dynamic client. The typed client |
There was a problem hiding this comment.
This lands in the middle of the applyClusterPluginBackup doc block, so the paragraphs above it are now the godoc of cnpgClusterGVR (go doc -u shows it). Move the var above that block or below the function.
There was a problem hiding this comment.
Moved cnpgClusterGVR above the applyClusterPluginBackup doc block in 67bf9ca. go doc -u ./internal/backupcontroller cnpgClusterGVR now prints only its own one-line doc, and the function has its full doc back.
|
|
||
| // testCNPGScheme returns a runtime.Scheme that knows the unstructured | ||
| // HelmRelease GVK used by the dynamic-client tests above. | ||
| // TestApplyClusterPluginBackup_ReadsTheLiveClusterNotTheCache pins the read |
There was a problem hiding this comment.
Same here: the testCNPGScheme comment just above now documents this test, and testCNPGScheme is left without one. Put the new test and cnpgDynamicFor above that comment.
There was a problem hiding this comment.
Moved the new test and cnpgDynamicFor above the testCNPGScheme comment in 67bf9ca, so testCNPGScheme has its doc back and the test has its own.
…efix
applyClusterPluginBackup preserves the serverName already on the Cluster, so
that a backup does not flip the archive prefix mid-stream. It read that value
through the manager's cached client, and this controller registers
For(&BackupJob{}) and nothing else, so a Cluster only reaches the cache
through a lazily started informer with no resync tied to its own changes.
An in-place RestoreJob deletes the Cluster and the chart re-renders it with
bootstrap.newServerName seconds later. A read served during that window
describes the object that was purged, so the source's serverName is written
back over the freshly rendered one and the restored cluster archives onto the
source's WAL prefix. barman-cloud-check-wal-archive then refuses to start with
"Expected empty archive" and the RestoreJob ends on its deadline -- the
failure newServerName was added to prevent.
The stale read carries a stale UID too, and that UID owns the ObjectStore this
function applies, so the store is garbage-collected as soon as it is written.
Reading through the dynamic client keeps the preservation rule and makes the
value it rests on truthful.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <[email protected]>
db3343b to
67bf9ca
Compare
github-actions
Bot
added
size/M
|
Pushed 67bf9ca, rebased onto current main (clean, still one commit) so the fork e2e lane gets a real run. On the non-blocking comment: the rationale is now a single two-line comment at the read, the |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
mattia-eleuteri LGTM. The doc comments are fixed. go doc -u now shows cnpgClusterGVR with its own two lines and applyClusterPluginBackup with its full doc, and in the test file testCNPGScheme has its own doc again, separate from the new helper.
The fix still holds on current main. BackupJobReconciler sets up its dynamic client in SetupWithManager, so the live read works in the real binary and not only in tests, and the ObjectStore owner UID now comes from the same live object. go test ./internal/backupcontroller/... passes on 67bf9ca. When I put the cached r.Get back in place of the dynamic read, TestApplyClusterPluginBackup_ReadsTheLiveClusterNotTheCache fails with "effective serverName = postgres-app".
CI is red, but not because of this diff. "Unit & controller tests" failed while downloading k8s.io/kube-openapi for the kube-ovn-webhook module (a stream error from proxy.golang.org). internal/backupcontroller passed in that same job. Because the Pull Request run failed, the fork e2e lane posted "e2e not run", so E2E Tests is red without a real e2e run (run 36589358812, attempt 1). The rebase was meant to get a real e2e run, so that job needs a rerun before merge.
7d44372
into
cozystack:main
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-4299-to-release-1.6
git worktree add --checkout .worktree/backport-4299-to-release-1.6 backport-4299-to-release-1.6
cd .worktree/backport-4299-to-release-1.6
git reset --hard HEAD^
git cherry-pick -x 67bf9ca317879299241bea62b0a172dfc01ea5a0
git push --force-with-lease |
3 participants
What this PR does
Fixes #4298.
applyClusterPluginBackuppreserves theserverNamealready on the Cluster so that a backup does not flip the WAL-archive prefix mid-stream, which would split the archive across two prefixes. That rule is right; the read it rests on is not reliable in one window.The value was read through the manager's cached client, and
BackupJobReconciler.SetupWithManagerregistersFor(&BackupJob{})and nothing else, so a Cluster only reaches the cache through a lazily started informer with no resync tied to the Cluster's own changes. An in-placeRestoreJobdeletes the Cluster and the chart re-renders it withbootstrap.newServerNameseconds later, so a read served during that window describes the object that was purged — and the source'sserverNameis written back over the freshly rendered one.barman-cloud-check-wal-archivethen refuses the destination withExpected empty archiveand theRestoreJobends on its 30-minute deadline, which is the failurenewServerNamewas added in #3988 to prevent.The same stale read carries a stale
UID, and thatUIDowns theObjectStorethis function applies, so the store is garbage-collected as soon as it is written.Reading through the dynamic client this reconciler already carries keeps the preservation rule and makes the value it rests on truthful. There is precedent in the same file: the HelmRelease suspend/resume path reads that way for the same reason.
Observed on a v1.6.3 cluster, twice on separate runs with the same four-second gap between the chart's apply and the driver's. The regression test makes the two clients disagree the way they disagree in that window and asserts the driver believes the live one; reverting the fix fails it on the serverName assertion.
Screenshots
Downstream repositories
Release note
Summary by CodeRabbit