Apply Feedback

- remove oos taint only when the node is healthy
- reuse existing expiry mechanism
- phrasing and other nits

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

Author: mshitrit

controllers/selfnoderemediation_controller.go

@@ -44,10 +44,9 @@ import (
 )
 
 const (
-	SNRFinalizer                    = "self-node-remediation.medik8s.io/snr-finalizer"
-	nhcTimeOutAnnotation            = "remediation.medik8s.io/nhc-timed-out"
-	excludeRemediationLabel         = "remediation.medik8s.io/exclude-from-remediation"
-	outOfServiceTimestampAnnotation = "remediation.medik8s.io/oos-timestamp"
+	SNRFinalizer            = "self-node-remediation.medik8s.io/snr-finalizer"
+	nhcTimeOutAnnotation    = "remediation.medik8s.io/nhc-timed-out"
+	excludeRemediationLabel = "remediation.medik8s.io/exclude-from-remediation"
 
 	eventReasonRemediationSkipped = "RemediationSkipped"
 
@@ -429,25 +428,24 @@ func (r *SelfNodeRemediationReconciler) useOutOfServiceTaint(node *v1.Node, snr
 		return 0, err
 	}
 
-	isTaintRemoved, err := r.checkAndHandleExpiredOutOfServiceTaint(node)
-	if err != nil {
-		r.logger.Error(err, "Failed to check/handle expired out-of-service taint", "node name", node.Name)
-		return 0, err
-	}
-
-	if isTaintRemoved {
-		return 0, nil
-	}
-
 	// We can not control to delete node resources by the "out-of-service" taint
 	// So timer is used to avoid to keep waiting to complete
 	if !r.isResourceDeletionCompleted(node) {
 		isExpired, timeLeft := r.isResourceDeletionExpired(snr)
 		if !isExpired {
 			return timeLeft, nil
+		} else if snr.GetDeletionTimestamp() != nil { // Time expired and node is healthy
+			err := r.removeOutOfServiceTaint(node)
+			if err != nil {
+				return 0, err
+			}
+			// Emit an event about the timeout expiration
+			events.WarningEvent(r.Recorder, node, eventReasonOutOfServiceTimestampExpired,
+				"Out-of-service taint automatically removed due to timeout expiration on a healthy node")
+			return 0, nil
+		} else { // if the timer is expired, but the node is still unhealthy exponential backoff is triggered
+			return 0, errors.New("Not ready to delete out-of-service taint")
 		}
-		// if the timer is expired, exponential backoff is triggered
-		return 0, errors.New("Not ready to delete out-of-service taint")
 	}
 
 	if err := r.removeOutOfServiceTaint(node); err != nil {
@@ -854,19 +852,12 @@ func (r *SelfNodeRemediationReconciler) addOutOfServiceTaint(node *v1.Node) erro
 	now := metav1.Now()
 	taint.TimeAdded = &now
 	node.Spec.Taints = append(node.Spec.Taints, taint)
-
-	// Add out-of-service timestamp annotation
-	if node.Annotations == nil {
-		node.Annotations = make(map[string]string)
-	}
-	node.Annotations[outOfServiceTimestampAnnotation] = now.Format(time.RFC3339)
-
 	if err := r.Client.Patch(context.Background(), node, patch); err != nil {
-		r.logger.Error(err, "Failed to add out-of-service taint and timestamp annotation on node", "node name", node.Name)
+		r.logger.Error(err, "Failed to add out-of-service taint on node", "node name", node.Name)
 		return err
 	}
 	events.NormalEvent(r.Recorder, node, eventReasonAddOutOfService, "Remediation process - add out-of-service taint to unhealthy node")
-	r.logger.Info("out-of-service taint and timestamp annotation added", "new taints", node.Spec.Taints, "timestamp", now.Format(time.RFC3339))
+	r.logger.Info("out-of-service taint added", "new taints", node.Spec.Taints)
 	return nil
 }
 
@@ -883,67 +874,15 @@ func (r *SelfNodeRemediationReconciler) removeOutOfServiceTaint(node *v1.Node) e
 	} else {
 		node.Spec.Taints = taints
 	}
-
-	delete(node.Annotations, outOfServiceTimestampAnnotation)
-
 	if err := r.Client.Patch(context.Background(), node, patch); err != nil {
-		r.logger.Error(err, "Failed to remove taint and timestamp annotation from node,", "node name", node.Name, "taint key", OutOfServiceTaint.Key, "taint effect", OutOfServiceTaint.Effect)
+		r.logger.Error(err, "Failed to remove taint from node,", "node name", node.Name, "taint key", OutOfServiceTaint.Key, "taint effect", OutOfServiceTaint.Effect)
 		return err
 	}
 	events.NormalEvent(r.Recorder, node, eventReasonRemoveOutOfService, "Remediation process - remove out-of-service taint from node")
-	r.logger.Info("out-of-service taint and timestamp annotation removed", "new taints", node.Spec.Taints)
+	r.logger.Info("out-of-service taint removed", "new taints", node.Spec.Taints)
 	return nil
 }
 
