From 0f261b4e8f7d442fce0e089a27266b51ee1303bb Mon Sep 17 00:00:00 2001 From: Sarthak Date: Thu, 1 Oct 2026 22:56:29 +0530 Subject: [PATCH 1/2] fix(controller): skip reaping succeeded Job pods (#127) Signed-off-by: Sarthak --- .../controller/pod_placement_controller_test.go | 14 ++++++++++++++ internal/controller/pod_placement_helpers.go | 6 ++++++ 2 files changed, 20 insertions(+) diff --git a/internal/controller/pod_placement_controller_test.go b/internal/controller/pod_placement_controller_test.go index 98b5696..24358cb 100644 --- a/internal/controller/pod_placement_controller_test.go +++ b/internal/controller/pod_placement_controller_test.go @@ -960,3 +960,17 @@ func TestReap_NonNebulaTerminalPodIsIgnored(t *testing.T) { t.Fatal("must not reap a non-Nebula Pod") } } + +func TestReap_SucceededJobPodIsKept(t *testing.T) { + // Deleting a Succeeded Job pod makes the Job lose its success record and read as Failed. + pod := terminalOwnedPod("p1", "default", "uid-1", corev1.PodSucceeded, true) + pod.OwnerReferences[0].APIVersion = "batch/v1" + pod.OwnerReferences[0].Kind = "Job" + r, c := newPlacementReconciler(t, []client.Object{pod}) + + reconcilePod(t, r, "default", "p1") + + if !podPresent(c, "default", "p1") { + t.Fatal("expected a Succeeded Job Pod to be kept") + } +} diff --git a/internal/controller/pod_placement_helpers.go b/internal/controller/pod_placement_helpers.go index 7bc5b43..1331d9f 100644 --- a/internal/controller/pod_placement_helpers.go +++ b/internal/controller/pod_placement_helpers.go @@ -431,6 +431,12 @@ func (r *PodPlacementReconciler) reapTerminalPod(ctx context.Context, pod *corev if !util.IsTerminalPodPhase(pod.Status.Phase) { return false, nil } + // Do not delete succeeded Pods owned by Jobs; keep for status reporting. + if pod.Status.Phase == corev1.PodSucceeded { + if owner := metav1.GetControllerOf(pod); owner != nil && owner.Kind == "Job" { + return false, nil + } + } if !isControllerOwned(pod) { return false, nil // bare Pod: leave it as a record } From 82e1a9289425d753fb8f883b257f9c3ca88cf2a6 Mon Sep 17 00:00:00 2001 From: Sarthak Date: Mon, 19 Oct 2026 02:50:25 +0530 Subject: [PATCH 2/2] fix(controller): move the succeeded Job Pod check into isSucceededPodOwnedByJob Check it after isControllerOwned, and keep the Pod only when its controller is a Job in the batch API group. Signed-off-by: Sarthak --- .../pod_placement_controller_test.go | 27 +++++++++++++++++++ internal/controller/pod_placement_helpers.go | 26 +++++++++++++----- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/internal/controller/pod_placement_controller_test.go b/internal/controller/pod_placement_controller_test.go index 24358cb..0a0eb55 100644 --- a/internal/controller/pod_placement_controller_test.go +++ b/internal/controller/pod_placement_controller_test.go @@ -974,3 +974,30 @@ func TestReap_SucceededJobPodIsKept(t *testing.T) { t.Fatal("expected a Succeeded Job Pod to be kept") } } + +func TestReap_FailedJobPodIsDeleted(t *testing.T) { + pod := terminalOwnedPod("p1", "default", "uid-1", corev1.PodFailed, true) + pod.OwnerReferences[0].APIVersion = "batch/v1" + pod.OwnerReferences[0].Kind = "Job" + r, c := newPlacementReconciler(t, []client.Object{pod}) + + reconcilePod(t, r, "default", "p1") + + if podPresent(c, "default", "p1") { + t.Fatal("expected a Failed Job Pod to be deleted") + } +} + +func TestReap_SucceededPodOfNonBatchJobIsDeleted(t *testing.T) { + // Only a batch Job is kept; a "Job" kind from another API group is not. + pod := terminalOwnedPod("p1", "default", "uid-1", corev1.PodSucceeded, true) + pod.OwnerReferences[0].APIVersion = "example.com/v1" + pod.OwnerReferences[0].Kind = "Job" + r, c := newPlacementReconciler(t, []client.Object{pod}) + + reconcilePod(t, r, "default", "p1") + + if podPresent(c, "default", "p1") { + t.Fatal("expected a Succeeded Pod of a non-batch Job to be deleted") + } +} diff --git a/internal/controller/pod_placement_helpers.go b/internal/controller/pod_placement_helpers.go index 1331d9f..7c0eca2 100644 --- a/internal/controller/pod_placement_helpers.go +++ b/internal/controller/pod_placement_helpers.go @@ -23,9 +23,11 @@ import ( "strings" "time" + batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -418,6 +420,7 @@ func removeGate(gates []corev1.PodSchedulingGate, name string) []corev1.PodSched // - terminal phase: only Failed/Succeeded (a live Pod is never touched). // - controller-owned: only Pods a ReplicaSet/Job will recreate; a bare Pod is // left as an inspectable record. +// - not a succeeded Job Pod: the Job counts it as a completion. // // Delete is UID-pinned so a Pod already replaced by a same-name recreate is not // clobbered, and a NotFound (already gone) is treated as success. @@ -431,15 +434,12 @@ func (r *PodPlacementReconciler) reapTerminalPod(ctx context.Context, pod *corev if !util.IsTerminalPodPhase(pod.Status.Phase) { return false, nil } - // Do not delete succeeded Pods owned by Jobs; keep for status reporting. - if pod.Status.Phase == corev1.PodSucceeded { - if owner := metav1.GetControllerOf(pod); owner != nil && owner.Kind == "Job" { - return false, nil - } - } if !isControllerOwned(pod) { return false, nil // bare Pod: leave it as a record } + if isSucceededPodOwnedByJob(pod) { + return false, nil // the Job counts it as a completion; deleting it loses that + } preconditions := metav1.Preconditions{UID: &pod.UID} if err := r.Delete(ctx, pod, &client.DeleteOptions{Preconditions: &preconditions}); err != nil { @@ -454,6 +454,20 @@ func isControllerOwned(pod *corev1.Pod) bool { return metav1.GetControllerOf(pod) != nil } +// isSucceededPodOwnedByJob reports whether the Pod has succeeded and is +// controlled by a batch Job. +func isSucceededPodOwnedByJob(pod *corev1.Pod) bool { + if pod.Status.Phase != corev1.PodSucceeded { + return false + } + owner := metav1.GetControllerOf(pod) + if owner == nil || owner.Kind != "Job" { + return false + } + gv, err := schema.ParseGroupVersion(owner.APIVersion) + return err == nil && gv.Group == batchv1.GroupName +} + // SetupWithManager wires the controller. It watches Pods; NodePool edits are not // watched because a Pod already gated will re-reconcile on the periodic resync, // and a newly-created Pod always triggers its own event.