5430 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
Aleksei Sviridkin	02a3fda471	test(e2e): capture host-netns address attribution and a healthy-pod reference for the dataplane collector (#3617) ... <!-- 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 Extends `hack/e2e-capture-dataplane.sh`, the diagnostic collector that runs after an e2e test failure, with the instrumentation issue #3615 asks for to close its address-attribution gap. Issue #3615 found that a NotReady pod's IP can turn out to be a local address of the node's host netns instead of reachable through the ovn0 route, which makes kubelet's probes connect to the node's own loopback and fail forever. The collector already captured `ip addr show ovn0` for the affected pod's node, but nothing that could name which interface actually holds a stolen address, since it only ever looked at ovn0. This adds `ip -o addr show` (every interface) and `ip route show table local` to the same host netns capture, so the interface actually holding a local address can be identified on the next occurrence. The collector also only ever inspected NotReady pods, so a run had no same-window baseline for what a normal route and conntrack state look like on the same node. This extracts the existing per-pod capture into a reusable function and runs it a second time against one Ready, Running pod on the same node as an in-run healthy-pod control, picked while explicitly excluding hostNetwork pods (their podIP is the node's own address, so one would show the exact same local-route fingerprint as the bug for an unrelated reason). When no eligible Ready pod exists on the node, that absence is recorded in the artifact instead of left silent. This is diagnostic tooling only: it changes what gets captured after a failure, not any test's pass/fail outcome. ### Known limitations The baseline runs at a reduced scope: the host route and the kernel conntrack for the healthy pod's address, and nothing else. What it drops -- cilium CT, the OVN port binding, the OVS `ovn-installed` flag, and the pod's own Ready conditions and events -- describes how a pod was programmed, and a pod picked for being Ready and Running is programmed correctly by definition. That scope is what holds the pod path's worst case at 579s against the 600s the `cozytest.sh` trap allows; the remaining 21 seconds are what this path leaves for the LoadBalancer section that runs after it, not headroom the run has. The other caller's envelope is 300s, and the worst case does not fit it -- nor did it at the merge base, where the same walk bounds at 519s. That overrun is tracked in #3671, and the term-by-term arithmetic is in the `MAX_PODS` note in the script. The 579s above assumes the lookup memo exists. When `mktemp` fails the memo is empty, every per-node pod lookup runs live again, and the same single-pod case bounds at 667s -- past the 600s envelope as well as the 300s one. At the merge base that memo-less case bounds at 585s, inside 600s, so this change is what moves it out. The degradation is visible rather than silent: the caller's backstop truncates the capture and the run reports `data-plane capture INCOMPLETE (exit 124)`. A baseline can be present and still carry nothing. Every capture at that scope goes through the node's `kube-ovn-cni` pod, so when that lookup does not resolve, the reference section is its banner and the scope note with nothing beneath them -- and, unlike `node-<node>.txt` next to it, no line saying why. Two different standards of honesty inside one artifact. The affected-pod path has the same shape today, so this extends an existing gap rather than opening one, and it is left alone here. The per-node baseline is cached under `TMPDIR` for the run. A SIGKILL -- which is what a caller's wall-clock backstop delivers -- skips the cleanup at the end of the script and leaves that directory behind. The lookup memo this script already used has the same exposure. Nodes are visited in the order their affected pods appear in the cluster-wide list, which the API chooses. Once `MAX_PODS` cuts the walk, which pods and therefore which nodes got captured is a function of that ordering and not of anything a reader would pick. ### Screenshots Not applicable, no UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note test(e2e): capture host netns address attribution and an in-run healthy-pod reference for the dataplane failure collector ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added diagnostics for node addresses and local routing information. * Added healthy, same-node reference captures for dataplane troubleshooting, reused across affected pods. * Expanded pod diagnostics to include status, events, connection tracking, routes, network bindings, and interface state. * **Tests** * Added coverage for pod selection, retries, caching, exclusions, incomplete records, unavailable responses, and missing baselines. * Recorded diagnostic failure reasons in artifacts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-09 15:39:36 +03:00
Aleksei Sviridkin	18be4ee272	test(e2e): capture host-netns address attribution and a healthy-pod reference ... The collector characterises the host->local-pod route for an affected pod through ovn0 alone, so when a pod IP turns out to be a local address of the host netns instead, nothing records which interface actually holds it. The host netns capture now also takes `ip -o addr show` across every interface and `ip route show table local`, so the owner can be named on the next occurrence rather than only ruled out on ovn0. The collector also inspects NotReady pods only, so a run has no same-window baseline for what a normal route and conntrack state look like on the same node. The per-pod capture becomes a reusable function and runs a second time against one Ready, Running, non-hostNetwork pod on that node, into the same file, so the comparison is in front of whoever opens it. hostNetwork is excluded because such a pod's podIP IS the node's own address and would show the same local-route fingerprint as the fault for an entirely ordinary reason. That baseline depends on the node and nothing else -- the candidates are the rows of the one snapshot the affected set was computed from, and a pod reporting Ready there is by definition not one this run is investigating -- so it is captured once per node and folded into each affected pod's file from a cache. Several affected pods on one node is what a wedged host->local-pod datapath looks like rather than an edge case, and capturing the same healthy pod once per affected pod would spend the caller's envelope re-asking a question already answered -- the cost the node-global dedup and the lookup memo beside it exist to avoid. This adds no read of its own. The cluster-wide pod list gains one column instead of being asked a second time, and that single read's status is what lets the artifact separate a node with no eligible pod from a list that never answered: one line for both would make an unchecked baseline read as an established absence. The two host-netns additions are best-effort execs, like the captures they sit beside. The new row filter carries the field count its jsonpath emits, so a list cut mid-record cannot hand over a fragment as the pod every reading of the affected one is compared against. The unit suite pins these on what they are rather than on what they leave behind. Both host-netns captures are asserted through the kubectl argv the collector issues, container included, because the section headers are separate echoes that outlive the exec being removed. The baseline is pinned from both sides: several affected pods on one node share a single capture, and an affected pod is served the healthy pod from its own node rather than from whichever node the walk reached first. The pod list's column order is asserted against the positions its readers index, because every runtime test answers that list from a stub written by hand and none of them can observe the jsonpath at all. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-09 14:42:19 +03:00
Aleksei Sviridkin	dda339f351	fix(e2e): keep a failed image-mirror lookup from pinning the public factory (#3677) ... ## What this PR does The tenant Kubernetes e2e suites decide once whether worker image pulls go to the in-cluster Talos mirror or to the public Image Factory, and that decision came from a plain `kubectl get deploy talos-image-cache`. A plain `get` exits non-zero for a connection refused, an Unauthorized and a real absence alike, so any blip during that one call read as "no mirror", and the answer went straight into the decision file every later tenant test reads. One unlucky moment sent every worker in the run to the public factory over live egress, which is the dependency the mirror exists to remove, and nothing in the log said so. The run went ahead against the flaky thing and the failures read as environment noise. The lookup now asks with `--ignore-not-found -o name`. A real absence answers exit 0 with no output, which leaves a non-zero exit meaning only that the question failed, and the helper re-asks a couple of times before giving up. Only the empty answer turns the suite toward the public factory. An unanswered lookup warns, falls back for its own caller alone, and is deliberately not cached, so the next tenant test asks again instead of inheriting a guess. The warning has to be backed by something, and kubectl's stderr alone is not enough. When the wall-clock bound kills an attempt, neither the bound nor the SIGTERMed kubectl writes a word, and that stall is precisely the failure the re-ask was written against, so a warning that sent the reader to kubectl's message would be pointing at an empty stretch of log exactly when the lookup mattered. The lookup therefore names the last attempt's exit status before giving up, in the vocabulary the diagnostic reads already use for 124 and 137, and kubectl's own text stays on stderr for the cases where there is any. Each attempt is bounded twice, and the two bounds are deliberately not equal: `timeout -k 5 40` around `--request-timeout=30s`. kubectl defaults the request timeout to 0, so a connection that establishes and then stalls has no deadline at all, and the flag on its own is not a wall-clock bound either, because kubectl retries discovery several times before it gives up. Re-asking an unbounded call three times would have multiplied a hang instead of riding out a blip. The ten seconds between the two are load-bearing, and I only found out why by measuring rather than reasoning. Driven against a stub apiserver the three failure modes are not alike: a refused port answers in under a second and names the refusal, one that completes TCP but stalls mid-TLS hits kubectl's own handshake deadline and still produces text despite being killed, and a blackholed address (SYN dropped, no RST, which is what a wedged node or a netpol looks like) produces nothing at all until some deadline expires. That last mode is the one the headroom buys back. With both bounds at the same value the expiry that arrives first is always the wall-clock kill, so the blackhole case ends as a bare exit 124; with the wrapper ten seconds above, the request deadline ends the attempt and kubectl says what happened, and the wrapper stays as the backstop for a client that overruns its own deadline. Controlled A/B, same target and same inner bound, only the outer differs: 30 over 30 gives exit 124 and zero bytes, 40 over 30 gives exit 1 at the thirty second mark and a message naming the cause. The cost is ten more seconds per attempt on a path where the apiserver is already unreachable, inside a 50m op. Worth stating separately, because it is a different question with a different answer: before this change the inner flag was not doing anything at all. `--request-timeout=30s` sat under a `timeout` of the same 30 seconds, and across all three measured modes it never once fired. The refused case never reached it, the TLS stall was ended by kubectl's own shorter handshake deadline, and the blackhole was ended by the wall-clock kill landing at the same instant. So the flag was decoration, and the comment above it claimed otherwise. That is a separate defect from the lost diagnostics, and it is fixed by the same separation rather than by adding anything. Where `timeout` is not installed the read runs unwrapped instead of through it. Its exit 127 would otherwise count as an attempt that did not answer, three times over, so a missing local binary would pin the whole suite to the public factory while every message blamed the apiserver. `talos_image_cache_diagnose` asks the same question on the node-join failure path, and #3676 landed its own three-way gate there while this branch was open. The two stay separate rather than sharing this helper. That one runs under a phase budget where re-asking spends what the tenant snapshot behind it needs, so it answers on a single attempt; this one runs on the happy path, where a re-ask costs a suite nothing and buys back the mirror. This branch leaves that gate as it was merged. Unit tests put a throwaway `kubectl` and `timeout` on PATH and drive all three outcomes, the re-ask, the caching rule, the `timeout`-absent path and the killed-attempt path where nothing but the lookup itself can explain the failure: `hack/cozytest.sh hack/talos-image-cache_test.bats`. The stubs are files rather than shell functions because an external `timeout` binary execs a real file and would never see a shell override. The re-ask is a retry loop, which `docs/agents/e2e-testing.md` restricts. It falls under the exemption there rather than the prohibition: what is retried is a network read with no product or test logic under it, and the deterministic steps the rule protects are untouched. This relates to #3668 and closes the first door only. The decision is cached once at the end of the function regardless of which inner check produced the negative answer, and inside the "mirror is present" branch the `rollout status` wait and the three reads in `_talos_image_cache_reachable_from_tenant` still conflate "the mirror does not work" with "the probe could not be run", with the same suite-wide effect one layer down. The issue's own status comment says it stays open after this change, so there is no closing keyword here. Three weaknesses this PR does not remove, named because each is a real thing a later reader will hit. The `absent` case in `resolve_talos_image_factory_url` is reached by `else` fall-through rather than by matching the token, so a fourth state added to the helper later would land in the branch that turns the suite toward the public factory rather than in the safe one; a test pins the `present` token against exactly that drift, but the shape stays fall-through. The lookup's own `30` and `5` are literals while the diagnostic reads next door run off a validated, overridable pair, and that is deliberate rather than an oversight: those knobs are named and documented for the dump budget, so wiring the lookup to them would let a test that turns the dumps down silently change which image factory a whole suite pulls from. And `_TALOS_IMAGE_CACHE_QUERY_TRIES` is the one knob in the file without that validation. A non-numeric value spins the loop forever rather than failing: the comparison sits in an `if` condition, so `set -e` is exempt, the branch is simply never taken, and the job burns to its ceiling with no output. Nothing outside the tests sets it, which is why it is here and not in the diff. Worth recording for whoever does fix it: the obvious remedy, reusing this file's own `_talos_image_cache_seconds`, does not work where the knob is assigned, because the validator is defined a hundred lines further down and the assignment runs at source time. The call resolves to nothing, the substitution yields an empty string, and an empty bound spins the same loop. A real fix has to move the definition up or validate inside the function at call time. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches `hack/e2e-chainsaw/_lib/talos-image-cache.sh` and `hack/talos-image-cache_test.bats` and nothing else: no file under `hack/` is moved or renamed, no make target changes behaviour, and no package, chart, CRD or values key is involved. The `cozystack/ccp` entry is the closest one and it does not fire. - [x] No downstream repository is affected by this change ### Release note ```release-note fix(e2e): a single failed apiserver lookup no longer sends a whole e2e run to the public Talos Image Factory ```	2026-08-09 14:06:12 +03:00
Aleksei Sviridkin	5400f165e3	fix(e2e): keep a failed mirror lookup from pinning the public factory ... resolve_talos_image_factory_url asked whether the in-cluster Talos image mirror exists with a plain `kubectl get deploy`, where a connection refused, an Unauthorized and a real absence all arrive as the same non-zero exit. It then cached that reading for the whole suite, so one unlucky moment sent every tenant worker to the public Image Factory over live egress -- the dependency the mirror exists to remove -- while the run still looked ordinary. Ask with --ignore-not-found -o name instead: a real absence answers exit 0 with no output, which leaves a non-zero exit meaning only that the question failed, and re-ask a couple of times before giving up. Only the empty answer turns the suite toward the public factory. An unanswered lookup warns, falls back for its own caller alone, and is not written to the decision file, so the next tenant test asks again rather than inheriting a guess. Each attempt is bounded twice, and the two bounds are deliberately not equal. kubectl defaults --request-timeout to 0, so a connection that establishes and then stalls has no deadline of its own; the flag alone is not a wall-clock bound either, because kubectl retries discovery several times before giving up. Measured against a stub apiserver, the three ways this call fails behave differently: a refused port answers in under a second and names the refusal, one that stalls mid-TLS gets kubectl's own handshake deadline and still produces text, and a blackholed address produces nothing until some deadline expires. With both bounds at the same value the expiry that arrives first is always the wall-clock kill, so --request-timeout can never fire and that last case stays silent. The wrapper therefore sits ten seconds above it: the request deadline ends the attempt and kubectl says why, and the wrapper remains the backstop for a client that overruns its own deadline. Where timeout is not installed the read runs unwrapped rather than through it: its exit 127 would otherwise count as an attempt that did not answer, three times over, and a missing local binary would pin the whole suite to the public factory while every message blamed the apiserver. kubectl's stderr is left alone, because it is the only thing separating refused from unauthorized from stalled. It is still not guaranteed, since a kill can land before the client has said anything, so the lookup also names the last attempt's exit status before giving up, in the vocabulary the diagnostic reads use for 124 and 137. The diagnostic path asks the same question and keeps its own answer to it. That one gives up after a single attempt, under a phase budget where re-asking spends what the snapshot behind it needs; this one runs on the happy path, where a re-ask costs a suite nothing and buys it the mirror. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-09 13:31:55 +03:00
Aleksei Sviridkin	04494555c9	test(e2e): pin that an unanswered mirror lookup is not a mirror absence ... The Talos image factory decision asks the apiserver once whether the in-cluster mirror Deployment exists, reads any non-zero exit as an absence, and caches that answer for the whole suite. A refused, throttled or timed-out lookup therefore sends every tenant worker in the run to the public Image Factory over live egress -- the dependency the mirror exists to remove -- and the run reads as ordinary environment noise rather than as a decision that was never actually made. Route that question through one helper and pin what the helper owes its caller: the three outcomes it must tell apart, the flags that make the three-way answer possible at all, a deadline on every attempt, the caching rule -- a real absence is resolved once and cached, an unanswered lookup falls back for its own caller alone -- and that a missing local timeout binary leaves the lookup asking the apiserver rather than answering unknown three times over. Two of those need saying carefully, because both concern what the caller is told when the lookup fails. A killed attempt gets its own case, driven by a wrapper that exits without running kubectl at all, so the lookup has to account for why it gave up rather than relay someone else's words: pinning the reason on a kubectl that failed loudly, as the sibling case does, pins it only where it was never in doubt. And the wall-clock bound is pinned as strictly greater than the request deadline beside it, which is the property that lets kubectl reach its own deadline and say what happened; level with it, the kill always lands first. The helper is a placeholder in this commit and claims present unconditionally, so the cases that need the distinction fail. The stubs are files on PATH rather than shell functions because the lookup runs under an external timeout binary, which execs a real file and would never see a shell override. xtrace stays off for the rest of a test that captures stderr: turned back on, a failing assertion wedges bats instead of reporting it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-09 13:31:54 +03:00
Aleksei Sviridkin	048088bd29	fix(e2e): bound the tenant failure diagnostics per read and as a phase (#3676) ... <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The node-join failure diagnostics in `hack/e2e-chainsaw/_lib/run-kubernetes.sh` issued about a dozen `kubectl` reads with neither an outer wall-clock bound nor `--request-timeout`, starting with a `describe nodes` through the tenant kubeconfig. That read targets the component least likely to answer in a node-join failure. An unbounded read there does not lose only itself: it holds the Chainsaw op until the op is killed, so everything scheduled after it is lost rather than truncated, the tenant crust-gather snapshot above all. Every read now carries the per-call bound the newer collectors on this path already use. A read that fails or is cut off says so instead of passing in silence, and says only what was observed, because a read that did not finish is silence about the cluster rather than a finding about it. The CDI importer walk gains a cap, and its listing is read on its own so a listing that never answered cannot read as a namespace with no importer in it. Per-read bounds alone do not end the phase, and the collectors here can together spend more than what is left of the op by the time they run. So the phase is bounded as a phase: once its budget is spent, the collectors that have not started are declined out loud. That is not a worst case and is not written as one, but it stops the snapshot being queued behind work begun past the deadline, which is how the snapshot was being lost. The op ceiling is unchanged, since a larger ceiling multiplied by a sum that grows with every collector added is the same problem one collector later. Because what runs last is what gets declined, the block is reordered so the cheap reads come first. The CSR list and the signer log are the discriminator for the failure mode a follow-up fix has to target, and behind two minute-scale collectors they were exactly what a tight run would drop. Adjacent work on the same failure path, all of it in service of the same requirement: - the sibling scheduling-gate branch ends in the same exit and depends on the same snapshot, so its two reads are bounded too; - `talos_image_cache_diagnose` gets its reads bounded and its gate gets `--ignore-not-found`, because `kubectl get` exits 1 for a refused connection or Unauthorized as readily as for NotFound, so a call that never reached the apiserver was being announced as a mirror that was never deployed; - the four numeric knobs are validated rather than only documented, since a suffix or a leading zero on the budget reached the arithmetic in the block's first statement and unwound the whole function before its own headline, leaving a failing run with no diagnostics at all. Comments carry invariants rather than arithmetic. A comment that states a sum has to be kept true forever in every copy of it, and any edit to the code or the order falsifies one of them. The numbers live in the code as values and in the guard that pins them. Relates to #3656. ### Known gaps, deliberately not closed here Some collectors on this path carry no wall-clock bound at all. The image-cache reachability re-probe makes seven unbounded management-cluster calls, and the guest-Talos capture walks its VMI rows uncapped with applies and waits bounded only by `--request-timeout`, which bounds one HTTP request rather than a client retrying against a wedged apiserver. The phase budget stops them being started late; it does not stop one running long once admitted. They are pre-existing and tracked in #3666. Bounding the re-probe in particular would change when the same function gives up on the happy path, and so which image factory tenant workers use, which is not a diagnostic's decision to make. Named improvements left for later, none of them regressions introduced here: - the phase deadline is stamped at block entry rather than against what is left of the op, so a long bringup still narrows the room the derivation assumes; - `bringup` and `largest` are literals in the guard, so a collector heavier than today's will not move them, and the margin is small by design; - the integer-knob validator now exists in four places in this tree, and a shared `_lib` file sourced ahead of both libraries would collapse two of them; - the per-request bound equals the wall-clock bound, so both deadlines fire together and which of the two notes appears is a race; - the ordered list in the chainsaw comments omits the ungated wedge check, which runs first; - section letters no longer sort after the reorder, and the comment-to-code ratio in the touched block is high. ### Testing `hack/run-kubernetes-node-join_test.bats` is new: 22 cases driving the block with a `kubectl` stub that hangs, refuses and answers in part, and a `timeout` stub that fails any read not wrapped in the bounding idiom. It pins that no read escapes a bound, that a cut-off read is named rather than reported as an empty cluster, that the walk cap reports both counts, that a failed listing is not reported as no importers, that the phase declines the rest out loud, that a malformed knob falls back and says so, that the block returns zero so the caller keeps its `exit 1`, and the source order that keeps the discriminating reads ahead of the expensive collectors. Every one was checked by mutation against the unfixed shape. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(e2e): bound every read in the tenant Kubernetes node-join failure diagnostics and give the collection phase its own wall-clock budget, so a wedged apiserver can no longer hold the Chainsaw op until it is killed and take the tenant crust-gather snapshot with it ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved Kubernetes node-join and scheduling failure diagnostics with bounded collection times, clearer timeout and failure reporting, and more reliable preservation of critical snapshots. - Added safer handling for unavailable diagnostic data, missing tools, failed commands, and undeployed image caches. - Added configurable and validated timeout controls for image-cache and failure diagnostics. - **Documentation** - Clarified diagnostic budgets, collection ordering, and behavior when available time is exhausted. - **Tests** - Added comprehensive coverage for diagnostic timeouts, ordering, validation, failure handling, and bounded reads. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-09 12:33:27 +03:00
Aleksei Sviridkin	86fb92abf5	docs(tests): correct five stale claims in the e2e capture tooling (#3662) ... <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Five things in the e2e tooling said something the code no longer does, or did not say it at all. All five are prose; no executable line changes. `docs/agents/e2e-testing.md` section 8 described two of the three on-failure capture legs. The missing one is `hack/e2e-capture-dataplane.sh`, the only leg that reads forwarding state off the node rather than object state, and the only one whose two callers bound it differently, 600s from the BATS runner and 300s from the Chainsaw catch. That split is what decides whether something added to it fits, so a contributor who reads the document and not the script sizes a change against the wrong budget. Its notes contract is described with the carve-out the script declares itself: the two EndpointSlice reads in the LoadBalancer path report nothing either way, so a `<none>` backend line there is the one place a reader still cannot tell an endpointless Service from a read that never answered. The same section said the runner's captures serve "the BATS tests" in two places. They serve the e2e ones. The runner keys on an `e2e-` prefix, on the suite's own filename or on its parent directory, because all three legs read cluster state and a unit runner has `kubectl` on PATH with no cluster behind it. The legs are not gated alike either, so the sentence says which fired where: two key on `kubectl`, and the snapshot leg keys on `crust-gather` being installed, which on a unit runner it usually is not. Two numbers in the capture-gate rationale in `hack/cozytest.sh` no longer described the code. The snapshot leg's cost was given as 360s, the deadline passed to `timeout`; the leg is invoked as `timeout -k 30 360`, so the trap waits out 390s, which is what the comment forty lines below and the op-budget arithmetic in `.chainsaw.yaml` already said. And the data-plane leg was still described as having no bound on its first pod list, and billed at the full 600s on that basis, after that read was given a 28s ceiling. Two files enumerated the BATS suites and both stopped one short. The global catch in `hack/e2e-chainsaw/.chainsaw.yaml` said the runner it names as its analog "only covers the BATS install/openapi tests", and `hack/e2e-chainsaw/README.md` named the same two files. Both miss `hack/e2e-prepare-cluster.bats`, which `packages/core/testing/Makefile` drives through that runner, and which is the suite whose failures leave the sandbox in the state hardest to reconstruct afterwards. `docs/agents/e2e-testing.md` already listed all three, so the tree disagreed with itself depending on which file you opened. A comment in `hack/e2e-install-cozystack.bats` pointed at `hack/e2e-apps/run-kubernetes.sh`, which does not exist; that directory holds two monitoring suites and no runner. The readiness gate it means is the `rollout status` wait in `hack/e2e-chainsaw/_lib/talos-image-cache.sh`, along with both fallbacks to the public factory. relates to #3626 and #3642 ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note NONE ```	2026-08-09 04:08:35 +03:00
Aleksei Sviridkin	06078359d0	docs(e2e): record the phase-budget pattern beside its rule ... The e2e guide tells a collector author to size the op timeout above the sum of the inner budgets and let the inner ones fire. That works while the sum is something an op timeout can sit above, and on the tenant-Kubernetes node-join path it no longer is: the terms run past the 50m op on their own, and several of the reads there carry only --request-timeout, which bounds one HTTP request rather than a client retrying against a wedged apiserver, so parts of that path have no ceiling to add up at all. Left as it stood, the next collector author follows the documented rule and adds a bounded read to a sum that already does not fit. So the phase-budget pattern goes next to it, with the two properties that are easy to get wrong: the budget bounds when a collector may start rather than when the phase ends, so it is not a worst case and the comment beside it must not claim to be one; and because what runs last is what gets declined, source order decides which evidence survives a tight run. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-09 02:51:06 +03:00
Aleksei Sviridkin	9278702018	docs(e2e): make the op-budget comment name what it guarantees ... The 50m ceiling on the kubernetes-* script ops is justified by a comment enumerating what the failure path can spend, and the enumeration was both incomplete and reassuring in the wrong direction. It listed some collectors, said nothing about the node-join diagnostics or the image-cache diagnosis, carried a figure for the guest Talos capture that was half its real cost, and closed by asking the reader to keep the op above the sum, which no ceiling can do because the sum runs past what is left of the op on its own. It now names the phase budget as the thing that keeps the snapshot reachable and lists the collectors this ceiling covers, in the order the phase spends it. It states no sums and no per-collector figures. Why the budget is that number is derived once, beside the budget, and pointed at from here: a figure repeated across three files is a figure that drifts, and the drift is what the reader trusts. It also says what the ceiling does not promise. The budget bounds when a collector may start rather than when the phase ends; several collectors on this path carry no wall-clock bound at all; and the bringup's own waits already exceed this op on ceilings before a single diagnostic runs. So no worst case is claimed, because none can be stated while that holds. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-09 02:51:06 +03:00
Aleksei Sviridkin	0d7beaf630	fix(e2e): bound the tenant failure diagnostics per read and as a phase ... The node-join diagnostics issued about a dozen kubectl reads with neither an outer wall-clock bound nor --request-timeout, starting with a describe nodes through the tenant kubeconfig. That read targets the component least likely to answer in a node-join failure, and an unbounded read there does not lose only itself: it holds the Chainsaw op until the op is killed, so everything after it is lost rather than truncated, the tenant crust-gather snapshot the failing exit triggers above all. Each read now carries the per-call bound the newer collectors on this path already use, sized lower because this block issues a dozen back to back where those bound one. A read that fails or is cut off says so instead of passing in silence, and says only what was observed: a read that did not finish is silence about the cluster, not a finding about it. The importer walk gains a cap for the same reason its listing is now read on its own, since a listing that never answered must not read as a namespace with no importer in it. Per-read bounds alone do not end the phase, and the collectors here can together spend more than what is left of the op by the time they run. Much of that is not ceiling-bounded at all: on the guest-Talos path the applies, waits and deletes carry --request-timeout or --timeout, which bound one HTTP request rather than a client retrying against a wedged apiserver. So the phase is bounded as a phase. Once its budget is spent the collectors that have not started are declined out loud. That is not a worst case and is not written as one, but it does stop the snapshot being queued behind work begun past the deadline, which is how the snapshot was being lost. Raising the op ceiling was the alternative and it is not one: a larger ceiling times a sum that grows with every collector added is the same problem later. Because what runs last is what gets declined, the block is reordered so the cheap reads come first. The CSR list and the signer log are the discriminator for the failure mode a follow-up fix has to target; behind a serial-console walk and a guest-Talos capture that can spend the whole budget between them, they were exactly what a tight run would drop. The comments carry invariants rather than arithmetic. A comment that states a sum has to be kept true forever in every copy of it, and any edit to the code or the order falsifies one of them. The numbers live in the code as values and in the guard that pins them; the prose keeps why the budget comes from an inequality, why admission gates the start rather than the finish, why the cheap reads run first, and which residuals the guard's literals do not cover. The sibling scheduling-gate branch ends in the same exit and depends on the same snapshot, so its two reads are bounded too. So are the reads in the talos-image-cache diagnosis, whose gate also gains --ignore-not-found: exit 1 from kubectl get means a refused connection or Unauthorized as readily as NotFound, and reading it as absent announced the mirror as never deployed on the strength of a call that never reached the apiserver. Bounding those dumps needed a note of its own first, since timeout prints nothing when it fires and a cut-off dump would otherwise leave a bare section header that reads as a cache with no Pod, no Service and no EndpointSlice. The knobs those bounds come from are validated rather than only documented. Each is pasted somewhere that takes digits and nothing else, and each failed differently and quietly: a suffixed or leading-zero budget reached the arithmetic in the block's first statement and unwound the whole function before its own headline, so a failing run produced no diagnostics at all; a suffixed read bound became a millisecond request timeout and made every note blame the cluster for a value; zero disabled the bound outright; a suffixed importer cap made the comparison exit non-zero, so the cap stopped existing; and a non-numeric grace makes timeout exit before running the command at all. A rejected value falls back and says so, which is what the rest of this tree does with its own knobs. All four are re-checked where they are used, since a value set after sourcing never passed the assignment, and the read bound is checked again before the per-request string is composed from it so the two halves cannot disagree. Each default is declared once as a constant, and the re-checks read the bare variable so an unset knob is named rather than silently defaulted. When timeout itself is absent the reads run unbounded rather than being bounded with a binary that is not there: the latter turns every read into an exit 127 and every note into the cluster refusing, and collects nothing, where unbounded is what this path did before any of this. The phase says so once, and says what it costs rather than reassuring. The reachability re-probe in the image-cache helper stays unbounded and its comment names all seven of the calls it makes, the fire-and-forget delete after the probe Pod included: the same function decides the mirror on the happy path, so changing when it gives up is not a diagnostic's decision to make. Its bounds are spelled at each call site rather than precomputed, since a prefix built at source time cannot follow a knob a test lowers afterwards while the --request-timeout beside it does. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-09 02:51:06 +03:00
Aleksei Sviridkin	b9846fe42e	test(release): clean up the guard test's scratch dir without an EXIT trap (#3673) ... ## What this PR does `make unit-tests` fails on a clean `main`: `hack/bats-no-exit-trap.bats` reports `release-freeze-contract.bats` as holding one EXIT trap with no declaration. Neither change that produced this was wrong on its own. The per-file trap guard replaced a frozen global count with declarations carried by each file, and the backport guard test arrived with an EXIT trap from a branch that predated the guard. Each was green against its own base; together they are red. The fix follows the convention the guard exists to promote rather than declaring fresh debt: the scratch directory is removed on the last line of the test body. That also preserves the behaviour the rule is for, since a failing test never reaches the cleanup and leaves its directory behind for inspection. ## Testing `hack/release-freeze-contract.bats` and `hack/bats-no-exit-trap.bats` both pass on this branch. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved cleanup of temporary test files after release-freeze validation. - Preserved existing validation of release-freeze guard behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 21:12:05 +03:00
Aleksei Sviridkin	a042c2d66f	test(release): clean up the guard test's scratch dir without an EXIT trap ... The freeze-contract suite gained a test that shelves its scratch directory with an EXIT trap. The per-file trap guard added since then reports the file as holding an undeclared trap, so the aggregate unit target fails on a clean tree. Cleaning up on the last line of the test body is what the rest of the converted suites do, and it keeps the failure behaviour the guard exists for: a test that fails leaves its scratch directory behind for inspection, because the cleanup is never reached. Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 21:02:20 +03:00
Aleksei Sviridkin	1b178886dc	ci(backport): skip a target whose backport already merged (#3616) ... <!-- 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 Adds a guard step to the `backport` job in `.github/workflows/backport.yaml` that looks up whether a merged pull request already exists on a target's deterministic backport branch (`backport-<pr>-to-<target>`) and skips the checkout and the backport-action step for that leg when one does. A second label event on a PR whose first backport already merged, for example `backport-previous` added shortly after `backport` merged, re-enters this job for the already-delivered target, and cherry-picking the same commits onto a branch that already has them produces an empty pick, which git still reports with a non-zero exit code. Under the `draft_commit_conflicts` setting this workflow already passes to the action, that non-zero exit is read as a real conflict, and the action's attempt to commit the (empty) working tree fails too, surfacing as a "Backport failed" comment on the original PR, for a branch the change has been on for a while. Relates to #3588, which has the full mechanism and a reproduction. The guard only treats a target as done when a PR on that branch actually merged, so a real retry after someone manually closed a conflicting, unmerged backport PR still runs normally; both cases are covered by the new tests. This workflow triggers on `pull_request_target`, which, like `workflow_run`, executes the workflow definition from the PR's base branch (the default branch for the PRs this fires on), not from the PR head, for the same fork-isolation reason. That means this guard only takes effect once this PR merges to `main`; a `labeled` event on an in-flight PR before that still runs whatever version of `backport.yaml` is currently on `main`. The empty-cherry-pick-reported-as-conflict behavior itself lives in `korthout/backport-action` and stays out of scope here; issue #3588 links the upstream report. ### Screenshots Not applicable, no UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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 ci(backport): skip re-running a backport whose target already has the merged change, instead of posting a false failure comment ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented duplicate backport pull requests by detecting matching merged backports before creating new ones. * Preserved the existing backport flow when no merged backport is found. * **Tests** * Added coverage for merged, closed, and empty pull request scenarios. * Verified that duplicate checks run before checkout and backport creation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 20:48:24 +03:00
Aleksei Sviridkin	1b52832f37	fix(e2e): stop the data-plane capture reporting what it never observed (#3672) ... <!-- 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 Six fixes to the e2e host->pod data-plane collector and the two call sites that invoke it, all of one class: a diagnostic that states a fact it never observed. Every one of them makes a failed run look healthier than it was, which is the worst direction for a tool that only ever runs on an already-failed test. - A list cut mid-record parsed as a valid record. Three `awk -F'|'` row filters selected by field value and never checked the field count, so an IP cut mid-octet still looked like an IP and a cut inside a nodeName still looked like a scheduled pod. The capture then probed an address the cluster never had and opened a per-node file for a node that does not exist. Each jsonpath emits a fixed field count per record, so a shorter row is a fragment by construction. - Five distinct paths ran zero probes and all reported "reachable, skipped": no node to probe from, a Service with no port, a probe node with no `kube-ovn-cni` pod, a host netns with no `nc`/`curl`/`wget`, and an exec that could not run. An image without a probe client alone would stamp every LoadBalancer in the cluster reachable and skip the whole heavy capture. They now report unprobed, each with a reason the run can actually support. - Both EndpointSlice reads discarded stderr and were read by emptiness alone, so a refused read wrote the same `<none>` a Service with no endpoints writes. They now carry their status. That change armed a landmine the silence had hidden: the target port was read with `{.items[0]...}`, and client-go's `evalArray` has no `allowMissingKeys` escape, so an endpoint-less Service is a hard error rather than an empty answer. The read now asks with `{.items[*]...}`. - `pod_on_node` turns its node argument into `--field-selector spec.nodeName=<node>`, so an empty node asks for pods whose nodeName is empty, i.e. unscheduled ones. Both tcpdump legs could hand a pending pod to a capture stamped ANNOUNCER or ENDPOINT. Each leg is now guarded and says in the artifact that it was skipped. - Both callers wrapped the collector in a wall-clock backstop and discarded the result, while the previous-logs leg beside each of them reports "INCOMPLETE (exit N)". A truncated `dataplane/` directory was indistinguishable from a complete one that found little. - The file's own header credited one caller where two exist, under different backstops (600s and 300s), so a change sized against the named one can overrun the omitted one. Diagnostics only. No retries, no cluster mutation, no change to any test's pass/fail outcome: the statuses that are now read are used to print a line and nothing else. Every behavioural change is pinned by a test in the existing bats surface, verified red against the committed tree before the fix. The two suites run 53 and 5 tests. Known limits and deliberate omissions, named so the next reader does not have to rediscover them: - A field count cannot catch a cut inside a record's LAST field, since every separator has already arrived. The comment on the first filter says so and says where it lands for each of the three. The MetalLB speaker loop is left without a count for the same reason: its records carry two fields, and `speaker-1|nod` is indistinguishable from a genuinely short node name, so a count there would reject nothing while reading as a closed gap. - The backstop lines report any non-zero status as `INCOMPLETE (exit N)`. Exit 127 (no `timeout` on PATH) means nothing ran, so "kept what landed" overstates it. Left alone deliberately: the same wording ships on the previous-logs and crust-gather legs beside them, and splitting only the two new ones would create the asymmetry this PR exists to remove. All four want the same treatment together. - When a probe's cni-server lookup fails once and then succeeds while no probe client is present, the reason line names the lookup for all three attempts. The named cause did occur, so nothing false reaches the artifact, but it is narrower than what happened. - `hack/capture-dataplane.bats` now needs `yq` for one test, and unlike three sibling suites it does not declare that at the top of the file. The fan-out caps are sized in units of work while the backstops are wall-clock, and the worst case exceeds both by multiples. That predates this branch and is unchanged by it, but the new INCOMPLETE line is what will make it visible in an artifact for the first time. Tracked separately in #3671. relates to #3658 #3660 #3654 #3657 #3663 #3649 ### 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): the host->pod data-plane capture no longer reports absence or reachability it never observed -- truncated list records are dropped instead of parsed, a LoadBalancer no probe reached is recorded as unprobed rather than reachable, an unread EndpointSlice backend is distinguished from an absent one, a tcpdump leg whose node was never identified is skipped instead of aimed at an unscheduled pod, and both callers report a capture their backstop cut short ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved data-plane capture reliability and reporting for timeouts, failed or partial reads, and missing collectors. * Prevented incomplete records and unknown probe results from being treated as valid or reachable. * Avoided capturing from unintended nodes when node information is unavailable. * Preserved partial diagnostic output while clearly reporting incomplete captures. * **Tests** * Expanded coverage for capture outcomes, artifact accuracy, timeout handling, and guarded node selection. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 20:47:11 +03:00
Aleksei Sviridkin	9a0c8c75f3	fix(ci): close the silent-green gaps in e2e selection and the helm unit sweep (#3670) ... ## What this PR does Four fixes in e2e test selection and the helm unit sweep. Relates to #3650 #3652 #3655 #3646. select-e2e built both dependency indexes as `$(build_owners_index | sort -u)`, so it read sort's exit status and never yq's (#3650). A broken yq left the index empty, every path fell through to escalation, and CI ran the full suite on every PR. The output was identical to "no owners matched", so it looked like the conservatism the selector is supposed to apply. The suite list one line up had the same shape and a worse result. `$(find ... | sed | sort)` reports sort, never find, and errors went to /dev/null. Every escalation prints that list, so a failed find made "escalate to the full suite" emit a blank line, which both e2e lanes read as "skip Chainsaw" before posting `E2E Tests` green. The outcome that means run everything delivered a run of nothing. find is not the only way to get an empty list, and the escalations are not its only reader, so the check sits where the list is built. The `examples/backups/<app>/` rule escalates nothing and still membership-tests against that list: with it empty, an edit to a backup harness that should run its suite reports nothing to run, on a path no escalation-side guard would ever see. An empty list is a broken enumeration, so the script refuses to answer at all. Both lanes run the step under `bash -e`, so the non-zero exit fails the job. I checked both workflows: neither overrides `shell:`, both capture the selector as a plain assignment. Fourteen tests used `[ "$(echo "$output" | wc -w)" -gt 5 ]` to mean "everything was selected" (#3652). There are 21 suites, so that passes on any six, and partial escalation stayed green. They compare against the whole list now, derived the way the script's own full-suite branch derives it. Drop one suite from that branch and thirteen of them go red. All thirteen passed under the threshold. The fourteenth is the cilium case, and equality shows it was never a full-suite path at all. It resolves through the dependency graph, which reaches 19 of the 21 suites. The two it misses are kuberture and securitygroup, whose sources are not `*-application`, and the mapping covers only those plus external-dns by name. Both controllers declare `cozystack.networking`, so a CNI change that breaks either one runs neither suite. That gap is #3665. The test is renamed for what it measures and pins the 19, so closing #3665 turns it red at the right moment. Editing `hack/e2e-chainsaw/backup/chainsaw-test.yaml.disabled` selected the full suite (#3655). The suffix was ignored when deriving the suite name, the intersection against existing suites emptied, and the empty-selection safety net escalated. A disabled suite runs nowhere, so its edits are the one file class that cannot regress a test, and they were buying the most expensive outcome the selector has. The selector now has a fourth outcome, a failed step rather than a selection, so the contract enumerations in the script header, in `docs/agents/e2e-testing.md` and in both workflow comments say so. The helm unit sweep never visited `packages/tests` (#3646). cozy-lib-tests, the whole test coverage for a helper library that 34 charts consume by symlink, ran only because `packages/library/cozy-lib/Makefile` delegates its `test` target to it. Remove that one line and the suite goes from one execution to zero with the sweep still exiting 0 and still printing "All Helm unit tests passed". Every check the sweep makes runs inside a directory the loop already reached, so a package that never enters the loop takes all of them with it, and what is left is `tests_found`, which counts suites across the whole run rather than per package. The directory is on the list now: 76 executed suites become 77, at 0.1s to 0.3s over six runs against a sweep of roughly 31 seconds. The delegation stays for `make -C packages/library/cozy-lib test` ergonomics, so the suite runs twice. Neither run is load-bearing alone. ### Testing `hack/select-e2e_test.bats` goes from 27 tests to 32, `hack/helm-unit-tests.bats` is new. Each commit is green on its own: 27, 30, 32, 32. The new tests stub the failing component and assert the exit status plus the line naming what broke: a broken yq, a broken find, an empty suite list across all three diff classes, the disabled-suite path, and a guard that the disabled rule drops only that path and not the rest of the diff. The yq test asserts exit 0 on purpose. A missing dependency graph still has a correct answer, the full suite, and a missing suite list does not. Pinning both halves keeps the header and the docs from drifting into claiming otherwise. Two suites fail on my macOS box and fail identically at `origin/main`, so neither comes from this branch: `hack/migration-seaweedfs-db-adopt.bats`, where a container running as the host UID cannot write the bind-mounted temp dir under Colima, and `hack/seaweedfs-naming-audit.bats`, which I confirmed red at the merge base but did not dig into. Every other `hack/*.bats` was run individually and is green. `make unit-tests` stops at the first failing file and both of those sort ahead of `select-e2e_test.bats`, so on macOS the suites this PR touches have to be run directly. ### Named improvements, not done here - The `.disabled` rule keys on the file rather than the parked directory, so it is both wider and narrower than the property it wants. A support file under `hack/e2e-chainsaw/backup/` still buys a full run, and a `.disabled` file dropped inside a live suite directory now selects nothing where it used to select that suite. The property covering both is one level up: a suite directory holding no live `chainsaw-test.yaml` is inert, testable with the same `grep -Fxq` membership check the `examples/backups/*/*` arm already uses. It is not a free win, since a half-written suite directory escalates today and would then select nothing. A cheaper half step is narrowing the glob to `hack/e2e-chainsaw/*/chainsaw-test.yaml*.disabled`, because case globs cross `/` and the shipped pattern also matches nested paths. - `packages/tests/**` is not classified by the selector and reaches the full suite through the unclassified fall-through instead of a rule. Tracked in #3667. - `full_suite_list()` in the bats file cannot fail. cozytest.sh's awk rewrites a lone `}` into `return 0; }`, so a broken find inside the helper would return an empty expected set instead of a red test. Nothing false can pass today, because the script hard-fails on an empty suite list before any assert runs, but that helper is what every equality assert measures against. - The yq check runs before the changed-file list is read, so with a broken graph even a docs-only diff escalates. Measured: a broken yq plus a `docs/x.md` diff returns 21 suites at head against 0 at the merge base. Only reachable in an already broken state, but the graph is consulted at the component lookup and in the closure walk, so moving the check to first use would charge nothing to docs, dashboards and inert-meta diffs. ### Screenshots No UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` file by file against the diff: four files under `hack/`, one under `docs/agents/`, and comments in the two e2e workflows. Nothing under `hack/` was moved or renamed and no make-target contract changed, so the ccp entry does not fire. It gates on `hack/package.mk` and `hack/common-envs.mk`, neither of which is in the diff. The node prerequisites in `hack/e2e-prepare-cluster.bats` that ansible-cozystack mirrors by hand are untouched, and so is `.github/workflows/tags.yaml`. `docs/agents/` is internal contributor documentation, not a website source. No package was added, renamed or removed, and no `values.schema.json` or version enum changed, so the website and provider entries do not fire either. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(ci): select-e2e no longer reports an empty selection when it cannot enumerate the Chainsaw suites, which made an escalation to the full suite indistinguishable from "skip Chainsaw" and left the E2E gate green with nothing run. A failure to list the suites is now named on stderr and fails the step, a yq failure is named and still escalates to the full suite, and an edit to a disabled Chainsaw suite is inert instead of escalating to a full run. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved end-to-end test selection for disabled, empty, or partially changed test suites. * Test selection now safely escalates to the full suite when suite discovery or dependency analysis fails. * Helm unit-test sweeps now include tests under `packages/tests`. * **Tests** * Added coverage for suite discovery failures, disabled suites, command failures, and exact test selection. * Added Helm test-sweep coverage, including failure handling. * **Documentation** * Clarified test-impact analysis rules and suite-selection outcomes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 20:47:06 +03:00
Aleksei Sviridkin	355fc8a24a	test(tests): let each bats file declare the EXIT traps it still holds (#3622) ... <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The ban on EXIT-trap cleanup in `hack/*.bats` was enforced by two lists inside `hack/cozyreport.bats`: one naming the files known to be clean, one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard kept failing. It is worth being exact about how badly, because the history is sharper than "it went stale a few times". **The inventory was never correct on main for a single commit.** It landed in #3567 at 15:25:20. #3195 had landed `multus-install-cni-plugins.bats` carrying twelve traps at 15:24:37, forty-three seconds earlier. Replaying the old guard's own logic against the tree at the very commit that introduced it already gives `found != frozen`. Main then stayed red for roughly twenty-two and three quarter hours. The first repair, #3584, landed *already red* the next morning at 10:42:43, because #3548 had brought in `run-kubernetes-talos-diagnostics_test.bats` with eight traps at 10:40:03, under three minutes ahead of it. That fix was correct and bought zero green time. Green arrived only with the second repair, #3602. Neither pair of PRs shared a line, and each was green against its own base. That is the whole mechanism, and it is why a third one-line repair is not the answer. The mechanism is structural rather than careless. A change that adds a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second one landed. A file *arriving* with traps was worse still, because nothing in its author's diff pointed at that string at all. That is exactly how `run-kubernetes-talos-diagnostics_test.bats` got in, twice. That contention is not in the past tense. Two open PRs are editing that one line right now, and they disagree about what it should say: #3575 adds `run-kubernetes-talos-diagnostics_test.bats=8` to it, repeating a repair that has already landed, and #3441 removes `select-e2e_test.bats=15` from it, because it converts that file. Neither PR is about EXIT traps. Both have to touch that string anyway, and whichever lands second is wrong until someone edits it again. So this PR is not fixing a red main. It removes the thing that keeps making main red, which is why it is worth more than the one-line fix that is now the established habit. One honest caveat about its own landing. The textual-conflict property is steady-state: a branch forked *before* the declaration exists has no line to disagree with, so it converts traps in its own file, merges clean, and the count only goes wrong once both sides are on main. I checked this against #3441, which takes `select-e2e_test.bats` from fifteen traps to zero. Merged after this, that file would declare fifteen and hold none. What the move buys even in that case is that the red names a file the branch already edited, the repair is one line inside it, and rebasing before merge catches it on the branch's own CI. None of those three held against the central inventory. Whoever merges this should expect one such adjustment on the conversion branches still in flight. So this replaces both lists with a declaration each file makes about itself: one `# EXIT-TRAP DEBT: N` comment in its leading comment block. A file carrying no declaration must install no EXIT trap. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count get a real textual conflict instead of silently invalidating each other, and a change that leaves the traps alone edits nothing. **The include list is redundant, not lost.** It named the files proven clean, so that a trap reappearing in one of them would fail. Under the new rule those files carry no declaration, and a file with no declaration must hold zero traps, so a trap reappearing in any of them fails on its own, with no list to be on. Coverage widens rather than narrows: the two lists named twenty files between them, and the rule covers all fifty bats files under `hack/`, subdirectories included, plus the ones added tomorrow. Rebasing this branch onto current main is the property working. Main has since gained `hack/kubernetes-pre-delete-hook.bats` and `hack/tenant-pre-delete-hook.bats`, and `hack/cozyreport.bats` grew by some eight hundred lines. Neither new file installs an EXIT trap, so neither needed a declaration and neither needed an edit here; the rebase took no conflict at all. Under the inventory, each arriving file was a coin toss on whether somebody had remembered the string. To be precise about what the inventory could and could not do, since it is easy to overstate: it did *notice* a new file carrying traps. The string it compared was built by scanning the directory, so an arriving file appended a token and failed the comparison, which is exactly how main went red. What it could not do is let that file arrive without an edit in a foreign suite. Being seen and being absorbable are different properties, and only the second one decides whether two changes can land independently. The declaration is pinned in both directions. Declaring N while holding N+1 fails, obviously; declaring N while holding N−1 fails too. Without that second half the number becomes a ceiling and rots upward: somebody converts half a file, the declaration stays, and the guard quietly licenses traps that were removed long ago. It is read only from the leading comment block, under the shebang and above the first line of code. A `.bats` file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test; honouring it there would let an unrelated fixture excuse a real trap, silently, with nothing in that test's own diff looking like a declaration. A comment block is the region with no interior; stopping instead at the first `@test` would still read a line out of a helper's heredoc. Not every counted handler is debt. A trap inside an explicit subshell does not replace the one the `bats` binary installs, so a test failing inside `( … )` still prints its `not ok`, checked against a test-level trap in the same file, where the TAP line vanishes. `hack/e2e-test-openapi.bats` kills a backgrounded `kubectl proxy` exactly that way, and "convert it like the others" would leak a process holding a fixed port and wedge the next run. Its declaration now records the carve-out instead of scheduling a conversion, and because the ratchet is exact in both directions, *removing* that trap fails too, so the count protects the construct rather than marking it for deletion. `docs/agents/e2e-testing.md` previously scoped this exception to Chainsaw `script` steps only; it now names the BATS subshell case as well. The counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed `trap … EXIT; cd "$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap … EXIT` and `(trap … EXIT; true)`; upper case missed `trap … exit`, which bash and dash both install. All of those are real handlers that scored zero. The left boundary is the one worth dwelling on, because the inventory being replaced had none at all and *did* catch the semicolon form. Getting it wrong here would have narrowed coverage while the commit claimed to widen it. A plain word boundary is not enough either: `bootstrap ` ends in `trap `, and it has to keep scoring nothing, or the documented answer to a red guard (add a debt line) would buy a file a permanent licence for one real trap to silence a line that has none. Two handlers sharing one line are reported rather than counted, because the count is a count of lines and the second would otherwise arrive free. Splitting such a line properly needs a shell parser, since a semicolon inside a handler's own quoted action is not a separator, and guessing wrong undercounts, the one direction a ratchet cannot afford. **What this does not fix.** The declaration is still a loophole: a new file can write `# EXIT-TRAP DEBT: 8` instead of cleaning up, and nothing here makes that impossible. What changes is that the admission is local and visible. It sits at the top of the file it excuses, in front of whoever reviews that file, instead of being a number in a neighbouring suite nobody in that review is reading. Today's loophole is the same size and invisible. Three more limits, all stated in the guard's own header rather than left to be discovered. The scan is lexical, so a signal computed at runtime and a quoted action spanning physical lines without a backslash are both invisible. An exact count catches addition and removal but never substitution: swap the openapi file's subshell trap for a test-level one and the total stays 1. And the scan reads `.bats` only, so a handler arriving through a sourced `.sh` is outside it. `hack/e2e-chainsaw/_lib/run-kubernetes.sh` installs two right now, and each is benign for its own reason rather than by design: the one in `cozy_capture_tenant_talos` because that function is declared with `(` and so runs in a subshell, the one in `run_kubernetes_test` because no `@test` calls it despite being declared with `{`. Three `hack/*.bats` source that library, and two tests in the converted file call `cozy_capture_tenant_talos`, so flipping a single `(` to `{` reinstates a test-level handler in both of them with the guard green. Widening the scan to `.sh` would mean counting handlers that are correct in a script and wrong only in a test body, so the honest answer is that this is where the instrument stops. Three further boundaries, recorded here so they land as known edges rather than as surprises. The old include list also failed when a file named on it disappeared from the tree; the new rule can only judge a file that is present, so a deleted converted file goes unnoticed. That is a genuinely smaller check, though its absence shows up in the diff that deletes the file. The guard's own failure messages are code lines, so they are scanned by the pattern they belong to: they pass today only because no bare `EXIT` or `0` happens to follow the keyword in any of them, and a rewording that introduced one would make the file demand a debt of itself. It fails loudly rather than quietly, and the fixture writers and test titles already split the keyword from the signal for this reason, but the messages do not. Finally, `docs/agents/e2e-testing.md` bans test-level `EXIT` *and* `RETURN` traps, while every mechanical guard this repo has had, the one being deleted included, matches only `EXIT` and `0`. `hack/` holds no RETURN trap today, so nothing regresses here, but half of that documented rule has never had an executor. The guard moves out of `hack/cozyreport.bats` into `hack/bats-no-exit-trap.bats`, because its subject is every unit suite under `hack/` and not the report collector it grew up in. Living inside `cozyreport.bats` is precisely why unrelated PRs kept converging on one line. `hack/md-no-hardwrap.bats` is the neighbouring precedent. Separately, the eight EXIT traps in `hack/run-kubernetes-talos-diagnostics_test.bats` are converted to end-of-body cleanup rather than given a declaration. That file was written six weeks after the ban was documented, so "it predates the rule" is not available to it, and handing it a debt is literally the move that failed the last two times. The remaining files keep their declarations, since a mass conversion would collide with the branches that own them. One note on where the prose sits. The guard's own header carries only what a reader has to keep true: why the ban exists, the two carve-outs, where the declaration lives and why there, that the count is exact in both directions, why the marker is anchored at column zero, and where the lexical scan stops. Everything above about *how* the inventory failed (the timestamps, the PR numbers, the arithmetic) is deliberately not in the file. It is true today and will rot on its own; a comment nobody is obliged to maintain is how a file grows a paragraph that is quietly false. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note test(tests): each hack/*.bats file now declares its own remaining EXIT-trap debt in a `# EXIT-TRAP DEBT: N` header comment, checked by hack/bats-no-exit-trap.bats, replacing the central inventory in hack/cozyreport.bats ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Testing** - Added automated auditing for `EXIT` traps across Bats end-to-end tests, including validation of tracking declarations and edge cases. - Improved diagnostics tests by replacing trap-based temporary-directory cleanup with explicit cleanup steps. - Added tracking annotations for remaining trap-related cleanup work. - **Documentation** - Clarified when traps are permitted inside self-contained subshells and how remaining cleanup debt is reported. - Updated review guidance for consistent end-to-end test maintenance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 20:46:49 +03:00
Aleksei Sviridkin	c6961541f3	fix(tests): report a data-plane capture cut short by its backstop ... Both callers of the data-plane collector wrap it in a wall-clock backstop and threw the result away with `|| true`, while the previous-instance logs leg sitting beside each of them records the exit status and prints "INCOMPLETE (exit N); kept what landed in ...". When a backstop killed the collector, nothing said so, and a truncated dataplane/ directory read exactly like a complete one that found little. The collector itself now names every read of its own it could not finish, which left these two call sites as the only remaining way for that leg to die without a word. The Chainsaw one is the tighter of the pair -- 300s against 600s, because it shares an op envelope with the snapshot leg -- so it is both likelier to fire and more expensive to leave silent. In each place the status is read, never propagated: a diagnostic still cannot change a test's outcome. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 17:08:54 +03:00
Aleksei Sviridkin	9b11db1b03	fix(e2e): report an LB no probe reached as unprobed, not reachable ... Five inputs walked into the LB probe gate's skip branch having run zero probes, and all five came out as "probe: LB reachable or not probeable ... -- reachable, skipped": no node to probe from, a Service with no port, a probe node the lookup says runs no kube-ovn-cni pod, a host netns with no nc/curl/wget, and an exec that could not run (no timeout on PATH, exec denied, the pod gone mid-probe). None of them observed the address. An e2e image without a probe client would stamp every LoadBalancer in the cluster reachable and skip the whole heavy capture, which reads as a healthy datapath. A sixth way in, the cni-server lookup not answering at all, already reported unknown and still does. Zero probes now routes to that same explicit unknown outcome, and each way of getting there carries a reason the run can actually support. The two structural inputs are named where they are detected. The rest are told apart by the shape of the outcome set, which is the only place the difference survives: the probe helper emits an explicit unknown token only when the lookup itself did not answer, so a set that is present and wholly unknown names the lookup, while an empty set names the three ways a probe dies after the lookup answered. Those three stay together -- the exec's status is swallowed by design, since this collector never fails a job -- but none of them is reported as a lookup that went unanswered, because it did not. The reachable line now says which evidence produced it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 17:08:54 +03:00
Aleksei Sviridkin	ed1c9e774b	fix(e2e): say when an LB backend is unread rather than absent ... The two EndpointSlice reads in the LB section discarded stderr and were interpreted by emptiness alone, so a refused or cut-off read wrote the same `<none>` into the artifact that a Service with no endpoints does. That is the assertion without the refutation: a reader holding the uploaded report, and not the run, has no way to tell which happened, and the section is at its weakest exactly when the apiserver is struggling. Both reads now carry their status beside the value, in the vocabulary the per-node captures already use. Reporting the status arms a landmine the silence used to hide: the target port was read with a jsonpath indexing `items[0]`, and client-go's evalArray has no allowMissingKeys escape, so an empty list is a hard error and kubectl exits 1. A Service with no endpointslices is an ordinary answer here and would have started reporting as a failed read, so the read now asks with `items[*]` -- empty output, exit 0 -- and takes the first value, the way the pod lookups already do. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 17:08:54 +03:00
Aleksei Sviridkin	33f672f3fc	fix(e2e): skip a tcpdump leg whose node was never identified ... pod_on_node turns its node argument into `--field-selector spec.nodeName=<node>`, and an empty node asks for pods whose nodeName is empty -- the unscheduled ones. So an unidentified node did not yield "no pod found": it yielded the first pending kube-ovn-cni pod in the namespace, and the tcpdump then ran against a pod that is not on that node and may not be running at all. Both halves of the cross-node capture had this, and each gap opens in the very state its own leg exists to characterise. The announcer is unknown exactly when MetalLB is misbehaving; a Service with no ready endpoint is the ordinary shape of an LB outage, and the endpoint side carried two such lookups, the cni-server pod and the backend's OVS interface. In either state the artifact came out holding a tcpdump from an unrelated pending pod under an ANNOUNCER or ENDPOINT label, or a confusing exec failure -- both pointing away from the actual problem. Each leg is now guarded the way the static capture above it already was, and records in the artifact that it was skipped rather than leaving a silent gap. The guards stay at the call sites rather than moving into pod_on_node: that helper serves five callers, and an early return there would have to invent a status for a lookup nobody made. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 17:03:00 +03:00
Aleksei Sviridkin	0cc49ad34b	fix(e2e): drop truncated records in the data-plane capture filters ... The record filters selected rows by field value and never checked the field count, so a list cut mid-record parsed as a valid record: an IP cut mid-octet still looks like an IP, and a cut inside the nodeName column still looks like a scheduled pod. The capture then probed an address the cluster never had and opened a per-node file for a node that does not exist -- confidently labelled data about objects that are artifacts of the cut. The kubectl jsonpath behind each filter emits a fixed field count per record, so a shorter row is a fragment by construction: seven for the Service list, six for the pod list, five for the endpoint list. Dropping it costs the reader nothing -- the read's own note already says the list did not finish. The endpoint filter is the one that matters most, because it stops at the first row it accepts: a fragment at its head is not one bad row among many, it is the backend, and its half-parsed node is the node every endpoint-side capture then aims at. Where the class lives in this file, so the map does not have to be rebuilt: four sites parse records, and the two loops walking pods and LoadBalancers are not among them -- they consume what these filters already returned, so a cut is caught upstream of them. Three of the four are the filters guarded here. The fourth, the MetalLB speaker loop, is at the limit of what a field count can do: its records carry two fields, a cut in the first leaves no separator and its existing emptiness guard already rejects that, and a cut in the second delivers every separator, which nothing in-band tells apart from a genuinely short node name. That limit is not confined to the speaker loop, and the comment on the first filter now says so rather than implying a guard that holds everywhere: a cut inside any record's LAST field passes a field count. It lands on a display-only column for Services, and on a decision column for the other two, where a truncated value matches neither state being excluded and the row is kept -- over-capturing rather than dropping evidence. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 17:03:00 +03:00
Aleksei Sviridkin	f266705784	docs(e2e): name both callers of the data-plane capture in its header ... The header credited cozytest.sh alone, while the Chainsaw global catch invokes the same script under a different wall-clock backstop -- 300s against cozytest.sh's 600s. A reader sizing this collector's budget against the caller the header names can still overrun the one it omits, and the arithmetic further down the file already assumes both exist. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 17:03:00 +03:00
Aleksei Sviridkin	33bd161a17	test(helm): sweep packages/tests so the library suite stands on its own ... The helm unit sweep iterates packages/apps, core, extra, system and library, and never visits packages/tests. The cozy-lib-tests suite — the whole test coverage for a helper library 34 charts consume by symlink — runs only because packages/library/cozy-lib/Makefile carries a line delegating its own 'test' target to it. Deleting that one line takes the visited-package count for the suite from one to zero and the executions from one to zero, with the sweep still exiting 0 and still printing "All Helm unit tests passed": the no-suites backstop is satisfied by the other directories. Nothing else in the sweep could have caught it. Every check it makes — the Makefile test, the `make -n test` probe, the collection of failing directories — runs inside a directory the loop already reached, so a package that never enters the loop takes all of them with it. What is left is tests_found, and that counts suites across the whole run rather than per package, so the other directories keep it satisfied. Add the directory to the loop. It holds one package today, so the sweep grows from 76 executed suites to 77, and that suite costs between 0.1s and 0.3s over six runs against a sweep of roughly 31 seconds. The delegation stays for the ergonomics of `make -C packages/library/cozy-lib test`, which means cozy-lib-tests now runs twice; the point is that neither run is load-bearing alone. Covered by a new bats file: one test that the sweep reaches the directory and executes what it finds there, one that a red suite there still fails the sweep rather than being swallowed. Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 16:46:36 +03:00
Aleksei Sviridkin	1abc653cfb	fix(ci): treat a disabled chainsaw suite as inert in test selection ... Editing hack/e2e-chainsaw/backup/chainsaw-test.yaml.disabled selects the whole Chainsaw suite. The per-suite rule reads the suite name off the directory and ignores the suffix, so the change selects "backup"; the final intersection against the suites that actually exist finds no such suite and empties; and the safety net for an empty selection escalates to the full run. A disabled suite is registered nowhere and executed by nothing, which makes its edits the one file class that provably cannot regress a test. The mapping had it backwards: they bought the most expensive outcome the selector has. Rare in practice, and paid on every commit by anyone grooming or re-enabling such a suite. Classify *.disabled as inert ahead of the per-suite rule, and record it alongside the other rules that select nothing so the header stays an accurate enumeration. The safety net keeps its job for paths no rule has looked at, which is what it was added for. Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 16:31:39 +03:00
Aleksei Sviridkin	37a395363f	fix(ci): stop select-e2e from deciding on inputs it never checked ... Three holes on the same seam: the script builds a list, never looks at whether the command that built it succeeded, and then decides from the empty result. The dependency indexes are built as `$(build_owners_index | sort -u)`, where the function is a single yq. A pipeline carries the status of its last command, so the script sees sort's zero and `set -e` never fires: a missing binary or a malformed source leaves the index empty, every changed path falls through to escalation, and CI runs the full Chainsaw suite on every pull request. The direction is safe and that is the problem — an empty index and "no owners matched" print the same thing, so the symptom is a full run, which reads as the conservatism the selector is supposed to apply. Selection can stay switched off indefinitely with nobody signalled. Check each half's status on its own and name yq once on stderr before taking the fallback the empty index would have reached anyway. The suite list one line up has the identical shape and a worse consequence: `$(find ... | sed | sort)` reports sort, never find, and errors went to /dev/null. That list is what every escalation prints. So a find that failed produced an empty list, and "escalate to the full suite" then emitted a blank line — which both e2e lanes read as "skip Chainsaw" before posting the required "E2E Tests" status green. The one outcome that means run everything delivered a run of nothing, on the pull request that earned a full run. Capture find on its own, check it, let its errors through. A failing find is not the only way to end up with an empty list, and the escalations are not its only reader, so the check sits once where the list is built. The examples/backups/<app>/ rule escalates nothing and still membership-tests against that list: with it empty the test fails, the app is not selected, and an edit to a backup harness that should run its suite reports nothing to run — output identical to a legitimately empty selection, on a path no escalation guard would ever see. An empty list is a broken enumeration, a moved directory or a wrong working directory, not a project without tests. Refuse to answer at all. Both lanes run the step under `bash -e`, so a non-zero exit fails the job rather than falling through to the empty selection this is here to prevent. Each is covered by a test that stubs the failing component and asserts both the exit status and that the script names what broke. Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 16:17:31 +03:00
Aleksei Sviridkin	b1f413d537	test(ci): assert the whole suite list where select-e2e must escalate ... Fourteen tests read `[ "$(echo "$output" | wc -w)" -gt 5 ]` to mean "the full suite was selected". A word count clears that bar on any selection of six or more, and the tree holds twenty-one suites, so a selector bug that escalates to most of them but not all stays green in every one of those tests. Partial escalation is the failure worth catching on exactly these paths; the threshold is blind to it and only sees total failure. Compare against the whole list instead, derived the way the script's full-suite branch derives it rather than pinned as a literal, so adding or disabling a suite does not need fourteen edits here. On a one-suite drop injected into that branch, thirteen of the converted tests go red; under the threshold all thirteen stayed green. The fourteenth is the cilium case, and equality shows it was never a full-suite path: packages/system/cilium is owned by cozystack.networking and resolves through the dependency graph, which reaches nineteen of the twenty-one suites. The two it misses, kuberture and securitygroup, sit on sources that are not *-application, and the source-to-suite mapping covers only those plus external-dns by name. Both controllers declare cozystack.networking as a dependency, so a CNI change that breaks either one runs neither suite. That gap belongs to the selector; the test is renamed for what it measures and pins the nineteen, so closing the gap fails it rather than passing unnoticed. Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 14:56:56 +03:00
Aleksei Sviridkin	92ec49e64e	fix(tenant): retry failed monitoring and gateway installs in place (#3633) ... <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The tenant chart renders monitoring and gateway with no install strategy, so they get Flux's default, and install remediation is an uninstall in every case Flux offers. Neither sets `spec.uninstall.disableHooks`, which defaults to false. So a release that missed nothing but a readiness budget gets uninstalled, and the app chart's `post-delete` cleanup Job runs against it. Those Jobs delete on purpose, and they are right to. The monitoring one removes PVCs labelled `apps.cozystack.io/application.name=<release>-system`. That label is not incidental either: the metrics and logs claim templates stamp it so the operator copies it onto the storage PVCs, and the Job selects on it to stay release-scoped. Both ends were built to meet. The result is that a slow first install deletes the tenant's metrics and logs, and nothing reconstructs them. The gateway Job deletes the `cozystack-acme-account` Secret. That key regenerates, so this one costs no data. It costs a re-registration against the ACME provider, and repeated cycles run into its rate limits, which can hold certificate issuance for the tenant down for hours. Smaller stake, same mechanism, and the two are not one class: only one of them owns storage. `RetryOnFailure` keeps the manifests applied and retries the failed release, so no uninstall happens and neither Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. Two things are traded away. A real upgrade failure now retries instead of rolling back, which gives up no stable safety net, since with `retries: -1` the default strategy was already rolling back and re-upgrading without end. And the uninstall was the only automatic clean slate, so an install wedged on a bad resource state retries against that state until an operator removes the release. For a release that owns data that is the right exchange. This removes the consequence rather than the trigger, and for monitoring the trigger is worth naming separately: the parent HelmRelease here waits `10m`, while the child `monitoring-system` release sets `15m` deliberately, with a comment explaining that 10m is too short for the OIDC users-Job inside its `activeDeadlineSeconds: 900`. The parent's wait covers the child, so the parent can fail while the child is behaving exactly as designed. That mismatch is untouched here and wants its own change. The chart's etcd and seaweedfs releases lose storage the same way and are outside this change. Beyond those four, computeplane, ingress and info stay on the default strategy, but their charts ship no destructive `post-delete` hook, so remediation costs them a wasteful teardown rather than data. Same shape as #3552 and #3581. Relates to #3624. ### 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(tenant): retry a failed monitoring or gateway install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the tenant's metrics and logs volumes or its ACME account key ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Monitoring and gateway deployments now automatically retry failed installs and upgrades every 30 seconds. * Existing unlimited remediation retries remain available, improving recovery from deployment issues. * **Validation** * Added automated coverage to verify retry behavior for both monitoring and gateway deployments. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 14:04:20 +03:00
Aleksei Sviridkin	36f83fa9b7	fix(tenant): retry failed etcd and seaweedfs installs in place (#3630) ... <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The tenant chart renders the etcd and seaweedfs releases with no install strategy, so they get Flux's default, and install remediation is an uninstall in every case Flux offers. Neither release sets `spec.uninstall.disableHooks`, which defaults to false, so that uninstall runs the app chart's `post-delete` cleanup Job. Nothing unusual is configured anywhere along that chain: fail early, uninstall, hooks. Those Jobs delete data on purpose. The etcd one removes the `data-etcd-<ordinal>` PVCs by name, because the etcd-operator does not reclaim them when the EtcdCluster goes. The seaweedfs one removes the volume PVCs by label and explicitly reclaims the keep-protected CNPG Cluster holding the filer metadata, because nothing else would. Both are right for a real uninstall, and neither hook is at fault here. Remediation is what calls them, on a release that is only starting slowly. So an install that misses its budget does not restart. It destroys the datastore the tenant control planes keep their state in, or the object store's data together with the metadata database that indexes it, and the next attempt begins from nothing. That budget is not a comfortable margin. The seaweedfs release has `timeout: 10m`, and `hack/e2e-install-cozystack.bats` already records that on a loaded runner the tenant stack only starts creating pods around 9 to 10 minutes in. The window CI observes and the window that triggers this are the same window. `RetryOnFailure` keeps the manifests applied and retries the failed release as an upgrade. No uninstall happens, so neither cleanup Job runs and the install already under way carries on instead of starting over. Both actions carry the strategy. The controller runs the retry of a failed install as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. One trade is that a real upgrade failure retries instead of rolling back. That costs no stable safety net. With `retries: -1` and the default strategy, upgrade remediation was already rolling back and retrying without end, so the release never rested in the rolled-back state either. The other side of the trade is that the uninstall was also the only automatic clean slate. On a tenant holding no data yet, an install that wedges on a bad resource state now retries against that state until an operator removes the release. For a release that owns data that is the right exchange, because lost data does not come back and a wedged install does. This covers two of the tenant's data-owning releases. The monitoring release carries the same default and a cleanup Job that deletes its PVCs as well, and is left for its own change, as is the rest of the catalog, whose nested releases want the question asked one at a time. Same shape as #3552 and #3581. Relates to #3555 and #3576. ### 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(tenant): retry a failed etcd or seaweedfs install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the etcd data volumes, the SeaweedFS volumes and the filer metadata database ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Improved installation and upgrade recovery for etcd and SeaweedFS by automatically retrying failed operations every 30 seconds. * Preserved ongoing remediation attempts to help services recover without uninstalling or rolling back after failures. * **Tests** * Added coverage to verify retry behavior and remediation settings for both services. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 14:04:17 +03:00
Aleksei Sviridkin	79ceaf1dc2	fix(harbor): retry the nested system install instead of uninstalling it (#3621) ... <!-- 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 Harbor's app chart renders a nested HelmRelease that owns the whole workload: the Deployments, the inline RedisFailover and its volume. It had no install strategy, so it got Flux's default, and install remediation is an uninstall in every case Flux offers. A first install that misses its budget gets torn down, and the next one provisions the redis volume and runs the master election again from scratch. That happens on a loaded runner. harbor-core waits on redis, redis waits on its volume and the master election, and the nginx Deployment hits the default `progressDeadlineSeconds` of 600s. Flux reads that as a stalled resource and fails the install early. The e2e wait budget is 10m, the same as the deadline, so after a teardown the second attempt has less time than a first install needs. `RetryOnFailure` keeps the manifests applied and retries as an upgrade. Nothing is deleted, so the volume, the elected master and harbor-core's startup all survive, and the retry costs 30s instead of a full install. A Deployment clears `ProgressDeadlineExceeded` by itself once its pods go ready, so the retry lands as soon as the stack converges. This drops the teardown, not the deadline. The suite applies the release in one step and waits for it in later ones, so part of the budget is still left when the fail-early hits. That leftover is useless to a fresh install and usable to a retry that resumes. Whether the deadline or the budget also needs moving is a separate question, and an early deadline only becomes safe once a failed install stops being torn down. In production the win holds without any of that: a slow first install no longer deletes the Redis volume and the database. Both actions carry the strategy. The controller runs the retry of a failed install as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. It goes on the nested release, not the parent. The parent is built from the Application by the apiserver, which already stamps this strategy on both actions. The nested one owns the workload and the volume a remediation deletes. Same shape as the tenant CNI and CSI releases in #3552 and #3581, and the unit test follows theirs. Relates to #3445. ### 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(harbor): retry a failed Harbor install instead of uninstalling the release, so a slow first install no longer deletes the Redis volume and starts over ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Harbor installations and upgrades now automatically retry every 30 seconds when failures occur. * Existing unlimited remediation retries remain available for both operations. * **Tests** * Added automated coverage to verify Harbor retry behavior and remediation settings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 14:04:13 +03:00
Aleksei Sviridkin	b11066e770	docs(e2e): name every suite that stayed on the BATS runner ... Two files enumerated the BATS suites and both stopped one short. The global catch in .chainsaw.yaml describes itself as the Chainsaw analog of the EXIT-trap host snapshot in hack/cozytest.sh, and said that runner "only covers the BATS install/openapi tests". The README said cluster bootstrap and the OpenAPI checks remain BATS and named only two files. Both miss e2e-prepare-cluster.bats, which packages/core/testing/Makefile drives through the same runner, and which is the suite whose failures leave the sandbox in the state hardest to reconstruct afterwards. docs/agents/e2e-testing.md already listed all three, so the count was answerable from the tree; it just disagreed with itself depending on which file you opened. The .chainsaw.yaml paragraph is reflowed because the correction is longer than the line it replaces; no other wording changed. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 11:49:15 +03:00
Aleksei Sviridkin	7afa76b5b1	docs(hack): point the comment at the code that does what it describes ... The note explaining that image-cache readiness is gated at point of use pointed into hack/e2e-apps/, which holds two monitoring suites and no runner. The readiness gate it means is the rollout status wait in hack/e2e-chainsaw/_lib/talos-image-cache.sh, along with both fallbacks to the public factory; run-kubernetes.sh only calls it. Naming the caller would have left the reader one hop short of the code. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 11:21:47 +03:00
Aleksei Sviridkin	2147e12d79	docs(hack): correct both budget claims in the capture-gate rationale ... Two numbers in this comment block no longer described the code. The snapshot leg's cost was given as 360s, the deadline passed to timeout. The leg is invoked as `timeout -k 30 360`, so a hung crust-gather is signalled at 360s and killed at 390s, and the trap waits out the later one. The comment 40 lines below already said 390s, as does the op-budget arithmetic in .chainsaw.yaml; the three now agree, and the number names the grace it includes, because the data-plane leg beside it is quoted at its bare deadline. The data-plane leg was still described as having no bound on its first pod list, and billed at the full 600s on that basis. Its reads carry their own ceilings now, 28s for a list and 20s for a single read, so the leg returns in tens of seconds and the paragraph's bottom line overstated the bill it exists to justify. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 11:21:47 +03:00
Aleksei Sviridkin	5728eca401	docs(e2e): name the data-plane collector and who gets the captures ... Section 8 described two of the three on-failure capture legs and not the third, so the collector that reads forwarding state off the node was absent from the one document that tells a contributor what diagnostics exist. It is also the leg whose two callers disagree about its budget, 600s from the BATS runner and 300s from the Chainsaw catch, which is what decides whether something added to it fits. Its notes contract is described with the carve-out the script itself declares: the two EndpointSlice reads in the LoadBalancer path report nothing either way, so a <none> backend line there is the one place a reader still cannot tell an endpointless Service from a read that never answered. Stating the contract without it would have promised the distinction on the path this collector exists to diagnose. The prose also still said the runner's captures serve "the BATS tests". They serve the e2e ones: the runner keys on an e2e- prefix, on the suite's filename or on its parent directory, because all three legs read cluster state and a unit runner has kubectl on PATH with no cluster behind it. Said once where the captures are introduced, and the two phrases that over-claimed now name the e2e suites. The legs are not gated alike, so the sentence says which fired where: two key on kubectl, and the snapshot leg keys on crust-gather being installed, which on a unit runner it usually is not. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 11:21:33 +03:00
Aleksei Sviridkin	d956536461	fix(tenant): render the chart without the platform's injected values (#3636) ... <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `helm lint packages/apps/tenant` fails on main with three errors. This removes two of the three; the branch name notwithstanding, lint still exits 1 afterwards on the third, which is a repository-wide convention question rather than a defect in this chart. **The chart cannot be rendered outside a cluster at all.** `templates/keycloakgroups.yaml` reads `index .Values._cluster "oidc-enabled"`, and `_cluster` is injected by the platform, so it is absent whenever the chart is rendered on its own. `index` on a nil map is a hard error, not an empty result, so both `helm lint` and a plain `helm template` die on line 1. The idiom the neighbouring charts use for the same lookup does not rescue this on its own, which is worth stating because it looks exactly like a guard. `(index .Values._cluster "x") | default "y"` still fails: `index` is evaluated first and errors on the nil map, so `default` never receives 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. A chart is broken only when it neither declares the empty map nor guards at the use site, and a lint sweep across the catalog found 27 in that state. Guarding at the use site is the choice here because it makes the template correct whatever `values.yaml` contains, and because `_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: ``` {{- $oidcEnabled := index (.Values._cluster | default dict) "oidc-enabled" }} ``` **This is behaviour-preserving, and that is checked rather than asserted.** 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. Rendering before and after gives byte-identical output: | values | before | after | | --- | --- | --- | | `_cluster.oidc-enabled: "true"` | 4 KeycloakRealmGroups | byte-identical | | `_cluster.oidc-enabled: "false"` | no groups | byte-identical | | no values at all | hard error | renders, no groups | Worth recording how that comparison has to be run, because the obvious way proves nothing. The KeycloakRealmGroup block sits behind `.Capabilities.APIVersions.Has "v1.edp.epam.com/v1"`, which is false under a plain `helm template`, so a first pass rendered the same output in *both* the enabled and disabled cases, and a diff of those two would have compared one empty result against another. The renders above pass `--api-versions v1.edp.epam.com/v1` so the guarded branch actually executes. Anyone proving neutrality on a capability-gated template needs that flag or the proof is vacuous. **`Chart.yaml` does not declare `cozy-lib`**, which is present as a symlink under `charts/`, so lint reports missing chart metadata. It is declared the way `packages/apps/qdrant` does it, that being the one chart in the tree which declares it and does not carry this error. Rendering is unchanged by the declaration, checked the same way as above. Two things that could plausibly have broken and did not, checked rather than assumed: rendering from a packaged `helm package` artifact is byte-identical, and the `charts/cozy-lib` symlink is still expanded into the tgz with the dependency declared. And neither `hack/update-crd.sh` nor the `fix-charts` target in `packages/apps/Makefile` reads the `dependencies:` block, so nothing needs regenerating. **The icon error is left in place on purpose, and the relative path is load-bearing.** `helm lint` wants a URL with a scheme, and `/logos/tenant.svg` has none. But `hack/update-crd.sh` consumes exactly this form: it rewrites a leading slash to `./`, then reads that file off disk and base64-encodes it into `.spec.dashboard.icon` of the generated ApplicationDefinition. `packages/apps/tenant/logos/tenant.svg` is that file. Give the chart a URL with a scheme and the generator's `[[ ! -f "$ICON_PATH" ]]` check no longer finds anything to encode. So the relative form is a requirement of the pipeline rather than a stylistic convention, all 31 icon-carrying charts use it, and satisfying helm lint here means changing the generator, not the charts. That is a repository-wide decision and not this PR's. `tests/keycloakgroups_oidc_test.yaml` pins the gate in all three directions: four realm groups when `oidc-enabled` is `"true"`, none when it is `"false"`, and none when the platform injected no `_cluster` at all. The last case is the one the guard exists for, and it needs no mocking, since `_cluster` is absent from `values.yaml` and "not injected" is just the chart's default state. Against the unguarded template that suite fails with `index of untyped nil`. The chart's other seven suites pass identically with and without the fix, which is precisely why the change needed its own. Two limits of the new suite, stated rather than left to be found. It pins how many documents render, not which: renaming the four groups or breaking a `realmRef` would pass. And the template's second gate, `.Capabilities.APIVersions.Has "v1.edp.epam.com/v1"`, is enabled for the whole suite, so no case covers its absence. Both are worth closing on the next touch of this file; neither weakens what the suite does pin, which is that the chart renders at all when the platform injected nothing. A trap for anyone reproducing this by hand: the gate compares against the string `"true"`, so `--set _cluster.oidc-enabled=true` passes a YAML boolean and renders nothing, while `--set-string` renders the four groups. The platform writes a genuine string, and it survives the round-trip through a Secret because `toYaml` quotes it. Note also that this chart's helpers reject a release name that does not begin with `tenant-`, which will silently render nothing for either form. One note in case a future change teaches the linter to inject the platform's values, which would hide this class of error without fixing it: the guard is still worth having, because it is what lets `helm template packages/apps/tenant` succeed with no values file at all. A chart that renders standalone can be inspected, diffed and unit-tested without reproducing the platform around it. ### 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(tenant): the tenant chart now renders without the values the platform injects, so `helm template` works on it standalone, and its `cozy-lib` dependency is declared in Chart.yaml; this clears two of the three `helm lint` errors it reported, the remaining one being the repository-wide relative icon URL ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for using the shared Cozy library in tenant deployments. - **Bug Fixes** - Improved tenant configuration rendering when cluster settings are unavailable. - OIDC-related Keycloak groups now render reliably when OIDC is enabled and required platform capabilities are present. - Preserved expected behavior when OIDC is disabled or configuration is omitted. - **Tests** - Added coverage for enabled, disabled, and missing cluster configuration scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 10:55:04 +03:00
Aleksei Sviridkin	905249fb39	fix(tests): bound the e2e data-plane capture's reads and stop inferring absence from silence (#3659) ... <!-- 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/e2e-capture-dataplane.sh` runs after an e2e failure to snapshot the host-to-pod data plane. Every `kubectl` read in it was unbounded, so a read against a wedged apiserver hung until the caller's own backstop killed the script, and then no capture file was written at all. The run that most needs the diagnostic is the run least likely to produce one. Each read now carries its own bound, 20s for a single read and 28s for a list, well inside the 300s the callers allow. Bounding the reads creates the second problem, which is most of this diff. A bounded read that gives up returns empty, and empty was already how the script recognised "there is nothing here". Five places turned a read that never answered into a fact about the cluster: no affected pods, no ovn-central, no LoadBalancer Service, no cilium-agent on a node, and a LoadBalancer recorded as reachable. Two of the five only became reachable because of the bounds. Before them the script died instead of writing the file, so a lookup that never answered could not be recorded as a pod that is not there. Each of those claims now has a counterpart chosen by the status of the read that produced it, and the counterpart says what is left unknown rather than what the read did. The last one is the subtlest, because nothing about it looks like a claim of absence: the probe resolves a cni-server pod before probing, and a lookup that never answered produced no probe outcome at all, which the decision helper read as "nothing failed" and the artifact stamped as reachable. That is a verdict about an address the script never touched. The probe now emits an explicit unknown, and a set of outcomes that is entirely unknown stays unknown. Only a wholly unknown set: one unrun probe beside real failures leaves the failures standing and still captures, because they are the evidence the capture exists to characterise. Every note ships beside the capture it explains, not only in the job log, because the reader who has the uploaded report and not the run is exactly the reader who cannot otherwise tell a capture that found nothing from one that never ran. Both sibling collectors already hold that contract. The middle commit is a separate and measurable thing. Each per-node pod lookup was issued up to three times per node; it is asked once now and memoised, with only answers and cutoffs cached, since an instant failure costs nothing to re-ask and caching it would make one transient permanent for the whole run. On an 8-pod, 8-node stub that is 264 kubectl calls down to 229 and 59 per-node lookups down to 24, with byte-identical output. Deliberately left, and worth naming so nobody reads this as the whole class. The two EndpointSlice reads still discard stderr and report neither outcome. The "reachable, skipped" wording still covers every path that reaches it having run zero probes, among them an unresolved probe node, a `_lbport` of 0, a cni-server image carrying none of `nc`, `curl` or `wget`, and a failure of the probe `exec` itself rather than of the lookup before it; that leftover is tracked in #3658. And the three call sites still disagree about what a partial answer means: `host_http_probe` discards a name that arrived with a non-zero status, while `capture_node` and `capture_lb_node` use a non-empty name regardless of it. All of these are verbatim at the merge base and this diff does not change their behaviour. Covered by 39 cases in `hack/capture-dataplane.bats`, each branch pinned by a stub that answers one read and fails another, and each with a positive anchor so a stub that stops short of the branch fails rather than passing quietly. relates to #3642 ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(tests): bound every kubectl read in the e2e data-plane capture so a hung read no longer costs the whole capture, and stop the collector reporting an unanswered read as an absent pod, service or reachable address ```	2026-08-08 10:55:00 +03:00
Aleksei Sviridkin	c1071dab9b	feat(kubernetes): let a node group log the guest serial console (#3637) ... ## What this PR does Both tenant Kubernetes e2e suites keep missing the node-Ready budget with zero Nodes registered, and the time goes inside the guest, between the worker VMI reaching Running and Talos asking for its apid certificate. Nothing collected today reaches that window. The in-guest capture the harness already has drives `talosctl`, so it needs the apid the worker never got to, and the management side has neither a Node nor a certificate request to read. Four attempts at this failure have been designed from outside the guest; the most recent raised the budget from 12m to 18m and the same fingerprint came back at the new deadline. KubeVirt can stream a VM's guest serial console into a `guest-console-log` container beside virt-launcher, readable with `kubectl logs` and needing nothing from inside the guest — no talosctl, no client certificate, no helper Pod, no reachable apid. That is the one property this failure class demands, so this turns it on for the suite's worker node group and collects it into the diagnostic bundle when node-join fails, ahead of the `talosctl` capture that cannot describe the same runs. Reads start at the beginning of the stream rather than tailing it: a guest that repaints its console would push the boot output out of any `--tail` window, while `--limit-bytes` truncates from the far end instead. Every read is bounded, and the walk stops at a cap that records both counts, so the collector cannot become an unbounded term in a failure path that already runs several others under one deadline. A read that returns no console is not filed as a quiet guest. Three outcomes get three labels, because only one of them says anything about the guest: the read failed and returned nothing, the read succeeded and returned nothing, or it broke off part way. Each read's stderr is kept beside the capture rather than inside it — kubectl's own `container guest-console-log is not valid for pod` would otherwise fill the file, and a console that never started would read as one that had spoken. That case is the headline one here, so the Pod state and events that explain it are collected alongside, once for the whole selector. The switch is a new opt-in `logSerialConsole` on a node group, off by default. It renders nothing unless set to true, including when set to false, because the `KubevirtMachineTemplate` is content-hash named and a byte emitted for a group that did not ask for it renames the template and rolls every live worker VM on upgrade. I verified that by rendering the base tree and this branch: absent and explicit-false produce a byte-identical template name on both charts. Worker VMs come from two charts, `kubernetes` and `kubernetes-nodes`; both carry the field, and the golden-parity check between them gains a case that keeps their output identical. The passing path checks separately that the container actually attached, and runs that check last, after everything the suite exists to prove — a debugging aid should not preempt the assertions that cover the cluster. Without it the suite would first learn the setting was inert in the one run that needed the console, when it can no longer be recovered. Relates to #3513. ### The risk this carries, stated up front The platform turns serial console logging off cluster-wide in the KubeVirt CR. That setting is a mitigation added in February 2026 for kubevirt/kubevirt#15989, where the `guest-console-log` container kept virt-launcher from starting and left VMs in `PodInitializing`. A per-VM `logSerialConsole` takes precedence over it — `isSerialConsoleLogEnabled` reads the VMI field first and only then falls back to the cluster setting — which is what makes this work, and also makes this the first thing on the platform to go through that mitigation. The mitigation was written against KubeVirt v1.6.x; the platform now ships v1.8.4, and the upstream issue is still open with no reproduction reported either way on the newer version. So the first run of these suites is also the experiment, and it doubles as a revalidation of that February mitigation on the version actually shipping, which nobody has done. The two outcomes are distinguishable rather than ambiguous: kubevirt/kubevirt#15989 shows up as virt-launcher wedged in `PodInitializing` with the VMI never reaching Running, while the failure this instruments shows the VMI Running and zero certificate requests. A red run with the first signature is an answer, not noise — and the collector is built so that it says which one happened instead of reporting silence. The blast radius is small: the product field is absent by default and only the e2e node group sets it, so backing this out is one line. The revert criterion, stated before the run rather than after it. If worker virt-launcher Pods sit in `Init` on `guest-console-log` and no Node registers, kubevirt/kubevirt#15989 still reproduces on v1.8.4: drop the `logSerialConsole: true` line from the e2e node group, keep the chart field, and the upstream issue gains the reproduction on a current version that its thread does not have. If the Pods reach Running and the console logs carry boot output, the mitigation is a candidate for removal platform-wide, which is its own change. Either way the run answers something that was open before it. Dropping `virtualMachineOptions.disableSerialConsoleLog` outright was the alternative. It is a smaller diff and needs no values key, but it exposes every VM on the platform to a race nobody has re-tested, instead of one node group in one test. That is its own revalidation exercise and belongs in its own change, on evidence this one produces. ### Verification `make unit-tests` passes. The new bats suite runs green under both `bash` and a `dash` `/bin/sh`, which matters because `hack/cozytest.sh` is `#!/bin/sh` and an earlier revision of this branch carried a bash-only construct that failed silently there. Every test in it is killed by a specific mutation of the code it covers. Both charts' helm-unittest suites pass, the golden-parity script reports byte-identical output across all four cases including the new one, and `make generate` is idempotent at the repo root. Reviewed by Claude across several rounds and by an independent second reader; the review loop found and fixed real defects in this branch, including a green-path check that had become unable to report the one thing it existed for. ### Screenshots Not a UI change. ### Downstream repositories - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: https://github.com/cozystack/terraform-provider-cozystack/issues/26 - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note feat(kubernetes): worker node groups accept `logSerialConsole`, which streams a worker VM's guest serial console into a `guest-console-log` container readable with `kubectl logs`. Off by default. It is the only way to observe a worker that stalls during boot before Talos apid answers, when no Node and no certificate request exist to diagnose from. Enabling it rolls the node group and overrides the platform's cluster-wide `disableSerialConsoleLog`. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an optional `logSerialConsole` setting for Kubernetes worker node groups. * Streams worker VM guest serial-console output to a dedicated log container when enabled. * Configuration is disabled by default and available through Kubernetes application settings. * **Bug Fixes** * Improved worker failure diagnostics with bounded serial-console output and pod-state information. * Validates that enabled worker pools attach the expected console-log container. * **Documentation** * Documented configuration behavior, rollout effects, defaults, and timeout considerations. * **Tests** * Added coverage for rendering, attachment validation, capture limits, and failure scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 10:47:00 +03:00
Aleksei Sviridkin	93ffb38a9b	fix(cozy-lib): make the cozyconfig helpers survive uninjected values (#3647) ... <!-- 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 `_cluster` and `_namespace` reach a chart through valuesFrom on the cozystack-values Secret, so they are absent whenever a chart is rendered on its own: `helm lint`, a plain `helm template`, and any helm-unittest suite that does not set them. Twelve of the helpers in `_cozyconfig.tpl` read straight through the missing map and abort the render instead of returning the empty default they document. Two spellings, one cause. `index .Values._cluster "root-host"` raises `index of untyped nil`; `.Values._namespace.host` raises `nil pointer evaluating interface {}.host`. In both, the trailing `| default ""` never runs, because it applies to the result and there is no result, only an error. That default covered a key missing from a map that exists, never a missing map. The guard goes on the map, which is what `cozy-lib.images-registry` in this same file already did: ``` (index (.Values._cluster | default dict) "root-host") | default "" ``` `index` on an empty map returns nil harmlessly, so the trailing default applies as it always read, and with the map present `| default dict` returns it untouched. **This fixes no chart that is failing today.** The charts that cannot render standalone read the injected values directly in their own templates rather than through these helpers, so a change to the library does not reach them; they are handled where they live. Worth being blunt about the same point from the other side: twelve of the thirteen helpers here currently have no callers anywhere in `packages/`. Only `cozy-lib.images-registry` is reached, from `cozy-lib.image`, and it was already guarded. The guard is still worth having on the rest, because they are the library's public surface, they are what a chart author copies when reaching for cluster configuration, and a broken spelling propagates by being read. `cozy-lib.image` is called from four templates, so these helpers do get adopted once they exist. **Rendered output does not change, and that is measured rather than argued.** All 34 charts that symlink cozy-lib were rendered before and after, both with the injected values populated and with them set to empty maps, and every result is byte-identical. That check needs a control: the first comparison showed eight charts differing, which looks like a regression in a library change. Rendering the *unchanged* tree twice reproduces exactly those eight, because they generate passwords. Subtracting the control leaves zero real differences. The suite lives in `cozy-lib-tests`, which is where library behaviour is pinned, since a library chart renders nothing of its own. One case covers the uninjected state, which needs no mocking because neither key appears in any values file, and one covers the injected state so the guard cannot quietly change what a populated map returns. The injected case pins `scheduling` and `branding` as whole YAML documents rather than by substring: those two had their bodies restructured rather than wrapped, and a substring assertion survives the loss of the `toYaml` that produces the document, because the key name also appears in Go's default map formatting. Against the unguarded helpers the first case fails with `index of untyped nil`. Three things left deliberately undone, so they are not mistaken for oversights. The header comments in `_cozyconfig.tpl` say where these values come from but not what the helpers now do when they are absent, which is the contract this change creates and would be better stated next to the helpers than only in the suite. `Chart.yaml` in cozy-lib-tests still carries `name: quota` from when that chart covered one helper family, so the runner prints `### Chart [ quota ]` for a suite that now covers three. And the mixed state, one map injected and the other not, has no case of its own; the two guards are independent so composition covers it, and a third case would pin nothing new. Relates to #3646. ### 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(cozy-lib): the cozyconfig helpers now return their documented empty defaults when the platform has not injected `_cluster` / `_namespace`, instead of aborting the render, so a chart using them can be rendered and unit-tested on its own ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved configuration handling when cluster or namespace settings are missing, preventing template rendering errors. - Preserved existing configuration values for scheduling, branding, networking, registries, and component namespaces. - **Tests** - Added coverage for configurations with missing platform values. - Added validation that injected cluster and namespace settings render correctly. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 10:46:56 +03:00
Aleksei Sviridkin	db590b493c	fix(hack): arm cozytest cluster captures for e2e suites only (#3645) ... <!-- 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/cozytest.sh` arms three cluster captures from its on-failure EXIT trap, and gates each on a binary being installed rather than on there being a cluster to read. A unit runner has kubectl and no cluster, so a red unit test walked into those legs on its way out and paid for snapshots of nothing. What that costs turns on how kubectl fails, which is why the defect survived: against an endpoint that refuses it is a second, and locally that is what happens. Against one that hangs, which is what a runner with a context pointing at a torn-down cluster has, I measured ~28s for the previous-logs leg and the full 600s for the data-plane one. The 28s is the leg self-bounding its own pod list, so it never reaches the 300s ceiling above it; the 600s is real, because that leg's first pod list carries no bound of its own. Ten minutes of dead waiting for a failure that should have cost seconds. Where crust-gather is installed too, the bill stops being empty and starts being wrong. That leg needs no reachable cluster to fire, only an ambient KUBECONFIG, and it will spend up to 360s snapshotting whatever cluster the current context happens to name. The gate now keys on the `e2e-` prefix, read from the suite's own name or from the directory holding it. At the top level that is the split the project already sorts on: the Makefile builds `BATS_UNIT_FILES` as `$(filter-out hack/e2e-%.bats,$(wildcard hack/*.bats))`, so a top-level `e2e-*` suite is exactly the one `make unit-tests` refuses to run and `packages/core/testing/Makefile` runs against a live cluster. That wildcard is not recursive, and `hack/e2e-apps/` holds live-cluster suites whose own filenames carry no prefix, so the directory is checked too. Matching the basename alone would take the captures away from suites that need them, which is the opposite of the point. Every leg, bound and message on the e2e path is unchanged. It is deliberately not a reachability probe, though a probe would also tell a cluster from a bare binary. A failing e2e run is frequently one whose apiserver is degraded, and a probe answering "no" there would skip the snapshot in precisely the case the snapshot exists for. A diagnostic should not condition itself on the health of the thing it diagnoses. The test drives the runner as a subprocess over fixtures that differ only in where they sit, with stub kubectl and crust-gather on PATH so neither direction is decided by a binary being absent. Each leg is pinned by the artefact it leaves in the report tree rather than by the one leg that echoes, so no single leg can be ungated without a test going red; that was checked by ungating each in turn. The two armed cases are the positive control for the disarmed one, whose assertion is negative and would otherwise pass just as well against a harness that ran nothing. Relates to #3626. ### 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(hack): cozytest.sh no longer arms its cluster-capture hooks for unit BATS suites, so a failing unit test fails fast instead of waiting out capture timeouts against a cluster that is not there ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Cluster diagnostics are now collected only for end-to-end test suites. * Unit tests no longer trigger unnecessary Kubernetes or cluster-capture operations. * End-to-end suites nested under application directories are correctly recognized. * Diagnostic capture remains subject to existing tool availability and timeout checks. * **Tests** * Added coverage verifying diagnostic artifacts are collected for end-to-end failures and omitted for unit-test failures. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 10:46:53 +03:00
Aleksei Sviridkin	7b36a5d2f2	test(tenant): give these template lists their real reason (#3651) ... ## What this PR does Two helm-unittest suites in `packages/apps/tenant/tests/` said they restrict rendering because `namespace.yaml`'s `lookup` errors under helm-unittest. It does not. The lookup returns empty offline and the template renders, which `namespace.yaml`'s own header states and which `tests/namespace_ancestor_labels_test.yaml` depends on. I measured what each list actually does by removing it, and the two suites turned out to be different cases rather than two copies of one. In `cleanup_role_watch_test.yaml` the list is load-bearing, for a template the comment never mentioned. That suite sets no values, and `keycloakgroups.yaml` indexes `.Values._cluster` on its first line, so the whole-chart render fails with `index of untyped nil` and all 8 tests error. The comment now says that, and says it conditionally: a guard on that index is already in flight, and once it lands the list stops being load-bearing. I checked that too, by applying the guard and re-running with the list removed, which passes 8/8. In `vmo_webhook_dependson_test.yaml` the list is not load-bearing at all. That suite sets `_cluster`, the chart renders without the list, and all 5 tests pass. It stays only to scope the suite, and the comment says so instead of inventing a technical reason. Each comment now states what holds for its own file and nothing wider. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches two files, both under `packages/apps/tenant/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified test-rendering rationale and suite-scoping comments. * Documented that rendering requires `_cluster` values and that template lists control test scope. * **Tests** * Updated explanatory comments only; test behavior and configuration remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 10:46:41 +03:00
Aleksei Sviridkin	4740daa7be	test(tenant, harbor, seaweedfs, api): anchor guards that pass on a missing document (#3639) ... ## What this PR does Five helm-unittest suites asserted things that stayed green when the thing they guard disappeared. No chart or template logic changes here — only the tests, and each fix is measured in both directions. **tenant `no_upgrade_force_test.yaml`.** Every test asserted `notExists: spec.upgrade.force` and nothing else. A `notExists` passes on a template that renders no document at all, so renaming the value that gates one of these releases leaves its test green while the release it guards is gone. Measured on `etcd.yaml`: with the gate renamed, the suite reported 6 passed and a sibling suite's `equal` was the only thing in the chart that noticed. Each test now pins the HelmRelease name alongside the absence. The same file also covers `computeplane.yaml` now. Seven templates in this chart render a HelmRelease and the list held six; the module landed after the suite was written. The invariant holds there today, so this closes an unguarded template rather than a live regression. That is the gap that produced this suite in the first place, when the first sweep missed `gateway.yaml`. Two rationales in that header were wrong and are corrected. The template list was said to be there because `namespace.yaml`'s `lookup` errors under helm-unittest; it does not, it returns empty offline and the template renders, which is what `namespace.yaml`'s own header says and what `namespace_ancestor_labels_test.yaml` relies on. Removing the whole list leaves the chart's suites passing, so the list stays for the reason that actually applies. And helm-controller does not deprecate `upgrade.force`: the CRD vendored at `internal/fluxinstall/manifests/fluxcd.yaml`, which is what the pinned Flux 2.8.x ships, describes it as a replacement strategy with no deprecation marker. `recreate` is the neighbouring field that carries one. **harbor, both suites.** Both addressed their subject by document index. An index is a position, so a template that gains a document ahead of the one under test moves every later assertion onto a different object. Measured by inserting one document at the head of each template: the cleanup suite reddens outright, while two of the four jobservice tests keep passing: the two `notExists` ones, satisfied against an object that never had the path. Every document these suites assert on is unique by kind within its template, so each test now names its subject. A selector that matches nothing fails loudly, including on the `notExists` tests. Indices are also easy to get wrong by hand: helm-unittest numbers documents in template source order while `helm template` prints them sorted by kind, so the rendered output is not the order being counted. **seaweedfs and cozystack-api.** Both forbade a rule by naming it in full, and helm-unittest compares every field it is handed by value equality, so one respelling walks past. For seaweedfs that is not theoretical: ungating the cnpg block and adding a verb to the rule is the same grant and more, and the guard stayed green while a Client instance held delete on another app's Postgres cluster. The Role is now bounded by shape. A Client renders exactly two rules, both in the core group, and the cnpg grant is gated between them, so ungating it adds a third whatever its spelling. For cozystack-api the count bound sits beside the existing named-rule assert rather than replacing it. Neither covers the other: `templates/rbac.yaml` carries no template directives, so the list is pinned at exactly sixteen rules from both sides, while a grant swapped in for an existing one leaves the count where it was and only the named-rule assert sees it. Measured on all three: append, delete, and swap. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches five files, all under `packages/*/*/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved validation of rendered Kubernetes resources across Harbor, tenant, SeaweedFS, and system API configurations. - Tests now identify resources by kind and relevant properties, making checks more reliable when rendered document ordering changes. - Expanded coverage for tenant upgrade settings, compute plane resources, storage class propagation, cleanup permissions, and network policy access. - Added stricter assertions for expected resource counts, names, rules, and omitted settings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 10:46:27 +03:00
Aleksei Sviridkin	f92306681d	fix(e2e): an unanswered read is not the same as an absent thing ... Five places turned an empty or missing result into a fact about the cluster: no affected pods, no ovn-central, no LoadBalancer service, no cilium-agent or ovs on a node, and a LoadBalancer recorded as reachable. Each is also what the script produces when the read behind it never answered, and with a note now saying the read failed, lines sat next to each other and disagreed. Each claim has a counterpart chosen by the status of the read that produced it, and each says what is left unknown rather than what the read did: no scheduled NotReady pods / whether any pod is affected no ovn-central pod in <ns> / whether <ns> runs ovn-central no Service type=LoadBalancer / whether any LoadBalancer needs capturing (no cilium-agent pod found) / could not determine whether one runs there LB reachable, skipped / whether the LB is reachable is unknown The last of those is the subtlest, because nothing about it looks like a claim of absence. host_http_probe resolves a cni-server before probing, and a lookup that never answered produced no probe outcome at all, which the decision helper read as "nothing failed" and the artifact stamped as reachable -- a verdict about an address the script never touched. The probe now emits an explicit unknown, and a set of outcomes that is entirely unknown stays unknown rather than collapsing into either answer. Only a wholly unknown set: one unrun probe beside real failures leaves the failures standing and still captures, because they are the evidence the capture exists to characterise. Wording that describes the read instead would be wrong in a case these bounds create. A list can fail after emitting rows, since a bound firing mid-stream leaves output on stdout and 124 in $?, so an empty result with a non-zero status can mean the read said nothing or that it said only uninteresting things. Before the bounds a read either answered or hung forever, and a partial answer was not a state this script could reach. For the same reason a failure note is unconditional on the status while the consequence -- skipped, unresolved -- is stated only by the branch that takes it, which is why the note inside pod_on_node names none at all: five call sites share it and their consequences differ. Two of these five are in the artifact rather than the log, and they are there because of the bounds. Unbounded, the per-node lookup blocked until the caller's backstop killed the script and no capture file was written; bounded, the script survives and writes the file, so a read that never answered would have been recorded as a pod that is not there or an address that answered. pod_on_node returns its status with the name and the memo stores it, since a hit that dropped it would launder a cutoff into a confident absence. The status travels packed with the value through stdout because every caller reads the helper through a command substitution, where a variable dies with the subshell -- the same property that makes the memo itself a file. Only answers and cutoffs are memoised: an instant failure costs nothing to ask again, and caching it would make one transient permanent for the whole run. Every note also ships beside the capture it explains, not only in the job log, because the reader who has the uploaded report and not the run is exactly the reader who cannot otherwise tell a capture that found nothing from one that never ran. Both sibling collectors hold that contract and this script's notes are modelled on them. The two phrasing helpers move above the sourcing guard so the unit suite can assert their branches directly. The stderr sink stays below it, because anything created above is created again by every test that sources this file and by every host that exits early for want of kubectl. What is deliberately left is the same distinction for the two EndpointSlice reads, which still discard stderr and report neither outcome. Each branch is pinned by a stub that answers one read and fails another, each partial-answer case by a stub that emits a row and then fails, the artifact cases in the artifact rather than the log, and the memo's status round-trip by consuming one lookup from both a miss and a hit. Every one of them carries a positive anchor as well, so a stub that stops short of the branch fails rather than passing quietly. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 10:07:21 +03:00
Aleksei Sviridkin	b921952fb0	perf(e2e): ask each per-node pod lookup once ... The same (namespace, label, node) is asked up to three times per affected pod across the sections of this script. That was free while the first call hung forever and the script died inside it; with a per-read bound each repeat costs its own timeout, and against a wedged apiserver the repetition becomes the largest consumer of the caller's envelope -- enough that a 300s budget buys two pods where it could buy five. Answers are memoised, empty ones included: "there is no cilium-agent on this node" is exactly the lookup worth not repeating. The memo is a file rather than a variable, which is not a style choice. The walk over affected pods runs on the right-hand side of a pipeline, so it executes in a subshell and a variable memo is discarded at the end of it, leaving every lookup to be paid for again in the sections that follow. With the variable form the suite still recorded each of the three lookups twice. That is also why this is not the shape of the node memo above it, which is a space-delimited string matched with a case glob and is correct for a dedup living entirely inside the walk. Keys are compared as whole tab-separated fields, so the '=' inside a label selector is ordinary payload; what the scheme rests on is that no namespace, label selector or node name contains a tab, which Kubernetes names and selectors cannot. An empty pod list is also separated from one that never answered, so the branch below it stops reporting a failed read as a cluster with nothing wrong. Pinned by a stub that records every per-node lookup it is given: the test fails if any is issued twice. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 09:11:51 +03:00
Aleksei Sviridkin	7d1137a889	fix(e2e): bound the dataplane capture's reads and name why one failed ... Six `kubectl get` calls ran with no bound of their own, while all 27 `kubectl exec` calls in the same script went through one. Against an apiserver that hangs rather than refuses, the first of them -- the cluster-wide pod list -- held the script until the caller's backstop killed it, so the capture wrote nothing at all. From the Chainsaw catch that is worse than empty: the run is cut off by the outer `timeout -k` and the script says nothing about having been cut short, so a truncated capture reads exactly like a complete one that found no affected pods. Measured against a kubectl stub that never returns: before, the script was still blocked on its first call when a 60s backstop killed it; after, it reaches its own end and says which read was cut off and what that leaves missing. Roughly 4s at a 1s bound, 40s at the 20s default. A per-read bound buys forward progress, not a smaller total. The sum of the bounds here is far past any caller's envelope and deliberately so, because the MAX_PODS and MAX_LBS caps bound work while the outer `timeout -k` bounds wall clock. What changes is that no single hung read can consume that envelope before anything is written. The reporting around them is built to survive being read by someone who was not there. Only 124 and 137 may name a timeout, since those are the only statuses a bound produces and every other one is kubectl answering -- refused, denied, a kind the cluster does not serve -- so the reads keep their stderr instead of discarding it and quote it back. 137 is described as what it is, a SIGKILL that cannot be told apart from the bound's own kill grace. Notes print through `printf` rather than `echo`, because under /bin/sh echo expands backslash escapes and kubectl's stderr now flows through the logger: a message holding a literal \n would otherwise split the note and forge a second "[capture-dataplane]" line that reads as this script's own verdict, which needs no malice given the jsonpath in these reads contains {"\n"} and kubectl quotes the expression back when it fails to parse it. The per-node lookups ask with `{.items[*].metadata.name}` rather than `{.items[0]...}`. client-go's evalArray has no allowMissingKeys escape the way evalField does, so indexing into an empty list is a hard error and kubectl exits 1 -- which meant nothing while the status was discarded, and would report "there is no cilium-agent on this node", the ordinary answer, as a read that failed. The tests drive the script as a subprocess against a stub kubectl that hangs, fails instantly, or returns a forged line, and read what it wrote. The stub that proves forward progress answers the pod list and hangs afterwards, so the walk over affected pods is actually entered. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 09:11:51 +03:00
myasnikovdaniil	68e1e41124	fix(ci): escalate unclassified paths in TIA instead of skipping e2e (#3511) ... ## What this PR does Closes #3392. Stacked on top of #3262 — that PR turns the `E2E Tests` merge gate into a commit status, which is what makes this fail-open path matter: a skipped required *job* used to pass branch protection either way, but a status posted `success` with no suite run is an explicit green verdict on untested code. `hack/select-e2e.sh` silently selected nothing for a changed path it did not recognise outside `packages/`. Both lanes read an empty selection as "skip Chainsaw" and then report green. The worked example from the issue is `hack/common-envs.mk`, which supplies the tag, push and output flags of every image in the tree and matched nothing: `full_suite_pattern` covered `hack/*.sh` and `hack/*.bats` but not `hack/*.mk`. Walking the tree turned up five more in the same position — `pkg/`, `tools/`, `go.mod`/`go.sum`, `hack/lib/` and `hack/buildkitd.toml` — plus `.github/workflows/e2e-fork.yaml`, which #3262 adds while `pull-requests.yaml` was already escalated. As the issue argues, a blanket "empty selection means full suite" fallback would be the wrong fix, because it escalates exactly the inert paths the design deliberately skips. Instead every changed path now lands in one of three classes by an explicit rule: - `full_suite_pattern` — shared build inputs and the e2e harness: `packages/library/`, `packages/core/`, the Go trees (`api/`, `cmd/`, `internal/`, `pkg/`), the codegen under `tools/`, `go.mod`/`go.sum`, `hack/*.sh|*.bats|*.mk`, `hack/lib/`, `hack/buildkitd.toml`, `hack/e2e-*.yaml`, the `Makefile`, and the workflows that run the suite. - `inert_config_pattern` — paths that cannot affect what e2e exercises: `examples/`, `.github/` (bar the e2e workflows), `.claude/`, `.gemini/`, `img/`, `hack/testdata/`, the codegen boilerplate header, and the top-level meta files (`LICENSE`, `.gitignore`, `.pre-commit-config.yaml`, `.coderabbit.yaml`). - the PackageSource graph for `packages/(apps|system|extra)/`, unchanged. A path matching none of them escalates to the full suite and logs which path forced it, so the next path someone adds fails safe rather than fails open. The two lists are checked in that order, so a specific escalation beats a broad inert directory — `.github/` is inert, `.github/workflows/e2e-fork.yaml` is not. `*.md`, `docs/` and `dashboards/` stay ahead of both, so a `README.md` under `packages/core/` does not inherit that tree's escalation. The second commit removes the `|| [ -z "$SELECTED_APPS" ]` arm from the Chainsaw step in both lanes. Under a step gated on `skip != 'true'` it could never decide anything the `FULL_E2E` test had not, while the comment above it advertised "full suite when the selection is empty" in the file whose whole point is that a green status means tests ran. ### Deliberate choices worth a second opinion - **Which workflows escalate.** The four that actually invoke the suite (`pull-requests`, `e2e-fork`, `e2e-tag`, `nightly`), enumerated rather than matched by prefix so an unrelated workflow does not burn a full run. That does mean an `e2e-tag.yaml` or `nightly.yaml` edit now costs a full PR suite even though the lane it changes is not the PR lane. Easy to narrow to the two PR lanes if that is the wrong trade. - **`go.mod`/`go.sum` escalate.** A dependency bump changes the shipped binaries, so this is the honest classification, but it does make Renovate's Go PRs pay for a full suite. ### Verification - `hack/select-e2e_test.bats`: 25 tests green (15 existing, 10 new), covering both sides of the classification. - Mutation-checked rather than asserted: reverting the fall-through fails the unclassified-path test; dropping `.github/` from the inert list fails the non-e2e-workflow test; swapping the order of the two list checks fails the existing `pull-requests.yaml` test. - The per-entry escalations are belt-and-braces by design: removing `hack/*.mk` from the pattern still escalates via the fall-through, so that test pins the outcome rather than the pattern entry. Called out so nobody reads it as stronger coverage than it is. - `make bats-unit-tests`: 417 tests, 0 failures. `actionlint` clean on both workflows. - Self-check: this branch's own diff selects the full suite (it touches both e2e workflows and a `hack/*.sh`). ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff file by file. The only `hack/`-related trigger is `cozystack/ccp`, which gates on `hack/package.mk` and `hack/common-envs.mk` existing as anchor files and on what `make generate` does — nothing here is moved or renamed, neither anchor file is touched, and no make target changes behaviour (`BATS_UNIT_FILES` already globbed `hack/*.bats`). The rest of the diff is e2e workflows and agent docs, which appear nowhere in the map. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(ci): Test-Impact Analysis now classifies every changed path explicitly and escalates an unrecognised one to the full E2E suite. Previously a path the selector did not recognise (`hack/*.mk`, `pkg/`, `go.mod`, `hack/lib/`, the fork E2E workflow) selected no suites, which both E2E lanes read as "nothing to test" and reported the required `E2E Tests` gate green with no suite run. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved end-to-end test selection so unrecognized or high-impact changes trigger the full suite instead of being skipped. * Inert-only changes now safely skip testing, while empty selections no longer trigger the full suite. * Full-suite testing is triggered explicitly by the `full-e2e` label or applicable change types. * **Tests** * Added coverage for build files, source code, workflows, scripts, documentation, metadata, and unclassified paths. * **Documentation** * Clarified end-to-end test selection, escalation rules, skipped file categories, and workflow handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-08 06:22:45 +03:00
Aleksei Sviridkin	8e273a7341	docs(ci): record what escalating on the non-PR workflows costs ... Three of the four workflows on the escalation list cannot be exercised by the run they trigger. `e2e-fork` runs on `workflow_run`, which always takes the default-branch copy; `e2e-tag` runs on `workflow_call` from a tag push; `nightly` runs on a schedule. Only `pull-requests.yaml` is executed from the pull request's own head, so only there does escalating test the edit rather than the tree around it. That argument existed in the reference doc and was removed by this branch rather than answered, and the list it argued against is what replaced it. Measured over six months of main, 26 commits touch one of the three with no other escalating path; each now pays a full Chainsaw run it used to skip, on a suite whose flake rate is the reason people avoid running it for nothing. Keeping them is defensible: the rule stays one idea rather than two, and dropping them leaves those files inert, which reads as an oversight rather than a decision. What was not defensible was paying it silently. The trade now sits next to the pattern that makes it, with the number, so the next person weighing it starts from measurement instead of rediscovering the mechanism. The reference doc points there rather than carrying its own copy. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 05:55:37 +03:00
Aleksei Sviridkin	50c5e2c14c	test(e2e): capture the tenant worker guest serial console ... The tenant Kubernetes suites fail the node-Ready budget with zero Nodes registered, and the time is lost inside the guest between the worker VMI reaching Running and Talos requesting its apid certificate. Nothing collected today covers that window: the existing in-guest capture drives talosctl, so it needs the very apid the worker never reached, and on the management side there is neither a Node nor a certificate request to read. Turn on the guest console log for the suite's node group and collect each virt-launcher's guest-console-log container from the management cluster. Reads start at the beginning of the stream rather than tailing it, so a guest that repaints its console cannot push the boot output out of the window; every read is bounded, and the walk stops at a cap that records both counts, so the collector cannot become the unbounded term in a failure path that already runs several others under one deadline. A read that returns no console is not filed as a quiet guest. Three outcomes get three labels, because only one of them says anything about the guest: the read failed and returned nothing, the read succeeded and returned nothing, or it broke off part way. Each read's stderr is kept beside the capture rather than inside it, since kubectl's own "container is not valid for pod" would otherwise fill the file and a console that never started would read as one that had spoken. That case is the headline one here, so the Pod state and events that explain it are collected alongside, once for the whole selector. Enabling the console overrides a cluster-wide setting the platform holds for an open upstream defect, so the run is also an experiment, and its own failure mode is the one that hides best: the container wedges virt-launcher in Init, no worker boots, and the suite reports "fewer than 2 tenant nodes Ready" -- the same sentence the failure under study produces. Read as the known signature, the experiment's answer would be filed as the flake it was meant to explain. So the failure path names that case first, before any other diagnostic, and only when it is true. The passing path checks separately that the container attached at all, and reports the ratio it saw rather than a bare success, since the check is deliberately satisfied by any one Pod. It keeps three outcomes apart too: a read that did not answer, a selector that matched no Pod, and Pods that matched and carry nothing. It reads the Pod name alongside the container names so the last two cannot collapse, and both container lists so a future move of the container cannot fail a run over a diagnostic. It runs last, after everything the suite exists to prove, because a debugging aid must not preempt the assertions that cover the cluster. The capture runs before the talosctl one because it is the only of the two that can describe a worker which never reached apid, the shape this failure keeps taking. kubectl reports a warning and an empty result alike, on stderr with a zero status, so the exit status decides which artifact its output lands in. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 05:48:38 +03:00
Aleksei Sviridkin	a6c94c4528	fix(ci): correct an inverted caveat and clean up after the assertion ... Two defects introduced by the previous two commits. The in-tree lane's comment said `skip=true` cannot mean "a workflow nobody classified", and then explained, after a colon, exactly how it can: `.github/` is inert as a directory and the e2e workflows are exempted from it by name, so a new one reads as inert. The clause meant to warn a reader instead told them the one unsafe case is safe, in the copy consulted when deciding whether to trust a green gate. The other three copies of the caveat state it correctly. The scratch-directory cleanup landed before the last assertion in every test added here. That passes the EXIT-trap guard and inverts the reason the guard exists: both runners set `-e`, so cleanup placed last is unreachable on failure and the fixture survives for inspection, which `docs/agents/e2e-testing.md` §3 states outright and the already-converted `hack/select-install_test.bats` follows. Placed early, a failing test deletes the evidence of its own failure, and one test had the call between two assertions, so which of them failed decided whether the directory survived. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 05:37:07 +03:00
Aleksei Sviridkin	bbee3d7a1b	docs(ci): finish the sweep for the third copy of the inert claim ... The previous pass corrected the same explanation in the selector header and in the in-tree lane and missed this one, which sits in the fork lane's selection step. It still said an empty selection means "verified inert" and pointed only at the inert pattern, and it lacked the caveat the other two copies gained: that a workflow added under `.github/workflows/` reads as inert until it is named in the escalation list. Three copies of one explanation is the underlying problem; leaving one of them a version behind is the way that problem shows up. A correction is not finished while a copy of the old claim is still alive. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 05:22:36 +03:00
Aleksei Sviridkin	3eee4c5590	test(ci): clean up the new selector tests without an EXIT trap ... The ten tests added with the classifier each installed `trap 'rm -rf "$tmp"' EXIT`, taking this file from fifteen such traps to twenty-five. `hack/cozyreport.bats` freezes that count per unconverted file, so `make unit-tests` failed, and that runs in the required checks job. The count is the visible half. The reason it is frozen is that an EXIT trap replaces the one the bats binary installs for its own bookkeeping, and a test failing under it prints no TAP line at all. Reverting the step-6 escalation in a scratch copy made the flagship pin for this whole change, the one asserting an unclassified path escalates, vanish from the output: the run reported executing twenty-six of twenty-seven tests and never named the missing one. After this change the same mutation prints `not ok 21` with its name, file and line. Clean up inline before the assertion instead, which is what the two tests added alongside them already did. Behaviour is unchanged, all twenty-seven still pass, and the frozen count returns to fifteen rather than being re-frozen higher, since re-freezing would bank ten fresh instances of the thing the freeze exists to stop. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 05:22:01 +03:00
Aleksei Sviridkin	55f3c79e9f	docs(ci): stop pointing at the inert list for paths it does not hold ... Four places said an empty selection means the inert pattern matched, and sent the reader to `inert_config_pattern` to see which paths those are. Documentation, dashboards and `*.md` are dropped before that pattern is ever consulted, and a backups example whose app has no suite is dropped where backup examples are matched. A reader checking the pattern for any of them finds nothing and has to reconstruct the order by reading the script. Name the rules that produce an empty selection instead, and keep the claim to the one the lanes rely on: empty is a decision some rule reached, not a path nothing looked at. The same bullet claimed every path is classified. One is not. The inert pattern makes `.github/` inert as a whole directory and the workflows that run the suite are exempted from it by name, so a workflow added under `.github/workflows/` reads as inert and the fall-through never sees it. Today's enumeration is correct and I checked it against `rg -l test-chainsaw`, so this is not a live hole, but the sentence saying it cannot exist is what makes the reminder next to that list look decorative. Record it as the guard it is. Separately, the fork lane's rationale for appending a newline described a mechanism this branch removes: the reader now guards its own loop, so the terminator is no longer what keeps the last path alive. Say that it is now the text-file contract and a second line of defence across two files, which is a reason to keep it and a different reason than the one written there. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-08 05:02:25 +03:00