From 0e171969e2513bdceaff30d91a9bc278ef145836 Mon Sep 17 00:00:00 2001 From: Alay Patel Date: Sun, 6 Dec 2020 23:06:47 -0500 Subject: [PATCH 1/4] refine the test case for time drift, calculate next schedule with math --- .../cronjob/cronjob_controller_test.go | 119 ++++++++++-------- .../cronjob/cronjob_controllerv2.go | 16 +-- .../cronjob/cronjob_controllerv2_test.go | 110 ++++++++-------- pkg/controller/cronjob/injection.go | 5 +- pkg/controller/cronjob/utils.go | 44 ++++--- pkg/controller/cronjob/utils_test.go | 119 +++++++++++++----- 6 files changed, 249 insertions(+), 164 deletions(-) diff --git a/pkg/controller/cronjob/cronjob_controller_test.go b/pkg/controller/cronjob/cronjob_controller_test.go index 66467ce71e1..a97f03c4c46 100644 --- a/pkg/controller/cronjob/cronjob_controller_test.go +++ b/pkg/controller/cronjob/cronjob_controller_test.go @@ -48,20 +48,29 @@ func justBeforeTheHour() time.Time { return T1 } -func topOfTheHour() time.Time { +func topOfTheHour() *time.Time { T1, err := time.Parse(time.RFC3339, "2016-05-19T10:00:00Z") if err != nil { panic("test setup error") } - return T1 + return &T1 } -func justAfterTheHour() time.Time { +func deltaTimeAfterTopOfTheHour(duration time.Duration) *time.Time { + T1, err := time.Parse(time.RFC3339, "2016-05-19T10:00:00Z") + if err != nil { + panic("test setup error") + } + t := T1.Add(duration) + return &t +} + +func justAfterTheHour() *time.Time { T1, err := time.Parse(time.RFC3339, "2016-05-19T10:01:00Z") if err != nil { panic("test setup error") } - return T1 + return &T1 } func weekAfterTheHour() time.Time { @@ -204,32 +213,32 @@ func TestSyncOne_RunOrNot(t *testing.T) { "never ran, not time, A": {A, F, onTheHour, noDead, F, F, justBeforeTheHour(), F, F, 0, 0}, "never ran, not time, F": {f, F, onTheHour, noDead, F, F, justBeforeTheHour(), F, F, 0, 0}, "never ran, not time, R": {R, F, onTheHour, noDead, F, F, justBeforeTheHour(), F, F, 0, 0}, - "never ran, is time, A": {A, F, onTheHour, noDead, F, F, justAfterTheHour(), T, F, 1, 0}, - "never ran, is time, F": {f, F, onTheHour, noDead, F, F, justAfterTheHour(), T, F, 1, 0}, - "never ran, is time, R": {R, F, onTheHour, noDead, F, F, justAfterTheHour(), T, F, 1, 0}, - "never ran, is time, suspended": {A, T, onTheHour, noDead, F, F, justAfterTheHour(), F, F, 0, 0}, - "never ran, is time, past deadline": {A, F, onTheHour, shortDead, F, F, justAfterTheHour(), F, F, 0, 0}, - "never ran, is time, not past deadline": {A, F, onTheHour, longDead, F, F, justAfterTheHour(), T, F, 1, 0}, + "never ran, is time, A": {A, F, onTheHour, noDead, F, F, *justAfterTheHour(), T, F, 1, 0}, + "never ran, is time, F": {f, F, onTheHour, noDead, F, F, *justAfterTheHour(), T, F, 1, 0}, + "never ran, is time, R": {R, F, onTheHour, noDead, F, F, *justAfterTheHour(), T, F, 1, 0}, + "never ran, is time, suspended": {A, T, onTheHour, noDead, F, F, *justAfterTheHour(), F, F, 0, 0}, + "never ran, is time, past deadline": {A, F, onTheHour, shortDead, F, F, *justAfterTheHour(), F, F, 0, 0}, + "never ran, is time, not past deadline": {A, F, onTheHour, longDead, F, F, *justAfterTheHour(), T, F, 1, 0}, "prev ran but done, not time, A": {A, F, onTheHour, noDead, T, F, justBeforeTheHour(), F, F, 0, 0}, "prev ran but done, not time, F": {f, F, onTheHour, noDead, T, F, justBeforeTheHour(), F, F, 0, 0}, "prev ran but done, not time, R": {R, F, onTheHour, noDead, T, F, justBeforeTheHour(), F, F, 0, 0}, - "prev ran but done, is time, A": {A, F, onTheHour, noDead, T, F, justAfterTheHour(), T, F, 1, 0}, - "prev ran but done, is time, F": {f, F, onTheHour, noDead, T, F, justAfterTheHour(), T, F, 1, 0}, - "prev ran but done, is time, R": {R, F, onTheHour, noDead, T, F, justAfterTheHour(), T, F, 1, 0}, - "prev ran but done, is time, suspended": {A, T, onTheHour, noDead, T, F, justAfterTheHour(), F, F, 0, 0}, - "prev ran but done, is time, past deadline": {A, F, onTheHour, shortDead, T, F, justAfterTheHour(), F, F, 0, 0}, - "prev ran but done, is time, not past deadline": {A, F, onTheHour, longDead, T, F, justAfterTheHour(), T, F, 1, 0}, + "prev ran but done, is time, A": {A, F, onTheHour, noDead, T, F, *justAfterTheHour(), T, F, 1, 0}, + "prev ran but done, is time, F": {f, F, onTheHour, noDead, T, F, *justAfterTheHour(), T, F, 1, 0}, + "prev ran but done, is time, R": {R, F, onTheHour, noDead, T, F, *justAfterTheHour(), T, F, 1, 0}, + "prev ran but done, is time, suspended": {A, T, onTheHour, noDead, T, F, *justAfterTheHour(), F, F, 0, 0}, + "prev ran but done, is time, past deadline": {A, F, onTheHour, shortDead, T, F, *justAfterTheHour(), F, F, 0, 0}, + "prev ran but done, is time, not past deadline": {A, F, onTheHour, longDead, T, F, *justAfterTheHour(), T, F, 1, 0}, "still active, not time, A": {A, F, onTheHour, noDead, T, T, justBeforeTheHour(), F, F, 1, 0}, "still active, not time, F": {f, F, onTheHour, noDead, T, T, justBeforeTheHour(), F, F, 1, 0}, "still active, not time, R": {R, F, onTheHour, noDead, T, T, justBeforeTheHour(), F, F, 1, 0}, - "still active, is time, A": {A, F, onTheHour, noDead, T, T, justAfterTheHour(), T, F, 2, 0}, - "still active, is time, F": {f, F, onTheHour, noDead, T, T, justAfterTheHour(), F, F, 1, 0}, - "still active, is time, R": {R, F, onTheHour, noDead, T, T, justAfterTheHour(), T, T, 1, 0}, - "still active, is time, suspended": {A, T, onTheHour, noDead, T, T, justAfterTheHour(), F, F, 1, 0}, - "still active, is time, past deadline": {A, F, onTheHour, shortDead, T, T, justAfterTheHour(), F, F, 1, 0}, - "still active, is time, not past deadline": {A, F, onTheHour, longDead, T, T, justAfterTheHour(), T, F, 2, 0}, + "still active, is time, A": {A, F, onTheHour, noDead, T, T, *justAfterTheHour(), T, F, 2, 0}, + "still active, is time, F": {f, F, onTheHour, noDead, T, T, *justAfterTheHour(), F, F, 1, 0}, + "still active, is time, R": {R, F, onTheHour, noDead, T, T, *justAfterTheHour(), T, T, 1, 0}, + "still active, is time, suspended": {A, T, onTheHour, noDead, T, T, *justAfterTheHour(), F, F, 1, 0}, + "still active, is time, past deadline": {A, F, onTheHour, shortDead, T, T, *justAfterTheHour(), F, F, 1, 0}, + "still active, is time, not past deadline": {A, F, onTheHour, longDead, T, T, *justAfterTheHour(), T, F, 2, 0}, // Controller should fail to schedule these, as there are too many missed starting times // and either no deadline or a too long deadline. @@ -628,13 +637,13 @@ func TestSyncOne_Status(t *testing.T) { "never ran, not time, A": {A, F, onTheHour, noDead, F, F, justBeforeTheHour(), F, F, F, F, F}, "never ran, not time, F": {f, F, onTheHour, noDead, F, F, justBeforeTheHour(), F, F, F, F, F}, "never ran, not time, R": {R, F, onTheHour, noDead, F, F, justBeforeTheHour(), F, F, F, F, F}, - "never ran, is time, A": {A, F, onTheHour, noDead, F, F, justAfterTheHour(), F, F, F, T, F}, - "never ran, is time, F": {f, F, onTheHour, noDead, F, F, justAfterTheHour(), F, F, F, T, F}, - "never ran, is time, R": {R, F, onTheHour, noDead, F, F, justAfterTheHour(), F, F, F, T, F}, - "never ran, is time, deleting": {A, F, onTheHour, noDead, F, F, justAfterTheHour(), F, F, T, F, F}, - "never ran, is time, suspended": {A, T, onTheHour, noDead, F, F, justAfterTheHour(), F, F, F, F, F}, - "never ran, is time, past deadline": {A, F, onTheHour, shortDead, F, F, justAfterTheHour(), F, F, F, F, F}, - "never ran, is time, not past deadline": {A, F, onTheHour, longDead, F, F, justAfterTheHour(), F, F, F, T, F}, + "never ran, is time, A": {A, F, onTheHour, noDead, F, F, *justAfterTheHour(), F, F, F, T, F}, + "never ran, is time, F": {f, F, onTheHour, noDead, F, F, *justAfterTheHour(), F, F, F, T, F}, + "never ran, is time, R": {R, F, onTheHour, noDead, F, F, *justAfterTheHour(), F, F, F, T, F}, + "never ran, is time, deleting": {A, F, onTheHour, noDead, F, F, *justAfterTheHour(), F, F, T, F, F}, + "never ran, is time, suspended": {A, T, onTheHour, noDead, F, F, *justAfterTheHour(), F, F, F, F, F}, + "never ran, is time, past deadline": {A, F, onTheHour, shortDead, F, F, *justAfterTheHour(), F, F, F, F, F}, + "never ran, is time, not past deadline": {A, F, onTheHour, longDead, F, F, *justAfterTheHour(), F, F, F, T, F}, "prev ran but done, not time, A": {A, F, onTheHour, noDead, T, F, justBeforeTheHour(), F, F, F, F, F}, "prev ran but done, not time, finished job, A": {A, F, onTheHour, noDead, T, T, justBeforeTheHour(), F, F, F, F, F}, @@ -649,31 +658,31 @@ func TestSyncOne_Status(t *testing.T) { "prev ran but done, not time, finished job, missing job, F": {f, F, onTheHour, noDead, T, T, justBeforeTheHour(), F, T, F, F, F}, "prev ran but done, not time, unexpected job, R": {R, F, onTheHour, noDead, T, F, justBeforeTheHour(), T, F, F, F, F}, - "prev ran but done, is time, A": {A, F, onTheHour, noDead, T, F, justAfterTheHour(), F, F, F, T, F}, - "prev ran but done, is time, finished job, A": {A, F, onTheHour, noDead, T, T, justAfterTheHour(), F, F, F, T, F}, - "prev ran but done, is time, unexpected job, A": {A, F, onTheHour, noDead, T, F, justAfterTheHour(), T, F, F, T, F}, - "prev ran but done, is time, finished job, unexpected job, A": {A, F, onTheHour, noDead, T, T, justAfterTheHour(), T, F, F, T, F}, - "prev ran but done, is time, F": {f, F, onTheHour, noDead, T, F, justAfterTheHour(), F, F, F, T, F}, - "prev ran but done, is time, finished job, F": {f, F, onTheHour, noDead, T, T, justAfterTheHour(), F, F, F, T, F}, - "prev ran but done, is time, unexpected job, F": {f, F, onTheHour, noDead, T, F, justAfterTheHour(), T, F, F, T, F}, - "prev ran but done, is time, finished job, unexpected job, F": {f, F, onTheHour, noDead, T, T, justAfterTheHour(), T, F, F, T, F}, - "prev ran but done, is time, R": {R, F, onTheHour, noDead, T, F, justAfterTheHour(), F, F, F, T, F}, - "prev ran but done, is time, finished job, R": {R, F, onTheHour, noDead, T, T, justAfterTheHour(), F, F, F, T, F}, - "prev ran but done, is time, unexpected job, R": {R, F, onTheHour, noDead, T, F, justAfterTheHour(), T, F, F, T, F}, - "prev ran but done, is time, finished job, unexpected job, R": {R, F, onTheHour, noDead, T, T, justAfterTheHour(), T, F, F, T, F}, - "prev ran but done, is time, deleting": {A, F, onTheHour, noDead, T, F, justAfterTheHour(), F, F, T, F, F}, - "prev ran but done, is time, suspended": {A, T, onTheHour, noDead, T, F, justAfterTheHour(), F, F, F, F, F}, - "prev ran but done, is time, finished job, suspended": {A, T, onTheHour, noDead, T, T, justAfterTheHour(), F, F, F, F, F}, - "prev ran but done, is time, unexpected job, suspended": {A, T, onTheHour, noDead, T, F, justAfterTheHour(), T, F, F, F, F}, - "prev ran but done, is time, finished job, unexpected job, suspended": {A, T, onTheHour, noDead, T, T, justAfterTheHour(), T, F, F, F, F}, - "prev ran but done, is time, past deadline": {A, F, onTheHour, shortDead, T, F, justAfterTheHour(), F, F, F, F, F}, - "prev ran but done, is time, finished job, past deadline": {A, F, onTheHour, shortDead, T, T, justAfterTheHour(), F, F, F, F, F}, - "prev ran but done, is time, unexpected job, past deadline": {A, F, onTheHour, shortDead, T, F, justAfterTheHour(), T, F, F, F, F}, - "prev ran but done, is time, finished job, unexpected job, past deadline": {A, F, onTheHour, shortDead, T, T, justAfterTheHour(), T, F, F, F, F}, - "prev ran but done, is time, not past deadline": {A, F, onTheHour, longDead, T, F, justAfterTheHour(), F, F, F, T, F}, - "prev ran but done, is time, finished job, not past deadline": {A, F, onTheHour, longDead, T, T, justAfterTheHour(), F, F, F, T, F}, - "prev ran but done, is time, unexpected job, not past deadline": {A, F, onTheHour, longDead, T, F, justAfterTheHour(), T, F, F, T, F}, - "prev ran but done, is time, finished job, unexpected job, not past deadline": {A, F, onTheHour, longDead, T, T, justAfterTheHour(), T, F, F, T, F}, + "prev ran but done, is time, A": {A, F, onTheHour, noDead, T, F, *justAfterTheHour(), F, F, F, T, F}, + "prev ran but done, is time, finished job, A": {A, F, onTheHour, noDead, T, T, *justAfterTheHour(), F, F, F, T, F}, + "prev ran but done, is time, unexpected job, A": {A, F, onTheHour, noDead, T, F, *justAfterTheHour(), T, F, F, T, F}, + "prev ran but done, is time, finished job, unexpected job, A": {A, F, onTheHour, noDead, T, T, *justAfterTheHour(), T, F, F, T, F}, + "prev ran but done, is time, F": {f, F, onTheHour, noDead, T, F, *justAfterTheHour(), F, F, F, T, F}, + "prev ran but done, is time, finished job, F": {f, F, onTheHour, noDead, T, T, *justAfterTheHour(), F, F, F, T, F}, + "prev ran but done, is time, unexpected job, F": {f, F, onTheHour, noDead, T, F, *justAfterTheHour(), T, F, F, T, F}, + "prev ran but done, is time, finished job, unexpected job, F": {f, F, onTheHour, noDead, T, T, *justAfterTheHour(), T, F, F, T, F}, + "prev ran but done, is time, R": {R, F, onTheHour, noDead, T, F, *justAfterTheHour(), F, F, F, T, F}, + "prev ran but done, is time, finished job, R": {R, F, onTheHour, noDead, T, T, *justAfterTheHour(), F, F, F, T, F}, + "prev ran but done, is time, unexpected job, R": {R, F, onTheHour, noDead, T, F, *justAfterTheHour(), T, F, F, T, F}, + "prev ran but done, is time, finished job, unexpected job, R": {R, F, onTheHour, noDead, T, T, *justAfterTheHour(), T, F, F, T, F}, + "prev ran but done, is time, deleting": {A, F, onTheHour, noDead, T, F, *justAfterTheHour(), F, F, T, F, F}, + "prev ran but done, is time, suspended": {A, T, onTheHour, noDead, T, F, *justAfterTheHour(), F, F, F, F, F}, + "prev ran but done, is time, finished job, suspended": {A, T, onTheHour, noDead, T, T, *justAfterTheHour(), F, F, F, F, F}, + "prev ran but done, is time, unexpected job, suspended": {A, T, onTheHour, noDead, T, F, *justAfterTheHour(), T, F, F, F, F}, + "prev ran but done, is time, finished job, unexpected job, suspended": {A, T, onTheHour, noDead, T, T, *justAfterTheHour(), T, F, F, F, F}, + "prev ran but done, is time, past deadline": {A, F, onTheHour, shortDead, T, F, *justAfterTheHour(), F, F, F, F, F}, + "prev ran but done, is time, finished job, past deadline": {A, F, onTheHour, shortDead, T, T, *justAfterTheHour(), F, F, F, F, F}, + "prev ran but done, is time, unexpected job, past deadline": {A, F, onTheHour, shortDead, T, F, *justAfterTheHour(), T, F, F, F, F}, + "prev ran but done, is time, finished job, unexpected job, past deadline": {A, F, onTheHour, shortDead, T, T, *justAfterTheHour(), T, F, F, F, F}, + "prev ran but done, is time, not past deadline": {A, F, onTheHour, longDead, T, F, *justAfterTheHour(), F, F, F, T, F}, + "prev ran but done, is time, finished job, not past deadline": {A, F, onTheHour, longDead, T, T, *justAfterTheHour(), F, F, F, T, F}, + "prev ran but done, is time, unexpected job, not past deadline": {A, F, onTheHour, longDead, T, F, *justAfterTheHour(), T, F, F, T, F}, + "prev ran but done, is time, finished job, unexpected job, not past deadline": {A, F, onTheHour, longDead, T, T, *justAfterTheHour(), T, F, F, T, F}, } for name, tc := range testCases { @@ -769,7 +778,7 @@ func TestSyncOne_Status(t *testing.T) { t.Errorf("%s: expected missing job to be removed from active list, actually active list = %#v", name, cjc.Updates[0].Status.Active) } - if tc.expectCreate && !cjc.Updates[1].Status.LastScheduleTime.Time.Equal(topOfTheHour()) { + if tc.expectCreate && !cjc.Updates[1].Status.LastScheduleTime.Time.Equal(*topOfTheHour()) { t.Errorf("%s: expected LastScheduleTime updated to %s, got %s", name, topOfTheHour(), cjc.Updates[1].Status.LastScheduleTime) } }) diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go index cb9f6037015..ab8525fd5ab 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2.go +++ b/pkg/controller/cronjob/cronjob_controllerv2.go @@ -477,8 +477,8 @@ func (jm *ControllerV2) syncCronJob( jm.recorder.Eventf(cj, corev1.EventTypeWarning, "UnparseableSchedule", "unparseable schedule: %s : %s", cj.Spec.Schedule, err) return cj, nil, nil } - times := getUnmetScheduleTimes(*cj, now, sched, jm.recorder) - if len(times) == 0 { + scheduledTime := getNextScheduleTime(*cj, now, sched, jm.recorder) + if scheduledTime == nil { // no unmet start time, return cj,. // The only time this should happen is if queue is filled after restart. // Otherwise, the queue is always suppose to trigger sync function at the time of @@ -488,7 +488,6 @@ func (jm *ControllerV2) syncCronJob( return cj, t, nil } - scheduledTime := times[len(times)-1] tooLate := false if cj.Spec.StartingDeadlineSeconds != nil { tooLate = scheduledTime.Add(time.Second * time.Duration(*cj.Spec.StartingDeadlineSeconds)).Before(now) @@ -509,9 +508,9 @@ func (jm *ControllerV2) syncCronJob( } if isJobInActiveList(&batchv1.Job{ ObjectMeta: metav1.ObjectMeta{ - Name: getJobName(cj, scheduledTime), + Name: getJobName(cj, *scheduledTime), Namespace: cj.Namespace, - }}, cj.Status.Active) || cj.Status.LastScheduleTime.Equal(&metav1.Time{Time: scheduledTime}) { + }}, cj.Status.Active) || cj.Status.LastScheduleTime.Equal(&metav1.Time{Time: *scheduledTime}) { klog.V(4).InfoS("Not starting job because the scheduled time is already processed", "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName()), "schedule", scheduledTime) t := nextScheduledTimeDuration(sched, now) return cj, t, nil @@ -546,7 +545,7 @@ func (jm *ControllerV2) syncCronJob( } } - jobReq, err := getJobFromTemplate2(cj, scheduledTime) + jobReq, err := getJobFromTemplate2(cj, *scheduledTime) if err != nil { klog.ErrorS(err, "Unable to make Job from template", "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName())) return cj, nil, err @@ -555,12 +554,15 @@ func (jm *ControllerV2) syncCronJob( switch { case errors.HasStatusCause(err, corev1.NamespaceTerminatingCause): case errors.IsAlreadyExists(err): + // If the job is created by other actor, assume it has updated the cronjob status accordingly klog.InfoS("Job already exists", "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName()), "job", klog.KRef(jobReq.GetNamespace(), jobReq.GetName())) + return cj, nil, err case err != nil: // default error handling jm.recorder.Eventf(cj, corev1.EventTypeWarning, "FailedCreate", "Error creating job: %v", err) return cj, nil, err } + klog.V(4).InfoS("Created Job", "job", klog.KRef(jobResp.GetNamespace(), jobResp.GetName()), "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName())) jm.recorder.Eventf(cj, corev1.EventTypeNormal, "SuccessfulCreate", "Created job %v", jobResp.Name) @@ -581,7 +583,7 @@ func (jm *ControllerV2) syncCronJob( return cj, nil, fmt.Errorf("unable to make object reference for job for %s", klog.KRef(cj.GetNamespace(), cj.GetName())) } cj.Status.Active = append(cj.Status.Active, *jobRef) - cj.Status.LastScheduleTime = &metav1.Time{Time: scheduledTime} + cj.Status.LastScheduleTime = &metav1.Time{Time: *scheduledTime} if _, err := jm.cronJobControl.UpdateStatus(cj); err != nil { klog.InfoS("Unable to update status", "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName()), "resourceVersion", cj.ResourceVersion, "err", err) return cj, nil, fmt.Errorf("unable to update status for %s (rv = %s): %v", klog.KRef(cj.GetNamespace(), cj.GetName()), cj.ResourceVersion, err) diff --git a/pkg/controller/cronjob/cronjob_controllerv2_test.go b/pkg/controller/cronjob/cronjob_controllerv2_test.go index e17c46f7ac8..7e0885adaa2 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2_test.go +++ b/pkg/controller/cronjob/cronjob_controllerv2_test.go @@ -18,18 +18,20 @@ package cronjob import ( "fmt" - "github.com/robfig/cron" - "k8s.io/apimachinery/pkg/labels" "reflect" "strings" "sync" "testing" "time" + "github.com/robfig/cron" batchv1 "k8s.io/api/batch/v1" batchv1beta1 "k8s.io/api/batch/v1beta1" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" batchv1listers "k8s.io/client-go/listers/batch/v1" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" @@ -45,7 +47,7 @@ func justASecondBeforeTheHour() time.Time { return T1 } -func Test_syncOne2(t *testing.T) { +func Test_syncCronJob(t *testing.T) { // Check expectations on deadline parameters if shortDead/60/60 >= 1 { t.Errorf("shortDead should be less than one hour") @@ -84,70 +86,72 @@ func Test_syncOne2(t *testing.T) { expectRequeueAfter bool jobStillNotFoundInLister bool jobPresentInCJActiveStatus bool + jobCreateError error }{ - "never ran, not valid schedule, A": {A, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F, T}, - "never ran, not valid schedule, F": {f, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F, T}, - "never ran, not valid schedule, R": {f, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F, T}, - "never ran, not time, A": {A, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, - "never ran, not time, F": {f, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, - "never ran, not time, R": {R, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, - "never ran, is time, A": {A, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "never ran, is time, F": {f, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "never ran, is time, R": {R, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "never ran, is time, suspended": {A, T, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, F, F, T}, - "never ran, is time, past deadline": {A, F, onTheHour, shortDead, F, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, T, F, T}, - "never ran, is time, not past deadline": {A, F, onTheHour, longDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "never ran, not valid schedule, A": {A, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F, T, nil}, + "never ran, not valid schedule, F": {f, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F, T, nil}, + "never ran, not valid schedule, R": {f, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F, T, nil}, + "never ran, not time, A": {A, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T, nil}, + "never ran, not time, F": {f, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T, nil}, + "never ran, not time, R": {R, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T, nil}, + "never ran, is time, A": {A, F, onTheHour, noDead, F, F, justAfterThePriorHour(), *justAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, + "never ran, is time, F": {f, F, onTheHour, noDead, F, F, justAfterThePriorHour(), *justAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, + "never ran, is time, R": {R, F, onTheHour, noDead, F, F, justAfterThePriorHour(), *justAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, + "never ran, is time, suspended": {A, T, onTheHour, noDead, F, F, justAfterThePriorHour(), *justAfterTheHour(), F, F, 0, 0, F, F, F, T, nil}, + "never ran, is time, past deadline": {A, F, onTheHour, shortDead, F, F, justAfterThePriorHour(), justAfterTheHour().Add(time.Minute * time.Duration(shortDead+1)), F, F, 0, 0, F, T, F, T, nil}, + "never ran, is time, not past deadline": {A, F, onTheHour, longDead, F, F, justAfterThePriorHour(), *justAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, - "prev ran but done, not time, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, - "prev ran but done, not time, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, - "prev ran but done, not time, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, - "prev ran but done, is time, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "prev ran but done, is time, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "prev ran but done, is time, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "prev ran but done, is time, suspended": {A, T, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, F, F, T}, - "prev ran but done, is time, past deadline": {A, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, T, F, T}, - "prev ran but done, is time, not past deadline": {A, F, onTheHour, longDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "prev ran but done, not time, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T, nil}, + "prev ran but done, not time, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T, nil}, + "prev ran but done, not time, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T, nil}, + "prev ran but done, is time, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), *justAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, + "prev ran but done, is time, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), *justAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, + "prev ran but done, is time, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), *justAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, + "prev ran but done, is time, suspended": {A, T, onTheHour, noDead, T, F, justAfterThePriorHour(), *justAfterTheHour(), F, F, 0, 0, F, F, F, T, nil}, + "prev ran but done, is time, past deadline": {A, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), *justAfterTheHour(), F, F, 0, 0, F, T, F, T, nil}, + "prev ran but done, is time, not past deadline": {A, F, onTheHour, longDead, T, F, justAfterThePriorHour(), *justAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, - "still active, not time, A": {A, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F, T}, - "still active, not time, F": {f, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F, T}, - "still active, not time, R": {R, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F, T}, - "still active, is time, A": {A, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), T, F, 2, 0, F, T, F, T}, - "still active, is time, F": {f, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), F, F, 1, 0, F, T, F, T}, - "still active, is time, R": {R, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), T, T, 1, 0, F, T, F, T}, - "still active, is time, suspended": {A, T, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), F, F, 1, 0, F, F, F, T}, - "still active, is time, past deadline": {A, F, onTheHour, shortDead, T, T, justAfterThePriorHour(), justAfterTheHour(), F, F, 1, 0, F, T, F, T}, - "still active, is time, not past deadline": {A, F, onTheHour, longDead, T, T, justAfterThePriorHour(), justAfterTheHour(), T, F, 2, 0, F, T, F, T}, + "still active, not time, A": {A, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F, T, nil}, + "still active, not time, F": {f, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F, T, nil}, + "still active, not time, R": {R, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F, T, nil}, + "still active, is time, A": {A, F, onTheHour, noDead, T, T, justAfterThePriorHour(), *justAfterTheHour(), T, F, 2, 0, F, T, F, T, nil}, + "still active, is time, F": {f, F, onTheHour, noDead, T, T, justAfterThePriorHour(), *justAfterTheHour(), F, F, 1, 0, F, T, F, T, nil}, + "still active, is time, R": {R, F, onTheHour, noDead, T, T, justAfterThePriorHour(), *justAfterTheHour(), T, T, 1, 0, F, T, F, T, nil}, + "still active, is time, suspended": {A, T, onTheHour, noDead, T, T, justAfterThePriorHour(), *justAfterTheHour(), F, F, 1, 0, F, F, F, T, nil}, + "still active, is time, past deadline": {A, F, onTheHour, shortDead, T, T, justAfterThePriorHour(), *justAfterTheHour(), F, F, 1, 0, F, T, F, T, nil}, + "still active, is time, not past deadline": {A, F, onTheHour, longDead, T, T, justAfterThePriorHour(), *justAfterTheHour(), T, F, 2, 0, F, T, F, T, nil}, // Controller should fail to schedule these, as there are too many missed starting times // and either no deadline or a too long deadline. - "prev ran but done, long overdue, not past deadline, A": {A, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, - "prev ran but done, long overdue, not past deadline, R": {R, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, - "prev ran but done, long overdue, not past deadline, F": {f, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, - "prev ran but done, long overdue, no deadline, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, - "prev ran but done, long overdue, no deadline, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, - "prev ran but done, long overdue, no deadline, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, + "prev ran but done, long overdue, not past deadline, A": {A, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T, nil}, + "prev ran but done, long overdue, not past deadline, R": {R, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T, nil}, + "prev ran but done, long overdue, not past deadline, F": {f, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T, nil}, + "prev ran but done, long overdue, no deadline, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T, nil}, + "prev ran but done, long overdue, no deadline, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T, nil}, + "prev ran but done, long overdue, no deadline, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T, nil}, - "prev ran but done, long overdue, past medium deadline, A": {A, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "prev ran but done, long overdue, past short deadline, A": {A, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "prev ran but done, long overdue, past medium deadline, A": {A, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, + "prev ran but done, long overdue, past short deadline, A": {A, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, - "prev ran but done, long overdue, past medium deadline, R": {R, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "prev ran but done, long overdue, past short deadline, R": {R, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "prev ran but done, long overdue, past medium deadline, R": {R, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, + "prev ran but done, long overdue, past short deadline, R": {R, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, - "prev ran but done, long overdue, past medium deadline, F": {f, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "prev ran but done, long overdue, past short deadline, F": {f, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "prev ran but done, long overdue, past medium deadline, F": {f, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, + "prev ran but done, long overdue, past short deadline, F": {f, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T, nil}, // Tests for time skews - "this ran but done, time drifted back, F": {f, F, onTheHour, noDead, T, F, justAfterTheHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, + // the controller sees job is created, takes no actions + "this ran but done, time drifted back, F": {f, F, onTheHour, noDead, T, F, *justAfterTheHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, F, errors.NewAlreadyExists(schema.GroupResource{Resource: "jobs", Group: "batch"}, "")}, // Tests for slow job lister - "this started but went missing, not past deadline, A": {A, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T, T}, - "this started but went missing, not past deadline, f": {f, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T, T}, - "this started but went missing, not past deadline, R": {R, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T, T}, + "this started but went missing, not past deadline, A": {A, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T, T, nil}, + "this started but went missing, not past deadline, f": {f, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T, T, nil}, + "this started but went missing, not past deadline, R": {R, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T, T, nil}, // Tests for slow cronjob list - "this started but is not present in cronjob active list, not past deadline, A": {A, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, F, F}, - "this started but is not present in cronjob active list, not past deadline, f": {f, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, F, F}, - "this started but is not present in cronjob active list, not past deadline, R": {R, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, F, F}, + "this started but is not present in cronjob active list, not past deadline, A": {A, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, F, F, nil}, + "this started but is not present in cronjob active list, not past deadline, f": {f, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, F, F, nil}, + "this started but is not present in cronjob active list, not past deadline, R": {R, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, F, F, nil}, } for name, tc := range testCases { name := name @@ -199,7 +203,7 @@ func Test_syncOne2(t *testing.T) { } } - jc := &fakeJobControl{Job: job} + jc := &fakeJobControl{Job: job, CreateErr: tc.jobCreateError} cjc := &fakeCJControl{CronJob: realCJ} recorder := record.NewFakeRecorder(10) diff --git a/pkg/controller/cronjob/injection.go b/pkg/controller/cronjob/injection.go index 0247550d80d..cbd925e5a37 100644 --- a/pkg/controller/cronjob/injection.go +++ b/pkg/controller/cronjob/injection.go @@ -148,6 +148,7 @@ type fakeJobControl struct { Jobs []batchv1.Job DeleteJobName []string Err error + CreateErr error UpdateJobName []string PatchJobName []string Patches [][]byte @@ -158,8 +159,8 @@ var _ jobControlInterface = &fakeJobControl{} func (f *fakeJobControl) CreateJob(namespace string, job *batchv1.Job) (*batchv1.Job, error) { f.Lock() defer f.Unlock() - if f.Err != nil { - return nil, f.Err + if f.CreateErr != nil { + return nil, f.CreateErr } job.SelfLink = fmt.Sprintf("/apis/batch/v1/namespaces/%s/jobs/%s", namespace, job.Name) f.Jobs = append(f.Jobs, *job) diff --git a/pkg/controller/cronjob/utils.go b/pkg/controller/cronjob/utils.go index e78bc6d9a0d..b53f5c9915d 100644 --- a/pkg/controller/cronjob/utils.go +++ b/pkg/controller/cronjob/utils.go @@ -150,12 +150,11 @@ func getRecentUnmetScheduleTimes(cj batchv1beta1.CronJob, now time.Time) ([]time return starts, nil } -// getUnmetScheduleTimes gets the slice of all the missed times from the time a job -// last was scheduled to up `now`. -// +// getNextScheduleTime gets the time of next schedule after last scheduled and before now +// it returns nil if no unmet schedule times. // If there are too many (>100) unstarted times, it will raise a warning and but still return // the list of missed times. -func getUnmetScheduleTimes(cj batchv1beta1.CronJob, now time.Time, schedule cron.Schedule, recorder record.EventRecorder) []time.Time { +func getNextScheduleTime(cj batchv1beta1.CronJob, now time.Time, schedule cron.Schedule, recorder record.EventRecorder) *time.Time { starts := []time.Time{} var earliestTime time.Time @@ -179,19 +178,12 @@ func getUnmetScheduleTimes(cj batchv1beta1.CronJob, now time.Time, schedule cron } } if earliestTime.After(now) { - return []time.Time{} + return nil } - // t := schedule.Next(earliestTime) - // t1 := schedule.Next(t) - // delta := t1 - t - // missed := now - earliestTime/delta - // last missed = earliestTime + delta * (missed - 1) - // TODO: @alpatel, convert the following for loop into above logic and add test cases - for t := schedule.Next(earliestTime); !t.After(now); t = schedule.Next(t) { - starts = append(starts, t) - } - if len(starts) > 100 { + t, numberOfMissedSchedules := getMostRecentScheduleTime(earliestTime, now, schedule) + + if numberOfMissedSchedules > 100 { // An object might miss several starts. For example, if // controller gets wedged on friday at 5:01pm when everyone has // gone home, and someone comes in on tuesday AM and discovers @@ -212,7 +204,27 @@ func getUnmetScheduleTimes(cj batchv1beta1.CronJob, now time.Time, schedule cron recorder.Eventf(&cj, corev1.EventTypeWarning, "TooManyMissedTimes", "too many missed start times: %d. Set or decrease .spec.startingDeadlineSeconds or check clock skew", len(starts)) klog.InfoS("too many missed times", "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName()), "missed times", len(starts)) } - return starts + return t +} + +// getMostRecentScheduleTime returns the latest schedule time between earliestTime and the count of number of +// schedules in between them +func getMostRecentScheduleTime(earliestTime time.Time, now time.Time, schedule cron.Schedule) (*time.Time, int64) { + t1 := schedule.Next(earliestTime) + t2 := schedule.Next(t1) + + if now.Before(t1) { + return nil, 0 + } + if now.Before(t2) { + return &t1, 1 + } + + timeBetweenTwoSchedules := int64(t2.Sub(t1).Seconds()) + timeElapsed := int64(now.Sub(t1).Seconds()) + numberOfMissedSchedules := (timeElapsed / timeBetweenTwoSchedules) + 1 + t := time.Unix(t1.Unix()+((numberOfMissedSchedules-1)*timeBetweenTwoSchedules), 0).UTC() + return &t, numberOfMissedSchedules } // getJobFromTemplate makes a Job from a CronJob diff --git a/pkg/controller/cronjob/utils_test.go b/pkg/controller/cronjob/utils_test.go index fb88926a21a..7e3c1921dc7 100644 --- a/pkg/controller/cronjob/utils_test.go +++ b/pkg/controller/cronjob/utils_test.go @@ -302,7 +302,7 @@ func TestGroupJobsByParent(t *testing.T) { } } -func TestGetLatestUnmetScheduleTimes(t *testing.T) { +func TestGetNextScheduleTime(t *testing.T) { // schedule is hourly on the hour schedule := "0 * * * ?" @@ -344,9 +344,9 @@ func TestGetLatestUnmetScheduleTimes(t *testing.T) { cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: T1.Add(-10 * time.Minute)} // Current time is more than creation time, but less than T1. now := T1.Add(-7 * time.Minute) - times := getUnmetScheduleTimes(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) - if len(times) != 0 { - t.Errorf("expected no start times, got: %v", times) + schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + if schedule != nil { + t.Errorf("expected no start time, got: %v", schedule) } } { @@ -355,11 +355,11 @@ func TestGetLatestUnmetScheduleTimes(t *testing.T) { cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: T1.Add(-10 * time.Minute)} // Current time is after T1 now := T1.Add(2 * time.Second) - times := getUnmetScheduleTimes(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) - if len(times) != 1 { - t.Errorf("expected 1 start time, got: %v", times) - } else if !times[0].Equal(T1) { - t.Errorf("expected: %v, got: %v", T1, times[0]) + schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + if schedule == nil { + t.Errorf("expected 1 start time, got nil") + } else if !schedule.Equal(T1) { + t.Errorf("expected: %v, got: %v", T1, schedule) } } { @@ -370,9 +370,9 @@ func TestGetLatestUnmetScheduleTimes(t *testing.T) { cj.Status.LastScheduleTime = &metav1.Time{Time: T1} // Current time is after T1 now := T1.Add(2 * time.Minute) - times := getUnmetScheduleTimes(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) - if len(times) != 0 { - t.Errorf("expected 0 start times, got: %v", times) + schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + if schedule != nil { + t.Errorf("expected 0 start times, got: %v", schedule) } } { @@ -383,11 +383,11 @@ func TestGetLatestUnmetScheduleTimes(t *testing.T) { cj.Status.LastScheduleTime = &metav1.Time{Time: T1} // Current time is after T1 and after T2 now := T2.Add(5 * time.Minute) - times := getUnmetScheduleTimes(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) - if len(times) != 1 { - t.Errorf("expected 1 start times, got: %v", times) - } else if !times[0].Equal(T2) { - t.Errorf("expected: %v, got: %v", T1, times[0]) + schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + if schedule == nil { + t.Errorf("expected 1 start times, got nil") + } else if !schedule.Equal(T2) { + t.Errorf("expected: %v, got: %v", T2, schedule) } } { @@ -396,16 +396,11 @@ func TestGetLatestUnmetScheduleTimes(t *testing.T) { cj.Status.LastScheduleTime = &metav1.Time{Time: T1.Add(-1 * time.Hour)} // Current time is after T1 and after T2 now := T2.Add(5 * time.Minute) - times := getUnmetScheduleTimes(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) - if len(times) != 2 { - t.Errorf("expected 2 start times, got: %v", times) - } else { - if !times[0].Equal(T1) { - t.Errorf("expected: %v, got: %v", T1, times[0]) - } - if !times[1].Equal(T2) { - t.Errorf("expected: %v, got: %v", T2, times[1]) - } + schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + if schedule == nil { + t.Errorf("expected 1 start times, got nil") + } else if !schedule.Equal(T2) { + t.Errorf("expected: %v, got: %v", T2, schedule) } } { @@ -413,8 +408,8 @@ func TestGetLatestUnmetScheduleTimes(t *testing.T) { cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: T1.Add(-2 * time.Hour)} cj.Status.LastScheduleTime = &metav1.Time{Time: T1.Add(-1 * time.Hour)} now := T2.Add(10 * 24 * time.Hour) - times := getUnmetScheduleTimes(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) - if len(times) == 0 { + schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + if schedule == nil { t.Errorf("expected more than 0 missed times") } } @@ -426,8 +421,8 @@ func TestGetLatestUnmetScheduleTimes(t *testing.T) { // Deadline is short deadline := int64(2 * 60 * 60) cj.Spec.StartingDeadlineSeconds = &deadline - times := getUnmetScheduleTimes(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) - if len(times) == 0 { + schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + if schedule == nil { t.Errorf("expected more than 0 missed times") } } @@ -631,3 +626,65 @@ func TestByJobStartTime(t *testing.T) { } } } + +func TestGetMostRecentScheduleTime(t *testing.T) { + type args struct { + earliestTime *time.Time + now time.Time + schedule string + } + tests := []struct { + name string + args args + expectedTime *time.Time + expectedNumberOfMisses int64 + }{ + { + name: "now before next schedule", + args: args{ + earliestTime: topOfTheHour(), + now: topOfTheHour().Add(time.Second * 30), + schedule: "0 * * * *", + }, + expectedTime: nil, + }, + { + name: "now just after next schedule", + args: args{ + earliestTime: topOfTheHour(), + now: topOfTheHour().Add(time.Second * 61), + schedule: "0 * * * *", + }, + expectedTime: justAfterTheHour(), + expectedNumberOfMisses: 1, + }, + { + name: "missed 5 schedules", + args: args{ + earliestTime: deltaTimeAfterTopOfTheHour(time.Second * 10), + now: *deltaTimeAfterTopOfTheHour(time.Minute * 5), + schedule: "0 * * * *", + }, + expectedTime: deltaTimeAfterTopOfTheHour(time.Minute * 5), + expectedNumberOfMisses: 5, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sched, err := cron.Parse(tt.args.schedule) + if err != nil { + t.Errorf("error setting up the test") + } + gotTime, gotNumberOfMisses := getMostRecentScheduleTime(*tt.args.earliestTime, tt.args.now, sched) + if gotTime == nil && tt.expectedTime != nil { + t.Errorf("getMostRecentScheduleTime() got nil, want %v", tt.expectedTime) + } + if gotTime != nil && tt.expectedTime != nil && !gotTime.Equal(*tt.expectedTime) { + t.Errorf("getMostRecentScheduleTime() got = %v, want %v", gotTime, tt.expectedTime) + } + if gotNumberOfMisses != tt.expectedNumberOfMisses { + t.Errorf("getMostRecentScheduleTime() got1 = %v, want %v", gotNumberOfMisses, tt.expectedNumberOfMisses) + } + }) + } +} From 4d7d14d24957999940210c0a3a453e422107ed5e Mon Sep 17 00:00:00 2001 From: Alay Patel Date: Sun, 6 Dec 2020 23:07:37 -0500 Subject: [PATCH 2/4] update bazel --- pkg/controller/cronjob/BUILD | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/controller/cronjob/BUILD b/pkg/controller/cronjob/BUILD index b60eae877d3..4f4c0729e08 100644 --- a/pkg/controller/cronjob/BUILD +++ b/pkg/controller/cronjob/BUILD @@ -61,8 +61,10 @@ go_test( "//staging/src/k8s.io/api/batch/v1:go_default_library", "//staging/src/k8s.io/api/batch/v1beta1:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/client-go/listers/batch/v1:go_default_library", From 6290a23ceb59e2fe79eff14555cf88b455c9942b Mon Sep 17 00:00:00 2001 From: Alay Patel Date: Mon, 22 Feb 2021 21:40:05 -0500 Subject: [PATCH 3/4] cronjob_controllerv2: gracefully handle 0 seconds between schedules --- .../cronjob/cronjob_controllerv2.go | 9 ++++++- pkg/controller/cronjob/utils.go | 24 ++++++++++++------- pkg/controller/cronjob/utils_test.go | 18 +++++++------- 3 files changed, 32 insertions(+), 19 deletions(-) diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go index ab8525fd5ab..765d7a445c1 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2.go +++ b/pkg/controller/cronjob/cronjob_controllerv2.go @@ -477,7 +477,14 @@ func (jm *ControllerV2) syncCronJob( jm.recorder.Eventf(cj, corev1.EventTypeWarning, "UnparseableSchedule", "unparseable schedule: %s : %s", cj.Spec.Schedule, err) return cj, nil, nil } - scheduledTime := getNextScheduleTime(*cj, now, sched, jm.recorder) + scheduledTime, err := getNextScheduleTime(*cj, now, sched, jm.recorder) + if err != nil { + // this is likely a user error in defining the spec value + // we should log the error and not reconcile this cronjob until an update to spec + klog.V(2).InfoS("invalid schedule", "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName()), "schedule", cj.Spec.Schedule, "err", err) + jm.recorder.Eventf(cj, corev1.EventTypeWarning, "InvalidSchedule", "invalid schedule schedule: %s : %s", cj.Spec.Schedule, err) + return cj, nil, nil + } if scheduledTime == nil { // no unmet start time, return cj,. // The only time this should happen is if queue is filled after restart. diff --git a/pkg/controller/cronjob/utils.go b/pkg/controller/cronjob/utils.go index b53f5c9915d..d73842c9610 100644 --- a/pkg/controller/cronjob/utils.go +++ b/pkg/controller/cronjob/utils.go @@ -154,7 +154,7 @@ func getRecentUnmetScheduleTimes(cj batchv1beta1.CronJob, now time.Time) ([]time // it returns nil if no unmet schedule times. // If there are too many (>100) unstarted times, it will raise a warning and but still return // the list of missed times. -func getNextScheduleTime(cj batchv1beta1.CronJob, now time.Time, schedule cron.Schedule, recorder record.EventRecorder) *time.Time { +func getNextScheduleTime(cj batchv1beta1.CronJob, now time.Time, schedule cron.Schedule, recorder record.EventRecorder) (*time.Time, error) { starts := []time.Time{} var earliestTime time.Time @@ -178,10 +178,10 @@ func getNextScheduleTime(cj batchv1beta1.CronJob, now time.Time, schedule cron.S } } if earliestTime.After(now) { - return nil + return nil, nil } - t, numberOfMissedSchedules := getMostRecentScheduleTime(earliestTime, now, schedule) + t, numberOfMissedSchedules, err := getMostRecentScheduleTime(earliestTime, now, schedule) if numberOfMissedSchedules > 100 { // An object might miss several starts. For example, if @@ -204,27 +204,33 @@ func getNextScheduleTime(cj batchv1beta1.CronJob, now time.Time, schedule cron.S recorder.Eventf(&cj, corev1.EventTypeWarning, "TooManyMissedTimes", "too many missed start times: %d. Set or decrease .spec.startingDeadlineSeconds or check clock skew", len(starts)) klog.InfoS("too many missed times", "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName()), "missed times", len(starts)) } - return t + return t, err } // getMostRecentScheduleTime returns the latest schedule time between earliestTime and the count of number of // schedules in between them -func getMostRecentScheduleTime(earliestTime time.Time, now time.Time, schedule cron.Schedule) (*time.Time, int64) { +func getMostRecentScheduleTime(earliestTime time.Time, now time.Time, schedule cron.Schedule) (*time.Time, int64, error) { t1 := schedule.Next(earliestTime) t2 := schedule.Next(t1) if now.Before(t1) { - return nil, 0 + return nil, 0, nil } if now.Before(t2) { - return &t1, 1 + return &t1, 1, nil } - timeBetweenTwoSchedules := int64(t2.Sub(t1).Seconds()) + // It is possible for cron.ParseStandard("59 23 31 2 *") to return an invalid schedule + // seconds - 59, minute - 23, hour - 31 (?!) dom - 2, and dow is optional, clearly 31 is invalid + // In this case the timeBetweenTwoSchedules will be 0, and we error out the invalid schedule + timeBetweenTwoSchedules := int64(t2.Sub(t1).Round(time.Second).Seconds()) + if timeBetweenTwoSchedules < 1 { + return nil, 0, fmt.Errorf("time difference between two schedules less than 1 second") + } timeElapsed := int64(now.Sub(t1).Seconds()) numberOfMissedSchedules := (timeElapsed / timeBetweenTwoSchedules) + 1 t := time.Unix(t1.Unix()+((numberOfMissedSchedules-1)*timeBetweenTwoSchedules), 0).UTC() - return &t, numberOfMissedSchedules + return &t, numberOfMissedSchedules, nil } // getJobFromTemplate makes a Job from a CronJob diff --git a/pkg/controller/cronjob/utils_test.go b/pkg/controller/cronjob/utils_test.go index 7e3c1921dc7..7afb581ea02 100644 --- a/pkg/controller/cronjob/utils_test.go +++ b/pkg/controller/cronjob/utils_test.go @@ -344,7 +344,7 @@ func TestGetNextScheduleTime(t *testing.T) { cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: T1.Add(-10 * time.Minute)} // Current time is more than creation time, but less than T1. now := T1.Add(-7 * time.Minute) - schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + schedule, _ := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule != nil { t.Errorf("expected no start time, got: %v", schedule) } @@ -355,7 +355,7 @@ func TestGetNextScheduleTime(t *testing.T) { cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: T1.Add(-10 * time.Minute)} // Current time is after T1 now := T1.Add(2 * time.Second) - schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + schedule, _ := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule == nil { t.Errorf("expected 1 start time, got nil") } else if !schedule.Equal(T1) { @@ -370,7 +370,7 @@ func TestGetNextScheduleTime(t *testing.T) { cj.Status.LastScheduleTime = &metav1.Time{Time: T1} // Current time is after T1 now := T1.Add(2 * time.Minute) - schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + schedule, _ := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule != nil { t.Errorf("expected 0 start times, got: %v", schedule) } @@ -383,7 +383,7 @@ func TestGetNextScheduleTime(t *testing.T) { cj.Status.LastScheduleTime = &metav1.Time{Time: T1} // Current time is after T1 and after T2 now := T2.Add(5 * time.Minute) - schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + schedule, _ := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule == nil { t.Errorf("expected 1 start times, got nil") } else if !schedule.Equal(T2) { @@ -396,7 +396,7 @@ func TestGetNextScheduleTime(t *testing.T) { cj.Status.LastScheduleTime = &metav1.Time{Time: T1.Add(-1 * time.Hour)} // Current time is after T1 and after T2 now := T2.Add(5 * time.Minute) - schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + schedule, _ := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule == nil { t.Errorf("expected 1 start times, got nil") } else if !schedule.Equal(T2) { @@ -408,7 +408,7 @@ func TestGetNextScheduleTime(t *testing.T) { cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: T1.Add(-2 * time.Hour)} cj.Status.LastScheduleTime = &metav1.Time{Time: T1.Add(-1 * time.Hour)} now := T2.Add(10 * 24 * time.Hour) - schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + schedule, _ := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule == nil { t.Errorf("expected more than 0 missed times") } @@ -421,7 +421,7 @@ func TestGetNextScheduleTime(t *testing.T) { // Deadline is short deadline := int64(2 * 60 * 60) cj.Spec.StartingDeadlineSeconds = &deadline - schedule := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) + schedule, _ := getNextScheduleTime(cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule == nil { t.Errorf("expected more than 0 missed times") } @@ -673,9 +673,9 @@ func TestGetMostRecentScheduleTime(t *testing.T) { t.Run(tt.name, func(t *testing.T) { sched, err := cron.Parse(tt.args.schedule) if err != nil { - t.Errorf("error setting up the test") + t.Errorf("error setting up the test, %s", err) } - gotTime, gotNumberOfMisses := getMostRecentScheduleTime(*tt.args.earliestTime, tt.args.now, sched) + gotTime, gotNumberOfMisses, _ := getMostRecentScheduleTime(*tt.args.earliestTime, tt.args.now, sched) if gotTime == nil && tt.expectedTime != nil { t.Errorf("getMostRecentScheduleTime() got nil, want %v", tt.expectedTime) } From cebbc6b48740756b4ddbbcd948f0e138dc620358 Mon Sep 17 00:00:00 2001 From: Alay Patel Date: Wed, 24 Feb 2021 13:50:03 -0500 Subject: [PATCH 4/4] cronjob: use the same schedule Parser for tests as the reconcile loop --- pkg/controller/cronjob/utils_test.go | 33 +++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/pkg/controller/cronjob/utils_test.go b/pkg/controller/cronjob/utils_test.go index 7afb581ea02..4e70cbc91cd 100644 --- a/pkg/controller/cronjob/utils_test.go +++ b/pkg/controller/cronjob/utils_test.go @@ -638,6 +638,7 @@ func TestGetMostRecentScheduleTime(t *testing.T) { args args expectedTime *time.Time expectedNumberOfMisses int64 + wantErr bool }{ { name: "now before next schedule", @@ -652,30 +653,50 @@ func TestGetMostRecentScheduleTime(t *testing.T) { name: "now just after next schedule", args: args{ earliestTime: topOfTheHour(), - now: topOfTheHour().Add(time.Second * 61), + now: topOfTheHour().Add(time.Minute * 61), schedule: "0 * * * *", }, - expectedTime: justAfterTheHour(), + expectedTime: deltaTimeAfterTopOfTheHour(time.Minute * 60), expectedNumberOfMisses: 1, }, { name: "missed 5 schedules", args: args{ earliestTime: deltaTimeAfterTopOfTheHour(time.Second * 10), - now: *deltaTimeAfterTopOfTheHour(time.Minute * 5), + now: *deltaTimeAfterTopOfTheHour(time.Minute * 301), schedule: "0 * * * *", }, - expectedTime: deltaTimeAfterTopOfTheHour(time.Minute * 5), + expectedTime: deltaTimeAfterTopOfTheHour(time.Minute * 300), expectedNumberOfMisses: 5, }, + { + name: "rogue cronjob", + args: args{ + earliestTime: deltaTimeAfterTopOfTheHour(time.Second * 10), + now: *deltaTimeAfterTopOfTheHour(time.Hour * 1000000), + schedule: "59 23 31 2 *", + }, + expectedTime: nil, + expectedNumberOfMisses: 0, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - sched, err := cron.Parse(tt.args.schedule) + sched, err := cron.ParseStandard(tt.args.schedule) if err != nil { t.Errorf("error setting up the test, %s", err) } - gotTime, gotNumberOfMisses, _ := getMostRecentScheduleTime(*tt.args.earliestTime, tt.args.now, sched) + gotTime, gotNumberOfMisses, err := getMostRecentScheduleTime(*tt.args.earliestTime, tt.args.now, sched) + if tt.wantErr { + if err == nil { + t.Error("getMostRecentScheduleTime() got no error when expected one") + } + return + } + if !tt.wantErr && err != nil { + t.Error("getMostRecentScheduleTime() got error when none expected") + } if gotTime == nil && tt.expectedTime != nil { t.Errorf("getMostRecentScheduleTime() got nil, want %v", tt.expectedTime) }