cluster: reject duplicated store peer address on put#11007
cluster: reject duplicated store peer address on put#11007JaySon-Huang wants to merge 2 commits into
Conversation
Prevent two live stores from registering the same non-empty peer_address, which can misroute Raft traffic (ref tikv#11002). Signed-off-by: JaySon-Huang <tshent@qq.com>
Also forbid a store's address/peer_address from matching another live store's peer_address/address to avoid Raft misrouting. Signed-off-by: JaySon-Huang <tshent@qq.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 |
|
Hi @JaySon-Huang. 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
needs-ok-to-test
📝 WalkthroughWalkthroughStore registration now rejects collisions between store addresses and non-empty peer addresses. Tests cover active, offline, destroyed, and tombstoned stores, same-store updates, empty peer addresses, and cross-collision cases. ChangesPeer address uniqueness
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/cluster/cluster.go (1)
1526-1547: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract repeated method calls outside the loop for cleaner readability.
The loop evaluates
store.GetAddress()andstore.GetPeerAddress()multiple times per iteration. While these are fast inlineable field accesses, extracting them into local variables before the loop improves readability and slightly reduces redundant method calls.💡 Proposed refactor
// TiFlash uses peer address for Raft replication, so a non-empty peer // address must also not collide with any other store's address (and vice versa). // Empty peer address is common for TiKV and should not participate in conflict checks. + storeAddr := store.GetAddress() + storePeerAddr := store.GetPeerAddress() for _, s := range c.GetStores() { // It's OK to start a new store on the same address if the old store has been removed or physically destroyed. if s.IsRemoved() || s.IsPhysicallyDestroyed() { continue } if s.GetID() == store.GetId() { continue } existingAddr := s.GetAddress() existingPeerAddr := s.GetMeta().GetPeerAddress() - if store.GetAddress() == existingAddr || - (existingPeerAddr != "" && store.GetAddress() == existingPeerAddr) { + if storeAddr == existingAddr || + (existingPeerAddr != "" && storeAddr == existingPeerAddr) { return errors.Errorf("duplicated store address: %v, already registered by %v", store, s.GetMeta()) } - if store.GetPeerAddress() != "" && - (store.GetPeerAddress() == existingAddr || store.GetPeerAddress() == existingPeerAddr) { + if storePeerAddr != "" && + (storePeerAddr == existingAddr || storePeerAddr == existingPeerAddr) { return errors.Errorf("duplicated store peer address: %v, already registered by %v", store, s.GetMeta()) } }🤖 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 `@server/cluster/cluster.go` around lines 1526 - 1547, In the store-address conflict-checking logic, extract store.GetAddress() and store.GetPeerAddress() into local variables before the loop over c.GetStores(), then use those variables throughout the comparisons while preserving the existing empty-peer-address behavior and errors.
🤖 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 `@server/cluster/cluster.go`:
- Around line 1526-1547: In the store-address conflict-checking logic, extract
store.GetAddress() and store.GetPeerAddress() into local variables before the
loop over c.GetStores(), then use those variables throughout the comparisons
while preserving the existing empty-peer-address behavior and errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c75620a1-a43f-4f20-8e76-fa1a75912303
📒 Files selected for processing (2)
server/cluster/cluster.goserver/cluster/cluster_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11007 +/- ##
=======================================
Coverage 79.22% 79.23%
=======================================
Files 541 541
Lines 75965 75991 +26
=======================================
+ Hits 60187 60211 +24
- Misses 11531 11548 +17
+ Partials 4247 4232 -15
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
What problem does this PR solve?
Issue Number: Close #11002
PD only enforced uniqueness of a store's main
addressduring registration.A new store could still be accepted with a unique main address but a
peer_addressalready used by another live store (or colliding with anotherstore's address). Raft prefers
peer_address, so messages can be misroutedand cause
StoreNotMatch, stuck placement operators, and unavailableTiFlash replicas.
What is changed and how does it work?
Check List
Tests
Related changes
Release note
Summary by CodeRabbit