Add abort functionality #224
Conversation
Codecov Report
@@ Coverage Diff @@
## master #224 +/- ##
=========================================
Coverage ? 85.14%
=========================================
Files ? 49
Lines ? 4376
Branches ? 0
=========================================
Hits ? 3726
Misses ? 460
Partials ? 190
Continue to review full report at Codecov.
|
rollout/canary.go
Outdated
| @@ -64,7 +64,12 @@ func (c *RolloutController) rolloutCanary(rollout *v1alpha1.Rollout, rsList []*a | |||
| } | |||
|
|
|||
| logCtx.Info("Reconciling AnalysisRun step") | |||
| if err = c.reconcileAnalysisRuns(roCtx); err != nil { | |||
| addPause, err := c.reconcileAnalysisRuns(roCtx) | |||
| if addPause { | |||
There was a problem hiding this comment.
Rather than surfacing a bool here, could we call
err := c.reconcileAnalysisRuns(roCtx)
if roCtx.PauseContext().HasAddPause() {
logCtx.Info("Detected pause due to inconclusive AnalysisRun")
return c.syncRolloutStatusCanary(roCtx)
}
if err != nil {
return err
}
rollout/analysis.go
Outdated
| @@ -41,44 +41,49 @@ func (c *RolloutController) getAnalysisRunsForRollout(rollout *v1alpha1.Rollout) | |||
| return ownedByRollout, nil | |||
| } | |||
|
|
|||
| func (c *RolloutController) reconcileAnalysisRuns(roCtx *canaryContext) error { | |||
| func (c *RolloutController) reconcileAnalysisRuns(roCtx *canaryContext) (bool, error) { | |||
There was a problem hiding this comment.
See my other comment, I think instead of making sure we are returning true/false correctly in this function, I think the caller can check for roCtx.PauseContext().HasAddPause() after calling this function. With my suggested approach, it means we don't have to ensure we correctly return true/false everywhere, because there is a single source of truth.
There was a problem hiding this comment.
Yeah I like that a lot more
| if currBackgroundAr != nil { | ||
| if currBackgroundAr.Status == nil || !currBackgroundAr.Status.Status.Completed() || analysisutil.IsTerminating(currBackgroundAr) { | ||
| if currBackgroundAr != nil && !r.Status.Abort { | ||
| if currBackgroundAr.Status == nil || !currBackgroundAr.Status.Status.Completed() { |
There was a problem hiding this comment.
I'm not sure why I made AnalysisRun.Status a bool pointer. Seems like it just makes the code annoying to deal with.
There was a problem hiding this comment.
Yeah, it adds a lot of nil checks.
|
The clearing of abort is implicit because I do not set the newStatus.Abort to the old status value. As a result, I have a couple locations that add the abort: argo-rollouts/rollout/canary.go Line 304 in 6bba90c argo-rollouts/rollout/canary.go Line 317 in 6bba90c argo-rollouts/rollout/canary.go Line 312 in 6bba90c |
|
I'm thinking about adding abort to the pauseContext and setting the newstatus using the CalculatePauseStatus method. |
Future work: