From 3852d1c0c15c90bd747ac7fd25e9c818b1f1c429 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20Wo=C5=BAniak?=
 <mimowo@users.noreply.github.com>
Date: Tue, 27 Feb 2024 22:09:43 +0100
Subject: [PATCH] Make explicit check in CronJob if Job is successful before
 setting LastSuccessfulTime (#123380)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* 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>
---
 .../cronjob/cronjob_controllerv2.go           |  4 +-
 pkg/controller/cronjob/utils.go               | 10 ++++
 pkg/controller/cronjob/utils_test.go          | 55 ++++++++++++++++++-
 3 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go
index bb2da6bf080..e1112ae2b6e 100644
--- a/pkg/controller/cronjob/cronjob_controllerv2.go
+++ b/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) {
-			// a job does not have to be in active list, as long as it is finished, we will process the timestamp
+		} else if 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
diff --git a/pkg/controller/cronjob/utils.go b/pkg/controller/cronjob/utils.go
index b88504d1182..681a27d18e3 100644
--- a/pkg/controller/cronjob/utils.go
+++ b/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
 
diff --git a/pkg/controller/cronjob/utils_test.go b/pkg/controller/cronjob/utils_test.go
index d190b310ae0..1791b585298 100644
--- a/pkg/controller/cronjob/utils_test.go
+++ b/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 {