Retry moveMasterPodsOffNode on failure instead of aborting (#3179)

attemptToMoveMasterPodsOffNode's transient errors (e.g. no synced
standby available yet) were treated as terminal, so the retry loop
gave up after a single ~4-5s attempt instead of retrying over
master_pod_move_timeout. This left masters stuck on draining nodes
indefinitely once a PodDisruptionBudget started rejecting evictions.

Log the failure and return (false, nil) so the retry loop keeps
polling every minute until the timeout expires.

Fixes #3176

docs/reference/operator_parameters.md

@@ -556,9 +556,10 @@ configuration they are grouped under the `kubernetes` key.
 * **master_pod_move_timeout**
   The period of time to wait for the success of migration of master pods from
   an unschedulable node. The migration includes Patroni switchovers to
-  respective replicas on healthy nodes. The situation where master pods still
-  exist on the old node after this timeout expires has to be fixed manually.
-  The default is 20 minutes.
+  respective replicas on healthy nodes. A failed migration attempt is retried
+  every minute until this timeout expires. The situation where master pods
+  still exist on the old node after this timeout expires has to be fixed
+  manually. The default is 20 minutes.
 
 * **enable_pod_antiaffinity**
   toggles [pod anti affinity](https://kubernetes.io/docs/concepts/configuration/assign-pod-node/)

pkg/controller/node.go

@@ -156,7 +156,8 @@ func (c *Controller) moveMasterPodsOffNode(node *v1.Node) {
 		func() (bool, error) {
 			err := c.attemptToMoveMasterPodsOffNode(node)
 			if err != nil {
-				return false, err
+				c.logger.Warningf("attempt to move master pods off node %q failed, will retry: %v", node.Name, err)
+				return false, nil
 			}
 			return true, nil
 		},

pkg/controller/node_test.go

@@ -1,11 +1,18 @@
 package controller
 
 import (
+	"fmt"
+	"strings"
 	"testing"
+	"time"
 
+	logrustest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/zalando/postgres-operator/v2/pkg/spec"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/fake"
+	k8stesting "k8s.io/client-go/testing"
 )
 
 const (
@@ -93,3 +100,32 @@ func TestNodeIsReady(t *testing.T) {
 		}
 	}
 }
+
+// TestMoveMasterPodsOffNodeRetriesOnError ensures a failed attempt to move
+// master pods off a node is retried rather than aborting the whole retry
+// loop on the first error.
+func TestMoveMasterPodsOffNodeRetriesOnError(t *testing.T) {
+	clientSet := fake.NewSimpleClientset()
+	clientSet.PrependReactor("list", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) {
+		return true, nil, fmt.Errorf("could not list pods")
+	})
+
+	controller := newNodeTestController()
+	controller.KubeClient.PodsGetter = clientSet.CoreV1()
+	// timeout == the retry interval hardcoded in moveMasterPodsOffNode, so
+	// the single retry attempt resolves synchronously without a real sleep.
+	controller.opConfig.MasterPodMoveTimeout = &metav1.Duration{Duration: 1 * time.Minute}
+
+	logger, hook := logrustest.NewNullLogger()
+	controller.logger = logger.WithField("pkg", "controller")
+
+	controller.moveMasterPodsOffNode(makeNode(map[string]string{}, false))
+
+	lastEntry := hook.LastEntry()
+	if lastEntry == nil {
+		t.Fatal("expected moveMasterPodsOffNode to log a warning")
+	}
+	if !strings.Contains(lastEntry.Message, "still failing after") {
+		t.Errorf("expected the retry loop to run out of attempts instead of aborting on the first error, got log message: %q", lastEntry.Message)
+	}
+}