refactor, add tests

tjmoore4 · tjmoore4 · commit 46ae3da6186e · 2024-12-12T18:12:15.000-05:00
diff --git a/internal/controller/postgrescluster/cluster.go b/internal/controller/postgrescluster/cluster.go
@@ -12,6 +12,7 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 
@@ -44,7 +45,7 @@ func (r *Reconciler) reconcileClusterConfigMap(
 
 	if err == nil {
 		err = patroni.ClusterConfigMap(ctx, cluster, pgHBAs, pgParameters,
-			clusterConfigMap)
+			clusterConfigMap, r.patroniLogSize(ctx, cluster))
 	}
 	if err == nil {
 		err = errors.WithStack(r.apply(ctx, clusterConfigMap))
@@ -53,6 +54,32 @@ func (r *Reconciler) reconcileClusterConfigMap(
 	return clusterConfigMap, err
 }
 
+// patroniLogSize attemps to parse the defined log file size, if configured.
+// If a value is set, this enables volume based log storage and triggers the
+// relevant Patroni configuration. If the value given cannot be parsed, the log
+// file size defaults to 25M and a warning event is triggered.
+func (r *Reconciler) patroniLogSize(ctx context.Context, cluster *v1beta1.PostgresCluster) int64 {
+
+	if cluster.Spec.Patroni != nil {
+		if cluster.Spec.Patroni.Log != nil {
+			if cluster.Spec.Patroni.Log.FileSize != nil {
+
+				fileSize, err := resource.ParseQuantity(*cluster.Spec.Patroni.Log.FileSize)
+				sizeInBytes := fileSize.Value()
+
+				if err != nil {
+					r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "InvalidPatroniLogFileSize",
+						"Error when parsing spec.patroni.log.fileSize. File size will default to 25M.")
+
+					sizeInBytes = 25000000
+				}
+				return sizeInBytes
+			}
+		}
+	}
+	return 0
+}
+
 // +kubebuilder:rbac:groups="",resources="services",verbs={create,patch}
 
 // reconcileClusterPodService writes the Service that can provide stable DNS
diff --git a/internal/controller/postgrescluster/cluster_test.go b/internal/controller/postgrescluster/cluster_test.go
@@ -23,6 +23,7 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/internal/testing/cmp"
+	"github.com/crunchydata/postgres-operator/internal/testing/events"
 	"github.com/crunchydata/postgres-operator/internal/testing/require"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
