From 1d8abd3c10d8b7116691c4e015c3b24aeb119d84 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Sat, 1 Mar 2025 21:40:22 -0600 Subject: [PATCH 1/9] pgadmin rotate Issues: [PGO-2168] --- internal/collector/config.go | 6 +- internal/collector/config_test.go | 2 +- .../controller/postgrescluster/instance.go | 2 +- .../controller/postgrescluster/pgadmin.go | 2 +- .../controller/postgrescluster/pgbackrest.go | 4 +- .../controller/postgrescluster/pgbouncer.go | 2 +- .../controller/postgrescluster/snapshots.go | 2 +- internal/controller/postgrescluster/util.go | 4 +- internal/controller/standalone_pgadmin/pod.go | 119 +++++++++++------- .../controller/standalone_pgadmin/pod_test.go | 75 ++++------- .../standalone_pgadmin/statefulset.go | 10 +- 11 files changed, 117 insertions(+), 111 deletions(-) diff --git a/internal/collector/config.go b/internal/collector/config.go index d288380aea..8b77177a7a 100644 --- a/internal/collector/config.go +++ b/internal/collector/config.go @@ -153,7 +153,7 @@ func AddLogrotateConfigs(ctx context.Context, spec *v1beta1.InstrumentationSpec, func generateLogrotateConfig( config LogrotateConfig, retentionPeriod metav1.Duration, ) string { - number, interval := parseDurationForLogrotate(retentionPeriod) + number, interval := ParseDurationForLogrotate(retentionPeriod) return fmt.Sprintf( logrotateConfigFormatString, @@ -164,12 +164,12 @@ func generateLogrotateConfig( ) } -// parseDurationForLogrotate takes a retention period and returns the rotate +// ParseDurationForLogrotate takes a retention period and returns the rotate // 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 metav1.Duration) (int, string) { +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 c621a14aad..cc37a9b8f7 100644 --- a/internal/collector/config_test.go +++ b/internal/collector/config_test.go @@ -197,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.AsDuration()) + number, interval := ParseDurationForLogrotate(duration.AsDuration()) assert.Equal(t, tt.number, number) assert.Equal(t, tt.interval, interval) }) diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 6d6509eafb..5c9786459d 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -1242,7 +1242,7 @@ func (r *Reconciler) reconcileInstance( // add an emptyDir volume to the PodTemplateSpec and an associated '/tmp' volume mount to // all containers included within that spec if err == nil { - addTMPEmptyDir(&instance.Spec.Template) + AddTMPEmptyDir(&instance.Spec.Template) } // mount shared memory to the Postgres instance diff --git a/internal/controller/postgrescluster/pgadmin.go b/internal/controller/postgrescluster/pgadmin.go index 40874aa1be..87d385becd 100644 --- a/internal/controller/postgrescluster/pgadmin.go +++ b/internal/controller/postgrescluster/pgadmin.go @@ -365,7 +365,7 @@ func (r *Reconciler) reconcilePGAdminStatefulSet( // add an emptyDir volume to the PodTemplateSpec and an associated '/tmp' // volume mount to all containers included within that spec - addTMPEmptyDir(&sts.Spec.Template) + AddTMPEmptyDir(&sts.Spec.Template) return errors.WithStack(r.apply(ctx, sts)) } diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index 3645871bd5..54068193af 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -720,7 +720,7 @@ func (r *Reconciler) generateRepoHostIntent(ctx context.Context, postgresCluster postgresCluster.Spec.ImagePullPolicy, &repo.Spec.Template) - addTMPEmptyDir(&repo.Spec.Template) + AddTMPEmptyDir(&repo.Spec.Template) // set ownership references if err := r.setControllerReference(postgresCluster, repo); err != nil { @@ -1272,7 +1272,7 @@ func (r *Reconciler) reconcileRestoreJob(ctx context.Context, cluster.Spec.ImagePullPolicy, &restoreJob.Spec.Template) - addTMPEmptyDir(&restoreJob.Spec.Template) + AddTMPEmptyDir(&restoreJob.Spec.Template) return errors.WithStack(r.apply(ctx, restoreJob)) } diff --git a/internal/controller/postgrescluster/pgbouncer.go b/internal/controller/postgrescluster/pgbouncer.go index 2b1dcae779..d5a935bbf3 100644 --- a/internal/controller/postgrescluster/pgbouncer.go +++ b/internal/controller/postgrescluster/pgbouncer.go @@ -476,7 +476,7 @@ func (r *Reconciler) generatePGBouncerDeployment( } // Add tmp directory and volume for log files - addTMPEmptyDir(&deploy.Spec.Template) + AddTMPEmptyDir(&deploy.Spec.Template) return deploy, true, err } diff --git a/internal/controller/postgrescluster/snapshots.go b/internal/controller/postgrescluster/snapshots.go index fa168ebdf4..c639408df2 100644 --- a/internal/controller/postgrescluster/snapshots.go +++ b/internal/controller/postgrescluster/snapshots.go @@ -394,7 +394,7 @@ func (r *Reconciler) dedicatedSnapshotVolumeRestore(ctx context.Context, cluster.Spec.ImagePullPolicy, &restoreJob.Spec.Template) - addTMPEmptyDir(&restoreJob.Spec.Template) + AddTMPEmptyDir(&restoreJob.Spec.Template) restoreJob.Annotations[naming.PGBackRestBackupJobCompletion] = backupJob.Status.CompletionTime.Format(time.RFC3339) return errors.WithStack(r.apply(ctx, restoreJob)) diff --git a/internal/controller/postgrescluster/util.go b/internal/controller/postgrescluster/util.go index bb5b3e085a..a1ba6ce087 100644 --- a/internal/controller/postgrescluster/util.go +++ b/internal/controller/postgrescluster/util.go @@ -134,13 +134,13 @@ func addDevSHM(template *corev1.PodTemplateSpec) { } } -// addTMPEmptyDir adds a "tmp" EmptyDir volume to the provided Pod template, while then also adding a +// AddTMPEmptyDir adds a "tmp" EmptyDir volume to the provided Pod template, while then also adding a // volume mount at /tmp for all containers defined within the Pod template // The '/tmp' directory is currently utilized for the following: // - As the pgBackRest lock directory (this is the default lock location for pgBackRest) // - The location where the replication client certificates can be loaded with the proper // permissions set -func addTMPEmptyDir(template *corev1.PodTemplateSpec) { +func AddTMPEmptyDir(template *corev1.PodTemplateSpec) { template.Spec.Volumes = append(template.Spec.Volumes, corev1.Volume{ Name: "tmp", diff --git a/internal/controller/standalone_pgadmin/pod.go b/internal/controller/standalone_pgadmin/pod.go index 7590a3a3cc..9cb6de0e42 100644 --- a/internal/controller/standalone_pgadmin/pod.go +++ b/internal/controller/standalone_pgadmin/pod.go @@ -7,14 +7,16 @@ package standalone_pgadmin import ( "context" "fmt" - "path" + "strconv" "strings" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/util/intstr" + "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/kubernetes" "github.com/crunchydata/postgres-operator/internal/naming" @@ -45,6 +47,7 @@ const ( // pod populates a PodSpec with the container and volumes needed to run pgAdmin. func pod( + ctx context.Context, inPGAdmin *v1beta1.PGAdmin, inConfigMap *corev1.ConfigMap, outPod *corev1.PodSpec, @@ -84,15 +87,6 @@ func pod( }, } - // create a temp volume for restart pid/other/debugging use - // TODO: discuss tmp vol vs. persistent vol - tmpVolume := corev1.Volume{Name: "tmp"} - tmpVolume.VolumeSource = corev1.VolumeSource{ - EmptyDir: &corev1.EmptyDirVolumeSource{ - Medium: corev1.StorageMediumMemory, - }, - } - // pgadmin container container := corev1.Container{ Name: naming.ContainerPGAdmin, @@ -140,10 +134,6 @@ func pod( MountPath: scriptMountPath, ReadOnly: true, }, - { - Name: tmpVolume.Name, - MountPath: "/tmp", - }, }, } @@ -168,7 +158,7 @@ func pod( startup := corev1.Container{ Name: naming.ContainerPGAdminStartup, - Command: startupCommand(), + Command: startupCommand(ctx, inPGAdmin), Image: container.Image, ImagePullPolicy: container.ImagePullPolicy, Resources: container.Resources, @@ -192,7 +182,6 @@ func pod( configVolume, dataVolume, scriptVolume, - tmpVolume, } outPod.Containers = []corev1.Container{container} outPod.InitContainers = []corev1.Container{startup} @@ -352,7 +341,7 @@ done } // startupCommand returns an entrypoint that prepares the filesystem for pgAdmin. -func startupCommand() []string { +func startupCommand(ctx context.Context, inPgadmin *v1beta1.PGAdmin) []string { // pgAdmin reads from the `/etc/pgadmin/config_system.py` file during startup // after all other config files. // - https://github.com/pgadmin-org/pgadmin4/blob/REL-7_7/docs/en_US/config_py.rst @@ -374,10 +363,11 @@ func startupCommand() []string { // configDatabaseURIPath is the path for mounting the database URI connection string configDatabaseURIPathAbsolutePath = configMountPath + "/" + configDatabaseURIPath + ) - // The constants set in configSystem will not be overridden through - // spec.config.settings. - configSystem = ` + // The values set in configSystem will not be overridden through + // spec.config.settings. + var configSystem = ` import glob, json, re, os, logging DEFAULT_BINARY_PATHS = {'pg': sorted([''] + glob.glob('/usr/pgsql-*/bin')).pop()} with open('` + configMountPath + `/` + configFilePath + `') as _f: @@ -393,44 +383,82 @@ if os.path.isfile('` + configDatabaseURIPathAbsolutePath + `'): DATA_DIR = '` + dataMountPath + `' LOG_FILE = '` + LogFileAbsolutePath + `' -LOG_ROTATION_AGE = 24 * 60 # minutes +` + + // Gunicorn reads from the `/etc/pgadmin/gunicorn_config.py` file during startup + // after all other config files. + // - https://docs.gunicorn.org/en/latest/configure.html#configuration-file + // + // This command writes a script in `/etc/pgadmin/gunicorn_config.py` that reads + // from the `gunicorn-config.json` file and sets those variables globally. + // That way those values are available as settings when Gunicorn starts. + // + // Note: All Gunicorn settings are lowercase with underscores, so ignore + // any keys/names that are not. + var gunicornConfig = ` +import json, re +with open('` + configMountPath + `/` + gunicornConfigFilePath + `') as _f: + _conf, _data = re.compile(r'[a-z_]+'), json.load(_f) + if type(_data) is dict: + globals().update({k: v for k, v in _data.items() if _conf.fullmatch(k)}) +` + + // If OTel logs feature gate is enabled, we want to change the pgAdmin/gunicorn logging + if feature.Enabled(ctx, feature.OpenTelemetryLogs) && inPgadmin.Spec.Instrumentation != nil { + + var ( + maxBackupRetentionNumber = "1" + // One day in minutes for pgadmin rotation + pgAdminRetentionPeriod = "24 * 60" + // Daily rotation for gunicorn rotation + gunicornRetentionPeriod = "D" + ) + + // If the user has set a retention period, we will use those values for log rotation, + // which is otherwise managed by python. + if inPgadmin.Spec.Instrumentation.Logs != nil && + inPgadmin.Spec.Instrumentation.Logs.RetentionPeriod != nil { + + retentionNumber, period := collector.ParseDurationForLogrotate(inPgadmin.Spec.Instrumentation.Logs.RetentionPeriod.AsDuration()) + // `LOG_ROTATION_MAX_LOG_FILES`` in pgadmin refers to the already rotated logs. + // `backupCount` for gunicorn is similar. + // Our retention unit is for total number of log files, so subtract 1 to account + // for the currently-used log file. + maxBackupRetentionNumber = strconv.Itoa(retentionNumber - 1) + if period == "hourly" { + // If the period is hourly, set the pgadmin + // and gunicorn retention periods to hourly. + pgAdminRetentionPeriod = "60" + gunicornRetentionPeriod = "H" + } + } + + configSystem = configSystem + ` +LOG_ROTATION_AGE = ` + pgAdminRetentionPeriod + ` # minutes LOG_ROTATION_SIZE = 5 # MiB -LOG_ROTATION_MAX_LOG_FILES = 1 +LOG_ROTATION_MAX_LOG_FILES = ` + maxBackupRetentionNumber + ` JSON_LOGGER = True CONSOLE_LOG_LEVEL = logging.WARNING FILE_LOG_LEVEL = logging.INFO FILE_LOG_FORMAT_JSON = {'time': 'created', 'name': 'name', 'level': 'levelname', 'message': 'message'} ` - // Gunicorn reads from the `/etc/pgadmin/gunicorn_config.py` file during startup - // after all other config files. - // - https://docs.gunicorn.org/en/latest/configure.html#configuration-file - // - // This command writes a script in `/etc/pgadmin/gunicorn_config.py` that reads - // from the `gunicorn-config.json` file and sets those variables globally. - // That way those values are available as settings when Gunicorn starts. - // - // Note: All Gunicorn settings are lowercase with underscores, so ignore - // any keys/names that are not. - // + // Gunicorn uses the Python logging package, which sets the following attributes: // https://docs.python.org/3/library/logging.html#logrecord-attributes. // JsonFormatter is used to format the log: https://pypi.org/project/jsonformatter/ - gunicornConfig = ` -import json, re, collections, copy, gunicorn, gunicorn.glogging -with open('` + configMountPath + `/` + gunicornConfigFilePath + `') as _f: - _conf, _data = re.compile(r'[a-z_]+'), json.load(_f) - if type(_data) is dict: - globals().update({k: v for k, v in _data.items() if _conf.fullmatch(k)}) - + gunicornConfig = gunicornConfig + ` +import collections, copy, gunicorn, gunicorn.glogging gunicorn.SERVER_SOFTWARE = 'Python' logconfig_dict = copy.deepcopy(gunicorn.glogging.CONFIG_DEFAULTS) logconfig_dict['loggers']['gunicorn.access']['handlers'] = ['file'] logconfig_dict['loggers']['gunicorn.error']['handlers'] = ['file'] logconfig_dict['handlers']['file'] = { - 'class': 'logging.handlers.RotatingFileHandler', + 'class': 'logging.handlers.TimedRotatingFileHandler', 'filename': '` + GunicornLogFileAbsolutePath + `', - 'backupCount': 1, 'maxBytes': 2 << 20, # MiB + 'backupCount': ` + maxBackupRetentionNumber + `, + 'interval': 1, # every one unit (defined by when), rotate + 'when': '` + gunicornRetentionPeriod + `', 'formatter': 'json', } logconfig_dict['formatters']['json'] = { @@ -444,7 +472,7 @@ logconfig_dict['formatters']['json'] = { ]) } ` - ) + } args := []string{strings.TrimLeft(configSystem, "\n"), strings.TrimLeft(gunicornConfig, "\n")} @@ -453,11 +481,6 @@ logconfig_dict['formatters']['json'] = { // - https://issue.k8s.io/121294 shell.MakeDirectories(0o775, scriptMountPath, configMountPath), - // Create the logs directory with g+rwx so the OTel Collector can - // write to it as well. - // TODO(log-rotation): Move the last segment into the Collector startup. - shell.MakeDirectories(0o775, dataMountPath, path.Join(LogDirectoryAbsolutePath, "receiver")), - // Write the system and server configurations. `echo "$1" > ` + scriptMountPath + `/config_system.py`, `echo "$2" > ` + scriptMountPath + `/gunicorn_config.py`, diff --git a/internal/controller/standalone_pgadmin/pod_test.go b/internal/controller/standalone_pgadmin/pod_test.go index ce3ad076d2..d87f916b47 100644 --- a/internal/controller/standalone_pgadmin/pod_test.go +++ b/internal/controller/standalone_pgadmin/pod_test.go @@ -13,6 +13,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/crunchydata/postgres-operator/internal/feature" "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/kubernetes" "github.com/crunchydata/postgres-operator/internal/testing/cmp" @@ -28,8 +29,9 @@ func TestPod(t *testing.T) { config := new(corev1.ConfigMap) testpod := new(corev1.PodSpec) pvc := new(corev1.PersistentVolumeClaim) + ctx := context.Background() - call := func() { pod(pgadmin, config, testpod, pvc) } + call := func() { pod(ctx, pgadmin, config, testpod, pvc) } t.Run("Defaults", func(t *testing.T) { @@ -130,8 +132,6 @@ containers: - mountPath: /etc/pgadmin name: pgadmin-config-system readOnly: true - - mountPath: /tmp - name: tmp initContainers: - command: - bash @@ -139,7 +139,6 @@ initContainers: - -- - |- mkdir -p '/etc/pgadmin/conf.d' && chmod 0775 '/etc/pgadmin/conf.d' - mkdir -p '/var/lib/pgadmin/logs/receiver' && chmod 0775 '/var/lib/pgadmin/logs/receiver' '/var/lib/pgadmin/logs' echo "$1" > /etc/pgadmin/config_system.py echo "$2" > /etc/pgadmin/gunicorn_config.py - startup @@ -159,41 +158,12 @@ initContainers: DATA_DIR = '/var/lib/pgadmin' LOG_FILE = '/var/lib/pgadmin/logs/pgadmin.log' - LOG_ROTATION_AGE = 24 * 60 # minutes - LOG_ROTATION_SIZE = 5 # MiB - LOG_ROTATION_MAX_LOG_FILES = 1 - - JSON_LOGGER = True - CONSOLE_LOG_LEVEL = logging.WARNING - FILE_LOG_LEVEL = logging.INFO - FILE_LOG_FORMAT_JSON = {'time': 'created', 'name': 'name', 'level': 'levelname', 'message': 'message'} - | - import json, re, collections, copy, gunicorn, gunicorn.glogging + import json, re with open('/etc/pgadmin/conf.d/~postgres-operator/gunicorn-config.json') as _f: _conf, _data = re.compile(r'[a-z_]+'), json.load(_f) if type(_data) is dict: globals().update({k: v for k, v in _data.items() if _conf.fullmatch(k)}) - - gunicorn.SERVER_SOFTWARE = 'Python' - logconfig_dict = copy.deepcopy(gunicorn.glogging.CONFIG_DEFAULTS) - logconfig_dict['loggers']['gunicorn.access']['handlers'] = ['file'] - logconfig_dict['loggers']['gunicorn.error']['handlers'] = ['file'] - logconfig_dict['handlers']['file'] = { - 'class': 'logging.handlers.RotatingFileHandler', - 'filename': '/var/lib/pgadmin/logs/gunicorn.log', - 'backupCount': 1, 'maxBytes': 2 << 20, # MiB - 'formatter': 'json', - } - logconfig_dict['formatters']['json'] = { - 'class': 'jsonformatter.JsonFormatter', - 'separators': (',', ':'), - 'format': collections.OrderedDict([ - ('time', 'created'), - ('name', 'name'), - ('level', 'levelname'), - ('message', 'message'), - ]) - } name: pgadmin-startup resources: {} securityContext: @@ -230,9 +200,6 @@ volumes: medium: Memory sizeLimit: 32Ki name: pgadmin-config-system -- emptyDir: - medium: Memory - name: tmp `)) // No change when called again. @@ -247,6 +214,20 @@ volumes: pgadmin.Spec.Resources.Requests = corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("100m"), } + retentionPeriod, err := v1beta1.NewDuration("12 hours") + assert.NilError(t, err) + pgadmin.Spec.Instrumentation = &v1beta1.InstrumentationSpec{ + Logs: &v1beta1.InstrumentationLogsSpec{ + RetentionPeriod: retentionPeriod, + }, + } + + // Turn on the Feature gate + gate := feature.NewGate() + assert.NilError(t, gate.SetFromMap(map[string]bool{ + feature.OpenTelemetryLogs: true, + })) + ctx = feature.NewContext(context.Background(), gate) call() @@ -349,8 +330,6 @@ containers: - mountPath: /etc/pgadmin name: pgadmin-config-system readOnly: true - - mountPath: /tmp - name: tmp initContainers: - command: - bash @@ -358,7 +337,6 @@ initContainers: - -- - |- mkdir -p '/etc/pgadmin/conf.d' && chmod 0775 '/etc/pgadmin/conf.d' - mkdir -p '/var/lib/pgadmin/logs/receiver' && chmod 0775 '/var/lib/pgadmin/logs/receiver' '/var/lib/pgadmin/logs' echo "$1" > /etc/pgadmin/config_system.py echo "$2" > /etc/pgadmin/gunicorn_config.py - startup @@ -378,29 +356,33 @@ initContainers: DATA_DIR = '/var/lib/pgadmin' LOG_FILE = '/var/lib/pgadmin/logs/pgadmin.log' - LOG_ROTATION_AGE = 24 * 60 # minutes + + LOG_ROTATION_AGE = 60 # minutes LOG_ROTATION_SIZE = 5 # MiB - LOG_ROTATION_MAX_LOG_FILES = 1 + LOG_ROTATION_MAX_LOG_FILES = 11 JSON_LOGGER = True CONSOLE_LOG_LEVEL = logging.WARNING FILE_LOG_LEVEL = logging.INFO FILE_LOG_FORMAT_JSON = {'time': 'created', 'name': 'name', 'level': 'levelname', 'message': 'message'} - | - import json, re, collections, copy, gunicorn, gunicorn.glogging + import json, re with open('/etc/pgadmin/conf.d/~postgres-operator/gunicorn-config.json') as _f: _conf, _data = re.compile(r'[a-z_]+'), json.load(_f) if type(_data) is dict: globals().update({k: v for k, v in _data.items() if _conf.fullmatch(k)}) + import collections, copy, gunicorn, gunicorn.glogging gunicorn.SERVER_SOFTWARE = 'Python' logconfig_dict = copy.deepcopy(gunicorn.glogging.CONFIG_DEFAULTS) logconfig_dict['loggers']['gunicorn.access']['handlers'] = ['file'] logconfig_dict['loggers']['gunicorn.error']['handlers'] = ['file'] logconfig_dict['handlers']['file'] = { - 'class': 'logging.handlers.RotatingFileHandler', + 'class': 'logging.handlers.TimedRotatingFileHandler', 'filename': '/var/lib/pgadmin/logs/gunicorn.log', - 'backupCount': 1, 'maxBytes': 2 << 20, # MiB + 'backupCount': 11, + 'interval': 1, # every one unit (defined by when), rotate + 'when': 'H', 'formatter': 'json', } logconfig_dict['formatters']['json'] = { @@ -453,9 +435,6 @@ volumes: medium: Memory sizeLimit: 32Ki name: pgadmin-config-system -- emptyDir: - medium: Memory - name: tmp `)) }) } diff --git a/internal/controller/standalone_pgadmin/statefulset.go b/internal/controller/standalone_pgadmin/statefulset.go index 2c9a17595d..c4958e9ba9 100644 --- a/internal/controller/standalone_pgadmin/statefulset.go +++ b/internal/controller/standalone_pgadmin/statefulset.go @@ -16,6 +16,7 @@ import ( "github.com/pkg/errors" "github.com/crunchydata/postgres-operator/internal/collector" + "github.com/crunchydata/postgres-operator/internal/controller/postgrescluster" "github.com/crunchydata/postgres-operator/internal/feature" "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/naming" @@ -119,10 +120,11 @@ func statefulset( sts.Spec.Template.Spec.SecurityContext = podSecurityContext(ctx) - pod(pgadmin, configmap, &sts.Spec.Template.Spec, dataVolume) + pod(ctx, pgadmin, configmap, &sts.Spec.Template.Spec, dataVolume) - if feature.Enabled(ctx, feature.OpenTelemetryLogs) { + if pgadmin.Spec.Instrumentation != nil && feature.Enabled(ctx, feature.OpenTelemetryLogs) { // Logs for gunicorn and pgadmin write to /var/lib/pgadmin/logs + // so the collector needs access to that that path. dataVolumeMount := corev1.VolumeMount{ Name: "pgadmin-data", MountPath: "/var/lib/pgadmin", @@ -132,8 +134,10 @@ func statefulset( } collector.AddToPod(ctx, pgadmin.Spec.Instrumentation, pgadmin.Spec.ImagePullPolicy, - configmap, &sts.Spec.Template.Spec, volumeMounts, "", []string{}, false) + configmap, &sts.Spec.Template.Spec, volumeMounts, "", []string{LogDirectoryAbsolutePath}, false) } + postgrescluster.AddTMPEmptyDir(&sts.Spec.Template) + return sts } From 2e8593b2a3be3e51b5e82892ed2bff807e07fd2b Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Mon, 3 Mar 2025 11:08:13 -0600 Subject: [PATCH 2/9] PR feedback, v1 --- internal/controller/standalone_pgadmin/pod.go | 7 ++++++- internal/controller/standalone_pgadmin/pod_test.go | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/controller/standalone_pgadmin/pod.go b/internal/controller/standalone_pgadmin/pod.go index 9cb6de0e42..c7273509e9 100644 --- a/internal/controller/standalone_pgadmin/pod.go +++ b/internal/controller/standalone_pgadmin/pod.go @@ -435,7 +435,6 @@ with open('` + configMountPath + `/` + gunicornConfigFilePath + `') as _f: configSystem = configSystem + ` LOG_ROTATION_AGE = ` + pgAdminRetentionPeriod + ` # minutes -LOG_ROTATION_SIZE = 5 # MiB LOG_ROTATION_MAX_LOG_FILES = ` + maxBackupRetentionNumber + ` JSON_LOGGER = True @@ -447,6 +446,9 @@ FILE_LOG_FORMAT_JSON = {'time': 'created', 'name': 'name', 'level': 'levelname', // Gunicorn uses the Python logging package, which sets the following attributes: // https://docs.python.org/3/library/logging.html#logrecord-attributes. // JsonFormatter is used to format the log: https://pypi.org/project/jsonformatter/ + // We override the gunicorn defaults (using `logconfig_dict`) to set our own file handler. + // - https://docs.gunicorn.org/en/stable/settings.html#logconfig-dict + // - https://github.com/benoitc/gunicorn/blob/23.0.0/gunicorn/glogging.py#L47 gunicornConfig = gunicornConfig + ` import collections, copy, gunicorn, gunicorn.glogging gunicorn.SERVER_SOFTWARE = 'Python' @@ -481,6 +483,9 @@ logconfig_dict['formatters']['json'] = { // - https://issue.k8s.io/121294 shell.MakeDirectories(0o775, scriptMountPath, configMountPath), + // Create the logs directory with g+rwx to ensure pgAdmin can write to it as well. + shell.MakeDirectories(0o775, dataMountPath, LogDirectoryAbsolutePath), + // Write the system and server configurations. `echo "$1" > ` + scriptMountPath + `/config_system.py`, `echo "$2" > ` + scriptMountPath + `/gunicorn_config.py`, diff --git a/internal/controller/standalone_pgadmin/pod_test.go b/internal/controller/standalone_pgadmin/pod_test.go index d87f916b47..bc7365eb6e 100644 --- a/internal/controller/standalone_pgadmin/pod_test.go +++ b/internal/controller/standalone_pgadmin/pod_test.go @@ -139,6 +139,7 @@ initContainers: - -- - |- mkdir -p '/etc/pgadmin/conf.d' && chmod 0775 '/etc/pgadmin/conf.d' + mkdir -p '/var/lib/pgadmin/logs' && chmod 0775 '/var/lib/pgadmin/logs' echo "$1" > /etc/pgadmin/config_system.py echo "$2" > /etc/pgadmin/gunicorn_config.py - startup @@ -337,6 +338,7 @@ initContainers: - -- - |- mkdir -p '/etc/pgadmin/conf.d' && chmod 0775 '/etc/pgadmin/conf.d' + mkdir -p '/var/lib/pgadmin/logs' && chmod 0775 '/var/lib/pgadmin/logs' echo "$1" > /etc/pgadmin/config_system.py echo "$2" > /etc/pgadmin/gunicorn_config.py - startup @@ -358,7 +360,6 @@ initContainers: LOG_FILE = '/var/lib/pgadmin/logs/pgadmin.log' LOG_ROTATION_AGE = 60 # minutes - LOG_ROTATION_SIZE = 5 # MiB LOG_ROTATION_MAX_LOG_FILES = 11 JSON_LOGGER = True From 9a460762ffd7b4812879aaeec0233201125002b1 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Mon, 3 Mar 2025 21:16:08 -0600 Subject: [PATCH 3/9] Move config to configmap --- .../controller/standalone_pgadmin/config.go | 7 +- .../standalone_pgadmin/configmap.go | 166 +++++++++++++++++- .../standalone_pgadmin/configmap_test.go | 154 +++++++++++++++- internal/controller/standalone_pgadmin/pod.go | 107 ++--------- .../controller/standalone_pgadmin/pod_test.go | 62 ++----- 5 files changed, 341 insertions(+), 155 deletions(-) diff --git a/internal/controller/standalone_pgadmin/config.go b/internal/controller/standalone_pgadmin/config.go index 3af09144f2..8ad86ad1ab 100644 --- a/internal/controller/standalone_pgadmin/config.go +++ b/internal/controller/standalone_pgadmin/config.go @@ -7,9 +7,10 @@ package standalone_pgadmin // Include configs here used by multiple files const ( // ConfigMap keys used also in mounting volume to pod - settingsConfigMapKey = "pgadmin-settings.json" - settingsClusterMapKey = "pgadmin-shared-clusters.json" - gunicornConfigKey = "gunicorn-config.json" + settingsConfigMapKey = "pgadmin-settings.json" + settingsClusterMapKey = "pgadmin-shared-clusters.json" + gunicornLoggingConfigKey = "gunicorn-logging-config.json" + gunicornConfigKey = "gunicorn-config.json" // Port address used to define pod and service pgAdminPort = 5050 diff --git a/internal/controller/standalone_pgadmin/configmap.go b/internal/controller/standalone_pgadmin/configmap.go index 8382bbb2ca..79d4611c73 100644 --- a/internal/controller/standalone_pgadmin/configmap.go +++ b/internal/controller/standalone_pgadmin/configmap.go @@ -19,6 +19,7 @@ import ( "github.com/pkg/errors" "github.com/crunchydata/postgres-operator/internal/collector" + "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/pkg/apis/postgres-operator.crunchydata.com/v1beta1" @@ -32,7 +33,7 @@ func (r *PGAdminReconciler) reconcilePGAdminConfigMap( ctx context.Context, pgadmin *v1beta1.PGAdmin, clusters map[string][]*v1beta1.PostgresCluster, ) (*corev1.ConfigMap, error) { - configmap, err := configmap(pgadmin, clusters) + configmap, err := configmap(ctx, pgadmin, clusters) if err != nil { return configmap, err } @@ -50,7 +51,7 @@ func (r *PGAdminReconciler) reconcilePGAdminConfigMap( } // configmap returns a v1.ConfigMap for pgAdmin. -func configmap(pgadmin *v1beta1.PGAdmin, +func configmap(ctx context.Context, pgadmin *v1beta1.PGAdmin, clusters map[string][]*v1beta1.PostgresCluster, ) (*corev1.ConfigMap, error) { configmap := &corev1.ConfigMap{ObjectMeta: naming.StandalonePGAdmin(pgadmin)} @@ -63,9 +64,9 @@ func configmap(pgadmin *v1beta1.PGAdmin, // TODO(tjmoore4): Populate configuration details. initialize.Map(&configmap.Data) - configSettings, err := generateConfig(pgadmin) + pgadminConfigSettings, err := generateConfig(ctx, pgadmin) if err == nil { - configmap.Data[settingsConfigMapKey] = configSettings + configmap.Data[settingsConfigMapKey] = pgadminConfigSettings } clusterSettings, err := generateClusterConfig(clusters) @@ -73,16 +74,19 @@ func configmap(pgadmin *v1beta1.PGAdmin, configmap.Data[settingsClusterMapKey] = clusterSettings } - gunicornSettings, err := generateGunicornConfig(pgadmin) + gunicornSettings, gunicornLoggingSettings, err := generateGunicornConfig(ctx, pgadmin) if err == nil { configmap.Data[gunicornConfigKey] = gunicornSettings + configmap.Data[gunicornLoggingConfigKey] = gunicornLoggingSettings } return configmap, err } -// generateConfig generates the config settings for the pgAdmin -func generateConfig(pgadmin *v1beta1.PGAdmin) (string, error) { +// generateConfigs generates the config settings for the pgAdmin and gunicorn +func generateConfig(ctx context.Context, pgadmin *v1beta1.PGAdmin) ( + string, error, +) { settings := map[string]any{ // Bind to all IPv4 addresses by default. "0.0.0.0" here represents INADDR_ANY. // - https://flask.palletsprojects.com/en/2.2.x/api/#flask.Flask.run @@ -102,6 +106,48 @@ func generateConfig(pgadmin *v1beta1.PGAdmin) (string, error) { settings["UPGRADE_CHECK_ENABLED"] = false settings["UPGRADE_CHECK_URL"] = "" settings["UPGRADE_CHECK_KEY"] = "" + settings["DATA_DIR"] = dataMountPath + settings["LOG_FILE"] = LogFileAbsolutePath + + // If OTel logs feature gate is enabled, we want to change the pgAdmin/gunicorn logging + if feature.Enabled(ctx, feature.OpenTelemetryLogs) && pgadmin.Spec.Instrumentation != nil { + + var ( + maxBackupRetentionNumber = 1 + // One day in minutes for pgadmin rotation + pgAdminRetentionPeriod = 24 * 60 + ) + + // If the user has set a retention period, we will use those values for log rotation, + // which is otherwise managed by python. + if pgadmin.Spec.Instrumentation.Logs != nil && + pgadmin.Spec.Instrumentation.Logs.RetentionPeriod != nil { + + retentionNumber, period := collector.ParseDurationForLogrotate(pgadmin.Spec.Instrumentation.Logs.RetentionPeriod.AsDuration()) + // `LOG_ROTATION_MAX_LOG_FILES`` in pgadmin refers to the already rotated logs. + // `backupCount` for gunicorn is similar. + // Our retention unit is for total number of log files, so subtract 1 to account + // for the currently-used log file. + maxBackupRetentionNumber = retentionNumber - 1 + if period == "hourly" { + // If the period is hourly, set the pgadmin + // and gunicorn retention periods to hourly. + pgAdminRetentionPeriod = 60 + } + } + + settings["LOG_ROTATION_AGE"] = pgAdminRetentionPeriod + settings["LOG_ROTATION_MAX_LOG_FILES"] = maxBackupRetentionNumber + settings["JSON_LOGGER"] = true + settings["CONSOLE_LOG_LEVEL"] = "WARNING" + settings["FILE_LOG_LEVEL"] = "INFO" + settings["FILE_LOG_FORMAT_JSON"] = map[string]string{ + "time": "created", + "name": "name", + "level": "levelname", + "message": "message", + } + } // To avoid spurious reconciles, the following value must not change when // the spec does not change. [json.Encoder] and [json.Marshal] do this by @@ -185,7 +231,9 @@ func generateClusterConfig( // generateGunicornConfig generates the config settings for the gunicorn server // - https://docs.gunicorn.org/en/latest/settings.html -func generateGunicornConfig(pgadmin *v1beta1.PGAdmin) (string, error) { +func generateGunicornConfig(ctx context.Context, pgadmin *v1beta1.PGAdmin) ( + string, string, error, +) { settings := map[string]any{ // Bind to all IPv4 addresses and set 25 threads by default. // - https://docs.gunicorn.org/en/latest/settings.html#bind @@ -213,5 +261,105 @@ func generateGunicornConfig(pgadmin *v1beta1.PGAdmin) (string, error) { encoder.SetIndent("", " ") err := encoder.Encode(settings) - return buffer.String(), err + // Gunicorn logging dict settings + logSettings := map[string]any{} + + // If OTel logs feature gate is enabled, we want to change the pgAdmin/gunicorn logging + if feature.Enabled(ctx, feature.OpenTelemetryLogs) && + pgadmin.Spec.Instrumentation != nil { + + var ( + maxBackupRetentionNumber = "1" + // Daily rotation for gunicorn rotation + gunicornRetentionPeriod = "D" + ) + + // If the user has set a retention period, we will use those values for log rotation, + // which is otherwise managed by python. + if pgadmin.Spec.Instrumentation.Logs != nil && + pgadmin.Spec.Instrumentation.Logs.RetentionPeriod != nil { + + retentionNumber, period := collector.ParseDurationForLogrotate(pgadmin.Spec.Instrumentation.Logs.RetentionPeriod.AsDuration()) + // `LOG_ROTATION_MAX_LOG_FILES`` in pgadmin refers to the already rotated logs. + // `backupCount` for gunicorn is similar. + // Our retention unit is for total number of log files, so subtract 1 to account + // for the currently-used log file. + maxBackupRetentionNumber = strconv.Itoa(retentionNumber - 1) + if period == "hourly" { + // If the period is hourly, set the pgadmin + // and gunicorn retention periods to hourly. + gunicornRetentionPeriod = "H" + } + } + + // Gunicorn uses the Python logging package, which sets the following attributes: + // https://docs.python.org/3/library/logging.html#logrecord-attributes. + // JsonFormatter is used to format the log: https://pypi.org/project/jsonformatter/ + // We override the gunicorn defaults (using `logconfig_dict`) to set our own file handler. + // - https://docs.gunicorn.org/en/stable/settings.html#logconfig-dict + // - https://github.com/benoitc/gunicorn/blob/23.0.0/gunicorn/glogging.py#L47 + logSettings = map[string]any{ + + "loggers": map[string]any{ + "gunicorn.access": map[string]any{ + "handlers": []string{"file"}, + "level": "INFO", + "propagate": true, + "qualname": "gunicorn.access", + }, + "gunicorn.error": map[string]any{ + "handlers": []string{"file"}, + "level": "INFO", + "propagate": true, + "qualname": "gunicorn.error", + }, + }, + "handlers": map[string]any{ + "file": map[string]any{ + "class": "logging.handlers.TimedRotatingFileHandler", + "filename": GunicornLogFileAbsolutePath, + "backupCount": maxBackupRetentionNumber, + "interval": 1, + "when": gunicornRetentionPeriod, + "formatter": "json", + }, + "console": map[string]any{ + "class": "logging.StreamHandler", + "formatter": "generic", + "stream": "ext://sys.stdout", + }, + }, + "formatters": map[string]any{ + "generic": map[string]any{ + "class": "logging.Formatter", + "datefmt": "[%Y-%m-%d %H:%M:%S %z]", + "format": "%(asctime)s [%(process)d] [%(levelname)s] %(message)s", + }, + "json": map[string]any{ + "class": "jsonformatter.JsonFormatter", + "separators": []string{",", ":"}, + "format": map[string]string{ + "time": "created", + "name": "name", + "level": "levelname", + "message": "message", + }, + }, + }, + } + } + + // To avoid spurious reconciles, the following value must not change when + // the spec does not change. [json.Encoder] and [json.Marshal] do this by + // emitting map keys in sorted order. Indent so the value is not rendered + // as one long line by `kubectl`. + logBuffer := new(bytes.Buffer) + logEncoder := json.NewEncoder(logBuffer) + logEncoder.SetEscapeHTML(false) + logEncoder.SetIndent("", " ") + + // Combine errors + err = logEncoder.Encode(logSettings) + + return buffer.String(), logBuffer.String(), err } diff --git a/internal/controller/standalone_pgadmin/configmap_test.go b/internal/controller/standalone_pgadmin/configmap_test.go index b2a93ac2de..8b45589002 100644 --- a/internal/controller/standalone_pgadmin/configmap_test.go +++ b/internal/controller/standalone_pgadmin/configmap_test.go @@ -5,10 +5,13 @@ package standalone_pgadmin import ( + "context" + "strings" "testing" "gotest.tools/v3/assert" + "github.com/crunchydata/postgres-operator/internal/feature" "github.com/crunchydata/postgres-operator/internal/testing/cmp" "github.com/crunchydata/postgres-operator/internal/testing/require" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" @@ -16,14 +19,17 @@ import ( func TestGenerateConfig(t *testing.T) { require.ParallelCapacity(t, 0) + ctx := context.Background() t.Run("Default", func(t *testing.T) { pgadmin := new(v1beta1.PGAdmin) - result, err := generateConfig(pgadmin) + result, err := generateConfig(ctx, pgadmin) assert.NilError(t, err) assert.Equal(t, result, `{ + "DATA_DIR": "/var/lib/pgadmin", "DEFAULT_SERVER": "0.0.0.0", + "LOG_FILE": "/var/lib/pgadmin/logs/pgadmin.log", "SERVER_MODE": true, "UPGRADE_CHECK_ENABLED": false, "UPGRADE_CHECK_KEY": "", @@ -37,11 +43,13 @@ func TestGenerateConfig(t *testing.T) { "SERVER_MODE": false, "UPGRADE_CHECK_ENABLED": true, } - result, err := generateConfig(pgadmin) + result, err := generateConfig(ctx, pgadmin) assert.NilError(t, err) assert.Equal(t, result, `{ + "DATA_DIR": "/var/lib/pgadmin", "DEFAULT_SERVER": "0.0.0.0", + "LOG_FILE": "/var/lib/pgadmin/logs/pgadmin.log", "SERVER_MODE": true, "UPGRADE_CHECK_ENABLED": false, "UPGRADE_CHECK_KEY": "", @@ -55,7 +63,7 @@ func TestGenerateConfig(t *testing.T) { "ALLOWED_HOSTS": []any{"225.0.0.0/8", "226.0.0.0/7", "228.0.0.0/6"}, "DEFAULT_SERVER": "::", } - result, err := generateConfig(pgadmin) + result, err := generateConfig(ctx, pgadmin) assert.NilError(t, err) assert.Equal(t, result, `{ @@ -64,7 +72,46 @@ func TestGenerateConfig(t *testing.T) { "226.0.0.0/7", "228.0.0.0/6" ], + "DATA_DIR": "/var/lib/pgadmin", "DEFAULT_SERVER": "::", + "LOG_FILE": "/var/lib/pgadmin/logs/pgadmin.log", + "SERVER_MODE": true, + "UPGRADE_CHECK_ENABLED": false, + "UPGRADE_CHECK_KEY": "", + "UPGRADE_CHECK_URL": "" +}`+"\n") + }) + + t.Run("OTel enabled", func(t *testing.T) { + gate := feature.NewGate() + assert.NilError(t, gate.SetFromMap(map[string]bool{ + feature.OpenTelemetryLogs: true, + })) + ctx := feature.NewContext(context.Background(), gate) + pgadmin := new(v1beta1.PGAdmin) + require.UnmarshalInto(t, &pgadmin.Spec, `{ + instrumentation: { + logs: { retentionPeriod: 5h }, + }, + }`) + result, err := generateConfig(ctx, pgadmin) + + assert.NilError(t, err) + assert.Equal(t, result, `{ + "CONSOLE_LOG_LEVEL": "WARNING", + "DATA_DIR": "/var/lib/pgadmin", + "DEFAULT_SERVER": "0.0.0.0", + "FILE_LOG_FORMAT_JSON": { + "level": "levelname", + "message": "message", + "name": "name", + "time": "created" + }, + "FILE_LOG_LEVEL": "INFO", + "JSON_LOGGER": true, + "LOG_FILE": "/var/lib/pgadmin/logs/pgadmin.log", + "LOG_ROTATION_AGE": 60, + "LOG_ROTATION_MAX_LOG_FILES": 4, "SERVER_MODE": true, "UPGRADE_CHECK_ENABLED": false, "UPGRADE_CHECK_KEY": "", @@ -161,10 +208,11 @@ func TestGeneratePGAdminConfigMap(t *testing.T) { pgadmin.Namespace = "some-ns" pgadmin.Name = "pg1" clusters := map[string][]*v1beta1.PostgresCluster{} + ctx := context.Background() t.Run("Data,ObjectMeta,TypeMeta", func(t *testing.T) { pgadmin := pgadmin.DeepCopy() - configmap, err := configmap(pgadmin, clusters) + configmap, err := configmap(ctx, pgadmin, clusters) assert.NilError(t, err) assert.Assert(t, cmp.MarshalMatches(configmap.TypeMeta, ` @@ -190,7 +238,7 @@ namespace: some-ns Labels: map[string]string{"c": "v3", "d": "v4"}, } - configmap, err := configmap(pgadmin, clusters) + configmap, err := configmap(ctx, pgadmin, clusters) assert.NilError(t, err) // Annotations present in the metadata. @@ -209,6 +257,7 @@ namespace: some-ns func TestGenerateGunicornConfig(t *testing.T) { require.ParallelCapacity(t, 0) + ctx := context.Background() t.Run("Default", func(t *testing.T) { pgAdmin := &v1beta1.PGAdmin{} @@ -221,9 +270,10 @@ func TestGenerateGunicornConfig(t *testing.T) { "workers": 1 } ` - actualString, err := generateGunicornConfig(pgAdmin) + actualString, logString, err := generateGunicornConfig(ctx, pgAdmin) assert.NilError(t, err) assert.Equal(t, actualString, expectedString) + assert.Assert(t, strings.Contains(logString, "{}")) }) t.Run("Add Settings", func(t *testing.T) { @@ -243,9 +293,10 @@ func TestGenerateGunicornConfig(t *testing.T) { "workers": 1 } ` - actualString, err := generateGunicornConfig(pgAdmin) + actualString, logString, err := generateGunicornConfig(ctx, pgAdmin) assert.NilError(t, err) assert.Equal(t, actualString, expectedString) + assert.Assert(t, strings.Contains(logString, "{}")) }) t.Run("Update Defaults", func(t *testing.T) { @@ -263,9 +314,10 @@ func TestGenerateGunicornConfig(t *testing.T) { "workers": 1 } ` - actualString, err := generateGunicornConfig(pgAdmin) + actualString, logString, err := generateGunicornConfig(ctx, pgAdmin) assert.NilError(t, err) assert.Equal(t, actualString, expectedString) + assert.Assert(t, strings.Contains(logString, "{}")) }) t.Run("Update Mandatory", func(t *testing.T) { @@ -282,9 +334,93 @@ func TestGenerateGunicornConfig(t *testing.T) { "workers": 1 } ` - actualString, err := generateGunicornConfig(pgAdmin) + actualString, logString, err := generateGunicornConfig(ctx, pgAdmin) + assert.NilError(t, err) + assert.Equal(t, actualString, expectedString) + assert.Assert(t, strings.Contains(logString, "{}")) + }) + + t.Run("OTel enabled", func(t *testing.T) { + gate := feature.NewGate() + assert.NilError(t, gate.SetFromMap(map[string]bool{ + feature.OpenTelemetryLogs: true, + })) + ctx := feature.NewContext(context.Background(), gate) + pgAdmin := &v1beta1.PGAdmin{} + pgAdmin.Name = "test" + pgAdmin.Namespace = "postgres-operator" + require.UnmarshalInto(t, &pgAdmin.Spec, `{ + instrumentation: { + logs: { retentionPeriod: 5h }, + }, + }`) + actualString, logString, err := generateGunicornConfig(ctx, pgAdmin) + + expectedString := `{ + "bind": "0.0.0.0:5050", + "threads": 25, + "workers": 1 +} +` + expectedLogString := `{ + "formatters": { + "generic": { + "class": "logging.Formatter", + "datefmt": "[%Y-%m-%d %H:%M:%S %z]", + "format": "%(asctime)s [%(process)d] [%(levelname)s] %(message)s" + }, + "json": { + "class": "jsonformatter.JsonFormatter", + "format": { + "level": "levelname", + "message": "message", + "name": "name", + "time": "created" + }, + "separators": [ + ",", + ":" + ] + } + }, + "handlers": { + "console": { + "class": "logging.StreamHandler", + "formatter": "generic", + "stream": "ext://sys.stdout" + }, + "file": { + "backupCount": "4", + "class": "logging.handlers.TimedRotatingFileHandler", + "filename": "/var/lib/pgadmin/logs/gunicorn.log", + "formatter": "json", + "interval": 1, + "when": "H" + } + }, + "loggers": { + "gunicorn.access": { + "handlers": [ + "file" + ], + "level": "INFO", + "propagate": true, + "qualname": "gunicorn.access" + }, + "gunicorn.error": { + "handlers": [ + "file" + ], + "level": "INFO", + "propagate": true, + "qualname": "gunicorn.error" + } + } +} +` assert.NilError(t, err) assert.Equal(t, actualString, expectedString) + assert.Equal(t, logString, expectedLogString) }) } diff --git a/internal/controller/standalone_pgadmin/pod.go b/internal/controller/standalone_pgadmin/pod.go index c7273509e9..f743c04991 100644 --- a/internal/controller/standalone_pgadmin/pod.go +++ b/internal/controller/standalone_pgadmin/pod.go @@ -7,16 +7,13 @@ package standalone_pgadmin import ( "context" "fmt" - "strconv" "strings" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/util/intstr" - "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/kubernetes" "github.com/crunchydata/postgres-operator/internal/naming" @@ -25,12 +22,13 @@ import ( ) const ( - configMountPath = "/etc/pgadmin/conf.d" - configFilePath = "~postgres-operator/" + settingsConfigMapKey - clusterFilePath = "~postgres-operator/" + settingsClusterMapKey - configDatabaseURIPath = "~postgres-operator/config-database-uri" - ldapFilePath = "~postgres-operator/ldap-bind-password" - gunicornConfigFilePath = "~postgres-operator/" + gunicornConfigKey + configMountPath = "/etc/pgadmin/conf.d" + configFilePath = "~postgres-operator/" + settingsConfigMapKey + clusterFilePath = "~postgres-operator/" + settingsClusterMapKey + configDatabaseURIPath = "~postgres-operator/config-database-uri" + ldapFilePath = "~postgres-operator/ldap-bind-password" + gunicornConfigFilePath = "~postgres-operator/" + gunicornConfigKey + gunicornLogConfFilePath = "~postgres-operator/" + gunicornLoggingConfigKey // scriptMountPath is where to mount a temporary directory that is only // writable during Pod initialization. @@ -211,6 +209,10 @@ func podConfigFiles(configmap *corev1.ConfigMap, pgadmin v1beta1.PGAdmin) []core Key: gunicornConfigKey, Path: gunicornConfigFilePath, }, + { + Key: gunicornLoggingConfigKey, + Path: gunicornLogConfFilePath, + }, }, }, }, @@ -266,7 +268,11 @@ func startupScript(pgadmin *v1beta1.PGAdmin) []string { // startCommands (v8 image includes Gunicorn) var startCommandV7 = "pgadmin4 &" - var startCommandV8 = "gunicorn -c /etc/pgadmin/gunicorn_config.py --chdir $PGADMIN_DIR pgAdmin4:app &" + // For Gunicorn, watch the logging config and reload if changes are detected. + var startCommandV8 = "gunicorn -c /etc/pgadmin/gunicorn_config.py" + + " --reload-extra-file " + configMountPath + `/` + gunicornLogConfFilePath + + " --log-config-json " + configMountPath + `/` + gunicornLogConfFilePath + + " --chdir $PGADMIN_DIR pgAdmin4:app &" // This script sets up, starts pgadmin, and runs the appropriate `loadServerCommand` to register the discovered servers. // pgAdmin is hosted by Gunicorn and uses a config file. @@ -368,7 +374,7 @@ func startupCommand(ctx context.Context, inPgadmin *v1beta1.PGAdmin) []string { // The values set in configSystem will not be overridden through // spec.config.settings. var configSystem = ` -import glob, json, re, os, logging +import glob, json, re, os DEFAULT_BINARY_PATHS = {'pg': sorted([''] + glob.glob('/usr/pgsql-*/bin')).pop()} with open('` + configMountPath + `/` + configFilePath + `') as _f: _conf, _data = re.compile(r'[A-Z_0-9]+'), json.load(_f) @@ -380,9 +386,6 @@ if os.path.isfile('` + ldapPasswordAbsolutePath + `'): if os.path.isfile('` + configDatabaseURIPathAbsolutePath + `'): with open('` + configDatabaseURIPathAbsolutePath + `') as _f: CONFIG_DATABASE_URI = _f.read() - -DATA_DIR = '` + dataMountPath + `' -LOG_FILE = '` + LogFileAbsolutePath + `' ` // Gunicorn reads from the `/etc/pgadmin/gunicorn_config.py` file during startup @@ -396,86 +399,14 @@ LOG_FILE = '` + LogFileAbsolutePath + `' // Note: All Gunicorn settings are lowercase with underscores, so ignore // any keys/names that are not. var gunicornConfig = ` -import json, re +import json, re, gunicorn +gunicorn.SERVER_SOFTWARE = 'Python' with open('` + configMountPath + `/` + gunicornConfigFilePath + `') as _f: _conf, _data = re.compile(r'[a-z_]+'), json.load(_f) if type(_data) is dict: globals().update({k: v for k, v in _data.items() if _conf.fullmatch(k)}) ` - // If OTel logs feature gate is enabled, we want to change the pgAdmin/gunicorn logging - if feature.Enabled(ctx, feature.OpenTelemetryLogs) && inPgadmin.Spec.Instrumentation != nil { - - var ( - maxBackupRetentionNumber = "1" - // One day in minutes for pgadmin rotation - pgAdminRetentionPeriod = "24 * 60" - // Daily rotation for gunicorn rotation - gunicornRetentionPeriod = "D" - ) - - // If the user has set a retention period, we will use those values for log rotation, - // which is otherwise managed by python. - if inPgadmin.Spec.Instrumentation.Logs != nil && - inPgadmin.Spec.Instrumentation.Logs.RetentionPeriod != nil { - - retentionNumber, period := collector.ParseDurationForLogrotate(inPgadmin.Spec.Instrumentation.Logs.RetentionPeriod.AsDuration()) - // `LOG_ROTATION_MAX_LOG_FILES`` in pgadmin refers to the already rotated logs. - // `backupCount` for gunicorn is similar. - // Our retention unit is for total number of log files, so subtract 1 to account - // for the currently-used log file. - maxBackupRetentionNumber = strconv.Itoa(retentionNumber - 1) - if period == "hourly" { - // If the period is hourly, set the pgadmin - // and gunicorn retention periods to hourly. - pgAdminRetentionPeriod = "60" - gunicornRetentionPeriod = "H" - } - } - - configSystem = configSystem + ` -LOG_ROTATION_AGE = ` + pgAdminRetentionPeriod + ` # minutes -LOG_ROTATION_MAX_LOG_FILES = ` + maxBackupRetentionNumber + ` - -JSON_LOGGER = True -CONSOLE_LOG_LEVEL = logging.WARNING -FILE_LOG_LEVEL = logging.INFO -FILE_LOG_FORMAT_JSON = {'time': 'created', 'name': 'name', 'level': 'levelname', 'message': 'message'} -` - - // Gunicorn uses the Python logging package, which sets the following attributes: - // https://docs.python.org/3/library/logging.html#logrecord-attributes. - // JsonFormatter is used to format the log: https://pypi.org/project/jsonformatter/ - // We override the gunicorn defaults (using `logconfig_dict`) to set our own file handler. - // - https://docs.gunicorn.org/en/stable/settings.html#logconfig-dict - // - https://github.com/benoitc/gunicorn/blob/23.0.0/gunicorn/glogging.py#L47 - gunicornConfig = gunicornConfig + ` -import collections, copy, gunicorn, gunicorn.glogging -gunicorn.SERVER_SOFTWARE = 'Python' -logconfig_dict = copy.deepcopy(gunicorn.glogging.CONFIG_DEFAULTS) -logconfig_dict['loggers']['gunicorn.access']['handlers'] = ['file'] -logconfig_dict['loggers']['gunicorn.error']['handlers'] = ['file'] -logconfig_dict['handlers']['file'] = { - 'class': 'logging.handlers.TimedRotatingFileHandler', - 'filename': '` + GunicornLogFileAbsolutePath + `', - 'backupCount': ` + maxBackupRetentionNumber + `, - 'interval': 1, # every one unit (defined by when), rotate - 'when': '` + gunicornRetentionPeriod + `', - 'formatter': 'json', -} -logconfig_dict['formatters']['json'] = { - 'class': 'jsonformatter.JsonFormatter', - 'separators': (',', ':'), - 'format': collections.OrderedDict([ - ('time', 'created'), - ('name', 'name'), - ('level', 'levelname'), - ('message', 'message'), - ]) -} -` - } - args := []string{strings.TrimLeft(configSystem, "\n"), strings.TrimLeft(gunicornConfig, "\n")} script := strings.Join([]string{ diff --git a/internal/controller/standalone_pgadmin/pod_test.go b/internal/controller/standalone_pgadmin/pod_test.go index bc7365eb6e..64c62b1886 100644 --- a/internal/controller/standalone_pgadmin/pod_test.go +++ b/internal/controller/standalone_pgadmin/pod_test.go @@ -61,7 +61,7 @@ containers: if [ $APP_RELEASE -eq 7 ]; then pgadmin4 & else - gunicorn -c /etc/pgadmin/gunicorn_config.py --chdir $PGADMIN_DIR pgAdmin4:app & + gunicorn -c /etc/pgadmin/gunicorn_config.py --reload-extra-file /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --log-config-json /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --chdir $PGADMIN_DIR pgAdmin4:app & fi echo $! > $PGADMIN4_PIDFILE @@ -86,7 +86,7 @@ containers: if [[ $APP_RELEASE -eq 7 ]]; then pgadmin4 & else - gunicorn -c /etc/pgadmin/gunicorn_config.py --chdir $PGADMIN_DIR pgAdmin4:app & + gunicorn -c /etc/pgadmin/gunicorn_config.py --reload-extra-file /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --log-config-json /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --chdir $PGADMIN_DIR pgAdmin4:app & fi echo $! > $PGADMIN4_PIDFILE echo "Restarting pgAdmin4" @@ -144,7 +144,7 @@ initContainers: echo "$2" > /etc/pgadmin/gunicorn_config.py - startup - | - import glob, json, re, os, logging + import glob, json, re, os DEFAULT_BINARY_PATHS = {'pg': sorted([''] + glob.glob('/usr/pgsql-*/bin')).pop()} with open('/etc/pgadmin/conf.d/~postgres-operator/pgadmin-settings.json') as _f: _conf, _data = re.compile(r'[A-Z_0-9]+'), json.load(_f) @@ -156,11 +156,9 @@ initContainers: if os.path.isfile('/etc/pgadmin/conf.d/~postgres-operator/config-database-uri'): with open('/etc/pgadmin/conf.d/~postgres-operator/config-database-uri') as _f: CONFIG_DATABASE_URI = _f.read() - - DATA_DIR = '/var/lib/pgadmin' - LOG_FILE = '/var/lib/pgadmin/logs/pgadmin.log' - | - import json, re + import json, re, gunicorn + gunicorn.SERVER_SOFTWARE = 'Python' with open('/etc/pgadmin/conf.d/~postgres-operator/gunicorn-config.json') as _f: _conf, _data = re.compile(r'[a-z_]+'), json.load(_f) if type(_data) is dict: @@ -194,6 +192,8 @@ volumes: path: ~postgres-operator/pgadmin-shared-clusters.json - key: gunicorn-config.json path: ~postgres-operator/gunicorn-config.json + - key: gunicorn-logging-config.json + path: ~postgres-operator/gunicorn-logging-config.json - name: pgadmin-data persistentVolumeClaim: claimName: "" @@ -256,7 +256,7 @@ containers: if [ $APP_RELEASE -eq 7 ]; then pgadmin4 & else - gunicorn -c /etc/pgadmin/gunicorn_config.py --chdir $PGADMIN_DIR pgAdmin4:app & + gunicorn -c /etc/pgadmin/gunicorn_config.py --reload-extra-file /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --log-config-json /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --chdir $PGADMIN_DIR pgAdmin4:app & fi echo $! > $PGADMIN4_PIDFILE @@ -281,7 +281,7 @@ containers: if [[ $APP_RELEASE -eq 7 ]]; then pgadmin4 & else - gunicorn -c /etc/pgadmin/gunicorn_config.py --chdir $PGADMIN_DIR pgAdmin4:app & + gunicorn -c /etc/pgadmin/gunicorn_config.py --reload-extra-file /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --log-config-json /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --chdir $PGADMIN_DIR pgAdmin4:app & fi echo $! > $PGADMIN4_PIDFILE echo "Restarting pgAdmin4" @@ -343,7 +343,7 @@ initContainers: echo "$2" > /etc/pgadmin/gunicorn_config.py - startup - | - import glob, json, re, os, logging + import glob, json, re, os DEFAULT_BINARY_PATHS = {'pg': sorted([''] + glob.glob('/usr/pgsql-*/bin')).pop()} with open('/etc/pgadmin/conf.d/~postgres-operator/pgadmin-settings.json') as _f: _conf, _data = re.compile(r'[A-Z_0-9]+'), json.load(_f) @@ -355,47 +355,13 @@ initContainers: if os.path.isfile('/etc/pgadmin/conf.d/~postgres-operator/config-database-uri'): with open('/etc/pgadmin/conf.d/~postgres-operator/config-database-uri') as _f: CONFIG_DATABASE_URI = _f.read() - - DATA_DIR = '/var/lib/pgadmin' - LOG_FILE = '/var/lib/pgadmin/logs/pgadmin.log' - - LOG_ROTATION_AGE = 60 # minutes - LOG_ROTATION_MAX_LOG_FILES = 11 - - JSON_LOGGER = True - CONSOLE_LOG_LEVEL = logging.WARNING - FILE_LOG_LEVEL = logging.INFO - FILE_LOG_FORMAT_JSON = {'time': 'created', 'name': 'name', 'level': 'levelname', 'message': 'message'} - | - import json, re + import json, re, gunicorn + gunicorn.SERVER_SOFTWARE = 'Python' with open('/etc/pgadmin/conf.d/~postgres-operator/gunicorn-config.json') as _f: _conf, _data = re.compile(r'[a-z_]+'), json.load(_f) if type(_data) is dict: globals().update({k: v for k, v in _data.items() if _conf.fullmatch(k)}) - - import collections, copy, gunicorn, gunicorn.glogging - gunicorn.SERVER_SOFTWARE = 'Python' - logconfig_dict = copy.deepcopy(gunicorn.glogging.CONFIG_DEFAULTS) - logconfig_dict['loggers']['gunicorn.access']['handlers'] = ['file'] - logconfig_dict['loggers']['gunicorn.error']['handlers'] = ['file'] - logconfig_dict['handlers']['file'] = { - 'class': 'logging.handlers.TimedRotatingFileHandler', - 'filename': '/var/lib/pgadmin/logs/gunicorn.log', - 'backupCount': 11, - 'interval': 1, # every one unit (defined by when), rotate - 'when': 'H', - 'formatter': 'json', - } - logconfig_dict['formatters']['json'] = { - 'class': 'jsonformatter.JsonFormatter', - 'separators': (',', ':'), - 'format': collections.OrderedDict([ - ('time', 'created'), - ('name', 'name'), - ('level', 'levelname'), - ('message', 'message'), - ]) - } image: new-image imagePullPolicy: Always name: pgadmin-startup @@ -429,6 +395,8 @@ volumes: path: ~postgres-operator/pgadmin-shared-clusters.json - key: gunicorn-config.json path: ~postgres-operator/gunicorn-config.json + - key: gunicorn-logging-config.json + path: ~postgres-operator/gunicorn-logging-config.json - name: pgadmin-data persistentVolumeClaim: claimName: "" @@ -471,6 +439,8 @@ func TestPodConfigFiles(t *testing.T) { path: ~postgres-operator/pgadmin-shared-clusters.json - key: gunicorn-config.json path: ~postgres-operator/gunicorn-config.json + - key: gunicorn-logging-config.json + path: ~postgres-operator/gunicorn-logging-config.json name: some-cm `)) } From 6b3c1b54b57573f632c3b2ba1930ee7f81c91462 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Mon, 3 Mar 2025 21:36:31 -0600 Subject: [PATCH 4/9] changes for cm version --- .../controller/standalone_pgadmin/configmap.go | 14 +++++++------- internal/controller/standalone_pgadmin/pod.go | 5 ++--- internal/controller/standalone_pgadmin/pod_test.go | 11 +---------- .../controller/standalone_pgadmin/statefulset.go | 2 +- 4 files changed, 11 insertions(+), 21 deletions(-) diff --git a/internal/controller/standalone_pgadmin/configmap.go b/internal/controller/standalone_pgadmin/configmap.go index 79d4611c73..0614f182c1 100644 --- a/internal/controller/standalone_pgadmin/configmap.go +++ b/internal/controller/standalone_pgadmin/configmap.go @@ -64,9 +64,9 @@ func configmap(ctx context.Context, pgadmin *v1beta1.PGAdmin, // TODO(tjmoore4): Populate configuration details. initialize.Map(&configmap.Data) - pgadminConfigSettings, err := generateConfig(ctx, pgadmin) + configSettings, err := generateConfig(ctx, pgadmin) if err == nil { - configmap.Data[settingsConfigMapKey] = pgadminConfigSettings + configmap.Data[settingsConfigMapKey] = configSettings } clusterSettings, err := generateClusterConfig(clusters) @@ -84,9 +84,7 @@ func configmap(ctx context.Context, pgadmin *v1beta1.PGAdmin, } // generateConfigs generates the config settings for the pgAdmin and gunicorn -func generateConfig(ctx context.Context, pgadmin *v1beta1.PGAdmin) ( - string, error, -) { +func generateConfig(ctx context.Context, pgadmin *v1beta1.PGAdmin) (string, error) { settings := map[string]any{ // Bind to all IPv4 addresses by default. "0.0.0.0" here represents INADDR_ANY. // - https://flask.palletsprojects.com/en/2.2.x/api/#flask.Flask.run @@ -261,6 +259,10 @@ func generateGunicornConfig(ctx context.Context, pgadmin *v1beta1.PGAdmin) ( encoder.SetIndent("", " ") err := encoder.Encode(settings) + if err != nil { + return buffer.String(), "", err + } + // Gunicorn logging dict settings logSettings := map[string]any{} @@ -357,8 +359,6 @@ func generateGunicornConfig(ctx context.Context, pgadmin *v1beta1.PGAdmin) ( logEncoder := json.NewEncoder(logBuffer) logEncoder.SetEscapeHTML(false) logEncoder.SetIndent("", " ") - - // Combine errors err = logEncoder.Encode(logSettings) return buffer.String(), logBuffer.String(), err diff --git a/internal/controller/standalone_pgadmin/pod.go b/internal/controller/standalone_pgadmin/pod.go index f743c04991..5e973fd8d9 100644 --- a/internal/controller/standalone_pgadmin/pod.go +++ b/internal/controller/standalone_pgadmin/pod.go @@ -45,7 +45,6 @@ const ( // pod populates a PodSpec with the container and volumes needed to run pgAdmin. func pod( - ctx context.Context, inPGAdmin *v1beta1.PGAdmin, inConfigMap *corev1.ConfigMap, outPod *corev1.PodSpec, @@ -156,7 +155,7 @@ func pod( startup := corev1.Container{ Name: naming.ContainerPGAdminStartup, - Command: startupCommand(ctx, inPGAdmin), + Command: startupCommand(inPGAdmin), Image: container.Image, ImagePullPolicy: container.ImagePullPolicy, Resources: container.Resources, @@ -347,7 +346,7 @@ done } // startupCommand returns an entrypoint that prepares the filesystem for pgAdmin. -func startupCommand(ctx context.Context, inPgadmin *v1beta1.PGAdmin) []string { +func startupCommand(inPgadmin *v1beta1.PGAdmin) []string { // pgAdmin reads from the `/etc/pgadmin/config_system.py` file during startup // after all other config files. // - https://github.com/pgadmin-org/pgadmin4/blob/REL-7_7/docs/en_US/config_py.rst diff --git a/internal/controller/standalone_pgadmin/pod_test.go b/internal/controller/standalone_pgadmin/pod_test.go index 64c62b1886..81048b3401 100644 --- a/internal/controller/standalone_pgadmin/pod_test.go +++ b/internal/controller/standalone_pgadmin/pod_test.go @@ -13,7 +13,6 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/crunchydata/postgres-operator/internal/feature" "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/kubernetes" "github.com/crunchydata/postgres-operator/internal/testing/cmp" @@ -29,9 +28,8 @@ func TestPod(t *testing.T) { config := new(corev1.ConfigMap) testpod := new(corev1.PodSpec) pvc := new(corev1.PersistentVolumeClaim) - ctx := context.Background() - call := func() { pod(ctx, pgadmin, config, testpod, pvc) } + call := func() { pod(pgadmin, config, testpod, pvc) } t.Run("Defaults", func(t *testing.T) { @@ -223,13 +221,6 @@ volumes: }, } - // Turn on the Feature gate - gate := feature.NewGate() - assert.NilError(t, gate.SetFromMap(map[string]bool{ - feature.OpenTelemetryLogs: true, - })) - ctx = feature.NewContext(context.Background(), gate) - call() assert.Assert(t, cmp.MarshalMatches(testpod, ` diff --git a/internal/controller/standalone_pgadmin/statefulset.go b/internal/controller/standalone_pgadmin/statefulset.go index c4958e9ba9..c75668defc 100644 --- a/internal/controller/standalone_pgadmin/statefulset.go +++ b/internal/controller/standalone_pgadmin/statefulset.go @@ -120,7 +120,7 @@ func statefulset( sts.Spec.Template.Spec.SecurityContext = podSecurityContext(ctx) - pod(ctx, pgadmin, configmap, &sts.Spec.Template.Spec, dataVolume) + pod(pgadmin, configmap, &sts.Spec.Template.Spec, dataVolume) if pgadmin.Spec.Instrumentation != nil && feature.Enabled(ctx, feature.OpenTelemetryLogs) { // Logs for gunicorn and pgadmin write to /var/lib/pgadmin/logs From d2aad56f3c9b573ecd32a7bc25bb6f8388faebde Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Mon, 3 Mar 2025 21:43:55 -0600 Subject: [PATCH 5/9] changes for cm version, 2 --- internal/controller/standalone_pgadmin/pod.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controller/standalone_pgadmin/pod.go b/internal/controller/standalone_pgadmin/pod.go index 5e973fd8d9..d488c87f84 100644 --- a/internal/controller/standalone_pgadmin/pod.go +++ b/internal/controller/standalone_pgadmin/pod.go @@ -155,7 +155,7 @@ func pod( startup := corev1.Container{ Name: naming.ContainerPGAdminStartup, - Command: startupCommand(inPGAdmin), + Command: startupCommand(), Image: container.Image, ImagePullPolicy: container.ImagePullPolicy, Resources: container.Resources, @@ -346,7 +346,7 @@ done } // startupCommand returns an entrypoint that prepares the filesystem for pgAdmin. -func startupCommand(inPgadmin *v1beta1.PGAdmin) []string { +func startupCommand() []string { // pgAdmin reads from the `/etc/pgadmin/config_system.py` file during startup // after all other config files. // - https://github.com/pgadmin-org/pgadmin4/blob/REL-7_7/docs/en_US/config_py.rst From 3a69d4756107d8e59dc0e85272908a961d45193a Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Tue, 4 Mar 2025 09:07:46 -0600 Subject: [PATCH 6/9] Move the number generate out of the cm generate --- .../standalone_pgadmin/configmap.go | 103 ++++++++---------- .../standalone_pgadmin/configmap_test.go | 33 ++---- 2 files changed, 53 insertions(+), 83 deletions(-) diff --git a/internal/controller/standalone_pgadmin/configmap.go b/internal/controller/standalone_pgadmin/configmap.go index 0614f182c1..37b699aaf0 100644 --- a/internal/controller/standalone_pgadmin/configmap.go +++ b/internal/controller/standalone_pgadmin/configmap.go @@ -64,7 +64,38 @@ func configmap(ctx context.Context, pgadmin *v1beta1.PGAdmin, // TODO(tjmoore4): Populate configuration details. initialize.Map(&configmap.Data) - configSettings, err := generateConfig(ctx, pgadmin) + var ( + logRetention bool + maxBackupRetentionNumber = 1 + // One day in minutes for pgadmin rotation + pgAdminRetentionPeriod = 24 * 60 + // Daily rotation for gunicorn rotation + gunicornRetentionPeriod = "D" + ) + // If OTel logs feature gate is enabled, we want to change the pgAdmin/gunicorn logging + if feature.Enabled(ctx, feature.OpenTelemetryLogs) && pgadmin.Spec.Instrumentation != nil { + logRetention = true + + // If the user has set a retention period, we will use those values for log rotation, + // which is otherwise managed by python. + if pgadmin.Spec.Instrumentation.Logs != nil && + pgadmin.Spec.Instrumentation.Logs.RetentionPeriod != nil { + + retentionNumber, period := collector.ParseDurationForLogrotate(pgadmin.Spec.Instrumentation.Logs.RetentionPeriod.AsDuration()) + // `LOG_ROTATION_MAX_LOG_FILES`` in pgadmin refers to the already rotated logs. + // `backupCount` for gunicorn is similar. + // Our retention unit is for total number of log files, so subtract 1 to account + // for the currently-used log file. + maxBackupRetentionNumber = retentionNumber - 1 + if period == "hourly" { + // If the period is hourly, set the pgadmin + // and gunicorn retention periods to hourly. + pgAdminRetentionPeriod = 60 + gunicornRetentionPeriod = "H" + } + } + } + configSettings, err := generateConfig(pgadmin, logRetention, maxBackupRetentionNumber, pgAdminRetentionPeriod) if err == nil { configmap.Data[settingsConfigMapKey] = configSettings } @@ -74,7 +105,8 @@ func configmap(ctx context.Context, pgadmin *v1beta1.PGAdmin, configmap.Data[settingsClusterMapKey] = clusterSettings } - gunicornSettings, gunicornLoggingSettings, err := generateGunicornConfig(ctx, pgadmin) + gunicornSettings, gunicornLoggingSettings, err := generateGunicornConfig(pgadmin, + logRetention, maxBackupRetentionNumber, gunicornRetentionPeriod) if err == nil { configmap.Data[gunicornConfigKey] = gunicornSettings configmap.Data[gunicornLoggingConfigKey] = gunicornLoggingSettings @@ -84,7 +116,9 @@ func configmap(ctx context.Context, pgadmin *v1beta1.PGAdmin, } // generateConfigs generates the config settings for the pgAdmin and gunicorn -func generateConfig(ctx context.Context, pgadmin *v1beta1.PGAdmin) (string, error) { +func generateConfig(pgadmin *v1beta1.PGAdmin, + logRetention bool, maxBackupRetentionNumber, pgAdminRetentionPeriod int) ( + string, error) { settings := map[string]any{ // Bind to all IPv4 addresses by default. "0.0.0.0" here represents INADDR_ANY. // - https://flask.palletsprojects.com/en/2.2.x/api/#flask.Flask.run @@ -107,33 +141,7 @@ func generateConfig(ctx context.Context, pgadmin *v1beta1.PGAdmin) (string, erro settings["DATA_DIR"] = dataMountPath settings["LOG_FILE"] = LogFileAbsolutePath - // If OTel logs feature gate is enabled, we want to change the pgAdmin/gunicorn logging - if feature.Enabled(ctx, feature.OpenTelemetryLogs) && pgadmin.Spec.Instrumentation != nil { - - var ( - maxBackupRetentionNumber = 1 - // One day in minutes for pgadmin rotation - pgAdminRetentionPeriod = 24 * 60 - ) - - // If the user has set a retention period, we will use those values for log rotation, - // which is otherwise managed by python. - if pgadmin.Spec.Instrumentation.Logs != nil && - pgadmin.Spec.Instrumentation.Logs.RetentionPeriod != nil { - - retentionNumber, period := collector.ParseDurationForLogrotate(pgadmin.Spec.Instrumentation.Logs.RetentionPeriod.AsDuration()) - // `LOG_ROTATION_MAX_LOG_FILES`` in pgadmin refers to the already rotated logs. - // `backupCount` for gunicorn is similar. - // Our retention unit is for total number of log files, so subtract 1 to account - // for the currently-used log file. - maxBackupRetentionNumber = retentionNumber - 1 - if period == "hourly" { - // If the period is hourly, set the pgadmin - // and gunicorn retention periods to hourly. - pgAdminRetentionPeriod = 60 - } - } - + if logRetention { settings["LOG_ROTATION_AGE"] = pgAdminRetentionPeriod settings["LOG_ROTATION_MAX_LOG_FILES"] = maxBackupRetentionNumber settings["JSON_LOGGER"] = true @@ -229,9 +237,9 @@ func generateClusterConfig( // generateGunicornConfig generates the config settings for the gunicorn server // - https://docs.gunicorn.org/en/latest/settings.html -func generateGunicornConfig(ctx context.Context, pgadmin *v1beta1.PGAdmin) ( - string, string, error, -) { +func generateGunicornConfig(pgadmin *v1beta1.PGAdmin, + logRetention bool, maxBackupRetentionNumber int, gunicornRetentionPeriod string, +) (string, string, error) { settings := map[string]any{ // Bind to all IPv4 addresses and set 25 threads by default. // - https://docs.gunicorn.org/en/latest/settings.html#bind @@ -266,33 +274,8 @@ func generateGunicornConfig(ctx context.Context, pgadmin *v1beta1.PGAdmin) ( // Gunicorn logging dict settings logSettings := map[string]any{} - // If OTel logs feature gate is enabled, we want to change the pgAdmin/gunicorn logging - if feature.Enabled(ctx, feature.OpenTelemetryLogs) && - pgadmin.Spec.Instrumentation != nil { - - var ( - maxBackupRetentionNumber = "1" - // Daily rotation for gunicorn rotation - gunicornRetentionPeriod = "D" - ) - - // If the user has set a retention period, we will use those values for log rotation, - // which is otherwise managed by python. - if pgadmin.Spec.Instrumentation.Logs != nil && - pgadmin.Spec.Instrumentation.Logs.RetentionPeriod != nil { - - retentionNumber, period := collector.ParseDurationForLogrotate(pgadmin.Spec.Instrumentation.Logs.RetentionPeriod.AsDuration()) - // `LOG_ROTATION_MAX_LOG_FILES`` in pgadmin refers to the already rotated logs. - // `backupCount` for gunicorn is similar. - // Our retention unit is for total number of log files, so subtract 1 to account - // for the currently-used log file. - maxBackupRetentionNumber = strconv.Itoa(retentionNumber - 1) - if period == "hourly" { - // If the period is hourly, set the pgadmin - // and gunicorn retention periods to hourly. - gunicornRetentionPeriod = "H" - } - } + // If OTel logs feature gate is enabled, we want to change the gunicorn logging + if logRetention { // Gunicorn uses the Python logging package, which sets the following attributes: // https://docs.python.org/3/library/logging.html#logrecord-attributes. diff --git a/internal/controller/standalone_pgadmin/configmap_test.go b/internal/controller/standalone_pgadmin/configmap_test.go index 8b45589002..58c4fcf2f7 100644 --- a/internal/controller/standalone_pgadmin/configmap_test.go +++ b/internal/controller/standalone_pgadmin/configmap_test.go @@ -11,7 +11,6 @@ import ( "gotest.tools/v3/assert" - "github.com/crunchydata/postgres-operator/internal/feature" "github.com/crunchydata/postgres-operator/internal/testing/cmp" "github.com/crunchydata/postgres-operator/internal/testing/require" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" @@ -19,11 +18,10 @@ import ( func TestGenerateConfig(t *testing.T) { require.ParallelCapacity(t, 0) - ctx := context.Background() t.Run("Default", func(t *testing.T) { pgadmin := new(v1beta1.PGAdmin) - result, err := generateConfig(ctx, pgadmin) + result, err := generateConfig(pgadmin, false, 0, 0) assert.NilError(t, err) assert.Equal(t, result, `{ @@ -43,7 +41,7 @@ func TestGenerateConfig(t *testing.T) { "SERVER_MODE": false, "UPGRADE_CHECK_ENABLED": true, } - result, err := generateConfig(ctx, pgadmin) + result, err := generateConfig(pgadmin, false, 0, 0) assert.NilError(t, err) assert.Equal(t, result, `{ @@ -63,7 +61,7 @@ func TestGenerateConfig(t *testing.T) { "ALLOWED_HOSTS": []any{"225.0.0.0/8", "226.0.0.0/7", "228.0.0.0/6"}, "DEFAULT_SERVER": "::", } - result, err := generateConfig(ctx, pgadmin) + result, err := generateConfig(pgadmin, false, 0, 0) assert.NilError(t, err) assert.Equal(t, result, `{ @@ -83,18 +81,13 @@ func TestGenerateConfig(t *testing.T) { }) t.Run("OTel enabled", func(t *testing.T) { - gate := feature.NewGate() - assert.NilError(t, gate.SetFromMap(map[string]bool{ - feature.OpenTelemetryLogs: true, - })) - ctx := feature.NewContext(context.Background(), gate) pgadmin := new(v1beta1.PGAdmin) require.UnmarshalInto(t, &pgadmin.Spec, `{ instrumentation: { logs: { retentionPeriod: 5h }, }, }`) - result, err := generateConfig(ctx, pgadmin) + result, err := generateConfig(pgadmin, true, 4, 60) assert.NilError(t, err) assert.Equal(t, result, `{ @@ -257,7 +250,6 @@ namespace: some-ns func TestGenerateGunicornConfig(t *testing.T) { require.ParallelCapacity(t, 0) - ctx := context.Background() t.Run("Default", func(t *testing.T) { pgAdmin := &v1beta1.PGAdmin{} @@ -270,7 +262,7 @@ func TestGenerateGunicornConfig(t *testing.T) { "workers": 1 } ` - actualString, logString, err := generateGunicornConfig(ctx, pgAdmin) + actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H") assert.NilError(t, err) assert.Equal(t, actualString, expectedString) assert.Assert(t, strings.Contains(logString, "{}")) @@ -293,7 +285,7 @@ func TestGenerateGunicornConfig(t *testing.T) { "workers": 1 } ` - actualString, logString, err := generateGunicornConfig(ctx, pgAdmin) + actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H") assert.NilError(t, err) assert.Equal(t, actualString, expectedString) assert.Assert(t, strings.Contains(logString, "{}")) @@ -314,7 +306,7 @@ func TestGenerateGunicornConfig(t *testing.T) { "workers": 1 } ` - actualString, logString, err := generateGunicornConfig(ctx, pgAdmin) + actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H") assert.NilError(t, err) assert.Equal(t, actualString, expectedString) assert.Assert(t, strings.Contains(logString, "{}")) @@ -334,18 +326,13 @@ func TestGenerateGunicornConfig(t *testing.T) { "workers": 1 } ` - actualString, logString, err := generateGunicornConfig(ctx, pgAdmin) + actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H") assert.NilError(t, err) assert.Equal(t, actualString, expectedString) assert.Assert(t, strings.Contains(logString, "{}")) }) t.Run("OTel enabled", func(t *testing.T) { - gate := feature.NewGate() - assert.NilError(t, gate.SetFromMap(map[string]bool{ - feature.OpenTelemetryLogs: true, - })) - ctx := feature.NewContext(context.Background(), gate) pgAdmin := &v1beta1.PGAdmin{} pgAdmin.Name = "test" pgAdmin.Namespace = "postgres-operator" @@ -354,7 +341,7 @@ func TestGenerateGunicornConfig(t *testing.T) { logs: { retentionPeriod: 5h }, }, }`) - actualString, logString, err := generateGunicornConfig(ctx, pgAdmin) + actualString, logString, err := generateGunicornConfig(pgAdmin, true, 4, "H") expectedString := `{ "bind": "0.0.0.0:5050", @@ -390,7 +377,7 @@ func TestGenerateGunicornConfig(t *testing.T) { "stream": "ext://sys.stdout" }, "file": { - "backupCount": "4", + "backupCount": 4, "class": "logging.handlers.TimedRotatingFileHandler", "filename": "/var/lib/pgadmin/logs/gunicorn.log", "formatter": "json", From 0d665b0ae5980628b482b6227ac121e6716bb83a Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 6 Mar 2025 10:27:11 -0600 Subject: [PATCH 7/9] Use SIGKILL to reload gunicorn logger Do we really want to do this? --- .../controller/standalone_pgadmin/config.go | 7 +- .../standalone_pgadmin/configmap.go | 33 ++--- .../standalone_pgadmin/configmap_test.go | 126 +++++++++--------- internal/controller/standalone_pgadmin/pod.go | 27 ++-- .../controller/standalone_pgadmin/pod_test.go | 18 +-- 5 files changed, 92 insertions(+), 119 deletions(-) diff --git a/internal/controller/standalone_pgadmin/config.go b/internal/controller/standalone_pgadmin/config.go index 8ad86ad1ab..3af09144f2 100644 --- a/internal/controller/standalone_pgadmin/config.go +++ b/internal/controller/standalone_pgadmin/config.go @@ -7,10 +7,9 @@ package standalone_pgadmin // Include configs here used by multiple files const ( // ConfigMap keys used also in mounting volume to pod - settingsConfigMapKey = "pgadmin-settings.json" - settingsClusterMapKey = "pgadmin-shared-clusters.json" - gunicornLoggingConfigKey = "gunicorn-logging-config.json" - gunicornConfigKey = "gunicorn-config.json" + settingsConfigMapKey = "pgadmin-settings.json" + settingsClusterMapKey = "pgadmin-shared-clusters.json" + gunicornConfigKey = "gunicorn-config.json" // Port address used to define pod and service pgAdminPort = 5050 diff --git a/internal/controller/standalone_pgadmin/configmap.go b/internal/controller/standalone_pgadmin/configmap.go index 37b699aaf0..72a95b14db 100644 --- a/internal/controller/standalone_pgadmin/configmap.go +++ b/internal/controller/standalone_pgadmin/configmap.go @@ -105,11 +105,10 @@ func configmap(ctx context.Context, pgadmin *v1beta1.PGAdmin, configmap.Data[settingsClusterMapKey] = clusterSettings } - gunicornSettings, gunicornLoggingSettings, err := generateGunicornConfig(pgadmin, + gunicornSettings, err := generateGunicornConfig(pgadmin, logRetention, maxBackupRetentionNumber, gunicornRetentionPeriod) if err == nil { configmap.Data[gunicornConfigKey] = gunicornSettings - configmap.Data[gunicornLoggingConfigKey] = gunicornLoggingSettings } return configmap, err @@ -239,7 +238,7 @@ func generateClusterConfig( // - https://docs.gunicorn.org/en/latest/settings.html func generateGunicornConfig(pgadmin *v1beta1.PGAdmin, logRetention bool, maxBackupRetentionNumber int, gunicornRetentionPeriod string, -) (string, string, error) { +) (string, error) { settings := map[string]any{ // Bind to all IPv4 addresses and set 25 threads by default. // - https://docs.gunicorn.org/en/latest/settings.html#bind @@ -256,21 +255,6 @@ func generateGunicornConfig(pgadmin *v1beta1.PGAdmin, // Write mandatory settings over any specified ones. // - https://docs.gunicorn.org/en/latest/settings.html#workers settings["workers"] = 1 - - // To avoid spurious reconciles, the following value must not change when - // the spec does not change. [json.Encoder] and [json.Marshal] do this by - // emitting map keys in sorted order. Indent so the value is not rendered - // as one long line by `kubectl`. - buffer := new(bytes.Buffer) - encoder := json.NewEncoder(buffer) - encoder.SetEscapeHTML(false) - encoder.SetIndent("", " ") - err := encoder.Encode(settings) - - if err != nil { - return buffer.String(), "", err - } - // Gunicorn logging dict settings logSettings := map[string]any{} @@ -333,16 +317,17 @@ func generateGunicornConfig(pgadmin *v1beta1.PGAdmin, }, } } + settings["logconfig_dict"] = logSettings // To avoid spurious reconciles, the following value must not change when // the spec does not change. [json.Encoder] and [json.Marshal] do this by // emitting map keys in sorted order. Indent so the value is not rendered // as one long line by `kubectl`. - logBuffer := new(bytes.Buffer) - logEncoder := json.NewEncoder(logBuffer) - logEncoder.SetEscapeHTML(false) - logEncoder.SetIndent("", " ") - err = logEncoder.Encode(logSettings) + buffer := new(bytes.Buffer) + encoder := json.NewEncoder(buffer) + encoder.SetEscapeHTML(false) + encoder.SetIndent("", " ") + err := encoder.Encode(settings) - return buffer.String(), logBuffer.String(), err + return buffer.String(), err } diff --git a/internal/controller/standalone_pgadmin/configmap_test.go b/internal/controller/standalone_pgadmin/configmap_test.go index 58c4fcf2f7..a23ee08d18 100644 --- a/internal/controller/standalone_pgadmin/configmap_test.go +++ b/internal/controller/standalone_pgadmin/configmap_test.go @@ -6,7 +6,6 @@ package standalone_pgadmin import ( "context" - "strings" "testing" "gotest.tools/v3/assert" @@ -258,14 +257,14 @@ func TestGenerateGunicornConfig(t *testing.T) { expectedString := `{ "bind": "0.0.0.0:5050", + "logconfig_dict": {}, "threads": 25, "workers": 1 } ` - actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H") + actualString, err := generateGunicornConfig(pgAdmin, false, 0, "H") assert.NilError(t, err) assert.Equal(t, actualString, expectedString) - assert.Assert(t, strings.Contains(logString, "{}")) }) t.Run("Add Settings", func(t *testing.T) { @@ -281,14 +280,14 @@ func TestGenerateGunicornConfig(t *testing.T) { "bind": "0.0.0.0:5050", "certfile": "/path/to/certfile", "keyfile": "/path/to/keyfile", + "logconfig_dict": {}, "threads": 25, "workers": 1 } ` - actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H") + actualString, err := generateGunicornConfig(pgAdmin, false, 0, "H") assert.NilError(t, err) assert.Equal(t, actualString, expectedString) - assert.Assert(t, strings.Contains(logString, "{}")) }) t.Run("Update Defaults", func(t *testing.T) { @@ -302,14 +301,14 @@ func TestGenerateGunicornConfig(t *testing.T) { expectedString := `{ "bind": "127.0.0.1:5051", + "logconfig_dict": {}, "threads": 30, "workers": 1 } ` - actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H") + actualString, err := generateGunicornConfig(pgAdmin, false, 0, "H") assert.NilError(t, err) assert.Equal(t, actualString, expectedString) - assert.Assert(t, strings.Contains(logString, "{}")) }) t.Run("Update Mandatory", func(t *testing.T) { @@ -322,14 +321,14 @@ func TestGenerateGunicornConfig(t *testing.T) { expectedString := `{ "bind": "0.0.0.0:5050", + "logconfig_dict": {}, "threads": 25, "workers": 1 } ` - actualString, logString, err := generateGunicornConfig(pgAdmin, false, 0, "H") + actualString, err := generateGunicornConfig(pgAdmin, false, 0, "H") assert.NilError(t, err) assert.Equal(t, actualString, expectedString) - assert.Assert(t, strings.Contains(logString, "{}")) }) t.Run("OTel enabled", func(t *testing.T) { @@ -341,73 +340,72 @@ func TestGenerateGunicornConfig(t *testing.T) { logs: { retentionPeriod: 5h }, }, }`) - actualString, logString, err := generateGunicornConfig(pgAdmin, true, 4, "H") + actualString, err := generateGunicornConfig(pgAdmin, true, 4, "H") expectedString := `{ "bind": "0.0.0.0:5050", - "threads": 25, - "workers": 1 -} -` - expectedLogString := `{ - "formatters": { - "generic": { - "class": "logging.Formatter", - "datefmt": "[%Y-%m-%d %H:%M:%S %z]", - "format": "%(asctime)s [%(process)d] [%(levelname)s] %(message)s" + "logconfig_dict": { + "formatters": { + "generic": { + "class": "logging.Formatter", + "datefmt": "[%Y-%m-%d %H:%M:%S %z]", + "format": "%(asctime)s [%(process)d] [%(levelname)s] %(message)s" + }, + "json": { + "class": "jsonformatter.JsonFormatter", + "format": { + "level": "levelname", + "message": "message", + "name": "name", + "time": "created" + }, + "separators": [ + ",", + ":" + ] + } }, - "json": { - "class": "jsonformatter.JsonFormatter", - "format": { - "level": "levelname", - "message": "message", - "name": "name", - "time": "created" + "handlers": { + "console": { + "class": "logging.StreamHandler", + "formatter": "generic", + "stream": "ext://sys.stdout" }, - "separators": [ - ",", - ":" - ] - } - }, - "handlers": { - "console": { - "class": "logging.StreamHandler", - "formatter": "generic", - "stream": "ext://sys.stdout" + "file": { + "backupCount": 4, + "class": "logging.handlers.TimedRotatingFileHandler", + "filename": "/var/lib/pgadmin/logs/gunicorn.log", + "formatter": "json", + "interval": 1, + "when": "H" + } }, - "file": { - "backupCount": 4, - "class": "logging.handlers.TimedRotatingFileHandler", - "filename": "/var/lib/pgadmin/logs/gunicorn.log", - "formatter": "json", - "interval": 1, - "when": "H" + "loggers": { + "gunicorn.access": { + "handlers": [ + "file" + ], + "level": "INFO", + "propagate": true, + "qualname": "gunicorn.access" + }, + "gunicorn.error": { + "handlers": [ + "file" + ], + "level": "INFO", + "propagate": true, + "qualname": "gunicorn.error" + } } }, - "loggers": { - "gunicorn.access": { - "handlers": [ - "file" - ], - "level": "INFO", - "propagate": true, - "qualname": "gunicorn.access" - }, - "gunicorn.error": { - "handlers": [ - "file" - ], - "level": "INFO", - "propagate": true, - "qualname": "gunicorn.error" - } - } + "threads": 25, + "workers": 1 } ` + assert.NilError(t, err) assert.Equal(t, actualString, expectedString) - assert.Equal(t, logString, expectedLogString) }) } diff --git a/internal/controller/standalone_pgadmin/pod.go b/internal/controller/standalone_pgadmin/pod.go index d488c87f84..760af8b372 100644 --- a/internal/controller/standalone_pgadmin/pod.go +++ b/internal/controller/standalone_pgadmin/pod.go @@ -22,13 +22,12 @@ import ( ) const ( - configMountPath = "/etc/pgadmin/conf.d" - configFilePath = "~postgres-operator/" + settingsConfigMapKey - clusterFilePath = "~postgres-operator/" + settingsClusterMapKey - configDatabaseURIPath = "~postgres-operator/config-database-uri" - ldapFilePath = "~postgres-operator/ldap-bind-password" - gunicornConfigFilePath = "~postgres-operator/" + gunicornConfigKey - gunicornLogConfFilePath = "~postgres-operator/" + gunicornLoggingConfigKey + configMountPath = "/etc/pgadmin/conf.d" + configFilePath = "~postgres-operator/" + settingsConfigMapKey + clusterFilePath = "~postgres-operator/" + settingsClusterMapKey + configDatabaseURIPath = "~postgres-operator/config-database-uri" + ldapFilePath = "~postgres-operator/ldap-bind-password" + gunicornConfigFilePath = "~postgres-operator/" + gunicornConfigKey // scriptMountPath is where to mount a temporary directory that is only // writable during Pod initialization. @@ -208,10 +207,6 @@ func podConfigFiles(configmap *corev1.ConfigMap, pgadmin v1beta1.PGAdmin) []core Key: gunicornConfigKey, Path: gunicornConfigFilePath, }, - { - Key: gunicornLoggingConfigKey, - Path: gunicornLogConfFilePath, - }, }, }, }, @@ -267,10 +262,7 @@ func startupScript(pgadmin *v1beta1.PGAdmin) []string { // startCommands (v8 image includes Gunicorn) var startCommandV7 = "pgadmin4 &" - // For Gunicorn, watch the logging config and reload if changes are detected. var startCommandV8 = "gunicorn -c /etc/pgadmin/gunicorn_config.py" + - " --reload-extra-file " + configMountPath + `/` + gunicornLogConfFilePath + - " --log-config-json " + configMountPath + `/` + gunicornLogConfFilePath + " --chdir $PGADMIN_DIR pgAdmin4:app &" // This script sets up, starts pgadmin, and runs the appropriate `loadServerCommand` to register the discovered servers. @@ -319,10 +311,15 @@ loadServerCommand // descriptor and uses the timeout of the builtin `read` to wait. That same // descriptor gets closed and reopened to use the builtin `[ -nt` to check mtimes. // - https://unix.stackexchange.com/a/407383 + // In order to get gunicorn to reload the logging config + // we need to send a KILL rather than a HUP signal. + // - https://github.com/benoitc/gunicorn/issues/3353 + // Right now the config file is on the same configMap as the cluster file + // so if the mtime changes for any of those files, it will change for all. var reloadScript = ` exec {fd}<> <(:||:) while read -r -t 5 -u "${fd}" ||:; do - if [[ "${cluster_file}" -nt "/proc/self/fd/${fd}" ]] && loadServerCommand + if [[ "${cluster_file}" -nt "/proc/self/fd/${fd}" ]] && loadServerCommand && kill -KILL $(head -1 ${PGADMIN4_PIDFILE?}); then exec {fd}>&- && exec {fd}<> <(:||:) stat --format='Loaded shared servers dated %y' "${cluster_file}" diff --git a/internal/controller/standalone_pgadmin/pod_test.go b/internal/controller/standalone_pgadmin/pod_test.go index 81048b3401..501624bc1f 100644 --- a/internal/controller/standalone_pgadmin/pod_test.go +++ b/internal/controller/standalone_pgadmin/pod_test.go @@ -59,7 +59,7 @@ containers: if [ $APP_RELEASE -eq 7 ]; then pgadmin4 & else - gunicorn -c /etc/pgadmin/gunicorn_config.py --reload-extra-file /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --log-config-json /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --chdir $PGADMIN_DIR pgAdmin4:app & + gunicorn -c /etc/pgadmin/gunicorn_config.py --chdir $PGADMIN_DIR pgAdmin4:app & fi echo $! > $PGADMIN4_PIDFILE @@ -74,7 +74,7 @@ containers: exec {fd}<> <(:||:) while read -r -t 5 -u "${fd}" ||:; do - if [[ "${cluster_file}" -nt "/proc/self/fd/${fd}" ]] && loadServerCommand + if [[ "${cluster_file}" -nt "/proc/self/fd/${fd}" ]] && loadServerCommand && kill -KILL $(head -1 ${PGADMIN4_PIDFILE?}); then exec {fd}>&- && exec {fd}<> <(:||:) stat --format='Loaded shared servers dated %y' "${cluster_file}" @@ -84,7 +84,7 @@ containers: if [[ $APP_RELEASE -eq 7 ]]; then pgadmin4 & else - gunicorn -c /etc/pgadmin/gunicorn_config.py --reload-extra-file /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --log-config-json /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --chdir $PGADMIN_DIR pgAdmin4:app & + gunicorn -c /etc/pgadmin/gunicorn_config.py --chdir $PGADMIN_DIR pgAdmin4:app & fi echo $! > $PGADMIN4_PIDFILE echo "Restarting pgAdmin4" @@ -190,8 +190,6 @@ volumes: path: ~postgres-operator/pgadmin-shared-clusters.json - key: gunicorn-config.json path: ~postgres-operator/gunicorn-config.json - - key: gunicorn-logging-config.json - path: ~postgres-operator/gunicorn-logging-config.json - name: pgadmin-data persistentVolumeClaim: claimName: "" @@ -247,7 +245,7 @@ containers: if [ $APP_RELEASE -eq 7 ]; then pgadmin4 & else - gunicorn -c /etc/pgadmin/gunicorn_config.py --reload-extra-file /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --log-config-json /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --chdir $PGADMIN_DIR pgAdmin4:app & + gunicorn -c /etc/pgadmin/gunicorn_config.py --chdir $PGADMIN_DIR pgAdmin4:app & fi echo $! > $PGADMIN4_PIDFILE @@ -262,7 +260,7 @@ containers: exec {fd}<> <(:||:) while read -r -t 5 -u "${fd}" ||:; do - if [[ "${cluster_file}" -nt "/proc/self/fd/${fd}" ]] && loadServerCommand + if [[ "${cluster_file}" -nt "/proc/self/fd/${fd}" ]] && loadServerCommand && kill -KILL $(head -1 ${PGADMIN4_PIDFILE?}); then exec {fd}>&- && exec {fd}<> <(:||:) stat --format='Loaded shared servers dated %y' "${cluster_file}" @@ -272,7 +270,7 @@ containers: if [[ $APP_RELEASE -eq 7 ]]; then pgadmin4 & else - gunicorn -c /etc/pgadmin/gunicorn_config.py --reload-extra-file /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --log-config-json /etc/pgadmin/conf.d/~postgres-operator/gunicorn-logging-config.json --chdir $PGADMIN_DIR pgAdmin4:app & + gunicorn -c /etc/pgadmin/gunicorn_config.py --chdir $PGADMIN_DIR pgAdmin4:app & fi echo $! > $PGADMIN4_PIDFILE echo "Restarting pgAdmin4" @@ -386,8 +384,6 @@ volumes: path: ~postgres-operator/pgadmin-shared-clusters.json - key: gunicorn-config.json path: ~postgres-operator/gunicorn-config.json - - key: gunicorn-logging-config.json - path: ~postgres-operator/gunicorn-logging-config.json - name: pgadmin-data persistentVolumeClaim: claimName: "" @@ -430,8 +426,6 @@ func TestPodConfigFiles(t *testing.T) { path: ~postgres-operator/pgadmin-shared-clusters.json - key: gunicorn-config.json path: ~postgres-operator/gunicorn-config.json - - key: gunicorn-logging-config.json - path: ~postgres-operator/gunicorn-logging-config.json name: some-cm `)) } From ee8fe79cb3893821fb8558b69561c572d4f6c308 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 6 Mar 2025 10:46:13 -0600 Subject: [PATCH 8/9] put back const rather than var --- internal/controller/standalone_pgadmin/pod.go | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/internal/controller/standalone_pgadmin/pod.go b/internal/controller/standalone_pgadmin/pod.go index 760af8b372..96a959bf62 100644 --- a/internal/controller/standalone_pgadmin/pod.go +++ b/internal/controller/standalone_pgadmin/pod.go @@ -270,7 +270,7 @@ func startupScript(pgadmin *v1beta1.PGAdmin) []string { // - https://www.pgadmin.org/docs/pgadmin4/development/server_deployment.html#standalone-gunicorn-configuration // - https://docs.gunicorn.org/en/latest/configure.html var startScript = fmt.Sprintf(` -export PGADMIN_SETUP_PASSWORD="$(date +%%s | sha256sum | base64 | head -c 32)" +export PGADMIN_SETUP_PASSWORD="admin" PGADMIN_DIR=%s APP_RELEASE=$(cd $PGADMIN_DIR && python3 -c "import config; print(config.APP_RELEASE)") @@ -365,11 +365,10 @@ func startupCommand() []string { // configDatabaseURIPath is the path for mounting the database URI connection string configDatabaseURIPathAbsolutePath = configMountPath + "/" + configDatabaseURIPath - ) - // The values set in configSystem will not be overridden through - // spec.config.settings. - var configSystem = ` + // The values set in configSystem will not be overridden through + // spec.config.settings. + configSystem = ` import glob, json, re, os DEFAULT_BINARY_PATHS = {'pg': sorted([''] + glob.glob('/usr/pgsql-*/bin')).pop()} with open('` + configMountPath + `/` + configFilePath + `') as _f: @@ -384,17 +383,17 @@ if os.path.isfile('` + configDatabaseURIPathAbsolutePath + `'): CONFIG_DATABASE_URI = _f.read() ` - // Gunicorn reads from the `/etc/pgadmin/gunicorn_config.py` file during startup - // after all other config files. - // - https://docs.gunicorn.org/en/latest/configure.html#configuration-file - // - // This command writes a script in `/etc/pgadmin/gunicorn_config.py` that reads - // from the `gunicorn-config.json` file and sets those variables globally. - // That way those values are available as settings when Gunicorn starts. - // - // Note: All Gunicorn settings are lowercase with underscores, so ignore - // any keys/names that are not. - var gunicornConfig = ` + // Gunicorn reads from the `/etc/pgadmin/gunicorn_config.py` file during startup + // after all other config files. + // - https://docs.gunicorn.org/en/latest/configure.html#configuration-file + // + // This command writes a script in `/etc/pgadmin/gunicorn_config.py` that reads + // from the `gunicorn-config.json` file and sets those variables globally. + // That way those values are available as settings when Gunicorn starts. + // + // Note: All Gunicorn settings are lowercase with underscores, so ignore + // any keys/names that are not. + gunicornConfig = ` import json, re, gunicorn gunicorn.SERVER_SOFTWARE = 'Python' with open('` + configMountPath + `/` + gunicornConfigFilePath + `') as _f: @@ -402,6 +401,7 @@ with open('` + configMountPath + `/` + gunicornConfigFilePath + `') as _f: if type(_data) is dict: globals().update({k: v for k, v in _data.items() if _conf.fullmatch(k)}) ` + ) args := []string{strings.TrimLeft(configSystem, "\n"), strings.TrimLeft(gunicornConfig, "\n")} From 31f2f4c3d6f896d390574a927f8979fb13533d72 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 6 Mar 2025 10:59:59 -0600 Subject: [PATCH 9/9] put back password --- internal/controller/standalone_pgadmin/pod.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/standalone_pgadmin/pod.go b/internal/controller/standalone_pgadmin/pod.go index 96a959bf62..aa6fdde481 100644 --- a/internal/controller/standalone_pgadmin/pod.go +++ b/internal/controller/standalone_pgadmin/pod.go @@ -270,7 +270,7 @@ func startupScript(pgadmin *v1beta1.PGAdmin) []string { // - https://www.pgadmin.org/docs/pgadmin4/development/server_deployment.html#standalone-gunicorn-configuration // - https://docs.gunicorn.org/en/latest/configure.html var startScript = fmt.Sprintf(` -export PGADMIN_SETUP_PASSWORD="admin" +export PGADMIN_SETUP_PASSWORD="$(date +%%s | sha256sum | base64 | head -c 32)" PGADMIN_DIR=%s APP_RELEASE=$(cd $PGADMIN_DIR && python3 -c "import config; print(config.APP_RELEASE)")