Repository navigation
fix(backups/postgres): purge stale recovery cluster on repeat in-place restore - #3312
IvanHunters wants to merge 2 commits into
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]>
📝 WalkthroughWalkthroughThe CNPG restore controller now uses recovery-bootstrap state and Cluster creation time to distinguish freshly recovered Clusters from stale leftovers before purging. Unit and reconcile tests cover timestamp decisions, terminating or missing Clusters, and repeat in-place restore scenarios. ChangesCNPG restore purge behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: 🚥 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 |
github-actions
Bot
added
area/uncategorized
…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 cozystack#3311 lived, not just the pure helpers. Signed-off-by: Ivan Okhotnikov <[email protected]>
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 resolves a data-integrity issue in the CNPG backup driver where repeat in-place restore operations failed to trigger the necessary purge of existing resources. By improving the logic that determines whether a recovery cluster should be purged, the system now correctly differentiates between leftover clusters from previous operations and clusters currently being managed by the active restore job. This ensures that repeat restores properly re-bootstrap from the backup instead of erroneously skipping the destructive purge 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
|
dosubot
Bot
added
the
area/database
![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 refactors the CNPG restore logic to prevent skipping destructive purges on stale recovery clusters left over from prior restores. It introduces a timestamp comparison (cnpgClusterFreshlyRecovered) to distinguish between a freshly-recovered cluster and a leftover one, fixing a bug where repeat in-place restores would silently do nothing. Unit and integration tests have been updated and added to cover these scenarios. The review feedback suggests a minor code optimization to return the address of cluster.CreationTimestamp directly instead of copying it to a local variable first.
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.
| created := cluster.CreationTimestamp | ||
| return true, &created, nil |
There was a problem hiding this comment.
You can return the address of cluster.CreationTimestamp directly instead of copying it to a local variable created first. Since cluster is already a pointer, its fields are addressable and safe to return as pointers.
| 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: 1
🤖 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.go`:
- Around line 337-372: The timestamp-only check in cnpgClusterFreshlyRecovered
is insufficient to establish RestoreJob provenance; require a per-run identity
marker alongside recovery state and creation-time validation. Update
internal/backupcontroller/cnpgstrategy_controller.go lines 337-372 to accept and
validate the marker, and lines 663-674 to pass it before bypassing purge. Update
internal/backupcontroller/cnpgstrategy_controller_test.go lines 1715-1720 to
cover a stale recovery Cluster recreated after StartedAt and a genuinely fresh
Cluster carrying the matching marker.
🪄 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: 0b76c582-8896-4a97-b06d-e25bf304e95e
📒 Files selected for processing (2)
internal/backupcontroller/cnpgstrategy_controller.gointernal/backupcontroller/cnpgstrategy_controller_test.go
| // cnpgClusterFreshlyRecovered reports whether a live cnpg.io Cluster that | ||
| // carries spec.bootstrap.recovery was produced by THIS RestoreJob's own purge | ||
| // + chart re-render (its creationTimestamp is strictly after the job's | ||
| // StartedAt) rather than being left over from an earlier, already-completed | ||
| // restore. | ||
| // | ||
| // Only the former is safe to skip re-purging. A leftover recovery Cluster from | ||
| // a prior restore predates StartedAt and still holds the prior data, so it must | ||
| // be purged for the new restore to re-bootstrap from the backup. The fresh | ||
| // Cluster in the status-write-race case is always created after StartedAt (the | ||
| // job sets StartedAt on its first reconcile, long before it purges and the | ||
| // chart re-renders), so the timestamp comparison cleanly separates the two. | ||
| // | ||
| // The comparison is strict (created > started), so an exact tie resolves to | ||
| // "not fresh" and the caller purges. That matches the conservative default | ||
| // below: the only classification that must never be wrong is calling a stale | ||
| // leftover "fresh" (which reintroduces the silent no-op), and a fresh Cluster | ||
| // is always created well after StartedAt (see above), never exactly at it. | ||
| // | ||
| // An identity-based alternative was considered - recording the purged Cluster's | ||
| // UID on the TargetPurged condition and comparing UIDs on later reconciles - | ||
| // which is immune to clock skew. It was rejected because it leans on the same | ||
| // status write that the status-write-race path (the whole reason this skip | ||
| // exists) assumes can fail, so it cannot cover that case; the timestamp | ||
| // comparison needs no extra persisted state and the fresh-vs-stale gap | ||
| // (a full purge + re-render cycle) always dwarfs any plausible control-plane | ||
| // clock skew. | ||
| // | ||
| // Returns false when freshness cannot be determined (no recovery bootstrap, or | ||
| // a missing timestamp): the caller then proceeds to purge, which is the safe | ||
| // default for any pre-existing, non-fresh Cluster. | ||
| 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 RestoreJob provenance.
A stale recovery configuration can be re-rendered after StartedAt, causing the controller to skip purge and potentially report success against old data.
internal/backupcontroller/cnpgstrategy_controller.go#L337-L372: require a per-run identity marker in addition to timestamp/bootstrap state.internal/backupcontroller/cnpgstrategy_controller.go#L663-L674: pass and validate that marker before bypassing purge.internal/backupcontroller/cnpgstrategy_controller_test.go#L1715-L1720: test a stale recovery Cluster recreated afterStartedAt, plus a genuinely fresh Cluster carrying the matching marker.
📍 Affects 2 files
internal/backupcontroller/cnpgstrategy_controller.go#L337-L372(this comment)internal/backupcontroller/cnpgstrategy_controller.go#L663-L674internal/backupcontroller/cnpgstrategy_controller_test.go#L1715-L1720
🤖 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 337 - 372,
The timestamp-only check in cnpgClusterFreshlyRecovered is insufficient to
establish RestoreJob provenance; require a per-run identity marker alongside
recovery state and creation-time validation. Update
internal/backupcontroller/cnpgstrategy_controller.go lines 337-372 to accept and
validate the marker, and lines 663-674 to pass it before bypassing purge. Update
internal/backupcontroller/cnpgstrategy_controller_test.go lines 1715-1720 to
cover a stale recovery Cluster recreated after StartedAt and a genuinely fresh
Cluster carrying the matching marker.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — correct, minimal, and well-tested fix for the silent repeat-restore no-op; the freshness discriminator preserves the status-write-race skip while purging a stale leftover recovery Cluster.
Business context: A repeat in-place PostgreSQL restore reported Succeeded but did nothing, because the guard that skips re-purging a freshly-recovered Cluster (bootstrap.recovery present) could not tell that fresh Cluster apart from one left over by an earlier completed restore (#3311).
The root cause is correctly identified — bootstrap.enabled is never reset, so the live Cluster keeps spec.bootstrap.recovery and the old "has recovery" signal is ambiguous — and the fix resolves it in the right layer: cnpgClusterFreshlyRecovered compares the live Cluster's creationTimestamp to the RestoreJob's StartedAt. I verified the invariant the fix rests on: StartedAt is write-once (if StartedAt == nil, with an explicit refetch to prevent sliding it forward) and is set on a reconcile that returns before the purge block, so a genuinely fresh Cluster is always created well after StartedAt, while a stale leftover predates it. The conservative tie (created > started, exact-second tie → purge) is the safe direction: re-purging a fresh Cluster re-recovers from the same backup, whereas skipping a stale one is the silent data-integrity failure. The design note rejecting the UID-marker alternative is sound — it would lean on the same status write the race path assumes can fail.
Test coverage is exemplary: cnpgClusterFreshlyRecovered is pinned across fresh / stale / exact-tie / missing-cluster-timestamp / missing-start-timestamp / no-recovery, TestCNPGPurgeNeeded gains the repeat-restore row, and TestReconcileCNPGRestore_RepeatInPlacePurgesStaleRecoveryCluster drives the call site end to end. The renamed recoveryBootstrapClusterState has no dangling callers, and the red Build packages/* checks are fork-registry push denials (denied: Anonymous users…), unrelated to the diff — the relevant Unit & controller tests check is green.
Non-blocking follow-ups
- The timestamp discriminator cannot prove RestoreJob provenance if a stale recovery Cluster is externally deleted and re-created after
StartedAt(a bot flagged this). In normal operation this cannot happen — a repeat restore does not change the HelmRelease values (bootstrap.enabledis already true), so the stale Cluster is updated in place and keeps its oldcreationTimestamp; recreation only follows this restore's own purge. Worth keeping in mind as the one residual gap the accepted design trades away; a UID marker on the non-race path would be defense-in-depth, not a requirement. - Consider adding
Closes #3311so the bug auto-closes on merge (the body references it but does not link it).
|
Superseded by #, recreated from an origin branch instead of a fork. |
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
Bug Fixes
Tests