-
Notifications
You must be signed in to change notification settings - Fork 10.2k
[bitnami/redis] Add support to redis master service with useHostnames false #35536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[bitnami/redis] Add support to redis master service with useHostnames false #35536
Conversation
…e and masterService Signed-off-by: Fran Méndez <[email protected]>
Signed-off-by: Fran Méndez <[email protected]>
Signed-off-by: Fran Méndez <[email protected]>
Signed-off-by: Bitnami Bot <[email protected]>
…mes-false Signed-off-by: Carlos Rodríguez Hernández <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this great PR! Absolutely brilliant, please check my comments.
Signed-off-by: Fran Méndez <[email protected]>
…of github.com:fmendez89/charts into redis-support-master-service-with-use-hostnames-false
…service-with-use-hostnames-false
Signed-off-by: Bitnami Bot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ensure you merge the latest changes in the origin's main
branch in your fork's branch and then, update the PR. Thanks in advance.
if [ -f "/etc/shared/current" ]; then | ||
current_master=$(< /etc/shared/current) | ||
[ -f /etc/shared/previous ] && previous_master=$(< /etc/shared/previous) || previous_master="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please split this in 2 lines:
[ -f /etc/shared/previous ] && previous_master=$(< /etc/shared/previous) || previous_master="" | |
previous_master="" | |
[ -f /etc/shared/previous ] && previous_master=$(< /etc/shared/previous) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, it's better that way
echo "Current master: $current_master" | ||
if [ -n "$previous_master" ] && [ "$current_master" = "$previous_master" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the 1st condition redundant?
if [ -n "$previous_master" ] && [ "$current_master" = "$previous_master" ]; then | |
if [ "$current_master" = "$previous_master" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, good catch
echo "Current master: $current_master" | ||
if [ -n "$previous_master" ] && [ "$current_master" = "$previous_master" ]; then | ||
echo "Master ($current_master = $previous_master) has not changed, skipping label update and cleaning state" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous echo
you already displayed the variable value:
echo "Master ($current_master = $previous_master) has not changed, skipping label update and cleaning state" | |
echo "Master has not changed, skipping label update and cleaning state" | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, removing it
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. |
Due to the lack of activity in the last 5 days since it was marked as "stale", we proceed to close this Pull Request. Do not hesitate to reopen it later if necessary. |
…of github.com:fmendez89/charts into redis-support-master-service-with-use-hostnames-false
…-master-service-with-use-hostnames-false
Signed-off-by: Fran Méndez <[email protected]>
Hi @juan131, how can I reopen de PR to push the changes? |
Signed-off-by: Fran Méndez <[email protected]>
Thanks for your contribution! Could you please bump the chart version in the Chart.yaml? This is necessary to test the changes and cut a new release. Probably it was missing after the latest rebase. |
Description of the change
This PR improves the way the current Redis master pod is labeled by Sentinel by introducing synchronization and avoiding race conditions when multiple pods detect a failover. It adds the following changes:
Benefits
Possible drawbacks
Applicable issues
Fixes #29217
Additional information
This feature is particularly useful when using external services that rely on pod labels (e.g., a Service selector for the master role). It ensures the label is always up-to-date based on Sentinel decisions.
Checklist