From cb4c244fe24b8f725ad46afbb4e460c38a7d2477 Mon Sep 17 00:00:00 2001 From: zachaller Date: Wed, 25 Jan 2023 13:52:00 -0600 Subject: [PATCH 1/5] wip Signed-off-by: zachaller --- rollout/trafficrouting.go | 7 +++++++ rollout/trafficrouting_test.go | 2 ++ 2 files changed, 9 insertions(+) diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go index 41e2db48be..b4626f1788 100644 --- a/rollout/trafficrouting.go +++ b/rollout/trafficrouting.go @@ -171,6 +171,13 @@ func (c *rolloutContext) reconcileTrafficRouting() error { return err } + // If we are aborting, we want to set the desired weight to 0, but we also want to set the canary service + // to point to the stable service. + err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.stableRS, true) + if err != nil { + return err + } + } else if c.newRS == nil || c.newRS.Status.AvailableReplicas == 0 { // when newRS is not available or replicas num is 0. never weight to canary weightDestinations = append(weightDestinations, c.calculateWeightDestinationsFromExperiment()...) diff --git a/rollout/trafficrouting_test.go b/rollout/trafficrouting_test.go index 8ac4a08e67..ee510316ec 100644 --- a/rollout/trafficrouting_test.go +++ b/rollout/trafficrouting_test.go @@ -893,6 +893,7 @@ func TestDynamicScalingDontIncreaseWeightWhenAborted(t *testing.T) { f.objects = append(f.objects, r2) f.expectPatchRolloutAction(r2) + f.expectPatchServiceAction(canarySvc, rs1PodHash) f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything, mock.Anything).Return(nil) @@ -964,6 +965,7 @@ func TestDynamicScalingDecreaseWeightAccordingToStableAvailabilityWhenAborted(t f.objects = append(f.objects, r2) f.expectPatchRolloutAction(r2) + f.expectPatchServiceAction(canarySvc, rs1PodHash) f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything, mock.Anything).Return(nil) From 2ca5c0befcaccab8a92414c6db1646ec7a1e2d72 Mon Sep 17 00:00:00 2001 From: zachaller Date: Wed, 8 Feb 2023 21:56:59 -0600 Subject: [PATCH 2/5] work with both dynamic stable scale on and off Signed-off-by: zachaller --- rollout/trafficrouting.go | 22 +++++++--- rollout/trafficrouting_test.go | 74 +++++++++++++++++++++++++++++++++- 2 files changed, 89 insertions(+), 7 deletions(-) diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go index b4626f1788..e696eeb1cd 100644 --- a/rollout/trafficrouting.go +++ b/rollout/trafficrouting.go @@ -166,14 +166,24 @@ func (c *rolloutContext) reconcileTrafficRouting() error { desiredWeight = minInt(desiredWeight, c.rollout.Status.Canary.Weights.Canary.Weight) } } - err := reconciler.RemoveManagedRoutes() - if err != nil { - return err + + if c.rollout.Spec.Strategy.Canary.DynamicStableScale && desiredWeight == 0 { + // If we are using dynamic stable scale we need to also make sure that desiredWeight=0 aka we are completely + // done with aborting before resetting the canary service selectors back to stable + err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.stableRS, true) + if err != nil { + return err + } + } else if !c.rollout.Spec.Strategy.Canary.DynamicStableScale { + // We are not using dynamic stable scale here, so it is safe to reset the canary service selectors right + //away because we will also be immediately setting desiredWeight=0 + err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.stableRS, true) + if err != nil { + return err + } } - // If we are aborting, we want to set the desired weight to 0, but we also want to set the canary service - // to point to the stable service. - err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.stableRS, true) + err := reconciler.RemoveManagedRoutes() if err != nil { return err } diff --git a/rollout/trafficrouting_test.go b/rollout/trafficrouting_test.go index ee510316ec..da82f812a4 100644 --- a/rollout/trafficrouting_test.go +++ b/rollout/trafficrouting_test.go @@ -965,7 +965,6 @@ func TestDynamicScalingDecreaseWeightAccordingToStableAvailabilityWhenAborted(t f.objects = append(f.objects, r2) f.expectPatchRolloutAction(r2) - f.expectPatchServiceAction(canarySvc, rs1PodHash) f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything, mock.Anything).Return(nil) @@ -979,3 +978,76 @@ func TestDynamicScalingDecreaseWeightAccordingToStableAvailabilityWhenAborted(t f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil) f.run(getKey(r1, t)) } + +// TestDynamicScalingDecreaseWeightAccordingToStableAvailabilityWhenAbortedAndResetService verifies we decrease the weight +// to the canary depending on the availability of the stable ReplicaSet when aborting and also that at the end of the abort +// we reset the canary service selectors back to the stable service +func TestDynamicScalingDecreaseWeightAccordingToStableAvailabilityWhenAbortedAndResetService(t *testing.T) { + f := newFixture(t) + defer f.Close() + + steps := []v1alpha1.CanaryStep{ + { + SetWeight: pointer.Int32Ptr(50), + }, + { + Pause: &v1alpha1.RolloutPause{}, + }, + } + r1 := newCanaryRollout("foo", 5, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(1)) + r1.Spec.Strategy.Canary.DynamicStableScale = true + r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ + SMI: &v1alpha1.SMITrafficRouting{}, + } + r1.Spec.Strategy.Canary.CanaryService = "canary" + r1.Spec.Strategy.Canary.StableService = "stable" + r1.Status.ReadyReplicas = 5 + r1.Status.AvailableReplicas = 5 + r1.Status.Abort = true + r1.Status.AbortedAt = &metav1.Time{Time: time.Now().Add(-1 * time.Minute)} + r2 := bumpVersion(r1) + + rs1 := newReplicaSetWithStatus(r1, 5, 5) + rs2 := newReplicaSetWithStatus(r2, 0, 0) + + rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + canarySelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash} + stableSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs1PodHash} + canarySvc := newService("canary", 80, canarySelector, r1) + stableSvc := newService("stable", 80, stableSelector, r1) + r2.Status.StableRS = rs1PodHash + r2.Status.Canary.Weights = &v1alpha1.TrafficWeights{ + Canary: v1alpha1.WeightDestination{ + Weight: 20, + ServiceName: "canary", + PodTemplateHash: rs2PodHash, + }, + Stable: v1alpha1.WeightDestination{ + Weight: 80, + ServiceName: "stable", + PodTemplateHash: rs1PodHash, + }, + } + + f.kubeobjects = append(f.kubeobjects, rs1, rs2, canarySvc, stableSvc) + f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) + + f.rolloutLister = append(f.rolloutLister, r2) + f.objects = append(f.objects, r2) + + f.expectPatchRolloutAction(r2) + f.expectPatchServiceAction(canarySvc, rs1PodHash) + + f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() + f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything, mock.Anything).Return(nil) + f.fakeTrafficRouting.On("SetWeight", mock.Anything, mock.Anything).Return(func(desiredWeight int32, additionalDestinations ...v1alpha1.WeightDestination) error { + // make sure SetWeight was called with correct value + assert.Equal(t, int32(0), desiredWeight) + return nil + }) + f.fakeTrafficRouting.On("SetHeaderRoute", mock.Anything, mock.Anything).Return(nil) + f.fakeTrafficRouting.On("RemoveManagedRoutes", mock.Anything, mock.Anything).Return(nil) + f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil) + f.run(getKey(r1, t)) +} From afc08c6e25f0f21206e536aa74f4d0f8a1af41aa Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 9 Feb 2023 09:36:24 -0600 Subject: [PATCH 3/5] add e2e tests Signed-off-by: zachaller --- test/e2e/canary_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/e2e/canary_test.go b/test/e2e/canary_test.go index 8732db50da..6a8a73ea48 100644 --- a/test/e2e/canary_test.go +++ b/test/e2e/canary_test.go @@ -538,6 +538,10 @@ func (s *CanarySuite) TestCanaryScaleDownOnAbort() { WaitForRolloutStatus("Paused"). AbortRollout(). WaitForRolloutStatus("Degraded"). + Then(). + //Expect that the canary service selector has been moved back to stable + ExpectServiceSelector("canary-scaledowndelay-canary", map[string]string{"app": "canary-scaledowndelay", "rollouts-pod-template-hash": "66597877b7"}, false). + When(). Sleep(3*time.Second). Then(). ExpectRevisionPodCount("2", 0) @@ -586,9 +590,24 @@ func (s *CanarySuite) TestCanaryDynamicStableScale() { WaitForRevisionPodCount("2", 1). Then(). ExpectRevisionPodCount("1", 4). + //Assert that the canary service selector is still not set to stable rs because of dynamic stable scale still in progress + Assert(func(t *fixtures.Then) { + canarySvc, stableSvc := t.GetServices() + assert.NotEqual(s.T(), canarySvc.Spec.Selector["rollouts-pod-template-hash"], stableSvc.Spec.Selector["rollouts-pod-template-hash"]) + }). When(). MarkPodsReady("1", 1). // mark last remaining stable pod as ready (4/4 stable are ready) WaitForRevisionPodCount("2", 0). Then(). + //Expect that the canary service selector is now set to stable because of dynamic stable scale is over and we have all pods up on stable rs + ExpectServiceSelector("dynamic-stable-scale-canary", map[string]string{"app": "dynamic-stable-scale", "rollouts-pod-template-hash": "868d98995b"}, false). ExpectRevisionPodCount("1", 4) } + +//stable: 868d98995b +//canary: f8754d5b4 + +//rollouts-pod-template-hash -> 6889b96b9 + +//canary: f8754d5b4 +//stable: 868d98995b From e265e0ebb849d7699b5a7ff0cef62430877b93c8 Mon Sep 17 00:00:00 2001 From: zachaller Date: Thu, 9 Feb 2023 13:20:35 -0600 Subject: [PATCH 4/5] cleanup comments Signed-off-by: zachaller --- test/e2e/canary_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/test/e2e/canary_test.go b/test/e2e/canary_test.go index 6a8a73ea48..00b588c598 100644 --- a/test/e2e/canary_test.go +++ b/test/e2e/canary_test.go @@ -603,11 +603,3 @@ func (s *CanarySuite) TestCanaryDynamicStableScale() { ExpectServiceSelector("dynamic-stable-scale-canary", map[string]string{"app": "dynamic-stable-scale", "rollouts-pod-template-hash": "868d98995b"}, false). ExpectRevisionPodCount("1", 4) } - -//stable: 868d98995b -//canary: f8754d5b4 - -//rollouts-pod-template-hash -> 6889b96b9 - -//canary: f8754d5b4 -//stable: 868d98995b From 820f516bd1aa0b003e9ebc956528053d8f69e8a8 Mon Sep 17 00:00:00 2001 From: zachaller Date: Mon, 27 Feb 2023 14:02:07 -0600 Subject: [PATCH 5/5] cleanup if logic Signed-off-by: zachaller --- rollout/trafficrouting.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go index e696eeb1cd..6064b60dad 100644 --- a/rollout/trafficrouting.go +++ b/rollout/trafficrouting.go @@ -167,20 +167,13 @@ func (c *rolloutContext) reconcileTrafficRouting() error { } } - if c.rollout.Spec.Strategy.Canary.DynamicStableScale && desiredWeight == 0 { + if (c.rollout.Spec.Strategy.Canary.DynamicStableScale && desiredWeight == 0) || !c.rollout.Spec.Strategy.Canary.DynamicStableScale { // If we are using dynamic stable scale we need to also make sure that desiredWeight=0 aka we are completely // done with aborting before resetting the canary service selectors back to stable err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.stableRS, true) if err != nil { return err } - } else if !c.rollout.Spec.Strategy.Canary.DynamicStableScale { - // We are not using dynamic stable scale here, so it is safe to reset the canary service selectors right - //away because we will also be immediately setting desiredWeight=0 - err = c.ensureSVCTargets(c.rollout.Spec.Strategy.Canary.CanaryService, c.stableRS, true) - if err != nil { - return err - } } err := reconciler.RemoveManagedRoutes()