Merge pull request #2250 from JoelSpeed/mhc-targets

✨ Add Health Check logic to MachineHealthCheck Reconciler

api/v1alpha3/machinehealthcheck_types.go

@@ -44,6 +44,11 @@ type MachineHealthCheckSpec struct {
 	// "selector" are not healthy.
 	// +optional
 	MaxUnhealthy *intstr.IntOrString `json:"maxUnhealthy,omitempty"`
+
+	// Machines older than this duration without a node will be considered to have
+	// failed and will be remediated.
+	// +optional
+	NodeStartupTimeout *metav1.Duration `json:"nodeStartupTimeout,omitempty"`
 }
 
 // ANCHOR_END: MachineHealthCHeckSpec
@@ -73,11 +78,11 @@ type UnhealthyCondition struct {
 type MachineHealthCheckStatus struct {
 	// total number of machines counted by this machine health check
 	// +kubebuilder:validation:Minimum=0
-	ExpectedMachines int32 `json:"expectedMachines"`
+	ExpectedMachines int32 `json:"expectedMachines,omitempty"`
 
 	// total number of healthy machines counted by this machine health check
 	// +kubebuilder:validation:Minimum=0
-	CurrentHealthy int32 `json:"currentHealthy"`
+	CurrentHealthy int32 `json:"currentHealthy,omitempty"`
 }
 
 // ANCHOR_END: MachineHealthCheckStatus

api/v1alpha3/machinehealthcheck_webhook.go

@@ -18,6 +18,7 @@ package v1alpha3
 
 import (
 	"fmt"
+	"time"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -28,6 +29,14 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 )
 
+var (
+	// Default time allowed for a node to start up. Can be made longer as part of
+	// spec if required for particular provider.
+	// 10 minutes should allow the instance to start and the node to join the
+	// cluster on most providers.
+	defaultNodeStartupTimeout = metav1.Duration{Duration: 10 * time.Minute}
+)
+
 func (m *MachineHealthCheck) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(m).
@@ -46,6 +55,10 @@ func (m *MachineHealthCheck) Default() {
 		defaultMaxUnhealthy := intstr.FromString("100%")
 		m.Spec.MaxUnhealthy = &defaultMaxUnhealthy
 	}
+
+	if m.Spec.NodeStartupTimeout == nil {
+		m.Spec.NodeStartupTimeout = &defaultNodeStartupTimeout
+	}
 }
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
@@ -86,6 +99,13 @@ 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 at least 30s"),
+		)
+	}
+
 	if len(allErrs) == 0 {
 		return nil
 	}

api/v1alpha3/machinehealthcheck_webhook_test.go

@@ -18,6 +18,7 @@ package v1alpha3
 
 import (
 	"testing"
+	"time"
 
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -30,6 +31,8 @@ func TestMachineHealthCheckDefault(t *testing.T) {
 	mhc.Default()
 
 	g.Expect(mhc.Spec.MaxUnhealthy.String()).To(Equal("100%"))
+	g.Expect(mhc.Spec.NodeStartupTimeout).ToNot(BeNil())
+	g.Expect(*mhc.Spec.NodeStartupTimeout).To(Equal(metav1.Duration{Duration: 10 * time.Minute}))
 }
 
 func TestMachineHealthCheckLabelSelectorAsSelectorValidation(t *testing.T) {
@@ -115,3 +118,66 @@ func TestMachineHealthCheckClusterNameImmutable(t *testing.T) {
 		})
 	}
 }
