Skip to content

Conversation

sobotklp
Copy link

@sobotklp sobotklp commented Aug 28, 2025

Description of the change

This is a fix for #23036

It implements graceful failover for terminations of Redis Cluster master pods. This will limit the impact of terminations and failover events, since Redis cluster clients will have the chance to update their topology before the previous pod terminates. This will improve uptime during planned maintenance events.

Benefits

Improved uptime by failing master pods over to other replicas.

Possible drawbacks

If PodDisruptionBudget isn't being used, it may initiate a failover to a node that is also about to also be terminated. The script will attempt to filter out replicas that aren't candidates for promotion.

Applicable issues

Additional information

This is an attempt to mirror the graceful failover behaviour of the redis and valkey charts when using Sentinel.

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

@github-actions github-actions bot added redis-cluster triage Triage is needed labels Aug 28, 2025
@github-actions github-actions bot requested a review from carrodher August 28, 2025 16:28
@sobotklp sobotklp force-pushed the redis-cluster-prestop branch 2 times, most recently from 75ff4fa to a83c273 Compare August 28, 2025 16:32
@carrodher carrodher changed the title [bitnami/redis-cluster]: add preStop hook that gracefully fails over … [bitnami/redis-cluster]: add preStop hook that gracefully fails over master nodes on pod termination Aug 28, 2025
@carrodher
Copy link
Member

Thank you for initiating this pull request. We appreciate your effort. This is just a friendly reminder that signing your commits is important. Your signature certifies that you either authored the patch or have the necessary rights to contribute to the changes. You can find detailed information on how to do this in the “Sign your work” section of our contributing guidelines.

Feel free to reach out if you have any questions or need assistance with the signing process.

@sobotklp sobotklp force-pushed the redis-cluster-prestop branch from ffefc85 to 6d09087 Compare August 28, 2025 16:58
sobotklp and others added 2 commits September 4, 2025 19:59
…master nodes on pod termination

Signed-off-by: Lewis Sobotkiewicz <[email protected]>
Signed-off-by: Bitnami Bot <[email protected]>
@sobotklp sobotklp force-pushed the redis-cluster-prestop branch from c0cbf9b to 59f9262 Compare September 5, 2025 02:00
@sobotklp
Copy link
Author

sobotklp commented Sep 5, 2025

Thank you for initiating this pull request. We appreciate your effort. This is just a friendly reminder that signing your commits is important. Your signature certifies that you either authored the patch or have the necessary rights to contribute to the changes. You can find detailed information on how to do this in the “Sign your work” section of our contributing guidelines.

Feel free to reach out if you have any questions or need assistance with the signing process.

Hi @carrodher . Thanks for the tips on signing my commits. I've rebased my commit and amended my change with my signature. Let me know if I can do anything else. :)

bitnami-bot and others added 2 commits September 5, 2025 02:05
Signed-off-by: Bitnami Bot <[email protected]>
Signed-off-by: Lewis Sobotkiewicz <[email protected]>
@sobotklp
Copy link
Author

sobotklp commented Sep 5, 2025

The script uses this basic logic on pod termination:

  • If the node is a master node, then
    • Get the node ID
    • List the available replicas for the node ID
    • Select a replica from the list and attempt to call CLUSTER FAILOVER.

I tested this by deploying a 9-node, 3-shard cluster locally without authentication or TLS, then manually terminating master nodes.
I could see that the commands ROLE and CLUSTER MYID were called on the terminating master node, as expected.
On the target replicas, I saw the expected output in the logs:

 * Manual failover user request accepted.
 * Received replication offset for paused master manual failover: 1148
 * All master replication stream processed, manual failover can start.
 * Start of election delayed for 0 milliseconds (rank #0, offset 1148).
 * Starting a failover election for epoch 11.

@sobotklp
Copy link
Author

sobotklp commented Sep 5, 2025

The script uses this basic logic on shutdown:

  • If the node is a master node, then
    • Get the node ID
    • List the available replicas for the node ID
    • Select a replica from the list and attempt to call CLUSTER FAILOVER.

I tested this by deploying a 9-node, 3-shard cluster locally without authentication or TLS, then manually terminating master nodes. I could see that the commands ROLE and CLUSTER MYID were called on the terminating master node, as expected. On the target replicas, I saw the expected output in the logs:

 * Manual failover user request accepted.
 * Received replication offset for paused master manual failover: 1148
 * All master replication stream processed, manual failover can start.
 * Start of election delayed for 0 milliseconds (rank #0, offset 1148).
 * Starting a failover election for epoch 11.

It's also possible to kubectl exec -it -- bash into a master node and run the script manually, initiating a manual failover without terminating the pod.

@javsalgar javsalgar added verify Execute verification workflow for these changes in-progress labels Sep 5, 2025
@github-actions github-actions bot removed the triage Triage is needed label Sep 5, 2025
@github-actions github-actions bot removed the request for review from carrodher September 5, 2025 06:53
@github-actions github-actions bot requested a review from migruiz4 September 5, 2025 06:53
if [[ "$result" == "OK" ]]; then
{{- if .Values.cluster.redisShutdownWaitFailover }}
# Wait for clients to update their topology
sleep 10

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make this configurable instead of fixed 10 seconds?

Copy link
Author

@sobotklp sobotklp Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be configurable up to a maximum of {{- $.Values.redis.terminationGracePeriodSeconds }} I would think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this to wait $.Values.redis.terminationGracePeriodSeconds - 10 seconds

mapfile -t REPLICA_IPS < <( get_replica_ips )
NUM_REPLICAS=${#REPLICA_IPS[@]}
echo "Found $NUM_REPLICAS available replicas"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should cover 0 replicas found. But maybe its better to add a warning message?
No replica found for failover; proceeding with shutdown

Copy link

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution.

@github-actions github-actions bot added the stale 15 days without activity label Sep 21, 2025
@sobotklp
Copy link
Author

@migruiz4 would you like me to do anything more here to validate this PR?

Signed-off-by: Bitnami Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-progress redis-cluster stale 15 days without activity verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bitnami/redis-cluster] Request failover when a master node gracefully shuts down
6 participants