fix(controller): measure durationThreshold from readiness, not the last exit

README.md

@@ -66,7 +66,7 @@ Short name: `clp` (`kubectl get clp`).
 |---|---|---|
 | `watchReasons` | `[CrashLoopBackOff, ImagePullBackOff, ErrImagePull, CreateContainerConfigError, InvalidImageName, RunContainerError]` | Container waiting reasons to watch |
 | `restartThreshold` | `10` | Number of container restarts before action |
-| `durationThreshold` | `30m` | How long a pod must be failing before action. Go duration format, rejected by the API server if malformed |
+| `durationThreshold` | `30m` | How long a pod must have been continuously not ready before action. Go duration format, rejected by the API server if malformed |
 | `allReplicasFailing` | `true` | Require all replicas to be failing |
 | `targets` | `[Deployment, StatefulSet, CronJob]` | Workload types to act on. Only these three values are accepted |
 | `namespaceSelector` | `nil` | Label selector for namespaces to watch (nil = all) |
@@ -247,7 +247,11 @@ work through the conditions the operator applies, in the order it applies them:
 - **Neither threshold is exceeded.** A workload is only acted on once
   `restartThreshold` restarts are reached **or** the pod has been failing for
   `durationThreshold`. A pod that never starts has no restarts, so it is the
-  duration that applies.
+  duration that applies. The duration is measured from the moment the pod
+  stopped being ready, not from the last container exit, so a pod restarting
+  in a loop accumulates it correctly rather than having the clock reset on
+  every restart. A container that recovers and later fails again starts the
+  clock over.
 - **`allReplicasFailing` is true and some replica is healthy.** This defaults to
   true, so a Deployment with one broken and one running pod is left alone by
   design. Set it to `false` if you want partial failure to count.

internal/controller/crashlooppolicy_controller_test.go

