Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(controller): Fix workflows with retryStrategy left Running after completion #5497

Merged
merged 3 commits into from
Mar 23, 2021

Conversation

simster7
Copy link
Member

Fixes: #5481

Signed-off-by: Simon Behar simbeh7@gmail.com

Checklist:

Signed-off-by: Simon Behar <simbeh7@gmail.com>
@@ -1802,7 +1802,7 @@ func (woc *wfOperationCtx) executeTemplate(ctx context.Context, nodeName string,
if retryNodeName != "" {
retryNode := woc.wf.GetNodeByName(retryNodeName)
if !retryNode.Fulfilled() && node.Fulfilled() { // if the retry child has completed we need to update outself
node, err = woc.executeTemplate(ctx, retryNodeName, orgTmpl, tmplCtx, args, opts)
retryNode, err = woc.executeTemplate(ctx, retryNodeName, orgTmpl, tmplCtx, args, opts)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the code is responsible for ensuring the retry node gets marked as completed. In the case of affected workflows in this issue, this retry node is the root node.

The code works correctly, however, it currently doesn't return the updated node (in this change retryNode), but the old node (node). Simply return the updated node so that code downstream can act on the completed root node instead of an old and incomplete reference.

Signed-off-by: Simon Behar <simbeh7@gmail.com>
@simster7 simster7 marked this pull request as ready for review March 23, 2021 14:57
@@ -6314,3 +6314,63 @@ func TestGenerateOutputResultRegex(t *testing.T) {
assert.Equal(t, `steps\.template-name\.outputs\.result`, ref)
assert.Equal(t, `steps\[['\"]template-name['\"]\]\.outputs.result`, expr)
}

const rootRetryStrategyCompletes =`apiVersion: argoproj.io/v1alpha1
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already ensured this is minimal

@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #5497 (365e294) into master (be44ce9) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 365e294 differs from pull request most recent head a91afbc. Consider uploading reports for the commit a91afbc to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5497      +/-   ##
==========================================
+ Coverage   46.66%   46.69%   +0.02%     
==========================================
  Files         240      240              
  Lines       14985    14985              
==========================================
+ Hits         6993     6997       +4     
+ Misses       7093     7089       -4     
  Partials      899      899              
Impacted Files Coverage Δ
workflow/controller/operator.go 70.53% <100.00%> (ø)
cmd/argo/commands/get.go 57.33% <0.00%> (+0.66%) ⬆️
cmd/argoexec/commands/emissary.go 50.00% <0.00%> (+1.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be44ce9...a91afbc. Read the comment docs.

@alexec alexec changed the title fix: Return retryNode after completion update fix(controller): Return retryNode after completion update Mar 23, 2021
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I ask for a clearer PR title - state what you are fixing rather than what you are changing?

Copy link
Member

@sarabala1979 sarabala1979 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Please fix the lint

@simster7
Copy link
Member Author

Can I ask for a clearer PR title - state what you are fixing rather than what you are changing?

Yup, will do so from now on

@simster7 simster7 changed the title fix(controller): Return retryNode after completion update fix(controller): Fix workflows with retryStrategy left Running after completion Mar 23, 2021
Signed-off-by: Simon Behar <simbeh7@gmail.com>
@simster7 simster7 merged commit dfe6ceb into argoproj:master Mar 23, 2021
@simster7 simster7 mentioned this pull request Mar 23, 2021
34 tasks
simster7 added a commit that referenced this pull request Mar 23, 2021
…completion (#5497)

Signed-off-by: Simon Behar <simbeh7@gmail.com>
@simster7 simster7 mentioned this pull request Mar 29, 2021
77 tasks
@simster7 simster7 mentioned this pull request Apr 19, 2021
50 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workflow not marking itself as completed when finished
3 participants