feat(controller): Support any() and all() for TaskGroup in depends logic. Closes #3405 #3964

markterm · 2020-09-08T16:22:56Z

If a DAG task depends on a task that uses withItems, then can depend on any or all of the children being in a status.

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed the CLA.
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

markterm · 2020-09-08T16:27:24Z

FYI @simster7

simster7 · 2020-09-08T16:27:59Z

Thanks, will take a look at this over the coming days

markterm · 2020-09-21T13:18:31Z

Just a little nudge @simster7 ;)

simster7 · 2020-09-23T13:57:26Z

Hi @mark9white, thanks for the nudge and sorry for the delay. I'll take a look today

simster7 · 2020-09-23T21:03:42Z

Hey @mark9white. Thanks for submitting this change and sorry for the delay. We agreed to support this feature, but we want to consider a different syntax.

Having any(task.Succeeded), doesn't make much sense as: (1) there is nothing that distinguishes whether task actually expanded or not, (2) overloads the meaning of task.Succeeded: if it's inside any(...) then it refers to the expanded tasks, while if it's not it refers to the parent task, and (3) frankly it doesn't make much sense.

We want to experiment a bit with possible syntaxes. Ideas that came around include:

Having an "expander" such as task.ExpandedTasks resolve to a list of individual tasks statuses. We can then leverage expr to evaluate on them (such as "Succeeded" in task.ExpandedTasks)
Having dedicated boolean attributes (such as task.AnySucceeded, task.AllFailed)
Others?

Thoughts?

markterm · 2020-09-24T12:53:09Z

Thanks for taking a look. I think dedicated boolean attributes as you suggest, eg task.AnySucceeded, would be easiest for the users to understand and use. I could change the PR implementation to that?

simster7 · 2020-09-24T20:25:41Z

Sure sounds good. I'm thinking that we only need to have task.AnySucceeded since task.AllFailed can be inferred from it:

task.AllFailed = task.Failed && !task.AnySucceeded

What do you think? Are there any more than we might need?

markterm · 2020-09-25T10:44:56Z

I think if we want to minimise the surface area then task.AnySucceeded would be enough for most requirements, and I'd be fine with just implementing that.

However if we want to provide full flexibility then I would add all the permutations. For example, it's not exactly possible to calculated all failed as you suggest - eg the case where one was skipped, one failed and none succeeded. And having a direct AllFailed makes it easier to read Argo workflows than combinations of other conditions.

simster7 · 2020-09-25T17:16:58Z

Sure let's start with AnySucceeded and AllFailed. Please add validation such that if a user tries to use these variables in a task which does not expand, they will get an error

alexec · 2020-10-08T16:22:49Z

@mark9white let me know if you need any help with this.

markterm · 2020-10-08T17:27:35Z

@alexec Thanks, I should be able to get to it in a couple of weeks ..

alexec

Changes requested

markterm · 2020-11-05T12:02:54Z

@simster7 @alexec FYI I have made the requested changes.

alexec · 2020-11-05T17:20:36Z

Makefile

@@ -47,7 +47,7 @@ CONTROLLER_IMAGE_FILE  := dist/controller-image.marker

 # perform static compilation
 STATIC_BUILD          ?= true
-STATIC_FILES          ?= true
+STATIC_FILES          ?= false


revert this

simster7

Gave this a very quick first pass

simster7 · 2020-11-06T13:55:03Z

workflow/common/ancestry.go

