Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Calling abort rather than finalize on permanent failure #449

Merged
merged 1 commit into from Jun 6, 2022
Merged

Calling abort rather than finalize on permanent failure #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/controller/nodes/executor.go
View file
Open in desktop
Expand Up @@ -659,7 +659,7 @@ func (c *nodeExecutor) handleNode(ctx context.Context, dag executors.DAGStructur

if currentPhase == v1alpha1.NodePhaseFailing {
logger.Debugf(ctx, "node failing")
if err := c.finalize(ctx, h, nCtx); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we, instead, change finalize implementation in webapi to call delete?

Having both finalize and abort has always bothered me.. I think we only need one (perhaps finalize)... If you look at the implementations in all plugins, the code is pretty much the same... And when it isn't, it's usually a bug waiting to happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can certainly do that. I had two thoughts:
(1) The plugin abstraction, especially since users may write there own, makes it difficult to know if this issue exists in other plugins as well as the webapi async. On a permanent failure we should probably be attempting to abort anyways right? If we make the change in the webapi plugin are we potentially missing out on this fix in other plugins?
(2) I agree the two calls is confusing - especially when they are very similar. Since the existing node-level abort implementation calls both Abort and Finaliz if we introduce a Delete call in the webapi plugin are there any concerns about handling ResourceNotFound errors in webapi plugins? Just want to make sure we are not going to introduce downstream issues in custom plugins. This should already be handled right?

if err := c.abort(ctx, h, nCtx, "node failing"); err != nil {
return executors.NodeStatusUndefined, err
}
nodeStatus.UpdatePhase(v1alpha1.NodePhaseFailed, v1.Now(), nodeStatus.GetMessage(), nodeStatus.GetExecutionError())
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/nodes/executor_test.go
View file
Open in desktop
Expand Up @@ -790,6 +790,7 @@ func TestNodeExecutor_RecursiveNodeHandler_Recurse(t *testing.T) {
} else {
h.OnFinalizeMatch(mock.Anything, mock.Anything).Return(nil)
}
h.OnAbortMatch(mock.Anything, mock.Anything, mock.Anything).Return(nil)
hf.OnGetHandler(v1alpha1.NodeKindTask).Return(h, nil)

mockWf, _, mockNodeStatus := createSingleNodeWf(test.currentNodePhase, 0)
Expand Down