Implementing a storm recovery mechanics which will postponed creation of new remediation as long as the remediation "storm" is active.

Signed-off-by: Michael Shitrit <[email protected]>

Author: mshitrit

api/v1alpha1/nodehealthcheck_types.go

@@ -107,6 +107,18 @@ type NodeHealthCheckSpec struct {
 	//+operator-sdk:csv:customresourcedefinitions:type=spec
 	MaxUnhealthy *intstr.IntOrString `json:"maxUnhealthy,omitempty"`
 
+	// StormRecoveryThreshold defines the number of unhealthy nodes at which storm recovery mode should exit.
+	// When the number of unhealthy nodes drops to this threshold or below, the storm recovery mode will deactivate,
+	// allowing creation of new remediations.
+	//
+	// This threshold must be less than (totalNodes - minHealthy) to prevent permanent storm recovery lock.
+	// This parameter is optional and when not specified, the original minHealthy/maxUnhealthy behavior is preserved.
+	//
+	//+optional
+	//+kubebuilder:validation:Minimum=0
+	//+operator-sdk:csv:customresourcedefinitions:type=spec
+	StormRecoveryThreshold *int `json:"stormRecoveryThreshold,omitempty"`
+
 	// RemediationTemplate is a reference to a remediation template
 	// provided by an infrastructure provider.
 	//
@@ -266,6 +278,24 @@ type NodeHealthCheckStatus struct {
 	//+operator-sdk:csv:customresourcedefinitions:type=status,xDescriptors="urn:alm:descriptor:io.kubernetes.phase:reason"
 	Reason string `json:"reason,omitempty"`
 
+	// StormRecoveryActive indicates if storm recovery mode is currently active.
+	// Storm recovery mode is activated when the number of healthy nodes drops to or below minHealthy,
+	// and remediation is delayed until the number of unhealthy nodes reaches stormRecoveryThreshold.
+	//
+	//+optional
+	//+operator-sdk:csv:customresourcedefinitions:type=status
+	StormRecoveryActive *bool `json:"stormRecoveryActive,omitempty"`
+
+	// StormRecoveryStartTime records when storm recovery mode was activated.
+	// This field is set when StormRecoveryActive becomes true and helps track
+	// how long the system has been in storm recovery mode.
+	//
+	//+optional
+	//+kubebuilder:validation:Type=string
+	//+kubebuilder:validation:Format=date-time
+	//+operator-sdk:csv:customresourcedefinitions:type=status
+	StormRecoveryStartTime *metav1.Time `json:"stormRecoveryStartTime,omitempty"`
+
 	// LastUpdateTime is the last time the status was updated.
 	//
 	//+optional

api/v1alpha1/nodehealthcheck_webhook.go

@@ -27,7 +27,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/util/errors"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -106,8 +106,9 @@ func (v *customValidator) ValidateDelete(_ context.Context, obj runtime.Object)
 }
 
 func (v *customValidator) validate(ctx context.Context, nhc *NodeHealthCheck) error {
-	aggregated := errors.NewAggregate([]error{
+	aggregated := utilerrors.NewAggregate([]error{
 		ValidateMinHealthyMaxUnhealthy(nhc),
+		v.validateStormRecoveryThreshold(ctx, nhc),
 		v.validateSelector(nhc),
 		v.validateMutualRemediations(nhc),
 		v.validateEscalatingRemediations(ctx, nhc),
@@ -152,7 +153,7 @@ func (v *customValidator) validateEscalatingRemediations(ctx context.Context, nh
 		return nil
 	}
 
-	aggregated := errors.NewAggregate([]error{
+	aggregated := utilerrors.NewAggregate([]error{
 		v.validateEscalatingRemediationsUniqueOrder(nhc),
 		v.validateEscalatingRemediationsTimeout(nhc),
 		v.validateEscalatingRemediationsUniqueRemediator(ctx, nhc),
@@ -262,3 +263,72 @@ func ValidateMinHealthyMaxUnhealthy(nhc *NodeHealthCheck) error {
 	}
 	return nil
 }
+
+func (v *customValidator) validateStormRecoveryThreshold(ctx context.Context, nhc *NodeHealthCheck) error {
+	// StormRecoveryThreshold is optional, so skip validation if not specified
+	if nhc.Spec.StormRecoveryThreshold == nil {
+		return nil
+	}
+
+	selector, err := metav1.LabelSelectorAsSelector(&nhc.Spec.Selector)
+	if err != nil {
+		return fmt.Errorf("invalid selector for storm recovery validation: %v", err)
+	}
+
+	var nodes corev1.NodeList
+	// Fetch nodes matching the selector to get actual total count for comprehensive validation
+	if err := v.Client.List(ctx, &nodes, client.MatchingLabelsSelector{Selector: selector}); err != nil {
+		// Storm recovery validation requires actual node count - cannot proceed without it
+		return fmt.Errorf("failed to fetch nodes for storm recovery validation: %v", err)
+	}
+
+	totalNodes := len(nodes.Items)
+	if totalNodes == 0 {
+		return fmt.Errorf("no nodes match the selector, cannot validate storm recovery threshold")
+	}
+
+	// Get the storm recovery threshold value
+	stormThreshold := *nhc.Spec.StormRecoveryThreshold
+
+	// Calculate minHealthy directly to validate the critical constraint
+	minHealthy, err := GetMinHealthy(nhc, totalNodes)
+	if err != nil {
+		nodehealthchecklog.Error(err, "failed to calculate the number of minimum healthy nodes")
+		return err
+	}
+
+	// Critical validation: stormRecoveryThreshold < (totalNodes - minHealthy)
+	// This ensures storm recovery can actually be exited and prevents permanent storm recovery lock
+	maxAllowedStormThreshold := totalNodes - minHealthy
+	if stormThreshold >= maxAllowedStormThreshold {
+		return fmt.Errorf("stormRecoveryThreshold (%d) must be less than (totalNodes - minHealthy) = (%d - %d) = %d to prevent permanent storm recovery lock",
+			stormThreshold, totalNodes, minHealthy, maxAllowedStormThreshold)
+	}
+
+	return nil
+}
+
+func GetMinHealthy(nhc *NodeHealthCheck, total int) (int, error) {
+	err := ValidateMinHealthyMaxUnhealthy(nhc)
+	if err != nil {
+		return 0, err
+	}
+	if nhc.Spec.MinHealthy != nil {
+		minHealthy, err := intstr.GetScaledValueFromIntOrPercent(nhc.Spec.MinHealthy, total, true)
+		if minHealthy < 0 && err == nil {
+			err = fmt.Errorf("minHealthy is negative: %d", minHealthy)
+		}
+		return minHealthy, err
+	}
+	if nhc.Spec.MaxUnhealthy != nil {
+		maxUnhealthy, err := intstr.GetScaledValueFromIntOrPercent(nhc.Spec.MaxUnhealthy, total, true)
+		if maxUnhealthy < 0 && err == nil {
+			err = fmt.Errorf("maxUnhealthy is negative: %d", maxUnhealthy)
+		}
+		if maxUnhealthy > total && err == nil {
+			err = fmt.Errorf("maxUnhealthy is greater than the number of selected nodes: %d", maxUnhealthy)
+		}
+		return total - maxUnhealthy, err
+	}
+	return 0, fmt.Errorf("one of minHealthy and maxUnhealthy should be specified")
+}

api/v1alpha1/nodehealthcheck_webhook_test.go

@@ -2,6 +2,7 @@ package v1alpha1
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -428,6 +429,119 @@ var _ = Describe("NodeHealthCheck Validation", func() {
 				Expect(nhc.isRemediating()).To(BeTrue())
 			})
 		})
+
+		Context("Storm Recovery Validation", func() {
+			When("valid storm recovery configuration", func() {
+				BeforeEach(func() {
+					// Setup: 10 nodes, minHealthy=60% (6), stormRecoveryThreshold=2
+					// Valid because: 2 < (10-6) = 4
+					mh := intstr.FromString("60%")
+					nhc.Spec.MinHealthy = &mh
+					stormThreshold := 2
+					nhc.Spec.StormRecoveryThreshold = &stormThreshold
+					// Keep the RemediationTemplate from parent BeforeEach
+					nhc.Spec.RemediationTemplate = &v1.ObjectReference{
+						Kind:       "R",
+						Namespace:  "dummy",
+						Name:       "r",
+						APIVersion: "r",
+					}
+					nhc.Spec.Selector = metav1.LabelSelector{
+						MatchExpressions: []metav1.LabelSelectorRequirement{
+							{
+								Key:      "node-role.kubernetes.io/control-plane",
+								Operator: metav1.LabelSelectorOpDoesNotExist,
+							},
+						},
+					}
+
+					// Mock 10 nodes
+					mockValidatorClient.listFunc = func(ctx context.Context, nodesList client.ObjectList, opts ...client.ListOption) error {
+						nodeList := nodesList.(*v1.NodeList)
+						nodeList.Items = make([]v1.Node, 10)
+						for i := 0; i < 10; i++ {
+							nodeList.Items[i] = v1.Node{
+								ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("node-%d", i)},
+							}
+						}
+						return nil
+					}
+				})
+
+				It("should be allowed", func() {
+					Expect(validator.validate(context.Background(), nhc)).To(Succeed())
+				})
+			})
+
+			When("storm recovery threshold too high with percentage minHealthy", func() {
+				BeforeEach(func() {
+					// Setup: 5 nodes, minHealthy=60% (3), stormRecoveryThreshold=3
+					// Invalid because: 3 >= (5-3) = 2
+					mh := intstr.FromString("60%")
+					nhc.Spec.MinHealthy = &mh
+					stormThreshold := 3
+					nhc.Spec.StormRecoveryThreshold = &stormThreshold
+					// Keep the RemediationTemplate from parent BeforeEach
+					nhc.Spec.RemediationTemplate = &v1.ObjectReference{
+						Kind:       "R",
+						Namespace:  "dummy",
+						Name:       "r",
+						APIVersion: "r",
+					}
+
+					// Mock 5 nodes
+					mockValidatorClient.listFunc = func(ctx context.Context, nodesList client.ObjectList, opts ...client.ListOption) error {
+						nodeList := nodesList.(*v1.NodeList)
+						nodeList.Items = make([]v1.Node, 5)
+						for i := 0; i < 5; i++ {
+							nodeList.Items[i] = v1.Node{
+								ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("node-%d", i)},
+							}
+						}
+						return nil
+					}
+				})
+
+				It("should be denied", func() {
+					Expect(validator.validate(context.Background(), nhc)).To(MatchError(ContainSubstring("stormRecoveryThreshold (3) must be less than (totalNodes - minHealthy) = (5 - 3) = 2 to prevent permanent storm recovery lock")))
+				})
+			})
+
+			When("storm recovery threshold too high with fixed maxUnhealthy", func() {
+				BeforeEach(func() {
+					// Setup: 6 nodes, maxUnhealthy=2 (minHealthy=4), stormRecoveryThreshold=3
+					// Invalid because: 3 >= (6-4) = 2
+					nhc.Spec.MinHealthy = nil
+					maxUnhealthy := intstr.FromInt(2)
+					nhc.Spec.MaxUnhealthy = &maxUnhealthy
+					stormThreshold := 3
+					nhc.Spec.StormRecoveryThreshold = &stormThreshold
+					// Keep the RemediationTemplate from parent BeforeEach
+					nhc.Spec.RemediationTemplate = &v1.ObjectReference{
+						Kind:       "R",
+						Namespace:  "dummy",
+						Name:       "r",
+						APIVersion: "r",
+					}
+
+					// Mock 6 nodes
+					mockValidatorClient.listFunc = func(ctx context.Context, nodesList client.ObjectList, opts ...client.ListOption) error {
+						nodeList := nodesList.(*v1.NodeList)
+						nodeList.Items = make([]v1.Node, 6)
+						for i := 0; i < 6; i++ {
+							nodeList.Items[i] = v1.Node{
+								ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("node-%d", i)},
+							}
+						}
+						return nil
+					}
+				})
+
+				It("should be denied", func() {
+					Expect(validator.validate(context.Background(), nhc)).To(MatchError(ContainSubstring("stormRecoveryThreshold (3) must be less than (totalNodes - minHealthy) = (6 - 4) = 2 to prevent permanent storm recovery lock")))
+				})
+			})
+		})
 	})
 })
 

