mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-30 08:37:31 +09:00
11bac54f931682dd94de4081bb9e8a7eef60bef1
138
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
1f30ac4598 |
fix(tests): let the mariadb waits cover a first boot (#3769)
## What this PR does Closes #3741 Several waits for a database HelmRelease to become Ready gave up before the release could: the mariadb, mongodb, rabbitmq and clickhouse backup walkthroughs waited 300 seconds, and both mariadb Chainsaw suites gave their HelmRelease-ready assert 6 minutes. A generated release carries a 10-minute install timeout (`pkg/cmd/server/start.go:91`), so a failed install was reported as a bare timeout, because the release had not yet recorded why it failed, and for mariadb a first boot the startup probe was still willing to wait for failed the harness instead. A wait on such a release has two floors and has to beat the larger. The first is the app's own first boot: cozystack-api leaves `spec.waitStrategy` unset and helm-controller defaults it to `poller`, so the install waits through kstatus, and kstatus maps the operator CR's `Ready=False` onto `InProgress`. For mariadb that boot may take the chart's 310-second startup budget (`startupProbe.failureThreshold: 30` in `packages/apps/mariadb/templates/mariadb.yaml` with the operator's probe defaults of 20 and 10); mongodb, rabbitmq and clickhouse set no startup probe. The second is the install timeout. At or below it the waiter is the one that expires, because its clock starts when the step runs and the install's only once the Application has been reconciled and the chart artifact fetched. The install timeout binds in every case here, so the walkthrough waits go to 660 seconds and the mariadb asserts to 11 minutes. Raising a wait inside a walkthrough spends the Chainsaw op that runs the script, and an op that runs out SIGKILLs the process group and loses the script's error. Each op therefore grows by exactly what the waits it reaches gained: mariadb's round-trip from 30 to 42 minutes, rabbitmq's from 30 to 42, mongodb's from 30 to 36 (its round-trip runs with `SKIP_RESTORE`, so only the source wait counts), and clickhouse's from 60 to 72, which still sits above the sum of every inner ceiling. That bounds a failure at a single wait, not a run that is slow at every stage and then fails late; bounding that needs a phase budget, not a larger literal. postgres keeps its 360-second waits and every walkthrough keeps its 300-second bucket wait: whether a suite pays the extra minutes on every red run is a per-suite call. The mariadb endpoint asserts keep their job. A freshly applied CR carries no conditions and kstatus reads it as current, so the install wait can end before any pod is up; an endpoint address closes that window. That is also why the `wait_for_field` on the CR stays in the walkthrough. `wait_hr_ready` now dumps every condition and the release history on timeout and on `Stalled=True`, instead of the single Ready message a scheduled retry can overwrite before anyone reads it. It also stops printing one `(none recorded)` line for different answers: a release that never appeared, a release deleted during the wait, a lookup kubectl could not answer, a failed history read and an empty history each say so, with kubectl's own stderr kept, and a release seen earlier still gets its dump when only the last lookup failed. The change goes to every byte-identical copy of the helper, in `examples/backups/{clickhouse,mariadb,mongodb,postgres,rabbitmq}/00-helpers.sh`, and they stay identical. `hack/mariadb-first-boot-waits.bats` pins the mariadb budgets against the tree. It derives both floors from their sources instead of pinning literals, so raising `failureThreshold` past the install timeout, or changing the install-timeout default, reds it; each source has a test that refuses to proceed when it cannot be read, including the ApplicationDefinition, whose two override annotations would each invalidate the derivation. It holds the round-trip op at a fixed remainder over the ceilings up to and including the source wait, bucket stage and S3 preflight included, and at a fixed slack once both mariadb waits are paid, so raising a wait without the op reds. The extractors are also tested against fixtures, because the tree cannot tell a correct extractor from a broken one. Beyond mariadb, the file runs every exit branch of `wait_hr_ready` in each copy against a stubbed kubectl, and pins the copies byte-identical. The population is every definition in the tree found by grep, minus two named exceptions held to still differ, so a new copy joins without editing the test and an exception whose copy converges goes red: - `examples/backups/redis/00-helpers.sh` is written in the compact one-line style of the rest of that file, so converging it rewrites its shape rather than applying the shared body. - `examples/backups/kafka-metadata/00-helpers.sh` is a helper of its own with the same diagnostic gap on timeout (it prints no reason at all), and closing that is a change to kafka-metadata. `docs/agents/e2e-testing.md` replaces the reviewer checklist's 5-6m standard and its list of exceptions, which the tree had outgrown, with the floor a wait has to beat and the rule that a longer wait carries its reason in-line. The derivation gets a section of its own, opening with the rule in one line. For a release an app chart renders itself the floor is `spec.install.timeout`, then `spec.timeout`, then helm-controller's 5m default, and the nested `<name>-system` releases never take cozystack-api's 10m. The harbor comments name the floor each of their asserts sits against, and the opensearch comment stops citing the removed standard. Adding `hack/mariadb-first-boot-waits.bats` does not escalate the E2E selection: `hack/[^/]+\.bats$` sits in `inert_config_pattern` in `hack/select-e2e.sh`, and `full_suite_pattern` escalates only `hack/e2e-*.bats`. On this changeset `hack/select-e2e.sh` selects `clickhouse harbor mariadb mongodb opensearch postgres rabbitmq`. What this costs is paid only on a failing run; the raised numbers are ceilings. Summing the timeouts stated on each suite's own steps, mariadb goes from 92m to 114m, clickhouse from 116m to 128m, rabbitmq from 67m to 79m and mongodb from 95m to 101m, before the untimed steps that fall back to the defaults in `.chainsaw.yaml`. clickhouse is the most expensive suite by that sum. All of them compete for the 215m `timeout-minutes` of the whole `e2e` job, which also covers image download, sandbox preparation and the Cozystack install, and a job cancelled at the cap loses both its log and `cozyreport.tgz`. This does not fix the failure that led here. In the run that prompted the investigation the mariadb replica never opened port 3306 at all, spending its whole 310-second budget inside `mariadb-install-db` before being killed, and no wait budget would have saved it. That defect is #3744. What this fixes is a second, independent defect found while reading the same code. One thing here is argued rather than measured: the mechanism is checked against helm-controller and kstatus, and every number is derived from the tree, but no run ID pins that it is what any particular red run hit. Both floors justify 660s whatever the cause was, so being wrong about the cause leaves slack rather than breakage. ### Not in this PR - This PR retires the mariadb, mongodb, rabbitmq and clickhouse walkthrough waits and the two mariadb asserts. The rest is tracked in #4404: the other Chainsaw HR-Ready asserts under their floor, the harbor asserts, the 300-second bucket waits, the postgres waits, and the redis and kafka-metadata diagnostic gap. - The clickhouse round-trip comment says every wait fails fast, `wait_hr_ready` on `Stalled=True`, "so a genuinely broken backup reports in seconds". A release whose install is still running is neither Ready nor Stalled, so that holds for a stalled release and not for an install that has not finished. The sentence predates this change. - `hack/mariadb-first-boot-waits.bats` is large for what it guards (two floors, two ratchets on the op, and the helper identity pin), with many fixture tests on its own extractors. Trimming it to that core is a separate decision. - Two comments in that file state facts about the mariadb suite that are true today and that nothing checks: three script ops carry a minute timeout, and the only one ahead of the round-trip is the 8m pre-clean. - The mariadb op comment says a bound on the whole flow "needs a phase budget (docs/agents/e2e-testing.md)"; the doc describes it as bounding the collection phase as a phase (`COZY_DIAG_PHASE_BUDGET`), so the pointer would resolve faster if it named that bullet. - Cosmetic: the behavioural test's loop body is not indented under its `for`, one line of the bats header runs past the width of the rest, and the checklist's sentence under the example assert ("what this block demonstrates least") reads awkwardly. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff, file by file, for all eighteen files. The five `examples/backups/*/00-helpers.sh` files change the diagnostics of `wait_hr_ready`, and the mariadb, mongodb, rabbitmq and clickhouse walkthrough scripts change wait budgets; the manifests the walkthroughs apply and the flows they document are unchanged, so no website page restates anything that moved. Four Chainsaw suites (mariadb, clickhouse, mongodb, rabbitmq) change timeouts and comments, and the harbor and opensearch suites change comments only. `docs/agents/e2e-testing.md` changes the reviewer checklist entry and adds the sizing section. `hack/mariadb-first-boot-waits.bats` is a new file picked up by the existing `BATS_UNIT_FILES` wildcard. `hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh` are untouched, and no `values.schema.json`, `values.yaml` default, version enum, `ApplicationDefinition`, CRD, namespace or release asset is touched. - [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: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(tests): the MariaDB, MongoDB, RabbitMQ and ClickHouse backup examples and the MariaDB Chainsaw suites now wait for a HelmRelease past the release's own install timeout, so a failed install reports its own reason instead of a bare timeout, and the examples' HelmRelease wait now tells a release that never appeared, one deleted mid-wait and a kubectl that could not answer apart. ``` |
||
|
Aleksei Sviridkin
|
b1519acfe7 |
fix(e2e): cap the bucket S3 reads so the verify step honors its timeout (#4281)
## What this PR does Refs #4268 — a plain reference, not `Fixes`: the issue asks for a `script` op to be cut off at its declared ceiling, and that is Chainsaw behaviour this repo cannot change. Chainsaw runs the op's shell under Go's `exec.CommandContext`, whose default cancel SIGKILLs the shell PID alone and then blocks on the inherited stdout/stderr pipe until an orphaned `mc` closes it, so the op returns only when `mc` exits on its own rather than at the ceiling. This PR mitigates that where it bit — per-command caps so no descendant outlives its own budget — but the op-timeout gap itself lives in Chainsaw, so the link stays a reference rather than a `Fixes`. The sibling step that produced the 606s, `verify-every-bucket-takes-a-write` in the `bucket-1-capacity` Test, is on `main` and already carries its own per-call caps; this PR applies the same shape to `verify-s3-access`. On run 34948279363 the bucket suite's `verify-every-bucket-takes-a-write` step declared `timeout: 5m` but ran 606s and ended on `mc`'s own non-zero status rather than being cut at the ceiling. `verify-s3-access` in `hack/e2e-chainsaw/bucket/chainsaw-test.yaml` is the same-shaped step in this suite. This PR wraps every S3 (`mc`) and apiserver (`kubectl`) read in `verify-s3-access` in its own `timeout`, so a hung read dies at a known budget instead of holding the step open, and corrects the two places (`hack/e2e-chainsaw/.chainsaw.yaml` and `docs/agents/e2e-testing.md`) that explained the op ceiling with a mechanism that does not hold. It caps `verify-s3-access` for sharing the shape, not for producing the 606s number. ### Which of the issue's two candidate mechanisms holds The issue named two candidates and stated the log could not separate them. The Chainsaw v0.2.15 source separates them, and it is the second one, with the exact refinement the issue proposed. **Candidate 1 — "an operation `timeout` does not bound a `script` op" — is refuted.** The op timeout is applied and does reach the process: `pkg/runner/operations/script.go:24` copies the step's `timeout` into `Timeouts.Exec`, and `:38` wraps the op context with `ctx, cancel := context.WithTimeout(ctx, tc.Timeouts().Exec)` before calling `op.Exec(ctx, …)`. So a 5m step produces a real 300s deadline on the context the command runs under. **Candidate 2 — "it bounds it and kills the shell, but Chainsaw waits on a pipe a grandchild (`mc`) still holds open" — is confirmed.** In `pkg/engine/operations/script/operation.go` the command is built with `exec.CommandContext(ctx, shell, …)` (`:105`), and in `execute` its stdout/stderr are set to `*bytes.Buffer` (`:128`–`:129`, the buffers are `internal.CommandOutput` in `pkg/engine/operations/internal/command_output.go:12`–`13`) and run via `cmd.Run()` (`:130`). Two Go `os/exec` properties then produce the overrun: `exec.CommandContext`'s default cancel is `Process.Kill()` on the shell PID alone — not a process-group kill, and the operation sets no `SysProcAttr.Setpgid` — and because stdout/stderr are not `*os.File`, `os/exec` allocates OS pipes and copier goroutines, so `cmd.Wait()` blocks until those pipes reach EOF. EOF needs every inherited write-end closed. When the shell forks `mc`, `mc` inherits those fds; at 300s the deadline SIGKILLs the shell but `mc` is orphaned, keeps retrying the S3 PUT, and holds the pipe, so the op does not return until `mc` exits on its own at 606s. `cmd.Run()` then returns `signal: killed`, which is stored in the `$error` binding and never printed — only the `($error == null): true` check failure is logged — which is why grepping the run for `signal: killed` found nothing. **Empirical confirmation.** A standalone Go program reproduces it exactly. `exec.CommandContext` with a 1s timeout, `*bytes.Buffer` stdout/stderr, running `sh -c 'sleep 2 & sleep 4'` (a descendant that inherits the pipe): `cmd.Run()` returns after ~4s, not 1s, with `err=signal: killed` and `ctx.Err()=context deadline exceeded` — the ceiling fired at 1s, control came back only when the descendant closed the pipe. Redirecting the descendants' stdout/stderr away from the pipe (`… >/dev/null 2>&1`) returns at 1.00s. That is the whole mechanism: the ceiling cuts the shell but not a descendant that still holds the pipe. ### The fix Per-command caps, the remedy the issue itself points to ("every read carries its own cap") and the shape already used elsewhere in the suite. The bucket step now defines a `k()` wrapper (`timeout 30 kubectl`) and an `mc()` wrapper (`timeout 15 mc`) for the reads, and calls `timeout 45 mc cp` directly for the two PUTs; a hung read is killed at its own budget, releasing the inherited pipe promptly, so even when the op ceiling fires the residual is one inner cap rather than `mc`'s full retry exhaustion. The terminal precedents for these budgets are the redis suite's terminal `k()` (30s, `redis/chainsaw-test.yaml:298`) and the sibling `verify-every-bucket-takes-a-write` step's terminal `mc cp` (45s, `:390`, sized for a volume grow); the 15s cap on `mc` in `hack/e2e-chainsaw/_lib/backup-access-preflight.sh` is not a precedent, because it sits inside a `while :;` retry loop where a 124 costs one attempt, whereas here under `set -e` a 124 ends the step. The inner budgets — 2×30 for the two `k` reads + 2×45 for the two `mc cp` PUTs + at most 7×15 for the other `mc` calls (eight other call sites, but the two cleanup `mc rm` are mutually exclusive) + the 30s port-forward readiness wait = at most 285s — sit under the 5m op ceiling, which stays as a backstop, per the "op above the sum of inner budgets" rule in `docs/agents/e2e-testing.md`. The long-lived `kubectl port-forward` keeps no cap by design (it must persist for the whole step, and its stdout/stderr are already redirected to `/dev/null`, so it cannot hold the step's pipe). The `mc()` wrapper is non-recursive because `timeout` execs its argument through a PATH lookup that bypasses shell functions — verified in `dash`, `bash` and `sh`. The `catch` block's diagnostic read gets the same treatment — `timeout 30 kubectl` under a 1m op `timeout` — for the same pipe reason. ### The readonly deny probe fails on a timeout The readonly-upload probe used to branch on `mc` directly (`if mc cp …; then`), which suspends `set -e`, so a `timeout` there (exit 124) was indistinguishable from the access-denied non-zero the probe expects: a 45s hang on the deny path would exit 124, take the false branch, reach cleanup and exit 0 — a long hang turned green, the exact window this PR exists to bound. This is the first `ro-user` write in the step, so no earlier call proved that path healthy. The probe now captures the status: exit 0 is the readonly regression, 124 is a timed-out probe, both fail; any other non-zero is the expected denial. ### No unit surface; why the evidence is the proof This is an e2e-config and doc-correctness change. The embedded Chainsaw script is not extracted into a unit harness anywhere in the tree, so there is no red-to-green unit test to add without inventing a bespoke script-linter (which would be a new checker standing in for the fix). The regression is instead pinned by the mechanism proof above: the source citations show the ceiling cannot cut a descendant that holds the pipe, and the caps remove the only way a descendant outlives its budget. The deny-probe status handling and the pipe-vs-file secret reads were exercised against stub exit codes (0 → regression-fail, 124 → timeout-fail, other non-zero → expected denial) and the whole script passes `sh -n` and `bash -n`. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The change touches only `hack/e2e-chainsaw/` suite content and `docs/agents/e2e-testing.md`; it moves or renames nothing under `hack/`, and touches no node-prerequisite or `package.mk` path. No downstream repository is reached. - [x] No downstream repository is affected by this change ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved end-to-end S3 access verification so stalled Kubernetes API and S3 operations fail promptly. - Correctly distinguishes timed-out access checks from expected permission denials. - Preserves partial command output and allows subsequent test steps to continue after individual timeouts. - Applies separate time limits to bucket reads, uploads, and access checks for more predictable test behavior. - **Documentation** - Clarified operation timeout behavior and recommended per-command limits within the overall operation limit. - Documented timeout handling for orphaned processes and updated worst-case timing guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
b96c822bde |
docs(agents): give the HR-Ready budget derivation its own section
The checklist entry had grown into a derivation and buried the rule it states. The rule stays in the checklist and the derivation moves to a section of its own. The derivation under-budgeted in two ways. It stated the 10m install timeout as universal, while releases an app chart renders itself take their template's spec.install.timeout, then its spec.timeout, or helm-controller's 5m where neither is set. And it restated which kinds kstatus gates on, which is easy to get wrong, so it now points at the upstream table instead. The harbor comments now name the floor each of their asserts actually sits against, and the opensearch comment stops citing a 5-6m standard the checklist no longer states. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
97cb2781db |
test(e2e): pin the release-history read against the upstream type
The test that claimed to pin the HelmRelease v2 history shape wrote a HelmRelease document containing the field names it expected, then read that same document back with the same jsonpath. It established that yq can read a fixture the test itself composed. Its own comment described the failure it was meant to catch: a future Flux release renames the field, the jsonpath matches nothing, the guard reports no remediation cycle, and real cycles pass the e2e assertion. A rename moves neither the fixture nor the expression, so the test stays green through exactly that event. The comment also named where the truth lives, github.com/fluxcd/helm-controller/api/v2, and pinned a copy of the author's expectations instead. Replace it with a Go test that supplies neither half. The expression is read out of the shell library that ships it, and the object is built from the upstream type at the version go.mod holds, then serialized and matched the way kubectl does. A renamed or retyped field is a compile error or a failed match; a guard that stops reading history fails the lookup with a message saying so. Both directions were confirmed by mutation. Locating the expression by searching the file is not enough on its own. Inline in a long script, locating it means deciding which occurrences are code and which are prose, and every way of getting that wrong fails green: an example quoted near a broken read satisfies the pin while the guard reads nothing. So the expression moves into a named assignment in remediation-guard.sh, which the guard already sources, and the test sources that library and takes the value the shell is left holding. Deciding which text is the assignment is then not the test's problem: an `export`, an indent, a second assignment later in the file all resolve the way the guard will resolve them, because the thing resolving them is the shell. Sourcing borrows the environment along with the shell, so the snippet unsets the name before it sources. `go test` hands the child its own environment, and a shell that had exported this name would answer in place of a library that stopped assigning it: the value comes back non-empty, the empty check stays quiet, and the pin passes over a guard that reads nothing. That is this file's own subject reaching it through the environment rather than through the source. Moving the expression out of the call site puts a step between what is pinned and what runs, so the call site is checked too, and the check starts from the call rather than from the text: the local the guard is handed must be assigned exactly once, by an anchored assignment whose jsonpath is the expansion of the shared name. That a well-formed read exists somewhere in the file is a weaker claim than it reads as, since a second assignment to the same local, or a read filling another one, satisfies it while the guard runs a literal nothing here pins. The expansion and not the name: kubectl echoes a braceless template back verbatim, so a read that dropped the `$` would return its own text, clear the script's empty-history backstop, and carry no status the cycle check matches. The library is checked by sourcing rather than by reading, so an assignment the shell would run is resolved by the shell instead of by a guess about which text the shell would pick. Each rule is pinned by its own test, so a regression names which one it opened rather than only that extraction broke. How kubectl's printer treats an absent path is its own configuration rather than part of this contract, so it is left unasserted: guessing at it would put the expectation and the fixture back in the same hands. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9efe78ce44 |
test(e2e): hold the tenant-cluster install wait to an ordering rule (#3802)
A test that creates a tenant `Kubernetes` cluster must not end while that release is still installing. The kind drops the install wait on its main resources, but the chart's blocking post-install hook still holds the install open, so a test that asserts and ends hands the rest of it to teardown. Teardown then reds on a step that tests nothing. Reading the cluster mid-install is fine. Ending mid-install is not. `hack/e2e-chainsaw/_lib/run-kubernetes.sh` already waits for the parent release before its tenant-cluster assertions, and that wait is older than this change. Nothing held it there. This adds the guard that does. The guard reads the script, not Chainsaw fixtures: the cluster comes from a heredoc, so a guard walking `apply.file` would find nothing to check and pass. It requires the CR, then a readiness wait on the parent release, then the assertions the test ends on. The wait has to block, and it has to be unconditional in the body that holds it. One more case holds this script to being the only creator of that kind, since a second one would be governed by no rule here. The ordering is read within one function body, so splitting `run_kubernetes_test` reds the unit lane until the rule is pointed at the new shape. Timeout length stays with review. The floor depends on where the wait sits, and the script text does not carry that. Which spellings each matcher takes, which rewrites it refuses instead of judging, and the blind spots it names are written in the guard's header next to the code, each with its mechanism and whether it fails loudly or quietly. Fixtures mark their own lines, so a probe resolves by name instead of by a line number that drifts. No runtime code here: the guard, its fixtures, and the conventions that state the rule. ```release-note NONE ``` |
||
|
myasnikovdaniil
|
41ef4a6437 |
fix(ci): teach the overlay to repair an unstamped images/*.tag ref (#4362)
## What `hack/overlay-main-images.sh` walks `images/*.tag`, but every branch of its line classifier needs either a YAML key or an `@sha256:`. A `.tag` file holds one bare reference and nothing else, so until a build stamps it the line has neither, the drift guard reads it as a non-ref change, and the committed reference survives. For a package onboarded with the `:v0.0.0` placeholder this repo already uses for a new image, that surviving reference is one no registry serves. The overlay is the only thing that would replace it for a PR that does not rebuild the package, so once such a package is on main every full-suite run that skips its build keeps the placeholder. The same placeholder inside a `values.yaml` repairs itself, because there it sits under an `image:` key the classifier recognises. The difference is the file shape, not the placeholder. ## Reproduction A fixture holding a committed bare tag against an artifact copy carrying a stamped one: ``` $ cd fixture/fakerepo && sh ../../hack/overlay-main-images.sh ../mainpkgs '[]' '' overlay: packages/system/x/values.yaml -> current-main drift (non-ref change) in packages/apps/x/images/x.tag -> keeping committed ref ``` Both files differ only in their reference. The `values.yaml` one is repaired; the `.tag` one is not. Adding any digest to the same placeholder makes it pass, which is what narrows this to the classifier rather than the file. ## Why it has not bitten yet 17 of the 19 `.tag` files in the tree carry `@sha256:` and pass on that branch alone. The only other digest-less one is `packages/extra/seaweedfs/images/seaweedfs-cosi-driver.tag`, and it survives only because it is third-party: `build-main` never rebuilds it, the artifact copy is byte-identical, and the classifier is never reached. The first digest-less first-party `.tag` is what turns the gap live, and one is coming in #3426. ## The fix `hack/promote-rewrite-tags.sh` already carries a second match branch for exactly this shape, with a comment saying why ("the second branch is what catches a bare `.tag` file, whose content has no YAML key at all"). This gives the overlay the same one, scoped to `*.tag` so an unkeyed line in a `values.yaml` still counts as drift, because only in a `.tag` file is a whole line known to be a reference and nothing else. Two tests in `hack/overlay-main-images_test.bats`: the unstamped placeholder is overlaid, and a `.tag` whose non-reference line differs still counts as drift, so the rule cannot quietly widen into "every line in a `.tag` file is a reference". Removing the fix fails the first one. ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Fixed image overlay handling for `.tag` files containing bare image references, allowing current references to be applied correctly. - Preserved drift detection when `.tag` files also contain unrelated, non-reference changes. - **Documentation** - Clarified how image-reference tooling handles bare references and package-tree processing. - **Tests** - Added coverage for bare-reference overlays and mixed reference/comment changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
ac92b6b95c |
fix(e2e): cap the bucket verify S3 reads so the step honors its timeout
A Chainsaw `script` op runs its shell under exec.CommandContext with stdout and stderr wired to in-process buffers. When the op timeout fires it SIGKILLs the shell process alone, not its descendants, and cmd.Wait() does not return until every process that inherited the pipe fds closes them. An mc that keeps retrying an S3 call after the shell is gone holds the pipe open, so the op runs to mc's own exhaustion instead of stopping at its ceiling. This is why the bucket suite's verify-every-bucket-takes-a-write step ran 606s under a 300s ceiling. verify-s3-access shares that shape, so wrap each S3 and apiserver read in its own `timeout`: a hung read dies at a known budget and the op timeout stays a backstop. The 30s kubectl cap matches the terminal `k` wrapper the redis suite carries. The two `mc cp` PUTs take 45s, the budget verify-every-bucket-takes-a-write already gives a write, because a working PUT can still wait while the master grows a volume for a freshly created bucket; 15s left too little headroom on that grow and could turn a slow-but-healthy write red on an unrelated PR. The 15s cap on the mc reads has no terminal precedent: the backup-access preflight caps `mc cp` at 15s too, but inside a retry loop where a 124 costs one attempt rather than the step, so it is not one. Land each secret's encoded value in a file before base64 -d so a timed-out kubectl read (exit 124) stops the step under set -e rather than masking as an empty-credential mc alias set further down. The readonly-upload deny probe branched on mc directly (if mc cp ...), which suspends set -e, so a timeout there (exit 124) was indistinguishable from the access-denied non-zero the probe expects: a hung deny path would exit 124, take the false branch, and reach a green cleanup, the exact window this change bounds. Capture the status instead: exit 0 is the readonly regression, 124 is a timed-out probe, both fail; any other non-zero is the expected denial. The suite's own catch read (the `kubectl get pods` in namespace cozy-objectstorage-controller) carried neither an op timeout nor an inner cap, so it inherits timeouts.exec (5m) and can hold the pipe the same way; give it a 30s inner cap and a 1m op timeout so the rule the rest of the file follows holds for it too. The .chainsaw.yaml notes still read the op ceiling as a reliable cut, the old SIGKILL as a de-facto cap and a smaller timeout as guaranteeing a kill; correct them, since the shell-only kill plus the pipe wait are why an inner cap on every read is required rather than optional. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Timur Tukaev
|
e32353e839 |
docs(agents): correct the labeler's trigger list (#4090)
## What this PR does The PR Title Auto-Labeling section still lists `edited` among the events `.github/workflows/pr-labeler.yaml` runs on. That trigger was dropped in #3211 — the workflow carries the note at the top of its `on:` block, and the v1.6.0 changelog records the change — so the document and the workflow have disagreed since. I hit it the way the document's intended reader would. Three PRs opened with GitHub's branch-derived titles, corrected afterwards to Conventional Commits, and the labels did not follow: `area/uncategorized` on all three and no `kind/feature`, because nothing re-ran. The second paragraph this PR adds is that consequence, which is the part a contributor actually needs: a corrected title does not re-label until the next push or a reopen, and since the labeler only ever adds, the `area/uncategorized` left by the first title stays until a human removes it. ### Screenshots Not applicable — no UI change. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against this diff. The only file it touches is that document, and the two triggers pointing at it are narrower than this change: `cozystack/community` restates the **commit convention** from it (types, scopes, sign-off, the `Assisted-by: LLM` trailer) and the **PR template and downstream checklist**, and `cozystack/ccp` gates its `package-bump` skill on the same commit convention. Neither restates the labeler's event list, which is what changes here. Nothing else in the map reaches a docs-only change. - [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: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Clarified when pull request title auto-labeling runs. - Documented that title edits do not trigger relabeling. - Explained that existing labels remain until a push or reopen, while additional uncategorized labels require manual removal. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
1118b340a7 |
fix(ci): teach the overlay to repair an unstamped images/*.tag ref
hack/overlay-main-images.sh walks images/*.tag, but every branch of its line classifier needs either a YAML key or an @sha256:. A .tag file holds one bare reference and nothing else, so until a build stamps it the line has neither, the drift guard reads it as a non-ref change, and the committed reference survives. For a package onboarded with the :v0.0.0 placeholder this repo already uses for a new image, that surviving reference is one no registry serves. The overlay is the only thing that would replace it for a PR that does not rebuild the package, so once such a package is on main every full-suite run that skips its build keeps the placeholder. The same placeholder inside a values.yaml repairs itself, because there it sits under an `image:` key the classifier recognises: the difference is the file shape, not the placeholder. The gap is latent rather than new. 17 of the 19 .tag files in the tree carry @sha256: and pass on that branch alone; the only other digest-less one, extra/seaweedfs/images/seaweedfs-cosi-driver.tag, is third-party, so build-main never rebuilds it, the artifact copy is byte-identical and the classifier is never reached. The first digest-less first-party .tag is what turns it live. hack/promote-rewrite-tags.sh already carries a second match branch for this shape and states why. This is that branch, scoped to *.tag so an unkeyed line in a values.yaml still counts as drift. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
5fe595d4c0 |
test(e2e): hold the tenant-cluster install wait to an ordering rule
A test that creates a tenant Kubernetes cluster cannot END while that release is still installing. A Helm release cannot be uninstalled mid-install, and for this kind the install can outlive the fast assertions: the kind carries helm-install-disable-wait, which drops the wait on the main resources but not on the chart's blocking post-install hook. A test that asserts and ends hands the remainder of the install to teardown, where it is reported red on a step that tests nothing. The rule is about ending, not about touching the cluster at all. The hook polls for the admin kubeconfig in the modes that have one, and that Secret is what the test needs to reach the tenant API, so bringing up a worker or waiting on a child release happens while the parent is still installing. Reading mid-install is fine. run-kubernetes.sh already does the right thing: it waits for the parent release to reach Ready before the tenant-cluster assertions. Nothing held it there. This adds the guard that does, and states the rule where the e2e conventions live. The guard reads the script rather than Chainsaw fixtures, because no fixture applies this kind and a guard walking apply.file would pass by having nothing to check. A case asserts that this stays the only script creating it, and the sweep behind it keys on the apiVersion and kind together so that an indented apply.resource block counts as a creator too. It reads scripts and manifests only, and that rule is pre-emptive rather than a response to what the root holds today: a manifest quoted in prose or parked in a `.disabled` file creates no cluster, and this repository writes the first shape in docs that lie outside the root. The rules it holds the script to -- what counts as a wait with teeth, where in the body it has to stand, what may not switch errexit off above it, and which call shapes count as an end-assertion -- are in the file's header beside the code that enforces them, each with the spellings it accepts and what it cannot see. How to size the budget is derived once in the e2e conventions. Nothing is added at the wait itself: hack/e2e-chainsaw/_lib is shared by every Chainsaw suite, so a comment there escalates this change from no e2e suites to the full suite, and the guard already reds on the move it would have warned about. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
975ea6acb7 |
ci(workflows): gate Talos builds on their inputs
Classify the rename-blind changed-path list against direct Talos sources and shared build inputs. This lets same-repository pull requests skip the heavy build while removals, renames, and build configuration changes still select it. Fork builds remain unconditional. Preserve the matchbox digest produced by a successful Talos build when finalize overlays base-branch image references. When the job is skipped, bootbox continues to receive the current base reference. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Andrei Kvapil
|
774f788016 |
ci(agents): check commit trailers on every pull request (#4103)
## What this PR does `docs/agents/contributing.md` makes a few properties of a commit message blocking at review: the attribution trailer takes the value `LLM` and nothing else and appears once, no message carries a session trailer or a link to a transcript, no line is a generation byline naming the tool. Nothing checked any of it, so the only reader was a human scrolling `git log`. That reader is expensive and misses things. One open PR carried eight commits naming a model in the trailer, another four, all found by hand after the work was done, each one costing a review round and a rebase. `hack/check-commit-trailers.sh` decides all of it from the text, and the `Commit trailers` job in `.github/workflows/pre-commit.yml` runs it over the commits between the merge base and the head. Over all 6062 commits on `main` it rejects 1300, every one a true positive and no prose false-fires: 1289 attribution trailers naming a model, 24 `Generated with <tool>` bylines, 15 commits carrying two trailers, two `claude.ai/code/session_...` links. ### Where it lives Next to the other per-PR text lint in `pre-commit.yml`. The unit-test job in `pull-requests.yaml` was the other candidate and is wrong for this: it is gated on the diff touching code, so a docs-only PR skips it, and a trailer is wrong or right whatever files the commit changed. The range is `merge-base..head`, recomputed every run, so a rebase or a force-push is read as it now is. It never reaches the history on `main`, roughly a quarter of which carries the old form. ### Decisions worth arguing with It fails rather than warns. A warning in a job nobody watches guards nothing. It is not a required check, and that part is deliberate. Branch protection on `main` requires `pre-commit` and `E2E Tests`, and `Commit trailers` is a new context, so a PR can still merge with it red. 77 of the 200 open PRs carry at least one commit this rejects, so requiring it today turns those red at their next push. Flipping it is a branch-protection change once the open set has drained, and somebody else's call. Fork PRs are checked like any other. The job runs on `pull_request` with a read-only token and reads commit messages as data. A merge commit with no trailer of its own passes. Nothing is required of a commit, only what is written is judged, so a merge commit that does carry a bad trailer fails like the rest. Backport branches are exempt. The backport action cherry-picks merged commits verbatim, so those messages are history and not the author's to rewrite. The exemption matches the generated name `backport-<pr>-to-<branch>`, which is the only signal a `pull_request` event carries, since the label sits on the original PR. So it is a convention, not something enforced. The checker is read from the base branch instead of the checked-out PR. A `pull_request` event takes the workflow definition from the merge of base and head, so the job shows up on every open PR the moment it lands, including the ones whose head predates the script, where reading it from the head is exit 127 and a red check saying nothing about trailers. It also keeps the checker out of reach of the PR being measured, though the job around it is not. Two shapes are left to the reviewer. A model named in `Co-authored-by:` needs a list of model names to tell it from a person, and such a list either misses next month's model or fires on somebody's colleague; most of the 771 such lines on `main` name a model, so it would be deciding a lot of PRs. A `Generated-by:` trailer has never appeared here at all. The byline rule matches a capital `G` for a related reason: case-insensitively, one line in 25 on `main` is an ordinary sentence that wrapped onto "generated with cozyvalues-gen". ### Known costs `hack/check-commit-trailers.sh` matches `hack/[^/]+\.sh$` in `select-e2e.sh`'s `full_suite_pattern`, so this PR and any later one touching the script escalates to the full e2e suite. That pattern's own comment scopes it to the e2e harness, which this script is not, and `inert_config_pattern` already carries `hack/[^/]+\.bats$`. Escalation is fail-safe so it is cost rather than breakage, and reclassifying it wants its own review. The step drops the explicit `git fetch origin "$BASE_REF"` that `api-review-gate.yaml` keeps before its own `merge-base`. `fetch-depth: 0` already fetches `+refs/heads/*:refs/remotes/origin/*`, and if that were ever wrong the failure is a loud `merge-base` error under `set -euo pipefail`, not a silent pass. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. The two entries that could fire are `cozystack/ccp` and `cozystack/community`, both keyed on a change to the commit convention itself. The convention text is untouched here, only its enforcement is new, so both copies stay accurate. `ccp`'s `package-bump` skill already writes `Assisted-by: LLM`, which this check accepts. - [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: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note ci(agents): a pull request now fails when one of its commits carries an attribution trailer whose value is not `LLM`, a second attribution trailer, a session trailer or transcript link, or a `Generated with <tool>` byline. The check reads only the commits the pull request adds; backport branches are exempt. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Pull requests now include automated checks for commit attribution trailers, session links, and generation bylines. * Backport branches are excluded from this validation. * **Documentation** * Added guidance describing the commit-message checks, accepted attribution formats, exceptions, and how to run the validation locally. * **Tests** * Added coverage for valid and invalid commit-message formats, range handling, merge commits, and exempted content. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
94a202bc5a |
test(e2e): run the merge-gating lanes on Talos containers (#4020)
Replaces QEMU substrate with Talos containers on both lanes that gate a merge. srv1-srv3 run as containers, so tenant worker sits at L2 instead of L3. Measured in CI on commits where both substrates ran the same tree: | | qemu | container | |---|---|---| | `kubernetes-latest` / `-previous` | 1143-2040s, red on every vgif-absent runner | 466-809s, green in both strata | | whole job | | 24-49 min faster | Almost all of it is in Chainsaw. Install takes about half an hour either way. Node-join soft-red gate goes with it. It was a prosthetic for nested virt: a worker that registers in minutes elsewhere could miss any deadline this test can afford, and #3513 established the discriminator is host `kvm_amd` `vgif`, not anything about the product. Remove the nesting level and there is nothing left to tolerate, so `hack/e2e-node-join-soft-red.sh`, the marker and the `soft_red` outputs are gone from all four workflows. ### What moves off the per-PR path DRBD, and with it `replicated` StorageClass, its `Immediate` binding mode, tenant StorageClass propagation (applies only to remotely-accessible classes), and cozystack talos node image with its extensions. None of it is lost - `nightly.yaml` and `e2e-tag.yaml` still run the suite on QEMU, so all four keep nightly and release-candidate coverage. Live migration is not on that list because nothing tests it today anyway. One more belongs on the list and was missing from it: the nocloud disk build. `build-talos` no longer runs `make -C packages/core/talos talos-nocloud`, so `images/talos/profiles/nocloud.yaml` is compiled by `nightly.yaml` and by the tag build's `make assets` rather than by anything a PR runs, and `promote-rc.yaml` still hard-fails a promotion whose rc release has no `nocloud-amd64.raw.xz`. Nothing in that chain breaks, because the tag build supplies the asset as before. What a PR no longer catches is narrow: it still compiles `installer.yaml` through the same imager, and the two profiles are generated by the same script and differ only in `platform`, `kind`, `imageOptions` and `outFormat`. ### Product fixes Four, all found by measurement while building the lane, each on its own commit: The linstor `drbd.enabled` gate. Without it drbd-logger sidecar exits, satellite is never Ready, piraeus registers zero nodes and every PVC hangs Pending behind a cluster that looks healthy. linstor `--strict-topology`. Without it external-provisioner passes every topology segment as `requisite`, so a node-pinned volume can be provisioned away from its pod and the pod is then unschedulable forever with nothing erroring anywhere. kubevirt-cdi importer resources are configurable now. Default restates CDI's own values exactly so production behaviour does not move, and the lane raises the ceiling through its own Package. vm-disk binds standalone disks immediately. A VMDisk on `local` silently never populated, while the same disk on `replicated` did. ### Where the run stands The lane has gone green twice, most recently on `cc70cef7b` where `E2E (in-tree)` took 2h3m against its 215m cap with 45 Chainsaw tests passing. The measurements quoted elsewhere in this description come from the first of those runs, on a commit that has since been rebased away: the two tenant-Kubernetes suites came in at 569.72s (`kubernetes-latest`) and 470.19s (`kubernetes-previous`) against a 67m operation. Both runs answer the four earlier reds, all of which were the lua-protobuf segfault from #4012 once `vminstance`/`vmdisk` were fixed on the branch. ghcr.io mirror removal is carried here too and is the same change as #4007, whichever lands first makes the other a no-op. ### What review changed `drbd.enabled=false` now refuses to render together with `talos.enabled=false`. It removed the logger sidecar only, while the two DRBD initContainers piraeus contributes unconditionally are deleted by the Talos configuration alone, so that combination produced a satellite that can never reach Ready. `isp-full-generic` sets `talos.enabled=false`, so it was reachable and not theoretical. vm-disk keeps unconditional immediate binding, a standalone disk has no consumer to wait for. What it costs on a node-pinned class is in the chart README now, and the annotation is pinned per source type and on the existing-DataVolume path where `helm upgrade` retrofits it. The rest is the harness holding its own promises: HelmRelease gate reads are bounded and its minimum-count half is pinned by a test that fails without it, two Chainsaw ops sit above the deadlines they contain, the fallback-StorageClass render restores `replicated` when it fails, container lane compose project and pool files are per-checkout, and three comments that said the opposite of the code now say what is true (QEMU is still wired into nightly and e2e-tag, the lane replaces two Packages and not one, the argument-count assertion cannot see upstream drift). ### Release note ```release-note fix(linstor): CSI provisioning is constrained to the node the scheduler chose (`--strict-topology`), and the DRBD sidecar can be switched off with `drbd.enabled` on substrates whose kernel cannot run DRBD. On an existing cluster the new default re-templates `spec.csiController.podTemplate`, so piraeus rolls `linstor-csi-controller` once during the upgrade. `drbd.enabled=false` requires `talos.enabled=true` and the chart now refuses the other combination at render time, because only the Talos satellite configuration removes the DRBD initContainers. fix(kubevirt-cdi): CDI worker pod resources are configurable through `importerResources`, which reaches the importer, the uploader and the host-assisted cloner. The default restates CDI's own built-in values exactly, so an install that sets nothing behaves as before. fix(vm-disk): every disk requests immediate binding, not only `upload` sources, so a standalone disk on a WaitForFirstConsumer StorageClass is populated instead of waiting for a consumer that may never arrive. On a node-pinned class such as `local` the volume then binds where CDI's worker pod was scheduled rather than where the VM will run; see the chart README before choosing such a class for VM disks. The default `replicated` is unaffected. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added container-based end-to-end testing with local storage support. * Added optional backup-access preflight checks for database backup examples. * Added configurable CDI worker resources and LINSTOR storage settings. * Added configurable immediate storage binding for VM disks. * Added stronger readiness checks, diagnostics, and parallel test execution. * **Bug Fixes** * Improved cleanup failure reporting, capacity validation, and missing-image detection. * Improved FoundationDB health verification and storage placement behavior. * **Removed** * Removed GHCR mirror support and soft-red node-join tolerance. * Removed standalone OIDC render test suites. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
d45663f1af |
ci(agents): check commit trailers on every pull request
The attribution and session-link rules in docs/agents/contributing.md are decided by the text of a commit message alone, and until now the only reader was a human scrolling git log. That reader is expensive and misses things: one open pull request carried eight commits naming a model in the attribution trailer, another four, all found by hand after the work was otherwise done. Every one of them cost a review round to report and a rebase to fix. The check reads what is written and requires nothing, so a commit with no trailer passes and a merge commit that carries none passes with it. A trailer that exists and is wrong fails, naming the commit and the accepted form. It runs over the commits between the merge base and the head, so it never reaches the history on main, roughly a quarter of which predates the rule. A backport branch is exempt: its commits are cherry-picked verbatim and their messages are not the author's to rewrite. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
c748c5b707 |
docs(e2e): drop the references the removed suites and carve-out left behind
Convention 7 still sent readers to the node-join carve-out, while section 1 of the same file -- rewritten on this branch -- says the script, the marker and the soft_red output no longer exist. Restated as what the deadline now is. hack/e2e-chainsaw/README.md still listed kubernetes-oidc-system and kubernetes-oidc-customconfig among the suites, and select-install.sh's suite_to_source() still mapped both. The mapping is harmless today because the round-trip pin walks existing directories, but select-e2e.sh says the two functions have to stay in step, so a stale half is a trap for whoever reads that promise. sandbox-runner-headroom.bats keeps reading the QEMU sandbox file for every lane, container lanes included: `prepare-env` also matches `prepare-env-container`, and the compose file gives its nodes the same 8 CPU / 24576 MiB by design. The figures are therefore right for both substrates, but only because the two agree -- and that they agree is pinned in container-lane-capacity_test.bats rather than here. Said so in the test, so the next reader knows what would have to change if the container lane is ever sized differently. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: LLM |
||
|
Myasnikov Daniil
|
12157a2f93 |
docs(agents): describe the container lane as the merged standard
The container lane moves out of "In-flight direction (not yet the merged standard)" and into the body, because it is now what both merge-gating lanes run. The node-join section is rewritten the other way round: it described a deadline that failed the suite without blocking the lane, and there is no longer any such carve-out to describe. What the section gains is the part that was missing while the lane was a spike -- what moves off the per-PR path with the substrate, and where it still gets covered. DRBD and the replicated StorageClass were already named; Immediate binding, tenant StorageClass propagation and the Cozystack Talos node image were not, and all three now say nightly and release-candidate rather than reading as lost. The host CPU and memory visibility gap is stated too, since it is what a reader meets first on a red run and it looks like a product bug. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Marian Koreniuk
|
f8abaa6249 |
docs(agents): correct the labeler's trigger list
The auto-labeling section still lists `edited` among the events the labeler runs on. That trigger was removed in #3211, and the workflow carries a note explaining why: it fires on every body edit, so long-lived bot PRs re-ran the job dozens of times on an unchanged commit. The gap is not cosmetic for the reader this document is written for. Acting on it means correcting a wrong PR title and expecting the labels to follow, which they do not until the next push — and since labels are only added, never removed, the `area/uncategorized` the first title produced stays behind either way. Signed-off-by: Marian Koreniuk <moriarti@cp.if.ua> Assisted-by: LLM |
||
|
Aleksei Sviridkin
|
3a369fed99 |
feat(redis): add opt-in TLS for Redis and Sentinel (#2729)
## What this PR does `tls.enabled` puts Redis and Sentinel on a TLS-only listener with a per-release cert-manager chain, and hands the tenant the CA certificate as `redis-<name>.tenant-ca`. Off unless set. redis-operator v3.3.5 has no TLS support. The work is ported upstream in freshworks-oss/redis-operator#85, which is open and not in a release, so this carries it as three patches over the v3.3.5 tarball. `images/redis-operator/patches/tls.diff` is the source half, cut from that PR's commit `2ad2ed8d1`, which the patch header pins; it is applied by the glob the image Dockerfile already runs, after the Dockerfile strips the tarball's `manifests`, `charts`, `example` and `.github`, so the image builds from source alone and the upstream tests that read the CRD YAML skip instead of failing against the stale v3.3.5 copy. `patches/crd-tls.diff` adds `spec.tls` to the vendored CRD, and with it the CEL rule that fixes `tls.enabled` at creation; without the schema block the apiserver prunes the field on admission and the patched binary waits for something it never sees, and the rule cannot exist without the block it references. `patches/rbac-tls.diff` grants Create, Get and Update on Secrets, so the operator can publish the CA-only Secret it addresses by name. That is the whole delta, and it is narrower than the ported operator's own chart asks for: those three are the only Secret calls in the patched source, and cert-manager Certificates get no grant at all, because the operator only touches one under `spec.tls.certManager` and this chart pins `certificateSecret` mode instead. One change in `tls.diff` is not TLS and takes effect with TLS off, named in the patch header: `MakeSlaveOfWithPort` connects to the replica on its own port rather than on the master's. It changes a dial, not a pod template, so shipping the rebuilt image rolls nothing; the plaintext pod templates stay byte-identical to v3.3.5. The image build registration this branch used to carry (`make -C packages/system/redis-operator image` in the root `build:` target) landed on main with the Valkey service (#3380), so the rebuilt operator image is produced by CI from main as well; the digest in `values.yaml` is still refreshed by release preparation, not here. The chart patches re-apply at the end of `update:`, the same way reloader, grafana-operator, ingress-nginx, seaweedfs and kamaji carry theirs. Otherwise `rm -rf charts` takes TLS back out on the next `make update` and nothing fails. `make update` reproduces the committed tree byte for byte. TLS is chosen when the instance is created. The config gets `port 0` and the listener moves to `tls-port`, so TLS replaces plaintext rather than running beside it, and a running instance cannot change mode: a replica restarted onto the TLS listener cannot replicate from a master still on plaintext, and Sentinels already on TLS would fail over to a stale replica. The RedisFailover schema in the carried CRD patch rejects a change of `tls.enabled`, which on an existing instance surfaces as a failed HelmRelease upgrade while the instance keeps serving what it did; the README points at creating a new instance and migrating. That is also why TLS is not inferred from `external`: an externally reachable instance is the one most likely to have clients an upgrade must not drop. Existing releases keep their plaintext listener and are not restarted by the rebuilt operator image. The tenant gets `ca.crt` and nothing else, through `tenantsecrets`. The chart's own Secrets take a dot between the release and the suffix (`<release>.ca-tls`, `<release>.tls`, `<release>.ca-cert`): the suffixes are dot-free, so the split is unique and release `foo`'s CA Secret can no longer be release `foo-ca`'s leaf, the collision the hyphenated form had. The chart renders a `TenantProjection` pointing at `<release>.ca-tls`, and the CA-extraction controller re-encodes the one named key from the parsed certificate, so nothing that is not a certificate gets through. Same path as postgres, mongodb, nats and qdrant. No Secret of this release is granted to the tenant by name beyond the generated password, and that includes `<release>.ca-cert`, the CA-only copy the operator publishes for itself. It holds no key today, but the operator has Create and Update on Secrets cluster-wide, so what sits at that name is the operator's call rather than the platform's, and RBAC cannot filter by key. `hack/check-redis-rd-secrets.bats` pins the whole rendered Secret surface instead of a list of known-bad names, because an include with no `resourceNames` and no `matchLabels` restricts nothing. The chainsaw suite for redis gains two cases. A TLS fixture asserts the cert-manager chain is issued, the TLS-probed Redis replicas and Sentinel come up, the operator publishes its CA-only Secret under the widened ClusterRole, the tenant receives exactly `ca.crt` through `tenantsecrets` while the key-bearing Secrets stay off that path, and a `redis-cli --tls` PING through the service name completes with the same CA. A second case applies `tls.enabled` to a running plaintext release and asserts the refusal: the HelmRelease upgrade fails with the schema's message, the StatefulSet keeps its revision with both replicas Ready, plaintext still answers and no TLS listener appears. Certificate renewal rolls the pods from inside the operator. Redis and Sentinel read the certificate once at startup, so the patched operator stamps a hash of the TLS Secret's `tls.crt` and `ca.crt` into the pod template of both the Redis StatefulSet and the Sentinel Deployment on every reconcile, as `redis-failover.freshworks.com/tls-secret-hash`. A renewal changes the hash; the operator's own roller replaces the Redis pods one at a time and the Deployment controller rolls Sentinel. README names the fallback: if the roll cannot run, the deadline is the old certificate's expiry, past which the operator's own TLS connections fail along with the clients. `tls.authClients` maps to `tls-auth-clients` and defaults to `no`. The platform mints no client certificate for the tenant and the CA key is out of reach, so `yes` is only usable when the client certificate comes from whoever also controls the CA. Changing the value on a running release restarts the Redis and Sentinel pods: Redis reads the directive from its config file only at startup, so the operator folds the effective value into the certificate hash that drives the rollout, and the change lands when it is made rather than at the next unrelated restart. Follow-ups in the carried operator patch go upstream first, to freshworks-oss/redis-operator#85 or a successor there, and are picked up here afterwards; the carried patch does not grow Go that upstream has not seen. `tlsConfigFor` fetches the Secret on every call, tens of GETs per reconcile. The renewal roll is static analysis so far: the live check, `cmctl renew` on a running release while watching the `rfr-*` and `rfs-*` pods roll, is still owed and belongs with the e2e follow-up. Two facts about the shipped image rather than the patch. `packages/system/redis-operator/values.yaml` keeps the digest of the image built before the patch until release preparation rebuilds it, so on `main` between this merge and the next release-prep, `tls.enabled` renders the chain and `spec.tls` while the running operator ignores them and serves plaintext. And the pinned v3.3.5 tarball carries GO-2026-5026 in `golang.org/x/net` v0.53.0, reached through `PodService.UpdatePodLabels`; the port neither introduces nor fixes it, upstream `master` is already on v0.55.0, so it lands with the next tag bump rather than here. Outside the patch: `cozyvalues-gen` renders `{}` in the README Value column for `tls.authClients`: a child property without a default inherits the parent object's `{}`, and `make generate` reproduces it byte for byte, so it is a generator bug rather than something to hand-edit here. ### Screenshots No UI changes. ### Downstream repositories - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: cozystack/terraform-provider-cozystack#37 - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: I walked the trigger map against the diff file by file. terraform-provider-cozystack is reached, tracked as cozystack/terraform-provider-cozystack#37 rather than a speculative PR: `values.schema.json` gains a `tls` object, and the provider hand-writes a schema, model and expand/flatten pair per Kind. It already models `tls` for postgres, kafka, nats and qdrant, not for redis. Redis needs different wording from those four, whose descriptions say to omit the block to follow `external`, which is the one thing redis does not do. Nothing else is reached. The website regenerates this package's reference page from `README.md` on a stable tag, so the README changes need no PR there. No package added, renamed or removed. `packages/core/platform/values.yaml`, variant and bundle composition and release asset names are untouched, and so are `hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh`, which is what ccp and external-apps-example read. The two new files under `hack/` are bats unit tests. No node prerequisite, installer value, telemetry metric or cozy-proxy label changes. ### Release note ```release-note feat(redis): add opt-in TLS for Redis and Sentinel. Set `tls.enabled: true` to serve both on a TLS-only listener backed by a per-release cert-manager chain; the tenant reads the CA certificate as the `<release>.tenant-ca` tenant secret. TLS is off unless set and is not inferred from `external`, so existing releases keep serving plaintext and are not restarted by the rebuilt operator image. TLS is chosen at creation: a running instance cannot switch modes, the change is rejected and surfaces as a failed HelmRelease upgrade, so add TLS by creating a new instance and migrating to it. Requires cert-manager, which the platform's `system` bundle installs on every variant. A renewed certificate rolls the Redis and Sentinel pods through the operator. TLS support is carried as a downstream patch over redis-operator v3.3.5 until freshworks-oss/redis-operator#85 lands in a release. ``` |
||
|
Aleksei Sviridkin
|
61026b732c |
docs(agents): require the neutral Assisted-by: LLM trailer
The attribution trailer exists for disclosure, not to advertise a model. A model name in the trailer tells a reviewer nothing they can act on, and it goes stale with every tool change, so the accepted form is a single `Assisted-by: LLM` line however many tools took part. A link to an assistant session is worse still: it rots, and it carries nothing about the change itself, so no session trailer and no transcript URL belong in a commit, a PR description, or a comment. The commit body explains why a change is made; the diff already shows what changed. The hygiene rules for messages, trailers and in-code comments are written down as review blockers for the same reason: a NOT LGTM on one of them is then a stated rule, not a reviewer's taste. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d2fcf41fb8 |
docs(agents): drop redis-operator from the build-target violations
The root Makefile build: list now carries the redis-operator image, so the package stamps a reference from inside the list every other producer iterates, and the image-refs rule no longer names it as a live violation. flux-plunger, keycloak-operator and kilo still are. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
IvanHunters
|
0fa6cf72a5 |
chore(ci): complete the backport label rename across all consumers
The initial rename missed several literal consumers of the old label names beyond labels.yml and backport.yaml: - stale.yaml exempt-pr-labels still listed backport,backport-previous. actions/stale matches label names exactly and does not resolve label-sync aliases, so after the rename a kind/backport PR would lose its stale exemption and get auto-closed after inactivity. This is the one behavioural regression; the rest are documentation drift. - PULL_REQUEST_TEMPLATE.md told contributors to apply the backport label. - release.md label-target table and the gh pr list --search commands referenced the old names (the searches would silently return nothing). - contributing.md claimed a backport label is auto-applied on the [Backport] prefix; pr-labeler only adds area/release, so the line was already stale and is corrected while renaming. Historical narrative in release.md and the backport scope-to-area doc rows (a Conventional-Commit scope, not the label) are left as-is. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
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> |
||
|
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 -->
|
||
|
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> |
||
|
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> |
||
|
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 --> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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 --> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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 --> |
||
|
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 -->
|
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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>
|
||
|
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> |
||
|
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 --> |
||
|
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 ``` |
||
|
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> |
||
|
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 --> |