Address comments

api/v1beta1/machine_types.go

@@ -207,63 +207,64 @@ const (
 )
 
 // Machine's HealthCheckSucceeded condition and corresponding reasons that will be used in v1Beta2 API version.
-// Note: HealthCheckSucceeded condition ia set by the MachineHealthCheck controller.
+// Note: HealthCheckSucceeded condition is set by the MachineHealthCheck controller.
 const (
 	// MachineHealthCheckSucceededV1Beta2Condition is true if MHC instances targeting this machine report the Machine
 	// is healthy according to the definition of healthy present in the spec of the MachineHealthCheck object.
 	MachineHealthCheckSucceededV1Beta2Condition = "HealthCheckSucceeded"
 
-	// MachineHealthCheckPassedV1Beta2Reason surfaces when a machine passes all the health checks defined by a MachineHealthCheck object.
-	MachineHealthCheckPassedV1Beta2Reason = "HealthCheckPassed"
+	// MachineHealthCheckSucceededV1Beta2Reason surfaces when a machine passes all the health checks defined by a MachineHealthCheck object.
+	MachineHealthCheckSucceededV1Beta2Reason = "HealthCheckSucceeded"
 
-	// MachineHealthCheckUnhealthyNodeV1Beta2Reason surfaces when the node hosted on the machine do not pass the health checks
+	// MachineHealthCheckUnhealthyNodeV1Beta2Reason surfaces when the node hosted on the machine does not pass the health checks
 	// defined by a MachineHealthCheck object.
 	MachineHealthCheckUnhealthyNodeV1Beta2Reason = "UnhealthyNode"
 
 	// MachineHealthCheckNodeStartupTimeoutV1Beta2Reason surfaces when the node hosted on the machine does not appear within
-	// he timeout defined by a MachineHealthCheck object.
+	// the timeout defined by a MachineHealthCheck object.
 	MachineHealthCheckNodeStartupTimeoutV1Beta2Reason = "NodeStartupTimeout"
 
 	// MachineHealthCheckNodeDeletedV1Beta2Reason surfaces when a MachineHealthCheck detect that the node hosted on the
 	// machine has been deleted while the Machine is still running.
 	MachineHealthCheckNodeDeletedV1Beta2Reason = "NodeDeleted"
 
-	// MachineHealthCheckManuallyRemediatedV1Beta2Reason surfaces a MachineHealthCheck detects a machine manually remediated
+	// MachineHealthCheckHasRemediateMachineAnnotationV1Beta2Reason surfaces a MachineHealthCheck detects a machine manually remediated
 	// via the remediate-machine annotation.
-	MachineHealthCheckManuallyRemediatedV1Beta2Reason = "ManuallyRemediated"
+	MachineHealthCheckHasRemediateMachineAnnotationV1Beta2Reason = "HasRemediateAnnotation"
 )
 
 // Machine's OwnerRemediated conditions and corresponding reasons that will be used in v1Beta2 API version.
-// Note: OwnerRemediated condition is created by the MachineHealthCheck controller, and then Set by the Machine's owner controller.
+// Note: OwnerRemediated condition is initially set by the MachineHealthCheck controller; then it is up to the Machine's
+// owner controller to update or delete this condition.
 const (
 	// MachineOwnerRemediatedV1Beta2Condition is only present if MHC instances targeting this machine
 	// determine that the controller owning this machine should perform remediation.
 	MachineOwnerRemediatedV1Beta2Condition = "OwnerRemediated"
 
-	// MachineOwnerRemediatedWaitingForOwnerV1Beta2Reason surfaces the machine is waiting for the owner controller
+	// MachineOwnerRemediatedWaitingForOwnerRemediationV1Beta2Reason surfaces the machine is waiting for the owner controller
 	// to start remediation.
-	MachineOwnerRemediatedWaitingForOwnerV1Beta2Reason = "WaitingForOwner"
+	MachineOwnerRemediatedWaitingForOwnerRemediationV1Beta2Reason = "WaitingForRemediation"
 )
 
 // Machine's ExternallyRemediated conditions and corresponding reasons that will be used in v1Beta2 API version.
