From 58c44005cdaec53fe3cb49b2d7a308df3af2d081 Mon Sep 17 00:00:00 2001 From: lukashankeln <50749658+lukashankeln@users.noreply.github.com> Date: Fri, 5 Jul 2024 12:57:47 +0200 Subject: [PATCH] fix(cronjob): lastSuccessfullTime not set when successfulJobsHistoryLimit equal to zero (#122025) * fix(cronjob): lastSuccessfullTime not set when successfulJobsHistoryLimit equal to zero * fix(cronjob): added tests for successfulJobsHistoryLimit mutations --- .../cronjob/cronjob_controllerv2.go | 28 ++++--- .../cronjob/cronjob_controllerv2_test.go | 82 ++++++++++++++++--- 2 files changed, 86 insertions(+), 24 deletions(-) diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go index ef623bf3e1b61..f67bd466ba213 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2.go +++ b/pkg/controller/cronjob/cronjob_controllerv2.go @@ -444,21 +444,25 @@ func (jm *ControllerV2) syncCronJob( // 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 && jobutil.IsJobFinished(j) { - _, condition := jobutil.FinishedCondition(j) - deleteFromActiveList(cronJob, j.ObjectMeta.UID) - jm.recorder.Eventf(cronJob, corev1.EventTypeNormal, "SawCompletedJob", "Saw completed job: %s, condition: %v", j.Name, condition) - updateStatus = true - } else if jobutil.IsJobSucceeded(j) { - // a job does not have to be in active list, as long as it has completed successfully, we will process the timestamp - if cronJob.Status.LastSuccessfulTime == nil { - cronJob.Status.LastSuccessfulTime = j.Status.CompletionTime + } else if jobutil.IsJobFinished(j) { + if found { + _, condition := jobutil.FinishedCondition(j) + deleteFromActiveList(cronJob, j.ObjectMeta.UID) + jm.recorder.Eventf(cronJob, corev1.EventTypeNormal, "SawCompletedJob", "Saw completed job: %s, condition: %v", j.Name, condition) updateStatus = true } - if j.Status.CompletionTime != nil && j.Status.CompletionTime.After(cronJob.Status.LastSuccessfulTime.Time) { - cronJob.Status.LastSuccessfulTime = j.Status.CompletionTime - updateStatus = true + if jobutil.IsJobSucceeded(j) { + // a job does not have to be in active list, as long as it has completed successfully, we will process the timestamp + if cronJob.Status.LastSuccessfulTime == nil { + cronJob.Status.LastSuccessfulTime = j.Status.CompletionTime + updateStatus = true + } + if j.Status.CompletionTime != nil && j.Status.CompletionTime.After(cronJob.Status.LastSuccessfulTime.Time) { + cronJob.Status.LastSuccessfulTime = j.Status.CompletionTime + updateStatus = true + } } + } } diff --git a/pkg/controller/cronjob/cronjob_controllerv2_test.go b/pkg/controller/cronjob/cronjob_controllerv2_test.go index 6859f5187e958..32297342c7f3c 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2_test.go +++ b/pkg/controller/cronjob/cronjob_controllerv2_test.go @@ -187,11 +187,12 @@ func TestControllerV2SyncCronJob(t *testing.T) { testCases := map[string]struct { // cj spec - concurrencyPolicy batchv1.ConcurrencyPolicy - suspend bool - schedule string - timeZone *string - deadline int64 + concurrencyPolicy batchv1.ConcurrencyPolicy + suspend bool + schedule string + timeZone *string + deadline int64 + successfulJobsHistoryLimit *int32 // cj status ranPreviously bool @@ -208,6 +209,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { // expectations expectCreate bool expectDelete bool + expectCompleted bool expectActive int expectedWarnings int expectErr bool @@ -419,6 +421,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, not time, F": { concurrencyPolicy: "Forbid", @@ -431,6 +434,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, not time, R": { concurrencyPolicy: "Replace", @@ -443,6 +447,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, is time, A": { concurrencyPolicy: "Allow", @@ -457,6 +462,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, is time, create job failed, A": { concurrencyPolicy: "Allow", @@ -469,6 +475,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectErr: false, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, is time, job not present in CJ active status, create job failed, A": { concurrencyPolicy: "Allow", @@ -495,6 +502,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, is time, R": { concurrencyPolicy: "Replace", @@ -509,6 +517,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, is time, suspended": { concurrencyPolicy: "Allow", @@ -520,6 +529,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { now: *justAfterTheHour(), expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, is time, past deadline": { concurrencyPolicy: "Allow", @@ -532,6 +542,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, is time, not past deadline": { concurrencyPolicy: "Allow", @@ -546,6 +557,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "still active, not time, A": { @@ -701,6 +713,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, not past deadline, R": { concurrencyPolicy: "Replace", @@ -716,6 +729,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, not past deadline, F": { concurrencyPolicy: "Forbid", @@ -731,6 +745,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, no deadline, A": { concurrencyPolicy: "Allow", @@ -746,6 +761,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, no deadline, R": { concurrencyPolicy: "Replace", @@ -761,6 +777,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, no deadline, F": { concurrencyPolicy: "Forbid", @@ -776,6 +793,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, past medium deadline, A": { @@ -791,6 +809,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, past short deadline, A": { concurrencyPolicy: "Allow", @@ -805,6 +824,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, past medium deadline, R": { @@ -820,6 +840,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, past short deadline, R": { concurrencyPolicy: "Replace", @@ -834,6 +855,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, past medium deadline, F": { @@ -849,6 +871,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "prev ran but done, long overdue, past short deadline, F": { concurrencyPolicy: "Forbid", @@ -863,6 +886,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, // Tests for time skews @@ -1026,6 +1050,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "with @every schedule, prev ran but done, is time": { concurrencyPolicy: "Allow", @@ -1042,6 +1067,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "with @every schedule, prev ran but done, is time, past deadline": { concurrencyPolicy: "Allow", @@ -1056,6 +1082,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, // This test will fail: the logic around StartingDeadlineSecond in getNextScheduleTime messes up // the time that calculating schedule.Next(earliestTime) is based on. While this works perfectly @@ -1157,6 +1184,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "with @every schedule, prev ran but done, long overdue, past deadline": { concurrencyPolicy: "Allow", @@ -1172,6 +1200,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta, expectUpdateStatus: true, jobPresentInCJActiveStatus: true, + expectCompleted: true, }, "do nothing if the namespace is terminating": { jobCreateError: &errors.StatusError{ErrStatus: metav1.Status{Details: &metav1.StatusDetails{Causes: []metav1.StatusCause{ @@ -1193,6 +1222,29 @@ func TestControllerV2SyncCronJob(t *testing.T) { expectErr: true, jobPresentInCJActiveStatus: false, }, + "set lastsuccessfultime if successfulJobHistoryLimit is zero": { + successfulJobsHistoryLimit: pointer.Int32(0), + ranPreviously: true, + schedule: onTheHour, + expectUpdateStatus: true, + expectCompleted: true, + jobPresentInCJActiveStatus: true, + }, + "set lastsuccessfultime if successfulJobHistoryLimit is ten": { + successfulJobsHistoryLimit: pointer.Int32(10), + ranPreviously: true, + schedule: onTheHour, + expectUpdateStatus: true, + expectCompleted: true, + jobPresentInCJActiveStatus: true, + }, + "set lastsuccessfultime if successfulJobHistoryLimit is nil": { + ranPreviously: true, + schedule: onTheHour, + expectUpdateStatus: true, + expectCompleted: true, + jobPresentInCJActiveStatus: true, + }, } for name, tc := range testCases { name := name @@ -1204,6 +1256,7 @@ func TestControllerV2SyncCronJob(t *testing.T) { cj.Spec.Suspend = &tc.suspend cj.Spec.Schedule = tc.schedule cj.Spec.TimeZone = tc.timeZone + cj.Spec.SuccessfulJobsHistoryLimit = tc.successfulJobsHistoryLimit if tc.deadline != noDead { cj.Spec.StartingDeadlineSeconds = &tc.deadline } @@ -1229,14 +1282,16 @@ func TestControllerV2SyncCronJob(t *testing.T) { } job.UID = "1234" job.Namespace = cj.Namespace + + ref, err := getRef(job) + if err != nil { + t.Fatalf("%s: unexpected error getting the job object reference: %v", name, err) + } + if tc.jobPresentInCJActiveStatus { + cj.Status.Active = []v1.ObjectReference{*ref} + } + if tc.stillActive { - ref, err := getRef(job) - if err != nil { - t.Fatalf("%s: unexpected error getting the job object reference: %v", name, err) - } - if tc.jobPresentInCJActiveStatus { - cj.Status.Active = []v1.ObjectReference{*ref} - } realCJ.Status.Active = []v1.ObjectReference{*ref} if !tc.jobStillNotFoundInLister { js = append(js, job) @@ -1341,6 +1396,9 @@ func TestControllerV2SyncCronJob(t *testing.T) { if tc.expectDelete { expectedEvents++ } + if tc.expectCompleted { + expectedEvents++ + } if name == "still active, is time, F" { // this is the only test case where we would raise an event for not scheduling expectedEvents++