mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-03 10:37:21 +09:00
docs/system-memory-limits
5540
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Myasnikov Daniil
|
64253ed2db |
docs(platform): narrow two claims to what actually holds
The bold lead asked for the limit to stay above the largest memory request "declared in any system namespace", which is wider than the check beneath it and wider than the truth. A container declaring its own memory limit is never defaulted, so its request cannot collide with the knob. The script already filters those out; the lead now says the same thing. The VPA sentence stated proportional limit rescaling as a general property. It holds only under controlledValues: RequestsAndLimits. That is the upstream default and no VPA in packages/ sets the field at all, so the claim is true here, but under RequestsOnly a raised request would leave the defaulted limit behind and produce exactly the admission rejection the paragraph rules out. Say which case is being relied on. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
bb82696a2b |
docs(platform): keep NODE and NODE_IP in scope past the subshell
The previous round wrapped the cgroup check in ( ... ) so that set -eu and the exit in the QoS case could not take an interactive shell down with them. That also moved the POD_UID, NODE and NODE_IP assignments into the subshell, while two later uses stayed outside it: the talosctl ls fence and the kubectl debug node/$NODE sentence. Both expanded empty. Hoist the three lookups back to the top level and keep the subshell around the part that needs it. Guard the values explicitly rather than relying on set -e, which never covered this case either: kubectl get -o jsonpath exits zero and prints nothing when the field is absent, which is the ordinary state of an unscheduled pod, so the old form ran talosctl -n with an empty address. The QoS case and the cgroup path are unchanged. Verified against stubbed kubectl and talosctl that all four branches render the same argv as before and that both later uses now expand. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
d5f8d6ce0f |
docs(platform): fix two claims about the Talos OOM handler
Two statements about upstream Talos v1.13.6 were wrong.
The last paragraph put static pods in the set of causes that
reach the trigger's first clause. They cannot. Talos maps
cgroup subtrees to QoS classes in the OOM controller's
PopulatePsiToCtx: init and system go to System, podruntime to
Podruntime, and kubepods/{besteffort,burstable,guaranteed} to
the three pod classes. Static pods get pod cgroups under
kubepods/ like any other pod, so their class carries no weight
in either of the first clause's vectors, and
multiply_qos_vectors skips a class the weight literal does not
name. Only the cause-blind second clause can react to them.
kubelet and the CRI containerd do live under podruntime, so
that half of the sentence was right and is kept.
The note on cross-checking the CEL defaults gave the wrong
reason. The OOMConfig doc comments neither inline the
expression values nor reference the constants; they describe
the expressions' inputs. The rendered reference gets its
expressions from the same constants via the examples:
annotation, so the expressions there are correct. The reason
to distrust it is that it renders one example document rather
than a table of defaults: it re-serialises the expressions
with different parenthesisation and sets sampleInterval to
100ms against a 500ms DefaultOOMSampleInterval, with nothing
marking which fields are defaults.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
a4ac54ac72 |
docs(platform): scope the JVM exemption to the one on main
#3495 merged as
|
||
|
Myasnikov Daniil
|
69c6e89b4a |
docs(platform): pin the CEL defaults and narrow the admission warning
Three points from lexfrei's review. Pins both CEL blocks to the constants that define them, DefaultOOMCgroupRankingExpression and DefaultOOMTriggerExpression in pkg/machinery/constants/constants.go, and says to cross-check the constant at the tag rather than a rendered configuration reference. Verified both against siderolabs/talos at v1.13.6: they match this page verbatim. The reference pages are worth steering away from, because the OOMConfig doc comments point at the constant instead of inlining its value, so a rendered page can show an older expression while the shipped default has moved on. Narrows the admission-ordering warning to the case that can actually happen. The page presented a defaulted limit landing below a container's own request as the general hazard of lowering the knob, which overstates the routes into it. LimitRanger is an in-tree admission plugin at position 6 of AllOrderedPlugins while MutatingAdmissionWebhook is at 33 (pkg/kubeapiserver/options/plugins.go, release-1.35), so the LimitRange has already defaulted a container before any webhook sees the pod, and VPA "will also maintain ratios between requests and limits that were specified in initial containers configuration", so it rescales that limit rather than leaving a bare request above it. Only a statically declared request reaches the rejection. That also corrects why the check needs live pods: not to catch a webhook-raised request, but to catch a container still running from before the LimitRange was applied. Corrects the scope of the kube-ovn init containers. hostpath-init is not confined to the node DaemonSets: it runs on six workloads, three DaemonSets (ovs-ovn, kube-ovn-pinger, kube-ovn-cni) and three Deployments (ovn-central, kube-ovn-controller, kube-ovn-monitor), plus ovn-ic-controller under func.ENABLE_IC. install-cni is the one that is DaemonSet-only. Also follows the operator's field rename, describing what is logged as the workload rather than the pod. No command text changed. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
59856008d8 |
docs(platform): narrow the coverage claim and fix three commands
Addresses the review on the memory-limits page.
The opening claim that every container in a system namespace gets a
memory limit was too broad, but not for the reason offered. The
LimitRange defaults a limit onto any container that declares none,
whoever created the pod, so chart-level exemptions do not survive it:
a namespace LimitRange with default memory 4Gi turns a container
requesting 700Mi with no limit into one carrying limits.memory 4Gi.
Verified on kind. The real gaps are the ones admission cannot reach or
the operator withholds: Talos static pods bypass LimitRanger entirely,
and a namespace holding a live container that requests more than the
configured limit and declares none of its own loses its LimitRange
altogether. Both are now named in the opening paragraph, and the
withheld-namespace case joins the "does not cover" list with the VPA
maxAllowed of 8Gi in packages/system/monitoring as the case to expect
it in.
Records the two deliberate chart-level exemptions, linstor-controller
and linstor-satellite, as what they are: the same linstor-server JVM,
where a memory.max is what container-aware heap sizing reads, so a
guessed ceiling shrinks the heap rather than capping the process. Both
still receive the namespace default, so they do leave the victim set,
at a namespace-wide figure rather than a fitted one; closing that needs
a peak measured at a high DRBD resource count plus an explicit heap
share.
Three commands were wrong or unsafe as written:
- the pre-change request check queried live Pods only, so it missed
scaled-to-zero workloads and CronJobs. It now reads pod, workload
and CronJob templates alongside live pods, needing both halves
because a template is the only place a scaled-to-zero workload
appears and a live pod the only place a webhook-raised request
does. It also failed silently: with an unreachable cluster it
printed nothing and exited 0, reading as a clean bill of health.
Wrapped in a subshell under set -eu -o pipefail with an explicit
empty-namespace-list check, it now exits non-zero instead.
- the cgroup check hardcoded the burstable QoS segment while the
prose two lines below explained the segment varies. It now derives
the segment from .status.qosClass, and an unreadable class stops
the command rather than sending it to a path that cannot exist.
- the closing pressure diagnosis asserted that persistent triggering
means a pod is at its memory ceiling. The first trigger clause
weighs only System and Podruntime pressure, so it says nothing of
the kind; the diagnosis now sends the reader to the trigger context
first.
Also labels the two CEL fences, which markdownlint reports as MD040,
and drops the em dashes throughout.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Aleksei Sviridkin
|
1c00ddb0f3 |
fix(platform): give node DaemonSets memory requests and limits (#3495)
## What this PR does Sets memory requests and limits on the node DaemonSets that had none. Companion to the namespace-wide LimitRange; the reasoning for why a limit specifically (and not a request, and not a `PriorityClass`) is in that PR and in `docs/operations/system-memory-limits.md`. Requests come from 14-day peak working set measured on a 3-node dev cluster. Limits are deliberately loose, sized as a runaway backstop rather than a fitted ceiling, for two reasons: several of these scale with cluster size in ways a small dev cluster understates (cilium with endpoint and policy count, fluent-bit with log volume, multus and kube-ovn with pods per node, velero node-agent during a backup), and undersizing converts a rare pressure-driven kill into a deterministic one. This repository already carries a scar from that failure mode, recorded in the `vertical-pod-autoscaler` values where a 110Mi limit OOM-killed the updater and cascaded into failed HelmReleases. Two DaemonSets are not chart-rendered and needed their own seam. `linstor-satellite` goes through the `LinstorSatelliteConfiguration` podTemplate, including the two plunger sidecars — every container in the pod needs a limit for the pod cgroup to get `memory.max`, so one limit-free sidecar would put the whole pod back in the victim set. `virt-handler` goes through KubeVirt `customizeComponents.patches`, because virt-operator owns that DaemonSet and chart values cannot reach it. `linstor-controller` is deliberately left limit-free. It is a JVM that must not be OOM-killed mid-operation, and `cluster_test.yaml` pins that as an invariant. The satellite is a Go binary with a bounded working set, so the opposite call applies there. Not covered, because upstream hardcodes the containers with no `resources` knob: the four frr-k8s `cp-*` init containers, the kube-ovn `hostpath-init` and `install-cni` init containers, and `cozy-proxy`, whose vendored chart exposes no resources value at all. `hami` and `kilo` are left alone as optional components with no usage measurements behind them. All of these still receive a limit from the namespace LimitRange, so they are not left exposed — they simply carry no request. Worth stating plainly: this PR does **not** grant OOM immunity on its own, because the init containers it cannot reach leave the pod cgroup without `memory.max`. That only closes with the LimitRange PR in place. ### Verification Every touched package renders and its helm-unittest suite passes. New assertions cover the metallb speaker and all five frr-k8s containers, the linstor satellite and plunger sidecars, and the KubeVirt patch. The metallb suite was mutation-tested: removing the frr-k8s reloader limit fails it. One gap. The KubeVirt patch shape was validated against the live CRD and pushed through the real update path with `kubectl patch --dry-run=server` (accepted; live object confirmed unchanged), but virt-operator actually reconciling the DaemonSet with those resources has **not** been observed, because that means restarting virt-handler fleet-wide. Worth confirming in e2e or on a dedicated stand before merge. ### Downstream repositories Walked the trigger map against the diff. No entry is triggered: this changes chart values and cozystack-owned templates under `packages/system/`, not the `cozy-proxy` label and annotation contract, not node prerequisites, not an app schema or version enum. Leaving the boxes empty rather than ticking "none affected", because the diff surfaced a real gap in a downstream repository even though nothing here forces a change there: the vendored `cozy-proxy` chart exposes no `resources` value, so its DaemonSet cannot be given one from this side at all. That wants an issue or PR in cozystack/cozy-proxy followed by a re-vendor. Flagging it for a maintainer decision rather than filing speculatively. ### Release note ```release-note fix(platform): node DaemonSets (cilium, metallb speaker and frr-k8s, kube-ovn, linstor-satellite, virt-handler, fluent-bit, node-exporter, multus, velero node-agent, kubevirt-csi-node) now declare memory requests and limits, so they are no longer selected as victims by the Talos userspace OOM handler when an unrelated workload drives node memory pressure. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Resource Management** - Added CPU and memory requests and limits across Cilium, KubeVirt CSI, KubeVirt, Linstor, MetalLB, monitoring agents, Multus, and Velero workloads. - Improved workload stability with defined memory ceilings and resource reservations. - **Multus** - Moved resources to the dedicated `cozy-multus` namespace and configured Cilium as the master CNI. - Added optional CNI plugin staging with enhanced security settings. - **Tests** - Added and expanded Helm tests validating resource configurations and Multus behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
b7b56530b9 |
docs(platform): explain system component memory limits (#3496)
## What this PR does Adds `docs/operations/system-memory-limits.md`, recording why system components carry memory limits — which is not recoverable from the values alone, and is the kind of thing that gets "cleaned up" by a well-meaning future change. The Talos userspace OOM handler (v1.12+) selects victims by ranking cgroups that lack `memory.max`. Three consequences are each easy to get wrong independently: only a limit grants immunity (a request just moves BestEffort to Burstable, which under `strictCgroupClassOrdering` means "killed second"); every container in the pod needs one, init containers included, because kubelet only sets pod-level `memory.max` when they all have it; and `PriorityClass` — `system-node-critical` included — has no effect whatsoever, because the handler walks cgroupfs and never talks to the API server. That last one is the trap, since setting it looks like a fix. The page also covers the two installer knobs and the constraint that makes them dangerous to lower (a defaulted limit below a container's own request fails admission), how to verify a pod cgroup actually carries `memory.max` rather than trusting the QoS class, what is deliberately left uncovered (Kubernetes static pods are Talos-managed; a handful of vendored containers expose no upstream resources knob), and how to relax the global-PSI backstop clause of the trigger for a cluster that trips it persistently. Ordering note: this documents behaviour introduced by the LimitRange and DaemonSet PRs, so it should not merge ahead of them. ### Downstream repositories Walked the trigger map against the diff. This adds one file under `docs/operations/`, alongside the existing pages there; nothing in the map covers that path, and the website keeps its own copy of user-facing documentation rather than generating it from this directory. A maintainer may still want a version of this on the website, which is a judgement call rather than a forced follow-up. - [x] No downstream repository is affected by this change ### Release note ```release-note docs(platform): document why system components carry memory limits, how the Talos userspace OOM handler selects victims, how to tune or disable the system-namespace LimitRange, and why PriorityClass does not influence any of it. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added guidance on Talos userspace out-of-memory behavior and system component memory limits. * Documented namespace boundaries, installer tuning, validation constraints, rollout behavior, and verification steps. * Added instructions for inspecting OOM actions, identifying uncovered workloads, evaluating trigger expressions, and configuring optional overrides. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
22ce998890 |
fix(flux-shard-operator): repair sharded helm-controller crashloop behind an HTTP proxy (#3546)
## What this PR does Fixes #3544. `flux-shard-operator` clones the flux-aio `helm-controller` container into each `helm-controller-shard<i>` Deployment. It already sanitises the all-in-one localhost wiring, but keeps two things that stop the standalone shard from starting behind a corporate HTTP proxy: 1. **Inherited `HTTP_PROXY`/`HTTPS_PROXY`/`NO_PROXY`.** A standalone shard needs no external egress (source-controller fetches artifacts). Behind an unreachable proxy the controller makes a blocking startup HTTPS call that never completes, the manager never serves `/healthz`, and the liveness probe crashloops the pod forever. flux-aio survives only because it starts once and never restarts. On an affected cluster every HelmRelease carrying a `sharding.fluxcd.io/key` (150 of ~240) was frozen for days, silently. 2. **No `startupProbe`.** The clone inherits only a ~30s liveness window, so any slow start (large cache sync) is fatal and unrecoverable. This PR extends the sanitisation to drop the proxy env, and derives a `startupProbe` from the liveness handler with a generous budget so a slow-but-progressing start is not killed. Verified by an A/B on a live cluster (throwaway helm-controller with a non-matching selector): with the proxy env it hangs; without it, it starts cleanly (`Starting workers`). ### Downstream repositories - [x] No downstream repository is affected by this change Internal operator behavior only; no API/CRD/values contract change. ### Release note ```release-note fix(flux-shard-operator): sharded helm-controller no longer crashloops at startup behind an HTTP proxy; the cloned controller drops the inherited proxy env and gains a startupProbe, so HelmReleases assigned to shards keep reconciling in proxied environments. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented inherited proxy settings, including uppercase and lowercase variants of `NO_PROXY`, from affecting Flux shard deployments. * Automatically adds a startup health check when only a liveness check is configured. * Preserved the liveness check’s handler and timeout while applying appropriate startup timing and failure settings. * **Documentation** * Documented startup health checks for slow helm-controller startup. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
b7e51d54a5 |
fix(backupstrategy-controller): gate the default MongoDB strategy on bootstrap (#3815)
## What this PR does Closes #3814. Unblocks a fresh install of `main`. Since #3562 the `backupstrategy-controller` chart renders one default Strategy CR ungated, and that makes the whole release fail on a cluster where its CRDs are not yet applied: ``` Helm install failed for release cozy-backup-controller/backupstrategy-controller resource mapping not found for name: "cozy-default-mongodb" no matches for kind "MongoDB" in version "strategy.backups.cozystack.io/v1alpha1" ``` The chart ships its own eight `strategy.backups.cozystack.io` CRDs as ordinary templates, since `templates/crds.yaml` globs `definitions/*.yaml`, and Helm pre-applies only a `crds/` directory. Helm then resolves every document in the release manifest through the cluster's RESTMapper in `Build()` before it applies any of them, so an ungated Strategy CR has no mapping on the first install, `Build()` fails, and the release applies **zero** objects, including the CRDs that would have made the mapping resolve. Nothing recovers from that. Every retry renders the same manifest, so it is permanent rather than a race, and Helm's kind sorter cannot help because it orders the apply and the failure is before the apply. The install log shows `Warning InstallFailed 43s (x36 over 20m)` and then `Install failed (no retry)`. ### Why the seven siblings never hit this Each of them gates on a resolved `$bucketName`, which is empty until the COSI BucketClaim status is reconciled, so they are simply absent from the first render. By the time `DefaultObjectsGate` forces a second revision the CRDs from revision 1 exist and the mapping resolves. That gate is documented as "the CR needs the bucket name", and it silently doubles as the CRD-ordering guard. MongoDB genuinely does not need the bucket name, the template argues that correctly, so the gate was left off and the accidental protection went with it. This gates it too, for the ordering reason rather than the value, and records that reason in the template so the next bucket-independent strategy keeps it. Convergence is unchanged. `DefaultObjectsGate` already routes `apps.cozystack.io/MongoDB` to `cozy-default-mongodb` and forces a revision once any routed object is missing, which is exactly how the seven siblings materialise today. ### Why this was invisible `tests/rendering_test.yaml` asserted `count: 1` for the MongoDB template under a case named "bootstrap window (no resolvable bucket name)", so the defect was codified as the expected behaviour and shipped green. helm-unittest has no RESTMapper and cannot see the consequence. That assertion is flipped here, with the ordering reason written next to it. It was also masked in CI until yesterday by #3806. `StreamVisitor.Visit` returns immediately on a validation error but only continues on a mapping error, so the OpenAPI defect aborted the manifest stream at the first document and the mapping check was never reached. #3808 fixed that and this surfaced underneath it. ### Screenshots Not applicable. ### Downstream repositories - [x] No downstream repository is affected by this change ### Testing Verified by rendering both windows, since the whole bug is which documents appear in the first render and a cluster adds nothing to that. Before, bootstrap window: one Strategy document, `MongoDB`, alongside the eight not-yet-applied CRDs. After: none. With `backupStorage.bucketNameOverride=b`, which stands in for a resolved BucketClaim: all eight, MongoDB included. ``` helm template bsc packages/system/backupstrategy-controller | grep -c '^kind: MongoDB$' ``` `helm unittest packages/system/backupstrategy-controller` passes, 4 suites, 18 tests. ### Follow-up, not in this PR The durable fix is to stop shipping these CRDs as templates and split `definitions/` into a `backupstrategy-controller-crds` package with a `dependsOn` edge, which is the convention six other packages already follow. That cannot be a hotfix: none of the eight definitions carries `helm.sh/resource-policy: keep`, so the upgrade that removes them from this chart's manifest would have Helm delete the eight CRDs and cascade-delete every live Strategy CR on existing clusters. It needs the `keep` annotation shipped a release ahead. ### Release note ```release-note fix(backupstrategy-controller): gate the default MongoDB backup strategy on the bootstrap window, so a fresh platform install no longer fails with "no matches for kind MongoDB" and applies nothing ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented the default MongoDB backup strategy from rendering before the required S3 storage configuration is available. * Improved first-install reliability by avoiding premature strategy resource creation. * **Tests** * Updated bootstrap validation to confirm the MongoDB strategy is omitted until a bucket name is resolved. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
4f9c20cac0 |
fix(backupstrategy-controller): gate the default MongoDB strategy on bootstrap
A fresh install of packages/system/backupstrategy-controller fails permanently and applies nothing: Helm install failed for release cozy-backup-controller/backupstrategy-controller resource mapping not found for name: "cozy-default-mongodb" no matches for kind "MongoDB" in version "strategy.backups.cozystack.io/v1alpha1" The chart ships its own strategy.backups.cozystack.io CRDs as ordinary templates (templates/crds.yaml globs definitions/*.yaml), and Helm resolves every document in a release manifest through the cluster's RESTMapper in Build() before it applies any of them. An ungated Strategy CR therefore has no mapping on the first install, Build() fails, and the release applies ZERO objects, including the CRDs that would have made the mapping resolve. Nothing recovers: every retry renders the same manifest, so this is permanent rather than a race, and Helm's kind sorter cannot help because it orders the apply, not the Build. The seven sibling strategies never hit this because each gates on a resolved $bucketName, which is empty until the COSI BucketClaim status is reconciled. That gate is documented as "the CR needs the bucket name", and it silently doubles as the CRD-ordering guard. MongoDB genuinely does not need the bucket name, so the gate was left off and the accidental protection went with it. Gate it too, for the ordering reason rather than the value, and record that reason in the template so the next bucket-independent strategy keeps the gate. Convergence is unchanged: DefaultObjectsGate already routes apps.cozystack.io/MongoDB to cozy-default-mongodb and forces a second revision once any routed object is missing, which is how the seven siblings materialise today. Verified by rendering both windows. Before: the bootstrap render emits one Strategy, MongoDB, alongside the eight not-yet-applied CRDs. After: it emits none, and with backupStorage.bucketNameOverride set it emits all eight. tests/rendering_test.yaml asserted count: 1 for MongoDB under a case named "bootstrap window (no resolvable bucket name)", so the defect was codified green. helm-unittest has no RESTMapper and cannot see it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
15459c96e5 |
fix(api): declare OpenAPIModelName for core and sdn types (#3808)
## What this PR does Fixes #3806. Declares `OpenAPIModelName` for the `core` and `sdn` API groups the way #3273 already did for `apps`, so every published OpenAPI definition name is the dotted Kubernetes model name rather than a Go import path. Worth noting the two are the same event. #3273 landed the k8s 0.35 work, hit the loud half of this in server-side apply where no apps kind resolved at all, fixed `apps`, and left `core` and `sdn` with the quieter half. Its merge commit `960d85370` is exactly where the window opens: 190 failed-E2E logs from 2026-06-25 to 07-19 carry no occurrence, and the first one is 11 hours after it. Before this, 20 definitions shipped under names like `github.com/cozystack/cozystack/pkg/apis/core/v1alpha1.Option`, while every `$ref` pointing at them spelled each slash `~1`. Clients resolve a `$ref` by trimming `#/definitions/` and looking the remainder up verbatim with no unescaping, so the two spellings never meet and the reference dangles. `kubectl apply --validate` against a cluster running a `cozystack-api` built after 2026-07-21 therefore fails on any resource, and in e2e the platform install fails on `cozy-backup-controller/backupstrategy-controller` and never completes. The mechanism is a Kubernetes 0.35 change: `DefinitionNamer.GetDefinitionName` now returns the model name verbatim, where it previously converted the Go import-path form into the friendly reversed-path form. So whatever name the generated map uses becomes both the published key and the escaped `$ref`, and slash-freedom stops being a nice-to-have and becomes the correctness condition. ### Why hand-written methods openapi-gen can emit these with `--output-model-name-file`, but that also rewrites `zz_generated.model_name.go` inside the read-only apimachinery and apiextensions module-cache packages that the shared `gen_openapi` helper always passes as inputs, which fails for any consumer that vendors deps from the cache, CI included. The `apps` group settled this the same way and the reasoning is repeated in each new `model_name.go` so nobody simplifies it back into the flag. Once a package carries `+k8s:openapi-model-package`, openapi-gen emits `Type{}.OpenAPIModelName()` for every type it writes a schema or a `$ref` for, so a missing method is a build failure rather than a silent skip. That makes a future omission inside these groups loud by construction, and the new tests make an omission on a future group loud at the group level. ### Guard tests `pkg/generated/openapi/definitions_test.go` adds two, both asserting the invariant rather than today's 20 names, and both refusing to pass on an empty or cozystack-free definition map so they cannot go vacuously green. `TestDefinitionNamesAreDottedModelNames` fails on any definition name containing a slash, naming the offender and the fix. `TestDefinitionRefsResolve` builds each `$ref` the way kube-openapi's builder does and resolves it the way a client does, which checks the failure directly rather than through the slash-freedom proxy, and also catches a group whose types disagree with each other. Nothing asserted this before, which is why the defect reached v1.6.0 and v1.6.1 unnoticed. ### Screenshots Not applicable. ### Downstream repositories Walked the trigger map against the diff. Published model names change, so I swept for consumers pinning the old form: every hit on `github.com/cozystack/cozystack/pkg/apis` in the tree is a Go import, which is unaffected because the package path does not change, and `api/api-rules/cozystack_api_violation_exceptions.list` keys off the Go package path in its own format and was unchanged by regeneration. No string literal anywhere pins the old model-name form, and `apiPrefix` in `pkg/cmd/server/openapi.go` is an apps-only const that this does not touch. - [x] No downstream repository is affected by this change ### Testing - `go build ./... && go vet ./...` - `go test ./...` - `make generate && git diff --exit-code`, clean and idempotent - `pre-commit run --all-files` - Both guard tests run against the pre-fix tree first: `TestDefinitionNamesAreDottedModelNames` named all 20 offenders and `TestDefinitionRefsResolve` reproduced the reported error string with 15 dangling refs, fewer than 20 because some of the 20 are only referenced-from and never referencing. - An independent check driving the real `openapi.NewDefinitionNamer(apiserver.Scheme)` reports 104 published definitions with 0 slashes, 106 emitted refs with 0 dangling, and all 23 cozystack kinds resolving to a published definition carrying `x-kubernetes-group-version-kind`, which also confirms the `apps` server-side-apply fix still holds. `make unit-tests` aborts at `hack/ghcr-mirror_test.bats`, which fails on a pristine `origin/main` in my environment while passing in CI, so I ran the chain standalone: the other 60 bats files pass, as do `helm-unit-tests`, `go-unit-tests`, `rd-presets-check`, `test-check-readiness` and `migrations-target-check`. ### Release note ```release-note fix(api): publish OpenAPI model names for the core and sdn API groups as dotted Kubernetes names, so `$ref`s resolve and client-side validation works again against the aggregated apiserver ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Enhancements** * Added OpenAPI metadata for Core v1alpha1 API models. * Added OpenAPI metadata for SDN v1alpha1 API models. * API schemas now provide consistent canonical model names, supporting more reliable documentation and schema references. * Added package-level OpenAPI markers to improve API discovery and tooling compatibility. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
1b4b6ff5e5 |
test(e2e): warm the ghcr.io mirror before the tenant suites need it (#3768)
## What this PR does The most expensive e2e flake, cozystack/cozystack#3513, is a tenant worker missing the 18m node-Ready budget with zero Nodes registered. The in-guest capture in cozystack/cozystack#3548 diagnosed one variant: the worker's kubelet image pull crawls to ghcr.io at 40-165 KB/s over the runner egress and does not finish inside the budget, Talos holds the kubelet service until its image arrives, so the node never registers and the tenant cilium HelmRelease then fails for want of a schedulable node rather than for any fault of its own. The sandbox already runs an in-cluster pull-through mirror for ghcr.io, but a pull-through cache fetches a blob only when a client first asks for it, so the first tenant worker pays the cold fetch inside its node-Ready budget. Measured with the same fetch script against a local instance of the same registry image: 32m47s cold, 1.1s warm, for the same tags the failing runs stalled on. This PR fills the cache from a Job started by the same install step that deploys the mirror, tens of minutes before the kubernetes suites run. It warms the two kubelet tags the suites actually select, resolved from the chart's version map in the same order the suites resolve it, and one platform per tag (the one the workers run, ~61 MiB each) - warming what nobody pulls would be a true addition to the same throttled egress the install itself is using. The warm-up is best-effort by construction: a Job that fails, times out or never starts does not fail the install, and the suites behave exactly as they did before it existed. The Job itself exits non-zero on any missed tag, so its status stays honest. The node-join failure diagnostics now answer the questions a red run used to leave open: whether a worker pulled through the mirror at all (counting GET and HEAD access lines per request, excluding the warm-up's own requests, which name themselves via user-agent), how long the mirror took to answer (server-side `http.response.duration` is the only after-the-fact signal separating a warm cache from a cold one, since the proxy stores blobs in the background and the Job reports success either way), and whether the warm-up itself got anywhere. The diagnostic section grows from five to seven bounded reads, moving its worst case from roughly 100s to roughly 150s inside the existing phase budget gate; unbudgeted diagnostic residuals stay tracked in cozystack/cozystack#3666. This addresses one variant of cozystack/cozystack#3513 and does not close it: the zero-CSR class remains untouched, and a green suite after this change is not evidence the flake is gone. The 18m node-Ready budget is not raised. Covered by 65 unit tests in `hack/ghcr-mirror_test.bats`: tag selection and ordering against the suites' own expressions, rejection of malformed and injected tag values, Job shape and restricted security context, the readiness wait as a wall clock, every failure path of the new diagnostics including an over-1-MiB log fixture that pins the duration filter against real log sizes, and the exclusion of the warm-up's own traffic from both the request count and the duration report. ### Screenshots No UI changes. ### Downstream repositories The diff is confined to e2e infrastructure under `hack/`: the mirror manifest, the warm-up library and its tests, the install step, and diagnostic labels. Walked the trigger map file by file - no chart values, no APIs, no documentation content, nothing any downstream repository consumes. - [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: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added best-effort GHCR mirror warm-up during installation. * Preloads selected Kubernetes image manifests and blobs to improve cache readiness. * Warm-up failures safely fall back to direct image pulls. * **Diagnostics** * Improved request classification, timing, repository identification, and reporting of warm-up status and logs. * **Tests** * Expanded coverage for warm-up behavior, validation, retries, security settings, caching, logging, installation, and diagnostics. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
cea1703a6d |
test(api): guard published openapi definition names and refs
Nothing asserted that a published definition name contains no slash, which is why the core and sdn groups shipped as Go import paths in v1.6.0 and v1.6.1 without anyone noticing. The same omission on a future group would be just as invisible. TestDefinitionNamesAreDottedModelNames fails on any definition name containing "/", names the offender and points at the fix (the +k8s:openapi-model-package marker plus an OpenAPIModelName method). TestDefinitionRefsResolve is the failure mode itself: it builds each $ref the way kube-openapi's builder does, JSON pointer escaped, then resolves it the way a client does, trimming "#/definitions/" with no unescaping, and reports any that dangles. It also checks the declared Dependencies against the published names, which catches a group whose types disagree with each other rather than uniformly. Both assert the invariant instead of listing today's 20 names, so a new group that omits the marker is caught rather than a changed count, and both guard against going vacuous if the definition map is ever emptied or stops covering cozystack's own types. Both fail on the pre-fix tree: the first names all 20 offending definitions, and the second reproduces the reported error verbatim, down to "github.com~1cozystack~1cozystack~1pkg~1apis~1core~1v1alpha1.OptionSpec". Refs: https://github.com/cozystack/cozystack/issues/3806 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
5f3ad9cd58 |
fix(api): declare OpenAPIModelName for core and sdn types
The aggregated apiserver publishes the core.cozystack.io and
sdn.cozystack.io models under their Go import path, so a $ref to any of
them does not resolve and client-side validation fails on every
resource of those groups, not only the one named in the error:
error validating data: SchemaError(…core/v1alpha1.Option.spec):
unknown model in reference:
"github.com~1cozystack~1…v1alpha1.OptionSpec"
Since Kubernetes 0.35 the apiserver's DefinitionNamer.GetDefinitionName
returns the model name it is handed verbatim; it no longer converts the
Go import-path form into the "friendly" reversed-path form. Whatever
name the generated openapi map uses therefore becomes both the
published definition key and, JSON pointer escaped, the $ref pointing
at it. A name containing "/" ships as a definition keyed on the raw
path while every reference to it spells each slash "~1", and clients
resolve a $ref by trimming "#/definitions/" without unescaping
(kube-openapi pkg/util/proto/document.go). The two spellings never
meet, the reference dangles, and validation of the whole document
fails.
Mirror
|
||
|
Aleksei Sviridkin
|
829edfadb3 |
test(e2e): warm the ghcr.io mirror before the tenant suites need it
The in-sandbox pull-through cache fetches a blob only when a client asks for it, so the first tenant worker's kubelet pull is a live fetch from ghcr.io. That leg is what misses the node-Ready budget: measured at 40-165 KB/s for a 61 MiB image, with two blob streams dying mid-transfer after 7-8 MiB on an upstream HTTP/2 reset. Talos holds the kubelet service in PreFunc until the pull completes, so the node never registers, and the tenant cilium HelmRelease then fails for want of a schedulable node rather than for any fault of its own. Fill the cache from a Job started in the same install step that deploys the mirror, tens of minutes before the kubernetes suites run. The Job waits for the mirror to serve, then asks it for each kubelet tag's index, walks the manifests it gets back, and discards the blob bodies; distribution populates its blobstore on the way past. It warms one platform per tag -- the one the workers run -- picked out of the index by record, which keeps the walk a grep. Walking both platforms was a true addition to the same throttled leg: arm64 is bytes nothing in the sandbox pulls, and the digest ordering put it first for both tags, so on the slow runs this exists for the deadline expired with the second tag still cold. It warms the two minors the suites select, not the whole version map. The map would be ~300 MiB at the one-platform walk this ships (~61 MiB per tag), three fifths of it for minors no suite can resolve, spent on the same throttled egress the rest of the install is using. The tag list still comes from the chart's map rather than a copy, ordered the way the suites resolve it, so the set is theirs without restating their expression. The readiness wait is a wall clock, not a per-attempt count. Cilium answers a Service with no ready endpoints by rejecting, so a probe costs nothing when the mirror is absent and the full timeout when it is merely slow; a constant was wrong in both directions across two attempts at picking one. Tags are refused by character class before the pattern is applied, so a value cannot pass by having a valid first line: grep matches per line and the pattern's anchor is end-of-line, not end-of-input. The Job exits non-zero on any miss, so backoffLimit is a real bound and a warm-up that reached nothing cannot report success. It names its own requests, because the node-join diagnostic counts kubelet GETs in the access log to tell a worker that pulled through the mirror from one that fell back to public ghcr.io. Naming them is only half of that. The same diagnostic now reports the Job's status and result and the Job outlives the suites, because excluding the warm-up without reporting it would leave a red run unable to distinguish a warm cache from a warm-up that never started. That counter also moved onto the access-line shape: registry:2.8.3 writes three lines carrying the path per request, so matching the path counted one pull as three. It reads the repository from the same variable the warm-up does, so a rename cannot leave the diagnostic counting zero while the workers pull under the new name. GET and HEAD both count: containerd resolves a manifest with a HEAD first, so a GET-anchored count reports a pull that died before its first GET as no request at all. The failure path also prints how long the mirror took to answer the worker. A warm cache and a cold one are otherwise indistinguishable in the artifacts -- the Job reports success either way, because the proxy stores the blob in the background and a completed GET is a different event from a stored blob -- and server-side duration separates them by orders of magnitude. It filters the log in the shell that read it: a single argv element is capped at 131072 bytes on Linux regardless of ARG_MAX, and probe traffic carries this log past that within about twenty minutes of mirror uptime, so handing it to a helper would empty the section on every run worth reading. Nothing is printed when the log could not be read at all, because a heading with no lines under it reads as a finding about the workers. Measured against a local pull-through registry using the script the builder emits: 32m47s cold, 1.1s warm for the same tags, and the blobs it fetches are the same digests the failing runs stalled on. A warm-up that fails, times out or never starts does not fail the step, and the suites behave as they did before it existed. It is not free, though: it pulls across the same throttled egress the install is using, which is why the warm set is two tags and not five. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
abd331b2c8 |
test(system): anchor index-addressed negative asserts to what they name (#3608)
## What this PR does
Some negative assertions in these suites can't fail for the reason their
test names give. `contains` and `notContains` compare list elements for
equality, so `--pinned-tenants=` never equals a rendered
`--pinned-tenants=tenant-a=shard0`, and `{name:
GF_AUTH_GENERIC_OAUTH_CLIENT_SECRET}` never matches an env entry
carrying a `valueFrom` beside the name. Others forbid one rendered value
under a name that claims the flag itself, so `notContains
--compatibility=cilium` leaves the kilo default render free to carry
`--compatibility=flannel` off the same values key.
Flag guards become `notMatchRegex` on `args[*]`, which matches per
element, so any value fails and so does the bare form. Mapping guards
get `any: true`. flux-shard-operator gets an `equal` on the whole
default arg list instead, and `--shard-cpu-limit=` is dropped because
that list covers it. The vendored ouroboros templates keep per-flag
guards: a whole-list pin there would go red on any upstream flag change,
under a test name about one flag.
Several of these address `containers[0]` with nothing pinning that
index, so a sidecar rendered ahead of the workload turns them green
while the flag is live on the real container. Those get an `equal` on
the container name first. A Cilium deny rule has no name, so that one
pins the selector plus the rule and `toPorts` counts. Where a positive
`contains` already sits on the same path it holds the index by itself
and gets no extra anchor.
Assertions of the same shape remain in files this PR doesn't touch,
including one in a package it does.
## Tests
Every dead guard was confirmed dead first: make the forbidden content
render, watch the old form stay green. `compatibility: flannel` in
kilo's values, `--gateway-api=true` in place of the bare flag,
`--proxy-fqdn` baked beside the composition flags, the OIDC client
secret in the `mode=None` grafana render. Each one turns the new
assertion red. Anchors checked the same way, with a sidecar rendered
ahead of the workload.
`hack/helm-unit-tests.sh` is green.
### Screenshots
None. This changes test files.
### Downstream repositories
Every changed file is under a `tests/` directory. No templates, no
values, no rendered output.
- [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:
### Release note
```release-note
NONE
```
|
||
|
Aleksei Sviridkin
|
9dc49de1c9 |
fix(apps): make the post-delete cleanup hooks report failure (#3593)
## What this PR does The qdrant and harbor post-delete cleanup hooks could not fail. Every delete was guarded by `|| echo "WARNING: ..." >&2`, neither script set `-e`, and both ended on an unconditional completion message, so the container exited 0 on every path. Helm read that as a successful hook, `helm.sh/hook-delete-policy: hook-succeeded` removed the Job together with the pod that held the warning, and an uninstall that reclaimed nothing looked exactly like one that had nothing to reclaim. Written up in #3566. Both hooks now count failures and set the exit code at the end. `set -e` would abort at the first failed delete and skip the rest of the sweep, which is the opposite of what is wanted: every remaining object should still be attempted and the outcome reported once. Already-gone objects stay a success, since every delete keeps `--ignore-not-found`. Each failure message names the object it left behind. `packages/apps/mariadb` already uses this shape. Both hooks also consumed `kubectl get` output directly: harbor by iterating `$(kubectl get ...)`, qdrant by piping it into `grep`, whose status the pipeline then reports. A failed listing arrived as an empty list, and the sweep reported success without having looked. Each listing is now captured before it is used. #3566 attributes that shape to harbor's two loops alone. qdrant has it too: `pvcs=$(kubectl get pvc ... | grep -E ... || true)` reports grep's status, so a `kubectl get` that failed left `$pvcs` empty and the hook announced "no data PVCs found; nothing to delete". Both instances are fixed here. ### The cost A hook that exits non-zero keeps its Job and its pods, because `hook-succeeded` does not fire for it. That is the point, the pods hold the log, but it means a failed uninstall now leaves objects behind where it previously left none. Nothing collects them on its own: `before-hook-creation` only fires when the hook runs again, and a `post-delete` hook runs only on an uninstall, so they stay until a release of that name is installed and uninstalled again, or until the namespace goes. The release itself is not stranded. Helm appends the post-delete hook error to its error list instead of returning on it, marks the release uninstalled, purges it from storage anyway, and only then reports `uninstallation completed with N error(s)`. The failure surfaces once and the app still goes away. Both Jobs get one retry instead of the Kubernetes default of six, and instead of none. Every delete is idempotent, so a second attempt re-issues an API call rather than re-running any logic, and one short backoff separates a transient apiserver error from a real one. These failures become visible for the first time with this change, and a signal that fires on noise stops being read, including on the day it is right. Six is the far end of the same mistake: ten minutes of backoff across seven pods outlasts the hook timeout, so Helm reports that timeout instead of the hook's own error. `packages/apps/clickhouse` and `packages/apps/mariadb` already exit non-zero from their cleanup hooks and set no retry limit, so a real failure in either spends the full backoff and reaches the operator as a timeout rather than as the error the hook wrote. Adding the exit code without the cap reproduces that. One limitation is deliberate. harbor's ACME sweep cannot tell "this kind is not served" from "I could not look". If cert-manager is torn down before the release, `kubectl get challenge` fails and the uninstall is reported failed with nothing to sweep. cert-manager is a hard dependency of the bundle harbor ships in, so the window is narrow, and a CRD-existence probe would add a second silent-skip path of its own: it would swallow a discovery failure the way this change stops swallowing a listing failure. ### Tests `hack/app-cleanup-hooks_test.bats` renders each hook's script out of its chart and runs it against a fake `kubectl`, because the property under test is the container's exit code and matching against manifest text cannot establish it. There are eight failure paths across the two hooks, six in harbor and two in qdrant, and each has a case that goes red when that one path alone is reverted to a swallowed warning. Also covered: a clean delete, and an object that is already gone in both its shapes, an empty listing and a `NotFound` on the delete itself. The fake `kubectl` models `--ignore-not-found` instead of exiting 0 unconditionally, so dropping that flag from a hook turns the already-gone cases red. Message assertions are anchored on the hook's own `ERROR: ` prefix, because the fake names the object in its own diagnostic too, and an unanchored match is satisfied by the fixture. Two manifest fields are pinned in each chart's existing helm-unittest suite, where manifest fields belong. The retry cap is one. The other is `helm.sh/hook-delete-policy`: adding `hook-failed` to that annotation hands the failed Job straight back to Helm to delete, which undoes the whole change. I checked that before adding the assert, and the reversal left both suites fully green. The qdrant Chainsaw suite explained its post-delete pod collector by quoting the guard string this change removes, and concluded that Helm always collects the pod away. That conclusion inverts here, so the note is rewritten, along with the sentence in the harbor suite that points at it. ### Screenshots Not a UI change. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff, file by file. It touches two app chart templates and adds one file under `hack/`. No package added, renamed or removed, no `values.schema.json`, no `values.yaml` default, no version enum, no `ApplicationDefinition`, no namespace, no node prerequisite, and neither `hack/package.mk` nor `hack/update-crd.sh`. - [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: ### Release note ```release-note fix(apps): the qdrant and harbor post-delete cleanup hooks now exit non-zero when a delete or a listing fails, instead of reporting success on every path. `helm uninstall` reports the failure rather than returning clean, and the hook Job and its pods stay in the namespace so the log survives to be read. Nothing collects them on its own: they remain until a release of that name is installed and uninstalled again, or until the namespace is removed. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved Harbor and Qdrant cleanup reliability by detecting listing and deletion failures. - Cleanup jobs now retry once and report failures with a non-zero status. - Cleanup continues processing remaining resources after individual failures. - Successful cleanup remains safe and repeatable, including when resources are already missing. - Harbor cleanup remains limited to resources associated with the configured Harbor host. - **Tests** - Added comprehensive coverage for successful, repeated, partial, and failed cleanup scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
d2551b6ea7 |
fix(keycloak): keep KeycloakRealmUser roles when spec.roles is unset (#3531)
<!-- 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 `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The vendored keycloak-operator image is built from a pinned upstream commit with `images/keycloak-operator/patches/*.diff` applied at build time. This patch is a backport of epam/edp-keycloak-operator#372 onto that pin, so it is dropped in the same commit that moves `COMMIT_REF` past the upstream merge. The bug is epam/edp-keycloak-operator#371. Under the default `full` strategy the operator treats `spec.roles` and `spec.clientRoles[].roles` as authoritative sets: after adding what is missing, it deletes every mapping the spec does not list. A resource that manages the user record and leaves those fields out therefore loses all of its realm roles on the first reconcile, `default-roles-<realm>` included, and with it `offline_access`, `uma_authorization` and the `account` client roles. The user keeps logging in and stops being able to do anything. Upstream fixes this in two halves that only work together. The controller returns early when the desired list is nil, so an omitted field means "not managed" while an explicit `roles: []` keeps its meaning of "remove every role". The json tags on `Roles` and `ClientRoles[].Roles` lose `omitempty`, because with it an explicit empty list is erased the first time the operator writes the object back, which adding its own finalizer already does, and comes back indistinguishable from an omitted field. Take that half away and the distinction holds only until the operator next touches the object. All eight files from the upstream PR are here. Six apply to the pin unchanged. `sync_user_roles.go` and its test are rewritten against the pinned context, where the generated client package is still named `keycloakv2` (upstream renamed it to `keycloakapi` after the pin, which is the only reason those two do not apply as-is). Tests match upstream: four controller cases for unset and empty realm and client roles, two in `api/v1` for the serialization round trip. Checked by extracting the pinned tarball, applying every `.diff` in the patches directory in the order the Dockerfile globs them, then `go build ./...` and `go test ./internal/controller/keycloakrealmuser/... ./api/...`, all green. Reverting each of the three conditions in turn fails exactly the tests that cover it, and nothing else. One caveat a reader deserves up front: this patch does not reach any cluster yet. No CI path rebuilds this package, so `hack/build-matrix.sh` returns an empty matrix for a change confined to it, and `values.yaml` keeps pointing at an image built by hand in March. The same happened to `9c5fbb69a` and `5b9b420ff`, which edited this Dockerfile and left the pin alone, so the running binary is already older than the tree says. That gap is tracked in https://github.com/cozystack/cozystack/issues/3416 and is deliberately not addressed here: the fix for it is a choice between adding these packages to the build list and dropping the in-repo build, and the first option wakes a known mirror gap for split-host references. Landing the patch now means it is in place the moment that is resolved. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> Not a 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. --> Walked the trigger map against the diff. It touches one build-time patch file under `packages/system/keycloak-operator/images/`, adds no package under `packages/apps/` or `packages/extra/`, and changes no values key, schema, enum, default, CRD, `ApplicationDefinition`, namespace, label, annotation, metric, release asset, variant or node prerequisite. Nothing in the map fires. - [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: ### 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(keycloak): a `KeycloakRealmUser` that omits `spec.roles` or leaves `roles` out of a `spec.clientRoles` entry no longer has the matching Keycloak role mappings deleted under the default `full` reconciliation strategy. Users keep `default-roles-<realm>` and the `offline_access`, `uma_authorization` and `account` roles it grants. Setting an explicit empty list still removes every role. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Keycloak role management for realm users and client roles. * Leaving a role list unspecified now preserves existing roles. * Explicitly providing an empty role list now removes all assigned roles. * **Documentation** * Updated API and resource documentation to clarify role-list behavior. * **Tests** * Added coverage for omitted, empty, managed, and unmanaged role configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
3d4e8c07e9 |
test(tenant): anchor the operator-gate absence and refresh a spent reason (#3661)
## What this PR does Two tests in `packages/apps/tenant/tests/vmo_webhook_dependson_test.yaml` say the seaweedfs and gateway HelmReleases deliberately carry no operator gate, and assert that with `notExists: spec.dependsOn` and nothing else. A bare absence assert also passes when the template renders no document at all, so both stayed green with those two releases switched off entirely. With `seaweedfs: false, gateway: false` the suite still reported 5 of 5 passing; pinning `metadata.name` next to the absence turns the same run into 2 failures. An earlier check on these asserts injected a `dependsOn` and watched them redden. That proves they catch an added gate, which is the other direction. For a negative assert the direction that matters is the document going away, and that one was never covered. Pairing `equal: metadata.name` with the absence is not a new idiom introduced here. `no_upgrade_force_test.yaml`, in the same directory, already anchors all seven of its releases that way, and this brings the last two suites in the chart into line with it. Both suite headers also carried a reason for their `templates:` list that has expired: that rendering the whole chart fails because `keycloakgroups.yaml` indexes `.Values._cluster` unguarded. #3636 added the guard, so a no-values render of the chart now succeeds. The list is still load-bearing, for the reason `no_upgrade_force_test.yaml` already records — it keeps an unrelated template breaking out of this suite's result. Checked by adding a nil dereference to a template neither suite asserts on: both suites stay green with their lists and error without them. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches two files, both under `packages/apps/tenant/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [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: ### Release note ```release-note NONE ``` |
||
|
Aleksei Sviridkin
|
340483fdf0 |
fix(hack): declare the shared package targets and the root test target phony (#3353)
## What this PR does `hack/package.mk` set `.PHONY` with `=` instead of `:`: ```make .PHONY=help show diff apply delete update image ``` That assigns a variable which happens to be named `.PHONY`; it declares nothing. `make -p` prints it back among the variables (`.PHONY = help show diff apply delete update image`) rather than as a special target, so none of the targets `package.mk` defines was phony in any package that includes it. Nothing in the tree collides with those names today, so this is preventive rather than a fix for present breakage. The failure it forecloses is quiet: with a directory named `image` beside a package Makefile, `make image` prints `make: 'image' is up to date.` and exits 0 without building. A file of that name is enough on its own where the rule has no prerequisites; where it has one, the recipe keeps running until that prerequisite is shadowed too. The list changed as well. `suspend`, `resume`, `check` and `clean` are defined in this file and were missing, and `check` is the one that matters most, since it is the prerequisite that keeps `show`, `diff`, `apply` and `delete` rebuilding. `update` and `image` are gone, because `package.mk` does not define them; the including package Makefile does. Naming an undefined target in `.PHONY` creates it as an empty target, turning `No rule to make target` (exit 2) into `Nothing to be done` (exit 0) wherever the rule is missing. That reaches CI too: `hack/build-matrix.sh` parses the root Makefile `build:` target and runs `make -C <pkg> image` per unit, so a package arriving in that list without an `image:` rule would go from a red job to a green one that builds nothing. `.DEFAULT_GOAL` on the line above keeps its `=`. That one is a real variable, so the assignment form is right there. A comment now says which of the two lines is which, since they read alike and one of them just changed shape. `hack/package-mk-phony.bats` covers the declaration and is picked up automatically by the `hack/*.bats` glob in `bats-unit-tests`. Five of its six tests cover `package.mk`: that the declared names are really phony with a file of each name sitting beside the Makefile; a control that mutates a copy back to the assignment form and requires the probe to report up-to-date, so the first cannot pass vacuously; that the declared list and the defined targets are the same set, checked both directions; and two pinning the `update`/`image` exclusion, one requiring the loud failure to survive and one extending the list in a copy and requiring it to disappear. The loud-failure test pins both the exit status 2 and the diagnostic. A wrapper that kept make's wording while returning something else would pass a check on the message alone. One point decides where the guarantee actually lives. The first test reads its names from the `.PHONY:` line, so it cannot see a name dropped from that line at all. The set-equality test is what guards every name's presence. One known gap is left in place. The first test decides by matching make's `is up to date`, which make prints only for a target that has a recipe, and a recipe-less alias never draws it: with no prerequisite both forms print `Nothing to be done`, and with a prerequisite that is itself out of date, which every target in this file is for being phony, the working form prints that prerequisite's recipe while the broken form prints `Nothing to be done`. The probe separates neither pair, so such a name would pass the first test either way. Measured on both shapes, not assumed. Every target `package.mk` defines has a recipe, so nothing is uncovered today; the control test does not close the gap either, since an alias is a defined target and the control requires `is up to date` from it, which turns it red against the probe rather than against the alias. Worth closing if an alias is ever added. One more for whoever edits the suite next: `hack/cozytest.sh` rewrites every bare `}` at column 0 into `return 0` followed by `}`. That is aimed at `@test` blocks, but it does not distinguish them from a plain shell function, so a helper factored out of these tests would silently become unable to return a non-zero status. Tests 1 and 2 repeat a loop rather than share one for reasons of their own, but this is the trap waiting for anyone who merges them. Two properties of the suite are worth stating, because both are easy to get wrong in the other direction. The set-equality test extracts the defined targets with a pattern that accepts a rule naming several targets at once, so `suspend resume: check` keeps both names in the set; anchoring the colon to a single name drops every name on such a line, and for a target nothing else depends on that goes unnoticed. And the control test probes the targets `package.mk` defines rather than the names on its `.PHONY:` line, because `is up to date` is only a meaningful answer for a name that has a rule: a declared-but-undefined name prints `Nothing to be done` in both the working and the broken form, so probing it in the control fails against the probe, while the set-equality test holds the message that names the defect. Last one, about how the tests find the repo. All six read it as `repo=$(pwd)`, which assumes the runner starts at the repo root. That holds for `make bats-unit-tests` and for the invocation the header documents, so nothing is broken, and the assumption is the ordinary shape in `hack/` rather than an outlier: about half the bats files there resolve repo-relative paths against the cwd the same way, several stating it in their own headers, and this is not even the only one spelling it with `$(pwd)`. What is worth naming is the failure mode: started elsewhere, these tests go red with `no '.PHONY:' declaration found in hack/package.mk`, which reads like a real finding rather than a wrong directory. The root `Makefile` had the same defect, and there it had already fired. `test` was not on its `.PHONY:` line and a `test/` directory sits beside it, so `make test` printed `'test' is up to date` and exited 0 without running anything, while `docs/agents/overview.md` advertises that command as the way to run the full suite. CI never used it: the workflow calls `test-controllers` and drives e2e out of `packages/core/testing`, so the cost fell on anyone following the docs by hand. One name on the declaration closes it, and the suite gained a sixth test that goes red without it. Every other undeclared target in that file is unshadowed today, so none of them is reachable the way `test` was, and they are left alone here. #3709 covers the same class in the four `packages/core/` Makefiles, which include only `hack/common-envs.mk` and so are out of reach of this change entirely. ### Downstream repositories `scripts/package.mk` in cozystack/external-apps-example was a byte-for-byte copy of `hack/package.mk` (identical sha256 before this change), carrying the same inert `.PHONY=` line. This PR splits the two files, and the copy was fixed separately. It landed the same nine names, and `update` and `image` are absent there for the reason they are absent here: `scripts/package.mk` does not define them either, and neither does any Makefile that includes it. What separates the two files now is the two-line comment added here, and nothing else. - [ ] 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: - [x] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: https://github.com/cozystack/external-apps-example/pull/4 - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed task declaration handling so supported commands are correctly recognized and executed. * Updated available commands by replacing `update` and `image` with `suspend`, `resume`, `check`, and `clean`. * **Tests** * Added coverage to verify command declarations, prevent regressions, and ensure invalid or missing commands report errors correctly. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
d978a371a2 |
test(hack): cover the .PHONY declarations this tree relies on
`.PHONY=a b c` and `.PHONY: a b c` differ in the `=` against the `:`, and only the second declares anything. The first defines an ordinary variable named `.PHONY` that nothing reads, leaving every listed target file-backed. A file or directory of that name beside a package Makefile then stands in as the product of the rule: for a target with no prerequisites make reports it up to date, skips the recipe and exits 0, and a target whose prerequisite is itself file-less keeps rebuilding until that prerequisite is shadowed too. The neighbouring `.DEFAULT_GOAL=help` is a real variable and is correct with `=`, so the two lines read alike while only one of them works, which is what let the inert form sit there unnoticed. Add hack/package-mk-phony.bats, six tests, picked up automatically by the hack/*.bats glob in `bats-unit-tests`. Three cover the declaration itself. The first reads the names off the `.PHONY:` line rather than repeating them, plants a file named after each beside a stub Makefile, and requires `make --dry-run` to print the recipe instead of "is up to date". A negative control mutates a copy back to the assignment form and requires the probe to report up-to-date, over the targets package.mk defines rather than the names it declares, because "is up to date" is only a meaningful answer for a name that has a rule. The third keeps those two sets equal: it requires the declared list and the defined targets to be the same set. Defined but undeclared is the direction the list rots in: check, clean, suspend and resume sat undeclared for as long as the declaration itself was inert. Declared but undefined is the empty-target trap in its general form, so a future name cannot slip in the way update and image once did. Two more pin what the list deliberately leaves out. package.mk defines neither `update` nor `image`; the including package Makefile does, and most packages define neither. Naming them here would create them as empty targets everywhere, turning "No rule to make target" into "Nothing to be done". Since the CI build matrix is parsed from the root Makefile `build:` target and runs `make -C <pkg> image` per unit, a package reaching that list without an `image:` rule would go from a red job to a green one that builds no image. One test requires that loud failure to survive, pinning both of the observables it is made of: make's exit status 2 and its diagnostic. They are independent, and a wrapper that kept the wording while returning something else would satisfy a check on the message alone. Its control extends the declaration with the two names in a copy and requires the failure to disappear, so the pin tracks the list rather than some property of the harness. Every decoy carries one fixed timestamp. Six of the nine targets take check as a prerequisite, and apply and delete reach it through suspend as well, so touching the decoys in list order can leave check newer than its own dependents. Make then rebuilds them for that reason and prints a recipe whatever .PHONY says, and the probe reports success against a declaration that does nothing. Equal mtimes remove the question in any list order. Measured on GNU Make 4.3: list order left the per-target assertion inert for up to six of the nine, moving with where the clock tick fell, while 3.81 fits the loop inside a single timestamp and shows none of it. The control test plants its decoys the same way for the same reason, and goes red if that is undone in its own loop. The two loops are independent, so neither guards the other, and the header is what keeps the arrangement from being unpicked. A sixth test leaves package.mk and covers the root Makefile, where this class had already bitten: `test` is the one target in the tree with a path of its own name beside it. It probes with --dry-run because the recipe wants a live cluster, and fails loudly if that path ever goes away rather than passing on a question with no subject. `%-update` is not coverable: .PHONY matches target names literally and does not expand patterns. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
c9ba7a3cca |
fix(platform): give node DaemonSets memory requests and limits
The Talos userspace OOM handler (v1.12+) ranks cgroups with
memory_max.hasValue() ? 0.0 :
{Besteffort: 1.0, Burstable: 0.5, ...}[class] * memory_current
and discards every cgroup scoring zero, so only a memory limit takes a
pod out of the victim set. Selection is decoupled from the trigger: a
tenant workload thrashing against its own limit drives node memory PSI,
and a limit-free system DaemonSet is killed for it. metallb-speaker and
linstor-satellite were the observed victims.
Set requests and limits on the node DaemonSets that had none. Requests
come from 14d peak working set measured on a 3-node dev cluster; limits
are deliberately loose, sized as a runaway backstop rather than a fitted
ceiling, because several of these scale with cluster size that a dev
cluster understates and because undersizing converts a rare
pressure-driven kill into a deterministic one. A limit costs nothing
until it is hit -- only the request reserves.
memory.max lands on the POD cgroup only when EVERY container in the pod
carries a limit, so sidecars and init containers are covered too; one
limit-free container returns the whole pod to the victim set.
Two DaemonSets are not chart-rendered and needed their own seam:
linstor-satellite via the LinstorSatelliteConfiguration podTemplate, and
virt-handler via KubeVirt customizeComponents.patches, since virt-operator
owns that DaemonSet. Note linstor-controller stays deliberately limit-free
-- it is a JVM that must not be OOM-killed mid-operation, and cluster_test
pins that.
Two things the multus manifest needed beyond the values change.
customize-deployment.patch is the source for that template, so the
limits are carried there rather than only in the generated file, which
`make update` would discard. And the reason kube-multus had no memory
limit was that this patch DELETED upstream's 50Mi rather than raising
it -- 50Mi being well under the 230Mi measured peak -- which is what
removed memory.max from the pod cgroup in the first place.
Not covered, because upstream hardcodes the containers with no resources
knob: the four frr-k8s cp-* init containers, the kube-ovn hostpath-init
and install-cni init containers, and cozy-proxy, whose vendored chart
exposes no resources value at all. hami and kilo are left alone as
optional components with no usage measurements behind them. All of these
still get a limit from the default LimitRange the operator maintains in
every system namespace.
Every limit is pinned by a helm-unittest assertion, and every assertion
was mutation-tested: changing the rendered value turns its suite red.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov Daniil
|
cf87201099 |
test(monitoring-agents): run this package's helm suite
hack/helm-unit-tests.sh discovers work by probing each package for a `test` Makefile target, so a package without one is skipped in silence however many suites sit under tests/. monitoring-agents had tests/fluent-bit_probes_test.yaml and no target, so that suite had never run once since it was written. It passes; the only thing missing was the two lines that reach it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
ca7d2b0182 |
docs(platform): explain system component memory limits
Add docs/operations/system-memory-limits.md, recording why system components carry memory limits. That reasoning is not recoverable from the values themselves, and is exactly the kind of thing a well-meaning later change removes as redundant. The Talos userspace OOM handler (v1.12+, enabled by default) selects its victim by ranking cgroups that lack memory.max. Three consequences are each easy to get wrong on their own: only a limit grants immunity, since a request merely moves BestEffort to Burstable and under strictCgroupClassOrdering that means "killed second"; every container in the pod needs one, init containers included, because kubelet sets pod-level memory.max only when all of them have it; and PriorityClass, system-node-critical included, has no bearing on it whatsoever, because the handler walks cgroupfs and holds no Kubernetes client. That last one is the trap, since setting a priority looks like a fix. The page also covers the two installer knobs and the constraint that makes them dangerous to lower (a defaulted limit below a container's own request fails admission), how to confirm a pod cgroup really carries memory.max rather than trusting the QoS class, what is deliberately left uncovered, and how to relax the global-PSI backstop clause of the trigger on a cluster that trips it persistently. Every Talos claim is checked against the siderolabs/talos v1.13.6 tag, which is the version packages/core/talos currently ships, and every Cozystack claim against the tree. Ordering: this documents behaviour introduced by the system-namespace LimitRange and the node-DaemonSet resource changes, so it should not merge ahead of them. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
IvanHunters
|
09dc7f47fb |
feat(backups): add a native MongoDB (psmdb) backup-strategy driver (#3562)
## What this PR does Adds a native MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so `apps.cozystack.io/MongoDB` reaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore into a differently-named instance, and point-in-time recovery — all through the unified `BackupJob` / `RestoreJob` / `BackupClass` / `Plan` interface (closes #3545). The driver orchestrates the operator's `PerconaServerMongoDBBackup` / `PerconaServerMongoDBRestore` CRDs: - **BackupJob**: creates a one-shot `PerconaServerMongoDBBackup`, polls `status.state`, and persists the S3 destination + storage descriptor (by reference — Secret name only, never a raw credential) onto the Cozystack `Backup` artifact. - **RestoreJob**: drives `PerconaServerMongoDBRestore` via `backupSource` (read back from the source backup status, with a snapshot fallback for a reaped operator CR) so the same artifact restores in-place **or** into a differently-named instance. PITR is exposed as `spec.options.recoveryTime` (RFC3339), matching the CNPG driver's option exactly; unrecognised `spec.options` keys are surfaced as a `UnknownRestoreOption` Warning event rather than silently dropped. Because psmdb resolves the backup storage by name against the cluster's `spec.backup.storages`, the driver names the chart's storage (`s3-storage`) via the `cozy-default-mongodb` Strategy rather than injecting it, and surfaces a clear `Ready=False` precondition when a target has backups disabled instead of hanging until the deadline. **Wiring** - `api/backups/strategy/v1alpha1`: `MongoDB` strategy type + generated CRD/deepcopy. - `internal/backupcontroller`: `psmdbstrategy_controller` + `psmdbtypes` / `mongodbapp` partial-schema mirrors; BackupJob/RestoreJob dispatch, supported-kinds, cleanup-on-delete branch, and scheme registration. - `backupstrategy-controller`: `cozy-default` MongoDB route, `cozy-default-mongodb` Strategy CR, and `psmdb.percona.com` RBAC (get/list/watch, +create on backups/restores, no delete — the driver does not own the S3 archive). - `apps/mongodb`: a `backup.insecureSkipTLSVerify` value (psmdb's s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed in-cluster seaweedfs. Default `false`, fully backward-compatible; regenerated schema/README/RD. **Tests + e2e** - Unit tests for the driver: applicationRef validation, ensure-idempotency (backup + restore), the backups-enabled/storage precondition gate, `backupSource` resolution (live-preferred / snapshot fallback / no-destination), snapshot round-trip, the producer-seam (`createMongoDBBackupArtifact`) driver-metadata, the cleanup-on-delete no-Velero-fall-through guard, and `recoveryTime` → psmdb-UTC mapping + unknown-option reporting. - `examples/backups/mongodb/` + a `mongodb-2-backup-roundtrip` Chainsaw suite that drives `run-all.sh` end-to-end (a sentinel document round-trips through S3 into a restored copy while the source stays untouched), per `docs/agents/e2e-testing.md`. - `docs/operations/backup-classes.md` updated (new route, the app-owns-the-storage model, and the `recoveryTime` PITR usage). Verified end-to-end on a live dev cluster against the platform-default `cozy-backups` bucket: backup via `cozy-default` reached the bucket and the Cozystack `Backup` went `Ready`, and a to-copy `RestoreJob` restored the sentinel into a differently-named instance with the source left intact. ### Screenshots N/A — no UI changes (controller, chart values, CRD, docs and tests only). ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` file by file against the diff: - **cozystack/website** — the only app-facing change is `packages/apps/mongodb` (a new `backup.insecureSkipTLSVerify` value + regenerated `README.md`). The managed-app reference page is regenerated from the package `README.md` by the release bot for apps already in the website `Makefile` (mongodb is one), so it refreshes automatically on the next tag — no manual follow-up. `docs/operations/backup-classes.md` is not a website-mirrored doc in the trigger map. - **cozystack/terraform-provider-cozystack** — the one candidate trigger ("a field added to an app's `values.schema.json`") lands inside the mongodb `backup` block, which the provider's `internal/provider/mongodb_schema.go` explicitly states it does **not** manage ("The deprecated backup block, the bootstrap restore block, and detailed shardingConfig are not managed"). No enum, default, `release.prefix`, Secret/Service name, or hand-typed CRD (`Plan`, `RestoreJob`) changed; the new `strategy.backups.cozystack.io/MongoDB` CRD is opaque to the provider. Not affected. - All other repositories — no matching trigger (no `hack/` layout or make-target change, no namespace rename, no `ApplicationDefinition` semantics change, no `packages/core/platform` values, no telemetry metric or `cozy-proxy` label rename). - [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: ### Release note ```release-note feat(backups): add a native MongoDB (psmdb) backup-strategy driver — logical backup to the cozy-default BackupClass, restore into a differently-named instance, and point-in-time recovery via spec.options.recoveryTime ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added MongoDB backup and restore support, including scheduled and on-demand backups, restores to new or existing clusters, and point-in-time recovery. * Added configurable S3 TLS certificate verification for MongoDB backups, defaulting to secure verification. * Added MongoDB support to the default backup configuration. * **Documentation** * Added MongoDB backup and restore examples, workflows, and operational guidance. * **Tests** * Added automated coverage for MongoDB backup, restore, cleanup, and end-to-end scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
61a236cd96 |
feat(ci): auto-apply triage labels to issues (#3574)
## What this PR does Issues that arrive through the API or `gh issue create` bypass the issue templates, so they carry no `triage/*` label and nothing in the repository adds one afterwards. Listing every open issue through the same endpoint the workflow uses, `gh api --paginate "/repos/cozystack/cozystack/issues?state=open&per_page=100"`, returns 567 entries, 171 of them pull requests, at the time of writing. That leaves 396 open issues, and 252 of them carry no triage label at all. Every count in this description is a point-in-time reading of a backlog that keeps moving, so a later run of the same query will differ by a few. The triage queue is impossible to filter on in that state, and there is no way to see how much of the backlog nobody has looked at. This adds a workflow that labels an issue when it is opened or reopened, plus a daily sweep that backstops labels removed by hand and works through the existing backlog. An issue somebody has assigned, or marked with one of `priority/critical-urgent`, `priority/important-soon`, `priority/important-longterm` or `epic`, gets `triage/accepted`, since a maintainer has already looked at it. Everything else gets `triage/needs-triage`. An issue already carrying any `triage/*` label is left alone, so the sweep never overwrites a decision someone made by hand, and pull requests returned by the same listing endpoint are skipped. Against the listing above, the sweep applies `triage/accepted` to 39 issues and `triage/needs-triage` to 213, and touches none of the 144 that are already triaged. Deleting a triage label by hand does not undo any of this. An issue left carrying no `triage/*` label is relabeled by the next sweep, so a triage decision is changed by replacing the label. `AGENTS.md`'s labeling section now says so, because that is the file somebody reads before labeling by hand. Two conditions decide what counts as a signal. Membership of `stale.yaml`'s `exempt-issue-labels` is necessary: `triage/accepted` is itself on that list, so applying it does more than reset a clock, the issue stops being auto-closable, and a signal that is not already exempt would hand out that permanent reprieve as a side effect of being labeled. `priority/backlog`, the one tier `stale.yaml` deliberately leaves reapable, is excluded for exactly that reason. Membership is not sufficient, which is why the signal list is not simply the exempt list. `lifecycle/frozen` is exempt and still not a signal: a `lifecycle/*` label states what the stale bot may do with an issue and says nothing about whether anybody reviewed it. Freezing an untriaged issue to keep the bot off it is the case where reading it as accepted is wrong. Of the five open issues whose only signal would have been `lifecycle/frozen`, one is blocked on an upstream project and carries `help wanted`, and one is an older feature request that an open epic now covers. Neither is ready to be worked on, and stamping either accepted would have kept it out of triage for good, since the sweep never revisits an issue that already carries a triage label. `epic` passes the same test from the other side: it is a maintainer-authored planning artifact rather than an inbound report, and all eight open issues whose only signal is `epic` are roadmap trackers that `needs-triage` would describe falsely. A contract test pins the rule and not the instance, so no label from the `lifecycle/*` namespace can quietly become a signal later. `security/*` stays out on the same grounds, though the case is closer: those labels record what is known about a vulnerability rather than whether anyone triaged the report, and the cost of being wrong runs the cheap way, since a `needs-triage` on a confirmed report is one click to remove while a wrong `accepted` is permanent because the sweep never revisits. One caveat stated plainly: the signals stand for a maintainer decision already recorded on the issue, which is a weaker claim than the one `triage/accepted` makes in `.github/labels.yml`, and `priority/important-longterm` says outright that the work may not be staffed. Of the two labels this workflow can write it is still the closer one, since `needs-triage` on an issue somebody already prioritised or took asks for a triage that has happened, and a maintainer who disagrees replaces the label in one click. An assignee is the single signal that is neither a label nor already exempt, so an issue whose only signal is an assignee does gain a permanent reprieve. That is the intended reading rather than a side effect: somebody took ownership, which makes it a known long-tail task rather than an abandoned one, and it is the same argument `stale.yaml`'s own comment makes for exempting `triage/accepted` in the first place. Three operational details. First, the sweep paces its writes a second apart and caps a run at 400, because GitHub allows 80 content-generating requests a minute and 500 an hour, this client carries no throttling plugin and no retries, and a throttled write answers `403`, which is exempt from retry anyway. Unpaced, the first pass over the backlog would not slow down when throttled, it would fail the job partway and leave the backlog half labeled. Whatever a run leaves behind stays unlabeled, so the next one picks it up. Second, adding a label bumps an issue's `updated_at`, which `stale.yaml` reads as activity, so the first sweep restarts the 60-day staleness clock on every issue it writes to, all 252 of them, and un-marks the 32 that carry `lifecycle/stale` today. Both are deliberate and one-time: the backlog gets triaged now and ages again from there, rather than being closed unread while it is still unlabeled. Third, a run writes a job summary with the counts, the issue numbers under each label, and both remainders it can leave behind: the write cap's deferral, and how much of the listing it never reached when a refusal stopped it early. A sweep over the backlog logs a line per issue, and the summary is what makes the outcome readable without paging through the 252 the current backlog produces. A write that fails does not take the rest of the run with it, but the two failure shapes are told apart rather than counted together. Any status other than a `403` or `429` lets the run carry on, most often a `404` or `410` from an issue transferred or deleted mid-sweep, since a failure about one issue says nothing about the next. A `403` or `429` means the token lost `issues: write` or the pacing stopped being enough, which applies to every write left in the run, so the sweep stops at the first one and the job fails loudly. Repeating a rejected request up to the cap is what gets an integration throttled harder. `workflow_dispatch` takes a `dry_run` input that logs what would be labeled without writing anything. It is on by default, so a manual run writes nothing until somebody unchecks the box, and `schedule` is the one event that writes without being asked. The cap applies in dry-run too, so a dry run predicts what a real one would do instead of printing a longer list nobody gets. The job asks for `issues: write` and everything else is read-only, the action it uses is pinned by digest, and a 20-minute timeout keeps a hung call from holding the queue for the default six hours. No pull request lane can exercise this workflow, so its first real execution is a sweep over every open issue with nothing having run it before. `hack/issue-triage-contract.bats` pins the executable lines that bound that blast radius: the pull-request skip, the already-triaged skip, the pacing rate and the write cap, the job timeout, the failure handling, the digest-pinned action, the token scopes, the serialised queue, the dry-run default and the derivation that does not depend on it, the cron ordering against `stale.yaml`, that both labels it writes exist in `.github/labels.yml`, that no accepted-signal label sits outside `stale.yaml`'s exempt list or inside the `lifecycle/*` namespace, and that the summary is written with the `await` that makes it flush and accounts for every outcome a run can end on. Each assertion was checked against a mutated copy of the workflow to confirm it goes red rather than merely existing, with the mutations run through `hack/cozytest.sh` rather than `bats`. The two runners disagree about negated assertions, so a pin that `bats` reports as live can be dead under the runner CI actually uses, and every negative assertion in the file captures grep's status instead of leading with `!`. Suggested first step after merge: run the workflow by hand and read the summary before the 05:53 cron arms itself. The box arrives checked, so that run costs nothing. ### Screenshots Not a UI change, so there is nothing to show. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff. The diff adds two files, `.github/workflows/issue-triage.yaml` and `hack/issue-triage-contract.bats`, and appends one line to `AGENTS.md`. No row matches: it adds and renames no package, changes no chart values, no schema, no version enum, no `ApplicationDefinition`, no CRD, no namespace, no variant or bundle, no label or annotation any downstream repository matches on, no metric and no release asset. Its `hack/` rows name `hack/upload-assets.sh`, `hack/e2e-prepare-cluster.bats`, `hack/package.mk`, `hack/update-crd.sh` and `hack/common-envs.mk`, which downstream repositories copy, anchor on or mirror by hand, plus one row for moving or renaming anything under `hack/`; a new bats file added next to them is none of those. The map has no row for the agent documentation, and no downstream repository copies `AGENTS.md`. - [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: ### Release note ```release-note feat(ci): new and reopened issues now get a `triage/*` label automatically, and a daily sweep labels any open issue that has none: `triage/accepted` when it is assigned or carries an `epic` or a priority above `priority/backlog`, `triage/needs-triage` otherwise. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added automated issue triage triggered by new or reopened issues, scheduled reviews, or manual runs. * Issues can be labeled as needing triage or accepted, with pull requests and already-triaged items excluded. * Added dry-run support and reporting for triage results. * **Documentation** * Added guidance for replacing existing triage labels when making decisions. * **Tests** * Added coverage for workflow behavior, permissions, scheduling, failure handling, and safety limits. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
59878850f9 |
test(e2e): capture worker CPU throttling counters on node-join failure (#3723)
<!-- 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 `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
When a tenant worker misses the node-Ready deadline, the failure bundle
currently cannot tell two different causes apart: a worker held at its
own CFS ceiling, and a worker losing host CPU it was entitled to. Both
look identical from outside the Pod, and node-level utilisation answers
neither, because it shows what the node used rather than what the
container was allowed.
This adds a collector that reads the CFS counters and the ceiling itself
from the kubelet's cAdvisor endpoint, for every tenant worker on every
node in the sandbox. The counters are the only place that difference is
recorded, and nothing else in the tree collects them for these workers.
The motivating measurement is on nightly run 31290999592: the `compute`
container reports 0.968 and 0.920 core against a `limits.cpu` of 1,
which is 92-97% of a one-core ceiling. Whether the container ever
actually met that ceiling is exactly what the artifact cannot say, and
what these counters answer.
Most of the change is the failure taxonomy rather than the read. An
empty capture is the dangerous outcome here, because zero bytes reads as
a container that never hit its ceiling -- the conclusion the collector
exists to stop a reader reaching by default. So every way the capture
can come up short gets its own sentence: the kubelet was not read, the
read was cut short part way, the metric stream could not be read back on
this runner, the kubelet answered and carried no series for this
namespace, the namespace was there and the worker Pods were not. The one
non-failure outcome is written down too: a container with no CPU limit
puts a single line on the wire, which is also what a truncated read
leaves behind, so it says which one it is.
### Ordering
The collector runs first among the gated collectors, ahead of the guest
serial console. Its ceiling is four bounded reads, about 100s of a 480s
phase budget, so on a slow run the console capture starts that much
later. That is a deliberate trade and not a side effect of putting the
cheapest first: cost is not what settles the order, survivability of the
answer is. The console evidence cannot be re-taken, and this collector's
question has no other answer in the tree, so both go ahead of the
captures whose state is partly recoverable from reads above them.
### Known limits, named rather than left to be found
- The outcome ladder enumerates combinations of read status, filter
status and result emptiness. Every new source of partial output needs a
new rung, and that has already happened twice while this branch was in
review. Deriving the label from the three axes instead would make a new
source land in an existing axis.
- The uncapped note is decided over the whole per-node file, so one
quota line anywhere in it suppresses the note. That is the conservative
direction -- it never claims uncapped over a file that shows a ceiling
-- but it makes the note rare.
- The messages say `see read-error.log` while the file on disk is
`<node>.read-error.log`. That is the convention the neighbouring
serial-console collector already uses, and changing one side alone would
make the two inconsistent.
- The `mkdir -p` for the report directory is unchecked, as it is in four
neighbouring collectors in the same file. Worth fixing across the file
rather than in one collector.
- The section letters in the failure block no longer track emission
order. They already did not before this change; this adds one more entry
to the sequence.
- The count of bats files sourcing the library, in
`docs/agents/e2e-testing.md`, is corrected here from three to seven.
Nothing pins it, so it will drift again.
- The node walk iterates `for node in ${nodes}` unquoted, so pathname
expansion applies alongside word splitting. Node names are RFC1123 so it
cannot bite today; `while IFS= read -r node` would make that structural
instead of incidental.
### 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:
### 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
test(e2e): capture tenant worker CPU throttling counters and their CFS ceiling when a node-join fails, so a worker held at its own limit can be told apart from one losing host CPU
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added worker CPU-throttling diagnostics to Kubernetes troubleshooting
reports.
* Reports now identify CPU quota gaps, partial reads, filtering issues,
and incomplete results.
* Added safeguards for bounded node traversal and configurable
collection budgets.
* **Bug Fixes**
* Improved failure handling when diagnostic commands, timeouts, or
metric reads are unavailable.
* **Tests**
* Expanded coverage for CPU-throttling capture, node-join diagnostics,
timeout behavior, configuration validation, and partial failures.
* **Documentation**
* Updated diagnostic collector counts and collection-order
documentation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
3ad4d452fc |
fix(tests): fail when a chart runs no Helm unit test suites (#3632)
<!-- 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 `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `helm unittest` is fail-closed on a suite it cannot parse and on one declaring `tests: []`, and fail-open on suites that are not there at all: it prints `Test Suites: 0 passed, 0 total` and exits 0. `hack/helm-unit-tests.sh` judges each package by that exit code alone, so a chart whose suites were deleted, moved, or renamed past the `tests/*_test.yaml` glob reports success having asserted nothing. That is measurable in both directions rather than argued. Move `packages/system/velero/tests/velero_test.yaml` aside and run the script as it exists on main today: ```text Running tests in packages/system/velero helm unittest . ### Chart [ cozy-velero ] . Charts: 1 passed, 1 total Test Suites: 0 passed, 0 total Tests: 0 passed, 0 total ``` The run ends with `All Helm unit tests passed.` and exit 0. The chart passed, and the count of things it checked was zero. With this PR the same tree exits 1 and names the package and the reason. Neither `--strict` nor an explicit non-matching `-f` glob changes this on its own; all three forms exit 0, checked against plugin 1.1.1. So the script now reads the captured output and fails the package when the zero-suite line appears. Matching that line names a cause and a remedy where it applies, but the set of ways to run nothing is not closed. A recipe that never invokes `helm unittest`, and a `test:` rule that make finds nothing to do for, print no marker of any kind and exit 0, so no additional match reaches them; enumerating absences leaves a fresh hole every time a new shape turns up. So the script also requires the line a real run always emits, a `Test Suites:` count of at least one. That turns the question round: every way of asserting nothing exits 0, and "what did this run assert" is the question that has an answer. All 76 packages the script visits emit that line today. The tradeoff is that a `test` target which legitimately runs no `helm unittest` would need renaming or an opt-out, which is the same bargain the zero-suite check already strikes. The discovery gate has the same shape from the other side. `make -C dir -n test` succeeds against a file-backed target, so a path named `test` beside a package Makefile whose rule is not phony would make both the gate and the run exit 0 without the recipe ever firing. The check keys on what make reports rather than on the path existing, because a package that declares the target phony runs its recipe whatever sits next to it, and refusing that would be a false failure. The quoting around the target name differs between make 3.x and 4.x, so it accepts either opening quote; anchoring on the trailing quote alone would also match a sub-make reporting some other target whose name ends in `test`. That second check is guarding a live gap rather than a hypothetical one. `hack/package.mk` line 2 reads `.PHONY=help show diff apply delete update image`, which is a variable assignment and not a target declaration, so it makes nothing phony. Of the packages the script visits that define a `test` rule, 18 declare it phony in their own Makefile; the rest are unprotected if a path of that name ever appears. No such path exists today. Fixing the shared include is #3353 and is not this PR. One deliberate non-change, stated so it is not rediscovered as an oversight. Running the suite now captures output instead of streaming it, because a pipeline's exit status in POSIX `sh` reports the last command rather than `make`. Streaming reads better and loses the status, so it stays buffered. The run is pinned to `LC_ALL=C`, since both checks match English wording and a localized `make` would disarm one of them silently, which is the failure mode the whole change exists to remove. The guard is all-or-nothing, which is worth stating plainly. A chart that loses four of its five suites still reports `Test Suites: 1 passed` and passes; only total disappearance is caught. Tying the expected count to what each chart actually has would be a per-chart number to maintain, and a number maintained in one place while the suites move in another is the failure this repo has been paying for elsewhere, so the cheap check that catches the total loss is the one worth having. One behaviour worth stating because the message does not: a suite in which every test carries `skip:` reports `Test Suites: 0 passed, 1 skipped, 1 total` and exits 0, and the positive-evidence check refuses it. That refusal is intended, since a wholly skipped suite asserted nothing, but the message talks about suites expected under `tests/`, which in that case are present and skipped on purpose. No package is in that state today. Partial skips are unaffected: `2 passed, 1 skipped, 3 total` satisfies the check. Three known gaps in what the change ships, none of them a wrong statement. The positive-evidence message explains its cause but stops short of naming a remedy, where the other two messages both end in an action; the remedy for the legitimate case, renaming the target or giving it an opt-out, currently lives only in the code comment. That same message enumerates two ways a run reports nothing, and the skipped-suite case above is a third, so it reads as a diagnosis where it is really a list of the common causes. And no document in the tree states the convention this tightens: `docs/agents/overview.md` still describes the script accurately as running over every package that defines a `test` target, but the contract is now that such a package must also report at least one passing suite. All three are improvements to make on the next touch of these files rather than reasons to hold the change. One adjacent gap stays open, and it is worth naming rather than leaving someone to assume otherwise. This catches a chart whose suite *files* vanished; it does not catch a package that loses its `test:` rule along with them. Nothing ties the presence of `tests/*_test.yaml` to the presence of a `test` target, so a change that removes both is skipped in silence, and the script only complains when no package in the tree has a `test` target at all. The test named `a package with no test target is skipped, not failed` pins that permissiveness deliberately, because the many packages that legitimately define no `test` rule would otherwise turn every run red. Closing it needs an instrument keyed on the suite files rather than on the Makefile target, which is a separate change. `hack/helm-unit-tests.bats` adds eight tests, picked up automatically by the `hack/*.bats` glob in `make unit-tests`. Three pin the new refusals, and each matches its own message so none can stay green on a refusal that came from another. Two pin what must not be refused: a phony target with a colliding path, and a sub-make reporting an unrelated target up to date. One pins that a chart which does run suites still passes, which is what stops the refusals being satisfied by rejecting everything. Two pin behaviour that was already there, namely that a failing package is still reported by name and that a package with no `test` rule is skipped rather than failed. That last one builds a tree carrying a second package that does run a suite: with only the target-less package present, the script takes an early exit above the failure summary and the assertion would hold whatever the script had done. The fixtures stub `make` output instead of invoking helm, so they need no plugin installed. Relates to #3453. ### 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: ### 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(tests): `make helm-unit-tests` now fails a chart that runs no Helm unit test suites, instead of reporting success for a chart whose suite files were deleted, moved, or renamed out of the `tests/*_test.yaml` glob ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Helm test validation to detect skipped test recipes, zero discovered suites, and runs without evidence of a passing suite. * Helm test failures now provide clearer diagnostics, including affected directories and failed commands. * Test output is consistently captured and replayed for easier troubleshooting. * **Tests** * Added comprehensive coverage for successful, failed, skipped, empty, and invalid Helm test scenarios. * Expanded coverage across package test targets, including phony and unrelated sub-make behavior. * **Documentation** * Clarified requirements for package test targets and suite reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
3c009509ba |
test(cozystack-controller): guard the Secret grant by effective permission (#3627)
## What this PR does `packages/system/cozystack-controller/tests/rbac_test.yaml` has a test named "does not grant a blanket wildcard verb on secrets" that does not guard that. Its only assert is a `notContains` on `path: rules` holding a rule object. helm-unittest compares every field it is given by value equality, and `any: true` only changes which fields get compared, so nothing in that assert bounds what a verbs list may hold. `verbs: ["get", "*"]` passes every spelling of it. Each row below is a mutation of `templates/rbac.yaml`, confirmed visible in `helm template` output before I read any verdict. | # | widened Secret grant | whole suite | the test named as the guard | | --- | --- | --- | --- | | 1 | `verbs: ["*"]` on `""`/secrets, the exact object it names | red | red | | 2 | `resources: ["*"]`, `verbs: ["*"]` in the core group | red (positive pin) | green | | 3 | `apiGroups: ["*"]`, secrets, `verbs: ["*"]` | red (positive pin) | green | | 4 | `"*"` appended to the declared verb list | red (positive pin) | green | | 5 | the exact named object added as a second rule | red | red | | 6 | `verbs: ["*"]` plus a `resourceNames` field | red (positive pin) | green | | 7 | `deletecollection` spelled out | red (positive pin) | green | | 8 | declared rule kept, `apiGroups:['*'] resources:['*'] verbs:['*']` appended | green, 2 passed 2 total | green | | 9 | declared rule kept, `secrets: [deletecollection]` appended | green, 2 passed 2 total | green | Rows 8 and 9 are the bad ones. The declared rule stays untouched, so the positive pin is happy, the suite reports a clean pass, and the controller holds every verb on every Secret in every tenant namespace. The replacement is `hack/cozystack-controller-secret-rbac.bats`, which reads effective permission instead of manifest text. It renders the chart, unions the verbs every ClusterRole and Role rule confers on core Secrets, expands a `*` on any of the three axes, and compares that union against the declared set. All nine rows above are red under it, and so are widening the namespaced `Role`, bundling `secrets` with another resource under `verbs: ["*"]`, and dropping `delete` from the set. That last one is deliberate: the comparison is an equality rather than a ceiling, because both reconcilers need every verb in it and the controller is cluster-scoped, so a widening reaches every tenant. Reflowing the rule to block style changes the text and nothing else, and stays green. Green on `main` under both `bats` and `hack/cozytest.sh`. Expanding wildcards is what makes it hold at all. The chart ends with a blanket `apiGroups: ['*'] resources: ['*'] verbs: [get, list, watch]` rule that does reach Secrets, so a guard phrased as "no wildcard near secrets" would be red on `main` today. That rule predates this branch and is tracked separately in #3625. This guard stays green either way, whether the blanket rule is narrowed or left alone, as long as Secret access matches the declared set. The positive `contains` pin stays. Value equality is the right primitive for pinning a rule as written and the wrong one for bounding what the rule may become. The same shape exists in `packages/system/cozystack-api/tests/rbac_test.yaml` and `packages/extra/seaweedfs/tests/cleanup_client_test.yaml`, and more weakly in `packages/system/etcd-operator/tests/selector-fix-hook_test.yaml`, which matches a scalar inside `rules[0].verbs`. Out of scope here, listing them so they are not lost. `BATS_UNIT_FILES` picks the new file up on its own (`hack/*.bats`, no `e2e-` prefix), so `make unit-tests` runs it with no Makefile change. It needs helm, yq and jq, the same three `hack/admin-kubeconfig-invariant.bats` already needs. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. The only entry that looks at `hack/` is cozystack/ccp, which triggers on moving or renaming something there, or on changing what a make target does. This adds a file and touches neither `hack/package.mk`, `hack/common-envs.mk`, nor any target. - [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: ### Release note ```release-note NONE ``` |
||
|
Aleksei Sviridkin
|
94c41d100f |
fix(hack): fail when a chart runs no Helm unit test suites
helm-unittest is fail-closed on a suite it cannot parse and on one declaring `tests: []`, and fail-open on suites that are not there at all: it prints "Test Suites: 0 passed, 0 total" and exits 0. Measured on plugin 1.1.1 against a throwaway chart, all three forms. hack/helm-unit-tests.sh judges each package by that exit code alone, so a chart whose suites were deleted, moved, or renamed past the tests/*_test.yaml glob reports success having asserted nothing. Moving packages/system/velero/tests aside reproduces it: the script prints "All Helm unit tests passed" and exits 0. Check the captured output for the zero-suite line and fail the package when it appears. Neither `--strict` nor an explicit non-matching `-f` glob does this on its own; both exit 0. Matching that line names the cause where it applies, but the set of ways to run nothing is not closed: a recipe that never invokes helm-unittest, and a `test:` rule make finds nothing to do for, both print no marker at all, so no further match reaches them. So also require the line a real run always emits, a `Test Suites:` count of at least one. Every way of asserting nothing exits 0, and asking what the run asserted is the question that has an answer. All 77 packages the script visits emit that line today. The tradeoff is that a `test` target which legitimately runs no helm-unittest would need renaming or an opt-out, which is the bargain the zero-suite check already struck. That is a contract contributors have to know about, so docs/agents/overview.md states it where it describes the sweep. Because the check matches helm-unittest's own summary wording, a plugin upgrade that reworded it would fail every package at once while blaming each package's Makefile. The error text says so, so the reader of a tree-wide red has the real cause in front of them rather than 77 copies of the wrong one. The discovery gate has the same shape from the other side. `make -C dir -n test` succeeds against a file-backed target, so a path named `test` beside a package Makefile whose rule is not phony would make both the gate and the run exit 0 without the recipe firing. Of the packages the script visits that define a `test` rule, 18 declare it phony, and no such path exists today. Match what make reports rather than looking for the path, because a package that declares the target phony runs its recipe whatever sits next to it, and refusing that would be a false failure. The quoting around the target name differs between make 3.x and 4.x, so the match accepts either opening quote; the trailing quote alone would also match a sub-make reporting another target whose name ends in "test". Running the suite now captures output rather than streaming it, because a pipeline's exit status in POSIX sh reports the last command rather than make. The capture file is allocated once and removed by the EXIT trap that already owns the failure list, and the run is pinned to LC_ALL=C: all three checks match English wording, one on make's and two on helm-unittest's, so a localized make would disarm the first of them silently, which is the failure mode this guard exists to remove. The locale reaches every package's recipe, not only make. Extend hack/helm-unit-tests.bats with eight tests. Three pin the new refusals; two pin what they must not refuse, a phony target with a colliding path and a sub-make reporting an unrelated target up to date; one pins that a chart which does run suites still passes; two pin behaviour that was already there, namely that a failing package is still reported by name and a package with no `test` rule is skipped rather than failed. That last one carries a second package that does run a suite, because with only the target-less package present the script takes its early exit above the failure summary and the assertion holds whatever the script did. The fixtures stub make output instead of invoking helm, so they need no plugin installed. Both tests that assert a package is named in the failure summary match the summary's " - " prefix rather than the bare path. The script announces "Running tests in $dir" before it runs anything, so the bare path is present in the output whatever the summary does; matching it asserted nothing, and the summary could stop naming packages entirely with the suite still green. The failing-package fixture reports a passing suite count alongside its non-zero exit, so that the exit status is the only thing that can decide the outcome. A fixture that fails without printing the marker is refused by the positive-evidence check first, which leaves the exit-status branch unpinned. That shape occurs in the tree: a `test` target that runs helm-unittest and then a second step, a render-parity script or a vendored-version guard or go tests, emits the marker and still fails. The two tests already in that file cover a package reached under packages/tests, and their fixture printed a marker and nothing else. That is the shape the positive-evidence check refuses, so the fixture now also emits the line a real run emits; without it both would fail for a reason unrelated to what they assert. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b8a5b9ba1d |
test(e2e): capture worker CPU throttling counters on node-join failure
When a tenant worker misses the node-Ready deadline, the sandbox node it runs on is routinely at half its capacity with a clean kernel log. In that state "the host starved the VM" and "the VM was held at its own CFS ceiling" produce identical evidence: node-level utilisation reports what the node used, never what the container was allowed. The CFS counters are the only place the difference is recorded, and nothing in the tree collected them for the tenant workers, so every diagnosis of this failure so far had to be inferred from outside. Read them from the kubelet's cAdvisor endpoint on the node hosting each worker. Five series carry the whole answer and cAdvisor publishes all of them per container, so nothing is computed here: the scheduled and throttled period counts, the time spent throttled, and the quota and period that form the ceiling. The throttled counters alone say a container hit some ceiling, not which one, and a VM capped at one core and a VM capped at eight are the same number without the quota. Absence of the quota series is itself a reading rather than a gap. cAdvisor emits it only when the quota is non-zero, so a container reporting the period and no quota is one running uncapped - which is one of the two answers this collector exists to distinguish, arriving without being derived. Reading the kubelet rather than the container matters beyond convenience. A reader running inside the container shares the cgroup it measures, so it slows down in proportion to how much the answer matters and would need a budget sized for the pathological case; the kubelet is outside that cgroup. It also drops any dependency on what the container image ships and on how the host lays cgroups out. The namespace is not a worker filter and is not used as one: it also holds the tenant control plane, whose apiserver and etcd carry CPU limits of their own and can be genuinely throttled. The Pod name carries the second half of the filter, so a throttled apiserver cannot answer under a heading that says worker. The walk is over nodes rather than Pods, because the endpoint is per node and one read covers every worker it hosts. It is gated on the diagnostics phase budget, and placed first among the five gated collectors: the budget declines whatever has not started when it runs out, its own ceiling is four bounded reads rather than the minutes the four after it can spend, and it is the only one whose question has no other answer in the tree. That order is a trade, stated here because it is a decision rather than a detail - it puts the guest serial console one place further from the budget, and that capture is the only one describing a worker which never reached apid. Both reads take the block's own read-budget knob rather than a literal equal to it, so lowering that knob lowers these too. Each keeps its own exit status rather than routing through the shared helper, which always returns zero by design: this collector has to tell "the kubelet was not read" from "the kubelet answered and carried no worker", and only the status separates them. The stream is captured whole and filtered after, since a pipeline reports its last command's status and would otherwise hand that decision to grep. Every way a read can come up short is named rather than left as zero bytes: a kubelet that was not read, one that answered with nothing for the namespace, a stream cut short, and exit 124 - which cannot distinguish this collector timing out from the read exiting 124 on its own - each get their own line, and each says whether an error log exists to explain it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c681ae1243 |
test(tenant): anchor the operator-gate absence and refresh a spent reason
The two "carries no operator gate" tests asserted only `notExists: spec.dependsOn`. A bare absence assert passes on a template that renders no document, so toggling `seaweedfs` and `gateway` off left both green while the releases they guard were gone. Measured: all 5 tests passed with the templates rendering nothing, and 2 of them fail once the name is pinned alongside the absence. An earlier check on these two asserts injected a `dependsOn` and watched them redden, which proves only that they catch an added gate. That is the other direction; the one that matters for a negative assert is the document disappearing. Both suite headers also justified their `templates:` list with a reason that has expired: keycloakgroups.yaml now guards its `.Values._cluster` index, so a no-values render of the whole chart succeeds. The list is still load-bearing, for the reason the sibling suite already records. It keeps an unrelated template out of this suite's result: with a nil dereference added outside the guard of a template neither suite asserts on, both suites stay green with their list and error without it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e9411d6b8c |
fix(apps): make the post-delete cleanup hooks report failure
The qdrant and harbor post-delete hooks guarded every delete with `|| echo "WARNING: ..." >&2`, set no `-e`, and ended on an unconditional completion message, so the container exited 0 on every path. Helm read that as a successful hook and the hook-succeeded delete policy removed the Job together with the pod that carried the warning, leaving an uninstall that reclaimed nothing indistinguishable from one that had nothing to reclaim. Count the failures instead and set the exit code at the end, so a failed delete does not skip the deletes that follow it. An object that is already gone stays a success: every delete keeps --ignore-not-found, which exits 0 in that case. This is the shape packages/apps/mariadb already uses for the same problem. Both hooks also consumed the output of `kubectl get` directly -- harbor by iterating it, qdrant by piping it into grep, whose status the pipeline then reports. A listing that failed therefore arrived as an empty list and the sweep was skipped rather than reported. Each listing is now captured before it is used, so a listing that failed and one that returned nothing are different events. A hook that exits non-zero keeps its Job and pods, which is the point: they hold the log. Give the Job one retry rather than the Kubernetes default of six or none at all. Every delete is idempotent, so a second attempt re-issues an API call rather than re-running any logic, and one short backoff is what separates a transient apiserver error from a real one -- these failures become visible for the first time here, and a signal that fires on noise stops being read, including on the day it is right. Six is the far end of the same mistake: ten minutes of backoff across seven pods outlasts the hook timeout, and Helm reports that timeout instead of the hook's own error. The qdrant Chainsaw suite explained its post-delete pod collector by quoting the guard string removed here and concluding the pod is always collected away. That is now backwards: a failed cleanup is exactly when the pod survives and the collector is the evidence. Rewrite the note, and the sentence in the harbor suite pointing at it. hack/app-cleanup-hooks_test.bats renders both scripts and runs them against a fake kubectl. There are eight failure paths across the two hooks -- six in harbor, two in qdrant -- and each has a case that goes red when that one path alone is reverted to a swallowed warning. Message assertions are anchored on the hook's own `ERROR: ` prefix, because the fake kubectl names the object in its own diagnostic too and an unanchored match is satisfied by the fixture. The helm-unittest suites pin the retry limit and the hook delete policy: adding hook-failed to that annotation hands the Job back to Helm to delete on failure and destroys the evidence this change exists to keep, and until these asserts that reversal left both suites green. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
5d436fad43 |
fix(build): declare the root test target phony
The root Makefile declares nine of its targets phony and `test` is not among them, while a `test/` directory sits beside it. Make therefore takes that directory for the target's product: `make test` prints "'test' is up to date", exits 0, and the recipe -- which installs the platform and runs the e2e suite -- never executes. docs/agents/overview.md advertises the command as the way to run the full suite, so someone following it by hand gets a green exit and no tests. CI is unaffected: the workflow calls test-controllers, and the e2e path is driven out of packages/core/testing directly. Every other undeclared target in this file is unshadowed today, so this is the only one that misbehaves, and one name closes it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
91b9f58057 |
fix(hack): declare the shared package targets phony
package.mk set .PHONY with `=` rather than `:`, which assigns a variable named .PHONY instead of declaring anything phony, so none of the targets package.mk defines was phony in any package that includes it. Declare exactly the concrete targets package.mk defines: help, show, diff, apply, delete, suspend, resume, check and clean. The earlier list named update and image, which package.mk does not define — naming an undefined target in .PHONY creates it as an empty target, so `make image` and `make update` would report success without running anything in the many packages that lack those recipes. It also omitted check and clean, the targets a same-named file or directory could actually shadow. The neighbouring .DEFAULT_GOAL is a variable and correctly keeps its equals sign. A comment now records which of the two adjacent lines is which, so the corrected one does not pull the other into its shape by analogy. The trigger map in docs/agents/contributing.md called the two files a byte-for-byte copy, which was exact rather than loose: they were the same object. They stop being one here, so the entry now records that they were identical and no longer are, and asks for a diff-and-port, the wording the map already uses for the other copy that has diverged. It deliberately does not enumerate which lines differ: that set moves as the copy catches up, and an entry that names it would be wrong again by the next port. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
5f8a820494 |
fix(keycloak): keep KeycloakRealmUser roles when spec.roles is unset
The operator reconciles user roles as an authoritative set. Under the default `full` strategy it deletes every realm role mapping missing from spec.roles, and every client role mapping missing from the matching spec.clientRoles entry. A consumer that manages only the user record and leaves those fields out therefore has all of its roles stripped on the first reconcile, `default-roles-<realm>` included, which costs the user offline_access, uma_authorization and the account client roles. Treat an omitted list as "not managed" and leave Keycloak untouched for that field, the same convention spec.identityProviders already uses. An empty but non-nil list keeps its meaning, so revoking every role is still possible with an explicit `roles: []`. Drop omitempty from the two json tags as well. Without that an explicit empty list is erased the first time the operator writes the object back, adding its own finalizer is enough, and returns indistinguishable from an omitted field, so the distinction does not survive a round trip. The patch mirrors epam/edp-keycloak-operator#372 file for file, so it can be dropped in one move once COMMIT_REF moves past the upstream merge. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
62204d4903 |
test(cozystack-controller): guard the Secret grant by effective permission
The suite's "does not grant a blanket wildcard verb on secrets" assert was a notContains over a rule object. helm-unittest compares every field it is handed by value equality, and `any: true` changes which fields are compared rather than how, so no spelling of that assert bounds what a verbs list may hold: `verbs: ["get", "*"]` satisfies all of them. Measured against the rendered ClusterRole, seven realistic spellings of a widened Secret grant left that assert green: a wildcard in resources, a wildcard in apiGroups, a "*" appended to the declared verb list, an extra field beside the verbs, deletecollection spelled out, and two where the grant is added as a separate rule. Those last two leave the whole suite green - appending apiGroups:['*'] resources:['*'] verbs:['*'] keeps the positive pin satisfied while the controller gains every verb on every Secret cluster-wide. Replace it with a bats guard over the rendered manifests. It unions the verbs that every ClusterRole and Role rule confers on core Secrets, expanding "*" on each of the three axes, and compares that union with the declared set. Equality rather than an upper bound: the controller is cluster-scoped, so a widening reaches every tenant namespace, and a narrowing breaks the two reconcilers that share the grant. The positive pin stays in tests/rbac_test.yaml. Value equality is the right primitive for pinning a rule as written and the wrong one for bounding what the rule may become. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
ea78d09dd0 |
feat(ci): auto-apply triage labels to issues
Issues created via the API or gh bypass the issue templates and arrive without any triage/* label; 252 of the 396 open issues carry none. Label issues on arrival (opened/reopened) and run a daily sweep as a backstop: issues already prioritised, assigned, or marked as epics get triage/accepted, the rest triage/needs-triage. A signal has to be on stale.yaml's exempt-issue-labels, because triage/accepted is itself exempt and a signal that is not would grant a permanent stale reprieve as a side effect of being labeled. That is necessary and not sufficient: lifecycle/frozen is exempt and still not a signal, because a lifecycle/* label says what the stale bot may do with an issue and nothing about whether anybody reviewed it. The sweep paces its writes and caps how many it makes per run, so a pass over a large backlog stays inside GitHub's secondary rate limits instead of failing partway through. Each run writes a job summary with the counts, the issue numbers per label, and both remainders it can leave behind, because a backlog pass logs a line per issue and nobody reads 252 of those to find out what happened. The manual dispatch defaults to dry-run, so the one entry point a human can reach writes nothing until somebody unchecks the box. The schedule is the one event that writes without being asked. Deleting a triage label does not undo a decision: an issue left with none is relabeled by the next sweep, so AGENTS.md's labeling section says to change a decision by replacing the label instead. No PR lane can exercise this workflow, so the first real run is a sweep over every open issue. A contract test pins the executable lines that bound that: the pull-request skip, the already-triaged skip, the pacing and the cap, the dry-run default, the pinned action, the token scopes, that no signal comes from the lifecycle/* namespace, and that the summary is written with the await that makes it flush. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
myasnikovdaniil
|
e944619e98 |
docs(release): add changelog for v1.6.1 (#3551)
This PR adds the changelog for release `v1.6.1`.
✅ Changelog has been automatically generated in
`docs/changelogs/v1.6.1.md`.
|
||
|
Aleksei Sviridkin
|
d73c102945 |
test(system): anchor index-addressed negative asserts to what they name
notContains passes whenever its path does not resolve, and also
whenever it resolves to something that simply lacks the forbidden
content. A different container at the same index qualifies, so these
guards go green the moment a sidecar is placed ahead of the workload
they were written for, while the flag or env var they forbid comes
back on the real container.
Assert the identity at that index first, in each affected case: the
container name for the Deployment-shaped ones, and for the Cilium
policy its selector, since a deny rule has no name. The selector
identifies that rule only while there is one rule to identify, so it
is paired with a length assertion, and with a second one on the
rule's toPorts, because that path carries two indexes and anchoring
the outer one alone leaves the inner free.
The anchor goes only where nothing else holds the index. A positive
contains on the same path fails on its own once a sidecar arrives
ahead of the workload, and an equal on the whole argument list fails
the same way, so neither carries a separate anchor. A negative does,
including the per-element form below: a wildcard resolving to the
sidecar's own arguments matches nothing and passes.
Several of the guards could not fail for the reason they name, and
the anchor alone would have left that hidden behind a comment saying
they were covered. contains and notContains compare elements for
equality, so a mapping is compared whole: `{name: GF_...}` never
matched an entry carrying a value beside the name, and `{port: 7473}`
would not match that port re-added with a protocol beside it, which
is how the neighbouring entry in the same list is written. Those get
`any: true`.
Equality against a rendered argument is the same failure worn
differently, and it arrives in three disguises. A prefix that never
equals an element: `--pinned-tenants=` never equalled
`--pinned-tenants=tenant-a=shard0`, and `--target-host=` never
equalled a rendered `--target-host=<value>`. One value out of many
under a name that claims the flag: `--compatibility=cilium` left the
kilo default render free to carry `--compatibility=flannel` off the
same values key, and the two `--proxy-fqdn` negatives forbade only
the FQDN the default cluster-domain produces. And a form that holds
by luck: `--gateway-api` still fails today only because that template
renders it bare, so an upstream switch to `--gateway-api=true` would
retire the guard in silence.
All of those become notMatchRegex over args[*], which matches the
flag name per element and fails on any value and on the bare form
alike. A length assertion would not have covered the prefix cases,
since an upstream bump that adds the flag while consolidating another
argument keeps the count. A wildcard that resolves to nothing still
passes, so the index anchors stay.
The whole argument list is a different instrument, answering what the
default invocation is rather than whether one flag is absent. It
fails on an argument added, removed or swapped instead of only on the
ones someone thought to enumerate, which is worth having where the
template is first-party and a failure is a deliberate change asking
to be read. flux-shard-operator gets it in `renders the default
Deployment shape`, replacing eleven individual contains that listed
exactly the same eleven elements; `--shard-cpu-limit=` was dead for
the prefix reason and is dropped rather than repaired, because that
list now covers it. The vendored ouroboros templates get no such pin:
upstream churn arrives there on its own schedule, and a whole-list
assertion would fail a test named for one flag for a reason that has
nothing to do with that flag.
Anchor every index on a path, not the outermost one. The Cilium
assertion reads spec.ingressDeny[0].toPorts[0].ports, and pinning the
rule list alone would still let the port return on a second toPorts
entry of the same rule.
Each guard was checked by rendering what it forbids onto the element
it names, and each anchor by rendering the shift it exists to catch.
The form is already used in this tree, in opensearch-operator and
dashboard. Assertions of the same shape remain in files this commit
does not touch, including one in a package it does, and stay out of
scope here.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
|
||
|
myasnikovdaniil
|
516be4abfe |
fix(cozystack-basics): normalize tenant hostname VAP comparisons with lowerAscii (#3208)
## What this PR does The Ingress, Gateway, and Route hostname `ValidatingAdmissionPolicy` objects compare tenant hostnames against the tenant apex with case-sensitive CEL. The resource-side host is always lowercase — the apiserver's hostname validation permits nothing else — but both apex operands can carry uppercase: the `namespace.cozystack.io/host` label value, which Kubernetes permits to carry uppercase and which the tenant chart writes verbatim from `tenant.spec.host`, and the templated `_cluster.root-host`, a free-form platform value that nothing validates for case. A mixed-case apex label wrongly denies a tenant its own legitimate lowercase in-apex hostname. An uppercase `_cluster.root-host` is the dangerous direction: the Ingress outside-root exclusion compares a lowercase host against the uppercase literal, never matches, so the exclusion stops excluding and the policy admits any non-wildcard host — every non-wildcard platform-apex name a tenant asks for, including a hostname belonging to another tenant and the root apex itself. Only the anti-wildcard gate still holds. This normalizes both operands of every hostname comparison with `.lowerAscii()` (host, apex label, and the templated root-host literal) across all three policies, so the match follows DNS case semantics. There is no behavior change in the default all-lowercase configuration. The Gateway policy's fail-open on an absent or empty host label is deliberately left as it was: that is the platform's one documented fail-open exception in `docs/security/threat-model.md`, so changing the posture is a security-model decision rather than a side effect of normalizing case. Its current shape is pinned by a test. Test coverage: - helm-unittest pins each comparison as a whole operand pair — equality and `endsWith`, across the rule-host and tls-host branches — so dropping the normalization from any single operand is caught. A pattern matching a lone `.lowerAscii()` would not: the apex appears four times in the Ingress expression, so stripping one operand still leaves three matches. - A rendering case with `_cluster.root-host: Example.ORG` pins the outside-root escape shut; it is red pre-fix. - e2e probes set a mixed-case apex on an isolated throwaway namespace and assert both halves of the match: a lowercase in-apex HTTPRoute and Ingress are still admitted (red pre-fix, green post-fix), while a foreign apex and a hostname ending with the apex without a dot boundary are still denied. Vacuous-pass guards re-read the persisted label and assert the probe apex really is mixed-case. Normalizing the apex at the write site — the tenant namespace template and `_cluster.root-host` — is tracked separately in #3328. It would not remove the need for this change: the host label is settable out-of-band by a trusted caller, so these policies must not assume the apex arrives normalized. Closes #3205. ### Screenshots N/A (no UI changes). ### Release note ```release-note fix(cozystack-basics): tenant hostname admission policies now compare against the tenant apex and the platform root apex case-insensitively. This closes a bypass where an uppercase `_cluster.root-host` made the Ingress policy's outside-root exclusion stop matching, so the policy admitted any non-wildcard host — including a hostname under the platform apex belonging to another tenant, and the root apex itself. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Hostname validation now consistently matches case-insensitively across Gateway, Ingress, HTTPRoute, and TLSRoute. * Mixed-case tenant and apex hostnames are accepted when they meet the intended domain boundaries. * Foreign domains and prefix-adjacent hostnames remain correctly rejected; validation behaves as documented when the tenant host is empty. * **Tests** * Expanded unit and e2e coverage for case-insensitive hostname rendering and boundary rejections across the supported resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
b496fb1ae8 |
docs(backups): note to-copy MongoDB restore needs target creds that can read the source bucket
A to-copy restore reads the dump from the source backup's bucket/endpoint but authenticates with the target cluster's own S3 credentials (the source release's Secret dies with it in a DR scenario). Document that the target's credentials must therefore be able to read the source bucket - the common same-bucket cozy-backups case - and that a cross-account/bucket target fails loudly with AccessDenied rather than silently restoring nothing. Addresses the MINOR credential-scoping note from PR review. Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
b30c7f4272 |
fix(backups): requeue MongoDB restore on a transient source-CR read error
resolveMongoDBBackupSource folded two failure classes into one error return:
a transient API read error on the live PerconaServerMongoDBBackup CR (cache
not yet synced, apiserver hiccup) and the terminal no-restorable-destination
case. The RestoreJob caller turned every error into markRestoreJobFailed, so a
retryable read error failed a restore that would have succeeded on the next
reconcile. The backup path already treats the symmetric live-CR read error as
transient (return ctrl.Result{}, err).
Wrap the transient read error in a new errTransientRestoreSource sentinel and
requeue on it at the caller; only a genuinely unresolvable source stays
terminal. Adds a non-vacuous interceptor-driven test.
Addresses the MINOR robustness note from PR review.
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
|
||
|
Andrey Kolkov
|
b97c443a16 |
fix(backups): harden MongoDB restore auth + PITR option handling from review
Address the two blocking findings in PR #3562: - Restore into a differently-named target now authenticates with the TARGET cluster's own S3 credentials instead of the source's. The backupSource carried the source release's -s3-creds Secret, which is deleted together with the source app — the disaster-recovery case backups exist for — leaving a to-copy restore unable to reach S3. The restore replays into the target (gated on backup.enabled just above), whose own -s3-creds Secret is present and grants access to the same bucket; only the credential reference is swapped, bucket/prefix/endpoint still come from the source archive. Falls back to the source reference when the target declares no discoverable s3 credentials (in-place restore resolves to the same Secret, so it is a no-op there). - A non-string recoveryTime (spec.options is free-form runtime.RawExtension, so {"recoveryTime": 20260805} passes admission) no longer silently degrades a PITR request to a plain snapshot restore reported Succeeded. When the typed decode fails and the blob carries a recoveryTime key, the RestoreJob now fails terminally — the same guarantee pitrSpec already makes for a malformed RFC3339 string. Minor findings also addressed: - BackupJob tolerates a not-yet-present MongoDB app CR with the same bounded grace the psmdb-CR existence gate uses, instead of failing terminally on a GitOps single-commit ordering race. - examples: the to-copy target writes into its OWN S3 prefix (mongodb-target/) rather than the source's, so the two clusters' oplog and backup metadata no longer interleave under shared keys (both run replset rs0). - examples: wait_psmdb_ready no longer treats a transient state=error as terminal — the cluster state is recomputed each reconcile and can blip through error during replset init before converging to ready. Unit tests cover the target-credential selector (preferred/default/sole storage) and the load-bearing recoveryTime-key detection. Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
0d4a3e3134 |
refactor(backups): make MongoDB PITR option uniform with CNPG + warn on unknown restore options
PITR: replace the bespoke spec.options.pitr{type,date} blob with a single
spec.options.recoveryTime RFC3339 string, matching the CNPG strategy's option
exactly (CNPGRestoreOptions.RecoveryTime). The driver converts recoveryTime to
the psmdb Restore's pitr{type:date,date} in the operator's "YYYY-MM-DD HH:MM:SS"
UTC format. An empty recoveryTime restores the snapshot as-taken (documented
difference from CNPG, whose empty value means latest WAL — a psmdb logical
backup is already consistent). A non-RFC3339 recoveryTime fails terminally.
Unknown options: parseMongoDBRestoreOptions now also returns the set of
spec.options keys the driver does not recognise; reconcileMongoDBRestore emits a
UnknownRestoreOption Warning event per unknown key (and logs it) instead of
silently ignoring it — so a "recoverytime" typo no longer silently degrades a
PITR restore to a snapshot restore. (BackupJob carries no spec.options blob, so
there is no backup-path equivalent; the only backup-time input is the free-form
BackupClass parameters used as template vars.)
Docs + examples updated to recoveryTime; tests cover the RFC3339→UTC mapping,
empty/invalid recoveryTime, and unknown-key reporting.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
|
||
|
Andrey KolkovandClaude Opus 4.8
|
540bb94f58 |
test(backups): cover createMongoDBBackupArtifact producer seam
Address branch-review blocker: add TestCreateMongoDBBackupArtifact_ArtifactShape, mirroring the MariaDB sibling. createMongoDBBackupArtifact writes the driver-metadata keys (backup-name/namespace/destination), the Status.Artifact URI, and the persisted snapshot that the restore path reads back — the producer half of a seam previously exercised only end-to-end. A key-name drift between this producer and resolveMongoDBBackupSource would otherwise pass every unit test and surface only in e2e. The test asserts the metadata keys, phase, artifact URI, and that the snapshot decodes to a usable by-reference backupSource; a second case pins the empty-destination path (nil Artifact, no destination key). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
bc5e47c64f |
fix(backups): add explicit MongoDB cleanup-on-delete branch + its guard test
Address branch-review recommendation: give the MongoDB strategy an explicit case in BackupReconciler.cleanupOnDelete, matching CNPG / Altinity / MariaDB / FoundationDB. A deleted MongoDB Backup artifact previously fell through to the Velero default (a safe no-op — MongoDB driverMetadata never carries velero.io/backup-name), but the explicit branch documents the "we do not own the psmdb S3 archive" contract and guards the seam against a future shared helper incidentally stamping velero.io/backup-name onto MongoDB metadata. Ships with TestBackupCleanup_MongoDB_DoesNotFallThroughToVelero, mirroring the one-test-per-branch convention its three siblings follow: it seeds the velero metadata keys + a matching velero.io/Backup and asserts no DeleteBackupRequest is synthesised and the Velero Backup survives — so deleting the branch makes the test fail, which is what makes the guard self-verifying. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
dff56aa998 |
feat(backups): add a native MongoDB (psmdb) backup-strategy driver
Adds a MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so `apps.cozystack.io/MongoDB` reaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore-to-differently-named instances, and point-in-time recovery through the unified BackupJob/RestoreJob/BackupClass/Plan interface (closes #3545). The driver orchestrates the operator's PerconaServerMongoDBBackup / PerconaServerMongoDBRestore CRDs: - BackupJob: creates a one-shot PerconaServerMongoDBBackup, polls status.state, and persists the S3 destination + storage descriptor (by-reference, no credentials) onto the Cozystack Backup artifact. - RestoreJob: drives PerconaServerMongoDBRestore via backupSource (read back from the source backup status, snapshot fallback) so the same artifact restores in-place or into a differently-named instance; PITR via spec.options.pitr (date/latest). Because psmdb resolves the backup storage by name against the cluster's spec.backup.storages, the driver names the chart's storage (s3-storage) via the cozy-default-mongodb Strategy rather than injecting it, and surfaces a clear Ready=False precondition when a target has backups disabled. Wiring: - api/backups/strategy/v1alpha1: MongoDB strategy type + generated CRD/deepcopy. - internal/backupcontroller: psmdbstrategy_controller + psmdbtypes/mongodbapp partial-schema mirrors; dispatch, supported-kinds and scheme registration. - backupstrategy-controller: cozy-default MongoDB route, cozy-default-mongodb Strategy CR, and psmdb.percona.com RBAC. - apps/mongodb: backup.insecureSkipTLSVerify knob (psmdb s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed seaweedfs. Tests + e2e: - unit tests for the driver (validation, ensure-idempotency, precondition, backupSource resolution, snapshot round-trip, PITR option parsing). - examples/backups/mongodb + a mongodb-2-backup-roundtrip Chainsaw suite that drives run-all.sh end-to-end (sentinel document round-trips through S3 into a restored copy while the source stays untouched), per docs/agents/e2e-testing.md. - docs/operations/backup-classes.md updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
scooby87
|
a242f34593 |
fix(clickhouse): assign DB_LIST so backup retention cleanup runs (#3760)
## What this PR does The legacy per-instance ClickHouse backup CronJob (rendered when `backup.enabled=true`, `backup.schedule` is set and `backup.useSystemBucket=false`) never applied its retention policy. `packages/apps/clickhouse/templates/backup-script.yaml` printed `echo DB_LIST=$(...)` instead of assigning `DB_LIST`, so the variable stayed unset and the cleanup loop `for db in $DB_LIST` ran zero iterations — `restic forget` and `restic prune` were never invoked, snapshots accumulated in object storage indefinitely despite the configured `cleanupStrategy`, and the Job still exited 0 so nothing surfaced the failure. This drops the stray `echo` so `DB_LIST` is assigned from the table list and the retention cleanup runs as configured. MariaDB's equivalent backup script already assigns `DB_LIST` correctly; this brings ClickHouse in line. A helm-unittest regression over the rendered `backup.sh` asserts the assignment is present and the `echo DB_LIST=` typo is gone. Verified with `make -C packages/apps/clickhouse test` (18/18 pass). Fixes #3758. ### Screenshots <!-- No UI changes. --> ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The change is a one-line shell fix inside a Helm template plus a helm-unittest case — it touches no `values.yaml`, `values.schema.json`, `README.md`, `ApplicationDefinition`, version enum, variant/bundle, release asset, `hack/` layout or namespace. No downstream repository restates any of this. - [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: ### Release note ```release-note fix(clickhouse): ClickHouse backup retention now prunes old snapshots. The scheduled backup job's cleanup step never ran because `DB_LIST` was left unset, so snapshots accumulated in object storage indefinitely; it now honors the configured retention (`cleanupStrategy` / `--keep-last=N`). ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed backup processing so table lists are assigned correctly, allowing retention cleanup to run as expected. * Removed unintended output of the shuffled table list during backups. * **Tests** * Added regression coverage to verify correct backup list assignment and prevent the previous malformed assignment from recurring. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |