Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Fix MachinePool node taint patching #8462

Merged
merged 1 commit into from
Apr 4, 2023
+279 −24
Merged

🐛 Fix MachinePool node taint patching #8462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 38 additions & 24 deletions exp/internal/controllers/machinepool_controller_noderef.go
View file
Open in desktop
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)
CecileRobertMichon marked this conversation as resolved.
Show resolved Hide resolved
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.
Copy link
Contributor Author

@CecileRobertMichon CecileRobertMichon Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the real change here (line 225-228). Everything else is just extracting the code into a function so it can be unit tested.

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
231 changes: 231 additions & 0 deletions exp/internal/controllers/machinepool_controller_noderef_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ import (
"k8s.io/client-go/tools/record"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
)

func TestMachinePoolGetNodeReference(t *testing.T) {
Expand Down Expand Up @@ -199,3 +202,231 @@ 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,
},
},
},
&corev1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node-3",
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-3",
},
},
Spec: corev1.NodeSpec{
ProviderID: "aws://us-west-2/id-node-3",
Taints: []corev1.Taint{
{
Key: "some-other-taint",
Value: "SomeEffect",
},
clusterv1.NodeUninitializedTaint,
},
},
},
&corev1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node-4",
Annotations: map[string]string{
"cluster.x-k8s.io/cluster-name": "cluster-1",
"cluster.x-k8s.io/cluster-namespace": "my-namespace",
},
},
Spec: corev1.NodeSpec{
ProviderID: "aws://us-west-2/id-node-4",
Taints: []corev1.Taint{
{
Key: "some-other-taint",
Value: "SomeEffect",
},
},
},
},
}

testCases := []struct {
CecileRobertMichon marked this conversation as resolved.
Show resolved Hide resolved
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"},
{Name: "node-3"},
{Name: "node-4"},
},
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",
},
},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "node-3",
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",
},
},
Spec: corev1.NodeSpec{
Taints: []corev1.Taint{
{
Key: "some-other-taint",
Value: "SomeEffect",
},
},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "node-4",
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",
},
},
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
Open in desktop
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",
CecileRobertMichon marked this conversation as resolved.
Show resolved Hide resolved
node: &corev1.Node{Spec: corev1.NodeSpec{
Taints: []corev1.Taint{
taint1,
}}},
dropTaint: taint1,
wantTaints: []corev1.Taint{},
wantModified: true,
},
}

for _, tt := range tests {
Expand Down