diff --git a/rollout/service_test.go b/rollout/service_test.go index 3e0affc53f..aa4f1d020b 100644 --- a/rollout/service_test.go +++ b/rollout/service_test.go @@ -459,7 +459,10 @@ func TestCanaryAWSVerifyTargetGroupsNotYetReady(t *testing.T) { } fakeELB.On("DescribeTargetHealth", mock.Anything, mock.Anything).Return(&thOut, nil) - r1 := newCanaryRollout("foo", 3, nil, nil, nil, intstr.FromString("25%"), intstr.FromString("25%")) + r1 := newCanaryRollout("foo", 3, nil, []v1alpha1.CanaryStep{{ + SetWeight: pointer.Int32Ptr(10), + }}, pointer.Int32Ptr(0), intstr.FromString("25%"), intstr.FromString("25%")) + r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ ALB: &v1alpha1.ALBTrafficRouting{ Ingress: "ingress", @@ -553,7 +556,9 @@ func TestCanaryAWSVerifyTargetGroupsReady(t *testing.T) { } fakeELB.On("DescribeTargetHealth", mock.Anything, mock.Anything).Return(&thOut, nil) - r1 := newCanaryRollout("foo", 3, nil, nil, nil, intstr.FromString("25%"), intstr.FromString("25%")) + r1 := newCanaryRollout("foo", 3, nil, []v1alpha1.CanaryStep{{ + SetWeight: pointer.Int32Ptr(10), + }}, pointer.Int32Ptr(0), intstr.FromString("25%"), intstr.FromString("25%")) r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ ALB: &v1alpha1.ALBTrafficRouting{ Ingress: "ingress", @@ -610,7 +615,9 @@ func TestCanaryAWSVerifyTargetGroupsSkip(t *testing.T) { f := newFixture(t) defer f.Close() - r1 := newCanaryRollout("foo", 3, nil, nil, nil, intstr.FromString("25%"), intstr.FromString("25%")) + r1 := newCanaryRollout("foo", 3, nil, []v1alpha1.CanaryStep{{ + SetWeight: pointer.Int32Ptr(10), + }}, pointer.Int32Ptr(0), intstr.FromString("25%"), intstr.FromString("25%")) r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ ALB: &v1alpha1.ALBTrafficRouting{ Ingress: "ingress", diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go index 8c3598e9ce..ebc22b9704 100644 --- a/rollout/trafficrouting.go +++ b/rollout/trafficrouting.go @@ -3,6 +3,7 @@ package rollout import ( "fmt" "reflect" + "strconv" "strings" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" @@ -232,27 +233,26 @@ func (c *rolloutContext) reconcileTrafficRouting() error { c.newStatus.Canary.Weights = newWeights } - // 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 limiting - shouldVerifyWeight := c.rollout.Status.StableRS != "" && - !rolloututil.IsFullyPromoted(c.rollout) && - currentStep != nil && currentStep.SetWeight != nil + weightVerified, err := reconciler.VerifyWeight(desiredWeight, weightDestinations...) + c.newStatus.Canary.Weights.Verified = weightVerified + if err != nil { + 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 + } - if shouldVerifyWeight { - weightVerified, err := reconciler.VerifyWeight(desiredWeight, weightDestinations...) - c.newStatus.Canary.Weights.Verified = weightVerified - if err != nil { - 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 - } - if weightVerified != nil { - if *weightVerified { - c.log.Infof("Desired weight (stepIdx: %d) %d verified", *index, desiredWeight) - } else { - c.log.Infof("Desired weight (stepIdx: %d) %d not yet verified", *index, desiredWeight) - c.enqueueRolloutAfter(c.rollout, defaults.GetRolloutVerifyRetryInterval()) - } + var indexString string + if index != nil { + indexString = strconv.FormatInt(int64(*index), 10) + } else { + indexString = "n/a" + } + + if weightVerified != nil { + if *weightVerified { + c.log.Infof("Desired weight (stepIdx: %s) %d verified", indexString, desiredWeight) + } else { + c.log.Infof("Desired weight (stepIdx: %s) %d not yet verified", indexString, desiredWeight) + c.enqueueRolloutAfter(c.rollout, defaults.GetRolloutVerifyRetryInterval()) } } } diff --git a/rollout/trafficrouting/alb/alb.go b/rollout/trafficrouting/alb/alb.go index 55fd0b83be..62d1a465d4 100644 --- a/rollout/trafficrouting/alb/alb.go +++ b/rollout/trafficrouting/alb/alb.go @@ -5,6 +5,8 @@ import ( "fmt" "strconv" + rolloututil "github.com/argoproj/argo-rollouts/utils/rollout" + "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -117,7 +119,8 @@ func (r *Reconciler) SetHeaderRoute(headerRouting *v1alpha1.SetHeaderRoute) erro return nil } -func (r *Reconciler) shouldVerifyWeight() bool { +// Gets the controller configuration flag for verifying alb weights +func (r *Reconciler) getShouldVerifyWeightCfg() bool { if r.cfg.VerifyWeight != nil { return *r.cfg.VerifyWeight } @@ -125,13 +128,25 @@ func (r *Reconciler) shouldVerifyWeight() bool { } func (r *Reconciler) VerifyWeight(desiredWeight int32, additionalDestinations ...v1alpha1.WeightDestination) (*bool, error) { - if !r.shouldVerifyWeight() { + if !r.getShouldVerifyWeightCfg() { r.cfg.Status.ALB = nil return nil, nil } + + if !rolloututil.ShouldVerifyWeight(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 + // we return early to avoid calling AWS apis. + if r.cfg.Rollout.Status.ALB != nil { + return nil, nil + } + } + if r.cfg.Status.ALB == nil { r.cfg.Status.ALB = &v1alpha1.ALBStatus{} } + ctx := context.TODO() rollout := r.cfg.Rollout ingressName := rollout.Spec.Strategy.Canary.TrafficRouting.ALB.Ingress diff --git a/rollout/trafficrouting/alb/alb_test.go b/rollout/trafficrouting/alb/alb_test.go index e455926836..7e26f6f16e 100644 --- a/rollout/trafficrouting/alb/alb_test.go +++ b/rollout/trafficrouting/alb/alb_test.go @@ -463,6 +463,10 @@ func (f *fakeAWSClient) getAlbStatus() *v1alpha1.ALBStatus { func TestVerifyWeight(t *testing.T) { newFakeReconciler := func(status *v1alpha1.RolloutStatus) (*Reconciler, *fakeAWSClient) { ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443) + ro.Status.StableRS = "a45fe23" + ro.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{ + SetWeight: pointer.Int32Ptr(10), + }} i := ingress("ingress", STABLE_SVC, CANARY_SVC, STABLE_SVC, 443, 5, ro.Name, false) i.Status.LoadBalancer = corev1.LoadBalancerStatus{ Ingress: []corev1.LoadBalancerIngress{ @@ -503,6 +507,29 @@ func TestVerifyWeight(t *testing.T) { assert.False(t, *weightVerified) } + // VeryifyWeight not needed + { + var status v1alpha1.RolloutStatus + r, _ := newFakeReconciler(&status) + status.StableRS = "" + r.cfg.Rollout.Status.StableRS = "" + weightVerified, err := r.VerifyWeight(10) + assert.NoError(t, err) + assert.False(t, *weightVerified) + } + + // VeryifyWeight that we do not need to verify weight and status.ALB is already set + { + var status v1alpha1.RolloutStatus + r, _ := newFakeReconciler(&status) + r.cfg.Rollout.Status.ALB = &v1alpha1.ALBStatus{} + r.cfg.Rollout.Status.CurrentStepIndex = nil + r.cfg.Rollout.Spec.Strategy.Canary.Steps = nil + weightVerified, err := r.VerifyWeight(10) + assert.NoError(t, err) + assert.Nil(t, weightVerified) + } + // LoadBalancer found, not at weight { var status v1alpha1.RolloutStatus @@ -642,6 +669,10 @@ func TestVerifyWeightWithAdditionalDestinations(t *testing.T) { } newFakeReconciler := func(status *v1alpha1.RolloutStatus) (*Reconciler, *fakeAWSClient) { ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443) + ro.Status.StableRS = "a45fe23" + ro.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{ + SetWeight: pointer.Int32Ptr(10), + }} i := ingress("ingress", STABLE_SVC, CANARY_SVC, STABLE_SVC, 443, 0, ro.Name, false) i.Annotations["alb.ingress.kubernetes.io/actions.stable-svc"] = fmt.Sprintf(actionTemplateWithExperiments, CANARY_SVC, 443, 10, weightDestinations[0].ServiceName, 443, weightDestinations[0].Weight, weightDestinations[1].ServiceName, 443, weightDestinations[1].Weight, STABLE_SVC, 443, 85) diff --git a/rollout/trafficrouting_test.go b/rollout/trafficrouting_test.go index ac7d9b6516..abb0cc09f6 100644 --- a/rollout/trafficrouting_test.go +++ b/rollout/trafficrouting_test.go @@ -177,7 +177,7 @@ func TestRolloutUseDesiredWeight(t *testing.T) { return nil }) f.fakeTrafficRouting.On("SetHeaderRoute", mock.Anything, mock.Anything).Return(nil) - f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(true, nil) + f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil) f.run(getKey(r2, t)) } @@ -226,7 +226,7 @@ func TestRolloutUseDesiredWeight100(t *testing.T) { return nil }) f.fakeTrafficRouting.On("SetHeaderRoute", mock.Anything, mock.Anything).Return(nil) - f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(true, nil) + f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil) f.run(getKey(r2, t)) } @@ -723,7 +723,9 @@ func TestCanaryWithTrafficRoutingAddScaleDownDelay(t *testing.T) { f := newFixture(t) defer f.Close() - r1 := newCanaryRollout("foo", 1, nil, nil, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(1)) + r1 := newCanaryRollout("foo", 1, nil, []v1alpha1.CanaryStep{{ + SetWeight: pointer.Int32Ptr(10), + }}, pointer.Int32Ptr(0), intstr.FromInt(1), intstr.FromInt(1)) r1.Spec.Strategy.Canary.CanaryService = "canary" r1.Spec.Strategy.Canary.StableService = "stable" r1.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ @@ -735,7 +737,6 @@ func TestCanaryWithTrafficRoutingAddScaleDownDelay(t *testing.T) { rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2 = updateCanaryRolloutStatus(r2, rs2PodHash, 2, 1, 2, false) r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) - r2.Status.CurrentStepIndex = nil availableCondition, _ := newAvailableCondition(true) conditions.SetRolloutCondition(&r2.Status, availableCondition) _, r2.Status.Canary.Weights = calculateWeightStatus(r2, rs2PodHash, rs2PodHash, 0) @@ -763,7 +764,9 @@ func TestCanaryWithTrafficRoutingScaleDownLimit(t *testing.T) { inTheFuture := timeutil.MetaNow().Add(10 * time.Second).UTC().Format(time.RFC3339) - r1 := newCanaryRollout("foo", 1, nil, nil, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(1)) + r1 := newCanaryRollout("foo", 1, nil, []v1alpha1.CanaryStep{{ + SetWeight: pointer.Int32Ptr(10), + }}, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(1)) rs1 := newReplicaSetWithStatus(r1, 1, 1) rs1.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey] = inTheFuture r1.Spec.Strategy.Canary.ScaleDownDelayRevisionLimit = pointer.Int32Ptr(1) diff --git a/utils/rollout/rolloututil.go b/utils/rollout/rolloututil.go index c9243c3215..0b7df7ff38 100644 --- a/utils/rollout/rolloututil.go +++ b/utils/rollout/rolloututil.go @@ -4,6 +4,8 @@ import ( "fmt" "strconv" + replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset" + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/argoproj/argo-rollouts/utils/annotations" "github.com/argoproj/argo-rollouts/utils/conditions" @@ -179,3 +181,16 @@ func CanaryStepString(c v1alpha1.CanaryStep) string { } return "invalid" } + +// 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 { + 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 + return shouldVerifyWeight +} diff --git a/utils/rollout/rolloututil_test.go b/utils/rollout/rolloututil_test.go index efb76af9a3..572bd1cb51 100644 --- a/utils/rollout/rolloututil_test.go +++ b/utils/rollout/rolloututil_test.go @@ -414,3 +414,21 @@ func TestIsUnpausing(t *testing.T) { assert.Equal(t, v1alpha1.RolloutPhaseProgressing, status) assert.Equal(t, "waiting for rollout to unpause", message) } + +func TestShouldVerifyWeight(t *testing.T) { + ro := newCanaryRollout() + ro.Status.StableRS = "34feab23f" + ro.Status.CurrentStepIndex = pointer.Int32Ptr(0) + ro.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{ + SetWeight: pointer.Int32Ptr(20), + }} + assert.Equal(t, true, ShouldVerifyWeight(ro)) + + ro.Status.StableRS = "" + assert.Equal(t, false, ShouldVerifyWeight(ro)) + + ro.Status.StableRS = "34feab23f" + ro.Status.CurrentStepIndex = nil + ro.Spec.Strategy.Canary.Steps = nil + assert.Equal(t, false, ShouldVerifyWeight(ro)) +}