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 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..515163cd1 100644 --- a/internal/management/controller/pgbackrest_stanza_test.go +++ b/internal/management/controller/pgbackrest_stanza_test.go @@ -20,8 +20,20 @@ SPDX-License-Identifier: Apache-2.0 package controller import ( + "context" + "os" + "path/filepath" + "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" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -41,3 +53,120 @@ 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 +` + // #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) + } + 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) // #nosec G304 -- Test-owned path inside t.TempDir. + 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) + // #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) + } + 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) // #nosec G304 -- Test-owned path inside t.TempDir. + 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") + } + }) + } +}