GEP 3388 Retry Budget API Design

[ericdbishop]

What type of PR is this?

/kind gep

What this PR does / why we need it:

Following up on #3488, where GEP 3388 was moved to provisional and the general goals and some potential designs for retry budgets within Gateway API were agreed upon. This PR will present multiple API implementations based off of previous discussion.

Which issue(s) this PR fixes:

Fixes #3388

Does this PR introduce a user-facing change?:

[k8s-ci-robot]

Hi @ericdbishop. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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-sigs/prow repository.

[mikemorris]

Planning to expand on the doc comments further during implementation phase, but put together an initial draft of possible Go struct and YAML representations.

The tricky part i tried to illustrate in YAML examples is the potential complexity of applicability to both N/S (ingress) and E/W (mesh) traffic, indicating the need for some sort of source discriminator (the from clause - I'm expecting we'll likely want a similar pattern eventually for AuthZ policies @youngnick @LiorLieberman), and how bundling everything into BackendTrafficPolicy might make various combinations too unwieldy to configure or status too difficult to parse.

Open questions/comments:

• Is there merit to including the HTTP retry codes/attempts/backoff fields in this policy? (Nested under an http stanza to allow an optional parallel grpc stanza in the future is desired)
• Need to add some language on allowed/default values:
  • I think explicitly setting budgetInterval and minRetryRate.interval to 0s should be the way to configure Envoy's existing behavior for tracking concurrent in-flight requests only rather than measuring over a period of time. If we choose an over-time default value for each of these (Linkerd's defaults seem sensible and well-tested), I think there's a clear path for implementing that functionality in Envoy.

@robscott @kflynn @howardjohn @rajatsharma94

[mikemorris]

/ok-to-test

[robscott]

@robscott I expect it should take at most a few days to get this sufficiently cleaned up - would like to request an extension until end of next week, Feburary 7th, 2025.

Thanks @mikemorris! I've talked with @mlavacca, @shaneutt, and @youngnick and we're approving this exception. Of course we would strongly prefer that this gets in sooner to leave time for API types, docs, conformance tests, etc. Let us know if there's anything we can help with!

[shaneutt]

Yes, just stay in touch! Feel free to reach out on Slack if you're feeling stuck 🫡

[tonya11en]

@mikemorris notes:

To clarify, I'm suggesting the unset/default behavior likely be a standard "count over time interval" (maybe 10s is the good default, which is not the stock Envoy behavior and will require what I expect will be a minor enhancement directly in Envoy upstream and configuration to set that in Envoy-based Gateway API implementations), and 0s is basically a "divide by zero" shorthand to express the existing Envoy behavior of "don't track requests over time, only active connections".

I kinda feel like only tracking active connections isn't actually implementing budgeted retries (it's honestly a little tough for me to come up with a "retry" semantic based on connections that even makes sense). I'd vote for disallowing "0s" and setting a minimum for the budget calculation window; if we want some funky connection-only mode, let's maybe call it something totally different.

@kflynn the Envoy retry budget doesn't care about connections. You're right to be confused by that. It only tracks concurrent requests and the ratio of those that are retries.

FWIW, I agree that modifying Envoy in the way folks here are suggesting (adding a time interval parameter) will work and be a fairly innocuous change. I said something similar in envoyproxy/envoy#30205 (comment). I can help with making that change in Envoy whenever you folks flesh out the specifics here.

[robscott]

@mikemorris @ericdbishop any updates on this one? Are you still trying to get this in to v1.3?

[mikemorris]

@mikemorris @ericdbishop any updates on this one? Are you still trying to get this in to v1.3?

Yes, I've just been a bit swamped with other tasks this week and I think @ericdbishop has been busy on-call. Opened ericdbishop#2 against Eric's branch with revisions in response to discussion in comments, after merging that I think @ericdbishop would just need to accept @kflynn's minor phrasing tweaks then we would be in good shape!

[ericdbishop]

@mikemorris @robscott Done, it's been a low-bandwidth week for me, apologies.

[mikemorris]

@robscott @kflynn @howardjohn I think this should be ready for final review now!

/cc @shaneutt @mlavacca @youngnick

[LiorLieberman]

Looks good to me, thank you both!
/lgtm

[robscott]

Thanks @ericdbishop and @mikemorris! A few follow up questions we can cover in the next phase, but otherwise LGTM.

/approve

[robscott]

Can save this for the follow up, but we'll need some more comprehensive validation here, at least a min and max

[robscott]

What's the minimum viable set of fields here for an implementation to say that they support retry budgets?

[robscott]

We'll likely need to add some guidance in the spec for how this overlaps with the other retry config already in the API.

[mikemorris]

Oh this might've just been an inadvertent copy-paste, was talking with @kflynn earlier about how we should add guidance on interaction with HTTPRoute retry clause - my initial thought is a retry budget is a constraint, but wouldn't on its own imply requests should be retried - thoughts?

[ericdbishop]

Agreed, as a retry budget implies an overall constraint, and additionally application developers will decide which routes should require retries based on idempotency of requests, etc.

[robscott]

Are both of the fields in here meant to be optional? Surely at least one of them needs to be set? Can they both be set at the same time?

[mikemorris]

Yes, both would likely be set at the same time. I'm not sure if there's an obvious enough default for either field on its own, so maybe both should be required, and what should actually be optional and could have a reasonable default is just the MinRetryRate field of this type in CommonRetryPolicy?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericdbishop, robscott

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:

• geps/OWNERS [robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[robscott]

/hold cancel