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 <dongtianxin.tx@alibaba-inc.com>

* change var name in chart values

Signed-off-by: FogDong <dongtianxin.tx@alibaba-inc.com>

* fix ci

Signed-off-by: FogDong <dongtianxin.tx@alibaba-inc.com>

.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:

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.

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              |

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:

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

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              |

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:

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

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

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 <workflow-name>` to resume it.
 

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))

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
 	}

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)

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() {

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)