mcs: reject registration reusing another instance's address - #11043
mcs: reject registration reusing another instance's address#11043bufferflies wants to merge 2 commits into
Conversation
Claim the service registry key with an atomic create-if-absent transaction so that an instance advertising a duplicate address cannot overwrite the registry entry of another live instance and further join the primary election with the same identity. A stale entry left by a crashed instance expires with its lease, so the registration retries within the lease TTL before giving up. Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
dco-signoff: yes
📝 WalkthroughWalkthroughService registration now uses lease-backed conditional etcd transactions, rejects live duplicate addresses, retries occupied registrations within a TTL-based deadline, and revokes failed leases. Tests cover conflict rejection, lease expiry, and same-value lease ownership. ChangesService registration conflict handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceRegister
participant Etcd
participant ServiceContext
ServiceRegister->>Etcd: Grant lease and conditionally claim registry key
Etcd-->>ServiceRegister: Return success or errServiceAddrOccupied
ServiceRegister->>ServiceContext: Wait for retry interval or cancellation
ServiceRegister->>Etcd: Retry claim before the TTL-based deadline
Etcd-->>ServiceRegister: Register after the previous lease expires
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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)
pkg/mcs/discovery/register_test.go (1)
109-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
errors.Isover substring matching for the sentinel error check.Since this is an internal test (same package), asserting
errors.Is(err, errServiceAddrOccupied)directly ties the test to the actual sentinel error instead of its message wording, which is more robust against future message-copy changes.♻️ Proposed refactor
err := sr2.Register() re.Error(err) - re.Contains(err.Error(), "occupied") + re.ErrorIs(err, errServiceAddrOccupied)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/mcs/discovery/register_test.go` around lines 109 - 111, Update the Register test’s error assertion after sr2.Register() to use errors.Is with the errServiceAddrOccupied sentinel instead of checking for the "occupied" message substring, while retaining the existing error assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/mcs/discovery/register_test.go`:
- Around line 109-111: Update the Register test’s error assertion after
sr2.Register() to use errors.Is with the errServiceAddrOccupied sentinel instead
of checking for the "occupied" message substring, while retaining the existing
error assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b278b836-d3c2-4210-9736-a876500f995b
📒 Files selected for processing (2)
pkg/mcs/discovery/register.gopkg/mcs/discovery/register_test.go
| // A stale registry entry left by a crashed instance with the same advertised | ||
| // address will be removed automatically once its lease expires, so retry | ||
| // within the lease TTL before giving up. | ||
| deadline := time.Now().Add(time.Duration(sr.ttl+1) * time.Second) |
There was a problem hiding this comment.
6s might not be enough: across an etcd leader change, Promote(ElectionTimeout()) and Lease.refresh resets expiry to now + RemainingTTL + extend.
| // a keepalive failure while the previous lease has not expired yet), take | ||
| // it over with the new lease. | ||
| takeoverResp, err := sr.cli.Txn(ctx). | ||
| If(clientv3.Compare(clientv3.Value(sr.key), "=", sr.value)). |
There was a problem hiding this comment.
[Blocker] Only comparing values doesn't tell two processes apart: the only per-process field is StartTimestamp, in whole seconds (pkg/mcs/server/server.go:58). Two instances on the same host with the same advertise addr, started in the same second, write the same value — so the second one just takes the key.
I hit this on a real 3-node PD: two scheduling instances, same advertise addr, started ~3ms apart. Both registered, neither saw occupied, both campaigned, and the loser deleted the other's leader key via participant.go:238. That's #10998 — the election ID is still just a hash of the address.
Fix: put a nonce in ServiceRegistryEntry, or compare kvs[0].Lease instead of the value.
- Compare the registry key's lease ID instead of its serialized value to decide whether an existing entry belongs to this instance. Two distinct instances can serialize an identical ServiceRegistryEntry (StartTimestamp has only second precision), so a value match does not prove ownership and could let one live instance take over another's registration. - Extend the registration retry deadline past the raw lease TTL to account for etcd extending a lease's expiry on leader change (Lessor.Promote / Lease.refresh), so a legitimate retry does not give up before a stale entry's lease has actually expired.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/mcs/discovery/register.go`:
- Around line 59-63: Update the ownership check in the registration flow to
require sr.leaseID to be nonzero before comparing it with an existing registry
key’s lease ID. Reject unleased existing keys instead of allowing a freshly
started process to overwrite them, while preserving the current behavior for
matching nonzero leases.
🪄 Autofix
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 Plus
Run ID: e0d08bb6-7b14-42ec-a39a-b224a604a8ae
📒 Files selected for processing (2)
pkg/mcs/discovery/register.gopkg/mcs/discovery/register_test.go
| // leaseID is the lease this instance most recently registered the key | ||
| // with, used to prove ownership of an existing entry on re-registration. | ||
| // Zero (clientv3.NoLease) until the first successful put, so a freshly | ||
| // started process can never match an existing key's lease by accident. | ||
| leaseID clientv3.LeaseID |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate register.go and etcd deps =="
git ls-files | rg '(^|/)register\.go$|go\.mod$|go\.sum$'
echo
echo "== relevant snippet =="
if [ -f pkg/mcs/discovery/register.go ]; then
nl -ba pkg/mcs/discovery/register.go | sed -n '1,240p'
fi
echo
echo "== etcd/client deps =="
if [ -f go.mod ]; then
rg 'go.etcd.io/etcd|clientv3|leases|registry' go.mod go.sum | head -n 120
fiRepository: tikv/pd
Length of output: 398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the repository has any local etcd client source that can be inspected for LeaseValue semantics.
for d in module go mod cache; do
if [ -d "${d}" ]; then
echo "directory ${d} exists"
find "${d}" -path '*etcd*client*' -type f | head -n 80 || true
fi
done
echo
echo "== search LeaseValue definitions/usages in repo =="
rg -n "func LeaseValue|LeaseValue\(|NoLease|LeaseID\\(" -S . | head -n 200Repository: tikv/pd
Length of output: 1001
🌐 Web query:
go etcd clientv3 LeaseValue response Lease zero NoLease semantics
💡 Result:
In the etcd clientv3 library and etcd API, a lease ID of 0 is a special value defined as NoLease [1][2]. This value represents the absence of a lease [1][3]. Key semantic details include: 1. NoLease Semantics: The constant NoLease (defined as 0) is used to explicitly indicate that no lease is attached to a specific key-value pair [4][5][6]. In the etcd key-value store, each key may be attached to at most one lease; if the lease ID field for a key is 0, the key is effectively unassociated with any lease and will not be automatically deleted by lease expiration or revocation [4][5][6]. 2. LeaseValue Helper: In the clientv3 package, the LeaseValue function allows developers to compare a key's current lease ID against a specific value within a transaction (Txn) [7][8]. When using this helper, comparing against 0 (NoLease) is the standard way to verify that a key does not currently have a lease attached [7][9]. 3. LeaseGrant API: While a client can request a specific lease ID, setting the ID to 0 in a LeaseGrantRequest instructs the etcd cluster to automatically generate and assign a unique lease ID [4][5]. 4. Internal Consistency: Both the client-side (clientv3) and the server-side (lessor) packages use NoLease = 0 to maintain consistent semantics across the system [1][3][2]. Attempting to perform lease-specific operations using the NoLease ID will typically result in an error, such as ErrLeaseNotFound [3][10].
Citations:
- 1: https://github.com/etcd-io/etcd/blob/main/client/v3/lease.go
- 2: https://github.com/etcd-io/etcd/blob/v3.3.27/clientv3/lease.go
- 3: https://github.com/etcd-io/etcd/blob/master/server/lease/lessor.go
- 4: https://etcd.io/docs/v3.7/learning/api/
- 5: https://etcd.io/docs/v3.5/learning/api/
- 6: https://etcd.io/docs/v3.4/learning/api/
- 7: https://github.com/etcd-io/etcd/blob/8ff746c2/client/v3/compare.go
- 8: https://godocs.io/go.etcd.io/etcd/client/v3
- 9: https://github.com/etcd-io/etcd/blob/8ff746c2/client/v3/leasing/kv.go
- 10: https://github.com/etcd-io/etcd/blob/main/server/etcdserver/api/v3rpc/lease.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
export LANG=C
echo "== register.go relevant section =="
awk 'NR>=40 && NR<=70 {printf "%6d\t%s\n", NR, $0} NR>=180 && NR<=215 {printf "%6d\t%s\n", NR, $0}' pkg/mcs/discovery/register.go
echo
echo "== dependencies =="
awk '/go\.etcd\.io\/etcd|client\.v3|etcd (client)|registry/ {printf "%s\n", $0}' go.mod go.sum | sed -n '1,160p'
echo
echo "== deterministic semantic probe using repository comment/condition context =="
python3 - <<'PY'
from pathlib import Path
p=Path('pkg/mcs/discovery/register.go').read_text()
checks=[
("fresh leaseID zero", "sr.leaseID = clientv3.NoLease" in p or "leaseID clientv3.LeaseID" in p),
("current ownership condition compares lease", "clientv3.LeaseID(kvs[0].Lease) != sr.leaseID" in p),
("current transaction compares LeaseValue to leaseID", "clientv3.LeaseValue(sr.key)" in p and "sr.leaseID" in p),
("NoLease constant likely zero per source comment", "clientv3.NoLease" in p),
]
for name, ok in checks:
print(f"{name}: {ok}")
PYRepository: tikv/pd
Length of output: 5423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
export LANG=C
echo "== register.go relevant section =="
awk 'NR>=1 && NR<=240 {printf "%6d\t%s\n", NR, $0}' pkg/mcs/discovery/register.go
echo
echo "== dependency lines =="
grep -E 'go\.etcd\.io/etcd|client\.v3|etcd \(client\)|registry|lease' go.mod go.sum | sed -n '1,180p' || true
echo
echo "== exact semantic probe from source =="
python3 - <<'PY'
from pathlib import Path
p=Path('pkg/mcs/discovery/register.go').read_text()
print("fresh leaseID field:", "leaseID clientv3.LeaseID" in p)
print("init zero leaseID:", "sr.leaseID = clientv3.NoLease" in p)
print("ownership compares existing lease to register leaseID:", "clientv3.LeaseID(kvs[0].Lease) != sr.leaseID" in p)
print("transaction compares current LeaseValue to register leaseID:", "clientv3.LeaseValue(sr.key)" in p)
print("comment explains NoLease/zero:", "Zero (clientv3.NoLease)" in p)
PYRepository: tikv/pd
Length of output: 11681
🏁 Script executed:
#!/bin/bash
set -euo pipefail
export LANG=C
echo "== register.go relevant section =="
awk 'NR>=1 && NR<=240 {printf "%6d\t%s\n", NR, $0}' pkg/mcs/discovery/register.go
echo
echo "== dependency lines =="
grep -E 'go\.etcd\.io/etcd|client\.v3|etcd \(client\)|registry|lease' go.mod go.sum | sed - n '1,180p' || true
echo
echo "== exact semantic probe from source =="
python3 - <<'PY'
from pathlib import Path
p=Path('pkg/mcs/discovery/register.go').read_text()
print("fresh leaseID field:", "leaseID clientv3.LeaseID" in p)
print("init zero leaseID:", "sr.leaseID = clientv3.NoLease" in p)
print("ownership compares existing lease to register leaseID:", "clientv3.LeaseID(kvs[0].Lease) != sr.leaseID" in p)
print("transaction compares current LeaseValue to register leaseID:", "clientv3.LeaseValue(sr.key)" in p)
print("comment explains NoLease/zero:", "Zero (clientv3.NoLease)" in p)
PYRepository: tikv/pd
Length of output: 10114
Reject unleased existing registry keys.
sr.leaseID starts as clientv3.NoLease, and an existing etcd key without a lease also has lease ID zero. The ownership check can then treat that key as owned by a freshly started process and allow re-registration to overwrite it. Require sr.leaseID to be nonzero before accepting lease ownership.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/mcs/discovery/register.go` around lines 59 - 63, Update the ownership
check in the registration flow to require sr.leaseID to be nonzero before
comparing it with an existing registry key’s lease ID. Reject unleased existing
keys instead of allowing a freshly started process to overwrite them, while
preserving the current behavior for matching nonzero leases.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11043 +/- ##
==========================================
+ Coverage 79.25% 79.34% +0.09%
==========================================
Files 541 542 +1
Lines 76037 77058 +1021
==========================================
+ Hits 60262 61145 +883
- Misses 11534 11602 +68
- Partials 4241 4311 +70
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: YuhaoZhang00 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 |
2 participants
What problem does this PR solve?
Issue Number: Close #11001, Close #10998
A microservice instance (TSO / Scheduling) started with an
--advertise-listen-addralready used by another live instance could blindly overwrite that instance's etcd registry entry and then join the primary election with the same participant identity.What is changed and how does it work?
Validate the registry key at registration time in
pkg/mcs/discovery:putWithTTLnow claims the registry key with an atomic create-if-absent etcd transaction instead of an unconditionalPut, so a duplicate advertised address can no longer overwrite another live instance's entry.Registerretries the claim within the lease TTL window, so a stale entry left by a crashed instance with the same address expires and the restart succeeds without manual intervention, while a genuine duplicate live instance keeps its lease refreshed and the orphan fails startup before ever entering the primary election (all MCS servers callRegisterbeforestartServer).Check List
Tests
TestRegisterConflictverifies a duplicate live registration is rejected without overwriting the existing entry, and that a new instance can register after the stale lease expires.tests/integrations/mcs/discoverypasses.Release note
🤖 Generated with Claude Code
Summary by CodeRabbit