Skip to content

tonic/xds: wire up lbv2 as a feature - #2722

Open
gu0keno0 wants to merge 15 commits into
grpc:masterfrom
gu0keno0:lbv2-wiring
Open

tonic/xds: wire up lbv2 as a feature#2722
gu0keno0 wants to merge 15 commits into
grpc:masterfrom
gu0keno0:lbv2-wiring

Conversation

@gu0keno0

@gu0keno0 gu0keno0 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Add tonic-xds' own LB module as a feature

Motivation

Tower's LB is limited. We cannot support advanced xds features such as customized load balancer and outlier detection on top of it.

We have been developing tonic-xds' own LB for a while, and this PR make it ready for future consumptions.
We will still need to wire up xds configs for OD and RingHash, those can be done in future PRs.

Solution

Added the wire-up modules to make the tonic-xds' LB usable behind a feature flag.

Comment thread tonic-xds/src/client/loadbalance/service.rs
@gu0keno0 gu0keno0 changed the title [wip]Lbv2 wiring tonic/xds: wire up lbv2 as a feature Jul 21, 2026
@gu0keno0
gu0keno0 marked this pull request as ready for review July 21, 2026 22:34
@gu0keno0
gu0keno0 requested a review from YutaoMa July 21, 2026 22:34
@YutaoMa

YutaoMa commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Found one potential issue with the new LB's readiness logic that needs verification:

  1. LoadBalancer::poll_ready treats non-empty self.ready as sufficient readiness check, but when a cluster is dropped it doesn't evict from self.ready, leaving them stale forever. If those endpoints are down call will fail forever and even if new CDS config comes in using the same config name, because the code will reuse the existing registry entry, it would continue to reuse the broken set of self.ready instead of rebuild one.
  2. The old tower::Balance path is less problematic because it re-polls readiness on every call and evict broken endpoints.

I think to fix it we either need to do eviction in call and/or rebuild entry in ClusterLbRegistry.

@gu0keno0

Copy link
Copy Markdown
Contributor Author

Found one potential issue with the new LB's readiness logic that needs verification:

  1. LoadBalancer::poll_ready treats non-empty self.ready as sufficient readiness check, but when a cluster is dropped it doesn't evict from self.ready, leaving them stale forever. If those endpoints are down call will fail forever and even if new CDS config comes in using the same config name, because the code will reuse the existing registry entry, it would continue to reuse the broken set of self.ready instead of rebuild one.
  2. The old tower::Balance path is less problematic because it re-polls readiness on every call and evict broken endpoints.

I think to fix it we either need to do eviction in call and/or rebuild entry in ClusterLbRegistry.

poll_ready calls poll_discovery, so IMO it is poll_discovery's responsibility to handle cluster removal / adding back. Currently this is not working because cluster discovery is not re-established. I have a TODO here: https://github.com/gu0keno0/tonic/blob/39cda4e88d2e532581c3b9749c7150bb726c513f/tonic-xds/src/xds/lb_discovery.rs#L17-L19 .

This is also an issue for tower-lb as well: https://github.com/grpc/grpc-rust/blob/master/tonic-xds/src/xds/cluster_discovery.rs#L179 . If cluster_watch is closed (I think cluster removal triggers it today), cluster discovery dies.

So both tower-lb and lbv2 are under the same issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants