From 0f52eac7f37c68c3dacd253296f0c8f5e9c84680 Mon Sep 17 00:00:00 2001 From: dthomson25 Date: Sat, 18 Jan 2020 09:46:01 -0800 Subject: [PATCH] Fix honoring autoPromotionSeconds (#360) --- rollout/analysis.go | 4 ++-- rollout/bluegreen.go | 3 ++- rollout/bluegreen_test.go | 10 ++++++++-- rollout/canary.go | 2 +- rollout/controller.go | 2 +- rollout/experiment.go | 2 +- rollout/pause.go | 8 ++++---- rollout/sync.go | 4 +--- 8 files changed, 20 insertions(+), 15 deletions(-) diff --git a/rollout/analysis.go b/rollout/analysis.go index e70739e112..2592610ba0 100644 --- a/rollout/analysis.go +++ b/rollout/analysis.go @@ -114,7 +114,7 @@ func (c *RolloutController) reconcileBackgroundAnalysisRun(roCtx *canaryContext) return nil, nil } - if roCtx.PauseContext().GetPauseCondition(v1alpha1.PauseReasonInconclusiveAnalysis) != nil { + if getPauseCondition(rollout, v1alpha1.PauseReasonInconclusiveAnalysis) != nil { return currentAr, nil } @@ -160,7 +160,7 @@ func (c *RolloutController) reconcileStepBasedAnalysisRun(roCtx *canaryContext) step, index := replicasetutil.GetCurrentCanaryStep(rollout) currentAr := analysisutil.FilterAnalysisRunsByName(currentArs, rollout.Status.Canary.CurrentStepAnalysisRun) - if roCtx.PauseContext().GetPauseCondition(v1alpha1.PauseReasonInconclusiveAnalysis) != nil { + if getPauseCondition(rollout, v1alpha1.PauseReasonInconclusiveAnalysis) != nil { return currentAr, nil } diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index b4e0f224e3..f633b633df 100644 --- a/rollout/bluegreen.go +++ b/rollout/bluegreen.go @@ -154,7 +154,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev return false } - cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.PauseReasonBlueGreenPause) + cond := getPauseCondition(rollout, 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.PauseReasonBlueGreenPause) @@ -168,6 +168,7 @@ func (c *RolloutController) reconcileBlueGreenPause(activeSvc, previewSvc *corev now := metav1.Now() if now.After(switchDeadline) { roCtx.PauseContext().RemovePauseCondition(v1alpha1.PauseReasonBlueGreenPause) + return false } } diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 829b52dcff..5ea8381908 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -326,11 +326,17 @@ func TestBlueGreenHandlePause(t *testing.T) { expectedPatchWithoutSubs := `{ "status": { + "blueGreen": { + "activeSelector": "%s" + }, "pauseConditions": null, - "controllerPause": null + "controllerPause": null, + "selector": "foo=bar,rollouts-pod-template-hash=%s" } }` - expectedPatch := calculatePatch(r2, expectedPatchWithoutSubs) + expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash, rs2PodHash)) + f.expectPatchServiceAction(activeSvc, rs2PodHash) + f.expectPatchReplicaSetAction(rs1) patchRolloutIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch) f.run(getKey(r2, t)) diff --git a/rollout/canary.go b/rollout/canary.go index fbab8eba02..6d8e102042 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -132,7 +132,7 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { if currentStep.Pause == nil { return false } - cond := roCtx.PauseContext().GetPauseCondition(v1alpha1.PauseReasonCanaryPauseStep) + cond := getPauseCondition(rollout, 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 diff --git a/rollout/controller.go b/rollout/controller.go index fe152050f2..4fd283a18f 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -284,7 +284,7 @@ func (c *RolloutController) syncHandler(key string) error { return err } - if len(rollout.Status.PauseConditions) > 0 || r.Spec.Paused || isScalingEvent { + if getPauseCondition(r, v1alpha1.PauseReasonInconclusiveAnalysis) != nil || r.Spec.Paused || isScalingEvent { return c.syncReplicasOnly(r, rsList, isScalingEvent) } diff --git a/rollout/experiment.go b/rollout/experiment.go index 7db38e7695..70e360d069 100644 --- a/rollout/experiment.go +++ b/rollout/experiment.go @@ -144,7 +144,7 @@ func (c *RolloutController) reconcileExperiments(roCtx *canaryContext) error { return c.cancelExperiments(roCtx, allExs) } - if roCtx.PauseContext().GetPauseCondition(v1alpha1.PauseReasonInconclusiveAnalysis) != nil { + if getPauseCondition(rollout, v1alpha1.PauseReasonInconclusiveAnalysis) != nil { return nil } diff --git a/rollout/pause.go b/rollout/pause.go index b9485bf8a9..92249721f3 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -105,9 +105,9 @@ func (pCtx *pauseContext) CalculatePauseStatus(newStatus *v1alpha1.RolloutStatus newStatus.PauseConditions = newPauseConditions } -func (pCtx *pauseContext) GetPauseCondition(reason v1alpha1.PauseReason) *v1alpha1.PauseCondition { - for i := range pCtx.rollout.Status.PauseConditions { - cond := pCtx.rollout.Status.PauseConditions[i] +func getPauseCondition(rollout *v1alpha1.Rollout, reason v1alpha1.PauseReason) *v1alpha1.PauseCondition { + for i := range rollout.Status.PauseConditions { + cond := rollout.Status.PauseConditions[i] if cond.Reason == reason { return &cond } @@ -117,7 +117,7 @@ func (pCtx *pauseContext) GetPauseCondition(reason v1alpha1.PauseReason) *v1alph func (pCtx *pauseContext) CompletedPauseStep(pause v1alpha1.RolloutPause) bool { rollout := pCtx.rollout - pauseCondition := pCtx.GetPauseCondition(v1alpha1.PauseReasonCanaryPauseStep) + pauseCondition := getPauseCondition(rollout, v1alpha1.PauseReasonCanaryPauseStep) if pause.Duration != nil { now := metav1.Now() diff --git a/rollout/sync.go b/rollout/sync.go index 8097d860c6..6c95b48906 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -210,8 +210,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) - isControllerPaused := len(r.Status.PauseConditions) > 0 - logCtx.Infof("Syncing replicas only (controllerPaused: %v, userPaused %v, isScaling: %v)", isControllerPaused, r.Spec.Paused, isScaling) + logCtx.Infof("Syncing replicas only (userPaused %v, isScaling: %v)", r.Spec.Paused, isScaling) newRS, oldRSs, err := c.getAllReplicaSetsAndSyncRevision(r, rsList, false) if err != nil { return err @@ -228,7 +227,6 @@ func (c *RolloutController) syncReplicasOnly(r *v1alpha1.Rollout, rsList []*apps // so we can abort this resync return err } - 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.