fix: Promote full did not work against BlueGreen with previewReplicaC…

…ount (#1384) Signed-off-by: Jesse Suen <[email protected]>
argoproj · Jul 30, 2021 · c63b6c1 · c63b6c1
1 parent 4622ae4
commit c63b6c1
Show file tree

Hide file tree

Showing 5 changed files with 123 additions and 55 deletions.
diff --git a/test/e2e/bluegreen_test.go b/test/e2e/bluegreen_test.go
@@ -278,3 +278,103 @@ spec:
 		Then().
 		ExpectActiveRevision("2")
 }
+
+// TestBlueGreenPreviewReplicaCount verifies the previewReplicaCount feature
+func (s *BlueGreenSuite) TestBlueGreenPreviewReplicaCount() {
+	s.Given().
+		RolloutObjects(newService("bluegreen-preview-replicas-active")).
+		RolloutObjects(newService("bluegreen-preview-replicas-preview")).
+		RolloutObjects(`
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: bluegreen-preview-replicas
+spec:
+  replicas: 2
+  strategy:
+    blueGreen:
+      activeService: bluegreen-preview-replicas-active
+      previewService: bluegreen-preview-replicas-preview
+      previewReplicaCount: 1
+      scaleDownDelaySeconds: 5
+      autoPromotionEnabled: false
+  selector:
+    matchLabels:
+      app: bluegreen-preview-replicas
+  template:
+    metadata:
+      labels:
+        app: bluegreen-preview-replicas
+    spec:
+      containers:
+      - name: bluegreen-preview-replicas
+        image: nginx:1.19-alpine
+        resources:
+          requests:
+            memory: 16Mi
+            cpu: 1m
+`).
+		When().
+		ApplyManifests().
+		WaitForRolloutStatus("Healthy").
+		UpdateSpec().
+		WaitForRolloutStatus("Paused").
+		Then().
+		ExpectRevisionPodCount("2", 1).
+		ExpectRevisionPodCount("1", 2).
+		ExpectReplicaCounts(2, 3, 1, 2, 2). // desired, current, updated, ready, available
+		When().
+		PromoteRollout().
+		WaitForRolloutStatus("Healthy").
+		Then().
+		ExpectReplicaCounts(2, 4, 2, 2, 2)
+}
+
+// TestBlueGreenPreviewReplicaCountPromoteFull verifies promote full works with previewReplicaCount
+func (s *FunctionalSuite) TestBlueGreenPreviewReplicaCountPromoteFull() {
+	s.Given().
+		RolloutObjects(newService("bluegreen-preview-replicas-active")).
+		RolloutObjects(`
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: bluegreen-preview-replicas-promote-full
+spec:
+  replicas: 2
+  progressDeadlineSeconds: 1   # use a very short value to cause Degraded condition frequently
+  strategy:
+    blueGreen:
+      activeService: bluegreen-preview-replicas-active
+      previewReplicaCount: 1
+      autoPromotionEnabled: false
+  selector:
+    matchLabels:
+      app: bluegreen-preview-replicas-promote-full
+  template:
+    metadata:
+      labels:
+        app: bluegreen-preview-replicas-promote-full
+    spec:
+      containers:
+      - name: bluegreen-preview-replicas-promote-full
+        image: nginx:1.19-alpine
+        resources:
+          requests:
+            memory: 16Mi
+            cpu: 1m
+`).
+		When().
+		ApplyManifests().
+		WaitForRolloutStatus("Healthy").
+		UpdateSpec().
+		WaitForRolloutStatus("Paused").
+		Sleep(2*time.Second). // sleep for longer than progressDeadlineSeconds
+		Then().
+		ExpectRolloutStatus("Paused").      // the fact that we are paused for longer than progressDeadlineSeconds, should not cause Degraded
+		ExpectReplicaCounts(2, 3, 1, 2, 2). // desired, current, updated, ready, available
+		When().
+		PromoteRolloutFull().
+		WaitForRolloutStatus("Healthy").
+		Then().
+		ExpectReplicaCounts(2, 4, 2, 2, 2)
+}
diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go
@@ -706,57 +706,6 @@ func (s *FunctionalSuite) TestBlueGreenUpdate() {
 		})
 }
 