+
+func TestMachineHealthCheckNodeStartupTimeout(t *testing.T) {
+	zero := metav1.Duration{Duration: 0}
+	twentyNineSeconds := metav1.Duration{Duration: 29 * time.Second}
+	thirtySeconds := metav1.Duration{Duration: 30 * time.Second}
+	oneMinute := metav1.Duration{Duration: 1 * time.Minute}
+	minusOneMinute := metav1.Duration{Duration: -1 * time.Minute}
+
+	tests := []struct {
+		name      string
+		timeout   *metav1.Duration
+		expectErr bool
+	}{
+		{
+			name:      "when the nodeStartupTimeout is not given",
+			timeout:   nil,
+			expectErr: false,
+		},
+		{
+			name:      "when the nodeStartupTimeout is greater than 30s",
+			timeout:   &oneMinute,
+			expectErr: false,
+		},
+		{
+			name:      "when the nodeStartupTimeout is 30s",
+			timeout:   &thirtySeconds,
+			expectErr: false,
+		},
+		{
+			name:      "when the nodeStartupTimeout is 29s",
+			timeout:   &twentyNineSeconds,
+			expectErr: true,
+		},
+		{
+			name:      "when the nodeStartupTimeout is less than 0",
+			timeout:   &minusOneMinute,
+			expectErr: true,
+		},
+		{
+			name:      "when the nodeStartupTimeout is 0",
+			timeout:   &zero,
+			expectErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		g := NewWithT(t)
+
+		mhc := &MachineHealthCheck{
+			Spec: MachineHealthCheckSpec{
+				NodeStartupTimeout: tt.timeout,
+			},
+		}
+
+		if tt.expectErr {
+			g.Expect(mhc.ValidateCreate()).NotTo(Succeed())
+			g.Expect(mhc.ValidateUpdate(mhc)).NotTo(Succeed())
+		} else {
+			g.Expect(mhc.ValidateCreate()).To(Succeed())
+			g.Expect(mhc.ValidateUpdate(mhc)).To(Succeed())
+		}
+	}
+}

config/crd/bases/cluster.x-k8s.io_machinehealthchecks.yaml

@@ -67,6 +67,10 @@ spec:
                 description: Any further remediation is only allowed if at most "MaxUnhealthy"
                   machines selected by "selector" are not healthy.
                 x-kubernetes-int-or-string: true
+              nodeStartupTimeout:
+                description: Machines older than this duration without a node will
+                  be considered to have failed and will be remediated.
+                type: string
               selector:
                 description: Label selector to match machines whose health will be
                   exercised
@@ -158,9 +162,6 @@ spec:
                 format: int32
                 minimum: 0
                 type: integer
-            required:
-            - currentHealthy
-            - expectedMachines
             type: object
         type: object
     served: true

controllers/machine_helpers.go

@@ -20,6 +20,8 @@ import (
 	"context"
 
 	"github.com/pkg/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -47,3 +49,20 @@ func getActiveMachinesInCluster(ctx context.Context, c client.Client, namespace,
 	}
 	return machines, nil
 }
+
+// hasMatchingLabels verifies that the Label Selector matches the given Labels
+func hasMatchingLabels(matchSelector metav1.LabelSelector, matchLabels map[string]string) bool {
+	// This should never fail, validating webhook should catch this first
+	selector, err := metav1.LabelSelectorAsSelector(&matchSelector)
+	if err != nil {
+		return false
+	}
+	// If a nil or empty selector creeps in, it should match nothing, not everything.
+	if selector.Empty() {
+		return false
+	}
+	if !selector.Matches(labels.Set(matchLabels)) {
+		return false
+	}
+	return true
+}

controllers/machine_helpers_test.go

@@ -127,3 +127,74 @@ func Test_getActiveMachinesInCluster(t *testing.T) {
 		})
 	}
 }
+
+func TestMachineHealthCheckHasMatchingLabels(t *testing.T) {
+	testCases := []struct {
+		name     string
+		selector metav1.LabelSelector
+		labels   map[string]string
+		expected bool
+	}{
+		{
+			name: "selector matches labels",
+
+			selector: metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"foo": "bar",
+				},
+			},
+
+			labels: map[string]string{
+				"foo": "bar",
+			},
+
+			expected: true,
+		},
+		{
+			name: "selector does not match labels",
+
+			selector: metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"foo": "bar",
+				},
+			},
+
+			labels: map[string]string{
+				"no": "match",
+			},
+			expected: false,
+		},
+		{
+			name:     "selector is empty",
+			selector: metav1.LabelSelector{},
+			labels:   map[string]string{},
+			expected: false,
+		},
+		{
+			name: "seelctor is invalid",
+			selector: metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"foo": "bar",
+				},
+				MatchExpressions: []metav1.LabelSelectorRequirement{
+					{
+						Operator: "bad-operator",
+					},
+				},
+			},
+			labels: map[string]string{
+				"foo": "bar",
+			},
+			expected: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			got := hasMatchingLabels(tc.selector, tc.labels)
+			g.Expect(got).To(Equal(tc.expected))
+		})
+	}
+}