fix: correct retry logic #13734
fix: correct retry logic #13734
Conversation
agilgur5
added
the
area/retry-manual
|
This might supersede #12105 |
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
isubasinghe
force-pushed
the
fix-retry-logic
branch
from
abe5b8f to
57e7638
Compare
Signed-off-by: isubasinghe <[email protected]>
workflow/util/util_test.go
Outdated
| func TestNestedDAG(t *testing.T) { | ||
| require := require.New(t) | ||
| wf := wfv1.MustUnmarshalWorkflow(nestedDAG) | ||
|
|
||
| newWf, podsToDelete, err := FormulateRetryWorkflow(context.Background(), wf, true, "id=dag-nested-zxlc2-744943701", []string{}) | ||
| require.NoError(err) | ||
| _ = newWf | ||
| _ = podsToDelete | ||
|
|
||
| } |
There was a problem hiding this comment.
These should actually test for individual node status as well.
| ` | ||
|
|
||
| func TestStepsRetryWorkflow(t *testing.T) { | ||
| assert := assert.New(t) |
There was a problem hiding this comment.
Why do you do this for assert and require here and elsewhere?
There was a problem hiding this comment.
Ah I was waiting for an opinion on #13734 (comment). I plan to use these when I fill in the tests.
I was unsure if it was worth the effort of these tests. I wrote these to test edge conditions and make sure they worked as expected. They will prevent regressions though, so I will add them.
| @@ -1026,51 +1025,6 @@ func TestRetryExitHandler(t *testing.T) { | |||
| func TestFormulateRetryWorkflow(t *testing.T) { | |||
| ctx := context.Background() | |||
| wfClient := argofake.NewSimpleClientset().ArgoprojV1alpha1().Workflows("my-ns") | |||
| createdTime := metav1.Time{Time: time.Now().Add(-1 * time.Second).UTC()} | |||
| finishedTime := metav1.Time{Time: createdTime.Add(time.Second * 2)} | |||
| t.Run("Steps", func(t *testing.T) { | |||
There was a problem hiding this comment.
Dropping this entire test feels wrong, why can't it be checked still?
There was a problem hiding this comment.
It's not really testable easily, we cannot establish a dag for these status fields. The root node is not present.
This will error out complaining that the root node couldn't be found.
workflow/util/util_test.go
Outdated
| func TestNestedDAG(t *testing.T) { | ||
| require := require.New(t) | ||
| wf := wfv1.MustUnmarshalWorkflow(nestedDAG) | ||
|
|
||
| newWf, podsToDelete, err := FormulateRetryWorkflow(context.Background(), wf, true, "id=dag-nested-zxlc2-744943701", []string{}) | ||
| require.NoError(err) | ||
| _ = newWf | ||
| _ = podsToDelete | ||
|
|
||
| } |
workflow/util/util.go
Outdated
| } | ||
|
|
||
| func isGroupNode(node wfv1.NodeStatus) bool { | ||
| return node.Type == wfv1.NodeTypeDAG || node.Type == wfv1.NodeTypeTaskGroup || node.Type == wfv1.NodeTypeStepGroup || node.Type == wfv1.NodeTypeSteps | ||
| func consumeTill(n *node, should tillFn, resetFunc resetFn) (*node, error) { |
There was a problem hiding this comment.
nit: Rename to resetUntil
reset instead of consume, as you call the resetFunc. Or call that consumeFunc
till has more meanings, until is much clearer if English isn't your first language.
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
workflow/util/util.go
Outdated
| default: | ||
| // Do not allow retry of workflows with pods in Running/Pending phase | ||
| return nil, nil, errors.InternalErrorf("Workflow cannot be retried with node %s in %s phase", node.Name, node.Phase) | ||
| deleteNodes, err := getNodeIDsToResetNoChildren(restartSuccessful, nodeFieldSelector, wf.Status.Nodes) |
There was a problem hiding this comment.
Would I have to set the --restart-successful in order to identify the nodes that need to be deleted, even if I only wish to retry a specific failed node?
There was a problem hiding this comment.
No this wouldn't be the case
workflow/util/util.go
Outdated
| addToDelete(curr.n.ID, true) | ||
| children := getChildren(curr) | ||
| for childID := range children { | ||
| addToDelete(childID, true) |
There was a problem hiding this comment.
Ok, I follow and agree now. Thanks.
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
Fixes #12543 and other retry related issues.
Motivation
The current retry logic is too simplistic, it relies only on resetting nodes by traversing boundary nodes.
This approach does not work, it needs to be a combination of manual traversal and boundary node traversal.
In addition to this the current logic does not take care of various edge cases, for example container sets.
It was clear some rethinking needed to be done such that:
a) edge cases were taken care of.
b) retry logic was simpler to understand and hopefully fix if any further bug were to arise.
Modifications
This is a complete rewrite of the
FormulateRetryWorkflowfunction.Verification
Extensive testing performed manually to verify that behaviour is as expected.