fix sidecarset ExpectUpdated block upgrade container (openkruise#1424)

Signed-off-by: liheng.zms <[email protected]>

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) {

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 {

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 {

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))

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)
 			}

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")
 		})
 

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