Mark primary node as alive immediately if reachable and failover is not possible #1927

hpatro · 2025-04-07T23:14:01Z

Mark primary node as alive immediately if reachable and failover is not possible

Added test case for failover explicity disabled via cluster-replica-no-failover. Currently, we wait for cluster_node_timeout * 2 period to mark a failed primary as alive if we are able to communicate with it. For scenario, where a failover won't get triggered, we can mark it as immediately available for better availability.

Before

[ok]: no failover - verify replica is not promoted if failover has been disabled (6006 ms)
[ok]: no failover - primary is in failed state (123 ms)
[ok]: no failover - cluster is in healthy state (10138 ms)

After

[ok]: no failover - verify replica is not promoted if failover has been disabled (5863 ms)
[ok]: no failover - primary is in failed state (120 ms)
[ok]: no failover - cluster is in healthy state (1 ms)

The last test no failover - cluster is in healthy state showcases the cluster state reached to ok after 1ms (with this change) compared to 10ms (unstable) where cluster node timeout is set to 5 ms.

Signed-off-by: Harkrishn Patro <[email protected]>

codecov · 2025-04-07T23:30:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.06%. Comparing base (204097d) to head (a26e140).
Report is 6 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1927      +/-   ##
============================================
- Coverage     71.07%   71.06%   -0.02%     
============================================
  Files           123      123              
  Lines         65683    65778      +95     
============================================
+ Hits          46687    46743      +56     
- Misses        18996    19035      +39

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

... and 17 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.

madolson · 2025-04-08T02:12:26Z

src/cluster_legacy.c

@@ -2146,12 +2146,21 @@ void clearNodeFailureIfNeeded(clusterNode *node) {
        clusterDoBeforeSleep(CLUSTER_TODO_UPDATE_STATE | CLUSTER_TODO_SAVE_CONFIG);
    }

+    /* If any of the replica of a given primary can't failover, then immediately mark it as alive. */
+    int cant_failover = 1;


Is this a common enough failure mode to have special handling for it? Most people only temporarily disable failover when doing upgrades and they probably shouldn't disable it on all replicas. It's also possible there is another replica we are unaware of that is eligible.

I looked at the request behind this feature. Seems like user like to place replicas closer to the application for faster reads but don't want it to be part of the cluster failover process. The other use case mentioned is replica used only for backup reasons. Few others don't want automated failover to kick in and only perform the failover(s) manually. All of this documented here: redis/redis#3021

Also I believe with this change we make availability better for cluster with primary only setup. We are accelerating availability where it seems feasible and in the worst case I believe it was always possible to enter a partitioned state and have multiple primaries in a shard.

I agree with this change. ultimately, the truth lies with the primary in question. whether or not a bystander node like this one marks the primary healthy immediately doesn't really change the fact that the primary will regain its primaryship after a flash restart. So the more important thing is the old primary staying down for 2 times cluster-node-timeout; it's less about the observer marking it alive immediately. this approach amplifies an existing problem a bit to trade consistency for availability, which I think is a reasonable decision, provided we are reasonably sure that there won't be replicas competing for the primaryship.

we have a zoneid in the internal fork (and we will gossip it), usually replicas in the same az as the primary node will have a better ranking. Of course, replication offset is the top priority. When the offsets are the same, the replicas of the same az have better rank and can initiate elections faster. We almost never use the no-failover configuration option.

@madolson and I were talking about the node placement with zonal awareness thing and we can also improve the voting members placement and avoid placing all of them in same zone. Let me file an issue on this.

PingXie · 2025-04-08T17:16:40Z

src/cluster_legacy.c

@@ -2146,12 +2146,21 @@ void clearNodeFailureIfNeeded(clusterNode *node) {
        clusterDoBeforeSleep(CLUSTER_TODO_UPDATE_STATE | CLUSTER_TODO_SAVE_CONFIG);
    }

+    /* If any of the replica of a given primary can't failover, then immediately mark it as alive. */
+    int cant_failover = 1;


