fix job pod could not be removed while removing finalizer failed

Project: kubernetes/kubernetes | Language: Go | Concepts: retry-backoff, error-handling, testing-strategy, refactoring
Difficulty: mid
Link: https://github.com/kubernetes/kubernetes/pull/141329

The problem

When a Kubernetes Job is deleted, its pods may have a "tracking finalizer" that must be removed before the pod can be deleted. If removing this finalizer fails (e.g., due to a transient error), the pod can get stuck with the finalizer and never be removed, because the controller does not retry the operation.

Why it's non-trivial

A naive fix might simply re-attempt the operation immediately or ignore the error, but this could lead to tight retry loops or missed retries if the controller does not properly re-enqueue the failed pod. The controller must use a rate-limited queue to ensure retries happen with backoff, and must do so for both single pods and pods selected via label selectors.

The approach

1. Refactor enqueueOrphanPod to support rate-limited retries

Previously, enqueueOrphanPod did not support rate-limited re-enqueueing. The function signature is changed to accept a backoff parameter:

-func (jm *Controller) enqueueOrphanPod(obj *v1.Pod) {
+func (jm *Controller) enqueueOrphanPod(obj *v1.Pod, backoff bool) {
     orphanPodKey := orphanPodKey{
         kind:      OrphanPodKeyKindName,
         namespace: obj.Namespace,
         value:     obj.Name,
     }
-    jm.orphanQueue.Add(orphanPodKey)
+    if backoff {
+        jm.orphanQueue.AddRateLimited(orphanPodKey)
+    } else {
+        jm.orphanQueue.Add(orphanPodKey)
+    }
 }

This change allows the controller to decide whether to requeue the pod immediately or with backoff, depending on the context.

2. Update all call-sites to use the new parameter

All places that previously called enqueueOrphanPod(pod) are updated to pass false for the backoff parameter, preserving existing behavior for normal enqueues:

-    jm.enqueueOrphanPod(pod)
+    jm.enqueueOrphanPod(pod, false)

This ensures that only error cases use rate-limited retries.

3. Retry failed orphan pod processing with backoff

In syncOrphanPodsBySelector, if handling a pod fails, it is now re-enqueued with backoff:

 for _, pod := range pods {
     if err := jm.handleSingleOrphanPod(ctx, pod); err != nil {
         logger.Error(err, "syncing orphan pod failed", "pod", klog.KObj(pod))
+        jm.enqueueOrphanPod(pod, true)
     }
 }

This ensures that pods which failed to have their finalizer removed will be retried with exponential backoff.

4. Add a unit test to verify retry behavior

A new test, TestSyncOrphanPodsBySelectorRetryOnFailure, is added to ensure that failed orphan pod processing results in rate-limited retries:

+func TestSyncOrphanPodsBySelectorRetryOnFailure(t *testing.T) {
+    const podsCount = 3
+    ...
+    fakePodControl.Err = errors.New("server temporarily unavailable")
+
+    err = manager.syncOrphanPod(ctx, orphanKey)
+    if err != nil {
+        t.Fatalf("syncOrphanPod should not return error even if some pods fail: %v", err)
+    }
+    ...
+    // Wait for the rate limiter backoff delay to expire before retrying.
+    time.Sleep(2 * time.Second)
+
+    for range podsCount {
+        key, quit := manager.orphanQueue.Get()
+        ...
+        err := manager.syncOrphanPod(ctx, key)
+        if err != nil {
+            t.Fatalf("syncOrphanPod failed on retry: %v", err)
+        }
+    }
+    ...
+}

This test simulates a transient error, verifies that each pod is retried, and checks that the correct patch is applied to remove the finalizer.

From the review thread

The fix is sound, but the manual creation of the orphanPodKey is worrisome. How about refactoring enqueueOrphanPod, which constructs that key for us, such that it will invoke jm.orphanQueue.Add or jm.orphanQueue.AddRateLimited conditionally.

@soltysh do you mean that we add a param for enqueueOrphanPod, then we also call enqueueOrphanPod here but pass a param to call jm.orphanQueue.AddRateLimited?

Takeaways

• Use rate-limited queues for retrying transient failures to avoid tight retry loops and to ensure eventual progress.
• Refactor helper methods to encapsulate key construction and queueing logic, reducing duplication and error-prone manual key management.
• Always add or update tests when changing error-handling or retry logic, to verify correct behavior in both success and failure scenarios.