Repository navigation
fix(backups, postgres): refuse a restore into another PostgreSQL major - #4669
Conversation
A CNPG restore is physical: the recovered data files only open on the major version that wrote them. The driver purged the target Cluster and its PVCs without comparing the two, so a PostgreSQL 18 backup restored into a v16 application, or in place into an application upgraded since the backup, emptied the target. The recovery pods then looped on "database files are incompatible with server" until the restore deadline, and the RestoreJob failed with nothing left to fall back on. The restore now fails before the purge, naming both versions, when they differ. The target side is spec.version, since that is what the chart renders the recovered Cluster from once the live one is gone. The backup side is CNPG's status.majorVersion, copied into the artifact's driverMetadata at backup time because retention deletes the cnpg.io/Backup while the artifact stays restorable; artifacts taken before that fall back to the cnpg.io/Backup. When either side is unknown the restore goes ahead as before, so older backups keep restoring. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe CNPG controller now records positive backup PostgreSQL major versions in artifact metadata. Before purging a restore target, it compares the known backup major version with the target app version and refuses the restore when they differ. ChangesCNPG major-version restore check
Priority: ⚪ Not assessed Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant RestoreController
participant BackupArtifact
participant CNPGBackup
participant TargetPostgresApp
RestoreController->>BackupArtifact: Read backup major-version metadata
opt Metadata is absent or invalid
RestoreController->>CNPGBackup: Read status major version
end
RestoreController->>TargetPostgresApp: Read spec.version
alt Known versions differ
RestoreController->>RestoreController: Return MajorVersionMismatch before purge
else Versions match or either is unknown
RestoreController->>RestoreController: Continue restore
end
Merge Risk: ⚪ Minimal · up to Supported PostgreSQL versions are recognized by the restore check, and no actionable merge risk remains after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
There was a problem hiding this comment.
mattia-eleuteri LGTM.
The bug is real on current main. reconcileCNPGRestore patches the app and purges the Cluster and its PVCs without looking at either major version, and CNPG cannot open data files written by another major, so a cross-major restore ends with an empty target. The new check sits at the top of the purge block, before the WAL gate, the HelmRelease suspend and the patch, so the refusal path changes nothing on the target. Falling back to "unknown, go ahead" keeps older artifacts restorable, and status.majorVersion is in the vendored CNPG Backup CRD.
What I ran on a merge of the head with current main: go test ./internal/backupcontroller/... passes. Turning the backupMajor != targetMajor case off fails the three refusal subtests of TestReconcileCNPGRestore_RefusesCrossMajorRestore. Skipping the driverMetadata record fails TestCreateCNPGBackupArtifact_RecordsMajorVersion. Dropping the v prefix handling in postgresAppMajorVersion fails the refusal cases again, since the target major then reads as unknown.
Adding Version to postgresapp.PostgresSpec is safe for the restore patch: patchPostgresAppForRestore uses client.MergeFrom, and an unchanged field does not appear in the merge patch.
…mputeplane pool, fix the backupcontroller build (#4768) ## What this PR does Makes the backupcontroller test package compile again on main. #4587 added a flavor argument to `createCNPGBackupArtifact` and `marshalCNPGBackupSnapshot`, and #4669, whose CI ran before that, added two test calls with the old argument lists. Both PRs were green on their own, and together they break `go test ./internal/backupcontroller/`. The two calls now pass the same default flavor as the other tests in the file. The second commit fixes an e2e suite that fails the required E2E check here. It is the `computeplane` suite from #4713, and its worker pool was one node with a single vCPU. After the kubelet reservations that node has 900m of CPU to allocate. The tenant addons (cilium, coredns, csi, ingress-nginx, metrics-server, cert-manager) request exactly 900m between them. If cert-manager's startupapicheck hook pod is created last, it can't be scheduled and the install times out after 10m. The reinstall after remediation runs into the same full node. Whether that pod comes last depends on install order, so the suite passed on #4713 and failed here. The pool now has two workers with one vCPU each, the same shape the kubernetes suites use. One vCPU per worker stays on purpose, because of #3513. Two workers exposed a deadlock in the tenant cluster-autoscaler. The suite's pool is created at zero replicas and then gets a min size of 2, which CAPI does not apply to the live replica count on upgrade (#4786). The autoscaler's scale-up for a pending pod asks for one node, and the clusterapi provider refuses it because one is under the min size, although it is an increase. That refusal backs the group off, and the min-size enforcement skips groups in backoff, so the pool never leaves zero. Upstream had the same report in kubernetes/autoscaler#4942, closed as stale, and it is now tracked in kubernetes/autoscaler#10400. Our autoscaler image already patches the mirror case for max size, and the patch now refuses only a decrease below the min size. `go vet` and `go test ./internal/backupcontroller/...` pass, and so do the bats pins for the computeplane suite. The autoscaler patch adds a test for each direction, and the clusterapi package tests pass with it applied to 1.32.0. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved cluster autoscaling for worker groups outside configured size limits: groups below their minimum can scale up toward it, and nodes can be removed when a group exceeds its maximum. Scale-down requests below the minimum remain restricted. * **Tests** * Updated backup and restore test calls with PostgreSQL and empty flavor values. * Updated ComputePlane checks for a two-worker pool and clarified cert-manager installation timing expectations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
A CNPG restore is physical, so the recovered data files only open on the PostgreSQL major version that wrote them.
reconcileCNPGRestorepurged the target Cluster and its PVCs without comparing the two. Observed on v1.6.3: a PostgreSQL 18 backup restored into a v16 application emptied the target, thefull-recoverypods then looped onFATAL: database files are incompatible with server, and the RestoreJob failed at the 30-minute deadline with the target left empty. An in-place restore into an application whosespec.versionwas raised since the backup ends the same way.The restore now compares the two majors before the purge, ahead of the WAL-archive gate, and when they differ marks the RestoreJob
Failedwith reasonMajorVersionMismatchand a message naming both versions. Nothing is suspended, patched or deleted on that path.spec.version(v13tov18). That is what the chart renders the recovered Cluster from once the live one is gone, so the live Cluster's image would be the wrong thing to compare.cnpg.io/Backup'sstatus.majorVersion, now copied into the artifact'sdriverMetadataascnpg.io/major-versionat backup time, because barman retention deletes thecnpg.io/Backupwhile the artifact stays restorable. An artifact taken before this change falls back to thecnpg.io/Backupwhile it still exists.cnpg.io/Backupgone, a CNPG that predatesmajorVersion, an emptyspec.version), the restore goes ahead as before and the controller logs it, so existing backups keep restoring.TestReconcileCNPGRestore_RefusesCrossMajorRestoredrives the real reconcile against a fake client with a live Cluster and PVC on the target and counts everyDelete. With the check removed, the two refusal cases (18 into a v16 copy, 16 in place into an app moved to v18) and the fallback case fail: the Cluster and the PVC are deleted and the app is patched for recovery. With the check in place they endFailedwith zero deletes, while the same-major and unknown-version cases still reach the purge.TestCreateCNPGBackupArtifact_RecordsMajorVersioncovers thedriverMetadatarecord.Screenshots
Downstream repositories
Walked against the diff: it touches only
internal/backupcontroller, with no CRD, no appvalues.yamlor schema and no ApplicationDefinition change. The newcnpg.io/major-versionentry lives in the free-formBackup.spec.driverMetadatamap, which no downstream repository reads by key.Release note
Summary by CodeRabbit