orca: add out-of-band reporting for client-side WRR - #44403
Conversation
d940d58 to
5f1690d
Compare
|
/retest |
|
Thanks for starting the work on this /assign @efimki |
|
Tagging @markdroth to comment from gRPC perspective. Reading https://github.com/grpc/proposal/blob/master/A51-custom-backend-metrics.md#out-of-band-reporting-1 it seems like in gRPC case server is pushing OOB load reports to connected clients. |
|
From https://grpc.io/docs/guides/custom-backend-metrics/: We're still matching the model where an upstream server pushes reports to connected clients (Envoy here) but a client needs to initiate the connection and communicate |
|
kindly ping @efimki @adisuissa |
|
I'll rework this and discuss the design a bit more in the issue |
00e398a to
939210a
Compare
|
Reworked the implementation here based on feedback |
|
/coverage |
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-cncf-pr/44403/coverage/index.html For comparison, current coverage on https://storage.googleapis.com/envoy-cncf-postsubmit/main/coverage/index.html The coverage results are (re-)rendered each time the CI |
|
/retest |
|
Summary of the updated implementation:
|
|
/retest |
|
Hey @paul-r-gall could you take another look at this? |
Signed-off-by: jukie <10012479+jukie@users.noreply.github.com>
|
Requesting review from @yanavlasov or @botengyao for senior maintainer. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements out-of-band (OOB) ORCA load reporting for the client_side_weighted_round_robin load balancing policy, introducing an OrcaOobManager to handle per-host gRPC streams. The implementation includes new connection creation logic in the Host interface and LogicalHost. Feedback points out that the authority() method's fallback to a string representation for certain address types, such as Unix Domain Sockets, could potentially lead to invalid characters in the HTTP :authority header.
…ing for non-IP hosts Signed-off-by: jukie <10012479+jukie@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements out-of-band (OOB) ORCA load reporting for the client-side weighted round-robin load balancing policy using server-streaming gRPC. It introduces the OrcaOobManager to manage per-host gRPC sessions and updates upstream host abstractions to support dedicated reporting connections. The review feedback highlights an issue with the :authority header formatting for IPv6 addresses, suggesting the use of asString() to ensure compliance with RFC 3986 by including brackets and the port number.
…ger.cc Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Isaac Wilson <isaac.wilson514@gmail.com>
|
/assign @botengyao |
botengyao
left a comment
There was a problem hiding this comment.
sorry for the delay, and lgtm module one q.
|
|
||
| void OrcaOobManager::OobSession::decodeData(Buffer::Instance& data, bool end_stream) { | ||
| std::vector<Grpc::Frame> frames; | ||
| if (!decoder_.decode(data, frames).ok()) { |
There was a problem hiding this comment.
Consider setting a max gRPC frame/message size? ORCA reports should be small, but this currently lets every upstream OOB stream buffer arbitrarily large frames, can be a follow-up.
There was a problem hiding this comment.
@botengyao updated. Please let me know if you have a different value in mind but I think 64KiB should be high enough. We can make this configurable once the per-host/cluster config surface is added for overrides.
There was a problem hiding this comment.
sgtm, thanks for adding this.
Signed-off-by: jukie <10012479+jukie@users.noreply.github.com>
Signed-off-by: jukie <10012479+jukie@users.noreply.github.com>
Signed-off-by: jukie <10012479+jukie@users.noreply.github.com>
Commit Message: orca: add out-of-band reporting for client-side WRR Additional Description: Adds ORCA out-of-band load reporting support for `client_side_weighted_round_robin`. When `enable_oob_load_report` is true, Envoy opens a per-host HTTP/2 gRPC stream to `xds.service.orca.v3.OpenRcaService/StreamCoreMetrics`, decodes server-streamed `OrcaLoadReport`s, and feeds them into the existing ORCA weight manager used by in-band reports. This also adds a dedicated host connection path for ORCA reporting, including LogicalHost address snapshotting, and cluster-scoped `lb_orca_oob.*` stats for reporting/session state. Risk Level: Low Testing: Added unit and integration coverage for: - ORCA OOB manager lifecycle, report decoding, transient failures, GOAWAY handling, and terminal `UNIMPLEMENTED` - host/logical-host ORCA reporting connection behavior - CSWRR OOB config defaults/overrides - basic OOB integration stats/report flow Docs Changes: N/A Release Notes: Added changelog entry for `client_side_weighted_round_robin` ORCA out-of-band reporting Platform Specific Features: N/A xRef envoyproxy#44318 xRef envoyproxy#44329 AI Disclosure: AI was used during implementation --------- Signed-off-by: jukie <10012479+jukie@users.noreply.github.com> Signed-off-by: Isaac Wilson <isaac.wilson514@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Alireza <alrzazz98@gmail.com>

Commit Message: orca: add out-of-band reporting for client-side WRR
Additional Description: Adds ORCA out-of-band load reporting support for
client_side_weighted_round_robin. Whenenable_oob_load_reportis true, Envoy opens a per-host HTTP/2 gRPC stream toxds.service.orca.v3.OpenRcaService/StreamCoreMetrics, decodes server-streamedOrcaLoadReports, and feeds them into the existing ORCA weight manager used by in-band reports. This also adds a dedicated host connection path for ORCA reporting, including LogicalHost address snapshotting, and cluster-scopedlb_orca_oob.*stats for reporting/session state.Risk Level: Low
Testing: Added unit and integration coverage for:
UNIMPLEMENTEDDocs Changes: N/A
Release Notes: Added changelog entry for
client_side_weighted_round_robinORCA out-of-band reportingPlatform Specific Features: N/A
xRef #44318
xRef #44329
AI Disclosure: AI was used during implementation