Optimize rollout state transition related code

Signed-off-by: liheng.zms <[email protected]>

api/v1alpha1/rollout_types.go

@@ -36,9 +36,6 @@ type RolloutSpec struct {
 }
 
 type ObjectRef struct {
-	// workloadRef, revisionRef
-	// default is workloadRef
-	Type ObjectRefType `json:"type,omitempty"`
 	// WorkloadRef contains enough information to let you identify a workload for Rollout
 	// Batch release of the bypass
 	WorkloadRef *WorkloadRef `json:"workloadRef,omitempty"`
@@ -75,9 +72,6 @@ type RolloutStrategy struct {
 	// Paused indicates that the Rollout is paused.
 	// Default value is false
 	Paused bool `json:"paused,omitempty"`
-	// canary, BlueGreenPlan
-	// Default value is canary
-	Type RolloutStrategyType `json:"type,omitempty"`
 	// +optional
 	Canary *CanaryStrategy `json:"canary,omitempty"`
 	// +optional

config/crd/bases/rollouts.kruise.io_batchreleases.yaml

@@ -102,9 +102,6 @@ spec:
                 description: TargetRef contains the GVK and name of the workload that
                   we need to upgrade to.
                 properties:
-                  type:
-                    description: workloadRef, revisionRef default is workloadRef
-                    type: string
                   workloadRef:
                     description: WorkloadRef contains enough information to let you
                       identify a workload for Rollout Batch release of the bypass

config/crd/bases/rollouts.kruise.io_rollouts.yaml

@@ -61,9 +61,6 @@ spec:
                   Important: Run "make" to regenerate code after modifying this file
                   ObjectRef indicates workload'
                 properties:
-                  type:
-                    description: workloadRef, revisionRef default is workloadRef
-                    type: string
                   workloadRef:
                     description: WorkloadRef contains enough information to let you
                       identify a workload for Rollout Batch release of the bypass
@@ -165,9 +162,6 @@ spec:
                     description: Paused indicates that the Rollout is paused. Default
                       value is false
                     type: boolean
-                  type:
-                    description: canary, BlueGreenPlan Default value is canary
-                    type: string
                 type: object
             required:
             - objectRef

pkg/controller/rollout/batchrelease/batchrelease.go

@@ -21,8 +21,8 @@ import rolloutv1alpha1 "github.com/openkruise/rollouts/api/v1alpha1"
 // BatchRelease is not the actual controller of the BatchRelease controller,
 // but rather the ability to interact with the BatchRelease controller through the BatchRelease CRD to achieve a batch release
 type BatchRelease interface {
-	// Verify will create batchRelease or update batchRelease steps configuration
-	// isLatest(Bool) determines if the batchRelease configuration is the latest
+	// Verify will create batchRelease or update batchRelease steps configuration and
+	// return whether the batchRelease configuration is consistent with the rollout step
 	Verify(index int32) (bool, error)
 
 	// 1. Promote release workload in step(index), 1<=index<=len(step)

pkg/controller/rollout/batchrelease/inner_batchrelease.go

@@ -276,7 +276,6 @@ func createBatchRelease(rollout *rolloutv1alpha1.Rollout, batchName string) *rol
 		},
 		Spec: rolloutv1alpha1.BatchReleaseSpec{
 			TargetRef: rolloutv1alpha1.ObjectRef{
-				Type: rolloutv1alpha1.WorkloadRefType,
 				WorkloadRef: &rolloutv1alpha1.WorkloadRef{
 					APIVersion: rollout.Spec.ObjectRef.WorkloadRef.APIVersion,
 					Kind:       rollout.Spec.ObjectRef.WorkloadRef.Kind,

pkg/controller/rollout/canary.go

@@ -201,7 +201,7 @@ func (r *rolloutContext) doCanaryPaused() (bool, error) {
 	// need manual confirmation
 	if currentStep.Pause.Duration == nil {
 		klog.Infof("rollout(%s/%s) don't set pause duration, and need manual confirmation", r.rollout.Namespace, r.rollout.Name)
-		cond.Message = fmt.Sprintf("Rollout is in step(%d/%d), and you need manually confirm(kube-cli approve) to enter the next step", canaryStatus.CurrentStepIndex, steps)
+		cond.Message = fmt.Sprintf("Rollout is in step(%d/%d), and you need manually confirm to enter the next step", canaryStatus.CurrentStepIndex, steps)
 		r.newStatus.Message = cond.Message
 		return false, nil
 	}

pkg/controller/rollout/progressing.go

@@ -73,16 +73,16 @@ func (r *RolloutReconciler) reconcileRolloutProgressing(rollout *rolloutv1alpha1
 		}
 
 	case rolloutv1alpha1.ProgressingReasonInRolling:
-		// paused rollout progress
-		if rollout.Spec.Strategy.Paused {
-			klog.Infof("rollout(%s/%s) is Progressing, but paused", rollout.Namespace, rollout.Name)
-			progressingStateTransition(newStatus, corev1.ConditionFalse, rolloutv1alpha1.ProgressingReasonPaused, "Rollout has been paused, you can resume it by kube-cli")
-			// rollout canceled, indicates rollback(v1 -> v2 -> v1)
-		} else if workload.IsInRollback {
+		// rollout canceled, indicates rollback(v1 -> v2 -> v1)
+		if workload.IsInRollback {
 			newStatus.CanaryStatus.CanaryRevision = workload.CanaryRevision
 			r.Recorder.Eventf(rollout, corev1.EventTypeNormal, "Progressing", "workload has been rollback, then rollout is canceled")
 			klog.Infof("rollout(%s/%s) workload has been rollback, then rollout canceled", rollout.Namespace, rollout.Name)
 			progressingStateTransition(newStatus, corev1.ConditionFalse, rolloutv1alpha1.ProgressingReasonCancelling, "The workload has been rolled back and the rollout process will be cancelled")
+			// paused rollout progress
+		} else if rollout.Spec.Strategy.Paused {
+			klog.Infof("rollout(%s/%s) is Progressing, but paused", rollout.Namespace, rollout.Name)
+			progressingStateTransition(newStatus, corev1.ConditionFalse, rolloutv1alpha1.ProgressingReasonPaused, "Rollout has been paused, you can resume it by kube-cli")
 			// In case of continuous publishing(v1 -> v2 -> v3), then restart publishing
 		} else if newStatus.CanaryStatus.CanaryRevision != "" && workload.CanaryRevision != newStatus.CanaryStatus.CanaryRevision {
 			r.Recorder.Eventf(rollout, corev1.EventTypeNormal, "Progressing", "workload continuous publishing canaryRevision, then restart publishing")
@@ -143,7 +143,14 @@ func (r *RolloutReconciler) reconcileRolloutProgressing(rollout *rolloutv1alpha1
 		}
 
 	case rolloutv1alpha1.ProgressingReasonPaused:
-		if !rollout.Spec.Strategy.Paused {
+		// rollout canceled, indicates rollback(v1 -> v2 -> v1)
+		if workload.IsInRollback {
+			newStatus.CanaryStatus.CanaryRevision = workload.CanaryRevision
+			r.Recorder.Eventf(rollout, corev1.EventTypeNormal, "Progressing", "workload has been rollback, then rollout is canceled")
+			klog.Infof("rollout(%s/%s) workload has been rollback, then rollout canceled", rollout.Namespace, rollout.Name)
+			progressingStateTransition(newStatus, corev1.ConditionFalse, rolloutv1alpha1.ProgressingReasonCancelling, "The workload has been rolled back and the rollout process will be cancelled")
+			// from paused to inRolling
+		} else if !rollout.Spec.Strategy.Paused {
 			klog.Infof("rollout(%s/%s) is Progressing, but paused", rollout.Namespace, rollout.Name)
 			progressingStateTransition(newStatus, corev1.ConditionFalse, rolloutv1alpha1.ProgressingReasonInRolling, "")
 		}
@@ -188,10 +195,7 @@ func progressingStateTransition(status *rolloutv1alpha1.RolloutStatus, condStatu
 
 func (r *RolloutReconciler) doProgressingInitializing(rollout *rolloutv1alpha1.Rollout, newStatus *rolloutv1alpha1.RolloutStatus) (bool, string, error) {
 	// canary release
-	if rollout.Spec.Strategy.Type == "" || rollout.Spec.Strategy.Type == rolloutv1alpha1.RolloutStrategyCanary {
-		return r.verifyCanaryStrategy(rollout, newStatus)
-	}
-	return true, "", nil
+	return r.verifyCanaryStrategy(rollout, newStatus)
 }
 
 func (r *RolloutReconciler) doProgressingInRolling(rollout *rolloutv1alpha1.Rollout, newStatus *rolloutv1alpha1.RolloutStatus) (*time.Time, error) {

pkg/controller/rollout/rollout_controller_test.go

@@ -37,7 +37,6 @@ var (
 		},
 		Spec: rolloutv1alpha1.RolloutSpec{
 			ObjectRef: rolloutv1alpha1.ObjectRef{
-				Type: rolloutv1alpha1.WorkloadRefType,
 				WorkloadRef: &rolloutv1alpha1.WorkloadRef{
 					APIVersion: "apps/v1",
 					Kind:       "Deployment",
@@ -103,7 +102,6 @@ var (
 		},
 		Spec: rolloutv1alpha1.BatchReleaseSpec{
 			TargetRef: rolloutv1alpha1.ObjectRef{
-				Type: rolloutv1alpha1.WorkloadRefType,
 				WorkloadRef: &rolloutv1alpha1.WorkloadRef{
 					APIVersion: "apps/v1",
 					Kind:       "Deployment",

pkg/controller/rollout/trafficrouting.go

@@ -36,7 +36,7 @@ import (
 )
 
 func (r *rolloutContext) doCanaryTrafficRouting() (bool, error) {
-	if r.rollout.Spec.Strategy.Canary.TrafficRoutings == nil {
+	if len(r.rollout.Spec.Strategy.Canary.TrafficRoutings) == 0 {
 		return true, nil
 	}
 
@@ -134,7 +134,7 @@ func (r *rolloutContext) doCanaryTrafficRouting() (bool, error) {
 }
 
 func (r *rolloutContext) restoreStableService() (bool, error) {
-	if r.rollout.Spec.Strategy.Canary.TrafficRoutings == nil {
+	if len(r.rollout.Spec.Strategy.Canary.TrafficRoutings) == 0 {
 		return true, nil
 	}
 
@@ -179,7 +179,7 @@ func (r *rolloutContext) restoreStableService() (bool, error) {
 }
 
 func (r *rolloutContext) doFinalisingTrafficRouting() (bool, error) {
-	if r.rollout.Spec.Strategy.Canary.TrafficRoutings == nil {
+	if len(r.rollout.Spec.Strategy.Canary.TrafficRoutings) == 0 {
 		return true, nil
 	}
 

pkg/webhook/rollout/validating/rollout_create_update_handler.go

@@ -92,14 +92,9 @@ func (h *RolloutCreateUpdateHandler) validateRolloutUpdate(oldObj, newObj *appsv
 		if !reflect.DeepEqual(oldObj.Spec.ObjectRef, newObj.Spec.ObjectRef) {
 			return field.ErrorList{field.Forbidden(field.NewPath("Spec.ObjectRef"), "Rollout 'ObjectRef' field is immutable")}
 		}
-		if oldObj.Spec.Strategy.Type != newObj.Spec.Strategy.Type {
-			return field.ErrorList{field.Forbidden(field.NewPath("Spec.Strategy"), "Rollout 'Strategy.type' field is immutable")}
-		}
 		// canary strategy
-		if oldObj.Spec.Strategy.Type != appsv1alpha1.RolloutStrategyBlueGreen {
-			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.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")}
 		}
 	}
 
@@ -147,24 +142,14 @@ func validateRolloutSpec(rollout *appsv1alpha1.Rollout, fldPath *field.Path) fie
 }
 
 func validateRolloutSpecObjectRef(objectRef *appsv1alpha1.ObjectRef, fldPath *field.Path) field.ErrorList {
-	switch objectRef.Type {
-	case "", appsv1alpha1.WorkloadRefType:
-		if objectRef.WorkloadRef == nil || (objectRef.WorkloadRef.Kind != "Deployment" && objectRef.WorkloadRef.Kind != "CloneSet") {
-			return field.ErrorList{field.Invalid(fldPath.Child("WorkloadRef"), objectRef.WorkloadRef, "WorkloadRef only support 'Deployments', 'CloneSet'")}
-		}
-	default:
-		return field.ErrorList{field.Invalid(fldPath.Child("Type"), objectRef.Type, "ObjectRef only support 'workloadRef' type")}
+	if objectRef.WorkloadRef == nil || (objectRef.WorkloadRef.Kind != "Deployment" && objectRef.WorkloadRef.Kind != "CloneSet") {
+		return field.ErrorList{field.Invalid(fldPath.Child("WorkloadRef"), objectRef.WorkloadRef, "WorkloadRef only support 'Deployments', 'CloneSet'")}
 	}
 	return nil
 }
 
 func validateRolloutSpecStrategy(strategy *appsv1alpha1.RolloutStrategy, fldPath *field.Path) field.ErrorList {
-	switch strategy.Type {
-	case "", appsv1alpha1.RolloutStrategyCanary:
-		return validateRolloutSpecCanaryStrategy(strategy.Canary, fldPath.Child("Canary"))
-	default:
-		return field.ErrorList{field.Invalid(fldPath.Child("Type"), strategy.Type, "Strategy type only support 'canary'")}
-	}
+	return validateRolloutSpecCanaryStrategy(strategy.Canary, fldPath.Child("Canary"))
 }
 
 func validateRolloutSpecCanaryStrategy(canary *appsv1alpha1.CanaryStrategy, fldPath *field.Path) field.ErrorList {
@@ -173,6 +158,9 @@ func validateRolloutSpecCanaryStrategy(canary *appsv1alpha1.CanaryStrategy, fldP
 	}
 
 	errList := validateRolloutSpecCanarySteps(canary.Steps, fldPath.Child("Steps"))
+	if len(canary.TrafficRoutings) > 1 {
+		errList = append(errList, field.Invalid(fldPath, canary.TrafficRoutings, "Rollout currently only support single TrafficRouting."))
+	}
 	for _, traffic := range canary.TrafficRoutings {
 		errList = append(errList, validateRolloutSpecCanaryTraffic(traffic, fldPath.Child("TrafficRouting"))...)
 	}

pkg/webhook/rollout/validating/rollout_create_update_handler_test.go

@@ -29,15 +29,13 @@ var (
 		},
 		Spec: appsv1alpha1.RolloutSpec{
 			ObjectRef: appsv1alpha1.ObjectRef{
-				Type: appsv1alpha1.WorkloadRefType,
 				WorkloadRef: &appsv1alpha1.WorkloadRef{
 					APIVersion: apps.SchemeGroupVersion.String(),
 					Kind:       "Deployment",
 					Name:       "deployment-demo",
 				},
 			},
 			Strategy: appsv1alpha1.RolloutStrategy{
-				Type: appsv1alpha1.RolloutStrategyCanary,
 				Canary: &appsv1alpha1.CanaryStrategy{
 					Steps: []appsv1alpha1.CanaryStep{
 						{
@@ -234,24 +232,6 @@ func TestRolloutValidateCreate(t *testing.T) {
 		//		return []client.Object{object}
 		//	},
 		//},
-		{
-			Name:    "Wrong objectRef type",
-			Succeed: false,
-			GetObject: func() []client.Object {
-				object := rollout.DeepCopy()
-				object.Spec.ObjectRef.Type = "Whatever"
-				return []client.Object{object}
-			},
-		},
-		{
-			Name:    "Wrong strategy type",
-			Succeed: false,
-			GetObject: func() []client.Object {
-				object := rollout.DeepCopy()
-				object.Spec.Strategy.Type = "Whatever"
-				return []client.Object{object}
-			},
-		},
 		{
 			Name:    "Wrong Traffic type",
 			Succeed: false,

pkg/webhook/workload/mutating/workload_update_handler.go

@@ -185,8 +185,7 @@ func (h *WorkloadHandler) fetchMatchedRollout(obj client.Object) (*appsv1alpha1.
 	}
 	for i := range rolloutList.Items {
 		rollout := &rolloutList.Items[i]
-		if !rollout.DeletionTimestamp.IsZero() || rollout.Spec.ObjectRef.Type == appsv1alpha1.RevisionRefType ||
-			rollout.Spec.ObjectRef.WorkloadRef == nil {
+		if !rollout.DeletionTimestamp.IsZero() || rollout.Spec.ObjectRef.WorkloadRef == nil {
 			continue
 		}
 		ref := rollout.Spec.ObjectRef.WorkloadRef

pkg/webhook/workload/mutating/workload_update_handler_test.go

@@ -159,7 +159,6 @@ var (
 		},
 		Spec: appsv1alpha1.RolloutSpec{
 			ObjectRef: appsv1alpha1.ObjectRef{
-				Type: appsv1alpha1.WorkloadRefType,
 				WorkloadRef: &appsv1alpha1.WorkloadRef{
 					APIVersion: "apps/v1",
 					Kind:       "Deployment",

test/e2e/rollout_test.go

@@ -122,15 +122,18 @@ var _ = SIGDescribe("Rollout", func() {
 	}
 
 	ResumeRolloutCanary := func(name string) {
-		Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		Eventually(func() bool {
 			clone := &rolloutsv1alpha1.Rollout{}
-			err := GetObject(name, clone)
-			if err != nil {
-				return err
+			Expect(GetObject(name, clone)).NotTo(HaveOccurred())
+			if clone.Status.CanaryStatus.CurrentStepState != rolloutsv1alpha1.CanaryStepStatePaused {
+				fmt.Println("resume rollout success, and CurrentStepState", util.DumpJSON(clone.Status))
+				return true
 			}
+
 			body := fmt.Sprintf(`{"status":{"canaryStatus":{"currentStepState":"%s"}}}`, rolloutsv1alpha1.CanaryStepStateReady)
-			return k8sClient.Status().Patch(context.TODO(), clone, client.RawPatch(types.MergePatchType, []byte(body)))
-		})).NotTo(HaveOccurred())
+			Expect(k8sClient.Status().Patch(context.TODO(), clone, client.RawPatch(types.MergePatchType, []byte(body)))).NotTo(HaveOccurred())
+			return false
+		}, 10*time.Second, time.Second).Should(BeTrue())
 	}
 
 	WaitDeploymentAllPodsReady := func(deployment *apps.Deployment) {