storage: disallow down-replication on below quorum ranges

Change `filterUnremovableCandidates` to return zero candidates if the
range is currently below quorum. This likely has little effect in
practice as a below quorum range would be unable to process a replica
removal.

Release note: None

pkg/storage/allocator.go

@@ -1224,16 +1224,19 @@ func filterUnremovableReplicas(
 	brandNewReplicaID roachpb.ReplicaID,
 ) []roachpb.ReplicaDescriptor {
 	upToDateReplicas := filterBehindReplicas(raftStatus, replicas)
-	quorum := computeQuorum(len(replicas) - 1)
-	if len(upToDateReplicas) < quorum {
-		// The number of up-to-date replicas is less than quorum. No replicas can
-		// be removed.
+	oldQuorum := computeQuorum(len(replicas))
+	if len(upToDateReplicas) < oldQuorum {
+		// The number of up-to-date replicas is less than the old quorum. No
+		// replicas can be removed. A below quorum range won't be able to process a
+		// replica removal in any case. The logic here prevents any attempt to even
+		// try the removal.
 		return nil
 	}
 
-	if len(upToDateReplicas) > quorum {
-		// The number of up-to-date replicas is larger than quorum. Any replica can
-		// be removed, though we want to filter out brandNewReplicaID.
+	newQuorum := computeQuorum(len(replicas) - 1)
+	if len(upToDateReplicas) > newQuorum {
+		// The number of up-to-date replicas is larger than the new quorum. Any
+		// replica can be removed, though we want to filter out brandNewReplicaID.
 		if brandNewReplicaID != 0 {
 			candidates := make([]roachpb.ReplicaDescriptor, 0, len(replicas)-len(upToDateReplicas))
 			for _, r := range replicas {
@@ -1246,6 +1249,9 @@ func filterUnremovableReplicas(
 		return replicas
 	}
 
+	// The number of up-to-date replicas is equal to the new quorum. Only allow
+	// removal of behind replicas (except for brandNewReplicaID which is given a
+	// free pass).
 	candidates := make([]roachpb.ReplicaDescriptor, 0, len(replicas)-len(upToDateReplicas))
 	necessary := func(r roachpb.ReplicaDescriptor) bool {
 		if r.ReplicaID == brandNewReplicaID {

pkg/storage/allocator_test.go

@@ -5252,25 +5252,27 @@ func TestFilterUnremovableReplicas(t *testing.T) {
 	}{
 		{0, []uint64{0}, 0, nil},
 		{1, []uint64{1}, 0, nil},
-		{1, []uint64{0, 1}, 0, []uint64{0}},
+		{1, []uint64{0, 1}, 0, nil},
+		{1, []uint64{1, 2}, 0, []uint64{1, 2}},
 		{1, []uint64{1, 2, 3}, 0, []uint64{1, 2, 3}},
 		{2, []uint64{1, 2, 3}, 0, []uint64{1}},
 		{3, []uint64{1, 2, 3}, 0, nil},
 		{1, []uint64{1, 2, 3, 4}, 0, []uint64{1, 2, 3, 4}},
 		{2, []uint64{1, 2, 3, 4}, 0, []uint64{1, 2, 3, 4}},
-		{3, []uint64{1, 2, 3, 4}, 0, []uint64{1, 2}},
+		{3, []uint64{1, 2, 3, 4}, 0, nil},
 		{2, []uint64{1, 2, 3, 4, 5}, 0, []uint64{1, 2, 3, 4, 5}},
 		{3, []uint64{1, 2, 3, 4, 5}, 0, []uint64{1, 2}},
 		{1, []uint64{1, 0}, 2, nil},
-		// TODO(peter): Is this the desired behavior? For a 2-replica range, we
-		// won't be able to process the removal of a replica unless both are
-		// up-to-date.
-		{1, []uint64{1, 0}, 1, []uint64{0}},
+		{1, []uint64{2, 1}, 2, []uint64{2}},
+		{1, []uint64{1, 0}, 1, nil},
+		{1, []uint64{2, 1}, 1, []uint64{1}},
 		{3, []uint64{3, 2, 1}, 3, nil},
 		{3, []uint64{3, 2, 0}, 3, nil},
-		{3, []uint64{4, 3, 2, 1}, 4, []uint64{2}},
-		{3, []uint64{4, 3, 2, 0}, 3, []uint64{0}},
-		{3, []uint64{4, 3, 2, 0}, 4, []uint64{2}},
+		{2, []uint64{4, 3, 2, 1}, 4, []uint64{4, 3, 2}},
+		{2, []uint64{4, 3, 2, 0}, 3, []uint64{4, 3, 0}},
+		{2, []uint64{4, 3, 2, 0}, 4, []uint64{4, 3, 2}},
+		{3, []uint64{4, 3, 2, 1}, 0, nil},
+		{3, []uint64{4, 3, 2, 1}, 4, nil},
 	}
 	for _, c := range testCases {
 		t.Run("", func(t *testing.T) {
@@ -5320,7 +5322,7 @@ func TestSimulateFilterUnremovableReplicas(t *testing.T) {
 		expected          []uint64
 	}{
 		{1, []uint64{1, 0}, 2, []uint64{1}},
-		{1, []uint64{1, 0}, 1, []uint64{0}},
+		{1, []uint64{1, 0}, 1, nil},
 		{3, []uint64{3, 2, 1}, 3, []uint64{2}},
 		{3, []uint64{3, 2, 0}, 3, []uint64{2}},
 		{3, []uint64{4, 3, 2, 1}, 4, []uint64{4, 3, 2}},