add condition and event info for not upgradable pods when update side…

…carset (openkruise#1272) (openkruise#1309) * add condition for pods and event for sidecarset when detecting not upgradable pod (openkruise#1272) * add e2e test for sidecarset upgrade out of image fields(openkruise#1272) * only update condition to true when all sidecarset upgradable (openkruise#1272) ---------
ppbits · Jul 10, 2023 · 5979679 · 5979679
1 parent 79f3d2b
commit 5979679
Show file tree

Hide file tree

Showing 6 changed files with 332 additions and 26 deletions.
diff --git a/pkg/control/sidecarcontrol/util.go b/pkg/control/sidecarcontrol/util.go
@@ -61,6 +61,9 @@ const (
 	// SidecarsetInplaceUpdateStateKey records the state of inplace-update.
 	// The value of annotation is SidecarsetInplaceUpdateStateKey.
 	SidecarsetInplaceUpdateStateKey string = "kruise.io/sidecarset-inplace-update-state"
+
+	// SidecarSetUpgradable is a pod condition to indicate whether the pod's sidecarset is upgradable
+	SidecarSetUpgradable corev1.PodConditionType = "SidecarSetUpgradable"
 )
 
 var (

diff --git a/pkg/controller/sidecarset/sidecarset_processor.go b/pkg/controller/sidecarset/sidecarset_processor.go
@@ -27,10 +27,12 @@ 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"
 	webhookutil "github.com/openkruise/kruise/pkg/webhook/util"
+	podutil "k8s.io/kubernetes/pkg/api/v1/pod"
 
 	apps "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -155,7 +157,18 @@ func (p *Processor) UpdateSidecarSet(sidecarSet *appsv1alpha1.SidecarSet) (recon
 func (p *Processor) updatePods(control sidecarcontrol.SidecarControl, pods []*corev1.Pod) error {
 	sidecarset := control.GetSidecarset()
 	// compute next updated pods based on the sidecarset upgrade strategy
-	upgradePods := NewStrategy().GetNextUpgradePods(control, pods)
+	upgradePods, notUpgradablePods := NewStrategy().GetNextUpgradePods(control, pods)
+	for _, pod := range notUpgradablePods {
+		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
+		}
+		sidecarcontrol.UpdateExpectations.ExpectUpdated(sidecarset.Name, sidecarcontrol.GetSidecarSetRevision(sidecarset), pod)
+	}
+	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))
+	}
+
 	if len(upgradePods) == 0 {
 		klog.V(3).Infof("sidecarSet next update is nil, skip this round, name: %s", sidecarset.Name)
 		return nil
@@ -181,7 +194,7 @@ func (p *Processor) updatePodSidecarAndHash(control sidecarcontrol.SidecarContro
 	sidecarSet := control.GetSidecarset()
 	err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		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 from client", control.GetSidecarset().Name)
+			klog.Errorf("sidecarset(%s) error getting updated pod %s/%s from client", control.GetSidecarset().Name, pod.Namespace, pod.Name)
 		}
 		// update pod sidecar container
 		updatePodSidecarContainer(control, podClone)
@@ -197,10 +210,16 @@ func (p *Processor) updatePodSidecarAndHash(control sidecarcontrol.SidecarContro
 			klog.Errorf("sidecarSet(%s) patch pod(%s/%s) metadata failed: %s", sidecarSet.Name, podClone.Namespace, podClone.Name, err.Error())
 			return err
 		}
-		//update pod in store
+		// update pod in store
 		return p.Client.Update(context.TODO(), podClone)
 	})
