fix: fix fastFail flag bug. Fixes #10312

[JasonChen86899]

Fixes #10312

Modifications

v1:

1. Modify dag.go: funcassessDAGPhase funcevaluateDependsLogic
2. Modify 'workflow_phase.go': add WorkflowCanceled phase

v2:

1. Modify dag.go: funcassessDAGPhase funcexecuteDAGTask
2. Add func in dag.go: funccheckDAGFastFailed

Verification

1. test fail fast flag and it takes effect

[JasonChen86899]

@agilgur5

[JasonChen86899]

@agilgur5 please review this PR

[terrytangyuan]

I am not sure if we need a new phase for this since it's a one-off use case

[JasonChen86899]

I am not sure if we need a new phase for this since it's a one-off use case

This is indeed a controversial place, this status just is used to record that this workflow has failed fast. If this status is unreasonable, I can also add hasFailedFast field in WorkflowStatus struct. Do you have any suggestions

[jmeridth]

@terrytangyuan I'd love for this to become a permanent phase as requested in #11849. This change would allow for that, as a side effect. If we could leave as an actual phase, in another PR we could update the UI and API to allow filtering of this phase. 🤔

[terrytangyuan]

Introducing a new phase would be a much bigger change with potential side effects since we would need to go through the codebase to make sure we handle the phases correctly.

[jmeridth]

Understood. Just a brief look on my own has shown it is a substantial change. Still might take a stab at it. 😃 will see.

[JasonChen86899]

From my personal understanding of the source code logic, adding such a phase can be seen from two perspectives. If it is only an intermediate state, it will not actually have an impact. If it is the final state, it requires comprehensive code testing.

So what should we do next about this pr? How about add hasFailedFast field in WorkflowStatus struct? I need a field in status to indicate that the current workflow needs to fail fast

[agilgur5]

Yea I did mention it would require a sizeable refactor in #11849 (comment). It should almost certainly be its own change.

RE: failFast, I don't think it would be an appropriate place for a Workflow-level Cancelled status either. failFast could potentially use a Node-level Cancelled status (as opposed to Skipped) as mentioned in #10312 (comment).

So what should we do next about this pr? How about add hasFailedFast field in WorkflowStatus struct? I need a field in status to indicate that the current workflow needs to fail fast

Are you unable to infer the status without such a field? If failFast is true or unset, and a node in a branch of the DAG has failed, then we can infer that the rest of the Workflow should fail fast.

[JasonChen86899]

Are you unable to infer the status without such a field? If failFast is true or unset, and a node in a branch of the DAG has failed, then we can infer that the rest of the Workflow should fail fast.

At this time the logic of DAG is a loop: ExecuteDAG ->AssessDAGPhase ->End/Continue running. A BFS traversal and the result calculation of the target tasks has already been done in AssessDAGPhase. My design is to save the results of the last traversal to a field in the status, so that the next loop will directly obtain the fastFail judgment result, without the need for another iteration and calculation of the target tasks.

I want to know which design you prefer so that I can make further modifications

[JasonChen86899]

@agilgur5 I have modified logic, please review the logic code again

[sarabala1979]

Just I would like to make sure we understand the functionality of flags

When I was using FailFast, I found that this parameter was invalid. Regardless of setting True/False, step C failed, and BDE would execute it.'

FailFast means fail the DAG soon if any of the DAG nodes fail irrespective of the branch. The original issue can be addressed using continueOn if they want to skip failed and continue the rest of the branches.

Here is the purpose/functionality of fastfail and continueon

	// This flag is for DAG logic. The DAG logic has a built-in "fail fast" feature to stop scheduling new steps,
	// as soon as it detects that one of the DAG nodes is failed. Then it waits until all DAG nodes are completed
	// before failing the DAG itself.
	// The FailFast flag default is true,  if set to false, it will allow a DAG to run all branches of the DAG to
	// completion (either success or failure), regardless of the failed outcomes of branches in the DAG.
	// More info and example about this feature at https://github.com/argoproj/argo-workflows/issues/1442
	FailFast *bool `json:"failFast,omitempty" protobuf:"varint,3,opt,name=failFast"`

	// ContinueOn makes argo to proceed with the following task even if this task fails.
	// Errors and Failed states can be specified
	ContinueOn *ContinueOn `json:"continueOn,omitempty" protobuf:"bytes,10,opt,name=continueOn"`

---

failFast: false

if DAG fails before executing BDE, it is a bag and it should be fixed where we are evaluating.

Are HOOKs also not working in failFast scenario?

[JasonChen86899]

Are HOOKs also not working in failFast scenario?

The previous logic was failFast not working but ensure that Hooks would be executed. In this PR modification, I also ensured that Hooks would be executed, but the Hooks of the Skipped node would not be executed. I think this should be logical and all test cases passed.

[agilgur5]

FailFast means fail the DAG soon if any of the DAG nodes fail irrespective of the branch. The original issue can be addressed using continueOn if they want to skip failed and continue the rest of the branches.

@sarabala1979 the issue and comments state that the failFast flag has no effect currently, regardless of what it is set to. They set it as a test, not as a use-case. Users wanted the rest of the branches to fail fast but they don't, despite documentation of the feature; that is a bug

@agilgur5

I was out sick for a bit per my GH status, still catching up. Thanks to other folks reviewing in the interim! Also left a comment in-line above