Consider global value in sync + tiny refactoring

hughcapet · hughcapet · commit babb7c2289cb · 2022-10-31T09:31:19.000+01:00
diff --git a/charts/postgres-operator/crds/operatorconfigurations.yaml b/charts/postgres-operator/crds/operatorconfigurations.yaml
@@ -636,7 +636,7 @@ spec:
               patroni:
                 type: object
                 properties:
-                  enable_failsafe_mode:
+                  failsafe_mode:
                     type: boolean
                     default: false
           status:
diff --git a/charts/postgres-operator/values.yaml b/charts/postgres-operator/values.yaml
@@ -411,7 +411,7 @@ configConnectionPooler:
 
 configPatroni:
   # enable Patroni DCS failsafe_mode feature
-  enable_failsafe_mode: false
+  failsafe_mode: false
 
 # Zalando's internal CDC stream feature
 enableStreams: false
diff --git a/manifests/operatorconfiguration.crd.yaml b/manifests/operatorconfiguration.crd.yaml
@@ -634,7 +634,7 @@ spec:
               patroni:
                 type: object
                 properties:
-                  enable_failsafe_mode:
+                  failsafe_mode:
                     type: boolean
                     default: false
           status:
diff --git a/manifests/postgresql-operator-default-configuration.yaml b/manifests/postgresql-operator-default-configuration.yaml
@@ -199,4 +199,4 @@ configuration:
     # connection_pooler_schema: "pooler"
     # connection_pooler_user: "pooler"
   patroni:
-    # enable_failsafe_mode: "false"
+    # failsafe_mode: "false"
diff --git a/pkg/apis/acid.zalan.do/v1/crds.go b/pkg/apis/acid.zalan.do/v1/crds.go
@@ -1461,6 +1461,14 @@ var OperatorConfigCRDResourceValidation = apiextv1.CustomResourceValidation{
 							},
 						},
 					},
