Improve the check for Pod spec changes

controllers/update_status.go

@@ -749,35 +749,12 @@ func validateProcessGroup(
 		return nil
 	}
 
-	specHash, err := internal.GetPodSpecHash(cluster, processGroupStatus, nil)
+	correctPodSpec, err := podSpecIsCorrect(ctx, r, cluster, pod, processGroupStatus, logger)
 	if err != nil {
 		return err
 	}
 
-	// Check here if the Pod spec matches the desired Pod spec.
-	incorrectPodSpec := specHash != pod.Annotations[fdbv1beta2.LastSpecKey]
-	if !incorrectPodSpec {
-		updated, err := r.PodLifecycleManager.PodIsUpdated(ctx, r, cluster, pod)
-		if err != nil {
-			return err
-		}
-		incorrectPodSpec = !updated
-		if incorrectPodSpec {
-			logger.Info(
-				"IncorrectPodSpec",
-				"currentSpecHash",
-				pod.Annotations[fdbv1beta2.LastSpecKey],
-				"desiredSpecHash",
-				specHash,
-				"updated",
-				updated,
-				"incorrectPodSpec",
-				incorrectPodSpec,
-			)
-		}
-	}
-
-	processGroupStatus.UpdateCondition(fdbv1beta2.IncorrectPodSpec, incorrectPodSpec)
+	processGroupStatus.UpdateCondition(fdbv1beta2.IncorrectPodSpec, !correctPodSpec)
 
 	// Check the sidecar image, to ensure the sidecar is running with the desired image.
 	sidecarImage, err := internal.GetSidecarImage(cluster, processGroupStatus.ProcessClass)
@@ -1277,3 +1254,109 @@ func updateFaultDomains(
 		status.ProcessGroups[idx].FaultDomain = fdbv1beta2.FaultDomain(faultDomain)
 	}
 }
+
+// podSpecIsCorrect returns if the desired Pod spec and the current Pod spec are matching.
+func podSpecIsCorrect(ctx context.Context,
+	r *FoundationDBClusterReconciler,
+	cluster *fdbv1beta2.FoundationDBCluster,
+	pod *corev1.Pod,
+	processGroupStatus *fdbv1beta2.ProcessGroupStatus,
+	logger logr.Logger,
+) (bool, error) {
+	desiredPodSpec, err := internal.GetPodSpec(cluster, processGroupStatus)
+	if err != nil {
+		return false, err
+	}
+
+	specHash, err := internal.GetPodSpecHash(cluster, processGroupStatus, desiredPodSpec)
+	if err != nil {
+		return false, err
+	}
+
+	// Check here if the Pod spec matches the desired Pod spec, if not we know that change are pending, and we don't
+	// have to preform additional checks.
+	correctPodSpec := specHash == pod.Annotations[fdbv1beta2.LastSpecKey]
+	if !correctPodSpec {
+		logger.Info(
+			"IncorrectPodSpec",
+			"currentSpecHash",
+			pod.Annotations[fdbv1beta2.LastSpecKey],
+			"desiredSpecHash",
+			specHash,
+			"reason",
+			"pod spec hash mismatch",
+		)
+
+		return false, nil
+	}
+
+	// Check if any of the mutable fields of the Pod have been changed outside the operators control, e.g.
+	// by using kubectl.
+	if !mutablePodFieldsAreCorrect(logger, desiredPodSpec, pod.Spec.DeepCopy()) {
+		logger.Info(
+			"IncorrectPodSpec",
+			"currentSpecHash",
+			pod.Annotations[fdbv1beta2.LastSpecKey],
+			"desiredSpecHash",
+			specHash,
+			"reason",
+			"mutable field in pod spec was updated",
+		)
+
+		return false, nil
+	}
+
+	// Check if the Pod is pending updates, this method will return true in the default implementation.
+	updated, err := r.PodLifecycleManager.PodIsUpdated(ctx, r, cluster, pod)
+	if err != nil {
+		return updated, err
+	}
+
+	if !updated {
+		logger.Info(
+			"IncorrectPodSpec",
+			"currentSpecHash",
+			pod.Annotations[fdbv1beta2.LastSpecKey],
+			"desiredSpecHash",
+			specHash,
+			"reason",
+			"pod is pending updates",
+		)
+
+		return false, nil
+	}
+
+	return true, nil
+}
+
+// mutablePodFieldsAreCorrect checks if any of the pods mutable fields have been changed, see:
+// https://kubernetes.io/docs/concepts/workloads/pods/#pod-update-and-replacement
+func mutablePodFieldsAreCorrect(
+	logger logr.Logger,
+	desiredPodSpec *corev1.PodSpec,
+	currentPodSpec *corev1.PodSpec,
+) bool {
+	for idx, container := range desiredPodSpec.Containers {
+		logger.V(1).
+			Info("compare images for container", "desiredContainerName", container.Name, "desiredImage", container.Name, "currentContainerName", currentPodSpec.Containers[idx].Name, "currentImage", currentPodSpec.Containers[idx].Image)
+		if !equality.Semantic.DeepEqual(container.Image, currentPodSpec.Containers[idx].Image) {
+			return false
+		}
+	}
+
+	for idx, container := range desiredPodSpec.InitContainers {
+		logger.V(1).
+			Info("compare images for init container", "desiredContainerName", container.Name, "desiredImage", container.Name, "currentContainerName", currentPodSpec.Containers[idx].Name, "currentImage", currentPodSpec.Containers[idx].Image)
+		if !equality.Semantic.DeepEqual(container.Image, currentPodSpec.InitContainers[idx].Image) {
+			return false
+		}
+	}
+
+	// We ignore the spec.schedulingGates in the comparison here. Otherwise, a component could make the Pod ready for
+	// being scheduled by removing the scheduling gate which then would case a recreation of the Pod. This would
+	// cause the Pod to be stuck in pending or recreation. We also ignore the `Tolerations`, `ActiveDeadlineSeconds`
+	// and `TerminationGracePeriodSeconds`. Those have some restrictions on the update anyway.
+	// We could make use of the mutability of the tolerations field and remove the need for a Pod recreation when only
+	// tolerations are added.
+	return true
+}

