[cluster-bus] Send a MEET packet to a node if there is no inbound link #1307

pieturin · 2024-11-14T20:03:46Z

In some cases, when meeting a new node, if the handshake times out, we can end up with an inconsistent view of the cluster where the new node knows about all the nodes in the cluster, but the cluster does not know about this new node (or vice versa).
To detect this inconsistency, we now check if a node has an outbound link but no inbound link, in this case it probably means this node does not know us. In this case we (re-)send a MEET packet to this node to do a new handshake with it.

This fixes the bug described in #1251.

In some cases, when meeting a new node, if the handshake times out, we can end up with an inconsistent view of the cluster where the new node knows about all the nodes in the cluster, but the cluster does not know about this new node (or vice versa). To detect this inconsistency, we now check if a node has an outbound link but no inbound link, in this case it probably means this node does not know us. In this case we (re-)send a MEET packet to this node to do a new handshake with it. Signed-off-by: Pierre Turin <[email protected]>

Signed-off-by: Pierre Turin <[email protected]>

codecov · 2024-11-14T20:32:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.67%. Comparing base (32f7541) to head (6c67d41).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1307      +/-   ##
============================================
- Coverage     70.69%   70.67%   -0.02%     
============================================
  Files           115      115              
  Lines         63153    63163      +10     
============================================
  Hits          44643    44643              
- Misses        18510    18520      +10

Files with missing lines	Coverage Δ
src/cluster_legacy.c	`86.20% <100.00%> (+0.01%)`	⬆️

... and 13 files with indirect coverage changes

hpatro

What if we disconnect the outbound link if inbound link is not available? I think it will lead to the same reconnection flow. Would it help with having simpler code and one unified flow. I'm not sure if it will perform the MEET operation though.

hpatro · 2024-11-14T21:18:27Z

src/cluster_legacy.c

-        }

-        /* If this is a MEET packet from an unknown node, we still process
-         * the gossip section here since we have to trust the sender because
-         * of the message type. */
-        if (!sender && type == CLUSTERMSG_TYPE_MEET) clusterProcessGossipSection(hdr, link);
+            /* If this is a MEET packet from an unknown node, we still process
+             * the gossip section here since we have to trust the sender because
+             * of the message type. */
+            clusterProcessGossipSection(hdr, link);
+        }


We don't need this change.

True, but this double if with the same condition was driving me crazy.

I don't mind this. But in general we avoid changes to the lines of code not related to the PR.

hpatro · 2024-11-14T21:20:18Z

src/cluster_legacy.h

@@ -60,6 +60,7 @@ typedef struct clusterLink {
 #define nodeIsPrimary(n) ((n)->flags & CLUSTER_NODE_PRIMARY)
 #define nodeIsReplica(n) ((n)->flags & CLUSTER_NODE_REPLICA)
 #define nodeInHandshake(n) ((n)->flags & CLUSTER_NODE_HANDSHAKE)
+#define nodeIsMeeting(n) ((n)->flags & CLUSTER_NODE_MEET)


How about nodeInMeetProcess / nodeInMeetState ?

Ah yes, I do prefer nodeInMeetState().

hpatro · 2024-11-14T21:23:06Z

src/cluster_legacy.c

        clusterDelNode(node);
        return 1;
    }
