Skip to content

fix(backups, postgres): refuse a restore into another PostgreSQL major - #4669

Merged
Aleksei Sviridkin (lexfrei) merged 1 commit into
cozystack:mainfrom
mattia-eleuteri:fix/backups-refuse-cross-major-postgres-restore
Oct 6, 2026
Merged

Aleksei Sviridkin (lexfrei) merged 1 commit into
cozystack:mainfrom
mattia-eleuteri:fix/backups-refuse-cross-major-postgres-restore

Conversation

@mattia-eleuteri

@mattia-eleuteri mattia-eleuteri commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

What this PR does

A CNPG restore is physical, so the recovered data files only open on the PostgreSQL major version that wrote them. reconcileCNPGRestore purged 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, the full-recovery pods then looped on FATAL: 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 whose spec.version was 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 Failed with reason MajorVersionMismatch and a message naming both versions. Nothing is suspended, patched or deleted on that path.

  • Target major: the Postgres application's spec.version (v13 to v18). 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.
  • Backup major: the cnpg.io/Backup's status.majorVersion, now copied into the artifact's driverMetadata as cnpg.io/major-version at backup time, because barman retention deletes the cnpg.io/Backup while the artifact stays restorable. An artifact taken before this change falls back to the cnpg.io/Backup while it still exists.
  • When either side is unknown (no record and the cnpg.io/Backup gone, a CNPG that predates majorVersion, an empty spec.version), the restore goes ahead as before and the controller logs it, so existing backups keep restoring.

TestReconcileCNPGRestore_RefusesCrossMajorRestore drives the real reconcile against a fake client with a live Cluster and PVC on the target and counts every Delete. 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 end Failed with zero deletes, while the same-major and unknown-version cases still reach the purge. TestCreateCNPGBackupArtifact_RecordsMajorVersion covers the driverMetadata record.

Screenshots

Downstream repositories

Walked against the diff: it touches only internal/backupcontroller, with no CRD, no app values.yaml or schema and no ApplicationDefinition change. The new cnpg.io/major-version entry lives in the free-form Backup.spec.driverMetadata map, which no downstream repository reads by key.

Release note

fix(backups, postgres): a Postgres restore whose backup was taken on another PostgreSQL major version than the target's spec.version now fails before touching the target, instead of purging it and then failing to recover.

Summary by CodeRabbit

  • Bug Fixes
    • Cross-major PostgreSQL restores are now refused before the target is changed. The error identifies the backup and target versions.
    • Restores proceed when either version is unavailable or cannot be parsed.
  • New Features
    • Backup metadata now records the PostgreSQL major version when it is available.

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]>
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: cozystack/cozystack/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5b8e7445-2701-4687-93af-3a0f31288479
📥 Commits

Reviewing files that changed from the base of the PR and between bc400df and f514f87.

📒 Files selected for processing (4)
  • internal/backupcontroller/cnpgstrategy_controller.go
  • internal/backupcontroller/cnpgstrategy_controller_test.go
  • internal/backupcontroller/cnpgtypes/types.go
  • internal/backupcontroller/postgresapp/types.go

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

CNPG major-version restore check

Layer / File(s) Summary
Record backup major version
internal/backupcontroller/cnpgtypes/types.go, internal/backupcontroller/cnpgstrategy_controller.go, internal/backupcontroller/cnpgstrategy_controller_test.go
Backup status exposes MajorVersion. Artifacts store positive values as decimal metadata, and tests cover recording and omission.
Check versions before restore
internal/backupcontroller/postgresapp/types.go, internal/backupcontroller/cnpgstrategy_controller.go, internal/backupcontroller/cnpgstrategy_controller_test.go
Restore reconciliation reads the backup version from artifact metadata or the CNPG Backup, then compares it with the target app version before purging. Tests cover mismatches, matching versions, and unknown or unparseable versions.

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
Loading

Merge Risk: ⚪ Minimal · up to f514f

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: refusing restores when the target PostgreSQL major version differs from the backup.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added area/database Issues or PRs related to managed databases (postgres, mariadb, redis, etcd, kafka, clickhouse) area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/bug Categorizes issue or PR as related to a bug size/L This PR changes 100-499 lines, ignoring generated files labels Oct 1, 2026
@mattia-eleuteri
mattia-eleuteri marked this pull request as ready for review October 6, 2026 10:02

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lexfrei
Aleksei Sviridkin (lexfrei) merged commit 3fc45ed into cozystack:main Oct 6, 2026
56 checks passed
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Oct 7, 2026
…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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/database Issues or PRs related to managed databases (postgres, mariadb, redis, etcd, kafka, clickhouse) area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/bug Categorizes issue or PR as related to a bug size/L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants