-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Enable session affinity for canaries #7371
Conversation
Hi @wasker. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
@rikatz This is a replacement PR for canary affinity fix. |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor reviews from Go side (mostly about code cleanup/keeping the standard).
From lua side, have some questions that we will discuss in some zoom call :)
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rikatz, wasker 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 |
What this PR does / why we need it:
If canary ingress is configured, session affinity settings no longer apply, which results in requests within the same session being served by both canary and non-canary endpoints. This affects scenarios when backwards-incompatible changes were deployed to canary, and in inconsistent results being served in general.
Types of changes
The change makes canaries respect session affinity settings by default, as this makes more sense than the current behavior. Nonetheless, there's a way to restore original behavior by adding a special annotation on a non-canary ingress definition, if someone prefers that behavior for some reason.
Which issue/s this PR fixes
Fixes #3717.
How Has This Been Tested?
Used repro steps from the bug to simulate the issue before and after changes.
Checklist: