From d141a43d86745fe807dc4f07dde3f7209c3e77e1 Mon Sep 17 00:00:00 2001 From: peay Date: Sat, 7 Jan 2017 16:06:02 -0500 Subject: [PATCH] Do not list CronJob unmet starting times beyond deadline --- .../cronjob/cronjob_controller_test.go | 56 ++++++++++++++++--- pkg/controller/cronjob/utils.go | 10 +++- pkg/controller/cronjob/utils_test.go | 15 ++++- 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/pkg/controller/cronjob/cronjob_controller_test.go b/pkg/controller/cronjob/cronjob_controller_test.go index 6082fd57ad..a13a110a83 100644 --- a/pkg/controller/cronjob/cronjob_controller_test.go +++ b/pkg/controller/cronjob/cronjob_controller_test.go @@ -56,6 +56,14 @@ func justAfterTheHour() time.Time { return T1 } +func weekAfterTheHour() time.Time { + T1, err := time.Parse(time.RFC3339, "2016-05-26T10:00:00Z") + if err != nil { + panic("test setup error") + } + return T1 +} + func justBeforeThePriorHour() time.Time { T1, err := time.Parse(time.RFC3339, "2016-05-19T08:59:00Z") if err != nil { @@ -129,17 +137,31 @@ func newJob(UID string) batch.Job { } var ( - shortDead int64 = 10 - longDead int64 = 1000000 - noDead int64 = -12345 - A batch.ConcurrencyPolicy = batch.AllowConcurrent - f batch.ConcurrencyPolicy = batch.ForbidConcurrent - R batch.ConcurrencyPolicy = batch.ReplaceConcurrent - T bool = true - F bool = false + shortDead int64 = 10 + mediumDead int64 = 2 * 60 * 60 + longDead int64 = 1000000 + noDead int64 = -12345 + A batch.ConcurrencyPolicy = batch.AllowConcurrent + f batch.ConcurrencyPolicy = batch.ForbidConcurrent + R batch.ConcurrencyPolicy = batch.ReplaceConcurrent + T bool = true + F bool = false ) func TestSyncOne_RunOrNot(t *testing.T) { + // Check expectations on deadline parameters + if shortDead/60/60 >= 1 { + t.Errorf("shortDead should be less than one hour") + } + + if mediumDead/60/60 < 1 || mediumDead/60/60 >= 24 { + t.Errorf("mediumDead should be between one hour and one day") + } + + if longDead/60/60/24 < 10 { + t.Errorf("longDead should be at least ten days") + } + testCases := map[string]struct { // sj spec concurrencyPolicy batch.ConcurrencyPolicy @@ -188,6 +210,24 @@ func TestSyncOne_RunOrNot(t *testing.T) { "still active, is time, suspended": {A, T, onTheHour, noDead, T, T, justAfterTheHour(), F, F, 1}, "still active, is time, past deadline": {A, F, onTheHour, shortDead, T, T, justAfterTheHour(), F, F, 1}, "still active, is time, not past deadline": {A, F, onTheHour, longDead, T, T, justAfterTheHour(), T, F, 2}, + + // 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, weekAfterTheHour(), F, F, 0}, + "prev ran but done, long overdue, not past deadline, R": {R, F, onTheHour, longDead, T, F, weekAfterTheHour(), F, F, 0}, + "prev ran but done, long overdue, not past deadline, F": {f, F, onTheHour, longDead, T, F, weekAfterTheHour(), F, F, 0}, + "prev ran but done, long overdue, no deadline, A": {A, F, onTheHour, noDead, T, F, weekAfterTheHour(), F, F, 0}, + "prev ran but done, long overdue, no deadline, R": {R, F, onTheHour, noDead, T, F, weekAfterTheHour(), F, F, 0}, + "prev ran but done, long overdue, no deadline, F": {f, F, onTheHour, noDead, T, F, weekAfterTheHour(), F, F, 0}, + + "prev ran but done, long overdue, past medium deadline, A": {A, F, onTheHour, mediumDead, T, F, weekAfterTheHour(), T, F, 1}, + "prev ran but done, long overdue, past short deadline, A": {A, F, onTheHour, shortDead, T, F, weekAfterTheHour(), T, F, 1}, + + "prev ran but done, long overdue, past medium deadline, R": {R, F, onTheHour, mediumDead, T, F, weekAfterTheHour(), T, F, 1}, + "prev ran but done, long overdue, past short deadline, R": {R, F, onTheHour, shortDead, T, F, weekAfterTheHour(), T, F, 1}, + + "prev ran but done, long overdue, past medium deadline, F": {f, F, onTheHour, mediumDead, T, F, weekAfterTheHour(), T, F, 1}, + "prev ran but done, long overdue, past short deadline, F": {f, F, onTheHour, shortDead, T, F, weekAfterTheHour(), T, F, 1}, } for name, tc := range testCases { sj := cronJob() diff --git a/pkg/controller/cronjob/utils.go b/pkg/controller/cronjob/utils.go index d3912a55e2..52eec868f1 100644 --- a/pkg/controller/cronjob/utils.go +++ b/pkg/controller/cronjob/utils.go @@ -125,6 +125,7 @@ func getRecentUnmetScheduleTimes(sj batch.CronJob, now time.Time) ([]time.Time, if err != nil { return starts, fmt.Errorf("Unparseable schedule: %s : %s", sj.Spec.Schedule, err) } + var earliestTime time.Time if sj.Status.LastScheduleTime != nil { earliestTime = sj.Status.LastScheduleTime.Time @@ -137,7 +138,14 @@ func getRecentUnmetScheduleTimes(sj batch.CronJob, now time.Time) ([]time.Time, // CronJob as last known start time. earliestTime = sj.ObjectMeta.CreationTimestamp.Time } + if sj.Spec.StartingDeadlineSeconds != nil { + // Controller is not going to schedule anything below this point + schedulingDeadline := now.Add(-time.Second * time.Duration(*sj.Spec.StartingDeadlineSeconds)) + if schedulingDeadline.After(earliestTime) { + earliestTime = schedulingDeadline + } + } if earliestTime.After(now) { return []time.Time{}, nil } @@ -163,7 +171,7 @@ func getRecentUnmetScheduleTimes(sj batch.CronJob, now time.Time) ([]time.Time, // but less than "lots". if len(starts) > 100 { // We can't get the most recent times so just return an empty slice - return []time.Time{}, fmt.Errorf("Too many missed start times to list") + return []time.Time{}, fmt.Errorf("Too many missed start time (> 100). Set or decrease .spec.startingDeadlineSeconds or check clock skew.") } } return starts, nil diff --git a/pkg/controller/cronjob/utils_test.go b/pkg/controller/cronjob/utils_test.go index 82cc34fe4c..d447833d0b 100644 --- a/pkg/controller/cronjob/utils_test.go +++ b/pkg/controller/cronjob/utils_test.go @@ -367,7 +367,7 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) { } } { - // Case 6: now is way way ahead of last start time. + // Case 6: now is way way ahead of last start time, and there is no deadline. sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: T1.Add(-2 * time.Hour)} sj.Status.LastScheduleTime = &metav1.Time{Time: T1.Add(-1 * time.Hour)} now := T2.Add(10 * 24 * time.Hour) @@ -376,5 +376,18 @@ func TestGetRecentUnmetScheduleTimes(t *testing.T) { t.Errorf("unexpected lack of error") } } + { + // Case 7: now is way way ahead of last start time, but there is a short deadline. + sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: T1.Add(-2 * time.Hour)} + sj.Status.LastScheduleTime = &metav1.Time{Time: T1.Add(-1 * time.Hour)} + now := T2.Add(10 * 24 * time.Hour) + // Deadline is short + deadline := int64(2 * 60 * 60) + sj.Spec.StartingDeadlineSeconds = &deadline + _, err := getRecentUnmetScheduleTimes(sj, now) + if err != nil { + t.Errorf("unexpected error") + } + } }