Make cluster meet reliable under link failures #461

srgsanky · 2024-05-08T02:46:26Z

When there is a link failure while an ongoing MEET request is sent the sending node stops sending anymore MEET and starts sending PINGs. Since every node responds to PINGs from unknown nodes with a PONG, the receiving node never adds the sending node. But the sending node adds the receiving node when it sees a PONG. This can lead to asymmetry in cluster membership. This changes makes the sender keep sending MEET until it sees a PONG, avoiding the asymmetry.

srgsanky · 2024-05-08T02:48:05Z

Posting this for initial comments. I can migrate the test based on the new framework once #442 is merged.

codecov · 2024-05-08T03:13:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.20%. Comparing base (168da8b) to head (7ac84b6).
Report is 6 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #461      +/-   ##
============================================
- Coverage     70.22%   70.20%   -0.02%     
============================================
  Files           109      109              
  Lines         59956    59967      +11     
============================================
- Hits          42104    42102       -2     
- Misses        17852    17865      +13

Files	Coverage Δ
src/cluster_legacy.c	`86.52% <100.00%> (+0.07%)`	⬆️
src/debug.c	`54.07% <100.00%> (+0.13%)`	⬆️

... and 9 files with indirect coverage changes

srgsanky · 2024-05-08T17:14:34Z

@madolson @hpatro @PingXie can one of you help review this change?

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

src/cluster_legacy.c

madolson · 2024-05-13T01:23:16Z

src/cluster_legacy.c

-     * normal PING packets. */
-    node->flags &= ~CLUSTER_NODE_MEET;
-
+    /* NOTE: We cannot clear the MEET flag from the node until we get a response


This makes sense to me, but I thought we briefly discussed just only sending the meet once, and on reconnect just not sending another meet. The previously logic was "We'll only send a single meet", I'm wondering if there was any logic somewhere that relied on that behavior.

Are we getting into the territory that CLUSTER MEET should only be sent when it was generated by the user/admin?

The initial MEET is still triggered by an admin/user command.

/* We can clear the flag after the first packet is sent. * If we'll never receive a PONG, we'll never send new packets * to this node. Instead after the PONG is received and we * are no longer in meet/handshake status, we want to send * normal PING packets. */

The previous code comment above doesn't restrict the MEET to be sent exactly once. I can't think of any scenario where sending multiple MEETs will break - this is similar to when an admin sends MEET multiple times. We send MEET only when the connection is established in clusterLinkConnectHandler, so we still limit sending MEET when connection is newly setup and not in every cluster cron run.

this is similar to when an admin sends MEET multiple times

I believe the cluster deduplicate multiple identical requests to the same IP/port.

B isn't processing these right? It's just immediately dropping the first three and not processing them, it only ever processes the 4th one once correct?

I am not concerned with sending multiple "MEET". It should be idempotent but agreed that we should add a test to validate it (not using the debug hook to fail the previous MEETs)

B isn't processing these right? It's just immediately dropping the first three and not processing them, it only ever processes the 4th one once correct?

Correct. It starts processing when we drop the filter - which can be 4th or later.

agreed that we should add a test to validate it (not using the debug hook to fail the previous MEETs)

To test this scenario, the test has to send packets to the target node pretending to be a Valkey node. This way we can send multiple MEETs. Can you point me to any test that directly sends cluster messages to nodes, so I can write one with multiple MEETs?

I think the test could look like:

Node A is told to meet Node B. We drop all incoming packets on A, so although B get's the response it is never acknowledged.

We force B to drop it's connection with A. We support DEBUG CLUSTERLINK KILL <node id> all. (I forgot about this till just now).

A will reconnect, and send another meet.

Thanks! Added test to make sure multiple meets are handled just like a PING.

**MEET** Node A -----------------> Node B X <---------------- PONG **MEET** ------------------> X <---------------- PONG PING <------------------ ------------------> PONG **MEET** ------------------> X <---------------- PONG ... [Remove DROP PONG] **MEET** ------------------> <------------------ PONG PING ------------------> <------------------ PONG PING <------------------ ------------------> PONG

