Rotate postgres logs

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))