K8SPG-577: Add spec.pmm.querySource (#870)

inelpandzic · hors · egegunes · web-flow · commit 89107664c4ef · 2024-09-06T14:21:18.000+03:00
* Add spec.pmm.querySource.

* Set pgstatements as a default.

* Set pg_stat_monitor as a default. Load pg_stat_statements if enabled.

* Fix test.

* Fix

* Update CRDs.

* Update CRD validation.

* Fix config.

* Update e2e test.

* Fix

* Test fix

* Fix

* Update timeout.

* Properly set the extension.

* Update test.

* Remove unused param.

* Update CRDs

* Set querySource default to pgstatmonitor.

* fix defults in cr.yaml

* Update pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go

Co-authored-by: Ege Güneş &lt;ege.gunes@percona.com&gt;

---------

Co-authored-by: Viacheslav Sarzhan &lt;slava.sarzhan@percona.com&gt;
Co-authored-by: Ege Güneş &lt;ege.gunes@percona.com&gt;
diff --git a/build/crd/crunchy/generated/postgres-operator.crunchydata.com_postgresclusters.yaml b/build/crd/crunchy/generated/postgres-operator.crunchydata.com_postgresclusters.yaml
@@ -8224,6 +8224,8 @@ spec:
                     type: boolean
                   pgStatMonitor:
                     type: boolean
+                  pgStatStatements:
+                    type: boolean
                 type: object
               image:
                 description: |-
diff --git a/build/crd/percona/generated/pgv2.percona.com_perconapgclusters.yaml b/build/crd/percona/generated/pgv2.percona.com_perconapgclusters.yaml
@@ -13535,6 +13535,12 @@ spec:
                     - Never
                     - IfNotPresent
                     type: string
+                  querySource:
+                    default: pgstatmonitor
+                    enum:
+                    - pgstatmonitor
+                    - pgstatstatements
+                    type: string
                   resources:
                     description: Compute resources of a PMM container.
                     properties:
@@ -13603,6 +13609,7 @@ spec:
                 required:
                 - enabled
                 - image
+                - querySource
                 - secret
                 - serverHost
                 type: object
diff --git a/config/crd/bases/pgv2.percona.com_perconapgclusters.yaml b/config/crd/bases/pgv2.percona.com_perconapgclusters.yaml
@@ -13940,6 +13940,12 @@ spec:
                     - Never
                     - IfNotPresent
                     type: string
+                  querySource:
+                    default: pgstatmonitor
+                    enum:
+                    - pgstatmonitor
+                    - pgstatstatements
+                    type: string
                   resources:
                     description: Compute resources of a PMM container.
                     properties:
@@ -14008,6 +14014,7 @@ spec:
                 required:
                 - enabled
                 - image
+                - querySource
                 - secret
                 - serverHost
                 type: object
diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml
@@ -8184,6 +8184,8 @@ spec:
                     type: boolean
                   pgStatMonitor:
                     type: boolean
+                  pgStatStatements:
+                    type: boolean
                 type: object
               image:
                 description: |-
diff --git a/deploy/bundle.yaml b/deploy/bundle.yaml
@@ -14233,6 +14233,12 @@ spec:
                     - Never
                     - IfNotPresent
                     type: string
+                  querySource:
+                    default: pgstatmonitor
+                    enum:
+                    - pgstatmonitor
+                    - pgstatstatements
+                    type: string
                   resources:
                     description: Compute resources of a PMM container.
                     properties:
@@ -14301,6 +14307,7 @@ spec:
                 required:
                 - enabled
                 - image
+                - querySource
                 - secret
                 - serverHost
                 type: object
@@ -33661,6 +33668,8 @@ spec:
                     type: boolean
                   pgStatMonitor:
                     type: boolean
+                  pgStatStatements:
+                    type: boolean
                 type: object
               image:
                 description: |-
diff --git a/deploy/cr.yaml b/deploy/cr.yaml
@@ -448,6 +448,7 @@ spec:
 #    imagePullPolicy: IfNotPresent
     secret: cluster1-pmm-secret
     serverHost: monitoring-service
+#    querySource: pgstatmonitor
 #  patroni:
 #    # Some values of the Liveness/Readiness probes of the patroni container are calulated using syncPeriodSeconds by the following formulas:
 #    # - timeoutSeconds:   syncPeriodSeconds / 2;
diff --git a/deploy/crd.yaml b/deploy/crd.yaml
@@ -14233,6 +14233,12 @@ spec:
                     - Never
                     - IfNotPresent
                     type: string
+                  querySource:
+                    default: pgstatmonitor
+                    enum:
+                    - pgstatmonitor
+                    - pgstatstatements
+                    type: string
                   resources:
                     description: Compute resources of a PMM container.
                     properties:
@@ -14301,6 +14307,7 @@ spec:
                 required:
                 - enabled
                 - image
+                - querySource
                 - secret
                 - serverHost
                 type: object
@@ -33661,6 +33668,8 @@ spec:
                     type: boolean
                   pgStatMonitor:
                     type: boolean
+                  pgStatStatements:
+                    type: boolean
                 type: object
               image:
                 description: |-
diff --git a/deploy/cw-bundle.yaml b/deploy/cw-bundle.yaml
@@ -14233,6 +14233,12 @@ spec:
                     - Never
                     - IfNotPresent
                     type: string
+                  querySource:
+                    default: pgstatmonitor
+                    enum:
+                    - pgstatmonitor
+                    - pgstatstatements
+                    type: string
                   resources:
                     description: Compute resources of a PMM container.
                     properties:
@@ -14301,6 +14307,7 @@ spec:
                 required:
                 - enabled
                 - image
+                - querySource
                 - secret
                 - serverHost
                 type: object
@@ -33661,6 +33668,8 @@ spec:
                     type: boolean
                   pgStatMonitor:
                     type: boolean
+                  pgStatStatements:
+                    type: boolean
                 type: object
               image:
                 description: |-
diff --git a/e2e-tests/tests/monitoring/06-check-pgstatstatements-query-source.yaml b/e2e-tests/tests/monitoring/06-check-pgstatstatements-query-source.yaml
@@ -0,0 +1,17 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: |-
+      set -o errexit
+      set -o xtrace
+
+      source ../../functions
+
+      kubectl -n ${NAMESPACE} patch perconapgcluster/monitoring --type=merge -p '{"spec":{"pmm":{"querySource":"pgstatstatements"}}}'
+
+      # Wait for the pg_stat_statements extension to be created
+      sleep 60
+
+      primary=$(get_pod_by_role monitoring master name)
+      res=$(kubectl -n ${NAMESPACE} exec ${primary} -- bash -c 'psql -c "SELECT extname FROM pg_extension;"')
+      echo ${res} | grep -q pg_stat_statements
diff --git a/e2e-tests/tests/monitoring/07-check-password-leak.yaml b/e2e-tests/tests/monitoring/07-check-password-leak.yaml
diff --git a/e2e-tests/tests/monitoring/08-deleted-from-server-inventory.yaml b/e2e-tests/tests/monitoring/08-deleted-from-server-inventory.yaml
diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go
@@ -48,6 +48,7 @@ import (
 	"github.com/percona/percona-postgresql-operator/internal/pgbouncer"
 	"github.com/percona/percona-postgresql-operator/internal/pgmonitor"
 	"github.com/percona/percona-postgresql-operator/internal/pgstatmonitor"
+	"github.com/percona/percona-postgresql-operator/internal/pgstatstatements"
 	"github.com/percona/percona-postgresql-operator/internal/pki"
 	"github.com/percona/percona-postgresql-operator/internal/pmm"
 	"github.com/percona/percona-postgresql-operator/internal/postgres"
@@ -225,6 +226,9 @@ func (r *Reconciler) Reconcile(
 	if cluster.Spec.Extensions.PGAudit {
 		pgaudit.PostgreSQLParameters(&pgParameters)
 	}
+	if cluster.Spec.Extensions.PGStatStatements {
+		pgstatstatements.PostgreSQLParameters(&pgParameters)
+	}
 	pgbackrest.PostgreSQL(cluster, &pgParameters)
 	pgmonitor.PostgreSQLParameters(cluster, &pgParameters)
 
diff --git a/internal/controller/postgrescluster/postgres.go b/internal/controller/postgrescluster/postgres.go
@@ -41,6 +41,7 @@ import (
 	"github.com/percona/percona-postgresql-operator/internal/naming"
 	"github.com/percona/percona-postgresql-operator/internal/pgaudit"
 	"github.com/percona/percona-postgresql-operator/internal/pgstatmonitor"
+	"github.com/percona/percona-postgresql-operator/internal/pgstatstatements"
 	"github.com/percona/percona-postgresql-operator/internal/postgis"
 	"github.com/percona/percona-postgresql-operator/internal/postgres"
 	pgpassword "github.com/percona/percona-postgresql-operator/internal/postgres/password"
@@ -242,7 +243,7 @@ func (r *Reconciler) reconcilePostgresDatabases(
 
 	// Calculate a hash of the SQL that should be executed in PostgreSQL.
 
-	var pgAuditOK, pgStatMonitorOK, postgisInstallOK bool
+	var pgAuditOK, pgStatMonitorOK, pgStatStatementsOK, postgisInstallOK bool
 	create := func(ctx context.Context, exec postgres.Executor) error {
 		if cluster.Spec.Extensions.PGStatMonitor {
 			if pgStatMonitorOK = pgstatmonitor.EnableInPostgreSQL(ctx, exec) == nil; !pgStatMonitorOK {
@@ -276,6 +277,18 @@ func (r *Reconciler) reconcilePostgresDatabases(
 			}
 		}
 
+		if cluster.Spec.Extensions.PGStatStatements {
+			if pgStatStatementsOK = pgstatstatements.EnableInPostgreSQL(ctx, exec) == nil; !pgStatStatementsOK {
+				r.Recorder.Event(cluster, corev1.EventTypeWarning, "pgStatStatementsDisabled",
+					"Unable to install pgStatStatements")
+			}
+		} else {
+			if pgStatStatementsOK = pgstatstatements.DisableInPostgreSQL(ctx, exec) == nil; !pgStatStatementsOK {
+				r.Recorder.Event(cluster, corev1.EventTypeWarning, "pgStatStatementsEnabled",
+					"Unable to disable pgStatStatements")
+			}
+		}
+
 		// Enabling PostGIS extensions is a one-way operation
 		// e.g., you can take a PostgresCluster and turn it into a PostGISCluster,
 		// but you cannot reverse the process, as that would potentially remove an extension
diff --git a/internal/pgstatstatements/pgstatstatement.go b/internal/pgstatstatements/pgstatstatement.go
@@ -0,0 +1,44 @@
+package pgstatstatements
+
+import (
+	"context"
+
+	"github.com/percona/percona-postgresql-operator/internal/logging"
+	"github.com/percona/percona-postgresql-operator/internal/postgres"
+)
+
+func EnableInPostgreSQL(ctx context.Context, exec postgres.Executor) error {
+	log := logging.FromContext(ctx)
+
+	stdout, stderr, err := exec.ExecInAllDatabases(ctx,
+		`SET client_min_messages = WARNING; CREATE EXTENSION IF NOT EXISTS pg_stat_statements;`,
+		map[string]string{
+			"ON_ERROR_STOP": "on", // Abort when any one command fails.
+			"QUIET":         "on", // Do not print successful commands to stdout.
+		})
+
+	log.V(1).Info("enabled pg_stat_statements", "stdout", stdout, "stderr", stderr)
+
+	return err
+}
+
+func DisableInPostgreSQL(ctx context.Context, exec postgres.Executor) error {
+	log := logging.FromContext(ctx)
+
+	stdout, stderr, err := exec.ExecInAllDatabases(ctx,
+		`SET client_min_messages = WARNING; DROP EXTENSION IF EXISTS pg_stat_statements;`,
+		map[string]string{
+			"ON_ERROR_STOP": "on", // Abort when any one command fails.
+			"QUIET":         "on", // Do not print successful commands to stdout.
+		})
+
+	log.V(1).Info("disabled pg_stat_statements", "stdout", stdout, "stderr", stderr)
+
+	return err
+}
+
+func PostgreSQLParameters(outParameters *postgres.Parameters) {
+
+	outParameters.Mandatory.AppendToList("shared_preload_libraries", "pg_stat_statements")
+	outParameters.Mandatory.Add("pg_stat_statements.track", "all")
+}
diff --git a/internal/pgstatstatements/pgstatstatement_test.go b/internal/pgstatstatements/pgstatstatement_test.go
@@ -0,0 +1,88 @@
+package pgstatstatements
+
+import (
+	"context"
+	"errors"
+	"io"
+	"strings"
+	"testing"
+
+	"gotest.tools/v3/assert"
+
+	"github.com/percona/percona-postgresql-operator/internal/postgres"
+)
+
+func TestEnableInPostgreSQL(t *testing.T) {
+	expected := errors.New("whoops")
+	exec := func(
+		_ context.Context, stdin io.Reader, stdout, stderr io.Writer, command ...string,
+	) error {
+		assert.Assert(t, stdout != nil, "should capture stdout")
+		assert.Assert(t, stderr != nil, "should capture stderr")
+
+		assert.Assert(t, strings.Contains(strings.Join(command, "\n"),
+			`SELECT datname FROM pg_catalog.pg_database`,
+		), "expected all databases and templates")
+
+		b, err := io.ReadAll(stdin)
+		assert.NilError(t, err)
+		assert.Equal(t, string(b), strings.Trim(`
+SET client_min_messages = WARNING; CREATE EXTENSION IF NOT EXISTS pg_stat_statements;
+		`, "\t\n"))
+
+		return expected
+	}
+
+	ctx := context.Background()
+	assert.Equal(t, expected, EnableInPostgreSQL(ctx, exec))
+}
+
+func TestDisableInPostgreSQL(t *testing.T) {
+	expected := errors.New("whoops")
+	exec := func(
+		_ context.Context, stdin io.Reader, stdout, stderr io.Writer, command ...string,
+	) error {
+		assert.Assert(t, stdout != nil, "should capture stdout")
+		assert.Assert(t, stderr != nil, "should capture stderr")
+
+		assert.Assert(t, strings.Contains(strings.Join(command, "\n"),
+			`SELECT datname FROM pg_catalog.pg_database`,
+		), "expected all databases and templates")
+
+		b, err := io.ReadAll(stdin)
+		assert.NilError(t, err)
+		assert.Equal(t, string(b), strings.Trim(`
+SET client_min_messages = WARNING; DROP EXTENSION IF EXISTS pg_stat_statements;
+		`, "\t\n"))
+
+		return expected
+	}
+
+	ctx := context.Background()
+	assert.Equal(t, expected, DisableInPostgreSQL(ctx, exec))
+}
+
+func TestPostgreSQLParameters(t *testing.T) {
+	parameters := postgres.Parameters{
+		Mandatory: postgres.NewParameterSet(),
+	}
+
+	// No comma when empty.
+	PostgreSQLParameters(&parameters)
+
+	assert.Assert(t, parameters.Default == nil)
+	assert.DeepEqual(t, parameters.Mandatory.AsMap(), map[string]string{
+		"pg_stat_statements.track": "all",
+		"shared_preload_libraries": "pg_stat_statements",
+	})
+
+	// Appended when not empty.
+	parameters.Mandatory.Add("shared_preload_libraries", "some,existing")
+	PostgreSQLParameters(&parameters)
+
+	assert.Assert(t, parameters.Default == nil)
+	assert.DeepEqual(t, parameters.Mandatory.AsMap(), map[string]string{
+		"pg_stat_statements.track": "all",
+		"shared_preload_libraries": "some,existing,pg_stat_statements",
+	})
+}
diff --git a/percona/controller/pgcluster/controller.go b/percona/controller/pgcluster/controller.go
@@ -225,7 +225,7 @@ func (r *PGClusterReconciler) Reconcile(ctx context.Context, request reconcile.R
 		return reconcile.Result{}, errors.Wrap(err, "reconcile backups")
 	}
 
-	if err := r.addPMMSidecar(ctx, cr); err != nil {
+	if err := r.reconcilePMM(ctx, cr); err != nil {
 		return reconcile.Result{}, errors.Wrap(err, "failed to add pmm sidecar")
 	}
 
@@ -278,7 +278,7 @@ func (r *PGClusterReconciler) Reconcile(ctx context.Context, request reconcile.R
 	return ctrl.Result{}, nil
 }
 
-func (r *PGClusterReconciler) addPMMSidecar(ctx context.Context, cr *v2.PerconaPGCluster) error {
+func (r *PGClusterReconciler) reconcilePMM(ctx context.Context, cr *v2.PerconaPGCluster) error {
 	if !cr.PMMEnabled() {
 		return nil
 	}
diff --git a/percona/pmm/pmm.go b/percona/pmm/pmm.go
diff --git a/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go b/pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go
diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go
