From 9ed70271734d0871254626d7909cb48fcb348d78 Mon Sep 17 00:00:00 2001
From: Austen McClernon <austen@cockroachlabs.com>
Date: Wed, 5 Jul 2023 18:46:14 +0000
Subject: [PATCH] allocator: promote non-voter in 1-voter rebalance

When there is only 1 voter and 1 non-voter, it was not possible to
rebalance the voter replica onto the store which had the non-voter. When
attempting to rebalance with only 1 voter, we fall back to adding the
new replica first. Adding this new replica fails if the store already
has a non-voting replica.

This patch updates the 1 voter case in  `ReplicationChangesForRebalance`
to also consider whether the add target store already has a non-voting
replica. If a non-voting replica is found, a promotion is now issued
rather than just adding the voter.

Fixes: #104066
Epic: none

Release note: None
---
 pkg/kv/kvserver/allocator/plan/util.go | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/pkg/kv/kvserver/allocator/plan/util.go b/pkg/kv/kvserver/allocator/plan/util.go
index 0bd14b2873dd..cbb8def379eb 100644
--- a/pkg/kv/kvserver/allocator/plan/util.go
+++ b/pkg/kv/kvserver/allocator/plan/util.go
@@ -36,6 +36,7 @@ func ReplicationChangesForRebalance(
 	addTarget, removeTarget roachpb.ReplicationTarget,
 	rebalanceTargetType allocatorimpl.TargetReplicaType,
 ) (chgs []kvpb.ReplicationChange, performingSwap bool, err error) {
+	rdesc, found := desc.GetReplicaDescriptor(addTarget.StoreID)
 	if rebalanceTargetType == allocatorimpl.VoterTarget && numExistingVoters == 1 {
 		// If there's only one replica, the removal target is the
 		// leaseholder and this is unsupported and will fail. However,
@@ -58,14 +59,29 @@ func ReplicationChangesForRebalance(
 		// when we know it's necessary, picking the smaller of two evils.
 		//
 		// See https://github.com/cockroachdb/cockroach/issues/40333.
-		chgs = []kvpb.ReplicationChange{
-			{ChangeType: roachpb.ADD_VOTER, Target: addTarget},
-		}
 		log.KvDistribution.Infof(ctx, "can't swap replica due to lease; falling back to add")
+
+		// Even when there is only 1 existing voter, there may be other replica
+		// types in the range. Check if the add target already has a replica, if so
+		// it must be a non-voter or the rebalance is invalid.
+		if found && rdesc.Type == roachpb.NON_VOTER {
+			// The receiving store already has a non-voting replica. Instead of just
+			// adding a voter to the receiving store, we *must* promote the non-voting
+			// replica to a voter.
+			chgs = kvpb.ReplicationChangesForPromotion(addTarget)
+		} else if !found {
+			chgs = []kvpb.ReplicationChange{
+				{ChangeType: roachpb.ADD_VOTER, Target: addTarget},
+			}
+		} else {
+			return nil, false, errors.AssertionFailedf(
+				"invalid rebalancing decision: trying to"+
+					" move voter to a store that already has a replica %s for the range", rdesc,
+			)
+		}
 		return chgs, false, err
 	}
 
-	rdesc, found := desc.GetReplicaDescriptor(addTarget.StoreID)
 	switch rebalanceTargetType {
 	case allocatorimpl.VoterTarget:
 		// Check if the target being added already has a non-voting replica.