github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-08-03 09:22:44 +00:00
Code Issues Projects Releases Wiki Activity

fix(cronjob): lastSuccessfullTime not set when successfulJobsHistoryLimit equal to zero (#122025)

Browse Source 
* fix(cronjob): lastSuccessfullTime not set when successfulJobsHistoryLimit equal to zero

* fix(cronjob): added tests for successfulJobsHistoryLimit mutations
This commit is contained in:
lukashankeln 2024-07-05 12:57:47 +02:00 committed by GitHub
parent 95debfb5b6
commit 58c44005cd
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 86 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
pkg/controller/cronjob/cronjob_controllerv2.go
View File

@ -444,21 +444,25 @@ func (jm *ControllerV2) syncCronJob(
// This could happen if we crashed right after creating the Job and before updating the status, // 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 // 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. // a job that they wanted us to adopt.
} else if found && jobutil.IsJobFinished(j) { } else if jobutil.IsJobFinished(j) {
_, condition := jobutil.FinishedCondition(j) if found {
deleteFromActiveList(cronJob, j.ObjectMeta.UID) _, condition := jobutil.FinishedCondition(j)
jm.recorder.Eventf(cronJob, corev1.EventTypeNormal, "SawCompletedJob", "Saw completed job: %s, condition: %v", j.Name, condition) deleteFromActiveList(cronJob, j.ObjectMeta.UID)
updateStatus = true jm.recorder.Eventf(cronJob, corev1.EventTypeNormal, "SawCompletedJob", "Saw completed job: %s, condition: %v", j.Name, condition)
} 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
updateStatus = true updateStatus = true
} }
if j.Status.CompletionTime != nil && j.Status.CompletionTime.After(cronJob.Status.LastSuccessfulTime.Time) { if jobutil.IsJobSucceeded(j) {
cronJob.Status.LastSuccessfulTime = j.Status.CompletionTime // a job does not have to be in active list, as long as it has completed successfully, we will process the timestamp
updateStatus = true 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
}
} }
} }
} }

82
pkg/controller/cronjob/cronjob_controllerv2_test.go
View File