-// Note: ExternallyRemediated condition is created by the MachineHealthCheck controller, and then Set by the external
-// remediation controller.
+// Note: ExternallyRemediated condition is initially set by the MachineHealthCheck controller; then it is up to the external
+// remediation controller to update or delete this condition.
 const (
 	// MachineExternallyRemediatedV1Beta2Condition is only present if MHC instances targeting this machine
 	// determine that an external controller should perform remediation.
 	MachineExternallyRemediatedV1Beta2Condition = "ExternallyRemediated"
 
 	// MachineExternallyRemediatedWaitingForExternalRemediationV1Beta2Reason surfaces the machine is waiting for the
 	// external remediation controller to start remediation.
-	MachineExternallyRemediatedWaitingForExternalRemediationV1Beta2Reason = "WaitingForExternalRemediation"
+	MachineExternallyRemediatedWaitingForExternalRemediationV1Beta2Reason = "WaitingForRemediation"
 
-	// MachineOwnerRemediateExternalRemediationTemplateNotFoundV1Beta2Reason surfaces that the MachineHealthCheck cannot
+	// MachineExternallyRemediatedTemplateNotFoundV1Beta2Reason surfaces that the MachineHealthCheck cannot
 	// find the template for a external remediation request.
-	MachineOwnerRemediateExternalRemediationTemplateNotFoundV1Beta2Reason = "ExternalRemediationTemplateNotFound"
+	MachineExternallyRemediatedTemplateNotFoundV1Beta2Reason = "RemediationTemplateNotFound"
 
-	// MachineOwnerRemediateExternalRemediationRequestCreationFailedV1Beta2Reason surfaces that the MachineHealthCheck cannot
+	// MachineExternallyRemediatedRequestCreationFailedV1Beta2Reason surfaces that the MachineHealthCheck cannot
 	// create a request for the external remediation controller.
-	MachineOwnerRemediateExternalRemediationRequestCreationFailedV1Beta2Reason = "ExternalRemediationRequestCreationFailed"
+	MachineExternallyRemediatedRequestCreationFailedV1Beta2Reason = "RemediationRequestCreationFailed"
 )
 
 // Machine's Deleting condition and corresponding reasons that will be used in v1Beta2 API version.

internal/controllers/machinehealthcheck/machinehealthcheck_controller.go

