From 6d07b92202fe1d8cb1eb721f2e1257f34c87bd1a Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Sun, 6 Sep 2026 11:22:20 +0200 Subject: [PATCH] feat(controller): detect slow restart loops via termination reason watchReasons only sees containers that are waiting. Kubelet resets its restart backoff once a container has stayed up longer than roughly twice the maximum backoff, so a container that survives beyond that between deaths restarts immediately every time and never enters CrashLoopBackOff. A memory leak has exactly that shape, and such a workload can reach hundreds of restarts while remaining invisible. Adds watchTerminationReasons, empty by default so nothing changes for existing policies, matching on why the container last exited rather than on what it is waiting for. A match needs the restart threshold reached, a listed reason, and the exit to fall inside the new restartWindow. That window is not optional: the restart count is cumulative for a pod's whole life and never decays, so without it a workload that misbehaved last month would still be scaled down. A pod in this state is running when observed, so the change is not confined to the failure predicate. It needs a second field index, a widened watch predicate, and a shared predicate between the reconcile loop, allReplicasFailing and policyWouldAct, since the sibling check runs under allReplicasFailing, which defaults to true and would otherwise leave the feature inert. Guards against acting on pods being deleted, which would sweep every replica of a rolling update in at once, on completed pods, on containers still inside their startup probe, and on classic init containers, which run once and cannot loop. Sidecars declared with restartPolicy Always do count. Also documents that termination reasons never appear as waiting reasons, so putting OOMKilled in watchReasons silently matches nothing. Closes #67 Signed-off-by: Simon Lauger --- README.md | 43 +++- api/v1alpha1/crashlooppolicy_types.go | 25 +++ api/v1alpha1/zz_generated.deepcopy.go | 5 + ...-operator.lauger.de_crashlooppolicies.yaml | 28 +++ ...-operator.lauger.de_crashlooppolicies.yaml | 28 +++ internal/controller/constants.go | 3 + .../controller/crashlooppolicy_controller.go | 41 ++-- .../crashlooppolicy_controller_test.go | 196 +++++++++++++++++- internal/controller/helpers.go | 98 ++++++++- internal/controller/indexes.go | 94 +++++++++ internal/controller/policyresolve.go | 39 +++- internal/controller/testutil_test.go | 1 + 12 files changed, 572 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 92a3cfc..aaf558c 100644 --- a/README.md +++ b/README.md @@ -64,7 +64,9 @@ Short name: `clp` (`kubectl get clp`). | Field | Default | Description | |---|---|---| -| `watchReasons` | `[CrashLoopBackOff, ImagePullBackOff, ErrImagePull, CreateContainerConfigError, InvalidImageName, RunContainerError]` | Container waiting reasons to watch | +| `watchReasons` | `[CrashLoopBackOff, ImagePullBackOff, ErrImagePull, CreateContainerConfigError, InvalidImageName, RunContainerError]` | Container **waiting** reasons to watch. Termination reasons such as `OOMKilled` never appear here; use `watchTerminationReasons` for those | +| `watchTerminationReasons` | `[]` | Container **termination** reasons to act on, for containers that restart repeatedly without settling into a watched waiting state. Off by default | +| `restartWindow` | `1h` | How recently the last termination must have happened for `watchTerminationReasons` to match | | `restartThreshold` | `10` | Number of container restarts before action | | `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 | @@ -226,6 +228,41 @@ Two alerts worth having: Note that `crashloop_scaled_down_total` counts dry-run actions too. Filter with `dry_run="false"` when alerting on real ones. +### Slow restart loops + +`watchReasons` only sees containers that are **waiting**. Kubelet resets its +restart backoff once a container has stayed up longer than roughly twice the +maximum backoff, so a container that survives beyond that between deaths +restarts immediately every time and never enters `CrashLoopBackOff`. A memory +leak has exactly this shape: run, grow, get OOM-killed, restart at once, +repeat. Such a workload can reach hundreds of restarts unnoticed. + +`watchTerminationReasons` covers that case by matching on why the container +last exited rather than on what it is waiting for: + +```yaml +spec: + watchTerminationReasons: + - OOMKilled + restartThreshold: 10 + restartWindow: 1h +``` + +The workload is acted on when a container has reached `restartThreshold` +restarts **and** its most recent exit carries a listed reason **and** that exit +happened within `restartWindow`. The window matters: the restart count is +cumulative for the pod's whole life and never decays, so without it a workload +that misbehaved last month would still be scaled down. + +`OOMKilled` is the safe value to start with. `Error` also works but is broad, +covering ordinary crashes, liveness kills and SIGKILL after the grace period +alike. + +Not counted: pods being deleted, pods that have completed, containers still +inside their startup probe, and classic init containers, which run once and +cannot loop. Init containers declared with `restartPolicy: Always` are sidecars +and do count. + ## Troubleshooting ### The policy exists but nothing is scaled down @@ -260,7 +297,9 @@ work through the conditions the operator applies, in the order it applies them: default rather than adding to it. - **`namespaceSelector` or `excludeWorkloadSelector` filters it out.** - **The failure reason is not watched.** `watchReasons` matches the container's - waiting reason exactly. Check it with + waiting reason exactly. Note that termination reasons such as `OOMKilled` + never appear as a waiting reason, so putting one in `watchReasons` matches + nothing; see [Slow restart loops](#slow-restart-loops). Check it with `kubectl get pod -o jsonpath='{.status.containerStatuses[*].state.waiting.reason}'`. ### Ready is False diff --git a/api/v1alpha1/crashlooppolicy_types.go b/api/v1alpha1/crashlooppolicy_types.go index 15cf0f4..576c4b8 100644 --- a/api/v1alpha1/crashlooppolicy_types.go +++ b/api/v1alpha1/crashlooppolicy_types.go @@ -19,6 +19,31 @@ type CrashLoopPolicySpec struct { // +kubebuilder:default={"CrashLoopBackOff","ImagePullBackOff","ErrImagePull","CreateContainerConfigError","InvalidImageName","RunContainerError"} WatchReasons []string `json:"watchReasons,omitempty"` + // WatchTerminationReasons lists container termination reasons to act on, + // for containers that restart repeatedly without ever settling into a + // watched waiting state. Kubelet forgets its restart backoff once a + // container has stayed up long enough, so a container that dies every + // fifteen minutes restarts immediately every time and never enters + // CrashLoopBackOff, which makes it invisible to WatchReasons. + // + // Empty by default, which leaves behaviour unchanged. "OOMKilled" is the + // safe value to start with. "Error" also works but is broad: it covers + // ordinary crashes, liveness kills and SIGKILL after the grace period + // alike. + // + // A match additionally requires RestartThreshold to be reached and the + // most recent termination to fall inside RestartWindow. + // +optional + WatchTerminationReasons []string `json:"watchTerminationReasons,omitempty"` + + // RestartWindow bounds how recently the last termination must have + // happened for WatchTerminationReasons to match. Without it the check + // would act on a lifetime restart counter that never decays, so a + // workload that misbehaved last month would still be scaled down. + // +kubebuilder:default="1h" + // +kubebuilder:validation:Pattern=`^([0-9]+(\.[0-9]+)?(ns|us|ms|s|m|h))+$` + RestartWindow string `json:"restartWindow,omitempty"` + // RestartThreshold is the number of container restarts before action. // +kubebuilder:default=10 // +kubebuilder:validation:Minimum=1 diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index e985a79..2374a3f 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -76,6 +76,11 @@ func (in *CrashLoopPolicySpec) DeepCopyInto(out *CrashLoopPolicySpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.WatchTerminationReasons != nil { + in, out := &in.WatchTerminationReasons, &out.WatchTerminationReasons + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.AllReplicasFailing != nil { in, out := &in.AllReplicasFailing, &out.AllReplicasFailing *out = new(bool) diff --git a/charts/crashloop-operator/crds/crashloop-operator.lauger.de_crashlooppolicies.yaml b/charts/crashloop-operator/crds/crashloop-operator.lauger.de_crashlooppolicies.yaml index 38c0786..c68c26c 100644 --- a/charts/crashloop-operator/crds/crashloop-operator.lauger.de_crashlooppolicies.yaml +++ b/charts/crashloop-operator/crds/crashloop-operator.lauger.de_crashlooppolicies.yaml @@ -196,6 +196,15 @@ spec: format: int32 minimum: 1 type: integer + restartWindow: + default: 1h + description: |- + RestartWindow bounds how recently the last termination must have + happened for WatchTerminationReasons to match. Without it the check + would act on a lifetime restart counter that never decays, so a + workload that misbehaved last month would still be scaled down. + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|ms|s|m|h))+$ + type: string targets: default: - Deployment @@ -221,6 +230,25 @@ spec: items: type: string type: array + watchTerminationReasons: + description: |- + WatchTerminationReasons lists container termination reasons to act on, + for containers that restart repeatedly without ever settling into a + watched waiting state. Kubelet forgets its restart backoff once a + container has stayed up long enough, so a container that dies every + fifteen minutes restarts immediately every time and never enters + CrashLoopBackOff, which makes it invisible to WatchReasons. + + Empty by default, which leaves behaviour unchanged. "OOMKilled" is the + safe value to start with. "Error" also works but is broad: it covers + ordinary crashes, liveness kills and SIGKILL after the grace period + alike. + + A match additionally requires RestartThreshold to be reached and the + most recent termination to fall inside RestartWindow. + items: + type: string + type: array type: object status: description: CrashLoopPolicyStatus defines the observed state of CrashLoopPolicy. diff --git a/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml b/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml index 38c0786..c68c26c 100644 --- a/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml +++ b/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml @@ -196,6 +196,15 @@ spec: format: int32 minimum: 1 type: integer + restartWindow: + default: 1h + description: |- + RestartWindow bounds how recently the last termination must have + happened for WatchTerminationReasons to match. Without it the check + would act on a lifetime restart counter that never decays, so a + workload that misbehaved last month would still be scaled down. + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|ms|s|m|h))+$ + type: string targets: default: - Deployment @@ -221,6 +230,25 @@ spec: items: type: string type: array + watchTerminationReasons: + description: |- + WatchTerminationReasons lists container termination reasons to act on, + for containers that restart repeatedly without ever settling into a + watched waiting state. Kubelet forgets its restart backoff once a + container has stayed up long enough, so a container that dies every + fifteen minutes restarts immediately every time and never enters + CrashLoopBackOff, which makes it invisible to WatchReasons. + + Empty by default, which leaves behaviour unchanged. "OOMKilled" is the + safe value to start with. "Error" also works but is broad: it covers + ordinary crashes, liveness kills and SIGKILL after the grace period + alike. + + A match additionally requires RestartThreshold to be reached and the + most recent termination to fall inside RestartWindow. + items: + type: string + type: array type: object status: description: CrashLoopPolicyStatus defines the observed state of CrashLoopPolicy. diff --git a/internal/controller/constants.go b/internal/controller/constants.go index 6e295e6..b063ddc 100644 --- a/internal/controller/constants.go +++ b/internal/controller/constants.go @@ -27,6 +27,9 @@ var DefaultWatchReasons = []string{ "RunContainerError", } +// DefaultRestartWindow mirrors the kubebuilder default on spec.restartWindow. +const DefaultRestartWindow = time.Hour + // DefaultTargets mirrors the kubebuilder default on spec.targets. var DefaultTargets = []string{"Deployment", "StatefulSet", "CronJob"} diff --git a/internal/controller/crashlooppolicy_controller.go b/internal/controller/crashlooppolicy_controller.go index 883d399..aa74888 100644 --- a/internal/controller/crashlooppolicy_controller.go +++ b/internal/controller/crashlooppolicy_controller.go @@ -84,6 +84,7 @@ func (r *CrashLoopPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Requ targets := effectiveTargets(policy) requireAllReplicasFailing := effectiveAllReplicasFailing(policy) dryRun := effectiveDryRun(policy) + restartWindow := effectiveRestartWindow(policy) // Every policy sees every pod, so overlapping policies have to agree on who // acts. Load the full set once and let the most restrictive matching policy @@ -105,7 +106,7 @@ func (r *CrashLoopPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Ask the cache only for pods waiting on one of the reasons this policy // watches. Listing every pod in the cluster and discarding the healthy // ones does not scale with cluster size. - pods, err := listPodsByWaitingReasons(ctx, r.Client, watchReasons) + pods, err := listCandidatePods(ctx, r.Client, watchReasons, effectiveTerminationReasons(policy)) if err != nil { logger.Error(err, "failed to list failing pods") return ctrl.Result{}, err @@ -134,16 +135,19 @@ func (r *CrashLoopPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Requ continue } - // Check if pod has a matching failure reason - reason, failing := podHasFailureReason(pod, watchReasons) - if !failing { + // Check if pod has a matching failure reason. A restart loop already + // carries its own restart and recency thresholds, so only the waiting + // path is gated on restartThreshold and durationThreshold here. + reason, waitingFailing := podHasFailureReason(pod, watchReasons) + loopReason, looping := podIsRestartLooping(pod, + effectiveTerminationReasons(policy), restartThreshold, restartWindow) + if !waitingFailing && !looping { continue } - - // Check thresholds: restart count OR duration - restartExceeded := podExceedsRestartThreshold(pod, restartThreshold) - durationExceeded := podExceedsDurationThreshold(pod, durationThreshold) - if !restartExceeded && !durationExceeded { + if !waitingFailing { + reason = loopReason + } else if !podExceedsRestartThreshold(pod, restartThreshold) && + !podExceedsDurationThreshold(pod, durationThreshold) && !looping { continue } @@ -186,7 +190,7 @@ func (r *CrashLoopPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Check if all replicas are failing (if configured) if requireAllReplicasFailing { - allFailing, err := allReplicasFailing(ctx, r.Client, owner, watchReasons) + allFailing, err := allReplicasFailing(ctx, r.Client, owner, policy) if err != nil { logger.Error(err, "failed to check all replicas", "workload", key) loopErrors++ @@ -328,6 +332,11 @@ func (r *CrashLoopPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { ); err != nil { return err } + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), &corev1.Pod{}, IndexPodTerminationReason, podTerminationReasons, + ); err != nil { + return err + } return ctrl.NewControllerManagedBy(mgr). For(&crashloopv1alpha1.CrashLoopPolicy{}). @@ -336,15 +345,17 @@ func (r *CrashLoopPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { handler.EnqueueRequestsFromMapFunc(r.mapPodToPolicy), // Healthy pods vastly outnumber stuck ones and cannot trigger an // action, so filtering them out here keeps the queue quiet. - builder.WithPredicates(predicate.NewPredicateFuncs(podHasWaitingContainer)), + builder.WithPredicates(predicate.NewPredicateFuncs(podIsInteresting)), ). Complete(r) } -// podHasWaitingContainer reports whether any container is waiting with a -// reason, which is the precondition for a pod being interesting to any policy. -func podHasWaitingContainer(obj client.Object) bool { - return len(podWaitingReasons(obj)) > 0 +// podIsInteresting reports whether a pod could matter to any policy: it is +// either waiting with a reason, or it has restarted with a recorded +// termination reason. A pod in a slow restart loop is running when observed, +// so the waiting check alone would drop its events. +func podIsInteresting(obj client.Object) bool { + return len(podWaitingReasons(obj)) > 0 || len(podTerminationReasons(obj)) > 0 } // mapPodToPolicy maps a pod event to the CrashLoopPolicy objects that should be reconciled. diff --git a/internal/controller/crashlooppolicy_controller_test.go b/internal/controller/crashlooppolicy_controller_test.go index 37752cd..b7a4bea 100644 --- a/internal/controller/crashlooppolicy_controller_test.go +++ b/internal/controller/crashlooppolicy_controller_test.go @@ -2,6 +2,7 @@ package controller import ( "regexp" + "strings" "testing" "time" @@ -1093,11 +1094,11 @@ func TestPodWaitingReasons(t *testing.T) { } } -func TestPodHasWaitingContainer(t *testing.T) { - if podHasWaitingContainer(newHealthyPod("healthy", rsOwnerRef())) { +func TestPodIsInteresting(t *testing.T) { + if podIsInteresting(newHealthyPod("healthy", rsOwnerRef())) { t.Error("healthy pod should not pass the watch predicate") } - if !podHasWaitingContainer(newFailingPod("failing", testNamespace, rsOwnerRef(), "ImagePullBackOff", 1)) { + if !podIsInteresting(newFailingPod("failing", testNamespace, rsOwnerRef(), "ImagePullBackOff", 1)) { t.Error("failing pod should pass the watch predicate") } } @@ -1219,7 +1220,7 @@ func TestAllReplicasFailing_PicksTheNewestJobRegardlessOfListOrder(t *testing.T) c := setupTestClient(cj, newJobObj, oldJob, healthyPod, oldPod) owner := &ownerWorkload{Kind: "CronJob", Name: "my-cj", Namespace: testNamespace} - allFailing, err := allReplicasFailing(testCtx(), c, owner, DefaultWatchReasons) + allFailing, err := allReplicasFailing(testCtx(), c, owner, newCrashLoopPolicy("p")) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1227,3 +1228,190 @@ func TestAllReplicasFailing_PicksTheNewestJobRegardlessOfListOrder(t *testing.T) t.Error("expected the newest job to be evaluated, which is healthy") } } + +// newLoopingPod builds the state a slow restart loop actually produces: the +// container is running right now, has restarted many times, and its last exit +// carries the reason and is recent. It is never in a watched waiting state, +// which is exactly why the waiting-based detection cannot see it. +func newLoopingPod(name string, ownerRef metav1.OwnerReference, reason string, restarts int32, diedAgo time.Duration) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testNamespace, + OwnerReferences: []metav1.OwnerReference{ownerRef}, + CreationTimestamp: metav1.NewTime(metav1.Now().Add(-6 * time.Hour)), + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "app", + RestartCount: restarts, + Ready: true, + Started: new(true), + State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}, + LastTerminationState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Reason: reason, + ExitCode: 137, + FinishedAt: metav1.NewTime(metav1.Now().Add(-diedAgo)), + }, + }, + }, + }, + }, + } +} + +func TestPodIsRestartLooping(t *testing.T) { + tests := []struct { + name string + pod *corev1.Pod + reasons []string + want bool + }{ + { + name: "off by default", + pod: newLoopingPod("p", rsOwnerRef(), "OOMKilled", 20, time.Minute), + reasons: nil, + want: false, + }, + { + name: "matches a watched reason", + pod: newLoopingPod("p", rsOwnerRef(), "OOMKilled", 20, time.Minute), + reasons: []string{"OOMKilled"}, + want: true, + }, + { + name: "ignores an unwatched reason", + pod: newLoopingPod("p", rsOwnerRef(), "Error", 20, time.Minute), + reasons: []string{"OOMKilled"}, + want: false, + }, + { + name: "below the restart threshold", + pod: newLoopingPod("p", rsOwnerRef(), "OOMKilled", 2, time.Minute), + reasons: []string{"OOMKilled"}, + want: false, + }, + { + name: "outside the recency window", + // The lifetime counter is high but the last death was weeks ago, + // so the loop is over and must not be acted on. + pod: newLoopingPod("p", rsOwnerRef(), "OOMKilled", 50, 400*time.Hour), + reasons: []string{"OOMKilled"}, + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, got := podIsRestartLooping(tc.pod, tc.reasons, 10, time.Hour) + if got != tc.want { + t.Errorf("podIsRestartLooping() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestPodIsRestartLooping_Guards(t *testing.T) { + reasons := []string{"OOMKilled"} + + t.Run("terminating pod is skipped", func(t *testing.T) { + pod := newLoopingPod("p", rsOwnerRef(), "OOMKilled", 20, time.Minute) + now := metav1.Now() + pod.DeletionTimestamp = &now + if _, ok := podIsRestartLooping(pod, reasons, 10, time.Hour); ok { + t.Error("a pod being deleted must not count; a rollout drains every replica at once") + } + }) + + t.Run("finished pod is skipped", func(t *testing.T) { + pod := newLoopingPod("p", rsOwnerRef(), "OOMKilled", 20, time.Minute) + pod.Status.Phase = corev1.PodSucceeded + if _, ok := podIsRestartLooping(pod, reasons, 10, time.Hour); ok { + t.Error("a completed pod must not count") + } + }) + + t.Run("startup probe still running is skipped", func(t *testing.T) { + pod := newLoopingPod("p", rsOwnerRef(), "OOMKilled", 20, time.Minute) + pod.Status.ContainerStatuses[0].Started = new(false) + if _, ok := podIsRestartLooping(pod, reasons, 10, time.Hour); ok { + t.Error("a container still starting must not count as looping") + } + }) + + t.Run("classic init container is skipped", func(t *testing.T) { + pod := newLoopingPod("p", rsOwnerRef(), "OOMKilled", 20, time.Minute) + pod.Status.InitContainerStatuses = pod.Status.ContainerStatuses + pod.Status.ContainerStatuses = nil + if _, ok := podIsRestartLooping(pod, reasons, 10, time.Hour); ok { + t.Error("a classic init container runs once and cannot be in a loop") + } + }) + + t.Run("restartable init container counts", func(t *testing.T) { + pod := newLoopingPod("p", rsOwnerRef(), "OOMKilled", 20, time.Minute) + pod.Status.InitContainerStatuses = pod.Status.ContainerStatuses + pod.Status.ContainerStatuses = nil + always := corev1.ContainerRestartPolicyAlways + pod.Spec.InitContainers = []corev1.Container{{Name: "app", RestartPolicy: &always}} + if _, ok := podIsRestartLooping(pod, reasons, 10, time.Hour); !ok { + t.Error("a sidecar declared with restartPolicy Always can be in a loop") + } + }) +} + +func TestReconcile_ScalesDownASlowRestartLoop(t *testing.T) { + // End to end: a pod that never enters a watched waiting state, and is + // therefore invisible without the opt-in, is acted on once enabled. + policy := newCrashLoopPolicy("loop-policy", withAllReplicasFailing(false)) + policy.Spec.WatchTerminationReasons = []string{"OOMKilled"} + deploy := newDeployment("leaky", testNamespace, 2) + rs := newReplicaSet("leaky-rs", testNamespace, "leaky") + pod := newLoopingPod("leaky-pod", rsOwnerRef(), "OOMKilled", 25, 3*time.Minute) + pod.OwnerReferences[0].Name = "leaky-rs" + + c := setupTestClient(policy, deploy, rs, pod) + r := newReconciler(c) + + if _, err := r.Reconcile(testCtx(), testRequest("loop-policy")); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + updated := &appsv1.Deployment{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "leaky", Namespace: testNamespace}, updated); err != nil { + t.Fatalf("failed to get deployment: %v", err) + } + if updated.Spec.Replicas == nil || *updated.Spec.Replicas != 0 { + t.Fatal("expected the looping workload to be scaled down") + } + if got := updated.Annotations[AnnotationScaledDownReason]; !strings.Contains(got, "OOMKilled") { + t.Errorf("expected the termination reason to be recorded, got %q", got) + } +} + +func TestReconcile_IgnoresRestartLoopWithoutOptIn(t *testing.T) { + // The same pod, with the policy left at its defaults, must be untouched. + policy := newCrashLoopPolicy("default-policy", withAllReplicasFailing(false)) + deploy := newDeployment("leaky", testNamespace, 2) + rs := newReplicaSet("leaky-rs", testNamespace, "leaky") + pod := newLoopingPod("leaky-pod", rsOwnerRef(), "OOMKilled", 25, 3*time.Minute) + pod.OwnerReferences[0].Name = "leaky-rs" + + c := setupTestClient(policy, deploy, rs, pod) + r := newReconciler(c) + + if _, err := r.Reconcile(testCtx(), testRequest("default-policy")); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + updated := &appsv1.Deployment{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "leaky", Namespace: testNamespace}, updated); err != nil { + t.Fatalf("failed to get deployment: %v", err) + } + if updated.Spec.Replicas != nil && *updated.Spec.Replicas == 0 { + t.Error("expected no action without watchTerminationReasons set") + } +} diff --git a/internal/controller/helpers.go b/internal/controller/helpers.go index 4369ef9..88586f6 100644 --- a/internal/controller/helpers.go +++ b/internal/controller/helpers.go @@ -78,6 +78,90 @@ func podHasFailureReason(pod *corev1.Pod, watchReasons []string) (string, bool) return "", false } +// podIsRestartLooping reports whether a container keeps dying for one of the +// watched termination reasons, and returns that reason. +// +// This covers the loop that WatchReasons cannot see. Kubelet resets its +// restart backoff once a container has stayed up longer than twice the maximum +// backoff, so a container that survives beyond that between deaths restarts +// immediately every time and never enters CrashLoopBackOff. A memory leak is +// the usual shape: run, grow, get OOM-killed, restart at once, repeat. +// +// The recency bound is what makes this safe. RestartCount is cumulative for a +// pod's whole life and never decays, so without it a workload that misbehaved +// last month would still be acted on. +func podIsRestartLooping(pod *corev1.Pod, reasons []string, threshold int32, window time.Duration) (string, bool) { + if len(reasons) == 0 { + return "", false + } + // A pod on its way out restarts nothing and would otherwise sweep every + // replica of a rolling update into the candidate set at once. + if pod.DeletionTimestamp != nil { + return "", false + } + if pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed { + return "", false + } + + reasonSet := make(map[string]struct{}, len(reasons)) + for _, r := range reasons { + reasonSet[r] = struct{}{} + } + + restartable := restartableInitContainers(pod) + check := func(statuses []corev1.ContainerStatus, initOnly bool) (string, bool) { + for _, cs := range statuses { + if initOnly { + // A classic init container runs once; only sidecars, which + // declare restartPolicy Always, can be in a restart loop. + if _, ok := restartable[cs.Name]; !ok { + continue + } + } + // The container must still be part of the running pod, and past + // any startup probe, so a slow start is not mistaken for a loop. + if cs.State.Terminated != nil { + continue + } + if cs.Started != nil && !*cs.Started { + continue + } + if cs.RestartCount < threshold { + continue + } + term := cs.LastTerminationState.Terminated + if term == nil || term.Reason == "" { + continue + } + if _, ok := reasonSet[term.Reason]; !ok { + continue + } + if term.FinishedAt.IsZero() || time.Since(term.FinishedAt.Time) > window { + continue + } + return term.Reason, true + } + return "", false + } + + if reason, ok := check(pod.Status.ContainerStatuses, false); ok { + return reason, true + } + return check(pod.Status.InitContainerStatuses, true) +} + +// restartableInitContainers returns the names of init containers declared with +// restartPolicy Always, which Kubernetes treats as sidecars. +func restartableInitContainers(pod *corev1.Pod) map[string]struct{} { + names := make(map[string]struct{}) + for _, c := range pod.Spec.InitContainers { + if c.RestartPolicy != nil && *c.RestartPolicy == corev1.ContainerRestartPolicyAlways { + names[c.Name] = struct{}{} + } + } + return names +} + // podExceedsRestartThreshold checks if any container has restarted more than threshold times. func podExceedsRestartThreshold(pod *corev1.Pod, threshold int32) bool { for _, cs := range pod.Status.ContainerStatuses { @@ -237,8 +321,12 @@ func parseDuration(s string, fallback time.Duration) (time.Duration, bool) { return d, true } -// allReplicasFailing checks if all pods of a workload are in a failing state. -func allReplicasFailing(ctx context.Context, c client.Client, owner *ownerWorkload, watchReasons []string) (bool, error) { +// allReplicasFailing reports whether every replica of the workload is failing +// in a way the policy acts on. It takes the whole policy rather than a reason +// list so that it cannot disagree with the reconcile loop about what counts as +// failing, which would silently make an opted-in restart loop inert whenever +// allReplicasFailing is true, as it is by default. +func allReplicasFailing(ctx context.Context, c client.Client, owner *ownerWorkload, policy *crashloopv1alpha1.CrashLoopPolicy) (bool, error) { switch owner.Kind { case "Deployment": deploy := &appsv1.Deployment{} @@ -265,7 +353,7 @@ func allReplicasFailing(ctx context.Context, c client.Client, owner *ownerWorklo return false, nil } for i := range podList.Items { - if _, failing := podHasFailureReason(&podList.Items[i], watchReasons); !failing { + if !podMatchesPolicy(policy, &podList.Items[i]) { return false, nil } } @@ -295,7 +383,7 @@ func allReplicasFailing(ctx context.Context, c client.Client, owner *ownerWorklo return false, nil } for i := range podList.Items { - if _, failing := podHasFailureReason(&podList.Items[i], watchReasons); !failing { + if !podMatchesPolicy(policy, &podList.Items[i]) { return false, nil } } @@ -347,7 +435,7 @@ func allReplicasFailing(ctx context.Context, c client.Client, owner *ownerWorklo return false, nil } for i := range jobPods { - if _, failing := podHasFailureReason(&jobPods[i], watchReasons); !failing { + if !podMatchesPolicy(policy, &jobPods[i]) { return false, nil } } diff --git a/internal/controller/indexes.go b/internal/controller/indexes.go index fbd6d21..10241c4 100644 --- a/internal/controller/indexes.go +++ b/internal/controller/indexes.go @@ -44,6 +44,100 @@ func podWaitingReasons(obj client.Object) []string { return reasons } +// IndexPodTerminationReason indexes pods by the termination reason of their +// last container exit. It is a separate index from the waiting one because the +// two describe different states: a container that is currently down versus one +// that keeps dying but is running right now. +const IndexPodTerminationReason = "status.terminationReason" + +// podTerminationReasons returns the distinct last-termination reasons across a +// pod's containers, for containers that have actually restarted. A pod that +// has never restarted yields nothing, so the index stays small. +func podTerminationReasons(obj client.Object) []string { + pod, ok := obj.(*corev1.Pod) + if !ok { + return nil + } + + seen := make(map[string]struct{}) + collect := func(statuses []corev1.ContainerStatus) { + for _, cs := range statuses { + if cs.RestartCount == 0 { + continue + } + if term := cs.LastTerminationState.Terminated; term != nil && term.Reason != "" { + seen[term.Reason] = struct{}{} + } + } + } + collect(pod.Status.ContainerStatuses) + collect(pod.Status.InitContainerStatuses) + + if len(seen) == 0 { + return nil + } + reasons := make([]string, 0, len(seen)) + for r := range seen { + reasons = append(reasons, r) + } + return reasons +} + +// listPodsByIndex returns the pods matching any of the given values on the +// given index, deduplicated by UID since a pod can match several at once. +func listPodsByIndex(ctx context.Context, c client.Client, index string, values []string) ([]corev1.Pod, error) { + var pods []corev1.Pod + seen := make(map[types.UID]struct{}) + + for _, value := range values { + podList := &corev1.PodList{} + if err := c.List(ctx, podList, client.MatchingFields{index: value}); err != nil { + return nil, err + } + for i := range podList.Items { + pod := podList.Items[i] + if _, dup := seen[pod.UID]; dup { + continue + } + seen[pod.UID] = struct{}{} + pods = append(pods, pod) + } + } + return pods, nil +} + +// listCandidatePods returns the pods a policy could act on: those waiting with +// a watched reason, plus, when the policy opts in, those that keep restarting +// with a watched termination reason. +func listCandidatePods( + ctx context.Context, c client.Client, waitingReasons, terminationReasons []string, +) ([]corev1.Pod, error) { + pods, err := listPodsByIndex(ctx, c, IndexPodWaitingReason, waitingReasons) + if err != nil { + return nil, err + } + if len(terminationReasons) == 0 { + return pods, nil + } + + restarting, err := listPodsByIndex(ctx, c, IndexPodTerminationReason, terminationReasons) + if err != nil { + return nil, err + } + + seen := make(map[types.UID]struct{}, len(pods)) + for i := range pods { + seen[pods[i].UID] = struct{}{} + } + for i := range restarting { + if _, dup := seen[restarting[i].UID]; dup { + continue + } + pods = append(pods, restarting[i]) + } + return pods, nil +} + // listPodsByWaitingReasons returns the pods matching any of the given waiting // reasons, deduplicated by UID since a pod can match several reasons at once. func listPodsByWaitingReasons(ctx context.Context, c client.Client, reasons []string) ([]corev1.Pod, error) { diff --git a/internal/controller/policyresolve.go b/internal/controller/policyresolve.go index ba045e0..b714aa1 100644 --- a/internal/controller/policyresolve.go +++ b/internal/controller/policyresolve.go @@ -65,6 +65,31 @@ func effectiveDryRun(p *crashloopv1alpha1.CrashLoopPolicy) bool { return p.Spec.DryRun != nil && *p.Spec.DryRun } +func effectiveTerminationReasons(p *crashloopv1alpha1.CrashLoopPolicy) []string { + return p.Spec.WatchTerminationReasons +} + +func effectiveRestartWindow(p *crashloopv1alpha1.CrashLoopPolicy) time.Duration { + d, _ := parseDuration(p.Spec.RestartWindow, DefaultRestartWindow) + return d +} + +// podMatchesPolicy reports whether the pod is failing in a way this policy +// acts on. Both the waiting state and the restart loop are checked here so +// that the sibling check in allReplicasFailing cannot disagree with the +// reconcile loop about what counts as failing, which would leave an opted-in +// restart loop inert whenever allReplicasFailing is true, as it is by default. +func podMatchesPolicy(p *crashloopv1alpha1.CrashLoopPolicy, pod *corev1.Pod) bool { + if _, ok := podHasFailureReason(pod, effectiveWatchReasons(p)); ok { + return true + } + _, looping := podIsRestartLooping(pod, + effectiveTerminationReasons(p), + effectiveRestartThreshold(p), + effectiveRestartWindow(p)) + return looping +} + // isMoreRestrictive reports whether a would act on a workload sooner, or more // forcefully, than b. The comparison is a total order so that the winner among // a set of matching policies is deterministic and independent of list order: @@ -145,10 +170,18 @@ func policyWouldAct( } watchReasons := effectiveWatchReasons(policy) - if _, failing := podHasFailureReason(pod, watchReasons); !failing { + _, waitingFailing := podHasFailureReason(pod, watchReasons) + _, looping := podIsRestartLooping(pod, + effectiveTerminationReasons(policy), + effectiveRestartThreshold(policy), + effectiveRestartWindow(policy)) + if !waitingFailing && !looping { return false, nil } - if !podExceedsRestartThreshold(pod, effectiveRestartThreshold(policy)) && + // A restart loop carries its own thresholds, so the waiting-state + // thresholds only gate the waiting path. + if !looping && + !podExceedsRestartThreshold(pod, effectiveRestartThreshold(policy)) && !podExceedsDurationThreshold(pod, effectiveDurationThreshold(policy)) { return false, nil } @@ -164,7 +197,7 @@ func policyWouldAct( } if effectiveAllReplicasFailing(policy) { - allFailing, err := allReplicasFailing(ctx, c, owner, watchReasons) + allFailing, err := allReplicasFailing(ctx, c, owner, policy) if err != nil { return false, err } diff --git a/internal/controller/testutil_test.go b/internal/controller/testutil_test.go index 1481371..617c348 100644 --- a/internal/controller/testutil_test.go +++ b/internal/controller/testutil_test.go @@ -44,6 +44,7 @@ func setupTestClient(objs ...client.Object) client.Client { // Mirrors the index SetupWithManager registers on the real cache, so // tests exercise the same indexed lookup the operator uses. WithIndex(&corev1.Pod{}, IndexPodWaitingReason, podWaitingReasons). + WithIndex(&corev1.Pod{}, IndexPodTerminationReason, podTerminationReasons). Build() }