Address comments

Author: Sedef

api/v1alpha3/condition_consts.go

@@ -117,7 +117,7 @@ const (
 )
 
 const (
-	// MachineOwnerRemediatedCondition is set on machines that have failed a healthcheck by the Machine's owner controller.
+	// MachineOwnerRemediatedCondition is set on machines that have failed a healthcheck by the MachineHealthCheck controller.
 	// MachineOwnerRemediatedCondition is set to False after a health check fails, but should be changed to True by the owning controller after remediation succeeds.
 	MachineOwnerRemediatedCondition ConditionType = "OwnerRemediated"
 

controllers/machine_controller_node.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/pkg/errors"
 	apicorev1 "k8s.io/api/core/v1"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 	capierrors "sigs.k8s.io/cluster-api/errors"
@@ -91,33 +92,33 @@ func (r *MachineReconciler) reconcileNode(ctx context.Context, cluster *clusterv
 	}
 
 	// Do the remaining node health checks, then set the node health to true if all checks pass.
-	if !checkNodeConditions(node) {
-		conditions.MarkFalse(machine, clusterv1.MachineNodeHealthyCondition, clusterv1.NodeConditionsFailedReason, clusterv1.ConditionSeverityWarning, "")
+	if err := hasHealthyNodeConditions(node); err != nil {
+		conditions.MarkFalse(machine, clusterv1.MachineNodeHealthyCondition, clusterv1.NodeConditionsFailedReason, clusterv1.ConditionSeverityWarning, "Unhealthy conditions: %s", err.Error())
 		return nil
 	}
-	conditions.MarkTrue(machine, clusterv1.MachineNodeHealthyCondition)
 
+	conditions.MarkTrue(machine, clusterv1.MachineNodeHealthyCondition)
 	return nil
 }
 
-// checkNodeConditions checks if a Node is healthy and does not have any semantically negative Conditions.
-// Returns true if NodeMemoryPressure, NodeDiskPressure, or NodePIDPressure Conditions are false or Ready Condition is true.
-func checkNodeConditions(node *apicorev1.Node) bool {
+// hasHealthyNodeConditions checks if a Node is healthy and does not have any semantically negative Conditions.
+// Returns error if NodeMemoryPressure, NodeDiskPressure, or NodePIDPressure Conditions are false or Ready Condition is true.
+func hasHealthyNodeConditions(node *apicorev1.Node) error {
+	errList := []error{}
+
 	for _, condition := range node.Status.Conditions {
-		if condition.Type == apicorev1.NodeMemoryPressure && condition.Status == apicorev1.ConditionTrue {
-			return false
-		}
-		if condition.Type == apicorev1.NodeDiskPressure && condition.Status == apicorev1.ConditionTrue {
-			return false
-		}
-		if condition.Type == apicorev1.NodePIDPressure && condition.Status == apicorev1.ConditionTrue {
-			return false
-		}
-		if condition.Type == apicorev1.NodeReady && condition.Status == apicorev1.ConditionFalse {
-			return false
+		switch condition.Type {
+		case apicorev1.NodeMemoryPressure, apicorev1.NodeDiskPressure, apicorev1.NodePIDPressure:
+			if condition.Status == apicorev1.ConditionTrue {
+				errList = append(errList, errors.Errorf("Condition %s is true on Node: %s", condition.Type, node.Name))
+			}
+		case apicorev1.NodeReady:
+			if condition.Status == apicorev1.ConditionFalse {
+				errList = append(errList, errors.Errorf("Condition %s is false on Node: %s", condition.Type, node.Name))
+			}
 		}
 	}
-	return true
+	return kerrors.NewAggregate(errList)
 }
 
 func (r *MachineReconciler) getNode(c client.Reader, providerID *noderefutil.ProviderID) (*apicorev1.Node, error) {

controllers/machine_controller_node_test.go

@@ -108,20 +108,20 @@ func TestGetNodeReference(t *testing.T) {
 			providerID, err := noderefutil.NewProviderID(test.providerID)
 			gt.Expect(err).NotTo(HaveOccurred(), "Expected no error parsing provider id %q, got %v", test.providerID, err)
 
-			reference, err := r.getNode(client, providerID)
+			node, err := r.getNode(client, providerID)
 			if test.err == nil {
 				g.Expect(err).To(BeNil())
 			} else {
 				gt.Expect(err).NotTo(BeNil())
 				gt.Expect(err).To(Equal(test.err), "Expected error %v, got %v", test.err, err)
 			}
 
-			if test.expected == nil && reference == nil {
+			if test.expected == nil && node == nil {
 				return
 			}
 
-			gt.Expect(reference.Name).To(Equal(test.expected.Name), "Expected NodeRef's name to be %v, got %v", reference.Name, test.expected.Name)
-			gt.Expect(reference.Namespace).To(Equal(test.expected.Namespace), "Expected NodeRef's namespace to be %v, got %v", reference.Namespace, test.expected.Namespace)
+			gt.Expect(node.Name).To(Equal(test.expected.Name), "Expected NodeRef's name to be %v, got %v", node.Name, test.expected.Name)
+			gt.Expect(node.Namespace).To(Equal(test.expected.Namespace), "Expected NodeRef's namespace to be %v, got %v", node.Namespace, test.expected.Namespace)
 		})
 
 	}