@@ -417,7 +417,7 @@ func (r *Reconciler) patchUnhealthyTargets(ctx context.Context, logger logr.Logg
 					v1beta2conditions.Set(t.Machine, metav1.Condition{
 						Type:    clusterv1.MachineExternallyRemediatedV1Beta2Condition,
 						Status:  metav1.ConditionFalse,
-						Reason:  clusterv1.MachineOwnerRemediateExternalRemediationTemplateNotFoundV1Beta2Reason,
+						Reason:  clusterv1.MachineExternallyRemediatedTemplateNotFoundV1Beta2Reason,
 						Message: fmt.Sprintf("error retrieving remediation template %s %s", m.Spec.RemediationTemplate.Kind, klog.KRef(t.Machine.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))
@@ -451,9 +451,10 @@ func (r *Reconciler) patchUnhealthyTargets(ctx context.Context, logger logr.Logg
 					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.MachineOwnerRemediateExternalRemediationRequestCreationFailedV1Beta2Reason,
+						Type:    clusterv1.MachineExternallyRemediatedV1Beta2Condition,
+						Status:  metav1.ConditionFalse,
+						Reason:  clusterv1.MachineExternallyRemediatedRequestCreationFailedV1Beta2Reason,
+						Message: "take a look at the controller logs",
 					})
 					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
@@ -476,7 +477,7 @@ func (r *Reconciler) patchUnhealthyTargets(ctx context.Context, logger logr.Logg
 					v1beta2conditions.Set(t.Machine, metav1.Condition{
 						Type:   clusterv1.MachineOwnerRemediatedV1Beta2Condition,
 						Status: metav1.ConditionFalse,
-						Reason: clusterv1.MachineOwnerRemediatedWaitingForOwnerV1Beta2Reason,
+						Reason: clusterv1.MachineOwnerRemediatedWaitingForOwnerRemediationV1Beta2Reason,
 					})
 				}
 			}

internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go

@@ -2689,7 +2689,7 @@ func TestPatchTargets(t *testing.T) {
 	v1beta2conditions.Set(machine1, metav1.Condition{
 		Type:   clusterv1.MachineHealthCheckSucceededV1Beta2Condition,
 		Status: metav1.ConditionTrue,
-		Reason: clusterv1.MachineHealthCheckPassedV1Beta2Reason,
+		Reason: clusterv1.MachineHealthCheckSucceededV1Beta2Reason,
 	})
 
 	machine2 := machine1.DeepCopy()

internal/controllers/machinehealthcheck/machinehealthcheck_targets.go

@@ -102,8 +102,8 @@ func (t *healthCheckTarget) needsRemediation(logger logr.Logger, timeoutForMachi
 		v1beta2conditions.Set(t.Machine, metav1.Condition{
 			Type:    clusterv1.MachineHealthCheckSucceededV1Beta2Condition,
 			Status:  metav1.ConditionFalse,
-			Reason:  clusterv1.MachineHealthCheckManuallyRemediatedV1Beta2Reason,
-			Message: "Marked for remediation via remediate-machine annotation",
+			Reason:  clusterv1.MachineHealthCheckHasRemediateMachineAnnotationV1Beta2Reason,
+			Message: "Marked for remediation via cluster.x-k8s.io/remediate-machine annotation",
 		})
 		return true, time.Duration(0)
 	}
@@ -221,7 +221,7 @@ func (t *healthCheckTarget) needsRemediation(logger logr.Logger, timeoutForMachi
 				Type:    clusterv1.MachineHealthCheckSucceededV1Beta2Condition,
 				Status:  metav1.ConditionFalse,
 				Reason:  clusterv1.MachineHealthCheckUnhealthyNodeV1Beta2Reason,
-				Message: fmt.Sprintf("Condition %s on node is reporting status %s for more than %s", c.Type, c.Status, c.Timeout.Duration.String()),
+				Message: fmt.Sprintf("Condition %s on Node is reporting status %s for more than %s", c.Type, c.Status, c.Timeout.Duration.String()),
 			})
 			return true, time.Duration(0)
 		}
@@ -360,7 +360,7 @@ func (r *Reconciler) healthCheckTargets(targets []healthCheckTarget, logger logr
 			v1beta2conditions.Set(t.Machine, metav1.Condition{
 				Type:   clusterv1.MachineHealthCheckSucceededV1Beta2Condition,
 				Status: metav1.ConditionTrue,
-				Reason: clusterv1.MachineHealthCheckPassedV1Beta2Reason,
+				Reason: clusterv1.MachineHealthCheckSucceededV1Beta2Reason,
 			})
 			healthy = append(healthy, t)
 		}

internal/controllers/machinehealthcheck/machinehealthcheck_targets_test.go

@@ -372,7 +372,7 @@ func TestHealthCheckTargets(t *testing.T) {
 		nodeMissing: false,
 	}
 	nodeUnknown400Condition := newFailedHealthCheckCondition(clusterv1.UnhealthyNodeConditionReason, "Condition Ready on node is reporting status Unknown for more than %s", timeoutForUnhealthyConditions)
-	nodeUnknown400V1Beta2Condition := newFailedHealthCheckV1Beta2Condition(clusterv1.MachineHealthCheckUnhealthyNodeV1Beta2Reason, "Condition Ready on node is reporting status Unknown for more than %s", timeoutForUnhealthyConditions)
+	nodeUnknown400V1Beta2Condition := newFailedHealthCheckV1Beta2Condition(clusterv1.MachineHealthCheckUnhealthyNodeV1Beta2Reason, "Condition Ready on Node is reporting status Unknown for more than %s", timeoutForUnhealthyConditions)
 
 	// Target for when a node is healthy
 	testNodeHealthy := newTestNode("node1")
