Rotate postgres logs according to retentionPeriod in spec.

Refactor logrotate config creation.
Refactor logic around adding collector to postgres instance pod.
Use metav1.Duration in helper functions to avoid error handling.

internal/collector/config.go

@@ -9,8 +9,11 @@ import (
 	_ "embed"
 	"fmt"
 	"math"
+	"strings"
+	"time"
 
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"sigs.k8s.io/yaml"
 
@@ -54,6 +57,13 @@ type Pipeline struct {
 	Receivers  []ComponentID
 }
 
+// LogrotateConfig represents the configurable pieces of a log rotate config
+// that can vary based on the specific component whose logs are being rotated
+type LogrotateConfig struct {
+	LogFiles         []string
+	PostrotateScript string
+}
+
 func (c *Config) ToYAML() (string, error) {
 	const yamlGeneratedWarning = "" +
 		"# Generated by postgres-operator. DO NOT EDIT.\n" +
@@ -114,48 +124,43 @@ func NewConfig(spec *v1beta1.InstrumentationSpec) *Config {
 	return config
 }
 
-// AddLogrotateConfig generates a logrotate configuration and adds it to the
-// provided configmap
-func AddLogrotateConfig(ctx context.Context, spec *v1beta1.InstrumentationSpec,
-	outInstanceConfigMap *corev1.ConfigMap, logFilePath, postrotateScript string,
-) error {
-	var err error
-	var retentionPeriod *v1beta1.Duration
-
+// AddLogrotateConfigs generates a logrotate configuration for each LogrotateConfig
+// provided via the configs parameter and adds them to the provided configmap.
+func AddLogrotateConfigs(ctx context.Context, spec *v1beta1.InstrumentationSpec,
+	outInstanceConfigMap *corev1.ConfigMap, configs []LogrotateConfig,
+) {
 	if outInstanceConfigMap.Data == nil {
 		outInstanceConfigMap.Data = make(map[string]string)
 	}
 
 	// If retentionPeriod is set in the spec, use that value; otherwise, we want
 	// to use a reasonably short duration. Defaulting to 1 day.
+	retentionPeriod := metav1.Duration{Duration: 24 * time.Hour}
 	if spec != nil && spec.Logs != nil && spec.Logs.RetentionPeriod != nil {
-		retentionPeriod = spec.Logs.RetentionPeriod
-	} else {
-		retentionPeriod, err = v1beta1.NewDuration("1d")
-		if err != nil {
-			return err
-		}
+		retentionPeriod = spec.Logs.RetentionPeriod.AsDuration()
 	}
 
-	outInstanceConfigMap.Data["logrotate.conf"] = generateLogrotateConfig(logFilePath,
-		retentionPeriod, postrotateScript)
+	logrotateConfig := ""
+	for _, config := range configs {
+		logrotateConfig += generateLogrotateConfig(config, retentionPeriod)
+	}
 
-	return err
+	outInstanceConfigMap.Data["logrotate.conf"] = logrotateConfig
 }
 
 // generateLogrotateConfig generates a configuration string for logrotate based
 // on the provided full log file path, retention period, and postrotate script
-func generateLogrotateConfig(logFilePath string, retentionPeriod *v1beta1.Duration,
-	postrotateScript string,
+func generateLogrotateConfig(
+	config LogrotateConfig, retentionPeriod metav1.Duration,
 ) string {
 	number, interval := parseDurationForLogrotate(retentionPeriod)
 
 	return fmt.Sprintf(
 		logrotateConfigFormatString,
-		logFilePath,
+		strings.Join(config.LogFiles, " "),
 		number,
 		interval,
-		postrotateScript,
+		config.PostrotateScript,
 	)
 }
 
@@ -164,8 +169,8 @@ func generateLogrotateConfig(logFilePath string, retentionPeriod *v1beta1.Durati
 // If the retentionPeriod is less than 24 hours, the function will return the
 // number of hours and "hourly"; otherwise, we will round up to the nearest day
 // and return the day count and "daily"
-func parseDurationForLogrotate(retentionPeriod *v1beta1.Duration) (int, string) {
-	hours := math.Ceil(retentionPeriod.AsDuration().Hours())
+func parseDurationForLogrotate(retentionPeriod metav1.Duration) (int, string) {
+	hours := math.Ceil(retentionPeriod.Hours())
 	if hours < 24 {
 		return int(hours), "hourly"
 	}

internal/collector/config_test.go

@@ -66,15 +66,16 @@ service:
 
 func TestGenerateLogrotateConfig(t *testing.T) {
 	for _, tt := range []struct {
-		logFilePath      string
-		retentionPeriod  string
-		postrotateScript string
-		result           string
+		config          LogrotateConfig
+		retentionPeriod string
+		result          string
 	}{
 		{
-			logFilePath:      "/this/is/a/file.path",
-			retentionPeriod:  "12h",
-			postrotateScript: "echo 'Hello, World'",
+			config: LogrotateConfig{
+				LogFiles:         []string{"/this/is/a/file.path"},
+				PostrotateScript: "echo 'Hello, World'",
+			},
+			retentionPeriod: "12h",
 			result: `/this/is/a/file.path {
       rotate 12
       missingok
@@ -89,9 +90,11 @@ func TestGenerateLogrotateConfig(t *testing.T) {
 `,
 		},
 		{
-			logFilePath:      "/tmp/test.log",
-			retentionPeriod:  "5 days",
-			postrotateScript: "",
+			config: LogrotateConfig{
+				LogFiles:         []string{"/tmp/test.log"},
+				PostrotateScript: "",
+			},
+			retentionPeriod: "5 days",
 			result: `/tmp/test.log {
       rotate 5
       missingok
@@ -106,10 +109,12 @@ func TestGenerateLogrotateConfig(t *testing.T) {
 `,
 		},
 		{
-			logFilePath:      "/tmp/test.log",
-			retentionPeriod:  "5wk",
-			postrotateScript: "pkill -HUP --exact pgbouncer",
-			result: `/tmp/test.log {
+			config: LogrotateConfig{
+				LogFiles:         []string{"/tmp/test.csv", "/tmp/test.json"},
+				PostrotateScript: "pkill -HUP --exact pgbouncer",
+			},
+			retentionPeriod: "5wk",
+			result: `/tmp/test.csv /tmp/test.json {
       rotate 35
       missingok
       sharedscripts
@@ -126,7 +131,7 @@ func TestGenerateLogrotateConfig(t *testing.T) {
 		t.Run(tt.retentionPeriod, func(t *testing.T) {
 			duration, err := v1beta1.NewDuration(tt.retentionPeriod)
 			assert.NilError(t, err)
-			result := generateLogrotateConfig(tt.logFilePath, duration, tt.postrotateScript)
+			result := generateLogrotateConfig(tt.config, duration.AsDuration())
 			assert.Equal(t, tt.result, result)
 		})
 	}
@@ -192,7 +197,7 @@ func TestParseDurationForLogrotate(t *testing.T) {
 		t.Run(tt.retentionPeriod, func(t *testing.T) {
 			duration, err := v1beta1.NewDuration(tt.retentionPeriod)
 			assert.NilError(t, err)
-			number, interval := parseDurationForLogrotate(duration)
+			number, interval := parseDurationForLogrotate(duration.AsDuration())
 			assert.Equal(t, tt.number, number)
 			assert.Equal(t, tt.interval, interval)
 		})

internal/collector/postgres.go

@@ -9,7 +9,11 @@ import (
 	_ "embed"
 	"encoding/json"
 	"fmt"
+	"math"
 	"slices"
+	"time"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/crunchydata/postgres-operator/internal/feature"
 	"github.com/crunchydata/postgres-operator/internal/naming"
@@ -84,14 +88,22 @@ func EnablePostgresLogging(
 		// PostgreSQL v8.3 adds support for CSV logging, and
 		// PostgreSQL v15 adds support for JSON logging. The latter is preferred
 		// because newlines are escaped as "\n", U+005C + U+006E.
-		if inCluster.Spec.PostgresVersion < 15 {
+		if inCluster != nil && inCluster.Spec.PostgresVersion < 15 {
 			outParameters.Add("log_destination", "csvlog")
 		} else {
 			outParameters.Add("log_destination", "jsonlog")
 		}
 
-		// Keep seven days of logs named for the day of the week;
-		// this has been the default produced by `initdb` for some time now.
+		// If retentionPeriod is set in the spec, use that value; otherwise, we want
+		// to use a reasonably short duration. Defaulting to 1 day.
+		retentionPeriod := metav1.Duration{Duration: 24 * time.Hour}
+		if inCluster != nil && inCluster.Spec.Instrumentation != nil &&
+			inCluster.Spec.Instrumentation.Logs != nil &&
+			inCluster.Spec.Instrumentation.Logs.RetentionPeriod != nil {
+			retentionPeriod = inCluster.Spec.Instrumentation.Logs.RetentionPeriod.AsDuration()
+		}
+		logFilename, logRotationAge := generateLogFilenameAndRotationAge(retentionPeriod)
+
 		// NOTE: The automated portions of log_filename are *entirely* based
 		// on time. There is no spelling that is guaranteed to be unique or
 		// monotonically increasing.
@@ -100,9 +112,9 @@ func EnablePostgresLogging(
 		// probably requires another process that deletes the oldest files.
 		//
 		// The ".log" suffix is replaced by ".json" for JSON log files.
-		outParameters.Add("log_filename", "postgresql-%a.log")
+		outParameters.Add("log_filename", logFilename)
 		outParameters.Add("log_file_mode", "0660")
-		outParameters.Add("log_rotation_age", "1d")
+		outParameters.Add("log_rotation_age", logRotationAge)
 		outParameters.Add("log_rotation_size", "0")
 		outParameters.Add("log_truncate_on_rotation", "on")
 
@@ -272,3 +284,37 @@ func EnablePostgresLogging(
 		}
 	}
 }
+
+// generateLogFilenameAndRotationAge takes a retentionPeriod and returns a
+// log_filename and log_rotation_age to be used to configure postgres logging
+func generateLogFilenameAndRotationAge(
+	retentionPeriod metav1.Duration,
+) (logFilename, logRotationAge string) {
+	// Given how postgres does its log rotation with the truncate feature, we
+	// will always need to make up the total retention period with multiple log
+	// files that hold subunits of the total time (e.g. if the retentionPeriod
+	// is an hour, there will be 60 1-minute long files; if the retentionPeriod
+	// is a day, there will be 24 1-hour long files, etc)
+
+	hours := math.Ceil(retentionPeriod.Hours())
+
+	switch true {
+	case hours <= 1: // One hour's worth of logs in 60 minute long log files
+		logFilename = "postgresql-%M.log"
+		logRotationAge = "1min"
+	case hours <= 24: // One day's worth of logs in 24 hour long log files
+		logFilename = "postgresql-%H.log"
+		logRotationAge = "1h"
+	case hours <= 24*7: // One week's worth of logs in 7 day long log files
+		logFilename = "postgresql-%a.log"
+		logRotationAge = "1d"
+	case hours <= 24*28: // One month's worth of logs in 28-31 day long log files
+		logFilename = "postgresql-%d.log"
+		logRotationAge = "1d"
+	default: // One year's worth of logs in 365 day long log files
+		logFilename = "postgresql-%j.log"
+		logRotationAge = "1d"
+	}
+
+	return
+}

internal/controller/postgrescluster/instance.go

@@ -1200,26 +1200,27 @@ func (r *Reconciler) reconcileInstance(
 			spec, instanceCertificates, instanceConfigMap, &instance.Spec.Template)
 	}
 
+	// If either OpenTelemetry feature is enabled, we want to add the collector config to the pod
 	if err == nil &&
-		(feature.Enabled(ctx, feature.OpenTelemetryLogs) && !feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
+		(feature.Enabled(ctx, feature.OpenTelemetryLogs) || feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
+
+		// If the OpenTelemetryMetrics feature is enabled, we need to get the pgpassword from the
+		// monitoring user secret
+		pgPassword := ""
+		if feature.Enabled(ctx, feature.OpenTelemetryMetrics) {
+			monitoringUserSecret := &corev1.Secret{ObjectMeta: naming.MonitoringUserSecret(cluster)}
+			// Create new err variable to avoid abandoning the rest of the reconcile loop if there
+			// is an error getting the monitoring user secret
+			err := errors.WithStack(
+				r.Client.Get(ctx, client.ObjectKeyFromObject(monitoringUserSecret), monitoringUserSecret))
+			if err == nil {
+				pgPassword = string(monitoringUserSecret.Data["password"])
+			}
+		}
 
-		// TODO: Setting the includeLogrotate argument to false for now. This
-		// should be changed when we implement log rotation for postgres
+		// For now, we are not using logrotate to rotate postgres or patroni logs
 		collector.AddToPod(ctx, cluster.Spec.Instrumentation, cluster.Spec.ImagePullPolicy, instanceConfigMap, &instance.Spec.Template.Spec,
-			[]corev1.VolumeMount{postgres.DataVolumeMount()}, "", false)
-	}
-
-	if err == nil &&
-		feature.Enabled(ctx, feature.OpenTelemetryMetrics) {
-
-		monitoringUserSecret := &corev1.Secret{ObjectMeta: naming.MonitoringUserSecret(cluster)}
-		err = errors.WithStack(
-			r.Client.Get(ctx, client.ObjectKeyFromObject(monitoringUserSecret), monitoringUserSecret))
-
-		if err == nil {
-			collector.AddToPod(ctx, cluster.Spec.Instrumentation, cluster.Spec.ImagePullPolicy, instanceConfigMap, &instance.Spec.Template.Spec,
-				[]corev1.VolumeMount{postgres.DataVolumeMount()}, string(monitoringUserSecret.Data["password"]), false)
-		}
+			[]corev1.VolumeMount{postgres.DataVolumeMount()}, pgPassword, false)
 	}
 
 	// Add postgres-exporter to the instance Pod spec

internal/controller/postgrescluster/pgbouncer.go

@@ -105,8 +105,12 @@ func (r *Reconciler) reconcilePGBouncerConfigMap(
 	}
 	// If OTel logging is enabled, add logrotate config
 	if err == nil && otelConfig != nil && feature.Enabled(ctx, feature.OpenTelemetryLogs) {
-		err = collector.AddLogrotateConfig(ctx, cluster.Spec.Instrumentation, configmap,
-			naming.PGBouncerFullLogPath, collector.PGBouncerPostRotateScript)
+		logrotateConfig := collector.LogrotateConfig{
+			LogFiles:         []string{naming.PGBouncerFullLogPath},
+			PostrotateScript: collector.PGBouncerPostRotateScript,
+		}
+		collector.AddLogrotateConfigs(ctx, cluster.Spec.Instrumentation, configmap,
+			[]collector.LogrotateConfig{logrotateConfig})
 	}
 	if err == nil {
 		err = errors.WithStack(r.apply(ctx, configmap))