From 827ce595c8d011c1bba05d48ffc0d110d27f438b Mon Sep 17 00:00:00 2001 From: benminter-treatwell <84128121+benminter-treatwell@users.noreply.github.com> Date: Tue, 6 Aug 2024 16:36:00 +0100 Subject: [PATCH] =?UTF-8?q?fix(controller):=20use=20the=20stableRS=20from?= =?UTF-8?q?=20the=20rollout=20context=20rather=20tha=E2=80=A6=20(#3664)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * fix(controller): update tests which were relying on this bug(?) Signed-off-by: ben.minter * fix(controller): add clarity to comment in the case there is no stableRS Signed-off-by: ben.minter * 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 --------- Signed-off-by: ben.minter --- rollout/analysis_test.go | 3 +- rollout/bluegreen.go | 16 +++--- rollout/bluegreen_test.go | 89 +++++++++++++++++++++++++++++++ rollout/ephemeralmetadata_test.go | 2 +- 4 files changed, 98 insertions(+), 12 deletions(-) diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index e12a898c1a..fa7fdc5b96 100644 --- a/rollout/analysis_test.go +++ b/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"} diff --git a/rollout/bluegreen.go b/rollout/bluegreen.go index 9904de9dab..7613852470 100644 --- a/rollout/bluegreen.go +++ b/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 } diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 2ff9515fe3..b78ed8704d 100644 --- a/rollout/bluegreen_test.go +++ b/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) diff --git a/rollout/ephemeralmetadata_test.go b/rollout/ephemeralmetadata_test.go index 59daf1be0e..8241a1831c 100644 --- a/rollout/ephemeralmetadata_test.go +++ b/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{