gep: add GEP-1731 Configurable Retries #3199
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
size/L
mikemorris
changed the title
mikemorris
force-pushed
the
gep-1731-retry
branch
from
10a32e4
to
63f5f63
Compare
howardjohn
reviewed
Jul 16, 2024
k8s-ci-robot
added
size/XL
mikemorris
commented
Jul 22, 2024
mikemorris
commented
Jul 22, 2024
mikemorris
commented
Jul 23, 2024
mikemorris
commented
Jul 23, 2024
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
youngnick
reviewed
Jul 23, 2024
k8s-ci-robot
added
the
approved
sjberman
reviewed
Jul 23, 2024
robscott
reviewed
Jul 26, 2024
There was a problem hiding this comment.
Nicely done @mikemorris! Very thorough and well written GEP. Seems like the most complex part will be having some portability around what status codes can be retried, left some comments/ideas around that.
k8s-ci-robot
added
the
needs-rebase
robscott
reviewed
Jul 30, 2024
mikemorris
force-pushed
the
gep-1731-retry
branch
from
b80bb50
to
2367d35
Compare
k8s-ci-robot
added
size/L
kflynn
reviewed
Aug 1, 2024
|
@kate-osborn Do you have a suggestion for the language you would want around behavior with singleton backends? |
kflynn
reviewed
Aug 1, 2024
Co-authored-by: Flynn <kflynn@users.noreply.github.com>
Co-authored-by: Flynn <kflynn@users.noreply.github.com>
- change connection error and backend timeout retry from MUST to SHOULD - add conformance flags for connection errors and backend timeouts - add UNRESOLVED warning
mikemorris
commented
Aug 2, 2024
|
Status Update: I believe we should merge this GEP for v1.2. Despite this being just past the extension for the GEP deadline, this one was largely good to go yesterday with only formatting bits to cleanup. We've had multiple review cycles with a variety of reviewers, and the resulting API change is fairly safe and contained. I'll defer to @shaneutt or @mlavacca for the final call here. Thanks to @mikemorris for all the work on this one! /approve |
mlavacca
approved these changes
Aug 2, 2024
There was a problem hiding this comment.
Thank you for all the work on this, @mikemorris!
/lgtm
/unhold
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikemorris, mlavacca, robscott, youngnick The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
xtineskim
pushed a commit
to xtineskim/gateway-api
that referenced
this pull request
Aug 8, 2024
* gep: add GEP-1731 Configurable Retries * minor updates to NGINX summary * add clarification around retry on connection errors * fixup details formatting for implementation background * add Linkerd retry summary * Update geps/gep-1731/index.md * Update geps/gep-1731/index.md * website: add GEP-1731 to mkdocs.yml index * update status code rules for retries * update conformance details for 5xx * add note on streams out of scope as non-goal * add note on mesh consumer vs product routes * add introduction * Fixup YAML to nest retry stanza under HTTPRouteRule, not BackendRefs * add note on HTTPRouteFilter alternative * remove allowance for [1-9]xx range values other than 5xx * Update geps/gep-1731/index.md Co-authored-by: Kate Osborn <50597707+kate-osborn@users.noreply.github.com> * Update geps/gep-1731/index.md Co-authored-by: Kate Osborn <50597707+kate-osborn@users.noreply.github.com> * update GEP-1731 metadata * rename GEP-1731 * add SupportHTTPRouteRetryBackoff conformance details * split out Other Considerations section, update retry budget future-proofing question * fixup nested list formatting in conformance details * add future excludeRetryOnTimeout consideration * move gRPC retries to non-goals, remove Envoy gRPC retry details * clarify streaming support non-goal as needing a separate GEP * update Linkerd retry background section Co-authored-by: Flynn <kflynn@users.noreply.github.com> * update streaming language in non-goals Co-authored-by: Flynn <kflynn@users.noreply.github.com> * update connection error and timeout, add warning - change connection error and backend timeout retry from MUST to SHOULD - add conformance flags for connection errors and backend timeouts - add UNRESOLVED warning * remove 5xx shorthand, switch HTTPRouteStatusCode to int * Update geps/gep-1731/index.md --------- Co-authored-by: Mike Morris <1149913+mikemorris@users.noreply.github.com> Co-authored-by: Kate Osborn <50597707+kate-osborn@users.noreply.github.com> Co-authored-by: Flynn <kflynn@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind gep
What this PR does / why we need it:
Proposes configuration within HTTPRoute to retry unsuccessful requests to backends before sending a response to a client.
Which issue(s) this PR fixes:
Fixes #1731
Does this PR introduce a user-facing change?: