Merge pull request #55 from artem-zinnatullin/az/delete-hanging-jobs

Delete hanging jobs (DeadlineExceeded, Active == 1 but actually completed)

Author: lwolf

.gitignore

@@ -11,3 +11,6 @@ bin/
 
 # Output of the go coverage tool, specifically when used with LiteIDE
 *.out
+
+# Coverage report generated by `make test`.
+coverage.txt

pkg/controller/job.go

@@ -8,11 +8,6 @@ import (
 )
 
 func shouldDeleteJob(job *batchv1.Job, deleteSuccessfulAfter, deleteFailedAfter time.Duration, ignoreCronJobs bool) bool {
-	// skip the job if it has any active pods
-	if job.Status.Active > 0 {
-		return false
-	}
-
 	if ignoreCronJobs {
 		owners := getJobOwnerKinds(job)
 		if isOwnedByCronJob(owners) {
@@ -33,7 +28,7 @@ func shouldDeleteJob(job *batchv1.Job, deleteSuccessfulAfter, deleteFailedAfter
 			return true
 		}
 	}
-	if job.Status.Failed > 0 {
+	if isFailed(job) {
 		if deleteFailedAfter > 0 && timeSinceFinish >= deleteFailedAfter {
 			return true
 		}
@@ -65,6 +60,22 @@ func jobFinishTime(jobObj *batchv1.Job) time.Time {
 	return time.Time{}
 }
 
+func isFailed(jobObj *batchv1.Job) bool {
+    if jobObj.Status.Failed > 0 {
+        return true
+    }
+
+    // In case when Job fails due to the Deadline set on it (DeadlineExceeded), the job.Status.Failed does not contain a value
+    // Thus we have to iterate over job.Status.Conditions and look for a JobFailed in state "true".
+    for _, jc := range jobObj.Status.Conditions {
+        if jc.Type == batchv1.JobFailed && jc.Status == corev1.ConditionTrue {
+            return true
+        }
+    }
+
+    return false
+}
+
 // isOwnedByCronJob returns true if and only if job has a single owner CronJob
 // and this owners kind is CronJob
 func isOwnedByCronJob(ownerKinds []string) bool {

pkg/controller/job_test.go

@@ -5,10 +5,11 @@ import (
 	"time"
 
 	batchv1 "k8s.io/api/batch/v1"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func createJob(ownedByCronJob bool, completed time.Time, active, succeeded, failed int32) *batchv1.Job {
+func createJob(ownedByCronJob bool, completed time.Time, active, succeeded, failed int32, conditions []batchv1.JobCondition) *batchv1.Job {
 	ts := metav1.NewTime(completed)
 	job := batchv1.Job{
 		Spec: batchv1.JobSpec{},
@@ -17,6 +18,7 @@ func createJob(ownedByCronJob bool, completed time.Time, active, succeeded, fail
 			Active:         active,
 			Succeeded:      succeeded,
 			Failed:         failed,
+			Conditions:     conditions,
 		},
 	}
 	if ownedByCronJob {
@@ -39,54 +41,100 @@ func TestKleaner_DeleteJob(t *testing.T) {
 		expected   bool
 	}{
 		"jobs owned by cronjobs should be ignored": {
-			jobSpec:    createJob(true, ts.Add(-time.Minute), 0, 0, 0),
+			jobSpec:    createJob(true, ts.Add(-time.Minute), 0, 0, 0, []batchv1.JobCondition{}),
 			successful: time.Second,
 			failed:     time.Second,
 			ignoreCron: true,
 			expected:   false,
 		},
 		"jobs owned by cronjobs should be deleted": {
-			jobSpec:    createJob(true, ts.Add(-time.Minute), 0, 0, 0),
+			jobSpec:    createJob(true, ts.Add(-time.Minute), 0, 0, 0, []batchv1.JobCondition{}),
 			successful: time.Second,
 			failed:     time.Second,
 			ignoreCron: false,
 			expected:   false,
 		},
 		"jobs with active pods should not be deleted": {
-			jobSpec:    createJob(false, ts.Add(-time.Minute), 1, 0, 0), // job.Status.Active > 0
+			jobSpec:    createJob(false, ts.Add(-time.Minute), 1, 0, 0, []batchv1.JobCondition{}), // job.Status.Active > 0
 			successful: time.Second,
 			failed:     time.Second,
 			ignoreCron: false,
 			expected:   false,
 		},
 		"expired successful jobs should be deleted": {
-			jobSpec:    createJob(false, ts.Add(-time.Minute), 0, 1, 0),
+			jobSpec:    createJob(false, ts.Add(-time.Minute), 0, 1, 0, []batchv1.JobCondition{}),
 			successful: time.Second,
 			failed:     time.Second,
 			ignoreCron: false,
 			expected:   true,
 		},
 		"non-expired successful jobs should not be deleted": {
-			jobSpec:    createJob(false, ts.Add(-time.Minute), 0, 1, 0),
+			jobSpec:    createJob(false, ts.Add(-time.Minute), 0, 1, 0, []batchv1.JobCondition{}),
 			successful: time.Minute * 2,
 			failed:     time.Second,
 			ignoreCron: false,
 			expected:   false,
 		},
 		"expired failed jobs should be deleted": {
-			jobSpec:    createJob(false, ts.Add(-time.Minute), 0, 0, 1),
+			jobSpec:    createJob(false, ts.Add(-time.Minute), 0, 0, 1, []batchv1.JobCondition{}),
 			successful: time.Second,
 			failed:     time.Second,
 			ignoreCron: false,
 			expected:   true,
 		},
 		"non-expired failed jobs should not be deleted": {
-			jobSpec:    createJob(false, ts.Add(-time.Minute), 0, 0, 1),
+			jobSpec:    createJob(false, ts.Add(-time.Minute), 0, 0, 1, []batchv1.JobCondition{}),
 			successful: time.Second,
 			failed:     time.Minute * 2,
 			ignoreCron: false,
 			expected:   false,
 		},
+		"failed (based on JobCondition) but not marked as failed jobs should be deleted": {
+			jobSpec: createJob(false, time.Time{}, 0, 0, 0, []batchv1.JobCondition{
+				batchv1.JobCondition{
+					Type:               batchv1.JobFailed,
+					Status:             corev1.ConditionTrue,
+					LastProbeTime:      metav1.NewTime(ts),
+					LastTransitionTime: metav1.NewTime(ts.Add(-time.Minute)),
+					Reason:             "DeadlineExceeded",
+					Message:            "Job was active longer than specified deadline",
+				},
+			}),
+			successful: time.Second,
+			failed:     time.Second,
+			ignoreCron: false,
+			expected:   true,
+		},
+		"successful but 'active' jobs should be deleted": {
+			jobSpec:    createJob(false, ts.Add(-time.Minute), 1, 1, 0, []batchv1.JobCondition{}),
+			successful: time.Second,
+			failed:     time.Second,
+			ignoreCron: false,
+			expected:   true,
+		},
+		"failed but 'active' jobs should be deleted": {
+			jobSpec:    createJob(false, ts.Add(-time.Minute), 1, 0, 1, []batchv1.JobCondition{}),
+			successful: time.Second,
+			failed:     time.Second,
+			ignoreCron: false,
+			expected:   true,
+		},
+		"failed (based on JobCondition) but 'active' jobs should be deleted": {
+			jobSpec: createJob(false, ts.Add(-time.Minute), 1, 0, 0, []batchv1.JobCondition{
+				batchv1.JobCondition{
+					Type:               batchv1.JobFailed,
+					Status:             corev1.ConditionTrue,
+					LastProbeTime:      metav1.NewTime(ts),
+					LastTransitionTime: metav1.NewTime(ts.Add(-time.Minute)),
+					Reason:             "DeadlineExceeded",
+					Message:            "Job was active longer than specified deadline",
+				},
+			}),
+			successful: time.Second,
+			failed:     time.Second,
+			ignoreCron: false,
+			expected:   true,
+		},
 	}
 	for name, tc := range testCases {
 		t.Run(name, func(t *testing.T) {