fix: correct retry logic #13734

isubasinghe · 2024-10-10T10:40:22Z

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 FormulateRetryWorkflow function.

Verification

Extensive testing performed manually to verify that behaviour is as expected.

Joibel · 2024-10-17T11:01:47Z

This might supersede #12105

Signed-off-by: isubasinghe <[email protected]>

isubasinghe · 2024-10-21T07:13:28Z

workflow/util/util_test.go

+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
+
+}


These should actually test for individual node status as well.

workflow/util/util_test.go

Joibel · 2024-10-21T12:17:07Z

workflow/util/util_test.go

+`
+
+func TestStepsRetryWorkflow(t *testing.T) {
+	assert := assert.New(t)


Why do you do this for assert and require here and elsewhere?

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.

Joibel · 2024-10-21T12:21:03Z

workflow/util/util_test.go

@@ -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) {


Dropping this entire test feels wrong, why can't it be checked still?

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.

Joibel · 2024-10-21T12:30:50Z

workflow/util/util_test.go

+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
+
+}


Joibel · 2024-10-21T12:31:38Z

workflow/util/util.go

 }

-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) {


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.

workflow/util/util.go

Signed-off-by: isubasinghe <[email protected]>

gzbvas · 2024-11-01T07:57:00Z

workflow/util/util.go

-		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)


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?

No this wouldn't be the case

Joibel

LGTM.

workflow/util/util.go

Joibel · 2024-11-05T10:23:55Z

workflow/util/util.go

+	addToDelete(curr.n.ID, true)
+	children := getChildren(curr)
+	for childID := range children {
+		addToDelete(childID, true)


Ok, I follow and agree now. Thanks.

Signed-off-by: isubasinghe <[email protected]>

shuangkun · 2025-02-20T06:29:10Z

Could this PR be cherry-picked into the 3.6.5 branch? Some customers have inquired about it, but I haven't found any prior discussions related to the 3.6.x patches. Thanks @Joibel

Joibel · 2025-03-06T10:40:57Z

I've created a patch releases discussion #14259.

I haven't backported this yet, it feels like #14124 should be addressed before this is backported otherwise we're knowingly introducing a regression.

Joibel · 2025-04-23T10:08:17Z

/cherry-pick release-3.6

Signed-off-by: isubasinghe <[email protected]> (cherry picked from commit 497f338)

Signed-off-by: isubasinghe <[email protected]> (cherry picked from commit 497f338) Signed-off-by: Alan Clucas <[email protected]>

Signed-off-by: isubasinghe <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: Isitha Subasinghe <[email protected]>

Signed-off-by: isubasinghe <[email protected]> (cherry picked from commit 497f338) Signed-off-by: Alan Clucas <[email protected]>

…abilities fixes (Cr 28355) (#358) * fix: bump deps for k8schain to fix ecr-login (argoproj#14008) (release-3.6 cherry-pick) (argoproj#14174) * fix(ci): python sdk release process (release-3.6) (argoproj#14183) Signed-off-by: Alan Clucas <[email protected]> * docs: clarify qps/burst on controller (cherry-pick argoproj#14190) (argoproj#14192) Signed-off-by: Tim Collins <[email protected]> Co-authored-by: Tim Collins <[email protected]> * fix(api/jsonschema): use unchanging JSON Schema version (cherry-pick argoproj#14092) (argoproj#14256) Signed-off-by: Roger Peppe <[email protected]> Co-authored-by: Roger Peppe <[email protected]> * fix(api/jsonschema): use working `$id` (cherry-pick argoproj#14257) (argoproj#14258) Signed-off-by: Roger Peppe <[email protected]> Co-authored-by: Roger Peppe <[email protected]> * docs: autogenerate tested k8s versions and centralize config (argoproj#14176) (release-3.6) (argoproj#14262) Signed-off-by: Mason Malone <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: Mason Malone <[email protected]> * chore(deps): bump minio-go to newer version (argoproj#14185) (release-3.6) (argoproj#14261) Co-authored-by: Vaibhav Kaushik <[email protected]> * fix: split pod controller from workflow controller (argoproj#14129) (release-3.6) (argoproj#14263) * chore(deps): fix snyk (argoproj#14264) (release-3.6) (argoproj#14268) * chore: revert to correct k8s versions Accidental bump from argoproj#14176 cherry-pick Signed-off-by: Alan Clucas <[email protected]> * chore(deps): bump github.com/go-jose/go-jose/v3 from 3.0.3 to 3.0.4 in the go_modules group (cherry-pick argoproj#14231) (argoproj#14269) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix: wait for workflow informer to sync before pod informer (cherry-pick argoproj#14248) (argoproj#14266) Signed-off-by: Rohan K <[email protected]> Co-authored-by: Rohan K <[email protected]> * fix(cli): remove red from log colour selection. Fixes argoproj#6740 (cherry-pick argoproj#14215) (argoproj#14278) Signed-off-by: Prabakaran Kumaresshan <[email protected]> Co-authored-by: Prabakaran Kumaresshan <[email protected]> * fix: correct semaphore configmap keys for multiple semaphores (argoproj#14184) (release-3.6) (argoproj#14281) * fix: don't print help for non-validation errors. Fixes argoproj#14234 (cherry-pick argoproj#14249) (argoproj#14283) Signed-off-by: Koichi Shimada <[email protected]> Signed-off-by: Mason Malone <[email protected]> Co-authored-by: koichi <[email protected]> Co-authored-by: Mason Malone <[email protected]> * docs: fix kubernetes versions (release-3.6) (argoproj#14273) Signed-off-by: Alan Clucas <[email protected]> * fix(workflow/sync): use RWMutex to prevent concurrent map access (cherry-pick argoproj#14321) (argoproj#14322) Signed-off-by: Ryan Currah <[email protected]> Co-authored-by: Ryan Currah <[email protected]> * chore(lint): update golangci-lint to 2.1.1 (argoproj#14390) (cherry-pick release-3.6) (argoproj#14417) * chore: bump golang 1.23->1.24 (argoproj#14385) (cherry-pick release-3.6) (argoproj#14418) * fix: gracefully handle invalid CronWorkflows and simplify logic. (cherry-pick argoproj#14197) (argoproj#14419) Signed-off-by: Mason Malone <[email protected]> * fix: prevent dfs sorter infinite recursion on cycle. Fixes argoproj#13395 (cherry-pick argoproj#14391) (argoproj#14420) Signed-off-by: Adrien Delannoy <[email protected]> Co-authored-by: Adrien Delannoy <[email protected]> * chore(deps): bump github.com/expr-lang/expr from 1.16.9 to 1.17.0 (argoproj#14307) (release-3.6) (argoproj#14421) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps)!: update k8s and argo-events (release-3.6) (argoproj#14424) Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: william.vanhevelingen <[email protected]> Signed-off-by: Mason Malone <[email protected]> Signed-off-by: William Van Hevelingen <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: William Van Hevelingen <[email protected]> Co-authored-by: Mason Malone <[email protected]> * fix: correct retry logic (argoproj#13734) (release-3.6) (argoproj#14428) Signed-off-by: isubasinghe <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: Isitha Subasinghe <[email protected]> * fix: manual retries exit handler cleanup. Fixes argoproj#14180 (argoproj#14181) (release-3.6) (argoproj#14429) Signed-off-by: isubasinghe <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: Isitha Subasinghe <[email protected]> * fix: correct manual retry logic. Fixes argoproj#14124 (argoproj#14328) (release-3.6) (argoproj#14430) Signed-off-by: oninowang <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: jswxstw <[email protected]> * fix: disable ALPN in argo-server as a workaround (argoproj#14433) Signed-off-by: Alan Clucas <[email protected]> * result of codegen Signed-off-by: Kim <[email protected]> * fix:lint Signed-off-by: Kim <[email protected]> --------- Signed-off-by: Alan Clucas <[email protected]> Signed-off-by: Tim Collins <[email protected]> Signed-off-by: Roger Peppe <[email protected]> Signed-off-by: Mason Malone <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Rohan K <[email protected]> Signed-off-by: Prabakaran Kumaresshan <[email protected]> Signed-off-by: Koichi Shimada <[email protected]> Signed-off-by: Ryan Currah <[email protected]> Signed-off-by: Adrien Delannoy <[email protected]> Signed-off-by: william.vanhevelingen <[email protected]> Signed-off-by: William Van Hevelingen <[email protected]> Signed-off-by: isubasinghe <[email protected]> Signed-off-by: oninowang <[email protected]> Signed-off-by: Kim <[email protected]> Co-authored-by: Alan Clucas <[email protected]> Co-authored-by: gcp-cherry-pick-bot[bot] <98988430+gcp-cherry-pick-bot[bot]@users.noreply.github.com> Co-authored-by: Tim Collins <[email protected]> Co-authored-by: Roger Peppe <[email protected]> Co-authored-by: Mason Malone <[email protected]> Co-authored-by: Vaibhav Kaushik <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Rohan K <[email protected]> Co-authored-by: Prabakaran Kumaresshan <[email protected]> Co-authored-by: koichi <[email protected]> Co-authored-by: Ryan Currah <[email protected]> Co-authored-by: Adrien Delannoy <[email protected]> Co-authored-by: William Van Hevelingen <[email protected]> Co-authored-by: Isitha Subasinghe <[email protected]> Co-authored-by: jswxstw <[email protected]>

isubasinghe mentioned this pull request Oct 10, 2024

Retrying specific failed node does not work #12543

Closed

agilgur5 added the area/retry-manual Manual workflow "Retry" Action (API/CLI/UI). See retryStrategy for template-level retries label Oct 10, 2024

isubasinghe added 9 commits October 21, 2024 14:48

fix: correct retry logic

5c80de3

Signed-off-by: isubasinghe <[email protected]>

fix: corrected test data

02bb19e

Signed-off-by: isubasinghe <[email protected]>

fix: make test pass

b34da14

Signed-off-by: isubasinghe <[email protected]>

fix: linting

f35e9d5

Signed-off-by: isubasinghe <[email protected]>

fix: linting

32627eb

Signed-off-by: isubasinghe <[email protected]>

fix: ran linter

ca24c43

Signed-off-by: isubasinghe <[email protected]>

fix: delete onExit pod

b32e8ee

Signed-off-by: isubasinghe <[email protected]>

fix: delete onExit pod

c809da6

Signed-off-by: isubasinghe <[email protected]>

fix: consume boundaries

57e7638

Signed-off-by: isubasinghe <[email protected]>

isubasinghe force-pushed the fix-retry-logic branch from abe5b8f to 57e7638 Compare October 21, 2024 03:48

fix: skip if child succeeded

de7516e

Signed-off-by: isubasinghe <[email protected]>

isubasinghe marked this pull request as ready for review October 21, 2024 07:12

isubasinghe commented Oct 21, 2024

View reviewed changes

Joibel requested changes Oct 21, 2024

View reviewed changes

isubasinghe added 3 commits October 28, 2024 15:23

fix: address feedback, find boundaries when stepgroup or taskgroup

ab68186

Signed-off-by: isubasinghe <[email protected]>

fix: completed test files

9ed648e

Signed-off-by: isubasinghe <[email protected]>

fix: address all feedback

074de52

Signed-off-by: isubasinghe <[email protected]>

gzbvas reviewed Nov 1, 2024

View reviewed changes

Joibel approved these changes Nov 5, 2024

View reviewed changes

isubasinghe merged commit 497f338 into argoproj:main Nov 21, 2024
28 checks passed

Joibel deleted the fix-retry-logic branch November 29, 2024 10:17

Joibel pushed a commit to Joibel/argo-workflows that referenced this pull request Dec 5, 2024

fix: correct retry logic (argoproj#13734)

2287d48

Signed-off-by: isubasinghe <[email protected]>

Joibel pushed a commit to Joibel/argo-workflows that referenced this pull request Dec 6, 2024

fix: correct retry logic (argoproj#13734)

6f5ee4e

Signed-off-by: isubasinghe <[email protected]>

Joibel pushed a commit to Joibel/argo-workflows that referenced this pull request Dec 6, 2024

fix: correct retry logic (argoproj#13734)

3147ea5

Signed-off-by: isubasinghe <[email protected]>

jswxstw mentioned this pull request Jan 26, 2025

HTTP and Plugin nodes cannot be manually retried #14124

Closed

4 tasks

Joibel mentioned this pull request Mar 6, 2025

Release v3.6 patch releases discussion #14259

Open

Joibel pushed a commit that referenced this pull request Apr 28, 2025

fix: correct retry logic (#13734)

9af4833

Signed-off-by: isubasinghe <[email protected]> (cherry picked from commit 497f338)

Joibel mentioned this pull request Apr 28, 2025

fix: correct retry logic (#13734) (release-3.6) #14428

Merged

Joibel pushed a commit that referenced this pull request Apr 28, 2025

fix: correct retry logic (#13734)

54ca725

Signed-off-by: isubasinghe <[email protected]> (cherry picked from commit 497f338) Signed-off-by: Alan Clucas <[email protected]>

Joibel pushed a commit that referenced this pull request Apr 28, 2025

fix: correct retry logic (#13734)

65fe116

Signed-off-by: isubasinghe <[email protected]> (cherry picked from commit 497f338) Signed-off-by: Alan Clucas <[email protected]>

Joibel pushed a commit that referenced this pull request Apr 28, 2025

fix: correct retry logic (#13734)

bdef66f

Signed-off-by: isubasinghe <[email protected]> (cherry picked from commit 497f338) Signed-off-by: Alan Clucas <[email protected]>

Joibel added a commit that referenced this pull request Apr 28, 2025

fix: correct retry logic (#13734) (release-3.6) (#14428)

3993b12

Signed-off-by: isubasinghe <[email protected]> Signed-off-by: Alan Clucas <[email protected]> Co-authored-by: Isitha Subasinghe <[email protected]>

Joibel pushed a commit that referenced this pull request Apr 28, 2025

fix: correct retry logic (#13734)

96132ac

Signed-off-by: isubasinghe <[email protected]> (cherry picked from commit 497f338) Signed-off-by: Alan Clucas <[email protected]>

fix: correct retry logic #13734

fix: correct retry logic #13734

Uh oh!

Conversation

isubasinghe commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verification

Uh oh!

Joibel commented Oct 17, 2024 • edited by agilgur5 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joibel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shuangkun commented Feb 20, 2025

Uh oh!

Joibel commented Mar 6, 2025

Uh oh!

Joibel commented Apr 23, 2025

Uh oh!

Uh oh!

isubasinghe commented Oct 10, 2024 •

edited

Loading

Joibel commented Oct 17, 2024 •

edited by agilgur5

Loading