Limiting the new reconnections for failed nodes

[sarthakaggarwal97]

In cluster mode, the node attempts to reconnect to nodes where link == NULL during each execution cycle, which occurs every 100 milliseconds by default. This behavior results in continuous reconnection attempts to nodes that are unreachable or in a failed state (PFAIL or FAIL)

This PR addresses an issue where the system aggressively attempts to reconnect to failed nodes, which can lead to resource exhaustion and potential instability. This change limits the number of attempts we make per failed node.

The PR improves engine CPU of the P99 nodes (20-30 nodes in a cluster) in the by 35%, P90 (200-300 nodes) and Avg (across all nodes) by 10% when there are failed nodes in the cluster.

Resolves #2122

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.25%. Comparing base (0999007) to head (c78a420).
Report is 3 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2154      +/-   ##
============================================
- Coverage     71.44%   71.25%   -0.19%     
============================================
  Files           123      123              
  Lines         67122    67139      +17     
============================================
- Hits          47955    47842     -113     
- Misses        19167    19297     +130     

Files with missing lines	Coverage Δ
src/cluster_legacy.c	86.94% <100.00%> (+0.21%)	⬆️
src/server.c	88.09% <100.00%> (+<0.01%)	⬆️
src/server.h	100.00% <ø> (ø)

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[hpatro]

Could you also add unit tests for the new dictionary API(s)?

Consider rehashing, entry addition/removal during the iteration.

[sungming2]

It might be minor improvement, but is it necessary to maintain two iterations in the cron?

valkey/src/cluster_legacy.c

Lines 5396 to 5407 in 2287261

	di = dictGetSafeIterator(server.cluster->nodes);
	while ((de = dictNext(di)) != NULL) {
	clusterNode *node = dictGetVal(de);
	/* We free the inbound or outboud link to the node if the link has an
	* oversized message send queue and immediately try reconnecting. */
	clusterNodeCronFreeLinkOnBufferLimitReached(node);
	/* The protocol is that function(s) below return non-zero if the node was
	* terminated.
	*/
	if (clusterNodeCronHandleReconnect(node, now)) continue;
	}
	dictReleaseIterator(di);

valkey/src/cluster_legacy.c

Lines 5443 to 5444 in 2287261

	di = dictGetSafeIterator(server.cluster->nodes);
	while ((de = dictNext(di)) != NULL) {

[hpatro]

Just thought about this @sarthakaggarwal97, let me know if you've already explored this and tested.

Could we make the diff smaller by removing the random iterator part and just introduce the delay in attempting reconnection. I think that should solve the issue. As during steady state iterating over all the entries doesn't lead to any regression and there is no syscall involved.

[sarthakaggarwal97]

Sharing the benchmark numbers with the latest change when I kill 450 out of 1000 primaries. The P99 is still improved by a 75% (best case) while avg and P90 still see 10% improvement.

[hpatro]

Thanks for simplifying it @sarthakaggarwal97, looks good to me. The logic is dynamic based on timeout value, each node will attempt reconnection after every 750 ms for the default config value i.e. attempt 10 connection retries within pfail state to fail (if all agree).

Some more explanation:

For the default cluster node timeout period (15 seconds), we try disconnecting and reconnecting at half of that interval which is at 7.5 seconds. With the current unstable, after we treat the node as pfail, we disconnect the link and then retry connecting at each cron cycle i.e. 75 times (7.5 seconds / 100 ms). With this change, we introduce a delay between the connection attempts.

This change avoids too many connection attempts being made in case of large no. of node failures in a cluster which caused high CPU utilization and spikes.

[hpatro]

@enjoy-binbin / @madolson If either of you could take a look at this? that would be great.

[madolson]

The new strategy seems pretty smart, I like it!

[enjoy-binbin]

LGTM.

[hpatro]

Don't see any outstanding comments left. Will merge this in, after the test runs.