+type DependencyType int
+
+const (
+	TaskDependency  DependencyType = 0


Suggested change

TaskDependency DependencyType = 0

DependencyTypeTaskDependency DependencyType = iota

See: https://yourbasic.org/golang/iota/

Only needed for the first item

simster7 · 2020-11-06T13:56:02Z

workflow/common/ancestry.go

 	for _, matchGroup := range matches {
 		// We have matched a taskName.TaskResult
 		if matchGroup[2] != -1 {
 			match := depends[matchGroup[2]:matchGroup[3]]
 			split := strings.Split(match, ".")
-			dependencies[split[0]] = true
+			if split[1] == "AnySucceeded" || split[1] == "AllFailed" {


Why use raw strings instead of our const variables?

simster7 · 2020-11-06T14:00:22Z

workflow/controller/dag.go

@@ -44,7 +44,7 @@ type dagContext struct {

 	// dependencies is a list of all the tasks a specific task depends on. Because dependencies are computed using regex
 	// and regex is expensive, we cache the results so that they are only computed once per operation
-	dependencies map[string][]string
+	dependencies map[string]map[string]common.DependencyType


I'm almost surely wrong, but I didn't see a place where we actually use the value of the inner map (i.e. common.DependencyType). Can you explain why it's necessary, point me where it's used?

The issue is that dagValidationContext needs the inner map, and was getting passed in as a dagContext, so I modified dagContext to keep them matching. This wasn't ideal, I've fixed it up.

markterm · 2020-11-10T09:48:40Z

I don't think the build failure is related to my change.

markterm · 2020-11-23T10:46:29Z

@alexec @simster7 Am checking next steps here?

alexec · 2020-12-01T00:22:14Z

@simster7 do you have anymore feedback on this please?

simster7

Sorry for the delay. This LGTM, I just want @jessesuen to approve of the syntax. I will bring this up with him tomorrow

simster7 · 2020-12-01T01:31:41Z

@markterm Can you merger/rebase this with master please?

… depends logic. Closes argoproj#3405 Signed-off-by: Mark White <mark@markwhite.com>

markterm · 2020-12-01T12:11:34Z

@simster7 done

Changes made

simster7 · 2020-12-03T12:01:37Z

@markterm Could you check this box?

This way we can merge this branch with master on our own to merge it faster

markterm · 2020-12-03T12:16:26Z

I don't have that option, so I've given access to the repo which will hopefully have the same effect. I've also triggered a branch update with master.

markterm changed the title ~~3405 any all depends logic~~ feat(controller): Support any() and all() for TaskGroup in depends logic Sep 8, 2020

markterm changed the title ~~feat(controller): Support any() and all() for TaskGroup in depends logic~~ feat(controller): Support any() and all() for TaskGroup in depends logic. Closes #3405 Sep 8, 2020

simster7 self-assigned this Sep 8, 2020

alexec self-assigned this Oct 8, 2020

alexec unassigned simster7 and alexec Oct 19, 2020

alexec previously requested changes Oct 19, 2020

View reviewed changes

markterm force-pushed the 3405-any-all-depends-logic branch 2 times, most recently from f89abcd to 5ae3db2 Compare October 30, 2020 18:03

markterm force-pushed the 3405-any-all-depends-logic branch from 5ae3db2 to ac6a722 Compare November 5, 2020 12:04

alexec reviewed Nov 5, 2020

View reviewed changes

markterm force-pushed the 3405-any-all-depends-logic branch from ac6a722 to ba7a33c Compare November 5, 2020 17:24

simster7 self-assigned this Nov 5, 2020

simster7 suggested changes Nov 6, 2020

View reviewed changes

markterm force-pushed the 3405-any-all-depends-logic branch 3 times, most recently from be79772 to c6fc802 Compare November 9, 2020 17:54

markterm force-pushed the 3405-any-all-depends-logic branch 2 times, most recently from 5ebd1bb to 4bd2e0e Compare November 10, 2020 10:09

simster7 approved these changes Dec 1, 2020

View reviewed changes

feat(controller): Support .AnySucceeded / .AllFailed for TaskGroup in…

a9517af

… depends logic. Closes argoproj#3405 Signed-off-by: Mark White <mark@markwhite.com>

markterm force-pushed the 3405-any-all-depends-logic branch from 454d38d to a9517af Compare December 1, 2020 11:51

Merge branch 'master' into 3405-any-all-depends-logic

aaa9838

Merge branch 'master' into 3405-any-all-depends-logic

74a73e8

simster7 merged commit 1212df4 into argoproj:master Dec 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(controller): Support any() and all() for TaskGroup in depends logic. Closes #3405 #3964

feat(controller): Support any() and all() for TaskGroup in depends logic. Closes #3405 #3964

markterm commented Sep 8, 2020 •

edited

markterm commented Sep 8, 2020

simster7 commented Sep 8, 2020

markterm commented Sep 21, 2020

simster7 commented Sep 23, 2020

simster7 commented Sep 23, 2020

markterm commented Sep 24, 2020

simster7 commented Sep 24, 2020

markterm commented Sep 25, 2020

simster7 commented Sep 25, 2020

alexec commented Oct 8, 2020

markterm commented Oct 8, 2020

alexec left a comment

markterm commented Nov 5, 2020

alexec Nov 5, 2020

markterm Nov 5, 2020

simster7 left a comment

simster7 Nov 6, 2020

markterm Nov 9, 2020

simster7 Nov 6, 2020

simster7 Nov 6, 2020

markterm Nov 9, 2020

markterm commented Nov 10, 2020

markterm commented Nov 23, 2020

alexec commented Dec 1, 2020

simster7 left a comment

simster7 commented Dec 1, 2020

markterm commented Dec 1, 2020

simster7 commented Dec 3, 2020

markterm commented Dec 3, 2020

	TaskDependency DependencyType = 0
	DependencyTypeTaskDependency DependencyType = iota

feat(controller): Support any() and all() for TaskGroup in depends logic. Closes #3405 #3964

feat(controller): Support any() and all() for TaskGroup in depends logic. Closes #3405 #3964

Conversation

markterm commented Sep 8, 2020 • edited

markterm commented Sep 8, 2020

simster7 commented Sep 8, 2020

markterm commented Sep 21, 2020

simster7 commented Sep 23, 2020

simster7 commented Sep 23, 2020

markterm commented Sep 24, 2020

simster7 commented Sep 24, 2020

markterm commented Sep 25, 2020

simster7 commented Sep 25, 2020

alexec commented Oct 8, 2020

markterm commented Oct 8, 2020

alexec left a comment

Choose a reason for hiding this comment

markterm commented Nov 5, 2020

alexec Nov 5, 2020

Choose a reason for hiding this comment

markterm Nov 5, 2020

Choose a reason for hiding this comment

simster7 left a comment

Choose a reason for hiding this comment

simster7 Nov 6, 2020

Choose a reason for hiding this comment

markterm Nov 9, 2020

Choose a reason for hiding this comment

simster7 Nov 6, 2020

Choose a reason for hiding this comment

simster7 Nov 6, 2020

Choose a reason for hiding this comment

markterm Nov 9, 2020

Choose a reason for hiding this comment

markterm commented Nov 10, 2020

markterm commented Nov 23, 2020

alexec commented Dec 1, 2020

simster7 left a comment

Choose a reason for hiding this comment

simster7 commented Dec 1, 2020

markterm commented Dec 1, 2020

simster7 commented Dec 3, 2020

markterm commented Dec 3, 2020

markterm commented Sep 8, 2020 •

edited