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.

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 {

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
+// mostRecentScheduleTime returns:
-// it returns nil if no unmet schedule times.
+//   - 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,
-// If there are too many (>100) unstarted times, it will raise a warning and but still return
+//   - number of missed schedules
-// the list of missed times.
+//   - error in an edge case where the schedule specification is grammatically correct,
-func getNextScheduleTime(cj batchv1.CronJob, now time.Time, schedule cron.Schedule, recorder record.EventRecorder) (*time.Time, error) {
+//     but logically doesn't make sense (31st day for months with only 30 days, for example).
-	var (
+func mostRecentScheduleTime(cj *batchv1.CronJob, now time.Time, schedule cron.Schedule, includeStartingDeadlineSeconds bool) (time.Time, *time.Time, int64, error) {
-		earliestTime time.Time
+	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 {
+	if includeStartingDeadlineSeconds && cj.Spec.StartingDeadlineSeconds != nil {
-		// Controller is not going to schedule anything below this point
+		// 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
+	return mostRecentTime, 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
 }
 
 func copyLabels(template *batchv1.JobTemplateSpec) labels.Set {

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")
+	T1 := *topOfTheHour()
-	if err != nil {
-		t.Errorf("test setup error: %v", err)
-	}
 	// T2 is a scheduled start time of that schedule after T1
-	T2, err := time.Parse(time.RFC3339, "2016-05-19T11:00:00Z")
+	T2 := *deltaTimeAfterTopOfTheHour(1 * time.Hour)
-	if err != nil {
-		t.Errorf("test setup error: %v", err)
-	}
 
 	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},
+			input:    []*batchv1.Job{bNil, aNil},
-			expected: []batchv1.Job{aNil, bNil},
+			expected: []*batchv1.Job{aNil, bNil},
 		},
 		{
 			name:     "only the first has a nil start time",
-			input:    []batchv1.Job{aNil, bSet},
+			input:    []*batchv1.Job{aNil, bSet},
-			expected: []batchv1.Job{bSet, aNil},
+			expected: []*batchv1.Job{bSet, aNil},
 		},
 		{
 			name:     "only the second has a nil start time",
-			input:    []batchv1.Job{aSet, bNil},
+			input:    []*batchv1.Job{aSet, bNil},
-			expected: []batchv1.Job{aSet, bNil},
+			expected: []*batchv1.Job{aSet, bNil},
 		},
 		{
 			name:     "both have non-nil, equal start time",
-			input:    []batchv1.Job{bSet, aSet},
+			input:    []*batchv1.Job{bSet, aSet},
-			expected: []batchv1.Job{aSet, bSet},
+			expected: []*batchv1.Job{aSet, bSet},
 		},
 		{
 			name:     "both have non-nil, different start time",
-			input:    []batchv1.Job{aSetLater, bSet},
+			input:    []*batchv1.Job{aSetLater, bSet},
-			expected: []batchv1.Job{bSet, aSetLater},
+			expected: []*batchv1.Job{bSet, aSetLater},
 		},
 	}
 
@@ -277,84 +285,179 @@ func TestByJobStartTime(t *testing.T) {
 	}
 }
 
-func TestGetMostRecentScheduleTime(t *testing.T) {
+func TestMostRecentScheduleTime(t *testing.T) {
-	type args struct {
+	metav1TopOfTheHour := metav1.NewTime(*topOfTheHour())
-		earliestTime *time.Time
+	metav1HalfPastTheHour := metav1.NewTime(*deltaTimeAfterTopOfTheHour(30 * time.Minute))
-		now          time.Time
+	oneMinute := int64(60)
-		schedule     string
+
-	}
 	tests := []struct {
 		name                   string
-		args                   args
+		cj                     *batchv1.CronJob
-		expectedTime           *time.Time
+		includeSDS             bool
+		now                    time.Time
+		expectedEarliestTime   time.Time
+		expectedRecentTime     *time.Time
 		expectedNumberOfMisses int64
 		wantErr                bool
 	}{
 		{
 			name: "now before next schedule",
-			args: args{
+			cj: &batchv1.CronJob{
-				earliestTime: topOfTheHour(),
+				ObjectMeta: metav1.ObjectMeta{
-				now:          topOfTheHour().Add(time.Second * 30),
+					CreationTimestamp: metav1TopOfTheHour,
-				schedule:     "0 * * * *",
+				},
+				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{
+			cj: &batchv1.CronJob{
-				earliestTime: topOfTheHour(),
+				ObjectMeta: metav1.ObjectMeta{
-				now:          topOfTheHour().Add(time.Minute * 61),
+					CreationTimestamp: metav1TopOfTheHour,
-				schedule:     "0 * * * *",
+				},
+				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{
+			cj: &batchv1.CronJob{
-				earliestTime: deltaTimeAfterTopOfTheHour(time.Second * 10),
+				ObjectMeta: metav1.ObjectMeta{
-				now:          *deltaTimeAfterTopOfTheHour(time.Minute * 301),
+					CreationTimestamp: metav1.NewTime(*deltaTimeAfterTopOfTheHour(10 * time.Second)),
-				schedule:     "0 * * * *",
+				},
+				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{
+			cj: &batchv1.CronJob{
-				earliestTime: deltaTimeAfterTopOfTheHour(time.Second * 10),
+				ObjectMeta: metav1.ObjectMeta{
-				now:          *deltaTimeAfterTopOfTheHour(time.Hour * 1000000),
+					CreationTimestamp: metav1.NewTime(*deltaTimeAfterTopOfTheHour(10 * time.Second)),
-				schedule:     "59 23 31 2 *",
+				},
+				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 {
+			if gotEarliestTime.IsZero() {
-				t.Errorf("getMostRecentScheduleTime() got nil, want %v", tt.expectedTime)
+				t.Errorf("earliestTime should never be 0, want %v", tt.expectedEarliestTime)
 			}
-			if gotTime != nil && tt.expectedTime != nil && !gotTime.Equal(*tt.expectedTime) {
+			if !gotEarliestTime.Equal(tt.expectedEarliestTime) {
-				t.Errorf("getMostRecentScheduleTime() got = %v, want %v", gotTime, tt.expectedTime)
+				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)
 			}
 		})
 	}