I agree with this change. ultimately, the truth lies with the primary in question. whether or not a bystander node like this one marks the primary healthy immediately doesn't really change the fact that the primary will regain its primaryship after a flash restart. So the more important thing is the old primary staying down for 2 times cluster-node-timeout; it's less about the observer marking it alive immediately. this approach amplifies an existing problem a bit to trade consistency for availability, which I think is a reasonable decision, provided we are reasonably sure that there won't be replicas competing for the primaryship.

src/cluster_legacy.c

tests/unit/cluster/failover2.tcl

Signed-off-by: Harkrishn Patro <[email protected]>

enjoy-binbin

Make sense to me.

src/cluster_legacy.c

enjoy-binbin · 2025-04-09T03:31:24Z

src/cluster_legacy.c

+            break;
+        }
+    }
+
    /* If it is a primary and...
     * 1) The FAIL state is old enough.


let's also update this line to mention the dont_wait.

enjoy-binbin · 2025-04-09T03:32:00Z

src/cluster_legacy.c

@@ -4827,14 +4841,19 @@ void clusterHandleReplicaFailover(void) {
     * 3) We don't have the no failover configuration set, and this is


this line need an update.

Signed-off-by: Binbin <[email protected]>

Mark primary as alive if reachable and failover is disabled

58c8d13

Signed-off-by: Harkrishn Patro <[email protected]>

hpatro added the cluster label Apr 7, 2025

hpatro requested review from enjoy-binbin and zuiderkwast April 7, 2025 23:49

madolson reviewed Apr 8, 2025

View reviewed changes

zuiderkwast requested a review from PingXie April 8, 2025 10:50

PingXie reviewed Apr 8, 2025

View reviewed changes

Add tests for primary only cluster

a153ce2

Signed-off-by: Harkrishn Patro <[email protected]>

hpatro changed the title ~~Mark primary node as alive immediately if reachable and failover is disabled~~ Mark primary node as alive immediately if reachable and failover is not possible Apr 8, 2025

Rename variable

fae539b

Signed-off-by: Harkrishn Patro <[email protected]>

enjoy-binbin reviewed Apr 9, 2025

View reviewed changes

Update src/cluster_legacy.c

a26e140

Signed-off-by: Binbin <[email protected]>

hpatro mentioned this pull request Apr 9, 2025

[NEW] Propagate node availability-zone information across cluster and make failover smarter #1937

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark primary node as alive immediately if reachable and failover is not possible #1927

Mark primary node as alive immediately if reachable and failover is not possible #1927

hpatro commented Apr 7, 2025 •

edited

Loading

codecov bot commented Apr 7, 2025 •

edited

Loading

madolson Apr 8, 2025

hpatro Apr 8, 2025

PingXie Apr 8, 2025

enjoy-binbin Apr 9, 2025 •

edited

Loading

hpatro Apr 9, 2025 •

edited

Loading

PingXie Apr 8, 2025

enjoy-binbin left a comment

enjoy-binbin Apr 9, 2025

enjoy-binbin Apr 9, 2025

		@@ -4827,14 +4841,19 @@ void clusterHandleReplicaFailover(void) {
		* 3) We don't have the no failover configuration set, and this is

Mark primary node as alive immediately if reachable and failover is not possible #1927

Are you sure you want to change the base?

Mark primary node as alive immediately if reachable and failover is not possible #1927

Conversation

hpatro commented Apr 7, 2025 • edited Loading

codecov bot commented Apr 7, 2025 • edited Loading

Codecov Report

madolson Apr 8, 2025

Choose a reason for hiding this comment

hpatro Apr 8, 2025

Choose a reason for hiding this comment

PingXie Apr 8, 2025

Choose a reason for hiding this comment

enjoy-binbin Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

hpatro Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

PingXie Apr 8, 2025

Choose a reason for hiding this comment

enjoy-binbin left a comment

Choose a reason for hiding this comment

enjoy-binbin Apr 9, 2025

Choose a reason for hiding this comment

enjoy-binbin Apr 9, 2025

Choose a reason for hiding this comment

hpatro commented Apr 7, 2025 •

edited

Loading

codecov bot commented Apr 7, 2025 •

edited

Loading

enjoy-binbin Apr 9, 2025 •

edited

Loading

hpatro Apr 9, 2025 •

edited

Loading