Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add traffic mirroring for Istio service mesh #311

Merged
merged 5 commits into from
Oct 5, 2019

Conversation

andrewjjenkins
Copy link
Contributor

@andrewjjenkins andrewjjenkins commented Sep 23, 2019

Traffic mirroring is a pre-stage for canary deployments. When mirroring
is enabled, at the beginning of a canary deployment traffic is mirrored
to the canary instead of shifted for one canary period. The service
mesh should mirror by copying the request and sending one copy to the
primary and one copy to the canary; only the response from the primary
is sent to the user. The response from the canary is only used for
collecting metrics.

Once the mirror period is over, the canary proceeds as usual, shifting
traffic from primary to canary until complete.

Traffic mirroring can also be used for Blue/Green deployments Fix: #313

@andrewjjenkins
Copy link
Contributor Author

I marked this as work-in-progress as I think it needs some high-level review and deciding what adjustments need to be made to the approach.

This implementation creates a traffic mirroring pre-stage for canary deployments. It does this by adding a field to the canary spec called Mirror. If true, we run a pre-stage where we tell Istio to mirror traffic to the canary. Here's an example prometheus plot:

image

The top teal line is primary, the red line is mirrored traffic to canary, the green line is shifted traffic to canary.

First question I have is what should be the appropriate behavior if the service mesh doesn't support mirroring? I implemented and tested for Istio but some meshes & SMI don't support mirroring.

@stefanprodan
Copy link
Member

@andrewjjenkins I think mirroring should be enabled for Blue/Green deployments as well.

First question I have is what should be the appropriate behavior if the service mesh doesn't support mirroring? I implemented and tested for Istio but some meshes & SMI don't support mirroring.

For SMI and others, mirroring should be off be default, the code looks ok as it this. Thanks

@stefanprodan
Copy link
Member

I have changed the way promotion works in #310 I'll merge it soon, please rebase with master after the merge, as some changes could conflict with your branch.

Andrew Jenkins added 2 commits September 24, 2019 16:15
The mirror option will be used to tell routers to configure traffic
mirroring.  Implement mirror for GetRoutes and SetRoutes for Istio.  For
other routers, GetRoutes always returns mirror == false, and SetRoutes
ignores mirror.

After this change there is no behavior change because no code sets
mirror true (yet).

Enhanced TestIstioRouter_SetRoutes and TestIstioRouter_GetRoutes.
SetupMocks() currently takes a bool switch that tells it to configure
against either a shifting canary or an A-B canary.  I'll need a third
canary that has mirroring turned on so I changed this to an interface
that just takes the canary to configure (and configs the default
shifting canary if you pass nil).
@andrewjjenkins
Copy link
Contributor Author

andrewjjenkins commented Sep 24, 2019

OK, rebased in #310, reduced the log message you pointed out to Infof and deleted some all others (left over from debugging).

Still remaining: Blue/Green implementation and I think I should add this to the docs as well.

@andrewjjenkins andrewjjenkins changed the title WIP: Add traffic mirroring for Istio service mesh Add traffic mirroring for Istio service mesh Sep 24, 2019
@andrewjjenkins
Copy link
Contributor Author

@stefanprodan I think I've addressed your comments & work that I'm aware of.

The B/G implementation: if "mirror: true" then mirror traffic for the entire canary analysis phase. Open to suggestions here if you had a different idea on how it should work.

Thanks & I think it's ready for you to take a look.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the mirror field to the CRD yamls (artifacts/flagger, charts/flagger and kustomize/base).

Andrew Jenkins added 3 commits October 3, 2019 14:33
Traffic mirroring is a pre-stage for canary deployments.  When mirroring
is enabled, at the beginning of a canary deployment traffic is mirrored
to the canary instead of shifted for one canary period.  The service
mesh should mirror by copying the request and sending one copy to the
primary and one copy to the canary; only the response from the primary
is sent to the user.  The response from the canary is only used for
collecting metrics.

Once the mirror period is over, the canary proceeds as usual, shifting
traffic from primary to canary until complete.

Added TestScheduler_Mirroring unit test.
Traffic mirroring for blue/green will mirror traffic for the entire
canary analysis phase of the blue/green deployment.
@andrewjjenkins
Copy link
Contributor Author

Sorry for the delay, added to those three copies of the CRD and rebased in (see e.g. e384b03#diff-5614edd054a6dc84c5fab6ccd18b9600 ).

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks a lot @andrewjjenkins 🥇

@andrewjjenkins
Copy link
Contributor Author

Thanks, I don't see a merge button so I'll wait for you to click it.

@stefanprodan stefanprodan merged commit 9a9baad into fluxcd:master Oct 5, 2019
@stefanprodan stefanprodan mentioned this pull request Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support traffic shadowing for Blue/Green deployments
2 participants