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 shifting strategy #2560

Closed
wants to merge 1 commit into from
Closed

Add traffic shifting strategy #2560

wants to merge 1 commit into from

Conversation

chenquanzhao
Copy link

What this PR does / why we need it:

For the sake of reducing downtime and risk of updating a service, we usually adopt Blue-Green Deployment or Gray Deployment that ensures the coexistence of the old and new service version. We can shift partial traffic to the new service by weight percentage or some extra routing rules, such as some keyword from cookie or request header and so on. And nginx ingress controller is served as the traffic entrypoint for all cluster services, it should offer the capability of traffic management.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #2419

Special notes for your reviewer:
Design doc: https://docs.google.com/document/d/1AkffGGtjy38n6GXRlTMPQpRUsto8zKjzCroZcJuXIs8/edit?usp=sharing
@aledbf

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 24, 2018
@aledbf
Copy link
Member

aledbf commented May 24, 2018

@chenqz1987 not sure about this because requires too many changes

@chenquanzhao
Copy link
Author

@aledbf yeah... but it is a very valuable feature, and we can discuss the design and plan together. Do you have any idea or comments about the design?

@geun
Copy link

geun commented May 31, 2018

+1

1 similar comment
@BSWANG
Copy link

BSWANG commented Jun 6, 2018

+1

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 7, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 7, 2018
@barockok
Copy link

barockok commented Jun 8, 2018

+1

1 similar comment
@dahsing
Copy link

dahsing commented Jun 8, 2018

+1

@chenquanzhao chenquanzhao changed the title [WIP] Add traffic shifting strategy Add traffic shifting strategy Jun 15, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2018
@codecov-io
Copy link

Codecov Report

Merging #2560 into master will decrease coverage by 1.81%.
The diff coverage is 23.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2560      +/-   ##
==========================================
- Coverage   40.77%   38.95%   -1.82%     
==========================================
  Files          74       76       +2     
  Lines        5077     5650     +573     
==========================================
+ Hits         2070     2201     +131     
- Misses       2724     3133     +409     
- Partials      283      316      +33
Impacted Files Coverage Δ
internal/ingress/controller/config/config.go 98.23% <ø> (ø) ⬆️
internal/ingress/controller/controller.go 1.68% <0%> (-0.54%) ⬇️
internal/ingress/controller/nginx.go 11.45% <0%> (-0.06%) ⬇️
internal/ingress/annotations/annotations.go 81.25% <100%> (+0.6%) ⬆️
internal/ingress/types_equals.go 10.58% <3.75%> (-1.84%) ⬇️
internal/ingress/annotations/serviceweight/main.go 38.23% <38.23%> (ø)
internal/ingress/annotations/servicematch/main.go 40.35% <40.35%> (ø)
internal/ingress/controller/template/template.go 60.15% <48.63%> (-3.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fee8704...28c19ce. Read the comment docs.

@aledbf aledbf added the do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. label Jun 15, 2018
@aoxn
Copy link

aoxn commented Jul 12, 2018

+1

@cheyang
Copy link

cheyang commented Jul 12, 2018

+1

1 similar comment
@yuyang0
Copy link

yuyang0 commented Jul 17, 2018

+1

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chenquanzhao
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: dcbw

If they are not already assigned, you can assign the PR to them by writing /assign @dcbw in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 20, 2018
@ElvinEfendi
Copy link
Member

@chenquanzhao thanks for this PR! I'd like to help to get this feature out as soon as possible. As @aledbf mentions above the PR introduces significant changes. I drafted a quick proposal that achieves approximately the same result with minimal changes only: https://docs.google.com/document/d/1qKTyLBLuKIYE6d6BsFXRM7zYB-2MUk6qJjtBL1KCz78/edit?usp=sharing

It would be great to get your opinions on it.

@chenquanzhao
Copy link
Author

@ElvinEfendi Appreciate your help and I already add some comments to the document for your reference.

@yaron2
Copy link

yaron2 commented Oct 31, 2018

Hi, any updates regarding this PR?

@ElvinEfendi
Copy link
Member

@yaron2 the feature is being implemented at Shopify#120. Most likely it will land in dev build of master branch by the end of this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

traffic split by request header or cookie