api/v1alpha1/zz_generated.deepcopy.go

@@ -137,6 +137,15 @@ spec:
           to work with an empty selector, which matches all nodes."
         displayName: Selector
         path: selector
+      - description: "StormRecoveryThreshold defines the number of unhealthy nodes
+          at which storm recovery mode should exit. When the number of unhealthy nodes
+          drops to this threshold or below, the storm recovery mode will deactivate,
+          allowing creation of new remediations. \n This threshold must be less than
+          (totalNodes - minHealthy) to prevent permanent storm recovery lock. This
+          parameter is optional and when not specified, the original minHealthy/maxUnhealthy
+          behavior is preserved."
+        displayName: Storm Recovery Threshold
+        path: stormRecoveryThreshold
       - description: UnhealthyConditions contains a list of the conditions that determine
           whether a node is considered unhealthy.  The conditions are combined in
           a logical OR, i.e. if any of the conditions is met, the node is unhealthy.
@@ -189,6 +198,17 @@ spec:
         path: reason
         x-descriptors:
         - urn:alm:descriptor:io.kubernetes.phase:reason
+      - description: StormRecoveryActive indicates if storm recovery mode is currently
+          active. Storm recovery mode is activated when the number of healthy nodes
+          drops to or below minHealthy, and remediation is delayed until the number
+          of unhealthy nodes reaches stormRecoveryThreshold.
+        displayName: Storm Recovery Active
+        path: stormRecoveryActive
+      - description: StormRecoveryStartTime records when storm recovery mode was activated.
+          This field is set when StormRecoveryActive becomes true and helps track
+          how long the system has been in storm recovery mode.
+        displayName: Storm Recovery Start Time
+        path: stormRecoveryStartTime
       - description: UnhealthyNodes tracks currently unhealthy nodes and their remediations.
         displayName: Unhealthy Nodes
         path: unhealthyNodes

