fix: cache configmap don't create with workflow has retrystrategy. Fixes: #12490 #10426 #12491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Would also fix the second part of #10426 |
|
I'm going to have a build and test of this tomorrow, but the code looks like it does the right thing. Thanks @shuangkun. |
Thanks for your review! |
|
This breaks the workflow example from #10426. Without this change the workflow doesn't correctly memoize but does run correctly to completion. With this change you get That's how it comes out in the logs, sorry it's horridly formatted. Could you look into this? |
Thanks， I will test it tommorow！ |
shuangkun
force-pushed
the
fix/MemoizeNotCreatedInRetryStrategy
branch
from
44f0ce3 to
a8cc3d0
Compare
…argoproj#12490 Signed-off-by: shuangkun <tsk2013uestc@163.com>
shuangkun
force-pushed
the
fix/MemoizeNotCreatedInRetryStrategy
branch
from
7248c81 to
e745202
Compare
Thanks for your good find. I test it and find the root cause is when the node hit the memoize, it won't has children even though it is a retry type node. So i think we should change node output from children when it didn't hit memoize. |
shuangkun
force-pushed
the
fix/MemoizeNotCreatedInRetryStrategy
branch
from
e745202 to
6f98655
Compare
Joibel
requested changes
Jan 15, 2024
workflow/controller/exit_handler.go
Outdated
| @@ -15,7 +15,7 @@ import ( | |||
|
|
|||
| func (woc *wfOperationCtx) runOnExitNode(ctx context.Context, exitHook *wfv1.LifecycleHook, parentNode *wfv1.NodeStatus, boundaryID string, tmplCtx *templateresolution.Context, prefix string, scope *wfScope) (bool, *wfv1.NodeStatus, error) { | |||
| outputs := parentNode.Outputs | |||
| if parentNode.Type == wfv1.NodeTypeRetry { | |||
| if parentNode.Type == wfv1.NodeTypeRetry && !(parentNode.MemoizationStatus != nil && parentNode.MemoizationStatus.Hit) { | |||
There was a problem hiding this comment.
This is a complex thing to get your head around (not that it wasn't before your change). I agree it does the right thing, but I'd like to wrap up this if and the subsequent getChildNodeIndex() call into a single helper function.
It feels like it may result in breakage in the future unless we do this.
// Check if we have a retry node which wasn't memoized and return that if we do
func (woc *wfOperationCtx) possiblyGetRetryChildNode(node *wfv1.NodeStatus) *wfv1.NodeStatus {
if node.Type == wfv1.NodeTypeRetry && !(node.MemoizationStatus != nil && node.MemoizationStatus.Hit) {
return getChildNodeIndex(node, woc.wf.Status.Nodes, -1)
}
return nil
}
and then this becomes
if lastChildNode := possiblyGetRetryChildNode(node); lastChildNode != nil {
outputs = lastChildNode.Outputs
}
There was a problem hiding this comment.
Thanks! I agree with you！ I have changed all.
workflow/controller/hooks.go
Outdated
| @@ -75,7 +75,7 @@ func (woc *wfOperationCtx) executeTmplLifeCycleHook(ctx context.Context, scope * | |||
| // executeTemplated should be invoked when hookedNode != nil, because we should reexecute the function to check mutex condition, etc. | |||
| if execute || hookedNode != nil { | |||
| outputs := parentNode.Outputs | |||
| if parentNode.Type == wfv1.NodeTypeRetry { | |||
| if parentNode.Type == wfv1.NodeTypeRetry && !(parentNode.MemoizationStatus != nil && parentNode.MemoizationStatus.Hit) { | |||
workflow/controller/operator.go
Outdated
| @@ -2978,7 +2988,7 @@ func (woc *wfOperationCtx) requeueIfTransientErr(err error, nodeName string) (*w | |||
| func (woc *wfOperationCtx) buildLocalScope(scope *wfScope, prefix string, node *wfv1.NodeStatus) { | |||
| // It may be that the node is a retry node, in which case we want to get the outputs of the last node | |||
| // in the retry group instead of the retry node itself. | |||
| if node.Type == wfv1.NodeTypeRetry { | |||
| if node.Type == wfv1.NodeTypeRetry && !(node.MemoizationStatus != nil && node.MemoizationStatus.Hit) { | |||
shuangkun
force-pushed
the
fix/MemoizeNotCreatedInRetryStrategy
branch
from
6f98655 to
20d5acc
Compare
Signed-off-by: shuangkun <tsk2013uestc@163.com>
shuangkun
force-pushed
the
fix/MemoizeNotCreatedInRetryStrategy
branch
from
20d5acc to
bca2466
Compare
Joibel
approved these changes
Jan 15, 2024
There was a problem hiding this comment.
Nice, thank you.
LGTM, @terrytangyuan could you take a look please.
shuangkun
changed the title
Yes，Is this okay? |
terrytangyuan
approved these changes
Jan 19, 2024
agilgur5
added
area/memoization
area/retry-manual
6 tasks
5 tasks
isubasinghe
pushed a commit
to isubasinghe/argo-workflows
that referenced
this pull request
Feb 20, 2024
isubasinghe
pushed a commit
to isubasinghe/argo-workflows
that referenced
this pull request
Feb 27, 2024
isubasinghe
pushed a commit
to isubasinghe/argo-workflows
that referenced
this pull request
Feb 28, 2024
: argoproj#12490 argoproj#10426 (argoproj#12491) Signed-off-by: Isitha Subasinghe <isubasinghe@student.unimelb.edu.au>
6 tasks
agilgur5
added
area/retryStrategy
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I think MemoizationStatus should like inputs, if the executeTmpl has Memoize, the node should have MemoizationStatus.
I test it and if i Set the MemoizationStatus, the ut pass. If did't，the ut will failed.
Fixes #12490
Fixes #10426
Motivation
Modifications
Verification