Configure Patroni Logs to be stored in a file

tjmoore4 · tjmoore4 · commit ededc3b26f3a · 2024-12-17T15:02:21.000-05:00
This commit allows for the configuration of the Postgres instance Patroni logs to go to a file on the 'pgdata' volume instead of to stdout. This file is located at '/pgdata/patroni/log/patroni.log'. Both the log size limit and log level are configurable; only the size limit setting is required. If not set, the default behavior of sending all logs to stdout is maintained. Changes to this configuration require a reload to take effect. - https://patroni.readthedocs.io/en/latest/yaml_configuration.html#log Issue: PGO-1701
diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml
@@ -11630,6 +11630,35 @@ spec:
                     format: int32
                     minimum: 3
                     type: integer
+                  logging:
+                    description: Patroni log configuration settings.
+                    properties:
+                      level:
+                        default: INFO
+                        description: |-
+                          The Patroni log level.
+                          https://docs.python.org/3.6/library/logging.html#levels
+                        enum:
+                        - CRITICAL
+                        - ERROR
+                        - WARNING
+                        - INFO
+                        - DEBUG
+                        - NOTSET
+                        type: string
+                      storageLimit:
+                        anyOf:
+                        - type: integer
+                        - type: string
+                        description: |-
+                          Limits the total amount of space taken by Patroni Log files.
+                          Minimum value is 25MB.
+                          https://pkg.go.dev/k8s.io/apimachinery/pkg/api/resource#Quantity
+                        pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
+                        x-kubernetes-int-or-string: true
+                    required:
+                    - storageLimit
+                    type: object
                   port:
                     default: 8008
                     description: |-
