diff --git a/docs/examples/deployment-rollout/README.md b/docs/examples/deployment-rollout/README.md index 89c712923..f2f28569a 100644 --- a/docs/examples/deployment-rollout/README.md +++ b/docs/examples/deployment-rollout/README.md @@ -12,24 +12,13 @@ kubectl apply -f docs/examples/deployment-rollout/webservice-definition.yaml ```shell kubectl apply -f docs/examples/deployment-rollout/app-source.yaml ``` -Wait for the application's status to be "running" -3. Prepare the application for rolling out -```shell -kubectl apply -f docs/examples/deployment-rollout/app-source-prep.yaml -``` - -4. Modify the application image and apply +3. Modify the application image and apply ```shell kubectl apply -f docs/examples/deployment-rollout/app-target.yaml ``` -5. Mark the application as normal -```shell -kubectl apply -f docs/examples/deployment-rollout/app-target-done.yaml -``` - -6. Apply the application deployment with pause +4. Apply the application deployment with pause ```shell kubectl apply -f docs/examples/deployment-rollout/app-rollout-pause.yaml ``` diff --git a/docs/examples/deployment-rollout/app-rollout-pause.yaml b/docs/examples/deployment-rollout/app-rollout-pause.yaml index 15bc13837..97668113e 100644 --- a/docs/examples/deployment-rollout/app-rollout-pause.yaml +++ b/docs/examples/deployment-rollout/app-rollout-pause.yaml @@ -5,7 +5,6 @@ metadata: spec: # application (revision) reference targetAppRevisionName: test-rolling-v2 - sourceAppRevisionName: test-rolling-v1 # HPA reference (optional) componentList: - metrics-provider diff --git a/docs/examples/deployment-rollout/app-source-prep.yaml b/docs/examples/deployment-rollout/app-source-prep.yaml deleted file mode 100644 index fdb055c9b..000000000 --- a/docs/examples/deployment-rollout/app-source-prep.yaml +++ /dev/null @@ -1,18 +0,0 @@ -apiVersion: core.oam.dev/v1beta1 -kind: Application -metadata: - name: test-rolling - annotations: - "app.oam.dev/rolling-components": "metrics-provider" - "app.oam.dev/rollout-template": "true" -spec: - components: - - name: metrics-provider - type: webservice - properties: - cmd: - - ./podinfo - - stress-cpu=1 - image: stefanprodan/podinfo:4.0.6 - port: 8080 - updateStrategyType: InPlaceIfPossible \ No newline at end of file diff --git a/docs/examples/deployment-rollout/app-source.yaml b/docs/examples/deployment-rollout/app-source.yaml index a64ecb67b..1607b9dba 100644 --- a/docs/examples/deployment-rollout/app-source.yaml +++ b/docs/examples/deployment-rollout/app-source.yaml @@ -2,6 +2,8 @@ apiVersion: core.oam.dev/v1beta1 kind: Application metadata: name: test-rolling + annotations: + "app.oam.dev/rollout-template": "true" spec: components: - name: metrics-provider @@ -11,5 +13,4 @@ spec: - ./podinfo - stress-cpu=1 image: stefanprodan/podinfo:4.0.6 - port: 8080 - updateStrategyType: InPlaceIfPossible \ No newline at end of file + port: 8080 \ No newline at end of file diff --git a/docs/examples/deployment-rollout/app-target-done.yaml b/docs/examples/deployment-rollout/app-target-done.yaml deleted file mode 100644 index 73047aa78..000000000 --- a/docs/examples/deployment-rollout/app-target-done.yaml +++ /dev/null @@ -1,15 +0,0 @@ -apiVersion: core.oam.dev/v1beta1 -kind: Application -metadata: - name: test-rolling -spec: - components: - - name: metrics-provider - type: webservice - properties: - cmd: - - ./podinfo - - stress-cpu=1 - image: stefanprodan/podinfo:5.0.2 - port: 8080 - updateStrategyType: InPlaceIfPossible \ No newline at end of file diff --git a/docs/examples/deployment-rollout/app-target.yaml b/docs/examples/deployment-rollout/app-target.yaml index 31b245edf..541351bb6 100644 --- a/docs/examples/deployment-rollout/app-target.yaml +++ b/docs/examples/deployment-rollout/app-target.yaml @@ -3,7 +3,6 @@ kind: Application metadata: name: test-rolling annotations: - "app.oam.dev/rolling-components": "metrics-provider" "app.oam.dev/rollout-template": "true" spec: components: @@ -14,5 +13,4 @@ spec: - ./podinfo - stress-cpu=1 image: stefanprodan/podinfo:5.0.2 - port: 8080 - updateStrategyType: InPlaceIfPossible \ No newline at end of file + port: 8080 \ No newline at end of file diff --git a/docs/examples/deployment-rollout/webservice-definition.yaml b/docs/examples/deployment-rollout/webservice-definition.yaml index 168db19be..de7368c70 100644 --- a/docs/examples/deployment-rollout/webservice-definition.yaml +++ b/docs/examples/deployment-rollout/webservice-definition.yaml @@ -13,14 +13,18 @@ spec: schematic: cue: template: | + import ( + apps "kube/apps/v1" + ) + output: apps.#Deployment output: { - apiVersion: "apps/v1" - kind: "Deployment" spec: { selector: matchLabels: { "app.oam.dev/component": context.name } - replicas: parameter.replicas + if parameter["replicas"] != _|_ { + replicas: parameter.replicas + } template: { metadata: labels: { "app.oam.dev/component": context.name @@ -91,6 +95,6 @@ spec: // +usage=Number of CPU units for the service, like `0.5` (0.5 CPU core), `1` (1 CPU core) cpu?: string // +usage=Number of pods in the deployment - replicas: *5 | int + replicas?: int } diff --git a/pkg/controller/common/rollout/workloads/cloneset_rollout_controller.go b/pkg/controller/common/rollout/workloads/cloneset_rollout_controller.go index 389f1f6a1..9ddd20ca8 100644 --- a/pkg/controller/common/rollout/workloads/cloneset_rollout_controller.go +++ b/pkg/controller/common/rollout/workloads/cloneset_rollout_controller.go @@ -43,12 +43,14 @@ func NewCloneSetRolloutController(client client.Client, recorder event.Recorder, rolloutSpec *v1alpha1.RolloutPlan, rolloutStatus *v1alpha1.RolloutStatus, workloadName types.NamespacedName) *CloneSetRolloutController { return &CloneSetRolloutController{ cloneSetController: cloneSetController{ - client: client, - recorder: recorder, - parentController: parentController, - rolloutSpec: rolloutSpec, - rolloutStatus: rolloutStatus, - workloadNamespacedName: workloadName, + workloadController: workloadController{ + client: client, + recorder: recorder, + parentController: parentController, + rolloutSpec: rolloutSpec, + rolloutStatus: rolloutStatus, + }, + targetNamespacedName: workloadName, }, } } diff --git a/pkg/controller/common/rollout/workloads/cloneset_rollout_integration_test.go b/pkg/controller/common/rollout/workloads/cloneset_rollout_integration_test.go index e7685be64..ca66e0b9d 100644 --- a/pkg/controller/common/rollout/workloads/cloneset_rollout_integration_test.go +++ b/pkg/controller/common/rollout/workloads/cloneset_rollout_integration_test.go @@ -53,21 +53,24 @@ var _ = Describe("cloneset controller", func() { appRollout := v1beta1.AppRollout{ObjectMeta: metav1.ObjectMeta{Name: name}} namespacedName = client.ObjectKey{Name: name, Namespace: namespace} c = CloneSetRolloutController{ - cloneSetController{ - client: k8sClient, - rolloutSpec: &v1alpha1.RolloutPlan{ - RolloutBatches: []v1alpha1.RolloutBatch{ - { - Replicas: intstr.FromInt(1), + cloneSetController: cloneSetController{ + workloadController: workloadController{ + client: k8sClient, + rolloutSpec: &v1alpha1.RolloutPlan{ + RolloutBatches: []v1alpha1.RolloutBatch{ + { + Replicas: intstr.FromInt(1), + }, }, }, + rolloutStatus: &v1alpha1.RolloutStatus{RollingState: v1alpha1.RolloutSucceedState}, + parentController: &appRollout, + recorder: event.NewAPIRecorder(mgr.GetEventRecorderFor("AppRollout")). + WithAnnotations("controller", "AppRollout"), }, - rolloutStatus: &v1alpha1.RolloutStatus{RollingState: v1alpha1.RolloutSucceedState}, - parentController: &appRollout, - recorder: event.NewAPIRecorder(mgr.GetEventRecorderFor("AppRollout")). - WithAnnotations("controller", "AppRollout"), - workloadNamespacedName: namespacedName, - }} + targetNamespacedName: namespacedName, + }, + } cloneSet = kruise.CloneSet{ TypeMeta: metav1.TypeMeta{APIVersion: kruise.GroupVersion.String(), Kind: "CloneSet"}, @@ -112,14 +115,17 @@ var _ = Describe("cloneset controller", func() { workloadNamespacedName := client.ObjectKey{Name: name, Namespace: namespace} got := NewCloneSetRolloutController(k8sClient, recorder, parentController, rolloutSpec, rolloutStatus, workloadNamespacedName) c := &CloneSetRolloutController{ - cloneSetController{ - client: k8sClient, - recorder: recorder, - parentController: parentController, - rolloutSpec: rolloutSpec, - rolloutStatus: rolloutStatus, - workloadNamespacedName: workloadNamespacedName, - }} + cloneSetController: cloneSetController{ + workloadController: workloadController{ + client: k8sClient, + recorder: recorder, + parentController: parentController, + rolloutSpec: rolloutSpec, + rolloutStatus: rolloutStatus, + }, + targetNamespacedName: workloadNamespacedName, + }, + } Expect(got).Should(Equal(c)) }) }) @@ -207,7 +213,7 @@ var _ = Describe("cloneset controller", func() { initialized, err := c.Initialize(ctx) Expect(initialized).Should(BeTrue()) Expect(err).Should(BeNil()) - Expect(k8sClient.Get(ctx, c.workloadNamespacedName, &cloneSet)).Should(Succeed()) + Expect(k8sClient.Get(ctx, c.targetNamespacedName, &cloneSet)).Should(Succeed()) Expect(len(cloneSet.GetOwnerReferences())).Should(BeEquivalentTo(1)) }) @@ -220,7 +226,7 @@ var _ = Describe("cloneset controller", func() { initialized, err := c.Initialize(ctx) Expect(initialized).Should(BeTrue()) Expect(err).Should(BeNil()) - Expect(k8sClient.Get(ctx, c.workloadNamespacedName, &cloneSet)).Should(Succeed()) + Expect(k8sClient.Get(ctx, c.targetNamespacedName, &cloneSet)).Should(Succeed()) Expect(len(cloneSet.GetOwnerReferences())).Should(BeEquivalentTo(1)) }) }) @@ -254,7 +260,7 @@ var _ = Describe("cloneset controller", func() { Expect(done).Should(BeTrue()) Expect(err).Should(BeNil()) Expect(c.rolloutStatus.UpgradedReplicas).Should(BeEquivalentTo(3)) - Expect(k8sClient.Get(ctx, c.workloadNamespacedName, &cloneSet)).Should(Succeed()) + Expect(k8sClient.Get(ctx, c.targetNamespacedName, &cloneSet)).Should(Succeed()) Expect(cloneSet.Spec.UpdateStrategy.Partition.IntValue()).Should(BeEquivalentTo(7)) }) }) @@ -457,7 +463,7 @@ var _ = Describe("cloneset controller", func() { By("finalizing with patch") finalized := c.Finalize(ctx, false) Expect(finalized).Should(BeTrue()) - Expect(k8sClient.Get(ctx, c.workloadNamespacedName, &cloneSet)).Should(Succeed()) + Expect(k8sClient.Get(ctx, c.targetNamespacedName, &cloneSet)).Should(Succeed()) Expect(len(cloneSet.GetOwnerReferences())).Should(BeEquivalentTo(1)) Expect(cloneSet.GetOwnerReferences()[0].Kind).Should(Equal("Deployment")) Expect(cloneSet.Spec.UpdateStrategy.Paused).Should(BeTrue()) diff --git a/pkg/controller/common/rollout/workloads/cloneset_rollout_unit_test.go b/pkg/controller/common/rollout/workloads/cloneset_rollout_unit_test.go index 598f2a895..186c21b1f 100644 --- a/pkg/controller/common/rollout/workloads/cloneset_rollout_unit_test.go +++ b/pkg/controller/common/rollout/workloads/cloneset_rollout_unit_test.go @@ -38,12 +38,14 @@ func TestVerifyRolloutBatchReplicaValue4CloneSet(t *testing.T) { }{ "ClonsetTargetSizeIsNotAvaialbe": { c: &CloneSetRolloutController{ - cloneSetController{ - rolloutSpec: &v1alpha1.RolloutPlan{ - TargetSize: &int2, - RolloutBatches: []v1alpha1.RolloutBatch{{ - Replicas: intstr.FromInt(1), - }, + cloneSetController: cloneSetController{ + workloadController: workloadController{ + rolloutSpec: &v1alpha1.RolloutPlan{ + TargetSize: &int2, + RolloutBatches: []v1alpha1.RolloutBatch{{ + Replicas: intstr.FromInt(1), + }, + }, }, }, }, @@ -53,11 +55,13 @@ func TestVerifyRolloutBatchReplicaValue4CloneSet(t *testing.T) { }, "BatchSizeMismatchesClonesetSize": { c: &CloneSetRolloutController{ - cloneSetController{ - rolloutSpec: &v1alpha1.RolloutPlan{ - RolloutBatches: []v1alpha1.RolloutBatch{{ - Replicas: intstr.FromInt(1), - }, + cloneSetController: cloneSetController{ + workloadController: workloadController{ + rolloutSpec: &v1alpha1.RolloutPlan{ + RolloutBatches: []v1alpha1.RolloutBatch{{ + Replicas: intstr.FromInt(1), + }, + }, }, }, }, @@ -67,14 +71,16 @@ func TestVerifyRolloutBatchReplicaValue4CloneSet(t *testing.T) { }, "BatchSizeMatchesCloneSetSize": { c: &CloneSetRolloutController{ - cloneSetController{ - rolloutSpec: &v1alpha1.RolloutPlan{ - RolloutBatches: []v1alpha1.RolloutBatch{ - { - Replicas: intstr.FromInt(1), - }, - { - Replicas: intstr.FromInt(2), + cloneSetController: cloneSetController{ + workloadController: workloadController{ + rolloutSpec: &v1alpha1.RolloutPlan{ + RolloutBatches: []v1alpha1.RolloutBatch{ + { + Replicas: intstr.FromInt(1), + }, + { + Replicas: intstr.FromInt(2), + }, }, }, }, diff --git a/pkg/controller/common/rollout/workloads/cloneset_scale_controller.go b/pkg/controller/common/rollout/workloads/cloneset_scale_controller.go index a0c90ab18..6041caf32 100644 --- a/pkg/controller/common/rollout/workloads/cloneset_scale_controller.go +++ b/pkg/controller/common/rollout/workloads/cloneset_scale_controller.go @@ -43,12 +43,14 @@ type CloneSetScaleController struct { func NewCloneSetScaleController(client client.Client, recorder event.Recorder, parentController oam.Object, rolloutSpec *v1alpha1.RolloutPlan, rolloutStatus *v1alpha1.RolloutStatus, workloadName types.NamespacedName) *CloneSetScaleController { return &CloneSetScaleController{ cloneSetController: cloneSetController{ - client: client, - recorder: recorder, - parentController: parentController, - rolloutSpec: rolloutSpec, - rolloutStatus: rolloutStatus, - workloadNamespacedName: workloadName, + workloadController: workloadController{ + client: client, + recorder: recorder, + parentController: parentController, + rolloutSpec: rolloutSpec, + rolloutStatus: rolloutStatus, + }, + targetNamespacedName: workloadName, }, } } @@ -66,7 +68,7 @@ func (s *CloneSetScaleController) VerifySpec(ctx context.Context) (bool, error) // the rollout has to have a target size in the scale case if s.rolloutSpec.TargetSize == nil { return false, fmt.Errorf("the rollout plan is attempting to scale the cloneset %s without a target", - s.workloadNamespacedName.Name) + s.targetNamespacedName.Name) } // record the target size s.rolloutStatus.RolloutTargetSize = *s.rolloutSpec.TargetSize diff --git a/pkg/controller/common/rollout/workloads/cloneset_scale_integration_test.go b/pkg/controller/common/rollout/workloads/cloneset_scale_integration_test.go index 8e137b037..f5598d8bc 100644 --- a/pkg/controller/common/rollout/workloads/cloneset_scale_integration_test.go +++ b/pkg/controller/common/rollout/workloads/cloneset_scale_integration_test.go @@ -51,28 +51,31 @@ var _ = Describe("cloneset controller", func() { appRollout := v1beta1.AppRollout{ObjectMeta: metav1.ObjectMeta{Name: name}} namespacedName = client.ObjectKey{Name: name, Namespace: namespace} s = CloneSetScaleController{ - cloneSetController{ - client: k8sClient, - rolloutSpec: &v1alpha1.RolloutPlan{ - TargetSize: pointer.Int32Ptr(10), - RolloutBatches: []v1alpha1.RolloutBatch{ - { - Replicas: intstr.FromInt(1), - }, - { - Replicas: intstr.FromString("20%"), - }, - { - Replicas: intstr.FromString("80%"), + cloneSetController: cloneSetController{ + workloadController: workloadController{ + client: k8sClient, + rolloutSpec: &v1alpha1.RolloutPlan{ + TargetSize: pointer.Int32Ptr(10), + RolloutBatches: []v1alpha1.RolloutBatch{ + { + Replicas: intstr.FromInt(1), + }, + { + Replicas: intstr.FromString("20%"), + }, + { + Replicas: intstr.FromString("80%"), + }, }, }, + rolloutStatus: &v1alpha1.RolloutStatus{RollingState: v1alpha1.RolloutSucceedState}, + parentController: &appRollout, + recorder: event.NewAPIRecorder(mgr.GetEventRecorderFor("AppRollout")). + WithAnnotations("controller", "AppRollout"), }, - rolloutStatus: &v1alpha1.RolloutStatus{RollingState: v1alpha1.RolloutSucceedState}, - parentController: &appRollout, - recorder: event.NewAPIRecorder(mgr.GetEventRecorderFor("AppRollout")). - WithAnnotations("controller", "AppRollout"), - workloadNamespacedName: namespacedName, - }} + targetNamespacedName: namespacedName, + }, + } cloneSet = kruise.CloneSet{ TypeMeta: metav1.TypeMeta{APIVersion: kruise.GroupVersion.String(), Kind: "CloneSet"}, @@ -117,13 +120,15 @@ var _ = Describe("cloneset controller", func() { workloadNamespacedName := client.ObjectKey{Name: name, Namespace: namespace} got := NewCloneSetScaleController(k8sClient, recorder, parentController, rolloutSpec, rolloutStatus, workloadNamespacedName) controller := &CloneSetScaleController{ - cloneSetController{ - client: k8sClient, - recorder: recorder, - parentController: parentController, - rolloutSpec: rolloutSpec, - rolloutStatus: rolloutStatus, - workloadNamespacedName: workloadNamespacedName, + cloneSetController: cloneSetController{ + workloadController: workloadController{ + client: k8sClient, + recorder: recorder, + parentController: parentController, + rolloutSpec: rolloutSpec, + rolloutStatus: rolloutStatus, + }, + targetNamespacedName: workloadNamespacedName, }} Expect(got).Should(Equal(controller)) }) @@ -265,7 +270,7 @@ var _ = Describe("cloneset controller", func() { Expect(done).Should(BeTrue()) Expect(err).Should(BeNil()) Expect(s.rolloutStatus.UpgradedReplicas).Should(BeEquivalentTo(3)) - Expect(k8sClient.Get(ctx, s.workloadNamespacedName, &cloneSet)).Should(Succeed()) + Expect(k8sClient.Get(ctx, s.targetNamespacedName, &cloneSet)).Should(Succeed()) Expect(*cloneSet.Spec.Replicas).Should(BeEquivalentTo(3)) }) }) @@ -471,7 +476,7 @@ var _ = Describe("cloneset controller", func() { By("finalizing with patch") finalized := s.Finalize(ctx, false) Expect(finalized).Should(BeTrue()) - Expect(k8sClient.Get(ctx, s.workloadNamespacedName, &cloneSet)).Should(Succeed()) + Expect(k8sClient.Get(ctx, s.targetNamespacedName, &cloneSet)).Should(Succeed()) Expect(len(cloneSet.GetOwnerReferences())).Should(BeEquivalentTo(1)) Expect(cloneSet.GetOwnerReferences()[0].Kind).Should(Equal("Deployment")) }) diff --git a/pkg/controller/common/rollout/workloads/common_test.go b/pkg/controller/common/rollout/workloads/common_test.go index dfba2845e..b367edd05 100644 --- a/pkg/controller/common/rollout/workloads/common_test.go +++ b/pkg/controller/common/rollout/workloads/common_test.go @@ -362,11 +362,16 @@ func Test_VerifyBatchesWithScaleFailCases(t *testing.T) { originalSize: 16, targetSize: 32, }, - "numeric decrease": { + "numeric decrease 1": { rolloutSpec: rolloutNumericSpec, originalSize: 13, targetSize: 10, }, + "numeric decrease 2": { + rolloutSpec: rolloutNumericSpec, + originalSize: 16, + targetSize: 10, + }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { diff --git a/pkg/controller/common/rollout/workloads/controller.go b/pkg/controller/common/rollout/workloads/controller.go index ffef67881..fb6fa5dca 100644 --- a/pkg/controller/common/rollout/workloads/controller.go +++ b/pkg/controller/common/rollout/workloads/controller.go @@ -60,16 +60,20 @@ type WorkloadController interface { Finalize(ctx context.Context, succeed bool) bool } -// cloneSetController is the place to hold fields needed for handle Cloneset type of workloads -type cloneSetController struct { +type workloadController struct { client client.Client recorder event.Recorder parentController oam.Object - rolloutSpec *v1alpha1.RolloutPlan - rolloutStatus *v1alpha1.RolloutStatus - workloadNamespacedName types.NamespacedName - cloneSet *kruise.CloneSet + rolloutSpec *v1alpha1.RolloutPlan + rolloutStatus *v1alpha1.RolloutStatus +} + +// cloneSetController is the place to hold fields needed for handle Cloneset type of workloads +type cloneSetController struct { + workloadController + targetNamespacedName types.NamespacedName + cloneSet *kruise.CloneSet } // size fetches the Cloneset and returns the replicas (not the actual number of pods) @@ -90,7 +94,7 @@ func (c *cloneSetController) size(ctx context.Context) (int32, error) { func (c *cloneSetController) fetchCloneSet(ctx context.Context) error { // get the cloneSet workload := kruise.CloneSet{} - err := c.client.Get(ctx, c.workloadNamespacedName, &workload) + err := c.client.Get(ctx, c.targetNamespacedName, &workload) if err != nil { if !apierrors.IsNotFound(err) { c.recorder.Event(c.parentController, event.Warning("Failed to get the Cloneset", err)) diff --git a/pkg/controller/common/rollout/workloads/deployment_rollout_controller.go b/pkg/controller/common/rollout/workloads/deployment_rollout_controller.go index 74cdb35c9..fda163e8e 100644 --- a/pkg/controller/common/rollout/workloads/deployment_rollout_controller.go +++ b/pkg/controller/common/rollout/workloads/deployment_rollout_controller.go @@ -31,19 +31,15 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" + "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" "github.com/oam-dev/kubevela/pkg/controller/utils" "github.com/oam-dev/kubevela/pkg/oam" ) -// DeploymentController is responsible for handling rollout deployment type of workloads -type DeploymentController struct { - client client.Client - recorder event.Recorder - parentController oam.Object - - rolloutSpec *v1alpha1.RolloutPlan - rolloutStatus *v1alpha1.RolloutStatus +// DeploymentRolloutController is responsible for handling rollout deployment type of workloads +type DeploymentRolloutController struct { + workloadController targetNamespacedName types.NamespacedName sourceNamespacedName types.NamespacedName sourceDeploy apps.Deployment @@ -53,20 +49,22 @@ type DeploymentController struct { // NewDeploymentController creates a new deployment rollout controller func NewDeploymentController(client client.Client, recorder event.Recorder, parentController oam.Object, rolloutSpec *v1alpha1.RolloutPlan, rolloutStatus *v1alpha1.RolloutStatus, sourceNamespacedName, - targetNamespacedName types.NamespacedName) *DeploymentController { - return &DeploymentController{ - client: client, - recorder: recorder, - parentController: parentController, - rolloutSpec: rolloutSpec, - rolloutStatus: rolloutStatus, - sourceNamespacedName: sourceNamespacedName, + targetNamespacedName types.NamespacedName) *DeploymentRolloutController { + return &DeploymentRolloutController{ + workloadController: workloadController{ + client: client, + recorder: recorder, + parentController: parentController, + rolloutSpec: rolloutSpec, + rolloutStatus: rolloutStatus, + }, targetNamespacedName: targetNamespacedName, + sourceNamespacedName: sourceNamespacedName, } } // VerifySpec verifies that the rollout resource is consistent with the rollout spec -func (c *DeploymentController) VerifySpec(ctx context.Context) (bool, error) { +func (c *DeploymentRolloutController) VerifySpec(ctx context.Context) (bool, error) { var verifyErr error defer func() { @@ -142,7 +140,7 @@ func (c *DeploymentController) VerifySpec(ctx context.Context) (bool, error) { } // Initialize makes sure that the source and target deployment is under our control -func (c *DeploymentController) Initialize(ctx context.Context) (bool, error) { +func (c *DeploymentRolloutController) Initialize(ctx context.Context) (bool, error) { err := c.fetchDeployments(ctx) if err != nil { c.rolloutStatus.RolloutRetry(err.Error()) @@ -167,7 +165,7 @@ func (c *DeploymentController) Initialize(ctx context.Context) (bool, error) { // RolloutOneBatchPods calculates the number of pods we can upgrade once according to the rollout spec // and then set the partition accordingly -func (c *DeploymentController) RolloutOneBatchPods(ctx context.Context) (bool, error) { +func (c *DeploymentRolloutController) RolloutOneBatchPods(ctx context.Context) (bool, error) { err := c.fetchDeployments(ctx) if err != nil { // don't fail the rollout just because of we can't get the resource @@ -202,7 +200,7 @@ func (c *DeploymentController) RolloutOneBatchPods(ctx context.Context) (bool, e } // CheckOneBatchPods checks to see if the pods are all available according to the rollout plan -func (c *DeploymentController) CheckOneBatchPods(ctx context.Context) (bool, error) { +func (c *DeploymentRolloutController) CheckOneBatchPods(ctx context.Context) (bool, error) { err := c.fetchDeployments(ctx) if err != nil { // don't fail the rollout just because of we can't get the resource @@ -248,7 +246,7 @@ func (c *DeploymentController) CheckOneBatchPods(ctx context.Context) (bool, err } // FinalizeOneBatch makes sure that the rollout status are updated correctly -func (c *DeploymentController) FinalizeOneBatch(ctx context.Context) (bool, error) { +func (c *DeploymentRolloutController) FinalizeOneBatch(ctx context.Context) (bool, error) { err := c.fetchDeployments(ctx) if err != nil { // don't fail the rollout just because of we can't get the resource @@ -258,7 +256,7 @@ func (c *DeploymentController) FinalizeOneBatch(ctx context.Context) (bool, erro sourceTarget := getDeployReplicaSize(&c.sourceDeploy) targetTarget := getDeployReplicaSize(&c.targetDeploy) if sourceTarget+targetTarget != c.rolloutStatus.RolloutTargetSize { - err = fmt.Errorf("deployment targest don't match total rollout, sourceTarget = %d, targetTarget = %d, "+ + err = fmt.Errorf("deployment targets don't match total rollout, sourceTarget = %d, targetTarget = %d, "+ "rolloutTargetSize = %d", sourceTarget, targetTarget, c.rolloutStatus.RolloutTargetSize) klog.ErrorS(err, "the batch is not valid", "current batch", c.rolloutStatus.CurrentBatch) return false, err @@ -267,7 +265,7 @@ func (c *DeploymentController) FinalizeOneBatch(ctx context.Context) (bool, erro } // Finalize makes sure the Deployment is all upgraded -func (c *DeploymentController) Finalize(ctx context.Context, succeed bool) bool { +func (c *DeploymentRolloutController) Finalize(ctx context.Context, succeed bool) bool { err := c.fetchDeployments(ctx) if err != nil { // don't fail the rollout just because of we can't get the resource @@ -293,32 +291,7 @@ func (c *DeploymentController) Finalize(ctx context.Context, succeed bool) bool /* ---------------------------------- The functions below are helper functions ------------------------------------- */ -// calculateRolloutTotalSize fetches the Deployment and returns the replicas (not the actual number of pods) -func (c *DeploymentController) calculateRolloutTotalSize() (int32, error) { - sourceSize := getDeployReplicaSize(&c.sourceDeploy) - // the spec target size is the truth if it's set - if c.rolloutSpec.TargetSize != nil { - targetSize := *c.rolloutSpec.TargetSize - if targetSize < sourceSize { - return -1, fmt.Errorf("target size `%d` less than source size `%d`", targetSize, sourceSize) - } - return targetSize, nil - } - // otherwise, we assume that the source is the total - return sourceSize, nil -} - -// check if the replicas in all the rollout batches add up to the right number -func (c *DeploymentController) verifyRolloutBatchReplicaValue(totalReplicas int32) error { - // use a common function to check if the sum of all the batches can match the Deployment size - err := verifyBatchesWithRollout(c.rolloutSpec, totalReplicas) - if err != nil { - return err - } - return nil -} - -func (c *DeploymentController) fetchDeployments(ctx context.Context) error { +func (c *DeploymentRolloutController) fetchDeployments(ctx context.Context) error { err := c.client.Get(ctx, c.sourceNamespacedName, &c.sourceDeploy) if err != nil { if !apierrors.IsNotFound(err) { @@ -339,7 +312,7 @@ func (c *DeploymentController) fetchDeployments(ctx context.Context) error { // add the parent controller to the owner of the deployment, unpause it and initialize the size // before kicking start the update and start from every pod in the old version -func (c *DeploymentController) claimDeployment(ctx context.Context, deploy *apps.Deployment, initSize *int32) error { +func (c *DeploymentRolloutController) claimDeployment(ctx context.Context, deploy *apps.Deployment, initSize *int32) error { deployPatch := client.MergeFrom(deploy.DeepCopyObject()) if controller := metav1.GetControllerOf(deploy); controller == nil { ref := metav1.NewControllerRef(c.parentController, v1beta1.AppRolloutKindVersionKind) @@ -357,7 +330,86 @@ func (c *DeploymentController) claimDeployment(ctx context.Context, deploy *apps return nil } -func (c *DeploymentController) rolloutBatchFirstHalf(ctx context.Context, +// patch the deployment's target, returns if succeeded +func (c *DeploymentRolloutController) patchDeployment(ctx context.Context, target int32, deploy *apps.Deployment) error { + deployPatch := client.MergeFrom(deploy.DeepCopyObject()) + deploy.Spec.Replicas = pointer.Int32Ptr(target) + // patch the Deployment + if err := c.client.Patch(ctx, deploy, deployPatch, client.FieldOwner(c.parentController.GetUID())); err != nil { + c.recorder.Event(c.parentController, event.Warning(event.Reason(fmt.Sprintf( + "Failed to update the deployment %s to the correct target %d", deploy.GetName(), target)), err)) + return err + } + klog.InfoS("Submitted upgrade quest for deployment", "deployment", + deploy.GetName(), "target replica size", target, "batch", c.rolloutStatus.CurrentBatch) + return nil +} + +func (c *DeploymentRolloutController) releaseDeployment(ctx context.Context, deploy *apps.Deployment) error { + deployPatch := client.MergeFrom(deploy.DeepCopyObject()) + // remove the parent controller from the resources' owner list + var newOwnerList []metav1.OwnerReference + found := false + for _, owner := range deploy.GetOwnerReferences() { + if owner.Kind == v1beta1.AppRolloutKind && owner.APIVersion == v1beta1.SchemeGroupVersion.String() { + found = true + continue + } + newOwnerList = append(newOwnerList, owner) + } + if !found { + klog.InfoS("the deployment is already released", "deploy", deploy.Name) + return nil + } + deploy.SetOwnerReferences(newOwnerList) + // patch the Deployment + if err := c.client.Patch(ctx, deploy, deployPatch, client.FieldOwner(c.parentController.GetUID())); err != nil { + c.recorder.Event(c.parentController, event.Warning("Failed to the finalize the Deployment", err)) + c.rolloutStatus.RolloutRetry(err.Error()) + return err + } + return nil +} + +// calculateRolloutTotalSize fetches the Deployment and returns the replicas (not the actual number of pods) +func (c *DeploymentRolloutController) calculateRolloutTotalSize() (int32, error) { + sourceSize := getDeployReplicaSize(&c.sourceDeploy) + // the spec target size is the truth if it's set + if c.rolloutSpec.TargetSize != nil { + targetSize := *c.rolloutSpec.TargetSize + if targetSize < sourceSize { + return -1, fmt.Errorf("target size `%d` less than source size `%d`", targetSize, sourceSize) + } + return targetSize, nil + } + // otherwise, we assume that the source is the total + return sourceSize, nil +} + +// check if the replicas in all the rollout batches add up to the right number +func (c *DeploymentRolloutController) verifyRolloutBatchReplicaValue(totalReplicas int32) error { + // use a common function to check if the sum of all the batches can match the Deployment size + err := verifyBatchesWithRollout(c.rolloutSpec, totalReplicas) + if err != nil { + return err + } + return nil +} + +// the target deploy size for the current batch +func (c *DeploymentRolloutController) calculateCurrentTarget(totalSize int32) int32 { + return int32(calculateNewBatchTarget(c.rolloutSpec, 0, int(totalSize), int(c.rolloutStatus.CurrentBatch))) +} + +// the source deploy size for the current batch +func (c *DeploymentRolloutController) calculateCurrentSource(totalSize int32) int32 { + sourceSize := totalSize - c.calculateCurrentTarget(totalSize) + klog.InfoS("Calculated the number of pods in the source deployment after current batch", + "current batch", c.rolloutStatus.CurrentBatch, "source deploy size", sourceSize) + return sourceSize +} + +func (c *DeploymentRolloutController) rolloutBatchFirstHalf(ctx context.Context, rolloutStrategy v1alpha1.RolloutStrategyType) (finished bool, rolloutError error) { targetSize := c.calculateCurrentTarget(c.rolloutStatus.RolloutTargetSize) defer func() { @@ -408,7 +460,7 @@ func (c *DeploymentController) rolloutBatchFirstHalf(ctx context.Context, return false, fmt.Errorf("encountered an unknown rolloutStrategy `%s`", rolloutStrategy) } -func (c *DeploymentController) rolloutBatchSecondHalf(ctx context.Context, +func (c *DeploymentRolloutController) rolloutBatchSecondHalf(ctx context.Context, rolloutStrategy v1alpha1.RolloutStrategyType, targetSize int32) bool { var err error sourceSize := c.calculateCurrentSource(c.rolloutStatus.RolloutTargetSize) @@ -462,57 +514,3 @@ func (c *DeploymentController) rolloutBatchSecondHalf(ctx context.Context, } return true } - -// the target deploy size for the current batch -func (c *DeploymentController) calculateCurrentTarget(totalSize int32) int32 { - return int32(calculateNewBatchTarget(c.rolloutSpec, 0, int(totalSize), int(c.rolloutStatus.CurrentBatch))) -} - -// the source deploy size for the current batch -func (c *DeploymentController) calculateCurrentSource(totalSize int32) int32 { - sourceSize := totalSize - c.calculateCurrentTarget(totalSize) - klog.InfoS("Calculated the number of pods in the source deployment after current batch", - "current batch", c.rolloutStatus.CurrentBatch, "source deploy size", sourceSize) - return sourceSize -} - -// patch the deployment's target, returns if succeeded -func (c *DeploymentController) patchDeployment(ctx context.Context, target int32, deploy *apps.Deployment) error { - deployPatch := client.MergeFrom(deploy.DeepCopyObject()) - deploy.Spec.Replicas = pointer.Int32Ptr(target) - // patch the Deployment - if err := c.client.Patch(ctx, deploy, deployPatch, client.FieldOwner(c.parentController.GetUID())); err != nil { - c.recorder.Event(c.parentController, event.Warning(event.Reason(fmt.Sprintf( - "Failed to update the deployment %s to the correct target %d", deploy.GetName(), target)), err)) - return err - } - klog.InfoS("Submitted upgrade quest for deployment", "deployment", - deploy.GetName(), "target replica size", target, "batch", c.rolloutStatus.CurrentBatch) - return nil -} - -func (c *DeploymentController) releaseDeployment(ctx context.Context, deploy *apps.Deployment) error { - deployPatch := client.MergeFrom(deploy.DeepCopyObject()) - // remove the parent controller from the resources' owner list - var newOwnerList []metav1.OwnerReference - found := false - for _, owner := range deploy.GetOwnerReferences() { - if owner.Kind == v1beta1.AppRolloutKind && owner.APIVersion == v1beta1.SchemeGroupVersion.String() { - found = true - continue - } - newOwnerList = append(newOwnerList, owner) - } - if !found { - klog.InfoS("the deployment is already released", "deploy", deploy.Name) - return nil - } - deploy.SetOwnerReferences(newOwnerList) - // patch the Deployment - if err := c.client.Patch(ctx, deploy, deployPatch, client.FieldOwner(c.parentController.GetUID())); err != nil { - c.recorder.Event(c.parentController, event.Warning("Failed to the finalize the Deployment", err)) - c.rolloutStatus.RolloutRetry(err.Error()) - return err - } - return nil -} diff --git a/pkg/controller/common/rollout/workloads/deployment_rollout_integration_test.go b/pkg/controller/common/rollout/workloads/deployment_rollout_integration_test.go index b265928f4..c9ee139e8 100644 --- a/pkg/controller/common/rollout/workloads/deployment_rollout_integration_test.go +++ b/pkg/controller/common/rollout/workloads/deployment_rollout_integration_test.go @@ -38,7 +38,7 @@ import ( var _ = Describe("deployment controller", func() { var ( - c DeploymentController + c DeploymentRolloutController ns corev1.Namespace namespaceName string sourceName string @@ -57,27 +57,29 @@ var _ = Describe("deployment controller", func() { appRollout := v1beta1.AppRollout{ObjectMeta: metav1.ObjectMeta{Name: "test-rollout"}} sourceNamespacedName = client.ObjectKey{Name: sourceName, Namespace: namespaceName} targetNamespacedName = client.ObjectKey{Name: targetName, Namespace: namespaceName} - c = DeploymentController{ - client: k8sClient, - rolloutSpec: &v1alpha1.RolloutPlan{ - RolloutBatches: []v1alpha1.RolloutBatch{ - { - Replicas: intstr.FromInt(2), - }, - { - Replicas: intstr.FromInt(3), - }, - { - Replicas: intstr.FromString("50%"), + c = DeploymentRolloutController{ + workloadController: workloadController{ + client: k8sClient, + rolloutSpec: &v1alpha1.RolloutPlan{ + RolloutBatches: []v1alpha1.RolloutBatch{ + { + Replicas: intstr.FromInt(2), + }, + { + Replicas: intstr.FromInt(3), + }, + { + Replicas: intstr.FromString("50%"), + }, }, }, + rolloutStatus: &v1alpha1.RolloutStatus{RollingState: v1alpha1.RolloutSucceedState}, + parentController: &appRollout, + recorder: event.NewAPIRecorder(mgr.GetEventRecorderFor("AppRollout")). + WithAnnotations("controller", "AppRollout"), }, - rolloutStatus: &v1alpha1.RolloutStatus{RollingState: v1alpha1.RolloutSucceedState}, - parentController: &appRollout, - sourceNamespacedName: sourceNamespacedName, targetNamespacedName: targetNamespacedName, - recorder: event.NewAPIRecorder(mgr.GetEventRecorderFor("AppRollout")). - WithAnnotations("controller", "AppRollout"), + sourceNamespacedName: sourceNamespacedName, } targetDeploy = appsv1.Deployment{ @@ -141,14 +143,16 @@ var _ = Describe("deployment controller", func() { workloadNamespacedName := client.ObjectKey{Name: sourceName, Namespace: namespaceName} got := NewDeploymentController(k8sClient, recorder, parentController, rolloutSpec, rolloutStatus, workloadNamespacedName, workloadNamespacedName) - c := &DeploymentController{ - client: k8sClient, - recorder: recorder, - parentController: parentController, - rolloutSpec: rolloutSpec, - rolloutStatus: rolloutStatus, - sourceNamespacedName: workloadNamespacedName, + c := &DeploymentRolloutController{ + workloadController: workloadController{ + client: k8sClient, + recorder: recorder, + parentController: parentController, + rolloutSpec: rolloutSpec, + rolloutStatus: rolloutStatus, + }, targetNamespacedName: workloadNamespacedName, + sourceNamespacedName: workloadNamespacedName, } Expect(got).Should(Equal(c)) }) @@ -736,4 +740,73 @@ var _ = Describe("deployment controller", func() { Expect(err).Should(BeNil()) }) }) + + Context("TestFinalizeOneBatch", func() { + It("failed to fetch Deployment", func() { + finalized, err := c.FinalizeOneBatch(ctx) + Expect(finalized).Should(BeFalse()) + Expect(err).Should(BeNil()) + }) + + It("test rollout batch configured correctly", func() { + By("Create the deployments") + sourceDeploy.Spec.Replicas = pointer.Int32Ptr(8) + Expect(k8sClient.Create(ctx, &sourceDeploy)).Should(SatisfyAny(Succeed(), &util.AlreadyExistMatcher{})) + targetDeploy.Spec.Replicas = pointer.Int32Ptr(5) + Expect(k8sClient.Create(ctx, &targetDeploy)).Should(SatisfyAny(Succeed(), &util.AlreadyExistMatcher{})) + By("Fail if the targets don't add up") + c.rolloutSpec = rolloutRelaxSpec + c.rolloutSpec.RolloutStrategy = v1alpha1.DecreaseFirstRolloutStrategyType + c.rolloutStatus.CurrentBatch = 1 + c.rolloutStatus.RolloutTargetSize = 10 + finalized, err := c.FinalizeOneBatch(ctx) + Expect(finalized).Should(BeFalse()) + Expect(err.Error()).Should(ContainSubstring("deployment targets don't match total rollout")) + By("Success if they do") + // sum of target and source + c.rolloutStatus.RolloutTargetSize = 13 + finalized, err = c.FinalizeOneBatch(ctx) + Expect(finalized).Should(BeTrue()) + Expect(err).Should(BeNil()) + }) + }) + + Context("TestFinalize", func() { + It("failed to fetch deployment", func() { + finalized := c.Finalize(ctx, true) + Expect(finalized).Should(BeFalse()) + }) + + It("release success without ownership", func() { + By("Create the deployments") + Expect(k8sClient.Create(ctx, &sourceDeploy)).Should(SatisfyAny(Succeed(), &util.AlreadyExistMatcher{})) + Expect(k8sClient.Create(ctx, &targetDeploy)).Should(SatisfyAny(Succeed(), &util.AlreadyExistMatcher{})) + By("no op success if we are not the owner") + finalized := c.Finalize(ctx, true) + Expect(finalized).Should(BeTrue()) + }) + + It("release success as the owner", func() { + By("Create the deployments") + sourceDeploy.SetOwnerReferences([]metav1.OwnerReference{{ + APIVersion: v1beta1.SchemeGroupVersion.String(), + Kind: v1beta1.AppRolloutKind, + Name: "def", + UID: "123456", + Controller: pointer.BoolPtr(true), + }}) + Expect(k8sClient.Create(ctx, &sourceDeploy)).Should(SatisfyAny(Succeed(), &util.AlreadyExistMatcher{})) + targetDeploy.SetOwnerReferences([]metav1.OwnerReference{{ + APIVersion: v1beta1.SchemeGroupVersion.String(), + Kind: v1beta1.ApplicationKind, + Name: "def", + UID: "123456", + Controller: pointer.BoolPtr(true), + }}) + Expect(k8sClient.Create(ctx, &targetDeploy)).Should(SatisfyAny(Succeed(), &util.AlreadyExistMatcher{})) + By("success if we are the owner") + finalized := c.Finalize(ctx, true) + Expect(finalized).Should(BeTrue()) + }) + }) }) diff --git a/pkg/controller/common/rollout/workloads/deployment_rollout_unit_test.go b/pkg/controller/common/rollout/workloads/deployment_rollout_unit_test.go index 640e69f67..ed6efceeb 100644 --- a/pkg/controller/common/rollout/workloads/deployment_rollout_unit_test.go +++ b/pkg/controller/common/rollout/workloads/deployment_rollout_unit_test.go @@ -81,10 +81,12 @@ func TestCalculateCurrentSource(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { - controller := DeploymentController{ - rolloutSpec: tc.rolloutSpec, - rolloutStatus: &v1alpha1.RolloutStatus{ - CurrentBatch: tc.currentBatch, + controller := DeploymentRolloutController{ + workloadController: workloadController{ + rolloutSpec: tc.rolloutSpec, + rolloutStatus: &v1alpha1.RolloutStatus{ + CurrentBatch: tc.currentBatch, + }, }, } ct := controller.calculateCurrentSource(tc.totalSize)