mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 22:13:18 +09:00
4e5b76f2820cf805ced8ced4010fe1883ccd7d70
618
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. ``` |
||
|
IvanHunters
|
a444ac4498 |
Merge branch 'main' into fix/tap-pin-digest
Reconcile the tap digest-pin change with #5071 (no-install-on-connect), now merged into main. Keep this branch's resetTapPinState, the verify-before-pull Trust text, and the index-README trust model, while adopting #5071's wording that installing (not tapping) runs charts and dropping the stale "re-registration" phrasing from the shared tap.go and rest.go comments. Conflicts resolved in cmd/cozypkg/cmd/tap.go, pkg/registry/core/tap/rest.go, and hack/packages-index/README.md. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Andrey Kolkov
|
07301c562c |
fix(workloadmonitor): stop doubling the bucket- prefix in bucket Workload names (#5083)
<!-- 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 Fixes #5014. A bucket's `Workload` was named `"bucket-" + <BucketClaim name>`, and a Bucket application names its claim after its release, which already starts with `bucket-`. A bucket an operator knows as `cozy-backups` therefore appeared as `bucket-bucket-cozy-backups`, and that string is the `workload` of the bucket's consumer in every billing usage report. - **Naming.** The `bucket-` prefix is now added only when the claim name does not already carry it. A Bucket application's claim `bucket-cozy-backups` gives the `Workload` `bucket-cozy-backups`; a claim without the prefix, such as `my-bucket`, still gives `bucket-my-bucket`. In the tree today every monitored BucketClaim comes from the Bucket application (Harbor's registry claim carries no labels its monitors select, so it has no `Workload` on either side of this change). - **Migration of existing Workloads.** Nothing garbage-collects bucket `Workload`s by name: they are owned by their BucketClaim and live as long as it does. Without a migration the old `bucket-bucket-…` object would stay next to the new one and the bucket would be metered twice. When the new `Workload` is reconciled, the controller looks up the legacy name and, only if the same BucketClaim (matched by UID) is its sole owner, uses its recorded sizes as the starting point and then deletes it, with UID and resourceVersion preconditions so a `Workload` that took over the name in the meantime survives. If the legacy `Workload` has other owners as well, it is another bucket's renamed `Workload` that a rolled-back controller attached this claim to: the controller takes neither its sizes nor the object and only drops its own owner reference. A legacy `Workload` with no owners left (a claim deleted with `--cascade=orphan` and recreated) that still carries this monitor's label is deleted the same way, without taking its sizes. Sizes already recorded at the new name win over the legacy ones only when the claim already owned that `Workload`. Carrying the sizes over matters because the controller deliberately keeps the last known size when the object-store metrics are unavailable; a fresh `Workload` would otherwise report no size until the next successful poll, which billing reads as an empty bucket. - **Never taking over another claim's Workload.** With Bucket applications `x` and `bucket-x` in one namespace, the legacy name of the first (`bucket-bucket-x`) is the new name of the second. The controller never takes over a `Workload` that has owners when the claim is not one of them (in practice another BucketClaim). While the new name is held that way, it keeps metering the bucket under its legacy name, without migrating, and emits a `BucketWorkloadNameTaken` warning on the monitor. For Bucket applications the other claim renames its `Workload` away on its own reconcile and the migration completes on the next pass. Only when the legacy name is held as well — which for a claim without the prefix, whose two names coincide, means a claim `x` outside the Bucket application next to the application `x` — is the bucket skipped, and the warning says it is not metered. Without this, whichever claim reconciled first decided whether the second bucket ended up with the first one's size or with no size at all. The same rule keeps two claims that map to one name (`x` and `bucket-x`) from sharing a `Workload`. **Rollback.** The migration is one-way. A controller rolled back to a previous version recreates `bucket-bucket-<name>` next to the renamed `bucket-<name>`, which its monitor keeps alive, so the bucket is metered twice until the `bucket-<name>` Workloads of Bucket applications are deleted by hand. Upgrading again after such a rollback converges without deleting another bucket's `Workload` (see above). **Known limitation.** Taking over an ownerless `Workload` keeps the sizes recorded on it as the last known ones, as the controller always has. If such an object was left behind by another bucket (for example `kubectl delete bucketclaim --cascade=orphan`), its sizes count for the new claim until the next successful size poll. **Effect on billing, worth knowing before the upgrade:** `Workload` names are the identity billing integrates over. A bucket's consumer changes name once, at the upgrade, so a usage report whose window spans the upgrade lists the bucket twice: under the old name up to the switch and under the new one after it. The totals are right; the split is cosmetic. Reports for windows entirely before or after the upgrade are unaffected. Tests (`internal/controller/workloadmonitor_controller_test.go`): - `TestBucketWorkloadName`: the naming rule for a Bucket application claim, a plain claim and a Harbor claim. - `TestReconcileBucketApp_NoDoublePrefix`: a Bucket application gets `bucket-cozy-backups` and no `bucket-bucket-cozy-backups`. - `TestReconcileBucketApp_ReplacesLegacyWorkload`: an existing `bucket-bucket-cozy-backups` owned by the claim is deleted, and its recorded logical and physical sizes (no metrics endpoint in the test) are carried over to the new `Workload`. - `TestReconcileBucketApp_KeepsLegacyNameOwnedByAnotherClaim`: a `Workload` under the legacy name owned by a different claim is left alone and its size is not taken. - `TestReconcileBucketApps_LegacyNameIsAnotherAppsNewName`: Bucket applications `x` and `bucket-x` with their legacy `Workload`s, reconciled in both orders, end with `bucket-x` and `bucket-bucket-x`, each with one owner and its own size, and the legacy `bucket-bucket-bucket-x` gone. - `TestReconcileBucketClaim_DoesNotAdoptAnotherClaimsWorkload`: a claim whose name is held by another claim's `Workload` leaves it untouched, emits `BucketWorkloadNameTaken` and is metered under its legacy name. - `TestReconcileBucketClaim_KeepsMeteringUnderLegacyNameWhileNewNameIsHeld`: the legacy `Workload` keeps being updated, with its size and owner, across repeated reconciles while the new name is held. - `TestReconcileBucketClaim_UnprefixedClaimWithHeldNameIsSkipped` and `TestReconcileBucketClaim_BothNamesHeldIsSkipped`: when no free name is left, nothing is taken over and the event says the bucket is not metered. - `TestReconcileBucketClaim_UnprefixedClaimKeepsItsWorkload`: for a claim without the prefix, where the legacy and the new name coincide, its own `Workload` and size survive repeated reconciles. - `TestReconcileBucketApp_RecordedSizeWinsOverLegacy`: a size already recorded on the new `Workload` takes precedence over the legacy one. - `TestReconcileBucketClaim_UpdatesItsOwnWorkload`: a claim's own `Workload`, with its owner reference carrying the `BucketClaim` kind as a live cluster's cache records it, keeps being updated and raises no warning (the fake client leaves that kind empty on objects the controller creates, so the other tests do not exercise this). - `TestReconcileBucketClaim_AdoptsOwnerlessWorkload`: a `Workload` that lost all owner references is taken over by the claim whose name it carries, rather than blocking that name forever. - `TestReconcileBucketApp_ReleasesSharedLegacyWorkload`: a legacy `Workload` that another claim also owns (the rollback case) keeps its size and its other owner; only this claim's reference is dropped, and its size is not carried over. - `TestReconcileBucketApp_KeepsWorkloadThatReplacedLegacy`: a `Workload` that replaces the legacy one under the same name between the lookup and the delete survives. - `TestReconcileBucketApp_RemovesOrphanedLegacyWorkload`, `TestReconcileBucketApp_KeepsOrphanOfAnotherMonitor`: an ownerless legacy `Workload` with this monitor's label is removed without carrying its sizes; one labelled for another monitor is left alone. - `TestReconcileBucketApp_LegacySizeWinsOverOwnerlessNewWorkload`: a stale size on an ownerless `Workload` at the new name loses to the migrating legacy `Workload`'s size. - `TestReconcileBucketApp_LegacyAlreadyGone`: a legacy `Workload` that disappears between the lookup and the delete (NotFound) does not fail the per-claim reconcile, and the size is still carried over. Reverting the naming rule, the legacy delete, the size carry-over, the owner check, the refusal to adopt, the same-name guard, the size precedence ignoring NotFound on the legacy delete, the own-claim match in the adoption check, the ownerless case, the physical size carry-over, the sole-owner rule for legacy Workloads, the delete preconditions, the legacy-name fallback or its own held check, the ownerless-legacy rule or the size precedence on takeover each fails one of these. `go test ./internal/... -count=1` and `go vet ./internal/controller/` pass. ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff, which touches only `internal/controller/workloadmonitor_controller.go` and its test: no package, values, schema, CRD, variant, `hack/` tooling, telemetry metric or cozy-proxy contract changes. - [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(workloadmonitor): Workloads of Bucket applications are no longer named with a doubled "bucket-" prefix (bucket-bucket-<name> becomes bucket-<name>). Existing Workloads are replaced on upgrade, keeping their recorded size; billing usage reports whose window spans the upgrade list such a bucket once under each name. Rolling the controller back afterwards requires deleting the bucket-<name> Workloads of Bucket applications, or the buckets are metered twice. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Bucket workloads now use a consistent name without duplicating the `bucket-` prefix. * Existing workload size data is retained when workloads are renamed or updated. * Workloads owned by another claim are preserved. When a naming conflict occurs, metering continues under the legacy name when possible; if both names are unavailable, metering is skipped and a warning is emitted. * Legacy workloads are cleaned up or updated according to their ownership, helping prevent duplicate or orphaned workloads. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
372c85575f |
fix(controller): carry healthCheckExprs and waitStrategy to existing releases (#5087)
## What this PR does This PR syncs `healthCheckExprs` and `waitStrategy` from ApplicationDefinition to HelmReleases that already exist. cozystack-api writes both fields only when it builds a release, and the definition reconciler copied chartRef, valuesFrom, disable-wait setting and labels but not these two, so release created before its definition got expressions keeps waiting on objects the definition already exempts. Most visible case is vm-disk. Every disk created before the DataVolume `current: "true"` expression was added still waits on its DataVolume, so after platform upgrade an existing disk with import still in progress fails the upgrade after 10 minutes with `timeout waiting for: [DataVolume/<namespace>/vm-disk-<name> status: 'InProgress']`, then retries forever and holds one of 5 workers of its helm-controller shard. Reconciler now derives both fields same way the API does (`config.ResolveWaitStrategy`) and syncs them in both directions, so existing release ends up same as freshly written one. On current definitions only vm-disk (DataVolume) and postgres (KEDA ScaledObject) set expressions, both `current: "true"`, so this can only relax waits. Releases of other kinds have nothing to sync and are not rewritten. HelmReleases rendered by another chart (tenant modules like Etcd, Ingress, Monitoring) carry Flux origin label `helm.toolkit.fluxcd.io/name` and are skipped, their gate comes from their own chart. Their chartRef is still synced as before. Failed update was logged and dropped before, and nothing requeues the definition for hours after that. Now errors are returned, so a conflict is retried. Unit tests cover both sync directions, `waitStrategy: legacy` without expressions, no rewrite of in-sync release (nil vs empty slice too), chart-rendered and other-kind releases left alone, and update failure returned as error. Fixes #4409 ### 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(controller): existing HelmReleases now get `healthCheckExprs` and `waitStrategy` from their ApplicationDefinition. vm-disk and postgres releases created before these expressions were added stop waiting on DataVolume and KEDA ScaledObject during upgrades. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Helm releases now synchronize health-check expressions and wait strategies configured in their application definition, including when those values are removed or no expressions are specified. * Releases with chart-rendered health-check settings retain those settings, and releases outside the matching application kind are unaffected. * Update failures are now reported during reconciliation, while updates to other releases continue. Reconciliation can retry after a failed update. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
7dc5c0b5e6 |
fix(marketplace): do not install a connected repository's components on connect (#5071)
## What this PR does Connecting a marketplace repository materialized its `PackageSource`s and then auto-created a tap-managed `Package` per source. The Package reconciler installs a `HelmRelease` for every install-marked component of the default variant, and Flux runs those under its `flux` service account, which is bound to `cluster-admin` with no impersonation (`--default-service-account` is not set, and neither `internal/operator` nor `pkg/registry/apps` set `serviceAccountName` on the HelmRelease). A publisher's install chart therefore ran as `cluster-admin` on connect, with no operator step, contradicting the documented behaviour that tapping installs nothing. The `privileged` flag is a prompt to the admin and does not protect against a publisher acting in bad faith. This removes the on-connect auto-registration (`ensureRegistration`). Connecting now only materializes `PackageSource`s. Installing an app is an explicit `cozypkg add`, which becomes the sole creator of a `Package`; every other `Package` reference in the operator is a read or a watch. Removing the call also fixes the second effect of the same path: a `Package` the admin removed with `cozypkg del` is no longer re-created on the next source revision. A second commit reconciles the texts this behaviour change made wrong: they promised that tapping registers a repository's apps in the catalog and runs its charts. Reworded the `cozypkg tap`/`untap` help, the tap success output (it now prints a `cozypkg add <name>` line per materialized PackageSource), the `cozypkg init` scaffold README, the packages-index trust model, and the dashboard Repositories banner and per-repository empty state. Accepted consequence: a connected repository's apps appear in the catalog only after an admin runs `cozypkg add`, since the dashboard catalog is computed from `ApplicationDefinition`s. Install from a connected repository is CLI-only for now; the console connects and disconnects repositories but does not create a `Package`. Populating the catalog from `PackageSource` metadata without an install is the long-term shape in #4359 and is out of scope here. The privileged-confirmation gaps #4358 and #4359 stay open. Upgrade note: a `Package` that Cozystack 1.7.0 auto-created on connect stays after the upgrade, and the privileged-confirmation gate still applies to it, so this is not a regression for existing installs. For a newly connected repository no tap-labelled `Package` is created, so the gate does not come into play on that path. An operator who does not want a 1.7.0 auto-created package can remove it with `cozypkg del`. Addresses #4978. ### Screenshots Dashboard text-only change (Repositories banner and per-repository empty state). No layout change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The diff is operator logic plus CLI/dashboard/doc text; it matches no concrete trigger (no package, API, schema, tooling, annotation, metric or commit-convention change). The dashboard console is vendored in this repo (`packages/system/dashboard`), so its text change travels in this PR. Leaving every box empty rather than ticking "No downstream repository is affected". ### Release note ```release-note fix(marketplace): connecting a marketplace repository no longer installs its components. A connected repository's apps are materialized as PackageSources but not installed until an explicit `cozypkg add`; install from a connected repository is CLI-only for now. Previously, connecting auto-created a Package whose install-marked components were deployed by Flux under its cluster-admin service account. A Package that Cozystack 1.7.0 auto-created on connect stays after upgrade and can be removed with `cozypkg del`. ``` |
||
|
IvanHunters
|
09b0688abd |
fix(marketplace): fail closed on an unparseable verified digest, and cover the generation gate
A verified cosign signature whose manifest digest could not be parsed fell back to an unpinned tag pull, which reopens the tag-mutation window the verification was meant to close. Return an error instead. Add a test that isolates the status.observedGeneration == metadata.generation conjunct of artifactReady; the existing tests exercised only the tag-prefix conjunct, so a mutation of the generation check went uncaught. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
7494d1fad2 |
fix(marketplace): pin a tap digest only from a current-generation artifact
The materializer pinned whatever digest status.artifact reported, even when the artifact still described the previous reference. On a re-connect to a new tag, source-controller has not re-resolved yet, so the operator materialized and pinned the old tag's content (issue #4979). Gate materialization and pinning on the artifact reflecting the current spec: status.observedGeneration equals metadata.generation, and for a tag-only source a revision that belongs to that tag (<tag>@<digest>). A tap connected before digest pinning existed already has its materialized-revision equal to the artifact revision, so Reconcile short-circuited and never pinned, letting it follow the tag one more time. Pin the current digest before that early return. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
2f39e304f0 |
fix(marketplace): reconcile tap texts with no-install-on-connect
Removing on-connect auto-registration made several user-facing and doc texts wrong: they promised that tapping registers a repository's apps in the catalog and runs its charts. Tapping now installs nothing, so the texts told operators to wait for apps that never appear. Reword the `cozypkg tap`/`untap` help, the tap success output (now printing a `cozypkg add <name>` line per materialized PackageSource), the `cozypkg init` scaffold README, the packages-index trust model, the dashboard Repositories banner and per-repository empty state, and the stale re-registration code comments. No behaviour changes beyond the added `cozypkg add` hint in the tap output. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Myasnikov Daniil
|
8d8179f20a |
fix(controller): carry healthCheckExprs and waitStrategy to existing releases
cozystack-api writes healthCheckExprs and the resolved wait strategy from the ApplicationDefinition only when it builds a HelmRelease, and the definition reconciler that keeps existing releases in step copied neither. A release created before its definition exempted an object kept waiting on that object at every upgrade. For vm-disk that is every disk created before the DataVolume expression was added: a disk whose import is still running after a platform upgrade fails the upgrade at the timeout and, under RetryOnFailure, retries forever while holding a helm-controller worker. The reconciler now derives both fields the way the API does, in both directions, so an existing release ends up with what a fresh write would produce. On the shipped definitions this only relaxes waits: the vm-disk DataVolume and the postgres KEDA ScaledObject. A release that another chart renders carries Flux's origin label and keeps the gate its chart sets. A failed update used to be logged and dropped, and nothing else re-enqueues the definition for hours, so that release stayed out of sync. The errors are now returned and the definition is retried. Fixes https://github.com/cozystack/cozystack/issues/4409 Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Andrey Kolkov
|
a6c14f6834 |
fix(workloadmonitor): migrate ownerless legacy bucket Workloads and prefer legacy sizes on takeover
A BucketClaim deleted with --cascade=orphan and recreated leaves its bucket-bucket-<name> Workload without owners. The migration recognised a legacy Workload only by the claim's UID among its owners, so that object stayed next to the new one with a frozen size and the bucket was metered twice. Treat an ownerless legacy Workload that carries the monitor's label as the claim's: delete it with the same preconditions, but take no sizes from it, since they may predate the claim. The size carry-over kept whatever the Workload at the new name had recorded. When that Workload was not yet owned by the claim, an ownerless leftover, its sizes were not written for this bucket, yet they won over the migrating legacy Workload's. Prefer the recorded sizes only when the claim already owned the Workload. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
36ce21db79 |
fix(workloadmonitor): keep metering a bucket under its legacy name while the new one is taken
When the new Workload name was held by another owner the claim was skipped altogether, so its legacy Workload stopped being refreshed and billing kept reading the last size it had recorded. That happens briefly with Bucket applications "x" and "bucket-x" until the first one has migrated, and for good with a claim "x" outside the Bucket application next to the application "x", which map to the same name. Keep reconciling the bucket under its legacy name instead, without migrating, until the new name is free. Skip it only when the legacy name is held by another owner as well, which is the only option left for a claim without the prefix, whose two names coincide; the warning event then says that the bucket is not metered. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
fd69a8fbd6 |
fix(workloadmonitor): release, not delete, a legacy bucket Workload shared with another claim
A controller rolled back to the doubled prefix runs CreateOrUpdate on bucket-bucket-x, which by then is the renamed Workload of the Bucket application "bucket-x", and appends claim bucket-x as a second owner. After the next upgrade claim bucket-x took that object for its own legacy Workload: it carried the other bucket's size over and deleted the object, and the other bucket's Workload came back without a size. Treat a legacy Workload as the claim's own only when the claim is its sole owner. When it has other owners, take neither its sizes nor the object: drop the claim's owner reference and leave the rest. Delete the legacy Workload with UID and resourceVersion preconditions, so a Workload that took over the name between the lookup and the delete survives; a conflict is not an error and the next reconcile retries. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
f9b7e936d7 |
refactor(workloadmonitor): leave alone any bucket Workload the claim does not own
The adoption check only refused a Workload owned by another BucketClaim, and the kind test in it had no case that could tell it apart: a bucket Workload only ever has BucketClaim owners. Refuse any Workload that has owners when the claim is not among them, which needs no kind at all, and keep taking over one that has lost every owner reference, since nothing else would ever release its name. Pin the ownerless case. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
54a916e31f |
test(workloadmonitor): pin that a claim keeps updating its own bucket Workload
On a live cluster the cache records the BucketClaim kind in a Workload's owner reference, so a claim's own Workload passes through the "owned by another claim" check on every reconcile, and only the own-UID match keeps it from being skipped. The fake client leaves that kind empty, so no test exercised it. Seed a Workload whose owner reference carries the kind and the claim's UID, and expect it to be updated with no warning event. Pin the physical size carry-over alongside the logical one, and word the warning event so it holds when two claims map to one name for good. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
myasnikovdaniil
|
a26c441d78 |
fix(vm-disk): let CDI adopt the PVC a Velero restore recreates (#4955)
## What this PR does After a Velero restore of a VMDisk or of a VMInstance (`RestoreJob` with the Velero strategy), the disk's HelmRelease fails on its next upgrade and stays failed: ``` Helm upgrade failed for release <ns>/vm-disk-<name> with chart vm-disk@...: failed to create resource: admission webhook "datavolume-validate.cdi.kubevirt.io" denied the request: Destination PVC <ns>/vm-disk-<name> already exists ``` The VMInstance release that references the disk then fails in turn (`Specified disk not exists in cluster`). Seen on a v1.6.5 cluster with CDI v1.66.1, where each failed upgrade is retried and the release history of one such disk had grown past 46,000 revisions. ### Cause The restore controller deletes the disk's DataVolume (`prepareForRestore`) and Velero restores the PVC under the same name, with no owner. The restored release matches its chart and values, so Flux does not upgrade it and the DataVolume is not recreated right away; the next Helm upgrade (a platform update, an edit of the VMDisk) does recreate it, and CDI's webhook refuses it. In CDI v1.66.1 the webhook admits a DataVolume over an existing claim only when `ClaimMayExistBeforeDataVolume` is true (`pkg/controller/common/util.go`): either the PVC carries `cdi.kubevirt.io/storage.populatedFor: <dv name>`, or `AllowClaimAdoption` returns true, which reads `cdi.kubevirt.io/allowClaimAdoption` **from the DataVolume** and otherwise falls back to the `DataVolumeClaimAdoption` feature gate. The Velero strategy's resource modifiers (`createResourceModifiersConfigMap`) put `allowClaimAdoption=true` on the restored **PVC**, where CDI never reads it, and the `CDI` CR in `packages/system/kubevirt-cdi` enables neither the gate nor anything equivalent. Migration 29 creates its PVCs with the same PVC-side annotation. ### Fix The vm-disk chart sets `cdi.kubevirt.io/allowClaimAdoption: "true"` on the DataVolume it renders, on the creation path and on the existing-DataVolume path alike, so the annotation does not flap between upgrades. - A fresh disk is unaffected: no PVC exists when the DataVolume is admitted, and the PVC CDI then creates carries `cdi.kubevirt.io/createdForDataVolume: <dv uid>`, for which `AllowClaimAdoption` returns false, so CDI imports, clones or uploads as before. - An existing DataVolume already controls its PVC, so the annotation changes nothing there; a Helm upgrade only adds it to the DataVolume's metadata, which CDI allows (only the spec is immutable). - After a restore, the webhook admits the recreated DataVolume and the DataVolume controller adopts the claim: `pvcRequiresWork` is false, it takes controller ownership of the PVC and the DataVolume goes `Succeeded` without populating it again, so the restored data is kept. Checked against a v1.6.5 cluster carrying the broken state, by rendering the chart for an affected disk and running `kubectl create --dry-run=server`: the DataVolume without the annotation is refused with the error above, the same one with the annotation is admitted. On that cluster two affected disks were later recovered through the claim-adoption path (the same `AllowClaimAdoption` check): the PVC kept its uid, became controlled by the new DataVolume, and the DataVolume reached `Succeeded` with no import. ### Alternatives considered - **Enable the `DataVolumeClaimAdoption` feature gate in the `CDI` CR.** It works too, but for every DataVolume in the cluster (tenant Kubernetes worker disks, golden images, KubeVirt `dataVolumeTemplates`), each of which would then silently take over any unowned PVC of its name. The per-DataVolume annotation keeps the behaviour to the one chart whose DataVolumes are expected to meet a restored claim, and CDI has said it means to retire the gate (kubevirt/containerized-data-importer#3480) while the annotation is checked first and independently of it. - **Stamp `cdi.kubevirt.io/storage.populatedFor` on the restored PVC** from the resource modifiers. That would also pass the webhook, but only for disks restored by the Velero strategy, not for claims created by migration 29 or by hand, and it would need a controller image change where a chart change suffices. The PVC-side `allowClaimAdoption` rule in `createResourceModifiersConfigMap` (and the one in migration 29) is inert under CDI v1.66.1. This PR leaves the rule and its unit-test assertions in place, and corrects the comments that credited it with the adoption (the collector and `createResourceModifiersConfigMap` in `velerostrategy_controller.go`, and step 5 of `examples/backups/vmi/scenario-user-restore.md`): they now say CDI adopts the restored PVC through the annotation the vm-disk chart sets on the DataVolume. The backports carry only the template and the unittest suite, so these comment and doc edits do not reach them. One behaviour change to be aware of: a PVC named `vm-disk-<name>` left behind without an owner (a restored disk deleted before this fix, whose PVC Velero restored without an owner) is now adopted by a new VMDisk of the same name, with its old contents, instead of blocking that VMDisk forever. That PVC lives in the same tenant namespace. ### Tests - `packages/apps/vm-disk/tests/dv_claim_adoption_test.yaml`: the post-restore state (a restored PVC, no DataVolume) renders the DataVolume through the creation path with the annotation, and the existing-DataVolume path keeps it. Both fail with the template change reverted. `helm unittest packages/apps/vm-disk`: 116 passed. - `examples/backups/vmdisk/05-restore-in-place.sh` (run by the `vmdisk-2-backup-roundtrip` Chainsaw suite): after the restore, the HelmRelease was already reported Ready because no upgrade ran, which is why CI did not catch this. The script now forces a Helm upgrade (`reconcile.fluxcd.io/forceAt` + `requestedAt`), then requires the release Ready, the DataVolume `Succeeded`, and the PVC to keep its restored uid and be controlled by the DataVolume. ### Backports Opened by hand as #4956 (`release-1.6`) and #4957 (`release-1.7`) rather than through the `kind/backport` labels: `examples/backups/vmdisk/` does not exist on either branch, so the bot's cherry-pick would conflict. They carry the template change and the helm unittest suite. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff: a vm-disk template change (no `values.yaml`, schema, `README.md` or `Chart.yaml` change), a new helm unittest file, and a backup walkthrough script. No app, field, default, CRD, platform value or tooling changes, and no documented workaround on the website is made obsolete (the only mention of `allowClaimAdoption` there is in the archived v0 docs, about a different manual procedure). - [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(vm-disk): a VMDisk restored by Velero, alone or as part of a VMInstance, no longer fails its next upgrade with "Destination PVC ... already exists": the DataVolume now carries cdi.kubevirt.io/allowClaimAdoption, so CDI adopts the restored PVC and keeps its data. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Improved in-place VM disk restores so restored storage is correctly adopted when the disk resource is recreated. * The restore process now waits for the disk to become ready and reports an error if it fails or the restored storage is not retained. * Clarified the adoption behavior in restore documentation and ensured recreated disks use the required adoption setting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
730135903f |
test(workloadmonitor): pin that a vanished legacy bucket Workload is not an error
The legacy Workload can disappear between the lookup and the delete, for example through owner garbage collection, and the delete ignores NotFound for that reason. Reconcile only logs per-claim errors, so check reconcileBucketClaimForMonitor's own result with a delete that reports NotFound. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
a51ad0ed54 |
fix(workloadmonitor): never adopt a bucket Workload owned by another claim
With Bucket applications "x" and "bucket-x" in one namespace, the legacy name of the first, bucket-bucket-x, is the new name of the second. When the second reconciled first, CreateOrUpdate took over the first one's legacy Workload, which already carried bucket x's size, and appended a second owner. The first claim then found its own UID among the owners, treated the object as its legacy Workload and deleted it, and the second bucket's Workload came back without a size. Leave a Workload alone while it is owned by a different BucketClaim, log it and emit a BucketWorkloadNameTaken warning on the monitor. In the case above the other claim renames its Workload away on its own reconcile, and the name is free on the next pass. The same refusal keeps two claims that map to one name, such as "x" and "bucket-x", from sharing one Workload. Pin the edge cases the migration depends on: the reconcile order above in both directions, a claim without the prefix keeping its own Workload, and a recorded size taking precedence over the legacy one. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
164b5ec859 |
fix(workloadmonitor): stop doubling the bucket- prefix in bucket Workload names
A bucket's Workload was named "bucket-" + the BucketClaim name, and a Bucket application names its claim after its release, which already starts with "bucket-". A bucket an operator knows as cozy-backups showed up as bucket-bucket-cozy-backups in every billing usage report. Add the prefix only when the claim name does not carry it. Claims from other charts, such as Harbor's <release>-registry, keep their names. Existing Workloads under the doubled name are owned by their claim and would otherwise live on next to the new one, metering the bucket twice. When the replacement is reconciled, take the legacy Workload's recorded sizes as the starting point, so the rename leaves no gap in the metered size, and delete the legacy Workload. Only a Workload owned by the same BucketClaim is touched. Billing sees the renamed Workload as a new consumer: a usage report spanning the upgrade lists the bucket once under each name. Fixes #5014 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
IvanHunters
|
c837838cc2 |
fix(marketplace): pin a tag-following tap source to the resolved digest
A tapped repository followed a mutable tag: the OCIRepository carried only spec.ref.tag with a 5m interval, so Flux re-pulled whatever the tag pointed at, and a moved tag reached running tenant application instances within the interval. Whoever controlled the tag controlled what ran in the cluster. The materializer now pins the source to the digest Flux resolved for the artifact it just verified: it reads the manifest digest from status.artifact.revision (source-controller formats an OCI tag revision as '<tag>@<digest>') and writes it to spec.ref.digest, which Flux resolves ahead of the tag. The tag is kept informational. Moving the tag upstream no longer changes what the cluster pulls; an explicit re-connect is required to move the pin. A user-supplied explicit digest is never rewritten, and a revision without a well-formed digest leaves the source following its tag rather than writing a malformed reference. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
3fa534f3cf |
fix(marketplace): do not install a connected repository's components on connect
Connecting a marketplace repository materialized its PackageSources and then auto-created a tap-managed Package per source. The Package reconciler installed a HelmRelease for every install-marked component of the default variant, which Flux runs under its cluster-admin service account with no impersonation. A publisher's install chart therefore ran as cluster-admin on connect, with no operator step, contradicting the documented behaviour that tapping installs nothing. Remove the on-connect auto-registration. Connecting now only materializes PackageSources; installing an app is an explicit cozypkg add (or dashboard action), which is the sole remaining creator of a Package. This also stops a Package the admin removed with cozypkg del from being re-created on the next source revision. Catalog population from PackageSource metadata without an install is left to the long-term design in #4359, and the privileged-confirmation gaps #4358 and #4359 stay open. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
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> |