+					"patroni": {
+						Type: "object",
+						Properties: map[string]apiextv1.JSONSchemaProps{
+							"failsafe_mode": {
+								Type: "boolean",
+							},
+						},
+					},
 					"postgres_pod_resources": {
 						Type: "object",
 						Properties: map[string]apiextv1.JSONSchemaProps{
diff --git a/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go b/pkg/apis/acid.zalan.do/v1/operator_configuration_type.go
@@ -229,7 +229,7 @@ type OperatorLogicalBackupConfiguration struct {
 
 // PatroniConfiguration defines configuration for Patroni
 type PatroniConfiguration struct {
-	EnableFailsafeMode bool `json:"enable_failsafe_mode,omitempty"`
+	FailsafeMode *bool `json:"failsafe_mode,omitempty"`
 }
 
 // OperatorConfigurationData defines the operation config
@@ -264,7 +264,7 @@ type OperatorConfigurationData struct {
 	Scalyr                        ScalyrConfiguration                `json:"scalyr"`
 	LogicalBackup                 OperatorLogicalBackupConfiguration `json:"logical_backup"`
 	ConnectionPooler              ConnectionPoolerConfiguration      `json:"connection_pooler"`
-	Patroni                       PatroniConfiguration               `json:"patroni_configuration"`
+	Patroni                       PatroniConfiguration               `json:"patroni"`
 
 	MinInstances                      int32  `json:"min_instances,omitempty"`
 	MaxInstances                      int32  `json:"max_instances,omitempty"`
diff --git a/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go b/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go
diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go
@@ -59,7 +59,7 @@ type patroniDCS struct {
 	SynchronousNodeCount     uint32                       `json:"synchronous_node_count,omitempty"`
 	PGBootstrapConfiguration map[string]interface{}       `json:"postgresql,omitempty"`
 	Slots                    map[string]map[string]string `json:"slots,omitempty"`
-	FailsafeMode             bool                         `json:"failsafe_mode,omitempty"`
+	FailsafeMode             *bool                        `json:"failsafe_mode,omitempty"`
 }
 
 type pgBootstrap struct {
@@ -380,8 +380,8 @@ PatroniInitDBParams:
 		config.Bootstrap.DCS.SynchronousNodeCount = patroni.SynchronousNodeCount
 	}
 	if patroni.FailsafeMode != nil {
-		config.Bootstrap.DCS.FailsafeMode = *patroni.FailsafeMode
-	} else {
+		config.Bootstrap.DCS.FailsafeMode = patroni.FailsafeMode
+	} else if opConfig.EnablePatroniFailsafeMode != nil {
 		config.Bootstrap.DCS.FailsafeMode = opConfig.EnablePatroniFailsafeMode
 	}
 
diff --git a/pkg/cluster/k8sres_test.go b/pkg/cluster/k8sres_test.go
@@ -118,7 +118,7 @@ func TestGenerateSpiloJSONConfiguration(t *testing.T) {
 				Auth: config.Auth{
 					PamRoleName: "zalandos",
 				},
-				EnablePatroniFailsafeMode: true,
+				EnablePatroniFailsafeMode: util.True(),
 			},
 			result: `{"postgresql":{"bin_dir":"/usr/lib/postgresql/14/bin"},"bootstrap":{"initdb":[{"auth-host":"md5"},{"auth-local":"trust"}],"users":{"zalandos":{"password":"","options":["CREATEDB","NOLOGIN"]}},"dcs":{"failsafe_mode":true}}}`,
 		},
@@ -132,9 +132,9 @@ func TestGenerateSpiloJSONConfiguration(t *testing.T) {
 				Auth: config.Auth{
 					PamRoleName: "zalandos",
 				},
-				EnablePatroniFailsafeMode: true,
+				EnablePatroniFailsafeMode: util.True(),
 			},
-			result: `{"postgresql":{"bin_dir":"/usr/lib/postgresql/14/bin"},"bootstrap":{"initdb":[{"auth-host":"md5"},{"auth-local":"trust"}],"users":{"zalandos":{"password":"","options":["CREATEDB","NOLOGIN"]}},"dcs":{}}}`,
+			result: `{"postgresql":{"bin_dir":"/usr/lib/postgresql/14/bin"},"bootstrap":{"initdb":[{"auth-host":"md5"},{"auth-local":"trust"}],"users":{"zalandos":{"password":"","options":["CREATEDB","NOLOGIN"]}},"dcs":{"failsafe_mode":false}}}`,
 		},
 		{
 			subtest: "Patroni failsafe_mode disabled globally, configured for cluster",
@@ -146,7 +146,7 @@ func TestGenerateSpiloJSONConfiguration(t *testing.T) {
 				Auth: config.Auth{
 					PamRoleName: "zalandos",
 				},
-				EnablePatroniFailsafeMode: false,
+				EnablePatroniFailsafeMode: util.False(),
 			},
 			result: `{"postgresql":{"bin_dir":"/usr/lib/postgresql/14/bin"},"bootstrap":{"initdb":[{"auth-host":"md5"},{"auth-local":"trust"}],"users":{"zalandos":{"password":"","options":["CREATEDB","NOLOGIN"]}},"dcs":{"failsafe_mode":true}}}`,
 		},
diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go
@@ -566,8 +566,21 @@ func (c *Cluster) checkAndSetGlobalPostgreSQLConfiguration(pod *v1.Pod, effectiv
 	if desiredPatroniConfig.TTL > 0 && desiredPatroniConfig.TTL != effectivePatroniConfig.TTL {
 		configToSet["ttl"] = desiredPatroniConfig.TTL
 	}
-	if desiredPatroniConfig.FailsafeMode != effectivePatroniConfig.FailsafeMode {
-		configToSet["failsafe_mode"] = desiredPatroniConfig.FailsafeMode
+
+	var desiredFailsafe *bool
+	if desiredPatroniConfig.FailsafeMode != nil {
+		desiredFailsafe = desiredPatroniConfig.FailsafeMode
+	} else if c.OpConfig.EnablePatroniFailsafeMode != nil {
+		desiredFailsafe = c.OpConfig.EnablePatroniFailsafeMode
+	}
+
+	effectiveFailsafe := effectivePatroniConfig.FailsafeMode
+
+	if desiredFailsafe != nil {
+		c.logger.Info("desiredFailsafe is not nil")
+		if effectiveFailsafe == nil || *desiredFailsafe != *effectiveFailsafe {
+			configToSet["failsafe_mode"] = *desiredFailsafe
+		}
 	}
 
 	// check if specified slots exist in config and if they differ
diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go
@@ -20,6 +20,7 @@ import (
 	acidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1"
 	fakeacidv1 "github.com/zalando/postgres-operator/pkg/generated/clientset/versioned/fake"
 	"github.com/zalando/postgres-operator/pkg/spec"
+	"github.com/zalando/postgres-operator/pkg/util"
 	"github.com/zalando/postgres-operator/pkg/util/config"
 	"github.com/zalando/postgres-operator/pkg/util/k8sutil"
 	"github.com/zalando/postgres-operator/pkg/util/patroni"
@@ -147,20 +148,23 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 
+	defaultPgParameters := map[string]string{
+		"log_min_duration_statement": "200",
+		"max_connections":            "50",
+	}
+	defaultPatroniParameters := acidv1.Patroni{
+		TTL: 20,
+	}
+
 	pg := acidv1.Postgresql{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      clusterName,
 			Namespace: namespace,
 		},
 		Spec: acidv1.PostgresSpec{
-			Patroni: acidv1.Patroni{
-				TTL: 20,
-			},
+			Patroni: defaultPatroniParameters,
 			PostgresqlParam: acidv1.PostgresqlParam{
-				Parameters: map[string]string{
-					"log_min_duration_statement": "200",
-					"max_connections":            "50",
-				},
+				Parameters: defaultPgParameters,
 			},
 			Volume: acidv1.Volume{
 				Size: "1Gi",
@@ -222,9 +226,7 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) {
 		},
 		{
 			subtest: "multiple Postgresql.Parameters differ - restart replica first",
-			patroni: acidv1.Patroni{
-				TTL: 20,
-			},
+			patroni: defaultPatroniParameters,
 			pgParams: map[string]string{
 				"log_min_duration_statement": "500", // desired 200
 				"max_connections":            "100", // desired 50
@@ -233,9 +235,7 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) {
 		},
 		{
 			subtest: "desired max_connections bigger - restart replica first",
-			patroni: acidv1.Patroni{
-				TTL: 20,
-			},
+			patroni: defaultPatroniParameters,
 			pgParams: map[string]string{
 				"log_min_duration_statement": "200",
 				"max_connections":            "30", // desired 50
@@ -244,9 +244,7 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) {
 		},
 		{
 			subtest: "desired max_connections smaller - restart master first",
-			patroni: acidv1.Patroni{
-				TTL: 20,
-			},
+			patroni: defaultPatroniParameters,
 			pgParams: map[string]string{
 				"log_min_duration_statement": "200",
 				"max_connections":            "100", // desired 50
@@ -265,6 +263,109 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) {
 			t.Errorf("%s - %s: wrong master restart strategy, got restart %v, expected restart %v", testName, tt.subtest, requirePrimaryRestart, tt.restartPrimary)
 		}
 	}
+
+	testsFailsafe := []struct {
+		subtest         string
+		operatorVal     *bool
+		effectiveVal    *bool
+		desiredVal      bool
+		shouldBePatched bool
+		restartPrimary  bool
+	}{
+		{
+			subtest:         "Not set in operator config, not set for pg cluster. Set to true in the pg config.",
+			operatorVal:     nil,
+			effectiveVal:    nil,
+			desiredVal:      true,
+			shouldBePatched: true,
+			restartPrimary:  false,
+		},
+		{
+			subtest:         "Not set in operator config, disabled for pg cluster. Set to true in the pg config.",
+			operatorVal:     nil,
+			effectiveVal:    util.False(),
+			desiredVal:      true,
+			shouldBePatched: true,
+			restartPrimary:  false,
+		},
+		{
+			subtest:         "Not set in operator config, not set for pg cluster. Set to false in the pg config.",
+			operatorVal:     nil,
+			effectiveVal:    nil,
+			desiredVal:      false,
+			shouldBePatched: true,
+			restartPrimary:  false,
+		},
+		{
+			subtest:         "Not set in operator config, enabled for pg cluster. Set to false in the pg config.",
+			operatorVal:     nil,
+			effectiveVal:    util.True(),
+			desiredVal:      false,
+			shouldBePatched: true,
+			restartPrimary:  false,
+		},
+		{
+			subtest:         "Enabled in operator config, not set for pg cluster. Set to false in the pg config.",
+			operatorVal:     util.True(),
+			effectiveVal:    nil,
+			desiredVal:      false,
+			shouldBePatched: true,
+			restartPrimary:  false,
+		},
+		{
+			subtest:         "Enabled in operator config, disabled for pg cluster. Set to true in the pg config.",
+			operatorVal:     util.True(),
+			effectiveVal:    util.False(),
+			desiredVal:      true,
+			shouldBePatched: true,
+			restartPrimary:  false,
+		},
+		{
+			subtest:         "Disabled in operator config, not set for pg cluster. Set to true in the pg config.",
+			operatorVal:     util.False(),
+			effectiveVal:    nil,
+			desiredVal:      true,
+			shouldBePatched: true,
+			restartPrimary:  false,
+		},
+		{
+			subtest:         "Disabled in operator config, enabled for pg cluster. Set to false in the pg config.",
+			operatorVal:     util.False(),
+			effectiveVal:    util.True(),
+			desiredVal:      false,
+			shouldBePatched: true,
+			restartPrimary:  false,
+		},
+		{
+			subtest:         "Disabled in operator config, enabled for pg cluster. Set to true in the pg config.",
+			operatorVal:     util.False(),
+			effectiveVal:    util.True(),
+			desiredVal:      true,
+			shouldBePatched: false, // should not require patching
+			restartPrimary:  true,
+		},
+	}
+
+	for _, tt := range testsFailsafe {
+		patroniConf := defaultPatroniParameters
+
+		if tt.operatorVal != nil {
+			cluster.OpConfig.EnablePatroniFailsafeMode = tt.operatorVal
+		}
+		if tt.effectiveVal != nil {
+			patroniConf.FailsafeMode = tt.effectiveVal
+		}
+		cluster.Spec.Patroni.FailsafeMode = &tt.desiredVal
+
+		configPatched, requirePrimaryRestart, err := cluster.checkAndSetGlobalPostgreSQLConfiguration(mockPod, patroniConf, cluster.Spec.Patroni, defaultPgParameters, cluster.Spec.Parameters)
+		assert.NoError(t, err)
+		if configPatched != tt.shouldBePatched {
+			t.Errorf("%s - %s: expected update went wrong", testName, tt.subtest)
+		}
+		if requirePrimaryRestart != tt.restartPrimary {
+			t.Errorf("%s - %s: wrong master restart strategy, got restart %v, expected restart %v", testName, tt.subtest, requirePrimaryRestart, tt.restartPrimary)
+		}
+	}
 }
 
 func TestUpdateSecret(t *testing.T) {
diff --git a/pkg/controller/operator_config.go b/pkg/controller/operator_config.go
@@ -78,7 +78,6 @@ func (c *Controller) importConfigurationFromCRD(fromCRD *acidv1.OperatorConfigur
 	result.SpiloRunAsUser = fromCRD.Kubernetes.SpiloRunAsUser
 	result.SpiloRunAsGroup = fromCRD.Kubernetes.SpiloRunAsGroup
 	result.SpiloFSGroup = fromCRD.Kubernetes.SpiloFSGroup
-	result.EnablePatroniFailsafeMode = fromCRD.Patroni.EnableFailsafeMode
 	result.AdditionalPodCapabilities = fromCRD.Kubernetes.AdditionalPodCapabilities
 	result.ClusterDomain = util.Coalesce(fromCRD.Kubernetes.ClusterDomain, "cluster.local")
 	result.WatchedNamespace = fromCRD.Kubernetes.WatchedNamespace
@@ -217,6 +216,9 @@ func (c *Controller) importConfigurationFromCRD(fromCRD *acidv1.OperatorConfigur
 	result.ScalyrCPULimit = fromCRD.Scalyr.ScalyrCPULimit
 	result.ScalyrMemoryLimit = fromCRD.Scalyr.ScalyrMemoryLimit
 
+	// Patroni config
+	result.EnablePatroniFailsafeMode = util.CoalesceBool(fromCRD.Patroni.FailsafeMode, util.False())
+
 	// Connection pooler. Looks like we can't use defaulting in CRD before 1.17,
 	// so ensure default values here.
 	result.ConnectionPooler.NumberOfInstances = util.CoalesceInt32(
diff --git a/pkg/util/config/config.go b/pkg/util/config/config.go
@@ -234,7 +234,7 @@ type Config struct {
 	TargetMajorVersion                     string            `name:"target_major_version" default:"14"`
 	PatroniAPICheckInterval                time.Duration     `name:"patroni_api_check_interval" default:"1s"`
 	PatroniAPICheckTimeout                 time.Duration     `name:"patroni_api_check_timeout" default:"5s"`
-	EnablePatroniFailsafeMode              bool              `name:"enable_patroni_failsafe_mode" default:"false"`
+	EnablePatroniFailsafeMode              *bool             `name:"enable_patroni_failsafe_mode" default:"false"`
 }
 
 // MustMarshal marshals the config or panics

Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ type patroniDCS struct {`
`59`	`59`	SynchronousNodeCount uint32 `json:"synchronous_node_count,omitempty"`
`60`	`60`	PGBootstrapConfiguration map[string]interface{} `json:"postgresql,omitempty"`
`61`	`61`	Slots map[string]map[string]string `json:"slots,omitempty"`
`62`		- FailsafeMode bool `json:"failsafe_mode,omitempty"`
	`62`	+ FailsafeMode *bool `json:"failsafe_mode,omitempty"`
`63`	`63`	`}`
`64`	`64`
`65`	`65`	`type pgBootstrap struct {`
`@@ -380,8 +380,8 @@ PatroniInitDBParams:`
`380`	`380`	`config.Bootstrap.DCS.SynchronousNodeCount = patroni.SynchronousNodeCount`
`381`	`381`	`}`
`382`	`382`	`if patroni.FailsafeMode != nil {`
`383`		`- config.Bootstrap.DCS.FailsafeMode = *patroni.FailsafeMode`
`384`		`- } else {`
	`383`	`+ config.Bootstrap.DCS.FailsafeMode = patroni.FailsafeMode`
	`384`	`+ } else if opConfig.EnablePatroniFailsafeMode != nil {`
`385`	`385`	`config.Bootstrap.DCS.FailsafeMode = opConfig.EnablePatroniFailsafeMode`
`386`	`386`	`}`
`387`	`387`