Attempt admission control extension point for circuit breaker#43792
Attempt admission control extension point for circuit breaker#43792mathetake wants to merge 3 commits intoenvoyproxy:mainfrom
Conversation
|
/retest |
Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com>
002eaa3 to
949f083
Compare
Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
| // circuit breakers. | ||
| // | ||
| // In addition, extensions in this field may deny the initial attempt of a request. | ||
| core.v3.TypedExtensionConfig attempt_admission_control = 9; |
There was a problem hiding this comment.
May just call it circuit_breaker_extension or something is easier to under stand. :) Or is any strong reason to call it attempt_admission_control?
There was a problem hiding this comment.
basically this is an extension point that controls "attempt" in the retry routine, not the entire circuit breaker - so that's why it's called "attempt admission control" - but yes I am open to add _extension suffix to be clear
There was a problem hiding this comment.
If this is mainly for attempt in retry routine, should this only be an extension in the retry_budget?
There was a problem hiding this comment.
not exactly the "budget" itself either so that feels a bit weird (see the virtual interface)
There was a problem hiding this comment.
I see. After re check the implementation code of this PR, I get what you mean.
|
This PR introduced a new extension point to our core API of Cluster, so will let Mark (represent non-Envoy xDS implementations) to take a check to the API. From my perspective, now I have no very strong point except i prefer a more frankly name like Another point from me is could we achieve similar target with upstream L7 filter + shared filter state? (I didn't see your actual implementation for this new extension point, but the shared filter state could be used to record state and upstream filter could be used to collect necessary context for every attempt. May be it's could be more flexible rather than introducing new extension point. But not sure.) |
|
Hey @wbpcode , Here's some more context in how this was developed. We've been leveraging something like this internally for a while and @mathetake was nice enough to spend the time upstreaming :). This is similar to #31565, which was an attempt to make retry budgets extension points. This design would allow us to move the existing retry budgets of static, budget percent as drop in extensions https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/cluster/v3/circuit_breaker.proto#config-cluster-v3-circuitbreakers-thresholds-retrybudget e.g. moving core code -> extensions without regressions. The primary reason this is applied on a cluster is different clusters might have different policies for rejecting attempts / retries -- for example particular clusters might have request distributions of different priorities and might want to curtail them differently e.g. as a particular svc gets a lot of hedges in one case. |
Commit Message: Attempt admission control extension point for circuit breaker
Additional Description:
This adds a new extension point for circuit breaker to allow new extensions to determine whether an attempt to a single stream should be allowed. For example, this enables some advanced request context aware retry strategy like user foo should be allowed at this point vs bar shouldn't be etc.
Note: this commit itself doesn't add any concrete extension point implementation except inside the test. The plan is to add the dynamic module-based implementation as well as we already have an implementation internally too.
Risk Level: mid (some small change in the core)
Testing: done
Docs Changes: done (on API)
Release Notes: no concrete implementation yet
Platform Specific Features: n/a
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]