api: support retry on in BackendTrafficPolicy #2168

tmsnan · 2023-11-09T07:04:00Z

api: support retry on in BackendTrafficPolicy

Relates to #1821 & #1907

codecov · 2023-11-09T09:05:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (20b8497) 64.71% compared to head (d3057c4) 64.71%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2168   +/-   ##
=======================================
  Coverage   64.71%   64.71%           
=======================================
  Files         115      115           
  Lines       17431    17431           
=======================================
  Hits        11281    11281           
  Misses       5433     5433           
  Partials      717      717

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arkodg · 2023-11-10T18:58:16Z

@kflynn and @AliceProxy, can you help review this one

kflynn · 2023-11-14T15:31:36Z

@arkodg @tmsnan Sorry for the delay here! and many thanks for digging into this, I appreciate that. 🙂

That said, I'd like to formalize something I've said on many other occasions:

Flynn's First Law of Envoy: Having end users interact directly with Envoy configuration concepts is almost always a huge mistake. 🙂

In Linkerd, the basic retry configuration is one line: isRetryable: true. You can go beyond that, but you don't have to. In Emissary, it's a little more complex than that, but it's still only... three lines, I think? The fact that Envoy has a ton of options is nearly always irrelevant to the user; let's not force them to use configuration structures that look anything like Envoy.

My strong preference here is that the base case should be a single line of configuration: to get sane retries with good defaults, the user should need only to add isRetryable: true, or something very similar, at the top level of the BackendTrafficPolicy. Having a separate stanza for configuring policy is entirely reasonable but it should be optional.

It's certainly possible that I missed something here, and if so I apologize! 🙂 But in Envoy Gateway's name, the "Gateway" word is the really important bit, right? 🙂

tmsnan · 2023-11-16T08:18:50Z

@arkodg @tmsnan Sorry for the delay here! and many thanks for digging into this, I appreciate that. 🙂

That said, I'd like to formalize something I've said on many other occasions:

Flynn's First Law of Envoy: Having end users interact directly with Envoy configuration concepts is almost always a huge mistake. 🙂

In Linkerd, the basic retry configuration is one line: isRetryable: true. You can go beyond that, but you don't have to. In Emissary, it's a little more complex than that, but it's still only... three lines, I think? The fact that Envoy has a ton of options is nearly always irrelevant to the user; let's not force them to use configuration structures that look anything like Envoy.

My strong preference here is that the base case should be a single line of configuration: to get sane retries with good defaults, the user should need only to add isRetryable: true, or something very similar, at the top level of the BackendTrafficPolicy. Having a separate stanza for configuring policy is entirely reasonable but it should be optional.

It's certainly possible that I missed something here, and if so I apologize! 🙂 But in Envoy Gateway's name, the "Gateway" word is the really important bit, right? 🙂

IMO, the RetryStrategy in BackendTrafficPolicy offers a more comprehensive range of options. Users can customize the retry backoff strategy, retry budget strategy, and retry triggering conditions. However, these advanced features may be too complex for typical Gateway(Not envoy) users. We need to consider whether to fully retain these higher-order uses.
It is indeed true that most users do not customize the configuration and prefer to use the recommended settings provided by the gateway to enable the retry feature. I agree with this. Perhaps we could also keep the above-mentioned API and add a toggle switch to enable the commonly used retry function. @arkodg @kflynn

kflynn · 2023-12-07T20:19:52Z

@tmsnan, so sorry for the delay (again)!

In practice, we haven't really found that everything needs support, and many things just don't get used all that often. Here's an alternative that deliberately tries to minimize the amount of indentation and configuration needed for the things I think are very common:

Suggestion

---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
  name: example-policy
