From 88bf49668f1e2b5fbc10d79de82ff466d8b846e8 Mon Sep 17 00:00:00 2001 From: MarkLux Date: Thu, 8 Jun 2023 20:09:20 +0800 Subject: [PATCH 1/3] add condition for pods and event for sidecarset when detecting not upgradable pod (#1272) Signed-off-by: MarkLux --- pkg/control/sidecarcontrol/util.go | 3 + .../sidecarset/sidecarset_processor.go | 63 +++++++++++++++- .../sidecarset/sidecarset_strategy.go | 22 ++++-- .../sidecarset/sidecarset_strategy_test.go | 73 ++++++++++++++----- test/e2e/apps/sidecarset.go | 3 + 5 files changed, 140 insertions(+), 24 deletions(-) diff --git a/pkg/control/sidecarcontrol/util.go b/pkg/control/sidecarcontrol/util.go index 19297b8656..4801e26f29 100644 --- 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 index 0d50e31553..4ea1e0a218 100644 --- a/pkg/controller/sidecarset/sidecarset_processor.go +++ b/pkg/controller/sidecarset/sidecarset_processor.go @@ -31,6 +31,7 @@ import ( 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 +156,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.updateNotUpgradablePodCondition(sidecarset, pod); 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 @@ -197,9 +209,26 @@ 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) }) + + if err != nil { + 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 } @@ -602,3 +631,33 @@ func inconsistentStatus(sidecarSet *appsv1alpha1.SidecarSet, status *appsv1alpha func isSidecarSetUpdateFinish(status *appsv1alpha1.SidecarSetStatus) bool { return status.UpdatedPods >= status.MatchedPods } + +func (p *Processor) updateNotUpgradablePodCondition(sidecarset *appsv1alpha1.SidecarSet, notUpgradablePod *corev1.Pod) error { + + podClone := &corev1.Pod{} + + 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 + } + err := p.Client.Status().Update(context.TODO(), podClone) + if err != nil { + return err + } + klog.V(3).Infof("sidecarSet(%s) updated pods(%s/%s) condition(%s=%s) success", sidecarset.Name, notUpgradablePod.Namespace, notUpgradablePod.Name, sidecarcontrol.SidecarSetUpgradable, corev1.ConditionFalse) + return nil + }) + + return err +} diff --git a/pkg/controller/sidecarset/sidecarset_strategy.go b/pkg/controller/sidecarset/sidecarset_strategy.go index edb14d0295..895a2e1125 100644 --- 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 index 48a0c9ee42..17cde67404 100644 --- a/pkg/controller/sidecarset/sidecarset_strategy_test.go +++ b/pkg/controller/sidecarset/sidecarset_strategy_test.go @@ -110,11 +110,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{}, @@ -147,10 +155,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)", @@ -166,7 +175,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)", @@ -182,7 +192,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)", @@ -198,7 +209,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)", @@ -214,7 +226,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)", @@ -234,7 +247,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)", @@ -254,7 +268,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)", @@ -274,7 +289,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)", @@ -294,7 +310,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)", @@ -316,7 +333,26 @@ 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() @@ -324,10 +360,13 @@ func testGetNextUpgradePods(t *testing.T, factoryPods FactoryPods, factorySideca 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)) + } }) } } @@ -524,7 +563,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/test/e2e/apps/sidecarset.go b/test/e2e/apps/sidecarset.go index 7c1a417853..638d89c13f 100644 --- a/test/e2e/apps/sidecarset.go +++ b/test/e2e/apps/sidecarset.go @@ -39,6 +39,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" clientset "k8s.io/client-go/kubernetes" + podutil "k8s.io/kubernetes/pkg/api/v1/pod" "k8s.io/kubernetes/pkg/controller/history" utilpointer "k8s.io/utils/pointer" ) @@ -739,6 +740,8 @@ var _ = SIGDescribe("SidecarSet", func() { gomega.Expect(err).NotTo(gomega.HaveOccurred()) for _, pod := range pods { gomega.Expect(pod.Spec.Containers[0].Image).Should(gomega.Equal(BusyboxImage)) + _, sidecarSetUpgradable := podutil.GetPodCondition(&pod.Status, sidecarcontrol.SidecarSetUpgradable) + gomega.Expect(sidecarSetUpgradable.Status).Should(gomega.Equal(corev1.ConditionTrue)) } ginkgo.By(fmt.Sprintf("sidecarSet upgrade cold sidecar container image done")) }) From 7d81a0e33c632c538ae1cc71519d556631d6229a Mon Sep 17 00:00:00 2001 From: MarkLux Date: Wed, 28 Jun 2023 10:56:10 +0800 Subject: [PATCH 2/3] add e2e test for sidecarset upgrade out of image fields(#1272) Signed-off-by: MarkLux --- test/e2e/apps/sidecarset.go | 63 +++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/test/e2e/apps/sidecarset.go b/test/e2e/apps/sidecarset.go index 638d89c13f..7d0f6e85a2 100644 --- a/test/e2e/apps/sidecarset.go +++ b/test/e2e/apps/sidecarset.go @@ -836,6 +836,69 @@ var _ = SIGDescribe("SidecarSet", func() { ginkgo.By(fmt.Sprintf("sidecarSet upgrade cold sidecar container failed image, and only update one pod done")) }) + framework.ConformanceIt("sidecarSet upgrade sidecar container (more than image field), no pod should be updated", func() { + // create sidecarSet + sidecarSetIn := tester.NewBaseSidecarSet(ns) + sidecarSetIn.Spec.UpdateStrategy = appsv1alpha1.SidecarSetUpdateStrategy{ + Type: appsv1alpha1.RollingUpdateSidecarSetStrategyType, + } + sidecarSetIn.Spec.Containers = sidecarSetIn.Spec.Containers[:1] + ginkgo.By(fmt.Sprintf("Creating SidecarSet %s", sidecarSetIn.Name)) + sidecarSetIn, _ = tester.CreateSidecarSet(sidecarSetIn) + 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) + + sidecarSetIn, err := kc.AppsV1alpha1().SidecarSets().Get(context.TODO(), sidecarSetIn.Name, metav1.GetOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + //check pod sidecar upgrade spec annotations + pods, err := tester.GetSelectorPods(deploymentIn.Namespace, deploymentIn.Spec.Selector) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + for _, pod := range pods { + origin := sets.String{} + for _, sidecar := range sidecarSetIn.Spec.Containers { + origin.Insert(sidecar.Name) + } + // SidecarSetHashAnnotation = "kruise.io/sidecarset-hash" + upgradeSpec1 := sidecarcontrol.GetPodSidecarSetUpgradeSpecInAnnotations(sidecarSetIn.Name, sidecarcontrol.SidecarSetHashAnnotation, pod) + gomega.Expect(upgradeSpec1.SidecarSetName).To(gomega.Equal(sidecarSetIn.Name)) + gomega.Expect(upgradeSpec1.SidecarSetHash).To(gomega.Equal(sidecarcontrol.GetSidecarSetRevision(sidecarSetIn))) + target1 := sets.NewString(upgradeSpec1.SidecarList...) + gomega.Expect(reflect.DeepEqual(origin.List(), target1.List())).To(gomega.Equal(true)) + // SidecarSetHashWithoutImageAnnotation = "kruise.io/sidecarset-hash-without-image" + upgradeSpec2 := sidecarcontrol.GetPodSidecarSetUpgradeSpecInAnnotations(sidecarSetIn.Name, sidecarcontrol.SidecarSetHashWithoutImageAnnotation, pod) + gomega.Expect(upgradeSpec2.SidecarSetName).To(gomega.Equal(sidecarSetIn.Name)) + gomega.Expect(upgradeSpec2.SidecarSetHash).To(gomega.Equal(sidecarcontrol.GetSidecarSetWithoutImageRevision(sidecarSetIn))) + target2 := sets.NewString(upgradeSpec2.SidecarList...) + gomega.Expect(reflect.DeepEqual(origin.List(), target2.List())).To(gomega.Equal(true)) + } + + // modify sidecarSet sidecar field out of image + sidecarSetIn.Spec.Containers[0].Command = []string{"sleep", "1000"} + tester.UpdateSidecarSet(sidecarSetIn) + except := &appsv1alpha1.SidecarSetStatus{ + MatchedPods: 2, + UpdatedPods: 0, + UpdatedReadyPods: 0, + ReadyPods: 2, + } + tester.WaitForSidecarSetUpgradeComplete(sidecarSetIn, except) + + // check all the pods' condition + 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)) + } + + ginkgo.By("sidecarSet upgrade sidecar container (more than image field), no pod should be updated done") + }) + framework.ConformanceIt("sidecarSet upgrade cold sidecar container image, and paused", func() { // create sidecarSet sidecarSetIn := tester.NewBaseSidecarSet(ns) From 4db30bae83a7ce31c46bd9ce96a0d071683e7e60 Mon Sep 17 00:00:00 2001 From: MarkLux Date: Sat, 1 Jul 2023 14:43:04 +0800 Subject: [PATCH 3/3] only update condition to true when all sidecarset upgradable (#1272) Signed-off-by: MarkLux --- .../sidecarset/sidecarset_processor.go | 90 +++++++++++-------- pkg/controller/util/pod_condition_utils.go | 24 +++++ test/e2e/apps/sidecarset.go | 89 ++++++++++++++++++ 3 files changed, 165 insertions(+), 38 deletions(-) create mode 100644 pkg/controller/util/pod_condition_utils.go diff --git a/pkg/controller/sidecarset/sidecarset_processor.go b/pkg/controller/sidecarset/sidecarset_processor.go index 4ea1e0a218..54233be433 100644 --- 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 } @@ -193,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) @@ -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,56 @@ func isSidecarSetUpdateFinish(status *appsv1alpha1.SidecarSetStatus) bool { return status.UpdatedPods >= status.MatchedPods } -func (p *Processor) updateNotUpgradablePodCondition(sidecarset *appsv1alpha1.SidecarSet, notUpgradablePod *corev1.Pod) error { - - podClone := &corev1.Pod{} +func (p *Processor) updatePodSidecarSetUpgradableCondition(sidecarset *appsv1alpha1.SidecarSet, pod *corev1.Pod, upgradable bool) error { + podClone := pod.DeepCopy() - 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 new file mode 100644 index 0000000000..44fa65a7aa --- /dev/null +++ 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 index 7d0f6e85a2..8c636d129b 100644 --- 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)