Skip to content

Commit 9e8032d

Browse files
tbgroachdev-claude
andcommitted
roachtest: split impact threshold between perturbation and recovery intervals
Some perturbations are expected to crush foreground throughput while they run, with the meaningful pass/fail signal being whether the cluster returns to baseline afterwards. Letting one threshold gate both intervals forces a choice between flaky-during-perturbation and permissive-during-recovery. Add recoveryImpact alongside impact. Zero value falls back to impact at the use site (no eager copy), so tests that don't set it independently continue to use a single threshold for both intervals. Perturbations that need a permissive in-flight threshold can set the two independently. Epic: none Release note: None Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
1 parent 9ec1266 commit 9e8032d

1 file changed

Lines changed: 27 additions & 8 deletions

File tree

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

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -97,13 +97,25 @@ type variations struct {
9797
perturbation perturbation
9898
workload workloadType
9999
impact acceptableImpact
100-
cloud registry.CloudSet
101-
acMode admissionControlMode
102-
diskBandwidthLimit string
103-
histogramArgs string // set at test start for workload histogram export
104-
profileOptions []roachtestutil.ProfileOptionFunc
105-
specOptions []spec.Option
106-
clusterSettings map[string]string
100+
// recoveryImpact is the threshold applied to the recovery interval
101+
// (after the perturbation completes). The zero value means "use the
102+
// same threshold as the perturbation interval (impact)". Reading at
103+
// the runTest use site (rather than eager-copying at setup time)
104+
// keeps the fallback automatic — perturbations that override impact
105+
// after setup() don't have to remember to update recoveryImpact too.
106+
//
107+
// Separate from impact because some perturbations (e.g. mass splits)
108+
// put the cluster under heavy short-term stress that the cluster is
109+
// expected to absorb; the meaningful signal is whether the cluster
110+
// returns to baseline afterwards.
111+
recoveryImpact acceptableImpact
112+
cloud registry.CloudSet
113+
acMode admissionControlMode
114+
diskBandwidthLimit string
115+
histogramArgs string // set at test start for workload histogram export
116+
profileOptions []roachtestutil.ProfileOptionFunc
117+
specOptions []spec.Option
118+
clusterSettings map[string]string
107119
}
108120

109121
const NUM_REGIONS = 3
@@ -928,7 +940,14 @@ func (v variations) runTest(ctx context.Context, t test.Test, c cluster.Cluster)
928940
t.L().Printf("validating stats during the perturbation")
929941
failures := isAcceptableChange(t.L(), baselineStats, perturbationStats, v.impact)
930942
t.L().Printf("validating stats after the perturbation")
931-
failures = append(failures, isAcceptableChange(t.L(), baselineStats, afterStats, v.impact)...)
943+
// recoveryImpact is the zero value by default; fall back to impact so
944+
// callers that don't care about separate thresholds get the obvious
945+
// behavior automatically.
946+
recovery := v.recoveryImpact
947+
if recovery == (acceptableImpact{}) {
948+
recovery = v.impact
949+
}
950+
failures = append(failures, isAcceptableChange(t.L(), baselineStats, afterStats, recovery)...)
932951
if len(failures) > 0 {
933952
// Collect perf artifacts before failing so that roachperf still gets
934953
// the data for this run.

0 commit comments

Comments
 (0)