From e5893a4995094ba250975a2c0308b6112d421ef3 Mon Sep 17 00:00:00 2001 From: free6om Date: Fri, 22 Nov 2024 11:39:56 +0800 Subject: [PATCH] fix: wrong its ready condition during image in-place update (#8505) --- pkg/controller/instanceset/instance_util.go | 4 + .../instanceset/reconciler_status.go | 2 +- .../instanceset/reconciler_status_test.go | 105 +++++++++++++++--- .../instanceset/reconciler_update.go | 8 +- 4 files changed, 99 insertions(+), 20 deletions(-) diff --git a/pkg/controller/instanceset/instance_util.go b/pkg/controller/instanceset/instance_util.go index 7ab2df9a7e8..f1c793aee0c 100644 --- 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 index 32b4e27054b..168a30b3cc5 100644 --- 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 index ddb2f652725..928c6a0d2ef 100644 --- 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 index fbc0ff666dd..7e8c453063e 100644 --- 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