mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-10 06:24:09 +09:00
56dd6c2e413db2ede4e46f76ff384776d2002e0e
11
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
f203a47afe |
fix(backups): hold a Postgres backup until the plugin rollout is over (#4674)
<!-- 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 The first BackupJob on a Postgres app whose `cnpg.io/Cluster` has never carried the barman-cloud plugin — a fresh platform-flow app, or one an in-place restore re-rendered — attaches the plugin and creates the `cnpg.io/Backup` in the same reconcile. Attaching the plugin changes the instance pod spec, so CNPG rolls every instance (`Pod rollout required: volumes: element plugins has been added`). The Backup then lands on an instance without the plugin sidecar (`requested plugin is not available: barman-cloud.cloudnative-pg.io`) or on one CNPG restarts under it (`instance manager was restarted during backup`). CNPG does not retry a failed Backup, so the BackupJob stays Running in `CNPGBackupTransientFailure` until `cnpgDefaultBackupDeadline` and fails. The first backup of each such Postgres is always lost; this was observed on a v1.6.3 cluster and reproduced against `main`. The driver now holds the Backup back until that rollout is over. Before attaching the plugin to a Cluster that has none, it records `PluginRolledOut=False` on the BackupJob, with `Ready=False` reason `WaitingForPluginRollout` for the user, and requeues; the next pass attaches the plugin. The Backup is created once CNPG reports the Cluster in its healthy phase and every instance pod (`cnpg.io/cluster`, `cnpg.io/podRole=instance`) was created after the condition and is Ready; the condition then turns True and `Ready` is cleared, as before. Recording the condition first means a lost status write leaves the plugin unattached for the next pass instead of a Cluster that looks ready while it rolls. A Cluster that already runs the plugin is not rolled by the driver and its Backup is created on the first pass, as today. The wait is bounded by the existing `cnpgDefaultBackupDeadline`, after which the BackupJob fails with the phase or pod CNPG is stuck on. Why this signal, checked against the CNPG 1.29 source and the 1.30 Cluster CRD vendored here: the Cluster status has no `observedGeneration` and its `Ready` condition is derived from `status.phase`, so right after the attach the phase still reads healthy from before until CNPG reconciles the new spec. `status.pluginStatus` does not help either, since CNPG also lists plugins referenced only from `externalClusters` there, which is exactly the state of a Cluster a restore re-rendered. A pod spec change is never applied in place (only a pending Postgres restart is), so the instances are recreated and pods newer than the attach are the ones running the sidecar. This reads pod metadata and CNPG's documented labels only, not the plugin's container or volume names, and needs no RBAC change: the controller already lists and watches pods. Tests: `TestReconcileCNPG_WaitsForPluginRolloutBeforeBackup` drives a Cluster without the plugin through the attach, a stale healthy phase over the old pod, a rolling phase over the recreated pod, and the healthy phase, and asserts no Backup exists until the last step. `TestReconcileCNPG_AttachedPluginDoesNotDelayBackup` covers a Cluster that already runs the plugin, and `TestReconcileCNPG_PluginRolloutWaitIsBounded` the deadline. With the gate disabled, the first and third fail; with only the phase checked and not the pods, the first fails at the attach pass. ### 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. --> Not applicable: 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. --> The diff touches only `internal/backupcontroller` Go code: no package, values, chart, CRD, `hack/` tooling, telemetry metric or contributor convention. Walked against the trigger map, it reaches none of the repositories below, and the website documents no workaround for the lost first backup that this would make obsolete. - [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 <!-- 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): the CNPG backup driver no longer loses the first backup of a Postgres app whose Cluster had no barman-cloud plugin yet (a new platform-flow app, or one restored in place). It now waits for CNPG to finish rolling the instances onto the plugin before creating the cnpg.io Backup, and reports the wait on the BackupJob as Ready=False with reason WaitingForPluginRollout. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability** * Backups wait for the backup plugin rollout to complete and cluster instances to become healthy before starting. * If rollout does not finish before the backup deadline, the backup job fails instead of waiting indefinitely. * Backups can start without waiting for a new rollout when the plugin is already attached. * **Backup Configuration** * When no server name is configured, backups use the cluster name and application UID, giving recreated applications distinct archive prefixes. * Existing server names are preserved when the plugin is already attached. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Mattia Eleuteri
|
54ab863c0e |
fix(backups): hold a Postgres backup until the plugin rollout is over
A Cluster that has never carried the barman-cloud plugin gets it from
the first BackupJob: a fresh platform-flow app, or one an in-place
restore re-rendered. Attaching it changes the instance pod spec, so
CNPG rolls every instance. The cnpg.io/Backup the same reconcile
created then runs on an instance without the plugin sidecar ("requested
plugin is not available") or on one CNPG restarts under it ("instance
manager was restarted during backup"). CNPG never retries it, so the
BackupJob sat in CNPGBackupTransientFailure until the backup deadline
and failed: the first backup of every such Postgres was lost.
The driver now records a PluginRolledOut=False condition before it
attaches the plugin, and creates no Backup until CNPG reports the
Cluster healthy and every instance pod was created after that
condition and is Ready. Recording it first means a lost status write
leaves the plugin unattached for the next pass, rather than a Cluster
that looks ready while it rolls. A Cluster that already runs the plugin
is not rolled by the driver and is not held back.
The phase alone cannot tell the rollout is over. Cluster status has no
observedGeneration and its Ready condition is derived from the phase,
so until CNPG reconciles the new spec the phase still reads healthy
from before the attach. status.pluginStatus is no better: CNPG also
lists plugins referenced only from externalClusters there, which is
exactly the state of a Cluster a restore re-rendered. A pod spec change
is never applied in place, so the instances are recreated, and pods
newer than the attach are the ones running the sidecar. That reads only
pod metadata and CNPG's documented labels, not the plugin's container
names. The wait stays bounded by the existing backup deadline.
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
Assisted-by: LLM
|
||
|
Aleksei Sviridkin
|
3fc45ed183 |
fix(backups, postgres): refuse a restore into another PostgreSQL major (#4669)
<!-- 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 --> |
||
|
Mattia Eleuteri
|
cfa31d3a17 |
fix(postgres): record the live cluster's flavor on a backup
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> |
||
|
Mattia Eleuteri
|
f514f8722b |
fix(backups, postgres): refuse a restore into another PostgreSQL major
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> |
||
|
Mattia Eleuteri
|
497dd6fc4f |
fix(backups): start a Postgres restore from the backup it names
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> |
||
|
Aleksei Sviridkin
|
7a0aab2bdc |
refactor: apply the Go 1.27 modernizers
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>
|
||
|
Yan Bondarenko
|
9214430bd8 |
fix(backups): give the barman-cloud sidecar its own resources
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> |
||
|
Andrey KolkovandClaude Opus 4.8
|
b574e2176f |
fix(backups): cover Go-driven system-bucket ObjectStore + share the checksum helper
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> |
||
|
Andrey Kolkov
|
13ff340277 |
fix(backups): migrate CNPG backups to the barman-cloud plugin
Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
ed5ae16e36 |
feat(platform): implement backup strategy for postgres application
Signed-off-by: Andrey Kolkov <androndo@gmail.com> |