review fixes

Author: chrischdi

internal/controllers/machine/machine_controller_noderef.go

@@ -302,20 +302,19 @@ func (r *Reconciler) patchNode(ctx context.Context, remoteClient client.Client,
 		return errors.Wrapf(err, "failed to check if Node %s is outdated", klog.KRef("", node.Name))
 	}
 
-	// It is not possible to identify if we have to set the NodeOutdatedRevisionTaint if there is
-	// no OwnerReference or the owning MachineSet or MachineDeployment was not found.
-	// Note: This could happen e.g. during background deletion of objects.
-	if notFound {
-		return nil
-	}
-	if isOutdated {
-		hasTaintChanges = taints.EnsureNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
-	} else {
-		hasTaintChanges = taints.RemoveNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
-	}
+	// It is only possible to identify if we have to set or remove the NodeOutdatedRevisionTaint if shouldNodeHaveOutdatedTaint
+	// found all relevant objects.
+	// Example: when the MachineDeployment or Machineset can't be found due to a background deletion of objects.
+	if !notFound {
+		if isOutdated {
+			hasTaintChanges = taints.EnsureNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
+		} else {
+			hasTaintChanges = taints.RemoveNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
+		}
 
-	if !hasAnnotationChanges && !hasLabelChanges && !hasTaintChanges {
-		return nil
+		if !hasAnnotationChanges && !hasLabelChanges && !hasTaintChanges {
+			return nil
+		}
 	}
 
 	return remoteClient.Patch(ctx, newNode, client.StrategicMergeFrom(node))

internal/controllers/machine/machine_controller_noderef_test.go

@@ -773,6 +773,53 @@ func TestPatchNode(t *testing.T) {
 			ms:      newFakeMachineSet(metav1.NamespaceDefault, clusterName),
 			md:      newFakeMachineDeployment(metav1.NamespaceDefault, clusterName),
 		},
+		{
+			name: "Ensure Labels and Annotations still get patched if MachineSet cannot be found",
+			oldNode: &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: fmt.Sprintf("node-%s", util.RandomString(6)),
+				},
+			},
+			expectedAnnotations: map[string]string{
+				clusterv1.LabelsFromMachineAnnotation: "",
+			},
+			expectedTaints: []corev1.Taint{
+				{Key: "node.kubernetes.io/not-ready", Effect: "NoSchedule"}, // Added by the API server
+			},
+			machine: &clusterv1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fmt.Sprintf("ma-%s", util.RandomString(6)),
+					Namespace: metav1.NamespaceDefault,
+					Labels: map[string]string{
+						clusterv1.MachineSetNameLabel:        "test-ms-missing",
+						clusterv1.MachineDeploymentNameLabel: "test-md",
+					},
+					OwnerReferences: []metav1.OwnerReference{{
+						Kind:       "MachineSet",
+						Name:       "test-ms-missing",
+						APIVersion: clusterv1.GroupVersion.String(),
+						UID:        "uid",
+					}},
+				},
+				Spec: newFakeMachineSpec(metav1.NamespaceDefault, clusterName),
+			},
+			ms: nil,
+			md: &clusterv1.MachineDeployment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-md",
+					Namespace: metav1.NamespaceDefault,
+					Annotations: map[string]string{
+						clusterv1.RevisionAnnotation: "1",
+					},
+				},
+				Spec: clusterv1.MachineDeploymentSpec{
+					ClusterName: clusterName,
+					Template: clusterv1.MachineTemplateSpec{
+						Spec: newFakeMachineSpec(metav1.NamespaceDefault, clusterName),
+					},
+				},
+			},
+		},
 		{
 			name: "Ensure NodeOutdatedRevisionTaint to be set if a node is associated to an outdated machineset",
 			oldNode: &corev1.Node{
@@ -916,7 +963,9 @@ func TestPatchNode(t *testing.T) {
 
 			g.Expect(env.CreateAndWait(ctx, oldNode)).To(Succeed())
 			g.Expect(env.CreateAndWait(ctx, machine)).To(Succeed())
-			g.Expect(env.CreateAndWait(ctx, ms)).To(Succeed())
+			if ms != nil {
+				g.Expect(env.CreateAndWait(ctx, ms)).To(Succeed())
+			}
 			g.Expect(env.CreateAndWait(ctx, md)).To(Succeed())
 			t.Cleanup(func() {
 				_ = env.CleanupAndWait(ctx, oldNode, machine, ms, md)