mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-04 02:57:27 +09:00
Aleksei Sviridkin
312409ecd2
## 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. ```