Calling abort rather than finalize on permanent failure #449
Conversation
Signed-off-by: Daniel Rammer <[email protected]>
| @@ -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 { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
Signed-off-by: Daniel Rammer <[email protected]>
TL;DR
Currently FlytePropeller calls "finalize" rather than "abort" on permanent failures. Consequently, during instances where a resource is actively running we fail to abort it. For example when the number of timeouts exceeds the maximum number of retries we automatically transition from a retryable failure to a permanent failure.
Type
Are all requirements met?
Complete description
I do not believe there are any unintended side-effects by calling abort. If a node has not yet been executed (ex. dynamic, subworkflow, etc) the abort should still be successful. If this is not the case, we will need to revisit this logic to ensure we fail correctly.
Tracking Issue
fixes flyteorg/flyte#2298
Follow-up issue
NA