Don't run hooks created after job is finished (#49)

appuio · Oct 4, 2023 · e960221 · e960221
1 parent c6fbedd
commit e960221
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 10 deletions.
diff --git a/controllers/upgradejob_controller.go b/controllers/upgradejob_controller.go
@@ -87,20 +87,24 @@ func (r *UpgradeJobReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, nil
 	}
 
-	if apimeta.IsStatusConditionTrue(uj.Status.Conditions, managedupgradev1beta1.UpgradeJobConditionSucceeded) {
+	sc := apimeta.FindStatusCondition(uj.Status.Conditions, managedupgradev1beta1.UpgradeJobConditionSucceeded)
+	if sc != nil && sc.Status == metav1.ConditionTrue {
 		// Ignore hooks status, they can't influence the upgrade anymore.
-		_, eserr := r.executeHooks(ctx, &uj, managedupgradev1beta1.EventSuccess)
-		_, eferr := r.executeHooks(ctx, &uj, managedupgradev1beta1.EventFinish)
+		// Don't execute hooks created after the job was finished.
+		_, eserr := r.executeHooks(ctx, &uj, managedupgradev1beta1.EventSuccess, sc.LastTransitionTime.Time)
+		_, eferr := r.executeHooks(ctx, &uj, managedupgradev1beta1.EventFinish, sc.LastTransitionTime.Time)
 		return ctrl.Result{}, multierr.Combine(eserr, eferr, r.cleanupLock(ctx, &uj))
 	}
-	if apimeta.IsStatusConditionTrue(uj.Status.Conditions, managedupgradev1beta1.UpgradeJobConditionFailed) {
+	fc := apimeta.FindStatusCondition(uj.Status.Conditions, managedupgradev1beta1.UpgradeJobConditionFailed)
+	if fc != nil && fc.Status == metav1.ConditionTrue {
 		// Ignore hooks status, they can't influence the upgrade anymore.
-		_, efaerr := r.executeHooks(ctx, &uj, managedupgradev1beta1.EventFailure)
-		_, efierr := r.executeHooks(ctx, &uj, managedupgradev1beta1.EventFinish)
+		// Don't execute hooks created after the job was finished.
+		_, efaerr := r.executeHooks(ctx, &uj, managedupgradev1beta1.EventFailure, fc.LastTransitionTime.Time)
+		_, efierr := r.executeHooks(ctx, &uj, managedupgradev1beta1.EventFinish, fc.LastTransitionTime.Time)
 		return ctrl.Result{}, multierr.Combine(efaerr, efierr, r.cleanupLock(ctx, &uj))
 	}
 
-	cont, err := r.executeHooks(ctx, &uj, managedupgradev1beta1.EventCreate)
+	cont, err := r.executeHooks(ctx, &uj, managedupgradev1beta1.EventCreate, time.Time{})
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -142,7 +146,7 @@ func (r *UpgradeJobReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 func (r *UpgradeJobReconciler) reconcileStartedJob(ctx context.Context, uj *managedupgradev1beta1.UpgradeJob) (ctrl.Result, error) {
 	l := log.FromContext(ctx).WithName("UpgradeJobReconciler.reconcileStartedJob")
 
-	cont, err := r.executeHooks(ctx, uj, managedupgradev1beta1.EventStart)
+	cont, err := r.executeHooks(ctx, uj, managedupgradev1beta1.EventStart, time.Time{})
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -253,7 +257,7 @@ func (r *UpgradeJobReconciler) reconcileStartedJob(ctx context.Context, uj *mana
 		return ctrl.Result{}, nil
 	}
 
-	cont, err = r.executeHooks(ctx, uj, managedupgradev1beta1.EventUpgradeComplete)
+	cont, err = r.executeHooks(ctx, uj, managedupgradev1beta1.EventUpgradeComplete, time.Time{})
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -417,7 +421,7 @@ func (r *UpgradeJobReconciler) tryLockClusterVersion(ctx context.Context, versio
 	return nil
 }
 
-func (r *UpgradeJobReconciler) executeHooks(ctx context.Context, uj *managedupgradev1beta1.UpgradeJob, event managedupgradev1beta1.UpgradeEvent) (bool, error) {
+func (r *UpgradeJobReconciler) executeHooks(ctx context.Context, uj *managedupgradev1beta1.UpgradeJob, event managedupgradev1beta1.UpgradeEvent, cutoffTime time.Time) (bool, error) {
 	l := log.FromContext(ctx)
 
 	var allHooks managedupgradev1beta1.UpgradeJobHookList
@@ -430,6 +434,9 @@ func (r *UpgradeJobReconciler) executeHooks(ctx context.Context, uj *managedupgr
 		if !slices.Contains(hook.Spec.Events, event) {
 			continue
 		}
+		if !cutoffTime.IsZero() && hook.CreationTimestamp.After(cutoffTime) {
+			continue
+		}
 		sel, err := metav1.LabelSelectorAsSelector(&hook.Spec.Selector)
 		if err != nil {
 			l.Error(err, "failed to parse hook selector")

diff --git a/controllers/upgradejob_controller_test.go b/controllers/upgradejob_controller_test.go
@@ -279,6 +279,22 @@ func Test_UpgradeJobReconciler_Reconcile_E2E_Upgrade(t *testing.T) {
 	step(t, "`Success` and `Finish` hooks", func(t *testing.T) {
 		checkAndCompleteHook(t, c, subject, upgradeJob, upgradeJobHook, managedupgradev1beta1.EventSuccess, true)
 		checkAndCompleteHook(t, c, subject, upgradeJob, upgradeJobHook, managedupgradev1beta1.EventFinish, true)
+
+		clock.Advance(24 * time.Hour)
+
+		var jobs batchv1.JobList
+		require.NoError(t, c.List(ctx, &jobs))
+		prevJobs := len(jobs.Items)
+		upgradeJobHook2 := upgradeJobHook.DeepCopy()
+		upgradeJobHook2.ObjectMeta = metav1.ObjectMeta{
+			Name:              "notify2",
+			Namespace:         upgradeJobHook.Namespace,
+			CreationTimestamp: metav1.NewTime(clock.Now()),
+		}
+		require.NoError(t, c.Create(ctx, upgradeJobHook2))
+		reconcileNTimes(t, subject, ctx, requestForObject(upgradeJob), 3)
+		require.NoError(t, c.List(ctx, &jobs))
+		require.Equal(t, prevJobs, len(jobs.Items), "should have not created new jobs for hooks created after the upgrade job finished")
 	})
 }