From 071684cd41444174c191ffda2049a49d2739dc77 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Sun, 6 Sep 2026 12:03:53 +0200 Subject: [PATCH] feat(api): correlate failures by container name across replicas allReplicasFailing worked at pod level, requiring every replica to be failing in some way. That conflates a systematic cause, the same container broken in every replica because of a bad image or a missing Secret, with unrelated failures that happen to coincide. Adds failureCorrelation with Pod, the existing behaviour and the default, and Container, which additionally requires the same container name to be the failing one everywhere. Since the operator never scales anything back up, a stricter option is worth having. A new field rather than an enum on allReplicasFailing, which would have changed the type of a released field. The three per-kind branches ran identical sibling loops, so they now share one evaluator and cannot drift apart. isMoreRestrictive gains the new dimension: container correlation acts in fewer situations, so a policy using it ranks below one that does not, keeping the winner selection a total order. Closes #70 Signed-off-by: Simon Lauger --- README.md | 10 +- api/v1alpha1/crashlooppolicy_types.go | 15 +++ ...-operator.lauger.de_crashlooppolicies.yaml | 17 ++++ ...-operator.lauger.de_crashlooppolicies.yaml | 17 ++++ .../crashlooppolicy_controller_test.go | 89 +++++++++++++++++ internal/controller/helpers.go | 21 +--- internal/controller/policyresolve.go | 95 +++++++++++++++++++ 7 files changed, 244 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index aaf558c..2453686 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,7 @@ Short name: `clp` (`kubectl get clp`). | `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 | +| `failureCorrelation` | `Pod` | How replicas are compared when `allReplicasFailing` is true. `Container` additionally requires the same container name to be the failing one everywhere | | `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) | | `excludeNamespaces` | `[kube-system, kube-public, kube-node-lease]` | Namespaces to ignore (applied after namespaceSelector) | @@ -175,8 +176,9 @@ Restrictiveness is compared in this order, and the first difference decides: 1. Lower `restartThreshold` 2. Shorter `durationThreshold` 3. `allReplicasFailing: false` before `true`, since it also acts on partial failure -4. `dryRun: false` before `true`, so a real action outranks a simulated one -5. Name in ascending order, purely to break a remaining tie +4. `failureCorrelation: Pod` before `Container`, since it acts in more situations +5. `dryRun: false` before `true`, so a real action outranks a simulated one +6. Name in ascending order, purely to break a remaining tie Because the order is total, the winner does not depend on the order in which policies are evaluated. @@ -289,6 +291,10 @@ work through the conditions the operator applies, in the order it applies them: 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. +- **`failureCorrelation: Container` and the replicas fail in different + containers.** The stricter mode holds off when the failures do not share a + container name, on the grounds that they look coincidental rather than + systematic. Switch back to `Pod` if you want any broken replica to count. - **`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/api/v1alpha1/crashlooppolicy_types.go b/api/v1alpha1/crashlooppolicy_types.go index 576c4b8..4f870e4 100644 --- a/api/v1alpha1/crashlooppolicy_types.go +++ b/api/v1alpha1/crashlooppolicy_types.go @@ -64,6 +64,21 @@ type CrashLoopPolicySpec struct { // +optional AllReplicasFailing *bool `json:"allReplicasFailing,omitempty"` + // FailureCorrelation controls how AllReplicasFailing compares replicas. + // + // Pod, the default, requires every replica to be failing in some way. + // Container additionally requires the same container name to be the + // failing one in every replica, which distinguishes a systematic problem + // such as a bad image or a missing Secret from unrelated failures that + // happen to coincide. It is the stricter setting and acts in fewer + // situations. + // + // Has no effect when AllReplicasFailing is false. + // +kubebuilder:default=Pod + // +kubebuilder:validation:Enum=Pod;Container + // +optional + FailureCorrelation string `json:"failureCorrelation,omitempty"` + // Targets lists workload types to act on. // +kubebuilder:default={"Deployment","StatefulSet","CronJob"} // +kubebuilder:validation:items:Enum=Deployment;StatefulSet;CronJob 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 c68c26c..f0f8e14 100644 --- a/charts/crashloop-operator/crds/crashloop-operator.lauger.de_crashlooppolicies.yaml +++ b/charts/crashloop-operator/crds/crashloop-operator.lauger.de_crashlooppolicies.yaml @@ -133,6 +133,23 @@ spec: type: object type: object x-kubernetes-map-type: atomic + failureCorrelation: + default: Pod + description: |- + FailureCorrelation controls how AllReplicasFailing compares replicas. + + Pod, the default, requires every replica to be failing in some way. + Container additionally requires the same container name to be the + failing one in every replica, which distinguishes a systematic problem + such as a bad image or a missing Secret from unrelated failures that + happen to coincide. It is the stricter setting and acts in fewer + situations. + + Has no effect when AllReplicasFailing is false. + enum: + - Pod + - Container + type: string namespaceSelector: description: |- NamespaceSelector selects namespaces by labels. If set, only pods in diff --git a/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml b/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml index c68c26c..f0f8e14 100644 --- a/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml +++ b/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml @@ -133,6 +133,23 @@ spec: type: object type: object x-kubernetes-map-type: atomic + failureCorrelation: + default: Pod + description: |- + FailureCorrelation controls how AllReplicasFailing compares replicas. + + Pod, the default, requires every replica to be failing in some way. + Container additionally requires the same container name to be the + failing one in every replica, which distinguishes a systematic problem + such as a bad image or a missing Secret from unrelated failures that + happen to coincide. It is the stricter setting and acts in fewer + situations. + + Has no effect when AllReplicasFailing is false. + enum: + - Pod + - Container + type: string namespaceSelector: description: |- NamespaceSelector selects namespaces by labels. If set, only pods in diff --git a/internal/controller/crashlooppolicy_controller_test.go b/internal/controller/crashlooppolicy_controller_test.go index 267944c..2d82bb7 100644 --- a/internal/controller/crashlooppolicy_controller_test.go +++ b/internal/controller/crashlooppolicy_controller_test.go @@ -1493,3 +1493,92 @@ func TestScaleWorkloadToZero_DryRunTouchesNothing(t *testing.T) { t.Errorf("dry run must not write annotations, got %v", updated.Annotations) } } + +// newMultiContainerFailingPod builds a pod where exactly one named container is +// in a watched waiting state and the other is healthy. +func newMultiContainerFailingPod(name, failingContainer string) *corev1.Pod { + statusFor := func(cname string, failing bool) corev1.ContainerStatus { + cs := corev1.ContainerStatus{Name: cname, RestartCount: 12, Started: new(true)} + if failing { + cs.State = corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{Reason: "CrashLoopBackOff"}, + } + } else { + cs.Ready = true + cs.State = corev1.ContainerState{Running: &corev1.ContainerStateRunning{}} + } + return cs + } + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testNamespace, + Labels: map[string]string{"app": "my-app"}, + OwnerReferences: []metav1.OwnerReference{rsOwnerRef()}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{ + Type: corev1.ContainersReady, + Status: corev1.ConditionFalse, + LastTransitionTime: metav1.NewTime(metav1.Now().Add(-2 * time.Hour)), + }}, + ContainerStatuses: []corev1.ContainerStatus{ + statusFor("app", failingContainer == "app"), + statusFor("sidecar", failingContainer == "sidecar"), + }, + }, + } +} + +func TestReplicasAllFailing_Correlation(t *testing.T) { + sameContainer := []corev1.Pod{ + *newMultiContainerFailingPod("p1", "app"), + *newMultiContainerFailingPod("p2", "app"), + } + differentContainers := []corev1.Pod{ + *newMultiContainerFailingPod("p1", "app"), + *newMultiContainerFailingPod("p2", "sidecar"), + } + + podMode := newCrashLoopPolicy("pod-mode") + containerMode := newCrashLoopPolicy("container-mode") + containerMode.Spec.FailureCorrelation = CorrelationContainer + + tests := []struct { + name string + policy *crashloopv1alpha1.CrashLoopPolicy + pods []corev1.Pod + want bool + }{ + {"pod mode, same container", podMode, sameContainer, true}, + // Every pod is broken, so pod correlation acts. + {"pod mode, different containers", podMode, differentContainers, true}, + {"container mode, same container", containerMode, sameContainer, true}, + // The failures do not share a container, so this looks coincidental + // rather than systematic and the stricter mode holds off. + {"container mode, different containers", containerMode, differentContainers, false}, + {"no pods", podMode, nil, false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := replicasAllFailing(tc.policy, tc.pods); got != tc.want { + t.Errorf("replicasAllFailing() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestIsMoreRestrictive_PodCorrelationBeatsContainer(t *testing.T) { + podMode := newCrashLoopPolicy("aaa") + containerMode := newCrashLoopPolicy("aaa") + containerMode.Spec.FailureCorrelation = CorrelationContainer + + if !isMoreRestrictive(podMode, containerMode) { + t.Error("pod correlation acts in more situations and must rank as more restrictive") + } + if isMoreRestrictive(containerMode, podMode) { + t.Error("the ordering must not be symmetric") + } +} diff --git a/internal/controller/helpers.go b/internal/controller/helpers.go index cfb8c80..b7bc9bc 100644 --- a/internal/controller/helpers.go +++ b/internal/controller/helpers.go @@ -353,12 +353,7 @@ func allReplicasFailing(ctx context.Context, c client.Client, owner *ownerWorklo if len(podList.Items) == 0 { return false, nil } - for i := range podList.Items { - if !podMatchesPolicy(policy, &podList.Items[i]) { - return false, nil - } - } - return true, nil + return replicasAllFailing(policy, podList.Items), nil case "StatefulSet": sts := &appsv1.StatefulSet{} @@ -383,12 +378,7 @@ func allReplicasFailing(ctx context.Context, c client.Client, owner *ownerWorklo if len(podList.Items) == 0 { return false, nil } - for i := range podList.Items { - if !podMatchesPolicy(policy, &podList.Items[i]) { - return false, nil - } - } - return true, nil + return replicasAllFailing(policy, podList.Items), nil case "CronJob": cj := &batchv1.CronJob{} @@ -435,12 +425,7 @@ func allReplicasFailing(ctx context.Context, c client.Client, owner *ownerWorklo if len(jobPods) == 0 { return false, nil } - for i := range jobPods { - if !podMatchesPolicy(policy, &jobPods[i]) { - return false, nil - } - } - return true, nil + return replicasAllFailing(policy, jobPods), nil } return false, nil } diff --git a/internal/controller/policyresolve.go b/internal/controller/policyresolve.go index b714aa1..abeea61 100644 --- a/internal/controller/policyresolve.go +++ b/internal/controller/policyresolve.go @@ -65,6 +65,95 @@ func effectiveDryRun(p *crashloopv1alpha1.CrashLoopPolicy) bool { return p.Spec.DryRun != nil && *p.Spec.DryRun } +// FailureCorrelation modes. +const ( + CorrelationPod = "Pod" + CorrelationContainer = "Container" +) + +func effectiveFailureCorrelation(p *crashloopv1alpha1.CrashLoopPolicy) string { + if p.Spec.FailureCorrelation == CorrelationContainer { + return CorrelationContainer + } + return CorrelationPod +} + +// failingContainerNames returns the names of the containers that make this pod +// count as failing for the policy. Used to tell a systematic failure, the same +// container broken everywhere, from unrelated ones that coincide. +func failingContainerNames(p *crashloopv1alpha1.CrashLoopPolicy, pod *corev1.Pod) []string { + watchReasons := effectiveWatchReasons(p) + terminationReasons := effectiveTerminationReasons(p) + threshold := effectiveRestartThreshold(p) + window := effectiveRestartWindow(p) + + var names []string + for _, statuses := range [][]corev1.ContainerStatus{ + pod.Status.ContainerStatuses, pod.Status.InitContainerStatuses, + } { + for i := range statuses { + single := &corev1.Pod{ + ObjectMeta: pod.ObjectMeta, + Spec: pod.Spec, + Status: corev1.PodStatus{ + Phase: pod.Status.Phase, + ContainerStatuses: statuses[i : i+1], + }, + } + if _, ok := podHasFailureReason(single, watchReasons); ok { + names = append(names, statuses[i].Name) + continue + } + if _, ok := podIsRestartLooping(single, terminationReasons, threshold, window); ok { + names = append(names, statuses[i].Name) + } + } + } + return names +} + +// replicasAllFailing reports whether every replica counts as failing under the +// policy's correlation mode. +func replicasAllFailing(p *crashloopv1alpha1.CrashLoopPolicy, pods []corev1.Pod) bool { + if len(pods) == 0 { + return false + } + if effectiveFailureCorrelation(p) == CorrelationPod { + for i := range pods { + if !podMatchesPolicy(p, &pods[i]) { + return false + } + } + return true + } + + // Container mode: some container name must be failing in every replica. + var shared map[string]struct{} + for i := range pods { + names := failingContainerNames(p, &pods[i]) + if len(names) == 0 { + return false + } + current := make(map[string]struct{}, len(names)) + for _, n := range names { + current[n] = struct{}{} + } + if shared == nil { + shared = current + continue + } + for n := range shared { + if _, ok := current[n]; !ok { + delete(shared, n) + } + } + if len(shared) == 0 { + return false + } + } + return len(shared) > 0 +} + func effectiveTerminationReasons(p *crashloopv1alpha1.CrashLoopPolicy) []string { return p.Spec.WatchTerminationReasons } @@ -97,6 +186,7 @@ func podMatchesPolicy(p *crashloopv1alpha1.CrashLoopPolicy, pod *corev1.Pod) boo // 1. lower restartThreshold // 2. shorter durationThreshold // 3. allReplicasFailing false before true (acts on partial failure too) +// 3a. pod correlation before container correlation (acts in more situations) // 4. dryRun false before true (a real action outranks a simulated one) // 5. name ascending, purely to break remaining ties func isMoreRestrictive(a, b *crashloopv1alpha1.CrashLoopPolicy) bool { @@ -109,6 +199,11 @@ func isMoreRestrictive(a, b *crashloopv1alpha1.CrashLoopPolicy) bool { if aa, ab := effectiveAllReplicasFailing(a), effectiveAllReplicasFailing(b); aa != ab { return !aa } + // Container correlation acts in fewer situations than pod correlation, so + // a policy using it is the less restrictive of the two. + if ca, cb := effectiveFailureCorrelation(a), effectiveFailureCorrelation(b); ca != cb { + return ca == CorrelationPod + } if wa, wb := effectiveDryRun(a), effectiveDryRun(b); wa != wb { return !wa }