From e0f5db4ff28e6af332011ddbce2fd16d34771497 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 11 Jun 2024 11:33:48 -0500 Subject: [PATCH 1/8] fix: verify the weight of the alb at the end of the rollout when we auto set max weight Signed-off-by: Zach Aller --- rollout/trafficrouting/alb/alb.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rollout/trafficrouting/alb/alb.go b/rollout/trafficrouting/alb/alb.go index 8e81b113c7..180923e622 100644 --- a/rollout/trafficrouting/alb/alb.go +++ b/rollout/trafficrouting/alb/alb.go @@ -3,6 +3,7 @@ package alb import ( "context" "fmt" + "github.com/argoproj/argo-rollouts/utils/weightutil" "strconv" "strings" @@ -202,7 +203,7 @@ func (r *Reconciler) VerifyWeight(desiredWeight int32, additionalDestinations .. return nil, nil } - if !rolloututil.ShouldVerifyWeight(r.cfg.Rollout) { + if !rolloututil.ShouldVerifyWeight(r.cfg.Rollout) && desiredWeight != weightutil.MaxTrafficWeight(r.cfg.Rollout) { // If we should not verify weight but the ALB status has not been set yet due to a Rollout resource just being // installed in the cluster we want to actually run the rest of the function, so we do not return if // r.cfg.Rollout.Status.ALB is nil. However, if we should not verify, and we have already updated the status once From b03bee463fc1af72425e6612066a7cf443aaa7a1 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 11 Jun 2024 13:46:09 -0500 Subject: [PATCH 2/8] add unit test Signed-off-by: Zach Aller --- rollout/trafficrouting/alb/alb.go | 3 +- rollout/trafficrouting/alb/alb_test.go | 43 ++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/rollout/trafficrouting/alb/alb.go b/rollout/trafficrouting/alb/alb.go index 180923e622..d8e6b22bcf 100644 --- a/rollout/trafficrouting/alb/alb.go +++ b/rollout/trafficrouting/alb/alb.go @@ -3,10 +3,11 @@ package alb import ( "context" "fmt" - "github.com/argoproj/argo-rollouts/utils/weightutil" "strconv" "strings" + "github.com/argoproj/argo-rollouts/utils/weightutil" + rolloututil "github.com/argoproj/argo-rollouts/utils/rollout" "github.com/sirupsen/logrus" diff --git a/rollout/trafficrouting/alb/alb_test.go b/rollout/trafficrouting/alb/alb_test.go index fb131849d4..faa798e524 100644 --- a/rollout/trafficrouting/alb/alb_test.go +++ b/rollout/trafficrouting/alb/alb_test.go @@ -918,6 +918,7 @@ func TestVerifyWeight(t *testing.T) { { var status v1alpha1.RolloutStatus r, fakeClient := newFakeReconciler(&status) + fakeClient.loadBalancers = []*elbv2types.LoadBalancer{ { LoadBalancerName: pointer.StringPtr("lb-abc123-name"), @@ -955,6 +956,48 @@ func TestVerifyWeight(t *testing.T) { assert.Equal(t, *status.ALB, *fakeClient.getAlbStatus("ingress")) } + // LoadBalancer found, at max weight, end of rollout + { + var status v1alpha1.RolloutStatus + status.CurrentStepIndex = pointer.Int32Ptr(2) + r, fakeClient := newFakeReconciler(&status) + fakeClient.loadBalancers = []*elbv2types.LoadBalancer{ + { + LoadBalancerName: pointer.StringPtr("lb-abc123-name"), + LoadBalancerArn: pointer.StringPtr("arn:aws:elasticloadbalancing:us-east-2:123456789012:loadbalancer/app/lb-abc123-name/1234567890123456"), + DNSName: pointer.StringPtr("verify-weight-test-abc-123.us-west-2.elb.amazonaws.com"), + }, + } + fakeClient.targetGroups = []aws.TargetGroupMeta{ + { + TargetGroup: elbv2types.TargetGroup{ + TargetGroupName: pointer.StringPtr("canary-tg-abc123-name"), + TargetGroupArn: pointer.StringPtr("arn:aws:elasticloadbalancing:us-east-2:123456789012:targetgroup/canary-tg-abc123-name/1234567890123456"), + }, + Weight: pointer.Int32Ptr(100), + Tags: map[string]string{ + aws.AWSLoadBalancerV2TagKeyResourceID: "default/ingress-canary-svc:443", + }, + }, + { + TargetGroup: elbv2types.TargetGroup{ + TargetGroupName: pointer.StringPtr("stable-tg-abc123-name"), + TargetGroupArn: pointer.StringPtr("arn:aws:elasticloadbalancing:us-east-2:123456789012:targetgroup/stable-tg-abc123-name/1234567890123456"), + }, + Weight: pointer.Int32Ptr(0), + Tags: map[string]string{ + aws.AWSLoadBalancerV2TagKeyResourceID: "default/ingress-stable-svc:443", + }, + }, + } + + weightVerified, err := r.VerifyWeight(100) + assert.NoError(t, err) + assert.True(t, *weightVerified) + assert.Equal(t, status.ALBs[0], *status.ALB) + assert.Equal(t, *status.ALB, *fakeClient.getAlbStatus("ingress")) + } + // LoadBalancer found, but ARNs are unparsable { var status v1alpha1.RolloutStatus From c108959cdaf7e56699448ed826b1d7f4eeea3c12 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 11 Jun 2024 16:02:16 -0500 Subject: [PATCH 3/8] refactor add unit test Signed-off-by: Zach Aller --- rollout/trafficrouting/alb/alb.go | 4 +--- utils/rollout/rolloututil.go | 10 ++++++---- utils/rollout/rolloututil_test.go | 12 +++++++++--- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/rollout/trafficrouting/alb/alb.go b/rollout/trafficrouting/alb/alb.go index d8e6b22bcf..2abeb052db 100644 --- a/rollout/trafficrouting/alb/alb.go +++ b/rollout/trafficrouting/alb/alb.go @@ -6,8 +6,6 @@ import ( "strconv" "strings" - "github.com/argoproj/argo-rollouts/utils/weightutil" - rolloututil "github.com/argoproj/argo-rollouts/utils/rollout" "github.com/sirupsen/logrus" @@ -204,7 +202,7 @@ func (r *Reconciler) VerifyWeight(desiredWeight int32, additionalDestinations .. return nil, nil } - if !rolloututil.ShouldVerifyWeight(r.cfg.Rollout) && desiredWeight != weightutil.MaxTrafficWeight(r.cfg.Rollout) { + if !rolloututil.ShouldVerifyWeight(r.cfg.Rollout, desiredWeight) { // If we should not verify weight but the ALB status has not been set yet due to a Rollout resource just being // installed in the cluster we want to actually run the rest of the function, so we do not return if // r.cfg.Rollout.Status.ALB is nil. However, if we should not verify, and we have already updated the status once diff --git a/utils/rollout/rolloututil.go b/utils/rollout/rolloututil.go index 0b7df7ff38..5411f3f58c 100644 --- a/utils/rollout/rolloututil.go +++ b/utils/rollout/rolloututil.go @@ -4,6 +4,8 @@ import ( "fmt" "strconv" + "github.com/argoproj/argo-rollouts/utils/weightutil" + replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" @@ -184,13 +186,13 @@ func CanaryStepString(c v1alpha1.CanaryStep) string { // ShouldVerifyWeight We use this to test if we should verify weights because weight verification could involve // API calls to the cloud provider which could incur rate limiting -func ShouldVerifyWeight(ro *v1alpha1.Rollout) bool { +func ShouldVerifyWeight(ro *v1alpha1.Rollout, desiredWeight int32) bool { currentStep, _ := replicasetutil.GetCurrentCanaryStep(ro) // If we are in the middle of an update at a setWeight step, also perform weight verification. // Note that we don't do this every reconciliation because weight verification typically involves // API calls to the cloud provider which could incur rate limitingq - shouldVerifyWeight := ro.Status.StableRS != "" && - !IsFullyPromoted(ro) && - currentStep != nil && currentStep.SetWeight != nil + shouldVerifyWeight := (ro.Status.StableRS != "" && !IsFullyPromoted(ro) && currentStep != nil && currentStep.SetWeight != nil) || + (ro.Status.StableRS != "" && !IsFullyPromoted(ro) && currentStep == nil && desiredWeight == weightutil.MaxTrafficWeight(ro)) // We are at end of rollout + return shouldVerifyWeight } diff --git a/utils/rollout/rolloututil_test.go b/utils/rollout/rolloututil_test.go index 37c1810f00..d88f080f64 100644 --- a/utils/rollout/rolloututil_test.go +++ b/utils/rollout/rolloututil_test.go @@ -422,15 +422,21 @@ func TestShouldVerifyWeight(t *testing.T) { ro.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{ SetWeight: pointer.Int32Ptr(20), }} - assert.Equal(t, true, ShouldVerifyWeight(ro)) + assert.Equal(t, true, ShouldVerifyWeight(ro, 20)) ro.Status.StableRS = "" - assert.Equal(t, false, ShouldVerifyWeight(ro)) + assert.Equal(t, false, ShouldVerifyWeight(ro, 20)) ro.Status.StableRS = "34feab23f" ro.Status.CurrentStepIndex = nil ro.Spec.Strategy.Canary.Steps = nil - assert.Equal(t, false, ShouldVerifyWeight(ro)) + assert.Equal(t, false, ShouldVerifyWeight(ro, 20)) + + // Test when the weight is 100, because we are at end of rollout + ro.Status.StableRS = "34feab23f" + ro.Status.CurrentStepIndex = nil + ro.Spec.Strategy.Canary.Steps = nil + assert.Equal(t, true, ShouldVerifyWeight(ro, 100)) } func Test_isGenerationObserved(t *testing.T) { From 2c08009db0329c3f3756b266a48e508569fde918 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 11 Jun 2024 16:20:58 -0500 Subject: [PATCH 4/8] Trigger Build Signed-off-by: Zach Aller From a6f3e92c00ea8e83b2bc5190947e89c7fcd23ca0 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 11 Jun 2024 18:09:38 -0500 Subject: [PATCH 5/8] Trigger Build Signed-off-by: Zach Aller From ea3104347820951932c0da50b9550783d27f91ca Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Wed, 12 Jun 2024 09:02:57 -0500 Subject: [PATCH 6/8] block reconcile Signed-off-by: Zach Aller --- rollout/trafficrouting.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go index f8b01951a4..63ef0daadf 100644 --- a/rollout/trafficrouting.go +++ b/rollout/trafficrouting.go @@ -274,6 +274,12 @@ func (c *rolloutContext) reconcileTrafficRouting() error { c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.WeightVerifyErrorReason}, conditions.WeightVerifyErrorMessage, err) return nil // return nil instead of error since we want to continue with normal reconciliation } + // At the end of the rollout we need to verify the weight is correct, and return an error if not because we don't want the rest of the + // reconcile process to continue. We don't need to do this if we are in the middle of the rollout because the rest of the reconcile + // process won't scale down the old replicasets yet due to being in the middle of some steps. + if weightVerified != nil && *weightVerified == false && desiredWeight == weightutil.MaxTrafficWeight(c.rollout) && len(c.rollout.Spec.Strategy.Canary.Steps) >= int(*c.rollout.Status.CurrentStepIndex) { + return fmt.Errorf("end of rollout, desired weight %d not yet verified", desiredWeight) + } var indexString string if index != nil { From 0b32bc0ba1fc168c8836d0d5c793074adf5923f0 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Wed, 12 Jun 2024 09:28:49 -0500 Subject: [PATCH 7/8] add tests Signed-off-by: Zach Aller --- rollout/trafficrouting.go | 12 ++++----- rollout/trafficrouting_test.go | 47 ++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go index 63ef0daadf..fd0c91decc 100644 --- a/rollout/trafficrouting.go +++ b/rollout/trafficrouting.go @@ -274,12 +274,6 @@ func (c *rolloutContext) reconcileTrafficRouting() error { c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: conditions.WeightVerifyErrorReason}, conditions.WeightVerifyErrorMessage, err) return nil // return nil instead of error since we want to continue with normal reconciliation } - // At the end of the rollout we need to verify the weight is correct, and return an error if not because we don't want the rest of the - // reconcile process to continue. We don't need to do this if we are in the middle of the rollout because the rest of the reconcile - // process won't scale down the old replicasets yet due to being in the middle of some steps. - if weightVerified != nil && *weightVerified == false && desiredWeight == weightutil.MaxTrafficWeight(c.rollout) && len(c.rollout.Spec.Strategy.Canary.Steps) >= int(*c.rollout.Status.CurrentStepIndex) { - return fmt.Errorf("end of rollout, desired weight %d not yet verified", desiredWeight) - } var indexString string if index != nil { @@ -294,6 +288,12 @@ func (c *rolloutContext) reconcileTrafficRouting() error { } else { c.log.Infof("Desired weight (stepIdx: %s) %d not yet verified", indexString, desiredWeight) c.enqueueRolloutAfter(c.rollout, defaults.GetRolloutVerifyRetryInterval()) + // At the end of the rollout we need to verify the weight is correct, and return an error if not because we don't want the rest of the + // reconcile process to continue. We don't need to do this if we are in the middle of the rollout because the rest of the reconcile + // process won't scale down the old replicasets yet due to being in the middle of some steps. + if *weightVerified == false && desiredWeight == weightutil.MaxTrafficWeight(c.rollout) && len(c.rollout.Spec.Strategy.Canary.Steps) >= int(*c.rollout.Status.CurrentStepIndex) { + return fmt.Errorf("end of rollout, desired weight %d not yet verified", desiredWeight) + } } } } diff --git a/rollout/trafficrouting_test.go b/rollout/trafficrouting_test.go index f7dd459964..3c5eda4205 100644 --- a/rollout/trafficrouting_test.go +++ b/rollout/trafficrouting_test.go @@ -130,6 +130,53 @@ func TestReconcileTrafficRoutingVerifyWeightFalse(t *testing.T) { assert.True(t, enqueued) } +func TestReconcileTrafficRoutingVerifyWeightDesiredWeight100(t *testing.T) { + f := newFixture(t) + defer f.Close() + + steps := []v1alpha1.CanaryStep{ + { + SetWeight: pointer.Int32Ptr(10), + }, + { + Pause: &v1alpha1.RolloutPause{}, + }, + } + r1 := newCanaryRollout("foo", 10, nil, steps, pointer.Int32Ptr(2), intstr.FromInt(1), intstr.FromInt(0)) + r2 := bumpVersion(r1) + r2.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{} + r2.Spec.Strategy.Canary.CanaryService = "canary" + r2.Spec.Strategy.Canary.StableService = "stable" + + rs1 := newReplicaSetWithStatus(r1, 10, 10) + rs2 := newReplicaSetWithStatus(r2, 10, 10) + + 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, r2) + stableSvc := newService("stable", 80, stableSelector, r2) + + f.kubeobjects = append(f.kubeobjects, rs1, rs2, canarySvc, stableSvc) + f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) + + r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 10, 0, 10, false) + f.rolloutLister = append(f.rolloutLister, r2) + f.objects = append(f.objects, r2) + + 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(100), desiredWeight) + return nil + }) + f.fakeTrafficRouting.On("SetHeaderRoute", mock.Anything, mock.Anything).Return(nil) + f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(false), nil) + f.runExpectError(getKey(r2, t), true) +} + func TestRolloutUseDesiredWeight(t *testing.T) { f := newFixture(t) defer f.Close() From 68c6f6fa35e028cc839ea31bc9709c18c8f13393 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Wed, 12 Jun 2024 11:24:57 -0500 Subject: [PATCH 8/8] rename test and cleanup un-need logic check Signed-off-by: Zach Aller --- rollout/trafficrouting.go | 2 +- rollout/trafficrouting_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go index fd0c91decc..e992277a57 100644 --- a/rollout/trafficrouting.go +++ b/rollout/trafficrouting.go @@ -291,7 +291,7 @@ func (c *rolloutContext) reconcileTrafficRouting() error { // At the end of the rollout we need to verify the weight is correct, and return an error if not because we don't want the rest of the // reconcile process to continue. We don't need to do this if we are in the middle of the rollout because the rest of the reconcile // process won't scale down the old replicasets yet due to being in the middle of some steps. - if *weightVerified == false && desiredWeight == weightutil.MaxTrafficWeight(c.rollout) && len(c.rollout.Spec.Strategy.Canary.Steps) >= int(*c.rollout.Status.CurrentStepIndex) { + if desiredWeight == weightutil.MaxTrafficWeight(c.rollout) && len(c.rollout.Spec.Strategy.Canary.Steps) >= int(*c.rollout.Status.CurrentStepIndex) { return fmt.Errorf("end of rollout, desired weight %d not yet verified", desiredWeight) } } diff --git a/rollout/trafficrouting_test.go b/rollout/trafficrouting_test.go index 3c5eda4205..f358a18f64 100644 --- a/rollout/trafficrouting_test.go +++ b/rollout/trafficrouting_test.go @@ -130,7 +130,7 @@ func TestReconcileTrafficRoutingVerifyWeightFalse(t *testing.T) { assert.True(t, enqueued) } -func TestReconcileTrafficRoutingVerifyWeightDesiredWeight100(t *testing.T) { +func TestReconcileTrafficRoutingVerifyWeightEndOfRollout(t *testing.T) { f := newFixture(t) defer f.Close()