spec:
  ...
  isRetryable: true
  retryStrategy:
    maxBudget: 20    # percentage, default 20%
    maxRetries: 0    # count, if nonzero, maxBudget is ignored
    maxParallel: 3   # Optional
    minConcurrent: 3 # Optional

    retryOn:
      trigger: trigger-enum (see below)
      httpStatus: []int # valid when trigger is "retriable-status-code"

    perRetry:
      timeout: duration # optional
      idleTimeout: duration # optional
      backoff: # optional
        baseInterval: duration # required
        maxInterval: duration # optional
        # we can add rate limited based backoff config here if we want to

trigger-enum: one of
  5xx                       # HTTP events
  gateway-error             
  disconnect-reset
  connect-fail
  retriable-4xx
  refused-stream
  retriable-status-codes

  cancelled                 # GRPC events
  deadline-exceeded
  internal
  resource-exhausted
  unavailable

The idea here is that the proxy should know whether it's speaking HTTP or gRPC to a given backend. There's no point in configuring HTTP retries for a gRPC backend, and minimal config shouldn't require rewriting the retry config if you change types. (Obviously if you change protocols when you have configured a trigger, you'd need to change that.)

Examples

These are not meant to be exhaustive; they're meant to show specific cases that might come up.

Minimal config

---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
  name: example-policy
spec:
  isRetryable: true

Just give me retries and don't make me think about it. This will enable budgeted retries with a 20% retry budget. (Why budgeted and not counted? because the user experience is better for really simple configurations.)

Minimal counted retries

---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
  name: example-policy
spec:
  isRetryable: true
  retryStrategy:
    maxRetries: 1

Retry, but only once. (It might be worth considering whether the presence of retryStrategy implies isRetryable: true...)

Retry only on 499 or 505 (I don't recommend this)

---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
  name: example-policy
spec:
  isRetryable: true
  retryStrategy:
    retryOn:
      trigger: retriable-status-codes
      httpStatus: [ 499, 505 ]

Budgeted retries, but only retry on 499 or 505 status codes.

Retry, controlling timing, on gRPC `cancelled`.

---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
  name: example-policy
spec:
  isRetryable: true
  retryStrategy:
    retryOn:
      trigger: cancelled
    perRetry:
      timeout: 250ms
      idleTimeout: 5s
      backoff:
        baseInterval: 100ms
        maxInterval: 10s

Summary

This is the kind of thing I mean when I say that we don't need to expose all the Envoy config. Envoy's API almost always considers every possible configuration as equal weight, as well as allowing a certain amount of configuration that's kind of nonsensical -- in practice people do not mix HTTP and gRPC to the same endpoint, for example, so configuring both at the same time just doesn't make much sense. We can - and should - be more opinionated, so that the common things are easy.

In, of course, my opinion. 🙂

tmsnan · 2023-12-11T09:39:37Z

@kflynn, thanks, the API looks much simpler, and there's really no need to distinguish between HTTP and gRPC retry trigger conditions. It's a minor point that we might need to configure multiple conditions in the trigger simultaneously.

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
  name: example-policy
spec:
  isRetryable: true
  retryStrategy:
    retryOn:
      trigger:  gateway-error, retriable-4xx

I will update the API later.

github-actions · 2023-12-15T03:15:41Z

🚀 Deployed on https://gateway-pr-2168-preview--eg-preview.netlify.app

arkodg · 2024-01-10T19:18:10Z

api/v1alpha1/backendtrafficpolicy_types.go

+
+	// IsRetryAble indicates whether to enable budget retries with a 20% retry budget.
+	// +optional
+	IsRetryAble bool `json:"isRetryAble,omitempty"`


I would prefer we didn't include that and instead relied only on Retry

arkodg · 2024-01-10T19:18:49Z

api/v1alpha1/backendtrafficpolicy_types.go

+	// RetryStrategy provides more advanced retry usage, which allows users to customize the retry method (number of retries、retry budget and concurrency max retries),
+	// retry fallback strategy, and retry triggering conditions
+	// +optional
+	RetryStrategy *RetryStrategy `json:"retryStrategy,omitempty"`


can we rename to retry ? all fields in here are singular

arkodg · 2024-01-11T00:28:51Z

api/v1alpha1/retrystragegy_types.go

+	// NumRetries is the number of retries to be attempted. Defaults to 0. If nonzero, maxBudget is ignored.
+	//
+	// +optional
+	MaxRetries *int `json:"maxRetries,omitempty"`


is this numRetries or maxRetries ?

arkodg · 2024-01-11T00:35:43Z

api/v1alpha1/retrystragegy_types.go

+
+	// RetryOn specifies the retry trigger condition.
+	//
+	// +optional


lets add some defaults ?

like istio?

retryOn: trigger: retriable-status-codes，connection-failure, refused-stream, unavailable,cancelled httpStatus: [ 503 ]

yeah as well as numRetries

Signed-off-by: nan zhao <[email protected]>

Signed-off-by: zhaonan <[email protected]>

Signed-off-by: nan zhao <[email protected]>

arkodg · 2024-01-18T23:30:33Z

hey @tmsnan, sorry for the delay in review

can we rm MaxBudget , MaxParallel & MinConcurrent in this first iteration
can we decide on sane defaults, so when a user sets

retry: {}

it results in

NumRetries - X
RetryOn.Triggers - [......]

Signed-off-by: nan zhao <[email protected]>

tmsnan · 2024-01-19T09:27:39Z

hey @tmsnan, sorry for the delay in review

can we rm MaxBudget , MaxParallel & MinConcurrent in this first iteration

can we decide on sane defaults, so when a user sets
retry: {}
it results in

NumRetries - X

RetryOn.Triggers - [......]

done

arkodg · 2024-01-22T20:28:46Z

api/v1alpha1/retry_types.go

+}
+
+// TriggerEnum specifies the conditions that trigger retries.
+type TriggerEnum string


