From cb491a8d0f2759d2de4cd53da20fb0682c32c251 Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Tue, 28 Jun 2022 16:59:38 +0200 Subject: [PATCH] Cleanups in controller utils 1. Squash two identical sorters byTime 2. Move helper for searching active jobs into utils to exist next to its counterpart --- .../cronjob/cronjob_controllerv2.go | 21 +++-------- pkg/controller/cronjob/utils.go | 35 ++++++++----------- 2 files changed, 19 insertions(+), 37 deletions(-) diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go index 551861a1825..4d5a6c6a204 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2.go +++ b/pkg/controller/cronjob/cronjob_controllerv2.go @@ -431,13 +431,13 @@ func (jm *ControllerV2) syncCronJob( childrenJobs := make(map[types.UID]bool) for _, j := range jobs { childrenJobs[j.ObjectMeta.UID] = true - found := inActiveList(*cronJob, j.ObjectMeta.UID) + found := inActiveList(cronJob, j.ObjectMeta.UID) if !found && !IsJobFinished(j) { cjCopy, err := jm.cronJobControl.GetCronJob(ctx, cronJob.Namespace, cronJob.Name) if err != nil { return nil, nil, updateStatus, err } - if inActiveList(*cjCopy, j.ObjectMeta.UID) { + if inActiveList(cjCopy, j.ObjectMeta.UID) { cronJob = cjCopy continue } @@ -553,11 +553,11 @@ func (jm *ControllerV2) syncCronJob( t := nextScheduledTimeDuration(*cronJob, sched, now) return cronJob, t, updateStatus, nil } - if isJobInActiveList(&batchv1.Job{ + if inActiveListByName(cronJob, &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{ Name: getJobName(cronJob, *scheduledTime), Namespace: cronJob.Namespace, - }}, cronJob.Status.Active) || cronJob.Status.LastScheduleTime.Equal(&metav1.Time{Time: *scheduledTime}) { + }}) || 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) return cronJob, t, updateStatus, nil @@ -724,7 +724,7 @@ func (jm *ControllerV2) removeOldestJobs(cj *batchv1.CronJob, js []*batchv1.Job, klog.V(4).InfoS("Cleaning up jobs from CronJob list", "deletejobnum", numToDelete, "jobnum", len(js), "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName())) - sort.Sort(byJobStartTimeStar(js)) + sort.Sort(byJobStartTime(js)) for i := 0; i < numToDelete; i++ { klog.V(4).InfoS("Removing job from CronJob list", "job", js[i].Name, "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName())) if deleteJob(cj, js[i], jm.jobControl, jm.recorder) { @@ -734,17 +734,6 @@ func (jm *ControllerV2) removeOldestJobs(cj *batchv1.CronJob, js []*batchv1.Job, return updateStatus } -// 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 { - for _, j := range activeJobs { - if j.Name == job.Name && j.Namespace == job.Namespace { - return true - } - } - return false -} - // deleteJob reaps a job, deleting the job, the pods and the reference in the active list func deleteJob(cj *batchv1.CronJob, job *batchv1.Job, jc jobControlInterface, recorder record.EventRecorder) bool { nameForLog := fmt.Sprintf("%s/%s", cj.Namespace, cj.Name) diff --git a/pkg/controller/cronjob/utils.go b/pkg/controller/cronjob/utils.go index 3a16082cf26..2e6af912fba 100644 --- a/pkg/controller/cronjob/utils.go +++ b/pkg/controller/cronjob/utils.go @@ -33,7 +33,8 @@ import ( // Utilities for dealing with Jobs and CronJobs and time. -func inActiveList(cj batchv1.CronJob, uid types.UID) bool { +// inActiveList checks if cronjob's .status.active has a job with the same UID. +func inActiveList(cj *batchv1.CronJob, uid types.UID) bool { for _, j := range cj.Status.Active { if j.UID == uid { return true @@ -42,6 +43,17 @@ func inActiveList(cj batchv1.CronJob, uid types.UID) bool { return false } +// inActiveListByName checks if cronjob's status.active has a job with the same +// name and namespace. +func inActiveListByName(cj *batchv1.CronJob, job *batchv1.Job) bool { + for _, j := range cj.Status.Active { + if j.Name == job.Name && j.Namespace == job.Namespace { + return true + } + } + return false +} + func deleteFromActiveList(cj *batchv1.CronJob, uid types.UID) { if cj == nil { return @@ -200,7 +212,7 @@ func IsJobFinished(j *batchv1.Job) bool { } // byJobStartTime sorts a list of jobs by start timestamp, using their names as a tie breaker. -type byJobStartTime []batchv1.Job +type byJobStartTime []*batchv1.Job func (o byJobStartTime) Len() int { return len(o) } func (o byJobStartTime) Swap(i, j int) { o[i], o[j] = o[j], o[i] } @@ -217,22 +229,3 @@ 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) -}