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

Signed-off-by: MarkLux <[email protected]>

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"
@@ -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.updateUpgradablePodCondition(sidecarSet, podClone)
 }
 
 func (p *Processor) listMatchedSidecarSets(pod *corev1.Pod) string {
@@ -632,6 +622,60 @@ func isSidecarSetUpdateFinish(status *appsv1alpha1.SidecarSetStatus) bool {
 	return status.UpdatedPods >= status.MatchedPods
 }
 
+func (p *Processor) updateUpgradablePodCondition(sidecarset *appsv1alpha1.SidecarSet, upgradablePod *corev1.Pod) error {
+
+	_, oldCondition := podutil.GetPodCondition(&upgradablePod.Status, sidecarcontrol.SidecarSetUpgradable)
+
+	var condition *corev1.PodCondition
+
+	if oldCondition != nil {
+		condition = oldCondition.DeepCopy()
+	} else {
+		condition = &corev1.PodCondition{
+			Type:   sidecarcontrol.SidecarSetUpgradable,
+			Status: corev1.ConditionTrue,
+		}
+	}
+
+	// get message kv from condition message
+	messageKv, err := controlutil.GetMessageKvFromCondition(condition)
+	if err != nil {
+		return err
+	}
+
+	// mark sidecarset upgradable status to true
+	messageKv[sidecarset.Name] = true
+
+	// update condition message
+	allSidecarsetUpgradable := true
+	for _, v := range messageKv {
+		if v == false {
+			allSidecarsetUpgradable = false
+			break
+		}
+	}
+
+	if allSidecarsetUpgradable {
+		condition.Status = corev1.ConditionTrue
+		condition.Reason = "AllSidecarsetUpgradable"
+	}
+
+	err = controlutil.UpdateMessageKvCondition(messageKv, condition)
+
+	if err != nil {
+		return err
+	}
+
+	// patch SidecarSetUpgradable condition
+	if conditionChanged := podutil.UpdatePodCondition(&upgradablePod.Status, condition); !conditionChanged {
+		// reduce unnecessary patch.
+		return nil
+	}
+
+	mergePatch := fmt.Sprintf(`{"status": {"conditions": [%s]}}`, util.DumpJSON(condition))
+	return p.Client.Status().Patch(context.TODO(), upgradablePod, client.RawPatch(types.StrategicMergePatchType, []byte(mergePatch)))
+}
+
 func (p *Processor) updateNotUpgradablePodCondition(sidecarset *appsv1alpha1.SidecarSet, notUpgradablePod *corev1.Pod) error {
 
 	podClone := &corev1.Pod{}
@@ -641,17 +685,34 @@ func (p *Processor) updateNotUpgradablePodCondition(sidecarset *appsv1alpha1.Sid
 			klog.Errorf("error getting pod %s/%s from client", notUpgradablePod.Namespace, notUpgradablePod.Name)
 			return err
 		}
+
+		_, oldCondition := podutil.GetPodCondition(&podClone.Status, sidecarcontrol.SidecarSetUpgradable)
+
+		condition := &corev1.PodCondition{
+			Type:   sidecarcontrol.SidecarSetUpgradable,
+			Status: corev1.ConditionFalse,
+			Reason: "UpdateImmutableField",
+		}
+
+		// get message kv from old pod condition, if conditoin not exist, an empty map will be returned
+		messageKv, err := controlutil.GetMessageKvFromCondition(oldCondition)
+		if err != nil {
+			return err
+		}
+		// mark this sidecarSet as false
+		messageKv[sidecarset.Name] = false
+		// update message kv
+		err = controlutil.UpdateMessageKvCondition(messageKv, condition)
+		if err != nil {
+			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",
-		})
+		conditionUpdateResult := podutil.UpdatePodCondition(&podClone.Status, condition)
 		if !conditionUpdateResult {
 			return nil
 		}
-		err := p.Client.Status().Update(context.TODO(), podClone)
+		err = p.Client.Status().Update(context.TODO(), podClone)
 		if err != nil {
 			return err
 		}

pkg/controller/util/pod_condition_utils.go

@@ -0,0 +1,28 @@
+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) error {
+	message, err := json.Marshal(kv)
+	if err != nil {
+		return err
+	}
+	condition.Message = string(message)
+	return nil
+}

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)