needs a validation annotation

arkodg · 2024-01-22T20:29:48Z

api/v1alpha1/retry_types.go

+	//
+	// +optional
+	// +kubebuilder:validation:Format=duration
+	IdleTimeout *metav1.Duration `json:"idleTimeout,omitempty"`


can we also rm IdleTimeout in the first iteration ?

arkodg · 2024-02-13T00:47:31Z

hey @tmsnan, this API is pretty close to the finish line, can you address the review comments ? tia

guydc · 2024-02-16T22:37:16Z

api/v1alpha1/retry_types.go

+	//
+	// +optional
+	// +kubebuilder:default=2
+	NumRetries *int `json:"numRetries,omitempty"`


maybe int32 is more explicit. Also, we can validate that the value is >= 0.

guydc · 2024-02-16T22:37:19Z

api/v1alpha1/retry_types.go

+	// HttpStatusCodes specifies the http status codes to be retried.
+	//
+	// +optional
+	HTTPStatusCodes []int `json:"httpStatusCodes,omitempty"`


maybe validate status in accepted range 1xx - 5xx

tmsnan · 2024-02-18T03:22:43Z

hey @tmsnan, this API is pretty close to the finish line, can you address the review comments ? tia

Sorry for the late reply, there have been some family matters recently, and I will handle them soon.

arkodg · 2024-02-19T16:25:23Z

hey @tmsnan, this API is pretty close to the finish line, can you address the review comments ? tia

Sorry for the late reply, there have been some family matters recently, and I will handle them soon.

Sorry to hear @tmsnan.
No rush to pick up this PR, @guydc can help move this PR forward

guydc · 2024-02-23T01:11:10Z

Closing as this work was merged in #2657

tmsnan requested a review from a team as a code owner November 9, 2023 07:04

tmsnan marked this pull request as draft November 9, 2023 07:12

tmsnan marked this pull request as ready for review November 9, 2023 09:19

Xunzhuo added the road-to-ga label Dec 6, 2023

Xunzhuo added this to the v1.0.0-rc1 milestone Dec 6, 2023

