Feedback #3

Author: JoelSpeed

api/v1alpha3/machinehealthcheck_webhook.go

@@ -102,7 +102,7 @@ func (m *MachineHealthCheck) validate(old *MachineHealthCheck) error {
 	if m.Spec.NodeStartupTimeout != nil && m.Spec.NodeStartupTimeout.Seconds() < 30 {
 		allErrs = append(
 			allErrs,
-			field.Invalid(field.NewPath("spec", "nodeStartupTimeout"), m.Spec.NodeStartupTimeout, "must be greater at least 30s"),
+			field.Invalid(field.NewPath("spec", "nodeStartupTimeout"), m.Spec.NodeStartupTimeout, "must be at least 30s"),
 		)
 	}
 

controllers/machinehealthcheck_controller.go

@@ -303,26 +303,7 @@ func (r *MachineHealthCheckReconciler) nodeToMachineHealthCheck(o handler.MapObj
 		return nil
 	}
 
-	mhcList := &clusterv1.MachineHealthCheckList{}
-	if err := r.Client.List(
-		context.Background(),
-		mhcList,
-		&client.ListOptions{Namespace: machine.Namespace},
-		client.MatchingFields{mhcClusterNameIndex: machine.Spec.ClusterName},
-	); err != nil {
-		r.Log.Error(err, "Unable to list MachineHealthChecks", "node", node.Name, "machine", machine.Name, "namespace", machine.Namespace)
-		return nil
-	}
-
-	var requests []reconcile.Request
-	for k := range mhcList.Items {
-		mhc := &mhcList.Items[k]
-		if hasMatchingLabels(mhc.Spec.Selector, machine.Labels) {
-			key := util.ObjectKey(mhc)
-			requests = append(requests, reconcile.Request{NamespacedName: key})
-		}
-	}
-	return requests
+	return r.machineToMachineHealthCheck(handler.MapObject{Object: machine})
 }
 
 func (r *MachineHealthCheckReconciler) getMachineFromNode(nodeName string) (*clusterv1.Machine, error) {

controllers/machinehealthcheck_controller_test.go

@@ -33,6 +33,7 @@ import (
 	"k8s.io/client-go/kubernetes/scheme"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/controllers/external"
+	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
@@ -114,7 +115,7 @@ var _ = Describe("MachineHealthCheck Reconciler", func() {
 
 			Eventually(func() map[string]string {
 				mhc := &clusterv1.MachineHealthCheck{}
-				err := k8sClient.Get(ctx, client.ObjectKey{Namespace: mhcToCreate.GetNamespace(), Name: mhcToCreate.GetName()}, mhc)
+				err := k8sClient.Get(ctx, util.ObjectKey(mhcToCreate), mhc)
 				if err != nil {
 					return nil
 				}
@@ -165,7 +166,7 @@ var _ = Describe("MachineHealthCheck Reconciler", func() {
 
 			Eventually(func() []metav1.OwnerReference {
 				mhc := &clusterv1.MachineHealthCheck{}
-				err := k8sClient.Get(ctx, client.ObjectKey{Namespace: mhcToCreate.GetNamespace(), Name: mhcToCreate.GetName()}, mhc)
+				err := k8sClient.Get(ctx, util.ObjectKey(mhcToCreate), mhc)
 				if err != nil {
 					return []metav1.OwnerReference{}
 				}
@@ -207,7 +208,7 @@ func cleanupTestMachineHealthChecks(ctx context.Context, c client.Client) error
 func ownerReferenceForCluster(ctx context.Context, c *clusterv1.Cluster) metav1.OwnerReference {
 	// Fetch the cluster to populate the UID
 	cc := &clusterv1.Cluster{}
-	Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: c.GetNamespace(), Name: c.GetName()}, cc)).To(Succeed())
+	Expect(k8sClient.Get(ctx, util.ObjectKey(c), cc)).To(Succeed())
 
 	return metav1.OwnerReference{
 		APIVersion: clusterv1.GroupVersion.String(),
@@ -341,11 +342,8 @@ func TestClusterToMachineHealthCheck(t *testing.T) {
 					gs.Expect(r.Client.Delete(ctx, &o)).To(Succeed())
 				}()
 				// Check the cache is populated
-				key, err := client.ObjectKeyFromObject(&o)
-				gs.Expect(err).ToNot(HaveOccurred())
-
 				getObj := func() error {
-					return r.Client.Get(ctx, key, &clusterv1.MachineHealthCheck{})
+					return r.Client.Get(ctx, util.ObjectKey(&o), &clusterv1.MachineHealthCheck{})
 				}
 				gs.Eventually(getObj, timeout).Should(Succeed())
 			}
@@ -539,11 +537,8 @@ func TestMachineToMachineHealthCheck(t *testing.T) {
 					gs.Expect(r.Client.Delete(ctx, &o)).To(Succeed())
 				}()
 				// Check the cache is populated
-				key, err := client.ObjectKeyFromObject(&o)
-				gs.Expect(err).ToNot(HaveOccurred())
-
 				getObj := func() error {
-					return r.Client.Get(ctx, key, &clusterv1.MachineHealthCheck{})
+					return r.Client.Get(ctx, util.ObjectKey(&o), &clusterv1.MachineHealthCheck{})
 				}
 				gs.Eventually(getObj, timeout).Should(Succeed())
 			}
@@ -675,7 +670,7 @@ func TestNodeToMachineHealthCheck(t *testing.T) {
 			expected: []reconcile.Request{mhc1Req},
 		},
 		{
-			name:        "when two NachineHealthChecks exist for the Node in the Machine's namespace",
+			name:        "when two MachineHealthChecks exist for the Node in the Machine's namespace",
 			mhcToCreate: []clusterv1.MachineHealthCheck{*mhc1, *mhc2},
 			mToCreate:   []clusterv1.Machine{*machine1},
 			object: handler.MapObject{
@@ -706,9 +701,7 @@ func TestNodeToMachineHealthCheck(t *testing.T) {
 					gs.Expect(r.Client.Delete(ctx, &o)).To(Succeed())
 				}()
 				// Check the cache is populated
-				key, err := client.ObjectKeyFromObject(&o)
-				gs.Expect(err).ToNot(HaveOccurred())
-
+				key := util.ObjectKey(&o)
 				getObj := func() error {
 					return r.Client.Get(ctx, key, &clusterv1.MachineHealthCheck{})
 				}
@@ -720,14 +713,12 @@ func TestNodeToMachineHealthCheck(t *testing.T) {
 				defer func() {
 					gs.Expect(r.Client.Delete(ctx, &o)).To(Succeed())
 				}()
-				// Ensure the status is set (required for matchin node to machine)
+				// Ensure the status is set (required for matching node to machine)
 				o.Status = obj.Status
 				gs.Expect(r.Client.Status().Update(ctx, &o)).To(Succeed())
 
 				// Check the cache is up to date with the status update
-				key, err := client.ObjectKeyFromObject(&o)
-				gs.Expect(err).ToNot(HaveOccurred())
-
+				key := util.ObjectKey(&o)
 				checkStatus := func() clusterv1.MachineStatus {
 					m := &clusterv1.Machine{}
 					err := r.Client.Get(ctx, key, m)

controllers/machinehealthcheck_targets.go

@@ -63,7 +63,7 @@ func (t *healthCheckTarget) nodeName() string {
 }
 
 // Determine whether or not a given target needs remediation.
-// The node will be need rememdiation if any of the following are true:
+// The node will need remediation if any of the following are true:
 // - The Machine has failed for some reason
 // - The Machine did not get a node before `timeoutForMachineToHaveNode` elapses
 // - The Node has gone away
@@ -103,7 +103,6 @@ func (t *healthCheckTarget) needsRemediation(logger logr.Logger, timeoutForMachi
 
 	// check conditions
 	for _, c := range t.MHC.Spec.UnhealthyConditions {
-		now := time.Now()
 		nodeCondition := getNodeCondition(t.Node, c.Type)
 
 		// Skip when current node condition is different from the one reported
@@ -165,7 +164,7 @@ func (r *MachineHealthCheckReconciler) getTargetsFromMHC(clusterClient client.Cl
 func (r *MachineHealthCheckReconciler) getMachinesFromMHC(mhc *clusterv1.MachineHealthCheck) ([]clusterv1.Machine, error) {
 	selector, err := metav1.LabelSelectorAsSelector(&mhc.Spec.Selector)
 	if err != nil {
-		return nil, errors.New("failed to build selector")
+		return nil, errors.Wrap(err, "failed to build selector")
 	}
 
 	options := client.ListOptions{
@@ -188,8 +187,7 @@ func (r *MachineHealthCheckReconciler) getNodeFromMachine(clusterClient client.C
 
 	node := &corev1.Node{}
 	nodeKey := types.NamespacedName{
-		Namespace: machine.Status.NodeRef.Namespace,
-		Name:      machine.Status.NodeRef.Name,
+		Name: machine.Status.NodeRef.Name,
 	}
 	err := clusterClient.Get(context.TODO(), nodeKey, node)
 	return node, err
@@ -248,9 +246,9 @@ func minDuration(durations []time.Duration) time.Duration {
 		return time.Duration(0)
 	}
 
-	// durations should all be less than 1 Hour
-	minDuration := time.Hour
-	for _, nc := range durations {
+	minDuration := durations[0]
+	// Ignore first element as that is already minDuration
+	for _, nc := range durations[1:] {
 		if nc < minDuration {
 			minDuration = nc
 		}

controllers/machinehealthcheck_targets_test.go

@@ -231,7 +231,7 @@ func TestHealthCheckTargets(t *testing.T) {
 		Node:    testNodeUnknown400,
 	}
 
-	// Traget for when a node is healthy
+	// Target for when a node is healthy
 	testNodeHealthy := newTestNode("node1")
 	testNodeHealthy.UID = "12345"
 	nodeHealthy := healthCheckTarget{
@@ -355,6 +355,10 @@ func newTestMachine(name, namespace, clusterName, nodeName string, labels map[st
 
 func newTestNode(name string) *corev1.Node {
 	return &corev1.Node{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "v1",
+			Kind:       "Node",
+		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,
 		},