From c5142ee5ecf38bd495a413e9b38df4262ae1c04c Mon Sep 17 00:00:00 2001 From: Annie Li Date: Fri, 6 Mar 2026 11:27:31 -0800 Subject: [PATCH 1/4] add fix to recreate non running pods in syncStatefulsets --- pkg/cluster/pod.go | 30 ++++- pkg/cluster/pod_test.go | 280 +++++++++++++++++++++++++++++++++++++++ pkg/cluster/sync.go | 16 ++- pkg/cluster/sync_test.go | 133 +++++++++++++++++++ 4 files changed, 456 insertions(+), 3 deletions(-) diff --git a/pkg/cluster/pod.go b/pkg/cluster/pod.go index 959bacb54..c99c1c322 100644 --- a/pkg/cluster/pod.go +++ b/pkg/cluster/pod.go @@ -376,6 +376,30 @@ func (c *Cluster) getPatroniMemberData(pod *v1.Pod) (patroni.MemberData, error) return memberData, nil } +// podIsNotRunning returns true if a pod is not in a healthy running state, +// e.g. stuck in CreateContainerConfigError, CrashLoopBackOff, ImagePullBackOff, etc. +func podIsNotRunning(pod *v1.Pod) bool { + if pod.Status.Phase != v1.PodRunning { + return true + } + for _, cs := range pod.Status.ContainerStatuses { + if cs.State.Waiting != nil || cs.State.Terminated != nil { + return true + } + } + return false +} + +// allPodsRunning returns true only if every pod in the list is in a healthy running state. +func (c *Cluster) allPodsRunning(pods []v1.Pod) bool { + for i := range pods { + if podIsNotRunning(&pods[i]) { + return false + } + } + return true +} + func (c *Cluster) recreatePod(podName spec.NamespacedName) (*v1.Pod, error) { stopCh := make(chan struct{}) ch := c.registerPodSubscriber(podName) @@ -444,7 +468,8 @@ func (c *Cluster) recreatePods(pods []v1.Pod, switchoverCandidates []spec.Namesp // switchover if // 1. we have not observed a new master pod when re-creating former replicas // 2. we know possible switchover targets even when no replicas were recreated - if newMasterPod == nil && len(replicas) > 0 { + // 3. the master pod is actually running (can't switchover a dead master) + if newMasterPod == nil && len(replicas) > 0 && !podIsNotRunning(masterPod) { masterCandidate, err := c.getSwitchoverCandidate(masterPod) if err != nil { // do not recreate master now so it will keep the update flag and switchover will be retried on next sync @@ -455,6 +480,9 @@ func (c *Cluster) recreatePods(pods []v1.Pod, switchoverCandidates []spec.Namesp } } else if newMasterPod == nil && len(replicas) == 0 { c.logger.Warningf("cannot perform switch over before re-creating the pod: no replicas") + } else if podIsNotRunning(masterPod) { + c.logger.Warningf("master pod %q is not running, skipping switchover and recreating directly", + util.NameFromMeta(masterPod.ObjectMeta)) } c.logger.Infof("recreating old master pod %q", util.NameFromMeta(masterPod.ObjectMeta)) diff --git a/pkg/cluster/pod_test.go b/pkg/cluster/pod_test.go index 6816b4d7a..90a182472 100644 --- a/pkg/cluster/pod_test.go +++ b/pkg/cluster/pod_test.go @@ -112,3 +112,283 @@ func TestGetSwitchoverCandidate(t *testing.T) { } } } + +func TestPodIsNotRunning(t *testing.T) { + tests := []struct { + subtest string + pod v1.Pod + expected bool + }{ + { + subtest: "pod running with all containers ready", + pod: v1.Pod{ + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Running: &v1.ContainerStateRunning{}, + }, + }, + }, + }, + }, + expected: false, + }, + { + subtest: "pod in pending phase", + pod: v1.Pod{ + Status: v1.PodStatus{ + Phase: v1.PodPending, + }, + }, + expected: true, + }, + { + subtest: "pod running but container in CreateContainerConfigError", + pod: v1.Pod{ + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Waiting: &v1.ContainerStateWaiting{ + Reason: "CreateContainerConfigError", + Message: `secret "some-secret" not found`, + }, + }, + }, + }, + }, + }, + expected: true, + }, + { + subtest: "pod running but container in CrashLoopBackOff", + pod: v1.Pod{ + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Waiting: &v1.ContainerStateWaiting{ + Reason: "CrashLoopBackOff", + }, + }, + }, + }, + }, + }, + expected: true, + }, + { + subtest: "pod running but container terminated", + pod: v1.Pod{ + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Terminated: &v1.ContainerStateTerminated{ + ExitCode: 137, + }, + }, + }, + }, + }, + }, + expected: true, + }, + { + subtest: "pod running with mixed container states - one healthy one broken", + pod: v1.Pod{ + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Running: &v1.ContainerStateRunning{}, + }, + }, + { + State: v1.ContainerState{ + Waiting: &v1.ContainerStateWaiting{ + Reason: "CreateContainerConfigError", + }, + }, + }, + }, + }, + }, + expected: true, + }, + { + subtest: "pod in failed phase", + pod: v1.Pod{ + Status: v1.PodStatus{ + Phase: v1.PodFailed, + }, + }, + expected: true, + }, + { + subtest: "pod running with multiple healthy containers", + pod: v1.Pod{ + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Running: &v1.ContainerStateRunning{}, + }, + }, + { + State: v1.ContainerState{ + Running: &v1.ContainerStateRunning{}, + }, + }, + }, + }, + }, + expected: false, + }, + { + subtest: "pod running with ImagePullBackOff", + pod: v1.Pod{ + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Waiting: &v1.ContainerStateWaiting{ + Reason: "ImagePullBackOff", + }, + }, + }, + }, + }, + }, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.subtest, func(t *testing.T) { + result := podIsNotRunning(&tt.pod) + if result != tt.expected { + t.Errorf("podIsNotRunning() = %v, expected %v", result, tt.expected) + } + }) + } +} + +func TestAllPodsRunning(t *testing.T) { + client, _ := newFakeK8sSyncClient() + + var cluster = New( + Config{ + OpConfig: config.Config{ + Resources: config.Resources{ + ClusterLabels: map[string]string{"application": "spilo"}, + ClusterNameLabel: "cluster-name", + PodRoleLabel: "spilo-role", + }, + }, + }, client, acidv1.Postgresql{}, logger, eventRecorder) + + tests := []struct { + subtest string + pods []v1.Pod + expected bool + }{ + { + subtest: "all pods running", + pods: []v1.Pod{ + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + {State: v1.ContainerState{Running: &v1.ContainerStateRunning{}}}, + }, + }, + }, + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + {State: v1.ContainerState{Running: &v1.ContainerStateRunning{}}}, + }, + }, + }, + }, + expected: true, + }, + { + subtest: "one pod not running", + pods: []v1.Pod{ + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + {State: v1.ContainerState{Running: &v1.ContainerStateRunning{}}}, + }, + }, + }, + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Waiting: &v1.ContainerStateWaiting{ + Reason: "CreateContainerConfigError", + }, + }, + }, + }, + }, + }, + }, + expected: false, + }, + { + subtest: "all pods not running", + pods: []v1.Pod{ + { + Status: v1.PodStatus{ + Phase: v1.PodPending, + }, + }, + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Waiting: &v1.ContainerStateWaiting{ + Reason: "CrashLoopBackOff", + }, + }, + }, + }, + }, + }, + }, + expected: false, + }, + { + subtest: "empty pod list", + pods: []v1.Pod{}, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.subtest, func(t *testing.T) { + result := cluster.allPodsRunning(tt.pods) + if result != tt.expected { + t.Errorf("allPodsRunning() = %v, expected %v", result, tt.expected) + } + }) + } +} diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index ffebd306c..771965eba 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -718,14 +718,26 @@ func (c *Cluster) syncStatefulSet() error { if configPatched, restartPrimaryFirst, restartWait, err = c.syncPatroniConfig(pods, c.Spec.Patroni, requiredPgParameters); err != nil { c.logger.Warningf("Patroni config updated? %v - errors during config sync: %v", configPatched, err) postponeReasons = append(postponeReasons, "errors during Patroni config sync") - isSafeToRecreatePods = false + // Only mark unsafe if all pods are running. If some pods are not running, + // Patroni API errors are expected and should not block pod recreation, + // which is the only way to fix non-running pods. + if c.allPodsRunning(pods) { + isSafeToRecreatePods = false + } else { + c.logger.Warningf("ignoring Patroni config sync errors because some pods are not running") + } } // restart Postgres where it is still pending if err = c.restartInstances(pods, restartWait, restartPrimaryFirst); err != nil { c.logger.Errorf("errors while restarting Postgres in pods via Patroni API: %v", err) postponeReasons = append(postponeReasons, "errors while restarting Postgres via Patroni API") - isSafeToRecreatePods = false + // Same logic: don't let unreachable non-running pods block recreation. + if c.allPodsRunning(pods) { + isSafeToRecreatePods = false + } else { + c.logger.Warningf("ignoring Patroni restart errors because some pods are not running") + } } // if we get here we also need to re-create the pods (either leftovers from the old diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index f7d46d427..8685dbdb1 100644 --- a/pkg/cluster/sync_test.go +++ b/pkg/cluster/sync_test.go @@ -1053,3 +1053,136 @@ func TestUpdateSecretNameConflict(t *testing.T) { expectedError := fmt.Sprintf("syncing secret %s failed: error while checking for password rotation: could not update secret because of user name mismatch", "default/prepared-owner-user.acid-test-cluster.credentials") assert.Contains(t, err.Error(), expectedError) } + +func TestSyncStatefulSetNonRunningPodsDoNotBlockRecreation(t *testing.T) { + testName := "test that non-running pods do not block rolling update" + client, _ := newFakeK8sSyncClient() + clusterName := "acid-test-cluster" + namespace := "default" + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + pg := acidv1.Postgresql{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Namespace: namespace, + }, + Spec: acidv1.PostgresSpec{ + NumberOfInstances: 1, + Volume: acidv1.Volume{ + Size: "1Gi", + }, + }, + } + + var cluster = New( + Config{ + OpConfig: config.Config{ + PatroniAPICheckInterval: time.Duration(1), + PatroniAPICheckTimeout: time.Duration(5), + PodManagementPolicy: "ordered_ready", + Resources: config.Resources{ + ClusterLabels: map[string]string{"application": "spilo"}, + ClusterNameLabel: "cluster-name", + DefaultCPURequest: "300m", + DefaultCPULimit: "300m", + DefaultMemoryRequest: "300Mi", + DefaultMemoryLimit: "300Mi", + PodRoleLabel: "spilo-role", + ResourceCheckInterval: time.Duration(3), + ResourceCheckTimeout: time.Duration(10), + }, + }, + }, client, pg, logger, eventRecorder) + + cluster.Name = clusterName + cluster.Namespace = namespace + + // mock Patroni API that always fails (simulates unreachable pod) + mockClient := mocks.NewMockHTTPClient(ctrl) + mockClient.EXPECT().Do(gomock.Any()).Return(nil, fmt.Errorf("connection refused")).AnyTimes() + mockClient.EXPECT().Get(gomock.Any()).Return(nil, fmt.Errorf("connection refused")).AnyTimes() + cluster.patroni = patroni.New(patroniLogger, mockClient) + + // test allPodsRunning with non-running pods returns false + nonRunningPods := []v1.Pod{ + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Waiting: &v1.ContainerStateWaiting{ + Reason: "CreateContainerConfigError", + Message: `secret "old-secret" not found`, + }, + }, + }, + }, + }, + }, + } + + if cluster.allPodsRunning(nonRunningPods) { + t.Errorf("%s: allPodsRunning should return false for pods in CreateContainerConfigError", testName) + } + + // test allPodsRunning with running pods returns true + runningPods := []v1.Pod{ + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Running: &v1.ContainerStateRunning{}, + }, + }, + }, + }, + }, + } + + if !cluster.allPodsRunning(runningPods) { + t.Errorf("%s: allPodsRunning should return true for running pods", testName) + } + + // test mixed: 3-node cluster with 1 broken replica + mixedPods := []v1.Pod{ + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + {State: v1.ContainerState{Running: &v1.ContainerStateRunning{}}}, + }, + }, + }, + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + {State: v1.ContainerState{Running: &v1.ContainerStateRunning{}}}, + }, + }, + }, + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Waiting: &v1.ContainerStateWaiting{ + Reason: "CreateContainerConfigError", + }, + }, + }, + }, + }, + }, + } + + if cluster.allPodsRunning(mixedPods) { + t.Errorf("%s: allPodsRunning should return false when one pod is in CreateContainerConfigError", testName) + } +} From d47b666870dc4a3bf72ab9d609fefa7de4dabeaf Mon Sep 17 00:00:00 2001 From: Annie Li Date: Fri, 6 Mar 2026 13:16:44 -0800 Subject: [PATCH 2/4] remove TestSyncStatefulSetNonRunningPodsDoNotBlockRecreatio --- pkg/cluster/pod_test.go | 117 +--------------------------------- pkg/cluster/sync_test.go | 133 --------------------------------------- 2 files changed, 1 insertion(+), 249 deletions(-) diff --git a/pkg/cluster/pod_test.go b/pkg/cluster/pod_test.go index 90a182472..8df5ad305 100644 --- a/pkg/cluster/pod_test.go +++ b/pkg/cluster/pod_test.go @@ -1,118 +1,3 @@ -package cluster - -import ( - "bytes" - "fmt" - "io" - "net/http" - "testing" - "time" - - "github.com/golang/mock/gomock" - "github.com/zalando/postgres-operator/mocks" - acidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1" - "github.com/zalando/postgres-operator/pkg/spec" - "github.com/zalando/postgres-operator/pkg/util/config" - "github.com/zalando/postgres-operator/pkg/util/k8sutil" - "github.com/zalando/postgres-operator/pkg/util/patroni" -) - -func TestGetSwitchoverCandidate(t *testing.T) { - testName := "test getting right switchover candidate" - namespace := "default" - - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - var cluster = New( - Config{ - OpConfig: config.Config{ - PatroniAPICheckInterval: time.Duration(1), - PatroniAPICheckTimeout: time.Duration(5), - }, - }, k8sutil.KubernetesClient{}, acidv1.Postgresql{}, logger, eventRecorder) - - // simulate different member scenarios - tests := []struct { - subtest string - clusterJson string - syncModeEnabled bool - expectedCandidate spec.NamespacedName - expectedError error - }{ - { - subtest: "choose sync_standby over replica", - clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 1}, {"name": "acid-test-cluster-1", "role": "sync_standby", "state": "streaming", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 1, "lag": 0}, {"name": "acid-test-cluster-2", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.3:8008/patroni", "host": "192.168.100.3", "port": 5432, "timeline": 1, "lag": 0}]}`, - syncModeEnabled: true, - expectedCandidate: spec.NamespacedName{Namespace: namespace, Name: "acid-test-cluster-1"}, - expectedError: nil, - }, - { - subtest: "no running sync_standby available", - clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 1}, {"name": "acid-test-cluster-1", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 1, "lag": 0}]}`, - syncModeEnabled: true, - expectedCandidate: spec.NamespacedName{}, - expectedError: fmt.Errorf("failed to get Patroni cluster members: unexpected end of JSON input"), - }, - { - subtest: "choose replica with lowest lag", - clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 1}, {"name": "acid-test-cluster-1", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 1, "lag": 5}, {"name": "acid-test-cluster-2", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.3:8008/patroni", "host": "192.168.100.3", "port": 5432, "timeline": 1, "lag": 2}]}`, - syncModeEnabled: false, - expectedCandidate: spec.NamespacedName{Namespace: namespace, Name: "acid-test-cluster-2"}, - expectedError: nil, - }, - { - subtest: "choose first replica when lag is equal everywhere", - clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 1}, {"name": "acid-test-cluster-1", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 1, "lag": 5}, {"name": "acid-test-cluster-2", "role": "replica", "state": "running", "api_url": "http://192.168.100.3:8008/patroni", "host": "192.168.100.3", "port": 5432, "timeline": 1, "lag": 5}]}`, - syncModeEnabled: false, - expectedCandidate: spec.NamespacedName{Namespace: namespace, Name: "acid-test-cluster-1"}, - expectedError: nil, - }, - { - subtest: "no running replica available", - clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 2}, {"name": "acid-test-cluster-1", "role": "replica", "state": "starting", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 2}]}`, - syncModeEnabled: false, - expectedCandidate: spec.NamespacedName{}, - expectedError: fmt.Errorf("failed to get Patroni cluster members: unexpected end of JSON input"), - }, - { - subtest: "replicas with different status", - clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 1}, {"name": "acid-test-cluster-1", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 1, "lag": 5}, {"name": "acid-test-cluster-2", "role": "replica", "state": "in archive recovery", "api_url": "http://192.168.100.3:8008/patroni", "host": "192.168.100.3", "port": 5432, "timeline": 1, "lag": 2}]}`, - syncModeEnabled: false, - expectedCandidate: spec.NamespacedName{Namespace: namespace, Name: "acid-test-cluster-2"}, - expectedError: nil, - }, - } - - for _, tt := range tests { - // mocking cluster members - r := io.NopCloser(bytes.NewReader([]byte(tt.clusterJson))) - - response := http.Response{ - StatusCode: 200, - Body: r, - } - - mockClient := mocks.NewMockHTTPClient(ctrl) - mockClient.EXPECT().Get(gomock.Any()).Return(&response, nil).AnyTimes() - - p := patroni.New(patroniLogger, mockClient) - cluster.patroni = p - mockMasterPod := newMockPod("192.168.100.1") - mockMasterPod.Namespace = namespace - cluster.Spec.Patroni.SynchronousMode = tt.syncModeEnabled - - candidate, err := cluster.getSwitchoverCandidate(mockMasterPod) - if err != nil && err.Error() != tt.expectedError.Error() { - t.Errorf("%s - %s: unexpected error, %v", testName, tt.subtest, err) - } - - if candidate != tt.expectedCandidate { - t.Errorf("%s - %s: unexpect switchover candidate, got %s, expected %s", testName, tt.subtest, candidate, tt.expectedCandidate) - } - } -} - func TestPodIsNotRunning(t *testing.T) { tests := []struct { subtest string @@ -200,7 +85,7 @@ func TestPodIsNotRunning(t *testing.T) { expected: true, }, { - subtest: "pod running with mixed container states - one healthy one broken", + subtest: "pod running with mixed container states", pod: v1.Pod{ Status: v1.PodStatus{ Phase: v1.PodRunning, diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index 8685dbdb1..f7d46d427 100644 --- a/pkg/cluster/sync_test.go +++ b/pkg/cluster/sync_test.go @@ -1053,136 +1053,3 @@ func TestUpdateSecretNameConflict(t *testing.T) { expectedError := fmt.Sprintf("syncing secret %s failed: error while checking for password rotation: could not update secret because of user name mismatch", "default/prepared-owner-user.acid-test-cluster.credentials") assert.Contains(t, err.Error(), expectedError) } - -func TestSyncStatefulSetNonRunningPodsDoNotBlockRecreation(t *testing.T) { - testName := "test that non-running pods do not block rolling update" - client, _ := newFakeK8sSyncClient() - clusterName := "acid-test-cluster" - namespace := "default" - - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - pg := acidv1.Postgresql{ - ObjectMeta: metav1.ObjectMeta{ - Name: clusterName, - Namespace: namespace, - }, - Spec: acidv1.PostgresSpec{ - NumberOfInstances: 1, - Volume: acidv1.Volume{ - Size: "1Gi", - }, - }, - } - - var cluster = New( - Config{ - OpConfig: config.Config{ - PatroniAPICheckInterval: time.Duration(1), - PatroniAPICheckTimeout: time.Duration(5), - PodManagementPolicy: "ordered_ready", - Resources: config.Resources{ - ClusterLabels: map[string]string{"application": "spilo"}, - ClusterNameLabel: "cluster-name", - DefaultCPURequest: "300m", - DefaultCPULimit: "300m", - DefaultMemoryRequest: "300Mi", - DefaultMemoryLimit: "300Mi", - PodRoleLabel: "spilo-role", - ResourceCheckInterval: time.Duration(3), - ResourceCheckTimeout: time.Duration(10), - }, - }, - }, client, pg, logger, eventRecorder) - - cluster.Name = clusterName - cluster.Namespace = namespace - - // mock Patroni API that always fails (simulates unreachable pod) - mockClient := mocks.NewMockHTTPClient(ctrl) - mockClient.EXPECT().Do(gomock.Any()).Return(nil, fmt.Errorf("connection refused")).AnyTimes() - mockClient.EXPECT().Get(gomock.Any()).Return(nil, fmt.Errorf("connection refused")).AnyTimes() - cluster.patroni = patroni.New(patroniLogger, mockClient) - - // test allPodsRunning with non-running pods returns false - nonRunningPods := []v1.Pod{ - { - Status: v1.PodStatus{ - Phase: v1.PodRunning, - ContainerStatuses: []v1.ContainerStatus{ - { - State: v1.ContainerState{ - Waiting: &v1.ContainerStateWaiting{ - Reason: "CreateContainerConfigError", - Message: `secret "old-secret" not found`, - }, - }, - }, - }, - }, - }, - } - - if cluster.allPodsRunning(nonRunningPods) { - t.Errorf("%s: allPodsRunning should return false for pods in CreateContainerConfigError", testName) - } - - // test allPodsRunning with running pods returns true - runningPods := []v1.Pod{ - { - Status: v1.PodStatus{ - Phase: v1.PodRunning, - ContainerStatuses: []v1.ContainerStatus{ - { - State: v1.ContainerState{ - Running: &v1.ContainerStateRunning{}, - }, - }, - }, - }, - }, - } - - if !cluster.allPodsRunning(runningPods) { - t.Errorf("%s: allPodsRunning should return true for running pods", testName) - } - - // test mixed: 3-node cluster with 1 broken replica - mixedPods := []v1.Pod{ - { - Status: v1.PodStatus{ - Phase: v1.PodRunning, - ContainerStatuses: []v1.ContainerStatus{ - {State: v1.ContainerState{Running: &v1.ContainerStateRunning{}}}, - }, - }, - }, - { - Status: v1.PodStatus{ - Phase: v1.PodRunning, - ContainerStatuses: []v1.ContainerStatus{ - {State: v1.ContainerState{Running: &v1.ContainerStateRunning{}}}, - }, - }, - }, - { - Status: v1.PodStatus{ - Phase: v1.PodRunning, - ContainerStatuses: []v1.ContainerStatus{ - { - State: v1.ContainerState{ - Waiting: &v1.ContainerStateWaiting{ - Reason: "CreateContainerConfigError", - }, - }, - }, - }, - }, - }, - } - - if cluster.allPodsRunning(mixedPods) { - t.Errorf("%s: allPodsRunning should return false when one pod is in CreateContainerConfigError", testName) - } -} From 27196f40f8755822a85ecf6a3467fa7a8d204468 Mon Sep 17 00:00:00 2001 From: Annie Li Date: Fri, 6 Mar 2026 13:19:50 -0800 Subject: [PATCH 3/4] revert pod_test --- pkg/cluster/pod_test.go | 118 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 117 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/pod_test.go b/pkg/cluster/pod_test.go index 8df5ad305..18bfbc863 100644 --- a/pkg/cluster/pod_test.go +++ b/pkg/cluster/pod_test.go @@ -1,3 +1,119 @@ +package cluster + +import ( + "bytes" + "fmt" + "io" + "net/http" + "testing" + "time" + + "github.com/golang/mock/gomock" + "github.com/zalando/postgres-operator/mocks" + acidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1" + "github.com/zalando/postgres-operator/pkg/spec" + "github.com/zalando/postgres-operator/pkg/util/config" + "github.com/zalando/postgres-operator/pkg/util/k8sutil" + "github.com/zalando/postgres-operator/pkg/util/patroni" + v1 "k8s.io/api/core/v1" +) + +func TestGetSwitchoverCandidate(t *testing.T) { + testName := "test getting right switchover candidate" + namespace := "default" + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + var cluster = New( + Config{ + OpConfig: config.Config{ + PatroniAPICheckInterval: time.Duration(1), + PatroniAPICheckTimeout: time.Duration(5), + }, + }, k8sutil.KubernetesClient{}, acidv1.Postgresql{}, logger, eventRecorder) + + // simulate different member scenarios + tests := []struct { + subtest string + clusterJson string + syncModeEnabled bool + expectedCandidate spec.NamespacedName + expectedError error + }{ + { + subtest: "choose sync_standby over replica", + clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 1}, {"name": "acid-test-cluster-1", "role": "sync_standby", "state": "streaming", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 1, "lag": 0}, {"name": "acid-test-cluster-2", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.3:8008/patroni", "host": "192.168.100.3", "port": 5432, "timeline": 1, "lag": 0}]}`, + syncModeEnabled: true, + expectedCandidate: spec.NamespacedName{Namespace: namespace, Name: "acid-test-cluster-1"}, + expectedError: nil, + }, + { + subtest: "no running sync_standby available", + clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 1}, {"name": "acid-test-cluster-1", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 1, "lag": 0}]}`, + syncModeEnabled: true, + expectedCandidate: spec.NamespacedName{}, + expectedError: fmt.Errorf("failed to get Patroni cluster members: unexpected end of JSON input"), + }, + { + subtest: "choose replica with lowest lag", + clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 1}, {"name": "acid-test-cluster-1", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 1, "lag": 5}, {"name": "acid-test-cluster-2", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.3:8008/patroni", "host": "192.168.100.3", "port": 5432, "timeline": 1, "lag": 2}]}`, + syncModeEnabled: false, + expectedCandidate: spec.NamespacedName{Namespace: namespace, Name: "acid-test-cluster-2"}, + expectedError: nil, + }, + { + subtest: "choose first replica when lag is equal everywhere", + clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 1}, {"name": "acid-test-cluster-1", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 1, "lag": 5}, {"name": "acid-test-cluster-2", "role": "replica", "state": "running", "api_url": "http://192.168.100.3:8008/patroni", "host": "192.168.100.3", "port": 5432, "timeline": 1, "lag": 5}]}`, + syncModeEnabled: false, + expectedCandidate: spec.NamespacedName{Namespace: namespace, Name: "acid-test-cluster-1"}, + expectedError: nil, + }, + { + subtest: "no running replica available", + clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 2}, {"name": "acid-test-cluster-1", "role": "replica", "state": "starting", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 2}]}`, + syncModeEnabled: false, + expectedCandidate: spec.NamespacedName{}, + expectedError: fmt.Errorf("failed to get Patroni cluster members: unexpected end of JSON input"), + }, + { + subtest: "replicas with different status", + clusterJson: `{"members": [{"name": "acid-test-cluster-0", "role": "leader", "state": "running", "api_url": "http://192.168.100.1:8008/patroni", "host": "192.168.100.1", "port": 5432, "timeline": 1}, {"name": "acid-test-cluster-1", "role": "replica", "state": "streaming", "api_url": "http://192.168.100.2:8008/patroni", "host": "192.168.100.2", "port": 5432, "timeline": 1, "lag": 5}, {"name": "acid-test-cluster-2", "role": "replica", "state": "in archive recovery", "api_url": "http://192.168.100.3:8008/patroni", "host": "192.168.100.3", "port": 5432, "timeline": 1, "lag": 2}]}`, + syncModeEnabled: false, + expectedCandidate: spec.NamespacedName{Namespace: namespace, Name: "acid-test-cluster-2"}, + expectedError: nil, + }, + } + + for _, tt := range tests { + // mocking cluster members + r := io.NopCloser(bytes.NewReader([]byte(tt.clusterJson))) + + response := http.Response{ + StatusCode: 200, + Body: r, + } + + mockClient := mocks.NewMockHTTPClient(ctrl) + mockClient.EXPECT().Get(gomock.Any()).Return(&response, nil).AnyTimes() + + p := patroni.New(patroniLogger, mockClient) + cluster.patroni = p + mockMasterPod := newMockPod("192.168.100.1") + mockMasterPod.Namespace = namespace + cluster.Spec.Patroni.SynchronousMode = tt.syncModeEnabled + + candidate, err := cluster.getSwitchoverCandidate(mockMasterPod) + if err != nil && err.Error() != tt.expectedError.Error() { + t.Errorf("%s - %s: unexpected error, %v", testName, tt.subtest, err) + } + + if candidate != tt.expectedCandidate { + t.Errorf("%s - %s: unexpect switchover candidate, got %s, expected %s", testName, tt.subtest, candidate, tt.expectedCandidate) + } + } +} + func TestPodIsNotRunning(t *testing.T) { tests := []struct { subtest string @@ -85,7 +201,7 @@ func TestPodIsNotRunning(t *testing.T) { expected: true, }, { - subtest: "pod running with mixed container states", + subtest: "pod running with mixed container states - one healthy one broken", pod: v1.Pod{ Status: v1.PodStatus{ Phase: v1.PodRunning, From c1c8760a3dfd38fb0fb6d76194f0d220c9714f9a Mon Sep 17 00:00:00 2001 From: Annie Li Date: Fri, 6 Mar 2026 13:43:18 -0800 Subject: [PATCH 4/4] pod without status --- pkg/cluster/pod.go | 8 +++++++- pkg/cluster/pod_test.go | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/pod.go b/pkg/cluster/pod.go index c99c1c322..6658ba414 100644 --- a/pkg/cluster/pod.go +++ b/pkg/cluster/pod.go @@ -376,9 +376,15 @@ func (c *Cluster) getPatroniMemberData(pod *v1.Pod) (patroni.MemberData, error) return memberData, nil } -// podIsNotRunning returns true if a pod is not in a healthy running state, +// podIsNotRunning returns true if a pod is known to be in a non-running state, // e.g. stuck in CreateContainerConfigError, CrashLoopBackOff, ImagePullBackOff, etc. +// Pods with no status information are not considered non-running, as they may +// simply not have reported status yet. func podIsNotRunning(pod *v1.Pod) bool { + if pod.Status.Phase == "" { + // No status reported yet — don't treat as non-running + return false + } if pod.Status.Phase != v1.PodRunning { return true } diff --git a/pkg/cluster/pod_test.go b/pkg/cluster/pod_test.go index 18bfbc863..6ab3f9207 100644 --- a/pkg/cluster/pod_test.go +++ b/pkg/cluster/pod_test.go @@ -120,6 +120,13 @@ func TestPodIsNotRunning(t *testing.T) { pod v1.Pod expected bool }{ + { + subtest: "pod with no status reported yet", + pod: v1.Pod{ + Status: v1.PodStatus{}, + }, + expected: false, + }, { subtest: "pod running with all containers ready", pod: v1.Pod{ @@ -382,6 +389,18 @@ func TestAllPodsRunning(t *testing.T) { pods: []v1.Pod{}, expected: true, }, + { + subtest: "pods with no status reported yet", + pods: []v1.Pod{ + { + Status: v1.PodStatus{}, + }, + { + Status: v1.PodStatus{}, + }, + }, + expected: true, + }, } for _, tt := range tests {