Extend DestinationRule with retry budget #2585
Conversation
|
Hi @quraynai. Thanks for your PR. I'm waiting for a istio 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. |
| @@ -791,6 +791,9 @@ message ConnectionPoolSettings { | |||
| // Note that when this is set to true, h2_upgrade_policy will be ineffective i.e. the client | |||
| // connections will not be upgraded to http2. | |||
| bool use_client_protocol = 7; | |||
|
|
|||
| // Specifies a limit on concurrent retries in relation to the number of active requests. This parameter is optional. | |||
| RetryBudget retry_budget = 8; | |||
There was a problem hiding this comment.
It seems confusing to configure retries in virtual service, but a retry budget in destination rule. Wouldn't a user configure those together?
There was a problem hiding this comment.
@howardjohn
Retry in virtualservice I think it's for configuring route.
But base on envoy document this is for configuring cluster.
cluster.circuit_breakers.thresholds.retry_budget
Also other fields of HTTPSettings will try to configure
cluster.circuit_breakers.thresholds.*
I think it might be reasonable to put it here.
https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/cluster/v3/circuit_breaker.proto
There was a problem hiding this comment.
I agree with @howardjohn 's concern on DR. With your porposal, what if user configures retry budget on DR but forgot to configure retry in VS?
There was a problem hiding this comment.
@linsun
I understand that this will make user to maintain 2 different resources (VS, DR) in case if they want to include retry_budget
but not sure if we move this configuration to VS will cause another confusion to user or not?
due to DR is for configuring cluster and VS is for configuring route
if user create VS user has to check into envoy config about route only. then user has to check for route and cluster instead
and another point is retry_budget from istio is about playing around with max_retries that already existed in DR.
also base on envoy Circuit Breaker other related config is already in DR as well
https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/cluster/v3/circuit_breaker.proto#envoy-v3-api-msg-config-cluster-v3-circuitbreakers-thresholds-retrybudget
https://istio.io/latest/docs/reference/config/networking/destination-rule/#ConnectionPoolSettings-HTTPSettings
There was a problem hiding this comment.
also this will make implementation on istio/istio more align with current implementation
https://github.com/istio/istio/pull/42232/files
There was a problem hiding this comment.
@linsun
from @ramaraochavali replied. Is it okay to have it on istioAnalyze instead?
There was a problem hiding this comment.
@linsun hi could you please help check this MR again?
There was a problem hiding this comment.
@linsun can you help on this? 1.17 is already released. we will try to aim for 1.18 instead.
In our company we are running 1.16 forked to support retry budget at the moment.
we really want this functionality support from istio and then we can aim for next istio upgrade.
There was a problem hiding this comment.
@howardjohn @hzxuzhonghu can you help on this?
There was a problem hiding this comment.
@linsun @howardjohn @hzxuzhonghu could you please help review this again?
About confusion of retry is it okay to go with IstioAnalayze instead?
|
@howardjohn hi can you help review this again? |
|
PR needs rebase. 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. |
Issue: istio/istio#27419
Istio: istio/istio#42232