-// checkAndHandleExpiredOutOfServiceTaint checks if the out-of-service taint has been on the node for longer than OutOfServiceTimeoutDuration
-// and removes it if expired
-func (r *SelfNodeRemediationReconciler) checkAndHandleExpiredOutOfServiceTaint(node *v1.Node) (bool, error) {
-	// Check if node has the timestamp annotation
-	if node.Annotations == nil {
-		return false, nil
-	}
-
-	timestampStr, exists := node.Annotations[outOfServiceTimestampAnnotation]
-	if !exists {
-		r.logger.Info("out-of-service taint exists but no timestamp annotation found", "node name", node.Name)
-		return false, nil
-	}
-
-	// Parse the timestamp
-	timestamp, err := time.Parse(time.RFC3339, timestampStr)
-	if err != nil {
-		r.logger.Error(err, "Failed to parse out-of-service timestamp annotation", "node name", node.Name, "timestamp", timestampStr)
-		return false, err
-	}
-
-	// Check if the timeout has expired
-	if time.Since(timestamp) >= OutOfServiceTimeoutDuration {
-		r.logger.Info("out-of-service taint timeout expired, removing taint and annotation",
-			"node name", node.Name,
-			"timestamp", timestampStr,
-			"elapsed", time.Since(timestamp),
-			"timeout", OutOfServiceTimeoutDuration)
-
-		// Remove the out-of-service taint and annotation
-		if err := r.removeOutOfServiceTaint(node); err != nil {
-			return false, err
-		}
-
-		// Emit an event about the timeout expiration
-		events.WarningEvent(r.Recorder, node, eventReasonOutOfServiceTimestampExpired,
-			"Out-of-service taint automatically removed due to timeout expiration")
-
-		return true, nil
-	}
-
-	r.logger.Info("out-of-service taint timeout not yet expired",
-		"node name", node.Name,
-		"elapsed", time.Since(timestamp),
-		"timeout", OutOfServiceTimeoutDuration)
-
-	return false, nil
-}
-
 func (r *SelfNodeRemediationReconciler) isResourceDeletionCompleted(node *v1.Node) bool {
 	pods := &v1.PodList{}
 	if err := r.Client.List(context.Background(), pods); err != nil {
@@ -976,7 +915,7 @@ func (r *SelfNodeRemediationReconciler) isPodTerminating(pod *v1.Pod) bool {
 }
 
 func (r *SelfNodeRemediationReconciler) isResourceDeletionExpired(snr *v1alpha1.SelfNodeRemediation) (bool, time.Duration) {
-	waitTime := snr.Status.TimeAssumedRebooted.Add(300 * time.Second)
+	waitTime := snr.Status.TimeAssumedRebooted.Add(OutOfServiceTimeoutDuration)
 
 	if waitTime.After(time.Now()) {
 		return false, 5 * time.Second

controllers/tests/controller/selfnoderemediation_controller_test.go

@@ -380,25 +380,10 @@ var _ = Describe("SNR Controller", func() {
 				// Verify timestamp annotation was added
 				nodeKey := types.NamespacedName{Name: shared.UnhealthyNodeName}
 				updatedNode := &v1.Node{}
-				Eventually(func() bool {
-					err := k8sClient.Get(context.Background(), nodeKey, updatedNode)
-					if err != nil {
-						return false
-					}
-					if updatedNode.Annotations == nil {
-						return false
-					}
-					timestampStr, exists := updatedNode.Annotations["remediation.medik8s.io/oos-timestamp"]
-					if !exists || timestampStr == "" {
-						return false
-					}
-					// Verify timestamp is valid RFC3339 format
-					_, err = time.Parse(time.RFC3339, timestampStr)
-					return err == nil
-				}, 10*time.Second, 250*time.Millisecond).Should(BeTrue(), "out-of-service timestamp annotation should exist and be valid")
-
 				verifyEvent("Normal", "AddOutOfService", "Remediation process - add out-of-service taint to unhealthy node")
 
+				// Simulate NHC trying to delete SNR because the node is healthy
+				deleteSNR(snr)
 				// Wait for less than timeout (1 second) and verify taint still exists
 				time.Sleep(1 * time.Second)
 				verifyOutOfServiceTaintExist()
@@ -416,25 +401,11 @@ var _ = Describe("SNR Controller", func() {
 					})
 				}, 10*time.Second, 250*time.Millisecond).Should(BeTrue(), "out-of-service taint should be automatically removed after 3 second timeout")
 
-				// Verify timestamp annotation was also removed
-				Eventually(func() bool {
-					err := k8sClient.Get(context.Background(), nodeKey, updatedNode)
-					if err != nil {
-						return false
-					}
-					if updatedNode.Annotations == nil {
-						return true
-					}
-					_, exists := updatedNode.Annotations["remediation.medik8s.io/oos-timestamp"]
-					return !exists
-				}, 10*time.Second, 250*time.Millisecond).Should(BeTrue(), "out-of-service timestamp annotation should be removed")
-
 				// Verify warning event was emitted for timeout expiration
-				verifyEvent("Warning", "OutOfServiceTimestampExpired", "Out-of-service taint automatically removed due to timeout expiration")
+				verifyEvent("Warning", "OutOfServiceTimestampExpired", "Out-of-service taint automatically removed due to timeout expiration on a healthy node")
 
 				// Clean up: manually delete the terminating pod and SNR to complete the test
 				deleteTerminatingPod()
-				deleteSNR(snr)
 				verifyNodeIsSchedulable()
 				removeUnschedulableTaint()
 				verifyNoExecuteTaintRemoved()