From 9304fd14cfca40ca4716a9440119dbad08046d07 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Thu, 16 Jul 2026 18:05:28 +0200 Subject: [PATCH 1/3] feat: honor the operator's check_empty_wal_archive decision Archive() and the restore job hook each re-derived, on their own, whether to verify the WAL archive destination is empty, by reading a Cluster annotation and, for Archive, an on-disk marker file. That decision belongs to the operator, which already tracks both the annotation and the marker file's lifecycle. Honor cnpg-i's new WALArchiveRequest/RestoreRequest field CheckEmptyWalArchive when the operator sets it: obey it directly, without re-inspecting the marker file. Only fall back to the previous annotation-and-marker-file logic when talking to an operator that predates this field. Signed-off-by: Marco Nenciarini --- go.mod | 2 +- go.sum | 2 ++ internal/cnpgi/common/wal.go | 20 +++++++++++++++----- internal/cnpgi/restore/restore.go | 16 ++++++++++++++-- 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/go.mod b/go.mod index 5a95b0eb..999c45a8 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/cloudnative-pg/api v1.30.0 github.com/cloudnative-pg/barman-cloud v0.5.2-0.20260709152604-43158c204df1 github.com/cloudnative-pg/cloudnative-pg v1.30.0 - github.com/cloudnative-pg/cnpg-i v0.5.0 + github.com/cloudnative-pg/cnpg-i v0.5.1-0.20260716144117-db806aee0db1 github.com/cloudnative-pg/cnpg-i-machinery v0.4.2 github.com/cloudnative-pg/machinery v0.5.0 github.com/onsi/ginkgo/v2 v2.32.0 diff --git a/go.sum b/go.sum index ada804db..f0fc6762 100644 --- a/go.sum +++ b/go.sum @@ -26,6 +26,8 @@ github.com/cloudnative-pg/cloudnative-pg v1.30.0 h1:fnhVq44xXx97MNiuvJsPrX1vSjYb github.com/cloudnative-pg/cloudnative-pg v1.30.0/go.mod h1:QkolwBOWZ+GvAiJt6KpDSymwkpf0K19/p4Q6MQlTM8U= github.com/cloudnative-pg/cnpg-i v0.5.0 h1:/TOzpNT6cwNgrpftTtrnLKdoHgMwd+88vZgXjlVgXeE= github.com/cloudnative-pg/cnpg-i v0.5.0/go.mod h1:7Gh4+UzhBpGhr4DreB1GN9wGYfvxwXCXZUyVt3zE/3I= +github.com/cloudnative-pg/cnpg-i v0.5.1-0.20260716144117-db806aee0db1 h1:HO7S6jJ21DhSS6oQg7Ok1+uT23hF6I58im1IIRMPufk= +github.com/cloudnative-pg/cnpg-i v0.5.1-0.20260716144117-db806aee0db1/go.mod h1:4kcpLAj+feMTnh0TVadXRASdVnEhBytNSm/BvktLgmI= github.com/cloudnative-pg/cnpg-i-machinery v0.4.2 h1:0reS9MtyLYINHXQ/MfxJ9jp39hhBf8e3Qdj+T5Nsq6I= github.com/cloudnative-pg/cnpg-i-machinery v0.4.2/go.mod h1:gvrKabgxXq0zGthXGucemDdsxakLEQDMxn43M4HLW30= github.com/cloudnative-pg/machinery v0.5.0 h1:hhTnkzn+AiN3NmbjCQ6RXj5rfqV3K6arzq6kdXAzcnQ= diff --git a/internal/cnpgi/common/wal.go b/internal/cnpgi/common/wal.go index bb7d3e08..a7d66e6c 100644 --- a/internal/cnpgi/common/wal.go +++ b/internal/cnpgi/common/wal.go @@ -155,13 +155,23 @@ func (w WALServiceImplementation) Archive( return nil, err } - // Step 2: Check if the archive location is safe to perform archiving - checkFileExisting, err := fileutils.FileExists(emptyWalArchiveFile) - if err != nil { - return nil, fmt.Errorf("while checking for empty wal archive check file %q: %w", emptyWalArchiveFile, err) + // Step 2: Check if the archive location is safe to perform archiving. + // The operator owns the marker file's lifecycle, so a non-nil decision + // already accounts for it and is obeyed as-is. A nil value means an + // operator that predates this field; fall back to checking the marker + // file and Cluster annotation directly. + var checkEmptyWalArchive bool + if request.CheckEmptyWalArchive != nil { + checkEmptyWalArchive = *request.CheckEmptyWalArchive + } else { + checkFileExisting, err := fileutils.FileExists(emptyWalArchiveFile) + if err != nil { + return nil, fmt.Errorf("while checking for empty wal archive check file %q: %w", emptyWalArchiveFile, err) + } + checkEmptyWalArchive = utils.IsEmptyWalArchiveCheckEnabled(&configuration.Cluster.ObjectMeta) && checkFileExisting } - if utils.IsEmptyWalArchiveCheckEnabled(&configuration.Cluster.ObjectMeta) && checkFileExisting { + if checkEmptyWalArchive { if err := CheckBackupDestination( ctx, &objectStore.Spec.Configuration, diff --git a/internal/cnpgi/restore/restore.go b/internal/cnpgi/restore/restore.go index f286565e..0b46f44a 100644 --- a/internal/cnpgi/restore/restore.go +++ b/internal/cnpgi/restore/restore.go @@ -113,6 +113,7 @@ func (impl JobHookImpl) Restore( configuration.Cluster, &targetObjectStore.Spec.Configuration, targetObjectStore.Name, + req.CheckEmptyWalArchive, ); err != nil { return nil, err } @@ -250,6 +251,7 @@ func (impl *JobHookImpl) checkBackupDestination( cluster *cnpgv1.Cluster, barmanConfiguration *cnpgv1.BarmanObjectStoreConfiguration, objectStoreName string, + operatorCheckEmptyWalArchive *bool, ) error { // Get environment from cache env, err := barmanCredentials.EnvSetCloudCredentialsAndCertificates(ctx, @@ -288,8 +290,18 @@ func (impl *JobHookImpl) checkBackupDestination( } } - // Check if we're ok to archive in the desired destination - if utils.IsEmptyWalArchiveCheckEnabled(&cluster.ObjectMeta) { + // Check if we're ok to restore from the desired destination. Unlike + // archiving, restore is a one-shot operation that has never been gated + // on the first-archive marker file, so the only fallback needed here + // (for an operator that predates this field) is the Cluster annotation. + var checkEmptyWalArchive bool + if operatorCheckEmptyWalArchive != nil { + checkEmptyWalArchive = *operatorCheckEmptyWalArchive + } else { + checkEmptyWalArchive = utils.IsEmptyWalArchiveCheckEnabled(&cluster.ObjectMeta) + } + + if checkEmptyWalArchive { return common.CheckBackupDestination(ctx, barmanConfiguration, walArchiver, serverName) } From 37a7937368ad11fc28ff2b05c705616095e9f0c4 Mon Sep 17 00:00:00 2001 From: Armando Ruocco Date: Fri, 17 Jul 2026 16:19:08 +0200 Subject: [PATCH 2/3] refactor: extract the empty WAL archive decision into a helper The WAL archive hook and the restore job hook each derived, inline, whether to verify the archive destination before proceeding. Pull each decision into a small function so the operator-decision-versus-fallback logic can be exercised on its own. Behavior is unchanged: archiving still fails on a marker-file stat error, and restore still falls back to the Cluster annotation alone. Signed-off-by: Armando Ruocco --- internal/cnpgi/common/wal.go | 45 ++++++++++++++++++++++--------- internal/cnpgi/restore/restore.go | 26 +++++++++--------- 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/internal/cnpgi/common/wal.go b/internal/cnpgi/common/wal.go index a7d66e6c..9bdc22ce 100644 --- a/internal/cnpgi/common/wal.go +++ b/internal/cnpgi/common/wal.go @@ -156,19 +156,13 @@ func (w WALServiceImplementation) Archive( } // Step 2: Check if the archive location is safe to perform archiving. - // The operator owns the marker file's lifecycle, so a non-nil decision - // already accounts for it and is obeyed as-is. A nil value means an - // operator that predates this field; fall back to checking the marker - // file and Cluster annotation directly. - var checkEmptyWalArchive bool - if request.CheckEmptyWalArchive != nil { - checkEmptyWalArchive = *request.CheckEmptyWalArchive - } else { - checkFileExisting, err := fileutils.FileExists(emptyWalArchiveFile) - if err != nil { - return nil, fmt.Errorf("while checking for empty wal archive check file %q: %w", emptyWalArchiveFile, err) - } - checkEmptyWalArchive = utils.IsEmptyWalArchiveCheckEnabled(&configuration.Cluster.ObjectMeta) && checkFileExisting + checkEmptyWalArchive, err := resolveArchiveEmptyWalArchiveCheck( + request.CheckEmptyWalArchive, + configuration.Cluster, + emptyWalArchiveFile, + ) + if err != nil { + return nil, err } if checkEmptyWalArchive { @@ -233,6 +227,31 @@ func (w WALServiceImplementation) Archive( return &wal.WALArchiveResult{}, nil } +// resolveArchiveEmptyWalArchiveCheck reports whether the WAL archive +// destination must be verified before archiving this segment. +// +// The operator owns the marker file's lifecycle, so when it sets the decision +// (non-nil) that value already accounts for the marker and is obeyed as-is. A +// nil value comes from an operator that predates this field, so we fall back to +// the previous logic: the Cluster annotation combined with the on-disk marker +// file. +func resolveArchiveEmptyWalArchiveCheck( + operatorDecision *bool, + cluster *cnpgv1.Cluster, + markerFilePath string, +) (bool, error) { + if operatorDecision != nil { + return *operatorDecision, nil + } + + markerFilePresent, err := fileutils.FileExists(markerFilePath) + if err != nil { + return false, fmt.Errorf("while checking for empty wal archive check file %q: %w", markerFilePath, err) + } + + return utils.IsEmptyWalArchiveCheckEnabled(&cluster.ObjectMeta) && markerFilePresent, nil +} + // Restore implements the WALService interface func (w WALServiceImplementation) Restore( ctx context.Context, diff --git a/internal/cnpgi/restore/restore.go b/internal/cnpgi/restore/restore.go index 0b46f44a..9ee73548 100644 --- a/internal/cnpgi/restore/restore.go +++ b/internal/cnpgi/restore/restore.go @@ -290,24 +290,26 @@ func (impl *JobHookImpl) checkBackupDestination( } } - // Check if we're ok to restore from the desired destination. Unlike - // archiving, restore is a one-shot operation that has never been gated - // on the first-archive marker file, so the only fallback needed here - // (for an operator that predates this field) is the Cluster annotation. - var checkEmptyWalArchive bool - if operatorCheckEmptyWalArchive != nil { - checkEmptyWalArchive = *operatorCheckEmptyWalArchive - } else { - checkEmptyWalArchive = utils.IsEmptyWalArchiveCheckEnabled(&cluster.ObjectMeta) - } - - if checkEmptyWalArchive { + if resolveRestoreEmptyWalArchiveCheck(operatorCheckEmptyWalArchive, cluster) { return common.CheckBackupDestination(ctx, barmanConfiguration, walArchiver, serverName) } return nil } +// resolveRestoreEmptyWalArchiveCheck reports whether the destination must be +// verified before restoring. When the operator sets the decision (non-nil) it +// is obeyed as-is; a nil value comes from an operator that predates this field, +// so we fall back to the Cluster annotation. Unlike archiving, restore is a +// one-shot operation that has never been gated on the first-archive marker +// file, so the annotation is the only fallback needed. +func resolveRestoreEmptyWalArchiveCheck(operatorDecision *bool, cluster *cnpgv1.Cluster) bool { + if operatorDecision != nil { + return *operatorDecision + } + return utils.IsEmptyWalArchiveCheckEnabled(&cluster.ObjectMeta) +} + // restoreCustomWalDir moves the current pg_wal data to the specified custom wal dir and applies the symlink // returns indicating if any changes were made and any error encountered in the process func (impl JobHookImpl) restoreCustomWalDir(ctx context.Context) (bool, error) { From 664f808d7e56b51bda5dec95b8d598e81ed86d3c Mon Sep 17 00:00:00 2001 From: Armando Ruocco Date: Fri, 17 Jul 2026 16:19:16 +0200 Subject: [PATCH 3/3] test: cover the empty WAL archive decision in both hooks Add unit coverage for the WAL archive and restore-job decision helpers. The tables pin the version-skew contract: when the operator sets the decision it overrides both the Cluster annotation and, for archiving, the on-disk marker file, while a nil decision (an operator that predates the field) reproduces the previous annotation-based behavior. Signed-off-by: Armando Ruocco --- internal/cnpgi/common/wal_test.go | 72 +++++++++++++++++++++++++- internal/cnpgi/restore/restore_test.go | 66 +++++++++++++++++++++++ internal/cnpgi/restore/suite_test.go | 32 ++++++++++++ 3 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 internal/cnpgi/restore/restore_test.go create mode 100644 internal/cnpgi/restore/suite_test.go diff --git a/internal/cnpgi/common/wal_test.go b/internal/cnpgi/common/wal_test.go index 92d836c0..4e66bb3c 100644 --- a/internal/cnpgi/common/wal_test.go +++ b/internal/cnpgi/common/wal_test.go @@ -20,12 +20,18 @@ SPDX-License-Identifier: Apache-2.0 package common import ( + "os" + "path/filepath" + cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/metadata" "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/operator/config" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" ) var _ = Describe("resolveRestoreObjectStore", func() { @@ -96,3 +102,65 @@ var _ = Describe("resolveRestoreObjectStore", func() { "cluster-server", "cluster-store"), ) }) + +var _ = Describe("resolveArchiveEmptyWalArchiveCheck", func() { + // skipAnnotation mirrors the unexported constant in cloudnative-pg's + // pkg/utils; hard-coding the literal makes a divergence surface as a + // failing test rather than silently disabling the check. + const skipAnnotation = "cnpg.io/skipEmptyWalArchiveCheck" + + clusterWith := func(annotationValue *string) *cnpgv1.Cluster { + cluster := &cnpgv1.Cluster{} + if annotationValue != nil { + cluster.Annotations = map[string]string{skipAnnotation: *annotationValue} + } + return cluster + } + + // markerPath returns the marker file path inside a fresh temp dir, + // creating the file there when present is true. + markerPath := func(present bool) string { + filePath := filepath.Join(GinkgoT().TempDir(), metadata.CheckEmptyWalArchiveFile) + if present { + Expect(os.WriteFile(filePath, []byte{}, 0o600)).To(Succeed()) + } + return filePath + } + + When("the operator sets the decision", func() { + It("obeys true, ignoring the annotation and the marker file", func() { + // annotation would skip the check and the marker is absent, yet the + // operator's explicit true must still win. + got, err := resolveArchiveEmptyWalArchiveCheck( + ptr.To(true), clusterWith(ptr.To("enabled")), markerPath(false)) + Expect(err).NotTo(HaveOccurred()) + Expect(got).To(BeTrue()) + }) + + It("obeys false, ignoring the annotation and the marker file", func() { + // annotation would keep the check on and the marker is present, yet the + // operator's explicit false must still win. + got, err := resolveArchiveEmptyWalArchiveCheck( + ptr.To(false), clusterWith(nil), markerPath(true)) + Expect(err).NotTo(HaveOccurred()) + Expect(got).To(BeFalse()) + }) + }) + + When("the operator predates the field (nil decision)", func() { + DescribeTable( + "falls back to the annotation combined with the marker file", + func(annotationValue *string, markerPresent bool, expected bool) { + got, err := resolveArchiveEmptyWalArchiveCheck( + nil, clusterWith(annotationValue), markerPath(markerPresent)) + Expect(err).NotTo(HaveOccurred()) + Expect(got).To(Equal(expected)) + }, + Entry("no annotation and marker present: check runs", nil, true, true), + Entry("no annotation and marker absent: check skipped", nil, false, false), + Entry("opt-out annotation and marker present: check skipped", ptr.To("enabled"), true, false), + Entry("unrelated annotation value and marker present: check runs", ptr.To("something-else"), true, true), + Entry("empty annotation value and marker present: check runs", ptr.To(""), true, true), + ) + }) +}) diff --git a/internal/cnpgi/restore/restore_test.go b/internal/cnpgi/restore/restore_test.go new file mode 100644 index 00000000..a88a7f39 --- /dev/null +++ b/internal/cnpgi/restore/restore_test.go @@ -0,0 +1,66 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package restore + +import ( + cnpgv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1" + "k8s.io/utils/ptr" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("resolveRestoreEmptyWalArchiveCheck", func() { + // skipAnnotation mirrors the unexported constant in cloudnative-pg's + // pkg/utils; hard-coding the literal makes a divergence surface as a + // failing test rather than silently disabling the check. + const skipAnnotation = "cnpg.io/skipEmptyWalArchiveCheck" + + clusterWith := func(annotationValue *string) *cnpgv1.Cluster { + cluster := &cnpgv1.Cluster{} + if annotationValue != nil { + cluster.Annotations = map[string]string{skipAnnotation: *annotationValue} + } + return cluster + } + + When("the operator sets the decision", func() { + It("obeys true even when the annotation would skip the check", func() { + Expect(resolveRestoreEmptyWalArchiveCheck(ptr.To(true), clusterWith(ptr.To("enabled")))).To(BeTrue()) + }) + + It("obeys false even when the annotation would keep the check on", func() { + Expect(resolveRestoreEmptyWalArchiveCheck(ptr.To(false), clusterWith(nil))).To(BeFalse()) + }) + }) + + When("the operator predates the field (nil decision)", func() { + DescribeTable( + "falls back to the Cluster annotation, never to a marker file", + func(annotationValue *string, expected bool) { + Expect(resolveRestoreEmptyWalArchiveCheck(nil, clusterWith(annotationValue))).To(Equal(expected)) + }, + Entry("no annotation: check runs", nil, true), + Entry("opt-out annotation: check skipped", ptr.To("enabled"), false), + Entry("unrelated annotation value: check runs", ptr.To("something-else"), true), + Entry("empty annotation value: check runs", ptr.To(""), true), + ) + }) +}) diff --git a/internal/cnpgi/restore/suite_test.go b/internal/cnpgi/restore/suite_test.go new file mode 100644 index 00000000..b110bef4 --- /dev/null +++ b/internal/cnpgi/restore/suite_test.go @@ -0,0 +1,32 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package restore + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestRestore(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Restore job hook test suite") +}