From c92e2a30120c5064dda96919a74525b95d395a84 Mon Sep 17 00:00:00 2001
From: Sebastian Widmer <sebastian.widmer@vshn.net>
Date: Fri, 21 Jun 2024 14:26:56 +0200
Subject: [PATCH] Wait until `startAfter` before skipping upgrade job (#90)

---
 controllers/upgradejob_controller.go      | 50 ++++++++++++++++-------
 controllers/upgradejob_controller_test.go |  7 ++++
 2 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/controllers/upgradejob_controller.go b/controllers/upgradejob_controller.go
index bd64a81..358cb88 100644
--- a/controllers/upgradejob_controller.go
+++ b/controllers/upgradejob_controller.go
@@ -121,22 +121,15 @@ func (r *UpgradeJobReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 
 	now := r.Clock.Now()
 
-	window, err := r.matchingUpgradeSuspensionWindow(ctx, uj, now)
-	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed to search for matching upgrade suspension window: %w", err)
-	}
-	if window != nil {
-		l.Info("Upgrade job skipped by UpgradeSuspensionWindow", "window", window.Name, "reason", window.Spec.Reason, "start", window.Spec.Start.Time, "end", window.Spec.End.Time)
-		r.setStatusCondition(&uj.Status.Conditions, metav1.Condition{
-			Type:    managedupgradev1beta1.UpgradeJobConditionSucceeded,
-			Status:  metav1.ConditionTrue,
-			Reason:  managedupgradev1beta1.UpgradeJobReasonSkipped,
-			Message: fmt.Sprintf("Upgrade job skipped by UpgradeSuspensionWindow %q, reason: %q", window.Name, window.Spec.Reason),
-		})
-		return ctrl.Result{}, r.Status().Update(ctx, &uj)
-	}
-
 	if now.After(uj.Spec.StartBefore.Time) {
+		skipped, err := r.checkAndMarkSkipped(ctx, uj, now)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+		if skipped {
+			return ctrl.Result{}, nil
+		}
+
 		r.setStatusCondition(&uj.Status.Conditions, metav1.Condition{
 			Type:    managedupgradev1beta1.UpgradeJobConditionFailed,
 			Status:  metav1.ConditionTrue,
@@ -148,6 +141,14 @@ func (r *UpgradeJobReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 
 	if !now.Before(uj.Spec.StartAfter.Time) {
+		skipped, err := r.checkAndMarkSkipped(ctx, uj, now)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+		if skipped {
+			return ctrl.Result{}, nil
+		}
+
 		r.setStatusCondition(&uj.Status.Conditions, metav1.Condition{
 			Type:    managedupgradev1beta1.UpgradeJobConditionStarted,
 			Status:  metav1.ConditionTrue,
@@ -1010,3 +1011,22 @@ func (r *UpgradeJobReconciler) matchingUpgradeSuspensionWindow(ctx context.Conte
 
 	return nil, nil
 }
+
+// checkAndMarkSkipped checks if the upgrade job should be skipped due to an UpgradeSuspensionWindow and marks it as skipped if necessary.
+func (r *UpgradeJobReconciler) checkAndMarkSkipped(ctx context.Context, uj managedupgradev1beta1.UpgradeJob, now time.Time) (skipped bool, err error) {
+	window, err := r.matchingUpgradeSuspensionWindow(ctx, uj, now)
+	if err != nil {
+		return true, fmt.Errorf("failed to search for matching upgrade suspension window: %w", err)
+	}
+	if window != nil {
+		log.FromContext(ctx).Info("Upgrade job skipped by UpgradeSuspensionWindow", "window", window.Name, "reason", window.Spec.Reason, "start", window.Spec.Start.Time, "end", window.Spec.End.Time)
+		r.setStatusCondition(&uj.Status.Conditions, metav1.Condition{
+			Type:    managedupgradev1beta1.UpgradeJobConditionSucceeded,
+			Status:  metav1.ConditionTrue,
+			Reason:  managedupgradev1beta1.UpgradeJobReasonSkipped,
+			Message: fmt.Sprintf("Upgrade job skipped by UpgradeSuspensionWindow %q, reason: %q", window.Name, window.Spec.Reason),
+		})
+		return true, r.Status().Update(ctx, &uj)
+	}
+	return false, nil
+}
diff --git a/controllers/upgradejob_controller_test.go b/controllers/upgradejob_controller_test.go
index f73d7b5..74ed142 100644
--- a/controllers/upgradejob_controller_test.go
+++ b/controllers/upgradejob_controller_test.go
@@ -406,7 +406,14 @@ func Test_UpgradeJobReconciler_Reconcile_Skipped_Job(t *testing.T) {
 		ManagedUpstreamClusterVersionName: "version",
 	}
 
+	step(t, "startAfter not yet reached, should not skip job yet", func(t *testing.T) {
+		reconcileNTimes(t, subject, ctx, requestForObject(upgradeJob), 10)
+		require.Nil(t, apimeta.FindStatusCondition(upgradeJob.Status.Conditions, managedupgradev1beta1.UpgradeJobConditionSucceeded))
+	})
+
 	step(t, "Upgrade Skipped because of window", func(t *testing.T) {
+		// Move to start after time
+		clock.Advance(time.Hour)
 		reconcileNTimes(t, subject, ctx, requestForObject(upgradeJob), 10)
 		require.NoError(t, c.Get(ctx, requestForObject(upgradeJob).NamespacedName, upgradeJob))
 		sc := apimeta.FindStatusCondition(upgradeJob.Status.Conditions, managedupgradev1beta1.UpgradeJobConditionSucceeded)