+    if (node->link != NULL && node->inbound_link == NULL &&
+        !nodeInHandshake(node) && !nodeIsMeeting(node) && !nodeTimedOut(node) &&


Should we create a macro for this node state check? Not readable at this point.

nodeInNormalState()?

nodeInHealthyState() ?

src/cluster_legacy.c

tests/unit/cluster/cluster-reliable-meet.tcl

hpatro · 2024-11-14T21:30:27Z

tests/unit/cluster/cluster-reliable-meet.tcl

+                [llength [R 0 CLUSTER NODES]] == 26 &&
+                [llength [R 1 CLUSTER NODES]] == 26 &&
+                [llength [R 2 CLUSTER NODES]] == 26


Could we match certain value in the string output? I don't like this magic number comparison which can change in the near future.

tests/unit/cluster/cluster-reliable-meet.tcl

pieturin · 2024-11-14T22:23:27Z

What if we disconnect the outbound link if inbound link is not available?

In this case we would just re-open an outbound connection, which the other node will accept, but it won't force the other node to recognize us as being part of the cluster if it doesn't trust us yet. The only way to force the other node to add us to its cluster view is for us to send a MEET packet.

Update test to check node IDs instead of relying on number of words. Rename nodeIsMeeting() to nodeInMeetState(). Introduce nodeInNormalState() macro. Signed-off-by: Pierre Turin <[email protected]>

hpatro · 2024-11-14T23:19:16Z

What if we disconnect the outbound link if inbound link is not available?

In this case we would just re-opened an outbound connection, which the other node will accept, but it won't force the other node to recognize us as being part of the cluster if it doesn't trust us yet. The only way to force the other node to add us to its cluster view is for us to send a MEET packet.

CLUSTER MEET is an admin operation but I guess we are fine with the case of reinitiating it if the operation wasn't successful in first place and retry it.

hpatro · 2024-11-15T03:29:09Z

tests/unit/cluster/cluster-reliable-meet.tcl

+proc cluster_3_nodes_all_know_each_other {} {
+    set node0_id [dict get [get_myself 0] id]
+    set node1_id [dict get [get_myself 1] id]
+    set node2_id [dict get [get_myself 2] id]
+
+    if {
+        [cluster_get_node_by_id 0 $node0_id] != {} &&
+        [cluster_get_node_by_id 0 $node1_id] != {} &&
+        [cluster_get_node_by_id 0 $node2_id] != {} &&
+        [cluster_get_node_by_id 1 $node0_id] != {} &&
+        [cluster_get_node_by_id 1 $node1_id] != {} &&
+        [cluster_get_node_by_id 1 $node2_id] != {} &&
+        [cluster_get_node_by_id 2 $node0_id] != {} &&
+        [cluster_get_node_by_id 2 $node1_id] != {} &&
+        [cluster_get_node_by_id 2 $node2_id] != {} &&
+        [llength [R 0 CLUSTER LINKS]] == 4 &&
+        [llength [R 1 CLUSTER LINKS]] == 4 &&
+        [llength [R 2 CLUSTER LINKS]] == 4
+    } {
+        return 1
+    } else {
+        return 0
+    }
+}


From ChatGPT:

Suggested change

proc cluster_3_nodes_all_know_each_other {} {

set node0_id [dict get [get_myself 0] id]

set node1_id [dict get [get_myself 1] id]

set node2_id [dict get [get_myself 2] id]

if {

[cluster_get_node_by_id 0 $node0_id] != {} &&

[cluster_get_node_by_id 0 $node1_id] != {} &&

[cluster_get_node_by_id 0 $node2_id] != {} &&

[cluster_get_node_by_id 1 $node0_id] != {} &&

[cluster_get_node_by_id 1 $node1_id] != {} &&

[cluster_get_node_by_id 1 $node2_id] != {} &&

[cluster_get_node_by_id 2 $node0_id] != {} &&

[cluster_get_node_by_id 2 $node1_id] != {} &&

[cluster_get_node_by_id 2 $node2_id] != {} &&

[llength [R 0 CLUSTER LINKS]] == 4 &&

[llength [R 1 CLUSTER LINKS]] == 4 &&

[llength [R 2 CLUSTER LINKS]] == 4

} {

return 1

} else {

return 0

}

}

proc cluster_nodes_all_know_each_other {num_nodes} {

# Collect node IDs dynamically

set node_ids {}

for {set i 0} {$i < $num_nodes} {incr i} {

lappend node_ids [dict get [get_myself $i] id]

}

# Check if all nodes know each other

foreach node_id $node_ids {

foreach check_node_id $node_ids {

for {set node_index 0} {$node_index < $num_nodes} {incr node_index} {

if {[cluster_get_node_by_id $node_index $check_node_id] == {}} {

return 0

}

}

}

}

# Verify cluster link counts for each node

set expected_links [expr {2 * ($num_nodes - 1)}]

for {set i 0} {$i < $num_nodes} {incr i} {

if {[llength [R $i CLUSTER LINKS]] != $expected_links} {

return 0

}

}

return 1

}

pieturin added 2 commits November 14, 2024 19:53

Fix typo in comment

d65423e

Signed-off-by: Pierre Turin <[email protected]>

pieturin force-pushed the cluster-handshake-fix branch from 1920952 to d65423e Compare November 14, 2024 20:16

hpatro reviewed Nov 14, 2024

View reviewed changes

Addressed PR comments

6c67d41

Update test to check node IDs instead of relying on number of words. Rename nodeIsMeeting() to nodeInMeetState(). Introduce nodeInNormalState() macro. Signed-off-by: Pierre Turin <[email protected]>

hpatro reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cluster-bus] Send a MEET packet to a node if there is no inbound link #1307

[cluster-bus] Send a MEET packet to a node if there is no inbound link #1307

pieturin commented Nov 14, 2024

codecov bot commented Nov 14, 2024 •

edited

Loading

hpatro left a comment

hpatro Nov 14, 2024

pieturin Nov 14, 2024

hpatro Nov 15, 2024

hpatro Nov 14, 2024

pieturin Nov 14, 2024

hpatro Nov 14, 2024

pieturin Nov 14, 2024

hpatro Nov 15, 2024

hpatro Nov 14, 2024

pieturin commented Nov 14, 2024 •

edited

Loading

hpatro commented Nov 14, 2024

hpatro Nov 15, 2024

[cluster-bus] Send a MEET packet to a node if there is no inbound link #1307

Are you sure you want to change the base?

[cluster-bus] Send a MEET packet to a node if there is no inbound link #1307

Conversation

pieturin commented Nov 14, 2024

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

hpatro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieturin commented Nov 14, 2024 • edited Loading

hpatro commented Nov 14, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2024 •

edited

Loading

pieturin commented Nov 14, 2024 •

edited

Loading