From e960221e4026ee04d6c209dcfa8a30a757631d86 Mon Sep 17 00:00:00 2001 From: Sebastian Widmer Date: Wed, 4 Oct 2023 13:35:07 +0200 Subject: [PATCH] Don't run hooks created after job is finished (#49) --- controllers/upgradejob_controller.go | 27 ++++++++++++++--------- controllers/upgradejob_controller_test.go | 16 ++++++++++++++ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/controllers/upgradejob_controller.go b/controllers/upgradejob_controller.go index 292926b..cec2b5e 100644 --- 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 index 4d1a47e..e2d78e7 100644 --- 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") }) }