cozystack

0 Fork 0

mirror of https://github.com/cozystack/cozystack.git synced 2026-11-09 05:25:14 +09:00

Author	SHA1	Message	Date
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 -->	2026-10-28 13:43:42 +04:00
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 -->	2026-10-27 11:47:21 +05:00
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`. ```	2026-10-26 21:10:45 +05:00
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>	2026-10-26 15:18:14 +03:00
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>	2026-10-23 14:35:34 +00:00
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>	2026-10-23 17:45:04 +04:00
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>	2026-10-23 17:34:25 +04:00
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>	2026-10-23 17:06:57 +04:00
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>	2026-10-23 16:55:32 +04:00
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>	2026-10-23 16:50:39 +04:00
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 -->	2026-10-23 17:39:50 +05:00
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>	2026-10-23 15:43:29 +04:00
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>	2026-10-23 15:06:36 +04:00
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>	2026-10-23 14:50:10 +04:00
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>	2026-10-22 21:50:26 +03:00
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>	2026-10-21 12:36:22 +02:00
myasnikovdaniil	1f92bcd20e	fix(fluxcd): close the flux-plunger review follow-ups for the hook, drain and Warnings (#4914 ) ## What this PR does Follow-up to the flux-plunger uninstall fence: the points the last review round raised and left for after the merge, plus four found by a later independent review. None changes the fence's design; each closes a gap in what it reports, how long it waits, or what its documentation says. Pre-delete hook. - A release being deleted that only the fence still holds is no longer unfenced in the same step that lifts its suspension. helm-controller runs the uninstall of a release being deleted once it is no longer suspended, but removing the fence at once deleted the object before helm-controller could reconcile it. The hook now keeps that fence while helm-controller handles the generation the lifted suspension wrote, until shortly before its retry window closes, and names a release it still had to let go of as before. A release whose state cannot be read keeps its fence for the wait the same way, rather than being let go of on one failed read. - A release being deleted while still suspended, by anyone but flux-plunger, was reported as cleanly released, although helm-controller skips the uninstall of a suspended release. It is now named as deleted without an uninstall and counted with the others. - Each release gets its own share of the retry window, capped at the window's end. One release that could never be read used to take the whole window and leave every release listed after it a single attempt. The share is a cap, not a floor of attempts: a floor would let enough failing releases run the Job past its deadline, which kills it before its report is written. - The hook stops, waits for and lists again only for the flux-plunger of its own release. Another one (a second installation under another release name, changed labels, a binary outside the cluster) is not stopped by the hook, keeps the fences it sets and gives them up itself; the README now says so instead of the hook re-listing unconditionally, which would not close that case and would fail the uninstall for as long as a second installation is recovering anything. - helm-controller's finalizer, which the hook reads to tell whether a release being deleted is still held, is tied to the constant the controller uses, as the fence already was. Two comments are corrected: the retry window counts from the script's start, not the Job's, and kubectl scale's internal conflict retry never runs here. Shutdown drain. - A drain deadline of zero or less is refused at startup, and the chart's lowest accepted grace period is held to the same constants as the drain subtraction, so raising the manager's stop bound cannot make the chart render a deadline that is already spent. The timings the README and the values comment state are held to those constants too. - Fences left behind are reported as an error naming each release, not an Info line; a drain that never got a listing answered says so instead of reporting a count of -1; a flux-plunger that never led no longer claims it released every fence. - The drain's Events fold repeats into one counted Event per release and reason, as the manager's recorder does, instead of one object per pass. The fold is a patch, the verb the chart grants on events, and every write is bounded by the drain's deadline. It does not add a per-release Event for fences left at the end: by then the drain's deadline is spent, and writing one would reach past it. - Tests now hold the outcome line, the retry of a release whose fence cannot be given up yet, and the pause between passes, none of which any suite noticed when removed. Controller. The `SuspensionNotOwned` and `TerminatingWhileSuspended` Warnings were keyed on the processed-version annotation, which stays on every release flux-plunger ever recovered, so a later restore, migration or operator suspension of a healthy release drew a Warning telling the reader to resume it, and a migration's deliberate suspend-then-delete was called an accident. Both now key on the state a leftover actually freezes the release in, `Ready=False` with `has no deployed releases`, since helm-controller rewrites no condition of a suspended release; the README's candidate query follows. Two comments that claimed more than holds are qualified. Chart and README. A `shutdownGracePeriodSeconds` of a million or more now gets the range message rather than being refused as a fraction. The README gives the recovery steps in the order the controller runs them (the annotation is written before the Secret is deleted), and names the window in which a backup restore started during a recovery is unsuspended under it, since the etcd, CNPG and Velero restore steps write `spec.suspend` only when it differs. ### Testing Each change was written test first: a test that builds the binary and runs it with a deadline of zero, helm-unittest for the chart, unit tests with a captured logger and the fake client for the drain's outcome, Events and passes (the Event fold under a client that refuses the update verb, as the chart's role does), table tests for both Warnings, and the bats suite for the hook, whose fake kubectl now models helm-controller handling a release some reads after its suspension is lifted and a state read that fails. Every fix was then reverted on its own, and the suite went red each time. The envtest suites pass unchanged. The README's candidate query was run against HelmReleases read from a live cluster, with a stuck, a healthy, an owned and an unsuspended variant, and lists only the stuck one. ### Screenshots Not applicable: no UI change. ### Downstream repositories The trigger map was walked against the diff: `internal/controller/fluxplunger`, `cmd/flux-plunger`, the `packages/system/flux-plunger` chart (templates, tests, README) and its bats test under `hack/`. The website's FluxPlunger section in `guides/concepts.md` draws the recovery with the Secret deleted before the annotation is written, which is the order before the fence change; it needs the same correction as the chart README. That change is held as a draft until v1.7.0 is released and the website's `next/` is re-initialized: the fence change is not part of release-1.7, so the corrected order would be wrong for v1.7 docs, which are copied from `next/`. No other repository is reached by this diff. - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: https://github.com/cozystack/website/pull/752 (draft, merge after v1.7.0) - [ ] [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(fluxcd): flux-plunger's pre-delete hook now lets helm-controller run the uninstall of a release being deleted before it removes the release's fence, gives each release its own share of its retries, and reports releases deleted under another manager's suspension; the shutdown drain reports the fences it leaves as errors and refuses a deadline that leaves no time ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * Bug Fixes * Shutdown drain timeouts must be positive, and invalid values now stop startup with a clear error. * Uninstall cleanup waits for Helm to handle releases before removing their fences, while bounding retries and reporting releases that remain incomplete. * Recovery and suspension warnings now reflect the release’s actual failure state, and recovery can retry stale storage cleanup against the same revision. * Documentation * Clarified shutdown grace-period requirements, uninstall behavior, and guidance for backup restores and existing suspensions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-10-20 17:44:34 +05:00
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>	2026-10-13 18:05:45 +04:00
Alexey Artamonov	699555c7cb	fix(fluxcd): give each release its own share of the hook's retries The pre-delete hook's retry budget was one window spent in listing order, so a release that could never be read or unfenced, listed first, took all of it and every release after it got a single attempt. The same transient failure then released a fence or not depending on where its release sorted. Each release now gets its own share of the window, capped at the window's end, and the steps that cover every release keep the whole of it. Two comments said more than holds. The retry window is measured from the script's start, not the Job's, so scheduling and the image pull come out of its one-minute reserve rather than being covered by it. And the five-minute conflict retry inside kubectl scale never runs here, since without a precondition it sends a merge patch with no resourceVersion to conflict on. helm-controller's finalizer, which the hook reads to tell whether a release being deleted is still held, is now tied to the constant the controller uses, as the fence already was. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>	2026-10-13 15:09:00 +03:00
Alexey Artamonov	7b84919db3	fix(fluxcd): warn about a foreign suspension only on a stuck release flux-plunger raised its SuspensionNotOwned and TerminatingWhileSuspended Warnings on a suspension it does not own when the release carried its processed-version annotation. That annotation is never removed, so every later suspension of such a release, by a migration, a backup restore or an operator, drew a Warning: one telling the reader to resume it, which mid-restore is the one thing nobody wants, and one calling a migration's deliberate suspend-then-delete an accident. Both also missed the leftover they are for when the first recovery stopped before writing the annotation. A suspension an earlier flux-plunger left mid-recovery freezes the release in the failure recovery started from, since helm-controller rewrites no condition of a suspended release. Both Warnings now key on that state instead, and the README's candidate query with them. Two comments are qualified where they claimed more than holds: a stop that overran the manager's grace period can still let a reconcile set a fence after the drain's last listing, and a process stopped before controller-runtime closes Elected holds the Lease yet does not drain. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>	2026-10-13 15:09:00 +03:00
Alexey Artamonov	caeaa6b5e5	fix(fluxcd): make flux-plunger's shutdown drain report what it left The drain's only report was an Info line, so an alert on errors never saw fences left behind; it named no release, and printed held: -1 when no listing had answered. A flux-plunger that never led logged that it had released every fence next to the line saying it drained nothing. Fences left behind are now an error naming each release, an unanswered listing says so, and a process that never led says only that. The drain writes its Events itself, since the manager's recorder stops with the manager, but it created one Event object per pass for a release it revisits every second, and each write had a deadline of its own outside the drain's. Repeats now fold into one counted Event, as the manager's recorder folds them, and every write is bounded by the drain. Tests now hold the outcome line, the retry of a release whose fence cannot be given up yet, and the pause between passes, none of which any suite noticed when removed. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>	2026-10-13 15:09:00 +03:00
Alexey Artamonov	8bcb709ce8	fix(fluxcd): refuse a flux-plunger drain deadline that leaves no time A drain deadline of zero or less is already spent when the drain starts, so flux-plunger would exit holding every uninstall fence without listing one. The chart could produce one: its 40-second floor was a literal, so raising the manager's stop bound or the margin before SIGKILL, which the drain subtraction is already tied to, would leave the floor rendering a non-positive deadline. The binary now refuses such a flag at startup, before it reaches for a cluster, and the chart's floor and the timings the README and the values comment state are held to the same constants. A whole number of a million seconds or more printed in exponent form and was refused as a fraction; the range is now checked first, so it gets the range message. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>	2026-10-13 15:09:00 +03:00
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	2026-10-13 12:34:24 +04:00
Aleksei Sviridkin	785cbc391b	fix(fluxcd): keep Helm uninstall working during flux-plunger recovery (#4293 ) ## What this PR does Fixes #4060. `flux-plunger` recovers a HelmRelease stuck with `has no deployed releases` by briefly suspending it, deleting the stale Helm storage Secret, recording a processed-version annotation, and unsuspending. helm-controller v1.5.x skips the Helm uninstall and drops its finalizer when it handles a delete on a suspended HelmRelease, and it wakes on the same delete event flux-plunger does, so a delete arriving inside that window orphaned every resource the release owns (for a VMDisk: DataVolume, PVCs, the CDI importer pod, RBAC, WorkloadMonitor and the storage Secret itself). Lifting the suspension on the delete event cannot win that race reliably. The bug is not specific to VMDisk. The change makes such a delete reach the uninstall: - An uninstall fence. The suspension is applied together with a flux-plunger finalizer, `flux-plunger.cozystack.io/uninstall-fence`, in one server-side apply, so flux-plunger never holds a suspension without it. If a delete arrives while the release is suspended, the fence keeps the object whether helm-controller or flux-plunger handles the delete first: flux-plunger lifts its suspension and keeps the fence, helm-controller reconciles the unsuspended generation and runs the uninstall, and flux-plunger removes the fence once helm-controller's finalizer is gone and it has observed that generation. A recovery that completes lifts the suspension and the fence together. A terminating HelmRelease never begins recovery, and termination is re-checked immediately before deleting the storage Secret. - Only flux-plunger's own suspension is acted on. Ownership is decided by which field manager owns `spec.suspend` in `metadata.managedFields`: the apiserver transfers the field away the moment an operator (or `cozyhr resume`, or a backup/restore controller) writes a different value to it, so a later suspension is never misread as flux-plunger's. A suspension set through the shared `flux-client-side-apply` manager (platform migrations, the tenant delete hook, an operator's manual `kubectl apply`) is never claimed. A release deleted under another manager's suspension is that manager's decision: flux-plunger does not hold it with the fence, and helm-controller leaves its resources in place, as the migrations that suspend and then delete rely on. - No suspension taken over in a race. Acquiring applies without forcing first and, on a conflict, re-fetches and forces only if `spec.suspend` is still `false`: a backup controller owns the field at `false` via a dynamic-client `Update`, which a non-forced apply can never take. Every apply flux-plunger sends, acquiring as well as lifting the suspension or the fence, is preconditioned on the `resourceVersion` and `uid` of the read that decided on it. The `resourceVersion` makes a write landing in between fail the apply instead of being absorbed: acquire then defers rather than adopt or take over a suspension set meanwhile, and lifting the suspension and the fence from a release read as live cannot delete it without an uninstall if helm-controller has handled a delete in the meantime. The `uid` stops an apply to a release that has gone from creating an empty one, since server-side apply creates what does not exist. A conflict is retried through the requeue. The apply carries only `spec.suspend` and the fence, so it cannot leak the required `spec.interval` and conflict on it. - Fresh reads. The release, every re-check and the storage Secret list are read from the apiserver (an uncached `APIReader`), not the informer cache. A lagging cache would let the processed-version guard delete the revision below the one just reclaimed, and the ClusterRole grants secrets `get`/`list` but not `watch`, which an informer needs to stay synced; flux-plunger deliberately does not cache Secrets. Every step requeues with backoff on failure. - Events. A Warning when flux-plunger holds a suspension it failed to clear, and a Warning when a delete has been held by the fence for more than ten minutes waiting for helm-controller (the held state is re-checked on a timer, not only on helm-controller's next status write). A release deleted under another manager's suspension gets a Normal event when that is a migration's choice, and a Warning when the release carries flux-plunger's processed-version annotation, because then the skipped uninstall is an accident, not a decision. Normal events also mark an externally-owned suspension left alone and a recovery deferred because the release changed between read and write; an external suspension on a release flux-plunger has recovered before is a Warning instead, because it is most likely one an earlier flux-plunger left mid-recovery (see Operating notes). Events from the shutdown drain are written straight to the apiserver, since the manager's recorder stops with the manager. - The fence is released when flux-plunger goes. Once its manager has stopped, and with it every reconcile, flux-plunger drains its fences under a deadline inside the pod's termination grace period; a stop that overran its 10s bound is drained too and still reported: a recovery in progress is aborted, the suspension and the fence lifted together for the next flux-plunger to start again, and a release being deleted has flux-plunger's suspension lifted and keeps the fence until helm-controller has run its uninstall. A release helm-controller does not handle in time keeps its fence. The drain lives in the controller, so the envtest suites hold it against a real apiserver's generations and finalizers. A pre-delete hook on the chart is the backstop for what the drain could not finish, so uninstalling the component never leaves a release it cannot finish deleting: it stops flux-plunger, waits out its grace period, then lifts any remaining suspension and fence without waiting again, naming each release it lets go of before helm-controller uninstalled it. It lifts a suspension only where flux-plunger alone holds it, the outcome the controller's own relinquish has, with a JSON patch of `spec.suspend` preconditioned on the `resourceVersion` and `uid` it read; not with `kubectl apply --server-side`, which on a release last written by a client-side `kubectl apply` moves that apply's fields to the applying manager and strips every one the body leaves out. helm-controller runs a pre-delete hook under the release's uninstall timeout, 300s by default and not set by the operator, so the Job's deadline sits under it. A Deployment that cannot be stopped or a listing that never succeeds ends the hook before any fence is touched, and a fence it cannot remove ends it after the sweep; either way it exits non-zero, because a failed pre-delete hook leaves the release untouched and helm-controller retries the uninstall, hook included, whereas exiting 0 would let the component go and strand the fence. A flux-plunger pod still there after its grace period, typically one stuck `Terminating` on a `NotReady` node, is only reported, and the pod wait is cut to what is left of the hook's retry window; because such a pod may still be setting fences, the fences are listed again after the sweep and any found fails the hook. The Job runs once and is kept after success so its report stays readable until its TTL; `helm.sh/hook-output-log-policy` is deliberately not used, because helm-controller never sets Helm's hook output writer and calls a nil one for it (fluxcd/helm-controller#1562). The hook's kubectl image is digest-pinned in the chart values, and its RBAC is only what `kubectl scale`, the listing and the patches issue. The chart's README documents the finalizer and how to clear it by hand. A single writer is kept by leader election, not by the rollout. A second process acting under the same field manager could clear a suspension the first is still recovering, and no Deployment strategy prevents one: the next pod starts as soon as the old one is terminating (Kubernetes v1.35 has no pod-replacement policy for Deployments), and an eviction or a node drain replaces a pod the same way, while the old one stops and drains. The chart now passes `--leader-elect` with a Role on `leases` in the release namespace. The Lease is not given up on stop and outlasts the last renewal, the renew deadline, the manager's bounded stop (10s) and the drain, and only a flux-plunger that led drains at all, so the next one acts only once the old one is done; the price is that a new pod waits for the Lease to run out, a little under two minutes at the default. The timings derive from one value, `shutdownGracePeriodSeconds` (90 by default, refused outside 40 to 140 at render): the pod's termination grace period, the drain deadline (25s less: the manager's 10s stop plus a 15s margin before the kubelet's SIGKILL, both tied to the Go constants by a contract test) and the hook's pod wait. The strategy keeps `maxSurge: 0` and `maxUnavailable: 1` and stays `RollingUpdate` rather than `Recreate` on purpose: the chart shipped without a strategy block, so every existing install's live Deployment carries the apiserver-defaulted `rollingUpdate` object and helm-controller applies server-side, where laying `type: Recreate` over that populated block is rejected on upgrade. ### Operating notes A HelmRelease carries the fence only while flux-plunger is recovering it or waiting for helm-controller to uninstall it. A fenced release whose uninstall keeps failing stays `Terminating` exactly as any HelmRelease does under helm-controller's own finalizer, and raises the Warning above. Stopping flux-plunger drains its fences, and uninstalling it releases whatever the drain left through the pre-delete hook; a fenced release that is deleted while flux-plunger is merely scaled to zero, or killed without a graceful stop, and that helm-controller has not uninstalled, stays `Terminating` until flux-plunger runs again. To release one by hand, after confirming its uninstall has run or is not wanted: ```bash kubectl patch helmrelease NAME --namespace NAMESPACE --type json --patch '[{"op":"test","path":"/metadata/finalizers/INDEX","value":"flux-plunger.cozystack.io/uninstall-fence"},{"op":"remove","path":"/metadata/finalizers/INDEX"}]' ``` where `INDEX` is the position of `flux-plunger.cozystack.io/uninstall-fence` in `metadata.finalizers`; the `test` operation makes the patch fail rather than remove another finalizer if the list has changed. Earlier flux-plunger releases set `spec.suspend` through the shared `flux-client-side-apply` manager, so a suspension one of them left behind cannot be told apart from a migration's or an operator's and is treated as external: it is not cleared automatically. Such a leftover exists only if the old pod stopped in the middle of a recovery, and flux-plunger raises a `SuspensionNotOwned` Warning on it. The chart README carries this section too. To list candidates, suspended releases carrying the flux-plunger annotation whose `spec.suspend` is not owned by flux-plunger's apply: ```bash kubectl get helmreleases.helm.toolkit.fluxcd.io --all-namespaces --output json \| jq --raw-output '.items[] \| select(.spec.suspend == true) \| select(.metadata.annotations["flux-plunger.cozystack.io/last-processed-version"] != null) \| select([.metadata.managedFields[]? \| select(.manager == "flux-plunger" and .operation == "Apply" and .fieldsV1["f:spec"]["f:suspend"] != null)] \| length == 0) \| "\(.metadata.namespace)/\(.metadata.name)"' ``` Clearing one is an operator action: confirm nothing else (a migration, a restore) is holding it, then resume it with `flux resume helmrelease NAME --namespace NAMESPACE`. ### Testing The regression the issue asks for is `TestEnvtest_DeleteDuringRecoveryReachesUninstall`: against a real apiserver, a delete lands while flux-plunger holds its recovery suspension, and helm-controller's v1.5.0 delete handling is reproduced step for step (uninstall only when not suspended, finalizer dropped either way, observed generation recorded). It covers both orders, helm-controller handling the delete first and flux-plunger handling it first; without the fence the first order leaves the release gone with no uninstall. The same suite pins the managedFields ownership dynamics a fake client cannot represent: the field transfer, the forced acquire over a dynamic-`Update` owner, the `resourceVersion` precondition rejecting a write that races either apply, the boundary that a `flux-client-side-apply` suspension is never claimed, and interval preservation. The fence mechanics were also checked against a live helm-controller: with another finalizer holding a deleted, suspended release, lifting the suspension made helm-controller run the uninstall and remove the release's resources. The envtest suite runs on every pull request: a new `make test-controllers-envtest` target builds a pinned `setup-envtest` and fetches a kube-apiserver and etcd into `_out/envtest`, which CI caches and the target reuses without asking the release index, takes the HelmRelease CRD from the in-tree Flux manifests, and CI calls it next to `make test-controllers`; the control plane gets a longer start timeout because it starts beside the other test goals. The drain is held there too: it aborts a live recovery, carries a delete through the uninstall and lifts the fence only once helm-controller has observed the unsuspended generation, keeps the fence when helm-controller never answers, releases a fence left without a suspension at once, and leaves another manager's suspension alone. `TestEnvtest_TheNextFluxPlungerLeadsOnlyAfterTheDrain` runs two real managers on one Lease: a clean stop still drains, and the second manager becomes leader only after the first one's drain has ended; it goes red if the Lease is given up on stop. Unit tests cover the controller logic (every branch, ordering, requeue, and the ownership decision through an injected seam), the Lease timing relations, that only a leader drains and through the manager's API reader rather than its stopped cache, that a failed listing is retried, that a stop which overran its grace period is still drained and its events reach the apiserver, and contract tests tie `main.go`, the chart's flags, the drain's arithmetic and the hook's names to the Go code. The pre-delete hook is covered by helm-unittest and by `hack/flux-plunger-fence-hook_test.bats`, which runs its rendered script against a fake `kubectl` that keeps each release's finalizers, suspension owner, generation and observed generation, bumps the generation on a spec write as the apiserver does, answers NotFound without `--ignore-not-found`, spends simulated time on waits, lists a release once however many copies of the fence it carries, refuses `kubectl apply`, lifts a suspension only through the preconditioned JSON patch and never one another manager also holds, reads a negative `kubectl wait` timeout as a week as kubectl does, and applies a finalizer removal only when it is the guarded test-then-remove at an index that holds the fence. Every mutation in a set covering each predicate conjunct, each retry, each exit path, the re-listing, the timings and the selector turns the hook suite red, as does every mutation in the Go set covering the drain's branches, the Lease timings and the events; three drain mutations that still survive are tracked in #4782. The hook script was also run as a Job in the hook's own image on a live cluster, succeeding, with the Deployment already gone, and with patch denied by RBAC, where it exited non-zero and named the release it left; that run caught that a per-call `--request-timeout` makes kubectl drop its in-cluster config, so the hook sets none and the fake rejects it. A later live run caught `kubectl apply --server-side` stripping the finalizers and spec of a release created by a client-side apply; with the JSON patch the same releases kept every field, the suspension flux-plunger alone held was lifted, a co-owned and a foreign one stayed, and every fence went. ### Remaining limitations - A delete that arrives after recovery has deleted the storage Secret of a release whose only revision was that one finds no revision to uninstall; helm-controller then treats the release as already gone. - A writer that sets `spec.suspend: true` while flux-plunger already holds it at `true` does not take the field over, because the value does not change. If such a writer then deletes the release inside the recovery window (the suspend-then-delete that the tenant delete hook and some platform migrations use to skip the uninstall), flux-plunger still sees its own suspension, lifts it, and helm-controller runs the uninstall that writer meant to skip. The same held before this change, when the end of recovery unsuspended unconditionally. - The already-processed guard compares revision numbers only. When Helm re-creates a revision under the same number after recovery deleted it (a release whose only revision fails again the same way), recovery runs again for it. - `listHelmReleaseSecrets` reads the local cluster only, so a HelmRelease that targets a remote cluster via `spec.kubeConfig` (whose storage Secrets live in the remote cluster) is not recovered; it is skipped, never mis-handled. - A new flux-plunger pod waits for the old one's Lease to run out before it acts, on every restart and after a crash: a little under two minutes at the default. Recovery is not latency-sensitive, and this is what keeps a single writer while the old pod drains. - The leader Lease is left in the release namespace after the chart is uninstalled, as with any controller-runtime leader election; it is inert. ### Screenshots Not applicable: no UI change. ### Downstream repositories The trigger map was walked against the diff: it touches `internal/controller/fluxplunger`, `cmd/flux-plunger`, the `packages/system/flux-plunger` chart (templates including RBAC for the leader Lease, a pre-delete hook, values, tests, its package `Makefile`, README), a new bats test under `hack/`, the root `Makefile` (a new `test-controllers-envtest` target; existing targets are unchanged) and the pull-request workflow that calls it and caches its binaries. No row matches: ccp gates on existing `hack/` files and make targets, and nothing there is moved, renamed or changed in behaviour; a new test file and a new target reach none of them. The website's FluxPlunger section describes the recovery steps, which still hold; it does not describe the fence or the drain, which the chart README now documents. - [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(fluxcd): a HelmRelease deleted while flux-plunger is recovering it now reaches the Helm uninstall instead of leaving its resources (DataVolume, PVCs, RBAC, storage Secret) behind ```	2026-10-12 15:23:49 +03:00
Aleksei Sviridkin	8deaa8e14c	fix(vm-disk): name the importer error in the disk's WorkloadsReady message (#4815 ) <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does A VMDisk whose HTTP source answers 404 or does not resolve never finishes importing, and since #4380 its `WorkloadsReady` condition says only `DataVolume vm-disk-x is ImportInProgress`. That reads the same as a large download that is still running. CDI fails an import only on a fatal error (a checksum mismatch) and restarts the importer pod otherwise, so the phase never moves, and the reason the disk will not finish was visible only in the importer logs. CDI already keeps that reason on the DataVolume: the `Running` condition goes `False` with the importer's termination message, and keeps it through the back-off between restarts. The WorkloadMonitor now appends it, so the condition reads `DataVolume vm-disk-x is ImportInProgress: Unable to connect to http data source: expected status code 200, got 404. Status: 404 Not Found`. The console already shows the `WorkloadsReady` message on the disk's overview page, so no UI change is needed. The same text covers an unschedulable importer, a failed image pull and a checksum failure, since CDI reports all of them through `Running=False`. Only `Running=False` is read: `Unknown` is not a failure, and `Bound` repeats the same error behind PVC details. For a host-assisted clone CDI joins the target and source pod messages as `<target> and <source>`, which comes out as ` and ` when both pods are still being created, so empty halves are dropped. Behaviour to be aware of: while the importer pod is briefly running between restarts, CDI flips `Running` to `True` and clears the message, so the condition shows the bare phase for those seconds. This is not a terminal state either: a disk with a broken source still stays `ImportInProgress` and keeps retrying. Deciding when such an import should count as failed is left for a separate change. ### Tests A table test covers a 404, an unresolvable host, an unschedulable importer, a fatal checksum error, a DataVolume without a phase and joined clone messages with one or both halves empty, plus the cases that must not add text: an importer that has not started, a running importer, an `Unknown` condition, a populated disk and an error that only sits on `Bound`. Each conjunct of the condition check and the append itself were mutated and each mutant fails the test. A second test runs against a DataVolume captured from a test cluster after its importer had restarted about two thousand times on a 404, with only names and the URL changed, so the parsing is pinned against what CDI actually writes rather than a hand-built shape. ### Screenshots No UI code changes: the console renders the `WorkloadsReady` message it already receives. A message that carries a long source URL is not wrapped on the overview card; that is a styling follow-up for the console and is left out of this 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: The only non-Go change is a sentence in the `packages/apps/vm-disk/README.md` notes; no values, schema, API kind or default changes. The website reference page for vm-disk is regenerated from that README by the release bot. ### Release note ```release-note fix(vm-disk): the WorkloadsReady condition of a VM disk now includes the importer error reported by CDI, such as a 404 or an unresolvable host, so a disk whose source keeps failing shows why it is stuck instead of only ImportInProgress. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * Bug Fixes * Workload status messages now include available DataVolume import failure details, such as HTTP errors or DNS resolution problems, making the cause of an import issue clearer. * Documentation * Updated the workload readiness documentation to explain when DataVolume phase and import error details appear in status messages. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-10-12 12:29:50 +03:00
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>	2026-10-10 23:49:09 +03:00
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>	2026-10-10 23:49:09 +03:00
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>	2026-10-10 23:49:08 +03:00
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>	2026-10-10 23:49:08 +03:00
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 `2234824fe` the DataVolume was deleted with background propagation, so garbage collection removed the claim, but nothing waited for it. Deleting the claim is only safe when Velero can put the disk back, so before any preparation the reconciler now gets the Velero Backup and fails the RestoreJob unless it exists and is `Completed`. The Backup artifact outlives the Velero Backup, which Velero garbage-collects at its TTL, and without the check a restore of an expired backup would halt the VM and delete its disks before failing. The check runs for every Velero restore, whatever `keepOriginalPVC` says. A DataVolume that cannot be deleted now fails the RestoreJob too, and the preparation waits for every DataVolume to be gone before it goes further: a surviving DataVolume makes CDI recreate the claim, which Velero then skips. Reading the DataVolume back needs `get` on `datavolumes`, which the backupstrategy-controller ClusterRole now grants next to `delete`. `TestPrepareForRestore_KeepOriginalPVCFalse_SkipsRename` asserted the old behaviour, that the original PVC survives, and is replaced by `TestPrepareForRestore_KeepOriginalPVCFalse_DeletesPVC`. `TestPrepareForRestore_KeepOriginalPVCFalse_WaitsForTerminatingPVC` covers the wait. Both fail on `upstream/main` without the controller change. `TestReconcileVeleroRestore_UnusableVeleroBackupTouchesNothing` covers an expired Velero Backup and one being deleted, `TestPrepareForRestore_KeepOriginalPVCFalse_KeepsPVCWhileDataVolumeRemains` a failed and a pending DataVolume deletion, and `TestPrepareForRestore_KeepOriginalPVCTrue_FailedRenameKeepsPVC` a failed rename under `keepOriginalPVC: true`, which must leave the claim in place. A helm-unittest suite, `tests/restore_rbac_test.yaml`, checks the new verb. Each test fails with its fix reverted, with one exception: `TestPrepareForRestore_KeepOriginalPVCTrue_FailedRenameKeepsPVC` still passes with the Step 5 condition cut to `vmRes != nil`, because a failed rename holds the restore back before Step 5 is reached. That guard is pinned by `TestPrepareForRestore_KeepOriginalPVC_CacheLagsBehindWrites`, which fails with the same cut. `helm unittest packages/system/backupstrategy-controller` passes. `go test ./internal/backupcontroller/...` passes. Related: #4601 reports the same Velero mechanism for a standalone VMDisk, where `prepareForRestore` does nothing at all because `collectUnderlyingResources` records no disk. #3427 makes a VMDisk report its own disk; with `keepOriginalPVC: false` that path then runs into this defect too, and this change covers it once both are in. This PR does not touch the VMDisk path itself. ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map against the diff, which is the restore preparation in `internal/backupcontroller/velerostrategy_controller.go` and its unit tests, plus one verb in the backupstrategy-controller ClusterRole and its helm-unittest: no package added or renamed, no `values.yaml` or `values.schema.json` change, no CRD, ApplicationDefinition, label, annotation, namespace or metric change. `keepOriginalPVC` lives in the opaque `RestoreJob.spec.options`, and a search of the organisation finds it only in this repository, so neither the website nor the Terraform provider documents or models it. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(backups): an in-place VMInstance RestoreJob with keepOriginalPVC=false now deletes each original disk PVC, and waits for it to be gone, before Velero restores it. Previously the PVC was left in place, Velero skipped restoring its data, the RestoreJob still reported Succeeded, and the disk's HelmRelease could never recreate its DataVolume ("Destination PVC already exists"). A Velero RestoreJob now also fails before touching the application when its Velero Backup is gone or not Completed, for example after the Backup expired at its TTL. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * Bug Fixes * When a restore is configured not to keep the original disk claim, the claim is deleted before Velero restores it, allowing Velero to recreate it. * Restores wait while the original disk claim or its DataVolume is being deleted. Deletion or status-check errors stop restore preparation. * Restores configured to keep the original disk claim continue to preserve it. * Restores stop before changing resources if the referenced Velero backup is missing or has not completed. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-10-10 22:26:10 +03:00
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` (`6addef4d3`) 10, 8 and 5 of them still got a Velero Backup over three runs. With this change: 0, 0 and 0. The harness is in the collapsed block below; it needs `KUBEBUILDER_ASSETS` and `REPO_ROOT`, and is not part of the PR. 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. The reproduction above covers the backup side only; on the restore side the unit test below shows what a pass on a stale copy does. 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 all eleven drivers at once. The cost is one GET per pass of an active job; a job the cache already reports as finished costs nothing extra. `TestReconcile_DoesNotRetryAJobItAlreadyFailed` and `TestReconcileRestore_DoesNotRetryAJobItAlreadyFailed` run the full `Reconcile` on a cache holding the job active while the apiserver holds it `Failed`. Each first checks that the same fixture does create the Velero Backup, or suspend the HelmRelease and create the Velero Restore, when the apiserver agrees with the cache. With the re-read reverted to the cached client, both fail: a Velero Backup is created and the phase is written back to `Running`, the HelmRelease is suspended and a Velero Restore is created. `go test ./internal/backupcontroller/...` passes. <details> <summary>envtest reproduction harness</summary> ```go //go:build repro // Reproduction, not for commit: does the real BackupJob controller, on a real // apiserver and the manager's real cache, create a Velero Backup for a // BackupJob it has itself marked Failed? // // The only fault injected is one 503 on the first GET of each application // through apps.cozystack.io, the aggregated cozystack-api, standing for a // blip of that API. Nothing else writes the BackupJob. // // Run: KUBEBUILDER_ASSETS=<envtest bin dir> go test -tags repro -run TestReproFinishedJobRace -v ./internal/backupcontroller/ package backupcontroller import ( "context" "fmt" "io" "net/http" "os" "strings" "sync" "testing" "time" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/log/zap" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" strategyv1alpha1 "github.com/cozystack/cozystack/api/backups/strategy/v1alpha1" backupsv1alpha1 "github.com/cozystack/cozystack/api/backups/v1alpha1" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" ) type blipOnce struct { next http.RoundTripper mu sync.Mutex seen map[string]bool } func (b blipOnce) RoundTrip(req http.Request) (http.Response, error) { p := req.URL.Path if req.Method == http.MethodGet && strings.Contains(p, "/apis/apps.cozystack.io/v1alpha1/namespaces/") { b.mu.Lock() first := !b.seen[p] b.seen[p] = true b.mu.Unlock() if first { body := `{"kind":"Status","apiVersion":"v1","status":"Failure","message":"the server is currently unable to handle the request","reason":"ServiceUnavailable","code":503}` return &http.Response{ StatusCode: http.StatusServiceUnavailable, Header: http.Header{"Content-Type": []string{"application/json"}}, Body: io.NopCloser(strings.NewReader(body)), Request: req, }, nil } } return b.next.RoundTrip(req) } func TestReproFinishedJobRace(t testing.T) { ctrl.SetLogger(zap.New(zap.WriteTo(io.Discard))) root := os.Getenv("REPO_ROOT") dummyCRD := &apiextensionsv1.CustomResourceDefinition{ ObjectMeta: metav1.ObjectMeta{Name: "dummies.apps.cozystack.io"}, Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "apps.cozystack.io", Names: apiextensionsv1.CustomResourceDefinitionNames{Kind: "Dummy", ListKind: "DummyList", Plural: "dummies", Singular: "dummy"}, Scope: apiextensionsv1.NamespaceScoped, Versions: []apiextensionsv1.CustomResourceDefinitionVersion{{ Name: "v1alpha1", Served: true, Storage: true, Schema: &apiextensionsv1.CustomResourceValidation{OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ Type: "object", XPreserveUnknownFields: new(true), }}, }}, }, } env := &envtest.Environment{ CRDInstallOptions: envtest.CRDInstallOptions{ Paths: []string{ root + "/packages/system/backup-controller/definitions", root + "/packages/system/backupstrategy-controller/definitions", root + "/packages/system/velero/charts/velero/crds/backups.yaml", }, CRDs: []apiextensionsv1.CustomResourceDefinition{dummyCRD}, ErrorIfPathMissing: true, }, } cfg, err := env.Start() if err != nil { t.Fatalf("envtest: %v", err) } defer env.Stop() s := runtime.NewScheme() _ = scheme.AddToScheme(s) _ = backupsv1alpha1.AddToScheme(s) _ = strategyv1alpha1.AddToScheme(s) _ = velerov1.AddToScheme(s) direct, err := client.New(cfg, client.Options{Scheme: s}) if err != nil { t.Fatal(err) } ctx, cancel := context.WithCancel(context.Background()) defer cancel() mgrCfg := cfg mgrCfg.WrapTransport = func(rt http.RoundTripper) http.RoundTripper { return &blipOnce{next: rt, seen: map[string]bool{}} } mgr, err := ctrl.NewManager(&mgrCfg, ctrl.Options{Scheme: s, Metrics: metricsserver.Options{BindAddress: "0"}}) if err != nil { t.Fatal(err) } r := &BackupJobReconciler{Client: mgr.GetClient(), Scheme: s, Recorder: mgr.GetEventRecorderFor("repro")} if err := r.SetupWithManager(mgr); err != nil { t.Fatal(err) } go func() { _ = mgr.Start(ctx) }() must := func(o client.Object) { if err := direct.Create(ctx, o); err != nil { t.Fatalf("create %T %s: %v", o, o.GetName(), err) } } must(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "tenant"}}) must(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: veleroNamespace}}) must(&strategyv1alpha1.Velero{ObjectMeta: metav1.ObjectMeta{Name: "velero"}}) strategyGroup := strategyv1alpha1.GroupVersion.Group appGroup := "apps.cozystack.io" must(&backupsv1alpha1.BackupClass{ ObjectMeta: metav1.ObjectMeta{Name: "bc"}, Spec: backupsv1alpha1.BackupClassSpec{Strategies: []backupsv1alpha1.BackupClassStrategy{{ StrategyRef: corev1.TypedLocalObjectReference{APIGroup: &strategyGroup, Kind: strategyv1alpha1.VeleroStrategyKind, Name: "velero"}, Application: backupsv1alpha1.ApplicationSelector{APIGroup: &appGroup, Kind: "Dummy"}, }}}, }) time.Sleep(2 * time.Second) // let the cache list the fixtures n := 40 if v := os.Getenv("REPRO_N"); v != "" { fmt.Sscan(v, &n) } for i := range n { app := &unstructured.Unstructured{} app.SetGroupVersionKind(schema.GroupVersionKind{Group: appGroup, Version: "v1alpha1", Kind: "Dummy"}) app.SetNamespace("tenant") app.SetName(fmt.Sprintf("app-%d", i)) must(app) must(&backupsv1alpha1.BackupJob{ ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: fmt.Sprintf("bj-%d", i)}, Spec: backupsv1alpha1.BackupJobSpec{ BackupClassName: "bc", ApplicationRef: corev1.TypedLocalObjectReference{APIGroup: &appGroup, Kind: "Dummy", Name: app.GetName()}, }, }) time.Sleep(200 * time.Millisecond) } time.Sleep(5 * time.Second) phases := map[backupsv1alpha1.BackupJobPhase]int{} orphans := 0 for i := range n { j := &backupsv1alpha1.BackupJob{} if err := direct.Get(ctx, client.ObjectKey{Namespace: "tenant", Name: fmt.Sprintf("bj-%d", i)}, j); err != nil { t.Fatal(err) } phases[j.Status.Phase]++ vbs := &velerov1.BackupList{} if err := direct.List(ctx, vbs, client.InNamespace(veleroNamespace), client.MatchingLabels{ backupsv1alpha1.OwningJobNamespaceLabel: "tenant", backupsv1alpha1.OwningJobNameLabel: j.Name, }); err != nil { t.Fatal(err) } if j.Status.Phase == backupsv1alpha1.BackupJobPhaseFailed && len(vbs.Items) > 0 { orphans++ if orphans <= 3 { t.Logf("%s: phase=%s message=%q, yet Velero Backup %s/%s exists (created %s, job completedAt %s)", j.Name, j.Status.Phase, j.Status.Message, veleroNamespace, vbs.Items[0].Name, vbs.Items[0].CreationTimestamp.Format(time.RFC3339Nano), j.Status.CompletedAt.Format(time.RFC3339Nano)) } } } t.Logf("jobs=%d phases=%v failed-with-velero-backup=%d", n, phases, orphans) } ``` </details> ### Screenshots N/A — no UI changes. ### 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. --> Walked the trigger map against the diff, which is the two job reconcilers in `internal/backupcontroller/` and a test file: no package, `values.yaml`, `values.schema.json`, CRD, ApplicationDefinition, label, annotation, namespace or `hack/` change. - [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): a BackupJob or RestoreJob that the controller has just marked Failed no longer gets a driver object created for it by the next reconcile, which could start on a cache that had not yet seen the Failed status and retry the step that failed — leaving, for instance, an untracked Velero Backup behind a Failed BackupJob, or a restore that halts the VM after its RestoreJob failed. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * Bug Fixes * Backup and restore jobs that have already succeeded or failed are no longer retried based on outdated status, preventing duplicate operations and unnecessary changes to related resources. * Reconciliation stops when a job has been removed and reports errors if its current status cannot be checked. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-10-10 22:15:54 +03:00
Aleksei Sviridkin	d9689bd6e7	fix(gateway): stop describing Cilium 1.19.5 as the shipped release With Cilium at 1.20.2 the tenant gateway comments, README and one status message still explained behaviour as that of "the pin" at 1.19.5. Several of those claims no longer hold: a Gateway with two routed passthrough ports now gets an Envoy listener per port, Cilium judges a route's kinds against the listener the route names, and the shipped operator accepts an HTTPS and a passthrough listener on one port when the passthrough hostname is strictly narrower. Behaviour that is unchanged is re-anchored on the 1.20.2 source, behaviour that is gone is stated as history, and the refusal of one hostname on two passthrough ports no longer blames a Cilium limitation the shipped release does not have. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-10-10 06:32:42 +03:00
Aleksei Sviridkin	ad3f15d4f3	feat(gateway)!: reserve a passthrough listener's name by the spec alone The TenantGateway controller withheld the HTTPS-terminate listener for a name a passthrough listener answers only once a TLSRoute was attached there and resolved a backend, mirroring how Cilium builds its filter chains. That kept a served endpoint up on Cilium 1.19.5, but it tied every route's status to one implementation's translator and needed Service and ReferenceGrant watches to land the deferred decision. The shipped operator accepts a wildcard terminate listener beside a narrower passthrough listener on 443 and keeps conflicting every other overlap. A terminate listener for the passthrough name itself is not narrower, so on 443 the rule now protects the dataplane outright; on a native port Gateway API would serve the pair, and the rule holds as a policy that keeps a platform name with its passthrough listener. Reading it off the spec makes the outcome the same on every implementation. Under http01 an HTTPRoute claiming such a name gets no listener and no certificate whether or not a TLSRoute is attached. A route whose every hostname is reserved reads Accepted=False with NoMatchingListenerHostname; one that also claims a free hostname stays Accepted and is served there, as Gateway API accepts a route whose hostnames intersect at least one listener. dns01 and existingSecret are unchanged: their wildcard listener still covers such a name, and the passthrough listener answers it by its exact server name. The chart writes the passthrough services on the publishing tenant alone, so the names reserved by default are the ones the platform publishes there through its own TLSRoutes. The shape an upgrade changes is an HTTPRoute on such a name whose platform TLSRoute forwards nowhere, because it never attached (the package owning the service is not installed) or because none of its backends resolve: that route kept a terminate listener and now gets none. The backend resolution, its watches and the RBAC rules that served them go away. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-10-10 06:32:42 +03:00
Aleksei Sviridkin	bdca165f4c	feat(gateway): report a GatewayClass that does not serve TLSRoute A GatewayClass lists the features it serves in status.supportedFeatures, and TLS passthrough is TLSRoute. On a class that leaves it out the passthrough listeners are accepted and serve nothing, so the TenantGateway now reads Ready=False/GatewayClassUnsupported naming the class instead of Ready. A class that lists no features at all is not judged: implementations predating the field leave it empty. The class writes that list after it is accepted and may change it when its controller is upgraded, so a GatewayClass event requeues the TenantGateways on it. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-10-09 17:53:25 +03:00
Aleksei Sviridkin	d624049a3a	feat(gateway): allow tlsPassthroughListeners under dns01 and existingSecret The refusal existed because Cilium 1.19.5 served every Gateway port from one Envoy listener matched by SNI alone, so the "*.<apex>" terminate listener of the wildcard modes and a passthrough listener under the apex could reach each other's backend. The shipped Cilium 1.20.2 gives each port its own Envoy listener when SNI overlaps across ports, and Gateway API treats listeners on different ports as compatible, so the pair is served as declared. edge still refuses the field: it renders no TLS listener for an entry to bind to. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-10-09 17:53:25 +03:00
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>	2026-10-08 14:09:08 +02:00
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 -->	2026-10-08 13:11:46 +03:00
Alexey Artamonov	970ccc2578	test(vm-disk): pin the importer error against a real stuck DataVolume The table test builds its Running conditions by hand, so it can only confirm the shape it already assumes. A DataVolume read off a cluster where the importer had failed on a 404 for days carries a Bound condition with the same error, which the message must not pick up, and proves the error sits where the controller looks for it. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>	2026-10-08 09:46:36 +03:00
Alexey Artamonov	0b9a801a2b	fix(vm-disk): name the importer error in the disk's WorkloadsReady message A disk whose HTTP source answers 404 or does not resolve never leaves ImportInProgress: CDI fails an import only on a fatal error and restarts the importer pod otherwise, so the condition only ever said the disk was still importing. The reason it will not finish was visible only in the importer logs and on the DataVolume, whose Running condition carries the error through the back-off between restarts. Appending that message lets the user see why the disk is stuck from the application status and the console, which already shows the WorkloadsReady message for a disk. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>	2026-10-08 09:46:36 +03:00
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>	2026-10-07 11:32:46 +02:00
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>	2026-10-07 11:23:37 +02:00
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>	2026-10-07 11:23:17 +02:00
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	2026-10-07 11:15:43 +02:00
Aleksei Sviridkin	8c1a78dc9b	feat(operator): default system namespace memory via LimitRange (#3494 ) ## What this PR does System components now get a default container memory limit, so their pod cgroups carry a `memory.max` and drop out of the Talos userspace OOM handler's victim set. `cozystack-operator` maintains a LimitRange named `cozystack-system-defaults` in every system namespace it reconciles. The Talos handler (v1.12+) scores cgroups with `memory_max.hasValue() ? 0.0 : {Besteffort: 1.0, Burstable: 0.5, ...}[class] * memory_current` and drops everything that scores zero. Only a memory limit sets `memory.max`, so only a limit takes a pod out of the candidate set. A request just moves a pod from BestEffort to Burstable, which under the default `strictCgroupClassOrdering: true` still means killed second. `PriorityClass` does nothing here, because the handler reads cgroupfs and never talks to the apiserver. Victim selection is also unrelated to the trigger, so a tenant workload thrashing against its own limit can drive node memory pressure while a system component with no limit is what gets killed. An audit of a running cluster found around 70 workloads across ~30 packages with at least one container missing a memory limit. Per-chart resources would not cover that set, and would not keep covering it as packages are added. So the LimitRange is written by `reconcileNamespaces`, which already creates these namespaces and already tells system from tenant. It also covers init containers, which #3495 can't reach: one of them without a limit leaves the pod cgroup without `memory.max`, whatever the main containers declare. Tenant namespaces are skipped. Where the tenant chart's own `tenant-range-limits` exists (only when the operator configured `resourceQuotas`), a second LimitRange next to it would leave the result to the order LimitRanger iterates them. Where it doesn't exist, a tenant namespace is not the place for the platform to impose a default. ### Why the default is 32Gi The handler grants immunity for `memory.max` existing at all, whatever its value. So the ceiling doesn't need to be fitted, and 32Gi is high enough that nothing plausibly reaches it. A ceiling low enough to bind would turn a rare pressure-driven kill into a deterministic one for any component whose real usage nobody measured. This repo already has one such case, recorded in the `vertical-pod-autoscaler` values, where a 110Mi limit OOM-killed the updater and failed HelmReleases with it. Scheduling is unaffected, since pods are scheduled against requests and the paired default request is 32Mi. An operator who wants the ceiling to bind can lower it, per component in that component's chart or fleet-wide with the knob, once real usage is measured. ### When the default is withheld A default below a container's own request makes that pod impossible to admit, so the operator checks before it applies. It scans Deployments, StatefulSets, DaemonSets, CronJobs, Jobs and Pods in the namespace, init containers included, for a container that requests more than the configured limit and declares no limit of its own. If it finds one, it withdraws the managed LimitRange and records a hold in the `operator.cozystack.io/system-memory-state` annotation on the Namespace. The hold does not clear by itself when the blocker disappears, because an empty scan looks the same as a pod between deletion and recreation. Lowering a previously applied limit also creates a hold, even with no visible blocker, because LimitRanger writes the defaulted limit onto admitted pods and the scan can no longer tell which containers declare one. A cluster administrator releases the hold by copying the `<hold>:<target>` token from the state into the `operator.cozystack.io/system-memory-ack` annotation. The token is used once and only for that hold and target. If a blocker is still visible, the token is rejected and the hold stays. Package reconciles are scheduled at least every five minutes, so an acknowledgement takes effect on a quiet cluster too. The reason and the token also appear in the operator log under `withholding the default container memory limit`. ### Namespace another LimitRange already governs If a system namespace already has a LimitRange that says anything about memory, at the Container or the Pod scope, the operator writes nothing there and withdraws what it wrote before. A second default leaves the effective value to iteration order. A foreign `max` below this default, a `min` above the default request or a `maxLimitRequestRatio` rejects pods outright, which takes out a whole namespace. At the Pod scope the collision can't be decided in advance, because it depends on how many containers a future pod has. A LimitRange that bounds only cpu or storage coexists normally. A same-named LimitRange without the operator's managed-by label is treated as foreign and left alone. This matters most in `kube-system`, which is in scope because `cozystack-scheduler` installs there, and where a distribution's or an administrator's own LimitRange is likeliest to sit. ### Disabling Setting `cozystackOperator.systemNamespaceMemoryLimit` to `0` disables the feature. On the next reconcile the operator removes every LimitRange it owns cluster-wide, found by the managed-by label. While the feature is on, it also removes its LimitRange from namespaces no active Package targets any more. The safety state on the Namespace stays in both cases. ### Docs `docs/operations/system-memory-limits.md` landed in #3496 ahead of this code, so main describes a `4Gi` default and installer values that do nothing yet. The page is corrected to the 32Gi default and now describes withholding, the acknowledgement, and the foreign LimitRange rule. ### Not in this PR linstor policy. #3824 extends the controller's existing exemption from limits to the satellite, and a linstor limit here would contradict it, so linstor stays with that PR. The scan reads six workload kinds through `APIReader`, uncached, on every reconcile of every system namespace. That cost is tracked in #3977, since the system namespace set isn't known when the cache is built and the right fix depends on a reconcile-rate measurement nobody has taken. ### Verification `go vet` and `go test` pass for `cmd/cozystack-operator` and `internal/operator`, and `helm unittest` passes on the installer chart for the knobs unset, set, and the limit at `0`. The reconciler tests cover withholding on an oversized request, the hold surviving the blocker's removal, the hold on a lowered limit, acknowledgement and token rejection, every memory field of a foreign LimitRange at both scopes, ownership by label, the disabled sweep, and a deleting Package not creating policy. The withhold design has not been run on a live cluster yet. An earlier design of this PR was, and that run does not carry over. ### Screenshots Not a UI change. ### Downstream repositories I walked the trigger map against the diff. `packages/core/installer/values.yaml` gains two keys, but the `ansible-cozystack` trigger is for a required key or a rename of one the role passes, and these are new, optional and default to `""`, so the role is unaffected. Nothing else in the map is reached by the Go changes, the operator template or the page. - [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 feat(operator): system namespaces now get a default container memory limit through a LimitRange maintained by cozystack-operator, which keeps system components out of the Talos userspace OOM handler's victim set. Tune it with cozystackOperator.systemNamespaceMemoryLimit (default 32Gi) and cozystackOperator.systemNamespaceMemoryRequest (default 32Mi); set the limit to 0 to disable it. The default is far above any real working set on purpose, because the handler grants immunity for a memory.max existing at all, not for its value. A namespace with a container that requests more than the limit and declares no limit of its own gets no default, and stays without one until a cluster administrator acknowledges the hold recorded on the Namespace (see docs/operations/system-memory-limits.md); lowering a previously applied limit needs the same acknowledgement. A namespace where another LimitRange already sets anything about memory is left alone. A container that declares its own memory limit is never changed. ```	2026-10-07 11:19:26 +03:00
Myasnikov Daniil	9927c299d4	fix(operator): keep deleting packages out of memory policy creation A foreground-deleting Package can survive long enough to reconcile after cleanup retires its default. It must not reinstate that policy or consume an acknowledgement while its components are being removed. An active Package sharing the namespace can still manage the default. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-10-07 06:06:52 +03:00
Myasnikov Daniil	9d5d6d8f45	test(operator): exercise failed memory safety scans Foreign ownership short-circuited the read-error fixtures before their interceptors ran. Require the injected failure to occur and preserve both policy and Namespace state, including a pending acknowledgement. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-10-07 06:06:52 +03:00
Myasnikov Daniil	5b585d6eda	fix(operator): persist holds for unsafe memory defaults An admitted Pod loses the distinction between explicit and defaulted limits. Its disappearance cannot prove a lower default is safe for its replacement. Keep that decision on the Namespace until an administrator acknowledges the particular target, without waiving current blockers or foreign policy ownership. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-10-07 06:06:51 +03:00
Myasnikov Daniil	30ecc41070	fix(operator): close memory policy safety gaps Preserve foreign same-name objects and keep create-time SSA non-force. Only ignore failed pods controlled by terminal Jobs. Make the failure-containment test inject Apply errors for real. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-10-07 06:06:51 +03:00
Myasnikov Daniil	22a27e75ac	test(operator): cover memory cleanup failures Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: LLM	2026-10-07 06:06:51 +03:00
Myasnikov Daniil	e83b4eb492	fix(operator): align memory policy ownership messages Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: LLM	2026-10-07 06:06:50 +03:00

1 2 3 4 5 ...