domain: warn when multiple TiDB instances advertise the same status endpoint#69899
domain: warn when multiple TiDB instances advertise the same status endpoint#69899OliverS929 wants to merge 8 commits into
Conversation
|
[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 |
|
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:
📝 WalkthroughWalkthroughThe change adds etcd-backed advertised status endpoint claims to server-info synchronization, including ownership, reattachment, cleanup, conflict warnings, and shutdown handling. Domain and session initialization now propagate options that disable claims for temporary domains, with unit and integration coverage. ChangesStatus endpoint claims
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TiDBDomain
participant ServerInfoSyncer
participant Etcd
TiDBDomain->>ServerInfoSyncer: initialize with claim options
ServerInfoSyncer->>Etcd: create, inspect, or reattach endpoint claim
ServerInfoSyncer->>Etcd: store server information
ServerInfoSyncer->>Etcd: remove claim during cleanup
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/domain/serverinfo/syncer_test.go (1)
820-825: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid tight wall-clock upper bounds in integration tests.
The 2–2.5 second ceilings can fail under CI scheduling pauses even when cleanup deadlines and backoff are correct. Prefer the captured deadline/backoff state plus a generous outer test timeout.
As per coding guidelines, test changes should remain minimal and deterministic.
Also applies to: 936-938
🤖 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/domain/serverinfo/syncer_test.go` around lines 820 - 825, Update the integration tests around syncer.NewSessionAndStoreServerInfo and the corresponding assertions near the other referenced case to avoid tight elapsed-time upper bounds. Assert the captured cleanup deadline/backoff state directly, and use a generous outer test timeout for runaway protection while preserving the existing error assertion and minimal deterministic coverage.Source: Coding guidelines
pkg/session/tidb_test.go (1)
88-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise and verify the complete active-claim warning contract.
The seeded key has no lease, so it does not model a live serving claimant. Attach it to a granted lease, then assert the warning includes the endpoint, local identity, and
existing-serveridentity—not only the local ID.🤖 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/session/tidb_test.go` around lines 88 - 115, Update the active-claim setup in the test around BootstrapSession to grant an etcd lease and attach the seeded claim key to that lease, modeling a live claimant. Expand the warning assertions for “advertised status endpoint already has an active claim” to verify the endpoint, the local server identity, and the existing “existing-server” identity in the warning context.
🤖 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/domain/serverinfo/syncer.go`:
- Around line 172-176: Update the host normalization logic around
netip.ParseAddr to skip unspecified IP addresses such as 0.0.0.0 and :: when
constructing claims, rather than adding them as usable endpoints. Preserve the
existing normalized hostname handling and ensure unusable status endpoints do
not participate in duplicate detection.
- Around line 591-593: Update the restart flow around waitForServerInfoRestart
so it checks the server shutdown signal before invoking Restart or registering a
new session/endpoint claim. Ensure s.Done() takes precedence when both s.Done()
and exitCh are ready, while preserving the existing exitCh handling and return
behavior.
---
Nitpick comments:
In `@pkg/domain/serverinfo/syncer_test.go`:
- Around line 820-825: Update the integration tests around
syncer.NewSessionAndStoreServerInfo and the corresponding assertions near the
other referenced case to avoid tight elapsed-time upper bounds. Assert the
captured cleanup deadline/backoff state directly, and use a generous outer test
timeout for runaway protection while preserving the existing error assertion and
minimal deterministic coverage.
In `@pkg/session/tidb_test.go`:
- Around line 88-115: Update the active-claim setup in the test around
BootstrapSession to grant an etcd lease and attach the seeded claim key to that
lease, modeling a live claimant. Expand the warning assertions for “advertised
status endpoint already has an active claim” to verify the endpoint, the local
server identity, and the existing “existing-server” identity in the warning
context.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 03a095c1-04eb-4bb6-bed9-c5e3266d5068
📒 Files selected for processing (12)
.agents/skills/tidb-test-guidelines/references/domain-case-map.md.agents/skills/tidb-test-guidelines/references/session-case-map.mdpkg/domain/domain.gopkg/domain/infosync/info.gopkg/domain/serverinfo/BUILD.bazelpkg/domain/serverinfo/info.gopkg/domain/serverinfo/syncer.gopkg/domain/serverinfo/syncer_test.gopkg/session/BUILD.bazelpkg/session/global_init.gopkg/session/tidb.gopkg/session/tidb_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69899 +/- ##
================================================
- Coverage 76.3191% 75.5334% -0.7858%
================================================
Files 2041 2087 +46
Lines 559917 583409 +23492
================================================
+ Hits 427324 440669 +13345
- Misses 131692 140572 +8880
- Partials 901 2168 +1267
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| // Note: collate.newCollationEnabled is set to 1 in init(), so if the | ||
| // new_collate=false during first bootstrap, they mismatch. | ||
| dom, err := domap.GetOrCreateWithFilter(store, systemDBFilter{}) | ||
| dom, err := domap.getTemporaryWithFilter(store, systemDBFilter{}) |
There was a problem hiding this comment.
Temporary is not a good term here
There was a problem hiding this comment.
Renamed this to describe its actual purpose: the Domain used for global-variable initialization. Updated in 5c5a1d0.
| return storeErr | ||
| } | ||
|
|
||
| func (s *Syncer) claimStatusEndpoint(ctx context.Context) statusEndpointClaimResult { |
There was a problem hiding this comment.
server info syncer already contains a lot of functionality, maybe abstract those endpoint claim related staff into a new struct, or new pkg
There was a problem hiding this comment.
Moved the endpoint claim logic into a private statusEndpointClaim struct in 5c5a1d0.
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/domain/serverinfo/status_endpoint_claim.go`:
- Around line 85-105: Update buildStatusEndpointClaim so the claim key includes
info.Keyspace as a separate path segment in addition to the encoded endpoint.
Preserve the existing endpoint value and validation behavior, and construct the
returned claim path using the keyspace segment plus the endpoint segment.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8ac3be41-dbf3-4823-944d-9fcf1072138d
📒 Files selected for processing (9)
.agents/skills/tidb-test-guidelines/references/session-case-map.mdpkg/domain/infosync/info.gopkg/domain/serverinfo/BUILD.bazelpkg/domain/serverinfo/status_endpoint_claim.gopkg/domain/serverinfo/syncer.gopkg/domain/serverinfo/syncer_test.gopkg/session/global_init.gopkg/session/tidb.gopkg/session/tidb_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- .agents/skills/tidb-test-guidelines/references/session-case-map.md
- pkg/domain/infosync/info.go
- pkg/session/tidb_test.go
- pkg/session/tidb.go
- pkg/domain/serverinfo/syncer_test.go
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 9
- Inline comments: 6
- Summary-only findings (no inline anchor): 3
Findings (highest risk first)
⚠️ [Major] (1)
- acquire() duplicates the tryCreate-then-classify block instead of extracting a helper (pkg/domain/serverinfo/status_endpoint_claim.go:130, pkg/domain/serverinfo/status_endpoint_claim.go:158)
🟡 [Minor] (7)
checkname hides that it writes/claims the status endpoint in etcd (pkg/domain/serverinfo/status_endpoint_claim.go:109, pkg/domain/serverinfo/syncer.go:144)- Endpoint-claim cleanup logging is copy-pasted across three call sites (pkg/domain/serverinfo/syncer.go:163, pkg/domain/serverinfo/syncer.go:169, pkg/domain/serverinfo/syncer.go:339)
- Tests in two packages hardcode the internal status-endpoint-claim etcd key encoding instead of reusing serverinfo helpers/constants (pkg/session/tidb_test.go:91, pkg/session/tidb_test.go:144, pkg/domain/serverinfo/syncer_test.go:171, pkg/domain/serverinfo/syncer_test.go:836, pkg/domain/serverinfo/status_endpoint_claim.go:34)
- Endpoint-claim enable parameter renamed to
enabled, diverging fromreportStatusused at every other hop (pkg/domain/serverinfo/status_endpoint_claim.go:72, pkg/domain/serverinfo/syncer.go:120) checksilently drops the claim result on context cancellation with no comment explaining why (pkg/domain/serverinfo/status_endpoint_claim.go:111)- Stale-server cleanup does not reclaim the new status endpoint claim, causing false duplicate-config warnings after ungraceful restart (pkg/domain/serverinfo/syncer.go:145, pkg/domain/serverinfo/syncer.go:300, pkg/domain/serverinfo/status_endpoint_claim.go:250)
- New exit-vs-restart precedence and fixed backoff in ServerInfoSyncLoop are undocumented (pkg/domain/serverinfo/syncer.go:368, pkg/domain/serverinfo/syncer.go:395)
ℹ️ [Info] (1)
- Info-syncer restart backoff is a fixed 1s interval with no jitter (pkg/domain/serverinfo/syncer.go:396, pkg/domain/serverinfo/syncer.go:372)
Unanchored findings
⚠️ [Major] (1)
- acquire() duplicates the tryCreate-then-classify block instead of extracting a helper
- Request: Extract a private helper, e.g.
classifyTryCreate(ctx, lease) (done bool, result statusEndpointClaimResult), that runstryCreateand applies the shared classification, returning whetheracquireshould return immediately. Call it once beforereattachand once after, instead of inlining the same 11+ line block twice.
- Request: Extract a private helper, e.g.
🟡 [Minor] (2)
- Endpoint-claim cleanup logging is copy-pasted across three call sites
- Request: Add a small helper on
SyncerorstatusEndpointClaim, e.g.logClaimOpFailure(level, msg, lease, action, err), and call it from all three sites instead of repeating the fields-build-then-log pattern.
- Request: Add a small helper on
- Tests in two packages hardcode the internal status-endpoint-claim etcd key encoding instead of reusing serverinfo helpers/constants
- Request: Export a small helper from
serverinfo(e.g. reusestatusEndpointClaimTestKey-equivalent logic as an exported test helper, or expose the key-building function) and havetidb_test.gocall it instead of re-deriving the literal; replace the raw"/tidb/server/info/"literal withserverinfo.ServerInformationPath.
- Request: Export a small helper from
| return endpoint, claimKey | ||
| } | ||
|
|
||
| func (c *statusEndpointClaim) check(ctx context.Context, lease clientv3.LeaseID) { |
There was a problem hiding this comment.
🟡 [Minor] check name hides that it writes/claims the status endpoint in etcd
Impact
A maintainer reading NewSessionAndStoreServerInfo or the sync-loop restart path only sees a call to check, described in a comment as best-effort, and can reasonably assume it is a read-only inspection.
If a future change short-circuits or skips check under that assumption (for example a fast path that thinks it is only diagnostic), the actual etcd write that claims this instance's status endpoint is silently lost, and duplicate-status-endpoint detection stops working for that instance.
Scope
pkg/domain/serverinfo/status_endpoint_claim.go:109—statusEndpointClaim.checkpkg/domain/serverinfo/syncer.go:144
Evidence
check calls acquire, which performs a clientv3.Txn(...).Then(clientv3.OpPut(...)) compare-and-swap write to actually claim the endpoint key via tryCreate/reattach, not merely read it. The call-site comment at syncer.go:144 reinforces the misleading framing ("Endpoint claim checks are best-effort"), and the failure state is named statusEndpointClaimCheckFailed even though the underlying operation writes to etcd.
Change request
Rename check to a verb that signals the mutation, e.g. claim or tryClaim, and adjust the accompanying comment and statusEndpointClaimCheckFailed state name so callers can tell at a glance that this call mutates etcd state rather than just inspecting it.
There was a problem hiding this comment.
Renamed the wrapper to make the claim attempt explicit and adjusted the call-site comment in 388e17e. I kept statusEndpointClaimCheckFailed because it describes the diagnostic result when the claim cannot be read or updated.
There was a problem hiding this comment.
✅ The rename to tryAcquireAndReport plus the reworded call-site comment now make the etcd mutation obvious at the call site, which was the crux here. Retaining statusEndpointClaimCheckFailed for the read-or-update diagnostic result is a fair distinction.
| report func(statusEndpointClaimResult) | ||
| } | ||
|
|
||
| func newStatusEndpointClaim(etcdClient *clientv3.Client, info *ServerInfo, enabled bool) *statusEndpointClaim { |
There was a problem hiding this comment.
🟡 [Minor] Endpoint-claim enable parameter renamed to enabled, diverging from reportStatus used at every other hop
Impact
The same boolean concept, whether the syncer should advertise/report its status endpoint, is called reportStatus at the call site in newSyncer and again in buildStatusEndpointClaim, but enabled in newStatusEndpointClaim.
A reader jumping from newStatusEndpointClaim(etcdCli, info, reportStatus) to its definition sees an unrelated, generic parameter name and has to re-derive that enabled means "report the configured status endpoint" rather than some standalone claim-feature toggle.
Scope
pkg/domain/serverinfo/status_endpoint_claim.go:72—newStatusEndpointClaimpkg/domain/serverinfo/syncer.go:120
Evidence
newSyncer computes reportStatus := config.GetGlobalConfig().Status.ReportStatus && !args.skipStatusEndpointClaim (syncer.go:120) and passes it into newStatusEndpointClaim(etcdCli, info, reportStatus), whose signature names the same value enabled (status_endpoint_claim.go:72), while the function it forwards to, buildStatusEndpointClaim, calls it reportStatus again (status_endpoint_claim.go:85).
Change request
Use one consistent parameter name, e.g. reportStatus, across newStatusEndpointClaim and buildStatusEndpointClaim so the concept reads the same at every call boundary.
There was a problem hiding this comment.
Aligned this path on claimEnabled in 388e17e; it represents both report-status and the explicit claim opt-out.
|
|
||
| func (c *statusEndpointClaim) check(ctx context.Context, lease clientv3.LeaseID) { | ||
| result := c.acquire(ctx, lease) | ||
| if ctx.Err() == nil && c.report != nil { |
There was a problem hiding this comment.
🟡 [Minor] check silently drops the claim result on context cancellation with no comment explaining why
Impact
When the caller's context is canceled while acquire is still running, check swallows the result, including a genuine statusEndpointClaimCheckFailed error, instead of reporting it, and there is no comment stating this is intentional.
A future reader modifying check or investigating a missing claim-conflict warning has no signal in the code that this suppression on cancellation is deliberate shutdown-quieting behavior rather than an oversight.
Scope
pkg/domain/serverinfo/status_endpoint_claim.go:111
Evidence
check guards the c.report(result) call with ctx.Err() == nil (status_endpoint_claim.go:111); the intent is only documented indirectly by the syncer_test.go case "parent cancellation produces no misleading claim warning", not in the production code itself.
Change request
Add a short comment on the ctx.Err() == nil guard in check stating that reporting is suppressed on cancellation to avoid emitting a misleading claim warning during shutdown/abort.
There was a problem hiding this comment.
Added a comment in 388e17e explaining that result reporting is suppressed after parent cancellation to avoid a misleading warning during shutdown or abort.
There was a problem hiding this comment.
✅ The guard now carries a comment noting that reporting is skipped once the context is canceled, so the shutdown-quieting behavior is visible in the production code rather than only inferable from the test name.
| return s.StoreServerInfo(ctx) | ||
|
|
||
| // Endpoint claim checks are best-effort; conflicts and check errors must not block server info registration. | ||
| s.endpointClaim.check(ctx, session.Lease()) |
There was a problem hiding this comment.
🟡 [Minor] Stale-server cleanup does not reclaim the new status endpoint claim, causing false duplicate-config warnings after ungraceful restart
Impact
After a kill-9/OOM restart, the same TiDB node comes back with a fresh syncer UUID (ddl.GetID() = uuid.New()), so its endpoint-claim check sees the dead predecessor's claim (whose lease survives up to SessionTTL = 90s) as a different-ID owner and reports statusEndpointClaimConflict.
This logs "advertised status endpoint already has an active claim" on a node that is actually the sole owner, undermining the feature's stated purpose of detecting duplicate advertise-address/status-port settings, and the legitimate node never holds the advisory claim during that window.
Scope
pkg/domain/serverinfo/syncer.go:145—NewSessionAndStoreServerInfopkg/domain/serverinfo/syncer.go:300—cleanupStaleServerAndOwnerInfopkg/domain/serverinfo/status_endpoint_claim.go:250—acquire
Evidence
cleanupStaleServerAndOwnerInfo runs right before s.endpointClaim.check(...) and deletes the stale server-info key and DDL owner key for a same-IP+Port predecessor, but not the stale serverStatusAddressPath claim keyed by the same IP+StatusPort. The claim state machine in acquire/tryCreate treats the leftover observed.id != localID as statusEndpointClaimConflict, which reportResult logs as a warning.
Change request
In cleanupStaleServerAndOwnerInfo, also remove the stale status endpoint claim for each matched dead predecessor (reconstruct its claim key from si.IP+si.StatusPort, or delete by owner ID), so a legitimate crash-restarted node does not emit a spurious duplicate-endpoint conflict and can immediately reclaim the endpoint.
There was a problem hiding this comment.
The claim intentionally stays attached to the server-info lease and can remain for up to SessionTTL after an ungraceful exit; the replacement still registers and serves SQL. The legacy stale-server cleanup only has decoded server info and does not retain the old etcd KV lease, so deleting by endpoint or owner ID alone could remove a newer claim. Safe eager cleanup would require additional identity-and-lease fencing and would change the accepted TTL behavior, so I would keep the current cleanup in this PR.
There was a problem hiding this comment.
✅ Makes sense — the stale-cleanup path only carries decoded server info, not the predecessor's etcd lease, so deleting a claim by endpoint or owner ID could clobber a newer owner's claim. Letting it lapse with the lease inside SessionTTL is the safer default; the replacement still registers and serves SQL during that window. Eager fenced cleanup is reasonable as a separate follow-up.
| case <-ticker.C: | ||
| s.reporter.ReportMinStartTS(store, s.session) | ||
| case <-s.Done(): | ||
| if serverInfoSyncLoopExitRequested(exitCh) { |
There was a problem hiding this comment.
🟡 [Minor] New exit-vs-restart precedence and fixed backoff in ServerInfoSyncLoop are undocumented
Impact
ServerInfoSyncLoop now calls serverInfoSyncLoopExitRequested right after <-s.Done() fires, even though the same select already has a case <-exitCh: return, and waits a fixed 1s via waitForServerInfoRestart after a failed restart before retrying. Neither the explicit re-check nor the fixed wait is explained, so a future edit to this select (reordering cases, dropping the check) could reintroduce a restart-during-shutdown race with nothing warning against it.
Scope
pkg/domain/serverinfo/syncer.go:368—ServerInfoSyncLooppkg/domain/serverinfo/syncer.go:395—waitForServerInfoRestart
Evidence
serverInfoSyncLoopExitRequested exists because Go's select picks pseudo-randomly among ready cases, so <-s.Done() firing does not guarantee <-exitCh would have been chosen even if both channels are ready at the same time; that reasoning is not written near the check at syncer.go:368 or near the 1s wait in waitForServerInfoRestart at syncer.go:395.
Change request
Add a brief comment at the serverInfoSyncLoopExitRequested check explaining the select-ordering race it guards against, and a short comment on waitForServerInfoRestart stating the fixed wait exists to avoid a tight restart loop on persistent failures while staying responsive to exitCh.
There was a problem hiding this comment.
Clarified in 388e17e why exitCh is rechecked after s.Done() and renamed the wait helper to describe the exit-interruptible restart backoff.
There was a problem hiding this comment.
✅ Both spots are now documented — the recheck after s.Done() explains the select race where a ready exitCh may not win, and waitForServerInfoRestartRetry was renamed with a doc comment describing the exit-interruptible backoff. Resolving.
| } | ||
|
|
||
| func waitForServerInfoRestart(exitCh <-chan struct{}) bool { | ||
| timer := time.NewTimer(time.Second) |
There was a problem hiding this comment.
ℹ️ [Info] Info-syncer restart backoff is a fixed 1s interval with no jitter
When many TiDB nodes lose their server-info etcd session at the same moment (etcd leader change or a brief partition), every node re-enters this restart loop and retries on the same fixed 1s cadence.
Without jitter the retries stay phase-aligned across nodes, producing synchronized session-grant bursts against etcd during the exact window it is recovering. Blast radius is low (about one attempt per node per second, and NewSession's own internal retries partially desynchronize it), so this is an observation rather than a defect.
There was a problem hiding this comment.
The fixed one-second wait keeps the outer restart loop to at most one new Restart attempt per second after a failure. NewSession also performs its own retries, which provides some natural desynchronization. This delay was added only to prevent a tight loop, not to define a complete distributed backoff policy, so I would keep jitter out of this PR.
There was a problem hiding this comment.
✅ Agreed this is just a tight-loop guard, not a full distributed backoff policy — one Restart attempt per second, with NewSession's own retries adding some natural desync. Given the low blast radius, leaving jitter out of this PR is fine.
|
/retest |
What problem does this PR solve?
Issue Number: close #69689
Problem Summary:
TiDB publishes each instance's advertised status endpoint in server info, but it does not report when two different TiDB instances publish the same endpoint. This makes duplicate
advertise-addressandstatus-portconfiguration difficult to diagnose.What changed and how does it work?
TiDB now detects when different TiDB instances advertise the same status endpoint. The conflict produces a structured warning containing both TiDB identities and the duplicated endpoint, without blocking server-info registration or startup.
To make this detection reliable during concurrent startup, each serving primary TiDB tries to create an etcd claim for its normalized advertised endpoint. The claim records the TiDB identity and uses the same lease as its server-info record.
In keyspace-enabled deployments, endpoint claims use the same logical-keyspace namespace as server info, so duplicate detection is scoped to one keyspace. CrossKS virtual registrations, the non-serving Domain created during global-variable initialization, and registrations without an enabled, usable status endpoint skip this check. The check is warning-only and does not continuously reconcile claims after the current holder exits.
Check List
Tests
Manual test:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit