OCPBUGS-42810: actively move bootstrap member lead

pkg/etcdcli/etcdcli.go

@@ -264,6 +264,20 @@ func (g *etcdClientGetter) MemberRemove(ctx context.Context, memberID uint64) er
 	return err
 }
 
+func (g *etcdClientGetter) MoveLeader(ctx context.Context, toMember uint64) error {
+	cli, err := g.clientPool.Get()
+	if err != nil {
+		return err
+	}
+
+	defer g.clientPool.Return(cli)
+
+	ctx, cancel := context.WithTimeout(ctx, DefaultClientTimeout)
+	defer cancel()
+	_, err = cli.MoveLeader(ctx, toMember)
+	return err
+}
+
 func (g *etcdClientGetter) MemberList(ctx context.Context) ([]*etcdserverpb.Member, error) {
 	cli, err := g.clientPool.Get()
 	if err != nil {
@@ -443,26 +457,20 @@ func (g *etcdClientGetter) IsMemberHealthy(ctx context.Context, member *etcdserv
 	return false, nil
 }
 
-func (g *etcdClientGetter) MemberStatus(ctx context.Context, member *etcdserverpb.Member) string {
+func (g *etcdClientGetter) MemberStatus(ctx context.Context, member *etcdserverpb.Member) (*clientv3.StatusResponse, error) {
 	cli, err := g.clientPool.Get()
 	if err != nil {
-		klog.Errorf("error getting etcd client: %#v", err)
-		return EtcdMemberStatusUnknown
+		return nil, err
 	}
 	defer g.clientPool.Return(cli)
 
-	if len(member.ClientURLs) == 0 && member.Name == "" {
-		return EtcdMemberStatusNotStarted
+	if len(member.ClientURLs) == 0 {
+		return nil, fmt.Errorf("member has no etcd clientURLs")
 	}
 	ctx, cancel := context.WithTimeout(ctx, DefaultClientTimeout)
 	defer cancel()
-	_, err = cli.Status(ctx, member.ClientURLs[0])
-	if err != nil {
-		klog.Errorf("error getting etcd member %s status: %#v", member.Name, err)
-		return EtcdMemberStatusUnhealthy
-	}
 
-	return EtcdMemberStatusAvailable
+	return cli.Status(ctx, member.ClientURLs[0])
 }
 
 // Defragment creates a new uncached clientv3 to the given member url and calls clientv3.Client.Defragment.

pkg/etcdcli/helpers.go

@@ -105,6 +105,14 @@ func (f *fakeEtcdClient) MemberRemove(ctx context.Context, memberID uint64) erro
 	return nil
 }
 
+func (f *fakeEtcdClient) MoveLeader(ctx context.Context, toMember uint64) error {
+	for _, status := range f.opts.status {
+		status.Leader = toMember
+	}
+
+	return nil
+}
+
 func (f *fakeEtcdClient) MemberHealth(ctx context.Context) (memberHealth, error) {
 	var healthy, unhealthy int
 	var memberHealth memberHealth
@@ -162,8 +170,18 @@ func (f *fakeEtcdClient) HealthyVotingMembers(ctx context.Context) ([]*etcdserve
 	return filterVotingMembers(members), nil
 }
 
-func (f *fakeEtcdClient) MemberStatus(ctx context.Context, member *etcdserverpb.Member) string {
-	panic("implement me")
+func (f *fakeEtcdClient) MemberStatus(ctx context.Context, member *etcdserverpb.Member) (*clientv3.StatusResponse, error) {
+	// Find the status for this member
+	for _, status := range f.opts.status {
+		if status.Header != nil && status.Header.MemberId == member.ID {
+			return status, nil
+		}
+	}
+	// Return a default status if none found
+	return &clientv3.StatusResponse{
+		Header: &etcdserverpb.ResponseHeader{MemberId: member.ID},
+		Leader: member.ID, // Default to self as leader
+	}, nil
 }
 
 func (f *fakeEtcdClient) GetMember(ctx context.Context, name string) (*etcdserverpb.Member, error) {
@@ -251,6 +269,14 @@ func WithFakeStatus(status []*clientv3.StatusResponse) FakeClientOption {
 	}
 }
 
+func WithLeader(leader uint64) FakeClientOption {
+	return func(fo *FakeClientOptions) {
+		for _, status := range fo.status {
+			status.Leader = leader
+		}
+	}
+}
+
 // WithFakeDefragErrors configures each call to Defrag to consume one error from the given slice
 func WithFakeDefragErrors(errors []error) FakeClientOption {
 	return func(fo *FakeClientOptions) {

pkg/etcdcli/interfaces.go

@@ -25,6 +25,7 @@ type EtcdClient interface {
 	HealthyMemberLister
 	UnhealthyMemberLister
 	MemberStatusChecker
+	LeaderMover
 	Status
 
 	GetMember(ctx context.Context, name string) (*etcdserverpb.Member, error)
@@ -64,6 +65,10 @@ type MemberRemover interface {
 	MemberRemove(ctx context.Context, memberID uint64) error
 }
 
+type LeaderMover interface {
+	MoveLeader(ctx context.Context, toMember uint64) error
+}
+
 type MemberLister interface {
 	// MemberList lists all members in a cluster
 	MemberList(ctx context.Context) ([]*etcdserverpb.Member, error)
@@ -86,5 +91,6 @@ type UnhealthyMemberLister interface {
 }
 
 type MemberStatusChecker interface {
-	MemberStatus(ctx context.Context, member *etcdserverpb.Member) string
+	// MemberStatus will return the etcd status response for the given member
+	MemberStatus(ctx context.Context, member *etcdserverpb.Member) (*clientv3.StatusResponse, error)
 }

pkg/operator/bootstrapteardown/bootstrap_teardown_controller.go

@@ -3,9 +3,11 @@ package bootstrapteardown
 import (
 	"context"
 	"fmt"
-	"k8s.io/client-go/tools/cache"
 	"time"
 
+	"go.etcd.io/etcd/api/v3/etcdserverpb"
+	"k8s.io/client-go/tools/cache"
+
 	operatorv1 "github.com/openshift/api/operator/v1"
 	configv1listers "github.com/openshift/client-go/config/listers/config/v1"
 	"github.com/openshift/cluster-etcd-operator/pkg/operator/health"
@@ -68,12 +70,12 @@ func (c *BootstrapTeardownController) sync(ctx context.Context, _ factory.SyncCo
 		return fmt.Errorf("failed to get bootstrap scaling strategy: %w", err)
 	}
 	// checks the actual etcd cluster membership API if etcd-bootstrap exists
-	safeToRemoveBootstrap, hasBootstrap, bootstrapID, err := c.canRemoveEtcdBootstrap(ctx, scalingStrategy)
+	safeToRemoveBootstrap, hasBootstrap, bootstrapMember, err := c.canRemoveEtcdBootstrap(ctx, scalingStrategy)
 	if err != nil {
 		return fmt.Errorf("error while canRemoveEtcdBootstrap: %w", err)
 	}
 
-	err = c.removeBootstrap(timeoutCtx, safeToRemoveBootstrap, hasBootstrap, bootstrapID)
+	err = c.removeBootstrap(timeoutCtx, safeToRemoveBootstrap, hasBootstrap, bootstrapMember)
 	if err != nil {
 		_, _, updateErr := v1helpers.UpdateStatus(ctx, c.operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{
 			Type:    "BootstrapTeardownDegraded",
@@ -96,13 +98,13 @@ func (c *BootstrapTeardownController) sync(ctx context.Context, _ factory.SyncCo
 	return updateErr
 }
 
-func (c *BootstrapTeardownController) removeBootstrap(ctx context.Context, safeToRemoveBootstrap bool, hasBootstrap bool, bootstrapID uint64) error {
+func (c *BootstrapTeardownController) removeBootstrap(ctx context.Context, safeToRemoveBootstrap bool, hasBootstrap bool, bootstrapMember *etcdserverpb.Member) error {
 	if !hasBootstrap {
 		klog.V(4).Infof("no bootstrap anymore setting removal status")
 		// this is to ensure the status is always set correctly, even if the status update below failed
-		updateErr := setSuccessfulBoostrapRemovalStatus(ctx, c.operatorClient)
+		updateErr := setSuccessfulBootstrapRemovalStatus(ctx, c.operatorClient)
 		if updateErr != nil {
-			return fmt.Errorf("error while setSuccessfulBoostrapRemovalStatus: %w", updateErr)
+			return fmt.Errorf("error while setSuccessfulBootstrapRemovalStatus: %w", updateErr)
 		}
 
 		// if the bootstrap isn't present, then clearly we're available enough to terminate. This avoids any risk of flapping.
@@ -120,6 +122,7 @@ func (c *BootstrapTeardownController) removeBootstrap(ctx context.Context, safeT
 		return nil
 	}
 
+	bootstrapID := bootstrapMember.ID
 	if !safeToRemoveBootstrap {
 		_, _, updateErr := v1helpers.UpdateStatus(ctx, c.operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{
 			Type:    "EtcdRunningInCluster",
@@ -147,6 +150,29 @@ func (c *BootstrapTeardownController) removeBootstrap(ctx context.Context, safeT
 	if isBootstrapComplete, err := bootstrap.IsBootstrapComplete(c.configmapLister); !isBootstrapComplete || err != nil {
 		return err
 	}
+
+	moved, err := c.ensureBootstrapIsNotLeader(ctx, bootstrapMember)
+	if err != nil {
+		return err
+	}
+
+	// if we have just moved it, we will skip this sync iteration to backoff the controller - the next resync will happen after a minute anyway
+	if moved {
+		klog.Warningf("Leader just moved, waiting for next resync to remove bootstrap member [%x]", bootstrapID)
+		c.eventRecorder.Eventf("Bootstrap member was leader and was moved away", "bootstrap member [%x] should no longer be leader", bootstrapID)
+		_, _, updateErr := v1helpers.UpdateStatus(ctx, c.operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{
+			Type:    "EtcdLeaderMovedAwayFromBootstrap",
+			Status:  operatorv1.ConditionTrue,
+			Reason:  "BootstrapIsLeader",
+			Message: "bootstrap was leader and has been moved",
+		}))
+		if updateErr != nil {
+			return fmt.Errorf("error while updating EtcdLeaderMovedAwayFromBootstrap: %w", updateErr)
+		}
+
+		return nil
+	}
+
 	klog.Warningf("Removing bootstrap member [%x]", bootstrapID)
 	c.eventRecorder.Eventf("Removing bootstrap member", "attempting to remove bootstrap member [%x]", bootstrapID)
 
@@ -159,10 +185,10 @@ func (c *BootstrapTeardownController) removeBootstrap(ctx context.Context, safeT
 	c.eventRecorder.Eventf("Bootstrap member removed", "successfully removed bootstrap member [%x]", bootstrapID)
 	// below might fail, since the member removal can cause some downtime for raft to settle on a quorum
 	// it's important that everything below is properly retried above during normal controller reconciliation
-	return setSuccessfulBoostrapRemovalStatus(ctx, c.operatorClient)
+	return setSuccessfulBootstrapRemovalStatus(ctx, c.operatorClient)
 }
 
-func setSuccessfulBoostrapRemovalStatus(ctx context.Context, client v1helpers.StaticPodOperatorClient) error {
+func setSuccessfulBootstrapRemovalStatus(ctx context.Context, client v1helpers.StaticPodOperatorClient) error {
 	_, _, updateErr := v1helpers.UpdateStatus(ctx, client, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{
 		Type:    "EtcdBootstrapMemberRemoved",
 		Status:  operatorv1.ConditionTrue,
@@ -173,57 +199,76 @@ func setSuccessfulBoostrapRemovalStatus(ctx context.Context, client v1helpers.St
 }
 
 // canRemoveEtcdBootstrap returns whether it is safe to remove bootstrap, whether bootstrap is in the list, and an error
-func (c *BootstrapTeardownController) canRemoveEtcdBootstrap(ctx context.Context, scalingStrategy ceohelpers.BootstrapScalingStrategy) (bool, bool, uint64, error) {
+func (c *BootstrapTeardownController) canRemoveEtcdBootstrap(ctx context.Context, scalingStrategy ceohelpers.BootstrapScalingStrategy) (bool, bool, *etcdserverpb.Member, error) {
 	members, err := c.etcdClient.MemberList(ctx)
 	if err != nil {
-		return false, false, 0, err
+		return false, false, nil, err
 	}
 
 	var hasBootstrap bool
-	var bootstrapMemberID uint64
+	var bootstrapMember *etcdserverpb.Member
 	for _, member := range members {
 		if member.Name == "etcd-bootstrap" {
 			hasBootstrap = true
-			bootstrapMemberID = member.ID
+			bootstrapMember = member
 			break
 		}
 	}
 	if !hasBootstrap {
-		return false, hasBootstrap, bootstrapMemberID, nil
+		return false, hasBootstrap, bootstrapMember, nil
 	}
 
 	// First, enforce the main HA invariants in terms of member counts.
 	switch scalingStrategy {
 	case ceohelpers.HAScalingStrategy:
 		if len(members) < 4 {
-			return false, hasBootstrap, bootstrapMemberID, nil
+			return false, hasBootstrap, bootstrapMember, nil
 		}
 	case ceohelpers.DelayedHAScalingStrategy, ceohelpers.TwoNodeScalingStrategy:
 		if len(members) < 3 {
-			return false, hasBootstrap, bootstrapMemberID, nil
+			return false, hasBootstrap, bootstrapMember, nil
 		}
 	case ceohelpers.UnsafeScalingStrategy, ceohelpers.DelayedTwoNodeScalingStrategy:
 		if len(members) < 2 {
-			return false, hasBootstrap, bootstrapMemberID, nil
+			return false, hasBootstrap, bootstrapMember, nil
 		}
 	}
 
 	// Next, given member counts are satisfied, check member health.
 	unhealthyMembers, err := c.etcdClient.UnhealthyMembers(ctx)
 	if err != nil {
-		return false, hasBootstrap, bootstrapMemberID, nil
+		return false, hasBootstrap, bootstrapMember, nil
 	}
 
 	// the etcd-bootstrap member is allowed to be unhealthy and can still be removed
 	switch {
 	case len(unhealthyMembers) == 0:
-		return true, hasBootstrap, bootstrapMemberID, nil
+		return true, hasBootstrap, bootstrapMember, nil
 	case len(unhealthyMembers) > 1:
-		return false, hasBootstrap, bootstrapMemberID, nil
+		return false, hasBootstrap, bootstrapMember, nil
 	default:
 		if unhealthyMembers[0].Name == "etcd-bootstrap" {
-			return true, true, unhealthyMembers[0].ID, nil
+			return true, true, unhealthyMembers[0], nil
 		}
-		return false, hasBootstrap, bootstrapMemberID, nil
+		return false, hasBootstrap, bootstrapMember, nil
 	}
 }
+
+func (c *BootstrapTeardownController) ensureBootstrapIsNotLeader(ctx context.Context, bootstrapMember *etcdserverpb.Member) (bool, error) {
+	members, err := c.etcdClient.MemberList(ctx)
+	if err != nil {
+		return false, fmt.Errorf("could not list while ensuring bootstrap is not the leader: %w", err)
+	}
+
+	leader, err := ceohelpers.FindLeader(ctx, c.etcdClient, members)
+	if err != nil || leader == nil {
+		return false, fmt.Errorf("could not find leader: %w", err)
+	}
+
+	if bootstrapMember.ID != leader.ID {
+		return false, nil
+	}
+
+	klog.Warningf("Bootstrap member [%x] (%s) detected as leader, trying to move elsewhere...", bootstrapMember.ID, bootstrapMember.GetClientURLs()[0])
+	return ceohelpers.MoveLeaderToAnotherMember(ctx, c.etcdClient, leader, members)
+}