fix: Fixed hanging http template manual retry with previous outputs. Fixes #11889

[wesleyscholl]

This PR Fixes #11889.

Motivation

Created the initial issue and indicated that I'd be willing to submit a PR to fix this issue.

Changes

As suggested here, Replaced return err with continue to allow TaskSets to be reconciled when manually retrying with previous outputs.

Screenshots

Step Workflow With Exit Hook:

ManualRetryWithPreviousOutputsFix720.mov

Step Workflow Without Exit Hook:

Single Workflow With Exit Hook:

Single Workflow Without Exit Hook:

Multistep Workflow With Exit Hook:

Multistep Workflow Without Exit Hook:

Verification

Tested and verified manual retry does not hang for:

• Http retry step workflow with exit hook.
• Http retry step workflow without exit hook.
• Http retry single workflow with exit hook.
• Http retry single workflow without exit hook.
• Http retry multistep workflow with exit hook.
• Http retry multistep workflow without exit hook.

[terrytangyuan]

cc @toyamagu-2021 Could you help review this?

[wesleyscholl]

Apologies for multiple commits, I'm having GitHub Codespaces gpg key issues.

[toyamagu-2021]

Thank you for submitting PR.
We should delete unnecessary file (likely from codespace?) and improve logging, but this PR fixes #11189.

FYI: root cause is described in original issue

[wesleyscholl]

Thank you for submitting PR. We should delete unnecessary file (likely from codespace?) and improve logging, but this PR fixes #11189.

FYI: root cause is described in original issue

Removed the modifications from files.go, unintentionally created by GitHub Codespaces when running make pre-commit -B.

Output from the CLI:

webpack 5.89.0 compiled with 17 warnings in 82357 ms
Done in 85.07s.
# Pack UI into a Go file
/home/vscode/go/bin/staticfiles -o server/static/files.go ui/dist/app

[agilgur5]

Removed the modifications from files.go, unintentionally created by GitHub Codespaces when running make pre-commit -B.

Huh we do have an exact version specified in the Makefile, odd that it produced different output.
staticfiles is also deprecated in favor of native embed functionality in later versions of Go (#11654), so I also don't think it's had updates either. Strange 🤔

[agilgur5]

we should definitely be more specific here -- if you read this message in the Controller's logs, it would be pretty ambiguous.

Suggested change

	woc.log.Info("Continuing to retry")
	woc.log.Debugf("Unable to obtain node for %s during TaskSet Reconciliation. Expected during a retry, continuing", nodeId)

I also think debug level makes more sense here, since this is now an expected error

[wesleyscholl]

After further testing, I discovered that the outputs from the failed pods were saved and not cleared.

As mentioned here,TaskSets needs to be deleted before retrying. - #11889 (comment)

woc.patchTaskSet(ctx, woc.getDeleteTaskAndNodePatch(), types.MergePatchType)

Adding this line at the beginning of reconcileTaskSet deletes TaskSets and clears previous outputs from failed pods. Then the workflow retries as expected on manual retry.

ArgoWorkflowsManualRetryClearedOutputsFixed.mov

[wesleyscholl]

This issue has been fixed in >= v3.5.5.

ArgoMultiStepFailRetrySucccess720.mov

[agilgur5]

Superseded by #12620 based on this issue comment. Thanks all!