From 81b5d527fe4b4efadbfcccfe41cb2fd3f90806ec Mon Sep 17 00:00:00 2001 From: berg Date: Mon, 25 Sep 2023 11:05:23 +0800 Subject: [PATCH] fix sidecarset ExpectUpdated block upgrade container (#1424) Signed-off-by: liheng.zms --- pkg/control/pubcontrol/pub_control.go | 1 - pkg/control/sidecarcontrol/api.go | 4 ++- .../sidecarcontrol/sidecarset_control.go | 12 +++---- .../sidecarset/sidecarset_processor.go | 3 +- .../sidecarset/sidecarset_strategy.go | 5 +-- test/e2e/apps/sidecarset.go | 32 ++++++++++++++++--- test/e2e/framework/sidecarset_utils.go | 18 ++++++++++- 7 files changed, 58 insertions(+), 17 deletions(-) diff --git a/pkg/control/pubcontrol/pub_control.go b/pkg/control/pubcontrol/pub_control.go index ec9823ded9..7f2da9f871 100644 --- a/pkg/control/pubcontrol/pub_control.go +++ b/pkg/control/pubcontrol/pub_control.go @@ -44,7 +44,6 @@ type commonControl struct { } func (c *commonControl) IsPodReady(pod *corev1.Pod) bool { - klog.Infof("IsPodReady pod(%s)", util.DumpJSON(pod)) // 1. pod.Status.Phase == v1.PodRunning // 2. pod.condition PodReady == true if !util.IsRunningAndReady(pod) { diff --git a/pkg/control/sidecarcontrol/api.go b/pkg/control/sidecarcontrol/api.go index f0b5d3823f..a2e54e439d 100644 --- a/pkg/control/sidecarcontrol/api.go +++ b/pkg/control/sidecarcontrol/api.go @@ -63,7 +63,9 @@ type SidecarControl interface { // In Kubernetes native scenarios, only Container Image upgrades are allowed // When modifying other fields of the container, e.g. volumemounts, the sidecarSet will not depart to upgrade the sidecar container logic in-place, // and needs to be done by rebuilding the pod - IsSidecarSetUpgradable(pod *v1.Pod) bool + // consistent indicates pod.spec and pod.status is consistent, + // when pod.spec.image is v2 and pod.status.image is v1, then it is inconsistent. + IsSidecarSetUpgradable(pod *v1.Pod) (canUpgrade, consistent bool) } func New(cs *appsv1alpha1.SidecarSet) SidecarControl { diff --git a/pkg/control/sidecarcontrol/sidecarset_control.go b/pkg/control/sidecarcontrol/sidecarset_control.go index 394571894f..7e09f79bf3 100644 --- a/pkg/control/sidecarcontrol/sidecarset_control.go +++ b/pkg/control/sidecarcontrol/sidecarset_control.go @@ -181,12 +181,12 @@ func (c *commonControl) IsPodStateConsistent(pod *v1.Pod, sidecarContainers sets return IsSidecarContainerUpdateCompleted(pod, sets.NewString(sidecarset.Name), sidecarContainers) } -// k8s only allow modify pod.spec.container[x].image, -// only when annotations[SidecarSetHashWithoutImageAnnotation] is the same, sidecarSet can upgrade pods -func (c *commonControl) IsSidecarSetUpgradable(pod *v1.Pod) bool { +func (c *commonControl) IsSidecarSetUpgradable(pod *v1.Pod) (canUpgrade, consistent bool) { sidecarSet := c.GetSidecarset() + // k8s only allow modify pod.spec.container[x].image, + // only when annotations[SidecarSetHashWithoutImageAnnotation] is the same, sidecarSet can upgrade pods if GetPodSidecarSetWithoutImageRevision(sidecarSet.Name, pod) != GetSidecarSetWithoutImageRevision(sidecarSet) { - return false + return false, false } // cStatus: container.name -> containerStatus.Ready @@ -200,11 +200,11 @@ func (c *commonControl) IsSidecarSetUpgradable(pod *v1.Pod) bool { // indicates that sidecar container is in the process of being upgraded // wait for the last upgrade to complete before performing this upgrade if cStatus[sidecar] && !c.IsPodStateConsistent(pod, sets.NewString(sidecar)) { - return false + return true, false } } - return true + return true, true } func (c *commonControl) IsPodAvailabilityChanged(pod, oldPod *v1.Pod) bool { diff --git a/pkg/controller/sidecarset/sidecarset_processor.go b/pkg/controller/sidecarset/sidecarset_processor.go index da3e06c9ec..3aa3592870 100644 --- a/pkg/controller/sidecarset/sidecarset_processor.go +++ b/pkg/controller/sidecarset/sidecarset_processor.go @@ -163,7 +163,8 @@ func (p *Processor) updatePods(control sidecarcontrol.SidecarControl, pods []*co klog.Errorf("update NotUpgradable PodCondition error, s:%s, pod:%s, err:%v", sidecarset.Name, pod.Name, err) return err } - sidecarcontrol.UpdateExpectations.ExpectUpdated(sidecarset.Name, sidecarcontrol.GetSidecarSetRevision(sidecarset), pod) + // Since the pod sidecarSet hash is not updated here, it cannot be called ExpectUpdated + // TODO: add ResourceVersionExpectation instead of UpdateExpectations } if len(notUpgradablePods) > 0 { p.recorder.Eventf(sidecarset, corev1.EventTypeNormal, "NotUpgradablePods", "SidecarSet in-place update detected %d not upgradable pod(s) in this round, will skip them.", len(notUpgradablePods)) diff --git a/pkg/controller/sidecarset/sidecarset_strategy.go b/pkg/controller/sidecarset/sidecarset_strategy.go index 8b998c6d94..11e037d0d7 100644 --- a/pkg/controller/sidecarset/sidecarset_strategy.go +++ b/pkg/controller/sidecarset/sidecarset_strategy.go @@ -72,9 +72,10 @@ func (p *spreadingStrategy) GetNextUpgradePods(control sidecarcontrol.SidecarCon for index, pod := range pods { isUpdated := sidecarcontrol.IsPodSidecarUpdated(sidecarset, pod) if !isUpdated && isSelected(pod) { - if control.IsSidecarSetUpgradable(pod) { + canUpgrade, consistent := control.IsSidecarSetUpgradable(pod) + if canUpgrade && consistent { waitUpgradedIndexes = append(waitUpgradedIndexes, index) - } else if sidecarcontrol.GetPodSidecarSetWithoutImageRevision(sidecarset.Name, pod) != sidecarcontrol.GetSidecarSetWithoutImageRevision(sidecarset) { + } else if !canUpgrade { // only image field can be in-place updated, if other fields changed, mark pod as not upgradable notUpgradableIndexes = append(notUpgradableIndexes, index) } diff --git a/test/e2e/apps/sidecarset.go b/test/e2e/apps/sidecarset.go index 550d757b1e..4aed6fff0f 100644 --- a/test/e2e/apps/sidecarset.go +++ b/test/e2e/apps/sidecarset.go @@ -842,14 +842,15 @@ var _ = SIGDescribe("SidecarSet", func() { sidecarSetIn.Spec.UpdateStrategy = appsv1alpha1.SidecarSetUpdateStrategy{ Type: appsv1alpha1.RollingUpdateSidecarSetStrategyType, } + sidecarSetIn.Spec.InitContainers = nil sidecarSetIn.Spec.Containers = sidecarSetIn.Spec.Containers[:1] - ginkgo.By(fmt.Sprintf("Creating SidecarSet %s", sidecarSetIn.Name)) + ginkgo.By(fmt.Sprintf("Creating SidecarSet %s", util.DumpJSON(sidecarSetIn))) sidecarSetIn, _ = tester.CreateSidecarSet(sidecarSetIn) time.Sleep(time.Second) // create deployment deploymentIn := tester.NewBaseDeployment(ns) - deploymentIn.Spec.Replicas = utilpointer.Int32Ptr(2) + deploymentIn.Spec.Replicas = utilpointer.Int32Ptr(1) ginkgo.By(fmt.Sprintf("Creating Deployment(%s/%s)", deploymentIn.Namespace, deploymentIn.Name)) tester.CreateDeployment(deploymentIn) @@ -878,13 +879,18 @@ var _ = SIGDescribe("SidecarSet", func() { } // modify sidecarSet sidecar field out of image - sidecarSetIn.Spec.Containers[0].Command = []string{"sleep", "1000"} + sidecarSetIn.Spec.Containers[0].Env = []corev1.EnvVar{ + { + Name: "version", + Value: "v2", + }, + } tester.UpdateSidecarSet(sidecarSetIn) except := &appsv1alpha1.SidecarSetStatus{ - MatchedPods: 2, + MatchedPods: 1, UpdatedPods: 0, UpdatedReadyPods: 0, - ReadyPods: 2, + ReadyPods: 1, } tester.WaitForSidecarSetUpgradeComplete(sidecarSetIn, except) @@ -896,6 +902,22 @@ var _ = SIGDescribe("SidecarSet", func() { gomega.Expect(condition.Status).Should(gomega.Equal(corev1.ConditionFalse)) } + // scale deployment replicas=2 + deploymentIn.Spec.Replicas = utilpointer.Int32Ptr(2) + tester.UpdateDeployment(deploymentIn) + time.Sleep(time.Second) + + // update sidecarSet image + sidecarSetIn.Spec.Containers[0].Image = NewNginxImage + tester.UpdateSidecarSet(sidecarSetIn) + time.Sleep(time.Second * 3) + except = &appsv1alpha1.SidecarSetStatus{ + MatchedPods: 2, + UpdatedPods: 1, + UpdatedReadyPods: 1, + ReadyPods: 2, + } + tester.WaitForSidecarSetUpgradeComplete(sidecarSetIn, except) ginkgo.By("sidecarSet upgrade sidecar container (more than image field), no pod should be updated done") }) diff --git a/test/e2e/framework/sidecarset_utils.go b/test/e2e/framework/sidecarset_utils.go index ec38759c24..c8f3f0a6ef 100644 --- a/test/e2e/framework/sidecarset_utils.go +++ b/test/e2e/framework/sidecarset_utils.go @@ -165,6 +165,21 @@ func (s *SidecarSetTester) UpdateSidecarSet(sidecarSet *appsv1alpha1.SidecarSet) gomega.Expect(err).NotTo(gomega.HaveOccurred()) } +func (s *SidecarSetTester) UpdateDeployment(obj *apps.Deployment) { + objClone, _ := s.c.AppsV1().Deployments(obj.Namespace).Get(context.TODO(), obj.Name, metav1.GetOptions{}) + err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { + objClone.Spec = obj.Spec + _, updateErr := s.c.AppsV1().Deployments(obj.Namespace).Update(context.TODO(), objClone, metav1.UpdateOptions{}) + if updateErr == nil { + return nil + } + objClone, _ = s.c.AppsV1().Deployments(obj.Namespace).Get(context.TODO(), obj.Name, metav1.GetOptions{}) + return updateErr + }) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + s.WaitForDeploymentRunning(obj) +} + func (s *SidecarSetTester) UpdatePod(pod *corev1.Pod) { Logf("update pod(%s/%s)", pod.Namespace, pod.Name) podClone := pod.DeepCopy() @@ -273,7 +288,8 @@ func (s *SidecarSetTester) WaitForDeploymentRunning(deployment *apps.Deployment) if err != nil { return false, nil } - if *inner.Spec.Replicas == inner.Status.ReadyReplicas { + if inner.Status.ObservedGeneration == inner.Generation && *inner.Spec.Replicas == inner.Status.UpdatedReplicas && + *inner.Spec.Replicas == inner.Status.ReadyReplicas && *inner.Spec.Replicas == inner.Status.Replicas { return true, nil } return false, nil