Rotate logs whether retentionPeriod is set or not. Default to 1 day when not set.

Move PGBouncerPostRotateScript to collector package.
Round daily retention count up to the nearest day.

Author: dsessler7

internal/collector/config.go

@@ -119,18 +119,29 @@ func NewConfig(spec *v1beta1.InstrumentationSpec) *Config {
 // provided configmap
 func AddLogrotateConfig(ctx context.Context, spec *v1beta1.InstrumentationSpec,
 	outInstanceConfigMap *corev1.ConfigMap, logFilePath, postrotateScript string,
-) {
-	var logrotateConfig string
+) error {
+	var err error
+	var retentionPeriod *v1beta1.Duration
+
 	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.
 	if spec != nil && spec.Logs != nil && spec.Logs.RetentionPeriod != nil {
-		logrotateConfig = generateLogrotateConfig(logFilePath, spec.Logs.RetentionPeriod,
-			postrotateScript)
+		retentionPeriod = spec.Logs.RetentionPeriod
+	} else {
+		retentionPeriod, err = v1beta1.NewDuration("1d")
+		if err != nil {
+			return err
+		}
 	}
 
-	outInstanceConfigMap.Data["logrotate.conf"] = logrotateConfig
+	outInstanceConfigMap.Data["logrotate.conf"] = generateLogrotateConfig(logFilePath,
+		retentionPeriod, postrotateScript)
+
+	return err
 }
 
 // generateLogrotateConfig generates a configuration string for logrotate based
@@ -150,11 +161,14 @@ func generateLogrotateConfig(logFilePath string, retentionPeriod *v1beta1.Durati
 }
 
 // parseDurationForLogrotate takes a retention period and returns the rotate
-// number and interval string that should be used in the logrotate config
+// number and interval string that should be used in the logrotate config.
+// 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.Round(retentionPeriod.AsDuration().Hours())
 	if hours < 24 {
 		return int(hours), "hourly"
 	}
-	return int(math.Round(hours / 24)), "daily"
+	return int(math.Ceil(hours / 24)), "daily"
 }

internal/collector/config_test.go

@@ -64,7 +64,6 @@ service:
 	})
 }
 
-// TODO: write this test after rebasing on new retention API changes.
 func TestGenerateLogrotateConfig(t *testing.T) {
 	for _, tt := range []struct {
 		logFilePath      string
@@ -151,7 +150,7 @@ func TestParseDurationForLogrotate(t *testing.T) {
 		},
 		{
 			retentionPeriod: "35hour",
-			number:          1,
+			number:          2,
 			interval:        "daily",
 		},
 		{

internal/collector/pgbouncer.go

@@ -22,6 +22,10 @@ import (
 //go:embed "generated/pgbouncer_metrics_queries.json"
 var pgBouncerMetricsQueries json.RawMessage
 
+// PGBouncerPostRotateScript is the script that is run after pgBouncer's log
+// files have been rotated. The pgbouncer process is sent a sighup signal.
+const PGBouncerPostRotateScript = "pkill -HUP --exact pgbouncer"
+
 // NewConfigForPgBouncerPod creates a config for the OTel collector container
 // that runs as a sidecar in the pgBouncer Pod
 func NewConfigForPgBouncerPod(

internal/controller/postgrescluster/pgbouncer.go

@@ -103,12 +103,10 @@ func (r *Reconciler) reconcilePGBouncerConfigMap(
 		(feature.Enabled(ctx, feature.OpenTelemetryLogs) || feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
 		err = collector.AddToConfigMap(ctx, otelConfig, configmap)
 	}
-	// If OTel logging is enabled and retentionPeriod is set, add logrotate config
-	if err == nil && otelConfig != nil && feature.Enabled(ctx, feature.OpenTelemetryLogs) &&
-		cluster.Spec.Instrumentation != nil && cluster.Spec.Instrumentation.Logs != nil &&
-		cluster.Spec.Instrumentation.Logs.RetentionPeriod != nil {
-		collector.AddLogrotateConfig(ctx, cluster.Spec.Instrumentation, configmap,
-			naming.PGBouncerFullLogPath, naming.PGBouncerPostRotateScript)
+	// 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)
 	}
 	if err == nil {
 		err = errors.WithStack(r.apply(ctx, configmap))

internal/naming/names.go

@@ -161,12 +161,6 @@ const (
 	// PGbouncerFullLogPath is the full path to the pgbouncer log file
 	PGBouncerFullLogPath = PGBouncerLogPath + "/pgbouncer.log"
 
-	// PGBouncerPostRotateScript is the script that is run after pgBouncer's log
-	// files have been rotated. The pgbouncer process is sent a sighup signal.
-	// TODO: Should this go here or in controller/postgrescluster/pgbouncer.go?
-	// Or somewhere in the internal/pgbouncer folder, for that matter?
-	PGBouncerPostRotateScript = "pkill -HUP --exact pgbouncer"
-
 	// suffix used with postgrescluster name for associated configmap.
 	// for instance, if the cluster is named 'mycluster', the
 	// configmap will be named 'mycluster-pgbackrest-config'