mcs, tso: fix expected-primary transient marker races - #11123
Open
bufferflies wants to merge 5 commits into
Open
mcs, tso: fix expected-primary transient marker races#11123bufferflies wants to merge 5 commits into
bufferflies wants to merge 5 commits into
Conversation
The expected-primary transient marker mechanism introduced in aa5a988 has four correctness gaps that let `{service}/primary/transfer` silently no-op or be bypassed: - markExpectedPrimaryFlag wrote the marker unconditionally; a caller that lost leadership between its IsServing() check and the write could still publish a marker that the real, already-serving primary never reacts to. - DeleteExpectedPrimaryFlag could not tell "marker gone" from "marker overwritten by a newer transfer", so a newer transfer's target was never promoted when the write raced a winning campaign. - ExpectedPrimaryCmp returned no guard for the empty-marker case, letting a campaigner that observed no transfer win anyway if a transfer installed a marker and released the leader key in the meantime. - Guarding the marker write on the leader key's Value is not enough to fence a specific election term, since MemberValue() never changes for a participant's lifetime; guard on CreateRevision instead, captured right after the IsServing() check rather than right before the write. markExpectedPrimaryFlag now takes extra etcd comparisons folded into the same transaction as the Put. DeleteExpectedPrimaryFlag takes the campaigning participant and returns whether it was superseded by a newer transfer, so its three callers (scheduling, resource manager, TSO) step down instead of silently keeping serving. ExpectedPrimaryCmp always returns a real comparison, asserting the marker is absent (CreateRevision == 0) when no transfer was observed. TransferPrimary now reads the leader key right after its IsServing() check and fences the marker write on that CreateRevision, and revokes the newly granted lease on any failure path. See tikv#11122 for the analysis and reproduction reasoning behind each issue. Signed-off-by: bufferflies <1045931706@qq.com>
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
release-note
Contributor
|
[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/XL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change fences expected-primary marker writes to the observed leadership term, reconciles marker replacement during elections, and makes campaigners step down when superseded. Tests cover marker races, lease cleanup, and election-term fencing. Transfer recovery comments now describe the one-lease window. ChangesExpected-primary marker reconciliation
Transfer write term fencing
Campaign cleanup and step-down
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TransferPrimary
participant etcd
participant Campaign
participant DeleteExpectedPrimaryFlag
TransferPrimary->>etcd: capture leader-key create revision
TransferPrimary->>etcd: publish guarded expected-primary marker
TransferPrimary->>TransferPrimary: resign
Campaign->>etcd: campaign with expected-primary comparison
Campaign->>DeleteExpectedPrimaryFlag: reconcile marker
DeleteExpectedPrimaryFlag-->>Campaign: report target status
Campaign->>Campaign: step down if superseded
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
🧹 Nitpick comments (1)
pkg/mcs/utils/expected_primary.go (1)
103-107: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDistinguish a failed read from an absent marker.
deleteMarkerIfEqualsreturns("", false)both when the marker does not exist and when the transaction fails. Line 103 collapses the two cases intosuperseded = false. If the failure is transient and a newer transfer had already retargeted the marker to another member, this member keeps serving and the transfer does nothing until the marker TTL expires. That is the silent no-op class this PR removes elsewhere.Return the transaction error from the helper and retry once, or treat an unknown outcome conservatively.
♻️ Proposed change to separate the failure case
-func deleteMarkerIfEquals(client *clientv3.Client, path, want string) (current string, deleted bool) { +func deleteMarkerIfEquals(client *clientv3.Client, path, want string) (current string, deleted bool, err error) { resp, err := kv.NewSlowLogTxn(client). If(clientv3.Compare(clientv3.Value(path), "=", want)). Then(clientv3.OpGet(path), clientv3.OpDelete(path)). Else(clientv3.OpGet(path)). Commit() if err != nil { log.Warn("failed to delete expected primary flag", zap.String("primary-path", path), errs.ZapError(err)) - return "", false + return "", false, err }Then retry once in
DeleteExpectedPrimaryFlagbefore assuming the marker is gone.🤖 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/mcs/utils/expected_primary.go` around lines 103 - 107, Update deleteMarkerIfEquals to return the transaction error separately from the empty-marker result, then have DeleteExpectedPrimaryFlag retry once when the read fails before treating the marker as absent. Preserve the existing false/superseded behavior only after a successful read confirms no marker exists, and handle the retry’s remaining error conservatively.
🤖 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 `@pkg/mcs/resourcemanager/server/server.go`:
- Around line 245-252: The step-down branch in campaignLeader currently returns
false without the delay used by the earlier expected-primary mismatch path. Add
the same bounded 200ms sleep before returning, so primaryElectionLoop does not
immediately re-campaign after DeleteExpectedPrimaryFlag reports a newer transfer
target.
In `@pkg/mcs/utils/expected_primary.go`:
- Around line 208-216: Update the leader-key read in TransferPrimary around
client.Get to use a derived context with etcdutil.DefaultRequestTimeout,
matching the existing timeout pattern used near lines 154 and 307. Pass that
bounded context to client.Get while preserving the existing error annotation and
leadership validation.
---
Nitpick comments:
In `@pkg/mcs/utils/expected_primary.go`:
- Around line 103-107: Update deleteMarkerIfEquals to return the transaction
error separately from the empty-marker result, then have
DeleteExpectedPrimaryFlag retry once when the read fails before treating the
marker as absent. Preserve the existing false/superseded behavior only after a
successful read confirms no marker exists, and handle the retry’s remaining
error conservatively.
🪄 Autofix
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 Plus
Run ID: 85665042-8d0a-4c4a-aeb0-dcf18e837881
📒 Files selected for processing (5)
pkg/mcs/resourcemanager/server/server.gopkg/mcs/scheduling/server/server.gopkg/mcs/utils/expected_primary.gopkg/mcs/utils/expected_primary_test.gopkg/tso/allocator.go
Comment on lines
+245
to
+252
| // failure re-elects immediately instead of waiting for the flag's TTL. If a newer | ||
| // transfer rewrote the flag to another member while we were winning, step down so | ||
| // the re-election routes leadership to that target. | ||
| if utils.DeleteExpectedPrimaryFlag(s.GetClient(), &s.participant.MsParam, expectedPrimary, s.participant) { | ||
| log.Info("the expected primary has been changed to another member, stepping down", | ||
| zap.String("server-name", s.Name())) | ||
| return false | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the resource manager election loop and every DeleteExpectedPrimaryFlag call site.
set -euo pipefail
ast-grep outline pkg/mcs/resourcemanager/server/server.go --items all
rg -nP -C 12 '\bcampaignLeader\s*\(' pkg/mcs/resourcemanager/server/server.go
rg -nP -C 4 '\bDeleteExpectedPrimaryFlag\s*\(' --type=goRepository: tikv/pd
Length of output: 5760
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## server.go primaryElectionLoop and campaignLeader"
sed -n '145,265p' pkg/mcs/resourcemanager/server/server.go | cat -n
echo "## All DeleteExpectedPrimaryFlag references"
rg -n -C 5 'DeleteExpectedPrimaryFlag' --type=go
echo "## campaignLeader return call sites"
rg -n -C 6 'campaignLeader\b|backoff|time.Sleep|Sleep' pkg/mcs/resourcemanager/server/server.go --type=go
echo "## Compare similar loops in scheduler/tso if present"
for f in $(git ls-files | rg 'pkg/.*/.*_server\.go$|pkg/.*/.*server\.go$' | rg -E 'scheduling|tso|mcs/|resources'); do
[ -f "$f" ] || continue
if rg -q 'primaryElectionLoop|campaignLeader|ExpectedPrimaryCmp|DeleteExpectedPrimaryFlag' "$f"; then
echo "### $f"
rg -n -C 8 'primaryElectionLoop|campaignLeader|ExpectedPrimaryCmp|DeleteExpectedPrimaryFlag' "$f"
fi
doneRepository: tikv/pd
Length of output: 6958
Add a short sleep before re-campaigning on the step-down path.
campaignLeader returns false here, but primaryElectionLoop immediately returns to the top and starts the next CampaignWithCmps without sleeping. The earlier expected-primary mismatch path sleeps 200ms before re-checking. Since a newer transfer can rewrite the flag while this server campaigned, add the same bounded sleep after this false return to avoid tight-loop re-election attempts.
🤖 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/mcs/resourcemanager/server/server.go` around lines 245 - 252, The
step-down branch in campaignLeader currently returns false without the delay
used by the earlier expected-primary mismatch path. Add the same bounded 200ms
sleep before returning, so primaryElectionLoop does not immediately re-campaign
after DeleteExpectedPrimaryFlag reports a newer transfer target.
Comment on lines
+208
to
+216
| leaderKeyPath := p.GetLeadership().GetLeaderKey() | ||
| leaderResp, err := client.Get(client.Ctx(), leaderKeyPath) | ||
| if err != nil { | ||
| return errors.Annotate(err, "failed to read leader key for transfer guard") | ||
| } | ||
| if len(leaderResp.Kvs) == 0 || string(leaderResp.Kvs[0].Value) != p.MemberValue() { | ||
| return errors.New("current member is not serving as primary, please check leadership") | ||
| } | ||
| leaderKeyGuard := clientv3.Compare(clientv3.CreateRevision(leaderKeyPath), "=", leaderResp.Kvs[0].CreateRevision) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the leader-key read with a request timeout.
client.Get(client.Ctx(), leaderKeyPath) inherits the etcd client lifetime context. It has no deadline. TransferPrimary runs on the {service}/primary/transfer request path, so a slow or hung etcd read blocks that request without bound. The new code at lines 154 and 307 already uses etcdutil.DefaultRequestTimeout. Use the same bound here.
As per coding guidelines: "Use context-aware timeouts and backoff for retries".
🛡️ Proposed fix
leaderKeyPath := p.GetLeadership().GetLeaderKey()
- leaderResp, err := client.Get(client.Ctx(), leaderKeyPath)
+ getCtx, getCancel := context.WithTimeout(client.Ctx(), etcdutil.DefaultRequestTimeout)
+ leaderResp, err := client.Get(getCtx, leaderKeyPath)
+ getCancel()
if err != nil {
return errors.Annotate(err, "failed to read leader key for transfer guard")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| leaderKeyPath := p.GetLeadership().GetLeaderKey() | |
| leaderResp, err := client.Get(client.Ctx(), leaderKeyPath) | |
| if err != nil { | |
| return errors.Annotate(err, "failed to read leader key for transfer guard") | |
| } | |
| if len(leaderResp.Kvs) == 0 || string(leaderResp.Kvs[0].Value) != p.MemberValue() { | |
| return errors.New("current member is not serving as primary, please check leadership") | |
| } | |
| leaderKeyGuard := clientv3.Compare(clientv3.CreateRevision(leaderKeyPath), "=", leaderResp.Kvs[0].CreateRevision) | |
| leaderKeyPath := p.GetLeadership().GetLeaderKey() | |
| getCtx, getCancel := context.WithTimeout(client.Ctx(), etcdutil.DefaultRequestTimeout) | |
| leaderResp, err := client.Get(getCtx, leaderKeyPath) | |
| getCancel() | |
| if err != nil { | |
| return errors.Annotate(err, "failed to read leader key for transfer guard") | |
| } | |
| if len(leaderResp.Kvs) == 0 || string(leaderResp.Kvs[0].Value) != p.MemberValue() { | |
| return errors.New("current member is not serving as primary, please check leadership") | |
| } | |
| leaderKeyGuard := clientv3.Compare(clientv3.CreateRevision(leaderKeyPath), "=", leaderResp.Kvs[0].CreateRevision) |
🤖 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/mcs/utils/expected_primary.go` around lines 208 - 216, Update the
leader-key read in TransferPrimary around client.Get to use a derived context
with etcdutil.DefaultRequestTimeout, matching the existing timeout pattern used
near lines 154 and 307. Pass that bounded context to client.Get while preserving
the existing error annotation and leadership validation.
Source: Coding guidelines
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11123 +/- ##
==========================================
+ Coverage 79.35% 79.41% +0.05%
==========================================
Files 542 542
Lines 76993 77091 +98
==========================================
+ Hits 61097 61218 +121
+ Misses 11594 11580 -14
+ Partials 4302 4293 -9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
rleungx
reviewed
Aug 7, 2026
| path := keypath.ExpectedPrimaryPath(msParam) | ||
| if expectedValue == "" { | ||
| return nil | ||
| return clientv3.Compare(clientv3.CreateRevision(path), "=", 0) |
Member
There was a problem hiding this comment.
An afa43111d replica still omits this comparison after observing an empty marker. During a rolling upgrade it can pause after that read, resume after an upgraded primary installs a transfer marker and resigns, and win the unconditional campaign, so /primary/transfer returns success with leadership on a non-target replica.
rleungx
reviewed
Aug 7, 2026
| } | ||
| if current == "" { | ||
| // The marker is already gone (deleted, expired, or it never existed), or the | ||
| // transaction failed; in the latter case the marker TTL bounds the staleness. |
Member
There was a problem hiding this comment.
If this reconciliation transaction returns a transient error after an older in-flight transfer rewrites the marker, current == "" lets this winner continue to PromoteSelf. A serving primary does not watch that marker, so the transfer has already returned success but never takes effect; marker expiry does not trigger another election.
…ccess
A replica that predates the expected-primary marker mechanism (e.g. a
not-yet-upgraded node during a rolling upgrade) does not read the marker and
can win the now-vacated leader key through its own unguarded campaign, so
{service}/primary/transfer could return success while leadership actually
went to a different, non-target replica.
TransferPrimary now threads a ctx through to a new post-resign verification
step: it polls the leader key until its holder's identity matches new_primary
on two consecutive checks, or the marker's own TTL elapses, whichever comes
first. Two consecutive matches (not one) are required because the winner of a
campaign still runs its own post-campaign steps (marker reconcile,
primaryCallbacks) before it is durably promoted, and any of those can make it
step back down again shortly after a transient win. The verification is
skipped when new_primary is empty (pick any valid secondary), since there is
no fixed target to check against.
ctx is threaded from each HTTP handler's request context (so an abandoned
request does not leave verification polling for the rest of its timeout
regardless) and from the priority-check background loop's own context.
Updated three integration test assertions that relied on the old
fire-and-forget response: with skipGrantLeader forcing the target to never
win, the initial transfer call now honestly reports failure instead of a
premature 200, while the cluster's eventual recovery (verified separately by
each test) is unaffected.
Signed-off-by: bufferflies <1045931706@qq.com>
ti-chi-bot
Bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/mcs/utils/expected_primary.go`:
- Around line 353-375: Update the verification loop around GetProtoMsgWithModRev
so each etcd read uses the outer ctx rather than client.Ctx(). Either replace it
with client.Get(ctx, leaderKeyPath) and decode the returned participant value,
or extend the helper to accept and propagate ctx while preserving the existing
target validation and error handling.
🪄 Autofix
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 Plus
Run ID: 764a00c6-cbef-4b7c-bde7-7519880f8128
📒 Files selected for processing (6)
pkg/mcs/resourcemanager/server/apis/v1/api.gopkg/mcs/scheduling/server/apis/v1/api.gopkg/mcs/tso/server/apis/v1/api.gopkg/mcs/utils/expected_primary.gopkg/tso/keyspace_group_manager.gotests/integrations/mcs/members/member_test.go
Comment on lines
+353
to
+375
| target := member.NewParticipantByService(serviceName) | ||
| if ok, _, err := etcdutil.GetProtoMsgWithModRev(client, leaderKeyPath, target); err == nil && ok { | ||
| if target.GetName() == newPrimary || slices.Contains(target.GetListenUrls(), newPrimary) { | ||
| matchStreak++ | ||
| if matchStreak >= primaryTransferStableChecks { | ||
| return nil | ||
| } | ||
| } else { | ||
| matchStreak = 0 | ||
| } | ||
| currentPrimary = target.GetName() | ||
| } else { | ||
| matchStreak = 0 | ||
| currentPrimary = "" | ||
| } | ||
| select { | ||
| case <-ctx.Done(): | ||
| if currentPrimary == "" { | ||
| return errors.Errorf("transfer requested to %s, but no primary was elected: %v", newPrimary, ctx.Err()) | ||
| } | ||
| return errors.Errorf("transfer requested to %s, but %s is currently primary: %v", newPrimary, currentPrimary, ctx.Err()) | ||
| case <-ticker.C: | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/utils/etcdutil/etcdutil.go --match GetProtoMsgWithModRev --view expanded
rg -n -C 6 'func GetProtoMsgWithModRev|func get\(|client\.Get\(' \
pkg/utils/etcdutil/etcdutil.go pkg/mcs/utils/expected_primary.goRepository: tikv/pd
Length of output: 4225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== expected_primary relevant section =="
sed -n '320,380p' pkg/mcs/utils/expected_primary.go
echo "== etcdutil relevant helper =="
sed -n '232,265p' pkg/utils/etcdutil/etcdutil.go
echo "== searches for GetProtoMsgWithModRev callers and signature variants =="
rg -n 'GetProtoMsgWithModRev\(' pkg/mcs/pkg pkg/utils/etcdutil || trueRepository: tikv/pd
Length of output: 4463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== EtcdKVGet implementation =="
rg -n -C 8 'func EtcdKVGet|func Get\(ctx context.Context|WithRequireLeader\(ctx\), timeout' pkg/utils/etcdutil/etcdutil.go pkg/utils/etcdutil || true
echo "== all GetProtoMsgWithModRev callers =="
rg -n 'etcdutil\.GetProtoMsgWithModRev|GetProtoMsgWithModRev\(' .
echo "== import context in expected_primary and etcdutil =="
sed -n '1,40p' pkg/mcs/utils/expected_primary.go
sed -n '1,30p' pkg/utils/etcdutil/etcdutil.goRepository: tikv/pd
Length of output: 7986
Make each verification read honor ctx.
GetProtoMsgWithModRev(client, leaderKeyPath, target) currently calls client.Ctx() for its read, so the loop timeout cannot cancel a slow etcd get. Use client.Get(ctx, leaderKeyPath) or make the helper accept the outer context before decoding the participant value.
🤖 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/mcs/utils/expected_primary.go` around lines 353 - 375, Update the
verification loop around GetProtoMsgWithModRev so each etcd read uses the outer
ctx rather than client.Ctx(). Either replace it with client.Get(ctx,
leaderKeyPath) and decode the returned participant value, or extend the helper
to accept and propagate ctx while preserving the existing target validation and
error handling.
Source: Coding guidelines
…y window Two follow-up review comments on the post-transfer verification added in 295501d: - waitForPrimaryTransfer compared the caller-supplied new_primary directly against the winner's identity. When a service is registered with a name distinct from its advertise address (the default configuration), new_primary is the registry name while the leader proto stores the advertise address (an "address-groupID" composite for TSO), so the comparison never matched - a successful transfer would still wait out the full marker TTL and report 500. Compare against primaryID instead: it's the resolved ServiceAddr chosen during candidate selection, the same identity Participant.IsExpectedPrimary already relies on (a marker value is matched against ListenUrls), so it's guaranteed to appear in the winner's own ListenUrls regardless of what form the caller supplied. new_primary is kept only for the error message, so a failure still reads naturally to whoever issued the request. - The stability requirement (2 consecutive polls ~200ms apart) was too short: the leader key is written as soon as the campaign transaction commits, well before the winner is durably promoted - it still has to reconcile the marker, run primaryCallbacks, and for TSO initialize the allocator (real I/O via syncTimestamp) first, and a failure in any of those steps can make it step back down shortly after. Two checks 200ms apart could both land inside that window. Replaced the fixed poll count with a continuous-match duration requirement (2s) sized to more comfortably outlast those post-campaign steps under normal conditions - this narrows, and is honestly documented as not eliminating, the same class of gap; a full fix would need to ask the target directly rather than infer from etcd state. Signed-off-by: bufferflies <1045931706@qq.com>
matchSince was keyed only on the target's advertised address, not the leader key's own revision. If the target loses and re-acquires the leader key between two polls - invisible to us if both polls happen to observe a "matched" state either side of the gap - matchSince keeps counting across that gap as if it were one continuous term. The 2-second stability window could then be satisfied by time accumulated partly against the earlier, already-reverted term, letting the endpoint report success while the new term is still mid primaryCallbacks/TSO initialization - exactly the failure mode the window exists to guard against. GetProtoMsgWithModRev already returns the leader key's ModRevision, and Leadership.Campaign requires CreateRevision(leaderKey) == 0 to win while removeLeaderKey deletes the key outright, so every fresh term necessarily gets a new ModRevision. Track the ModRevision alongside matchSince and restart the window whenever it changes, even if the address still matches - the 2 seconds now only ever accumulate within one term. Signed-off-by: bufferflies <1045931706@qq.com>
TransferPrimary previously polled the leader key after marking and resigning, waiting for the target to be observed stably serving before reporting success. That verification only narrowed, never eliminated, the gap between the leader key being written and the target actually finishing initialization (see tikv#11122 discussion), while making the API call block for up to the marker's TTL and fail on a short-lived caller context even when the transfer itself was proceeding normally. Drop the wait: TransferPrimary now reports success as soon as the marker is written and the current primary has resigned. The correctness fixes from the prior commits (mark-before-resign ordering, the atomic leader-key guard, and DeleteExpectedPrimaryFlag clearing the marker as soon as any member wins a campaign) are unaffected and still stand. Without verification, the worst-case unavailability a transfer can leave behind - when the target never wins a single campaign at all - is bounded by the marker's TTL. Shrink that TTL from 3 leader leases to 1: a target that never wins even once already has no fixed cost to amortize by waiting longer, and a target that does win clears the marker immediately regardless of whether it goes on to initialize successfully, so the multiplier only ever pays for the "never wins" case. Signed-off-by: bufferflies <1045931706@qq.com>
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
2 participants
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.
What problem does this PR solve?
Issue Number: Close #11122
The expected-primary transient marker mechanism (introduced in aa5a988) has four correctness gaps that let
{service}/primary/transfersilently no-op or be bypassed under leadership churn. See #11122 for the full analysis and reproduction reasoning behind each one.What is changed and how does it work?
Update: dropped post-transfer verification, capped marker TTL at 1 lease
An earlier revision of this PR added
waitForPrimaryTransfer: after markingand resigning,
TransferPrimarypolled the leader key and only reportedsuccess once the target was observed stably holding it. Review discussion
(see the thread on #11122 / the linked pd-cse PR) established that this
verification only narrowed, never eliminated, the gap between the leader
key being written and the target actually finishing initialization (TSO's
syncTimestampin particular does real I/O and can fail well after the keylooks stable) - it could not be made airtight without querying the target's
own local serving state instead of polling etcd, which is out of scope here.
Meanwhile it made the API call block for up to the marker's TTL and could
report a false failure to a caller with a short-lived context even while the
transfer was proceeding normally in the background.
Given the verification could not be made complete anyway, this PR now drops
it:
TransferPrimaryreports success as soon as the marker is written andthe current primary has resigned, without waiting for the target to actually
win or initialize. The other correctness fixes above (mark-before-resign
ordering, the atomic leader-key guard,
DeleteExpectedPrimaryFlagclearingthe marker as soon as any member wins a campaign) are unaffected.
Without verification, the worst-case unavailability a transfer can leave
behind - when the target never wins a single campaign at all (down,
unreachable, or stuck) - is bounded by the marker's TTL
(
TransferPrimaryLeaseMultiplier * leaderLease), since every other candidatebacks off in its favor for as long as the marker is valid. This PR also
shrinks that multiplier from 3 to 1: a target that does win clears the marker
immediately regardless of whether it goes on to initialize successfully, so
the multiplier's only effect is on the "never wins even once" case, and there
is no reason to let that cost more than one leader lease - the same duration
the cluster already tolerates a primary being unreachable everywhere else.
Check List
Tests
Side effects
Release note
Summary by CodeRabbit