fix: rollout paused longer than progressDeadlineSeconds would briefly degrade (argoproj#1268)

* Do not exit status cmd for ProgressDeadlineExceeded error

Signed-off-by: khhirani <[email protected]>

* Do not return status if RO is paused in sync.go

Signed-off-by: khhirani <[email protected]>

Author: khhirani

pkg/kubectl-argo-rollouts/cmd/status/status.go

@@ -65,8 +65,8 @@ func NewCmdStatus(o *options.ArgoRolloutsOptions) *cobra.Command {
 				controller.RegisterCallback(func(roInfo *rollout.RolloutInfo) {
 					rolloutUpdates <- roInfo
 				})
-				go statusOptions.WatchStatus(ctx.Done(), cancel, statusOptions.Timeout, rolloutUpdates)
-				controller.Run(ctx)
+				go controller.Run(ctx)
+				statusOptions.WatchStatus(ctx.Done(), rolloutUpdates)
 
 				finalRi, err := controller.GetRolloutInfo()
 				if err != nil {
@@ -88,14 +88,14 @@ func NewCmdStatus(o *options.ArgoRolloutsOptions) *cobra.Command {
 	return cmd
 }
 
-func (o *StatusOptions) WatchStatus(stopCh <-chan struct{}, cancelFunc context.CancelFunc, timeoutDuration time.Duration, rolloutUpdates chan *rollout.RolloutInfo) {
+func (o *StatusOptions) WatchStatus(stopCh <-chan struct{}, rolloutUpdates chan *rollout.RolloutInfo) string {
 	timeout := make(chan bool)
 	var roInfo *rollout.RolloutInfo
 	var preventFlicker time.Time
 
-	if timeoutDuration != 0 {
+	if o.Timeout != 0 {
 		go func() {
-			time.Sleep(timeoutDuration)
+			time.Sleep(o.Timeout)
 			timeout <- true
 		}()
 	}
@@ -105,18 +105,16 @@ func (o *StatusOptions) WatchStatus(stopCh <-chan struct{}, cancelFunc context.C
 		case roInfo = <-rolloutUpdates:
 			if roInfo != nil && roInfo.Status == "Healthy" || roInfo.Status == "Degraded" {
 				fmt.Fprintln(o.Out, roInfo.Status)
-				cancelFunc()
-				return
+				return roInfo.Status
 			}
 			if roInfo != nil && time.Now().After(preventFlicker.Add(200*time.Millisecond)) {
 				fmt.Fprintf(o.Out, "%s - %s\n", roInfo.Status, roInfo.Message)
 				preventFlicker = time.Now()
 			}
 		case <-stopCh:
-			return
+			return ""
 		case <-timeout:
-			cancelFunc()
-			return
+			return ""
 		}
 	}
 }

rollout/analysis_test.go

@@ -1475,6 +1475,9 @@ func TestDoNotCreateBackgroundAnalysisRunAfterInconclusiveRun(t *testing.T) {
 	pausedCondition, _ := newPausedCondition(true)
 	conditions.SetRolloutCondition(&r2.Status, pausedCondition)
 
+	availableCondition, _ := newAvailableCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, availableCondition)
+
 	f.rolloutLister = append(f.rolloutLister, r2)
 	f.analysisTemplateLister = append(f.analysisTemplateLister, at)
 	f.objects = append(f.objects, r2, at)
@@ -1844,7 +1847,7 @@ func TestRolloutPrePromotionAnalysisSwitchServiceAfterSuccess(t *testing.T) {
 	f.expectPatchReplicaSetAction(rs1)
 	patchIndex := f.expectPatchRolloutActionWithPatch(r2, OnlyObservedGenerationPatch)
 	f.run(getKey(r2, t))
-	patch := f.getPatchedRollout(patchIndex)
+	patch := f.getPatchedRolloutWithoutConditions(patchIndex)
 	expectedPatch := fmt.Sprintf(`{
 		"status": {
 			"blueGreen": {
@@ -1912,7 +1915,7 @@ func TestRolloutPrePromotionAnalysisHonorAutoPromotionSeconds(t *testing.T) {
 	f.expectPatchReplicaSetAction(rs1)
 	patchIndex := f.expectPatchRolloutActionWithPatch(r2, OnlyObservedGenerationPatch)
 	f.run(getKey(r2, t))
-	patch := f.getPatchedRollout(patchIndex)
+	patch := f.getPatchedRolloutWithoutConditions(patchIndex)
 	expectedPatch := fmt.Sprintf(`{
 		"status": {
 			"blueGreen": {
@@ -1965,6 +1968,9 @@ func TestRolloutPrePromotionAnalysisDoNothingOnInconclusiveAnalysis(t *testing.T
 	pausedCondition, _ := newPausedCondition(true)
 	conditions.SetRolloutCondition(&r2.Status, pausedCondition)
 
+	availableCondition, _ := newAvailableCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, availableCondition)
+
 	activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash}
 	activeSvc := newService("active", 80, activeSelector, r2)
 

rollout/bluegreen_test.go

@@ -440,7 +440,7 @@ func TestBlueGreenHandlePause(t *testing.T) {
 		patchRolloutIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch)
 		f.run(getKey(r2, t))
 
-		rolloutPatch := f.getPatchedRollout(patchRolloutIndex)
+		rolloutPatch := f.getPatchedRolloutWithoutConditions(patchRolloutIndex)
 		assert.Equal(t, expectedPatch, rolloutPatch)
 	})
 
@@ -798,7 +798,7 @@ func TestBlueGreenRolloutStatusHPAStatusFieldsActiveSelectorSet(t *testing.T) {
 	patchIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch)
 	f.run(getKey(r2, t))
 
-	rolloutPatch := f.getPatchedRollout(patchIndex)
+	rolloutPatch := f.getPatchedRolloutWithoutConditions(patchIndex)
 	assert.Equal(t, expectedPatch, rolloutPatch)
 }
 
@@ -1060,6 +1060,8 @@ func TestBlueGreenRolloutCompletedFalse(t *testing.T) {
 	conditions.SetRolloutCondition(&r2.Status, progressingCondition)
 	pausedCondition, _ := newPausedCondition(true)
 	conditions.SetRolloutCondition(&r2.Status, pausedCondition)
+	availableCondition, _ := newAvailableCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, availableCondition)
 
 	f.kubeobjects = append(f.kubeobjects, rs2)
 	f.replicaSetLister = append(f.replicaSetLister, rs2)
@@ -1084,7 +1086,7 @@ func TestBlueGreenRolloutCompletedFalse(t *testing.T) {
 	err := json.Unmarshal([]byte(patch), &rolloutPatch)
 	assert.NoError(t, err)
 
-	index := len(rolloutPatch.Status.Conditions) - 1
+	index := len(rolloutPatch.Status.Conditions) - 2
 	assert.Equal(t, v1alpha1.RolloutCompleted, rolloutPatch.Status.Conditions[index].Type)
 	assert.Equal(t, corev1.ConditionFalse, rolloutPatch.Status.Conditions[index].Status)
 }
@@ -1322,3 +1324,74 @@ func TestBlueGreenAbort(t *testing.T) {
 	patch := f.getPatchedRollout(patchIndex)
 	assert.Equal(t, calculatePatch(r2, expectedPatch), patch)
 }
+
+func TestBlueGreenHandlePauseAutoPromoteWithConditions(t *testing.T) {
+	f := newFixture(t)
+	defer f.Close()
+
+	r1 := newBlueGreenRollout("foo", 1, nil, "active", "preview")
+	r2 := bumpVersion(r1)
+	r2.Spec.Strategy.BlueGreen.AutoPromotionSeconds = 10
+
+	rs1 := newReplicaSetWithStatus(r1, 1, 1)
+	rs2 := newReplicaSetWithStatus(r2, 1, 1)
+	rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+	rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+
+	r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, rs1PodHash, 1, 1, 2, 1, true, true)
+	now := metav1.Now()
+	before := metav1.NewTime(now.Add(-1 * time.Minute))
+	r2.Status.PauseConditions[0].StartTime = before
+	r2.Status.ControllerPause = true
+	progressingCondition, _ := newProgressingCondition(conditions.RolloutPausedReason, rs2, "")
+	conditions.SetRolloutCondition(&r2.Status, progressingCondition)
+
+	pausedCondition, _ := newPausedCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, pausedCondition)
+	r2.Status.Phase, r2.Status.Message = rolloututil.CalculateRolloutPhase(r2.Spec, r2.Status)
+
+	availableCondition, _ := newAvailableCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, availableCondition)
+	r2.Status.Phase, r2.Status.Message = rolloututil.CalculateRolloutPhase(r2.Spec, r2.Status)
+
+	activeSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash}
+	activeSvc := newService("active", 80, activeSelector, r2)
+	previewSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash}
+	previewSvc := newService("preview", 80, previewSelector, r2)
+
+	f.objects = append(f.objects, r2)
+	f.kubeobjects = append(f.kubeobjects, activeSvc, previewSvc, rs1, rs2)
+	f.rolloutLister = append(f.rolloutLister, r2)
+	f.replicaSetLister = append(f.replicaSetLister, rs1, rs2)
+	f.serviceLister = append(f.serviceLister, activeSvc, previewSvc)
+
+	expectedPatchWithoutSubs := `{
+			"status": {
+				"blueGreen": {
+					"activeSelector": "%s"
+				},
+				"conditions": [%s, %s, %s],
+				"stableRS": "%s",
+				"pauseConditions": null,
+				"controllerPause": null,
+				"selector": "foo=bar,rollouts-pod-template-hash=%s",
+				"phase": "Healthy",
+				"message": null
+			}
+		}`
+	availableCondBytes, err := json.Marshal(r2.Status.Conditions[0])
+	assert.Nil(t, err)
+	updatedProgressingCond, _ := newProgressingCondition(conditions.ReplicaSetUpdatedReason, rs2, fmt.Sprintf("ReplicaSet \"%s\" is progressing.", rs2.Name))
+	progressingCondBytes, err := json.Marshal(updatedProgressingCond)
+	assert.Nil(t, err)
+	pausedCondBytes, err := json.Marshal(r2.Status.Conditions[2])
+	assert.Nil(t, err)
+	expectedPatch := calculatePatch(r2, fmt.Sprintf(expectedPatchWithoutSubs, rs2PodHash, string(availableCondBytes), string(pausedCondBytes), string(progressingCondBytes), rs2PodHash, rs2PodHash))
+	f.expectPatchServiceAction(activeSvc, rs2PodHash)
+	f.expectPatchReplicaSetAction(rs1)
+	patchRolloutIndex := f.expectPatchRolloutActionWithPatch(r2, expectedPatch)
+	f.run(getKey(r2, t))
+
+	rolloutPatch := f.getPatchedRollout(patchRolloutIndex)
+	assert.Equal(t, expectedPatch, rolloutPatch)
+}