-	return err
+
+	if err != nil {
+		return err
+	}
+
+	// update pod condition of sidecar upgradable
+	return p.updatePodSidecarSetUpgradableCondition(sidecarSet, pod, true)
 }
 
 func (p *Processor) listMatchedSidecarSets(pod *corev1.Pod) string {
@@ -602,3 +621,57 @@ func inconsistentStatus(sidecarSet *appsv1alpha1.SidecarSet, status *appsv1alpha
 func isSidecarSetUpdateFinish(status *appsv1alpha1.SidecarSetStatus) bool {
 	return status.UpdatedPods >= status.MatchedPods
 }
+
+func (p *Processor) updatePodSidecarSetUpgradableCondition(sidecarset *appsv1alpha1.SidecarSet, pod *corev1.Pod, upgradable bool) error {
+	podClone := pod.DeepCopy()
+
+	_, oldCondition := podutil.GetPodCondition(&podClone.Status, sidecarcontrol.SidecarSetUpgradable)
+	var condition *corev1.PodCondition
+	if oldCondition != nil {
+		condition = oldCondition.DeepCopy()
+	} else {
+		condition = &corev1.PodCondition{
+			Type: sidecarcontrol.SidecarSetUpgradable,
+		}
+	}
+
+	// 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
+		}
+	}
+
+	// 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
+	}
+
+	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/sidecarset/sidecarset_strategy.go b/pkg/controller/sidecarset/sidecarset_strategy.go
@@ -22,7 +22,8 @@ type Strategy interface {
 	//2. Sort Pods with default sequence
 	//3. sort waitUpdateIndexes based on the scatter rules
 	//4. calculate max count of pods can update with maxUnavailable
-	GetNextUpgradePods(control sidecarcontrol.SidecarControl, pods []*corev1.Pod) []*corev1.Pod
+	//5. also return the pods that are not upgradable
+	GetNextUpgradePods(control sidecarcontrol.SidecarControl, pods []*corev1.Pod) (upgradePods []*corev1.Pod, notUpgradablePods []*corev1.Pod)
 }
 
 type spreadingStrategy struct{}
@@ -35,10 +36,12 @@ func NewStrategy() Strategy {
 	return globalSpreadingStrategy
 }
 
