From be44d67566ac87e85d450227765cd86df1ba9bd6 Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Tue, 28 Jun 2022 17:02:12 +0200 Subject: [PATCH] Re-use common parts between getNextScheduleTime and nextScheduledTimeDuration The two methods nextScheduledTimeDuration and getNextScheduleTime have a lot of similarities, so this commit squashes the common parts together along with getMostRecentScheduleTime to avoid code duplication. --- .../cronjob/cronjob_controllerv2.go | 38 +-- pkg/controller/cronjob/utils.go | 113 ++++---- pkg/controller/cronjob/utils_test.go | 241 +++++++++++++----- 3 files changed, 242 insertions(+), 150 deletions(-) diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go index 4d5a6c6a204..cb706bbae45 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2.go +++ b/pkg/controller/cronjob/cronjob_controllerv2.go @@ -398,7 +398,7 @@ func (jm *ControllerV2) updateCronJob(old interface{}, curr interface{}) { return } now := jm.now() - t := nextScheduledTimeDuration(*newCJ, sched, now) + t := nextScheduleTimeDuration(newCJ, now, sched) jm.enqueueControllerAfter(curr, *t) return @@ -517,7 +517,7 @@ func (jm *ControllerV2) syncCronJob( return cronJob, nil, updateStatus, nil } - scheduledTime, err := getNextScheduleTime(*cronJob, now, sched, jm.recorder) + scheduledTime, err := nextScheduleTime(cronJob, 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 @@ -531,7 +531,7 @@ func (jm *ControllerV2) syncCronJob( // Otherwise, the queue is always suppose to trigger sync function at the time of // the scheduled time, that will give atleast 1 unmet time schedule klog.V(4).InfoS("No unmet start times", "cronjob", klog.KRef(cronJob.GetNamespace(), cronJob.GetName())) - t := nextScheduledTimeDuration(*cronJob, sched, now) + t := nextScheduleTimeDuration(cronJob, now, sched) return cronJob, t, updateStatus, nil } @@ -550,7 +550,7 @@ func (jm *ControllerV2) syncCronJob( // Status.LastScheduleTime, Status.LastMissedTime), and then so we won't generate // and event the next time we process it, and also so the user looking at the status // can see easily that there was a missed execution. - t := nextScheduledTimeDuration(*cronJob, sched, now) + t := nextScheduleTimeDuration(cronJob, now, sched) return cronJob, t, updateStatus, nil } if inActiveListByName(cronJob, &batchv1.Job{ @@ -559,7 +559,7 @@ func (jm *ControllerV2) syncCronJob( Namespace: cronJob.Namespace, }}) || cronJob.Status.LastScheduleTime.Equal(&metav1.Time{Time: *scheduledTime}) { klog.V(4).InfoS("Not starting job because the scheduled time is already processed", "cronjob", klog.KRef(cronJob.GetNamespace(), cronJob.GetName()), "schedule", scheduledTime) - t := nextScheduledTimeDuration(*cronJob, sched, now) + t := nextScheduleTimeDuration(cronJob, now, sched) return cronJob, t, updateStatus, nil } if cronJob.Spec.ConcurrencyPolicy == batchv1.ForbidConcurrent && len(cronJob.Status.Active) > 0 { @@ -574,7 +574,7 @@ func (jm *ControllerV2) syncCronJob( // But that would mean that you could not inspect prior successes or failures of Forbid jobs. klog.V(4).InfoS("Not starting job because prior execution is still running and concurrency policy is Forbid", "cronjob", klog.KRef(cronJob.GetNamespace(), cronJob.GetName())) jm.recorder.Eventf(cronJob, corev1.EventTypeNormal, "JobAlreadyActive", "Not starting job because prior execution is running and concurrency policy is Forbid") - t := nextScheduledTimeDuration(*cronJob, sched, now) + t := nextScheduleTimeDuration(cronJob, now, sched) return cronJob, t, updateStatus, nil } if cronJob.Spec.ConcurrencyPolicy == batchv1.ReplaceConcurrent { @@ -635,7 +635,7 @@ func (jm *ControllerV2) syncCronJob( cronJob.Status.LastScheduleTime = &metav1.Time{Time: *scheduledTime} updateStatus = true - t := nextScheduledTimeDuration(*cronJob, sched, now) + t := nextScheduleTimeDuration(cronJob, now, sched) return cronJob, t, updateStatus, nil } @@ -643,30 +643,6 @@ func getJobName(cj *batchv1.CronJob, scheduledTime time.Time) string { return fmt.Sprintf("%s-%d", cj.Name, getTimeHashInMinutes(scheduledTime)) } -// nextScheduledTimeDuration returns the time duration to requeue based on -// the schedule and last schedule time. It adds a 100ms padding to the next requeue to account -// for Network Time Protocol(NTP) time skews. If the time drifts are adjusted which in most -// realistic cases would be around 100s, scheduled cron will still be executed without missing -// the schedule. -func nextScheduledTimeDuration(cj batchv1.CronJob, sched cron.Schedule, now time.Time) *time.Duration { - earliestTime := cj.ObjectMeta.CreationTimestamp.Time - if cj.Status.LastScheduleTime != nil { - earliestTime = cj.Status.LastScheduleTime.Time - } - mostRecentTime, _, err := getMostRecentScheduleTime(earliestTime, now, sched) - if err != nil { - // we still have to requeue at some point, so aim for the next scheduling slot from now - mostRecentTime = &now - } else if mostRecentTime == nil { - // no missed schedules since earliestTime - mostRecentTime = &earliestTime - } - - t := sched.Next(*mostRecentTime).Add(nextScheduleDelta).Sub(now) - - return &t -} - // cleanupFinishedJobs cleanups finished jobs created by a CronJob // It returns a bool to indicate an update to api-server is needed func (jm *ControllerV2) cleanupFinishedJobs(ctx context.Context, cj *batchv1.CronJob, js []*batchv1.Job) bool { diff --git a/pkg/controller/cronjob/utils.go b/pkg/controller/cronjob/utils.go index 2e6af912fba..9f40e655eb5 100644 --- a/pkg/controller/cronjob/utils.go +++ b/pkg/controller/cronjob/utils.go @@ -69,39 +69,78 @@ func deleteFromActiveList(cj *batchv1.CronJob, uid types.UID) { cj.Status.Active = newActive } -// 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 getNextScheduleTime(cj batchv1.CronJob, now time.Time, schedule cron.Schedule, recorder record.EventRecorder) (*time.Time, error) { - var ( - earliestTime time.Time - ) +// mostRecentScheduleTime returns: +// - the last schedule time or CronJob's creation time, +// - the most recent time a Job should be created or nil, if that's after now, +// - number of missed schedules +// - error in an edge case where the schedule specification is grammatically correct, +// but logically doesn't make sense (31st day for months with only 30 days, for example). +func mostRecentScheduleTime(cj *batchv1.CronJob, now time.Time, schedule cron.Schedule, includeStartingDeadlineSeconds bool) (time.Time, *time.Time, int64, error) { + earliestTime := cj.ObjectMeta.CreationTimestamp.Time if cj.Status.LastScheduleTime != nil { earliestTime = cj.Status.LastScheduleTime.Time - } else { - // If none found, then this is either a recently created cronJob, - // or the active/completed info was somehow lost (contract for status - // in kubernetes says it may need to be recreated), or that we have - // started a job, but have not noticed it yet (distributed systems can - // have arbitrary delays). In any case, use the creation time of the - // CronJob as last known start time. - earliestTime = cj.ObjectMeta.CreationTimestamp.Time } - if cj.Spec.StartingDeadlineSeconds != nil { - // Controller is not going to schedule anything below this point + if includeStartingDeadlineSeconds && cj.Spec.StartingDeadlineSeconds != nil { + // controller is not going to schedule anything below this point schedulingDeadline := now.Add(-time.Second * time.Duration(*cj.Spec.StartingDeadlineSeconds)) if schedulingDeadline.After(earliestTime) { earliestTime = schedulingDeadline } } - if earliestTime.After(now) { - return nil, nil + + t1 := schedule.Next(earliestTime) + t2 := schedule.Next(t1) + + if now.Before(t1) { + return earliestTime, nil, 0, nil + } + if now.Before(t2) { + return earliestTime, &t1, 1, nil } - t, numberOfMissedSchedules, err := getMostRecentScheduleTime(earliestTime, now, schedule) + // It is possible for cron.ParseStandard("59 23 31 2 *") to return an invalid schedule + // minute - 59, hour - 23, dom - 31, month - 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 earliestTime, nil, 0, fmt.Errorf("time difference between two schedules is less than 1 second") + } + timeElapsed := int64(now.Sub(t1).Seconds()) + numberOfMissedSchedules := (timeElapsed / timeBetweenTwoSchedules) + 1 + mostRecentTime := time.Unix(t1.Unix()+((numberOfMissedSchedules-1)*timeBetweenTwoSchedules), 0).UTC() + + return earliestTime, &mostRecentTime, numberOfMissedSchedules, nil +} + +// nextScheduleTimeDuration returns the time duration to requeue based on +// the schedule and last schedule time. It adds a 100ms padding to the next requeue to account +// for Network Time Protocol(NTP) time skews. If the time drifts the adjustment, which in most +// realistic cases should be around 100s, the job will still be executed without missing +// the schedule. +func nextScheduleTimeDuration(cj *batchv1.CronJob, now time.Time, schedule cron.Schedule) *time.Duration { + earliestTime, mostRecentTime, _, err := mostRecentScheduleTime(cj, now, schedule, false) + if err != nil { + // we still have to requeue at some point, so aim for the next scheduling slot from now + mostRecentTime = &now + } else if mostRecentTime == nil { + // no missed schedules since earliestTime + mostRecentTime = &earliestTime + } + + t := schedule.Next(*mostRecentTime).Add(nextScheduleDelta).Sub(now) + return &t +} + +// nextScheduleTime returns the time.Time of the next schedule after the last scheduled +// and before now, or nil if no unmet schedule times, and an error. +// If there are too many (>100) unstarted times, it will also record a warning. +func nextScheduleTime(cj *batchv1.CronJob, now time.Time, schedule cron.Schedule, recorder record.EventRecorder) (*time.Time, error) { + _, mostRecentTime, numberOfMissedSchedules, err := mostRecentScheduleTime(cj, now, schedule, true) + + if mostRecentTime == nil || mostRecentTime.After(now) { + return nil, err + } if numberOfMissedSchedules > 100 { // An object might miss several starts. For example, if @@ -121,36 +160,10 @@ func getNextScheduleTime(cj batchv1.CronJob, now time.Time, schedule cron.Schedu // // I've somewhat arbitrarily picked 100, as more than 80, // but less than "lots". - recorder.Eventf(&cj, corev1.EventTypeWarning, "TooManyMissedTimes", "too many missed start times: %d. Set or decrease .spec.startingDeadlineSeconds or check clock skew", numberOfMissedSchedules) + recorder.Eventf(cj, corev1.EventTypeWarning, "TooManyMissedTimes", "too many missed start times: %d. Set or decrease .spec.startingDeadlineSeconds or check clock skew", numberOfMissedSchedules) klog.InfoS("too many missed times", "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName()), "missed times", numberOfMissedSchedules) } - 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, error) { - t1 := schedule.Next(earliestTime) - t2 := schedule.Next(t1) - - if now.Before(t1) { - return nil, 0, nil - } - if now.Before(t2) { - return &t1, 1, nil - } - - // 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, nil + return mostRecentTime, err } func copyLabels(template *batchv1.JobTemplateSpec) labels.Set { diff --git a/pkg/controller/cronjob/utils_test.go b/pkg/controller/cronjob/utils_test.go index 092df867ea1..e5f5ebcf3b6 100644 --- a/pkg/controller/cronjob/utils_test.go +++ b/pkg/controller/cronjob/utils_test.go @@ -88,7 +88,7 @@ func TestGetJobFromTemplate2(t *testing.T) { } } -func TestGetNextScheduleTime(t *testing.T) { +func TestNextScheduleTime(t *testing.T) { // schedule is hourly on the hour schedule := "0 * * * ?" @@ -102,15 +102,9 @@ func TestGetNextScheduleTime(t *testing.T) { } recorder := record.NewFakeRecorder(50) // T1 is a scheduled start time of that schedule - T1, err := time.Parse(time.RFC3339, "2016-05-19T10:00:00Z") - if err != nil { - t.Errorf("test setup error: %v", err) - } + T1 := *topOfTheHour() // T2 is a scheduled start time of that schedule after T1 - T2, err := time.Parse(time.RFC3339, "2016-05-19T11:00:00Z") - if err != nil { - t.Errorf("test setup error: %v", err) - } + T2 := *deltaTimeAfterTopOfTheHour(1 * time.Hour) cj := batchv1.CronJob{ ObjectMeta: metav1.ObjectMeta{ @@ -130,7 +124,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, _ := nextScheduleTime(&cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule != nil { t.Errorf("expected no start time, got: %v", schedule) } @@ -141,7 +135,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, _ := nextScheduleTime(&cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule == nil { t.Errorf("expected 1 start time, got nil") } else if !schedule.Equal(T1) { @@ -156,7 +150,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, _ := nextScheduleTime(&cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule != nil { t.Errorf("expected 0 start times, got: %v", schedule) } @@ -169,7 +163,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, _ := nextScheduleTime(&cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule == nil { t.Errorf("expected 1 start times, got nil") } else if !schedule.Equal(T2) { @@ -182,7 +176,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, _ := nextScheduleTime(&cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule == nil { t.Errorf("expected 1 start times, got nil") } else if !schedule.Equal(T2) { @@ -194,7 +188,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, _ := nextScheduleTime(&cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule == nil { t.Errorf("expected more than 0 missed times") } @@ -207,65 +201,79 @@ 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, _ := nextScheduleTime(&cj, now, PraseSchedule(cj.Spec.Schedule), recorder) if schedule == nil { t.Errorf("expected more than 0 missed times") } } + { + // Case 8: ensure the error from mostRecentScheduleTime gets populated up + cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: T1.Add(10 * time.Second)} + cj.Status.LastScheduleTime = nil + now := *deltaTimeAfterTopOfTheHour(1 * time.Hour) + // rouge schedule + schedule, err := nextScheduleTime(&cj, now, PraseSchedule("59 23 31 2 *"), recorder) + if schedule != nil { + t.Errorf("expected no start time, got: %v", schedule) + } + if err == nil { + t.Errorf("expected error") + } + } } func TestByJobStartTime(t *testing.T) { now := metav1.NewTime(time.Date(2018, time.January, 1, 2, 3, 4, 5, time.UTC)) later := metav1.NewTime(time.Date(2019, time.January, 1, 2, 3, 4, 5, time.UTC)) - aNil := batchv1.Job{ + aNil := &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{Name: "a"}, Status: batchv1.JobStatus{}, } - bNil := batchv1.Job{ + bNil := &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{Name: "b"}, Status: batchv1.JobStatus{}, } - aSet := batchv1.Job{ + aSet := &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{Name: "a"}, Status: batchv1.JobStatus{StartTime: &now}, } - bSet := batchv1.Job{ + bSet := &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{Name: "b"}, Status: batchv1.JobStatus{StartTime: &now}, } - aSetLater := batchv1.Job{ + aSetLater := &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{Name: "a"}, Status: batchv1.JobStatus{StartTime: &later}, } testCases := []struct { name string - input, expected []batchv1.Job + input, expected []*batchv1.Job }{ { name: "both have nil start times", - input: []batchv1.Job{bNil, aNil}, - expected: []batchv1.Job{aNil, bNil}, + input: []*batchv1.Job{bNil, aNil}, + expected: []*batchv1.Job{aNil, bNil}, }, { name: "only the first has a nil start time", - input: []batchv1.Job{aNil, bSet}, - expected: []batchv1.Job{bSet, aNil}, + input: []*batchv1.Job{aNil, bSet}, + expected: []*batchv1.Job{bSet, aNil}, }, { name: "only the second has a nil start time", - input: []batchv1.Job{aSet, bNil}, - expected: []batchv1.Job{aSet, bNil}, + input: []*batchv1.Job{aSet, bNil}, + expected: []*batchv1.Job{aSet, bNil}, }, { name: "both have non-nil, equal start time", - input: []batchv1.Job{bSet, aSet}, - expected: []batchv1.Job{aSet, bSet}, + input: []*batchv1.Job{bSet, aSet}, + expected: []*batchv1.Job{aSet, bSet}, }, { name: "both have non-nil, different start time", - input: []batchv1.Job{aSetLater, bSet}, - expected: []batchv1.Job{bSet, aSetLater}, + input: []*batchv1.Job{aSetLater, bSet}, + expected: []*batchv1.Job{bSet, aSetLater}, }, } @@ -277,84 +285,179 @@ func TestByJobStartTime(t *testing.T) { } } -func TestGetMostRecentScheduleTime(t *testing.T) { - type args struct { - earliestTime *time.Time - now time.Time - schedule string - } +func TestMostRecentScheduleTime(t *testing.T) { + metav1TopOfTheHour := metav1.NewTime(*topOfTheHour()) + metav1HalfPastTheHour := metav1.NewTime(*deltaTimeAfterTopOfTheHour(30 * time.Minute)) + oneMinute := int64(60) + tests := []struct { name string - args args - expectedTime *time.Time + cj *batchv1.CronJob + includeSDS bool + now time.Time + expectedEarliestTime time.Time + expectedRecentTime *time.Time expectedNumberOfMisses int64 wantErr bool }{ { name: "now before next schedule", - args: args{ - earliestTime: topOfTheHour(), - now: topOfTheHour().Add(time.Second * 30), - schedule: "0 * * * *", + cj: &batchv1.CronJob{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1TopOfTheHour, + }, + Spec: batchv1.CronJobSpec{ + Schedule: "0 * * * *", + }, }, - expectedTime: nil, + now: topOfTheHour().Add(30 * time.Second), + expectedRecentTime: nil, + expectedEarliestTime: *topOfTheHour(), }, { name: "now just after next schedule", - args: args{ - earliestTime: topOfTheHour(), - now: topOfTheHour().Add(time.Minute * 61), - schedule: "0 * * * *", + cj: &batchv1.CronJob{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1TopOfTheHour, + }, + Spec: batchv1.CronJobSpec{ + Schedule: "0 * * * *", + }, }, - expectedTime: deltaTimeAfterTopOfTheHour(time.Minute * 60), + now: topOfTheHour().Add(61 * time.Minute), + expectedRecentTime: deltaTimeAfterTopOfTheHour(60 * time.Minute), + expectedEarliestTime: *topOfTheHour(), expectedNumberOfMisses: 1, }, { name: "missed 5 schedules", - args: args{ - earliestTime: deltaTimeAfterTopOfTheHour(time.Second * 10), - now: *deltaTimeAfterTopOfTheHour(time.Minute * 301), - schedule: "0 * * * *", + cj: &batchv1.CronJob{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1.NewTime(*deltaTimeAfterTopOfTheHour(10 * time.Second)), + }, + Spec: batchv1.CronJobSpec{ + Schedule: "0 * * * *", + }, }, - expectedTime: deltaTimeAfterTopOfTheHour(time.Minute * 300), + now: *deltaTimeAfterTopOfTheHour(301 * time.Minute), + expectedRecentTime: deltaTimeAfterTopOfTheHour(300 * time.Minute), + expectedEarliestTime: *deltaTimeAfterTopOfTheHour(10 * time.Second), expectedNumberOfMisses: 5, }, { name: "rogue cronjob", - args: args{ - earliestTime: deltaTimeAfterTopOfTheHour(time.Second * 10), - now: *deltaTimeAfterTopOfTheHour(time.Hour * 1000000), - schedule: "59 23 31 2 *", + cj: &batchv1.CronJob{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1.NewTime(*deltaTimeAfterTopOfTheHour(10 * time.Second)), + }, + Spec: batchv1.CronJobSpec{ + Schedule: "59 23 31 2 *", + }, }, - expectedTime: nil, + now: *deltaTimeAfterTopOfTheHour(1 * time.Hour), + expectedRecentTime: nil, expectedNumberOfMisses: 0, wantErr: true, }, + { + name: "earliestTime being CreationTimestamp and LastScheduleTime", + cj: &batchv1.CronJob{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1TopOfTheHour, + }, + Spec: batchv1.CronJobSpec{ + Schedule: "0 * * * *", + }, + Status: batchv1.CronJobStatus{ + LastScheduleTime: &metav1TopOfTheHour, + }, + }, + now: *deltaTimeAfterTopOfTheHour(30 * time.Second), + expectedEarliestTime: *topOfTheHour(), + expectedRecentTime: nil, + }, + { + name: "earliestTime being LastScheduleTime", + cj: &batchv1.CronJob{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1TopOfTheHour, + }, + Spec: batchv1.CronJobSpec{ + Schedule: "*/5 * * * *", + }, + Status: batchv1.CronJobStatus{ + LastScheduleTime: &metav1HalfPastTheHour, + }, + }, + now: *deltaTimeAfterTopOfTheHour(31 * time.Minute), + expectedEarliestTime: *deltaTimeAfterTopOfTheHour(30 * time.Minute), + expectedRecentTime: nil, + }, + { + name: "earliestTime being LastScheduleTime (within StartingDeadlineSeconds)", + cj: &batchv1.CronJob{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1TopOfTheHour, + }, + Spec: batchv1.CronJobSpec{ + Schedule: "*/5 * * * *", + StartingDeadlineSeconds: &oneMinute, + }, + Status: batchv1.CronJobStatus{ + LastScheduleTime: &metav1HalfPastTheHour, + }, + }, + now: *deltaTimeAfterTopOfTheHour(31 * time.Minute), + expectedEarliestTime: *deltaTimeAfterTopOfTheHour(30 * time.Minute), + expectedRecentTime: nil, + }, + { + name: "earliestTime being LastScheduleTime (outside StartingDeadlineSeconds)", + cj: &batchv1.CronJob{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1TopOfTheHour, + }, + Spec: batchv1.CronJobSpec{ + Schedule: "*/5 * * * *", + StartingDeadlineSeconds: &oneMinute, + }, + Status: batchv1.CronJobStatus{ + LastScheduleTime: &metav1HalfPastTheHour, + }, + }, + includeSDS: true, + now: *deltaTimeAfterTopOfTheHour(32 * time.Minute), + expectedEarliestTime: *deltaTimeAfterTopOfTheHour(31 * time.Minute), + expectedRecentTime: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - sched, err := cron.ParseStandard(tt.args.schedule) + sched, err := cron.ParseStandard(tt.cj.Spec.Schedule) if err != nil { t.Errorf("error setting up the test, %s", err) } - gotTime, gotNumberOfMisses, err := getMostRecentScheduleTime(*tt.args.earliestTime, tt.args.now, sched) + gotEarliestTime, gotRecentTime, gotNumberOfMisses, err := mostRecentScheduleTime(tt.cj, tt.now, sched, tt.includeSDS) if tt.wantErr { if err == nil { - t.Error("getMostRecentScheduleTime() got no error when expected one") + t.Error("mostRecentScheduleTime() got no error when expected one") } return } if !tt.wantErr && err != nil { - t.Error("getMostRecentScheduleTime() got error when none expected") + t.Error("mostRecentScheduleTime() got error when none expected") } - if gotTime == nil && tt.expectedTime != nil { - t.Errorf("getMostRecentScheduleTime() got nil, want %v", tt.expectedTime) + if gotEarliestTime.IsZero() { + t.Errorf("earliestTime should never be 0, want %v", tt.expectedEarliestTime) } - if gotTime != nil && tt.expectedTime != nil && !gotTime.Equal(*tt.expectedTime) { - t.Errorf("getMostRecentScheduleTime() got = %v, want %v", gotTime, tt.expectedTime) + if !gotEarliestTime.Equal(tt.expectedEarliestTime) { + t.Errorf("expectedEarliestTime - got %v, want %v", gotEarliestTime, tt.expectedEarliestTime) + } + if !reflect.DeepEqual(gotRecentTime, tt.expectedRecentTime) { + t.Errorf("expectedRecentTime - got %v, want %v", gotRecentTime, tt.expectedRecentTime) } if gotNumberOfMisses != tt.expectedNumberOfMisses { - t.Errorf("getMostRecentScheduleTime() got1 = %v, want %v", gotNumberOfMisses, tt.expectedNumberOfMisses) + t.Errorf("expectedNumberOfMisses - got %v, want %v", gotNumberOfMisses, tt.expectedNumberOfMisses) } }) }