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() }