storage: remove the replica up to date check when transfering lease

[darinpp]

Previously, we had a check that filtered out all replicas that
are lagging behind the leader in case of a lease transfer.

We remove that check so in case of lease preference for
a node that is constantly lagging - the lease transfer can occur without delay.

This removes the check that the candidates for lease transfer are only
replicas that aren't lagging behind. etcd implements the
3.10 Leadership transfer extension where the old leader will
bring up to date the new leader's log while blocking any new requests.

Release note (bug fix): now possible to transfer range leases to lagging replicas

[cockroach-teamcity]

This change is 

[tbg]

I agree that this particular check is misguided, but there'll probably be some way in which we're shooting ourselves in the foot if we just remove it. For example, I'm not seeing any code in TransferLeaseTarget that would stop it from transferring to a node that just went down, so the transferred lease will time out, someone else will get the lease, attempt to transfer again, time out, ... until the store dead timeout has passed. I think we at least want to check the follower's liveness. There is some kind of check here:

cockroach/pkg/storage/allocator.go

Line 852 in 0cf7360

existing, _ = a.storePool.liveAndDeadReplicas(rangeID, existing)

but note that it's never reached if we have lease preferences, and it uses the 5min timeout.

store.cfg.NodeLiveness.GetLivenessStatusMap() (or the per-node .GetLiveness(nodeID).IsLive seem like a better signal.

So you could replace filterBehindReplicas with a new onlyLiveReplicas(...), and perhaps also remove some of the inefficient liveness checking in TransferLeaseTarget (though all callers would have to prune, maybe it's better to push the liveness map into TransferLeaseTarget instead?)

[darinpp]

This is already the case. The first thing that TransferLeaseTarget does is call StorePool.getStoreList. It grabs the NodeLiveness record for each node and calls Liveness.LivenessStatus and in turn Liveness.IsLive.

[tbg]

As an end-to-end test, we should do something like #38065: set up a cluster in which latencies are nontrivial, start with lease preferences in region 1 on a table that is seeing steady write traffic, then flip the lease preferences over to a faraway region and make sure that the leases transfer reasonably quickly (and stay where they ought to be). One optional test would be to kill one of the new preferred leaseholders just before you update the leaseholder preference, and make sure that the outage isn't >10s.

Writing this test will be annoying. For one, roachtests don't really hard-code the geography, but you want to know the latencies between them because to get a repro, you want to move the new lease preference as far away from the old one as possible. It also almost seems like you should be able to use ./cockroach demo --global, but this isn't quite there and there's no precedent for writing complex logic against it. You could also look into doing this via shakespeare which would actually be a pretty good fit, but you would have to get some @knz time to get started, and we don't run shakespeare in nightlies yet. Another problem that will have to be solved either way is that - I think - we don't actually have a way to query whether the leaseholder preference is met (we can query crdb_internal.ranges, but this isn't how we want to do it). @andreimatei, are leaseholder prefs in the constraint violation report? I think I looked and found that the answer was no (or the lease transferred faster than I could check).

[ajwerner]

I wonder if there’s a way to utilize the new latency stuff we added for cockroach demo and then write the test as an augmentation of partitionccl/TestRepartitioning by putting some load into the test on a cluster which was set up with high latency. The first commit of #41240 adds leaseholder preferences to that test. The test rather crudely checks for leaseholder placement by examining traces for keys. That may be an easier path to a reliable reproduction cycle than relying on roachtests or hooking up Shakespeare.

[tbg]

Using the latency injection in unit tests seems like a good idea in general, if we can make that work I'd greatly prefer it.

[darinpp]

I added a test as well. Also verified that the test fails with the old check.

[tbg]

, thanks!

It looks like the test is flaky:

Maybe stress pull request] --- FAIL: TestTransferLeaseToLaggingNode (56.02s)
[05:41:21]
[Maybe stress pull request]     replicate_queue_test.go:604: condition failed to evaluate within 45s: waiting for n1 to get the lease
[05:41:21]
[Maybe stress pull request]         goroutine 25 [running]:
[05:41:21]
[Maybe stress pull request]         runtime/debug.Stack(0xc0003161c0, 0x4562000, 0xc00046aa60)
[05:41:21]
[Maybe stress pull request]         	/usr/local/go/src/runtime/debug/stack.go:24 +0x9d
[05:41:21]
[Maybe stress pull request]         github.com/cockroachdb/cockroach/pkg/testutils.SucceedsSoon(0x4660a00, 0xc0001ca800, 0xc0003161c0)
[05:41:21]
[Maybe stress pull request]         	/go/src/github.com/cockroachdb/cockroach/pkg/testutils/soon.go:37 +0x6b
[05:41:21]
[Maybe stress pull request]         github.com/cockroachdb/cockroach/pkg/storage_test.TestTransferLeaseToLaggingNode(0xc0001ca800)
[05:41:21]
[Maybe stress pull request]         	/go/src/github.com/cockroachdb/cockroach/pkg/storage/replicate_queue_test.go:604 +0x905
[05:41:21]
[Maybe stress pull request]         testing.tRunner(0xc0001ca800, 0x3d9e638)
[05:41:21]
[Maybe stress pull request]         	/usr/local/go/src/testing/testing.go:865 +0xc0
[05:41:21]
[Maybe stress pull request]         created by testing.(*T).Run
[05:41:21]
[Maybe stress pull request]         	/usr/local/go/src/testing/testing.go:916 +0x35a
[05:41:21]
[Maybe stress pull request] FAIL

https://teamcity.cockroachdb.com/viewLog.html?buildId=1597934&buildTypeId=Cockroach_UnitTests_Test&tab=buildLog&_focus=689#_state=689

Make sure it survives stress{,rac}ing for a few minutes on your GCEworker (with STRESSFLAGS='-p 8' or something like that).

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @darinpp)

---

pkg/storage/replicate_queue.go, line 940 at r1 (raw file):

Previously, darinpp wrote…

This is already the case. The first thing that TransferLeaseTarget does is call StorePool.getStoreList. It grabs the NodeLiveness record for each node and calls Liveness.LivenessStatus and in turn Liveness.IsLive.

👍

---

pkg/storage/replicate_queue_test.go, line 567 at r2 (raw file):

	s := sqlutils.MakeSQLRunner(tc.Conns[0])

	// Create a table and set the replication factor to 3

.

---

pkg/storage/replicate_queue_test.go, line 579 at r2 (raw file):

	}

	// Get the table's range

.

---

pkg/storage/replicate_queue_test.go, line 618 at r2 (raw file):

		rangeDesc := n1Repl.Desc()
		for _, r := range rangeDesc.Replicas().Voters() {
			// We are interested in the replica on the 3rd node

.

[darinpp]

bors r+

[andreimatei]

If bors fails, please put a Fixes #38065 line in the commit message.

Also may I suggest a more user-understandable release note like:
Release note(bug fix): A bug causing a range's lease to sometimes not be transferred to the location indicated in lease_preferences in geographically-distributed deployments has been fixed.

[craig]

Build succeeded

• GitHub CI (Cockroach)