-
Notifications
You must be signed in to change notification settings - Fork 578
feat: support retries in ext proc #7169
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
Conversation
Signed-off-by: Shreemaan Abhishek <[email protected]>
7f3e989
to
3863a53
Compare
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (50.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7169 +/- ##
==========================================
- Coverage 71.06% 70.96% -0.11%
==========================================
Files 229 229
Lines 40942 41021 +79
==========================================
+ Hits 29097 29112 +15
- Misses 10127 10191 +64
Partials 1718 1718 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Shreemaan Abhishek <[email protected]>
@arkodg, test cases added! |
Group: gateway.envoyproxy.io | ||
backendSettings: | ||
# START: Test data for retry policy | ||
retry: |
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.
Test data for retry policy
proxyProtocol: | ||
version: V2 | ||
# START: Test data for retry policy | ||
retry: |
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.
Test data for retry policy
Signed-off-by: Shreemaan Abhishek <[email protected]>
hey @shreealt can you run |
Co-authored-by: zirain <[email protected]> Signed-off-by: shreealt <[email protected]>
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.
Do grpc services support all the options in retry policy? It seems like the original PR that added support for retry_policy in grpc_service only covered backoff settings: envoyproxy/envoy#24701. Are other options like retryOn supported?
Signed-off-by: Shreemaan Abhishek <[email protected]>
IIUC Envoy has a separate gRPC client codepath for xDS config subscription vs async sidestream calls (e.g. from ext_auth and ext_proc). envoyproxy/envoy#24701 implements it for the xDS config subscription code path. I believe the regular async sidestream call client has supported Here, we see the gRPC client sets retry policy directly on the HTTP stream options - https://github.com/envoyproxy/envoy/blob/f18433b0e76190cece6cad73fd58d4fbd9284a0d/source/common/grpc/async_client_impl.cc#L130 So yes, it should be supported fully. But please double check me :) |
/retest |
1 similar comment
/retest |
tests are still failing |
Signed-off-by: Shreemaan Abhishek <[email protected]>
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.
LGTM, do we need a release notes for backport?
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.
LGTM!
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.
LGTM thanks !
merging this one to avoid merge conflicts, lets add a release note for this in a follow up
What type of PR is this?
Support retry policy in ext proc.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes: #7127
Release Notes: Yes/No