-
Notifications
You must be signed in to change notification settings - Fork 347
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
API: Support Circuit Breakers in BackendTrafficPolicy #2284
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,12 @@ type BackendTrafficPolicySpec struct { | |
// | ||
// +optional | ||
TCPKeepalive *TCPKeepalive `json:"tcpKeepalive,omitempty"` | ||
|
||
// Circuit Breaker settings for the upstream connections and requests. | ||
// If not set, circuit breakers will be enabled with the highest supported thresholds | ||
// | ||
// +optional | ||
CircuitBreakers *CircuitBreakers `json:"circuitBreakers,omitempty"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why plural? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the term used by Envoy, the Backend Traffic Policy and other OSS projects like Emissary Ingress. A single There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we only need one |
||
} | ||
|
||
// BackendTrafficPolicyStatus defines the state of BackendTrafficPolicy | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
// Copyright Envoy Gateway Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// The full text of the Apache license is available in the LICENSE file at | ||
// the root of the repo. | ||
|
||
package v1alpha1 | ||
|
||
// CircuitBreakers defines the Circuit Breakers configuration. | ||
type CircuitBreakers struct { | ||
// List of Circuit Breaker Thresholds | ||
// At most one Thresholds resource is supported. | ||
// | ||
// +kubebuilder:validation:MaxItems:=1 | ||
// +optional | ||
Thresholds []Thresholds `json:"thresholds,omitempty"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in the description of this PR, only one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept this as list for two reasons:
We can decide to keep things simpler for the common use case of tweaking default circuit breakers, and, in the future, allow a list for non-default priorities.
If the maintainers support this I don't have any objection . WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works for me. A one-member list just looks weird to me. But would love to hear @AliceProxy 's opinion on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I vote to keep it singular for now
I dont see routing priority being added in the upstream Gateway API, and if it does, we can deprecate |
||
} | ||
|
||
type Thresholds struct { | ||
// The maximum number of connections that Envoy will make to the referenced backend (per xRoute). | ||
// Default: 1024 | ||
// | ||
// +optional | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. better go with CEL for all these fields, for example: // +kubebuilder:validation:Minimum=xxx
// +kubebuilder:validation:Maximum=xxx
// +kubebuilder:default=xxx |
||
MaxConnections *uint32 `json:"maxConnections,omitempty"` | ||
|
||
// The maximum number of pending requests that Envoy will allow to the referenced backend (per xRoute). | ||
// Default: 1024 | ||
// | ||
// +optional | ||
MaxPendingRequests *uint32 `json:"maxPendingRequests,omitempty"` | ||
|
||
// The maximum number of parallel requests that Envoy will make to the referenced backend (per xRoute). | ||
// Default: 1024 | ||
// | ||
// +optional | ||
MaxRequests *uint32 `json:"maxParallelRequests,omitempty"` | ||
|
||
// The maximum number of parallel retries that Envoy will allow to the referenced backend (per xRoute). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. vote to rm this for now, raise a issue to track max parallel retries, and once the retry API is complete, we can revisit this field and decide on the right home for this |
||
// Default: 3 | ||
// | ||
// +optional | ||
MaxRetries *uint32 `json:"maxRetries,omitempty"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're talking about two different things here: the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. However, note that #2168 deals with In Envoy, the separation of route and cluster settings is pretty clear. Multiple routes can point to the same cluster, and so In Envoy Gateway, we have a cluster for each If in the future Envoy Gateway does support a notion of shared backends (e.g. by translating services to clusters in some situations) and Envoy Gateway will support a Another aspect to consider is that these settings are scoped to a Routing Priority level. As long as only the I'm willing to drop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
IMO, the concurrent max retries setting belongs to Circuit Breaker logic because it enforces back pressure on the clients. Therefore, EG probably should not mix it with the request retries configuration. Use Istio as an example: Istio puts them into two places: the concurrent max retries setting in the DestinationRule and request retries in the VirtualService. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, we should distinguish between the Regarding the shared cluster, it's an aspect that requires careful consideration. However, I'm currently uncertain about its usage. It might be an implementation similar to the Istio DestinationRule resource. If that's the case, one could patch the BackendTrafficPolicy to the DestinationRule (DR). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I vote -1 on this. Even though both have "retries" in their name, they serve two different purposes. The concurrent max retries setting is inherently associated with the Circuit Breaker, which fails requests quickly when a lot of retries happen and apply back pressure on downstream. On the other hand, request retries are specifically designed to mitigate transient network issues. Would love more insights from @kflynn and other @envoyproxy/gateway-maintainers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since @arkodg #2284 (comment) and @tmsnan support removing |
||
} | ||
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.