Skip to content

Commit

Permalink
🐛 Fix MachinePool node taint patching
Browse files Browse the repository at this point in the history
  • Loading branch information
@CecileRobertMichon
CecileRobertMichon committed Apr 3, 2023
1 parent 01f619f commit 875d36a
Show file tree
Hide file tree
Showing 3 changed files with 199 additions and 24 deletions.
62 changes: 38 additions & 24 deletions exp/internal/controllers/machinepool_controller_noderef.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down
151 changes: 151 additions & 0 deletions exp/internal/controllers/machinepool_controller_noderef_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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))
}
})
}
}
10 changes: 10 additions & 0 deletions internal/util/taints/taints_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 875d36a

Please sign in to comment.