[BUG] Two primaries in same shard

[deepakrn]

Describe the bug

Bug where a shard ends up with 2 primaries and when cluster-allow-replica-migration is disabled.

To reproduce

Let’s say a shard has a primary(A) and a replica(B). Also let's assume that cluster-allow-replica-migration is disabled.

1. Node A goes down, and Node B takes over primaryship.
2. Node A continues to be down while another Node C is added as a replica of B.
3. Node B goes down, and Node C takes over primaryship.
4. Node A and Node B come back up and start learning about the topology.
5. Node A comes up thinking it was the primary (but has an older config epoch compared to C).
6. Node A learns about Node C via gossip and assigns it a random shard_id.
7. Node A receives a direct ping from Node C.
   a. Node C advertises the same set of slots that Node A was earlier owning.
   b. Since Node A assigns a random shard ID to Node C, Node A thinks that it is still a primary and it lost all its slots to Node C, which is in another shard.
8. Node A then updates the actual shard_id of Node C while processing shard_id in ping extensions.
9. Node A and Node C end up being primaries in the same shard while Node C continues to own slots.

Expected behavior

I would have expected node A to become a replica of node C after learning that node C is in the same shard.

Potential fix

Currently, when a node A receives a ping from node C, it first processes the slots config and then processes the shard_id in the ping extension. One way to fix this could be to process the shard_id or all the ping extensions and then later update slots configuration.

[enjoy-binbin]

8. Node A then updates the actual shard_id of Node C while processing shard_id in ping extensions.
9. Node A and Node C end up being primaries in the same shard while Node C continues to own slots.

So in the step 8, does Node A and Node C has the same shard_id? I am guessing they have the same shard_id, and both report themselves as a primary, but Node C has a bigger config epoch, i guess in this case, we can try to make Node A become a replica of Node C? This seems safe to me

@PingXie Thoughts?

[deepakrn]

@enjoy-binbin - yeah, I agree. That seems less intrusive than changing the order of processing ping extensions and slot updates. This should probably address all scenarios that could result in 2 primaries in a shard where one of them have a higher config epoch.

I'll wait for @PingXie to chime in as well.

[enjoy-binbin]

A diff like this should be fine. I guess we can also write a test case to reproduce the scenario you mentioned. I will try it later.

     updateShardId(sender, ext_shardid);
+
+    /* In some cases, we may first learn about other nodes through gossip (assigning a
+     * random shard_id), and then learn the sender's shard_id through ping extensions.
+     * Since the processing order is random, we may end up with multiple primary nodes
+     * in a shard, in this case, if the sender's config epoch is greater, we configure
+     * myself as its replica. */
+    if (nodeIsPrimary(myself) && nodeIsPrimary(sender) && areInSameShard(myself, sender) &&
+        nodeEpoch(sender) > nodeEpoch(myself)) {
+        serverLog(LL_NOTICE, "Two primaries in same shard, and the sender has a greater config epoch. "
+                             "Reconfiguring myself as a replica of %.40s (%s)",
+                  sender->name, sender->human_nodename);
+        clusterSetPrimary(sender, 1, 0);
+        clusterDoBeforeSleep(CLUSTER_TODO_SAVE_CONFIG | CLUSTER_TODO_UPDATE_STATE | CLUSTER_TODO_FSYNC_CONFIG);
+    }

It was a detailed report, thank you for the great writing btw.

[PingXie]

thanks for the detailed analysis, @deepakrn!

8. Node A then updates the actual shard_id of Node C while processing shard_id in ping extensions.
9. Node A and Node C end up being primaries in the same shard while Node C continues to own slots.

So in the step 8, does Node A and Node C has the same shard_id? I am guessing they have the same shard_id, and both report themselves as a primary, but Node C has a bigger config epoch, i guess in this case, we can try to make Node A become a replica of Node C? This seems safe to me

I feel that this is a bit hacky. apparently clusterUpdateSlotsConfigWith now has dependency on correct shard-id. rather than going with the incorrect shard-id in clusterUpdateSlotsConfigWith and attempting to fix it later, I wonder if we should process extensions first, or at least the shard-id extension?

[enjoy-binbin]

I feel that this is a bit hacky. apparently clusterUpdateSlotsConfigWith now has dependency on correct shard-id. rather than going with the incorrect shard-id in clusterUpdateSlotsConfigWith and attempting to fix it later, I wonder if we should process extensions first, or at least the shard-id extension?

yes, you made a good point. Now the whole clusterProcessPacket often depends on shard_id, so we do probably need to move it to the very front. @zuiderkwast @hpatro Thoughts?