Refactor logrotate config creation.

Refactor logic around adding collector to postgres instance pod.

Author: dsessler7

internal/collector/config.go

@@ -55,6 +55,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" +
@@ -116,10 +123,10 @@ 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, logFilesToRotate []string, postrotateScript string,
+// 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,
 ) error {
 	var err error
 	var retentionPeriod *v1beta1.Duration
@@ -139,25 +146,29 @@ func AddLogrotateConfig(ctx context.Context, spec *v1beta1.InstrumentationSpec,
 		}
 	}
 
-	outInstanceConfigMap.Data["logrotate.conf"] = generateLogrotateConfig(logFilesToRotate,
-		retentionPeriod, postrotateScript)
+	logrotateConfig := ""
+	for _, config := range configs {
+		logrotateConfig += generateLogrotateConfig(config, retentionPeriod)
+	}
+
+	outInstanceConfigMap.Data["logrotate.conf"] = logrotateConfig
 
 	return err
 }
 
 // generateLogrotateConfig generates a configuration string for logrotate based
 // on the provided full log file path, retention period, and postrotate script
-func generateLogrotateConfig(logFilesToRotate []string, retentionPeriod *v1beta1.Duration,
-	postrotateScript string,
+func generateLogrotateConfig(
+	config LogrotateConfig, retentionPeriod *v1beta1.Duration,
 ) string {
 	number, interval := parseDurationForLogrotate(retentionPeriod)
 
 	return fmt.Sprintf(
 		logrotateConfigFormatString,
-		strings.Join(logFilesToRotate, " "),
+		strings.Join(config.LogFiles, " "),
 		number,
 		interval,
-		postrotateScript,
+		config.PostrotateScript,
 	)
 }
 

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)
 			assert.Equal(t, tt.result, result)
 		})
 	}

internal/controller/postgrescluster/instance.go

@@ -1200,26 +1200,32 @@ 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)) {
-
-		// TODO: Setting the includeLogrotate argument to false for now. This
-		// should be changed when we implement log rotation for postgres
-		collector.AddToPod(ctx, cluster.Spec.Instrumentation, cluster.Spec.ImagePullPolicy, instanceConfigMap, &instance.Spec.Template.Spec,
-			[]corev1.VolumeMount{postgres.DataVolumeMount()}, "", true)
-	}
-
-	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))
+		(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"])
+			}
+		}
 
-		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)
+		// If the OpenTelemetryLogging feature is enabled, we need to set includeLogrotate to true
+		includeLogrotate := false
+		if feature.Enabled(ctx, feature.OpenTelemetryLogs) {
+			includeLogrotate = true
 		}
+
+		collector.AddToPod(ctx, cluster.Spec.Instrumentation, cluster.Spec.ImagePullPolicy, instanceConfigMap, &instance.Spec.Template.Spec,
+			[]corev1.VolumeMount{postgres.DataVolumeMount()}, pgPassword, includeLogrotate)
 	}
 
 	// Add postgres-exporter to the instance Pod spec
@@ -1429,12 +1435,16 @@ func (r *Reconciler) reconcileInstanceConfigMap(
 	}
 	// If OTel logging is enabled, add logrotate config
 	if err == nil && otelConfig != nil && feature.Enabled(ctx, feature.OpenTelemetryLogs) {
-		logFilesToRotate := []string{
+		postgresFilesToRotate := []string{
 			postgres.LogDirectory() + "/postgresql.json",
 			postgres.LogDirectory() + "/postgresql.csv",
 		}
-		err = collector.AddLogrotateConfig(ctx, cluster.Spec.Instrumentation, instanceConfigMap,
-			logFilesToRotate, collector.PostgresPostRotateScript)
+		postgresLogrotateConfig := collector.LogrotateConfig{
+			LogFiles:         postgresFilesToRotate,
+			PostrotateScript: collector.PostgresPostRotateScript,
+		}
+		err = collector.AddLogrotateConfigs(ctx, cluster.Spec.Instrumentation, instanceConfigMap,
+			[]collector.LogrotateConfig{postgresLogrotateConfig})
 	}
 	if err == nil {
 		err = patroni.InstanceConfigMap(ctx, cluster, spec, instanceConfigMap)

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,
-			[]string{naming.PGBouncerFullLogPath}, collector.PGBouncerPostRotateScript)
+		logrotateConfig := collector.LogrotateConfig{
+			LogFiles:         []string{naming.PGBouncerFullLogPath},
+			PostrotateScript: collector.PGBouncerPostRotateScript,
+		}
+		err = collector.AddLogrotateConfigs(ctx, cluster.Spec.Instrumentation, configmap,
+			[]collector.LogrotateConfig{logrotateConfig})
 	}
 	if err == nil {
 		err = errors.WithStack(r.apply(ctx, configmap))