tso: prevent dual writers when upgrading from PD service mode#10473
tso: prevent dual writers when upgrading from PD service mode#10473YuhaoZhang00 wants to merge 3 commits intotikv:masterfrom
Conversation
In PD service mode (isKeyspaceGroupEnabled=false), SaveTimestamp now atomically checks for an existing TSO microservice primary before writing the timestamp window. If a TSO primary is detected, the write is rejected, causing the allocator to reset and resign PD leadership. This allows a PD in API service mode to take over and proxy TSO requests, preventing dual writers during rolling upgrades. Also remove TestMixedTSODeployment which tested seamless coexistence of PD service mode and TSO microservice — a scenario not supported by the official upgrade procedure (PD restart is required). Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
ti-chi-bot
bot
added
release-note-none
|
[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
contribution
|
Hi @YuhaoZhang00. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
ti-chi-bot
bot
added
the
size/L
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant Allocator
participant TimestampOracle
participant StorageEndpoint
participant Database
Server->>Allocator: NewAllocator(..., checkTSOPrimary)
Allocator->>TimestampOracle: initialize(checkTSOPrimary)
Note over TimestampOracle: generate/prepare timestamp
TimestampOracle->>StorageEndpoint: SaveTimestamp(ts, checkTSOPrimary)
alt checkTSOPrimary == true
StorageEndpoint->>Database: TX read TSO primary election key
Database-->>StorageEndpoint: key value
alt key non-empty
StorageEndpoint-->>TimestampOracle: return error (reject write)
else key empty
StorageEndpoint->>Database: TX save timestamp
Database-->>StorageEndpoint: success
StorageEndpoint-->>TimestampOracle: success
end
else checkTSOPrimary == false
StorageEndpoint->>Database: TX save timestamp
Database-->>StorageEndpoint: success
StorageEndpoint-->>TimestampOracle: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
/cc @JmPotato PTAL |
|
@YuhaoZhang00: GitHub didn't allow me to request PR reviews from the following users: PTAL. Note that only tikv members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions 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. |
![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 the current code and only fix it if needed.
Inline comments:
In `@pkg/storage/endpoint/tso.go`:
- Around line 113-115: The error string returned in the conditional checking
tsoPrimary (the return in the if tsoPrimary != "" branch that calls
errors.Errorf) uses capitalized "PD" and a trailing punctuation; change the
message to be all lowercase with no trailing punctuation (e.g., use "pd
microservice primary exists, pd in pd service mode must yield" or a concise
lowercase variant) so it follows the repository's error-string convention.
In `@server/server.go`:
- Around line 511-514: The current approach lets the PD-service-mode etcd leader
re-acquire PD leadership after allocator yields because allocatorUpdater calls
Reset(true) (which only resigns PD leadership) while leaderLoop will re-campaign
from the current etcd leader; update the yield path so that when s.tsoAllocator
(created in s.tsoAllocator) detects an existing TSO primary and allocatorUpdater
calls Reset(true), you also either (a) prevent leaderLoop from re-campaigning
for that member by setting/clearing a "yielded" flag on the server (e.g., on s)
that leaderLoop checks before starting a new campaign, or (b) perform an etcd
leadership transfer of s.member to another peer before calling Reset(true) so
the yielding node cannot immediately win; implement the chosen approach in the
allocatorUpdater/Reset invocation and ensure leaderLoop observes the yielded
flag (or verify transfer completion) to stop immediate re-campaigns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dcc0a32a-3ca6-4e86-b375-6cc94420454b
📒 Files selected for processing (8)
pkg/storage/endpoint/tso.gopkg/storage/storage_tso_test.gopkg/tso/allocator.gopkg/tso/keyspace_group_manager.gopkg/tso/tso.gopkg/utils/keypath/absolute_key_path.goserver/server.gotests/integrations/tso/client_test.go
💤 Files with no reviewable changes (1)
- tests/integrations/tso/client_test.go
pkg/storage/endpoint/tso.go
Outdated
| if tsoPrimary != "" { | ||
| return errors.Errorf("tso microservice primary exists, PD in PD service mode must yield") | ||
| } |
There was a problem hiding this comment.
Keep the new error string lowercase.
Line 114 introduces capitalized PD, which breaks the repo's error-string convention.
As per coding guidelines, "Error strings must be lowercase with no trailing punctuation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/storage/endpoint/tso.go` around lines 113 - 115, The error string
returned in the conditional checking tsoPrimary (the return in the if tsoPrimary
!= "" branch that calls errors.Errorf) uses capitalized "PD" and a trailing
punctuation; change the message to be all lowercase with no trailing punctuation
(e.g., use "pd microservice primary exists, pd in pd service mode must yield" or
a concise lowercase variant) so it follows the repository's error-string
convention.
| } | ||
|
|
||
| // TSODefaultPrimaryPath returns the primary election path for the default keyspace group's TSO service. | ||
| func TSODefaultPrimaryPath() string { |
There was a problem hiding this comment.
Maybe resue ElectionPath rather than adding a new one?
pkg/storage/endpoint/tso.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if tsoPrimary != "" { |
There was a problem hiding this comment.
Prefer using len cause it also checks the nil case.
| if tsoPrimary != "" { | |
| if len(tsoPrimary) == 0 { |
|
|
||
| // When we upgrade the PD cluster, there may be a period of time that the old and new PDs are running at the same time. | ||
| // So we need another cluster to run this test. | ||
| func TestMixedTSODeployment(t *testing.T) { |
There was a problem hiding this comment.
This test asserts that PD (in PD service mode) and the TSO microservice can serve TSO concurrently as dual writers - which is exactly the unsafe behavior this PR prevents.
Anyway, I should not have removed this test case; it should be FIXED rather than removed. Will address this in follow-up commits.
There was a problem hiding this comment.
Please check the fixed test case :)
Reuse ElectionPath instead of adding a redundant TSODefaultPrimaryPath function, and use len() check for string emptiness per repo convention. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Add TestDualWriterPrevention to verify that a PD leader in PD service mode yields its TSO allocator when a TSO microservice primary is detected, replacing the removed TestMixedTSODeployment which asserted the old (unsafe) dual-writer coexistence behavior. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
2 participants
What problem does this PR solve?
Issue Number: ref #10329
When upgrading from PD service mode (
isKeyspaceGroupEnabled=false) to API service mode with a TSO microservice, there is a window where both the PD leader and the TSO microservice could write to the same TSO timestamp key, causing dual writers and potential timestamp regression.What is changed and how does it work?
In PD service mode,
SaveTimestampnow atomically checks for an existing TSO microservice primary before writing the timestamp window. If a TSO primary is detected, the write is rejected, causing the allocator to reset and resign PD leadership. This allows a PD in API service mode to take over and proxy TSO requests, preventing dual writers during rolling upgrades.Also remove
TestMixedTSODeploymentwhich tested seamless coexistence of PD service mode and TSO microservice — a scenario not supported by the official upgrade procedure (PD restart is required).Key changes:
checkTSOPrimaryparameter toSaveTimestampthat atomically loads the TSO primary election key (/ms/{cluster_id}/tso/00000/primary) within the same etcd transaction before writing the timestamp window. When a TSO primary is detected, the write is rejected.checkTSOPrimaryis only true in PD service mode (!isKeyspaceGroupEnabled). In API service mode, PD uses the existingcheckTSOServicestop/start mechanism instead.TestMixedTSODeployment- seamless coexistence of PD service mode and TSO microservice is not a supported upgrade path.Check List
Tests
Code changes
None.
Side effects
None.
Related changes
None.
Release note
Summary by CodeRabbit
Bug Fixes
Tests