mcs: use the configured lease for the service registry#11017
mcs: use the configured lease for the service registry#11017bufferflies wants to merge 2 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
📝 WalkthroughWalkthroughMCS services expose leader lease values, and registration uses the configured lease as its registry TTL when larger than the discovery default. Discovery keepalive retries are capped and deregistration retains a fixed request timeout for long TTL configurations. ChangesMCS registry lease
Estimated code review effort: 2 (Simple) | ~15 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 | ✅ 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 |
| // 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.
| 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?
[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.25% +0.02%
==========================================
Files 541 541
Lines 75965 75998 +33
==========================================
+ Hits 60187 60234 +47
+ Misses 11531 11519 -12
+ Partials 4247 4245 -2
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.
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