5308 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
Aleksei Sviridkin	bd3f3b93d0	fix(tenant): render the chart without the platform's injected values ... `helm lint packages/apps/tenant` fails, and two of its three errors are the chart's own. `templates/keycloakgroups.yaml` reads `index .Values._cluster "oidc-enabled"`. `_cluster` is injected by the platform, so it is absent whenever the chart is rendered on its own, and `index` on a nil map is a hard error: `index of untyped nil`. The chart cannot be rendered or linted outside a cluster at all. The idiom the neighbouring charts use for the same lookup does not rescue this on its own, which is worth stating because it looks like it does. `(index .Values._cluster "x") | default "y"` still errors: `index` evaluates first and fails on the nil map, so `default` never sees a result to substitute; it only ever covered a key missing from a map that exists. The spelling alone does not decide whether a chart breaks, though. `packages/system/cozystack-basics` uses it and lints clean, because line 1 of its values.yaml is `_cluster: {}`, which makes the map non-nil before `index` runs. So a chart is broken only when it neither declares the empty map nor guards at the use site. Guarding at the use site is preferred here because it makes the template correct whatever values.yaml contains, which is what lets the chart render standalone; and `_cluster` is platform-internal, while this chart's values.yaml, schema and README are generated together from api/apps/v1alpha1/tenant/types.go as user-facing output. The guard has to sit on the map rather than on the value it returns, hence `index (.Values._cluster | default dict) "oidc-enabled"`. That is behaviour-preserving rather than merely quieter. With `_cluster` present, `| default dict` returns it untouched and the lookup is what it was. With it absent the lookup is nil, the `eq` below is false, and the block stays unrendered, which is what an unconfigured tenant should produce. Rendered both ways before and after, with `--api-versions v1.edp.epam.com/v1` so the capability-gated block actually executes, the output is identical byte for byte: four KeycloakRealmGroups when `oidc-enabled` is `"true"`, none when it is `"false"`. Rendering with no values at all goes from a hard error to a clean render with no groups. `Chart.yaml` also does not declare `cozy-lib`, which is present as a symlink under `charts/`, so lint reports missing chart metadata. Declare it the way `packages/apps/qdrant` does, that being the one chart in the tree that declares it and does not carry this error. Rendering is unchanged by the declaration, checked the same way. `tests/keycloakgroups_oidc_test.yaml` pins all three branches of the gate: four realm groups when `oidc-enabled` is `"true"`, none when it is `"false"`, and none when the platform injected no `_cluster` at all. That third case is the one the guard exists for, and it needs no mock, because `_cluster` is absent from values.yaml and so "not injected" is simply the chart's own default state. Against the unguarded template the suite fails with `index of untyped nil`; the chart's other seven suites pass identically either way, which is what left the change uncovered. The third error, `invalid icon URL '/logos/tenant.svg'`, is deliberately left. Lint wants a URL with a scheme, and all 31 charts in the tree that carry an icon use a relative path, none an absolute one. Changing this one would make it the only chart with a different convention, and choosing the URL a relative path should become is a decision about where those assets are served from, not about lint. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 02:25:05 +03:00
Aleksei Sviridkin	605030b5a7	test(backupstrategy-controller): anchor the metrics-absence guard to its container (#3605) ... ## What this PR does The `metrics.enabled=false` case in `deployment_test.yaml` guards against the `metrics.enable` typo regression by asserting that the metrics arg and port are absent. Both assertions address the controller by index, and `notContains` passes whenever its path resolves to something that lacks the forbidden content. A different container at that index qualifies, so a sidecar placed ahead of the controller leaves the guard green while the flag it exists to forbid comes back on the real container. This adds an `equal` on `containers[0].name` ahead of the two negatives. It fails when the index stops pointing at the controller, which is the case the negatives structurally cannot see. Verified rather than argued. Rendering a sidecar ahead of the controller against the current tests leaves the case green and reddens only the two neighbouring tests, which use positive `contains` and defend themselves; the guard that exists for this scenario is not among them. With the anchor, the case fails on the anchor. This is not a new convention. `packages/system/opensearch-operator/tests/leader_election_test.yaml` and `packages/system/dashboard/tests/gatekeeper_test.yaml` already anchor index-addressed negatives the same way, the second with the reasoning spelled out in a comment. This brings one more package in line with what the tree already does. ## What this PR does not do It does not switch the assertions to a name filter. helm-unittest resolves `containers[?(@.name=="...")]`, but `notContains` against a filter that matches nothing passes just as quietly, so that trade swaps "wrong container" for "no container" inside the same family of silent green. It does not touch the other two cases in the file. Both are positive `contains`, both went red under the same injection, so they anchor themselves. ## Tests The change is itself a test fix. Package suite is green: 11 tests, 3 suites. ### Screenshots None. This changes a test file. ### Downstream repositories Walked the trigger map against the diff. One helm-unittest file, nine added lines, no chart output changes. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved deployment validation by confirming the correct container is targeted when metrics are disabled. * Added checks that metrics-related arguments and ports are absent in this configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 17:48:10 +03:00
Aleksei Sviridkin	0ba567dd73	fix(tenant): bound the pre-delete cleanup waits and the hook Job (#3598) ... ## What this PR does The tenant pre-delete hook deletes the tenant's HelmReleases in two waves with `kubectl delete --wait=true` and no `--timeout`. kubectl reads `--timeout=0` as "wait forever" and substitutes a week, so a wave hangs for as long as any child HelmRelease cannot finalize. helm then returns from the uninstall without deleting anything: the release stays in storage, flux keeps the finalizer, and the same teardown gets retried. The delete policy carries no `hook-failed`, so the waiting Job survives between attempts too. Each wave now gets 90s. Neither is guarded, on purpose. Nothing stands behind these deletes, so a wave that did not finish ends the run instead of reporting a cleanup that never happened. That also keeps the order the two waves exist for, since the tenant modules are what the applications run on. Whatever got deleted stays deleted, so the next attempt resumes from there. The Job gets `activeDeadlineSeconds: 270`. It bounds what the waits do not, since the DELETE calls carry kubectl's default `--request-timeout` of 0, and it sits under the 300s uninstall budget flux applies when the HelmRelease has no timeout of its own. That is the budget for tenants cozystack-api generates; `tenant-root` is the exception, a static release in `cozystack-basics` carrying `timeout: 15m0s`, and the ordering holds there too since 280 is under 900. The 30s gap is not cosmetic. The deadline runs from the Job's `.status.startTime` while helm's budget runs from before the Job is created, so at equal values helm gives up first, on a Job that is still running. That is the state this change is meant to end, and it would look fixed, because a number would be there. `terminationGracePeriodSeconds: 10` is part of the same arithmetic. Since 1.31 the Job controller holds back the terminal condition until every pod has finished terminating, so what helm waits for is the deadline plus the termination. At the 30s default that lands on 270+30, level with the budget, which is the gap the deadline was set to buy. That period gets spent in full, every time. The SIGTERM goes to PID 1, which is the shell: the script runs several commands, so the shell never execs into kubectl and stays PID 1 itself. A signal carrying its default disposition is discarded by the kernel when the target is PID 1, and a non-interactive shell installs no SIGTERM handler, so nothing acts on it. kubectl is a child and was not signalled at all. The pod ends on SIGKILL. Nothing in that script needs to finish gracefully, the DELETE calls are already sent and the process is only watching. The pod also gets resource requests, matching the sibling hook in `packages/apps/kubernetes`. Without them it is BestEffort and first to go under node pressure, which mattered less when a retry inside the Job absorbed an eviction. With a single attempt it does not. `backoffLimit: 0` and `restartPolicy: Never` go together, and neither implies the other. At the default limit of 6 the Job does not reach `Failed` inside the budget, so helm falls back on its own timeout with the same nameless "timed out waiting for the condition" as before. Under `OnFailure` the pod survives its own failure and the kubelet re-runs the container in place, and the Job controller finds out afterwards by summing `restartCount`. The script starts over, re-enters the applications wave that just expired, and gets cut down mid-run when the controller catches up. `Never` puts a failed pod in front of the controller directly. ## What this PR does not do No remedy behind the bounded waits. The `packages/apps/kubernetes` hook force-clears child finalizers when its own wait expires. That needs `patch` on HelmReleases in the Role, and it is only safe there because the whole tenant control plane is going away in the same run. Here a wave that expires ends the attempt. `packages/apps/kubernetes/templates/delete.yaml` is untouched. It does not make a doomed teardown converge. A tenant whose children cannot finalize still gets its uninstall retried. What changes is that each attempt ends in bounded time, instead of eating the whole uninstall budget and leaving a pod behind. 90s is a guess, and it is tight. The first wave deletes every application HelmRelease in the namespace, including kinds that are slow to tear down: a nested Tenant runs this same hook one level down before its release finalizes, and an app of kind Kubernetes tears a control plane down. Either one can outrun 90s. The bound is what is left once the Job deadline sits under helm's budget instead of level with it, split between the two waves. The 90s the deadline keeps back for scheduling and the image pull is not calibrated either, and I would rather say so than imply it was. `docker.io/clastix/kubectl:v1.32` comes from Docker Hub with no mirror in front of it, and this repo has measured Docker Hub pulls of a smaller image at about 1m42s in CI. So that remainder is roughly the observed worst case rather than comfortably above it. A cold pull that slow lands on the `activeDeadlineSeconds` path, which is the one that deletes the pod and leaves nothing to read. There is a band that pays for this, and I would rather name it than round it off. Neither shape above fits inside 300s today, so for them the bound costs nothing that works. But every generated child HelmRelease carries a wait strategy, so a wave inherits its slowest child, and a wave that settles in something like 95s does complete inside the budget today. After this change it expires instead, and that tenant trades one clean attempt for an extra flux retry. In wall clock that retry is about five minutes, since `helmrelease-interval` defaults to `5m`, so a tenant in that band takes noticeably longer to delete than it does now. I think that is the right side of the trade: an unbounded wave does not fail, it spends the whole budget and leaves helm reporting a timeout that names nothing. If the band turns out to be populated, the waits and the deadline move together, and the bats suite pins their ordering rather than their values. One caveat on the diagnostics, since the comments in the chart used to promise more than they deliver. The failing wave leaves its output in the failed pod, which `ttlSecondsAfterFinished: 300` then reaps while flux's uninstall backoff is still climbing. And on the `activeDeadlineSeconds` path there is no log at all, because the Job controller deletes the active pods when the deadline expires. That path is the backstop for a kubectl that never returns; the expected path is the wave's own `--timeout`, where the script exits on its own and the pod survives to be read. ## Tests `hack/tenant-pre-delete-hook.bats` renders the chart, pulls the hook script out of the Job and runs it against a stub `kubectl` on `PATH`. It covers a clean run reporting success, every delete reaching kubectl with a non-zero bound, the ordering of the four numbers, and a wave that gave up ending the run without going on to the next one. `--timeout` is implemented inside kubectl, so no stub can make that deadline actually elapse. The tests read the arguments that reached kubectl and inject the non-zero exit an expired delete produces. No EXIT traps. The ordering test reads the numbers out of the rendered Job instead of pinning them, so moving a value keeps it honest while a value that breaks the ordering still fails. An absent deadline or termination period is rejected explicitly, otherwise `null` satisfies the numeric comparison and a Job with no bound at all passes. The Job's own shape is pinned in the chart's helm-unittest suite, since the script cannot observe that about itself. Both suites are green locally, and I mutated each pinned value to confirm they fail for the right reason: dropping either `--timeout`, dropping `activeDeadlineSeconds`, dropping `terminationGracePeriodSeconds`, dropping the requests, `backoffLimit` back to 6, `restartPolicy` back to `OnFailure`, and a `|| echo` appended to the applications wave. Each one goes red in the test that claims that ground. Worth flagging so the check marks are not read as more than they are: `make bats-unit-tests` is red at the merge base, unrelated to this branch, and the loop it runs exits on the first failing file. `hack/cozyreport.bats` sorts ahead of this suite, so on CI the new bats file is not reached at all right now. It is not failing, it is not running. The helm-unittest cases are unaffected and do execute. ### Screenshots None. This changes a Job manifest and its tests, with no user-visible surface. ### Downstream repositories Walked the trigger map against the diff. The diff is one Job manifest inside the tenant chart, its helm-unittest suite, and a new `hack/*.bats` file. No package is added, renamed or removed; no `values.schema.json`, `values.yaml` default or version enum changes; no `ApplicationDefinition` semantics, namespace, variant or release asset changes; nothing under `hack/` is moved or renamed and no make target changes behaviour, since `bats-unit-tests` already globs `hack/*.bats`. - [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(tenant): bound the pre-delete cleanup hook's HelmRelease deletions and give its Job a wall-clock deadline, so a tenant whose children cannot finalize no longer leaves the hook waiting indefinitely and blocking the uninstall ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Tenant cleanup jobs now enforce execution deadlines and terminate gracefully. * Cleanup operations have bounded deletion timeouts and no automatic retries. * Failed cleanup steps now stop subsequent operations and correctly report failure. * Cleanup jobs now specify CPU and memory resource requirements. * **Tests** * Added comprehensive coverage for cleanup behavior, timeouts, failure handling, and job lifecycle settings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 17:48:02 +03:00
Aleksei Sviridkin	954be15341	fix(e2e): bound every read in the report collector and the step that runs it (#3596) ... <!-- 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 A single hung read in `hack/cozyreport.sh` did not truncate `cozyreport.tgz`, it lost the artifact outright. The collector writes its tarball on its last line, the `Collect report` step carried no `timeout-minutes` of its own, and `Upload cozyreport.tgz` is the step directly after it — so a read that never returned spent whatever was left of the job budget, the job was cancelled, and the upload never ran. The runs where the report is the only evidence left are the runs that produced none. `|| true` guards against a non-zero exit, not against a read that does not return. Two halves, independent of each other. **The step gets a ceiling of its own**, 30 minutes, in all three workflows that run it. The number is measured against the step rather than divided out of the job: pulling the `Collect report` timings out of recent e2e runs gives 3 to 42 seconds, on failed jobs as much as on green ones, and 30 minutes leaves five sixths of the job budget for the steps that upload what this one produced. It carries `continue-on-error: true` beside it, because the step's `|| true` is its standing statement that collecting diagnostics is never fatal and a step killed at its ceiling never reaches that `|| true` — without the tolerance, a passing run whose collection ran long would be published as failed, and every later `failure()` in the job would start meaning "the tests failed or the report was slow". That tolerance also renders the overrun green, so a following step turns the collect step's `outcome` into a `::warning::` annotation: a bound that fires is recorded, and the outer bound is not exempt from the rule the inner ones follow. **Every read in the collector is bounded**, against four sections before, and gates and host commands are reads too. An `if kubectl get deploy …` that decides whether a module runs costs more unbounded than anything it guards, because the module below it never starts; `df -h` blocks indefinitely on a stale mount and takes the tarball with it by exactly the mechanism above. A gate cut off short is recorded, since skipping the module leaves the same empty tree as a cluster that never ran the component, while a gate answering "not installed" is recorded nowhere — that is the cluster answering correctly, and a marker firing on every install without KubeVirt is one a reader learns to skip. The six LINSTOR reads run a command inside a pod and go through a reader of their own. They are the only reads whose duration a workload sets rather than the apiserver, and `kubectl exec` returns the remote command's exit status — a 124 there is either the collector's deadline or a command that itself exited 124, so the existing classifier would have named this script's own timeout as the cause of something that may have happened inside a pod. The new note states what the status leaves open, and sits beside the output rather than inside it, since two of the six stream a gzipped tarball. A cut-off bundle is kept rather than dropped, and the note says so: the emptiness check at those call sites asks whether the archive lists a first member, which a truncated gzip does. What this does not do is bound the report. The pod, VM, VMI and services walks carry a count cap and a time budget; every other per-object walk carries neither, so their cost is the cluster's to set, and a failed install with a hundred not-Ready HelmReleases spends four hundred bounded reads in one of them. A per-read ceiling removes the read that never returns, not the thousand reads that each return slowly. The file header says so where it previously implied the opposite, and the step ceiling is what covers the rest. Two frozen lists in the tests are gone, replaced by one invariant derived from the code: no read below the library guard runs without a ceiling. Both lists were already wrong. The header named four bounded sections while the file had ten, and the per-section counts of unbounded reads only ever matched a redirect into `$VAR/`, which no gate, selector or command substitution has — so they described the scan rather than the file. The guard that held the old exec assumption was satisfied by leaving every exec unbounded, which is the state the file was in. It now also requires each exec to be bounded, and requires that 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 it: half of these calls spell it `kubectl -n cozy-linstor exec`, which that string never saw. Two properties worth knowing before this lands. `kubectl get events -A --sort-by=.lastTimestamp` sorts client-side, so kubectl buffers the whole event list and writes nothing until it finishes — under a flat per-read ceiling a cut-off there leaves a file holding only the marker, not a short tail. That read had no ceiling at all before, where the same slowness could take the whole tarball, so this is a narrower failure rather than a new one. And `kubectl logs -n cozy-kamaji deployment/kamaji` gains `--tail=2000`, matching every other controller log in the file: it was the only unbounded-in-size log, which under a wall-clock ceiling is the one most likely to be cut at its *end*, where a control-plane bootstrap failure sits. Longer kamaji history than 2000 lines is no longer collected. Two accepted imperfections in how the change is split across commits, named rather than left to be found. `docs/agents/e2e-testing.md` keeps this material in a single shared paragraph that all three commits touch, so in the middle commit one sentence about the module gates lands a commit ahead of the gates themselves. And two corrections to the exec reader — it now keeps the remote command's whole message in the listing rather than quoting the line `kubectl exec` always appends last, and its cut-off note names both of the two things that can happen to a partial bundle instead of asserting one — sit in the last commit rather than in the one that introduced that reader. Both are true of the branch tip, where every statement is correct and appears once; the cost of moving them is another rebase across the one shared paragraph, which is not worth what it buys. Refs #3568. ### 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): every read in the diagnostic report collector now carries a wall-clock ceiling — module gates, the reads that run a command inside a LINSTOR pod, and the host commands included — and a ceiling that fires is recorded in the artifact rather than only in the job log. The workflow step that runs the collector carries a ceiling of its own, so a read that does not return costs one file and a warning annotation instead of consuming the job budget and losing cozyreport.tgz entirely. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added bounded diagnostic collection across Kubernetes, Talos, LINSTOR, host, controller, and command outputs. - Reports preserve partial results and identify timeouts, failures, missing output, and truncated artifacts. - Added warnings when report collection fails or exceeds its time limit. - **Bug Fixes** - Corrected resource targets, selectors, deployment extraction, and command matching. - Improved handling for empty selections, unavailable resources, and missing timeout tools. - **Documentation** - Expanded guidance for diagnostic markers, collection limits, timeouts, and incomplete output. - **Tests** - Added coverage for bounded reads, failure reporting, markers, selectors, logs, and subprocess mocking. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 17:47:51 +03:00
Aleksei Sviridkin	0b7a8f67d1	fix(apps/kubernetes): report the pre-delete cleanup outcome honestly (#3592) ... ## What this PR does The pre-delete cleanup Job for the `kubernetes` app ended with an unconditional `echo "Cleanup completed successfully"`. Several of its steps swallow a failure behind `|| echo "WARNING: ..."` so that `set -e` cannot end the run, which means that line was printed in exactly the situation those warnings exist to report: child HelmReleases still holding Flux finalizers, or a Kamaji datastore Secret still pinning its namespace. Someone reading the Job log to find out why a namespace will not delete was told the cleanup had worked. The Job now records what a step left behind and closes with either the success line or the list of steps that left something. Only residue gets recorded. A bounded delete that times out has a remedy right behind it (Step 2 falls through to force-clearing finalizers, Step 4 falls through to Step 4b), and whether anything survived is decided by that remedy, which reports its own outcome. A tenant with no working nodes takes that route as a matter of course, which is the reason the wait is bounded at all, so recording the timeout itself would report a finished teardown as incomplete on the commonest path there is and send an operator hunting for objects that are not there. That is the same defect as the one being fixed, pointing the other way. A read that fails looks exactly like one that found nothing. An empty list and a failed list both give a loop nothing to iterate; an absent object and an unreachable apiserver both make a probe non-zero. So the whole script was walked for that shape instead of patching the instances as they were reported. Four reads reach the closing line. Step 2's force-clear re-list now keeps its status and reports when it cannot list what it was about to clear. Step 4b's two probes became three-way, present, absent or unknown, so a Secret whose existence could not be established is no longer announced as absent. Step 1's list is the one left swallowing its status, and that is deliberate: Step 2 deletes by label rather than from that list, so a suspend that never happened leaves nothing of its own behind, and reporting it would name residue that does not exist. Making the TenantControlPlane probe three-way also repairs the gate above it, and that is intended. The gate is there so the datastore Secret's finalizer is not stripped while Kamaji may still own it, and a two-way probe defeated it in the one direction that does damage: any error read as "confirmed absent" and the strip went ahead. Its likeliest cause, a throttled or briefly unreachable apiserver, is also likeliest exactly when a broken tenant is being torn down. Step 4b declining to strip a finalizer while the TenantControlPlane is still alive is the one residue that no failing command marks: every call in that branch succeeds, and the namespace stays blocked regardless. It now reports, and names the TenantControlPlane as the thing holding it up. What this PR does not do: the hook still exits zero, even with every backstop engaged. That is the difference between a main path and a backstop. `packages/apps/tenant/templates/cleanup-job.yaml` is the same kind of hook and handles it the opposite way, correctly: deleting the HelmReleases is that hook's whole purpose, so a failure there means the cleanup did not happen, and `set -e` ending the run before the success line is honest. The swallowed sites here are backstops. They run only after the normal deletion has already timed out, and they exist to push teardown further along. Failing there aborts the Helm uninstall before the release is removed, so the release stays in storage and the same doomed teardown is retried forever, at precisely the moment the backstop was written for. The steps that are not backstopped still end the run under `set -e` and never reach the closing message at all. This makes the message truthful. It does not make it reachable. The Job carries `helm.sh/hook-delete-policy: hook-succeeded`, and the hook exits zero by design on every backstopped run, so Helm reaps the Job, and its pod log, right after the hook completes. Whether the closing line survives long enough to be read is a question about the delivery channel, not about its wording, and changing that channel (an Event on the CR, or keeping the Job when something was left behind) has a different blast radius. Left for a separate change. The step comment and the runtime warning both cited the tracking issue for the Kamaji datastore-secret leak as though it were live. It is closed, and closed by this step rather than by a fix in Kamaji, so following the pointer showed a resolved report and no reason for the code to exist. The comment now says the ordering it covers is still reachable, and the warning names the finalizer and the object to clear by hand instead of an issue number. The workaround itself is untouched: nothing indicates the upstream cause has gone away. `hack/kubernetes-pre-delete-hook.bats` renders the Job, extracts the script and runs it against a stub `kubectl`, covering a clean run, a fallback route that succeeded, a TenantControlPlane outliving Step 4, each probe that could not answer, every remedy failing at once, an unbackstopped step ending the run, and the zero-exit invariant. The helm-unittest cases match the script's source text and cannot tell whether the closing message is reachable. Every case was checked by mutation, including reinstating the unconditional success line and reinstating the over-reporting the fallback case guards against. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change ### Release note ```release-note fix(kubernetes): the pre-delete cleanup hook no longer reports success when a cleanup step left something behind, and no longer reports a completed teardown as incomplete when it merely fell back to force-clearing finalizers. The Job log now names the steps that left residue. The hook still exits zero so the Helm uninstall can remove the release. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Kubernetes cleanup handling during application deletion. * Cleanup now distinguishes unavailable resource checks from confirmed absence, avoiding unsafe finalizer removal. * Reports leftover resources when recoverable cleanup steps fail while still completing the deletion process. * Improved handling of child releases, control-plane resources, and datastore secrets. * **Tests** * Added comprehensive coverage for successful cleanup, partial failures, resource residue, probe errors, and unrecoverable steps. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 17:47:43 +03:00
Aleksei Sviridkin	f4cde3054a	docs(ci): correct the job-level permissions comment (#3591) ... ## What this PR does The comment above the top-level `permissions:` block said jobs request the minimum extra scopes on top of it. That's backwards. A job-level block replaces the workflow-level one, and [every scope it does not name is set to `none`](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions). Write `packages: write` alone in a job and it loses `contents: read` and can't check the repo out. Same wording in every file that carried the old line. It matches what #3574 uses for the workflow it adds. `backport.yaml` and `pull-requests.yaml` also appear in open #3569. Its changed lines there are the `cancel-in-progress` expressions two lines below the comment, and a three-way merge is clean in either order. Comment-only: every changed line starts with `#`, the files still parse, and actionlint reports the same 45 pre-existing findings before and after. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: Walked the trigger map against the diff. The only row that names a file here is the ccp one on `.github/workflows/tags.yaml`, and it triggers on release-prep behaviour, which this doesn't touch. ### Release note ```release-note docs(ci): the comment above the top-level `permissions` block in the workflow files now says a job-level block replaces it instead of adding to it ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified workflow permission guidance across automation processes. * Documented that the default token permissions are read-only. * Clarified that job-level permissions replace top-level defaults and must explicitly declare all required scopes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 17:47:33 +03:00
Aleksei Sviridkin	cab44d56ac	test(tests): pin the keep annotation on cert-manager and etcd-operator CRDs (#3590) ... ## What this PR does `cert-manager-crds` and `etcd-operator-crds` are the two packages this repository points at as the in-tree precedent for protecting CRDs with `helm.sh/resource-policy: keep`. Both carry the annotation, neither had a test, so it could be lost with nothing reporting the loss. This adds a suite to each, plus the `test:` target that `hack/helm-unit-tests.sh` needs before it will run one. No annotation is added and no rendered output changes: the point is to close the way of losing what is already there. The two are exposed differently, so the suites say so instead of sharing one explanation. In `etcd-operator-crds` the annotation is ours. `make update` stamps it with an awk step anchored on the `controller-gen.kubebuilder.io/version:` line that the upstream CRDs carry. If a future controller-gen stops emitting that line, the step matches nothing, writes the CRDs out unstamped and exits zero. In `cert-manager-crds` the annotation comes from the upstream template, emitted under `{{- if .Values.crds.keep }}`, and the value that switches it on is ours. That is two independent ways to lose it. The templates are also not maintained in that package: `packages/system/cert-manager/Makefile` deletes and refills its `templates/` directory during its own `make update`, so a cert-manager bump rewrites a neighbouring package that whoever runs the bump has no reason to open. Each suite is scoped with the wildcard `templates/*.yaml` and asserts across every document rendered under it, rather than listing CRD names or pinning a document count, so a CRD that upstream starts shipping is covered without editing the test. Each also asserts the kind, because the annotation assertion on its own would accept a non-CRD document that happened to carry it. Both charts render only CRDs today, checked with `helm template`. The `test:` target is load-bearing rather than boilerplate. `hack/helm-unit-tests.sh` runs a package's suite only when its Makefile defines that target. With the target present, breaking a chart makes the whole run exit 1 and name the directory. With the fixtures present but the target removed, the same broken chart gives exit 0 and "All Helm unit tests passed", having executed the suite zero times. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: I walked the trigger map in `docs/agents/contributing.md` against the file list of this diff. It adds two test files and two `test:` targets under `packages/system/`, and touches nothing else: no `hack/package.mk`, no app `values.schema.json`, no platform values, no `ApplicationDefinition`, no release asset names. The website's "developer tooling (the package Makefiles)" trigger is the one worth checking, so I read `content/en/docs/next/development.md` there: it documents `make test` in general terms and does not go stale from a package gaining that target. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added automated validation for cert-manager and etcd operator CRD charts. * Confirmed rendered resources are correctly identified as CustomResourceDefinitions. * Verified CRDs retain the required `keep` resource policy during Helm operations. * **Chores** * Added convenient test commands for running Helm chart validation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 17:47:24 +03:00
Aleksei Sviridkin	177da7843d	fix(kubernetes): keep tenant addon CRDs when a release is uninstalled (#3586) ... ## What this PR does Four tenant addon HelmReleases render their CRDs from a chart's `templates/` directory and remediate a failed install by uninstalling the release. An uninstall deletes what the chart rendered, and deleting a CRD makes the API server garbage-collect every custom resource of that kind, so a failed reinstall takes every Gateway, VerticalPodAutoscaler, ServiceMonitor or VolumeSnapshot in the tenant cluster with it. This annotates the CRDs of the four packages behind them, `gateway-api-crds`, `vertical-pod-autoscaler-crds`, `prometheus-operator-crds` and `vsnap-crd` (that last one is declared by `helmreleases/volumesnapshot-crd.yaml`), with `helm.sh/resource-policy: keep`, which is what `cert-manager-crds` and `etcd-operator-crds` already carry in this tree. Three of the four regenerate `templates/` from upstream on `make update`: two delete the directory outright and the third overwrites its file. So the annotation is written into the vendored files and reapplied by a step in each `update:` target. Without that second half the next regeneration drops it silently, with nothing reporting the loss. I ran `make update` for real to check the pair holds: `gateway-api-crds` and `vsnap-crd` come back byte-identical to what is committed here, and `vertical-pod-autoscaler-crds` reproduces the annotation on both of its CRDs. The rest of the VPA diff is upstream drift, because that package's `update:` tracks a branch rather than a pinned ref; the bump itself is not in this PR. The step is idempotent, so running it twice does not stack a second annotation into the same mapping. `prometheus-operator-crds` ships its CRDs in a vendored subchart that `make update` re-pulls, so nothing under `charts/` is edited. The upstream template already renders `.Values.annotations` onto every CRD it emits, and the annotation is set through the package's `values.yaml`. The Gateway API kustomization also emits a `ValidatingAdmissionPolicy` and its binding. Those hold no user data, so the stamping step selects on `kind: CustomResourceDefinition` and leaves them alone. Each package gains a helm-unittest suite and the `test:` target that `hack/helm-unit-tests.sh` requires before it will run a suite at all. Every suite asserts the annotation across all CRDs its chart renders, not only the ones listed by name, so a CRD arriving in a later upstream bump is covered without editing the test. The annotation has a cost worth stating plainly: uninstalling one of these releases on purpose now leaves the CRDs behind, to be removed by hand. That is the same trade-off `cert-manager-crds` already carries. This covers the first of the two directions in #3555. The second one, setting `install.strategy.name: RetryOnFailure` on these releases, is a separate decision with a different blast radius and is not part of this PR. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: I walked the trigger map in `docs/agents/contributing.md` against the file list of this diff. It touches four packages under `packages/system/` and nothing else: no `hack/package.mk`, no app `values.schema.json`, no platform values, no `ApplicationDefinition`, no release asset names. The one line that looked close is the website's "developer tooling (the package Makefiles)" trigger, so I read `content/en/docs/next/development.md` in the website repo: it documents `make update` and `make test` in general terms, and neither goes stale from this change. ### Release note ```release-note fix(kubernetes): tenant addon CRDs for Gateway API, VerticalPodAutoscaler, Prometheus Operator and VolumeSnapshot now carry `helm.sh/resource-policy: keep`. A failed install that remediates by uninstalling the release no longer deletes those CRDs and, with them, every custom resource of those kinds in the tenant cluster. In exchange, uninstalling one of these releases on purpose now leaves the CRDs in the cluster to be removed by hand. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Custom Resource Definitions for Gateway API, Prometheus Operator, Vertical Pod Autoscaler, and VolumeSnapshot are now retained when their Helm charts are uninstalled. * Prevents loss of custom resources and related configuration during chart lifecycle operations. * **Tests** * Added automated validation confirming all included CRDs receive the retention policy. * Added chart test commands to simplify verification of CRD updates. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 17:47:14 +03:00
Aleksei Sviridkin	c598067160	fix(etcd): fail the CRD vendoring when the keep stamp misses (#3595) ... ## What this PR does `make update` in `packages/system/etcd-operator-crds` vendors three CRDs and stamps `helm.sh/resource-policy: keep` onto each with an awk step anchored on the `controller-gen.kubebuilder.io/version:` line. That line is upstream's to emit. If a future controller-gen stops writing it, the awk matches nothing, writes the CRD out unstamped and exits zero, so the regeneration reports success and the CRDs land without the annotation that keeps a chart uninstall from deleting them, and with them every EtcdCluster, EtcdMember and EtcdSnapshot in the cluster. The step now counts what it produced and requires exactly one annotation per CRD. Zero means the anchor is gone; more than one means it matched somewhere unintended. Every CRD is staged in a temporary directory and `templates/` is replaced only after all of them have been counted, so a failure on any one leaves the vendored files exactly as they were rather than wiping the directory and refilling part of the set. I checked it by substituting the upstream response, so what ran was the recipe that ships rather than a copy of its logic: a CRD with no controller-gen line fails with `stamped 0 keep annotations, expected 1`, a CRD carrying that line twice fails with 2, a failure on the second of the three leaves all three committed files untouched, and the real upstream still vendors byte-identical output, with `make update` leaving `git status` showing nothing but this Makefile. On `main` today this is the only thing guarding those three CRDs. A helm-unittest suite that fails when one of them renders without the annotation is in #3590, which is open and not merged. Once that lands the two catch the same loss at different moments: the suite on a test run, this step in front of whoever ran the regeneration. The same stamping shape exists in three more packages, but only on the branch of #3586, which adds it to `gateway-api-crds`, `vsnap-crd` and `vertical-pod-autoscaler-crds`. There is nothing to guard there on `main` yet, so they are deliberately out of this change and need the same step once #3586 merges. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: I walked the trigger map in `docs/agents/contributing.md` against the file list of this diff, which is one package Makefile and nothing else. `hack/package.mk` is untouched, so the `ccp` and `external-apps-example` triggers that key on it do not fire, and the website's "developer tooling (the package Makefiles)" line is the only near miss: `content/en/docs/next/development.md` describes `make update` in general terms and does not go stale from a package validating its own output. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved CRD update validation to ensure generated definitions include the required preservation annotation. * Prevented incomplete or invalid updates from replacing existing CRD templates. * Added automatic cleanup of temporary update files. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 17:36:32 +03:00
Aleksei Sviridkin	ab8675161c	fix(kubernetes): bound the remaining pre-delete hook deletions (#3604) ... ## What this PR does Three of the six steps in the `kubernetes` pre-delete hook call `kubectl delete` with neither `--wait=false` nor `--timeout`. `--wait` defaults to true and `--timeout=0` is implemented as a week, so the KamajiControlPlane delete in step 3, the DataVolume delete in step 5 and the LoadBalancer Service delete in step 6 each park the hook for 168 hours on one stuck finalizer. A DataVolume held by a CDI importer or a PVC finalizer is the reachable case. The waits in steps 2 and 4 were bounded earlier, in response to a report that named the child-HelmRelease wait and nothing else. The fix stopped where the report did, so the shape it described stayed in the three steps nobody had looked at. None of the three waits now. Nothing downstream reads the result of steps 5 and 6. What matters after step 3 is the TenantControlPlane, and step 4 deletes that one directly and waits for it, so blocking on the KamajiControlPlane first only adds a second place for a finalizer to stall. An expired wait and `--wait=false` leave the same cluster state, with the same objects finalizing in the background. Not waiting also keeps these steps under `set -e` with no guard, which matters more than it looks. A guard cannot tell an expired wait from a rejected request, so it would report a failed delete as cleanup that happened. With no timeout to expire, every non-zero exit left is a real failure. The `watch` verb goes away for those three resources. kubectl opens a watch only on the wait path, so the grant was unexercised. `helmreleases` and `tenantcontrolplanes` keep it because steps 2 and 4 still wait, and there the verb is load-bearing in a way that is easy to miss: removing it does not produce an error, it hangs the delete. That is the failure this repo already hit once at the tenant level, so the whole rule set is pinned rather than left to the next reviewer of this Role. `backoffLimit` goes to 0, against a default of 6 that `restartPolicy: Never` turns into seven pods in series, each restarting the script from step 1 and each paying its own image pull. ## What this PR does not do It does not add `activeDeadlineSeconds`, and that is arithmetic rather than oversight. A deadline has to sit above the sum of the remaining waits, or it cuts runs that were going to finish, and far enough under helm-controller's uninstall budget to be the thing helm reports rather than something helm waits through. Those two bounds do not currently leave a gap, and the image pull comes from Docker Hub with no mirror in front of it. Room comes from raising `spec.uninstall.timeout` on the generated HelmRelease first; squeezing the waits further is the wrong direction. Worth stating plainly, because it is the one change here that is not purely a tightening: with `backoffLimit: 0` a pod eviction now fails the whole uninstall attempt, where six retries previously absorbed it. That is deliberate. Both remaining waits are guarded, so an attempt can spend its entire budget without failing, and a retry would restart from step 1 with the clock already there; on a nodeless tenant it would also re-pay step 4, since the first attempt leaves the TenantControlPlane still terminating. The failure a retry would cover is an eviction, which is not bounded in time and costs most when it arrives late. Flux still repeats the uninstall on its own interval, so the end state is preserved and the trade is wall-clock for predictability. It does not bound the requests. `--wait=false` and `--timeout` bound the waiting, so an apiserver that accepts the connection and then stops answering hangs this hook exactly as before. That is a different failure from the stuck finalizer, it is not made worse here, and it wants one answer across every call rather than a flag on the deletes. ## Tests `packages/apps/kubernetes/tests/delete_hook_test.yaml` gains cases for the bounds and for the Role. Every delete in the hook is asserted to carry one, the Job is pinned to a single attempt, and the rule set is pinned whole. The regex patterns walk only continuation lines, the ones ending in a backslash, so each stays inside the command it names. A cross-line wildcard reaches a flag belonging to the next command instead, and RE2 has no lookahead to exclude the `echo` in between. The Role assertions use whole-rule `contains` plus `lengthEqual` rather than per-index verb checks. `notContains` passes when its path does not resolve, so an index-addressed "and nowhere else" goes quiet the moment the rules list is reordered or shortened, which is exactly the edit it exists to catch. Matching the entire rule pins the verb set in both directions and names the resource when it fails. Every pinned value was mutation-tested: dropping `--wait=false` from each of the three deletes, dropping `--timeout` from either of the two that still wait, `backoffLimit` in both directions, adding `watch` back, removing `watch` from a resource that waits, appending a rule past everything asserted, and reordering the rules list. Each one fails the assertion that claims that ground and no other. ### Screenshots None. This changes a Job manifest and its tests, with no user-visible surface. ### Downstream repositories Walked the trigger map against the diff. The diff is one Job manifest inside the kubernetes chart and its helm-unittest suite. No package is added, renamed or removed; no `values.schema.json`, `values.yaml` default or version enum changes; no `ApplicationDefinition` semantics, namespace, variant or release asset changes; nothing under `hack/` is touched and no make target changes behaviour. - [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(kubernetes): bound the pre-delete hook's remaining unbounded deletions and limit its Job to a single attempt, so a stuck finalizer on a DataVolume, LoadBalancer Service or KamajiControlPlane no longer parks the hook and blocks the uninstall ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Kubernetes resource cleanup during deletion to avoid indefinite waits. * Cleanup operations now complete with bounded waiting and fewer retry attempts, improving reliability and responsiveness. * Removed unnecessary access permissions from cleanup operations, strengthening security. * **Tests** * Added coverage to verify deletion timeouts, retry behavior, and required permissions across supported resources. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 17:36:23 +03:00
Aleksei Sviridkin	b2e9e67ac6	test(apps): anchor index-addressed negative asserts to what they name (#3606) ... ## What this PR does Four negative assertions in two suites address an element by index. `notContains` passes whenever its path resolves to something that simply lacks the forbidden content, and a different element at the same index qualifies. So if the Kafka Role's rule order changed, or a nodePool were added ahead of the one the OpenSearch test names, those guards would go green while the resource they forbid was live on the element they meant. This asserts the addressed element before asserting what it must not have. The Kafka Role rules carry no identifier, so the anchor is the in-cluster bootstrap Service that rule must keep. The OpenSearch pool does have one, `component`, and that is what is pinned rather than a role: roles are shared between pools, so a role-based anchor would be satisfied by a second data-bearing pool inserted ahead of this one. The anchors turn out to cover more than the ordering shift they were written for. `contains` fails on a path that does not resolve, where `notContains` passes, so a positive assert on the same path also catches the document-index half of the same defect. Swap the Role and RoleBinding documents in the Kafka template and all three guards go red, where before they stayed green. ## What this PR does not do It does not raise the guards above the index. A `rules[0]`-scoped assertion cannot express "the Role must not grant this Service at all", because a grant reappearing on a rule appended further down satisfies both the anchor and the negative. That ceiling is inherent to index-scoped assertions and is unchanged here. It does not touch the two cases in these files that were already paired with a positive assert on the same path, and it does not extend to the pure-negative tests elsewhere in the same suites, which are tracked as their own sweep. ## Tests The change is itself a test fix. Both suites pass: kafka 35/35, opensearch 58/58. Each anchor was checked against the shift it exists to catch and against the same shift with the anchor removed. Without them the blocks stay green while the forbidden content is live on the element they name; with them the blocks fail. The OpenSearch mutation deliberately inserts a data-bearing pool, which is what distinguishes the `component` anchor from a role-based one. ### Screenshots None. This changes test files. ### Downstream repositories Walked the trigger map against the diff. Two helm-unittest files, additive only, no chart output changes. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved validation of Kafka resource mappings by confirming the intended internal service rule before checking external-service restrictions. * Strengthened OpenSearch node-role checks by confirming the intended node pool before validating its supported roles. * These updates reduce the risk of false-positive test results when configuration rules or components are reordered. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 17:36:03 +03:00
Aleksei Sviridkin	4e3a679e7b	test(apps): anchor index-addressed negative asserts to what they name ... notContains passes whenever its path resolves to something that simply lacks the forbidden content, and a different element at the same index qualifies. These guards would go green if the Role's rule order changed or a nodePool were added ahead of the one they name, while the resource they forbid came back on the element they meant. Assert the addressed element before asserting what it must not have. The Kafka Role rules have no identifier, so the anchor is the in-cluster bootstrap Service that rule must keep. The OpenSearch pool does have one, `component`, and that is what is pinned: roles are shared between pools, so a role-based anchor would be satisfied by a second data-bearing pool inserted ahead of this one. Each anchor was checked against the shift it exists to catch, and against the same shift with the anchors removed: without them the blocks stay green while the forbidden content is back on the element they name. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 15:35:43 +03:00
Aleksei Sviridkin	8aadbd5731	test(backupstrategy-controller): anchor the metrics-absence guard to its container ... The two notContains assertions in the metrics.enabled=false case address the controller by index. notContains passes whenever its path resolves to something that lacks the forbidden content, and a different container at that index qualifies, so a sidecar inserted ahead of the controller leaves both assertions green while the flag they forbid comes back on the real container. Assert the container's name at the same index first. That fails when the index stops pointing at the controller, which is the case the negatives structurally cannot see. Verified by rendering a sidecar ahead of the controller: before this change the case stayed green and only unrelated tests in the same file went red, by accident rather than by design; after it, the anchor fails. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 15:08:56 +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
Aleksei Sviridkin	b60c08f110	fix(kubernetes): bound the remaining pre-delete hook deletions ... Three of the six steps in the pre-delete hook call kubectl delete with neither --wait=false nor --timeout. kubectl defaults --wait to true and reads --timeout=0 as "wait forever", which it implements as a week, so the KamajiControlPlane delete in step 3, the DataVolume delete in step 5 and the LoadBalancer Service delete in step 6 each park the hook for 168 hours on a single stuck finalizer. A DataVolume held by a CDI importer or a PVC finalizer is the reachable case. None of the three waits now. Nothing downstream reads the result of steps 5 and 6, and what matters after step 3 is the TenantControlPlane, which step 4 deletes and waits for directly; blocking on the KamajiControlPlane first only adds a second place for a CAPI finalizer to stall. An expired wait and --wait=false leave the same cluster state either way, with the same objects finalizing in the background once the hook has moved on. Not waiting also keeps these steps under `set -e` with no guard. A guard cannot tell an expired wait from a rejected request, so it would report a failed delete as cleanup that happened; with no timeout to expire, every non-zero exit left is a real failure. Drop the watch verb from the three resources those steps delete, and pin the whole rule set. kubectl opens a watch only on the wait path, so the grant was unexercised for the three; for helmreleases and tenantcontrolplanes its absence would not error but hang, which is the failure mode hardest to read from the outside. Set backoffLimit to 0, against a default of 6 that restartPolicy Never turns into seven pods in series. Zero rather than one because both remaining waits are guarded, so an attempt can spend its whole budget without failing and a retry would start from step 1 with the clock already there; on a nodeless tenant it would also re-pay step 4, since attempt one leaves the TenantControlPlane still terminating. The failure a retry would cover is an eviction, which is not bounded in time and is worst exactly when it arrives late. The price is that such an eviction fails the whole attempt and flux repeats it on its own interval. No activeDeadlineSeconds: the waits and helm-controller's uninstall budget leave no gap a deadline could sit in, once a Docker Hub image pull is counted. That needs the budget raised first, through spec.uninstall.timeout on the generated HelmRelease, and is left out rather than set to a number that would cut runs which were going to finish. These bounds cover the waiting, not the request: --request-timeout is still 0 on every call in the hook, so an unresponsive apiserver hangs it as before. That is a different failure from the stuck finalizer, and it wants one answer across every call rather than a flag on the deletes. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:38:17 +03:00
Aleksei Sviridkin	cf85716444	fix(tenant): bound the pre-delete cleanup waits and the hook Job ... kubectl reads --timeout=0 as "wait forever" and substitutes a week, so the two `kubectl delete helmreleases --wait=true` calls in the tenant pre-delete hook hang for as long as any child HelmRelease cannot finalize. helm returns from Uninstall.Run before deleting anything, so the release stays in storage and flux retries the same teardown; helm.sh/hook-delete-policy carries no hook-failed, so the waiting Job survives between attempts as well. Bound each wave at 90s. Neither is guarded: nothing stands behind these deletes, so a wave that did not finish ends the run rather than report a cleanup that did not happen, and ending it keeps the tenant modules from being torn out from under applications that are still deleting. Whatever was deleted stays deleted, so the next attempt resumes from there. The bound has a price and it is not zero. A nested Tenant, or an app of kind Kubernetes whose own hook spends two 120s waits first, misses 90s and converges across uninstall retries instead; neither fits the 300s budget as it stands either. Between those and the fast case sits a band that does pay: a wave inherits its slowest child, so one that settles around 95s completes inside the budget today and expires here instead, trading a clean attempt for one more retry. An unbounded wave does not fail, it spends the whole budget and reports a timeout naming nothing, so the trade is worth taking. Give the Job an activeDeadlineSeconds of 270. It bounds what the waits do not — the DELETE calls carry kubectl's default --request-timeout of 0 — and sits under the 300s uninstall budget flux applies when the HelmRelease carries no timeout of its own, because the deadline runs from the Job's startTime while that budget runs from just before the Job is created. At equal values helm gives up first, on a Job still running. Cap the pod's termination at 10s, which is part of that arithmetic rather than a detail of it. Since 1.31 the Job controller withholds the terminal condition until every pod has finished terminating, so helm learns of the failure at the deadline plus this period; the 30s default would land it at 270+30, level with the budget the deadline is set under. The period is spent in full: the SIGTERM goes to PID 1, which is a shell running a multi-command script, and a default disposition sent to PID 1 is discarded by the kernel with no handler installed to catch it. kubectl is a child and is never signalled. Set backoffLimit to 0 and restartPolicy to Never. The limit is needed under either policy, since the default of 6 keeps the failure from reaching helm inside the budget. The policy matters because under OnFailure the pod survives its own failure and the kubelet re-runs the container in place, with the controller applying the cap only afterwards by summing restartCount: the script re-enters the wave that just expired and is cut down mid-run. Never leaves a failed pod the controller sees directly. Add resource requests to match the sibling hook in packages/apps/kubernetes. Without them the pod is BestEffort and first out under node pressure, which a retry inside the Job used to absorb and a single attempt does not. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:38:06 +03:00
Aleksei Sviridkin	d90a6a5f46	fix(prometheus-operator-crds): keep the CRDs on uninstall ... The tenant addon HelmRelease remediates a failed install by uninstalling the release, and an uninstall deletes whatever the chart rendered. Deleting a CRD makes the API server garbage-collect every custom resource of that kind, so a failed reinstall takes every ServiceMonitor, PodMonitor, PrometheusRule and ScrapeConfig in the tenant cluster with it. The CRDs ship inside a vendored subchart that `make update` re-pulls, so the annotation is set through the chart values the subchart already renders rather than by editing templates that would be overwritten. Removing the release on purpose now leaves the definitions behind to be deleted by hand. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:37:46 +03:00
Aleksei Sviridkin	82f5255f5a	fix(vsnap-crd): keep the CRDs when the release is uninstalled ... The tenant addon HelmRelease remediates a failed install by uninstalling the release, and an uninstall deletes whatever the chart rendered. Deleting a CRD makes the API server garbage-collect every custom resource of that kind, so a failed reinstall takes every VolumeSnapshot and VolumeSnapshotContent in the tenant cluster with it — the objects that carry the only reference to a snapshot on the storage backend. Stamp helm.sh/resource-policy: keep on each CustomResourceDefinition and teach `make update` to reapply it, since that target rebuilds templates/ from upstream and would otherwise drop the annotation silently. Removing the release on purpose now leaves the definitions behind to be deleted by hand. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:37:46 +03:00
Aleksei Sviridkin	5d5d8f58aa	fix(vertical-pod-autoscaler-crds): keep the CRDs on uninstall ... The tenant addon HelmRelease remediates a failed install by uninstalling the release, and an uninstall deletes whatever the chart rendered. Deleting a CRD makes the API server garbage-collect every custom resource of that kind, so a failed reinstall takes every VerticalPodAutoscaler in the tenant cluster with it, along with the recommendation history held in the checkpoints. Stamp helm.sh/resource-policy: keep on each CustomResourceDefinition and teach `make update` to reapply it, since that target refetches the manifest from upstream and would otherwise drop the annotation silently. Removing the release on purpose now leaves the definitions behind to be deleted by hand. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:37:46 +03:00
Aleksei Sviridkin	a822ad10ff	fix(gateway-api-crds): keep the CRDs when the release is uninstalled ... The tenant addon HelmRelease remediates a failed install by uninstalling the release, and an uninstall deletes whatever the chart rendered. Deleting a CRD makes the API server garbage-collect every custom resource of that kind, so a failed reinstall takes every Gateway, HTTPRoute and ReferenceGrant in the tenant cluster with it. Stamp helm.sh/resource-policy: keep on each CustomResourceDefinition and teach `make update` to reapply it, since that target rebuilds templates/ from upstream and would otherwise drop the annotation silently. The ValidatingAdmissionPolicy pair the experimental kustomization also emits holds no user data and stays unannotated. Removing the release on purpose now leaves the definitions behind to be deleted by hand. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:37:46 +03:00
Aleksei Sviridkin	ac2f882829	test(etcd-operator-crds): pin the keep annotation on the CRDs ... The CRDs carry helm.sh/resource-policy: keep, which is what stops a release uninstall from deleting them and taking every EtcdCluster, EtcdMember and EtcdSnapshot in the cluster with it. Nothing asserted that. The annotation is stamped by the awk step in `make update`, anchored on the controller-gen version line the upstream CRDs carry. If that line stops being emitted, the step matches nothing and exits zero. Scope by wildcard rather than a list of filenames, so a CRD added to the vendoring list is covered without editing the test, and assert the kind alongside the annotation, since the annotation assertion on its own accepts a non-CRD document that carries it. Add the test: target too. hack/helm-unit-tests.sh runs a package's suite only when its Makefile defines that target, so without it the fixtures sit unread and the run reports success on a chart that has already lost the annotation. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:36:07 +03:00
Aleksei Sviridkin	ead3ab4ed6	test(cert-manager-crds): pin the keep annotation on the CRDs ... The CRDs carry helm.sh/resource-policy: keep, which is what stops a release uninstall from deleting them and taking every Certificate, Issuer and Order in the cluster with it. Nothing asserted that. The annotation is not local to this package on either side: it comes from the upstream template under a conditional, the value that switches it on is ours, and the templates are refilled from the vendored upstream chart by packages/system/cert-manager on its own update. The assertion reads the rendered output, so it does not depend on which of those moves. Scope by wildcard rather than a list of filenames, so a CRD that upstream starts shipping is covered without editing the test, and assert the kind alongside the annotation, since the annotation assertion on its own accepts a non-CRD document that carries it. Add the test: target too. hack/helm-unit-tests.sh runs a package's suite only when its Makefile defines that target, so without it the fixtures sit unread and the run reports success on a chart that has already lost the annotation. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:36:06 +03:00
Aleksei Sviridkin	c6eb584914	fix(tests): freeze the EXIT-trap count for run-kubernetes-talos-diagnostics_test.bats (#3602) ... ## What this PR does `hack/run-kubernetes-talos-diagnostics_test.bats` landed on main carrying eight EXIT traps and was not added to the frozen set in `hack/cozyreport.bats`, so `found` and `frozen` differ by exactly that one entry and the guard fails on every branch cut from main since then. This adds the entry. That is what the comment above the guard prescribes for a file that did not exist when the freeze was written: the counts are expected to move, and the file is updated rather than converted, because it is owned by another branch. The guard behaved correctly here, it noticed. ## Why it is worth fixing on its own rather than waiting The red check is the smallest part of the cost. `hack/cozytest.sh` stops at the first failing test in a file, and this guard is test 83 of 166 in `cozyreport.bats`, so the remaining 83 never run. The Makefile loop is `for f in $(BATS_UNIT_FILES); do hack/cozytest.sh "$f" || exit 1; done`, which aborts the whole loop rather than the single file, and `cozyreport.bats` sorts fourteenth of forty-two, so twenty-eight further files never run either. `finalize` needs `checks`, and the e2e job needs `finalize`, so `E2E Tests` ends up skipped rather than executed, and a skipped required context satisfies branch protection. The practical effect is that a PR can be green enough to merge while neither its new unit tests nor e2e have run. ## Verification `bats` walks past a failure where `cozytest.sh` stops, so running the file under `bats` shows what is behind the cut. With this entry present, `cozyreport.bats` is 166 of 166. I also ran the twenty-eight files that sort after it: 368 tests pass and 15 fail, all 15 confined to `migration-seaweedfs-db-adopt.bats` and `seaweedfs-naming-audit.bats`, both for local reasons on macOS (a docker bind mount that does not resolve, and BSD versus GNU column formatting). Both are pre-existing on main and unrelated to this change, so nothing has rotted behind the cut while it was unreachable. ## Note on overlap `#3575` carries the same one-line change as part of a larger feature. This PR exists so that main can be unblocked without waiting for that one, and the two will resolve to the same line. ### Downstream repositories Walked the trigger map against the diff. This changes one string inside a bats guard that exists only in this repository, so nothing downstream sees it. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ```	2026-08-07 14:11:06 +03:00
Aleksei Sviridkin	c78326281e	fix(tests): freeze the EXIT-trap count for run-kubernetes-talos-diagnostics_test.bats ... hack/run-kubernetes-talos-diagnostics_test.bats arrived carrying eight EXIT traps and was not added to the frozen set, so `found` and `frozen` disagree by exactly that one entry and the guard fails on every branch cut from main. Updating the count is what the comment above the guard prescribes for a file that did not exist when the freeze was written. The guard did its job: it noticed. The cost is wider than one red check. hack/cozytest.sh stops at the first failing test in a file, and the guard is test 83 of 166 in cozyreport.bats, so the remaining 83 do not run. The Makefile loop is `for f in $(BATS_UNIT_FILES); do hack/cozytest.sh "$f" || exit 1; done`, which aborts the whole loop, and cozyreport.bats sorts fourteenth of forty-two, so twenty-eight files do not run either. Downstream of that, finalize needs checks, and the e2e job needs finalize, so E2E Tests is skipped rather than executed. Verified by running the file to completion with bats, which walks past a failure unlike cozytest.sh: 166 of 166 pass with this entry present. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 14:09:43 +03:00
Aleksei Sviridkin	6c82988570	fix(tests): freeze the EXIT-trap count for multus-install-cni-plugins.bats (#3584) ... <!-- 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 `hack/multus-install-cni-plugins.bats` arrived on main in 7739d1e29 carrying 12 EXIT traps, one day before the EXIT-trap ratchet in `hack/cozyreport.bats` was written, so the frozen list never learned about it. The guard therefore fails on main itself, and on every PR whose CI builds a merge with main — `make bats-unit-tests` exits 1 with: ``` FAIL: the set of unconverted EXIT-trap files changed. frozen: ... nightly-mirror_test.bats=5 ... found: ... multus-install-cni-plugins.bats=12 nightly-mirror_test.bats=5 ... ``` This records the count, which is what the guard's own comment prescribes for exactly this case: *"a file that did not exist when this guard was written arrives carrying its own \[traps\] ... Counts are updated rather than the files converted: all of them are owned by other branches, and a conflict there costs more than an uncovered trap."* Converting the file is not in scope here and belongs to whoever owns the multus branch. Verified by reproducing the guard's logic against a pristine checkout of `main` at 879d0f6db: the `found` set differs from `frozen` by exactly this one entry before the change, and matches after it. Found while investigating a red `Unit & controller tests` on #3571, which carries none of this. ### Screenshots No UI changes. ### 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 ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated the expected EXIT-trap inventory to include `multus-install-cni-plugins.bats`. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 10:42:43 +03:00
IvanHunters	318be935ca	fix(e2e): capture Talos logs for tenant worker join failures (#3548) ... ## What this PR does Addresses #3513. When tenant Kubernetes workers miss the 18-minute Ready deadline, the E2E runner now mints a one-hour `os:reader` Talos client certificate from the tenant's existing issuer and runs authenticated `talosctl` from a hardened same-namespace diagnostics Pod. It captures bounded `dmesg` and kubelet output for every worker VMI in the cozyreport snapshot, records missing IPs and command exit codes, continues across unreachable workers, and preserves the original test failure. The diagnostics Pod has no service-account token or Secret volume; credentials are streamed into an `emptyDir`, and the Certificate, Secret, and Pod are labeled for cleanup. Workers that have not reached Talos `apid` can only yield a bounded connection error, while workers that reached `apid` provide guest logs. This adds the evidence needed to distinguish the remaining guest boot failure but does not claim to resolve its underlying cause. The readiness budget and success path are unchanged. Because this changes a shared E2E helper, test-impact analysis selects the full E2E suite for this PR. ### Screenshots Not applicable; no UI changes. ### Testing - `hack/cozytest.sh hack/run-kubernetes-talos-diagnostics_test.bats` (10 tests) - `hack/cozytest.sh hack/run-kubernetes-drain_test.bats` - `hack/cozytest.sh hack/cozyreport-talos.bats` - `bash -n hack/e2e-chainsaw/_lib/run-kubernetes.sh` - `git diff --check origin/main...HEAD` The full `make bats-unit-tests` run reached the unrelated `hack/migration-seaweedfs-db-adopt.bats` suite and could not continue because Docker is unavailable in the development environment. A live failure-path run was not performed because the available development environment has no Talos tenant fixture; this PR is intended to exercise that path in Cozystack CI. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(e2e): capture authenticated Talos kernel and kubelet diagnostics when tenant Kubernetes workers miss the Ready deadline ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved troubleshooting when worker nodes fail to join by collecting per-node operating system and kubelet diagnostics. * Diagnostics now continue across partial failures and clean up temporary resources afterward. * Extended Kubernetes test operation timeouts to allow failure diagnostics to complete reliably. * **Tests** * Added regression coverage for diagnostic collection, timeout handling, multi-worker scenarios, missing node addresses, and cleanup. * **Chores** * Ensured the required diagnostic image is preloaded on nodes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-07 10:40:03 +03:00
Aleksei Sviridkin	ed6135d77d	fix(etcd): fail the CRD vendoring when the keep stamp misses ... `make update` stamps helm.sh/resource-policy: keep onto each vendored CRD with an awk step anchored on the controller-gen version line, which upstream owns. If upstream stops emitting that line the step matches nothing, writes the CRD out unstamped and exits zero, so the release quietly loses the protection that keeps a chart uninstall from deleting the CRDs and every EtcdCluster, EtcdMember and EtcdSnapshot under them. Count the annotations the step produced and require exactly one per CRD. Zero means the anchor is gone; more than one means it matched somewhere unintended. Stage every CRD in a temporary directory and replace templates/ only once all of them are counted, so a failure on any one leaves the vendored files exactly as they were instead of wiping the directory and refilling part of the set. Verified by substituting the upstream response: a CRD with no controller-gen line fails with "stamped 0 keep annotations, expected 1", one carrying the line twice fails with 2, a failure on the second CRD leaves all three committed files untouched, and the unmodified upstream still vendors byte-identical output. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 08:11:39 +03:00
Aleksei Sviridkin	0827bcb300	fix(apps/kubernetes): report the pre-delete cleanup outcome honestly ... The pre-delete cleanup Job ended with an unconditional "Cleanup completed successfully". Several of its steps swallow a failure behind `|| echo "WARNING: ..."` so that `set -e` cannot end the run, so that line was printed in exactly the situation the warnings exist to report: child HelmReleases still holding finalizers, or a Kamaji datastore Secret still pinning its namespace in Terminating. Record what a step leaves behind and close with either the success line or the list of steps that left something. Residue is the thing worth recording, not every fallback: a bounded delete that times out has a remedy right behind it -- Step 2 falls through to force-clearing finalizers, Step 4 falls through to Step 4b -- and whether anything survived is decided by that remedy, which reports its own outcome. A tenant with no working nodes takes that route as a matter of course, which is why the wait is bounded in the first place, and calling such a run incomplete would send an operator hunting for objects that are not there. A read that fails looks exactly like one that found nothing: an empty list and a failed list both give a loop nothing to iterate, and an absent object and an unreachable apiserver both make a probe non-zero. Every read the summary rests on now keeps its status. Step 2's force-clear re-list reports when it cannot list what it was about to clear, because everything it could not return is still holding its finalizer. Step 4b's two probes became three-way -- present, absent or unknown -- so a Secret whose existence could not be established is no longer announced as absent. Step 1's list is the one read still swallowing its status, and that is a decision rather than an omission: Step 2 deletes by label rather than from that list, so a suspend that never happened leaves nothing of its own behind and reporting it would name residue that does not exist. Making the TenantControlPlane probe three-way also repairs the gate above it, and that is intended rather than incidental. The gate exists so the datastore Secret's finalizer is not stripped while Kamaji may still own it, and a two-way probe defeated it in the one direction that does damage: any error reads as "confirmed absent" and the strip goes ahead. Its likeliest cause, a throttled or briefly unreachable apiserver, is also likeliest exactly when a broken tenant is being torn down. Step 4b declining to strip a finalizer while the TenantControlPlane is still alive is the one residue that no failing command marks: every call in that branch succeeds and the namespace stays in Terminating regardless. It now reports, and names the TenantControlPlane as the thing holding the namespace up. The exit code is deliberately unchanged and still zero. These are backstops, not the main path: they run after the normal deletion has already timed out, and they exist to push teardown further along, so failing there would abort the Helm uninstall at the very moment the backstop was written for, leaving the release in storage and the same teardown retried forever. That makes the closing message the only outcome report an operator gets, which is why it has to be accurate. The steps that are not backstopped still end the run under `set -e` and never reach that message. Rework the runtime warning and the RBAC comment that pointed at the tracking issue for the Kamaji datastore-secret leak. That issue is closed, and closed by this very step rather than by a fix in Kamaji, so a reader who followed the pointer found a resolved report and no reason for the code to exist. The step comment now says the ordering it covers is still reachable, and the warning names the finalizer and the object to clear by hand. Cover the behaviour with a bats file that renders the Job, extracts the script and runs it against a stub kubectl, since the helm-unittest cases match the script's source text and cannot tell whether the closing message is reachable. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 06:49:56 +03:00
Aleksei Sviridkin	8aa30472b6	docs(ci): correct the job-level permissions comment ... The comment above the top-level `permissions` block claimed that jobs request the minimum extra scopes on top of it. A job-level block does not add to the workflow-level one: it replaces it, and every scope the job does not name is set to none. A job needing one extra scope has to restate `contents: read` alongside it or lose read access to the repository. The corrected wording states the replace semantics, so the next job block written against this comment grants the whole set it needs rather than only the delta. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-07 00:34:53 +03:00
Aleksei Sviridkin	b4e203164a	fix(kubernetes): retry the tenant CSI install instead of uninstalling it (#3581) ... ## What this PR does The tenant CSI HelmRelease carried `remediation.retries: -1` with no strategy, so Flux applied its default, `RemediateOnFailure`. Install remediation is an uninstall in every case the API offers, so an install that missed its budget was torn down between attempts: the tenant lost its CSI node plugin and the StorageClasses this release propagates until a later attempt applied them again, and that attempt restarted the DaemonSet rollout instead of resuming one already under way. That teardown races the rollout it exists to recover, and the run recorded in #3576 shows how narrowly. Both DaemonSet pods were scheduled at the same second on both workers, so nothing was waiting on capacity; the ten minutes went to image pulls. The slower worker started its last container six seconds after the install timeout fired, and the uninstall removed it one second after that. `RetryOnFailure` leaves the manifests applied and retries the failed release. Both actions carry it because the controller picks the active retry from `.status.lastAttemptedReleaseAction` and performs that retry as an upgrade: set on install alone, the retry still happens, but its own failure is then handled by upgrade remediation, whose default is a rollback with no previous release to roll back to. The `remediation` blocks stay, because an absent release consults `install.remediation` and an out-of-sync one consults `upgrade`, both by retry count rather than by strategy. This is the change #3552 makes for the tenant CNI, and the reason for putting the strategy on both actions is the one given there. It is also the shape upstream prescribes: the "Recommended settings" block in the [HelmRelease spec shipped with the helm-controller this installs](https://github.com/fluxcd/helm-controller/blob/v1.5.1/docs/spec/v2/helmreleases.md#recommended-settings) puts `strategy.name: RetryOnFailure` under both `install` and `upgrade` for production deployments. Nothing changes on the test side, and that is deliberate rather than deferred. Because no manifest is removed at the attempt boundary, the rollout continues across it and the kubelet finishes what it started, while the retry re-applies the same manifests every 30s. The release therefore reports Ready once the rollout completes rather than once a whole attempt has fit inside the timeout, so what the existing e2e wait has to outlast is the rollout and no longer an attempt boundary that discards it. That widens what the wait tolerates without making it unbounded: a rollout slower than the wait itself still fails, and this does not claim otherwise. Scope. #3576 records more than this fixes, so this does not close it. The other tenant addon HelmReleases this chart renders keep the remediation-only shape; CSI is the one this changes. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: The trigger map was walked against the diff. The change is a HelmRelease template and a chart test, not a `values.schema.json` field, a `values.yaml` default, a version enum, a `release.prefix`, an `ApplicationDefinition`, a chart source kind or a namespace. ### Release note ```release-note fix(kubernetes): the tenant CSI HelmRelease now retries a failed install instead of being uninstalled between attempts, so a failed install no longer takes the tenant's CSI node plugin and its propagated StorageClasses away while it retries. The same strategy applies to the upgrade action, which it has to: a retried install runs as an upgrade. One consequence is operator-visible — a failed CSI upgrade in a tenant cluster now retries the new manifests every 30s instead of rolling back to the previous release. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved reliability when installing or upgrading the CSI component by automatically retrying failed actions every 30 seconds. * Retains unlimited remediation retries to help recover from transient failures. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-06 23:47:03 +03:00
Aleksei Sviridkin	0dae3ca95a	fix(e2e): budget tenant scheduling separately from workload readiness (#3579) ... ## What this PR does The tenant backend Deployment in the kubernetes suites had one 300s budget to reach `condition=Available`, and two unrelated variable costs shared it. The first is scheduling. What the suite establishes before that point is two tenant nodes Ready, which is weaker than schedulable: a Ready node still carries `node.cilium.io/agent-not-ready` until the tenant cilium agent claims it, and a node the bringup has not finished with is `SchedulingDisabled`. In the run recorded in #3577 scheduling took 2m18s and 1m57s in the two suites, which left the image pull to finish inside what remained. It did not, and both suites reported the same `timed out waiting for the condition` for what were two different shortfalls. So this waits for a node that actually accepts a Pod before creating the workload, on its own budget and its own failure message. The gate encodes the scheduler's rule for a Pod that tolerates nothing (Ready, not unschedulable, no `NoSchedule` or `NoExecute` taint) over a `custom-columns` probe, prints the node table on both outcomes so a timeout names the taint that held it, and treats a failed probe as not-schedulable so an API blip cannot release it. On the happy path it adds no wall time: it spends the seconds the Pod would otherwise spend Pending, plus at most one 5s poll interval. On a failing run the two budgets stack, so a run that exhausts both now gives up at ~600s where it used to give up at 300s, inside the 40m Chainsaw script op the suites document as a ~25m bringup. The readiness wait keeps its 300s, now starting from a schedulable node, and dumps deployment, pod and event state when it runs out. The workload image is also pinned by digest. The tenant workers reach no Docker Hub mirror (`hack/e2e-talos-image-cache.yaml` serves the Talos worker OS disk image over HTTP and is not a registry mirror), so nginx is pulled from Docker Hub on every run either way. The digest does not take the pull off the critical path; it fixes what that pull returns instead of leaving a floating tag free to change size and content under a fixed deadline. Preloading the image would need infrastructure this tree does not have, and is not attempted here. Nothing will bump the pin: no renovate manager reads `hack/`, and the comment says so, because for a throwaway test workload the freeze is the point. What this does not claim: it removes two measured consumers from a fixed budget, both taken from the events of a single run. Whether that budget was the only thing making those suites red is not established from one run, so this is not offered as the fix for a red pipeline. `hack/run-kubernetes-schedulable_test.bats` covers the new logic: every branch of the predicate, the multi-node scan, the poll-again path, the deadline, and a probe that fails. Each test was verified by mutating the helper and checking that the intended test, and no other, went red. Note for whoever reviews alongside #3575: that branch mirrors `ghcr.io` for tenant worker pulls, not `docker.io`, so it does not change the Docker Hub pull described here. `git merge-tree` reports no conflict between the two, nor with #3548. Observed in #3577. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: The trigger map was walked against the diff. The change is confined to `hack/e2e-chainsaw/_lib/run-kubernetes.sh` and one new `hack/*.bats` unit test. It moves and renames nothing under `hack/`, changes no make target, and does not touch `hack/e2e-prepare-cluster.bats`, any package, any CRD or any namespace name, so none of the listed repositories are reached. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Kubernetes deployment readiness by waiting for a suitable, schedulable node before timing backend startup. * Added clearer diagnostics when nodes are unavailable or backend readiness fails. * Replaced the floating backend container image tag with a fixed, verified version. * **Tests** * Added coverage for node readiness, cordoning, taints, polling, timeouts, probe failures, and deployment sequencing. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-06 23:46:50 +03:00
Mattia Eleuteri	3aef42b993	fix(tests): freeze the EXIT-trap count for multus-install-cni-plugins.bats ... hack/multus-install-cni-plugins.bats arrived on main in 7739d1e29 carrying 12 EXIT traps, one day before the ratchet in hack/cozyreport.bats was written, so the frozen list never learned about it. The guard therefore fails on main itself, and on every PR whose CI builds a merge with it: FAIL: the set of unconverted EXIT-trap files changed. frozen: ... nightly-mirror_test.bats=5 ... found: ... multus-install-cni-plugins.bats=12 nightly-mirror_test.bats=5 ... Record the count, which is what the guard's own comment prescribes for a file that did not exist when it was written: "Counts are updated rather than the files converted: all of them are owned by other branches, and a conflict there costs more than an uncovered trap." Verified by reproducing the guard's logic against a pristine checkout of main: the difference is exactly this one entry, before the change and none after it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Mattia Eleuteri <mattia@hidora.io>	2026-08-06 16:58:08 +02:00
Aleksei Sviridkin	879d0f6db6	fix(installer): make operator readiness honest in the chart and the install gate (#3466) ... ## What this PR does The operator Deployment ran `replicas: 1` with `maxUnavailable: 1`, so the Deployment controller took the minimum available replica count as `1 - 1 = 0` and the `Available` condition reported `True` with no running pod at all. Measured against an operator whose image tag does not exist: the condition was `True` with `reason: MinimumReplicasAvailable` while the only pod sat in `ImagePullBackOff`, `unavailableReplicas: 1`, and no `availableReplicas` key in the status. That broke two separate things, so this fixes both. The E2E install gate asked `kubectl wait --for=condition=Available` and passed in 156 ms against that dead operator. The install then carried on and died about two minutes later on the `packages.cozystack.io` CRD wait further down the file, which sends you after CRD installation when the operator never started. It now uses `kubectl rollout status`, which reads updated and available replicas directly, and dumps the deployment, pods, operator logs and namespace events when it fails. This half is deliberately independent of the chart: `rollout status` is correct under any rollout strategy, so a later change to that strategy cannot quietly make the gate vacuous again. The chart is the other half, and fixing the gate does not cover it. `Available` is read by more than this test, and until the strategy changes an alert on that condition, a health dashboard, or a person reading `kubectl get deploy` all show green while the operator is dead. The strategy moves to `maxSurge: 1` / `maxUnavailable: 0`, which restores the ordinary meaning of the condition. Kubernetes refuses `maxSurge: 0` and `maxUnavailable: 0` together, so the surge has to move for the unavailability to reach zero. That pair is worth reading twice, because it is not a configuration I picked. At `replicas: 1` the Kubernetes default of `25%` / `25%` resolves to exactly `maxSurge: 1` and `maxUnavailable: 0`, since the surge rounds up and the unavailability rounds down. What this PR does is stop overriding the default, not introduce a tuning of its own, and the risk of the change should be judged on that basis. Moving the surge is safe here, and I checked rather than assumed, because the pair came from somewhere. It was carried over from the earlier `cozystack` Deployment, where a sidecar published host port 8123 and `hostNetwork` left the surge pod unschedulable on a single-node cluster. It got here in two steps, which is worth knowing if you go looking for the reason: by the commit before this file existed the sidecar was already gone and no port was declared, yet the pair stayed. It had outlived its own reason before it was ever copied, which is why there is no justification to find in this file's own history. On a single-node cluster the surge pod is scheduled next to the outgoing one and the rollout completes; adding a declared container port reproduces the original `didn't have free ports for the requested pod ports` failure exactly. Nothing else in the pod holds a port either: metrics and health probes are bound to `0`, and the configured webhook server never starts because the operator registers no webhook and never asks the manager for the server. The surge makes leader election load-bearing, though it covers less of the overlap than it first appears. While `maxSurge: 0` capped total pods at `replicas` a second manager process could not exist, so `--leader-elect=true` was defence in depth. With a surge pod the lease is what keeps the incoming manager's reconcile loops off the cluster while the outgoing one is still leader, and because there is no readiness probe the incoming pod counts as available as soon as its container starts, so that overlap covers the whole image pull. It does not cover everything: the operator installs CRDs, installs Flux and creates the platform PackageSource before `mgr.Start()`, and the lease is only taken inside `mgr.Start()`, so those three privileged installs run outside leader election entirely. They are safe because both pods write the same objects through server-side apply under the same field manager, which is a different guarantee from the lease. The `k8s-await-election` wrapper this operator replaced did hold the whole process until it won leadership, so if that property sounds familiar it belongs to the predecessor rather than to this binary. The sibling `opensearch-operator` chart turns leader election off at one replica for a real reason: a transient lease-renewal blip makes controller-runtime exit the process, which at one replica reads as a crashlooping Deployment. This operator installs first, when the apiserver is at its shakiest, so that scenario applies here too. I am accepting the risk knowingly, because after `maxSurge: 1` the alternative is two managers reconciling the same cluster, which is worse. The flag is pinned in the unit test so it cannot be turned off without the test going red. One consequence is permanent, so I am writing it down now instead of leaving it to be found in a year. Because any bound port breaks the surge pod under `hostNetwork`, an `httpGet` readiness probe is off the table for this Deployment for good and only an `exec` probe could ever work. So `Available` still answers "a container started", not "the operator is healthy": an operator that starts and then dies partway through installing CRDs is caught by the CRD waits below the gate, not by the gate itself. The surge also opens one new window. The operator installs CRDs and Flux before the manager starts, which puts that work outside leader election, so the incoming pod runs those applies while the outgoing pod is still leader. Both apply the same objects server-side and the outgoing pod exits moments later. `--wait` is dropped from the helm step in the same change. Helm decides a Deployment is ready with the same `replicas - maxUnavailable` arithmetic, so on the old strategy it was satisfied at zero ready pods and gated nothing, which is why the broken install got past it as well. Once the strategy is honest it would gate for real, but on its own 2m budget, and it would fail the install before the rollout gate ran and hide the diagnostics behind a bare Helm timeout. Hooks are waited for with or without the flag, and the release otherwise carries only the Deployment and its RBAC objects, so the rollout gate is the better single place to decide the operator is up. A helm unittest suite pins the strategy and all four preconditions the surge makes load-bearing: no container or initContainer declares a port, both listener arguments stay disabled, leader election stays on, and no probe uses `httpGet`. That last one follows from the two disabled listeners leaving no port for a probe to reach, and it is the worst of the four to get wrong, because the others wedge an upgrade while a `livenessProbe` against a disabled listener crashloops the pod on every cluster, fresh install included. Only the `httpGet` field is forbidden rather than the probes themselves, so an `exec` probe stays available. All four run against all three chart variants, because the template branches on variant inside the container spec and a port added under one branch would render for that variant alone. I checked the suite fails for the right reasons instead of trusting it. Reverting the strategy, declaring a `containerPort` on the operator container, declaring one under a single variant branch, declaring one on an initContainer, pointing a bind-address at a real address, turning leader election off, and adding a readiness, liveness or startup probe with `httpGet` each turns it red on its own assert. Inserting a container ahead of the operator trips the container-name pin before the argument asserts can start describing the wrong container. An `exec` probe correctly stays green. The rationale is split by audience. What an operator applying the rendered manifest has to respect stays in plain comments and travels into `_out/assets/cozystack-operator-*.yaml`: why the strategy is what it is, that no port may be declared or bound, and that leader election has to stay on. Everything that is only about this repository moves into a `{{/* */}}` template comment that stops at the chart boundary. That covers the pointer to the test and which assert pins what, where the strategy was copied from, why the webhook server on 9443 is not a third port binder, and why `opensearch-operator` reaches the opposite conclusion on leader election. The other templates in this chart already keep maintainer notes that way. ### Screenshots Not applicable, no UI changes. ### Downstream repositories I left the boxes empty on purpose rather than to skip the question, because the honest answer is not one of them and someone else should make the call. Walking the trigger map in `docs/agents/contributing.md`, nothing fires. The `hack/` triggers name `hack/e2e-prepare-cluster.bats`, `hack/package.mk`, `hack/update-crd.sh` and moves or renames under `hack/`, and this PR only edits the body of `hack/e2e-install-cozystack.bats`. The `ansible-cozystack` trigger is renaming an object the role waits on, and `deployment/cozystack-operator` keeps its name. The behaviour that role sees does change, though. It runs `kubectl wait deployment/cozystack-operator --for=condition=Available`, the same vacuous check this PR replaces here, so the chart change turns its wait from one that returns instantly into one that means something. Its `cozystack_operator_wait_timeout` defaults to 300 seconds, the same budget I gave the gate here, so a cold image pull fits and I do not think anything needs to change over there. No follow-up PR seemed warranted for a change that only makes an existing check work, but that is a judgement call rather than a trigger, which is why the boxes are empty. - [ ] 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(installer): the cozystack-operator Deployment no longer reports Available with zero running pods. It ran replicas: 1 with maxUnavailable: 1, which made the minimum available replica count 0, so the condition stayed True while the operator was dead and anything reading it, including the E2E install gate, saw green. The rollout strategy is now maxSurge: 1 / maxUnavailable: 0 and the install gate waits for the rollout instead of the condition. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved operator installation checks by monitoring deployment rollouts and providing deployment, pod, log, and event details when a rollout fails. * Updated operator upgrades to maintain availability during rollout, including in single-node environments. * Documented rollout requirements and behavior across supported installation configurations. * **Tests** * Added coverage for rollout settings and upgrade compatibility across supported installation variants. * Clarified Helm unit-test directory coverage documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-06 15:25:39 +03: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	a20f3620b1	fix(e2e): select catch pod logs by the nested release instance label (#3564) ... ## What this PR does The harbor, openbao and qdrant catch blocks selected pod logs on the app release name. Each of these charts deploys its workload through a nested HelmRelease named `<release>-system` and passes `fullnameOverride: <release>` down to it, so the inner chart stamps `app.kubernetes.io/instance` from its own release name, with the suffix, while object names come from the override, without it. The selector matched no pods, and a failed run collected nothing. - points the three `podLogs` catch collectors at the `-system` instance label - adds a `podLogs` collector for Harbor's CNPG database, which no collector reached before, selected on `cnpg.io/cluster` rather than on the instance label CNPG merely derives from the Cluster name - pins the instance label on a resource in an assert each suite already runs, so a chart renaming that label fails a step instead of quietly emptying a capture - prints the instance label of every pod in the namespace from each test-wide catch, because `podLogs` cannot report that it matched nothing - keeps the unsuffixed value as a second collector in qdrant only, because the parent chart's PVC-cleanup Job labels its pod template with the parent release name and that suite deletes the parent inside a `try:` Harbor renders the same hook and deliberately does not get the same collector: it has no `delete` op, so its parent release is uninstalled only in Chainsaw's cleanup phase, which no catch can observe: catch is a defer inside the step, while the cleaner runs from `t.Cleanup`, after the test function returns. A comment records that asymmetry so the next reader does not restore the collector for symmetry. Each comment states what the pin does and does not cover: it does not catch a wrong value typed into a selector, and it catches nothing when a run fails before reaching the assert. The qdrant comment is equally explicit about how narrow its collector's window is, because the hook always exits 0, so Helm calls it succeeded and `hook-succeeded` takes the pod away; what remains is an uninstall that hangs with Helm still waiting. Telling a lookup's outcomes apart is one helper under `hack/e2e-chainsaw/_lib/` rather than a copy per call site, with the outcome logic split from the kubectl call so it can be pinned without a cluster — the shape `_lib/etcd-probe.sh` already uses. Both the census and the qdrant cleanup listing report through it, differing only in wording. Each lookup keeps stderr apart from the value it captures: kubectl exits 0 while logging that it could not reach an API group, and an aggregated APIService in that state is what a failed run usually looks like, so folding the streams would make the "nothing found" branch unreachable and print that log line as though it were a result. Note what the extraction costs in CI: touching the shared `_lib/` and adding a top-level `hack/*.bats` escalates test-impact analysis from the three suites this change edits to the full Chainsaw suite, so this PR runs everything and is exposed to the whole spread of known flakes. A red suite unrelated to harbor, openbao or qdrant is that exposure, not a defect in this diff. ## Testing - `hack/pod-label-census_test.bats`, 9 cases over the helper's outcomes, run via `hack/cozytest.sh` and picked up by `make unit-tests` through the existing `BATS_UNIT_FILES` glob - mutation-checked rather than assumed: folding stderr back into the captured value, propagating kubectl's exit code out of the helper, and collapsing the failed/empty distinction each turn the suite red, and each is green again on revert - `chainsaw lint test --file` on each of the three suites, and `chainsaw test --no-cluster --config hack/e2e-chainsaw/.chainsaw.yaml --include-test-regex '^$'` over all three, both with v0.2.15, the version the sandbox image pins - `shellcheck --shell=sh` on the helper and on the catch scripts extracted from the YAML - the qdrant step catch driven against a stub kubectl that fails: it reports the failed lookup rather than dying on it - every selector, pin and comment claim checked against the chart that stamps it ### Screenshots Not applicable. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(e2e): collect harbor, openbao and qdrant pod logs on a failed run ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added pod-label census diagnostics to improve visibility into workload labels during test failures. * Enhanced failure diagnostics for Harbor, OpenBao, and Qdrant by collecting relevant workload and database logs. * Added bounded reporting for missing pods, lookup failures, and incomplete cleanup resources. * **Bug Fixes** * Updated diagnostic selectors and readiness checks to match chart-generated instance labels. * Improved PVC cleanup diagnostics with clearer reporting of remaining resources. * **Tests** * Added coverage for successful, empty, failed, and partial diagnostic results. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-06 15:25:09 +03:00
Aleksei Sviridkin	a4a8e4143f	fix(kubernetes): retry the tenant CNI install instead of uninstalling it (#3552) ... ## What this PR does The tenant Cilium HelmRelease carried no install strategy, so Flux applied its default, `RemediateOnFailure`, which uninstalls the release between attempts. Cilium is the tenant cluster's only CNI, so every cycle takes it away and gives it back when the next install applies the manifests again, and `retries: -1` lets that repeat without end. What sets a cycle off is an install that outruns its budget, which can happen while the tenant still has no registered node, since the operator Deployment cannot become Available until one exists. Remediation neither causes that delay nor is fixed by this change: what it adds is a repeated teardown of the CNI on top of a delay that resolves itself once the node arrives. This removes the amplifier, not the cause. `RetryOnFailure` keeps the applied manifests in place and retries a failed install as an upgrade, so the agent DaemonSet rolls out as soon as a worker registers. Both actions carry the strategy because helm-controller selects the active retry from `.status.lastAttemptedReleaseAction`, and the retry of a failed install runs as an upgrade. With the strategy on `install` alone that retry still happens, but once it is the last attempted action its own failure is handled by upgrade remediation, whose default is a rollback with no previous release to roll back to. The `remediation` blocks stay because two branches consult the retry count rather than the strategy and refuse to act once it is exhausted: an absent release reads the install block, an out-of-sync one reads upgrade. `retries: -1` keeps both open. The cost is that a genuine upgrade failure no longer rolls back either, it retries on the failed manifests instead. That is forced, since the controller cannot tell a retried install from a real upgrade, and what it replaces was an unbounded rollback-and-retry flap rather than a stable safety net: `upgrade.remediation.retries: -1` with the default rollback strategy already retried forever. This is the strategy and retry interval that `pkg/registry/apps/application/rest.go` and `internal/operator/package_reconciler.go` already set on every HelmRelease they generate, except that they leave `remediation` nil where this release keeps it. The other tenant addons this chart renders still carry the remediation-only shape; this PR changes the CNI alone, where the teardown removes the cluster's only CNI rather than churning a single component. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(kubernetes): the tenant Cilium HelmRelease now retries a failed install instead of being uninstalled between attempts, so a failed install no longer takes the tenant cluster's only CNI away while it retries. The same strategy applies to the upgrade action, which it has to: a retried install runs as an upgrade. One consequence is operator-visible, a failed Cilium upgrade in a tenant cluster now retries the new manifests every 30s instead of rolling back to the previous release. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Cilium installation and upgrade reliability by automatically retrying failed operations every 30 seconds. * Preserved deployed resources during retries and continued supporting unlimited remediation attempts. * **Tests** * Added coverage validating installation and upgrade retry behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-06 15:24:57 +03:00
Aleksei Sviridkin	b758c3bde2	feat(multus)!: stage reference CNI plugins from the multus image (#3195) ... ## What this PR does Multus-backed NetworkAttachmentDefinitions delegate to the upstream containernetworking reference plugins (`bridge`, `macvlan`, `ipvlan`, ...), and nothing guarantees a node has them where Multus looks. On k3s they live under `/var/lib/rancher/k3s/data/cni` while Multus and Cilium install into `/opt/cni/bin`, which is left without a `bridge` binary. A `bridge`-type NAD then fails with `failed to find plugin "bridge" in path [/opt/cni/bin]`, and today you work around it by copying the binary in by hand. #3196 documents one such pattern (attaching a VM to an external VLAN) that this unblocks. This bundles 14 of the reference plugins into the `multus-cni` image and adds an `install-cni-plugins` init container that installs them into the host `/opt/cni/bin`, mirroring the existing `install-multus-binary` init container. ## Where it applies, and how to turn it off The new `networking.stageCniPlugins` value defaults to on for both bundles. Talos puts six binaries in that directory -- `bridge`, `firewall`, `host-local`, `loopback` and `portmap` stripped from the same upstream release, plus `flannel`, which is a different project -- so the other ten are missing there exactly as on generic Linux and a NAD naming `macvlan`, `ipvlan` or `vlan` fails on either platform. Four of those six are names this change stages; `loopback` is deliberately not staged and `flannel` was never in the reference set. The four names that overlap are rewritten on every Multus pod recreation with the same upstream version -- Talos pins `containernetworking/plugins` `v1.9.1` in `siderolabs/pkgs`, and so does this image -- so it is not a downgrade of a system component. 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. `isp-hosted` ships no multus at all and is unaffected. Both defaults can be overridden either way. If you run a generic-Linux cluster and manage `/opt/cni/bin` yourself, this changes behaviour on upgrade. Multus starts installing 14 plugins into that directory every time its pod is recreated, replacing files of the same name, so a hand-placed or locally patched plugin there will not survive. Set `networking.stageCniPlugins: false` to decline, and set it **before** the upgrade that first enables staging: nothing here removes or restores a plugin, so declining afterwards protects what is still yours and does not give back what was already replaced. Changing the value edits the Multus DaemonSet's pod template, so Multus rolls node by node, and while a node's daemon is down pod creation and deletion on that node fail for the length of the restart. The four plugins the package never touches are `loopback`, `dhcp`, `dummy` and `tap`. Where staging runs it is unconditional, not only-when-absent, because nothing here ever revisits a plugin it has written. Installing only when absent would pin each node to whichever version arrived first, and a later `CNI_PLUGINS_VERSION` bump, including one fixing a binary that runs as root on every node, would reach zero existing nodes. The directory is not ours alone and the decision does not assume it is: kube-ovn copies `loopback`, `portmap` and `macvlan` there from its own image, two of which this change also writes, node provisioning may place the set, and `/opt/cni/bin` is also the CNI bin directory on RKE2. Overwriting is what delivering a version requires, and `networking.stageCniPlugins: false` is how an operator who owns those files declines it. ## Notes - The manifest change lives in `patches/customize-deployment.patch` so it survives `make update`, and the release rewrite in that target runs after the patch rather than before it, so the line the patch adds lands on the same reference as the two it already had. The patch seeds that line with the upstream image reference while the committed template carries the pinned cozystack digest, because the package's `image:` target rewrites every `multus-cni` image line to one digest. So it does not reverse-apply against the committed template. That `make update` reproduces the tree is verified forward from pristine upstream instead. - `/opt/cni/bin` stays shared where staging is on. kube-ovn installs `portmap` and `macvlan` there and has been seen to do so with a plain copy onto the live path, which truncates in place. Each plugin is installed under a temporary name and swapped in with a rename, atomic within the mount, so the kubelet never execs a half-written binary. The Dockerfile records which kube-ovn release the version pin was checked against, and `hack/cni-plugins-staging-contract.bats` fails when that marker and the vendored release part ways. That check fires on the next kube-ovn bump and compares the marker with the vendored release; it does not compare the two `CNI_PLUGINS_VERSION` pins. Moving the marker is the acknowledgement that someone looked -- so read kube-ovn's `dist/images/Dockerfile.base` before moving it, because a silent divergence there makes `portmap` and `macvlan` flap between the two DaemonSets on every pod recreation. - The init container runs unprivileged, with capabilities dropped and no mount propagation. Writing into a hostPath needs root inside the container and nothing on the host, which is how cilium's `install-cni-binaries` copies plugin binaries into the same directory on the same nodes. `Bidirectional` propagation governs mounts made inside a container, this script makes none, and asking for it is what would force `privileged`. The tests pin the absence. - `loopback` is excluded because replacing it is what a node cannot survive: the runtime calls it for every sandbox, and Cilium and kube-ovn already install it. `dhcp` needs a host daemon this package does not run. `tap` and `dummy` have no consumer here. `portmap` is included because a NAD conflist may chain it for hostPort. - The plugin version is pinned via `ARG CNI_PLUGINS_VERSION` (v1.9.1) and the release tarball is verified against a per-arch SHA-256 before extraction. These are root-executed node binaries, so a tag alone is not enough. An unknown or empty `TARGETARCH` fails the build. - The init container skips staging when the image has no `/cni-plugins`, rather than crashlooping the DaemonSet node-wide. The manifest and the image digest are re-pinned on different schedules, so a tree taken between a Dockerfile change and the next release bake can carry one without the other. A plugin it cannot copy is reported on stderr and skipped, for the same reason. `multusConfigFile: auto` makes `00-multus.conf` the primary CNI config and there is no `cleanupConfigOnExit`, so on a node where Multus has already run, an init container that never completes leaves the shim's socket gone with that config still primary, and the node stops scheduling anything that goes through CNI; host-network pods still start. Failing on a copy error would therefore leave the node worse off than before this change, which only ever left it without one plugin. The cost of continuing is that the pod still reports Ready, so the summary is also written to `/dev/termination-log`, which the kubelet surfaces in `kubectl describe pod` regardless of the exit code. - The bundled plugins add roughly 67 MiB uncompressed per architecture to an image pulled by every node, including installs where staging is off. Measured by extracting the staged set from the v1.9.1 release tarball. ## Testing `packages/core/platform/tests/bundles_multus_staging_test.yaml` pins the delivery point: both variant defaults, both explicit overrides, and that multus is still shipped in each bundle at all. That last one because multus is now included by hand in two branches rather than by the shared helper. `hack/multus-install-cni-plugins.bats` extracts the init container's script from the rendered DaemonSet and runs it. It pins that every plugin lands executable, that a replacement swaps the inode rather than truncating in place, that temp files never leak, that a missing or empty directory skips instead of failing, and that a plugin which cannot be installed is reported on stderr and skipped rather than failing the container. Both halves of that last one are asserted, because exit 0 alone would still pass with the report deleted. `hack/cni-plugins-staging-contract.bats` covers what the rendered manifest cannot show: that the Dockerfile stages the intended plugins and only those, copies them into the final stage, and that nothing there removes them again, that the tarball is verified before extraction, and that the version marker matches the vendored kube-ovn release. helm-unittest pins both states of the value, including off, which is what most installs run, plus the quoted-string forms a values file can produce. An explicit `null` is covered too: it is how a values file clears the key, and the chart folds it onto its own default rather than refusing it as an unreadable spelling. The feature is inert until the image is re-baked. All three `image:` lines still point at the pre-change digest, which has no `/cni-plugins`, so on `main` after merge a generic install renders the init container, logs `no /cni-plugins in this image; skipping staging`, and installs nothing until release-prep re-pins. That is the intended flow (first-party digests are release-managed, not hand-edited), and the guide documents the log line, but anyone testing this on `main` will meet it first and the pod will look healthy. No CI job stages plugins on a node. CI boots Talos, where the value is off, and `isp-full-generic` appears in no e2e workflow. The mechanics are covered by the bats above, which run the real script from the real manifest and also under `dash`, the shell in the runtime image, but nothing demonstrates it end to end on a node. Defaulting it on for the generic bundle is a deliberate maintainer decision taken with that gap known, not a side effect of the diff. Shipping it off would leave the bug in place on the only platform where the feature does anything, and would not add coverage either, since the code stays unexecuted in CI both ways. What makes the risk acceptable: the init container skips rather than crashlooping when the image carries no plugins, each plugin is swapped in with an atomic rename instead of being written onto the live path, the tarball is checksummed before extraction, and declining is a value rather than disabling the package. And a plugin it cannot install is reported and skipped rather than failing the pod, so the worst case leaves the node exactly as it was before this change rather than unable to schedule. ### Screenshots N/A, no UI changes. ### Release note ```release-note feat(multus): install the reference CNI plugins (bridge, macvlan, ipvlan, ...) into /opt/cni/bin on generic-Linux installs, where nodes do not otherwise provide them. On by default on every platform: Talos ships five of the fourteen, and the four that overlap are rewritten with the same upstream version. On generic clusters this replaces same-named files in /opt/cni/bin every time the Multus pod is recreated, so a locally managed plugin there will not survive. Set networking.stageCniPlugins: false to decline. ``` 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. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Multus can now stage reference CNI plugins into `/opt/cni/bin`, adding missing plugins and safely replacing matching versions. - Added the `stageCniPlugins` setting, with platform-specific defaults and explicit opt-out support. - Invalid configuration values now produce clear rendering errors. - Improved plugin installation diagnostics and failure reporting. - **Documentation** - Expanded guidance for CNI prerequisites, plugin staging behavior, troubleshooting, rollout impact, and configuration. - Clarified Multus image reference handling in platform documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-06 15:24:37 +03:00
Aleksei Sviridkin	0c4c11c845	fix(kubernetes): retry the tenant CSI install instead of uninstalling it ... Flux defaults a HelmRelease to the RemediateOnFailure strategy, whose install remediation is always an uninstall. A tenant CSI install that misses its timeout was therefore torn down between attempts, taking the CSI node plugin and the StorageClasses this release propagates with it, and the next attempt restarted the DaemonSet rollout from nothing instead of resuming one already under way. That teardown races the rollout it is meant to recover. In one observed run both DaemonSet pods were scheduled immediately and spent the whole budget pulling images; the slower node started its last container six seconds after the install timeout fired, and the uninstall removed it one second later. Retrying in place keeps that work. RetryOnFailure leaves the manifests applied and retries the failed release. Both actions carry it because the controller picks the active retry by .status.lastAttemptedReleaseAction and runs that retry as an upgrade: set on install alone, the retry happens but its own failure falls through to upgrade remediation, which rolls back by default with no previous release to roll back to. The cost is that a real upgrade failure now retries rather than rolls back, and that gives up no stable safety net: with retries: -1 and the default rollback strategy, upgrade remediation already rolled back and retried without end. The remediation blocks stay for a separate reason. An absent release consults install.remediation and an out-of-sync one consults upgrade, both by retry count rather than by strategy, so retries: -1 is what keeps those two branches acting. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-06 12:43:40 +03:00
Aleksei Sviridkin	5542ae52b7	fix(e2e): budget tenant scheduling separately from workload readiness ... The tenant backend Deployment had a single 300s budget to reach condition=Available, and two unrelated variable costs shared it. The first is scheduling. What the suite establishes before that point is two tenant nodes Ready, which is weaker than schedulable: a Ready node still carries node.cilium.io/agent-not-ready until the tenant cilium agent claims it, and a node the bringup has not finished with is SchedulingDisabled. Scheduling took 2m18s and 1m57s in the two tenant suites of one run, leaving the image pull to finish inside what was left. It did not, and both suites failed with the same message for two different shortfalls. Wait for a node that actually accepts a toleration-free Pod before creating the workload, on its own budget and its own failure message. The gate encodes the scheduler's rule for such a Pod (Ready, not unschedulable, no NoSchedule or NoExecute taint) over a custom-columns probe, prints the node table on both outcomes so a timeout names the taint that held it, and treats a failed probe as not-schedulable so an API blip cannot release it. The readiness wait keeps its 300s, now starting from a schedulable node, and dumps deployment, pod and event state when it runs out. Pin the workload image by digest. The tenant workers reach no registry mirror, so nginx is pulled from Docker Hub on every run either way; the digest fixes what that pull returns instead of leaving a floating tag free to change size and content under a fixed deadline. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-06 12:20:50 +03:00
Aleksei Sviridkin	4d16f4fdb2	ci: re-run e2e in a confirmed green window ... Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-05 19:29:53 +03:00
Aleksei Sviridkin	115d9bbaaf	fix(installer): let the operator Deployment report Available honestly ... At replicas: 1 with maxUnavailable: 1 the Deployment controller takes the minimum available replica count as 1 - 1 = 0, so the Available condition reports True with no running pod at all. That reading is what every consumer of the status sees: an alert on Available, a health dashboard, or a person reading kubectl get deploy all show green while the operator is dead. The replacement is not a tuned pair, it is the platform default written down. At replicas: 1 the default 25% / 25% resolves to maxSurge 1 and maxUnavailable 0, because the surge rounds up and the unavailability rounds down. So this change stops overriding the default rather than introducing a configuration of its own, and Kubernetes rejects maxUnavailable: 0 together with maxSurge: 0, so the surge could not have gone the other way regardless. The overridden pair was carried over from the earlier cozystack Deployment, where a darkhttpd sidecar published a host port and hostNetwork left a surge pod unschedulable on a single-node cluster. It arrived here in two steps: by the commit before this file was created the sidecar was already gone and no port was declared, yet the pair stayed, so it had outlived its own reason before it was ever copied. That is why looking for a justification in this file's history finds none. With no port declared anywhere in the pod there is no hostNetwork port conflict left, so the surge pod schedules beside the outgoing one; declaring one reintroduces exactly the scheduling failure the pair was added for. Not exercised on a live cluster in this rework. Stay on RollingUpdate rather than move to Recreate, which would fix the condition too, and by a shorter route: MaxUnavailable() reports 0 for any strategy that is not RollingUpdate, so the minimum available replica count becomes replicas - 0 = 1 and the condition is honest without any surge pod at all. The transition was not attempted, and the failure this project fixed in ouroboros 0.8.1 does not transfer to it. There the rollingUpdate block was the one the apiserver defaults onto a Deployment created without one, so it belonged to no applier and outlived the switch to Recreate. This Deployment has carried an explicit block since the commit that created it, and nothing applies it server-side, so whether Recreate would upgrade cleanly is an open question about the field-ownership history of the live object rather than a known rejection. What keeps RollingUpdate here is availability rather than apply semantics: maxUnavailable: 0 leaves the outgoing operator running when the replacement never starts, where Recreate deletes it first and leaves none. Editing the numbers inside the block is upgrade-safe from every state the field can already be in. The surge introduces new windows, and leader election covers less of them than it looks. The operator runs four privileged install steps before mgr.Start(), and the lease is only acquired inside mgr.Start(): the CRD install, the Flux install, the platform source resource and the platform PackageSource. All four therefore run in the incoming pod outside leader election entirely. What the lease does cover is the reconcile loops, the part that would otherwise conflict continuously instead of once at startup. That leaves three seams, and only the first two happen on a healthy upgrade. The routine one is the incoming pod's install steps against the outgoing pod's live reconcile loops, since the outgoing pod ran its own installs at its startup and has been inside mgr.Start() ever since. Those do not contend for the same fields: the installs write as field manager cozystack-operator, while the reconcilers write as cozystack-packagesource-controller and cozystack-package-controller and go mostly to status subresources. Different owners, different objects, so this seam is safe by separation rather than by any shared manager. The second is version skew rather than field contention, and maxSurge: 1 widens it rather than opening it. Under maxSurge: 0 the surge budget counts desired replicas rather than live pods, so the incoming pod is created while the outgoing one may still be Terminating - the overlap existed. What it did not have was a live counterparty: scale-down sends SIGTERM before the new pod is created, and that cancels the manager context, so the old loops were already unwinding. Here the outgoing pod is not signalled until the incoming one is available, so the overlap runs against a fully live reconciler for a termination grace period instead of a vestigial one. The incoming pod reports Ready the instant its container starts, because there is no readiness probe, so its CRD install runs while the outgoing binary's reconcile loops are still live and now writing against a schema the incoming pod has already moved. Old code against a new schema, which no field manager separates. It is bounded rather than benign: the outgoing pod exits on SIGTERM within seconds, a rejected write is transient and the next reconcile repairs it, and no object is left corrupt. Widening the skew, by adding a probe that delays Ready or by a migration the old code cannot round-trip, is what would make this seam matter. The third seam is install against install, and it needs the outgoing pod to be restarting, since a crashlooping pod re-runs its four steps on every restart, which is exactly the broken operator this gate exists to catch. Three of the four steps use server-side apply under the same field manager, so they never conflict and resolve last-writer-wins whenever the two pods carry different manifests. That ordering matters in one direction, and it is accepted here rather than guarded against: the CRD install and the Flux install both apply with Force: true, so an outgoing pod that writes last silently overwrites what the incoming pod has just applied, with no conflict and no error to exit on. The incoming pod is inside mgr.Start() by then and will not apply again until it restarts, so that revert of the upgrade's install phase persists rather than healing itself. Reaching it needs a restart landing in the seconds between the incoming pod's installs and the outgoing pod's SIGTERM. installPlatformSourceResource is the remaining exception, a read-modify-write of a Get followed by Create or by Update with the observed resourceVersion, so two pods running it at once can collide on AlreadyExists or on a conflict; that failure is loud and self-healing, since any error there exits the process and the pod restarts, and the object is not left corrupt. Pin the strategy and all four preconditions the surge makes load-bearing in a unit test: no container or initContainer declares a port, both listener arguments stay disabled, leader election stays on, and no probe uses httpGet. They fail differently and none is visible to the others, since a declared port stops the scheduler, a bound one only fails once the incoming process starts, and a second manager without a lease fails nothing at all until it writes. The httpGet precondition follows from the two disabled listeners leaving no port to target, and it is the worst of the four to get wrong: the other three wedge an upgrade, while a livenessProbe against a disabled listener crashloops the pod on every cluster, including a fresh install where no surge happens. Only the httpGet field is forbidden rather than the probes themselves, because an exec probe stays legitimate. All four are checked against all three variants, because the template branches on variant inside the container spec and a port added under one branch renders for that variant alone. The strategy type is pinned alongside its two values, since maxSurge and maxUnavailable say nothing on their own once the type is no longer RollingUpdate. The listener preconditions are pinned less tightly than the other two, and the test says so rather than implying coverage it does not have. A membership assert has no notion of position while Go's flag package keeps the last occurrence, so a duplicate after the disabled one wins and the assert still passes, while a duplicate before it loses and the assert passes just the same - the test cannot tell those two apart. Moving the disabled flag last fixes the process without making the assert catch anything. Rejecting the two addresses this binary defaults to closes the shape a values-gated block to enable metrics would take, and nothing available here closes an arbitrary address, since helm-unittest cannot express that a flag appears at most once. Note in hack/helm-unit-tests.sh that the loop covers packages/core, which its comment omitted. The suite added here lives there, so a later edit reconciling the loop with the stale comment would have silently stopped running it. Split the rationale by audience rather than keeping it all in shipped comments. What an operator applying the rendered manifest has to respect stays in plain comments and travels into the install artifacts: that the pair is the default, why maxUnavailable must stay 0, that no port may be declared or bound, that no probe may use httpGet, and that leader election has to stay on. Most of what stays in the template comment is what the chart's own suite pins, so it cannot drift out of step with the chart without a test going red: the four preconditions and which assert covers each, and why the variant loop is needed. Three things there are deliberate exceptions, kept because anyone editing the strategy has to know them and no assert can carry them - that the webhook server configured on 9443 is not a port binder, since controller-runtime adds it to the runnables only inside GetWebhookServer() and this binary never calls it; that two overlaps between the pods sit outside the lease; and why RollingUpdate is kept rather than Recreate. Those name what exists. The seam analysis above, which is how each one works, deliberately stays here instead. It describes another file's internals, nothing pins it, and a commit message is fixed in time, so it cannot quietly fall out of date the way a comment can. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-05 18:15:41 +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	3fc8e524b2	fix(e2e): select catch pod logs by the nested release instance label ... The harbor, openbao and qdrant app charts deploy their workload through a nested HelmRelease named `<release>-system`, and pass `fullnameOverride: <release>` down to it. The inner chart therefore stamps app.kubernetes.io/instance from its own release name, with the suffix, while object names come from the override, without it. The catch blocks selected on the outer release name, so podLogs matched no pods and every failed run collected an empty result. A pod name says nothing about its instance label; the two come from different sources on purpose. In qdrant the unsuffixed value is therefore not retired but kept as a collector of its own: the parent chart's PVC-cleanup Job labels its pod template with the parent release name, and that suite deletes the parent inside a `try:`, so the Job is in scope while the test still runs. The comment there is explicit about how narrow that window is -- the hook always exits 0, so Helm calls it succeeded and `hook-succeeded` takes the pod away; what is left is an uninstall that hangs with Helm still waiting on it. Harbor renders the same hook and does not get the same collector. It has no `delete` op, so its parent release is uninstalled only in Chainsaw's cleanup phase, which no catch can observe: catch is a defer inside the step, while the cleaner runs from t.Cleanup, after the test function returns. That collector would be the same permanently-empty capture this commit removes. A comment records the asymmetry so the next reader does not restore it for symmetry. openbao renders no such hook at all. Each workload value is now also pinned on a resource in an assert the suite already runs, so the chart renaming a label fails a step instead of quietly emptying a capture. The comments state what that does and does not cover: it does not catch a wrong value typed into a selector, and it catches nothing when the run fails before the assert. Harbor's database was reached by no collector at all. It is selected on `cnpg.io/cluster`, CNPG's own primary label, rather than on the instance label CNPG merely derives from the Cluster name -- the primary label has nothing to drift from, and it is what the postgres suite and the WorkloadMonitors already use. Each lookup keeps stderr apart from the value it captures. kubectl exits 0 while logging a klog line about an API group it could not reach, and an aggregated APIService in that state is what a failed run usually looks like; folded into the value, that line makes the "nothing found" branch unreachable and prints as though it were a result. Telling those outcomes apart is one helper in hack/e2e-chainsaw/_lib/ rather than a copy per call site, separated from the kubectl call so it can be pinned without a cluster, the way etcd-probe.sh already is. Both the census and the qdrant cleanup listing report through it, differing only in wording, so the discrimination is implemented and tested once. The tests cover each outcome for both wordings, including the zero exit with a klog line on stderr -- the case a folded stream turned into a row. A collector that matches nothing cannot say so: podLogs is silent, and `kubectl get -l` exits zero on an empty match. Each test-wide catch prints the instance label of every pod in the namespace, and the qdrant cleanup catch reports its three outcomes apart from a single call -- printing what that call returned rather than running it again, which could put a healthy listing under a failure heading. That census runs on a degraded cluster, where kubectl otherwise retries discovery against an unreachable apiserver until something kills the script, so every lookup carries `--request-timeout`. Measured against an unreachable apiserver, a bounded call costs about five times the flag, since kubectl retries discovery five times before it gives up. Each script then carries an explicit op timeout sized above its own calls -- 2m for the single-call censuses, 5m for the qdrant cleanup listing, which makes three. An inner budget is what should fire: a SIGKILL at the op boundary yields nothing, where a kubectl that gives up leaves an error to print. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-05 16:46:50 +03:00
Aleksei Sviridkin	ff867b6a4f	ci: re-run e2e after install-phase ClusterIP unreachability in the sandbox ... Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-05 16:29:54 +03:00
Aleksei Sviridkin	e9e56097bc	ci: re-run e2e after infra-level apiserver unreachability in the sandbox ... Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-05 16:11:08 +03:00
Aleksei Sviridkin	d3ae21b7f0	fix(e2e): name every way the previous-instance capture falls short ... The capture wrote its reasons to the job log only. A reader holding the uploaded artifact saw N logs in a directory with no way to tell N from all of them: the container cap that dropped the rest, a pod list that never returned, a previous instance that produced nothing, and a read the outer backstop cut short all looked alike from outside. Every one of those lines now goes to capture-notes.txt as well, and the directory is created before the first of them, so a capture that found nothing is distinguishable from one that never ran. A pod list that failed is no longer reported as nothing having restarted. That is a claim about the cluster which the script never observed, and it is the claim most likely to talk a triager out of the crash-loop hypothesis on exactly the run where it is right. Exit 137 no longer asserts whose signal ended the read. 124 comes from timeout and from nothing else, but 137 is 128+SIGKILL, which the kill grace produces and so does the OOM killer or a teardown signalling the process group. Both belong in the cut-off branch, since the capture ends early either way, but reported flatly as the script's own timeout a read killed at second two reads as one that waited the full eighteen. COZY_PREVLOG_TAIL rejects a value kubectl cannot use and says so. A zero tail asks for no lines while kubectl still exits 0, which the empty-output branch would otherwise report as the container having logged nothing, discarding the only remaining copy of a log the kubelet has already collected. A leading zero is rejected because `$(( ))` reads it as octal while `[` reads it as decimal, so one string means two numbers. Each kept log names the bound it was written under, since --tail drops the oldest lines and nothing else in the output says so, and -1 suppresses that note because no bound was applied. An output directory that cannot be written is reported as the local condition it is. It arrives as an argument, so it can be a file, or missing, or unwritable; the redirection then fails before kubectl runs and hands the shell's own status to the branch that names kubectl, which reported "no previous instance retrieved (kubectl exit 2)" once per container. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-05 16:03:57 +03:00
Aleksei Sviridkin	ceccac27b0	test(hack): clean up bats temp dirs without EXIT traps ... A `trap ... EXIT` inside an `@test` replaces the one the bats binary installs for its own bookkeeping, so a test that then fails prints no TAP line at all. Not `not ok`, nothing: the run ends with a warning that N of M tests executed, and anyone judging it by grepping for `not ok` reads a green suite. Cleanup moves to the last line of each body instead. Both runners set -e, so a failed test leaves its scratch directory behind, which is what you want to look at anyway. hack/promote-retag_test.bats was failing this way already, reporting five passes and no failures while six of its eleven tests printed nothing. Those six invoke the script for real, and it requires sha256sum; they pin PATH to the stub directory plus /usr/bin and /bin, which is where coreutils installs that binary and where macOS does not, so the failure was of the platform rather than of the code and CI never saw it. The stub directory already supplied yq and skopeo for the same reason and now supplies sha256sum too, resolved from absolute candidates because the stub itself is first on PATH. Two file headers described the trap arrangement they no longer use and are corrected rather than left to outlive it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-05 16:03:57 +03:00
Andrei Kvapil	df157da618	ci(workflows): open the E2E breakpoint for maintainer-authored PRs (#3158) ... ## What this PR does Decouples opening the E2E SSH breakpoint from the `debug` label. Today, the only way to open a breakpoint on a failed feature PR is to add the `debug` label. That same label also flips every job's `runs-on` onto self-hosted runners, so you cannot get a breakpoint on the normal ephemeral runner — debugging always drags the whole pipeline onto self-hosted infrastructure. This adds a maintainer-authorship path to the breakpoint's `if` condition: a failed PR whose author is an `OWNER`, `MEMBER`, or `COLLABORATOR` now opens the breakpoint automatically, with no label required and without moving the jobs onto self-hosted runners. The breakpoint simply holds the default `oracle-vm` ephemeral runner the job already runs on. The existing `debug` and `release` label paths are unchanged, and the `authorized-users` list still controls who may actually SSH into an open breakpoint (second defense layer). Workflow triggers are untouched — this is purely an `if`-condition change. ### Release note ```release-note ci(workflows): open the E2E SSH breakpoint automatically on failed maintainer-authored PRs, without requiring the `debug` label or moving the jobs onto self-hosted runners ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated E2E failure debugging so breakpoints can now open for pull requests authored by repository maintainers, not just when specific labels are applied. * Clarified the conditions and runner behavior for breakpoint access in the workflow documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-05 14:29:45 +02:00