Make explicit check in CronJob if Job is successful before setting LastSuccessfulTime (#123380)

* Make explicit check in CronJob if Job is successful

before setting LastSuccessfulTime

* Review remarks for the CronJob

Co-authored-by: Filip Křepinský <fkrepins@redhat.com>

---------

Co-authored-by: Filip Křepinský <fkrepins@redhat.com>

pkg/controller/cronjob/cronjob_controllerv2.go

@@ -443,8 +443,8 @@ func (jm *ControllerV2) syncCronJob(
 			deleteFromActiveList(cronJob, j.ObjectMeta.UID)
 			jm.recorder.Eventf(cronJob, corev1.EventTypeNormal, "SawCompletedJob", "Saw completed job: %s, status: %v", j.Name, status)
 			updateStatus = true
-		} else if IsJobFinished(j) {
+		} else if IsJobSucceeded(j) {
-			// a job does not have to be in active list, as long as it is finished, we will process the timestamp
+			// 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

pkg/controller/cronjob/utils.go

@@ -292,6 +292,16 @@ func IsJobFinished(j *batchv1.Job) bool {
 	return isFinished
 }
 
+// IsJobSucceeded returns whether a job has completed successfully.
+func IsJobSucceeded(j *batchv1.Job) bool {
+	for _, c := range j.Status.Conditions {
+		if c.Type == batchv1.JobComplete && c.Status == corev1.ConditionTrue {
+			return true
+		}
+	}
+	return false
+}
+
 // byJobStartTime sorts a list of jobs by start timestamp, using their names as a tie breaker.
 type byJobStartTime []*batchv1.Job
 

pkg/controller/cronjob/utils_test.go

@@ -25,7 +25,7 @@ import (
 
 	cron "github.com/robfig/cron/v3"
 	batchv1 "k8s.io/api/batch/v1"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
@@ -706,6 +706,59 @@ func TestNextScheduleTimeDuration(t *testing.T) {
 	}
 }
 
+func TestIsJobSucceeded(t *testing.T) {
+	tests := map[string]struct {
+		job        batchv1.Job
+		wantResult bool
+	}{
+		"job doesn't have any conditions": {
+			wantResult: false,
+		},
+		"job has Complete=True condition": {
+			job: batchv1.Job{
+				Status: batchv1.JobStatus{
+					Conditions: []batchv1.JobCondition{
+						{
+							Type:   batchv1.JobSuspended,
+							Status: v1.ConditionFalse,
+						},
+						{
+							Type:   batchv1.JobComplete,
+							Status: v1.ConditionTrue,
+						},
+					},
+				},
+			},
+			wantResult: true,
+		},
+		"job has Complete=False condition": {
+			job: batchv1.Job{
+				Status: batchv1.JobStatus{
+					Conditions: []batchv1.JobCondition{
+						{
+							Type:   batchv1.JobFailed,
+							Status: v1.ConditionTrue,
+						},
+						{
+							Type:   batchv1.JobComplete,
+							Status: v1.ConditionFalse,
+						},
+					},
+				},
+			},
+			wantResult: false,
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			gotResult := IsJobSucceeded(&tc.job)
+			if tc.wantResult != gotResult {
+				t.Errorf("unexpected result, want=%v, got=%v", tc.wantResult, gotResult)
+			}
+		})
+	}
+}
+
 func topOfTheHour() *time.Time {
 	T1, err := time.Parse(time.RFC3339, "2016-05-19T10:00:00Z")
 	if err != nil {