Skip to content

Commit 58c4400

Browse files
lukashankelnlukashankeln
authored
fix(cronjob): lastSuccessfullTime not set when successfulJobsHistoryLimit equal to zero (kubernetes#122025)
* fix(cronjob): lastSuccessfullTime not set when successfulJobsHistoryLimit equal to zero * fix(cronjob): added tests for successfulJobsHistoryLimit mutations
1 parent 95debfb commit 58c4400

File tree

2 files changed

+86
-24
lines changed

2 files changed

+86
-24
lines changed

‎pkg/controller/cronjob/cronjob_controllerv2.go‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -444,21 +444,25 @@ func (jm *ControllerV2) syncCronJob(
444444
// This could happen if we crashed right after creating the Job and before updating the status,
445445
// or if our jobs list is newer than our cj status after a relist, or if someone intentionally created
446446
// a job that they wanted us to adopt.
447-
} else if found && jobutil.IsJobFinished(j) {
448-
_, condition := jobutil.FinishedCondition(j)
449-
deleteFromActiveList(cronJob, j.ObjectMeta.UID)
450-
jm.recorder.Eventf(cronJob, corev1.EventTypeNormal, "SawCompletedJob", "Saw completed job: %s, condition: %v", j.Name, condition)
451-
updateStatus = true
452-
} else if jobutil.IsJobSucceeded(j) {
453-
// a job does not have to be in active list, as long as it has completed successfully, we will process the timestamp
454-
if cronJob.Status.LastSuccessfulTime == nil {
455-
cronJob.Status.LastSuccessfulTime = j.Status.CompletionTime
447+
} else if jobutil.IsJobFinished(j) {
448+
if found {
449+
_, condition := jobutil.FinishedCondition(j)
450+
deleteFromActiveList(cronJob, j.ObjectMeta.UID)
451+
jm.recorder.Eventf(cronJob, corev1.EventTypeNormal, "SawCompletedJob", "Saw completed job: %s, condition: %v", j.Name, condition)
456452
updateStatus = true
457453
}
458-
if j.Status.CompletionTime != nil && j.Status.CompletionTime.After(cronJob.Status.LastSuccessfulTime.Time) {
459-
cronJob.Status.LastSuccessfulTime = j.Status.CompletionTime
460-
updateStatus = true
454+
if jobutil.IsJobSucceeded(j) {
455+
// a job does not have to be in active list, as long as it has completed successfully, we will process the timestamp
456+
if cronJob.Status.LastSuccessfulTime == nil {
457+
cronJob.Status.LastSuccessfulTime = j.Status.CompletionTime
458+
updateStatus = true
459+
}
460+
if j.Status.CompletionTime != nil && j.Status.CompletionTime.After(cronJob.Status.LastSuccessfulTime.Time) {
461+
cronJob.Status.LastSuccessfulTime = j.Status.CompletionTime
462+
updateStatus = true
463+
}
461464
}
465+
462466
}
463467
}
464468

‎pkg/controller/cronjob/cronjob_controllerv2_test.go‎

Lines changed: 70 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -187,11 +187,12 @@ func TestControllerV2SyncCronJob(t *testing.T) {
187187

188188
testCases := map[string]struct {
189189
// cj spec
190-
concurrencyPolicy batchv1.ConcurrencyPolicy
191-
suspend bool
192-
schedule string
193-
timeZone *string
194-
deadline int64
190+
concurrencyPolicy batchv1.ConcurrencyPolicy
191+
suspend bool
192+
schedule string
193+
timeZone *string
194+
deadline int64
195+
successfulJobsHistoryLimit *int32
195196

196197
// cj status
197198
ranPreviously bool
@@ -208,6 +209,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
208209
// expectations
209210
expectCreate bool
210211
expectDelete bool
212+
expectCompleted bool
211213
expectActive int
212214
expectedWarnings int
213215
expectErr bool
@@ -419,6 +421,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
419421
expectedRequeueDuration: 1*time.Minute + nextScheduleDelta,
420422
expectUpdateStatus: true,
421423
jobPresentInCJActiveStatus: true,
424+
expectCompleted: true,
422425
},
423426
"prev ran but done, not time, F": {
424427
concurrencyPolicy: "Forbid",
@@ -431,6 +434,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
431434
expectedRequeueDuration: 1*time.Minute + nextScheduleDelta,
432435
expectUpdateStatus: true,
433436
jobPresentInCJActiveStatus: true,
437+
expectCompleted: true,
434438
},
435439
"prev ran but done, not time, R": {
436440
concurrencyPolicy: "Replace",
@@ -443,6 +447,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
443447
expectedRequeueDuration: 1*time.Minute + nextScheduleDelta,
444448
expectUpdateStatus: true,
445449
jobPresentInCJActiveStatus: true,
450+
expectCompleted: true,
446451
},
447452
"prev ran but done, is time, A": {
448453
concurrencyPolicy: "Allow",
@@ -457,6 +462,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
457462
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
458463
expectUpdateStatus: true,
459464
jobPresentInCJActiveStatus: true,
465+
expectCompleted: true,
460466
},
461467
"prev ran but done, is time, create job failed, A": {
462468
concurrencyPolicy: "Allow",
@@ -469,6 +475,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
469475
expectErr: false,
470476
expectUpdateStatus: true,
471477
jobPresentInCJActiveStatus: true,
478+
expectCompleted: true,
472479
},
473480
"prev ran but done, is time, job not present in CJ active status, create job failed, A": {
474481
concurrencyPolicy: "Allow",
@@ -495,6 +502,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
495502
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
496503
expectUpdateStatus: true,
497504
jobPresentInCJActiveStatus: true,
505+
expectCompleted: true,
498506
},
499507
"prev ran but done, is time, R": {
500508
concurrencyPolicy: "Replace",
@@ -509,6 +517,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
509517
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
510518
expectUpdateStatus: true,
511519
jobPresentInCJActiveStatus: true,
520+
expectCompleted: true,
512521
},
513522
"prev ran but done, is time, suspended": {
514523
concurrencyPolicy: "Allow",
@@ -520,6 +529,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
520529
now: *justAfterTheHour(),
521530
expectUpdateStatus: true,
522531
jobPresentInCJActiveStatus: true,
532+
expectCompleted: true,
523533
},
524534
"prev ran but done, is time, past deadline": {
525535
concurrencyPolicy: "Allow",
@@ -532,6 +542,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
532542
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
533543
expectUpdateStatus: true,
534544
jobPresentInCJActiveStatus: true,
545+
expectCompleted: true,
535546
},
536547
"prev ran but done, is time, not past deadline": {
537548
concurrencyPolicy: "Allow",
@@ -546,6 +557,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
546557
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
547558
expectUpdateStatus: true,
548559
jobPresentInCJActiveStatus: true,
560+
expectCompleted: true,
549561
},
550562

551563
"still active, not time, A": {
@@ -701,6 +713,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
701713
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
702714
expectUpdateStatus: true,
703715
jobPresentInCJActiveStatus: true,
716+
expectCompleted: true,
704717
},
705718
"prev ran but done, long overdue, not past deadline, R": {
706719
concurrencyPolicy: "Replace",
@@ -716,6 +729,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
716729
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
717730
expectUpdateStatus: true,
718731
jobPresentInCJActiveStatus: true,
732+
expectCompleted: true,
719733
},
720734
"prev ran but done, long overdue, not past deadline, F": {
721735
concurrencyPolicy: "Forbid",
@@ -731,6 +745,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
731745
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
732746
expectUpdateStatus: true,
733747
jobPresentInCJActiveStatus: true,
748+
expectCompleted: true,
734749
},
735750
"prev ran but done, long overdue, no deadline, A": {
736751
concurrencyPolicy: "Allow",
@@ -746,6 +761,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
746761
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
747762
expectUpdateStatus: true,
748763
jobPresentInCJActiveStatus: true,
764+
expectCompleted: true,
749765
},
750766
"prev ran but done, long overdue, no deadline, R": {
751767
concurrencyPolicy: "Replace",
@@ -761,6 +777,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
761777
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
762778
expectUpdateStatus: true,
763779
jobPresentInCJActiveStatus: true,
780+
expectCompleted: true,
764781
},
765782
"prev ran but done, long overdue, no deadline, F": {
766783
concurrencyPolicy: "Forbid",
@@ -776,6 +793,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
776793
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
777794
expectUpdateStatus: true,
778795
jobPresentInCJActiveStatus: true,
796+
expectCompleted: true,
779797
},
780798

781799
"prev ran but done, long overdue, past medium deadline, A": {
@@ -791,6 +809,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
791809
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
792810
expectUpdateStatus: true,
793811
jobPresentInCJActiveStatus: true,
812+
expectCompleted: true,
794813
},
795814
"prev ran but done, long overdue, past short deadline, A": {
796815
concurrencyPolicy: "Allow",
@@ -805,6 +824,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
805824
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
806825
expectUpdateStatus: true,
807826
jobPresentInCJActiveStatus: true,
827+
expectCompleted: true,
808828
},
809829

810830
"prev ran but done, long overdue, past medium deadline, R": {
@@ -820,6 +840,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
820840
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
821841
expectUpdateStatus: true,
822842
jobPresentInCJActiveStatus: true,
843+
expectCompleted: true,
823844
},
824845
"prev ran but done, long overdue, past short deadline, R": {
825846
concurrencyPolicy: "Replace",
@@ -834,6 +855,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
834855
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
835856
expectUpdateStatus: true,
836857
jobPresentInCJActiveStatus: true,
858+
expectCompleted: true,
837859
},
838860

839861
"prev ran but done, long overdue, past medium deadline, F": {
@@ -849,6 +871,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
849871
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
850872
expectUpdateStatus: true,
851873
jobPresentInCJActiveStatus: true,
874+
expectCompleted: true,
852875
},
853876
"prev ran but done, long overdue, past short deadline, F": {
854877
concurrencyPolicy: "Forbid",
@@ -863,6 +886,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
863886
expectedRequeueDuration: 1*time.Hour + nextScheduleDelta,
864887
expectUpdateStatus: true,
865888
jobPresentInCJActiveStatus: true,
889+
expectCompleted: true,
866890
},
867891

868892
// Tests for time skews
@@ -1026,6 +1050,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
10261050
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
10271051
expectUpdateStatus: true,
10281052
jobPresentInCJActiveStatus: true,
1053+
expectCompleted: true,
10291054
},
10301055
"with @every schedule, prev ran but done, is time": {
10311056
concurrencyPolicy: "Allow",
@@ -1042,6 +1067,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
10421067
expectedRequeueDuration: 1*time.Hour - 1*time.Minute + nextScheduleDelta,
10431068
expectUpdateStatus: true,
10441069
jobPresentInCJActiveStatus: true,
1070+
expectCompleted: true,
10451071
},
10461072
"with @every schedule, prev ran but done, is time, past deadline": {
10471073
concurrencyPolicy: "Allow",
@@ -1056,6 +1082,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
10561082
expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta,
10571083
expectUpdateStatus: true,
10581084
jobPresentInCJActiveStatus: true,
1085+
expectCompleted: true,
10591086
},
10601087
// This test will fail: the logic around StartingDeadlineSecond in getNextScheduleTime messes up
10611088
// the time that calculating schedule.Next(earliestTime) is based on. While this works perfectly
@@ -1157,6 +1184,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
11571184
expectedRequeueDuration: 1*time.Minute + nextScheduleDelta,
11581185
expectUpdateStatus: true,
11591186
jobPresentInCJActiveStatus: true,
1187+
expectCompleted: true,
11601188
},
11611189
"with @every schedule, prev ran but done, long overdue, past deadline": {
11621190
concurrencyPolicy: "Allow",
@@ -1172,6 +1200,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
11721200
expectedRequeueDuration: 1*time.Hour - time.Second*time.Duration(shortDead+1) + nextScheduleDelta,
11731201
expectUpdateStatus: true,
11741202
jobPresentInCJActiveStatus: true,
1203+
expectCompleted: true,
11751204
},
11761205
"do nothing if the namespace is terminating": {
11771206
jobCreateError: &errors.StatusError{ErrStatus: metav1.Status{Details: &metav1.StatusDetails{Causes: []metav1.StatusCause{
@@ -1193,6 +1222,29 @@ func TestControllerV2SyncCronJob(t *testing.T) {
11931222
expectErr: true,
11941223
jobPresentInCJActiveStatus: false,
11951224
},
1225+
"set lastsuccessfultime if successfulJobHistoryLimit is zero": {
1226+
successfulJobsHistoryLimit: pointer.Int32(0),
1227+
ranPreviously: true,
1228+
schedule: onTheHour,
1229+
expectUpdateStatus: true,
1230+
expectCompleted: true,
1231+
jobPresentInCJActiveStatus: true,
1232+
},
1233+
"set lastsuccessfultime if successfulJobHistoryLimit is ten": {
1234+
successfulJobsHistoryLimit: pointer.Int32(10),
1235+
ranPreviously: true,
1236+
schedule: onTheHour,
1237+
expectUpdateStatus: true,
1238+
expectCompleted: true,
1239+
jobPresentInCJActiveStatus: true,
1240+
},
1241+
"set lastsuccessfultime if successfulJobHistoryLimit is nil": {
1242+
ranPreviously: true,
1243+
schedule: onTheHour,
1244+
expectUpdateStatus: true,
1245+
expectCompleted: true,
1246+
jobPresentInCJActiveStatus: true,
1247+
},
11961248
}
11971249
for name, tc := range testCases {
11981250
name := name
@@ -1204,6 +1256,7 @@ func TestControllerV2SyncCronJob(t *testing.T) {
12041256
cj.Spec.Suspend = &tc.suspend
12051257
cj.Spec.Schedule = tc.schedule
12061258
cj.Spec.TimeZone = tc.timeZone
1259+
cj.Spec.SuccessfulJobsHistoryLimit = tc.successfulJobsHistoryLimit
12071260
if tc.deadline != noDead {
12081261
cj.Spec.StartingDeadlineSeconds = &tc.deadline
12091262
}
@@ -1229,14 +1282,16 @@ func TestControllerV2SyncCronJob(t *testing.T) {
12291282
}
12301283
job.UID = "1234"
12311284
job.Namespace = cj.Namespace
1285+
1286+
ref, err := getRef(job)
1287+
if err != nil {
1288+
t.Fatalf("%s: unexpected error getting the job object reference: %v", name, err)
1289+
}
1290+
if tc.jobPresentInCJActiveStatus {
1291+
cj.Status.Active = []v1.ObjectReference{*ref}
1292+
}
1293+
12321294
if tc.stillActive {
1233-
ref, err := getRef(job)
1234-
if err != nil {
1235-
t.Fatalf("%s: unexpected error getting the job object reference: %v", name, err)
1236-
}
1237-
if tc.jobPresentInCJActiveStatus {
1238-
cj.Status.Active = []v1.ObjectReference{*ref}
1239-
}
12401295
realCJ.Status.Active = []v1.ObjectReference{*ref}
12411296
if !tc.jobStillNotFoundInLister {
12421297
js = append(js, job)
@@ -1341,6 +1396,9 @@ func TestControllerV2SyncCronJob(t *testing.T) {
13411396
if tc.expectDelete {
13421397
expectedEvents++
13431398
}
1399+
if tc.expectCompleted {
1400+
expectedEvents++
1401+
}
13441402
if name == "still active, is time, F" {
13451403
// this is the only test case where we would raise an event for not scheduling
13461404
expectedEvents++

0 commit comments

Comments
 (0)