kvserver: don't refuse to fwd lease proposals in some edge cases #58722

andreimatei · 2021-01-11T20:54:58Z

This patch backpedals a little bit on the logic introduced in #55148.
That patch said that, if a leader is known, every other replica refuses
to propose a lease acquisition. Instead, the replica in question
redirects whomever was triggering the lease acquisition to the leader,
thinking that the leader should take the lease.
That patch introduced a deadlock: some replicas refuse to take the lease
because they are not VOTER_FULL (see CheckCanReceiveLease()). To fix the
deadlock, this patch incorporates that check in the proposal buffer's
decision about whether or not to reject a proposal: if the leader is
believed to refuse to take the lease, then we again forward our own
lease request.

An edge case to the edge case is when the leader is not even part of the
proposer's range descriptor. This can happen if the proposer is far
behind. In this case, we assume that the leader is eligible. If it
isn't, the deadlock will resolve once the proposer catches up.

A future patch will relax the conditions under which a replica agrees to
take the lease. VOTER_INCOMING replicas should take the lease.
VOTER_DEMOTING are more controversial.

Fixes #57798

Release note: None

cockroach-teamcity · 2021-01-11T20:55:08Z

This change is

nvanbenschoten

Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 5 of 5 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, and @tbg)

pkg/kv/kvserver/replica_proposal_buf.go, line 163 at r3 (raw file):

	leaderKnown bool

	leader roachpb.ReplicaID

Might as well give this a comment, mentioning the value of this field when !leaderKnown.

pkg/kv/kvserver/replica_proposal_buf.go, line 168 at r3 (raw file):

	// leaderIneligibleForLease is set if the leader is known, but its type of
	// replica prevents it from acquiring a lease.
	leaderIneligibleForLease bool

nit: Invert this condition to avoid the double-negative down below.

pkg/kv/kvserver/replica_proposal_buf.go, line 178 at r3 (raw file):

	replicaID() roachpb.ReplicaID
	leaderStatusRLocked(raftGroup proposerRaft) leaderInfo

stray line?

pkg/kv/kvserver/replica_proposal_buf.go, line 446 at r3 (raw file):

	buf := b.arr.asSlice()[:used]
	ents := make([]raftpb.Entry, 0, used)
	var leaderInfo leaderInfo

nit: move this down to where the other variable declarations use to live. No need to mix it up with these slices of entries.

