cockroachdb / cockroachdb/cockroach

kvserver: allow reliably pinning and expiring leases in tests

Open
#107,524 1 comment 0 reactions 0 assignees View on GitHub
A-testing C-enhancement quality-friday T-kv
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

In many tests[^a][^d] we "just" want the lease for range X to be on store Y and to _stay there_ even when the test is being stressed under nightly and perhaps on an overloaded VM. There are a number of factors that can get in the way of this humble goal.

- ability to acquire lease is tied to raft leadership[^1][^2]
- leases are expiration-based (either directly or through liveness)

The second point is a major source of flakiness, and a pretty annoying one at that: pretty rare, but affects lots of tests, and no satisfying solutions (skip test under stress? Mess with timeouts?).

The reason our leases have an expiration is because we want the ability to fail over in case of an unexpected node crash. But in our TestCluster integration tests, this is not a concern and we should be able to pin leases by essentially giving them an infinite lifetime. (Such leases can still be shortened later). Liveness makes this a little more annoying, but we could likely just use "vanilla" expiration-based leases by default, which may be the direction CRDB as a whole is heading as well[^3]

So one concrete suggestion would be for `(TestCluster).TransferLease` to default to giving out long-lived expiration-based leases (you'd have to opt into shorter leases with a config option and tests would fail of nodes in the cluster got shut down without this option). Additionally, automatic lease transfers (replicate queue / store rebalancer) should be off by default. This can be seen as a refinement of the `ReplicationMode`. Note that there is evidence that a number of tests are "accidentally" using the default `ReplicationAuto` which also allows for rebalances and lease transfers - this is often explicitly counter to what the test wants[^q]

[^q]: https://github.com/cockroachdb/cockroach/issues/101824#issuecomment-1649893853

Expiring leases is another use case. The best strategy currently used is to set up the cluster with a hybrid manual clock (which ticks like a clock but can also be accelerated by a delta) and to go through some arcane incantations to forward the clock by the right amount, together with a retry loop just in case a lease acquisition is racing with the clock bump[^b]. This could be made easier, too, so that we can "just" call `tc.ExpireLeases()` or something like that[^c], and this should work by default.

Note: I see that we already have some way to "pin leases": https://github.com/cockroachdb/cockroach/blob/66c9f93ae86bddd7ba4c5f6a6b8b6cb700ca23ce/pkg/kv/kvserver/testing_knobs.go#L527-L551

I assume it works by restricting which raft instance can win elections, and which replica can become leader, by suppressing any message that could lead to an unwanted outcome. Seems like an involved hammer but maybe it can be adopted more, too, and improved.

[^1]: see https://github.com/cockroachdb/cockroach/issues/107523
[^2]: I think calling `TransferLease` takes care of this though, so maybe not an actual issue
[^3]: https://github.com/cockroachdb/cockroach/issues/94592

[^a]: https://github.com/cockroachdb/cockroach/issues/107200#issuecomment-1647808624
[^b]: https://github.com/cockroachdb/cockroach/pull/107442
[^c]: see https://github.com/cockroachdb/cockroach/blob/fd6f2830764bf6ca0c94f849ad2aa9cca10fd38b/pkg/testutils/testcluster/testcluster.go#L1099-L1106 for something ~related that we could build out
[^d]: see https://github.com/cockroachdb/cockroach/issues/101824#issuecomment-1614777449

Jira issue: CRDB-30095

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.