rollout/canary_test.go

@@ -230,6 +230,9 @@ func TestCanaryRolloutUpdatePauseConditionWhilePaused(t *testing.T) {
 	pausedCondition, _ := newPausedCondition(true)
 	conditions.SetRolloutCondition(&r2.Status, pausedCondition)
 
+	availableCondition, _ := newAvailableCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, availableCondition)
+
 	rs1 := newReplicaSetWithStatus(r1, 10, 10)
 	rs2 := newReplicaSetWithStatus(r2, 0, 0)
 
@@ -896,6 +899,9 @@ func TestSyncRolloutWaitAddToQueue(t *testing.T) {
 	pausedCondition, _ := newPausedCondition(true)
 	conditions.SetRolloutCondition(&r2.Status, pausedCondition)
 
+	availableCondition, _ := newAvailableCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, availableCondition)
+
 	r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation))
 	f.rolloutLister = append(f.rolloutLister, r2)
 	f.objects = append(f.objects, r2)
@@ -941,6 +947,9 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) {
 	pausedCondition, _ := newPausedCondition(true)
 	conditions.SetRolloutCondition(&r2.Status, pausedCondition)
 
+	availableCondition, _ := newAvailableCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, availableCondition)
+
 	f.rolloutLister = append(f.rolloutLister, r2)
 	f.objects = append(f.objects, r2)
 
