From d07edd0a706cf5d54e90302d8d135443b6605380 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Wed, 27 Nov 2024 21:26:30 +0100 Subject: [PATCH] Drop unnecessary etcd call from KCP --- .../internal/controllers/controller.go | 26 +++++--- .../internal/controllers/fakes_test.go | 3 +- .../kubeadm/internal/workload_cluster.go | 3 +- .../kubeadm/internal/workload_cluster_etcd.go | 32 ++-------- .../internal/workload_cluster_etcd_test.go | 60 ++++++++++++++++--- 5 files changed, 79 insertions(+), 45 deletions(-) diff --git a/controlplane/kubeadm/internal/controllers/controller.go b/controlplane/kubeadm/internal/controllers/controller.go index c1b4710e4331..1a0bf8170363 100644 --- a/controlplane/kubeadm/internal/controllers/controller.go +++ b/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") } diff --git a/controlplane/kubeadm/internal/controllers/fakes_test.go b/controlplane/kubeadm/internal/controllers/fakes_test.go index a88b30c7dfae..948d47f641bf 100644 --- a/controlplane/kubeadm/internal/controllers/fakes_test.go +++ b/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 } diff --git a/controlplane/kubeadm/internal/workload_cluster.go b/controlplane/kubeadm/internal/workload_cluster.go index 9bb758743a5b..048df78ff59f 100644 --- a/controlplane/kubeadm/internal/workload_cluster.go +++ b/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. diff --git a/controlplane/kubeadm/internal/workload_cluster_etcd.go b/controlplane/kubeadm/internal/workload_cluster_etcd.go index 38a41635b46f..07f53f5090e3 100644 --- a/controlplane/kubeadm/internal/workload_cluster_etcd.go +++ b/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. diff --git a/controlplane/kubeadm/internal/workload_cluster_etcd_test.go b/controlplane/kubeadm/internal/workload_cluster_etcd_test.go index df9ccf9d252b..8327c58573e3 100644 --- a/controlplane/kubeadm/internal/workload_cluster_etcd_test.go +++ b/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