From 0716c5d4417ec1cc507b24b3a400d07e4bf24303 Mon Sep 17 00:00:00 2001 From: Kareena Hirani Date: Tue, 16 Nov 2021 10:33:48 -0800 Subject: [PATCH] fix: Route traffic to Experiment even if Canary RS not scaled (#1638) * fix: Route traffic to Experiment even if canary RS not scaled Signed-off-by: khhirani --- pkg/apis/rollouts/v1alpha1/generated.proto | 4 + rollout/trafficrouting.go | 50 +++++----- .../rollout-alb-experiment-no-setweight.yaml | 99 +++++++++++++++++++ test/e2e/aws_test.go | 50 ++++++++++ 4 files changed, 181 insertions(+), 22 deletions(-) create mode 100644 test/e2e/alb/rollout-alb-experiment-no-setweight.yaml diff --git a/pkg/apis/rollouts/v1alpha1/generated.proto b/pkg/apis/rollouts/v1alpha1/generated.proto index a4dc739791..f0bb8bf009 100644 --- a/pkg/apis/rollouts/v1alpha1/generated.proto +++ b/pkg/apis/rollouts/v1alpha1/generated.proto @@ -60,6 +60,7 @@ message AmbassadorTrafficRouting { // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // +kubebuilder:resource:path=analysisruns, shortName=ar // +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase",description="AnalysisRun status" +// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time since resource was created" message AnalysisRun { optional k8s.io.apimachinery.pkg.apis.meta.v1.ObjectMeta metadata = 1; @@ -134,6 +135,7 @@ message AnalysisRunStrategy { // +genclient // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // +kubebuilder:resource:path=analysistemplates,shortName=at +// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time since resource was created" message AnalysisTemplate { optional k8s.io.apimachinery.pkg.apis.meta.v1.ObjectMeta metadata = 1; @@ -453,6 +455,7 @@ message CloudWatchMetricStatMetricDimension { // +genclient:nonNamespaced // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // +kubebuilder:resource:path=clusteranalysistemplates,shortName=cat +// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time since resource was created" message ClusterAnalysisTemplate { optional k8s.io.apimachinery.pkg.apis.meta.v1.ObjectMeta metadata = 1; @@ -478,6 +481,7 @@ message DatadogMetric { // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // +kubebuilder:resource:path=experiments,shortName=exp // +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase",description="Experiment status" +// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time since resource was created" message Experiment { optional k8s.io.apimachinery.pkg.apis.meta.v1.ObjectMeta metadata = 1; diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go index 83a0f8f6ed..77907d09ba 100644 --- a/rollout/trafficrouting.go +++ b/rollout/trafficrouting.go @@ -101,6 +101,7 @@ func (c *rolloutContext) reconcileTrafficRouting() error { } } 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()...) } else if index != nil { atDesiredReplicaCount := replicasetutil.AtDesiredReplicaCountsForCanary(c.rollout, c.newRS, c.stableRS, c.otherRSs, nil) if !atDesiredReplicaCount && !c.rollout.Status.PromoteFull { @@ -121,28 +122,7 @@ func (c *rolloutContext) reconcileTrafficRouting() error { // last setWeight step, which is set by GetCurrentSetWeight. desiredWeight = replicasetutil.GetCurrentSetWeight(c.rollout) } - - // Checks for experiment step - // If current experiment exists, then create WeightDestinations for each experiment template - exStep := replicasetutil.GetCurrentExperimentStep(c.rollout) - if exStep != nil && c.currentEx != nil && c.currentEx.Status.Phase == v1alpha1.AnalysisPhaseRunning { - getTemplateWeight := func(name string) *int32 { - for _, tmpl := range exStep.Templates { - if tmpl.Name == name { - return tmpl.Weight - } - } - return nil - } - for _, templateStatus := range c.currentEx.Status.TemplateStatuses { - templateWeight := getTemplateWeight(templateStatus.Name) - weightDestinations = append(weightDestinations, v1alpha1.WeightDestination{ - ServiceName: templateStatus.ServiceName, - PodTemplateHash: templateStatus.PodTemplateHash, - Weight: *templateWeight, - }) - } - } + weightDestinations = append(weightDestinations, c.calculateWeightDestinationsFromExperiment()...) } err = reconciler.SetWeight(desiredWeight, weightDestinations...) @@ -209,3 +189,29 @@ func calculateWeightStatus(ro *v1alpha1.Rollout, canaryHash, stableHash string, !reflect.DeepEqual(prevWeights.Additional, weights.Additional) return modified, &weights } + +// calculateWeightDestinationsFromExperiment checks for experiment step +// If current experiment exists, then create WeightDestinations for each experiment template +func (c *rolloutContext) calculateWeightDestinationsFromExperiment() []v1alpha1.WeightDestination { + weightDestinations := make([]v1alpha1.WeightDestination, 0) + exStep := replicasetutil.GetCurrentExperimentStep(c.rollout) + if exStep != nil && c.currentEx != nil && c.currentEx.Status.Phase == v1alpha1.AnalysisPhaseRunning { + getTemplateWeight := func(name string) *int32 { + for _, tmpl := range exStep.Templates { + if tmpl.Name == name { + return tmpl.Weight + } + } + return nil + } + for _, templateStatus := range c.currentEx.Status.TemplateStatuses { + templateWeight := getTemplateWeight(templateStatus.Name) + weightDestinations = append(weightDestinations, v1alpha1.WeightDestination{ + ServiceName: templateStatus.ServiceName, + PodTemplateHash: templateStatus.PodTemplateHash, + Weight: *templateWeight, + }) + } + } + return weightDestinations +} diff --git a/test/e2e/alb/rollout-alb-experiment-no-setweight.yaml b/test/e2e/alb/rollout-alb-experiment-no-setweight.yaml new file mode 100644 index 0000000000..cb80c86468 --- /dev/null +++ b/test/e2e/alb/rollout-alb-experiment-no-setweight.yaml @@ -0,0 +1,99 @@ +apiVersion: v1 +kind: Service +metadata: + name: alb-rollout-root +spec: + type: NodePort + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: alb-rollout +--- +apiVersion: v1 +kind: Service +metadata: + name: alb-rollout-canary +spec: + type: NodePort + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: alb-rollout +--- +apiVersion: v1 +kind: Service +metadata: + name: alb-rollout-stable +spec: + type: NodePort + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: alb-rollout +--- +apiVersion: networking.k8s.io/v1beta1 +kind: Ingress +metadata: + name: alb-rollout-ingress + annotations: + kubernetes.io/ingress.class: alb +spec: + rules: + - http: + paths: + - path: /* + backend: + serviceName: alb-rollout-root + servicePort: use-annotation +--- +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: alb-rollout +spec: + selector: + matchLabels: + app: alb-rollout + template: + metadata: + labels: + app: alb-rollout + spec: + containers: + - name: alb-rollout + image: nginx:1.19-alpine + ports: + - name: http + containerPort: 80 + protocol: TCP + resources: + requests: + memory: 16Mi + cpu: 5m + strategy: + canary: + canaryService: alb-rollout-canary + stableService: alb-rollout-stable + trafficRouting: + alb: + ingress: alb-rollout-ingress + rootService: alb-rollout-root + servicePort: 80 + steps: + - experiment: + templates: + - name: experiment-alb-canary + specRef: canary + weight: 20 + - name: experiment-alb-stable + specRef: stable + weight: 20 diff --git a/test/e2e/aws_test.go b/test/e2e/aws_test.go index 1501e0b289..f367da7e7c 100644 --- a/test/e2e/aws_test.go +++ b/test/e2e/aws_test.go @@ -25,6 +25,7 @@ func TestAWSSuite(t *testing.T) { const actionTemplate = `{"Type":"forward","ForwardConfig":{"TargetGroups":[{"ServiceName":"%s","ServicePort":"%d","Weight":%d},{"ServiceName":"%s","ServicePort":"%d","Weight":%d}]}}` const actionTemplateWithExperiment = `{"Type":"forward","ForwardConfig":{"TargetGroups":[{"ServiceName":"%s","ServicePort":"%d","Weight":%d},{"ServiceName":"%s","ServicePort":"%d","Weight":%d},{"ServiceName":"%s","ServicePort":"%d","Weight":%d}]}}` +const actionTemplateWithExperiments = `{"Type":"forward","ForwardConfig":{"TargetGroups":[{"ServiceName":"%s","ServicePort":"%d","Weight":%d},{"ServiceName":"%s","ServicePort":"%d","Weight":%d},{"ServiceName":"%s","ServicePort":"%d","Weight":%d},{"ServiceName":"%s","ServicePort":"%d","Weight":%d}]}}` // TestALBUpdate is a simple integration test which verifies the controller can work in a real AWS @@ -105,3 +106,52 @@ func (s *AWSSuite) TestALBExperimentStep() { }) } +func (s *AWSSuite) TestALBExperimentStepNoSetWeight() { + s.Given(). + RolloutObjects("@alb/rollout-alb-experiment-no-setweight.yaml"). + When(). + ApplyManifests(). + WaitForRolloutStatus("Healthy"). + Then(). + Assert(func(t *fixtures.Then) { + ingress := t.GetALBIngress() + action, ok := ingress.Annotations["alb.ingress.kubernetes.io/actions.alb-rollout-root"] + assert.True(s.T(), ok) + + port := 80 + expectedAction := fmt.Sprintf(actionTemplate, "alb-rollout-canary", port, 0, "alb-rollout-stable", port, 100) + assert.Equal(s.T(), expectedAction, action) + }). + ExpectExperimentCount(0). + When(). + UpdateSpec(). + Sleep(10*time.Second). + Then(). + Assert(func(t *fixtures.Then) { + ingress := t.GetALBIngress() + action, ok := ingress.Annotations["alb.ingress.kubernetes.io/actions.alb-rollout-root"] + assert.True(s.T(), ok) + + experiment := t.GetRolloutExperiments().Items[0] + exService1, exService2 := experiment.Status.TemplateStatuses[0].ServiceName, experiment.Status.TemplateStatuses[1].ServiceName + + port := 80 + expectedAction := fmt.Sprintf(actionTemplateWithExperiments, "alb-rollout-canary", port, 0, exService1, port, 20, exService2, port, 20, "alb-rollout-stable", port, 60) + assert.Equal(s.T(), expectedAction, action) + }). + When(). + PromoteRollout(). + WaitForRolloutStatus("Healthy"). + Sleep(1*time.Second). // stable is currently set first, and then changes made to VirtualServices/DestinationRules + Then(). + Assert(func(t *fixtures.Then) { + ingress := t.GetALBIngress() + action, ok := ingress.Annotations["alb.ingress.kubernetes.io/actions.alb-rollout-root"] + assert.True(s.T(), ok) + + port := 80 + expectedAction := fmt.Sprintf(actionTemplate, "alb-rollout-canary", port, 0, "alb-rollout-stable", port, 100) + assert.Equal(s.T(), expectedAction, action) + }) +} +