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.