diff --git a/controllers/update_status.go b/controllers/update_status.go index 21bf06552..927dc26e4 100644 --- a/controllers/update_status.go +++ b/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 +} diff --git a/controllers/update_status_test.go b/controllers/update_status_test.go index ab4f7466a..d8915c467 100644 --- a/controllers/update_status_test.go +++ b/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()) + }) + }) + }) }) diff --git a/e2e/test_operator/operator_test.go b/e2e/test_operator/operator_test.go index b198331a2..b7d4889ec 100644 --- a/e2e/test_operator/operator_test.go +++ b/e2e/test_operator/operator_test.go @@ -2587,4 +2587,66 @@ var _ = Describe("Operator", Label("e2e", "pr"), func() { }) }, ) + + When("the main container image of a pod is changed outside of the operator", func() { + var initialReplacementDuration time.Duration + var targetPod *corev1.Pod + badImage := "broken" + + BeforeEach(func() { + availabilityCheck = false + + // Disable automatic replacements. + initialReplacementDuration = time.Duration( + fdbCluster.GetCachedCluster().GetFailureDetectionTimeSeconds(), + ) * time.Second + + Expect( + fdbCluster.SetAutoReplacementsWithWait(false, 10*time.Hour, false), + ).NotTo(HaveOccurred()) + + targetPod = ptr.To(factory.RandomPickOnePod(fdbCluster.GetStatelessPods().Items)) + for idx, container := range targetPod.Spec.Containers { + if container.Name != fdbv1beta2.MainContainerName { + continue + } + + targetPod.Spec.Containers[idx].Image = badImage + break + } + + Expect( + factory.GetControllerRuntimeClient().Update(context.Background(), targetPod), + ).To(Succeed()) + }) + + AfterEach(func() { + Expect( + fdbCluster.SetAutoReplacements(true, initialReplacementDuration), + ).NotTo(HaveOccurred()) + }) + + It("should recreate the Pod with the correct image", + func() { + Eventually(func(g Gomega) { + currentPod := &corev1.Pod{} + g.Expect(factory.GetControllerRuntimeClient().Get(context.Background(), ctrlClient.ObjectKeyFromObject(targetPod), currentPod)). + To(Succeed()) + + var checkedMainImage bool + for _, container := range currentPod.Spec.Containers { + if container.Name != fdbv1beta2.MainContainerName { + continue + } + + g.Expect(container.Image).NotTo(Equal(badImage)) + checkedMainImage = true + break + } + + Expect(checkedMainImage).To(BeTrue()) + }).To(Succeed()) + }, + ) + }) })