Skip to content

Commit ef76e92

Browse files
tbgroachdev-claude
andcommitted
roachtest: add per-test token-return wait override for perturbation tests
Some perturbations leave non-trivial kvadmission flow-control churn at the end of the workload phase that takes longer than the 10-minute default to fully drain to zero in the post-test ValidateTokensReturned check. A previous workaround (raise to 1h when v.diskBandwidthLimit is set) covers one such case. Generalize that to a per-test v.tokenReturnTime field that any perturbation can set; the existing defaults remain in place when the field is zero. No behavior change for existing tests. Epic: none Release note: None Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
1 parent 9e8032d commit ef76e92

1 file changed

Lines changed: 25 additions & 14 deletions

File tree

pkg/cmd/roachtest/tests/perturbation/framework.go

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -88,15 +88,22 @@ type variations struct {
8888
splits int
8989
scatter bool // pass --scatter when initializing the workload table
9090
timeout time.Duration
91-
numNodes int
92-
numWorkloadNodes int
93-
vcpu int
94-
disks int
95-
mem spec.MemPerCPU
96-
leaseType registry.LeaseType
97-
perturbation perturbation
98-
workload workloadType
99-
impact acceptableImpact
91+
// tokenReturnTime is the budget the post-test ValidateTokensReturned
92+
// check is given to observe all kvadmission flow-control tokens
93+
// returned. Zero means use the framework default (10m, 1h if a disk
94+
// bandwidth limit is set). Tests that drive large send queues or hold
95+
// many tokens at the end of the perturbation should bump this so the
96+
// check doesn't trip while drain finishes.
97+
tokenReturnTime time.Duration
98+
numNodes int
99+
numWorkloadNodes int
100+
vcpu int
101+
disks int
102+
mem spec.MemPerCPU
103+
leaseType registry.LeaseType
104+
perturbation perturbation
105+
workload workloadType
106+
impact acceptableImpact
100107
// recoveryImpact is the threshold applied to the recovery interval
101108
// (after the perturbation completes). The zero value means "use the
102109
// same threshold as the perturbation interval (impact)". Reading at
@@ -954,12 +961,16 @@ func (v variations) runTest(ctx context.Context, t test.Test, c cluster.Cluster)
954961
artifactsutil.CollectPerfArtifacts(ctx, t, c)
955962
require.Fail(t, strings.Join(failures, "\n"))
956963
}
964+
// Per-test override wins; otherwise fall back to the previous default
965+
// (10m, lifted to 1h when a disk bandwidth limit is set per #137017).
957966
// TODO(baptist): Look at the time for token return in actual tests to
958-
// determine if this can be lowered further.
959-
tokenReturnTime := 10 * time.Minute
960-
// TODO(#137017): Increase the return time if disk bandwidth limit is set.
961-
if v.diskBandwidthLimit != "0" {
962-
tokenReturnTime = 1 * time.Hour
967+
// determine if the default can be lowered further.
968+
tokenReturnTime := v.tokenReturnTime
969+
if tokenReturnTime == 0 {
970+
tokenReturnTime = 10 * time.Minute
971+
if v.diskBandwidthLimit != "0" {
972+
tokenReturnTime = 1 * time.Hour
973+
}
963974
}
964975
roachtestutil.ValidateTokensReturned(ctx, t, v, v.stableNodes(), tokenReturnTime)
965976
}

0 commit comments

Comments
 (0)