From 5367025a404b44ecc1e43810ae5d74f7b6c074ba Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Sun, 6 Sep 2026 10:30:02 +0200 Subject: [PATCH] fix(controller): measure durationThreshold from readiness, not the last exit podExceedsDurationThreshold measured from the container's last termination time. Kubelet restarts a crashing container with backoff capped at a few minutes and rewrites FinishedAt on every restart, so for a steady crash loop that timestamp is always recent and a threshold above the cap could never be reached. durationThreshold was therefore inert for CrashLoopBackOff, the operator's headline failure reason: crash loops were gated on restartThreshold alone. The field only ever worked for containers that never started, which reach the RestartCount == 0 branch and fall back to the pod creation time. The README promised something the code did not do. Measures from the readiness condition instead, which flips to False when the container first stops serving and stays there while it keeps failing. A container that recovers and fails again restarts the clock, which is the intended meaning. ContainersReady is preferred over Ready because Ready can also be held false by readiness gates, which say nothing about the containers. A pod with no readiness condition yet returns no verdict rather than falling back to the creation timestamp, which would fire immediately on an old pod that only just broke. The test that covered this asserted a state kubelet cannot produce: 15 restarts with a termination an hour old. Replaced with the state a live kubelet does produce, plus cases for a recovered pod and a missing condition. Verified against the previous implementation that the new crash loop test fails on it. Closes #64 Signed-off-by: Simon Lauger --- README.md | 8 +- .../crashlooppolicy_controller_test.go | 93 ++++++++++++++++++- internal/controller/helpers.go | 75 ++++++++------- 3 files changed, 133 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index 5662bd2..92a3cfc 100644 --- a/README.md +++ b/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. diff --git a/internal/controller/crashlooppolicy_controller_test.go b/internal/controller/crashlooppolicy_controller_test.go index 869cf71..1edbfa4 100644 --- a/internal/controller/crashlooppolicy_controller_test.go +++ b/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") } } diff --git a/internal/controller/helpers.go b/internal/controller/helpers.go index a3cc14f..7a98b6a 100644 --- a/internal/controller/helpers.go +++ b/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.