Skip to content

Commit 52fc099

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #8462 from CecileRobertMichon/fix-mp-taint-drop
🐛 Fix MachinePool node taint patching
2 parents f49fa9c + 3095518 commit 52fc099

File tree

3 files changed

+279
-24
lines changed

3 files changed

+279
-24
lines changed

‎exp/internal/controllers/machinepool_controller_noderef.go

Lines changed: 38 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ func (r *MachinePoolReconciler) reconcileNodeRefs(ctx context.Context, cluster *
5555
}
5656

5757
// Check that the Machine doesn't already have a NodeRefs.
58+
// Return early if there is no work to do.
5859
if mp.Status.Replicas == mp.Status.ReadyReplicas && len(mp.Status.NodeRefs) == int(mp.Status.ReadyReplicas) {
5960
conditions.MarkTrue(mp, expv1.ReplicasReadyCondition)
6061
return ctrl.Result{}, nil
@@ -94,30 +95,10 @@ func (r *MachinePoolReconciler) reconcileNodeRefs(ctx context.Context, cluster *
9495
log.Info("Set MachinePools's NodeRefs", "noderefs", mp.Status.NodeRefs)
9596
r.recorder.Event(mp, corev1.EventTypeNormal, "SuccessfulSetNodeRefs", fmt.Sprintf("%+v", mp.Status.NodeRefs))
9697

97-
// Reconcile node annotations.
98-
for _, nodeRef := range nodeRefsResult.references {
99-
node := &corev1.Node{}
100-
if err := clusterClient.Get(ctx, client.ObjectKey{Name: nodeRef.Name}, node); err != nil {
101-
log.V(2).Info("Failed to get Node, skipping setting annotations", "err", err, "nodeRef.Name", nodeRef.Name)
102-
continue
103-
}
104-
patchHelper, err := patch.NewHelper(node, clusterClient)
105-
if err != nil {
106-
return ctrl.Result{}, err
107-
}
108-
desired := map[string]string{
109-
clusterv1.ClusterNameAnnotation: mp.Spec.ClusterName,
110-
clusterv1.ClusterNamespaceAnnotation: mp.GetNamespace(),
111-
clusterv1.OwnerKindAnnotation: mp.Kind,
112-
clusterv1.OwnerNameAnnotation: mp.Name,
113-
}
114-
// Add annotations and drop NodeUninitializedTaint.
115-
if annotations.AddAnnotations(node, desired) || taints.RemoveNodeTaint(node, clusterv1.NodeUninitializedTaint) {
116-
if err := patchHelper.Patch(ctx, node); err != nil {
117-
log.V(2).Info("Failed patch node to set annotations and drop taints", "err", err, "node name", node.Name)
118-
return ctrl.Result{}, err
119-
}
120-
}
98+
// Reconcile node annotations and taints.
99+
err = r.patchNodes(ctx, clusterClient, nodeRefsResult.references, mp)
100+
if err != nil {
101+
return ctrl.Result{}, err
121102
}
122103

123104
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.
221202
return getNodeReferencesResult{nodeRefs, available, ready}, nil
222203
}
223204

205+
// patchNodes patches the nodes with the cluster name and cluster namespace annotations.
206+
func (r *MachinePoolReconciler) patchNodes(ctx context.Context, c client.Client, references []corev1.ObjectReference, mp *expv1.MachinePool) error {
207+
log := ctrl.LoggerFrom(ctx)
208+
for _, nodeRef := range references {
209+
node := &corev1.Node{}
210+
if err := c.Get(ctx, client.ObjectKey{Name: nodeRef.Name}, node); err != nil {
211+
log.V(2).Info("Failed to get Node, skipping setting annotations", "err", err, "nodeRef.Name", nodeRef.Name)
212+
continue
213+
}
214+
patchHelper, err := patch.NewHelper(node, c)
215+
if err != nil {
216+
return err
217+
}
218+
desired := map[string]string{
219+
clusterv1.ClusterNameAnnotation: mp.Spec.ClusterName,
220+
clusterv1.ClusterNamespaceAnnotation: mp.GetNamespace(),
221+
clusterv1.OwnerKindAnnotation: mp.Kind,
222+
clusterv1.OwnerNameAnnotation: mp.Name,
223+
}
224+
// Add annotations and drop NodeUninitializedTaint.
225+
hasAnnotationChanges := annotations.AddAnnotations(node, desired)
226+
hasTaintChanges := taints.RemoveNodeTaint(node, clusterv1.NodeUninitializedTaint)
227+
// Patch the node if needed.
228+
if hasAnnotationChanges || hasTaintChanges {
229+
if err := patchHelper.Patch(ctx, node); err != nil {
230+
log.V(2).Info("Failed patch node to set annotations and drop taints", "err", err, "node name", node.Name)
231+
return err
232+
}
233+
}
234+
}
235+
return nil
236+
}
237+
224238
func nodeIsReady(node *corev1.Node) bool {
225239
for _, n := range node.Status.Conditions {
226240
if n.Type == corev1.NodeReady {

‎exp/internal/controllers/machinepool_controller_noderef_test.go

Lines changed: 231 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ import (
2525
"k8s.io/client-go/tools/record"
2626
"sigs.k8s.io/controller-runtime/pkg/client"
2727
"sigs.k8s.io/controller-runtime/pkg/client/fake"
28+
29+
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
30+
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
2831
)
2932

3033
func TestMachinePoolGetNodeReference(t *testing.T) {
@@ -199,3 +202,231 @@ func TestMachinePoolGetNodeReference(t *testing.T) {
199202
})
200203
}
201204
}
205+
206+
func TestMachinePoolPatchNodes(t *testing.T) {
207+
r := &MachinePoolReconciler{
208+
Client: fake.NewClientBuilder().Build(),
209+
recorder: record.NewFakeRecorder(32),
210+
}
211+
212+
nodeList := []client.Object{
213+
&corev1.Node{
214+
ObjectMeta: metav1.ObjectMeta{
215+
Name: "node-1",
216+
},
217+
Spec: corev1.NodeSpec{
218+
ProviderID: "aws://us-east-1/id-node-1",
219+
Taints: []corev1.Taint{
220+
clusterv1.NodeUninitializedTaint,
221+
},
222+
},
223+
},
224+
&corev1.Node{
225+
ObjectMeta: metav1.ObjectMeta{
226+
Name: "node-2",
227+
Annotations: map[string]string{
228+
"foo": "bar",
229+
},
230+
},
231+
Spec: corev1.NodeSpec{
232+
ProviderID: "aws://us-west-2/id-node-2",
233+
Taints: []corev1.Taint{
234+
{
235+
Key: "some-other-taint",
236+
Value: "SomeEffect",
237+
},
238+
clusterv1.NodeUninitializedTaint,
239+
},
240+
},
241+
},
242+
&corev1.Node{
243+
ObjectMeta: metav1.ObjectMeta{
244+
Name: "node-3",
245+
Annotations: map[string]string{
246+
"cluster.x-k8s.io/cluster-name": "cluster-1",
247+
"cluster.x-k8s.io/cluster-namespace": "my-namespace",
248+
"cluster.x-k8s.io/owner-kind": "MachinePool",
249+
"cluster.x-k8s.io/owner-name": "machinepool-3",
250+
},
251+
},
252+
Spec: corev1.NodeSpec{
253+
ProviderID: "aws://us-west-2/id-node-3",
254+
Taints: []corev1.Taint{
255+
{
256+
Key: "some-other-taint",
257+
Value: "SomeEffect",
258+
},
259+
clusterv1.NodeUninitializedTaint,
260+
},
261+
},
262+
},
263+
&corev1.Node{
264+
ObjectMeta: metav1.ObjectMeta{
265+
Name: "node-4",
266+
Annotations: map[string]string{
267+
"cluster.x-k8s.io/cluster-name": "cluster-1",
268+
"cluster.x-k8s.io/cluster-namespace": "my-namespace",
269+
},
270+
},
271+
Spec: corev1.NodeSpec{
272+
ProviderID: "aws://us-west-2/id-node-4",
273+
Taints: []corev1.Taint{
274+
{
275+
Key: "some-other-taint",
276+
Value: "SomeEffect",
277+
},
278+
},
279+
},
280+
},
281+
}
282+
283+
testCases := []struct {
284+
name string
285+
machinePool *expv1.MachinePool
286+
nodeRefs []corev1.ObjectReference
287+
expectedNodes []corev1.Node
288+
err error
289+
}{
290+
{
291+
name: "Node with uninitialized taint should be patched",
292+
machinePool: &expv1.MachinePool{
293+
TypeMeta: metav1.TypeMeta{
294+
Kind: "MachinePool",
295+
},
296+
ObjectMeta: metav1.ObjectMeta{
297+
Name: "machinepool-1",
298+
Namespace: "my-namespace",
299+
},
300+
Spec: expv1.MachinePoolSpec{
301+
ClusterName: "cluster-1",
302+
ProviderIDList: []string{"aws://us-east-1/id-node-1"},
303+
},
304+
},
305+
nodeRefs: []corev1.ObjectReference{
306+
{Name: "node-1"},
307+
},
308+
expectedNodes: []corev1.Node{
309+
{
310+
ObjectMeta: metav1.ObjectMeta{
311+
Name: "node-1",
312+
Annotations: map[string]string{
313+
"cluster.x-k8s.io/cluster-name": "cluster-1",
314+
"cluster.x-k8s.io/cluster-namespace": "my-namespace",
315+
"cluster.x-k8s.io/owner-kind": "MachinePool",
316+
"cluster.x-k8s.io/owner-name": "machinepool-1",
317+
},
318+
},
319+
Spec: corev1.NodeSpec{
320+
Taints: nil,
321+
},
322+
},
323+
},
324+
},
325+
{
326+
name: "Node with existing annotations and taints should be patched",
327+
machinePool: &expv1.MachinePool{
328+
TypeMeta: metav1.TypeMeta{
329+
Kind: "MachinePool",
330+
},
331+
ObjectMeta: metav1.ObjectMeta{
332+
Name: "machinepool-2",
333+
Namespace: "my-namespace",
334+
},
335+
Spec: expv1.MachinePoolSpec{
336+
ClusterName: "cluster-1",
337+
ProviderIDList: []string{"aws://us-west-2/id-node-2"},
338+
},
339+
},
340+
nodeRefs: []corev1.ObjectReference{
341+
{Name: "node-2"},
342+
{Name: "node-3"},
343+
{Name: "node-4"},
344+
},
345+
expectedNodes: []corev1.Node{
346+
{
347+
ObjectMeta: metav1.ObjectMeta{
348+
Name: "node-2",
349+
Annotations: map[string]string{
350+
"cluster.x-k8s.io/cluster-name": "cluster-1",
351+
"cluster.x-k8s.io/cluster-namespace": "my-namespace",
352+
"cluster.x-k8s.io/owner-kind": "MachinePool",
353+
"cluster.x-k8s.io/owner-name": "machinepool-2",
354+
"foo": "bar",
355+
},
356+
},
357+
Spec: corev1.NodeSpec{
358+
Taints: []corev1.Taint{
359+
{
360+
Key: "some-other-taint",
361+
Value: "SomeEffect",
362+
},
363+
},
364+
},
365+
},
366+
{
367+
ObjectMeta: metav1.ObjectMeta{
368+
Name: "node-3",
369+
Annotations: map[string]string{
370+
"cluster.x-k8s.io/cluster-name": "cluster-1",
371+
"cluster.x-k8s.io/cluster-namespace": "my-namespace",
372+
"cluster.x-k8s.io/owner-kind": "MachinePool",
373+
"cluster.x-k8s.io/owner-name": "machinepool-2",
374+
},
375+
},
376+
Spec: corev1.NodeSpec{
377+
Taints: []corev1.Taint{
378+
{
379+
Key: "some-other-taint",
380+
Value: "SomeEffect",
381+
},
382+
},
383+
},
384+
},
385+
{
386+
ObjectMeta: metav1.ObjectMeta{
387+
Name: "node-4",
388+
Annotations: map[string]string{
389+
"cluster.x-k8s.io/cluster-name": "cluster-1",
390+
"cluster.x-k8s.io/cluster-namespace": "my-namespace",
391+
"cluster.x-k8s.io/owner-kind": "MachinePool",
392+
"cluster.x-k8s.io/owner-name": "machinepool-2",
393+
},
394+
},
395+
Spec: corev1.NodeSpec{
396+
Taints: []corev1.Taint{
397+
{
398+
Key: "some-other-taint",
399+
Value: "SomeEffect",
400+
},
401+
},
402+
},
403+
},
404+
},
405+
},
406+
}
407+
408+
for _, test := range testCases {
409+
t.Run(test.name, func(t *testing.T) {
410+
g := NewWithT(t)
411+
412+
fakeClient := fake.NewClientBuilder().WithObjects(nodeList...).Build()
413+
414+
err := r.patchNodes(ctx, fakeClient, test.nodeRefs, test.machinePool)
415+
if test.err == nil {
416+
g.Expect(err).To(BeNil())
417+
} else {
418+
g.Expect(err).NotTo(BeNil())
419+
g.Expect(err).To(Equal(test.err), "Expected error %v, got %v", test.err, err)
420+
}
421+
422+
// Check that the nodes have the desired taints and annotations
423+
for _, expected := range test.expectedNodes {
424+
node := &corev1.Node{}
425+
err := fakeClient.Get(ctx, client.ObjectKey{Name: expected.Name}, node)
426+
g.Expect(err).To(BeNil())
427+
g.Expect(node.Annotations).To(Equal(expected.Annotations))
428+
g.Expect(node.Spec.Taints).To(Equal(expected.Spec.Taints))
429+
}
430+
})
431+
}
432+
}

‎internal/util/taints/taints_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,16 @@ func TestRemoveNodeTaint(t *testing.T) {
5555
wantTaints: []corev1.Taint{taint2},
5656
wantModified: false,
5757
},
58+
{
59+
name: "drop last taint should return true",
60+
node: &corev1.Node{Spec: corev1.NodeSpec{
61+
Taints: []corev1.Taint{
62+
taint1,
63+
}}},
64+
dropTaint: taint1,
65+
wantTaints: []corev1.Taint{},
66+
wantModified: true,
67+
},
5868
}
5969

6070
for _, tt := range tests {

0 commit comments

Comments
 (0)