mhc: Don't set OwnerRemediated or ExternallyRemediated on deleting machines

Author: chrischdi

internal/controllers/machinehealthcheck/machinehealthcheck_controller.go

@@ -445,12 +445,15 @@ func (r *Reconciler) patchUnhealthyTargets(ctx context.Context, logger logr.Logg
 				if err != nil {
 					conditions.MarkFalse(m, clusterv1.ExternalRemediationTemplateAvailableCondition, clusterv1.ExternalRemediationTemplateNotFoundReason, clusterv1.ConditionSeverityError, err.Error())
 
-					v1beta2conditions.Set(t.Machine, metav1.Condition{
-						Type:    clusterv1.MachineExternallyRemediatedV1Beta2Condition,
-						Status:  metav1.ConditionFalse,
-						Reason:  clusterv1.MachineExternallyRemediatedRemediationTemplateNotFoundV1Beta2Reason,
-						Message: fmt.Sprintf("Error retrieving remediation template %s %s", m.Spec.RemediationTemplate.Kind, klog.KRef(m.Spec.RemediationTemplate.Namespace, m.Spec.RemediationTemplate.Name)),
-					})
+					// Only set condition if not already deleting.
+					if t.Machine.DeletionTimestamp.IsZero() {
+						v1beta2conditions.Set(t.Machine, metav1.Condition{
+							Type:    clusterv1.MachineExternallyRemediatedV1Beta2Condition,
+							Status:  metav1.ConditionFalse,
+							Reason:  clusterv1.MachineExternallyRemediatedRemediationTemplateNotFoundV1Beta2Reason,
+							Message: fmt.Sprintf("Error retrieving remediation template %s %s", m.Spec.RemediationTemplate.Kind, klog.KRef(m.Spec.RemediationTemplate.Namespace, m.Spec.RemediationTemplate.Name)),
+						})
+					}
 					errList = append(errList, errors.Wrapf(err, "error retrieving remediation template %v %q for machine %q in namespace %q within cluster %q", m.Spec.RemediationTemplate.GroupVersionKind(), m.Spec.RemediationTemplate.Name, t.Machine.Name, t.Machine.Namespace, m.Spec.ClusterName))
 					return errList
 				}
@@ -481,21 +484,27 @@ func (r *Reconciler) patchUnhealthyTargets(ctx context.Context, logger logr.Logg
 				if err := r.Client.Create(ctx, to); err != nil {
 					conditions.MarkFalse(m, clusterv1.ExternalRemediationRequestAvailableCondition, clusterv1.ExternalRemediationRequestCreationFailedReason, clusterv1.ConditionSeverityError, err.Error())
 
-					v1beta2conditions.Set(t.Machine, metav1.Condition{
-						Type:    clusterv1.MachineExternallyRemediatedV1Beta2Condition,
-						Status:  metav1.ConditionFalse,
-						Reason:  clusterv1.MachineExternallyRemediatedRemediationRequestCreationFailedV1Beta2Reason,
-						Message: "Please check controller logs for errors",
-					})
+					// Only set condition if not already deleting.
+					if t.Machine.DeletionTimestamp.IsZero() {
+						v1beta2conditions.Set(t.Machine, metav1.Condition{
+							Type:    clusterv1.MachineExternallyRemediatedV1Beta2Condition,
+							Status:  metav1.ConditionFalse,
+							Reason:  clusterv1.MachineExternallyRemediatedRemediationRequestCreationFailedV1Beta2Reason,
+							Message: "Please check controller logs for errors",
+						})
+					}
 					errList = append(errList, errors.Wrapf(err, "error creating remediation request for machine %q in namespace %q within cluster %q", t.Machine.Name, t.Machine.Namespace, t.Machine.Spec.ClusterName))
 					return errList
 				}
 
-				v1beta2conditions.Set(t.Machine, metav1.Condition{
-					Type:   clusterv1.MachineExternallyRemediatedV1Beta2Condition,
-					Status: metav1.ConditionFalse,
-					Reason: clusterv1.MachineExternallyRemediatedWaitingForRemediationV1Beta2Reason,
-				})
+				// Only set condition if not already deleting.
+				if t.Machine.DeletionTimestamp.IsZero() {
+					v1beta2conditions.Set(t.Machine, metav1.Condition{
+						Type:   clusterv1.MachineExternallyRemediatedV1Beta2Condition,
+						Status: metav1.ConditionFalse,
+						Reason: clusterv1.MachineExternallyRemediatedWaitingForRemediationV1Beta2Reason,
+					})
+				}
 			} else {
 				logger.Info("Target has failed health check, marking for remediation", "target", t.string(), "reason", condition.Reason, "message", condition.Message)
 				// NOTE: MHC is responsible for creating MachineOwnerRemediatedCondition if missing or to trigger another remediation if the previous one is completed;
@@ -504,7 +513,8 @@ func (r *Reconciler) patchUnhealthyTargets(ctx context.Context, logger logr.Logg
 					conditions.MarkFalse(t.Machine, clusterv1.MachineOwnerRemediatedCondition, clusterv1.WaitingForRemediationReason, clusterv1.ConditionSeverityWarning, "")
 				}
 
-				if ownerRemediatedCondition := v1beta2conditions.Get(t.Machine, clusterv1.MachineOwnerRemediatedV1Beta2Condition); ownerRemediatedCondition == nil || ownerRemediatedCondition.Status == metav1.ConditionTrue {
+				// If the machine is already in deletion, the OwnerRemediated condition should not be set.
+				if ownerRemediatedCondition := v1beta2conditions.Get(t.Machine, clusterv1.MachineOwnerRemediatedV1Beta2Condition); t.Machine.DeletionTimestamp.IsZero() && (ownerRemediatedCondition == nil || ownerRemediatedCondition.Status == metav1.ConditionTrue) {
 					v1beta2conditions.Set(t.Machine, metav1.Condition{
 						Type:    clusterv1.MachineOwnerRemediatedV1Beta2Condition,
 						Status:  metav1.ConditionFalse,

internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go

@@ -376,7 +376,7 @@ func TestMachineHealthCheck_Reconcile(t *testing.T) {
 		assertMachinesNotHealthy(g, mhc, 0)
 	})
 
-	t.Run("it marks unhealthy machines for remediation when there is one unhealthy Machine", func(t *testing.T) {
+	t.Run("it marks unhealthy machines for remediation when there is one unhealthy Machine and skips deleting machines", func(t *testing.T) {
 		g := NewWithT(t)
 		cluster := createCluster(g, ns.Name)
 
@@ -404,7 +404,16 @@ func TestMachineHealthCheck_Reconcile(t *testing.T) {
 			machineLabels(mhc.Spec.Selector.MatchLabels),
 		)
 		defer cleanup2()
-		machines = append(machines, unhealthyMachines...)
+		// Unhealthy nodes and machines but already in deletion.
+		_, unhealthyMachinesDeleting, cleanup3 := createMachinesWithNodes(g, cluster,
+			count(1),
+			createNodeRefForMachine(true),
+			nodeStatus(corev1.ConditionUnknown),
+			machineLabels(mhc.Spec.Selector.MatchLabels),
+			machineDeleting(),
+		)
+		defer cleanup3()
+		machines = append(append(machines, unhealthyMachines...), unhealthyMachinesDeleting...)
 		targetMachines := make([]string, len(machines))
 		for i, m := range machines {
 			targetMachines[i] = m.Name
@@ -419,7 +428,7 @@ func TestMachineHealthCheck_Reconcile(t *testing.T) {
 			}
 			return &mhc.Status
 		}).Should(MatchMachineHealthCheckStatus(&clusterv1.MachineHealthCheckStatus{
-			ExpectedMachines:    3,
+			ExpectedMachines:    4,
 			CurrentHealthy:      2,
 			RemediationsAllowed: 2,
 			ObservedGeneration:  1,
@@ -441,7 +450,7 @@ func TestMachineHealthCheck_Reconcile(t *testing.T) {
 			},
 		}))
 
-		assertMachinesNotHealthy(g, mhc, 1)
+		assertMachinesNotHealthy(g, mhc, 2)
 		assertMachinesOwnerRemediated(g, mhc, 1)
 	})
 
@@ -2450,6 +2459,8 @@ type machinesWithNodes struct {
 	labels                     map[string]string
 	failureReason              string
 	failureMessage             string
+	finalizers                 []string
+	deleted                    bool
 }
 
 type machineWithNodesOption func(m *machinesWithNodes)
@@ -2496,6 +2507,13 @@ func machineFailureMessage(s string) machineWithNodesOption {
 	}
 }
 
+func machineDeleting() machineWithNodesOption {
+	return func(m *machinesWithNodes) {
+		m.finalizers = append(m.finalizers, "test.cluster.io/deleting")
+		m.deleted = true
+	}
+}
+
 func createMachinesWithNodes(
 	g *WithT,
 	c *clusterv1.Cluster,
@@ -2535,6 +2553,9 @@ func createMachinesWithNodes(
 			Name:       infraMachine.GetName(),
 			Namespace:  infraMachine.GetNamespace(),
 		}
+		if len(o.finalizers) > 0 {
+			machine.Finalizers = o.finalizers
+		}
 		g.Expect(env.Create(ctx, machine)).To(Succeed())
 		fmt.Printf("machine created: %s\n", machine.GetName())
 
@@ -2598,6 +2619,12 @@ func createMachinesWithNodes(
 			machine.Status.FailureMessage = ptr.To(o.failureMessage)
 		}
 
+		// Set deletiontimestamp before updating status to ensure its not reconciled
+		// without having the deletionTimestamp set.
+		if o.deleted {
+			g.Expect(env.Delete(ctx, machine)).To(Succeed())
+		}
+
 		// Adding one second to ensure there is a difference from the
 		// original time so that the patch works. That is, ensure the
 		// precision isn't lost during conversions.
@@ -2618,7 +2645,16 @@ func createMachinesWithNodes(
 			}
 		}
 		for _, m := range machines {
-			g.Expect(env.Delete(ctx, m)).To(Succeed())
+			if m.DeletionTimestamp.IsZero() {
+				g.Expect(env.Delete(ctx, m)).To(Succeed())
+			}
+			if len(m.Finalizers) > 1 {
+				g.Expect(env.Get(ctx, util.ObjectKey(m), m)).To(Succeed())
+				machinePatchHelper, err := patch.NewHelper(m, env.Client)
+				g.Expect(err).ToNot(HaveOccurred())
+				m.Finalizers = nil
+				g.Expect(machinePatchHelper.Patch(ctx, m)).To(Succeed())
+			}
 		}
 		for _, im := range infraMachines {
 			if err := env.Delete(ctx, im); !apierrors.IsNotFound(err) {