feat(controller): Add failFast flag to DAG and Step templates #5315

simster7 · 2021-03-05T18:13:06Z

pkg/apis/workflow/v1alpha1/workflow_types.go

terrytangyuan

I feel like this should be more generalized instead of focusing on pods and their failures. Perhaps something similar to failure/success conditions for resource templates would be more useful?

alexec

I like how frugal this solution to a popular issue is. We need to get @jessesuen to sign-off on the proto changes.

workflow/controller/operator.go

Signed-off-by: Simon Behar <simbeh7@gmail.com>

simster7 · 2021-03-12T15:44:03Z

Makefile

+GREP_LOGS             := ""
+


Adds a grep string for goreman output

simster7 · 2021-03-12T15:46:53Z

workflow/controller/operator_test.go

@@ -6005,3 +6005,200 @@ func TestHasOutputResultRef(t *testing.T) {
 	assert.True(t, hasOutputResultRef("generate-random", &wf.Spec.Templates[0]))
 	assert.True(t, hasOutputResultRef("generate-random-1", &wf.Spec.Templates[0]))
 }
+
+const stepsFailFast = `


I've already ensured this is minimal :)

This one is particularly longer because it tests nested boundary behavior: failFast should only affect a single depth of boundary and not be affected by children boundaries (such as a retryStrategy)

Signed-off-by: Simon Behar <simbeh7@gmail.com>

alexec · 2021-03-12T16:14:12Z

workflow/controller/node_counters.go

+	}
+}
+
+func (c count) count(key counterType) {


Maybe call this “inc”

alexec · 2021-03-12T16:14:50Z

workflow/controller/node_counters.go

+
+type count map[counterType]int
+
+func (c count) addKeyIfNotPresent(key counterType) {


I’d merge this logic into “count”

alexec · 2021-03-12T16:15:19Z

workflow/controller/node_counters.go

+
+type counter struct {
+	key    counterType
+	ifNode func(wfv1.NodeStatus) bool


Can you comment these fields?

alexec · 2021-03-12T16:16:03Z

workflow/controller/node_counters_test.go

+	}
+}
+
+func TestCounters(t *testing.T) {


I’m assuming you’ve checked your code coverage.

alexec · 2021-03-12T16:16:42Z

workflow/controller/node_counters.go

+	wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
+)
+
+type counter struct {


This refactoring is a good idea

alexec · 2021-03-12T16:17:18Z

workflow/controller/operator.go

@@ -2175,54 +2122,70 @@ func (woc *wfOperationCtx) markNodeWaitingForLock(nodeName string, lockName stri
 }

 // checkParallelism checks if the given template is able to be executed, considering the current active pods and workflow/template parallelism
-func (woc *wfOperationCtx) checkParallelism(tmpl *wfv1.Template, node *wfv1.NodeStatus, boundaryID string) error {
+func (woc *wfOperationCtx) checkParallelism(tmpl *wfv1.Template, node *wfv1.NodeStatus, boundaryID string) (*wfv1.NodeStatus, error) {


I don’t see the new mode being used anywhere?

alexec · 2021-03-12T16:18:06Z

workflow/controller/operator.go

-				activeSiblings := woc.countActiveChildren(boundaryID)
+				counts := woc.countNodes(getActiveChildrenCounter(boundaryID), getFailedOrErroredChildrenCounter(boundaryID))
+				activeSiblings := int64(counts.getCountType(counterTypeActiveChildren))
+				templateFailedOrErroredChildren := int64(counts.getCountType(counterTypeFailedOrErroredChildren))


“FailedOrErrored” is wordy, how about “unsuccessfully”?

These could be expensive (O(N x 3) ?) operations. But only needed when you have FailFast? Maybe guard them this that flag.

alexec · 2021-03-12T16:20:05Z

workflow/controller/operator.go

+				activeSiblings := int64(counts.getCountType(counterTypeActiveChildren))
+				templateFailedOrErroredChildren := int64(counts.getCountType(counterTypeFailedOrErroredChildren))
+
+				if boundaryTemplate.FailFast != nil && *boundaryTemplate.FailFast && templateFailedOrErroredChildren > 0 {


Maybe add add “tmpl.IsFailFast()”?

Signed-off-by: Simon Behar <simbeh7@gmail.com>

simster7 requested a review from jessesuen as a code owner March 5, 2021 18:13

simster7 commented Mar 5, 2021

View reviewed changes

pkg/apis/workflow/v1alpha1/workflow_types.go Outdated Show resolved Hide resolved

simster7 mentioned this pull request Mar 5, 2021

Break sequential loop if an iteration fails #3644

Closed

terrytangyuan reviewed Mar 5, 2021

View reviewed changes

alexec changed the title ~~feat: Allow to specify maxFailed pods under template~~ feat(controller: Allow to specify maxFailed pods under template Mar 8, 2021

alexec changed the title ~~feat(controller: Allow to specify maxFailed pods under template~~ feat(controller): Allow to specify maxFailed pods under template Mar 8, 2021

alexec reviewed Mar 8, 2021

View reviewed changes

workflow/controller/operator.go Outdated Show resolved Hide resolved

alexec assigned jessesuen Mar 8, 2021

simster7 marked this pull request as draft March 10, 2021 15:19

simster7 changed the title ~~feat(controller): Allow to specify maxFailed pods under template~~ feat(controller): Add failFast flag to DAG and Step templates Mar 10, 2021

prog

348bea3

Signed-off-by: Simon Behar <simbeh7@gmail.com>

simster7 force-pushed the max-failed branch from f5be9eb to 348bea3 Compare March 11, 2021 16:33

simster7 added 4 commits March 12, 2021 07:04

done

18801e3

Signed-off-by: Simon Behar <simbeh7@gmail.com>

test

70bb402

Signed-off-by: Simon Behar <simbeh7@gmail.com>

another test

a77d12c

Signed-off-by: Simon Behar <simbeh7@gmail.com>

lint

d6e8886

Signed-off-by: Simon Behar <simbeh7@gmail.com>

simster7 marked this pull request as ready for review March 12, 2021 15:43

simster7 commented Mar 12, 2021

View reviewed changes

codegen

c61be2b

Signed-off-by: Simon Behar <simbeh7@gmail.com>

alexec reviewed Mar 12, 2021

View reviewed changes

simster7 added 2 commits March 12, 2021 11:23

better

e92660c

Signed-off-by: Simon Behar <simbeh7@gmail.com>

prog

7a587fc

Signed-off-by: Simon Behar <simbeh7@gmail.com>

alexec approved these changes Mar 12, 2021

View reviewed changes

simster7 added 4 commits March 12, 2021 11:53

prog

19750b4

Signed-off-by: Simon Behar <simbeh7@gmail.com>

Merge branch 'master' into max-failed

e46a2bd

simplify

bf0bf63

Signed-off-by: Simon Behar <simbeh7@gmail.com>

parenthesis

404e165

Signed-off-by: Simon Behar <simbeh7@gmail.com>

simster7 merged commit 57c05df into argoproj:master Mar 12, 2021

simster7 mentioned this pull request Mar 15, 2021

v3.0.0-rc7 cherry-pick #5409

Closed

27 tasks

simster7 mentioned this pull request Mar 29, 2021

v2.12.11 cherry-pick #5542

Closed

77 tasks

book987 mentioned this pull request Mar 30, 2021

v3.0.0-rc10 cherry-pick #5541

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(controller): Add failFast flag to DAG and Step templates #5315

feat(controller): Add failFast flag to DAG and Step templates #5315

simster7 commented Mar 5, 2021

terrytangyuan left a comment •

edited

Loading

alexec left a comment

simster7 Mar 12, 2021

simster7 Mar 12, 2021

alexec Mar 12, 2021

alexec Mar 12, 2021

alexec Mar 12, 2021

alexec Mar 12, 2021

alexec Mar 12, 2021

alexec Mar 12, 2021

alexec Mar 12, 2021

alexec Mar 12, 2021

alexec Mar 12, 2021

alexec Mar 12, 2021


		type count map[counterType]int

		func (c count) addKeyIfNotPresent(key counterType) {

		GREP_LOGS := ""

feat(controller): Add failFast flag to DAG and Step templates #5315

feat(controller): Add failFast flag to DAG and Step templates #5315

Conversation

simster7 commented Mar 5, 2021

terrytangyuan left a comment • edited Loading

Choose a reason for hiding this comment

alexec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrytangyuan left a comment •

edited

Loading