diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go index 3144b45fd60..1cd6e8e2b7e 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2.go +++ b/pkg/controller/cronjob/cronjob_controllerv2.go @@ -396,7 +396,7 @@ func (jm *ControllerV2) updateCronJob(old interface{}, curr interface{}) { return } now := jm.now() - t := nextScheduledTimeDuration(sched, now) + t := nextScheduledTimeDuration(*newCJ, sched, now) jm.enqueueControllerAfter(curr, *t) return @@ -529,7 +529,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(sched, now) + t := nextScheduledTimeDuration(*cronJob, sched, now) return cronJob, t, updateStatus, nil } @@ -548,7 +548,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(sched, now) + t := nextScheduledTimeDuration(*cronJob, sched, now) return cronJob, t, updateStatus, nil } if isJobInActiveList(&batchv1.Job{ @@ -557,7 +557,7 @@ func (jm *ControllerV2) syncCronJob( Namespace: cronJob.Namespace, }}, cronJob.Status.Active) || 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(sched, now) + t := nextScheduledTimeDuration(*cronJob, sched, now) return cronJob, t, updateStatus, nil } if cronJob.Spec.ConcurrencyPolicy == batchv1.ForbidConcurrent && len(cronJob.Status.Active) > 0 { @@ -572,7 +572,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(sched, now) + t := nextScheduledTimeDuration(*cronJob, sched, now) return cronJob, t, updateStatus, nil } if cronJob.Spec.ConcurrencyPolicy == batchv1.ReplaceConcurrent { @@ -633,7 +633,7 @@ func (jm *ControllerV2) syncCronJob( cronJob.Status.LastScheduleTime = &metav1.Time{Time: *scheduledTime} updateStatus = true - t := nextScheduledTimeDuration(sched, now) + t := nextScheduledTimeDuration(*cronJob, sched, now) return cronJob, t, updateStatus, nil } @@ -642,12 +642,26 @@ func getJobName(cj *batchv1.CronJob, scheduledTime time.Time) string { } // nextScheduledTimeDuration returns the time duration to requeue based on -// the schedule and current time. It adds a 100ms padding to the next requeue to account +// 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(sched cron.Schedule, now time.Time) *time.Duration { - t := sched.Next(now).Add(nextScheduleDelta).Sub(now) +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 } diff --git a/pkg/controller/cronjob/cronjob_controllerv2_test.go b/pkg/controller/cronjob/cronjob_controllerv2_test.go index 55e6295e390..507f0b722c8 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2_test.go +++ b/pkg/controller/cronjob/cronjob_controllerv2_test.go @@ -23,8 +23,6 @@ import ( "testing" "time" - "github.com/robfig/cron/v3" - batchv1 "k8s.io/api/batch/v1" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -53,6 +51,7 @@ var ( errorSchedule = "obvious error schedule" // schedule is hourly on the hour onTheHour = "0 * * * ?" + everyHour = "@every 1h" errorTimeZone = "bad timezone" newYork = "America/New_York" @@ -154,6 +153,14 @@ func justBeforeTheHour() time.Time { return T1 } +func justBeforeTheNextHour() time.Time { + T1, err := time.Parse(time.RFC3339, "2016-05-19T10:59:00Z") + if err != nil { + panic("test setup error") + } + return T1 +} + func weekAfterTheHour() time.Time { T1, err := time.Parse(time.RFC3339, "2016-05-26T10:00:00Z") if err != nil { @@ -189,11 +196,13 @@ func TestControllerV2SyncCronJob(t *testing.T) { stillActive bool // environment - jobCreationTime time.Time - now time.Time - jobCreateError error - jobGetErr error - enableTimeZone bool + cronjobCreationTime time.Time + jobCreationTime time.Time + lastScheduleTime time.Time + now time.Time + jobCreateError error + jobGetErr error + enableTimeZone bool // expectations expectCreate bool @@ -202,6 +211,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedWarnings int expectErr bool expectRequeueAfter bool + expectedRequeueDuration time.Duration expectUpdateStatus bool jobStillNotFoundInLister bool jobPresentInCJActiveStatus bool @@ -251,6 +261,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { jobCreationTime: justAfterThePriorHour(), now: justBeforeTheHour(), expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, jobPresentInCJActiveStatus: true}, "never ran, not time, F": { concurrencyPolicy: "Forbid", @@ -259,6 +270,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { jobCreationTime: justAfterThePriorHour(), now: justBeforeTheHour(), expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, jobPresentInCJActiveStatus: true, }, "never ran, not time, R": { @@ -268,6 +280,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { jobCreationTime: justAfterThePriorHour(), now: justBeforeTheHour(), expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, jobPresentInCJActiveStatus: true, }, "never ran, not time in zone": { @@ -279,6 +292,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { now: justBeforeTheHour(), enableTimeZone: true, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, jobPresentInCJActiveStatus: true, }, "never ran, is time, A": { @@ -290,6 +304,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -302,6 +317,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -314,6 +330,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -328,6 +345,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -342,6 +360,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -356,6 +375,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectedWarnings: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -375,6 +395,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { jobCreationTime: justAfterThePriorHour(), now: justAfterTheHour().Add(time.Minute * time.Duration(shortDead+1)), expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute - time.Minute*time.Duration(shortDead+1) + nextScheduleDelta, jobPresentInCJActiveStatus: true, }, "never ran, is time, not past deadline": { @@ -386,6 +407,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -398,6 +420,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { jobCreationTime: justAfterThePriorHour(), now: justBeforeTheHour(), expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -409,6 +432,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { jobCreationTime: justAfterThePriorHour(), now: justBeforeTheHour(), expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -420,6 +444,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { jobCreationTime: justAfterThePriorHour(), now: justBeforeTheHour(), expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -433,6 +458,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -458,6 +484,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -471,6 +498,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -493,6 +521,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { jobCreationTime: justAfterThePriorHour(), now: *justAfterTheHour(), expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -506,6 +535,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -520,6 +550,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { now: justBeforeTheHour(), expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, jobPresentInCJActiveStatus: true, }, "still active, not time, F": { @@ -532,6 +563,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { now: justBeforeTheHour(), expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, jobPresentInCJActiveStatus: true, }, "still active, not time, R": { @@ -544,6 +576,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { now: justBeforeTheHour(), expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, jobPresentInCJActiveStatus: true, }, "still active, is time, A": { @@ -557,6 +590,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 2, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -570,6 +604,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { now: *justAfterTheHour(), expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, jobPresentInCJActiveStatus: true, }, "still active, is time, R": { @@ -584,6 +619,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectDelete: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -622,6 +658,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { now: *justAfterTheHour(), expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, jobPresentInCJActiveStatus: true, }, "still active, is time, not past deadline": { @@ -635,6 +672,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 2, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -652,6 +690,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectActive: 1, expectedWarnings: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -666,6 +705,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectActive: 1, expectedWarnings: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -680,6 +720,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectActive: 1, expectedWarnings: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -694,6 +735,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectActive: 1, expectedWarnings: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -708,6 +750,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectActive: 1, expectedWarnings: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -722,6 +765,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectActive: 1, expectedWarnings: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -736,6 +780,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -749,6 +794,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -763,6 +809,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -776,6 +823,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -790,6 +838,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -803,6 +852,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectCreate: true, expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, }, @@ -810,15 +860,16 @@ func TestControllerV2SyncCronJob(t *testing.T) { // Tests for time skews // the controller sees job is created, takes no actions "this ran but done, time drifted back, F": { - concurrencyPolicy: "Forbid", - schedule: onTheHour, - deadline: noDead, - ranPreviously: true, - jobCreationTime: *justAfterTheHour(), - now: justBeforeTheHour(), - jobCreateError: errors.NewAlreadyExists(schema.GroupResource{Resource: "jobs", Group: "batch"}, ""), - expectRequeueAfter: true, - expectUpdateStatus: true, + concurrencyPolicy: "Forbid", + schedule: onTheHour, + deadline: noDead, + ranPreviously: true, + jobCreationTime: *justAfterTheHour(), + now: justBeforeTheHour(), + jobCreateError: errors.NewAlreadyExists(schema.GroupResource{Resource: "jobs", Group: "batch"}, ""), + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, + expectUpdateStatus: true, }, // Tests for slow job lister @@ -832,6 +883,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { now: justAfterTheHour().Add(time.Millisecond * 100), expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute - time.Millisecond*100 + nextScheduleDelta, jobStillNotFoundInLister: true, jobPresentInCJActiveStatus: true, }, @@ -845,6 +897,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { now: justAfterTheHour().Add(time.Millisecond * 100), expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute - time.Millisecond*100 + nextScheduleDelta, jobStillNotFoundInLister: true, jobPresentInCJActiveStatus: true, }, @@ -858,43 +911,259 @@ func TestControllerV2SyncCronJob(t *testing.T) { now: justAfterTheHour().Add(time.Millisecond * 100), expectActive: 1, expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute - time.Millisecond*100 + nextScheduleDelta, jobStillNotFoundInLister: true, jobPresentInCJActiveStatus: true, }, // Tests for slow cronjob list "this started but is not present in cronjob active list, not past deadline, A": { - concurrencyPolicy: "Allow", - schedule: onTheHour, - deadline: longDead, - ranPreviously: true, - stillActive: true, - jobCreationTime: topOfTheHour().Add(time.Millisecond * 100), - now: justAfterTheHour().Add(time.Millisecond * 100), - expectActive: 1, - expectRequeueAfter: true, + concurrencyPolicy: "Allow", + schedule: onTheHour, + deadline: longDead, + ranPreviously: true, + stillActive: true, + jobCreationTime: topOfTheHour().Add(time.Millisecond * 100), + now: justAfterTheHour().Add(time.Millisecond * 100), + expectActive: 1, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute - time.Millisecond*100 + nextScheduleDelta, }, "this started but is not present in cronjob active list, not past deadline, f": { - concurrencyPolicy: "Forbid", - schedule: onTheHour, - deadline: longDead, - ranPreviously: true, - stillActive: true, - jobCreationTime: topOfTheHour().Add(time.Millisecond * 100), - now: justAfterTheHour().Add(time.Millisecond * 100), - expectActive: 1, - expectRequeueAfter: true, + concurrencyPolicy: "Forbid", + schedule: onTheHour, + deadline: longDead, + ranPreviously: true, + stillActive: true, + jobCreationTime: topOfTheHour().Add(time.Millisecond * 100), + now: justAfterTheHour().Add(time.Millisecond * 100), + expectActive: 1, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute - time.Millisecond*100 + nextScheduleDelta, }, "this started but is not present in cronjob active list, not past deadline, R": { - concurrencyPolicy: "Replace", - schedule: onTheHour, - deadline: longDead, - ranPreviously: true, - stillActive: true, - jobCreationTime: topOfTheHour().Add(time.Millisecond * 100), - now: justAfterTheHour().Add(time.Millisecond * 100), - expectActive: 1, - expectRequeueAfter: true, + concurrencyPolicy: "Replace", + schedule: onTheHour, + deadline: longDead, + ranPreviously: true, + stillActive: true, + jobCreationTime: topOfTheHour().Add(time.Millisecond * 100), + now: justAfterTheHour().Add(time.Millisecond * 100), + expectActive: 1, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute - time.Millisecond*100 + nextScheduleDelta, + }, + + // Tests for @every-style schedule + "with @every schedule, never ran, not time": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: noDead, + cronjobCreationTime: justBeforeTheHour(), + jobCreationTime: justBeforeTheHour(), + now: *topOfTheHour(), + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, + jobPresentInCJActiveStatus: true, + }, + "with @every schedule, never ran, is time": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: noDead, + cronjobCreationTime: justBeforeThePriorHour(), + jobCreationTime: justBeforeThePriorHour(), + now: justBeforeTheHour(), + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, + jobPresentInCJActiveStatus: true, + expectCreate: true, + expectActive: 1, + expectUpdateStatus: true, + }, + "with @every schedule, never ran, is time, past deadline": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: shortDead, + cronjobCreationTime: justBeforeThePriorHour(), + jobCreationTime: justBeforeThePriorHour(), + now: justBeforeTheHour().Add(time.Second * time.Duration(shortDead+1)), + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta, + jobPresentInCJActiveStatus: true, + }, + "with @every schedule, never ran, is time, not past deadline": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: longDead, + cronjobCreationTime: justBeforeThePriorHour(), + jobCreationTime: justBeforeThePriorHour(), + now: justBeforeTheHour().Add(time.Second * time.Duration(shortDead-1)), + expectCreate: true, + expectActive: 1, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead-1) + nextScheduleDelta, + expectUpdateStatus: true, + jobPresentInCJActiveStatus: true, + }, + "with @every schedule, prev ran but done, not time": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: noDead, + ranPreviously: true, + cronjobCreationTime: justBeforeThePriorHour(), + jobCreationTime: justBeforeThePriorHour(), + lastScheduleTime: justBeforeTheHour(), + now: *topOfTheHour(), + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, + expectUpdateStatus: true, + jobPresentInCJActiveStatus: true, + }, + "with @every schedule, prev ran but done, is time": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: noDead, + ranPreviously: true, + cronjobCreationTime: justBeforeThePriorHour(), + jobCreationTime: justBeforeThePriorHour(), + lastScheduleTime: justBeforeTheHour(), + now: topOfTheHour().Add(1 * time.Hour), + expectCreate: true, + expectActive: 1, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, + expectUpdateStatus: true, + jobPresentInCJActiveStatus: true, + }, + "with @every schedule, prev ran but done, is time, past deadline": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: shortDead, + ranPreviously: true, + cronjobCreationTime: justBeforeThePriorHour(), + jobCreationTime: justBeforeThePriorHour(), + lastScheduleTime: justBeforeTheHour(), + now: justBeforeTheNextHour().Add(time.Second * time.Duration(shortDead+1)), + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta, + expectUpdateStatus: true, + jobPresentInCJActiveStatus: true, + }, + // This test will fail: the logic around StartingDeadlineSecond in getNextScheduleTime messes up + // the time that calculating schedule.Next(earliestTime) is based on. While this works perfectly + // well for classic cron scheduled, with @every X, schedule.Next(earliestTime) just returns the time + // offset by X relative to the earliestTime. + // "with @every schedule, prev ran but done, is time, not past deadline": { + // concurrencyPolicy: "Allow", + // schedule: everyHour, + // deadline: shortDead, + // ranPreviously: true, + // cronjobCreationTime: justBeforeThePriorHour(), + // jobCreationTime: justBeforeThePriorHour(), + // lastScheduleTime: justBeforeTheHour(), + // now: justBeforeTheNextHour().Add(time.Second * time.Duration(shortDead-1)), + // expectCreate: true, + // expectActive: 1, + // expectRequeueAfter: true, + // expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead-1) + nextScheduleDelta, + // expectUpdateStatus: true, + // jobPresentInCJActiveStatus: true, + // }, + "with @every schedule, still active, not time": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: noDead, + ranPreviously: true, + stillActive: true, + cronjobCreationTime: justBeforeThePriorHour(), + jobCreationTime: justBeforeTheHour(), + lastScheduleTime: justBeforeTheHour(), + now: *topOfTheHour(), + expectActive: 1, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, + jobPresentInCJActiveStatus: true, + }, + "with @every schedule, still active, is time": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: noDead, + ranPreviously: true, + stillActive: true, + cronjobCreationTime: justBeforeThePriorHour(), + jobCreationTime: justBeforeThePriorHour(), + lastScheduleTime: justBeforeThePriorHour(), + now: *justAfterTheHour(), + expectCreate: true, + expectActive: 2, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - 2*time.Minute + nextScheduleDelta, + expectUpdateStatus: true, + jobPresentInCJActiveStatus: true, + }, + "with @every schedule, still active, is time, past deadline": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: shortDead, + ranPreviously: true, + stillActive: true, + cronjobCreationTime: justBeforeThePriorHour(), + jobCreationTime: justBeforeTheHour(), + lastScheduleTime: justBeforeTheHour(), + now: justBeforeTheNextHour().Add(time.Second * time.Duration(shortDead+1)), + expectActive: 1, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta, + jobPresentInCJActiveStatus: true, + }, + "with @every schedule, still active, is time, not past deadline": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: longDead, + ranPreviously: true, + stillActive: true, + cronjobCreationTime: justBeforeThePriorHour(), + jobCreationTime: justBeforeTheHour(), + lastScheduleTime: justBeforeTheHour(), + now: justBeforeTheNextHour().Add(time.Second * time.Duration(shortDead-1)), + expectCreate: true, + expectActive: 2, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead-1) + nextScheduleDelta, + expectUpdateStatus: true, + jobPresentInCJActiveStatus: true, + }, + "with @every schedule, prev ran but done, long overdue, no deadline": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: noDead, + ranPreviously: true, + cronjobCreationTime: justAfterThePriorHour(), + lastScheduleTime: *justAfterTheHour(), + jobCreationTime: justAfterThePriorHour(), + now: weekAfterTheHour(), + expectCreate: true, + expectActive: 1, + expectedWarnings: 1, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, + expectUpdateStatus: true, + jobPresentInCJActiveStatus: true, + }, + "with @every schedule, prev ran but done, long overdue, past deadline": { + concurrencyPolicy: "Allow", + schedule: everyHour, + deadline: shortDead, + ranPreviously: true, + cronjobCreationTime: justAfterThePriorHour(), + lastScheduleTime: *justAfterTheHour(), + jobCreationTime: justAfterThePriorHour(), + now: weekAfterTheHour().Add(1 * time.Minute).Add(time.Second * time.Duration(shortDead+1)), + expectActive: 1, + expectRequeueAfter: true, + expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta, + expectUpdateStatus: true, + jobPresentInCJActiveStatus: true, }, } for name, tc := range testCases { @@ -921,7 +1190,13 @@ func TestControllerV2SyncCronJob(t *testing.T) { realCJ := cj.DeepCopy() if tc.ranPreviously { cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeThePriorHour()} + if !tc.cronjobCreationTime.IsZero() { + cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: tc.cronjobCreationTime} + } cj.Status.LastScheduleTime = &metav1.Time{Time: justAfterThePriorHour()} + if !tc.lastScheduleTime.IsZero() { + cj.Status.LastScheduleTime = &metav1.Time{Time: tc.lastScheduleTime} + } job, err = getJobFromTemplate2(&cj, tc.jobCreationTime) if err != nil { t.Fatalf("%s: unexpected error creating a job from template: %v", name, err) @@ -952,6 +1227,9 @@ func TestControllerV2SyncCronJob(t *testing.T) { } } else { cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeTheHour()} + if !tc.cronjobCreationTime.IsZero() { + cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: tc.cronjobCreationTime} + } if tc.stillActive { t.Errorf("%s: test setup error: this case makes no sense", name) } @@ -974,13 +1252,8 @@ func TestControllerV2SyncCronJob(t *testing.T) { t.Errorf("%s: expected error got none with requeueAfter time: %#v", name, requeueAfter) } if tc.expectRequeueAfter { - sched, err := cron.ParseStandard(tc.schedule) - if err != nil { - t.Errorf("%s: test setup error: the schedule %s is unparseable: %#v", name, tc.schedule, err) - } - expectedRequeueAfter := nextScheduledTimeDuration(sched, tc.now) - if !reflect.DeepEqual(requeueAfter, expectedRequeueAfter) { - t.Errorf("%s: expected requeueAfter: %+v, got requeueAfter time: %+v", name, expectedRequeueAfter, requeueAfter) + if !reflect.DeepEqual(requeueAfter, &tc.expectedRequeueDuration) { + t.Errorf("%s: expected requeueAfter: %+v, got requeueAfter time: %+v", name, tc.expectedRequeueDuration, requeueAfter) } } if updateStatus != tc.expectUpdateStatus { @@ -1132,6 +1405,9 @@ func TestControllerV2UpdateCronJob(t *testing.T) { Spec: jobSpec(), }, }, + Status: batchv1.CronJobStatus{ + LastScheduleTime: &metav1.Time{Time: justBeforeTheHour()}, + }, }, newCronJob: &batchv1.CronJob{ Spec: batchv1.CronJobSpec{ @@ -1144,6 +1420,77 @@ func TestControllerV2UpdateCronJob(t *testing.T) { Spec: jobSpec(), }, }, + Status: batchv1.CronJobStatus{ + LastScheduleTime: &metav1.Time{Time: justBeforeTheHour()}, + }, + }, + expectedDelay: 1*time.Second + nextScheduleDelta, + }, + { + name: "spec.schedule with @every changed - cadence decrease", + oldCronJob: &batchv1.CronJob{ + Spec: batchv1.CronJobSpec{ + Schedule: "@every 1m", + JobTemplate: batchv1.JobTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"a": "b"}, + Annotations: map[string]string{"x": "y"}, + }, + Spec: jobSpec(), + }, + }, + Status: batchv1.CronJobStatus{ + LastScheduleTime: &metav1.Time{Time: justBeforeTheHour()}, + }, + }, + newCronJob: &batchv1.CronJob{ + Spec: batchv1.CronJobSpec{ + Schedule: "@every 3m", + JobTemplate: batchv1.JobTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"a": "foo"}, + Annotations: map[string]string{"x": "y"}, + }, + Spec: jobSpec(), + }, + }, + Status: batchv1.CronJobStatus{ + LastScheduleTime: &metav1.Time{Time: justBeforeTheHour()}, + }, + }, + expectedDelay: 2*time.Minute + 1*time.Second + nextScheduleDelta, + }, + { + name: "spec.schedule with @every changed - cadence increase", + oldCronJob: &batchv1.CronJob{ + Spec: batchv1.CronJobSpec{ + Schedule: "@every 3m", + JobTemplate: batchv1.JobTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"a": "b"}, + Annotations: map[string]string{"x": "y"}, + }, + Spec: jobSpec(), + }, + }, + Status: batchv1.CronJobStatus{ + LastScheduleTime: &metav1.Time{Time: justBeforeTheHour()}, + }, + }, + newCronJob: &batchv1.CronJob{ + Spec: batchv1.CronJobSpec{ + Schedule: "@every 1m", + JobTemplate: batchv1.JobTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"a": "foo"}, + Annotations: map[string]string{"x": "y"}, + }, + Spec: jobSpec(), + }, + }, + Status: batchv1.CronJobStatus{ + LastScheduleTime: &metav1.Time{Time: justBeforeTheHour()}, + }, }, expectedDelay: 1*time.Second + nextScheduleDelta, },