From 3b034869e5165dcf36edbd34c047b19cd865b0b6 Mon Sep 17 00:00:00 2001 From: yunbo Date: Fri, 7 Jun 2024 14:56:02 +0800 Subject: [PATCH] nextStepIndex default value from 0 to -1 Signed-off-by: yunbo --- api/v1alpha1/deployment_types.go | 2 - api/v1beta1/deployment_types.go | 18 +++-- api/v1beta1/rollout_types.go | 8 --- api/v1beta1/zz_generated.deepcopy.go | 8 +-- .../rollout/rollout_progressing_test.go | 4 -- pkg/util/rollout_utils.go | 2 +- .../rollout_create_update_handler.go | 68 +++++++++++++++---- test/e2e/deployment_test.go | 21 ++++++ test/e2e/rollout_test.go | 19 ++++-- 9 files changed, 100 insertions(+), 50 deletions(-) diff --git a/api/v1alpha1/deployment_types.go b/api/v1alpha1/deployment_types.go index 99f95cf7..98fc474c 100644 --- a/api/v1alpha1/deployment_types.go +++ b/api/v1alpha1/deployment_types.go @@ -61,8 +61,6 @@ const ( CanaryRollingStyle RollingStyleType = "Canary" // BlueGreenRollingStyle means rolling in blue-green way, and will NOT create a canary Deployment. BlueGreenRollingStyle RollingStyleType = "BlueGreen" - // Empty means both Canary and BlueGreen are empty - EmptyRollingStyle RollingStyleType = "Empty" ) // DeploymentExtraStatus is extra status field for Advanced Deployment diff --git a/api/v1beta1/deployment_types.go b/api/v1beta1/deployment_types.go index d36f6e9a..9975e989 100644 --- a/api/v1beta1/deployment_types.go +++ b/api/v1beta1/deployment_types.go @@ -38,9 +38,9 @@ const ( // which labels whether the deployment is controlled by advanced-deployment-controller. AdvancedDeploymentControlLabel = "rollouts.kruise.io/controlled-by-advanced-deployment-controller" - // OriginalSettingAnnotation is annotation for workload in BlueGreen Release, - // it will storage the original setting of the workload, which will be used to restore the workload - OriginalSettingAnnotation = "rollouts.kruise.io/original-setting" + // OriginalDeploymentStrategyAnnotation is annotation for workload in BlueGreen Release, + // it will store the original setting of the workload, which will be used to restore the workload + OriginalDeploymentStrategyAnnotation = "rollouts.kruise.io/original-deployment-strategy" // MaxProgressSeconds is the value we set for ProgressDeadlineSeconds // MaxReadySeconds is the value we set for MinReadySeconds, which is one less than ProgressDeadlineSeconds @@ -62,12 +62,12 @@ type DeploymentStrategy struct { Partition intstr.IntOrString `json:"partition,omitempty"` } -// OriginalSetting storages part of the fileds of a workload, +// OriginalDeploymentStrategy stores part of the fileds of a workload, // so that it can be restored when finalizing. // It is only used for BlueGreen Release -// Similar to DeploymentStrategy, it is stored in workload annotation -// However, unlike DeploymentStrategy, it is only used to storage and restore -type OriginalSetting struct { +// Similar to DeploymentStrategy, it is an annotation used in workload +// However, unlike DeploymentStrategy, it is only used to store and restore the user's strategy +type OriginalDeploymentStrategy struct { // The deployment strategy to use to replace existing pods with new ones. // +optional // +patchStrategy=retainKeys @@ -96,8 +96,6 @@ const ( CanaryRollingStyle RollingStyleType = "Canary" // BlueGreenRollingStyle means rolling in blue-green way, and will NOT create a extra Deployment. BlueGreenRollingStyle RollingStyleType = "BlueGreen" - // Empty means both Canary and BlueGreen are empty - EmptyRollingStyle RollingStyleType = "Empty" ) // DeploymentExtraStatus is extra status field for Advanced Deployment @@ -141,7 +139,7 @@ func SetDefaultDeploymentStrategy(strategy *DeploymentStrategy) { } } -func SetDefaultSetting(setting *OriginalSetting) { +func SetDefaultSetting(setting *OriginalDeploymentStrategy) { if setting.ProgressDeadlineSeconds == nil { setting.ProgressDeadlineSeconds = new(int32) *setting.ProgressDeadlineSeconds = 600 diff --git a/api/v1beta1/rollout_types.go b/api/v1beta1/rollout_types.go index d1b7b750..89f7a2bc 100644 --- a/api/v1beta1/rollout_types.go +++ b/api/v1beta1/rollout_types.go @@ -81,9 +81,6 @@ type RolloutStrategy struct { // Get the rolling style based on the strategy func (r *RolloutStrategy) GetRollingStyle() RollingStyleType { - if r.BlueGreen == nil && r.Canary == nil { - return EmptyRollingStyle - } if r.BlueGreen != nil { return BlueGreenRollingStyle } @@ -105,11 +102,6 @@ func (r *RolloutStrategy) IsCanaryStragegy() bool { return r.GetRollingStyle() == CanaryRollingStyle || r.GetRollingStyle() == PartitionRollingStyle } -// r.GetRollingStyle() == EmptyRollingStyle -func (r *RolloutStrategy) IsEmptyRelease() bool { - return r.GetRollingStyle() == EmptyRollingStyle -} - // Get the steps based on the rolling style func (r *RolloutStrategy) GetSteps() []CanaryStep { switch r.GetRollingStyle() { diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 528ed99d..98e46fef 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -411,7 +411,7 @@ func (in *ObjectRef) DeepCopy() *ObjectRef { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *OriginalSetting) DeepCopyInto(out *OriginalSetting) { +func (in *OriginalDeploymentStrategy) DeepCopyInto(out *OriginalDeploymentStrategy) { *out = *in if in.Strategy != nil { in, out := &in.Strategy, &out.Strategy @@ -425,12 +425,12 @@ func (in *OriginalSetting) DeepCopyInto(out *OriginalSetting) { } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OriginalSetting. -func (in *OriginalSetting) DeepCopy() *OriginalSetting { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OriginalDeploymentStrategy. +func (in *OriginalDeploymentStrategy) DeepCopy() *OriginalDeploymentStrategy { if in == nil { return nil } - out := new(OriginalSetting) + out := new(OriginalDeploymentStrategy) in.DeepCopyInto(out) return out } diff --git a/pkg/controller/rollout/rollout_progressing_test.go b/pkg/controller/rollout/rollout_progressing_test.go index 696f5dbd..42412b3e 100644 --- a/pkg/controller/rollout/rollout_progressing_test.go +++ b/pkg/controller/rollout/rollout_progressing_test.go @@ -67,10 +67,6 @@ func TestReconcileRolloutProgressing(t *testing.T) { s.CanaryStatus.StableRevision = "pod-template-hash-v1" s.CanaryStatus.CanaryRevision = "6f8cc56547" s.CanaryStatus.CurrentStepIndex = 1 - // s.CanaryStatus.NextStepIndex will be initialized as 0 in ReconcileRolloutProgressing - // util.NextBatchIndex(rollout, s.CanaryStatus.CurrentStepIndex), which is 2 here. - // However the ReconcileRolloutProgressing won't update it, and thus expected value of it - // in the next cases will be 0 (zero in zero out), without update. This is as expected s.CanaryStatus.NextStepIndex = 2 s.CanaryStatus.CurrentStepState = v1beta1.CanaryStepStateUpgrade return s diff --git a/pkg/util/rollout_utils.go b/pkg/util/rollout_utils.go index 464370c7..324bfe04 100644 --- a/pkg/util/rollout_utils.go +++ b/pkg/util/rollout_utils.go @@ -175,7 +175,7 @@ func NextBatchIndex(rollout *rolloutv1beta1.Rollout, CurrentStepIndex int32) int // Patching NextStepIndex to 0 is meaningless for user, if doing so, nothing happen // and step jump won't happen if CurrentStepIndex >= allSteps { - return 0 + return -1 } return CurrentStepIndex + 1 } diff --git a/pkg/webhook/rollout/validating/rollout_create_update_handler.go b/pkg/webhook/rollout/validating/rollout_create_update_handler.go index 0d2c78b9..43db8927 100644 --- a/pkg/webhook/rollout/validating/rollout_create_update_handler.go +++ b/pkg/webhook/rollout/validating/rollout_create_update_handler.go @@ -133,12 +133,11 @@ func (h *RolloutCreateUpdateHandler) validateRolloutUpdate(oldObj, newObj *appsv if !reflect.DeepEqual(oldObj.Spec.WorkloadRef, newObj.Spec.WorkloadRef) { return field.ErrorList{field.Forbidden(field.NewPath("Spec.ObjectRef"), "Rollout 'ObjectRef' field is immutable")} } - // canary strategy - if !reflect.DeepEqual(oldObj.Spec.Strategy.Canary.TrafficRoutings, newObj.Spec.Strategy.Canary.TrafficRoutings) { - return field.ErrorList{field.Forbidden(field.NewPath("Spec.Strategy.Canary.TrafficRoutings"), "Rollout 'Strategy.Canary.TrafficRoutings' field is immutable")} + if !reflect.DeepEqual(oldObj.Spec.Strategy.GetTrafficRouting(), newObj.Spec.Strategy.GetTrafficRouting()) { + return field.ErrorList{field.Forbidden(field.NewPath("Spec.Strategy.Canary|BlueGreen.TrafficRoutings"), "Rollout 'Strategy.Canary|BlueGreen.TrafficRoutings' field is immutable")} } - if oldObj.Spec.Strategy.Canary.EnableExtraWorkloadForCanary != newObj.Spec.Strategy.Canary.EnableExtraWorkloadForCanary { - return field.ErrorList{field.Forbidden(field.NewPath("Spec.Strategy.Canary"), "Rollout enableExtraWorkloadForCanary is immutable")} + if oldObj.Spec.Strategy.GetRollingStyle() != newObj.Spec.Strategy.GetRollingStyle() { + return field.ErrorList{field.Forbidden(field.NewPath("Spec.Strategy.Canary|BlueGreen"), "Rollout style and enableExtraWorkloadForCanary are immutable")} } } @@ -198,15 +197,32 @@ func validateRolloutSpecObjectRef(workloadRef *appsv1beta1.ObjectRef, fldPath *f } func validateRolloutSpecStrategy(strategy *appsv1beta1.RolloutStrategy, fldPath *field.Path) field.ErrorList { + if strategy.Canary == nil && strategy.BlueGreen == nil { + return field.ErrorList{field.Invalid(fldPath, nil, "Canary and BlueGreen cannot both be empty")} + } + if strategy.Canary != nil && strategy.BlueGreen != nil { + return field.ErrorList{field.Invalid(fldPath, nil, "Canary and BlueGreen cannot both be set")} + } + if strategy.BlueGreen != nil { + return validateRolloutSpecBlueGreenStrategy(strategy.BlueGreen, fldPath.Child("BlueGreen")) + } return validateRolloutSpecCanaryStrategy(strategy.Canary, fldPath.Child("Canary")) } +type TrafficRule string + +const ( + TrafficRuleCanary TrafficRule = "Canary" + TrafficRuleBlueGreen TrafficRule = "BlueGreen" + NoTraffic TrafficRule = "NoTraffic" +) + func validateRolloutSpecCanaryStrategy(canary *appsv1beta1.CanaryStrategy, fldPath *field.Path) field.ErrorList { - if canary == nil { - return field.ErrorList{field.Invalid(fldPath, nil, "Canary cannot be empty")} + trafficRule := NoTraffic + if len(canary.TrafficRoutings) > 0 { + trafficRule = TrafficRuleCanary } - - errList := validateRolloutSpecCanarySteps(canary.Steps, fldPath.Child("Steps"), len(canary.TrafficRoutings) > 0) + errList := validateRolloutSpecCanarySteps(canary.Steps, fldPath.Child("Steps"), trafficRule) if len(canary.TrafficRoutings) > 1 { errList = append(errList, field.Invalid(fldPath, canary.TrafficRoutings, "Rollout currently only support single TrafficRouting.")) } @@ -216,6 +232,21 @@ func validateRolloutSpecCanaryStrategy(canary *appsv1beta1.CanaryStrategy, fldPa return errList } +func validateRolloutSpecBlueGreenStrategy(blueGreen *appsv1beta1.BlueGreenStrategy, fldPath *field.Path) field.ErrorList { + trafficRule := NoTraffic + if len(blueGreen.TrafficRoutings) > 0 { + trafficRule = TrafficRuleBlueGreen + } + errList := validateRolloutSpecCanarySteps(blueGreen.Steps, fldPath.Child("Steps"), trafficRule) + if len(blueGreen.TrafficRoutings) > 1 { + errList = append(errList, field.Invalid(fldPath, blueGreen.TrafficRoutings, "Rollout currently only support single TrafficRouting.")) + } + for _, traffic := range blueGreen.TrafficRoutings { + errList = append(errList, validateRolloutSpecCanaryTraffic(traffic, fldPath.Child("TrafficRouting"))...) + } + return errList +} + func validateRolloutSpecCanaryTraffic(traffic appsv1beta1.TrafficRoutingRef, fldPath *field.Path) field.ErrorList { errList := field.ErrorList{} if len(traffic.Service) == 0 { @@ -240,7 +271,7 @@ func validateRolloutSpecCanaryTraffic(traffic appsv1beta1.TrafficRoutingRef, fld return errList } -func validateRolloutSpecCanarySteps(steps []appsv1beta1.CanaryStep, fldPath *field.Path, isTraffic bool) field.ErrorList { +func validateRolloutSpecCanarySteps(steps []appsv1beta1.CanaryStep, fldPath *field.Path, trafficRule TrafficRule) field.ErrorList { stepCount := len(steps) if stepCount == 0 { return field.ErrorList{field.Invalid(fldPath, steps, "The number of Canary.Steps cannot be empty")} @@ -258,14 +289,21 @@ func validateRolloutSpecCanarySteps(steps []appsv1beta1.CanaryStep, fldPath *fie return field.ErrorList{field.Invalid(fldPath.Index(i).Child("Replicas"), s.Replicas, `replicas must be positive number, or a percentage with "0%" < canaryReplicas <= "100%"`)} } - if !isTraffic { + if trafficRule == NoTraffic || s.Traffic == nil { continue } - if s.Traffic != nil { - is := intstr.FromString(*s.Traffic) - weight, err := intstr.GetScaledValueFromIntOrPercent(&is, 100, true) + is := intstr.FromString(*s.Traffic) + weight, err := intstr.GetScaledValueFromIntOrPercent(&is, 100, true) + switch trafficRule { + case TrafficRuleBlueGreen: + // traffic "0%" is allowed in blueGreen strategy + if err != nil || weight < 0 || weight > 100 { + return field.ErrorList{field.Invalid(fldPath.Index(i).Child("steps"), steps, `traffic must be percentage with "0%" <= traffic <= "100%" in blueGreen strategy`)} + } + default: + // traffic "0%" is not allowed in canary strategy if err != nil || weight <= 0 || weight > 100 { - return field.ErrorList{field.Invalid(fldPath.Index(i).Child("steps"), steps, `traffic must be percentage with "0%" < traffic <= "100%"`)} + return field.ErrorList{field.Invalid(fldPath.Index(i).Child("steps"), steps, `traffic must be percentage with "0%" < traffic <= "100%" in canary strategy`)} } } } diff --git a/test/e2e/deployment_test.go b/test/e2e/deployment_test.go index b857f929..c8379bc4 100644 --- a/test/e2e/deployment_test.go +++ b/test/e2e/deployment_test.go @@ -26,6 +26,16 @@ import ( var _ = SIGDescribe("Advanced Deployment", func() { var namespace string + + DumpAllResources := func() { + deploy := &apps.DeploymentList{} + k8sClient.List(context.TODO(), deploy, client.InNamespace(namespace)) + fmt.Println(util.DumpJSON(deploy)) + rs := &apps.ReplicaSetList{} + k8sClient.List(context.TODO(), rs, client.InNamespace(namespace)) + fmt.Println(util.DumpJSON(rs)) + } + defaultRetry := wait.Backoff{ Steps: 10, Duration: 10 * time.Millisecond, @@ -132,7 +142,12 @@ var _ = SIGDescribe("Advanced Deployment", func() { CheckReplicas := func(deployment *apps.Deployment, replicas, available, updated int32) { var clone *apps.Deployment + start := time.Now() Eventually(func() bool { + if start.Add(time.Minute * 2).Before(time.Now()) { + DumpAllResources() + Expect(true).Should(BeFalse()) + } clone = &apps.Deployment{} err := GetObject(deployment.Namespace, deployment.Name, clone) Expect(err).NotTo(HaveOccurred()) @@ -239,6 +254,7 @@ var _ = SIGDescribe("Advanced Deployment", func() { deployment.Namespace = namespace Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred()) CreateObject(deployment) + CheckReplicas(deployment, 5, 5, 5) UpdateDeployment(deployment, "version2") UpdatePartitionWithCheck(deployment, intstr.FromInt(0)) UpdatePartitionWithCheck(deployment, intstr.FromInt(1)) @@ -255,6 +271,7 @@ var _ = SIGDescribe("Advanced Deployment", func() { Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred()) deployment.Spec.Replicas = pointer.Int32(10) CreateObject(deployment) + CheckReplicas(deployment, 10, 10, 10) UpdateDeployment(deployment, "version2") UpdatePartitionWithCheck(deployment, intstr.FromString("0%")) UpdatePartitionWithCheck(deployment, intstr.FromString("40%")) @@ -287,6 +304,7 @@ var _ = SIGDescribe("Advanced Deployment", func() { `{"rollingStyle":"Partition","rollingUpdate":{"maxUnavailable":1,"maxSurge":0}}` deployment.Spec.MinReadySeconds = 10 CreateObject(deployment) + CheckReplicas(deployment, 5, 5, 5) UpdateDeployment(deployment, "version2") UpdatePartitionWithCheck(deployment, intstr.FromInt(0)) UpdatePartitionWithoutCheck(deployment, intstr.FromInt(3)) @@ -303,6 +321,7 @@ var _ = SIGDescribe("Advanced Deployment", func() { deployment.Namespace = namespace Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred()) CreateObject(deployment) + CheckReplicas(deployment, 5, 5, 5) UpdateDeployment(deployment, "version2") UpdatePartitionWithCheck(deployment, intstr.FromInt(0)) UpdatePartitionWithCheck(deployment, intstr.FromInt(2)) @@ -317,6 +336,7 @@ var _ = SIGDescribe("Advanced Deployment", func() { deployment.Namespace = namespace Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred()) CreateObject(deployment) + CheckReplicas(deployment, 5, 5, 5) UpdateDeployment(deployment, "version2") UpdatePartitionWithCheck(deployment, intstr.FromInt(0)) UpdatePartitionWithCheck(deployment, intstr.FromInt(2)) @@ -335,6 +355,7 @@ var _ = SIGDescribe("Advanced Deployment", func() { Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred()) deployment.Annotations["rollouts.kruise.io/deployment-strategy"] = `{"rollingUpdate":{"maxUnavailable":0,"maxSurge":1}}` CreateObject(deployment) + CheckReplicas(deployment, 5, 5, 5) UpdateDeployment(deployment, "version2", "busybox:not-exists") UpdatePartitionWithoutCheck(deployment, intstr.FromInt(1)) CheckReplicas(deployment, 6, 5, 1) diff --git a/test/e2e/rollout_test.go b/test/e2e/rollout_test.go index c8a7cae2..c2ae86c8 100644 --- a/test/e2e/rollout_test.go +++ b/test/e2e/rollout_test.go @@ -204,18 +204,25 @@ var _ = SIGDescribe("Rollout", func() { } ResumeRolloutCanary := func(name string) { + clone := &v1alpha1.Rollout{} + Expect(GetObject(name, clone)).NotTo(HaveOccurred()) + currentIndex := clone.Status.CanaryStatus.CurrentStepIndex Eventually(func() bool { clone := &v1alpha1.Rollout{} Expect(GetObject(name, clone)).NotTo(HaveOccurred()) - if clone.Status.CanaryStatus.CurrentStepState != v1alpha1.CanaryStepStatePaused { + if clone.Status.CanaryStatus.CurrentStepIndex == currentIndex && clone.Status.CanaryStatus.CurrentStepState == v1alpha1.CanaryStepStatePaused { + klog.Info("patch to stepReady") + body := fmt.Sprintf(`{"status":{"canaryStatus":{"currentStepState":"%s"}}}`, v1alpha1.CanaryStepStateReady) + Expect(k8sClient.Status().Patch(context.TODO(), clone, client.RawPatch(types.MergePatchType, []byte(body)))).NotTo(HaveOccurred()) + return false + } else { fmt.Println("resume rollout success, and CurrentStepState", util.DumpJSON(clone.Status)) return true } - - body := fmt.Sprintf(`{"status":{"canaryStatus":{"currentStepState":"%s"}}}`, v1alpha1.CanaryStepStateReady) - Expect(k8sClient.Status().Patch(context.TODO(), clone, client.RawPatch(types.MergePatchType, []byte(body)))).NotTo(HaveOccurred()) - return false - }, 10*time.Second, time.Second).Should(BeTrue()) + // interval was critical before: + // too small: StepReady could be overidden by StepPaused + // too big: StepReady could progress to StepPaused of next Step + }, 10*time.Second, 2*time.Second).Should(BeTrue()) } WaitDeploymentAllPodsReady := func(deployment *apps.Deployment) {