vip, server: optimize failover for graceful shutdown with VIP (#625)

Author: djshow832

pkg/balance/metricsreader/backend_reader.go

@@ -39,7 +39,7 @@ const (
 	readerOwnerKeyPrefix = "/tiproxy/metric_reader"
 	readerOwnerKeySuffix = "owner"
 	// sessionTTL is the session's TTL in seconds for backend reader owner election.
-	sessionTTL = 30
+	sessionTTL = 15
 	// backendMetricPath is the path of backend HTTP API to read metrics.
 	backendMetricPath = "/metrics"
 	// ownerMetricPath is the path of reading backend metrics from the backend reader owner.

pkg/manager/elect/election.go

@@ -264,6 +264,7 @@ func (m *election) watchOwner(ctx context.Context, session *concurrency.Session,
 func (m *election) Close() {
 	if m.cancel != nil {
 		m.cancel()
+		m.cancel = nil
 	}
 	m.wg.Wait()
 }

pkg/manager/elect/election_test.go

@@ -56,6 +56,11 @@ func TestElectOwner(t *testing.T) {
 		_, err := elec.GetOwnerID(context.Background())
 		require.Error(t, err)
 	}
+	// double closing elections is allowed
+	{
+		elec := ts.getElection("3")
+		elec.Close()
+	}
 }
 
 func TestEtcdServerDown(t *testing.T) {

pkg/manager/vip/manager.go

@@ -6,6 +6,7 @@ package vip
 import (
 	"context"
 	"net"
+	"sync/atomic"
 
 	"github.com/pingcap/tiproxy/lib/config"
 	"github.com/pingcap/tiproxy/pkg/manager/elect"
@@ -17,23 +18,25 @@ const (
 	// vipKey is the key in etcd for VIP election.
 	vipKey = "/tiproxy/vip/owner"
 	// sessionTTL is the session's TTL in seconds for VIP election.
-	sessionTTL = 5
+	// The etcd client keeps alive every TTL/3 seconds.
+	// The TTL determines the failover time so it should be short.
+	sessionTTL = 3
 )
 
 type VIPManager interface {
 	Start(context.Context, *clientv3.Client) error
-	OnElected()
-	OnRetired()
+	Resign()
 	Close()
 }
 
 var _ VIPManager = (*vipManager)(nil)
 
 type vipManager struct {
-	operation NetworkOperation
-	cfgGetter config.ConfigGetter
-	election  elect.Election
-	lg        *zap.Logger
+	operation   NetworkOperation
+	cfgGetter   config.ConfigGetter
+	election    elect.Election
+	delOnRetire atomic.Bool
+	lg          *zap.Logger
 }
 
 func NewVIPManager(lg *zap.Logger, cfgGetter config.ConfigGetter) (*vipManager, error) {
@@ -55,6 +58,7 @@ func NewVIPManager(lg *zap.Logger, cfgGetter config.ConfigGetter) (*vipManager,
 		return nil, err
 	}
 	vm.operation = operation
+	vm.delOnRetire.Store(true)
 	return vm, nil
 }
 
@@ -81,6 +85,16 @@ func (vm *vipManager) Start(ctx context.Context, etcdCli *clientv3.Client) error
 }
 
 func (vm *vipManager) OnElected() {
+	vm.addVIP()
+}
+
+func (vm *vipManager) OnRetired() {
+	if vm.delOnRetire.Load() {
+		vm.delVIP()
+	}
+}
+
+func (vm *vipManager) addVIP() {
 	hasIP, err := vm.operation.HasIP()
 	if err != nil {
 		vm.lg.Error("checking addresses failed", zap.Error(err))
@@ -101,7 +115,7 @@ func (vm *vipManager) OnElected() {
 	vm.lg.Info("adding VIP success")
 }
 
-func (vm *vipManager) OnRetired() {
+func (vm *vipManager) delVIP() {
 	hasIP, err := vm.operation.HasIP()
 	if err != nil {
 		vm.lg.Error("checking addresses failed", zap.Error(err))
@@ -118,9 +132,19 @@ func (vm *vipManager) OnRetired() {
 	vm.lg.Info("deleting VIP success")
 }
 
-func (vm *vipManager) Close() {
-	// The OnRetired() will be called when the election is closed.
+// Resign stops compaign but does not delete the VIP.
+// It's called before graceful wait to avoid that the VIP is deleted before another member adds VIP.
+func (vm *vipManager) Resign() {
+	vm.delOnRetire.Store(false)
 	if vm.election != nil {
 		vm.election.Close()
+		vm.election = nil
 	}
 }
+
+// Close stops compaign and deletes the VIP.
+// It's called after graceful wait to ensure the VIP is finally deleted.
+func (vm *vipManager) Close() {
+	vm.Resign()
+	vm.delVIP()
+}

pkg/manager/vip/manager_test.go

@@ -123,6 +123,7 @@ func TestNetworkOperation(t *testing.T) {
 		cfgGetter: newMockConfigGetter(config.HA{VirtualIP: "10.10.10.10/24", Interface: "eth0"}),
 		operation: operation,
 	}
+	vm.delOnRetire.Store(true)
 	vm.election = newMockElection(ch, vm)
 	childCtx, cancel := context.WithCancel(context.Background())
 	vm.election.Start(childCtx)
@@ -142,3 +143,45 @@ func TestNetworkOperation(t *testing.T) {
 	cancel()
 	vm.Close()
 }
+
+func TestResign(t *testing.T) {
+	tests := []struct {
+		hasIP       bool
+		expectedLog string
+	}{
+		{
+			hasIP:       false,
+			expectedLog: "do nothing",
+		},
+		{
+			hasIP:       true,
+			expectedLog: "deleting VIP success",
+		},
+	}
+
+	for i, test := range tests {
+		lg, text := logger.CreateLoggerForTest(t)
+		operation := newMockNetworkOperation()
+		vm := &vipManager{
+			lg:        lg,
+			cfgGetter: newMockConfigGetter(config.HA{VirtualIP: "10.10.10.10/24", Interface: "eth0"}),
+			operation: operation,
+		}
+		vm.delOnRetire.Store(true)
+		operation.hasIP.Store(test.hasIP)
+
+		// resign doesn't delete vip
+		vm.Resign()
+		if test.hasIP {
+			vm.OnRetired()
+		}
+		time.Sleep(100 * time.Millisecond)
+		require.False(t, strings.Contains(text.String(), test.expectedLog), "case %d", i)
+
+		// close deletes vip
+		vm.Close()
+		require.Eventually(t, func() bool {
+			return strings.Contains(text.String(), test.expectedLog)
+		}, 3*time.Second, 10*time.Millisecond, "case %d", i)
+	}
+}

pkg/server/server.go

@@ -209,13 +209,17 @@ func (s *Server) Close() error {
 	metrics.ServerEventCounter.WithLabelValues(metrics.EventClose).Inc()
 
 	errs := make([]error, 0, 4)
-	// Remove the VIP before closing the SQL server so that clients connect to other nodes.
+	// Resign the VIP owner before graceful wait so that clients connect to other nodes.
 	if s.vipManager != nil && !reflect.ValueOf(s.vipManager).IsNil() {
-		s.vipManager.Close()
+		s.vipManager.Resign()
 	}
 	if s.proxy != nil {
 		errs = append(errs, s.proxy.Close())
 	}
+	// Delete VIP after graceful wait.
+	if s.vipManager != nil && !reflect.ValueOf(s.vipManager).IsNil() {
+		s.vipManager.Close()
+	}
 	if s.apiServer != nil {
 		errs = append(errs, s.apiServer.Close())
 	}