<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `kind/backport` if it's a bugfix that needs to be
backported to a previous version.
-->
## 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
<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->
### Downstream repositories
<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.
Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.
IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:
1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->
- [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:
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
<!-- Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->
```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.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## 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.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
The chart refuses a flavor change on an existing Cluster, but the
Postgres app keeps the refused value. A BackupJob taken after such a
change recorded the app's flavor, not the one the data directory was
written by, so a RestoreJob would accept a target of the wrong image
family and refuse the right one.
The driver already reads the live Cluster to keep its barman
serverName; it now takes the flavor from that Cluster's imageName the
same way the chart's flavor guard compares images.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
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 <mattia@hidora.io>
Without recoveryTarget.backupID the barman-cloud plugin chooses the
base backup itself: the newest one in the catalog when no recoveryTime
is set, and the newest one ending by recoveryTime otherwise, on any
timeline since no targetTLI is passed. A RestoreJob naming an older
Backup therefore started from another one, and on an archive holding
sibling timelines - left by in-place restores that archived back under
the source serverName - from another branch, bringing back that
branch's data.
The restore reads barman's backupId and the backup's stoppedAt from
the live cnpg.io/Backup behind the Backup, and passes the id through
the chart as bootstrap.recovery.recoveryTarget.backupID. The plugin
gives backupID precedence over time-based selection, and CNPG does not
count it among the mutually exclusive targets, so it combines with
targetTime.
Two cases keep the plugin's choice. A recoveryTime earlier than one
second past stoppedAt: the named backup cannot serve an instant before
it is consistent, and stoppedAt is truncated to the second while
barman compares its end time to the microsecond. And a cnpg.io/Backup
that is gone: plugin-barman-cloud's retention deletes it together with
the base backup it describes, and the driver deletes one only with its
artifact, so pinning its id would purge the target and then fail the
recovery on every retry. That restore records a BaseBackupGone warning
event instead.
A re-restore into the same target clears a backupID left by a previous
one.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
Run go fix over every first-party module: interface{} becomes any,
hand-written loops become slices, maps, min/max and range-over-int,
errors.As becomes errors.AsType, pointer helpers become new(expr) and
are inlined at their call sites, and so on. Every rewrite keeps
behaviour.
Three rewrites stay out. embedlit flattens embedded-field literals
such as ObjectMeta, syntax only Go 1.27 accepts, which would break
backports to release branches still on Go 1.26. omitzero drops
omitempty from struct fields, and openapi-gen reads omitempty as
optional, so metadata, spec and status would turn required in the
published schemas. And errorsastype rewrote an errors.As whose
target, apierrors.APIStatus, does not implement error, which
errors.AsType rejects at compile time.
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The barman-cloud plugin injects its sidecar without resource requests or
limits. Tenant namespaces ship a LimitRange that defaults containers to
128Mi, so the sidecar inherits that default and is OOMKilled mid-backup:
the ObjectStore stays healthy, the Cluster stays Ready, and only the
backup fails. A cgroup high-water mark of 254 MiB on a 38 MB database
shows the default cannot hold it.
The shared cozy-lib helper and the Go path that builds the platform's own
ObjectStore now request 100m/256Mi and cap memory at 1Gi. No CPU limit is
set, matching the entityOperator precedent in the kafka chart: a
throttled sidecar stalls WAL archiving instead of failing it.
The helper is renamed from checksumSidecarConfiguration to
sidecarConfiguration, since it no longer carries only the checksum pin.
Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com>
Address review on the S3 request-checksum fix:
- Extend the fix to the platform-managed useSystemBucket=true flow, whose
ObjectStore is built in Go by applyClusterPluginBackup (SSA-applied), not
chart-rendered. Add InstanceSidecarConfiguration to cnpgtypes.ObjectStoreSpec
and set AWS_REQUEST_CHECKSUM_CALCULATION=when_required on it via
barmanSidecarConfiguration(). The platform's own default system bucket is
SeaweedFS, a non-AWS S3 gateway of the same class as Ceph RGW. Unit test in
cnpgstrategy_controller_test.go asserts the applied ObjectStore carries it.
- Factor the duplicated instanceSidecarConfiguration block (3x across two
charts) into a shared cozy-lib helper (cozy-lib.barman.checksumSidecarConfiguration).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io>
Aleksei Sviridkin