nextStepIndex default value from 0 to -1

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

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

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

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() {

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

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
 }

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

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)

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) {