mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-25 12:37:06 +09:00
Aleksei Sviridkin
f203a47afe
<!-- 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 -->