[bitnami/redis] PreStop Hook to Initiate Failover for Sentinel on PodTermination #5528

0x46616c6b · 2021-02-17T13:03:40Z

Description of the change

We have encountered problems with Redis Sentinel and Rolling Upgrades. In this case, the Master Election takes several minutes, which ensures that the service does not work correctly during that time.
Therefore, we implemented a PreStop hook that checks if the pod is the master and initiates a failover.

Benefits

Service is useable during a Rolling Upgrade
Rolling Upgrades are much faster

Possible drawbacks

Covers only the case that the Pod receives a SIGTERM

Applicable issues

#5418
#5400

Checklist

Chart version bumped in Chart.yaml according to semver.
Title of the PR starts with chart name (e.g. [bitnami/chart])

juan131

Thanks so much for this great contribution! @miguelaeh @rafariossaa could you please take a look too?

bitnami/redis/templates/configmap-scripts.yaml

Co-authored-by: Juan Ariza Toledano <[email protected]>

bitnami/redis/templates/configmap-scripts.yaml

miguelaeh · 2021-02-18T10:07:41Z

bitnami/redis/templates/configmap-scripts.yaml

+    fi
+
+    if is_boolean_yes "$REDIS_SENTINEL_TLS_ENABLED"; then
+        sentinel_info_command="redis-cli {{- if .Values.usePassword }} -a $REDIS_PASSWORD {{- end }} -h $REDIS_SERVICE -p {{ .Values.sentinel.port }} --tls --cert ${REDIS_SENTINEL_TLS_CERT_FILE} --key ${REDIS_SENTINEL_TLS_KEY_FILE} --cacert ${REDIS_SENTINEL_TLS_CA_FILE} sentinel get-master-addr-by-name {{ .Values.sentinel.masterSet }}"


Suggested change

sentinel_info_command="redis-cli {{- if .Values.usePassword }} -a $REDIS_PASSWORD {{- end }} -h $REDIS_SERVICE -p {{ .Values.sentinel.port }} --tls --cert ${REDIS_SENTINEL_TLS_CERT_FILE} --key ${REDIS_SENTINEL_TLS_KEY_FILE} --cacert ${REDIS_SENTINEL_TLS_CA_FILE} sentinel get-master-addr-by-name {{ .Values.sentinel.masterSet }}"

sentinel_info_command="redis-cli {{- if .Values.usePassword }} -a ${REDIS_PASSWORD} {{- end }} -h ${REDIS_SERVICE} -p {{ .Values.sentinel.port }} --tls --cert ${REDIS_SENTINEL_TLS_CERT_FILE} --key ${REDIS_SENTINEL_TLS_KEY_FILE} --cacert ${REDIS_SENTINEL_TLS_CA_FILE} sentinel get-master-addr-by-name {{ .Values.sentinel.masterSet }}"

Also, using -a will show a warning becuase of using the password in the command, it would be better to set it with the env var REDISCLI_AUTH

Is there an example? The other scripts in the ConfigMap works exactly the same way.

The command itelf is correct, but there could be issues with variables expansions, that's why they should be in the way ${XXX} not $XXX

miguelaeh · 2021-02-18T10:08:18Z

bitnami/redis/templates/configmap-scripts.yaml

+    if is_boolean_yes "$REDIS_SENTINEL_TLS_ENABLED"; then
+        sentinel_info_command="redis-cli {{- if .Values.usePassword }} -a $REDIS_PASSWORD {{- end }} -h $REDIS_SERVICE -p {{ .Values.sentinel.port }} --tls --cert ${REDIS_SENTINEL_TLS_CERT_FILE} --key ${REDIS_SENTINEL_TLS_KEY_FILE} --cacert ${REDIS_SENTINEL_TLS_CA_FILE} sentinel get-master-addr-by-name {{ .Values.sentinel.masterSet }}"
+      else
+        sentinel_info_command="redis-cli {{- if .Values.usePassword }} -a $REDIS_PASSWORD {{- end }} -h $REDIS_SERVICE -p {{ .Values.sentinel.port }} sentinel get-master-addr-by-name {{ .Values.sentinel.masterSet }}"


Suggested change

sentinel_info_command="redis-cli {{- if .Values.usePassword }} -a $REDIS_PASSWORD {{- end }} -h $REDIS_SERVICE -p {{ .Values.sentinel.port }} sentinel get-master-addr-by-name {{ .Values.sentinel.masterSet }}"

sentinel_info_command="redis-cli {{- if .Values.usePassword }} -a ${REDIS_PASSWORD} {{- end }} -h ${REDIS_SERVICE} -p {{ .Values.sentinel.port }} sentinel get-master-addr-by-name {{ .Values.sentinel.masterSet }}"

bitnami/redis/templates/configmap-scripts.yaml

miguelaeh

Thank you for the PR!
Please take a look at the comments

bitnami/redis/Chart.yaml

rafariossaa · 2021-02-18T10:18:01Z

Thanks for the PR!
I have nothing to add to the current review done.
The logic LGTM.

Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

juan131

LGTM

