diff --git a/docs/architecture.md b/docs/architecture.md index b92a21164c..e5640c025b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -34,7 +34,7 @@ Note also that the replica sets that take part in a Rollout are fully managed by This is the mechanism that traffic from live users enters your cluster and is redirected to the appropriate version. Argo Rollouts use the [standard Kubernetes service resource](https://kubernetes.io/docs/concepts/services-networking/service/), but with some extra metadata needed for management. Argo Rollouts is very flexible on networking options. First of all you can have different services during a Rollout, that go only to the new version, only to the old version or both. -Specifically for Canary deployments, Argo Rollouts supports several [service mesh and ingress solutions](../features/traffic-management/) for splitting traffic with specific percentages instead of simple balancing based on pod counts. +Specifically for Canary deployments, Argo Rollouts supports several [service mesh and ingress solutions](../features/traffic-management/) for splitting traffic with specific percentages instead of simple balancing based on pod counts and it is possible to use multiple routing providers simultaneously. ## AnalysisTemplate and AnalysisRun diff --git a/docs/features/traffic-management/index.md b/docs/features/traffic-management/index.md index 26b7c5a720..0c3aecca63 100644 --- a/docs/features/traffic-management/index.md +++ b/docs/features/traffic-management/index.md @@ -23,6 +23,7 @@ Argo Rollouts enables traffic management by manipulating the Service Mesh resour - [Istio](istio.md) - [Nginx Ingress Controller](nginx.md) - [Service Mesh Interface (SMI)](smi.md) +- [Multiple Providers](mixed.md) - File a ticket [here](https://github.com/argoproj/argo-rollouts/issues) if you would like another implementation (or thumbs up it if that issue already exists) Regardless of the Service Mesh used, the Rollout object has to set a canary Service and a stable Service in its spec. Here is an example with those fields set: diff --git a/docs/features/traffic-management/mixed.md b/docs/features/traffic-management/mixed.md new file mode 100644 index 0000000000..20e2880b89 --- /dev/null +++ b/docs/features/traffic-management/mixed.md @@ -0,0 +1,60 @@ +# Multiple Providers +!!! note + + Multiple trafficRouting is available since Argo Rollouts v1.2 + +The usage of multiple providers tries to cover scenarios where, for some reason, we have to use +different providers on North-South and West-East traffic routing or any other hybrid architecture that +requires the use of multiple providers. + +## Examples of when you can use multiple providers + +### Avoid injecting sidecars on your Ingress controller + +This is a common requirement of the service mesh and with multiple trafficRoutings you can leverage North-South traffic shifting to NGiNX +and West-East traffic shifting to SMI, avoiding the need of adding the Ingress controller inside the mesh. + +### Avoid manipulation of the host header at the Ingress + +Another common side effect of adding some of the Ingress controllers into the mesh, and is caused by the usage of those +mesh host headers to be pointing into a mesh hostname in order to be routed. + +### Avoid Big-Bang + +This takes place on existing fleets where downtime is very reduced or nearly impossible. +To avoid [big-bang-adoption](https://en.wikipedia.org/wiki/Big_bang_adoption) the use of multiple providers can ease +how teams can implement gradually new technologies. An example, where an existing fleet that is using a provider +such as Ambassador and is already performing canary in a North-South fashion as part of their rollouts can gradually +implement more providers such as Istio, SMI, etc. + +### Hybrid Scenarios + +In this case, its very similar to avoiding the Big-Bang, either if it is part of the platform roadmap or a new redesign +of the architecture, there are multiple scenarios where having the capacity of using multiple trafficRoutings is very +much in need: gradual implementation, eased rollback of architecture or even for a fallback. + +## Requirements + +The use of multiple providers requires that both providers comply with its minimum requirements independently. +By example, if you want to use NGiNX and SMI you would need to have both SMI and NGiNX in place and produce the rollout configuration +for both. + +```yaml +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: rollouts-demo +spec: + strategy: + canary: + # Reference to a Service which the controller will update to point to the canary ReplicaSet + canaryService: rollouts-demo-canary + # Reference to a Service which the controller will update to point to the stable ReplicaSet + stableService: rollouts-demo-stable + trafficRouting: + nginx: + # Reference to an Ingress which has a rule pointing to the stable service (e.g. rollouts-demo-stable) + # This ingress will be cloned with a new name, in order to achieve NGINX traffic splitting. + stableIngress: rollouts-demo-stable + smi: {} +``` diff --git a/docs/getting-started.md b/docs/getting-started.md index 8b819b43e5..43b1f8ea5c 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -178,5 +178,6 @@ provider to achieve more advanced traffic shaping. * [ALB Guide](getting-started/alb/index.md) * [Ambassador Guide](getting-started/ambassador/index.md) * [Istio Guide](getting-started/istio/index.md) +* [Multiple Providers Guide](getting-started/mixed/index.md) * [NGINX Guide](getting-started/nginx/index.md) * [SMI Guide](getting-started/smi/index.md) diff --git a/docs/getting-started/mixed/index.md b/docs/getting-started/mixed/index.md new file mode 100644 index 0000000000..4a6d12f02b --- /dev/null +++ b/docs/getting-started/mixed/index.md @@ -0,0 +1,233 @@ +# Getting Started - Multiple Providers (Service Mesh Interface and NGiNX Ingress) + +!!! important + Available since v1.2 + +This guide covers how Argo Rollouts integrates with multiple TrafficRoutings, using +[Linkerd](https://linkerd.io) and +[NGINX Ingress Controller](https://github.com/kubernetes/ingress-nginx) for traffic shaping, but you +should be able to produce any other combination between the existing trafficRouting options. + +This guide builds upon the concepts of the [basic getting started guide](../../getting-started.md), +[NGINX Guide](getting-started/nginx/index.md), and [SMI Guide](getting-started/smi/index.md). + +## Requirements +- Kubernetes cluster with Linkerd installed +- Kubernetes cluster with NGINX ingress controller installed and part of the mesh + +!!! tip + See the [environment setup guide for linkerd](../setup/index.md#linkerd-setup) + on how to setup a local minikube environment with linkerd and nginx. + +## 1. Deploy the Rollout, Services, and Ingress + +When SMI is used as one the traffic routers, the Rollout canary strategy must define +the following mandatory fields: + +```yaml +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: rollouts-demo +spec: + strategy: + canary: + # Reference to a Service which the controller will update to point to the canary ReplicaSet + canaryService: rollouts-demo-canary + # Reference to a Service which the controller will update to point to the stable ReplicaSet + stableService: rollouts-demo-stable + trafficRouting: + smi: {} +``` +When NGINX Ingress is used as the traffic router, the Rollout canary strategy must define +the following mandatory fields: + +```yaml +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: rollouts-demo +spec: + strategy: + canary: + # Reference to a Service which the controller will update to point to the canary ReplicaSet + canaryService: rollouts-demo-canary + # Reference to a Service which the controller will update to point to the stable ReplicaSet + stableService: rollouts-demo-stable + trafficRouting: + nginx: + # Reference to an Ingress which has a rule pointing to the stable service (e.g. rollouts-demo-stable) + # This ingress will be cloned with a new name, in order to achieve NGINX traffic splitting. + stableIngress: rollouts-demo-stable +... +``` + +A combination of both should have comply with each TrafficRouting requirements, in this case: + +```yaml +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: rollouts-demo +spec: + strategy: + canary: + # Reference to a Service which the controller will update to point to the canary ReplicaSet + canaryService: rollouts-demo-canary + # Reference to a Service which the controller will update to point to the stable ReplicaSet + stableService: rollouts-demo-stable + trafficRouting: + nginx: + # Reference to an Ingress which has a rule pointing to the stable service (e.g. rollouts-demo-stable) + # This ingress will be cloned with a new name, in order to achieve NGINX traffic splitting. + stableIngress: rollouts-demo-stable + smi: {} +``` + +The Ingress referenced in `canary.trafficRouting.nginx.stableIngress` is required to have a host +rule which has a backend targeting the Service referenced under `canary.stableService`. +In our example, that stable Service is named: `rollouts-demo-stable`: + +```yaml +apiVersion: networking.k8s.io/v1beta1 +kind: Ingress +metadata: + name: rollouts-demo-stable + annotations: + kubernetes.io/ingress.class: nginx +spec: + rules: + - host: rollouts-demo.local + http: + paths: + - path: / + backend: + # Reference to a Service name, also specified in the Rollout spec.strategy.canary.stableService field + serviceName: rollouts-demo-stable + servicePort: 80 +``` + +Run the following commands to deploy: + +* A Rollout with the Linkerd `linkerd.io/inject: enabled` annotation +* Two Services (stable and canary) +* An Ingress + +```shell +kubectl apply -f https://raw.githubusercontent.com/argoproj/argo-rollouts/master/docs/getting-started/mixed/rollout.yaml +kubectl apply -f https://raw.githubusercontent.com/argoproj/argo-rollouts/master/docs/getting-started/mixed/services.yaml +kubectl apply -f https://raw.githubusercontent.com/argoproj/argo-rollouts/master/docs/getting-started/mixed/ingress.yaml +``` + +After applying the manifests you should see the following rollout, services, and ingress resources +in the cluster: + +```shell +$ kubectl get ro +NAME DESIRED CURRENT UP-TO-DATE AVAILABLE +rollouts-demo 1 2 1 2 + +$ kubectl get svc +NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE +rollouts-demo-canary ClusterIP 10.111.69.188 80/TCP 23m +rollouts-demo-stable ClusterIP 10.109.175.248 80/TCP 23m + +$ kubectl get ing +NAME CLASS HOSTS ADDRESS PORTS AGE +rollouts-demo-stable rollouts-demo.local 192.168.64.2 80 23m +``` + +You should also see a TrafficSplit resource which is created automatically and owned by the rollout: + +``` +$ kubectl get trafficsplit +NAME SERVICE +rollouts-demo rollouts-demo-stable +``` + +When inspecting the generated TrafficSplit resource, the weights are automatically configured to +send 100% traffic to the `rollouts-demo-stable` service, and 0% traffic to the `rollouts-demo-canary`. +These values will be updated during an update. + +```yaml +apiVersion: split.smi-spec.io/v1alpha1 +kind: TrafficSplit +metadata: + name: rollouts-demo + namespace: default +spec: + backends: + - service: rollouts-demo-canary + weight: "0" + - service: rollouts-demo-stable + weight: "100" + service: rollouts-demo-stable +``` + +You should also notice a second ingress created by the rollouts controller, +`rollouts-demo-rollouts-demo-stable-canary`. This ingress is the "canary ingress", which is a +clone of the user-managed Ingress referenced under `nginx.stableIngress`. It is used by nginx +ingress controller to achieve canary traffic splitting. The name of the generated ingress is +formulated using `--canary`. More details on the second Ingress are +discussed in the following section. + +## 2. Perform an update + +Now perform an update the rollout by changing the image, and wait for it to reached the paused state. + +```shell +kubectl argo rollouts set image rollouts-demo rollouts-demo=argoproj/rollouts-demo:yellow +kubectl argo rollouts get rollout rollouts-demo +``` + +![Rollout Paused](../nginx/paused-rollout-nginx.png) + +At this point, both the canary and stable version of the Rollout are running, with 5% of the +traffic directed to the canary and 95% to the stable. When inspecting the TrafficSplit generated by +the controller, we see that the weight has been updated to reflect the current `setWeight: 5` step of +the canary deploy. + +```yaml +apiVersion: split.smi-spec.io/v1alpha1 +kind: TrafficSplit +metadata: + name: rollouts-demo + namespace: default +spec: + backends: + - service: rollouts-demo-canary + weight: "5" + - service: rollouts-demo-stable + weight: "95" + service: rollouts-demo-stable +``` +When inspecting the rollout controller generated Ingress copy, we see that it has the following +changes over the original ingress: + +1. Two additional +[NGINX specific canary annotations](https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#canary) +are added to the annotations. +2. The Ingress rules will have an rule which points the backend to the *canary* service. + + +```yaml +apiVersion: extensions/v1beta1 +kind: Ingress +metadata: + name: rollouts-demo-rollouts-demo-stable-canary + annotations: + kubernetes.io/ingress.class: nginx + nginx.ingress.kubernetes.io/canary: "true" + nginx.ingress.kubernetes.io/canary-weight: "5" +spec: + rules: + - host: rollouts-demo.local + http: + paths: + - backend: + serviceName: rollouts-demo-canary + servicePort: 80 +``` + +As the Rollout progresses through steps, the weights in the TrafficSplit and Ingress resource will be adjusted +to match the current setWeight of the steps. diff --git a/docs/getting-started/mixed/ingress.yaml b/docs/getting-started/mixed/ingress.yaml new file mode 100644 index 0000000000..1020c7bb64 --- /dev/null +++ b/docs/getting-started/mixed/ingress.yaml @@ -0,0 +1,16 @@ +apiVersion: networking.k8s.io/v1beta1 +kind: Ingress +metadata: + name: rollouts-demo-stable + annotations: + kubernetes.io/ingress.class: nginx +spec: + rules: + - host: rollouts-demo.local + http: + paths: + - path: / + backend: + # Reference to a Service name, also specified in the Rollout spec.strategy.canary.stableService field + serviceName: rollouts-demo-stable + servicePort: 80 diff --git a/docs/getting-started/mixed/rollout.yaml b/docs/getting-started/mixed/rollout.yaml new file mode 100644 index 0000000000..049431831b --- /dev/null +++ b/docs/getting-started/mixed/rollout.yaml @@ -0,0 +1,41 @@ +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: rollouts-demo +spec: + replicas: 1 + strategy: + canary: + canaryService: rollouts-demo-canary + stableService: rollouts-demo-stable + trafficRouting: + nginx: + # Reference to an Ingress which has a rule pointing to the stable service (e.g. rollouts-demo-stable) + # This ingress will be cloned with a new name, in order to achieve NGINX traffic splitting. + stableIngress: rollouts-demo-stable + smi: {} + steps: + - setWeight: 5 + - pause: {} + revisionHistoryLimit: 2 + selector: + matchLabels: + app: rollouts-demo + template: + metadata: + annotations: + linkerd.io/inject: enabled + labels: + app: rollouts-demo + spec: + containers: + - name: rollouts-demo + image: argoproj/rollouts-demo:blue + ports: + - name: http + containerPort: 8080 + protocol: TCP + resources: + requests: + memory: 32Mi + cpu: 5m diff --git a/docs/getting-started/mixed/services.yaml b/docs/getting-started/mixed/services.yaml new file mode 100644 index 0000000000..ad072f2487 --- /dev/null +++ b/docs/getting-started/mixed/services.yaml @@ -0,0 +1,30 @@ +apiVersion: v1 +kind: Service +metadata: + name: rollouts-demo-canary +spec: + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: rollouts-demo + # This selector will be updated with the pod-template-hash of the canary ReplicaSet. e.g.: + # rollouts-pod-template-hash: 7bf84f9696 + +--- +apiVersion: v1 +kind: Service +metadata: + name: rollouts-demo-stable +spec: + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: rollouts-demo + # This selector will be updated with the pod-template-hash of the stable ReplicaSet. e.g.: + # rollouts-pod-template-hash: 789746c88d diff --git a/docs/index.md b/docs/index.md index fcc6b42ec5..0174ed9f48 100644 --- a/docs/index.md +++ b/docs/index.md @@ -29,6 +29,7 @@ For these reasons, in large scale high-volume production environments, a rolling * Customizable metric queries and analysis of business KPIs * Ingress controller integration: NGINX, ALB * Service Mesh integration: Istio, Linkerd, SMI +* Simultaneous usage of multiple providers: SMI + NGINX, Istio + ALB, etc. * Metric provider integration: Prometheus, Wavefront, Kayenta, Web, Kubernetes Jobs, Datadog, New Relic, Graphite ## Quick Start diff --git a/rollout/controller.go b/rollout/controller.go index 8103e4605c..d333e443e4 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -139,9 +139,9 @@ type reconcilerBase struct { podRestarter RolloutPodRestarter // used for unit testing - enqueueRollout func(obj interface{}) //nolint:structcheck - enqueueRolloutAfter func(obj interface{}, duration time.Duration) //nolint:structcheck - newTrafficRoutingReconciler func(roCtx *rolloutContext) (trafficrouting.TrafficRoutingReconciler, error) //nolint:structcheck + enqueueRollout func(obj interface{}) //nolint:structcheck + enqueueRolloutAfter func(obj interface{}, duration time.Duration) //nolint:structcheck + newTrafficRoutingReconciler func(roCtx *rolloutContext) ([]trafficrouting.TrafficRoutingReconciler, error) //nolint:structcheck // recorder is an event recorder for recording Event resources to the Kubernetes API. recorder record.EventRecorder @@ -211,7 +211,6 @@ func NewController(cfg ControllerConfig) *Controller { VirtualServiceInformer: cfg.IstioVirtualServiceInformer, DestinationRuleInformer: cfg.IstioDestinationRuleInformer, }) - controller.newTrafficRoutingReconciler = controller.NewTrafficRoutingReconciler log.Info("Setting up event handlers") diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 110af21c21..f8881bc33d 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -102,8 +102,9 @@ type fixture struct { unfreezeTime func() error // events holds all the K8s Event Reasons emitted during the run - events []string - fakeTrafficRouting *mocks.TrafficRoutingReconciler + events []string + fakeTrafficRouting []*mocks.TrafficRoutingReconciler + fakeSingleTrafficRouting *mocks.TrafficRoutingReconciler } func newFixture(t *testing.T) *fixture { @@ -116,7 +117,9 @@ func newFixture(t *testing.T) *fixture { patch, err := mpatch.PatchMethod(time.Now, func() time.Time { return now }) assert.NoError(t, err) f.unfreezeTime = patch.Unpatch + f.fakeTrafficRouting = newFakeTrafficRoutingReconciler() + f.fakeSingleTrafficRouting = newFakeSingleTrafficRoutingReconciler() return f } @@ -550,12 +553,13 @@ func (f *fixture) newController(resync resyncFunc) (*Controller, informers.Share c.enqueueRolloutAfter = func(obj interface{}, duration time.Duration) { c.enqueueRollout(obj) } - - c.newTrafficRoutingReconciler = func(roCtx *rolloutContext) (trafficrouting.TrafficRoutingReconciler, error) { + c.newTrafficRoutingReconciler = func(roCtx *rolloutContext) ([]trafficrouting.TrafficRoutingReconciler, error) { if roCtx.rollout.Spec.Strategy.Canary == nil || roCtx.rollout.Spec.Strategy.Canary.TrafficRouting == nil { return nil, nil } - return f.fakeTrafficRouting, nil + var reconcilers = []trafficrouting.TrafficRoutingReconciler{} + reconcilers = append(reconcilers, f.fakeSingleTrafficRouting) + return reconcilers, nil } for _, r := range f.rolloutLister { diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go index 1162d8e865..dda57ea96f 100644 --- a/rollout/trafficrouting.go +++ b/rollout/trafficrouting.go @@ -18,168 +18,197 @@ import ( ) // NewTrafficRoutingReconciler identifies return the TrafficRouting Plugin that the rollout wants to modify -func (c *Controller) NewTrafficRoutingReconciler(roCtx *rolloutContext) (trafficrouting.TrafficRoutingReconciler, error) { +func (c *Controller) NewTrafficRoutingReconciler(roCtx *rolloutContext) ([]trafficrouting.TrafficRoutingReconciler, error) { rollout := roCtx.rollout + // define an empty list of trafficReconcilers to be populated + // by the ones declared in the rolloutContext + trafficReconcilers := []trafficrouting.TrafficRoutingReconciler{} + if rollout.Spec.Strategy.Canary.TrafficRouting == nil { return nil, nil } if rollout.Spec.Strategy.Canary.TrafficRouting.Istio != nil { if c.IstioController.VirtualServiceInformer.HasSynced() { - return istio.NewReconciler(rollout, c.IstioController.DynamicClientSet, c.recorder, c.IstioController.VirtualServiceLister, c.IstioController.DestinationRuleLister), nil + trafficReconcilers = append(trafficReconcilers, istio.NewReconciler(rollout, c.IstioController.DynamicClientSet, c.recorder, c.IstioController.VirtualServiceLister, c.IstioController.DestinationRuleLister)) } else { - return istio.NewReconciler(rollout, c.IstioController.DynamicClientSet, c.recorder, nil, nil), nil + trafficReconcilers = append(trafficReconcilers, istio.NewReconciler(rollout, c.IstioController.DynamicClientSet, c.recorder, nil, nil)) } } if rollout.Spec.Strategy.Canary.TrafficRouting.Nginx != nil { - return nginx.NewReconciler(nginx.ReconcilerConfig{ + trafficReconcilers = append(trafficReconcilers, nginx.NewReconciler(nginx.ReconcilerConfig{ Rollout: rollout, Client: c.kubeclientset, Recorder: c.recorder, ControllerKind: controllerKind, IngressLister: c.ingressesLister, - }), nil + })) } if rollout.Spec.Strategy.Canary.TrafficRouting.ALB != nil { - return alb.NewReconciler(alb.ReconcilerConfig{ + alb_reconcilier, err := alb.NewReconciler(alb.ReconcilerConfig{ Rollout: rollout, Client: c.kubeclientset, Recorder: c.recorder, ControllerKind: controllerKind, IngressLister: c.ingressesLister, }) + if err != nil { + return trafficReconcilers, err + } + trafficReconcilers = append(trafficReconcilers, alb_reconcilier) } if rollout.Spec.Strategy.Canary.TrafficRouting.SMI != nil { - return smi.NewReconciler(smi.ReconcilerConfig{ + smi_reconcilier, err := smi.NewReconciler(smi.ReconcilerConfig{ Rollout: rollout, Client: c.smiclientset, Recorder: c.recorder, ControllerKind: controllerKind, }) + if err != nil { + return trafficReconcilers, err + } + trafficReconcilers = append(trafficReconcilers, smi_reconcilier) } if rollout.Spec.Strategy.Canary.TrafficRouting.Ambassador != nil { ac := ambassador.NewDynamicClient(c.dynamicclientset, rollout.GetNamespace()) - return ambassador.NewReconciler(rollout, ac, c.recorder), nil + trafficReconcilers = append(trafficReconcilers, ambassador.NewReconciler(rollout, ac, c.recorder)) } + + // ensure that the trafficReconcilers is a healthy list and its not empty + if len(trafficReconcilers) > 0 { + return trafficReconcilers, nil + } + return nil, nil } func (c *rolloutContext) reconcileTrafficRouting() error { - reconciler, err := c.newTrafficRoutingReconciler(c) + reconcilers, err := c.newTrafficRoutingReconciler(c) + // a return here does ensure that all trafficReconcilers are healthy + // and sane in syntax if err != nil { return err } - if reconciler == nil { - // Not using traffic routing + // ensure that trafficReconcilers list is healthy + if len(reconcilers) == 0 { + c.log.Info("No TrafficRouting Reconcilers found") c.newStatus.Canary.Weights = nil return nil } - c.log.Infof("Reconciling TrafficRouting with type '%s'", reconciler.Type()) - - var canaryHash, stableHash string - if c.stableRS != nil { - stableHash = c.stableRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - } - if c.newRS != nil { - canaryHash = c.newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - } - err = reconciler.UpdateHash(canaryHash, stableHash) - if err != nil { - return err + if reconcilers == nil { + // Not using traffic routing + c.newStatus.Canary.Weights = nil + return nil } + c.log.Infof("Found %d TrafficRouting Reconcilers", len(reconcilers)) + // iterate over the list of trafficReconcilers + for _, reconciler := range reconcilers { + c.log.Infof("Reconciling TrafficRouting with type '%s'", reconciler.Type()) - currentStep, index := replicasetutil.GetCurrentCanaryStep(c.rollout) - desiredWeight := int32(0) - weightDestinations := make([]v1alpha1.WeightDestination, 0) - if rolloututil.IsFullyPromoted(c.rollout) { - // when we are fully promoted. desired canary weight should be 0 - } else if c.pauseContext.IsAborted() { - // when aborted, desired canary weight should be 0 (100% to stable), *unless* we - // are using dynamic stable scaling. In that case, we can only decrease canary weight - // according to available replica counts of the stable. - if c.rollout.Spec.Strategy.Canary.DynamicStableScale { - desiredWeight = 100 - ((100 * c.stableRS.Status.AvailableReplicas) / *c.rollout.Spec.Replicas) + var canaryHash, stableHash string + if c.stableRS != nil { + stableHash = c.stableRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] } - } else if c.newRS == nil || c.newRS.Status.AvailableReplicas == 0 { - // when newRS is not available or replicas num is 0. never weight to canary - } else if index != nil { - atDesiredReplicaCount := replicasetutil.AtDesiredReplicaCountsForCanary(c.rollout, c.newRS, c.stableRS, c.otherRSs, nil) - if !atDesiredReplicaCount { - // Use the previous weight since the new RS is not ready for a new weight - for i := *index - 1; i >= 0; i-- { - step := c.rollout.Spec.Strategy.Canary.Steps[i] - if step.SetWeight != nil { - desiredWeight = *step.SetWeight - break - } - } - } else if *index != int32(len(c.rollout.Spec.Strategy.Canary.Steps)) { - // This if statement prevents the desiredWeight from being set to 100 - // when the rollout has progressed through all the steps. The rollout - // should send all traffic to the stable service by using a weight of - // 0. If the rollout is progressing through the steps, the desired - // weight of the traffic routing service should be at the value of the - // last setWeight step, which is set by GetCurrentSetWeight. - desiredWeight = replicasetutil.GetCurrentSetWeight(c.rollout) + if c.newRS != nil { + canaryHash = c.newRS.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + } + err = reconciler.UpdateHash(canaryHash, stableHash) + if err != nil { + return err } - // 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 + currentStep, index := replicasetutil.GetCurrentCanaryStep(c.rollout) + desiredWeight := int32(0) + weightDestinations := make([]v1alpha1.WeightDestination, 0) + if rolloututil.IsFullyPromoted(c.rollout) { + // when we are fully promoted. desired canary weight should be 0 + } else if c.pauseContext.IsAborted() { + // when aborted, desired canary weight should be 0 (100% to stable), *unless* we + // are using dynamic stable scaling. In that case, we can only decrease canary weight + // according to available replica counts of the stable. + if c.rollout.Spec.Strategy.Canary.DynamicStableScale { + desiredWeight = 100 - ((100 * c.stableRS.Status.AvailableReplicas) / *c.rollout.Spec.Replicas) + } + } else if c.newRS == nil || c.newRS.Status.AvailableReplicas == 0 { + // when newRS is not available or replicas num is 0. never weight to canary + } else if index != nil { + atDesiredReplicaCount := replicasetutil.AtDesiredReplicaCountsForCanary(c.rollout, c.newRS, c.stableRS, c.otherRSs, nil) + if !atDesiredReplicaCount { + // Use the previous weight since the new RS is not ready for a new weight + for i := *index - 1; i >= 0; i-- { + step := c.rollout.Spec.Strategy.Canary.Steps[i] + if step.SetWeight != nil { + desiredWeight = *step.SetWeight + break } } - return nil + } else if *index != int32(len(c.rollout.Spec.Strategy.Canary.Steps)) { + // This if statement prevents the desiredWeight from being set to 100 + // when the rollout has progressed through all the steps. The rollout + // should send all traffic to the stable service by using a weight of + // 0. If the rollout is progressing through the steps, the desired + // weight of the traffic routing service should be at the value of the + // last setWeight step, which is set by GetCurrentSetWeight. + desiredWeight = replicasetutil.GetCurrentSetWeight(c.rollout) } - for _, templateStatus := range c.currentEx.Status.TemplateStatuses { - templateWeight := getTemplateWeight(templateStatus.Name) - weightDestinations = append(weightDestinations, v1alpha1.WeightDestination{ - ServiceName: templateStatus.ServiceName, - PodTemplateHash: templateStatus.PodTemplateHash, - Weight: *templateWeight, - }) + + // 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, + }) + } } } - } - err = reconciler.SetWeight(desiredWeight, weightDestinations...) - if err != nil { - c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: "TrafficRoutingError"}, err.Error()) - return err - } - if modified, newWeights := calculateWeightStatus(c.rollout, canaryHash, stableHash, desiredWeight, weightDestinations...); modified { - c.log.Infof("Previous weights: %v", c.rollout.Status.Canary.Weights) - c.log.Infof("New weights: %v", newWeights) - 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 - - if shouldVerifyWeight { - weightVerified, err := reconciler.VerifyWeight(desiredWeight, weightDestinations...) - c.newStatus.Canary.Weights.Verified = weightVerified + err = reconciler.SetWeight(desiredWeight, weightDestinations...) 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 + c.recorder.Warnf(c.rollout, record.EventOptions{EventReason: "TrafficRoutingError"}, err.Error()) + return err } - 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()) + if modified, newWeights := calculateWeightStatus(c.rollout, canaryHash, stableHash, desiredWeight, weightDestinations...); modified { + c.log.Infof("Previous weights: %v", c.rollout.Status.Canary.Weights) + c.log.Infof("New weights: %v", newWeights) + 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 + + 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()) + } } } } - return nil } diff --git a/rollout/trafficrouting_test.go b/rollout/trafficrouting_test.go index e669413879..1a2c8c569b 100644 --- a/rollout/trafficrouting_test.go +++ b/rollout/trafficrouting_test.go @@ -27,21 +27,35 @@ import ( ) // newFakeTrafficRoutingReconciler returns a fake TrafficRoutingReconciler with mocked success return values -func newFakeTrafficRoutingReconciler() *mocks.TrafficRoutingReconciler { - r := mocks.TrafficRoutingReconciler{} - r.On("Type").Return("fake") - r.On("SetWeight", mock.Anything).Return(nil) - r.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil) - r.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) - return &r +func newFakeTrafficRoutingReconciler() []*mocks.TrafficRoutingReconciler { + reconcilerList := []*mocks.TrafficRoutingReconciler{} + for _, trafficRoutingReconciler := range reconcilerList { + trafficRoutingReconciler.On("Type").Return("fake") + trafficRoutingReconciler.On("SetWeight", mock.Anything).Return(nil) + trafficRoutingReconciler.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil) + trafficRoutingReconciler.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) + } + return reconcilerList +} + +// newFakeTrafficRoutingReconciler returns a fake TrafficRoutingReconciler with mocked success return values +func newFakeSingleTrafficRoutingReconciler() *mocks.TrafficRoutingReconciler { + trafficRoutingReconciler := mocks.TrafficRoutingReconciler{} + trafficRoutingReconciler.On("Type").Return("fake") + trafficRoutingReconciler.On("SetWeight", mock.Anything).Return(nil) + trafficRoutingReconciler.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil) + trafficRoutingReconciler.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) + return &trafficRoutingReconciler } // newUnmockedFakeTrafficRoutingReconciler returns a fake TrafficRoutingReconciler with unmocked // methods (except Type() mocked) -func newUnmockedFakeTrafficRoutingReconciler() *mocks.TrafficRoutingReconciler { - r := mocks.TrafficRoutingReconciler{} - r.On("Type").Return("fake") - return &r +func newUnmockedFakeTrafficRoutingReconciler() *[]*mocks.TrafficRoutingReconciler { + reconcilerList := []*mocks.TrafficRoutingReconciler{} + for _, trafficRoutingReconciler := range reconcilerList { + trafficRoutingReconciler.On("Type").Return("fake") + } + return &reconcilerList } func newTrafficWeightFixture(t *testing.T) (*fixture, *v1alpha1.Rollout) { @@ -81,40 +95,45 @@ func newTrafficWeightFixture(t *testing.T) (*fixture, *v1alpha1.Rollout) { func TestReconcileTrafficRoutingSetWeightErr(t *testing.T) { f, ro := newTrafficWeightFixture(t) defer f.Close() - f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() - f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) - f.fakeTrafficRouting.On("SetWeight", mock.Anything).Return(errors.New("Error message")) - f.runExpectError(getKey(ro, t), true) + f.fakeTrafficRouting = *newUnmockedFakeTrafficRoutingReconciler() + for _, fakeTrafficRouting := range f.fakeTrafficRouting { + fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) + fakeTrafficRouting.On("SetWeight", mock.Anything).Return(errors.New("Error message")) + f.runExpectError(getKey(ro, t), true) + } } // verify error is not returned when VerifyWeight returns error (so that we can continue reconciling) func TestReconcileTrafficRoutingVerifyWeightErr(t *testing.T) { f, ro := newTrafficWeightFixture(t) defer f.Close() - f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() - f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) - f.fakeTrafficRouting.On("SetWeight", mock.Anything).Return(nil) - f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(false), errors.New("Error message")) - f.expectPatchRolloutAction(ro) - f.run(getKey(ro, t)) + f.fakeTrafficRouting = *newUnmockedFakeTrafficRoutingReconciler() + for _, fakeTrafficRouting := range f.fakeTrafficRouting { + fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) + fakeTrafficRouting.On("SetWeight", mock.Anything).Return(nil) + fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(false), errors.New("Error message")) + f.runExpectError(getKey(ro, t), true) + } } // verify we requeue when VerifyWeight returns false func TestReconcileTrafficRoutingVerifyWeightFalse(t *testing.T) { f, ro := newTrafficWeightFixture(t) defer f.Close() - f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() - f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) - f.fakeTrafficRouting.On("SetWeight", mock.Anything).Return(nil) - f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(false), nil) - c, i, k8sI := f.newController(noResyncPeriodFunc) - enqueued := false - c.enqueueRolloutAfter = func(obj interface{}, duration time.Duration) { - enqueued = true + f.fakeTrafficRouting = *newUnmockedFakeTrafficRoutingReconciler() + for _, fakeTrafficRouting := range f.fakeTrafficRouting { + fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) + fakeTrafficRouting.On("SetWeight", mock.Anything).Return(nil) + fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(false), nil) + c, i, k8sI := f.newController(noResyncPeriodFunc) + enqueued := false + c.enqueueRolloutAfter = func(obj interface{}, duration time.Duration) { + enqueued = true + } + f.expectPatchRolloutAction(ro) + f.runController(getKey(ro, t), true, false, c, i, k8sI) + assert.True(t, enqueued) } - f.expectPatchRolloutAction(ro) - f.runController(getKey(ro, t), true, false, c, i, k8sI) - assert.True(t, enqueued) } func TestRolloutUseDesiredWeight(t *testing.T) { @@ -160,14 +179,16 @@ func TestRolloutUseDesiredWeight(t *testing.T) { f.expectPatchRolloutAction(r2) - f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() - f.fakeTrafficRouting.On("UpdateHash", 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(10), desiredWeight) - return nil - }) - f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil) + f.fakeTrafficRouting = *newUnmockedFakeTrafficRoutingReconciler() + for _, fakeTrafficRouting := range f.fakeTrafficRouting { + fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) + 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(10), desiredWeight) + return nil + }) + fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(true, nil) + } f.run(getKey(r2, t)) } @@ -224,41 +245,45 @@ func TestRolloutWithExperimentStep(t *testing.T) { t.Run("Experiment Running - WeightDestination created", func(t *testing.T) { ex.Status.Phase = v1alpha1.AnalysisPhaseRunning - f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() - f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) - f.fakeTrafficRouting.On("SetWeight", mock.Anything, mock.Anything).Return(func(desiredWeight int32, weightDestinations ...v1alpha1.WeightDestination) error { - // make sure SetWeight was called with correct value - assert.Equal(t, int32(10), desiredWeight) - assert.Equal(t, int32(5), weightDestinations[0].Weight) - assert.Equal(t, ex.Status.TemplateStatuses[0].ServiceName, weightDestinations[0].ServiceName) - assert.Equal(t, ex.Status.TemplateStatuses[0].PodTemplateHash, weightDestinations[0].PodTemplateHash) - return nil - }) - f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(func(desiredWeight int32, weightDestinations ...v1alpha1.WeightDestination) error { - assert.Equal(t, int32(10), desiredWeight) - assert.Equal(t, int32(5), weightDestinations[0].Weight) - assert.Equal(t, ex.Status.TemplateStatuses[0].ServiceName, weightDestinations[0].ServiceName) - assert.Equal(t, ex.Status.TemplateStatuses[0].PodTemplateHash, weightDestinations[0].PodTemplateHash) - return nil - }) + f.fakeTrafficRouting = *newUnmockedFakeTrafficRoutingReconciler() + for _, fakeTrafficRouting := range f.fakeTrafficRouting { + fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) + fakeTrafficRouting.On("SetWeight", mock.Anything, mock.Anything).Return(func(desiredWeight int32, weightDestinations ...v1alpha1.WeightDestination) error { + // make sure SetWeight was called with correct value + assert.Equal(t, int32(10), desiredWeight) + assert.Equal(t, int32(5), weightDestinations[0].Weight) + assert.Equal(t, ex.Status.TemplateStatuses[0].ServiceName, weightDestinations[0].ServiceName) + assert.Equal(t, ex.Status.TemplateStatuses[0].PodTemplateHash, weightDestinations[0].PodTemplateHash) + return nil + }) + fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(func(desiredWeight int32, weightDestinations ...v1alpha1.WeightDestination) error { + assert.Equal(t, int32(10), desiredWeight) + assert.Equal(t, int32(5), weightDestinations[0].Weight) + assert.Equal(t, ex.Status.TemplateStatuses[0].ServiceName, weightDestinations[0].ServiceName) + assert.Equal(t, ex.Status.TemplateStatuses[0].PodTemplateHash, weightDestinations[0].PodTemplateHash) + return nil + }) + } f.run(getKey(r2, t)) }) t.Run("Experiment Pending - no WeightDestination created", func(t *testing.T) { ex.Status.Phase = v1alpha1.AnalysisPhasePending - f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() - f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) - f.fakeTrafficRouting.On("SetWeight", mock.Anything, mock.Anything).Return(func(desiredWeight int32, weightDestinations ...v1alpha1.WeightDestination) error { - // make sure SetWeight was called with correct value - assert.Equal(t, int32(10), desiredWeight) - assert.Len(t, weightDestinations, 0) - return nil - }) - f.fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(func(desiredWeight int32, weightDestinations ...v1alpha1.WeightDestination) error { - assert.Equal(t, int32(10), desiredWeight) - assert.Len(t, weightDestinations, 0) - return nil - }) + f.fakeTrafficRouting = *newUnmockedFakeTrafficRoutingReconciler() + for _, fakeTrafficRouting := range f.fakeTrafficRouting { + fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) + fakeTrafficRouting.On("SetWeight", mock.Anything, mock.Anything).Return(func(desiredWeight int32, weightDestinations ...v1alpha1.WeightDestination) error { + // make sure SetWeight was called with correct value + assert.Equal(t, int32(10), desiredWeight) + assert.Len(t, weightDestinations, 0) + return nil + }) + fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(func(desiredWeight int32, weightDestinations ...v1alpha1.WeightDestination) error { + assert.Equal(t, int32(10), desiredWeight) + assert.Len(t, weightDestinations, 0) + return nil + }) + } f.run(getKey(r2, t)) }) } @@ -301,15 +326,17 @@ func TestRolloutUsePreviousSetWeight(t *testing.T) { f.expectUpdateReplicaSetAction(rs2) f.expectPatchRolloutAction(r2) - f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() - f.fakeTrafficRouting.On("UpdateHash", 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(10), desiredWeight) - return nil - }) - f.fakeTrafficRouting.On("VerifyWeight", mock.Anything, mock.Anything).Return(pointer.BoolPtr(true), nil) - f.fakeTrafficRouting.On("error patching alb ingress", mock.Anything, mock.Anything).Return(true, nil) + f.fakeTrafficRouting = *newUnmockedFakeTrafficRoutingReconciler() + for _, fakeTrafficRouting := range f.fakeTrafficRouting { + fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) + 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(10), desiredWeight) + return nil + }) + fakeTrafficRouting.On("VerifyWeight", mock.Anything, mock.Anything).Return(pointer.BoolPtr(true), nil) + fakeTrafficRouting.On("error patching alb ingress", mock.Anything, mock.Anything).Return(true, nil) + } f.run(getKey(r2, t)) } @@ -343,14 +370,17 @@ func TestRolloutSetWeightToZeroWhenFullyRolledOut(t *testing.T) { f.objects = append(f.objects, r1) f.expectPatchRolloutAction(r1) - f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler() - f.fakeTrafficRouting.On("UpdateHash", 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("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil) + + f.fakeTrafficRouting = *newUnmockedFakeTrafficRoutingReconciler() + for _, fakeTrafficRouting := range f.fakeTrafficRouting { + fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything).Return(nil) + 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 + }) + fakeTrafficRouting.On("VerifyWeight", mock.Anything).Return(pointer.BoolPtr(true), nil) + } f.run(getKey(r1, t)) } @@ -400,10 +430,12 @@ func TestNewTrafficRoutingReconciler(t *testing.T) { rollout: r, log: logutil.WithRollout(r), } - networkReconciler, err := rc.NewTrafficRoutingReconciler(roCtx) - assert.Nil(t, err) - assert.NotNil(t, networkReconciler) - assert.Equal(t, istio.Type, networkReconciler.Type()) + networkReconcilerList, err := rc.NewTrafficRoutingReconciler(roCtx) + for _, networkReconciler := range networkReconcilerList { + assert.Nil(t, err) + assert.NotNil(t, networkReconciler) + assert.Equal(t, istio.Type, networkReconciler.Type()) + } } { // With istioVirtualServiceLister @@ -420,10 +452,12 @@ func TestNewTrafficRoutingReconciler(t *testing.T) { rollout: r, log: logutil.WithRollout(r), } - networkReconciler, err := rc.NewTrafficRoutingReconciler(roCtx) - assert.Nil(t, err) - assert.NotNil(t, networkReconciler) - assert.Equal(t, istio.Type, networkReconciler.Type()) + networkReconcilerList, err := rc.NewTrafficRoutingReconciler(roCtx) + for _, networkReconciler := range networkReconcilerList { + assert.Nil(t, err) + assert.NotNil(t, networkReconciler) + assert.Equal(t, istio.Type, networkReconciler.Type()) + } } { r := newCanaryRollout("foo", 10, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(0)) @@ -434,10 +468,12 @@ func TestNewTrafficRoutingReconciler(t *testing.T) { rollout: r, log: logutil.WithRollout(r), } - networkReconciler, err := rc.NewTrafficRoutingReconciler(roCtx) - assert.Nil(t, err) - assert.NotNil(t, networkReconciler) - assert.Equal(t, nginx.Type, networkReconciler.Type()) + networkReconcilerList, err := rc.NewTrafficRoutingReconciler(roCtx) + for _, networkReconciler := range networkReconcilerList { + assert.Nil(t, err) + assert.NotNil(t, networkReconciler) + assert.Equal(t, nginx.Type, networkReconciler.Type()) + } } { r := newCanaryRollout("foo", 10, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(0)) @@ -448,10 +484,12 @@ func TestNewTrafficRoutingReconciler(t *testing.T) { rollout: r, log: logutil.WithRollout(r), } - networkReconciler, err := rc.NewTrafficRoutingReconciler(roCtx) - assert.Nil(t, err) - assert.NotNil(t, networkReconciler) - assert.Equal(t, alb.Type, networkReconciler.Type()) + networkReconcilerList, err := rc.NewTrafficRoutingReconciler(roCtx) + for _, networkReconciler := range networkReconcilerList { + assert.Nil(t, err) + assert.NotNil(t, networkReconciler) + assert.Equal(t, alb.Type, networkReconciler.Type()) + } } { tsController := Controller{} @@ -463,10 +501,61 @@ func TestNewTrafficRoutingReconciler(t *testing.T) { rollout: r, log: logutil.WithRollout(r), } - networkReconciler, err := tsController.NewTrafficRoutingReconciler(roCtx) - assert.Nil(t, err) - assert.NotNil(t, networkReconciler) - assert.Equal(t, smi.Type, networkReconciler.Type()) + networkReconcilerList, err := tsController.NewTrafficRoutingReconciler(roCtx) + for _, networkReconciler := range networkReconcilerList { + assert.Nil(t, err) + assert.NotNil(t, networkReconciler) + assert.Equal(t, smi.Type, networkReconciler.Type()) + } + } + { + // (2) Multiple Reconcilers (Nginx + SMI) + tsController := Controller{} + r := newCanaryRollout("foo", 10, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(0)) + r.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ + Nginx: &v1alpha1.NginxTrafficRouting{}, + SMI: &v1alpha1.SMITrafficRouting{}, + } + roCtx := &rolloutContext{ + rollout: r, + log: logutil.WithRollout(r), + } + networkReconcilerList, err := tsController.NewTrafficRoutingReconciler(roCtx) + for position, networkReconciler := range networkReconcilerList { + if position == 0 { + assert.Equal(t, nginx.Type, networkReconciler.Type()) + } else if position == 1 { + assert.Equal(t, smi.Type, networkReconciler.Type()) + } + assert.Nil(t, err) + assert.NotNil(t, networkReconciler) + } + } + { + // (3) Multiple Reconcilers (ALB + Nginx + SMI) + tsController := Controller{} + r := newCanaryRollout("foo", 10, nil, steps, pointer.Int32Ptr(1), intstr.FromInt(1), intstr.FromInt(0)) + r.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ + ALB: &v1alpha1.ALBTrafficRouting{}, + Nginx: &v1alpha1.NginxTrafficRouting{}, + SMI: &v1alpha1.SMITrafficRouting{}, + } + roCtx := &rolloutContext{ + rollout: r, + log: logutil.WithRollout(r), + } + networkReconcilerList, err := tsController.NewTrafficRoutingReconciler(roCtx) + for position, networkReconciler := range networkReconcilerList { + if position == 0 { + assert.Equal(t, nginx.Type, networkReconciler.Type()) + } else if position == 1 { + assert.Equal(t, alb.Type, networkReconciler.Type()) + } else if position == 2 { + assert.Equal(t, smi.Type, networkReconciler.Type()) + } + assert.Nil(t, err) + assert.NotNil(t, networkReconciler) + } } } diff --git a/test/e2e/smi_ingress/rollout-smi-ingress-canary.yaml b/test/e2e/smi_ingress/rollout-smi-ingress-canary.yaml new file mode 100644 index 0000000000..024203b9f3 --- /dev/null +++ b/test/e2e/smi_ingress/rollout-smi-ingress-canary.yaml @@ -0,0 +1,89 @@ +apiVersion: v1 +kind: Service +metadata: + name: rollout-smi-ingress-canary-canary +spec: + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: rollout-smi-ingress-canary + # This selector will be updated with the pod-template-hash of the canary ReplicaSet. e.g.: + # rollouts-pod-template-hash: 7bf84f9696 +--- +apiVersion: v1 +kind: Service +metadata: + name: rollout-smi-ingress-canary-stable +spec: + ports: + - port: 80 + targetPort: http + protocol: TCP + name: http + selector: + app: rollout-smi-ingress-canary + # This selector will be updated with the pod-template-hash of the stable ReplicaSet. e.g.: + # rollouts-pod-template-hash: 789746c88d +--- +apiVersion: networking.k8s.io/v1beta1 +kind: Ingress +metadata: + name: rollout-smi-ingress-canary-stable + annotations: + kubernetes.io/ingress.class: nginx +spec: + rules: + - host: rollout-smi-ingress-canary.local + http: + paths: + - path: / + backend: + # Reference to a Service name, also specified in the Rollout spec.strategy.canary.stableService field + serviceName: rollout-smi-ingress-canary-stable + servicePort: 80 +--- +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: rollout-smi-ingress-canary +spec: + replicas: 1 + strategy: + canary: + canaryService: rollout-smi-ingress-canary-canary + stableService: rollout-smi-ingress-canary-stable + trafficRouting: + nginx: + stableIngress: rollout-smi-ingress-canary-stable + smi: + trafficSplitName: rollout-smi-ingress-canary-trafficsplit + steps: + - setWeight: 5 + - pause: + duration: 15 + - setWeight: 50 + - pause: + duration: 15 + revisionHistoryLimit: 2 + selector: + matchLabels: + app: rollout-smi-ingress-canary + template: + metadata: + labels: + app: rollout-smi-ingress-canary + spec: + containers: + - name: rollout-smi-ingress-canary + image: nginx:1.19-alpine + ports: + - name: http + containerPort: 80 + protocol: TCP + resources: + requests: + memory: 16Mi + cpu: 5m diff --git a/test/e2e/smi_ingress_test.go b/test/e2e/smi_ingress_test.go new file mode 100644 index 0000000000..2086935f84 --- /dev/null +++ b/test/e2e/smi_ingress_test.go @@ -0,0 +1,150 @@ +// +build e2e + +package e2e + +import ( + "testing" + "time" + + "github.com/stretchr/testify/suite" + "github.com/tj/assert" + + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" + "github.com/argoproj/argo-rollouts/test/fixtures" +) + +type SMIIngressSuite struct { + fixtures.E2ESuite +} + +func TestSMIIngressSuite(t *testing.T) { + suite.Run(t, new(SMIIngressSuite)) +} + +func (s *SMIIngressSuite) SetupSuite() { + s.E2ESuite.SetupSuite() + if !s.SMIEnabled { + s.T().SkipNow() + } +} + +func (s *SMIIngressSuite) TestSMIIngressCanaryStep() { + const ( + canaryService = "rollout-smi-ingress-canary-canary" + stableService = "rollout-smi-ingress-canary-stable" + canaryAnnotation = "nginx.ingress.kubernetes.io/canary" + canaryWeightAnnotation = "nginx.ingress.kubernetes.io/canary-weight" + ) + s.Given(). + RolloutObjects("@smi_ingress/rollout-smi-ingress-canary.yaml"). + When(). + ApplyManifests(). + WaitForRolloutStatus("Healthy"). + Then(). + Assert(func(t *fixtures.Then) { + ts := t.GetTrafficSplit() + + assert.Len(s.T(), ts.Spec.Backends, 2) + assert.Equal(s.T(), canaryService, ts.Spec.Backends[0].Service) + assert.Equal(s.T(), int64(0), ts.Spec.Backends[0].Weight.Value()) + + ingressStable := t.GetNginxIngressStable() + _, ko := ingressStable.Annotations[canaryAnnotation] + assert.False(s.T(), ko) + + desired, stable := t.GetServices() + rs1 := t.GetReplicaSetByRevision("1") + assert.Equal(s.T(), rs1.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], desired.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]) + assert.Equal(s.T(), rs1.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], stable.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]) + }). + ExpectExperimentCount(0). + When(). + UpdateSpec(). + WaitForRolloutCanaryStepIndex(1). + Sleep(5*time.Second). + Then(). + Assert(func(t *fixtures.Then) { + ts := t.GetTrafficSplit() + + assert.Len(s.T(), ts.Spec.Backends, 2) + + assert.Equal(s.T(), canaryService, ts.Spec.Backends[0].Service) + assert.Equal(s.T(), int64(5), ts.Spec.Backends[0].Weight.Value()) + + ingressCanary := t.GetNginxIngressCanary() + _, ok := ingressCanary.Annotations[canaryAnnotation] + assert.True(s.T(), ok) + assert.Equal(s.T(), string("5"), ingressCanary.Annotations[canaryWeightAnnotation]) + + ingressStable := t.GetNginxIngressStable() + _, ko := ingressStable.Annotations[canaryAnnotation] + assert.False(s.T(), ko) + + assert.Equal(s.T(), stableService, ts.Spec.Backends[1].Service) + assert.Equal(s.T(), int64(95), ts.Spec.Backends[1].Weight.Value()) + + desired, stable := t.GetServices() + rs1 := t.GetReplicaSetByRevision("1") + rs2 := t.GetReplicaSetByRevision("2") + assert.Equal(s.T(), rs2.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], desired.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]) + assert.Equal(s.T(), rs1.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], stable.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]) + }). + ExpectExperimentCount(0). + When(). + WaitForRolloutCanaryStepIndex(2). + Sleep(3*time.Second). + Then(). + Assert(func(t *fixtures.Then) { + ts := t.GetTrafficSplit() + + assert.Len(s.T(), ts.Spec.Backends, 2) + + assert.Equal(s.T(), canaryService, ts.Spec.Backends[0].Service) + assert.Equal(s.T(), int64(50), ts.Spec.Backends[0].Weight.Value()) + + ingressCanary := t.GetNginxIngressCanary() + _, ok := ingressCanary.Annotations[canaryAnnotation] + assert.True(s.T(), ok) + assert.Equal(s.T(), string("50"), ingressCanary.Annotations[canaryWeightAnnotation]) + + ingressStable := t.GetNginxIngressStable() + _, ko := ingressStable.Annotations[canaryAnnotation] + assert.False(s.T(), ko) + + assert.Equal(s.T(), stableService, ts.Spec.Backends[1].Service) + assert.Equal(s.T(), int64(50), ts.Spec.Backends[1].Weight.Value()) + + desired, stable := t.GetServices() + rs1 := t.GetReplicaSetByRevision("1") + rs2 := t.GetReplicaSetByRevision("2") + assert.Equal(s.T(), rs2.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], desired.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]) + assert.Equal(s.T(), rs1.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], stable.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]) + }). + 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) { + ts := t.GetTrafficSplit() + + assert.Len(s.T(), ts.Spec.Backends, 2) + + ingressCanary := t.GetNginxIngressCanary() + _, ko := ingressCanary.Annotations[canaryAnnotation] + assert.True(s.T(), ko) + assert.Equal(s.T(), string("0"), ingressCanary.Annotations[canaryWeightAnnotation]) + + assert.Equal(s.T(), canaryService, ts.Spec.Backends[0].Service) + assert.Equal(s.T(), int64(0), ts.Spec.Backends[0].Weight.Value()) + + assert.Equal(s.T(), stableService, ts.Spec.Backends[1].Service) + assert.Equal(s.T(), int64(100), ts.Spec.Backends[1].Weight.Value()) + + desired, stable := t.GetServices() + rs2 := t.GetReplicaSetByRevision("2") + assert.Equal(s.T(), rs2.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], desired.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]) + assert.Equal(s.T(), rs2.Spec.Template.Labels[v1alpha1.DefaultRolloutUniqueLabelKey], stable.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey]) + }). + ExpectRevisionPodCount("1", 1) // don't scale down old replicaset since it will be within scaleDownDelay +} diff --git a/test/fixtures/common.go b/test/fixtures/common.go index 4a734c78ff..fa0c5b7def 100644 --- a/test/fixtures/common.go +++ b/test/fixtures/common.go @@ -535,6 +535,22 @@ func (c *Common) GetALBIngress() *extensionsv1beta1.Ingress { return ingress } +func (c *Common) GetNginxIngressStable() *extensionsv1beta1.Ingress { + ro := c.Rollout() + name := ro.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress + ingress, err := c.kubeClient.ExtensionsV1beta1().Ingresses(c.namespace).Get(c.Context, name, metav1.GetOptions{}) + c.CheckError(err) + return ingress +} + +func (c *Common) GetNginxIngressCanary() *extensionsv1beta1.Ingress { + ro := c.Rollout() + name := ro.Name + "-" + ro.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress + "-canary" + ingress, err := c.kubeClient.ExtensionsV1beta1().Ingresses(c.namespace).Get(c.Context, name, metav1.GetOptions{}) + c.CheckError(err) + return ingress +} + func (c *Common) GetTrafficSplit() *smiv1alpha1.TrafficSplit { ro := c.Rollout() name := ro.Spec.Strategy.Canary.TrafficRouting.SMI.TrafficSplitName