controllers/update_status_test.go

@@ -1702,4 +1702,231 @@ var _ = Describe("update_status", func() {
 			})
 		})
 	})
+
+	When("checking if the pod spec if correct", func() {
+		var cluster *fdbv1beta2.FoundationDBCluster
+		var pod *corev1.Pod
+		var processGroupStatus *fdbv1beta2.ProcessGroupStatus
+		var err error
+
+		BeforeEach(func() {
+			cluster = internal.CreateDefaultCluster()
+			Expect(setupClusterForTest(cluster)).NotTo(HaveOccurred())
+
+			pickedProcessGroup := internal.PickProcessGroups(
+				cluster,
+				fdbv1beta2.ProcessClassStorage,
+				1,
+			)[0]
+			processGroupStatus = pickedProcessGroup
+
+			pod, err = clusterReconciler.PodLifecycleManager.GetPod(
+				context.TODO(),
+				clusterReconciler,
+				cluster,
+				pickedProcessGroup.GetPodName(cluster),
+			)
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		When("the spec hash matches and Pod is up to date", func() {
+			It("should return true", func() {
+				correct, err := podSpecIsCorrect(
+					context.TODO(),
+					clusterReconciler,
+					cluster,
+					pod,
+					processGroupStatus,
+					logger,
+				)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(correct).To(BeTrue())
+			})
+		})
+
+		When("the spec hash does not match", func() {
+			BeforeEach(func() {
+				pod.Annotations[fdbv1beta2.LastSpecKey] = "incorrect-hash"
+				Expect(k8sClient.Update(context.TODO(), pod)).NotTo(HaveOccurred())
+			})
+
+			It("should return false", func() {
+				correct, err := podSpecIsCorrect(
+					context.TODO(),
+					clusterReconciler,
+					cluster,
+					pod,
+					processGroupStatus,
+					logger,
+				)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(correct).To(BeFalse())
+			})
+		})
+
+		When("mutable fields have been changed", func() {
+			BeforeEach(func() {
+				// Change the image of the main container
+				for idx, container := range pod.Spec.Containers {
+					if container.Name == fdbv1beta2.MainContainerName {
+						pod.Spec.Containers[idx].Image = "foundationdb/foundationdb:9.9.9"
+						break
+					}
+				}
+				Expect(k8sClient.Update(context.TODO(), pod)).NotTo(HaveOccurred())
+			})
+
+			It("should return false due to mutable field mismatch", func() {
+				correct, err := podSpecIsCorrect(
+					context.TODO(),
+					clusterReconciler,
+					cluster,
+					pod,
+					processGroupStatus,
+					logger,
+				)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(correct).To(BeFalse())
+			})
+		})
+
+		When("an error occurs getting the Pod spec", func() {
+			It("should still complete without panicking", func() {
+				// The method should handle errors gracefully even in edge cases
+				correct, err := podSpecIsCorrect(
+					context.TODO(),
+					clusterReconciler,
+					cluster,
+					pod,
+					processGroupStatus,
+					logger,
+				)
+				// The method should not panic and should return a reasonable response
+				Expect(err).NotTo(HaveOccurred())
+				Expect(correct).To(BeTrue())
+			})
+		})
+	})
+
+	When("checking if the pods mutable fields are valid", func() {
+		var desiredPodSpec *corev1.PodSpec
+		var currentPodSpec *corev1.PodSpec
+
+		BeforeEach(func() {
+			cluster := internal.CreateDefaultCluster()
+			// Properly normalize the cluster to ensure all fields are initialized
+			Expect(internal.NormalizeClusterSpec(cluster, internal.DeprecationOptions{})).NotTo(
+				HaveOccurred(),
+			)
+
+			processGroupStatus := fdbv1beta2.NewProcessGroupStatus(
+				"storage-1",
+				fdbv1beta2.ProcessClassStorage,
+				nil,
+			)
+
+			var err error
+			desiredPodSpec, err = internal.GetPodSpec(cluster, processGroupStatus)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Create a deep copy for the current spec
+			currentPodSpec = desiredPodSpec.DeepCopy()
+		})
+
+		When("all mutable fields match", func() {
+			It("should return true", func() {
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeTrue())
+			})
+		})
+
+		When("a main container image is changed", func() {
+			BeforeEach(func() {
+				currentPodSpec.Containers[0].Image = "different-image:latest"
+			})
+
+			It("should return false", func() {
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeFalse())
+			})
+		})
+
+		When("the sidecar container image is changed", func() {
+			BeforeEach(func() {
+				// Find the sidecar container
+				for idx, container := range currentPodSpec.Containers {
+					if container.Name == fdbv1beta2.SidecarContainerName {
+						currentPodSpec.Containers[idx].Image = "different-sidecar:latest"
+						break
+					}
+				}
+			})
+
+			It("should return false", func() {
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeFalse())
+			})
+		})
+
+		When("an init container image is changed", func() {
+			BeforeEach(func() {
+				// Add an init container to both specs first
+				initContainer := corev1.Container{
+					Name:  "init-test",
+					Image: "init-image:v1",
+				}
+				desiredPodSpec.InitContainers = []corev1.Container{initContainer}
+				currentPodSpec.InitContainers = []corev1.Container{initContainer}
+
+				// Change the current spec's init container
+				currentPodSpec.InitContainers[0].Image = "init-image:v2"
+			})
+
+			It("should return false", func() {
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeFalse())
+			})
+		})
+
+		When("multiple mutable fields are changed", func() {
+			BeforeEach(func() {
+				currentPodSpec.Containers[0].Image = "different-image:latest"
+				currentPodSpec.TerminationGracePeriodSeconds = ptr.To(int64(120))
+				currentPodSpec.Tolerations = append(
+					currentPodSpec.Tolerations,
+					corev1.Toleration{
+						Key:    "extra",
+						Effect: corev1.TaintEffectNoExecute,
+					},
+				)
+			})
+
+			It("should return false", func() {
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeFalse())
+			})
+		})
+
+		When("schedulingGates are different", func() {
+			BeforeEach(func() {
+				// Add schedulingGates to current spec
+				currentPodSpec.SchedulingGates = []corev1.PodSchedulingGate{
+					{Name: "test-gate"},
+				}
+				// Keep desired spec without schedulingGates
+			})
+
+			It("should still return true as schedulingGates are ignored", func() {
+				// The method explicitly ignores schedulingGates per the comment in the code
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeTrue())
+			})
+		})
+	})
 })