From bb0d21794101ae09e10969167eb430a2aa1eb104 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Mon, 27 Jul 2026 17:47:05 +0200 Subject: [PATCH] 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{