From 61014a618d412026ff98cbf0c597a97bb8a3669e Mon Sep 17 00:00:00 2001 From: Anthony Landreth Date: Wed, 24 Jul 2024 13:49:16 -0400 Subject: [PATCH 1/4] archive-async by default with spool-path Issue: PGO-1371 PGO-1142 --- .../postgrescluster/pgbackrest_test.go | 8 ++++---- internal/pgbackrest/config.go | 4 ++++ internal/pgbackrest/config_test.go | 2 ++ internal/postgres/config.go | 19 +++++++++++++++---- internal/postgres/reconcile_test.go | 2 ++ .../pgbackrest-init/05--check-spool-path.yaml | 16 ++++++++++++++++ .../06--check-spool-path.yaml | 17 +++++++++++++++++ 7 files changed, 60 insertions(+), 8 deletions(-) create mode 100644 testing/kuttl/e2e/pgbackrest-init/05--check-spool-path.yaml create mode 100644 testing/kuttl/e2e/wal-pvc-pgupgrade/06--check-spool-path.yaml diff --git a/internal/controller/postgrescluster/pgbackrest_test.go b/internal/controller/postgrescluster/pgbackrest_test.go index 8ca6a08b01..40c3ecf034 100644 --- a/internal/controller/postgrescluster/pgbackrest_test.go +++ b/internal/controller/postgrescluster/pgbackrest_test.go @@ -2072,7 +2072,7 @@ func TestReconcileCloudBasedDataSource(t *testing.T) { result: testResult{ configCount: 1, jobCount: 1, pvcCount: 1, expectedClusterCondition: nil, - conf: "|\n # Generated by postgres-operator. DO NOT EDIT.\n # Your changes will not be saved.\n\n [global]\n log-path = /pgdata/pgbackrest/log\n repo1-path = /pgbackrest/repo1\n\n [db]\n pg1-path = /pgdata/pg13\n pg1-port = 5432\n pg1-socket-path = /tmp/postgres\n", + conf: "|\n # Generated by postgres-operator. DO NOT EDIT.\n # Your changes will not be saved.\n\n [global]\n archive-async = y\n log-path = /pgdata/pgbackrest/log\n repo1-path = /pgbackrest/repo1\n spool-path = /pgdata/pgbackrest-spool\n\n [db]\n pg1-path = /pgdata/pg13\n pg1-port = 5432\n pg1-socket-path = /tmp/postgres\n", }, }, { desc: "global/configuration set", @@ -2089,7 +2089,7 @@ func TestReconcileCloudBasedDataSource(t *testing.T) { result: testResult{ configCount: 1, jobCount: 1, pvcCount: 1, expectedClusterCondition: nil, - conf: "|\n # Generated by postgres-operator. DO NOT EDIT.\n # Your changes will not be saved.\n\n [global]\n log-path = /pgdata/pgbackrest/log\n repo1-path = elephant\n\n [db]\n pg1-path = /pgdata/pg13\n pg1-port = 5432\n pg1-socket-path = /tmp/postgres\n", + conf: "|\n # Generated by postgres-operator. DO NOT EDIT.\n # Your changes will not be saved.\n\n [global]\n archive-async = y\n log-path = /pgdata/pgbackrest/log\n repo1-path = elephant\n spool-path = /pgdata/pgbackrest-spool\n\n [db]\n pg1-path = /pgdata/pg13\n pg1-port = 5432\n pg1-socket-path = /tmp/postgres\n", }, }, { desc: "invalid option: stanza", @@ -2104,7 +2104,7 @@ func TestReconcileCloudBasedDataSource(t *testing.T) { result: testResult{ configCount: 1, jobCount: 0, pvcCount: 1, expectedClusterCondition: nil, - conf: "|\n # Generated by postgres-operator. DO NOT EDIT.\n # Your changes will not be saved.\n\n [global]\n log-path = /pgdata/pgbackrest/log\n repo1-path = /pgbackrest/repo1\n\n [db]\n pg1-path = /pgdata/pg13\n pg1-port = 5432\n pg1-socket-path = /tmp/postgres\n", + conf: "|\n # Generated by postgres-operator. DO NOT EDIT.\n # Your changes will not be saved.\n\n [global]\n archive-async = y\n log-path = /pgdata/pgbackrest/log\n repo1-path = /pgbackrest/repo1\n spool-path = /pgdata/pgbackrest-spool\n\n [db]\n pg1-path = /pgdata/pg13\n pg1-port = 5432\n pg1-socket-path = /tmp/postgres\n", }, }, { desc: "cluster bootstrapped init condition missing", @@ -2123,7 +2123,7 @@ func TestReconcileCloudBasedDataSource(t *testing.T) { Reason: "ClusterAlreadyBootstrapped", Message: "The cluster is already bootstrapped", }, - conf: "|\n # Generated by postgres-operator. DO NOT EDIT.\n # Your changes will not be saved.\n\n [global]\n log-path = /pgdata/pgbackrest/log\n repo1-path = /pgbackrest/repo1\n\n [db]\n pg1-path = /pgdata/pg13\n pg1-port = 5432\n pg1-socket-path = /tmp/postgres\n", + conf: "|\n # Generated by postgres-operator. DO NOT EDIT.\n # Your changes will not be saved.\n\n [global]\n archive-async = y\nlog-path = /pgdata/pgbackrest/log\n repo1-path = /pgbackrest/repo1\n spool-path = /pgdata/pgbackrest-spool\n\n [db]\n pg1-path = /pgdata/pg13\n pg1-port = 5432\n pg1-socket-path = /tmp/postgres\n", }, }} diff --git a/internal/pgbackrest/config.go b/internal/pgbackrest/config.go index ba2abafd2f..199a399f73 100644 --- a/internal/pgbackrest/config.go +++ b/internal/pgbackrest/config.go @@ -291,6 +291,10 @@ func populatePGInstanceConfigurationMap( global := iniMultiSet{} stanza := iniMultiSet{} + // For faster and more robust WAL archiving, we turn on pgBackRest archive-async. + global.Set("archive-async", "y") + // pgBackRest spool-path should always be co-located with the Postgres WAL path. + global.Set("spool-path", "/pgdata/pgbackrest-spool") // pgBackRest will log to the pgData volume for commands run on the PostgreSQL instance global.Set("log-path", naming.PGBackRestPGDataLogPath) diff --git a/internal/pgbackrest/config_test.go b/internal/pgbackrest/config_test.go index c6f7f9ed02..a518e95299 100644 --- a/internal/pgbackrest/config_test.go +++ b/internal/pgbackrest/config_test.go @@ -131,6 +131,7 @@ pg1-socket-path = /tmp/postgres # Your changes will not be saved. [global] +archive-async = y log-path = /pgdata/pgbackrest/log repo1-host = repo-hostname-0.pod-service-name.test-ns.svc.`+domain+` repo1-host-ca-file = /etc/pgbackrest/conf.d/~postgres-operator/tls-ca.crt @@ -151,6 +152,7 @@ repo4-s3-bucket = s-bucket repo4-s3-endpoint = endpoint-s repo4-s3-region = earth repo4-type = s3 +spool-path = /pgdata/pgbackrest-spool [db] pg1-path = /pgdata/pg12 diff --git a/internal/postgres/config.go b/internal/postgres/config.go index e9ea18b56d..6eb876c0c7 100644 --- a/internal/postgres/config.go +++ b/internal/postgres/config.go @@ -103,12 +103,17 @@ func DataDirectory(cluster *v1beta1.PostgresCluster) string { func WALDirectory( cluster *v1beta1.PostgresCluster, instance *v1beta1.PostgresInstanceSetSpec, ) string { - // When no WAL volume is specified, store WAL files on the main data volume. - walStorage := dataMountPath + return fmt.Sprintf("%s/pg%d_wal", WALStorage(instance), cluster.Spec.PostgresVersion) +} + +// WALStorage returns the absolute path to the disk where an instance stores its +// WAL files. Use [WALDirectory] for the exact directory that Postgres uses. +func WALStorage(instance *v1beta1.PostgresInstanceSetSpec) string { if instance.WALVolumeClaimSpec != nil { - walStorage = walMountPath + return walMountPath } - return fmt.Sprintf("%s/pg%d_wal", walStorage, cluster.Spec.PostgresVersion) + // When no WAL volume is specified, store WAL files on the main data volume. + return dataMountPath } // Environment returns the environment variables required to invoke PostgreSQL @@ -307,6 +312,12 @@ chmod +x /tmp/pg_rewind_tde.sh `echo Initializing ...`, `results 'uid' "$(id -u ||:)" 'gid' "$(id -G ||:)"`, + // The pgbackrest spool path should be co-located with wal. + // If a wal volume exists, link the spool-path to it. + `if [[ "${pgwal_directory}" == *"pgwal/"* ]] && [[ ! -d "/pgwal/pgbackrest-spool" ]];then rm -rf "/pgdata/pgbackrest-spool" && mkdir -p "/pgwal/pgbackrest-spool" && ln --force --symbolic "/pgwal/pgbackrest-spool" "/pgdata/pgbackrest-spool";fi`, + // When a pgwal volume is removed, force pgbackrest to recreate spool-path. + `if [[ ! "${pgwal_directory}" == *"pgwal/"* ]];then rm -rf /pgdata/pgbackrest-spool;fi`, + // Abort when the PostgreSQL version installed in the image does not // match the cluster spec. `results 'postgres path' "$(command -v postgres ||:)"`, diff --git a/internal/postgres/reconcile_test.go b/internal/postgres/reconcile_test.go index de5dfb0d30..89d8e24a28 100644 --- a/internal/postgres/reconcile_test.go +++ b/internal/postgres/reconcile_test.go @@ -255,6 +255,8 @@ initContainers: ) echo Initializing ... results 'uid' "$(id -u ||:)" 'gid' "$(id -G ||:)" + if [[ "${pgwal_directory}" == *"pgwal/"* ]] && [[ ! -d "/pgwal/pgbackrest-spool" ]];then rm -rf "/pgdata/pgbackrest-spool" && mkdir -p "/pgwal/pgbackrest-spool" && ln --force --symbolic "/pgwal/pgbackrest-spool" "/pgdata/pgbackrest-spool";fi + if [[ ! "${pgwal_directory}" == *"pgwal/"* ]];then rm -rf /pgdata/pgbackrest-spool;fi results 'postgres path' "$(command -v postgres ||:)" results 'postgres version' "${postgres_version:=$(postgres --version ||:)}" [[ "${postgres_version}" =~ ") ${expected_major_version}"($|[^0-9]) ]] || diff --git a/testing/kuttl/e2e/pgbackrest-init/05--check-spool-path.yaml b/testing/kuttl/e2e/pgbackrest-init/05--check-spool-path.yaml new file mode 100644 index 0000000000..0595ec0963 --- /dev/null +++ b/testing/kuttl/e2e/pgbackrest-init/05--check-spool-path.yaml @@ -0,0 +1,16 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: +- script: | + PRIMARY=$( + kubectl get pod --namespace "${NAMESPACE}" \ + --output name --selector ' + postgres-operator.crunchydata.com/role=master' + ) + + LIST=$( + kubectl exec --namespace "${NAMESPACE}" -c database "${PRIMARY}" -- \ + ls -l /pgdata + ) + + [[ "$LIST" =~ "pgbackrest-spool" ]] || exit 1 diff --git a/testing/kuttl/e2e/wal-pvc-pgupgrade/06--check-spool-path.yaml b/testing/kuttl/e2e/wal-pvc-pgupgrade/06--check-spool-path.yaml new file mode 100644 index 0000000000..ca00bd31fe --- /dev/null +++ b/testing/kuttl/e2e/wal-pvc-pgupgrade/06--check-spool-path.yaml @@ -0,0 +1,17 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: +- script: | + PRIMARY=$( + kubectl get pod --namespace "${NAMESPACE}" \ + --output name --selector ' + postgres-operator.crunchydata.com/role=master' + ) + + LIST=$( + kubectl exec --namespace "${NAMESPACE}" -c database "${PRIMARY}" -- \ + ls -l /pgdata + ) + + # Confirm that the pgbackrest spool-path has been symlinked to the wal volume. + [[ $LIST == *"pgbackrest-spool -> /pgwal/pgbackrest-spool"* ]] || exit 1 From 31a3274e7ac171eac7b9b420c12ce7a45755245c Mon Sep 17 00:00:00 2001 From: Anthony Landreth Date: Mon, 29 Jul 2024 16:55:17 -0400 Subject: [PATCH 2/4] shell-agnostic tests --- .../{05--check-spool-path.yaml => 06--check-spool-path.yaml} | 3 ++- testing/kuttl/e2e/wal-pvc-pgupgrade/06--check-spool-path.yaml | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) rename testing/kuttl/e2e/pgbackrest-init/{05--check-spool-path.yaml => 06--check-spool-path.yaml} (76%) diff --git a/testing/kuttl/e2e/pgbackrest-init/05--check-spool-path.yaml b/testing/kuttl/e2e/pgbackrest-init/06--check-spool-path.yaml similarity index 76% rename from testing/kuttl/e2e/pgbackrest-init/05--check-spool-path.yaml rename to testing/kuttl/e2e/pgbackrest-init/06--check-spool-path.yaml index 0595ec0963..e32cc2fc87 100644 --- a/testing/kuttl/e2e/pgbackrest-init/05--check-spool-path.yaml +++ b/testing/kuttl/e2e/pgbackrest-init/06--check-spool-path.yaml @@ -13,4 +13,5 @@ commands: ls -l /pgdata ) - [[ "$LIST" =~ "pgbackrest-spool" ]] || exit 1 + contains() { bash -ceu '[[ "$1" == *"$2"* ]]' - "$@"; } + contains "$LIST" "pgbackrest-spool" || exit 1 diff --git a/testing/kuttl/e2e/wal-pvc-pgupgrade/06--check-spool-path.yaml b/testing/kuttl/e2e/wal-pvc-pgupgrade/06--check-spool-path.yaml index ca00bd31fe..4b52bce16e 100644 --- a/testing/kuttl/e2e/wal-pvc-pgupgrade/06--check-spool-path.yaml +++ b/testing/kuttl/e2e/wal-pvc-pgupgrade/06--check-spool-path.yaml @@ -13,5 +13,7 @@ commands: ls -l /pgdata ) + contains() { bash -ceu '[[ "$1" == *"$2"* ]]' - "$@"; } + # Confirm that the pgbackrest spool-path has been symlinked to the wal volume. - [[ $LIST == *"pgbackrest-spool -> /pgwal/pgbackrest-spool"* ]] || exit 1 + contains "$LIST" "pgbackrest-spool -> /pgwal/pgbackrest-spool" || exit 1 From 800a8cd308472210925757ad9771a8bca7385a29 Mon Sep 17 00:00:00 2001 From: Anthony Landreth Date: Tue, 30 Jul 2024 11:59:17 -0400 Subject: [PATCH 3/4] delete broken symlink --- internal/postgres/config.go | 7 +++---- internal/postgres/reconcile_test.go | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/internal/postgres/config.go b/internal/postgres/config.go index 6eb876c0c7..2063b09112 100644 --- a/internal/postgres/config.go +++ b/internal/postgres/config.go @@ -312,11 +312,10 @@ chmod +x /tmp/pg_rewind_tde.sh `echo Initializing ...`, `results 'uid' "$(id -u ||:)" 'gid' "$(id -G ||:)"`, - // The pgbackrest spool path should be co-located with wal. - // If a wal volume exists, link the spool-path to it. + // The pgbackrest spool path should be co-located with wal. If a wal volume exists, symlink the spool-path to it. `if [[ "${pgwal_directory}" == *"pgwal/"* ]] && [[ ! -d "/pgwal/pgbackrest-spool" ]];then rm -rf "/pgdata/pgbackrest-spool" && mkdir -p "/pgwal/pgbackrest-spool" && ln --force --symbolic "/pgwal/pgbackrest-spool" "/pgdata/pgbackrest-spool";fi`, - // When a pgwal volume is removed, force pgbackrest to recreate spool-path. - `if [[ ! "${pgwal_directory}" == *"pgwal/"* ]];then rm -rf /pgdata/pgbackrest-spool;fi`, + // When a pgwal volume is removed, the symlink will be broken; force pgbackrest to recreate spool-path. + `if [[ ! -e "/pgdata/pgbackrest-spool" ]];then rm -rf /pgdata/pgbackrest-spool;fi`, // Abort when the PostgreSQL version installed in the image does not // match the cluster spec. diff --git a/internal/postgres/reconcile_test.go b/internal/postgres/reconcile_test.go index 89d8e24a28..2d8315b626 100644 --- a/internal/postgres/reconcile_test.go +++ b/internal/postgres/reconcile_test.go @@ -256,7 +256,7 @@ initContainers: echo Initializing ... results 'uid' "$(id -u ||:)" 'gid' "$(id -G ||:)" if [[ "${pgwal_directory}" == *"pgwal/"* ]] && [[ ! -d "/pgwal/pgbackrest-spool" ]];then rm -rf "/pgdata/pgbackrest-spool" && mkdir -p "/pgwal/pgbackrest-spool" && ln --force --symbolic "/pgwal/pgbackrest-spool" "/pgdata/pgbackrest-spool";fi - if [[ ! "${pgwal_directory}" == *"pgwal/"* ]];then rm -rf /pgdata/pgbackrest-spool;fi + if [[ ! -e "/pgdata/pgbackrest-spool" ]];then rm -rf /pgdata/pgbackrest-spool;fi results 'postgres path' "$(command -v postgres ||:)" results 'postgres version' "${postgres_version:=$(postgres --version ||:)}" [[ "${postgres_version}" =~ ") ${expected_major_version}"($|[^0-9]) ]] || From 1bd891a051a8e5909352ee40dda309ac4d1cd96d Mon Sep 17 00:00:00 2001 From: Anthony Landreth Date: Tue, 30 Jul 2024 12:06:49 -0400 Subject: [PATCH 4/4] Adds whitespace to test --- internal/controller/postgrescluster/pgbackrest_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/postgrescluster/pgbackrest_test.go b/internal/controller/postgrescluster/pgbackrest_test.go index 40c3ecf034..e50c3a4daf 100644 --- a/internal/controller/postgrescluster/pgbackrest_test.go +++ b/internal/controller/postgrescluster/pgbackrest_test.go @@ -2123,7 +2123,7 @@ func TestReconcileCloudBasedDataSource(t *testing.T) { Reason: "ClusterAlreadyBootstrapped", Message: "The cluster is already bootstrapped", }, - conf: "|\n # Generated by postgres-operator. DO NOT EDIT.\n # Your changes will not be saved.\n\n [global]\n archive-async = y\nlog-path = /pgdata/pgbackrest/log\n repo1-path = /pgbackrest/repo1\n spool-path = /pgdata/pgbackrest-spool\n\n [db]\n pg1-path = /pgdata/pg13\n pg1-port = 5432\n pg1-socket-path = /tmp/postgres\n", + conf: "|\n # Generated by postgres-operator. DO NOT EDIT.\n # Your changes will not be saved.\n\n [global]\n archive-async = y\n log-path = /pgdata/pgbackrest/log\n repo1-path = /pgbackrest/repo1\n spool-path = /pgdata/pgbackrest-spool\n\n [db]\n pg1-path = /pgdata/pg13\n pg1-port = 5432\n pg1-socket-path = /tmp/postgres\n", }, }}