From 875d36abe82965afc63450fa978d5e18eb6b192b Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Mon, 3 Apr 2023 21:57:56 +0000 Subject: [PATCH] :bug: Fix MachinePool node taint patching --- .../machinepool_controller_noderef.go | 62 ++++--- .../machinepool_controller_noderef_test.go | 151 ++++++++++++++++++ internal/util/taints/taints_test.go | 10 ++ 3 files changed, 199 insertions(+), 24 deletions(-) diff --git a/exp/internal/controllers/machinepool_controller_noderef.go b/exp/internal/controllers/machinepool_controller_noderef.go index 1d680ed058fe..d2990d74f0a2 100644 --- a/exp/internal/controllers/machinepool_controller_noderef.go +++ b/exp/internal/controllers/machinepool_controller_noderef.go @@ -55,6 +55,7 @@ func (r *MachinePoolReconciler) reconcileNodeRefs(ctx context.Context, cluster * } // Check that the Machine doesn't already have a NodeRefs. + // Return early if there is no work to do. if mp.Status.Replicas == mp.Status.ReadyReplicas && len(mp.Status.NodeRefs) == int(mp.Status.ReadyReplicas) { conditions.MarkTrue(mp, expv1.ReplicasReadyCondition) return ctrl.Result{}, nil @@ -94,30 +95,10 @@ func (r *MachinePoolReconciler) reconcileNodeRefs(ctx context.Context, cluster * log.Info("Set MachinePools's NodeRefs", "noderefs", mp.Status.NodeRefs) r.recorder.Event(mp, corev1.EventTypeNormal, "SuccessfulSetNodeRefs", fmt.Sprintf("%+v", mp.Status.NodeRefs)) - // Reconcile node annotations. - for _, nodeRef := range nodeRefsResult.references { - node := &corev1.Node{} - if err := clusterClient.Get(ctx, client.ObjectKey{Name: nodeRef.Name}, node); err != nil { - log.V(2).Info("Failed to get Node, skipping setting annotations", "err", err, "nodeRef.Name", nodeRef.Name) - continue - } - patchHelper, err := patch.NewHelper(node, clusterClient) - if err != nil { - return ctrl.Result{}, err - } - desired := map[string]string{ - clusterv1.ClusterNameAnnotation: mp.Spec.ClusterName, - clusterv1.ClusterNamespaceAnnotation: mp.GetNamespace(), - clusterv1.OwnerKindAnnotation: mp.Kind, - clusterv1.OwnerNameAnnotation: mp.Name, - } - // Add annotations and drop NodeUninitializedTaint. - if annotations.AddAnnotations(node, desired) || taints.RemoveNodeTaint(node, clusterv1.NodeUninitializedTaint) { - if err := patchHelper.Patch(ctx, node); err != nil { - log.V(2).Info("Failed patch node to set annotations and drop taints", "err", err, "node name", node.Name) - return ctrl.Result{}, err - } - } + // Reconcile node annotations and taints. + err = r.patchNodes(ctx, clusterClient, nodeRefsResult.references, mp) + if err != nil { + return ctrl.Result{}, err } if mp.Status.Replicas != mp.Status.ReadyReplicas || len(nodeRefsResult.references) != int(mp.Status.ReadyReplicas) { @@ -221,6 +202,39 @@ func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client. return getNodeReferencesResult{nodeRefs, available, ready}, nil } +// patchNodes patches the nodes with the cluster name and cluster namespace annotations. +func (r *MachinePoolReconciler) patchNodes(ctx context.Context, c client.Client, references []corev1.ObjectReference, mp *expv1.MachinePool) error { + log := ctrl.LoggerFrom(ctx) + for _, nodeRef := range references { + node := &corev1.Node{} + if err := c.Get(ctx, client.ObjectKey{Name: nodeRef.Name}, node); err != nil { + log.V(2).Info("Failed to get Node, skipping setting annotations", "err", err, "nodeRef.Name", nodeRef.Name) + continue + } + patchHelper, err := patch.NewHelper(node, c) + if err != nil { + return err + } + desired := map[string]string{ + clusterv1.ClusterNameAnnotation: mp.Spec.ClusterName, + clusterv1.ClusterNamespaceAnnotation: mp.GetNamespace(), + clusterv1.OwnerKindAnnotation: mp.Kind, + clusterv1.OwnerNameAnnotation: mp.Name, + } + // Add annotations and drop NodeUninitializedTaint. + hasAnnotationChanges := annotations.AddAnnotations(node, desired) + hasTaintChanges := taints.RemoveNodeTaint(node, clusterv1.NodeUninitializedTaint) + // Patch the node if needed. + if hasAnnotationChanges || hasTaintChanges { + if err := patchHelper.Patch(ctx, node); err != nil { + log.V(2).Info("Failed patch node to set annotations and drop taints", "err", err, "node name", node.Name) + return err + } + } + } + return nil +} + func nodeIsReady(node *corev1.Node) bool { for _, n := range node.Status.Conditions { if n.Type == corev1.NodeReady { diff --git a/exp/internal/controllers/machinepool_controller_noderef_test.go b/exp/internal/controllers/machinepool_controller_noderef_test.go index b82b8c1fe7a8..8e34ce06c844 100644 --- a/exp/internal/controllers/machinepool_controller_noderef_test.go +++ b/exp/internal/controllers/machinepool_controller_noderef_test.go @@ -23,6 +23,8 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -199,3 +201,152 @@ func TestMachinePoolGetNodeReference(t *testing.T) { }) } } + +func TestMachinePoolPatchNodes(t *testing.T) { + r := &MachinePoolReconciler{ + Client: fake.NewClientBuilder().Build(), + recorder: record.NewFakeRecorder(32), + } + + nodeList := []client.Object{ + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + }, + Spec: corev1.NodeSpec{ + ProviderID: "aws://us-east-1/id-node-1", + Taints: []corev1.Taint{ + clusterv1.NodeUninitializedTaint, + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-2", + Annotations: map[string]string{ + "foo": "bar", + }, + }, + Spec: corev1.NodeSpec{ + ProviderID: "aws://us-west-2/id-node-2", + Taints: []corev1.Taint{ + { + Key: "some-other-taint", + Value: "SomeEffect", + }, + clusterv1.NodeUninitializedTaint, + }, + }, + }, + } + + testCases := []struct { + name string + machinePool *expv1.MachinePool + nodeRefs []corev1.ObjectReference + expectedNodes []corev1.Node + err error + }{ + { + name: "Node with uninitialized taint should be patched", + machinePool: &expv1.MachinePool{ + TypeMeta: metav1.TypeMeta{ + Kind: "MachinePool", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool-1", + Namespace: "my-namespace", + }, + Spec: expv1.MachinePoolSpec{ + ClusterName: "cluster-1", + ProviderIDList: []string{"aws://us-east-1/id-node-1"}, + }, + }, + nodeRefs: []corev1.ObjectReference{ + {Name: "node-1"}, + }, + expectedNodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node-1", + Annotations: map[string]string{ + "cluster.x-k8s.io/cluster-name": "cluster-1", + "cluster.x-k8s.io/cluster-namespace": "my-namespace", + "cluster.x-k8s.io/owner-kind": "MachinePool", + "cluster.x-k8s.io/owner-name": "machinepool-1", + }, + }, + Spec: corev1.NodeSpec{ + Taints: nil, + }, + }, + }, + }, + { + name: "Node with existing annotations and taints should be patched", + machinePool: &expv1.MachinePool{ + TypeMeta: metav1.TypeMeta{ + Kind: "MachinePool", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool-2", + Namespace: "my-namespace", + }, + Spec: expv1.MachinePoolSpec{ + ClusterName: "cluster-1", + ProviderIDList: []string{"aws://us-west-2/id-node-2"}, + }, + }, + nodeRefs: []corev1.ObjectReference{ + {Name: "node-2"}, + }, + expectedNodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node-2", + Annotations: map[string]string{ + "cluster.x-k8s.io/cluster-name": "cluster-1", + "cluster.x-k8s.io/cluster-namespace": "my-namespace", + "cluster.x-k8s.io/owner-kind": "MachinePool", + "cluster.x-k8s.io/owner-name": "machinepool-2", + "foo": "bar", + }, + }, + Spec: corev1.NodeSpec{ + Taints: []corev1.Taint{ + { + Key: "some-other-taint", + Value: "SomeEffect", + }, + }, + }, + }, + }, + }, + } + + for _, test := range testCases { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + fakeClient := fake.NewClientBuilder().WithObjects(nodeList...).Build() + + err := r.patchNodes(ctx, fakeClient, test.nodeRefs, test.machinePool) + if test.err == nil { + g.Expect(err).To(BeNil()) + } else { + g.Expect(err).NotTo(BeNil()) + g.Expect(err).To(Equal(test.err), "Expected error %v, got %v", test.err, err) + } + + // Check that the nodes have the desired taints and annotations + for _, expected := range test.expectedNodes { + node := &corev1.Node{} + err := fakeClient.Get(ctx, client.ObjectKey{Name: expected.Name}, node) + g.Expect(err).To(BeNil()) + g.Expect(node.Annotations).To(Equal(expected.Annotations)) + g.Expect(node.Spec.Taints).To(Equal(expected.Spec.Taints)) + } + }) + } +} diff --git a/internal/util/taints/taints_test.go b/internal/util/taints/taints_test.go index f830c9573756..2777a79e30dd 100644 --- a/internal/util/taints/taints_test.go +++ b/internal/util/taints/taints_test.go @@ -55,6 +55,16 @@ func TestRemoveNodeTaint(t *testing.T) { wantTaints: []corev1.Taint{taint2}, wantModified: false, }, + { + name: "drop last taint should return true", + node: &corev1.Node{Spec: corev1.NodeSpec{ + Taints: []corev1.Taint{ + taint1, + }}}, + dropTaint: taint1, + wantTaints: []corev1.Taint{}, + wantModified: true, + }, } for _, tt := range tests {