mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-26 21:17:32 +09:00
Aleksei Sviridkin
1d813c9445
## What this PR does Fixes #3322. An in-place Postgres (CNPG) restore decided whether to skip deleting the live `Cluster` by comparing its `creationTimestamp` with the RestoreJob's `StartedAt`. The timestamp says the Cluster is new, not which restore rendered it, so two cases let a restore skip its purge and report `Succeeded` on data from another restore: 1. Two RestoreJobs into the same application at once. The second sees the Cluster the first produced, created after its own `StartedAt`, and skips its purge. 2. The live Cluster is deleted while the restore waits for the WAL archive. The still-active HelmRelease re-renders it from the previous restore's values, again created after `StartedAt`. ### Refuse a restore into an application another restore holds The RestoreJob controller now refuses, for every driver, a RestoreJob that has not started yet when another unfinished RestoreJob in the namespace writes into the same application and goes first. The refused job fails with reason `ConcurrentRestore`, and its message names the job that goes first. It fails instead of waiting, so the application is never restored a second time later, after the first restore, by a job its author may no longer expect to run. A started restore goes before one that has not started. Between two that have not, the older goes first, by creation time and then by name; creation times have one-second precision, so two jobs created in the same second are ordered by name. A running restore is never failed by a newer one. The target application is resolved by `restoreJobTarget`, the same helper the BackupJob hold uses, so a Velero restore into another namespace does not collide with an in-place one. The gate lists RestoreJobs through the manager's API reader, not the informer cache. A job reconciled before the cache holds its peer's `StartedAt` would otherwise see that peer as not started and, on a same-second name tie, proceed alongside it. The controller runs a single worker, so a peer's `StartedAt` written by an earlier reconcile is already on the API server when the next job reads it; `SetupWithManager` now sets `MaxConcurrentReconciles: 1` explicitly, since the guarantee depends on it. ### Skip the CNPG purge only for this restore's Cluster The restore patch already sets `bootstrap.newServerName` to `restoredServerName(cluster, RestoreJob UID)`, a value unique to each RestoreJob. The postgres chart now annotates the `Cluster` with it as `postgres.cozystack.io/restored-server-name`. The annotation is rendered even when the barman plugin is not; the plugin's `serverName` also carries the value, but the plugin renders only with chart-level S3 coordinates. The driver skips the purge only when the annotation names this RestoreJob. A Cluster produced by a concurrent restore, or re-rendered from an earlier restore's values, carries another value and is purged. The mark lives on the rendered Cluster rather than in RestoreJob status. That keeps it valid in the case the skip exists for: the purge succeeded, but the `TargetPurged` status write failed. A UID recorded in status was rejected in the code for exactly that reason. A Cluster rendered by a chart that predates the annotation has none and keeps the creation-time check, so a restore in flight across an upgrade behaves as before. ### Tests - `TestRestoreAhead` covers order, a started job going first, the name tie-break, finished and terminating jobs, other applications, retargeted restores and a Velero restore into another namespace. - `TestReconcileRestore_ConcurrentRestoreIsRefused` checks that the newer job fails with `ConcurrentRestore` and the running one is left alone. - `TestReconcileRestore_ConcurrentRestoreReadsPastTheCache` gives the cache a stale, not-started peer and the API reader the started one, and requires the job to fail with `ConcurrentRestore`. - `TestCNPGClusterFreshlyRecovered` adds both cases from the issue: a Cluster marked by another restore and created after `StartedAt` must be purged. The timestamp-only cases are kept for the unmarked fallback. - `TestReconcileCNPGRestore_RepeatInPlacePurgesStaleRecoveryCluster` gains two reconcile-level cases. One purges a Cluster that another restore marked, even though it was created after `StartedAt`. The other runs a full purge, takes the mark from the `bootstrap.newServerName` the restore patch wrote, drops `TargetPurged` as a lost status write would, and requires a Cluster carrying that mark to survive, although it predates `StartedAt`. That pins the purge decision to the value the chart actually renders. - `packages/apps/postgres/tests/restore_mark_test.yaml` checks that the annotation is rendered with and without the barman plugin, and is absent on a Cluster that was never restored. Each new test was checked against the code it guards: disabling the gate call, listing through the cache, flipping the started-job precedence, ignoring the annotation, or deriving the expected mark from the app name instead of the cluster name each turns one red. `go test ./internal/backupcontroller/...` and `helm unittest packages/apps/postgres` (152 tests) pass. The restore flow was not run against a live cluster. `docs/operations/backup-classes.md` documents the one-restore-at-a-time rule and the annotation. ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff: Go changes in `internal/backupcontroller` (no CRD or API type change), one annotation added to the postgres chart's Cluster (no `values.yaml` or `values.schema.json` change), and a docs paragraph. No package is added or renamed, no platform values, ApplicationDefinition, RestoreJob CRD, metric, label or namespace changes, so no trigger is reached. - [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 fix(backups): a RestoreJob now fails with reason ConcurrentRestore when another unfinished RestoreJob is already restoring the same application, instead of running alongside it; create it again once the first restore has finished. An in-place Postgres restore now skips deleting the live Cluster only when that Cluster was rendered for this RestoreJob, identified by the new postgres.cozystack.io/restored-server-name annotation, so it no longer reports Succeeded on data from a concurrent or earlier restore. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Restores targeting the same application are now handled one at a time. A competing restore is refused rather than queued, with started restores taking priority. * PostgreSQL now calculates a default `shared_buffers` value when the memory limit is below 512 MiB, while respecting explicitly configured values. * **Bug Fixes** * Recovery clusters are more reliably distinguished from manual recoveries and clusters created by other restores, reducing the risk of purging the wrong cluster. * **Documentation** * Clarified restore ordering and how recovery-cluster cleanup is determined. <!-- end of auto-generated comment: release notes by coderabbit.ai -->