pkg/kv/kvserver/replica_proposal_buf.go, line 501 at r3 (raw file):

		// A special case is when the leader is known, but is ineligible to get the
		// lease. In that case, we have no choice but to continue with the proposal.
		if !leaderInfo.iAmTheLeader && p.Request.IsLeaseRequest() {

We may need to extend this check even more. In 91f58af, I noticed that we are rejecting lease extensions when we are the current leaseholder if we haven't yet grabbed raft leadership. That seems wrong.

No need to do anything here.

pkg/kv/kvserver/replica_proposal_buf.go, line 503 at r3 (raw file):

		if !leaderInfo.iAmTheLeader && p.Request.IsLeaseRequest() {
			proposeAnyway := leaderInfo.leaderIneligibleForLease || b.testing.allowLeaseProposalWhenNotLeader
			if leaderInfo.leaderKnown && !proposeAnyway {

This grouping seems odd to me. We're saying leader known && (either leader eligible || not testing allow when not leader). So if leaderKnown && !leaderIneligibleForLease, aren't we rejecting, regardless of the testing knob? I don't think that's the intention. I think we want to group the first two conditions together like:

leaderKnownAndEligible := ...
if leaderKnownAndEligible && !b.testing.allowLeaseProposalWhenNotLeader {
    ...
}

pkg/kv/kvserver/replica_proposal_buf.go, line 764 at r3 (raw file):

}

func (rp *replicaProposer) leaderStatusRLocked(raftGroup proposerRaft) leaderInfo {

Keep this in order with the interface. Either pull this all the way up above destroyed() (but don't do that) or shuffle the interface def. Ideally, this would go right above rejectProposalWithRedirectLocked in the declaration and the definition.

pkg/kv/kvserver/replica_proposal_buf.go, line 775 at r3 (raw file):

	if leaderKnown {
		// Figure out if the leader is eligible for getting a lease. Only VOTER_FULL
		// replicas are eligible.

This seems like the kind of comment that is going to rot. How should we avoid that?

pkg/kv/kvserver/replica_proposal_buf.go, line 785 at r3 (raw file):

			leaderIneligibleForLease = false
		} else {
			err := batcheval.CheckCanReceiveLease(leaderRep, *rangeDesc)

nit: Seems like we should just pass the descriptor by pointer, since that's what every caller wants and that's generally how we interact with RangeDescriptors.

pkg/kv/kvserver/replica_proposal_buf_test.go, line 59 at r3 (raw file):

}

func (t *testProposer) leaderStatusRLocked(raftGroup proposerRaft) leaderInfo {

Why define this method in such a random place? I know these kinds of comments seem like nits, but a few of these changes in a row and the entire structure of a piece of code falls apart and we're left with chaos! And then a month later, we'll decide that we need to rewrite everything because the code is jumbled and hard to read. And then we introduce a bug during the rewrite. And then data gets corrupted in an important deployment. And then a SpaceX satellite falls out of the sky. And then SpaceX goes bankrupt due to a lawsuit. And we never make it to Mars. Don't you want to go to Mars??

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

// If we decide to start using long-lived learners at some point, that math may change.

If we decide?

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

// point, that math may change.
func checkCanReceiveLease(
	newLeaseholer roachpb.ReplicaDescriptor, rngDesc roachpb.RangeDescriptor,

Should rngDesc be a pointer?

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

// out the edge cases. If we decide to start using long-lived learners at some
// point, that math may change.
func CheckCanReceiveLease(

Want to add a unit test around this here in pkg/kv/kvserver/batcheval/cmd_lease.go?

andreimatei

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, @nvanbenschoten, and @tbg)

pkg/kv/kvserver/replica_proposal_buf.go, line 163 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Might as well give this a comment, mentioning the value of this field when !leaderKnown.

done

pkg/kv/kvserver/replica_proposal_buf.go, line 178 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

stray line?

done

pkg/kv/kvserver/replica_proposal_buf.go, line 446 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: move this down to where the other variable declarations use to live. No need to mix it up with these slices of entries.

done

pkg/kv/kvserver/replica_proposal_buf.go, line 503 at r3 (raw file):

So if leaderKnown && !leaderIneligibleForLease, aren't we rejecting, regardless of the testing knob? I don't think that's the intention.

This was, in fact, the intention. The testing knob is kinda crap, and is only used by one rando. I wanted to keep the scope of the knob as small as possible - i.e. only apply it in the most regular situation (when the leader is eligible). In particular, I don't want the knob to introduce the possibility of the deadlock.
WDYT

pkg/kv/kvserver/replica_proposal_buf.go, line 764 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Keep this in order with the interface. Either pull this all the way up above destroyed() (but don't do that) or shuffle the interface def. Ideally, this would go right above rejectProposalWithRedirectLocked in the declaration and the definition.

done

pkg/kv/kvserver/replica_proposal_buf.go, line 775 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This seems like the kind of comment that is going to rot. How should we avoid that?

I've deleted it :P

pkg/kv/kvserver/replica_proposal_buf.go, line 785 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: Seems like we should just pass the descriptor by pointer, since that's what every caller wants and that's generally how we interact with RangeDescriptors.

yeah... I had made it a value for symmetry with the ReplicaDescriptor arg, where we do use values more. Changed.

pkg/kv/kvserver/replica_proposal_buf_test.go, line 59 at r3 (raw file):

Don't you want to go to Mars??

https://www.youtube.com/watch?v=KDXbl54HpQE

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

// If we decide to start using long-lived learners at some point, that math may change.

If we decide?

well I'm just moving a comment around

But yeah, I've deleted it. Cause we have decided, but the math is not changing (as far as learners go)

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should rngDesc be a pointer?

done

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Want to add a unit test around this here in pkg/kv/kvserver/batcheval/cmd_lease.go?

done

nvanbenschoten

though there are still a few minor tweaks to make in that inner condition to unravel it a little.

We should get more eyes on this too, since I assume we're planning for another backport attempt.

Reviewed 6 of 6 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, and @tbg)

pkg/kv/kvserver/replica_proposal_buf.go, line 168 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: Invert this condition to avoid the double-negative down below.

I still think this would be an improvement.

pkg/kv/kvserver/replica_proposal_buf.go, line 503 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

So if leaderKnown && !leaderIneligibleForLease, aren't we rejecting, regardless of the testing knob? I don't think that's the intention.

This was, in fact, the intention. The testing knob is kinda crap, and is only used by one rando. I wanted to keep the scope of the knob as small as possible - i.e. only apply it in the most regular situation (when the leader is eligible). In particular, I don't want the knob to introduce the possibility of the deadlock.
WDYT

I think this makes the knob super hard to reason about. I'd be in favor of broadening it to just disable this entire rejection mechanism. That's the effect it had before.

nvanbenschoten

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, and @tbg)

pkg/kv/kvserver/replica_proposal_buf_test.go, line 59 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Don't you want to go to Mars??

https://www.youtube.com/watch?v=KDXbl54HpQE

https://www.youtube.com/watch?v=qoHU57KtUws

This assertion had become nonsensical in #9915f0d0f104aa918c94340e9e47129b90421999. It's from a time where the surrounding function was dealing with the local store only. Release note: None

andreimatei

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, and @tbg)

pkg/kv/kvserver/replica_proposal_buf.go, line 168 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I still think this would be an improvement.

done

pkg/kv/kvserver/replica_proposal_buf.go, line 503 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think this makes the knob super hard to reason about. I'd be in favor of broadening it to just disable this entire rejection mechanism. That's the effect it had before.

ok, done

nvanbenschoten

Reviewed 6 of 6 files at r9.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell and @tbg)

pkg/kv/kvserver/replica_proposal_buf.go, line 501 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We may need to extend this check even more. In 91f58af, I noticed that we are rejecting lease extensions when we are the current leaseholder if we haven't yet grabbed raft leadership. That seems wrong.

No need to do anything here.

By the way, this is now tracked in #59179.

tbg

Reviewed 6 of 6 files at r7, 3 of 3 files at r8, 6 of 6 files at r9.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @bdarnell)

pkg/kv/kvserver/batcheval/cmd_lease.go, line 51 at r9 (raw file):

	wouldbeLeaseholer roachpb.ReplicaDescriptor, rngDesc *roachpb.RangeDescriptor,
) error {
	repDesc, ok := rngDesc.GetReplicaDescriptorByID(wouldbeLeaseholer.ReplicaID)

Leaseholder

Make checkCanReceiveLease() more nimble, anticipating broader use. Release note: None

This patch backpedals a little bit on the logic introduced in cockroachdb#55148. That patch said that, if a leader is known, every other replica refuses to propose a lease acquisition. Instead, the replica in question redirects whomever was triggering the lease acquisition to the leader, thinking that the leader should take the lease. That patch introduced a deadlock: some replicas refuse to take the lease because they are not VOTER_FULL (see CheckCanReceiveLease()). To fix the deadlock, this patch incorporates that check in the proposal buffer's decision about whether or not to reject a proposal: if the leader is believed to refuse to take the lease, then we again forward our own lease request. An edge case to the edge case is when the leader is not even part of the proposer's range descriptor. This can happen if the proposer is far behind. In this case, we assume that the leader is eligible. If it isn't, the deadlock will resolve once the proposer catches up. A future patch will relax the conditions under which a replica agrees to take the lease. VOTER_INCOMING replicas should take the lease. VOTER_DEMOTING are more controversial. Fixes cockroachdb#57798 Release note: None

andreimatei

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

pkg/kv/kvserver/batcheval/cmd_lease.go, line 51 at r9 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Leaseholder

Done.

andreimatei

bors r+

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

craig · 2021-01-25T20:06:49Z

Build succeeded:

GitHub CI (Cockroach)

Fixes cockroachdb#57342. This looks to have been the same thing as cockroachdb#57798, and was fixed by cockroachdb#58722. Release note: None

59762: roachtest: unskip acceptance/bank/cluster-recovery r=irfansharif a=irfansharif Fixes #57342. This looks to have been the same thing as #57798, and was fixed by #58722. Release note: None Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>

andreimatei requested review from bdarnell, tbg and nvanbenschoten January 11, 2021 20:54

nvanbenschoten reviewed Jan 13, 2021

View reviewed changes

andreimatei force-pushed the leader-lease branch from 383e6a8 to 230a2cc Compare January 14, 2021 22:36

andreimatei commented Jan 14, 2021

View reviewed changes

nvanbenschoten approved these changes Jan 15, 2021

View reviewed changes

nvanbenschoten reviewed Jan 16, 2021

View reviewed changes

nvanbenschoten mentioned this pull request Jan 21, 2021

roachtest: tpccbench/nodes=6/cpu=16/multi-az failed #58298

Closed

kvserver: remove non-sensical assertion

70b90af

This assertion had become nonsensical in #9915f0d0f104aa918c94340e9e47129b90421999. It's from a time where the surrounding function was dealing with the local store only. Release note: None

andreimatei force-pushed the leader-lease branch from 230a2cc to e94acdd Compare January 22, 2021 18:42

andreimatei commented Jan 22, 2021

View reviewed changes

nvanbenschoten approved these changes Jan 23, 2021

View reviewed changes

tbg approved these changes Jan 25, 2021

View reviewed changes

andreimatei added 2 commits January 25, 2021 12:11

kvserver: reduce function deps

5332106

Make checkCanReceiveLease() more nimble, anticipating broader use. Release note: None

andreimatei force-pushed the leader-lease branch from e94acdd to a767cdd Compare January 25, 2021 17:13

andreimatei commented Jan 25, 2021

View reviewed changes

craig bot merged commit 2ec7d92 into cockroachdb:master Jan 25, 2021

irfansharif mentioned this pull request Feb 3, 2021

roachtest: unskip acceptance/bank/cluster-recovery #59762

Merged

andreimatei mentioned this pull request Mar 18, 2021

release-20.2: backport fixes for "wedged replicas in need of snapshot" #62204

Merged

andreimatei deleted the leader-lease branch December 16, 2021 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: don't refuse to fwd lease proposals in some edge cases #58722

kvserver: don't refuse to fwd lease proposals in some edge cases #58722

andreimatei commented Jan 11, 2021

cockroach-teamcity commented Jan 11, 2021

nvanbenschoten left a comment

andreimatei left a comment

nvanbenschoten left a comment

nvanbenschoten left a comment

andreimatei left a comment

nvanbenschoten left a comment

tbg left a comment

andreimatei left a comment

andreimatei left a comment

craig bot commented Jan 25, 2021

kvserver: don't refuse to fwd lease proposals in some edge cases #58722

kvserver: don't refuse to fwd lease proposals in some edge cases #58722

Conversation

andreimatei commented Jan 11, 2021

cockroach-teamcity commented Jan 11, 2021

nvanbenschoten left a comment

Choose a reason for hiding this comment

andreimatei left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

andreimatei left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

tbg left a comment

Choose a reason for hiding this comment

andreimatei left a comment

Choose a reason for hiding this comment

andreimatei left a comment

Choose a reason for hiding this comment

craig bot commented Jan 25, 2021