cockroachdb / cockroachdb/cockroach

roachtest/testselector: add duration-aware selection heuristics

Open
#171,737 4 comments 0 reactions 0 assignees View on GitHub
A-testeng-foundations C-enhancement T-testeng
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

## Summary

Add duration-aware heuristics to the test selector so we can systematically cap or balance test-suite wall time, instead of treating duration as a passive output field.

## Motivation

#171671 documents that the nightly roachtest GCE suite consistently times out. That issue identifies multiple contributors (#170652 benchmark retries, GCE quota, suite growth) and explicitly suggests duration-aware selection in several places:

- Under "Stagger CI Start Times": "the test selection heuristic could be modified to aim for a 12 hour execution time"
- Under "Schedules": explicit reference to bin-packing as a longer-term solution
- Under "Benchmark Quality": prioritization as a way to deprioritize less important benchmarks

The test selection migration in [#169712](https://github.com/cockroachdb/cockroach/issues/169712) can contribute to duration-aware selection. Selection would run in a Go function we can change, against a CRDB cluster that already records per-run duration. Specifically:

- `roachtest_runs.duration_ms` records per-run duration
- `aggregate.totalDuration` / `totalSuccessful` are computed in
`aggregateHistory`
- `TestDetails.AvgDurationInMillis` is already surfaced downstream

## Proposed approach

Add a post-`Categorize` pass that demotes selected tests whose avg duration exceeds N minutes — but **only when they were selected for a discretionary reason**. The cap explicitly does NOT override the coverage-floor rules:

| Reason | Cap overrides? | Why |
|--------|----------------|-----|
| RecentFailure | No | Broken tests must run |
| Preempt | No | Same |
| UnknownNotFromSelector | No | Investigate weirdness |
| **Stale** | No | Coverage floor — every test runs at least every LastRunOn days |
| NewTest | Yes | Newness is nice-to-have, not essential |

This is "prefer cheap tests when there's slack," not "blacklist slow tests." A test that consistently exceeds the cap still runs every `LastRunOn` days via the Stale rule, so coverage is preserved — just at a lower cadence than fast tests. Selector-skip rows are recorded as UNKNOWN, which `aggregateHistory` excludes from `lastRun`, so the Stale clock correctly ticks from the test's last *real* run.

Surface a metric for tests demoted by the cap so we can tune N and observe the suite's slow-test distribution.

## What the cap intentionally doesn't account for

The cap operates on raw avg duration in isolation — a single per-test check. That's deliberate (start small), but it means several real-world factors that affect "how expensive is this test, really" are ignored. These would all be potential additions when considering an ideal test selection heuristic:

| Factor | Why a raw-duration cap misses it |
| -------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| **Effective parallelism** | Wall time ≈ compute time ÷ N. A 60-min test on a high-parallelism worker pool barely registers in wall time; on a starved pool it blocks everything. The cap can't tell which is which. |
| **Node count × spec** | A 60-min test on 1 small node ≠ a 60-min test on 12 big nodes. The cap measures runtime, but the budget actually consumed is `duration × nodes × spec`. Weighting by node count would be more honest. |
| **Cluster reuse** | Tests with `ReusePolicy != None` skip the spin-up cost. A 30-min slow-but-reusable test is cheaper overall than a 20-min fresh-cluster test. The cap doesn't differentiate. |
| **Setup / teardown overhead** | A 10-min test really costs ~15 min in VM time once you include cluster spin-up + teardown. The cap on raw runtime undercounts short tests' true cost. |

The cap accepts these inaccuracies for simplicity. A future iteration could weight by node count, segregate by VM spec, or migrate the budget logic to the runner (where parallelism and quota are observable). That's out of scope here — see Non-goals.

## Risk

A poorly-tuned cap could degrade selection quality *below* the duration-blind baseline:

- N too low → too many demotions → coverage gaps the Stale rule has to spend its slots fighting against (slow tests crowd out new tests on the days they're forced to run)
- N too high → ineffective, adds complexity for no benefit

Mitigations:

- Ship behind a feature flag so we can disable cleanly if telemetry shows it making things worse
- Start with a conservative N (high enough that few tests are demoted on day 1)
- Monitor the demotion-rate metric to detect over-aggressive caps

## Non-goals

- **Fixing the immediate timeout outage.** The dominant contributor per #171671 is the benchmark-retry mechanism (#170652) plus GCE quota. This work is structural mitigation, not the answer to today's overruns.
- **Scheduling / runner orchestration.** Selection picks *what* to run; scheduling picks *when/where*. The factors above (parallelism, cluster reuse, packing) properly belong to the runner. Don't conflate.
- **VM/quota management.** The bigger root cause per #171671; this issue is one of several mitigations, not a replacement.

## Open questions

- **Which statistic does the cap read?** Avg is simple but fragile to outliers (one 90-min run on a normally-5-min test skews everything). Options: avg, median, P95, trimmed mean. Each has opposite failure modes — choosing the wrong direction could either over- or under-demote.
- **Default cap value (N).** Conservative starting point. Possibilities: fixed minutes, derived from suite percentile, per-suite tunable.
- **Slow-by-design escape hatch.** Some tests are meant to be long (compat matrices, end-to-end scenarios). Options: explicit allowlist, a `TestSpec.IgnoreDurationCap` flag, or suite-scoped cap that exempts certain suites.

## Related

- #171671 — broader CI overrun problem
- #169712 — migration of test selection from Snowflake to CRDB (provides the duration data this cap needs)

Epic: none

Jira issue: CRDB-64872

Contributor guide

Open the contributing guide

Research direction

Start at the Go test-selector pass after Categorize, then trace aggregateHistory and the existing duration fields: roachtest_runs.duration_ms, aggregate.totalDuration, aggregate.totalSuccessful, and TestDetails.AvgDurationInMillis. Resolve the statistic, default cap, escape hatch, and feature-flag choices; done means discretionary tests can be demoted without overriding RecentFailure, Preempt, UnknownNotFromSelector, or Stale, with a demotion metric exposed.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
testing-qa, tooling
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.