From abf9270473d864322319f7bad63f2c0e570a1115 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Mon, 1 Mar 2021 01:00:27 -0800 Subject: [PATCH] manual e2e test example with bug fixes --- apis/core.oam.dev/v1alpha2/core_types.go | 2 +- .../v1alpha1/rollout_plan_types.go | 2 +- .../v1alpha1/rollout_state.go | 12 +- .../core.oam.dev_applicationdeployments.yaml | 8 +- .../crds/core.oam.dev_applications.yaml | 9 +- .../crds/core.oam.dev_components.yaml | 1 - .../crds/standard.oam.dev_rollouttraits.yaml | 8 +- cmd/core/main.go | 3 +- docs/examples/advanced-cue/app4.yaml | 2 +- docs/examples/rollout/README.md | 14 ++ docs/examples/rollout/app-deploy.yaml | 18 ++ docs/examples/rollout/app-source-prep.yaml | 17 ++ docs/examples/rollout/app-source.yaml | 15 ++ docs/examples/rollout/app-target.yaml | 17 ++ docs/examples/rollout/clonesetDefinition.yaml | 103 ++++++++++ .../core.oam.dev_applicationdeployments.yaml | 8 +- .../crds/core.oam.dev_applications.yaml | 9 +- .../crds/core.oam.dev_components.yaml | 1 - .../crds/standard.oam.dev_rollouttraits.yaml | 8 +- .../common/rollout/rollout_plan_controller.go | 45 +++-- .../rollout/workloads/cloneset_controller.go | 34 ++-- .../application/application_controller.go | 3 - .../application_controller_test.go | 15 +- .../v1alpha2/application/apply.go | 51 ++--- .../v1alpha2/application/apply_test.go | 31 +-- .../v1alpha2/application/suite_test.go | 6 +- .../applicationconfiguration.go | 31 ++- .../applicationconfiguration_test.go | 21 +- .../applicationconfiguration/apply.go | 7 +- .../applicationconfiguration/render.go | 45 +++-- .../applicationconfiguration/render_test.go | 186 +++++++++++++++--- .../applicationconfiguration/workloads.go | 2 +- .../applicationdeployment_controller.go | 32 ++- pkg/oam/labels.go | 5 - pkg/oam/util/helper.go | 11 +- pkg/utils/apply/patch.go | 5 +- pkg/webhook/common/rollout/rollout_plan.go | 6 + 37 files changed, 569 insertions(+), 224 deletions(-) create mode 100644 docs/examples/rollout/README.md create mode 100644 docs/examples/rollout/app-deploy.yaml create mode 100644 docs/examples/rollout/app-source-prep.yaml create mode 100644 docs/examples/rollout/app-source.yaml create mode 100644 docs/examples/rollout/app-target.yaml create mode 100644 docs/examples/rollout/clonesetDefinition.yaml diff --git a/apis/core.oam.dev/v1alpha2/core_types.go b/apis/core.oam.dev/v1alpha2/core_types.go index 1314f7998..23e6d7ce9 100644 --- a/apis/core.oam.dev/v1alpha2/core_types.go +++ b/apis/core.oam.dev/v1alpha2/core_types.go @@ -296,7 +296,7 @@ type ComponentStatus struct { type Revision struct { Name string `json:"name"` Revision int64 `json:"revision"` - RevisionHash string `json:"revisionHash"` + RevisionHash string `json:"revisionHash,omitempty"` } // +kubebuilder:object:root=true diff --git a/apis/standard.oam.dev/v1alpha1/rollout_plan_types.go b/apis/standard.oam.dev/v1alpha1/rollout_plan_types.go index d77e84101..919b8205f 100644 --- a/apis/standard.oam.dev/v1alpha1/rollout_plan_types.go +++ b/apis/standard.oam.dev/v1alpha1/rollout_plan_types.go @@ -98,7 +98,7 @@ type RolloutPlan struct { // This is designed for the operators to manually rollout // Default is the the number of batches which will rollout all the batches // +optional - BatchPartition *int32 `json:"lastBatchToRollout,omitempty"` + BatchPartition *int32 `json:"batchPartition,omitempty"` // Paused the rollout, default is false // +optional diff --git a/apis/standard.oam.dev/v1alpha1/rollout_state.go b/apis/standard.oam.dev/v1alpha1/rollout_state.go index 37156cf56..4c4361c21 100644 --- a/apis/standard.oam.dev/v1alpha1/rollout_state.go +++ b/apis/standard.oam.dev/v1alpha1/rollout_state.go @@ -157,11 +157,13 @@ func (r *RolloutStatus) RolloutFailed(reason string) { func (r *RolloutStatus) StateTransition(event RolloutEvent) { rollingState := r.RollingState batchRollingState := r.BatchRollingState - defer klog.InfoS("try to execute a rollout state transition", - "pre rolling state", rollingState, - "pre batch rolling state", batchRollingState, - "post rolling state", r.RollingState, - "post batch rolling state", r.BatchRollingState) + defer func() { + klog.InfoS("try to execute a rollout state transition", + "pre rolling state", rollingState, + "pre batch rolling state", batchRollingState, + "post rolling state", r.RollingState, + "post batch rolling state", r.BatchRollingState) + }() // we have special transition for these two types of event if event == RollingFailedEvent || event == RollingRetriableFailureEvent { diff --git a/charts/vela-core/crds/core.oam.dev_applicationdeployments.yaml b/charts/vela-core/crds/core.oam.dev_applicationdeployments.yaml index d2fffb45f..98b429aca 100644 --- a/charts/vela-core/crds/core.oam.dev_applicationdeployments.yaml +++ b/charts/vela-core/crds/core.oam.dev_applicationdeployments.yaml @@ -45,6 +45,10 @@ spec: rolloutPlan: description: RolloutPlan is the details on how to rollout the resources properties: + batchPartition: + description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches + format: int32 + type: integer canaryMetric: description: CanaryMetric provides a way for the rollout process to automatically check certain metrics before complete the process items: @@ -96,10 +100,6 @@ spec: - name type: object type: array - lastBatchToRollout: - description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches - format: int32 - type: integer numBatches: description: The number of batches, default = 1 format: int32 diff --git a/charts/vela-core/crds/core.oam.dev_applications.yaml b/charts/vela-core/crds/core.oam.dev_applications.yaml index 6e9e717e1..171a613d9 100644 --- a/charts/vela-core/crds/core.oam.dev_applications.yaml +++ b/charts/vela-core/crds/core.oam.dev_applications.yaml @@ -73,6 +73,10 @@ spec: rolloutPlan: description: RolloutPlan is the details on how to rollout the resources The controller simply replace the old resources with the new one if there is no rollout plan involved properties: + batchPartition: + description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches + format: int32 + type: integer canaryMetric: description: CanaryMetric provides a way for the rollout process to automatically check certain metrics before complete the process items: @@ -124,10 +128,6 @@ spec: - name type: object type: array - lastBatchToRollout: - description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches - format: int32 - type: integer numBatches: description: The number of batches, default = 1 format: int32 @@ -362,7 +362,6 @@ spec: required: - name - revision - - revisionHash type: object rollingState: description: RollingState is the Rollout State diff --git a/charts/vela-core/crds/core.oam.dev_components.yaml b/charts/vela-core/crds/core.oam.dev_components.yaml index 3727d146d..b20e7145e 100644 --- a/charts/vela-core/crds/core.oam.dev_components.yaml +++ b/charts/vela-core/crds/core.oam.dev_components.yaml @@ -119,7 +119,6 @@ spec: required: - name - revision - - revisionHash type: object observedGeneration: description: The generation observed by the component controller. diff --git a/charts/vela-core/crds/standard.oam.dev_rollouttraits.yaml b/charts/vela-core/crds/standard.oam.dev_rollouttraits.yaml index bf937fab1..85a0b24dc 100644 --- a/charts/vela-core/crds/standard.oam.dev_rollouttraits.yaml +++ b/charts/vela-core/crds/standard.oam.dev_rollouttraits.yaml @@ -35,6 +35,10 @@ spec: rolloutPlan: description: RolloutPlan is the details on how to rollout the resources properties: + batchPartition: + description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches + format: int32 + type: integer canaryMetric: description: CanaryMetric provides a way for the rollout process to automatically check certain metrics before complete the process items: @@ -86,10 +90,6 @@ spec: - name type: object type: array - lastBatchToRollout: - description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches - format: int32 - type: integer numBatches: description: The number of batches, default = 1 format: int32 diff --git a/cmd/core/main.go b/cmd/core/main.go index a9f53ff7e..7734bf159 100644 --- a/cmd/core/main.go +++ b/cmd/core/main.go @@ -27,6 +27,7 @@ import ( crdv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -117,7 +118,7 @@ func main() { } else { w = os.Stdout } - + klog.InitFlags(nil) ctrl.SetLogger(zap.New(func(o *zap.Options) { o.Development = true o.DestWritter = w diff --git a/docs/examples/advanced-cue/app4.yaml b/docs/examples/advanced-cue/app4.yaml index 8dedb82be..81796f709 100644 --- a/docs/examples/advanced-cue/app4.yaml +++ b/docs/examples/advanced-cue/app4.yaml @@ -5,7 +5,7 @@ metadata: spec: components: - name: express-server4 - type: webserver + type: webservice settings: cmd: - node diff --git a/docs/examples/rollout/README.md b/docs/examples/rollout/README.md new file mode 100644 index 000000000..11b4d5484 --- /dev/null +++ b/docs/examples/rollout/README.md @@ -0,0 +1,14 @@ +# Rollout Example +[kubevela.io](https://kubevela.io) + +## Install kruise +```shell +helm install kruise https://github.com/openkruise/kruise/releases/download/v0.7.0/kruise-chart.tgz +kubectl apply -f charts/vela-core/crds +kubectl apply -f charts/vela-core/templates/defwithtemplate +kubectl apply -f docs/examples/rollout/clonesetDefinition.yaml +kubectl apply -f docs/examples/rollout/app-source.yaml +kubectl apply -f docs/examples/rollout/app-source-prep.yaml +kubectl apply -f docs/examples/rollout/app-target.yaml +kubectl apply -f docs/examples/rollout/app-deploy.yaml +``` \ No newline at end of file diff --git a/docs/examples/rollout/app-deploy.yaml b/docs/examples/rollout/app-deploy.yaml new file mode 100644 index 000000000..e82fbbed0 --- /dev/null +++ b/docs/examples/rollout/app-deploy.yaml @@ -0,0 +1,18 @@ +apiVersion: core.oam.dev/v1alpha2 +kind: ApplicationDeployment +metadata: + name: rolling-test +spec: + # application (revision) reference + targetApplicationName: test-rolling-v2 + sourceApplicationName: test-rolling-v1 + # HPA reference (optional) + componentList: + - metrics-provider + rolloutPlan: + rolloutStrategy: "IncreaseFirst" + rolloutBatches: + - replicas: 10% + - replicas: 2 + - replicas: 2 + batchPartition: 2 diff --git a/docs/examples/rollout/app-source-prep.yaml b/docs/examples/rollout/app-source-prep.yaml new file mode 100644 index 000000000..7051a781e --- /dev/null +++ b/docs/examples/rollout/app-source-prep.yaml @@ -0,0 +1,17 @@ +apiVersion: core.oam.dev/v1alpha2 +kind: Application +metadata: + name: test-rolling + annotations: + "app.oam.dev/rollout-template": "true" +spec: + components: + - name: metrics-provider + type: clonesetservice + settings: + 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/rollout/app-source.yaml b/docs/examples/rollout/app-source.yaml new file mode 100644 index 000000000..b70719ded --- /dev/null +++ b/docs/examples/rollout/app-source.yaml @@ -0,0 +1,15 @@ +apiVersion: core.oam.dev/v1alpha2 +kind: Application +metadata: + name: test-rolling +spec: + components: + - name: metrics-provider + type: clonesetservice + settings: + 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/rollout/app-target.yaml b/docs/examples/rollout/app-target.yaml new file mode 100644 index 000000000..c9d015246 --- /dev/null +++ b/docs/examples/rollout/app-target.yaml @@ -0,0 +1,17 @@ +apiVersion: core.oam.dev/v1alpha2 +kind: Application +metadata: + name: test-rolling + annotations: + "app.oam.dev/rollout-template": "true" +spec: + components: + - name: metrics-provider + type: clonesetservice + settings: + 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/rollout/clonesetDefinition.yaml b/docs/examples/rollout/clonesetDefinition.yaml new file mode 100644 index 000000000..d292eeeab --- /dev/null +++ b/docs/examples/rollout/clonesetDefinition.yaml @@ -0,0 +1,103 @@ +# Code generated by KubeVela templates. DO NOT EDIT. +apiVersion: core.oam.dev/v1alpha2 +kind: WorkloadDefinition +metadata: + name: clonesetservice + annotations: + definition.oam.dev/description: "Describes long-running, scalable, containerized services that have a stable network endpoint to receive external network traffic from customers. + If workload type is skipped for any service defined in Appfile, it will be defaulted to `webservice` type." +spec: + definitionRef: + name: clonesets.apps.kruise.io + template: | + output: { + apiVersion: "apps.kruise.io/v1alpha1" + kind: "CloneSet" + metadata: labels: { + "app.oam.dev/component": context.name + } + spec: { + replicas: parameter.replicas + selector: matchLabels: { + "app.oam.dev/component": context.name + } + + template: { + metadata: labels: { + "app.oam.dev/component": context.name + } + + spec: { + containers: [{ + name: context.name + image: parameter.image + + if parameter["cmd"] != _|_ { + command: parameter.cmd + } + + if parameter["env"] != _|_ { + env: parameter.env + } + + if context["config"] != _|_ { + env: context.config + } + + ports: [{ + containerPort: parameter.port + }] + + if parameter["cpu"] != _|_ { + resources: { + limits: + cpu: parameter.cpu + requests: + cpu: parameter.cpu + } + } + }] + } + } + if parameter["updateStrategyType"] != _|_ { + updateStrategy: { + type: parameter.updateStrategyType + } + } + } + } + parameter: { + // +usage=Which image would you like to use for your service + // +short=i + image: string + + // +usage=Commands to run in the container + cmd?: [...string] + + // +usage=Which port do you want customer traffic sent to + // +short=p + port: *80 | int + // +usage=Define arguments by using environment variables + env?: [...{ + // +usage=Environment variable name + name: string + // +usage=The value of the environment variable + value?: string + // +usage=Specifies a source the value of this var should come from + valueFrom?: { + // +usage=Selects a key of a secret in the pod's namespace + secretKeyRef: { + // +usage=The name of the secret in the pod's namespace to select from + name: string + // +usage=The key of the secret to select from. Must be a valid secret key + key: string + } + } + }] + // +usage=Number of CPU units for the service, like `0.5` (0.5 CPU core), `1` (1 CPU core) + cpu?: string + // +usage=Cloneset updateStrategy, candidates are `ReCreate`/`InPlaceIfPossible`/`InPlaceOnly` + updateStrategyType?: string + // +usage=Number of pods in the cloneset + replicas: *5 | int + } \ No newline at end of file diff --git a/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationdeployments.yaml b/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationdeployments.yaml index 4890c248c..f51da80b3 100644 --- a/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationdeployments.yaml +++ b/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationdeployments.yaml @@ -45,6 +45,10 @@ spec: rolloutPlan: description: RolloutPlan is the details on how to rollout the resources properties: + batchPartition: + description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches + format: int32 + type: integer canaryMetric: description: CanaryMetric provides a way for the rollout process to automatically check certain metrics before complete the process items: @@ -96,10 +100,6 @@ spec: - name type: object type: array - lastBatchToRollout: - description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches - format: int32 - type: integer numBatches: description: The number of batches, default = 1 format: int32 diff --git a/legacy/charts/vela-core-legacy/crds/core.oam.dev_applications.yaml b/legacy/charts/vela-core-legacy/crds/core.oam.dev_applications.yaml index dfbefd12e..7f6177217 100644 --- a/legacy/charts/vela-core-legacy/crds/core.oam.dev_applications.yaml +++ b/legacy/charts/vela-core-legacy/crds/core.oam.dev_applications.yaml @@ -73,6 +73,10 @@ spec: rolloutPlan: description: RolloutPlan is the details on how to rollout the resources The controller simply replace the old resources with the new one if there is no rollout plan involved properties: + batchPartition: + description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches + format: int32 + type: integer canaryMetric: description: CanaryMetric provides a way for the rollout process to automatically check certain metrics before complete the process items: @@ -124,10 +128,6 @@ spec: - name type: object type: array - lastBatchToRollout: - description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches - format: int32 - type: integer numBatches: description: The number of batches, default = 1 format: int32 @@ -362,7 +362,6 @@ spec: required: - name - revision - - revisionHash type: object rollingState: description: RollingState is the Rollout State diff --git a/legacy/charts/vela-core-legacy/crds/core.oam.dev_components.yaml b/legacy/charts/vela-core-legacy/crds/core.oam.dev_components.yaml index 8eea6bde6..ec42f5c28 100644 --- a/legacy/charts/vela-core-legacy/crds/core.oam.dev_components.yaml +++ b/legacy/charts/vela-core-legacy/crds/core.oam.dev_components.yaml @@ -119,7 +119,6 @@ spec: required: - name - revision - - revisionHash type: object observedGeneration: description: The generation observed by the component controller. diff --git a/legacy/charts/vela-core-legacy/crds/standard.oam.dev_rollouttraits.yaml b/legacy/charts/vela-core-legacy/crds/standard.oam.dev_rollouttraits.yaml index 4e9821df1..ba2de4a54 100644 --- a/legacy/charts/vela-core-legacy/crds/standard.oam.dev_rollouttraits.yaml +++ b/legacy/charts/vela-core-legacy/crds/standard.oam.dev_rollouttraits.yaml @@ -35,6 +35,10 @@ spec: rolloutPlan: description: RolloutPlan is the details on how to rollout the resources properties: + batchPartition: + description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches + format: int32 + type: integer canaryMetric: description: CanaryMetric provides a way for the rollout process to automatically check certain metrics before complete the process items: @@ -86,10 +90,6 @@ spec: - name type: object type: array - lastBatchToRollout: - description: All pods in the batches up to the batchPartition (included) will have the target resource specification while the rest still have the source resource This is designed for the operators to manually rollout Default is the the number of batches which will rollout all the batches - format: int32 - type: integer numBatches: description: The number of batches, default = 1 format: int32 diff --git a/pkg/controller/common/rollout/rollout_plan_controller.go b/pkg/controller/common/rollout/rollout_plan_controller.go index 29c135fc0..6fcf12151 100644 --- a/pkg/controller/common/rollout/rollout_plan_controller.go +++ b/pkg/controller/common/rollout/rollout_plan_controller.go @@ -3,9 +3,11 @@ package rollout import ( "context" "fmt" + "reflect" "time" "github.com/crossplane/crossplane-runtime/pkg/event" + kruisev1 "github.com/openkruise/kruise-api/apps/v1alpha1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" @@ -37,15 +39,22 @@ type Controller struct { // NewRolloutPlanController creates a RolloutPlanController func NewRolloutPlanController(client client.Client, parentController oam.Object, recorder event.Recorder, - rolloutSpec *v1alpha1.RolloutPlan, - rolloutStatus *v1alpha1.RolloutStatus, targetWorkload, - sourceWorkload *unstructured.Unstructured) *Controller { + rolloutSpec *v1alpha1.RolloutPlan, rolloutStatus *v1alpha1.RolloutStatus, + targetWorkload, sourceWorkload *unstructured.Unstructured) *Controller { + initializedRolloutStatus := rolloutStatus.DeepCopy() + // use Mutation webhook? + if len(initializedRolloutStatus.RollingState) == 0 { + initializedRolloutStatus.RollingState = v1alpha1.VerifyingState + } + if len(initializedRolloutStatus.BatchRollingState) == 0 { + initializedRolloutStatus.BatchRollingState = v1alpha1.BatchInitializingState + } return &Controller{ client: client, parentController: parentController, recorder: recorder, rolloutSpec: rolloutSpec.DeepCopy(), - rolloutStatus: rolloutStatus.DeepCopy(), + rolloutStatus: initializedRolloutStatus, targetWorkload: targetWorkload, sourceWorkload: sourceWorkload, } @@ -56,16 +65,17 @@ func (r *Controller) Reconcile(ctx context.Context) (res reconcile.Result, statu klog.InfoS("Reconcile the rollout plan", "rollout Spec", r.rolloutSpec, "target workload", klog.KObj(r.targetWorkload)) if r.sourceWorkload != nil { - klog.InfoS("we will do rolling upgrades", "source workload", klog.KObj(r.sourceWorkload)) + klog.InfoS("We will do rolling upgrades", "source workload", klog.KObj(r.sourceWorkload)) } - klog.InfoS("rollout spec ", "rollout state", r.rolloutStatus.RollingState, "batch rolling state", + klog.InfoS("rollout status", "rollout state", r.rolloutStatus.RollingState, "batch rolling state", r.rolloutStatus.BatchRollingState, "current batch", r.rolloutStatus.CurrentBatch, "upgraded Replicas", r.rolloutStatus.UpgradedReplicas) - defer klog.InfoS("Finished reconciling rollout plan", "rollout state", status.RollingState, - "batch rolling state", status.BatchRollingState, "current batch", status.CurrentBatch, - "upgraded Replicas", status.UpgradedReplicas, "reconcile result ", res) - + defer func() { + klog.InfoS("Finished reconciling rollout plan", "rollout state", status.RollingState, + "batch rolling state", status.BatchRollingState, "current batch", status.CurrentBatch, + "upgraded Replicas", status.UpgradedReplicas, "reconcile result ", res) + }() status = r.rolloutStatus defer func() { @@ -293,6 +303,8 @@ func (r *Controller) validateRollingBatchStatus(totalSize int) bool { if i < currentBatch { batchSize, _ := intstr.GetValueFromIntOrPercent(&r.Replicas, totalSize, true) podCount += batchSize + } else { + break } } // the recorded number should be at least as much as the all the pods before the current batch @@ -327,12 +339,11 @@ func (r *Controller) GetWorkloadController() (workloads.WorkloadController, erro Name: r.targetWorkload.GetName(), } - switch kind { - case "CloneSet": - return workloads.NewCloneSetController(r.client, r.recorder, r.parentController, - r.rolloutSpec, r.rolloutStatus, target), nil - - default: - return nil, fmt.Errorf("the workload kind `%s` is not supported", kind) + if r.targetWorkload.GroupVersionKind().Group == kruisev1.GroupVersion.Group { + if r.targetWorkload.GetKind() == reflect.TypeOf(kruisev1.CloneSet{}).Name() { + return workloads.NewCloneSetController(r.client, r.recorder, r.parentController, + r.rolloutSpec, r.rolloutStatus, target), nil + } } + return nil, fmt.Errorf("the workload kind `%s` is not supported", kind) } diff --git a/pkg/controller/common/rollout/workloads/cloneset_controller.go b/pkg/controller/common/rollout/workloads/cloneset_controller.go index a19af6bf8..b71e63926 100644 --- a/pkg/controller/common/rollout/workloads/cloneset_controller.go +++ b/pkg/controller/common/rollout/workloads/cloneset_controller.go @@ -92,19 +92,10 @@ func (c *CloneSetController) Verify(ctx context.Context) (status *v1alpha1.Rollo return } - // the rollout batch partition is either automatic or zero - if c.rolloutSpec.BatchPartition != nil && *c.rolloutSpec.BatchPartition != 0 { - verifyErr = fmt.Errorf("the rollout plan has to start from zero, partition= %d", *c.rolloutSpec.BatchPartition) - c.rolloutStatus.RolloutFailed(verifyErr.Error()) - return - } - - // Cloneset should not be in the middle of an upgrade (the number of new version pod should be 0) - oldVersionPod, _ := intstr.GetValueFromIntOrPercent(c.cloneSet.Spec.UpdateStrategy.Partition, int(totalReplicas), - true) - if oldVersionPod != int(totalReplicas) { - verifyErr = fmt.Errorf("the cloneset was still in the middle of updating, number of old pods= %d", oldVersionPod) - c.rolloutStatus.RolloutFailed(verifyErr.Error()) + if !c.cloneSet.Spec.UpdateStrategy.Paused { + verifyErr = fmt.Errorf("the cloneset %s is in the middle of updating, need to be paused first", + c.cloneSet.GetName()) + c.rolloutStatus.RolloutRetry(verifyErr.Error()) return } @@ -117,11 +108,22 @@ func (c *CloneSetController) Verify(ctx context.Context) (status *v1alpha1.Rollo // Initialize makes sure that func (c *CloneSetController) Initialize(ctx context.Context) *v1alpha1.RolloutStatus { - if c.fetchCloneSet(ctx) != nil { + totalReplicas, err := c.Size(ctx) + if err != nil { return c.rolloutStatus } + // kick start the update and start from every pod in the old version + clonePatch := client.MergeFrom(c.cloneSet.DeepCopyObject()) + c.cloneSet.Spec.UpdateStrategy.Paused = false + c.cloneSet.Spec.UpdateStrategy.Partition = &intstr.IntOrString{Type: intstr.Int, IntVal: totalReplicas} - // mark the rollout initialized, there is nothing we need to do for Cloneset for now + // patch the CloneSet + if err := c.client.Patch(ctx, c.cloneSet, clonePatch, client.FieldOwner(c.parentController.GetUID())); err != nil { + c.recorder.Event(c.parentController, event.Warning("Failed to the start the cloneset update", err)) + c.rolloutStatus.RolloutRetry(err.Error()) + return c.rolloutStatus + } + // mark the rollout initialized c.recorder.Event(c.parentController, event.Normal("Initialized", "Rollout resource are initialized")) c.rolloutStatus.StateTransition(v1alpha1.RollingInitializedEvent) return c.rolloutStatus @@ -139,7 +141,7 @@ func (c *CloneSetController) RolloutOneBatchPods(ctx context.Context) *v1alpha1. IntVal: cloneSetSize - int32(newPodTarget)} // patch the Cloneset if err := c.client.Patch(ctx, c.cloneSet, clonePatch, client.FieldOwner(c.parentController.GetUID())); err != nil { - c.recorder.Event(c.parentController, event.Warning("Failed to patch update the Cloneset", err)) + c.recorder.Event(c.parentController, event.Warning("Failed to update the cloneset to upgrade", err)) c.rolloutStatus.RolloutRetry(err.Error()) return c.rolloutStatus } diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go index cd6c2b157..0091d1545 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go @@ -35,7 +35,6 @@ import ( "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" "github.com/oam-dev/kubevela/pkg/appfile" core "github.com/oam-dev/kubevela/pkg/controller/core.oam.dev" - "github.com/oam-dev/kubevela/pkg/oam" "github.com/oam-dev/kubevela/pkg/oam/discoverymapper" oamutil "github.com/oam-dev/kubevela/pkg/oam/util" ) @@ -102,7 +101,6 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { } // pass the App label and annotation to ac except some app specific ones oamutil.PassLabelAndAnnotation(app, ac) - oamutil.RemoveAnnotations(ac, []string{oam.AnnotationAppRollout}) app.Status.SetConditions(readyCondition("Built")) applog.Info("apply appConfig & component to the cluster") // apply appConfig & component to the cluster @@ -113,7 +111,6 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { } app.Status.SetConditions(readyCondition("Applied")) - app.Status.Phase = v1alpha2.ApplicationHealthChecking applog.Info("check application health status") // check application health status diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller_test.go index 8d3a2471f..51caa8382 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller_test.go @@ -239,7 +239,7 @@ var _ = Describe("Test Application Controller", func() { Expect(component.Status.LatestRevision).ShouldNot(BeNil()) // check that the new appconfig has the correct annotation and labels - Expect(appConfig.GetAnnotations()[oam.AnnotationNewAppConfig]).Should(Equal(strconv.FormatBool(true))) + Expect(appConfig.GetAnnotations()[oam.AnnotationAppRollout]).Should(BeEmpty()) Expect(appConfig.GetLabels()[oam.LabelAppConfigHash]).ShouldNot(BeEmpty()) // check the workload created should be the same as the raw data in the component @@ -647,12 +647,6 @@ var _ = Describe("Test Application Controller", func() { Expect(json.Unmarshal(component7.Spec.Workload.Raw, gotD3)).Should(BeNil()) fmt.Println(cmp.Diff(gotD3, expDeployment7)) Expect(gotD3).Should(BeEquivalentTo(expDeployment7)) - - Expect(k8sClient.Get(ctx, client.ObjectKey{ - Namespace: app.Namespace, - Name: "myweb6", - }, component6)).Should(&util.NotFoundMatcher{}) - Expect(k8sClient.Delete(ctx, app)).Should(BeNil()) }) @@ -821,8 +815,8 @@ var _ = Describe("Test Application Controller", func() { compName := rolloutApp.Spec.Components[0].Name // set the annotation rolloutApp.SetAnnotations(map[string]string{ - oam.AnnotationAppRollout: "true", - "keep": "true", + oam.AnnotationAppRollout: strconv.FormatBool(true), + "keep": strconv.FormatBool(true), }) Expect(k8sClient.Create(ctx, rolloutApp)).Should(BeNil()) @@ -860,10 +854,9 @@ var _ = Describe("Test Application Controller", func() { Expect(component.Status.LatestRevision.Revision).Should( SatisfyAny(BeEquivalentTo(1), BeEquivalentTo(2))) // check that the new appconfig has the correct annotation and labels - Expect(appConfig.GetAnnotations()[oam.AnnotationNewAppConfig]).Should(Equal(strconv.FormatBool(true))) + Expect(appConfig.GetAnnotations()[oam.AnnotationAppRollout]).Should(Equal(strconv.FormatBool(true))) Expect(appConfig.GetAnnotations()[oam.AnnotationRollingComponent]).Should(Equal(component.Status.LatestRevision.Name)) Expect(appConfig.GetAnnotations()["keep"]).Should(Equal("true")) - Expect(appConfig.GetAnnotations()[oam.AnnotationAppRollout]).Should(BeEmpty()) Expect(appConfig.GetLabels()[oam.LabelAppConfigHash]).ShouldNot(BeEmpty()) Expect(appConfig.Spec.Components[0].ComponentName).Should(BeEmpty()) Expect(appConfig.Spec.Components[0].RevisionName).Should(Equal(component.Status.LatestRevision.Name)) diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/apply.go b/pkg/controller/core.oam.dev/v1alpha2/application/apply.go index 79231ddee..71f8bc548 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/apply.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/apply.go @@ -82,14 +82,7 @@ func (h *appHandler) apply(ctx context.Context, ac *v1alpha2.ApplicationConfigur Controller: pointer.BoolPtr(true), }} ac.SetOwnerReferences(owners) - hasRolloutLogic := false var newComponents []string - // Check if we are doing rolling out - if _, exist := h.app.GetAnnotations()[oam.AnnotationAppRollout]; exist || h.app.Spec.RolloutPlan != nil { - h.logger.Info("The application rolling out is controlled by a rollout plan") - hasRolloutLogic = true - } - for _, comp := range comps { comp.SetOwnerReferences(owners) newComp := comp.DeepCopy() @@ -98,7 +91,7 @@ func (h *appHandler) apply(ctx context.Context, ac *v1alpha2.ApplicationConfigur if err != nil { return err } - if newRevision && hasRolloutLogic { + if newRevision { newComponents = append(newComponents, revisionName) } // find the ACC that contains this component @@ -112,32 +105,14 @@ func (h *appHandler) apply(ctx context.Context, ac *v1alpha2.ApplicationConfigur } } // set the annotation on ac to point out which component are newly changed - ac.SetAnnotations(oamutil.MergeMapOverrideWithDst(ac.GetAnnotations(), map[string]string{ + // make sure that it won't remove the value on the subsequent reconcile loops + ac.SetAnnotations(oamutil.MergeMapOverrideWithDst(map[string]string{ oam.AnnotationRollingComponent: strings.Join(newComponents, common.RollingComponentsSep), - })) + }, ac.GetAnnotations())) if err := h.createOrUpdateAppConfig(ctx, ac); err != nil { return err } - // Garbage Collection for no used Components. - // There's no need to ApplicationConfiguration Garbage Collection, it has the same name with Application. - for _, comp := range h.app.Status.Components { - var exist = false - for _, cc := range comps { - if comp.Name == cc.Name { - exist = true - break - } - } - if exist { - continue - } - // Component not exits in current Application, should be deleted - var oldC = &v1alpha2.Component{ObjectMeta: metav1.ObjectMeta{Name: comp.Name, Namespace: ac.Namespace}} - if err := h.r.Delete(ctx, oldC); err != nil { - return err - } - } return nil } @@ -313,8 +288,9 @@ func (h *appHandler) createOrUpdateAppConfig(ctx context.Context, appConfig *v1a if curAppConfig.GetLabels()[oam.LabelAppConfigHash] == appConfig.GetLabels()[oam.LabelAppConfigHash] { // Just to be safe that it's not because of a random Hash collision if apiequality.Semantic.DeepEqual(&curAppConfig.Spec, &appConfig.Spec) { - // same spec, no need to create another AC - return nil + // same spec, no need to create another AC, still need to update the AC to apply label/annotation + oamutil.PassLabelAndAnnotation(appConfig, &curAppConfig) + return h.r.Update(ctx, &curAppConfig) } h.logger.Info("encountered a different app spec with same hash", "current spec", curAppConfig.Spec, "new appConfig spec", appConfig.Spec) @@ -336,11 +312,14 @@ func (h *appHandler) createNewAppConfig(ctx context.Context, appConfig *v1alpha2 RevisionHash: appConfig.GetLabels()[oam.LabelAppConfigHash], } appConfig.Name = revisionName - // indicate that the application is created by the applicationController - // appConfig controller should set this to false after the first successful reconcile - appConfig.SetAnnotations(oamutil.MergeMapOverrideWithDst(appConfig.GetAnnotations(), map[string]string{ - oam.AnnotationNewAppConfig: strconv.FormatBool(true), - })) + // indicate that the applicationConfig is created if we are doing rolling out + if _, exist := h.app.GetAnnotations()[oam.AnnotationAppRollout]; exist || h.app.Spec.RolloutPlan != nil { + h.logger.Info(fmt.Sprintf("The application %s rolling out is controlled by a rollout plan", h.app.Name)) + appConfig.SetAnnotations(oamutil.MergeMapOverrideWithDst(appConfig.GetAnnotations(), map[string]string{ + oam.AnnotationAppRollout: strconv.FormatBool(true), + })) + } + // record that last appConfig we created first in the app's status // make sure that we persist the latest revision first if err := h.r.UpdateStatus(ctx, h.app); err != nil { diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go index 4a1f64c86..09f47cf8e 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go @@ -112,17 +112,19 @@ var _ = Describe("Test Application apply", func() { By("Verify that the application status has the lastRevision name ") Expect(curApp.Status.LatestRevision.Revision).Should(BeEquivalentTo(1)) - Expect(curApp.Status.LatestRevision.Name).Should(Equal(app.Name + "-v1")) + Expect(curApp.Status.LatestRevision.Name).Should(Equal(utils.ConstructRevisionName(app.Name, 1))) curAC := &v1alpha2.ApplicationConfiguration{} Expect(handler.r.Get(ctx, - types.NamespacedName{Namespace: ns.Name, Name: app.Name + "-v1"}, + types.NamespacedName{Namespace: ns.Name, Name: utils.ConstructRevisionName(app.Name, 1)}, curAC)).NotTo(HaveOccurred()) // check that the annotation/labels are correctly applied - Expect(curAC.GetAnnotations()[oam.AnnotationNewAppConfig]).Should(BeIdenticalTo("true")) Expect(curAC.GetLabels()[oam.LabelAppConfigHash]).ShouldNot(BeEmpty()) hashValue := curAC.GetLabels()[oam.LabelAppConfigHash] + Expect(hashValue).ShouldNot(BeEmpty()) Expect(curApp.Status.LatestRevision.RevisionHash).Should(Equal(hashValue)) + // TODO: verify that label and annotation change will be passed down + By("[TEST] apply the same appConfig mimic application controller, should do nothing") // this should not lead to a new AC err = handler.createOrUpdateAppConfig(ctx, appConfig.DeepCopy()) @@ -138,9 +140,10 @@ var _ = Describe("Test Application apply", func() { By("Verify that the lastest revision does not change") Expect(curApp.Status.LatestRevision.Revision).Should(BeEquivalentTo(1)) - Expect(curApp.Status.LatestRevision.Name).Should(Equal(app.Name + "-v1")) + Expect(curApp.Status.LatestRevision.Name).Should(Equal(utils.ConstructRevisionName(app.Name, 1))) + Expect(curApp.Status.LatestRevision.RevisionHash).Should(Equal(hashValue)) Expect(handler.r.Get(ctx, - types.NamespacedName{Namespace: ns.Name, Name: app.Name + "-v1"}, + types.NamespacedName{Namespace: ns.Name, Name: curApp.Status.LatestRevision.Name}, curAC)).NotTo(HaveOccurred()) By("[TEST] Modify the applicationConfiguration mimic AC controller, should only update") @@ -148,8 +151,8 @@ var _ = Describe("Test Application apply", func() { curAC.Status.SetConditions(readyCondition("newType")) Expect(handler.r.Status().Update(ctx, curAC)).NotTo(HaveOccurred()) // set the new AppConfig annotation as false AC controller would do - cl := curAC.GetAnnotations() - cl[oam.AnnotationNewAppConfig] = strconv.FormatBool(false) + cl := make(map[string]string) + cl[oam.AnnotationAppRollout] = strconv.FormatBool(false) curAC.SetAnnotations(cl) Expect(handler.r.Update(ctx, curAC)).NotTo(HaveOccurred()) // this should not lead to a new AC @@ -166,17 +169,18 @@ var _ = Describe("Test Application apply", func() { By("Verify that the lastest revision does not change") Expect(curApp.Status.LatestRevision.Revision).Should(BeEquivalentTo(1)) - Expect(curApp.Status.LatestRevision.Name).Should(Equal(app.Name + "-v1")) + Expect(curApp.Status.LatestRevision.Name).Should(Equal(utils.ConstructRevisionName(app.Name, 1))) + Expect(curApp.Status.LatestRevision.RevisionHash).Should(Equal(hashValue)) Expect(handler.r.Get(ctx, - types.NamespacedName{Namespace: ns.Name, Name: app.Name + "-v1"}, + types.NamespacedName{Namespace: ns.Name, Name: curApp.Status.LatestRevision.Name}, curAC)).NotTo(HaveOccurred()) // check that the new app annotation is false - Expect(curAC.GetAnnotations()[oam.AnnotationNewAppConfig]).Should(Equal(strconv.FormatBool(false))) + Expect(curAC.GetAnnotations()[oam.AnnotationAppRollout]).Should(Equal(strconv.FormatBool(false))) Expect(curAC.GetLabels()[oam.LabelAppConfigHash]).Should(Equal(hashValue)) Expect(curAC.GetCondition("newType").Status).Should(BeEquivalentTo(corev1.ConditionTrue)) // check that no new appConfig created - Expect(handler.r.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: app.Name + "-v2"}, - curAC)).Should(&oamutil.NotFoundMatcher{}) + Expect(handler.r.Get(ctx, types.NamespacedName{Namespace: ns.Name, + Name: utils.ConstructRevisionName(app.Name, 2)}, curAC)).Should(&oamutil.NotFoundMatcher{}) By("[TEST] Modify the applicationConfiguration spec, should lead to a new AC") // update the spec of the AC which should lead to a new AC being created @@ -215,9 +219,8 @@ var _ = Describe("Test Application apply", func() { // check that the new app annotation exist and the hash value has changed Expect(handler.r.Get(ctx, - types.NamespacedName{Namespace: ns.Name, Name: app.Name + "-v2"}, + types.NamespacedName{Namespace: ns.Name, Name: curApp.Status.LatestRevision.Name}, curAC)).NotTo(HaveOccurred()) - Expect(curAC.GetAnnotations()[oam.AnnotationNewAppConfig]).Should(BeIdenticalTo(strconv.FormatBool(true))) Expect(curAC.GetLabels()[oam.LabelAppConfigHash]).ShouldNot(BeEmpty()) Expect(curAC.GetLabels()[oam.LabelAppConfigHash]).ShouldNot(Equal(hashValue)) // check that no more new appConfig created diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/suite_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/suite_test.go index f71fd86f7..ea9e068fc 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/suite_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/suite_test.go @@ -22,12 +22,11 @@ import ( "testing" "time" - "github.com/crossplane/crossplane-runtime/pkg/logging" - "github.com/go-logr/logr" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "sigs.k8s.io/controller-runtime/pkg/source" + "github.com/crossplane/crossplane-runtime/pkg/logging" + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" @@ -38,6 +37,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/envtest/printer" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/controller-runtime/pkg/source" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go index a39c57f73..08d30db86 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go @@ -52,12 +52,8 @@ import ( const ( reconcileTimeout = 1 * time.Minute dependCheckWait = 10 * time.Second - shortWait = 30 * time.Second - longWait = 1 * time.Minute ) -var errResult = reconcile.Result{RequeueAfter: shortWait} - // Reconcile error strings. const ( errGetAppConfig = "cannot get application configuration" @@ -230,7 +226,7 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco // stop processing this resource return ctrl.Result{}, nil } - return errResult, errors.Wrap(err, errGetAppConfig) + return reconcile.Result{}, errors.Wrap(err, errGetAppConfig) } acPatch := ac.DeepCopy() if ac.ObjectMeta.DeletionTimestamp.IsZero() { @@ -252,8 +248,8 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco // make sure this is the last functional defer function to be called defer func() { // Make sure if error occurs, reconcile will not happen too frequency - if returnErr != nil && result.RequeueAfter < shortWait { - result.RequeueAfter = shortWait + if returnErr != nil { + result.RequeueAfter = 0 } }() @@ -291,11 +287,11 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco // we have special logics for application generated applicationConfiguration if isControlledByApp(ac) { - if ac.GetAnnotations()[oam.AnnotationNewAppConfig] == strconv.FormatBool(true) { + if ac.GetAnnotations()[oam.AnnotationAppRollout] == strconv.FormatBool(true) { defer func() { // we only flip the annotation after a successful reconcile if returnErr == nil { - ac.GetAnnotations()[oam.AnnotationNewAppConfig] = strconv.FormatBool(false) + ac.GetAnnotations()[oam.AnnotationAppRollout] = strconv.FormatBool(false) returnErr = r.client.Update(ctx, ac) } }() @@ -304,20 +300,20 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco workloads, depStatus, err := r.components.Render(ctx, ac) if err != nil { - log.Info("Cannot render components", "error", err, "requeue-after", time.Now().Add(shortWait)) + log.Info("Cannot render components", "error", err) r.record.Event(ac, event.Warning(reasonCannotRenderComponents, err)) ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errRenderComponents))) - return errResult, errors.Wrap(r.UpdateStatus(ctx, ac), errUpdateAppConfigStatus) + return reconcile.Result{}, errors.Wrap(r.UpdateStatus(ctx, ac), errUpdateAppConfigStatus) } log.Debug("Successfully rendered components", "workloads", len(workloads)) r.record.Event(ac, event.Normal(reasonRenderComponents, "Successfully rendered components", "workloads", strconv.Itoa(len(workloads)))) applyOpts := []apply.ApplyOption{apply.MustBeControllableBy(ac.GetUID()), applyOnceOnly(ac, r.applyOnceOnlyMode, log)} if err := r.workloads.Apply(ctx, ac.Status.Workloads, workloads, applyOpts...); err != nil { - log.Debug("Cannot apply workload", "error", err, "requeue-after", time.Now().Add(shortWait)) + log.Debug("Cannot apply workload", "error", err) r.record.Event(ac, event.Warning(reasonCannotApplyComponents, err)) ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errApplyComponents))) - return errResult, errors.Wrap(r.UpdateStatus(ctx, ac), errUpdateAppConfigStatus) + return reconcile.Result{}, errors.Wrap(r.UpdateStatus(ctx, ac), errUpdateAppConfigStatus) } log.Debug("Successfully applied components", "workloads", len(workloads)) r.record.Event(ac, event.Normal(reasonApplyComponents, "Successfully applied components", "workloads", strconv.Itoa(len(workloads)))) @@ -334,10 +330,10 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco record := r.record.WithAnnotations("kind", e.GetKind(), "name", e.GetName()) if err := r.client.Delete(ctx, &e); resource.IgnoreNotFound(err) != nil { - log.Debug("Cannot garbage collect component", "error", err, "requeue-after", time.Now().Add(shortWait)) + log.Debug("Cannot garbage collect component", "error", err) record.Event(ac, event.Warning(reasonCannotGGComponents, err)) ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errGCComponent))) - return errResult, errors.Wrap(r.UpdateStatus(ctx, ac), errUpdateAppConfigStatus) + return reconcile.Result{}, errors.Wrap(r.UpdateStatus(ctx, ac), errUpdateAppConfigStatus) } log.Debug("Garbage collected resource") record.Event(ac, event.Normal(reasonGGComponent, "Successfully garbage collected component")) @@ -347,7 +343,7 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco r.updateStatus(ctx, ac, acPatch, workloads) ac.Status.Dependency = v1alpha2.DependencyStatus{} - waitTime := longWait + var waitTime time.Duration if len(depStatus.Unsatisfied) != 0 { waitTime = dependCheckWait ac.Status.Dependency = *depStatus @@ -464,6 +460,9 @@ func hasScope(ac *v1alpha2.ApplicationConfiguration) bool { // A Workload produced by an OAM ApplicationConfiguration. type Workload struct { + // SkipApply indicates that the workload should not be applied + SkipApply bool + // ComponentName that produced this workload. ComponentName string diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go index c532f2328..1c4317893 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go @@ -41,7 +41,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" - "github.com/oam-dev/kubevela/pkg/oam/mock" "github.com/oam-dev/kubevela/pkg/utils/apply" ) @@ -158,7 +157,7 @@ func TestReconciler(t *testing.T) { }, }, want: want{ - result: reconcile.Result{RequeueAfter: shortWait}, + result: reconcile.Result{}, err: errors.Wrap(errBoom, errGetAppConfig), }, }, @@ -187,7 +186,7 @@ func TestReconciler(t *testing.T) { }, }, want: want{ - result: reconcile.Result{RequeueAfter: shortWait}, + result: reconcile.Result{}, }, }, "ApplyComponentsError": { @@ -217,7 +216,7 @@ func TestReconciler(t *testing.T) { }, }, want: want{ - result: reconcile.Result{RequeueAfter: shortWait}, + result: reconcile.Result{}, }, }, "GCDeleteError": { @@ -251,7 +250,7 @@ func TestReconciler(t *testing.T) { }, }, want: want{ - result: reconcile.Result{RequeueAfter: shortWait}, + result: reconcile.Result{}, }, }, "Has dependency": { @@ -355,7 +354,7 @@ func TestReconciler(t *testing.T) { return reconcile.Result{RequeueAfter: 15 * time.Second}, errBoom })), WithPosthook("postHook", ControllerHooksFn(func(ctx context.Context, ac *v1alpha2.ApplicationConfiguration, logger logging.Logger) (reconcile.Result, error) { - return reconcile.Result{RequeueAfter: shortWait}, nil + return reconcile.Result{}, nil })), }, }, @@ -423,7 +422,7 @@ func TestReconciler(t *testing.T) { return []unstructured.Unstructured{*trait} })), WithPosthook("preHookSuccess", ControllerHooksFn(func(ctx context.Context, ac *v1alpha2.ApplicationConfiguration, logger logging.Logger) (reconcile.Result, error) { - return reconcile.Result{RequeueAfter: shortWait}, nil + return reconcile.Result{}, nil })), WithPosthook("preHookFailed", ControllerHooksFn(func(ctx context.Context, ac *v1alpha2.ApplicationConfiguration, logger logging.Logger) (reconcile.Result, error) { return reconcile.Result{RequeueAfter: 15 * time.Second}, errBoom @@ -474,7 +473,7 @@ func TestReconciler(t *testing.T) { return reconcile.Result{RequeueAfter: 15 * time.Second}, errBoom })), WithPosthook("preHookSuccess", ControllerHooksFn(func(ctx context.Context, ac *v1alpha2.ApplicationConfiguration, logger logging.Logger) (reconcile.Result, error) { - return reconcile.Result{RequeueAfter: shortWait}, nil + return reconcile.Result{}, nil })), WithPosthook("preHookFailed", ControllerHooksFn(func(ctx context.Context, ac *v1alpha2.ApplicationConfiguration, logger logging.Logger) (reconcile.Result, error) { return reconcile.Result{RequeueAfter: 15 * time.Second}, errBoom @@ -541,15 +540,15 @@ func TestReconciler(t *testing.T) { return []unstructured.Unstructured{*trait} })), WithPrehook("preHook", ControllerHooksFn(func(ctx context.Context, ac *v1alpha2.ApplicationConfiguration, logger logging.Logger) (reconcile.Result, error) { - return reconcile.Result{RequeueAfter: shortWait}, nil + return reconcile.Result{}, nil })), WithPosthook("postHook", ControllerHooksFn(func(ctx context.Context, ac *v1alpha2.ApplicationConfiguration, logger logging.Logger) (reconcile.Result, error) { - return reconcile.Result{RequeueAfter: shortWait}, nil + return reconcile.Result{}, nil })), }, }, want: want{ - result: reconcile.Result{RequeueAfter: longWait}, + result: reconcile.Result{RequeueAfter: 0}, }, }, "RegisterFinalizer": { diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go index 49e173df6..9c29dc42d 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply.go @@ -33,6 +33,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" + "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" @@ -104,7 +105,11 @@ func (a *workloads) Apply(ctx context.Context, status []v1alpha2.WorkloadStatus, // they are all in the same namespace var namespace = w[0].Workload.GetNamespace() for _, wl := range w { - if !wl.HasDep { + if wl.SkipApply { + klog.InfoS("skip apply a workload due to rollout", "component name", wl.ComponentName, "component revision", + wl.ComponentRevisionName) + } + if !wl.HasDep && !wl.SkipApply { // Apply the DataInputs to this workload if err := a.ApplyInputRef(ctx, wl.Workload, wl.DataInputs, namespace, ao...); err != nil { return err diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go index 1a2fe56b1..dbeb45dcc 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render.go @@ -99,22 +99,23 @@ func (r *components) Render(ctx context.Context, ac *v1alpha2.ApplicationConfigu workloads := make([]*Workload, 0, len(ac.Spec.Components)) dag := newDAG() - // we have special logics for application generated applicationConfiguration - isControlledByApp := isControlledByApp(ac) + // we have special logics for application generated applicationConfiguration during rolling out phase rollingComponents := make(map[string]bool) - if isControlledByApp { - if ac.GetAnnotations()[oam.AnnotationNewAppConfig] == strconv.FormatBool(true) { - if anc, exist := ac.GetAnnotations()[oam.AnnotationRollingComponent]; exist { - // the new components annotation contains all the changed components in the application - for _, revisionName := range strings.Split(anc, common.RollingComponentsSep) { - rollingComponents[revisionName] = true - } + var rolloutTemplate string + var exist bool + if rolloutTemplate, exist = ac.GetAnnotations()[oam.AnnotationAppRollout]; exist { + // we only care about the new components when there is rolling out, no matter if it's true or not + if anc, exist := ac.GetAnnotations()[oam.AnnotationRollingComponent]; exist { + // the new components annotation contains all the changed components in the application + for _, revisionName := range strings.Split(anc, common.RollingComponentsSep) { + rollingComponents[revisionName] = true } } } for _, acc := range ac.Spec.Components { - w, err := r.renderComponent(ctx, acc, ac, isControlledByApp, rollingComponents[acc.RevisionName], dag) + w, err := r.renderComponent(ctx, acc, ac, isControlledByApp(ac), rollingComponents[acc.RevisionName], + rolloutTemplate == strconv.FormatBool(true), dag) if err != nil { return nil, nil, err } @@ -136,12 +137,14 @@ func (r *components) Render(ctx context.Context, ac *v1alpha2.ApplicationConfigu } func (r *components) renderComponent(ctx context.Context, acc v1alpha2.ApplicationConfigurationComponent, - ac *v1alpha2.ApplicationConfiguration, isControlledByApp, isCompRolling bool, dag *dag) (*Workload, error) { + ac *v1alpha2.ApplicationConfiguration, isControlledByApp, isChanged, isRolloutTemplate bool, dag *dag) (*Workload, error) { if acc.RevisionName != "" { acc.ComponentName = utils.ExtractComponentName(acc.RevisionName) } klog.InfoS("render a component", "component name", acc.ComponentName, "component revision", acc.RevisionName, - "is generated by application", isControlledByApp, "is a newly changed component", isCompRolling) + "is generated by application", isControlledByApp, "is a changed component", isChanged, + "is the appConfig a rollout template", isRolloutTemplate) + c, componentRevisionName, err := util.GetComponent(ctx, r.client, acc, ac.GetNamespace()) if err != nil { return nil, err @@ -171,7 +174,7 @@ func (r *components) renderComponent(ctx context.Context, acc v1alpha2.Applicati // pass through labels and annotation from app-config to workload util.PassLabelAndAnnotation(ac, w) // don't pass the following annotation as those are for appConfig only - util.RemoveAnnotations(w, []string{oam.AnnotationNewAppConfig, oam.AnnotationRollingComponent}) + util.RemoveAnnotations(w, []string{oam.AnnotationAppRollout, oam.AnnotationRollingComponent}) ref := metav1.NewControllerRef(ac, v1alpha2.ApplicationConfigurationGroupVersionKind) w.SetOwnerReferences([]metav1.OwnerReference{*ref}) w.SetNamespace(ac.GetNamespace()) @@ -190,7 +193,7 @@ func (r *components) renderComponent(ctx context.Context, acc v1alpha2.Applicati // pass through labels and annotation from app-config to trait util.PassLabelAndAnnotation(ac, t) - util.RemoveAnnotations(t, []string{oam.AnnotationNewAppConfig, oam.AnnotationRollingComponent}) + util.RemoveAnnotations(t, []string{oam.AnnotationAppRollout, oam.AnnotationRollingComponent}) traits = append(traits, &Trait{Object: *t, Definition: *traitDef}) traitDefs = append(traitDefs, *traitDef) } @@ -210,9 +213,9 @@ func (r *components) renderComponent(ctx context.Context, acc v1alpha2.Applicati return nil, err } SetAppWorkloadInstanceName(acc.ComponentName, w, revision) - if isCompRolling { - // we have a special logic to emit the workload disabled so that the rollout process can take over - // this is the only place the appConfig controller is involved in the rollout process + if isChanged && isRolloutTemplate { + // we have a special logic to emit the workload disabled for the first time so that the rollout + // controller can take over. if err := prepWorkloadInstanceForRollout(w); err != nil { return nil, err } @@ -247,9 +250,11 @@ func (r *components) renderComponent(ctx context.Context, acc v1alpha2.Applicati } addDataOutputsToDAG(dag, acc.DataOutputs, w) - - return &Workload{ComponentName: acc.ComponentName, ComponentRevisionName: componentRevisionName, - Workload: w, Traits: traits, RevisionEnabled: isRevisionEnabled(traitDefs), Scopes: scopes}, nil + // To avoid conflict with rollout controller, we will not render the workload until the rollout phase is over + // indicated by the AnnotationAppRollout annotation disappear + return &Workload{SkipApply: isChanged && !isRolloutTemplate, ComponentName: acc.ComponentName, + ComponentRevisionName: componentRevisionName, + Workload: w, Traits: traits, RevisionEnabled: isRevisionEnabled(traitDefs), Scopes: scopes}, nil } func (r *components) renderTrait(ctx context.Context, ct v1alpha2.ComponentTrait, ac *v1alpha2.ApplicationConfiguration, diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render_test.go index f49b6ccce..7bd2436e6 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/render_test.go @@ -92,20 +92,27 @@ func TestRender(t *testing.T) { }, } - controlledAC := revAC.DeepCopy() - controlledAC.OwnerReferences = []metav1.OwnerReference{ + controlledTemplateAC := revAC.DeepCopy() + controlledTemplateAC.OwnerReferences = []metav1.OwnerReference{ { APIVersion: v1alpha2.SchemeGroupVersion.String(), Kind: v1alpha2.ApplicationKind, Controller: pointer.BoolPtr(true), }, } - controlledAC.Spec.Components[0].RevisionName = revisionName2 - controlledAC.SetAnnotations(map[string]string{ - oam.AnnotationNewAppConfig: strconv.FormatBool(true), + controlledTemplateAC.Spec.Components[0].RevisionName = revisionName2 + controlledTemplateAC.SetAnnotations(map[string]string{ + oam.AnnotationAppRollout: strconv.FormatBool(true), oam.AnnotationRollingComponent: revisionName + "," + revisionName2, "keep": strconv.FormatBool(true), }) + controlledNoneTemplateAC := controlledTemplateAC.DeepCopy() + controlledNoneTemplateAC.SetAnnotations(map[string]string{ + oam.AnnotationAppRollout: strconv.FormatBool(false), + oam.AnnotationRollingComponent: revisionName + "," + revisionName2, + "keep": strconv.FormatBool(true), + }) + ref := metav1.NewControllerRef(ac, v1alpha2.ApplicationConfigurationGroupVersionKind) errTrait := errors.New("errTrait") @@ -599,7 +606,7 @@ func TestRender(t *testing.T) { return t, nil }), }, - args: args{ac: controlledAC}, + args: args{ac: controlledTemplateAC}, want: want{ w: []Workload{ { @@ -677,7 +684,7 @@ func TestRender(t *testing.T) { return t, nil }), }, - args: args{ac: controlledAC}, + args: args{ac: controlledTemplateAC}, want: want{ w: []Workload{ { @@ -698,6 +705,85 @@ func TestRender(t *testing.T) { }, }, }, + "Success-With-NoneTemplate-Deployment": { + reason: "We do not render the workload after the template is out", + fields: fields{ + client: &test.MockClient{MockGet: test.NewMockGetFn(nil, func(obj runtime.Object) error { + switch defObj := obj.(type) { + case *v1alpha2.Component: + ccomp := v1alpha2.Component{ + Status: v1alpha2.ComponentStatus{ + LatestRevision: &v1alpha2.Revision{Name: revisionName2}, + }, + } + ccomp.DeepCopyInto(defObj) + case *v1alpha2.TraitDefinition: + ttrait := v1alpha2.TraitDefinition{ObjectMeta: metav1.ObjectMeta{Name: traitName}, + Spec: v1alpha2.TraitDefinitionSpec{RevisionEnabled: true}} + ttrait.DeepCopyInto(defObj) + case *v1.ControllerRevision: + rev := &v1.ControllerRevision{ + ObjectMeta: metav1.ObjectMeta{Name: revisionName, Namespace: namespace}, + Data: runtime.RawExtension{Object: &v1alpha2.Component{ + ObjectMeta: metav1.ObjectMeta{ + Name: componentName, + Namespace: namespace, + }, + Spec: v1alpha2.ComponentSpec{ + Workload: runtime.RawExtension{ + Object: &unstructured.Unstructured{}, + }, + }, + Status: v1alpha2.ComponentStatus{ + LatestRevision: &v1alpha2.Revision{Name: revisionName2}, + }, + }}, + Revision: 2, + } + rev.DeepCopyInto(defObj) + } + return nil + })}, + params: ParameterResolveFn(func(_ []v1alpha2.ComponentParameter, _ []v1alpha2.ComponentParameterValue) ([]Parameter, error) { + return nil, nil + }), + workload: ResourceRenderFn(func(_ []byte, _ ...Parameter) (*unstructured.Unstructured, error) { + w := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + }, + } + return w, nil + }), + trait: ResourceRenderFn(func(_ []byte, _ ...Parameter) (*unstructured.Unstructured, error) { + t := &unstructured.Unstructured{} + t.SetName(traitName) + return t, nil + }), + }, + args: args{ac: controlledNoneTemplateAC}, + want: want{ + w: []Workload{ + { + SkipApply: true, + ComponentName: componentName, + ComponentRevisionName: revisionName2, + Workload: func() *unstructured.Unstructured { + w := &unstructured.Unstructured{} + w.SetNamespace(namespace) + w.SetName(revisionName2) + w.SetOwnerReferences([]metav1.OwnerReference{*ref}) + w.SetAnnotations(map[string]string{ + oam.AnnotationAppGeneration: "0", + }) + return w + }(), + RevisionEnabled: true, + }, + }, + }, + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { @@ -718,9 +804,9 @@ func TestRender(t *testing.T) { if diff := cmp.Diff(tc.want.w[0].Workload.GetName(), got[0].Workload.GetName()); diff != "" { t.Errorf("\n%s\nr.Render(...): -want, +got:\n%s\n", tc.reason, diff) } - if _, exit := got[0].Workload.GetAnnotations()[oam.AnnotationNewAppConfig]; exit { + if _, exit := got[0].Workload.GetAnnotations()[oam.AnnotationAppRollout]; exit { t.Errorf("\n%s\nr.Render(...) workload should not get annotation:%s\n", tc.reason, - oam.AnnotationNewAppConfig) + oam.AnnotationAppRollout) } if _, exit := got[0].Workload.GetAnnotations()[oam.AnnotationRollingComponent]; exit { t.Errorf("\n%s\nr.Render(...) workload should not get annotation:%s\n", tc.reason, @@ -738,6 +824,10 @@ func TestRender(t *testing.T) { t.Errorf("\n%s\nr.Render(...): trait should get annotation:%s\n", tc.reason, "keep") } + isTempalte := tc.args.ac.GetAnnotations()[oam.AnnotationAppRollout] == strconv.FormatBool(true) + if !isTempalte && !got[0].SkipApply { + t.Errorf("\n%s\nr.Render(...): none template workload should be skip apply\n", tc.reason) + } } else { if diff := cmp.Diff(tc.want.w, got); diff != "" { t.Errorf("\n%s\nr.Render(...): -want, +got:\n%s\n", tc.reason, diff) @@ -756,7 +846,8 @@ func TestRenderComponent(t *testing.T) { type arg struct { ac *v1alpha2.ApplicationConfiguration isControlledByApp bool - isCompRolling bool + isCompChanged bool + isRollingTemplate bool dag *dag } type want struct { @@ -837,6 +928,46 @@ func TestRenderComponent(t *testing.T) { //TODO: Add more failure cases // add more dependency related tests for any future changes // add more trait related tests + "Newly-Changed-Component-NotRolling": { + reason: "newly changed workload should not be rendered if it's not rolling anymore (not the first time)", + fields: field{ + client: &test.MockClient{MockGet: mockGet}, + + workload: ResourceRenderFn(func(_ []byte, _ ...Parameter) (*unstructured.Unstructured, error) { + w := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + }, + } + return w, nil + }), + trait: ResourceRenderFn(func(_ []byte, _ ...Parameter) (*unstructured.Unstructured, error) { + t := &unstructured.Unstructured{} + t.SetName(traitName) + return t, nil + }), + }, + args: arg{ + ac: revAC, + isControlledByApp: true, + isCompChanged: true, + isRollingTemplate: false, + }, + want: want{ + w: Workload{ + ComponentName: componentName, + ComponentRevisionName: revisionName, + Workload: func() *unstructured.Unstructured { + w := &unstructured.Unstructured{} + w.SetName(revisionName) + w.SetOwnerReferences([]metav1.OwnerReference{*ref}) + return w + }(), + RevisionEnabled: true, + }, + }, + }, "Success-With-Newly-Changed-Component-Deployment": { reason: "Workload name should be revision name for deployment and it should be disabled", fields: field{ @@ -860,7 +991,8 @@ func TestRenderComponent(t *testing.T) { args: arg{ ac: revAC, isControlledByApp: true, - isCompRolling: true, + isCompChanged: true, + isRollingTemplate: true, }, want: want{ w: Workload{ @@ -899,7 +1031,8 @@ func TestRenderComponent(t *testing.T) { args: arg{ ac: revAC2, isControlledByApp: true, - isCompRolling: true, + isCompChanged: true, + isRollingTemplate: true, }, want: want{ w: Workload{ @@ -938,10 +1071,12 @@ func TestRenderComponent(t *testing.T) { args: arg{ ac: revAC, isControlledByApp: true, - isCompRolling: false, + isCompChanged: false, + isRollingTemplate: false, }, want: want{ w: Workload{ + SkipApply: true, ComponentName: componentName, ComponentRevisionName: revisionName, Workload: func() *unstructured.Unstructured { @@ -960,7 +1095,7 @@ func TestRenderComponent(t *testing.T) { r := &components{tc.fields.client, mock.NewMockDiscoveryMapper(), mockParams, tc.fields.workload, tc.fields.trait} got, err := r.renderComponent(ctx, tc.args.ac.Spec.Components[0], tc.args.ac, tc.args.isControlledByApp, - tc.args.isCompRolling, tc.args.dag) + tc.args.isCompChanged, tc.args.isRollingTemplate, tc.args.dag) if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\nr.Render(...): -want error, +got error:\n%s\n", tc.reason, diff) } @@ -975,15 +1110,20 @@ func TestRenderComponent(t *testing.T) { t.Errorf("\n%s\nr.Render(...): -want, +got:\n%s\n", tc.reason, diff) } } - if tc.args.isCompRolling { - wantedSpec, exist, err := unstructured.NestedFieldCopy(tc.want.w.Workload.Object, "spec") - assert.True(t, exist) - assert.True(t, err == nil) - gotSpec, exist, err := unstructured.NestedFieldCopy(got.Workload.Object, "spec") - assert.True(t, exist) - assert.True(t, err == nil) - if diff := cmp.Diff(wantedSpec, gotSpec); diff != "" { - t.Errorf("\n%s\nr.Render(...): -want, +got:\n%s\n", tc.reason, diff) + if tc.args.isCompChanged { + if tc.args.isRollingTemplate { + wantedSpec, exist, err := unstructured.NestedFieldCopy(tc.want.w.Workload.Object, "spec") + assert.True(t, exist) + assert.True(t, err == nil) + gotSpec, exist, err := unstructured.NestedFieldCopy(got.Workload.Object, "spec") + assert.True(t, exist) + assert.True(t, err == nil) + if diff := cmp.Diff(wantedSpec, gotSpec); diff != "" { + t.Errorf("\n%s\nr.Render(...): -want, +got:\n%s\n", tc.reason, diff) + } + } else { + // newly changed workloads are not rendered after the first time + assert.True(t, got.SkipApply) } } else { // we won't touch the spec diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/workloads.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/workloads.go index 61404de13..a7b7a2693 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/workloads.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/workloads.go @@ -54,7 +54,7 @@ func prepWorkloadInstanceForRollout(workload *unstructured.Unstructured) error { // TODO: we can get the workloadDefinition name from workload.GetLabels()["oam.WorkloadTypeLabel"] // and use a special field like "disablePath" in the definition to allow configurable behavior - // we hard code the behavior depends on the known workload group/kind for now. We need to + // we hard code the behavior depends on the known workload group/kind for now. if workload.GroupVersionKind().Group == v1alpha1.GroupVersion.Group { switch workload.GetKind() { case reflect.TypeOf(v1alpha1.CloneSet{}).Name(): diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationdeployment/applicationdeployment_controller.go b/pkg/controller/core.oam.dev/v1alpha2/applicationdeployment/applicationdeployment_controller.go index d256790de..514fd5a99 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationdeployment/applicationdeployment_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationdeployment/applicationdeployment_controller.go @@ -10,6 +10,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" ktypes "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" "k8s.io/klog/v2" "k8s.io/kubectl/pkg/util/slice" ctrl "sigs.k8s.io/controller-runtime" @@ -17,13 +18,16 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" oamv1alpha2 "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" + "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" "github.com/oam-dev/kubevela/pkg/controller/common/rollout" controller "github.com/oam-dev/kubevela/pkg/controller/core.oam.dev" + "github.com/oam-dev/kubevela/pkg/oam" "github.com/oam-dev/kubevela/pkg/oam/discoverymapper" + oamutil "github.com/oam-dev/kubevela/pkg/oam/util" ) const appDeployFinalizer = "finalizers.applicationdeployment.oam.dev" -const reconcileTimeOut = 30 * time.Second +const reconcileTimeOut = 60 * time.Second // Reconciler reconciles an ApplicationDeployment object type Reconciler struct { @@ -71,7 +75,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (res reconcile.Result, retErr e // Get the target application var targetApp oamv1alpha2.ApplicationConfiguration - var sourceApp *oamv1alpha2.ApplicationConfiguration + sourceApp := &oamv1alpha2.ApplicationConfiguration{} targetAppName := appDeploy.Spec.TargetApplicationName if err := r.Get(ctx, ktypes.NamespacedName{Namespace: req.Namespace, Name: targetAppName}, &targetApp); err != nil { @@ -84,6 +88,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (res reconcile.Result, retErr e sourceAppName := appDeploy.Spec.SourceApplicationName if sourceAppName == "" { klog.Info("source app fields not filled, we assume it is deployed for the first time") + sourceApp = nil } else if err := r.Get(ctx, ktypes.NamespacedName{Namespace: req.Namespace, Name: sourceAppName}, sourceApp); err != nil { klog.ErrorS(err, "cannot locate source application", "source application", klog.KRef(req.Namespace, sourceAppName)) @@ -109,8 +114,29 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (res reconcile.Result, retErr e result, rolloutStatus := rolloutPlanController.Reconcile(ctx) // make sure that the new status is copied back appDeploy.Status.RolloutStatus = *rolloutStatus + if rolloutStatus.RollingState == v1alpha1.RolloutSucceedState { + // remove the rollout annotation so that the target appConfig controller can take over the rest of the work + oamutil.RemoveAnnotations(&targetApp, []string{oam.AnnotationAppRollout}) + if err := r.Update(ctx, &targetApp); err != nil { + klog.ErrorS(err, "cannot remove the rollout annotation", "target application", + klog.KRef(req.Namespace, targetAppName)) + return ctrl.Result{}, err + } + } // update the appDeploy status - return result, r.Update(ctx, &appDeploy) + return result, r.updateStatus(ctx, &appDeploy) +} + +// UpdateStatus updates v1alpha2.ApplicationDeployment's Status with retry.RetryOnConflict +func (r *Reconciler) updateStatus(ctx context.Context, app *oamv1alpha2.ApplicationDeployment, opts ...client.UpdateOption) error { + status := app.DeepCopy().Status + return retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { + if err = r.Get(ctx, client.ObjectKey{Namespace: app.Namespace, Name: app.Name}, app); err != nil { + return + } + app.Status = status + return r.Status().Update(ctx, app, opts...) + }) } func (r *Reconciler) handleFinalizer(appDeploy *oamv1alpha2.ApplicationDeployment) { diff --git a/pkg/oam/labels.go b/pkg/oam/labels.go index 3a24aaede..e96ec19b9 100644 --- a/pkg/oam/labels.go +++ b/pkg/oam/labels.go @@ -57,11 +57,6 @@ const ( // the application controller should not reconcile it yet AnnotationAppRollout = "app.oam.dev/rollout-template" - // AnnotationNewAppConfig indicates that the application configuration is new - // this is to enable the applicationConfiguration controller to handle the - // first reconcile logic differently similar to what "finalize" field - AnnotationNewAppConfig = "app.oam.dev/new-appConfig" - // AnnotationRollingComponent indicates that the component is new // this is to enable any concerned controllers to handle the first component apply logic differently // the value of the annotation is a list of revision name of all the new component diff --git a/pkg/oam/util/helper.go b/pkg/oam/util/helper.go index fa35494cf..3750affba 100644 --- a/pkg/oam/util/helper.go +++ b/pkg/oam/util/helper.go @@ -553,19 +553,18 @@ func AddAnnotations(o labelAnnotationObject, annos map[string]string) { o.SetAnnotations(MergeMapOverrideWithDst(o.GetAnnotations(), annos)) } -// MergeMapOverrideWithDst merges two could be nil maps. If any conflicts, override src with dst. +// MergeMapOverrideWithDst merges two could be nil maps. Keep the dst for any conflicts, func MergeMapOverrideWithDst(src, dst map[string]string) map[string]string { if src == nil && dst == nil { return nil } r := make(map[string]string) + for k, v := range src { + r[k] = v + } + // override the src for the same key for k, v := range dst { r[k] = v } - for k, v := range src { - if _, exist := r[k]; !exist { - r[k] = v - } - } return r } diff --git a/pkg/utils/apply/patch.go b/pkg/utils/apply/patch.go index 935484221..74c8c6500 100644 --- a/pkg/utils/apply/patch.go +++ b/pkg/utils/apply/patch.go @@ -120,7 +120,10 @@ func getModifiedConfiguration(obj runtime.Object, updateAnnotation bool) ([]byte if updateAnnotation { annots[oam.AnnotationLastAppliedConfig] = string(modified) - _ = metadataAccessor.SetAnnotations(obj, annots) + err = metadataAccessor.SetAnnotations(obj, annots) + if err != nil { + return nil, err + } modified, err = json.Marshal(obj) if err != nil { return nil, err diff --git a/pkg/webhook/common/rollout/rollout_plan.go b/pkg/webhook/common/rollout/rollout_plan.go index ad0fe0a6f..dd13d310a 100644 --- a/pkg/webhook/common/rollout/rollout_plan.go +++ b/pkg/webhook/common/rollout/rollout_plan.go @@ -33,6 +33,12 @@ func ValidateCreate(rollout *v1alpha1.RolloutPlan, rootPath *field.Path) field.E var allErrs field.ErrorList // TODO: The total number of num in the batches match the current target resource pod size + // the rollout batch partition is either automatic or positive + if rollout.BatchPartition != nil && *rollout.BatchPartition < 0 { + allErrs = append(allErrs, field.Invalid(rootPath.Child("batchPartition"), rollout.BatchPartition, + "the rollout plan has to be positive")) + } + // NumBatches has to be the size of RolloutBatches if rollout.NumBatches != nil && len(rollout.RolloutBatches) != int(*rollout.NumBatches) { allErrs = append(allErrs, field.Invalid(rootPath.Child("numBatches"), rollout.NumBatches,