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 4 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
|
[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/M
|
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
| cluster, err := tests.NewTestCluster(ctx, 1) | ||
| defer cluster.Destroy() | ||
| re.NoError(err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent nil pointer panic on test failure.
If tests.NewTestCluster encounters an error, cluster will be nil. Calling defer cluster.Destroy() before verifying the error will result in a nil pointer panic during test cleanup, which masks the original failure reason. Move the defer to execute only after asserting that no error occurred.
🐛 Proposed fix
cluster, err := tests.NewTestCluster(ctx, 1)
- defer cluster.Destroy()
re.NoError(err)
+ defer cluster.Destroy()📝 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.
| cluster, err := tests.NewTestCluster(ctx, 1) | |
| defer cluster.Destroy() | |
| re.NoError(err) | |
| cluster, err := tests.NewTestCluster(ctx, 1) | |
| re.NoError(err) | |
| defer cluster.Destroy() |
🤖 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 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.
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.
bufferflies
force-pushed
the
claude/github-issue-review-72bf0f
branch
from
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
| // Layer 2: atomically claim each URL. | ||
| for _, url := range advertiseClientURLs { | ||
| key := clientURLRegistryPath(clusterID, url) | ||
| resp, err := kv.NewSlowLogTxn(client). | ||
| If(clientv3.Compare(clientv3.CreateRevision(key), "=", 0)). | ||
| Then(clientv3.OpPut(key, name)). | ||
| Else(clientv3.OpGet(key)). | ||
| Commit() | ||
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| if resp.Succeeded { | ||
| continue // freshly claimed | ||
| } | ||
| // The key already exists; make sure we are the owner. | ||
| kvs := resp.Responses[0].GetResponseRange().Kvs | ||
| if len(kvs) == 0 { | ||
| return errors.Errorf("failed to claim advertise-client-urls %q, please retry", url) | ||
| } | ||
| if owner := string(kvs[0].Value); owner != name { | ||
| return errors.Errorf( | ||
| "advertise-client-urls %q is already claimed by member %q", url, owner) | ||
| } | ||
| log.Info("re-claimed an advertised client URL owned by this member", | ||
| zap.String("name", name), zap.String("client-url", url)) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Tie persistent URL ownership to successful join completion.
Claims currently survive partial validation and downstream join failures, leaving registry entries without corresponding members.
server/join/client_url.go#L85-L110: claim the complete URL set atomically and expose conditional release.server/join/join.go#L159-L167: release this incarnation’s claims wheneverMemberAddfails.
📍 Affects 2 files
server/join/client_url.go#L85-L110(this comment)server/join/join.go#L159-L167
🤖 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 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.
… 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
|
@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. |
| // 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.
| 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.
| 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.
| // 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.
| // 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.
2 participants
What problem does this PR solve?
Issue Number: close #10999
A PD member could join a cluster with a unique peer URL while advertising a
--advertise-client-urlsvalue already owned by an existing member. Themember list then contained two logical members sharing one client URL, which
corrupts membership and health semantics.
Root cause:
MemberAddcarries only peer URLs and etcd does not enforceclient-URL uniqueness, while the self-join guard only compares
--joinagainstthe member's own advertised client URL. When
--joinpoints to a differentstring (e.g. a Kubernetes Service URL instead of the duplicated per-Pod client
URL), the guard is bypassed and etcd accepts the unique peer URL.
Impact: after the orphan Pod was deleted, its member lingered and still
appeared healthy because probing its (duplicated) client URL routes to the
healthy original PD. In a reproduced 4-member cluster carrying one orphan,
isolating a single healthy PD left only 2/4 raft members available, so PD APIs
failed and control-plane SQL timed out — while a clean 3-member cluster
tolerated the same isolation.
What is changed and how does it work?
Add a standalone
checkAndClaimClientURLs(server/join/client_url.go), invokedfrom
PrepareJoinClusteron every startup, before the local etcd (re)startsand republishes — both a fresh join and a restart that changed its
--advertise-client-urls. It has two layers:reject if this member's advertised client URL is already used by any other
member. "Other" is decided by peer URL (unique per member), not name, so a
different member sharing our name is still checked.
transaction (
If CreateRevision==0 Then Put(owner) Else Get) atomicallyclaims the URL in a registry keyed by the (hex-encoded) client URL. Because
the transaction is serialized through raft, two concurrent joiners cannot
both take the same client URL — the first wins, later ones are rejected. The
owner record stores the member's name and peer URLs, and identity is
matched by peer URL, so even two concurrent joiners that share a name are
correctly rejected. A restart keeps the same peer URLs and re-claims its own
URL without error.
Because the check runs before the member's etcd (re)publishes its attributes, a
bad join/restart never enters the member list. The restart path is best-effort
on connectivity: if the cluster is unreachable, a member is still allowed to
start from its local data directory (it is not blocked).
Known limitation (follow-up): the claim is persistent, so if a member is removed
and a different member later reuses its client URL, the stale entry must be
cleaned up first. Cleanup on member removal is tracked as follow-up work.
Tests
TestJoinWithDuplicateClientURL— a fresh join reusing a live member's clientURL is rejected; membership is untouched.
TestRestartWithModifiedDuplicateClientURL— a restart (data dir exists) thatchanged its client URL to another member's is rejected on the data-dir path.
TestSimpleJoin— legitimate joins with unique URLs are unaffected.Check List
Tests
Code changes
/pd/{cluster_id}/member/client-urls/{hex(url)})Side effects
Related changes
Release note
Summary by CodeRabbit