fix(controller): use the stableRS from the rollout context rather tha… (argoproj#3664)

* fix(controller): use the stableRS from the rollout context rather than inferring it from the active selector, to deal with the edge case where the stableRS != activeRS during analysis templates

Signed-off-by: ben.minter <[email protected]>

* fix(controller): update tests which were relying on this bug(?)

Signed-off-by: ben.minter <[email protected]>

* fix(controller): add clarity to comment in the case there is no stableRS

Signed-off-by: ben.minter <[email protected]>

* fix(controller): add a test to assert that the stablers is not scaled by the reconiliation on start, by checking the log

Signed-off-by: ben.minter <[email protected]>

---------

Signed-off-by: ben.minter <[email protected]>

Author: benminter-treatwell

rollout/analysis_test.go

@@ -2532,7 +2532,7 @@ func TestRolloutPostPromotionAnalysisSuccess(t *testing.T) {
 		Status: v1alpha1.AnalysisPhaseRunning,
 	}
 
-	rs1 := newReplicaSetWithStatus(r1, 0, 0)
+	rs1 := newReplicaSetWithStatus(r1, 1, 1)
 	rs2 := newReplicaSetWithStatus(r2, 1, 1)
 	rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
 	rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
@@ -2558,6 +2558,7 @@ func TestRolloutPostPromotionAnalysisSuccess(t *testing.T) {
 	patch := f.getPatchedRollout(patchIndex)
 	expectedPatch := fmt.Sprintf(`{
 		"status": {
+			"replicas":2,
 			"stableRS": "%s",
 			"blueGreen": {
 				"postPromotionAnalysisRunStatus":{"status":"Successful"}

rollout/bluegreen.go

@@ -71,18 +71,14 @@ func (c *rolloutContext) rolloutBlueGreen() error {
 	return c.syncRolloutStatusBlueGreen(previewSvc, activeSvc)
 }
 
-func (c *rolloutContext) reconcileBlueGreenStableReplicaSet(activeSvc *corev1.Service) error {
-	if _, ok := activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]; !ok {
-		return nil
-	}
-	activeRS, _ := replicasetutil.GetReplicaSetByTemplateHash(c.allRSs, activeSvc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey])
-	if activeRS == nil {
-		c.log.Warn("There shouldn't be a nil active replicaset if the active Service selector is set")
+func (c *rolloutContext) reconcileBlueGreenStableReplicaSet() error {
+	if c.stableRS == nil {
+		c.log.Info("Stable ReplicaSet doesn't exist and hence no reconciliation is required.")
 		return nil
 	}
 
-	c.log.Infof("Reconciling stable ReplicaSet '%s'", activeRS.Name)
-	_, _, err := c.scaleReplicaSetAndRecordEvent(activeRS, defaults.GetReplicasOrDefault(c.rollout.Spec.Replicas))
+	c.log.Infof("Reconciling stable ReplicaSet '%s'", c.stableRS.Name)
+	_, _, err := c.scaleReplicaSetAndRecordEvent(c.stableRS, defaults.GetReplicasOrDefault(c.rollout.Spec.Replicas))
 	if err != nil {
 		return fmt.Errorf("failed to scaleReplicaSetAndRecordEvent in reconcileBlueGreenStableReplicaSet: %w", err)
 	}
@@ -94,7 +90,7 @@ func (c *rolloutContext) reconcileBlueGreenReplicaSets(activeSvc *corev1.Service
 	if err != nil {
 		return err
 	}
-	err = c.reconcileBlueGreenStableReplicaSet(activeSvc)
+	err = c.reconcileBlueGreenStableReplicaSet()
 	if err != nil {
 		return err
 	}

rollout/bluegreen_test.go

@@ -1,12 +1,15 @@
 package rollout
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
 
+	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -1007,6 +1010,92 @@ func TestBlueGreenRolloutScaleUpdateActiveRS(t *testing.T) {
 	f.run(getKey(r2, t))
 }
 
+func TestBlueGreenRolloutScaleUpdateStableRS(t *testing.T) {
+	f := newFixture(t)
+	defer f.Close()
+
+	r1 := newBlueGreenRollout("foo", 1, nil, "active", "")
+	rs1 := newReplicaSetWithStatus(r1, 1, 1)
+	r2 := bumpVersion(r1)
+
+	rs2 := newReplicaSetWithStatus(r2, 1, 1)
+	f.kubeobjects = append(f.kubeobjects, rs1, rs2)
+	f.replicaSetLister = append(f.replicaSetLister, rs1, rs2)
+
+	rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+	rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+
+	// Make the new RS the active and the old one stable to simulate post-promotion analysis step.
+	r2 = updateBlueGreenRolloutStatus(r2, rs2PodHash, rs2PodHash, rs1PodHash, 1, 1, 2, 1, false, true, false)
+
+	f.rolloutLister = append(f.rolloutLister, r2)
+
+	f.objects = append(f.objects, r2)
+	activeSvc := newService("active", 80, map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash}, r2)
+	f.kubeobjects = append(f.kubeobjects, activeSvc)
+	f.serviceLister = append(f.serviceLister, activeSvc)
+
+	f.expectPatchRolloutAction(r1)
+
+	// Patch the rollout to get it in the state we want (old RS is stable and new is active)
+	f.run(getKey(r2, t))
+	// Actually update the replicas now that we are in the desired state (old RS is stable and new is active)
+	r2.Spec.Replicas = pointer.Int32Ptr(2)
+
+	f.expectUpdateReplicaSetAction(rs1)
+	f.expectUpdateReplicaSetAction(rs2)
+	f.run(getKey(r2, t))
+}
+
+func TestBlueGreenStableRSReconciliationShouldNotScaleOnFirstTimeRollout(t *testing.T) {
+	f := newFixture(t)
+	prevOutput := log.StandardLogger().Out
+	defer func() {
+		log.SetOutput(prevOutput)
+	}()
+	defer f.Close()
+
+	// Setup Logging capture
+	buf := bytes.NewBufferString("")
+	log.SetOutput(buf)
+
+	r := newBlueGreenRollout("foo", 1, nil, "active", "preview")
+	r.Status.Conditions = []v1alpha1.RolloutCondition{}
+	f.rolloutLister = append(f.rolloutLister, r)
+	f.objects = append(f.objects, r)
+	previewSvc := newService("preview", 80, nil, r)
+	activeSvc := newService("active", 80, nil, r)
+	f.kubeobjects = append(f.kubeobjects, previewSvc, activeSvc)
+	f.serviceLister = append(f.serviceLister, activeSvc, previewSvc)
+
+	rs := newReplicaSet(r, 1)
+	rsPodHash := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+	generatedConditions := generateConditionsPatchWithCompleted(false, conditions.ReplicaSetUpdatedReason, rs, false, "", true)
+
+	f.expectCreateReplicaSetAction(rs)
+	f.expectPatchServiceAction(previewSvc, rsPodHash)
+	f.expectUpdateReplicaSetAction(rs) // scale up RS
+	f.expectUpdateRolloutStatusAction(r)
+	expectedPatchWithoutSubs := `{
+		"status":{
+			"blueGreen" : {
+				"previewSelector": "%s"
+			},
+			"conditions": %s,
+			"selector": "foo=bar",
+			"stableRS": "%s",
+			"phase": "Progressing",
+			"message": "more replicas need to be updated"
+		}
+	}`
+	expectedPatch := calculatePatch(r, fmt.Sprintf(expectedPatchWithoutSubs, rsPodHash, generatedConditions, rsPodHash))
+	f.expectPatchRolloutActionWithPatch(r, expectedPatch)
+	f.run(getKey(r, t))
+
+	logMessage := buf.String()
+	assert.True(t, strings.Contains(logMessage, "msg=\"Stable ReplicaSet doesn't exist and hence no reconciliation is required.\""), logMessage)
+}
+
 func TestPreviewReplicaCountHandleScaleUpPreviewCheckPoint(t *testing.T) {
 	t.Run("TrueAfterMeetingMinAvailable", func(t *testing.T) {
 		f := newFixture(t)

rollout/ephemeralmetadata_test.go

@@ -169,7 +169,7 @@ func TestSyncBlueGreenEphemeralMetadataSecondRevision(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()
 
-	r1 := newBlueGreenRollout("foo", 1, nil, "active", "preview")
+	r1 := newBlueGreenRollout("foo", 3, nil, "active", "preview")
 	r1.Spec.Strategy.BlueGreen.AutoPromotionEnabled = pointer.BoolPtr(false)
 	r1.Annotations[annotations.RevisionAnnotation] = "1"
 	r1.Spec.Strategy.BlueGreen.PreviewMetadata = &v1alpha1.PodTemplateMetadata{