Drop unnecessary etcd call from KCP

Author: fabriziopandini

controlplane/kubeadm/internal/controllers/controller.go

@@ -1095,7 +1095,25 @@ func (r *KubeadmControlPlaneReconciler) reconcileEtcdMembers(ctx context.Context
 		return nil
 	}
 
+	// No op if there are potential issues affecting the list of etcdMembers
+	if !controlPlane.EtcdMembersAgreeOnMemberList || !controlPlane.EtcdMembersAgreeOnClusterID {
+		return nil
+	}
+
+	// No op if for any reason the etcdMember list is not populated at this stage.
+	if controlPlane.EtcdMembers == nil {
+		return nil
+	}
+
+	// Potential inconsistencies between the list of members and the list of machines/nodes are
+	// surfaced using the EtcdClusterHealthyCondition; if this condition is true, meaning no inconsistencies exists, return early.
+	if conditions.IsTrue(controlPlane.KCP, controlplanev1.EtcdClusterHealthyCondition) {
+		return nil
+	}
+
 	// Collect all the node names.
+	// Note: EtcdClusterHealthyCondition true also implies that there are no machines still provisioning,
+	// so we can ignore this case.
 	nodeNames := []string{}
 	for _, machine := range controlPlane.Machines {
 		if machine.Status.NodeRef == nil {
@@ -1105,19 +1123,13 @@ func (r *KubeadmControlPlaneReconciler) reconcileEtcdMembers(ctx context.Context
 		nodeNames = append(nodeNames, machine.Status.NodeRef.Name)
 	}
 
-	// Potential inconsistencies between the list of members and the list of machines/nodes are
-	// surfaced using the EtcdClusterHealthyCondition; if this condition is true, meaning no inconsistencies exists, return early.
-	if conditions.IsTrue(controlPlane.KCP, controlplanev1.EtcdClusterHealthyCondition) {
-		return nil
-	}
-
 	workloadCluster, err := controlPlane.GetWorkloadCluster(ctx)
 	if err != nil {
 		// Failing at connecting to the workload cluster can mean workload cluster is unhealthy for a variety of reasons such as etcd quorum loss.
 		return errors.Wrap(err, "cannot get remote client to workload cluster")
 	}
 
-	removedMembers, err := workloadCluster.ReconcileEtcdMembers(ctx, nodeNames)
+	removedMembers, err := workloadCluster.ReconcileEtcdMembersAndControlPlaneNodes(ctx, controlPlane.EtcdMembers, nodeNames)
 	if err != nil {
 		return errors.Wrap(err, "failed attempt to reconcile etcd members")
 	}

controlplane/kubeadm/internal/controllers/fakes_test.go

@@ -27,6 +27,7 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
+	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/etcd"
 	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util/collections"
 )
@@ -85,7 +86,7 @@ func (f *fakeWorkloadCluster) ForwardEtcdLeadership(_ context.Context, _ *cluste
 	return nil
 }
 
-func (f *fakeWorkloadCluster) ReconcileEtcdMembers(_ context.Context, _ []string) ([]string, error) {
+func (f *fakeWorkloadCluster) ReconcileEtcdMembersAndControlPlaneNodes(_ context.Context, _ []*etcd.Member, _ []string) ([]string, error) {
 	return nil, nil
 }
 

controlplane/kubeadm/internal/workload_cluster.go

@@ -46,6 +46,7 @@ import (
 	bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
 	kubeadmtypes "sigs.k8s.io/cluster-api/bootstrap/kubeadm/types"
 	controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1"
+	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/etcd"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/proxy"
 	"sigs.k8s.io/cluster-api/internal/util/kubeadm"
 	"sigs.k8s.io/cluster-api/util"
@@ -117,7 +118,7 @@ type WorkloadCluster interface {
 	UpdateClusterConfiguration(ctx context.Context, version semver.Version, mutators ...func(*bootstrapv1.ClusterConfiguration)) error
 
 	// State recovery tasks.
-	ReconcileEtcdMembers(ctx context.Context, nodeNames []string) ([]string, error)
+	ReconcileEtcdMembersAndControlPlaneNodes(ctx context.Context, members []*etcd.Member, nodeNames []string) ([]string, error)
 }
 
 // Workload defines operations on workload clusters.

controlplane/kubeadm/internal/workload_cluster_etcd.go

@@ -33,37 +33,14 @@ type etcdClientFor interface {
 	forLeader(ctx context.Context, nodeNames []string) (*etcd.Client, error)
 }
 
-// ReconcileEtcdMembers iterates over all etcd members and finds members that do not have corresponding nodes.
+// ReconcileEtcdMembersAndControlPlaneNodes iterates over all etcd members and finds members that do not have corresponding nodes.
 // If there are any such members, it deletes them from etcd and removes their nodes from the kubeadm configmap so that kubeadm does not run etcd health checks on them.
-func (w *Workload) ReconcileEtcdMembers(ctx context.Context, nodeNames []string) ([]string, error) {
-	allRemovedMembers := []string{}
-	allErrs := []error{}
-	for _, nodeName := range nodeNames {
-		removedMembers, errs := w.reconcileEtcdMember(ctx, nodeNames, nodeName)
-		allRemovedMembers = append(allRemovedMembers, removedMembers...)
-		allErrs = append(allErrs, errs...)
-	}
-
-	return allRemovedMembers, kerrors.NewAggregate(allErrs)
-}
-
-func (w *Workload) reconcileEtcdMember(ctx context.Context, nodeNames []string, nodeName string) ([]string, []error) {
-	// Create the etcd Client for the etcd Pod scheduled on the Node
-	etcdClient, err := w.etcdClientGenerator.forFirstAvailableNode(ctx, []string{nodeName})
-	if err != nil {
-		return nil, nil
-	}
-	defer etcdClient.Close()
-
-	members, err := etcdClient.Members(ctx)
-	if err != nil {
-		return nil, nil
-	}
-
+func (w *Workload) ReconcileEtcdMembersAndControlPlaneNodes(ctx context.Context, members []*etcd.Member, nodeNames []string) ([]string, error) {
 	// Check if any member's node is missing from workload cluster
 	// If any, delete it with best effort
 	removedMembers := []string{}
 	errs := []error{}
+
 loopmembers:
 	for _, member := range members {
 		// If this member is just added, it has a empty name until the etcd pod starts. Ignore it.
@@ -84,7 +61,8 @@ loopmembers:
 			errs = append(errs, err)
 		}
 	}
-	return removedMembers, errs
+
+	return removedMembers, kerrors.NewAggregate(errs)
 }
 
 // UpdateEtcdLocalInKubeadmConfigMap sets etcd local configuration in the kubeadm config map.

controlplane/kubeadm/internal/workload_cluster_etcd_test.go

@@ -552,7 +552,7 @@ func TestForwardEtcdLeadership(t *testing.T) {
 	})
 }
 
-func TestReconcileEtcdMembers(t *testing.T) {
+func TestReconcileEtcdMembersAndControlPlaneNodes(t *testing.T) {
 	kubeadmConfig := &corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      kubeadmConfigKey,
@@ -590,13 +590,15 @@ func TestReconcileEtcdMembers(t *testing.T) {
 	}
 	node2 := node1.DeepCopy()
 	node2.Name = "ip-10-0-0-2.ec2.internal"
+	node3 := node1.DeepCopy()
+	node3.Name = "ip-10-0-0-3.ec2.internal"
 
 	fakeEtcdClient := &fake2.FakeEtcdClient{
 		MemberListResponse: &clientv3.MemberListResponse{
 			Members: []*pb.Member{
-				{Name: "ip-10-0-0-1.ec2.internal", ID: uint64(1)},
-				{Name: "ip-10-0-0-2.ec2.internal", ID: uint64(2)},
-				{Name: "ip-10-0-0-3.ec2.internal", ID: uint64(3)},
+				{Name: node1.Name, ID: uint64(1)},
+				{Name: node2.Name, ID: uint64(2)},
+				{Name: node3.Name, ID: uint64(3)},
 			},
 		},
 		AlarmResponse: &clientv3.AlarmResponse{
@@ -607,16 +609,51 @@ func TestReconcileEtcdMembers(t *testing.T) {
 	tests := []struct {
 		name                string
 		objs                []client.Object
+		members             []*etcd.Member
 		nodes               []string
 		etcdClientGenerator etcdClientFor
 		expectErr           bool
 		assert              func(*WithT, client.Client)
 	}{
+		{
+			// no op if nodes and members match
+			name: "no op if nodes and members match",
+			objs: []client.Object{node1.DeepCopy(), node2.DeepCopy(), node3.DeepCopy(), kubeadmConfigWithoutClusterStatus.DeepCopy()},
+			members: []*etcd.Member{
+				{Name: node1.Name, ID: uint64(1)},
+				{Name: node2.Name, ID: uint64(2)},
+				{Name: node3.Name, ID: uint64(3)},
+			},
+			nodes: []string{node1.Name, node2.Name, node3.Name},
+			etcdClientGenerator: &fakeEtcdClientGenerator{
+				forNodesClient: &etcd.Client{
+					EtcdClient: fakeEtcdClient,
+				},
+			},
+			expectErr: false,
+			assert: func(g *WithT, c client.Client) {
+				g.Expect(fakeEtcdClient.RemovedMember).To(Equal(uint64(0))) // no member removed
+
+				var actualConfig corev1.ConfigMap
+				g.Expect(c.Get(
+					ctx,
+					client.ObjectKey{Name: kubeadmConfigKey, Namespace: metav1.NamespaceSystem},
+					&actualConfig,
+				)).To(Succeed())
+				// Kubernetes version >= 1.22.0 does not have ClusterStatus
+				g.Expect(actualConfig.Data).ToNot(HaveKey(clusterStatusKey))
+			},
+		},
 		{
 			// the node to be removed is ip-10-0-0-3.ec2.internal since the
 			// other two have nodes
-			name:  "successfully removes the etcd member without a node",
-			objs:  []client.Object{node1.DeepCopy(), node2.DeepCopy(), kubeadmConfigWithoutClusterStatus.DeepCopy()},
+			name: "successfully removes the etcd member without a node",
+			objs: []client.Object{node1.DeepCopy(), node2.DeepCopy(), kubeadmConfigWithoutClusterStatus.DeepCopy()},
+			members: []*etcd.Member{
+				{Name: node1.Name, ID: uint64(1)},
+				{Name: node2.Name, ID: uint64(2)},
+				{Name: node3.Name, ID: uint64(3)},
+			},
 			nodes: []string{node1.Name, node2.Name},
 			etcdClientGenerator: &fakeEtcdClientGenerator{
 				forNodesClient: &etcd.Client{
@@ -638,8 +675,13 @@ func TestReconcileEtcdMembers(t *testing.T) {
 			},
 		},
 		{
-			name:  "return error if there aren't enough control plane nodes",
-			objs:  []client.Object{node1.DeepCopy(), kubeadmConfig.DeepCopy()},
+			// only one node left, no removal should happen
+			name: "return error if there aren't enough control plane nodes",
+			objs: []client.Object{node1.DeepCopy(), kubeadmConfig.DeepCopy()},
+			members: []*etcd.Member{
+				{Name: "ip-10-0-0-1.ec2.internal", ID: uint64(1)},
+				{Name: "ip-10-0-0-2.ec2.internal", ID: uint64(2)},
+			},
 			nodes: []string{node1.Name},
 			etcdClientGenerator: &fakeEtcdClientGenerator{
 				forNodesClient: &etcd.Client{
@@ -666,7 +708,7 @@ func TestReconcileEtcdMembers(t *testing.T) {
 				etcdClientGenerator: tt.etcdClientGenerator,
 			}
 			ctx := context.TODO()
-			_, err := w.ReconcileEtcdMembers(ctx, tt.nodes)
+			_, err := w.ReconcileEtcdMembersAndControlPlaneNodes(ctx, tt.members, tt.nodes)
 			if tt.expectErr {
 				g.Expect(err).To(HaveOccurred())
 				return