From 70eeec4c894fed65707b1d3e7ca3be65de704531 Mon Sep 17 00:00:00 2001 From: wyike <77846369+wangyikewxgm@users.noreply.github.com> Date: Thu, 17 Jun 2021 22:30:07 +0800 Subject: [PATCH] support restart scale operation by modify targetSize of AppRollout (#1812) * WIP first commit fix * more wait time * fix flaky test --- .../applicationrollout_controller.go | 9 --- .../applicationrollout_controller_test.go | 38 ++++++++++++ .../v1alpha2/applicationrollout/helper.go | 24 ++++++++ .../applicationrollout/helper_test.go | 40 ++++++++++++- .../v1alpha2/applicationrollout/validation.go | 5 ++ test/e2e-test/rollout_plan_test.go | 60 ++++++++++++++++++- .../rollout/cloneset/appRolloutScale.yaml | 14 +++++ 7 files changed, 176 insertions(+), 14 deletions(-) create mode 100644 test/e2e-test/testdata/rollout/cloneset/appRolloutScale.yaml diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/applicationrollout_controller.go b/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/applicationrollout_controller.go index 6d6c101d0..f01d213a9 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/applicationrollout_controller.go +++ b/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() { diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/applicationrollout_controller_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/applicationrollout_controller_test.go index 4128154f9..cc75a54ab 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/applicationrollout_controller_test.go +++ b/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) { diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/helper.go b/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/helper.go index 6464a0eb2..ba3ed67b0 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/helper.go +++ b/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)) +} diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/helper_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/helper_test.go index c726eca04..c861d630a 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationrollout/helper_test.go +++ b/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) diff --git a/pkg/webhook/core.oam.dev/v1alpha2/applicationrollout/validation.go b/pkg/webhook/core.oam.dev/v1alpha2/applicationrollout/validation.go index 812867927..a054ce5fb 100644 --- a/pkg/webhook/core.oam.dev/v1alpha2/applicationrollout/validation.go +++ b/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 diff --git a/test/e2e-test/rollout_plan_test.go b/test/e2e-test/rollout_plan_test.go index f7640196e..9ada7ddea 100644 --- a/test/e2e-test/rollout_plan_test.go +++ b/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") diff --git a/test/e2e-test/testdata/rollout/cloneset/appRolloutScale.yaml b/test/e2e-test/testdata/rollout/cloneset/appRolloutScale.yaml new file mode 100644 index 000000000..6d9454494 --- /dev/null +++ b/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 \ No newline at end of file