fix(controller): Cron re-apply update #3883
Conversation
| @@ -98,7 +98,6 @@ func (woc *cronWfOperationCtx) validateCronWorkflow() error { | |||
| woc.reportCronWorkflowError(v1alpha1.ConditionTypeSpecError, fmt.Sprint(err)) | |||
| } else { | |||
| woc.cronWf.Status.Conditions.RemoveCondition(v1alpha1.ConditionTypeSpecError) | |||
| woc.persistUpdate() | |||
There was a problem hiding this comment.
this is the bug I think, cannot reapplyUpdates twice
| @@ -117,58 +116,68 @@ func getWorkflowObjectReference(wf *v1alpha1.Workflow, runWf *v1alpha1.Workflow) | |||
| } | |||
|
|
|||
| func (woc *cronWfOperationCtx) persistUpdate() { | |||
| _, err := woc.cronWfIf.Update(woc.cronWf) | |||
| if woc.origCronWf.ResourceVersion != woc.cronWf.ResourceVersion { | |||
There was a problem hiding this comment.
this is a guard-rail
There was a problem hiding this comment.
This might also not be necessary depending on https://github.com/argoproj/argo/pull/3883/files#r480375881
|
Flaky: |
| @@ -98,7 +98,6 @@ func (woc *cronWfOperationCtx) validateCronWorkflow() error { | |||
| woc.reportCronWorkflowError(v1alpha1.ConditionTypeSpecError, fmt.Sprint(err)) | |||
| } else { | |||
| woc.cronWf.Status.Conditions.RemoveCondition(v1alpha1.ConditionTypeSpecError) | |||
| woc.persistUpdate() | |||
| } | ||
| } | ||
| woc.cronWf = cronWf |
There was a problem hiding this comment.
Actually, I'm not too sure what the purpose of this is. In theory, once persistUpdates is called, it should be the end of the cron operation context and nothing else should access woc.cronWf. Could you explain a bit why this line is necessary?
There was a problem hiding this comment.
I've included this so that subsequent calls to this func will have the updated version, and as a result cannot accidentally reapplyUpdate twice.
There was a problem hiding this comment.
I've included this so that subsequent calls to this func will have the updated version, and as a result cannot accidentally reapplyUpdate twice.
In that case wouldn't the guard rail here (#3883 (comment)) always fail? The resource version of the new woc.cronWf will always be different than the old.
It seems like we don't intend to call persistUpdates more than once at all. Should we instead ensure that it doesn't happen altogether?
There was a problem hiding this comment.
My goals to make sure we don't regress by putting in place checks. I would want the guard condition to fail - as it indicates a programming error.
Checklist:
"fix(controller): Updates such and such. Fixes #1234".