@@ -997,7 +1006,7 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) {
 	patchIndex := f.expectPatchRolloutAction(r2)
 	f.run(getKey(r2, t))
 
-	patch := f.getPatchedRollout(patchIndex)
+	patch := f.getPatchedRolloutWithoutConditions(patchIndex)
 	expectedPatch := `{
 		"status":{
 			"controllerPause": null,
@@ -1048,7 +1057,7 @@ func TestCanaryRolloutStatusHPAStatusFields(t *testing.T) {
 	index := f.expectPatchRolloutActionWithPatch(r2, expectedPatchWithSub)
 	f.run(getKey(r2, t))
 
-	patch := f.getPatchedRollout(index)
+	patch := f.getPatchedRolloutWithoutConditions(index)
 	assert.Equal(t, calculatePatch(r2, expectedPatchWithSub), patch)
 }
 
@@ -1285,7 +1294,7 @@ func TestCanaryRolloutScaleWhilePaused(t *testing.T) {
 	updatedRS := f.getUpdatedReplicaSet(updatedIndex)
 	assert.Equal(t, int32(8), *updatedRS.Spec.Replicas)
 
-	patch := f.getPatchedRollout(patchIndex)
+	patch := f.getPatchedRolloutWithoutConditions(patchIndex)
 	expectedPatch := calculatePatch(r2, OnlyObservedGenerationPatch)
 	assert.Equal(t, expectedPatch, patch)
 }
@@ -1381,7 +1390,7 @@ func TestNoResumeAfterPauseDurationIfUserPaused(t *testing.T) {
 	_ = f.expectPatchRolloutAction(r2) // this just sets a conditions. ignore for now
 	patchIndex := f.expectPatchRolloutAction(r2)
 	f.run(getKey(r2, t))
-	patch := f.getPatchedRollout(patchIndex)
+	patch := f.getPatchedRolloutWithoutConditions(patchIndex)
 	expectedPatch := `{
 		"status": {
 			"message": "manually paused"
@@ -1419,6 +1428,9 @@ func TestHandleNilNewRSOnScaleAndImageChange(t *testing.T) {
 	pausedCondition, _ := newPausedCondition(true)
 	conditions.SetRolloutCondition(&r2.Status, pausedCondition)
 
+	availableCondition, _ := newAvailableCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, availableCondition)
+
 	f.kubeobjects = append(f.kubeobjects, rs1)
 	f.replicaSetLister = append(f.replicaSetLister, rs1)
 	f.rolloutLister = append(f.rolloutLister, r2)

rollout/controller_test.go

@@ -965,6 +965,25 @@ func (f *fixture) getPatchedRollout(index int) string {
 	return string(patchAction.GetPatch())
 }
 
+func (f *fixture) getPatchedRolloutWithoutConditions(index int) string {
+	action := filterInformerActions(f.client.Actions())[index]
+	patchAction, ok := action.(core.PatchAction)
+	if !ok {
+		f.t.Fatalf("Expected Patch action, not %s", action.GetVerb())
+	}
+	ro := make(map[string]interface{})
+	err := json.Unmarshal(patchAction.GetPatch(), &ro)
+	if err != nil {
+		f.t.Fatalf("Unable to unmarshal Patch")
+	}
+	unstructured.RemoveNestedField(ro, "status", "conditions")
+	roBytes, err := json.Marshal(ro)
+	if err != nil {
+		f.t.Fatalf("Unable to marshal Patch")
+	}
+	return string(roBytes)
+}
+
 func (f *fixture) getPatchedRolloutAsObject(index int) *v1alpha1.Rollout {
 	action := filterInformerActions(f.client.Actions())[index]
 	patchAction, ok := action.(core.PatchAction)

rollout/sync.go

@@ -571,11 +571,14 @@ func (c *rolloutContext) patchCondition(r *v1alpha1.Rollout, newStatus *v1alpha1
 	return nil
 }
 
-// isIndefiniteStep returns whether or not the rollout is at an Experiment or Analysis step which should
+// isIndefiniteStep returns whether or not the rollout is at an Experiment or Analysis or Pause step which should
 // not affect the progressDeadlineSeconds
 func isIndefiniteStep(r *v1alpha1.Rollout) bool {
 	currentStep, _ := replicasetutil.GetCurrentCanaryStep(r)
-	return currentStep != nil && (currentStep.Experiment != nil || currentStep.Analysis != nil)
+	if currentStep != nil && (currentStep.Experiment != nil || currentStep.Analysis != nil || currentStep.Pause != nil) {
+		return true
+	}
+	return false
 }
 
 func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutStatus) v1alpha1.RolloutStatus {
@@ -605,10 +608,6 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt
 		}
 	}
 
-	if isPaused {
-		return newStatus
-	}
-
 	// If there is only one replica set that is active then that means we are not running
 	// a new rollout and this is a resync where we don't need to estimate any progress.
 	// In such a case, we should simply not estimate any progress for this rollout.

test/e2e/functional_test.go

@@ -1237,3 +1237,40 @@ func (s *FunctionalSuite) TestControllerMetrics() {
 	assert.NoError(s.T(), err)
 	assert.Equal(s.T(), http.StatusOK, resp.StatusCode)
 }
+
+func (s *FunctionalSuite) TestRolloutPauseDurationGreaterThanProgressDeadlineSeconds() {
+	(s.Given().
+		HealthyRollout(`
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: rollout-canary
+spec:
+  replicas: 3
+  progressDeadlineSeconds: 5
+  selector:
+    matchLabels:
+      app: rollout-canary
+  template:
+    metadata:
+      labels:
+        app: rollout-canary
+    spec:
+      containers:
+      - name: rollouts-demo
+        image: nginx:1.19-alpine
+        ports:
+        - containerPort: 80
+  strategy:
+    canary:
+      steps:
+      - setWeight: 32
+      - pause: {duration: 30s}
+      - setWeight: 67
+`).
+		When().
+		UpdateSpec().
+		WatchRolloutStatus("Healthy").
+		Then().
+		ExpectRolloutStatus("Healthy"))
+}