Skip to content

mcs: use the configured lease for the service registry #11017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bufferflies wants to merge 2 commits into
base: master
Choose a base branch
from
Open

mcs: use the configured lease for the service registry #11017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da75f8d
mcs: use configured lease for the service registry
bufferflies Jul 17, 2026
9be2870
mcs: cap service registry recovery time
bufferflies Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions pkg/mcs/discovery/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import (
// DefaultLeaseInSeconds is the default lease time in seconds.
const DefaultLeaseInSeconds = 5

const maxKeepAliveRetryInterval = time.Duration(DefaultLeaseInSeconds) * time.Second

// ServiceRegister is used to register the service to etcd.
type ServiceRegister struct {
ctx context.Context
Expand Down Expand Up @@ -88,7 +90,7 @@ func (sr *ServiceRegister) Register() error {
}

func (sr *ServiceRegister) renewKeepalive() <-chan *clientv3.LeaseKeepAliveResponse {
t := time.NewTicker(time.Duration(sr.ttl) * time.Second / 2)
t := time.NewTicker(sr.keepAliveRetryInterval())
defer t.Stop()
for {
select {
Expand All @@ -111,6 +113,14 @@ func (sr *ServiceRegister) renewKeepalive() <-chan *clientv3.LeaseKeepAliveRespo
}
}

func (sr *ServiceRegister) keepAliveRetryInterval() time.Duration {
interval := time.Duration(sr.ttl) * time.Second / 2
if interval > maxKeepAliveRetryInterval {
return maxKeepAliveRetryInterval
}
return interval
}

func (sr *ServiceRegister) putWithTTL() (clientv3.LeaseID, error) {
ctx, cancel := context.WithTimeout(sr.ctx, etcdutil.DefaultRequestTimeout)
defer cancel()
Expand All @@ -120,8 +130,12 @@ func (sr *ServiceRegister) putWithTTL() (clientv3.LeaseID, error) {
// Deregister deregisters the service from etcd.
func (sr *ServiceRegister) Deregister() error {
sr.cancel()
ctx, cancel := context.WithTimeout(context.Background(), time.Duration(sr.ttl)*time.Second)
ctx, cancel := context.WithTimeout(context.Background(), deregisterTimeout())
defer cancel()
_, err := sr.cli.Delete(ctx, sr.key)
return err
}

func deregisterTimeout() time.Duration {
return etcdutil.DefaultRequestTimeout
}
8 changes: 8 additions & 0 deletions pkg/mcs/discovery/register_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@ func TestRegister(t *testing.T) {
etcd.Close()
}

func TestServiceRegisterLongTTLDoesNotDelayRecoveryOperations(t *testing.T) {
re := require.New(t)
sr := NewServiceRegister(context.Background(), nil, "test_service", "127.0.0.1:1", "127.0.0.1:1", 3600)

re.Equal(time.Duration(DefaultLeaseInSeconds)*time.Second, sr.keepAliveRetryInterval())
re.Equal(etcdutil.DefaultRequestTimeout, deregisterTimeout())
}

func getKeyAfterLeaseExpired(ctx context.Context, re *require.Assertions, client *clientv3.Client, key string) string {
time.Sleep(DefaultLeaseInSeconds * time.Second) // ensure that the lease is expired
time.Sleep(500 * time.Millisecond) // wait for the etcd to clean up the expired keys
Expand Down
5 changes: 5 additions & 0 deletions pkg/mcs/resourcemanager/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,11 @@ func (s *Server) GetConfig() *Config {
return s.cfg
}

// GetLeaderLease returns the configured leader lease in seconds.
func (s *Server) GetLeaderLease() int64 {
return s.cfg.LeaderLease
}

// GetParticipant returns the participant.
func (s *Server) GetParticipant() *member.Participant {
return s.participant
Expand Down
7 changes: 7 additions & 0 deletions pkg/mcs/router/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,13 @@ func (s *Server) GetAddr() string {
return s.cfg.ListenAddr
}

// 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 (*Server) GetLeaderLease() int64 {
return 0
}

// GetAdvertiseListenAddr returns the advertise address of the server.
func (s *Server) GetAdvertiseListenAddr() string {
return s.cfg.AdvertiseListenAddr
Expand Down
5 changes: 5 additions & 0 deletions pkg/mcs/scheduling/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,11 @@ func (s *Server) GetConfig() *config.Config {
return cfg
}

// GetLeaderLease returns the configured leader lease in seconds.
func (s *Server) GetLeaderLease() int64 {
return s.cfg.LeaderLease
}

// CreateServer creates the Server
func CreateServer(ctx context.Context, cfg *config.Config) *Server {
svr := &Server{
Expand Down
5 changes: 5 additions & 0 deletions pkg/mcs/tso/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,11 @@ func (s *Server) GetConfig() *Config {
return s.cfg
}

// GetLeaderLease returns the configured leader lease in seconds.
func (s *Server) GetLeaderLease() int64 {
return s.cfg.GetLease()
}

// GetTLSConfig gets the security config.
func (s *Server) GetTLSConfig() *grpcutil.TLSConfig {
return &s.cfg.Security.TLSConfig
Expand Down
12 changes: 10 additions & 2 deletions pkg/mcs/utils/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ type server interface {
diagnosticspb.DiagnosticsServer
StartTimestamp() int64
Name() string
// GetLeaderLease returns the configured leader lease in seconds. It is
// reused as the service registry lease TTL. Services without a lease
// config should return 0, in which case discovery.DefaultLeaseInSeconds
// is used.
GetLeaderLease() int64
}

// InitClient initializes the etcd and http clients.
Expand Down Expand Up @@ -295,9 +300,12 @@ func Register(s server, serviceName string) (*discovery.ServiceRegistryEntry, *d
if err != nil {
return nil, nil, err
}
// 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)

@rleungx rleungx Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lhy1024 lhy1024 Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lhy1024 lhy1024 Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@rleungx rleungx Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

serviceRegister := discovery.NewServiceRegister(s.Context(), s.GetEtcdClient(),
serviceName, s.GetAdvertiseListenAddr(), serializedEntry,
discovery.DefaultLeaseInSeconds)
serviceName, s.GetAdvertiseListenAddr(), serializedEntry, lease)

@rleungx rleungx Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err := serviceRegister.Register(); err != nil {
log.Error("failed to register the service", zap.String("service-name", serviceName), errs.ZapError(err))
return nil, nil, err
Expand Down
Loading