mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 05:25:14 +09:00
ace69cb0750d2fa94f45b415e1ea10cbf14715bf
169
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
a7ac37be65 |
test(e2e): let HR-Ready waits outlast the install timeout of the release they wait on (#4471)
## What this PR does Stacked on #4468, which holds the raised backup waits to a start margin; review that one first. This PR's own diff is the six commits on top of it. Most HR-Ready waits in the E2E suites gave up before the release they were waiting on could fail its install. cozystack-api gives every release it generates a 600s install timeout, and the Chainsaw asserts in front of those releases sat at 5 or 6 minutes. When an install genuinely failed, the suite reported a bare assert timeout against a release that was still installing, and the reason the install failed, which the release records when its own timeout fires, was never reached. Red runs failed just as often as they should; they only said less. I raised every HR-Ready wait above the install timeout of its own release, by the 60s start margin #4468 introduces: - Chainsaw asserts on releases cozystack-api generates go to 11m, across bucket, clickhouse, etcd, external-dns, foundationdb, harbor, mongodb, openbao, opensearch, postgres, qdrant, rabbitmq, redis, valkey and vminstance. - Asserts on nested `<name>-system` releases take their floor from the template that renders them: 11m for external-dns, openbao and qdrant (10m `spec.timeout`), 16m for `harbor-test-system` (15m). `harbor-test` sat exactly on its 600s floor, which expires first because the assert's clock starts before the install's; it goes to 11m too. - The kuberture Package release takes its 10m from cozystack-operator and goes to 11m. - In the backup walkthroughs, the bucket waits go from 300s to 660s in clickhouse, kafka, mariadb, mongodb, postgres, rabbitmq and redis, and the two postgres waits go from 360s to 660s. The kafka bucket wait passed no budget and silently took the helper's 300s default, which is why earlier scans for literal budgets missed it. Each Chainsaw op that runs a walkthrough grows by exactly what its waits gained (clickhouse 78m, kafka 46m, mariadb 48m, mongodb 42m, postgres 66m, rabbitmq 48m, redis 36m), so a failure at any later wait keeps the headroom it had and is still reported by the script instead of being cut off by the op. Two waits stay as they are, on purpose: - kafka: `kafka-rd` sets a 7h install timeout, which no budget inside the 215-minute E2E job can clear. The kafka assert and the Kafka waits in the kafka and kafka-metadata walkthroughs keep their numbers, and the assert now says why in-line. - `vm-instance-test`: `vm-instance-rd` disables the install wait, so its release goes Ready as soon as helm has applied and there is no floor to clear. Earlier lists of this problem included it by mistake. The cost falls on red runs only: a suite whose release never goes Ready now spends up to six more minutes before it reports, and in exchange the report carries the release's own reason. Green runs are unchanged. Suites run one after another in a single job capped at 215 minutes, so a systemic failure that reddens many suites now reaches that cap sooner, and a job cancelled at the cap loses its log and `cozyreport.tgz`. I took that trade because a failed install that says why is worth more than a red run that ends a few minutes earlier. `docs/agents/e2e-testing.md` stopped being true in several places once the waits were raised: it described most of the tree as under the floor, mariadb as the only suite raised, the walkthrough bucket waits as under it, and harbor's asserts as sitting on and under theirs. I rewrote those sentences, and the one that pointed at "every wait above the band" of 5-6m, kept the start-margin wording from #4468, and added that a walkthrough wait passing no budget takes the helper's default. The comments in `hack/mariadb-first-boot-waits.bats` that quoted the old op and bucket figures are restated; its existing checks and constants are unchanged, because every op grew by exactly its bucket delta and each remainder still sits at exactly its constant. I also added postgres to the table that file holds the walkthroughs to, so its raised waits and its op cannot drift back apart unnoticed, and a test that holds each table suite's bucket wait to the bucket release's floor, read from the api-server default with `bucket-rd` checked for an override; the op check alone could not catch a bucket wait dropping back, since a lower wait only widens the slack. redis stays out: its walkthrough waits on no redis HelmRelease, only on the bucket and on the sentinel master, so the table's floor check has nothing to read there. How I checked it, since E2E cannot run locally: - I re-ran the enumeration from the sizing section of `docs/agents/e2e-testing.md` over every `hack/e2e-chainsaw/*/chainsaw-test.yaml` and every `wait_hr_ready` call under `examples/backups/`, literal or default, and held each against its own release's floor. Every `Ready=True` assert and every `wait_hr_ready` call a suite drives now clears its floor, except the kafka ones named above. The opt-in etcd round-trip uses bare `kubectl wait hr` calls at 300s instead of that helper; they are listed in #4470. The `Ready=False` assert on the rejected redis upgrade is not a readiness wait and is left out. - `make bats-unit-mariadb-first-boot-waits` passes, 31 of 31. For the new postgres row, putting the op back to 50m fails only the postgres slack test, and a postgres wait at 659s fails only the postgres floor test. Setting any one table suite's bucket wait to 659s fails only the bucket test, naming only that suite, and an install-timeout override on `bucket-rd` fails it too. I ran the mutations under plain `bats` so a fail-fast runner could not hide a second failure. - `hack/select-e2e.sh` over this diff selects bucket, clickhouse, etcd, external-dns, foundationdb, harbor, kafka, kuberture, mariadb, mongodb, openbao, opensearch, postgres, qdrant, rabbitmq, redis, valkey and vminstance. Out of scope, and tracked in #4470: the tenant addon waits in `hack/e2e-chainsaw/_lib/run-kubernetes.sh` (an edit there escalates the whole E2E run), the redis and kafka-metadata `wait_hr_ready` helpers that print no reason on timeout, and the opt-in etcd round-trip's bare `kubectl wait` calls. Known and left for later, none of them changed here: - Only the mariadb Chainsaw asserts and the walkthroughs in the guard's table are held by a test. The other raised asserts, and the redis and kafka bucket waits, rest on literals with in-line reasons; #4470 covers extending the guard. - The Kafka waits in `examples/backups/kafka/04-create-kafka.sh`, `07-restore-to-copy.sh` and `kafka-metadata/run-all.sh` stay at 300s against the 7h floor without an in-line reason at the call site. The doc's rule asks that of Chainsaw asserts only, and the kafka Chainsaw assert carries it. The postgres waits are the ones #3748 is about. Their budget now clears the install-timeout floor and a test holds it there; what a CNPG first boot costs on this platform is still unmeasured, so that issue stays open. Closes #3771 Closes #3753 ### Screenshots Not a UI change. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff. It touches E2E suites, backup walkthrough scripts, a unit-lane bats file and an agent doc, none of which any downstream repository restates. - [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): HelmRelease readiness waits in the E2E suites and backup walkthroughs now outlast the install timeout of the release they wait on, so a failed install reports its own reason instead of a bare timeout. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Extended readiness and backup round-trip test timeouts across multiple services to accommodate slower startup and provisioning. * Added checks to verify that bucket readiness waits meet configured installation-timeout requirements. * **Documentation** * Updated testing guidance to clarify how readiness waits relate to release-specific timeouts and overall test budgets. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
2d0c359f4c |
test(e2e): stop bringing up the monitoring instance nothing looks at (#4023)
## What this PR does This PR turns off monitoring instance of root tenant in e2e install and removes waits on it. Install patched root tenant with `monitoring: true` and then waited up to 15m each on vmalert and vmalertmanager, `vlclusters/generic`, shortterm and longterm vmclusters, `grafana-db` CNPG cluster and grafana deployment, but nothing in the run looks at any of it. No chainsaw suite touches tenant-root monitoring objects. Only one that mentions monitoring is `foundationdb`, it asserts `WorkloadMonitor` whose CRD comes from `cozystack-controller` and exists without monitoring stack. Monitoring OIDC wiring is covered by `packages/system/monitoring/tests/oidc_test.yaml` since #4019 removed orphaned BATS suites. Kubernetes suites run with `monitoringAgents.enabled: false` and tracing is off by default (`tracingStorages` is empty), so they don't depend on this instance either. Those waits were not free. Monitoring stack competes with rest of install for node capacity, and removed block own comment said slow victorialogs start can fail install on PR that never touched monitoring - that's why it was moved to 15m budget. ### What stays Monitoring package stays installed, because `tenant-application` has `dependsOn cozystack.monitoring-application` and without the package platform release is never resolved. Only tenant-root instance is off. Container lane `logsStorages` override for victorialogs also stays. It does nothing while monitoring is off, but keeps a run that turns monitoring back on from creating immutable replicated PVC on lane that has only `local`. Its comment, paragraph in `docs/agents/e2e-testing.md` and comment on Install step env in `pull-requests.yaml` now say that instead of calling root monitoring install-time workload. ### What is lost `curl` through `root-ingress-controller` with `Host: grafana.example.org`, it was the only check that root ingress actually routes to backend. Grafana was the target just because it was installed. Moving it to seaweedfs host needs expected response checked on live stand first, so it's tracked in #4615. ### Savings Not measured. The happy path never spent the 15m budgets, so what this returns is real bring-up time of the stack plus tail risk, and only CI runs can show how much it is. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note NONE ``` |
||
|
Aleksei Sviridkin
|
67e42bbbc0 |
docs(agents): stop describing the HR-Ready waits as under their floor
The sizing guidance said most of the tree sat under the install-timeout floor, that mariadb was the only suite raised, that the walkthrough bucket waits were under it, and that harbor's asserts were on and under theirs. None of that holds once the waits clear their floors, and a reader would take the old band as the norm to copy. Name kafka as the floor no budget can clear, keep harbor as the example of two floors in one suite, and note that a walkthrough wait passing no budget takes its helper's default, which a grep for the number misses. The list of definitions that disable the install wait also missed vm-instance-rd, which is why its assert has no floor to clear, and the section never said where a Package release's floor comes from. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
94b328c6eb |
test(e2e): hold every raised backup wait to a start margin
The wait-budget guard accepted a wait one second over the install timeout, although the wait's clock starts before the install's by the Application reconcile, the chart fetch and the helm-controller queue. A wait lowered from 660 to 601 passed every test and brought back the bare assert timeout the budgets exist to avoid. A wait now has to clear its floor by a named 60s margin. Nothing in the tree bounds those three delays, so the value is policy: the headroom every current wait already carries. The sizing guide in docs/agents/e2e-testing.md asked only for "strictly above", which the guard now rejects; it points at the margin instead of restating a number. The mongodb, rabbitmq and clickhouse walkthrough waits were raised for the same reason as mariadb's, and their Chainsaw ops grown with them, but nothing held either. Each suite is now a row in one table and gets its own floor and op tests, so a regression reds only its own suite. Floors come from the api-server install-timeout default and, for mariadb, the chart's startup probe; each suite's RD is checked for an override instead of being assumed not to carry one. Each op keeps the remainder it had before the raise over every HelmRelease wait it reaches, the bucket one included, so raising a wait and its op together keeps the remainder instead of widening the slack. The mongodb target behind SKIP_RESTORE=1 is not reached and is left out until the op stops passing it. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
312409ecd2 |
test(e2e): fail the suite whose pods outlive its cleanup (#4492)
## What this PR does A Chainsaw suite could pass while the workloads it created kept running after its own cleanup, and nothing noticed until a later suite failed. On one E2E run the brokers of four Kafka releases from suites that had passed and deleted their applications (#4488), plus a TenantControlPlane sitting on its finalizer with two apiserver pods (#4489), pushed pod memory requests to the node limit. Harbor then failed on `Insufficient memory` two suites later, under the wrong name. This makes the suite that leaked fail, and name what it left behind. - Chainsaw v0.2.15 has no hook that runs after a test's cleanup: its `Configuration` carries `error.catch`, which runs only on failure. So `make test-chainsaw` now runs `hack/e2e-chainsaw-run.sh`, which invokes `chainsaw test` once per suite directory and checks the cluster between invocations with `hack/e2e-chainsaw/_lib/leak-guard.sh`. - Attribution: a pod is charged to a suite when all three hold. It is not terminal. Its UID was not in the pod list taken before the suite. The root of its ownerReferences chain was created at or after the suite started. A platform pod replaced mid-suite by a rollout, an eviction or a crash traces back to an object the install created, so it is not charged. A pre-existing pod adopted by a controller recreated mid-suite is excluded by the UID baseline. A leak is charged once, to its own suite. - The suite start is read off an apiserver, from the `creationTimestamp` of a server-side dry-run create, rather than off the sandbox. With several control-plane nodes the reading and a root's timestamp can come from different apiservers, so the rule assumes their clocks agree more closely than the gap between the reading and the suite's first object; no skew margin is added. Source for that behaviour: the generic registry `Store.create` stamps it through `rest.FillObjectMetaSystemFields`, then `DryRunnableStorage.Create` copies it back without persisting (`staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go` and `dryrun.go` at Kubernetes v1.33.0). - Budget: charged pods get 300s to disappear, the default `timeouts.cleanup` in `.chainsaw.yaml`; a unit test keeps the two equal. A suite that overrides its own cleanup timeout (kafka sets 8m) is covered the same way, because the check runs only after `chainsaw test` has returned, which is after every cleanup in the suite has finished under its own timeout; the 300s start from there. The leaks this targets sat about 110 minutes (Kafka brokers) and 30+ minutes (the TenantControlPlane) past their suites. Every green suite logs `leak-guard: suite=<dir> clear after Ns`, so the number can be checked against later runs. - Past the budget the run prints `LEAKED-PODS suite=<dir> budget=300s count=N`, then one line per pod with its phase, deletion state, root and `app.kubernetes.io/instance`. It raises a `leaked-pods` error annotation and adds a failed `leak-guard` test case to `chainsaw-report.xml`. The run continues through the remaining suites and exits non-zero at the end, as the single invocation did on a red test. - A suite that already failed on its own is checked once without the wait. Its leftovers are reported as a `::warning` with `LEAKED-PODS suite=<dir> checked-once=chainsaw-failed`, since some may still be tearing down. - Each pod gets one `kubectl wait` inside the remaining budget, and nothing is retried. A timeout or a spent budget gives the verdict from the last good pod list, also when the re-list after it fails, so a leak fails the suite. A wait that fails only after running for its whole `--timeout` counts as a timeout whatever it printed: kubectl maps an expired context to "timed out waiting for the condition" only inside the watch, so a deadline that passes during its initial lookup surfaces as "context deadline exceeded" (`wait.go` and `delete.go` in kubectl v1.33.0). A spent budget is caught before calling kubectl and never passed as `--timeout=0`, which means "check once". - Any other error on a wait or a re-list, before the budget is spent and before any timeout, stops the waiting and is reported as `LEAKED-PODS suite=<dir> count=N inconclusive="<error>"`. That is a warning, not a leak failure, so an apiserver blip cannot become a leak red. - A failed first reading after a suite, including a pod list that cannot be parsed, leaves the suite unchecked. It fails with a `leak-guard` annotation and a report case saying the cluster could not be read, not that it leaked. Errors that are not leaks carry the annotation title `leak-guard`, not `leaked-pods`. - Known leaks are listed in `hack/e2e-chainsaw/_lib/leak-allowlist.txt` as `<suite-dir> <issue-ref>`: `kubernetes-latest` and `kubernetes-previous` against #4489 (both suites create a TenantControlPlane through `_lib/run-kubernetes.sh`). `kafka` and `kafka-metadata` were listed against #4488 until #4490 fixed it; those entries are gone. An allowlisted suite prints its leak as a `::warning` and does not fail. An entry that names a directory that is not a suite stops the run before the first suite, so a rename cannot keep an exemption alive. The PR fixing each issue deletes its lines. Suite selection is unchanged. The runner runs the named directories when there are any, and otherwise every directory directly under `hack/e2e-chainsaw/`, each handed to Chainsaw's own discovery; it refuses a tree with a test at its top level, where that split would stop being exact. I verified this with Chainsaw v0.2.15's own `discovery.DiscoverTests`, built from source at that tag. For the full tree, for `kafka redis harbor` and for `kubernetes-latest`, the union of the per-suite discoveries equals what a single `chainsaw test [suites]` discovers: 57, 7 and 2 tests, identical lists. 57 matches the test count in an earlier full run's report. A unit test pins that the runner passes chainsaw the suites and flags it was given, in order. The order changes too: suites used to run in Chainsaw's own order and now run one directory at a time in alphabetical order, which moves the kubernetes suites after the kafka ones. Before #4490 fixed #4488 this starved them (see the E2E criterion below). With the fix merged the order has no effect, and a future leak that lands before a memory-heavy suite is named at the suite that leaked. One behaviour does change: a selected name that is not a directory used to fail the single invocation before any test ran; now only that suite fails and the others run. `chainsaw-report.xml` keeps its path and shape: one `<testsuites>` root with the summed counts, one `<testsuite>` per suite directory. Its consumers are the `Collect chainsaw report` / `Upload chainsaw report` steps in `pull-requests.yaml`, `e2e-fork.yaml`, `nightly.yaml` and `e2e-tag.yaml`, which `docker cp` the file and upload it as an artifact. Nothing in the repository parses it. The merge is unit-tested on two per-suite reports in Chainsaw's real shape, one of them failed, and the result is checked to parse as XML. Tests: `hack/chainsaw-leak-guard_test.bats` (31 tests, in `make unit-tests`; named without the `e2e-` prefix, which the unit lane excludes) covers the attribution rule, the budget, the allowlist, the merge and the runner, all against stub `kubectl` and `chainsaw`. Every rule was mutation-checked: removing the guard call, having the Makefile bypass the runner, inverting the allowlist match, flipping or tightening the root-timestamp comparison, removing the UID baseline, removing the terminal-phase filter, skipping the budget wait, changing the budget, lowering or raising the depth cap, accepting a stale allowlist entry, reading an unreadable pod list as empty, dropping the leak case from the report, skipping directories in the selection, treating any failed owner read as a missing owner, dropping the separate report for a suite that already failed, dropping the not-run reason from the report, reading any wait error as a timeout, removing the inconclusive branch, letting a failed re-list after a timeout turn a leak inconclusive, ignoring a failed re-list, reporting an unreadable cluster as a leak, reading an unparsable pod list as clean, reporting a kubectl warning instead of the error, passing the report name without its extension, wording a failed re-list as a failed wait, dropping an allowlist's last line when it has no newline, dropping the check that `--set-string` carries a value, and reading a wait that ran its whole timeout but printed a context error as inconclusive. Each one turns its named test red, and restoring turns it green. E2E criterion, registered before the run: in the `Run E2E tests` log of this PR, every suite that ran prints `leak-guard: suite=<dir> clear after`, a `LEAKED-PODS suite=<dir>` line carrying `allowlisted=`, or one carrying `checked-once=` (a suite that failed on its own) or `inconclusive=` (waiting stopped on a failed read), and no line matches `LEAKED-PODS suite=.* budget=`. With #4490 merged, `kafka` and `kafka-metadata` are no longer allowlisted and must print `leak-guard: suite=kafka clear after` and `leak-guard: suite=kafka-metadata clear after`, which is the live check that #4490 fixed #4488. The first run of this PR (before #4490, run 36223896793) is the positive control: it printed `LEAKED-PODS suite=kafka count=15` and `suite=kafka-metadata count=8`, allowlisted, and every other suite cleared in 0-37s. That run also went red on both kubernetes suites, because those 23 leaked brokers left no memory for the second tenant worker VM (`0/3 nodes are available: 3 Insufficient memory`), which the order change described above made possible. The guard has not yet run against a real cluster, so the first run of this PR should carry the `full-e2e` label; its `clear after` lines are the first measured teardown times, and any unexpected `LEAKED-PODS` line is either a real leak or a false positive to understand before merge. Follow-ups, known and out of scope here: - The allowlist exempts a whole suite, so a new leak in an allowlisted suite stays hidden until the issue's fix removes the line. - Attribution is per suite directory, not per Chainsaw `Test`; the leak lines carry the release instance, which usually names the test. - Only pods are checked. A leaked PVC, LoadBalancer IP or bucket holds no node memory and is not seen. - The 300s budget rests on Chainsaw's cleanup timeout, not on a measured pod-gone latency; the `clear after` lines are where that measurement will come from. - A platform object recreated mid-suite (for example a HelmRelease uninstall/reinstall remediation, or a force-replace on an immutable field) has a root newer than the suite start, and its pods would be charged to whichever suite was running as a leak; the first CI runs are where to look for this. - The per-pod wait recomputes the remaining budget before each pod, and no unit test pins that. - `kubernetes-previous` is allowlisted for #4489 although the stuck finalizer was seen only in `kubernetes-latest`; both suites create a TenantControlPlane the same way, and the exemption hides any new leak there until #4489 is fixed. - Each clean suite now waits for its own pods to finish terminating before the next starts, where the tail used to overlap the next suite. The first full run's `clear after` lines say what that costs against the E2E job's 215-minute cap. - After a timeout followed by a failed re-list, the leak lines come from the list taken before the wait, so a pod whose own wait had already succeeded is still named and the count can be too high; the verdict is unaffected. - Likewise, when every wait succeeded and the re-list then fails, the inconclusive report still lists the pods whose deletion was just confirmed. In the boundary case where the budget runs out during the last successful wait and the re-list then fails, the result is inconclusive rather than a verdict from the last good list, because the guard only treats the budget as spent when it finds it exhausted at a check. - A mistyped `COZY_LEAK_ALLOWLIST` path passes allowlist validation; it fails closed, since no suite is then exempt. - An allowlisted suite is not waited on, so `kubernetes-latest` and `kubernetes-previous` will print `LEAKED-PODS ... allowlisted=` on most runs even for pods still in normal teardown; waiting out the budget first and applying the allowlist only to what remains would stop that noise. - A slow teardown in each of several suites can add up to 300s apiece, against the E2E job's 215-minute cap. On the first full run of this PR (run 36223896793) the `clear after` values of the suites that were waited on summed to 210s; the last full run before this change took 158 of the 215 minutes. - A pod whose owner's resource type is no longer served gets "the server doesn't have a resource type" rather than NotFound, so its suite is reported unchecked instead of leaked; it still fails. - `kubectl wait --for=delete` waits for deletion, while the guard skips pods in `Succeeded` or `Failed`; a pod that finishes but is never deleted (a Job under a leaked root) spends the rest of the budget before the suite shows `clear`. The verdict is right; only job time is lost. - The unit tests do not pin the one-line empty report Chainsaw writes for a directory with no tests (`_lib`), nor the branch that catches a spent budget before calling `kubectl wait`; both are verified against the Chainsaw and kubectl sources rather than by a test. - The runner's per-suite report directory from `mktemp -d` is not removed; the sandbox is discarded after the job. - Each non-suite directory under `hack/e2e-chainsaw/` (today `_lib`) costs one empty Chainsaw invocation, because the runner leaves the decision of what is a test to Chainsaw. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: The diff adds files under `hack/`, changes the `test-chainsaw` target, and rewords one comment and the failure line in the four E2E workflows. The trigger map's `hack/` entry is for cozystack/ccp, whose skills anchor on `hack/package.mk`, `hack/common-envs.mk` and `make generate`, none of which change here. A GitHub code search for `chainsaw` across the cozystack organisation finds hits only in this repository. ### Release note ```release-note test(e2e): the E2E app suite now fails the suite whose pods outlive its own cleanup by more than 300s, naming each leftover pod and its owner; suites with a filed leak are listed in hack/e2e-chainsaw/_lib/leak-allowlist.txt and only warn. ``` |
||
|
Aleksei Sviridkin
|
08011cef09 |
fix(ci): escalate an edit to the shared backup walkthrough helpers
examples/backups/<app>/ selects that app's suite, and a directory whose name is no suite selects nothing. A helpers directory shared by every walkthrough, examples/backups/_lib/, falls into the second case: an edit to it alone produces an empty selection, which both lanes read as "skip Chainsaw", so a change reaching every backup round-trip would run none. It now escalates to the full suite like hack/e2e-chainsaw/_lib/. The set of suites that run a walkthrough is spread across the suites' own scripts, and a derived subset that misses one would fail open. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a6ec30d56f |
fix(ci): discover Chainsaw suites written as chainsaw-test.yml
Chainsaw loads chainsaw-test.yml when a directory has no chainsaw-test.yaml, but select-e2e and select-install enumerated the .yaml name only. A suite spelled that way ran whenever the whole directory was handed to chainsaw, yet no escalation listed it and no edit to it could select it, so it silently dropped out of every narrowed run. select-install also sent find's errors to /dev/null inside a pipeline that reported sort's status, so a failed listing validated no suite at all and passed. It is now captured and fails validation. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e070bc7d1d |
fix(ci): escalate a per-suite Chainsaw path that names no suite
A path under hack/e2e-chainsaw/<dir>/ put <dir> into the selection without checking that it is a suite, leaving the final intersection to drop it. Alone the path escalated through the empty-selection backstop, but beside any path that contributed a suite the escalation vanished and the run narrowed to that suite with nothing on stderr. Shared material next to _lib/ or a suite nested deeper than discovery looks then went untested in exactly the pull requests that also touch an app. The directory name is now membership-tested where the path is classified, as the hack/e2e-apps/ arm already does, so the decision is a property of the path and names it. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9ac67d76a2 |
test(tests): preserve Bats failure reports during fixture cleanup (#3848)
## What this PR does Test-level `EXIT` cleanup overrides the Bats TAP handler, so a failed assertion can disappear from test results. This moves cleanup in eight unit files after their assertions. Failed tests keep scratch files for inspection. Prerequisite for [the Bats runner change](https://github.com/cozystack/cozystack/pull/3849). Existing subshell traps in the OpenAPI and CAPK tests remain supported. Closes #3623 ### Validation `make unit-tests test-controllers -j4 --output-sync=target` passed. The converted files passed under Bats 1.14.0 and `dash`. A deliberately failed assertion produces `not ok` after this change, and the trap audit rejects an added test-level handler. ### Downstream repositories Test cleanup and its documentation only. No downstream consumer changes. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note NONE ``` |
||
|
Myasnikov Daniil
|
57bf10acae |
test(e2e): stop bringing up the monitoring instance nothing looks at
The install turned monitoring on in the root Tenant and then waited up to 15m each on vmalert and vmalertmanager, vlclusters/generic, the shortterm and longterm vmclusters, the grafana-db CNPG cluster and the Grafana Deployment. No suite inspects any of it. The one Chainsaw suite that mentions monitoring, foundationdb, asserts a WorkloadMonitor, whose CRD ships with cozystack-controller and exists without the monitoring stack. The OIDC wiring of the monitoring chart is covered by its helm-unittest suite rather than by the e2e run. Those waits were not idle. The stack competes with the rest of the install for node capacity, and their own comment recorded that a slow VictoriaLogs start could fail the install on a pull request that never touched monitoring. The run was paying bring-up time and flake risk for a component it never looked at. The monitoring package stays installed: tenant-application declares dependsOn cozystack.monitoring-application, so dropping the package would leave the platform release unresolved. Only the tenant-root instance is turned off. The container lane's VictoriaLogs storage override stays. It does nothing while monitoring is off, and it keeps a run that turns monitoring back on from creating a replicated PVC on a lane that only has local storage. One check goes with the Grafana waits: the curl through root-ingress-controller at grafana.example.org, the only assertion that the root ingress routes to a backend. Grafana was its target because it happened to be installed, and pointing it at a seaweedfs host needs the expected response checked on a live stand first. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Marian Koreniuk
|
7500a9fbae |
test(kubernetes): add the Proxmox e2e suite, parked disabled
Upstream CI has no hypervisor, so this suite cannot run there; it ships
`.disabled` and is run by a lane that owns a Proxmox stand.
Named for what the chart actually creates. The resource definition prefixes the
release, so a Kubernetes CR called k8s-e2e produces
ProxmoxCluster/kubernetes-k8s-e2e in the tenant's own namespace, and the admin
kubeconfig Secret is kubernetes-k8s-e2e-admin-kubeconfig.
Stand-specific values are variables rather than hardcoded. Every one of them is
a bare ${COZY_PVE_*} that run-proxmox.sh expands with envsubst, supplying the
defaults itself — GNU envsubst leaves a ${VAR:-default} in the text exactly as
it is, set or not, and that reaches the apiserver as a string where the schema
wants an integer — and refusing a fixture with a variable still in it. The
StorageClass and the PVC probe read the same COZY_PROXMOX_STORAGE, so the volume
lands where the probe looks. The runner reads the tenant kubeconfig's .conf key,
the address a runner outside the management cluster can reach.
The stand's resource pool comes in through COZY_PVE_POOL, empty by default.
hack/proxmox-e2e-fixture_test.bats pins the shape of the fixture and the render.
The PVC probe reads the storage again after the claim is deleted: a deleted
claim whose disk is still there is the leak the pre-delete hook's Step 1b
exists to prevent. keep_tenant is honoured by the suite's own teardown, not
only by the workflow's. The workflow carries no fallback for the stand's
variables: an unset one fails the job before a key is sent anywhere. The
schedule runs only where vars.PROXMOX_NIGHTLY is true: the job targets a
self-hosted runner with the `proxmox` label, and a scheduled run on a repository
without one would sit in the queue until GitHub cancels it.
The suite also asserts the half of the deployment that lives in the management
cluster — the CSI controller and the cloud-controller-manager — because that is
the property the pair is designed around and a tenant-only assertion would pass
with the token in the wrong place.
Signed-off-by: Marian Koreniuk <moriarti@cp.if.ua>
Assisted-by: LLM
|
||
|
Aleksei Sviridkin
|
51a2d859ac |
docs(release): describe what the release job builds and what blocks an rc
The amd64 release job builds the Talos installer and matchbox for both arches, so the pages that said CI never builds both in one job, and that build-amd64 builds amd64 only, were wrong. Every rc stitch also skips matchbox and the e2e sandbox, and the nightly report lists matchbox and the refs an in-tree rebuild stamps over; the page said every such skip and line fails the next rc. Name the ones that do not. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
71144d4fd6 |
ci(release): pin the matchbox rebuild in the release job
tags.yaml only runs on tags, so no pull request exercises it. The matchbox rebuild is the only thing that makes its ref an index, and losing the step, moving it past the tree capture or letting it switch the default builder would surface as a failed gate on the next rc. The comments that called the talos images amd64 only now say they are built for both arches in the amd64 job. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c740031453 |
Merge remote-tracking branch 'origin/main' into ci/build-arm64-leg
Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
10727c0391 |
test(tests): preserve Bats failure reports during fixture cleanup
Test-level EXIT handlers replace the handler Bats uses for TAP output, so a failed assertion can disappear from the reported test results. Cleanup must preserve those failures before the unit runner changes. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
cb035f8eac |
docs(agents): stop calling the image-ref known gaps unfixed
Two of the gaps listed there are handled in the tree now, and each entry already says whether it is. The heading sentence still claimed all of them were open. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
79854b16fb |
Merge remote-tracking branch 'origin/fix/build-list-orphan-images' into feat/arm64-image-builds
Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
5924b3fd34 |
docs: describe how a pre-release is built and gated for arm64
Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
144d080342 |
feat(build): gate a release on every pinned image being multi-arch
A release pins one ref per image for clusters of either architecture. A ref that resolves to a single-platform manifest, or to an index missing amd64 or arm64, installs on one and fails to pull on the other, and nothing before an arm64 install would notice. hack/verify-multiarch.sh resolves every ref hack/lib/image-refs.sh enumerates, third-party ones included since an arm64 node pulls them too, and fails naming each ref that is not an index carrying both linux/amd64 and linux/arm64. A ref that cannot be resolved fails as well, rather than passing unchecked. A rate limit or a 5xx is the one answer retried, a bounded number of times, and a ref still refused fails as rate-limited rather than as single-arch. --report prints the same list without failing, for a tree that is not being released. Images that cannot be built for both architectures go in hack/multiarch-allowlist by repository, each with a reason, so a bump needs no edit; they are still listed, and an entry that matches no single-arch ref is named as stale. --refs-file checks a list of refs instead of a tree, a tag-only ref resolved by its tag, with the same allowlist and output. It is for images the tree does not pin by digest, such as the ones an e2e run's nodes actually pulled. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
7753394bfe |
build: rebuild every package that has an image target
flux-plunger, keycloak-operator, kilo and migration-controller have image targets but were missing from the root build list, which the build matrix, the main build and release preparation all iterate. No CI path rebuilt them, so their pins stayed on images pushed by hand while Dockerfile, patch and base-image changes never shipped, and the multi-arch pre-release build would find single-arch pins it cannot replace. migration-controller shipped a bare :latest with no digest. Add the four packages, and a test that derives the set from the package Makefiles so a new image target cannot be left out again. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
f02963a2c7 |
fix(keycloak-operator): keep the image host inside repository
The image target stamped only `tag`, leaving the committed `registry: ghcr.io` in place, so a build against the CI registry would render the public host with a digest that exists only in the build registry. Stamping the build host into `registry` instead would split the source registry across two keys, where the nightly mirror's substring host rewrite cannot reach it and the published tree would keep pointing at the private registry. Set `registry: ""` and keep the whole host in `repository`, stamped by the image target. The upstream chart joins the two keys, so the rendered image string is unchanged, and the ref now has the same shape as every other stamped reference the mirror and promotion already handle. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
769d3b3897 |
fix(build): push the kamaji provider image under the build's IMAGE_TAG
The capi-providers-cpprovider Makefile assigned IMAGE_TAG itself, which beats the value CI puts in the environment. Every PR, main and release line build therefore pushed the same component-version tag and overwrote each other's, and its second tag was built from an undefined TAG variable and ended in a bare "-". The image now goes through image-tags like every other package: the build-unique IMAGE_TAG always, the component version only under PUBLISH_VERSIONED=1. The stamped refs keep the component version as their tag, so the promote, retag and mirror tooling see the same refs as before. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9a7094427e |
feat(build): stitch amd64 and arm64 builds into multi-arch indexes
The arm64 images are built on native arm64 runners, apart from the amd64 build, so each build pushes a single-platform image. The stitch joins every first-party image with its -arm64 twin into one index, moves every tag this build pushed for it to that index, and repins the tree on the index digest, so one ref serves both architectures. Most refs in the tree name only the build's IMAGE_TAG, while under PUBLISH_VERSIONED=1 the build also pushes the component version, so the refs alone cannot say which tags to move. The image-tags macro therefore appends every tag it pushes to PUSHED_TAGS_LOG when that is set, and the stitch moves exactly those, plus IMAGE_TAG and the tag a ref carries. It reads no other tag: another build's tag that sits on the same amd64 bytes is not this build's to move, and scanning every tag of every repository would cost thousands of registry reads per release. It reads refs through hack/lib/image-refs.sh like every other consumer, rewrites only the files that enumeration names, and then searches the whole tree outside charts/, gzip payloads included, for any old digest it re-pointed. A ref whose digest is also pinned where the rewrite cannot reach is skipped instead of half rewritten. The kamaji control-plane provider's chart ships its ref only inside files/components.gz, built from a plain copy with gzip -n, which no text search sees. The stitch rewrites both copies, recompressing with gzip -n so the result is what the package's image target would have written, rather than leaving that image amd64 only. An image with no arm64 twin (talos, the e2e sandbox), or a pair that is not one amd64 and one arm64 image, is skipped and named. A registry read answered with a rate limit or a 5xx is retried a bounded number of times; any other registry error fails the run at once rather than reading as a missing arm64 image. On a rerun after a stitch that failed partway, the build tag can already sit on an index of the pinned image and an arm64 image, not necessarily this run's if the arm64 leg rebuilt it. The stitch repins on that index instead of skipping the image, and copies it to any of the build's other tags still left on the amd64 image. A tag moved to anything else fails the run, since the tree then no longer matches the registry. MATRIX_ARCH=arm64 makes hack/build-matrix.sh leave out the e2e sandbox, which pins linux/amd64: an arm64 leg building it would push a second amd64 image under its -arm64 tag. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
6907c22302 |
docs(agents): describe the talos package as multi-arch
The build notes said the talos package pins linux/amd64, which no longer holds and would steer an agent into re-adding the pin. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b6a30bd504 |
docs(e2e): describe the per-suite runner and the pod-leak guard
The agent guide, the suite README and a workflow comment said the E2E step is a single chainsaw invocation; it is now one per suite, and a suite whose pods outlive its cleanup fails. Say how a pod is charged to a suite, what the budget is, and how the allowlist is kept honest. The workflows' failure line now points at LEAKED-PODS as well, since a run that failed only on a leak has no assertion diff to point at. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
084ac47237 |
feat(build): build published images for amd64 and arm64 by default
PLATFORM defaulted to empty, so buildx built for the host alone. On an arm64 workstation `make image` published an arm64-only digest, which amd64 nodes then fail to pull; the cilium pin shipped exactly that until it was rebuilt by hand. A build now targets linux/amd64 and linux/arm64 unless PLATFORM is set, and the digest a package stamps is the index digest. LOAD=1 keeps building for the host only. The classic docker image store cannot load a multi-platform index, and loading is the documented local workflow. This changes local pushes: a `make image` on the default docker driver without the containerd image store now fails with "Multi-platform build is not supported for the docker driver", where it used to build for the host. Such a build needs a docker-container builder (BUILDER), or LOAD=1 or an explicit PLATFORM for a single architecture. talos and testing pin linux/amd64 with `override`, placed before the include because common-envs.mk expands BUILDX_ARGS immediately. The Talos assets are amd64: matchbox bakes the kernel and initramfs into its image, and the e2e sandbox boots the disk under qemu-system-x86_64. An arm64 variant of either would still serve or boot amd64 files. CI pins linux/amd64 on every image build. Nothing there sets up QEMU, fork builds export a single-platform archive, and tags.yaml builds on the default docker driver, so the default would break or emulate those jobs. Native arm64 builds belong in a leg of their own. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
368d00b5d4 |
test(e2e): remove unused monitoring suites and audit BATS runners (#4019)
## What this PR does I removed the two monitoring OIDC BATS suites that had no runner after the Chainsaw migration, and brought the runner audit and wrapper OIDC test from [#3778](https://github.com/cozystack/cozystack/pull/3778). Also added checks for audience mapper and `OIDC_MODE=System`. The named `KeycloakRealmGroup` test documents coverage the document-count and kind assertions already provided. Runner audit reports unused BATS suites, missing files in runner paths, and unresolved Make variables (including `$(E2E_SUITE)`). Live Monitoring API admission, Grafana operator reconciliation and mode transitions are not tested here. ### Validation - `make -j4 unit-tests SHELL=/usr/bin/dash` passed. - Monitoring Helm tests: 53 system-chart tests and 10 wrapper-chart tests passed. - Runner audit: 12 cases passed under dash and Bats 1.14.0. The full shell syntax and BATS sweep passed under dash too. - Three chart mutations and seven runner mutations failed as expected. ### Downstream repositories - [x] No downstream repository is affected by this change ### Release note ```release-note NONE ``` |
||
|
Myasnikov Daniil
|
c55ab268e2 |
test(monitoring): remove orphaned OIDC BATS suites
The two pre-Chainsaw monitoring OIDC suites have had no invocation site since the app suite migrated to Chainsaw. Remove them instead of carrying dead tests that force the selector to run every live suite. Keep the existing selector and capture guards for the historical path shape, and update their comments now that the directory is empty. Retain a named chart assertion for the no-KeycloakRealmGroup ownership invariant; the existing count and kind assertions already enforce the same behavior. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Timofei Larkin
|
180030be72 |
fix(api): refuse only what a name declaration actually breaks
Three cases refused creates of a kind, or were documented as refused, without the declaration being at fault. A schema that is not a JSON object was reported as an invalid x-cozystack-name, although such a schema has no declaration to blame, and the spec schema's own loader already reports it. It now declares nothing, so creates keep working and a kind with a legacy cap keeps it. Any unknown key inside a declaration took the kind offline, so a generator adding a title or an example would do the same. Keywords that constrain nothing, the JSON Schema annotations and x- extensions, are now ignored. A constraint keyword the server does not implement is still refused, because accepting it would drop it silently. The docs said a declaration no name satisfies is refused. That holds for lengths only: a pattern is checked to compile, not to match anything. The docs now say so. The parser moves to pkg/apis/apps/validation so the API review gate can reuse it rather than keep a copy of its rules. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
146e06e75e |
feat(api): take Application name limits from the application's own schema (#4367)
## What this PR does
The aggregated API server capped two kinds' names by knowing what their
charts render. A `Kubernetes` cluster name was held to 32 characters
because every worker pool is a `KubernetesNodes` release named
`kubernetes-nodes-<name>-<pool>`, and the default `md0` pool has to fit
the 53-character Helm release name limit. A `Kafka` name was held to 44
because the KRaft controller pod `kafka-<name>-c-<hash>-<id>` has to fit
the 63-character DNS-1123 label limit. The arithmetic is right, but it
is the charts' knowledge living in the platform: the next application
with a sub-resource budget needs another `if r.kindName == ...` branch,
and an application served from outside this repository — the whole point
of `ApplicationDefinition` — cannot get one at all.
An application now declares the limits on its own name in its own
schema, and the server applies whatever it finds there for whatever kind
the definition describes. The chart's `values.yaml` carries a `## @name`
annotation, `cozyvalues-gen` v1.7.0 publishes it at the root of
`values.schema.json` as `x-cozystack-name`, and `hack/update-crd.sh`
carries it into the `ApplicationDefinition`:
```yaml
## @name {string} - Cluster name. Every worker pool is a separate KubernetesNodes release named `kubernetes-nodes-<name>-<pool>`, so the name must leave room for the default `md0` pool inside the 53-character Helm release name limit.
## @maxLength 32
```
```json
"x-cozystack-name": {
"type": "string",
"description": "Cluster name. Every worker pool is a separate KubernetesNodes release named `kubernetes-nodes-<name>-<pool>`, so the name must leave room for the default `md0` pool inside the 53-character Helm release name limit.",
"maxLength": 32
}
```
`maxLength`, `minLength` and `pattern` are honoured; `pattern` is
unanchored, as JSON Schema specifies. A declaration only tightens:
`prefix + name` must still fit the 53-character Helm limit. A rejected
name is answered with the chart's own description, or with "limit
declared by the <Kind> application" when the chart gives none.
The schema is untrusted input — definitions from other repositories are
written by hand — so a declaration that cannot be enforced as written
(not an object, a type error, an unknown keyword such as a misspelled
`maxLenght`, `maxLength` below 1, a negative `minLength`, `minLength`
above `maxLength` or above the Helm budget, a pattern that is not RE2)
refuses every create of that kind with `configuration error: the <Kind>
ApplicationDefinition declares an invalid x-cozystack-name: …`, and logs
the same at start-up. Dropping the declaration would admit names the
chart cannot render; applying it would reject every name and blame the
user. Other kinds are unaffected, and existing objects of the affected
kind can still be read, updated and deleted.
An upgrade brings `cozystack-api` up before `kubernetes-rd` and
`kafka-rd` re-render (`kubernetes-rd` depends on `cozystack-engine`), so
the new server can briefly read a definition that predates its
declaration. For exactly that case — a definition that declares nothing
— the two previous caps stay in `legacyNameCaps` as a floor, to be
removed once no supported upgrade starts from such a definition. That is
the only per-kind knowledge left in the name checks.
Also in this PR:
- The `pre-commit` job pins `cozyvalues-gen` v1.7.0, the first release
that understands `@name`
([cozystack/cozyvalues-gen#33](https://github.com/cozystack/cozyvalues-gen/pull/33)).
- The API review gate now diffs the root `x-cozystack-name` of every
definition as `metadata.name`, so a chart that lowers its cap or adds a
pattern is reported like any other tightened field. On this PR it
reports the Kubernetes and Kafka caps as added; their effective limits
are unchanged.
- The migration 56 comment that pointed at the removed
`maxKubernetesClusterName` now points at the chart's declaration.
Not in scope, because none of it is a static limit a chart can declare:
the `Tenant` name pattern in `pkg/apis/apps/validation`, the computed
tenant-namespace length check (it depends on the parent namespace at
request time), the tenant quota hierarchy, and the Phase 2 removed-field
warning for `Kubernetes`.
### How it is tested
`TestNameCapComesFromTheShippedSchema` reads the shipped `kubernetes`
and `kafka` ApplicationDefinitions off disk, builds storage through
`NewREST` as the server does, and checks the cap and the chart's
explanation; it then rewrites `maxLength` inside the shipped schema and
checks the limit moves with it, which a cap held in code would not.
`TestInvalidNameSchemaFailsClosed` covers each malformed declaration
through the same path, `TestLegacyNameCapsCoverUndeclaredDefinitions`
pins the upgrade floor and that a declaration replaces it, and
`TestClassifyNameDeclaration` covers the gate.
### Downstream repositories
Two repositories are affected and neither follow-up is filed yet, so no
box is ticked.
- `cozystack/website` — `content/en/docs/next/development.md` documents
the `values.yaml` annotations and needs `@name`;
`content/en/docs/next/cozystack-api/application-definitions.md`
describes `openAPISchema` and needs `x-cozystack-name`.
- `cozystack/external-apps-example` — it pins no `cozyvalues-gen`
version and regenerates against whatever is current; nothing in it
declares a name constraint, so it is unaffected until it chooses to.
- [ ] 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
feat(api): Application name limits now come from the application's own schema instead of being hardcoded per kind in the aggregated API server. A chart declares them with a `## @name` annotation in `values.yaml`, which cozyvalues-gen v1.7.0 publishes as `x-cozystack-name`; the API server enforces `maxLength`, `minLength` and `pattern` on `metadata.name` for any kind, answers a rejected name with the chart's own explanation, and refuses creates of a kind whose declaration it cannot enforce with a configuration error. The Kubernetes (32) and Kafka (44) name caps are unchanged in effect and are now declared by their charts.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Applications can declare minimum and maximum name lengths and
formatting patterns.
* Kubernetes cluster names are limited to 32 characters, and Kafka
cluster names to 44 characters, to meet generated resource naming
limits.
* Names that violate declared constraints are rejected with an
explanatory message. Tighter name constraints are also reported as
breaking changes.
* Declared maximum lengths cannot exceed the platform’s existing Helm
release-name budget.
* **Documentation**
* Added guidance on declaring and enforcing chart-specific resource-name
limits.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
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> |
||
|
Timofei Larkin
|
a3911811c7 |
feat(api): take Application name limits from the application's schema
The API server capped Kubernetes and Kafka names by knowing what those two charts render: worker pool releases under a 17-character prefix, controller pods with a 13-character suffix. That is the charts' knowledge sitting in the platform, and it does not generalise. The next application with a sub-resource budget needs another branch on Kind, and an application served from outside this repository cannot get one at all. An application now declares the limits on its own name in its schema, under x-cozystack-name, and the server applies whatever it finds there for whatever kind the ApplicationDefinition describes. A declaration only tightens: prefix + name must still fit the 53-character Helm release name limit. A rejected name is answered with the chart's own explanation of the limit. The schema is untrusted input, and definitions from other repositories are written by hand. A declaration that cannot be enforced as written, whether an unknown keyword, a wrong type, a limit no name satisfies or a pattern Go cannot compile, refuses creates of that kind with a configuration error. Dropping it would admit names the chart cannot render; applying it would reject every name and blame the user. An upgrade brings cozystack-api up before the definitions it serves are re-rendered, so for a while the new server can read a Kubernetes or Kafka definition that predates its declaration. The two old caps stay as a floor for exactly that case, a definition that declares nothing, until no supported upgrade starts from one. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@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> |