-func (p *spreadingStrategy) GetNextUpgradePods(control sidecarcontrol.SidecarControl, pods []*corev1.Pod) (upgradePods []*corev1.Pod) {
+func (p *spreadingStrategy) GetNextUpgradePods(control sidecarcontrol.SidecarControl, pods []*corev1.Pod) (upgradePods []*corev1.Pod, notUpgradablePods []*corev1.Pod) {
 	sidecarset := control.GetSidecarset()
 	// wait to upgrade pod index
 	var waitUpgradedIndexes []int
+	// because SidecarSet in-place update only support upgrading Image, if other fields are changed they will not be upgraded.
+	var notUpgradableIndexes []int
 	strategy := sidecarset.Spec.UpdateStrategy
 
 	// If selector is not nil, check whether the pods is selected to upgrade
@@ -68,12 +71,17 @@ func (p *spreadingStrategy) GetNextUpgradePods(control sidecarcontrol.SidecarCon
 	//  * It is to determine whether there are other fields that have been modified for pod.
 	for index, pod := range pods {
 		isUpdated := sidecarcontrol.IsPodSidecarUpdated(sidecarset, pod)
-		if !isUpdated && isSelected(pod) && control.IsSidecarSetUpgradable(pod) {
-			waitUpgradedIndexes = append(waitUpgradedIndexes, index)
+		if !isUpdated && isSelected(pod) {
+			if control.IsSidecarSetUpgradable(pod) {
+				waitUpgradedIndexes = append(waitUpgradedIndexes, index)
+			} else if sidecarcontrol.GetPodSidecarSetWithoutImageRevision(sidecarset.Name, pod) != sidecarcontrol.GetSidecarSetWithoutImageRevision(sidecarset) {
+				// only image field can be in-place updated, if other fields changed, mark pod as not upgradable
+				notUpgradableIndexes = append(notUpgradableIndexes, index)
+			}
 		}
 	}
 
-	klog.V(3).Infof("sidecarSet(%s) matchedPods(%d) waitUpdated(%d)", sidecarset.Name, len(pods), len(waitUpgradedIndexes))
+	klog.V(3).Infof("sidecarSet(%s) matchedPods(%d) waitUpdated(%d) notUpgradable(%d)", sidecarset.Name, len(pods), len(waitUpgradedIndexes), len(notUpgradableIndexes))
 	//2. sort Pods with default sequence and scatter
 	waitUpgradedIndexes = SortUpdateIndexes(strategy, pods, waitUpgradedIndexes)
 
@@ -87,6 +95,10 @@ func (p *spreadingStrategy) GetNextUpgradePods(control sidecarcontrol.SidecarCon
 	for _, idx := range waitUpgradedIndexes {
 		upgradePods = append(upgradePods, pods[idx])
 	}
+	// 5. pods that are not upgradable will not be skipped in the following process
+	for _, idx := range notUpgradableIndexes {
+		notUpgradablePods = append(notUpgradablePods, pods[idx])
+	}
 	return
 }
 

diff --git a/pkg/controller/sidecarset/sidecarset_strategy_test.go b/pkg/controller/sidecarset/sidecarset_strategy_test.go
@@ -111,11 +111,19 @@ func factoryPods(count, upgraded, upgradedAndReady int) []*corev1.Pod {
 }
 
 func factorySidecarSet() *appsv1alpha1.SidecarSet {
+	return createFactorySidecarSet("bbb", "without-aaa")
+}
+
+func factorySidecarSetNotUpgradable() *appsv1alpha1.SidecarSet {
+	return createFactorySidecarSet("bbb", "without-bbb")
+}
+
+func createFactorySidecarSet(sidecarsetHash string, sidecarsetHashWithoutImage string) *appsv1alpha1.SidecarSet {
 	sidecarSet := &appsv1alpha1.SidecarSet{
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: map[string]string{
-				sidecarcontrol.SidecarSetHashAnnotation:             "bbb",
-				sidecarcontrol.SidecarSetHashWithoutImageAnnotation: "without-aaa",
+				sidecarcontrol.SidecarSetHashAnnotation:             sidecarsetHash,
+				sidecarcontrol.SidecarSetHashWithoutImageAnnotation: sidecarsetHashWithoutImage,
 			},
 			Name:   "test-sidecarset",
 			Labels: map[string]string{},
@@ -148,10 +156,11 @@ func TestGetNextUpgradePods(t *testing.T) {
 
 func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySidecar FactorySidecarSet) {
 	cases := []struct {
-		name                   string
-		getPods                func() []*corev1.Pod
-		getSidecarset          func() *appsv1alpha1.SidecarSet
-		exceptNeedUpgradeCount int
+		name                     string
+		getPods                  func() []*corev1.Pod
+		getSidecarset            func() *appsv1alpha1.SidecarSet
+		exceptNeedUpgradeCount   int
+		exceptNotUpgradableCount int
 	}{
 		{
 			name: "only maxUnavailable(int=10), and pods(count=100, upgraded=30, upgradedAndReady=26)",
@@ -167,7 +176,8 @@ func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySideca
 				}
 				return sidecarSet
 			},
-			exceptNeedUpgradeCount: 6,
+			exceptNeedUpgradeCount:   6,
+			exceptNotUpgradableCount: 0,
 		},
 		{
 			name: "only maxUnavailable(string=10%), and pods(count=1000, upgraded=300, upgradedAndReady=260)",
@@ -183,7 +193,8 @@ func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySideca
 				}
 				return sidecarSet
 			},
-			exceptNeedUpgradeCount: 60,
+			exceptNeedUpgradeCount:   60,
+			exceptNotUpgradableCount: 0,
 		},
 		{
 			name: "only maxUnavailable(string=5%), and pods(count=1000, upgraded=300, upgradedAndReady=250)",
@@ -199,7 +210,8 @@ func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySideca
 				}
 				return sidecarSet
 			},
-			exceptNeedUpgradeCount: 0,
+			exceptNeedUpgradeCount:   0,
+			exceptNotUpgradableCount: 0,
 		},
 		{
 			name: "only maxUnavailable(int=100), and pods(count=100, upgraded=30, upgradedAndReady=27)",
@@ -215,7 +227,8 @@ func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySideca
 				}
 				return sidecarSet
 			},
-			exceptNeedUpgradeCount: 70,
+			exceptNeedUpgradeCount:   70,
+			exceptNotUpgradableCount: 0,
 		},
 		{
 			name: "partition(int=180) maxUnavailable(int=100), and pods(count=1000, upgraded=800, upgradedAndReady=760)",
@@ -235,7 +248,8 @@ func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySideca
 				}
 				return sidecarSet
 			},
