From 300070c5699bcf028c179120e75d7acae332be78 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Wed, 16 Oct 2019 14:45:42 -0700 Subject: [PATCH 01/16] Introduce pause context --- rollout/analysis.go | 4 +++ rollout/bluegreen.go | 49 +++++++++++++++++----------- rollout/bluegreen_test.go | 2 +- rollout/canary.go | 23 +++++++------ rollout/context.go | 28 +++++++++++++--- rollout/controller.go | 4 +-- rollout/pause.go | 58 ++++++++++++++------------------- rollout/service.go | 13 +++++--- rollout/sync.go | 68 ++++++++++++++++++++++++++++----------- 9 files changed, 157 insertions(+), 92 deletions(-) diff --git a/rollout/analysis.go b/rollout/analysis.go index 7151f4249c..ad63c497e2 100644 --- a/rollout/analysis.go +++ b/rollout/analysis.go @@ -147,6 +147,10 @@ func (c *RolloutController) reconcileStepBasedAnalysisRun(roCtx *canaryContext) return currentAr, err } + if currentAr.Status != nil && currentAr.Status.Status == v1alpha1.AnalysisStatusInconclusive { + roCtx.PauseContext().AddPause() + } + return currentAr, nil } diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index 37b142212d..baf9b0c4fd 100644 --- a/rollout/bluegreen.go +++ b/rollout/bluegreen.go @@ -32,8 +32,9 @@ func (c *RolloutController) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*apps logCtx := roCtx.Log() allRSs := roCtx.AllRSs() if reconcileBlueGreenTemplateChange(roCtx) { + roCtx.PauseContext().RemovePause() logCtx.Infof("New pod template or template change detected") - return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx, false) + return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } // Scale up, if we can. @@ -61,44 +62,44 @@ func (c *RolloutController) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*apps if scaledUp { logCtx.Infof("Not finished reconciling new ReplicaSet '%s'", newRS.Name) - return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx, false) + return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } if scaledDown { logCtx.Info("Not finished reconciling old replica sets") - return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx, false) + return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } if switchPreviewSvc { logCtx.Infof("Not finished reconciling preview service' %s'", previewSvc.Name) - return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx, false) + return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } if !replicasetutil.ReadyForPause(r, newRS, allRSs) { logutil.WithRollout(r).Infof("New RS '%s' is not fully saturated", newRS.Name) - return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx, false) + return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } noFastRollback := true if newRS != nil { - _, ok := newRS.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey] - noFastRollback = !ok - if !noFastRollback { - logCtx.Infof("Detected '%s' annotation and will skip pause", newRS.Name) + _, hasScaleDownDeadlineAnnotationKey := newRS.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey] + if hasScaleDownDeadlineAnnotationKey { + logCtx.Infof("Detected scale down annotation for ReplicaSet '%s' and will skip pause", newRS.Name) } + noFastRollback = !hasScaleDownDeadlineAnnotationKey } - if !defaults.GetAutoPromotionEnabledOrDefault(r) && noFastRollback { + if noFastRollback { logCtx.Info("Reconciling pause") pauseBeforeSwitchActive := c.reconcileBlueGreenPause(activeSvc, previewSvc, roCtx) if pauseBeforeSwitchActive { logCtx.Info("Not finished reconciling pause") - return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx, true) + return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } } logCtx.Infof("Reconciling active service '%s'", activeSvc.Name) if !annotations.IsSaturated(r, newRS) { logCtx.Infof("New RS '%s' is not fully saturated", newRS.Name) - return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx, false) + return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } switchActiveSvc, err := c.reconcileActiveService(roCtx, previewSvc, activeSvc) if err != nil { @@ -106,7 +107,7 @@ func (c *RolloutController) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*apps } if switchActiveSvc { logCtx.Infof("Not finished reconciling active service '%s'", activeSvc.Name) - return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx, false) + return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } if _, ok := newRS.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey]; ok { @@ -118,7 +119,7 @@ func (c *RolloutController) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*apps } } - return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx, false) + return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } // reconcileBlueGreenTemplateChange returns true if we detect there was a change in the pod template @@ -134,6 +135,12 @@ func reconcileBlueGreenTemplateChange(roCtx *blueGreenContext) bool { func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev1.Service, roCtx *blueGreenContext) bool { rollout := roCtx.Rollout() + + if defaults.GetAutoPromotionEnabledOrDefault(rollout) { + roCtx.PauseContext().RemovePause() + return false + } + newRS := roCtx.NewRS() newRSPodHash := newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] @@ -142,6 +149,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev } // If the rollout is not paused and the active service is not point at the newRS, we should pause the rollout. if !rollout.Spec.Paused && rollout.Status.PauseStartTime == nil && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { + roCtx.PauseContext().AddPause() return true } @@ -149,6 +157,12 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev autoPromoteActiveServiceDelaySeconds := rollout.Spec.Strategy.BlueGreen.AutoPromotionSeconds if autoPromoteActiveServiceDelaySeconds != nil && pauseStartTime != nil { c.checkEnqueueRolloutDuringWait(rollout, *pauseStartTime, *autoPromoteActiveServiceDelaySeconds) + switchDeadline := pauseStartTime.Add(time.Duration(*autoPromoteActiveServiceDelaySeconds) * time.Second) + now := metav1.Now() + if now.After(switchDeadline) { + roCtx.PauseContext().RemovePause() + } + } return rollout.Spec.Paused && pauseStartTime != nil @@ -200,7 +214,7 @@ func (c *RolloutController) scaleDownOldReplicaSetsForBlueGreen(oldRSs []*appsv1 return totalScaledDown, nil } -func (c *RolloutController) syncRolloutStatusBlueGreen(previewSvc *corev1.Service, activeSvc *corev1.Service, roCtx *blueGreenContext, addPause bool) error { +func (c *RolloutController) syncRolloutStatusBlueGreen(previewSvc *corev1.Service, activeSvc *corev1.Service, roCtx *blueGreenContext) error { r := roCtx.Rollout() newRS := roCtx.NewRS() oldRSs := roCtx.OlderRSs() @@ -236,11 +250,10 @@ func (c *RolloutController) syncRolloutStatusBlueGreen(previewSvc *corev1.Servic newStatus.Selector = metav1.FormatLabelSelector(r.Spec.Selector) } - pauseStartTime, paused := calculatePauseStatus(roCtx, addPause) - newStatus.PauseStartTime = pauseStartTime newStatus.BlueGreen.ScaleUpPreviewCheckPoint = calculateScaleUpPreviewCheckPoint(roCtx, activeRS) + newStatus = c.calculateRolloutConditions(roCtx, newStatus) - return c.persistRolloutStatus(r, &newStatus, &paused) + return c.persistRolloutStatus(roCtx, &newStatus) } func calculateScaleUpPreviewCheckPoint(roCtx *blueGreenContext, activeRS *appsv1.ReplicaSet) bool { diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 5a81011651..af7a9729ef 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -661,7 +661,7 @@ func TestBlueGreenRolloutStatusHPAStatusFieldsNoActiveSelector(t *testing.T) { c, _, _ := f.newController(noResyncPeriodFunc) - err := c.syncRolloutStatusBlueGreen(nil, activeSvc, roCtx, false) + err := c.syncRolloutStatusBlueGreen(nil, activeSvc, roCtx) assert.Nil(t, err) assert.Len(t, f.client.Actions(), 1) result := f.client.Actions()[0].(core.PatchAction).GetPatch() diff --git a/rollout/canary.go b/rollout/canary.go index 79b5d9231f..f6187def05 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -254,8 +254,9 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error c.recorder.Event(r, corev1.EventTypeNormal, "SkipSteps", msg) } } + roCtx.PauseContext().RemovePause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) - return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false)) + return c.persistRolloutStatus(roCtx, &newStatus) } if r.Status.Canary.StableRS == "" { @@ -269,8 +270,9 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error newStatus.CurrentStepIndex = &stepCount } + roCtx.PauseContext().RemovePause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) - return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false)) + return c.persistRolloutStatus(roCtx, &newStatus) } currStepAr := analysisutil.GetCurrentStepAnalysisRun(currArs) @@ -293,8 +295,9 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error logCtx.Info("New RS has successfully progressed") newStatus.Canary.StableRS = newStatus.CurrentPodHash } + roCtx.PauseContext().RemovePause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) - return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false)) + return c.persistRolloutStatus(roCtx, &newStatus) } if *currentStepIndex == stepCount { @@ -304,8 +307,9 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error logCtx.Info("New RS has successfully progressed") newStatus.Canary.StableRS = newStatus.CurrentPodHash } + roCtx.PauseContext().RemovePause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) - return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false)) + return c.persistRolloutStatus(roCtx, &newStatus) } // reconcileCanaryPause will ensure we will requeue this rollout at the appropriate time @@ -320,8 +324,9 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error } logCtx.Infof("Incrementing the Current Step Index to %d", *currentStepIndex) c.recorder.Eventf(r, corev1.EventTypeNormal, "SetStepIndex", "Set Step Index to %d", int(*currentStepIndex)) + roCtx.PauseContext().RemovePause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) - return c.persistRolloutStatus(r, &newStatus, pointer.BoolPtr(false)) + return c.persistRolloutStatus(roCtx, &newStatus) } if currExp != nil { @@ -331,12 +336,12 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error } } - addPause := !r.Spec.Paused && currentStep != nil && currentStep.Pause != nil - var paused bool - newStatus.PauseStartTime, paused = calculatePauseStatus(roCtx, addPause) + if !r.Spec.Paused && currentStep != nil && currentStep.Pause != nil { + roCtx.PauseContext().AddPause() + } newStatus.CurrentStepIndex = currentStepIndex newStatus = c.calculateRolloutConditions(roCtx, newStatus) - return c.persistRolloutStatus(r, &newStatus, &paused) + return c.persistRolloutStatus(roCtx, &newStatus) } func (c *RolloutController) reconcileCanaryReplicaSets(roCtx *canaryContext) (bool, error) { diff --git a/rollout/context.go b/rollout/context.go index ccc39779c9..e3b2e3f17e 100644 --- a/rollout/context.go +++ b/rollout/context.go @@ -21,14 +21,19 @@ type rolloutContext interface { OtherAnalysisRuns() []*v1alpha1.AnalysisRun CurrentExperiment() *v1alpha1.Experiment OtherExperiments() []*v1alpha1.Experiment + + PauseContext() *pauseContext } type blueGreenContext struct { - rollout *v1alpha1.Rollout - log *log.Entry + rollout *v1alpha1.Rollout + log *log.Entry + newRS *appsv1.ReplicaSet olderRSs []*appsv1.ReplicaSet allRSs []*appsv1.ReplicaSet + + pauseContext *pauseContext } type canaryContext struct { @@ -45,16 +50,21 @@ type canaryContext struct { currentEx *v1alpha1.Experiment otherExs []*v1alpha1.Experiment + + pauseContext *pauseContext } func newBlueGreenCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, olderRSs []*appsv1.ReplicaSet) *blueGreenContext { allRSs := append(olderRSs, newRS) return &blueGreenContext{ - rollout: r, - log: logutil.WithRollout(r), + rollout: r, + log: logutil.WithRollout(r), + newRS: newRS, olderRSs: olderRSs, allRSs: allRSs, + + pauseContext: &pauseContext{}, } } @@ -93,6 +103,10 @@ func (bgCtx *blueGreenContext) OtherExperiments() []*v1alpha1.Experiment { return nil } +func (bgCtx *blueGreenContext) PauseContext() *pauseContext { + return bgCtx.pauseContext +} + func newCanaryCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, stableRS *appsv1.ReplicaSet, olderRSs []*appsv1.ReplicaSet, exList []*v1alpha1.Experiment, arList []*v1alpha1.AnalysisRun) *canaryContext { allRSs := append(olderRSs, newRS) if stableRS != nil { @@ -115,6 +129,8 @@ func newCanaryCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, stableRS *appsv currentEx: currentEx, otherExs: otherExs, + + pauseContext: &pauseContext{}, } } @@ -164,3 +180,7 @@ func (cCtx *canaryContext) CurrentExperiment() *v1alpha1.Experiment { func (cCtx *canaryContext) OtherExperiments() []*v1alpha1.Experiment { return cCtx.otherExs } + +func (cCtx *canaryContext) PauseContext() *pauseContext { + return cCtx.pauseContext +} diff --git a/rollout/controller.go b/rollout/controller.go index 054219f48f..34f972fceb 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -243,14 +243,12 @@ func (c *RolloutController) syncHandler(key string) error { generation := conditions.ComputeGenerationHash(r.Spec) if r.Status.ObservedGeneration != generation || !reflect.DeepEqual(invalidSpecCond, prevCond) { newStatus := r.Status.DeepCopy() - newStatus.ObservedGeneration = generation // SetRolloutCondition only updates the condition when the status and/or reason changes, but // the controller should update the invalidSpec if there is a change in why the spec is invalid if prevCond != nil && prevCond.Message != invalidSpecCond.Message { conditions.RemoveRolloutCondition(newStatus, v1alpha1.InvalidSpec) } - conditions.SetRolloutCondition(newStatus, *invalidSpecCond) - err := c.persistRolloutStatus(r, newStatus, nil) + err := c.patchCondition(r, newStatus, invalidSpecCond) if err != nil { return err } diff --git a/rollout/pause.go b/rollout/pause.go index 14b1bd0261..5b7d9183d2 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -6,10 +6,26 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" - "github.com/argoproj/argo-rollouts/utils/defaults" logutil "github.com/argoproj/argo-rollouts/utils/log" ) +type pauseContext struct { + addPause bool + removePause bool +} + +func (pCtx *pauseContext) AddPause() { + pCtx.addPause = true +} + +func (pCtx *pauseContext) RemovePause() { + pCtx.removePause = true +} + +func (pCtx *pauseContext) HasPauseChanged() bool { + return pCtx.addPause || pCtx.removePause +} + func completedPauseStep(rollout *v1alpha1.Rollout, pause v1alpha1.RolloutPause) bool { logCtx := logutil.WithRollout(rollout) if pause.Duration != nil { @@ -40,30 +56,18 @@ func (c *RolloutController) checkEnqueueRolloutDuringWait(rollout *v1alpha1.Roll } } -// calculatePauseStatus finds the fields related to a pause step for a rollout. If the pause is nil, -// the rollout will use the previous values -func calculatePauseStatus(roCtx rolloutContext, addPause bool) (*metav1.Time, bool) { +// calculatePauseStatus determines if the rollout should be paused by the controller. +// func calculatePauseStatus(roCtx rolloutContext, addPause bool) (*metav1.Time, bool) { +func calculatePauseStatus(roCtx rolloutContext) (*metav1.Time, bool) { rollout := roCtx.Rollout() logCtx := roCtx.Log() + pauseCtx := roCtx.PauseContext() pauseStartTime := rollout.Status.PauseStartTime paused := rollout.Spec.Paused if !paused { pauseStartTime = nil } - if rollout.Spec.Strategy.BlueGreen != nil && defaults.GetAutoPromotionEnabledOrDefault(rollout) { - return nil, false - } - - pauseForInconclusiveAnalysisRun := false - currArs := roCtx.CurrentAnalysisRuns() - for i := range currArs { - ar := currArs[i] - if ar != nil && ar.Status != nil && ar.Status.Status == v1alpha1.AnalysisStatusInconclusive { - pauseForInconclusiveAnalysisRun = true - } - } - - if addPause || pauseForInconclusiveAnalysisRun { + if pauseCtx.addPause { if pauseStartTime == nil { now := metav1.Now() logCtx.Infof("Setting PauseStartTime to %s", now.UTC().Format(time.RFC3339)) @@ -71,21 +75,9 @@ func calculatePauseStatus(roCtx rolloutContext, addPause bool) (*metav1.Time, bo paused = true } } - - if rollout.Spec.Strategy.BlueGreen != nil { - bgCtx := roCtx.(*blueGreenContext) - if reconcileBlueGreenTemplateChange(bgCtx) { - return nil, false - } - if paused && pauseStartTime != nil && rollout.Spec.Strategy.BlueGreen.AutoPromotionSeconds != nil { - now := metav1.Now() - autoPromoteActiveServiceDelaySeconds := *rollout.Spec.Strategy.BlueGreen.AutoPromotionSeconds - switchDeadline := pauseStartTime.Add(time.Duration(autoPromoteActiveServiceDelaySeconds) * time.Second) - if now.After(switchDeadline) { - return nil, false - } - return pauseStartTime, true - } + if pauseCtx.removePause { + return nil, false } + return pauseStartTime, paused } diff --git a/rollout/service.go b/rollout/service.go index 1d4550e044..759a46fefd 100644 --- a/rollout/service.go +++ b/rollout/service.go @@ -119,7 +119,8 @@ func (c *RolloutController) reconcileActiveService(roCtx *blueGreenContext, prev } // getReferencedService returns service references in rollout spec and sets warning condition if service does not exist -func (c *RolloutController) getReferencedService(r *v1alpha1.Rollout, serviceName string) (*corev1.Service, error) { +func (c *RolloutController) getReferencedService(roCtx rolloutContext, serviceName string) (*corev1.Service, error) { + r := roCtx.Rollout() svc, err := c.servicesLister.Services(r.Namespace).Get(serviceName) if err != nil { if errors.IsNotFound(err) { @@ -128,7 +129,7 @@ func (c *RolloutController) getReferencedService(r *v1alpha1.Rollout, serviceNam newStatus := r.Status.DeepCopy() cond := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionFalse, conditions.ServiceNotFoundReason, msg) conditions.SetRolloutCondition(newStatus, *cond) - c.persistRolloutStatus(r, newStatus, &r.Spec.Paused) + c.persistRolloutStatus(roCtx, newStatus) } return nil, err } @@ -139,8 +140,10 @@ func (c *RolloutController) getPreviewAndActiveServices(r *v1alpha1.Rollout) (*c var previewSvc *corev1.Service var activeSvc *corev1.Service var err error + + roCtx := newBlueGreenCtx(r, nil, nil) if r.Spec.Strategy.BlueGreen.PreviewService != "" { - previewSvc, err = c.getReferencedService(r, r.Spec.Strategy.BlueGreen.PreviewService) + previewSvc, err = c.getReferencedService(roCtx, r.Spec.Strategy.BlueGreen.PreviewService) if err != nil { return nil, nil, err } @@ -148,7 +151,7 @@ func (c *RolloutController) getPreviewAndActiveServices(r *v1alpha1.Rollout) (*c if r.Spec.Strategy.BlueGreen.ActiveService == "" { return nil, nil, fmt.Errorf("Invalid Spec: Rollout missing field ActiveService") } - activeSvc, err = c.getReferencedService(r, r.Spec.Strategy.BlueGreen.ActiveService) + activeSvc, err = c.getReferencedService(roCtx, r.Spec.Strategy.BlueGreen.ActiveService) if err != nil { return nil, nil, err } @@ -162,7 +165,7 @@ func (c *RolloutController) reconcileCanaryService(roCtx *canaryContext) error { return nil } - svc, err := c.getReferencedService(r, r.Spec.Strategy.Canary.CanaryService) + svc, err := c.getReferencedService(roCtx, r.Spec.Strategy.Canary.CanaryService) if err != nil { return err } diff --git a/rollout/sync.go b/rollout/sync.go index c939163a36..f588091dd4 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -182,8 +182,7 @@ func (c *RolloutController) getNewReplicaSet(rollout *v1alpha1.Rollout, rsList, c.recorder.Event(rollout, corev1.EventTypeWarning, conditions.FailedRSCreateReason, msg) newStatus := rollout.Status.DeepCopy() cond := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionFalse, conditions.FailedRSCreateReason, msg) - conditions.SetRolloutCondition(newStatus, *cond) - c.persistRolloutStatus(rollout, newStatus, &rollout.Spec.Paused) + err := c.patchCondition(rollout, newStatus, cond) return nil, err } @@ -225,7 +224,8 @@ func (c *RolloutController) syncReplicasOnly(r *v1alpha1.Rollout, rsList []*apps // so we can abort this resync return err } - return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx, r.Spec.Paused) + c.reconcileBlueGreenPause(previewSvc, activeSvc, roCtx) + return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } // The controller wants to use the rolloutCanary method to reconcile the rolllout if the rollout is not paused. // If there are no scaling events, the rollout should only sync its status @@ -419,26 +419,52 @@ func (c *RolloutController) checkPausedConditions(r *v1alpha1.Rollout) error { } pausedCondExists := cond != nil && cond.Reason == conditions.PausedRolloutReason - newStatus := r.Status.DeepCopy() - needsUpdate := false + var updatedConditon *v1alpha1.RolloutCondition if r.Spec.Paused && !pausedCondExists { - condition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.PausedRolloutReason, conditions.PausedRolloutMessage) - conditions.SetRolloutCondition(newStatus, *condition) - needsUpdate = true + updatedConditon = conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.PausedRolloutReason, conditions.PausedRolloutMessage) } else if !r.Spec.Paused && pausedCondExists { - condition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.ResumedRolloutReason, conditions.ResumeRolloutMessage) - conditions.SetRolloutCondition(newStatus, *condition) - needsUpdate = true + updatedConditon = conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.ResumedRolloutReason, conditions.ResumeRolloutMessage) } - if !needsUpdate { + if updatedConditon == nil { return nil } - err := c.persistRolloutStatus(r, newStatus, &r.Spec.Paused) + newStatus := r.Status.DeepCopy() + err := c.patchCondition(r, newStatus, updatedConditon) return err } +func (c *RolloutController) patchCondition(r *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus, condition *v1alpha1.RolloutCondition) error { + conditions.SetRolloutCondition(newStatus, *condition) + newStatus.ObservedGeneration = conditions.ComputeGenerationHash(r.Spec) + + logCtx := logutil.WithRollout(r) + patch, modified, err := diff.CreateTwoWayMergePatch( + &v1alpha1.Rollout{ + Status: r.Status, + }, + &v1alpha1.Rollout{ + Status: *newStatus, + }, v1alpha1.Rollout{}) + if err != nil { + logCtx.Errorf("Error constructing app status patch: %v", err) + return err + } + if !modified { + logCtx.Info("No status changes. Skipping patch") + return nil + } + logCtx.Debugf("Rollout Condition Patch: %s", patch) + _, err = c.argoprojclientset.ArgoprojV1alpha1().Rollouts(r.Namespace).Patch(r.Name, patchtypes.MergePatchType, patch) + if err != nil { + logCtx.Warningf("Error patching rollout: %v", err) + return err + } + logCtx.Info("Condition Patch status successfully") + return nil +} + func (c *RolloutController) calculateRolloutConditions(roCtx rolloutContext, newStatus v1alpha1.RolloutStatus) v1alpha1.RolloutStatus { r := roCtx.Rollout() allRSs := roCtx.AllRSs() @@ -541,12 +567,16 @@ func (c *RolloutController) calculateRolloutConditions(roCtx rolloutContext, new } // persistRolloutStatus persists updates to rollout status. If no changes were made, it is a no-op -func (c *RolloutController) persistRolloutStatus(orig *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus, newPause *bool) error { +func (c *RolloutController) persistRolloutStatus(roCtx rolloutContext, newStatus *v1alpha1.RolloutStatus) error { + orig := roCtx.Rollout() specCopy := orig.Spec.DeepCopy() - paused := specCopy.Paused - if newPause != nil { - paused = *newPause - specCopy.Paused = *newPause + + pauseStartTime, newPause := calculatePauseStatus(roCtx) + newStatus.PauseStartTime = pauseStartTime + + pauseCtx := roCtx.PauseContext() + if pauseCtx.addPause || pauseCtx.removePause { + specCopy.Paused = newPause } newStatus.ObservedGeneration = conditions.ComputeGenerationHash(*specCopy) @@ -560,7 +590,7 @@ func (c *RolloutController) persistRolloutStatus(orig *v1alpha1.Rollout, newStat }, &v1alpha1.Rollout{ Spec: v1alpha1.RolloutSpec{ - Paused: paused, + Paused: newPause, }, Status: *newStatus, }, v1alpha1.Rollout{}) From ee3f46751d79a44306a3f0ee0279af056bb550bc Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Wed, 16 Oct 2019 16:17:56 -0700 Subject: [PATCH 02/16] Add status.controllerPause field --- manifests/crds/rollout-crd.yaml | 2 ++ manifests/install.yaml | 2 ++ manifests/namespace-install.yaml | 2 ++ pkg/apis/rollouts/v1alpha1/openapi_generated.go | 7 +++++++ pkg/apis/rollouts/v1alpha1/types.go | 2 ++ 5 files changed, 15 insertions(+) diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index 68411afed9..7ef91db093 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -2680,6 +2680,8 @@ spec: - type type: object type: array + controllerPause: + type: boolean currentPodHash: type: string currentStepHash: diff --git a/manifests/install.yaml b/manifests/install.yaml index a165369a2a..1106abbd12 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -10354,6 +10354,8 @@ spec: - type type: object type: array + controllerPause: + type: boolean currentPodHash: type: string currentStepHash: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 4bc229b0ff..1f3cc6472e 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -10354,6 +10354,8 @@ spec: - type type: object type: array + controllerPause: + type: boolean currentPodHash: type: string currentStepHash: diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index c91b8a13cc..2c81b8a14a 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -1657,6 +1657,13 @@ func schema_pkg_apis_rollouts_v1alpha1_RolloutStatus(ref common.ReferenceCallbac SchemaProps: spec.SchemaProps{ Description: "RolloutStatus is the status for a Rollout resource", Properties: map[string]spec.Schema{ + "controllerPause": { + SchemaProps: spec.SchemaProps{ + Description: "ControllerPause indicates the controller paused the rollout (i.e. pause step or inconclusive run)", + Type: []string{"boolean"}, + Format: "", + }, + }, "currentPodHash": { SchemaProps: spec.SchemaProps{ Description: "CurrentPodHash the hash of the current pod template", diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 24a853ff60..4242d7c25b 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -256,6 +256,8 @@ type RolloutPause struct { // RolloutStatus is the status for a Rollout resource type RolloutStatus struct { + //ControllerPause indicates the controller paused the rollout (i.e. pause step or inconclusive run) + ControllerPause bool `json:"controllerPause,omitempty"` // CurrentPodHash the hash of the current pod template // +optional CurrentPodHash string `json:"currentPodHash,omitempty"` From 0e43bf5791404048dc08bca368b98fd42cd63780 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Wed, 16 Oct 2019 17:08:53 -0700 Subject: [PATCH 03/16] Move spec.pause to status.controllerPause --- rollout/analysis.go | 4 ++-- rollout/analysis_test.go | 4 +--- rollout/bluegreen.go | 12 ++++++------ rollout/bluegreen_test.go | 12 +++--------- rollout/canary.go | 19 +++++++++---------- rollout/canary_test.go | 17 +++++------------ rollout/controller.go | 2 +- rollout/controller_test.go | 6 ++++-- rollout/pause.go | 25 ++++++++++--------------- rollout/sync.go | 24 ++++++------------------ utils/replicaset/replicaset.go | 2 +- 11 files changed, 48 insertions(+), 79 deletions(-) diff --git a/rollout/analysis.go b/rollout/analysis.go index ad63c497e2..c4d31369cd 100644 --- a/rollout/analysis.go +++ b/rollout/analysis.go @@ -44,7 +44,7 @@ func (c *RolloutController) getAnalysisRunsForRollout(rollout *v1alpha1.Rollout) func (c *RolloutController) reconcileAnalysisRuns(roCtx *canaryContext) error { rollout := roCtx.Rollout() otherArs := roCtx.OtherAnalysisRuns() - if rollout.Spec.Paused { + if rollout.Status.ControllerPause { return nil } newCurrentAnalysisRuns := []*v1alpha1.AnalysisRun{} @@ -148,7 +148,7 @@ func (c *RolloutController) reconcileStepBasedAnalysisRun(roCtx *canaryContext) } if currentAr.Status != nil && currentAr.Status.Status == v1alpha1.AnalysisStatusInconclusive { - roCtx.PauseContext().AddPause() + roCtx.PauseContext().AddControllerPause() } return currentAr, nil diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index d757a2f250..57777d66c3 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -542,10 +542,8 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { patch := f.getPatchedRollout(patchIndex) now := metav1.Now().UTC().Format(time.RFC3339) expectedPatch := `{ - "spec":{ - "paused": true - }, "status": { + "controllerPause": true, "conditions": %s, "canary": { "currentStepAnalysisRun": null diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index baf9b0c4fd..1b2657b7a2 100644 --- a/rollout/bluegreen.go +++ b/rollout/bluegreen.go @@ -32,7 +32,7 @@ func (c *RolloutController) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*apps logCtx := roCtx.Log() allRSs := roCtx.AllRSs() if reconcileBlueGreenTemplateChange(roCtx) { - roCtx.PauseContext().RemovePause() + roCtx.PauseContext().RemoveControllerPause() logCtx.Infof("New pod template or template change detected") return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } @@ -137,7 +137,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev rollout := roCtx.Rollout() if defaults.GetAutoPromotionEnabledOrDefault(rollout) { - roCtx.PauseContext().RemovePause() + roCtx.PauseContext().RemoveControllerPause() return false } @@ -148,8 +148,8 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev return false } // If the rollout is not paused and the active service is not point at the newRS, we should pause the rollout. - if !rollout.Spec.Paused && rollout.Status.PauseStartTime == nil && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { - roCtx.PauseContext().AddPause() + if !rollout.Status.ControllerPause && rollout.Status.PauseStartTime == nil && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { + roCtx.PauseContext().AddControllerPause() return true } @@ -160,12 +160,12 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev switchDeadline := pauseStartTime.Add(time.Duration(*autoPromoteActiveServiceDelaySeconds) * time.Second) now := metav1.Now() if now.After(switchDeadline) { - roCtx.PauseContext().RemovePause() + roCtx.PauseContext().RemoveControllerPause() } } - return rollout.Spec.Paused && pauseStartTime != nil + return rollout.Status.ControllerPause && pauseStartTime != nil } // scaleDownOldReplicaSetsForBlueGreen scales down old replica sets when rollout strategy is "Blue Green". diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index af7a9729ef..7812671c9e 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -157,10 +157,8 @@ func TestBlueGreenHandlePause(t *testing.T) { f.run(getKey(r2, t)) expectedPatch := `{ - "spec": { - "paused": true - }, "status": { + "controllerPause": true, "pauseStartTime": "%s" } }` @@ -311,10 +309,8 @@ func TestBlueGreenHandlePause(t *testing.T) { f.serviceLister = append(f.serviceLister, activeSvc, previewSvc) expectedPatchWithoutSubs := `{ - "spec": { - "paused": null - }, "status": { + "controllerPause": null, "pauseStartTime": null } }` @@ -403,10 +399,8 @@ func TestBlueGreenHandlePause(t *testing.T) { now := metav1.Now().UTC().Format(time.RFC3339) expectedPatchWithoutSubs := `{ - "spec": { - "paused": true - }, "status": { + "controllerPause": true, "pauseStartTime": "%s" } }` diff --git a/rollout/canary.go b/rollout/canary.go index f6187def05..780ca2621f 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -119,7 +119,9 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { if currentStep.Pause == nil { return false } - + if !rollout.Status.ControllerPause { + roCtx.PauseContext().AddControllerPause() + } if currentStep.Pause.Duration == nil { return true } @@ -239,7 +241,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error newStatus.HPAReplicas = replicasetutil.GetActualReplicaCountForReplicaSets(allRSs) newStatus.Selector = metav1.FormatLabelSelector(r.Spec.Selector) - currentStep, currentStepIndex := replicasetutil.GetCurrentCanaryStep(r) + _, currentStepIndex := replicasetutil.GetCurrentCanaryStep(r) newStatus.Canary.StableRS = r.Status.Canary.StableRS newStatus.CurrentStepHash = conditions.ComputeStepHash(r) stepCount := int32(len(r.Spec.Strategy.Canary.Steps)) @@ -254,7 +256,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error c.recorder.Event(r, corev1.EventTypeNormal, "SkipSteps", msg) } } - roCtx.PauseContext().RemovePause() + roCtx.PauseContext().RemoveControllerPause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } @@ -270,7 +272,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error newStatus.CurrentStepIndex = &stepCount } - roCtx.PauseContext().RemovePause() + roCtx.PauseContext().RemoveControllerPause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } @@ -295,7 +297,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error logCtx.Info("New RS has successfully progressed") newStatus.Canary.StableRS = newStatus.CurrentPodHash } - roCtx.PauseContext().RemovePause() + roCtx.PauseContext().RemoveControllerPause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } @@ -307,7 +309,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error logCtx.Info("New RS has successfully progressed") newStatus.Canary.StableRS = newStatus.CurrentPodHash } - roCtx.PauseContext().RemovePause() + roCtx.PauseContext().RemoveControllerPause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } @@ -324,7 +326,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error } logCtx.Infof("Incrementing the Current Step Index to %d", *currentStepIndex) c.recorder.Eventf(r, corev1.EventTypeNormal, "SetStepIndex", "Set Step Index to %d", int(*currentStepIndex)) - roCtx.PauseContext().RemovePause() + roCtx.PauseContext().RemoveControllerPause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } @@ -336,9 +338,6 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error } } - if !r.Spec.Paused && currentStep != nil && currentStep.Pause != nil { - roCtx.PauseContext().AddPause() - } newStatus.CurrentStepIndex = currentStepIndex newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) diff --git a/rollout/canary_test.go b/rollout/canary_test.go index df009a6abc..4005235268 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -104,10 +104,8 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) { patch := f.getPatchedRollout(patchIndex) expectedPatchTemplate := `{ - "spec":{ - "paused": true - }, "status":{ + "controllerPause": true, "pauseStartTime":"%s", "conditions": %s } @@ -775,10 +773,8 @@ func TestSyncRolloutsSetPauseStartTime(t *testing.T) { f.objects = append(f.objects, r2) expectedPatchWithoutTime := `{ - "spec" :{ - "paused": true - }, "status":{ + "controllerPause": true, "pauseStartTime": "%s", "conditions": %s } @@ -1070,7 +1066,6 @@ func TestResumeRolloutAfterPauseDuration(t *testing.T) { rs1 := newReplicaSetWithStatus(r1, 1, 1) rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r1 = updateCanaryRolloutStatus(r1, rs1PodHash, 1, 1, 1, true) - r1.Spec.Paused = true overAMinuteAgo := metav1.Time{Time: time.Now().Add(-61 * time.Second)} r1.Status.ObservedGeneration = conditions.ComputeGenerationHash(r1.Spec) r1.Status.PauseStartTime = &overAMinuteAgo @@ -1089,14 +1084,12 @@ func TestResumeRolloutAfterPauseDuration(t *testing.T) { err := json.Unmarshal([]byte(patch), &patchObj) assert.NoError(t, err) - spec := patchObj["spec"].(map[string]interface{}) - paused, ok := spec["paused"] - assert.True(t, ok) - assert.Nil(t, paused) - status := patchObj["status"].(map[string]interface{}) assert.Equal(t, float64(2), status["currentStepIndex"]) pauseStartTime, ok := status["pauseStartTime"] assert.True(t, ok) assert.Equal(t, nil, pauseStartTime) + controllerPause, ok := status["controllerPause"] + assert.True(t, ok) + assert.Nil(t, controllerPause) } diff --git a/rollout/controller.go b/rollout/controller.go index 34f972fceb..4a36f26661 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -273,7 +273,7 @@ func (c *RolloutController) syncHandler(key string) error { return err } - if rollout.Spec.Paused || isScalingEvent { + if rollout.Status.ControllerPause || isScalingEvent { return c.syncReplicasOnly(r, rsList, isScalingEvent) } diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 71bdbed676..e9ab3e9521 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -263,7 +263,7 @@ func updateBaseRolloutStatus(r *v1alpha1.Rollout, availableReplicas, updatedRepl newRollout.Status.UpdatedReplicas = updatedReplicas newRollout.Status.HPAReplicas = hpaReplicas if pause { - newRollout.Spec.Paused = pause + newRollout.Status.ControllerPause = pause now := metav1.Now() newRollout.Status.PauseStartTime = &now } @@ -876,9 +876,11 @@ func TestRequeueStuckRollout(t *testing.T) { r := &v1alpha1.Rollout{ Spec: v1alpha1.RolloutSpec{ Replicas: pointer.Int32Ptr(0), - Paused: rolloutPaused, ProgressDeadlineSeconds: progessDeadlineSeconds, }, + Status: v1alpha1.RolloutStatus{ + ControllerPause: rolloutPaused, + }, } if rolloutCompleted { r.Status.ObservedGeneration = conditions.ComputeGenerationHash(r.Spec) diff --git a/rollout/pause.go b/rollout/pause.go index 5b7d9183d2..3e1fad55ac 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -10,20 +10,16 @@ import ( ) type pauseContext struct { - addPause bool - removePause bool + addControllerPause bool + removeControllerPause bool } -func (pCtx *pauseContext) AddPause() { - pCtx.addPause = true +func (pCtx *pauseContext) AddControllerPause() { + pCtx.addControllerPause = true } -func (pCtx *pauseContext) RemovePause() { - pCtx.removePause = true -} - -func (pCtx *pauseContext) HasPauseChanged() bool { - return pCtx.addPause || pCtx.removePause +func (pCtx *pauseContext) RemoveControllerPause() { + pCtx.removeControllerPause = true } func completedPauseStep(rollout *v1alpha1.Rollout, pause v1alpha1.RolloutPause) bool { @@ -37,7 +33,7 @@ func completedPauseStep(rollout *v1alpha1.Rollout, pause v1alpha1.RolloutPause) return true } } - } else if rollout.Status.PauseStartTime != nil && !rollout.Spec.Paused { + } else if rollout.Status.PauseStartTime != nil && !rollout.Status.ControllerPause { logCtx.Info("Rollout has been unpaused") return true } @@ -57,17 +53,16 @@ func (c *RolloutController) checkEnqueueRolloutDuringWait(rollout *v1alpha1.Roll } // calculatePauseStatus determines if the rollout should be paused by the controller. -// func calculatePauseStatus(roCtx rolloutContext, addPause bool) (*metav1.Time, bool) { func calculatePauseStatus(roCtx rolloutContext) (*metav1.Time, bool) { rollout := roCtx.Rollout() logCtx := roCtx.Log() pauseCtx := roCtx.PauseContext() pauseStartTime := rollout.Status.PauseStartTime - paused := rollout.Spec.Paused + paused := rollout.Status.ControllerPause if !paused { pauseStartTime = nil } - if pauseCtx.addPause { + if pauseCtx.addControllerPause { if pauseStartTime == nil { now := metav1.Now() logCtx.Infof("Setting PauseStartTime to %s", now.UTC().Format(time.RFC3339)) @@ -75,7 +70,7 @@ func calculatePauseStatus(roCtx rolloutContext) (*metav1.Time, bool) { paused = true } } - if pauseCtx.removePause { + if pauseCtx.removeControllerPause { return nil, false } diff --git a/rollout/sync.go b/rollout/sync.go index f588091dd4..5668d9220b 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -207,7 +207,7 @@ func (c *RolloutController) getNewReplicaSet(rollout *v1alpha1.Rollout, rsList, // syncReplicasOnly is responsible for reconciling rollouts on scaling events. func (c *RolloutController) syncReplicasOnly(r *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet, isScaling bool) error { logCtx := logutil.WithRollout(r) - logCtx.Infof("Syncing replicas only (paused: %v, isScaling: %v)", r.Spec.Paused, isScaling) + logCtx.Infof("Syncing replicas only (paused: %v, isScaling: %v)", r.Status.ControllerPause, isScaling) newRS, oldRSs, err := c.getAllReplicaSetsAndSyncRevision(r, rsList, false) if err != nil { return err @@ -420,9 +420,9 @@ func (c *RolloutController) checkPausedConditions(r *v1alpha1.Rollout) error { pausedCondExists := cond != nil && cond.Reason == conditions.PausedRolloutReason var updatedConditon *v1alpha1.RolloutCondition - if r.Spec.Paused && !pausedCondExists { + if r.Status.ControllerPause && !pausedCondExists { updatedConditon = conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.PausedRolloutReason, conditions.PausedRolloutMessage) - } else if !r.Spec.Paused && pausedCondExists { + } else if !r.Status.ControllerPause && pausedCondExists { updatedConditon = conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.ResumedRolloutReason, conditions.ResumeRolloutMessage) } @@ -469,7 +469,7 @@ func (c *RolloutController) calculateRolloutConditions(roCtx rolloutContext, new r := roCtx.Rollout() allRSs := roCtx.AllRSs() newRS := roCtx.NewRS() - if r.Spec.Paused { + if r.Status.ControllerPause { return newStatus } @@ -570,28 +570,16 @@ func (c *RolloutController) calculateRolloutConditions(roCtx rolloutContext, new func (c *RolloutController) persistRolloutStatus(roCtx rolloutContext, newStatus *v1alpha1.RolloutStatus) error { orig := roCtx.Rollout() specCopy := orig.Spec.DeepCopy() - pauseStartTime, newPause := calculatePauseStatus(roCtx) newStatus.PauseStartTime = pauseStartTime - - pauseCtx := roCtx.PauseContext() - if pauseCtx.addPause || pauseCtx.removePause { - specCopy.Paused = newPause - } + newStatus.ControllerPause = newPause newStatus.ObservedGeneration = conditions.ComputeGenerationHash(*specCopy) - logCtx := logutil.WithRollout(orig) patch, modified, err := diff.CreateTwoWayMergePatch( &v1alpha1.Rollout{ - Spec: v1alpha1.RolloutSpec{ - Paused: orig.Spec.Paused, - }, Status: orig.Status, }, &v1alpha1.Rollout{ - Spec: v1alpha1.RolloutSpec{ - Paused: newPause, - }, Status: *newStatus, }, v1alpha1.Rollout{}) if err != nil { @@ -627,7 +615,7 @@ func (c *RolloutController) requeueStuckRollout(r *v1alpha1.Rollout, newStatus v return time.Duration(-1) } // No need to estimate progress if the rollout is complete or already timed out. - if conditions.RolloutComplete(r, &newStatus) || currentCond.Reason == conditions.TimedOutReason || r.Spec.Paused { + if conditions.RolloutComplete(r, &newStatus) || currentCond.Reason == conditions.TimedOutReason || r.Status.ControllerPause { return time.Duration(-1) } // If there is no sign of progress at this point then there is a high chance that the diff --git a/utils/replicaset/replicaset.go b/utils/replicaset/replicaset.go index 6378611aa1..f5b6a7d3f9 100644 --- a/utils/replicaset/replicaset.go +++ b/utils/replicaset/replicaset.go @@ -85,7 +85,7 @@ func NewRSNewReplicas(rollout *v1alpha1.Rollout, allRSs []*appsv1.ReplicaSet, ne if newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] != rollout.Status.CurrentPodHash { return *rollout.Spec.Strategy.BlueGreen.PreviewReplicaCount, nil } - if !rollout.Spec.Paused && rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint { + if !rollout.Status.ControllerPause && rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint { return defaults.GetRolloutReplicasOrDefault(rollout), nil } return *rollout.Spec.Strategy.BlueGreen.PreviewReplicaCount, nil From c13d68a7b6a2926f542e26bcaf31ab6a329d5357 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Thu, 17 Oct 2019 14:26:51 -0700 Subject: [PATCH 04/16] Move more into pause context --- rollout/canary.go | 36 +++++++++++---------- rollout/context.go | 18 ++++++++--- rollout/pause.go | 79 +++++++++++++++++++++++++++++++--------------- rollout/service.go | 13 +++----- rollout/sync.go | 9 ++++-- 5 files changed, 98 insertions(+), 57 deletions(-) diff --git a/rollout/canary.go b/rollout/canary.go index 780ca2621f..1f0282d30c 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -277,13 +277,19 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error return c.persistRolloutStatus(roCtx, &newStatus) } - currStepAr := analysisutil.GetCurrentStepAnalysisRun(currArs) - if currStepAr != nil { - if currStepAr.Status == nil || !currStepAr.Status.Status.Completed() || analysisutil.IsTerminating(currStepAr) { - newStatus.Canary.CurrentStepAnalysisRun = currStepAr.Name + if currentStepIndex != nil && *currentStepIndex == stepCount { + logCtx.Info("Rollout has executed every step") + newStatus.CurrentStepIndex = &stepCount + if newRS != nil && newRS.Status.AvailableReplicas == defaults.GetRolloutReplicasOrDefault(r) { + //TODO(dthomson) cancel background analysis here not when we reach currentStepIndex == stepCount + logCtx.Info("New RS has successfully progressed") + newStatus.Canary.StableRS = newStatus.CurrentPodHash } - + roCtx.PauseContext().RemoveControllerPause() + newStatus = c.calculateRolloutConditions(roCtx, newStatus) + return c.persistRolloutStatus(roCtx, &newStatus) } + currBackgroundAr := analysisutil.GetCurrentBackgroundAnalysisRun(currArs) if currBackgroundAr != nil { if currBackgroundAr.Status == nil || !currBackgroundAr.Status.Status.Completed() || analysisutil.IsTerminating(currBackgroundAr) { @@ -302,18 +308,6 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error return c.persistRolloutStatus(roCtx, &newStatus) } - if *currentStepIndex == stepCount { - logCtx.Info("Rollout has executed every step") - newStatus.CurrentStepIndex = &stepCount - if newRS != nil && newRS.Status.AvailableReplicas == defaults.GetRolloutReplicasOrDefault(r) { - logCtx.Info("New RS has successfully progressed") - newStatus.Canary.StableRS = newStatus.CurrentPodHash - } - roCtx.PauseContext().RemoveControllerPause() - newStatus = c.calculateRolloutConditions(roCtx, newStatus) - return c.persistRolloutStatus(roCtx, &newStatus) - } - // reconcileCanaryPause will ensure we will requeue this rollout at the appropriate time // if we are at a pause step with a duration. c.reconcileCanaryPause(roCtx) @@ -331,6 +325,14 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error return c.persistRolloutStatus(roCtx, &newStatus) } + currStepAr := analysisutil.GetCurrentStepAnalysisRun(currArs) + if currStepAr != nil { + if currStepAr.Status == nil || !currStepAr.Status.Status.Completed() || analysisutil.IsTerminating(currStepAr) { + newStatus.Canary.CurrentStepAnalysisRun = currStepAr.Name + } + + } + if currExp != nil { newStatus.Canary.CurrentExperiment = currExp.Name if conditions.ExperimentTimeOut(currExp, currExp.Status) { diff --git a/rollout/context.go b/rollout/context.go index e3b2e3f17e..390ddd8a3c 100644 --- a/rollout/context.go +++ b/rollout/context.go @@ -56,15 +56,20 @@ type canaryContext struct { func newBlueGreenCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, olderRSs []*appsv1.ReplicaSet) *blueGreenContext { allRSs := append(olderRSs, newRS) + logCtx := logutil.WithRollout(r) return &blueGreenContext{ rollout: r, - log: logutil.WithRollout(r), + log: logCtx, newRS: newRS, olderRSs: olderRSs, allRSs: allRSs, - pauseContext: &pauseContext{}, + pauseContext: &pauseContext{ + log: logCtx, + controllerPause: r.Status.ControllerPause, + pauseStartTime: r.Status.PauseStartTime, + }, } } @@ -116,9 +121,10 @@ func newCanaryCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, stableRS *appsv currentArs, otherArs := analysisutil.FilterCurrentRolloutAnalysisRuns(arList, r) currentEx := experimentutil.GetCurrentExperiment(r, exList) otherExs := experimentutil.GetOldExperiments(r, exList) + logCtx := logutil.WithRollout(r) return &canaryContext{ rollout: r, - log: logutil.WithRollout(r), + log: logCtx, newRS: newRS, stableRS: stableRS, olderRSs: olderRSs, @@ -130,7 +136,11 @@ func newCanaryCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, stableRS *appsv currentEx: currentEx, otherExs: otherExs, - pauseContext: &pauseContext{}, + pauseContext: &pauseContext{ + log: logCtx, + controllerPause: r.Status.ControllerPause, + pauseStartTime: r.Status.PauseStartTime, + }, } } diff --git a/rollout/pause.go b/rollout/pause.go index 3e1fad55ac..bce1c0aa04 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -3,6 +3,7 @@ package rollout import ( "time" + log "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" @@ -10,6 +11,10 @@ import ( ) type pauseContext struct { + log *log.Entry + controllerPause bool + pauseStartTime *metav1.Time + addControllerPause bool removeControllerPause bool } @@ -22,34 +27,28 @@ func (pCtx *pauseContext) RemoveControllerPause() { pCtx.removeControllerPause = true } -func completedPauseStep(rollout *v1alpha1.Rollout, pause v1alpha1.RolloutPause) bool { - logCtx := logutil.WithRollout(rollout) - if pause.Duration != nil { - now := metav1.Now() - if rollout.Status.PauseStartTime != nil { - expiredTime := rollout.Status.PauseStartTime.Add(time.Duration(*pause.Duration) * time.Second) - if now.After(expiredTime) { - logCtx.Info("Rollout has waited the duration of the pause step") - return true - } - } - } else if rollout.Status.PauseStartTime != nil && !rollout.Status.ControllerPause { - logCtx.Info("Rollout has been unpaused") - return true +func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus) { + if pCtx.removeControllerPause { + return } - return false -} -func (c *RolloutController) checkEnqueueRolloutDuringWait(rollout *v1alpha1.Rollout, startTime metav1.Time, durationInSeconds int32) { - logCtx := logutil.WithRollout(rollout) - now := metav1.Now() - expiredTime := startTime.Add(time.Duration(durationInSeconds) * time.Second) - nextResync := now.Add(c.resyncPeriod) - if nextResync.After(expiredTime) && expiredTime.After(now.Time) { - timeRemaining := expiredTime.Sub(now.Time) - logCtx.Infof("Enqueueing Rollout in %s seconds", timeRemaining.String()) - c.enqueueRolloutAfter(rollout, timeRemaining) + pauseStartTime := pCtx.pauseStartTime + paused := pCtx.controllerPause + if !paused { + pauseStartTime = nil } + if pCtx.addControllerPause { + if pauseStartTime == nil { + now := metav1.Now() + pCtx.log.Infof("Setting PauseStartTime to %s", now.UTC().Format(time.RFC3339)) + pauseStartTime = &now + paused = true + } + } + + newStatus.ControllerPause = paused + newStatus.PauseStartTime = pauseStartTime + // newStatus.PauseReason } // calculatePauseStatus determines if the rollout should be paused by the controller. @@ -76,3 +75,33 @@ func calculatePauseStatus(roCtx rolloutContext) (*metav1.Time, bool) { return pauseStartTime, paused } + +func completedPauseStep(rollout *v1alpha1.Rollout, pause v1alpha1.RolloutPause) bool { + logCtx := logutil.WithRollout(rollout) + if pause.Duration != nil { + now := metav1.Now() + if rollout.Status.PauseStartTime != nil { + expiredTime := rollout.Status.PauseStartTime.Add(time.Duration(*pause.Duration) * time.Second) + if now.After(expiredTime) { + logCtx.Info("Rollout has waited the duration of the pause step") + return true + } + } + } else if rollout.Status.PauseStartTime != nil && !rollout.Status.ControllerPause { + logCtx.Info("Rollout has been unpaused") + return true + } + return false +} + +func (c *RolloutController) checkEnqueueRolloutDuringWait(rollout *v1alpha1.Rollout, startTime metav1.Time, durationInSeconds int32) { + logCtx := logutil.WithRollout(rollout) + now := metav1.Now() + expiredTime := startTime.Add(time.Duration(durationInSeconds) * time.Second) + nextResync := now.Add(c.resyncPeriod) + if nextResync.After(expiredTime) && expiredTime.After(now.Time) { + timeRemaining := expiredTime.Sub(now.Time) + logCtx.Infof("Enqueueing Rollout in %s seconds", timeRemaining.String()) + c.enqueueRolloutAfter(rollout, timeRemaining) + } +} diff --git a/rollout/service.go b/rollout/service.go index 759a46fefd..7ad3db428a 100644 --- a/rollout/service.go +++ b/rollout/service.go @@ -119,8 +119,7 @@ func (c *RolloutController) reconcileActiveService(roCtx *blueGreenContext, prev } // getReferencedService returns service references in rollout spec and sets warning condition if service does not exist -func (c *RolloutController) getReferencedService(roCtx rolloutContext, serviceName string) (*corev1.Service, error) { - r := roCtx.Rollout() +func (c *RolloutController) getReferencedService(r *v1alpha1.Rollout, serviceName string) (*corev1.Service, error) { svc, err := c.servicesLister.Services(r.Namespace).Get(serviceName) if err != nil { if errors.IsNotFound(err) { @@ -128,8 +127,7 @@ func (c *RolloutController) getReferencedService(roCtx rolloutContext, serviceNa c.recorder.Event(r, corev1.EventTypeWarning, conditions.ServiceNotFoundReason, msg) newStatus := r.Status.DeepCopy() cond := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionFalse, conditions.ServiceNotFoundReason, msg) - conditions.SetRolloutCondition(newStatus, *cond) - c.persistRolloutStatus(roCtx, newStatus) + c.patchCondition(r, newStatus, cond) } return nil, err } @@ -141,9 +139,8 @@ func (c *RolloutController) getPreviewAndActiveServices(r *v1alpha1.Rollout) (*c var activeSvc *corev1.Service var err error - roCtx := newBlueGreenCtx(r, nil, nil) if r.Spec.Strategy.BlueGreen.PreviewService != "" { - previewSvc, err = c.getReferencedService(roCtx, r.Spec.Strategy.BlueGreen.PreviewService) + previewSvc, err = c.getReferencedService(r, r.Spec.Strategy.BlueGreen.PreviewService) if err != nil { return nil, nil, err } @@ -151,7 +148,7 @@ func (c *RolloutController) getPreviewAndActiveServices(r *v1alpha1.Rollout) (*c if r.Spec.Strategy.BlueGreen.ActiveService == "" { return nil, nil, fmt.Errorf("Invalid Spec: Rollout missing field ActiveService") } - activeSvc, err = c.getReferencedService(roCtx, r.Spec.Strategy.BlueGreen.ActiveService) + activeSvc, err = c.getReferencedService(r, r.Spec.Strategy.BlueGreen.ActiveService) if err != nil { return nil, nil, err } @@ -165,7 +162,7 @@ func (c *RolloutController) reconcileCanaryService(roCtx *canaryContext) error { return nil } - svc, err := c.getReferencedService(roCtx, r.Spec.Strategy.Canary.CanaryService) + svc, err := c.getReferencedService(r, r.Spec.Strategy.Canary.CanaryService) if err != nil { return err } diff --git a/rollout/sync.go b/rollout/sync.go index 5668d9220b..06aedb9468 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -250,6 +250,10 @@ func (c *RolloutController) syncReplicasOnly(r *v1alpha1.Rollout, rsList []*apps return err } } + // reconcileCanaryPause will ensure we will requeue this rollout at the appropriate time + // if we are at a pause step with a duration. + c.reconcileCanaryPause(roCtx) + return c.syncRolloutStatusCanary(roCtx) } return fmt.Errorf("no rollout strategy provided") @@ -570,9 +574,8 @@ func (c *RolloutController) calculateRolloutConditions(roCtx rolloutContext, new func (c *RolloutController) persistRolloutStatus(roCtx rolloutContext, newStatus *v1alpha1.RolloutStatus) error { orig := roCtx.Rollout() specCopy := orig.Spec.DeepCopy() - pauseStartTime, newPause := calculatePauseStatus(roCtx) - newStatus.PauseStartTime = pauseStartTime - newStatus.ControllerPause = newPause + + roCtx.PauseContext().CalculatePauseStatus(newStatus) newStatus.ObservedGeneration = conditions.ComputeGenerationHash(*specCopy) logCtx := logutil.WithRollout(orig) patch, modified, err := diff.CreateTwoWayMergePatch( From 6ee5a714481ebeff88d11d13ff7bdea5038cc489 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Sat, 19 Oct 2019 17:55:13 -0700 Subject: [PATCH 05/16] Add pause conditions --- manifests/crds/rollout-crd.yaml | 13 +++ manifests/install.yaml | 13 +++ manifests/namespace-install.yaml | 13 +++ .../rollouts/v1alpha1/openapi_generated.go | 42 +++++++++- pkg/apis/rollouts/v1alpha1/types.go | 20 +++++ .../v1alpha1/zz_generated.deepcopy.go | 24 ++++++ rollout/analysis.go | 2 +- rollout/analysis_test.go | 8 +- rollout/bluegreen.go | 8 +- rollout/bluegreen_test.go | 20 +++-- rollout/canary.go | 19 ++--- rollout/canary_test.go | 18 +++- rollout/context.go | 2 + rollout/controller_test.go | 16 ++++ rollout/pause.go | 82 ++++++++++--------- 15 files changed, 234 insertions(+), 66 deletions(-) diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index 7ef91db093..d3985f8fda 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -2691,6 +2691,19 @@ spec: type: integer observedGeneration: type: string + pauseConditions: + items: + properties: + reason: + type: string + startTime: + format: date-time + type: string + required: + - reason + - startTime + type: object + type: array pauseStartTime: format: date-time type: string diff --git a/manifests/install.yaml b/manifests/install.yaml index 1106abbd12..c6269b85dc 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -10365,6 +10365,19 @@ spec: type: integer observedGeneration: type: string + pauseConditions: + items: + properties: + reason: + type: string + startTime: + format: date-time + type: string + required: + - reason + - startTime + type: object + type: array pauseStartTime: format: date-time type: string diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 1f3cc6472e..79d35f04b5 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -10365,6 +10365,19 @@ spec: type: integer observedGeneration: type: string + pauseConditions: + items: + properties: + reason: + type: string + startTime: + format: date-time + type: string + required: + - reason + - startTime + type: object + type: array pauseStartTime: format: date-time type: string diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index 2c81b8a14a..b99f876d70 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -54,6 +54,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.Metric": schema_pkg_apis_rollouts_v1alpha1_Metric(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.MetricProvider": schema_pkg_apis_rollouts_v1alpha1_MetricProvider(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.MetricResult": schema_pkg_apis_rollouts_v1alpha1_MetricResult(ref), + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.PauseCondition": schema_pkg_apis_rollouts_v1alpha1_PauseCondition(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.PodTemplateMetadata": schema_pkg_apis_rollouts_v1alpha1_PodTemplateMetadata(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.PrometheusMetric": schema_pkg_apis_rollouts_v1alpha1_PrometheusMetric(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.Rollout": schema_pkg_apis_rollouts_v1alpha1_Rollout(ref), @@ -1238,6 +1239,32 @@ func schema_pkg_apis_rollouts_v1alpha1_MetricResult(ref common.ReferenceCallback } } +func schema_pkg_apis_rollouts_v1alpha1_PauseCondition(ref common.ReferenceCallback) common.OpenAPIDefinition { + return common.OpenAPIDefinition{ + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Description: "PauseCondition the reason for a pause and when it started", + Properties: map[string]spec.Schema{ + "reason": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, + "startTime": { + SchemaProps: spec.SchemaProps{ + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Time"), + }, + }, + }, + Required: []string{"reason", "startTime"}, + }, + }, + Dependencies: []string{ + "k8s.io/apimachinery/pkg/apis/meta/v1.Time"}, + } +} + func schema_pkg_apis_rollouts_v1alpha1_PodTemplateMetadata(ref common.ReferenceCallback) common.OpenAPIDefinition { return common.OpenAPIDefinition{ Schema: spec.Schema{ @@ -1657,6 +1684,19 @@ func schema_pkg_apis_rollouts_v1alpha1_RolloutStatus(ref common.ReferenceCallbac SchemaProps: spec.SchemaProps{ Description: "RolloutStatus is the status for a Rollout resource", Properties: map[string]spec.Schema{ + "pauseConditions": { + SchemaProps: spec.SchemaProps{ + Description: "PauseConditions indicates why the rollout is currently paused", + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Ref: ref("github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.PauseCondition"), + }, + }, + }, + }, + }, "controllerPause": { SchemaProps: spec.SchemaProps{ Description: "ControllerPause indicates the controller paused the rollout (i.e. pause step or inconclusive run)", @@ -1776,7 +1816,7 @@ func schema_pkg_apis_rollouts_v1alpha1_RolloutStatus(ref common.ReferenceCallbac }, }, Dependencies: []string{ - "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.BlueGreenStatus", "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.CanaryStatus", "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutCondition", "k8s.io/apimachinery/pkg/apis/meta/v1.Time"}, + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.BlueGreenStatus", "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.CanaryStatus", "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.PauseCondition", "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutCondition", "k8s.io/apimachinery/pkg/apis/meta/v1.Time"}, } } diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 4242d7c25b..2ad99c6311 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -254,8 +254,28 @@ type RolloutPause struct { Duration *int32 `json:"duration,omitempty"` } +// PauseReason reasons that the rollout can pause +type PauseReason string + +const ( + // InconclusiveAnalysisRun pauses rollout when rollout has an inconclusive analysis run + InconclusiveAnalysisRun PauseReason = "InconclusiveAnalysisRun" + // CanaryPauseStep pause rollout for canary pause step + CanaryPauseStep PauseReason = "CanaryPauseStep" + // BlueGreenPause pause rollout before promoting rollout + BlueGreenPause PauseReason = "BlueGreenPause" +) + +// PauseCondition the reason for a pause and when it started +type PauseCondition struct { + Reason PauseReason `json:"reason"` + StartTime metav1.Time `json:"startTime"` +} + // RolloutStatus is the status for a Rollout resource type RolloutStatus struct { + // PauseConditions indicates why the rollout is currently paused + PauseConditions []PauseCondition `json:"pauseConditions,omitempty"` //ControllerPause indicates the controller paused the rollout (i.e. pause step or inconclusive run) ControllerPause bool `json:"controllerPause,omitempty"` // CurrentPodHash the hash of the current pod template diff --git a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go index 828f12d8d6..81a6865fca 100644 --- a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go @@ -709,6 +709,23 @@ func (in *MetricResult) DeepCopy() *MetricResult { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PauseCondition) DeepCopyInto(out *PauseCondition) { + *out = *in + in.StartTime.DeepCopyInto(&out.StartTime) + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PauseCondition. +func (in *PauseCondition) DeepCopy() *PauseCondition { + if in == nil { + return nil + } + out := new(PauseCondition) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PodTemplateMetadata) DeepCopyInto(out *PodTemplateMetadata) { *out = *in @@ -959,6 +976,13 @@ func (in *RolloutSpec) DeepCopy() *RolloutSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RolloutStatus) DeepCopyInto(out *RolloutStatus) { *out = *in + if in.PauseConditions != nil { + in, out := &in.PauseConditions, &out.PauseConditions + *out = make([]PauseCondition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } if in.CurrentStepIndex != nil { in, out := &in.CurrentStepIndex, &out.CurrentStepIndex *out = new(int32) diff --git a/rollout/analysis.go b/rollout/analysis.go index c4d31369cd..69055b1475 100644 --- a/rollout/analysis.go +++ b/rollout/analysis.go @@ -148,7 +148,7 @@ func (c *RolloutController) reconcileStepBasedAnalysisRun(roCtx *canaryContext) } if currentAr.Status != nil && currentAr.Status.Status == v1alpha1.AnalysisStatusInconclusive { - roCtx.PauseContext().AddControllerPause() + roCtx.PauseContext().AddControllerPause(v1alpha1.InconclusiveAnalysisRun) } return currentAr, nil diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index 57777d66c3..e503a95455 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -548,12 +548,16 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { "canary": { "currentStepAnalysisRun": null }, - "pauseStartTime": "%s" + "pauseStartTime": "%s", + "pauseConditions": [{ + "reason": "InconclusiveAnalysisRun", + "startTime": "%s" + }] } }` condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, now)), patch) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, now, now)), patch) } func TestErrorConditionAfterErrorAnalysisRun(t *testing.T) { diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index 1b2657b7a2..b5e1ddcdf4 100644 --- a/rollout/bluegreen.go +++ b/rollout/bluegreen.go @@ -32,7 +32,7 @@ func (c *RolloutController) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*apps logCtx := roCtx.Log() allRSs := roCtx.AllRSs() if reconcileBlueGreenTemplateChange(roCtx) { - roCtx.PauseContext().RemoveControllerPause() + roCtx.PauseContext().ClearPauseReasons() logCtx.Infof("New pod template or template change detected") return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } @@ -137,7 +137,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev rollout := roCtx.Rollout() if defaults.GetAutoPromotionEnabledOrDefault(rollout) { - roCtx.PauseContext().RemoveControllerPause() + roCtx.PauseContext().RemoveControllerPause(v1alpha1.BlueGreenPause) return false } @@ -149,7 +149,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev } // If the rollout is not paused and the active service is not point at the newRS, we should pause the rollout. if !rollout.Status.ControllerPause && rollout.Status.PauseStartTime == nil && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { - roCtx.PauseContext().AddControllerPause() + roCtx.PauseContext().AddControllerPause(v1alpha1.BlueGreenPause) return true } @@ -160,7 +160,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev switchDeadline := pauseStartTime.Add(time.Duration(*autoPromoteActiveServiceDelaySeconds) * time.Second) now := metav1.Now() if now.After(switchDeadline) { - roCtx.PauseContext().RemoveControllerPause() + roCtx.PauseContext().RemoveControllerPause(v1alpha1.BlueGreenPause) } } diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 7812671c9e..7fa4473d47 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -159,10 +159,15 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatch := `{ "status": { "controllerPause": true, - "pauseStartTime": "%s" + "pauseStartTime": "%s", + "pauseConditions": [{ + "reason": "BlueGreenPause", + "startTime": "%s" + }] } }` - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, metav1.Now().UTC().Format(time.RFC3339))), patch) + now := metav1.Now().UTC().Format(time.RFC3339) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now, now)), patch) }) @@ -311,7 +316,8 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatchWithoutSubs := `{ "status": { "controllerPause": null, - "pauseStartTime": null + "pauseStartTime": null, + "pauseConditions": null } }` expectedPatch := calculatePatch(r2, expectedPatchWithoutSubs) @@ -401,10 +407,14 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatchWithoutSubs := `{ "status": { "controllerPause": true, - "pauseStartTime": "%s" + "pauseStartTime": "%s", + "pauseConditions": [{ + "reason":"BlueGreenPause", + "startTime": "%s" + }] } }` - expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, now)) + expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, now, now)) patchIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch) f.run(getKey(r2, t)) diff --git a/rollout/canary.go b/rollout/canary.go index 1f0282d30c..dbcf0b4adb 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -119,8 +119,8 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { if currentStep.Pause == nil { return false } - if !rollout.Status.ControllerPause { - roCtx.PauseContext().AddControllerPause() + if !rollout.Status.ControllerPause && rollout.Status.PauseStartTime == nil { + roCtx.PauseContext().AddControllerPause(v1alpha1.CanaryPauseStep) } if currentStep.Pause.Duration == nil { return true @@ -208,7 +208,7 @@ func completedCurrentCanaryStep(roCtx *canaryContext) bool { return false } if currentStep.Pause != nil { - return completedPauseStep(r, *currentStep.Pause) + return completedPauseStep(roCtx, *currentStep.Pause) } if currentStep.SetWeight != nil && replicasetutil.AtDesiredReplicaCountsForCanary(r, roCtx.NewRS(), roCtx.StableRS(), roCtx.OlderRSs()) { logCtx.Info("Rollout has reached the desired state for the correct weight") @@ -256,7 +256,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error c.recorder.Event(r, corev1.EventTypeNormal, "SkipSteps", msg) } } - roCtx.PauseContext().RemoveControllerPause() + roCtx.PauseContext().ClearPauseReasons() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } @@ -272,7 +272,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error newStatus.CurrentStepIndex = &stepCount } - roCtx.PauseContext().RemoveControllerPause() + roCtx.PauseContext().ClearPauseReasons() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } @@ -285,7 +285,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error logCtx.Info("New RS has successfully progressed") newStatus.Canary.StableRS = newStatus.CurrentPodHash } - roCtx.PauseContext().RemoveControllerPause() + roCtx.PauseContext().ClearPauseReasons() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } @@ -303,15 +303,10 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error logCtx.Info("New RS has successfully progressed") newStatus.Canary.StableRS = newStatus.CurrentPodHash } - roCtx.PauseContext().RemoveControllerPause() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } - // reconcileCanaryPause will ensure we will requeue this rollout at the appropriate time - // if we are at a pause step with a duration. - c.reconcileCanaryPause(roCtx) - if completedCurrentCanaryStep(roCtx) { *currentStepIndex++ newStatus.CurrentStepIndex = currentStepIndex @@ -320,7 +315,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error } logCtx.Infof("Incrementing the Current Step Index to %d", *currentStepIndex) c.recorder.Eventf(r, corev1.EventTypeNormal, "SetStepIndex", "Set Step Index to %d", int(*currentStepIndex)) - roCtx.PauseContext().RemoveControllerPause() + roCtx.PauseContext().RemoveControllerPause(v1alpha1.CanaryPauseStep) newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } diff --git a/rollout/canary_test.go b/rollout/canary_test.go index 4005235268..c91446c72e 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -107,12 +107,17 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) { "status":{ "controllerPause": true, "pauseStartTime":"%s", + "pauseConditions":[{ + "reason": "%s", + "startTime": "%s" + }], "conditions": %s } }` conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) - expectedPatchWithoutObservedGen := fmt.Sprintf(expectedPatchTemplate, metav1.Now().UTC().Format(time.RFC3339), conditions) + now := metav1.Now().UTC().Format(time.RFC3339) + expectedPatchWithoutObservedGen := fmt.Sprintf(expectedPatchTemplate, now, v1alpha1.CanaryPauseStep, now, conditions) expectedPatch := calculatePatch(r2, expectedPatchWithoutObservedGen) assert.Equal(t, expectedPatch, patch) } @@ -776,11 +781,16 @@ func TestSyncRolloutsSetPauseStartTime(t *testing.T) { "status":{ "controllerPause": true, "pauseStartTime": "%s", - "conditions": %s + "conditions": %s, + "pauseConditions":[{ + "reason": "%s", + "startTime": "%s" + }] } }` - condtions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) - expectedPatch := fmt.Sprintf(expectedPatchWithoutTime, metav1.Now().UTC().Format(time.RFC3339), condtions) + conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) + now := metav1.Now().UTC().Format(time.RFC3339) + expectedPatch := fmt.Sprintf(expectedPatchWithoutTime, now, conditions, v1alpha1.CanaryPauseStep, now) index := f.expectPatchRolloutActionWithPatch(r2, expectedPatch) f.run(getKey(r2, t)) diff --git a/rollout/context.go b/rollout/context.go index 390ddd8a3c..01f5bb56d8 100644 --- a/rollout/context.go +++ b/rollout/context.go @@ -69,6 +69,7 @@ func newBlueGreenCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, olderRSs []* log: logCtx, controllerPause: r.Status.ControllerPause, pauseStartTime: r.Status.PauseStartTime, + pauseConditions: r.Status.PauseConditions, }, } } @@ -140,6 +141,7 @@ func newCanaryCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, stableRS *appsv log: logCtx, controllerPause: r.Status.ControllerPause, pauseStartTime: r.Status.PauseStartTime, + pauseConditions: r.Status.PauseConditions, }, } } diff --git a/rollout/controller_test.go b/rollout/controller_test.go index e9ab3e9521..363a9c4645 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -248,11 +248,27 @@ func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active string, a newRollout.Status.BlueGreen.PreviewSelector = preview cond, _ := newAvailableCondition(available) newRollout.Status.Conditions = append(newRollout.Status.Conditions, cond) + if pause { + now := metav1.Now() + cond := v1alpha1.PauseCondition{ + Reason: v1alpha1.BlueGreenPause, + StartTime: now, + } + newRollout.Status.PauseConditions = append(newRollout.Status.PauseConditions, cond) + } return newRollout } func updateCanaryRolloutStatus(r *v1alpha1.Rollout, stableRS string, availableReplicas, updatedReplicas, hpaReplicas int32, pause bool) *v1alpha1.Rollout { newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, availableReplicas, hpaReplicas, pause) newRollout.Status.Canary.StableRS = stableRS + if pause { + now := metav1.Now() + cond := v1alpha1.PauseCondition{ + Reason: v1alpha1.CanaryPauseStep, + StartTime: now, + } + newRollout.Status.PauseConditions = append(newRollout.Status.PauseConditions, cond) + } return newRollout } diff --git a/rollout/pause.go b/rollout/pause.go index bce1c0aa04..d947978b48 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -14,32 +14,64 @@ type pauseContext struct { log *log.Entry controllerPause bool pauseStartTime *metav1.Time + pauseConditions []v1alpha1.PauseCondition - addControllerPause bool - removeControllerPause bool + addPauseReasons []v1alpha1.PauseReason + removePauseReasons []v1alpha1.PauseReason + clearPauseReasons bool } -func (pCtx *pauseContext) AddControllerPause() { - pCtx.addControllerPause = true +func (pCtx *pauseContext) AddControllerPause(reason v1alpha1.PauseReason) { + pCtx.addPauseReasons = append(pCtx.addPauseReasons, reason) } -func (pCtx *pauseContext) RemoveControllerPause() { - pCtx.removeControllerPause = true +func (pCtx *pauseContext) RemoveControllerPause(reason v1alpha1.PauseReason) { + pCtx.removePauseReasons = append(pCtx.removePauseReasons, reason) +} +func (pCtx *pauseContext) ClearPauseReasons() { + pCtx.clearPauseReasons = true } func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus) { - if pCtx.removeControllerPause { + if pCtx.clearPauseReasons { return } + statusToRemove := map[v1alpha1.PauseReason]bool{} + for i := range pCtx.removePauseReasons { + statusToRemove[pCtx.removePauseReasons[i]] = true + } + + newPauseConditions := []v1alpha1.PauseCondition{} + pauseAlreadyExists := map[v1alpha1.PauseReason]bool{} + for _, cond := range pCtx.pauseConditions { + if remove := statusToRemove[cond.Reason]; !remove { + newPauseConditions = append(newPauseConditions, cond) + } + pauseAlreadyExists[cond.Reason] = true + } + + now := metav1.Now() + for i := range pCtx.addPauseReasons { + reason := pCtx.addPauseReasons[i] + if exists := pauseAlreadyExists[reason]; !exists { + pCtx.log.Infof("Adding pause reason %s with start time %s", reason, now.UTC().Format(time.RFC3339)) + cond := v1alpha1.PauseCondition{ + Reason: reason, + StartTime: now, + } + newPauseConditions = append(newPauseConditions, cond) + } + } pauseStartTime := pCtx.pauseStartTime paused := pCtx.controllerPause - if !paused { - pauseStartTime = nil + + if len(newPauseConditions) == 0 { + return } - if pCtx.addControllerPause { + + if len(pCtx.addPauseReasons) > 0 { if pauseStartTime == nil { - now := metav1.Now() pCtx.log.Infof("Setting PauseStartTime to %s", now.UTC().Format(time.RFC3339)) pauseStartTime = &now paused = true @@ -48,36 +80,12 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus newStatus.ControllerPause = paused newStatus.PauseStartTime = pauseStartTime - // newStatus.PauseReason + newStatus.PauseConditions = newPauseConditions } -// calculatePauseStatus determines if the rollout should be paused by the controller. -func calculatePauseStatus(roCtx rolloutContext) (*metav1.Time, bool) { +func completedPauseStep(roCtx *canaryContext, pause v1alpha1.RolloutPause) bool { rollout := roCtx.Rollout() logCtx := roCtx.Log() - pauseCtx := roCtx.PauseContext() - pauseStartTime := rollout.Status.PauseStartTime - paused := rollout.Status.ControllerPause - if !paused { - pauseStartTime = nil - } - if pauseCtx.addControllerPause { - if pauseStartTime == nil { - now := metav1.Now() - logCtx.Infof("Setting PauseStartTime to %s", now.UTC().Format(time.RFC3339)) - pauseStartTime = &now - paused = true - } - } - if pauseCtx.removeControllerPause { - return nil, false - } - - return pauseStartTime, paused -} - -func completedPauseStep(rollout *v1alpha1.Rollout, pause v1alpha1.RolloutPause) bool { - logCtx := logutil.WithRollout(rollout) if pause.Duration != nil { now := metav1.Now() if rollout.Status.PauseStartTime != nil { From 2d66918d93d949667f93b93b15bbe49a092fb92b Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Sun, 20 Oct 2019 09:48:15 -0700 Subject: [PATCH 06/16] Migrate to PauseConditons and ControllerSetPause --- manifests/crds/rollout-crd.yaml | 2 ++ manifests/install.yaml | 2 ++ manifests/namespace-install.yaml | 2 ++ .../rollouts/v1alpha1/openapi_generated.go | 7 +++++++ pkg/apis/rollouts/v1alpha1/types.go | 2 ++ rollout/analysis.go | 2 +- rollout/analysis_test.go | 3 ++- rollout/bluegreen.go | 4 ++-- rollout/bluegreen_test.go | 12 +++++++++--- rollout/canary.go | 4 ++-- rollout/canary_test.go | 10 ++++++++-- rollout/context.go | 18 ++++++++++-------- rollout/controller.go | 2 +- rollout/controller_test.go | 10 +++++++--- rollout/pause.go | 13 +++++++++---- rollout/sync.go | 11 ++++++----- utils/replicaset/replicaset.go | 2 +- 17 files changed, 73 insertions(+), 33 deletions(-) diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index d3985f8fda..9f9f20f96b 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -2682,6 +2682,8 @@ spec: type: array controllerPause: type: boolean + controllerSetPause: + type: boolean currentPodHash: type: string currentStepHash: diff --git a/manifests/install.yaml b/manifests/install.yaml index c6269b85dc..215c4320e5 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -10356,6 +10356,8 @@ spec: type: array controllerPause: type: boolean + controllerSetPause: + type: boolean currentPodHash: type: string currentStepHash: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 79d35f04b5..81f05e8cc4 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -10356,6 +10356,8 @@ spec: type: array controllerPause: type: boolean + controllerSetPause: + type: boolean currentPodHash: type: string currentStepHash: diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index b99f876d70..5645883a97 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -1684,6 +1684,13 @@ func schema_pkg_apis_rollouts_v1alpha1_RolloutStatus(ref common.ReferenceCallbac SchemaProps: spec.SchemaProps{ Description: "RolloutStatus is the status for a Rollout resource", Properties: map[string]spec.Schema{ + "controllerSetPause": { + SchemaProps: spec.SchemaProps{ + Description: "ControllerSetPause indicates the controller paused the rollout (i.e. pause step or inconclusive run)", + Type: []string{"boolean"}, + Format: "", + }, + }, "pauseConditions": { SchemaProps: spec.SchemaProps{ Description: "PauseConditions indicates why the rollout is currently paused", diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 2ad99c6311..396224d8ce 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -274,6 +274,8 @@ type PauseCondition struct { // RolloutStatus is the status for a Rollout resource type RolloutStatus struct { + //ControllerSetPause indicates the controller paused the rollout (i.e. pause step or inconclusive run) + ControllerSetPause bool `json:"controllerSetPause,omitempty"` // PauseConditions indicates why the rollout is currently paused PauseConditions []PauseCondition `json:"pauseConditions,omitempty"` //ControllerPause indicates the controller paused the rollout (i.e. pause step or inconclusive run) diff --git a/rollout/analysis.go b/rollout/analysis.go index 69055b1475..4f8734ea78 100644 --- a/rollout/analysis.go +++ b/rollout/analysis.go @@ -44,7 +44,7 @@ func (c *RolloutController) getAnalysisRunsForRollout(rollout *v1alpha1.Rollout) func (c *RolloutController) reconcileAnalysisRuns(roCtx *canaryContext) error { rollout := roCtx.Rollout() otherArs := roCtx.OtherAnalysisRuns() - if rollout.Status.ControllerPause { + if len(rollout.Status.PauseConditions) > 0 { return nil } newCurrentAnalysisRuns := []*v1alpha1.AnalysisRun{} diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index e503a95455..cffb01f3e6 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -552,7 +552,8 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { "pauseConditions": [{ "reason": "InconclusiveAnalysisRun", "startTime": "%s" - }] + }], + "controllerSetPause": true } }` condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index b5e1ddcdf4..4c24b5928c 100644 --- a/rollout/bluegreen.go +++ b/rollout/bluegreen.go @@ -148,7 +148,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev return false } // If the rollout is not paused and the active service is not point at the newRS, we should pause the rollout. - if !rollout.Status.ControllerPause && rollout.Status.PauseStartTime == nil && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { + if len(rollout.Status.PauseConditions) == 0 && !rollout.Status.ControllerSetPause && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { roCtx.PauseContext().AddControllerPause(v1alpha1.BlueGreenPause) return true } @@ -165,7 +165,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev } - return rollout.Status.ControllerPause && pauseStartTime != nil + return len(rollout.Status.PauseConditions) > 0 && rollout.Status.ControllerSetPause } // scaleDownOldReplicaSetsForBlueGreen scales down old replica sets when rollout strategy is "Blue Green". diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 7fa4473d47..a4071cfe10 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -163,7 +163,8 @@ func TestBlueGreenHandlePause(t *testing.T) { "pauseConditions": [{ "reason": "BlueGreenPause", "startTime": "%s" - }] + }], + "controllerSetPause": true } }` now := metav1.Now().UTC().Format(time.RFC3339) @@ -299,6 +300,7 @@ func TestBlueGreenHandlePause(t *testing.T) { now := metav1.Now() before := metav1.NewTime(now.Add(-1 * time.Minute)) r2.Status.PauseStartTime = &before + r2.Status.ControllerSetPause = true pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2) conditions.SetRolloutCondition(&r2.Status, pausedCondition) @@ -317,7 +319,8 @@ func TestBlueGreenHandlePause(t *testing.T) { "status": { "controllerPause": null, "pauseStartTime": null, - "pauseConditions": null + "pauseConditions": null, + "controllerSetPause": null } }` expectedPatch := calculatePatch(r2, expectedPatchWithoutSubs) @@ -411,7 +414,8 @@ func TestBlueGreenHandlePause(t *testing.T) { "pauseConditions": [{ "reason":"BlueGreenPause", "startTime": "%s" - }] + }], + "controllerSetPause": true } }` expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, now, now)) @@ -481,6 +485,7 @@ func TestBlueGreenHandlePause(t *testing.T) { r2.Spec.Strategy.BlueGreen.ScaleDownDelaySeconds = pointer.Int32Ptr(10) r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, 1, 1, 2, 1, false, true) + r2.Status.ControllerSetPause = true now := metav1.Now() r2.Status.PauseStartTime = &now pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2) @@ -521,6 +526,7 @@ func TestBlueGreenHandlePause(t *testing.T) { "activeSelector": "%s" }, "pauseStartTime": null, + "controllerSetPause":null, "conditions": %s, "selector": "%s" } diff --git a/rollout/canary.go b/rollout/canary.go index dbcf0b4adb..542eb1bde7 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -119,13 +119,13 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { if currentStep.Pause == nil { return false } - if !rollout.Status.ControllerPause && rollout.Status.PauseStartTime == nil { + if len(rollout.Status.PauseConditions) == 0 && !rollout.Status.ControllerSetPause { roCtx.PauseContext().AddControllerPause(v1alpha1.CanaryPauseStep) } if currentStep.Pause.Duration == nil { return true } - if rollout.Status.PauseStartTime == nil { + if !rollout.Status.ControllerSetPause { return true } c.checkEnqueueRolloutDuringWait(rollout, *rollout.Status.PauseStartTime, *currentStep.Pause.Duration) diff --git a/rollout/canary_test.go b/rollout/canary_test.go index c91446c72e..e9cff7d230 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -111,7 +111,8 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) { "reason": "%s", "startTime": "%s" }], - "conditions": %s + "conditions": %s, + "controllerSetPause": true } }` @@ -214,6 +215,7 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { r2.Status.AvailableReplicas = 10 now := metav1.Now() r2.Status.PauseStartTime = &now + r2.Status.ControllerSetPause = true f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -226,6 +228,7 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { expectedPatchTemplate := `{ "status":{ "pauseStartTime": null, + "controllerSetPause": null, "conditions" : %s, "currentStepIndex": 1 } @@ -785,7 +788,8 @@ func TestSyncRolloutsSetPauseStartTime(t *testing.T) { "pauseConditions":[{ "reason": "%s", "startTime": "%s" - }] + }], + "controllerSetPause": true } }` conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) @@ -907,6 +911,7 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { earlier := metav1.Now() earlier.Time = earlier.Add(-10 * time.Second) r2.Status.PauseStartTime = &earlier + r2.Status.ControllerSetPause = true f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -918,6 +923,7 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { expectedPatch := `{ "status":{ "pauseStartTime": null, + "controllerSetPause": null, "currentStepIndex":2, "conditions": %s } diff --git a/rollout/context.go b/rollout/context.go index 01f5bb56d8..45e569ac8d 100644 --- a/rollout/context.go +++ b/rollout/context.go @@ -66,10 +66,11 @@ func newBlueGreenCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, olderRSs []* allRSs: allRSs, pauseContext: &pauseContext{ - log: logCtx, - controllerPause: r.Status.ControllerPause, - pauseStartTime: r.Status.PauseStartTime, - pauseConditions: r.Status.PauseConditions, + log: logCtx, + controllerPause: r.Status.ControllerPause, + pauseStartTime: r.Status.PauseStartTime, + pauseConditions: r.Status.PauseConditions, + controllerSetPause: r.Status.ControllerSetPause, }, } } @@ -138,10 +139,11 @@ func newCanaryCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, stableRS *appsv otherExs: otherExs, pauseContext: &pauseContext{ - log: logCtx, - controllerPause: r.Status.ControllerPause, - pauseStartTime: r.Status.PauseStartTime, - pauseConditions: r.Status.PauseConditions, + log: logCtx, + controllerPause: r.Status.ControllerPause, + pauseStartTime: r.Status.PauseStartTime, + pauseConditions: r.Status.PauseConditions, + controllerSetPause: r.Status.ControllerSetPause, }, } } diff --git a/rollout/controller.go b/rollout/controller.go index 4a36f26661..a3cde1be10 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -273,7 +273,7 @@ func (c *RolloutController) syncHandler(key string) error { return err } - if rollout.Status.ControllerPause || isScalingEvent { + if len(rollout.Status.PauseConditions) > 0 || isScalingEvent { return c.syncReplicasOnly(r, rsList, isScalingEvent) } diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 363a9c4645..b5a17cd015 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -254,6 +254,7 @@ func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active string, a Reason: v1alpha1.BlueGreenPause, StartTime: now, } + newRollout.Status.ControllerSetPause = true newRollout.Status.PauseConditions = append(newRollout.Status.PauseConditions, cond) } return newRollout @@ -267,6 +268,7 @@ func updateCanaryRolloutStatus(r *v1alpha1.Rollout, stableRS string, availableRe Reason: v1alpha1.CanaryPauseStep, StartTime: now, } + newRollout.Status.ControllerSetPause = true newRollout.Status.PauseConditions = append(newRollout.Status.PauseConditions, cond) } return newRollout @@ -894,9 +896,11 @@ func TestRequeueStuckRollout(t *testing.T) { Replicas: pointer.Int32Ptr(0), ProgressDeadlineSeconds: progessDeadlineSeconds, }, - Status: v1alpha1.RolloutStatus{ - ControllerPause: rolloutPaused, - }, + } + if rolloutPaused { + r.Status.PauseConditions = []v1alpha1.PauseCondition{{ + Reason: v1alpha1.BlueGreenPause, + }} } if rolloutCompleted { r.Status.ObservedGeneration = conditions.ComputeGenerationHash(r.Spec) diff --git a/rollout/pause.go b/rollout/pause.go index d947978b48..fbf4cfeb7c 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -11,10 +11,11 @@ import ( ) type pauseContext struct { - log *log.Entry - controllerPause bool - pauseStartTime *metav1.Time - pauseConditions []v1alpha1.PauseCondition + log *log.Entry + controllerPause bool + controllerSetPause bool + pauseStartTime *metav1.Time + pauseConditions []v1alpha1.PauseCondition addPauseReasons []v1alpha1.PauseReason removePauseReasons []v1alpha1.PauseReason @@ -36,6 +37,8 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus if pCtx.clearPauseReasons { return } + + controllerPause := pCtx.controllerSetPause statusToRemove := map[v1alpha1.PauseReason]bool{} for i := range pCtx.removePauseReasons { statusToRemove[pCtx.removePauseReasons[i]] = true @@ -60,6 +63,7 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus StartTime: now, } newPauseConditions = append(newPauseConditions, cond) + controllerPause = true } } @@ -78,6 +82,7 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus } } + newStatus.ControllerSetPause = controllerPause newStatus.ControllerPause = paused newStatus.PauseStartTime = pauseStartTime newStatus.PauseConditions = newPauseConditions diff --git a/rollout/sync.go b/rollout/sync.go index 06aedb9468..bddc6d39d0 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -207,7 +207,8 @@ func (c *RolloutController) getNewReplicaSet(rollout *v1alpha1.Rollout, rsList, // syncReplicasOnly is responsible for reconciling rollouts on scaling events. func (c *RolloutController) syncReplicasOnly(r *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet, isScaling bool) error { logCtx := logutil.WithRollout(r) - logCtx.Infof("Syncing replicas only (paused: %v, isScaling: %v)", r.Status.ControllerPause, isScaling) + isPaused := len(r.Status.PauseConditions) > 0 + logCtx.Infof("Syncing replicas only (paused: %v, isScaling: %v)", isPaused, isScaling) newRS, oldRSs, err := c.getAllReplicaSetsAndSyncRevision(r, rsList, false) if err != nil { return err @@ -424,9 +425,9 @@ func (c *RolloutController) checkPausedConditions(r *v1alpha1.Rollout) error { pausedCondExists := cond != nil && cond.Reason == conditions.PausedRolloutReason var updatedConditon *v1alpha1.RolloutCondition - if r.Status.ControllerPause && !pausedCondExists { + if len(r.Status.PauseConditions) > 0 && !pausedCondExists { updatedConditon = conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.PausedRolloutReason, conditions.PausedRolloutMessage) - } else if !r.Status.ControllerPause && pausedCondExists { + } else if len(r.Status.PauseConditions) == 0 && pausedCondExists { updatedConditon = conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionUnknown, conditions.ResumedRolloutReason, conditions.ResumeRolloutMessage) } @@ -473,7 +474,7 @@ func (c *RolloutController) calculateRolloutConditions(roCtx rolloutContext, new r := roCtx.Rollout() allRSs := roCtx.AllRSs() newRS := roCtx.NewRS() - if r.Status.ControllerPause { + if len(r.Status.PauseConditions) > 0 { return newStatus } @@ -618,7 +619,7 @@ func (c *RolloutController) requeueStuckRollout(r *v1alpha1.Rollout, newStatus v return time.Duration(-1) } // No need to estimate progress if the rollout is complete or already timed out. - if conditions.RolloutComplete(r, &newStatus) || currentCond.Reason == conditions.TimedOutReason || r.Status.ControllerPause { + if conditions.RolloutComplete(r, &newStatus) || currentCond.Reason == conditions.TimedOutReason || len(r.Status.PauseConditions) > 0 { return time.Duration(-1) } // If there is no sign of progress at this point then there is a high chance that the diff --git a/utils/replicaset/replicaset.go b/utils/replicaset/replicaset.go index f5b6a7d3f9..e6a9ce44ab 100644 --- a/utils/replicaset/replicaset.go +++ b/utils/replicaset/replicaset.go @@ -85,7 +85,7 @@ func NewRSNewReplicas(rollout *v1alpha1.Rollout, allRSs []*appsv1.ReplicaSet, ne if newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] != rollout.Status.CurrentPodHash { return *rollout.Spec.Strategy.BlueGreen.PreviewReplicaCount, nil } - if !rollout.Status.ControllerPause && rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint { + if len(rollout.Status.PauseConditions) == 0 && rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint { return defaults.GetRolloutReplicasOrDefault(rollout), nil } return *rollout.Spec.Strategy.BlueGreen.PreviewReplicaCount, nil From f9c151acbe3595f60ce905fd2fa949ea20c9261e Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Sun, 20 Oct 2019 11:37:54 -0700 Subject: [PATCH 07/16] Use PauseConditions instead of pauseStartTime --- rollout/bluegreen.go | 13 ++++++------ rollout/bluegreen_test.go | 1 + rollout/canary.go | 15 +++++++------- rollout/canary_test.go | 20 +++++++++++++------ rollout/context.go | 14 ++++--------- rollout/pause.go | 42 +++++++++++++++++++++++---------------- 6 files changed, 59 insertions(+), 46 deletions(-) diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index 4c24b5928c..0778c0db51 100644 --- a/rollout/bluegreen.go +++ b/rollout/bluegreen.go @@ -147,17 +147,18 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev if _, ok := activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]; !ok { return false } + + cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.BlueGreenPause) // If the rollout is not paused and the active service is not point at the newRS, we should pause the rollout. - if len(rollout.Status.PauseConditions) == 0 && !rollout.Status.ControllerSetPause && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { + if cond == nil && !rollout.Status.ControllerSetPause && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { roCtx.PauseContext().AddControllerPause(v1alpha1.BlueGreenPause) return true } - pauseStartTime := rollout.Status.PauseStartTime autoPromoteActiveServiceDelaySeconds := rollout.Spec.Strategy.BlueGreen.AutoPromotionSeconds - if autoPromoteActiveServiceDelaySeconds != nil && pauseStartTime != nil { - c.checkEnqueueRolloutDuringWait(rollout, *pauseStartTime, *autoPromoteActiveServiceDelaySeconds) - switchDeadline := pauseStartTime.Add(time.Duration(*autoPromoteActiveServiceDelaySeconds) * time.Second) + if autoPromoteActiveServiceDelaySeconds != nil && cond != nil { + c.checkEnqueueRolloutDuringWait(rollout, cond.StartTime, *autoPromoteActiveServiceDelaySeconds) + switchDeadline := cond.StartTime.Add(time.Duration(*autoPromoteActiveServiceDelaySeconds) * time.Second) now := metav1.Now() if now.After(switchDeadline) { roCtx.PauseContext().RemoveControllerPause(v1alpha1.BlueGreenPause) @@ -165,7 +166,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev } - return len(rollout.Status.PauseConditions) > 0 && rollout.Status.ControllerSetPause + return cond != nil && rollout.Status.ControllerSetPause } // scaleDownOldReplicaSetsForBlueGreen scales down old replica sets when rollout strategy is "Blue Green". diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index a4071cfe10..b27837cb18 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -300,6 +300,7 @@ func TestBlueGreenHandlePause(t *testing.T) { now := metav1.Now() before := metav1.NewTime(now.Add(-1 * time.Minute)) r2.Status.PauseStartTime = &before + r2.Status.PauseConditions[0].StartTime = before r2.Status.ControllerSetPause = true pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2) conditions.SetRolloutCondition(&r2.Status, pausedCondition) diff --git a/rollout/canary.go b/rollout/canary.go index 542eb1bde7..623e5ed81a 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -119,16 +119,17 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { if currentStep.Pause == nil { return false } - if len(rollout.Status.PauseConditions) == 0 && !rollout.Status.ControllerSetPause { - roCtx.PauseContext().AddControllerPause(v1alpha1.CanaryPauseStep) - } - if currentStep.Pause.Duration == nil { + cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.CanaryPauseStep) + if cond == nil { + if !rollout.Status.ControllerSetPause { + roCtx.PauseContext().AddControllerPause(v1alpha1.CanaryPauseStep) + } return true } - if !rollout.Status.ControllerSetPause { + if currentStep.Pause.Duration == nil { return true } - c.checkEnqueueRolloutDuringWait(rollout, *rollout.Status.PauseStartTime, *currentStep.Pause.Duration) + c.checkEnqueueRolloutDuringWait(rollout, cond.StartTime, *currentStep.Pause.Duration) return true } @@ -208,7 +209,7 @@ func completedCurrentCanaryStep(roCtx *canaryContext) bool { return false } if currentStep.Pause != nil { - return completedPauseStep(roCtx, *currentStep.Pause) + return roCtx.PauseContext().CompletedPauseStep(*currentStep.Pause) } if currentStep.SetWeight != nil && replicasetutil.AtDesiredReplicaCountsForCanary(r, roCtx.NewRS(), roCtx.StableRS(), roCtx.OlderRSs()) { logCtx.Info("Rollout has reached the desired state for the correct weight") diff --git a/rollout/canary_test.go b/rollout/canary_test.go index e9cff7d230..c5c02e7707 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -908,11 +908,17 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 10, 1, 10, false) + pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2) + conditions.SetRolloutCondition(&r2.Status, pausedCondition) + earlier := metav1.Now() earlier.Time = earlier.Add(-10 * time.Second) r2.Status.PauseStartTime = &earlier r2.Status.ControllerSetPause = true - + r2.Status.PauseConditions = []v1alpha1.PauseCondition{{ + Reason: v1alpha1.CanaryPauseStep, + StartTime: earlier, + }} f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -924,12 +930,11 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { "status":{ "pauseStartTime": null, "controllerSetPause": null, - "currentStepIndex":2, - "conditions": %s + "pauseConditions": null, + "currentStepIndex":2 } }` - newCondtions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, newCondtions)), patch) + assert.Equal(t, calculatePatch(r2, expectedPatch), patch) } func TestCanaryRolloutStatusHPAStatusFields(t *testing.T) { @@ -1085,7 +1090,10 @@ func TestResumeRolloutAfterPauseDuration(t *testing.T) { overAMinuteAgo := metav1.Time{Time: time.Now().Add(-61 * time.Second)} r1.Status.ObservedGeneration = conditions.ComputeGenerationHash(r1.Spec) r1.Status.PauseStartTime = &overAMinuteAgo - + r1.Status.PauseConditions = []v1alpha1.PauseCondition{{ + Reason: v1alpha1.CanaryPauseStep, + StartTime: overAMinuteAgo, + }} f.kubeobjects = append(f.kubeobjects, rs1) f.replicaSetLister = append(f.replicaSetLister, rs1) f.rolloutLister = append(f.rolloutLister, r1) diff --git a/rollout/context.go b/rollout/context.go index 45e569ac8d..c38aacf552 100644 --- a/rollout/context.go +++ b/rollout/context.go @@ -66,11 +66,8 @@ func newBlueGreenCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, olderRSs []* allRSs: allRSs, pauseContext: &pauseContext{ - log: logCtx, - controllerPause: r.Status.ControllerPause, - pauseStartTime: r.Status.PauseStartTime, - pauseConditions: r.Status.PauseConditions, - controllerSetPause: r.Status.ControllerSetPause, + rollout: r, + log: logCtx, }, } } @@ -139,11 +136,8 @@ func newCanaryCtx(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet, stableRS *appsv otherExs: otherExs, pauseContext: &pauseContext{ - log: logCtx, - controllerPause: r.Status.ControllerPause, - pauseStartTime: r.Status.PauseStartTime, - pauseConditions: r.Status.PauseConditions, - controllerSetPause: r.Status.ControllerSetPause, + rollout: r, + log: logCtx, }, } } diff --git a/rollout/pause.go b/rollout/pause.go index fbf4cfeb7c..bd8209eff1 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -11,11 +11,8 @@ import ( ) type pauseContext struct { - log *log.Entry - controllerPause bool - controllerSetPause bool - pauseStartTime *metav1.Time - pauseConditions []v1alpha1.PauseCondition + rollout *v1alpha1.Rollout + log *log.Entry addPauseReasons []v1alpha1.PauseReason removePauseReasons []v1alpha1.PauseReason @@ -38,7 +35,7 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus return } - controllerPause := pCtx.controllerSetPause + controllerPause := pCtx.rollout.Status.ControllerSetPause statusToRemove := map[v1alpha1.PauseReason]bool{} for i := range pCtx.removePauseReasons { statusToRemove[pCtx.removePauseReasons[i]] = true @@ -46,7 +43,7 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus newPauseConditions := []v1alpha1.PauseCondition{} pauseAlreadyExists := map[v1alpha1.PauseReason]bool{} - for _, cond := range pCtx.pauseConditions { + for _, cond := range pCtx.rollout.Status.PauseConditions { if remove := statusToRemove[cond.Reason]; !remove { newPauseConditions = append(newPauseConditions, cond) } @@ -67,8 +64,8 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus } } - pauseStartTime := pCtx.pauseStartTime - paused := pCtx.controllerPause + pauseStartTime := pCtx.rollout.Status.PauseStartTime + paused := pCtx.rollout.Status.ControllerPause if len(newPauseConditions) == 0 { return @@ -88,20 +85,31 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus newStatus.PauseConditions = newPauseConditions } -func completedPauseStep(roCtx *canaryContext, pause v1alpha1.RolloutPause) bool { - rollout := roCtx.Rollout() - logCtx := roCtx.Log() +func (pCtx *pauseContext) GetPauseCondition(reason v1alpha1.PauseReason) *v1alpha1.PauseCondition { + for i := range pCtx.rollout.Status.PauseConditions { + cond := pCtx.rollout.Status.PauseConditions[i] + if cond.Reason == reason { + return &cond + } + } + return nil +} + +func (pCtx *pauseContext) CompletedPauseStep(pause v1alpha1.RolloutPause) bool { + rollout := pCtx.rollout + pauseCondition := pCtx.GetPauseCondition(v1alpha1.CanaryPauseStep) + if pause.Duration != nil { now := metav1.Now() - if rollout.Status.PauseStartTime != nil { - expiredTime := rollout.Status.PauseStartTime.Add(time.Duration(*pause.Duration) * time.Second) + if pauseCondition != nil { + expiredTime := pauseCondition.StartTime.Add(time.Duration(*pause.Duration) * time.Second) if now.After(expiredTime) { - logCtx.Info("Rollout has waited the duration of the pause step") + pCtx.log.Info("Rollout has waited the duration of the pause step") return true } } - } else if rollout.Status.PauseStartTime != nil && !rollout.Status.ControllerPause { - logCtx.Info("Rollout has been unpaused") + } else if rollout.Status.ControllerSetPause && pauseCondition == nil { + pCtx.log.Info("Rollout has been unpaused") return true } return false From 522ccc1d0433a69c092a68e48e77380bd50438f1 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Sun, 20 Oct 2019 11:42:48 -0700 Subject: [PATCH 08/16] Remove use of controllerPause --- rollout/analysis_test.go | 1 - rollout/bluegreen_test.go | 3 --- rollout/canary_test.go | 8 +++----- rollout/controller_test.go | 2 +- rollout/pause.go | 3 --- 5 files changed, 4 insertions(+), 13 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index cffb01f3e6..1d3c9b092b 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -543,7 +543,6 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { now := metav1.Now().UTC().Format(time.RFC3339) expectedPatch := `{ "status": { - "controllerPause": true, "conditions": %s, "canary": { "currentStepAnalysisRun": null diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index b27837cb18..e36b19b18c 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -158,7 +158,6 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatch := `{ "status": { - "controllerPause": true, "pauseStartTime": "%s", "pauseConditions": [{ "reason": "BlueGreenPause", @@ -318,7 +317,6 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatchWithoutSubs := `{ "status": { - "controllerPause": null, "pauseStartTime": null, "pauseConditions": null, "controllerSetPause": null @@ -410,7 +408,6 @@ func TestBlueGreenHandlePause(t *testing.T) { now := metav1.Now().UTC().Format(time.RFC3339) expectedPatchWithoutSubs := `{ "status": { - "controllerPause": true, "pauseStartTime": "%s", "pauseConditions": [{ "reason":"BlueGreenPause", diff --git a/rollout/canary_test.go b/rollout/canary_test.go index c5c02e7707..ed32187ce6 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -105,7 +105,6 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) { patch := f.getPatchedRollout(patchIndex) expectedPatchTemplate := `{ "status":{ - "controllerPause": true, "pauseStartTime":"%s", "pauseConditions":[{ "reason": "%s", @@ -782,7 +781,6 @@ func TestSyncRolloutsSetPauseStartTime(t *testing.T) { expectedPatchWithoutTime := `{ "status":{ - "controllerPause": true, "pauseStartTime": "%s", "conditions": %s, "pauseConditions":[{ @@ -1113,7 +1111,7 @@ func TestResumeRolloutAfterPauseDuration(t *testing.T) { pauseStartTime, ok := status["pauseStartTime"] assert.True(t, ok) assert.Equal(t, nil, pauseStartTime) - controllerPause, ok := status["controllerPause"] - assert.True(t, ok) - assert.Nil(t, controllerPause) + // controllerPause, ok := status["controllerPause"] + // assert.True(t, ok) + // assert.Nil(t, controllerPause) } diff --git a/rollout/controller_test.go b/rollout/controller_test.go index b5a17cd015..ac0fc7e90c 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -281,7 +281,7 @@ func updateBaseRolloutStatus(r *v1alpha1.Rollout, availableReplicas, updatedRepl newRollout.Status.UpdatedReplicas = updatedReplicas newRollout.Status.HPAReplicas = hpaReplicas if pause { - newRollout.Status.ControllerPause = pause + // newRollout.Status.ControllerPause = pause now := metav1.Now() newRollout.Status.PauseStartTime = &now } diff --git a/rollout/pause.go b/rollout/pause.go index bd8209eff1..e909da1bf3 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -65,7 +65,6 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus } pauseStartTime := pCtx.rollout.Status.PauseStartTime - paused := pCtx.rollout.Status.ControllerPause if len(newPauseConditions) == 0 { return @@ -75,12 +74,10 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus if pauseStartTime == nil { pCtx.log.Infof("Setting PauseStartTime to %s", now.UTC().Format(time.RFC3339)) pauseStartTime = &now - paused = true } } newStatus.ControllerSetPause = controllerPause - newStatus.ControllerPause = paused newStatus.PauseStartTime = pauseStartTime newStatus.PauseConditions = newPauseConditions } From 69fdedb506888b5045f7ccf672eb55c1dac322fd Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Sun, 20 Oct 2019 11:52:29 -0700 Subject: [PATCH 09/16] Stop using pauseStartTime --- rollout/analysis_test.go | 3 +- rollout/bluegreen_test.go | 11 ++----- rollout/canary_test.go | 61 +------------------------------------- rollout/controller_test.go | 11 ++----- rollout/pause.go | 11 ------- 5 files changed, 7 insertions(+), 90 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index 1d3c9b092b..14bc9ae508 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -547,7 +547,6 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { "canary": { "currentStepAnalysisRun": null }, - "pauseStartTime": "%s", "pauseConditions": [{ "reason": "InconclusiveAnalysisRun", "startTime": "%s" @@ -557,7 +556,7 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { }` condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, now, now)), patch) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, now)), patch) } func TestErrorConditionAfterErrorAnalysisRun(t *testing.T) { diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index e36b19b18c..36bfeb2530 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -158,7 +158,6 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatch := `{ "status": { - "pauseStartTime": "%s", "pauseConditions": [{ "reason": "BlueGreenPause", "startTime": "%s" @@ -167,7 +166,7 @@ func TestBlueGreenHandlePause(t *testing.T) { } }` now := metav1.Now().UTC().Format(time.RFC3339) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now, now)), patch) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now)), patch) }) @@ -298,7 +297,6 @@ func TestBlueGreenHandlePause(t *testing.T) { r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, 1, 1, 2, 1, true, true) now := metav1.Now() before := metav1.NewTime(now.Add(-1 * time.Minute)) - r2.Status.PauseStartTime = &before r2.Status.PauseConditions[0].StartTime = before r2.Status.ControllerSetPause = true pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2) @@ -317,7 +315,6 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatchWithoutSubs := `{ "status": { - "pauseStartTime": null, "pauseConditions": null, "controllerSetPause": null } @@ -408,7 +405,6 @@ func TestBlueGreenHandlePause(t *testing.T) { now := metav1.Now().UTC().Format(time.RFC3339) expectedPatchWithoutSubs := `{ "status": { - "pauseStartTime": "%s", "pauseConditions": [{ "reason":"BlueGreenPause", "startTime": "%s" @@ -416,7 +412,7 @@ func TestBlueGreenHandlePause(t *testing.T) { "controllerSetPause": true } }` - expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, now, now)) + expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, now)) patchIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch) f.run(getKey(r2, t)) @@ -484,8 +480,6 @@ func TestBlueGreenHandlePause(t *testing.T) { r2.Spec.Strategy.BlueGreen.ScaleDownDelaySeconds = pointer.Int32Ptr(10) r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, 1, 1, 2, 1, false, true) r2.Status.ControllerSetPause = true - now := metav1.Now() - r2.Status.PauseStartTime = &now pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2) conditions.SetRolloutCondition(&r2.Status, pausedCondition) @@ -523,7 +517,6 @@ func TestBlueGreenHandlePause(t *testing.T) { "blueGreen": { "activeSelector": "%s" }, - "pauseStartTime": null, "controllerSetPause":null, "conditions": %s, "selector": "%s" diff --git a/rollout/canary_test.go b/rollout/canary_test.go index ed32187ce6..3332b5fbdc 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -105,7 +105,6 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) { patch := f.getPatchedRollout(patchIndex) expectedPatchTemplate := `{ "status":{ - "pauseStartTime":"%s", "pauseConditions":[{ "reason": "%s", "startTime": "%s" @@ -117,7 +116,7 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) { conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) now := metav1.Now().UTC().Format(time.RFC3339) - expectedPatchWithoutObservedGen := fmt.Sprintf(expectedPatchTemplate, now, v1alpha1.CanaryPauseStep, now, conditions) + expectedPatchWithoutObservedGen := fmt.Sprintf(expectedPatchTemplate, v1alpha1.CanaryPauseStep, now, conditions) expectedPatch := calculatePatch(r2, expectedPatchWithoutObservedGen) assert.Equal(t, expectedPatch, patch) } @@ -212,8 +211,6 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 10, 0, 10, false) r2.Status.AvailableReplicas = 10 - now := metav1.Now() - r2.Status.PauseStartTime = &now r2.Status.ControllerSetPause = true f.rolloutLister = append(f.rolloutLister, r2) @@ -226,7 +223,6 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { patch := f.getPatchedRollout(patchIndex) expectedPatchTemplate := `{ "status":{ - "pauseStartTime": null, "controllerSetPause": null, "conditions" : %s, "currentStepIndex": 1 @@ -752,55 +748,6 @@ func TestCanaryRolloutIncrementStepIfSetWeightsAreCorrect(t *testing.T) { assert.Equal(t, calculatePatch(r3, fmt.Sprintf(expectedPatch, newConditions)), patch) } -func TestSyncRolloutsSetPauseStartTime(t *testing.T) { - f := newFixture(t) - defer f.Close() - - steps := []v1alpha1.CanaryStep{ - { - SetWeight: int32Ptr(10), - }, { - Pause: &v1alpha1.RolloutPause{ - Duration: int32Ptr(10), - }, - }, - } - r1 := newCanaryRollout("foo", 10, nil, steps, int32Ptr(1), intstr.FromInt(1), intstr.FromInt(0)) - r2 := bumpVersion(r1) - - rs1 := newReplicaSetWithStatus(r1, 9, 9) - rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - rs2 := newReplicaSetWithStatus(r2, 1, 1) - - f.kubeobjects = append(f.kubeobjects, rs1, rs2) - f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) - - r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 10, 1, 10, false) - f.rolloutLister = append(f.rolloutLister, r2) - f.objects = append(f.objects, r2) - - expectedPatchWithoutTime := `{ - "status":{ - "pauseStartTime": "%s", - "conditions": %s, - "pauseConditions":[{ - "reason": "%s", - "startTime": "%s" - }], - "controllerSetPause": true - } - }` - conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) - now := metav1.Now().UTC().Format(time.RFC3339) - expectedPatch := fmt.Sprintf(expectedPatchWithoutTime, now, conditions, v1alpha1.CanaryPauseStep, now) - - index := f.expectPatchRolloutActionWithPatch(r2, expectedPatch) - f.run(getKey(r2, t)) - - patch := f.getPatchedRollout(index) - assert.Equal(t, calculatePatch(r2, expectedPatch), patch) -} - func TestSyncRolloutWaitAddToQueue(t *testing.T) { f := newFixture(t) defer f.Close() @@ -911,7 +858,6 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { earlier := metav1.Now() earlier.Time = earlier.Add(-10 * time.Second) - r2.Status.PauseStartTime = &earlier r2.Status.ControllerSetPause = true r2.Status.PauseConditions = []v1alpha1.PauseCondition{{ Reason: v1alpha1.CanaryPauseStep, @@ -926,7 +872,6 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { patch := f.getPatchedRollout(patchIndex) expectedPatch := `{ "status":{ - "pauseStartTime": null, "controllerSetPause": null, "pauseConditions": null, "currentStepIndex":2 @@ -1087,7 +1032,6 @@ func TestResumeRolloutAfterPauseDuration(t *testing.T) { r1 = updateCanaryRolloutStatus(r1, rs1PodHash, 1, 1, 1, true) overAMinuteAgo := metav1.Time{Time: time.Now().Add(-61 * time.Second)} r1.Status.ObservedGeneration = conditions.ComputeGenerationHash(r1.Spec) - r1.Status.PauseStartTime = &overAMinuteAgo r1.Status.PauseConditions = []v1alpha1.PauseCondition{{ Reason: v1alpha1.CanaryPauseStep, StartTime: overAMinuteAgo, @@ -1108,9 +1052,6 @@ func TestResumeRolloutAfterPauseDuration(t *testing.T) { status := patchObj["status"].(map[string]interface{}) assert.Equal(t, float64(2), status["currentStepIndex"]) - pauseStartTime, ok := status["pauseStartTime"] - assert.True(t, ok) - assert.Equal(t, nil, pauseStartTime) // controllerPause, ok := status["controllerPause"] // assert.True(t, ok) // assert.Nil(t, controllerPause) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index ac0fc7e90c..9811d42d6e 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -238,7 +238,7 @@ func generateConditionsPatch(available bool, progressingReason string, progressi // func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active string, availableReplicas, updatedReplicas, hpaReplicas int32, pause bool, available bool, progressingStatus string) *v1alpha1.Rollout { func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active string, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas int32, pause bool, available bool) *v1alpha1.Rollout { - newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas, pause) + newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas) selector := newRollout.Spec.Selector.DeepCopy() if active != "" { selector.MatchLabels[v1alpha1.DefaultRolloutUniqueLabelKey] = active @@ -260,7 +260,7 @@ func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active string, a return newRollout } func updateCanaryRolloutStatus(r *v1alpha1.Rollout, stableRS string, availableReplicas, updatedReplicas, hpaReplicas int32, pause bool) *v1alpha1.Rollout { - newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, availableReplicas, hpaReplicas, pause) + newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, availableReplicas, hpaReplicas) newRollout.Status.Canary.StableRS = stableRS if pause { now := metav1.Now() @@ -274,17 +274,12 @@ func updateCanaryRolloutStatus(r *v1alpha1.Rollout, stableRS string, availableRe return newRollout } -func updateBaseRolloutStatus(r *v1alpha1.Rollout, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas int32, pause bool) *v1alpha1.Rollout { +func updateBaseRolloutStatus(r *v1alpha1.Rollout, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas int32) *v1alpha1.Rollout { newRollout := r.DeepCopy() newRollout.Status.Replicas = totalReplicas newRollout.Status.AvailableReplicas = availableReplicas newRollout.Status.UpdatedReplicas = updatedReplicas newRollout.Status.HPAReplicas = hpaReplicas - if pause { - // newRollout.Status.ControllerPause = pause - now := metav1.Now() - newRollout.Status.PauseStartTime = &now - } return newRollout } diff --git a/rollout/pause.go b/rollout/pause.go index e909da1bf3..22fef2e97c 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -64,21 +64,10 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus } } - pauseStartTime := pCtx.rollout.Status.PauseStartTime - if len(newPauseConditions) == 0 { return } - - if len(pCtx.addPauseReasons) > 0 { - if pauseStartTime == nil { - pCtx.log.Infof("Setting PauseStartTime to %s", now.UTC().Format(time.RFC3339)) - pauseStartTime = &now - } - } - newStatus.ControllerSetPause = controllerPause - newStatus.PauseStartTime = pauseStartTime newStatus.PauseConditions = newPauseConditions } From 2c0ef23e40d80cefb00579e30afb282bd97349df Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Sun, 20 Oct 2019 12:04:52 -0700 Subject: [PATCH 10/16] Rename ControllerSetPause to ControllerPause --- manifests/crds/rollout-crd.yaml | 2 -- manifests/install.yaml | 2 -- manifests/namespace-install.yaml | 2 -- pkg/apis/rollouts/v1alpha1/openapi_generated.go | 9 +-------- pkg/apis/rollouts/v1alpha1/types.go | 4 +--- rollout/analysis_test.go | 2 +- rollout/bluegreen.go | 4 ++-- rollout/bluegreen_test.go | 12 ++++++------ rollout/canary.go | 2 +- rollout/canary_test.go | 10 +++++----- rollout/controller_test.go | 4 ++-- rollout/pause.go | 6 +++--- 12 files changed, 22 insertions(+), 37 deletions(-) diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index 9f9f20f96b..d3985f8fda 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -2682,8 +2682,6 @@ spec: type: array controllerPause: type: boolean - controllerSetPause: - type: boolean currentPodHash: type: string currentStepHash: diff --git a/manifests/install.yaml b/manifests/install.yaml index 215c4320e5..c6269b85dc 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -10356,8 +10356,6 @@ spec: type: array controllerPause: type: boolean - controllerSetPause: - type: boolean currentPodHash: type: string currentStepHash: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 81f05e8cc4..79d35f04b5 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -10356,8 +10356,6 @@ spec: type: array controllerPause: type: boolean - controllerSetPause: - type: boolean currentPodHash: type: string currentStepHash: diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index 5645883a97..cb90b402d6 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -1684,13 +1684,6 @@ func schema_pkg_apis_rollouts_v1alpha1_RolloutStatus(ref common.ReferenceCallbac SchemaProps: spec.SchemaProps{ Description: "RolloutStatus is the status for a Rollout resource", Properties: map[string]spec.Schema{ - "controllerSetPause": { - SchemaProps: spec.SchemaProps{ - Description: "ControllerSetPause indicates the controller paused the rollout (i.e. pause step or inconclusive run)", - Type: []string{"boolean"}, - Format: "", - }, - }, "pauseConditions": { SchemaProps: spec.SchemaProps{ Description: "PauseConditions indicates why the rollout is currently paused", @@ -1706,7 +1699,7 @@ func schema_pkg_apis_rollouts_v1alpha1_RolloutStatus(ref common.ReferenceCallbac }, "controllerPause": { SchemaProps: spec.SchemaProps{ - Description: "ControllerPause indicates the controller paused the rollout (i.e. pause step or inconclusive run)", + Description: "ControllerPause indicates the controller has paused the rollout", Type: []string{"boolean"}, Format: "", }, diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 396224d8ce..c994ae68a4 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -274,11 +274,9 @@ type PauseCondition struct { // RolloutStatus is the status for a Rollout resource type RolloutStatus struct { - //ControllerSetPause indicates the controller paused the rollout (i.e. pause step or inconclusive run) - ControllerSetPause bool `json:"controllerSetPause,omitempty"` // PauseConditions indicates why the rollout is currently paused PauseConditions []PauseCondition `json:"pauseConditions,omitempty"` - //ControllerPause indicates the controller paused the rollout (i.e. pause step or inconclusive run) + //ControllerPause indicates the controller has paused the rollout ControllerPause bool `json:"controllerPause,omitempty"` // CurrentPodHash the hash of the current pod template // +optional diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index 14bc9ae508..ea72ef6381 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -551,7 +551,7 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { "reason": "InconclusiveAnalysisRun", "startTime": "%s" }], - "controllerSetPause": true + "controllerPause": true } }` condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index 0778c0db51..d85187c6fe 100644 --- a/rollout/bluegreen.go +++ b/rollout/bluegreen.go @@ -150,7 +150,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.BlueGreenPause) // If the rollout is not paused and the active service is not point at the newRS, we should pause the rollout. - if cond == nil && !rollout.Status.ControllerSetPause && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { + if cond == nil && !rollout.Status.ControllerPause && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { roCtx.PauseContext().AddControllerPause(v1alpha1.BlueGreenPause) return true } @@ -166,7 +166,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev } - return cond != nil && rollout.Status.ControllerSetPause + return cond != nil && rollout.Status.ControllerPause } // scaleDownOldReplicaSetsForBlueGreen scales down old replica sets when rollout strategy is "Blue Green". diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 36bfeb2530..05919d68f5 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -162,7 +162,7 @@ func TestBlueGreenHandlePause(t *testing.T) { "reason": "BlueGreenPause", "startTime": "%s" }], - "controllerSetPause": true + "controllerPause": true } }` now := metav1.Now().UTC().Format(time.RFC3339) @@ -298,7 +298,7 @@ func TestBlueGreenHandlePause(t *testing.T) { now := metav1.Now() before := metav1.NewTime(now.Add(-1 * time.Minute)) r2.Status.PauseConditions[0].StartTime = before - r2.Status.ControllerSetPause = true + r2.Status.ControllerPause = true pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2) conditions.SetRolloutCondition(&r2.Status, pausedCondition) @@ -316,7 +316,7 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatchWithoutSubs := `{ "status": { "pauseConditions": null, - "controllerSetPause": null + "controllerPause": null } }` expectedPatch := calculatePatch(r2, expectedPatchWithoutSubs) @@ -409,7 +409,7 @@ func TestBlueGreenHandlePause(t *testing.T) { "reason":"BlueGreenPause", "startTime": "%s" }], - "controllerSetPause": true + "controllerPause": true } }` expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, now)) @@ -479,7 +479,7 @@ func TestBlueGreenHandlePause(t *testing.T) { r2.Spec.Strategy.BlueGreen.ScaleDownDelaySeconds = pointer.Int32Ptr(10) r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, 1, 1, 2, 1, false, true) - r2.Status.ControllerSetPause = true + r2.Status.ControllerPause = true pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2) conditions.SetRolloutCondition(&r2.Status, pausedCondition) @@ -517,7 +517,7 @@ func TestBlueGreenHandlePause(t *testing.T) { "blueGreen": { "activeSelector": "%s" }, - "controllerSetPause":null, + "controllerPause":null, "conditions": %s, "selector": "%s" } diff --git a/rollout/canary.go b/rollout/canary.go index 623e5ed81a..2cb5b4cb18 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -121,7 +121,7 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { } cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.CanaryPauseStep) if cond == nil { - if !rollout.Status.ControllerSetPause { + if !rollout.Status.ControllerPause { roCtx.PauseContext().AddControllerPause(v1alpha1.CanaryPauseStep) } return true diff --git a/rollout/canary_test.go b/rollout/canary_test.go index 3332b5fbdc..18b6f5e20a 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -110,7 +110,7 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) { "startTime": "%s" }], "conditions": %s, - "controllerSetPause": true + "controllerPause": true } }` @@ -211,7 +211,7 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 10, 0, 10, false) r2.Status.AvailableReplicas = 10 - r2.Status.ControllerSetPause = true + r2.Status.ControllerPause = true f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -223,7 +223,7 @@ func TestCanaryRolloutIncrementStepAfterUnPaused(t *testing.T) { patch := f.getPatchedRollout(patchIndex) expectedPatchTemplate := `{ "status":{ - "controllerSetPause": null, + "controllerPause": null, "conditions" : %s, "currentStepIndex": 1 } @@ -858,7 +858,7 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { earlier := metav1.Now() earlier.Time = earlier.Add(-10 * time.Second) - r2.Status.ControllerSetPause = true + r2.Status.ControllerPause = true r2.Status.PauseConditions = []v1alpha1.PauseCondition{{ Reason: v1alpha1.CanaryPauseStep, StartTime: earlier, @@ -872,7 +872,7 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { patch := f.getPatchedRollout(patchIndex) expectedPatch := `{ "status":{ - "controllerSetPause": null, + "controllerPause": null, "pauseConditions": null, "currentStepIndex":2 } diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 9811d42d6e..fc2ee6fb6b 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -254,7 +254,7 @@ func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active string, a Reason: v1alpha1.BlueGreenPause, StartTime: now, } - newRollout.Status.ControllerSetPause = true + newRollout.Status.ControllerPause = true newRollout.Status.PauseConditions = append(newRollout.Status.PauseConditions, cond) } return newRollout @@ -268,7 +268,7 @@ func updateCanaryRolloutStatus(r *v1alpha1.Rollout, stableRS string, availableRe Reason: v1alpha1.CanaryPauseStep, StartTime: now, } - newRollout.Status.ControllerSetPause = true + newRollout.Status.ControllerPause = true newRollout.Status.PauseConditions = append(newRollout.Status.PauseConditions, cond) } return newRollout diff --git a/rollout/pause.go b/rollout/pause.go index 22fef2e97c..8fb5da0f28 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -35,7 +35,7 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus return } - controllerPause := pCtx.rollout.Status.ControllerSetPause + controllerPause := pCtx.rollout.Status.ControllerPause statusToRemove := map[v1alpha1.PauseReason]bool{} for i := range pCtx.removePauseReasons { statusToRemove[pCtx.removePauseReasons[i]] = true @@ -67,7 +67,7 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus if len(newPauseConditions) == 0 { return } - newStatus.ControllerSetPause = controllerPause + newStatus.ControllerPause = controllerPause newStatus.PauseConditions = newPauseConditions } @@ -94,7 +94,7 @@ func (pCtx *pauseContext) CompletedPauseStep(pause v1alpha1.RolloutPause) bool { return true } } - } else if rollout.Status.ControllerSetPause && pauseCondition == nil { + } else if rollout.Status.ControllerPause && pauseCondition == nil { pCtx.log.Info("Rollout has been unpaused") return true } From d6fbb9c926408e8b4fd0f33e3e132b038d38adee Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Sun, 20 Oct 2019 12:08:43 -0700 Subject: [PATCH 11/16] Change kubectl resume command to use pauseConditons --- pkg/kubectl-argo-rollouts/cmd/resume/resume.go | 2 +- pkg/kubectl-argo-rollouts/cmd/resume/resume_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/kubectl-argo-rollouts/cmd/resume/resume.go b/pkg/kubectl-argo-rollouts/cmd/resume/resume.go index 632c3e7eaf..2392e8d995 100644 --- a/pkg/kubectl-argo-rollouts/cmd/resume/resume.go +++ b/pkg/kubectl-argo-rollouts/cmd/resume/resume.go @@ -29,7 +29,7 @@ func NewCmdResume(o *options.ArgoRolloutsOptions) *cobra.Command { } rolloutIf := o.RolloutsClientset().ArgoprojV1alpha1().Rollouts(o.Namespace()) for _, name := range args { - ro, err := rolloutIf.Patch(name, types.MergePatchType, []byte(`{"spec":{"paused":false}}`)) + ro, err := rolloutIf.Patch(name, types.MergePatchType, []byte(`{"status":{"pauseConditions":null}}`)) if err != nil { return err } diff --git a/pkg/kubectl-argo-rollouts/cmd/resume/resume_test.go b/pkg/kubectl-argo-rollouts/cmd/resume/resume_test.go index d670a745e3..20adf90b4d 100644 --- a/pkg/kubectl-argo-rollouts/cmd/resume/resume_test.go +++ b/pkg/kubectl-argo-rollouts/cmd/resume/resume_test.go @@ -46,8 +46,8 @@ func TestResumeCmdSuccess(t *testing.T) { fakeClient.ReactionChain = nil fakeClient.AddReactor("patch", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { if patchAction, ok := action.(kubetesting.PatchAction); ok { - if string(patchAction.GetPatch()) == `{"spec":{"paused":false}}` { - ro.Spec.Paused = false + if string(patchAction.GetPatch()) == `{"spec":{"pauseConditions":null}}` { + ro.Status.PauseConditions = nil } } return true, &ro, nil @@ -59,7 +59,7 @@ func TestResumeCmdSuccess(t *testing.T) { err := cmd.Execute() assert.Nil(t, err) - assert.False(t, ro.Spec.Paused) + assert.Nil(t, ro.Status.PauseConditions) stdout := o.Out.(*bytes.Buffer).String() stderr := o.ErrOut.(*bytes.Buffer).String() assert.Equal(t, stdout, "rollout 'guestbook' resumed\n") From 92adf074f159a1ee6adc87a1daaf63934c309ed1 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Sun, 20 Oct 2019 12:26:04 -0700 Subject: [PATCH 12/16] Linting and small cleanup --- rollout/analysis.go | 2 +- rollout/analysis_test.go | 4 ++-- rollout/bluegreen.go | 7 +++---- rollout/bluegreen_test.go | 8 ++++---- rollout/canary.go | 24 ++++++++++++------------ rollout/canary_test.go | 6 +++--- rollout/pause.go | 16 ++++++++-------- 7 files changed, 33 insertions(+), 34 deletions(-) diff --git a/rollout/analysis.go b/rollout/analysis.go index 4f8734ea78..5ec41de4b2 100644 --- a/rollout/analysis.go +++ b/rollout/analysis.go @@ -148,7 +148,7 @@ func (c *RolloutController) reconcileStepBasedAnalysisRun(roCtx *canaryContext) } if currentAr.Status != nil && currentAr.Status.Status == v1alpha1.AnalysisStatusInconclusive { - roCtx.PauseContext().AddControllerPause(v1alpha1.InconclusiveAnalysisRun) + roCtx.PauseContext().AddPauseCondition(v1alpha1.InconclusiveAnalysisRun) } return currentAr, nil diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index ea72ef6381..9b0cba2996 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -548,7 +548,7 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { "currentStepAnalysisRun": null }, "pauseConditions": [{ - "reason": "InconclusiveAnalysisRun", + "reason": "%s", "startTime": "%s" }], "controllerPause": true @@ -556,7 +556,7 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { }` condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, now)), patch) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.InconclusiveAnalysisRun, now)), patch) } func TestErrorConditionAfterErrorAnalysisRun(t *testing.T) { diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index d85187c6fe..84e32a46dd 100644 --- a/rollout/bluegreen.go +++ b/rollout/bluegreen.go @@ -32,7 +32,7 @@ func (c *RolloutController) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*apps logCtx := roCtx.Log() allRSs := roCtx.AllRSs() if reconcileBlueGreenTemplateChange(roCtx) { - roCtx.PauseContext().ClearPauseReasons() + roCtx.PauseContext().ClearPauseConditions() logCtx.Infof("New pod template or template change detected") return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc, roCtx) } @@ -137,7 +137,6 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev rollout := roCtx.Rollout() if defaults.GetAutoPromotionEnabledOrDefault(rollout) { - roCtx.PauseContext().RemoveControllerPause(v1alpha1.BlueGreenPause) return false } @@ -151,7 +150,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.BlueGreenPause) // If the rollout is not paused and the active service is not point at the newRS, we should pause the rollout. if cond == nil && !rollout.Status.ControllerPause && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { - roCtx.PauseContext().AddControllerPause(v1alpha1.BlueGreenPause) + roCtx.PauseContext().AddPauseCondition(v1alpha1.BlueGreenPause) return true } @@ -161,7 +160,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev switchDeadline := cond.StartTime.Add(time.Duration(*autoPromoteActiveServiceDelaySeconds) * time.Second) now := metav1.Now() if now.After(switchDeadline) { - roCtx.PauseContext().RemoveControllerPause(v1alpha1.BlueGreenPause) + roCtx.PauseContext().RemovePauseCondition(v1alpha1.BlueGreenPause) } } diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 05919d68f5..c41b4f2721 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -159,14 +159,14 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatch := `{ "status": { "pauseConditions": [{ - "reason": "BlueGreenPause", + "reason": "%s", "startTime": "%s" }], "controllerPause": true } }` now := metav1.Now().UTC().Format(time.RFC3339) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, now)), patch) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, v1alpha1.BlueGreenPause, now)), patch) }) @@ -406,13 +406,13 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatchWithoutSubs := `{ "status": { "pauseConditions": [{ - "reason":"BlueGreenPause", + "reason":"%s", "startTime": "%s" }], "controllerPause": true } }` - expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, now)) + expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, v1alpha1.BlueGreenPause, now)) patchIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch) f.run(getKey(r2, t)) diff --git a/rollout/canary.go b/rollout/canary.go index 2cb5b4cb18..9860d99b3c 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -122,7 +122,7 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.CanaryPauseStep) if cond == nil { if !rollout.Status.ControllerPause { - roCtx.PauseContext().AddControllerPause(v1alpha1.CanaryPauseStep) + roCtx.PauseContext().AddPauseCondition(v1alpha1.CanaryPauseStep) } return true } @@ -257,7 +257,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error c.recorder.Event(r, corev1.EventTypeNormal, "SkipSteps", msg) } } - roCtx.PauseContext().ClearPauseReasons() + roCtx.PauseContext().ClearPauseConditions() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } @@ -273,11 +273,18 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error newStatus.CurrentStepIndex = &stepCount } - roCtx.PauseContext().ClearPauseReasons() + roCtx.PauseContext().ClearPauseConditions() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } + currBackgroundAr := analysisutil.GetCurrentBackgroundAnalysisRun(currArs) + if currBackgroundAr != nil { + if currBackgroundAr.Status == nil || !currBackgroundAr.Status.Status.Completed() || analysisutil.IsTerminating(currBackgroundAr) { + newStatus.Canary.CurrentBackgroundAnalysisRun = currBackgroundAr.Name + } + } + if currentStepIndex != nil && *currentStepIndex == stepCount { logCtx.Info("Rollout has executed every step") newStatus.CurrentStepIndex = &stepCount @@ -286,18 +293,11 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error logCtx.Info("New RS has successfully progressed") newStatus.Canary.StableRS = newStatus.CurrentPodHash } - roCtx.PauseContext().ClearPauseReasons() + roCtx.PauseContext().ClearPauseConditions() newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } - currBackgroundAr := analysisutil.GetCurrentBackgroundAnalysisRun(currArs) - if currBackgroundAr != nil { - if currBackgroundAr.Status == nil || !currBackgroundAr.Status.Status.Completed() || analysisutil.IsTerminating(currBackgroundAr) { - newStatus.Canary.CurrentBackgroundAnalysisRun = currBackgroundAr.Name - } - } - if stepCount == 0 { logCtx.Info("Rollout has no steps") if newRS != nil && newRS.Status.AvailableReplicas == defaults.GetRolloutReplicasOrDefault(r) { @@ -316,7 +316,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error } logCtx.Infof("Incrementing the Current Step Index to %d", *currentStepIndex) c.recorder.Eventf(r, corev1.EventTypeNormal, "SetStepIndex", "Set Step Index to %d", int(*currentStepIndex)) - roCtx.PauseContext().RemoveControllerPause(v1alpha1.CanaryPauseStep) + roCtx.PauseContext().RemovePauseCondition(v1alpha1.CanaryPauseStep) newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } diff --git a/rollout/canary_test.go b/rollout/canary_test.go index 18b6f5e20a..3b18126075 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -1052,7 +1052,7 @@ func TestResumeRolloutAfterPauseDuration(t *testing.T) { status := patchObj["status"].(map[string]interface{}) assert.Equal(t, float64(2), status["currentStepIndex"]) - // controllerPause, ok := status["controllerPause"] - // assert.True(t, ok) - // assert.Nil(t, controllerPause) + controllerPause, ok := status["controllerPause"] + assert.True(t, ok) + assert.Nil(t, controllerPause) } diff --git a/rollout/pause.go b/rollout/pause.go index 8fb5da0f28..d8d8b45836 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -14,24 +14,24 @@ type pauseContext struct { rollout *v1alpha1.Rollout log *log.Entry - addPauseReasons []v1alpha1.PauseReason - removePauseReasons []v1alpha1.PauseReason - clearPauseReasons bool + addPauseReasons []v1alpha1.PauseReason + removePauseReasons []v1alpha1.PauseReason + clearPauseConditions bool } -func (pCtx *pauseContext) AddControllerPause(reason v1alpha1.PauseReason) { +func (pCtx *pauseContext) AddPauseCondition(reason v1alpha1.PauseReason) { pCtx.addPauseReasons = append(pCtx.addPauseReasons, reason) } -func (pCtx *pauseContext) RemoveControllerPause(reason v1alpha1.PauseReason) { +func (pCtx *pauseContext) RemovePauseCondition(reason v1alpha1.PauseReason) { pCtx.removePauseReasons = append(pCtx.removePauseReasons, reason) } -func (pCtx *pauseContext) ClearPauseReasons() { - pCtx.clearPauseReasons = true +func (pCtx *pauseContext) ClearPauseConditions() { + pCtx.clearPauseConditions = true } func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus) { - if pCtx.clearPauseReasons { + if pCtx.clearPauseConditions { return } From 9c9157d680ea71f71b2f99754cae9c3460b5525f Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Mon, 21 Oct 2019 22:33:51 -0700 Subject: [PATCH 13/16] Address feedback --- rollout/bluegreen.go | 2 +- rollout/canary.go | 5 +++++ rollout/sync.go | 4 +--- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index 84e32a46dd..68304d89f8 100644 --- a/rollout/bluegreen.go +++ b/rollout/bluegreen.go @@ -84,8 +84,8 @@ func (c *RolloutController) rolloutBlueGreen(r *v1alpha1.Rollout, rsList []*apps _, hasScaleDownDeadlineAnnotationKey := newRS.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey] if hasScaleDownDeadlineAnnotationKey { logCtx.Infof("Detected scale down annotation for ReplicaSet '%s' and will skip pause", newRS.Name) + noFastRollback = false } - noFastRollback = !hasScaleDownDeadlineAnnotationKey } if noFastRollback { logCtx.Info("Reconciling pause") diff --git a/rollout/canary.go b/rollout/canary.go index 9860d99b3c..895fa46838 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -121,6 +121,11 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { } cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.CanaryPauseStep) if cond == nil { + // When the pause condition is null, that means the rollout is in an not paused state. + // As a result,, the controller needs to detect whether a rollout was unpaused or the + // rollout needs to be paused for the first time. If the ControllerPause is false, + // the controller has not paused the rollout yet and needs to do so before it + // can proceed. if !rollout.Status.ControllerPause { roCtx.PauseContext().AddPauseCondition(v1alpha1.CanaryPauseStep) } diff --git a/rollout/sync.go b/rollout/sync.go index bddc6d39d0..ebd3345c7a 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -574,10 +574,8 @@ func (c *RolloutController) calculateRolloutConditions(roCtx rolloutContext, new // persistRolloutStatus persists updates to rollout status. If no changes were made, it is a no-op func (c *RolloutController) persistRolloutStatus(roCtx rolloutContext, newStatus *v1alpha1.RolloutStatus) error { orig := roCtx.Rollout() - specCopy := orig.Spec.DeepCopy() - roCtx.PauseContext().CalculatePauseStatus(newStatus) - newStatus.ObservedGeneration = conditions.ComputeGenerationHash(*specCopy) + newStatus.ObservedGeneration = conditions.ComputeGenerationHash(orig.Spec) logCtx := logutil.WithRollout(orig) patch, modified, err := diff.CreateTwoWayMergePatch( &v1alpha1.Rollout{ From aa0935ad0847078a2962a43fefb71da68225cd77 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Mon, 21 Oct 2019 22:59:28 -0700 Subject: [PATCH 14/16] Rename PauseReasons --- pkg/apis/rollouts/v1alpha1/types.go | 12 ++++++------ rollout/analysis.go | 2 +- rollout/analysis_test.go | 2 +- rollout/bluegreen.go | 6 +++--- rollout/bluegreen_test.go | 4 ++-- rollout/canary.go | 6 +++--- rollout/canary_test.go | 6 +++--- rollout/controller_test.go | 6 +++--- rollout/pause.go | 2 +- 9 files changed, 23 insertions(+), 23 deletions(-) diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index c994ae68a4..7a3bc759b7 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -258,12 +258,12 @@ type RolloutPause struct { type PauseReason string const ( - // InconclusiveAnalysisRun pauses rollout when rollout has an inconclusive analysis run - InconclusiveAnalysisRun PauseReason = "InconclusiveAnalysisRun" - // CanaryPauseStep pause rollout for canary pause step - CanaryPauseStep PauseReason = "CanaryPauseStep" - // BlueGreenPause pause rollout before promoting rollout - BlueGreenPause PauseReason = "BlueGreenPause" + // PauseReasonInconclusiveAnalysisRun pauses rollout when rollout has an inconclusive analysis run + PauseReasonInconclusiveAnalysisRun PauseReason = "InconclusiveAnalysisRun" + // PauseReasonCanaryPauseStep pause rollout for canary pause step + PauseReasonCanaryPauseStep PauseReason = "CanaryPauseStep" + // PauseReasonBlueGreenPause pause rollout before promoting rollout + PauseReasonBlueGreenPause PauseReason = "BlueGreenPause" ) // PauseCondition the reason for a pause and when it started diff --git a/rollout/analysis.go b/rollout/analysis.go index 5ec41de4b2..ecac9fe769 100644 --- a/rollout/analysis.go +++ b/rollout/analysis.go @@ -148,7 +148,7 @@ func (c *RolloutController) reconcileStepBasedAnalysisRun(roCtx *canaryContext) } if currentAr.Status != nil && currentAr.Status.Status == v1alpha1.AnalysisStatusInconclusive { - roCtx.PauseContext().AddPauseCondition(v1alpha1.InconclusiveAnalysisRun) + roCtx.PauseContext().AddPauseCondition(v1alpha1.PauseReasonInconclusiveAnalysisRun) } return currentAr, nil diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index 9b0cba2996..051ce292d5 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -556,7 +556,7 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { }` condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.InconclusiveAnalysisRun, now)), patch) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.PauseReasonInconclusiveAnalysisRun, now)), patch) } func TestErrorConditionAfterErrorAnalysisRun(t *testing.T) { diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index 68304d89f8..d902016a29 100644 --- a/rollout/bluegreen.go +++ b/rollout/bluegreen.go @@ -147,10 +147,10 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev return false } - cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.BlueGreenPause) + cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.PauseReasonBlueGreenPause) // If the rollout is not paused and the active service is not point at the newRS, we should pause the rollout. if cond == nil && !rollout.Status.ControllerPause && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint && activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != newRSPodHash { - roCtx.PauseContext().AddPauseCondition(v1alpha1.BlueGreenPause) + roCtx.PauseContext().AddPauseCondition(v1alpha1.PauseReasonBlueGreenPause) return true } @@ -160,7 +160,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev switchDeadline := cond.StartTime.Add(time.Duration(*autoPromoteActiveServiceDelaySeconds) * time.Second) now := metav1.Now() if now.After(switchDeadline) { - roCtx.PauseContext().RemovePauseCondition(v1alpha1.BlueGreenPause) + roCtx.PauseContext().RemovePauseCondition(v1alpha1.PauseReasonBlueGreenPause) } } diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index c41b4f2721..42435bea6a 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -166,7 +166,7 @@ func TestBlueGreenHandlePause(t *testing.T) { } }` now := metav1.Now().UTC().Format(time.RFC3339) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, v1alpha1.BlueGreenPause, now)), patch) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, v1alpha1.PauseReasonBlueGreenPause, now)), patch) }) @@ -412,7 +412,7 @@ func TestBlueGreenHandlePause(t *testing.T) { "controllerPause": true } }` - expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, v1alpha1.BlueGreenPause, now)) + expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, v1alpha1.PauseReasonBlueGreenPause, now)) patchIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch) f.run(getKey(r2, t)) diff --git a/rollout/canary.go b/rollout/canary.go index 895fa46838..c83512840d 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -119,7 +119,7 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { if currentStep.Pause == nil { return false } - cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.CanaryPauseStep) + cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.PauseReasonCanaryPauseStep) if cond == nil { // When the pause condition is null, that means the rollout is in an not paused state. // As a result,, the controller needs to detect whether a rollout was unpaused or the @@ -127,7 +127,7 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { // the controller has not paused the rollout yet and needs to do so before it // can proceed. if !rollout.Status.ControllerPause { - roCtx.PauseContext().AddPauseCondition(v1alpha1.CanaryPauseStep) + roCtx.PauseContext().AddPauseCondition(v1alpha1.PauseReasonCanaryPauseStep) } return true } @@ -321,7 +321,7 @@ func (c *RolloutController) syncRolloutStatusCanary(roCtx *canaryContext) error } logCtx.Infof("Incrementing the Current Step Index to %d", *currentStepIndex) c.recorder.Eventf(r, corev1.EventTypeNormal, "SetStepIndex", "Set Step Index to %d", int(*currentStepIndex)) - roCtx.PauseContext().RemovePauseCondition(v1alpha1.CanaryPauseStep) + roCtx.PauseContext().RemovePauseCondition(v1alpha1.PauseReasonCanaryPauseStep) newStatus = c.calculateRolloutConditions(roCtx, newStatus) return c.persistRolloutStatus(roCtx, &newStatus) } diff --git a/rollout/canary_test.go b/rollout/canary_test.go index 3b18126075..150fc9f794 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -116,7 +116,7 @@ func TestCanaryRolloutEnterPauseState(t *testing.T) { conditions := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) now := metav1.Now().UTC().Format(time.RFC3339) - expectedPatchWithoutObservedGen := fmt.Sprintf(expectedPatchTemplate, v1alpha1.CanaryPauseStep, now, conditions) + expectedPatchWithoutObservedGen := fmt.Sprintf(expectedPatchTemplate, v1alpha1.PauseReasonCanaryPauseStep, now, conditions) expectedPatch := calculatePatch(r2, expectedPatchWithoutObservedGen) assert.Equal(t, expectedPatch, patch) } @@ -860,7 +860,7 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { earlier.Time = earlier.Add(-10 * time.Second) r2.Status.ControllerPause = true r2.Status.PauseConditions = []v1alpha1.PauseCondition{{ - Reason: v1alpha1.CanaryPauseStep, + Reason: v1alpha1.PauseReasonCanaryPauseStep, StartTime: earlier, }} f.rolloutLister = append(f.rolloutLister, r2) @@ -1033,7 +1033,7 @@ func TestResumeRolloutAfterPauseDuration(t *testing.T) { overAMinuteAgo := metav1.Time{Time: time.Now().Add(-61 * time.Second)} r1.Status.ObservedGeneration = conditions.ComputeGenerationHash(r1.Spec) r1.Status.PauseConditions = []v1alpha1.PauseCondition{{ - Reason: v1alpha1.CanaryPauseStep, + Reason: v1alpha1.PauseReasonCanaryPauseStep, StartTime: overAMinuteAgo, }} f.kubeobjects = append(f.kubeobjects, rs1) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index fc2ee6fb6b..7263e54034 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -251,7 +251,7 @@ func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active string, a if pause { now := metav1.Now() cond := v1alpha1.PauseCondition{ - Reason: v1alpha1.BlueGreenPause, + Reason: v1alpha1.PauseReasonBlueGreenPause, StartTime: now, } newRollout.Status.ControllerPause = true @@ -265,7 +265,7 @@ func updateCanaryRolloutStatus(r *v1alpha1.Rollout, stableRS string, availableRe if pause { now := metav1.Now() cond := v1alpha1.PauseCondition{ - Reason: v1alpha1.CanaryPauseStep, + Reason: v1alpha1.PauseReasonCanaryPauseStep, StartTime: now, } newRollout.Status.ControllerPause = true @@ -894,7 +894,7 @@ func TestRequeueStuckRollout(t *testing.T) { } if rolloutPaused { r.Status.PauseConditions = []v1alpha1.PauseCondition{{ - Reason: v1alpha1.BlueGreenPause, + Reason: v1alpha1.PauseReasonBlueGreenPause, }} } if rolloutCompleted { diff --git a/rollout/pause.go b/rollout/pause.go index d8d8b45836..f53751c7d4 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -83,7 +83,7 @@ func (pCtx *pauseContext) GetPauseCondition(reason v1alpha1.PauseReason) *v1alph func (pCtx *pauseContext) CompletedPauseStep(pause v1alpha1.RolloutPause) bool { rollout := pCtx.rollout - pauseCondition := pCtx.GetPauseCondition(v1alpha1.CanaryPauseStep) + pauseCondition := pCtx.GetPauseCondition(v1alpha1.PauseReasonCanaryPauseStep) if pause.Duration != nil { now := metav1.Now() From 05b0b2bb8b9e697d248ffac39605690d96517696 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Tue, 22 Oct 2019 06:50:24 -0700 Subject: [PATCH 15/16] Add unpause of .spec.paused to cmd back --- pkg/apis/rollouts/v1alpha1/types.go | 4 ++-- pkg/kubectl-argo-rollouts/cmd/resume/resume.go | 10 +++++++++- pkg/kubectl-argo-rollouts/cmd/resume/resume_test.go | 11 ++++++++++- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 7a3bc759b7..2ac26f9253 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -258,8 +258,8 @@ type RolloutPause struct { type PauseReason string const ( - // PauseReasonInconclusiveAnalysisRun pauses rollout when rollout has an inconclusive analysis run - PauseReasonInconclusiveAnalysisRun PauseReason = "InconclusiveAnalysisRun" + // PauseReasonInconclusiveAnalysis pauses rollout when rollout has an inconclusive analysis run + PauseReasonInconclusiveAnalysis PauseReason = "InconclusiveAnalysisRun" // PauseReasonCanaryPauseStep pause rollout for canary pause step PauseReasonCanaryPauseStep PauseReason = "CanaryPauseStep" // PauseReasonBlueGreenPause pause rollout before promoting rollout diff --git a/pkg/kubectl-argo-rollouts/cmd/resume/resume.go b/pkg/kubectl-argo-rollouts/cmd/resume/resume.go index 2392e8d995..a73493da31 100644 --- a/pkg/kubectl-argo-rollouts/cmd/resume/resume.go +++ b/pkg/kubectl-argo-rollouts/cmd/resume/resume.go @@ -14,6 +14,14 @@ const ( # Resume a rollout %[1]s resume guestbook ` + unpausePatch = `{ + "spec": { + "paused": false + }, + "status: { + "pauseConditions": null + } +}` ) // NewCmdResume returns a new instance of an `rollouts resume` command @@ -29,7 +37,7 @@ func NewCmdResume(o *options.ArgoRolloutsOptions) *cobra.Command { } rolloutIf := o.RolloutsClientset().ArgoprojV1alpha1().Rollouts(o.Namespace()) for _, name := range args { - ro, err := rolloutIf.Patch(name, types.MergePatchType, []byte(`{"status":{"pauseConditions":null}}`)) + ro, err := rolloutIf.Patch(name, types.MergePatchType, []byte(unpausePatch)) if err != nil { return err } diff --git a/pkg/kubectl-argo-rollouts/cmd/resume/resume_test.go b/pkg/kubectl-argo-rollouts/cmd/resume/resume_test.go index 20adf90b4d..ce6f370ef4 100644 --- a/pkg/kubectl-argo-rollouts/cmd/resume/resume_test.go +++ b/pkg/kubectl-argo-rollouts/cmd/resume/resume_test.go @@ -38,6 +38,12 @@ func TestResumeCmdSuccess(t *testing.T) { Spec: v1alpha1.RolloutSpec{ Paused: true, }, + Status: v1alpha1.RolloutStatus{ + PauseConditions: []v1alpha1.PauseCondition{{ + Reason: v1alpha1.PauseReasonCanaryPauseStep, + }}, + ControllerPause: true, + }, } tf, o := options.NewFakeArgoRolloutsOptions(&ro) @@ -46,8 +52,9 @@ func TestResumeCmdSuccess(t *testing.T) { fakeClient.ReactionChain = nil fakeClient.AddReactor("patch", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { if patchAction, ok := action.(kubetesting.PatchAction); ok { - if string(patchAction.GetPatch()) == `{"spec":{"pauseConditions":null}}` { + if string(patchAction.GetPatch()) == unpausePatch { ro.Status.PauseConditions = nil + ro.Spec.Paused = false } } return true, &ro, nil @@ -60,6 +67,8 @@ func TestResumeCmdSuccess(t *testing.T) { assert.Nil(t, err) assert.Nil(t, ro.Status.PauseConditions) + assert.False(t, ro.Spec.Paused) + assert.True(t, ro.Status.ControllerPause) stdout := o.Out.(*bytes.Buffer).String() stderr := o.ErrOut.(*bytes.Buffer).String() assert.Equal(t, stdout, "rollout 'guestbook' resumed\n") From 79bbbeaf2ad784ae98740191da5a49fcb220dc17 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Tue, 22 Oct 2019 06:50:44 -0700 Subject: [PATCH 16/16] Remove run from pauseReason name --- rollout/analysis.go | 2 +- rollout/analysis_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rollout/analysis.go b/rollout/analysis.go index ecac9fe769..164068eec3 100644 --- a/rollout/analysis.go +++ b/rollout/analysis.go @@ -148,7 +148,7 @@ func (c *RolloutController) reconcileStepBasedAnalysisRun(roCtx *canaryContext) } if currentAr.Status != nil && currentAr.Status.Status == v1alpha1.AnalysisStatusInconclusive { - roCtx.PauseContext().AddPauseCondition(v1alpha1.PauseReasonInconclusiveAnalysisRun) + roCtx.PauseContext().AddPauseCondition(v1alpha1.PauseReasonInconclusiveAnalysis) } return currentAr, nil diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index 051ce292d5..96a6a63740 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -556,7 +556,7 @@ func TestPausedStepAfterInconclusiveAnalysisRun(t *testing.T) { }` condition := generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, r2, false) - assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.PauseReasonInconclusiveAnalysisRun, now)), patch) + assert.Equal(t, calculatePatch(r2, fmt.Sprintf(expectedPatch, condition, v1alpha1.PauseReasonInconclusiveAnalysis, now)), patch) } func TestErrorConditionAfterErrorAnalysisRun(t *testing.T) {