admin/cozystack
1
0 Fork 0
mirror of https://github.com/cozystack/cozystack.git synced 2026-11-03 10:37:21 +09:00
Code Issues Packages Projects Releases Wiki Activity
5,813 Commits 540 Branches 294 Tags
kafka-multi-instance
Commit Graph
117 Commits
Author SHA1 Message Date
Aleksei Sviridkin ad62a8d62b test(e2e): stop blocking merges on the tenant node-join deadline 
The tenant node-join wait no longer discriminates. Nested
virtualisation on the shared runners degrades far enough that a guest
which registers in minutes on real hardware can miss any deadline this
test can afford, so a red on that one wait says as much about the
machine the run landed on as about the product, and it teaches a
reader to re-run the job.

The suite still fails on it. That is what keeps Chainsaw's catch
running: the previous-instance container logs, the host snapshot, the
data-plane capture and the event dump are all keyed on the test
failing, and the report stays honest. What changes is one layer up.
The wait writes a marker beside its diagnostics, and a new lane-side
gate reads that marker to decide whether the job blocks. It opens only
when every failed test in the run carries one, and blocks on anything
it cannot attribute: a report it could not read, a failed test the
report does not name, a failure with no marker.

Only the lanes that block a merge ask it. The two pull-request lanes
do; nightly and the tag lane run the same suites and stay hard. A soft
pass on the tag lane would ship a release candidate whose tenant
clusters brought up no worker nodes, and nightly is the only place an
e2e failure reaches anyone who did not open a pull request, since no
lane in this tree notifies anywhere.

A softened red finishes the job green, so the job's result stops
answering whether the suite passed, and both kinds of consumer are
handed the decision instead of inferring it. A step gated on the job
failing would silently stop running: the SSH breakpoint is that case,
and it still opens for a pull request marked for debugging. A job that
reports the result would silently start reporting a pass: the required
commit status now says the deadline was missed and that the merge is not
blocked on it, rather than that the suite passed.

The deadline goes to 29m. That is the test's own budget and nothing
more: its clock starts after the machinedeployment wait, the
LoadBalancer assignment and the healthz probe, while the chart's
autoscaler and health-check timers have been running since the Machine
was created, so no ordering against them is claimed. The chart's 20m
and 30m give a scale to pick against, and a budget over the first
leaves room for a remediation cycle without promising that one fits.

