mcs, tso: fix expected-primary transient marker races - #11123
mcs, tso: fix expected-primary transient marker races#11123bufferflies wants to merge 7 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
|
[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
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:
|
| path := keypath.ExpectedPrimaryPath(msParam) | ||
| if expectedValue == "" { | ||
| return nil | ||
| return clientv3.Compare(clientv3.CreateRevision(path), "=", 0) |
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.
There was a problem hiding this comment.
Fixed already — ExpectedPrimaryCmp now asserts CreateRevision(marker) == 0 when the observed flag is empty, closing exactly this race: a campaigner that paused after reading an empty flag and resumed after a transfer installed a target marker will fail this atomic comparison at commit time instead of winning unconditionally. See ExpectedPrimaryCmp in pkg/mcs/utils/expected_primary.go.
| } | ||
| 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. |
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.
There was a problem hiding this comment.
Good catch. Fixed in 1905731: deleteMarkerIfEquals now distinguishes a failed reconcile transaction (a real RPC/etcd error) from a confirmed-absent marker, and DeleteExpectedPrimaryFlag treats the former as superseded=true (step down) instead of silently continuing to serve. Since a serving primary never watches the marker again, collapsing a transaction failure into "nothing to do" could otherwise strand a newer transfer indefinitely — stepping down on a transient error is a much cheaper trade-off.
…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
…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>
A GetExpectedPrimaryFlag read failure previously skipped campaigning
entirely and just retried, on the assumption that treating the flag
as empty would let a non-target member win without the affinity
guard. ExpectedPrimaryCmp("") already closes that gap atomically -
it asserts the marker is still absent at commit time - so a read
failure no longer needs to block campaigning; it can only fail closed
if a marker actually exists. Skipping campaigning on every read
failure left the service leaderless for as long as the reads kept
failing, even with no transfer in progress.
Keep the same 200ms backoff after a read-failure-triggered campaign
attempt (win or lose): the failure usually means etcd itself is
degraded, and campaigning is heavier than the read that just failed,
so retrying without backoff would add pressure right when etcd can
least take it.
Signed-off-by: bufferflies <1045931706@qq.com>
…s, backoff on supersede
- DeleteExpectedPrimaryFlag/deleteMarkerIfEquals: distinguish a failed
reconcile transaction from a confirmed-absent marker. A transaction
failure previously collapsed into "marker absent, keep serving,"
which could silently strand a newer transfer forever since a serving
primary never watches the marker again. Fail closed instead: step
down so the next campaign re-checks state fresh.
- TransferPrimary: bound the leader-key read with
etcdutil.DefaultRequestTimeout, matching the pattern already used
elsewhere in this file, so a hung read can't block the
{service}/primary/transfer request indefinitely.
- resourcemanager/scheduling/tso: add the same 200ms backoff already
used elsewhere in each election loop to the step-down path taken
when a newer transfer superseded this member right after it won -
otherwise that path re-campaigns in a tight loop.
Signed-off-by: bufferflies <1045931706@qq.com>
|
@bufferflies: The following tests 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 #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