mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-23 03:27:50 +09:00
main
568
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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 --> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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 |
||
|
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 ``` |
||
|
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 --> |
||
|
Andrey Kolkov
|
6a75883013 |
fix(backups): keep the CNPG purge for unmarked Clusters this restore did not render
A recovery Cluster without the restored-server-name annotation fell back to the creation-time comparison. On the current chart the annotation is absent when bootstrap.newServerName is empty, i.e. a manually configured recovery: re-rendered during the WAL-archive wait it is created after StartedAt, the purge was skipped and the restore could report success on the manual recovery source. Use the creation-time fallback only when the application's bootstrap.newServerName already carries this restore's server name, which keeps it for Clusters rendered by a chart that predates the annotation. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
35ca217839 |
fix(backups): name the restore that goes first in ConcurrentRestore
restoreAhead returned the alphabetically smallest of the jobs ahead, so with more than one ahead the ConcurrentRestore message could name a job that is not the one holding the application. Return the first job in restoreGoesFirst order, and pin the creation-time tie-break with cases where creation order and name order disagree. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
e4e991e837 |
fix(backups): skip the CNPG restore purge only for this restore's Cluster
The in-place CNPG restore skips deleting the live Cluster when it carries bootstrap.recovery and was created after the RestoreJob started, taking that as "this restore's own purge and re-render produced it". The timestamp does not say which restore rendered the Cluster. A Cluster deleted while the restore waits for the WAL archive is re-rendered by the still-active HelmRelease from the previous restore's values and is created after StartedAt too; the restore then skipped its purge and reported Succeeded on the previous restore's data. The restore patch already sets bootstrap.newServerName to restoredServerName(cluster, RestoreJob UID). The chart now annotates the Cluster with it as postgres.cozystack.io/restored-server-name, whether or not it renders the barman plugin that also carries the value, and the purge is skipped only when the annotation names this RestoreJob. The mark lives on the rendered Cluster rather than in RestoreJob status, so it still covers the failed status write the skip exists for. A Cluster rendered by a chart that predates the annotation keeps the creation-time check. Fixes #3322 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
3daf942dce |
fix(backups): refuse a restore into an application another restore holds
Nothing stopped two RestoreJobs from rewriting one application at once. Each restores the target from its own Backup, so the application ends up on whichever finishes last, and the CNPG driver's purge decision cannot tell which of the two produced the live Cluster: the second job could skip its purge and report Succeeded on the first job's data. A RestoreJob that has not started now fails with reason ConcurrentRestore when another unfinished RestoreJob in the namespace writes into the same application and goes first. A started restore goes before one that has not; between two that have not, the older one goes first, by creation time and then name, so a running restore is never failed by a newer one. The refused job fails instead of waiting, so the application is not restored a second time later, after the first restore, by a job its author may no longer expect to run. The gate lists RestoreJobs through the manager's API reader rather than the informer cache. Creation times have one-second precision, so two jobs created in the same second are ordered by name, and a job reconciled before the cache held its peer's StartedAt would see that peer as not started and proceed alongside it. The controller runs one worker, so a peer's StartedAt written by an earlier reconcile is on the API server by the time the next job reads it; SetupWithManager now states that. The target is resolved as the BackupJob hold resolves it, so a Velero restore into another namespace does not collide with one in place. Refs #3322 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Aleksei Sviridkin
|
f4bde9e50d |
fix(backups): delete the original PVC of an in-place VM restore without keepOriginalPVC (#4672)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `kind/backport` if it's a bugfix that needs to be
backported to a previous version.
-->
## What this PR does
An in-place VMInstance restore with `keepOriginalPVC: false` reports
`Succeeded` without restoring the disks, and leaves every disk it
touched unable to come back. `prepareForRestore` deletes each DataVolume
with Orphan propagation and leaves the PVC in place, on the assumption,
stated in the `deleteDataVolume` comment, that Velero overwrites it.
Velero does not overwrite a PVC. Its CSI restore action returns early
for a claim that already exists, so no DataDownload is ever created, and
with `existingResourcePolicy: update` the merge patch it attempts
instead is rejected because a bound claim's spec is immutable. All that
reaches the claim is the labels-only fallback patch, which also drops
the `cdi.kubevirt.io/allowClaimAdoption` annotation the resource
modifier added.
The claim left behind then blocks the disk for good. Orphan propagation
has removed its owner reference to the DataVolume, and it lacks the
`cdi.kubevirt.io/storage.populatedFor` annotation that
kubevirt-velero-plugin writes on the backed-up copy, so when the
HelmRelease resumes, the CDI webhook refuses the DataVolume it creates
with `Destination PVC <ns>/<name> already exists`, on every reconcile.
Deleting the disk through the API leaves the PVC behind, and a new disk
of the same name fails the same way.
Observed on a v1.6.3 cluster, for a VMInstance restore with
`keepOriginalPVC: false` whose disk was attached to the VM. The Velero
log for the restore, trimmed:
```text
level=info msg="Retrieved per-restore resource modifiers from configmap cozy-velero/restore-modifiers-..."
level=warning msg="PVC already exists and ExistingVolumeDataPolicy is not in-place restore. Skip restore this PVC." Action=PVCCSIRestoreItemAction PVC=tenant-datalab/vm-disk-e2edisk
level=info msg="Applying resource modifier patch on tenant-datalab/vm-disk-e2edisk"
level=warning msg="patch attempt failed for /v1, Kind=PersistentVolumeClaim tenant-datalab/vm-disk-e2edisk: PersistentVolumeClaim \"vm-disk-e2edisk\" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims"
level=info msg="backup/restore labels successfully updated for PersistentVolumeClaim tenant-datalab/vm-disk-e2edisk"
```
`velero restore describe --details` lists the PVC as `(failed)` and the
Restore is `Completed` with two warnings. Afterwards the claim had its
original creation timestamp and UID, no owner reference, no
`allowClaimAdoption`, no `populatedFor`, and the disk's HelmRelease was
`Ready=False` with the webhook error above. That cluster runs a Velero
build from `main`, whose wording is quoted above; the released v1.17.1
and v1.18.3 return early at the same point with `PVC already exists.
Skip restore this PVC.` (`pkg/restore/actions/csi/pvc_action.go`).
The fix deletes the PVC when `keepOriginalPVC` is false, only once its
DataVolume is confirmed gone so that CDI cannot recreate it, and holds
the Velero Restore until the claim has actually disappeared. A claim
still mounted stays `Terminating` behind `kubernetes.io/pvc-protection`,
and creating the Restore then would hit the same existing-claim path.
Velero then creates the PVC from the backup, the data mover fills it,
and the recreated DataVolume adopts it through `populatedFor`. A failed
deletion now fails the RestoreJob instead of letting it report
`Succeeded` over the old data. Before
|
||
|
Aleksei Sviridkin
|
b1192bfcbc |
fix(backups): create no driver object for a job that already finished (#4673)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `kind/backport` if it's a bugfix that needs to be
backported to a previous version.
-->
## What this PR does
The BackupJob and RestoreJob reconcilers decide a job is still active
from the informer cache, and the cache can trail their own writes. A
driver's first pass stamps `StartedAt`, which requeues the job, and may
then fail the job in the same pass on a transient error, for instance a
503 from the aggregated `apps.cozystack.io` API while
`createVeleroBackup` reads the application. The requeued pass can start
before the informer has seen `Failed`: it retries the step that failed
and, once the error has cleared, creates the driver object anyway. Its
own status write then conflicts on the stale resourceVersion, the next
pass sees `Failed` and skips the job, and a Velero Backup is left
running that no `Backup` artifact will reference and nothing tracks.
Reproduced against a real apiserver (envtest 1.34) with the real manager
cache and the reconciler wired through `SetupWithManager`, nothing else
writing the BackupJobs. The only fault injected is one 503 on the first
GET of each application through `apps.cozystack.io`. Out of 40
BackupJobs, every one ends `Failed` with `failed to create Velero
Backup: the server is currently unable to handle the request`, and on
`main` (
|
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
Mattia Eleuteri
|
06e72b26dd |
test(backups): give the restore stale-cache test a Completed Velero Backup
A Velero RestoreJob is about to require its Velero Backup to exist and be Completed before it halts the application. Without one in the fixture, the first pass of this test fails the RestoreJob before it suspends the HelmRelease, so the test would stop exercising the stale cache it is about. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Aleksei Sviridkin
|
f203a47afe |
fix(backups): hold a Postgres backup until the plugin rollout is over (#4674)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The first BackupJob on a Postgres app whose `cnpg.io/Cluster` has never carried the barman-cloud plugin — a fresh platform-flow app, or one an in-place restore re-rendered — attaches the plugin and creates the `cnpg.io/Backup` in the same reconcile. Attaching the plugin changes the instance pod spec, so CNPG rolls every instance (`Pod rollout required: volumes: element plugins has been added`). The Backup then lands on an instance without the plugin sidecar (`requested plugin is not available: barman-cloud.cloudnative-pg.io`) or on one CNPG restarts under it (`instance manager was restarted during backup`). CNPG does not retry a failed Backup, so the BackupJob stays Running in `CNPGBackupTransientFailure` until `cnpgDefaultBackupDeadline` and fails. The first backup of each such Postgres is always lost; this was observed on a v1.6.3 cluster and reproduced against `main`. The driver now holds the Backup back until that rollout is over. Before attaching the plugin to a Cluster that has none, it records `PluginRolledOut=False` on the BackupJob, with `Ready=False` reason `WaitingForPluginRollout` for the user, and requeues; the next pass attaches the plugin. The Backup is created once CNPG reports the Cluster in its healthy phase and every instance pod (`cnpg.io/cluster`, `cnpg.io/podRole=instance`) was created after the condition and is Ready; the condition then turns True and `Ready` is cleared, as before. Recording the condition first means a lost status write leaves the plugin unattached for the next pass instead of a Cluster that looks ready while it rolls. A Cluster that already runs the plugin is not rolled by the driver and its Backup is created on the first pass, as today. The wait is bounded by the existing `cnpgDefaultBackupDeadline`, after which the BackupJob fails with the phase or pod CNPG is stuck on. Why this signal, checked against the CNPG 1.29 source and the 1.30 Cluster CRD vendored here: the Cluster status has no `observedGeneration` and its `Ready` condition is derived from `status.phase`, so right after the attach the phase still reads healthy from before until CNPG reconciles the new spec. `status.pluginStatus` does not help either, since CNPG also lists plugins referenced only from `externalClusters` there, which is exactly the state of a Cluster a restore re-rendered. A pod spec change is never applied in place (only a pending Postgres restart is), so the instances are recreated and pods newer than the attach are the ones running the sidecar. This reads pod metadata and CNPG's documented labels only, not the plugin's container or volume names, and needs no RBAC change: the controller already lists and watches pods. Tests: `TestReconcileCNPG_WaitsForPluginRolloutBeforeBackup` drives a Cluster without the plugin through the attach, a stale healthy phase over the old pod, a rolling phase over the recreated pod, and the healthy phase, and asserts no Backup exists until the last step. `TestReconcileCNPG_AttachedPluginDoesNotDelayBackup` covers a Cluster that already runs the plugin, and `TestReconcileCNPG_PluginRolloutWaitIsBounded` the deadline. With the gate disabled, the first and third fail; with only the phase checked and not the pods, the first fails at the attach pass. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> Not applicable: no UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> The diff touches only `internal/backupcontroller` Go code: no package, values, chart, CRD, `hack/` tooling, telemetry metric or contributor convention. Walked against the trigger map, it reaches none of the repositories below, and the website documents no workaround for the lost first backup that this would make obsolete. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(backups): the CNPG backup driver no longer loses the first backup of a Postgres app whose Cluster had no barman-cloud plugin yet (a new platform-flow app, or one restored in place). It now waits for CNPG to finish rolling the instances onto the plugin before creating the cnpg.io Backup, and reports the wait on the BackupJob as Ready=False with reason WaitingForPluginRollout. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability** * Backups wait for the backup plugin rollout to complete and cluster instances to become healthy before starting. * If rollout does not finish before the backup deadline, the backup job fails instead of waiting indefinitely. * Backups can start without waiting for a new rollout when the plugin is already attached. * **Backup Configuration** * When no server name is configured, backups use the cluster name and application UID, giving recreated applications distinct archive prefixes. * Existing server names are preserved when the plugin is already attached. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
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> |
||
|
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> |
||
|
Mattia Eleuteri
|
eafa441b9c |
fix(backups): create no driver object for a job that already finished
The BackupJob and RestoreJob reconcilers decide a job is still active from the informer cache, and the cache can trail their own writes. A driver's first pass stamps StartedAt, which requeues the job, and may then fail the job in the same pass on a transient error, such as a 503 from the aggregated apps.cozystack.io API while it reads the application. The requeued pass can start before the informer has seen Failed: it retries the step that failed and, once the error has cleared, creates the driver object anyway. Its own status write then conflicts, the pass after it sees Failed and skips the job, and a Velero Backup is left running that no Backup artifact will reference and nothing tracks. Against a real apiserver and the manager's cache, with one 503 on the first application read of each job, 5 to 10 BackupJobs out of 40 ended Failed with a Velero Backup. A restore pass that starts on such a stale copy does worse, since the step it retries can be destructive: suspending the VM's HelmRelease and halting the VM before creating a Velero Restore, or patching the Postgres application and purging its cluster. Both reconcilers now re-read the job through the manager's uncached reader when the cached copy says it is still active, and skip it when the apiserver holds it as finished. The check sits ahead of the dispatch, so it covers every driver, and a job the cache already reports as finished costs no extra request. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
6d064a8602 |
fix(backups): delete a restored disk's PVC only once the restore can run
Deleting the original PVC of a keepOriginalPVC=false restore is only safe when Velero can put the disk back. The Backup artifact outlives the Velero Backup it points to, which Velero garbage-collects at TTL, and nothing checked it before prepareForRestore halted the VM and deleted the disks: a restore of an expired backup lost the volume where it used to fail with the disk intact. The Velero Backup must now exist and be Completed before any preparation starts. A DataVolume that survives its deletion makes CDI recreate the claim under the original name, which Velero then skips as an existing PVC, so the restore would end Succeeded on a freshly imported disk. A failed DataVolume deletion now fails the restore, and the PVCs are deleted only once every DataVolume is gone. Reading the DataVolume back needs get on datavolumes, which the ClusterRole did not grant. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
6ea713d09b |
fix(backups): delete the original PVC of an in-place VM restore without keepOriginalPVC
With keepOriginalPVC=false the restore deleted each DataVolume with
Orphan propagation and left its PVC in place for Velero to overwrite.
Velero never overwrites a PVC: its CSI restore action skips the data
mover for a claim that already exists, and existingResourcePolicy=update
fails to patch the immutable spec of a bound claim, so it only stamps
its labels on it. The RestoreJob then reports Succeeded with the disk
still holding its pre-restore data.
The claim left behind also breaks the disk for good. It has lost its
owner reference to the orphaned DataVolume, and it lacks the
cdi.kubevirt.io/storage.populatedFor annotation that the backed-up copy
carries, so the CDI webhook refuses the DataVolume the resumed
HelmRelease creates ("Destination PVC already exists"), on every
reconcile.
Delete the PVC once its DataVolume is gone, and hold the Velero Restore
until the claim has actually disappeared: one still mounted stays
Terminating behind pvc-protection, and Velero would take it for an
existing claim again.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
|
||
|
Mattia Eleuteri
|
54ab863c0e |
fix(backups): hold a Postgres backup until the plugin rollout is over
A Cluster that has never carried the barman-cloud plugin gets it from
the first BackupJob: a fresh platform-flow app, or one an in-place
restore re-rendered. Attaching it changes the instance pod spec, so
CNPG rolls every instance. The cnpg.io/Backup the same reconcile
created then runs on an instance without the plugin sidecar ("requested
plugin is not available") or on one CNPG restarts under it ("instance
manager was restarted during backup"). CNPG never retries it, so the
BackupJob sat in CNPGBackupTransientFailure until the backup deadline
and failed: the first backup of every such Postgres was lost.
The driver now records a PluginRolledOut=False condition before it
attaches the plugin, and creates no Backup until CNPG reports the
Cluster healthy and every instance pod was created after that
condition and is Ready. Recording it first means a lost status write
leaves the plugin unattached for the next pass, rather than a Cluster
that looks ready while it rolls. A Cluster that already runs the plugin
is not rolled by the driver and is not held back.
The phase alone cannot tell the rollout is over. Cluster status has no
observedGeneration and its Ready condition is derived from the phase,
so until CNPG reconciles the new spec the phase still reads healthy
from before the attach. status.pluginStatus is no better: CNPG also
lists plugins referenced only from externalClusters there, which is
exactly the state of a Cluster a restore re-rendered. A pod spec change
is never applied in place, so the instances are recreated, and pods
newer than the attach are the ones running the sidecar. That reads only
pod metadata and CNPG's documented labels, not the plugin's container
names. The wait stays bounded by the existing backup deadline.
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
Assisted-by: LLM
|
||
|
Aleksei Sviridkin
|
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.
```
|
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
Myasnikov Daniil
|
22a27e75ac |
test(operator): cover memory cleanup failures
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: LLM |
||
|
Myasnikov Daniil
|
e83b4eb492 |
fix(operator): align memory policy ownership messages
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: LLM |
||
|
Myasnikov Daniil
|
0d5b2e35c4 |
docs(operator): align scan comments with withholding
Replace three stale descriptions of the removed ceiling-raise behavior with the current withhold and withdraw semantics. Historical explanations of why the raise design was rejected remain intact. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
6a521f0c68 |
fix(operator): sweep memory defaults when disabled
List LimitRanges by the ownership label once per Package reconcile when the feature is disabled, and remove managed objects even in namespaces outside the current Package target set. Skip the per-namespace path while disabled and keep cleanup failures non-fatal to namespace reconciliation. Update the flag help, installer guidance, and operations documentation to describe the global cleanup boundary. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
492c9acdd0 |
fix(operator): ignore failed pods from finished jobs
Record terminal Job UIDs while scanning templates, then ignore only Failed pods owned by those Jobs. Failures from unfinished controllers and ownerless pods remain evidence because they may still be recreated. Also remove the stale constructor comment that still described raising a namespace ceiling. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
2e3c40b125 |
fix(operator): ignore non-pod LimitRange scopes
Kubernetes accepts memory keys on PVC-scoped LimitRange items when a storage bound is also present, but those quantities cannot affect pod admission. Restrict the foreign-policy guard to Container and Pod items. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
f6dcb69b61 |
refactor(operator): withhold instead of raising, and drop the grace period
The raise-and-grace machinery cannot work: LimitRanger writes the raised limit onto the very container that justified the raise, so at that container's next admission it carries a limit, the scan skips containers that have one, and the raise has erased its own evidence. The ceiling then drops on schedule and the workload is rejected permanently. Any such machinery approximates admission logic ahead of admission, and that is incomplete by construction: a LimitRange default is one number for a whole namespace and cannot mean "the configured limit, or this container's request, whichever is larger". Only per-pod conditional defaulting expresses that, which is a mutating webhook and not this. The justification for the machinery had also expired. It was designed when the default was 4Gi, where withholding was self-perpetuating for VPA-managed namespaces. At 32Gi nothing in the tree approaches the ceiling -- the largest static memory request is 2Gi -- and VPA sets requests and limits together, so those containers carry their own limit and were never blockers. So a namespace holding a container that requests more than the configured limit with no limit of its own is now left with no default at all, and anything written there earlier is withdrawn. It fails in the safe direction, which is the whole argument: a namespace with no memory.max is a namespace behaving as it did in every release before this, while a default below a request is a namespace whose workload cannot be admitted. It needs no stored state and no timer, because withholding is not a decision that has to survive between reconciles -- once the oversized request is gone the default is applied again on the next pass. Gone with it: the grace period, the clear-since annotation and its clamp, the injected clock, containerMemoryDefault, raisedCeiling, and fourteen tests. The scan stays, because deciding whether to apply or withhold is what it was always for. The foreign-LimitRange ownership guard, the disable path and the 32Gi default are untouched. The residual sharp edge is stated rather than machinery: a workload whose only evidence is a live Pod, built from a custom resource with no template this scan can read, can still be missed if the scan runs while that Pod is absent, and applying the default then rejects its recreation. At 32Gi that is unreachable in this tree. It becomes reachable if an operator lowers the knob below such a workload's request, which the flag help, values.yaml and the operations page now say plainly. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
d3df7c3d4d |
style(operator): gofmt stray blank lines in the package reconciler
Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
588ecbaeeb |
fix(operator): scan pods every reconcile, and key ownership on the label
The pod half of the scan was skipped once a namespace defaulted container memory at exactly the configured limit, on the argument that the shape the scan looks for could no longer be admitted there. That argument proves such a pod cannot come to exist. It does not prove one cannot be attempted, and a failed attempt leaves nothing behind: a workload built from a custom resource whose request grows above the limit after its namespace settled was rejected permanently, with no artefact anywhere that could justify raising the ceiling again. The premise was not sound even for existing pods, since LimitRanger reads LimitRanges through an informer and the shape is admissible in the window before the plugin sees the apply. The skip bought one namespaced List out of six and paid for it with a silent permanent-rejection mode, so it is gone along with memoryDefaultSettledAt and the two tests that pinned it. One of those tests carried the counterexample in its own fixture: a settled 4Gi LimitRange beside an 8Gi pod, asserted invisible. The coexistence guard identified its own object by name. The name is not reserved, so an administrator's LimitRange called cozystack-system-defaults was skipped by the guard and then overwritten by the forced apply, and because LimitRangeSpec.Limits is an atomic list that replaced every item in it, taking their max and cpu policy along. Ownership is now the managed-by label, which is what the delete path has always used. That leaves a same-named object with no label ambiguous: ours with the label stripped, or an administrator's. The two mistakes are not symmetric, so the ambiguous object is treated as foreign and left entirely alone rather than adopted. Re-stamping it with the label goes, and its test is replaced, because the same path reaches an administrator's object and adopting it means the disable path later deletes their policy. Declining to delete is no longer silent either. Also: containerMemoryDefault now takes the last matching item rather than the first, which is what LimitRanger applies, so a multi-item spec no longer reports a ceiling the cluster is not using. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
c6342bf885 |
fix(operator): bring the flag help in line with the guard and removal scope
The --help text still carried two claims that values.yaml and the operations page had already narrowed, and it is the one surface an operator reads without opening the tree. Both were wrong in the direction that costs the reader something. "A namespace where another LimitRange already defaults or caps container memory is left alone" described the guard before it was widened, so an administrator holding a container memory min, a maxLimitRequestRatio or any Pod-scoped bound would read it, expect the LimitRange to be applied there anyway, and get a namespace with no memory.max for a reason the help says does not apply. "Removes any previously created one" is the universal removal claim, and removal is scoped to namespaces an active Package still targets. Two comments corrected while here. constrainsMemory said it switched on the item's own maps "rather than a hand-picked few", which contradicts the hand-picked list immediately below it; what is actually true is that a field added upstream has to be added there too, which is the reason it has its own table test. And memoryDefaultSettledAt claimed the count of raised namespaces falls to none once the oversized requests are gone, which stopped being true when the scan started keeping Failed pods: a carcass Kubernetes never collects holds both the raise and that namespace's per-reconcile pod List open. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
81c12bc953 |
fix(operator): withhold on any foreign memory policy, not two fields
The coexistence guard treated a namespace as already-governed only when another LimitRange carried a container memory default or max, and said so in its comment. That is not the set of fields that collide with what this reconciler writes. A container min above the default request rejects the pod, and so does a maxLimitRequestRatio, since a 32Gi default against a 32Mi default request is a ratio of 1024:1 and no cap allows it. So the namespace-wide admission failure the guard exists to prevent stayed reachable through fields the guard did not read, and kube-system is where it is likeliest, this feature having deliberately brought it in scope. Enumerating the colliding fields does not fix it, because at the Pod scope the collision is not decidable here. Whether a per-container default of 32Gi breaches a pod-wide max depends on how many containers the pod has, and the pods in question do not exist yet: any arithmetic about them is a guess, and a wrong guess costs a namespace its entire pod admission. So the rule is ownership rather than arithmetic. If anything else in the namespace has an opinion about memory, at either scope and in any field, the namespace is not this reconciler's to default. Narrow still on the resource, because nothing here writes cpu or storage and a LimitRange bounding those must keep coexisting. This is the rule the tenant-namespace exclusion already rests on. The pod scan no longer skips Failed pods, only Succeeded. A pod that ran to completion is never recreated from its spec, but an eviction or a node crash leaves a Failed carcass holding the same oversized request that its controller will recreate, and that carcass is frequently the only surviving evidence the namespace needs a raised ceiling, since the shape the scan looks for cannot be admitted once the namespace has settled. Dropping it lowered the ceiling under a workload that was coming back, which is what the grace period exists to prevent. A grace stamp dated ahead of the clock is clamped to now. Unclamped it held the ceiling for the skew plus the grace period, silently, so an hour of skew bought ninety minutes instead of thirty. Clamping never shortens the wait, it only stops it being extended. The two flag defaults are named constants the flag registration and its test both read. The test previously duplicated the literal and still asserted 4Gi after the default moved to 32Gi, so it misinformed a reader about the shipped value and pinned nothing. The disable claim is narrowed to what the code guarantees. "Removes any previously created one" was universal, but the reconciler only runs for namespaces an active Package still targets, so a namespace whose Package was removed keeps its LimitRange. The flag help, values.yaml and the operations page now say that, and name the managed-by label for finding leftovers by hand. The test asserting a Pod-scoped memory max should be ignored is gone, since under the ownership rule a Pod-scoped memory bound withholds. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
30ee57f892 |
fix(operator): route every downward ceiling move through the grace period
The grace period guarded only the branch where the scan came back empty, which left the same defect standing in the branch where it finds something smaller than the ceiling already in place. A namespace raised to 16Gi for a pod visible only as a live Pod, with a second oversized pod at 8Gi beside it, loses the larger pod to a node drain. The scan still finds the 8Gi one, so the empty-scan guard never engages, and the ceiling was written straight down to 8Gi -- rejecting the larger pod's recreation exactly as writing the configured limit would have. A blocker smaller than the standing ceiling is not evidence that a larger one is gone. So the rule is now about direction rather than about why: the ceiling the evidence justifies is computed first, and any move that lowers it goes through the grace period, whichever of the three reasons prompted it -- an empty scan, a smaller blocker, or the configured limit itself being lowered beneath a standing ceiling. The eventual drop lands on what the scan justifies rather than on the configured limit, so a smaller blocker that is genuinely still there keeps the ceiling it needs. The raise is also no longer logged when it changes nothing. A namespace holding a permanently oversized pod re-justifies the same ceiling on every reconcile, and announcing that put a line in the log for every such namespace on every HelmRelease status change. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
9fe93a3d7e |
docs(operator): fix three claims that overstated what the code does
The static-pod note said containers built from an on-disk manifest are unaffected "in both directions". True of the container, which the kubelet runs from disk whatever the API says, but the mirror pod is an ordinary API object that passes admission like any other, so a static pod declaring a memory request above the ceiling with no limit of its own has its mirror pod rejected -- leaving the container running with no representation in the API. Worth naming, since kube-system is in scope by design. The scan does cover it: a mirror pod is a live Pod. The ReplicaSet exclusion was justified on live ones being copies of their Deployment's template. That does not reach an ownerless ReplicaSet, which at replicas 0 has neither pods to be seen through nor a Deployment template standing in for it. It stays out of scope -- nothing here ships a bare ReplicaSet -- but as a gap left open rather than one the stated reason closes. The delete guard's comment claimed the enabled apply always adopts and stamps a same-named foreign object. It does not when that object's spec already matches, because then the apply is skipped entirely. The outcome is the same one the guard gives on purpose, but it is reached by the apply not running. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
e131a49514 |
feat(operator): default the system namespace memory ceiling to 32Gi
The guard in front of this default reads declared memory requests. It cannot see a container's real working set, so a system component that requests little but legitimately uses more than the ceiling, and declares no limit of its own, was handed a deterministic kernel OOM on its next recreation in exchange for leaving the Talos handler's victim set. Since the feature ships enabled, that trade fell on the whole fleet on upgrade. It is not a trade this feature has to make. The handler discards a cgroup for carrying a memory.max at all, whatever its value, so immunity does not require a fitted ceiling -- only a real one. A default far above any plausible working set buys the immunity and leaves nothing for the ceiling to bind on, which also keeps it clear of the failure mode this repository already carries a scar from, where an undersized limit converted a rare pressure-driven kill into a reliable one. Nothing about scheduling changes: only requests are scheduled against, and the paired default request stays 32Mi. An operator who wants the ceiling to bind lowers it, per component in a chart or fleet-wide with this knob, once real usage is measured -- which is the direction that needs evidence, and now the direction that needs it rather than the default. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
ad3c9ef04e |
fix(operator): stay out of a namespace another LimitRange governs
The reconcile only read its own LimitRange by name, so a system namespace already carrying an administrator's LimitRange got a second one beside it. LimitRanger applies defaults from every LimitRange in the namespace and then validates the pod against every one, and nothing orders them, so the effective container ceiling moved between the administrator's value and this one for reasons no chart author could see. That is the same hazard this feature cites to keep itself out of tenant namespaces, and it does not stop being one because the namespace is a system namespace. A max is the sharper half. This reconciler writes a default and no max, so an administrator's LimitRange carrying a container memory max below the configured default had every container that declares no limit of its own rejected: the default is written first, then validated against the max it exceeds. That takes out a whole namespace, and it reaches kube-system, which this feature enters for cozystack-scheduler and which is where a distribution's or an administrator's own LimitRange is likeliest to sit. Such a namespace is now left alone and logged, and anything this reconciler wrote there earlier is withdrawn rather than left sitting beside the foreign object, since that object is half of the ambiguity. The check is deliberately narrow -- only a container memory default or max collides with what is written here -- because read more widely it would be a blanket opt-out that an unrelated cpu policy could trip. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
5630954e34 |
fix(operator): hold a raised memory ceiling before lowering it
A raised container memory ceiling was recomputed from the scan on every reconcile, so a single scan that found nothing lowered it back to the configured limit. An empty scan is not evidence that the oversized request is gone: it is equally consistent with the pod being between deletion and recreation. For a blocker that appears only as a live Pod, which is a class this repo ships -- every CNPG Cluster in a system namespace builds its instance pods directly rather than through a StatefulSet -- an ordinary Talos node drain opens that window, and reconciles follow HelmRelease status churn, of which a node upgrade produces plenty. The ceiling was lowered under a pod that was merely being rescheduled; its recreation was then defaulted a limit below its own request and rejected at admission for good, because the rejected pod never exists so no later scan can find it, and the namespace settles at the configured limit where the pod half of the scan stops running at all. The drop was not even logged. Lowering now waits until the same emptiness has held for memoryCeilingDropGrace, with the instant it was first observed recorded on the object so it survives an operator restart and a change of leader. A wall clock rather than a count of consecutive clean scans, because the reconcile rate rises exactly when the risk does and any small count can be burned inside one rescheduling window. A blocker seen again cancels the countdown, an unreadable stamp restarts it, and both the hold and the drop are logged. The steady-state comparison now also covers the two metadata fields this reconciler manages, because the grace stamp is one of them. That incidentally fixes the managed-by label never being re-stamped when only metadata had drifted, which had left turning the knob off unable to remove an object the operator did own. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
3c1d8d6988 |
docs(operator): bound the managed-by guard to the disabled state
The comment on the label check in deleteSystemDefaultsLimitRange reads as a general promise not to touch an administrator's own LimitRange of that name. It is narrower than that, and the difference is worth writing down: while the feature is enabled the apply takes the name over with ForceOwnership and stamps the managed-by label itself, so a hand-written cozystack-system-defaults is adopted and then deleted here when the knob goes off. That is the intended SSA behaviour, not a defect. What the label actually buys is that a LimitRange of that name the reconciler never wrote to survives the knob, which is a real guarantee and a smaller one. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
accb4f9e1b |
refactor(operator): drop the dead error check from the settled-state guard
The guard read `err == nil && apiequality.Semantic.DeepEqual(...)`, which looks like it consults the earlier Get and does not. That Get's error was reassigned by the scan two statements up, and the scan's non-nil case has already returned, so the clause is always true by the time it is evaluated. It was harmless only by accident: a namespace whose Get returned NotFound carries an empty spec, which never compares equal to the desired one, so the apply happened anyway. Removing the clause makes that the reason rather than a coincidence sitting behind a condition that cannot fire. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |