Skip to content

Sentinel: re-elect primary on failover between already-known nodes#3130

Open
rebaseandpanic wants to merge 1 commit into
StackExchange:mainfrom
rebaseandpanic:pr/sentinel-main
Open

Sentinel: re-elect primary on failover between already-known nodes#3130
rebaseandpanic wants to merge 1 commit into
StackExchange:mainfrom
rebaseandpanic:pr/sentinel-main

Conversation

@rebaseandpanic

Copy link
Copy Markdown

On an in-place Sentinel failover (the current primary is demoted to a replica
without changing endpoint, and a known replica is promoted), the client never
switches to the new primary and keeps writing to the demoted node until restart.
Seen in production on a Sentinel-managed Valkey deployment on Kubernetes (pods
not recreated): a Redis Streams consumer stayed pinned to the demoted node for
hours until the process was restarted.

SwitchPrimary only reconfigured when the new primary was an endpoint it had
never seen (!servers.Contains(...)). When the promoted node is already known,
that guard is false and it returns without re-electing. A plain reconfigure also
isn't enough: with reconfigureAll: false an already-connected node doesn't
re-read its role, so the election just re-picks the stale node.

Fix: when a known primary's cached view is stale, reconfigure with
reconfigureAll: true so connected nodes re-read their role first (the guard is
self-clearing — no reconfigure storm). Also run that reconfigure outside the
sentinelConnectionChildren lock so one service's failover doesn't block others.

Refs #1891 — the failure logs there show this known-endpoint failover, despite
the title mentioning IP changes; the separate new-primary-IP path is untouched.

SwitchPrimary only reconfigured when the new primary was an unknown
endpoint. On an in-place failover (old master demoted, replica promoted),
the new master is already in `servers`, so it no-op'd and the client kept
writing to the demoted node until restart.

Reconfigure with reconfigureAll:true when a known primary's cached role is
stale, so connected nodes re-read their role before re-election. Also move
the switch reconfigure out of the sentinelConnectionChildren lock.

Refs StackExchange#1891
@mgravell

Copy link
Copy Markdown
Collaborator

any preference between #3130 and #3131?

@rebaseandpanic

Copy link
Copy Markdown
Author

I noticed you keep both a v2 and a v3/main line, so I figured a PR into each made sense (#3130 → main, #3131 → v2) — it's the exact same commit, cherry-picked, so they're identical.

I opened the v2 one specifically because we're running the 2.x line in production and hit this bug in the wild there, so having the fix land in v2 matters to us directly — not just main.

That said, I see contributions here normally target main only, so if you'd prefer to handle the v2 backport yourself, I'm happy to close #3131 and keep just #3130. Whatever's easiest for you.

@mgravell

Copy link
Copy Markdown
Collaborator

ah, I hadn't noticed that branch detail! sorry, the cost of reading things in a hurry; I'll try to review, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants