Skip to content

e2e/qa: stop counting devices that cannot accept users as failures - #4168

Merged
ben-dz merged 4 commits into
mainfrom
bdz/qa-skip-devices-not-accepting-users
Aug 18, 2026
Merged

e2e/qa: stop counting devices that cannot accept users as failures#4168
ben-dz merged 4 commits into
mainfrom
bdz/qa-skip-devices-not-accepting-users

Conversation

@ben-dz

@ben-dz ben-dz commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary of Changes

Mainnet Beta QA failed 7 times over Aug 8-10; three (infra runs 31248009915, 31266248546, 31286128831) had one cause. qa_alldevices_unicast_test.go checks device readiness at five sites, but each increments FailedTests before that check, so the carve-out suppressed only the log line. laconic-{dfw,mia,was}-sw01 have been activated at max_users=0 since Aug 6 and the CLI refuses those connects outright; against cmh-mn-qa01's 13-device pool that read as 21-29% on a 20% per-host gate.

Gating that counter is not enough: Success() also requires non-zero packet counts, which a device that never connected cannot produce. Not-ready devices are therefore excluded from ComputeFailureStats entirely — per-host denominator included — behind a new Device.Ready() replacing the condition previously spelled out at six call sites.

Exclusion opens a fail-open path worth reviewing closely: the rate is then measured over a shrinking pool, and with every device excluded it is 0/0, where NaN > threshold is false. Coverage is now gated on the skipped rate against -skipped-threshold (default 0.5), fleet-wide and per host — one metro draining is a few percent of the fleet but all of a host's pool, and per-host is the gate that fires in practice. Testing nothing fails independently of that threshold, which -skipped-threshold=1 would otherwise switch off. The gate uses t.Errorf, not t.Fatalf, so the publishers still run: the skipped count lands as devices_skipped beside devices_tested in InfluxDB and ClickHouse, the latter via an idempotent ADD COLUMN IF NOT EXISTS, best-effort so a writer without ALTER rights still gets its per-device rows in.

Ready() is deliberately a subset of is_device_eligible_for_provisioning: a device at users_count + reserved_seats >= max_users hits the same CLI rejection and still counts as a failure, but fixing that means re-adding the capacity check #3697 removed, so it is a follow-up.

Separately, client_unicast.go wrapped an always-nil lastErr, so every failure this weekend read failed to ping after 3 retries: %!w(<nil>). It now reports packet counts.

Testing Verification

go test ./e2e/internal/qa/... and golangci-lint run --build-tags=qa ./e2e/... pass. The new subtests fail against the unfixed aggregation: the mainnet shape reports total=6 failed=3 for total=3 failed=0, and a host whose whole pool is drained was absent from PerHost rather than visible there as zero coverage.

Not verified: the qa-tagged tests need live fleet access, so nothing here runs against the real fleet and no run has written a devices_skipped row yet. Mainnet has 5 of ~92 non-transit devices unusable today, well under the 0.5 default.

Two pre-existing holes on the same axis go to separate PRs: a not-ready device still consumes a batch host slot, which on the two-host devnet run can trip the fewer than 2 clients connected fatal, and the Grafana active-device filter fails open on a query error but not on an empty successful response.

@ben-dz
ben-dz requested review from bgm-malbeclabs and nikw9944 and removed request for nikw9944 August 10, 2026 15:51
@ben-dz
ben-dz marked this pull request as ready for review August 10, 2026 15:52

@nikw9944 nikw9944 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exclusion is well-targeted and the premise checks out — is_device_eligible_for_provisioning is enforced client-side in the CLI before the onchain qa_allowlist can exempt the connect, so a max_users=0 device genuinely proves nothing. One gap in the new fail-open guard: it only fires at zero eligible devices, so a large partial drain still goes green over a fraction of the fleet with no durable signal of the lost coverage.

Comment thread e2e/qa_alldevices_unicast_test.go
@ben-dz
ben-dz requested a review from nikw9944 August 17, 2026 20:08

@nikw9944 nikw9944 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at 89dec13: the coverage gap is closed and verified in code, not just claimed. SkippedRate() now gates fleet-wide and per host against -skipped-threshold, zero tested devices still fails independently of that threshold, Errorf replaces Fatalf so the publishers below it run, and devices_skipped lands in both InfluxDB and ClickHouse. The per-host gate is strictly more lenient than the 20% failure gate it displaces for skipped devices, so it adds no new false reds. No further findings.

@ben-dz
ben-dz enabled auto-merge (squash) August 17, 2026 20:50
ben-dz added 4 commits August 18, 2026 13:17
The all-devices unicast test checked device readiness at four sites and
logged "Ignoring <x> failure for device not ready for users", but each
site incremented FailedTests before the check, so the carve-out only
suppressed the log line while the device still counted as failed.

Devices left activated with max_users=0 are drained on purpose and the
CLI refuses the connect, so exclude them from ComputeFailureStats
entirely rather than gating the counter: Success() also requires
PacketsSent > 0 && PacketsReceived > 0, which a device that never
connected can never satisfy.

Also fix the ping failure message, which wrapped an always-nil lastErr
and rendered as "failed to ping after 3 retries: %!w(<nil>)" for every
connectivity failure. Report the packet counts instead.
Excluding not-ready devices from the failure stats opened a fail-open
path: with every device excluded, totalDevices is 0, the overall rate is
NaN, and NaN > threshold is false, so a run that tested nothing passed.
Reachable when a fleet is drained metro-wide or newly created (devices
are created with max_users=0 and unlocked on activation).

Report which devices were skipped so an operator can tell "3 devices
skipped" from "3 devices tested and fine", and fail when nothing was
left to rate.
The zero-eligible-devices guard only caught a total drain: with 10 of a
13-device host pool refusing users, the run reported green over the
remaining 3 and the skipped set reached only the test log.

Coverage is now gated on the skipped rate at both resolutions against
-skipped-threshold (default 0.5) — fleet-wide misses a single metro
draining, which is a few percent of the fleet but all of one host's pool.
Testing nothing at all still fails independently of the threshold, which
-skipped-threshold=1 would otherwise switch off. The gate reports through
Errorf rather than Fatalf so the publishers below it still run, and the
skipped count publishes as devices_skipped next to devices_tested in
InfluxDB and ClickHouse, so a collapse that stays under the threshold is
visible on the dashboard.
@ben-dz
ben-dz force-pushed the bdz/qa-skip-devices-not-accepting-users branch from 89dec13 to c4a722b Compare August 18, 2026 19:18
@ben-dz
ben-dz merged commit 43e10bf into main Aug 18, 2026
37 checks passed
@ben-dz
ben-dz deleted the bdz/qa-skip-devices-not-accepting-users branch August 18, 2026 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants