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 <kareena.hirani@gmail.com>

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
+}

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

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)
+		})
+}
+