Repository navigation
fix(backups/postgres): purge stale recovery cluster on repeat in-place restore - #3318
Conversation
…e restore A repeat in-place PostgreSQL RestoreJob into an already-restored target silently no-opped: the destructive purge was skipped because the live Cluster still carried spec.bootstrap.recovery left over from the previous restore, and the re-purge guard mistook that for a freshly-recovered Cluster produced by this restore's own purge. The job then reported Succeeded while the PVC, disk, and data were never touched. Tell the two apart by the Cluster's creationTimestamp relative to the RestoreJob's StartedAt: only a recovery Cluster created at/after StartedAt is treated as this restore's own and skipped; one that predates StartedAt is leftover from an earlier completed restore and is purged so the restore re-bootstraps from the backup. Signed-off-by: Ivan Okhotnikov <[email protected]>
…onservative freshness tie Address review feedback on the repeat in-place restore fix: - Make the freshness discriminator strict (creationTimestamp > StartedAt) so an exact tie resolves to "not fresh" and the caller purges, matching the function's stated conservative default; a fresh Cluster is always created well after StartedAt so this never misclassifies the status-write-race path. - Document why a wall-clock timestamp comparison was chosen over recording the purged Cluster's UID on the TargetPurged condition (the UID approach leans on the same status write the race path assumes can fail). - Add a reconcile-level regression test that drives reconcileCNPGRestore end to end with a fake client: a stale leftover recovery Cluster from a prior restore is purged, and a Cluster this restore just re-created is not re-purged. This covers the actual call site where #3311 lived, not just the pure helpers. Signed-off-by: Ivan Okhotnikov <[email protected]>
IvanHunters
added
kind/bug
github-actions
Bot
added
area/uncategorized
📝 WalkthroughWalkthroughCNPG restore purge logic now distinguishes freshly recovered clusters from stale recovery clusters by comparing cluster creation time with the RestoreJob start time. Recovery state detection returns creation timestamps, reconciliation uses the freshness result, and unit and reconciliation tests cover repeat in-place restores. ChangesCNPG recovery purge handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a data-integrity bug in the CNPG backup driver where repeat in-place restore operations would silently fail to re-bootstrap the database. By refining the logic that determines whether a destructive purge of the existing cluster is required, the system can now accurately identify stale recovery clusters left over from previous operations. This ensures that subsequent restore attempts correctly clear old state and perform a fresh bootstrap from the backup, while still protecting against unnecessary purges caused by race conditions during the initial recovery phase. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CNPG backup controller strategy to fix a bug where repeat in-place restores were silent no-ops. It introduces cnpgClusterFreshlyRecovered to distinguish between a freshly-recovered CNPG Cluster produced by the current restore job (which should skip purging) and a stale leftover recovery Cluster from a previous restore (which must be purged). It also updates the helper method recoveryBootstrapClusterState to return the cluster's creation timestamp and adds comprehensive unit and integration tests to verify this behavior. The review feedback suggests two minor optimizations: comparing the underlying time.Time fields directly using .Time in cnpgClusterFreshlyRecovered, and returning a pointer to cluster.CreationTimestamp directly in recoveryBootstrapClusterState to avoid an unnecessary copy.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if !hasRecovery || clusterCreatedAt == nil || restoreStartedAt == nil { | ||
| return false | ||
| } | ||
| return clusterCreatedAt.After(restoreStartedAt.Time) |
There was a problem hiding this comment.
To be more explicit and avoid relying on implicit method promotion or pointer dereferencing, we can compare the underlying time.Time fields directly using .Time.
| return clusterCreatedAt.After(restoreStartedAt.Time) | |
| return clusterCreatedAt.Time.After(restoreStartedAt.Time) |
| created := cluster.CreationTimestamp | ||
| return true, &created, nil |
There was a problem hiding this comment.
Instead of copying the metav1.Time struct to a local variable created and then taking its address, we can directly return a pointer to the CreationTimestamp field of the heap-allocated cluster struct. This is more concise and avoids an unnecessary copy.
| created := cluster.CreationTimestamp | |
| return true, &created, nil | |
| return true, &cluster.CreationTimestamp, nil |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/backupcontroller/cnpgstrategy_controller_test.go`:
- Around line 1715-1740: Add a test case alongside “freshly-recovered cluster
from this restore is not re-purged” that recreates a recovery Cluster with the
prior restore configuration and a creation timestamp after StartedAt, without
this restore creating it. Run reconcileCNPGRestore and assert the recreated
Cluster is marked for deletion, proving timestamp alone does not establish
ownership. Reuse the existing mkRecoveryCluster, RestoreJobReconciler, and test
client setup.
In `@internal/backupcontroller/cnpgstrategy_controller.go`:
- Around line 368-372: Replace the timestamp-only logic in
cnpgClusterFreshlyRecovered with a durable restore-specific marker or persisted
pre-purge Cluster identity so recovery is confirmed by provenance, not merely
creation time; update internal/backupcontroller/cnpgstrategy_controller.go lines
368-372 accordingly. Add a test in
internal/backupcontroller/cnpgstrategy_controller_test.go lines 1715-1740
covering a Cluster recreated after StartedAt that retains prior restore settings
and is still patched and purged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 016949f3-4158-4726-a513-178cdd7924f4
📒 Files selected for processing (2)
internal/backupcontroller/cnpgstrategy_controller.gointernal/backupcontroller/cnpgstrategy_controller_test.go
| t.Run("freshly-recovered cluster from this restore is not re-purged", func(t *testing.T) { | ||
| backup := mkBackupArtifact(t) | ||
| // creationTimestamp a minute after StartedAt: this restore's own re-render. | ||
| fresh := metav1.NewTime(startedAt.Add(time.Minute)) | ||
| c := newCNPGStrategyTestClient(t, backup, mkRestoreJob(), strategy, cnpgBackup, | ||
| newPostgresApp(appName, ns), mkRecoveryCluster(fresh)) | ||
| r := &RestoreJobReconciler{Client: c, Interface: dynamicfake.NewSimpleDynamicClient(testCNPGScheme(t))} | ||
|
|
||
| rj := &backupsv1alpha1.RestoreJob{} | ||
| if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: "rj"}, rj); err != nil { | ||
| t.Fatalf("get seeded RestoreJob: %v", err) | ||
| } | ||
| if _, err := r.reconcileCNPGRestore(ctx, rj, backup); err != nil { | ||
| t.Fatalf("reconcileCNPGRestore: %v", err) | ||
| } | ||
|
|
||
| // The freshly-recovered Cluster must survive - re-purging it would | ||
| // destroy the recovery this restore just started (the status-write-race | ||
| // protection the guard was built for). | ||
| got := &cnpgtypes.Cluster{} | ||
| if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: clusterName}, got); err != nil { | ||
| t.Fatalf("expected freshly-recovered Cluster to survive, got err=%v", err) | ||
| } | ||
| if !got.DeletionTimestamp.IsZero() { | ||
| t.Fatalf("freshly-recovered Cluster must not be marked for deletion") | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Cover recreation after StartedAt that was not caused by this restore.
Add a case where a recovery Cluster using the prior restore configuration is recreated after StartedAt, before the current restore purges it. It must still be purged; otherwise the test preserves the timestamp-equals-ownership flaw.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/backupcontroller/cnpgstrategy_controller_test.go` around lines 1715
- 1740, Add a test case alongside “freshly-recovered cluster from this restore
is not re-purged” that recreates a recovery Cluster with the prior restore
configuration and a creation timestamp after StartedAt, without this restore
creating it. Run reconcileCNPGRestore and assert the recreated Cluster is marked
for deletion, proving timestamp alone does not establish ownership. Reuse the
existing mkRecoveryCluster, RestoreJobReconciler, and test client setup.
| func cnpgClusterFreshlyRecovered(hasRecovery bool, clusterCreatedAt, restoreStartedAt *metav1.Time) bool { | ||
| if !hasRecovery || clusterCreatedAt == nil || restoreStartedAt == nil { | ||
| return false | ||
| } | ||
| return clusterCreatedAt.After(restoreStartedAt.Time) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Creation time does not prove that this restore produced the Cluster.
StartedAt is persisted before purge processing, while the HelmRelease remains active. A Cluster recreated from the previous recovery configuration during that window postdates StartedAt, causing the controller to skip the current patch and purge and potentially report success with stale data.
internal/backupcontroller/cnpgstrategy_controller.go#L368-L372: use a durable restore-specific marker or persisted pre-purge Cluster identity instead of timestamp ordering alone.internal/backupcontroller/cnpgstrategy_controller_test.go#L1715-L1740: add a post-StartedAtrecreation case that retains prior restore settings and must still be purged.
📍 Affects 2 files
internal/backupcontroller/cnpgstrategy_controller.go#L368-L372(this comment)internal/backupcontroller/cnpgstrategy_controller_test.go#L1715-L1740
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/backupcontroller/cnpgstrategy_controller.go` around lines 368 - 372,
Replace the timestamp-only logic in cnpgClusterFreshlyRecovered with a durable
restore-specific marker or persisted pre-purge Cluster identity so recovery is
confirmed by provenance, not merely creation time; update
internal/backupcontroller/cnpgstrategy_controller.go lines 368-372 accordingly.
Add a test in internal/backupcontroller/cnpgstrategy_controller_test.go lines
1715-1740 covering a Cluster recreated after StartedAt that retains prior
restore settings and is still patched and purged.
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM — correctly fixes the silent repeat-in-place-restore no-op (#3311) by distinguishing a recovery Cluster left over from a prior completed restore (must purge) from one this restore just re-created (must not re-purge), with tests covering the full contract and no behavioral regression.
Business context: A repeat in-place PostgreSQL restore reported Succeeded while skipping the destructive purge, leaving the database with its pre-restore contents (#3311). This change gates the purge-skip on the live Cluster's creationTimestamp relative to the RestoreJob's StartedAt.
Review notes
- Root cause and fix check out.
StartedAtis stamped on the first reconcile, which returns early, so the purge and chart re-render always happen on later reconciles — a genuinely fresh Cluster is created well afterStartedAt, while a prior-restore leftover predates it. The reconcile-level regression exercises both directions. - No regression: the new skip condition (
hasRecovery && createdAt > StartedAt) is a strict subset of the previous one (hasRecovery). The change never skips a purge the old logic would have run; it only adds purges. The exact-second tie resolves conservatively to purge. - Tests cover the full contract — truth table (fresh / stale / exact-second tie / missing timestamp) plus a reconcile-level test asserting the stale Cluster and its labelled PVC are purged while the freshly-recovered one survives.
- Scope is internal reconcile logic only; no API, CRD, chart-values, or documentation surface changes.
Non-blocking follow-ups
- Concurrent RestoreJobs targeting the same app are not serialized (the RestoreJob controller uses a plain reconciler with no per-target exclusion). If a second restore starts before the first's replacement Cluster is rendered, the timestamp heuristic cannot prove which job produced that Cluster, so the second could skip its purge and succeed against the wrong backup. This is pre-existing — the previous
hasRecovery-only logic had the same exposure — and is not worsened here, but it is worth a separate hardening issue (e.g. record the purged Cluster's UID on theTargetPurgedcondition, or enforce target-level mutual exclusion). - The two low-priority inline suggestions already on the diff (explicit
.Time.After, returning the timestamp pointer directly) are optional — author's discretion.
|
Successfully created backport PR for |
2 participants
What this PR does
Fixes a silent data-integrity failure in the CNPG backup driver: a repeat in-place PostgreSQL
RestoreJobinto a target that was already restored once reportedSucceededbut did nothing. The PVC was untouched (same uid), no*-full-recoverypods appeared, and the database kept its pre-restore contents.Root cause: the restore patch sets
spec.bootstrap.enabled=trueon the Postgres app and never resets it, so the chart keeps renderingspec.bootstrap.recoveryon the live cnpg.io Cluster. On a secondRestoreJobthe re-purge guard (cnpgPurgeNeeded) saw that leftoverbootstrap.recoveryand treated it as "an earlier reconcile of this restore already purged", skipping the destructive Cluster + PVC purge. That guard was meant only to avoid re-purging a freshly-recovered Cluster when the post-purge status write races, but it could not tell that case apart from a Cluster left over by a previous completed restore.The fix distinguishes the two by the live Cluster's
creationTimestamprelative to theRestoreJob'sStartedAt:StartedAt: freshly recovered by this restore's own purge, so it is skipped (unchanged behaviour for the status-write-race path);StartedAt: leftover from an earlier completed restore, so it is purged and the restore re-bootstraps from the backup.Design note: an identity-based alternative (record the purged Cluster's UID on the
TargetPurgedcondition and compare UIDs on later reconciles) was considered and rejected. It is immune to clock skew but leans on the same status write that the status-write-race path assumes can fail, so it cannot cover the exact case the skip exists for. The timestamp comparison needs no extra persisted state, and the fresh-vs-stale gap (a full purge plus chart re-render cycle) always dwarfs any plausible control-plane clock skew. The comparison is strict, so an exact-second tie resolves to purge (the conservative direction).Tests
cnpgClusterFreshlyRecoveredunit test: fresh / stale / boundary / missing-timestamp cases.TestCNPGPurgeNeededextended with the repeat-restore (stale recovery cluster) case that reproduced the bug.recoveryBootstrapClusterStatetest updated to assert the returnedcreationTimestamp.reconcileCNPGRestoreend to end: a stale leftover recovery Cluster is purged, a freshly-recovered Cluster is not re-purged.Bug report: #3311
Screenshots
Not applicable (no UI changes).
Downstream repositories
The diff touches only
internal/backupcontroller(operator reconcile logic); no API types, CRDs, chart values, or docs change, so no downstream repository is affected.Release note
Summary by CodeRabbit