mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 22:13:18 +09:00
4e5b76f2820cf805ced8ced4010fe1883ccd7d70
183
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Andrey Kolkov
|
4e5b76f282 |
test(backups): pin the Redis retry check
Cover the two retry-check paths the Redis driver shares with MariaDB and CNPG: a failed existence check creates the Backup without probing the bucket, and an existing Backup that cannot be read back is returned as errBackupNotReadBack so the reconcile is retried. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
db545c6e8e |
fix(backups): never skip TLS verification in place of a vanished CA
The Redis probe reads the CA Secret when it runs, not when the backup Job did. If the pod mounted a CA Secret that has since been deleted and the strategy also opts into S3_INSECURE_SKIP_VERIFY, the probe skipped verification against an endpoint the Job may have verified. Refuse to probe in that case. Without skip-verify a missing CA Secret still falls back to the system roots, as the script does. The endpoint comment claimed a bare host; it can carry a path, which the shared client now refuses. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
1d80a543dc |
fix(backups): probe the Redis snapshot with the trust its Job used
The default Redis strategy script verifies the S3 endpoint against ca.crt, then tls.crt, from the Secret mounted as the s3-ca volume (backupStorage.endpointCASecretName), and only without either honours S3_INSECURE_SKIP_VERIFY=true. The probe always verified against the system roots, so on a cluster with a private CA, such as a self-signed SeaweedFS, every Redis and Valkey backup succeeded with no artifact. Read the same settings from the pod spec of the completed backup Job, in the order the script applies them, and pass them to the S3 connection. The shared transport gains an InsecureSkipVerify switch that is set only from that explicit opt-in. The projector always stores the endpoint as a bare host, so the branch that parsed a scheme out of it was unreachable; drop it and its test. Cover the endpoint and access-key terms of the Secret completeness check. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
2b5fa1f0eb |
feat(backups): record the RDB snapshot location and size for Redis backups
Backups made by the Redis strategy, which also serves Valkey, carried no Backup.status.artifact at all: neither a URI nor a size. The snapshot is one object per backup, <namespace>/<app>/<backup>.rdb, whose key the driver already records, and the default strategy uploads it with the S3 coordinates of the backup credentials Secret the controller projects into the application namespace. On completion the driver now stats that object with the same coordinates and records its s3:// URI and size. The artifact is set only when the object is found, so a custom strategy that writes elsewhere gets no URI pointing at a missing object. A Backup that already exists is returned without probing, as for the MariaDB and CNPG drivers. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
9e8e10c307 |
fix(backups): re-render the default backup objects when the S3 endpoint changes (#5230)
## What this PR does Refs #5205. The S3 endpoint the platform backups use is copied twice, and neither copy is refreshed after `publishing.host` changes. The COSI sidecar writes `BucketInfo` once per `BucketAccess`, so the credentials Secret `bucket-cozy-backups-system-credentials` rendered from it keeps the old endpoint. The Velero BSL and the default Strategy CRs keep whatever endpoint the last render of `backupstrategy-controller` saw. The default objects gate only acted on missing objects, so backups kept going to the old host and nothing reported it. The gate now also forces a render when a copy is stale: - the bucket's `<bucket>-system` release, when the credentials Secret's endpoint differs from the COSI Secret it is rendered from; - `backupstrategy-controller`, when the endpoint an object was rendered with differs from the credentials Secret's. The chart stamps the rendered endpoint on the BSL and on every default Strategy as `backups.cozystack.io/endpoint`, because the endpoint itself sits at a different path in every kind. Objects without the annotation count as stale until a render stamps it; `MinForceInterval` bounds the forces. Stale credentials are forced before the BackupClass and the routed objects are read, so an absent BackupClass or an unreadable Strategy still fails the check but does not keep the old endpoint in the credentials Secret. A `BucketInfo` without an endpoint is not compared, since rendering from it would write an empty endpoint or fail the bucket release. Repairing missing objects keeps working while the credentials comparison is stale or fails, and a `systemSecretName` that is not derived from a COSI Secret (no `-credentials` suffix, or the raw COSI Secret itself) is not compared at all. RBAC is unchanged: the controller already holds cluster-wide `get` on Secrets. No new metric: nothing would consume it. What this does not do: the COSI Secret still holds the endpoint the `BucketAccess` was granted with, so after a host change the endpoint follows only once an operator deletes the `BucketAccess` and forces `bucket-cozy-backups` to recreate it (this issues new S3 keys). From there the gate replaces the remaining manual steps of the recovery described in the issue. Automatic re-grant is left for a separate issue, and the operator runbook goes to the website, hence `Refs` rather than `Fixes`. Known limitation: the credentials comparison runs only when `backupStorage.systemSecretName` is `bucket-<bucketName>-system-credentials`, the Secret the forced `bucket-<bucketName>-system` release renders. If an operator sets `bucketName` and `systemSecretName` inconsistently, the comparison is switched off rather than forcing a release that never rewrites the compared Secret, so a stale endpoint in that Secret is not repaired. Repairing a missing credentials Secret still forces the release derived from `bucketName`, as before this PR. Tested with unit tests for the gate (including the comparison normalisation shared with the chart helpers), helm-unittest for the annotation and the wiring, and a bats guard that every rendered Strategy CR and the BSL carry the annotation. On a live cluster, a server-side render of the chart produced exactly the annotation the gate derives from the real credentials Secret, so the steady state does not force anything. ### Screenshots Not a UI change. ### Downstream repositories - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: an operator runbook for changing `publishing.host` with platform backups enabled (re-grant the `BucketAccess`, then let the gate converge; restart ClickHouse pods that read the credentials from environment variables). Not filed yet, pending approval of the text. - [ ] [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): the backup controller now re-renders the Velero BSL, the default backup strategies and the platform bucket's credentials Secret when their S3 endpoint is stale, so after the platform bucket's BucketAccess is re-granted for a new publishing.host, backups move to the new endpoint without forcing releases by hand. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **New Features** * Backup strategies and storage locations now record their storage endpoint, enabling detection and repair of outdated endpoint settings after bucket access changes. * Bucket credentials and backup objects are refreshed independently when their endpoints are missing or out of date. * **Documentation** * Clarified when endpoint changes trigger updates and how suspended releases are handled. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
ee5e51ee6c |
fix(backups): keep forcing objects that still lack the endpoint annotation
An object without backups.cozystack.io/endpoint stopped counting as stale as soon as one forced render had been requested, whether or not that render succeeded. A failed upgrade left the objects unannotated and the gate silent until the controller restarted, which also hid the climbing force counter meant to report exactly that failure. Such an object now stays stale until a render stamps it; MinForceInterval already bounds the cost. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
bd2b6c4b49 |
fix(backups): leave the credentials Secret alone when BucketInfo has no endpoint
A BucketInfo without an endpoint differs from every credentials Secret that has one, so the gate forced the bucket release on every throttle window. That render cannot converge: an empty endpoint would be written into the Secret every driver reads, and with the key absent the template's trimPrefix fails and parks the release in Failed. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
399ccd3f4a |
fix(backups): refresh stale credentials even when the routed objects are unreadable
The credentials endpoint comparison needs only the credentials Secret and the COSI Secret it is rendered from, yet its forced render was issued only after the BackupClass and every routed object had been read. A missing BackupClass or one unreadable Strategy therefore kept the old endpoint in the credentials Secret, which the projector hands to every driver, not just the one whose object failed. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
ef7626a951 |
fix(backups): keep repairing missing default objects while credentials lag
The credentials comparison returned early when the credentials Secret was stale or the COSI Secret could not be read, so a missing default Strategy or BSL was no longer created on that tick, nor on any later one while the bucket release stayed suspended, kept failing, or the COSI Secret stayed unreadable. Repairing missing objects is what the gate did before the comparison existed, and it does not depend on the answer. Run the object check on every tick and force each release for what it renders, reporting a comparison error alongside instead of in place of the repair. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
d0f47f7dd5 |
fix(backups): compare the credentials endpoint only for a Secret derived from COSI
backupStorage.systemSecretName may name a Secret the bucket chart does not render from a COSI Secret: the raw COSI Secret itself, which the projector accepts, or any Secret without the -credentials suffix. The COSI Secret name was then the source name unchanged, so the gate compared a Secret with itself and either reported it stale on every tick or failed the check, and stopped repairing missing default objects as well. Skip the comparison when there is no distinct COSI Secret or no endpoint key to compare. The gate compares the annotation with what the chart's endpoint helper renders, and the helper renders a present but blank endpoint as a bare https:// instead of falling back, so the gate has to compute the same value or its expectation and the chart disagree for that Secret. The gate's docs, metric help and log lines named only missing objects, so they did not cover a stale endpoint they now report. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
myasnikovdaniil
|
970191dc8a |
fix(clickhouse): restore the archive a Backup was taken as (#5208)
## What this PR does A ClickHouse `RestoreJob` restored the newest archive under the app prefix, whichever `Backup` it named. Strategy pod generated the archive name with `date -u` and nothing was recorded on the `Backup`, so restore could only list the remote archives and take the last one. Restoring an older `Backup` brought back the latest data. Now the controller names the archive before it creates the backup Job (`clickhouse-<app>-<startedAt>` from persisted `status.startedAt`, so retried pods write the same one), passes it to the pod as `.ArchiveName` and records it in the `Backup` `driverMetadata`. Restore takes exactly that archive and fails with its name when storage has no such archive. The name is recorded only when the Job's pod got it, so a Job left from the previous controller during an upgrade, or a custom strategy with its own names, leaves no name instead of a wrong one. A retried pod reuses the name, so it waits for an attempt the sidecar still runs (no 423) and asks for `resume`. A `Backup` outlives its `BackupJob` and a Plan names `BackupJob`s after the schedule slot, so a `BackupJob` created again under a used name met the earlier run's `Backup` when its Job completed, took it as its result and left the archive it just wrote recorded nowhere. Now it fails if that `Backup` records another archive, and the message names both. A retry of the same run still gets its own `Backup` back. Old `Backup`s have no recorded name and stay restorable: the pod logs a `WARNING` and takes the newest archive stamped no later than `takenAt`, and fails when nothing qualifies. While the chart rolls out, previous controller pod can still render the new template and pass no `takenAt`, so a restore of a `Backup` without recorded name fails before it touches any table, and works when created again after the rollout. Fixes https://github.com/cozystack/cozystack/issues/5199 Tests are go unit tests for naming, recording, a reused `BackupJob` name and restore render, and `hack/altinity-backup-strategy.bats`, which runs the shipped script against a stubbed clickhouse-backup API (the restore tests go red on the old template). Checked on a live ClickHouse with two backups of 1500 and 2000 rows: restore of the older one by recorded name ends with 1500 rows, fallback for an unnamed older backup picks the same archive with the warning, a missing archive fails without touching the table, a retry over an existing local backup completes under the same name. The controller binary itself was not run on the cluster, only its rendered output in the pod. ### Downstream repositories - [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(clickhouse): a RestoreJob now restores the archive its Backup was taken as, not the newest one. New Backups record the archive name, Backups taken before this release are restored best effort from the newest archive stamped no later than their takenAt, with a warning in the restore pod log. A ClickHouse BackupJob created again under a name whose Backup still exists now fails instead of reporting that Backup as its result. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **New Features** * Backups now use a consistent archive name tied to the backup job, and completed backups retain that name for future restores. * Restores can select the recorded archive and backup timestamp. If no archive was recorded, the system selects the newest eligible archive for the application. * **Bug Fixes** * A restore now fails when its recorded archive is missing instead of silently selecting a different one. * Backup requests avoid starting a duplicate action when the matching action is already in progress. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
d7ae34dfc3 |
fix(backups): fail a ClickHouse BackupJob whose Backup name is taken
A Backup outlives the BackupJob that made it, so a BackupJob created again under a name already used meets the earlier run's Backup when its own Job completes. That happens by hand, and from a Plan reconciled again inside its starting deadline, since a Plan names each BackupJob after its schedule slot. The Altinity driver took the earlier Backup as its result: the BackupJob turned Succeeded with a BackupRef recording the earlier run's archive, the archive this run wrote was recorded on nothing, and restoring what the BackupJob reported brought back the older data. An existing Backup is now taken as this run's only when it records this run's archive, which is what an earlier reconcile of the same run leaves when its status write is lost. Any other fails the BackupJob, naming both archives, and leaves the earlier Backup untouched. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Alexey Artamonov
|
cd91138f33 |
fix(backups): re-render the default backup objects when the S3 endpoint changes
The S3 endpoint the platform backups use is copied twice and neither copy is refreshed. The COSI sidecar writes BucketInfo once per BucketAccess, so the credentials Secret rendered from it keeps its endpoint, and the Velero BSL and the default Strategy CRs keep whatever endpoint the last render of backupstrategy-controller saw. The default objects gate only acted on objects that were missing, so after a publishing.host change every backup kept going to the old host with nothing reporting it. The gate now also forces a render when a copy is stale: the bucket's <bucket>-system release when the credentials Secret's endpoint differs from the COSI Secret it is rendered from, and backupstrategy-controller when the endpoint an object was rendered with differs from the credentials Secret's. That endpoint sits at a different path in every kind, in some only as a bare host, so the chart stamps the rendered value on each object as backups.cozystack.io/endpoint and the gate compares that. Objects rendered before the annotation existed count as stale until one forced render has gone through, which stamps it. The COSI Secret itself still holds the endpoint the BucketAccess was granted with; it follows a host change only once the BucketAccess is re-granted. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Myasnikov Daniil
|
59f0013b1f |
fix(backups): attribute a MariaDB dump only when no other run can own it
Every backup of a MariaDB application writes its dump under one shared prefix, so the probe took the dump whose file-name timestamp (the backup pod's clock) and LastModified (the object store's clock) both fall in the operator Backup's lifetime, with a minute of slack for skew. That keeps a neighbour's dump out only while this run's own dump is in the window. With the pod clock more than the slack behind, the own dump is named before the window opens, and a previous run that finished within the minute before this one was created becomes the single match: a 123-byte dump was reported as 666 bytes. A pod clock ahead or an object store clock behind does the same with the next run, and an own dump that is gone by the time it is looked up, as it can be for a restore long after the backup, leaves a neighbour as the single match with no skew at all. A dump is now attributed only when ownership is proven: - it is the one dump that either clock places in the window. A dump that one clock places there and the other does not may be this run's own with a skewed clock, so it leaves any other match unproven, the way a second dump in the window already did, and is not taken itself; - both of its clocks place it in the window; - neither clock places it in the window of another operator Backup that writes under the same bucket and prefix and has completed. This rejects a neighbour's dump when this run's own is missing, which the clocks alone cannot tell. Operator Backups still running are not counted as neighbours: their window has no end, and one stuck short of Complete (a Job whose pod never starts stays JobRunning for good) would otherwise keep every later run of the application from being sized. A dump such a run writes inside this run's window still competes with this run's own. Narrower slack does not prove ownership: a neighbour that finished within the narrower slack still matches, and a smaller skew pushes the own dump out. The Job's start and completion times are stamped by the control plane, like the operator Backup's own, so they narrow the window by seconds without removing the dependence on the two clocks a dump carries. What is left: a neighbour's dump is taken only when this run's own is missing from the listing or misplaced on both clocks and the neighbour's operator Backup no longer exists or has not completed. Every other unproven case leaves the size unset, as two runs that overlap or follow each other within the slack already did. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
560c44f71e |
fix(backups): read a MariaDB dump from where its operator Backup wrote it
The size probe took the bucket, endpoint, prefix, credentials and TLS settings from the strategy as it renders on the reconcile that creates the Backup, which is not necessarily what it rendered when the operator Backup was created: an existing operator Backup is reused untouched, and a backup can run for up to the driver's 30-minute deadline. An admin who edits the strategy in between sends the probe to the new location, where it measures the wrong prefix or nothing at all. The operator Backup's own spec.storage.s3 is where the dump went, so the probe reads that. It is also the only source a restore can use, since it targets that operator Backup long after the strategy may have changed. An operator Backup that does not store to S3 is refused before anything is listed. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
21f51a2e74 |
refactor(backups): return the matched MariaDB dump, not only its size
A MariaDB restore has to name the dump it restores from: without a target time the operator restores the newest dump under the prefix, which is not necessarily the one of the Backup being restored. The size probe already finds that dump, but it returns only its size, folds a missing, an ambiguous and an unreadable listing into the same 0, and lives on BackupJobReconciler, which the restore reconciler is not. Split the probe in two: findMariaDBDump reads the bucket and returns the matched object (key, file name, the timestamp in its name, size and LastModified) or an error that tells "not found" from "ambiguous"; the size path keeps its timeout and reads only the size. The Secret lookup and the S3 client move onto an objectStore value that any reconciler can build from its own client, and BackupJobReconciler keeps one-line wrappers so the CNPG probe is unchanged. sizeBytes is reported exactly as before. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
c140354977 |
test(backups): build operator Backups with the operator's success shape
mariadb-operator marks a Backup Complete=True for a failed Job as well as a successful one; only the reason tells them apart (JobComplete with message Success, or JobFailed with message Failed). The fixtures that stand for a finished backup set Complete=True and nothing else, so they describe a state the operator never produces and would read as a failure to a driver that checks the reason. Give them the success shape the operator writes. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Andrey Kolkov
|
92fa979690 |
fix(backups): trust only the endpoint CA where the backup writer does
With an endpoint CA the probe trusted the system roots plus that CA. barman-cloud gets endpointCA as AWS_CA_BUNDLE, which replaces the default trust store, so a CNPG probe could accept an endpoint the backup itself would have refused. Let the connection say whether the CA adds to the system roots or replaces them, and replace them for CNPG. mariadb-operator appends its CA to the system pool, so MariaDB keeps that. Pin in reconcile tests that a Create the apiserver rejects still fails the BackupJob, so only the read-back of an existing Backup is retried. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
080df1b2f2 |
fix(backups): surface a Backup that cannot be read back and skip its probe
A Backup that exists but cannot be read back is retried through the controller's backoff, which left the BackupJob Running with nothing on it saying why. Set Ready=False with reason BackupNotReadBack before handing the error back. When the check for an existing Backup itself fails, the Backup may well exist, so a size probe would only be discarded, and a read that keeps failing reran it, up to the probe timeout on the single BackupJob worker, on every retry. Skip the probe in that case and create the Backup without a size. Drive both behaviours through the MariaDB and CNPG reconciles in tests. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
523d74a503 |
fix(backups): retry instead of failing when an existing Backup cannot be read
When Create collides with a Backup an earlier reconcile already made and the read that follows fails, the drivers failed the BackupJob, although the backup and its Backup both exist. Return a distinct error instead, which the drivers hand back to the controller so the reconcile is retried with backoff and then finds the Backup. minio.New already refuses an endpoint with a path prefix and accepts a trailing slash, so drop the duplicate check in the shared client; the CNPG probe keeps its own check because it parses the URL itself and would otherwise drop the path before minio sees it. Say in the sizeBytes API doc what the value covers, such as the base backup without the shared WAL archive for CNPG, and that strategies which cannot attribute stored bytes to one backup leave it unset. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
2b21763577 |
fix(backups): do not fail a backup on a transient retry-check read
The check for a Backup an earlier reconcile already created returned its read error, and the drivers turn any error from that path into a terminally Failed BackupJob. An apiserver timeout right after the operator backup completed therefore lost a backup whose data was safely in S3. The check only saves a probe, and the Create that follows already handles an existing Backup, so log the error and go on to Create. A trailing slash on the S3 endpoint is not a path prefix; the operators accept it, so do not refuse it. Pin the uncached AlreadyExists refetch, the upper slack of the MariaDB window and the probe timeout with tests. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
a3d854739d |
fix(backups): attribute MariaDB dumps on two clocks and read retries uncached
The MariaDB probe identified this run's dump by the timestamp in its file name, which comes from the backup pod's clock. With that clock skewed past the slack, this run's dump falls outside the window while a neighbouring run's dump can be the single match left, and its size is reported as this backup's. A dump now has to fall inside the window by both its name and its LastModified, which the object store stamps, so a single skewed clock leaves the size unset instead. The name parser also accepts the layout later mariadb-operator releases write for compressed dumps, backup.<ts>.sql.<ext>, and checks the compression token in either layout, so an operator upgrade does not silently stop the measurement. The retry check and the AlreadyExists refetch read the Backup through the cache. A retry usually follows a Create made moments earlier; a cache that does not show it yet turned the refetch into NotFound and failed a backup that had succeeded. Read it from the API server instead. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
3b86f57803 |
fix(backups): refuse to probe an S3 endpoint behind a path prefix
An endpoint such as https://s3.example.org/prefix cannot be addressed by minio-go, which has no path-prefix option, and the prefix cannot be added below the client either, because the SigV4 signature covers the request path. The CNPG probe was worse than failing: it kept only the host of endpointURL and listed the bucket at a location barman never writes to. Reject such endpoints with an explicit error, in the CNPG probe and in the shared client, so the size is left unset instead of measured somewhere else. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
038f35376a |
fix(backups): do not report a partial listing or re-probe on retry
minio-go ends a listing whose context is cancelled between pages by closing the channel without an error item. When the probe timeout expired between pages of a large CNPG base backup, the pages already read were summed and reported as the measured size. Check the context once the listing channel closes and leave the size unset instead. The probe ran before the Backup was created, so a reconcile that retried after the Backup already existed probed the bucket again, held the single BackupJob worker for up to the timeout, and then discarded the result in favour of the existing object. Return an existing Backup before probing. Log the ambiguous MariaDB case (several dumps in the window) at info like the other reasons the size stays unset, and cover the session-token credential path with a test. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
f222e52863 |
fix(backups): harden the artifact size probe
The probe runs inside the reconcile that creates the Backup, and the BackupJob controller has a single worker, so its timeout delays every other BackupJob while an endpoint hangs. Lower it from 30s to 10s. List the MariaDB prefix non-recursively, as mariadb-operator does. The default MariaDB prefix and the default CNPG destinationPath are the same <namespace>/<name>/ path, so a recursive listing of a MariaDB backed up next to a same-named Postgres walked the whole WAL archive and ran out of time. The CNPG base-backup prefix is still listed recursively. Build the custom-CA transport from minio.DefaultTransport so it keeps the proxy-from-environment and dial/TLS timeout settings a bare http.Transport drops, and cover it with a test against a TLS endpoint with a self-signed certificate. Log the reasons the size could not be measured (credentials, client, listing, empty CNPG prefix) at info. The chart runs the controller at info, which hid them before. The expected ambiguity of several MariaDB dumps in the window stays at debug. Skip an endpointCA without a Secret name, as buildBarmanObjectStore does. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
de60488a2c |
fix(backups): report sizeBytes for CNPG backup artifacts
CNPG backups arrived without Backup.status.artifact.sizeBytes: cnpg.io/Backup status carries no size, and the data lives in barman's object-store layout rather than in a single object. barman-cloud uploads every base backup under <destinationPath>/<serverName>/base/<backupID>/ (backup.info plus the data and tablespace tarballs, possibly split into parts). On completion the driver now lists that prefix with the credentials and endpoint CA of the rendered barmanObjectStore and records the total. The WAL archive is not counted: it is one stream shared by every base backup of the server. The field stays unset when the backup ID is unknown, the bucket cannot be read, or nothing is stored under the prefix. The S3 client and Secret lookup are now shared with the MariaDB driver. Refs #4316 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
d4ad5612d3 |
fix(backups): report sizeBytes for MariaDB backup artifacts
Backup.status.artifact.sizeBytes was only ever set by the etcd strategy, so MariaDB backups always arrived without a size. mariadb-operator writes each logical dump as <prefix>/backup.<RFC3339>[.<compression>].sql under a prefix shared by every backup of the application, and the URI the driver records is synthetic, so there is no single object to HEAD. On completion the driver now lists the prefix with the tenant credentials the operator Backup was rendered with and takes the size of the one dump whose timestamp falls inside the operator Backup's lifetime. When the bucket cannot be read or the window does not match exactly one dump, the field stays unset. The API doc now states that an absent sizeBytes means "unknown", and the Velero strategy documents why it leaves the field unset: kopia stores data content-addressed across backups, so there is no per-backup stored size. Refs #4316 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Myasnikov Daniil
|
47b497194e |
fix(backups): restore the ClickHouse archive a Backup was taken as
The Altinity strategy named each archive inside its Pod with date -u and recorded nothing on the Backup, so a restore could only guess: it listed the remote archives under the application's prefix and took the newest. A RestoreJob of an older Backup therefore restored whichever backup had run last. The controller now names the archive before it creates the backup Job, from the BackupJob's persisted StartedAt, so every reconcile and every retried Pod of the Job writes the same one. The name is recorded in the Backup's driverMetadata next to the strategy parameters, and a restore renders it into the strategy Pod, which restores exactly that archive and fails, naming it, when remote storage no longer holds it. The name is recorded only when the Job's Pod was given it, so a Job created by an earlier controller, or a strategy that names its own archives, leaves the Backup without one instead of pointing it at an archive that does not exist. Because a retried Pod reuses the name, it waits on an attempt the sidecar is still running instead of being refused with 423, and asks for resume, so a local or partial remote backup an earlier attempt left is completed rather than refused as existing. Backups taken before names were recorded stay restorable: the Pod picks the newest archive whose name is stamped no later than the Backup's takenAt, which is set only after the backup Job finished, and logs a warning that the choice is a best effort. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
4087218a02 |
docs: move the repo-only guides onto the website (#5034)
## What this PR does Seven guides under `docs/` were operator facing and written alongside the code, and left in the source tree where no user looks. They are on the website now, so this deletes them here and repoints every reference. ``` docs/operations/backup-classes.md -> operations/services/backup-classes docs/operations/resource-presets.md -> guides/resource-management docs/storage-immutability.md -> operations/storage-class-immutability docs/operations/system-memory-limits.md -> operations/configuration/system-memory-limits docs/gateway-classes.md -> networking/gateway-classes docs/observability/distributed-tracing.md -> operations/services/monitoring/distributed-tracing docs/operations/seaweedfs-capacity-for-backups.md -> operations/services/object-storage/capacity-for-backups ``` `docs/release.md`, `docs/deprecating-applications.md` and the two SeaweedFS runbooks in `docs/operations/` stay. A workflow, `AGENTS.md`, a migration and a bats suite read them by path, which is the test from #3777. References point at `https://cozystack.io/docs/v1/...`. The site rewrites that short prefix to the latest version in `layouts/404.html`, so the links follow releases and pin none. Backup Classes, resource management and `storageClass` immutability exist in the latest version already. The other four pages are only in `next/` until 1.7.0 is promoted, and their links start to resolve then. The generated mongodb files come from `values.yaml` through `make generate` with cozyvalues-gen v1.7.0, the version pinned in `.github/workflows/pre-commit.yml`. Tests: helm unit tests for mongodb (151), postgres (209), vm-disk (114) and kubernetes-nodes (223), go tests for backupcontroller, presets and the operator, rd-presets-check, the migration-50 bats suite. ### Downstream repositories - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: cozystack/website#763 Walked the rest of the trigger map against the diff. No provider follow-up: the mongodb `values.schema.json` diff is one `description` string, with no field added, removed or renamed, no enum, no default, and no `kind`, `plural` or `release.prefix` change. `packages/core/platform/values.yaml` changed in comments only and no key, so the hand written platform-package table on the site is unaffected. Please merge after cozystack/website#763, the links in the code only resolve once those pages deploy. ### Release note ```release-note docs: seven guides move from the docs/ directory of this repository to cozystack.io (Backup Classes, resource presets, storageClass immutability, system component memory limits, GatewayClass selection, distributed tracing, SeaweedFS capacity for backups), and in-repo references point there. ``` |
||
|
Myasnikov Daniil
|
b83cc087cc |
docs: move the repo-only guides onto the website
Seven guides in docs/ were written next to the code and left in the source tree, where no operator looks. The website carries them now, so a second copy here only lets the two drift again: the Backup Classes page had already fallen behind its original. docs/release.md, docs/deprecating-applications.md and the two SeaweedFS runbooks in docs/operations stay. A migration, a bats suite, a workflow or AGENTS.md reads each of them by path, which is the test for keeping a document in this repository. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Mattia Eleuteri
|
b5d7e43c49 |
docs(backups): say CDI adopts a restored PVC through the DataVolume
The restore controller and the user-restore example described the allowClaimAdoption annotation its resource modifiers put on restored PVCs as what lets CDI adopt them. CDI v1.66 reads that annotation only on the DataVolume; on a PVC it reads nothing but createdForDataVolume. Adoption rests on the vm-disk chart annotating the DataVolume Flux recreates, and a reader trusting the PVC rule could rely on it for a restore without such a DataVolume, or drop the DataVolume annotation as redundant. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Andrey Kolkov
|
9af1c0f902 |
fix(backups): prepare standalone VMDisk restore targets before the data mover runs
collectUnderlyingResources returned nothing for a non-VMInstance kind, so prepareForRestore did nothing for a standalone VMDisk: no HelmRelease suspend and no DataVolume delete. An in-place VMDisk restore then raced Flux/CDI — the restored HelmRelease reinstalled and CDI recreated the DataVolume over the same PVC name the data mover was repopulating, wedging the RestoreJob. Report a standalone VMDisk's own disk as its underlying resource, so the existing (VMInstance-proven) restore preparation suspends the vm-disk-<name> HelmRelease, deletes its DataVolume and — with keepOriginalPVC — renames the live PVC before the data mover recreates and repopulates it. No VM, so no IP/MAC is collected. A VMInstance is halted before its disks are touched; a VMDisk target is not, and every VMInstance disk is a VMDisk. Renaming the PVC of a disk a running VM mounts leaves the original Terminating behind pvc-protection, Velero skips a PVC that still exists, and the RestoreJob would report Succeeded having moved nothing while the VM loses its disk on the next restart. An in-place VMDisk restore now fails before changing anything when a Pod that has not terminated mounts the disk's PVC, naming the Pod and telling the user to stop the VM or restore the VMInstance instead. A VMDisk Backup taken before the disk was recorded as its underlying resource has none, so its disk is now derived from the application name and such a restore is prepared and checked for a mounting Pod as well. The per-disk orLabelSelectors stay a VMInstance concern. A VMDisk strategy already selects its disk, and Velero rejects a Backup or Restore that carries both labelSelector and orLabelSelectors, so appending them for a VMDisk would break a strategy written with labelSelector. When an in-place restore fails after preparation, nothing is rolled back: resuming the disk's HelmRelease while its PVC is renamed and its DataVolume deleted would have CDI import a fresh disk under the original name, which looks healthy and hides that the data sits in the -orig- PVC. The failure message and a Warning event now name the suspended HelmReleases and the -orig- PVCs so the user can recover them, read back from the cluster, so a failure before a step ran does not name what it would have left. Preparation takes the target apart before Velero validates the Restore, so it is refused up front when the Velero Backup is gone or not Completed. That check now runs on every pass behind the same failure path as the rest of preparation, and a FailedValidation Restore fails the RestoreJob like a Failed one instead of leaving it Running. A HelmRelease suspend or a VM halt that did not happen now stops preparation instead of renaming and deleting under a running release or VM, and a VMInstance's disks are renamed only once its launcher Pod has let go of them. A VMDisk held only by a Pod already being deleted is waited for rather than refused, for up to 30 minutes after the RestoreJob started. Between renaming a kept disk's PVC and deleting its DataVolume, CDI can recreate an empty PVC under the original name. Velero then skips the PVC as already present, the RestoreJob reports Succeeded and the VM boots a blank disk. Once the DataVolume is gone, preparation now deletes a PVC found under the original name, read uncached, before the Velero Restore is created. Preparation spans several reconciles, so the RestoreJob records a PreparationStarted condition before it first changes the target, and retries without changing anything when recording it fails. A refusal on a later pass (the Velero Backup expired, a VM started on the disk) then names what preparation left behind instead of claiming that nothing changed, and a preparation still waiting 30 minutes after it started fails the same way instead of polling forever. Now that a VMDisk target is prepared, keepOriginalPVC=false reaches it too: its DataVolume is deleted with orphan propagation and the bound PVC is deleted before the Restore, as for a VMInstance disk. Left in place, Velero would skip it and CDI would refuse the DataVolume the resumed HelmRelease creates. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
0c2f131192 |
feat(backups): back up Valkey with the Redis strategy
Valkey runs on the same spotahome RedisFailover operator as Redis and speaks the same replication protocol and RDB format, so it needs no strategy kind of its own. The only difference the backup Job sees is the chart's resource-name prefix: valkey-<app> instead of redis-<app> for the sentinel Service and the auth Secret. The Redis driver now accepts applicationRef kind Valkey as well as Redis and hands the template .Release.Prefix, derived from the app's kind, which cozy-default-redis uses for every name it builds. A kind with no known prefix fails the render instead of producing an empty one. cozy-default routes apps.cozystack.io/Valkey to cozy-default-redis. A restore must target an app of the same kind as the backup's source: replaying a Redis dump into Valkey, or the reverse, is not something the strategy can promise across engine releases. The shipped template reads the prefix as `.Release.Prefix | default "redis-"`: during an upgrade the previous controller, which has no Prefix in its render context and serves only Redis, renders the new CR, and a bare reference would print "<no value>" into the names. The redis:8 image already loads RDB v11, which both shipped Valkey versions (7.2, 8.1) write. Signed-off-by: Andrey Kolkov <androndo@gmail.com> Assisted-By: LLM |
||
|
Andrey Kolkov
|
6a75883013 |
fix(backups): keep the CNPG purge for unmarked Clusters this restore did not render
A recovery Cluster without the restored-server-name annotation fell back to the creation-time comparison. On the current chart the annotation is absent when bootstrap.newServerName is empty, i.e. a manually configured recovery: re-rendered during the WAL-archive wait it is created after StartedAt, the purge was skipped and the restore could report success on the manual recovery source. Use the creation-time fallback only when the application's bootstrap.newServerName already carries this restore's server name, which keeps it for Clusters rendered by a chart that predates the annotation. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
35ca217839 |
fix(backups): name the restore that goes first in ConcurrentRestore
restoreAhead returned the alphabetically smallest of the jobs ahead, so with more than one ahead the ConcurrentRestore message could name a job that is not the one holding the application. Return the first job in restoreGoesFirst order, and pin the creation-time tie-break with cases where creation order and name order disagree. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
e4e991e837 |
fix(backups): skip the CNPG restore purge only for this restore's Cluster
The in-place CNPG restore skips deleting the live Cluster when it carries bootstrap.recovery and was created after the RestoreJob started, taking that as "this restore's own purge and re-render produced it". The timestamp does not say which restore rendered the Cluster. A Cluster deleted while the restore waits for the WAL archive is re-rendered by the still-active HelmRelease from the previous restore's values and is created after StartedAt too; the restore then skipped its purge and reported Succeeded on the previous restore's data. The restore patch already sets bootstrap.newServerName to restoredServerName(cluster, RestoreJob UID). The chart now annotates the Cluster with it as postgres.cozystack.io/restored-server-name, whether or not it renders the barman plugin that also carries the value, and the purge is skipped only when the annotation names this RestoreJob. The mark lives on the rendered Cluster rather than in RestoreJob status, so it still covers the failed status write the skip exists for. A Cluster rendered by a chart that predates the annotation keeps the creation-time check. Fixes #3322 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
3daf942dce |
fix(backups): refuse a restore into an application another restore holds
Nothing stopped two RestoreJobs from rewriting one application at once. Each restores the target from its own Backup, so the application ends up on whichever finishes last, and the CNPG driver's purge decision cannot tell which of the two produced the live Cluster: the second job could skip its purge and report Succeeded on the first job's data. A RestoreJob that has not started now fails with reason ConcurrentRestore when another unfinished RestoreJob in the namespace writes into the same application and goes first. A started restore goes before one that has not; between two that have not, the older one goes first, by creation time and then name, so a running restore is never failed by a newer one. The refused job fails instead of waiting, so the application is not restored a second time later, after the first restore, by a job its author may no longer expect to run. The gate lists RestoreJobs through the manager's API reader rather than the informer cache. Creation times have one-second precision, so two jobs created in the same second are ordered by name, and a job reconciled before the cache held its peer's StartedAt would see that peer as not started and proceed alongside it. The controller runs one worker, so a peer's StartedAt written by an earlier reconcile is on the API server by the time the next job reads it; SetupWithManager now states that. The target is resolved as the BackupJob hold resolves it, so a Velero restore into another namespace does not collide with one in place. Refs #3322 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
f4bde9e50d |
fix(backups): delete the original PVC of an in-place VM restore without keepOriginalPVC (#4672)
<!-- 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
An in-place VMInstance restore with `keepOriginalPVC: false` reports
`Succeeded` without restoring the disks, and leaves every disk it
touched unable to come back. `prepareForRestore` deletes each DataVolume
with Orphan propagation and leaves the PVC in place, on the assumption,
stated in the `deleteDataVolume` comment, that Velero overwrites it.
Velero does not overwrite a PVC. Its CSI restore action returns early
for a claim that already exists, so no DataDownload is ever created, and
with `existingResourcePolicy: update` the merge patch it attempts
instead is rejected because a bound claim's spec is immutable. All that
reaches the claim is the labels-only fallback patch, which also drops
the `cdi.kubevirt.io/allowClaimAdoption` annotation the resource
modifier added.
The claim left behind then blocks the disk for good. Orphan propagation
has removed its owner reference to the DataVolume, and it lacks the
`cdi.kubevirt.io/storage.populatedFor` annotation that
kubevirt-velero-plugin writes on the backed-up copy, so when the
HelmRelease resumes, the CDI webhook refuses the DataVolume it creates
with `Destination PVC <ns>/<name> already exists`, on every reconcile.
Deleting the disk through the API leaves the PVC behind, and a new disk
of the same name fails the same way.
Observed on a v1.6.3 cluster, for a VMInstance restore with
`keepOriginalPVC: false` whose disk was attached to the VM. The Velero
log for the restore, trimmed:
```text
level=info msg="Retrieved per-restore resource modifiers from configmap cozy-velero/restore-modifiers-..."
level=warning msg="PVC already exists and ExistingVolumeDataPolicy is not in-place restore. Skip restore this PVC." Action=PVCCSIRestoreItemAction PVC=tenant-datalab/vm-disk-e2edisk
level=info msg="Applying resource modifier patch on tenant-datalab/vm-disk-e2edisk"
level=warning msg="patch attempt failed for /v1, Kind=PersistentVolumeClaim tenant-datalab/vm-disk-e2edisk: PersistentVolumeClaim \"vm-disk-e2edisk\" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims"
level=info msg="backup/restore labels successfully updated for PersistentVolumeClaim tenant-datalab/vm-disk-e2edisk"
```
`velero restore describe --details` lists the PVC as `(failed)` and the
Restore is `Completed` with two warnings. Afterwards the claim had its
original creation timestamp and UID, no owner reference, no
`allowClaimAdoption`, no `populatedFor`, and the disk's HelmRelease was
`Ready=False` with the webhook error above. That cluster runs a Velero
build from `main`, whose wording is quoted above; the released v1.17.1
and v1.18.3 return early at the same point with `PVC already exists.
Skip restore this PVC.` (`pkg/restore/actions/csi/pvc_action.go`).
The fix deletes the PVC when `keepOriginalPVC` is false, only once its
DataVolume is confirmed gone so that CDI cannot recreate it, and holds
the Velero Restore until the claim has actually disappeared. A claim
still mounted stays `Terminating` behind `kubernetes.io/pvc-protection`,
and creating the Restore then would hit the same existing-claim path.
Velero then creates the PVC from the backup, the data mover fills it,
and the recreated DataVolume adopts it through `populatedFor`. A failed
deletion now fails the RestoreJob instead of letting it report
`Succeeded` over the old data. Before
|
||
|
Aleksei Sviridkin
|
b1192bfcbc |
fix(backups): create no driver object for a job that already finished (#4673)
<!-- 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 BackupJob and RestoreJob reconcilers decide a job is still active
from the informer cache, and the cache can trail their own writes. A
driver's first pass stamps `StartedAt`, which requeues the job, and may
then fail the job in the same pass on a transient error, for instance a
503 from the aggregated `apps.cozystack.io` API while
`createVeleroBackup` reads the application. The requeued pass can start
before the informer has seen `Failed`: it retries the step that failed
and, once the error has cleared, creates the driver object anyway. Its
own status write then conflicts on the stale resourceVersion, the next
pass sees `Failed` and skips the job, and a Velero Backup is left
running that no `Backup` artifact will reference and nothing tracks.
Reproduced against a real apiserver (envtest 1.34) with the real manager
cache and the reconciler wired through `SetupWithManager`, nothing else
writing the BackupJobs. The only fault injected is one 503 on the first
GET of each application through `apps.cozystack.io`. Out of 40
BackupJobs, every one ends `Failed` with `failed to create Velero
Backup: the server is currently unable to handle the request`, and on
`main` (
|
||
|
Mattia Eleuteri
|
06e72b26dd |
test(backups): give the restore stale-cache test a Completed Velero Backup
A Velero RestoreJob is about to require its Velero Backup to exist and be Completed before it halts the application. Without one in the fixture, the first pass of this test fails the RestoreJob before it suspends the HelmRelease, so the test would stop exercising the stale cache it is about. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
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
|
eafa441b9c |
fix(backups): create no driver object for a job that already finished
The BackupJob and RestoreJob reconcilers decide a job is still active from the informer cache, and the cache can trail their own writes. A driver's first pass stamps StartedAt, which requeues the job, and may then fail the job in the same pass on a transient error, such as a 503 from the aggregated apps.cozystack.io API while it reads the application. The requeued pass can start before the informer has seen Failed: it retries the step that failed and, once the error has cleared, creates the driver object anyway. Its own status write then conflicts, the pass after it sees Failed and skips the job, and a Velero Backup is left running that no Backup artifact will reference and nothing tracks. Against a real apiserver and the manager's cache, with one 503 on the first application read of each job, 5 to 10 BackupJobs out of 40 ended Failed with a Velero Backup. A restore pass that starts on such a stale copy does worse, since the step it retries can be destructive: suspending the VM's HelmRelease and halting the VM before creating a Velero Restore, or patching the Postgres application and purging its cluster. Both reconcilers now re-read the job through the manager's uncached reader when the cached copy says it is still active, and skip it when the apiserver holds it as finished. The check sits ahead of the dispatch, so it covers every driver, and a job the cache already reports as finished costs no extra request. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
6d064a8602 |
fix(backups): delete a restored disk's PVC only once the restore can run
Deleting the original PVC of a keepOriginalPVC=false restore is only safe when Velero can put the disk back. The Backup artifact outlives the Velero Backup it points to, which Velero garbage-collects at TTL, and nothing checked it before prepareForRestore halted the VM and deleted the disks: a restore of an expired backup lost the volume where it used to fail with the disk intact. The Velero Backup must now exist and be Completed before any preparation starts. A DataVolume that survives its deletion makes CDI recreate the claim under the original name, which Velero then skips as an existing PVC, so the restore would end Succeeded on a freshly imported disk. A failed DataVolume deletion now fails the restore, and the PVCs are deleted only once every DataVolume is gone. Reading the DataVolume back needs get on datavolumes, which the ClusterRole did not grant. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
6ea713d09b |
fix(backups): delete the original PVC of an in-place VM restore without keepOriginalPVC
With keepOriginalPVC=false the restore deleted each DataVolume with
Orphan propagation and left its PVC in place for Velero to overwrite.
Velero never overwrites a PVC: its CSI restore action skips the data
mover for a claim that already exists, and existingResourcePolicy=update
fails to patch the immutable spec of a bound claim, so it only stamps
its labels on it. The RestoreJob then reports Succeeded with the disk
still holding its pre-restore data.
The claim left behind also breaks the disk for good. It has lost its
owner reference to the orphaned DataVolume, and it lacks the
cdi.kubevirt.io/storage.populatedFor annotation that the backed-up copy
carries, so the CDI webhook refuses the DataVolume the resumed
HelmRelease creates ("Destination PVC already exists"), on every
reconcile.
Delete the PVC once its DataVolume is gone, and hold the Velero Restore
until the claim has actually disappeared: one still mounted stays
Terminating behind pvc-protection, and Velero would take it for an
existing claim again.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
|
||
|
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
|
520af57905 |
chore: drop private cluster, host and personal names from the tree
Private infrastructure names, a real hostname, a commented-out internal mirror block, an internal product name, and a reviewer's name in a comment do not belong in a public repository; drop them and replace what needs a stand-in with neutral placeholders. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d2e3eb4f82 |
fix(kubernetes, e2e): unblock autoscaling below min size, size the computeplane pool, fix the backupcontroller build (#4768)
## What this PR does Makes the backupcontroller test package compile again on main. #4587 added a flavor argument to `createCNPGBackupArtifact` and `marshalCNPGBackupSnapshot`, and #4669, whose CI ran before that, added two test calls with the old argument lists. Both PRs were green on their own, and together they break `go test ./internal/backupcontroller/`. The two calls now pass the same default flavor as the other tests in the file. The second commit fixes an e2e suite that fails the required E2E check here. It is the `computeplane` suite from #4713, and its worker pool was one node with a single vCPU. After the kubelet reservations that node has 900m of CPU to allocate. The tenant addons (cilium, coredns, csi, ingress-nginx, metrics-server, cert-manager) request exactly 900m between them. If cert-manager's startupapicheck hook pod is created last, it can't be scheduled and the install times out after 10m. The reinstall after remediation runs into the same full node. Whether that pod comes last depends on install order, so the suite passed on #4713 and failed here. The pool now has two workers with one vCPU each, the same shape the kubernetes suites use. One vCPU per worker stays on purpose, because of #3513. Two workers exposed a deadlock in the tenant cluster-autoscaler. The suite's pool is created at zero replicas and then gets a min size of 2, which CAPI does not apply to the live replica count on upgrade (#4786). The autoscaler's scale-up for a pending pod asks for one node, and the clusterapi provider refuses it because one is under the min size, although it is an increase. That refusal backs the group off, and the min-size enforcement skips groups in backoff, so the pool never leaves zero. Upstream had the same report in kubernetes/autoscaler#4942, closed as stale, and it is now tracked in kubernetes/autoscaler#10400. Our autoscaler image already patches the mirror case for max size, and the patch now refuses only a decrease below the min size. `go vet` and `go test ./internal/backupcontroller/...` pass, and so do the bats pins for the computeplane suite. The autoscaler patch adds a test for each direction, and the clusterapi package tests pass with it applied to 1.32.0. ### Screenshots Not a UI change. ### Downstream repositories - [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 NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved cluster autoscaling for worker groups outside configured size limits: groups below their minimum can scale up toward it, and nodes can be removed when a group exceeds its maximum. Scale-down requests below the minimum remain restricted. * **Tests** * Updated backup and restore test calls with PostgreSQL and empty flavor values. * Updated ComputePlane checks for a two-worker pool and clarified cert-manager installation timing expectations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ff3c409881 |
fix(backups): refuse a foreign -orig PVC and record the kept disk's reclaim policy (#4671)
## What this PR does Two defects in `renamePVC` (`internal/backupcontroller/velerostrategy_controller.go`), the step of an in-place VMInstance restore that keeps each disk as `<dv>-orig-<shortHash(RestoreJob name)>` when `keepOriginalPVC` is true, the default. ### A taken `-orig` name skipped the rename, and the restore went on `renamePVC` returned `nil` as soon as a PVC with the `-orig` name existed (`main:1263-1266`), whatever that PVC was. `prepareForRestore` then reached Step 4 (`main:1172-1183`), which deletes the DataVolume of every disk regardless of what Step 3 did, because a Step 3 error was only ever turned into a Warning event (`main:1165-1168`). The restore therefore ran exactly as `keepOriginalPVC: false` does and reported `Succeeded`: the live disk was not kept, and the `-orig` name held a different, older disk that the user takes for the pre-restore copy. When that name can already exist: - A RestoreJob created under the name of a deleted one. The hash covers only the RestoreJob name, and nothing in the controller ever deletes an `-orig` PVC, so the predecessor's PVC is still there. This is the realistic case, and it is what a scripted restore with a fixed RestoreJob name runs into on its second run unless the `-orig` PVC was removed in between. - Two RestoreJobs with different names whose 4-hex-digit hashes collide (16 bits) while restoring the same disk in the same namespace. Rare, but nothing guards it. - A reconcile of the same RestoreJob after a controller restart in the middle of a rename. The name is ours, but the early return treated a half-done rename as complete: the original PVC stayed in place and the PV stayed bound to it. - Truncation does not apply: the name is not truncated, and one longer than 253 characters makes the `Create` fail, an error that was swallowed in the same way. Each `-orig` PVC now carries the UID of the RestoreJob that created it, in `backups.cozystack.io/restore-job-uid`. `prepareForRestore` checks every `-orig` name before it suspends or halts anything, and fails the RestoreJob, naming the PVC, when any of them is held by a PVC another RestoreJob created. When the PVC is ours, `renamePVC` resumes: it deletes the original PVC only while it still claims the kept PV, and repoints the PV unless it already points at the `-orig` PVC, so a rerun is a no-op. Any other rename error now holds the restore back before Step 4 deletes a DataVolume, and the restore is retried, instead of the error becoming an event (see the next section). An `-orig` PVC created by an earlier controller has no UID annotation and is refused like any foreign PVC. The only RestoreJob that loses by this is one whose controller was upgraded after its rename and before its Velero Restore was created; it fails with the message above rather than going on. ### The rename read its own write back through the cache `renamePVC` created the `-orig` PVC, then read it back through the cached client to learn its UID for the PV's claimRef (`main:1314-1316`). The informer often has not seen a PVC created a moment earlier. On our lab, running Cozystack v1.6.3, an in-place restore of a two-disk VM with `keepOriginalPVC: true` renamed and restored the system disk, while the data disk lost the race, with this event on the RestoreJob: `Failed to keep old PVC vm-disk-e2e-dc-fix-data: failed to get -orig PVC vm-disk-e2e-dc-fix-data-orig-a574: PersistentVolumeClaim "vm-disk-e2e-dc-fix-data-orig-a574" not found`. As the error was only an event, the data disk's DataVolume was deleted and its original PVC stayed in place. Velero skipped that PVC and created no DataDownload for it, so its content was not restored, yet the restore finished `Succeeded`. The `-orig` PVC was left `Pending` on the same PV, which was by then `Retain`. `Create` already fills in the UID, so the read-back is gone. The other reads the rename decides on (the `-orig` PVC, the original PVC and the PV, before and after its own writes) go through the manager's uncached reader, wired as `APIReader`. The PV rebind retries on conflict, since the PV controller also writes the PV once the original PVC is deleted. A rename that fails for any reason other than a foreign `-orig` name requeues the restore rather than failing it. The DataVolume is still there, and the retry finds its own `-orig` PVC by the RestoreJob UID and resumes where it stopped. A cause that never clears keeps the RestoreJob `Running`, with a Warning event on each attempt. #4673 adds the same `APIReader` field and `apiReader()` helper to `RestoreJobReconciler`, for its finished-job guard. Whichever of the two lands second drops its own copy when it rebases. ### The PV's reclaim policy was overwritten without being recorded `renamePVC` switches the PV to `Retain` so it survives the deletion of the original PVC (`main:1289-1293`), and the policy it had was lost. I looked for a path in the controller that releases or cleans up `-orig` PVCs, and there is none: `cleanupOnDelete` on a Velero RestoreJob removes only the Velero `Restore` and the resource-modifier ConfigMaps, and no code deletes an `-orig` PVC or touches its PV again. A PV that was `Delete` therefore stays `Retain` for good, and its volume leaks when an operator deletes the `-orig` PVC. The original policy is now recorded in `backups.cozystack.io/original-reclaim-policy`, on the PV and on the `-orig` PVC, following the `backups.cozystack.io/` prefix the controller already uses for `underlying-resources`. A value already present on the PV is kept, as it predates any rename. Since there is no release path to restore it from, `examples/backups/vmi/scenario-user-restore.md` gains a section for the operator who deletes an `-orig` PVC: set the recorded policy back on the PV first, with a snippet that leaves a PV without the annotation on `Retain`. Restoring the policy from the controller itself, for instance once the Velero Restore has completed and the PV is `Bound` to the `-orig` PVC, would close the leak without the operator. I did not do it, because it changes what deleting an `-orig` PVC destroys, from nothing to the volume; that is a maintainer call, and the annotation is what it would read. ### How this fits with #3427 #3427 is complementary, and the two do not touch the same code: it changes `collectUnderlyingResources`, this PR changes `prepareForRestore` and `renamePVC`. Both append tests to `velerostrategy_controller_test.go`, so whichever lands second may need a trivial rebase. - #3427 makes a standalone VMDisk report its own disk, which sends VMDisk in-place restores through this same `renamePVC`. Once both land, the VMDisk path gets the same guard and the same annotation. - Its `examples/backups/{vmi,vmdisk}/cleanup.sh` and the Chainsaw `finally` reclaim each `-orig` PVC by patching its PV to `Delete` before deleting the PVC. That works around, in the e2e teardown, the leak described above. It is correct on the LINSTOR classes, which are `Delete`, but it would destroy a volume whose class was `Retain`. With this PR the scripts can read `backups.cozystack.io/original-reclaim-policy` instead of hard-coding `Delete`. I have not changed them here, since they only exist on that branch. - Its round-trips use fixed RestoreJob names and assert that a `<pvc>-orig-*` PVC exists after the restore. If a failed teardown leaves one behind, today the next run skips the rename and the assertion still passes on the stale PVC; with this PR the RestoreJob fails and names it. - Its cleanup comments describe the `-orig` PVC as a bare ObjectMeta with no labels or owner. That stays true: this PR adds annotations only, so its name-based lookup is unaffected. ### Tests New tests in `internal/backupcontroller/velerostrategy_controller_test.go`, run through `prepareForRestore` with the DataVolume in the dynamic fake client: - `TestPrepareForRestore_KeepOriginalPVC_RefusesForeignOrigPVC`: an `-orig` PVC without the annotation, and one carrying another RestoreJob's UID, both fail the restore with the PVC named, and leave the DataVolume, the live PVC, the PV and the foreign PVC untouched. - `TestPrepareForRestore_KeepOriginalPVC_ResumesOwnRename`: a rename interrupted after the `-orig` PVC was created completes on the next pass, and a second pass changes nothing. - `TestPrepareForRestore_KeepOriginalPVC_FailedRenameKeepsDataVolume`: a rename that fails for another reason (missing PV) requeues the restore without failing it and keeps the DataVolume, and once the PV exists the retry completes the rename. - `TestPrepareForRestore_KeepOriginalPVC_CacheLagsBehindWrites`: the reconciler's cached client serves every read from the objects as they were seeded, through an `interceptor` over the fake client, as an informer that has not caught up with this reconcile's writes would, while `APIReader` sees them. The rename must complete. - `TestRenamePVC_RecordsOriginalReclaimPolicy`: a `Delete` PV and a `Retain` PV both end up `Retain`, with their original policy recorded on the PV and on the `-orig` PVC. With the controller reverted to `main` (only the two annotation constants kept so the tests compile), all four fail: `prepareForRestore() = ready true, nil error` with `DataVolume was deleted although the disk could not be kept` for both foreign cases and for the failed rename, `original PVC still exists` and the PV still bound to it for the interrupted rename, and an empty `original-reclaim-policy` on the PV and on the PVC. For the cache race, the first commit alone fails `CacheLagsBehindWrites` with the lab's error, `failed to keep PVC vm-disk-ubuntu-source: failed to get -orig PVC vm-disk-ubuntu-source-orig-de3e: persistentvolumeclaims "vm-disk-ubuntu-source-orig-de3e" not found`, and `FailedRenameKeepsDataVolume` with a terminal error instead of a requeue. On `main` the race test reproduces the lab outcome: `original PVC still exists`, and the PV still claimed by it. With both commits, `go test -count=1 ./internal/backupcontroller/...` passes. No API type, CRD, chart or values file changes, so there is nothing to regenerate. ### Screenshots N/A, no UI change. ### Downstream repositories Walked the trigger map against the diff: the restore preparation in `internal/backupcontroller/velerostrategy_controller.go`, its tests, and `examples/backups/vmi/scenario-user-restore.md`. No CRD, ApplicationDefinition, values, schema, enum, namespace, label, metric or tooling change, so terraform-provider-cozystack, ansible-cozystack, ccp, talm, cozyhr, cozy-proxy, cozystack-telemetry-server, external-apps-example, examples and community are not reached. `RestoreJob.spec.options` stays opaque to the provider. cozystack/website is reached: `content/en/docs/next/virtualization/backup-and-recovery.md` (and each released copy) tells users to delete the `*-orig-*` PVCs "to free up storage". A PV left on `Retain` frees nothing, and the page should explain the recorded policy and the new failure for a reused RestoreJob name. No follow-up exists there yet, so the box stays empty until one is filed and linked. - [ ] 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): an in-place VM restore with keepOriginalPVC now fails, before the VM is halted, when the <disk>-orig-<hash> PVC it would keep the disk under already exists and was not created by this RestoreJob (for instance one left by a deleted RestoreJob of the same name), instead of skipping the rename and restoring without keeping the current disk. Any other failure to keep a disk, including a race in which the rename read its just-created -orig PVC back through the cache and failed, now holds the restore back and retries it before its DataVolume is deleted. Previously such a restore reported success without restoring the disk. The PV's reclaim policy before the switch to Retain is recorded in the backups.cozystack.io/original-reclaim-policy annotation on the PV and the -orig PVC; set it back on the PV before deleting the -orig PVC, or its volume is not reclaimed. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * In-place restores that preserve original PVCs can now resume safely after interruptions, including when reads lag behind recent changes. * Restores stop before suspending the VM if a conflicting `-orig` PVC already exists. PVC rename failures can be retried, while conflicting names remain fatal. * Original PV reclaim policies are recorded and restored before preserved PVCs are deleted. * **Documentation** * Updated the restore guide with disk-reclamation steps and guidance for resolving leftover PVC name conflicts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ae738e6047 |
test(backups): pass the flavor in the major-version test calls
Two tests that came with the PostgreSQL major-version check call createCNPGBackupArtifact and marshalCNPGBackupSnapshot with the argument lists from before the flavor parameter was added, so the backupcontroller test package does not compile on main. Both calls take the default flavor the other tests in the file pass. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Mattia Eleuteri
|
5956b459fb |
fix(backups): give each Postgres instance its own default WAL prefix
The default CNPG strategy archived a Postgres app under <namespace>-<name>, inside a destinationPath that is also keyed by namespace and name, and the driver fell back to the Cluster name when a strategy set no serverName. Deleting the app does not empty the bucket, so an app recreated under the same name archived its WALs and base backups into its predecessor's stream: both start on timeline 1, and a later restore of the old archive could pick the new cluster's base backup or WAL segments. barman-cloud refuses a non-empty archive only while .check-empty-wal-archive exists in PGDATA. CNPG removes that file as soon as ContinuousArchiving turns True, and with no plugin attached archive_command is a successful no-op, so the file is gone minutes after initdb, before the first BackupJob attaches the plugin. Suffix the default serverName with the application UID, in the strategy template and in the driver's fallback. The template guards the UID with `with`, so an object without one renders the previous name rather than "<no value>". A Cluster that already has the plugin keeps its serverName, so only clusters never backed up take the new prefix, and restores keep reading the source prefix from driverMetadata. Signed-off-by: Mattia Eleuteri <mattia@hidora.io> Assisted-by: LLM |