332 Commits

Include renames

Author	SHA1	Message	Date
Aleksei Sviridkin	ef96292b64	fix(ci): give the nightly e2e runner headroom over its guest vCPUs (#3833) ... ## What this PR does The e2e sandbox boots three QEMU nodes at `-smp 8`, so 24 guest vCPUs are committed before any tenant VM exists. Four jobs boot that sandbox. Three run on the 32-vCPU class; nightly ran on the 24-vCPU one, where 24 guests against 24 host vCPUs leaves nothing for QEMU's own emulation and IO threads, containerd, the kernel and the runner agent. The other lanes moved to the larger class at some point and nightly was not moved with them. The sandbox keeps its 24 vCPUs. That guest count is part of what e2e claims to test, and a smaller nightly cluster stops matching the one PRs run against. This is hygiene, not a fix. Measured boot-phase data across 18 runs says the load a workflow puts on the host dominates guest starvation, not the runner class: the lanes already on 32 vCPU boot guests slower than nightly did on 24, and class and workflow kind are fully confounded there, so neither separates out. Do not read this as making anything faster or as addressing a particular red run. What is left is the arithmetic. A 1:1 commit has no headroom at all, and two lanes running the identical sandbox on different classes is drift nothing was watching. The rest is a check, because the guest demand and the runner classes live in different files and nothing compared them. `hack/sandbox-runner-headroom.bats` reads `-smp` and `-m` times the loop count out of `hack/e2e-prepare-cluster.bats`, reads the runner class of every job that calls the sandbox make targets, and wants the runner strictly larger on both axes. Neither side is a hardcoded list, so bumping `-smp`, adding a management node, or adding a fifth lane on a smaller runner turns it red on the same commit. Memory is in there because the same argument applies to it and `-m 24576` sits on the same line as `-smp 8`, not because a class that violates it exists today. It stops instead of guessing when it cannot read something, since a guess comes out smaller than the real demand and passes a runner that cannot hold the sandbox. Refused: a missing or non-literal `-smp` / `-m`, a launch written outside any `@test` block or inside one left open at end of file, a `for` opener the strict pattern does not match, a `for` list holding anything but literal words, a `while` sharing the block, two launches or two loops in one block, and a `runs-on` with no `oracle-vm-<n>cpu-<m>gb` in it. Unparsed loop openers are counted separately from parsed ones, so moving `do` to the next line is refused rather than silently multiplied by one. There is also a cross-check that every workflow calling the sandbox make targets yields at least one matched job, so a renamed target turns the parser red instead of quietly shrinking what it covers. The derivation runs against the live tree, which passes whether or not those refusals still work, so each one is pinned by a fixture that feeds a synthetic file to the same program. That matters more than it sounds: two of the refusals above exist because the first version of this file did not have them, and a launch moved into a file-level helper would have halved the measured demand and stayed green. The numbers come from: ```bash sed -n '98,107p' hack/e2e-prepare-cluster.bats # three nodes, -smp 8, -m 24576 grep -rn 'prepare-env' .github/workflows/ # the four jobs that boot it grep -rn 'runs-on: oracle-vm-' .github/workflows/ # their runner classes ``` The rule also goes into `docs/agents/e2e-testing.md` next to the other environment invariants, so whoever adds the fifth lane picks its runner class from the docs instead of from a red unit suite. Checked that it bites, twice over. Fourteen mutations of the inputs: reverting the runner class, `-smp 11`, a fourth and fifth management node, `-smp cpus=8`, `-m size=24576`, `do` on the next line, `$(seq 1 3)` as the list, a `while` loop, the launch moved into a file-level helper, renaming the make target, `ubuntu-latest`, a matrix expression in `runs-on`, a hypothetical `32cpu-64gb` class, and a new lane on a 4-vCPU runner. Then eleven mutations of the parser itself, one per refusal, each of which kills exactly the fixture that names it and nothing else. Green on this tree, and the same numbers out of gawk, mawk and BSD awk. Closes #3831. ### Screenshots No UI change. ### Downstream repositories Walked the trigger map against the diff. It changes one `runs-on` line plus a comment in `.github/workflows/nightly.yaml` and adds one file under `hack/`. The `ccp` trigger is moving or renaming something under `hack/`, or changing what a make target does; `hack/package.mk` and `hack/common-envs.mk` are untouched, and the new test is picked up by the existing `hack/*.bats` wildcard without the target changing. The `ansible-cozystack` and `talm` triggers are node prerequisites in `hack/e2e-prepare-cluster.bats`, which this reads and does not modify. - [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(ci): run the nightly e2e suite on a runner class larger than the sandbox's guest vCPU demand, and add a unit check that fails when an e2e lane loses that headroom ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated nightly end-to-end testing to run on a higher-capacity runner, improving support for multiple QEMU guests and required resource headroom. * Added validation to prevent sandbox runners from being undersized or oversubscribed. * **Documentation** * Documented the resource requirements and deterministic conventions for end-to-end sandbox testing. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-15 12:59:55 +03:00
Aleksei Sviridkin	527d4ea00f	test(e2e): measure tenant worker CPU as a rate, and the sandbox nodes under it ... Two instruments for the node-join failure the kubernetes-* suites keep hitting. Neither changes what the suites assert. The worker CPU capture now reads container_cpu_usage_seconds_total beside the CFS counters, and reads them twice a fixed interval apart instead of once. The throttled counters say a container met its ceiling and cannot say how much CPU it got, which are the two candidate explanations for a stalled worker: a guest held at a one-core ceiling, and a guest whose vCPU thread is rarely scheduled at all. Both leave throttled periods behind. Every counter involved accumulates from container start, so a single reading is an average over the whole uptime and no rate at all, and the profile under investigation is a guest that freezes for tens of seconds and then runs flat out: the average comes out low, the instantaneous figure comes out at the cap, and only the second separates it from a guest that is simply capped. The sandbox nodes are a new subject. The worker captures describe a cgroup, so a guest that is runnable and never scheduled looks from there like a guest that asked for nothing. Whether the sandbox node itself was given its turn is recorded only in the steal column of its own /proc/stat: no kubelet endpoint carries it, and crust-gather cannot reach it because PodSecurity refuses its node-shell Pod here, so the read goes over the Talos API, which is already how the report reaches those nodes for dmesg. The rows are captured verbatim with their column legend beside them rather than reduced to a percentage, because /proc/stat has no header, steal and guest sit next to each other, and guest is large on a node running VMs and already counted inside user. Both subjects are read on either side of one shared wait, so the pair pays it once. The wait is not the interval either subject's counters span, though: each pass walks its own nodes and the other subject's pass falls between a subject's two readings, and on the run this exists for those passes are the slow part. Each capture therefore stamps both ends of its read, so a rate comes from subtracting stamps rather than from assuming the knob, and the width of those brackets is the uncertainty. That wait is a measurement interval rather than a stand-in for a condition nobody wrote a wait for, so the e2e conventions carve it out by name instead of marking it as a fixed timeout to be replaced. The pair is gated below the serial console rather than above it. Its ceiling is sixteen bounded reads plus the interval, which is most of the phase budget, and admission gates only when a collector may start, so above the console a run that hit those bounds would leave the console never started rather than merely cut short. The console is the only capture that survives a worker which never reached apid, which is the shape this failure usually takes. That overrun is inside the term the budget derivation already allows for one late admission, and a guard holds the pair's ceiling against it. No collector here may leave an empty directory, since an empty directory reads as a healthy node: a missing binary, a missing talosconfig, a listing that never answered, a node with no address and a read that was cut off each land as a named note or a COLLECTION-FAILED.txt. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-15 10:19:19 +03:00
Aleksei Sviridkin	db2ddfab2b	fix(ci): give the nightly e2e runner headroom over its guest vCPUs ... The e2e sandbox boots three QEMU management nodes at `-smp 8`, so 24 guest vCPUs are committed before any tenant VM starts. The nightly lane ran that sandbox on a 24-vCPU runner, a 1:1 commit that leaves the host nothing for QEMU's emulation and IO threads, containerd, the kernel and the runner agent. The tag, fork and pull-request lanes already run the same sandbox on the 32-vCPU class, so move nightly there too. This aligns headroom and claims no performance effect. Measured boot data shows the load a workflow puts on the host, not the runner class, dominates guest starvation, and the two are confounded in that data; no red run is attributed to the class here. The case is the arithmetic: a 1:1 commit leaves no headroom, and two lanes running the identical sandbox on different classes is drift nothing was watching. Add hack/sandbox-runner-headroom.bats to keep the two sides paired. It derives the guest vCPU and memory demand from the sandbox bats file, and the runner class of every job that drives the sandbox make targets, then requires the runner to be strictly larger on both axes. Neither side is a literal list, so raising `-smp`, adding a management node or adding a lane on a smaller runner turns it red instead of drifting apart silently. Input the derivation cannot read in full is refused rather than guessed: a guess produces a demand smaller than the real one, which passes a runner that cannot hold the sandbox, while a refusal names the line. That covers a launch written outside any `@test` block, a block left open at end of file, a `for` opener the strict pattern misses, a non-literal loop list, and a `while` loop sharing the block. The derivation runs against the live tree, which passes whether or not those refusals still work, so each one is pinned by a fixture feeding a synthetic file to the same program. Loosening the parser turns a fixture red rather than quietly widening what the check accepts. Record the rule alongside the other environment invariants in docs/agents/e2e-testing.md, so a new lane picks its runner class from the documentation rather than from a failing unit suite. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-15 02:41:19 +03:00
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 -->	2026-08-14 13:49:46 +03:00
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 -->	2026-08-14 10:54:10 +03:00
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>	2026-08-13 16:29:48 +05:00
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 -->	2026-08-13 11:18:46 +03:00
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 -->	2026-08-13 02:24:50 +03:00
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>	2026-08-12 18:45:22 +03:00
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>	2026-08-12 18:43:11 +03:00
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>	2026-08-12 18:42:58 +03:00
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`.	2026-08-12 18:27:03 +05:00
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>	2026-08-12 12:00:33 +04:00
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>	2026-08-12 12:00:33 +04:00
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>	2026-08-12 12:00:33 +04:00
Aleksei Sviridkin	cf6bf2e379	fix(ci): stop labeled events from cancelling and re-running backports (#3569) ... ## What this PR does A run joins its concurrency group before any job-level `if` is evaluated, so `prepare`'s guard in `backport.yaml` is too late to help. By the time it works out that a label event is irrelevant, that run has already cancelled whatever was in flight for the same PR. Labels set with the default `GITHUB_TOKEN` start no run, which is why the in-repo labelers never showed this. Third-party GitHub Apps do start runs, and they arrive in bursts. The delivery survived, which is why this went unnoticed for so long. The old guard admitted a run on the PR's cumulative label set, so the last label of a burst, the one nothing cancels, requalified and redid the work. In the bursts on record the kill lands within seconds of the merge run starting, well before the cherry-pick, so the cost was a restart rather than a lost backport, recorded as nothing more than a cancelled run sitting next to a green one. Two changes to the concurrency key, and both are needed. A label event that requests no backport now goes into a group of its own, so it can neither cancel nor displace the run doing the work. A label event that does request one queues behind that run instead of killing it, because `backport` and `backport-previous` are separate requests and both have to finish. Splitting alone would let `backport` cancel the merge-triggered run. Queuing alone would let an unrelated label evict a pending request, since the group holds a single pending run and a newer one replaces it. `prepare`'s condition is scoped in the same commit. It read the PR's cumulative label set, so once a merged PR carried `backport`, any later unrelated label re-entered the job to redo a backport already delivered. That run used to get cancelled by the next label in the burst. After the change above it runs to completion, so the scoping has to land here rather than as a follow-up. The coupling runs the other way too, and it is the sharper half. That redundant run is exactly what redelivered the backport the burst had just killed, so narrowing the guard on its own would convert a noisy delivery into a missing one. Neither change is safe to land without the other. Both jobs also gain a `timeout-minutes` ceiling, which is a consequence of the queuing rather than housekeeping. A cancelling key disposed of a stuck run by killing it; queuing makes the next genuine request wait behind it instead, on the six-hour job default, which turns one wedged job into a six-hour hole in the release line. That ceiling reaches a stuck job and nothing else: a run can also wedge at the run level with every job already finished, and `timeout-minutes` has no run-level equivalent, so nothing in the workflow reaches that state. `docs/release.md` now tells an operator to look for a run still in flight and clear it before re-applying the label, because the retry queues behind it rather than replacing it. The ceiling bounds execution, not the wait for a runner, and thirty minutes is generous rather than calculated. Execution on run 29844315422 was six seconds for `prepare` and two and sixteen for the matrix legs, against 9m24s, 5m28s and 5m45s of runner queue for the same three jobs. That queue is a third unbounded case: GitHub's limits page says "job execution time" for the six-hour job cap while spelling out that the 35-day run cap "includes execution duration, and time spent on waiting and approval", and [orgs/community#50926](https://github.com/orgs/community/discussions/50926) shows a job sitting six hours on "Waiting for a runner to pick up this job" under `timeout-minutes: 5`. So the value is picked to leave room for a cherry-pick far larger than any on record while replacing the six-hour default, and nothing here is sized against the queue, because nothing here can be. This removes a mitigation, and the trade is worth naming on both sides. Under an unconditional cancel a run wedged at the run level cleared itself, because the next event killed whatever was sitting in the group. That is the same behaviour that killed live backports, so the mitigation is inseparable from the bug being fixed: keeping it means keeping label events that cancel a backport in flight. What it buys is a change of failure mode rather than one fewer failure. A backport lost silently, discovered weeks later as a fix missing from a release line, becomes a delay the operator can see in the run list, with the recovery written into `docs/release.md`. There is no lossless alternative here: `queue: max` removes the eviction but only by dropping the conditional cancel entirely, and the group key cannot tell a manual retry from a second genuine request. The run cited above, 31232645133, is still live and retained on purpose: it is the only observed instance of the run-level wedge, so clearing it would remove the evidence for the paragraph that describes it. Its exposure is one pull request, #3262, which carries no backport label, so nothing is queued behind it. The old condition also carried a disjunct that could never decide anything. A `labeled` payload already lists the new label in `pull_request.labels`, so `contains` was true whenever the explicit `github.event.label.name` check was, and the name check only ever agreed with a `contains` that had already matched. The restructure drops it by construction rather than by deletion: the name check is now the whole of what a `labeled` event may match on. ### Why this no longer touches `pull-requests.yaml` `main` fixed it there by splitting the group key, then refined that so a `full-e2e` label stays in the main group instead of publishing a second `E2E Tests` status on one SHA. That is the better mechanism, it neither queues nor delays, and `gate-concurrency-contract.bats` now pins `cancel-in-progress: true` in that file, so what this PR originally did would turn it red. Nothing was left to add there, so the hunk is dropped and its pin with it. The split-key idea is applied in `backport.yaml` instead, where it did not exist. One claim from the earlier description is withdrawn. It said the leftover risk was a duplicate backport PR, opened when the original had merged and its branch had been deleted before the queued run pushed. `main` has since gained a guard that lists closed PRs on the `backport-<n>-to-<target>` branch and skips the target when one of them merged. Branch deletion does not defeat it: `GET /branches/backport-3510-to-release-1.6` is 404 while the same head filter on `/pulls?state=closed` still returns the merged PR. So a redundant run only cost runner minutes, and the scoping above removes the run. ### Screenshots Not applicable, no UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The only workflow it cites is `tags.yaml`, for the release-prep behaviour `cozystack/ccp` documents. Every `hack/` trigger it lists either names a specific file another repository mirrors or gates on, or covers moving and renaming under `hack/`. This PR appends tests to a contract file the map does not name, and moves nothing. No package, chart, values file, CRD, image reference, namespace, variant or node prerequisite in the diff. - [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(ci): stop `labeled` pull request events from cancelling the automatic backport run already in flight. A burst of labels from a third-party app could previously kill the backport the merge had just started, leaving the work to be redone by whichever label arrived last. ```	2026-08-11 16:50:50 +03:00
Aleksei Sviridkin	e0d6c6be92	fix(ci): stop labeled events from cancelling and re-running backports ... A run joins its concurrency group before any job-level `if` is evaluated, so `prepare`'s guard cannot stop a label event from disturbing the run already backporting a PR. Labels set with the default GITHUB_TOKEN start no run, but labels set by third-party GitHub Apps do, and they arrive in bursts, so each one cancelled the backport the merge had just started. The delivery survived anyway, which is why this went unnoticed: the old guard admitted a run on the PR's cumulative labels, so the last label of a burst requalified and redid the work. In the bursts on record the kill lands within seconds, well before the cherry-pick, so the cost was a restart rather than a lost backport. Move a label event that requests no backport into a group of its own, and let one that does request a backport queue rather than cancel. Both halves are needed. `backport` and `backport-previous` are separate requests that both have to complete, so they share a group and must not cancel each other, while every other label does no work here and only has to stay out of the way. Scope `prepare` to the labels each trigger is entitled to read at the same time. Reading the cumulative label set on a `labeled` event let any later unrelated label on a merged PR still carrying `backport` re-enter the job to redo a backport already delivered. That redo is also what redelivered a killed backport, so narrowing the guard alone would turn a noisy delivery into a missing one: neither change is safe to land without the other. Bound both jobs with `timeout-minutes` for the same reason, since a queued request now waits on the run ahead of it rather than replacing it. The ceiling bounds execution and not the wait for a runner, which no job-level key reaches, so 30 is generous rather than calculated: every execution measured on this workflow has been seconds, against minutes of queue that nothing here can bound. Document the recovery path the narrowing removes: with no accidental re-run left, re-applying whichever backport label the PR already carries is how an operator retries a merge-triggered run that died. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-11 03:14:09 +03:00
Andrey Kolkov	d0990299ed	feat(clickhouse): let the backup sidecar trust a private S3 CA, unblocking the backup e2e round-trip (#3385) ... ## What this PR does Gives the ClickHouse `clickhouse-backup` sidecar a way to trust an S3 endpoint whose certificate is signed by a private CA, and uses that to make the ClickHouse backup/restore e2e round-trip actually run in CI. This actualizes #2600, whose feature code and example scripts have since landed on `main` independently, and whose remaining contribution — the e2e test — was still in the retired BATS format. #2600 can be closed once this merges. ### 1. `backup.endpointCA` on the ClickHouse chart The sidecar had **no TLS/CA surface at all**: bucket coordinates and credentials arrive as `S3_*` env, and nothing let it verify an endpoint signed by a private CA. Cozystack's own in-cluster SeaweedFS is exactly that case — it serves `:8333` behind the self-signed "SeaweedFS CA" — so a ClickHouse release could only ever back up to a publicly-trusted endpoint. MariaDB (`storage.s3.tls.caSecretKeyRef`) and Postgres (barman `endpointCA`) have had a CA field all along; this closes the gap for ClickHouse. `backup.endpointCA` takes `{name, key}`. When set, the chart mounts that Secret into the sidecar and points `SSL_CERT_DIR` at it. Empty — the default — renders no env, no mount and no volume, so existing releases are byte-for-byte unchanged. **Why `SSL_CERT_DIR` and not `AWS_CA_BUNDLE`.** Both are honoured by `altinity/clickhouse-backup:2.7.4`; I measured them against a self-signed endpoint and public AWS S3 in turn. Go reads `SSL_CERT_DIR` *in addition to* the system bundle, so both endpoints verify. `AWS_CA_BUNDLE` *replaces* the SDK's trust pool: the private endpoint verifies, and public S3 then fails `x509: certificate signed by unknown authority` — it would silently break a release that also talks to a public endpoint. `S3_DISABLE_CERT_VERIFICATION` was rejected outright, since it drops verification rather than trusting one more CA. ### 2. The e2e suite, and what it was really testing Two Tests added to the existing `hack/e2e-chainsaw/clickhouse/` suite (in name order, mirroring the `etcd`/`mariadb`/`postgres` shape — see the layout rationale below for why they live here rather than in a new directory): - **`clickhouse-1-backup-contracts`** (CI, `tenant-test`, ~98s): a backup-enabled ClickHouse asserted against the *running* cluster — the `clickhouse-backup` sidecar on the `ch-backup-api` `:7171` port, and the `<release>-backup-api-auth` Secret whose keys the sidecar's `API_USERNAME`/`API_PASSWORD` actually resolve `valueFrom`. Each is a silent break if a chart bump renames it. Needs no object store. - **`clickhouse-2-backup-roundtrip`** (CI, `tenant-root`): `Bucket` → `Altinity` strategy + `BackupClass` → source ClickHouse with a sentinel row → `BackupJob` → in-place `RestoreJob` → to-copy `RestoreJob` into a second instance, driving `examples/backups/clickhouse/run-all.sh` so test and docs cannot drift. The round-trip was originally gated behind `CLICKHOUSE_E2E_S3_ROUNDTRIP` and **skipped in CI** — its `PASS (1.01s)` meant the gate fired, not that a backup happened. `backup.endpointCA` removes the reason for the gate, so the suite now adopts the same three preconditions mariadb/postgres rely on: run in `tenant-root` (an isolated tenant's Cilium egress allowlist blocks its Pods from reaching `tenant-root`'s SeaweedFS), target the in-cluster endpoint instead of the `Bucket`'s external ingress URL (an unroutable placeholder in CI), and trust its certificate via the copied CA. It also pre-cleans with `cleanup.sh`, because `tenant-root` is shared and a leftover `Succeeded` `BackupJob` would satisfy the harness's wait without a backup ever running. Both backup Tests live in the existing `hack/e2e-chainsaw/clickhouse/` suite rather than a new suite directory, so no selector mapping is added: `hack/select-e2e.sh` already derives `clickhouse` both from `packages/apps/clickhouse/**` (the `*-application` rule) and from `examples/backups/clickhouse/**` (the harness path), and both paths therefore select the suite that actually runs the harness. That alignment is the reason for the layout — a separate `clickhouse-backup/` directory would have made an edit to the harness select a suite that cannot exercise it. `hack/select-e2e.sh` and `hack/select-install.sh` are otherwise untouched (the only change is a comment recording that invariant), and the existing `backup example harness edit selects its app suite` case in `hack/select-e2e_test.bats` covers the mapping. ### Verification - `helm unittest` — 17 pass, including 5 new `endpointCA` cases: the unset default renders nothing, the env/mount/volume wiring, a custom key still projected as `ca.crt`, composition with `useSystemBucket`, and `backup.enabled=false` renders nothing. - The CA-trust behaviour was measured against the real image, not assumed: without a CA knob the sidecar reports `x509: certificate signed by unknown authority`; with `SSL_CERT_DIR` the handshake succeeds. - `hack/select-e2e_test.bats` (16) and `hack/select-install_test.bats` (17) pass; `select-install.sh --validate` reports graph OK. - `make generate` is idempotent, so the pre-commit job's `git diff --exit-code` stays clean. - The contracts Test passed in CI on earlier runs of this branch. The round-trip only became CI-live with this work, so it still needs a green E2E run before merge — the branch has since been rebased onto current `main` to get one. ### Screenshots Not applicable — no UI changes. ### Downstream repositories Walked the trigger map against the diff, file by file. The load-bearing change is the new `backup.endpointCA` field in `packages/apps/clickhouse/values.yaml` / `values.schema.json`; everything else is `hack/e2e-chainsaw/`, `examples/backups/`, one docs paragraph, and regenerated artifacts, none of which appear in the map. - [ ] No downstream repository is affected by this change - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: https://github.com/cozystack/terraform-provider-cozystack/issues/18 — "add, remove or rename a field in an app's `values.schema.json`" reaches the provider's hand-maintained schema, model and expand/flatten pair, so the new nested object is not surfaced until it is added there. Opened as an issue rather than a PR because the natural moment is alongside that repo's next `api/apps/v1alpha1` pin bump, which is a maintainer call. Not ticked, with reasons rather than silence: **cozystack/website** — the trigger is *adding, renaming or removing a package*, and this adds a field to an existing one. `clickhouse` is already in the website `Makefile`'s app list and its reference page is generated from this package's `README.md`, so the regenerated `backup.endpointCA` rows are carried by the existing release-tag automation with no manual PR. **cozystack/external-apps-example** — vendors `hack/package.mk` and `update-crd.sh`, neither of which this PR touches. **cozystack/ansible-cozystack**, **ccp**, **talm**, **cozyhr**, **cozy-proxy**, **cozystack-telemetry-server**, **examples** — no installer values, `hack/` layout, namespace, variant, node-prerequisite, annotation, label or metric in their trigger rows is touched here. ### Release note ```release-note feat(clickhouse): add `backup.endpointCA` so the clickhouse-backup sidecar can verify an S3 endpoint whose certificate is signed by a private CA, such as Cozystack's in-cluster SeaweedFS. Leave it unset for publicly-trusted endpoints; existing releases are unaffected. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional private S3 endpoint CA support for ClickHouse backup/restore sidecars via `backup.endpointCA`, including configurable Secret name and CA bundle key (default `ca.crt`). * Sidecars mount the CA and extend trust while leaving public endpoints unchanged. * **Documentation** * Updated ClickHouse chart docs, operational backup guidance, and backup/restore examples to cover CA setup, overrides, and cleanup. * **Tests** * Added helm-unittest and end-to-end coverage validating CA rendering/mounting behavior and ClickHouse backup/restore workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-10 21:17:47 +04:00
IvanHunters	cfaf2e086c	fix(backupstrategy-controller): repair lookup-gated backup objects (#3524) ... <!-- 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 Fixes #3518. Objects in `packages/system/backupstrategy-controller` and `packages/system/bucket` are gated on a Helm `lookup` of an object that is still being created. When the lookup is empty the object is skipped — and since helm-controller does not re-render a release whose chart and values did not change (drift detection is off on operator-generated HelmReleases), **the skip is permanent**, not a bootstrap window. On the reporter's cluster the install-timing window was 4 seconds and the objects were still missing months later: - the 7 default backup `Strategy` CRs and the Velero `BackupStorageLocation`, gated on `include "backupstrategy-controller.bucketName"` → a `lookup` of `BucketClaim`; - the `<bucket>-<user>-credentials` Secret (`packages/system/bucket/templates/user-credentials.yaml`), gated on a `lookup` of the COSI Secret. Only `BackupClass cozy-default` was present, because it renders unconditionally. Its own comment predicted this case while assuming "Flux re-renders the chart", which does not happen — the same trap `packages/core/platform/sources/cozystack-basics.yaml` already documents: *"Drift detection is off on operator-generated HelmReleases, so a capability gate in the templates would render the policy out at first install and never add it back."* Consequence: migration 50 (etcd adoption, v1.6.0) is fail-closed on the snapshot target. It resolves the target from the projected credentials Secret, which has no source without `<bucket>-<user>-credentials`, so its Job reboots forever and the cluster cannot be upgraded at all. ### 1. `backupstrategy-controller` reconciles the objects into existence New leader-elected `DefaultObjectsGate` runnable (`internal/backupcontroller/default_objects_gate.go`), modelled on the existing `SystemCredentialsProjector`. Every minute: - resolves the bucket name from the **projector's source Secret** (`parseSourceSecret`) — no new dependency, no `lookup`, and it covers the external-S3 path too. While it is unresolvable the gate does nothing: forcing then would only re-run the same empty lookup; - reads `BackupClass cozy-default` as the manifest of what must exist (its `strategyRef`s), plus the `cozy-default` BSL when `velero.bslEnabled`; - when any is missing, stamps `reconcile.fluxcd.io/forceAt` **and** `requestedAt` on its own HelmRelease. Both are required: `requestedAt` alone only requests a reconcile, which is a no-op for an unchanged release; `forceAt` is what triggers a real Helm upgrade, and it is only honoured together with `requestedAt`; - throttled to one forced upgrade per 5 minutes, and a no-op in the steady state. A `strategyRef` whose CRD is not installed is not counted as missing, so a missing CRD cannot drive an endless force loop. Two new metrics: `cozystack_backup_default_objects_missing{backupclass}` — the alert that would have caught this, since it is non-zero regardless of the HelmRelease's `Ready` condition — and `cozystack_backup_default_objects_force_reconciles_total`. RBAC delta is two lines: `patch` on `helmreleases` (`get` was already there) and `get` on `backupstoragelocations`. Both accesses are point Gets through the dynamic client, so no cluster-wide informer and no `list`/`watch`. Off switch: `backupStorage.reconcileDefaultObjects: false`. **Why not a render-time `fail` in this chart:** it would deadlock. `templates/bucket.yaml` in this same chart is the producer of the `Bucket` whose `BucketClaim` the lookup reads, so a failed render never applies it and the condition can never resolve. **Why not template the objects in Go:** their bodies are values-driven (endpoint, region, `forcePathStyle`, the Altinity strategy's whole PodTemplateSpec). Helm stays their single author; the gate only makes sure the render that produces them actually happens. **Why not `dependsOn`:** an edge guarantees the `Bucket` CR is *applied*, not that its claim is *reconciled*, so it does not close the race. ### 2. The gate also repairs the credentials Secret, one release earlier The `<bucket>-<user>-credentials` Secret is the same trap as the Strategy CRs, one release up the chain: `packages/system/bucket/templates/user-credentials.yaml` renders it behind a `lookup` of the COSI Secret, and skips it just as permanently. It is also the *first* domino — the projector reads it, so without it there is no `cozy-backups-creds`, no strategy, no Velero, and no migration 50. An earlier revision of this PR made that render `fail` so the release would retry and converge. [@IvanHunters pointed out](https://github.com/cozystack/cozystack/pull/3524#issuecomment-5214269078) that Helm cannot render a partial set, so the `fail` aborts the **whole** `<bucket>-system` release — the other users' Secrets and the bucket UI Deployment/Service/Ingress/HTTPRoute with them. One declared user whose `BucketAccess` never provisions would take down every other user of that bucket, and on an installed release park it in `Failed`, blocking every later upgrade. That was a real blast-radius regression on a universal path, and it is gone. The repair now lives in the gate, where it is per-object: - while the source Secret is absent, or carries no bucket name, the gate stamps `forceAt` + `requestedAt` on the bucket's `bucket-<name>-system` HelmRelease instead of doing nothing; - **no self-deadlock:** the `BucketClaim` and the `BucketAccess` whose COSI Secret that lookup reads are rendered unconditionally by the **parent** release (`packages/apps/bucket/templates/bucketclaim.yaml`), not by the one being forced; - **no RBAC delta:** `patch` on `helmreleases` is already cluster-scoped; - the two releases are throttled **independently**. They are forced in sequence on a bootstrap (credentials, then the objects the resolved bucket unblocks), so one shared timestamp would delay the second by a full `MinForceInterval` for no reason; - coordinates come from the chart under `provisionBucket`, and are omitted on external S3, where the Secret is admin-managed and no release renders it. The bucket chart keeps its partial render. Its comment now explains why the skip is not self-healing and why failing is the wrong lever, and a test renders the UI with *every* user unresolvable so a reintroduced `fail` fails the suite. **Scope this narrows, deliberately:** tenant buckets keep the pre-existing silent skip. The gate owns only the platform bucket, which is the cluster-breaking path. Generalising the repair to every bucket needs a controller that owns tenant buckets and is a separate change. Two more corrections from the same review: - `cozystack_backup_default_objects_missing` was only written on the happy path, so the state this PR exists to catch reported `0` and the documented alert never fired. The credentials Secret now counts in `missing`, so an absent or rotated-away Secret moves the gauge. On an API error the gauge is deliberately left alone rather than flapping, and a new `cozystack_backup_default_objects_check_errors_total` marks it stale; the doc pairs the two instead of overstating the gauge. `force_reconciles_total` gained `namespace`/`name`, since two releases can now be forced. - `forceHelmRelease` does a point `Get` and skips a release with `spec.suspend: true`. helm-controller ignores both annotations while suspended (`cozyhr suspend` sets exactly that), so the gate was re-stamping every `MinForceInterval` for the whole suspension and climbing the force counter — which the runbook attributes to a render that is not producing the objects. The skip is logged and not counted. ### 3. Corrected comments and docs `_helpers.tpl`, `backupclass-default.yaml`, `values.yaml` and `docs/operations/backup-classes.md` all claimed the skip self-heals on the HelmRelease interval, and the doc suggested a `flux reconcile` that does not re-render. They now say it does not, and why. The two-release manual recovery for already-affected clusters is documented, including the easy-to-miss part: the credentials Secret is rendered by the `bucket-<name>-system` release, not by `bucket-<name>`. ### Manual recovery for already-affected clusters Only needed on a version without the gate. A plain `flux reconcile helmrelease` does **nothing** — it does not re-render. ```bash # 0. Confirm the bucket name is resolvable first — forcing before that # just re-runs the same empty lookup. kubectl -n tenant-root get bucketclaim bucket-cozy-backups -o jsonpath='{.status.bucketName}' # 1. The credentials Secret. Rendered by the -system release, NOT by # bucket-cozy-backups — easy to miss, and the projector (hence every # strategy, Velero, and migration 50) has no source without it. ts=$(date -u +%Y-%m-%dT%H:%M:%SZ) kubectl -n tenant-root annotate helmrelease bucket-cozy-backups-system \ reconcile.fluxcd.io/forceAt="$ts" reconcile.fluxcd.io/requestedAt="$ts" --overwrite # 2. The Strategy CRs and the Velero BSL. ts=$(date -u +%Y-%m-%dT%H:%M:%SZ) kubectl -n cozy-backup-controller annotate helmrelease backupstrategy-controller \ reconcile.fluxcd.io/forceAt="$ts" reconcile.fluxcd.io/requestedAt="$ts" --overwrite ``` ### Validation - `go test ./internal/... -count=1` — pass, including 15 cases for the gate: force on missing, no-op in the steady state, throttling, unmapped kinds ignored, BSL skipped when disabled, patch failure surfaced without advancing the throttle, absent HelmRelease tolerated, plus force-on-absent-Secret, force-on-empty-bucket-name, external-S3 reports-but-does-not-force, independent throttles, suspend skip on each of the two releases, gauge written on the unresolved path, and errors counter on a failed check. - `make helm-unit-tests` — pass, with 2 new suites: 3 cases for the bucket chart (partial render preserved, and the UI still renders with every user unresolvable — the guard against reintroducing the `fail`), 7 for the gate's env + RBAC wiring. Those include that `.Release.Name` and `.Release.Namespace` match the HelmRelease the operator generates (`releaseName` becomes the HR's `metadata.name`, and no `spec.releaseName`/`targetNamespace` is set, so the Helm release name equals the HR name), that the bucket release coordinates track a renamed bucket together with `systemSecretName`, and that they are unwired on `provisionBucket: false`. - `make generate` — no diff. - `make go-unit-tests rd-presets-check test-check-readiness migrations-target-check` — pass. - `make bats-unit-tests` — every suite passes except `hack/migration-seaweedfs-db-adopt.bats`, which needs a Docker daemon not available in this environment ("docker is required: these tests run the migrations inside alpine"). Unrelated to this diff. **Not verified — please read this before approving.** There was still no live-cluster run. What that gap covers is smaller than it was: dropping the chart `fail` removed the universal behaviour change on every bucket's install path, so there is no longer a render-blind failure mode whose convergence has to be observed. What remains is one mechanism — `forceAt` + `requestedAt` on an unchanged release — applied to two releases instead of one, argued from read code rather than an observed install: `Strategy.Name = RetryOnFailure` on operator-generated HelmReleases (`internal/operator/package_reconciler.go`), and `bucketName = "bucket-" + claim.UID` in `packages/system/objectstorage-controller/images/objectstorage/patches/92-bucketclaim-propagate-ready.diff`. It is the same procedure as the manual recovery below, which has been run by hand on the reporter's cluster. Someone with a bootstrap cluster should still confirm that the gate fires within a minute or two of the bucket becoming ready, and that forcing `bucket-<name>-system` materialises the credentials Secret. ### Screenshots Not applicable: no UI change. This PR touches a controller, two system charts, and docs. <!-- 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. --> Walked file by file against the diff of this PR. **`website` — affected, follow-up issue opened.** `content/en/docs/next/operations/services/backup-classes.md` mirrors this repo's `docs/operations/backup-classes.md` and carries the exact stale claim this PR corrects, plus a `flux reconcile helmrelease` recovery suggestion that has no effect. `operations/configuration/platform-package.md` enumerates the forwarded `backupStorage` keys, and `reconcileDefaultObjects` is admin-settable through `spec.components.platform.values.backupStorage` (the whole block is deep-merged), so that list becomes incomplete. Filed as an issue rather than a PR because the wording depends on the shape this PR lands in, and whether to touch the frozen `v1.5`/`v1.6` copies is a maintainer call. No existing website issue or PR covered it. **`terraform-provider-cozystack` — not affected.** Checked, not assumed: `packages/apps/**` is untouched by this diff (`git show --stat HEAD -- packages/apps/` is empty), so no app `values.schema.json`, no `values.yaml` default, no version enum, and no `ApplicationDefinition` `kind`/`plural`/`release.prefix` changed. `reconcileDefaultObjects` is a key on a **system** chart, which has no `values.schema.json` and is not modelled by the provider. (An earlier revision added `requireUserCredentials` to the `bucket` system chart; that is gone, and no key was added to `apps/bucket`, so the app API is untouched.) The `<bucket>-<user>-credentials` Secret the provider looks up by literal name is **not renamed** — this PR only changes whether it gets created, which if anything removes a case where the provider silently read null from a Secret that never existed. Everything else: no `hack/` layout or make-target change (`ccp`, `external-apps-example`, `cozyhr`), no `ApplicationDefinition` CRD change, no namespace rename, no node prerequisites in `hack/e2e-prepare-cluster.bats` (`talm`, `ansible-cozystack`), no `cozyhr.cozystack.io/values-files` annotation or chart-source-kind change, no `service-proxy-name` / `wholeIP` / `allowICMP` rename (`cozy-proxy`), no telemetry metric or label rename (`cozystack-telemetry-server` — the two metrics here are new, not renamed), no node or network requirement change (`examples`). - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: https://github.com/cozystack/website/issues/639 - [ ] [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(backupstrategy-controller): the default backup Strategy CRs, the Velero BackupStorageLocation and the bucket user-credentials Secret were skipped permanently when their install-time Helm `lookup` came back empty, leaving clusters with a BackupClass but no strategies and blocking the v1.6.0 etcd migration. The controller now detects the missing objects and forces the Helm upgrade that creates them, including on the bucket release that renders the credentials Secret. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added automatic detection and recovery for missing default backup resources. - Missing resources trigger a throttled Helm upgrade once bucket configuration is available. - Added configuration to enable or disable default-object reconciliation. - Added validation requiring user credential Secrets during bucket installation by default, with an offline rendering option. - **Bug Fixes** - Improved recovery from incomplete backup and Velero storage configuration. - **Documentation** - Expanded operational guidance, monitoring metrics, alerts, and manual recovery steps. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-10 13:10:08 +03:00
Aleksei Sviridkin	587a7b9fbd	fix(ci): decide E2E coverage per changed path ... The full-suite escalation for a changed package that reaches no suite was read off the merged selection, so it depended on the whole diff rather than on the path that needed it. Any other changed path contributing a suite name made the selection non-empty and the escalation vanished with nothing in the output to record it: edit a platform component, adjust the Chainsaw test beside it, and the component ran that single suite instead of everything. Over one values.yaml per package under packages/{apps,system,extra}, 82 of the 159 escalate on their own and stop escalating once an unrelated suite edit joins the diff. Coverage is now decided per changed path. Each path resolves through its owning sources to the suites that exist, and a path reaching none of them forces the full run whatever the rest of the diff selected. The unit is the path rather than the source: system/postgres-operator belongs to two PackageSources where one reaches no suite, so deciding per source would escalate on that half and run everything for every change to it. The escalation names its cause on stderr, as every other escalation in this script already does. Most packages reach no suite of their own, so this is the branch a full run usually takes; without a line saying which sources bought it, "why did this run everything" has no answer in the log. The walk moves into resolve_suites() to be called once per path. POSIX sh has no `local`, so the function writes into the caller's scope and is safe only inside the $( ) its single call site wraps it in; a test pins that the call count stays one. Two entries leave src_to_suites. `postgres-application` restated what stripping the suffix produces and `external-dns` restated what carrying the source name through produces, so both made the exception list read as longer than the set of real exceptions. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-09 22:24:37 +03:00
Aleksei Sviridkin	0980e755eb	fix(ci): map E2E suites from sources not named after an app ... src_to_suites() covered PackageSource names ending in -application plus external-dns, and the filter after the graph walk dropped every other source it reached. cozystack.kuberture and cozystack.securitygroup- controller are neither, so both were walked to and then discarded: a CNI change ran neither of the two controller suites most likely to regress from it, and a change to either package reached no suite at all and escalated to the full run instead of selecting the one that covers it. Every reached source now maps through the table. That table is the inverse of select-install.sh's suite_to_source(), and a test walks every suite through both, so the next source name fitting neither convention fails there rather than quietly making its own suite unreachable. The cilium test measured the gap rather than hid it, asserting the 19 suites the selector could reach and naming the two it could not. The subtraction is removed rather than inverted: kept as an exclusion list it would stay green if the walk stopped reaching them for an unrelated reason. What replaces it is a second assertion against a tree holding one suite the graph reaches and one no source names, which an escalation cannot satisfy. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-09 21:28:00 +03:00
Aleksei Sviridkin	048088bd29	fix(e2e): bound the tenant failure diagnostics per read and as a phase (#3676) ... <!-- 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 node-join failure diagnostics in `hack/e2e-chainsaw/_lib/run-kubernetes.sh` issued about a dozen `kubectl` reads with neither an outer wall-clock bound nor `--request-timeout`, starting with a `describe nodes` through the tenant kubeconfig. That read targets the component least likely to answer in a node-join failure. An unbounded read there does not lose only itself: it holds the Chainsaw op until the op is killed, so everything scheduled after it is lost rather than truncated, the tenant crust-gather snapshot above all. Every read now carries the per-call bound the newer collectors on this path already use. A read that fails or is cut off says so instead of passing in silence, and says only what was observed, because a read that did not finish is silence about the cluster rather than a finding about it. The CDI importer walk gains a cap, and its listing is read on its own so a listing that never answered cannot read as a namespace with no importer in it. Per-read bounds alone do not end the phase, and the collectors here can together spend more than what is left of the op by the time they run. So the phase is bounded as a phase: once its budget is spent, the collectors that have not started are declined out loud. That is not a worst case and is not written as one, but it stops the snapshot being queued behind work begun past the deadline, which is how the snapshot was being lost. The op ceiling is unchanged, since a larger ceiling multiplied by a sum that grows with every collector added is the same problem one collector later. Because what runs last is what gets declined, the block is reordered so the cheap reads come first. The CSR list and the signer log are the discriminator for the failure mode a follow-up fix has to target, and behind two minute-scale collectors they were exactly what a tight run would drop. Adjacent work on the same failure path, all of it in service of the same requirement: - the sibling scheduling-gate branch ends in the same exit and depends on the same snapshot, so its two reads are bounded too; - `talos_image_cache_diagnose` gets its reads bounded and its gate gets `--ignore-not-found`, because `kubectl get` exits 1 for a refused connection or Unauthorized as readily as for NotFound, so a call that never reached the apiserver was being announced as a mirror that was never deployed; - the four numeric knobs are validated rather than only documented, since a suffix or a leading zero on the budget reached the arithmetic in the block's first statement and unwound the whole function before its own headline, leaving a failing run with no diagnostics at all. Comments carry invariants rather than arithmetic. A comment that states a sum has to be kept true forever in every copy of it, and any edit to the code or the order falsifies one of them. The numbers live in the code as values and in the guard that pins them. Relates to #3656. ### Known gaps, deliberately not closed here Some collectors on this path carry no wall-clock bound at all. The image-cache reachability re-probe makes seven unbounded management-cluster calls, and the guest-Talos capture walks its VMI rows uncapped with applies and waits bounded only by `--request-timeout`, which bounds one HTTP request rather than a client retrying against a wedged apiserver. The phase budget stops them being started late; it does not stop one running long once admitted. They are pre-existing and tracked in #3666. Bounding the re-probe in particular would change when the same function gives up on the happy path, and so which image factory tenant workers use, which is not a diagnostic's decision to make. Named improvements left for later, none of them regressions introduced here: - the phase deadline is stamped at block entry rather than against what is left of the op, so a long bringup still narrows the room the derivation assumes; - `bringup` and `largest` are literals in the guard, so a collector heavier than today's will not move them, and the margin is small by design; - the integer-knob validator now exists in four places in this tree, and a shared `_lib` file sourced ahead of both libraries would collapse two of them; - the per-request bound equals the wall-clock bound, so both deadlines fire together and which of the two notes appears is a race; - the ordered list in the chainsaw comments omits the ungated wedge check, which runs first; - section letters no longer sort after the reorder, and the comment-to-code ratio in the touched block is high. ### Testing `hack/run-kubernetes-node-join_test.bats` is new: 22 cases driving the block with a `kubectl` stub that hangs, refuses and answers in part, and a `timeout` stub that fails any read not wrapped in the bounding idiom. It pins that no read escapes a bound, that a cut-off read is named rather than reported as an empty cluster, that the walk cap reports both counts, that a failed listing is not reported as no importers, that the phase declines the rest out loud, that a malformed knob falls back and says so, that the block returns zero so the caller keeps its `exit 1`, and the source order that keeps the discriminating reads ahead of the expensive collectors. Every one was checked by mutation against the unfixed shape. ### 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(e2e): bound every read in the tenant Kubernetes node-join failure diagnostics and give the collection phase its own wall-clock budget, so a wedged apiserver can no longer hold the Chainsaw op until it is killed and take the tenant crust-gather snapshot with it ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved Kubernetes node-join and scheduling failure diagnostics with bounded collection times, clearer timeout and failure reporting, and more reliable preservation of critical snapshots. - Added safer handling for unavailable diagnostic data, missing tools, failed commands, and undeployed image caches. - Added configurable and validated timeout controls for image-cache and failure diagnostics. - **Documentation** - Clarified diagnostic budgets, collection ordering, and behavior when available time is exhausted. - **Tests** - Added comprehensive coverage for diagnostic timeouts, ordering, validation, failure handling, and bounded reads. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-09 12:33:27 +03:00
Aleksei Sviridkin	86fb92abf5	docs(tests): correct five stale claims in the e2e capture tooling (#3662) ... <!-- 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 Five things in the e2e tooling said something the code no longer does, or did not say it at all. All five are prose; no executable line changes. `docs/agents/e2e-testing.md` section 8 described two of the three on-failure capture legs. The missing one is `hack/e2e-capture-dataplane.sh`, the only leg that reads forwarding state off the node rather than object state, and the only one whose two callers bound it differently, 600s from the BATS runner and 300s from the Chainsaw catch. That split is what decides whether something added to it fits, so a contributor who reads the document and not the script sizes a change against the wrong budget. Its notes contract is described with the carve-out the script declares itself: the two EndpointSlice reads in the LoadBalancer path report nothing either way, so a `<none>` backend line there is the one place a reader still cannot tell an endpointless Service from a read that never answered. The same section said the runner's captures serve "the BATS tests" in two places. They serve the e2e ones. The runner keys on an `e2e-` prefix, on the suite's own filename or on its parent directory, because all three legs read cluster state and a unit runner has `kubectl` on PATH with no cluster behind it. The legs are not gated alike either, so the sentence says which fired where: two key on `kubectl`, and the snapshot leg keys on `crust-gather` being installed, which on a unit runner it usually is not. Two numbers in the capture-gate rationale in `hack/cozytest.sh` no longer described the code. The snapshot leg's cost was given as 360s, the deadline passed to `timeout`; the leg is invoked as `timeout -k 30 360`, so the trap waits out 390s, which is what the comment forty lines below and the op-budget arithmetic in `.chainsaw.yaml` already said. And the data-plane leg was still described as having no bound on its first pod list, and billed at the full 600s on that basis, after that read was given a 28s ceiling. Two files enumerated the BATS suites and both stopped one short. The global catch in `hack/e2e-chainsaw/.chainsaw.yaml` said the runner it names as its analog "only covers the BATS install/openapi tests", and `hack/e2e-chainsaw/README.md` named the same two files. Both miss `hack/e2e-prepare-cluster.bats`, which `packages/core/testing/Makefile` drives through that runner, and which is the suite whose failures leave the sandbox in the state hardest to reconstruct afterwards. `docs/agents/e2e-testing.md` already listed all three, so the tree disagreed with itself depending on which file you opened. A comment in `hack/e2e-install-cozystack.bats` pointed at `hack/e2e-apps/run-kubernetes.sh`, which does not exist; that directory holds two monitoring suites and no runner. The readiness gate it means is the `rollout status` wait in `hack/e2e-chainsaw/_lib/talos-image-cache.sh`, along with both fallbacks to the public factory. relates to #3626 and #3642 ### 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 NONE ```	2026-08-09 04:08:35 +03:00
Aleksei Sviridkin	06078359d0	docs(e2e): record the phase-budget pattern beside its rule ... The e2e guide tells a collector author to size the op timeout above the sum of the inner budgets and let the inner ones fire. That works while the sum is something an op timeout can sit above, and on the tenant-Kubernetes node-join path it no longer is: the terms run past the 50m op on their own, and several of the reads there carry only --request-timeout, which bounds one HTTP request rather than a client retrying against a wedged apiserver, so parts of that path have no ceiling to add up at all. Left as it stood, the next collector author follows the documented rule and adds a bounded read to a sum that already does not fit. So the phase-budget pattern goes next to it, with the two properties that are easy to get wrong: the budget bounds when a collector may start rather than when the phase ends, so it is not a worst case and the comment beside it must not claim to be one; and because what runs last is what gets declined, source order decides which evidence survives a tight run. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-09 02:51:06 +03:00
Aleksei Sviridkin	9a0c8c75f3	fix(ci): close the silent-green gaps in e2e selection and the helm unit sweep (#3670) ... ## What this PR does Four fixes in e2e test selection and the helm unit sweep. Relates to #3650 #3652 #3655 #3646. select-e2e built both dependency indexes as `$(build_owners_index | sort -u)`, so it read sort's exit status and never yq's (#3650). A broken yq left the index empty, every path fell through to escalation, and CI ran the full suite on every PR. The output was identical to "no owners matched", so it looked like the conservatism the selector is supposed to apply. The suite list one line up had the same shape and a worse result. `$(find ... | sed | sort)` reports sort, never find, and errors went to /dev/null. Every escalation prints that list, so a failed find made "escalate to the full suite" emit a blank line, which both e2e lanes read as "skip Chainsaw" before posting `E2E Tests` green. The outcome that means run everything delivered a run of nothing. find is not the only way to get an empty list, and the escalations are not its only reader, so the check sits where the list is built. The `examples/backups/<app>/` rule escalates nothing and still membership-tests against that list: with it empty, an edit to a backup harness that should run its suite reports nothing to run, on a path no escalation-side guard would ever see. An empty list is a broken enumeration, so the script refuses to answer at all. Both lanes run the step under `bash -e`, so the non-zero exit fails the job. I checked both workflows: neither overrides `shell:`, both capture the selector as a plain assignment. Fourteen tests used `[ "$(echo "$output" | wc -w)" -gt 5 ]` to mean "everything was selected" (#3652). There are 21 suites, so that passes on any six, and partial escalation stayed green. They compare against the whole list now, derived the way the script's own full-suite branch derives it. Drop one suite from that branch and thirteen of them go red. All thirteen passed under the threshold. The fourteenth is the cilium case, and equality shows it was never a full-suite path at all. It resolves through the dependency graph, which reaches 19 of the 21 suites. The two it misses are kuberture and securitygroup, whose sources are not `*-application`, and the mapping covers only those plus external-dns by name. Both controllers declare `cozystack.networking`, so a CNI change that breaks either one runs neither suite. That gap is #3665. The test is renamed for what it measures and pins the 19, so closing #3665 turns it red at the right moment. Editing `hack/e2e-chainsaw/backup/chainsaw-test.yaml.disabled` selected the full suite (#3655). The suffix was ignored when deriving the suite name, the intersection against existing suites emptied, and the empty-selection safety net escalated. A disabled suite runs nowhere, so its edits are the one file class that cannot regress a test, and they were buying the most expensive outcome the selector has. The selector now has a fourth outcome, a failed step rather than a selection, so the contract enumerations in the script header, in `docs/agents/e2e-testing.md` and in both workflow comments say so. The helm unit sweep never visited `packages/tests` (#3646). cozy-lib-tests, the whole test coverage for a helper library that 34 charts consume by symlink, ran only because `packages/library/cozy-lib/Makefile` delegates its `test` target to it. Remove that one line and the suite goes from one execution to zero with the sweep still exiting 0 and still printing "All Helm unit tests passed". Every check the sweep makes runs inside a directory the loop already reached, so a package that never enters the loop takes all of them with it, and what is left is `tests_found`, which counts suites across the whole run rather than per package. The directory is on the list now: 76 executed suites become 77, at 0.1s to 0.3s over six runs against a sweep of roughly 31 seconds. The delegation stays for `make -C packages/library/cozy-lib test` ergonomics, so the suite runs twice. Neither run is load-bearing alone. ### Testing `hack/select-e2e_test.bats` goes from 27 tests to 32, `hack/helm-unit-tests.bats` is new. Each commit is green on its own: 27, 30, 32, 32. The new tests stub the failing component and assert the exit status plus the line naming what broke: a broken yq, a broken find, an empty suite list across all three diff classes, the disabled-suite path, and a guard that the disabled rule drops only that path and not the rest of the diff. The yq test asserts exit 0 on purpose. A missing dependency graph still has a correct answer, the full suite, and a missing suite list does not. Pinning both halves keeps the header and the docs from drifting into claiming otherwise. Two suites fail on my macOS box and fail identically at `origin/main`, so neither comes from this branch: `hack/migration-seaweedfs-db-adopt.bats`, where a container running as the host UID cannot write the bind-mounted temp dir under Colima, and `hack/seaweedfs-naming-audit.bats`, which I confirmed red at the merge base but did not dig into. Every other `hack/*.bats` was run individually and is green. `make unit-tests` stops at the first failing file and both of those sort ahead of `select-e2e_test.bats`, so on macOS the suites this PR touches have to be run directly. ### Named improvements, not done here - The `.disabled` rule keys on the file rather than the parked directory, so it is both wider and narrower than the property it wants. A support file under `hack/e2e-chainsaw/backup/` still buys a full run, and a `.disabled` file dropped inside a live suite directory now selects nothing where it used to select that suite. The property covering both is one level up: a suite directory holding no live `chainsaw-test.yaml` is inert, testable with the same `grep -Fxq` membership check the `examples/backups/*/*` arm already uses. It is not a free win, since a half-written suite directory escalates today and would then select nothing. A cheaper half step is narrowing the glob to `hack/e2e-chainsaw/*/chainsaw-test.yaml*.disabled`, because case globs cross `/` and the shipped pattern also matches nested paths. - `packages/tests/**` is not classified by the selector and reaches the full suite through the unclassified fall-through instead of a rule. Tracked in #3667. - `full_suite_list()` in the bats file cannot fail. cozytest.sh's awk rewrites a lone `}` into `return 0; }`, so a broken find inside the helper would return an empty expected set instead of a red test. Nothing false can pass today, because the script hard-fails on an empty suite list before any assert runs, but that helper is what every equality assert measures against. - The yq check runs before the changed-file list is read, so with a broken graph even a docs-only diff escalates. Measured: a broken yq plus a `docs/x.md` diff returns 21 suites at head against 0 at the merge base. Only reachable in an already broken state, but the graph is consulted at the component lookup and in the closure walk, so moving the check to first use would charge nothing to docs, dashboards and inert-meta diffs. ### Screenshots No UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` file by file against the diff: four files under `hack/`, one under `docs/agents/`, and comments in the two e2e workflows. Nothing under `hack/` was moved or renamed and no make-target contract changed, so the ccp entry does not fire. It gates on `hack/package.mk` and `hack/common-envs.mk`, neither of which is in the diff. The node prerequisites in `hack/e2e-prepare-cluster.bats` that ansible-cozystack mirrors by hand are untouched, and so is `.github/workflows/tags.yaml`. `docs/agents/` is internal contributor documentation, not a website source. No package was added, renamed or removed, and no `values.schema.json` or version enum changed, so the website and provider entries do not fire either. - [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(ci): select-e2e no longer reports an empty selection when it cannot enumerate the Chainsaw suites, which made an escalation to the full suite indistinguishable from "skip Chainsaw" and left the E2E gate green with nothing run. A failure to list the suites is now named on stderr and fails the step, a yq failure is named and still escalates to the full suite, and an edit to a disabled Chainsaw suite is inert instead of escalating to a full run. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved end-to-end test selection for disabled, empty, or partially changed test suites. * Test selection now safely escalates to the full suite when suite discovery or dependency analysis fails. * Helm unit-test sweeps now include tests under `packages/tests`. * **Tests** * Added coverage for suite discovery failures, disabled suites, command failures, and exact test selection. * Added Helm test-sweep coverage, including failure handling. * **Documentation** * Clarified test-impact analysis rules and suite-selection outcomes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 20:47:06 +03:00
Aleksei Sviridkin	355fc8a24a	test(tests): let each bats file declare the EXIT traps it still holds (#3622) ... <!-- 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 ban on EXIT-trap cleanup in `hack/*.bats` was enforced by two lists inside `hack/cozyreport.bats`: one naming the files known to be clean, one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard kept failing. It is worth being exact about how badly, because the history is sharper than "it went stale a few times". **The inventory was never correct on main for a single commit.** It landed in #3567 at 15:25:20. #3195 had landed `multus-install-cni-plugins.bats` carrying twelve traps at 15:24:37, forty-three seconds earlier. Replaying the old guard's own logic against the tree at the very commit that introduced it already gives `found != frozen`. Main then stayed red for roughly twenty-two and three quarter hours. The first repair, #3584, landed *already red* the next morning at 10:42:43, because #3548 had brought in `run-kubernetes-talos-diagnostics_test.bats` with eight traps at 10:40:03, under three minutes ahead of it. That fix was correct and bought zero green time. Green arrived only with the second repair, #3602. Neither pair of PRs shared a line, and each was green against its own base. That is the whole mechanism, and it is why a third one-line repair is not the answer. The mechanism is structural rather than careless. A change that adds a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second one landed. A file *arriving* with traps was worse still, because nothing in its author's diff pointed at that string at all. That is exactly how `run-kubernetes-talos-diagnostics_test.bats` got in, twice. That contention is not in the past tense. Two open PRs are editing that one line right now, and they disagree about what it should say: #3575 adds `run-kubernetes-talos-diagnostics_test.bats=8` to it, repeating a repair that has already landed, and #3441 removes `select-e2e_test.bats=15` from it, because it converts that file. Neither PR is about EXIT traps. Both have to touch that string anyway, and whichever lands second is wrong until someone edits it again. So this PR is not fixing a red main. It removes the thing that keeps making main red, which is why it is worth more than the one-line fix that is now the established habit. One honest caveat about its own landing. The textual-conflict property is steady-state: a branch forked *before* the declaration exists has no line to disagree with, so it converts traps in its own file, merges clean, and the count only goes wrong once both sides are on main. I checked this against #3441, which takes `select-e2e_test.bats` from fifteen traps to zero. Merged after this, that file would declare fifteen and hold none. What the move buys even in that case is that the red names a file the branch already edited, the repair is one line inside it, and rebasing before merge catches it on the branch's own CI. None of those three held against the central inventory. Whoever merges this should expect one such adjustment on the conversion branches still in flight. So this replaces both lists with a declaration each file makes about itself: one `# EXIT-TRAP DEBT: N` comment in its leading comment block. A file carrying no declaration must install no EXIT trap. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count get a real textual conflict instead of silently invalidating each other, and a change that leaves the traps alone edits nothing. **The include list is redundant, not lost.** It named the files proven clean, so that a trap reappearing in one of them would fail. Under the new rule those files carry no declaration, and a file with no declaration must hold zero traps, so a trap reappearing in any of them fails on its own, with no list to be on. Coverage widens rather than narrows: the two lists named twenty files between them, and the rule covers all fifty bats files under `hack/`, subdirectories included, plus the ones added tomorrow. Rebasing this branch onto current main is the property working. Main has since gained `hack/kubernetes-pre-delete-hook.bats` and `hack/tenant-pre-delete-hook.bats`, and `hack/cozyreport.bats` grew by some eight hundred lines. Neither new file installs an EXIT trap, so neither needed a declaration and neither needed an edit here; the rebase took no conflict at all. Under the inventory, each arriving file was a coin toss on whether somebody had remembered the string. To be precise about what the inventory could and could not do, since it is easy to overstate: it did *notice* a new file carrying traps. The string it compared was built by scanning the directory, so an arriving file appended a token and failed the comparison, which is exactly how main went red. What it could not do is let that file arrive without an edit in a foreign suite. Being seen and being absorbable are different properties, and only the second one decides whether two changes can land independently. The declaration is pinned in both directions. Declaring N while holding N+1 fails, obviously; declaring N while holding N−1 fails too. Without that second half the number becomes a ceiling and rots upward: somebody converts half a file, the declaration stays, and the guard quietly licenses traps that were removed long ago. It is read only from the leading comment block, under the shebang and above the first line of code. A `.bats` file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test; honouring it there would let an unrelated fixture excuse a real trap, silently, with nothing in that test's own diff looking like a declaration. A comment block is the region with no interior; stopping instead at the first `@test` would still read a line out of a helper's heredoc. Not every counted handler is debt. A trap inside an explicit subshell does not replace the one the `bats` binary installs, so a test failing inside `( … )` still prints its `not ok`, checked against a test-level trap in the same file, where the TAP line vanishes. `hack/e2e-test-openapi.bats` kills a backgrounded `kubectl proxy` exactly that way, and "convert it like the others" would leak a process holding a fixed port and wedge the next run. Its declaration now records the carve-out instead of scheduling a conversion, and because the ratchet is exact in both directions, *removing* that trap fails too, so the count protects the construct rather than marking it for deletion. `docs/agents/e2e-testing.md` previously scoped this exception to Chainsaw `script` steps only; it now names the BATS subshell case as well. The counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed `trap … EXIT; cd "$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap … EXIT` and `(trap … EXIT; true)`; upper case missed `trap … exit`, which bash and dash both install. All of those are real handlers that scored zero. The left boundary is the one worth dwelling on, because the inventory being replaced had none at all and *did* catch the semicolon form. Getting it wrong here would have narrowed coverage while the commit claimed to widen it. A plain word boundary is not enough either: `bootstrap ` ends in `trap `, and it has to keep scoring nothing, or the documented answer to a red guard (add a debt line) would buy a file a permanent licence for one real trap to silence a line that has none. Two handlers sharing one line are reported rather than counted, because the count is a count of lines and the second would otherwise arrive free. Splitting such a line properly needs a shell parser, since a semicolon inside a handler's own quoted action is not a separator, and guessing wrong undercounts, the one direction a ratchet cannot afford. **What this does not fix.** The declaration is still a loophole: a new file can write `# EXIT-TRAP DEBT: 8` instead of cleaning up, and nothing here makes that impossible. What changes is that the admission is local and visible. It sits at the top of the file it excuses, in front of whoever reviews that file, instead of being a number in a neighbouring suite nobody in that review is reading. Today's loophole is the same size and invisible. Three more limits, all stated in the guard's own header rather than left to be discovered. The scan is lexical, so a signal computed at runtime and a quoted action spanning physical lines without a backslash are both invisible. An exact count catches addition and removal but never substitution: swap the openapi file's subshell trap for a test-level one and the total stays 1. And the scan reads `.bats` only, so a handler arriving through a sourced `.sh` is outside it. `hack/e2e-chainsaw/_lib/run-kubernetes.sh` installs two right now, and each is benign for its own reason rather than by design: the one in `cozy_capture_tenant_talos` because that function is declared with `(` and so runs in a subshell, the one in `run_kubernetes_test` because no `@test` calls it despite being declared with `{`. Three `hack/*.bats` source that library, and two tests in the converted file call `cozy_capture_tenant_talos`, so flipping a single `(` to `{` reinstates a test-level handler in both of them with the guard green. Widening the scan to `.sh` would mean counting handlers that are correct in a script and wrong only in a test body, so the honest answer is that this is where the instrument stops. Three further boundaries, recorded here so they land as known edges rather than as surprises. The old include list also failed when a file named on it disappeared from the tree; the new rule can only judge a file that is present, so a deleted converted file goes unnoticed. That is a genuinely smaller check, though its absence shows up in the diff that deletes the file. The guard's own failure messages are code lines, so they are scanned by the pattern they belong to: they pass today only because no bare `EXIT` or `0` happens to follow the keyword in any of them, and a rewording that introduced one would make the file demand a debt of itself. It fails loudly rather than quietly, and the fixture writers and test titles already split the keyword from the signal for this reason, but the messages do not. Finally, `docs/agents/e2e-testing.md` bans test-level `EXIT` *and* `RETURN` traps, while every mechanical guard this repo has had, the one being deleted included, matches only `EXIT` and `0`. `hack/` holds no RETURN trap today, so nothing regresses here, but half of that documented rule has never had an executor. The guard moves out of `hack/cozyreport.bats` into `hack/bats-no-exit-trap.bats`, because its subject is every unit suite under `hack/` and not the report collector it grew up in. Living inside `cozyreport.bats` is precisely why unrelated PRs kept converging on one line. `hack/md-no-hardwrap.bats` is the neighbouring precedent. Separately, the eight EXIT traps in `hack/run-kubernetes-talos-diagnostics_test.bats` are converted to end-of-body cleanup rather than given a declaration. That file was written six weeks after the ban was documented, so "it predates the rule" is not available to it, and handing it a debt is literally the move that failed the last two times. The remaining files keep their declarations, since a mass conversion would collide with the branches that own them. One note on where the prose sits. The guard's own header carries only what a reader has to keep true: why the ban exists, the two carve-outs, where the declaration lives and why there, that the count is exact in both directions, why the marker is anchored at column zero, and where the lexical scan stops. Everything above about *how* the inventory failed (the timestamps, the PR numbers, the arithmetic) is deliberately not in the file. It is true today and will rot on its own; a comment nobody is obliged to maintain is how a file grows a paragraph that is quietly false. ### 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(tests): each hack/*.bats file now declares its own remaining EXIT-trap debt in a `# EXIT-TRAP DEBT: N` header comment, checked by hack/bats-no-exit-trap.bats, replacing the central inventory in hack/cozyreport.bats ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Testing** - Added automated auditing for `EXIT` traps across Bats end-to-end tests, including validation of tracking declarations and edge cases. - Improved diagnostics tests by replacing trap-based temporary-directory cleanup with explicit cleanup steps. - Added tracking annotations for remaining trap-related cleanup work. - **Documentation** - Clarified when traps are permitted inside self-contained subshells and how remaining cleanup debt is reported. - Updated review guidance for consistent end-to-end test maintenance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 20:46:49 +03:00
Aleksei Sviridkin	1abc653cfb	fix(ci): treat a disabled chainsaw suite as inert in test selection ... Editing hack/e2e-chainsaw/backup/chainsaw-test.yaml.disabled selects the whole Chainsaw suite. The per-suite rule reads the suite name off the directory and ignores the suffix, so the change selects "backup"; the final intersection against the suites that actually exist finds no such suite and empties; and the safety net for an empty selection escalates to the full run. A disabled suite is registered nowhere and executed by nothing, which makes its edits the one file class that provably cannot regress a test. The mapping had it backwards: they bought the most expensive outcome the selector has. Rare in practice, and paid on every commit by anyone grooming or re-enabling such a suite. Classify *.disabled as inert ahead of the per-suite rule, and record it alongside the other rules that select nothing so the header stays an accurate enumeration. The safety net keeps its job for paths no rule has looked at, which is what it was added for. Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 16:31:39 +03:00
Aleksei Sviridkin	37a395363f	fix(ci): stop select-e2e from deciding on inputs it never checked ... Three holes on the same seam: the script builds a list, never looks at whether the command that built it succeeded, and then decides from the empty result. The dependency indexes are built as `$(build_owners_index | sort -u)`, where the function is a single yq. A pipeline carries the status of its last command, so the script sees sort's zero and `set -e` never fires: a missing binary or a malformed source leaves the index empty, every changed path falls through to escalation, and CI runs the full Chainsaw suite on every pull request. The direction is safe and that is the problem — an empty index and "no owners matched" print the same thing, so the symptom is a full run, which reads as the conservatism the selector is supposed to apply. Selection can stay switched off indefinitely with nobody signalled. Check each half's status on its own and name yq once on stderr before taking the fallback the empty index would have reached anyway. The suite list one line up has the identical shape and a worse consequence: `$(find ... | sed | sort)` reports sort, never find, and errors went to /dev/null. That list is what every escalation prints. So a find that failed produced an empty list, and "escalate to the full suite" then emitted a blank line — which both e2e lanes read as "skip Chainsaw" before posting the required "E2E Tests" status green. The one outcome that means run everything delivered a run of nothing, on the pull request that earned a full run. Capture find on its own, check it, let its errors through. A failing find is not the only way to end up with an empty list, and the escalations are not its only reader, so the check sits once where the list is built. The examples/backups/<app>/ rule escalates nothing and still membership-tests against that list: with it empty the test fails, the app is not selected, and an edit to a backup harness that should run its suite reports nothing to run — output identical to a legitimately empty selection, on a path no escalation guard would ever see. An empty list is a broken enumeration, a moved directory or a wrong working directory, not a project without tests. Refuse to answer at all. Both lanes run the step under `bash -e`, so a non-zero exit fails the job rather than falling through to the empty selection this is here to prevent. Each is covered by a test that stubs the failing component and asserts both the exit status and that the script names what broke. Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 16:17:31 +03:00
Aleksei Sviridkin	5728eca401	docs(e2e): name the data-plane collector and who gets the captures ... Section 8 described two of the three on-failure capture legs and not the third, so the collector that reads forwarding state off the node was absent from the one document that tells a contributor what diagnostics exist. It is also the leg whose two callers disagree about its budget, 600s from the BATS runner and 300s from the Chainsaw catch, which is what decides whether something added to it fits. Its notes contract is described with the carve-out the script itself declares: the two EndpointSlice reads in the LoadBalancer path report nothing either way, so a <none> backend line there is the one place a reader still cannot tell an endpointless Service from a read that never answered. Stating the contract without it would have promised the distinction on the path this collector exists to diagnose. The prose also still said the runner's captures serve "the BATS tests". They serve the e2e ones: the runner keys on an e2e- prefix, on the suite's filename or on its parent directory, because all three legs read cluster state and a unit runner has kubectl on PATH with no cluster behind it. Said once where the captures are introduced, and the two phrases that over-claimed now name the e2e suites. The legs are not gated alike, so the sentence says which fired where: two key on kubectl, and the snapshot leg keys on crust-gather being installed, which on a unit runner it usually is not. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 11:21:33 +03:00
Aleksei Sviridkin	8e273a7341	docs(ci): record what escalating on the non-PR workflows costs ... Three of the four workflows on the escalation list cannot be exercised by the run they trigger. `e2e-fork` runs on `workflow_run`, which always takes the default-branch copy; `e2e-tag` runs on `workflow_call` from a tag push; `nightly` runs on a schedule. Only `pull-requests.yaml` is executed from the pull request's own head, so only there does escalating test the edit rather than the tree around it. That argument existed in the reference doc and was removed by this branch rather than answered, and the list it argued against is what replaced it. Measured over six months of main, 26 commits touch one of the three with no other escalating path; each now pays a full Chainsaw run it used to skip, on a suite whose flake rate is the reason people avoid running it for nothing. Keeping them is defensible: the rule stays one idea rather than two, and dropping them leaves those files inert, which reads as an oversight rather than a decision. What was not defensible was paying it silently. The trade now sits next to the pattern that makes it, with the number, so the next person weighing it starts from measurement instead of rediscovering the mechanism. The reference doc points there rather than carrying its own copy. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 05:55:37 +03:00
Aleksei Sviridkin	55f3c79e9f	docs(ci): stop pointing at the inert list for paths it does not hold ... Four places said an empty selection means the inert pattern matched, and sent the reader to `inert_config_pattern` to see which paths those are. Documentation, dashboards and `*.md` are dropped before that pattern is ever consulted, and a backups example whose app has no suite is dropped where backup examples are matched. A reader checking the pattern for any of them finds nothing and has to reconstruct the order by reading the script. Name the rules that produce an empty selection instead, and keep the claim to the one the lanes rely on: empty is a decision some rule reached, not a path nothing looked at. The same bullet claimed every path is classified. One is not. The inert pattern makes `.github/` inert as a whole directory and the workflows that run the suite are exempted from it by name, so a workflow added under `.github/workflows/` reads as inert and the fall-through never sees it. Today's enumeration is correct and I checked it against `rg -l test-chainsaw`, so this is not a live hole, but the sentence saying it cannot exist is what makes the reminder next to that list look decorative. Record it as the guard it is. Separately, the fork lane's rationale for appending a newline described a mechanism this branch removes: the reader now guards its own loop, so the terminator is no longer what keeps the last path alive. Say that it is now the text-file contract and a second line of defence across two files, which is a reason to keep it and a different reason than the one written there. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 05:02:25 +03:00
Myasnikov Daniil	ae4e9e55cb	fix(ci): classify every TIA path and escalate the unclassified ... `select-e2e.sh` silently selected nothing for a changed path it did not recognise outside `packages/`. Both e2e lanes read an empty selection as "skip Chainsaw" and then post the required "E2E Tests" status green, so an unrecognised path produced a green merge gate with no suite run. The worked example is `hack/common-envs.mk`, which supplies the tag, push and output flags of every image in the tree and matched nothing: `full_suite_pattern` covered `hack/*.sh` and `hack/*.bats` but not `hack/*.mk`. `pkg/`, `tools/`, `go.mod`, `hack/lib/` and `hack/buildkitd.toml` were in the same position, as was `.github/workflows/e2e-fork.yaml` while `pull-requests.yaml` was already escalated. A blanket "empty means full suite" fallback would be the wrong fix: it escalates the inert paths the design deliberately skips. Instead every path now lands in one of three classes by an explicit rule — the full-suite list for shared build inputs and the e2e harness, an inert list for paths that cannot affect what e2e exercises, and the PackageSource graph for `packages/(apps|system|extra)/`. A path matching none of them escalates and logs why, so the next path someone adds fails safe instead of failing open. The two lists are checked in that order, so a specific escalation beats a broad inert directory: `.github/` is inert, the workflows that run the suite are not. `*.md` and `docs/`/`dashboards/` stay ahead of both, so a README under `packages/core/` does not inherit that tree's escalation. Ten tests cover both sides of the classification. Mutation-checked: reverting the fall-through fails the unclassified-path test, dropping `.github/` from the inert list fails the non-e2e-workflow test, and swapping the two list checks fails the existing pull-requests.yaml test. Note the per-entry escalations are belt-and-braces — removing `hack/*.mk` from the pattern still escalates via the fall-through, which is the point of having it. Closes #3392 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-08 04:28:27 +03:00
myasnikovdaniil	7dca642894	fix(ci): restore e2e for fork PRs and close the merge-gate bypass (#3262) ... ## What this PR does Fixes #3257 — fork PRs never ran e2e, and the skipped required check let them merge anyway. Fork PRs get no secrets on `pull_request`, so `make image` pushed anonymously and died with `denied`. That failed every build, collapsed the `build → finalize → e2e` chain, and left the required **E2E Tests** job *skipped* — which branch protection treats as satisfied, so an approved fork PR could merge with e2e never having run. This implements **Option A** from the issue (privileged `workflow_run` split), pushing fork e2e images to the existing CI registry (OCIR): **Build primitive (`hack/common-envs.mk`)** - New `OCI_EXPORT_DIR` mode: build each image to a per-image OCI archive instead of pushing, and force `PUSH`/`LOAD` off. The digest is captured via `--metadata-file` regardless of output type, so the refs baked into `pr.patch` match what the privileged run later pushes. Threaded through the `image-tags` macro every package already uses — no per-package Makefile change. **`pull-requests.yaml` (unprivileged, fork branch)** - `build` / `build-talos` / `finalize` export images to OCI archives and upload them as artifacts instead of pushing → the red `denied` build wall disappears and the chain no longer collapses. - The in-tree e2e job is renamed off `"E2E Tests"` and guarded to same-repo PRs. A new `e2e-report` job concludes the required `"E2E Tests"` context as an **explicit check-run** (success on a docs-only PR or when e2e passed, failure otherwise). A skipped job can no longer satisfy the required check — this also closes the same hole for same-repo PRs whose build fails. **`e2e-fork.yaml` (new, privileged `workflow_run` from the default branch)** - Resolves the fork PR, opens a pending `"E2E Tests"` check-run, and fails-closed if the fork build failed. - Pushes the pre-built image archives to OCIR **by digest** and flux-pushes the digest-pinned packages tree. The OCIR token is live only for trusted binaries (`git`/`skopeo`/`flux`/`yq`) over data — never for `make` or any fork-authored script. - Runs the e2e suite against the pushed closure with **no push token** and concludes the check-run on the PR head SHA. Fork build code never receives registry credentials; the fork's images and test scripts run only in the credential-less, ephemeral e2e job — a stronger posture than a `pull_request_target` run that would hand fork code the secrets directly. ### Validation Local: buildx OCI-archive output + digest capture, macro expansion for single- and multi-image packages, non-fork push path unchanged, `actionlint` clean. Needs live validation (why this is a **draft**): `workflow_run` artifact download across runs; that the `"E2E Tests"` commit status satisfies the required context and keeps a fork PR non-mergeable until the privileged run concludes it; `refs/pull/<N>/merge` fetch + `git apply --3way`; `skopeo --preserve-digests` end-to-end. `workflow_run` only runs from the default branch, so the privileged half is exercised by mirroring the workflow onto a personal fork's default branch and opening a throwaway fork PR into it — not by merging first. _Since review_: the fork `e2e` job ran the pre-#2826 BATS app loop that the merged Chainsaw migration deleted (already broken); it now runs the same Chainsaw + TIA path as the in-tree job, with suite selection and the `full-e2e` override sourced from `resolve`'s trusted base-repo file list / labels (not a merge-ref diff). Publish job hardened per the non-blocking follow-ups: base-tree image allowlist, `skopeo copy --all`, pinned flux CLI. The required `E2E Tests` gate is now published as a commit **status** (default token + `statuses:write`), not a COZYSTACK_CI app check-run — the app lacked `checks:write` (`checks.create` 403'd) and a default-token check-run floated under a labeler suite; a commit status has neither problem, and forks can't post one (their `pull_request` token is read-only). Two review-bot findings fixed too: fork `OCI_EXPORT_DIR` is now absolute (a relative path misplaced archives under `make -C <pkg>` and every fork build would fail at upload), and the "≥1 archive" guard no longer no-ops under `nullglob`. ### Repo-admin prerequisites One repository setting lives outside this diff: 1. **Branch protection: keep `E2E Tests` required.** The gate is now a commit **status** named `E2E Tests` (posted with the default `GITHUB_TOKEN` — see `docs/agents/e2e-testing.md` §10), not a job or an app check-run. Branch protection already requires `E2E Tests` (currently the e2e job's name, renamed here to `E2E (in-tree)`), so the required context now resolves to the status — **no GitHub App, no `checks:write` grant, no app-id pin needed**. Just don't pin the context to a specific app. 2. **Actions → "Require approval for all outside collaborators".** Each fork run must be a deliberate maintainer click before it executes, since the fork's unprivileged run produces the artifacts the privileged `e2e-fork.yaml` consumes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff (7 files: two CI workflows, `docs/agents/e2e-testing.md`, `hack/common-envs.mk` + its bats suite, and two package `Makefile`s). The one trigger worth naming is "change developer tooling (`cozyvalues-gen`, `cozypkg`, the package `Makefile`s) → `development.md`" on the website. It does not fire here: `OCI_EXPORT_DIR` is a CI-only build mode that defaults off, and the local developer workflow is unchanged — `make image` still builds and pushes exactly as before, which the new default-path case in `hack/common-envs_test.bats` now pins. No chart values, no API surface, no app list, no platform variant, no Talos version bump and no release asset rename are touched. - [x] No downstream repository is affected by this change ### Release note ```release-note fix(ci): fork pull requests now run e2e via a privileged workflow, and a fork PR can no longer merge on a skipped (never-run) required E2E Tests check ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a dedicated fork E2E workflow that runs after the base PR and reports results via the required **E2E Tests** commit status. - Fork PRs now export build images as OCI archive artifacts for later publishing. - Added a same-repo E2E reporting job and renamed the in-tree E2E flow to **E2E (in-tree)**. - **Bug Fixes** - Added tighter, fail-closed gating to ensure merge blocking when **E2E Tests** isn’t correctly finalized. - **Documentation** - Documented fork vs same-repo E2E split, OCI export behavior, and required fork security settings. - **Tests** - Added Bats coverage validating `OCI_EXPORT_DIR` and preventing unintended registry pushes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 04:26:25 +03:00
Aleksei Sviridkin	806b66f25c	docs(ci): stop describing a one-workflow E2E CI ... Two sentences predate the second workflow and were left describing the world before it. The scope line at the top is the pointer AGENTS.md routes agents through before they touch the E2E CI, and it named only the in-tree workflow. A reader sent there to change the fork lane would not learn that the section holding its invariants exists, which is the one thing that line has to do. It now names both workflows and says which pull requests each serves. The TIA escalation list said edits to "the E2E workflows" escalate to the full suite. That was true while there was one. The selector's list names `pull-requests.yaml` and not `e2e-fork.yaml`, so a change touching only the fork workflow selects nothing: the Chainsaw step is skipped, while the platform install and the OpenAPI tests still run. Say that plainly, and say why it is tolerable rather than pretending it is designed. A `workflow_run` always executes the default-branch copy of the file, so running the suite against a change to it would not exercise that change anyway. It is the same shape as the fail-open the section already cites. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 03:22:08 +03:00
Aleksei Sviridkin	cc8b01d5cd	docs(ci): say when each lane's status opener actually lands ... The reference section claimed both lanes open the required context when their run starts, and enumerated the triggers it protects against: a re-run, a reopen, a `full-e2e` label. Every one of those starts a Pull Request run, and on a fork that run cannot open the status at all. The opener is guarded same-repo only, because a fork's `pull_request` token is read-only, and the fork's opener lives in the privileged workflow, which starts only once the unprivileged build has finished. So on the lane the sentence was most likely to be read about, an already-green head SHA keeps reading green for the whole build. The gap is structural. A read-only token cannot post a status at any point in the run, and that same read-only token is what makes publishing the gate with the default token safe in the first place. It is bounded by the build rather than open-ended. What was wrong was only the claim that it does not exist. Say so in all three places that carried the overstatement: the section itself, the opener's own comment, and the contract test, which pins that each lane has an opener and cannot express when it lands. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 02:10:35 +03:00
Aleksei Sviridkin	9c68b1f83e	fix(ci): stop a cancelled fork run from publishing a verdict ... The invariant that a cancelled run publishes nothing was written down in three places and held in two of them. The fork lane's terminal reporter ran under `always()`, which includes cancellation. When a second privileged run supersedes the first, the dying one still posted `failure` on a head SHA it no longer owned, for a suite that never finished. It self-corrected once the superseding run reported, but that can be two hours later, and until then a legitimate pull request reads red. The same-repo lane was moved off `always()` for exactly this reason. Move this one too: a failed publish or e2e still reports, since neither is a cancellation, and a cancelled run now leaves the `pending` it opened standing until its successor concludes. The same-repo guard's rationale cited a scenario that no longer exists. It described a label event cancelling the run that opened the pull request, which the concurrency split made impossible for every label the workflow discards. The guard is still needed, for a push, for a `full-e2e` label superseding the head run, and for a manual cancel, so the comment now names those instead of an extinct one. The reference section listed the pieces this invariant rests on and omitted the reporters, which is the half a reader is most likely to touch, and counted them, which was already wrong and would go wrong again on the next addition. It now enumerates without counting. The contract pins both reporters, because both lanes have had this defect at different times in the same shape. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 01:26:09 +03:00
Aleksei Sviridkin	8e097488c2	docs(ci): name every writer of the "E2E Tests" status, and why it opens ... The section that tells the next editor to keep exactly one run per head SHA responsible for the status listed only two of the three jobs that write it, and described the same-repo lane as posting once at the end. Both stopped being true when the same-repo lane gained an opener. The section also spent a paragraph on the concurrency invariants without saying why an opener has to exist at all, which is the part a reader would otherwise remove as redundant. Say that each lane opens with `pending` and concludes, that a status never expires so the opener is what stops a re-run inheriting the previous run's green, and that `plan`'s opener is `continue-on-error` because every build and test job hangs off that job. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 00:55:02 +03:00
Aleksei Sviridkin	fd06fb32e0	docs(ci): say what fork TIA hardening buys, and what it does not ... Four comments and a documentation bullet claimed that routing the changed-file list through the base-repo API stops a fork from making test selection pick nothing. It does not. The `e2e` job runs from the fork's own tree, so `hack/select-e2e.sh` is the fork's script: a PR that edits it to print nothing gets `skip=true`, the suite step never runs, the job succeeds and the required status goes green — the exact outcome the paragraph presented as prevented. The same holds for the suites under `hack/e2e-chainsaw/`, the install bats and the testing package's recipes, all of which the fork authors and this job executes. Hardening the inputs is still worth doing, since a merge-ref diff would be a second bypass on top of the one that exists, so the code does not change. What changes is the claim: the inputs are trusted, the decision is not, and the fork lane is a correctness gate against an honest contributor's regression rather than a boundary against a hostile fork. State plainly what does contain a hostile fork — the maintainer approval required before any fork run executes, the absence of any credential in that job, and a diff editing the harness being visible in review. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 00:04:15 +03:00
Aleksei Sviridkin	575b41e1cd	test(hack): let each bats file declare the EXIT traps it still holds ... The ban on EXIT-trap cleanup in hack/*.bats was enforced by two lists inside hack/cozyreport.bats: one naming the files known to be clean, one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard kept failing. A change adding a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second landed. A file arriving with traps hit the same wall from the other side -- the inventory did notice it, since the string it compared was built by scanning the directory, but absorbing it meant an edit in a file nothing in the author's diff pointed at. Replace both lists with a declaration each file makes about itself: one "# EXIT-TRAP DEBT: N" comment, exact rather than a ceiling. A file carrying none must install no EXIT trap, which covers the converted files, the files that never had one, and every file added later. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count collide textually instead of silently. The declaration is read from the leading comment block, not from anywhere in the file. A .bats file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test. Reading it there as a statement about the whole file would let an unrelated fixture excuse a real trap, and would do it silently, since nothing in that test's own diff looks like a declaration. A comment block is the region with no interior: stopping instead at the first @test would still read a line out of a helper's heredoc. Not every counted handler is debt. A trap inside an explicit subshell does not replace the bats binary's own, so a test failing inside `( ... )` still prints its `not ok`. hack/e2e-test-openapi.bats kills a backgrounded kubectl proxy that way, and moving the kill to the end of the body would leak a process holding a fixed port. Its declaration records that rather than scheduling a conversion, and because the ratchet is exact, removing the trap fails too -- the count protects the construct. What the count cannot do is tell the two apart: substituting a test-level trap for the subshell one keeps the total at 1 and stays green, which the header states rather than leaves to be found. Counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed `trap ... EXIT; cd "$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap ... EXIT` and `(trap ... EXIT; true)`; upper case missed `trap ... exit`, which bash and dash both install. All are real handlers that scored zero, and the left boundary matters most, since the inventory being replaced had none and caught the semicolon form. A bare word boundary is not enough either way: `bootstrap ` ends in `trap `, and it must keep scoring nothing. A quoted signal counts for the same reason as the rest. Two handlers sharing one line are reported rather than counted, since the count is a count of lines and the second would otherwise arrive without moving the total; splitting the line properly needs a shell parser, a semicolon inside a handler's own action not being a separator, and guessing wrong undercounts -- the one direction a ratchet cannot afford. The scan recurses, so hack/e2e-apps/*.bats is covered rather than sitting one directory below the guard that claims the tree. It reads .bats and nothing else, so a handler arriving through a sourced .sh stays outside it -- hack/e2e-chainsaw/_lib/run-kubernetes.sh installs two, each benign for its own reason rather than by design: one sits in a function declared with `(` and so runs in a subshell, the other in a brace function no @test calls. That boundary is stated in the header rather than papered over. The guard moves to hack/bats-no-exit-trap.bats: its subject is every unit suite under hack/, not the report collector it grew up in. Its fixture helpers assemble the trap keyword and the signal from separate arguments, because the guard scans its own source and a fixture written as a literal would be counted as a real trap in it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 23:53:40 +03:00
Aleksei Sviridkin	e2643ef4b9	docs(ci): explain the fork image allowlist and fix a test comment ... Two statements that were not true of the code next to them. The fork `publish` job accepts only archive names it can derive from the base parent, so a fork cannot name an artifact into an arbitrary registry repository. The consequence is that a fork PR adding a new image fails on `image '<name>' is not in the base-tree allowlist` with no way to pass, and nothing said so — including the E2E section that AGENTS.md points a reader at when a fork run fails. Record the constraint, the exact log line, and the two ways around it, so the failure reads as the guard working rather than as a broken PR. The concurrency contract's comment on `code_lines` claimed a real grep error propagates out of the helper. It does under the bats binary, and not under `hack/cozytest.sh`, whose translator appends `return 0` to every line that is exactly `}` — file-level helpers included — so a helper's last command never decides its status there. Both runners still fail closed, because each test asserts its extraction is non-empty before comparing, so a swallowed error surfaces as empty input rather than as a pass. Say that instead. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 23:10:19 +03:00
Aleksei Sviridkin	7c5e96947e	fix(ci): stop two runs from publishing "E2E Tests" on one SHA ... Giving every `labeled` event its own concurrency group stopped a discarded label from cancelling a live run, but it also stopped a `full-e2e` label from doing so — and that label is not discarded. `plan` proceeds for it and `e2e-report` publishes with it, so the labelled run and the head run it was meant to widen both post the `E2E Tests` context, on the same head SHA, with no ordering between them. That was a deliberate trade at the time: commit statuses are latest-wins per context, so the reasoning went, the full-suite verdict lands last whenever its suite finishes last, and the label is an explicit opt-in to a longer run. This commit reverses that decision, because the premise only holds while the full run succeeds. The full suite is the superset, so a full-e2e run that fails early — a build flake, a suite failing at minute twenty — finishes before the narrower head run it was meant to widen, and that run's later green erases the failure. The required status then reads success for a SHA whose suite did not pass. One publisher per head SHA is worth more than the minutes of runner time a cancelled narrow run costs. Move aside only the label events `plan` discards, by making the concurrency condition the exact complement of `plan`'s guard. A `full-e2e` run stays in the main group and supersedes the head run the way a push does, so exactly one publisher of the context is alive per head SHA. Opting into the full suite now costs one suite instead of two, and a second label landing afterwards can no longer cancel the full-e2e run, because the two are no longer grouped together. The contract test gains the complement — the group key must exclude the publishing label rather than every label — and a check that the label name is one and the same string in all three guards that read it, since renaming it in one place splits the decision without touching a line that reads load-bearing. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 23:07:33 +03:00
Aleksei Sviridkin	3c50a09457	fix(ci): keep a discarded run out of the fork e2e gate's group ... The privileged fork e2e run groups by fork repo + branch with cancel-in-progress, so every completed Pull Request run for that branch lands in the same group -- including a run that built nothing. Labelling an open fork PR produces exactly that: pull-requests.yaml triggers on `labeled` and `plan` discards every label other than full-e2e, so all jobs skip. The privileged run for that event cancels the one that is mid-suite on the same head SHA, then `resolve` recognises the discarded trigger and posts no status at all. The required "E2E Tests" context is a commit status, so the head SHA is left at "Expected" with nothing left to flip it -- the run that replaced the owner is the very one that decided to stay silent -- and the PR stays unmergeable until the contributor pushes again. Key the group by the triggering run's id when that run concluded `cancelled` or `skipped`, so a run that will post nothing can cancel only itself. Real runs keep superseding each other on fork repo + branch. The contract test ties the conclusions in the key to the ones `resolve` stays silent for. Those two lists sit in different languages in different halves of the file, and a third conclusion added to the guard alone would restore the wedge without touching a line a reviewer reads as load-bearing. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 22:40:34 +03:00
Aleksei Sviridkin	7e2db4da7e	fix(e2e): bound every remaining read in the report collector ... Four sections of hack/cozyreport.sh read with a wall-clock ceiling and the rest -- nodes, namespaces, events, HelmReleases, packages, packagesources, PVCs, cert-manager, COSI, kamaji, the Talos node reads and every module gate -- read with none. The tarball is written on the collector's last line, so a single read that never returns does not truncate the report, it loses it, on the run where it is the only evidence left. Each read now goes through the shape that fits it: the existing bounded reader for a read whose output is a file, the existing selector for one that feeds a loop, and a new probe for the `if kubectl get deploy ...` gates. A gate is a read like any other, and an unbounded one is worse than the reads it guards, since the module below it never starts. Its cut-off is recorded, because skipping the module leaves the same empty tree as a cluster that never had the component; a gate that answers "not installed" is recorded nowhere, since that is the cluster answering correctly and a marker firing on every install without KubeVirt is one a reader learns to skip. The reads that stay on the bare wrapper say so where they sit; what they share is that the output is not the reader's to write, going into a variable or through a pipeline that transforms it, so there is no moment at which a marker could attach. The header claimed a per-section list of what was bounded, and the tests froze that list plus a per-section count of the unbounded reads. Both are gone. The list had already gone stale, naming four sections while the file had ten, and the counts were an undercount by construction -- they matched a redirect to `$VAR/`, which no gate, selector or command substitution has. One test replaces both: no read below the library guard runs without a ceiling, checked by looking for a counterexample. A ceiling per read is not a ceiling on the report, and the header now says so where it used to imply the opposite. The pod, VM, VMI and services walks carry a count cap and a time budget; the walks over nodes, namespaces, HelmReleases, packages, PVCs and certificates carry neither, so their cost is the cluster's to set. What this removes is the read that never returns. The bounded reader now names the tool it ran and describes what that tool returned. It served kubectl alone until this change gave it talosctl and the four host commands, so a `df` that hit a stale mount left a marker reading "kubectl exited 1 ... not because the object ended" -- two false statements in one line, in an artifact whose premise is that its notes can be believed. The exec reader keeps the remote command's whole message in the listing it belongs to. `kubectl exec` appends its own `command terminated with exit code N` after whatever the command said, so quoting the last line there drops the cause and keeps the trailer, and sending the message away from the file leaves a zero-byte listing -- the "returned nothing, or never ran" question this collector exists to answer. Its cut-off note also stops asserting that the archive is beside it: a stream that got members across is kept and truncated, one killed before its first byte leaves zero bytes and the call site's emptiness check removes it, and the reader runs before that check, so it names both. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:38:29 +03:00
Aleksei Sviridkin	920ad3e30d	fix(e2e): bound the report reads that run inside a pod ... The LINSTOR module runs six reads through `kubectl exec`: three listings, the error-report index, and the controller and per-satellite ErrorReport bundles. They are the only reads in the collector whose duration is set by a workload rather than by the apiserver -- a wedged controller, a satellite that stops answering, or a broken SPDY upgrade holds the stream open with no clock anywhere in the path. The tarball is written on the collector's last line, so one of those never returning does not truncate the report, it loses it. They go through a reader of their own rather than the existing one. `kubectl exec` returns the remote command's exit status, so a 124 there is either this collector's deadline expiring or a command that itself exited 124, and the status does not separate them; describing it the way every `get`, `logs` and `describe` is described would state a mechanism about the machine writing the report for something that may have happened inside a pod. The note says what is open instead of resolving it, and goes beside the output rather than into it, since two of the six stream a gzipped tarball that a line of prose turns into an archive nothing can open. kubectl's own message travels with the note for the same reason -- until now it was merged into the listings, so every run put the "Defaulted container" line at the top of a table. The guard that held the old assumption asserted only that no bounded read was an exec, which leaving every exec unbounded satisfies. It now also requires each exec to be bounded and requires the reader not to borrow the classification written for reads whose 124 is unambiguous. Matching on the verb rather than on the string `kubectl exec` is part of the fix: half of these calls spell it `kubectl -n cozy-linstor exec`, which that string never saw. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:38:29 +03:00
Aleksei Sviridkin	96e33fe656	ci(e2e): give the report collection step a ceiling of its own ... The step that runs hack/cozyreport.sh carried no timeout-minutes in any of the three workflows that run it, so it inherited only the job's 180. The collector writes its tarball on its last line and the upload is the next step, so a read that never returns spends what is left of the job budget, the job is cancelled, and the upload never runs -- the artifact is lost entirely rather than truncated, on exactly the run where it is the only evidence left. `|| true` guards against a non-zero exit, not against a read that does not return. 30 minutes is measured against the step rather than divided out of the job: it takes 3-42s across sampled runs, on failed e2e jobs as well as green ones, and 30 leaves five sixths of the job budget for the steps that upload what this one produced. A test pins the ceiling in all three workflows and requires it to stay under the job's own, since a step ceiling at or above the job's cap is the same as no ceiling at all. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:38:29 +03:00
Mattia Eleuteri	f859b710a4	fix(backupstrategy-controller): repair the bucket credentials Secret from the gate, not a chart fail ... Addresses the four findings on #3524. **The `fail` in the bucket chart (MAJOR).** The <bucket>-<user>-credentials Secret was made to `fail` the render while its COSI lookup was empty, so the release would retry and converge instead of skipping the Secret permanently. Helm cannot render a partial set, so that fail aborts the WHOLE <bucket>-system release: the other users' Secrets, and the bucket UI Deployment/Service/Ingress/HTTPRoute with them. One declared user whose BucketAccess never provisions takes down every other user of that bucket, and on an already-installed release parks it in Failed, blocking every later upgrade. **No per-bucket escape (MAJOR).** Rather than add one, remove the need for it. The gate already forces the release that renders the lookup-gated Strategy CRs; the credentials Secret is the same trap one release earlier, and the gate reads that Secret to resolve the bucket name in the first place. So while it is absent, or carries no bucket name, force the bucket's <bucket>-system release instead of doing nothing. That cannot deadlock its own precondition: the BucketClaim and the BucketAccess whose COSI Secret the lookup reads are rendered unconditionally by the parent release, not by the one being forced. The two releases are throttled independently — they are forced in sequence on a bootstrap, so a shared timestamp would delay the second by a full MinForceInterval for no reason. Coordinates come from the chart under provisionBucket, and are omitted on external S3 where the Secret is admin-managed and no release renders it. No RBAC delta: patch on helmreleases is already cluster-scoped. The chart goes back to a partial render, with the comment now explaining why the skip is not self-healing and why failing is the wrong lever, and a test that renders the UI with every user unresolvable so a reintroduced `fail` fails the suite. Tenant buckets keep their pre-existing behaviour; generalising the repair to every bucket needs a controller that owns them and is tracked separately. **Stale gauge (MINOR).** cozystack_backup_default_objects_missing was only written on the happy path, so the state the gate exists to catch reported 0 and the documented alert never fired. It is now written on the unresolved path too — the credentials Secret counts as one of the objects the default backups depend on, which is what it always was. On an API error the gauge is deliberately left alone rather than flapping, so a new cozystack_backup_default_objects_check_errors_total marks it stale, and the doc pairs the two instead of overstating the gauge. **Suspended releases (MINOR).** forceHelmRelease now does a point Get and skips a release with spec.suspend: true. helm-controller ignores forceAt and requestedAt while suspended (`cozyhr suspend` sets exactly that), so the gate was re-stamping every MinForceInterval for the whole suspension and climbing the force counter — which the runbook attributes to a render that is not producing the objects, the wrong diagnosis. The skip is logged and not counted, so a climbing counter keeps its documented meaning. Reported-by: IvanHunters <xorokhotnikov@gmail.com> Signed-off-by: Mattia Eleuteri <mattia@hidora.io> Assisted-By: Claude <noreply@anthropic.com>	2026-08-07 11:10:41 +02:00
Aleksei Sviridkin	2527e915bd	fix(e2e): collect diagnostics for unready pods and record every truncated capture (#3567) ... ## What this PR does The e2e diagnostic tarball exists so a red run can be explained from the uploaded artifact alone. Several of its collectors answered "nothing is wrong" instead — some because they selected on a column that never carries the value they filtered for, others because they stopped early and left nothing saying they had. Both failure modes look identical to a healthy cluster from the outside, which is the worst possible shape for a diagnostic. Four selectors in `hack/cozyreport.sh` and `hack/cozyreport-summary.sh` were reading the wrong thing: - Per-pod evidence was chosen by the STATUS column, so a pod left at `0/1 Running` by a readiness probe that never passes was skipped. That is a common way for a suite to time out, and the artifact carried no trace of the pod the run died on. Selection now compares the two halves of the READY column; terminal `Succeeded`/`Completed` pods stay excluded. - The pending-LoadBalancer section filtered `$4`, which is CLUSTER-IP. CLUSTER-IP holds an address, `None` or `<none>` and never `<pending>`, so that loop had never executed. `<pending>` only ever appears in EXTERNAL-IP, `$5`. - The cozystack-apps section gated all three of its kinds on `kubectl get crd <kind>.apps.cozystack.io`. ApplicationDefinition is a CRD but in a different group, and Tenant comes from an aggregated APIService with no CRD to ask about, so the probe failed and the section never ran. - The kamaji section was gated on `cozy-linstor`'s `linstor-controller` deployment rather than kamaji's own, so a cluster without LINSTOR got no kamaji logs and no TenantControlPlanes — on exactly the runs that need them. The summary had three of its own: the HelmRelease listing printed `$5` where STATUS is the Ready condition's whole message, reducing every failed release to one word like "Helm"; the Flux sources listing used a jsonpath with a nested filter that kubectl rejects outright, so it printed nothing on every report ever taken; and pod AGE was read as `$6`, which shifts whenever RESTARTS prints the `7 (4m12s ago)` form — that is, for exactly the rows worth reading. The second half is bounding. The pod, VM, VMI and services walks now run under a count cap and a wall-clock budget, and every read is individually time-boxed. The point is not the bounds themselves but that each one names itself when it fires: a walk stopped by the cap or the clock leaves `COLLECTION-TRUNCATED.txt`, a pod list that never returned leaves `COLLECTION-FAILED.txt` instead of the empty tree a healthy cluster produces, a log cut short says so on its own last line, and a `--tail` bound is recorded beside the logs rather than inside them, since the platform's controllers log JSON per line and a trailing prose line costs a parser the whole file. A knob value the collector could not use is reported too, so a truncation note quoting `600` is never mistaken for the `10m` somebody actually set. Where `timeout` is absent the reads run unbounded rather than not at all, and the section says which of the two happened, because a missing local binary is not a statement about the cluster. One behaviour change worth stating plainly, because it is a trade and not a pure gain: the per-pod section used to be uncapped and read container logs whole, and it is now bounded by `COZY_REPORT_PODS_MAX` (40), `COZY_REPORT_PODS_BUDGET` (600s) and `COZY_REPORT_POD_TAIL` (2000). Below those thresholds the new tree is a strict superset of the old one — the old selector missed `Running 0/1` entirely and skipped logs for `Pending` pods. Above them the artifact is smaller than before, every drop is named in the tree, and `COZY_REPORT_POD_TAIL=-1` with a larger `COZY_REPORT_PODS_MAX` restores the old reach. Because widening the selection and capping the walk in one change could otherwise let a newly-in-scope pod evict one the uncapped walk always collected, the ordering splits by evidence class before namespace: pods that are not `Running` — the class the collector took before this — all rank ahead of pods that are `Running` but unready, so the cap can only ever drop a pod the old collector did not keep either. `hack/e2e-capture-previous-logs.sh` gets the same treatment: its notes now go to `capture-notes.txt` in the artifact as well as to the job log, and a pod list that never returned is reported as such rather than as "nothing restarted". That script now creates its own output directory before its first line of output, which makes the comment in `hack/e2e-chainsaw/.chainsaw.yaml` wrong about why the once-per-test guard there holds — the only change to that file is the corrected comment, saying the `-d` check keys on whether the catch block already ran rather than on what the helper found. The rest is test coverage — a new `hack/cozyreport.bats`, an extended `hack/capture-previous-logs.bats` — and the removal of `EXIT`-trap cleanup from the bats files those tests now cover. That trap is worth its own note: inside an `@test` body it replaces the one the `bats` binary installs for its own bookkeeping, so a test that then fails prints no TAP line at all. It does not appear as `not ok`; it disappears, and only the `# bats warning: Executed N instead of expected M tests` line at the end distinguishes the run from a green one. A guard test enforces the ban over a named list of files that grows as the remaining ones are converted. ### Testing Every collector this touches is now covered by unit tests that run without a cluster: a new `hack/cozyreport.bats` (166 tests) and an extended `hack/capture-previous-logs.bats` (34), driven through `hack/cozytest.sh`. Ten `hack/*.bats` files run green locally, 265 tests in total, with each file's `@test` count reconciled against its passing count rather than trusted to the exit status — that reconciliation is the point of the trap change, since an exit code answers "did anything fail" and never "did anything run". The guard test that bans `EXIT`-trap cleanup was checked by committing the violation rather than by reading it: a folded `trap 'rm -rf "$tmp"' \` + `EXIT` added to a file on its list turns the guard red and names that file, and removing it turns the guard green again. `shellcheck --shell=sh` over the three changed scripts reports strictly fewer findings than before the change (`hack/cozyreport.sh` SC2086 170 to 116, SC2162 15 to 13; the three SC2102 in `hack/cozyreport-summary.sh` are gone), and `hack/e2e-capture-previous-logs.sh` is clean. The parts that need a cluster — that the bounds fire, and that each one lands in the artifact — are exercised by the e2e run itself, since these collectors run on every failed suite. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. This PR touches `hack/cozyreport.sh`, `hack/cozyreport-summary.sh`, `hack/e2e-capture-previous-logs.sh`, `hack/e2e-chainsaw/.chainsaw.yaml`, ten `hack/*.bats` files and `docs/agents/e2e-testing.md`. Nothing is moved or renamed under `hack/`, no make target changes what it does, and the files downstream consumers key on (`hack/package.mk`, `hack/common-envs.mk`) are untouched. No package, values key, CRD, variant, namespace or release asset is involved. - [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(e2e): the diagnostic report now collects per-pod evidence for pods that are Running but not Ready, which it previously skipped, and several collector sections that silently produced nothing now report correctly. Collection that stops early on a bound now says so inside the artifact, so a truncated report can be told apart from a healthy cluster. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Diagnostic reports now prioritize relevant resources and collect bounded pod, VM, service, storage, certificate, and Flux data. - Reports preserve partial results and clearly identify timeouts, unavailable resources, truncation, warnings, and collection failures. - Previous-container logs now include bounded capture, namespace prioritization, failure details, and partial-read evidence. - Reports target the operator workload and safely handle missing dependencies or temporary storage failures. - **Documentation** - Expanded guidance for reliable end-to-end testing and diagnostic report collection. - **Tests** - Added comprehensive coverage for report generation, log capture, failure handling, limits, and portability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-06 15:25:20 +03:00
Aleksei Sviridkin	e80b8e370f	fix(e2e): collect logs for unready pods and record truncated captures ... The failure snapshot selected pods by their STATUS column, so a pod whose container runs while its readiness probe never passes -- STATUS Running, READY 0/1 -- was dropped from the report entirely. That is a common way for a suite to time out: the replica never joins, its owning HelmRelease sits in InProgress, and the artifact carries no trace of the pod the run died on. Select on the READY column instead, which also covers the partially ready multi-container case, and keep every phase that was collected before. On one degraded cluster that is four of the seven broken pods the old filter reported nothing about. The VMI and VM sections had the same blind spot and now share one selector that reads READY as the last field, because a Pending object prints fewer columns than a Running one and the two kinds differ only in how many columns precede READY. READY counts ready containers rather than the pod's Ready condition, so a pod held back by an unsatisfied readiness gate is out of scope; a test pins that rather than leaving it to be found in an empty directory. Logs are requested for every container of a selected pod rather than skipped for Pending ones, and the per-pod collection moved into a helper so it can be driven against a stub kubectl. kubectl's refusal is the finding, and an empty result is ambiguous on its own: a read against an unscheduled pod exits 0 having printed nothing, which is what a container that started and stayed quiet also leaves. Every site that classifies a failed read now orders its branches the same way, naming the timeout first where both a cutoff and an uncapturable stderr apply. The CRD gate in the summary had no such branch at all: it returned in silence, which is byte-identical to the CRD being absent, so every section behind that gate vanished from a report whose only real problem was that the machine writing it had nowhere to put a temp file. Collection used to stop silently. A walk that hit its cap said so in the job log and nowhere in the artifact. Now the pod walk leaves COLLECTION-TRUNCATED.txt when it stops at COZY_REPORT_PODS_MAX or COZY_REPORT_PODS_BUDGET, CONTAINER-LOGS-TRUNCATED.txt on a pod cut off part way through, COLLECTION-FAILED.txt when the pod list never came back rather than the empty tree a healthy cluster produces, and COLLECTION-UNBOUNDED.txt when timeout is missing locally, which is a fact about the machine writing the report and not about the cluster. Tenant namespaces are collected first, so a cap that only bites on a broadly degraded cluster is not spent on platform pods while the pod the suite died on falls off the end. Container logs are read with --tail=2000 where they were previously read whole, and every bounded file states the bound it was written under, because --tail drops the oldest lines and nothing else in the output would say so. COZY_REPORT_POD_TAIL=-1 restores the complete log. The bounded whole-object read no longer merges stderr into pod.yaml, vm.yaml and vmi.yaml, which are read by yq rather than by eye: a warning on an otherwise complete read left an unparseable file. stderr goes to a scratch file and is copied in only when kubectl returned no object, and the truncation markers those files carry are written as YAML comments, since a bare marker line after a complete mapping is a second top-level node. Six latent faults surfaced while making these reads report. The kamaji module was gated on the LINSTOR controller, so a cluster without LINSTOR got no kamaji evidence at all, which is what a tenant-kubernetes bootstrap timeout needs most. The Flux sources section nested one jsonpath filter inside another, which kubectl rejects outright, so it printed nothing on every report ever taken and read as "all sources Ready". The HelmRelease listing took a single field as STATUS where STATUS is the Ready condition's whole message, so every failed release rendered as one word. The services section filtered for a pending LoadBalancer address on the CLUSTER-IP column, which never carries that value. The cozystack module read its image from a Deployment the installer stopped shipping when it moved to the operator, putting kubectl's NotFound in the first file of the report. And the cozystack apps section gated all three of its kinds on a CRD in an aggregated API group, so it never ran once; two of those names were wrong, and both are collected again. The tests avoid EXIT-trap cleanup, and e2e-testing.md says why: a bats test that installs one and then fails prints no TAP line at all, so output grepped for `not ok` reads as green, while hack/cozytest.sh reports the same failure correctly. A guard enforces that over a named list of files rather than a glob with exceptions, because a list of exceptions is the same claim written so that forgetting to shorten it makes it false. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-05 17:37:04 +03:00
Aleksei Sviridkin	7739d1e294	feat(multus)!: stage reference CNI plugins from the multus image ... Multus-backed NetworkAttachmentDefinitions delegate to the upstream containernetworking reference plugins (bridge, macvlan, ipvlan, ...), and nothing guarantees a node has them in the directory Multus searches. On k3s they live under /var/lib/rancher/k3s/data/cni while Multus and Cilium install into /opt/cni/bin, so a bridge-type NAD fails there with `failed to find plugin "bridge" in path [/opt/cni/bin]`. Bundle 14 of the release's plugins into the multus image and add an install-cni-plugins init container that installs them into the host /opt/cni/bin, mirroring the existing install-multus-binary container. loopback, dhcp, tap and dummy are left out; the Dockerfile records why. Gate it on a new chart value, stageCniPlugins, resolved per bundle and on for both. Talos bundles only bridge, firewall, flannel, host-local, loopback and portmap, so the other ten are missing there exactly as on generic Linux; the four that overlap are rewritten with the same upstream version, v1.9.1 on both sides, rather than an older one. A marker in the image Dockerfile records the Talos release that pin was compared against, and a test holds it to the release the platform ships. multus therefore moves out of system.common-packages into the two variant branches, the way linstor already does for talos.enabled. networking.stageCniPlugins overrides the variant default in either direction. Where staging is on the install is unconditional: a plugin is replaced on every pod recreation, not only when the destination is missing. A destination that is already a directory is reported and left alone, since replacing it is not something this can do safely. Turning the value off afterwards does not undo it; nothing here removes or restores a plugin. The container is not privileged and takes no mount propagation. It runs in the spc_t SELinux domain, without which an enforcing node refuses the write. Each plugin is installed under a temporary name and swapped in with a rename: /opt/cni/bin is shared with cilium and kube-ovn. The plugin tarball is checksummed before extraction. A plugin that cannot be installed is reported on stderr and to /dev/termination-log, and the container still exits 0: Multus stays the node's primary CNI and nothing removes its config, so an init container that never completes would leave the shim with no daemon behind it. The manifest change lives in customize-deployment.patch so it survives make update. BREAKING CHANGE: enabling stageCniPlugins replaces reference CNI plugins in /opt/cni/bin on existing nodes, Talos included -- four of the names it writes are ones the Talos node image also carries. Opt out before upgrading: declining afterwards does not restore what was replaced. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-05 14:19:47 +03:00
cozystack-ci[bot]	eeaab0f21c	docs: add changelog for v1.6.1 ... Signed-off-by: cozystack-ci[bot] <274107086+cozystack-ci[bot]@users.noreply.github.com>	2026-08-05 10:04:43 +00:00