diff --git a/internal/controller/postgrescluster/cluster.go b/internal/controller/postgrescluster/cluster.go
@@ -44,7 +44,7 @@ func (r *Reconciler) reconcileClusterConfigMap(
 
 	if err == nil {
 		err = patroni.ClusterConfigMap(ctx, cluster, pgHBAs, pgParameters,
-			clusterConfigMap)
+			clusterConfigMap, r.patroniLogSize(cluster))
 	}
 	if err == nil {
 		err = errors.WithStack(r.apply(ctx, clusterConfigMap))
@@ -53,6 +53,31 @@ func (r *Reconciler) reconcileClusterConfigMap(
 	return clusterConfigMap, err
 }
 
+// patroniLogSize attempts to parse the defined log file storage limit, if configured.
+// If a value is set, this enables volume based log storage and triggers the
+// relevant Patroni configuration. If the value given is less than 25M, the log
+// file size storage limit defaults to 25M and an event is triggered.
+func (r *Reconciler) patroniLogSize(cluster *v1beta1.PostgresCluster) int64 {
+
+	if cluster.Spec.Patroni != nil {
+		if cluster.Spec.Patroni.Logging != nil {
+			if cluster.Spec.Patroni.Logging.StorageLimit != nil {
+
+				sizeInBytes := cluster.Spec.Patroni.Logging.StorageLimit.Value()
+
+				if sizeInBytes < 25000000 {
+					r.Recorder.Eventf(cluster, corev1.EventTypeNormal, "PatroniLogStorageLimitTooSmall",
+						"Configured Patroni log storage limit is too small. 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
@@ -13,6 +13,7 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/client-go/tools/record"
@@ -23,6 +24,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 +785,76 @@ postgres-operator.crunchydata.com/role: replica
 		`))
 	})
 }
+
+func TestPatroniLogSize(t *testing.T) {
+	ctx := context.Background()
+
+	oneHundredMeg, err := resource.ParseQuantity("100M")
+	assert.NilError(t, err)
+
+	tooSmall, err := resource.ParseQuantity("1k")
+	assert.NilError(t, err)
+
+	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{
+			Logging: &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{
+			Logging: &v1beta1.PatroniLogConfig{
+				StorageLimit: &oneHundredMeg,
+			}}
+
+		size := reconciler.patroniLogSize(ctx, &cluster)
+
+		assert.Equal(t, size, int64(100000000))
+		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{
+			Logging: &v1beta1.PatroniLogConfig{
+				StorageLimit: &tooSmall,
+			}}
+
+		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, "PatroniLogStorageLimitTooSmall")
+		assert.Equal(t, recorder.Events[0].Note, "Configured Patroni log storage limit is too small. File size will default to 25M.")
+	})
+}
diff --git a/internal/naming/names.go b/internal/naming/names.go
@@ -131,6 +131,10 @@ const (
 )
 
 const (
+	// PatroniPGDataLogPath is the Patroni default log path configuration used by the
+	// PostgreSQL instance.
+	PatroniPGDataLogPath = "/pgdata/patroni/log"
+
 	// PGBackRestRepoContainerName is the name assigned to the container used to run pgBackRest
 	PGBackRestRepoContainerName = "pgbackrest"
 
diff --git a/internal/patroni/config.go b/internal/patroni/config.go
@@ -43,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, patroniLogStorageLimit int64,
 ) (string, error) {
 	root := map[string]any{
 		// The cluster identifier. This value cannot change during the cluster's
@@ -152,6 +152,29 @@ func clusterYAML(
 		},
 	}
 
+	// if a Patroni log file size is configured, configure volume file storage
+	if patroniLogStorageLimit != 0 {
+
+		// Configure the Patroni log settings
+		// - https://patroni.readthedocs.io/en/latest/yaml_configuration.html#log
+		root["log"] = map[string]any{
+
+			"dir":  naming.PatroniPGDataLogPath,
+			"type": "json",
+
+			// defaults to "INFO"
+			"level": cluster.Spec.Patroni.Logging.Level,
+
+			// There will only be two log files. Cannot set to 1 or the logs won't rotate.
+			// - https://github.com/python/cpython/blob/3.11/Lib/logging/handlers.py#L134
+			"file_num": 1,
+
+			// Since there are two log files, ensure the total space used is under
+			// the configured limit.
+			"file_size": patroniLogStorageLimit / 2,
+		}
+	}
+
 	if !ClusterBootstrapped(cluster) {
 		// Patroni has not yet bootstrapped. Populate the "bootstrap.dcs" field to
 		// facilitate it. When Patroni is already bootstrapped, this field is ignored.
diff --git a/internal/patroni/config_test.go b/internal/patroni/config_test.go
@@ -13,6 +13,7 @@ import (
 
 	"gotest.tools/v3/assert"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/yaml"
 
@@ -32,7 +33,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.
@@ -90,7 +91,7 @@ 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.
@@ -136,6 +137,79 @@ restapi:
   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, err := resource.ParseQuantity("1k")
+		assert.NilError(t, err)
+
+		logLevel := "DEBUG"
+		cluster.Spec.Patroni.Logging = &v1beta1.PatroniLogConfig{
+			StorageLimit: &fileSize,
+			Level:        &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.
+# 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
+log:
+  dir: /pgdata/patroni/log
+  file_num: 1
+  file_size: 500
+  level: DEBUG
+  type: json
+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")
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,
+	patroniLogStorageLimit int64,
 ) error {
 	var err error
 
 	initialize.Map(&outClusterConfigMap.Data)
 
 	outClusterConfigMap.Data[configMapFileKey], err = clusterYAML(inCluster, inHBAs,
-		inParameters)
+		inParameters, patroniLogStorageLimit)
 
 	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)
 }
 
diff --git a/internal/postgres/config.go b/internal/postgres/config.go
@@ -291,9 +291,9 @@ chmod +x /tmp/pg_rewind_tde.sh
 `
 	}
 
-	args := []string{version, walDir, naming.PGBackRestPGDataLogPath}
+	args := []string{version, walDir, naming.PGBackRestPGDataLogPath, naming.PatroniPGDataLogPath}
 	script := strings.Join([]string{
-		`declare -r expected_major_version="$1" pgwal_directory="$2" pgbrLog_directory="$3"`,
+		`declare -r expected_major_version="$1" pgwal_directory="$2" pgbrLog_directory="$3" patroniLog_directory="$4"`,
 
 		// Function to print the permissions of a file or directory and its parents.
 		bashPermissions,
@@ -369,6 +369,11 @@ chmod +x /tmp/pg_rewind_tde.sh
 		`install --directory --mode=0775 "${pgbrLog_directory}" ||`,
 		`halt "$(permissions "${pgbrLog_directory}" ||:)"`,
 
+		// Create the Patroni log directory.
+		`results 'Patroni log directory' "${patroniLog_directory}"`,
+		`install --directory --mode=0775 "${patroniLog_directory}" ||`,
+		`halt "$(permissions "${patroniLog_directory}" ||:)"`,
+
 		// Copy replication client certificate files
 		// from the /pgconf/tls/replication directory to the /tmp/replication directory in order
 		// to set proper file permissions. This is required because the group permission settings
diff --git a/internal/postgres/reconcile_test.go b/internal/postgres/reconcile_test.go
diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/patroni_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/patroni_types.go
diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go

Original file line number	Diff line number	Diff line change
`@@ -131,6 +131,10 @@ const (`
`131`	`131`	`)`
`132`	`132`
`133`	`133`	`const (`
	`134`	`+ // PatroniPGDataLogPath is the Patroni default log path configuration used by the`
	`135`	`+ // PostgreSQL instance.`
	`136`	`+ PatroniPGDataLogPath = "/pgdata/patroni/log"`
	`137`	`+`
`134`	`138`	`// PGBackRestRepoContainerName is the name assigned to the container used to run pgBackRest`
`135`	`139`	`PGBackRestRepoContainerName = "pgbackrest"`
`136`	`140`