From bef24bdf22dbe74974d3112a70712e30c0fb934e Mon Sep 17 00:00:00 2001 From: Tianxin Dong Date: Tue, 8 Mar 2022 16:04:19 +0800 Subject: [PATCH] Fix: separate waiting and failed backoff time and make them configurable (#3362) * Fix: separate waiting and failed backoff time and make them configurable Signed-off-by: FogDong * change var name in chart values Signed-off-by: FogDong * fix ci Signed-off-by: FogDong --- .github/workflows/go.yml | 5 +++++ Makefile | 2 +- charts/vela-core/README.md | 9 ++++++++ .../templates/kubevela-controller.yaml | 3 +++ charts/vela-core/values.yaml | 14 ++++++++++++ charts/vela-minimal/README.md | 9 ++++++++ .../templates/kubevela-controller.yaml | 3 +++ charts/vela-minimal/values.yaml | 15 +++++++++++++ cmd/core/main.go | 8 +++++-- design/vela-core/workflow_policy.md | 8 +++++-- .../application_controller_test.go | 6 ++--- pkg/workflow/tasks/custom/task.go | 9 +++++--- pkg/workflow/tasks/custom/task_test.go | 2 +- pkg/workflow/workflow.go | 22 ++++++++++++++----- pkg/workflow/workflow_test.go | 4 ++-- 15 files changed, 100 insertions(+), 19 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index ed908aec5..e1d5d4a3c 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -108,6 +108,11 @@ jobs: with: go-version: ${{ env.GO_VERSION }} + - name: Setup node + uses: actions/setup-node@v2 + with: + node-version: '14' + - name: Cache Go Dependencies uses: actions/cache@v2 with: diff --git a/Makefile b/Makefile index 325a9ec99..5e36a3b28 100644 --- a/Makefile +++ b/Makefile @@ -48,7 +48,7 @@ staticcheck: staticchecktool lint: golangci $(GOLANGCILINT) run ./... -reviewable: manifests fmt vet lint staticcheck +reviewable: manifests fmt vet lint staticcheck helm-doc-gen go mod tidy # Execute auto-gen code commands and ensure branch is clean. diff --git a/charts/vela-core/README.md b/charts/vela-core/README.md index 0060f760c..5a6199bc2 100644 --- a/charts/vela-core/README.md +++ b/charts/vela-core/README.md @@ -51,6 +51,15 @@ helm install --create-namespace -n vela-system kubevela kubevela/vela-core --wai | `dependCheckWait` | dependCheckWait is the time to wait for ApplicationConfiguration's dependent-resource ready | `30s` | +### KubeVela workflow parameters + +| Name | Description | Value | +| -------------------------------------- | ------------------------------------------------------ | ----- | +| `workflow.backoff.maxTime.waitState` | The max backoff time of workflow in a wait condition | `60` | +| `workflow.backoff.maxTime.failedState` | The max backoff time of workflow in a failed condition | `300` | +| `workflow.step.errorRetryTimes` | The max retry times of a failed workflow step | `10` | + + ### KubeVela controller parameters | Name | Description | Value | diff --git a/charts/vela-core/templates/kubevela-controller.yaml b/charts/vela-core/templates/kubevela-controller.yaml index 05aaea0c8..e18e01685 100644 --- a/charts/vela-core/templates/kubevela-controller.yaml +++ b/charts/vela-core/templates/kubevela-controller.yaml @@ -136,6 +136,9 @@ spec: - "--concurrent-reconciles={{ .Values.concurrentReconciles }}" - "--kube-api-qps={{ .Values.kubeClient.qps }}" - "--kube-api-burst={{ .Values.kubeClient.brust }}" + - "--max-workflow-wait-backoff-time={{ .Values.workflow.backoff.maxTime.waitState }}" + - "--max-workflow-failed-backoff-time={{ .Values.workflow.backoff.maxTime.failedState }}" + - "--max-workflow-step-error-retry-times={{ .Values.workflow.step.errorRetryTimes }}" image: {{ .Values.imageRegistry }}{{ .Values.image.repository }}:{{ .Values.image.tag }} imagePullPolicy: {{ quote .Values.image.pullPolicy }} resources: diff --git a/charts/vela-core/values.yaml b/charts/vela-core/values.yaml index e382283fa..6c56b4683 100644 --- a/charts/vela-core/values.yaml +++ b/charts/vela-core/values.yaml @@ -33,6 +33,20 @@ enableFluxcdAddon: false dependCheckWait: 30s +## @section KubeVela workflow parameters + +## @param workflow.backoff.maxTime.waitState The max backoff time of workflow in a wait condition +## @param workflow.backoff.maxTime.failedState The max backoff time of workflow in a failed condition +## @param workflow.step.errorRetryTimes The max retry times of a failed workflow step +workflow: + backoff: + maxTime: + waitState: 60 + failedState: 300 + step: + errorRetryTimes: 10 + + ## @section KubeVela controller parameters ## @param replicaCount KubeVela controller replica count diff --git a/charts/vela-minimal/README.md b/charts/vela-minimal/README.md index 91a7290e6..ba660194c 100644 --- a/charts/vela-minimal/README.md +++ b/charts/vela-minimal/README.md @@ -70,6 +70,15 @@ helm install --create-namespace -n vela-system kubevela kubevela/vela-minimal -- | `dependCheckWait` | dependCheckWait is the time to wait for ApplicationConfiguration's dependent-resource ready | `30s` | +### KubeVela workflow parameters + +| Name | Description | Value | +| -------------------------------------- | ------------------------------------------------------ | ----- | +| `workflow.backoff.maxTime.waitState` | The max backoff time of workflow in a wait condition | `60` | +| `workflow.backoff.maxTime.failedState` | The max backoff time of workflow in a failed condition | `300` | +| `workflow.step.errorRetryTimes` | The max retry times of a failed workflow step | `10` | + + ### KubeVela controller parameters | Name | Description | Value | diff --git a/charts/vela-minimal/templates/kubevela-controller.yaml b/charts/vela-minimal/templates/kubevela-controller.yaml index 0dab3a9fd..5970c6cee 100644 --- a/charts/vela-minimal/templates/kubevela-controller.yaml +++ b/charts/vela-minimal/templates/kubevela-controller.yaml @@ -139,6 +139,9 @@ spec: - "--concurrent-reconciles={{ .Values.concurrentReconciles }}" - "--kube-api-qps={{ .Values.kubeClient.qps }}" - "--kube-api-burst={{ .Values.kubeClient.brust }}" + - "--max-workflow-wait-backoff-time={{ .Values.workflow.backoff.maxTime.waitState }}" + - "--max-workflow-failed-backoff-time={{ .Values.workflow.backoff.maxTime.failedState }}" + - "--max-workflow-step-error-retry-times={{ .Values.workflow.step.errorRetryTimes }}" image: {{ .Values.imageRegistry }}{{ .Values.image.repository }}:{{ .Values.image.tag }} imagePullPolicy: {{ quote .Values.image.pullPolicy }} resources: diff --git a/charts/vela-minimal/values.yaml b/charts/vela-minimal/values.yaml index c5faf517b..5c8a714e7 100644 --- a/charts/vela-minimal/values.yaml +++ b/charts/vela-minimal/values.yaml @@ -35,6 +35,21 @@ enableFluxcdAddon: false ## @param dependCheckWait dependCheckWait is the time to wait for ApplicationConfiguration's dependent-resource ready dependCheckWait: 30s + +## @section KubeVela workflow parameters + +## @param workflow.backoff.maxTime.waitState The max backoff time of workflow in a wait condition +## @param workflow.backoff.maxTime.failedState The max backoff time of workflow in a failed condition +## @param workflow.step.errorRetryTimes The max retry times of a failed workflow step +workflow: + backoff: + maxTime: + waitState: 60 + failedState: 300 + step: + errorRetryTimes: 10 + + ## @section KubeVela controller parameters ## @param replicaCount KubeVela controller replica count diff --git a/cmd/core/main.go b/cmd/core/main.go index d9b945ca5..2c22375af 100644 --- a/cmd/core/main.go +++ b/cmd/core/main.go @@ -30,8 +30,6 @@ import ( "strings" "time" - "github.com/oam-dev/kubevela/pkg/utils/util" - "k8s.io/klog/v2" "k8s.io/klog/v2/klogr" ctrl "sigs.k8s.io/controller-runtime" @@ -51,7 +49,10 @@ import ( "github.com/oam-dev/kubevela/pkg/resourcekeeper" "github.com/oam-dev/kubevela/pkg/utils/common" "github.com/oam-dev/kubevela/pkg/utils/system" + "github.com/oam-dev/kubevela/pkg/utils/util" oamwebhook "github.com/oam-dev/kubevela/pkg/webhook/core.oam.dev" + "github.com/oam-dev/kubevela/pkg/workflow" + "github.com/oam-dev/kubevela/pkg/workflow/tasks/custom" "github.com/oam-dev/kubevela/version" ) @@ -137,6 +138,9 @@ func main() { standardcontroller.AddOptimizeFlags() standardcontroller.AddAdmissionFlags() flag.IntVar(&resourcekeeper.MaxDispatchConcurrent, "max-dispatch-concurrent", 10, "Set the max dispatch concurrent number, default is 10") + flag.IntVar(&workflow.MaxWorkflowWaitBackoffTime, "max-workflow-wait-backoff-time", 60, "Set the max workflow wait backoff time, default is 60") + flag.IntVar(&workflow.MaxWorkflowFailedBackoffTime, "max-workflow-failed-backoff-time", 300, "Set the max workflow wait backoff time, default is 300") + flag.IntVar(&custom.MaxWorkflowStepErrorRetryTimes, "max-workflow-step-error-retry-times", 10, "Set the max workflow step error retry times, default is 10") flag.Parse() // setup logging diff --git a/design/vela-core/workflow_policy.md b/design/vela-core/workflow_policy.md index 36fc906fa..83dc799ee 100644 --- a/design/vela-core/workflow_policy.md +++ b/design/vela-core/workflow_policy.md @@ -142,7 +142,7 @@ If the status of workflow step is `waiting` or `failed`, the workflow will be re int(0.05 * 2^(n-1)) ``` -Based on the above formula, we will take `1s` and `600s` as our min and max time. +Based on the above formula, we will take `1s` min time. For example, if the workflow is `waiting`, the first ten reconciliation will be like: @@ -160,13 +160,17 @@ For example, if the workflow is `waiting`, the first ten reconciliation will be | 10 | 512 | 25.6 | 25 | | ... | ... | ... | ... | +If the workflow step is `waiting`, the max backoff time is `60s`, you can change it by setting `MaxWorkflowWaitBackoffTime`. + +If the workflow step is `failed`, the max backoff time is `300s`, you can change it by setting `MaxWorkflowFailedBackoffTime`. + #### Failed Workflow Steps If the workflow step is `failed`, it means that there may be some error in the workflow step, like some cue errors. > Note that if the workflow step is unhealthy, the workflow step will be marked as `wait` but not `failed` and it will wait for healthy. -For this case, we will retry the workflow step 10 times, and if the workflow step is still `failed`, we will suspend this workflow, and it's message will be `The workflow suspends automatically because the failed times of steps have reached the limit(10 times)`. +For this case, we will retry the workflow step 10 times by default, and if the workflow step is still `failed`, we will suspend this workflow, and it's message will be `The workflow suspends automatically because the failed times of steps have reached the limit`. You can change the retry times by setting `MaxWorkflowStepErrorRetryTimes`. After the workflow is suspended, we can change the workflow step to make it work, and then use `vela workflow resume ` to resume it. 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 8db7f1efc..f98760b87 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 @@ -1690,7 +1690,7 @@ var _ = Describe("Test Application Controller", func() { Expect(checkApp.Status.Workflow.Message).Should(BeEquivalentTo(workflow.MessageInitializingWorkflow)) By("verify the first twenty reconciles") - for i := 0; i < custom.MaxErrorTimes; i++ { + for i := 0; i < custom.MaxWorkflowStepErrorRetryTimes; i++ { testutil.ReconcileOnce(reconciler, reconcile.Request{NamespacedName: appKey}) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(checkApp.Status.Phase).Should(BeEquivalentTo(common.ApplicationRunningWorkflow)) @@ -1735,7 +1735,7 @@ var _ = Describe("Test Application Controller", func() { Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(checkApp.Status.Phase).Should(BeEquivalentTo(common.ApplicationRunningWorkflow)) - for i := 0; i < custom.MaxErrorTimes+1; i++ { + for i := 0; i < custom.MaxWorkflowStepErrorRetryTimes+1; i++ { testutil.ReconcileOnce(reconciler, reconcile.Request{NamespacedName: appKey}) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(checkApp.Status.Phase).Should(BeEquivalentTo(common.ApplicationRunningWorkflow)) @@ -1803,7 +1803,7 @@ var _ = Describe("Test Application Controller", func() { Expect(checkApp.Status.Workflow.Message).Should(BeEquivalentTo(workflow.MessageInitializingWorkflow)) By("verify the first twenty reconciles") - for i := 0; i < custom.MaxErrorTimes; i++ { + for i := 0; i < custom.MaxWorkflowStepErrorRetryTimes; i++ { testutil.ReconcileOnce(reconciler, reconcile.Request{NamespacedName: appKey}) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(checkApp.Status.Phase).Should(BeEquivalentTo(common.ApplicationRunningWorkflow)) diff --git a/pkg/workflow/tasks/custom/task.go b/pkg/workflow/tasks/custom/task.go index bc029959f..87cf23e91 100644 --- a/pkg/workflow/tasks/custom/task.go +++ b/pkg/workflow/tasks/custom/task.go @@ -39,6 +39,11 @@ import ( wfTypes "github.com/oam-dev/kubevela/pkg/workflow/types" ) +var ( + // MaxWorkflowStepErrorRetryTimes is the max retry times of the failed workflow step. + MaxWorkflowStepErrorRetryTimes = 10 +) + const ( // StatusReasonWait is the reason of the workflow progress condition which is Wait. StatusReasonWait = "Wait" @@ -54,8 +59,6 @@ const ( StatusReasonParameter = "ProcessParameter" // StatusReasonOutput is the reason of the workflow progress condition which is Output. StatusReasonOutput = "Output" - // MaxErrorTimes is the max times of the workflow progress condition which is Failed. - MaxErrorTimes = 10 ) // LoadTaskTemplate gets the workflowStep definition from cluster and resolve it. @@ -289,7 +292,7 @@ func (exec *executor) err(ctx wfContext.Context, err error, reason string) { func (exec *executor) checkErrorTimes(ctx wfContext.Context) { times := ctx.IncreaseCountValueInMemory(wfTypes.ContextPrefixFailedTimes, exec.wfStatus.ID) - if times >= MaxErrorTimes { + if times >= MaxWorkflowStepErrorRetryTimes { exec.wait = false exec.failedAfterRetries = true } diff --git a/pkg/workflow/tasks/custom/task_test.go b/pkg/workflow/tasks/custom/task_test.go index 5deadf3c7..d22d8b12b 100644 --- a/pkg/workflow/tasks/custom/task_test.go +++ b/pkg/workflow/tasks/custom/task_test.go @@ -258,7 +258,7 @@ close({ case "failed-after-retries": wfContext.CleanupMemoryStore("app-v1", "default") newCtx := newWorkflowContextForTest(t) - for i := 0; i < MaxErrorTimes; i++ { + for i := 0; i < MaxWorkflowStepErrorRetryTimes; i++ { status, operation, err = run.Run(newCtx, &types.TaskRunOptions{}) r.NoError(err) r.Equal(operation.Waiting, true) diff --git a/pkg/workflow/workflow.go b/pkg/workflow/workflow.go index 9f337dd27..9d64b1726 100644 --- a/pkg/workflow/workflow.go +++ b/pkg/workflow/workflow.go @@ -46,18 +46,20 @@ var ( DisableRecorder = false // StepStatusCache cache the step status StepStatusCache sync.Map + // MaxWorkflowWaitBackoffTime is the max time to wait before reconcile wait workflow again + MaxWorkflowWaitBackoffTime = 60 + // MaxWorkflowFailedBackoffTime is the max time to wait before reconcile failed workflow again + MaxWorkflowFailedBackoffTime = 300 ) const ( // minWorkflowBackoffWaitTime is the min time to wait before reconcile workflow again minWorkflowBackoffWaitTime = 1 - // maxWorkflowBackoffWaitTime is the max time to wait before reconcile workflow again - maxWorkflowBackoffWaitTime = 600 // backoffTimeCoefficient is the coefficient of time to wait before reconcile workflow again backoffTimeCoefficient = 0.05 // MessageFailedAfterRetries is the message of failed after retries - MessageFailedAfterRetries = "The workflow suspends automatically because the failed times of steps have reached the limit(10 times)" + MessageFailedAfterRetries = "The workflow suspends automatically because the failed times of steps have reached the limit" // MessageInitializingWorkflow is the message of initializing workflow MessageInitializingWorkflow = "Initializing workflow" ) @@ -301,14 +303,24 @@ func (e *engine) getBackoffWaitTime() int { return minWorkflowBackoffWaitTime } - interval := math.Pow(2, float64(minTimes)) * backoffTimeCoefficient + interval := int(math.Pow(2, float64(minTimes)) * backoffTimeCoefficient) if interval < minWorkflowBackoffWaitTime { return minWorkflowBackoffWaitTime } + maxWorkflowBackoffWaitTime := e.getMaxBackoffWaitTime() if interval > maxWorkflowBackoffWaitTime { return maxWorkflowBackoffWaitTime } - return int(interval) + return interval +} + +func (e *engine) getMaxBackoffWaitTime() int { + for _, step := range e.status.Steps { + if step.Phase == common.WorkflowStepPhaseFailed { + return MaxWorkflowFailedBackoffTime + } + } + return MaxWorkflowWaitBackoffTime } func (e *engine) setNextExecuteTime() { diff --git a/pkg/workflow/workflow_test.go b/pkg/workflow/workflow_test.go index 810a7f41c..e430e37c8 100644 --- a/pkg/workflow/workflow_test.go +++ b/pkg/workflow/workflow_test.go @@ -268,7 +268,7 @@ var _ = Describe("Test Workflow", func() { Expect(interval).Should(BeEquivalentTo(minWorkflowBackoffWaitTime)) } - for i := 0; i < 9; i++ { + for i := 0; i < 6; i++ { _, err = wf.ExecuteSteps(ctx, revision, runners) Expect(err).ToNot(HaveOccurred()) interval := e.getBackoffWaitTime() @@ -278,7 +278,7 @@ var _ = Describe("Test Workflow", func() { _, err = wf.ExecuteSteps(ctx, revision, runners) Expect(err).ToNot(HaveOccurred()) interval = e.getBackoffWaitTime() - Expect(interval).Should(BeEquivalentTo(maxWorkflowBackoffWaitTime)) + Expect(interval).Should(BeEquivalentTo(MaxWorkflowWaitBackoffTime)) By("Test get backoff time after clean") wfContext.CleanupMemoryStore(app.Name, app.Namespace)