From 22b4fb519592bd5f51f2e67d1c48d5c5c11880cd Mon Sep 17 00:00:00 2001
From: shralex <shralex@gmail.com>
Date: Thu, 6 Jan 2022 12:03:44 -0800
Subject: [PATCH] kvserver: allow VOTER_INCOMING to receive the lease

Previously, VOTER_INCOMING replicas joining the cluster weren't allowed
to receive the lease, even though there is no actual problem with doing so.
This change removes the restriction, as a step towards https://github.com/cockroachdb/cockroach/pull/74077

Release note: None
---
 pkg/kv/kvserver/batcheval/cmd_lease_test.go |  2 +-
 pkg/kv/kvserver/replica_learner_test.go     | 12 ++++++------
 pkg/roachpb/metadata_replicas.go            | 15 +++++++--------
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/pkg/kv/kvserver/batcheval/cmd_lease_test.go b/pkg/kv/kvserver/batcheval/cmd_lease_test.go
index d5cd5efa023e..601e4d4bd70f 100644
--- a/pkg/kv/kvserver/batcheval/cmd_lease_test.go
+++ b/pkg/kv/kvserver/batcheval/cmd_lease_test.go
@@ -270,7 +270,7 @@ func TestCheckCanReceiveLease(t *testing.T) {
 		eligible        bool
 	}{
 		{leaseholderType: roachpb.VOTER_FULL, eligible: true},
-		{leaseholderType: roachpb.VOTER_INCOMING, eligible: false},
+		{leaseholderType: roachpb.VOTER_INCOMING, eligible: true},
 		{leaseholderType: roachpb.VOTER_OUTGOING, eligible: false},
 		{leaseholderType: roachpb.VOTER_DEMOTING_LEARNER, eligible: false},
 		{leaseholderType: roachpb.VOTER_DEMOTING_NON_VOTER, eligible: false},
diff --git a/pkg/kv/kvserver/replica_learner_test.go b/pkg/kv/kvserver/replica_learner_test.go
index 202ed815b552..2700c7fc2749 100644
--- a/pkg/kv/kvserver/replica_learner_test.go
+++ b/pkg/kv/kvserver/replica_learner_test.go
@@ -839,8 +839,8 @@ func TestLearnerNoAcceptLease(t *testing.T) {
 	}
 }
 
-// TestJointConfigLease verifies that incoming and outgoing voters can't have the
-// lease transferred to them.
+// TestJointConfigLease verifies that incoming voters can have the
+// lease transferred to them, and outgoing voters cannot.
 func TestJointConfigLease(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
@@ -859,14 +859,14 @@ func TestJointConfigLease(t *testing.T) {
 	require.True(t, desc.Replicas().InAtomicReplicationChange(), desc)
 
 	err := tc.TransferRangeLease(desc, tc.Target(1))
-	exp := `replica cannot hold lease`
-	require.True(t, testutils.IsError(err, exp), err)
+	require.NoError(t, err)
 
 	// NB: we don't have to transition out of the previous joint config first
 	// because this is done automatically by ChangeReplicas before it does what
 	// it's asked to do.
-	desc = tc.RemoveVotersOrFatal(t, k, tc.Target(1))
-	err = tc.TransferRangeLease(desc, tc.Target(1))
+	desc = tc.RemoveVotersOrFatal(t, k, tc.Target(0))
+	err = tc.TransferRangeLease(desc, tc.Target(0))
+	exp := `replica cannot hold lease`
 	require.True(t, testutils.IsError(err, exp), err)
 }
 
diff --git a/pkg/roachpb/metadata_replicas.go b/pkg/roachpb/metadata_replicas.go
index 2262f32ff955..1c99da46f4ac 100644
--- a/pkg/roachpb/metadata_replicas.go
+++ b/pkg/roachpb/metadata_replicas.go
@@ -524,14 +524,13 @@ func CheckCanReceiveLease(wouldbeLeaseholder ReplicaDescriptor, rngDesc *RangeDe
 	repDesc, ok := rngDesc.GetReplicaDescriptorByID(wouldbeLeaseholder.ReplicaID)
 	if !ok {
 		return errReplicaNotFound
-	} else if t := repDesc.GetType(); t != VOTER_FULL {
-		// NB: there's no harm in transferring the lease to a VOTER_INCOMING,
-		// but we disallow it anyway. On the other hand, transferring to
-		// VOTER_OUTGOING would be a pretty bad idea since those voters are
-		// dropped when transitioning out of the joint config, which then
-		// amounts to removing the leaseholder without any safety precautions.
-		// This would either wedge the range or allow illegal reads to be
-		// served.
+	} else if !repDesc.IsVoterNewConfig() {
+		// NB: there's no harm in transferring the lease to a VOTER_INCOMING.
+		// On the other hand, transferring to VOTER_OUTGOING would be a pretty bad
+		// idea since those voters are dropped when transitioning out of the joint
+		// config, which then amounts to removing the leaseholder without any
+		// safety precautions. This would either wedge the range or allow illegal
+		// reads to be served.
 		//
 		// Since the leaseholder can't remove itself and is a VOTER_FULL, we
 		// also know that in any configuration there's at least one VOTER_FULL.