diff --git a/pkg/controller/cronjob/BUILD b/pkg/controller/cronjob/BUILD index 632fcf0bb09..b60eae877d3 100644 --- a/pkg/controller/cronjob/BUILD +++ b/pkg/controller/cronjob/BUILD @@ -25,6 +25,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go index 13d27a480e1..cb9f6037015 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2.go +++ b/pkg/controller/cronjob/cronjob_controllerv2.go @@ -19,6 +19,7 @@ package cronjob import ( "fmt" "reflect" + "sort" "time" "github.com/robfig/cron" @@ -181,13 +182,13 @@ func (jm *ControllerV2) sync(cronJobKey string) (*time.Duration, error) { return nil, err } - cronJob, requeueAfter, err := jm.syncCronJob(cronJob, jobsToBeReconciled) + cronJobCopy, requeueAfter, err := jm.syncCronJob(cronJob, jobsToBeReconciled) if err != nil { klog.V(2).InfoS("error reconciling cronjob", "cronjob", klog.KRef(cronJob.GetNamespace(), cronJob.GetName()), "err", err) return nil, err } - err = jm.cleanupFinishedJobs(cronJob, jobsToBeReconciled) + err = jm.cleanupFinishedJobs(cronJobCopy, jobsToBeReconciled) if err != nil { klog.V(2).InfoS("error cleaning up jobs", "cronjob", klog.KRef(cronJob.GetNamespace(), cronJob.GetName()), "resourceVersion", cronJob.GetResourceVersion(), "err", err) return nil, err @@ -222,7 +223,7 @@ func (jm *ControllerV2) resolveControllerRef(namespace string, controllerRef *me return cronJob } -func (jm *ControllerV2) getJobsToBeReconciled(cronJob *batchv1beta1.CronJob) ([]batchv1.Job, error) { +func (jm *ControllerV2) getJobsToBeReconciled(cronJob *batchv1beta1.CronJob) ([]*batchv1.Job, error) { var jobSelector labels.Selector if len(cronJob.Spec.JobTemplate.Labels) == 0 { jobSelector = labels.Everything() @@ -234,13 +235,13 @@ func (jm *ControllerV2) getJobsToBeReconciled(cronJob *batchv1beta1.CronJob) ([] return nil, err } - jobsToBeReconciled := []batchv1.Job{} + jobsToBeReconciled := []*batchv1.Job{} for _, job := range jobList { // If it has a ControllerRef, that's all that matters. if controllerRef := metav1.GetControllerOf(job); controllerRef != nil && controllerRef.Name == cronJob.Name { // this job is needs to be reconciled - jobsToBeReconciled = append(jobsToBeReconciled, *job) + jobsToBeReconciled = append(jobsToBeReconciled, job) } } return jobsToBeReconciled, nil @@ -397,7 +398,7 @@ func (jm *ControllerV2) updateCronJob(old interface{}, curr interface{}) { // that mutates the object func (jm *ControllerV2) syncCronJob( cj *batchv1beta1.CronJob, - js []batchv1.Job) (*batchv1beta1.CronJob, *time.Duration, error) { + js []*batchv1.Job) (*batchv1beta1.CronJob, *time.Duration, error) { cj = cj.DeepCopy() now := jm.now() @@ -406,14 +407,22 @@ func (jm *ControllerV2) syncCronJob( for _, j := range js { childrenJobs[j.ObjectMeta.UID] = true found := inActiveList(*cj, j.ObjectMeta.UID) - if !found && !IsJobFinished(&j) { + if !found && !IsJobFinished(j) { + cjCopy, err := jm.cronJobControl.GetCronJob(cj.Namespace, cj.Name) + if err != nil { + return nil, nil, err + } + if inActiveList(*cjCopy, j.ObjectMeta.UID) { + cj = cjCopy + continue + } jm.recorder.Eventf(cj, corev1.EventTypeWarning, "UnexpectedJob", "Saw a job that the controller did not create or forgot: %s", j.Name) // We found an unfinished job that has us as the parent, but it is not in our Active list. // This could happen if we crashed right after creating the Job and before updating the status, // or if our jobs list is newer than our cj status after a relist, or if someone intentionally created // a job that they wanted us to adopt. - } else if found && IsJobFinished(&j) { - _, status := getFinishedStatus(&j) + } else if found && IsJobFinished(j) { + _, status := getFinishedStatus(j) deleteFromActiveList(cj, j.ObjectMeta.UID) jm.recorder.Eventf(cj, corev1.EventTypeNormal, "SawCompletedJob", "Saw completed job: %s, status: %v", j.Name, status) } @@ -597,17 +606,17 @@ func nextScheduledTimeDuration(sched cron.Schedule, now time.Time) *time.Duratio } // cleanupFinishedJobs cleanups finished jobs created by a CronJob -func (jm *ControllerV2) cleanupFinishedJobs(cj *batchv1beta1.CronJob, js []batchv1.Job) error { +func (jm *ControllerV2) cleanupFinishedJobs(cj *batchv1beta1.CronJob, js []*batchv1.Job) error { // If neither limits are active, there is no need to do anything. if cj.Spec.FailedJobsHistoryLimit == nil && cj.Spec.SuccessfulJobsHistoryLimit == nil { return nil } - failedJobs := []batchv1.Job{} - successfulJobs := []batchv1.Job{} + failedJobs := []*batchv1.Job{} + successfulJobs := []*batchv1.Job{} for _, job := range js { - isFinished, finishedStatus := getFinishedStatus(&job) + isFinished, finishedStatus := jm.getFinishedStatus(job) if isFinished && finishedStatus == batchv1.JobComplete { successfulJobs = append(successfulJobs, job) } else if isFinished && finishedStatus == batchv1.JobFailed { @@ -616,19 +625,15 @@ func (jm *ControllerV2) cleanupFinishedJobs(cj *batchv1beta1.CronJob, js []batch } if cj.Spec.SuccessfulJobsHistoryLimit != nil { - removeOldestJobs(cj, + jm.removeOldestJobs(cj, successfulJobs, - jm.jobControl, - *cj.Spec.SuccessfulJobsHistoryLimit, - jm.recorder) + *cj.Spec.SuccessfulJobsHistoryLimit) } if cj.Spec.FailedJobsHistoryLimit != nil { - removeOldestJobs(cj, + jm.removeOldestJobs(cj, failedJobs, - jm.jobControl, - *cj.Spec.FailedJobsHistoryLimit, - jm.recorder) + *cj.Spec.FailedJobsHistoryLimit) } // Update the CronJob, in case jobs were removed from the list. @@ -636,6 +641,32 @@ func (jm *ControllerV2) cleanupFinishedJobs(cj *batchv1beta1.CronJob, js []batch return err } +func (jm *ControllerV2) getFinishedStatus(j *batchv1.Job) (bool, batchv1.JobConditionType) { + for _, c := range j.Status.Conditions { + if (c.Type == batchv1.JobComplete || c.Type == batchv1.JobFailed) && c.Status == corev1.ConditionTrue { + return true, c.Type + } + } + return false, "" +} + +// removeOldestJobs removes the oldest jobs from a list of jobs +func (jm *ControllerV2) removeOldestJobs(cj *batchv1beta1.CronJob, js []*batchv1.Job, maxJobs int32) { + numToDelete := len(js) - int(maxJobs) + if numToDelete <= 0 { + return + } + + nameForLog := fmt.Sprintf("%s/%s", cj.Namespace, cj.Name) + klog.V(4).Infof("Cleaning up %d/%d jobs from %s", numToDelete, len(js), nameForLog) + + sort.Sort(byJobStartTimeStar(js)) + for i := 0; i < numToDelete; i++ { + klog.V(4).Infof("Removing job %s from %s", js[i].Name, nameForLog) + deleteJob(cj, js[i], jm.jobControl, jm.recorder) + } +} + // isJobInActiveList take a job and checks if activeJobs has a job with the same // name and namespace. func isJobInActiveList(job *batchv1.Job, activeJobs []corev1.ObjectReference) bool { diff --git a/pkg/controller/cronjob/cronjob_controllerv2_test.go b/pkg/controller/cronjob/cronjob_controllerv2_test.go index 4ba9e68293b..e17c46f7ac8 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2_test.go +++ b/pkg/controller/cronjob/cronjob_controllerv2_test.go @@ -76,74 +76,78 @@ func Test_syncOne2(t *testing.T) { now time.Time // expectations - expectCreate bool - expectDelete bool - expectActive int - expectedWarnings int - expectErr bool - expectRequeueAfter bool - jobStillNotFoundInLister bool + expectCreate bool + expectDelete bool + expectActive int + expectedWarnings int + expectErr bool + expectRequeueAfter bool + jobStillNotFoundInLister bool + jobPresentInCJActiveStatus bool }{ - "never ran, not valid schedule, A": {A, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F}, - "never ran, not valid schedule, F": {f, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F}, - "never ran, not valid schedule, R": {f, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F}, - "never ran, not time, A": {A, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F}, - "never ran, not time, F": {f, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F}, - "never ran, not time, R": {R, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F}, - "never ran, is time, A": {A, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F}, - "never ran, is time, F": {f, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F}, - "never ran, is time, R": {R, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F}, - "never ran, is time, suspended": {A, T, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, F, F}, - "never ran, is time, past deadline": {A, F, onTheHour, shortDead, F, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, T, F}, - "never ran, is time, not past deadline": {A, F, onTheHour, longDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F}, + "never ran, not valid schedule, A": {A, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F, T}, + "never ran, not valid schedule, F": {f, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F, T}, + "never ran, not valid schedule, R": {f, F, errorSchedule, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 1, F, F, F, T}, + "never ran, not time, A": {A, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, + "never ran, not time, F": {f, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, + "never ran, not time, R": {R, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, + "never ran, is time, A": {A, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "never ran, is time, F": {f, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "never ran, is time, R": {R, F, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "never ran, is time, suspended": {A, T, onTheHour, noDead, F, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, F, F, T}, + "never ran, is time, past deadline": {A, F, onTheHour, shortDead, F, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, T, F, T}, + "never ran, is time, not past deadline": {A, F, onTheHour, longDead, F, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "prev ran but done, not time, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F}, - "prev ran but done, not time, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F}, - "prev ran but done, not time, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F}, - "prev ran but done, is time, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F}, - "prev ran but done, is time, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F}, - "prev ran but done, is time, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F}, - "prev ran but done, is time, suspended": {A, T, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, F, F}, - "prev ran but done, is time, past deadline": {A, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, T, F}, - "prev ran but done, is time, not past deadline": {A, F, onTheHour, longDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F}, + "prev ran but done, not time, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, + "prev ran but done, not time, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, + "prev ran but done, not time, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, + "prev ran but done, is time, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "prev ran but done, is time, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "prev ran but done, is time, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "prev ran but done, is time, suspended": {A, T, onTheHour, noDead, T, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, F, F, T}, + "prev ran but done, is time, past deadline": {A, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), justAfterTheHour(), F, F, 0, 0, F, T, F, T}, + "prev ran but done, is time, not past deadline": {A, F, onTheHour, longDead, T, F, justAfterThePriorHour(), justAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "still active, not time, A": {A, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F}, - "still active, not time, F": {f, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F}, - "still active, not time, R": {R, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F}, - "still active, is time, A": {A, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), T, F, 2, 0, F, T, F}, - "still active, is time, F": {f, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), F, F, 1, 0, F, T, F}, - "still active, is time, R": {R, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), T, T, 1, 0, F, T, F}, - "still active, is time, suspended": {A, T, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), F, F, 1, 0, F, F, F}, - "still active, is time, past deadline": {A, F, onTheHour, shortDead, T, T, justAfterThePriorHour(), justAfterTheHour(), F, F, 1, 0, F, T, F}, - "still active, is time, not past deadline": {A, F, onTheHour, longDead, T, T, justAfterThePriorHour(), justAfterTheHour(), T, F, 2, 0, F, T, F}, + "still active, not time, A": {A, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F, T}, + "still active, not time, F": {f, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F, T}, + "still active, not time, R": {R, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justBeforeTheHour(), F, F, 1, 0, F, T, F, T}, + "still active, is time, A": {A, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), T, F, 2, 0, F, T, F, T}, + "still active, is time, F": {f, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), F, F, 1, 0, F, T, F, T}, + "still active, is time, R": {R, F, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), T, T, 1, 0, F, T, F, T}, + "still active, is time, suspended": {A, T, onTheHour, noDead, T, T, justAfterThePriorHour(), justAfterTheHour(), F, F, 1, 0, F, F, F, T}, + "still active, is time, past deadline": {A, F, onTheHour, shortDead, T, T, justAfterThePriorHour(), justAfterTheHour(), F, F, 1, 0, F, T, F, T}, + "still active, is time, not past deadline": {A, F, onTheHour, longDead, T, T, justAfterThePriorHour(), justAfterTheHour(), T, F, 2, 0, F, T, F, T}, // 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, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F}, - "prev ran but done, long overdue, not past deadline, R": {R, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F}, - "prev ran but done, long overdue, not past deadline, F": {f, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F}, - "prev ran but done, long overdue, no deadline, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F}, - "prev ran but done, long overdue, no deadline, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F}, - "prev ran but done, long overdue, no deadline, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F}, + "prev ran but done, long overdue, not past deadline, A": {A, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, + "prev ran but done, long overdue, not past deadline, R": {R, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, + "prev ran but done, long overdue, not past deadline, F": {f, F, onTheHour, longDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, + "prev ran but done, long overdue, no deadline, A": {A, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, + "prev ran but done, long overdue, no deadline, R": {R, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, + "prev ran but done, long overdue, no deadline, F": {f, F, onTheHour, noDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 1, F, T, F, T}, - "prev ran but done, long overdue, past medium deadline, A": {A, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F}, - "prev ran but done, long overdue, past short deadline, A": {A, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F}, + "prev ran but done, long overdue, past medium deadline, A": {A, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "prev ran but done, long overdue, past short deadline, A": {A, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "prev ran but done, long overdue, past medium deadline, R": {R, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F}, - "prev ran but done, long overdue, past short deadline, R": {R, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F}, + "prev ran but done, long overdue, past medium deadline, R": {R, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "prev ran but done, long overdue, past short deadline, R": {R, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, - "prev ran but done, long overdue, past medium deadline, F": {f, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F}, - "prev ran but done, long overdue, past short deadline, F": {f, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F}, + "prev ran but done, long overdue, past medium deadline, F": {f, F, onTheHour, mediumDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, + "prev ran but done, long overdue, past short deadline, F": {f, F, onTheHour, shortDead, T, F, justAfterThePriorHour(), weekAfterTheHour(), T, F, 1, 0, F, T, F, T}, // Tests for time skews - "this ran but done, time drifted back, F": {f, F, onTheHour, noDead, T, F, justAfterTheHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F}, + "this ran but done, time drifted back, F": {f, F, onTheHour, noDead, T, F, justAfterTheHour(), justBeforeTheHour(), F, F, 0, 0, F, T, F, T}, // Tests for slow job lister - "this started but went missing, not past deadline, A": {A, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T}, - "this started but went missing, not past deadline, f": {f, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T}, - "this started but went missing, not past deadline, R": {R, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T}, + "this started but went missing, not past deadline, A": {A, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T, T}, + "this started but went missing, not past deadline, f": {f, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T, T}, + "this started but went missing, not past deadline, R": {R, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, T, T}, - // TODO: alpatel add tests for slow cronjob lister + // Tests for slow cronjob list + "this started but is not present in cronjob active list, not past deadline, A": {A, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, F, F}, + "this started but is not present in cronjob active list, not past deadline, f": {f, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, F, F}, + "this started but is not present in cronjob active list, not past deadline, R": {R, F, onTheHour, longDead, T, T, topOfTheHour().Add(time.Millisecond * 100), justAfterTheHour().Add(time.Millisecond * 100), F, F, 1, 0, F, T, F, F}, } for name, tc := range testCases { name := name @@ -164,7 +168,8 @@ func Test_syncOne2(t *testing.T) { job *batchv1.Job err error ) - js := []batchv1.Job{} + js := []*batchv1.Job{} + realCJ := cj.DeepCopy() if tc.ranPreviously { cj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeThePriorHour()} cj.Status.LastScheduleTime = &metav1.Time{Time: justAfterThePriorHour()} @@ -179,9 +184,12 @@ func Test_syncOne2(t *testing.T) { if err != nil { t.Fatalf("%s: unexpected error getting the job object reference: %v", name, err) } - cj.Status.Active = []v1.ObjectReference{*ref} + if tc.jobPresentInCJActiveStatus { + cj.Status.Active = []v1.ObjectReference{*ref} + } + realCJ.Status.Active = []v1.ObjectReference{*ref} if !tc.jobStillNotFoundInLister { - js = append(js, *job) + js = append(js, job) } } } else { @@ -192,7 +200,7 @@ func Test_syncOne2(t *testing.T) { } jc := &fakeJobControl{Job: job} - cjc := &fakeCJControl{} + cjc := &fakeCJControl{CronJob: realCJ} recorder := record.NewFakeRecorder(10) jm := ControllerV2{ @@ -458,7 +466,7 @@ func TestControllerV2_getJobList(t *testing.T) { name string fields fields args args - want []batchv1.Job + want []*batchv1.Job wantErr bool }{ { @@ -476,7 +484,7 @@ func TestControllerV2_getJobList(t *testing.T) { }, }}}, args: args{cronJob: &batchv1beta1.CronJob{ObjectMeta: metav1.ObjectMeta{Namespace: "foo-ns", Name: "fooer"}}}, - want: []batchv1.Job{}, + want: []*batchv1.Job{}, }, { name: "test getting jobs in namespace with a controller reference", @@ -499,7 +507,7 @@ func TestControllerV2_getJobList(t *testing.T) { }, }}}, args: args{cronJob: &batchv1beta1.CronJob{ObjectMeta: metav1.ObjectMeta{Namespace: "foo-ns", Name: "fooer"}}}, - want: []batchv1.Job{{ + want: []*batchv1.Job{{ ObjectMeta: metav1.ObjectMeta{Name: "foo1", Namespace: "foo-ns", OwnerReferences: []metav1.OwnerReference{ { @@ -524,7 +532,7 @@ func TestControllerV2_getJobList(t *testing.T) { }, }}}, args: args{cronJob: &batchv1beta1.CronJob{ObjectMeta: metav1.ObjectMeta{Namespace: "foo-ns", Name: "fooer"}}}, - want: []batchv1.Job{}, + want: []*batchv1.Job{}, }, } for _, tt := range tests { diff --git a/pkg/controller/cronjob/injection.go b/pkg/controller/cronjob/injection.go index db7cb71b91e..0247550d80d 100644 --- a/pkg/controller/cronjob/injection.go +++ b/pkg/controller/cronjob/injection.go @@ -19,6 +19,8 @@ package cronjob import ( "context" "fmt" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" "sync" batchv1 "k8s.io/api/batch/v1" @@ -56,11 +58,18 @@ func (c *realCJControl) UpdateStatus(cj *batchv1beta1.CronJob) (*batchv1beta1.Cr // fakeCJControl is the default implementation of cjControlInterface. type fakeCJControl struct { + CronJob *batchv1beta1.CronJob Updates []batchv1beta1.CronJob } func (c *fakeCJControl) GetCronJob(namespace, name string) (*batchv1beta1.CronJob, error) { - panic("implement me") + if name == c.CronJob.Name && namespace == c.CronJob.Namespace { + return c.CronJob, nil + } + return nil, errors.NewNotFound(schema.GroupResource{ + Group: "v1beta1", + Resource: "cronjobs", + }, name) } var _ cjControlInterface = &fakeCJControl{} diff --git a/pkg/controller/cronjob/utils.go b/pkg/controller/cronjob/utils.go index 29dcab3144b..e78bc6d9a0d 100644 --- a/pkg/controller/cronjob/utils.go +++ b/pkg/controller/cronjob/utils.go @@ -298,3 +298,22 @@ func (o byJobStartTime) Less(i, j int) bool { } return o[i].Status.StartTime.Before(o[j].Status.StartTime) } + +// byJobStartTimeStar sorts a list of jobs by start timestamp, using their names as a tie breaker. +type byJobStartTimeStar []*batchv1.Job + +func (o byJobStartTimeStar) Len() int { return len(o) } +func (o byJobStartTimeStar) Swap(i, j int) { o[i], o[j] = o[j], o[i] } + +func (o byJobStartTimeStar) Less(i, j int) bool { + if o[i].Status.StartTime == nil && o[j].Status.StartTime != nil { + return false + } + if o[i].Status.StartTime != nil && o[j].Status.StartTime == nil { + return true + } + if o[i].Status.StartTime.Equal(o[j].Status.StartTime) { + return o[i].Name < o[j].Name + } + return o[i].Status.StartTime.Before(o[j].Status.StartTime) +}