-			exceptNeedUpgradeCount: 20,
+			exceptNeedUpgradeCount:   20,
+			exceptNotUpgradableCount: 0,
 		},
 		{
 			name: "partition(int=100) maxUnavailable(int=100), and pods(count=1000, upgraded=800, upgradedAndReady=760)",
@@ -255,7 +269,8 @@ func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySideca
 				}
 				return sidecarSet
 			},
-			exceptNeedUpgradeCount: 60,
+			exceptNeedUpgradeCount:   60,
+			exceptNotUpgradableCount: 0,
 		},
 		{
 			name: "partition(string=18%) maxUnavailable(int=100), and pods(count=1000, upgraded=800, upgradedAndReady=760)",
@@ -275,7 +290,8 @@ func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySideca
 				}
 				return sidecarSet
 			},
-			exceptNeedUpgradeCount: 20,
+			exceptNeedUpgradeCount:   20,
+			exceptNotUpgradableCount: 0,
 		},
 		{
 			name: "partition(string=10%) maxUnavailable(int=100), and pods(count=1000, upgraded=800, upgradedAndReady=760)",
@@ -295,7 +311,8 @@ func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySideca
 				}
 				return sidecarSet
 			},
-			exceptNeedUpgradeCount: 60,
+			exceptNeedUpgradeCount:   60,
+			exceptNotUpgradableCount: 0,
 		},
 		{
 			name: "selector(app=test, count=30) maxUnavailable(int=100), and pods(count=1000, upgraded=0, upgradedAndReady=0)",
@@ -317,18 +334,40 @@ func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySideca
 				}
 				return sidecarSet
 			},
-			exceptNeedUpgradeCount: 30,
+			exceptNeedUpgradeCount:   30,
+			exceptNotUpgradableCount: 0,
+		},
+		{
+			name: "not upgradable sidecarset, maxUnavailable(int=100), and pods(count=100, upgraded=0, upgradedAndReady=0)",
+			getPods: func() []*corev1.Pod {
+				pods := factoryPods(100, 0, 0)
+				return Random(pods)
+			},
+			getSidecarset: func() *appsv1alpha1.SidecarSet {
+				sidecarSet := factorySidecarSetNotUpgradable()
+				sidecarSet.Spec.UpdateStrategy.MaxUnavailable = &intstr.IntOrString{
+					Type:   intstr.Int,
+					IntVal: 100,
+				}
+
+				return sidecarSet
+			},
+			exceptNeedUpgradeCount:   0,
+			exceptNotUpgradableCount: 100,
 		},
 	}
 	strategy := NewStrategy()
 	for _, cs := range cases {
 		t.Run(cs.name, func(t *testing.T) {
 			control := sidecarcontrol.New(cs.getSidecarset())
 			pods := cs.getPods()
-			upgradePods := strategy.GetNextUpgradePods(control, pods)
+			upgradePods, notUpgradablePods := strategy.GetNextUpgradePods(control, pods)
 			if cs.exceptNeedUpgradeCount != len(upgradePods) {
 				t.Fatalf("except NeedUpgradeCount(%d), but get value(%d)", cs.exceptNeedUpgradeCount, len(upgradePods))
 			}
+			if cs.exceptNotUpgradableCount != len(notUpgradablePods) {
+				t.Fatalf("except NotUpgradableCount(%d), but get value(%d)", cs.exceptNotUpgradableCount, len(notUpgradablePods))
+			}
 		})
 	}
 }
@@ -591,7 +630,7 @@ func testSortNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySidec
 		t.Run(cs.name, func(t *testing.T) {
 			control := sidecarcontrol.New(cs.getSidecarset())
 			pods := cs.getPods()
-			injectedPods := strategy.GetNextUpgradePods(control, pods)
+			injectedPods, _ := strategy.GetNextUpgradePods(control, pods)
 			if len(cs.exceptNextUpgradePods) != len(injectedPods) {
 				t.Fatalf("except NeedUpgradeCount(%d), but get value(%d)", len(cs.exceptNextUpgradePods), len(injectedPods))
 			}

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