@ -187,11 +187,12 @@ func TestControllerV2SyncCronJob(t *testing.T) {
testCases := map[string]struct { testCases := map[string]struct {
// cj spec // cj spec
concurrencyPolicy batchv1.ConcurrencyPolicy concurrencyPolicy batchv1.ConcurrencyPolicy
suspend bool suspend bool
schedule string schedule string
timeZone *string timeZone *string
deadline int64 deadline int64
successfulJobsHistoryLimit *int32
// cj status // cj status
ranPreviously bool ranPreviously bool
@ -208,6 +209,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
// expectations // expectations
expectCreate bool expectCreate bool
expectDelete bool expectDelete bool
expectCompleted bool
expectActive int expectActive int
expectedWarnings int expectedWarnings int
expectErr bool expectErr bool
@ -419,6 +421,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, expectedRequeueDuration: 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, not time, F": { "prev ran but done, not time, F": {
concurrencyPolicy: "Forbid", concurrencyPolicy: "Forbid",
@ -431,6 +434,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, expectedRequeueDuration: 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, not time, R": { "prev ran but done, not time, R": {
concurrencyPolicy: "Replace", concurrencyPolicy: "Replace",
@ -443,6 +447,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Minute + nextScheduleDelta, expectedRequeueDuration: 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, is time, A": { "prev ran but done, is time, A": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -457,6 +462,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, is time, create job failed, A": { "prev ran but done, is time, create job failed, A": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -469,6 +475,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectErr: false, expectErr: false,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, is time, job not present in CJ active status, create job failed, A": { "prev ran but done, is time, job not present in CJ active status, create job failed, A": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -495,6 +502,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, is time, R": { "prev ran but done, is time, R": {
concurrencyPolicy: "Replace", concurrencyPolicy: "Replace",
@ -509,6 +517,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, is time, suspended": { "prev ran but done, is time, suspended": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -520,6 +529,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
now: *justAfterTheHour(), now: *justAfterTheHour(),
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, is time, past deadline": { "prev ran but done, is time, past deadline": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -532,6 +542,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, is time, not past deadline": { "prev ran but done, is time, not past deadline": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -546,6 +557,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"still active, not time, A": { "still active, not time, A": {
@ -701,6 +713,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, not past deadline, R": { "prev ran but done, long overdue, not past deadline, R": {
concurrencyPolicy: "Replace", concurrencyPolicy: "Replace",
@ -716,6 +729,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, not past deadline, F": { "prev ran but done, long overdue, not past deadline, F": {
concurrencyPolicy: "Forbid", concurrencyPolicy: "Forbid",
@ -731,6 +745,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, no deadline, A": { "prev ran but done, long overdue, no deadline, A": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -746,6 +761,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, no deadline, R": { "prev ran but done, long overdue, no deadline, R": {
concurrencyPolicy: "Replace", concurrencyPolicy: "Replace",
@ -761,6 +777,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, no deadline, F": { "prev ran but done, long overdue, no deadline, F": {
concurrencyPolicy: "Forbid", concurrencyPolicy: "Forbid",
@ -776,6 +793,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, past medium deadline, A": { "prev ran but done, long overdue, past medium deadline, A": {
@ -791,6 +809,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, past short deadline, A": { "prev ran but done, long overdue, past short deadline, A": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -805,6 +824,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, past medium deadline, R": { "prev ran but done, long overdue, past medium deadline, R": {
@ -820,6 +840,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, past short deadline, R": { "prev ran but done, long overdue, past short deadline, R": {
concurrencyPolicy: "Replace", concurrencyPolicy: "Replace",
@ -834,6 +855,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, past medium deadline, F": { "prev ran but done, long overdue, past medium deadline, F": {
@ -849,6 +871,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"prev ran but done, long overdue, past short deadline, F": { "prev ran but done, long overdue, past short deadline, F": {
concurrencyPolicy: "Forbid", concurrencyPolicy: "Forbid",
@ -863,6 +886,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
// Tests for time skews // Tests for time skews
@ -1026,6 +1050,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"with @every schedule, prev ran but done, is time": { "with @every schedule, prev ran but done, is time": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -1042,6 +1067,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"with @every schedule, prev ran but done, is time, past deadline": { "with @every schedule, prev ran but done, is time, past deadline": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -1056,6 +1082,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
// This test will fail: the logic around StartingDeadlineSecond in getNextScheduleTime messes up // 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 // 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, expectedRequeueDuration: 1*time.Minute + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"with @every schedule, prev ran but done, long overdue, past deadline": { "with @every schedule, prev ran but done, long overdue, past deadline": {
concurrencyPolicy: "Allow", concurrencyPolicy: "Allow",
@ -1172,6 +1200,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta, expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta,
expectUpdateStatus: true, expectUpdateStatus: true,
jobPresentInCJActiveStatus: true, jobPresentInCJActiveStatus: true,
expectCompleted: true,
}, },
"do nothing if the namespace is terminating": { "do nothing if the namespace is terminating": {
jobCreateError: &errors.StatusError{ErrStatus: metav1.Status{Details: &metav1.StatusDetails{Causes: []metav1.StatusCause{ jobCreateError: &errors.StatusError{ErrStatus: metav1.Status{Details: &metav1.StatusDetails{Causes: []metav1.StatusCause{
@ -1193,6 +1222,29 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectErr: true, expectErr: true,
jobPresentInCJActiveStatus: false, 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 { for name, tc := range testCases {
name := name name := name
@ -1204,6 +1256,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
cj.Spec.Suspend = &tc.suspend cj.Spec.Suspend = &tc.suspend
cj.Spec.Schedule = tc.schedule cj.Spec.Schedule = tc.schedule
cj.Spec.TimeZone = tc.timeZone cj.Spec.TimeZone = tc.timeZone
cj.Spec.SuccessfulJobsHistoryLimit = tc.successfulJobsHistoryLimit
if tc.deadline != noDead { if tc.deadline != noDead {
cj.Spec.StartingDeadlineSeconds = &tc.deadline cj.Spec.StartingDeadlineSeconds = &tc.deadline
} }
@ -1229,14 +1282,16 @@ func TestControllerV2SyncCronJob(t *testing.T) {
} }
job.UID = "1234" job.UID = "1234"
job.Namespace = cj.Namespace 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 { 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} realCJ.Status.Active = []v1.ObjectReference{*ref}
if !tc.jobStillNotFoundInLister { if !tc.jobStillNotFoundInLister {
js = append(js, job) js = append(js, job)
@ -1341,6 +1396,9 @@ func TestControllerV2SyncCronJob(t *testing.T) {
if tc.expectDelete { if tc.expectDelete {
expectedEvents++ expectedEvents++
} }
if tc.expectCompleted {
expectedEvents++
}
if name == "still active, is time, F" { if name == "still active, is time, F" {
// this is the only test case where we would raise an event for not scheduling // this is the only test case where we would raise an event for not scheduling
expectedEvents++ expectedEvents++