mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-08 21:14:56 +09:00
b9846fe42eebbf01859b125ab9a900cd0ba7dc1e
780
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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> |
||
|
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 --> |
||
|
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 -->
|
||
|
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 --> |
||
|
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 --> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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 ``` |
||
|
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 --> |
||
|
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 --> |
||
|
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> |
||
|
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> |
||
|
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>
|
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
Aleksei Sviridkin
|
548dfa9520 |
docs(ci): say what an empty selection actually means
Four places on this branch describe an empty selection as "classified inert", against a list of three outcomes that every path is said to land in. There is a fourth: an edit under examples/backups/<app>/ where the app has no Chainsaw suite selects nothing, decided inline where backup examples are matched rather than through the inert list. The rule is deliberate and explained where it lives; it just is not one of the three the summary enumerates. The property the two lanes actually depend on is weaker and true: an empty selection means some rule decided the change cannot affect a suite, never that nothing looked at the path. State that instead, in the header that defines the classes and in the privileged lane's comment that consumes the result. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
cc522e7def |
fix(ci): classify the last path when the list has no trailing newline
The classifier this branch introduces is only total if every line reaches the loop that applies it. POSIX read assigns the final line and then returns non-zero when the input does not end in a newline, so a plain `while read` drops it. The fall-through that escalates an unrecognised path lives inside the loop body, so the dropped line is never classified at all and the selection comes back empty. Both lanes read empty as "skip Chainsaw" and post the required status green. The shape that matters is a list whose only unclassified path is last: `docs/x.md` followed by an unrecognised path with no terminator selects nothing, where the same list with a terminator escalates to all 21 suites. The safety net this branch is built around is bypassed by exactly the input it exists to catch. No caller triggers it today, because `git diff --name-only` terminates its output and the fork lane appends a newline explicitly. That makes it caller discipline documented in one of two callers, which the next caller will not read. Guard it where the assumption lives. Both new cases are red without the guard and the twenty-five existing ones stay green, so they pin this behaviour rather than the loop's general shape. They clean up inline instead of through an EXIT trap: that trap replaces the one the bats binary installs for its own bookkeeping, and a test failing under it can print no TAP line at all, which is the opposite of what a regression pin is for. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
ae4e9e55cb |
fix(ci): classify every TIA path and escalate the unclassified
`select-e2e.sh` silently selected nothing for a changed path it did not recognise outside `packages/`. Both e2e lanes read an empty selection as "skip Chainsaw" and then post the required "E2E Tests" status green, so an unrecognised path produced a green merge gate with no suite run. The worked example is `hack/common-envs.mk`, which supplies the tag, push and output flags of every image in the tree and matched nothing: `full_suite_pattern` covered `hack/*.sh` and `hack/*.bats` but not `hack/*.mk`. `pkg/`, `tools/`, `go.mod`, `hack/lib/` and `hack/buildkitd.toml` were in the same position, as was `.github/workflows/e2e-fork.yaml` while `pull-requests.yaml` was already escalated. A blanket "empty means full suite" fallback would be the wrong fix: it escalates the inert paths the design deliberately skips. Instead every path now lands in one of three classes by an explicit rule — the full-suite list for shared build inputs and the e2e harness, an inert list for paths that cannot affect what e2e exercises, and the PackageSource graph for `packages/(apps|system|extra)/`. A path matching none of them escalates and logs why, so the next path someone adds fails safe instead of failing open. The two lists are checked in that order, so a specific escalation beats a broad inert directory: `.github/` is inert, the workflows that run the suite are not. `*.md` and `docs/`/`dashboards/` stay ahead of both, so a README under `packages/core/` does not inherit that tree's escalation. Ten tests cover both sides of the classification. Mutation-checked: reverting the fall-through fails the unclassified-path test, dropping `.github/` from the inert list fails the non-e2e-workflow test, and swapping the two list checks fails the existing pull-requests.yaml test. Note the per-entry escalations are belt-and-braces — removing `hack/*.mk` from the pattern still escalates via the fall-through, which is the point of having it. Closes #3392 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
7dca642894 |
fix(ci): restore e2e for fork PRs and close the merge-gate bypass (#3262)
## What this PR does Fixes #3257 — fork PRs never ran e2e, and the skipped required check let them merge anyway. Fork PRs get no secrets on `pull_request`, so `make image` pushed anonymously and died with `denied`. That failed every build, collapsed the `build → finalize → e2e` chain, and left the required **E2E Tests** job *skipped* — which branch protection treats as satisfied, so an approved fork PR could merge with e2e never having run. This implements **Option A** from the issue (privileged `workflow_run` split), pushing fork e2e images to the existing CI registry (OCIR): **Build primitive (`hack/common-envs.mk`)** - New `OCI_EXPORT_DIR` mode: build each image to a per-image OCI archive instead of pushing, and force `PUSH`/`LOAD` off. The digest is captured via `--metadata-file` regardless of output type, so the refs baked into `pr.patch` match what the privileged run later pushes. Threaded through the `image-tags` macro every package already uses — no per-package Makefile change. **`pull-requests.yaml` (unprivileged, fork branch)** - `build` / `build-talos` / `finalize` export images to OCI archives and upload them as artifacts instead of pushing → the red `denied` build wall disappears and the chain no longer collapses. - The in-tree e2e job is renamed off `"E2E Tests"` and guarded to same-repo PRs. A new `e2e-report` job concludes the required `"E2E Tests"` context as an **explicit check-run** (success on a docs-only PR or when e2e passed, failure otherwise). A skipped job can no longer satisfy the required check — this also closes the same hole for same-repo PRs whose build fails. **`e2e-fork.yaml` (new, privileged `workflow_run` from the default branch)** - Resolves the fork PR, opens a pending `"E2E Tests"` check-run, and fails-closed if the fork build failed. - Pushes the pre-built image archives to OCIR **by digest** and flux-pushes the digest-pinned packages tree. The OCIR token is live only for trusted binaries (`git`/`skopeo`/`flux`/`yq`) over data — never for `make` or any fork-authored script. - Runs the e2e suite against the pushed closure with **no push token** and concludes the check-run on the PR head SHA. Fork build code never receives registry credentials; the fork's images and test scripts run only in the credential-less, ephemeral e2e job — a stronger posture than a `pull_request_target` run that would hand fork code the secrets directly. ### Validation Local: buildx OCI-archive output + digest capture, macro expansion for single- and multi-image packages, non-fork push path unchanged, `actionlint` clean. Needs live validation (why this is a **draft**): `workflow_run` artifact download across runs; that the `"E2E Tests"` commit status satisfies the required context and keeps a fork PR non-mergeable until the privileged run concludes it; `refs/pull/<N>/merge` fetch + `git apply --3way`; `skopeo --preserve-digests` end-to-end. `workflow_run` only runs from the default branch, so the privileged half is exercised by mirroring the workflow onto a personal fork's default branch and opening a throwaway fork PR into it — not by merging first. _Since review_: the fork `e2e` job ran the pre-#2826 BATS app loop that the merged Chainsaw migration deleted (already broken); it now runs the same Chainsaw + TIA path as the in-tree job, with suite selection and the `full-e2e` override sourced from `resolve`'s trusted base-repo file list / labels (not a merge-ref diff). Publish job hardened per the non-blocking follow-ups: base-tree image allowlist, `skopeo copy --all`, pinned flux CLI. The required `E2E Tests` gate is now published as a commit **status** (default token + `statuses:write`), not a COZYSTACK_CI app check-run — the app lacked `checks:write` (`checks.create` 403'd) and a default-token check-run floated under a labeler suite; a commit status has neither problem, and forks can't post one (their `pull_request` token is read-only). Two review-bot findings fixed too: fork `OCI_EXPORT_DIR` is now absolute (a relative path misplaced archives under `make -C <pkg>` and every fork build would fail at upload), and the "≥1 archive" guard no longer no-ops under `nullglob`. ### Repo-admin prerequisites One repository setting lives outside this diff: 1. **Branch protection: keep `E2E Tests` required.** The gate is now a commit **status** named `E2E Tests` (posted with the default `GITHUB_TOKEN` — see `docs/agents/e2e-testing.md` §10), not a job or an app check-run. Branch protection already requires `E2E Tests` (currently the e2e job's name, renamed here to `E2E (in-tree)`), so the required context now resolves to the status — **no GitHub App, no `checks:write` grant, no app-id pin needed**. Just don't pin the context to a specific app. 2. **Actions → "Require approval for all outside collaborators".** Each fork run must be a deliberate maintainer click before it executes, since the fork's unprivileged run produces the artifacts the privileged `e2e-fork.yaml` consumes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff (7 files: two CI workflows, `docs/agents/e2e-testing.md`, `hack/common-envs.mk` + its bats suite, and two package `Makefile`s). The one trigger worth naming is "change developer tooling (`cozyvalues-gen`, `cozypkg`, the package `Makefile`s) → `development.md`" on the website. It does not fire here: `OCI_EXPORT_DIR` is a CI-only build mode that defaults off, and the local developer workflow is unchanged — `make image` still builds and pushes exactly as before, which the new default-path case in `hack/common-envs_test.bats` now pins. No chart values, no API surface, no app list, no platform variant, no Talos version bump and no release asset rename are touched. - [x] No downstream repository is affected by this change ### Release note ```release-note fix(ci): fork pull requests now run e2e via a privileged workflow, and a fork PR can no longer merge on a skipped (never-run) required E2E Tests check ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a dedicated fork E2E workflow that runs after the base PR and reports results via the required **E2E Tests** commit status. - Fork PRs now export build images as OCI archive artifacts for later publishing. - Added a same-repo E2E reporting job and renamed the in-tree E2E flow to **E2E (in-tree)**. - **Bug Fixes** - Added tighter, fail-closed gating to ensure merge blocking when **E2E Tests** isn’t correctly finalized. - **Documentation** - Documented fork vs same-repo E2E split, OCI export behavior, and required fork security settings. - **Tests** - Added Bats coverage validating `OCI_EXPORT_DIR` and preventing unintended registry pushes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
bc6d5e67b7 |
fix(e2e): keep failure snapshots complete and gate tenant patch on aggregated-API auth (#3444)
## What this PR does Two unrelated defects made the sandbox install job's failure handling unreliable. This fixes both. The first hit every red run. When a test fails, the diagnostic snapshot runs `crust-gather collect`, and its host-log leg spawns a privileged debug pod per node. The sandbox runs baseline PodSecurity, which rejects each of those pods with a 403, so crust-gather keeps retrying until it burns its whole collection budget and exits 1. The snapshot verdict then printed `FAILED` on every failed run even though the namespaced object state was already captured, so the line that says whether the uploaded snapshot is trustworthy was always wrong. The host-log leg collects nothing usable here anyway, so this passes `--disable-additional-logs` at all three collect sites: the host snapshot from the BATS runner, the host snapshot from the Chainsaw error handler, and the nested tenant snapshot. Object collection now finishes and the verdict again separates a real collection failure from a complete snapshot. The second is a race in the install test. The "Configure Tenant" step issues its first `kubectl patch` of the root tenant as soon as an earlier test sees the aggregated APIService go `Available`. That condition only tracks endpoint availability, not the serving pod's auth state. A freshly rolled cozystack-api pod can start answering before it has loaded the requestheader client-CA, and until it does it drops the front-proxy identity and answers as `system:anonymous`, so the patch is rejected with a 403 on the test's very first command. This adds a short authenticated GET against the same resource before the patch, so the step proceeds only once the aggregated API both serves and authenticates the request. If the aggregation layer is actually wedged past two minutes the gate fails loud instead of hiding a real regression. Both were traced from a recent sandbox failure (run 30004030331, during PR #3441's CI). Out of scope: the broader convergence flakiness of the install step is not touched here. That covers the long reconcile windows for the storage and platform releases and the app-level races in the Chainsaw suite. The durable product-side fix for the second defect, where cozystack-api should not report ready before the requestheader client-CA is loaded, is a separate follow-up. This partially addresses #3368, its crust-gather diagnostics item, and does not close 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. --> No UI changes. ### 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. --> The diff touches only the e2e test harness (`hack/cozytest.sh`, `hack/e2e-chainsaw/.chainsaw.yaml`, `hack/e2e-chainsaw/_lib/run-kubernetes.sh`, `hack/e2e-install-cozystack.bats`). The trigger map's `hack/` couplings are `hack/e2e-prepare-cluster.bats` (node prerequisites for ansible-cozystack and talm) and `hack/package.mk`, `hack/common-envs.mk`, `hack/update-crd.sh` plus the package layout (ccp, external-apps-example, cozyhr). None of those are moved, renamed, or edited here, and nothing else in the map matches. - [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): keep failure snapshots complete when PodSecurity blocks crust-gather's host-log leg, and wait for the aggregated API to authenticate before patching the root tenant so the install step no longer flakes on a system:anonymous 403 ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved failure diagnostics during automated Kubernetes and tenant tests by avoiding unsupported privileged log collection steps. - Ensured failure snapshots complete more reliably instead of timing out or returning incomplete results. - Added a readiness check before configuring the root tenant, preventing intermittent authorization failures during installation tests. - **Tests** - Increased reliability of installation and end-to-end test workflows under baseline security policies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
8d5ab4cfee |
fix(hack): arm cozytest cluster captures for e2e suites only
The runner's on-failure EXIT trap gates each cluster capture 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. What that cost depends on how kubectl fails. Against an endpoint that refuses immediately it is a second; against one that hangs, measured with a stub that never returns, it is ~28s for the previous-logs leg, which self-bounds its pod list and so never reaches the 300s ceiling above it, plus the full 600s for the data-plane leg, whose first pod list carries no bound of its own. Ten minutes of dead waiting for a failure that should cost seconds. Where crust-gather is installed too the bill stops being empty and starts being wrong: that leg needs no reachable cluster to be pointless, only an ambient KUBECONFIG, and it will spend up to 360s snapshotting whatever cluster the current context names. Gate the three legs on the e2e- prefix instead, read from the suite's own name or from the directory holding it. At the top level that prefix is what 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 this change. The e2e path is otherwise unchanged, every leg, bound and message as it was. Not a reachability probe, though that 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. The test drives the runner as a subprocess over generated fixtures that differ only in where they sit, with stub kubectl and crust-gather on PATH. Stubbing both matters: the runner calls crust-gather directly rather than through a script that needs kubectl output first, so a kubectl-only stub would leave that leg reaching for the real binary against the ambient KUBECONFIG. Each leg is pinned by the artefact it leaves in the report tree, not only by the one leg that echoes, so no single leg can be ungated without a test going red; each was checked by ungating it alone. All three directions are asserted, and the two armed cases are the positive control for the disarmed one, whose assertion is negative and would otherwise pass just as well if the harness ran nothing; that case also pins that the fixture did fail. Two shell traps are handled rather than left to be rediscovered. The awk transform rewrites any line beginning `@test "` and any line that is exactly `}`, and knows nothing of heredocs, so a fixture written as a heredoc has its own `@test` line harvested into the outer suite as a phantom test that fails first and ends the run before the real tests; the fixtures are built with printf instead. And `set -e` ignores a command whose status is inverted with `!`, so the negative assertion is spelled as an `if`. Both were checked by building the broken variant, not reasoned about. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
cc05c92e47 |
test(ci): require the discarded-label guard per job, not just agreement
The check collected every comparison against the label name and required them to agree on one string. Agreement is not presence: deleting `e2e-report`'s guard outright leaves the other two agreeing with each other, so the assertion stayed green while a discarded label event began publishing again and clobbering the verdict of the run that had actually tested the SHA. That clobber is the defect two earlier commits were written to stop. Require the guard inside each of the two jobs that must carry it, and keep the agreement check for the rename it does catch. Deleting either copy now fails. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
cc8b01d5cd |
docs(ci): say when each lane's status opener actually lands
The reference section claimed both lanes open the required context when their run starts, and enumerated the triggers it protects against: a re-run, a reopen, a `full-e2e` label. Every one of those starts a Pull Request run, and on a fork that run cannot open the status at all. The opener is guarded same-repo only, because a fork's `pull_request` token is read-only, and the fork's opener lives in the privileged workflow, which starts only once the unprivileged build has finished. So on the lane the sentence was most likely to be read about, an already-green head SHA keeps reading green for the whole build. The gap is structural. A read-only token cannot post a status at any point in the run, and that same read-only token is what makes publishing the gate with the default token safe in the first place. It is bounded by the build rather than open-ended. What was wrong was only the claim that it does not exist. Say so in all three places that carried the overstatement: the section itself, the opener's own comment, and the contract test, which pins that each lane has an opener and cannot express when it lands. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9c68b1f83e |
fix(ci): stop a cancelled fork run from publishing a verdict
The invariant that a cancelled run publishes nothing was written down in three places and held in two of them. The fork lane's terminal reporter ran under `always()`, which includes cancellation. When a second privileged run supersedes the first, the dying one still posted `failure` on a head SHA it no longer owned, for a suite that never finished. It self-corrected once the superseding run reported, but that can be two hours later, and until then a legitimate pull request reads red. The same-repo lane was moved off `always()` for exactly this reason. Move this one too: a failed publish or e2e still reports, since neither is a cancellation, and a cancelled run now leaves the `pending` it opened standing until its successor concludes. The same-repo guard's rationale cited a scenario that no longer exists. It described a label event cancelling the run that opened the pull request, which the concurrency split made impossible for every label the workflow discards. The guard is still needed, for a push, for a `full-e2e` label superseding the head run, and for a manual cancel, so the comment now names those instead of an extinct one. The reference section listed the pieces this invariant rests on and omitted the reporters, which is the half a reader is most likely to touch, and counted them, which was already wrong and would go wrong again on the next addition. It now enumerates without counting. The contract pins both reporters, because both lanes have had this defect at different times in the same shape. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
8a21085324 |
fix(ci): decide docs-only from both sides of a rename
The two lanes answered "is this docs-only" from different sources and could disagree. `plan` reads `git diff --name-only`, which applies rename detection and reports a rename as its new path alone, so moving a code file into docs/ read as docs-only. The fork lane reads the PR file list, where a rename carries `previous_filename`, and reads the same change as code. Disagreement is not a near-miss here. The same-repo lane builds nothing and uploads no patch artifact; the fork lane concludes e2e is required and then fails closed because that artifact is missing, reporting that the fork build did not complete. It completed — it correctly built nothing. There is no push the contributor can make to clear it and no label that overrides it, so a legitimate contribution is permanently red. Point the docs-only decision at the rename-blind list, which the job already computes for the downstream-trigger-map flag and whose comment names this exact trap. Both lanes now agree, and the same-repo side stops shipping a removed code path untested — until now such a change skipped every build and reported e2e as not required. The build matrix keeps reading the rename-detected list: it picks packages to rebuild, and a path that no longer exists has nothing to build. The contract file gains the agreement as a pin, and grows a header to match what it now covers. The fork half is pinned on the docs-only expression rather than on `previous_filename`, which also appears in the file list a few lines down and would have kept the assertion satisfied with the check deleted. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
7fc568db4f |
test(ci): pin each status publisher by its call, not by a shared string
The case asserting that both lanes open the required status before they conclude it counted occurrences of the context string per file and required two. That measured nothing on the fork lane: the two occurrences there are the shared `setStatus` helper and the terminal report, while the opener calls the helper and contributes no occurrence of its own. Deleting the opener outright left all six tests green, under a comment promising that dropping either half would be caught — and the window it guards is the wider of the two, since a privileged run holds the head SHA for up to three hours. Scope every assertion to the job that owns it and pin the call rather than a string that happens to appear twice: `plan` opens with `pending` under a same-repo guard, `e2e-report` concludes, `resolve` opens through the helper, `report` concludes. Removing any one of the four now fails this test. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
ee844c98a5 |
fix(ci): open the "E2E Tests" status when the same-repo run starts
Moving the gate from a job to a commit status lost a property the job had for free. A commit status is latest-wins per (sha, context) and never expires, so a head SHA that already went green keeps reading green from the moment a new run starts until `e2e-report` posts at the end. Re-running all jobs, reopening, or adding full-e2e therefore leaves the pull request mergeable for the whole of a suite that may be about to fail. While the gate was a job, GitHub created a fresh non-success check run each time and the window did not exist. Claim the context at the start of `plan`, the way the fork lane already claims it in `resolve`, and let `e2e-report` conclude it as before. The call is same-repo only: a fork's `pull_request` token is read-only, so posting from there would 403 and fail the job on every fork PR — and that read-only token is what makes the whole commit-status design safe. Fork PRs get their opening status from the privileged workflow instead. The contract test grows a case pinning that each lane writes the context from two points, one of them pending. Both assertions on the same-repo opener are scoped to the `plan` job: the bare fork guard occurs on several steps in other jobs, so a whole-file "at least one" passed with the opener's guard deleted, pinning unrelated guards and reporting that as coverage. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
8068619732 |
test(ci): state the scope note's reasoning as transitive, not direct
The header claims the third silent path is unreachable because every job either carries the discarded-label guard or needs `plan`'s outputs. The `e2e` job does neither literally: it gates on the results of `finalize`, `build-talos` and `resolve_assets`, which are the jobs that read those outputs. The conclusion survives — a skipped `plan` still skips it, one hop further along — but transitivity is the part the claim rests on, and a reader checking the stated reason against `e2e` finds it false and has no way to tell whether the conclusion failed with it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
7696d2b98c |
test(ci): record which silent path the concurrency pin does not cover
The contract asserts that the conclusions routed into a per-run group match the ones `resolve` posts nothing for, which reads as covering every case where a run stays silent. It does not. `resolve` also returns silently when the triggering run succeeded but its `Plan build` job skipped, and such a run sits in the shared group where it can still cancel the live publisher. That path is unreachable today, because every job in pull-requests.yaml either carries the discarded-label guard or needs `plan`'s outputs, so a skipped `plan` yields a skipped run and the conclusion check catches it first. Adding a job that runs independently of `plan` would reopen it, and the extraction here matches on the conclusion constant and would not notice. Say so in the header rather than leaving the next reader to infer coverage the pin does not provide. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
6cfeaddf78 |
test(hack): clean up the Talos diagnostics tests without an EXIT trap
Eight of the ten tests in this file removed their scratch directory from a `trap ... EXIT`. Such a trap replaces the one the bats binary installs for its own bookkeeping, so a test that fails prints no TAP line at all: not `not ok`, nothing. The run ends with a warning that it executed nine of ten tests, and anyone reading the tail of the output, or grepping it for `not ok`, sees a suite with no failures in it. Break one assertion in this file and the difference is the whole point. Before: `1..10` followed by `# bats warning: Executed 9 instead of expected 10 tests`, with no line naming the test that failed. After: `not ok 2 reader Certificate is short-lived and never embeds credential data`. Remove the directory at the end of each body instead. Both runners set -e, so on failure the removal is unreachable and the directory survives for inspection, which is what a failed test wants anyway. The one test that runs the orchestrator from inside its scratch directory returns to where it started first -- not because rm would refuse, since BSD, busybox and GNU rm all remove a directory from inside it and return 0, but because the runner's own bookkeeping still runs in that subshell after the body returns, and would be doing so on an unlinked cwd. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
575b41e1cd |
test(hack): let each bats file declare the EXIT traps it still holds
The ban on EXIT-trap cleanup in hack/*.bats was enforced by two lists inside hack/cozyreport.bats: one naming the files known to be clean, one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard kept failing. A change adding a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second landed. A file arriving with traps hit the same wall from the other side -- the inventory did notice it, since the string it compared was built by scanning the directory, but absorbing it meant an edit in a file nothing in the author's diff pointed at. Replace both lists with a declaration each file makes about itself: one "# EXIT-TRAP DEBT: N" comment, exact rather than a ceiling. A file carrying none must install no EXIT trap, which covers the converted files, the files that never had one, and every file added later. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count collide textually instead of silently. The declaration is read from the leading comment block, not from anywhere in the file. A .bats file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test. Reading it there as a statement about the whole file would let an unrelated fixture excuse a real trap, and would do it silently, since nothing in that test's own diff looks like a declaration. A comment block is the region with no interior: stopping instead at the first @test would still read a line out of a helper's heredoc. Not every counted handler is debt. A trap inside an explicit subshell does not replace the bats binary's own, so a test failing inside `( ... )` still prints its `not ok`. hack/e2e-test-openapi.bats kills a backgrounded kubectl proxy that way, and moving the kill to the end of the body would leak a process holding a fixed port. Its declaration records that rather than scheduling a conversion, and because the ratchet is exact, removing the trap fails too -- the count protects the construct. What the count cannot do is tell the two apart: substituting a test-level trap for the subshell one keeps the total at 1 and stays green, which the header states rather than leaves to be found. Counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed `trap ... EXIT; cd "$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap ... EXIT` and `(trap ... EXIT; true)`; upper case missed `trap ... exit`, which bash and dash both install. All are real handlers that scored zero, and the left boundary matters most, since the inventory being replaced had none and caught the semicolon form. A bare word boundary is not enough either way: `bootstrap ` ends in `trap `, and it must keep scoring nothing. A quoted signal counts for the same reason as the rest. Two handlers sharing one line are reported rather than counted, since the count is a count of lines and the second would otherwise arrive without moving the total; splitting the line properly needs a shell parser, a semicolon inside a handler's own action not being a separator, and guessing wrong undercounts -- the one direction a ratchet cannot afford. The scan recurses, so hack/e2e-apps/*.bats is covered rather than sitting one directory below the guard that claims the tree. It reads .bats and nothing else, so a handler arriving through a sourced .sh stays outside it -- hack/e2e-chainsaw/_lib/run-kubernetes.sh installs two, each benign for its own reason rather than by design: one sits in a function declared with `(` and so runs in a subshell, the other in a brace function no @test calls. That boundary is stated in the header rather than papered over. The guard moves to hack/bats-no-exit-trap.bats: its subject is every unit suite under hack/, not the report collector it grew up in. Its fixture helpers assemble the trap keyword and the signal from separate arguments, because the guard scans its own source and a fixture written as a literal would be counted as a real trap in it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
999aee5638 |
test(ci): count occurrences in the gate contract, not matching lines
Six assertions ran `grep -c` against a variable holding a single line, so `-eq 1` only asked whether the pattern appeared at all. A key naming the same field twice — two run-id interpolations, two branch components — satisfied the pin exactly as the correct key does. Demonstrated on the run-id assertion: duplicating the interpolation leaves `grep -c` reporting 1 and passing, while counting occurrences reports 2 and fails. Use `grep -o … | wc -l` for the single-line pins. The two whole-file `cancel-in-progress` counts stay on `grep -c`, where counting matching lines is the intended question. This is the same correction made once already in the sibling contract suite; the form did not survive being copied. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e2643ef4b9 |
docs(ci): explain the fork image allowlist and fix a test comment
Two statements that were not true of the code next to them. The fork `publish` job accepts only archive names it can derive from the base parent, so a fork cannot name an artifact into an arbitrary registry repository. The consequence is that a fork PR adding a new image fails on `image '<name>' is not in the base-tree allowlist` with no way to pass, and nothing said so — including the E2E section that AGENTS.md points a reader at when a fork run fails. Record the constraint, the exact log line, and the two ways around it, so the failure reads as the guard working rather than as a broken PR. The concurrency contract's comment on `code_lines` claimed a real grep error propagates out of the helper. It does under the bats binary, and not under `hack/cozytest.sh`, whose translator appends `return 0` to every line that is exactly `}` — file-level helpers included — so a helper's last command never decides its status there. Both runners still fail closed, because each test asserts its extraction is non-empty before comparing, so a swallowed error surfaces as empty input rather than as a pass. Say that instead. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
7c5e96947e |
fix(ci): stop two runs from publishing "E2E Tests" on one SHA
Giving every `labeled` event its own concurrency group stopped a discarded label from cancelling a live run, but it also stopped a `full-e2e` label from doing so — and that label is not discarded. `plan` proceeds for it and `e2e-report` publishes with it, so the labelled run and the head run it was meant to widen both post the `E2E Tests` context, on the same head SHA, with no ordering between them. That was a deliberate trade at the time: commit statuses are latest-wins per context, so the reasoning went, the full-suite verdict lands last whenever its suite finishes last, and the label is an explicit opt-in to a longer run. This commit reverses that decision, because the premise only holds while the full run succeeds. The full suite is the superset, so a full-e2e run that fails early — a build flake, a suite failing at minute twenty — finishes before the narrower head run it was meant to widen, and that run's later green erases the failure. The required status then reads success for a SHA whose suite did not pass. One publisher per head SHA is worth more than the minutes of runner time a cancelled narrow run costs. Move aside only the label events `plan` discards, by making the concurrency condition the exact complement of `plan`'s guard. A `full-e2e` run stays in the main group and supersedes the head run the way a push does, so exactly one publisher of the context is alive per head SHA. Opting into the full suite now costs one suite instead of two, and a second label landing afterwards can no longer cancel the full-e2e run, because the two are no longer grouped together. The contract test gains the complement — the group key must exclude the publishing label rather than every label — and a check that the label name is one and the same string in all three guards that read it, since renaming it in one place splits the decision without touching a line that reads load-bearing. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
3c50a09457 |
fix(ci): keep a discarded run out of the fork e2e gate's group
The privileged fork e2e run groups by fork repo + branch with cancel-in-progress, so every completed Pull Request run for that branch lands in the same group -- including a run that built nothing. Labelling an open fork PR produces exactly that: pull-requests.yaml triggers on `labeled` and `plan` discards every label other than full-e2e, so all jobs skip. The privileged run for that event cancels the one that is mid-suite on the same head SHA, then `resolve` recognises the discarded trigger and posts no status at all. The required "E2E Tests" context is a commit status, so the head SHA is left at "Expected" with nothing left to flip it -- the run that replaced the owner is the very one that decided to stay silent -- and the PR stays unmergeable until the contributor pushes again. Key the group by the triggering run's id when that run concluded `cancelled` or `skipped`, so a run that will post nothing can cancel only itself. Real runs keep superseding each other on fork repo + branch. The contract test ties the conclusions in the key to the ones `resolve` stays silent for. Those two lists sit in different languages in different halves of the file, and a third conclusion added to the guard alone would restore the wedge without touching a line a reviewer reads as load-bearing. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
353b8a1a6d |
ci(backport): skip a target whose backport already merged
A second label event on a PR whose first backport already merged (for example backport-previous added shortly after backport merged) re-enters the backport job for the target that already has the change. The action's branch name for a backport is deterministic, so that leg's cherry-pick is empty rather than conflicting, and git still exits non-zero for an empty pick. Under this repository's draft_commit_conflicts setting the action reads that exit code as a conflict and tries to commit an empty working tree, which also fails and surfaces as a false backport-failed comment on the original PR for a branch the change has been on for a while. Add a guard step that looks up whether a merged pull request already exists on this target's deterministic backport branch, and skip the checkout and the backport action for that leg when one does. The guard changes nothing about the empty-cherry-pick behavior itself, which lives in the action and needs a fix there; it only keeps a known-stale re-run from reaching the action and misreporting. Extend the existing backport.yaml contract tests with the new guard's invariants: the merged (not merely closed) check on the deterministic branch name, both downstream steps gated on the guard's output, and step ordering. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
0ba567dd73 |
fix(tenant): bound the pre-delete cleanup waits and the hook Job (#3598)
## What this PR does The tenant pre-delete hook deletes the tenant's HelmReleases in two waves with `kubectl delete --wait=true` and no `--timeout`. kubectl reads `--timeout=0` as "wait forever" and substitutes a week, so a wave hangs for as long as any child HelmRelease cannot finalize. helm then returns from the uninstall without deleting anything: the release stays in storage, flux keeps the finalizer, and the same teardown gets retried. The delete policy carries no `hook-failed`, so the waiting Job survives between attempts too. Each wave now gets 90s. Neither is guarded, on purpose. Nothing stands behind these deletes, so a wave that did not finish ends the run instead of reporting a cleanup that never happened. That also keeps the order the two waves exist for, since the tenant modules are what the applications run on. Whatever got deleted stays deleted, so the next attempt resumes from there. The Job gets `activeDeadlineSeconds: 270`. It bounds what the waits do not, since the DELETE calls carry kubectl's default `--request-timeout` of 0, and it sits under the 300s uninstall budget flux applies when the HelmRelease has no timeout of its own. That is the budget for tenants cozystack-api generates; `tenant-root` is the exception, a static release in `cozystack-basics` carrying `timeout: 15m0s`, and the ordering holds there too since 280 is under 900. The 30s gap is not cosmetic. The deadline runs from the Job's `.status.startTime` while helm's budget runs from before the Job is created, so at equal values helm gives up first, on a Job that is still running. That is the state this change is meant to end, and it would look fixed, because a number would be there. `terminationGracePeriodSeconds: 10` is part of the same arithmetic. Since 1.31 the Job controller holds back the terminal condition until every pod has finished terminating, so what helm waits for is the deadline plus the termination. At the 30s default that lands on 270+30, level with the budget, which is the gap the deadline was set to buy. That period gets spent in full, every time. The SIGTERM goes to PID 1, which is the shell: the script runs several commands, so the shell never execs into kubectl and stays PID 1 itself. A signal carrying its default disposition is discarded by the kernel when the target is PID 1, and a non-interactive shell installs no SIGTERM handler, so nothing acts on it. kubectl is a child and was not signalled at all. The pod ends on SIGKILL. Nothing in that script needs to finish gracefully, the DELETE calls are already sent and the process is only watching. The pod also gets resource requests, matching the sibling hook in `packages/apps/kubernetes`. Without them it is BestEffort and first to go under node pressure, which mattered less when a retry inside the Job absorbed an eviction. With a single attempt it does not. `backoffLimit: 0` and `restartPolicy: Never` go together, and neither implies the other. At the default limit of 6 the Job does not reach `Failed` inside the budget, so helm falls back on its own timeout with the same nameless "timed out waiting for the condition" as before. Under `OnFailure` the pod survives its own failure and the kubelet re-runs the container in place, and the Job controller finds out afterwards by summing `restartCount`. The script starts over, re-enters the applications wave that just expired, and gets cut down mid-run when the controller catches up. `Never` puts a failed pod in front of the controller directly. ## What this PR does not do No remedy behind the bounded waits. The `packages/apps/kubernetes` hook force-clears child finalizers when its own wait expires. That needs `patch` on HelmReleases in the Role, and it is only safe there because the whole tenant control plane is going away in the same run. Here a wave that expires ends the attempt. `packages/apps/kubernetes/templates/delete.yaml` is untouched. It does not make a doomed teardown converge. A tenant whose children cannot finalize still gets its uninstall retried. What changes is that each attempt ends in bounded time, instead of eating the whole uninstall budget and leaving a pod behind. 90s is a guess, and it is tight. The first wave deletes every application HelmRelease in the namespace, including kinds that are slow to tear down: a nested Tenant runs this same hook one level down before its release finalizes, and an app of kind Kubernetes tears a control plane down. Either one can outrun 90s. The bound is what is left once the Job deadline sits under helm's budget instead of level with it, split between the two waves. The 90s the deadline keeps back for scheduling and the image pull is not calibrated either, and I would rather say so than imply it was. `docker.io/clastix/kubectl:v1.32` comes from Docker Hub with no mirror in front of it, and this repo has measured Docker Hub pulls of a smaller image at about 1m42s in CI. So that remainder is roughly the observed worst case rather than comfortably above it. A cold pull that slow lands on the `activeDeadlineSeconds` path, which is the one that deletes the pod and leaves nothing to read. There is a band that pays for this, and I would rather name it than round it off. Neither shape above fits inside 300s today, so for them the bound costs nothing that works. But every generated child HelmRelease carries a wait strategy, so a wave inherits its slowest child, and a wave that settles in something like 95s does complete inside the budget today. After this change it expires instead, and that tenant trades one clean attempt for an extra flux retry. In wall clock that retry is about five minutes, since `helmrelease-interval` defaults to `5m`, so a tenant in that band takes noticeably longer to delete than it does now. I think that is the right side of the trade: an unbounded wave does not fail, it spends the whole budget and leaves helm reporting a timeout that names nothing. If the band turns out to be populated, the waits and the deadline move together, and the bats suite pins their ordering rather than their values. One caveat on the diagnostics, since the comments in the chart used to promise more than they deliver. The failing wave leaves its output in the failed pod, which `ttlSecondsAfterFinished: 300` then reaps while flux's uninstall backoff is still climbing. And on the `activeDeadlineSeconds` path there is no log at all, because the Job controller deletes the active pods when the deadline expires. That path is the backstop for a kubectl that never returns; the expected path is the wave's own `--timeout`, where the script exits on its own and the pod survives to be read. ## Tests `hack/tenant-pre-delete-hook.bats` renders the chart, pulls the hook script out of the Job and runs it against a stub `kubectl` on `PATH`. It covers a clean run reporting success, every delete reaching kubectl with a non-zero bound, the ordering of the four numbers, and a wave that gave up ending the run without going on to the next one. `--timeout` is implemented inside kubectl, so no stub can make that deadline actually elapse. The tests read the arguments that reached kubectl and inject the non-zero exit an expired delete produces. No EXIT traps. The ordering test reads the numbers out of the rendered Job instead of pinning them, so moving a value keeps it honest while a value that breaks the ordering still fails. An absent deadline or termination period is rejected explicitly, otherwise `null` satisfies the numeric comparison and a Job with no bound at all passes. The Job's own shape is pinned in the chart's helm-unittest suite, since the script cannot observe that about itself. Both suites are green locally, and I mutated each pinned value to confirm they fail for the right reason: dropping either `--timeout`, dropping `activeDeadlineSeconds`, dropping `terminationGracePeriodSeconds`, dropping the requests, `backoffLimit` back to 6, `restartPolicy` back to `OnFailure`, and a `|| echo` appended to the applications wave. Each one goes red in the test that claims that ground. Worth flagging so the check marks are not read as more than they are: `make bats-unit-tests` is red at the merge base, unrelated to this branch, and the loop it runs exits on the first failing file. `hack/cozyreport.bats` sorts ahead of this suite, so on CI the new bats file is not reached at all right now. It is not failing, it is not running. The helm-unittest cases are unaffected and do execute. ### Screenshots None. This changes a Job manifest and its tests, with no user-visible surface. ### Downstream repositories Walked the trigger map against the diff. The diff is one Job manifest inside the tenant chart, its helm-unittest suite, and a new `hack/*.bats` file. No package is added, renamed or removed; no `values.schema.json`, `values.yaml` default or version enum changes; no `ApplicationDefinition` semantics, namespace, variant or release asset changes; nothing under `hack/` is moved or renamed and no make target changes behaviour, since `bats-unit-tests` already globs `hack/*.bats`. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(tenant): bound the pre-delete cleanup hook's HelmRelease deletions and give its Job a wall-clock deadline, so a tenant whose children cannot finalize no longer leaves the hook waiting indefinitely and blocking the uninstall ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Tenant cleanup jobs now enforce execution deadlines and terminate gracefully. * Cleanup operations have bounded deletion timeouts and no automatic retries. * Failed cleanup steps now stop subsequent operations and correctly report failure. * Cleanup jobs now specify CPU and memory resource requirements. * **Tests** * Added comprehensive coverage for cleanup behavior, timeouts, failure handling, and job lifecycle settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
954be15341 |
fix(e2e): bound every read in the report collector and the step that runs it (#3596)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does A single hung read in `hack/cozyreport.sh` did not truncate `cozyreport.tgz`, it lost the artifact outright. The collector writes its tarball on its last line, the `Collect report` step carried no `timeout-minutes` of its own, and `Upload cozyreport.tgz` is the step directly after it — so a read that never returned spent whatever was left of the job budget, the job was cancelled, and the upload never ran. The runs where the report is the only evidence left are the runs that produced none. `|| true` guards against a non-zero exit, not against a read that does not return. Two halves, independent of each other. **The step gets a ceiling of its own**, 30 minutes, in all three workflows that run it. The number is measured against the step rather than divided out of the job: pulling the `Collect report` timings out of recent e2e runs gives 3 to 42 seconds, on failed jobs as much as on green ones, and 30 minutes leaves five sixths of the job budget for the steps that upload what this one produced. It carries `continue-on-error: true` beside it, because the step's `|| true` is its standing statement that collecting diagnostics is never fatal and a step killed at its ceiling never reaches that `|| true` — without the tolerance, a passing run whose collection ran long would be published as failed, and every later `failure()` in the job would start meaning "the tests failed or the report was slow". That tolerance also renders the overrun green, so a following step turns the collect step's `outcome` into a `::warning::` annotation: a bound that fires is recorded, and the outer bound is not exempt from the rule the inner ones follow. **Every read in the collector is bounded**, against four sections before, and gates and host commands are reads too. An `if kubectl get deploy …` that decides whether a module runs costs more unbounded than anything it guards, because the module below it never starts; `df -h` blocks indefinitely on a stale mount and takes the tarball with it by exactly the mechanism above. A gate cut off short is recorded, since skipping the module leaves the same empty tree as a cluster that never ran the component, while a gate answering "not installed" is recorded nowhere — that is the cluster answering correctly, and a marker firing on every install without KubeVirt is one a reader learns to skip. The six LINSTOR reads run a command inside a pod and go through a reader of their own. They are the only reads whose duration a workload sets rather than the apiserver, and `kubectl exec` returns the remote command's exit status — a 124 there is either the collector's deadline or a command that itself exited 124, so the existing classifier would have named this script's own timeout as the cause of something that may have happened inside a pod. The new note states what the status leaves open, and sits beside the output rather than inside it, since two of the six stream a gzipped tarball. A cut-off bundle is kept rather than dropped, and the note says so: the emptiness check at those call sites asks whether the archive lists a first member, which a truncated gzip does. What this does not do is bound the report. The pod, VM, VMI and services walks carry a count cap and a time budget; every other per-object walk carries neither, so their cost is the cluster's to set, and a failed install with a hundred not-Ready HelmReleases spends four hundred bounded reads in one of them. A per-read ceiling removes the read that never returns, not the thousand reads that each return slowly. The file header says so where it previously implied the opposite, and the step ceiling is what covers the rest. Two frozen lists in the tests are gone, replaced by one invariant derived from the code: no read below the library guard runs without a ceiling. Both lists were already wrong. The header named four bounded sections while the file had ten, and the per-section counts of unbounded reads only ever matched a redirect into `$VAR/`, which no gate, selector or command substitution has — so they described the scan rather than the file. The guard that held the old exec assumption was satisfied by leaving every exec unbounded, which is the state the file was in. It now also requires each exec to be bounded, and requires that reader not to borrow the classification written for reads whose 124 is unambiguous. Matching on the verb rather than on the string `kubectl exec` is part of it: half of these calls spell it `kubectl -n cozy-linstor exec`, which that string never saw. Two properties worth knowing before this lands. `kubectl get events -A --sort-by=.lastTimestamp` sorts client-side, so kubectl buffers the whole event list and writes nothing until it finishes — under a flat per-read ceiling a cut-off there leaves a file holding only the marker, not a short tail. That read had no ceiling at all before, where the same slowness could take the whole tarball, so this is a narrower failure rather than a new one. And `kubectl logs -n cozy-kamaji deployment/kamaji` gains `--tail=2000`, matching every other controller log in the file: it was the only unbounded-in-size log, which under a wall-clock ceiling is the one most likely to be cut at its *end*, where a control-plane bootstrap failure sits. Longer kamaji history than 2000 lines is no longer collected. Two accepted imperfections in how the change is split across commits, named rather than left to be found. `docs/agents/e2e-testing.md` keeps this material in a single shared paragraph that all three commits touch, so in the middle commit one sentence about the module gates lands a commit ahead of the gates themselves. And two corrections to the exec reader — it now keeps the remote command's whole message in the listing rather than quoting the line `kubectl exec` always appends last, and its cut-off note names both of the two things that can happen to a partial bundle instead of asserting one — sit in the last commit rather than in the one that introduced that reader. Both are true of the branch tip, where every statement is correct and appears once; the cost of moving them is another rebase across the one shared paragraph, which is not worth what it buys. Refs #3568. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(e2e): every read in the diagnostic report collector now carries a wall-clock ceiling — module gates, the reads that run a command inside a LINSTOR pod, and the host commands included — and a ceiling that fires is recorded in the artifact rather than only in the job log. The workflow step that runs the collector carries a ceiling of its own, so a read that does not return costs one file and a warning annotation instead of consuming the job budget and losing cozyreport.tgz entirely. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added bounded diagnostic collection across Kubernetes, Talos, LINSTOR, host, controller, and command outputs. - Reports preserve partial results and identify timeouts, failures, missing output, and truncated artifacts. - Added warnings when report collection fails or exceeds its time limit. - **Bug Fixes** - Corrected resource targets, selectors, deployment extraction, and command matching. - Improved handling for empty selections, unavailable resources, and missing timeout tools. - **Documentation** - Expanded guidance for diagnostic markers, collection limits, timeouts, and incomplete output. - **Tests** - Added coverage for bounded reads, failure reporting, markers, selectors, logs, and subprocess mocking. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0b7a8f67d1 |
fix(apps/kubernetes): report the pre-delete cleanup outcome honestly (#3592)
## What this PR does The pre-delete cleanup Job for the `kubernetes` app ended with an unconditional `echo "Cleanup completed successfully"`. Several of its steps swallow a failure behind `|| echo "WARNING: ..."` so that `set -e` cannot end the run, which means that line was printed in exactly the situation those warnings exist to report: child HelmReleases still holding Flux finalizers, or a Kamaji datastore Secret still pinning its namespace. Someone reading the Job log to find out why a namespace will not delete was told the cleanup had worked. The Job now records what a step left behind and closes with either the success line or the list of steps that left something. Only residue gets recorded. A bounded delete that times out has a remedy right behind it (Step 2 falls through to force-clearing finalizers, Step 4 falls through to Step 4b), and whether anything survived is decided by that remedy, which reports its own outcome. A tenant with no working nodes takes that route as a matter of course, which is the reason the wait is bounded at all, so recording the timeout itself would report a finished teardown as incomplete on the commonest path there is and send an operator hunting for objects that are not there. That is the same defect as the one being fixed, pointing the other way. A read that fails looks exactly like one that found nothing. An empty list and a failed list both give a loop nothing to iterate; an absent object and an unreachable apiserver both make a probe non-zero. So the whole script was walked for that shape instead of patching the instances as they were reported. Four reads reach the closing line. Step 2's force-clear re-list now keeps its status and reports when it cannot list what it was about to clear. Step 4b's two probes became three-way, present, absent or unknown, so a Secret whose existence could not be established is no longer announced as absent. Step 1's list is the one left swallowing its status, and that is deliberate: Step 2 deletes by label rather than from that list, so a suspend that never happened leaves nothing of its own behind, and reporting it would name residue that does not exist. Making the TenantControlPlane probe three-way also repairs the gate above it, and that is intended. The gate is there so the datastore Secret's finalizer is not stripped while Kamaji may still own it, and a two-way probe defeated it in the one direction that does damage: any error read as "confirmed absent" and the strip went ahead. Its likeliest cause, a throttled or briefly unreachable apiserver, is also likeliest exactly when a broken tenant is being torn down. Step 4b declining to strip a finalizer while the TenantControlPlane is still alive is the one residue that no failing command marks: every call in that branch succeeds, and the namespace stays blocked regardless. It now reports, and names the TenantControlPlane as the thing holding it up. What this PR does not do: the hook still exits zero, even with every backstop engaged. That is the difference between a main path and a backstop. `packages/apps/tenant/templates/cleanup-job.yaml` is the same kind of hook and handles it the opposite way, correctly: deleting the HelmReleases is that hook's whole purpose, so a failure there means the cleanup did not happen, and `set -e` ending the run before the success line is honest. The swallowed sites here are backstops. They run only after the normal deletion has already timed out, and they exist to push teardown further along. Failing there aborts the Helm uninstall before the release is removed, so the release stays in storage and the same doomed teardown is retried forever, at precisely the moment the backstop was written for. The steps that are not backstopped still end the run under `set -e` and never reach the closing message at all. This makes the message truthful. It does not make it reachable. The Job carries `helm.sh/hook-delete-policy: hook-succeeded`, and the hook exits zero by design on every backstopped run, so Helm reaps the Job, and its pod log, right after the hook completes. Whether the closing line survives long enough to be read is a question about the delivery channel, not about its wording, and changing that channel (an Event on the CR, or keeping the Job when something was left behind) has a different blast radius. Left for a separate change. The step comment and the runtime warning both cited the tracking issue for the Kamaji datastore-secret leak as though it were live. It is closed, and closed by this step rather than by a fix in Kamaji, so following the pointer showed a resolved report and no reason for the code to exist. The comment now says the ordering it covers is still reachable, and the warning names the finalizer and the object to clear by hand instead of an issue number. The workaround itself is untouched: nothing indicates the upstream cause has gone away. `hack/kubernetes-pre-delete-hook.bats` renders the Job, extracts the script and runs it against a stub `kubectl`, covering a clean run, a fallback route that succeeded, a TenantControlPlane outliving Step 4, each probe that could not answer, every remedy failing at once, an unbackstopped step ending the run, and the zero-exit invariant. The helm-unittest cases match the script's source text and cannot tell whether the closing message is reachable. Every case was checked by mutation, including reinstating the unconditional success line and reinstating the over-reporting the fallback case guards against. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change ### Release note ```release-note fix(kubernetes): the pre-delete cleanup hook no longer reports success when a cleanup step left something behind, and no longer reports a completed teardown as incomplete when it merely fell back to force-clearing finalizers. The Job log now names the steps that left residue. The hook still exits zero so the Helm uninstall can remove the release. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Kubernetes cleanup handling during application deletion. * Cleanup now distinguishes unavailable resource checks from confirmed absence, avoiding unsafe finalizer removal. * Reports leftover resources when recoverable cleanup steps fail while still completing the deletion process. * Improved handling of child releases, control-plane resources, and datastore secrets. * **Tests** * Added comprehensive coverage for successful cleanup, partial failures, resource residue, probe errors, and unrecoverable steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |