support restart scale operation by modify targetSize of AppRollout (#1812)

* WIP first commit

fix

* more wait time

* fix flaky test

pkg/controller/core.oam.dev/v1alpha2/applicationrollout/applicationrollout_controller.go

@@ -214,15 +214,6 @@ func (r *Reconciler) DoReconcile(ctx context.Context, appRollout *v1beta1.AppRol
 	return result, nil
 }
 
-// check if either the source or the target of the appRollout has changed
-func isRolloutModified(appRollout v1beta1.AppRollout) bool {
-	return appRollout.Status.RollingState != v1alpha1.RolloutDeletingState &&
-		((appRollout.Status.LastUpgradedTargetAppRevision != "" &&
-			appRollout.Status.LastUpgradedTargetAppRevision != appRollout.Spec.TargetAppRevisionName) ||
-			(appRollout.Status.LastSourceAppRevision != "" &&
-				appRollout.Status.LastSourceAppRevision != appRollout.Spec.SourceAppRevisionName))
-}
-
 // handle adding and handle finalizer logic, it turns if we should continue to reconcile
 func (r *Reconciler) handleFinalizer(ctx context.Context, appRollout *v1beta1.AppRollout) (bool, reconcile.Result, error) {
 	if appRollout.DeletionTimestamp.IsZero() {

pkg/controller/core.oam.dev/v1alpha2/applicationrollout/applicationrollout_controller_test.go

@@ -19,6 +19,8 @@ package applicationrollout
 import (
 	"testing"
 
+	"k8s.io/utils/pointer"
+
 	"github.com/oam-dev/kubevela/apis/core.oam.dev/common"
 	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1"
 	"github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1"
@@ -135,6 +137,42 @@ func Test_isRolloutModified(t *testing.T) {
 			},
 			want: false,
 		},
+		"restart a scale operation": {
+			appRollout: v1beta1.AppRollout{
+				Spec: v1beta1.AppRolloutSpec{
+					TargetAppRevisionName: "target1",
+					RolloutPlan: v1alpha1.RolloutPlan{
+						TargetSize: pointer.Int32Ptr(1),
+					},
+				},
+				Status: common.AppRolloutStatus{
+					RolloutStatus: v1alpha1.RolloutStatus{
+						RollingState:      v1alpha1.RolloutSucceedState,
+						RolloutTargetSize: 2,
+					},
+					LastUpgradedTargetAppRevision: "target1",
+				},
+			},
+			want: true,
+		},
+		"scale have finished": {
+			appRollout: v1beta1.AppRollout{
+				Spec: v1beta1.AppRolloutSpec{
+					TargetAppRevisionName: "target1",
+					RolloutPlan: v1alpha1.RolloutPlan{
+						TargetSize: pointer.Int32Ptr(2),
+					},
+				},
+				Status: common.AppRolloutStatus{
+					RolloutStatus: v1alpha1.RolloutStatus{
+						RollingState:      v1alpha1.RolloutSucceedState,
+						RolloutTargetSize: 2,
+					},
+					LastUpgradedTargetAppRevision: "target1",
+				},
+			},
+			want: false,
+		},
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {

pkg/controller/core.oam.dev/v1alpha2/applicationrollout/helper.go

@@ -86,6 +86,16 @@ func handleRollingTerminated(appRollout v1beta1.AppRollout) bool {
 		appRollout.Status.RollingState == oamstd.RolloutFailedState {
 		if appRollout.Status.LastUpgradedTargetAppRevision == appRollout.Spec.TargetAppRevisionName &&
 			appRollout.Status.LastSourceAppRevision == appRollout.Spec.SourceAppRevisionName {
+			// spec.targetSize could be nil, If targetSize isn't nil and not equal to status.RolloutTargetSize it's
+			// means user have modified targetSize to restart an scale operation
+			if appRollout.Spec.RolloutPlan.TargetSize != nil {
+				if appRollout.Status.RolloutTargetSize == *appRollout.Spec.RolloutPlan.TargetSize {
+					klog.InfoS("rollout completed, no need to reconcile", "source", appRollout.Spec.SourceAppRevisionName,
+						"target", appRollout.Spec.TargetAppRevisionName)
+					return true
+				}
+				return false
+			}
 			klog.InfoS("rollout completed, no need to reconcile", "source", appRollout.Spec.SourceAppRevisionName,
 				"target", appRollout.Spec.TargetAppRevisionName)
 			return true
@@ -93,3 +103,17 @@ func handleRollingTerminated(appRollout v1beta1.AppRollout) bool {
 	}
 	return false
 }
+
+// check if either the source or the target of the appRollout has changed.
+// when reset the state machine, the controller will set the status.RolloutTargetSize as -1 in AppLocating phase
+// so we should ignore this case.
+// if status.RolloutTargetSize isn't equal to Spec.RolloutPlan.TargetSize, it's means user want trigger another scale operation.
+func isRolloutModified(appRollout v1beta1.AppRollout) bool {
+	return appRollout.Status.RollingState != oamstd.RolloutDeletingState &&
+		((appRollout.Status.LastUpgradedTargetAppRevision != "" &&
+			appRollout.Status.LastUpgradedTargetAppRevision != appRollout.Spec.TargetAppRevisionName) ||
+			(appRollout.Status.LastSourceAppRevision != "" &&
+				appRollout.Status.LastSourceAppRevision != appRollout.Spec.SourceAppRevisionName) ||
+			(appRollout.Spec.RolloutPlan.TargetSize != nil && appRollout.Status.RolloutTargetSize != -1 &&
+				appRollout.Status.RolloutTargetSize != *appRollout.Spec.RolloutPlan.TargetSize))
+}

pkg/controller/core.oam.dev/v1alpha2/applicationrollout/helper_test.go

@@ -138,8 +138,8 @@ func TestHandleTerminated(t *testing.T) {
 					TargetAppRevisionName: "v2",
 				},
 				Status: common.AppRolloutStatus{
-					LastSourceAppRevision:         "v2",
-					LastUpgradedTargetAppRevision: "v1",
+					LastSourceAppRevision:         "v1",
+					LastUpgradedTargetAppRevision: "v2",
 					RolloutStatus: oamstandard.RolloutStatus{
 						RollingState: oamstandard.RollingInBatchesState,
 					},
@@ -147,6 +147,42 @@ func TestHandleTerminated(t *testing.T) {
 			},
 			want: false,
 		},
+		"last scale have finished": {
+			rollout: v1beta1.AppRollout{
+				Spec: v1beta1.AppRolloutSpec{
+					TargetAppRevisionName: "v1",
+					RolloutPlan: oamstandard.RolloutPlan{
+						TargetSize: pointer.Int32Ptr(2),
+					},
+				},
+				Status: common.AppRolloutStatus{
+					LastUpgradedTargetAppRevision: "v1",
+					RolloutStatus: oamstandard.RolloutStatus{
+						RollingState:      oamstandard.RolloutSucceedState,
+						RolloutTargetSize: 2,
+					},
+				},
+			},
+			want: true,
+		},
+		"modify targetSize trigger scale operation again": {
+			rollout: v1beta1.AppRollout{
+				Spec: v1beta1.AppRolloutSpec{
+					TargetAppRevisionName: "v1",
+					RolloutPlan: oamstandard.RolloutPlan{
+						TargetSize: pointer.Int32Ptr(4),
+					},
+				},
+				Status: common.AppRolloutStatus{
+					LastUpgradedTargetAppRevision: "v1",
+					RolloutStatus: oamstandard.RolloutStatus{
+						RollingState:      oamstandard.RollingInBatchesState,
+						RolloutTargetSize: 2,
+					},
+				},
+			},
+			want: false,
+		},
 	}
 	for casename, c := range testcases {
 		got := handleRollingTerminated(c.rollout)

pkg/webhook/core.oam.dev/v1alpha2/applicationrollout/validation.go

@@ -155,6 +155,11 @@ func (h *ValidatingHandler) ValidateUpdate(new, old *v1beta1.AppRollout) field.E
 		if old.Spec.SourceAppRevisionName == new.Spec.SourceAppRevisionName &&
 			old.Spec.TargetAppRevisionName == new.Spec.TargetAppRevisionName {
 			if !apiequality.Semantic.DeepEqual(&old.Spec.RolloutPlan, &new.Spec.RolloutPlan) {
+				// here we allow user restart a scale rollout by modifying rollout targetSize
+				if old.Spec.RolloutPlan.TargetSize != nil && new.Spec.RolloutPlan.TargetSize != nil &&
+					*old.Spec.RolloutPlan.TargetSize != *new.Spec.RolloutPlan.TargetSize {
+					return errList
+				}
 				errList = append(errList, field.Invalid(fldPath, new.Spec,
 					"a successful or failed rollout cannot be modified without changing the target or the source"))
 				return errList

test/e2e-test/rollout_plan_test.go

@@ -181,7 +181,7 @@ var _ = Describe("Cloneset based rollout tests", func() {
 				err := k8sClient.Get(ctx, client.ObjectKey{Namespace: namespaceName, Name: appRolloutName}, &appRollout)
 				return apierrors.IsNotFound(err)
 			},
-			time.Second*3, time.Millisecond*500).Should(BeTrue())
+			time.Second*60, time.Millisecond*500).Should(BeTrue())
 	}
 
 	verifyRolloutSucceeded := func(targetAppName string) {
@@ -326,8 +326,20 @@ var _ = Describe("Cloneset based rollout tests", func() {
 
 	AfterEach(func() {
 		By("Clean up resources after a test")
-		k8sClient.Delete(ctx, &app)
-		k8sClient.Delete(ctx, &appRollout)
+		Eventually(func() error {
+			err := k8sClient.Delete(ctx, &app)
+			if err == nil || apierrors.IsNotFound(err) {
+				return nil
+			}
+			return err
+		}, 15*time.Second, 300*time.Microsecond).Should(BeNil())
+		Eventually(func() error {
+			err := k8sClient.Delete(ctx, &appRollout)
+			if err == nil || apierrors.IsNotFound(err) {
+				return nil
+			}
+			return err
+		}, 15*time.Second, 300*time.Microsecond).Should(BeNil())
 		verifyRolloutDeleted()
 		By(fmt.Sprintf("Delete the entire namespaceName %s", ns.Name))
 		// delete the namespaceName with all its resources
@@ -690,6 +702,48 @@ var _ = Describe("Cloneset based rollout tests", func() {
 		}, time.Second*30, 300*time.Microsecond).Should(util.NotFoundMatcher{})
 	})
 
+	It("Test scale again by modify targetSize", func() {
+		var err error
+		CreateClonesetDef()
+		applySourceApp("app-no-replica.yaml")
+		By("Apply the application rollout go directly to the target")
+		appRollout = v1beta1.AppRollout{}
+		Expect(common.ReadYamlToObject("testdata/rollout/cloneset/appRolloutScale.yaml", &appRollout)).Should(BeNil())
+		appRollout.Namespace = namespaceName
+		appRollout.Spec.SourceAppRevisionName = ""
+		appRollout.Spec.TargetAppRevisionName = utils.ConstructRevisionName(app.GetName(), 1)
+		appRollout.Spec.RolloutPlan.TargetSize = pointer.Int32Ptr(3)
+		appRollout.Spec.RolloutPlan.BatchPartition = nil
+		By("create appRollout initial targetSize is 3")
+		createAppRolling(&appRollout)
+		appRolloutName = appRollout.Name
+		verifyRolloutSucceeded(appRollout.Spec.TargetAppRevisionName)
+		By("modify appRollout targetSize to 5")
+		Eventually(func() error {
+			if err = k8sClient.Get(ctx, client.ObjectKey{Namespace: namespaceName, Name: appRolloutName}, &appRollout); err != nil {
+				return err
+			}
+			appRollout.Spec.RolloutPlan.TargetSize = pointer.Int32Ptr(5)
+			if err = k8sClient.Update(ctx, &appRollout); err != nil {
+				return err
+			}
+			return nil
+		}, 60*time.Second, 300*time.Microsecond).Should(BeNil())
+		verifyRolloutSucceeded(appRollout.Spec.TargetAppRevisionName)
+		By("modify appRollout targetSize to 7")
+		Eventually(func() error {
+			if err = k8sClient.Get(ctx, client.ObjectKey{Namespace: namespaceName, Name: appRolloutName}, &appRollout); err != nil {
+				return err
+			}
+			appRollout.Spec.RolloutPlan.TargetSize = pointer.Int32Ptr(7)
+			if err = k8sClient.Update(ctx, &appRollout); err != nil {
+				return err
+			}
+			return nil
+		}, 60*time.Second, 300*time.Microsecond).Should(BeNil())
+		verifyRolloutSucceeded(appRollout.Spec.TargetAppRevisionName)
+	})
+
 	PIt("Test rolling by changing the definition", func() {
 		CreateClonesetDef()
 		applySourceApp("app-source.yaml")

test/e2e-test/testdata/rollout/cloneset/appRolloutScale.yaml

@@ -0,0 +1,14 @@
+apiVersion: core.oam.dev/v1beta1
+kind: AppRollout
+metadata:
+  name: rolling-e2e-test
+spec:
+  # application (revision) reference
+  targetAppRevisionName: test-e2e-rolling-v1
+  componentList:
+    - metrics-provider
+  rolloutPlan:
+    rolloutStrategy: "IncreaseFirst"
+    rolloutBatches:
+      - replicas: 2
+    batchPartition: 3