-// TestBlueGreenPreviewReplicaCount verifies the previewReplicaCount feature
-func (s *FunctionalSuite) TestBlueGreenPreviewReplicaCount() {
-	s.Given().
-		RolloutObjects(newService("bluegreen-preview-replicas-active")).
-		RolloutObjects(newService("bluegreen-preview-replicas-preview")).
-		RolloutObjects(`
-apiVersion: argoproj.io/v1alpha1
-kind: Rollout
-metadata:
-  name: bluegreen-preview-replicas
-spec:
-  replicas: 2
-  strategy:
-    blueGreen:
-      activeService: bluegreen-preview-replicas-active
-      previewService: bluegreen-preview-replicas-preview
-      previewReplicaCount: 1
-      scaleDownDelaySeconds: 5
-      autoPromotionEnabled: false
-  selector:
-    matchLabels:
-      app: bluegreen-preview-replicas
-  template:
-    metadata:
-      labels:
-        app: bluegreen-preview-replicas
-    spec:
-      containers:
-      - name: bluegreen-preview-replicas
-        image: nginx:1.19-alpine
-        resources:
-          requests:
-            memory: 16Mi
-            cpu: 1m
-`).
-		When().
-		ApplyManifests().
-		WaitForRolloutStatus("Healthy").
-		UpdateSpec().
-		WaitForRolloutStatus("Paused").
-		Then().
-		ExpectRevisionPodCount("2", 1).
-		ExpectRevisionPodCount("1", 2).
-		ExpectReplicaCounts(2, 3, 1, 2, 2). // desired, current, updated, ready, available
-		When().
-		PromoteRollout().
-		WaitForRolloutStatus("Healthy").
-		Then().
-		ExpectReplicaCounts(2, 4, 2, 2, 2)
-}
-
 // TestBlueGreenToCanary tests behavior when migrating from bluegreen to canary
 func (s *FunctionalSuite) TestBlueGreenToCanary() {
 	s.Given().

diff --git a/test/fixtures/when.go b/test/fixtures/when.go
@@ -189,6 +189,7 @@ func (w *When) ScaleRollout(scale int) *When {
 }
 
 func (w *When) Sleep(d time.Duration) *When {
+	w.log.Infof("Sleeping %s", d)
 	time.Sleep(d)
 	return w
 }

diff --git a/utils/replicaset/replicaset.go b/utils/replicaset/replicaset.go
@@ -233,22 +233,32 @@ func FindOldReplicaSets(rollout *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet)
 // 2) Max number of pods allowed is reached: deployment's replicas + maxSurge == all RSs' replicas
 func NewRSNewReplicas(rollout *v1alpha1.Rollout, allRSs []*appsv1.ReplicaSet, newRS *appsv1.ReplicaSet) (int32, error) {
 	if rollout.Spec.Strategy.BlueGreen != nil {
+		desiredReplicas := defaults.GetReplicasOrDefault(rollout.Spec.Replicas)
 		if rollout.Spec.Strategy.BlueGreen.PreviewReplicaCount != nil {
 			activeRS, _ := GetReplicaSetByTemplateHash(allRSs, rollout.Status.BlueGreen.ActiveSelector)
 			if activeRS == nil || activeRS.Name == newRS.Name {
-				return defaults.GetReplicasOrDefault(rollout.Spec.Replicas), nil
+				// the active RS is our desired RS. we are already past the blue-green promote step
+				return desiredReplicas, nil
+			}
+			if rollout.Status.PromoteFull {
+				// we are doing a full promotion. ignore previewReplicaCount
+				return desiredReplicas, nil
 			}
 			if newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] != rollout.Status.CurrentPodHash {
+				// the desired RS is not equal to our previously recorded current RS.
+				// This must be a new update, so return previewReplicaCount
 				return *rollout.Spec.Strategy.BlueGreen.PreviewReplicaCount, nil
 			}
 			isNotPaused := !rollout.Spec.Paused && len(rollout.Status.PauseConditions) == 0
 			if isNotPaused && rollout.Status.BlueGreen.ScaleUpPreviewCheckPoint {
-				return defaults.GetReplicasOrDefault(rollout.Spec.Replicas), nil
+				// We are not paused, but we are already past our preview scale up checkpoint.
+				// If we get here, we were resumed after the pause, but haven't yet flipped the
+				// active service switch to the desired RS.
+				return desiredReplicas, nil
 			}
 			return *rollout.Spec.Strategy.BlueGreen.PreviewReplicaCount, nil
 		}
-
-		return defaults.GetReplicasOrDefault(rollout.Spec.Replicas), nil
+		return desiredReplicas, nil
 	}
 	if rollout.Spec.Strategy.Canary != nil {
 		stableRS := GetStableRS(rollout, newRS, allRSs)

diff --git a/utils/replicaset/replicaset_test.go b/utils/replicaset/replicaset_test.go
@@ -225,6 +225,7 @@ func TestNewRSNewReplicasWitPreviewReplicaCount(t *testing.T) {
 		overrideCurrentPodHash   string
 		scaleUpPreviewCheckpoint bool
 		expectReplicaCount       int32
+		promoteFull              bool
 	}{
 		{
 			name:               "No active rs is set",
@@ -253,6 +254,12 @@ func TestNewRSNewReplicasWitPreviewReplicaCount(t *testing.T) {
 			activeSelector:     "bar",
 			expectReplicaCount: previewReplicaCount,
 		},
+		{
+			name:               "Ignore preview replica count during promote full",
+			activeSelector:     "bar",
+			expectReplicaCount: replicaCount,
+			promoteFull:        true,
+		},
 	}
 	for i := range tests {
 		test := tests[i]
@@ -272,6 +279,7 @@ func TestNewRSNewReplicasWitPreviewReplicaCount(t *testing.T) {
 						ActiveSelector:           test.activeSelector,
 					},
 					CurrentPodHash: "foo",
+					PromoteFull:    test.promoteFull,
 				},
 			}
 			if test.overrideCurrentPodHash != "" {