elect, vip: fix incorrect metric_reader owner metrics (#637)

Author: djshow832

pkg/balance/metricsreader/backend_reader.go

@@ -556,8 +556,6 @@ func (br *BackendReader) PreClose() {
 	if br.election != nil {
 		br.election.Close()
 	}
-	// pretend to be not owner
-	br.isOwner.Store(false)
 }
 
 func (br *BackendReader) Close() {

pkg/manager/elect/election.go

@@ -287,12 +287,14 @@ func (m *election) watchOwner(ctx context.Context, session *concurrency.Session,
 	}
 }
 
-// Close is typically called before graceful shutdown. It resigns but doesn't retire or wait for the new owner.
-// The caller has to decide if it should retire after graceful wait.
+// Close resigns and retires.
 func (m *election) Close() {
 	if m.cancel != nil {
 		m.cancel()
 		m.cancel = nil
 	}
 	m.wg.Wait()
+	if m.isOwner {
+		m.onRetired()
+	}
 }

pkg/manager/elect/election_test.go

@@ -33,7 +33,7 @@ func TestElectOwner(t *testing.T) {
 		ownerID := ts.getOwnerID()
 		elec := ts.getElection(ownerID)
 		elec.Close()
-		ts.expectNoEvent(ownerID)
+		ts.expectEvent(ownerID, eventTypeRetired)
 		ownerID2 := ts.getOwnerID()
 		require.NotEqual(t, ownerID, ownerID2)
 		ts.expectEvent(ownerID2, eventTypeElected)
@@ -51,11 +51,10 @@ func TestElectOwner(t *testing.T) {
 	{
 		elec := ts.getElection("3")
 		elec.Close()
-		ts.expectNoEvent("3")
 		ownerID := ts.getOwnerID()
 		elec = ts.getElection(ownerID)
 		elec.Close()
-		ts.expectNoEvent(ownerID)
+		ts.expectEvent(ownerID, eventTypeRetired)
 		_, err := elec.GetOwnerID(context.Background())
 		require.Error(t, err)
 	}
@@ -159,4 +158,7 @@ func TestOwnerMetric(t *testing.T) {
 	checkMetric("key", false)
 	checkMetric("key2/1", true)
 	checkMetric("key3/1", true)
+
+	elec3.Close()
+	checkMetric("key3/1", false)
 }

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"
@@ -31,10 +32,11 @@ type VIPManager interface {
 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
+	lg          *zap.Logger
+	delOnRetire atomic.Bool
 }
 
 func NewVIPManager(lg *zap.Logger, cfgGetter config.ConfigGetter) (*vipManager, error) {
@@ -62,6 +64,7 @@ func NewVIPManager(lg *zap.Logger, cfgGetter config.ConfigGetter) (*vipManager,
 func (vm *vipManager) Start(ctx context.Context, etcdCli *clientv3.Client) error {
 	// This node may have bound the VIP before last failure.
 	vm.delVIP()
+	vm.delOnRetire.Store(true)
 
 	cfg := vm.cfgGetter.GetConfig()
 	ip, port, _, err := cfg.GetIPPort()
@@ -81,7 +84,9 @@ func (vm *vipManager) OnElected() {
 }
 
 func (vm *vipManager) OnRetired() {
-	vm.delVIP()
+	if vm.delOnRetire.Load() {
+		vm.delVIP()
+	}
 }
 
 func (vm *vipManager) addVIP() {
@@ -125,6 +130,7 @@ func (vm *vipManager) delVIP() {
 // PreClose resigns the owner but doesn't delete the VIP.
 // It makes use of the graceful-wait time to wait for the new owner to shorten the failover time.
 func (vm *vipManager) PreClose() {
+	vm.delOnRetire.Store(false)
 	if vm.election != nil {
 		vm.election.Close()
 	}

pkg/manager/vip/manager_test.go

@@ -126,6 +126,7 @@ func TestNetworkOperation(t *testing.T) {
 		cfgGetter: newMockConfigGetter(&config.Config{HA: 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)