server: make EtcdStartTimeout a no-progress timeout#11015
Conversation
startEtcd previously wrapped the whole "wait until etcd is ready" in a fixed context deadline of EtcdStartTimeout (5m). A slow-but-healthy startup that keeps catching up its raft log would be killed once the deadline elapsed, and on process restart it would loop forever. Wait for readiness using the etcd applied index as a liveness signal instead: keep waiting as long as the applied index advances, and only give up when there is no apply progress for EtcdStartTimeout. This tells a slow restore apart from a genuine hang (e.g. a removed member that can never rejoin the quorum), which still fails fast. Parent ctx cancellation is still honored for normal shutdown. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/M
📝 WalkthroughWalkthroughEmbedded etcd startup now treats ChangesEmbedded etcd startup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant EmbeddedEtcd
participant initMember
Server->>EmbeddedEtcd: waitEtcdReady(ctx, etcd)
EmbeddedEtcd-->>Server: readiness or startup result
Server->>initMember: initMember(initCtx, etcd)
initMember-->>Server: initialization result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/server.go`:
- Around line 118-121: Update the GoDoc comment associated with the exported
identifier near startEtcd/waitEtcdReady so its first words are the exact
exported identifier name, while preserving the existing explanation of the
no-progress timeout.
- Around line 442-446: Update the startup select in the etcd readiness flow to
monitor etcd.Err() alongside ReadyNotify() and ctx.Done(). When an asynchronous
etcd failure is received, return it using the same error-wrapping pattern as
embed.StartEtcd, while preserving the existing ready and cancellation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Extract the wait loop of waitEtcdReady into waitEtcdReadyProgress, which depends only on a ready channel, an applied-index getter and the two durations, so it can be driven deterministically without a real embedded etcd. Cover the four behaviors: ready immediately, keep waiting while the applied index advances (past what a fixed deadline would allow), give up after no apply progress, and honor ctx cancellation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
| case now := <-ticker.C: | ||
| applied := etcd.Server.AppliedIndex() | ||
| if applied > lastApplied { | ||
| log.Info("etcd is not ready yet but still making apply progress, keep waiting", |
There was a problem hiding this comment.
This logs at Info level on every one-second poll while the applied index advances. The exact startup path this change targets may run for many minutes or hours, producing hundreds or thousands of repetitive production log entries and adding logging overhead during recovery. Please rate-limit the progress message or lower it to Debug.
There was a problem hiding this comment.
Done in ab2cee0 — the progress log is now rate-limited to at most once per 30s (etcdStartProgressLogInterval) instead of one line per 1s poll. Kept at Info so operators can still distinguish a slow recovery from a hang.
ti-chi-bot
Bot
added
size/L
| // EtcdStartTimeout do we give up. This distinguishes a slow-but-healthy startup | ||
| // from a genuine hang, such as a removed member that can never rejoin the | ||
| // quorum. It also honors ctx cancellation for normal shutdown. | ||
| func (s *Server) waitEtcdReady(ctx context.Context, etcd *embed.Etcd) error { |
There was a problem hiding this comment.
Blocking: the *Server receiver is still unused, and the repository revive configuration reports unused-receiver here, so the required static check cannot pass. The newly extracted wrapper still does not depend on any Server state; please make it a package-level function or remove the unused receiver in another idiomatic way.
There was a problem hiding this comment.
Done in ab2cee0 — made waitEtcdReady a package-level function so there is no unused *Server receiver. golangci-lint run ./server/ is now clean (0 issues).
| // from a genuine hang, such as a removed member that can never rejoin the | ||
| // quorum. It also honors ctx cancellation for normal shutdown. | ||
| func (s *Server) waitEtcdReady(ctx context.Context, etcd *embed.Etcd) error { | ||
| return waitEtcdReadyProgress(ctx, etcd.Server.ReadyNotify(), etcd.Server.AppliedIndex, |
There was a problem hiding this comment.
Please also stop waiting when embedded etcd fails or stops before becoming ready. Listener and serving failures are reported through etcd.Err(), while internal EtcdServer.run failures can terminate the server and close etcd.Server.StopNotify() without producing an embed.Etcd.Err() value. Passing only ReadyNotify() into the core means a dead server is still misreported as ErrCancelStartEtcd after the full timeout. Please monitor both signals, preserve an underlying error from etcd.Err() when available, and fail fast on StopNotify().
There was a problem hiding this comment.
Done in ab2cee0 — waitEtcdReadyProgress now also selects on etcd.Server.StopNotify(). When the server stops before becoming ready it does a non-blocking read of etcd.Err() to preserve the underlying cause (wrapped in ErrStartEtcd), falling back to a bare ErrStartEtcd when none was published. Added a unit test for the stop-before-ready path.
| zap.Uint64("last-applied-index", lastApplied), | ||
| zap.Uint64("applied-index", applied)) | ||
| lastApplied = applied | ||
| lastProgress = now |
There was a problem hiding this comment.
now is the scheduled timestamp carried by the ticker event, but appliedIndex() is sampled only when this goroutine handles that event. If a tick remains buffered during a long scheduling or GC pause, this branch can observe fresh progress and record it with a stale time, causing a subsequent tick to declare a timeout immediately. Please use the actual sampling time from time.Now(), or preferably reset a dedicated no-progress timer whenever the applied index advances.
There was a problem hiding this comment.
Done in ab2cee0 — I now sample time.Now() when reading the applied index instead of reusing the ticker`s scheduled timestamp, so a tick buffered during a scheduling/GC pause cannot record progress with a stale time and trip a false no-progress timeout on the next tick.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/etcd_start_test.go (1)
82-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
require.ErrorIsfor sentinel error checks.The coding guidelines mandate using
errors.Is/Asfor sentinel error checks. Utilizing Testify'srequire.ErrorIsinherently delegates toerrors.Is, making the test more idiomatic and providing much clearer failure messages compared to manual boolean evaluation.
server/etcd_start_test.go#L82-L83: replacere.Error(err)andre.True(errors.ErrorEqual(err, errs.ErrCancelStartEtcd))withre.ErrorIs(err, errs.ErrCancelStartEtcd).server/etcd_start_test.go#L99-L100: apply the identical replacement in the cancellation case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/etcd_start_test.go` around lines 82 - 83, Replace the manual error assertions with re.ErrorIs(err, errs.ErrCancelStartEtcd) in both cancellation cases: server/etcd_start_test.go lines 82-83 and 99-100. Remove the separate re.Error and errors.ErrorEqual assertions while preserving the existing sentinel error expectation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/etcd_start_test.go`:
- Around line 90-102: Update the cancellation test around waitEtcdReadyProgress
to defer cancel() immediately after creating the cancellable context, ensuring
cleanup on early failure or panic. Remove the time-based start measurement and
re.Less assertion, while preserving the existing error and ErrCancelStartEtcd
assertions.
---
Nitpick comments:
In `@server/etcd_start_test.go`:
- Around line 82-83: Replace the manual error assertions with re.ErrorIs(err,
errs.ErrCancelStartEtcd) in both cancellation cases: server/etcd_start_test.go
lines 82-83 and 99-100. Remove the separate re.Error and errors.ErrorEqual
assertions while preserving the existing sentinel error expectation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2d606c6-52c0-4e7e-bc76-f20d9c5a5eed
📒 Files selected for processing (2)
server/etcd_start_test.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/server.go
Address review feedback on the etcd start no-progress timeout: - Monitor etcd.Err() while waiting for readiness. If the embedded etcd hits a fatal error in the background before ReadyNotify() closes, the error is now returned (wrapped in ErrStartEtcd) immediately instead of stalling for the whole no-progress timeout and returning a generic error. waitEtcdReadyProgress takes the error channel as a parameter so it stays unit-testable. - Start the EtcdStartTimeout GoDoc with the identifier name. - Add a unit test for the async-error path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11015 +/- ##
=======================================
Coverage 79.22% 79.23%
=======================================
Files 541 541
Lines 75965 76012 +47
=======================================
+ Hits 60187 60231 +44
- Misses 11531 11537 +6
+ Partials 4247 4244 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Follow-up on reviewer feedback for the etcd start no-progress timeout: - Drop the unused *Server receiver on waitEtcdReady (revive unused-receiver, which was failing the static check) by making it a package-level function. - Also monitor etcd.Server.StopNotify(): an internal EtcdServer.run failure can stop the server without publishing on etcd.Err(), which previously got misreported as a no-progress timeout. On stop, preserve the underlying etcd.Err() cause when available. - Sample time.Now() when reading the applied index instead of reusing the ticker's scheduled timestamp, so a tick buffered during a scheduling/GC pause cannot trip a false no-progress timeout. - Rate-limit the progress log to at most once per 30s (a slow recovery can run for many minutes), instead of logging on every poll. - Test: drop the flaky timing upper-bound assertion in the cancellation case, add defer cancel(), and cover the server-stopped path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/etcd_start_test.go (1)
113-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
errors.Isfor sentinel error checks.As per coding guidelines, sentinel error checks should use
errors.Isorerrors.As. Using testify'srequire.ErrorIsinherently applieserrors.Isand provides clearer failure messages thanrequire.Truecombined witherrors.ErrorEqual.
server/etcd_start_test.go#L113-L113: Replacere.True(errors.ErrorEqual(err, errs.ErrCancelStartEtcd))withre.ErrorIs(err, errs.ErrCancelStartEtcd).server/etcd_start_test.go#L130-L130: Replacere.True(errors.ErrorEqual(err, errs.ErrCancelStartEtcd))withre.ErrorIs(err, errs.ErrCancelStartEtcd).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/etcd_start_test.go` at line 113, Update both sentinel checks in server/etcd_start_test.go at lines 113-113 and 130-130 to use testify’s re.ErrorIs(err, errs.ErrCancelStartEtcd) instead of re.True(errors.ErrorEqual(...)); no other changes are needed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/etcd_start_test.go`:
- Line 113: Update both sentinel checks in server/etcd_start_test.go at lines
113-113 and 130-130 to use testify’s re.ErrorIs(err, errs.ErrCancelStartEtcd)
instead of re.True(errors.ErrorEqual(...)); no other changes are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b44476c-552a-4f9e-9646-8e287719106a
📒 Files selected for processing (2)
server/etcd_start_test.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/server.go
|
@bufferflies: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: Close #11014
server.EtcdStartTimeout(default5m) is applied as a fixed overall deadline in(*Server).startEtcd. The deadline counts down regardless of whether etcd is making progress, so a member that restarts and has to catch up a large raft log can be killed once 5 minutes elapse even though it is healthy and steadily applying entries. Under a process supervisor it then restarts and loops forever, never coming up. The timeout also cannot tell a slow-but-healthy startup apart from a genuine hang (e.g. a removed member that can never rejoin the quorum).What is changed and how does it work?
Wait for etcd readiness using the etcd applied index as a liveness signal instead of a fixed deadline:
waitEtcdReadypollsetcd.Server.AppliedIndex()while waiting forReadyNotify().EtcdStartTimeout, which still fails fast on a genuine hang.ctxcancellation is still honored for normal shutdown.initMembernow uses its ownEtcdStartTimeout-bounded context.AppliedIndexis used (rather than committed index) because during catch-up the committed index may already sit at the target while the state machine is still applying, which would look "stuck" while actually healthy.Known limitation: this covers the wait after
embed.StartEtcdreturns. The bulk snapshot load / WAL replay happens insideembed.StartEtcd, which takes noctxand runs before this wait, so that phase is out of scope here.Check List
Tests
TestWaitEtcdReadyProgress: ready-immediately, keeps-waiting-while-progressing, no-progress-timeout, ctx-cancellation)tests/server/joinsuite, which exercisesEtcdStartTimeoutand the removed-member hang path, still passes)Side effects
EtcdStartTimeoutto complete (intended, to avoid killing healthy slow startups).Release note
Summary by CodeRabbit