guydc mentioned this pull request Dec 9, 2023

API: Support Circuit Breakers in BackendTrafficPolicy #2284

Merged

tmsnan force-pushed the dev-support-retry-on branch from 01c7a9e to 24e2f66 Compare December 15, 2023 03:12

github-actions bot temporarily deployed to pull request December 15, 2023 03:15 Inactive

github-actions bot temporarily deployed to pull request December 15, 2023 06:44 Inactive

zhaohuabing mentioned this pull request Dec 19, 2023

Find the right home for maximum number of parallel retries #2322

Closed

arkodg reviewed Jan 10, 2024

View reviewed changes

arkodg reviewed Jan 11, 2024

View reviewed changes

tmsnan and others added 3 commits January 11, 2024 10:43

fix conflict

43532d4

Signed-off-by: nan zhao <[email protected]>

retry:fix lint

4a227f6

Signed-off-by: zhaonan <[email protected]>

fix retry on

1a24ca4

Signed-off-by: nan zhao <[email protected]>

tmsnan force-pushed the dev-support-retry-on branch from 3c9b667 to 1a24ca4 Compare January 11, 2024 03:02

tmsnan added 2 commits January 19, 2024 10:59

fix retry on api

da1f4fb

Signed-off-by: nan zhao <[email protected]>

merge main

d3057c4

Signed-off-by: nan zhao <[email protected]>

arkodg reviewed Jan 22, 2024

View reviewed changes

guydc reviewed Feb 16, 2024

View reviewed changes

guydc mentioned this pull request Feb 20, 2024

api: support retry on in BackendTrafficPolicy #2657

Merged

guydc closed this Feb 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: support retry on in BackendTrafficPolicy #2168

api: support retry on in BackendTrafficPolicy #2168

tmsnan commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 9, 2023 •

edited

Loading

arkodg commented Nov 10, 2023

kflynn commented Nov 14, 2023

tmsnan commented Nov 16, 2023 •

edited

Loading

kflynn commented Dec 7, 2023

tmsnan commented Dec 11, 2023 •

edited

Loading

github-actions bot commented Dec 15, 2023 •

edited

Loading

arkodg Jan 10, 2024

arkodg Jan 10, 2024

arkodg Jan 11, 2024

arkodg Jan 11, 2024

tmsnan Jan 11, 2024

arkodg Jan 18, 2024

arkodg commented Jan 18, 2024

tmsnan commented Jan 19, 2024

arkodg Jan 22, 2024

arkodg Jan 22, 2024

arkodg commented Feb 13, 2024

guydc Feb 16, 2024

guydc Feb 16, 2024

tmsnan commented Feb 18, 2024

arkodg commented Feb 19, 2024

guydc commented Feb 23, 2024

api: support retry on in BackendTrafficPolicy #2168

api: support retry on in BackendTrafficPolicy #2168

Conversation

tmsnan commented Nov 9, 2023 • edited Loading

codecov bot commented Nov 9, 2023 • edited Loading

Codecov Report

arkodg commented Nov 10, 2023

kflynn commented Nov 14, 2023

tmsnan commented Nov 16, 2023 • edited Loading

kflynn commented Dec 7, 2023

Suggestion

Examples

Minimal config

Minimal counted retries

Retry only on 499 or 505 (I don't recommend this)

Retry, controlling timing, on gRPC cancelled.

Summary

tmsnan commented Dec 11, 2023 • edited Loading

github-actions bot commented Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg commented Jan 18, 2024

tmsnan commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmsnan commented Feb 18, 2024

arkodg commented Feb 19, 2024

guydc commented Feb 23, 2024

tmsnan commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 9, 2023 •

edited

Loading

tmsnan commented Nov 16, 2023 •

edited

Loading

Retry, controlling timing, on gRPC `cancelled`.

tmsnan commented Dec 11, 2023 •

edited

Loading

github-actions bot commented Dec 15, 2023 •

edited

Loading