miguelaeh

Hi,
It seems you forgot to implement two of the suggestions, I have unresolved them, could you take a look?
Also, I just notice the Redis service is using template instead of include, added a comment below.

bitnami/redis/templates/configmap-scripts.yaml

Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

miguelaeh

Hi @0x46616c6b ,
Please check the comments again, you changed two lines that were correct.
There is a difference between "$VARIABLE" note the there are not {} and "substring${VARIABLE}substring2" note the {}

bitnami/redis/templates/configmap-scripts.yaml

Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

miguelaeh

Hi @0x46616c6b,
Sorry to be picky with this, I think those are the last needed changes. I tried to edit the files myself but I don't have permissions to do it. Did you allow the file editing by maintainers?

bitnami/redis/templates/configmap-scripts.yaml

Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

0x46616c6b · 2021-02-24T10:05:37Z

Thanks for the good review and your patience. Yes, you can edit the PR or need you some permissions from me?

miguelaeh

Thank you very much for addressing the changes!
LGTM!

About the changes, I can't edit the files on this PR, please rebase master to able to merge it

…Termination (bitnami#5528) * add prestop hook for redis sentinel to failover the master * Update bitnami/redis/templates/configmap-scripts.yaml Co-authored-by: Juan Ariza Toledano <[email protected]> * fix indentation * remove duplicated line * Apply suggestions from code review Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]> * Update bitnami/redis/templates/configmap-scripts.yaml Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]> * Apply suggestions from code review * Apply suggestions from code review Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]> * Apply suggestions from code review Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]> Co-authored-by: Juan Ariza Toledano <[email protected]> Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

srueg · 2021-04-12T12:53:17Z

bitnami/redis/templates/configmap-scripts.yaml

+      if is_boolean_yes "$REDIS_SENTINEL_TLS_ENABLED"; then
+        redis-cli {{- if .Values.usePassword }} -a "$REDIS_PASSWORD" {{- end }} -h "$REDIS_SERVICE" -p {{ .Values.sentinel.port }} --tls --cert "$REDIS_SENTINEL_TLS_CERT_FILE" --key "$REDIS_SENTINEL_TLS_KEY_FILE" --cacert "$REDIS_SENTINEL_TLS_CA_FILE" sentinel failover mymaster
+      else
+        redis-cli {{- if .Values.usePassword }} -a "$REDIS_PASSWORD" {{- end }} -h "$REDIS_SERVICE" -p {{ .Values.sentinel.port }} sentinel failover mymaster


Shouldn't this use the configured {{ .Values.sentinel.masterSet }} instead of hard-coding mymaster?

miguelaeh · 2021-04-13T08:54:22Z

Hi guys, thank you for noticing this issue and opening a PR to fix it! We are taking care of it.

This improves on bitnami#5528 by checking and waiting until the failover is finished on both the redis and the sentinel container. This completely eliminates momentary service interruption during rollouts. As we cannot guarantee the failover will be successful the wait time is capped by the termination grace period - 10s.

…rruption (#6080) * Wait until failover finishes during master pod shutdown This improves on #5528 by checking and waiting until the failover is finished on both the redis and the sentinel container. This completely eliminates momentary service interruption during rollouts. As we cannot guarantee the failover will be successful the wait time is capped by the termination grace period - 10s. * Separate terminationGracePeriod setings for each pod type * make the use of REDISCLI_AUTH clear * [bitnami/redis] Update components versions Signed-off-by: Bitnami Containers <[email protected]> Co-authored-by: Bitnami Containers <[email protected]>

add prestop hook for redis sentinel to failover the master

ff91154

juan131 reviewed Feb 18, 2021

View reviewed changes

bitnami/redis/templates/configmap-scripts.yaml Outdated Show resolved Hide resolved

bitnami/redis/templates/configmap-scripts.yaml Outdated Show resolved Hide resolved

bitnami/redis/templates/configmap-scripts.yaml Outdated Show resolved Hide resolved

0x46616c6b and others added 3 commits February 18, 2021 10:56

Update bitnami/redis/templates/configmap-scripts.yaml

6a1e8e6

Co-authored-by: Juan Ariza Toledano <[email protected]>

fix indentation

7a2b255

remove duplicated line

bb99391

miguelaeh suggested changes Feb 18, 2021

View reviewed changes

miguelaeh reviewed Feb 18, 2021

View reviewed changes

miguelaeh suggested changes Feb 18, 2021

View reviewed changes

bitnami/redis/Chart.yaml Outdated Show resolved Hide resolved

Apply suggestions from code review

5cf5f55

Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

0x46616c6b requested a review from miguelaeh February 18, 2021 12:12

juan131 approved these changes Feb 19, 2021

View reviewed changes

miguelaeh suggested changes Feb 19, 2021

View reviewed changes

bitnami/redis/templates/configmap-scripts.yaml Outdated Show resolved Hide resolved

0x46616c6b and others added 2 commits February 22, 2021 13:42

Update bitnami/redis/templates/configmap-scripts.yaml

0171145

Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

Apply suggestions from code review

9fdfa42

miguelaeh suggested changes Feb 23, 2021

View reviewed changes

bitnami/redis/templates/configmap-scripts.yaml Outdated Show resolved Hide resolved

bitnami/redis/templates/configmap-scripts.yaml Outdated Show resolved Hide resolved

0x46616c6b and others added 2 commits February 23, 2021 10:31

Apply suggestions from code review

510fbbe

Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

Merge branch 'master' into redis-sentinel-prestop-hook

110f62a

miguelaeh suggested changes Feb 24, 2021

View reviewed changes

bitnami/redis/templates/configmap-scripts.yaml Outdated Show resolved Hide resolved

bitnami/redis/templates/configmap-scripts.yaml Outdated Show resolved Hide resolved

0x46616c6b and others added 2 commits February 24, 2021 11:04

Apply suggestions from code review

318987b

Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

Merge branch 'master' into redis-sentinel-prestop-hook

bf08067

miguelaeh approved these changes Feb 25, 2021

View reviewed changes

Merge branch 'master' into redis-sentinel-prestop-hook

beaf9f7

miguelaeh merged commit 17271c2 into bitnami:master Feb 26, 2021

0x46616c6b deleted the redis-sentinel-prestop-hook branch March 2, 2021 10:52

srueg reviewed Apr 12, 2021

View reviewed changes

Gregy mentioned this pull request Apr 12, 2021

[bitnami/redis] Improve sentinel prestop hook to prevent service interruption #6080

Merged

3 tasks

GMartinez-Sisti mentioned this pull request Apr 14, 2021

redis + sentinel master pod reschedule / deletion results in two masters #5543

Closed

simon-baatz mentioned this pull request Sep 20, 2022

[bitnami/redis] sentinel failover in prestop hook causes write loss #12598

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bitnami/redis] PreStop Hook to Initiate Failover for Sentinel on PodTermination #5528

[bitnami/redis] PreStop Hook to Initiate Failover for Sentinel on PodTermination #5528

0x46616c6b commented Feb 17, 2021

juan131 left a comment

miguelaeh Feb 18, 2021

0x46616c6b Feb 19, 2021

miguelaeh Feb 22, 2021

miguelaeh Feb 18, 2021

miguelaeh left a comment

rafariossaa commented Feb 18, 2021

juan131 left a comment

miguelaeh left a comment •

edited

Loading

miguelaeh left a comment

miguelaeh left a comment

0x46616c6b commented Feb 24, 2021 •

edited

Loading

miguelaeh left a comment •

edited

Loading

srueg Apr 12, 2021 •

edited

Loading

miguelaeh commented Apr 13, 2021

	sentinel_info_command="redis-cli {{- if .Values.usePassword }} -a $REDIS_PASSWORD {{- end }} -h $REDIS_SERVICE -p {{ .Values.sentinel.port }} --tls --cert ${REDIS_SENTINEL_TLS_CERT_FILE} --key ${REDIS_SENTINEL_TLS_KEY_FILE} --cacert ${REDIS_SENTINEL_TLS_CA_FILE} sentinel get-master-addr-by-name {{ .Values.sentinel.masterSet }}"
	sentinel_info_command="redis-cli {{- if .Values.usePassword }} -a ${REDIS_PASSWORD} {{- end }} -h ${REDIS_SERVICE} -p {{ .Values.sentinel.port }} --tls --cert ${REDIS_SENTINEL_TLS_CERT_FILE} --key ${REDIS_SENTINEL_TLS_KEY_FILE} --cacert ${REDIS_SENTINEL_TLS_CA_FILE} sentinel get-master-addr-by-name {{ .Values.sentinel.masterSet }}"

[bitnami/redis] PreStop Hook to Initiate Failover for Sentinel on PodTermination #5528

[bitnami/redis] PreStop Hook to Initiate Failover for Sentinel on PodTermination #5528

Conversation

0x46616c6b commented Feb 17, 2021

juan131 left a comment

Choose a reason for hiding this comment

miguelaeh Feb 18, 2021

Choose a reason for hiding this comment

0x46616c6b Feb 19, 2021

Choose a reason for hiding this comment

miguelaeh Feb 22, 2021

Choose a reason for hiding this comment

miguelaeh Feb 18, 2021

Choose a reason for hiding this comment

miguelaeh left a comment

Choose a reason for hiding this comment

rafariossaa commented Feb 18, 2021

juan131 left a comment

Choose a reason for hiding this comment

miguelaeh left a comment • edited Loading

Choose a reason for hiding this comment

miguelaeh left a comment

Choose a reason for hiding this comment

miguelaeh left a comment

Choose a reason for hiding this comment

0x46616c6b commented Feb 24, 2021 • edited Loading

miguelaeh left a comment • edited Loading

Choose a reason for hiding this comment

srueg Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

miguelaeh commented Apr 13, 2021

miguelaeh left a comment •

edited

Loading

0x46616c6b commented Feb 24, 2021 •

edited

Loading

miguelaeh left a comment •

edited

Loading

srueg Apr 12, 2021 •

edited

Loading