fix(controller): Fix argo retry with PVCs. Fixes #4275 #4277
Conversation
workflow/controller/operator.go
Outdated
| @@ -388,7 +388,7 @@ func (woc *wfOperationCtx) operate() { | |||
| } | |||
| } | |||
|
|
|||
| err = woc.deletePVCs() | |||
| err = woc.deletePVCs(workflowStatus) | |||
There was a problem hiding this comment.
huh? how come woc.wf.Status.Phase is not suitable?
There was a problem hiding this comment.
At this point woc.wf.Status.Phase hasn't been changed. This happens in the switch block on line 414 onwards.
There was a problem hiding this comment.
I think this block of code should be moved from line 391 to line 443
There was a problem hiding this comment.
I think we would therefore need to change how to deal with the error scenario - we should not mark a workflow as success then changes to error. Probably just log it.
There was a problem hiding this comment.
[fix-argo-retry-with-pvcs 6c93455d] Move woc.deletePVCs() to after workflow phase has been updated
workflow/controller/operator_test.go
Outdated
| retryStrategy: | ||
| limit: 1 | ||
| steps: | ||
| - - arguments: {} |
There was a problem hiding this comment.
any wayto make this easier to understand? it's just see lots and lots of very long YAML and I don't know what is trying to be tested:
- reduce YAML to the minimum that is need to test bug
workflow/controller/operator_test.go
Outdated
| // Node 2 Failed | ||
| assert.Equal(node2.Phase, wfv1.NodeFailed) | ||
| // Hence, PVCs should stick around | ||
| assert.NotZero(len(woc.wf.Status.PersistentVolumeClaims), "PVCs not deleted") |
| // This test ensures that the PVCs used in the steps are not deleted when | ||
| // the workflow fails | ||
| func TestDeletePVCDoesNotDeletePVCOnFailedWorkflow(t *testing.T) { | ||
| assert := assert.New(t) |
workflow/controller/operator_test.go
Outdated
| cancel, controller := newController() | ||
| defer cancel() | ||
|
|
||
| wfcset := controller.wfclientset.ArgoprojV1alpha1().Workflows("") |
There was a problem hiding this comment.
rather than create wfcset, which is overly complicated and long-winded, can you look for other examples that do newController(wf)
workflow/controller/operator_test.go
Outdated
|
|
||
| woc := newWorkflowOperationCtx(wf, controller) | ||
|
|
||
| assert.Equal(len(woc.wf.Status.PersistentVolumeClaims), 1, "1 PVC before operating") |
workflow/controller/operator.go
Outdated
|
|
||
| err = woc.deletePVCs() | ||
| if err != nil { | ||
| woc.log.WithError(err).Error("failed to delete PVCs") |
There was a problem hiding this comment.
I think this should be warning
At the point we attempt to delete PVCs the workflow status hasn't been changed. Let's instead use the NodePhase status that will later be used to determine the new workflow status. Fixes #4275
|
@alexec Rebased onto master and addressed all comments. |
…roj#4277) Signed-off-by: Alex Capras <[email protected]>
Checklist:
"fix(controller): Updates such and such. Fixes #1234".