kvserver: do not allow lease transfers to replicas in StateSnapshot

[aayushshah15]

In some roachtest failures(#61541, for example) we've seen disruptive
effects from a range's lease being cooperatively transferred to a
follower replica that was either uninitialized or in need of a snapshot.

As of the time of this change, our default cluster settings make it such
that a snapshot would take ~64 seconds to transfer. This means that, if
our lease rebalancing heuristics (via either the replicate queue or the
store rebalancer) happen to make a decision to transfer a range lease to
a lagging follower during an unfortunate window of time (i.e. when the
follower is in need of a snapshot), we would be wedging all traffic on
that range until this follower applies this snapshot and then applies
the lease.

This commit prevents this behavior by rejecting lease transfers to replicas
that the leader (usually the leaseholder) knows are in need of a snapshot.

Resolves #61604

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

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

---

pkg/kv/kvserver/batcheval/cmd_lease_transfer.go, line 55 at r2 (raw file):

}

var errTransferLeaseToStateSnapshot = errors.Errorf("cannot transfer lease to replica in need of a snapshot")

Does kvnemesis hit this under stress and with a high rate of rebalancing + lease transfers?

---

pkg/kv/kvserver/batcheval/cmd_lease_transfer.go, line 90 at r2 (raw file):

	}

	if raftProgressTracker := cArgs.EvalCtx.GetProgress(); raftProgressTracker != nil {

Let's give this a comment, explaining the purpose of this check and that it is best-effort and only performed if the leaseholder also happens to be the leader.

---

pkg/kv/kvserver/batcheval/eval_context.go, line 102 at r2 (raw file):

	GetRangeInfo(context.Context) roachpb.RangeInfo

	// GetProgress returns this replica's raft progress tracker.

GetProgress is pretty ambiguous. What "progress" are we talking about? How about GetRaftProgress?

---

pkg/kv/kvserver/batcheval/eval_context.go, line 102 at r2 (raw file):

	GetRangeInfo(context.Context) roachpb.RangeInfo

	// GetProgress returns this replica's raft progress tracker.

What do you think about exposing the entire RaftStatus() method here? It seems more generally useful and allows us to be more explicit about that fact that the check you are adding is best-effort and only run if the leaseholder happens to also be the leader.

[andreimatei]

looks good to me, but a test would be bomb :) Ideally a unit test for TransferLease but I don't know if that's doable without a full cluster.

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15 and @nvanbenschoten)

---

pkg/kv/kvserver/replica_eval_context_span.go, line 217 at r3 (raw file):

// GetProgress is part of the EvalContext interface.
func (rec SpanSetReplicaEvalContext) RaftStatus() *raft.Status {
return rec.i.RaftStatus()

I think this is missing a tab :P

---

pkg/kv/kvserver/batcheval/eval_context.go, line 102 at r3 (raw file):

	GetRangeInfo(context.Context) roachpb.RangeInfo

	// RaftStatus returns this replica's raft status tracker.

maybe add some comments here about what a leader sees vs what a follower sees?

[aayushshah15]

looks good to me, but a test would be bomb :)

The super aggressive log truncation is currently making the test flake, but I did add a test for it. Am I misunderstanding you?

[andreimatei]

I just missed it; reviewable fail.

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

---

pkg/kv/kvserver/client_lease_test.go, line 289 at r3 (raw file):

		ReplicationMode: base.ReplicationManual,
		ServerArgs: base.TestServerArgs{
			// Encourage very aggressive log truncations.

tell me why. Isn't this redundant with MustForceRaftLogScanAndProcess ?

---

pkg/kv/kvserver/client_lease_test.go, line 334 at r3 (raw file):

		progress := leaseholderStore.RaftStatus(scratchDesc.RangeID).Progress
		if progress == nil {
			return errors.Errorf("expected leaseholder to be collocated with the leader")

nit: these error don't tell me if they're expected until they eventually go away, or if they're never expected. If they are expected for a while, consider rephrasing as leaseholder not yet collocated with the leader.

[aayushshah15]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15, @andreimatei, and @nvanbenschoten)

---

pkg/kv/kvserver/client_lease_test.go, line 289 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

tell me why. Isn't this redundant with MustForceRaftLogScanAndProcess ?

You're right, this wasn't needed.

---

pkg/kv/kvserver/client_lease_test.go, line 334 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: these error don't tell me if they're expected until they eventually go away, or if they're never expected. If they are expected for a while, consider rephrasing as leaseholder not yet collocated with the leader.

Done.

---

pkg/kv/kvserver/replica_eval_context_span.go, line 217 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think this is missing a tab :P

Fixed.

---

pkg/kv/kvserver/batcheval/cmd_lease_transfer.go, line 90 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's give this a comment, explaining the purpose of this check and that it is best-effort and only performed if the leaseholder also happens to be the leader.

Done.

---

pkg/kv/kvserver/batcheval/eval_context.go, line 102 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

What do you think about exposing the entire RaftStatus() method here? It seems more generally useful and allows us to be more explicit about that fact that the check you are adding is best-effort and only run if the leaseholder happens to also be the leader.

Done.

---

pkg/kv/kvserver/batcheval/eval_context.go, line 102 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

maybe add some comments here about what a leader sees vs what a follower sees?

Done.

[aayushshah15]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15, @andreimatei, and @nvanbenschoten)

