Conversation
Signed-off-by: Alex Leong <alex@buoyant.io>
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
overall, this looks right to me --- i left some relatively minor suggestions.
proto/http_route.proto
Outdated
| // A range of HTTP response status codes, inclusive. | ||
| message StatusRange { | ||
| uint32 min = 1; | ||
| uint32 max = 2; | ||
| } |
There was a problem hiding this comment.
destination.proto already has an HttpStatusRange message:
linkerd2-proxy-api/proto/destination.proto
Lines 272 to 279 in 5b948f9
would it make sense to use that here as well, rather than declaring a new message type?
There was a problem hiding this comment.
I thought about this, but I'm worried about backwards compatibility. Since the types are structurally identical, I think it should be fine, but it gave me the heebie-jeebies so I left it alone.
There was a problem hiding this comment.
Oh, on second reading I guess you're suggesting using the type defined in destionation.proto in the outbound policy. I think as a matter of hygiene the plan is to eventually deprecate the getProfile API so depending on its types feels weird.
There was a problem hiding this comment.
it seems like ideally there could be one type in http_types.proto, but the ship has sailed on that. 🤷♀️
proto/outbound.proto
Outdated
| Breaker breaker = 2; | ||
| } | ||
|
|
||
| message Http2 { | ||
| repeated HttpRoute routes = 1; | ||
| Breaker breaker = 2; | ||
| } | ||
|
|
||
| message Grpc { | ||
| repeated GrpcRoute routes = 1; | ||
| Breaker breaker = 2; |
There was a problem hiding this comment.
in the proxy, this is currently referred to as a "failure accrual policy" although this naming is certainly open to change...
There was a problem hiding this comment.
also, should we maybe document that this field is allowed to be empty and that this indicates that no circuit breaking should be performed?
There was a problem hiding this comment.
I'm open to renaming this but failure_accrual_policy feels a bit verbose and overly prescriptive. I could imagine other circuit breaker policies which trigger on conditions other than accrued failures (e.g. windowed success rate, latency, or other signals). The term breaker felt concise and generic.
There was a problem hiding this comment.
i would consider a windowed success rate a form of failure accrual, but 🤷♀️. not a strongly held opinion --- maybe the proxy naming should change, instead.
There was a problem hiding this comment.
I think it's really important that we be crisp with our terms. If these are breakers then we should stop referring to failure accrual everywhere else. Or the other way around.
In my view, circuit breakers are an emergent behavior that build off of a failure accrual configuration. I could go deeper on the distinction but for the sake of brevity, I think it's best to call this FailureAccrual
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
mateiidavid
left a comment
mateiidavid
left a comment
There was a problem hiding this comment.
This looks great to me. I know we're still debating the terminology, but I'll approve nonetheless so the change is not blocked.
Signed-off-by: Alex Leong <alex@buoyant.io>
|
|
||
| message Http1 { | ||
| repeated HttpRoute routes = 1; | ||
| // If empty, circuit breaking is not performed. |
There was a problem hiding this comment.
Session-level circuit breaking is still performed, fwiw.
This branch updates the proxy to configure failure accrual policies from the OutboundPolicy API. It updates the dependency on `linkerd2-proxy-api` to include changes from linkerd/linkerd2-proxy-api#223, which adds failure accrual configuration to the `Protocol::Http1`, `Protocol::Http2`, and `Protocol::Grpc` variants. Currently, `ConsecutiveFailures` (added in #2357) is the only supported failure accrual policy. If the proxy API does not configure a failure accrual policy, failures are not accrued, and endpoints do not become unavailable due to failure accrual.
4 participants
Add support for consecutive failure based circuit breaking to the outbound policy API.