Skip to content

Commit 96f8549

Browse files
committed
test(flowcontrol): address review feedback on benchmark suite
- Use the llm-d Authors copyright header on the new files (doc.go, detector_test.go) per review. - Fail fast if the PerformanceMatrix warmup request is not dispatched, matching the full-path harness, instead of silently timing a dead dispatch loop. - Fail fast if a matrix coordinate dispatches zero requests, so a wedged dispatch path produces a red build instead of plausible zero rows. - Drop the matrix harness TTL from 5m to 30s: nothing in a throughput benchmark legitimately waits minutes, and a tight TTL bounds a wedged coordinate to seconds of CI time instead of minutes per cell. - Document the liveness bias of the benchDetector's stuck-read reclaim heuristic: true queueing still reports saturated while releases flow; a stalled release transiently over-admits rather than latching. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
1 parent ef6b3bb commit 96f8549

4 files changed

Lines changed: 24 additions & 6 deletions

File tree

pkg/epp/flowcontrol/benchmark/benchmark_test.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,15 +72,17 @@ func BenchmarkFlowController_PerformanceMatrix(b *testing.B) {
7272
// runMatrixCoordinate executes a single coordinate of the performance hypercube.
7373
func runMatrixCoordinate(b *testing.B, m benchMatrix) {
7474
ctx, cancel := context.WithCancel(context.Background())
75+
defer cancel() // Ensure SUT goroutines are torn down even when the coordinate fails fatally.
7576

7677
fc, detector := setupBenchmarkHarness(ctx, b, m.priorities, m.limit, nil, nil)
7778

7879
// Warm up: block on one request until it's dispatched, proving the dispatch loop is live before
79-
// timing. Release the slot per the loop's immediate-drain protocol.
80+
// timing. Release the slot per the loop's immediate-drain protocol (a no-op under free-flow).
8081
warmup := &benchRequest{key: flowcontrol.FlowKey{ID: "warmup", Priority: 0}, byteSize: 1024}
81-
if outcome, _ := fc.EnqueueAndWait(ctx, warmup); outcome == types.QueueOutcomeDispatched && m.limit > 0 {
82-
detector.Release()
82+
if outcome, err := fc.EnqueueAndWait(ctx, warmup); outcome != types.QueueOutcomeDispatched {
83+
b.Fatalf("warmup request was not dispatched: outcome=%v, err=%v", outcome, err)
8384
}
85+
detector.Release()
8486

8587
reqs := make([]*benchRequest, m.flows)
8688
for i := 0; i < int(m.flows); i++ {
@@ -166,6 +168,13 @@ func runMatrixCoordinate(b *testing.B, m benchMatrix) {
166168
elapsed := b.Elapsed().Seconds()
167169
telemetry.report(b, elapsed)
168170

171+
// Every coordinate expects flow (W>L or free-flow), so zero dispatches means the dispatch path
172+
// wedged mid-run: fail loudly instead of publishing plausible-looking zero rows. Assert on the
173+
// benchmark goroutine (never inside RunParallel, where FailNow is invalid).
174+
if telemetry.dispatchCount.Load() == 0 {
175+
b.Fatalf("coordinate %s dispatched zero requests; the dispatch path is wedged", m.name())
176+
}
177+
169178
cancel() // Graceful teardown to prevent async skewing of subsequent coordinates.
170179
time.Sleep(50 * time.Millisecond) // Wait for SUT background goroutines to terminate.
171180
}

pkg/epp/flowcontrol/benchmark/detector_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright 2026 The Kubernetes Authors.
2+
Copyright 2026 The llm-d Authors.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.

pkg/epp/flowcontrol/benchmark/doc.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright 2026 The Kubernetes Authors.
2+
Copyright 2026 The llm-d Authors.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.

pkg/epp/flowcontrol/benchmark/helpers_test.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,12 @@ type benchDetector struct {
119119
// stuckReadThreshold is the number of consecutive saturated reads with zero intervening releases
120120
// after which the detector treats the outstanding grants as leaked idle permits and reclaims one.
121121
// >1 so that a single slow Release under load does not cause a spurious reclaim.
122+
//
123+
// The heuristic is deliberately biased toward liveness: if a genuine holder's Release stalls past
124+
// the threshold (e.g. its worker goroutine is descheduled), the reclaim transiently over-admits
125+
// beyond L rather than risking a latched-saturated deadlock. For a CPU-cost benchmark that is the
126+
// right trade: true queueing still reports saturated (1.0) while releases flow, and a slightly
127+
// loose L only shifts the coordinate's operating point, whereas a latch would hang the run.
122128
const stuckReadThreshold = 2
123129

124130
// Release frees the permit held by the dispatch that just completed.
@@ -284,7 +290,10 @@ func setupBenchmarkHarness(
284290
cfg := customCfg
285291
if cfg == nil {
286292
cfg = &controller.Config{
287-
DefaultRequestTTL: 5 * time.Minute,
293+
// Nothing in a throughput benchmark legitimately waits minutes: the worst observed
294+
// steady-state queue wait is ~10s (L=1, W=5000). A tight TTL bounds the cost of a wedged
295+
// coordinate to seconds of CI time instead of minutes per cell.
296+
DefaultRequestTTL: 30 * time.Second,
288297
ExpiryCleanupInterval: 1 * time.Hour, // Effectively disabled
289298
EnqueueChannelBufferSize: 2000,
290299
}

0 commit comments

Comments
 (0)