hpatro · 2024-05-13T14:23:40Z

src/cluster_legacy.c

-     * normal PING packets. */
-    node->flags &= ~CLUSTER_NODE_MEET;
-
+    /* NOTE: We cannot clear the MEET flag from the node until we get a response


Are we getting into the territory that CLUSTER MEET should only be sent when it was generated by the user/admin?

src/server.h

hpatro · 2024-05-13T14:32:01Z

I think it's worth investing on this redis/redis#11095 to avoid this issue altogether.

srgsanky · 2024-05-13T19:36:02Z

I think it's worth investing on this redis/redis#11095 to avoid this issue altogether.

Thanks, I wasn't aware of this linked issue. IMO these two issues can be solved independently. The linked issue tries to make the admin experience better for MEET command where as this PR tries to address a specific gap in MEET implementation.

With SYNC MEET, we will have to make changes to admin client timeout. This timeout can possible trickle up the stack in a control plane implementation.
If we choose to attempt handshake for a longer period of time, we either have to filter out nodes in handshake in cluster nodes output for a non-admin client or make the clients filter out the nodes with this new flag. This can require a client side change to avoid connecting to a node in handshake, experiencing availability issues.

The problem addressed in this PR (asymmetric cluster membership) can happen with SYNC MEET as well due to link failures. So, it is worth solving it. The handshake nodes will still be removed after the handshake timeout (same as node_timeout of 15s). Wdyt?

madolson · 2024-05-13T20:02:34Z

The problem addressed in this PR (asymmetric cluster membership) can happen with SYNC MEET as well due to link failures. So, it is worth solving it. The handshake nodes will still be removed after the handshake timeout (same as node_timeout of 15s). Wdyt?

Yeah, I still believe this a problem even with the #11095.

zuiderkwast · 2024-05-13T20:31:18Z

Awesome material for our next release which will be full of cluster improvements. Is it worth mentioning in release notes?

Btw @srgsanky you need to commit with -s. See the instructions on the DCO CI job's details page.

madolson · 2024-05-13T20:32:35Z

Awesome material for our next release which will be full of cluster improvements. Is it worth mentioning in release notes?

I would also be inclined to backport it.

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

srgsanky · 2024-05-19T21:12:21Z

Awesome material for our next release which will be full of cluster improvements. Is it worth mentioning in release notes?

Btw @srgsanky you need to commit with -s. See the instructions on the DCO CI job's details page.

When I tried to merge the new changes into my fork, I ended up with a merge commit

* 2ff9879fa (HEAD -> unstable, origin/unstable, origin/HEAD) Moved test under unit and addressed other comments
*   b826ef77a Merge branch 'valkey-io:unstable' into unstable
|\
| * d52c8f30e Include stddef in zmalloc.h (#516)
| * dcc9fd4fe Resolve numtests counter error (#514)
...
| * 315b7573c Update server function's name to valkey (#456)
* | 49a884c06 Make cluster meet reliable under link failures
|/
* 4e944cede Migrate kvstore.c unit tests to new test framework. (#446)

I want to signoff just 49a884c, but the rebase is adding a signoff to all commits 315b757..d52c8f3 which are not made by me.

Do you have any recommendation to fix this?

As an alternate option, I can start fresh and add a new commit from the tip of unstable. I am not sure if I will be able to reuse this PR.

zuiderkwast · 2024-05-19T22:15:50Z

I believe it's possible to undo a merge by git reset --hard 49a884c06 (the commit before the merge commit), then rebase to add the --signoff, then do git merge unstable again. The commit you added after merge commit can be cherry-picked after all this. Just remember the commit id.

If nothing works, then it's always possible to start from scratch with a new branch and cherry-pick all your commits into it. Then you can rename the branches and force-push to this PR's branch.

madolson · 2024-05-20T02:20:49Z

@srgsanky ~~The commit missing the DCO is just the top one. You should just be able to do git commit -s --amend with a no-op and force push over what you have.~~ It's the base commit, nevermind. I believe git rebase -i HEAD~3 should allow you to manually add the signature, maybe there is a better way to do it.

madolson

Just some minor nitpicks around the tests, it overall LGTM.

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

tests/cluster/tests/includes/init-tests.tcl

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

madolson · 2024-05-20T02:33:08Z

src/cluster_legacy.c

-     * normal PING packets. */
-    node->flags &= ~CLUSTER_NODE_MEET;
-
+    /* NOTE: We cannot clear the MEET flag from the node until we get a response


B isn't processing these right? It's just immediately dropping the first three and not processing them, it only ever processes the 4th one once correct?

When there is a link failure while an ongoing MEET request is sent the sending node stops sending anymore MEET and starts sending PINGs. Since every node responds to PINGs from unknown nodes with a PONG, the receiving node never adds the sending node. But the sending node adds the receiving node when it sees a PONG. This can lead to asymmetry in cluster membership. This changes makes the sender keep sending MEET until it sees a PONG, avoiding the asymmetry. Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

srgsanky · 2024-05-20T04:17:20Z

B isn't processing these right? It's just immediately dropping the first three and not processing them, it only ever processes the 4th one once correct?

Correct. It starts processing when we drop the filter - which can be 4th or later.

srgsanky · 2024-05-20T04:19:37Z

I believe it's possible to undo a merge by git reset --hard 49a884c (the commit before the merge commit), then rebase to add the --signoff, then do git merge unstable again. The commit you added after merge commit can be cherry-picked after all this. Just remember the commit id.

This worked. Thanks!

It's the base commit, nevermind. I believe git rebase -i HEAD~3 should allow you to manually add the signature, maybe there is a better way to do it.

I tried this and all the commits in the other branch of the merge was also annotated with my signoff. So, I decided to ask you folks for the best approach.

btw is there any reasoning behind the requirement for the signoff?

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

madolson · 2024-05-20T04:56:13Z

btw is there any reasoning behind the requirement for the signoff?

Technically we adopted it because it's an LF requirement, but it's also a good practice to force a trail of who committed what.

PingXie

this change LGTM overall.

src/server.h

src/cluster_legacy.c

PingXie · 2024-05-20T05:43:24Z

src/cluster_legacy.c

-     * normal PING packets. */
-    node->flags &= ~CLUSTER_NODE_MEET;
-
+    /* NOTE: We cannot clear the MEET flag from the node until we get a response


I am not concerned with sending multiple "MEET". It should be idempotent but agreed that we should add a test to validate it (not using the debug hook to fail the previous MEETs)

src/cluster_legacy.c

src/debug.c

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

1. Reworked code comment 1. Added serverLogs 1. Renamed debug variable 1. Made close link filter to be directly coupled with drop filter Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

Multiple MEETs will be handled like a normal PING message. Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

srgsanky · 2024-05-29T02:57:16Z

The clang-format checker is currently failing due to changes introduced by another PR. Mentioned this in #118 (comment)

LiiNen · 2024-05-29T03:23:07Z

sorry. maybe i missed some. fixed #570

ref: - #118 (my pervious change) - #461 (issuing that clang format checker fails due to my change) There was an issue that clang-format cheker failed. I don't know why I missed it and why it didn't catch. just running `clang-format -i bitops.c` was all. Signed-off-by: LiiNen <kjeonghoon065@gmail.com>

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

madolson · 2024-05-29T15:21:57Z

src/server.h

+    uint32_t
+        debug_cluster_close_link_on_packet_drop : 1; /* Debug config that goes along with cluster_drop_packet_filter.
+                                                        When set, the link is closed on packet drop. */


