Merge pull request #270 from mshitrit/oos-expiry

Add an expiry mechanism to out-of-service taint.

Author: openshift-merge-bot[bot]

controllers/selfnoderemediation_controller.go

@@ -51,17 +51,18 @@ const (
 	eventReasonRemediationSkipped = "RemediationSkipped"
 
 	// remediation
-	eventReasonAddFinalizer              = "AddFinalizer"
-	eventReasonMarkUnschedulable         = "MarkUnschedulable"
-	eventReasonAddNoExecute              = "AddNoExecute"
-	eventReasonAddOutOfService           = "AddOutOfService"
-	eventReasonUpdateTimeAssumedRebooted = "UpdateTimeAssumedRebooted"
-	eventReasonDeleteResources           = "DeleteResources"
-	eventReasonMarkSchedulable           = "MarkNodeSchedulable"
-	eventReasonRemoveFinalizer           = "RemoveFinalizer"
-	eventReasonRemoveNoExecute           = "RemoveNoExecuteTaint"
-	eventReasonRemoveOutOfService        = "RemoveOutOfService"
-	eventReasonNodeReboot                = "NodeReboot"
+	eventReasonAddFinalizer                 = "AddFinalizer"
+	eventReasonMarkUnschedulable            = "MarkUnschedulable"
+	eventReasonAddNoExecute                 = "AddNoExecute"
+	eventReasonAddOutOfService              = "AddOutOfService"
+	eventReasonUpdateTimeAssumedRebooted    = "UpdateTimeAssumedRebooted"
+	eventReasonDeleteResources              = "DeleteResources"
+	eventReasonMarkSchedulable              = "MarkNodeSchedulable"
+	eventReasonRemoveFinalizer              = "RemoveFinalizer"
+	eventReasonRemoveNoExecute              = "RemoveNoExecuteTaint"
+	eventReasonRemoveOutOfService           = "RemoveOutOfService"
+	eventReasonNodeReboot                   = "NodeReboot"
+	eventReasonOutOfServiceTimestampExpired = "OutOfServiceTimestampExpired"
 )
 
 var (
@@ -81,6 +82,9 @@ var (
 		Value:  "nodeshutdown",
 		Effect: v1.TaintEffectNoExecute,
 	}
+
+	// OutOfServiceTimeoutDuration - time after which out-of-service taint is automatically removed
+	OutOfServiceTimeoutDuration = 1 * time.Minute
 )
 
 type conditionReason string
@@ -430,9 +434,18 @@ func (r *SelfNodeRemediationReconciler) useOutOfServiceTaint(node *v1.Node, snr
 		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 {
@@ -902,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

@@ -352,6 +352,65 @@ var _ = Describe("SNR Controller", func() {
 
 					verifySNRDoesNotExists(snr)
 				})
+
+			It("should automatically remove out-of-service taint after timeout when workloads are not deleted", func() {
+				// Temporarily set a much shorter timeout for testing (instead of 1 minute)
+				originalTimeout := controllers.OutOfServiceTimeoutDuration
+				controllers.OutOfServiceTimeoutDuration = 3 * time.Second
+				DeferCleanup(func() {
+					controllers.OutOfServiceTimeoutDuration = originalTimeout
+				})
+
+				createSNR(snr, remediationStrategy)
+
+				node := verifyNodeIsUnschedulable()
+				addUnschedulableTaint(node)
+
+				verifyTypeConditions(snr, metav1.ConditionTrue, metav1.ConditionUnknown, "RemediationStarted")
+
+				// Create terminating pod but DON'T delete it - this simulates workloads stuck in terminating state
+				createTerminatingPod()
+
+				verifyTimeHasBeenRebootedExists(snr)
+				verifyWatchdogTriggered()
+				verifyFinalizerExists(snr)
+				verifyNoExecuteTaintExist()
+				verifyOutOfServiceTaintExist()
+
+				// Verify timestamp annotation was added
+				nodeKey := types.NamespacedName{Name: shared.UnhealthyNodeName}
+				updatedNode := &v1.Node{}
+				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()
+
+				// Wait for timeout to expire (3 seconds) and verify taint is automatically removed
+				Eventually(func() bool {
+					err := k8sClient.Get(context.Background(), nodeKey, updatedNode)
+					if err != nil {
+						return false
+					}
+					return !utils.TaintExists(updatedNode.Spec.Taints, &v1.Taint{
+						Key:    "node.kubernetes.io/out-of-service",
+						Value:  "nodeshutdown",
+						Effect: v1.TaintEffectNoExecute,
+					})
+				}, 10*time.Second, 250*time.Millisecond).Should(BeTrue(), "out-of-service taint should be automatically removed after 3 second timeout")
+
+				// Verify warning event was emitted for 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()
+				verifyNodeIsSchedulable()
+				removeUnschedulableTaint()
+				verifyNoExecuteTaintRemoved()
+				verifySNRDoesNotExists(snr)
+			})
 		})
 
 		Context("Remediation has a Machine Owner Ref", func() {