Fix Infinite loop with PreviewReplicaCount set (#308)

* Fix Infinite loop with PreviewReplicaCount set * Use xlarge instead of large
argoproj · Dec 5, 2019 · 1dd86d7 · 1dd86d7
1 parent baccc9d
commit 1dd86d7
Show file tree

Hide file tree

Showing 4 changed files with 114 additions and 31 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -5,7 +5,7 @@ jobs:
     docker:
       # CircleCI Go images available at: https://hub.docker.com/r/circleci/golang/
       - image: circleci/golang:1.13.1
-    resource_class: large
+    resource_class: xlarge
     environment:
       TEST_RESULTS: /tmp/test-results
     steps:

diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go
@@ -262,15 +262,23 @@ func (c *RolloutController) syncRolloutStatusBlueGreen(previewSvc *corev1.Servic
 func calculateScaleUpPreviewCheckPoint(roCtx *blueGreenContext, activeRS *appsv1.ReplicaSet) bool {
 	r := roCtx.Rollout()
 	newRS := roCtx.NewRS()
-	newRSAvailableCount := replicasetutil.GetAvailableReplicaCountForReplicaSets([]*appsv1.ReplicaSet{newRS})
-	if r.Spec.Strategy.BlueGreen.PreviewReplicaCount != nil && newRSAvailableCount == *r.Spec.Strategy.BlueGreen.PreviewReplicaCount {
-		return true
-	} else if reconcileBlueGreenTemplateChange(roCtx) {
+
+	if reconcileBlueGreenTemplateChange(roCtx) || r.Spec.Strategy.BlueGreen.PreviewReplicaCount == nil {
 		return false
-	} else if newRS != nil && activeRS != nil && activeRS.Name == newRS.Name {
+	}
+
+	if newRS == nil || activeRS == nil || activeRS.Name == newRS.Name {
 		return false
 	}
-	return r.Status.BlueGreen.ScaleUpPreviewCheckPoint
+
+	// Once the ScaleUpPreviewCheckPoint is set to true, the rollout should keep that value until
+	// the newRS becomes the new activeRS or there is a template change.
+	if r.Status.BlueGreen.ScaleUpPreviewCheckPoint {
+		return r.Status.BlueGreen.ScaleUpPreviewCheckPoint
+	}
+
+	newRSAvailableCount := replicasetutil.GetAvailableReplicaCountForReplicaSets([]*appsv1.ReplicaSet{newRS})
+	return newRSAvailableCount == *r.Spec.Strategy.BlueGreen.PreviewReplicaCount
 }
 
 // Should run only on scaling events and not during the normal rollout process.
@@ -303,18 +311,6 @@ func (c *RolloutController) scaleBlueGreen(rollout *v1alpha1.Rollout, newRS *app
 		}
 	}
 
-	previewRS, _ := replicasetutil.GetReplicaSetByTemplateHash(allRS, rollout.Status.BlueGreen.PreviewSelector)
-	if previewRS != nil {
-		previewReplicas := rolloutReplicas
-		if rollout.Spec.Strategy.BlueGreen.PreviewReplicaCount != nil && !rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint {
-			previewReplicas = *rollout.Spec.Strategy.BlueGreen.PreviewReplicaCount
-		}
-		if *(previewRS.Spec.Replicas) != previewReplicas {
-			_, _, err := c.scaleReplicaSetAndRecordEvent(previewRS, previewReplicas, rollout)
-			return err
-		}
-	}
-
 	if newRS != nil {
 		newRSReplicaCount, err := replicasetutil.NewRSNewReplicas(rollout, allRS, newRS)
 		if err != nil {

diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go
@@ -742,6 +742,105 @@ func TestBlueGreenRolloutScaleUpdateActiveRS(t *testing.T) {
 	f.run(getKey(r2, t))
 }
 
+func TestPreviewReplicaCountHandleScaleUpPreviewCheckPoint(t *testing.T) {
+	t.Run("TrueAfterMeetingMinAvailable", func(t *testing.T) {
+		f := newFixture(t)
+		defer f.Close()
+
+		r1 := newBlueGreenRollout("foo", 5, nil, "active", "")
+		r1.Spec.Strategy.BlueGreen.PreviewReplicaCount = pointer.Int32Ptr(3)
+		r1.Spec.Strategy.BlueGreen.AutoPromotionEnabled = pointer.BoolPtr(false)
+		rs1 := newReplicaSetWithStatus(r1, 5, 5)
+		r2 := bumpVersion(r1)
+
+		rs2 := newReplicaSetWithStatus(r2, 3, 3)
+		f.kubeobjects = append(f.kubeobjects, rs1, rs2)
+		f.replicaSetLister = append(f.replicaSetLister, rs1, rs2)
+
+		rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+		rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+
+		activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash})
+
+		r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, 3, 3, 8, 5, false, true)
+		f.rolloutLister = append(f.rolloutLister, r2)
+		f.objects = append(f.objects, r2)
+		f.kubeobjects = append(f.kubeobjects, activeSvc)
+		f.serviceLister = append(f.serviceLister, activeSvc)
+
+		patchIndex := f.expectPatchRolloutAction(r1)
+		f.run(getKey(r2, t))
+		patch := f.getPatchedRollout(patchIndex)
+
+		assert.Contains(t, patch, `"scaleUpPreviewCheckPoint":true`)
+
+	})
+	t.Run("FalseAfterActiveServiceSwitch", func(t *testing.T) {
+		f := newFixture(t)
+		defer f.Close()
+
+		r1 := newBlueGreenRollout("foo", 5, nil, "active", "")
+		r1.Spec.Strategy.BlueGreen.PreviewReplicaCount = pointer.Int32Ptr(3)
+		r1.Spec.Strategy.BlueGreen.AutoPromotionEnabled = pointer.BoolPtr(false)
+		rs1 := newReplicaSetWithStatus(r1, 5, 5)
+		now := metav1.Now().Add(10 * time.Second)
+		rs1.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey] = now.UTC().Format(time.RFC3339)
+		r2 := bumpVersion(r1)
+
+		rs2 := newReplicaSetWithStatus(r2, 5, 5)
+		f.kubeobjects = append(f.kubeobjects, rs1, rs2)
+		f.replicaSetLister = append(f.replicaSetLister, rs1, rs2)
+
+		rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+		rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+
+		activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash})
+
+		r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, 5, 5, 8, 5, false, true)
+		r2.Status.BlueGreen.ScaleUpPreviewCheckPoint = true
+		f.rolloutLister = append(f.rolloutLister, r2)
+		f.objects = append(f.objects, r2)
+		f.kubeobjects = append(f.kubeobjects, activeSvc)
+		f.serviceLister = append(f.serviceLister, activeSvc)
+
+		f.expectPatchReplicaSetAction(rs1)
+		patchIndex := f.expectPatchRolloutAction(r1)
+
+		f.run(getKey(r2, t))
+		patch := f.getPatchedRollout(patchIndex)
+		assert.Contains(t, patch, `"scaleUpPreviewCheckPoint":null`)
+	})
+	t.Run("TrueWhenScalingUpPreview", func(t *testing.T) {
+		f := newFixture(t)
+		defer f.Close()
+
+		r1 := newBlueGreenRollout("foo", 5, nil, "active", "")
+		r1.Spec.Strategy.BlueGreen.PreviewReplicaCount = pointer.Int32Ptr(3)
+		rs1 := newReplicaSetWithStatus(r1, 5, 5)
+		r2 := bumpVersion(r1)
+
+		rs2 := newReplicaSetWithStatus(r2, 3, 3)
+		f.kubeobjects = append(f.kubeobjects, rs1, rs2)
+		f.replicaSetLister = append(f.replicaSetLister, rs1, rs2)
+
+		rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+		rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+
+		activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash})
+
+		r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs1PodHash, 5, 5, 8, 5, false, true)
+		r2.Status.BlueGreen.ScaleUpPreviewCheckPoint = true
+		f.rolloutLister = append(f.rolloutLister, r2)
+		f.objects = append(f.objects, r2)
+		f.kubeobjects = append(f.kubeobjects, activeSvc)
+		f.serviceLister = append(f.serviceLister, activeSvc)
+
+		f.expectUpdateReplicaSetAction(rs1)
+		f.expectPatchRolloutAction(r2)
+		f.run(getKey(r2, t))
+	})
+}
+
 func TestBlueGreenRolloutIgnoringScalingUsePreviewRSCount(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()

diff --git a/rollout/replicaset.go b/rollout/replicaset.go
@@ -77,18 +77,6 @@ func (c *RolloutController) reconcileNewReplicaSet(roCtx rolloutContext) (bool,
 	}
 	roCtx.Log().Infof("Reconciling new ReplicaSet '%s'", newRS.Name)
 	allRSs := roCtx.AllRSs()
-	if rollout.Spec.Strategy.BlueGreen != nil {
-		rolloutReplicas := defaults.GetReplicasOrDefault(rollout.Spec.Replicas)
-		if *(newRS.Spec.Replicas) == rolloutReplicas {
-			// Scaling not required.
-			return false, nil
-		}
-		if *(newRS.Spec.Replicas) > rolloutReplicas {
-			// Scale down.
-			scaled, _, err := c.scaleReplicaSetAndRecordEvent(newRS, rolloutReplicas, rollout)
-			return scaled, err
-		}
-	}
 	newReplicasCount, err := replicasetutil.NewRSNewReplicas(rollout, allRSs, newRS)
 	if err != nil {
 		return false, err