nexstep initialization value from 0 tto -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`)}
 			}
 		}
 	}