@@ -578,6 +578,16 @@ func TestPodExceedsDurationThreshold_ImagePullBackOff(t *testing.T) {
 		},
 		Status: corev1.PodStatus{
 			Phase: corev1.PodPending,
+			// Kubelet publishes this as soon as it starts syncing the pod, so
+			// a pod that has been stuck for two hours carries a two-hour-old
+			// transition.
+			Conditions: []corev1.PodCondition{
+				{
+					Type:               corev1.ContainersReady,
+					Status:             corev1.ConditionFalse,
+					LastTransitionTime: metav1.NewTime(metav1.Now().Add(-2 * time.Hour)),
+				},
+			},
 			ContainerStatuses: []corev1.ContainerStatus{
 				{
 					Name:         "app",
@@ -596,9 +606,12 @@ func TestPodExceedsDurationThreshold_ImagePullBackOff(t *testing.T) {
 	}
 }
 
-func TestPodExceedsDurationThreshold_CrashLoopWithTermination(t *testing.T) {
-	// A pod in CrashLoopBackOff with LastTerminationState should use
-	// the termination time as the failure start.
+func TestPodExceedsDurationThreshold_SteadyCrashLoop(t *testing.T) {
+	// The state a live kubelet actually produces for a steady crash loop.
+	// Backoff is capped at a few minutes and every restart rewrites
+	// FinishedAt, so the last termination is always recent no matter how long
+	// the pod has been broken. Measuring from it can never reach a threshold
+	// above the cap, which is why readiness is the clock instead.
 	pod := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:              "crashloop",
@@ -607,6 +620,13 @@ func TestPodExceedsDurationThreshold_CrashLoopWithTermination(t *testing.T) {
 		},
 		Status: corev1.PodStatus{
 			Phase: corev1.PodRunning,
+			Conditions: []corev1.PodCondition{
+				{
+					Type:               corev1.ContainersReady,
+					Status:             corev1.ConditionFalse,
+					LastTransitionTime: metav1.NewTime(metav1.Now().Add(-1 * time.Hour)),
+				},
+			},
 			ContainerStatuses: []corev1.ContainerStatus{
 				{
 					Name:         "app",
@@ -618,7 +638,7 @@ func TestPodExceedsDurationThreshold_CrashLoopWithTermination(t *testing.T) {
 					},
 					LastTerminationState: corev1.ContainerState{
 						Terminated: &corev1.ContainerStateTerminated{
-							FinishedAt: metav1.NewTime(metav1.Now().Add(-1 * time.Hour)),
+							FinishedAt: metav1.NewTime(metav1.Now().Add(-90 * time.Second)),
 							ExitCode:   1,
 						},
 					},
@@ -627,7 +647,70 @@ func TestPodExceedsDurationThreshold_CrashLoopWithTermination(t *testing.T) {
 		},
 	}
 	if !podExceedsDurationThreshold(pod, 30*time.Minute) {
-		t.Error("expected crashlooping pod with 1h-old termination to exceed 30m duration threshold")
+		t.Error("expected a pod crash looping for an hour to exceed the 30m duration threshold")
+	}
+}
+
+func TestPodExceedsDurationThreshold_RecoveredPodResetsTheClock(t *testing.T) {
+	// A container that became ready again and only just failed must not count
+	// as having been broken for the whole time since its first ever failure.
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "recovered",
+			Namespace:         testNamespace,
+			CreationTimestamp: metav1.NewTime(metav1.Now().Add(-8 * time.Hour)),
+		},
+		Status: corev1.PodStatus{
+			Phase: corev1.PodRunning,
+			Conditions: []corev1.PodCondition{
+				{
+					Type:               corev1.ContainersReady,
+					Status:             corev1.ConditionFalse,
+					LastTransitionTime: metav1.NewTime(metav1.Now().Add(-2 * time.Minute)),
+				},
+			},
+			ContainerStatuses: []corev1.ContainerStatus{
+				{
+					Name:         "app",
+					RestartCount: 20,
+					State: corev1.ContainerState{
+						Waiting: &corev1.ContainerStateWaiting{Reason: "CrashLoopBackOff"},
+					},
+				},
+			},
+		},
+	}
+	if podExceedsDurationThreshold(pod, 30*time.Minute) {
+		t.Error("expected a pod that was healthy two minutes ago not to exceed the threshold")
+	}
+}
+
+func TestPodExceedsDurationThreshold_NoReadinessConditionYet(t *testing.T) {
+	// Very early in a pod's life kubelet has not published readiness. Deciding
+	// from the creation timestamp would fire immediately on an old pod that
+	// only just broke, so the answer is "not yet" and the next evaluation
+	// decides.
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "no-conditions",
+			Namespace:         testNamespace,
+			CreationTimestamp: metav1.NewTime(metav1.Now().Add(-5 * time.Hour)),
+		},
+		Status: corev1.PodStatus{
+			Phase: corev1.PodPending,
+			ContainerStatuses: []corev1.ContainerStatus{
+				{
+					Name:         "app",
+					RestartCount: 0,
+					State: corev1.ContainerState{
+						Waiting: &corev1.ContainerStateWaiting{Reason: "ImagePullBackOff"},
+					},
+				},
+			},
+		},
+	}
+	if podExceedsDurationThreshold(pod, 30*time.Minute) {
+		t.Error("expected no duration verdict without a readiness condition")
 	}
 }
 

internal/controller/helpers.go

@@ -93,47 +93,50 @@ func podExceedsRestartThreshold(pod *corev1.Pod, threshold int32) bool {
 	return false
 }
 
-// podExceedsDurationThreshold checks if the pod has been in a failing state
-// longer than the given duration. It uses the container's last state
-// transition to determine how long the failure has persisted, avoiding
-// false positives on slow-starting pods that are merely not-ready yet.
-func podExceedsDurationThreshold(pod *corev1.Pod, duration time.Duration) bool {
-	// Check container statuses for waiting state start time.
-	// A container in a waiting state with a LastTerminationState indicates
-	// it has been restarting; use the last termination time as the failure start.
-	for _, cs := range pod.Status.ContainerStatuses {
-		if cs.State.Waiting != nil && cs.LastTerminationState.Terminated != nil {
-			failingSince := cs.LastTerminationState.Terminated.FinishedAt.Time
-			if !failingSince.IsZero() && time.Since(failingSince) >= duration {
-				return true
-			}
-		}
-	}
-	for _, cs := range pod.Status.InitContainerStatuses {
-		if cs.State.Waiting != nil && cs.LastTerminationState.Terminated != nil {
-			failingSince := cs.LastTerminationState.Terminated.FinishedAt.Time
-			if !failingSince.IsZero() && time.Since(failingSince) >= duration {
-				return true
+// podFailingSince returns the instant from which the pod has been
+// continuously not ready, and whether that instant could be determined.
+//
+// The obvious clock, the container's last termination time, does not work.
+// Kubelet restarts a crashing container with exponential backoff capped at a
+// few minutes and writes a fresh FinishedAt on every restart, so for a pod in
+// a steady crash loop that timestamp is always recent and a threshold above
+// the backoff cap can never be reached.
+//
+// The readiness condition does not bounce that way: it flips to False when the
+// container first stops serving and stays there for as long as it keeps
+// failing. If the container does recover for a while and then fails again, the
+// condition flips too, which is the intended meaning: the pod was healthy in
+// between, so the clock should restart.
+//
+// ContainersReady is preferred over Ready because Ready can also be held false
+// by readiness gates, which say nothing about the containers.
+func podFailingSince(pod *corev1.Pod) (time.Time, bool) {
+	for _, condType := range []corev1.PodConditionType{corev1.ContainersReady, corev1.PodReady} {
+		for _, cond := range pod.Status.Conditions {
+			if cond.Type != condType || cond.Status != corev1.ConditionFalse {
+				continue
 			}
-		}
-	}
-	// For containers that have never run (e.g. ImagePullBackOff on first deploy),
-	// fall back to pod creation time as the failure start.
-	for _, cs := range pod.Status.ContainerStatuses {
-		if cs.State.Waiting != nil && cs.RestartCount == 0 {
-			if !pod.CreationTimestamp.IsZero() && time.Since(pod.CreationTimestamp.Time) >= duration {
-				return true
+			if !cond.LastTransitionTime.IsZero() {
+				return cond.LastTransitionTime.Time, true
 			}
 		}
 	}
-	for _, cs := range pod.Status.InitContainerStatuses {
-		if cs.State.Waiting != nil && cs.RestartCount == 0 {
-			if !pod.CreationTimestamp.IsZero() && time.Since(pod.CreationTimestamp.Time) >= duration {
-				return true
-			}
-		}
+	// No usable readiness condition. This happens in the first moments of a
+	// pod's life before kubelet has reported, so returning "unknown" simply
+	// defers the decision to the next evaluation rather than guessing from the
+	// creation timestamp, which would fire immediately on an old pod that only
+	// just broke.
+	return time.Time{}, false
+}
+
+// podExceedsDurationThreshold reports whether the pod has been failing for at
+// least the given duration.
+func podExceedsDurationThreshold(pod *corev1.Pod, duration time.Duration) bool {
+	failingSince, ok := podFailingSince(pod)
+	if !ok {
+		return false
 	}
-	return false
+	return time.Since(failingSince) >= duration
 }
 
 // ownerWorkload represents a resolved top-level workload that owns a pod.