From ca4acb86f086cd3e8b1a62bad4355e4b202b0107 Mon Sep 17 00:00:00 2001 From: Jesse Suen Date: Fri, 30 Jul 2021 15:12:19 -0700 Subject: [PATCH] fix: Promote full did not work against BlueGreen with previewReplicaCount Signed-off-by: Jesse Suen --- test/e2e/bluegreen_test.go | 100 ++++++++++++++++++++++++++++ test/e2e/functional_test.go | 51 -------------- test/fixtures/when.go | 1 + utils/replicaset/replicaset.go | 18 +++-- utils/replicaset/replicaset_test.go | 8 +++ 5 files changed, 123 insertions(+), 55 deletions(-) diff --git a/test/e2e/bluegreen_test.go b/test/e2e/bluegreen_test.go index 9c91c6c0ef..cc3061987a 100644 --- 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 index f038fa0fce..fc381773f3 100644 --- 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 index eb6791e831..81d44628e9 100644 --- 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 index 5d1c8fabcf..598b301ad1 100644 --- a/utils/replicaset/replicaset.go +++ b/utils/replicaset/replicaset.go @@ -234,22 +234,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 index 62b9c7b0da..09165dd113 100644 --- 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 != "" {