[-] retry failed etcd re-sync until etcd answers, closes #431 - #432
Closed
pashagolub wants to merge 8 commits into
Closed
pashagolub wants to merge 8 commits into
pashagolub wants to merge 8 commits into
Conversation
A single failed read after a lost watch (or at startup) left the state at false, and the healthy re-armed watch stays silent while the leader key is unchanged, so the VIP never came back until a restart.
go.mod requires go 1.26.7, but `go-version: '1.26'` picked the runner's cached go 1.26.4, and GOTOOLCHAIN=local refused to build.
|
coverage: 91.531% (+0.1%) from 91.434% — fix-etcd-resync-retry into master |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Synchronization can race, and the new tests need cancellation-and-wait cleanup.
Review effort: Lite
Findings: None
What changed in this PR
Improves etcd leader-state recovery by retrying failed reads during startup and watch re-synchronization.
Changes:
- Adds retrying synchronization logic.
- Adds regression tests for failed reads.
- Derives the CI Go version from
go.mod.
| File | Description |
|---|---|
checker/etcd_leader_checker.go |
Implements retrying etcd synchronization. |
checker/etcd_leader_checker_test.go |
Adds startup and re-sync retry tests. |
.github/workflows/build.yml |
Uses the module’s declared Go version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
pashagolub
commented
Sep 25, 2026
GetChangeNotificationStream ran the initial read in its own goroutine next to the watch. A read that returned the old leader could send false after the watch had already sent true for a newer leader change, and the VIP stayed down until the key changed again (#431). Now the read runs first and the watch starts at the next revision, so every send comes from one goroutine and later events always win. Watch() blocks while etcd is unreachable, so reading first also keeps false flowing during an outage at startup.
The retry tests cancelled their goroutines without waiting, so cleanup could close the client and the container while those goroutines still ran. EmitsOnConnectionError looped forever once its context expired because its break only left the select, so a regression hung the run for 10 minutes instead of failing.
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #431
Problem
When the etcd watch is lost, the checker re-arms it and re-reads the leader key once via
get(). If that single read fails (e.g.context deadline exceededwhile etcd is overloaded),falseis sent and the VIP is removed. The re-armed watch is healthy and silent as long as the leader key does not change, and Patroni renews leadership via a lease without rewriting the key, so the VIP never comes back until a restart or a failover. The initial read inGetChangeNotificationStreamhad the same one-shot flaw.Change
get()now reports whether etcd answered (an absent key counts as an answer).sync()retriesget()every second until etcd answers or the context is done.sync()is used for the startup read and for the re-sync after a lost watch.The demotion behaviour is unchanged: every failed read still emits
false, as before. Only recovery changed.Tests
TestEtcdLeaderChecker_watch_RetriesFailedResync: first watch dead, later watches healthy, first two reads fail;truemust come back.TestEtcdLeaderChecker_GetChangeNotificationStream_RetriesFailedInitialGet: initial read fails twice;truemust come back.Both fail on the previous code and pass with the fix; full suite passes.
Note for reviewers
Out of scope, not changed here: a hung etcd (tested by
docker pausefor 25s) never makes the watch report an error, so the VIP is not removed in that case — neither before nor after this PR. Closing that gap for #354 would need a periodic read next to the watch; worth a separate discussion because it trades against VIP flapping on an overloaded etcd.