diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f54bfcb97..1c9a747d19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ All notable changes to this project will be documented in this file. - `doublezero feed create` and `doublezero feed update` now read back every `--exchange` and `--group` argument, so a feed cannot name a metro or a multicast group that the ledger does not carry. A base58 argument used to pass straight through with no read, so `--group 4Nd1mBQtrMJVYVfKf2PJy9NZUZdTAsp7D4xWLs4gDB4T` created a feed whose group nobody can join. A code was always read back, so only the pubkey form changes. (#4172) - E2E/QA - `TestQA_MulticastSettlement`'s `validate_instant_allocation_price_matches_chain` no longer names a specific `doublezero_solana_version` in its skip path. Both the comment and the skip message said the pin was `0.5.10-1`; testnet has since moved to `0.5.11-1`, so a reader was told the pin was merely behind when in fact `instant_allocation_price` is in no release yet. They now name what actually gates the field — a doublezero-offchain release carrying doublezero-offchain#405 — and where the pin lives, neither of which goes stale as versions move. Comment and message only, no behaviour change. + - `TestQA_AllDevices_UnicastConnectivity` no longer counts a device that cannot accept users against its failure thresholds. Five sites already checked `activated && max_users > 0` and logged `Ignoring failure for device not ready for users`, but each incremented `FailedTests` before the check, so the carve-out suppressed only the log line while the device still counted as failed — and gating that counter alone would not have been enough, since `Success()` also requires a non-zero packet count a device that never connected cannot produce. Such devices are now excluded from `ComputeFailureStats` entirely, per-host denominator included. This is what failed mainnet-beta QA three times over 2026-08-08/09: `laconic-dfw-sw01`, `laconic-mia-sw01` and `laconic-was-sw01` have been activated at `max_users=0` since 08-06, the client CLI refuses those connects outright, and `cmh-mn-qa01` draws from a 13-device pool, so three unusable devices read as a 21-29% per-host rate against the 20% gate. The excluded codes are now reported in test output, so a skip is distinguishable from a pass. The exclusion is deliberately a subset of the program's `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, since narrowing that too would restore the capacity pre-filtering #3697 removed. A run that could not attempt more than half of the devices assigned to it — fleet-wide or on any single host — now fails (`-skipped-threshold`, default 0.5) rather than reporting green over the remnant, and testing nothing at all fails regardless of that threshold, where previously the rate was `0/0` and `NaN > threshold` passed silently. The gate is per host as well because a drained metro is a few percent of the fleet but all of one host's coverage. The count also publishes as `devices_skipped` next to `devices_tested` in InfluxDB and ClickHouse, so a collapse that stays under the threshold shows up on the dashboard and not only in the test log. Separately, a ping that never gets a reply reports its packet counts rather than `failed to ping after 3 retries: %!w()`; the wrapped error was always nil, because the retry loop returns early on any real failure. (#4168) ## [v0.34.0](https://github.com/malbeclabs/doublezero/compare/client/v0.33.0...client/v0.34.0) - 2026-08-07 diff --git a/e2e/internal/qa/clickhouse.go b/e2e/internal/qa/clickhouse.go index dd5b9233ba..347c93a8ef 100644 --- a/e2e/internal/qa/clickhouse.go +++ b/e2e/internal/qa/clickhouse.go @@ -23,8 +23,9 @@ type ClickhouseConfig struct { // ClickhouseConfigFromEnv reads ClickHouse connection settings from environment variables. // Returns nil if CLICKHOUSE_ADDR is not set, which disables ClickHouse publishing. // -// Schema changes to the tables created by PublishToClickhouse require manual ALTER TABLE -// or DROP TABLE on the ClickHouse side — there is no migration tooling (same as the controller). +// There is no migration tooling for the tables created by PublishToClickhouse (same as the +// controller): a new column must be added to the CREATE TABLE *and* to an idempotent ALTER +// in createQATables, or inserts break against deployments that already have the table. func ClickhouseConfigFromEnv() *ClickhouseConfig { addr := os.Getenv("CLICKHOUSE_ADDR") if addr == "" { @@ -76,7 +77,7 @@ func buildClickhouseOptions(addr, db, user, pass string, disableTLS bool) *click // PublishToClickhouse writes per-device results and a summary row to ClickHouse. // Both tables are created automatically on first use (CREATE TABLE IF NOT EXISTS). // If cfg is nil, publishing is skipped silently. -func PublishToClickhouse(ctx context.Context, log *slog.Logger, cfg *ClickhouseConfig, env string, results []DeviceTestResult, duration time.Duration) error { +func PublishToClickhouse(ctx context.Context, log *slog.Logger, cfg *ClickhouseConfig, env string, results []DeviceTestResult, skippedDevices int, duration time.Duration) error { if cfg == nil { log.Debug("ClickHouse publishing skipped: no configuration") return nil @@ -101,11 +102,11 @@ func PublishToClickhouse(ctx context.Context, log *slog.Logger, cfg *ClickhouseC return err } - if err := insertMetadata(ctx, conn, cfg.DB, env, results, duration); err != nil { + if err := insertMetadata(ctx, conn, cfg.DB, env, results, skippedDevices, duration); err != nil { return err } - log.Debug("published QA results to ClickHouse", "devices", len(results)) + log.Debug("published QA results to ClickHouse", "devices", len(results), "skipped", skippedDevices) return nil } @@ -130,6 +131,7 @@ func createQATables(ctx context.Context, conn clickhouse.Conn, db string) error devices_tested UInt32, devices_success UInt32, devices_failed UInt32, + devices_skipped UInt32, duration_s Float64 ) ENGINE = MergeTree PARTITION BY toYYYYMM(timestamp) @@ -143,6 +145,13 @@ func createQATables(ctx context.Context, conn clickhouse.Conn, db string) error return fmt.Errorf("failed to create QA table: %w", err) } } + + // The CREATE above is a no-op where the table predates devices_skipped. Best + // effort so a writer without ALTER rights still gets its per-device results + // in; a column that really is missing surfaces on the metadata insert. + _ = conn.Exec(ctx, fmt.Sprintf( + `ALTER TABLE "%s".qa_alldevices_metadata ADD COLUMN IF NOT EXISTS devices_skipped UInt32 AFTER devices_failed`, db, + )) return nil } @@ -172,7 +181,7 @@ func insertResults(ctx context.Context, conn clickhouse.Conn, db, env string, re return batch.Close() } -func insertMetadata(ctx context.Context, conn clickhouse.Conn, db, env string, results []DeviceTestResult, duration time.Duration) error { +func insertMetadata(ctx context.Context, conn clickhouse.Conn, db, env string, results []DeviceTestResult, skippedDevices int, duration time.Duration) error { var successCount, failedCount uint32 for _, r := range results { if r.Success { @@ -183,13 +192,13 @@ func insertMetadata(ctx context.Context, conn clickhouse.Conn, db, env string, r } batch, err := conn.PrepareBatch(ctx, fmt.Sprintf( - `INSERT INTO "%s".qa_alldevices_metadata (timestamp, env, devices_tested, devices_success, devices_failed, duration_s)`, db, + `INSERT INTO "%s".qa_alldevices_metadata (timestamp, env, devices_tested, devices_success, devices_failed, devices_skipped, duration_s)`, db, )) if err != nil { return fmt.Errorf("failed to prepare metadata batch: %w", err) } - if err := batch.Append(time.Now(), env, uint32(len(results)), successCount, failedCount, duration.Seconds()); err != nil { + if err := batch.Append(time.Now(), env, uint32(len(results)), successCount, failedCount, uint32(skippedDevices), duration.Seconds()); err != nil { return fmt.Errorf("failed to append metadata row: %w", err) } diff --git a/e2e/internal/qa/client.go b/e2e/internal/qa/client.go index 0e344b21eb..05f0f32bb1 100644 --- a/e2e/internal/qa/client.go +++ b/e2e/internal/qa/client.go @@ -108,6 +108,20 @@ type Device struct { DeviceType serviceability.DeviceDeviceType } +// Ready reports whether a connect against this device can get far enough to +// tell us anything. MaxUsers == 0 means drained or never enabled; either way the +// CLI's own precheck refuses the connect ("Device is not accepting more users") +// before the onchain qa_allowlist can exempt us, so failures against it say +// nothing about the network and must not count toward QA failure rates. +// +// This is a subset of the program's is_device_eligible_for_provisioning, which +// also requires UsersCount+ReservedSeats < MaxUsers; a device at capacity still +// reports as ready here. MaxUnicastUsers is deliberately not consulted: 0 there +// means "no per-type limit", the inverse of drained. +func (d *Device) Ready() bool { + return d.Status == serviceability.DeviceStatusActivated && d.MaxUsers > 0 +} + type Client struct { log *slog.Logger grpcClient pb.QAAgentServiceClient diff --git a/e2e/internal/qa/client_unicast.go b/e2e/internal/qa/client_unicast.go index 0eccc0035c..afb6ad554c 100644 --- a/e2e/internal/qa/client_unicast.go +++ b/e2e/internal/qa/client_unicast.go @@ -136,14 +136,12 @@ func (c *Client) TestUnicastConnectivity(t *testing.T, ctx context.Context, targ } var lastResp *pb.PingResult - var lastErr error for i := range unicastPingMaxRetries { resp, err := c.pingOnce(ctx, targetIP, sourceIP, iface) if err != nil { return nil, fmt.Errorf("failed to ping: %w", err) } lastResp = resp - lastErr = err if resp.PacketsSent == 0 { c.log.Warn("No packets sent", @@ -227,7 +225,18 @@ func (c *Client) TestUnicastConnectivity(t *testing.T, ctx context.Context, targ PacketsReceived: lastResp.PacketsReceived, } } - return result, fmt.Errorf("failed to ping after %d retries: %w", unicastPingMaxRetries, lastErr) + return result, pingFailureError(unicastPingMaxRetries, lastResp) +} + +// pingFailureError describes a ping that never got a reply. Every attempt's RPC +// succeeded — the loop above returns early otherwise — so there is no error to +// wrap and the packet counts are the only diagnostic available. +func pingFailureError(retries int, lastResp *pb.PingResult) error { + if lastResp == nil { + return fmt.Errorf("failed to ping after %d retries: no ping result returned", retries) + } + return fmt.Errorf("failed to ping after %d retries: %d/%d packets received on the last attempt", + retries, lastResp.PacketsReceived, lastResp.PacketsSent) } func (c *Client) pingOnce(ctx context.Context, targetIP string, sourceIP string, iface string) (*pb.PingResult, error) { diff --git a/e2e/internal/qa/client_unicast_test.go b/e2e/internal/qa/client_unicast_test.go index fe62777705..3621c2ed1d 100644 --- a/e2e/internal/qa/client_unicast_test.go +++ b/e2e/internal/qa/client_unicast_test.go @@ -322,3 +322,38 @@ func TestFindIBRLStatus(t *testing.T) { }) } } + +func TestPingFailureError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + lastResp *pb.PingResult + want string + }{ + { + name: "total loss reports the counts", + lastResp: &pb.PingResult{PacketsSent: 40, PacketsReceived: 0}, + want: "failed to ping after 3 retries: 0/40 packets received on the last attempt", + }, + { + name: "no packets sent reports zeroes", + lastResp: &pb.PingResult{}, + want: "failed to ping after 3 retries: 0/0 packets received on the last attempt", + }, + { + name: "no response at all says so", + lastResp: nil, + want: "failed to ping after 3 retries: no ping result returned", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := pingFailureError(3, tt.lastResp) + require.EqualError(t, err, tt.want) + require.NotContains(t, err.Error(), "%!w", "must not format a nil error with %%w") + }) + } +} diff --git a/e2e/internal/qa/device_assignment.go b/e2e/internal/qa/device_assignment.go index 0dd3738218..51cc6afd60 100644 --- a/e2e/internal/qa/device_assignment.go +++ b/e2e/internal/qa/device_assignment.go @@ -305,11 +305,18 @@ func AssignDevicesToClients(devices []*Device, clients []*Client, clientLatencie // HostFailureStats aggregates per-host failure information after deduping // repeated tests of the same (host, device) pair. type HostFailureStats struct { - Total int // unique devices assigned to this host + Total int // unique testable devices assigned to this host Failed int // unique devices that never succeeded on this host + Skipped int // unique devices excluded for not accepting users; disjoint from Total FailedDevices []string // sorted, deduped device codes } +// SkippedRate is this host's share of assigned devices that could not accept +// users. See FailureStats.SkippedRate for the fleet-wide equivalent. +func (h HostFailureStats) SkippedRate() float64 { + return skippedRate(h.Total, h.Skipped) +} + // DeviceRetest describes a (host, device) pair that was tested more than once. type DeviceRetest struct { Host string @@ -331,12 +338,30 @@ type FailureStats struct { DeviceResults []DeviceTestResult // one per unique device code PerHost map[string]HostFailureStats // keyed by host Retests []DeviceRetest // entries where Attempts > 1 + Skipped []string // sorted codes of devices that could not accept users +} + +// SkippedRate is the fraction of assigned devices left out of the tallies for +// not accepting users. +func (s FailureStats) SkippedRate() float64 { + return skippedRate(len(s.DeviceResults), len(s.Skipped)) +} + +// skippedRate is 1 when nothing was testable at all, so a gate on it never +// divides by zero nor reads a NaN as a pass. +func skippedRate(tested, skipped int) float64 { + assigned := tested + skipped + if assigned == 0 { + return 1 + } + return float64(skipped) / float64(assigned) } // ComputeFailureStats walks batchData once and applies the "any success // counts as success" rule per device. Repeated tests of the same // (host, device) collapse into a single result for both the overall device -// list and per-host stats. +// list and per-host stats. Devices that are not Ready are left out of the +// failure tallies and counted as skipped instead, fleet-wide and per host. func ComputeFailureStats(batchData BatchData) FailureStats { // hostDeviceAttempts[host][code] = number of attempts hostDeviceAttempts := make(map[string]map[string]int) @@ -344,12 +369,26 @@ func ComputeFailureStats(batchData BatchData) FailureStats { hostDeviceSuccesses := make(map[string]map[string]int) deviceSucceeded := make(map[string]bool) devicePubkey := make(map[string]string) + skipped := make(map[string]struct{}) + // hostSkipped[host] = set of codes skipped on that host + hostSkipped := make(map[string]map[string]struct{}) batchNums := slices.Sorted(maps.Keys(batchData)) for _, batchNum := range batchNums { hosts := slices.Sorted(maps.Keys(batchData[batchNum])) for _, host := range hosts { assignment := batchData[batchNum][host] + // A device that cannot accept users is excluded entirely, not + // counted as a failure: it must not inflate the denominator either. + // Skipped is the caller's audit trail for what was left out. + if !assignment.Device.Ready() { + skipped[assignment.Device.Code] = struct{}{} + if hostSkipped[host] == nil { + hostSkipped[host] = make(map[string]struct{}) + } + hostSkipped[host][assignment.Device.Code] = struct{}{} + continue + } code := assignment.Device.Code if hostDeviceAttempts[host] == nil { hostDeviceAttempts[host] = make(map[string]int) @@ -389,6 +428,14 @@ func ComputeFailureStats(batchData BatchData) FailureStats { perHost[host] = stats } + // A host left with nothing testable gets an entry with Total 0: absent from + // the map, its lost coverage would be indistinguishable from a clean run. + for host, codes := range hostSkipped { + hs := perHost[host] + hs.Skipped = len(codes) + perHost[host] = hs + } + var retests []DeviceRetest hosts := slices.Sorted(maps.Keys(hostDeviceAttempts)) for _, host := range hosts { @@ -413,5 +460,6 @@ func ComputeFailureStats(batchData BatchData) FailureStats { DeviceResults: deviceResults, PerHost: perHost, Retests: retests, + Skipped: slices.Sorted(maps.Keys(skipped)), } } diff --git a/e2e/internal/qa/device_assignment_test.go b/e2e/internal/qa/device_assignment_test.go index fa64345016..38718e6460 100644 --- a/e2e/internal/qa/device_assignment_test.go +++ b/e2e/internal/qa/device_assignment_test.go @@ -5,6 +5,8 @@ import ( "net" "reflect" "testing" + + serviceability "github.com/malbeclabs/doublezero/smartcontract/sdk/go/serviceability" ) func TestAssignDevicesToClients(t *testing.T) { @@ -554,9 +556,14 @@ func TestComputeFailureStats(t *testing.T) { return &BatchResult{Device: d, PacketsSent: 10, PacketsReceived: 0, FailedTests: 1} } - dev1 := &Device{Code: "dev1", PubKey: "pk1"} - dev2 := &Device{Code: "dev2", PubKey: "pk2"} - dev3 := &Device{Code: "dev3", PubKey: "pk3"} + // readyDevice builds a device that can accept users, so its results count. + readyDevice := func(code, pubkey string) *Device { + return &Device{Code: code, PubKey: pubkey, Status: serviceability.DeviceStatusActivated, MaxUsers: 128} + } + + dev1 := readyDevice("dev1", "pk1") + dev2 := readyDevice("dev2", "pk2") + dev3 := readyDevice("dev3", "pk3") t.Run("empty batch data", func(t *testing.T) { stats := ComputeFailureStats(BatchData{}) @@ -731,4 +738,180 @@ func TestComputeFailureStats(t *testing.T) { t.Errorf("retests: got %+v, want %+v", stats.Retests, wantRetests) } }) + + // A device left activated with max_users=0 is drained on purpose: the CLI + // refuses the connect, so it must not count against the host or inflate its + // denominator. This mirrors the mainnet-beta shape where three such devices + // pushed a 13-device host past the 20% per-host threshold. + t.Run("ignores activated devices with max_users=0", func(t *testing.T) { + drained := []*Device{ + {Code: "drained1", PubKey: "dpk1", Status: serviceability.DeviceStatusActivated}, + {Code: "drained2", PubKey: "dpk2", Status: serviceability.DeviceStatusActivated}, + {Code: "drained3", PubKey: "dpk3", Status: serviceability.DeviceStatusActivated}, + } + + batchData := BatchData{ + 0: {"hostA": pass(dev1)}, + 1: {"hostA": pass(dev2)}, + 2: {"hostA": pass(dev3)}, + 3: {"hostA": fail(drained[0])}, + 4: {"hostA": fail(drained[1])}, + 5: {"hostA": fail(drained[2])}, + } + stats := ComputeFailureStats(batchData) + + wantDevices := []DeviceTestResult{ + {DeviceCode: "dev1", DevicePubkey: "pk1", Success: true}, + {DeviceCode: "dev2", DevicePubkey: "pk2", Success: true}, + {DeviceCode: "dev3", DevicePubkey: "pk3", Success: true}, + } + if !reflect.DeepEqual(stats.DeviceResults, wantDevices) { + t.Errorf("device results: got %+v, want %+v", stats.DeviceResults, wantDevices) + } + hostA := stats.PerHost["hostA"] + if hostA.Total != 3 || hostA.Failed != 0 { + t.Errorf("hostA: got %+v, want total=3 failed=0", hostA) + } + if len(hostA.FailedDevices) != 0 { + t.Errorf("hostA failedDevices: got %v, want none", hostA.FailedDevices) + } + if len(stats.Retests) != 0 { + t.Errorf("expected no retests, got %+v", stats.Retests) + } + wantSkipped := []string{"drained1", "drained2", "drained3"} + if !reflect.DeepEqual(stats.Skipped, wantSkipped) { + t.Errorf("skipped: got %v, want %v", stats.Skipped, wantSkipped) + } + }) + + // Every device excluded must leave DeviceResults empty so the caller can + // tell "nothing was eligible" from "everything passed" — 0/0 is NaN and + // silently satisfies every threshold comparison. + t.Run("all devices skipped yields no results to rate", func(t *testing.T) { + drained := &Device{Code: "drained1", PubKey: "dpk1", Status: serviceability.DeviceStatusActivated} + + stats := ComputeFailureStats(BatchData{0: {"hostA": fail(drained)}}) + + if len(stats.DeviceResults) != 0 { + t.Errorf("expected no device results, got %+v", stats.DeviceResults) + } + if !reflect.DeepEqual(stats.Skipped, []string{"drained1"}) { + t.Errorf("skipped: got %v, want [drained1]", stats.Skipped) + } + if stats.SkippedRate() != 1 { + t.Errorf("skipped rate: got %v, want 1", stats.SkippedRate()) + } + hostA := stats.PerHost["hostA"] + if hostA.Total != 0 || hostA.Skipped != 1 { + t.Errorf("hostA: got %+v, want total=0 skipped=1", hostA) + } + }) + + t.Run("ignores devices that are not activated", func(t *testing.T) { + pending := &Device{Code: "pending1", PubKey: "ppk1", MaxUsers: 128} + + batchData := BatchData{ + 0: {"hostA": pass(dev1), "hostB": fail(pending)}, + } + stats := ComputeFailureStats(batchData) + + wantDevices := []DeviceTestResult{ + {DeviceCode: "dev1", DevicePubkey: "pk1", Success: true}, + } + if !reflect.DeepEqual(stats.DeviceResults, wantDevices) { + t.Errorf("device results: got %+v, want %+v", stats.DeviceResults, wantDevices) + } + // hostB tested nothing, which must be visible as skipped coverage rather + // than as an absent host indistinguishable from a clean run. + hostB := stats.PerHost["hostB"] + if hostB.Total != 0 || hostB.Skipped != 1 || hostB.Failed != 0 { + t.Errorf("hostB: got %+v, want total=0 skipped=1 failed=0", hostB) + } + if hostB.SkippedRate() != 1 { + t.Errorf("hostB skipped rate: got %v, want 1", hostB.SkippedRate()) + } + }) + + // A metro draining takes one host's whole pool while barely moving the + // fleet-wide rate, so the skipped count has to be attributed per host. + t.Run("attributes skipped devices to their host", func(t *testing.T) { + drained1 := &Device{Code: "drained1", PubKey: "dpk1", Status: serviceability.DeviceStatusActivated} + drained2 := &Device{Code: "drained2", PubKey: "dpk2", Status: serviceability.DeviceStatusActivated} + + batchData := BatchData{ + 0: {"hostA": fail(drained1), "hostB": pass(dev1)}, + 1: {"hostA": fail(drained2), "hostB": pass(dev2)}, + 2: {"hostA": fail(drained2), "hostB": pass(dev3)}, + } + stats := ComputeFailureStats(batchData) + + hostA := stats.PerHost["hostA"] + if hostA.Total != 0 || hostA.Skipped != 2 { + t.Errorf("hostA: got %+v, want total=0 skipped=2 (drained2 deduped)", hostA) + } + hostB := stats.PerHost["hostB"] + if hostB.Total != 3 || hostB.Skipped != 0 || hostB.SkippedRate() != 0 { + t.Errorf("hostB: got %+v rate=%v, want total=3 skipped=0 rate=0", hostB, hostB.SkippedRate()) + } + // 2 of 5 fleet-wide stays under a 0.5 gate while hostA tested nothing. + if stats.SkippedRate() != 2.0/5.0 { + t.Errorf("fleet skipped rate: got %v, want %v", stats.SkippedRate(), 2.0/5.0) + } + }) + + t.Run("still counts a ready device that fails", func(t *testing.T) { + drained := &Device{Code: "drained1", PubKey: "dpk1", Status: serviceability.DeviceStatusActivated} + + batchData := BatchData{ + 0: {"hostA": fail(dev1)}, + 1: {"hostA": fail(drained)}, + } + stats := ComputeFailureStats(batchData) + + wantDevices := []DeviceTestResult{ + {DeviceCode: "dev1", DevicePubkey: "pk1", Success: false}, + } + if !reflect.DeepEqual(stats.DeviceResults, wantDevices) { + t.Errorf("device results: got %+v, want %+v", stats.DeviceResults, wantDevices) + } + hostA := stats.PerHost["hostA"] + if hostA.Total != 1 || hostA.Failed != 1 { + t.Errorf("hostA: got %+v, want total=1 failed=1", hostA) + } + if !reflect.DeepEqual(hostA.FailedDevices, []string{"dev1"}) { + t.Errorf("hostA failedDevices: got %v, want [dev1]", hostA.FailedDevices) + } + }) +} + +func TestFailureStatsSkippedRate(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + tested int + skipped []string + want float64 + }{ + {name: "nothing skipped", tested: 4, want: 0}, + {name: "partial drain", tested: 10, skipped: []string{"d1", "d2", "d3"}, want: 3.0 / 13.0}, + {name: "majority drained", tested: 3, skipped: []string{"d1", "d2", "d3", "d4", "d5", "d6", "d7", "d8", "d9", "d10"}, want: 10.0 / 13.0}, + // Nothing testable must read as a total loss of coverage, not as 0/0. + {name: "everything skipped", skipped: []string{"d1"}, want: 1}, + {name: "nothing assigned", want: 1}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + stats := FailureStats{ + DeviceResults: make([]DeviceTestResult, tt.tested), + Skipped: tt.skipped, + } + if got := stats.SkippedRate(); got != tt.want { + t.Errorf("SkippedRate() = %v, want %v", got, tt.want) + } + }) + } } diff --git a/e2e/internal/qa/metrics.go b/e2e/internal/qa/metrics.go index 0606045081..2d04f018e3 100644 --- a/e2e/internal/qa/metrics.go +++ b/e2e/internal/qa/metrics.go @@ -47,7 +47,7 @@ func MetricsConfigFromEnv() *MetricsConfig { } } -func PublishMetrics(ctx context.Context, log *slog.Logger, cfg *MetricsConfig, env string, results []DeviceTestResult, duration time.Duration) error { +func PublishMetrics(ctx context.Context, log *slog.Logger, cfg *MetricsConfig, env string, results []DeviceTestResult, skippedDevices int, duration time.Duration) error { if cfg == nil { log.Debug("Metrics publishing skipped: no InfluxDB configuration") return nil @@ -98,6 +98,7 @@ func PublishMetrics(ctx context.Context, log *slog.Logger, cfg *MetricsConfig, e "devices_tested": len(results), "devices_success": successCount, "devices_failed": failureCount, + "devices_skipped": skippedDevices, "duration_s": duration.Seconds(), }, now, @@ -112,6 +113,7 @@ func PublishMetrics(ctx context.Context, log *slog.Logger, cfg *MetricsConfig, e "devices", len(results), "success", successCount, "failed", failureCount, + "skipped", skippedDevices, ) return nil diff --git a/e2e/qa_alldevices_unicast_test.go b/e2e/qa_alldevices_unicast_test.go index 8c932bffb7..73f3990f8f 100644 --- a/e2e/qa_alldevices_unicast_test.go +++ b/e2e/qa_alldevices_unicast_test.go @@ -26,6 +26,7 @@ var ( allocateAddrHosts = flag.String("allocate-addr-hosts", "", "comma separated list of hosts that will have `--allocate-addr` passed to `doublezero connect ibrl`") failureThreshold = flag.Float64("failure-threshold", 0.1, "maximum allowed overall device failure rate (0.0-1.0) before the test is marked as failed") perHostFailureThreshold = flag.Float64("per-host-failure-threshold", 0.2, "maximum allowed per-host device failure rate (0.0-1.0) before the test is marked as failed") + skippedThreshold = flag.Float64("skipped-threshold", 0.5, "maximum allowed rate of assigned devices skipped for not accepting users (0.0-1.0) before the test is marked as failed") ) func TestQA_AllDevices_UnicastConnectivity(t *testing.T) { @@ -232,6 +233,25 @@ func TestQA_AllDevices_UnicastConnectivity(t *testing.T) { // Evaluate failure rates against threshold totalDevices := len(stats.DeviceResults) + assignedDevices := totalDevices + len(stats.Skipped) + + if len(stats.Skipped) > 0 { + t.Logf("SKIPPED %d of %d devices not accepting users (excluded from failure rates): %s", + len(stats.Skipped), assignedDevices, strings.Join(stats.Skipped, ", ")) + } + + // A run whose fleet was mostly unusable proves nothing about the network and + // must not report green over the remnant. Errorf, not Fatalf: the metrics + // published below are the only durable record of the lost coverage. + if skippedRate := stats.SkippedRate(); skippedRate > *skippedThreshold { + t.Errorf("Skipped device rate %.1f%% (%d/%d not accepting users) exceeds threshold %.1f%%; only %d devices were tested", + skippedRate*100, len(stats.Skipped), assignedDevices, *skippedThreshold*100, totalDevices) + } + // Testing nothing fails even at -skipped-threshold=1, which the gate above satisfies. + if totalDevices == 0 { + t.Errorf("No devices were eligible for testing; all %d assigned devices were not accepting users", assignedDevices) + } + failedDevices := 0 var failedDeviceCodes []string for _, result := range stats.DeviceResults { @@ -241,21 +261,34 @@ func TestQA_AllDevices_UnicastConnectivity(t *testing.T) { } } - overallRate := float64(failedDevices) / float64(totalDevices) - log.Debug("Overall failure rate", - "failed", failedDevices, - "total", totalDevices, - "rate", fmt.Sprintf("%.1f%%", overallRate*100), - "threshold", fmt.Sprintf("%.1f%%", *failureThreshold*100), - ) - if overallRate > *failureThreshold { - t.Errorf("Overall device failure rate %.1f%% (%d/%d) exceeds threshold %.1f%%. Failed devices: %s", - overallRate*100, failedDevices, totalDevices, *failureThreshold*100, - strings.Join(failedDeviceCodes, ", ")) + // 0/0 is NaN and NaN > threshold is false, so an all-skipped run would pass + // this silently; the gate above has already reported it. + if totalDevices > 0 { + overallRate := float64(failedDevices) / float64(totalDevices) + log.Debug("Overall failure rate", + "failed", failedDevices, + "total", totalDevices, + "rate", fmt.Sprintf("%.1f%%", overallRate*100), + "threshold", fmt.Sprintf("%.1f%%", *failureThreshold*100), + ) + if overallRate > *failureThreshold { + t.Errorf("Overall device failure rate %.1f%% (%d/%d) exceeds threshold %.1f%%. Failed devices: %s", + overallRate*100, failedDevices, totalDevices, *failureThreshold*100, + strings.Join(failedDeviceCodes, ", ")) + } } + // Coverage is gated per host as well: a drained metro is a few percent of the + // fleet but all of one host's pool. for _, host := range slices.Sorted(maps.Keys(stats.PerHost)) { hs := stats.PerHost[host] + if skippedRate := hs.SkippedRate(); skippedRate > *skippedThreshold { + t.Errorf("Host %s skipped device rate %.1f%% (%d/%d not accepting users) exceeds threshold %.1f%%; only %d devices were tested", + host, skippedRate*100, hs.Skipped, hs.Skipped+hs.Total, *skippedThreshold*100, hs.Total) + } + if hs.Total == 0 { + continue + } hostRate := float64(hs.Failed) / float64(hs.Total) log.Debug("Per-host failure rate", "host", host, @@ -270,10 +303,10 @@ func TestQA_AllDevices_UnicastConnectivity(t *testing.T) { } } - if err := qa.PublishMetrics(ctx, log, qa.MetricsConfigFromEnv(), envArg, stats.DeviceResults, time.Since(startTime)); err != nil { + if err := qa.PublishMetrics(ctx, log, qa.MetricsConfigFromEnv(), envArg, stats.DeviceResults, len(stats.Skipped), time.Since(startTime)); err != nil { log.Error("Failed to publish metrics", "error", err) } - if err := qa.PublishToClickhouse(ctx, log, qa.ClickhouseConfigFromEnv(), envArg, stats.DeviceResults, time.Since(startTime)); err != nil { + if err := qa.PublishToClickhouse(ctx, log, qa.ClickhouseConfigFromEnv(), envArg, stats.DeviceResults, len(stats.Skipped), time.Since(startTime)); err != nil { log.Error("Failed to publish metrics to ClickHouse", "error", err) } } @@ -371,7 +404,7 @@ func connectClientsAndWaitForRoutes( if err != nil { log.Error("Failed to start connection", "client", c.Host, "device", device.Code, "error", err) batch[c.Host].FailedTests++ - if device.Status == serviceability.DeviceStatusActivated && device.MaxUsers > 0 { + if device.Ready() { t.Logf("DEVICE FAILURE: failed to connect client %s to device %s: %v", c.Host, device.Code, err) } else { log.Warn("Ignoring connection failure for device not ready for users", "device", device.Code, "status", device.Status, "maxUsers", device.MaxUsers) @@ -386,7 +419,7 @@ func connectClientsAndWaitForRoutes( if err != nil { log.Error("Client failed to reach status up", "client", c.Host, "error", err) batch[c.Host].FailedTests++ - if device.Status == serviceability.DeviceStatusActivated && device.MaxUsers > 0 { + if device.Ready() { t.Logf("DEVICE FAILURE: failed to wait for status for client %s: %v", c.Host, err) } else { log.Warn("Ignoring status failure for device not ready for users", "device", device.Code, "status", device.Status, "maxUsers", device.MaxUsers) @@ -433,7 +466,7 @@ func connectClientsAndWaitForRoutes( if err := c.WaitForRoutes(ctx, targets); err != nil { log.Error("Failed to wait for routes", "client", c.Host, "error", err) batch[c.Host].FailedTests++ - if device.Status == serviceability.DeviceStatusActivated && device.MaxUsers > 0 { + if device.Ready() { t.Logf("DEVICE FAILURE: failed to wait for routes on client %s: %v", c.Host, err) } else { log.Warn("Ignoring route failure for device not ready for users", "device", device.Code, "status", device.Status, "maxUsers", device.MaxUsers) @@ -482,10 +515,8 @@ func runConnectivitySubtests( mu.Lock() failedTests++ mu.Unlock() - // Only fail test if both devices are activated with max_users > 0 - srcReady := srcDevice.Status == serviceability.DeviceStatusActivated && srcDevice.MaxUsers > 0 - dstReady := dstDevice.Status == serviceability.DeviceStatusActivated && dstDevice.MaxUsers > 0 - if srcReady && dstReady { + // Only fail test if both devices can accept users + if srcDevice.Ready() && dstDevice.Ready() { t.Logf("DEVICE FAILURE: connectivity test failed from %s to %s (device %s -> %s): %v", src.Host, target.Host, srcDevice.Code, dstDevice.Code, err) } else {