cockroachdb / cockroachdb/cockroach

testcluster: default to queues and other moving parts disabled

Open
#107,528 1 comment 0 reactions 0 assignees View on GitHub
A-testeng-foundations C-enhancement db-cy-23 quality-friday T-testeng
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Is your feature request related to a problem? Please describe.**

I've been spending a lot of time on test flakes lately, and as is well known, we rely (some would say over-rely) on TestCluster tests fairly heavily to test very precise interactions, often in the transaction and lease protocols.

TestCluster has many moving parts and this brings an amount of nondeterminism that produces an static of test flakes. These are often not very interesting (the test expects the lease to be in one place, but the store rebalancer may move it, oops) and they are not very frequent for any individual test. However, they are very annoying to reproduce and hard to sort out, and since they affect many tests, they amount to a tax we are paying without anything too concrete in return.

Generally speaking, our testing should be more deterministic[^1] when we're in the context of a unit test, and less deterministic when we're end-to-end testing (i.e. roachtest). This issue is about the former.

Let's default to turning off all moving parts in TestCluster that aren't necessary for the correct functionality of a purely in-memory cluster.

Today, TestCluster has two "replication" modes:

- ReplicationAuto
- ReplicationManual

We should evolve this into a general "Mode" follows:

- StaticWithUpreplication // the default, waits for upreplication but then turns off all moving parts
- Static // no moving parts, no initial upreplication
- DynamicWithUpreplication // today's ReplicationAuto: wait for upreplication, leave moving parts on
- Dynamic // moving parts, no initial upreplication

For example, `StartTestCluster` in mode `ReplicationAuto` should turn the replicate queue off after it has waited for full replication. Similarly, we want to disable the other queues (+store rebalancer), perhaps with the exception of the Raft snapshot queue, though that one too should be disabled once we are convinced there aren't any more errant raft snapshots[^2].

To migrate over, we'd replace the semantics of `ReplicationAuto` with that of `StaticAfterUpreplication` and see which tests fail, migrating them into one of the other options as appropriate. We'd do something similar with all explicit uses of `ReplicationManual`.

Something similar goes for the few direct users of `TestServer` (which should likely migrate to `TestCluster` instead).

[^2]: https://github.com/cockroachdb/cockroach/issues/87553

[^1]: https://github.com/cockroachdb/cockroach/issues/105177

**Describe the solution you'd like**

**Describe alternatives you've considered**

**Additional context**

Adding quality-friday in case someone wants to prototype this and get an idea of how many tests fail loudly with the new semantics.

Jira issue: CRDB-30097

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.