Did clang format do this automatically? This is such a weird format to split the type and the variable name on to to different lines.

Yes, clang did it. Agree..

Yeah, this is why it's good that we don't run clang-format on merge. Now at least we have a chance to fix it manually. Just move the comment to it's own line.

Suggested change

uint32_t

debug_cluster_close_link_on_packet_drop : 1; /* Debug config that goes along with cluster_drop_packet_filter.

When set, the link is closed on packet drop. */

/* Debug config that goes along with cluster_drop_packet_filter.

* When set, the link is closed on packet drop. */

uint32_t debug_cluster_close_link_on_packet_drop : 1;

Btw, using one bit for this field will not save memory. We'd need to do it for some more fields and put them next to each other. Otherwise IMHO there's not point.

Agree, there is no value now, but by keeping it to a bit, we can keep the intent clear and do a subsequent merge of such fields like you pointed out in the other comment (#461 (comment)).

OK. You're welcome to include this refactoring in this PR, but I won't insist. We'll open a follow-up issue for it otherwise.

We'll open a follow-up issue for it otherwise.

We do have a hard problem to solve which is that the config system (as it is written) doesn't support bit fields since bit fields are not addressable. Most of the booleans we have are bit fields.

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

madolson reviewed May 13, 2024

View reviewed changes

tests/cluster/tests/30-reliable-meet.tcl Outdated Show resolved Hide resolved

madolson reviewed May 13, 2024

View reviewed changes

src/cluster_legacy.c Outdated Show resolved Hide resolved

madolson reviewed May 13, 2024

View reviewed changes

hpatro reviewed May 13, 2024

View reviewed changes

madolson added the release-notes This issue should get a line item in the release notes label May 13, 2024

madolson reviewed May 13, 2024

View reviewed changes

tests/cluster/tests/30-reliable-meet.tcl Outdated Show resolved Hide resolved

srgsanky force-pushed the unstable branch 2 times, most recently from d8aa71c to 2ff9879 Compare May 19, 2024 21:22

madolson reviewed May 20, 2024

View reviewed changes

srgsanky added 3 commits May 19, 2024 21:07

Merge commit 'd52c8f30e' into unstable

d8e6da5

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

Moved test under unit and addressed other comments

ec68829

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

Addressed comments about the unit test

a760aa6

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

srgsanky force-pushed the unstable branch from 2ff9879 to a760aa6 Compare May 20, 2024 04:20

PingXie reviewed May 20, 2024

View reviewed changes

hpatro approved these changes May 20, 2024

View reviewed changes

src/debug.c Outdated Show resolved Hide resolved

tests/unit/cluster/cluster-reliable-meet.tcl Outdated Show resolved Hide resolved

srgsanky added 5 commits May 20, 2024 19:15

Addressed comments

0cf724a

1. Reworked code comment 1. Added serverLogs 1. Renamed debug variable 1. Made close link filter to be directly coupled with drop filter Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

Added test case for multiple MEETs

1abf81c

Multiple MEETs will be handled like a normal PING message. Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

Merge remote-tracking branch 'upstream/unstable' into unstable

da01bb8

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

Resolved the merge conflict missed in previous commit

be5cb97

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

Fixed clang-format errors

d6b9cbd

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

LiiNen mentioned this pull request May 29, 2024

Fix bitops.c clang-format properly #570

Merged

Merge remote-tracking branch 'upstream/unstable' into unstable

50f896b

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

madolson reviewed May 29, 2024

View reviewed changes

Fixed format to keep type and field in same line

7ac84b6

Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>

Make cluster meet reliable under link failures #461

Are you sure you want to change the base?

Make cluster meet reliable under link failures #461

Conversation

srgsanky commented May 8, 2024

srgsanky commented May 8, 2024

codecov bot commented May 8, 2024 • edited

Codecov Report

srgsanky commented May 8, 2024

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

Choose a reason for hiding this comment

hpatro commented May 13, 2024

srgsanky commented May 13, 2024

madolson commented May 13, 2024

zuiderkwast commented May 13, 2024

madolson commented May 13, 2024

srgsanky commented May 19, 2024

zuiderkwast commented May 19, 2024

madolson commented May 20, 2024 • edited

madolson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srgsanky commented May 20, 2024

srgsanky commented May 20, 2024

madolson commented May 20, 2024

PingXie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srgsanky commented May 29, 2024 • edited

LiiNen commented May 29, 2024

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

codecov bot commented May 8, 2024 •

edited

madolson commented May 20, 2024 •

edited

srgsanky commented May 29, 2024 •

edited