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

[release-1.4] 🐛 Fix MachinePool node taint patching #8468

Merged
merged 1 commit into from
Apr 4, 2023
Merged

[release-1.4] 🐛 Fix MachinePool node taint patching #8468

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)
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
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 {
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",
node: &corev1.Node{Spec: corev1.NodeSpec{
Taints: []corev1.Taint{
taint1,
}}},
dropTaint: taint1,
wantTaints: []corev1.Taint{},
wantModified: true,
},
}

for _, tt := range tests {
Expand Down