The operation ceiling both suites give the script rises with the
deadline, and the diagnostics phase budget rises inside it, because
the collectors gated ahead of the serial console cost more between
them than the old budget covered. Both budget guards read their terms
from the sources that set them. The e2e job cap rises with them, and
its guard holds what a guard can hold: the pair's operation and catch
ceilings, and one figure across every lane that runs the suite. It
also records that the suites' full summed ceilings have never fit any
cap in that file, so the margin above the bound is taken from a
measured run instead. The prose that restates the cap elsewhere moves
with it, and the sweep that holds the deadline's restatements now
reads the figure spelled in words as well as in minutes.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-21 12:52:42 +03:00
Aleksei Sviridkin 9ccb133b66 fix(ci): de-escalate select-e2e and name every escalation (#3817) 
## Why

I replayed the 150 most recently merged pull requests through
`hack/select-e2e.sh` with every escalation branch instrumented. 118 of
them (78.7%) ran the full 21-suite Chainsaw run, and the causes, counted
once per escalating PR with the count where that cause was the only one
in brackets, were `FULL_PATTERN` 88 [68], `NO_SUITE_FOR_GROUP` 24 [24],
`CHAINSAW_SHARED` 18 [1] and `UNCLASSIFIED` 7 [4]. `YQ_BROKEN`,
`NO_GRAPH_OWNER` and `BACKSTOP_EMPTY` never fired.

Two problems come out of that. Most of the escalation is real but
unnecessary. Whole classes of path that provably cannot affect a
Chainsaw suite were escalating because no rule claimed them. And the
commonest cause of a full run was absent from the log, so "why did this
pull request run everything" could only be answered by re-deriving the
selection by hand.

## What changes

**Every escalation now names its cause on stderr.** Seven branches could
reach the full suite and four did it in silence, `full_suite_pattern`
among them, the commonest cause by a wide margin, so the usual answer
was the one the log never carried. The lines go to stderr and must stay
there: stdout is the suite list and both e2e lanes parse it, so a reason
line on stdout would be read as a suite name. A unit test asserts every
message, verified by muting each of the eleven in turn and confirming
the file goes red for all of them. A reason that regresses to silence
changes no selection and is otherwise invisible.

**`cozystack.etcd-application` gains its missing `dependsOn:
cozystack.etcd-operator`.** `packages/extra/etcd` renders `kind:
EtcdCluster` from `etcd-operator.cozystack.io/v1alpha2`, and this was
the only operator-backed application source in the tree with no edge to
its operator. It is a latent install bug as much as a selection gap:
without the edge `etcd-rd` registers the ApplicationDefinition as soon
as the engine is up, so a tenant can create an `Etcd` before the
operator exists and its HelmRelease fails on `no matches for kind
"EtcdCluster"`, and `hack/select-install.sh`'s forward closure for the
`etcd` suite omitted the operator for the same reason. On the selection
side `cozystack.etcd-operator` reached no runnable suite, so every
change to the operator ran all 21.

**`vm-disk-application` maps to the `vminstance` suite.** It resolved to
a `vm-disk` suite that does not exist, `intersect_suites()` dropped the
name, and the source reached nothing runnable. Neither did
`cozystack.kubevirt-cdi` above it, whose only other dependents are
`vm-default-images` and `vm-disk-application` itself, so every CDI
change ran all 21. The coverage was there and the table did not know it:
the `vminstance` suite creates a `VMDisk` and asserts the `DataVolume`
behind it (`hack/e2e-chainsaw/vminstance/vmdisk.yaml`,
`vmdisk-vmi.yaml`). Same defect class as the etcd edge above, on the
mapping side rather than the graph side, and with no production blast
radius of its own, since `src_to_suites` is read by the selector and by
nothing else.

**`cozystack.cozystack-basics` joins `cozystack.cozystack-engine` as a
propagation hub excluded from the reverse-dependency walk.** Every edge
into it exists so a namespace or a platform-wide policy is in place
before the dependent installs, which `kubevirt-cdi` states in its own
source ("Depend on cozystack-basics so the target namespace exists
first"), and that is install ordering rather than behaviour. The damage
runs opposite to the engine's: the engine fans one change out to every
app, basics narrows instead. It reaches no suite today so it escalates,
but it sits upstream of `kubevirt-cdi`, so the first suite to land under
CDI silently converts the platform's namespace-and-policy package from
the full run to that one suite. Not hypothetical — on #3426 enabling the
site-router suite takes `cozystack-basics` from 22 suites to
`site-router` alone, and nothing in the output says coverage was lost.
Dropping the reverse edges keeps the package reachable, stops it
propagating, and leaves a change to it running everything through the
per-path escalation.

**`hack/e2e-apps/<name>.bats` maps to the `<name>` suite**, off the
basename, exactly as the per-suite rule takes the name off a
`hack/e2e-chainsaw/<app>/` directory. Deliberately mapped rather than
marked inert: what remains there is wired to nothing after the Chainsaw
migration, and inert would bake that orphan status into the rule and go
quietly wrong the day a lane runs those files again. The basename is
membership-tested against the suite list on the spot, so an unmatched
one escalates there and then rather than having the verdict deferred to
the final intersection, where the rest of the diff would decide it.

**`packages/tests/` and three `.gitattributes` join
`inert_config_pattern`,** which is what the script's own header says to
do with a genuinely inert path instead of widening the fall-through.
`packages/tests/` is a helm-unittest fixture chart, and changing a test
*of* `cozy-lib` does not change `cozy-lib`, no PackageSource lists those
paths as a component, and nothing installs them, while a change to the
library itself still escalates through `packages/library/`. The
`.gitattributes` entries are enumerated rather than matched by filename,
because the justification is what those files contain (only
`linguist-generated` markers) and the name does not carry it:
`.gitattributes` can also set `filter`, `eol`, `working-tree-encoding`
and `export-subst`, each of which changes what lands in the working tree
and therefore what gets built.

**`full_suite_pattern` escalates only `hack/e2e-*.bats`, not every
`hack/*.bats`.** The root `Makefile` is the authority on the split and
draws it at exactly that prefix, `BATS_UNIT_FILES := $(filter-out
hack/e2e-%.bats,$(wildcard hack/*.bats))`, so the 60 files it keeps are
the unit lane and the e2e sandbox runs none of them, while the three it
filters out are what `packages/core/testing`'s recipes execute and those
still escalate. Being inert here does not leave them untested: `make
unit-tests` is gated on the `plan` job's `code` output, which
`pull-requests.yaml` computes as "any changed path outside `docs/`" and
never from `select-e2e.sh`, so a bats-only pull request still runs the
whole unit lane, plus install and the OpenAPI tests, since the `e2e` job
reads the same output rather than the selection.

## Measured effect

The before-and-after comparison below is a second measurement over a
slightly different population, stated separately because the two are not
interchangeable: it replays the last 150 first-parent merge commits on
`main` (`git diff --name-only $sha^1 $sha`) through the base selector
with base sources versus this branch with its own, where the histogram
above walks the 150 merged pull requests as GitHub lists them. The
populations overlap heavily and the direction is the same, and the
totals differ by two commits.

| | full suite | scoped | nothing |
|---|---|---|---|
| before | 120 | 20 | 10 |
| after | 106 | 25 | 19 |

Full-suite rate 80.0% → 70.7%, verdict changed on 15 commits. Fourteen
de-escalated and I read every one of their file lists: they are
unit-lane bats files, non-e2e workflows, `docs/`, `CODEOWNERS`,
helm-unittest fixtures and linguist markers. The fifteenth gets one
suite *wider*: a `cert-manager` change now also selects `etcd`, which is
correct, since `cert-manager` is a dependency of `etcd-operator` and the
etcd app now genuinely reaches it through the new edge.

That replay predates the two reachability rules, and replaying the same
150 first-parent merges across those two on their own — the selector at
`73bc4c0ae` against the selector at `adf231513`, same sources on both
sides — changes no verdict at all. Two of the 150 touch
`cozystack-basics` and both ran the full suite before and after, which
is the hub exclusion keeping something true rather than failing to do
anything, and none of the 150 touch `kubevirt-cdi` or `vm-disk`, so the
mapping shows up only in a direct selection.

A few individual selections, before → after:

| changed files | before | after |
|---|---|---|
| `hack/select-e2e_test.bats` | 21 suites | *nothing* |
| `hack/select-e2e_test.bats` +
`packages/apps/redis/templates/redis.yaml` | 21 suites | `redis` |
| `hack/e2e-install-cozystack.bats` | 21 suites | 21 suites |
| `packages/tests/cozy-lib-tests/**` | 21 suites | *nothing* |
| `packages/system/.gitattributes` | 21 suites | *nothing* |
| `hack/e2e-apps/postgres.bats` | 21 suites | `postgres` |
| `packages/system/etcd-operator/values.yaml` | 21 suites | `etcd` |
| `packages/system/kubevirt-cdi/values.yaml` | 21 suites | `vminstance`
|
| `packages/apps/vm-disk/values.yaml` | 21 suites | `vminstance` |
| `packages/system/cozystack-basics/values.yaml` | 21 suites | 21 suites
|

## Blast radius of the `dependsOn` edge

A `dependsOn` on a PackageSource is an install-ordering edge in
production, not only a test-selection hint, so this is the part worth
reviewing hardest. Both Packages are emitted by
`packages/core/platform/templates/bundles/system.yaml` under the single
`bundles.system.enabled` guard with no intervening conditional between
them, in every variant, so no supported configuration has the app
without the operator. `hack/select-install.sh --validate` reports the
graph still has no cycle and nothing dangling. The one new exposure is
an admin who lists `cozystack.etcd-operator` in
`bundles.disabledPackages` while keeping the app, which now leaves it
`DependenciesNotReady`. That is the same exposure postgres, mariadb,
kafka and redis already carry, with `Package.spec.ignoreDependencies` as
the escape hatch. On upgrade `etcd-rd` waits for the operator's two
HelmReleases, and an unhealthy `etcd-operator` already means broken etcd
apps.

## Testing

Every rule added or changed has a test in `hack/select-e2e_test.bats`,
the install-side change has one in `hack/select-install_test.bats`, and
the new graph edge has a helm-unittest guard at
`packages/core/platform/tests/sources_etcd_application_dependson_test.yaml`.
The round-trip test that walks every suite through both mapping tables
stays green, `vminstance` included now that two sources map onto it.

The two reachability rules were each checked by removing the rule and
confirming its own test reddens: without the mapping the CDI test falls
back to the full-suite escalation, and without the hub exclusion
`cozystack-basics` selects `redis vminstance` instead of everything. The
hub test has to seed its own hazard, since no suite sits downstream of
basics in this tree yet, so it gives `redis-application` an edge onto
basics in a copied sources directory and pins that a redis change still
selects redis, or the assertion would pass against a graph that resolved
nothing.

`make bats-unit-tests` aborts on the first failing file (`for f in …; do
… || exit 1; done`), and `hack/ghcr-mirror_test.bats` fails on `main`
today and sorts before `select-e2e_test.bats`, so the aggregate target
never reaches these tests. I ran the 60 unit files individually instead:
59 pass, and the one failure is that pre-existing one, byte-identical to
the base commit and referencing nothing this branch touches. Also green:
`make helm-unit-tests`, `make rd-presets-check migrations-target-check
test-check-readiness`, `hack/select-install.sh --validate`, and
`pre-commit run --all-files` leaving a clean tree.

## Two things a reviewer should know, neither addressed here

`assert_selection` in `hack/select-e2e_test.bats` aborts the whole file
with `exit 1` rather than failing a single test. That is the only thing
that works under `hack/cozytest.sh`, which provides no real `run`,
`$status` or `skip`, and it is the idiom the existing tests in that file
already use. It becomes wrong when #3497 and #3498 flatten these onto
real `bats(1)`, where a failing assert should end one test and let the
rest report. Worth converting with that work rather than ahead of it.

`cozystack.mongodb-application` carries the identical missing operator
edge: `packages/apps/mongodb` renders `psmdb.percona.com/v1` while the
source depends only on `cozystack.networking` and
`cozystack.cozystack-engine`, so `cozystack.mongodb-operator` still
reaches no runnable suite and every change to it escalates to all 21.
Same defect class and same fix shape as the etcd edge, but it is a
second production install-ordering change and belongs in its own pull
request.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- The default etcd application now includes its operator and required
platform dependencies, ensuring the EtcdCluster provider is available.

- **Bug Fixes**
- Improved end-to-end test selection for application-specific changes,
shared components, inert paths, and unresolved or unclassified files.
- Full-suite escalations now provide clear reasons through standard
error.

- **Tests**
- Expanded coverage for test selection, dependency reachability, and
etcd application dependency declarations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-08-20 14:12:56 +03:00
Aleksei Sviridkin 467e431765 chore(ci): add area/cilium label and map the cilium PR scope to it 
Cilium has enough open issues of its own (version pins and bumps, the
Gateway API implementation, network policy, IPAM next to kube-ovn,
LB-IPAM) that area/networking no longer narrows a search. Declare
area/cilium in labels.yml, route the cilium scope of PR titles to it
instead of area/networking, and update the scope table in the
contributing guide.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
 2026-08-19 16:28:50 +03:00
Aleksei Sviridkin b76cb2091c test(e2e): record the runner VM shape and region in the e2e report 
An e2e lane asks for a runner class, not for a machine. A flex shape is
filled from whatever capacity the pool has at the time, so one lane can
be served by two processor generations, and the region is a preference
with a fallback. None of that reached the report, so a red run and a
green one could not be compared by the hardware they ran on.

The read is taken on the runner VM, before the sandbox container is
created, because the metadata service answers on a link-local address
the container has no guaranteed route to: an "unreachable" answered from
in there would look exactly like a capture that had tried. It is wired
into the sandbox apply target rather than into a workflow step, so every
lane picks it up through `make prepare-env` on both the passing and the
failing path, and cozyreport folds the file into the artifact from
inside without going near the endpoint.

The keys are fetched one at a time and the root instance document never
is. That document carries the instance and compartment OCIDs, the image
OCID, the display name, the hostname and the user metadata bag, and the
report is a public artifact, so asking per key makes that property
something the wire settles rather than something a filter has to be
trusted for. It also matches where the shape sizing is published: it has
its own subpath and is not in the root document at all. From the sizing
object only the numbers are kept, so the rule holds for keys nobody has
seen yet, and an answer that is not one scalar is named rather than
printed.

Every read carries a wall-clock ceiling, since a read that never returns
costs the step its whole budget without reaching any exit status, and the
first key doubles as the probe so a machine that is not an OCI instance
pays one ceiling rather than one per key. Nothing here is fatal: this
runs inside the one retried step in the job, so a runner with no
metadata service leaves an "imds unreachable" line and an exit status
of 0.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
 2026-08-18 03:31:42 +03:00
myasnikovdaniil 9f5550abdd fix(release): publish rewritten packages during RC promotion (#3795) 
## What this PR does

Fixes #3477.

- Publishes a new packages OCI candidate after rewriting RC image tags
to stable tags.
- Pins the candidate digest in the stable installer without rebuilding
container images.
- Verifies the candidate against the prospective merge and original RC
artifact before creating stable names.
- Refuses promotion when the target maintenance branch lacks the
candidate-aware pipeline.
- Protects candidates referenced by open release PRs from retention
cleanup.
- Leaves already published v1.6.0 and v1.6.1 unchanged.

### Screenshots

Not applicable.

### Downstream repositories

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Testing

- `bats --no-tempdir-cleanup hack/promote-packages-artifact_test.bats
hack/verify-promoted-packages_test.bats
hack/promote-packages-contract.bats
hack/promotion-retention-contract.bats`
- `actionlint .github/workflows/promote-rc.yaml
.github/workflows/pull-requests.yaml
.github/workflows/pull-requests-release.yaml
.github/workflows/retention.yaml`
- `sh -n hack/promote-packages-artifact.sh
hack/verify-promoted-packages.sh`
- `git diff --check origin/main...HEAD`

### Release note

```release-note
fix(release): publish the rewritten stable packages artifact when promoting a release candidate
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Release candidates now use temporary, immutable package artifacts
verified before stable publication.
- Stable promotions validate package contents, installer references,
container digests, and release metadata before tagging.
- Temporary promotion artifacts are retained while needed and cleaned up
after expiration.

- **Bug Fixes**
- Improved handling of release labels, fork pull requests, and promotion
pull requests.
  - Added end-to-end verification before release promotion.
- Prevented invalid, ambiguous, or untrusted package references from
being promoted.

- **Documentation**
- Updated release and promotion guidance for candidate verification,
digest pinning, and transactional promotion.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-08-17 12:52:19 +05:00
Myasnikov Daniil adf231513f fix(ci): correct two reachability defects in the selection graph 
Both are cases of the walk reaching the wrong set of suites for a
package, and both were found by enabling the site-router suite on #3426,
which is the first suite in the tree to land downstream of kubevirt-cdi.

`vm-disk-application` now maps to the `vminstance` suite. It mapped to a
`vm-disk` suite that does not exist, which intersect_suites() drops, so
the source reached nothing runnable and neither did kubevirt-cdi above
it: every CDI change ran all 21 suites. The vminstance suite creates a
VMDisk and asserts the DataVolume behind it
(hack/e2e-chainsaw/vminstance/vmdisk.yaml, vmdisk-vmi.yaml), so it is the
suite that covers both. A CDI change now selects `vminstance` alone.

`cozystack.cozystack-basics` joins `cozystack.cozystack-engine` as a
propagation hub excluded from the reverse-dependency walk. Every edge
into it exists so a namespace or a platform-wide policy is in place
before the dependent installs -- kubevirt-cdi says exactly that in its
own source -- which is install ordering, not behaviour. The damage runs
opposite to the engine's: the engine fans one change out to every app,
while basics narrows down instead. It reaches no suite today so it
escalates, but it sits upstream of kubevirt-cdi, so the first suite to
land under CDI silently converts the platform's namespace-and-policy
package from the full run to that one suite. On #3426 that is 22 suites
down to `site-router` alone, with nothing in the output to say coverage
was lost. Dropping the reverse edges keeps the package reachable, stops
it propagating, and leaves a change to it running everything through the
per-path escalation.

Both rules have a test, each verified to redden with its rule removed:
the mapping test falls back to the full-suite escalation, and the hub
test -- which seeds the hazard by giving redis-application an edge onto
basics, since no suite sits downstream of it in this tree yet -- selects
`redis vminstance` instead of everything.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
 2026-08-16 10:44:44 +05:00
Aleksei Sviridkin cf2d9c6d13 test(e2e): split the tenant worker CPU time across QEMU threads 
The node-join diagnostics phase samples the tenant worker's CPU
counters twice on failure, but it reads them from the compute
container's cgroup, and that cgroup holds every thread QEMU runs:
the guest's vCPUs, QEMU's main thread, and its IO and worker
threads. A container burning most of its quota while the guest
reports no progress is two incompatible findings wearing one
number, and no container-level counter separates them.

Add a collector that execs into each Running worker's compute
container, finds the QEMU process and copies every
/proc/<pid>/task/<tid>/stat line into the artifact verbatim, with a
legend beside them. Nothing is computed here. The legend names
where utime and stime sit and why counting fields over whitespace
reads the wrong column for exactly the threads the split is about:
a KVM vCPU thread is named "CPU 0/KVM", that space is inside the
comm parentheses, and every field after it shifts by one.

It runs inside the sampling loop the two existing subjects already
share, so it costs no second wait: both readings ride the interval
already being paid, and the rate comes from the two files plus the
stamps each one carries.

Every way the read can come up short is a named outcome rather than
an empty directory, and the legend and the pairing instruction are
withheld from each of them: telling a reader to subtract two files
asserts that both hold a whole reading. The probe shells out to
nothing but getconf, whose absence it names, because a probe that
needed cat would report a container missing coreutils as a
container whose QEMU had exited, and it counts what it managed to
read so that having read no process at all stays separable from
having read them all without finding QEMU. Its stderr is suppressed
on the walk rather than on either read: redirections are applied
left to right, so a 2>/dev/null after an input redirect is not in
place yet when opening the input fails, and a thread that exits
between the glob and the read would otherwise put a shell
complaint in the artifact beside a healthy capture. It also reports
a QEMU that was named and then gone before its task files could be
opened, because a process heading alone is enough to make the
capture non-empty and would otherwise carry a legend describing
columns the file does not have.

The Pod listing the cAdvisor captures already share now takes its
whole query as a parameter, so this collector reuses that read
rather than carrying a third copy of it, and asks for the Running
Pods only: a launcher left over from an earlier attempt costs the
sibling caller nothing and would cost this one a bounded exec and a
slot under its cap. The guard that holds the sampling group against
the room the tenant snapshot needs now counts every capped walk in
the loop rather than the two spelled the same way, so the new
subject is inside the arithmetic rather than beside it.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
 2026-08-16 05:17:16 +03:00
Aleksei Sviridkin 959e90c9ab test(e2e): time the guest console capture and baseline it on a green run 
A tenant worker's guest console is stamped in seconds since that guest's
own kernel started, so a log ending mid-boot says how far the guest got
and nothing about when. Whether it ends there because the guest started
late or because the guest stopped printing decides which failure is being
looked at, and neither instant was recorded: the read's own instant was
nowhere in the file, surviving only as the artifact's mtime, which is
metadata rather than content and so says nothing to anyone reading the
log; and the guest's start only inside a describe dump that a capture
with no silent console never triggers. Across three collected runs every
capture exited zero and every log began at the first kernel line, so the
reads were complete at both ends and the logs end where the guests
stopped -- and which of the two happened could not be read off any of
them.

Each console log now carries the instant its own read started and
returned, and CAPTURE-TIMELINE.txt beside it carries each Pod's phase,
its start time and the start of its compute and console containers, from
one bounded read taken once for the whole selector. Those subtract to how
long a guest had been alive when it was read, which is what the log's
last kernel stamp is then compared against. The console container is
asked for in both status lists, since KubeVirt runs it as an init
container and reading only one of them would report a container that
started fine as one that never ran. A read that failed, a read cut off
part way and a read that answered with nothing are recorded as
themselves rather than left as a missing file, and the note names the
error log only when there is one.

The same capture also runs on the passing path, before the tenant is torn
down and the Pod holding the stream goes with it. Every console this tree
has collected came from a failure, so what a healthy boot looks like has
never been measured, while "slower than usual" is the claim the failure
keeps resting on. That caller asks for the pool's minimum rather than its
maximum, since two workers booting is the same measurement as ten, and it
warns in the job log rather than failing a suite that has already proved
everything it exists to prove. Both paths write the same directory, so
the capture also records which of them produced it.

That capture costs real wall clock at the tail of an operation with a
fixed ceiling, which is a way for a run that passed to be killed while
collecting a debugging aid. So it is declined out loud when the ceiling
no longer has room for it, against a reserve derived from its own cost
and held in step with the timeout both suites give the operation.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
 2026-08-15 23:33:33 +03:00
Myasnikov Daniil c28d7804af fix(ci): enumerate the inert .gitattributes instead of matching by name 
The rule was broader than its justification. It read `(^|/)\.gitattributes$`,
so it claimed every present and future .gitattributes anywhere, while
the argument for it was about content: every one in this tree holds
nothing but `linguist-generated` markers, which reach no build, chart or
test.

The filename does not carry that argument. .gitattributes can also set
`filter`, `eol`, `working-tree-encoding` and `export-subst`, each of
which changes what lands in the working tree and therefore what gets
built, so a by-name rule would eventually make a live file inert with
nothing to notice it.

Nothing is lost by narrowing, because only three files ever reached the
rule: api/.gitattributes and internal/crdinstall/manifests/.gitattributes
are escalated by full_suite_pattern first, so entries for them would be
dead. Measured against the base selector, those three are exactly the
paths whose classification this branch changes, and they still select
nothing.

Same trade as the workflow enumeration a few lines up, and the same
upkeep, which the comment now states: after adding or moving one,
re-check with `git ls-files '*.gitattributes' '.gitattributes'` and
confirm the contents are still only linguist markers. A .gitattributes
not on the list is classified by whatever rule its path falls under --
the graph inside a package, the unclassified fall-through at the root --
and both fail safe. A test pins that root case, so a future widening
back to a by-name match turns the file red.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
 2026-08-15 17:58:08 +05:00
Myasnikov Daniil b2702621d3 fix(ci): decide the per-app bats rule per path, not after the merge 
Two defects in the hack/e2e-apps/ arm added by the previous commit.

The arm set trigger_any=1 for whatever basename it derived and left the
verdict to the final intersection. That makes the escalation depend on
the REST of the diff: hack/e2e-apps/monitoring-oidc-system.bats alone
emptied the selection and the backstop escalated, but the same file
beside packages/apps/redis/templates/redis.yaml selected `redis` with
nothing on stderr, where the pre-change selector ran all 21 suites.
That is the merge-before-escalate shape #3330 removed from the graph
walk, reintroduced one rule over. The basename is now
membership-tested against the suite list on the spot and an unmatched
one sets trigger_full=1 and names the file.

POSIX case matches `/` with `*`, so the arm also sees nested paths, and
the unanchored capture turned hack/e2e-apps/fixtures/postgres.bats into
the "suite" fixtures/postgres -- right outcome by accident, reason line
printing a path where a suite name belongs. The capture is anchored to
one segment, and what the arm does with anything that is not a
top-level <name>.bats is now stated rather than left to fall out of a
regex: it is shared material for those suites the way
hack/e2e-chainsaw/_lib/ is for the Chainsaw ones, so it escalates for
the same reason.

Both claims that said otherwise are corrected -- the arm's own comment
and docs/agents/e2e-testing.md both asserted the deferred escalation
was equivalent to the old fall-through, which held only for a
single-file diff. The backstop comment drops the e2e-apps clause it
gained last commit, since that route no longer reaches it. The header's
"four of the seven" becomes count-free: this commit adds two escalating
branches, so a hardcoded total was already stale.

Tests: the mixed-diff case is the regression pin (the isolated case
passes with the escalation deferred and cannot see the bug), plus a
nested-path case and a non-.bats case. Both fixes are
mutation-verified -- restoring either defect turns the file red.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
 2026-08-15 17:58:08 +05:00
Myasnikov Daniil 4d4f5edf92 fix(ci): escalate only the e2e bats files, not the whole unit lane 
full_suite_pattern took every hack/*.bats, so all 63 of them ran the
whole Chainsaw suite -- including the 60 that are the unit lane and
that the e2e sandbox never executes.

The root Makefile is the authority on the split and draws it at
exactly the prefix this narrows to:

  BATS_UNIT_FILES := $(filter-out hack/e2e-%.bats,$(wildcard hack/*.bats))

The 60 files it keeps run under `make unit-tests`; the three it
filters out -- e2e-prepare-cluster, e2e-install-cozystack,
e2e-test-openapi -- are the ones packages/core/testing's recipes
execute inside the sandbox, and those still escalate. So no Chainsaw
suite can regress from an edit to one of the 60.

Inert here is not a green gate with nothing behind it. `make
unit-tests` is gated on pull-requests.yaml's `code` output, computed
in the workflow as "any changed path outside docs/" and never from
select-e2e.sh, so a bats-only pull request still runs the whole unit
lane. The `e2e` job reads the same output, so it still installs the
platform and runs the OpenAPI tests before skipping Chainsaw.

Two tests: one pins the rule on both sides of the prefix and asserts
an inert bats file beside an app path does not mask the selection, and
one walks every hack/*.bats in the tree and requires the selector's
verdict to agree with the Makefile's split, so a file named to fit
neither lane surfaces here rather than on the next PR that touches it.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
 2026-08-15 17:58:07 +05:00
Myasnikov Daniil 33428671fa fix(ci): classify hack/e2e-apps/ and packages/tests/ in select-e2e 
Three path classes matched no rule in hack/select-e2e.sh and escalated
to all 21 Chainsaw suites through the unclassified fall-through -- 7
of the last 150 merged pull requests between them.

hack/e2e-apps/<name>.bats now maps to the <name> suite, off the
basename exactly as the per-suite rule takes the name off a
hack/e2e-chainsaw/<app>/ directory. Not marked inert: what is left in
that directory after the Chainsaw migration is wired to nothing, and
inert would bake that orphan status into the rule and go quietly wrong
the day a lane runs those files again. A basename no suite carries
drops out of the intersection at the bottom of the script and
escalates through the backstop, which is the outcome those paths
already had, so mapping optimistically loses nothing and gains the
correct answer for every name that is a suite.

packages/tests/ and every .gitattributes join inert_config_pattern,
which is what the script's own header says to do with a genuinely
inert path rather than widening the fall-through. packages/tests/ is a
helm-unittest fixture chart: changing a test OF cozy-lib does not
change cozy-lib, no PackageSource lists those paths as a component,
and nothing installs them -- a change to the library itself still
escalates through packages/library/. .gitattributes is matched by name
wherever it lives because every one in this tree is a
linguist-generated marker, which reaches no build, chart or test; one
under a tree full_suite_pattern already covers still escalates, since
that pattern is checked first.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
 2026-08-15 17:58:07 +05:00
Myasnikov Daniil 3c3f9450ae fix(ci): name the cause on every select-e2e escalation 
Seven branches in hack/select-e2e.sh escalate to the full Chainsaw
suite and four of them did it in silence: the shared
hack/e2e-chainsaw/_lib/ and .chainsaw.yaml case, the
full_suite_pattern match, a packages/ path no PackageSource claims,
and the empty-selection backstop.

"Run everything" is the same 21 suite names whichever rule produced
it, so a reason line is the only thing that tells the seven apart.
full_suite_pattern is the commonest cause of a full run by a wide
margin -- 88 of the last 150 merged pull requests, 68 of them with no
other cause -- so the usual answer to "why did this pull request run
the whole suite" was the one the log never carried, recoverable only
by re-deriving the selection by hand.

Each new line names the path that caused it rather than only the rule,
and the backstop names the directly-selected names it could not
resolve. They go to stderr, never stdout: stdout is the suite list and
both e2e lanes parse it, so a reason line there would be read as a
suite name.

A unit test asserts each of the four messages. A reason that regresses
to silence changes no selection, so it is invisible to every other
test in the file.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
 2026-08-15 17:58:07 +05:00
Aleksei Sviridkin ef96292b64 fix(ci): give the nightly e2e runner headroom over its guest vCPUs (#3833) 
## What this PR does

The e2e sandbox boots three QEMU nodes at `-smp 8`, so 24 guest vCPUs
are committed before any tenant VM exists. Four jobs boot that sandbox.
Three run on the 32-vCPU class; nightly ran on the 24-vCPU one, where 24
guests against 24 host vCPUs leaves nothing for QEMU's own emulation and
IO threads, containerd, the kernel and the runner agent. The other lanes
moved to the larger class at some point and nightly was not moved with
them.

The sandbox keeps its 24 vCPUs. That guest count is part of what e2e
claims to test, and a smaller nightly cluster stops matching the one PRs
run against.

This is hygiene, not a fix. Measured boot-phase data across 18 runs says
the load a workflow puts on the host dominates guest starvation, not the
runner class: the lanes already on 32 vCPU boot guests slower than
nightly did on 24, and class and workflow kind are fully confounded
there, so neither separates out. Do not read this as making anything
faster or as addressing a particular red run. What is left is the
arithmetic. A 1:1 commit has no headroom at all, and two lanes running
the identical sandbox on different classes is drift nothing was
watching.

The rest is a check, because the guest demand and the runner classes
live in different files and nothing compared them.
`hack/sandbox-runner-headroom.bats` reads `-smp` and `-m` times the loop
count out of `hack/e2e-prepare-cluster.bats`, reads the runner class of
every job that calls the sandbox make targets, and wants the runner
strictly larger on both axes. Neither side is a hardcoded list, so
bumping `-smp`, adding a management node, or adding a fifth lane on a
smaller runner turns it red on the same commit. Memory is in there
because the same argument applies to it and `-m 24576` sits on the same
line as `-smp 8`, not because a class that violates it exists today.

It stops instead of guessing when it cannot read something, since a
guess comes out smaller than the real demand and passes a runner that
cannot hold the sandbox. Refused: a missing or non-literal `-smp` /
`-m`, a launch written outside any `@test` block or inside one left open
at end of file, a `for` opener the strict pattern does not match, a
`for` list holding anything but literal words, a `while` sharing the
block, two launches or two loops in one block, and a `runs-on` with no
`oracle-vm-<n>cpu-<m>gb` in it. Unparsed loop openers are counted
separately from parsed ones, so moving `do` to the next line is refused
rather than silently multiplied by one. There is also a cross-check that
every workflow calling the sandbox make targets yields at least one
matched job, so a renamed target turns the parser red instead of quietly
shrinking what it covers.

The derivation runs against the live tree, which passes whether or not
those refusals still work, so each one is pinned by a fixture that feeds
a synthetic file to the same program. That matters more than it sounds:
two of the refusals above exist because the first version of this file
did not have them, and a launch moved into a file-level helper would
have halved the measured demand and stayed green.

The numbers come from:

```bash
sed -n '98,107p' hack/e2e-prepare-cluster.bats    # three nodes, -smp 8, -m 24576
grep -rn 'prepare-env' .github/workflows/         # the four jobs that boot it
grep -rn 'runs-on: oracle-vm-' .github/workflows/ # their runner classes
```

The rule also goes into `docs/agents/e2e-testing.md` next to the other
environment invariants, so whoever adds the fifth lane picks its runner
class from the docs instead of from a red unit suite.

Checked that it bites, twice over. Fourteen mutations of the inputs:
reverting the runner class, `-smp 11`, a fourth and fifth management
node, `-smp cpus=8`, `-m size=24576`, `do` on the next line, `$(seq 1
3)` as the list, a `while` loop, the launch moved into a file-level
helper, renaming the make target, `ubuntu-latest`, a matrix expression
in `runs-on`, a hypothetical `32cpu-64gb` class, and a new lane on a
4-vCPU runner. Then eleven mutations of the parser itself, one per
refusal, each of which kills exactly the fixture that names it and
nothing else. Green on this tree, and the same numbers out of gawk, mawk
and BSD awk.

Closes #3831.

### Screenshots

No UI change.

### Downstream repositories

Walked the trigger map against the diff. It changes one `runs-on` line
plus a comment in `.github/workflows/nightly.yaml` and adds one file
under `hack/`. The `ccp` trigger is moving or renaming something under
`hack/`, or changing what a make target does; `hack/package.mk` and
`hack/common-envs.mk` are untouched, and the new test is picked up by
the existing `hack/*.bats` wildcard without the target changing. The
`ansible-cozystack` and `talm` triggers are node prerequisites in
`hack/e2e-prepare-cluster.bats`, which this reads and does not modify.

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

```release-note
fix(ci): run the nightly e2e suite on a runner class larger than the sandbox's guest vCPU demand, and add a unit check that fails when an e2e lane loses that headroom
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Updated nightly end-to-end testing to run on a higher-capacity runner,
improving support for multiple QEMU guests and required resource
headroom.
* Added validation to prevent sandbox runners from being undersized or
oversubscribed.

* **Documentation**
* Documented the resource requirements and deterministic conventions for
end-to-end sandbox testing.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-08-15 12:59:55 +03:00
Aleksei Sviridkin 527d4ea00f test(e2e): measure tenant worker CPU as a rate, and the sandbox nodes under it 
Two instruments for the node-join failure the kubernetes-* suites keep
hitting. Neither changes what the suites assert.

The worker CPU capture now reads container_cpu_usage_seconds_total
beside the CFS counters, and reads them twice a fixed interval apart
instead of once. The throttled counters say a container met its ceiling
and cannot say how much CPU it got, which are the two candidate
explanations for a stalled worker: a guest held at a one-core ceiling,
and a guest whose vCPU thread is rarely scheduled at all. Both leave
throttled periods behind. Every counter involved accumulates from
container start, so a single reading is an average over the whole uptime
and no rate at all, and the profile under investigation is a guest that
freezes for tens of seconds and then runs flat out: the average comes
out low, the instantaneous figure comes out at the cap, and only the
second separates it from a guest that is simply capped.

The sandbox nodes are a new subject. The worker captures describe a
cgroup, so a guest that is runnable and never scheduled looks from there
like a guest that asked for nothing. Whether the sandbox node itself was
given its turn is recorded only in the steal column of its own
/proc/stat: no kubelet endpoint carries it, and crust-gather cannot
reach it because PodSecurity refuses its node-shell Pod here, so the
read goes over the Talos API, which is already how the report reaches
those nodes for dmesg. The rows are captured verbatim with their column
legend beside them rather than reduced to a percentage, because
/proc/stat has no header, steal and guest sit next to each other, and
guest is large on a node running VMs and already counted inside user.

Both subjects are read on either side of one shared wait, so the pair
pays it once. The wait is not the interval either subject's counters
span, though: each pass walks its own nodes and the other subject's pass
falls between a subject's two readings, and on the run this exists for
those passes are the slow part. Each capture therefore stamps both ends
of its read, so a rate comes from subtracting stamps rather than from
assuming the knob, and the width of those brackets is the uncertainty.
That wait is a measurement interval rather than a stand-in for a
condition nobody wrote a wait for, so the e2e conventions carve it out
by name instead of marking it as a fixed timeout to be replaced.

The pair is gated below the serial console rather than above it. Its
ceiling is sixteen bounded reads plus the interval, which is most of the
phase budget, and admission gates only when a collector may start, so
above the console a run that hit those bounds would leave the console
never started rather than merely cut short. The console is the only
capture that survives a worker which never reached apid, which is the
shape this failure usually takes. That overrun is inside the term the
budget derivation already allows for one late admission, and a guard
holds the pair's ceiling against it.

No collector here may leave an empty directory, since an empty directory
reads as a healthy node: a missing binary, a missing talosconfig, a
listing that never answered, a node with no address and a read that was
cut off each land as a named note or a COLLECTION-FAILED.txt.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
 2026-08-15 10:19:19 +03:00
Aleksei Sviridkin db2ddfab2b fix(ci): give the nightly e2e runner headroom over its guest vCPUs 
The e2e sandbox boots three QEMU management nodes at `-smp 8`, so 24
guest vCPUs are committed before any tenant VM starts. The nightly lane
ran that sandbox on a 24-vCPU runner, a 1:1 commit that leaves the host
nothing for QEMU's emulation and IO threads, containerd, the kernel and
the runner agent. The tag, fork and pull-request lanes already run the
same sandbox on the 32-vCPU class, so move nightly there too.

This aligns headroom and claims no performance effect. Measured boot
data shows the load a workflow puts on the host, not the runner class,
dominates guest starvation, and the two are confounded in that data; no
red run is attributed to the class here. The case is the arithmetic: a
1:1 commit leaves no headroom, and two lanes running the identical
sandbox on different classes is drift nothing was watching.

Add hack/sandbox-runner-headroom.bats to keep the two sides paired. It
derives the guest vCPU and memory demand from the sandbox bats file, and
the runner class of every job that drives the sandbox make targets, then
requires the runner to be strictly larger on both axes. Neither side is
a literal list, so raising `-smp`, adding a management node or adding a
lane on a smaller runner turns it red instead of drifting apart
silently.

Input the derivation cannot read in full is refused rather than guessed:
a guess produces a demand smaller than the real one, which passes a
runner that cannot hold the sandbox, while a refusal names the line.
That covers a launch written outside any `@test` block, a block left
open at end of file, a `for` opener the strict pattern misses, a
non-literal loop list, and a `while` loop sharing the block.

The derivation runs against the live tree, which passes whether or not
those refusals still work, so each one is pinned by a fixture feeding a
synthetic file to the same program. Loosening the parser turns a fixture
red rather than quietly widening what the check accepts.

Record the rule alongside the other environment invariants in
docs/agents/e2e-testing.md, so a new lane picks its runner class from
the documentation rather than from a failing unit suite.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-15 02:41:19 +03:00
Myasnikov Daniil a37a3178e5 docs(release): name all four sourcers of the image-ref enumeration 
The library header still claimed three call sites. There are four:
this PR made hack/verify-promoted-packages.sh source it to compare
the candidate's refs against the rc artifact's. The historical
drift story stays with the original three, since the verifier has
only ever read the shared enumeration.

The consumer table gained hack/promote-packages-artifact.sh under a
bullet requiring every consumer of references to read all three
shapes through the library, while the exception below it still named
only hack/overlay-main-images.sh. The publisher sources nothing and
needs nothing: it hands the whole tree to `flux push artifact` and
touches one reference, the installer's own self-pin, by key with yq.
Say that, rather than implying a gap it does not have -- the real
exception remains overlay-main-images.sh, which does read references
and does walk the tree itself.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
 2026-08-14 18:37:59 +05:00
Myasnikov Daniil f21d277eba fix(release): retain live promotion candidates 
Protect package candidates referenced by open release PRs while pruning abandoned run-specific promotion artifacts. Document the stable-candidate lifecycle and maintenance-branch rollout requirement.

Assisted-By: Claude <noreply@anthropic.com>

Assisted-By: GPT-5 <noreply@openai.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
 2026-08-14 18:37:57 +05:00
Aleksei Sviridkin 340483fdf0 fix(hack): declare the shared package targets and the root test target phony (#3353) 
## What this PR does

`hack/package.mk` set `.PHONY` with `=` instead of `:`:

```make
.PHONY=help show diff apply delete update image
```

That assigns a variable which happens to be named `.PHONY`; it declares
nothing. `make -p` prints it back among the variables (`.PHONY = help
show diff apply delete update image`) rather than as a special target,
so none of the targets `package.mk` defines was phony in any package
that includes it.

Nothing in the tree collides with those names today, so this is
preventive rather than a fix for present breakage. The failure it
forecloses is quiet: with a directory named `image` beside a package
Makefile, `make image` prints `make: 'image' is up to date.` and exits 0
without building. A file of that name is enough on its own where the
rule has no prerequisites; where it has one, the recipe keeps running
until that prerequisite is shadowed too.

The list changed as well. `suspend`, `resume`, `check` and `clean` are
defined in this file and were missing, and `check` is the one that
matters most, since it is the prerequisite that keeps `show`, `diff`,
`apply` and `delete` rebuilding. `update` and `image` are gone, because
`package.mk` does not define them; the including package Makefile does.
Naming an undefined target in `.PHONY` creates it as an empty target,
turning `No rule to make target` (exit 2) into `Nothing to be done`
(exit 0) wherever the rule is missing. That reaches CI too:
`hack/build-matrix.sh` parses the root Makefile `build:` target and runs
`make -C <pkg> image` per unit, so a package arriving in that list
without an `image:` rule would go from a red job to a green one that
builds nothing.

`.DEFAULT_GOAL` on the line above keeps its `=`. That one is a real
variable, so the assignment form is right there. A comment now says
which of the two lines is which, since they read alike and one of them
just changed shape.

`hack/package-mk-phony.bats` covers the declaration and is picked up
automatically by the `hack/*.bats` glob in `bats-unit-tests`. Five of
its six tests cover `package.mk`: that the declared names are really
phony with a file of each name sitting beside the Makefile; a control
that mutates a copy back to the assignment form and requires the probe
to report up-to-date, so the first cannot pass vacuously; that the
declared list and the defined targets are the same set, checked both
directions; and two pinning the `update`/`image` exclusion, one
requiring the loud failure to survive and one extending the list in a
copy and requiring it to disappear. The loud-failure test pins both the
exit status 2 and the diagnostic. A wrapper that kept make's wording
while returning something else would pass a check on the message alone.

One point decides where the guarantee actually lives. The first test
reads its names from the `.PHONY:` line, so it cannot see a name dropped
from that line at all. The set-equality test is what guards every name's
presence.

One known gap is left in place. The first test decides by matching
make's `is up to date`, which make prints only for a target that has a
recipe, and a recipe-less alias never draws it: with no prerequisite
both forms print `Nothing to be done`, and with a prerequisite that is
itself out of date, which every target in this file is for being phony,
the working form prints that prerequisite's recipe while the broken form
prints `Nothing to be done`. The probe separates neither pair, so such a
name would pass the first test either way. Measured on both shapes, not
assumed. Every target `package.mk` defines has a recipe, so nothing is
uncovered today; the control test does not close the gap either, since
an alias is a defined target and the control requires `is up to date`
from it, which turns it red against the probe rather than against the
alias. Worth closing if an alias is ever added.

One more for whoever edits the suite next: `hack/cozytest.sh` rewrites
every bare `}` at column 0 into `return 0` followed by `}`. That is
aimed at `@test` blocks, but it does not distinguish them from a plain
shell function, so a helper factored out of these tests would silently
become unable to return a non-zero status. Tests 1 and 2 repeat a loop
rather than share one for reasons of their own, but this is the trap
waiting for anyone who merges them.

Two properties of the suite are worth stating, because both are easy to
get wrong in the other direction. The set-equality test extracts the
defined targets with a pattern that accepts a rule naming several
targets at once, so `suspend resume: check` keeps both names in the set;
anchoring the colon to a single name drops every name on such a line,
and for a target nothing else depends on that goes unnoticed. And the
control test probes the targets `package.mk` defines rather than the
names on its `.PHONY:` line, because `is up to date` is only a
meaningful answer for a name that has a rule: a declared-but-undefined
name prints `Nothing to be done` in both the working and the broken
form, so probing it in the control fails against the probe, while the
set-equality test holds the message that names the defect.

Last one, about how the tests find the repo. All six read it as
`repo=$(pwd)`, which assumes the runner starts at the repo root. That
holds for `make bats-unit-tests` and for the invocation the header
documents, so nothing is broken, and the assumption is the ordinary
shape in `hack/` rather than an outlier: about half the bats files there
resolve repo-relative paths against the cwd the same way, several
stating it in their own headers, and this is not even the only one
spelling it with `$(pwd)`. What is worth naming is the failure mode:
started elsewhere, these tests go red with `no '.PHONY:' declaration
found in hack/package.mk`, which reads like a real finding rather than a
wrong directory.

The root `Makefile` had the same defect, and there it had already fired.
`test` was not on its `.PHONY:` line and a `test/` directory sits beside
it, so `make test` printed `'test' is up to date` and exited 0 without
running anything, while `docs/agents/overview.md` advertises that
command as the way to run the full suite. CI never used it: the workflow
calls `test-controllers` and drives e2e out of `packages/core/testing`,
so the cost fell on anyone following the docs by hand. One name on the
declaration closes it, and the suite gained a sixth test that goes red
without it. Every other undeclared target in that file is unshadowed
today, so none of them is reachable the way `test` was, and they are
left alone here. #3709 covers the same class in the four
`packages/core/` Makefiles, which include only `hack/common-envs.mk` and
so are out of reach of this change entirely.

### Downstream repositories

`scripts/package.mk` in cozystack/external-apps-example was a
byte-for-byte copy of `hack/package.mk` (identical sha256 before this
change), carrying the same inert `.PHONY=` line. This PR splits the two
files, and the copy was fixed separately. It landed the same nine names,
and `update` and `image` are absent there for the reason they are absent
here: `scripts/package.mk` does not define them either, and neither does
any Makefile that includes it. What separates the two files now is the
two-line comment added here, and nothing else.

- [ ] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [x]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up: https://github.com/cozystack/external-apps-example/pull/4
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

```release-note
NONE
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Fixed task declaration handling so supported commands are correctly
recognized and executed.
* Updated available commands by replacing `update` and `image` with
`suspend`, `resume`, `check`, and `clean`.
* **Tests**
* Added coverage to verify command declarations, prevent regressions,
and ensure invalid or missing commands report errors correctly.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-08-14 10:54:10 +03:00
Aleksei Sviridkin 59878850f9 test(e2e): capture worker CPU throttling counters on node-join failure (#3723) 
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->

## What this PR does

When a tenant worker misses the node-Ready deadline, the failure bundle
currently cannot tell two different causes apart: a worker held at its
own CFS ceiling, and a worker losing host CPU it was entitled to. Both
look identical from outside the Pod, and node-level utilisation answers
neither, because it shows what the node used rather than what the
container was allowed.

This adds a collector that reads the CFS counters and the ceiling itself
from the kubelet's cAdvisor endpoint, for every tenant worker on every
node in the sandbox. The counters are the only place that difference is
recorded, and nothing else in the tree collects them for these workers.

The motivating measurement is on nightly run 31290999592: the `compute`
container reports 0.968 and 0.920 core against a `limits.cpu` of 1,
which is 92-97% of a one-core ceiling. Whether the container ever
actually met that ceiling is exactly what the artifact cannot say, and
what these counters answer.

Most of the change is the failure taxonomy rather than the read. An
empty capture is the dangerous outcome here, because zero bytes reads as
a container that never hit its ceiling -- the conclusion the collector
exists to stop a reader reaching by default. So every way the capture
can come up short gets its own sentence: the kubelet was not read, the
read was cut short part way, the metric stream could not be read back on
this runner, the kubelet answered and carried no series for this
namespace, the namespace was there and the worker Pods were not. The one
non-failure outcome is written down too: a container with no CPU limit
puts a single line on the wire, which is also what a truncated read
leaves behind, so it says which one it is.

### Ordering

The collector runs first among the gated collectors, ahead of the guest
serial console. Its ceiling is four bounded reads, about 100s of a 480s
phase budget, so on a slow run the console capture starts that much
later. That is a deliberate trade and not a side effect of putting the
cheapest first: cost is not what settles the order, survivability of the
answer is. The console evidence cannot be re-taken, and this collector's
question has no other answer in the tree, so both go ahead of the
captures whose state is partly recoverable from reads above them.

### Known limits, named rather than left to be found

- The outcome ladder enumerates combinations of read status, filter
status and result emptiness. Every new source of partial output needs a
new rung, and that has already happened twice while this branch was in
review. Deriving the label from the three axes instead would make a new
source land in an existing axis.
- The uncapped note is decided over the whole per-node file, so one
quota line anywhere in it suppresses the note. That is the conservative
direction -- it never claims uncapped over a file that shows a ceiling
-- but it makes the note rare.
- The messages say `see read-error.log` while the file on disk is
`<node>.read-error.log`. That is the convention the neighbouring
serial-console collector already uses, and changing one side alone would
make the two inconsistent.
- The `mkdir -p` for the report directory is unchecked, as it is in four
neighbouring collectors in the same file. Worth fixing across the file
rather than in one collector.
- The section letters in the failure block no longer track emission
order. They already did not before this change; this adds one more entry
to the sequence.
- The count of bats files sourcing the library, in
`docs/agents/e2e-testing.md`, is corrected here from three to seven.
Nothing pins it, so it will drift again.
- The node walk iterates `for node in ${nodes}` unquoted, so pathname
expansion applies alongside word splitting. Node names are RFC1123 so it
cannot bite today; `while IFS= read -r node` would make that structural
instead of incidental.


### Screenshots

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

### Downstream repositories

<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.

Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.

IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:

1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```release-note
test(e2e): capture tenant worker CPU throttling counters and their CFS ceiling when a node-join fails, so a worker held at its own limit can be told apart from one losing host CPU
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added worker CPU-throttling diagnostics to Kubernetes troubleshooting
reports.
* Reports now identify CPU quota gaps, partial reads, filtering issues,
and incomplete results.
* Added safeguards for bounded node traversal and configurable
collection budgets.

* **Bug Fixes**
* Improved failure handling when diagnostic commands, timeouts, or
metric reads are unavailable.

* **Tests**
* Expanded coverage for CPU-throttling capture, node-join diagnostics,
timeout behavior, configuration validation, and partial failures.

* **Documentation**
* Updated diagnostic collector counts and collection-order
documentation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-08-13 02:24:50 +03:00
Aleksei Sviridkin 94c41d100f fix(hack): fail when a chart runs no Helm unit test suites 
helm-unittest is fail-closed on a suite it cannot parse and on one
declaring `tests: []`, and fail-open on suites that are not there
at all: it prints "Test Suites: 0 passed, 0 total" and exits 0.
Measured on plugin 1.1.1 against a throwaway chart, all three
forms.

hack/helm-unit-tests.sh judges each package by that exit code
alone, so a chart whose suites were deleted, moved, or renamed past
the tests/*_test.yaml glob reports success having asserted nothing.
Moving packages/system/velero/tests aside reproduces it: the script
prints "All Helm unit tests passed" and exits 0.

Check the captured output for the zero-suite line and fail the
package when it appears. Neither `--strict` nor an explicit
non-matching `-f` glob does this on its own; both exit 0.

Matching that line names the cause where it applies, but the set of
ways to run nothing is not closed: a recipe that never invokes
helm-unittest, and a `test:` rule make finds nothing to do for,
both print no marker at all, so no further match reaches them. So
also require the line a real run always emits, a `Test Suites:` count
of at least one. Every way of asserting nothing exits 0, and asking
what the run asserted is the question that has an answer. All 77
packages the script visits emit that line today. The tradeoff is
that a `test` target which legitimately runs no helm-unittest would
need renaming or an opt-out, which is the bargain the zero-suite
check already struck. That is a contract contributors have to know
about, so docs/agents/overview.md states it where it describes the
sweep.

Because the check matches helm-unittest's own summary wording, a
plugin upgrade that reworded it would fail every package at once
while blaming each package's Makefile. The error text says so, so
the reader of a tree-wide red has the real cause in front of them
rather than 77 copies of the wrong one.

The discovery gate has the same shape from the other side.
`make -C dir -n test` succeeds against a file-backed target, so a
path named `test` beside a package Makefile whose rule is not phony
would make both the gate and the run exit 0 without the recipe
firing. Of the packages the script visits that define a `test` rule,
18 declare it phony, and no such path exists today. Match what make
reports rather than looking for the path, because a package that
declares the target phony runs its recipe whatever sits next to it,
and refusing that would be a false failure. The
quoting around the target name differs between make 3.x and 4.x, so
the match accepts either opening quote; the trailing quote alone
would also match a sub-make reporting another target whose name
ends in "test".

Running the suite now captures output rather than streaming it,
because a pipeline's exit status in POSIX sh reports the last
command rather than make. The capture file is allocated once and
removed by the EXIT trap that already owns the failure list, and the
run is pinned to LC_ALL=C: all three checks match English wording,
one on make's and two on helm-unittest's, so a localized make would
disarm the first of them silently, which is the failure mode this
guard exists to remove. The locale reaches every package's recipe,
not only make.

Extend hack/helm-unit-tests.bats with eight tests. Three pin the new
refusals; two pin what they must not refuse, a phony target with a
colliding path and a sub-make reporting an unrelated target up to
date; one pins that a chart which does run suites still passes; two
pin behaviour that was already there, namely that a failing package
is still reported by name and a package with no `test` rule is
skipped rather than failed. That last one carries a second package
that does run a suite, because with only the target-less package
present the script takes its early exit above the failure summary
and the assertion holds whatever the script did. The fixtures stub
make output instead of invoking helm, so they need no plugin
installed.

Both tests that assert a package is named in the failure summary
match the summary's "  - " prefix rather than the bare path. The
script announces "Running tests in $dir" before it runs anything, so
the bare path is present in the output whatever the summary does;
matching it asserted nothing, and the summary could stop naming
packages entirely with the suite still green.

The failing-package fixture reports a passing suite count alongside
its non-zero exit, so that the exit status is the only thing that
can decide the outcome. A fixture that fails without printing the
marker is refused by the positive-evidence check first, which leaves
the exit-status branch unpinned. That shape occurs in the tree: a
`test` target that runs helm-unittest and then a second step, a
render-parity script or a vendored-version guard or go tests, emits
the marker and still fails.

The two tests already in that file cover a package reached under
packages/tests, and their fixture printed a marker and nothing else.
That is the shape the positive-evidence check refuses, so the
fixture now also emits the line a real run emits; without it both
would fail for a reason unrelated to what they assert.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-12 18:45:22 +03:00
Aleksei Sviridkin b8a5b9ba1d test(e2e): capture worker CPU throttling counters on node-join failure 
When a tenant worker misses the node-Ready deadline, the sandbox node
it runs on is routinely at half its capacity with a clean kernel log.
In that state "the host starved the VM" and "the VM was held at its
own CFS ceiling" produce identical evidence: node-level utilisation
reports what the node used, never what the container was allowed. The
CFS counters are the only place the difference is recorded, and
nothing in the tree collected them for the tenant workers, so every
diagnosis of this failure so far had to be inferred from outside.

Read them from the kubelet's cAdvisor endpoint on the node hosting
each worker. Five series carry the whole answer and cAdvisor publishes
all of them per container, so nothing is computed here: the scheduled
and throttled period counts, the time spent throttled, and the quota
and period that form the ceiling. The throttled counters alone say a
container hit some ceiling, not which one, and a VM capped at one core
and a VM capped at eight are the same number without the quota.

Absence of the quota series is itself a reading rather than a gap.
cAdvisor emits it only when the quota is non-zero, so a container
reporting the period and no quota is one running uncapped - which is
one of the two answers this collector exists to distinguish, arriving
without being derived.

Reading the kubelet rather than the container matters beyond
convenience. A reader running inside the container shares the cgroup
it measures, so it slows down in proportion to how much the answer
matters and would need a budget sized for the pathological case; the
kubelet is outside that cgroup. It also drops any dependency on what
the container image ships and on how the host lays cgroups out.

The namespace is not a worker filter and is not used as one: it also
holds the tenant control plane, whose apiserver and etcd carry CPU
limits of their own and can be genuinely throttled. The Pod name
carries the second half of the filter, so a throttled apiserver cannot
answer under a heading that says worker.

The walk is over nodes rather than Pods, because the endpoint is per
node and one read covers every worker it hosts. It is gated on the
diagnostics phase budget, and placed first among the five gated
collectors: the budget declines whatever has not started when it runs
out, its own ceiling is four bounded reads rather than the minutes the
four after it can spend, and it is the only one whose question has no
other answer in the tree. That order is a trade, stated here because
it is a decision rather than a detail - it puts the guest serial
console one place further from the budget, and that capture is the
only one describing a worker which never reached apid.

Both reads take the block's own read-budget knob rather than a literal
equal to it, so lowering that knob lowers these too. Each keeps its own
exit status rather than routing through the shared helper, which always
returns zero by design: this collector has to tell "the kubelet was not
read" from "the kubelet answered and carried no worker", and only the
status separates them. The stream is captured whole and filtered after,
since a pipeline reports its last command's status and would otherwise
hand that decision to grep.

Every way a read can come up short is named rather than left as zero
bytes: a kubelet that was not read, one that answered with nothing for
the namespace, a stream cut short, and exit 124 - which cannot
distinguish this collector timing out from the read exiting 124 on its
own - each get their own line, and each says whether an error log
exists to explain it.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-12 18:43:11 +03:00
Aleksei Sviridkin 91b9f58057 fix(hack): declare the shared package targets phony 
package.mk set .PHONY with `=` rather than `:`, which assigns a
variable named .PHONY instead of declaring anything phony, so none
of the targets package.mk defines was phony in any package that
includes it.

Declare exactly the concrete targets package.mk defines: help, show,
diff, apply, delete, suspend, resume, check and clean. The earlier
list named update and image, which package.mk does not define —
naming an undefined target in .PHONY creates it as an empty target,
so `make image` and `make update` would report success without
running anything in the many packages that lack those recipes. It
also omitted check and clean, the targets a same-named file or
directory could actually shadow.

The neighbouring .DEFAULT_GOAL is a variable and correctly keeps its
equals sign. A comment now records which of the two adjacent lines is
which, so the corrected one does not pull the other into its shape by
analogy.

The trigger map in docs/agents/contributing.md called the two files a
byte-for-byte copy, which was exact rather than loose: they were the
same object. They stop being one here, so the entry now records that
they were identical and no longer are, and asks for a diff-and-port,
the wording the map already uses for the other copy that has
diverged. It deliberately does not enumerate which lines differ:
that set moves as the copy catches up, and an entry that names it
would be wrong again by the next port.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-12 18:42:58 +03:00
Aleksei Sviridkin 587a7b9fbd fix(ci): decide E2E coverage per changed path 
The full-suite escalation for a changed package that reaches no suite was
read off the merged selection, so it depended on the whole diff rather
than on the path that needed it. Any other changed path contributing a
suite name made the selection non-empty and the escalation vanished with
nothing in the output to record it: edit a platform component, adjust the
Chainsaw test beside it, and the component ran that single suite instead
of everything. Over one values.yaml per package under
packages/{apps,system,extra}, 82 of the 159 escalate on their own and
stop escalating once an unrelated suite edit joins the diff.

Coverage is now decided per changed path. Each path resolves through its
owning sources to the suites that exist, and a path reaching none of them
forces the full run whatever the rest of the diff selected. The unit is
the path rather than the source: system/postgres-operator belongs to two
PackageSources where one reaches no suite, so deciding per source would
escalate on that half and run everything for every change to it.

The escalation names its cause on stderr, as every other escalation in
this script already does. Most packages reach no suite of their own, so
this is the branch a full run usually takes; without a line saying which
sources bought it, "why did this run everything" has no answer in the log.

The walk moves into resolve_suites() to be called once per path. POSIX sh
has no `local`, so the function writes into the caller's scope and is
safe only inside the $( ) its single call site wraps it in; a test pins
that the call count stays one.

Two entries leave src_to_suites. `postgres-application` restated what
stripping the suffix produces and `external-dns` restated what carrying
the source name through produces, so both made the exception list read as
longer than the set of real exceptions.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-09 22:24:37 +03:00
Aleksei Sviridkin 0980e755eb fix(ci): map E2E suites from sources not named after an app 
src_to_suites() covered PackageSource names ending in -application plus
external-dns, and the filter after the graph walk dropped every other
source it reached. cozystack.kuberture and cozystack.securitygroup-
controller are neither, so both were walked to and then discarded: a CNI
change ran neither of the two controller suites most likely to regress
from it, and a change to either package reached no suite at all and
escalated to the full run instead of selecting the one that covers it.

Every reached source now maps through the table. That table is the
inverse of select-install.sh's suite_to_source(), and a test walks every
suite through both, so the next source name fitting neither convention
fails there rather than quietly making its own suite unreachable.

The cilium test measured the gap rather than hid it, asserting the
19 suites the selector could reach and naming the two it could not.
The subtraction is removed rather than inverted: kept as an exclusion
list it would stay green if the walk stopped reaching them for an
unrelated reason. What replaces it is a second assertion against a tree
holding one suite the graph reaches and one no source names, which an
escalation cannot satisfy.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-09 21:28:00 +03:00
Aleksei Sviridkin 048088bd29 fix(e2e): bound the tenant failure diagnostics per read and as a phase (#3676) 
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->

## What this PR does

The node-join failure diagnostics in
`hack/e2e-chainsaw/_lib/run-kubernetes.sh` issued about a dozen
`kubectl` reads with neither an outer wall-clock bound nor
`--request-timeout`, starting with a `describe nodes` through the tenant
kubeconfig. That read targets the component least likely to answer in a
node-join failure. An unbounded read there does not lose only itself: it
holds the Chainsaw op until the op is killed, so everything scheduled
after it is lost rather than truncated, the tenant crust-gather snapshot
above all.

Every read now carries the per-call bound the newer collectors on this
path already use. A read that fails or is cut off says so instead of
passing in silence, and says only what was observed, because a read that
did not finish is silence about the cluster rather than a finding about
it. The CDI importer walk gains a cap, and its listing is read on its
own so a listing that never answered cannot read as a namespace with no
importer in it.

Per-read bounds alone do not end the phase, and the collectors here can
together spend more than what is left of the op by the time they run. So
the phase is bounded as a phase: once its budget is spent, the
collectors that have not started are declined out loud. That is not a
worst case and is not written as one, but it stops the snapshot being
queued behind work begun past the deadline, which is how the snapshot
was being lost. The op ceiling is unchanged, since a larger ceiling
multiplied by a sum that grows with every collector added is the same
problem one collector later.

Because what runs last is what gets declined, the block is reordered so
the cheap reads come first. The CSR list and the signer log are the
discriminator for the failure mode a follow-up fix has to target, and
behind two minute-scale collectors they were exactly what a tight run
would drop.

Adjacent work on the same failure path, all of it in service of the same
requirement:

- the sibling scheduling-gate branch ends in the same exit and depends
on the same snapshot, so its two reads are bounded too;
- `talos_image_cache_diagnose` gets its reads bounded and its gate gets
`--ignore-not-found`, because `kubectl get` exits 1 for a refused
connection or Unauthorized as readily as for NotFound, so a call that
never reached the apiserver was being announced as a mirror that was
never deployed;
- the four numeric knobs are validated rather than only documented,
since a suffix or a leading zero on the budget reached the arithmetic in
the block's first statement and unwound the whole function before its
own headline, leaving a failing run with no diagnostics at all.

Comments carry invariants rather than arithmetic. A comment that states
a sum has to be kept true forever in every copy of it, and any edit to
the code or the order falsifies one of them. The numbers live in the
code as values and in the guard that pins them.

Relates to #3656.

### Known gaps, deliberately not closed here

Some collectors on this path carry no wall-clock bound at all. The
image-cache reachability re-probe makes seven unbounded
management-cluster calls, and the guest-Talos capture walks its VMI rows
uncapped with applies and waits bounded only by `--request-timeout`,
which bounds one HTTP request rather than a client retrying against a
wedged apiserver. The phase budget stops them being started late; it
does not stop one running long once admitted. They are pre-existing and
tracked in #3666. Bounding the re-probe in particular would change when
the same function gives up on the happy path, and so which image factory
tenant workers use, which is not a diagnostic's decision to make.

Named improvements left for later, none of them regressions introduced
here:

- the phase deadline is stamped at block entry rather than against what
is left of the op, so a long bringup still narrows the room the
derivation assumes;
- `bringup` and `largest` are literals in the guard, so a collector
heavier than today's will not move them, and the margin is small by
design;
- the integer-knob validator now exists in four places in this tree, and
a shared `_lib` file sourced ahead of both libraries would collapse two
of them;
- the per-request bound equals the wall-clock bound, so both deadlines
fire together and which of the two notes appears is a race;
- the ordered list in the chainsaw comments omits the ungated wedge
check, which runs first;
- section letters no longer sort after the reorder, and the
comment-to-code ratio in the touched block is high.

### Testing

`hack/run-kubernetes-node-join_test.bats` is new: 22 cases driving the
block with a `kubectl` stub that hangs, refuses and answers in part, and
a `timeout` stub that fails any read not wrapped in the bounding idiom.
It pins that no read escapes a bound, that a cut-off read is named
rather than reported as an empty cluster, that the walk cap reports both
counts, that a failed listing is not reported as no importers, that the
phase declines the rest out loud, that a malformed knob falls back and
says so, that the block returns zero so the caller keeps its `exit 1`,
and the source order that keeps the discriminating reads ahead of the
expensive collectors. Every one was checked by mutation against the
unfixed shape.


### Screenshots

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

### Downstream repositories

<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.

Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.

IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:

1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```release-note
fix(e2e): bound every read in the tenant Kubernetes node-join failure diagnostics and give the collection phase its own wall-clock budget, so a wedged apiserver can no longer hold the Chainsaw op until it is killed and take the tenant crust-gather snapshot with it
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Bug Fixes**
- Improved Kubernetes node-join and scheduling failure diagnostics with
bounded collection times, clearer timeout and failure reporting, and
more reliable preservation of critical snapshots.
- Added safer handling for unavailable diagnostic data, missing tools,
failed commands, and undeployed image caches.
- Added configurable and validated timeout controls for image-cache and
failure diagnostics.

- **Documentation**
- Clarified diagnostic budgets, collection ordering, and behavior when
available time is exhausted.

- **Tests**
- Added comprehensive coverage for diagnostic timeouts, ordering,
validation, failure handling, and bounded reads.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-08-09 12:33:27 +03:00
Aleksei Sviridkin 86fb92abf5 docs(tests): correct five stale claims in the e2e capture tooling (#3662) 
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->

## What this PR does

Five things in the e2e tooling said something the code no longer does,
or did not say it at all. All five are prose; no executable line
changes.

`docs/agents/e2e-testing.md` section 8 described two of the three
on-failure capture legs. The missing one is
`hack/e2e-capture-dataplane.sh`, the only leg that reads forwarding
state off the node rather than object state, and the only one whose two
callers bound it differently, 600s from the BATS runner and 300s from
the Chainsaw catch. That split is what decides whether something added
to it fits, so a contributor who reads the document and not the script
sizes a change against the wrong budget. Its notes contract is described
with the carve-out the script declares itself: the two EndpointSlice
reads in the LoadBalancer path report nothing either way, so a `<none>`
backend line there is the one place a reader still cannot tell an
endpointless Service from a read that never answered.

The same section said the runner's captures serve "the BATS tests" in
two places. They serve the e2e ones. The runner keys on an `e2e-`
prefix, on the suite's own filename or on its parent directory, because
all three legs read cluster state and a unit runner has `kubectl` on
PATH with no cluster behind it. The legs are not gated alike either, so
the sentence says which fired where: two key on `kubectl`, and the
snapshot leg keys on `crust-gather` being installed, which on a unit
runner it usually is not.

Two numbers in the capture-gate rationale in `hack/cozytest.sh` no
longer described the code. The snapshot leg's cost was given as 360s,
the deadline passed to `timeout`; the leg is invoked as `timeout -k 30
360`, so the trap waits out 390s, which is what the comment forty lines
below and the op-budget arithmetic in `.chainsaw.yaml` already said. And
the data-plane leg was still described as having no bound on its first
pod list, and billed at the full 600s on that basis, after that read was
given a 28s ceiling.

Two files enumerated the BATS suites and both stopped one short. The
global catch in `hack/e2e-chainsaw/.chainsaw.yaml` said the runner it
names as its analog "only covers the BATS install/openapi tests", and
`hack/e2e-chainsaw/README.md` named the same two files. Both miss
`hack/e2e-prepare-cluster.bats`, which `packages/core/testing/Makefile`
drives through that runner, and which is the suite whose failures leave
the sandbox in the state hardest to reconstruct afterwards.
`docs/agents/e2e-testing.md` already listed all three, so the tree
disagreed with itself depending on which file you opened.

A comment in `hack/e2e-install-cozystack.bats` pointed at
`hack/e2e-apps/run-kubernetes.sh`, which does not exist; that directory
holds two monitoring suites and no runner. The readiness gate it means
is the `rollout status` wait in
`hack/e2e-chainsaw/_lib/talos-image-cache.sh`, along with both fallbacks
to the public factory.

relates to #3626 and #3642


### Screenshots

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

### Downstream repositories

<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.

Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.

IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:

1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```release-note
NONE
```
 2026-08-09 04:08:35 +03:00
Aleksei Sviridkin 06078359d0 docs(e2e): record the phase-budget pattern beside its rule 
The e2e guide tells a collector author to size the op timeout above the
sum of
the inner budgets and let the inner ones fire. That works while the sum
is
something an op timeout can sit above, and on the tenant-Kubernetes
node-join
path it no longer is: the terms run past the 50m op on their own, and
several
of the reads there carry only --request-timeout, which bounds one HTTP
request
rather than a client retrying against a wedged apiserver, so parts of
that
path have no ceiling to add up at all.

Left as it stood, the next collector author follows the documented rule
and
adds a bounded read to a sum that already does not fit. So the
phase-budget
pattern goes next to it, with the two properties that are easy to get
wrong:
the budget bounds when a collector may start rather than when the phase
ends,
so it is not a worst case and the comment beside it must not claim to be
one;
and because what runs last is what gets declined, source order decides
which
evidence survives a tight run.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-09 02:51:06 +03:00
Aleksei Sviridkin 9a0c8c75f3 fix(ci): close the silent-green gaps in e2e selection and the helm unit sweep (#3670) 
## What this PR does

Four fixes in e2e test selection and the helm unit sweep. Relates to
#3650 #3652 #3655 #3646.

select-e2e built both dependency indexes as `$(build_owners_index | sort
-u)`, so it read sort's exit status and never yq's (#3650). A broken yq
left the index empty, every path fell through to escalation, and CI ran
the full suite on every PR. The output was identical to "no owners
matched", so it looked like the conservatism the selector is supposed to
apply.

The suite list one line up had the same shape and a worse result.
`$(find ... | sed | sort)` reports sort, never find, and errors went to
/dev/null. Every escalation prints that list, so a failed find made
"escalate to the full suite" emit a blank line, which both e2e lanes
read as "skip Chainsaw" before posting `E2E Tests` green. The outcome
that means run everything delivered a run of nothing.

find is not the only way to get an empty list, and the escalations are
not its only reader, so the check sits where the list is built. The
`examples/backups/<app>/` rule escalates nothing and still
membership-tests against that list: with it empty, an edit to a backup
harness that should run its suite reports nothing to run, on a path no
escalation-side guard would ever see. An empty list is a broken
enumeration, so the script refuses to answer at all. Both lanes run the
step under `bash -e`, so the non-zero exit fails the job. I checked both
workflows: neither overrides `shell:`, both capture the selector as a
plain assignment.

Fourteen tests used `[ "$(echo "$output" | wc -w)" -gt 5 ]` to mean
"everything was selected" (#3652). There are 21 suites, so that passes
on any six, and partial escalation stayed green. They compare against
the whole list now, derived the way the script's own full-suite branch
derives it. Drop one suite from that branch and thirteen of them go red.
All thirteen passed under the threshold.

The fourteenth is the cilium case, and equality shows it was never a
full-suite path at all. It resolves through the dependency graph, which
reaches 19 of the 21 suites. The two it misses are kuberture and
securitygroup, whose sources are not `*-application`, and the mapping
covers only those plus external-dns by name. Both controllers declare
`cozystack.networking`, so a CNI change that breaks either one runs
neither suite. That gap is #3665. The test is renamed for what it
measures and pins the 19, so closing #3665 turns it red at the right
moment.

Editing `hack/e2e-chainsaw/backup/chainsaw-test.yaml.disabled` selected
the full suite (#3655). The suffix was ignored when deriving the suite
name, the intersection against existing suites emptied, and the
empty-selection safety net escalated. A disabled suite runs nowhere, so
its edits are the one file class that cannot regress a test, and they
were buying the most expensive outcome the selector has.

The selector now has a fourth outcome, a failed step rather than a
selection, so the contract enumerations in the script header, in
`docs/agents/e2e-testing.md` and in both workflow comments say so.

The helm unit sweep never visited `packages/tests` (#3646).
cozy-lib-tests, the whole test coverage for a helper library that 34
charts consume by symlink, ran only because
`packages/library/cozy-lib/Makefile` delegates its `test` target to it.
Remove that one line and the suite goes from one execution to zero with
the sweep still exiting 0 and still printing "All Helm unit tests
passed". Every check the sweep makes runs inside a directory the loop
already reached, so a package that never enters the loop takes all of
them with it, and what is left is `tests_found`, which counts suites
across the whole run rather than per package. The directory is on the
list now: 76 executed suites become 77, at 0.1s to 0.3s over six runs
against a sweep of roughly 31 seconds. The delegation stays for `make -C
packages/library/cozy-lib test` ergonomics, so the suite runs twice.
Neither run is load-bearing alone.

### Testing

`hack/select-e2e_test.bats` goes from 27 tests to 32,
`hack/helm-unit-tests.bats` is new. Each commit is green on its own: 27,
30, 32, 32.

The new tests stub the failing component and assert the exit status plus
the line naming what broke: a broken yq, a broken find, an empty suite
list across all three diff classes, the disabled-suite path, and a guard
that the disabled rule drops only that path and not the rest of the
diff. The yq test asserts exit 0 on purpose. A missing dependency graph
still has a correct answer, the full suite, and a missing suite list
does not. Pinning both halves keeps the header and the docs from
drifting into claiming otherwise.

Two suites fail on my macOS box and fail identically at `origin/main`,
so neither comes from this branch:
`hack/migration-seaweedfs-db-adopt.bats`, where a container running as
the host UID cannot write the bind-mounted temp dir under Colima, and
`hack/seaweedfs-naming-audit.bats`, which I confirmed red at the merge
base but did not dig into. Every other `hack/*.bats` was run
individually and is green. `make unit-tests` stops at the first failing
file and both of those sort ahead of `select-e2e_test.bats`, so on macOS
the suites this PR touches have to be run directly.

### Named improvements, not done here

- The `.disabled` rule keys on the file rather than the parked
directory, so it is both wider and narrower than the property it wants.
A support file under `hack/e2e-chainsaw/backup/` still buys a full run,
and a `.disabled` file dropped inside a live suite directory now selects
nothing where it used to select that suite. The property covering both
is one level up: a suite directory holding no live `chainsaw-test.yaml`
is inert, testable with the same `grep -Fxq` membership check the
`examples/backups/*/*` arm already uses. It is not a free win, since a
half-written suite directory escalates today and would then select
nothing. A cheaper half step is narrowing the glob to
`hack/e2e-chainsaw/*/chainsaw-test.yaml*.disabled`, because case globs
cross `/` and the shipped pattern also matches nested paths.
- `packages/tests/**` is not classified by the selector and reaches the
full suite through the unclassified fall-through instead of a rule.
Tracked in #3667.
- `full_suite_list()` in the bats file cannot fail. cozytest.sh's awk
rewrites a lone `}` into `return 0; }`, so a broken find inside the
helper would return an empty expected set instead of a red test. Nothing
false can pass today, because the script hard-fails on an empty suite
list before any assert runs, but that helper is what every equality
assert measures against.
- The yq check runs before the changed-file list is read, so with a
broken graph even a docs-only diff escalates. Measured: a broken yq plus
a `docs/x.md` diff returns 21 suites at head against 0 at the merge
base. Only reachable in an already broken state, but the graph is
consulted at the component lookup and in the closure walk, so moving the
check to first use would charge nothing to docs, dashboards and
inert-meta diffs.

### Screenshots

No UI changes.

### Downstream repositories

Walked the trigger map in `docs/agents/contributing.md` file by file
against the diff: four files under `hack/`, one under `docs/agents/`,
and comments in the two e2e workflows. Nothing under `hack/` was moved
or renamed and no make-target contract changed, so the ccp entry does
not fire. It gates on `hack/package.mk` and `hack/common-envs.mk`,
neither of which is in the diff. The node prerequisites in
`hack/e2e-prepare-cluster.bats` that ansible-cozystack mirrors by hand
are untouched, and so is `.github/workflows/tags.yaml`. `docs/agents/`
is internal contributor documentation, not a website source. No package
was added, renamed or removed, and no `values.schema.json` or version
enum changed, so the website and provider entries do not fire either.

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

```release-note
fix(ci): select-e2e no longer reports an empty selection when it cannot enumerate the Chainsaw suites, which made an escalation to the full suite indistinguishable from "skip Chainsaw" and left the E2E gate green with nothing run. A failure to list the suites is now named on stderr and fails the step, a yq failure is named and still escalates to the full suite, and an edit to a disabled Chainsaw suite is inert instead of escalating to a full run.
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Improved end-to-end test selection for disabled, empty, or partially
changed test suites.
* Test selection now safely escalates to the full suite when suite
discovery or dependency analysis fails.
  * Helm unit-test sweeps now include tests under `packages/tests`.

* **Tests**
* Added coverage for suite discovery failures, disabled suites, command
failures, and exact test selection.
  * Added Helm test-sweep coverage, including failure handling.

* **Documentation**
  * Clarified test-impact analysis rules and suite-selection outcomes.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-08-08 20:47:06 +03:00
Aleksei Sviridkin 355fc8a24a test(tests): let each bats file declare the EXIT traps it still holds (#3622) 
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->

## What this PR does

The ban on EXIT-trap cleanup in `hack/*.bats` was enforced by two lists
inside `hack/cozyreport.bats`: one naming the files known to be clean,
one freezing an exact trap count for each file that was not. Both had to
be edited from whatever change moved them, and that is where the guard
kept failing.

It is worth being exact about how badly, because the history is sharper
than "it went stale a few times". **The inventory was never correct on
main for a single commit.** It landed in #3567 at 15:25:20. #3195 had
landed `multus-install-cni-plugins.bats` carrying twelve traps at
15:24:37, forty-three seconds earlier. Replaying the old guard's own
logic against the tree at the very commit that introduced it already
gives `found != frozen`. Main then stayed red for roughly twenty-two and
three quarter hours. The first repair, #3584, landed *already red* the
next morning at 10:42:43, because #3548 had brought in
`run-kubernetes-talos-diagnostics_test.bats` with eight traps at
10:40:03, under three minutes ahead of it. That fix was correct and
bought zero green time. Green arrived only with the second repair,
#3602.

Neither pair of PRs shared a line, and each was green against its own
base. That is the whole mechanism, and it is why a third one-line repair
is not the answer.

The mechanism is structural rather than careless. A change that adds a
trap to its own file had to edit a string in a suite it otherwise never
touches, so two changes sharing no line still invalidated each other:
each stayed green against its own base, git merged both cleanly, and the
guard went red only once the second one landed. A file *arriving* with
traps was worse still, because nothing in its author's diff pointed at
that string at all. That is exactly how
`run-kubernetes-talos-diagnostics_test.bats` got in, twice.

That contention is not in the past tense. Two open PRs are editing that
one line right now, and they disagree about what it should say: #3575
adds `run-kubernetes-talos-diagnostics_test.bats=8` to it, repeating a
repair that has already landed, and #3441 removes
`select-e2e_test.bats=15` from it, because it converts that file.
Neither PR is about EXIT traps. Both have to touch that string anyway,
and whichever lands second is wrong until someone edits it again.

So this PR is not fixing a red main. It removes the thing that keeps
making main red, which is why it is worth more than the one-line fix
that is now the established habit.

One honest caveat about its own landing. The textual-conflict property
is steady-state: a branch forked *before* the declaration exists has no
line to disagree with, so it converts traps in its own file, merges
clean, and the count only goes wrong once both sides are on main. I
checked this against #3441, which takes `select-e2e_test.bats` from
fifteen traps to zero. Merged after this, that file would declare
fifteen and hold none. What the move buys even in that case is that the
red names a file the branch already edited, the repair is one line
inside it, and rebasing before merge catches it on the branch's own CI.
None of those three held against the central inventory. Whoever merges
this should expect one such adjustment on the conversion branches still
in flight.

So this replaces both lists with a declaration each file makes about
itself: one `# EXIT-TRAP DEBT: N` comment in its leading comment block.
A file carrying no declaration must install no EXIT trap. Growing or
shedding a trap now fails in the file the change already edits, so two
changes that disagree about a count get a real textual conflict instead
of silently invalidating each other, and a change that leaves the traps
alone edits nothing.

**The include list is redundant, not lost.** It named the files proven
clean, so that a trap reappearing in one of them would fail. Under the
new rule those files carry no declaration, and a file with no
declaration must hold zero traps, so a trap reappearing in any of them
fails on its own, with no list to be on. Coverage widens rather than
narrows: the two lists named twenty files between them, and the rule
covers all fifty bats files under `hack/`, subdirectories included, plus
the ones added tomorrow.

Rebasing this branch onto current main is the property working. Main has
since gained `hack/kubernetes-pre-delete-hook.bats` and
`hack/tenant-pre-delete-hook.bats`, and `hack/cozyreport.bats` grew by
some eight hundred lines. Neither new file installs an EXIT trap, so
neither needed a declaration and neither needed an edit here; the rebase
took no conflict at all. Under the inventory, each arriving file was a
coin toss on whether somebody had remembered the string.

To be precise about what the inventory could and could not do, since it
is easy to overstate: it did *notice* a new file carrying traps. The
string it compared was built by scanning the directory, so an arriving
file appended a token and failed the comparison, which is exactly how
main went red. What it could not do is let that file arrive without an
edit in a foreign suite. Being seen and being absorbable are different
properties, and only the second one decides whether two changes can land
independently.

The declaration is pinned in both directions. Declaring N while holding
N+1 fails, obviously; declaring N while holding N−1 fails too. Without
that second half the number becomes a ceiling and rots upward: somebody
converts half a file, the declaration stays, and the guard quietly
licenses traps that were removed long ago.

It is read only from the leading comment block, under the shebang and
above the first line of code. A `.bats` file is shell that writes shell,
so the same line turns up inside a heredoc, a fixture writer or an
expected-output string, where it is data belonging to one test;
honouring it there would let an unrelated fixture excuse a real trap,
silently, with nothing in that test's own diff looking like a
declaration. A comment block is the region with no interior; stopping
instead at the first `@test` would still read a line out of a helper's
heredoc.

Not every counted handler is debt. A trap inside an explicit subshell
does not replace the one the `bats` binary installs, so a test failing
inside `( … )` still prints its `not ok`, checked against a test-level
trap in the same file, where the TAP line vanishes.
`hack/e2e-test-openapi.bats` kills a backgrounded `kubectl proxy`
exactly that way, and "convert it like the others" would leak a process
holding a fixed port and wedge the next run. Its declaration now records
the carve-out instead of scheduling a conversion, and because the
ratchet is exact in both directions, *removing* that trap fails too, so
the count protects the construct rather than marking it for deletion.
`docs/agents/e2e-testing.md` previously scoped this exception to
Chainsaw `script` steps only; it now names the BATS subshell case as
well.

The counting bounds the keyword and the signal the same way, at any
character that cannot be part of an identifier, and matches the signal
in either case. Whitespace on the right missed `trap … EXIT; cd "$tmp"`;
whitespace on the left missed `tmp=$(mktemp -d);trap … EXIT` and `(trap
… EXIT; true)`; upper case missed `trap … exit`, which bash and dash
both install. All of those are real handlers that scored zero. The left
boundary is the one worth dwelling on, because the inventory being
replaced had none at all and *did* catch the semicolon form. Getting it
wrong here would have narrowed coverage while the commit claimed to
widen it. A plain word boundary is not enough either: `bootstrap ` ends
in `trap `, and it has to keep scoring nothing, or the documented answer
to a red guard (add a debt line) would buy a file a permanent licence
for one real trap to silence a line that has none.

Two handlers sharing one line are reported rather than counted, because
the count is a count of lines and the second would otherwise arrive
free. Splitting such a line properly needs a shell parser, since a
semicolon inside a handler's own quoted action is not a separator, and
guessing wrong undercounts, the one direction a ratchet cannot afford.

**What this does not fix.** The declaration is still a loophole: a new
file can write `# EXIT-TRAP DEBT: 8` instead of cleaning up, and nothing
here makes that impossible. What changes is that the admission is local
and visible. It sits at the top of the file it excuses, in front of
whoever reviews that file, instead of being a number in a neighbouring
suite nobody in that review is reading. Today's loophole is the same
size and invisible.

Three more limits, all stated in the guard's own header rather than left
to be discovered. The scan is lexical, so a signal computed at runtime
and a quoted action spanning physical lines without a backslash are both
invisible. An exact count catches addition and removal but never
substitution: swap the openapi file's subshell trap for a test-level one
and the total stays 1. And the scan reads `.bats` only, so a handler
arriving through a sourced `.sh` is outside it.
`hack/e2e-chainsaw/_lib/run-kubernetes.sh` installs two right now, and
each is benign for its own reason rather than by design: the one in
`cozy_capture_tenant_talos` because that function is declared with `(`
and so runs in a subshell, the one in `run_kubernetes_test` because no
`@test` calls it despite being declared with `{`. Three `hack/*.bats`
source that library, and two tests in the converted file call
`cozy_capture_tenant_talos`, so flipping a single `(` to `{` reinstates
a test-level handler in both of them with the guard green. Widening the
scan to `.sh` would mean counting handlers that are correct in a script
and wrong only in a test body, so the honest answer is that this is
where the instrument stops.

Three further boundaries, recorded here so they land as known edges
rather than as surprises. The old include list also failed when a file
named on it disappeared from the tree; the new rule can only judge a
file that is present, so a deleted converted file goes unnoticed. That
is a genuinely smaller check, though its absence shows up in the diff
that deletes the file. The guard's own failure messages are code lines,
so they are scanned by the pattern they belong to: they pass today only
because no bare `EXIT` or `0` happens to follow the keyword in any of
them, and a rewording that introduced one would make the file demand a
debt of itself. It fails loudly rather than quietly, and the fixture
writers and test titles already split the keyword from the signal for
this reason, but the messages do not. Finally,
`docs/agents/e2e-testing.md` bans test-level `EXIT` *and* `RETURN`
traps, while every mechanical guard this repo has had, the one being
deleted included, matches only `EXIT` and `0`. `hack/` holds no RETURN
trap today, so nothing regresses here, but half of that documented rule
has never had an executor.

The guard moves out of `hack/cozyreport.bats` into
`hack/bats-no-exit-trap.bats`, because its subject is every unit suite
under `hack/` and not the report collector it grew up in. Living inside
`cozyreport.bats` is precisely why unrelated PRs kept converging on one
line. `hack/md-no-hardwrap.bats` is the neighbouring precedent.

Separately, the eight EXIT traps in
`hack/run-kubernetes-talos-diagnostics_test.bats` are converted to
end-of-body cleanup rather than given a declaration. That file was
written six weeks after the ban was documented, so "it predates the
rule" is not available to it, and handing it a debt is literally the
move that failed the last two times. The remaining files keep their
declarations, since a mass conversion would collide with the branches
that own them.

One note on where the prose sits. The guard's own header carries only
what a reader has to keep true: why the ban exists, the two carve-outs,
where the declaration lives and why there, that the count is exact in
both directions, why the marker is anchored at column zero, and where
the lexical scan stops. Everything above about *how* the inventory
failed (the timestamps, the PR numbers, the arithmetic) is deliberately
not in the file. It is true today and will rot on its own; a comment
nobody is obliged to maintain is how a file grows a paragraph that is
quietly false.

### Screenshots

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

### Downstream repositories

<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.

Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.

IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:

1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```release-note
test(tests): each hack/*.bats file now declares its own remaining EXIT-trap debt in a `# EXIT-TRAP DEBT: N` header comment, checked by hack/bats-no-exit-trap.bats, replacing the central inventory in hack/cozyreport.bats
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Testing**
- Added automated auditing for `EXIT` traps across Bats end-to-end
tests, including validation of tracking declarations and edge cases.
- Improved diagnostics tests by replacing trap-based temporary-directory
cleanup with explicit cleanup steps.
  - Added tracking annotations for remaining trap-related cleanup work.

- **Documentation**
- Clarified when traps are permitted inside self-contained subshells and
how remaining cleanup debt is reported.
  - Updated review guidance for consistent end-to-end test maintenance.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-08-08 20:46:49 +03:00
Aleksei Sviridkin 1abc653cfb fix(ci): treat a disabled chainsaw suite as inert in test selection 
Editing hack/e2e-chainsaw/backup/chainsaw-test.yaml.disabled selects the
whole Chainsaw suite. The per-suite rule reads the suite name off the
directory and ignores the suffix, so the change selects "backup"; the
final intersection against the suites that actually exist finds no such
suite and empties; and the safety net for an empty selection escalates to
the full run.

A disabled suite is registered nowhere and executed by nothing, which
makes its edits the one file class that provably cannot regress a test.
The mapping had it backwards: they bought the most expensive outcome the
selector has. Rare in practice, and paid on every commit by anyone
grooming or re-enabling such a suite.

Classify *.disabled as inert ahead of the per-suite rule, and record it
alongside the other rules that select nothing so the header stays an
accurate enumeration. The safety net keeps its job for paths no rule has
looked at, which is what it was added for.

Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-08 16:31:39 +03:00
Aleksei Sviridkin 37a395363f fix(ci): stop select-e2e from deciding on inputs it never checked 
Three holes on the same seam: the script builds a list, never looks at
whether the command that built it succeeded, and then decides from the
empty result.

The dependency indexes are built as `$(build_owners_index | sort -u)`,
where the function is a single yq. A pipeline carries the status of its
last command, so the script sees sort's zero and `set -e` never fires: a
missing binary or a malformed source leaves the index empty, every changed
path falls through to escalation, and CI runs the full Chainsaw suite on
every pull request. The direction is safe and that is the problem — an
empty index and "no owners matched" print the same thing, so the symptom
is a full run, which reads as the conservatism the selector is supposed to
apply. Selection can stay switched off indefinitely with nobody signalled.
Check each half's status on its own and name yq once on stderr before
taking the fallback the empty index would have reached anyway.

The suite list one line up has the identical shape and a worse
consequence: `$(find ... | sed | sort)` reports sort, never find, and
errors went to /dev/null. That list is what every escalation prints. So a
find that failed produced an empty list, and "escalate to the full suite"
then emitted a blank line — which both e2e lanes read as "skip Chainsaw"
before posting the required "E2E Tests" status green. The one outcome that
means run everything delivered a run of nothing, on the pull request that
earned a full run. Capture find on its own, check it, let its errors
through.

A failing find is not the only way to end up with an empty list, and the
escalations are not its only reader, so the check sits once where the list
is built. The examples/backups/<app>/ rule escalates nothing and still
membership-tests against that list: with it empty the test fails, the app
is not selected, and an edit to a backup harness that should run its suite
reports nothing to run — output identical to a legitimately empty
selection, on a path no escalation guard would ever see. An empty list is
a broken enumeration, a moved directory or a wrong working directory, not
a project without tests. Refuse to answer at all. Both lanes run the step
under `bash -e`, so a non-zero exit fails the job rather than falling
through to the empty selection this is here to prevent.

Each is covered by a test that stubs the failing component and asserts
both the exit status and that the script names what broke.

Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-08 16:17:31 +03:00
Aleksei Sviridkin 5728eca401 docs(e2e): name the data-plane collector and who gets the captures 
Section 8 described two of the three on-failure capture legs and not
the third, so the collector that reads forwarding state off the node
was absent from the one document that tells a contributor what
diagnostics exist. It is also the leg whose two callers disagree about
its budget, 600s from the BATS runner and 300s from the Chainsaw
catch, which is what decides whether something added to it fits.

Its notes contract is described with the carve-out the script itself
declares: the two EndpointSlice reads in the LoadBalancer path report
nothing either way, so a <none> backend line there is the one place a
reader still cannot tell an endpointless Service from a read that
never answered. Stating the contract without it would have promised
the distinction on the path this collector exists to diagnose.

The prose also still said the runner's captures serve "the BATS
tests". They serve the e2e ones: the runner keys on an e2e- prefix,
on the suite's filename or on its parent directory, because all three
legs read cluster state and a unit runner has kubectl on PATH with no
cluster behind it. Said once where the captures are introduced, and
the two phrases that over-claimed now name the e2e suites.

The legs are not gated alike, so the sentence says which fired where:
two key on kubectl, and the snapshot leg keys on crust-gather being
installed, which on a unit runner it usually is not.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-08 11:21:33 +03:00
Aleksei Sviridkin 8e273a7341 docs(ci): record what escalating on the non-PR workflows costs 
Three of the four workflows on the escalation list cannot be exercised
by the run they trigger. `e2e-fork` runs on `workflow_run`, which always
takes the default-branch copy; `e2e-tag` runs on `workflow_call` from a
tag push; `nightly` runs on a schedule. Only `pull-requests.yaml` is
executed from the pull request's own head, so only there does escalating
test the edit rather than the tree around it.

That argument existed in the reference doc and was removed by this
branch rather than answered, and the list it argued against is what
replaced it. Measured over six months of main, 26 commits touch one of
the three with no other escalating path; each now pays a full Chainsaw
run it used to skip, on a suite whose flake rate is the reason people
avoid running it for nothing.

Keeping them is defensible: the rule stays one idea rather than two, and
dropping them leaves those files inert, which reads as an oversight
rather than a decision. What was not defensible was paying it silently.
The trade now sits next to the pattern that makes it, with the number,
so the next person weighing it starts from measurement instead of
rediscovering the mechanism. The reference doc points there rather than
carrying its own copy.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-08 05:55:37 +03:00
Aleksei Sviridkin 55f3c79e9f docs(ci): stop pointing at the inert list for paths it does not hold 
Four places said an empty selection means the inert pattern matched, and
sent the reader to `inert_config_pattern` to see which paths those are.
Documentation, dashboards and `*.md` are dropped before that pattern is
ever consulted, and a backups example whose app has no suite is dropped
where backup examples are matched. A reader checking the pattern for any
of them finds nothing and has to reconstruct the order by reading the
script. Name the rules that produce an empty selection instead, and keep
the claim to the one the lanes rely on: empty is a decision some rule
reached, not a path nothing looked at.

The same bullet claimed every path is classified. One is not. The inert
pattern makes `.github/` inert as a whole directory and the workflows
that run the suite are exempted from it by name, so a workflow added
under `.github/workflows/` reads as inert and the fall-through never
sees it. Today's enumeration is correct and I checked it against `rg -l
test-chainsaw`, so this is not a live hole, but the sentence saying it
cannot exist is what makes the reminder next to that list look
decorative. Record it as the guard it is.

Separately, the fork lane's rationale for appending a newline described
a mechanism this branch removes: the reader now guards its own loop, so
the terminator is no longer what keeps the last path alive. Say that it
is now the text-file contract and a second line of defence across two
files, which is a reason to keep it and a different reason than the one
written there.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-08 05:02:25 +03:00
Myasnikov Daniil ae4e9e55cb fix(ci): classify every TIA path and escalate the unclassified 
`select-e2e.sh` silently selected nothing for a changed path it did not
recognise outside `packages/`. Both e2e lanes read an empty selection as
"skip Chainsaw" and then post the required "E2E Tests" status green, so
an unrecognised path produced a green merge gate with no suite run. The
worked example is `hack/common-envs.mk`, which supplies the tag, push
and output flags of every image in the tree and matched nothing:
`full_suite_pattern` covered `hack/*.sh` and `hack/*.bats` but not
`hack/*.mk`. `pkg/`, `tools/`, `go.mod`, `hack/lib/` and
`hack/buildkitd.toml` were in the same position, as was
`.github/workflows/e2e-fork.yaml` while `pull-requests.yaml` was
already escalated.

A blanket "empty means full suite" fallback would be the wrong fix: it
escalates the inert paths the design deliberately skips. Instead every
path now lands in one of three classes by an explicit rule — the
full-suite list for shared build inputs and the e2e harness, an inert
list for paths that cannot affect what e2e exercises, and the
PackageSource graph for `packages/(apps|system|extra)/`. A path
matching none of them escalates and logs why, so the next path someone
adds fails safe instead of failing open.

The two lists are checked in that order, so a specific escalation beats
a broad inert directory: `.github/` is inert, the workflows that run
the suite are not. `*.md` and `docs/`/`dashboards/` stay ahead of both,
so a README under `packages/core/` does not inherit that tree's
escalation.

Ten tests cover both sides of the classification. Mutation-checked:
reverting the fall-through fails the unclassified-path test, dropping
`.github/` from the inert list fails the non-e2e-workflow test, and
swapping the two list checks fails the existing pull-requests.yaml
test. Note the per-entry escalations are belt-and-braces — removing
`hack/*.mk` from the pattern still escalates via the fall-through,
which is the point of having it.

Closes #3392

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
 2026-08-08 04:28:27 +03:00
myasnikovdaniil 7dca642894 fix(ci): restore e2e for fork PRs and close the merge-gate bypass (#3262) 
## What this PR does

Fixes #3257 — fork PRs never ran e2e, and the skipped required check let
them merge anyway.

Fork PRs get no secrets on `pull_request`, so `make image` pushed
anonymously and died with `denied`. That failed every build, collapsed
the `build → finalize → e2e` chain, and left the required **E2E Tests**
job *skipped* — which branch protection treats as satisfied, so an
approved fork PR could merge with e2e never having run.

This implements **Option A** from the issue (privileged `workflow_run`
split), pushing fork e2e images to the existing CI registry (OCIR):

**Build primitive (`hack/common-envs.mk`)**
- New `OCI_EXPORT_DIR` mode: build each image to a per-image OCI archive
instead of pushing, and force `PUSH`/`LOAD` off. The digest is captured
via `--metadata-file` regardless of output type, so the refs baked into
`pr.patch` match what the privileged run later pushes. Threaded through
the `image-tags` macro every package already uses — no per-package
Makefile change.

**`pull-requests.yaml` (unprivileged, fork branch)**
- `build` / `build-talos` / `finalize` export images to OCI archives and
upload them as artifacts instead of pushing → the red `denied` build
wall disappears and the chain no longer collapses.
- The in-tree e2e job is renamed off `"E2E Tests"` and guarded to
same-repo PRs. A new `e2e-report` job concludes the required `"E2E
Tests"` context as an **explicit check-run** (success on a docs-only PR
or when e2e passed, failure otherwise). A skipped job can no longer
satisfy the required check — this also closes the same hole for
same-repo PRs whose build fails.

**`e2e-fork.yaml` (new, privileged `workflow_run` from the default
branch)**
- Resolves the fork PR, opens a pending `"E2E Tests"` check-run, and
fails-closed if the fork build failed.
- Pushes the pre-built image archives to OCIR **by digest** and
flux-pushes the digest-pinned packages tree. The OCIR token is live only
for trusted binaries (`git`/`skopeo`/`flux`/`yq`) over data — never for
`make` or any fork-authored script.
- Runs the e2e suite against the pushed closure with **no push token**
and concludes the check-run on the PR head SHA.

Fork build code never receives registry credentials; the fork's images
and test scripts run only in the credential-less, ephemeral e2e job — a
stronger posture than a `pull_request_target` run that would hand fork
code the secrets directly.

### Validation

Local: buildx OCI-archive output + digest capture, macro expansion for
single- and multi-image packages, non-fork push path unchanged,
`actionlint` clean.

Needs live validation (why this is a **draft**): `workflow_run` artifact
download across runs; that the `"E2E Tests"` commit status satisfies the
required context and keeps a fork PR non-mergeable until the privileged
run concludes it; `refs/pull/<N>/merge` fetch + `git apply --3way`;
`skopeo --preserve-digests` end-to-end. `workflow_run` only runs from
the default branch, so the privileged half is exercised by mirroring the
workflow onto a personal fork's default branch and opening a throwaway
fork PR into it — not by merging first.

_Since review_: the fork `e2e` job ran the pre-#2826 BATS app loop that
the merged Chainsaw migration deleted (already broken); it now runs the
same Chainsaw + TIA path as the in-tree job, with suite selection and
the `full-e2e` override sourced from `resolve`'s trusted base-repo file
list / labels (not a merge-ref diff). Publish job hardened per the
non-blocking follow-ups: base-tree image allowlist, `skopeo copy --all`,
pinned flux CLI. The required `E2E Tests` gate is now published as a
commit **status** (default token + `statuses:write`), not a COZYSTACK_CI
app check-run — the app lacked `checks:write` (`checks.create` 403'd)
and a default-token check-run floated under a labeler suite; a commit
status has neither problem, and forks can't post one (their
`pull_request` token is read-only). Two review-bot findings fixed too:
fork `OCI_EXPORT_DIR` is now absolute (a relative path misplaced
archives under `make -C <pkg>` and every fork build would fail at
upload), and the "≥1 archive" guard no longer no-ops under `nullglob`.

### Repo-admin prerequisites

One repository setting lives outside this diff:

1. **Branch protection: keep `E2E Tests` required.** The gate is now a
commit **status** named `E2E Tests` (posted with the default
`GITHUB_TOKEN` — see `docs/agents/e2e-testing.md` §10), not a job or an
app check-run. Branch protection already requires `E2E Tests` (currently
the e2e job's name, renamed here to `E2E (in-tree)`), so the required
context now resolves to the status — **no GitHub App, no `checks:write`
grant, no app-id pin needed**. Just don't pin the context to a specific
app.
2. **Actions → "Require approval for all outside collaborators".** Each
fork run must be a deliberate maintainer click before it executes, since
the fork's unprivileged run produces the artifacts the privileged
`e2e-fork.yaml` consumes.

### Downstream repositories

Walked the trigger map in `docs/agents/contributing.md` against the diff
(7 files: two CI workflows, `docs/agents/e2e-testing.md`,
`hack/common-envs.mk` + its bats suite, and two package `Makefile`s).

The one trigger worth naming is "change developer tooling
(`cozyvalues-gen`, `cozypkg`, the package `Makefile`s) →
`development.md`" on the website. It does not fire here:
`OCI_EXPORT_DIR` is a CI-only build mode that defaults off, and the
local developer workflow is unchanged — `make image` still builds and
pushes exactly as before, which the new default-path case in
`hack/common-envs_test.bats` now pins. No chart values, no API surface,
no app list, no platform variant, no Talos version bump and no release
asset rename are touched.

- [x] No downstream repository is affected by this change

### Release note

```release-note
fix(ci): fork pull requests now run e2e via a privileged workflow, and a fork PR can no longer merge on a skipped (never-run) required E2E Tests check
```

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Added a dedicated fork E2E workflow that runs after the base PR and
reports results via the required **E2E Tests** commit status.
- Fork PRs now export build images as OCI archive artifacts for later
publishing.
- Added a same-repo E2E reporting job and renamed the in-tree E2E flow
to **E2E (in-tree)**.
- **Bug Fixes**
- Added tighter, fail-closed gating to ensure merge blocking when **E2E
Tests** isn’t correctly finalized.
- **Documentation**
- Documented fork vs same-repo E2E split, OCI export behavior, and
required fork security settings.
- **Tests**
- Added Bats coverage validating `OCI_EXPORT_DIR` and preventing
unintended registry pushes.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-08-08 04:26:25 +03:00
Aleksei Sviridkin 806b66f25c docs(ci): stop describing a one-workflow E2E CI 
Two sentences predate the second workflow and were left describing the
world before it.

The scope line at the top is the pointer AGENTS.md routes agents through
before they touch the E2E CI, and it named only the in-tree workflow. A
reader sent there to change the fork lane would not learn that the
section holding its invariants exists, which is the one thing that line
has to do. It now names both workflows and says which pull requests each
serves.

The TIA escalation list said edits to "the E2E workflows" escalate to
the full suite. That was true while there was one. The selector's list
names `pull-requests.yaml` and not `e2e-fork.yaml`, so a change touching
only the fork workflow selects nothing: the Chainsaw step is skipped,
while the platform install and the OpenAPI tests still run. Say that
plainly, and say why it is tolerable rather than pretending it is
designed. A `workflow_run` always executes the default-branch copy of
the file, so running the suite against a change to it would not exercise
that change anyway. It is the same shape as the fail-open the section
already cites.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-08 03:22:08 +03:00
Aleksei Sviridkin cc8b01d5cd docs(ci): say when each lane's status opener actually lands 
The reference section claimed both lanes open the required context when
their run starts, and enumerated the triggers it protects against: a
re-run, a reopen, a `full-e2e` label. Every one of those starts a Pull
Request run, and on a fork that run cannot open the status at all. The
opener is guarded same-repo only, because a fork's `pull_request` token
is read-only, and the fork's opener lives in the privileged workflow,
which starts only once the unprivileged build has finished. So on the
lane the sentence was most likely to be read about, an already-green
head SHA keeps reading green for the whole build.

The gap is structural. A read-only token cannot post a status at any
point in the run, and that same read-only token is what makes publishing
the gate with the default token safe in the first place. It is bounded
by the build rather than open-ended. What was wrong was only the claim
that it does not exist.

Say so in all three places that carried the overstatement: the section
itself, the opener's own comment, and the contract test, which pins that
each lane has an opener and cannot express when it lands.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-08 02:10:35 +03:00
Aleksei Sviridkin 9c68b1f83e fix(ci): stop a cancelled fork run from publishing a verdict 
The invariant that a cancelled run publishes nothing was written down in
three places and held in two of them.

The fork lane's terminal reporter ran under `always()`, which includes
cancellation. When a second privileged run supersedes the first, the
dying one still posted `failure` on a head SHA it no longer owned, for a
suite that never finished. It self-corrected once the superseding run
reported, but that can be two hours later, and until then a legitimate
pull request reads red. The same-repo lane was moved off `always()` for
exactly this reason. Move this one too: a failed publish or e2e still
reports, since neither is a cancellation, and a cancelled run now leaves
the `pending` it opened standing until its successor concludes.

The same-repo guard's rationale cited a scenario that no longer exists.
It described a label event cancelling the run that opened the pull
request, which the concurrency split made impossible for every label the
workflow discards. The guard is still needed, for a push, for a
`full-e2e` label superseding the head run, and for a manual cancel, so
the comment now names those instead of an extinct one.

The reference section listed the pieces this invariant rests on and
omitted the reporters, which is the half a reader is most likely to
touch, and counted them, which was already wrong and would go wrong
again on the next addition. It now enumerates without counting.

The contract pins both reporters, because both lanes have had this
defect at different times in the same shape.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-08 01:26:09 +03:00
Aleksei Sviridkin 8e097488c2 docs(ci): name every writer of the "E2E Tests" status, and why it opens 
The section that tells the next editor to keep exactly one run per head
SHA responsible for the status listed only two of the three jobs that
write it, and described the same-repo lane as posting once at the end.
Both stopped being true when the same-repo lane gained an opener. The
section also spent a paragraph on the concurrency invariants without
saying why an opener has to exist at all, which is the part a reader
would otherwise remove as redundant.

Say that each lane opens with `pending` and concludes, that a status
never expires so the opener is what stops a re-run inheriting the
previous run's green, and that `plan`'s opener is `continue-on-error`
because every build and test job hangs off that job.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-08 00:55:02 +03:00
Aleksei Sviridkin fd06fb32e0 docs(ci): say what fork TIA hardening buys, and what it does not 
Four comments and a documentation bullet claimed that routing the
changed-file list through the base-repo API stops a fork from making
test selection pick nothing. It does not. The `e2e` job runs from the
fork's own tree, so `hack/select-e2e.sh` is the fork's script: a PR that
edits it to print nothing gets `skip=true`, the suite step never runs,
the job succeeds and the required status goes green — the exact outcome
the paragraph presented as prevented. The same holds for the suites
under `hack/e2e-chainsaw/`, the install bats and the testing package's
recipes, all of which the fork authors and this job executes.

Hardening the inputs is still worth doing, since a merge-ref diff would
be a second bypass on top of the one that exists, so the code does not
change. What changes is the claim: the inputs are trusted, the decision
is not, and the fork lane is a correctness gate against an honest
contributor's regression rather than a boundary against a hostile fork.
State plainly what does contain a hostile fork — the maintainer
approval required before any fork run executes, the absence of any
credential in that job, and a diff editing the harness being visible in
review.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-08 00:04:15 +03:00
Aleksei Sviridkin 575b41e1cd test(hack): let each bats file declare the EXIT traps it still holds 
The ban on EXIT-trap cleanup in hack/*.bats was enforced by two lists
inside hack/cozyreport.bats: one naming the files known to be clean, one
freezing an exact trap count for each file that was not. Both had to be
edited from whatever change moved them, and that is where the guard kept
failing. A change adding a trap to its own file had to edit a string in
a suite it otherwise never touches, so two changes sharing no line still
invalidated each other: each stayed green against its own base, git
merged both cleanly, and the guard went red only once the second landed.
A file arriving with traps hit the same wall from the other side -- the
inventory did notice it, since the string it compared was built by
scanning the directory, but absorbing it meant an edit in a file nothing
in the author's diff pointed at.

Replace both lists with a declaration each file makes about itself: one
"# EXIT-TRAP DEBT: N" comment, exact rather than a ceiling. A file
carrying none must install no EXIT trap, which covers the converted
files, the files that never had one, and every file added later.
Growing or shedding a trap now fails in the file the change already
edits, so two changes that disagree about a count collide textually
instead of silently.

The declaration is read from the leading comment block, not from
anywhere in the file. A .bats file is shell that writes shell, so the
same line turns up inside a heredoc, a fixture writer or an
expected-output string, where it is data belonging to one test. Reading
it there as a statement about the whole file would let an unrelated
fixture excuse a real trap, and would do it silently, since nothing in
that test's own diff looks like a declaration. A comment block is the
region with no interior: stopping instead at the first @test would still
read a line out of a helper's heredoc.

Not every counted handler is debt. A trap inside an explicit subshell
does not replace the bats binary's own, so a test failing inside
`( ... )` still prints its `not ok`. hack/e2e-test-openapi.bats kills a
backgrounded kubectl proxy that way, and moving the kill to the end of
the body would leak a process holding a fixed port. Its declaration
records that rather than scheduling a conversion, and because the
ratchet is exact, removing the trap fails too -- the count protects the
construct. What the count cannot do is tell the two apart: substituting
a test-level trap for the subshell one keeps the total at 1 and stays
green, which the header states rather than leaves to be found.

Counting bounds the keyword and the signal the same way, at any
character that cannot be part of an identifier, and matches the signal
in either case. Whitespace on the right missed `trap ... EXIT; cd
"$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap ... EXIT`
and `(trap ... EXIT; true)`; upper case missed `trap ... exit`, which
bash and dash both install. All are real handlers that scored zero, and
the left boundary matters most, since the inventory being replaced had
none and caught the semicolon form. A bare word boundary is not enough
either way: `bootstrap ` ends in `trap `, and it must keep scoring
nothing. A quoted signal counts for the same reason as the rest.

Two handlers sharing one line are reported rather than counted, since
the count is a count of lines and the second would otherwise arrive
without moving the total; splitting the line properly needs a shell
parser, a semicolon inside a handler's own action not being a
separator, and guessing wrong undercounts -- the one direction a ratchet
cannot afford.

The scan recurses, so hack/e2e-apps/*.bats is covered rather than
sitting one directory below the guard that claims the tree. It reads
.bats and nothing else, so a handler arriving through a sourced .sh
stays outside it -- hack/e2e-chainsaw/_lib/run-kubernetes.sh installs
two, each benign for its own reason rather than by design: one sits in
a function declared with `(` and so runs in a subshell, the other in a
brace function no @test calls. That boundary is stated in the header
rather than papered over.

The guard moves to hack/bats-no-exit-trap.bats: its subject is every
unit suite under hack/, not the report collector it grew up in. Its
fixture helpers assemble the trap keyword and the signal from separate
arguments, because the guard scans its own source and a fixture written
as a literal would be counted as a real trap in it.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-07 23:53:40 +03:00
Aleksei Sviridkin e2643ef4b9 docs(ci): explain the fork image allowlist and fix a test comment 
Two statements that were not true of the code next to them.

The fork `publish` job accepts only archive names it can derive from the
base parent, so a fork cannot name an artifact into an arbitrary
registry repository. The consequence is that a fork PR adding a new
image fails on `image '<name>' is not in the base-tree allowlist` with
no way to pass, and nothing said so — including the E2E section that
AGENTS.md points a reader at when a fork run fails. Record the
constraint, the exact log line, and the two ways around it, so the
failure reads as the guard working rather than as a broken PR.

The concurrency contract's comment on `code_lines` claimed a real grep
error propagates out of the helper. It does under the bats binary, and
not under `hack/cozytest.sh`, whose translator appends `return 0` to
every line that is exactly `}` — file-level helpers included — so a
helper's last command never decides its status there. Both runners
still fail closed, because each test asserts its extraction is
non-empty before comparing, so a swallowed error surfaces as empty
input rather than as a pass. Say that instead.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-07 23:10:19 +03:00
Aleksei Sviridkin 7c5e96947e fix(ci): stop two runs from publishing "E2E Tests" on one SHA 
Giving every `labeled` event its own concurrency group stopped a
discarded label from cancelling a live run, but it also stopped a
`full-e2e` label from doing so — and that label is not discarded. `plan`
proceeds for it and `e2e-report` publishes with it, so the labelled run
and the head run it was meant to widen both post the `E2E Tests`
context, on the same head SHA, with no ordering between them.

That was a deliberate trade at the time: commit statuses are latest-wins
per context, so the reasoning went, the full-suite verdict lands last
whenever its suite finishes last, and the label is an explicit opt-in to
a longer run. This commit reverses that decision, because the premise
only holds while the full run succeeds. The full suite is the superset,
so a full-e2e run that fails early — a build flake, a suite failing at
minute twenty — finishes before the narrower head run it was meant to
widen, and that run's later green erases the failure. The required
status then reads success for a SHA whose suite did not pass. One
publisher per head SHA is worth more than the minutes of runner time a
cancelled narrow run costs.

Move aside only the label events `plan` discards, by making the
concurrency condition the exact complement of `plan`'s guard. A
`full-e2e` run stays in the main group and supersedes the head run the
way a push does, so exactly one publisher of the context is alive per
head SHA. Opting into the full suite now costs one suite instead of two,
and a second label landing afterwards can no longer cancel the full-e2e
run, because the two are no longer grouped together.

The contract test gains the complement — the group key must exclude the
publishing label rather than every label — and a check that the label
name is one and the same string in all three guards that read it, since
renaming it in one place splits the decision without touching a line
that reads load-bearing.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-07 23:07:33 +03:00
Aleksei Sviridkin 3c50a09457 fix(ci): keep a discarded run out of the fork e2e gate's group 
The privileged fork e2e run groups by fork repo + branch with
cancel-in-progress, so every completed Pull Request run for that branch
lands in the same group -- including a run that built nothing.
Labelling an open fork PR produces exactly that: pull-requests.yaml
triggers on `labeled` and `plan` discards every label other than
full-e2e, so all jobs skip. The privileged run for that event cancels
the one that is mid-suite on the same head SHA, then `resolve`
recognises the discarded trigger and posts no status at all. The
required "E2E Tests" context is a commit status, so the head SHA is
left at "Expected" with nothing left to flip it -- the run that
replaced the owner is the very one that decided to stay silent -- and
the PR stays unmergeable until the contributor pushes again.

Key the group by the triggering run's id when that run concluded
`cancelled` or `skipped`, so a run that will post nothing can cancel
only itself. Real runs keep superseding each other on fork repo +
branch.

The contract test ties the conclusions in the key to the ones `resolve`
stays silent for. Those two lists sit in different languages in
different halves of the file, and a third conclusion added to the guard
alone would restore the wedge without touching a line a reviewer reads
as load-bearing.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-07 22:40:34 +03:00
Aleksei Sviridkin 7e2db4da7e fix(e2e): bound every remaining read in the report collector 
Four sections of hack/cozyreport.sh read with a wall-clock ceiling and
the rest -- nodes, namespaces, events, HelmReleases, packages,
packagesources, PVCs, cert-manager, COSI, kamaji, the Talos node reads
and every module gate -- read with none. The tarball is written on the
collector's last line, so a single read that never returns does not
truncate the report, it loses it, on the run where it is the only
evidence left.

Each read now goes through the shape that fits it: the existing bounded
reader for a read whose output is a file, the existing selector for one
that feeds a loop, and a new probe for the `if kubectl get deploy ...`
gates. A gate is a read like any other, and an unbounded one is worse
than the reads it guards, since the module below it never starts. Its
cut-off is recorded, because skipping the module leaves the same empty
tree as a cluster that never had the component; a gate that answers "not
installed" is recorded nowhere, since that is the cluster answering
correctly and a marker firing on every install without KubeVirt is one a
reader learns to skip. The reads that stay on the bare wrapper say so
where they sit; what they share is that the output is not the reader's
to write, going into a variable or through a pipeline that transforms
it, so there is no moment at which a marker could attach.

The header claimed a per-section list of what was bounded, and the tests
froze that list plus a per-section count of the unbounded reads. Both are
gone. The list had already gone stale, naming four sections while the
file had ten, and the counts were an undercount by construction -- they
matched a redirect to `$VAR/`, which no gate, selector or command
substitution has. One test replaces both: no read below the library guard
runs without a ceiling, checked by looking for a counterexample.

A ceiling per read is not a ceiling on the report, and the header now
says so where it used to imply the opposite. The pod, VM, VMI and
services walks carry a count cap and a time budget; the walks over nodes,
namespaces, HelmReleases, packages, PVCs and certificates carry neither,
so their cost is the cluster's to set. What this removes is the read that
never returns.

The bounded reader now names the tool it ran and describes what that tool
returned. It served kubectl alone until this change gave it talosctl and
the four host commands, so a `df` that hit a stale mount left a marker
reading "kubectl exited 1 ... not because the object ended" -- two false
statements in one line, in an artifact whose premise is that its notes
can be believed.

The exec reader keeps the remote command's whole message in the listing
it belongs to. `kubectl exec` appends its own `command terminated with
exit code N` after whatever the command said, so quoting the last line
there drops the cause and keeps the trailer, and sending the message
away from the file leaves a zero-byte listing -- the "returned nothing,
or never ran" question this collector exists to answer. Its cut-off note
also stops asserting that the archive is beside it: a stream that got
members across is kept and truncated, one killed before its first byte
leaves zero bytes and the call site's emptiness check removes it, and
the reader runs before that check, so it names both.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-07 14:38:29 +03:00
Aleksei Sviridkin 920ad3e30d fix(e2e): bound the report reads that run inside a pod 
The LINSTOR module runs six reads through `kubectl exec`: three
listings, the error-report index, and the controller and per-satellite
ErrorReport bundles. They are the only reads in the collector whose
duration is set by a workload rather than by the apiserver -- a wedged
controller, a satellite that stops answering, or a broken SPDY upgrade
holds the stream open with no clock anywhere in the path. The tarball is
written on the collector's last line, so one of those never returning
does not truncate the report, it loses it.

They go through a reader of their own rather than the existing one.
`kubectl exec` returns the remote command's exit status, so a 124 there
is either this collector's deadline expiring or a command that itself
exited 124, and the status does not separate them; describing it the way
every `get`, `logs` and `describe` is described would state a mechanism
about the machine writing the report for something that may have
happened inside a pod. The note says what is open instead of resolving
it, and goes beside the output rather than into it, since two of the six
stream a gzipped tarball that a line of prose turns into an archive
nothing can open. kubectl's own message travels with the note for the
same reason -- until now it was merged into the listings, so every run
put the "Defaulted container" line at the top of a table.

The guard that held the old assumption asserted only that no bounded
read was an exec, which leaving every exec unbounded satisfies. It now
also requires each exec to be bounded and requires the reader not to
borrow the classification written for reads whose 124 is unambiguous.
Matching on the verb rather than on the string `kubectl exec` is part of
the fix: half of these calls spell it `kubectl -n cozy-linstor exec`,
which that string never saw.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-07 14:38:29 +03:00
Aleksei Sviridkin 96e33fe656 ci(e2e): give the report collection step a ceiling of its own 
The step that runs hack/cozyreport.sh carried no timeout-minutes in any
of the three workflows that run it, so it inherited only the job's 180.
The collector writes its tarball on its last line and the upload is the
next step, so a read that never returns spends what is left of the job
budget, the job is cancelled, and the upload never runs -- the artifact
is lost entirely rather than truncated, on exactly the run where it is
the only evidence left. `|| true` guards against a non-zero exit, not
against a read that does not return.

30 minutes is measured against the step rather than divided out of the
job: it takes 3-42s across sampled runs, on failed e2e jobs as well as
green ones, and 30 leaves five sixths of the job budget for the steps
that upload what this one produced.

A test pins the ceiling in all three workflows and requires it to stay
under the job's own, since a step ceiling at or above the job's cap is
the same as no ceiling at all.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
 2026-08-07 14:38:29 +03:00
Aleksei Sviridkin 2527e915bd fix(e2e): collect diagnostics for unready pods and record every truncated capture (#3567) 
## What this PR does

The e2e diagnostic tarball exists so a red run can be explained from the
uploaded artifact alone. Several of its collectors answered "nothing is
wrong" instead — some because they selected on a column that never
carries the value they filtered for, others because they stopped early
and left nothing saying they had. Both failure modes look identical to a
healthy cluster from the outside, which is the worst possible shape for
a diagnostic.

Four selectors in `hack/cozyreport.sh` and `hack/cozyreport-summary.sh`
were reading the wrong thing:

- Per-pod evidence was chosen by the STATUS column, so a pod left at
`0/1 Running` by a readiness probe that never passes was skipped. That
is a common way for a suite to time out, and the artifact carried no
trace of the pod the run died on. Selection now compares the two halves
of the READY column; terminal `Succeeded`/`Completed` pods stay
excluded.
- The pending-LoadBalancer section filtered `$4`, which is CLUSTER-IP.
CLUSTER-IP holds an address, `None` or `<none>` and never `<pending>`,
so that loop had never executed. `<pending>` only ever appears in
EXTERNAL-IP, `$5`.
- The cozystack-apps section gated all three of its kinds on `kubectl
get crd <kind>.apps.cozystack.io`. ApplicationDefinition is a CRD but in
a different group, and Tenant comes from an aggregated APIService with
no CRD to ask about, so the probe failed and the section never ran.
- The kamaji section was gated on `cozy-linstor`'s `linstor-controller`
deployment rather than kamaji's own, so a cluster without LINSTOR got no
kamaji logs and no TenantControlPlanes — on exactly the runs that need
them.

The summary had three of its own: the HelmRelease listing printed `$5`
where STATUS is the Ready condition's whole message, reducing every
failed release to one word like "Helm"; the Flux sources listing used a
jsonpath with a nested filter that kubectl rejects outright, so it
printed nothing on every report ever taken; and pod AGE was read as
`$6`, which shifts whenever RESTARTS prints the `7 (4m12s ago)` form —
that is, for exactly the rows worth reading.

The second half is bounding. The pod, VM, VMI and services walks now run
under a count cap and a wall-clock budget, and every read is
individually time-boxed. The point is not the bounds themselves but that
each one names itself when it fires: a walk stopped by the cap or the
clock leaves `COLLECTION-TRUNCATED.txt`, a pod list that never returned
leaves `COLLECTION-FAILED.txt` instead of the empty tree a healthy
cluster produces, a log cut short says so on its own last line, and a
`--tail` bound is recorded beside the logs rather than inside them,
since the platform's controllers log JSON per line and a trailing prose
line costs a parser the whole file. A knob value the collector could not
use is reported too, so a truncation note quoting `600` is never
mistaken for the `10m` somebody actually set. Where `timeout` is absent
the reads run unbounded rather than not at all, and the section says
which of the two happened, because a missing local binary is not a
statement about the cluster.

One behaviour change worth stating plainly, because it is a trade and
not a pure gain: the per-pod section used to be uncapped and read
container logs whole, and it is now bounded by `COZY_REPORT_PODS_MAX`
(40), `COZY_REPORT_PODS_BUDGET` (600s) and `COZY_REPORT_POD_TAIL`
(2000). Below those thresholds the new tree is a strict superset of the
old one — the old selector missed `Running 0/1` entirely and skipped
logs for `Pending` pods. Above them the artifact is smaller than before,
every drop is named in the tree, and `COZY_REPORT_POD_TAIL=-1` with a
larger `COZY_REPORT_PODS_MAX` restores the old reach. Because widening
the selection and capping the walk in one change could otherwise let a
newly-in-scope pod evict one the uncapped walk always collected, the
ordering splits by evidence class before namespace: pods that are not
`Running` — the class the collector took before this — all rank ahead of
pods that are `Running` but unready, so the cap can only ever drop a pod
the old collector did not keep either.

`hack/e2e-capture-previous-logs.sh` gets the same treatment: its notes
now go to `capture-notes.txt` in the artifact as well as to the job log,
and a pod list that never returned is reported as such rather than as
"nothing restarted". That script now creates its own output directory
before its first line of output, which makes the comment in
`hack/e2e-chainsaw/.chainsaw.yaml` wrong about why the once-per-test
guard there holds — the only change to that file is the corrected
comment, saying the `-d` check keys on whether the catch block already
ran rather than on what the helper found.

The rest is test coverage — a new `hack/cozyreport.bats`, an extended
`hack/capture-previous-logs.bats` — and the removal of `EXIT`-trap
cleanup from the bats files those tests now cover. That trap is worth
its own note: inside an `@test` body it replaces the one the `bats`
binary installs for its own bookkeeping, so a test that then fails
prints no TAP line at all. It does not appear as `not ok`; it
disappears, and only the `# bats warning: Executed N instead of expected
M tests` line at the end distinguishes the run from a green one. A guard
test enforces the ban over a named list of files that grows as the
remaining ones are converted.

### Testing

Every collector this touches is now covered by unit tests that run
without a cluster: a new `hack/cozyreport.bats` (166 tests) and an
extended `hack/capture-previous-logs.bats` (34), driven through
`hack/cozytest.sh`. Ten `hack/*.bats` files run green locally, 265 tests
in total, with each file's `@test` count reconciled against its passing
count rather than trusted to the exit status — that reconciliation is
the point of the trap change, since an exit code answers "did anything
fail" and never "did anything run".

The guard test that bans `EXIT`-trap cleanup was checked by committing
the violation rather than by reading it: a folded `trap 'rm -rf "$tmp"'
\` + `EXIT` added to a file on its list turns the guard red and names
that file, and removing it turns the guard green again. `shellcheck
--shell=sh` over the three changed scripts reports strictly fewer
findings than before the change (`hack/cozyreport.sh` SC2086 170 to 116,
SC2162 15 to 13; the three SC2102 in `hack/cozyreport-summary.sh` are
gone), and `hack/e2e-capture-previous-logs.sh` is clean.

The parts that need a cluster — that the bounds fire, and that each one
lands in the artifact — are exercised by the e2e run itself, since these
collectors run on every failed suite.

### Screenshots

Not a UI change.

### Downstream repositories

Walked the trigger map in `docs/agents/contributing.md` against the
diff. This PR touches `hack/cozyreport.sh`,
`hack/cozyreport-summary.sh`, `hack/e2e-capture-previous-logs.sh`,
`hack/e2e-chainsaw/.chainsaw.yaml`, ten `hack/*.bats` files and
`docs/agents/e2e-testing.md`. Nothing is moved or renamed under `hack/`,
no make target changes what it does, and the files downstream consumers
key on (`hack/package.mk`, `hack/common-envs.mk`) are untouched. No
package, values key, CRD, variant, namespace or release asset is
involved.

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

```release-note
fix(e2e): the diagnostic report now collects per-pod evidence for pods that are Running but not Ready, which it previously skipped, and several collector sections that silently produced nothing now report correctly. Collection that stops early on a bound now says so inside the artifact, so a truncated report can be told apart from a healthy cluster.
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Diagnostic reports now prioritize relevant resources and collect
bounded pod, VM, service, storage, certificate, and Flux data.
- Reports preserve partial results and clearly identify timeouts,
unavailable resources, truncation, warnings, and collection failures.
- Previous-container logs now include bounded capture, namespace
prioritization, failure details, and partial-read evidence.
- Reports target the operator workload and safely handle missing
dependencies or temporary storage failures.

- **Documentation**
- Expanded guidance for reliable end-to-end testing and diagnostic
report collection.

- **Tests**
- Added comprehensive coverage for report generation, log capture,
failure handling, limits, and portability.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
 2026-08-06 15:25:20 +03:00