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 }