Merge e95b91dee9 into 76d4f22aa3

A cluster bootstrapped only via RecoveryBarmanObjectName, with no
continuing archiving or replica source, only ever needs the sidecar
for its one-time bootstrap restore. Gate its injection on
cluster.IsInitialized() so it stops being added once the cluster is
up.

This makes the operator's own drift-check see the already-running
pod's spec as outdated right after initialization completes, and roll
it out to drop the sidecar. That is deliberately accepted rather than
engineered around: it is one deterministic rollout using the same
machinery the operator already uses for every other pod-spec change
(a switchover if a replica is available, an in-place restart
otherwise), not a new or fragile risk.

Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>

internal/cnpgi/operator/lifecycle.go

@@ -330,26 +330,18 @@ func (impl LifecycleImplementation) collectAdditionalInstanceArgs(
 // 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.
+// it's gated on cluster.IsInitialized() instead.
 //
-// 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.
+// Once the cluster finishes initializing, this makes the operator's own
+// drift-check (checkPodSpecIsOutdated) see the running pod's spec as outdated
+// and roll it out to drop the sidecar. That's deliberately accepted rather
+// than engineered around: it's one deterministic rollout using the same
+// machinery the operator already uses for every other pod-spec change (a
+// switchover if a replica is available, an in-place restart otherwise), not a
+// new or fragile risk.
 func shouldInjectBarmanSidecar(
 	cluster *cnpgv1.Cluster,
 	pluginConfiguration *config.PluginConfiguration,
-	request *lifecycle.OperatorLifecycleRequest,
 ) bool {
 	if len(pluginConfiguration.BarmanObjectName) != 0 || len(pluginConfiguration.ReplicaSourceBarmanObjectName) != 0 {
 		return true
@@ -359,11 +351,7 @@ func shouldInjectBarmanSidecar(
 		return false
 	}
 
-	if request.GetOperationType().GetType() == lifecycle.OperatorOperationType_TYPE_CREATE {
-		return !cluster.IsInitialized()
-	}
-
-	return true
+	return !cluster.IsInitialized()
 }
 
 func reconcileInstancePod(
@@ -383,7 +371,7 @@ func reconcileInstancePod(
 
 	mutatedPod := pod.DeepCopy()
 
-	if shouldInjectBarmanSidecar(cluster, pluginConfiguration, request) {
+	if shouldInjectBarmanSidecar(cluster, pluginConfiguration) {
 		if err := reconcilePodSpec(
 			cluster,
 			&mutatedPod.Spec,

internal/cnpgi/operator/lifecycle_test.go

@@ -265,8 +265,8 @@ 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) {
+		It("does not inject the sidecar for a recovery-only cluster that has "+
+			"already completed its initial bootstrap", func(ctx SpecContext) {
 			recoveryOnlyConfig := &config.PluginConfiguration{
 				RecoveryBarmanObjectName: "minio-store-recovery",
 			}
@@ -279,9 +279,6 @@ var _ = Describe("LifecycleImplementation", func() {
 			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{})
@@ -290,35 +287,6 @@ var _ = Describe("LifecycleImplementation", func() {
 			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{