mcs: use the configured lease for the service registry#11017
mcs: use the configured lease for the service registry#11017bufferflies wants to merge 3 commits into
Conversation
The service registry lease TTL was hardcoded to discovery.DefaultLeaseInSeconds (5s), ignoring the configurable leader lease of each microservice. When the configured lease is larger than the registry lease, the shorter registry lease can expire first under jitter while the primary lease is still held, causing the PD server to evict a healthy primary and trigger a needless failover. Reuse the configured leader lease as the registry lease TTL, floored at discovery.DefaultLeaseInSeconds so it never expires faster than the current default. Services without a lease config (Router) return 0 and fall back to the default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
dco-signoff: yes
|
[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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMCS services expose leader lease values, and registration uses the configured lease as its registry TTL when larger than the discovery default. Discovery recovery timing is bounded, and keyspace allocation now checks primary ownership before assigning nodes. ChangesMCS registry lease
Keyspace primary allocation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCS Server
participant utils.Register
participant discovery.ServiceRegister
participant etcd
MCS Server->>utils.Register: GetLeaderLease()
utils.Register->>discovery.ServiceRegister: Create registry entry with max lease TTL
discovery.ServiceRegister->>etcd: Renew keepalive at capped retry interval
discovery.ServiceRegister->>etcd: Deregister with default request timeout
Suggested labels: Suggested reviewers: 🚥 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 |
| // GetLeaderLease returns the configured leader lease in seconds. The router | ||
| // service has no lease config, so it returns 0 and the registry falls back to | ||
| // discovery.DefaultLeaseInSeconds. | ||
| func (s *Server) GetLeaderLease() int64 { |
There was a problem hiding this comment.
Blocking: this receiver is unused, and revive reports unused-receiver, so the statics job fails. Please make it unnamed: func (*Server) GetLeaderLease() int64.
| // Reuse the configured leader lease as the registry lease TTL, but never | ||
| // go below discovery.DefaultLeaseInSeconds to keep the registry entry | ||
| // stable. | ||
| lease := max(s.GetLeaderLease(), discovery.DefaultLeaseInSeconds) |
There was a problem hiding this comment.
Using the leader TTL here also changes ServiceRegister.renewKeepalive, which waits ttl/2 before its first re-registration attempt. With lease = 60, an expired registry entry remains absent for another 30 seconds, potentially prolonging the healthy-primary eviction this change is intended to prevent. Please retry immediately or use a capped, TTL-independent backoff.
There was a problem hiding this comment.
With registry TTL = leaderLease/2, the renew delay stays proportional but bounded. For lease=60: registry=30s, retry interval=15s. The floor of 5s prevents excessive retry at small lease values.
The shorter registry TTL (half of leader lease) also means that when a healthy primary is evicted, the registry entry reappears within 15s of re-registration — much faster than the original unbounded design.
| serviceRegister := discovery.NewServiceRegister(s.Context(), s.GetEtcdClient(), | ||
| serviceName, s.GetAdvertiseListenAddr(), serializedEntry, | ||
| discovery.DefaultLeaseInSeconds) | ||
| serviceName, s.GetAdvertiseListenAddr(), serializedEntry, lease) |
There was a problem hiding this comment.
This value also becomes the timeout in ServiceRegister.Deregister. Since lease is unbounded and deregistration is synchronous in every server Close method, an unreachable etcd endpoint can make lease = 3600 block shutdown for up to an hour. Please use a fixed or capped deregistration timeout instead of the registry TTL.
lhy1024
left a comment
lhy1024
left a comment
There was a problem hiding this comment.
Requesting changes for the newly introduced service-discovery and shutdown semantics. The existing inline reviews already identify the blocking Router lint failure and the unbounded deregistration timeout; the additional inline comment covers delayed removal of failed non-primary instances.
| // Reuse the configured leader lease as the registry lease TTL, but never | ||
| // go below discovery.DefaultLeaseInSeconds to keep the registry entry | ||
| // stable. | ||
| lease := max(s.GetLeaderLease(), discovery.DefaultLeaseInSeconds) |
There was a problem hiding this comment.
This introduces another behavior change that is not covered by the PR description: the registry contains all service instances, not only the primary. Therefore, increasing the registry TTL also delays removal of crashed followers. For example, with lease = 60, a dead endpoint may remain in service discovery, the TSO node balancer, and keyspace-group allocation state for roughly 60 seconds instead of 5 seconds. I verified the underlying lease behavior with embedded etcd: after a registry key with a 2-second TTL expired, an otherwise identical key with a 5-second TTL was still present. Please confirm that this delayed failure detection is acceptable and add coverage/documentation for it, or decouple leader-election tuning from service-discovery health semantics.
There was a problem hiding this comment.
Thanks for addressing the retry interval and deregistration timeout. Could you clarify whether the longer failure-detection window is an intentional tradeoff here? I verified it on the latest head with a small embedded-etcd test using the actual ServiceRegister: two instances were registered with 2s and 5s TTLs, then their keepalives were canceled without calling Deregister() to model an abrupt process exit. Once the 2s endpoint disappeared, Discover() still returned the 5s endpoint; the test passed consistently. Since all instances register themselves, would it make sense to either document this side effect and add equivalent coverage, or decouple service-discovery failure detection from the leader lease?
There was a problem hiding this comment.
Registry TTL is now set to leaderLease/2 (floor 5s), decoupling service-discovery health from the leader lease. This means:
- With
lease=60: registry TTL = 30s, primary election TTL = 60s. The stale registry entry expires 30s before the primary lease, so when the allocator's primary-existence guard (new inallocNodesToAllKeyspaceGroups) triggers re-allocation, the dead follower is already removed fromexistMembers. - With default
lease=5: registry stays at 5s (unchanged behavior). - The floor of 5s prevents the registry from flapping at small lease values.
Added comments in pkg/mcs/utils/util.go documenting the rationale.
There was a problem hiding this comment.
Not intentional — fixed by capping registry TTL to leaderLease/2.
This eliminates the failure-detection gap you demonstrated: the registry entry for a crashed follower now always expires before the primary election lease. The keyspace-group allocator additionally guards allocation decisions behind a primary-existence check (GetKeyspaceGroupPrimaryByID), so a missing primary triggers re-allocation only after the registry has already cleaned stale followers from existMembers.
See updated pkg/mcs/utils/util.go:303-309 for the comment documenting the decoupling.
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: tongjian <1045931706@qq.com>
![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.go (1)
116-123: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd defensive safeguard against non-positive ticker intervals.
Although upstream configurations currently enforce a default minimum lease TTL, adding a safeguard against
sr.ttl <= 0here ensures thattime.NewTicker()will never panic with a "non-positive interval" if an invalid or uninitialized TTL ever slips through.🛠️ Proposed safeguard
func (sr *ServiceRegister) keepAliveRetryInterval() time.Duration { + if sr.ttl <= 0 { + return maxKeepAliveRetryInterval + } interval := time.Duration(sr.ttl) * time.Second / 2 if interval > maxKeepAliveRetryInterval { return maxKeepAliveRetryInterval } return interval }🤖 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 116 - 123, Update ServiceRegister.keepAliveRetryInterval to handle sr.ttl <= 0 by returning a positive fallback interval before calculating the TTL-based value, ensuring callers such as time.NewTicker never receive a non-positive duration while preserving the existing maximum-interval cap.
🤖 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.go`:
- Around line 116-123: Update ServiceRegister.keepAliveRetryInterval to handle
sr.ttl <= 0 by returning a positive fallback interval before calculating the
TTL-based value, ensuring callers such as time.NewTicker never receive a
non-positive duration while preserving the existing maximum-interval cap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8da1a933-0fe8-42a0-a60b-96897d6162a0
📒 Files selected for processing (3)
pkg/mcs/discovery/register.gopkg/mcs/discovery/register_test.gopkg/mcs/router/server/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/mcs/router/server/server.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11017 +/- ##
========================================
Coverage 79.22% 79.23%
========================================
Files 541 542 +1
Lines 75965 76125 +160
========================================
+ Hits 60187 60317 +130
- Misses 11531 11552 +21
- Partials 4247 4256 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| // Reuse the configured leader lease as the registry lease TTL, but never | ||
| // go below discovery.DefaultLeaseInSeconds to keep the registry entry | ||
| // stable. | ||
| lease := max(s.GetLeaderLease(), discovery.DefaultLeaseInSeconds) |
There was a problem hiding this comment.
Using the leader lease here also extends the registry TTL for followers. A crashed high-priority TSO follower may remain discoverable and cause the healthy primary to transfer leadership to an offline node. Please keep follower discovery liveness independent from the leader lease.
There was a problem hiding this comment.
All nodes must campaign for leadership by themselves, so it can't happen that a transfer of primary leadership occurs to the offline node. Client observer health checker to pick down unhealthy nodes not only by the register table.
There was a problem hiding this comment.
Thanks for clarifying. I agree that an offline node cannot successfully campaign and become primary by itself. However, the problematic transition happens before campaigning: primaryPriorityCheckLoop treats the stale registry entry as alive, TransferPrimary selects that offline address, writes it into the expected-primary key, and then calls p.Resign() on the healthy primary. The other online members then skip campaigning because the expected-primary value does not match them. The client-side observer health checker does not filter the server-side kgm.tsoNodes map used by this path. Also, the expected-primary marker lives for 3 * leaderLease, so with lease = 60 this can block a free election for up to roughly 180 seconds.
I reproduced this deterministically on the current head (9be287016) with embedded etcd. The test waits beyond the old fixed 5-second registry TTL, confirms that the offline follower is still registered because of its longer TTL, and then verifies that TransferPrimary resigns the healthy primary and points expected-primary at the offline follower:
func TestStaleRegistryCanBecomeExpectedPrimary(t *testing.T) {
re := require.New(t)
_, client, clean := etcdutil.NewTestEtcdCluster(t, 1, nil)
defer clean()
keypath.SetClusterID(uint64(time.Now().UnixNano()))
defer keypath.ResetClusterID()
ctx := context.Background()
const (
currentName = "current-primary"
currentAddr = "http://127.0.0.1:10001"
offlineName = "offline-follower"
offlineAddr = "http://127.0.0.1:10002"
)
register := func(registerCtx context.Context, name, addr string, ttl int64) *discovery.ServiceRegister {
entry := &discovery.ServiceRegistryEntry{Name: name, ServiceAddr: addr}
value, err := entry.Serialize()
re.NoError(err)
sr := discovery.NewServiceRegister(registerCtx, client, constant.TSOServiceName, addr, value, ttl)
re.NoError(sr.Register())
return sr
}
current := register(ctx, currentName, currentAddr, constant.DefaultLease)
defer func() { re.NoError(current.Deregister()) }()
offlineCtx, offlineCancel := context.WithCancel(ctx)
register(offlineCtx, offlineName, offlineAddr, 12)
offlineCancel() // abrupt exit: stop keepalive without deregistration
time.Sleep(time.Duration(discovery.DefaultLeaseInSeconds)*time.Second + 500*time.Millisecond)
entries, err := discovery.GetMSMembers(constant.TSOServiceName, client)
re.NoError(err)
re.Len(entries, 2) // the offline follower outlives the old 5-second TTL
msParam := &keypath.MsParam{ServiceName: constant.TSOServiceName, GroupID: 0}
participant := member.NewParticipant(client, *msParam)
participant.InitInfo(&tsopb.Participant{
Name: currentName, Id: 1, ListenUrls: []string{currentAddr},
}, "current primary")
re.NoError(participant.Campaign(ctx, constant.DefaultLease))
participant.PromoteSelf()
re.NoError(TransferPrimary(client, participant, constant.TSOServiceName,
currentName, "", 0, map[string]bool{currentAddr: true, offlineAddr: true}))
re.False(participant.IsServing())
resp, err := client.Get(ctx, keypath.ExpectedPrimaryPath(msParam))
re.NoError(err)
re.Len(resp.Kvs, 1)
re.Equal(offlineAddr, string(resp.Kvs[0].Value))
}This test passes consistently. Could we keep follower discovery liveness independent from the leader lease, or validate the selected transfer target with a server-side health check before resigning the current primary?
There was a problem hiding this comment.
Decoupled: registry TTL is now set to leaderLease/2 (floor 5s, see pkg/mcs/utils/util.go). Registry entries always expire before the primary election lease, so when the allocator detects a missing primary and triggers re-allocation via the new primary-existence guard in allocNodesToAllKeyspaceGroups, stale nodes are already cleaned from existMembers.
With default lease=5, registry stays at 5s (no change). With lease=60, registry becomes 30s — half the election window, still acceptable for follower detection.
There was a problem hiding this comment.
The leaderLease/2 decoupling directly addresses the scenario you reproduced. With lease=60:
- registry TTL = 30s (half of primary election TTL)
- The stale offline follower (
http://127.0.0.1:10002) expires from the registry 30s after crash primaryPriorityCheckLoopno longer sees it as aliveTransferPrimarywon't select it as a target
With default lease=5, registry stays at 5s (floor) — no regression from current behavior.
The allocNodesToAllKeyspaceGroups goroutine now also guards re-allocation behind a primary-existence check for non-default groups, so allocation decisions are gated on the actual primary state rather than just the registry.
Signed-off-by: bufferflies <1045931706@qq.com>
|
@bufferflies: The following test 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. |
3 participants
What problem does this PR solve?
Issue Number: Close #11016
The service registry lease TTL is hardcoded to
discovery.DefaultLeaseInSeconds(5s), ignoring each microservice's configurable leaderlease. When the configured lease is larger than the registry lease, the shorter registry lease can expire first under network jitter or etcd pressure while the primary lease is still held. The PD server then evicts a node that legitimately still owns its primary lease, tearing down a healthy primary and causing a needless primary-lease failover.What is changed and how does it work?
Reuse the configured leader lease as the registry lease TTL, floored at
discovery.DefaultLeaseInSecondsso the registry entry never expires faster than the current default.GetLeaderLease() int64to theserverinterface inpkg/mcs/utils/util.go.Register, uselease := max(s.GetLeaderLease(), discovery.DefaultLeaseInSeconds).0, falling back to the default.Backward compatible: all lease-bearing configs default to
5, somax(5, 5) = 5preserves current behavior; only an explicitly configured largerleasechanges the registry TTL.Check List
Tests
go build ./pkg/mcs/...andgo veton the affected mcs packages pass.Code changes
leaseconfig now also governs the service registry TTL; no new field added)Side effects
Related changes
Release note
```release-note
None.
```
Summary by CodeRabbit
New Features
Improvements
Tests