fix: wrong its ready condition during image in-place update (#8505)

apecloud · Nov 22, 2024 · e5893a4 · e5893a4
1 parent fe24a3a
commit e5893a4
Show file tree

Hide file tree

Showing 4 changed files with 99 additions and 20 deletions.
diff --git a/pkg/controller/instanceset/instance_util.go b/pkg/controller/instanceset/instance_util.go
@@ -168,6 +168,10 @@ func isContainersRunning(pod *corev1.Pod) bool {
 	return true
 }
 
+func isContainersReady(pod *corev1.Pod) bool {
+	return isImageMatched(pod) && isContainersRunning(pod)
+}
+
 // isCreated returns true if pod has been created and is maintained by the API server
 func isCreated(pod *corev1.Pod) bool {
 	return pod.Status.Phase != ""

diff --git a/pkg/controller/instanceset/reconciler_status.go b/pkg/controller/instanceset/reconciler_status.go
@@ -106,7 +106,7 @@ func (r *statusReconciler) Reconcile(tree *kubebuilderx.ObjectTree) (kubebuilder
 			replicas++
 			template2TemplatesStatus[templateName].Replicas++
 		}
-		if isRunningAndReady(pod) && !isTerminating(pod) {
+		if isContainersReady(pod) && isRunningAndReady(pod) && !isTerminating(pod) {
 			readyReplicas++
 			template2TemplatesStatus[templateName].ReadyReplicas++
 			notReadyNames.Delete(pod.Name)

diff --git a/pkg/controller/instanceset/reconciler_status_test.go b/pkg/controller/instanceset/reconciler_status_test.go
@@ -77,6 +77,32 @@ var _ = Describe("status reconciler test", func() {
 			Expect(res).Should(Equal(kubebuilderx.Continue))
 		}
 
+		makePodAvailableWithRevision := func(pod *corev1.Pod, revision string, updatePodAvailable bool) {
+			pod.Labels[appsv1.ControllerRevisionHashLabelKey] = revision
+			pod.Status.Phase = corev1.PodRunning
+			condition := corev1.PodCondition{
+				Type:               corev1.PodReady,
+				Status:             corev1.ConditionTrue,
+				LastTransitionTime: metav1.NewTime(time.Now()),
+			}
+			if updatePodAvailable {
+				condition.LastTransitionTime = metav1.NewTime(time.Now().Add(-1 * minReadySeconds * time.Second))
+			}
+			pod.Status.Conditions = []corev1.PodCondition{condition}
+			pod.Status.ContainerStatuses = nil
+			for _, c := range pod.Spec.Containers {
+				pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{
+					Name:  c.Name,
+					Image: c.Image,
+					State: corev1.ContainerState{
+						Running: &corev1.ContainerStateRunning{
+							StartedAt: metav1.NewTime(time.Now()),
+						},
+					},
+				})
+			}
+		}
+
 		It("should work well", func() {
 			By("PreCondition")
 			its.Generation = 1
@@ -123,19 +149,6 @@ var _ = Describe("status reconciler test", func() {
 			}
 
 			By("make all pods ready with old revision")
-			condition := corev1.PodCondition{
-				Type:               corev1.PodReady,
-				Status:             corev1.ConditionTrue,
-				LastTransitionTime: metav1.NewTime(time.Now()),
-			}
-			makePodAvailableWithRevision := func(pod *corev1.Pod, revision string, updatePodAvailable bool) {
-				pod.Labels[appsv1.ControllerRevisionHashLabelKey] = revision
-				pod.Status.Phase = corev1.PodRunning
-				if updatePodAvailable {
-					condition.LastTransitionTime = metav1.NewTime(time.Now().Add(-1 * minReadySeconds * time.Second))
-				}
-				pod.Status.Conditions = []corev1.PodCondition{condition}
-			}
 			pods := tree.List(&corev1.Pod{})
 			currentRevisionMap := map[string]string{}
 			oldRevision := "old-revision"
@@ -251,6 +264,72 @@ var _ = Describe("status reconciler test", func() {
 			Expect(its.Status.Conditions[2].Message).Should(BeEquivalentTo(message))
 		})
 
+		It("updates image in-place", func() {
+			tree := kubebuilderx.NewObjectTree()
+			tree.SetRoot(its)
+			its.Spec.PodManagementPolicy = appsv1.ParallelPodManagement
+			reconcilePods(tree)
+			reconciler = NewStatusReconciler()
+			Expect(reconciler.PreCondition(tree)).Should(Equal(kubebuilderx.ConditionSatisfied))
+
+			By("make all pods available")
+			pods := tree.List(&corev1.Pod{})
+			updateRevisions, err := GetRevisions(its.Status.UpdateRevisions)
+			Expect(err).Should(BeNil())
+			for _, object := range pods {
+				pod, ok := object.(*corev1.Pod)
+				Expect(ok).Should(BeTrue())
+				makePodAvailableWithRevision(pod, updateRevisions[pod.Name], true)
+			}
+			res, err := reconciler.Reconcile(tree)
+			Expect(err).Should(BeNil())
+			Expect(res).Should(Equal(kubebuilderx.Continue))
+			replicas := *its.Spec.Replicas
+			Expect(its.Status.Replicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.ReadyReplicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.AvailableReplicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.UpdatedReplicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.CurrentReplicas).Should(BeEquivalentTo(replicas))
+
+			By("update image")
+			newImage := "bar-new"
+			its.Spec.Template.Spec.Containers[0].Image = newImage
+			for _, object := range pods {
+				pod, ok := object.(*corev1.Pod)
+				Expect(ok).Should(BeTrue())
+				pod.Spec.Containers[0].Image = newImage
+			}
+			res, err = reconciler.Reconcile(tree)
+			Expect(err).Should(BeNil())
+			Expect(res).Should(Equal(kubebuilderx.Continue))
+			Expect(its.Status.Replicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.ReadyReplicas).Should(BeEquivalentTo(0))
+			Expect(its.Status.AvailableReplicas).Should(BeEquivalentTo(0))
+			Expect(its.Status.UpdatedReplicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.CurrentReplicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.Conditions).Should(HaveLen(2))
+			Expect(its.Status.Conditions[0].Type).Should(BeEquivalentTo(workloads.InstanceReady))
+			Expect(its.Status.Conditions[0].Status).Should(BeEquivalentTo(corev1.ConditionFalse))
+
+			By("make all containers of pod ready")
+			for _, object := range pods {
+				pod, ok := object.(*corev1.Pod)
+				Expect(ok).Should(BeTrue())
+				makePodAvailableWithRevision(pod, updateRevisions[pod.Name], true)
+			}
+			res, err = reconciler.Reconcile(tree)
+			Expect(err).Should(BeNil())
+			Expect(res).Should(Equal(kubebuilderx.Continue))
+			Expect(its.Status.Replicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.ReadyReplicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.AvailableReplicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.UpdatedReplicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.CurrentReplicas).Should(BeEquivalentTo(replicas))
+			Expect(its.Status.Conditions).Should(HaveLen(2))
+			Expect(its.Status.Conditions[0].Type).Should(BeEquivalentTo(workloads.InstanceReady))
+			Expect(its.Status.Conditions[0].Status).Should(BeEquivalentTo(corev1.ConditionTrue))
+		})
+
 		It("updates nodeSelectorOnce Annotation", func() {
 			tree := kubebuilderx.NewObjectTree()
 			tree.SetRoot(its)

diff --git a/pkg/controller/instanceset/reconciler_update.go b/pkg/controller/instanceset/reconciler_update.go
@@ -137,12 +137,8 @@ func (r *updateReconciler) Reconcile(tree *kubebuilderx.ObjectTree) (kubebuilder
 			break
 		}
 
-		if !isImageMatched(pod) {
-			tree.Logger.Info(fmt.Sprintf("InstanceSet %s/%s blocks on update as the image(s) of pod %s is not matched", its.Namespace, its.Name, pod.Name))
-			break
-		}
-		if !isContainersRunning(pod) {
-			tree.Logger.Info(fmt.Sprintf("InstanceSet %s/%s blocks on update as some the container(s) of pod %s are not started at least for %d seconds", its.Namespace, its.Name, pod.Name, its.Spec.MinReadySeconds))
+		if !isContainersReady(pod) {
+			tree.Logger.Info(fmt.Sprintf("InstanceSet %s/%s blocks on update as some the container(s) of pod %s are not ready", its.Namespace, its.Name, pod.Name))
 			// as no further event triggers the next reconciliation, we need a retry
 			needRetry = true
 			break