Fix removing finalizer from finished jobs

In some rare race conditions, the job controller might create new pods after the job is declared finished.

Change-Id: I8a00429c8845463259cd7f82bb3c241d0011583c

pkg/controller/job/job_controller.go

@@ -384,7 +384,8 @@ func (jm *Controller) deletePod(obj interface{}, final bool) {
 		return
 	}
 	job := jm.resolveControllerRef(pod.Namespace, controllerRef)
-	if job == nil {
+	if job == nil || IsJobFinished(job) {
+		// syncJob will not remove this finalizer.
 		if hasFinalizer {
 			jm.enqueueOrphanPod(pod)
 		}
@@ -585,7 +586,7 @@ func (jm Controller) syncOrphanPod(ctx context.Context, key string) error {
 	// Make sure the pod is still orphaned.
 	if controllerRef := metav1.GetControllerOf(sharedPod); controllerRef != nil {
 		job := jm.resolveControllerRef(sharedPod.Namespace, controllerRef)
-		if job != nil {
+		if job != nil && !IsJobFinished(job) {
 			// The pod was adopted. Do not remove finalizer.
 			return nil
 		}

pkg/controller/job/job_controller_test.go

@@ -2596,32 +2596,63 @@ func TestWatchOrphanPods(t *testing.T) {
 		jobSynced = true
 		return true, nil
 	}
-
-	// Create job but don't add it to the store.
-	testJob := newJob(2, 2, 6, batch.NonIndexedCompletion)
-
 	stopCh := make(chan struct{})
 	defer close(stopCh)
 	go sharedInformers.Core().V1().Pods().Informer().Run(stopCh)
 	go manager.Run(context.TODO(), 1)
 
-	orphanPod := buildPod().name("a").job(testJob).deletionTimestamp().trackingFinalizer().Pod
+	// Create job but don't add it to the store.
-	orphanPod, err := clientset.CoreV1().Pods("default").Create(context.Background(), orphanPod, metav1.CreateOptions{})
+	cases := map[string]struct {
-	if err != nil {
+		job           *batch.Job
-		t.Fatalf("Creating orphan pod: %v", err)
+		inCache       bool
+		wantJobSynced bool
+	}{
+		"job_does_not_exist": {
+			job: newJob(2, 2, 6, batch.NonIndexedCompletion),
+		},
+		"orphan": {},
+		"job_finished": {
+			job: func() *batch.Job {
+				j := newJob(2, 2, 6, batch.NonIndexedCompletion)
+				j.Status.Conditions = append(j.Status.Conditions, batch.JobCondition{
+					Type:   batch.JobComplete,
+					Status: v1.ConditionTrue,
+				})
+				return j
+			}(),
+			inCache: true,
+		},
 	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			jobSynced = false
+			if tc.inCache {
+				sharedInformers.Batch().V1().Jobs().Informer().GetIndexer().Add(tc.job)
+			}
 
-	if err := wait.Poll(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {
+			podBuilder := buildPod().name(name).deletionTimestamp().trackingFinalizer()
-		p, err := clientset.CoreV1().Pods(orphanPod.Namespace).Get(context.Background(), orphanPod.Name, metav1.GetOptions{})
+			if tc.job != nil {
-		if err != nil {
+				podBuilder = podBuilder.job(tc.job)
-			return false, err
+			}
-		}
+			orphanPod := podBuilder.Pod
-		return !hasJobTrackingFinalizer(p), nil
+			orphanPod, err := clientset.CoreV1().Pods("default").Create(context.Background(), orphanPod, metav1.CreateOptions{})
-	}); err != nil {
+			if err != nil {
-		t.Errorf("Waiting for Pod to get the finalizer removed: %v", err)
+				t.Fatalf("Creating orphan pod: %v", err)
-	}
+			}
-	if jobSynced {
+
-		t.Error("Tried to sync deleted job")
+			if err := wait.Poll(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {
+				p, err := clientset.CoreV1().Pods(orphanPod.Namespace).Get(context.Background(), orphanPod.Name, metav1.GetOptions{})
+				if err != nil {
+					return false, err
+				}
+				return !hasJobTrackingFinalizer(p), nil
+			}); err != nil {
+				t.Errorf("Waiting for Pod to get the finalizer removed: %v", err)
+			}
+			if !tc.inCache && jobSynced {
+				t.Error("Tried to sync deleted job")
+			}
+		})
 	}
 }