mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 14:02:56 +09:00
fix/select-e2e-find-not
485
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 IvanHunters
|
b0e07cadbb |
fix(operator): rebuild each changed ArtifactGenerator generation once (#4594)
<!-- 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 After an upgrade, an ArtifactGenerator can report Ready on its new spec while its artifacts were built from the old one, and source-watcher never rebuilds them. That is how the bucket-system chart in #4593 shipped without cozy-lib. The cause is upstream (fluxcd/source-watcher#384). source-watcher builds from the spec it read when the reconcile started, but the final status patch stamps Ready with the live generation. An upgrade changes the packages source and the copy rules within seconds, so a spec change often lands during a build. The generator status has nothing that tells a stale build from a good one. So the operator now rebuilds every changed generation once, after source-watcher first reports it Ready, through the force-drift path the operator already uses for fluxcd/pkg#934. An unchanged spec rebuilds to the same digest, so ExternalArtifacts and HelmReleases do not move. Generation 1 is skipped, since the race needs a spec change during a build. On the first rollout every generator past generation 1 gets one extra rebuild, and its PackageSource shows `Unknown/AwaitingSourceWatcherRecovery` until that rebuild lands. Nothing waits on that condition today. The same rebuild repairs clusters already stuck like #4593. The recovery messages on the PackageSource no longer blame fluxcd/pkg#934, because the same path now serves both cases. ### How it was tested Unit tests in `internal/operator/packagesource_reconciler_test.go` cover the forced rebuild on generation 2 and on a later generation, every case where no rebuild must happen, the rebuild settling without a second one, both failure orders (force fails, recording the generation fails), and a stale recovery counter. Each condition of the new predicate was mutated once to confirm a test fails. I also ran this commit's operator image on an arm64 test cluster running v1.7.0-alpha.3, the one where #4593 was found. Before that I had repaired its bucket generator by hand: deleting the stale ExternalArtifact and requesting a reconcile produced an artifact with `charts/cozy-lib` and both bucket HelmReleases went Ready. With the patched operator, the only generator past generation 1 got exactly one forced rebuild and the verified-generation annotation. source-watcher reported all its ExternalArtifacts up to date. The digests of all 214 ExternalArtifacts and the revisions of all 104 HelmReleases matched a snapshot taken before the swap, and every PackageSource and HelmRelease stayed Ready. The race itself was not reproduced on the cluster, since that needs a spec change landing during a build. Closes #4593 ### 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. --> ### 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. --> - [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(operator): rebuild an ArtifactGenerator once after each spec change, so an upgrade no longer leaves a component on artifacts built from the previous spec (for example a chart missing a library it now needs). ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved PackageSource recovery when an artifact generator reports ready: the system now verifies the generated output and triggers a rebuild when needed. * Failed rebuild or verification attempts can be retried during a later reconciliation, and recovery tracking is cleared once the rebuilt generation settles. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
2df578bffd |
test(e2e): read the opensearch release history through the pinned name
The opensearch suite wrote the history jsonpath out as a literal, so the contract test that runs HELMRELEASE_HISTORY_JSONPATH against the upstream Flux type never saw the expression this suite hands kubectl. A field rename upstream would have reached it only as an empty read. The suite now takes the shared expression, and the contract test reads every Chainsaw suite that calls the guard, so a suite copying the literal, or assigning the shared name after sourcing the library, fails in the unit lane instead of going unchecked. The read is matched inside the script step that calls the guard, because every step runs in a shell of its own and a read in another step never reaches the call. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
2eeaa59055 |
test(controller): seed the DataVolume informer for controller-runtime 0.25
controller-runtime 0.25 made FakeInformer's sync state a private channel, so the DataVolume watch tests no longer compile: they toggled the exported Synced field and its lock. FakeInformers also builds every informer it creates already synced, so an unsynced informer has to be seeded into InformersByGVK and marked synced with Synced() later. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
bf03c84711 |
chore(deps): move to the Kubernetes 1.37 libraries without the apimachinery fork (#4473)
## What this PR does This moves the root module's `k8s.io/*` from v0.35 to v0.37.1 and controller-runtime from v0.23 to v0.25.1, and drops the `k8s.io/apimachinery` fork on the way. Supersedes #3001, #4467 and #4472; those were a stack of the same commits. The four commits are meant to be read one by one, and each builds and passes the tests on its own: 1. **Plain watches stop getting the initial-events-end bookmark.** The apiserver defaults `sendInitialEvents=true` onto any watch with an empty or `0` resourceVersion and leaves bookmarks off, and our aggregated storages took that flag alone as a WatchList request. From `k8s.io/apiserver` v0.36 the watch handler compiled into cozystack-api panics on that bookmark, so `kubectl get -w` on any cozystack-api resource would die after the bump. The marker now needs both flags, the same condition the watch cache uses. There are tests per storage and one through the real handler chain. 2. **The apimachinery fork goes.** Every apps kind shares the `Application` Go type, and stock `Scheme.ConvertToVersion` picks the first kind registered for it. The fork patched that; the apps group now encodes through a serializer that keeps the kind the object carries. A handler-chain test covers create, get, list, update, watch, apply and table for two kinds and fails without it. 3. **MutatingAdmissionPolicy stays out of admission.** From v0.36 the plugin is on by default and refuses every write until it has synced `mutatingadmissionpolicies`, which cozystack-api has no RBAC to list and which clusters before 1.36 do not serve. Without this, every create, update and delete would fail after the bump. It is also re-applied in `Complete`, since `--disable-admission-plugins` replaces the list. 4. **The bump itself**, with the regenerated clients, openapi and strategy CRDs. Commits 2 and 4 are where the review effort goes; 1 and 3 are small. Known changes in behaviour: - Without the fork, the row objects of a Table requested with `includeObject=Object` carry the first registered app kind. kubectl asks for that only with `--sort-by`, and it sorts on fields, not on the kind. Nothing at this layer can fix it, because the handler converts through `APIGroupInfo.Scheme`, a concrete `*runtime.Scheme`. Admission webhooks and ValidatingAdmissionPolicy still see the right kind. - The alpha PodSpec field `workloadRef` became `schedulingGroup` upstream. Once the new strategy CRDs are applied, the apiserver prunes `workloadRef` from stored strategies it serves, and their next write erases it. It only ever worked behind an alpha feature gate. The branch is rebased onto the Renovate rollup (#4447), with `go mod tidy` and codegen rerun on top of it. ### Screenshots No UI changes. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note chore(deps): cozystack-api and the controllers build against the Kubernetes 1.37 libraries without the apimachinery fork. Plain watches no longer receive the WatchList initial-events-end bookmark, and objects in Table rows requested with includeObject=Object may carry another app's kind. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Application API responses now preserve the requested resource kind across operations, including lists and watches. * Backup strategy resources support additional Kubernetes pod settings, including probe protocols, volume ownership options, and scheduling groups. * Backup strategy schemas include updated Kubernetes pod fields and descriptions. * **Bug Fixes** * Initial-event watch bookmarks are sent only when requested; plain watches continue without an ending bookmark. * Mutating Admission Policy is disabled by default, including when admission plugin options are customized. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
9809b22280 |
fix(vm-disk): report DataVolume readiness through WorkloadsReady (#4380)
## What this PR does Nothing on a vm-disk application tells whether CDI ever finished populating the disk. Its WorkloadMonitor has no pods to count, and `Ready` follows the Helm release. So an import or clone that never completes can leave a green application over a disk nobody can use. cozystack/cozystack#2908 is one way to get there: a clone into a StorageClass that cannot place a replica where the source data sits. The expensive part there was the silence, not the stuck clone. The disk state was in the DataVolume phase all along, and nothing read it. With this change the WorkloadMonitor in cozystack-controller reads it. The monitor lists the DataVolumes its selector matches and reports not operational while any of them is in flight, Failed, Unknown or has no phase. An upload counts as in flight until it completes. That surfaces on the application as `WorkloadsReady=False` with reason `DataVolumeNotReady` and a message that names the DataVolume and its phase, for example `DataVolume vm-disk-x is ImportInProgress`. `Ready` still follows the HelmRelease, as for any other kind (`convertHelmReleaseToApplication` in `pkg/registry/apps/application/rest.go`), and for a vm-disk it no longer waits on the DataVolume at all. So the disk state has to be visible outside `Ready`, and this PR puts it where operators look by default: - `kubectl get` on any application kind gets a `WORKLOADS` column next to `READY`. It shows the `WorkloadsReady` status, or `<none>` when the application has no WorkloadMonitor. - The console badge turns amber and reads `DataVolumeNotReady` while `Ready` is True and `WorkloadsReady` is not True for that reason. A failing `Ready` still wins. The console shows only the disk case: other kinds need a separate decision, because a stopped VM legitimately has no pod and reports `WorkloadsReady=False` too. A Helm readiness gate on the DataVolume would be the obvious alternative, and it is not used on purpose. helm-controller runs that wait inside its reconcile, so an importing disk would hold one of the shard's worker slots for the whole install window, and a disk stuck for good would keep taking the slot back. cozystack/cozystack#4145 has the details. The monitor costs no slot, keeps watching after install, and covers existing disks without anyone writing to them. Without any gate, the install would still wait on the DataVolume. helm-controller's default poller falls back to a resource's `Ready` condition for kinds it has no rule for (`checkReadyCondition` in fluxcd/cli-utils `pkg/kstatus/status/status.go`), so a vm-disk install could hold a worker slot for the whole install timeout. The ApplicationDefinition therefore gets a `healthCheckExprs` entry for DataVolume with `current: "true"`, the same idiom postgres-rd uses for its ScaledObject. With it Helm never waits on the DataVolume. The entry is pinned by a helm-unittest in `packages/system/vm-disk-rd`, which gets a `test` target for it. Release settings reach a HelmRelease only when the application is created or updated, so an existing disk gets the entry on its next update. The reconciler that syncs definitions onto existing releases copies chartRef, valuesFrom and labels only. That affects any kind with release settings, so it is filed as cozystack/cozystack#4409 and not fixed here. `WorkloadsReady` covers every disk right away, since the monitor needs no write to the application. CDI copies the DataVolume labels onto the PVC it creates and makes the DataVolume its controller, so the monitor also publishes a `pvc-<name>` Workload for the disk. That Workload reads not operational while its DataVolume is not ready, even though the PVC is already Bound, so `kubectl get workloads` agrees with `WorkloadsReady`. While the DataVolumes cannot be read it keeps the verdict it has, like the monitor. The WorkloadMonitor CRD now requires at least one selector label. An empty selector would match every Pod, PVC, Service and DataVolume in the namespace. No chart renders one. When an application has several monitors that are not operational and only some of them name a cause, `WorkloadsReady` carries the named messages and reason only. A vm-disk has one monitor, so it does not hit this. ### Which phases count as ready Settled phases pass: `Succeeded`, `PendingPopulation`, `WaitForFirstConsumer`, `Paused`. A disk with `bindImmediately: false` on a `WaitForFirstConsumer` class rests in `PendingPopulation` or `WaitForFirstConsumer` until a VM consumes it. A multi-stage import rests in `Paused` until its next checkpoint, and the vm-disk chart renders no checkpoint source. These report ready while holding no data, because that is where such a disk is supposed to sit. `UploadReady` is not one of them. CDI enters it when the upload server pod is running and ready, and leaves it for `Succeeded` only when that pod succeeds (`updateStatusPhase` in `pkg/controller/datavolume/upload-controller.go` at v1.66.1), so the one phase covers a disk nobody has uploaded to and a half-uploaded one. A `source: upload` disk therefore reports `WorkloadsReady=False` from creation until `virtctl image-upload` completes. A phase the controller does not know also reads as ready, so a CDI release that adds or renames a phase cannot turn working disks red. The cost goes the other way: a renamed in-flight phase silently reads as settled. The list wants a re-read whenever the CDI version moves. It matches the named phases of the CDI v1.66.1 API, the version vendored in `packages/system/kubevirt-cdi-operator`. ### Clusters without CDI CDI comes with the IaaS bundle, while cozystack-controller runs everywhere, and CDI can be installed while the controller is running. A watch on a kind whose CRD is absent never syncs, and then the whole controller does not start. So the watch is not set up at startup. A runnable adds the DataVolume watch once the kind is served. It waits for the first reconcile, because only from then on does a new watch start at once, and then checks discovery right away and every 30s after that. It installs the DataVolume reader only after the DataVolume informer has synced within a timeout, because a reconcile context has no deadline and a List on an informer that never syncs would block every WorkloadMonitor reconcile. The watch source is started once: a controller-runtime `Kind` source adds an event handler to the informer and never removes it, so on a timeout the next check waits on the source it already has instead of starting another one. RBAC needs no change: the controller's ClusterRole already has read on everything. When the DataVolumes cannot be read, because the reader is not installed yet or the List fails, the monitor still publishes its replica counts and keeps its last DataVolume verdict, and a List error is returned so the request is retried and recorded as a `DataVolumesUnavailable` Warning event on the monitor. The status of a monitor without DataVolumes is not affected by a failing List. A disk whose verdict is already recorded does not read as ready in the window after a controller restart. A monitor with no recorded verdict yet, which is every monitor right after the upgrade that brings this change and any disk created right after a restart, reads ready until the DataVolume watch has synced. Because the watch starts on the first reconcile rather than on the next discovery tick, that window is the time to sync the DataVolume informer plus at most one 5s requeue, not a 30s poll interval. The window is not zero, since a monitor reconciled in it is published before its DataVolumes can be read. From then on, a reconcile that runs before the watch has synced is requeued after a few seconds, so the DataVolume events that queue it are not lost. A pass that changes nothing in the monitor status does not write it, so that requeue sends no status writes to the API server. Once the reader is installed, every monitor that holds a DataVolume verdict is queued once more. So a verdict whose DataVolume was deleted while the controller was down is recomputed then, not at the periodic resync. ### Tests The table test in `workloadmonitor_controller_test.go` covers the named phases of CDI v1.66.1, plus Failed, Unknown, an empty phase and a missing status. A second table checks the message and reason for an importing, a failed, a phaseless and a populated disk, and a third test lists two stuck DataVolumes in both orders and expects one message. It also covers a DataVolume outside the selector, the kind not being served, a List error that still publishes the replica counts, and a missing reader that keeps the last verdict. The watch tests check that the watch starts on the first reconcile and not before it. A source that never syncs leaves no reader and is retried on the same source, with the reconciles in between still requeued. A List failure records the Warning event, the sync window records none, and an unchanged pass writes no status. A manager that stops during the wait gets the cancellation back, and a reconcile that runs while the watch syncs is requeued without an error. A table test covers the PVC Workload: importing, populated, a PVC with no DataVolume controller or a controller of another kind or group, and the unread cases. A helm-unittest in `packages/system/cozystack-controller` pins the selector's `minProperties`. `rest_table_test.go` pins the `WORKLOADS` column, `rest_workloads_test.go` the condition message and reason, and `status.test.ts` the console badge, including a halted VM that stays green. In `hack/e2e-chainsaw/vminstance` the existing VMDisk scenarios now wait for the DataVolume to be Ready, then for `Ready` and `WorkloadsReady` on the VMDisk itself. That replaces the wait on the HelmRelease. Those asserts would pass even if the monitor ignored DataVolumes, so a new scenario imports from an `.invalid` URL. It first waits for the importer to have failed, which CDI reports on the DataVolume as `Running=False` with reason `Error`, and then for `Ready=True` with `WorkloadsReady=False` and reason `DataVolumeNotReady` on the VMDisk. Without the first wait the VMDisk assert passed in about a second, on a DataVolume that had no phase yet. The e2e runs on a fresh install only, so nothing here exercises an existing release across an upgrade. ### What it does not do #2908 lists possible fix directions, and none of them is here. The cross-class clone still never completes, it just stops being invisible. Keep the issue open. While the DataVolume watch syncs, every monitor is still requeued. Only the ones whose selector reaches a DataVolume need it, but the controller cannot tell which those are before it can read DataVolumes. Closes #4449 ### Screenshots There is no screenshot: I had no running console to take one from. The badge change is pinned by the vitest cases in `packages/system/dashboard/images/console/apps/console/src/lib/status.test.ts`: a VMDisk whose `WorkloadsReady` has reason `DataVolumeNotReady` goes amber, and a halted VMInstance with the generic reason stays green. ### Downstream repositories I walked the trigger map against the diff. cozystack/terraform-provider-cozystack is affected: its `wait_for_ready` reads only `Ready`, so on a `cozystack_vmdisk` it returns before the disk is populated once this lands. That needs a decision on the provider side, so it is an issue there. The release-time bot regenerates `packages/apps/vm-disk/README.md` into the website's virtualization pages, and `vm-disk` is in the `VMS` list that generator reads, so the website needs no follow-up. The ApplicationDefinition keeps its kind, plural, prefix and path. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: https://github.com/cozystack/terraform-provider-cozystack/issues/59 - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(vm-disk): report whether CDI finished populating a disk in the WorkloadsReady condition, which names the stuck DataVolume and its phase. kubectl get shows WorkloadsReady in a new WORKLOADS column for every application kind, and the console badge turns amber while a disk is not populated, so a disk whose import or clone never completes no longer looks healthy. Ready of a new or updated vm-disk no longer waits for the DataVolume, so wait on WorkloadsReady to know the disk is populated. ``` |
||
|
IvanHunters
|
339be8d42d |
fix(vm-default-images,kubernetes-worker-image,vm-disk): wait for golden images to import and refuse cloning a failed one (#4392)
## What this PR does Both golden image catalogs, `vm-default-images` and `kubernetes-worker-image`, went Ready before any image had imported. Each one renders a DataVolume per image with immediate bind, so every import starts at install, but neither PackageSource had `healthCheckExprs`. The HelmRelease went Ready as soon as helm applied the DataVolumes, and an image that failed or stalled stayed green. This PR makes both releases wait for the imports. Each component's `install:` block now has a DataVolume health check. `current` holds only on `Succeeded`, and `failed` holds on `Failed`. Any other phase, `Unknown` included, matches neither, and flux keeps waiting. A DataVolume with no `status` key makes the CEL evaluation error out. Flux reports that as Unknown and keeps waiting too. The other settled phases are safe to treat as not ready for these charts. `PendingPopulation` and `WaitForFirstConsumer` are where a volume waits for a consumer, and immediate bind removes that wait. `UploadReady` and `Paused` belong to upload and multi-stage sources, and both charts render only `source.http`. There is no per-component timeout, so I did not set one. The window is the operator-wide `--helmrelease-install-timeout`, 10m by default. The operator sets `RetryOnFailure` on both install and upgrade, so a download that outlasts the window is retried as an upgrade until the import finishes. Nothing uninstalls the DataVolumes in between, and the import keeps running. One image that never imports keeps its whole catalog not Ready until the image imports or its entry is removed. There is no time limit, and I kept it that way on purpose. A group/kind health check cannot tell one image from another, so it cannot be made per image. A per-component timeout would be an API change for one opt-in package. Both charts' `values.yaml` document this. Nothing on `main` lists either package in `dependsOn`, and the images that did import stay usable in the meantime. Tenant worker pools look up each golden directly and never wait on the catalog's status. The price is a helm-controller slot. The operator labels the generated HelmRelease with `cozystack.io/package`, plus `cozystack.io/privileged` when privileged. The flux-shard-operator acts only on HelmReleases that already carry `sharding.fluxcd.io/key`. So both catalogs go to the system helm-controller, which picks up releases without that key and runs with `--concurrent=5`. A DataVolume in `Failed` ends each attempt at once and costs next to nothing. An import that stalls without reaching `Failed` is the expensive case, for example an importer that keeps retrying an unreachable URL. Each attempt waits out the full window, and the next one starts 30s later. So one of those five slots stays busy about 95% of the time for as long as the image is stuck. #4145 describes the same cost on the shard controllers. The status change alone does not protect a vm-disk, because vm-disk clones from a golden without checking it. A golden still importing is fine, since CDI holds the clone until the source is populated. A golden whose import has Failed is never populated, so the clone would wait forever. vm-disk now refuses to render a new disk on a Failed golden and names the image. This matches the check the worker pools already have. A disk that already exists is exempt, so a golden that fails later does not break that disk's upgrades. The check fires only when CDI sets Failed, and at CDI v1.66.1 an import reaches Failed only on a checksum mismatch. The goldens carry no checksum, so a broken URL keeps the importer retrying and the new disk still waits, with only the catalog's not-Ready status pointing at the image. #4564 tracks that gap. A golden that does not exist at all is a separate gap, tracked in #4548. Tests pin the expressions for both catalogs. A helm-unittest case checks them on the rendered PackageSource. A Go test builds the HelmRelease spec from the shipped file and checks that they arrive with the poller wait strategy. The repo has no harness that evaluates flux CEL, so the phase behaviour above is argued from flux's evaluator and not exercised by a test. The vm-disk check has helm-unittest cases for a Failed, a Succeeded, and a still-importing golden, for a render with no cluster, and for an existing disk. Closes #4169 ### 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. --> No UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(vm-default-images, kubernetes-worker-image): the golden image catalogs now report Ready only after their images have finished importing, and a failed or stalled import keeps the catalog not Ready. A vm-disk created from a golden image whose DataVolume is in phase Failed is now refused with an error naming the image, instead of waiting forever on a clone that cannot complete. ``` |
||
|
Aleksei Sviridkin
|
ace69cb075 |
docs(controller): say what a nil DataVolume reader means
The field comment still said DataVolumes do not count while the reader is nil, and that the reader appears once the kind is served. Neither holds: the reader is installed only after the informer has synced, and until then a monitor keeps its stored verdict, which still decides Operational. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a6a32d79cd |
fix(controller): recompute stored DataVolume verdicts once the reader is in
A reconcile that runs before the DataVolume watch starts finds no reader, keeps the monitor's stored verdict and is not requeued. The watch's initial replay then delivers only the DataVolumes that still exist, so a monitor whose DataVolume was deleted while the controller was down kept reporting it not ready until the periodic resync, hours later. Installing the reader now queues every monitor that holds a DataVolume verdict, through the controller's own queue, so each is recomputed against the synced cache. Monitors without a verdict are left alone: the replay already reaches every DataVolume they could select. If the monitors cannot be listed, the next discovery poll retries. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
694f4f2ff9 |
fix(controller): report a disk's PVC Workload by its DataVolume
CDI copies the DataVolume labels onto the PVC it creates, so the vm-disk monitor also publishes a Workload for that PVC, and its readiness was bind state alone. Mid-import the PVC is already Bound: the Workload read operational while the monitor, and the Application's WorkloadsReady, said the disk was not populated. A PVC whose controller is a DataVolume the monitor finds not ready now reads not operational. While the DataVolumes cannot be read, its Workload keeps the verdict it has, as the monitor does. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
366b5e80f4 |
perf(controller): skip the monitor status write when nothing changed
While the DataVolume watch syncs, every WorkloadMonitor of every kind is requeued every few seconds, and an informer that cannot sync keeps that window open. Each pass wrote status unconditionally, so a cluster with many monitors sent the API server a steady stream of writes that changed nothing. Comparing against the stored status first leaves the requeue as cache reads only. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
dd72583dfd |
fix(controller): record an event when DataVolumes cannot be read
A failed DataVolume List keeps the monitor's last verdict and retries, and the only trace of it was a controller log line. A monitor with no verdict yet then reads operational while its disks are unreadable, with nothing a tenant can see saying why. A Warning event on the monitor makes the failure visible where the bucket metrics failure already is. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
beeb7994cc |
fix(controller): start the DataVolume watch source only once
A controller-runtime Kind source adds an event handler to the informer when it starts and never removes it; cancelling it on a sync timeout only ends its context. Building a new source on every failed sync attempt therefore left one more handler on the DataVolume informer per retry, each enqueueing the same events, until the controller restarted. The source is now started once and later attempts only wait for the informer to sync. Because the started source keeps delivering events between attempts, the syncing flag stays set until the reader is installed, so a reconcile queued in that gap is requeued as well. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a9b276f99b |
fix(controller): requeue monitors reconciled while the DataVolume watch syncs
Starting the DataVolume source replays every DataVolume to its handler at once, and the handler queues the matching monitors, while the reader is installed only after the source has synced. A monitor reconciled in that window saw no reader, kept its last verdict and returned with nothing left to queue it again, so a disk that failed or finished while the controller was down could keep a wrong WorkloadsReady until the next unrelated event. Such a reconcile still publishes the replica counts and now asks to be requeued after a few seconds, until the reader is in place or the sync attempt gives up. It is a requeue rather than an error because the window is expected: an error would count as a reconcile failure and log a line for every monitor in the cluster while it lasts. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
99f3d3b2b7 |
feat(controller): tag a DataVolume cause with a condition reason
WorkloadsReady=False covers different situations: a stopped VM has no pod and its monitor reports it not operational on purpose, while a disk whose DataVolume never populates is broken. A consumer that wants to react to one and not the other had only the message text to go by. The WorkloadMonitor status gains a Reason next to Message, set to DataVolumeNotReady by the DataVolume check, and the Application API carries it into the WorkloadsReady condition in place of the generic reason whenever a monitor that is not operational names one. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
81ffcda86f |
fix(controller): keep publishing monitor status when DataVolumes are unread
The DataVolume List sits on the reconcile path of every WorkloadMonitor, and returning its error before the status write froze the replica counts and Operational of a monitor that selects no DataVolume at all whenever CDI's API hiccupped. The status is now written either way, with the last DataVolume verdict kept whenever the DataVolumes could not be read, and the error still returned so the request is retried. The same applies while no reader is installed yet, so a stuck disk does not read as ready for the window between a controller restart and the DataVolume informer syncing. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
48ece79ffc |
feat(controller): name the DataVolume that keeps a disk not ready
For a vm-disk, WorkloadsReady is the one condition that reports whether CDI populated the disk, and its generic "One or more workloads are not operational" gives an operator nothing to act on: a disk has no workloads, and the stuck DataVolume and its phase are exactly what they need to know. The WorkloadMonitor status gains a message that the controller fills with each selected DataVolume that is not ready and its phase, and the Application API uses the messages of the monitors that are not operational in WorkloadsReady instead of the generic text whenever one of them names a cause. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
bf7e8f5bbb |
fix(controller): count DataVolume readiness in WorkloadMonitor
A vm-disk application reported its workloads operational whatever state its DataVolume was in, so an import or clone that never finished left the disk looking healthy while it was unusable. Its monitor has minReplicas 0 and only pods fed Operational. The monitor now also lists the DataVolumes its selector matches and reports not operational while any of them is in flight, Failed, Unknown or without a phase. UploadReady counts as in flight: CDI holds it from the moment the upload server is ready until the transfer succeeds, so it covers an empty disk and a half-uploaded one alike. Waiting for the first consumer counts as ready, since that is where such a disk rests, and so does a phase this controller does not know, so a CDI release that renames one cannot turn working disks red. This is deliberately not a Helm wait: a readiness gate holds a helm-controller worker for its whole window, and a disk that never populates would keep taking it back. The signal lands in the application's WorkloadsReady condition instead, and Ready keeps following the HelmRelease. CDI ships with the IaaS bundle only and can be installed while this controller is running. A watch on a kind whose CRD is absent keeps the controller from starting, so the DataVolume watch is added from a runnable that polls discovery until the kind is served. It starts on the first reconcile, the earliest point at which a new watch is started at once, rather than on the next tick, so a disk created just after a restart is not read as populated for a poll interval. The reader is installed only once that watch has synced within a bound: a reconcile context has no deadline, and a List on an informer that never syncs would park every WorkloadMonitor reconcile for good. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
2df29639df |
fix(clickhouse, workloadmonitor): add the cluster domain to keeper hosts and stop a PVC nil dereference (#4608)
## What this PR does Two small bug fixes, one per commit. Both were first proposed by @officialasishkumar in #2334 and #2336. Those PRs went stale, and part of each was fixed on main in the meantime, so this carries only what is still broken. ClickHouse Keeper hosts rendered without a cluster domain, as `chk-<name>-keeper-cluster1-0-0.<namespace>.svc.` with nothing after `svc.`. The zookeeper block in `clickhouse.yaml` redeclared `$clusterDomain` from `.Values.clusterDomain`, which the chart never defines, and that shadowed the value read from `_cluster.cluster-domain` at the top of the template. Keeper is enabled by default, so every ClickHouse release got these hosts. The keeper name test set `clusterDomain` itself and hid the bug. It now sets only `_cluster.cluster-domain`, like the platform does, and fails without the fix. The WorkloadMonitor controller panicked on a PVC with no `storageClassName`, which is any PVC that takes the cluster default class. The check used `||` where it needed `&&`. controller-runtime recovers the panic, but the reconcile of that monitor fails on every retry and never reaches its status update. A PVC with an empty `storageClassName` also got a resource key with an empty class name instead of `default`. A new table test covers unset, empty and explicit classes. The duplicate `template` key in the ClickHouse backup CronJob from #2334 and the `Operational` status fix from #2336 are already on main, so they are not in this PR. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(clickhouse, workloadmonitor): ClickHouse Keeper node hostnames now include the cluster domain, and the WorkloadMonitor controller no longer panics on a PVC without a storage class. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Workload resource labels now use the default storage class when a PVC’s storage class is unset or empty, while continuing to reflect explicitly configured storage classes. * ClickHouse ZooKeeper hostnames now consistently use the configured cluster domain, or `cozy.local` when no domain is set. This helps hostnames align with the cluster’s domain configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
mattia-eleuteri
|
250f89dc72 |
feat(scheduler): set scheduling-class label on mutated pods
The lineage-controller-webhook now sets a scheduler.cozystack.io/scheduling-class label on mutated pods (in addition to the existing annotation of the same name). This unlocks SchedulingClass authors writing podAffinity rules whose labelSelector targets pods across different applications that share the same class. A natural pattern becomes: define a class whose name is the affinity-group identifier, attach it to several applications, and have the scheduler co-locate them at the chosen topology key (region, zone, hostname). Behavior preserved when the SchedulingClass CR is missing: pods are left untouched (no scheduler injection, no label) so they do not stay Pending on a non-existent scheduler. Tests cover label/annotation/schedulerName injection, existing label preservation, non-Pod no-op, no-class no-op, and missing-CR no-op. Signed-off-by: mattia-eleuteri <mattia@hidora.io> |
||
|
harshitrwt
|
ffb6564f8b |
fix(controller): drop stale bucket-class label from BucketClaim labels before applying validated value
maps.Copy(workload.Labels, bc.Labels) can bring in a workloads.cozystack.io/bucket-class label directly set on the BucketClaim, which would survive unmodified when spec.bucketClassName is empty or invalid, leaving the Workload reporting a class that does not match the source BucketClaim. Signed-off-by: harshitrwt <harshit.rawat1804@gmail.com> |
||
|
harshitrwt
|
f41a500b07 |
test(controller): cover invalid bucket class label in reconcileBucketClaimForMonitor
A BucketClassName longer than 63 chars (valid as a Kubernetes object name, up to 253 chars) is not a valid label value, so the bucket-class label is skipped with an Info log rather than failing the Workload write. This can happen with SeaweedFS pool class names (<namespace>-<pool>-lock) when the tenant and pool names are both long -- a missing label in that case does not mean no class was set. Signed-off-by: harshitrwt <harshit.rawat1804@gmail.com> |
||
|
harshitrwt
|
882929ff86 |
fix(controller): skip bucket-class label when BucketClassName is not a valid label value
Signed-off-by: harshitrwt <harshit.rawat1804@gmail.com> |
||
|
harshitrwt
|
0c37fff5f4 |
feat(controller): expose bucket class on BucketClaim Workloads
Signed-off-by: harshitrwt <harshit.rawat1804@gmail.com> |
||
|
Aleksei Sviridkin
|
8b8d4115fc |
fix(workloadmonitor): stop panicking on a PVC without a storage class
The storage class check used || where it needed &&, so a PVC with no storageClassName (one that takes the cluster default) was dereferenced as a nil pointer. controller-runtime recovers the panic, but the reconcile of that WorkloadMonitor fails on every retry and never reaches its status update. A PVC with an empty storageClassName produced a resource key with an empty class name instead of falling back to "default". Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Andrey Kolkov
|
f6a615f693 |
fix(backups): scrub legacy plaintext passwords from CNPG Backup snapshots (#4558)
## What this PR does A CNPG `Backup` taken while the Postgres app still accepted `users[].password` copied that password into `status.underlyingResources`, where anyone with `get backups` in the namespace can read it. #4078 stopped new snapshots from carrying the field but left every existing `Backup` as it was (#4179). This adds `LegacyPasswordScrubber`, a leader-elected runnable in the backup strategy controller that, on start, lists `Backup` objects and removes `users.*.password` from every snapshot whose `kind`/`apiVersion` is the CNPG snapshot's. - **Why on every leader start and not a platform migration.** The migration hook runs pre-upgrade, while the old controller is still leader, so a `Backup` the old replica writes during the rollout would keep its password and the migration never re-runs. With `--leader-elect` on, the old replicas stop writing before the new leader's scrub runs, and a `Backup` brought back from an object-level export is cleaned on the next start too. On a clean cluster the whole pass is one List from the manager cache, which `BackupReconciler` already populates. - **Write shape.** The patch is a merge patch computed from the diff, so the request body only nulls the `password` keys and cannot overwrite status another writer changed since the List. The snapshot is handled as generic JSON (numbers kept as `json.Number`), so fields the current snapshot type does not know survive. - **Failure handling.** A `Backup` that cannot be decoded or patched is logged and skipped, and a failed List is logged too; `Start` always returns nil, because a returned error would stop the manager and take every backup down over one object. - **Restore compatibility.** Restore already ignored the key; a test decodes a scrubbed snapshot through `unmarshalCNPGBackupSnapshot` to pin that. - RBAC: `list` and `patch` on `backups.cozystack.io/backups` are already granted to the controller, so the chart is unchanged. - `docs/operations/backup-classes.md` no longer tells operators to delete pre-upgrade `Backup` objects, and says what the scrub does not reach (audit logs, etcd backups, external exports of the objects, Helm history of the init-script Secret), so a password once set through `users[].password` is still treated as exposed until rotated. The runnable carries a `TODO(#4179)` to remove it once upgrading from a release that accepted `users[].password` is no longer supported. Verification: `go test ./internal/backupcontroller/...` and `go build ./cmd/backupstrategy-controller/` are green. The new tests cover scrubbing with everything else kept (including an unknown field with a large integer), leaving non-CNPG, other-apiVersion, clean and snapshot-less Backups unpatched, and continuing past a Backup that fails to decode or patch. Disabling the delete, or returning on the first patch error, makes the tests fail. Not exercised on a live cluster yet. Fixes #4179 ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map against the diff: it touches only `internal/backupcontroller`, `cmd/backupstrategy-controller` and the in-repo `docs/operations/backup-classes.md`, with no package, values, schema, CRD, variant or asset change. The website carries no "delete pre-upgrade Backups" workaround this would make obsolete; its stale `users[name].password` rows are the #4078 follow-up already noted there. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(backups): the backup strategy controller now removes plaintext `users[].password` values left in `status.underlyingResources` of CNPG `Backup` objects taken before managed Postgres stopped accepting user passwords. It runs on every controller start and needs no action. It does not reach copies outside the cluster (audit logs, etcd backups, exported `Backup` objects), so a password once set through `users[].password` should still be treated as exposed until it is rotated. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Security** * At controller startup, plaintext user passwords are removed from matching CNPG backup snapshots. Other snapshot data is preserved. * This cleanup does not affect external copies or remove older init-script manifests from Helm history. Treat passwords previously set through `users[].password` as exposed until rotated. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
adc2dcb5ca |
fix(operator): rebuild each changed ArtifactGenerator generation once
source-watcher builds an ArtifactGenerator from the spec it read when its reconcile started, but its final status patch re-reads the live object and marks Ready with the live generation. If the spec changes during a build, which an upgrade does as it rolls out a new packages source and new copy rules within seconds of each other, the generator reports the new generation as built while its artifacts come from the old one, and source-watcher never rebuilds them. A chart can then ship without a library its templates now require. The generator status cannot tell a stale build from a good one, so the operator forces one rebuild of every changed generation once it reports Ready, through the existing force-drift path. An unchanged spec rebuilds to the same digest, so nothing downstream moves. The recovery messages no longer name one upstream race, since they now serve two. Upstream: fluxcd/source-watcher#384. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a09eded9ec |
fix(kubernetes-worker-image): wait for goldens to import before Ready
The Talos worker image catalog renders its goldens the same way the VM image collection does: DataVolumes with immediate bind whose import starts at install. With no health check the release went Ready as soon as helm applied them, so a golden that never imported left the catalog green. Gate it on the DataVolume phase with the same expressions. Worker pools read each golden directly and do not wait on the catalog status, so the gate changes what the status reports and delays no pool. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
51ac18776e |
fix(vm-default-images): wait for golden images to import before Ready
Every image is a DataVolume with immediate bind, so its import starts at install, but nothing waited for it: the HelmRelease went Ready as soon as helm applied the DataVolumes. An image that failed or stalled left the component green, and the failure surfaced later on a vm-disk cloning from it, far from the cause. Gate the release on the DataVolume phase. Only Succeeded counts as ready, and a Failed phase, when CDI sets one, ends the wait at once. Every other phase, and a missing status, keeps the wait open until the install window expires. There is no per-component timeout, so a download longer than that window is retried as an upgrade until it finishes, and the retry strategy never uninstalls the volumes. Signed-off-by: Aleksei Sviridkin <f@lex.la> Assisted-by: LLM |
||
|
Aleksei Sviridkin
|
7d44372a16 |
fix(postgres): read the live Cluster when deciding the WAL-archive prefix (#4299)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Fixes #4298. `applyClusterPluginBackup` preserves the `serverName` already on the Cluster so that a backup does not flip the WAL-archive prefix mid-stream, which would split the archive across two prefixes. That rule is right; the read it rests on is not reliable in one window. The value was read through the manager's cached client, and `BackupJobReconciler.SetupWithManager` registers `For(&BackupJob{})` and nothing else, so a Cluster only reaches the cache through a lazily started informer with no resync tied to the Cluster's own changes. An in-place `RestoreJob` deletes the Cluster and the chart re-renders it with `bootstrap.newServerName` seconds later, so a read served during that window describes the object that was purged — and the source's `serverName` is written back over the freshly rendered one. `barman-cloud-check-wal-archive` then refuses the destination with `Expected empty archive` and the `RestoreJob` ends on its 30-minute deadline, which is the failure `newServerName` was added in #3988 to prevent. The same stale read carries a stale `UID`, and that `UID` owns the `ObjectStore` this function applies, so the store is garbage-collected as soon as it is written. Reading through the dynamic client this reconciler already carries keeps the preservation rule and makes the value it rests on truthful. There is precedent in the same file: the HelmRelease suspend/resume path reads that way for the same reason. Observed on a v1.6.3 cluster, twice on separate runs with the same four-second gap between the chart's apply and the driver's. The regression test makes the two clients disagree the way they disagree in that window and asserts the driver believes the live one; reverting the fix fails it on the serverName assertion. ### 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. --> ### 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. --> - [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(postgres): an in-place restore no longer loses its fresh WAL-archive prefix to a stale Cluster read, which left the restored cluster archiving onto the source's prefix and the RestoreJob failing on its deadline ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Backup processing now uses the latest CNPG Cluster state rather than potentially outdated cached information. This ensures the cluster’s current server name and identity are used when associating backup ObjectStore resources, including when the cached and live cluster details differ. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
8ed34166cf |
perf(lineage): cache owner GETs and short-circuit re-admission (#2667)
## What this PR does The lineage mutating webhook walks the owner chain (Pod, StatefulSet, HelmRelease, application CR) on every admission, with one uncached dynamic GET per level, and sibling Pods of the same parent repeat the same lookups. On a multi-tenant cluster this put `lineage.cozystack.io` at 300-500ms p99 in `apiserver_admission_webhook_admission_duration_seconds`, the largest share of admission latency. Owners are now cached in-process for 60 seconds, and only successful lookups are stored. A failed lookup (RESTMapping, NotFound, timeout) leaves the walk without an owner, so the object is admitted with no lineage labels and stays unlabeled until its next UPDATE runs the walk again. Secrets, Services and PVCs can go a long time between updates, so a cached failure would leave every object admitted within the TTL unlabeled for that long. The aggregated cozystack-api makes such failures routine, since discovery of `apps.cozystack.io` fails while it rolls. The one staleness left is the scheduling class, which is read from the application's spec: a change to it reaches newly admitted Pods up to 60 seconds later. The cache is not swept below 4096 entries, so up to that many owner objects stay in memory for the life of the process. Above that, each insert drops the expired entries among a random sample of 64, which keeps the write lock short. Non-Pod objects that already carry every lineage label, `internal.cozystack.io/tenantresource` included, skip the walk, because the objectSelector still matches an UPDATE whose old object lacks the label. Pods go through regardless, as each Pod admission also sets the scheduling class. ### Screenshots Not a UI change. ### Downstream repositories The diff is Go under `pkg/lineage` and `internal/lineagecontrollerwebhook` only: no label, annotation, CRD, values key, `hack/` file or telemetry metric is added or renamed, so no entry of the trigger map applies. - [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 perf(lineage): the lineage mutating webhook caches owner lookups for 60 seconds and skips the owner walk for non-Pod objects that already carry lineage labels, cutting admission latency on clusters with many tenants. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Lineage and owner lookups now use a time-limited in-memory cache, reducing repeated API requests during admission processing. Scheduling-class updates may take up to 60 seconds to appear on new Pods. * Non-Pod objects with complete lineage labels are allowed without additional owner lookup or mutation. * **Tests** * Added coverage for cache hits, misses, expiration, and retrying lookups after errors. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
018b49929f |
feat(monitoring): auto-route tenant logs using namespace labels (#2195)
<!-- 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 tenant that runs its own monitoring stack gets a VictoriaLogs instance, but the node fluent-bit in `cozy-monitoring` sends every container log to `global.target` (`tenant-root`), so that instance stays empty. This PR routes each namespace's container logs to the VictoriaLogs named by its `namespace.cozystack.io/monitoring` label, the label the tenant chart already sets and the per-tenant vmagent already selects on. **Chart (`packages/system/monitoring-agents`).** `templates/_tenant-log-routes.tpl` looks up the namespaces and keeps those whose label is set and names a tenant other than `global.target`. For those, fluent-bit gets one `rewrite_tag` filter with a `Rule` per target (a single emitter however many tenants are routed), one `http` output per target to `vlinsert-generic.<target>.svc:9481`, and a per-target `modify Set tenant <target>`, because the re-tagged record re-enters the filter chain where the `Match *` `Add tenant` cannot overwrite the key. Unlabelled namespaces, empty labels and labels equal to `global.target` stay on the default outputs. **Controller (`internal/controller/tenantlogrouting`).** `lookup` results are not part of the digest helm-controller compares, so the chart alone would not pick up a tenant that turns monitoring on, or drop one that is removed, until an unrelated upgrade. The new reconciler in cozystack-controller digests the monitoring label across all namespaces and, when the digest moves, sets `reconcile.fluxcd.io/requestedAt` and `reconcile.fluxcd.io/forceAt` on `cozy-monitoring/monitoring-agents`, which forces an upgrade and re-runs the lookup. It digests every labelled namespace rather than repeating the chart's selection, so it cannot drift from the chart; the cost is an occasional upgrade that renders the same config. The digest it acted on is kept in its own annotation, so a manual `flux reconcile` does not make it force another upgrade. The existing ClusterRole already grants the verbs it needs. **Behaviour change for the platform operator.** Routed records are re-tagged with `keep false`: once a tenant runs its own monitoring, its namespaces' container logs go to that tenant's VictoriaLogs and no longer to the root one. Events and audit logs are not routed and stay on the root store. **Why not a per-tenant collector.** An earlier comment on this PR proposed a fluent-bit per tenant, following the per-tenant vmagent and the OTLP collector from #3763. That collector receives OTLP over the network; a log collector has to tail `/var/log` through `hostPath`, which the PodSecurity level of tenant namespaces rejects, and running it in a privileged system namespace instead would mean one DaemonSet per tenant tailing every node's logs. The routing therefore stays in the one node DaemonSet, with the reconciler closing the reconcile gap. Tests: `tests/fluent-bit_tenant_log_routing_test.yaml` mocks the namespaces with `kubernetesProvider` and pins which get a rule and an output, which are skipped (unlabelled, empty label, label equal to `global.target`), and that a routed record carries the target tenant. Three of its cases fail against the previous revision of this branch. `tests/fluent-bit_tenant_log_routing_offline_test.yaml` pins that an offline render routes nothing. `internal/controller/tenantlogrouting/reconciler_test.go` covers first sight, no-op while labels are unchanged (including after a manual reconcile), a tenant turning monitoring on, a routed namespace being deleted, and a missing release. Refs #2194 ### 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. --> ### 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. --> No entry of the trigger map matches the diff: it touches `packages/system/monitoring-agents` (values, a template helper, tests), a new reconciler in `internal/controller`, its registration in `cmd/cozystack-controller`, and a comment in the cozystack-controller ClusterRole. The one open question is `cozystack/website`: `content/en/docs/next/operations/services/monitoring/logs.md` describes log collection in general terms and does not say where a tenant's container logs land, so the behaviour change above could deserve a sentence there. I have left every box empty for a maintainer to decide. - [ ] 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 feat(monitoring): container logs from a tenant that runs its own monitoring stack now go to that tenant's VictoriaLogs instead of the root one. The node fluent-bit routes them by the namespace.cozystack.io/monitoring label, and cozystack-controller re-renders the node agents when that label changes on any namespace. ``` |
||
|
Mattia Eleuteri
|
ca24d8f5f6 |
perf(lineage): cache owner GETs and short-circuit re-admission
Every admission walks the owner chain (Pod, StatefulSet, HelmRelease, application CR) with one uncached GET per level, and sibling Pods repeat the same lookups. On a cluster with many tenants that put the lineage webhook at 300-500ms p99, the largest share of apiserver admission latency. Owners are now reused for 60 seconds, successful lookups only. A failed lookup, whether RESTMapping, NotFound or a timeout, leaves the walk without an owner, so the object is admitted with no lineage labels and stays unlabeled until its next UPDATE runs the walk again. Secrets, Services and PVCs can go a long time between updates, so a cached failure would leave every object admitted within the TTL unlabeled for that long, and the aggregated cozystack-api makes such failures routine: discovery of apps.cozystack.io fails while it rolls. Non-Pod objects that already carry every lineage label, tenantresource included, skip the walk, since the objectSelector still matches an UPDATE whose old object lacks the label. Pods go through it regardless, as each Pod admission also sets the scheduling class. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Aleksei Sviridkin
|
9b56e70ca6 |
feat(mongodb): support the system-bucket backup flow (useSystemBucket) (#3967)
## What this PR does MongoDB was the only `cozy-default` app with **no `useSystemBucket` flow**: its backup storage was always static on the `PerconaServerMongoDB` CR (bucket/endpoint/credentials from `backup.*` values plus a chart-emitted `<release>-s3-creds` Secret), so a tenant could not back up to the platform system bucket out of the box the way Postgres/ClickHouse/MariaDB/etcd do. This closes that gap so the default MongoDB backup-restore works OOTB. PBM takes `bucket`/`endpointUrl`/`region`/`prefix` as **static CR fields** (not secret refs), and the platform bucket name/endpoint are only known at BackupJob time — so, unlike ClickHouse's runtime `secretKeyRef` env, the chart cannot render them. The implementation mirrors the CNPG driver's pattern: - **Chart** (`packages/apps/mongodb`): a new `backup.useSystemBucket` flag keeps `spec.backup.enabled` (so the pbm agents run) but omits the static `storages`/`tasks`/`pitr` and the `<release>-s3-creds` Secret. Helm never owns `spec.backup.storages`, so a Flux re-render can't fight the driver. A `bootstrap.enabled + useSystemBucket` render guard mirrors Postgres, and `useSystemBucket: true` with `backup.enabled: false` is refused both at render time and by an `x-kubernetes-validations` rule on `backup` in the schema (the same mechanism as the `storageClass` immutability rule). Note on the schema rule: the aggregated apiserver does not evaluate CEL on application-backed resources yet (#2657), so today it is a UI/schema-level marker and the render-time refusal is what stops a Flux release; it also trips the API review gate (“validation rule added”), so this PR needs an API-owner approval on top of the code review. - **Strategy** (`cozy-default-mongodb`): now carries the S3 coordinates (`endpointUrl` already scheme'd by the `backupstrategy-controller.endpoint` helper, `credentialsSecret: cozy-backups-creds`, `prefix: <namespace>/<application>`). - **Driver** (`psmdbstrategy_controller.go`): SSA-injects `spec.backup.storages[s3-storage]` onto the live `PerconaServerMongoDB` at BackupJob time under a dedicated field manager (`cozystack-psmdb-backup-driver`) with `ForceOwnership`. Injection is **gated on the app's `backup.useSystemBucket` flag** (a legacy app leaves it false and is never touched; a useSystemBucket app is re-injected once per mint (once per BackupJob, plus once per storage-race re-mint), before its operator CR is created, so a later coordinate change catches up on the next job while a streaming dump is never patched under). The backup CR is minted only once the operator reports the storage pushed into pbm (`status.observedGeneration` caught up and `PBMReady` True): pbm writes to what its own config says, not to the spec, so a CR minted before that push would land at the previous coordinates while its Backup recorded the new ones; the job waits at `PerconaServerMongoDBPBMConfigPending` meanwhile, bounded by the backup deadline. Once a job has its CR, none of the mint-time reads or holds can fail it: the strategy and app reads the artifact still needs are bounded by the 24h running ceiling instead, and the cluster is not read at all **and on the release having rendered that flag**: the app CR is projected from the HelmRelease's desired values, so the flag reads true before helm-controller renders the revision, while the tenant's own storage, scheduled tasks and PITR stream from the previous render still sit on the cluster. As long as the cluster carries `spec.backup.tasks` or `spec.backup.pitr`, the driver does not inject and holds the BackupJob at `Ready=False PerconaServerMongoDBLegacyRender` (failing it at the deadline), so a wedged legacy revision never has its nightly dump and oplog redirected into the shared bucket. The backup snapshot falls back to the strategy coordinates when the operator status echo is empty; that fallback is gated on the CR's own `percona.com/delete-backup` finalizer (the flow the CR was minted on), not on the app flag re-read at completion, so a legacy backup never records the platform bucket and a flag flipped mid-dump does not change the artifact. The restore path rebuilds `backupSource` from it (the `cozy-backups-creds` reference is projected into the restore namespace before dispatch). - **Retention/cleanup**: on the `useSystemBucket` flow the driver owns the shared-bucket archive (the chart's psmdb `tasks[].keep` retention is gated out, and an on-demand backup is not covered by task retention). It stamps the operator's `percona.com/delete-backup` finalizer on each backup it mints **against a storage that carries the platform coordinates** — the strategy's bucket and `credentialsSecret`, read back from the live cluster after the injection — never on the flag alone: a strategy without an `s3` block, or a storage that points anywhere else, holds the job (`MongoDBStrategyHasNoS3` / `PerconaServerMongoDBStorageForeign`) instead of minting a finalized dump against a bucket the platform does not own. On Cozystack `Backup` deletion (by hand today; by platform retention once it lands, since a `Plan` only creates BackupJobs and never deletes a `Backup`) the driver deletes the operator `PerconaServerMongoDBBackup` CR so that finalizer frees the object — the finalizer's presence is the ownership marker cleanup reads back. That reading is unambiguous because cleanup resolves the CR by the name recorded in `driverMetadata` (the one the driver minted), not by scanning: the psmdb operator does stamp `percona.com/delete-backup` on its own `cron-*` task-minted backups, but those are never in scope. A legacy backup keeps the pre-existing hands-off contract, decided from the snapshot before any read of the operator CR. A terminating namespace or the `backups.cozystack.io/skip-artifact-cleanup: "true"` annotation strips the driver's finalizer and releases; a `Get` of the operator CR that fails with anything other than NotFound/NoMatchError still requeues, and the operator's own `release-lock` finalizer can hold the CR while the operator is down, so those two are what can keep a namespace in `Terminating`. `schedule` and `retentionPolicy` are inert on this flow (there is no engine-side retention and a `Plan` has no retention field yet); platform-level retention is tracked in [#4192](https://github.com/cozystack/cozystack/pull/4192) and teardown GC of orphaned backups in [#3966](https://github.com/cozystack/cozystack/issues/3966), so until those land bound `cozy-backups` with an S3 lifecycle policy. - **Bounds on the waits**: the injection race with the operator's cluster cache (an `unable to get storage` error on a freshly injected storage) is retried by deleting the errored CR and minting a fresh one, with the retry count kept on the BackupJob (`backups.cozystack.io/mongodb-storage-race-retries`) because the deleted CR is reaped before the next poll; after three retries the operator's error is terminal. A system-bucket dump that has started streaming is not cancelled on the 30-minute deadline, but fails after 24 hours in `running` (`BackupAbandoned` warning naming the operator CR, which is left in place as the handle to whatever pbm still writes). The deadline cancel of a not-yet-started CR pins its delete to the resourceVersion the live read answered for. - **RBAC**: `patch` on `perconaservermongodbs` for the injection (server-side apply), and `create`/`delete`/`patch` on `perconaservermongodbbackups` — `delete` to prune the shared-bucket archive on `Backup` deletion, `patch` to strip the driver's finalizer when releasing a wedged cleanup. Coverage: Go unit tests over `internal/backupcontroller` — the injection gate keyed off the app flag (legacy cluster never patched) and off the applied render (a cluster still carrying tasks or pitr is held, untouched), the SSA-injection precondition with the whole entry (per-application prefix included) asserted on the cluster, strategy templating per application, the finalizer refused against a storage without platform coordinates (strategy without `s3` over a legacy entry, foreign coordinates on the merged entry), the retry budget driven with the operator simulated reaping the deleted CR between polls, the restore snapshot fallback and credential swap across all four legacy/system flow combinations, the finalizer-owned cleanup with its escape hatches (skip annotation before any read, `NoMatchError` treated as gone, terminating-namespace release using the pre-read CR, a failed strip surfaced as an Event, a legacy CR never deleted, a legacy snapshot released without a read, a system-bucket snapshot reaching the prune), the deadline cancelling a not-yet-running operator backup only on a positive live read (including no live client at all, and a CR that moved since the read), and the running ceiling. Plus helm-unittest on both charts (chart skips the creds Secret and omits `spec.backup.storages`/`tasks`/`pitr` on `useSystemBucket` and keeps them on the legacy flow; the strategy renders the S3 coordinates and the `insecureSkipTLSVerify` knob). A live `useSystemBucket` backup+restore e2e against the platform bucket is a follow-up (needs the system bucket wired in CI, same status as the Postgres/ClickHouse useSystemBucket e2e). One item to confirm in e2e: the psmdb operator accepting `spec.backup.enabled=true` with an empty `storages` between install and the first BackupJob. ### Screenshots Not applicable (no UI changes). ### Downstream repositories The tenant-facing schema surface is one new field, `spec.backup.useSystemBucket`, on the MongoDB `values.schema.json` / CRD (regenerated), plus a validation rule on `spec.backup`. `terraform-provider-cozystack` hand-models MongoDB and may want to expose it, but it is a new optional boolean that defaults false (no drift for existing users), and the chart `README.md` regenerates on release. Leaving every box unticked per the trigger-map guidance so a maintainer decides on the provider follow-up. - [ ] 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: ### Release note ```release-note feat(mongodb): MongoDB backups now support the platform system bucket via backup.useSystemBucket, so the default cozy-default BackupClass works without per-app S3 credentials — the backup driver injects the system-bucket storage onto the cluster at backup time, once the release has rendered the flag. On this flow nothing prunes the shared bucket automatically yet (a Plan carries no retention field; tracked in #4192 and #3966), so an S3 lifecycle policy on cozy-backups is a prerequisite for enabling it, not a suggestion. useSystemBucket requires backup.enabled=true; the chart refuses the combination with backups disabled at render time, and the schema carries the same rule as a marker until the apiserver enforces CEL (#2657). A system-bucket dump still running after 24 hours fails its BackupJob with the operator backup left in place. A MongoDB restore now adopts the target cluster's S3 credentials only for a storage on the same bucket as the archive (it used to adopt them unconditionally); a legacy restore into a target on a different bucket that relied on one key pair covering both must keep or re-create the source release's -s3-creds Secret by name, and is held at Ready=False RestoreCredentialsMissing until it exists. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added opt-in system bucket mode for MongoDB backups. * Automatically injects platform bucket storage without application-managed S3 credentials. * Supports endpoint, region, path prefix, addressing, and TLS settings. * Manages shared-bucket backup artifacts when system bucket backups are deleted. * **Bug Fixes** * Improved restore credential selection to avoid cross-bucket mismatches. * Preserved backup metadata when storage details are unavailable. * **Documentation** * Documented configuration, scheduling, retention, PITR limitations, and restore guidance. * **Validation** * Prevented incompatible bootstrap and system bucket restore configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Mattia Eleuteri
|
e510d49b3d |
feat(monitoring): re-render node agents when a monitoring label changes
The node fluent-bit builds its tenant routes from a lookup of namespace labels, and helm-controller upgrades a release only when its chart or values digest changes. A tenant turning monitoring on therefore got no route, and a removed one kept its route, until an unrelated change upgraded monitoring-agents. The reconciler digests the label across all namespaces and requests a forced upgrade of the release when the digest moves. It digests every labelled namespace rather than repeating the chart's selection, so it cannot drift from the chart; the cost is an occasional upgrade that renders the same config. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
67bf9ca317 |
fix(postgres): read the live Cluster when deciding the WAL-archive prefix
applyClusterPluginBackup preserves the serverName already on the Cluster, so
that a backup does not flip the archive prefix mid-stream. It read that value
through the manager's cached client, and this controller registers
For(&BackupJob{}) and nothing else, so a Cluster only reaches the cache
through a lazily started informer with no resync tied to its own changes.
An in-place RestoreJob deletes the Cluster and the chart re-renders it with
bootstrap.newServerName seconds later. A read served during that window
describes the object that was purged, so the source's serverName is written
back over the freshly rendered one and the restored cluster archives onto the
source's WAL prefix. barman-cloud-check-wal-archive then refuses to start with
"Expected empty archive" and the RestoreJob ends on its deadline -- the
failure newServerName was added to prevent.
The stale read carries a stale UID too, and that UID owns the ObjectStore this
function applies, so the store is garbage-collected as soon as it is written.
Reading through the dynamic client keeps the preservation rule and makes the
value it rests on truthful.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
|
||
|
Aleksei Sviridkin
|
d9ed566326 |
fix(migration): restore the class reclaim policy on imported volumes (#4480)
<!-- 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 VM import handoff sets the transferred `PersistentVolume` to `Retain` so the data survives the swap from the Forklift-populated claim to the VMDisk's claim, and then never gives the policy back. Every imported disk therefore ends up with `persistentVolumeReclaimPolicy: Retain`, while its StorageClass says `Delete` and every VMDisk created directly carries `Delete`. When the tenant later deletes an imported VMDisk through the normal path, its DataVolume and claim go away, but the volume stays `Released` with its backing storage: an orphaned, billed volume that no tenant can see or remove. #### How it was measured On a lab cluster with a LINSTOR-backed `replicated` class (`reclaimPolicy: Delete`): a 16 GiB VM imported from vSphere produced a VMDisk whose PV was `Retain`, while every non-imported VMDisk's PV in the cluster was `Delete`. Deleting the VMInstance and then the VMDisk removed the DataVolume and the claim and left the PV `Released`, still `Retain`, with its LINSTOR resource in place. Patching that PV back to `Delete` made the CSI provisioner delete both the PV and the LINSTOR resource, which confirms the reclaim policy is the only thing keeping it. #### The fix `Retain` is now held only for the window it protects. A new step, `settleReclaimPolicy`, restores the policy the volume's StorageClass declares (`Delete` when the class names none or no longer exists) once the handoff is provably over, and until then reports "not yet" and touches nothing: - the replacement claim is `Bound` and the PV is `Bound` with a `claimRef` naming that claim by UID, as the PV controller reports it rather than as the controller wrote it, so no release is in flight; - the transferred claim is gone; - the claim's one and only owner is the VMDisk's DataVolume, as a controller reference whose UID matches the live DataVolume (CDI's adoption), so the only deletion that can reach the volume is the VMDisk's own; - the VMDisk exists and carries this task's output markers. `fulfill` runs this step after adopting every disk and before creating the VMInstance and deleting the scaffolding VM, so a VM is reported `Succeeded` only once its volumes carry their class's policy, and an import that fails after this point still leaves disks that clean up like any other. The step is a compare-and-set, so repeated passes and restarts are no-ops once settled. It also runs on the path that recovers outputs whose status was never written, for tasks caught mid-handoff by an upgrade. The comment that called permanent `Retain` "part of the contract" and the matching paragraph in `packages/system/migration-controller/README.md` are updated; the README also says how to fix volumes left on `Retain` by imports completed before this change, since those tasks are terminal and are not reconciled again. In practice a VM now stays in `Creating` for one more requeue (about 20 s) after the swap, while the PV controller rebinds and CDI adopts the claim. #### Tests - `TestSettleRestoresTheClassReclaimPolicy`: a finished handoff gets the class's policy back: `Delete`, `Retain` when the class asks for it, and `Delete` when the class names none or is gone. - `TestSettleIsIdempotent`: a second pass over a settled volume reports settled and does not rewrite the PV. - `TestSettleKeepsRetainWhileTheHandoffIsOpen`: thirteen cases, each breaking one condition (claim not created by this import, claim not bound, PV still `Released`, `claimRef` still on the old claim, `claimRef` on the new claim with a stale UID, transferred claim still present, claim not yet adopted by CDI, an extra owner, a DataVolume owner that is not the controller, an owner reference to a previous DataVolume, DataVolume missing, VMDisk missing, VMDisk not created by this import). Every one keeps `Retain`. - `TestFulfillRestoresTheReclaimPolicyBeforeFinishing`: behavioural regression over `fulfill`. The first pass swaps the volume, keeps `Retain`, creates no VMInstance and keeps the scaffolding VM. After simulating the PV controller and CDI, the second pass restores `Delete`, creates the VMInstance and removes the scaffolding VM. With the new step turned into a no-op, this test fails. - `TestFulfillSettlesReclaimedOutputs`: the recovery path for outputs whose status was never written. With the handoff still open the volume keeps `Retain`; once it is finished the policy goes back to `Delete`. - The existing `adoptVolume` tests still assert `Retain` during the swap. `go test ./internal/migrationcontroller/...` and `make test-controllers` pass, `go vet` is clean, and `make generate` leaves the tree unchanged. ### 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. --> ### 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. --> - [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 diff touches only `internal/migrationcontroller/`, `packages/system/migration-controller/README.md` and a comment in `packages/system/migration-controller/templates/rbac.yaml`, which match no entry of the trigger map; the website's VMware migration page does not describe the reclaim policy, so it needs no change. ### 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(migration): disks imported from VMware no longer keep a Retain reclaim policy after the import. The volume gets its StorageClass policy back once it is bound to its VMDisk, so deleting an imported VMDisk deletes its volume instead of leaving an orphaned Released PersistentVolume. Volumes imported before this fix keep Retain and have to be patched back by hand. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Imported volumes are protected during claim transfers, then returned to their StorageClass reclaim policy after the replacement claim and volume are safely adopted. If the class or policy is unavailable, the policy defaults to `Delete`. * VM imports wait for volume adoption and reclaim-policy settlement before completing, helping prevent premature volume deletion and incomplete imports. Previously unrecorded handoff outputs are also settled before being returned. * **Documentation** * Clarified reclaim-policy behavior during imports and how to restore cleanup behavior for volumes from older imports. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
717d091970 |
fix(backups, postgres): start a Postgres restore from the backup it names (#4443)
## What this PR does A Postgres `RestoreJob` now starts from the base backup its `Backup` refers to, and the PITR documentation describes the window the recovery actually has. **Starting from the named backup.** Without `recoveryTarget.backupID` the barman-cloud plugin picks the base backup itself (`internal/cnpgi/restore/restore.go` in plugin-barman-cloud, then `FindBackupInfo` in barman-cloud `pkg/catalog`): the newest backup of the catalog when no `recoveryTime` is set, otherwise the newest one whose end is not after `recoveryTime`, on any timeline since no `targetTLI` is passed. A `RestoreJob` naming an older `Backup` therefore started from another one, and on an archive that holds sibling timelines — left by in-place restores that archived back under the source `serverName`, before restored clusters got a fresh `newServerName` — it could start from another branch and bring back that branch's data. `FindBackupInfo` gives `backupID` precedence over time-based selection, and CNPG's webhook does not count it among the mutually exclusive targets, so it combines with `targetTime`. - on restore the CNPG driver reads barman's `backupId` and the backup's `stoppedAt` from the `Backup`'s `cnpg.io/Backup` and sets the new chart value `bootstrap.backupID`, rendered as `bootstrap.recovery.recoveryTarget.backupID`; - it does not pin for a `recoveryTime` earlier than one second past `stoppedAt`: the named backup cannot serve an instant before it is consistent, and `stoppedAt` is truncated to the second while the catalog compares barman's end time to the microsecond. Those requests keep the plugin's time-based choice, as today; - it does not pin either when the `cnpg.io/Backup` is gone. plugin-barman-cloud's retention deletes it together with the base backup it describes (`deleteBackupsNotInCatalog`), and the driver deletes one only with its artifact, so a missing one means the catalog has lost that base backup: pinning it would purge the target and then fail the recovery. The plugin's choice recovers to the same end of the archive, and the `RestoreJob` gets a `BaseBackupGone` warning event. A base backup deleted from the object store by other means is still pinned for as long as its `cnpg.io/Backup` stays, and that restore fails after the purge; plugin-barman-cloud deletes such a `cnpg.io/Backup` on its next maintenance pass (`deleteBackupsNotInCatalog`, every `retentionPolicyIntervalSeconds`, retention policy or not), but only from the primary of the cluster that took it, under the same UID. Telling it apart earlier would need reading the barman catalog, which the driver does not do; the documentation says so, and that deleting the `cnpg.io/Backup` by hand is the way out; - a re-restore into the same target clears a `backupID` left by a previous one. **Documentation** (`docs/operations/backup-classes.md`, PostgreSQL PITR): - the upper bound of the window is the last transaction (commit or abort) in the archived WAL, not the newest segment: recovery ends just before the first transaction after the target, and a segment switch or a base backup is not one. On a database at rest every instant after its last write fails with `recovery ended before configured recovery target was reached`, including "at/before the most recent completed backup", which the page recommended as the safe choice; the state as of the last write is obtained by omitting `recoveryTime`; - aiming at a backup's `stoppedAt` misses that backup (the plugin keeps only a backup whose end is not after the target), falling back to an older one or to `no target backup found`; the page now says to aim one second past it; - when a base backup removed from the bucket outside retention is still pinned, and how to unpin it; - how to read the upper bound from the archive, what pinning the base backup changes, and a section on archives with several timelines: PostgreSQL follows `recovery_target_timeline: latest` from the backup's timeline through the consecutively numbered history files it can find, so a missing `.history` confines the replay to the backup's own timeline and the bound is the last transaction on that timeline, while a targeted timeline that does not descend from the backup stops recovery with `requested timeline N is not a child of this server's history`. **Tests.** `TestCNPGRestoreBackupID` (the pinning rule), `TestReconcileCNPGRestore_PinsTheRequestedBackup` (drives a restore through the purge and reads back the patched `Postgres` app, including a `cnpg.io/Backup` removed by retention, with and without `recoveryTime`), `TestBuildPostgresAppRestorePatch_ReplacesBackupID`, a deep-copy test for the new `stoppedAt` field, and three `helm unittest` cases for `recoveryTarget`. With the rule disabled, 4 of the rule cases and 3 of the reconcile cases fail; with the template change removed, 2 chart cases fail; the retention case fails against the first version of this PR, which pinned an id recorded on the artifact; the deep-copy test fails once `StoppedAt` is dropped from the generated `DeepCopyInto`. `go test ./internal/backupcontroller/...` and `helm unittest packages/apps/postgres` (139/139, after rebasing on main) pass. Not run: the e2e suite (`hack/e2e-chainsaw`), and a restore against a live cluster with this driver. ### Screenshots 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. --> - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: https://github.com/cozystack/website/issues/716. `content/en/docs/next/operations/services/backup-classes.md` (and `v1.6/`) carries a copy of the corrected "The recoverable window" and "Discovering the earliest / latest restorable time" sections; the window and `stoppedAt` corrections apply to released versions too, the `backupID` part only once this is released. - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: none expected. The diff adds `bootstrap.backupID` to the postgres `values.schema.json`, but the provider does not manage the `bootstrap` block (`internal/provider/postgresql_schema.go`: "bootstrap blocks use server defaults"), and the `RestoreJob` CRD is unchanged. Left unticked for a maintainer to confirm. - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(backups): a Postgres RestoreJob now starts from the base backup its Backup refers to (recoveryTarget.backupID) instead of whichever backup the barman-cloud plugin picks, which on an archive with several timelines could restore another branch's data; a recoveryTime before that backup's end keeps the previous time-based choice. The PITR documentation now bounds the recoverable window by the last archived transaction rather than the newest WAL segment. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Restores can target a specific base backup and optionally replay to a requested recovery time. If no backup is specified, the newest eligible backup is selected automatically. * **Bug Fixes** * Restore handling warns when the referenced base backup is unavailable and allows the plugin to select a backup from the catalog. * **Documentation** * Clarified recovery-window boundaries, timestamp precision, idle databases, timeline branches, and behavior when a requested target cannot be reached or predates the earliest base backup. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Mattia Eleuteri
|
28e42fbe4d |
fix(backups): match a Velero restore in its Backup's API group
The Velero driver resolves a restore's target from the Backup's applicationRef, overriding only the kind and name, and restores the Backup's resources. A targetApplicationRef.apiGroup never reaches it. The restore gate honoured that group all the same, so a Velero RestoreJob naming another group let a BackupJob of the application actually being restored start against it, and held one of an application the restore never touches. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
497dd6fc4f |
fix(backups): start a Postgres restore from the backup it names
Without recoveryTarget.backupID the barman-cloud plugin chooses the base backup itself: the newest one in the catalog when no recoveryTime is set, and the newest one ending by recoveryTime otherwise, on any timeline since no targetTLI is passed. A RestoreJob naming an older Backup therefore started from another one, and on an archive holding sibling timelines - left by in-place restores that archived back under the source serverName - from another branch, bringing back that branch's data. The restore reads barman's backupId and the backup's stoppedAt from the live cnpg.io/Backup behind the Backup, and passes the id through the chart as bootstrap.recovery.recoveryTarget.backupID. The plugin gives backupID precedence over time-based selection, and CNPG does not count it among the mutually exclusive targets, so it combines with targetTime. Two cases keep the plugin's choice. A recoveryTime earlier than one second past stoppedAt: the named backup cannot serve an instant before it is consistent, and stoppedAt is truncated to the second while barman compares its end time to the microsecond. And a cnpg.io/Backup that is gone: plugin-barman-cloud's retention deletes it together with the base backup it describes, and the driver deletes one only with its artifact, so pinning its id would purge the target and then fail the recovery on every retry. That restore records a BaseBackupGone warning event instead. A re-restore into the same target clears a backupID left by a previous one. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
c6a4933cf6 |
test(migration): cover the recovery settle and the handoff guards
Recovering outputs that no status recorded goes through fulfill's vm == nil branch, which settles the reclaim policy of volumes an older controller left on Retain. Nothing drove that branch, so the write path could be removed with the suite still green. A fulfill test now runs it with the handoff open and finished. The guards that stop the controller from rewriting the policy of a volume it did not produce, or before CDI controls the claim, get one case each: a claim or VMDisk marked by another import, a non-controller DataVolume owner reference, and a claimRef naming the replacement claim under a stale UID. The storageclasses RBAC comment now also names the reclaim policy read. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Andrey Kolkov
|
19764c4bba |
fix(backups): scrub legacy plaintext passwords from CNPG Backup snapshots
A CNPG Backup taken while the Postgres app still accepted users[].password copied that password into status.underlyingResources, where anyone with `get backups` in the namespace can read it. Dropping the field stopped new snapshots from carrying it but left every existing Backup exposed. The backup strategy controller now removes users.*.password from CNPG snapshots when its leader starts. It runs on every start rather than once, so a Backup written by a pre-upgrade replica during the rollout, or brought back from an object-level export, is still cleaned; on a clean cluster it is a single cached List. The patch is a diff-computed merge patch that only nulls the password keys, and a Backup that cannot be decoded or patched is logged and skipped instead of stopping the manager. Fixes #4179 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
6555ae16b8 |
fix(mongodb): start a backup only once the operator has pushed the storage into pbm
The wait after a storage injection watched the cluster's spec, which is not what pbm writes to. At the pinned operator version the backup reconciler hands pbm only a storage name and records status.s3 from the spec entry it read, while the storage pbm uses is whatever the cluster reconciler last pushed into pbm's own config; that push flips PBMReady False and back to True and the reconcile stamps observedGeneration when it ends. A backup started between the apply and that push dumped to the previous coordinates while its Backup recorded the new ones, so both its restore and its prune pointed at the wrong bucket. The wait was also no wait at all: the status write that named it re-queued the job at once, and the live read always showed the apply. The mint now waits for observedGeneration to reach the cluster's generation and for PBMReady to be True, and fails at the backup deadline if the operator never gets there. The strategy, app and cluster reads still ran ahead of the operator CR lookup and were bounded by the backup deadline, so a strategy absent for one reconcile 40 minutes into a streaming dump failed the job and left the finalized CR unreachable. The CR is resolved first now; with one in hand the cluster is not read at all, and the reads the artifact needs are bounded by the running ceiling, past which the job fails naming the CR left in place. The settle check treated a field the entry lacked as agreement, so a strategy that newly set forcePathStyle minted in the same pass as the apply that changed addressing. A missing field now counts as its zero value, which keeps an entry the operator rewrote without an empty region settled while catching a value the apply adds; the storage type is compared outright. The runbook still said the finalizer keeps a Plan from accumulating objects, the RBAC comment still said the timeout cancel runs on either flow, and four wait messages read a duration as a delay. All corrected; the runbook also describes the pbm wait. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
841933dcec |
docs(mongodb): stop saying a Plan prunes the shared bucket
A Plan creates BackupJobs and never deletes a Backup, and PlanSpec has no retention field, so nothing on the platform bounds a MongoDB archive on the shared bucket today except an S3 lifecycle policy or a manual Backup delete. The runbook's tuning section said a Plan does, two hundred lines after the same page said a Plan carries no retention, and three driver comments named "Plan retention" as the path that reclaims the bucket. An operator reading the first sentence would skip the lifecycle policy the release note calls a prerequisite. All of them now say what deletes a Backup today, and that the finalizer frees the object whenever one is deleted, whatever does the deleting. The settle check after an apply compared only the fields the desired entry carries, so an entry written with forcePathStyle while the strategy still set it counted as settled once the strategy dropped the field, and the apply that removed it minted in the same pass. The check now treats a field the apply is about to remove as a change too. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
707e38d17d |
fix(mongodb): keep every mint-time hold off a job that already has its CR
The injection and the ownership switch were gated on the job having no operator CR yet; the precondition between them was not, and neither was the pair of holds ahead of them. Each of those fails the job at the 30-minute deadline, so a tenant disabling backups, or the platform dropping the strategy's s3 block, while a system-bucket dump streamed failed the BackupJob at 30 minutes with the CR left finalized and no Backup object to reach it, bypassing the 24-hour ceiling that exists so a shared-bucket archive is never stranded. Everything up to the mint now runs only while there is no CR; a job with one goes straight to the state switch. The settle wait after the apply was keyed on the storage key being present, so a change to the strategy coordinates rewrote an entry the operator had cached and minted in the same pass, the case the wait exists for. It is now keyed on the entry's content: only an apply that changes what the entry says costs a poll. The mint-time reads (the render's tasks and pitr, the storage entry) came from the manager's cache, which can lag a re-render back to the legacy flow by one reconcile and show the flag true with the tasks not yet visible. With a dynamic client they now come from the apiserver, the way the deadline cancel already reads the operator CR. Tests age the existing-CR case past the deadline and vary the three holds, pin the coordinate-change wait, the live read and the term that keeps the storage-race retry from swallowing an unrelated operator error. The runbook's tuning section no longer summarises the defaults as 30-day retention: only the CNPG and Velero strategies carry one. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
46a8bd7363 |
fix(mongodb): apply the storage once per job, and pin the terms that decide ownership
The system-bucket storage was applied on every reconcile of a job, not once per job: with the operator CR already minted nothing returned before the apply, so a streaming dump saw an SSA patch every poll for up to a day, and the only thing keeping that patch off a cluster that had re-acquired the chart's tasks and pitr since the mint was a predicate term no test held. The storage a CR names is fixed for its dump, so the apply now runs only before the CR exists; the legacy-render hold covers that window, and a test pins that an existing CR is never injected over. The other predicate that decides whether a Backup owns an archive compared the live entry's bucket and credentialsSecret against the strategy, and only the bucket half had a fixture. The credential is what tells a tenant's entry from the platform's when a bucket name collides, so a case varying only that term joins the foreign-bucket one. The cleanup shortcut's snapshot-kind term and the 24h running ceiling are pinned the same way, the latter as an absolute boundary pair. The generated field description said the schema rejects useSystemBucket without backup.enabled. The aggregated apiserver does not evaluate CEL on application schemas yet, so the render-time refusal is the one that holds; the description and the runbook now say so. The runbook's restore paragraph still described the unconditional credential swap this branch narrowed, exempted MariaDB from a retention warning its shipped strategy does not earn, and pointed at a Plan retention field that does not exist; the example README and a dispatcher test's comment still carried the pre-branch claim that the driver never injects and never deletes. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
a6bb0556fc |
fix(mongodb): own archives by where the storage points, and bound the open loops
The prune finalizer on a driver-minted PerconaServerMongoDBBackup means the platform owns the archive: cleanup deletes the operator CR on that marker and the operator removes the object from storage. It was stamped from the app's useSystemBucket flag, which says nothing about where the storage the CR names actually points. With a strategy carrying no s3 block nothing was injected, the presence-only precondition passed on a legacy tenant-bucket entry, and the CR went out finalized against the tenant's own bucket; deleting the Cozystack Backup then deleted the tenant's archive. The finalizer now follows the storage's coordinates as read back from the live cluster after the injection, and a strategy without s3 coordinates, or a storage pointing anywhere else, holds the job instead of minting. The injection keyed on the same desired value. The app CR is projected from the HelmRelease's spec.values, so the flag reads true before helm-controller renders the revision, while the tenant's own storage, scheduled tasks and PITR stream stay on the cluster. Injecting over that entry redirected the tenant's nightly dump and oplog into the shared bucket under platform credentials. A cluster still carrying tasks or pitr is now left alone and the job held until the release renders the flag, and the schema carries a rule marking useSystemBucket without backup.enabled invalid, the pair the chart already refuses at render. The storage-race retry counted surviving operator CRs against its budget, but an errored CR is reaped the moment it is deleted (the operator drops delete-backup without pruning on a non-ready CR), so the count never left one and the job minted a CR per poll for the whole deadline. The count now lives on the BackupJob and survives the reap. A system-bucket dump in running had no bound at all: a wedged pbm agent left the job Running forever and a nightly Plan stacked another beside it. It now fails after 24 hours with the operator CR left in place as the handle to whatever pbm still writes. The deadline cancel pins its delete to the resourceVersion the live read answered for, and the artifact-retry failure names the window that was actually in force. Tests pin the guards that were exercised only in the harmless direction: a system-bucket snapshot reaching the prune, the resolved prefix reaching the cluster, a missing live client deferring the cancel, and the finalizer refused against foreign coordinates. Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Mattia Eleuteri
|
af8cf743cd |
fix(backups): hold a BackupJob while its application is being restored
Nothing on the backup path looks at restores. A Plan creates its BackupJobs on schedule, and the BackupJob reconciler dispatches them to their driver whatever else is happening to the application. For CNPG that is not merely a backup of a half-restored state: every BackupJob reconcile re-applies the Cluster's barman plugin, and on a cluster under an in-place restore it can land after the chart has re-rendered the Cluster with its new serverName and put it back on the source's. The restored cluster then archives onto the source's WAL prefix and its recovery fails with "WAL archive check failed ... Expected empty archive". This was reproduced with a backup triggered a few seconds after an in-place restore, and an hourly plan overlaps a restore of up to thirty minutes about half the time. A BackupJob that has not started yet now waits while a non-terminal RestoreJob in its namespace writes into the same application, resolved as the drivers resolve it: the Backup's applicationRef, with the target's kind and name when a targetApplicationRef is set, and the target's API group only when it names one. A RestoreJob whose Backup is gone holds nothing, since the restore controller fails it on its next pass. options.targetNamespace is honoured only for a Backup taken by the Velero strategy, the one driver that reads it; the CNPG and Job-based drivers ignore the option and restore in place, so their restores hold whatever it says. The held job stays Pending with a Ready=False RestoreInProgress condition naming the restore and is requeued, rather than failed, so a scheduled run still happens once the restore ends. A failed write of that status is returned, so the manager retries it. When the restore ends the condition is removed before the job starts, since it would otherwise read "waiting" until the driver next writes Ready, which for a healthy run is when the backup completes. A run that has already started is left alone: its driver owns state the gate would strand. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Aleksei Sviridkin
|
e62f510faf |
chore(deps): move the Kubernetes libraries to v0.37.1
The root module's k8s.io/* move from v0.35 to v0.37.1 and controller-runtime from v0.23 to v0.25.1, its release for Kubernetes 1.37. Two things held them back: the apimachinery fork, which had to be rebased onto every minor, and the watch handler that from k8s.io/apiserver v0.36 panicked on the initial-events-end bookmark the aggregated storages sent to plain watches. Both are gone. The rest of the upgrades are what these releases require: cel-go v0.29.2, the etcd client v3.7.0, kube-openapi, structured-merge-diff and the x/ modules. code-generator moves with the libraries. Regenerating the strategy CRDs that embed core types adds the Kubernetes 1.37 fields and follows one removal: the alpha PodSpec field workloadRef became schedulingGroup upstream. Once the new CRD is applied, the apiserver prunes workloadRef from every stored strategy it serves, and the object's next write erases it from etcd. It only ever took effect behind an alpha feature gate. The gateway README and the tenantgateway CRD tests cite the apiextensions-apiserver list-type ratchet at the pinned version; it is byte-identical in v0.37.1, so only the version they name changes. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
3dde6503ec |
refactor(api): keep each app's kind without the apimachinery fork
Every apps kind is registered on the one Application Go type, and the stock Scheme.ConvertToVersion picks the first kind registered for a type, so without help a Redis comes back from the API as whichever app kind was registered first. So far the fix lived in a fork of k8s.io/apimachinery that patches ConvertToVersion. Upstream has not taken the change, and the fork has to be rebased onto every Kubernetes minor before the libraries can move. The apps group now encodes through a serializer that picks the kind the object carries, along the lines upstream suggested instead of changing Scheme. The replace directive goes, and with it the Renovate rule that kept the fork's stale release tags from being proposed as updates. The serializer relies on the storage stamping the kind on every object it returns, which it does. A test drives create, get, list, update, watch, apply and table through the real handler chain for two kinds and fails without the serializer on upstream apimachinery. One path converts through the Scheme rather than the serializer and goes back to the first registered kind: the row objects of a Table requested with includeObject=Object, which kubectl asks for only with --sort-by. Admission webhooks and ValidatingAdmissionPolicy get the request kind set explicitly. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
2c2c0301f7 |
chore(deps): roll up pending Renovate updates and move to Go 1.27.1 (#4447)
## What this PR does This replaces the open Renovate PRs with one rollup. Go modules move to their newest releases that keep the Kubernetes libraries and controller-runtime where they are, the toolchain and builder images move to Go 1.27.1, and the first-party code gets the Go 1.27 modernizers. Tests for the code the bumps touch come first, in their own commits, and pass on both the old and the new dependencies. Before the bumps: - The e2e sandbox image builds again: dl.min.io answers 410 for mc since MinIO archived it, so mc now comes from its last GitHub release with a pinned checksum. - Renovate stops offering tags of the apimachinery fork. They mirror upstream, so #2999 is a downgrade that drops our patch. - `make unit-tests` now runs the tests of the nested Go modules (kubevirt-csi-driver, token-proxy, kubeovn-webhook, api/apps/v1alpha1). They never ran in CI before. - New tests: JWT verification in the dashboard token proxy, the kube-ovn plunger metrics, cert-manager object idempotency in the tenant gateway, values-file layering in the operator, and the kubevirt CSI driver wrapper. - One real fix. The lineage webhook decoded admitted objects with encoding/json, so an int64 above 2^53 came back rounded. jsonpatch v2.5 compares numbers exactly and would emit a replace that rewrites such a field in the pod. The webhook now decodes with apimachinery's json. Held back on purpose, with the reasons in the commit messages: - Kubernetes libraries stay on v0.35. #3001 needs the apimachinery fork rebased onto release-1.37 first. - cel-go v0.29.2 and etcd v3.6.15, the lines k8s.io/apiserver v0.35 is built and tested with. - grpc v1.83.2. v1.84.0 is affected by GO-2026-6443 and no v1.84 patch release carries the fix yet. - kubevirt.io/api and the CDI API stay on what upstream csi-driver builds against. From CDI v1.60 it no longer compiles. - api/apps/v1alpha1 keeps go 1.26.4, so consumers of the published types are not forced onto Go 1.27. - go fix runs without embedlit (Go 1.27-only syntax, breaks backports to release branches on Go 1.26) and omitzero (would make metadata, spec and status required in the published OpenAPI schemas), and one errorsastype rewrite that did not compile is left out. govulncheck reports nothing reachable in any of the five modules, standard library included. Supersedes #4311, #4310, #4309, #3452, #3993, #3927, #3902, #3805, #3451, #3415, #3414, #3413, #3302, #4050 and #3363. #3001 stays open. Not checked locally: the images that build upstream projects (kube-ovn, kamaji, cilium, linstor-csi and others) with the Go 1.27 builder. The image build jobs of this PR cover them. Closes #4487. ### Screenshots No UI changes. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note chore(deps): move the toolchain and builder images to Go 1.27.1 and raise Go dependencies to their latest compatible releases, closing the published advisories; the lineage webhook no longer rewrites int64 fields above 2^53 in admitted objects. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a restore resource policy option for Velero strategies. * Added a test target to run Go tests across tracked modules. * **Bug Fixes** * Updated the end-to-end test environment to download and verify the MinIO client from its current release location. * Added coverage for token validation, webhook patching, controller behavior, and reconciliation. * **Maintenance** * Updated the Go toolchain, dependencies, and container image versions. * Refreshed pinned workflow actions and base image digests. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |