mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 18:47:04 +09:00
Aleksei Sviridkin
717d091970
## What this PR does A Postgres `RestoreJob` now starts from the base backup its `Backup` refers to, and the PITR documentation describes the window the recovery actually has. **Starting from the named backup.** Without `recoveryTarget.backupID` the barman-cloud plugin picks the base backup itself (`internal/cnpgi/restore/restore.go` in plugin-barman-cloud, then `FindBackupInfo` in barman-cloud `pkg/catalog`): the newest backup of the catalog when no `recoveryTime` is set, otherwise the newest one whose end is not after `recoveryTime`, on any timeline since no `targetTLI` is passed. A `RestoreJob` naming an older `Backup` therefore started from another one, and on an archive that holds sibling timelines — left by in-place restores that archived back under the source `serverName`, before restored clusters got a fresh `newServerName` — it could start from another branch and bring back that branch's data. `FindBackupInfo` gives `backupID` precedence over time-based selection, and CNPG's webhook does not count it among the mutually exclusive targets, so it combines with `targetTime`. - on restore the CNPG driver reads barman's `backupId` and the backup's `stoppedAt` from the `Backup`'s `cnpg.io/Backup` and sets the new chart value `bootstrap.backupID`, rendered as `bootstrap.recovery.recoveryTarget.backupID`; - it does not pin for 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 the catalog compares barman's end time to the microsecond. Those requests keep the plugin's time-based choice, as today; - it does not pin either when the `cnpg.io/Backup` is gone. plugin-barman-cloud's retention deletes it together with the base backup it describes (`deleteBackupsNotInCatalog`), and the driver deletes one only with its artifact, so a missing one means the catalog has lost that base backup: pinning it would purge the target and then fail the recovery. The plugin's choice recovers to the same end of the archive, and the `RestoreJob` gets a `BaseBackupGone` warning event. A base backup deleted from the object store by other means is still pinned for as long as its `cnpg.io/Backup` stays, and that restore fails after the purge; plugin-barman-cloud deletes such a `cnpg.io/Backup` on its next maintenance pass (`deleteBackupsNotInCatalog`, every `retentionPolicyIntervalSeconds`, retention policy or not), but only from the primary of the cluster that took it, under the same UID. Telling it apart earlier would need reading the barman catalog, which the driver does not do; the documentation says so, and that deleting the `cnpg.io/Backup` by hand is the way out; - a re-restore into the same target clears a `backupID` left by a previous one. **Documentation** (`docs/operations/backup-classes.md`, PostgreSQL PITR): - the upper bound of the window is the last transaction (commit or abort) in the archived WAL, not the newest segment: recovery ends just before the first transaction after the target, and a segment switch or a base backup is not one. On a database at rest every instant after its last write fails with `recovery ended before configured recovery target was reached`, including "at/before the most recent completed backup", which the page recommended as the safe choice; the state as of the last write is obtained by omitting `recoveryTime`; - aiming at a backup's `stoppedAt` misses that backup (the plugin keeps only a backup whose end is not after the target), falling back to an older one or to `no target backup found`; the page now says to aim one second past it; - when a base backup removed from the bucket outside retention is still pinned, and how to unpin it; - how to read the upper bound from the archive, what pinning the base backup changes, and a section on archives with several timelines: PostgreSQL follows `recovery_target_timeline: latest` from the backup's timeline through the consecutively numbered history files it can find, so a missing `.history` confines the replay to the backup's own timeline and the bound is the last transaction on that timeline, while a targeted timeline that does not descend from the backup stops recovery with `requested timeline N is not a child of this server's history`. **Tests.** `TestCNPGRestoreBackupID` (the pinning rule), `TestReconcileCNPGRestore_PinsTheRequestedBackup` (drives a restore through the purge and reads back the patched `Postgres` app, including a `cnpg.io/Backup` removed by retention, with and without `recoveryTime`), `TestBuildPostgresAppRestorePatch_ReplacesBackupID`, a deep-copy test for the new `stoppedAt` field, and three `helm unittest` cases for `recoveryTarget`. With the rule disabled, 4 of the rule cases and 3 of the reconcile cases fail; with the template change removed, 2 chart cases fail; the retention case fails against the first version of this PR, which pinned an id recorded on the artifact; the deep-copy test fails once `StoppedAt` is dropped from the generated `DeepCopyInto`. `go test ./internal/backupcontroller/...` and `helm unittest packages/apps/postgres` (139/139, after rebasing on main) pass. Not run: the e2e suite (`hack/e2e-chainsaw`), and a restore against a live cluster with this driver. ### Screenshots No UI change. ### 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. --> - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: https://github.com/cozystack/website/issues/716. `content/en/docs/next/operations/services/backup-classes.md` (and `v1.6/`) carries a copy of the corrected "The recoverable window" and "Discovering the earliest / latest restorable time" sections; the window and `stoppedAt` corrections apply to released versions too, the `backupID` part only once this is released. - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: none expected. The diff adds `bootstrap.backupID` to the postgres `values.schema.json`, but the provider does not manage the `bootstrap` block (`internal/provider/postgresql_schema.go`: "bootstrap blocks use server defaults"), and the `RestoreJob` CRD is unchanged. Left unticked for a maintainer to confirm. - [ ] [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 <!-- 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): a Postgres RestoreJob now starts from the base backup its Backup refers to (recoveryTarget.backupID) instead of whichever backup the barman-cloud plugin picks, which on an archive with several timelines could restore another branch's data; a recoveryTime before that backup's end keeps the previous time-based choice. The PITR documentation now bounds the recoverable window by the last archived transaction rather than the newest WAL segment. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Restores can target a specific base backup and optionally replay to a requested recovery time. If no backup is specified, the newest eligible backup is selected automatically. * **Bug Fixes** * Restore handling warns when the referenced base backup is unavailable and allows the plugin to select a backup from the catalog. * **Documentation** * Clarified recovery-window boundaries, timestamp precision, idle databases, timeline branches, and behavior when a requested target cannot be reached or predates the earliest base backup. <!-- end of auto-generated comment: release notes by coderabbit.ai -->