mcs: skip transferring primary to itself#10970
Conversation
ti-chi-bot
Bot
added
release-note
|
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:
📝 WalkthroughWalkthroughTransferPrimary now treats the current primary as a no-op target when the requested primary matches by name or service address, and the tests verify expected-primary lease and revision behavior for self-transfer and random-transfer paths. ChangesTSO Self-Transfer No-Op
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
lhy1024
force-pushed
the
lhy/fix-tso-self-transfer-primary
branch
from
b99c915 to
fbbec7e
Compare
![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/mcs/utils/expected_primary_test.go (1)
138-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
ctxto the first parameter position.
putRegistryEntryperforms an external etcd write withctx, socontext.Contextshould be the first parameter. Callers at Lines 50-51 and 106-107 need updating accordingly.As per coding guidelines: "First parameter must be
context.Contextfor external effects; never store contexts in structs".♻️ Proposed signature change (and call sites)
func putRegistryEntry( + ctx context.Context, re *require.Assertions, - ctx context.Context, client *clientv3.Client, name string, serviceAddr string, ) {Update call sites:
- putRegistryEntry(re, ctx, client, primaryName, primaryAddr) - putRegistryEntry(re, ctx, client, secondaryName, secondaryAddr) + putRegistryEntry(ctx, re, client, primaryName, primaryAddr) + putRegistryEntry(ctx, re, client, secondaryName, secondaryAddr)🤖 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_test.go` around lines 138 - 144, Move the context.Context argument to the first position in putRegistryEntry since it performs an external etcd write; update the function signature to take ctx before re and adjust all call sites that invoke putRegistryEntry accordingly, including the ones in expected_primary_test.go referenced by the review. Keep the rest of the parameter order unchanged so the helper remains consistent with the coding guideline about context-first APIs.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 `@pkg/mcs/utils/expected_primary_test.go`:
- Around line 138-144: Move the context.Context argument to the first position
in putRegistryEntry since it performs an external etcd write; update the
function signature to take ctx before re and adjust all call sites that invoke
putRegistryEntry accordingly, including the ones in expected_primary_test.go
referenced by the review. Keep the rest of the parameter order unchanged so the
helper remains consistent with the coding guideline about context-first APIs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66d021aa-029b-45bf-80de-1e9e5d1d07f6
📒 Files selected for processing (2)
pkg/mcs/utils/expected_primary.gopkg/mcs/utils/expected_primary_test.go
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 `@pkg/mcs/utils/expected_primary_test.go`:
- Around line 109-136: The test creates a lease with election.NewLease but never
tears it down, so add a deferred lease.Close() in this test setup just like the
matching lease handling in TestTransferPrimaryToSelfKeepsExpectedPrimaryLease.
Keep the change local to the test that uses lease.Grant and TransferPrimary so
the etcd keep-alive goroutine is cleaned up after the assertions complete.
- Around line 138-153: The helper putRegistryEntry currently violates the revive
context-as-argument rule because context.Context is not the first parameter.
Update the function signature so ctx is the first argument, keeping client,
name, and serviceAddr after it, and then adjust every call site in
expected_primary_test.go and any related test helpers to pass ctx first. Ensure
the change is applied consistently wherever putRegistryEntry is used so CI and
static analysis pass.
🪄 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: 248b7a3d-7d4c-4be0-b535-bb3827329215
📒 Files selected for processing (2)
pkg/mcs/utils/expected_primary.gopkg/mcs/utils/expected_primary_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/mcs/utils/expected_primary.go
lhy1024
force-pushed
the
lhy/fix-tso-self-transfer-primary
branch
from
5af893a to
08dd912
Compare
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
lhy1024
force-pushed
the
lhy/fix-tso-self-transfer-primary
branch
from
08dd912 to
5196294
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10970 +/- ##
==========================================
- Coverage 79.20% 79.18% -0.02%
==========================================
Files 541 541
Lines 75450 75640 +190
==========================================
+ Hits 59762 59899 +137
- Misses 11456 11495 +39
- Partials 4232 4246 +14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| continue | ||
| } | ||
| if (newPrimary == "" && member.Name != oldPrimary) || (newPrimary != "" && member.Name == newPrimary) { | ||
| if (newPrimary == "" && member.Name != oldPrimary) || (newPrimary != "" && isSamePrimary(member, newPrimary)) { |
There was a problem hiding this comment.
newPrimary ！= "" can be removed.
There was a problem hiding this comment.
I don't think the first newPrimary == "" guard can be removed. It separates the random-transfer path from the explicit-transfer path.
Example 1: random transfer.
members: A, B, C
oldPrimary: A
newPrimary: ""
In this case we should select any secondary except the old primary, so B and C are valid candidates. This is what (newPrimary == "" && member.Name != oldPrimary) does.
Example 2: explicit transfer.
members: A, B, C
oldPrimary: A
newPrimary: B
In this case we should only select B. With the current condition, only B is added:
A: false || false -> skipped
B: false || true -> added
C: false || false -> skipped
If the first newPrimary == "" guard is removed, the condition becomes effectively member.Name != oldPrimary || isSamePrimary(member, newPrimary). Then C would also be added because C != A:
A: false || false -> skipped
B: true || true -> added
C: true || false -> added
Then rand.IntN(len(primaryIDs)) may choose C, so an explicit transfer to B could unexpectedly transfer to C.
If you mean the second newPrimary != "" guard before isSamePrimary(member, newPrimary), that one is redundant because isSamePrimary already checks primary != "". But the first newPrimary == "" guard is still needed to preserve the explicit-transfer semantics.
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
|
/retest |
1 similar comment
|
/retest |
ti-chi-bot
Bot
added
needs-1-more-lgtm
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
|
@JmPotato PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
3 participants
What problem does this PR solve?
Issue Number: Close #10969
When a TSO primary transfer request specifies the current TSO primary itself as
new_primary, the current MCS transfer logic treats it as a real transfer rather than a no-op.That behavior is not expected. It can revoke the expected-primary lease, rewrite the expected-primary key, make the current primary exit the expected-primary watch loop, and force unnecessary primary churn.
This was observed during a TiDB Cloud TiDBX rolling-upgrade investigation. The confirmed TSO log sequence was:
2026-07-02T12:18:32.177Z: old primarydb-b5b0db46-d8u65qstarted transfer:from=db-b5b0db46-d8u65q,to=db-b5b0db46-bq855y.2026-07-02T12:18:32.196Z:db-b5b0db46-bq855yloggedcampaign tso primary ok.2026-07-02T12:18:32.209Z:db-b5b0db46-bq855yreceived another transfer request:from=db-b5b0db46-bq855y,to=db-b5b0db46-bq855y.2026-07-02T12:18:32.216Z:db-b5b0db46-bq855yloggedrevoke lease failed,primary exit the primary watch loop, andthe TSO primary will step down.2026-07-02T12:18:33.228Z:db-b5b0db46-bq855yreceived the same self-transfer request again.2026-07-02T12:18:33.249Z:db-b5b0db46-d8u65qloggedcampaign tso primary ok.2026-07-02T12:18:33.260Z:db-b5b0db46-bq855yloggedcampaign tso primary meets error due to txn conflict.The rollout was later observed with
TSOGroup updatedReplicas=1/2,db-b5b0db46-d8u65q isDefaultPrimary=true, anddb-b5b0db46-d8u65qstill on the old revision withPodNotUpToDate.This PR fixes the confirmed PD-side self-transfer behavior. It does not claim that the long-term rollout stuck state was caused only by PD. TCMS artifacts available in this investigation did not include the real controller/reconcile logs. A control-plane owner pointed out a separate likely control-plane issue: after a transfer request succeeds, the rollout expects to poll and observe the leader change; if the primary changes back before polling observes the expected state, the control-plane state machine may fail to continue or retry. That poll/retry behavior should be fixed or confirmed on the control-plane/operator side.
This PR is still needed because transfer-to-current-primary should be a no-op. In this incident, the control plane/operator request was the trigger: after
bq855yhad already become primary, another transfer request targetingbq855yitself exposed this PD behavior. Removing this unnecessary primary churn avoids creating or amplifying that race window.What is changed and how does it work?
The fix detects whether
new_primaryandoldPrimaryrefer to the same registered MCS member, either by member name or service address. If they do,TransferPrimaryreturns success without changing etcd state or closing the current lease.Validation
Unit tests:
TiUP real-process validation:
http://127.0.0.1:13380.Before this fix, the request returned
HTTP 200, but the primary performed a real self-transfer and left the primary role:After this fix, the same request returns
HTTP 200and only logs the no-op path:No leadership delete, expected-primary rewrite, lease revoke, watch-loop exit, primary role leave, or re-campaign is observed.
Check List
Tests
Code changes
Side effects
Related changes
Release note
Summary by CodeRabbit