From 361f83eea1422ccbf49d455bcf7833eed8301cbd Mon Sep 17 00:00:00 2001 From: Martin Hansen Date: Thu, 24 Sep 2026 14:04:08 +0200 Subject: [PATCH 1/3] Initialize new backup repositories before use A bucket or repository path change can retain the stanza name. The manager then mistakes initialization of the old repository for readiness of the new one. Backups only poll for stanza metadata and can time out before WAL archiving triggers recovery. Initialization must follow configuration changes and remain pending after failure, even when the next reconcile finds the file unchanged. Replicas and suspended clusters must defer initialization until they become eligible. --- .../controller/instance_controller.go | 42 +++--- internal/management/controller/manager.go | 6 +- .../controller/pgbackrest_stanza_test.go | 126 ++++++++++++++++++ 3 files changed, 154 insertions(+), 20 deletions(-) diff --git a/internal/management/controller/instance_controller.go b/internal/management/controller/instance_controller.go index d13d0f240..24cde7160 100644 --- a/internal/management/controller/instance_controller.go +++ b/internal/management/controller/instance_controller.go @@ -1165,22 +1165,9 @@ func (r *InstanceReconciler) reconcilePgBackRestConfig(ctx context.Context, clus log.FromContext(ctx).Error(err, "Failed to rotate pgbackrest logs, will retry on next reconcile") } - // Stanza creation is only needed on the primary — the stanza metadata - // lives in S3 and replicas access it directly from there. We re-run - // stanza-create whenever the stanza name changes (not just once), proactively - // initializes the new stanza instead of relying on the lazy WAL-archive - // recovery path. stanza-create is idempotent. - // While suspended, skip stanza-create so the cluster leaves no footprint in - // object storage; it runs on the next reconcile once the flag is cleared - // (e.g. on warm-pool adoption). archive_mode is untouched, so this is - // restart-free. - stanza := cluster.GetPgBackRestStanzaName() - if isPrimary && !cluster.IsPgBackRestSuspended() && stanzaCreateNeeded(r.pgBackRestStanzaCreated.Load(), stanza) { - if err := pgbackrest.StanzaCreate(ctx, stanza); err != nil { - log.FromContext(ctx).Error(err, "Failed to create pgbackrest stanza, will retry on next reconcile") - return nil - } - r.pgBackRestStanzaCreated.Store(&stanza) + if err := r.reconcilePgBackRestStanza(ctx, cluster, configChanged); err != nil { + log.FromContext(ctx).Error(err, "Failed to create pgbackrest stanza, will retry on next reconcile") + return nil } // Publish the generation whose pgbackrest configuration is now fully @@ -1190,6 +1177,29 @@ func (r *InstanceReconciler) reconcilePgBackRestConfig(ctx context.Context, clus return nil } +// reconcilePgBackRestStanza initializes the repository after a configuration change. +// Failed initialization remains pending even after the config file is up to date. +func (r *InstanceReconciler) reconcilePgBackRestStanza( + ctx context.Context, cluster *apiv1.Cluster, configChanged bool, +) error { + if configChanged { + r.pgBackRestStanzaCreated.Store(nil) + } + if r.instance.GetPodName() != cluster.Status.CurrentPrimary || cluster.IsPgBackRestSuspended() { + return nil + } + + stanza := cluster.GetPgBackRestStanzaName() + if !stanzaCreateNeeded(r.pgBackRestStanzaCreated.Load(), stanza) { + return nil + } + if err := pgbackrest.StanzaCreate(ctx, stanza); err != nil { + return err + } + r.pgBackRestStanzaCreated.Store(&stanza) + return nil +} + // reconcilePgBackRestTLSServer ensures that the server has loaded the current // configuration. A failed reload remains pending because the file is already // current and will not report another content change on the next reconcile. diff --git a/internal/management/controller/manager.go b/internal/management/controller/manager.go index 12cf0d400..50636e0a9 100644 --- a/internal/management/controller/manager.go +++ b/internal/management/controller/manager.go @@ -68,10 +68,8 @@ type InstanceReconciler struct { certificateReconciler certificateRefresher pluginRepository repository.Interface - // pgBackRestStanzaCreated holds the name of the pgbackrest stanza that was - // last created by this manager. It is a pointer (not a bool) so that a - // change of stanza name triggers stanza-create for the new stanza instead - // of being skipped. + // pgBackRestStanzaCreated records successful initialization. Configuration + // changes clear it so a new repository is initialized under the same stanza. pgBackRestStanzaCreated atomic.Pointer[string] pgBackRestReloadPending atomic.Bool pgBackRestTLSServer pgBackRestTLSServer diff --git a/internal/management/controller/pgbackrest_stanza_test.go b/internal/management/controller/pgbackrest_stanza_test.go index 5d54eab02..32a560429 100644 --- a/internal/management/controller/pgbackrest_stanza_test.go +++ b/internal/management/controller/pgbackrest_stanza_test.go @@ -20,6 +20,17 @@ SPDX-License-Identifier: Apache-2.0 package controller import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + apiv1 "github.com/xataio/xata-cnpg/api/v1" + "github.com/xataio/xata-cnpg/pkg/management/postgres" + "github.com/xataio/xata-cnpg/pkg/pgbackrest" + "github.com/xataio/xata-cnpg/pkg/utils" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" . "github.com/onsi/ginkgo/v2" @@ -41,3 +52,118 @@ var _ = Describe("pgbackrest stanzaCreateNeeded", func() { Expect(stanzaCreateNeeded(ptr.To("pool-cluster-xyz"), "branch-abc")).To(BeTrue()) }) }) + +func TestStanzaInitializationAfterConfigChange(t *testing.T) { + dir := t.TempDir() + calls := filepath.Join(dir, "calls") + fail := filepath.Join(dir, "fail") + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("STANZA_CALLS", calls) + t.Setenv("STANZA_FAIL", fail) + binary := `#!/bin/sh +printf '%s\n' "$*" >> "$STANZA_CALLS" +if [ -f "$STANZA_FAIL" ]; then exit 1; fi +` + if err := os.WriteFile(filepath.Join(dir, "pgbackrest"), []byte(binary), 0o700); err != nil { + t.Fatal(err) + } + cluster := &apiv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "branch"}, + Status: apiv1.ClusterStatus{CurrentPrimary: "branch-1"}, + } + r := &InstanceReconciler{instance: postgres.NewInstance().WithPodName("branch-1")} + r.pgBackRestStanzaCreated.Store(ptr.To("branch")) + + // An unchanged configuration does not initialize an existing stanza again. + if err := r.reconcilePgBackRestStanza(context.Background(), cluster, false); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(calls); !os.IsNotExist(err) { + t.Fatal("unexpected stanza-create for unchanged configuration") + } + + // Simulate a repository location change with the same stanza and a transient error. + if err := os.WriteFile(fail, nil, 0o600); err != nil { + t.Fatal(err) + } + if err := r.reconcilePgBackRestStanza(context.Background(), cluster, true); err == nil { + t.Fatal("expected initialization failure") + } + if r.pgBackRestStanzaCreated.Load() != nil { + t.Fatal("failed initialization must remain pending") + } + if err := os.Remove(fail); err != nil { + t.Fatal(err) + } + + // The file is already updated on retry, but initialization must still run. + if err := r.reconcilePgBackRestStanza(context.Background(), cluster, false); err != nil { + t.Fatal(err) + } + if err := r.reconcilePgBackRestStanza(context.Background(), cluster, false); err != nil { + t.Fatal(err) + } + content, err := os.ReadFile(calls) + if err != nil { + t.Fatal(err) + } + if strings.Count(string(content), "stanza-create") != 2 { + t.Fatalf("expected one failed attempt and one successful retry, got %s", content) + } +} + +func TestStanzaInitializationDeferredAfterConfigChange(t *testing.T) { + for _, reason := range []string{"replica", "suspended"} { + t.Run(reason, func(t *testing.T) { + dir := t.TempDir() + calls := filepath.Join(dir, "calls") + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("STANZA_CALLS", calls) + if err := os.WriteFile(filepath.Join(dir, "pgbackrest"), + []byte("#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$STANZA_CALLS\"\n"), 0o700); err != nil { + t.Fatal(err) + } + cluster := &apiv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "branch"}, + Status: apiv1.ClusterStatus{CurrentPrimary: "branch-1"}, + } + if reason == "replica" { + cluster.Status.CurrentPrimary = "branch-2" + } else { + cluster.Annotations = map[string]string{utils.PgBackRestSuspended: "enabled"} + } + r := &InstanceReconciler{instance: postgres.NewInstance().WithPodName("branch-1")} + r.pgBackRestStanzaCreated.Store(ptr.To("branch")) + + // A config change must invalidate initialization without writing to the + // repository from a replica or a suspended cluster. + if err := r.reconcilePgBackRestStanza(context.Background(), cluster, true); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(calls); !os.IsNotExist(err) { + t.Fatal("unexpected stanza-create while initialization is deferred") + } + if r.pgBackRestStanzaCreated.Load() != nil { + t.Fatal("initialization must remain pending") + } + + // Promotion or resumption must initialize the repository even though + // the configuration file is already current. + cluster.Status.CurrentPrimary = "branch-1" + cluster.Annotations = nil + if err := r.reconcilePgBackRestStanza(context.Background(), cluster, false); err != nil { + t.Fatal(err) + } + content, err := os.ReadFile(calls) + if err != nil { + t.Fatal(err) + } + if string(content) != "--config="+pgbackrest.ConfigFilePath+" --stanza=branch stanza-create --no-online\n" { + t.Fatalf("unexpected stanza-create invocation: %s", content) + } + if created := r.pgBackRestStanzaCreated.Load(); created == nil || *created != "branch" { + t.Fatal("successful initialization must be recorded") + } + }) + } +} From 1c401487c7647054f3595e80bae28be098232601 Mon Sep 17 00:00:00 2001 From: Martin Hansen Date: Thu, 24 Sep 2026 11:32:03 +0200 Subject: [PATCH 2/3] Remove generated files from diff --- .gitattributes | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 000000000..ee1d34cd6 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,17 @@ +# Collapse generated files in GitHub diffs and omit text patches in Git diffs. +# Use git diff --text to inspect generated changes locally. + +# make generate +/api/v1/zz_generated.deepcopy.go linguist-generated=true -diff +/pkg/client/applyconfiguration/** linguist-generated=true -diff + +# make manifests +/config/crd/bases/*.yaml linguist-generated=true -diff +/config/rbac/role.yaml linguist-generated=true -diff +/config/webhook/manifests.yaml linguist-generated=true -diff + +# make -C charts update-crds +/charts/cloudnative-pg/templates/crds/crds.yaml linguist-generated=true -diff + +# make apidoc +/docs/src/cloudnative-pg.v1.md linguist-generated=true -diff From 5d0c5414a4d9265b7d7bd301d46d8558563a84f5 Mon Sep 17 00:00:00 2001 From: Martin Hansen Date: Thu, 24 Sep 2026 14:34:28 +0200 Subject: [PATCH 3/3] Fix lint failures in stanza regression tests The fake pgBackRest command must be executable, and its output is read from a test-owned temporary directory. Document these narrow gosec exceptions so CI accepts the fixture without disabling security checks for other code. Keep imports in the configured gci groups. --- .../management/controller/pgbackrest_stanza_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/management/controller/pgbackrest_stanza_test.go b/internal/management/controller/pgbackrest_stanza_test.go index 32a560429..515163cd1 100644 --- a/internal/management/controller/pgbackrest_stanza_test.go +++ b/internal/management/controller/pgbackrest_stanza_test.go @@ -26,12 +26,13 @@ import ( "strings" "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + apiv1 "github.com/xataio/xata-cnpg/api/v1" "github.com/xataio/xata-cnpg/pkg/management/postgres" "github.com/xataio/xata-cnpg/pkg/pgbackrest" "github.com/xataio/xata-cnpg/pkg/utils" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/utils/ptr" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -64,6 +65,7 @@ func TestStanzaInitializationAfterConfigChange(t *testing.T) { printf '%s\n' "$*" >> "$STANZA_CALLS" if [ -f "$STANZA_FAIL" ]; then exit 1; fi ` + // #nosec G306 -- The fake executable needs owner execute permission inside t.TempDir. if err := os.WriteFile(filepath.Join(dir, "pgbackrest"), []byte(binary), 0o700); err != nil { t.Fatal(err) } @@ -103,7 +105,7 @@ if [ -f "$STANZA_FAIL" ]; then exit 1; fi if err := r.reconcilePgBackRestStanza(context.Background(), cluster, false); err != nil { t.Fatal(err) } - content, err := os.ReadFile(calls) + content, err := os.ReadFile(calls) // #nosec G304 -- Test-owned path inside t.TempDir. if err != nil { t.Fatal(err) } @@ -119,6 +121,7 @@ func TestStanzaInitializationDeferredAfterConfigChange(t *testing.T) { calls := filepath.Join(dir, "calls") t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) t.Setenv("STANZA_CALLS", calls) + // #nosec G306 -- The fake executable needs owner execute permission inside t.TempDir. if err := os.WriteFile(filepath.Join(dir, "pgbackrest"), []byte("#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$STANZA_CALLS\"\n"), 0o700); err != nil { t.Fatal(err) @@ -154,7 +157,7 @@ func TestStanzaInitializationDeferredAfterConfigChange(t *testing.T) { if err := r.reconcilePgBackRestStanza(context.Background(), cluster, false); err != nil { t.Fatal(err) } - content, err := os.ReadFile(calls) + content, err := os.ReadFile(calls) // #nosec G304 -- Test-owned path inside t.TempDir. if err != nil { t.Fatal(err) }