@@ -783,3 +784,73 @@ postgres-operator.crunchydata.com/role: replica
 		`))
 	})
 }
+
+func TestPatroniLogSize(t *testing.T) {
+	ctx := context.Background()
+
+	oneMeg := "1M"
+	badSize := "notASize"
+
+	cluster := v1beta1.PostgresCluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "sometest",
+			Namespace: "test-namespace",
+		},
+		Spec: v1beta1.PostgresClusterSpec{}}
+
+	t.Run("Default", func(t *testing.T) {
+		recorder := events.NewRecorder(t, runtime.Scheme)
+		reconciler := &Reconciler{Recorder: recorder}
+
+		size := reconciler.patroniLogSize(ctx, &cluster)
+
+		assert.Equal(t, size, int64(0))
+		assert.Equal(t, len(recorder.Events), 0)
+	})
+
+	t.Run("NoSize", func(t *testing.T) {
+		recorder := events.NewRecorder(t, runtime.Scheme)
+		reconciler := &Reconciler{Recorder: recorder}
+
+		cluster.Spec.Patroni = &v1beta1.PatroniSpec{
+			Log: &v1beta1.PatroniLogConfig{}}
+
+		size := reconciler.patroniLogSize(ctx, &cluster)
+
+		assert.Equal(t, size, int64(0))
+		assert.Equal(t, len(recorder.Events), 0)
+	})
+
+	t.Run("ValidSize", func(t *testing.T) {
+		recorder := events.NewRecorder(t, runtime.Scheme)
+		reconciler := &Reconciler{Recorder: recorder}
+
+		cluster.Spec.Patroni = &v1beta1.PatroniSpec{
+			Log: &v1beta1.PatroniLogConfig{
+				FileSize: &oneMeg,
+			}}
+
+		size := reconciler.patroniLogSize(ctx, &cluster)
+
+		assert.Equal(t, size, int64(1000000))
+		assert.Equal(t, len(recorder.Events), 0)
+	})
+
+	t.Run("BadSize", func(t *testing.T) {
+		recorder := events.NewRecorder(t, runtime.Scheme)
+		reconciler := &Reconciler{Recorder: recorder}
+
+		cluster.Spec.Patroni = &v1beta1.PatroniSpec{
+			Log: &v1beta1.PatroniLogConfig{
+				FileSize: &badSize,
+			}}
+
+		size := reconciler.patroniLogSize(ctx, &cluster)
+
+		assert.Equal(t, size, int64(25000000))
+		assert.Equal(t, len(recorder.Events), 1)
+		assert.Equal(t, recorder.Events[0].Regarding.Name, cluster.Name)
+		assert.Equal(t, recorder.Events[0].Reason, "InvalidPatroniLogFileSize")
+		assert.Equal(t, recorder.Events[0].Note, "Error when parsing spec.patroni.log.fileSize. File size will default to 25M.")
+	})
+}
diff --git a/internal/patroni/config.go b/internal/patroni/config.go
@@ -10,7 +10,6 @@ import (
 	"strings"
 
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/resource"
 	"sigs.k8s.io/yaml"
 
 	"github.com/crunchydata/postgres-operator/internal/config"
@@ -44,7 +43,7 @@ func quoteShellWord(s string) string {
 // clusterYAML returns Patroni settings that apply to the entire cluster.
 func clusterYAML(
 	cluster *v1beta1.PostgresCluster,
-	pgHBAs postgres.HBAs, pgParameters postgres.Parameters,
+	pgHBAs postgres.HBAs, pgParameters postgres.Parameters, patroniLogFileSize int64,
 ) (string, error) {
 	root := map[string]any{
 		// The cluster identifier. This value cannot change during the cluster's
@@ -153,28 +152,17 @@ func clusterYAML(
 		},
 	}
 
-	if cluster.Spec.Patroni != nil {
-		if cluster.Spec.Patroni.Log != nil {
-
-			fileSize, err := resource.ParseQuantity(*cluster.Spec.Patroni.Log.FileSize)
-			sizeInBytes := fileSize.Value()
-
-			// TODO: This isn't how I want to handle this, figure something better out...
-			if err != nil {
-				fmt.Printf("Bad Patroni log file size given, using 25MB instead, error: %v\n", err)
-				sizeInBytes = 25000000 // Patroni's default size
-			}
-
-			root["log"] = map[string]any{
-				// Configure the Patroni log settings
-				// - https://patroni.readthedocs.io/en/latest/yaml_configuration.html#log
-				"dir":       naming.PatroniPGDataLogPath,
-				"file_num":  1,
-				"file_size": sizeInBytes,
-				"type":      "json",
-				"level":     cluster.Spec.Patroni.Log.LogLevel, // defaults to "INFO"
-			}
-
+	// if a Patroni log file size is configured, configure volume file storage
+	if patroniLogFileSize != 0 {
+
+		root["log"] = map[string]any{
+			// Configure the Patroni log settings
+			// - https://patroni.readthedocs.io/en/latest/yaml_configuration.html#log
+			"dir":       naming.PatroniPGDataLogPath,
+			"file_num":  1,
+			"file_size": patroniLogFileSize,
+			"type":      "json",
+			"level":     cluster.Spec.Patroni.Log.LogLevel, // defaults to "INFO"
 		}
 	}
 
diff --git a/internal/patroni/config_test.go b/internal/patroni/config_test.go
@@ -32,7 +32,7 @@ func TestClusterYAML(t *testing.T) {
 		cluster.Namespace = "some-namespace"
 		cluster.Name = "cluster-name"
 
-		data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{})
+		data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, 0)
 		assert.NilError(t, err)
 		assert.Equal(t, data, strings.TrimSpace(`
 # Generated by postgres-operator. DO NOT EDIT.
@@ -58,9 +58,6 @@ kubernetes:
   role_label: postgres-operator.crunchydata.com/role
   scope_label: postgres-operator.crunchydata.com/patroni
   use_endpoints: true
-log:
-  dir: /pgdata/patroni/log
-  file_size: 0
 postgresql:
   authentication:
     replication:
@@ -93,7 +90,72 @@ watchdog:
 		cluster.Name = "cluster-name"
 		cluster.Spec.PostgresVersion = 14
 
