Skip to content

Commit

Permalink
fix(cronjob): lastSuccessfullTime not set when successfulJobsHistoryL…
Browse files Browse the repository at this point in the history 
…imit equal to zero (kubernetes#122025)

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

* fix(cronjob): added tests for successfulJobsHistoryLimit mutations
  • Loading branch information
@lukashankeln
lukashankeln authored Jul 5, 2024
1 parent 95debfb commit 58c4400
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 24 deletions.
28 changes: 16 additions & 12 deletions pkg/controller/cronjob/cronjob_controllerv2.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

}
}

Expand Down
82 changes: 70 additions & 12 deletions pkg/controller/cronjob/cronjob_controllerv2_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -208,6 +209,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
// expectations
expectCreate bool
expectDelete bool
expectCompleted bool
expectActive int
expectedWarnings int
expectErr bool
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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": {
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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": {
Expand All @@ -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",
Expand All @@ -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": {
Expand All @@ -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",
Expand All @@ -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": {
Expand All @@ -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",
Expand All @@ -863,6 +886,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
expectUpdateStatus: true,
jobPresentInCJActiveStatus: true,
expectCompleted: true,
},

// Tests for time skews
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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
Expand Down Expand Up @@ -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",
Expand All @@ -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{
Expand All @@ -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
Expand All @@ -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
}
Expand All @@ -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)
Expand Down Expand Up @@ -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++
Expand Down

0 comments on commit 58c4400

Please sign in to comment.