@@ -411,6 +411,7 @@ func TestHealthCheckTargets(t *testing.T) {
 
 	// Target for when the machine has the remediate machine annotation
 	const annotationRemediationMsg = "Marked for remediation via remediate-machine annotation"
+	const annotationRemediationV1Beta2Msg = "Marked for remediation via cluster.x-k8s.io/remediate-machine annotation"
 	testMachineAnnotationRemediation := testMachine.DeepCopy()
 	testMachineAnnotationRemediation.Annotations = map[string]string{clusterv1.RemediateMachineAnnotation: ""}
 	machineAnnotationRemediation := healthCheckTarget{
@@ -420,7 +421,7 @@ func TestHealthCheckTargets(t *testing.T) {
 		Node:    nil,
 	}
 	machineAnnotationRemediationCondition := newFailedHealthCheckCondition(clusterv1.HasRemediateMachineAnnotationReason, annotationRemediationMsg)
-	machineAnnotationRemediationV1Beta2Condition := newFailedHealthCheckV1Beta2Condition(clusterv1.MachineHealthCheckManuallyRemediatedV1Beta2Reason, annotationRemediationMsg)
+	machineAnnotationRemediationV1Beta2Condition := newFailedHealthCheckV1Beta2Condition(clusterv1.MachineHealthCheckHasRemediateMachineAnnotationV1Beta2Reason, annotationRemediationV1Beta2Msg)
 
 	testCases := []struct {
 		desc                                     string

util/conditions/v1beta2/getter_test.go

@@ -593,6 +593,34 @@ func TestConvertFromUnstructuredConditions(t *testing.T) {
 	}
 }
 
+func TestIsMethods(t *testing.T) {
+	g := NewWithT(t)
+
+	obj := objectWithValueGetter{
+		Status: objectWithValueGetterStatus{
+			Conditions: []metav1.Condition{
+				{Type: "trueCondition", Status: metav1.ConditionTrue},
+				{Type: "falseCondition", Status: metav1.ConditionFalse},
+				{Type: "unknownCondition", Status: metav1.ConditionUnknown},
+			},
+		},
+	}
+
+	// test isTrue
+	g.Expect(IsTrue(obj, "trueCondition")).To(BeTrue())
+	g.Expect(IsTrue(obj, "falseCondition")).To(BeFalse())
+	g.Expect(IsTrue(obj, "unknownCondition")).To(BeFalse())
+	// test isFalse
+	g.Expect(IsFalse(obj, "trueCondition")).To(BeFalse())
+	g.Expect(IsFalse(obj, "falseCondition")).To(BeTrue())
+	g.Expect(IsFalse(obj, "unknownCondition")).To(BeFalse())
+
+	// test isUnknown
+	g.Expect(IsUnknown(obj, "trueCondition")).To(BeFalse())
+	g.Expect(IsUnknown(obj, "falseCondition")).To(BeFalse())
+	g.Expect(IsUnknown(obj, "unknownCondition")).To(BeTrue())
+}
+
 type objectWithValueGetter struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

util/conditions/v1beta2/setter_test.go

@@ -177,3 +177,22 @@ func TestSet(t *testing.T) {
 		g.Expect(foo.Status.Conditions).To(Equal(expected), cmp.Diff(foo.Status.Conditions, expected))
 	})
 }
+
+func TestDelete(t *testing.T) {
+	g := NewWithT(t)
+
+	obj := &builder.Phase2Obj{
+		Status: builder.Phase2ObjStatus{
+			Conditions: []metav1.Condition{
+				{Type: "trueCondition", Status: metav1.ConditionTrue},
+				{Type: "falseCondition", Status: metav1.ConditionFalse},
+			},
+		},
+	}
+
+	Delete(nil, "foo") // no-op
+	Delete(obj, "trueCondition")
+	Delete(obj, "trueCondition") // no-op
+
+	g.Expect(obj.GetV1Beta2Conditions()).To(MatchConditions([]metav1.Condition{{Type: "falseCondition", Status: metav1.ConditionFalse}}, IgnoreLastTransitionTime(true)))
+}