-		data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{})
+		data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, 0)
+		assert.NilError(t, err)
+		assert.Equal(t, data, strings.TrimSpace(`
+# Generated by postgres-operator. DO NOT EDIT.
+# Your changes will not be saved.
+bootstrap:
+  dcs:
+    loop_wait: 10
+    postgresql:
+      parameters: {}
+      pg_hba: []
+      use_pg_rewind: true
+      use_slots: false
+    ttl: 30
+ctl:
+  cacert: /etc/patroni/~postgres-operator/patroni.ca-roots
+  certfile: /etc/patroni/~postgres-operator/patroni.crt+key
+  insecure: false
+  keyfile: null
+kubernetes:
+  labels:
+    postgres-operator.crunchydata.com/cluster: cluster-name
+  namespace: some-namespace
+  role_label: postgres-operator.crunchydata.com/role
+  scope_label: postgres-operator.crunchydata.com/patroni
+  use_endpoints: true
+postgresql:
+  authentication:
+    replication:
+      sslcert: /tmp/replication/tls.crt
+      sslkey: /tmp/replication/tls.key
+      sslmode: verify-ca
+      sslrootcert: /tmp/replication/ca.crt
+      username: _crunchyrepl
+    rewind:
+      sslcert: /tmp/replication/tls.crt
+      sslkey: /tmp/replication/tls.key
+      sslmode: verify-ca
+      sslrootcert: /tmp/replication/ca.crt
+      username: _crunchyrepl
+restapi:
+  cafile: /etc/patroni/~postgres-operator/patroni.ca-roots
+  certfile: /etc/patroni/~postgres-operator/patroni.crt+key
+  keyfile: null
+  verify_client: optional
+scope: cluster-name-ha
+watchdog:
+  mode: "off"
+	`)+"\n")
+	})
+
+	t.Run("PatroniLogSizeConfigured", func(t *testing.T) {
+		cluster := new(v1beta1.PostgresCluster)
+		cluster.Default()
+		cluster.Namespace = "some-namespace"
+		cluster.Name = "cluster-name"
+		cluster.Spec.PostgresVersion = 14
+
+		fileSize := "1k"
+		logLevel := "DEBUG"
+		cluster.Spec.Patroni.Log = &v1beta1.PatroniLogConfig{
+			FileSize: &fileSize,
+			LogLevel: &logLevel,
+		}
+
+		data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.Parameters{}, 1000)
 		assert.NilError(t, err)
 		assert.Equal(t, data, strings.TrimSpace(`
 # Generated by postgres-operator. DO NOT EDIT.
@@ -121,7 +183,10 @@ kubernetes:
   use_endpoints: true
 log:
   dir: /pgdata/patroni/log
-  file_size: 0
+  file_num: 1
+  file_size: 1000
+  level: DEBUG
+  type: json
 postgresql:
   authentication:
     replication:
diff --git a/internal/patroni/reconcile.go b/internal/patroni/reconcile.go
@@ -32,13 +32,14 @@ func ClusterConfigMap(ctx context.Context,
 	inHBAs postgres.HBAs,
 	inParameters postgres.Parameters,
 	outClusterConfigMap *corev1.ConfigMap,
+	patroniLogFileSize int64,
 ) error {
 	var err error
 
 	initialize.Map(&outClusterConfigMap.Data)
 
 	outClusterConfigMap.Data[configMapFileKey], err = clusterYAML(inCluster, inHBAs,
-		inParameters)
+		inParameters, patroniLogFileSize)
 
 	return err
 }
diff --git a/internal/patroni/reconcile_test.go b/internal/patroni/reconcile_test.go
@@ -29,15 +29,15 @@ func TestClusterConfigMap(t *testing.T) {
 
 	cluster.Default()
 	config := new(corev1.ConfigMap)
-	assert.NilError(t, ClusterConfigMap(ctx, cluster, pgHBAs, pgParameters, config))
+	assert.NilError(t, ClusterConfigMap(ctx, cluster, pgHBAs, pgParameters, config, 0))
 
 	// The output of clusterYAML should go into config.
-	data, _ := clusterYAML(cluster, pgHBAs, pgParameters)
+	data, _ := clusterYAML(cluster, pgHBAs, pgParameters, 0)
 	assert.DeepEqual(t, config.Data["patroni.yaml"], data)
 
 	// No change when called again.
 	before := config.DeepCopy()
-	assert.NilError(t, ClusterConfigMap(ctx, cluster, pgHBAs, pgParameters, config))
+	assert.NilError(t, ClusterConfigMap(ctx, cluster, pgHBAs, pgParameters, config, 0))
 	assert.DeepEqual(t, config, before)
 }
 