bundle/manifests/node-healthcheck-operator.clusterserviceversion.yaml

@@ -280,6 +280,17 @@ spec:
                     type: object
                 type: object
                 x-kubernetes-map-type: atomic
+              stormRecoveryThreshold:
+                description: |-
+                  StormRecoveryThreshold defines the number of unhealthy nodes at which storm recovery mode should exit.
+                  When the number of unhealthy nodes drops to this threshold or below, the storm recovery mode will deactivate,
+                  allowing creation of new remediations.
+
+
+                  This threshold must be less than (totalNodes - minHealthy) to prevent permanent storm recovery lock.
+                  This parameter is optional and when not specified, the original minHealthy/maxUnhealthy behavior is preserved.
+                minimum: 0
+                type: integer
               unhealthyConditions:
                 default:
                 - duration: 300s
@@ -438,6 +449,19 @@ spec:
               reason:
                 description: Reason explains the current phase in more detail.
                 type: string
+              stormRecoveryActive:
+                description: |-
+                  StormRecoveryActive indicates if storm recovery mode is currently active.
+                  Storm recovery mode is activated when the number of healthy nodes drops to or below minHealthy,
+                  and remediation is delayed until the number of unhealthy nodes reaches stormRecoveryThreshold.
+                type: boolean
+              stormRecoveryStartTime:
+                description: |-
+                  StormRecoveryStartTime records when storm recovery mode was activated.
+                  This field is set when StormRecoveryActive becomes true and helps track
+                  how long the system has been in storm recovery mode.
+                format: date-time
+                type: string
               unhealthyNodes:
                 description: UnhealthyNodes tracks currently unhealthy nodes and their
                   remediations.