Make cluster meet reliable under link failures

[srgsanky]

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]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.20%. Comparing base (168da8b) to head (7ac84b6).
Report is 33 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]

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

[hpatro]

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

[srgsanky]

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]

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]

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]

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.

[srgsanky]

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]

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]

@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.

[srgsanky]

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]

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?

[madolson]

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.

[srgsanky]

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

[LiiNen]

sorry. maybe i missed some. fixed #570

[PingXie]

Thanks!

[madolson]

@PingXie Sorry, I saw your approval and had intended to merge this on Friday but ran out of time and missed that you had comments. They seemed like small comments, so @srgsanky feel free to address, I commented on one of them.

[PingXie]

No worries. These can all be addressed incrementally.