From aba0f55da43a8bc075d0d01f8293081f0dde76b8 Mon Sep 17 00:00:00 2001 From: Drew Sessler Date: Thu, 20 Feb 2025 09:05:09 -0800 Subject: [PATCH] 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 | 51 +++++++++-------- internal/collector/config_test.go | 37 ++++++------ internal/collector/postgres.go | 56 +++++++++++++++++-- .../controller/postgrescluster/instance.go | 35 ++++++------ .../controller/postgrescluster/pgbouncer.go | 8 ++- 5 files changed, 124 insertions(+), 63 deletions(-) diff --git a/internal/collector/config.go b/internal/collector/config.go index 06ae6d9392..d288380aea 100644 --- a/internal/collector/config.go +++ b/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" } diff --git a/internal/collector/config_test.go b/internal/collector/config_test.go index 524c539e86..c621a14aad 100644 --- a/internal/collector/config_test.go +++ b/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) }) diff --git a/internal/collector/postgres.go b/internal/collector/postgres.go index 8e88cf1b33..04379fe08e 100644 --- a/internal/collector/postgres.go +++ b/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 +} diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 3bbd10b0c3..8300da5d0f 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/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 diff --git a/internal/controller/postgrescluster/pgbouncer.go b/internal/controller/postgrescluster/pgbouncer.go index 9fd4fb89fa..75550f11c3 100644 --- a/internal/controller/postgrescluster/pgbouncer.go +++ b/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))