only update condition to true when all sidecarset upgradable (#1272)

Signed-off-by: MarkLux <[email protected]>
openkruise · Jul 5, 2023 · 6a01d5a · 6a01d5a
1 parent 7d81a0e
commit 6a01d5a
Show file tree

Hide file tree

Showing 3 changed files with 167 additions and 36 deletions.
diff --git a/pkg/controller/sidecarset/sidecarset_processor.go b/pkg/controller/sidecarset/sidecarset_processor.go
@@ -27,6 +27,7 @@ import (
 
 	appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
 	"github.com/openkruise/kruise/pkg/control/sidecarcontrol"
+	controlutil "github.com/openkruise/kruise/pkg/controller/util"
 	"github.com/openkruise/kruise/pkg/util"
 	utilclient "github.com/openkruise/kruise/pkg/util/client"
 	historyutil "github.com/openkruise/kruise/pkg/util/history"
@@ -158,7 +159,7 @@ func (p *Processor) updatePods(control sidecarcontrol.SidecarControl, pods []*co
 	// compute next updated pods based on the sidecarset upgrade strategy
 	upgradePods, notUpgradablePods := NewStrategy().GetNextUpgradePods(control, pods)
 	for _, pod := range notUpgradablePods {
-		if err := p.updateNotUpgradablePodCondition(sidecarset, pod); err != nil {
+		if err := p.updatePodSidecarSetUpgradableCondition(sidecarset, pod, false); err != nil {
 			klog.Errorf("update NotUpgradable PodCondition error, s:%s, pod:%s, err:%v", sidecarset.Name, pod.Name, err)
 			return err
 		}
@@ -217,19 +218,8 @@ func (p *Processor) updatePodSidecarAndHash(control sidecarcontrol.SidecarContro
 		return err
 	}
 
-	// patch SidecarSetUpgradable condition
-	if conditionChanged := podutil.UpdatePodCondition(&podClone.Status, &corev1.PodCondition{
-		Type:   sidecarcontrol.SidecarSetUpgradable,
-		Status: corev1.ConditionTrue,
-	}); !conditionChanged {
-		// reduce unnecessary patch.
-		return nil
-	}
-
-	_, condition := podutil.GetPodCondition(&podClone.Status, sidecarcontrol.SidecarSetUpgradable)
-	mergePatch := fmt.Sprintf(`{"status": {"conditions": [%s]}}`, util.DumpJSON(condition))
-	err = p.Client.Status().Patch(context.TODO(), podClone, client.RawPatch(types.StrategicMergePatchType, []byte(mergePatch)))
-	return err
+	// update pod condition of sidecar upgradable
+	return p.updatePodSidecarSetUpgradableCondition(sidecarSet, pod, true)
 }
 
 func (p *Processor) listMatchedSidecarSets(pod *corev1.Pod) string {
@@ -632,32 +622,60 @@ func isSidecarSetUpdateFinish(status *appsv1alpha1.SidecarSetStatus) bool {
 	return status.UpdatedPods >= status.MatchedPods
 }
 
-func (p *Processor) updateNotUpgradablePodCondition(sidecarset *appsv1alpha1.SidecarSet, notUpgradablePod *corev1.Pod) error {
-
+func (p *Processor) updatePodSidecarSetUpgradableCondition(sidecarset *appsv1alpha1.SidecarSet, pod *corev1.Pod, upgradable bool) error {
 	podClone := &corev1.Pod{}
+	if err := p.Client.Get(context.TODO(), types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}, podClone); err != nil {
+		klog.Errorf("error getting updated pod %s/%s from client", pod.Namespace, pod.Name)
+		return err
+	}
 
-	err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
-		if err := p.Client.Get(context.TODO(), types.NamespacedName{Namespace: notUpgradablePod.Namespace, Name: notUpgradablePod.Name}, podClone); err != nil {
-			klog.Errorf("error getting pod %s/%s from client", notUpgradablePod.Namespace, notUpgradablePod.Name)
-			return err
-		}
-		// update pod condition
-		conditionUpdateResult := podutil.UpdatePodCondition(&podClone.Status, &corev1.PodCondition{
-			Type:    sidecarcontrol.SidecarSetUpgradable,
-			Status:  corev1.ConditionFalse,
-			Reason:  "UpdateImmutableField",
-			Message: "Pod's sidecar set is not upgradable due to changes out of image field",
-		})
-		if !conditionUpdateResult {
-			return nil
+	_, oldCondition := podutil.GetPodCondition(&podClone.Status, sidecarcontrol.SidecarSetUpgradable)
+	var condition *corev1.PodCondition
+	if oldCondition != nil {
+		condition = oldCondition.DeepCopy()
+	} else {
+		condition = &corev1.PodCondition{
+			Type: sidecarcontrol.SidecarSetUpgradable,
 		}
-		err := p.Client.Status().Update(context.TODO(), podClone)
-		if err != nil {
-			return err
+	}
+
+	// get message kv from condition message
+	messageKv, err := controlutil.GetMessageKvFromCondition(condition)
+	if err != nil {
+		return err
+	}
+	// mark sidecarset upgradable status
+	messageKv[sidecarset.Name] = upgradable
+	// update condition message
+	allSidecarsetUpgradable := true
+	for _, v := range messageKv {
+		if v == false {
+			allSidecarsetUpgradable = false
+			break
 		}
-		klog.V(3).Infof("sidecarSet(%s) updated pods(%s/%s) condition(%s=%s) success", sidecarset.Name, notUpgradablePod.Namespace, notUpgradablePod.Name, sidecarcontrol.SidecarSetUpgradable, corev1.ConditionFalse)
+	}
+
+	// only update condition status to true when all sidecarset upgradable status is true
+	if allSidecarsetUpgradable {
+		condition.Status = corev1.ConditionTrue
+		condition.Reason = "AllSidecarsetUpgradable"
+	} else {
+		condition.Status = corev1.ConditionFalse
+		condition.Reason = "UpdateImmutableField"
+	}
+
+	controlutil.UpdateMessageKvCondition(messageKv, condition)
+	// patch SidecarSetUpgradable condition
+	if conditionChanged := podutil.UpdatePodCondition(&podClone.Status, condition); !conditionChanged {
+		// reduce unnecessary patch.
 		return nil
-	})
+	}
 
-	return err
+	mergePatch := fmt.Sprintf(`{"status": {"conditions": [%s]}}`, util.DumpJSON(condition))
+	err = p.Client.Status().Patch(context.TODO(), podClone, client.RawPatch(types.StrategicMergePatchType, []byte(mergePatch)))
+	if err != nil {
+		return err
+	}
+	klog.V(3).Infof("sidecarSet(%s) update pod %s/%s condition(%s=%s) success", sidecarset.Name, pod.Namespace, pod.Name, sidecarcontrol.SidecarSetUpgradable, condition.Status)
+	return nil
 }
diff --git a/pkg/controller/util/pod_condition_utils.go b/pkg/controller/util/pod_condition_utils.go
@@ -0,0 +1,24 @@
+package util
+
+import (
+	"encoding/json"
+
+	v1 "k8s.io/api/core/v1"
+)
+
+// using pod condition message to get key-value pairs
+func GetMessageKvFromCondition(condition *v1.PodCondition) (map[string]interface{}, error) {
+	messageKv := make(map[string]interface{})
+	if condition != nil && condition.Message != "" {
+		if err := json.Unmarshal([]byte(condition.Message), &messageKv); err != nil {
+			return nil, err
+		}
+	}
+	return messageKv, nil
+}
+
+// using pod condition message to save key-value pairs
+func UpdateMessageKvCondition(kv map[string]interface{}, condition *v1.PodCondition) {
+	message, _ := json.Marshal(kv)
+	condition.Message = string(message)
+}
diff --git a/test/e2e/apps/sidecarset.go b/test/e2e/apps/sidecarset.go
@@ -899,6 +899,95 @@ var _ = SIGDescribe("SidecarSet", func() {
 			ginkgo.By("sidecarSet upgrade sidecar container (more than image field), no pod should be updated done")
 		})
 
+		framework.ConformanceIt("multi sidecarSet upgrade sidecar container, check pod condition", func() {
+			// create sidecarSet 1
+			sidecarSetIn1 := tester.NewBaseSidecarSet(ns)
+			sidecarSetIn1.Name = "test-sidecarset-1"
+			sidecarSetIn1.Spec.UpdateStrategy = appsv1alpha1.SidecarSetUpdateStrategy{
+				Type: appsv1alpha1.RollingUpdateSidecarSetStrategyType,
+			}
+			sidecarSetIn1.Spec.Containers = sidecarSetIn1.Spec.Containers[:1]
+			ginkgo.By(fmt.Sprintf("Creating SidecarSet %s", sidecarSetIn1.Name))
+			sidecarSetIn1, _ = tester.CreateSidecarSet(sidecarSetIn1)
+			time.Sleep(time.Second)
+
+			// create sidecarSet 2
+			sidecarSetIn2 := tester.NewBaseSidecarSet(ns)
+			sidecarSetIn2.Name = "test-sidecarset-2"
+			sidecarSetIn2.Spec.UpdateStrategy = appsv1alpha1.SidecarSetUpdateStrategy{
+				Type: appsv1alpha1.RollingUpdateSidecarSetStrategyType,
+			}
+			sidecarSetIn2.Spec.InitContainers = []appsv1alpha1.SidecarContainer{}
+			sidecarSetIn2.Spec.Containers = sidecarSetIn2.Spec.Containers[1:2]
+			ginkgo.By(fmt.Sprintf("Creating SidecarSet %s", sidecarSetIn2.Name))
+			sidecarSetIn2, _ = tester.CreateSidecarSet(sidecarSetIn2)
+			time.Sleep(time.Second)
+
+			// create deployment
+			deploymentIn := tester.NewBaseDeployment(ns)
+			deploymentIn.Spec.Replicas = utilpointer.Int32Ptr(2)
+			ginkgo.By(fmt.Sprintf("Creating Deployment(%s/%s)", deploymentIn.Namespace, deploymentIn.Name))
+			tester.CreateDeployment(deploymentIn)
+
+			sidecarSetIn1, err := kc.AppsV1alpha1().SidecarSets().Get(context.TODO(), sidecarSetIn1.Name, metav1.GetOptions{})
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+			sidecarSetIn2, err = kc.AppsV1alpha1().SidecarSets().Get(context.TODO(), sidecarSetIn2.Name, metav1.GetOptions{})
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+			// modify sidecarSet1 field out of image, should not update pod
+			sidecarSetIn1.Spec.Containers[0].Command = []string{"sleep", "1000"}
+			tester.UpdateSidecarSet(sidecarSetIn1)
+			except := &appsv1alpha1.SidecarSetStatus{
+				MatchedPods:      2,
+				UpdatedPods:      0,
+				UpdatedReadyPods: 0,
+				ReadyPods:        2,
+			}
+			tester.WaitForSidecarSetUpgradeComplete(sidecarSetIn1, except)
+
+			// modify sidecarSet2, only change image
+			sidecarSetIn2.Spec.Containers[0].Image = NginxImage
+			tester.UpdateSidecarSet(sidecarSetIn2)
+			except = &appsv1alpha1.SidecarSetStatus{
+				MatchedPods:      2,
+				UpdatedPods:      2,
+				UpdatedReadyPods: 2,
+				ReadyPods:        2,
+			}
+			tester.WaitForSidecarSetUpgradeComplete(sidecarSetIn2, except)
+
+			// check all the pods' condition, due to sidecarSet1 is not updated, so the condition should be false
+			pods, err := tester.GetSelectorPods(deploymentIn.Namespace, deploymentIn.Spec.Selector)
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+			for _, pod := range pods {
+				_, condition := podutil.GetPodCondition(&pod.Status, sidecarcontrol.SidecarSetUpgradable)
+				gomega.Expect(condition.Status).Should(gomega.Equal(corev1.ConditionFalse))
+			}
+
+			// then update sidecarSet1, all the pods should be updated
+			sidecarSetIn1.Spec.Containers[0].Image = NewNginxImage
+			sidecarSetIn1.Spec.Containers[0].Command = []string{"tail", "-f", "/dev/null"}
+			tester.UpdateSidecarSet(sidecarSetIn1)
+			except = &appsv1alpha1.SidecarSetStatus{
+				MatchedPods:      2,
+				UpdatedPods:      2,
+				UpdatedReadyPods: 2,
+				ReadyPods:        2,
+			}
+			tester.WaitForSidecarSetUpgradeComplete(sidecarSetIn1, except)
+
+			// all the sidecarset is updated, so the condition should be true
+			pods, err = tester.GetSelectorPods(deploymentIn.Namespace, deploymentIn.Spec.Selector)
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+			for _, pod := range pods {
+				_, condition := podutil.GetPodCondition(&pod.Status, sidecarcontrol.SidecarSetUpgradable)
+				gomega.Expect(condition.Status).Should(gomega.Equal(corev1.ConditionTrue))
+			}
+
+			ginkgo.By("multi sidecarSet upgrade sidecar container, check pod condition done")
+		})
+
 		framework.ConformanceIt("sidecarSet upgrade cold sidecar container image, and paused", func() {
 			// create sidecarSet
 			sidecarSetIn := tester.NewBaseSidecarSet(ns)