diff --git a/api/v1alpha1/rollout_types.go b/api/v1alpha1/rollout_types.go index 1787028d..6a5edfad 100644 --- a/api/v1alpha1/rollout_types.go +++ b/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 diff --git a/config/crd/bases/rollouts.kruise.io_batchreleases.yaml b/config/crd/bases/rollouts.kruise.io_batchreleases.yaml index d737c7f9..86e19774 100644 --- a/config/crd/bases/rollouts.kruise.io_batchreleases.yaml +++ b/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 diff --git a/config/crd/bases/rollouts.kruise.io_rollouts.yaml b/config/crd/bases/rollouts.kruise.io_rollouts.yaml index a557e25f..9819a100 100644 --- a/config/crd/bases/rollouts.kruise.io_rollouts.yaml +++ b/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 diff --git a/pkg/controller/rollout/batchrelease/batchrelease.go b/pkg/controller/rollout/batchrelease/batchrelease.go index 5b292434..99de7446 100644 --- a/pkg/controller/rollout/batchrelease/batchrelease.go +++ b/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) diff --git a/pkg/controller/rollout/batchrelease/inner_batchrelease.go b/pkg/controller/rollout/batchrelease/inner_batchrelease.go index 12e8cab9..d81fa4e2 100644 --- a/pkg/controller/rollout/batchrelease/inner_batchrelease.go +++ b/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, diff --git a/pkg/controller/rollout/canary.go b/pkg/controller/rollout/canary.go index e272a6e2..605fb7ba 100644 --- a/pkg/controller/rollout/canary.go +++ b/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 } diff --git a/pkg/controller/rollout/progressing.go b/pkg/controller/rollout/progressing.go index 53fe8380..066a1af8 100644 --- a/pkg/controller/rollout/progressing.go +++ b/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) { diff --git a/pkg/controller/rollout/rollout_controller_test.go b/pkg/controller/rollout/rollout_controller_test.go index d0dfa665..e0831946 100644 --- a/pkg/controller/rollout/rollout_controller_test.go +++ b/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", diff --git a/pkg/controller/rollout/trafficrouting.go b/pkg/controller/rollout/trafficrouting.go index 765c3600..e2ceaa02 100644 --- a/pkg/controller/rollout/trafficrouting.go +++ b/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 } diff --git a/pkg/webhook/rollout/validating/rollout_create_update_handler.go b/pkg/webhook/rollout/validating/rollout_create_update_handler.go index 5cd5442c..ebc57602 100644 --- a/pkg/webhook/rollout/validating/rollout_create_update_handler.go +++ b/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"))...) } diff --git a/pkg/webhook/rollout/validating/rollout_create_update_handler_test.go b/pkg/webhook/rollout/validating/rollout_create_update_handler_test.go index 743e115d..0c8b97bc 100644 --- a/pkg/webhook/rollout/validating/rollout_create_update_handler_test.go +++ b/pkg/webhook/rollout/validating/rollout_create_update_handler_test.go @@ -29,7 +29,6 @@ var ( }, Spec: appsv1alpha1.RolloutSpec{ ObjectRef: appsv1alpha1.ObjectRef{ - Type: appsv1alpha1.WorkloadRefType, WorkloadRef: &appsv1alpha1.WorkloadRef{ APIVersion: apps.SchemeGroupVersion.String(), Kind: "Deployment", @@ -37,7 +36,6 @@ var ( }, }, 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, diff --git a/pkg/webhook/workload/mutating/workload_update_handler.go b/pkg/webhook/workload/mutating/workload_update_handler.go index 8abefcb8..8b394ea5 100644 --- a/pkg/webhook/workload/mutating/workload_update_handler.go +++ b/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 diff --git a/pkg/webhook/workload/mutating/workload_update_handler_test.go b/pkg/webhook/workload/mutating/workload_update_handler_test.go index 885b3b3d..5d5f632b 100644 --- a/pkg/webhook/workload/mutating/workload_update_handler_test.go +++ b/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", diff --git a/test/e2e/rollout_test.go b/test/e2e/rollout_test.go index 709e47f2..1f778d91 100644 --- a/test/e2e/rollout_test.go +++ b/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) {