Rotate pgbackrest (CrunchyData#4108)

* Rotate pgbackrest logs in repohost and Postgres pod * Dir creation in collector startup Issues: [PGO-2171]
philrhurst · philrhurst · Mar 3, 2025 · Feb 7, 2025 · Dec 23, 2024 · Jan 6, 2025
commit b50bae9782aa202015feacbd3afedaa434fb630f
diff --git a/internal/collector/instance.go b/internal/collector/instance.go
@@ -7,13 +7,15 @@ package collector
 import (
 	"context"
 	"fmt"
+	"path"
 
 	corev1 "k8s.io/api/core/v1"
 
 	"github.com/crunchydata/postgres-operator/internal/config"
 	"github.com/crunchydata/postgres-operator/internal/feature"
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/naming"
+	"github.com/crunchydata/postgres-operator/internal/shell"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -44,9 +46,12 @@ func AddToPod(
 	outPod *corev1.PodSpec,
 	volumeMounts []corev1.VolumeMount,
 	sqlQueryPassword string,
+	logDirectories []string,
 	includeLogrotate bool,
 ) {
-	if !(feature.Enabled(ctx, feature.OpenTelemetryLogs) || feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
+	if spec == nil ||
+		!(feature.Enabled(ctx, feature.OpenTelemetryLogs) ||
+			feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
 		return
 	}
 
@@ -84,7 +89,7 @@ func AddToPod(
 		Name:            naming.ContainerCollector,
 		Image:           config.CollectorContainerImage(spec),
 		ImagePullPolicy: pullPolicy,
-		Command:         startCommand(includeLogrotate),
+		Command:         startCommand(logDirectories, includeLogrotate),
 		Env: []corev1.EnvVar{
 			{
 				Name: "K8S_POD_NAMESPACE",
@@ -146,13 +151,23 @@ func AddToPod(
 }
 
 // startCommand generates the command script used by the collector container
-func startCommand(includeLogrotate bool) []string {
+func startCommand(logDirectories []string, includeLogrotate bool) []string {
+	var mkdirScript string
+	if len(logDirectories) != 0 {
+		for _, logDir := range logDirectories {
+			mkdirScript = mkdirScript + `
+` + shell.MakeDirectories(0o775, logDir,
+				path.Join(logDir, "receiver"))
+		}
+	}
+
 	var logrotateCommand string
 	if includeLogrotate {
 		logrotateCommand = `logrotate -s /tmp/logrotate.status /etc/logrotate.d/logrotate.conf`
 	}
 
 	var startScript = fmt.Sprintf(`
+%s
 OTEL_PIDFILE=/tmp/otel.pid
 
 start_otel_collector() {
@@ -175,7 +190,7 @@ while read -r -t 5 -u "${fd}" ||:; do
 		start_otel_collector
 	fi
 done
-`, configDirectory, logrotateCommand)
+`, mkdirScript, configDirectory, logrotateCommand)
 
 	wrapper := `monitor() {` + startScript + `}; export directory="$1"; export -f monitor; exec -a "$0" bash -ceu monitor`
 

diff --git a/internal/collector/pgbackrest.go b/internal/collector/pgbackrest.go
@@ -47,10 +47,9 @@ func NewConfigForPgBackrestRepoHostPod(
 
 		// Keep track of what log records and files have been processed.
 		// Use a subdirectory of the logs directory to stay within the same failure domain.
-		// TODO(log-rotation): Create this directory during Collector startup.
 		config.Extensions["file_storage/pgbackrest_logs"] = map[string]any{
 			"directory":        directory + "/receiver",
-			"create_directory": true,
+			"create_directory": false,
 			"fsync":            true,
 		}
 

diff --git a/internal/collector/pgbackrest_test.go b/internal/collector/pgbackrest_test.go
@@ -39,7 +39,7 @@ exporters:
     verbosity: detailed
 extensions:
   file_storage/pgbackrest_logs:
-    create_directory: true
+    create_directory: false
     directory: /pgbackrest/repo1/log/receiver
     fsync: true
 processors:
@@ -131,7 +131,7 @@ exporters:
     project: google-project-name
 extensions:
   file_storage/pgbackrest_logs:
-    create_directory: true
+    create_directory: false
     directory: /pgbackrest/repo1/log/receiver
     fsync: true
 processors:

diff --git a/internal/collector/pgbouncer.go b/internal/collector/pgbouncer.go
@@ -54,12 +54,11 @@ func EnablePgBouncerLogging(ctx context.Context,
 
 		// Keep track of what log records and files have been processed.
 		// Use a subdirectory of the logs directory to stay within the same failure domain.
-		// TODO(log-rotation): Create this directory during Collector startup.
 		//
 		// https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/-/extension/storage/filestorage#readme
 		outConfig.Extensions["file_storage/pgbouncer_logs"] = map[string]any{
 			"directory":        directory + "/receiver",
-			"create_directory": true,
+			"create_directory": false,
 			"fsync":            true,
 		}
 

diff --git a/internal/collector/pgbouncer_test.go b/internal/collector/pgbouncer_test.go
@@ -35,7 +35,7 @@ exporters:
     verbosity: detailed
 extensions:
   file_storage/pgbouncer_logs:
-    create_directory: true
+    create_directory: false
     directory: /tmp/receiver
     fsync: true
 processors:
@@ -124,7 +124,7 @@ exporters:
     project: google-project-name
 extensions:
   file_storage/pgbouncer_logs:
-    create_directory: true
+    create_directory: false
     directory: /tmp/receiver
     fsync: true
 processors:

diff --git a/internal/collector/postgres.go b/internal/collector/postgres.go
@@ -27,10 +27,13 @@ func NewConfigForPostgresPod(ctx context.Context,
 ) *Config {
 	config := NewConfig(inCluster.Spec.Instrumentation)
 
+	// Metrics
 	EnablePostgresMetrics(ctx, inCluster, config)
 	EnablePatroniMetrics(ctx, inCluster, config)
-	EnablePatroniLogging(ctx, inCluster, config)
+
+	// Logging
 	EnablePostgresLogging(ctx, inCluster, config, outParameters)
+	EnablePatroniLogging(ctx, inCluster, config)
 
 	return config
 }
@@ -229,10 +232,9 @@ func EnablePostgresLogging(
 		}
 
 		// pgBackRest pipeline
-		// TODO(log-rotation): Create this directory during Collector startup.
 		outConfig.Extensions["file_storage/pgbackrest_logs"] = map[string]any{
 			"directory":        naming.PGBackRestPGDataLogPath + "/receiver",
-			"create_directory": true,
+			"create_directory": false,
 			"fsync":            true,
 		}
 

diff --git a/internal/collector/postgres_test.go b/internal/collector/postgres_test.go
@@ -40,7 +40,7 @@ exporters:
     verbosity: detailed
 extensions:
   file_storage/pgbackrest_logs:
-    create_directory: true
+    create_directory: false
     directory: /pgdata/pgbackrest/log/receiver
     fsync: true
   file_storage/postgres_logs:
@@ -272,7 +272,7 @@ exporters:
     project: google-project-name
 extensions:
   file_storage/pgbackrest_logs:
-    create_directory: true
+    create_directory: false
     directory: /pgdata/pgbackrest/log/receiver
     fsync: true
   file_storage/postgres_logs:

diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go
@@ -1219,8 +1219,10 @@ func (r *Reconciler) reconcileInstance(
 		}
 
 		// For now, we are not using logrotate to rotate postgres or patroni logs
+		// but we are using it for pgbackrest logs in the postgres pod
 		collector.AddToPod(ctx, cluster.Spec.Instrumentation, cluster.Spec.ImagePullPolicy, instanceConfigMap, &instance.Spec.Template.Spec,
-			[]corev1.VolumeMount{postgres.DataVolumeMount()}, pgPassword, false)
+			[]corev1.VolumeMount{postgres.DataVolumeMount()}, pgPassword,
+			[]string{naming.PGBackRestPGDataLogPath}, true)
 	}
 
 	// Add postgres-exporter to the instance Pod spec
@@ -1425,8 +1427,24 @@ func (r *Reconciler) reconcileInstanceConfigMap(
 		})
 
 	// If OTel logging or metrics is enabled, add collector config
-	if err == nil && (feature.Enabled(ctx, feature.OpenTelemetryLogs) || feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
+	if err == nil &&
+		(feature.Enabled(ctx, feature.OpenTelemetryLogs) ||
+			feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
 		err = collector.AddToConfigMap(ctx, otelConfig, instanceConfigMap)
+
+		// Add pgbackrest logrotate if OpenTelemetryLogs is enabled and
+		// local volumes are available
+		if err == nil &&
+			feature.Enabled(ctx, feature.OpenTelemetryLogs) &&
+			pgbackrest.RepoHostVolumeDefined(cluster) &&
+			cluster.Spec.Instrumentation != nil {
+
+			collector.AddLogrotateConfigs(ctx, cluster.Spec.Instrumentation,
+				instanceConfigMap,
+				[]collector.LogrotateConfig{{
+					LogFiles: []string{naming.PGBackRestPGDataLogPath + "/*.log"},
+				}})
+		}
 	}
 	if err == nil {
 		err = patroni.InstanceConfigMap(ctx, cluster, spec, instanceConfigMap)

diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go
@@ -688,18 +688,17 @@ func (r *Reconciler) generateRepoHostIntent(ctx context.Context, postgresCluster
 
 	if pgbackrest.RepoHostVolumeDefined(postgresCluster) {
 		// add the init container to make the pgBackRest repo volume log directory
-		pgbackrest.MakePGBackrestLogDir(&repo.Spec.Template, postgresCluster)
+		pgBackRestLogPath := pgbackrest.MakePGBackrestLogDir(&repo.Spec.Template, postgresCluster)
 
 		containersToAdd := []string{naming.PGBackRestRepoContainerName}
 
 		// If OpenTelemetryLogs is enabled, we want to add the collector to the pod
 		// and also add the RepoVolumes to the container.
-		if feature.Enabled(ctx, feature.OpenTelemetryLogs) {
-			// TODO: Setting the includeLogrotate argument to false for now. This
-			// should be changed when we implement log rotation for pgbackrest
+		if postgresCluster.Spec.Instrumentation != nil && feature.Enabled(ctx, feature.OpenTelemetryLogs) {
 			collector.AddToPod(ctx, postgresCluster.Spec.Instrumentation, postgresCluster.Spec.ImagePullPolicy,
 				&corev1.ConfigMap{ObjectMeta: naming.PGBackRestConfig(postgresCluster)},
-				&repo.Spec.Template.Spec, []corev1.VolumeMount{}, "", false)
+				&repo.Spec.Template.Spec, []corev1.VolumeMount{}, "",
+				[]string{pgBackRestLogPath}, true)
 
 			containersToAdd = append(containersToAdd, naming.ContainerCollector)
 		}

diff --git a/internal/controller/postgrescluster/pgbouncer.go b/internal/controller/postgrescluster/pgbouncer.go
@@ -100,7 +100,8 @@ func (r *Reconciler) reconcilePGBouncerConfigMap(
 	}
 	// If OTel logging or metrics is enabled, add collector config
 	if otelConfig != nil &&
-		(feature.Enabled(ctx, feature.OpenTelemetryLogs) || feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
+		(feature.Enabled(ctx, feature.OpenTelemetryLogs) ||
+			feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
 		err = collector.AddToConfigMap(ctx, otelConfig, configmap)
 	}
 	// If OTel logging is enabled, add logrotate config

diff --git a/internal/controller/standalone_pgadmin/statefulset.go b/internal/controller/standalone_pgadmin/statefulset.go
@@ -132,7 +132,7 @@ func statefulset(
 		}
 
 		collector.AddToPod(ctx, pgadmin.Spec.Instrumentation, pgadmin.Spec.ImagePullPolicy,
-			configmap, &sts.Spec.Template.Spec, volumeMounts, "", false)
+			configmap, &sts.Spec.Template.Spec, volumeMounts, "", []string{}, false)
 	}
 
 	return sts

diff --git a/internal/pgbackrest/config.go b/internal/pgbackrest/config.go
@@ -17,6 +17,7 @@ import (
 
 	"github.com/crunchydata/postgres-operator/internal/collector"
 	"github.com/crunchydata/postgres-operator/internal/config"
+	"github.com/crunchydata/postgres-operator/internal/feature"
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
@@ -129,11 +130,32 @@ func CreatePGBackRestConfigMapIntent(ctx context.Context, postgresCluster *v1bet
 				postgresCluster.Spec.Backups.PGBackRest.Global,
 			).String()
 
-		err = collector.AddToConfigMap(ctx, collector.NewConfigForPgBackrestRepoHostPod(
-			ctx,
-			postgresCluster.Spec.Instrumentation,
-			postgresCluster.Spec.Backups.PGBackRest.Repos,
-		), cm)
+		if RepoHostVolumeDefined(postgresCluster) &&
+			(feature.Enabled(ctx, feature.OpenTelemetryLogs) ||
+				feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
+			err = collector.AddToConfigMap(ctx, collector.NewConfigForPgBackrestRepoHostPod(
+				ctx,
+				postgresCluster.Spec.Instrumentation,
+				postgresCluster.Spec.Backups.PGBackRest.Repos,
+			), cm)
+
+			// If OTel logging is enabled, add logrotate config for the RepoHost
+			if err == nil &&
+				postgresCluster.Spec.Instrumentation != nil &&
+				feature.Enabled(ctx, feature.OpenTelemetryLogs) {
+				var pgBackRestLogPath string
+				for _, repo := range postgresCluster.Spec.Backups.PGBackRest.Repos {
+					if repo.Volume != nil {
+						pgBackRestLogPath = fmt.Sprintf(naming.PGBackRestRepoLogPath, repo.Name)
+						break
+					}
+				}
+
+				collector.AddLogrotateConfigs(ctx, postgresCluster.Spec.Instrumentation, cm, []collector.LogrotateConfig{{
+					LogFiles: []string{pgBackRestLogPath + "/*.log"},
+				}})
+			}
+		}
 	}
 
 	cm.Data[ConfigHashKey] = configHash
@@ -144,7 +166,7 @@ func CreatePGBackRestConfigMapIntent(ctx context.Context, postgresCluster *v1bet
 // MakePGBackrestLogDir creates the pgBackRest default log path directory used when a
 // dedicated repo host is configured.
 func MakePGBackrestLogDir(template *corev1.PodTemplateSpec,
-	cluster *v1beta1.PostgresCluster) {
+	cluster *v1beta1.PostgresCluster) string {
 
 	var pgBackRestLogPath string
 	for _, repo := range cluster.Spec.Backups.PGBackRest.Repos {
@@ -172,6 +194,8 @@ func MakePGBackrestLogDir(template *corev1.PodTemplateSpec,
 		}
 	}
 	template.Spec.InitContainers = append(template.Spec.InitContainers, container)
+
+	return pgBackRestLogPath
 }
 
 // RestoreCommand returns the command for performing a pgBackRest restore.  In addition to calling

diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go
@@ -192,7 +192,7 @@ func Pod(
 
 	if feature.Enabled(ctx, feature.OpenTelemetryLogs) || feature.Enabled(ctx, feature.OpenTelemetryMetrics) {
 		collector.AddToPod(ctx, inCluster.Spec.Instrumentation, inCluster.Spec.ImagePullPolicy, inConfigMap,
-			outPod, []corev1.VolumeMount{configVolumeMount}, string(inSecret.Data["pgbouncer-password"]),
+			outPod, []corev1.VolumeMount{configVolumeMount}, string(inSecret.Data["pgbouncer-password"]), []string{naming.PGBouncerLogPath},
 			true)
 	}
 }