mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-29 16:17:20 +09:00
docs/agents-dev-principles
42
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Myasnikov Daniil
|
cc942fed68 |
test(tests): run unit suites with Bats and retain POSIX coverage
The shell translator rewrites test bodies and loses Bats reporting. The unit lane needs complete TAP results and JUnit reports without changing how live-cluster suites run. Sourced production helpers still need a dash pass, and unit test bodies need nounset without exporting shell options into subprocesses. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
IvanHunters
|
9196b9798e |
fix(e2e): bound the dataplane capture collector by wall clock (#4640)
## What this PR does The e2e data-plane collector limited itself by counting pods and LoadBalancers, but both of its callers stop it by the clock. `hack/cozytest.sh` gives it 600s and the Chainsaw catch gives it 300s. One affected pod could already run past the smaller limit, and the LoadBalancer capture, which runs last, was the part that got lost. I moved the collector to a wall-clock budget and fixed the other issues filed against it. - Every kubectl call now goes through `dp_run`. It cuts the call's timeout to what is left of `COZY_DATAPLANE_BUDGET` and starts nothing once the budget is spent. The default is 270s, and `cozytest.sh` passes 570s. The pod section stops 90s early, so the LoadBalancer section always gets time to start. A call the deadline cut says so, a call cut by its own bound names that bound, and a refused call says it was not started and names its section; exec blocks carry that line inside the block. The node capture and the LoadBalancer probe used to blame an unanswered lookup or a missing cni-server when the budget stopped a call. Now they name the budget (#3671). - A static test fails on any kubectl call that bypasses `dp_run` and lists the lines. It matches by command position, so a call inside a double-quoted `$(...)` is still checked (#3694). - Per-pod and per-LoadBalancer capture files join namespace and name with `_`, which neither can contain. A second run into the same directory appends, with the run separator `capture-notes.txt` already uses (#3679). - `capture_node` and `capture_lb_node` no longer write their callers' loop variables. A static test fails when a function assigns a name that a `read -r` loop or another scope owns (#3675). - The tests that shrank every timeout to one second now cut only the read under test. A `timeout` shim decides by argv, so a slow machine can't cut a read the test needs answered (#3685). - `cilium-dbg bpf lb list` runs unfiltered in every affected node's capture, not only for a failing LoadBalancer (#3689). The call-site comment in `.chainsaw.yaml` and the collector paragraph in `docs/agents/e2e-testing.md` now match the code. Every behaviour change has a test in `hack/capture-dataplane.bats` that failed on the code before it. Two parts of the issues live elsewhere. The runner stopping a bats file at its first failing test is #4620, and a conntrack read filtered by service VIP is #4623. From #4629 I fixed the items on lines this PR already changes: the node capture now gets the run separator, a failed speaker log read writes a note, the endpoint tcpdump says when it falls back to the geneve interface, and the per-read timeout knobs are validated (a zero there would otherwise look like a spent budget). The rest of #4629 stays open: `ovn-lflows.txt` and `lb-speaker-logs.txt` are still replaced on each run, and two comments still describe older behaviour. - Closes #3694 - Closes #3671 - Closes #3679 - Closes #3675 - Closes #3685 - Closes #3689 - Refs #4629 ### 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 fix(e2e): bound the failure-path data-plane collector by a wall-clock budget, keep its per-pod and per-LoadBalancer captures from overwriting each other, and capture the cilium service map for ClusterIP failures. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Data-plane diagnostics now follow a configurable time budget, reserving time for LoadBalancer captures and stopping operations when the budget is spent. * Capture reports distinguish failed, successful, unknown, and budget-limited checks. LoadBalancers with successful probes are skipped for heavy capture. * Repeated captures are preserved together, and limits help control LoadBalancer diagnostic collection. * Diagnostics include additional network details from relevant nodes during probe replay. * **Documentation** * Updated guidance covers capture time limits and how unanswered EndpointSlice reads are reported. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
cd448fdc04 |
test(bats): widen the comments describing the errexit exemption
Six suites explain why they avoid `! cmd` by saying errexit is suppressed for a "negated pipeline". The exemption is not about pipelines: a bare `! true`, a negated `[ ... ]`, one inside a `for` body or a brace group, one in a case branch, and one following `;`, `&`, `&&` or `||` are all exempt too. Anyone reading the narrower wording could conclude the other shapes are safe. One of the six carries the sentence twice, and the second copy is split across two physical lines -- `` `!` `` ending one and `pipeline)` opening the next -- so a line-oriented search for the phrase does not see it. Prose wraps; a grep for it has to fold the lines first. Comment text only; every one of these files already uses the working form the comment recommends. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
57b5c83158 |
test(e2e): widen the clock margin of the own-bound cutoff test
The test that a call cut by its own bound just past the deadline is not blamed on the budget ran a 3s bound inside a 4s pod section. A whole-second tick of date +%s between DP_T0 and the first call left the bound reaching the deadline, the collector then blamed the budget, as it should, and the test failed. The pod section is now 10s against the same 3s bound, so a tick cannot reach the deadline. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
0c9806ab13 |
fix(e2e): report a dataplane pod read once and by name in its block
The Ready-conditions and events reads share one block and added a line of their own when they failed, on top of what their stderr put into the block. Once dp_run marked every cut and refusal there, a cut read carried two lines for one event, and before the budget status existed the second named a timeout the first said the deadline had chosen. The two reads now send stderr to the scratch file and, after their output, write kubectl's stderr into the block whole, without dp_run's marker, plus one line naming the read when it failed. kubectl's message is kept as it came, since an error can follow lines of throttling notices on a degraded apiserver. Without a scratch file their stderr goes into the block directly. The probe comments also say what the "spent" outcome now covers. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d47428277f |
fix(e2e): report a dataplane read the deadline cut as the budget's
Once dp_run marked a deadline cut inside an exec block, the reads that report their own outcome still read the 124 their shortened bound produced and called it the read's own timeout, quoting a bound the deadline had chosen. A Ready-conditions block then gave two reasons for one cut, and every list and lookup note did the same beside the section line that blamed the budget. dp_run now returns a status of its own for a deadline cut, which the read notes, the node and interface lookups and the LB probe all report as the budget. pod_on_node takes it from dp_run instead of deciding it a second time. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
25de9ba7aa |
fix(e2e): mark a dataplane exec block its own bound cut off
An exec block keeps whatever it printed before its bound fired, and nothing in the block said the bound had fired. The service map every affected node gets is the largest such block, and a truncated one answers the question it is there for, whether a backend is still in the map, wrongly while looking complete. dp_run now marks a cutoff where the call's output goes, as it already does a refusal: the deadline's own cut names the budget, any other names the call's bound. The section note also stops asserting that a call was cut when the budget only refused the next one. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
700c4b1f77 |
fix(e2e): credit a dataplane cutoff to the budget only when it was
dp_run credited a cutoff to the budget whenever the deadline had passed by the time the call returned. A call under its own, unshortened bound can fire, or come back from its kill grace, just past a deadline it never reached; the section note then said the budget cut a call, next to the read's own note naming its own timeout, and the walk closed as cut short by the budget. The bound alone is not enough either: a call killed early, as an OOM-killed remote process is, returns 137 under a bound cut to the deadline long before the deadline arrives. dp_run and pod_on_node now share one rule: a cutoff is the budget's only when the call's bound reached the deadline and the deadline has passed, and never when no timeout binary bounded the call at all. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e8a8626170 |
fix(e2e): point a cut-short dataplane walk at the notes file
A walk the budget cut short closed on "see the section line above", but that line goes only to capture-notes.txt, while the closing line also reaches the job log, where nothing sits above it to see. It now names the file. The budget knob replacement and the run without a timeout binary had no test of their own; both are now pinned. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9672fdc93e |
fix(e2e): close both dataplane walks on what actually stopped them
The LoadBalancer walk can stop on the budget, as the pod walk can, and still closed on "capture complete" right under the line naming what it did not probe. Both walks also called themselves complete when they ended on their own after the deadline had cut a call inside their last item. Each walk now closes as stopped early when it broke out of the loop, and as cut short by the budget when this run's section note says a call was cut or refused; the note's own text is the evidence, since the clock read after the loop can pass the deadline without any call having been cut. The LB cap break, which shares that closing line, closes the same way. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9ef7206548 |
fix(e2e): stop calling a dataplane pod walk complete when it stopped early
The pod walk can now also stop when the pod section's share of the budget runs out, as it already could at the pod cap. Both exits name what they left behind, and the line after them still announced the capture as complete. It now says the walk stopped early. The helper comment about the run separator also named every capture file, while the OVN dump and the speaker logs are still replaced on each run; it now lists the files it covers. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
65226fc585 |
fix(e2e): attribute a call the dataplane deadline cut to the budget
Once bounds are clipped to the budget, a cutoff has two meanings: the call's own bound fired, or the deadline shortened it. Node lookups cut the second way were still written up as lookups that did not answer, a cause about the cluster that nothing observed. And the section note was written only when a call was refused, so when the deadline cut a section's last call the truncated capture carried no word about the budget at all. A lookup cut under a bound the deadline shortened now names the budget in the node capture, the endpoint interface lookup and the LB probe, decided from the bound it ran under rather than from the clock when its status is read. A call the deadline cuts also writes the section note, as a refusal does. The heavy-capture line also stops claiming every probe failed when some of them were stopped by the budget. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e4e249bbf6 |
fix(e2e): note the dataplane reads that failed or fell back silently
A few reads that go through dp_run still failed or fell back without a trace. A speaker log read that failed looked like a speaker that never announced the address; it now writes a note. The endpoint tcpdump's fallback to the geneve interface is stated in the LB capture. The node capture now carries the run separator the pod and LB files use, through one helper. Two budget interactions named a cause that never happened. A per-read timeout knob of zero made every read look refused by the budget, and a value timeout cannot parse failed every read with 125, reported as kubectl failing; both knobs are now validated like the budget ones. An LB probe whose cni-server lookup the deadline cut was reported as a second, unanswered lookup; it now counts as stopped by the budget. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b21b0230ea |
fix(e2e): blame the budget, not the cluster, for dataplane reads it stopped
The budget can run out inside one capture rather than between them. A node lookup refused by dp_run was then written up as a lookup that did not answer, an LB probe whose cni-server lookup was refused as the same, and a probe exec refused, or cut by a bound the deadline had shortened, as a node with no cni-server or no probe client. Each names a cause that never happened. The lookup's own status now reaches its callers, the node capture says the lookup was not asked, and the probe reports a "spent" outcome that the decision weighs like an unrun probe and the reason line attributes to the section's budget. A reserve at least as large as the budget is also named, since the pod section then starts no call at all. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c4e6d169a0 |
fix(e2e): give the cozytest caller a dataplane budget that fits its backstop
The collector's default budget is sized under the 300s Chainsaw backstop. The cozytest.sh caller allows 600s, and on the default it would give up more than half of that window, so it passes a budget matching its own backstop. The budget test reads each caller's override and fails when a budget leaves more of its backstop unused than the backstop's kill grace. The Chainsaw call-site comment still described the fan-out caps as the bound, and the e2e testing guide described an EndpointSlice exception to the notes contract that the code does not have; both now describe what the collector does. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
e808dbc867 |
fix(e2e): name the section whose dataplane budget ran out
The pod section stops a reserve short of the end of the budget and the LoadBalancer section keeps starting calls after it, but a refusal was reported as the whole run's budget running out, which the LB capture written below it contradicts. The note was also deduplicated once per run, so the LB section running out later left no line of its own. Refusals now name their section, and the note is written once per section. Both budget knobs are also read as decimal: shell arithmetic took a leading zero for octal, so 08 aborted the run before it wrote a note and 010 meant 8. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
37592f9457 |
fix(e2e): keep per-LoadBalancer dataplane captures apart and across runs
The per-LB capture and its two tcpdump files joined namespace and Service name with a hyphen, which both may contain, so a-b/c and a/b-c wrote the same files, and each run truncated them while the notes beside them accumulate. They now use the separator and the run marker the per-pod files use. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
4377b68387 |
fix(e2e): do not cache a pod lookup cut short by the section deadline
The per-node lookup memo keeps a cutoff so a hung lookup is not paid for twice. Once bounds are clipped to the budget, a lookup can be cut after a second or two because the pod section's deadline arrived, and caching that made the LoadBalancer section report the component as unknown for the node while its reserve still had time to ask. A cutoff is now cached only when the lookup ran under its full bound. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
eec309e085 |
fix(e2e): say why a dataplane block is empty when the budget ran out
A call dp_run refuses for a spent budget returned silently, and every exec block swallows its status, so a capture the budget ran out in the middle of kept only section headers. A header with nothing under it reads as a command that found nothing, and the notes said nothing either, because the loops only decline whole pods and LBs. The refusal now goes to stderr, which lands in the block it emptied, and capture-notes.txt gets one line per run. The OVS interface lookup also stops calling a failed, cut or refused lookup an absent interface. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
ca234a76c1 |
fix(e2e): bound the dataplane collector by wall clock, not by count
The fan-out caps counted pods and LoadBalancers while both callers' backstops count seconds, and a single affected pod's worst case already ran past the 300s Chainsaw one. No count could be made to fit, and a cut-off run lost whatever came last, which was always the LoadBalancer capture. Every kubectl call now goes through dp_run, which clips the call's bound to what is left of a run-wide budget sized under the smaller backstop and starts nothing once it is spent. The pod section stops a reserve short of the end so the LoadBalancer section always gets time to start. A read cut by a clipped bound quotes the bound it ran under, a read never started says so instead of naming a timeout, and a section that runs out of budget names what it did not capture. Tests pin the budget against both callers' backstops, the behaviour on a cluster where every read hangs, and that no kubectl call bypasses dp_run, listing any that does by line. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
dc13587ef6 |
test(e2e): decide dataplane read cutoffs by argv, not by the clock
Three collector tests shrank both per-read bounds to one second to make the read under test fail fast. The same bounds also cover the reads that must answer, so on a loaded machine the service, endpointslice or pod list is cut instead, the collector correctly takes another branch, and the test fails as if the code under test had regressed. A `timeout` shim on PATH now cuts only the reads whose argv the test names and runs the rest unbounded against the instant stub, so the cut is the input rather than a race. A cut read that bypasses `timeout` still reaches the stub's sleep and runs into the test's own backstop. The tests also assert the collector finished on its own and the decision line it logs, not only that a file exists. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
6bba3fda3b |
fix(e2e): capture the cilium service map for ClusterIP failures too
`cilium-dbg bpf lb list` ran only inside the LoadBalancer path, which is entered only when a LoadBalancer IP fails its probe. The map describes every service translation on the node, so a pod that cannot reach a ClusterIP left no record of which backend the VIP resolved to or whether a backend that stopped answering was still in the map. It now runs unfiltered in the per-node capture, which runs for the node of each affected pod the collector reaches. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
8449f483f3 |
fix(e2e): stop dataplane capture helpers writing their callers' variables
POSIX sh has no local scope, so every assignment in a function lands in the caller. capture_node wrote `node`, the variable of the loop that calls it, and capture_lb_node wrote the LB loop's _lbip, _np, _epip and _of. Both stayed harmless only because each caller passed what it already held; any other argument would have made the rest of the iteration read another node's pods and file them under the right name. Both functions now use their own prefix, like the newer helpers in the file, and a static test fails when a function assigns a name that a `read -r` loop or another scope also owns. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
de66310baf |
fix(e2e): keep per-pod dataplane captures from overwriting each other
The per-pod capture file joined namespace and pod name with a hyphen, which both may contain, so a-b/c and a/b-c wrote the same file and the second capture silently replaced the first. An underscore can appear in neither, so it keeps every pod apart. The file was also truncated on each run while capture-notes.txt beside it accumulates, leaving notes from two runs next to evidence from one. It now appends with the same run separator the notes use. Assisted-by: LLM 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
|
18be4ee272 |
test(e2e): capture host-netns address attribution and a healthy-pod reference
The collector characterises the host->local-pod route for an affected pod through ovn0 alone, so when a pod IP turns out to be a local address of the host netns instead, nothing records which interface actually holds it. The host netns capture now also takes `ip -o addr show` across every interface and `ip route show table local`, so the owner can be named on the next occurrence rather than only ruled out on ovn0. The collector also inspects NotReady pods only, so a run has no same-window baseline for what a normal route and conntrack state look like on the same node. The per-pod capture becomes a reusable function and runs a second time against one Ready, Running, non-hostNetwork pod on that node, into the same file, so the comparison is in front of whoever opens it. hostNetwork is excluded because such a pod's podIP IS the node's own address and would show the same local-route fingerprint as the fault for an entirely ordinary reason. That baseline depends on the node and nothing else -- the candidates are the rows of the one snapshot the affected set was computed from, and a pod reporting Ready there is by definition not one this run is investigating -- so it is captured once per node and folded into each affected pod's file from a cache. Several affected pods on one node is what a wedged host->local-pod datapath looks like rather than an edge case, and capturing the same healthy pod once per affected pod would spend the caller's envelope re-asking a question already answered -- the cost the node-global dedup and the lookup memo beside it exist to avoid. This adds no read of its own. The cluster-wide pod list gains one column instead of being asked a second time, and that single read's status is what lets the artifact separate a node with no eligible pod from a list that never answered: one line for both would make an unchecked baseline read as an established absence. The two host-netns additions are best-effort execs, like the captures they sit beside. The new row filter carries the field count its jsonpath emits, so a list cut mid-record cannot hand over a fragment as the pod every reading of the affected one is compared against. The unit suite pins these on what they are rather than on what they leave behind. Both host-netns captures are asserted through the kubectl argv the collector issues, container included, because the section headers are separate echoes that outlive the exec being removed. The baseline is pinned from both sides: several affected pods on one node share a single capture, and an affected pod is served the healthy pod from its own node rather than from whichever node the walk reached first. The pod list's column order is asserted against the positions its readers index, because every runtime test answers that list from a stub written by hand and none of them can observe the jsonpath at all. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
1b52832f37 |
fix(e2e): stop the data-plane capture reporting what it never observed (#3672)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
Six fixes to the e2e host->pod data-plane collector and the two call
sites that invoke it, all of one class: a diagnostic that states a fact
it never observed. Every one of them makes a failed run look healthier
than it was, which is the worst direction for a tool that only ever runs
on an already-failed test.
- A list cut mid-record parsed as a valid record. Three `awk -F'|'` row
filters selected by field value and never checked the field count, so an
IP cut mid-octet still looked like an IP and a cut inside a nodeName
still looked like a scheduled pod. The capture then probed an address
the cluster never had and opened a per-node file for a node that does
not exist. Each jsonpath emits a fixed field count per record, so a
shorter row is a fragment by construction.
- Five distinct paths ran zero probes and all reported "reachable,
skipped": no node to probe from, a Service with no port, a probe node
with no `kube-ovn-cni` pod, a host netns with no `nc`/`curl`/`wget`, and
an exec that could not run. An image without a probe client alone would
stamp every LoadBalancer in the cluster reachable and skip the whole
heavy capture. They now report unprobed, each with a reason the run can
actually support.
- Both EndpointSlice reads discarded stderr and were read by emptiness
alone, so a refused read wrote the same `<none>` a Service with no
endpoints writes. They now carry their status. That change armed a
landmine the silence had hidden: the target port was read with
`{.items[0]...}`, and client-go's `evalArray` has no `allowMissingKeys`
escape, so an endpoint-less Service is a hard error rather than an empty
answer. The read now asks with `{.items[*]...}`.
- `pod_on_node` turns its node argument into `--field-selector
spec.nodeName=<node>`, so an empty node asks for pods whose nodeName is
empty, i.e. unscheduled ones. Both tcpdump legs could hand a pending pod
to a capture stamped ANNOUNCER or ENDPOINT. Each leg is now guarded and
says in the artifact that it was skipped.
- Both callers wrapped the collector in a wall-clock backstop and
discarded the result, while the previous-logs leg beside each of them
reports "INCOMPLETE (exit N)". A truncated `dataplane/` directory was
indistinguishable from a complete one that found little.
- The file's own header credited one caller where two exist, under
different backstops (600s and 300s), so a change sized against the named
one can overrun the omitted one.
Diagnostics only. No retries, no cluster mutation, no change to any
test's pass/fail outcome: the statuses that are now read are used to
print a line and nothing else.
Every behavioural change is pinned by a test in the existing bats
surface, verified red against the committed tree before the fix. The two
suites run 53 and 5 tests.
Known limits and deliberate omissions, named so the next reader does not
have to rediscover them:
- A field count cannot catch a cut inside a record's LAST field, since
every separator has already arrived. The comment on the first filter
says so and says where it lands for each of the three. The MetalLB
speaker loop is left without a count for the same reason: its records
carry two fields, and `speaker-1|nod` is indistinguishable from a
genuinely short node name, so a count there would reject nothing while
reading as a closed gap.
- The backstop lines report any non-zero status as `INCOMPLETE (exit
N)`. Exit 127 (no `timeout` on PATH) means nothing ran, so "kept what
landed" overstates it. Left alone deliberately: the same wording ships
on the previous-logs and crust-gather legs beside them, and splitting
only the two new ones would create the asymmetry this PR exists to
remove. All four want the same treatment together.
- When a probe's cni-server lookup fails once and then succeeds while no
probe client is present, the reason line names the lookup for all three
attempts. The named cause did occur, so nothing false reaches the
artifact, but it is narrower than what happened.
- `hack/capture-dataplane.bats` now needs `yq` for one test, and unlike
three sibling suites it does not declare that at the top of the file.
The fan-out caps are sized in units of work while the backstops are
wall-clock, and the worst case exceeds both by multiples. That predates
this branch and is unchanged by it, but the new INCOMPLETE line is what
will make it visible in an artifact for the first time. Tracked
separately in #3671.
relates to #3658 #3660 #3654 #3657 #3663 #3649
### Screenshots
<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->
### Downstream repositories
<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.
Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.
IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:
1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
<!-- Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->
```release-note
fix(e2e): the host->pod data-plane capture no longer reports absence or reachability it never observed -- truncated list records are dropped instead of parsed, a LoadBalancer no probe reached is recorded as unprobed rather than reachable, an unread EndpointSlice backend is distinguished from an absent one, a tcpdump leg whose node was never identified is skipped instead of aimed at an unscheduled pod, and both callers report a capture their backstop cut short
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved data-plane capture reliability and reporting for timeouts,
failed or partial reads, and missing collectors.
* Prevented incomplete records and unknown probe results from being
treated as valid or reachable.
* Avoided capturing from unintended nodes when node information is
unavailable.
* Preserved partial diagnostic output while clearly reporting incomplete
captures.
* **Tests**
* Expanded coverage for capture outcomes, artifact accuracy, timeout
handling, and guarded node selection.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
355fc8a24a |
test(tests): let each bats file declare the EXIT traps it still holds (#3622)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The ban on EXIT-trap cleanup in `hack/*.bats` was enforced by two lists inside `hack/cozyreport.bats`: one naming the files known to be clean, one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard kept failing. It is worth being exact about how badly, because the history is sharper than "it went stale a few times". **The inventory was never correct on main for a single commit.** It landed in #3567 at 15:25:20. #3195 had landed `multus-install-cni-plugins.bats` carrying twelve traps at 15:24:37, forty-three seconds earlier. Replaying the old guard's own logic against the tree at the very commit that introduced it already gives `found != frozen`. Main then stayed red for roughly twenty-two and three quarter hours. The first repair, #3584, landed *already red* the next morning at 10:42:43, because #3548 had brought in `run-kubernetes-talos-diagnostics_test.bats` with eight traps at 10:40:03, under three minutes ahead of it. That fix was correct and bought zero green time. Green arrived only with the second repair, #3602. Neither pair of PRs shared a line, and each was green against its own base. That is the whole mechanism, and it is why a third one-line repair is not the answer. The mechanism is structural rather than careless. A change that adds a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second one landed. A file *arriving* with traps was worse still, because nothing in its author's diff pointed at that string at all. That is exactly how `run-kubernetes-talos-diagnostics_test.bats` got in, twice. That contention is not in the past tense. Two open PRs are editing that one line right now, and they disagree about what it should say: #3575 adds `run-kubernetes-talos-diagnostics_test.bats=8` to it, repeating a repair that has already landed, and #3441 removes `select-e2e_test.bats=15` from it, because it converts that file. Neither PR is about EXIT traps. Both have to touch that string anyway, and whichever lands second is wrong until someone edits it again. So this PR is not fixing a red main. It removes the thing that keeps making main red, which is why it is worth more than the one-line fix that is now the established habit. One honest caveat about its own landing. The textual-conflict property is steady-state: a branch forked *before* the declaration exists has no line to disagree with, so it converts traps in its own file, merges clean, and the count only goes wrong once both sides are on main. I checked this against #3441, which takes `select-e2e_test.bats` from fifteen traps to zero. Merged after this, that file would declare fifteen and hold none. What the move buys even in that case is that the red names a file the branch already edited, the repair is one line inside it, and rebasing before merge catches it on the branch's own CI. None of those three held against the central inventory. Whoever merges this should expect one such adjustment on the conversion branches still in flight. So this replaces both lists with a declaration each file makes about itself: one `# EXIT-TRAP DEBT: N` comment in its leading comment block. A file carrying no declaration must install no EXIT trap. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count get a real textual conflict instead of silently invalidating each other, and a change that leaves the traps alone edits nothing. **The include list is redundant, not lost.** It named the files proven clean, so that a trap reappearing in one of them would fail. Under the new rule those files carry no declaration, and a file with no declaration must hold zero traps, so a trap reappearing in any of them fails on its own, with no list to be on. Coverage widens rather than narrows: the two lists named twenty files between them, and the rule covers all fifty bats files under `hack/`, subdirectories included, plus the ones added tomorrow. Rebasing this branch onto current main is the property working. Main has since gained `hack/kubernetes-pre-delete-hook.bats` and `hack/tenant-pre-delete-hook.bats`, and `hack/cozyreport.bats` grew by some eight hundred lines. Neither new file installs an EXIT trap, so neither needed a declaration and neither needed an edit here; the rebase took no conflict at all. Under the inventory, each arriving file was a coin toss on whether somebody had remembered the string. To be precise about what the inventory could and could not do, since it is easy to overstate: it did *notice* a new file carrying traps. The string it compared was built by scanning the directory, so an arriving file appended a token and failed the comparison, which is exactly how main went red. What it could not do is let that file arrive without an edit in a foreign suite. Being seen and being absorbable are different properties, and only the second one decides whether two changes can land independently. The declaration is pinned in both directions. Declaring N while holding N+1 fails, obviously; declaring N while holding N−1 fails too. Without that second half the number becomes a ceiling and rots upward: somebody converts half a file, the declaration stays, and the guard quietly licenses traps that were removed long ago. It is read only from the leading comment block, under the shebang and above the first line of code. A `.bats` file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test; honouring it there would let an unrelated fixture excuse a real trap, silently, with nothing in that test's own diff looking like a declaration. A comment block is the region with no interior; stopping instead at the first `@test` would still read a line out of a helper's heredoc. Not every counted handler is debt. A trap inside an explicit subshell does not replace the one the `bats` binary installs, so a test failing inside `( … )` still prints its `not ok`, checked against a test-level trap in the same file, where the TAP line vanishes. `hack/e2e-test-openapi.bats` kills a backgrounded `kubectl proxy` exactly that way, and "convert it like the others" would leak a process holding a fixed port and wedge the next run. Its declaration now records the carve-out instead of scheduling a conversion, and because the ratchet is exact in both directions, *removing* that trap fails too, so the count protects the construct rather than marking it for deletion. `docs/agents/e2e-testing.md` previously scoped this exception to Chainsaw `script` steps only; it now names the BATS subshell case as well. The counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed `trap … EXIT; cd "$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap … EXIT` and `(trap … EXIT; true)`; upper case missed `trap … exit`, which bash and dash both install. All of those are real handlers that scored zero. The left boundary is the one worth dwelling on, because the inventory being replaced had none at all and *did* catch the semicolon form. Getting it wrong here would have narrowed coverage while the commit claimed to widen it. A plain word boundary is not enough either: `bootstrap ` ends in `trap `, and it has to keep scoring nothing, or the documented answer to a red guard (add a debt line) would buy a file a permanent licence for one real trap to silence a line that has none. Two handlers sharing one line are reported rather than counted, because the count is a count of lines and the second would otherwise arrive free. Splitting such a line properly needs a shell parser, since a semicolon inside a handler's own quoted action is not a separator, and guessing wrong undercounts, the one direction a ratchet cannot afford. **What this does not fix.** The declaration is still a loophole: a new file can write `# EXIT-TRAP DEBT: 8` instead of cleaning up, and nothing here makes that impossible. What changes is that the admission is local and visible. It sits at the top of the file it excuses, in front of whoever reviews that file, instead of being a number in a neighbouring suite nobody in that review is reading. Today's loophole is the same size and invisible. Three more limits, all stated in the guard's own header rather than left to be discovered. The scan is lexical, so a signal computed at runtime and a quoted action spanning physical lines without a backslash are both invisible. An exact count catches addition and removal but never substitution: swap the openapi file's subshell trap for a test-level one and the total stays 1. And the scan reads `.bats` only, so a handler arriving through a sourced `.sh` is outside it. `hack/e2e-chainsaw/_lib/run-kubernetes.sh` installs two right now, and each is benign for its own reason rather than by design: the one in `cozy_capture_tenant_talos` because that function is declared with `(` and so runs in a subshell, the one in `run_kubernetes_test` because no `@test` calls it despite being declared with `{`. Three `hack/*.bats` source that library, and two tests in the converted file call `cozy_capture_tenant_talos`, so flipping a single `(` to `{` reinstates a test-level handler in both of them with the guard green. Widening the scan to `.sh` would mean counting handlers that are correct in a script and wrong only in a test body, so the honest answer is that this is where the instrument stops. Three further boundaries, recorded here so they land as known edges rather than as surprises. The old include list also failed when a file named on it disappeared from the tree; the new rule can only judge a file that is present, so a deleted converted file goes unnoticed. That is a genuinely smaller check, though its absence shows up in the diff that deletes the file. The guard's own failure messages are code lines, so they are scanned by the pattern they belong to: they pass today only because no bare `EXIT` or `0` happens to follow the keyword in any of them, and a rewording that introduced one would make the file demand a debt of itself. It fails loudly rather than quietly, and the fixture writers and test titles already split the keyword from the signal for this reason, but the messages do not. Finally, `docs/agents/e2e-testing.md` bans test-level `EXIT` *and* `RETURN` traps, while every mechanical guard this repo has had, the one being deleted included, matches only `EXIT` and `0`. `hack/` holds no RETURN trap today, so nothing regresses here, but half of that documented rule has never had an executor. The guard moves out of `hack/cozyreport.bats` into `hack/bats-no-exit-trap.bats`, because its subject is every unit suite under `hack/` and not the report collector it grew up in. Living inside `cozyreport.bats` is precisely why unrelated PRs kept converging on one line. `hack/md-no-hardwrap.bats` is the neighbouring precedent. Separately, the eight EXIT traps in `hack/run-kubernetes-talos-diagnostics_test.bats` are converted to end-of-body cleanup rather than given a declaration. That file was written six weeks after the ban was documented, so "it predates the rule" is not available to it, and handing it a debt is literally the move that failed the last two times. The remaining files keep their declarations, since a mass conversion would collide with the branches that own them. One note on where the prose sits. The guard's own header carries only what a reader has to keep true: why the ban exists, the two carve-outs, where the declaration lives and why there, that the count is exact in both directions, why the marker is anchored at column zero, and where the lexical scan stops. Everything above about *how* the inventory failed (the timestamps, the PR numbers, the arithmetic) is deliberately not in the file. It is true today and will rot on its own; a comment nobody is obliged to maintain is how a file grows a paragraph that is quietly false. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note test(tests): each hack/*.bats file now declares its own remaining EXIT-trap debt in a `# EXIT-TRAP DEBT: N` header comment, checked by hack/bats-no-exit-trap.bats, replacing the central inventory in hack/cozyreport.bats ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Testing** - Added automated auditing for `EXIT` traps across Bats end-to-end tests, including validation of tracking declarations and edge cases. - Improved diagnostics tests by replacing trap-based temporary-directory cleanup with explicit cleanup steps. - Added tracking annotations for remaining trap-related cleanup work. - **Documentation** - Clarified when traps are permitted inside self-contained subshells and how remaining cleanup debt is reported. - Updated review guidance for consistent end-to-end test maintenance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
c6961541f3 |
fix(tests): report a data-plane capture cut short by its backstop
Both callers of the data-plane collector wrap it in a wall-clock backstop and threw the result away with `|| true`, while the previous-instance logs leg sitting beside each of them records the exit status and prints "INCOMPLETE (exit N); kept what landed in ...". When a backstop killed the collector, nothing said so, and a truncated dataplane/ directory read exactly like a complete one that found little. The collector itself now names every read of its own it could not finish, which left these two call sites as the only remaining way for that leg to die without a word. The Chainsaw one is the tighter of the pair -- 300s against 600s, because it shares an op envelope with the snapshot leg -- so it is both likelier to fire and more expensive to leave silent. In each place the status is read, never propagated: a diagnostic still cannot change a test's outcome. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
9b11db1b03 |
fix(e2e): report an LB no probe reached as unprobed, not reachable
Five inputs walked into the LB probe gate's skip branch having run zero probes, and all five came out as "probe: LB reachable or not probeable ... -- reachable, skipped": no node to probe from, a Service with no port, a probe node the lookup says runs no kube-ovn-cni pod, a host netns with no nc/curl/wget, and an exec that could not run (no timeout on PATH, exec denied, the pod gone mid-probe). None of them observed the address. An e2e image without a probe client would stamp every LoadBalancer in the cluster reachable and skip the whole heavy capture, which reads as a healthy datapath. A sixth way in, the cni-server lookup not answering at all, already reported unknown and still does. Zero probes now routes to that same explicit unknown outcome, and each way of getting there carries a reason the run can actually support. The two structural inputs are named where they are detected. The rest are told apart by the shape of the outcome set, which is the only place the difference survives: the probe helper emits an explicit unknown token only when the lookup itself did not answer, so a set that is present and wholly unknown names the lookup, while an empty set names the three ways a probe dies after the lookup answered. Those three stay together -- the exec's status is swallowed by design, since this collector never fails a job -- but none of them is reported as a lookup that went unanswered, because it did not. The reachable line now says which evidence produced it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
ed1c9e774b |
fix(e2e): say when an LB backend is unread rather than absent
The two EndpointSlice reads in the LB section discarded stderr and were interpreted by emptiness alone, so a refused or cut-off read wrote the same `<none>` into the artifact that a Service with no endpoints does. That is the assertion without the refutation: a reader holding the uploaded report, and not the run, has no way to tell which happened, and the section is at its weakest exactly when the apiserver is struggling. Both reads now carry their status beside the value, in the vocabulary the per-node captures already use. Reporting the status arms a landmine the silence used to hide: the target port was read with a jsonpath indexing `items[0]`, and client-go's evalArray has no allowMissingKeys escape, so an empty list is a hard error and kubectl exits 1. A Service with no endpointslices is an ordinary answer here and would have started reporting as a failed read, so the read now asks with `items[*]` -- empty output, exit 0 -- and takes the first value, the way the pod lookups already do. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
33f672f3fc |
fix(e2e): skip a tcpdump leg whose node was never identified
pod_on_node turns its node argument into `--field-selector spec.nodeName=<node>`, and an empty node asks for pods whose nodeName is empty -- the unscheduled ones. So an unidentified node did not yield "no pod found": it yielded the first pending kube-ovn-cni pod in the namespace, and the tcpdump then ran against a pod that is not on that node and may not be running at all. Both halves of the cross-node capture had this, and each gap opens in the very state its own leg exists to characterise. The announcer is unknown exactly when MetalLB is misbehaving; a Service with no ready endpoint is the ordinary shape of an LB outage, and the endpoint side carried two such lookups, the cni-server pod and the backend's OVS interface. In either state the artifact came out holding a tcpdump from an unrelated pending pod under an ANNOUNCER or ENDPOINT label, or a confusing exec failure -- both pointing away from the actual problem. Each leg is now guarded the way the static capture above it already was, and records in the artifact that it was skipped rather than leaving a silent gap. The guards stay at the call sites rather than moving into pod_on_node: that helper serves five callers, and an early return there would have to invent a status for a lookup nobody made. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
0cc49ad34b |
fix(e2e): drop truncated records in the data-plane capture filters
The record filters selected rows by field value and never checked the field count, so a list cut mid-record parsed as a valid record: an IP cut mid-octet still looks like an IP, and a cut inside the nodeName column still looks like a scheduled pod. The capture then probed an address the cluster never had and opened a per-node file for a node that does not exist -- confidently labelled data about objects that are artifacts of the cut. The kubectl jsonpath behind each filter emits a fixed field count per record, so a shorter row is a fragment by construction: seven for the Service list, six for the pod list, five for the endpoint list. Dropping it costs the reader nothing -- the read's own note already says the list did not finish. The endpoint filter is the one that matters most, because it stops at the first row it accepts: a fragment at its head is not one bad row among many, it is the backend, and its half-parsed node is the node every endpoint-side capture then aims at. Where the class lives in this file, so the map does not have to be rebuilt: four sites parse records, and the two loops walking pods and LoadBalancers are not among them -- they consume what these filters already returned, so a cut is caught upstream of them. Three of the four are the filters guarded here. The fourth, the MetalLB speaker loop, is at the limit of what a field count can do: its records carry two fields, a cut in the first leaves no separator and its existing emptiness guard already rejects that, and a cut in the second delivers every separator, which nothing in-band tells apart from a genuinely short node name. That limit is not confined to the speaker loop, and the comment on the first filter now says so rather than implying a guard that holds everywhere: a cut inside any record's LAST field passes a field count. It lands on a display-only column for Services, and on a decision column for the other two, where a truncated value matches neither state being excluded and the row is kept -- over-capturing rather than dropping evidence. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
f92306681d |
fix(e2e): an unanswered read is not the same as an absent thing
Five places turned an empty or missing result into a fact about the cluster: no affected pods, no ovn-central, no LoadBalancer service, no cilium-agent or ovs on a node, and a LoadBalancer recorded as reachable. Each is also what the script produces when the read behind it never answered, and with a note now saying the read failed, lines sat next to each other and disagreed. Each claim has a counterpart chosen by the status of the read that produced it, and each says what is left unknown rather than what the read did: no scheduled NotReady pods / whether any pod is affected no ovn-central pod in <ns> / whether <ns> runs ovn-central no Service type=LoadBalancer / whether any LoadBalancer needs capturing (no cilium-agent pod found) / could not determine whether one runs there LB reachable, skipped / whether the LB is reachable is unknown The last of those is the subtlest, because nothing about it looks like a claim of absence. host_http_probe resolves a cni-server before probing, and a lookup that never answered produced no probe outcome at all, which the decision helper read as "nothing failed" and the artifact stamped as reachable -- a verdict about an address the script never touched. The probe now emits an explicit unknown, and a set of outcomes that is entirely unknown stays unknown rather than collapsing into either answer. Only a wholly unknown set: one unrun probe beside real failures leaves the failures standing and still captures, because they are the evidence the capture exists to characterise. Wording that describes the read instead would be wrong in a case these bounds create. A list can fail after emitting rows, since a bound firing mid-stream leaves output on stdout and 124 in $?, so an empty result with a non-zero status can mean the read said nothing or that it said only uninteresting things. Before the bounds a read either answered or hung forever, and a partial answer was not a state this script could reach. For the same reason a failure note is unconditional on the status while the consequence -- skipped, unresolved -- is stated only by the branch that takes it, which is why the note inside pod_on_node names none at all: five call sites share it and their consequences differ. Two of these five are in the artifact rather than the log, and they are there because of the bounds. Unbounded, the per-node lookup blocked until the caller's backstop killed the script and no capture file was written; bounded, the script survives and writes the file, so a read that never answered would have been recorded as a pod that is not there or an address that answered. pod_on_node returns its status with the name and the memo stores it, since a hit that dropped it would launder a cutoff into a confident absence. The status travels packed with the value through stdout because every caller reads the helper through a command substitution, where a variable dies with the subshell -- the same property that makes the memo itself a file. Only answers and cutoffs are memoised: an instant failure costs nothing to ask again, and caching it would make one transient permanent for the whole run. Every note also ships beside the capture it explains, not only in the job log, because the reader who has the uploaded report and not the run is exactly the reader who cannot otherwise tell a capture that found nothing from one that never ran. Both sibling collectors hold that contract and this script's notes are modelled on them. The two phrasing helpers move above the sourcing guard so the unit suite can assert their branches directly. The stderr sink stays below it, because anything created above is created again by every test that sources this file and by every host that exits early for want of kubectl. What is deliberately left is the same distinction for the two EndpointSlice reads, which still discard stderr and report neither outcome. Each branch is pinned by a stub that answers one read and fails another, each partial-answer case by a stub that emits a row and then fails, the artifact cases in the artifact rather than the log, and the memo's status round-trip by consuming one lookup from both a miss and a hit. Every one of them carries a positive anchor as well, so a stub that stops short of the branch fails rather than passing quietly. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b921952fb0 |
perf(e2e): ask each per-node pod lookup once
The same (namespace, label, node) is asked up to three times per affected pod across the sections of this script. That was free while the first call hung forever and the script died inside it; with a per-read bound each repeat costs its own timeout, and against a wedged apiserver the repetition becomes the largest consumer of the caller's envelope -- enough that a 300s budget buys two pods where it could buy five. Answers are memoised, empty ones included: "there is no cilium-agent on this node" is exactly the lookup worth not repeating. The memo is a file rather than a variable, which is not a style choice. The walk over affected pods runs on the right-hand side of a pipeline, so it executes in a subshell and a variable memo is discarded at the end of it, leaving every lookup to be paid for again in the sections that follow. With the variable form the suite still recorded each of the three lookups twice. That is also why this is not the shape of the node memo above it, which is a space-delimited string matched with a case glob and is correct for a dedup living entirely inside the walk. Keys are compared as whole tab-separated fields, so the '=' inside a label selector is ordinary payload; what the scheme rests on is that no namespace, label selector or node name contains a tab, which Kubernetes names and selectors cannot. An empty pod list is also separated from one that never answered, so the branch below it stops reporting a failed read as a cluster with nothing wrong. Pinned by a stub that records every per-node lookup it is given: the test fails if any is issued twice. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
7d1137a889 |
fix(e2e): bound the dataplane capture's reads and name why one failed
Six `kubectl get` calls ran with no bound of their own, while all 27
`kubectl exec` calls in the same script went through one. Against an
apiserver that hangs rather than refuses, the first of them -- the
cluster-wide pod list -- held the script until the caller's backstop
killed it, so the capture wrote nothing at all. From the Chainsaw catch
that is worse than empty: the run is cut off by the outer `timeout -k`
and the script says nothing about having been cut short, so a truncated
capture reads exactly like a complete one that found no affected pods.
Measured against a kubectl stub that never returns: before, the script
was still blocked on its first call when a 60s backstop killed it;
after, it reaches its own end and says which read was cut off and what
that leaves missing. Roughly 4s at a 1s bound, 40s at the 20s default.
A per-read bound buys forward progress, not a smaller total. The sum of
the bounds here is far past any caller's envelope and deliberately so,
because the MAX_PODS and MAX_LBS caps bound work while the outer
`timeout -k` bounds wall clock. What changes is that no single hung read
can consume that envelope before anything is written.
The reporting around them is built to survive being read by someone who
was not there. Only 124 and 137 may name a timeout, since those are the
only statuses a bound produces and every other one is kubectl answering
-- refused, denied, a kind the cluster does not serve -- so the reads
keep their stderr instead of discarding it and quote it back. 137 is
described as what it is, a SIGKILL that cannot be told apart from the
bound's own kill grace. Notes print through `printf` rather than `echo`,
because under /bin/sh echo expands backslash escapes and kubectl's
stderr now flows through the logger: a message holding a literal \n
would otherwise split the note and forge a second "[capture-dataplane]"
line that reads as this script's own verdict, which needs no malice
given the jsonpath in these reads contains {"\n"} and kubectl quotes the
expression back when it fails to parse it.
The per-node lookups ask with `{.items[*].metadata.name}` rather than
`{.items[0]...}`. client-go's evalArray has no allowMissingKeys escape
the way evalField does, so indexing into an empty list is a hard error
and kubectl exits 1 -- which meant nothing while the status was
discarded, and would report "there is no cilium-agent on this node", the
ordinary answer, as a read that failed.
The tests drive the script as a subprocess against a stub kubectl that
hangs, fails instantly, or returns a forged line, and read what it
wrote. The stub that proves forward progress answers the pod list and
hangs afterwards, so the walk over affected pods is actually entered.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
575b41e1cd |
test(hack): let each bats file declare the EXIT traps it still holds
The ban on EXIT-trap cleanup in hack/*.bats was enforced by two lists inside hack/cozyreport.bats: one naming the files known to be clean, one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard kept failing. A change adding a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second landed. A file arriving with traps hit the same wall from the other side -- the inventory did notice it, since the string it compared was built by scanning the directory, but absorbing it meant an edit in a file nothing in the author's diff pointed at. Replace both lists with a declaration each file makes about itself: one "# EXIT-TRAP DEBT: N" comment, exact rather than a ceiling. A file carrying none must install no EXIT trap, which covers the converted files, the files that never had one, and every file added later. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count collide textually instead of silently. The declaration is read from the leading comment block, not from anywhere in the file. A .bats file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test. Reading it there as a statement about the whole file would let an unrelated fixture excuse a real trap, and would do it silently, since nothing in that test's own diff looks like a declaration. A comment block is the region with no interior: stopping instead at the first @test would still read a line out of a helper's heredoc. Not every counted handler is debt. A trap inside an explicit subshell does not replace the bats binary's own, so a test failing inside `( ... )` still prints its `not ok`. hack/e2e-test-openapi.bats kills a backgrounded kubectl proxy that way, and moving the kill to the end of the body would leak a process holding a fixed port. Its declaration records that rather than scheduling a conversion, and because the ratchet is exact, removing the trap fails too -- the count protects the construct. What the count cannot do is tell the two apart: substituting a test-level trap for the subshell one keeps the total at 1 and stays green, which the header states rather than leaves to be found. Counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed `trap ... EXIT; cd "$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap ... EXIT` and `(trap ... EXIT; true)`; upper case missed `trap ... exit`, which bash and dash both install. All are real handlers that scored zero, and the left boundary matters most, since the inventory being replaced had none and caught the semicolon form. A bare word boundary is not enough either way: `bootstrap ` ends in `trap `, and it must keep scoring nothing. A quoted signal counts for the same reason as the rest. Two handlers sharing one line are reported rather than counted, since the count is a count of lines and the second would otherwise arrive without moving the total; splitting the line properly needs a shell parser, a semicolon inside a handler's own action not being a separator, and guessing wrong undercounts -- the one direction a ratchet cannot afford. The scan recurses, so hack/e2e-apps/*.bats is covered rather than sitting one directory below the guard that claims the tree. It reads .bats and nothing else, so a handler arriving through a sourced .sh stays outside it -- hack/e2e-chainsaw/_lib/run-kubernetes.sh installs two, each benign for its own reason rather than by design: one sits in a function declared with `(` and so runs in a subshell, the other in a brace function no @test calls. That boundary is stated in the header rather than papered over. The guard moves to hack/bats-no-exit-trap.bats: its subject is every unit suite under hack/, not the report collector it grew up in. Its fixture helpers assemble the trap keyword and the signal from separate arguments, because the guard scans its own source and a fixture written as a literal would be counted as a real trap in it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Myasnikov Daniil
|
6a41d65f16 |
test(e2e): avoid ripgrep dependency in diagnostics tests
Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Myasnikov Daniil
|
ad1adbf785 |
test(e2e): fix diagnostic collector blind spots
Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Aleksei Sviridkin
|
12fe3256fd |
test: make negative bats assertions fail under cozytest
These bats files are all run by hack/cozytest.sh, which executes each @test under `set -e`. POSIX errexit is suppressed for a pipeline that begins with `!`, so a bare `! cmd` must-not-happen assertion never fails the test: when the forbidden condition actually occurs (cmd succeeds) the negated pipeline returns non-zero but errexit ignores it and the test passes anyway. These assertions were silently vacuous. Replace each with `if cmd; then echo "<reason>"; false; fi`, the idiom already used and documented elsewhere in these files — the if-condition is exempt from errexit and the `false` in the taken branch fails the test loudly. The forbidden condition now fails the test as intended: - bucket: read-only user must not be able to upload (access control) - gateway: child tenant must not have its own gateway HelmRelease - kuberture: external-dns public/internal hostname isolation - e2e-install: tenant-name validation must reject (not warn); the version ConfigMap must be gone after delete - build-matrix: talos/installer excluded from the parallel matrix - capture-dataplane: kube-dns and pending rows are filtered out Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
3ece61ba04 |
test(e2e): make the LoadBalancer announcer detection failover-robust
The dataplane diagnostic's announcer-node detection reported the wrong
node in two failover situations and could match the wrong IP:
- A node that announced an IP and then withdrew it (with no
re-announce) was still reported as the announcer, because a withdraw
did not retract a prior announce.
- Speaker logs are concatenated per-pod, not globally time-sorted, so
a stale former owner's announce could outrank the true current
owner purely by its position in the input.
- The IP was matched as a substring, so a query for 192.0.2.5 also
matched a 192.0.2.50 line.
Reimplement the detection to report a node only when that node's LAST
own announce/withdraw event for the exact IP is an announce, which is
robust to both concat order and L2 failover, and anchor the IP match to
a whole IP-literal token instead of a substring.
Also fix the heavy-capture budget: the MAX_LBS cap counted every
enumerated LB, so a broken LB enumerated after a cap's worth of
reachable LBs was never captured. Count only LBs actually captured, via
a small pure budget helper, so reachable/skipped LBs do not consume the
budget. Document that this cap bounds work, not wall-clock -- at two or
more unreachable LBs the real wall-clock bound is the outer backstop at
the call site.
Add bats coverage pinning the fixed announcer behavior (withdraw,
concat-order, exact-IP) and the captured-budget accounting.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
631cdaa069 |
test(e2e): capture the LoadBalancer datapath when an LB IP is unreachable
The on-failure dataplane diagnostic only captured host->pod CNI state for NotReady pods. A recurrent flake presents differently: a tenant Service type=LoadBalancer IP goes unreachable while its backend pod/VMI stays Ready and Running, so there is no NotReady pod and the hook captured nothing -- leaving the host->cross-node->bridged-worker-VMI->nodePort path uncharacterised, which is exactly where root-causing has stalled. Add a gated LoadBalancer-datapath section. It enumerates LoadBalancer services with an ingress IP, derives the EndpointSlice backend, the nodePort/externalTrafficPolicy, and the announcer node from the hostNetwork MetalLB speaker logs, then probes the LB IP from a host netns. Only an LB whose every probe fails gets the heavy capture, and that probe doubles as the traffic for the tcpdumps. For a failing LB it captures, on both the announcer and endpoint nodes, the host cilium LB map, kernel conntrack/neigh, br-int flows, ovn-controller timing, and a live tcpdump on the geneve tunnel and the backend tap -- localising the failing hop to host-cilium programming vs kube-ovn delivery. In-guest VMI capture is documented as a deferred stretch that needs tenant-cluster access. The existing NotReady-pod capture is unchanged. The new section is best-effort and time-boxed like the rest of the script, and a unit test pins the pure enumeration, announcer-node, and capture-or-skip logic against mock input. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |