server, member: check target PD readiness before leader transfer#10994
server, member: check target PD readiness before leader transfer#10994HunDunDM wants to merge 3 commits into
Conversation
Signed-off-by: HunDunDM <hundundm@gmail.com>
Signed-off-by: HunDunDM <hundundm@gmail.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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLeader transfer now supports optional target readiness checks. Manual transfer, resign, and priority-based transfer paths use PD version and readiness endpoints to reject or skip unready members, with new error definitions, API path constants, server checks, and coverage tests. ChangesLeader transfer readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ti-chi-bot
Bot
added
the
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/member/member.go (1)
417-421: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a timeout-bounded context for the resign candidate checker loop.
MoveEtcdLeader(line 351) correctly usesmoveCtxwithmoveLeaderTimeoutfor the checker, butResignEtcdLeader(line 421) passes the rawctxtooptions.targetChecker. When called from API handlers viah.svr.Context()(a long-lived context), the checker loop across multiple candidates has no context-level deadline, relying solely on the HTTP client timeout. Adding a timeout-bounded context here would bound the total check duration and align with theMoveEtcdLeaderpattern.♻️ Suggested fix
options := newMoveEtcdLeaderOptions(opts...) if options.targetChecker != nil { + checkCtx, cancel := context.WithTimeout(ctx, moveLeaderTimeout) + defer cancel() readyEtcdLeaderIDs := make([]uint64, 0, len(etcdLeaderIDs)) var lastErr error for _, id := range etcdLeaderIDs { - if err := options.targetChecker(ctx, id); err != nil { + if err := options.targetChecker(checkCtx, id); err != nil { lastErr = err log.Warn("skip unready pd when resigning etcd leader", zap.Uint64("target-member-id", id), errs.ZapError(err)) continue🤖 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 `@pkg/member/member.go` around lines 417 - 421, ResignEtcdLeader’s target-checker loop uses the unbounded ctx, allowing candidate checks to exceed the intended operation timeout. Create a timeout-bounded moveCtx using moveLeaderTimeout before iterating candidates, pass moveCtx to options.targetChecker, and ensure the context is canceled appropriately, matching MoveEtcdLeader’s pattern.
🤖 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 `@pkg/member/member.go`:
- Around line 417-421: ResignEtcdLeader’s target-checker loop uses the unbounded
ctx, allowing candidate checks to exceed the intended operation timeout. Create
a timeout-bounded moveCtx using moveLeaderTimeout before iterating candidates,
pass moveCtx to options.targetChecker, and ensure the context is canceled
appropriately, matching MoveEtcdLeader’s pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e769ac2-1240-45f9-86db-dbdcad899e31
📒 Files selected for processing (15)
errors.tomlpkg/errs/errno.gopkg/member/member.gopkg/utils/apiutil/apiutil.gopkg/versioninfo/feature.gopkg/versioninfo/versioninfo_test.goserver/api/member.goserver/apiv2/middlewares/microservice_redirector.goserver/apiv2/router.goserver/member_ready.goserver/member_ready_test.goserver/server.gotests/integrations/client/http_client_test.gotests/server/api/member_test.gotests/server/member/member_test.go
|
/test pull-unit-test-next-gen-3 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10994 +/- ##
==========================================
+ Coverage 79.22% 79.27% +0.04%
==========================================
Files 541 542 +1
Lines 75965 76091 +126
==========================================
+ Hits 60187 60320 +133
- Misses 11531 11537 +6
+ Partials 4247 4234 -13
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| readyEtcdLeaderIDs := make([]uint64, 0, len(etcdLeaderIDs)) | ||
| var lastErr error | ||
| for _, id := range etcdLeaderIDs { | ||
| if err := options.targetChecker(ctx, id); err != nil { |
There was a problem hiding this comment.
I suggest not passing the outer ctx directly to the target checker here. In the API path, ResignEtcdLeader receives a server-level context with a long lifetime. If the checker blocks, or if a future checker implementation no longer relies only on HTTP request timeouts, the candidate filtering phase would not have a clear time bound.
MoveEtcdLeader already wraps the operation with moveLeaderTimeout; this path should have a similar timeout-bounded context for the checker. One detail: for resign with multiple candidates, a single shared 5s timeout may be consumed by the first unready candidate and prevent checking later ready candidates, so a per-candidate timeout or an explicitly documented total timeout would be safer.
| if err != nil { | ||
| return errors.Annotatef(err, "failed to get target pd member %d version from %s", memberID, clientURL) | ||
| } | ||
| pdVersion, err := versioninfo.ParseVersion(version) |
There was a problem hiding this comment.
Could we clarify the compatibility policy for unparsable target versions here? The current logic treats a successful /pd/api/v1/version response with an unparsable version string, such as "None", as if the target supports /pd/api/v2/ready, and then calls the ready API.
This is useful for local/dev builds where PDReleaseVersion can be "None". However, the issue states that if the target version does not support /ready, a successful version request should be treated as ready for compatibility. If an older or custom-built PD returns "None" or another non-standard version string and does not expose /pd/api/v2/ready, the current logic would reject the transfer.
Please make this policy explicit: either document that unparsable/None means a dev/current build and should still check ready, or treat unknown versions as not known to support the ready API and allow the transfer for compatibility. The table case in TestCheckMemberReadyURL should reflect that decision.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if clientURLs := findMemberClientURLs(members, memberID); len(clientURLs) > 0 { |
There was a problem hiding this comment.
This uses cached client URLs as long as the member is found, and only calls ReloadMembers() when the member is missing. If a member’s client URL was just updated, the cache may still contain the old URL; the ready check would fail against the stale address and would not force a reload/retry.
The cache TTL is only 1s, so the impact window is small. Still, leader transfer is a low-frequency admin operation, and one forced reload after all cached URLs fail should be cheap. Consider reloading members and retrying if the cached URLs all fail and the target member’s URLs changed.
2 participants
What problem does this PR solve?
Issue Number: Close #10993
PD exposes
/pd/api/v2/readyto indicate whether a member has finished initial region loading and can be promoted to leader. Before this change, selected PD leader transfer paths could move embedded etcd leadership to a target PD that had not finished region loading.What is changed and how does it work?
This PR adds an opt-in target readiness check before selected embedded etcd leader transfers.
Key changes:
ReadyAPIfeature metadata inpkg/versioninfo.MoveEtcdLeader,ResignEtcdLeader, andCheckPriority./pd/api/v1/leader/transfer/{next_leader}/pd/api/v1/leader/resignResignEtcdLeaderfilters ready candidates.Check List
Tests
Code changes
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests