kv: don't allow lease transfers to replicas in need of snapshot

[nvanbenschoten]

Fixes #81763.
Fixes #79385.
Part of #81561.

Background

When performing a lease transfer, the outgoing leaseholder revokes its lease before proposing the lease transfer request, meaning that it promises to stop using the previous lease to serve reads or writes. The lease transfer request is then proposed and committed to the Raft log, at which point the new lease officially becomes active. However, this new lease is not usable until the incoming leaseholder applies the Raft entry that contains the lease transfer and notices that it is now the leaseholder for the range.

The effect of this handoff is that there exists a "power vacuum" time period when the outgoing leaseholder has revoked its previous lease but the incoming leaseholder has not yet applied its new lease. During this time period, a range is effectively unavailable for strong reads and writes, because no replica will act as the leaseholder. Instead, requests that require the lease will be redirected back and forth between the outgoing leaseholder and the incoming leaseholder (the client backs off). To minimize the disruption caused by lease transfers, we need to minimize this time period.

We assume that if a lease transfer target is sufficiently caught up on its log such that it will be able to apply the lease transfer through log entry application then this unavailability window will be acceptable. This may be a faulty assumption in cases with severe replication lag, but we must balance any heuristics here that attempts to determine "too much lag" with the possibility of starvation of lease transfers under sustained write load and a resulting sustained replication lag. See #38065 and #42379, which removed such a heuristic. For now, we don't try to make such a determination.

Patch Details

However, with this change, we now draw a distinction between lease transfer targets that will be able to apply the lease transfer through log entry application and those that will require a Raft snapshot to catch up and apply the lease transfer. Raft snapshots are more expensive than Raft entry replication. They are also significantly more likely to be delayed due to queueing behind other snapshot traffic in the system. This potential for delay makes transferring a lease to a replica that needs a snapshot very risky, as doing so has the effect of inducing range unavailability until the snapshot completes, which could take seconds, minutes, or hours.

In the future, we will likely get better at prioritizing snapshots to improve the responsiveness of snapshots that are needed to recover availability. However, even in this world, it is not worth inducing unavailability that can only be recovered through a Raft snapshot. It is better to catch the desired lease target up on the log first and then initiate the lease transfer once its log is connected to the leader's. For this reason, unless we can guarantee that the lease transfer target does not need a Raft snapshot, we don't let it through.

This commit adds protection against such risky lease transfers at two levels. First, it includes hardened protection in the Replica proposal buffer, immediately before handing the lease transfer proposal off to etcd/raft. Second, it includes best-effort protection before a Replica begins to initiate a lease transfer in AdminTransferLease, which all lease transfer operations flow through.

The change includes protection at two levels because rejecting a lease transfer in the proposal buffer after we have revoked our current lease is more disruptive than doing so earlier, before we have revoked our current lease. Best-effort protection is also able to respond more gracefully to invalid targets (e.g. they pick the next best target).

However, the check in the Replica proposal buffer is the only place where the protection is airtight against race conditions because the check is performed:

1. by the current Raft leader, else the proposal will fail
2. while holding latches that prevent interleaving log truncation

Remaining Work

With this change, there is a single known race which can lead to an incoming leaseholder needing a snapshot. This is the case when a leader/leaseholder transfers the lease and then quickly loses Raft leadership to a peer that has a shorter log. Even if the older leader could have caught the incoming leaseholder up on its log, the new leader may not be able to because its log may not go back as far. Such a scenario has been possible since we stopped ensuring that all replicas have logs that start at the same index. For more details, see the discussion about #35701 in #81561.

This race is theoretical — we have not seen it in practice. It's not clear whether we will try to address it or rely on a mitigation like the one described in #81764 to limit its blast radius.

---

Release note (bug fix): Range lease transfers are no longer permitted to follower replicas that may require a Raft snapshot. This ensures that lease transfers are never delayed behind snapshots, which could previously create range unavailability until the snapshot completed. Lease transfers now err on the side of caution and are only allowed when the outgoing leaseholder can guarantee that the incoming leaseholder does not need a snapshot.

[cockroach-teamcity]

This change is 

[irfansharif]

Pretty great tests and pretty great commentary!

Reviewed 12 of 12 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @nvanbenschoten)

---

pkg/kv/kvserver/replica_proposal_buf.go line 558 at r4 (raw file):

// - RequestLease when the proposer is not the raft leader (with caveats).
// - TransferLease when the proposer cannot guarantee that the lease transfer
//   target does not currently need a Raft snapshot and therefore will not

[mega nit] The triple negative form makes this slightly hard to read.

---

pkg/kv/kvserver/client_replica_test.go line 2815 at r4 (raw file):

		transferErrC := make(chan error, 1)
		if rejectAfterRevoke {
			_ = tc.Stopper().RunAsyncTask(ctx, "transfer leas", func(ctx context.Context) {

s/leas/lease

[irfansharif]

Do we want to backport this to 22.1 (and maybe 21.2)?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @nvanbenschoten)

[nvanbenschoten]

TFTR!

Do we want to backport this to 22.1 (and maybe 21.2)?

Yes, I think we should. But I also think we'll want to be generous with the baking period. I'll let this sit on master for about a month before backporting. I'd be surprised if there wasn't some kind of fallout.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)

---

pkg/kv/kvserver/replica_proposal_buf.go line 558 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[mega nit] The triple negative form makes this slightly hard to read.

I hardly found that this was not difficult to confuse. Reduced to a double negative, which is now not non-trivial to read.

---

pkg/kv/kvserver/client_replica_test.go line 2815 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/leas/lease

Done.

[nvanbenschoten]

bors r+

🤞 the fallout is minimal.

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[ajwerner]

I'm interested in the backport here as well as any thoughts as to whether this is related to #82778. I'm adding the label so backboard reminds us.

[nvanbenschoten]

I'm adding the label so backboard reminds us.

Thanks. I didn't want to do this before the merge because the auto-backport will skew and fail. Now that this has merged, I'll add the other targets and use this comment to keep a list of the PRs that need to be backported together.

• kv: extract etcd/raft utilities into raftutil library #82797 (will skew with allocator: carve out a package boundary #79991)
• kv: clean up BatchRequest.IsLeaseRequest #82798
• kv: don't allow lease transfers to replicas in need of snapshot #82758
• kv: don't try to reject lease transfer when flushing proposal buffer #83520