fix: Ensure task dependencies run after onExit handler is fulfilled #3435
Conversation
workflow/controller/dag_test.go
Outdated
| @@ -1764,6 +1760,12 @@ func TestOnExitNonLeaf(t *testing.T) { | |||
| if assert.NotNil(t, retryNode) { | |||
| assert.Equal(t, wfv1.NodePending, retryNode.Phase) | |||
| } | |||
|
|
|||
| assert.Nil(t, woc.getNodeByName("exit-handler-bug-example.step-3")) | |||
There was a problem hiding this comment.
This modified test tests for the new expected behavior and in particular this line: step-3 should not run until the exit handler has finished
| return false, false, nil | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Note that this is not a regression of 2.9 or the DAG refactor, rather an already existing bug that hasn't been brought up before: in DAGs, the onExit handler of a task runs concurrently with the task's dependent steps, rather than waiting for the onExit handler to finish before running, as is done in steps
simster7
changed the title
| return nil | ||
| } | ||
| return &node | ||
| } |
There was a problem hiding this comment.
This function is just ported from the controller, this is not new code.
There was a problem hiding this comment.
can we call FindNodeByName as it return nil?
There was a problem hiding this comment.
Every instance where this is used there is a nil check done
| return nil | ||
| } | ||
| return &node | ||
| } |
There was a problem hiding this comment.
can we call FindNodeByName as it return nil?
workflow/controller/operator.go
Outdated
| @@ -452,12 +452,7 @@ func (woc *wfOperationCtx) setGlobalParameters(executionParameters wfv1.Argument | |||
| } | |||
|
|
|||
| func (woc *wfOperationCtx) getNodeByName(nodeName string) *wfv1.NodeStatus { | |||
Fixes: #3431
Fixes: #3432
Not ready for review.Tests are not expected to passTests should now passStill need to write new testsAdded tests