From 6290a23ceb59e2fe79eff14555cf88b455c9942b Mon Sep 17 00:00:00 2001 From: Alay Patel Date: Mon, 22 Feb 2021 21:40:05 -0500 Subject: [PATCH] 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) }