server/join: reject join reusing another member's client URL#11006
server/join: reject join reusing another member's client URL#11006bufferflies wants to merge 9 commits into
Conversation
A new PD member can join with a unique peer URL while advertising a client URL already owned by an existing member. The self-join check only compares --join against the member's own advertise-client-urls, so when --join points to a different string (e.g. a Service URL) it is bypassed, and etcd accepts the unique peer URL. The member list then contains two members sharing one client URL. Add an integration test that starts such an orphan member and asserts that two members end up advertising the same client URL, reproducing the membership corruption. Issue Number: ref tikv#10999 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
release-note-none
|
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:
📝 WalkthroughWalkthroughAdds etcd-backed advertised client URL ownership checks, rejects duplicate URLs before membership changes, adds restart-aware join handling, and tests both fresh-join and restart scenarios. ChangesDuplicate client URL membership
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PrepareJoinCluster
participant retryListEtcdMembers
participant checkClientURLConflict
participant claimClientURLs
participant Etcd
participant MemberAdd
PrepareJoinCluster->>retryListEtcdMembers: list members for restart-aware join
retryListEtcdMembers->>Etcd: request current members
Etcd-->>retryListEtcdMembers: member list or failure
PrepareJoinCluster->>checkClientURLConflict: validate advertised client URLs
checkClientURLConflict->>Etcd: inspect member URLs
Etcd-->>checkClientURLConflict: ownership candidates
PrepareJoinCluster->>claimClientURLs: claim URLs on fresh join
claimClientURLs->>Etcd: create ownership keys transactionally
Etcd-->>claimClientURLs: claim result
PrepareJoinCluster->>MemberAdd: add member after successful validation
🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/server/join/join_test.go (1)
206-212: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrevent double-counting member IDs.
If a member's
ClientURLslist somehow contains the duplicate URL multiple times, its ID would be appended todupOwnersmore than once, potentially satisfying there.Len(dupOwners, 2)assertion with only a single member. Adding abreakensures each member is only counted once.♻️ Proposed refactor
for _, m := range members.Members { for _, u := range m.ClientURLs { if u == dupClientURL { dupOwners = append(dupOwners, m.ID) + break } } }🤖 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 `@tests/server/join/join_test.go` around lines 206 - 212, Update the nested ClientURLs loop in the duplicate-owner collection to break after appending a member ID for dupClientURL, ensuring each member contributes at most once to dupOwners while preserving the existing matching behavior.
🤖 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 `@tests/server/join/join_test.go`:
- Around line 176-178: In the test setup around tests.NewTestCluster, verify err
with re.NoError before registering cleanup. Move defer cluster.Destroy() after
the assertion so cleanup is only scheduled when cluster creation succeeds and
failures preserve the original error.
---
Nitpick comments:
In `@tests/server/join/join_test.go`:
- Around line 206-212: Update the nested ClientURLs loop in the duplicate-owner
collection to break after appending a member ID for dupClientURL, ensuring each
member contributes at most once to dupOwners while preserving the existing
matching behavior.
🪄 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: 5f0ec241-ba42-4878-9e32-3e8aa7c1e815
📒 Files selected for processing (1)
tests/server/join/join_test.go
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/design/duplicate-client-url-membership.md`:
- Around line 60-64: Align the design document’s stated correctness goal with
the implementation actually selected: either narrow the goal to sequential joins
on upgraded nodes, or revise the initial A1 scope to include an authoritative,
concurrency-safe mechanism covering concurrent joins and bypassed join paths.
Update the corresponding goal and scope sections, including the referenced
A1/A1'/A2-c discussion and later guarantees, so they consistently describe the
chosen behavior without claiming deferred guarantees.
- Around line 88-95: Resolve the contradictory A2-c rollout decision throughout
the design document. Make the decision, recommendation, proposed solution,
rollout plan, implementation scope, testing expectations, and operational
guidance consistently state whether A2-c is included in this PR; if deferred,
remove language recommending shipment and clearly keep it as future work.
- Around line 217-227: Update the pseudocode fence around the member
reconciliation example to include an explicit language identifier such as text
or pseudocode, while preserving the existing example content.
- Around line 266-276: Clarify the C2 health semantics in the design section by
explicitly stating how duplicate client URLs affect individual member health,
aggregate health, and quorum decisions without C1; avoid ambiguity about whether
all sharers are marked unhealthy or a separate duplicate condition is reported.
Add tests covering the chosen behavior, including a duplicate URL where one
responder is otherwise healthy.
- Around line 101-108: The design must require the duplicate client-URL check to
run before PrepareJoinCluster performs MemberAdd, preventing rejected fresh
joins from leaving membership behind; alternatively, explicitly define rollback
cleanup and a regression test proving membership is unchanged after rejection.
🪄 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: d8ff3fa7-0bd9-4f46-b2db-c2e6b1233c09
📒 Files selected for processing (1)
docs/design/duplicate-client-url-membership.md
| 1. **Prevent** the common case (sequential join with a duplicated client URL) | ||
| cheaply and with a clear, early error. | ||
| 2. **Guarantee** correctness under concurrent joins and when the join path is | ||
| bypassed. | ||
| 3. **Make existing corruption visible and safe** — an orphan must never be |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align the stated correctness goal with the selected implementation scope.
The design goal requires correctness under concurrent joins and bypassed join paths, but A1 explicitly does not provide those guarantees, while A1' and A2-c are deferred. Either narrow the goal to sequential joins on upgraded nodes or include an authoritative/concurrency-safe mechanism in the initial fix.
Also applies to: 121-131, 314-321
🤖 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 `@docs/design/duplicate-client-url-membership.md` around lines 60 - 64, Align
the design document’s stated correctness goal with the implementation actually
selected: either narrow the goal to sequential joins on upgraded nodes, or
revise the initial A1 scope to include an authoritative, concurrency-safe
mechanism covering concurrent joins and bypassed join paths. Update the
corresponding goal and scope sections, including the referenced A1/A1'/A2-c
discussion and later guarantees, so they consistently describe the chosen
behavior without claiming deferred guarantees.
| - **C2 rides along with A1** (make any residual duplicate visibly unhealthy). | ||
| **C1 / A2** are left as possible future work, not part of the initial fix. | ||
|
|
||
| ## Proposed solution (layered) | ||
|
|
||
| The layers are complementary defense-in-depth. Per the decision above, the | ||
| initial fix is **A1 (mandatory) + C2**; A2 and C1 are documented but deferred. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Resolve the contradictory A2-c rollout decision.
The decision says A2-c is deferred, the recommendation says to ship it, and the rollout plan defers it again. Clarify whether A2-c is part of this PR; otherwise implementation, testing, and operational expectations are inconsistent.
Also applies to: 209-210, 280-289, 314-319
🤖 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 `@docs/design/duplicate-client-url-membership.md` around lines 88 - 95, Resolve
the contradictory A2-c rollout decision throughout the design document. Make the
decision, recommendation, proposed solution, rollout plan, implementation scope,
testing expectations, and operational guidance consistently state whether A2-c
is included in this PR; if deferred, remove language recommending shipment and
clearly keep it as future work.
| ``` | ||
| loop every memberReconcileInterval (leader only): | ||
| members = cluster.GetMembers(s.GetClient()) | ||
| owners = map[clientURL] -> []member // reverse index | ||
| for url, ms in owners where len(ms) > 1: | ||
| trueID = probeSelfID(url) // C1 endpoint; who actually answers | ||
| if trueID unknown: | ||
| warn + metric; continue // cannot identify -> do not remove | ||
| for m in ms where m.ID != trueID: // every other sharer is an orphan | ||
| quarantine(m) // mark unhealthy + warn + metric | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the pseudocode fence.
Use a fence such as text or pseudocode so Markdown lint does not report MD040.
Proposed fix
-```
+```text
loop every memberReconcileInterval (leader only):
members = cluster.GetMembers(s.GetClient())
owners = map[clientURL] -> []member
...📝 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.
| ``` | |
| loop every memberReconcileInterval (leader only): | |
| members = cluster.GetMembers(s.GetClient()) | |
| owners = map[clientURL] -> []member // reverse index | |
| for url, ms in owners where len(ms) > 1: | |
| trueID = probeSelfID(url) // C1 endpoint; who actually answers | |
| if trueID unknown: | |
| warn + metric; continue // cannot identify -> do not remove | |
| for m in ms where m.ID != trueID: // every other sharer is an orphan | |
| quarantine(m) // mark unhealthy + warn + metric | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 217-217: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/design/duplicate-client-url-membership.md` around lines 217 - 227,
Update the pseudocode fence around the member reconciliation example to include
an explicit language identifier such as text or pseudocode, while preserving the
existing example content.
Source: Linters/SAST tools
| ### C2 — Do not report duplicates as healthy (health hardening, backstop) | ||
|
|
||
| Independent of C1, harden `CheckHealth` / `GetHealthStatus`: | ||
|
|
||
| - Pre-scan the member list; when two members share a client URL, log a warning | ||
| and export a metric. | ||
| - Do not credit a single `200` on a shared URL to multiple members. Credit it | ||
| only to the identity-matching member (with C1), or mark the conflicting | ||
| members unhealthy to surface the problem (without C1). | ||
|
|
||
| C2 immediately removes the "orphan appears healthy" illusion and is zero-risk. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='docs/design/duplicate-client-url-membership.md'
echo '--- file size ---'
wc -l "$file"
echo '--- surrounding context around lines 220-320 ---'
sed -n '220,320p' "$file" | cat -n
echo '--- search for C1/C2 and health-related wording ---'
rg -n 'C1|C2|health|healthy|unhealthy|duplicate|client URL|quorum|availability' "$file"Repository: tikv/pd
Length of output: 10903
Define C2’s health semantics explicitly.
Without C1, “mark the conflicting members unhealthy” can also penalize the healthy responder, turning a duplicate URL into a false availability/quorum failure in small clusters. Spell out whether C2 reports a separate duplicate condition, affects only aggregate health, or intentionally marks every sharer unhealthy, and add test coverage for that behavior.
🤖 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 `@docs/design/duplicate-client-url-membership.md` around lines 266 - 276,
Clarify the C2 health semantics in the design section by explicitly stating how
duplicate client URLs affect individual member health, aggregate health, and
quorum decisions without C1; avoid ambiguity about whether all sharers are
marked unhealthy or a separate duplicate condition is reported. Add tests
covering the chosen behavior, including a duplicate URL where one responder is
otherwise healthy.
aabb2cb to
fa03d1a
Compare
ti-chi-bot
Bot
added
size/M
A joining member could pass with a unique peer URL while advertising a client URL already owned by an existing member, because MemberAdd only carries peer URLs and etcd does not enforce client-URL uniqueness. The result was two members sharing one client URL, corrupting membership and health semantics. Add a standalone checkAndClaimClientURLs invoked before MemberAdd. It lists the current members and rejects the join if the advertised client URL is already used by another member, then atomically claims each URL via an etcd transaction so concurrent joiners cannot both take the same URL. Flip the reproduction test into a regression test asserting the join is rejected and membership is untouched. Issue Number: close tikv#10999 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
size/L
bufferflies
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/server/join/join_test.go (1)
187-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the registry transaction rather than only the member-list guard.
Because
dupClientURLalready appears inpd1.ClientURLs, this test returns from layer 1 and never tests atomic claiming. Add concurrent joiners targeting an initially unowned URL and assert exactly one reaches membership; also cover cleanup after an injectedMemberAddfailure.🤖 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 `@tests/server/join/join_test.go` around lines 187 - 198, Extend the join tests around cluster.Join to use concurrent joiners targeting an initially unowned client URL, asserting exactly one succeeds and appears in membership to exercise atomic registry claiming. Add a separate failure-injection case for MemberAdd and verify the claimed URL is released afterward, allowing a subsequent join to succeed.
🤖 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 `@server/join/client_url.go`:
- Around line 53-55: Update the URL-claim ownership checks in the join flow,
including the logic around the registry entry and MemberAdd, so claim identity
is based on the member name plus stable advertised peer URL identity rather than
the name alone. Allow reclaiming an existing claim only when both the name and
peer URL(s) match; treat same-name claims with different or concurrent peer
identities as conflicts.
- Around line 85-110: Make advertised URL ownership transactional across join
completion: in server/join/client_url.go:85-110, update the client URL claim
flow to claim the complete URL set atomically and provide a conditional release
operation for this incarnation’s claims. In server/join/join.go:159-167, invoke
that release operation whenever MemberAdd fails, preserving claims owned by
other members.
- Around line 60-66: Update checkAndClaimClientURLs to accept a caller context,
pass the context from PrepareJoinCluster through the join path, and replace
NewSlowLogTxn with NewSlowLogTxnWithContext using that context so cancellation
is honored.
---
Nitpick comments:
In `@tests/server/join/join_test.go`:
- Around line 187-198: Extend the join tests around cluster.Join to use
concurrent joiners targeting an initially unowned client URL, asserting exactly
one succeeds and appears in membership to exercise atomic registry claiming. Add
a separate failure-injection case for MemberAdd and verify the claimed URL is
released afterward, allowing a subsequent join to succeed.
🪄 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: 59e6ffa4-fc7f-4aca-88e9-40ec700a9393
📒 Files selected for processing (3)
server/join/client_url.goserver/join/join.gotests/server/join/join_test.go
| func checkAndClaimClientURLs( | ||
| client *clientv3.Client, | ||
| clusterID uint64, | ||
| name string, | ||
| advertiseClientURLs []string, | ||
| members []*etcdserverpb.Member, | ||
| ) error { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- server/join/client_url.go (lines 1-220) ---'
sed -n '1,220p' server/join/client_url.go | cat -n
echo
echo '--- search for checkAndClaimClientURLs ---'
rg -n "checkAndClaimClientURLs|ClaimClientURLs|clientURL" server -SRepository: tikv/pd
Length of output: 5792
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'server/join/client_url.go' && echo FOUNDRepository: tikv/pd
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- server/join/join.go (around call site) ---'
sed -n '130,210p' server/join/join.go | cat -n
echo
echo '--- pkg/storage/kv search ---'
rg -n "func NewSlowLogTxn|type .*Txn|func .*Commit\(|context.Context|WithContext|NewTxn|SlowLogTxn" pkg/storage/kv -SRepository: tikv/pd
Length of output: 5754
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '135,185p' pkg/storage/kv/etcd_kv.go | cat -nRepository: tikv/pd
Length of output: 1997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' server/join/join.go | cat -nRepository: tikv/pd
Length of output: 5740
Thread a caller context through this join path. This helper still falls back to client.Ctx() via NewSlowLogTxn, so it won’t honor caller cancellation. Pass ctx from PrepareJoinCluster and use NewSlowLogTxnWithContext(ctx, client).
🤖 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/join/client_url.go` around lines 60 - 66, Update
checkAndClaimClientURLs to accept a caller context, pass the context from
PrepareJoinCluster through the join path, and replace NewSlowLogTxn with
NewSlowLogTxnWithContext using that context so cancellation is honored.
Source: Coding guidelines
… url Extend the duplicate advertise-client-urls guard to also run on a restart that changed its client URL (data-dir path), before the local etcd republishes, so such a change cannot slip a duplicate into the member list. The restart path is best-effort: an unreachable cluster does not block a member from starting off its local data. Key the client-url registry claim by the owner's peer URLs (globally unique, stable across restarts) instead of its name, so two concurrent joiners sharing a name are still rejected. Match self by peer URL in the member-list comparison for the same reason. Add a regression test for a restart with a modified, duplicated client URL. Issue Number: close tikv#10999 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/join/join.go (1)
129-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the error returned by
client.Close().As per coding guidelines, resources should be closed with
deferand their errors should be checked. Theclient.Close()method returns an error that is currently being silently ignored.♻️ Proposed refactor
- defer client.Close() + defer func() { + if err := client.Close(); err != nil { + log.Error("failed to close etcd client", errs.ZapError(err)) + } + }()🤖 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/join/join.go` at line 129, Update the deferred cleanup around client.Close in the join flow to capture and handle the error returned by client.Close instead of silently discarding it, using the surrounding function’s established error-reporting or return pattern.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 `@server/join/join.go`:
- Line 129: Update the deferred cleanup around client.Close in the join flow to
capture and handle the error returned by client.Close instead of silently
discarding it, using the surrounding function’s established error-reporting or
return pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b7b9773-506b-48f1-b6bf-b7e233bca9c5
📒 Files selected for processing (3)
server/join/client_url.goserver/join/join.gotests/server/join/join_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/server/join/join_test.go
…estart Split the client-URL guard so the write path cannot block cluster recovery: the read-only conflict check runs on every startup, while the etcd registry claim (a write needing quorum) runs only on the fresh-join path where quorum is required anyway. Otherwise a member restarting during a quorum loss could hang on the claim and fail to start. On the restart path, retry ListEtcdMembers with exponential backoff (3 times, ~20s) before skipping the best-effort check, so a transient failure does not silently bypass duplicate detection. Issue Number: close tikv#10999 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/join/join.go (2)
57-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winImplement context-aware backoff for retries. As per coding guidelines, functions making external effects should take a
context.Contextas their first parameter, and retries should use context-aware timeouts and backoffs. Currently,time.Sleepblocks without respecting context cancellation.
server/join/join.go#L57-L77: Addctx context.Contextas the first parameter and replacetime.Sleep(backoff)withselect { case <-ctx.Done(): return nil, ctx.Err(); case <-time.After(backoff): }. Passctxtoetcdutil.ListEtcdMembers.server/join/join.go#L173-L173: Update the call site to pass the client context:retryListEtcdMembers(client.Ctx(), client).🤖 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/join/join.go` around lines 57 - 77, Make retryListEtcdMembers context-aware by adding a context.Context first parameter, replacing time.Sleep with a context cancellation select that returns ctx.Err(), and passing ctx to etcdutil.ListEtcdMembers. Update server/join/join.go lines 57-77 accordingly; at server/join/join.go line 173, pass client.Ctx() to retryListEtcdMembers.Source: Coding guidelines
188-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse parsed URL slices to avoid repeated string splitting.
cfg.AdvertiseClientUrlsandcfg.AdvertisePeerUrlsare repeatedly split into slices across the function. Parsing them once before their first use improves clarity and avoids redundant allocations.
server/join/join.go#L188-L194: Hoist thestrings.Splitcalls into local variables (advertiseClientURLsandadvertisePeerURLs) right beforecheckClientURLConflictand pass the variables.server/join/join.go#L209-L209: Remove the inline declaration ofadvertisePeerURLssince it is already defined above.server/join/join.go#L236-L245: Pass the reusedadvertiseClientURLsandadvertisePeerURLsvariables intoclaimClientURLs.🤖 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/join/join.go` around lines 188 - 194, Parse cfg.AdvertiseClientUrls and cfg.AdvertisePeerUrls once into advertiseClientURLs and advertisePeerURLs before checkClientURLConflict in server/join/join.go:188-194, then reuse both variables for claimClientURLs at server/join/join.go:236-245. Remove the later inline advertisePeerURLs declaration at server/join/join.go:209 because the earlier parsed variable replaces it.
🤖 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/join/join.go`:
- Around line 57-77: Make retryListEtcdMembers context-aware by adding a
context.Context first parameter, replacing time.Sleep with a context
cancellation select that returns ctx.Err(), and passing ctx to
etcdutil.ListEtcdMembers. Update server/join/join.go lines 57-77 accordingly; at
server/join/join.go line 173, pass client.Ctx() to retryListEtcdMembers.
- Around line 188-194: Parse cfg.AdvertiseClientUrls and cfg.AdvertisePeerUrls
once into advertiseClientURLs and advertisePeerURLs before
checkClientURLConflict in server/join/join.go:188-194, then reuse both variables
for claimClientURLs at server/join/join.go:236-245. Remove the later inline
advertisePeerURLs declaration at server/join/join.go:209 because the earlier
parsed variable replaces it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1eca7846-0d4d-43dc-a406-1875f1f5418b
📒 Files selected for processing (2)
server/join/client_url.goserver/join/join.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/join/client_url.go
| // Atomically claim the advertised client URLs before MemberAdd so two | ||
| // concurrent joiners cannot both take the same client URL. Only the | ||
| // fresh-join path reaches here, where quorum is required anyway. | ||
| if err := claimClientURLs( |
There was a problem hiding this comment.
This write also runs when joinedFailedToStart is true. That recovery path may already be at 1/2 quorum, so the transaction fails and prevents the missing member from starting. TestFailedToStartPDAfterSuccessfulJoin already reproduces this; keep this recovery path write-free.
There was a problem hiding this comment.
Fixed in f744aa4 — the registry claim (the only write) is removed entirely, so the joinedFailedToStart recovery path no longer writes and can't hang on quorum. TestFailedToStartPDAfterSuccessfulJoin passes locally now.
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| for _, url := range advertiseClientURLs { |
There was a problem hiding this comment.
URLs are claimed one transaction at a time and never released if a later claim or MemberAdd fails, or when the member is removed. This leaves permanent orphan claims and blocks legitimate replacements. Claim the full set atomically and clean it up with the member lifecycle.
There was a problem hiding this comment.
Resolved by removing the persistent claim in f744aa4 — there is no registry left to leak or reconcile. Making membership uniqueness authoritative (an atomic reservation bound to the member lifecycle, with rollback on failure and cleanup on removal) is tracked as a follow-up.
| cfg.InitialClusterState = embed.ClusterStateFlagExisting | ||
| return nil | ||
| } | ||
| dataExists := isDataExist(filepath.Join(cfg.DataDir, "member")) |
There was a problem hiding this comment.
PrepareJoinCluster returns above when cfg.Join == "", so this check does not actually run on every startup. A member bootstrapped with initial-cluster can still restart with another member client URL. Please cover that path or narrow the claimed guarantee.
There was a problem hiding this comment.
Agreed — narrowed the claim. The read-only check now covers only the --join and restart-with-data paths; members bootstrapped with --initial-cluster (which return early) are listed as a follow-up, and the PR is downgraded to ref #10999.
| // data directory, nothing more to prepare here. The registry claim is | ||
| // intentionally skipped on this path — it writes to etcd (needs quorum), and | ||
| // a restart must not depend on quorum. | ||
| if dataExists { |
There was a problem hiding this comment.
This is still a check-then-publish race: restarting members skip the registry claim, so two restarts—or a restart and a fresh join—can both pass before either publishes the new URL. The PR can still create duplicate client URLs under concurrency.
There was a problem hiding this comment.
Correct. With the registry removed the check is read-only and does not close the concurrent pre-publish window, so I stopped claiming that guarantee. Concurrency-authoritative uniqueness (the atomic reservation + lifecycle reconciliation) is a follow-up — hence ref, not close.
| // another member. This is read-only (no quorum needed) and runs before the | ||
| // local etcd (re)starts and republishes its attributes, so a duplicate never | ||
| // enters the member list. See issue #10999. | ||
| if err := checkClientURLConflict( |
There was a problem hiding this comment.
#10999 also calls for health checks to verify the responding member identity. /pd/api/v1/ping still exposes no identity, so an orphan sharing a live URL remains falsely healthy. Please address this before closing the issue, or track it separately and change this PR to ref.
There was a problem hiding this comment.
Agreed, not addressed here. Changed to ref #10999 and listed responder-identity verification in health checks as a follow-up so an orphan sharing a live URL is not reported healthy.
YuhaoZhang00
left a comment
YuhaoZhang00
left a comment
There was a problem hiding this comment.
General feedback: The persistent client-URL claim records are not updated atomically with etcd membership, which might cause :-
(1) failed joins, member removal, or URL changes can leave stale claims that block legitimate joins, and
(2) claim writes can also prevent an already-added peer-only member from starting when quorum is unavailable.
Suggested changes:
- Separate new-member admission from peer-only recovery so an already-added member can start without a quorum-dependent claim write.
- Model client-URL claims as recoverable reservations around
MemberAdd: reserve all URLs atomically, bind them to the member ID on success, and conditionally roll them back on confirmed failure. - Reconcile reservations with membership for existing members, ambiguous joins, URL changes, and member removal.
The persistent client-URL registry claim (an etcd write) could block recovery: it also ran on the joinedFailedToStart path, which may be at 1/2 quorum, so the transaction failed and prevented the member from starting (reproduced by TestFailedToStartPDAfterSuccessfulJoin). It also left orphan claims that were never released on failure or member removal. Remove the registry entirely and keep only the read-only member-list conflict check, which rejects a join or a restart-with-data whose advertised client URL is already used by another published member. This covers the reported scenario without any quorum-dependent write. Making membership uniqueness authoritative under concurrency (an atomic reservation bound to the member lifecycle), covering initial-cluster nodes, and verifying responder identity in health checks are left as follow-ups; downgrade to ref accordingly. Also fix test cleanup ordering to not mask a cluster-creation failure. Issue Number: ref tikv#10999 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
| } | ||
| defer client.Close() | ||
|
|
||
| listResp, err := etcdutil.ListEtcdMembers(client.Ctx(), client) |
There was a problem hiding this comment.
This safety check is based on a non-linearizable MemberList. etcdutil.ListEtcdMembers explicitly sends Linearizable: false, so an isolated or lagging endpoint may return a local membership view that has not applied another member's latest ClientURLs.
I reproduced the control flow with a fake Cluster service: the first MemberList returned a stale member without its ClientURLs, MemberAdd then succeeded, and a later MemberList contained the duplicate URL. PrepareJoinCluster returned success because the conflict check is never repeated.
This means even a sequential fresh join can bypass the guard; it is not limited to the documented concurrent pre-publish race. Since fresh MemberAdd already requires quorum, please use a linearizable MemberList for fresh-join admission, or otherwise make the admission decision authoritative.
| errs.ZapError(err)) | ||
| cfg.InitialCluster = initialCluster | ||
| cfg.InitialClusterState = embed.ClusterStateFlagExisting | ||
| return nil |
There was a problem hiding this comment.
Returning nil here permanently bypasses the restart conflict check. I verified this with a Cluster service whose MemberList always fails: after the retries, PrepareJoinCluster returns success even when the configured client URL is known to be duplicated.
The local etcd then starts with that configuration, and there is no revalidation when connectivity or quorum later recovers before it publishes its attributes. Therefore the PR cannot generally guarantee that a restart with a duplicate client URL is rejected.
Please either add post-recovery/pre-publication reconciliation, or narrow this PR to fresh joins and avoid claiming restart enforcement in the title and release note.
| err error | ||
| ) | ||
| backoff := restartListMemberBackoff | ||
| for i := range restartListMemberRetryTimes { |
There was a problem hiding this comment.
Beyond the previously mentioned cancellation issue, the total pre-etcd startup delay is much larger than the comment states. There is one initial MemberList plus three retries, each with a 10s request timeout, and sleeps of 2s, 4s, and 8s:
10 + 2 + 10 + 4 + 10 + 8 + 10 = 54 seconds
I verified this with a healthy gRPC connection whose MemberList handler waits for each request context to expire; PrepareJoinCluster took 54.097s. This all happens before embedded etcd starts, whereas the previous data-directory path returned immediately. The function also has no caller context, so the delay cannot be cancelled gracefully.
This is a significant quorum-recovery regression. Please avoid synchronous remote retries before starting an existing member, or enforce a much smaller total deadline consistent with the recovery requirement.
| continue | ||
| } | ||
| for _, owned := range m.ClientURLs { | ||
| if owned == url { |
There was a problem hiding this comment.
Comparing raw URL strings does not detect semantically equivalent endpoints. I verified that an existing http://pd.example:2379 and a joining http://PD.EXAMPLE:2379 pass this check, although DNS hostnames are case-insensitive and both URLs reach the same endpoint.
Please parse and canonicalize URLs before comparison, at least normalizing the scheme and hostname, and define the expected behavior for trailing DNS dots and equivalent IPv6 representations. A table-driven unit test for these cases would make the intended uniqueness semantics explicit.
| for _, m := range members { | ||
| // Skip only our own entry, matched by peer URLs so a different | ||
| // member sharing our name is still checked. | ||
| if slice.EqualWithoutOrder(m.PeerURLs, advertisePeerURLs) { |
There was a problem hiding this comment.
On restart, this uses the configured peer URLs to identify the local member before embedded etcd restores its persisted identity. I'm not sure if this is the case, but if the configured peer URLs differ from the member-list record, the member treats its own record as another member and rejects its unchanged client URL as a duplicate, blocking a legitimate restart and potentially quorum recovery.
Comparing raw URL strings missed semantically equivalent endpoints, e.g. http://pd.example:2379 and http://PD.EXAMPLE:2379 (DNS is case-insensitive) would not be detected as the same client URL. Canonicalize scheme and hostname (lower-case, strip a trailing dot) and reduce IP literals — notably IPv6 — to their canonical form before comparison, for both the client-URL match and the peer-URL self-exclusion. Add table-driven unit tests covering case, trailing dot, and IPv6 equivalence. Issue Number: ref tikv#10999 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
size/XL
… list Address review: the restart-path enforcement caused recovery regressions — a synchronous ~54s pre-etcd retry, a best-effort skip that permanently bypassed the check when MemberList kept failing, and fragile self- identification before etcd restores the member ID. Restore the immediate data-directory return so a restart makes no remote call and is never delayed or blocked. For a genuinely new member (not the joinedFailedToStart recovery path), run the client-URL conflict check against a linearizable member list before MemberAdd, so a stale or lagging local view cannot miss a conflict that another member already published. The recovery path skips it and so never depends on quorum. Enforcing uniqueness on restart authoritatively (needs the persisted member ID and a pre-publish check point) and under concurrency remain follow-ups. Drop the now-obsolete restart test. Issue Number: ref tikv#10999 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
| // - A deleted PD joins to previous cluster. | ||
| { | ||
| // No need to add member if the PD is already in the cluster. | ||
| if !joinedFailedToStart { |
There was a problem hiding this comment.
This still leaves the joinedFailedToStart recovery path able to publish a duplicate client URL.
A concrete sequence is:
- pd1 is running and publishes client URL A.
MemberAddsucceeds for pd2's peer URL, but pd2 never starts, so the member list contains an unnamed pd2 entry.- pd2 is retried with the same peer URL but
AdvertiseClientUrls = A. - The initial non-linearizable
MemberListmarks this asjoinedFailedToStart, then this branch skipscheckClientURLConflict. - pd2 starts and publishes A, so two members now advertise the same client URL.
I verified this with the temporary integration test below. It passes on this PR head (85ee1e1f59e7c2dcfff024ed1fe2fc105eb04f38), and the final assertion observes two owners for the same client URL:
func TestReviewFailedStartRecoveryWithDuplicateClientURL(t *testing.T) {
re := require.New(t)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
cluster, err := tests.NewTestCluster(ctx, 1)
re.NoError(err)
defer cluster.Destroy()
re.NoError(cluster.RunInitialServers())
re.NotEmpty(cluster.WaitLeader())
pd1 := cluster.GetServer("pd1")
client := pd1.GetEtcdClient()
dupClientURL := pd1.GetConfig().AdvertiseClientUrls
// First join succeeds through MemberAdd but the new PD never starts, leaving
// an unnamed etcd member with pd2's peer URL.
pd2, err := cluster.Join(ctx)
re.NoError(err)
pd2Cfg := pd2.GetConfig().Clone()
re.NoError(pd2.Destroy())
// Retrying the same peer with another member's advertised client URL should
// be rejected before startup. The current implementation skips the check for
// joinedFailedToStart and allows the duplicate to be published.
pd2Cfg.AdvertiseClientUrls = dupClientURL
retryPD2, err := tests.NewTestServer(ctx, pd2Cfg, nil)
re.NoError(err)
defer retryPD2.Destroy()
re.NoError(retryPD2.Run())
members, err := etcdutil.ListEtcdMembers(ctx, client)
re.NoError(err)
dupOwners := make([]uint64, 0, 2)
for _, m := range members.Members {
for _, u := range m.ClientURLs {
if u == dupClientURL {
dupOwners = append(dupOwners, m.ID)
break
}
}
}
re.Len(dupOwners, 2)
}Command used:
go test -tags without_dashboard,deadlock ./tests/server/join -run TestReviewFailedStartRecoveryWithDuplicateClientURL -count=1Result:
ok github.com/tikv/pd/tests/server/join 20.975s
This does not need a quorum-dependent write to fix. The already-fetched listResp contains the currently published client URLs, so the recovery path can still run the read-only conflict check against that list while preserving the low-quorum recovery behavior.
| return listEtcdMembers(ctx, client, true) | ||
| } | ||
|
|
||
| func listEtcdMembers(ctx context.Context, client *clientv3.Client, linearizable bool) (*clientv3.MemberListResponse, error) { |
Run the read-only advertise-client-urls conflict check on the joinedFailedToStart recovery path as well, using the already-fetched non-linearizable member list so recovery is never blocked on quorum. Previously that path skipped the check, so a member whose peer was added but never started could re-join with another member's client URL and publish a duplicate. Add a regression test. Fix CI (statics): rename the private etcdutil helper to queryEtcdMembers (revive confusing-naming vs ListEtcdMembers) and fix the import order in client_url_test.go (gci). Issue Number: ref tikv#10999 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
YuhaoZhang00
left a comment
YuhaoZhang00
left a comment
There was a problem hiding this comment.
Please update PR body to match latest implementation
|
@YuhaoZhang00: adding LGTM is restricted to approvers and reviewers in OWNERS files. 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. |
| host = ip.String() | ||
| } | ||
| hostport := host | ||
| if port := u.Port(); port != "" { |
There was a problem hiding this comment.
Blocking: numeric ports are not canonicalized, so this still allows a sequential fresh join to publish the same client endpoint under a different spelling. For example, http://127.0.0.1:2379 and http://127.0.0.1:02379 are both accepted by Go and etcd and dial the same TCP port, but this function produces different comparison keys.
I verified the comparison with this temporary unit test. It fails on this head because checkClientURLConflict returns nil:
func TestReviewLeadingZeroPortBypassesConflictCheck(t *testing.T) {
re := require.New(t)
plain, err := net.ResolveTCPAddr("tcp", "127.0.0.1:2379")
re.NoError(err)
zeroPadded, err := net.ResolveTCPAddr("tcp", "127.0.0.1:02379")
re.NoError(err)
re.Equal(plain.Port, zeroPadded.Port)
members := []*etcdserverpb.Member{{
ID: 1,
Name: "pd1",
PeerURLs: []string{"http://127.0.0.1:2380"},
ClientURLs: []string{"http://127.0.0.1:2379"},
}}
err = checkClientURLConflict(
[]string{"http://127.0.0.1:02379"},
[]string{"http://127.0.0.1:3380"},
members,
)
re.Error(err, "the same TCP endpoint must be rejected despite zero padding")
}I also verified this end to end: pd1 advertised the normal form; pd2 used a unique peer URL and the zero-padded form. cluster.Join and pd2.Run both succeeded, and a linearizable MemberList contained both advertised URL spellings. The temporary integration test passed with:
go test -tags without_dashboard,deadlock ./tests/server/join -run TestReviewLeadingZeroPortBypassesDuplicateClientURLCheck -count=1
ok github.com/tikv/pd/tests/server/join 27.690s
Please normalize a valid numeric port before comparison, then add both the canonicalization case and the fresh-join regression test.
Signed-off-by: bufferflies <1045931706@qq.com>
|
The PR description is still out of sync with the implementation.
However, the title, problem statement, implementation section, commit-message block, test list, and release note still claim restart-with-data enforcement. The body also describes retrying MemberList on restart and lists Please either restore restart enforcement, or update the title, PR body, test list, and release note to state the narrower guarantee accurately. Otherwise the release note overstates the protection users receive. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024, YuhaoZhang00 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:
|
bufferflies
changed the title
|
@lhy1024 @YuhaoZhang00 updated the PR title, description and release note to match the implementation. The stated coverage is now the no-data join paths only:
Explicitly listed as not covered / follow-ups: a restart with existing data ( |
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11006 +/- ##
==========================================
+ Coverage 79.22% 79.27% +0.04%
==========================================
Files 541 542 +1
Lines 75965 76102 +137
==========================================
+ Hits 60187 60329 +142
+ Misses 11531 11526 -5
Partials 4247 4247
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| // Genuinely new member: use a linearizable list so a stale/lagging local | ||
| // view cannot miss a conflict that has already been published; a genuine | ||
| // join needs quorum for MemberAdd anyway. | ||
| linResp, lerr := etcdutil.ListEtcdMembersLinearizable(client.Ctx(), client) |
There was a problem hiding this comment.
The updated PR body should not say that "no path is blocked on quorum." This call is deliberately linearizable and fails without quorum; the MemberAdd below also requires quorum. Please scope that statement to the joinedFailedToStart recovery path: it adds no quorum-dependent operation, whereas a genuinely new join still requires quorum.
|
/ping @JmPotato |
4 participants
What problem does this PR solve?
Issue Number: ref #10999
A PD member could join a cluster while advertising a
--advertise-client-urlsvalue already used by another member, because
MemberAddcarries only peerURLs and etcd does not enforce client-URL uniqueness, while the self-join guard
only compares
--joinagainst the member's own advertised client URL. When--joinpoints to a different string (e.g. a Kubernetes Service URL instead ofthe duplicated per-Pod client URL), the guard is bypassed and etcd accepts the
unique peer URL. The member list then contained two members sharing one client
URL, corrupting membership and health semantics (an orphan member could still
appear healthy because its client URL routes to the healthy original PD).
What is changed and how does it work?
Add a standalone, read-only
checkClientURLConflict(
server/join/client_url.go), invoked fromPrepareJoinClusteron the no-datajoin paths, before
MemberAddand before the local etcd publishes itsattributes. It rejects the startup if this member's advertised client URL is
already used by any other member. "Other" is matched by peer URL (unique per
member), so a member that happens to share our name is still checked. URLs are
compared after canonicalization (lower-cased scheme/host, trailing DNS dot
stripped, IP literals — notably IPv6 — and numeric ports normalized), so
case-only or format-only spellings of the same endpoint are still detected.
Two entry points, chosen so a recovery never depends on quorum:
stale/lagging local view cannot miss an already-published conflict. A genuine
join needs quorum for
MemberAddanyway.joinedFailedToStart, i.e. the peer wasadded but never started, so the cluster may be at 1/2 quorum) — checked against
the non-linearizable list already fetched, issuing no quorum-dependent
call, so recovery is never blocked.
The check itself issues no etcd write. A genuinely new join still requires
quorum — both the linearizable member list and
MemberAddfail without it — butthe failed-start recovery re-join adds no quorum-dependent operation, so a member
recovering at 1/2 quorum is never blocked by this check.
Scope / follow-ups (why
ref, notclose)PrepareJoinClusterreturns immediately when the data directory exists, so thisPR covers only the no-data join paths. The following are not covered here
and are tracked separately:
MemberListcall anddoes not re-validate
advertise-client-urls.--initial-cluster(--joinempty), which returnbefore this check.
needs an authoritative reservation bound to the member lifecycle (atomic
reserve around
MemberAdd, rollback on failure, reconcile on removal).sharing a live URL is not reported healthy.
Tests
TestJoinWithDuplicateClientURL— a fresh join reusing a live member's clientURL is rejected; membership is untouched.
TestFailedStartRecoveryRejectsDuplicateClientURL— a failed-start recoveryre-join reusing another member's client URL is rejected (non-linearizable,
works at 1/2 quorum).
TestCanonicalizeURL/TestCheckClientURLConflict— table-driven unit testsfor case, trailing dot, IPv6, and numeric-port (e.g.
02379) equivalence, andpeer-URL self-exclusion.
TestSimpleJoin/TestFailedToStartPDAfterSuccessfulJoin— legitimate joinsand the low-quorum recovery path are unaffected.
Check List
Tests
Code changes
Side effects
Related changes
Release note