---

pkg/kv/kvserver/batcheval/cmd_lease_transfer.go, line 55 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Does kvnemesis hit this under stress and with a high rate of rebalancing + lease transfers?

I had a hard time hitting it with the 16MB COCKROACH_RAFT_LOG_TRUNCATION_THRESHOLD but with something like 1MB I can get kvnemesis to hit this reasonably quickly.

 error applying x.TransferLeaseOperation(ctx, /Table/50/"e5f12ee7", 7) // cannot transfer lease to replica in need of a snapshot

[nvanbenschoten]

Reviewed 1 of 4 files at r3, 9 of 9 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei and @nvanbenschoten)

[nvanbenschoten]

One change we could make to harden the protection here is to only allow a leaseholder to transfer its lease away if it is also the leader. However, that still is inherently racy because a leader doesn't immediately know that it needs to send a snapshot to a follower in need. So then we could change the condition from rejecting lease transfers to replicas in StateSnapshot to only allowing lease transfers to replicas in StateReplicate.

But we would need to be careful that doing so doesn't introduce an undesirable cyclical dependency between lease transfers and raft leaders. Those almost always have subtle and unexpected effects - just ask @andreimatei.

And this will all still be a little racy if we don't ensure lease transfer and log trunation are properly synchronized.

[nvanbenschoten]

So then we could change the condition from rejecting lease transfers to replicas in StateSnapshot to only allowing lease transfers to replicas in StateReplicate.

This actually seems like a targeted improvement to this PR that incurs very little additional risk. What do you think Aayush?

[aayushshah15]

This actually seems like a targeted improvement to this PR that incurs very little additional risk. What do you think Aayush?

Makes sense, I made the change.

The test added in this PR ended up being a little flakey because the raft leader doesn't learn about the present state of a follower until it receives a subsequent MsgAppResp from it. I've updated the test to account for this.

[aayushshah15]

@nvanbenschoten, I'd like your take on the following:

I'm not super sure about the approach implemented by this PR anymore. It makes AdminRelocateRange flakey (and thus, its callers like EXPERIMENTAL_RELOCATE ... ) because AdminRelocateRange attempts to transfer the range lease right after a rebalance operation. Often, what ends up happening is that the leader has the lease target in StateSnapshot until it receives some kind of raft message response from this potential lease target (which might take up to a heartbeat interval unless theres traffic on the range). This makes AdminRelocateRange fail to transfer the lease (which can fail the entire thing if the lease transfer was required to remove the current leaseholder).

• We can simply add some kind of re-try logic here and that will "solve" this issue, but I'm not sure how we feel about that.

• Another possibility is that we don't reject anything at the command evaluation level but instead have the allocator / store-rebalancer be cognizant of not transferring leases to replicas that the leader knows are not in StateReplicate.

What do you think?

[nvanbenschoten]

Both of these options sound reasonable to me. I think it would help to spell out each of the places that attempt to transfer leases and discuss what will happen in each case if they run into this new error. Will some mechanism retry? How much work will be rolled back? Do we risk thrashing?

This will help us determine where the check and any retry loops should live. For instance, it may lead us to decide that we need a retry loop in AdminTransferLease and that all other initiators of lease transfers already properly handle this error.

[aayushshah15]

I think it would help to spell out each of the places that attempt to transfer
leases and discuss what will happen in each case if they run into this new
error.

There's three places where we initiate lease transfers:

1. via the replicateQueue
2. via the StoreRebalancer
3. via AdminRelocateRange (used by the merge queue, store-rebalancer, and by EXPERIMENTAL_RELOCATE)

If 1 & 2 run into this new error, they will log an error and move on. The
replicateQueue will re-consider the replica in the next scanner cycle, and the
StoreRebalancer might reconsider the replica after about a minute or so.

For (3), AdminRelocateRange will try to do lease transfers for 2 reasons: If
the current leaseholder needs to be removed or in order to move the lease to
the first voting replica supplied to AdminRelocateRange (this is part of its
contract). If it gets this error, it will return an error to its client without
rolling any of its completed relocations back.

So, I dont believe we're risking thrashing of any sort under any circumstance.
I do think adding a retry logic in AdminTransferLease or even in AdminRelocateRange
is a little fraught because it feels hard to reason about the consequences of adding
non-deterministic delays to something like this. Additionally, a raft heartbeat
interval is ~1s based on our defaults, which is quite long (and it doesn't account
for replication lag).

I'm going try a new patch that teaches the replicateQueue and StoreRebalancer
to not initiate lease transfers to replicas in need of snapshots. It should be
a fair bit clearer and a much more reasonable backport candidate.

[aayushshah15]

Closing in favor of #69696