From ee97d63a2c9b2922e3ec55891fc6f7d58c88d8be Mon Sep 17 00:00:00 2001 From: Armando Ruocco Date: Mon, 20 Jul 2026 13:06:33 +0200 Subject: [PATCH 1/3] feat: serve restore hooks from the instance sidecar The bootstrap that used to run in a dedicated recovery Job now happens in-process inside the instance pod before PostgreSQL starts, so the sidecar shipped in that pod must answer the same Restore RPC the operator sends over the plugin sockets. The instance mode now registers the restore job hooks and advertises the restore-job service capability so the operator can reach them. A cluster that only bootstraps from an object store, without continued archiving, previously received no sidecar in its instance pods; the injection condition is widened to match what the plugin configuration already considers valid, so those clusters get the sidecar too. Signed-off-by: Armando Ruocco --- internal/cnpgi/common/common.go | 5 +++ internal/cnpgi/instance/identity.go | 7 +++ internal/cnpgi/instance/identity_test.go | 52 +++++++++++++++++++++++ internal/cnpgi/instance/start.go | 11 +++++ internal/cnpgi/operator/lifecycle.go | 7 ++- internal/cnpgi/operator/lifecycle_test.go | 42 ++++++++++++++++++ internal/cnpgi/restore/start.go | 5 +-- 7 files changed, 124 insertions(+), 5 deletions(-) create mode 100644 internal/cnpgi/instance/identity_test.go diff --git a/internal/cnpgi/common/common.go b/internal/cnpgi/common/common.go index 9526d7e..8329d64 100644 --- a/internal/cnpgi/common/common.go +++ b/internal/cnpgi/common/common.go @@ -52,6 +52,11 @@ const ( // BarmanEndpointCACertificateFileName is the name of the file in which the barman endpoint // CA certificate is stored. BarmanEndpointCACertificateFileName = "barman-ca.crt" + + // PgWalVolumePgWalPath is the path of the pg_wal directory inside the WAL volume, + // used when a separate WAL storage is configured. During a restore the pg_wal + // directory is moved here and symlinked back into PGDATA. + PgWalVolumePgWalPath = "/var/lib/postgresql/wal/pg_wal" ) // GetRestoreCABundleEnv gets the enveronment variables to be used when custom diff --git a/internal/cnpgi/instance/identity.go b/internal/cnpgi/instance/identity.go index ea3b2fd..7563de3 100644 --- a/internal/cnpgi/instance/identity.go +++ b/internal/cnpgi/instance/identity.go @@ -70,6 +70,13 @@ func (i IdentityImplementation) GetPluginCapabilities( }, }, }, + { + Type: &identity.PluginCapability_Service_{ + Service: &identity.PluginCapability_Service{ + Type: identity.PluginCapability_Service_TYPE_RESTORE_JOB, + }, + }, + }, }, }, nil } diff --git a/internal/cnpgi/instance/identity_test.go b/internal/cnpgi/instance/identity_test.go new file mode 100644 index 0000000..621145a --- /dev/null +++ b/internal/cnpgi/instance/identity_test.go @@ -0,0 +1,52 @@ +/* +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 instance + +import ( + "github.com/cloudnative-pg/cnpg-i/pkg/identity" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("IdentityImplementation", func() { + Describe("GetPluginCapabilities", func() { + It("declares the WAL, backup, metrics and restore-job services", func(ctx SpecContext) { + impl := IdentityImplementation{} + response, err := impl.GetPluginCapabilities(ctx, &identity.GetPluginCapabilitiesRequest{}) + Expect(err).NotTo(HaveOccurred()) + Expect(response).NotTo(BeNil()) + + var serviceTypes []identity.PluginCapability_Service_Type + for _, capability := range response.GetCapabilities() { + serviceTypes = append(serviceTypes, capability.GetService().GetType()) + } + + // The instance sidecar now runs the phase-0 restore in-process, so it must + // advertise TYPE_RESTORE_JOB alongside the services it already served. + Expect(serviceTypes).To(ConsistOf( + identity.PluginCapability_Service_TYPE_WAL_SERVICE, + identity.PluginCapability_Service_TYPE_BACKUP_SERVICE, + identity.PluginCapability_Service_TYPE_METRICS, + identity.PluginCapability_Service_TYPE_RESTORE_JOB, + )) + }) + }) +}) diff --git a/internal/cnpgi/instance/start.go b/internal/cnpgi/instance/start.go index b222653..a68cda2 100644 --- a/internal/cnpgi/instance/start.go +++ b/internal/cnpgi/instance/start.go @@ -25,11 +25,13 @@ import ( "github.com/cloudnative-pg/cnpg-i-machinery/pkg/pluginhelper/http" "github.com/cloudnative-pg/cnpg-i/pkg/backup" "github.com/cloudnative-pg/cnpg-i/pkg/metrics" + restore "github.com/cloudnative-pg/cnpg-i/pkg/restore/job" "github.com/cloudnative-pg/cnpg-i/pkg/wal" "google.golang.org/grpc" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/common" + barmanrestore "github.com/cloudnative-pg/plugin-barman-cloud/internal/cnpgi/restore" ) // CNPGI is the implementation of the PostgreSQL sidecar @@ -60,6 +62,15 @@ func (c *CNPGI) Start(ctx context.Context) error { metrics.RegisterMetricsServer(server, &metricsImpl{ Client: c.Client, }) + // The instance pod runs the phase-0 bootstrap in-process (no separate + // recovery Job), so the same sidecar must answer the Restore RPC that + // initializes PGDATA from the object store before PostgreSQL starts. + restore.RegisterRestoreJobHooksServer(server, &barmanrestore.JobHookImpl{ + Client: c.Client, + SpoolDirectory: c.SpoolDirectory, + PgDataPath: c.PGDataPath, + PgWalFolderToSymlink: common.PgWalVolumePgWalPath, + }) common.AddHealthCheck(server) return nil } diff --git a/internal/cnpgi/operator/lifecycle.go b/internal/cnpgi/operator/lifecycle.go index 8bb26d8..ca42f0b 100644 --- a/internal/cnpgi/operator/lifecycle.go +++ b/internal/cnpgi/operator/lifecycle.go @@ -339,7 +339,12 @@ func reconcileInstancePod( mutatedPod := pod.DeepCopy() + // A recovery-only cluster (only RecoveryBarmanObjectName set) still needs the + // sidecar in its instance pods: the phase-0 bootstrap restore and the WAL + // replay that follows both run inside the instance and rely on it. This + // condition therefore mirrors what pluginConfiguration.Validate() accepts. if len(pluginConfiguration.BarmanObjectName) != 0 || + len(pluginConfiguration.RecoveryBarmanObjectName) != 0 || len(pluginConfiguration.ReplicaSourceBarmanObjectName) != 0 { if err := reconcilePodSpec( cluster, @@ -353,7 +358,7 @@ func reconcileInstancePod( return nil, fmt.Errorf("while reconciling pod spec for pod: %w", err) } } else { - contextLogger.Debug("No need to mutate instance with no backup & archiving configuration") + contextLogger.Debug("No need to mutate instance with no barman object store configuration") } patch, err := object.CreatePatch(mutatedPod, pod) diff --git a/internal/cnpgi/operator/lifecycle_test.go b/internal/cnpgi/operator/lifecycle_test.go index a4851be..fb605a7 100644 --- a/internal/cnpgi/operator/lifecycle_test.go +++ b/internal/cnpgi/operator/lifecycle_test.go @@ -242,6 +242,48 @@ var _ = Describe("LifecycleImplementation", func() { HaveKey("value"))) }) + It("injects the sidecar for a recovery-only cluster", func(ctx SpecContext) { + recoveryOnlyConfig := &config.PluginConfiguration{ + RecoveryBarmanObjectName: "minio-store-recovery", + } + pod := &corev1.Pod{ + TypeMeta: podTypeMeta, + ObjectMeta: metav1.ObjectMeta{Name: "test-pod"}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "postgres"}}}, + } + podJSON, _ := json.Marshal(pod) + request := &lifecycle.OperatorLifecycleRequest{ + ObjectDefinition: podJSON, + } + + response, err := reconcileInstancePod(ctx, cluster, request, recoveryOnlyConfig, sidecarConfiguration{}) + Expect(err).NotTo(HaveOccurred()) + Expect(response).NotTo(BeNil()) + Expect(response.JsonPatch).NotTo(BeEmpty()) + var patch []map[string]interface{} + Expect(json.Unmarshal(response.JsonPatch, &patch)).To(Succeed()) + Expect(patch).To(ContainElement(HaveKeyWithValue("path", "/spec/initContainers"))) + }) + + It("does not mutate the pod when no object store is configured", func(ctx SpecContext) { + emptyConfig := &config.PluginConfiguration{} + pod := &corev1.Pod{ + TypeMeta: podTypeMeta, + ObjectMeta: metav1.ObjectMeta{Name: "test-pod"}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "postgres"}}}, + } + podJSON, _ := json.Marshal(pod) + request := &lifecycle.OperatorLifecycleRequest{ + ObjectDefinition: podJSON, + } + + response, err := reconcileInstancePod(ctx, cluster, request, emptyConfig, sidecarConfiguration{}) + Expect(err).NotTo(HaveOccurred()) + Expect(response).NotTo(BeNil()) + // An empty patch means the pod was left untouched: no sidecar injected. + Expect(response.JsonPatch).To(BeEmpty()) + }) + It("returns an error for invalid pod definition", func(ctx SpecContext) { request := &lifecycle.OperatorLifecycleRequest{ ObjectDefinition: []byte("invalid-json"), diff --git a/internal/cnpgi/restore/start.go b/internal/cnpgi/restore/start.go index efb7828..0f78f90 100644 --- a/internal/cnpgi/restore/start.go +++ b/internal/cnpgi/restore/start.go @@ -44,9 +44,6 @@ type CNPGI struct { // Start starts the GRPC service func (c *CNPGI) Start(ctx context.Context) error { - // PgWalVolumePgWalPath is the path of pg_wal directory inside the WAL volume when present - const PgWalVolumePgWalPath = "/var/lib/postgresql/wal/pg_wal" - enrich := func(server *grpc.Server) error { wal.RegisterWALServer(server, common.WALServiceImplementation{ InstanceName: c.InstanceName, @@ -60,7 +57,7 @@ func (c *CNPGI) Start(ctx context.Context) error { Client: c.Client, SpoolDirectory: c.SpoolDirectory, PgDataPath: c.PGDataPath, - PgWalFolderToSymlink: PgWalVolumePgWalPath, + PgWalFolderToSymlink: common.PgWalVolumePgWalPath, }) common.AddHealthCheck(server) From 4c089ce55f4d0839deb554c2a6977010492c70c1 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Fri, 24 Jul 2026 14:28:06 +0200 Subject: [PATCH 2/3] refactor: deduplicate barman object store presence check Validate() and the instance sidecar injection condition each independently re-encoded "is any of BarmanObjectName, RecoveryBarmanObjectName or ReplicaSourceBarmanObjectName set". The injection condition had already drifted from Validate() once, missing RecoveryBarmanObjectName until this PR added it back. Extract a single HasAnyBarmanObjectStore() method so the two checks can no longer drift apart the same way again. Signed-off-by: Marco Nenciarini --- internal/cnpgi/operator/config/config.go | 13 ++++++++++--- internal/cnpgi/operator/lifecycle.go | 4 +--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/internal/cnpgi/operator/config/config.go b/internal/cnpgi/operator/config/config.go index 7f16dda..ba596cc 100644 --- a/internal/cnpgi/operator/config/config.go +++ b/internal/cnpgi/operator/config/config.go @@ -102,6 +102,15 @@ func (config *PluginConfiguration) GetReplicaSourceBarmanObjectKey() types.Names } } +// HasAnyBarmanObjectStore returns true if the configuration references at least +// one barman object store, be it for backup/archiving, recovery, or as a +// replica source. +func (config *PluginConfiguration) HasAnyBarmanObjectStore() bool { + return len(config.BarmanObjectName) > 0 || + len(config.RecoveryBarmanObjectName) > 0 || + len(config.ReplicaSourceBarmanObjectName) > 0 +} + // GetReferredBarmanObjectsKey gets the list of barman objects referred by this // plugin configuration func (config *PluginConfiguration) GetReferredBarmanObjectsKey() []types.NamespacedName { @@ -263,9 +272,7 @@ func getReplicaSourcePlugin(cluster *cnpgv1.Cluster) *cnpgv1.PluginConfiguration func (config *PluginConfiguration) Validate() error { err := NewConfigurationError() - if len(config.BarmanObjectName) == 0 && - len(config.RecoveryBarmanObjectName) == 0 && - len(config.ReplicaSourceBarmanObjectName) == 0 { + if !config.HasAnyBarmanObjectStore() { return err.WithMessage("no reference to barmanObjectName have been included") } diff --git a/internal/cnpgi/operator/lifecycle.go b/internal/cnpgi/operator/lifecycle.go index ca42f0b..8938f63 100644 --- a/internal/cnpgi/operator/lifecycle.go +++ b/internal/cnpgi/operator/lifecycle.go @@ -343,9 +343,7 @@ func reconcileInstancePod( // sidecar in its instance pods: the phase-0 bootstrap restore and the WAL // replay that follows both run inside the instance and rely on it. This // condition therefore mirrors what pluginConfiguration.Validate() accepts. - if len(pluginConfiguration.BarmanObjectName) != 0 || - len(pluginConfiguration.RecoveryBarmanObjectName) != 0 || - len(pluginConfiguration.ReplicaSourceBarmanObjectName) != 0 { + if pluginConfiguration.HasAnyBarmanObjectStore() { if err := reconcilePodSpec( cluster, &mutatedPod.Spec, From bb0d21794101ae09e10969167eb430a2aa1eb104 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Mon, 27 Jul 2026 17:47:05 +0200 Subject: [PATCH 3/3] fix: avoid rolling out recovery-only pods just to drop the sidecar Gating sidecar injection purely on cluster initialization state made the operator's periodic EVALUATE drift-check disagree with the pod spec stored at creation time, so it would roll out the primary right after it finished bootstrapping, solely to strip the now-unneeded sidecar. Only stop adding the sidecar on TYPE_CREATE, once the cluster is initialized; keep injecting it unconditionally on TYPE_EVALUATE so an already-running pod never drifts against its own stored spec. Signed-off-by: Marco Nenciarini --- internal/cnpgi/operator/lifecycle.go | 52 +++++++++++++++++++--- internal/cnpgi/operator/lifecycle_test.go | 54 +++++++++++++++++++++++ 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/internal/cnpgi/operator/lifecycle.go b/internal/cnpgi/operator/lifecycle.go index 8938f63..bd8d86e 100644 --- a/internal/cnpgi/operator/lifecycle.go +++ b/internal/cnpgi/operator/lifecycle.go @@ -322,6 +322,50 @@ func (impl LifecycleImplementation) collectAdditionalInstanceArgs( return nil, nil } +// shouldInjectBarmanSidecar decides whether an instance pod needs the +// plugin-barman-cloud sidecar. +// +// A cluster doing backup/archiving or serving as a replica source needs the +// sidecar in every instance pod for as long as the cluster exists, so those +// two cases always inject it. A recovery-only cluster (only +// RecoveryBarmanObjectName set, mirroring what pluginConfiguration.Validate() +// accepts) only ever needs the sidecar for its one-time bootstrap restore, so +// once the cluster has completed that initial bootstrap there's no reason to +// keep carrying it on every future pod. +// +// That said, the two operation types this hook is invoked with can't be +// treated the same way here. TYPE_CREATE fires only when a Pod is actually +// about to be persisted (the bootstrap pod itself, a later replica, or any +// pod recreated for an unrelated reason), so it's safe to gate it on +// cluster.IsInitialized(): a pod created after bootstrap simply won't carry +// the sidecar. TYPE_EVALUATE, however, is also used by the operator's +// checkPodSpecIsOutdated to re-evaluate an already-running pod's spec for +// drift on every reconcile; gating that the same way would make an +// already-initialized cluster's freshly re-evaluated spec disagree with the +// spec stored at the pod's creation, and the operator would roll out the +// primary purely to strip the sidecar right after it finished bootstrapping. +// So EVALUATE always keeps the sidecar for a recovery-only cluster, and only +// CREATE actually stops adding it to pods created after initialization. +func shouldInjectBarmanSidecar( + cluster *cnpgv1.Cluster, + pluginConfiguration *config.PluginConfiguration, + request *lifecycle.OperatorLifecycleRequest, +) bool { + if len(pluginConfiguration.BarmanObjectName) != 0 || len(pluginConfiguration.ReplicaSourceBarmanObjectName) != 0 { + return true + } + + if len(pluginConfiguration.RecoveryBarmanObjectName) == 0 { + return false + } + + if request.GetOperationType().GetType() == lifecycle.OperatorOperationType_TYPE_CREATE { + return !cluster.IsInitialized() + } + + return true +} + func reconcileInstancePod( ctx context.Context, cluster *cnpgv1.Cluster, @@ -339,11 +383,7 @@ func reconcileInstancePod( mutatedPod := pod.DeepCopy() - // A recovery-only cluster (only RecoveryBarmanObjectName set) still needs the - // sidecar in its instance pods: the phase-0 bootstrap restore and the WAL - // replay that follows both run inside the instance and rely on it. This - // condition therefore mirrors what pluginConfiguration.Validate() accepts. - if pluginConfiguration.HasAnyBarmanObjectStore() { + if shouldInjectBarmanSidecar(cluster, pluginConfiguration, request) { if err := reconcilePodSpec( cluster, &mutatedPod.Spec, @@ -356,7 +396,7 @@ func reconcileInstancePod( return nil, fmt.Errorf("while reconciling pod spec for pod: %w", err) } } else { - contextLogger.Debug("No need to mutate instance with no barman object store configuration") + contextLogger.Debug("No need to mutate instance, sidecar not required for this configuration and pod") } patch, err := object.CreatePatch(mutatedPod, pod) diff --git a/internal/cnpgi/operator/lifecycle_test.go b/internal/cnpgi/operator/lifecycle_test.go index fb605a7..05351f6 100644 --- a/internal/cnpgi/operator/lifecycle_test.go +++ b/internal/cnpgi/operator/lifecycle_test.go @@ -265,6 +265,60 @@ var _ = Describe("LifecycleImplementation", func() { Expect(patch).To(ContainElement(HaveKeyWithValue("path", "/spec/initContainers"))) }) + It("does not inject the sidecar into a new pod of a recovery-only cluster "+ + "that has already completed its initial bootstrap", func(ctx SpecContext) { + recoveryOnlyConfig := &config.PluginConfiguration{ + RecoveryBarmanObjectName: "minio-store-recovery", + } + cluster.Status.LatestGeneratedNode = 1 + pod := &corev1.Pod{ + TypeMeta: podTypeMeta, + ObjectMeta: metav1.ObjectMeta{Name: "test-pod"}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "postgres"}}}, + } + podJSON, _ := json.Marshal(pod) + request := &lifecycle.OperatorLifecycleRequest{ + ObjectDefinition: podJSON, + OperationType: &lifecycle.OperatorOperationType{ + Type: lifecycle.OperatorOperationType_TYPE_CREATE, + }, + } + + response, err := reconcileInstancePod(ctx, cluster, request, recoveryOnlyConfig, sidecarConfiguration{}) + Expect(err).NotTo(HaveOccurred()) + Expect(response).NotTo(BeNil()) + Expect(response.JsonPatch).To(BeEmpty()) + }) + + It("keeps injecting the sidecar on EVALUATE for an already-bootstrapped recovery-only "+ + "cluster, so the operator never sees drift against the pod's stored spec and rolls "+ + "it out just to strip the sidecar", func(ctx SpecContext) { + recoveryOnlyConfig := &config.PluginConfiguration{ + RecoveryBarmanObjectName: "minio-store-recovery", + } + cluster.Status.LatestGeneratedNode = 1 + pod := &corev1.Pod{ + TypeMeta: podTypeMeta, + ObjectMeta: metav1.ObjectMeta{Name: "test-pod"}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "postgres"}}}, + } + podJSON, _ := json.Marshal(pod) + request := &lifecycle.OperatorLifecycleRequest{ + ObjectDefinition: podJSON, + OperationType: &lifecycle.OperatorOperationType{ + Type: lifecycle.OperatorOperationType_TYPE_EVALUATE, + }, + } + + response, err := reconcileInstancePod(ctx, cluster, request, recoveryOnlyConfig, sidecarConfiguration{}) + Expect(err).NotTo(HaveOccurred()) + Expect(response).NotTo(BeNil()) + Expect(response.JsonPatch).NotTo(BeEmpty()) + var patch []map[string]interface{} + Expect(json.Unmarshal(response.JsonPatch, &patch)).To(Succeed()) + Expect(patch).To(ContainElement(HaveKeyWithValue("path", "/spec/initContainers"))) + }) + It("does not mutate the pod when no object store is configured", func(ctx SpecContext) { emptyConfig := &config.PluginConfiguration{} pod := &corev1.Pod{