-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(controller): Cron re-apply update #3883
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
} | ||
return err | ||
} | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a guard-rail There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might also not be necessary depending on https://github.com/argoproj/argo/pull/3883/files#r480375881 |
||
woc.log.Error("cannot update cron workflow with mismatched resource versions") | ||
return | ||
} | ||
cronWf, err := woc.cronWfIf.Update(woc.cronWf) | ||
if err != nil { | ||
if errors.IsConflict(err) { | ||
reapplyErr := woc.reapplyUpdate() | ||
if reapplyErr != nil { | ||
woc.log.WithError(reapplyErr).WithField("original error", err).Error("failed to update CronWorkflow after reapply attempt") | ||
} | ||
} else { | ||
if !errors.IsConflict(err) { | ||
woc.log.WithError(err).Error("failed to update CronWorkflow") | ||
return | ||
} | ||
var reapplyErr error | ||
cronWf, reapplyErr = woc.reapplyUpdate() | ||
if err != nil { | ||
woc.log.WithError(reapplyErr).WithField("original error", err).Error("failed to update CronWorkflow after reapply attempt") | ||
return | ||
} | ||
} | ||
woc.cronWf = cronWf | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I'm not too sure what the purpose of this is. In theory, once There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In that case wouldn't the guard rail here (#3883 (comment)) always fail? The resource version of the new It seems like we don't intend to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
func (woc *cronWfOperationCtx) reapplyUpdate() error { | ||
func (woc *cronWfOperationCtx) reapplyUpdate() (*v1alpha1.CronWorkflow, error) { | ||
if woc.origCronWf.ResourceVersion != woc.cronWf.ResourceVersion { | ||
return nil, fmt.Errorf("cannot re-apply cron workflow update with mismatched resource versions") | ||
} | ||
orig, err := json.Marshal(woc.origCronWf) | ||
if err != nil { | ||
return err | ||
return nil, err | ||
} | ||
curr, err := json.Marshal(woc.cronWf) | ||
if err != nil { | ||
return err | ||
return nil, err | ||
} | ||
patch, err := jsonpatch.CreateMergePatch(orig, curr) | ||
if err != nil { | ||
return err | ||
return nil, err | ||
} | ||
attempts := 0 | ||
for { | ||
currCronWf, err := woc.cronWfIf.Get(woc.name, v1.GetOptions{}) | ||
if err != nil { | ||
return err | ||
return nil, err | ||
} | ||
currCronWfBytes, err := json.Marshal(currCronWf) | ||
if err != nil { | ||
return err | ||
return nil, err | ||
} | ||
newCronWfBytes, err := jsonpatch.MergePatch(currCronWfBytes, patch) | ||
if err != nil { | ||
return err | ||
return nil, err | ||
} | ||
var newCronWf v1alpha1.CronWorkflow | ||
err = json.Unmarshal(newCronWfBytes, &newCronWf) | ||
if err != nil { | ||
return err | ||
return nil, err | ||
} | ||
_, err = woc.cronWfIf.Update(&newCronWf) | ||
cronWf, err := woc.cronWfIf.Update(&newCronWf) | ||
if err == nil { | ||
return nil | ||
return cronWf, nil | ||
} | ||
attempts++ | ||
if attempts == 5 { | ||
return fmt.Errorf("ran out of retries when trying to reapply update: %s", err) | ||
return nil, fmt.Errorf("ran out of retries when trying to reapply update: %s", err) | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the bug I think, cannot reapplyUpdates twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch