Add more logging and ignore some mutable fields to only check the init container and main container images

Author: johscheuer

controllers/update_status.go

@@ -1292,7 +1292,7 @@ func podSpecIsCorrect(ctx context.Context,
 
 	// Check if any of the mutable fields of the Pod have been changed outside the operators control, e.g.
 	// by using kubectl.
-	if !mutablePodFieldsAreCorrect(desiredPodSpec, pod.Spec.DeepCopy()) {
+	if !mutablePodFieldsAreCorrect(logger, desiredPodSpec, pod.Spec.DeepCopy()) {
 		logger.Info(
 			"IncorrectPodSpec",
 			"currentSpecHash",
@@ -1332,39 +1332,31 @@ func podSpecIsCorrect(ctx context.Context,
 // 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
 		}
 	}
 
-	if !equality.Semantic.DeepEqual(
-		desiredPodSpec.ActiveDeadlineSeconds,
-		currentPodSpec.ActiveDeadlineSeconds,
-	) {
-		return false
-	}
-
-	if !equality.Semantic.DeepEqual(
-		desiredPodSpec.TerminationGracePeriodSeconds,
-		currentPodSpec.TerminationGracePeriodSeconds,
-	) {
-		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.
+	// 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 equality.Semantic.DeepEqual(desiredPodSpec.Tolerations, currentPodSpec.Tolerations)
+	return true
 }

controllers/update_status_test.go

@@ -1790,34 +1790,6 @@ var _ = Describe("update_status", func() {
 			})
 		})
 
-		When("tolerations have been changed", func() {
-			BeforeEach(func() {
-				pod.Spec.Tolerations = append(
-					pod.Spec.Tolerations,
-					corev1.Toleration{
-						Key:      "test-key",
-						Operator: corev1.TolerationOpEqual,
-						Value:    "test-value",
-						Effect:   corev1.TaintEffectNoSchedule,
-					},
-				)
-				Expect(k8sClient.Update(context.TODO(), pod)).NotTo(HaveOccurred())
-			})
-
-			It("should return false due to toleration 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
@@ -1863,7 +1835,9 @@ var _ = Describe("update_status", func() {
 
 		When("all mutable fields match", func() {
 			It("should return true", func() {
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeTrue())
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeTrue())
 			})
 		})
 
@@ -1873,7 +1847,9 @@ var _ = Describe("update_status", func() {
 			})
 
 			It("should return false", func() {
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeFalse())
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeFalse())
 			})
 		})
 
@@ -1889,7 +1865,9 @@ var _ = Describe("update_status", func() {
 			})
 
 			It("should return false", func() {
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeFalse())
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeFalse())
 			})
 		})
 
@@ -1908,85 +1886,9 @@ var _ = Describe("update_status", func() {
 			})
 
 			It("should return false", func() {
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeFalse())
-			})
-		})
-
-		When("ActiveDeadlineSeconds is changed", func() {
-			BeforeEach(func() {
-				desiredPodSpec.ActiveDeadlineSeconds = ptr.To(int64(3600))
-				currentPodSpec.ActiveDeadlineSeconds = ptr.To(int64(7200))
-			})
-
-			It("should return false", func() {
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeFalse())
-			})
-		})
-
-		When("TerminationGracePeriodSeconds is changed", func() {
-			BeforeEach(func() {
-				desiredPodSpec.TerminationGracePeriodSeconds = ptr.To(int64(30))
-				currentPodSpec.TerminationGracePeriodSeconds = ptr.To(int64(60))
-			})
-
-			It("should return false", func() {
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeFalse())
-			})
-		})
-
-		When("tolerations are added", func() {
-			BeforeEach(func() {
-				currentPodSpec.Tolerations = append(
-					currentPodSpec.Tolerations,
-					corev1.Toleration{
-						Key:      "added-toleration",
-						Operator: corev1.TolerationOpEqual,
-						Value:    "added-value",
-						Effect:   corev1.TaintEffectNoSchedule,
-					},
-				)
-			})
-
-			It("should return false", func() {
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeFalse())
-			})
-		})
-
-		When("tolerations are removed", func() {
-			BeforeEach(func() {
-				desiredPodSpec.Tolerations = []corev1.Toleration{
-					{
-						Key:      "test-toleration",
-						Operator: corev1.TolerationOpEqual,
-						Value:    "test-value",
-						Effect:   corev1.TaintEffectNoSchedule,
-					},
-				}
-				currentPodSpec.Tolerations = []corev1.Toleration{}
-			})
-
-			It("should return false", func() {
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeFalse())
-			})
-		})
-
-		When("tolerations are modified", func() {
-			BeforeEach(func() {
-				toleration := corev1.Toleration{
-					Key:      "test-toleration",
-					Operator: corev1.TolerationOpEqual,
-					Value:    "original-value",
-					Effect:   corev1.TaintEffectNoSchedule,
-				}
-				desiredPodSpec.Tolerations = []corev1.Toleration{toleration}
-
-				modifiedToleration := toleration
-				modifiedToleration.Value = "modified-value"
-				currentPodSpec.Tolerations = []corev1.Toleration{modifiedToleration}
-			})
-
-			It("should return false", func() {
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeFalse())
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeFalse())
 			})
 		})
 
@@ -2004,7 +1906,9 @@ var _ = Describe("update_status", func() {
 			})
 
 			It("should return false", func() {
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeFalse())
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeFalse())
 			})
 		})
 
@@ -2019,7 +1923,9 @@ var _ = Describe("update_status", func() {
 
 			It("should still return true as schedulingGates are ignored", func() {
 				// The method explicitly ignores schedulingGates per the comment in the code
-				Expect(mutablePodFieldsAreCorrect(desiredPodSpec, currentPodSpec)).To(BeTrue())
+				Expect(
+					mutablePodFieldsAreCorrect(testLogger, desiredPodSpec, currentPodSpec),
+				).To(BeTrue())
 			})
 		})
 	})