kvserver: improve handling for removal of a replica, when multiple replicas already exist on the same node #60546

lunevalex · 2021-02-12T22:08:20Z

The allocator in some cases allows for a range to have a replica
on multiple stores of the same node. If that happens, it should allow
itself to fix the situation by removing one of the offending replicas.
This was only half working due to an ordering problem in how the replicas
appeared in the descriptor. It could remove the first replica, but not the second one.

.

Release note: None

cockroach-teamcity · 2021-02-12T22:08:28Z

This change is

nvanbenschoten

but @aayushshah15 should also sign off. Thanks for the quick patch.

How far back will we need to backport this? Also, if we're going to then we should add a release note.

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

aayushshah15 · 2021-02-12T22:17:54Z

How far back will we need to backport this?

IIUC bug should only affect 20.2

aayushshah15

I think the backport to 20.2 may or may not be clean, my apologies for that in advance.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @lunevalex)

pkg/kv/kvserver/replica_command.go, line 1153 at r1 (raw file):

}

func validateReplicationChangesMultipleReplicasOnTheSameStore(

s/OnTheSameStore/OnTheSameNode

pkg/kv/kvserver/replica_command.go, line 1208 at r1 (raw file):

	}

	// Then, check that we're not adding a second replica on nodes that already

should this be s/nodes/stores?

pkg/kv/kvserver/replica_command.go, line 1211 at r1 (raw file):

	// have one, or "re-add" an existing replica. We delete from byNodeAndStoreID so that
	// after this loop, it contains only Nodes that we haven't seen in desc.
	for nodeID, descs := range descriptorsByNodeID {

should we rename descs to something like rDescsForNode, just to be very explicit?

pkg/kv/kvserver/replica_command.go, line 1217 at r1 (raw file):

		}
		delete(byNodeAndStoreID, nodeID)
		// The only valid thing to do when we already have multiple replicas on the

I'm having a bit of a hard time convincing myself that this is always true. When there's a (VOTER, LEARNER) pair on the same node, you're right that we'd expect the allocator to always try to remove that LEARNER, but I'm not sure if we want to make that assumption here?

For instance, the patch below produces an error, should it? I haven't tested this out, but the situation in the patch should be possible if the user intentionally / inadvertently constrains, say, 3 replicas to a node that has 3 stores.

Index: pkg/kv/kvserver/replica_command_test.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/pkg/kv/kvserver/replica_command_test.go b/pkg/kv/kvserver/replica_command_test.go
--- a/pkg/kv/kvserver/replica_command_test.go	(revision 688740b898700cd8ea5cd9db3ff0def457b83505)
+++ b/pkg/kv/kvserver/replica_command_test.go	(date 1613169923000)
@@ -198,7 +198,7 @@
 		InternalReplicas: []roachpb.ReplicaDescriptor{
 			{NodeID: 1, StoreID: 1},
 			{NodeID: 2, StoreID: 2},
-			{NodeID: 1, StoreID: 2, Type: &learnerType},
+			{NodeID: 1, StoreID: 3, Type: &learnerType},
 		},
 	}
 	err = validateReplicationChanges(descRebalancing, roachpb.ReplicationChanges{
@@ -208,7 +208,12 @@
 
 	// Test Case 15: same as 14 but remove the second node
 	err = validateReplicationChanges(descRebalancing, roachpb.ReplicationChanges{
-		{ChangeType: roachpb.REMOVE_VOTER, Target: roachpb.ReplicationTarget{NodeID: 1, StoreID: 2}},
+		{ChangeType: roachpb.REMOVE_VOTER, Target: roachpb.ReplicationTarget{NodeID: 1, StoreID: 3}},
+	})
+	require.NoError(t, err)
+
+	err = validateReplicationChanges(descRebalancing, roachpb.ReplicationChanges{
+		{roachpb.ADD_VOTER, roachpb.ReplicationTarget{NodeID: 1, StoreID: 4}},
 	})
 	require.NoError(t, err)

pkg/kv/kvserver/replica_command_test.go, line 201 at r1 (raw file):

			{NodeID: 1, StoreID: 1},
			{NodeID: 2, StoreID: 2},
			{NodeID: 1, StoreID: 2, Type: &learnerType},

nit: we should probably have 3 distinct store ids here.

lunevalex

Thats right only 20.2, good call on release note adding.

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

pkg/kv/kvserver/replica_command.go, line 1153 at r1 (raw file):