-
Notifications
You must be signed in to change notification settings - Fork 460
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
Adding GEP 3171: Percentage-based Request Mirroring #3178
Conversation
Welcome @jakebennert! |
Hi @jakebennert. Thanks for your PR. I'm waiting for a kubernetes-sigs 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-sigs/prow repository. |
Love the idea! I personally would vote for allowing two forms:
with validation allowing only one to be set (and we can define that if they are somehow both set, This is slightly harder on implementers, but I expect that the vast majority of users will only need an integral percentage, and forcing all of them to write mirrorFraction:
numerator: 5 instead of mirrorPercent: 5 feels like putting the difficulty on the wrong users. It's definitely important to support the case where you have enough traffic that you need to mirror 1/10000th of it, but I don't think that's the common case, and I'd rather the common case be simple. 🙂 |
I'd like to point out that we already have a concept of Weight that works on percentages, and it is a shared type. gateway-api/apis/v1/shared_types.go Lines 268 to 286 in c8fd84d
|
@candita, I think BackendRef's weight definition won't well for mirroring, sadly. BackendRef's weight scheme is almost the inverse of what we want: it relies on you to put large weights on the refs that should get a lot of traffic, and the only way to describe a small amount of traffic is to put large weights on all the other refs. This is problematic with mirroring, where we'd really like a really easy way to set up mirroring of a very small fraction of traffic without having to edit all the resources describing the critical production paths. Overall, I think we really are going to need an arbitrary-fraction type. |
Thanks @jakebennert! /ok-to-test |
Did you mean "the only way to describe a small amount of traffic is to put small weights on all the other refs"? It looks like the default e.g. if we have four backendRefs, and one of them we want to have a majority of all traffic. From the godoc, it follows that you can leave 3 |
@candita, in my experience, what I would often see happen is that the app devs will have a running workload, then decide that they want to enable shadowing for some reason, which means that they're in the situation where they want to add something that gets a small fraction of traffic after they've already set up the normal backends. It's much more user-friendly to add a single resource for "here's a backend that I want to shadow a tiny fraction of traffic" than to have to edit all your existing backends to add a large weight to them, then add a new resource that asks for a small fraction of traffic. Also, suppose you currently have three backends with weights of 50%, 25%, and 25%, you get 50K RPS, and you want to shadow no more than 50RPS. Quick, what weights do you need to use to specify that? Should you re-edit the weights back to 50/25/25 when you turn off shadowing a week later? How about if you decide the shadow needs to drop to 25RPS? Do we start telling people that the best practice is to deploy with a weight of 10000 for all their backends in case they need to mess with stuff like this? 😐 Let's just sidestep all those questions by having a way to say "here's a backend, give it 1/1000th of the traffic to this route, please." 🙂 |
@kflynn I think this is a good idea. I've updated my proposal - how does this look to you? |
@jakebennert Looks great to me, thank you!! 🙂 /lgtm |
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.
Thanks @jakebennert! One tiny nit about status, but otherwise LGTM. Would appreciate another LGTM from someone else.
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
@kflynn I know you LGTM'd in a previous comment, but I think because I made some commits after the fact, it's out of date. If it all looks good to you, would you be able to LGTM again? :) |
Thanks @jakebennert! Will defer to @kflynn (or anyone else) for LGTM /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jakebennert, robscott 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 |
/lgtm again 🙂 |
…3178) * Create index.md * Create metadata.yaml * Update index.md * Update metadata.yaml * Remove 'omitempty' from Fraction fields * Split MirrorPercent into two fields: MirrorPercent and MirrorFraction. * Update index.md * Add config examples. * Add Existing Support Table * Update field names to avoid 'stuttering' in names. * Update geps/gep-3171/index.md Co-authored-by: Rob Scott <rob.scott87@gmail.com> --------- Co-authored-by: Rob Scott <rob.scott87@gmail.com>
What type of PR is this?
/kind gep
What this PR does / why we need it:
This adds a new GEP proposing a new feature Percentage-based Request Mirroring.
Does this PR introduce a user-facing change?: