From d0ad501c929d0c331cf5e4183924334451881189 Mon Sep 17 00:00:00 2001 From: Anthony Landreth Date: Mon, 19 Aug 2024 14:59:07 -0400 Subject: [PATCH 01/13] Optional backups --- ...ator.crunchydata.com_postgresclusters.yaml | 3 -- .../controller/postgrescluster/controller.go | 1 + .../controller/postgrescluster/instance.go | 6 ++- .../controller/postgrescluster/pgbackrest.go | 46 ++++++++++++++++++- internal/pgbackrest/postgres.go | 11 ++++- .../v1beta1/postgrescluster_types.go | 11 +++-- .../e2e/optional-backups/00--cluster.yaml | 15 ++++++ .../kuttl/e2e/optional-backups/00-assert.yaml | 9 ++++ .../e2e/optional-backups/01--cluster.yaml | 26 +++++++++++ .../kuttl/e2e/optional-backups/01-assert.yaml | 16 +++++++ 10 files changed, 132 insertions(+), 12 deletions(-) create mode 100644 testing/kuttl/e2e/optional-backups/00--cluster.yaml create mode 100644 testing/kuttl/e2e/optional-backups/00-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/01--cluster.yaml create mode 100644 testing/kuttl/e2e/optional-backups/01-assert.yaml diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index 1a3bb00f9b..0550a17b94 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml @@ -4334,8 +4334,6 @@ spec: required: - volumeSnapshotClassName type: object - required: - - pgbackrest type: object config: properties: @@ -16873,7 +16871,6 @@ spec: - name x-kubernetes-list-type: map required: - - backups - instances - postgresVersion type: object diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index 098b38b30d..696279998f 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/internal/controller/postgrescluster/controller.go @@ -220,6 +220,7 @@ func (r *Reconciler) Reconcile( pgParameters := postgres.NewParameters() pgaudit.PostgreSQLParameters(&pgParameters) + pgbackrest.PostgreSQL(cluster, &pgParameters) pgmonitor.PostgreSQLParameters(cluster, &pgParameters) diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index beaaabcced..1464ee7615 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -1198,8 +1198,10 @@ func (r *Reconciler) reconcileInstance( postgresDataVolume, postgresWALVolume, tablespaceVolumes, &instance.Spec.Template.Spec) - addPGBackRestToInstancePodSpec( - ctx, cluster, instanceCertificates, &instance.Spec.Template.Spec) + if cluster.BackupsEnabled() { + addPGBackRestToInstancePodSpec( + ctx, cluster, instanceCertificates, &instance.Spec.Template.Spec) + } err = patroni.InstancePod( ctx, cluster, clusterConfigMap, clusterPodService, patroniLeaderService, diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index 85465ddbf2..39ebffde33 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -1297,7 +1297,6 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, result.Requeue = true return result, nil } - repoHostName = repoHost.GetName() if err := r.reconcilePGBackRestSecret(ctx, postgresCluster, repoHost, rootCA); err != nil { log.Error(err, "unable to reconcile pgBackRest secret") @@ -1327,8 +1326,15 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, for _, instance := range instances.forCluster { instanceNames = append(instanceNames, instance.Name) } + + if !postgresCluster.BackupsEnabled() { + // Return before reconciliation expects a repo host to exist. + return result, nil + } + // sort to ensure consistent ordering of hosts when creating pgBackRest configs sort.Strings(instanceNames) + repoHostName = repoHost.GetName() if err := r.reconcilePGBackRestConfig(ctx, postgresCluster, repoHostName, configHash, naming.ClusterPodService(postgresCluster).Name, postgresCluster.GetNamespace(), instanceNames); err != nil { @@ -1896,6 +1902,19 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, repoHostName, configHash, serviceName, serviceNamespace string, instanceNames []string) error { + // Remove the existing pgBackRest config map, if backups have been turned off. + if !postgresCluster.BackupsEnabled() { + existing := &corev1.ConfigMap{} + intent := &corev1.ConfigMap{ObjectMeta: naming.PGBackRestConfig(postgresCluster)} + err := errors.WithStack(client.IgnoreNotFound( + r.Client.Get(ctx, client.ObjectKeyFromObject(intent), existing))) + if err != nil { + return err + } + err = errors.WithStack(client.IgnoreNotFound(r.deleteControlled(ctx, postgresCluster, existing))) + return err + } + backrestConfig := pgbackrest.CreatePGBackRestConfigMapIntent(postgresCluster, repoHostName, configHash, serviceName, serviceNamespace, instanceNames) if err := controllerutil.SetControllerReference(postgresCluster, backrestConfig, @@ -1934,6 +1953,13 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, err := errors.WithStack(client.IgnoreNotFound( r.Client.Get(ctx, client.ObjectKeyFromObject(intent), existing))) + // If backups have been turned off, remove the pgBackRest secret. + if !cluster.BackupsEnabled() { + err = errors.WithStack(client.IgnoreNotFound( + r.deleteControlled(ctx, cluster, existing))) + return err + } + if err == nil { err = r.setControllerReference(cluster, intent) } @@ -2031,6 +2057,22 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, log := logging.FromContext(ctx).WithValues("reconcileResource", "repoHost") + if !postgresCluster.BackupsEnabled() { + // When backups are disabled, remove the backrest StatefulSet, if one exists. + name := fmt.Sprintf("%s-%s", postgresCluster.GetName(), "repo-host") + existing := &appsv1.StatefulSet{} + sts := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: postgresCluster.Namespace, Name: name}} + err := errors.WithStack(client.IgnoreNotFound( + r.Client.Get(ctx, client.ObjectKeyFromObject(sts), existing))) + if err != nil { + return nil, err + } + err = errors.WithStack(client.IgnoreNotFound( + r.deleteControlled(ctx, postgresCluster, existing))) + + return nil, err + } + // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { repoHostReady := metav1.Condition{ @@ -2514,7 +2556,7 @@ func (r *Reconciler) reconcileStanzaCreate(ctx context.Context, // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { var replicaCreateRepoStatus *v1beta1.RepoStatus - if len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { + if !postgresCluster.BackupsEnabled() || len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { return } replicaCreateRepoName := postgresCluster.Spec.Backups.PGBackRest.Repos[0].Name diff --git a/internal/pgbackrest/postgres.go b/internal/pgbackrest/postgres.go index 4636ee9db5..2e049cc428 100644 --- a/internal/pgbackrest/postgres.go +++ b/internal/pgbackrest/postgres.go @@ -38,9 +38,16 @@ func PostgreSQL( // - https://pgbackrest.org/user-guide.html#quickstart/configure-archiving // - https://pgbackrest.org/command.html#command-archive-push // - https://www.postgresql.org/docs/current/runtime-config-wal.html - archive := `pgbackrest --stanza=` + DefaultStanzaName + ` archive-push "%p"` outParameters.Mandatory.Add("archive_mode", "on") - outParameters.Mandatory.Add("archive_command", archive) + if inCluster.BackupsEnabled() { + archive := `pgbackrest --stanza=` + DefaultStanzaName + ` archive-push "%p"` + outParameters.Mandatory.Add("archive_command", archive) + } else { + //TODO: outParameters.Mandatory.Add("archive_check", "n") # Not needed? + // If backups are disabled, keep archive_mode on (to avoid a Postgres restart) + // and throw away WAL. + outParameters.Mandatory.Add("archive_command", `true`) + } // archive_timeout is used to determine at what point a WAL file is switched, // if the WAL archive has not reached its full size in # of transactions diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go index 0a066c076f..1e069f6bf3 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go @@ -17,6 +17,7 @@ package v1beta1 import ( "fmt" + "reflect" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -33,8 +34,8 @@ type PostgresClusterSpec struct { DataSource *DataSource `json:"dataSource,omitempty"` // PostgreSQL backup configuration - // +kubebuilder:validation:Required - Backups Backups `json:"backups"` + // +optional + Backups Backups `json:"backups, omitempty"` // The secret containing the Certificates and Keys to encrypt PostgreSQL // traffic will need to contain the server TLS certificate, TLS key and the @@ -322,7 +323,7 @@ func (s *PostgresClusterSpec) Default() { type Backups struct { // pgBackRest archive configuration - // +kubebuilder:validation:Required + // +optional PGBackRest PGBackRestArchive `json:"pgbackrest"` // VolumeSnapshot configuration @@ -668,6 +669,10 @@ func (c *PostgresCluster) Default() { } c.Spec.Default() } +func (c *PostgresCluster) BackupsEnabled() bool { + emptyBackupsSection := reflect.DeepEqual(c.Spec.Backups, Backups{PGBackRest: PGBackRestArchive{}}) + return !emptyBackupsSection +} // +kubebuilder:object:root=true diff --git a/testing/kuttl/e2e/optional-backups/00--cluster.yaml b/testing/kuttl/e2e/optional-backups/00--cluster.yaml new file mode 100644 index 0000000000..7b927831e0 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/00--cluster.yaml @@ -0,0 +1,15 @@ +apiVersion: postgres-operator.crunchydata.com/v1beta1 +kind: PostgresCluster +metadata: + name: created-without-backups +spec: + postgresVersion: ${KUTTL_PG_VERSION} + instances: + - name: instance1 + dataVolumeClaimSpec: + accessModes: + - "ReadWriteOnce" + resources: + requests: + storage: 1Gi + diff --git a/testing/kuttl/e2e/optional-backups/00-assert.yaml b/testing/kuttl/e2e/optional-backups/00-assert.yaml new file mode 100644 index 0000000000..5e90ab6f34 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/00-assert.yaml @@ -0,0 +1,9 @@ +apiVersion: postgres-operator.crunchydata.com/v1beta1 +kind: PostgresCluster +metadata: + name: created-without-backups +status: + instances: + - name: instance1 + pgbackrest: {} + diff --git a/testing/kuttl/e2e/optional-backups/01--cluster.yaml b/testing/kuttl/e2e/optional-backups/01--cluster.yaml new file mode 100644 index 0000000000..b57df2039c --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/01--cluster.yaml @@ -0,0 +1,26 @@ +apiVersion: postgres-operator.crunchydata.com/v1beta1 +kind: PostgresCluster +metadata: + name: created-without-backups +spec: + postgresVersion: ${KUTTL_PG_VERSION} + instances: + - name: instance1 + dataVolumeClaimSpec: + accessModes: + - "ReadWriteOnce" + resources: + requests: + storage: 1Gi + backups: + pgbackrest: + repos: + - name: repo1 + volume: + volumeClaimSpec: + accessModes: + - "ReadWriteOnce" + resources: + requests: + storage: 1Gi + diff --git a/testing/kuttl/e2e/optional-backups/01-assert.yaml b/testing/kuttl/e2e/optional-backups/01-assert.yaml new file mode 100644 index 0000000000..1bf980ba40 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/01-assert.yaml @@ -0,0 +1,16 @@ +# It should be possible to turn backups back on. +apiVersion: postgres-operator.crunchydata.com/v1beta1 +kind: PostgresCluster +metadata: + name: created-without-backups +status: + pgbackrest: + repoHost: + apiVersion: apps/v1 + kind: StatefulSet + ready: true + repos: + - bound: true + name: repo1 + replicaCreateBackupComplete: true + stanzaCreated: true From b4a362549e5467cd7e29970a5be42acd9a9946d9 Mon Sep 17 00:00:00 2001 From: Anthony Landreth Date: Tue, 20 Aug 2024 17:27:45 -0400 Subject: [PATCH 02/13] Checking for StatefulSet in BackupsEnabled --- .../controller/postgrescluster/controller.go | 6 +- .../controller/postgrescluster/instance.go | 6 +- .../controller/postgrescluster/pgbackrest.go | 86 +++++++++++++++++-- internal/pgbackrest/postgres.go | 3 +- .../v1beta1/postgrescluster_types.go | 5 -- 5 files changed, 90 insertions(+), 16 deletions(-) diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index 696279998f..4d473b039c 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/internal/controller/postgrescluster/controller.go @@ -221,7 +221,11 @@ func (r *Reconciler) Reconcile( pgParameters := postgres.NewParameters() pgaudit.PostgreSQLParameters(&pgParameters) - pgbackrest.PostgreSQL(cluster, &pgParameters) + backupsEnabled, err := r.BackupsEnabled(ctx, cluster) + if err != nil { + return reconcile.Result{}, err + } + pgbackrest.PostgreSQL(cluster, &pgParameters, backupsEnabled) pgmonitor.PostgreSQLParameters(cluster, &pgParameters) // Set huge_pages = try if a hugepages resource limit > 0, otherwise set "off" diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 1464ee7615..2c215929a3 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -1198,7 +1198,11 @@ func (r *Reconciler) reconcileInstance( postgresDataVolume, postgresWALVolume, tablespaceVolumes, &instance.Spec.Template.Spec) - if cluster.BackupsEnabled() { + backupsEnabled, err := r.BackupsEnabled(ctx, cluster) + if err != nil { + return err + } + if backupsEnabled { addPGBackRestToInstancePodSpec( ctx, cluster, instanceCertificates, &instance.Spec.Template.Spec) } diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index 39ebffde33..93fb3050ab 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -19,6 +19,7 @@ import ( "context" "fmt" "io" + "reflect" "regexp" "sort" "strings" @@ -1270,9 +1271,15 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, // add some additional context about what component is being reconciled log := logging.FromContext(ctx).WithValues("reconciler", "pgBackRest") - // if nil, create the pgBackRest status that will be updated when reconciling various - // pgBackRest resources - if postgresCluster.Status.PGBackRest == nil { + backupsEnabled, err := r.BackupsEnabled(ctx, postgresCluster) + fmt.Println("#######################", backupsEnabled, err) + if err != nil { + return reconcile.Result{}, err + } + + // if nil and backups are enabled, create the pgBackRest status that will be updated when + // reconciling various pgBackRest resources + if postgresCluster.Status.PGBackRest == nil && backupsEnabled { postgresCluster.Status.PGBackRest = &v1beta1.PGBackRestStatus{} } @@ -1327,7 +1334,8 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, instanceNames = append(instanceNames, instance.Name) } - if !postgresCluster.BackupsEnabled() { + fmt.Println("$$$$$$$$$$$$$$$$$$$$$$$", backupsEnabled) + if !backupsEnabled { // Return before reconciliation expects a repo host to exist. return result, nil } @@ -1902,8 +1910,12 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, repoHostName, configHash, serviceName, serviceNamespace string, instanceNames []string) error { + backupsEnabled, err := r.BackupsEnabled(ctx, postgresCluster) + if err != nil { + return err + } // Remove the existing pgBackRest config map, if backups have been turned off. - if !postgresCluster.BackupsEnabled() { + if !backupsEnabled { existing := &corev1.ConfigMap{} intent := &corev1.ConfigMap{ObjectMeta: naming.PGBackRestConfig(postgresCluster)} err := errors.WithStack(client.IgnoreNotFound( @@ -1953,8 +1965,12 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, err := errors.WithStack(client.IgnoreNotFound( r.Client.Get(ctx, client.ObjectKeyFromObject(intent), existing))) + backupsEnabled, err := r.BackupsEnabled(ctx, cluster) + if err != nil { + return err + } // If backups have been turned off, remove the pgBackRest secret. - if !cluster.BackupsEnabled() { + if !backupsEnabled { err = errors.WithStack(client.IgnoreNotFound( r.deleteControlled(ctx, cluster, existing))) return err @@ -2057,7 +2073,11 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, log := logging.FromContext(ctx).WithValues("reconcileResource", "repoHost") - if !postgresCluster.BackupsEnabled() { + backupsEnabled, err := r.BackupsEnabled(ctx, postgresCluster) + if err != nil { + return nil, err + } + if !backupsEnabled { // When backups are disabled, remove the backrest StatefulSet, if one exists. name := fmt.Sprintf("%s-%s", postgresCluster.GetName(), "repo-host") existing := &appsv1.StatefulSet{} @@ -2067,6 +2087,7 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, if err != nil { return nil, err } + err = errors.WithStack(client.IgnoreNotFound( r.deleteControlled(ctx, postgresCluster, existing))) @@ -2075,6 +2096,10 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { + backupsEnabled, _ = r.BackupsEnabled(ctx, postgresCluster) + if backupsEnabled { + return + } repoHostReady := metav1.Condition{ ObservedGeneration: postgresCluster.GetGeneration(), Type: ConditionRepoHostReady, @@ -2116,6 +2141,7 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, return nil, err } + // TODO: Why do we hit this line when backups should not be enabled? postgresCluster.Status.PGBackRest.RepoHost = getRepoHostStatus(repoHost) if isCreate { @@ -2556,7 +2582,11 @@ func (r *Reconciler) reconcileStanzaCreate(ctx context.Context, // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { var replicaCreateRepoStatus *v1beta1.RepoStatus - if !postgresCluster.BackupsEnabled() || len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { + backupsEnabled, err := r.BackupsEnabled(ctx, postgresCluster) + if err != nil { + return + } + if backupsEnabled || len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { return } replicaCreateRepoName := postgresCluster.Spec.Backups.PGBackRest.Repos[0].Name @@ -2957,3 +2987,43 @@ func (r *Reconciler) reconcilePGBackRestCronJob( } return err } + +func (r *Reconciler) BackupsEnabled(ctx context.Context, postgresCluster *v1beta1.PostgresCluster) (bool, error) { + var err error + backupSpecAbsent := reflect.DeepEqual(postgresCluster.Spec.Backups, v1beta1.Backups{PGBackRest: v1beta1.PGBackRestArchive{}}) + if backupSpecAbsent { + name := fmt.Sprintf("%s-%s", postgresCluster.GetName(), "repo-host") + existing := &appsv1.StatefulSet{} + sts := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: postgresCluster.Namespace, Name: name}} + err = errors.WithStack( + r.Client.Get(ctx, client.ObjectKeyFromObject(sts), existing)) + + fmt.Println(">>>>>>>>>>>>>>>>>>>>>>>>", err) + if apierrors.IsNotFound(err) { + fmt.Println(">>>>>>>>>>>>>>>>>>>>>>>> IN HERE") + return false, nil + } + + if err != nil { + return false, err + } + + if !r.FoundDestroyBackupsAnnotation(postgresCluster) { + return true, err + } + + return false, err + } + + return true, err +} + +func (r *Reconciler) FoundDestroyBackupsAnnotation(postgresCluster *v1beta1.PostgresCluster) bool { + annotations := postgresCluster.GetAnnotations() + for annotation := range annotations { + if annotation == "destroy-backup-volumes" { + return annotations["destroy-backup-volumes"] == "true" + } + } + return false +} diff --git a/internal/pgbackrest/postgres.go b/internal/pgbackrest/postgres.go index 2e049cc428..a1dce03b90 100644 --- a/internal/pgbackrest/postgres.go +++ b/internal/pgbackrest/postgres.go @@ -26,6 +26,7 @@ import ( func PostgreSQL( inCluster *v1beta1.PostgresCluster, outParameters *postgres.Parameters, + backupsEnabled bool, ) { if outParameters.Mandatory == nil { outParameters.Mandatory = postgres.NewParameterSet() @@ -39,7 +40,7 @@ func PostgreSQL( // - https://pgbackrest.org/command.html#command-archive-push // - https://www.postgresql.org/docs/current/runtime-config-wal.html outParameters.Mandatory.Add("archive_mode", "on") - if inCluster.BackupsEnabled() { + if backupsEnabled { archive := `pgbackrest --stanza=` + DefaultStanzaName + ` archive-push "%p"` outParameters.Mandatory.Add("archive_command", archive) } else { diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go index 1e069f6bf3..9477f3add2 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go @@ -17,7 +17,6 @@ package v1beta1 import ( "fmt" - "reflect" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -669,10 +668,6 @@ func (c *PostgresCluster) Default() { } c.Spec.Default() } -func (c *PostgresCluster) BackupsEnabled() bool { - emptyBackupsSection := reflect.DeepEqual(c.Spec.Backups, Backups{PGBackRest: PGBackRestArchive{}}) - return !emptyBackupsSection -} // +kubebuilder:object:root=true From 2c23d85243e0f6d057d479282e34ff484d682e5b Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Wed, 21 Aug 2024 11:00:03 -0500 Subject: [PATCH 03/13] Experimenting with reconciliation pausing, aug 21 --- backups enabled.md | 13 ++ .../controller/postgrescluster/controller.go | 2 +- .../controller/postgrescluster/instance.go | 2 +- .../controller/postgrescluster/pgbackrest.go | 126 +++++++++++++----- 4 files changed, 106 insertions(+), 37 deletions(-) create mode 100644 backups enabled.md diff --git a/backups enabled.md b/backups enabled.md new file mode 100644 index 0000000000..9efd261632 --- /dev/null +++ b/backups enabled.md @@ -0,0 +1,13 @@ +backups enabled +- backup spec present + - should we warn if annotation present? +- backup spec absent, sts present, annotation absent + - if spec absent and we consume spec = bad + +spec | sts | annotation | backupsEnabled | backupReconciliationAllowed +Y | | = Y | Y + +N | N | = N | Y +N | Y | Y = N | Y + +N | Y | N = ~Y | N diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index 4d473b039c..dae9cc552c 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/internal/controller/postgrescluster/controller.go @@ -221,7 +221,7 @@ func (r *Reconciler) Reconcile( pgParameters := postgres.NewParameters() pgaudit.PostgreSQLParameters(&pgParameters) - backupsEnabled, err := r.BackupsEnabled(ctx, cluster) + backupsEnabled, _, err := r.BackupsEnabled(ctx, cluster) if err != nil { return reconcile.Result{}, err } diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 2c215929a3..70209bde53 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -1198,7 +1198,7 @@ func (r *Reconciler) reconcileInstance( postgresDataVolume, postgresWALVolume, tablespaceVolumes, &instance.Spec.Template.Spec) - backupsEnabled, err := r.BackupsEnabled(ctx, cluster) + backupsEnabled, _, err := r.BackupsEnabled(ctx, cluster) if err != nil { return err } diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index 93fb3050ab..c8ab947785 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -19,6 +19,7 @@ import ( "context" "fmt" "io" + llog "log" "reflect" "regexp" "sort" @@ -348,6 +349,7 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, // If nothing has specified that the resource should not be deleted, then delete if delete { + llog.Println("Are we deleting the object here", ownedResources[i].GetName()) if err := r.Client.Delete(ctx, &ownedResources[i], client.PropagationPolicy(metav1.DeletePropagationBackground)); err != nil { return []unstructured.Unstructured{}, errors.WithStack(err) @@ -1271,8 +1273,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, // add some additional context about what component is being reconciled log := logging.FromContext(ctx).WithValues("reconciler", "pgBackRest") - backupsEnabled, err := r.BackupsEnabled(ctx, postgresCluster) - fmt.Println("#######################", backupsEnabled, err) + backupsEnabled, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) if err != nil { return reconcile.Result{}, err } @@ -1334,8 +1335,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, instanceNames = append(instanceNames, instance.Name) } - fmt.Println("$$$$$$$$$$$$$$$$$$$$$$$", backupsEnabled) - if !backupsEnabled { + if !backupsEnabled || backupsTurnedOff { // Return before reconciliation expects a repo host to exist. return result, nil } @@ -1910,16 +1910,17 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, repoHostName, configHash, serviceName, serviceNamespace string, instanceNames []string) error { - backupsEnabled, err := r.BackupsEnabled(ctx, postgresCluster) + backupsEnabled, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) if err != nil { return err } // Remove the existing pgBackRest config map, if backups have been turned off. if !backupsEnabled { - existing := &corev1.ConfigMap{} - intent := &corev1.ConfigMap{ObjectMeta: naming.PGBackRestConfig(postgresCluster)} + existing := &corev1.ConfigMap{ + ObjectMeta: naming.PGBackRestConfig(postgresCluster), + } err := errors.WithStack(client.IgnoreNotFound( - r.Client.Get(ctx, client.ObjectKeyFromObject(intent), existing))) + r.Client.Get(ctx, client.ObjectKeyFromObject(existing), existing))) if err != nil { return err } @@ -1927,6 +1928,12 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, return err } + // If we have no backup spec, yes STS, no annotation, pause reconciliation + // return an error, emit event, log warning? + if !backupReconciliationAllowed { + return nil + } + backrestConfig := pgbackrest.CreatePGBackRestConfigMapIntent(postgresCluster, repoHostName, configHash, serviceName, serviceNamespace, instanceNames) if err := controllerutil.SetControllerReference(postgresCluster, backrestConfig, @@ -1965,7 +1972,10 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, err := errors.WithStack(client.IgnoreNotFound( r.Client.Get(ctx, client.ObjectKeyFromObject(intent), existing))) - backupsEnabled, err := r.BackupsEnabled(ctx, cluster) + if err != nil { + return err + } + backupsEnabled, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, cluster) if err != nil { return err } @@ -1976,6 +1986,11 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, return err } + // exit early, pause reconciliation + if !backupReconciliationAllowed { + return err + } + if err == nil { err = r.setControllerReference(cluster, intent) } @@ -2073,11 +2088,12 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, log := logging.FromContext(ctx).WithValues("reconcileResource", "repoHost") - backupsEnabled, err := r.BackupsEnabled(ctx, postgresCluster) + backupsEnabled, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) if err != nil { return nil, err } if !backupsEnabled { + llog.Println("Backups are disabled in reconcileDedicatedRepoHost") // When backups are disabled, remove the backrest StatefulSet, if one exists. name := fmt.Sprintf("%s-%s", postgresCluster.GetName(), "repo-host") existing := &appsv1.StatefulSet{} @@ -2094,10 +2110,16 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, return nil, err } + // Do we want to get the existing STS and return it? + // What we don't want to do is generate one from the hobbled spec + if !backupReconciliationAllowed { + return nil, err + } + // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { backupsEnabled, _ = r.BackupsEnabled(ctx, postgresCluster) - if backupsEnabled { + if !backupsEnabled { return } repoHostReady := metav1.Condition{ @@ -2586,7 +2608,7 @@ func (r *Reconciler) reconcileStanzaCreate(ctx context.Context, if err != nil { return } - if backupsEnabled || len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { + if !backupsEnabled || len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { return } replicaCreateRepoName := postgresCluster.Spec.Backups.PGBackRest.Repos[0].Name @@ -2988,42 +3010,76 @@ func (r *Reconciler) reconcilePGBackRestCronJob( return err } -func (r *Reconciler) BackupsEnabled(ctx context.Context, postgresCluster *v1beta1.PostgresCluster) (bool, error) { - var err error - backupSpecAbsent := reflect.DeepEqual(postgresCluster.Spec.Backups, v1beta1.Backups{PGBackRest: v1beta1.PGBackRestArchive{}}) - if backupSpecAbsent { - name := fmt.Sprintf("%s-%s", postgresCluster.GetName(), "repo-host") - existing := &appsv1.StatefulSet{} - sts := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: postgresCluster.Namespace, Name: name}} - err = errors.WithStack( - r.Client.Get(ctx, client.ObjectKeyFromObject(sts), existing)) +// returns backups enabled +// +// reconciliation paused +func (r *Reconciler) BackupsEnabled( + ctx context.Context, + postgresCluster *v1beta1.PostgresCluster, +) ( + backupsEnabled bool, + backupReconciliationAllowed bool, + err error, +) { + specFound, stsNotFound, annotationFound, err := r.ObserveBackupUniverse(ctx, postgresCluster) - fmt.Println(">>>>>>>>>>>>>>>>>>>>>>>>", err) - if apierrors.IsNotFound(err) { - fmt.Println(">>>>>>>>>>>>>>>>>>>>>>>> IN HERE") - return false, nil - } + if err != nil { + return + } else if specFound { + backupsEnabled = true + backupReconciliationAllowed = true + } else if annotationFound || stsNotFound { + backupsEnabled = false + backupReconciliationAllowed = true + } else { + // No spec, yes STS, no annotation + backupsEnabled = true + backupReconciliationAllowed = false + } - if err != nil { - return false, err - } + return +} - if !r.FoundDestroyBackupsAnnotation(postgresCluster) { - return true, err - } +// Return backup spec found, +// return sts not found, +// return annotation found, +// return err +func (r *Reconciler) ObserveBackupUniverse(ctx context.Context, postgresCluster *v1beta1.PostgresCluster) (bool, bool, bool, error) { + var err error + backupSpecPresent := !reflect.DeepEqual(postgresCluster.Spec.Backups, v1beta1.Backups{PGBackRest: v1beta1.PGBackRestArchive{}}) - return false, err + name := fmt.Sprintf("%s-%s", postgresCluster.GetName(), "repo-host") + existing := &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: postgresCluster.Namespace, + Name: name, + }, } + err = errors.WithStack( + r.Client.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + + stsNotFound := apierrors.IsNotFound(err) + + // // No spec, sts unsure... what do we do? + // if err != nil && !stsNotFound { + // llog.Println("We shouldn't be here, because why would we error?") + // return true, false, err // FIXME: what's the correct behavior if we get an error + // } + + annotationFound := r.DestroyBackupsAnnotationPresent(postgresCluster) + + return backupSpecPresent, stsNotFound, annotationFound, err - return true, err } -func (r *Reconciler) FoundDestroyBackupsAnnotation(postgresCluster *v1beta1.PostgresCluster) bool { +func (r *Reconciler) DestroyBackupsAnnotationPresent(postgresCluster *v1beta1.PostgresCluster) bool { annotations := postgresCluster.GetAnnotations() for annotation := range annotations { if annotation == "destroy-backup-volumes" { + llog.Println("We shouldn't be here, because the annotation doesn't exist", annotations["destroy-backup-volumes"]) return annotations["destroy-backup-volumes"] == "true" } } + llog.Println("We should be here, because the annotation doesn't exist") return false } From 6d8e0a2e03881f1e2a176e15a6066d53535aa0ac Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Wed, 21 Aug 2024 11:00:29 -0500 Subject: [PATCH 04/13] add missed save file --- internal/controller/postgrescluster/pgbackrest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index c8ab947785..355663ec79 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -2111,7 +2111,7 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, } // Do we want to get the existing STS and return it? - // What we don't want to do is generate one from the hobbled spec + // What we don't want to do is generate a new STS from the hobbled spec if !backupReconciliationAllowed { return nil, err } From 5a21e789681a4018f62d778460902b3095944186 Mon Sep 17 00:00:00 2001 From: Anthony Landreth Date: Wed, 21 Aug 2024 15:32:12 -0400 Subject: [PATCH 05/13] Cleans up after dropping backupsSpec --- config/rbac/cluster/role.yaml | 12 +- config/rbac/namespace/role.yaml | 12 +- .../controller/postgrescluster/controller.go | 6 +- .../controller/postgrescluster/instance.go | 8 +- .../controller/postgrescluster/pgbackrest.go | 167 +++++++++++++----- 5 files changed, 131 insertions(+), 74 deletions(-) diff --git a/config/rbac/cluster/role.yaml b/config/rbac/cluster/role.yaml index 64c58a134c..1119eb0d5a 100644 --- a/config/rbac/cluster/role.yaml +++ b/config/rbac/cluster/role.yaml @@ -10,6 +10,7 @@ rules: - configmaps - persistentvolumeclaims - secrets + - serviceaccounts - services verbs: - create @@ -54,16 +55,6 @@ rules: - list - patch - watch -- apiGroups: - - '' - resources: - - serviceaccounts - verbs: - - create - - get - - list - - patch - - watch - apiGroups: - apps resources: @@ -167,6 +158,7 @@ rules: - roles verbs: - create + - delete - get - list - patch diff --git a/config/rbac/namespace/role.yaml b/config/rbac/namespace/role.yaml index 2193a7b674..d4ede32c6c 100644 --- a/config/rbac/namespace/role.yaml +++ b/config/rbac/namespace/role.yaml @@ -10,6 +10,7 @@ rules: - configmaps - persistentvolumeclaims - secrets + - serviceaccounts - services verbs: - create @@ -54,16 +55,6 @@ rules: - list - patch - watch -- apiGroups: - - '' - resources: - - serviceaccounts - verbs: - - create - - get - - list - - patch - - watch - apiGroups: - apps resources: @@ -167,6 +158,7 @@ rules: - roles verbs: - create + - delete - get - list - patch diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index dae9cc552c..5621a4992f 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/internal/controller/postgrescluster/controller.go @@ -221,11 +221,11 @@ func (r *Reconciler) Reconcile( pgParameters := postgres.NewParameters() pgaudit.PostgreSQLParameters(&pgParameters) - backupsEnabled, _, err := r.BackupsEnabled(ctx, cluster) - if err != nil { + backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, cluster) + if err != nil || !backupsReconciliationAllowed { return reconcile.Result{}, err } - pgbackrest.PostgreSQL(cluster, &pgParameters, backupsEnabled) + pgbackrest.PostgreSQL(cluster, &pgParameters, backupsSpecFound) pgmonitor.PostgreSQLParameters(cluster, &pgParameters) // Set huge_pages = try if a hugepages resource limit > 0, otherwise set "off" diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 70209bde53..1b4968746d 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -1198,16 +1198,16 @@ func (r *Reconciler) reconcileInstance( postgresDataVolume, postgresWALVolume, tablespaceVolumes, &instance.Spec.Template.Spec) - backupsEnabled, _, err := r.BackupsEnabled(ctx, cluster) - if err != nil { + backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, cluster) + if err != nil || !backupsReconciliationAllowed { return err } - if backupsEnabled { + if backupsSpecFound { addPGBackRestToInstancePodSpec( ctx, cluster, instanceCertificates, &instance.Spec.Template.Spec) } - err = patroni.InstancePod( + _ = patroni.InstancePod( ctx, cluster, clusterConfigMap, clusterPodService, patroniLeaderService, spec, instanceCertificates, instanceConfigMap, &instance.Spec.Template) } diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index 355663ec79..adf65568c6 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -1273,14 +1273,18 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, // add some additional context about what component is being reconciled log := logging.FromContext(ctx).WithValues("reconciler", "pgBackRest") - backupsEnabled, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) + backupsSpecFound, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) if err != nil { return reconcile.Result{}, err } + if !backupReconciliationAllowed { + return reconcile.Result{}, err + } + // if nil and backups are enabled, create the pgBackRest status that will be updated when // reconciling various pgBackRest resources - if postgresCluster.Status.PGBackRest == nil && backupsEnabled { + if postgresCluster.Status.PGBackRest == nil { postgresCluster.Status.PGBackRest = &v1beta1.PGBackRestStatus{} } @@ -1335,14 +1339,15 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, instanceNames = append(instanceNames, instance.Name) } - if !backupsEnabled || backupsTurnedOff { - // Return before reconciliation expects a repo host to exist. - return result, nil - } - // sort to ensure consistent ordering of hosts when creating pgBackRest configs sort.Strings(instanceNames) - repoHostName = repoHost.GetName() + + if !backupsSpecFound { + repoHostName = "none" + } else { + repoHostName = repoHost.GetName() + } + if err := r.reconcilePGBackRestConfig(ctx, postgresCluster, repoHostName, configHash, naming.ClusterPodService(postgresCluster).Name, postgresCluster.GetNamespace(), instanceNames); err != nil { @@ -1358,6 +1363,11 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, return result, nil } + if !backupsSpecFound { + // Return before reconciliation expects a repo host to exist. + return result, nil + } + // reconcile the pgBackRest stanza for all configuration pgBackRest repos configHashMismatch, err := r.reconcileStanzaCreate(ctx, postgresCluster, instances, configHash) // If a stanza create error then requeue but don't return the error. This prevents @@ -1910,12 +1920,18 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, repoHostName, configHash, serviceName, serviceNamespace string, instanceNames []string) error { - backupsEnabled, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) + backupsSpecFound, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) if err != nil { return err } + + // If we have no backup spec, yes STS, no annotation, pause reconciliation + // return an error, emit event, log warning? + if !backupReconciliationAllowed { + return nil + } // Remove the existing pgBackRest config map, if backups have been turned off. - if !backupsEnabled { + if !backupsSpecFound { existing := &corev1.ConfigMap{ ObjectMeta: naming.PGBackRestConfig(postgresCluster), } @@ -1928,12 +1944,6 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, return err } - // If we have no backup spec, yes STS, no annotation, pause reconciliation - // return an error, emit event, log warning? - if !backupReconciliationAllowed { - return nil - } - backrestConfig := pgbackrest.CreatePGBackRestConfigMapIntent(postgresCluster, repoHostName, configHash, serviceName, serviceNamespace, instanceNames) if err := controllerutil.SetControllerReference(postgresCluster, backrestConfig, @@ -1975,12 +1985,15 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, if err != nil { return err } - backupsEnabled, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, cluster) + backupSpecFound, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, cluster) if err != nil { return err } + if !backupReconciliationAllowed { + return err + } // If backups have been turned off, remove the pgBackRest secret. - if !backupsEnabled { + if !backupSpecFound { err = errors.WithStack(client.IgnoreNotFound( r.deleteControlled(ctx, cluster, existing))) return err @@ -2011,9 +2024,9 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, return err } -// +kubebuilder:rbac:groups="",resources="serviceaccounts",verbs={create,patch} -// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="roles",verbs={create,patch} -// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="rolebindings",verbs={create,patch} +// +kubebuilder:rbac:groups="",resources="serviceaccounts",verbs={create,patch, delete} +// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="roles",verbs={create,patch, delete} +// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="rolebindings",verbs={create,patch, delete} // reconcileInstanceRBAC reconciles the Role, RoleBinding, and ServiceAccount for // pgBackRest @@ -2021,12 +2034,63 @@ func (r *Reconciler) reconcilePGBackRestRBAC(ctx context.Context, postgresCluster *v1beta1.PostgresCluster) (*corev1.ServiceAccount, error) { sa := &corev1.ServiceAccount{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} + role := &rbacv1.Role{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} + binding := &rbacv1.RoleBinding{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} + + backupsSpecFound, backupsReconciliationAllowed, _ := r.BackupsEnabled(ctx, postgresCluster) + + // We should never have to run logic for !backupsReconciliationAllowed. + if !backupsReconciliationAllowed { + err := errors.WithStack(client.IgnoreNotFound( + r.Client.Get(ctx, client.ObjectKeyFromObject(sa), sa))) + if err != nil { + return nil, err + } + + return sa, err + } + + if !backupsSpecFound { + err := errors.WithStack(client.IgnoreNotFound( + r.Client.Get(ctx, client.ObjectKeyFromObject(sa), sa))) + // Delete the Secret when it exists and there is nothing we want to keep in it. + if err == nil && len(sa.UID) != 0 { + err = errors.WithStack(client.IgnoreNotFound( + r.deleteControlled(ctx, postgresCluster, sa))) + } + if err != nil { + return nil, err + } + + err = errors.WithStack(client.IgnoreNotFound( + r.Client.Get(ctx, client.ObjectKeyFromObject(role), role))) + // Delete the Secret when it exists and there is nothing we want to keep in it. + if err == nil && len(sa.UID) != 0 { + err = errors.WithStack(client.IgnoreNotFound( + r.deleteControlled(ctx, postgresCluster, role))) + } + if err != nil { + return nil, err + } + + err = errors.WithStack(client.IgnoreNotFound( + r.Client.Get(ctx, client.ObjectKeyFromObject(binding), binding))) + // Delete the Secret when it exists and there is nothing we want to keep in it. + if err == nil && len(sa.UID) != 0 { + err = errors.WithStack(client.IgnoreNotFound( + r.deleteControlled(ctx, postgresCluster, binding))) + } + if err != nil { + return nil, err + } + + return nil, nil + } + sa.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ServiceAccount")) - role := &rbacv1.Role{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} role.SetGroupVersionKind(rbacv1.SchemeGroupVersion.WithKind("Role")) - binding := &rbacv1.RoleBinding{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} binding.SetGroupVersionKind(rbacv1.SchemeGroupVersion.WithKind("RoleBinding")) if err := r.setControllerReference(postgresCluster, sa); err != nil { @@ -2088,11 +2152,15 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, log := logging.FromContext(ctx).WithValues("reconcileResource", "repoHost") - backupsEnabled, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) + backupsSpecFound, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) if err != nil { return nil, err } - if !backupsEnabled { + if !backupReconciliationAllowed { + return nil, err + } + + if !backupsSpecFound { llog.Println("Backups are disabled in reconcileDedicatedRepoHost") // When backups are disabled, remove the backrest StatefulSet, if one exists. name := fmt.Sprintf("%s-%s", postgresCluster.GetName(), "repo-host") @@ -2118,8 +2186,8 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { - backupsEnabled, _ = r.BackupsEnabled(ctx, postgresCluster) - if !backupsEnabled { + backupsSpecFound, backupReconciliationAllowed, _ := r.BackupsEnabled(ctx, postgresCluster) + if !backupReconciliationAllowed || !backupsSpecFound { return } repoHostReady := metav1.Condition{ @@ -2604,11 +2672,11 @@ func (r *Reconciler) reconcileStanzaCreate(ctx context.Context, // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { var replicaCreateRepoStatus *v1beta1.RepoStatus - backupsEnabled, err := r.BackupsEnabled(ctx, postgresCluster) - if err != nil { + backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) + if err != nil || !backupsReconciliationAllowed { return } - if !backupsEnabled || len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { + if !backupsSpecFound || len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { return } replicaCreateRepoName := postgresCluster.Spec.Backups.PGBackRest.Repos[0].Name @@ -3017,27 +3085,31 @@ func (r *Reconciler) BackupsEnabled( ctx context.Context, postgresCluster *v1beta1.PostgresCluster, ) ( - backupsEnabled bool, - backupReconciliationAllowed bool, + backupsSpecFound bool, + backupsReconciliationAllowed bool, err error, ) { specFound, stsNotFound, annotationFound, err := r.ObserveBackupUniverse(ctx, postgresCluster) - if err != nil { - return - } else if specFound { - backupsEnabled = true - backupReconciliationAllowed = true - } else if annotationFound || stsNotFound { - backupsEnabled = false - backupReconciliationAllowed = true - } else { - // No spec, yes STS, no annotation - backupsEnabled = true - backupReconciliationAllowed = false + switch { + case err != nil: + case specFound: + backupsSpecFound = true + backupsReconciliationAllowed = true + case annotationFound || stsNotFound: + backupsReconciliationAllowed = true + case !annotationFound && !stsNotFound: + // Destroying backups is a two key operation: + // 1. You must remove the backups section of the spec. + // 2. You must apply an annotation to the cluster. + // The existence of a StatefulSet without the backups spec is + // evidence of key 1 being turned without key 2 being turned + // -- block reconciliation until the annotation is added. + backupsReconciliationAllowed = false + default: + backupsReconciliationAllowed = false } - - return + return backupsSpecFound, backupsReconciliationAllowed, err } // Return backup spec found, @@ -3068,7 +3140,8 @@ func (r *Reconciler) ObserveBackupUniverse(ctx context.Context, postgresCluster annotationFound := r.DestroyBackupsAnnotationPresent(postgresCluster) - return backupSpecPresent, stsNotFound, annotationFound, err + // TODO: do something sensible with the err you're returning + return backupSpecPresent, stsNotFound, annotationFound, nil } From 59727aea2cfd0ae52b475c9095f9fe1311d96005 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Wed, 21 Aug 2024 21:23:42 -0500 Subject: [PATCH 06/13] Update KUTTL test --- .../controller/postgrescluster/pgbackrest.go | 13 +-- internal/naming/annotations.go | 6 ++ .../kuttl/e2e/optional-backups/00-assert.yaml | 31 +++++++- .../kuttl/e2e/optional-backups/01-assert.yaml | 16 ---- .../kuttl/e2e/optional-backups/01-errors.yaml | 29 +++++++ .../kuttl/e2e/optional-backups/02-assert.yaml | 15 ++++ .../kuttl/e2e/optional-backups/03-assert.yaml | 14 ++++ .../e2e/optional-backups/04--cluster.yaml | 16 ++++ .../kuttl/e2e/optional-backups/05-assert.yaml | 12 +++ .../kuttl/e2e/optional-backups/06-assert.yaml | 18 +++++ .../{01--cluster.yaml => 10--cluster.yaml} | 1 + .../kuttl/e2e/optional-backups/10-assert.yaml | 79 +++++++++++++++++++ .../kuttl/e2e/optional-backups/11-assert.yaml | 18 +++++ .../e2e/optional-backups/20--cluster.yaml | 6 ++ .../kuttl/e2e/optional-backups/20-assert.yaml | 63 +++++++++++++++ .../kuttl/e2e/optional-backups/21-assert.yaml | 18 +++++ .../e2e/optional-backups/22--cluster.yaml | 5 ++ .../kuttl/e2e/optional-backups/23-assert.yaml | 27 +++++++ .../kuttl/e2e/optional-backups/24-errors.yaml | 29 +++++++ .../kuttl/e2e/optional-backups/25-assert.yaml | 15 ++++ testing/kuttl/e2e/optional-backups/README.md | 13 +++ 21 files changed, 418 insertions(+), 26 deletions(-) delete mode 100644 testing/kuttl/e2e/optional-backups/01-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/01-errors.yaml create mode 100644 testing/kuttl/e2e/optional-backups/02-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/03-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/04--cluster.yaml create mode 100644 testing/kuttl/e2e/optional-backups/05-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/06-assert.yaml rename testing/kuttl/e2e/optional-backups/{01--cluster.yaml => 10--cluster.yaml} (96%) create mode 100644 testing/kuttl/e2e/optional-backups/10-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/11-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/20--cluster.yaml create mode 100644 testing/kuttl/e2e/optional-backups/20-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/21-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/22--cluster.yaml create mode 100644 testing/kuttl/e2e/optional-backups/23-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/24-errors.yaml create mode 100644 testing/kuttl/e2e/optional-backups/25-assert.yaml create mode 100644 testing/kuttl/e2e/optional-backups/README.md diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index adf65568c6..6145ece3d1 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -19,7 +19,6 @@ import ( "context" "fmt" "io" - llog "log" "reflect" "regexp" "sort" @@ -349,7 +348,6 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, // If nothing has specified that the resource should not be deleted, then delete if delete { - llog.Println("Are we deleting the object here", ownedResources[i].GetName()) if err := r.Client.Delete(ctx, &ownedResources[i], client.PropagationPolicy(metav1.DeletePropagationBackground)); err != nil { return []unstructured.Unstructured{}, errors.WithStack(err) @@ -1364,7 +1362,8 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, } if !backupsSpecFound { - // Return before reconciliation expects a repo host to exist. + // Clear the status and exit + postgresCluster.Status.PGBackRest = &v1beta1.PGBackRestStatus{} return result, nil } @@ -2161,7 +2160,6 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, } if !backupsSpecFound { - llog.Println("Backups are disabled in reconcileDedicatedRepoHost") // When backups are disabled, remove the backrest StatefulSet, if one exists. name := fmt.Sprintf("%s-%s", postgresCluster.GetName(), "repo-host") existing := &appsv1.StatefulSet{} @@ -3134,7 +3132,6 @@ func (r *Reconciler) ObserveBackupUniverse(ctx context.Context, postgresCluster // // No spec, sts unsure... what do we do? // if err != nil && !stsNotFound { - // llog.Println("We shouldn't be here, because why would we error?") // return true, false, err // FIXME: what's the correct behavior if we get an error // } @@ -3148,11 +3145,9 @@ func (r *Reconciler) ObserveBackupUniverse(ctx context.Context, postgresCluster func (r *Reconciler) DestroyBackupsAnnotationPresent(postgresCluster *v1beta1.PostgresCluster) bool { annotations := postgresCluster.GetAnnotations() for annotation := range annotations { - if annotation == "destroy-backup-volumes" { - llog.Println("We shouldn't be here, because the annotation doesn't exist", annotations["destroy-backup-volumes"]) - return annotations["destroy-backup-volumes"] == "true" + if annotation == naming.AuthorizeBackupRemovalAnnotation { + return annotations[naming.AuthorizeBackupRemovalAnnotation] == "true" } } - llog.Println("We should be here, because the annotation doesn't exist") return false } diff --git a/internal/naming/annotations.go b/internal/naming/annotations.go index 5f86d45aa7..21e8bd084b 100644 --- a/internal/naming/annotations.go +++ b/internal/naming/annotations.go @@ -72,4 +72,10 @@ const ( // has schemas automatically created for the users defined in `spec.users` for all of the databases // listed for that user. AutoCreateUserSchemaAnnotation = annotationPrefix + "autoCreateUserSchema" + + // AuthorizeBackupRemovalAnnotation is an annotation used to allow users + // to delete PVC-based backups when changing from a cluster with backups + // to a cluster without backups. As usual with the operator, we do not + // touch cloud-based backups. + AuthorizeBackupRemovalAnnotation = annotationPrefix + "authorizeBackupRemoval" ) diff --git a/testing/kuttl/e2e/optional-backups/00-assert.yaml b/testing/kuttl/e2e/optional-backups/00-assert.yaml index 5e90ab6f34..86392d0308 100644 --- a/testing/kuttl/e2e/optional-backups/00-assert.yaml +++ b/testing/kuttl/e2e/optional-backups/00-assert.yaml @@ -6,4 +6,33 @@ status: instances: - name: instance1 pgbackrest: {} - +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 + postgres-operator.crunchydata.com/role: pgdata +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 +--- +apiVersion: v1 +kind: Pod +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 + postgres-operator.crunchydata.com/role: master +status: + containerStatuses: + - ready: true + - ready: true diff --git a/testing/kuttl/e2e/optional-backups/01-assert.yaml b/testing/kuttl/e2e/optional-backups/01-assert.yaml deleted file mode 100644 index 1bf980ba40..0000000000 --- a/testing/kuttl/e2e/optional-backups/01-assert.yaml +++ /dev/null @@ -1,16 +0,0 @@ -# It should be possible to turn backups back on. -apiVersion: postgres-operator.crunchydata.com/v1beta1 -kind: PostgresCluster -metadata: - name: created-without-backups -status: - pgbackrest: - repoHost: - apiVersion: apps/v1 - kind: StatefulSet - ready: true - repos: - - bound: true - name: repo1 - replicaCreateBackupComplete: true - stanzaCreated: true diff --git a/testing/kuttl/e2e/optional-backups/01-errors.yaml b/testing/kuttl/e2e/optional-backups/01-errors.yaml new file mode 100644 index 0000000000..e702fcddb4 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/01-errors.yaml @@ -0,0 +1,29 @@ +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: created-without-backups-repo1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: created-without-backups-repo-host +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: created-without-backups-pgbackrest-config +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: created-without-backups-pgbackrest +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: created-without-backups-pgbackrest +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: created-without-backups-pgbackrest diff --git a/testing/kuttl/e2e/optional-backups/02-assert.yaml b/testing/kuttl/e2e/optional-backups/02-assert.yaml new file mode 100644 index 0000000000..eb3f70357f --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/02-assert.yaml @@ -0,0 +1,15 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +commands: +- script: | + pod=$(kubectl get pods -o name -n "${NAMESPACE}" \ + -l postgres-operator.crunchydata.com/cluster=created-without-backups) + + kubectl exec --stdin "${pod}" --namespace "${NAMESPACE}" -c database \ + -- psql -qb --set ON_ERROR_STOP=1 --file=- <<'SQL' + DO $$ + BEGIN + ASSERT current_setting('archive_command') LIKE 'true', + format('expected "true", got %L', current_setting('archive_command')); + END $$ + SQL diff --git a/testing/kuttl/e2e/optional-backups/03-assert.yaml b/testing/kuttl/e2e/optional-backups/03-assert.yaml new file mode 100644 index 0000000000..17ca1e4062 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/03-assert.yaml @@ -0,0 +1,14 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +commands: +- script: | + pod=$(kubectl get pods -o name -n "${NAMESPACE}" \ + -l postgres-operator.crunchydata.com/cluster=created-without-backups) + + kubectl exec --stdin "${pod}" --namespace "${NAMESPACE}" -c database \ + -- psql -qb --set ON_ERROR_STOP=1 \ + -c "CREATE TABLE important (data) AS VALUES ('treasure');" + + kubectl exec --stdin "${pod}" --namespace "${NAMESPACE}" -c database \ + -- psql -qb --set ON_ERROR_STOP=1 \ + -c "CHECKPOINT;" diff --git a/testing/kuttl/e2e/optional-backups/04--cluster.yaml b/testing/kuttl/e2e/optional-backups/04--cluster.yaml new file mode 100644 index 0000000000..fc39ff6ebe --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/04--cluster.yaml @@ -0,0 +1,16 @@ +apiVersion: postgres-operator.crunchydata.com/v1beta1 +kind: PostgresCluster +metadata: + name: created-without-backups +spec: + postgresVersion: ${KUTTL_PG_VERSION} + instances: + - name: instance1 + replicas: 2 + dataVolumeClaimSpec: + accessModes: + - "ReadWriteOnce" + resources: + requests: + storage: 1Gi + diff --git a/testing/kuttl/e2e/optional-backups/05-assert.yaml b/testing/kuttl/e2e/optional-backups/05-assert.yaml new file mode 100644 index 0000000000..08cd1f30fe --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/05-assert.yaml @@ -0,0 +1,12 @@ +apiVersion: v1 +kind: Pod +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 + postgres-operator.crunchydata.com/role: replica +status: + containerStatuses: + - ready: true + - ready: true \ No newline at end of file diff --git a/testing/kuttl/e2e/optional-backups/06-assert.yaml b/testing/kuttl/e2e/optional-backups/06-assert.yaml new file mode 100644 index 0000000000..c366545508 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/06-assert.yaml @@ -0,0 +1,18 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +commands: +- script: | + pod=$(kubectl get pods -o name -n "${NAMESPACE}" \ + -l postgres-operator.crunchydata.com/cluster=created-without-backups \ + -l postgres-operator.crunchydata.com/role=replica) + + kubectl exec --stdin "${pod}" --namespace "${NAMESPACE}" -c database \ + -- psql -qb --set ON_ERROR_STOP=1 --file=- <<'SQL' + DO $$ + DECLARE + everything jsonb; + BEGIN + SELECT jsonb_agg(important) INTO everything FROM important; + ASSERT everything = '[{"data":"treasure"}]', format('got %L', everything); + END $$ + SQL diff --git a/testing/kuttl/e2e/optional-backups/01--cluster.yaml b/testing/kuttl/e2e/optional-backups/10--cluster.yaml similarity index 96% rename from testing/kuttl/e2e/optional-backups/01--cluster.yaml rename to testing/kuttl/e2e/optional-backups/10--cluster.yaml index b57df2039c..6da85c93f9 100644 --- a/testing/kuttl/e2e/optional-backups/01--cluster.yaml +++ b/testing/kuttl/e2e/optional-backups/10--cluster.yaml @@ -6,6 +6,7 @@ spec: postgresVersion: ${KUTTL_PG_VERSION} instances: - name: instance1 + replicas: 1 dataVolumeClaimSpec: accessModes: - "ReadWriteOnce" diff --git a/testing/kuttl/e2e/optional-backups/10-assert.yaml b/testing/kuttl/e2e/optional-backups/10-assert.yaml new file mode 100644 index 0000000000..b145c1d558 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/10-assert.yaml @@ -0,0 +1,79 @@ +# It should be possible to turn backups back on. +apiVersion: postgres-operator.crunchydata.com/v1beta1 +kind: PostgresCluster +metadata: + name: created-without-backups +status: + pgbackrest: + repoHost: + apiVersion: apps/v1 + kind: StatefulSet + ready: true + repos: + - bound: true + name: repo1 + replicaCreateBackupComplete: true + stanzaCreated: true +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 + postgres-operator.crunchydata.com/role: pgdata +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: created-without-backups-repo1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: created-without-backups-repo-host +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: created-without-backups-pgbackrest-config +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: created-without-backups-pgbackrest +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: created-without-backups-pgbackrest +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: created-without-backups-pgbackrest +--- +apiVersion: v1 +kind: Pod +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 + postgres-operator.crunchydata.com/patroni: created-without-backups-ha + postgres-operator.crunchydata.com/role: master +status: + containerStatuses: + - ready: true + - ready: true + - ready: true + - ready: true \ No newline at end of file diff --git a/testing/kuttl/e2e/optional-backups/11-assert.yaml b/testing/kuttl/e2e/optional-backups/11-assert.yaml new file mode 100644 index 0000000000..5976d03f41 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/11-assert.yaml @@ -0,0 +1,18 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +commands: +- script: | + pod=$(kubectl get pods -o name -n "${NAMESPACE}" \ + -l postgres-operator.crunchydata.com/cluster=created-without-backup \ + -l postgres-operator.crunchydata.com/instance-set=instance1 \ + -l postgres-operator.crunchydata.com/patroni=created-without-backups-ha \ + -l postgres-operator.crunchydata.com/role=master) + + kubectl exec --stdin "${pod}" --namespace "${NAMESPACE}" -c database \ + -- psql -qb --set ON_ERROR_STOP=1 --file=- <<'SQL' + DO $$ + BEGIN + ASSERT current_setting('archive_command') LIKE 'pgbackrest --stanza=db archive-push "%p"', + format('expected "pgbackrest --stanza=db archive-push \"%p\"", got %L', current_setting('archive_command')); + END $$ + SQL diff --git a/testing/kuttl/e2e/optional-backups/20--cluster.yaml b/testing/kuttl/e2e/optional-backups/20--cluster.yaml new file mode 100644 index 0000000000..8e0d01cbf8 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/20--cluster.yaml @@ -0,0 +1,6 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: +- command: |- + kubectl patch postgrescluster created-without-backups --type 'merge' -p '{"spec":{"backups": null}}' + namespaced: true diff --git a/testing/kuttl/e2e/optional-backups/20-assert.yaml b/testing/kuttl/e2e/optional-backups/20-assert.yaml new file mode 100644 index 0000000000..b469e277f8 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/20-assert.yaml @@ -0,0 +1,63 @@ +# It should be possible to turn backups back on. +apiVersion: postgres-operator.crunchydata.com/v1beta1 +kind: PostgresCluster +metadata: + name: created-without-backups +status: + pgbackrest: + repoHost: + apiVersion: apps/v1 + kind: StatefulSet + ready: true + repos: + - bound: true + name: repo1 + replicaCreateBackupComplete: true + stanzaCreated: true +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 + postgres-operator.crunchydata.com/role: pgdata +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: created-without-backups-repo1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: created-without-backups-repo-host +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: created-without-backups-pgbackrest-config +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: created-without-backups-pgbackrest +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: created-without-backups-pgbackrest +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: created-without-backups-pgbackrest diff --git a/testing/kuttl/e2e/optional-backups/21-assert.yaml b/testing/kuttl/e2e/optional-backups/21-assert.yaml new file mode 100644 index 0000000000..5976d03f41 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/21-assert.yaml @@ -0,0 +1,18 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +commands: +- script: | + pod=$(kubectl get pods -o name -n "${NAMESPACE}" \ + -l postgres-operator.crunchydata.com/cluster=created-without-backup \ + -l postgres-operator.crunchydata.com/instance-set=instance1 \ + -l postgres-operator.crunchydata.com/patroni=created-without-backups-ha \ + -l postgres-operator.crunchydata.com/role=master) + + kubectl exec --stdin "${pod}" --namespace "${NAMESPACE}" -c database \ + -- psql -qb --set ON_ERROR_STOP=1 --file=- <<'SQL' + DO $$ + BEGIN + ASSERT current_setting('archive_command') LIKE 'pgbackrest --stanza=db archive-push "%p"', + format('expected "pgbackrest --stanza=db archive-push \"%p\"", got %L', current_setting('archive_command')); + END $$ + SQL diff --git a/testing/kuttl/e2e/optional-backups/22--cluster.yaml b/testing/kuttl/e2e/optional-backups/22--cluster.yaml new file mode 100644 index 0000000000..2e25309886 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/22--cluster.yaml @@ -0,0 +1,5 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: +- command: kubectl annotate postgrescluster created-without-backups postgres-operator.crunchydata.com/authorizeBackupRemoval="true" + namespaced: true diff --git a/testing/kuttl/e2e/optional-backups/23-assert.yaml b/testing/kuttl/e2e/optional-backups/23-assert.yaml new file mode 100644 index 0000000000..27a3ef7fca --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/23-assert.yaml @@ -0,0 +1,27 @@ +# It should be possible to turn backups back on. +apiVersion: postgres-operator.crunchydata.com/v1beta1 +kind: PostgresCluster +metadata: + name: created-without-backups +status: + instances: + - name: instance1 + pgbackrest: {} +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 + postgres-operator.crunchydata.com/role: pgdata +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + labels: + postgres-operator.crunchydata.com/cluster: created-without-backups + postgres-operator.crunchydata.com/data: postgres + postgres-operator.crunchydata.com/instance-set: instance1 +--- \ No newline at end of file diff --git a/testing/kuttl/e2e/optional-backups/24-errors.yaml b/testing/kuttl/e2e/optional-backups/24-errors.yaml new file mode 100644 index 0000000000..e702fcddb4 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/24-errors.yaml @@ -0,0 +1,29 @@ +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: created-without-backups-repo1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: created-without-backups-repo-host +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: created-without-backups-pgbackrest-config +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: created-without-backups-pgbackrest +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: created-without-backups-pgbackrest +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: created-without-backups-pgbackrest diff --git a/testing/kuttl/e2e/optional-backups/25-assert.yaml b/testing/kuttl/e2e/optional-backups/25-assert.yaml new file mode 100644 index 0000000000..eb3f70357f --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/25-assert.yaml @@ -0,0 +1,15 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +commands: +- script: | + pod=$(kubectl get pods -o name -n "${NAMESPACE}" \ + -l postgres-operator.crunchydata.com/cluster=created-without-backups) + + kubectl exec --stdin "${pod}" --namespace "${NAMESPACE}" -c database \ + -- psql -qb --set ON_ERROR_STOP=1 --file=- <<'SQL' + DO $$ + BEGIN + ASSERT current_setting('archive_command') LIKE 'true', + format('expected "true", got %L', current_setting('archive_command')); + END $$ + SQL diff --git a/testing/kuttl/e2e/optional-backups/README.md b/testing/kuttl/e2e/optional-backups/README.md new file mode 100644 index 0000000000..92c52d4136 --- /dev/null +++ b/testing/kuttl/e2e/optional-backups/README.md @@ -0,0 +1,13 @@ +## Optional backups + +### Steps + +00-02. Create cluster without backups, check that expected K8s objects do/don't exist, e.g., repo-host sts doesn't exist; check that the archive command is `true` + +03-06. Add data and a replica; check that the data successfully replicates to the replica. + +10-11. Update cluster to add backups, check that expected K8s objects do/don't exist, e.g., repo-host sts exists; check that the archive command is set to the usual + +20-21. Update cluster to remove backups but without annotation, check that no changes were made, including to the archive command + +22-25. Annotate cluster to remove existing backups, check that expected K8s objects do/don't exist, e.g., repo-host sts doesn't exist; check that the archive command is `true` From 7e37ca4fae96bb08a4ad18d9dab35eebcbd2ed68 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 22 Aug 2024 08:41:14 -0500 Subject: [PATCH 07/13] Alternative pgbackrest optional & cleanup --- backups enabled.md | 13 - .../controller/postgrescluster/cluster.go | 9 +- .../controller/postgrescluster/controller.go | 15 +- .../controller/postgrescluster/instance.go | 14 +- .../controller/postgrescluster/pgbackrest.go | 421 +++++++++--------- .../postgrescluster/pgbackrest_test.go | 15 +- internal/pgbackrest/postgres_test.go | 11 +- 7 files changed, 263 insertions(+), 235 deletions(-) delete mode 100644 backups enabled.md diff --git a/backups enabled.md b/backups enabled.md deleted file mode 100644 index 9efd261632..0000000000 --- a/backups enabled.md +++ /dev/null @@ -1,13 +0,0 @@ -backups enabled -- backup spec present - - should we warn if annotation present? -- backup spec absent, sts present, annotation absent - - if spec absent and we consume spec = bad - -spec | sts | annotation | backupsEnabled | backupReconciliationAllowed -Y | | = Y | Y - -N | N | = N | Y -N | Y | Y = N | Y - -N | Y | N = ~Y | N diff --git a/internal/controller/postgrescluster/cluster.go b/internal/controller/postgrescluster/cluster.go index 8d32679db3..cf463c118a 100644 --- a/internal/controller/postgrescluster/cluster.go +++ b/internal/controller/postgrescluster/cluster.go @@ -290,7 +290,9 @@ func (r *Reconciler) reconcileClusterReplicaService( func (r *Reconciler) reconcileDataSource(ctx context.Context, cluster *v1beta1.PostgresCluster, observed *observedInstances, clusterVolumes []corev1.PersistentVolumeClaim, - rootCA *pki.RootCertificateAuthority) (bool, error) { + rootCA *pki.RootCertificateAuthority, + backupsSpecFound, backupsReconciliationAllowed bool, +) (bool, error) { // a hash func to hash the pgBackRest restore options hashFunc := func(jobConfigs []string) (string, error) { @@ -413,12 +415,13 @@ func (r *Reconciler) reconcileDataSource(ctx context.Context, switch { case dataSource != nil: if err := r.reconcilePostgresClusterDataSource(ctx, cluster, dataSource, - configHash, clusterVolumes, rootCA); err != nil { + configHash, clusterVolumes, rootCA, + backupsSpecFound, backupsReconciliationAllowed); err != nil { return true, err } case cloudDataSource != nil: if err := r.reconcileCloudBasedDataSource(ctx, cluster, cloudDataSource, - configHash, clusterVolumes); err != nil { + configHash, clusterVolumes, backupsReconciliationAllowed); err != nil { return true, err } } diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index 5621a4992f..a605302ff2 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/internal/controller/postgrescluster/controller.go @@ -222,10 +222,11 @@ func (r *Reconciler) Reconcile( pgaudit.PostgreSQLParameters(&pgParameters) backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, cluster) - if err != nil || !backupsReconciliationAllowed { + // Alternatively, we could just bail here if backup reconciliation is paused? + if err != nil { return reconcile.Result{}, err } - pgbackrest.PostgreSQL(cluster, &pgParameters, backupsSpecFound) + pgbackrest.PostgreSQL(cluster, &pgParameters, backupsSpecFound || !backupsReconciliationAllowed) pgmonitor.PostgreSQLParameters(cluster, &pgParameters) // Set huge_pages = try if a hugepages resource limit > 0, otherwise set "off" @@ -292,7 +293,7 @@ func (r *Reconciler) Reconcile( // the controller should return early while data initialization is in progress, after // which it will indicate that an early return is no longer needed, and reconciliation // can proceed normally. - returnEarly, err := r.reconcileDataSource(ctx, cluster, instances, clusterVolumes, rootCA) + returnEarly, err := r.reconcileDataSource(ctx, cluster, instances, clusterVolumes, rootCA, backupsSpecFound, backupsReconciliationAllowed) if err != nil || returnEarly { return runtime.ErrorWithBackoff(errors.Join(err, patchClusterStatus())) } @@ -334,7 +335,9 @@ func (r *Reconciler) Reconcile( err = r.reconcileInstanceSets( ctx, cluster, clusterConfigMap, clusterReplicationSecret, rootCA, clusterPodService, instanceServiceAccount, instances, patroniLeaderService, - primaryCertificate, clusterVolumes, exporterQueriesConfig, exporterWebConfig) + primaryCertificate, clusterVolumes, exporterQueriesConfig, exporterWebConfig, + backupsSpecFound, backupsReconciliationAllowed, + ) } if err == nil { @@ -346,7 +349,9 @@ func (r *Reconciler) Reconcile( if err == nil { var next reconcile.Result - if next, err = r.reconcilePGBackRest(ctx, cluster, instances, rootCA); err == nil && !next.IsZero() { + if next, err = r.reconcilePGBackRest(ctx, cluster, + instances, rootCA, + backupsSpecFound, backupsReconciliationAllowed); err == nil && !next.IsZero() { result.Requeue = result.Requeue || next.Requeue if next.RequeueAfter > 0 { result.RequeueAfter = next.RequeueAfter diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 1b4968746d..e9afaff3ad 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -604,6 +604,7 @@ func (r *Reconciler) reconcileInstanceSets( primaryCertificate *corev1.SecretProjection, clusterVolumes []corev1.PersistentVolumeClaim, exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap, + backupsSpecFound, backupsReconciliationAllowed bool, ) error { // Go through the observed instances and check if a primary has been determined. @@ -640,7 +641,9 @@ func (r *Reconciler) reconcileInstanceSets( rootCA, clusterPodService, instanceServiceAccount, patroniLeaderService, primaryCertificate, findAvailableInstanceNames(*set, instances, clusterVolumes), - numInstancePods, clusterVolumes, exporterQueriesConfig, exporterWebConfig) + numInstancePods, clusterVolumes, exporterQueriesConfig, exporterWebConfig, + backupsSpecFound, backupsReconciliationAllowed, + ) if err == nil { err = r.reconcileInstanceSetPodDisruptionBudget(ctx, cluster, set) @@ -1079,6 +1082,7 @@ func (r *Reconciler) scaleUpInstances( numInstancePods int, clusterVolumes []corev1.PersistentVolumeClaim, exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap, + backupsSpecFound, backupsReconciliationAllowed bool, ) ([]*appsv1.StatefulSet, error) { log := logging.FromContext(ctx) @@ -1123,6 +1127,7 @@ func (r *Reconciler) scaleUpInstances( rootCA, clusterPodService, instanceServiceAccount, patroniLeaderService, primaryCertificate, instances[i], numInstancePods, clusterVolumes, exporterQueriesConfig, exporterWebConfig, + backupsSpecFound, backupsReconciliationAllowed, ) } if err == nil { @@ -1152,6 +1157,7 @@ func (r *Reconciler) reconcileInstance( numInstancePods int, clusterVolumes []corev1.PersistentVolumeClaim, exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap, + backupsSpecFound, backupsReconciliationAllowed bool, ) error { log := logging.FromContext(ctx).WithValues("instance", instance.Name) ctx = logging.NewContext(ctx, log) @@ -1198,11 +1204,7 @@ func (r *Reconciler) reconcileInstance( postgresDataVolume, postgresWALVolume, tablespaceVolumes, &instance.Spec.Template.Spec) - backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, cluster) - if err != nil || !backupsReconciliationAllowed { - return err - } - if backupsSpecFound { + if backupsSpecFound || !backupsReconciliationAllowed { addPGBackRestToInstancePodSpec( ctx, cluster, instanceCertificates, &instance.Spec.Template.Spec) } diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index 6145ece3d1..33d3227aa0 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -117,11 +117,14 @@ var regexRepoIndex = regexp.MustCompile(`\d+`) // RepoResources is used to store various resources for pgBackRest repositories and // repository hosts type RepoResources struct { + hosts []*appsv1.StatefulSet cronjobs []*batchv1.CronJob manualBackupJobs []*batchv1.Job replicaCreateBackupJobs []*batchv1.Job - hosts []*appsv1.StatefulSet pvcs []*corev1.PersistentVolumeClaim + sas []*corev1.ServiceAccount + roles []*rbacv1.Role + rolebindings []*rbacv1.RoleBinding } // applyRepoHostIntent ensures the pgBackRest repository host StatefulSet is synchronized with the @@ -192,24 +195,44 @@ func (r *Reconciler) applyRepoVolumeIntent(ctx context.Context, return repo, nil } +// +kubebuilder:rbac:groups="apps",resources="statefulsets",verbs={list} +// +kubebuilder:rbac:groups="batch",resources="cronjobs",verbs={list} +// +kubebuilder:rbac:groups="batch",resources="jobs",verbs={list} +// +kubebuilder:rbac:groups="",resources="configmaps",verbs={list} +// +kubebuilder:rbac:groups="",resources="persistentvolumeclaims",verbs={list} +// +kubebuilder:rbac:groups="",resources="secrets",verbs={list} +// +kubebuilder:rbac:groups="",resources="serviceaccounts",verbs={list} +// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="roles",verbs={list} +// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="rolebindings",verbs={list} + // getPGBackRestResources returns the existing pgBackRest resources that should utilized by the // PostgresCluster controller during reconciliation. Any items returned are verified to be owned // by the PostgresCluster controller and still applicable per the current PostgresCluster spec. // Additionally, and resources identified that no longer correspond to any current configuration // are deleted. func (r *Reconciler) getPGBackRestResources(ctx context.Context, - postgresCluster *v1beta1.PostgresCluster) (*RepoResources, error) { + postgresCluster *v1beta1.PostgresCluster, + backupsSpecFound, backupsReconciliationAllowed bool, +) (*RepoResources, error) { repoResources := &RepoResources{} gvks := []schema.GroupVersionKind{{ - Group: corev1.SchemeGroupVersion.Group, - Version: corev1.SchemeGroupVersion.Version, - Kind: "ConfigMapList", + Group: appsv1.SchemeGroupVersion.Group, + Version: appsv1.SchemeGroupVersion.Version, + Kind: "StatefulSetList", + }, { + Group: batchv1.SchemeGroupVersion.Group, + Version: batchv1.SchemeGroupVersion.Version, + Kind: "CronJobList", }, { Group: batchv1.SchemeGroupVersion.Group, Version: batchv1.SchemeGroupVersion.Version, Kind: "JobList", + }, { + Group: corev1.SchemeGroupVersion.Group, + Version: corev1.SchemeGroupVersion.Version, + Kind: "ConfigMapList", }, { Group: corev1.SchemeGroupVersion.Group, Version: corev1.SchemeGroupVersion.Version, @@ -219,13 +242,17 @@ func (r *Reconciler) getPGBackRestResources(ctx context.Context, Version: corev1.SchemeGroupVersion.Version, Kind: "SecretList", }, { - Group: appsv1.SchemeGroupVersion.Group, - Version: appsv1.SchemeGroupVersion.Version, - Kind: "StatefulSetList", + Group: corev1.SchemeGroupVersion.Group, + Version: corev1.SchemeGroupVersion.Version, + Kind: "ServiceAccountList", }, { - Group: batchv1.SchemeGroupVersion.Group, - Version: batchv1.SchemeGroupVersion.Version, - Kind: "CronJobList", + Group: rbacv1.SchemeGroupVersion.Group, + Version: rbacv1.SchemeGroupVersion.Version, + Kind: "RoleList", + }, { + Group: rbacv1.SchemeGroupVersion.Group, + Version: rbacv1.SchemeGroupVersion.Version, + Kind: "RoleBindingList", }} selector := naming.PGBackRestSelector(postgresCluster.GetName()) @@ -241,7 +268,7 @@ func (r *Reconciler) getPGBackRestResources(ctx context.Context, continue } - owned, err := r.cleanupRepoResources(ctx, postgresCluster, uList.Items) + owned, err := r.cleanupRepoResources(ctx, postgresCluster, uList.Items, backupsSpecFound, backupsReconciliationAllowed) if err != nil { return nil, errors.WithStack(err) } @@ -263,8 +290,11 @@ func (r *Reconciler) getPGBackRestResources(ctx context.Context, } // +kubebuilder:rbac:groups="",resources="persistentvolumeclaims",verbs={delete} +// +kubebuilder:rbac:groups="",resources="serviceaccounts",verbs={delete} // +kubebuilder:rbac:groups="apps",resources="statefulsets",verbs={delete} // +kubebuilder:rbac:groups="batch",resources="cronjobs",verbs={delete} +// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="roles",verbs={delete} +// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="rolebindings",verbs={delete} // cleanupRepoResources cleans up pgBackRest repository resources that should no longer be // reconciled by deleting them. This includes deleting repos (i.e. PersistentVolumeClaims) that @@ -272,7 +302,9 @@ func (r *Reconciler) getPGBackRestResources(ctx context.Context, // pgBackRest repository host resources if a repository host is no longer configured. func (r *Reconciler) cleanupRepoResources(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, - ownedResources []unstructured.Unstructured) ([]unstructured.Unstructured, error) { + ownedResources []unstructured.Unstructured, + backupsSpecFound, backupsReconciliationAllowed bool, +) ([]unstructured.Unstructured, error) { // stores the resources that should not be deleted ownedNoDelete := []unstructured.Unstructured{} @@ -287,11 +319,17 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, // spec switch { case hasLabel(naming.LabelPGBackRestConfig): + if !backupsSpecFound && backupsReconciliationAllowed { + break + } // Simply add the things we never want to delete (e.g. the pgBackRest configuration) // to the slice and do not delete ownedNoDelete = append(ownedNoDelete, owned) delete = false case hasLabel(naming.LabelPGBackRestDedicated): + if !backupsSpecFound && backupsReconciliationAllowed { + break + } // Any resources from before 5.1 that relate to the previously required // SSH configuration should be deleted. // TODO(tjmoore4): This can be removed once 5.0 is EOL. @@ -303,6 +341,9 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, delete = false } case hasLabel(naming.LabelPGBackRestRepoVolume): + if !backupsSpecFound && backupsReconciliationAllowed { + break + } // If a volume (PVC) is identified for a repo that no longer exists in the // spec then delete it. Otherwise add it to the slice and continue. for _, repo := range postgresCluster.Spec.Backups.PGBackRest.Repos { @@ -315,6 +356,9 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, } } case hasLabel(naming.LabelPGBackRestBackup): + if !backupsSpecFound && backupsReconciliationAllowed { + break + } // If a Job is identified for a repo that no longer exists in the spec then // delete it. Otherwise add it to the slice and continue. for _, repo := range postgresCluster.Spec.Backups.PGBackRest.Repos { @@ -324,6 +368,9 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, } } case hasLabel(naming.LabelPGBackRestCronJob): + if !backupsSpecFound && backupsReconciliationAllowed { + break + } for _, repo := range postgresCluster.Spec.Backups.PGBackRest.Repos { if repo.Name == owned.GetLabels()[naming.LabelPGBackRestRepo] { if backupScheduleFound(repo, @@ -335,6 +382,9 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, } } case hasLabel(naming.LabelPGBackRestRestore): + if !backupsSpecFound && backupsReconciliationAllowed { + break + } // When a cluster is prepared for restore, the system identifier is removed from status // and the cluster is therefore no longer bootstrapped. Only once the restore Job is // complete will the cluster then be bootstrapped again, which means by the time we @@ -344,6 +394,12 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, ownedNoDelete = append(ownedNoDelete, owned) delete = false } + case hasLabel(naming.LabelPGBackRest): + if !backupsSpecFound && backupsReconciliationAllowed { + break + } + ownedNoDelete = append(ownedNoDelete, owned) + delete = false } // If nothing has specified that the resource should not be deleted, then delete @@ -383,6 +439,24 @@ func unstructuredToRepoResources(kind string, repoResources *RepoResources, uList *unstructured.UnstructuredList) error { switch kind { + case "StatefulSetList": + var stsList appsv1.StatefulSetList + if err := runtime.DefaultUnstructuredConverter. + FromUnstructured(uList.UnstructuredContent(), &stsList); err != nil { + return errors.WithStack(err) + } + for i := range stsList.Items { + repoResources.hosts = append(repoResources.hosts, &stsList.Items[i]) + } + case "CronJobList": + var cronList batchv1.CronJobList + if err := runtime.DefaultUnstructuredConverter. + FromUnstructured(uList.UnstructuredContent(), &cronList); err != nil { + return errors.WithStack(err) + } + for i := range cronList.Items { + repoResources.cronjobs = append(repoResources.cronjobs, &cronList.Items[i]) + } case "JobList": var jobList batchv1.JobList if err := runtime.DefaultUnstructuredConverter. @@ -400,6 +474,9 @@ func unstructuredToRepoResources(kind string, repoResources *RepoResources, append(repoResources.manualBackupJobs, &jobList.Items[i]) } } + case "ConfigMapList": + // Repository host now uses mTLS for encryption, authentication, and authorization. + // Configmaps for SSHD are no longer managed here. case "PersistentVolumeClaimList": var pvcList corev1.PersistentVolumeClaimList if err := runtime.DefaultUnstructuredConverter. @@ -409,32 +486,38 @@ func unstructuredToRepoResources(kind string, repoResources *RepoResources, for i := range pvcList.Items { repoResources.pvcs = append(repoResources.pvcs, &pvcList.Items[i]) } - case "StatefulSetList": - var stsList appsv1.StatefulSetList + case "SecretList": + // Repository host now uses mTLS for encryption, authentication, and authorization. + // Secrets for SSHD are no longer managed here. + // TODO(tjmoore4): Consider adding all pgBackRest secrets to RepoResources to + // observe all pgBackRest secrets in one place. + case "ServiceAccountList": + var saList corev1.ServiceAccountList if err := runtime.DefaultUnstructuredConverter. - FromUnstructured(uList.UnstructuredContent(), &stsList); err != nil { + FromUnstructured(uList.UnstructuredContent(), &saList); err != nil { return errors.WithStack(err) } - for i := range stsList.Items { - repoResources.hosts = append(repoResources.hosts, &stsList.Items[i]) + for i := range saList.Items { + repoResources.sas = append(repoResources.sas, &saList.Items[i]) } - case "CronJobList": - var cronList batchv1.CronJobList + case "RoleList": + var roleList rbacv1.RoleList if err := runtime.DefaultUnstructuredConverter. - FromUnstructured(uList.UnstructuredContent(), &cronList); err != nil { + FromUnstructured(uList.UnstructuredContent(), &roleList); err != nil { return errors.WithStack(err) } - for i := range cronList.Items { - repoResources.cronjobs = append(repoResources.cronjobs, &cronList.Items[i]) + for i := range roleList.Items { + repoResources.roles = append(repoResources.roles, &roleList.Items[i]) + } + case "RoleBindingList": + var rb rbacv1.RoleBindingList + if err := runtime.DefaultUnstructuredConverter. + FromUnstructured(uList.UnstructuredContent(), &rb); err != nil { + return errors.WithStack(err) + } + for i := range rb.Items { + repoResources.rolebindings = append(repoResources.rolebindings, &rb.Items[i]) } - case "ConfigMapList": - // Repository host now uses mTLS for encryption, authentication, and authorization. - // Configmaps for SSHD are no longer managed here. - case "SecretList": - // Repository host now uses mTLS for encryption, authentication, and authorization. - // Secrets for SSHD are no longer managed here. - // TODO(tjmoore4): Consider adding all pgBackRest secrets to RepoResources to - // observe all pgBackRest secrets in one place. default: return fmt.Errorf("unexpected kind %q", kind) } @@ -1266,18 +1349,16 @@ func (r *Reconciler) generateRestoreJobIntent(cluster *v1beta1.PostgresCluster, func (r *Reconciler) reconcilePGBackRest(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, instances *observedInstances, - rootCA *pki.RootCertificateAuthority) (reconcile.Result, error) { + rootCA *pki.RootCertificateAuthority, + backupsSpecFound, backupsReconciliationAllowed bool, +) (reconcile.Result, error) { // add some additional context about what component is being reconciled log := logging.FromContext(ctx).WithValues("reconciler", "pgBackRest") - backupsSpecFound, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) - if err != nil { - return reconcile.Result{}, err - } - - if !backupReconciliationAllowed { - return reconcile.Result{}, err + // If backup reconciliation is paused, exit early + if !backupsReconciliationAllowed { + return reconcile.Result{}, nil } // if nil and backups are enabled, create the pgBackRest status that will be updated when @@ -1292,23 +1373,31 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, // Get all currently owned pgBackRest resources in the environment as needed for // reconciliation. This includes deleting resources that should no longer exist per the // current spec (e.g. if repos, repo hosts, etc. have been removed). - repoResources, err := r.getPGBackRestResources(ctx, postgresCluster) + repoResources, err := r.getPGBackRestResources(ctx, postgresCluster, backupsSpecFound, backupsReconciliationAllowed) if err != nil { // exit early if can't get and clean existing resources as needed to reconcile return reconcile.Result{}, errors.WithStack(err) } + // At this point, reconciliation is allowed, so if no backups spec is found + // clear the status and exit + if !backupsSpecFound { + postgresCluster.Status.PGBackRest = &v1beta1.PGBackRestStatus{} + return result, nil + } + var repoHost *appsv1.StatefulSet var repoHostName string // reconcile the pgbackrest repository host - repoHost, err = r.reconcileDedicatedRepoHost(ctx, postgresCluster, repoResources, instances) + repoHost, err = r.reconcileDedicatedRepoHost(ctx, postgresCluster, repoResources, instances, backupsSpecFound, backupsReconciliationAllowed) if err != nil { log.Error(err, "unable to reconcile pgBackRest repo host") result.Requeue = true return result, nil } - if err := r.reconcilePGBackRestSecret(ctx, postgresCluster, repoHost, rootCA); err != nil { + if err := r.reconcilePGBackRestSecret(ctx, postgresCluster, repoHost, + rootCA, backupsReconciliationAllowed); err != nil { log.Error(err, "unable to reconcile pgBackRest secret") result.Requeue = true } @@ -1348,27 +1437,28 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, if err := r.reconcilePGBackRestConfig(ctx, postgresCluster, repoHostName, configHash, naming.ClusterPodService(postgresCluster).Name, - postgresCluster.GetNamespace(), instanceNames); err != nil { + postgresCluster.GetNamespace(), instanceNames, + backupsReconciliationAllowed); err != nil { log.Error(err, "unable to reconcile pgBackRest configuration") result.Requeue = true } // reconcile the RBAC required to run pgBackRest Jobs (e.g. for backups) - sa, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster) + sa, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster, backupsReconciliationAllowed) if err != nil { log.Error(err, "unable to create replica creation backup") result.Requeue = true return result, nil } - if !backupsSpecFound { - // Clear the status and exit - postgresCluster.Status.PGBackRest = &v1beta1.PGBackRestStatus{} - return result, nil - } + // if !backupsSpecFound { + // // Clear the status and exit + // postgresCluster.Status.PGBackRest = &v1beta1.PGBackRestStatus{} + // return result, nil + // } // reconcile the pgBackRest stanza for all configuration pgBackRest repos - configHashMismatch, err := r.reconcileStanzaCreate(ctx, postgresCluster, instances, configHash) + configHashMismatch, err := r.reconcileStanzaCreate(ctx, postgresCluster, instances, configHash, backupsSpecFound, backupsReconciliationAllowed) // If a stanza create error then requeue but don't return the error. This prevents // stanza-create errors from bubbling up to the main Reconcile() function, which would // prevent subsequent reconciles from occurring. Also, this provides a better chance @@ -1431,7 +1521,9 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, func (r *Reconciler) reconcilePostgresClusterDataSource(ctx context.Context, cluster *v1beta1.PostgresCluster, dataSource *v1beta1.PostgresClusterDataSource, configHash string, clusterVolumes []corev1.PersistentVolumeClaim, - rootCA *pki.RootCertificateAuthority) error { + rootCA *pki.RootCertificateAuthority, + backupsSpecFound, backupsReconciliationAllowed bool, +) error { // grab cluster, namespaces and repo name information from the data source sourceClusterName := dataSource.ClusterName @@ -1513,7 +1605,7 @@ func (r *Reconciler) reconcilePostgresClusterDataSource(ctx context.Context, // Note that function reconcilePGBackRest only uses forCluster in observedInstances. result, err := r.reconcilePGBackRest(ctx, cluster, &observedInstances{ forCluster: []*Instance{instance}, - }, rootCA) + }, rootCA, backupsSpecFound, backupsReconciliationAllowed) if err != nil || result != (reconcile.Result{}) { return fmt.Errorf("unable to reconcile pgBackRest as needed to initialize "+ "PostgreSQL data for the cluster: %w", err) @@ -1589,7 +1681,9 @@ func (r *Reconciler) reconcilePostgresClusterDataSource(ctx context.Context, // data source, i.e., S3, etc. func (r *Reconciler) reconcileCloudBasedDataSource(ctx context.Context, cluster *v1beta1.PostgresCluster, dataSource *v1beta1.PGBackRestDataSource, - configHash string, clusterVolumes []corev1.PersistentVolumeClaim) error { + configHash string, clusterVolumes []corev1.PersistentVolumeClaim, + backupsReconciliationAllowed bool, +) error { // Ensure the proper instance and instance set can be identified via the status. The // StartupInstance and StartupInstanceSet values should be populated when the cluster @@ -1640,7 +1734,7 @@ func (r *Reconciler) reconcileCloudBasedDataSource(ctx context.Context, return nil } - if err := r.createRestoreConfig(ctx, cluster, configHash); err != nil { + if err := r.createRestoreConfig(ctx, cluster, configHash, backupsReconciliationAllowed); err != nil { return err } @@ -1693,7 +1787,7 @@ func (r *Reconciler) reconcileCloudBasedDataSource(ctx context.Context, // createRestoreConfig creates a configmap struct with pgBackRest pgbackrest.conf settings // in the data field, for use with restoring from cloud-based data sources func (r *Reconciler) createRestoreConfig(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, - configHash string) error { + configHash string, backupsReconciliationAllowed bool) error { postgresClusterWithMockedBackups := postgresCluster.DeepCopy() postgresClusterWithMockedBackups.Spec.Backups.PGBackRest.Global = postgresCluster.Spec. @@ -1703,7 +1797,7 @@ func (r *Reconciler) createRestoreConfig(ctx context.Context, postgresCluster *v } return r.reconcilePGBackRestConfig(ctx, postgresClusterWithMockedBackups, - "", configHash, "", "", []string{}) + "", configHash, "", "", []string{}, backupsReconciliationAllowed) } // copyRestoreConfiguration copies pgBackRest configuration from another cluster for use by @@ -1917,31 +2011,15 @@ func (r *Reconciler) copyConfigurationResources(ctx context.Context, cluster, func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, repoHostName, configHash, serviceName, serviceNamespace string, - instanceNames []string) error { - - backupsSpecFound, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) - if err != nil { - return err - } + instanceNames []string, backupsReconciliationAllowed bool, +) error { - // If we have no backup spec, yes STS, no annotation, pause reconciliation - // return an error, emit event, log warning? - if !backupReconciliationAllowed { + // As a safeguard, if reconciliation is not allowed, exit early. + // (We should never get here because the calling function should exit + // early under this condition.) + if !backupsReconciliationAllowed { return nil } - // Remove the existing pgBackRest config map, if backups have been turned off. - if !backupsSpecFound { - existing := &corev1.ConfigMap{ - ObjectMeta: naming.PGBackRestConfig(postgresCluster), - } - err := errors.WithStack(client.IgnoreNotFound( - r.Client.Get(ctx, client.ObjectKeyFromObject(existing), existing))) - if err != nil { - return err - } - err = errors.WithStack(client.IgnoreNotFound(r.deleteControlled(ctx, postgresCluster, existing))) - return err - } backrestConfig := pgbackrest.CreatePGBackRestConfigMapIntent(postgresCluster, repoHostName, configHash, serviceName, serviceNamespace, instanceNames) @@ -1962,7 +2040,15 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, // reconcilePGBackRestSecret reconciles the pgBackRest Secret. func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, cluster *v1beta1.PostgresCluster, repoHost *appsv1.StatefulSet, - rootCA *pki.RootCertificateAuthority) error { + rootCA *pki.RootCertificateAuthority, + backupsReconciliationAllowed bool, +) error { + // As a safeguard, if reconciliation is not allowed, exit early. + // (We should never get here because the calling function should exit + // early under this condition.) + if !backupsReconciliationAllowed { + return nil + } intent := &corev1.Secret{ObjectMeta: naming.PGBackRestSecret(cluster)} intent.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("Secret")) @@ -1984,24 +2070,6 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, if err != nil { return err } - backupSpecFound, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, cluster) - if err != nil { - return err - } - if !backupReconciliationAllowed { - return err - } - // If backups have been turned off, remove the pgBackRest secret. - if !backupSpecFound { - err = errors.WithStack(client.IgnoreNotFound( - r.deleteControlled(ctx, cluster, existing))) - return err - } - - // exit early, pause reconciliation - if !backupReconciliationAllowed { - return err - } if err == nil { err = r.setControllerReference(cluster, intent) @@ -2023,22 +2091,25 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, return err } -// +kubebuilder:rbac:groups="",resources="serviceaccounts",verbs={create,patch, delete} -// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="roles",verbs={create,patch, delete} -// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="rolebindings",verbs={create,patch, delete} +// +kubebuilder:rbac:groups="",resources="serviceaccounts",verbs={create,patch} +// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="roles",verbs={create,patch} +// +kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources="rolebindings",verbs={create,patch} // reconcileInstanceRBAC reconciles the Role, RoleBinding, and ServiceAccount for // pgBackRest func (r *Reconciler) reconcilePGBackRestRBAC(ctx context.Context, - postgresCluster *v1beta1.PostgresCluster) (*corev1.ServiceAccount, error) { + postgresCluster *v1beta1.PostgresCluster, + backupsReconciliationAllowed bool) (*corev1.ServiceAccount, error) { sa := &corev1.ServiceAccount{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} role := &rbacv1.Role{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} binding := &rbacv1.RoleBinding{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} - backupsSpecFound, backupsReconciliationAllowed, _ := r.BackupsEnabled(ctx, postgresCluster) - - // We should never have to run logic for !backupsReconciliationAllowed. + // As a safeguard, if reconciliation is not allowed, exit early. + // (We should never get here because the calling function should exit + // early under this condition.) + // Because the calling function expects a valid SA to be returned, + // we find the existing SA and return it if possible. if !backupsReconciliationAllowed { err := errors.WithStack(client.IgnoreNotFound( r.Client.Get(ctx, client.ObjectKeyFromObject(sa), sa))) @@ -2049,43 +2120,6 @@ func (r *Reconciler) reconcilePGBackRestRBAC(ctx context.Context, return sa, err } - if !backupsSpecFound { - err := errors.WithStack(client.IgnoreNotFound( - r.Client.Get(ctx, client.ObjectKeyFromObject(sa), sa))) - // Delete the Secret when it exists and there is nothing we want to keep in it. - if err == nil && len(sa.UID) != 0 { - err = errors.WithStack(client.IgnoreNotFound( - r.deleteControlled(ctx, postgresCluster, sa))) - } - if err != nil { - return nil, err - } - - err = errors.WithStack(client.IgnoreNotFound( - r.Client.Get(ctx, client.ObjectKeyFromObject(role), role))) - // Delete the Secret when it exists and there is nothing we want to keep in it. - if err == nil && len(sa.UID) != 0 { - err = errors.WithStack(client.IgnoreNotFound( - r.deleteControlled(ctx, postgresCluster, role))) - } - if err != nil { - return nil, err - } - - err = errors.WithStack(client.IgnoreNotFound( - r.Client.Get(ctx, client.ObjectKeyFromObject(binding), binding))) - // Delete the Secret when it exists and there is nothing we want to keep in it. - if err == nil && len(sa.UID) != 0 { - err = errors.WithStack(client.IgnoreNotFound( - r.deleteControlled(ctx, postgresCluster, binding))) - } - if err != nil { - return nil, err - } - - return nil, nil - } - sa.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ServiceAccount")) role.SetGroupVersionKind(rbacv1.SchemeGroupVersion.WithKind("Role")) @@ -2147,45 +2181,23 @@ func (r *Reconciler) reconcilePGBackRestRBAC(ctx context.Context, func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, repoResources *RepoResources, - observedInstances *observedInstances) (*appsv1.StatefulSet, error) { + observedInstances *observedInstances, + backupsSpecFound, backupsReconciliationAllowed bool, +) (*appsv1.StatefulSet, error) { log := logging.FromContext(ctx).WithValues("reconcileResource", "repoHost") - backupsSpecFound, backupReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) - if err != nil { - return nil, err - } - if !backupReconciliationAllowed { - return nil, err - } - - if !backupsSpecFound { - // When backups are disabled, remove the backrest StatefulSet, if one exists. - name := fmt.Sprintf("%s-%s", postgresCluster.GetName(), "repo-host") - existing := &appsv1.StatefulSet{} - sts := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: postgresCluster.Namespace, Name: name}} - err := errors.WithStack(client.IgnoreNotFound( - r.Client.Get(ctx, client.ObjectKeyFromObject(sts), existing))) - if err != nil { - return nil, err - } - - err = errors.WithStack(client.IgnoreNotFound( - r.deleteControlled(ctx, postgresCluster, existing))) - - return nil, err - } - - // Do we want to get the existing STS and return it? - // What we don't want to do is generate a new STS from the hobbled spec - if !backupReconciliationAllowed { - return nil, err + // As a safeguard, if reconciliation is not allowed, exit early. + // (We should never get here because the calling function should exit + // early under this condition.) + // FIXME: Do we want to get the existing STS and return it? + if !backupsReconciliationAllowed { + return nil, nil } // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { - backupsSpecFound, backupReconciliationAllowed, _ := r.BackupsEnabled(ctx, postgresCluster) - if !backupReconciliationAllowed || !backupsSpecFound { + if !backupsReconciliationAllowed || !backupsSpecFound { return } repoHostReady := metav1.Condition{ @@ -2665,16 +2677,14 @@ func (r *Reconciler) reconcileRepos(ctx context.Context, // propagated to the Pod). func (r *Reconciler) reconcileStanzaCreate(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, - instances *observedInstances, configHash string) (bool, error) { + instances *observedInstances, configHash string, + backupsSpecFound, backupsReconciliationAllowed bool, +) (bool, error) { // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { var replicaCreateRepoStatus *v1beta1.RepoStatus - backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) - if err != nil || !backupsReconciliationAllowed { - return - } - if !backupsSpecFound || len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { + if !backupsReconciliationAllowed || !backupsSpecFound || len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { return } replicaCreateRepoName := postgresCluster.Spec.Backups.PGBackRest.Repos[0].Name @@ -3076,9 +3086,15 @@ func (r *Reconciler) reconcilePGBackRestCronJob( return err } -// returns backups enabled +// BackupsEnabled checks the state of the backups (i.e., if backups are in the spec, +// if a repo-host StatefulSet exists, if the annotation permitting backup deletion exist) +// and determines whether reconciliation is allowed. +// Reconciliation of backup-related Kubernetes objects is paused if +// - a user created a cluster with backups; +// - the cluster is updated to remove backups; +// - the annotation authorizing that removal is missing. // -// reconciliation paused +// This function also returns whether the spec has a defined backups or not. func (r *Reconciler) BackupsEnabled( ctx context.Context, postgresCluster *v1beta1.PostgresCluster, @@ -3110,14 +3126,21 @@ func (r *Reconciler) BackupsEnabled( return backupsSpecFound, backupsReconciliationAllowed, err } -// Return backup spec found, -// return sts not found, -// return annotation found, -// return err -func (r *Reconciler) ObserveBackupUniverse(ctx context.Context, postgresCluster *v1beta1.PostgresCluster) (bool, bool, bool, error) { - var err error - backupSpecPresent := !reflect.DeepEqual(postgresCluster.Spec.Backups, v1beta1.Backups{PGBackRest: v1beta1.PGBackRestArchive{}}) +// ObserveBackupUniverse returns +// - wheterh the spec has backups defined; +func (r *Reconciler) ObserveBackupUniverse(ctx context.Context, + postgresCluster *v1beta1.PostgresCluster, +) ( + backupsSpecFound bool, + repoHostStatefulSetNotFound bool, + backupsRemovalAnnotationFound bool, + err error, +) { + + // Does the cluster have a blank Backups section + backupsSpecFound = !reflect.DeepEqual(postgresCluster.Spec.Backups, v1beta1.Backups{PGBackRest: v1beta1.PGBackRestArchive{}}) + // Does the repo-host StatefulSet exist? name := fmt.Sprintf("%s-%s", postgresCluster.GetName(), "repo-host") existing := &appsv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ @@ -3127,22 +3150,22 @@ func (r *Reconciler) ObserveBackupUniverse(ctx context.Context, postgresCluster } err = errors.WithStack( r.Client.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + repoHostStatefulSetNotFound = apierrors.IsNotFound(err) - stsNotFound := apierrors.IsNotFound(err) - - // // No spec, sts unsure... what do we do? - // if err != nil && !stsNotFound { - // return true, false, err // FIXME: what's the correct behavior if we get an error - // } - - annotationFound := r.DestroyBackupsAnnotationPresent(postgresCluster) + // If we have an error that is not related to a missing repo-host StatefulSet, + // we return an error and expect the calling function to correctly stop processing. + if err != nil && !repoHostStatefulSetNotFound { + return false, false, false, err + } - // TODO: do something sensible with the err you're returning - return backupSpecPresent, stsNotFound, annotationFound, nil + backupsRemovalAnnotationFound = authorizeBackupRemovalAnnotationPresent(postgresCluster) + // If we have reached this point, the err is either nil or an IsNotFound error + // which we do not care about; hence, pass nil rather than the err + return backupsSpecFound, repoHostStatefulSetNotFound, backupsRemovalAnnotationFound, nil } -func (r *Reconciler) DestroyBackupsAnnotationPresent(postgresCluster *v1beta1.PostgresCluster) bool { +func authorizeBackupRemovalAnnotationPresent(postgresCluster *v1beta1.PostgresCluster) bool { annotations := postgresCluster.GetAnnotations() for annotation := range annotations { if annotation == naming.AuthorizeBackupRemovalAnnotation { diff --git a/internal/controller/postgrescluster/pgbackrest_test.go b/internal/controller/postgrescluster/pgbackrest_test.go index 5b67da0bca..c43d5a80e5 100644 --- a/internal/controller/postgrescluster/pgbackrest_test.go +++ b/internal/controller/postgrescluster/pgbackrest_test.go @@ -206,7 +206,7 @@ func TestReconcilePGBackRest(t *testing.T) { postgresCluster := fakePostgresCluster(clusterName, ns.GetName(), clusterUID, true) // create a service account to test with - serviceAccount, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster) + serviceAccount, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster, true) assert.NilError(t, err) assert.Assert(t, serviceAccount != nil) @@ -243,7 +243,7 @@ func TestReconcilePGBackRest(t *testing.T) { rootCA, err := pki.NewRootCertificateAuthority() assert.NilError(t, err) - result, err := r.reconcilePGBackRest(ctx, postgresCluster, instances, rootCA) + result, err := r.reconcilePGBackRest(ctx, postgresCluster, instances, rootCA, true, true) if err != nil || result != (reconcile.Result{}) { t.Errorf("unable to reconcile pgBackRest: %v", err) } @@ -626,7 +626,7 @@ func TestReconcilePGBackRestRBAC(t *testing.T) { Repos: []v1beta1.RepoStatus{{Name: "repo1", StanzaCreated: false}}, } - serviceAccount, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster) + serviceAccount, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster, true) assert.NilError(t, err) assert.Assert(t, serviceAccount != nil) @@ -720,7 +720,7 @@ func TestReconcileStanzaCreate(t *testing.T) { Message: "pgBackRest dedicated repository host is ready", }) - configHashMismatch, err := r.reconcileStanzaCreate(ctx, postgresCluster, instances, "abcde12345") + configHashMismatch, err := r.reconcileStanzaCreate(ctx, postgresCluster, instances, "abcde12345", true, true) assert.NilError(t, err) assert.Assert(t, !configHashMismatch) @@ -759,7 +759,7 @@ func TestReconcileStanzaCreate(t *testing.T) { SystemIdentifier: "6952526174828511264", } - configHashMismatch, err = r.reconcileStanzaCreate(ctx, postgresCluster, instances, "abcde12345") + configHashMismatch, err = r.reconcileStanzaCreate(ctx, postgresCluster, instances, "abcde12345", true, true) assert.Error(t, err, "fake stanza create failed: ") assert.Assert(t, !configHashMismatch) @@ -1641,7 +1641,7 @@ func TestGetPGBackRestResources(t *testing.T) { assert.NilError(t, err) assert.NilError(t, tClient.Create(ctx, resource)) - resources, err := r.getPGBackRestResources(ctx, tc.cluster) + resources, err := r.getPGBackRestResources(ctx, tc.cluster, true, true) assert.NilError(t, err) assert.Assert(t, tc.result.jobCount == len(resources.replicaCreateBackupJobs)) @@ -1878,7 +1878,7 @@ func TestReconcilePostgresClusterDataSource(t *testing.T) { pgclusterDataSource = tc.dataSource.PostgresCluster } err := r.reconcilePostgresClusterDataSource(ctx, cluster, pgclusterDataSource, - "testhash", nil, rootCA) + "testhash", nil, rootCA, true, true) assert.NilError(t, err) restoreConfig := &corev1.ConfigMap{} @@ -2072,6 +2072,7 @@ func TestReconcileCloudBasedDataSource(t *testing.T) { pgclusterDataSource, "testhash", nil, + true, ) assert.NilError(t, err) diff --git a/internal/pgbackrest/postgres_test.go b/internal/pgbackrest/postgres_test.go index da41b86281..559388e926 100644 --- a/internal/pgbackrest/postgres_test.go +++ b/internal/pgbackrest/postgres_test.go @@ -28,7 +28,7 @@ func TestPostgreSQLParameters(t *testing.T) { cluster := new(v1beta1.PostgresCluster) parameters := new(postgres.Parameters) - PostgreSQL(cluster, parameters) + PostgreSQL(cluster, parameters, true) assert.DeepEqual(t, parameters.Mandatory.AsMap(), map[string]string{ "archive_mode": "on", "archive_command": `pgbackrest --stanza=db archive-push "%p"`, @@ -39,12 +39,19 @@ func TestPostgreSQLParameters(t *testing.T) { "archive_timeout": "60s", }) + PostgreSQL(cluster, parameters, false) + assert.DeepEqual(t, parameters.Mandatory.AsMap(), map[string]string{ + "archive_mode": "on", + "archive_command": "true", + "restore_command": `pgbackrest --stanza=db archive-get %f "%p"`, + }) + cluster.Spec.Standby = &v1beta1.PostgresStandbySpec{ Enabled: true, RepoName: "repo99", } - PostgreSQL(cluster, parameters) + PostgreSQL(cluster, parameters, true) assert.DeepEqual(t, parameters.Mandatory.AsMap(), map[string]string{ "archive_mode": "on", "archive_command": `pgbackrest --stanza=db archive-push "%p"`, From 4cb296ebe0282b175df2d10e53052652dc72fb3a Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 22 Aug 2024 15:08:30 -0500 Subject: [PATCH 08/13] Add test; stop reconciling --- .../controller/postgrescluster/cluster.go | 6 +- .../controller/postgrescluster/controller.go | 69 +- .../controller/postgrescluster/instance.go | 12 +- .../controller/postgrescluster/pgbackrest.go | 126 +-- .../postgrescluster/pgbackrest_test.go | 809 +++++++++++------- 5 files changed, 605 insertions(+), 417 deletions(-) diff --git a/internal/controller/postgrescluster/cluster.go b/internal/controller/postgrescluster/cluster.go index cf463c118a..2018dc3f95 100644 --- a/internal/controller/postgrescluster/cluster.go +++ b/internal/controller/postgrescluster/cluster.go @@ -291,7 +291,7 @@ func (r *Reconciler) reconcileDataSource(ctx context.Context, cluster *v1beta1.PostgresCluster, observed *observedInstances, clusterVolumes []corev1.PersistentVolumeClaim, rootCA *pki.RootCertificateAuthority, - backupsSpecFound, backupsReconciliationAllowed bool, + backupsSpecFound bool, ) (bool, error) { // a hash func to hash the pgBackRest restore options @@ -416,12 +416,12 @@ func (r *Reconciler) reconcileDataSource(ctx context.Context, case dataSource != nil: if err := r.reconcilePostgresClusterDataSource(ctx, cluster, dataSource, configHash, clusterVolumes, rootCA, - backupsSpecFound, backupsReconciliationAllowed); err != nil { + backupsSpecFound); err != nil { return true, err } case cloudDataSource != nil: if err := r.reconcileCloudBasedDataSource(ctx, cluster, cloudDataSource, - configHash, clusterVolumes, backupsReconciliationAllowed); err != nil { + configHash, clusterVolumes); err != nil { return true, err } } diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index a605302ff2..07eed90f79 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/internal/controller/postgrescluster/controller.go @@ -162,21 +162,23 @@ func (r *Reconciler) Reconcile( } var ( - clusterConfigMap *corev1.ConfigMap - clusterReplicationSecret *corev1.Secret - clusterPodService *corev1.Service - clusterVolumes []corev1.PersistentVolumeClaim - instanceServiceAccount *corev1.ServiceAccount - instances *observedInstances - patroniLeaderService *corev1.Service - primaryCertificate *corev1.SecretProjection - primaryService *corev1.Service - replicaService *corev1.Service - rootCA *pki.RootCertificateAuthority - monitoringSecret *corev1.Secret - exporterQueriesConfig *corev1.ConfigMap - exporterWebConfig *corev1.ConfigMap - err error + clusterConfigMap *corev1.ConfigMap + clusterReplicationSecret *corev1.Secret + clusterPodService *corev1.Service + clusterVolumes []corev1.PersistentVolumeClaim + instanceServiceAccount *corev1.ServiceAccount + instances *observedInstances + patroniLeaderService *corev1.Service + primaryCertificate *corev1.SecretProjection + primaryService *corev1.Service + replicaService *corev1.Service + rootCA *pki.RootCertificateAuthority + monitoringSecret *corev1.Secret + exporterQueriesConfig *corev1.ConfigMap + exporterWebConfig *corev1.ConfigMap + err error + backupsSpecFound bool + backupsReconciliationAllowed bool ) patchClusterStatus := func() error { @@ -214,19 +216,33 @@ func (r *Reconciler) Reconcile( meta.RemoveStatusCondition(&cluster.Status.Conditions, v1beta1.PostgresClusterProgressing) } + if err == nil { + backupsSpecFound, backupsReconciliationAllowed, err = r.BackupsEnabled(ctx, cluster) + + // If we cannot reconcile because the backup reconciliation is paused, set a condition and exit + if !backupsReconciliationAllowed { + meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ + Type: v1beta1.PostgresClusterProgressing, + Status: metav1.ConditionFalse, + Reason: "Paused", + Message: "Reconciliation is paused: please fill in spec.backups " + + "or add the postgres-operator.crunchydata.com/authorizeBackupRemoval " + + "annotation to authorize backup removal.", + + ObservedGeneration: cluster.GetGeneration(), + }) + return runtime.ErrorWithBackoff(patchClusterStatus()) + } else { + meta.RemoveStatusCondition(&cluster.Status.Conditions, v1beta1.PostgresClusterProgressing) + } + } + pgHBAs := postgres.NewHBAs() pgmonitor.PostgreSQLHBAs(cluster, &pgHBAs) pgbouncer.PostgreSQL(cluster, &pgHBAs) - pgParameters := postgres.NewParameters() pgaudit.PostgreSQLParameters(&pgParameters) - - backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, cluster) - // Alternatively, we could just bail here if backup reconciliation is paused? - if err != nil { - return reconcile.Result{}, err - } - pgbackrest.PostgreSQL(cluster, &pgParameters, backupsSpecFound || !backupsReconciliationAllowed) + pgbackrest.PostgreSQL(cluster, &pgParameters, backupsSpecFound) pgmonitor.PostgreSQLParameters(cluster, &pgParameters) // Set huge_pages = try if a hugepages resource limit > 0, otherwise set "off" @@ -293,7 +309,7 @@ func (r *Reconciler) Reconcile( // the controller should return early while data initialization is in progress, after // which it will indicate that an early return is no longer needed, and reconciliation // can proceed normally. - returnEarly, err := r.reconcileDataSource(ctx, cluster, instances, clusterVolumes, rootCA, backupsSpecFound, backupsReconciliationAllowed) + returnEarly, err := r.reconcileDataSource(ctx, cluster, instances, clusterVolumes, rootCA, backupsSpecFound) if err != nil || returnEarly { return runtime.ErrorWithBackoff(errors.Join(err, patchClusterStatus())) } @@ -336,7 +352,7 @@ func (r *Reconciler) Reconcile( ctx, cluster, clusterConfigMap, clusterReplicationSecret, rootCA, clusterPodService, instanceServiceAccount, instances, patroniLeaderService, primaryCertificate, clusterVolumes, exporterQueriesConfig, exporterWebConfig, - backupsSpecFound, backupsReconciliationAllowed, + backupsSpecFound, ) } @@ -350,8 +366,7 @@ func (r *Reconciler) Reconcile( if err == nil { var next reconcile.Result if next, err = r.reconcilePGBackRest(ctx, cluster, - instances, rootCA, - backupsSpecFound, backupsReconciliationAllowed); err == nil && !next.IsZero() { + instances, rootCA, backupsSpecFound); err == nil && !next.IsZero() { result.Requeue = result.Requeue || next.Requeue if next.RequeueAfter > 0 { result.RequeueAfter = next.RequeueAfter diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index e9afaff3ad..fb043c3013 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -604,7 +604,7 @@ func (r *Reconciler) reconcileInstanceSets( primaryCertificate *corev1.SecretProjection, clusterVolumes []corev1.PersistentVolumeClaim, exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap, - backupsSpecFound, backupsReconciliationAllowed bool, + backupsSpecFound bool, ) error { // Go through the observed instances and check if a primary has been determined. @@ -642,7 +642,7 @@ func (r *Reconciler) reconcileInstanceSets( patroniLeaderService, primaryCertificate, findAvailableInstanceNames(*set, instances, clusterVolumes), numInstancePods, clusterVolumes, exporterQueriesConfig, exporterWebConfig, - backupsSpecFound, backupsReconciliationAllowed, + backupsSpecFound, ) if err == nil { @@ -1082,7 +1082,7 @@ func (r *Reconciler) scaleUpInstances( numInstancePods int, clusterVolumes []corev1.PersistentVolumeClaim, exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap, - backupsSpecFound, backupsReconciliationAllowed bool, + backupsSpecFound bool, ) ([]*appsv1.StatefulSet, error) { log := logging.FromContext(ctx) @@ -1127,7 +1127,7 @@ func (r *Reconciler) scaleUpInstances( rootCA, clusterPodService, instanceServiceAccount, patroniLeaderService, primaryCertificate, instances[i], numInstancePods, clusterVolumes, exporterQueriesConfig, exporterWebConfig, - backupsSpecFound, backupsReconciliationAllowed, + backupsSpecFound, ) } if err == nil { @@ -1157,7 +1157,7 @@ func (r *Reconciler) reconcileInstance( numInstancePods int, clusterVolumes []corev1.PersistentVolumeClaim, exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap, - backupsSpecFound, backupsReconciliationAllowed bool, + backupsSpecFound bool, ) error { log := logging.FromContext(ctx).WithValues("instance", instance.Name) ctx = logging.NewContext(ctx, log) @@ -1204,7 +1204,7 @@ func (r *Reconciler) reconcileInstance( postgresDataVolume, postgresWALVolume, tablespaceVolumes, &instance.Spec.Template.Spec) - if backupsSpecFound || !backupsReconciliationAllowed { + if backupsSpecFound { addPGBackRestToInstancePodSpec( ctx, cluster, instanceCertificates, &instance.Spec.Template.Spec) } diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index 33d3227aa0..4927de2a53 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -212,7 +212,7 @@ func (r *Reconciler) applyRepoVolumeIntent(ctx context.Context, // are deleted. func (r *Reconciler) getPGBackRestResources(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, - backupsSpecFound, backupsReconciliationAllowed bool, + backupsSpecFound bool, ) (*RepoResources, error) { repoResources := &RepoResources{} @@ -268,7 +268,7 @@ func (r *Reconciler) getPGBackRestResources(ctx context.Context, continue } - owned, err := r.cleanupRepoResources(ctx, postgresCluster, uList.Items, backupsSpecFound, backupsReconciliationAllowed) + owned, err := r.cleanupRepoResources(ctx, postgresCluster, uList.Items, backupsSpecFound) if err != nil { return nil, errors.WithStack(err) } @@ -303,7 +303,7 @@ func (r *Reconciler) getPGBackRestResources(ctx context.Context, func (r *Reconciler) cleanupRepoResources(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, ownedResources []unstructured.Unstructured, - backupsSpecFound, backupsReconciliationAllowed bool, + backupsSpecFound bool, ) ([]unstructured.Unstructured, error) { // stores the resources that should not be deleted @@ -319,7 +319,7 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, // spec switch { case hasLabel(naming.LabelPGBackRestConfig): - if !backupsSpecFound && backupsReconciliationAllowed { + if !backupsSpecFound { break } // Simply add the things we never want to delete (e.g. the pgBackRest configuration) @@ -327,7 +327,7 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, ownedNoDelete = append(ownedNoDelete, owned) delete = false case hasLabel(naming.LabelPGBackRestDedicated): - if !backupsSpecFound && backupsReconciliationAllowed { + if !backupsSpecFound { break } // Any resources from before 5.1 that relate to the previously required @@ -341,7 +341,7 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, delete = false } case hasLabel(naming.LabelPGBackRestRepoVolume): - if !backupsSpecFound && backupsReconciliationAllowed { + if !backupsSpecFound { break } // If a volume (PVC) is identified for a repo that no longer exists in the @@ -356,7 +356,7 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, } } case hasLabel(naming.LabelPGBackRestBackup): - if !backupsSpecFound && backupsReconciliationAllowed { + if !backupsSpecFound { break } // If a Job is identified for a repo that no longer exists in the spec then @@ -368,7 +368,7 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, } } case hasLabel(naming.LabelPGBackRestCronJob): - if !backupsSpecFound && backupsReconciliationAllowed { + if !backupsSpecFound { break } for _, repo := range postgresCluster.Spec.Backups.PGBackRest.Repos { @@ -382,7 +382,7 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, } } case hasLabel(naming.LabelPGBackRestRestore): - if !backupsSpecFound && backupsReconciliationAllowed { + if !backupsSpecFound { break } // When a cluster is prepared for restore, the system identifier is removed from status @@ -395,7 +395,7 @@ func (r *Reconciler) cleanupRepoResources(ctx context.Context, delete = false } case hasLabel(naming.LabelPGBackRest): - if !backupsSpecFound && backupsReconciliationAllowed { + if !backupsSpecFound { break } ownedNoDelete = append(ownedNoDelete, owned) @@ -1350,17 +1350,12 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, instances *observedInstances, rootCA *pki.RootCertificateAuthority, - backupsSpecFound, backupsReconciliationAllowed bool, + backupsSpecFound bool, ) (reconcile.Result, error) { // add some additional context about what component is being reconciled log := logging.FromContext(ctx).WithValues("reconciler", "pgBackRest") - // If backup reconciliation is paused, exit early - if !backupsReconciliationAllowed { - return reconcile.Result{}, nil - } - // if nil and backups are enabled, create the pgBackRest status that will be updated when // reconciling various pgBackRest resources if postgresCluster.Status.PGBackRest == nil { @@ -1373,7 +1368,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, // Get all currently owned pgBackRest resources in the environment as needed for // reconciliation. This includes deleting resources that should no longer exist per the // current spec (e.g. if repos, repo hosts, etc. have been removed). - repoResources, err := r.getPGBackRestResources(ctx, postgresCluster, backupsSpecFound, backupsReconciliationAllowed) + repoResources, err := r.getPGBackRestResources(ctx, postgresCluster, backupsSpecFound) if err != nil { // exit early if can't get and clean existing resources as needed to reconcile return reconcile.Result{}, errors.WithStack(err) @@ -1389,7 +1384,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, var repoHost *appsv1.StatefulSet var repoHostName string // reconcile the pgbackrest repository host - repoHost, err = r.reconcileDedicatedRepoHost(ctx, postgresCluster, repoResources, instances, backupsSpecFound, backupsReconciliationAllowed) + repoHost, err = r.reconcileDedicatedRepoHost(ctx, postgresCluster, repoResources, instances) if err != nil { log.Error(err, "unable to reconcile pgBackRest repo host") result.Requeue = true @@ -1397,7 +1392,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, } if err := r.reconcilePGBackRestSecret(ctx, postgresCluster, repoHost, - rootCA, backupsReconciliationAllowed); err != nil { + rootCA); err != nil { log.Error(err, "unable to reconcile pgBackRest secret") result.Requeue = true } @@ -1428,37 +1423,25 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, // sort to ensure consistent ordering of hosts when creating pgBackRest configs sort.Strings(instanceNames) - - if !backupsSpecFound { - repoHostName = "none" - } else { - repoHostName = repoHost.GetName() - } - + repoHostName = repoHost.GetName() if err := r.reconcilePGBackRestConfig(ctx, postgresCluster, repoHostName, configHash, naming.ClusterPodService(postgresCluster).Name, postgresCluster.GetNamespace(), instanceNames, - backupsReconciliationAllowed); err != nil { + ); err != nil { log.Error(err, "unable to reconcile pgBackRest configuration") result.Requeue = true } // reconcile the RBAC required to run pgBackRest Jobs (e.g. for backups) - sa, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster, backupsReconciliationAllowed) + sa, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster) if err != nil { log.Error(err, "unable to create replica creation backup") result.Requeue = true return result, nil } - // if !backupsSpecFound { - // // Clear the status and exit - // postgresCluster.Status.PGBackRest = &v1beta1.PGBackRestStatus{} - // return result, nil - // } - // reconcile the pgBackRest stanza for all configuration pgBackRest repos - configHashMismatch, err := r.reconcileStanzaCreate(ctx, postgresCluster, instances, configHash, backupsSpecFound, backupsReconciliationAllowed) + configHashMismatch, err := r.reconcileStanzaCreate(ctx, postgresCluster, instances, configHash) // If a stanza create error then requeue but don't return the error. This prevents // stanza-create errors from bubbling up to the main Reconcile() function, which would // prevent subsequent reconciles from occurring. Also, this provides a better chance @@ -1522,7 +1505,7 @@ func (r *Reconciler) reconcilePostgresClusterDataSource(ctx context.Context, cluster *v1beta1.PostgresCluster, dataSource *v1beta1.PostgresClusterDataSource, configHash string, clusterVolumes []corev1.PersistentVolumeClaim, rootCA *pki.RootCertificateAuthority, - backupsSpecFound, backupsReconciliationAllowed bool, + backupsSpecFound bool, ) error { // grab cluster, namespaces and repo name information from the data source @@ -1605,7 +1588,7 @@ func (r *Reconciler) reconcilePostgresClusterDataSource(ctx context.Context, // Note that function reconcilePGBackRest only uses forCluster in observedInstances. result, err := r.reconcilePGBackRest(ctx, cluster, &observedInstances{ forCluster: []*Instance{instance}, - }, rootCA, backupsSpecFound, backupsReconciliationAllowed) + }, rootCA, backupsSpecFound) if err != nil || result != (reconcile.Result{}) { return fmt.Errorf("unable to reconcile pgBackRest as needed to initialize "+ "PostgreSQL data for the cluster: %w", err) @@ -1682,7 +1665,6 @@ func (r *Reconciler) reconcilePostgresClusterDataSource(ctx context.Context, func (r *Reconciler) reconcileCloudBasedDataSource(ctx context.Context, cluster *v1beta1.PostgresCluster, dataSource *v1beta1.PGBackRestDataSource, configHash string, clusterVolumes []corev1.PersistentVolumeClaim, - backupsReconciliationAllowed bool, ) error { // Ensure the proper instance and instance set can be identified via the status. The @@ -1734,7 +1716,7 @@ func (r *Reconciler) reconcileCloudBasedDataSource(ctx context.Context, return nil } - if err := r.createRestoreConfig(ctx, cluster, configHash, backupsReconciliationAllowed); err != nil { + if err := r.createRestoreConfig(ctx, cluster, configHash); err != nil { return err } @@ -1786,8 +1768,9 @@ func (r *Reconciler) reconcileCloudBasedDataSource(ctx context.Context, // createRestoreConfig creates a configmap struct with pgBackRest pgbackrest.conf settings // in the data field, for use with restoring from cloud-based data sources -func (r *Reconciler) createRestoreConfig(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, - configHash string, backupsReconciliationAllowed bool) error { +func (r *Reconciler) createRestoreConfig(ctx context.Context, + postgresCluster *v1beta1.PostgresCluster, + configHash string) error { postgresClusterWithMockedBackups := postgresCluster.DeepCopy() postgresClusterWithMockedBackups.Spec.Backups.PGBackRest.Global = postgresCluster.Spec. @@ -1797,7 +1780,7 @@ func (r *Reconciler) createRestoreConfig(ctx context.Context, postgresCluster *v } return r.reconcilePGBackRestConfig(ctx, postgresClusterWithMockedBackups, - "", configHash, "", "", []string{}, backupsReconciliationAllowed) + "", configHash, "", "", []string{}) } // copyRestoreConfiguration copies pgBackRest configuration from another cluster for use by @@ -2011,16 +1994,8 @@ func (r *Reconciler) copyConfigurationResources(ctx context.Context, cluster, func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, repoHostName, configHash, serviceName, serviceNamespace string, - instanceNames []string, backupsReconciliationAllowed bool, + instanceNames []string, ) error { - - // As a safeguard, if reconciliation is not allowed, exit early. - // (We should never get here because the calling function should exit - // early under this condition.) - if !backupsReconciliationAllowed { - return nil - } - backrestConfig := pgbackrest.CreatePGBackRestConfigMapIntent(postgresCluster, repoHostName, configHash, serviceName, serviceNamespace, instanceNames) if err := controllerutil.SetControllerReference(postgresCluster, backrestConfig, @@ -2041,14 +2016,7 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, cluster *v1beta1.PostgresCluster, repoHost *appsv1.StatefulSet, rootCA *pki.RootCertificateAuthority, - backupsReconciliationAllowed bool, ) error { - // As a safeguard, if reconciliation is not allowed, exit early. - // (We should never get here because the calling function should exit - // early under this condition.) - if !backupsReconciliationAllowed { - return nil - } intent := &corev1.Secret{ObjectMeta: naming.PGBackRestSecret(cluster)} intent.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("Secret")) @@ -2099,31 +2067,12 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, // pgBackRest func (r *Reconciler) reconcilePGBackRestRBAC(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, - backupsReconciliationAllowed bool) (*corev1.ServiceAccount, error) { - +) (*corev1.ServiceAccount, error) { sa := &corev1.ServiceAccount{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} - role := &rbacv1.Role{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} - binding := &rbacv1.RoleBinding{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} - - // As a safeguard, if reconciliation is not allowed, exit early. - // (We should never get here because the calling function should exit - // early under this condition.) - // Because the calling function expects a valid SA to be returned, - // we find the existing SA and return it if possible. - if !backupsReconciliationAllowed { - err := errors.WithStack(client.IgnoreNotFound( - r.Client.Get(ctx, client.ObjectKeyFromObject(sa), sa))) - if err != nil { - return nil, err - } - - return sa, err - } - sa.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ServiceAccount")) - + role := &rbacv1.Role{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} role.SetGroupVersionKind(rbacv1.SchemeGroupVersion.WithKind("Role")) - + binding := &rbacv1.RoleBinding{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} binding.SetGroupVersionKind(rbacv1.SchemeGroupVersion.WithKind("RoleBinding")) if err := r.setControllerReference(postgresCluster, sa); err != nil { @@ -2182,24 +2131,12 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, repoResources *RepoResources, observedInstances *observedInstances, - backupsSpecFound, backupsReconciliationAllowed bool, ) (*appsv1.StatefulSet, error) { log := logging.FromContext(ctx).WithValues("reconcileResource", "repoHost") - // As a safeguard, if reconciliation is not allowed, exit early. - // (We should never get here because the calling function should exit - // early under this condition.) - // FIXME: Do we want to get the existing STS and return it? - if !backupsReconciliationAllowed { - return nil, nil - } - // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { - if !backupsReconciliationAllowed || !backupsSpecFound { - return - } repoHostReady := metav1.Condition{ ObservedGeneration: postgresCluster.GetGeneration(), Type: ConditionRepoHostReady, @@ -2678,13 +2615,12 @@ func (r *Reconciler) reconcileRepos(ctx context.Context, func (r *Reconciler) reconcileStanzaCreate(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, instances *observedInstances, configHash string, - backupsSpecFound, backupsReconciliationAllowed bool, ) (bool, error) { // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { var replicaCreateRepoStatus *v1beta1.RepoStatus - if !backupsReconciliationAllowed || !backupsSpecFound || len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { + if len(postgresCluster.Spec.Backups.PGBackRest.Repos) == 0 { return } replicaCreateRepoName := postgresCluster.Spec.Backups.PGBackRest.Repos[0].Name @@ -3155,7 +3091,7 @@ func (r *Reconciler) ObserveBackupUniverse(ctx context.Context, // If we have an error that is not related to a missing repo-host StatefulSet, // we return an error and expect the calling function to correctly stop processing. if err != nil && !repoHostStatefulSetNotFound { - return false, false, false, err + return true, false, false, err } backupsRemovalAnnotationFound = authorizeBackupRemovalAnnotationPresent(postgresCluster) diff --git a/internal/controller/postgrescluster/pgbackrest_test.go b/internal/controller/postgrescluster/pgbackrest_test.go index c43d5a80e5..5cf331909f 100644 --- a/internal/controller/postgrescluster/pgbackrest_test.go +++ b/internal/controller/postgrescluster/pgbackrest_test.go @@ -197,137 +197,137 @@ func TestReconcilePGBackRest(t *testing.T) { }) t.Cleanup(func() { teardownManager(cancel, t) }) - clusterName := "hippocluster" - clusterUID := "hippouid" + t.Run("run reconcile with backups defined", func(t *testing.T) { + clusterName := "hippocluster" + clusterUID := "hippouid" - ns := setupNamespace(t, tClient) - - // create a PostgresCluster to test with - postgresCluster := fakePostgresCluster(clusterName, ns.GetName(), clusterUID, true) + ns := setupNamespace(t, tClient) + // create a PostgresCluster to test with + postgresCluster := fakePostgresCluster(clusterName, ns.GetName(), clusterUID, true) - // create a service account to test with - serviceAccount, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster, true) - assert.NilError(t, err) - assert.Assert(t, serviceAccount != nil) + // create a service account to test with + serviceAccount, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster) + assert.NilError(t, err) + assert.Assert(t, serviceAccount != nil) - // create the 'observed' instances and set the leader - instances := &observedInstances{ - forCluster: []*Instance{{Name: "instance1", - Pods: []*corev1.Pod{{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{naming.LabelRole: naming.RolePatroniLeader}, - }, - Spec: corev1.PodSpec{}, - }}, - }, {Name: "instance2"}, {Name: "instance3"}}, - } + // create the 'observed' instances and set the leader + instances := &observedInstances{ + forCluster: []*Instance{{Name: "instance1", + Pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{naming.LabelRole: naming.RolePatroniLeader}, + }, + Spec: corev1.PodSpec{}, + }}, + }, {Name: "instance2"}, {Name: "instance3"}}, + } - // set status - postgresCluster.Status = v1beta1.PostgresClusterStatus{ - Patroni: v1beta1.PatroniStatus{SystemIdentifier: "12345abcde"}, - PGBackRest: &v1beta1.PGBackRestStatus{ - RepoHost: &v1beta1.RepoHostStatus{Ready: true}, - Repos: []v1beta1.RepoStatus{{Name: "repo1", StanzaCreated: true}}}, - } + // set status + postgresCluster.Status = v1beta1.PostgresClusterStatus{ + Patroni: v1beta1.PatroniStatus{SystemIdentifier: "12345abcde"}, + PGBackRest: &v1beta1.PGBackRestStatus{ + RepoHost: &v1beta1.RepoHostStatus{Ready: true}, + Repos: []v1beta1.RepoStatus{{Name: "repo1", StanzaCreated: true}}}, + } - // set conditions - clusterConditions := map[string]metav1.ConditionStatus{ - ConditionRepoHostReady: metav1.ConditionTrue, - ConditionReplicaCreate: metav1.ConditionTrue, - } - for condition, status := range clusterConditions { - meta.SetStatusCondition(&postgresCluster.Status.Conditions, metav1.Condition{ - Type: condition, Reason: "testing", Status: status}) - } + // set conditions + clusterConditions := map[string]metav1.ConditionStatus{ + ConditionRepoHostReady: metav1.ConditionTrue, + ConditionReplicaCreate: metav1.ConditionTrue, + } + for condition, status := range clusterConditions { + meta.SetStatusCondition(&postgresCluster.Status.Conditions, metav1.Condition{ + Type: condition, Reason: "testing", Status: status}) + } - rootCA, err := pki.NewRootCertificateAuthority() - assert.NilError(t, err) + rootCA, err := pki.NewRootCertificateAuthority() + assert.NilError(t, err) - result, err := r.reconcilePGBackRest(ctx, postgresCluster, instances, rootCA, true, true) - if err != nil || result != (reconcile.Result{}) { - t.Errorf("unable to reconcile pgBackRest: %v", err) - } + result, err := r.reconcilePGBackRest(ctx, postgresCluster, instances, rootCA, true) + if err != nil || result != (reconcile.Result{}) { + t.Errorf("unable to reconcile pgBackRest: %v", err) + } - // repo is the first defined repo - repo := postgresCluster.Spec.Backups.PGBackRest.Repos[0] + // repo is the first defined repo + repo := postgresCluster.Spec.Backups.PGBackRest.Repos[0] - // test that the repo was created properly - t.Run("verify pgbackrest dedicated repo StatefulSet", func(t *testing.T) { + // test that the repo was created properly + t.Run("verify pgbackrest dedicated repo StatefulSet", func(t *testing.T) { - // get the pgBackRest repo sts using the labels we expect it to have - dedicatedRepos := &appsv1.StatefulSetList{} - if err := tClient.List(ctx, dedicatedRepos, client.InNamespace(ns.Name), - client.MatchingLabels{ - naming.LabelCluster: clusterName, - naming.LabelPGBackRest: "", - naming.LabelPGBackRestDedicated: "", - }); err != nil { - t.Fatal(err) - } + // get the pgBackRest repo sts using the labels we expect it to have + dedicatedRepos := &appsv1.StatefulSetList{} + if err := tClient.List(ctx, dedicatedRepos, client.InNamespace(ns.Name), + client.MatchingLabels{ + naming.LabelCluster: clusterName, + naming.LabelPGBackRest: "", + naming.LabelPGBackRestDedicated: "", + }); err != nil { + t.Fatal(err) + } - repo := appsv1.StatefulSet{} - // verify that we found a repo sts as expected - if len(dedicatedRepos.Items) == 0 { - t.Fatal("Did not find a dedicated repo sts") - } else if len(dedicatedRepos.Items) > 1 { - t.Fatal("Too many dedicated repo sts's found") - } else { - repo = dedicatedRepos.Items[0] - } + repo := appsv1.StatefulSet{} + // verify that we found a repo sts as expected + if len(dedicatedRepos.Items) == 0 { + t.Fatal("Did not find a dedicated repo sts") + } else if len(dedicatedRepos.Items) > 1 { + t.Fatal("Too many dedicated repo sts's found") + } else { + repo = dedicatedRepos.Items[0] + } - // verify proper number of replicas - if *repo.Spec.Replicas != 1 { - t.Errorf("%v replicas found for dedicated repo sts, expected %v", - repo.Spec.Replicas, 1) - } + // verify proper number of replicas + if *repo.Spec.Replicas != 1 { + t.Errorf("%v replicas found for dedicated repo sts, expected %v", + repo.Spec.Replicas, 1) + } - // verify proper ownership - var foundOwnershipRef bool - for _, r := range repo.GetOwnerReferences() { - if r.Kind == "PostgresCluster" && r.Name == clusterName && - r.UID == types.UID(clusterUID) { + // verify proper ownership + var foundOwnershipRef bool + for _, r := range repo.GetOwnerReferences() { + if r.Kind == "PostgresCluster" && r.Name == clusterName && + r.UID == types.UID(clusterUID) { - foundOwnershipRef = true - break + foundOwnershipRef = true + break + } } - } - if !foundOwnershipRef { - t.Errorf("did not find expected ownership references") - } + if !foundOwnershipRef { + t.Errorf("did not find expected ownership references") + } - // verify proper matching labels - expectedLabels := map[string]string{ - naming.LabelCluster: clusterName, - naming.LabelPGBackRest: "", - naming.LabelPGBackRestDedicated: "", - } - expectedLabelsSelector, err := metav1.LabelSelectorAsSelector( - metav1.SetAsLabelSelector(expectedLabels)) - if err != nil { - t.Error(err) - } - if !expectedLabelsSelector.Matches(labels.Set(repo.GetLabels())) { - t.Errorf("dedicated repo host is missing an expected label: found=%v, expected=%v", - repo.GetLabels(), expectedLabels) - } + // verify proper matching labels + expectedLabels := map[string]string{ + naming.LabelCluster: clusterName, + naming.LabelPGBackRest: "", + naming.LabelPGBackRestDedicated: "", + } + expectedLabelsSelector, err := metav1.LabelSelectorAsSelector( + metav1.SetAsLabelSelector(expectedLabels)) + if err != nil { + t.Error(err) + } + if !expectedLabelsSelector.Matches(labels.Set(repo.GetLabels())) { + t.Errorf("dedicated repo host is missing an expected label: found=%v, expected=%v", + repo.GetLabels(), expectedLabels) + } - template := repo.Spec.Template.DeepCopy() + template := repo.Spec.Template.DeepCopy() - // Containers and Volumes should be populated. - assert.Assert(t, len(template.Spec.Containers) != 0) - assert.Assert(t, len(template.Spec.InitContainers) != 0) - assert.Assert(t, len(template.Spec.Volumes) != 0) + // Containers and Volumes should be populated. + assert.Assert(t, len(template.Spec.Containers) != 0) + assert.Assert(t, len(template.Spec.InitContainers) != 0) + assert.Assert(t, len(template.Spec.Volumes) != 0) - // Ignore Containers and Volumes in the comparison below. - template.Spec.Containers = nil - template.Spec.InitContainers = nil - template.Spec.Volumes = nil + // Ignore Containers and Volumes in the comparison below. + template.Spec.Containers = nil + template.Spec.InitContainers = nil + template.Spec.Volumes = nil - // TODO(tjmoore4): Add additional tests to test appending existing - // topology spread constraints and spec.disableDefaultPodScheduling being - // set to true (as done in instance StatefulSet tests). - assert.Assert(t, marshalMatches(template.Spec, ` + // TODO(tjmoore4): Add additional tests to test appending existing + // topology spread constraints and spec.disableDefaultPodScheduling being + // set to true (as done in instance StatefulSet tests). + assert.Assert(t, marshalMatches(template.Spec, ` affinity: {} automountServiceAccountToken: false containers: null @@ -381,224 +381,298 @@ topologySpreadConstraints: maxSkew: 1 topologyKey: topology.kubernetes.io/zone whenUnsatisfiable: ScheduleAnyway - `)) + `)) - // verify that the repohost container exists and contains the proper env vars - var repoHostContExists bool - for _, c := range repo.Spec.Template.Spec.Containers { - if c.Name == naming.PGBackRestRepoContainerName { - repoHostContExists = true + // verify that the repohost container exists and contains the proper env vars + var repoHostContExists bool + for _, c := range repo.Spec.Template.Spec.Containers { + if c.Name == naming.PGBackRestRepoContainerName { + repoHostContExists = true + } + } + // now verify the proper env within the container + if !repoHostContExists { + t.Errorf("dedicated repo host is missing a container with name %s", + naming.PGBackRestRepoContainerName) } - } - // now verify the proper env within the container - if !repoHostContExists { - t.Errorf("dedicated repo host is missing a container with name %s", - naming.PGBackRestRepoContainerName) - } - repoHostStatus := postgresCluster.Status.PGBackRest.RepoHost - if repoHostStatus != nil { - if repoHostStatus.APIVersion != "apps/v1" || repoHostStatus.Kind != "StatefulSet" { - t.Errorf("invalid version/kind for dedicated repo host status") + repoHostStatus := postgresCluster.Status.PGBackRest.RepoHost + if repoHostStatus != nil { + if repoHostStatus.APIVersion != "apps/v1" || repoHostStatus.Kind != "StatefulSet" { + t.Errorf("invalid version/kind for dedicated repo host status") + } + } else { + t.Errorf("dedicated repo host status is missing") } - } else { - t.Errorf("dedicated repo host status is missing") - } - var foundConditionRepoHostsReady bool - for _, c := range postgresCluster.Status.Conditions { - if c.Type == "PGBackRestRepoHostReady" { - foundConditionRepoHostsReady = true - break + var foundConditionRepoHostsReady bool + for _, c := range postgresCluster.Status.Conditions { + if c.Type == "PGBackRestRepoHostReady" { + foundConditionRepoHostsReady = true + break + } + } + if !foundConditionRepoHostsReady { + t.Errorf("status condition PGBackRestRepoHostsReady is missing") } - } - if !foundConditionRepoHostsReady { - t.Errorf("status condition PGBackRestRepoHostsReady is missing") - } - assert.Check(t, wait.PollUntilContextTimeout(ctx, time.Second/2, Scale(time.Second*2), false, - func(ctx context.Context) (bool, error) { - events := &corev1.EventList{} - err := tClient.List(ctx, events, &client.MatchingFields{ - "involvedObject.kind": "PostgresCluster", - "involvedObject.name": clusterName, - "involvedObject.namespace": ns.Name, - "involvedObject.uid": clusterUID, - "reason": "RepoHostCreated", - }) - return len(events.Items) == 1, err - })) - }) + assert.Check(t, wait.PollUntilContextTimeout(ctx, time.Second/2, Scale(time.Second*2), false, + func(ctx context.Context) (bool, error) { + events := &corev1.EventList{} + err := tClient.List(ctx, events, &client.MatchingFields{ + "involvedObject.kind": "PostgresCluster", + "involvedObject.name": clusterName, + "involvedObject.namespace": ns.Name, + "involvedObject.uid": clusterUID, + "reason": "RepoHostCreated", + }) + return len(events.Items) == 1, err + })) + }) - t.Run("verify pgbackrest repo volumes", func(t *testing.T) { + t.Run("verify pgbackrest repo volumes", func(t *testing.T) { + + // get the pgBackRest repo sts using the labels we expect it to have + repoVols := &corev1.PersistentVolumeClaimList{} + if err := tClient.List(ctx, repoVols, client.InNamespace(ns.Name), + client.MatchingLabels{ + naming.LabelCluster: clusterName, + naming.LabelPGBackRest: "", + naming.LabelPGBackRestRepoVolume: "", + }); err != nil { + t.Fatal(err) + } + assert.Assert(t, len(repoVols.Items) > 0) - // get the pgBackRest repo sts using the labels we expect it to have - repoVols := &corev1.PersistentVolumeClaimList{} - if err := tClient.List(ctx, repoVols, client.InNamespace(ns.Name), - client.MatchingLabels{ - naming.LabelCluster: clusterName, - naming.LabelPGBackRest: "", - naming.LabelPGBackRestRepoVolume: "", - }); err != nil { - t.Fatal(err) - } - assert.Assert(t, len(repoVols.Items) > 0) + for _, r := range postgresCluster.Spec.Backups.PGBackRest.Repos { + if r.Volume == nil { + continue + } + var foundRepoVol bool + for _, v := range repoVols.Items { + if v.GetName() == + naming.PGBackRestRepoVolume(postgresCluster, r.Name).Name { + foundRepoVol = true + break + } + } + assert.Assert(t, foundRepoVol) + } + }) - for _, r := range postgresCluster.Spec.Backups.PGBackRest.Repos { - if r.Volume == nil { - continue + t.Run("verify pgbackrest configuration", func(t *testing.T) { + + config := &corev1.ConfigMap{} + if err := tClient.Get(ctx, types.NamespacedName{ + Name: naming.PGBackRestConfig(postgresCluster).Name, + Namespace: postgresCluster.GetNamespace(), + }, config); err != nil { + assert.NilError(t, err) } - var foundRepoVol bool - for _, v := range repoVols.Items { - if v.GetName() == - naming.PGBackRestRepoVolume(postgresCluster, r.Name).Name { - foundRepoVol = true - break + assert.Assert(t, len(config.Data) > 0) + + var instanceConfFound, dedicatedRepoConfFound bool + for k, v := range config.Data { + if v != "" { + if k == pgbackrest.CMInstanceKey { + instanceConfFound = true + } else if k == pgbackrest.CMRepoKey { + dedicatedRepoConfFound = true + } } } - assert.Assert(t, foundRepoVol) - } - }) + assert.Check(t, instanceConfFound) + assert.Check(t, dedicatedRepoConfFound) + }) - t.Run("verify pgbackrest configuration", func(t *testing.T) { + t.Run("verify pgbackrest schedule cronjob", func(t *testing.T) { - config := &corev1.ConfigMap{} - if err := tClient.Get(ctx, types.NamespacedName{ - Name: naming.PGBackRestConfig(postgresCluster).Name, - Namespace: postgresCluster.GetNamespace(), - }, config); err != nil { - assert.NilError(t, err) - } - assert.Assert(t, len(config.Data) > 0) - - var instanceConfFound, dedicatedRepoConfFound bool - for k, v := range config.Data { - if v != "" { - if k == pgbackrest.CMInstanceKey { - instanceConfFound = true - } else if k == pgbackrest.CMRepoKey { - dedicatedRepoConfFound = true - } + // set status + postgresCluster.Status = v1beta1.PostgresClusterStatus{ + Patroni: v1beta1.PatroniStatus{SystemIdentifier: "12345abcde"}, + PGBackRest: &v1beta1.PGBackRestStatus{ + Repos: []v1beta1.RepoStatus{{Name: "repo1", StanzaCreated: true}}}, } - } - assert.Check(t, instanceConfFound) - assert.Check(t, dedicatedRepoConfFound) - }) - t.Run("verify pgbackrest schedule cronjob", func(t *testing.T) { + // set conditions + clusterConditions := map[string]metav1.ConditionStatus{ + ConditionRepoHostReady: metav1.ConditionTrue, + ConditionReplicaCreate: metav1.ConditionTrue, + } - // set status - postgresCluster.Status = v1beta1.PostgresClusterStatus{ - Patroni: v1beta1.PatroniStatus{SystemIdentifier: "12345abcde"}, - PGBackRest: &v1beta1.PGBackRestStatus{ - Repos: []v1beta1.RepoStatus{{Name: "repo1", StanzaCreated: true}}}, - } + for condition, status := range clusterConditions { + meta.SetStatusCondition(&postgresCluster.Status.Conditions, metav1.Condition{ + Type: condition, Reason: "testing", Status: status}) + } - // set conditions - clusterConditions := map[string]metav1.ConditionStatus{ - ConditionRepoHostReady: metav1.ConditionTrue, - ConditionReplicaCreate: metav1.ConditionTrue, - } + requeue := r.reconcileScheduledBackups(ctx, postgresCluster, serviceAccount, fakeObservedCronJobs()) + assert.Assert(t, !requeue) - for condition, status := range clusterConditions { - meta.SetStatusCondition(&postgresCluster.Status.Conditions, metav1.Condition{ - Type: condition, Reason: "testing", Status: status}) - } + returnedCronJob := &batchv1.CronJob{} + if err := tClient.Get(ctx, types.NamespacedName{ + Name: postgresCluster.Name + "-repo1-full", + Namespace: postgresCluster.GetNamespace(), + }, returnedCronJob); err != nil { + assert.NilError(t, err) + } - requeue := r.reconcileScheduledBackups(ctx, postgresCluster, serviceAccount, fakeObservedCronJobs()) - assert.Assert(t, !requeue) + // check returned cronjob matches set spec + assert.Equal(t, returnedCronJob.Name, "hippocluster-repo1-full") + assert.Equal(t, returnedCronJob.Spec.Schedule, testCronSchedule) + assert.Equal(t, returnedCronJob.Spec.ConcurrencyPolicy, batchv1.ForbidConcurrent) + assert.Equal(t, returnedCronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Name, + "pgbackrest") + assert.Assert(t, returnedCronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].SecurityContext != &corev1.SecurityContext{}) - returnedCronJob := &batchv1.CronJob{} - if err := tClient.Get(ctx, types.NamespacedName{ - Name: postgresCluster.Name + "-repo1-full", - Namespace: postgresCluster.GetNamespace(), - }, returnedCronJob); err != nil { - assert.NilError(t, err) - } + }) - // check returned cronjob matches set spec - assert.Equal(t, returnedCronJob.Name, "hippocluster-repo1-full") - assert.Equal(t, returnedCronJob.Spec.Schedule, testCronSchedule) - assert.Equal(t, returnedCronJob.Spec.ConcurrencyPolicy, batchv1.ForbidConcurrent) - assert.Equal(t, returnedCronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Name, - "pgbackrest") - assert.Assert(t, returnedCronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].SecurityContext != &corev1.SecurityContext{}) + t.Run("verify pgbackrest schedule found", func(t *testing.T) { - }) + assert.Assert(t, backupScheduleFound(repo, "full")) - t.Run("verify pgbackrest schedule found", func(t *testing.T) { + testrepo := v1beta1.PGBackRestRepo{ + Name: "repo1", + BackupSchedules: &v1beta1.PGBackRestBackupSchedules{ + Full: &testCronSchedule, + Differential: &testCronSchedule, + Incremental: &testCronSchedule, + }} - assert.Assert(t, backupScheduleFound(repo, "full")) + assert.Assert(t, backupScheduleFound(testrepo, "full")) + assert.Assert(t, backupScheduleFound(testrepo, "diff")) + assert.Assert(t, backupScheduleFound(testrepo, "incr")) - testrepo := v1beta1.PGBackRestRepo{ - Name: "repo1", - BackupSchedules: &v1beta1.PGBackRestBackupSchedules{ - Full: &testCronSchedule, - Differential: &testCronSchedule, - Incremental: &testCronSchedule, - }} + }) - assert.Assert(t, backupScheduleFound(testrepo, "full")) - assert.Assert(t, backupScheduleFound(testrepo, "diff")) - assert.Assert(t, backupScheduleFound(testrepo, "incr")) + t.Run("verify pgbackrest schedule not found", func(t *testing.T) { - }) + assert.Assert(t, !backupScheduleFound(repo, "notabackuptype")) + + noscheduletestrepo := v1beta1.PGBackRestRepo{Name: "repo1"} + assert.Assert(t, !backupScheduleFound(noscheduletestrepo, "full")) + + }) + + t.Run("pgbackrest schedule suspended status", func(t *testing.T) { + + returnedCronJob := &batchv1.CronJob{} + if err := tClient.Get(ctx, types.NamespacedName{ + Name: postgresCluster.Name + "-repo1-full", + Namespace: postgresCluster.GetNamespace(), + }, returnedCronJob); err != nil { + assert.NilError(t, err) + } + + t.Run("pgbackrest schedule suspended false", func(t *testing.T) { + assert.Assert(t, !*returnedCronJob.Spec.Suspend) + }) + + t.Run("shutdown", func(t *testing.T) { + *postgresCluster.Spec.Shutdown = true + postgresCluster.Spec.Standby = nil + + requeue := r.reconcileScheduledBackups(ctx, + postgresCluster, serviceAccount, fakeObservedCronJobs()) + assert.Assert(t, !requeue) - t.Run("verify pgbackrest schedule not found", func(t *testing.T) { + assert.NilError(t, tClient.Get(ctx, types.NamespacedName{ + Name: postgresCluster.Name + "-repo1-full", + Namespace: postgresCluster.GetNamespace(), + }, returnedCronJob)) + + assert.Assert(t, *returnedCronJob.Spec.Suspend) + }) + + t.Run("standby", func(t *testing.T) { + *postgresCluster.Spec.Shutdown = false + postgresCluster.Spec.Standby = &v1beta1.PostgresStandbySpec{ + Enabled: true, + } - assert.Assert(t, !backupScheduleFound(repo, "notabackuptype")) + requeue := r.reconcileScheduledBackups(ctx, + postgresCluster, serviceAccount, fakeObservedCronJobs()) + assert.Assert(t, !requeue) - noscheduletestrepo := v1beta1.PGBackRestRepo{Name: "repo1"} - assert.Assert(t, !backupScheduleFound(noscheduletestrepo, "full")) + assert.NilError(t, tClient.Get(ctx, types.NamespacedName{ + Name: postgresCluster.Name + "-repo1-full", + Namespace: postgresCluster.GetNamespace(), + }, returnedCronJob)) + assert.Assert(t, *returnedCronJob.Spec.Suspend) + }) + }) }) - t.Run("pgbackrest schedule suspended status", func(t *testing.T) { + t.Run("run reconcile with backups not defined", func(t *testing.T) { + clusterName := "hippocluster2" + clusterUID := "hippouid2" - returnedCronJob := &batchv1.CronJob{} - if err := tClient.Get(ctx, types.NamespacedName{ - Name: postgresCluster.Name + "-repo1-full", - Namespace: postgresCluster.GetNamespace(), - }, returnedCronJob); err != nil { - assert.NilError(t, err) + ns := setupNamespace(t, tClient) + // create a PostgresCluster without backups to test with + postgresCluster := fakePostgresCluster(clusterName, ns.GetName(), clusterUID, true) + postgresCluster.Spec.Backups = v1beta1.Backups{} + + // create the 'observed' instances and set the leader + instances := &observedInstances{ + forCluster: []*Instance{{Name: "instance1", + Pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{naming.LabelRole: naming.RolePatroniLeader}, + }, + Spec: corev1.PodSpec{}, + }}, + }, {Name: "instance2"}, {Name: "instance3"}}, } - t.Run("pgbackrest schedule suspended false", func(t *testing.T) { - assert.Assert(t, !*returnedCronJob.Spec.Suspend) - }) + rootCA, err := pki.NewRootCertificateAuthority() + assert.NilError(t, err) - t.Run("shutdown", func(t *testing.T) { - *postgresCluster.Spec.Shutdown = true - postgresCluster.Spec.Standby = nil + result, err := r.reconcilePGBackRest(ctx, postgresCluster, instances, rootCA, false) + if err != nil { + t.Errorf("unable to reconcile pgBackRest: %v", err) + } + assert.Equal(t, result, reconcile.Result{}) - requeue := r.reconcileScheduledBackups(ctx, - postgresCluster, serviceAccount, fakeObservedCronJobs()) - assert.Assert(t, !requeue) + t.Run("verify pgbackrest dedicated repo StatefulSet", func(t *testing.T) { - assert.NilError(t, tClient.Get(ctx, types.NamespacedName{ - Name: postgresCluster.Name + "-repo1-full", - Namespace: postgresCluster.GetNamespace(), - }, returnedCronJob)) + // Verify the sts doesn't exist + dedicatedRepos := &appsv1.StatefulSetList{} + if err := tClient.List(ctx, dedicatedRepos, client.InNamespace(ns.Name), + client.MatchingLabels{ + naming.LabelCluster: clusterName, + naming.LabelPGBackRest: "", + naming.LabelPGBackRestDedicated: "", + }); err != nil { + t.Fatal(err) + } - assert.Assert(t, *returnedCronJob.Spec.Suspend) + assert.Equal(t, len(dedicatedRepos.Items), 0) }) - t.Run("standby", func(t *testing.T) { - *postgresCluster.Spec.Shutdown = false - postgresCluster.Spec.Standby = &v1beta1.PostgresStandbySpec{ - Enabled: true, + t.Run("verify pgbackrest repo volumes", func(t *testing.T) { + + // get the pgBackRest repo sts using the labels we expect it to have + repoVols := &corev1.PersistentVolumeClaimList{} + if err := tClient.List(ctx, repoVols, client.InNamespace(ns.Name), + client.MatchingLabels{ + naming.LabelCluster: clusterName, + naming.LabelPGBackRest: "", + naming.LabelPGBackRestRepoVolume: "", + }); err != nil { + t.Fatal(err) } - requeue := r.reconcileScheduledBackups(ctx, - postgresCluster, serviceAccount, fakeObservedCronJobs()) - assert.Assert(t, !requeue) + assert.Equal(t, len(repoVols.Items), 0) + }) - assert.NilError(t, tClient.Get(ctx, types.NamespacedName{ - Name: postgresCluster.Name + "-repo1-full", - Namespace: postgresCluster.GetNamespace(), - }, returnedCronJob)) + t.Run("verify pgbackrest configuration", func(t *testing.T) { - assert.Assert(t, *returnedCronJob.Spec.Suspend) + config := &corev1.ConfigMap{} + err := tClient.Get(ctx, types.NamespacedName{ + Name: naming.PGBackRestConfig(postgresCluster).Name, + Namespace: postgresCluster.GetNamespace(), + }, config) + assert.Equal(t, apierrors.IsNotFound(err), true) }) }) } @@ -626,7 +700,7 @@ func TestReconcilePGBackRestRBAC(t *testing.T) { Repos: []v1beta1.RepoStatus{{Name: "repo1", StanzaCreated: false}}, } - serviceAccount, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster, true) + serviceAccount, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster) assert.NilError(t, err) assert.Assert(t, serviceAccount != nil) @@ -720,7 +794,7 @@ func TestReconcileStanzaCreate(t *testing.T) { Message: "pgBackRest dedicated repository host is ready", }) - configHashMismatch, err := r.reconcileStanzaCreate(ctx, postgresCluster, instances, "abcde12345", true, true) + configHashMismatch, err := r.reconcileStanzaCreate(ctx, postgresCluster, instances, "abcde12345") assert.NilError(t, err) assert.Assert(t, !configHashMismatch) @@ -759,7 +833,7 @@ func TestReconcileStanzaCreate(t *testing.T) { SystemIdentifier: "6952526174828511264", } - configHashMismatch, err = r.reconcileStanzaCreate(ctx, postgresCluster, instances, "abcde12345", true, true) + configHashMismatch, err = r.reconcileStanzaCreate(ctx, postgresCluster, instances, "abcde12345") assert.Error(t, err, "fake stanza create failed: ") assert.Assert(t, !configHashMismatch) @@ -1641,7 +1715,7 @@ func TestGetPGBackRestResources(t *testing.T) { assert.NilError(t, err) assert.NilError(t, tClient.Create(ctx, resource)) - resources, err := r.getPGBackRestResources(ctx, tc.cluster, true, true) + resources, err := r.getPGBackRestResources(ctx, tc.cluster, true) assert.NilError(t, err) assert.Assert(t, tc.result.jobCount == len(resources.replicaCreateBackupJobs)) @@ -1878,7 +1952,7 @@ func TestReconcilePostgresClusterDataSource(t *testing.T) { pgclusterDataSource = tc.dataSource.PostgresCluster } err := r.reconcilePostgresClusterDataSource(ctx, cluster, pgclusterDataSource, - "testhash", nil, rootCA, true, true) + "testhash", nil, rootCA, true) assert.NilError(t, err) restoreConfig := &corev1.ConfigMap{} @@ -2072,7 +2146,6 @@ func TestReconcileCloudBasedDataSource(t *testing.T) { pgclusterDataSource, "testhash", nil, - true, ) assert.NilError(t, err) @@ -3672,3 +3745,167 @@ func TestSetScheduledJobStatus(t *testing.T) { assert.Assert(t, len(postgresCluster.Status.PGBackRest.ScheduledBackups) == 0) }) } + +func TestBackupsEnabled(t *testing.T) { + // Garbage collector cleans up test resources before the test completes + if strings.EqualFold(os.Getenv("USE_EXISTING_CLUSTER"), "true") { + t.Skip("USE_EXISTING_CLUSTER: Test fails due to garbage collection") + } + + cfg, tClient := setupKubernetes(t) + require.ParallelCapacity(t, 2) + + r := &Reconciler{} + ctx, cancel := setupManager(t, cfg, func(mgr manager.Manager) { + r = &Reconciler{ + Client: mgr.GetClient(), + Recorder: mgr.GetEventRecorderFor(ControllerName), + Tracer: otel.Tracer(ControllerName), + Owner: ControllerName, + } + }) + t.Cleanup(func() { teardownManager(cancel, t) }) + + t.Run("Cluster with backups, no sts can be reconciled", func(t *testing.T) { + clusterName := "hippocluster1" + clusterUID := "hippouid1" + + ns := setupNamespace(t, tClient) + + // create a PostgresCluster to test with + postgresCluster := fakePostgresCluster(clusterName, ns.GetName(), clusterUID, true) + + backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) + + assert.NilError(t, err) + assert.Assert(t, backupsSpecFound) + assert.Assert(t, backupsReconciliationAllowed) + }) + + t.Run("Cluster with backups, sts can be reconciled", func(t *testing.T) { + clusterName := "hippocluster2" + clusterUID := "hippouid2" + + ns := setupNamespace(t, tClient) + + // create a PostgresCluster to test with + postgresCluster := fakePostgresCluster(clusterName, ns.GetName(), clusterUID, true) + + // create the 'observed' instances and set the leader + instances := &observedInstances{ + forCluster: []*Instance{{Name: "instance1", + Pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{naming.LabelRole: naming.RolePatroniLeader}, + }, + Spec: corev1.PodSpec{}, + }}, + }, {Name: "instance2"}, {Name: "instance3"}}, + } + + rootCA, err := pki.NewRootCertificateAuthority() + assert.NilError(t, err) + + _, err = r.reconcilePGBackRest(ctx, postgresCluster, instances, rootCA, true) + assert.NilError(t, err) + + backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) + + assert.NilError(t, err) + assert.Assert(t, backupsSpecFound) + assert.Assert(t, backupsReconciliationAllowed) + }) + + t.Run("Cluster with no backups, no sts can reconcile", func(t *testing.T) { + // create a PostgresCluster to test with + clusterName := "hippocluster3" + clusterUID := "hippouid3" + + ns := setupNamespace(t, tClient) + + postgresCluster := fakePostgresCluster(clusterName, ns.GetName(), clusterUID, true) + postgresCluster.Spec.Backups = v1beta1.Backups{} + + backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) + + assert.NilError(t, err) + assert.Assert(t, !backupsSpecFound) + assert.Assert(t, backupsReconciliationAllowed) + }) + + t.Run("Cluster with no backups, sts cannot be reconciled", func(t *testing.T) { + clusterName := "hippocluster4" + clusterUID := "hippouid4" + + ns := setupNamespace(t, tClient) + + // create a PostgresCluster to test with + postgresCluster := fakePostgresCluster(clusterName, ns.GetName(), clusterUID, true) + + // create the 'observed' instances and set the leader + instances := &observedInstances{ + forCluster: []*Instance{{Name: "instance1", + Pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{naming.LabelRole: naming.RolePatroniLeader}, + }, + Spec: corev1.PodSpec{}, + }}, + }, {Name: "instance2"}, {Name: "instance3"}}, + } + + rootCA, err := pki.NewRootCertificateAuthority() + assert.NilError(t, err) + + _, err = r.reconcilePGBackRest(ctx, postgresCluster, instances, rootCA, true) + assert.NilError(t, err) + + postgresCluster.Spec.Backups = v1beta1.Backups{} + + backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) + + assert.NilError(t, err) + assert.Assert(t, !backupsSpecFound) + assert.Assert(t, !backupsReconciliationAllowed) + }) + + t.Run("Cluster with no backups, sts, annotation can be reconciled", func(t *testing.T) { + clusterName := "hippocluster5" + clusterUID := "hippouid5" + + ns := setupNamespace(t, tClient) + + // create a PostgresCluster to test with + postgresCluster := fakePostgresCluster(clusterName, ns.GetName(), clusterUID, true) + + // create the 'observed' instances and set the leader + instances := &observedInstances{ + forCluster: []*Instance{{Name: "instance1", + Pods: []*corev1.Pod{{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{naming.LabelRole: naming.RolePatroniLeader}, + }, + Spec: corev1.PodSpec{}, + }}, + }, {Name: "instance2"}, {Name: "instance3"}}, + } + + rootCA, err := pki.NewRootCertificateAuthority() + assert.NilError(t, err) + + _, err = r.reconcilePGBackRest(ctx, postgresCluster, instances, rootCA, true) + assert.NilError(t, err) + + postgresCluster.Spec.Backups = v1beta1.Backups{} + annotations := map[string]string{ + naming.AuthorizeBackupRemovalAnnotation: "true", + } + postgresCluster.Annotations = annotations + + backupsSpecFound, backupsReconciliationAllowed, err := r.BackupsEnabled(ctx, postgresCluster) + + assert.NilError(t, err) + assert.Assert(t, !backupsSpecFound) + assert.Assert(t, backupsReconciliationAllowed) + }) +} From ed7da1b59fc616a518e19bd606a96854438bf6e2 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 22 Aug 2024 15:29:05 -0500 Subject: [PATCH 09/13] cleanup --- .../controller/postgrescluster/controller.go | 1 + .../controller/postgrescluster/pgbackrest.go | 34 ++++++------------- internal/pgbackrest/postgres.go | 1 - .../kuttl/e2e/optional-backups/05-assert.yaml | 2 +- .../kuttl/e2e/optional-backups/10-assert.yaml | 2 +- .../kuttl/e2e/optional-backups/23-assert.yaml | 1 - 6 files changed, 13 insertions(+), 28 deletions(-) diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index 07eed90f79..c038d36e68 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/internal/controller/postgrescluster/controller.go @@ -240,6 +240,7 @@ func (r *Reconciler) Reconcile( pgHBAs := postgres.NewHBAs() pgmonitor.PostgreSQLHBAs(cluster, &pgHBAs) pgbouncer.PostgreSQL(cluster, &pgHBAs) + pgParameters := postgres.NewParameters() pgaudit.PostgreSQLParameters(&pgParameters) pgbackrest.PostgreSQL(cluster, &pgParameters, backupsSpecFound) diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index 4927de2a53..3878185c11 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -1356,7 +1356,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, // add some additional context about what component is being reconciled log := logging.FromContext(ctx).WithValues("reconciler", "pgBackRest") - // if nil and backups are enabled, create the pgBackRest status that will be updated when + // if nil, create the pgBackRest status that will be updated when // reconciling various pgBackRest resources if postgresCluster.Status.PGBackRest == nil { postgresCluster.Status.PGBackRest = &v1beta1.PGBackRestStatus{} @@ -1391,8 +1391,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, return result, nil } - if err := r.reconcilePGBackRestSecret(ctx, postgresCluster, repoHost, - rootCA); err != nil { + if err := r.reconcilePGBackRestSecret(ctx, postgresCluster, repoHost, rootCA); err != nil { log.Error(err, "unable to reconcile pgBackRest secret") result.Requeue = true } @@ -1426,8 +1425,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, repoHostName = repoHost.GetName() if err := r.reconcilePGBackRestConfig(ctx, postgresCluster, repoHostName, configHash, naming.ClusterPodService(postgresCluster).Name, - postgresCluster.GetNamespace(), instanceNames, - ); err != nil { + postgresCluster.GetNamespace(), instanceNames); err != nil { log.Error(err, "unable to reconcile pgBackRest configuration") result.Requeue = true } @@ -1664,8 +1662,7 @@ func (r *Reconciler) reconcilePostgresClusterDataSource(ctx context.Context, // data source, i.e., S3, etc. func (r *Reconciler) reconcileCloudBasedDataSource(ctx context.Context, cluster *v1beta1.PostgresCluster, dataSource *v1beta1.PGBackRestDataSource, - configHash string, clusterVolumes []corev1.PersistentVolumeClaim, -) error { + configHash string, clusterVolumes []corev1.PersistentVolumeClaim) error { // Ensure the proper instance and instance set can be identified via the status. The // StartupInstance and StartupInstanceSet values should be populated when the cluster @@ -1768,8 +1765,7 @@ func (r *Reconciler) reconcileCloudBasedDataSource(ctx context.Context, // createRestoreConfig creates a configmap struct with pgBackRest pgbackrest.conf settings // in the data field, for use with restoring from cloud-based data sources -func (r *Reconciler) createRestoreConfig(ctx context.Context, - postgresCluster *v1beta1.PostgresCluster, +func (r *Reconciler) createRestoreConfig(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, configHash string) error { postgresClusterWithMockedBackups := postgresCluster.DeepCopy() @@ -1994,8 +1990,7 @@ func (r *Reconciler) copyConfigurationResources(ctx context.Context, cluster, func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, repoHostName, configHash, serviceName, serviceNamespace string, - instanceNames []string, -) error { + instanceNames []string) error { backrestConfig := pgbackrest.CreatePGBackRestConfigMapIntent(postgresCluster, repoHostName, configHash, serviceName, serviceNamespace, instanceNames) if err := controllerutil.SetControllerReference(postgresCluster, backrestConfig, @@ -2015,8 +2010,7 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, // reconcilePGBackRestSecret reconciles the pgBackRest Secret. func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, cluster *v1beta1.PostgresCluster, repoHost *appsv1.StatefulSet, - rootCA *pki.RootCertificateAuthority, -) error { + rootCA *pki.RootCertificateAuthority) error { intent := &corev1.Secret{ObjectMeta: naming.PGBackRestSecret(cluster)} intent.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("Secret")) @@ -2035,10 +2029,6 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, err := errors.WithStack(client.IgnoreNotFound( r.Client.Get(ctx, client.ObjectKeyFromObject(intent), existing))) - if err != nil { - return err - } - if err == nil { err = r.setControllerReference(cluster, intent) } @@ -2066,8 +2056,7 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, // reconcileInstanceRBAC reconciles the Role, RoleBinding, and ServiceAccount for // pgBackRest func (r *Reconciler) reconcilePGBackRestRBAC(ctx context.Context, - postgresCluster *v1beta1.PostgresCluster, -) (*corev1.ServiceAccount, error) { + postgresCluster *v1beta1.PostgresCluster) (*corev1.ServiceAccount, error) { sa := &corev1.ServiceAccount{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} sa.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ServiceAccount")) role := &rbacv1.Role{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} @@ -2130,8 +2119,7 @@ func (r *Reconciler) reconcilePGBackRestRBAC(ctx context.Context, func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, repoResources *RepoResources, - observedInstances *observedInstances, -) (*appsv1.StatefulSet, error) { + observedInstances *observedInstances) (*appsv1.StatefulSet, error) { log := logging.FromContext(ctx).WithValues("reconcileResource", "repoHost") @@ -2178,7 +2166,6 @@ func (r *Reconciler) reconcileDedicatedRepoHost(ctx context.Context, return nil, err } - // TODO: Why do we hit this line when backups should not be enabled? postgresCluster.Status.PGBackRest.RepoHost = getRepoHostStatus(repoHost) if isCreate { @@ -2614,8 +2601,7 @@ func (r *Reconciler) reconcileRepos(ctx context.Context, // propagated to the Pod). func (r *Reconciler) reconcileStanzaCreate(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, - instances *observedInstances, configHash string, -) (bool, error) { + instances *observedInstances, configHash string) (bool, error) { // ensure conditions are set before returning as needed by subsequent reconcile functions defer func() { diff --git a/internal/pgbackrest/postgres.go b/internal/pgbackrest/postgres.go index a1dce03b90..566630657b 100644 --- a/internal/pgbackrest/postgres.go +++ b/internal/pgbackrest/postgres.go @@ -44,7 +44,6 @@ func PostgreSQL( archive := `pgbackrest --stanza=` + DefaultStanzaName + ` archive-push "%p"` outParameters.Mandatory.Add("archive_command", archive) } else { - //TODO: outParameters.Mandatory.Add("archive_check", "n") # Not needed? // If backups are disabled, keep archive_mode on (to avoid a Postgres restart) // and throw away WAL. outParameters.Mandatory.Add("archive_command", `true`) diff --git a/testing/kuttl/e2e/optional-backups/05-assert.yaml b/testing/kuttl/e2e/optional-backups/05-assert.yaml index 08cd1f30fe..d346e01a04 100644 --- a/testing/kuttl/e2e/optional-backups/05-assert.yaml +++ b/testing/kuttl/e2e/optional-backups/05-assert.yaml @@ -9,4 +9,4 @@ metadata: status: containerStatuses: - ready: true - - ready: true \ No newline at end of file + - ready: true diff --git a/testing/kuttl/e2e/optional-backups/10-assert.yaml b/testing/kuttl/e2e/optional-backups/10-assert.yaml index b145c1d558..7b740b310d 100644 --- a/testing/kuttl/e2e/optional-backups/10-assert.yaml +++ b/testing/kuttl/e2e/optional-backups/10-assert.yaml @@ -76,4 +76,4 @@ status: - ready: true - ready: true - ready: true - - ready: true \ No newline at end of file + - ready: true diff --git a/testing/kuttl/e2e/optional-backups/23-assert.yaml b/testing/kuttl/e2e/optional-backups/23-assert.yaml index 27a3ef7fca..8748ea015c 100644 --- a/testing/kuttl/e2e/optional-backups/23-assert.yaml +++ b/testing/kuttl/e2e/optional-backups/23-assert.yaml @@ -24,4 +24,3 @@ metadata: postgres-operator.crunchydata.com/cluster: created-without-backups postgres-operator.crunchydata.com/data: postgres postgres-operator.crunchydata.com/instance-set: instance1 ---- \ No newline at end of file From f1a69a569bbb8db346db63fdbff28d6fa09b27d0 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 22 Aug 2024 16:30:24 -0500 Subject: [PATCH 10/13] fix lint error --- .../v1beta1/postgrescluster_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go index 9477f3add2..0e50f3f0f7 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go @@ -34,7 +34,7 @@ type PostgresClusterSpec struct { // PostgreSQL backup configuration // +optional - Backups Backups `json:"backups, omitempty"` + Backups Backups `json:"backups,omitempty"` // The secret containing the Certificates and Keys to encrypt PostgreSQL // traffic will need to contain the server TLS certificate, TLS key and the From 7ed79e5423a31290531908fe1e8cfcdbf0564fd9 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 22 Aug 2024 20:07:19 -0500 Subject: [PATCH 11/13] ignore G115: int overrun --- internal/controller/postgrescluster/instance.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index fb043c3013..50f4beb266 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -346,7 +346,7 @@ func (r *Reconciler) observeInstances( status.DesiredPGDataVolume = make(map[string]string) for _, instance := range observed.bySet[name] { - status.Replicas += int32(len(instance.Pods)) + status.Replicas += int32(len(instance.Pods)) //nolint:gosec if ready, known := instance.IsReady(); known && ready { status.ReadyReplicas++ From 7a58f0598cbbfbb99266f0913d07988c51d1b3af Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Mon, 26 Aug 2024 14:18:15 -0500 Subject: [PATCH 12/13] Revert some minor space changes to help review --- internal/controller/postgrescluster/instance.go | 2 +- internal/controller/postgrescluster/pgbackrest.go | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/internal/controller/postgrescluster/instance.go b/internal/controller/postgrescluster/instance.go index 50f4beb266..fceeee9d6d 100644 --- a/internal/controller/postgrescluster/instance.go +++ b/internal/controller/postgrescluster/instance.go @@ -1209,7 +1209,7 @@ func (r *Reconciler) reconcileInstance( ctx, cluster, instanceCertificates, &instance.Spec.Template.Spec) } - _ = patroni.InstancePod( + err = patroni.InstancePod( ctx, cluster, clusterConfigMap, clusterPodService, patroniLeaderService, spec, instanceCertificates, instanceConfigMap, &instance.Spec.Template) } diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index 3878185c11..db64590a0b 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -1390,6 +1390,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, result.Requeue = true return result, nil } + repoHostName = repoHost.GetName() if err := r.reconcilePGBackRestSecret(ctx, postgresCluster, repoHost, rootCA); err != nil { log.Error(err, "unable to reconcile pgBackRest secret") @@ -1419,10 +1420,8 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context, for _, instance := range instances.forCluster { instanceNames = append(instanceNames, instance.Name) } - // sort to ensure consistent ordering of hosts when creating pgBackRest configs sort.Strings(instanceNames) - repoHostName = repoHost.GetName() if err := r.reconcilePGBackRestConfig(ctx, postgresCluster, repoHostName, configHash, naming.ClusterPodService(postgresCluster).Name, postgresCluster.GetNamespace(), instanceNames); err != nil { @@ -1991,6 +1990,7 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, repoHostName, configHash, serviceName, serviceNamespace string, instanceNames []string) error { + backrestConfig := pgbackrest.CreatePGBackRestConfigMapIntent(postgresCluster, repoHostName, configHash, serviceName, serviceNamespace, instanceNames) if err := controllerutil.SetControllerReference(postgresCluster, backrestConfig, @@ -2057,10 +2057,13 @@ func (r *Reconciler) reconcilePGBackRestSecret(ctx context.Context, // pgBackRest func (r *Reconciler) reconcilePGBackRestRBAC(ctx context.Context, postgresCluster *v1beta1.PostgresCluster) (*corev1.ServiceAccount, error) { + sa := &corev1.ServiceAccount{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} sa.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ServiceAccount")) + role := &rbacv1.Role{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} role.SetGroupVersionKind(rbacv1.SchemeGroupVersion.WithKind("Role")) + binding := &rbacv1.RoleBinding{ObjectMeta: naming.PGBackRestRBAC(postgresCluster)} binding.SetGroupVersionKind(rbacv1.SchemeGroupVersion.WithKind("RoleBinding")) @@ -3009,7 +3012,7 @@ func (r *Reconciler) reconcilePGBackRestCronJob( } // BackupsEnabled checks the state of the backups (i.e., if backups are in the spec, -// if a repo-host StatefulSet exists, if the annotation permitting backup deletion exist) +// if a repo-host StatefulSet exists, if the annotation permitting backup deletion exists) // and determines whether reconciliation is allowed. // Reconciliation of backup-related Kubernetes objects is paused if // - a user created a cluster with backups; @@ -3049,7 +3052,7 @@ func (r *Reconciler) BackupsEnabled( } // ObserveBackupUniverse returns -// - wheterh the spec has backups defined; +// - whether the spec has backups defined; func (r *Reconciler) ObserveBackupUniverse(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, ) ( From 2e2ff6e99a7f5dc7a5f969f0b69f02555f6054a5 Mon Sep 17 00:00:00 2001 From: Ben Blattberg Date: Thu, 29 Aug 2024 09:41:50 -0500 Subject: [PATCH 13/13] Correct comment --- internal/controller/postgrescluster/pgbackrest.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/controller/postgrescluster/pgbackrest.go b/internal/controller/postgrescluster/pgbackrest.go index db64590a0b..34414fe2cd 100644 --- a/internal/controller/postgrescluster/pgbackrest.go +++ b/internal/controller/postgrescluster/pgbackrest.go @@ -3053,6 +3053,8 @@ func (r *Reconciler) BackupsEnabled( // ObserveBackupUniverse returns // - whether the spec has backups defined; +// - whether the repo-host statefulset exists; +// - whether the cluster has the annotation authorizing backup removal. func (r *Reconciler) ObserveBackupUniverse(ctx context.Context, postgresCluster *v1beta1.PostgresCluster, ) (