PGO updates pgnodemx/pg_stat_statements (#3400)

* PGO updates pgnodemx/pg_stat_statements

Users reported that an updated image wouldn't trigger
an update of monitoring extensions. This changes that behavior by

* adding the monitor and pg image tags to the revision hash,
* adding update lines to the pgmonitor enable action.

Note: this _only_ targets these two extensions as updating other
extensions should probably be under the user's power.

Issue: [sc-14476]

* add KUTTL test for exporter upgrade errors

Author: benjaminjb

internal/controller/postgrescluster/pgmonitor.go

@@ -78,16 +78,24 @@ func (r *Reconciler) reconcilePGMonitorExporter(ctx context.Context,
 		writableInstance *Instance
 		writablePod      *corev1.Pod
 		setup            string
+		pgImageSHA       string
+		exporterImageSHA string
 	)
 
 	// Find the PostgreSQL instance that can execute SQL that writes to every
 	// database. When there is none, return early.
-
 	writablePod, writableInstance = instances.writablePod(naming.ContainerDatabase)
 	if writableInstance == nil || writablePod == nil {
 		return nil
 	}
 
+	// For the writableInstance found above
+	// 1) make sure the `exporter` container is running
+	// 2) get and save the imageIDs for the `exporter` and `database` containers, and
+	// 3) exit early if we can't get the ImageID of either of those containers.
+	// We use these ImageIDs in the hash we make to see if the operator needs to rerun
+	// the `EnableExporterInPostgreSQL` funcs; that way we are always running
+	// that function against an updated and running pod.
 	if pgmonitor.ExporterEnabled(cluster) {
 		running, known := writableInstance.IsRunning(naming.ContainerPGMonitorExporter)
 		if !running || !known {
@@ -97,11 +105,15 @@ func (r *Reconciler) reconcilePGMonitorExporter(ctx context.Context,
 
 		for _, containerStatus := range writablePod.Status.ContainerStatuses {
 			if containerStatus.Name == naming.ContainerPGMonitorExporter {
-				setup = containerStatus.ImageID
+				exporterImageSHA = containerStatus.ImageID
+			}
+			if containerStatus.Name == naming.ContainerDatabase {
+				pgImageSHA = containerStatus.ImageID
 			}
 		}
-		if setup == "" {
-			// Could not get exporter container imageID
+
+		// Could not get container imageIDs
+		if exporterImageSHA == "" || pgImageSHA == "" {
 			return nil
 		}
 	}
@@ -127,11 +139,13 @@ func (r *Reconciler) reconcilePGMonitorExporter(ctx context.Context,
 		) error {
 			_, err := io.Copy(hasher, stdin)
 			if err == nil {
-				_, err = fmt.Fprint(hasher, command)
+				// Use command and image tag in hash to execute hash on image update
+				_, err = fmt.Fprint(hasher, command, pgImageSHA, exporterImageSHA)
 			}
 			return err
 		})
 	})
+
 	if err != nil {
 		return err
 	}
@@ -149,7 +163,6 @@ func (r *Reconciler) reconcilePGMonitorExporter(ctx context.Context,
 		}
 
 		// Apply the necessary SQL and record its hash in cluster.Status
-
 		if err == nil {
 			err = action(ctx, func(_ context.Context, stdin io.Reader,
 				stdout, stderr io.Writer, command ...string) error {

internal/controller/postgrescluster/pgmonitor_test.go

@@ -317,8 +317,9 @@ func TestReconcilePGMonitorExporterSetupErrors(t *testing.T) {
 					},
 					Status: corev1.PodStatus{
 						ContainerStatuses: []corev1.ContainerStatus{{
-							Name:  naming.ContainerDatabase,
-							State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}},
+							Name:    naming.ContainerDatabase,
+							State:   corev1.ContainerState{Running: &corev1.ContainerStateRunning{}},
+							ImageID: "image@sha123",
 						}, {
 							Name:    naming.ContainerPGMonitorExporter,
 							State:   corev1.ContainerState{Running: &corev1.ContainerStateRunning{}},
@@ -437,7 +438,7 @@ func TestReconcilePGMonitorExporterStatus(t *testing.T) {
 		podExecCalled:   false,
 		// Status was generated manually for this test case
 		// TODO jmckulk: add code to generate status
-		status:                      v1beta1.MonitoringStatus{ExporterConfiguration: "5f599686cf"},
+		status:                      v1beta1.MonitoringStatus{ExporterConfiguration: "5dbc557689"},
 		statusChangedAfterReconcile: false,
 	}} {
 		t.Run(test.name, func(t *testing.T) {
@@ -468,8 +469,9 @@ func TestReconcilePGMonitorExporterStatus(t *testing.T) {
 						},
 						Status: corev1.PodStatus{
 							ContainerStatuses: []corev1.ContainerStatus{{
-								Name:  naming.ContainerDatabase,
-								State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}},
+								Name:    naming.ContainerDatabase,
+								State:   corev1.ContainerState{Running: &corev1.ContainerStateRunning{}},
+								ImageID: "image@sha123",
 							}},
 						},
 					}},

internal/pgmonitor/postgres.go

@@ -102,6 +102,9 @@ func EnableExporterInPostgreSQL(ctx context.Context, exec postgres.Executor,
 			// Exporter expects that extension(s) to be installed in all databases
 			// pg_stat_statements: https://access.crunchydata.com/documentation/pgmonitor/latest/exporter/
 			"CREATE EXTENSION IF NOT EXISTS pg_stat_statements;",
+
+			// Run idempotent update
+			"ALTER EXTENSION pg_stat_statements UPDATE;",
 		}, "\n"),
 		map[string]string{
 			"ON_ERROR_STOP": "on", // Abort when any one statement fails.
@@ -130,6 +133,9 @@ func EnableExporterInPostgreSQL(ctx context.Context, exec postgres.Executor,
 				// https://github.com/CrunchyData/pgmonitor/blob/master/postgres_exporter/common/queries_nodemx.yml
 				"CREATE EXTENSION IF NOT EXISTS pgnodemx WITH SCHEMA monitor;",
 
+				// Run idempotent update
+				"ALTER EXTENSION pgnodemx UPDATE;",
+
 				// ccp_monitoring user is created in Setup.sql without a
 				// password; update the password and ensure that the ROLE
 				// can login to the database

testing/kuttl/e2e-other/exporter-upgrade/00--cluster.yaml

@@ -0,0 +1,30 @@
+apiVersion: postgres-operator.crunchydata.com/v1beta1
+kind: PostgresCluster
+metadata:
+  name: exporter
+spec:
+  postgresVersion: 14
+  image: us.gcr.io/container-suite/crunchy-postgres:ubi8-14.0-5.0.3-0
+  instances:
+    - name: instance1
+      dataVolumeClaimSpec:
+        accessModes:
+        - "ReadWriteOnce"
+        resources:
+          requests:
+            storage: 1Gi
+  backups:
+    pgbackrest:
+      repos:
+      - name: repo1
+        volume:
+          volumeClaimSpec:
+            accessModes:
+            - "ReadWriteOnce"
+            resources:
+              requests:
+                storage: 1Gi
+  monitoring:
+    pgmonitor:
+      exporter:
+        image: registry.developers.crunchydata.com/crunchydata/crunchy-postgres-exporter:ubi8-5.2.0-0

testing/kuttl/e2e-other/exporter-upgrade/00-assert.yaml

@@ -0,0 +1,10 @@
+apiVersion: postgres-operator.crunchydata.com/v1beta1
+kind: PostgresCluster
+metadata:
+  name: exporter
+status:
+  instances:
+    - name: instance1
+      readyReplicas: 1
+      replicas: 1
+      updatedReplicas: 1

testing/kuttl/e2e-other/exporter-upgrade/01--check-exporter.yaml

@@ -0,0 +1,31 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: |
+      set -e
+      PRIMARY=$(
+        kubectl get pod --namespace "${NAMESPACE}" \
+          --output name --selector '
+            postgres-operator.crunchydata.com/cluster=exporter,
+            postgres-operator.crunchydata.com/role=master'
+      )
+
+      # Ensure that the metrics endpoint is available from inside the exporter container
+      for i in {1..5}; do
+        kubectl exec --namespace "${NAMESPACE}" "${PRIMARY}" -c exporter -- curl http://localhost:9187/metrics
+        sleep 2
+      done
+
+      # Ensure that the monitoring user exists and is configured.
+      kubectl exec --stdin --namespace "${NAMESPACE}" "${PRIMARY}" \
+        -- psql -qb --set ON_ERROR_STOP=1 --file=- <<'SQL'
+        DO $$
+        DECLARE
+          result record;
+        BEGIN
+          SELECT * INTO result FROM pg_catalog.pg_roles WHERE rolname = 'ccp_monitoring';
+          ASSERT FOUND, 'user not found';
+          ASSERT result.rolconfig @> '{jit=off}', format('got config: %L', result.rolconfig);
+        END $$
+      SQL

testing/kuttl/e2e-other/exporter-upgrade/02--update-cluster.yaml

@@ -0,0 +1,7 @@
+apiVersion: postgres-operator.crunchydata.com/v1beta1
+kind: PostgresCluster
+metadata:
+  name: exporter
+spec:
+  postgresVersion: 14
+  image: registry.developers.crunchydata.com/crunchydata/crunchy-postgres:ubi8-14.5-1

testing/kuttl/e2e-other/exporter-upgrade/02-assert.yaml

@@ -0,0 +1,24 @@
+apiVersion: postgres-operator.crunchydata.com/v1beta1
+kind: PostgresCluster
+metadata:
+  name: exporter
+status:
+  instances:
+    - name: instance1
+      readyReplicas: 1
+      replicas: 1
+      updatedReplicas: 1
+---
+apiVersion: batch/v1
+kind: Job
+metadata:
+  labels:
+    postgres-operator.crunchydata.com/cluster: exporter
+    postgres-operator.crunchydata.com/pgbackrest-backup: replica-create
+status:
+  succeeded: 1
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: exporter-primary

testing/kuttl/e2e-other/exporter-upgrade/03--check-exporter.yaml

@@ -0,0 +1,22 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: |
+      set -e
+      PRIMARY=$(
+        kubectl get pod --namespace "${NAMESPACE}" \
+          --output name --selector '
+            postgres-operator.crunchydata.com/cluster=exporter,
+            postgres-operator.crunchydata.com/role=master'
+      )
+
+      # Get errors from the exporter
+      # See the README.md for a discussion of these errors
+      ERR=$(kubectl logs --namespace "${NAMESPACE}" "${PRIMARY}" -c exporter | grep -e "Error running query on database")
+      ERR_COUNT=$(echo "$ERR" | wc -l)
+
+      if [[ "$ERR_COUNT" -gt 2 ]]; then
+        echo "Errors in log from exporter: ${ERR}"
+        exit 1
+      fi

testing/kuttl/e2e-other/exporter-upgrade/README.md

@@ -0,0 +1,31 @@
+The exporter-upgrade test makes sure that PGO updates an extension used for monitoring. This
+avoids an error where a user might update to a new PG image with a newer extension, but with an
+older extension operative.
+
+Note: This test relies on two `crunchy-postgres` images with known, different `pgnodemx` extensions:
+the image created in 00--cluster.yaml has `pgnodemx` 1.1; the image we update the cluster to in 
+02--update-cluster.yaml has `pgnodemx` 1.3. 
+
+00-01
+This starts up a cluster with a purposely outdated `pgnodemx` extension. Because we want a specific
+extension, the image used here is hard-coded (and so outdated it's not publicly available).
+
+(This image is so outdated that it doesn't finish creating a backup with the current PGO, which is 
+why the 00-assert.yaml only checks that the pod is ready; and why 01--check-exporter.yaml wraps the 
+call in a retry loop.)
+
+02-03
+The cluster is updated with a newer (and hardcoded) image with a newer version of `pgnodemx`. Due
+to the change made in https://github.com/CrunchyData/postgres-operator/pull/3400, this should no 
+longer produce multiple errors.
+
+Note: a few errors may be logged after the `exporter` container attempts to run the `pgnodemx`
+functions but before the extension is updated. So this checks that there are no more than 2 errors,
+since that was the observed maximum number of printed errors during manual tests of the check.
+
+For instance, using these hardcoded images (with `pgnodemx` versions 1.1 and 1.3), those errors were: 
+
+```
+Error running query on database \"localhost:5432\": ccp_nodemx_disk_activity pq: query-specified return tuple and function return type are not compatible" 
+Error running query on database \"localhost:5432\": ccp_nodemx_data_disk pq: query-specified return tuple and function return type are not compatible
+```