Skip to content

Introduce virtual host snapshots #6105

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

Merged
merged 3 commits into from
Mar 27, 2025
Merged

Conversation

jrhee17
Copy link
Contributor

@jrhee17 jrhee17 commented Feb 18, 2025

Motivation:

We are planning on deprecating XdsEndpointGroup and introducing XdsPreprocessor at #6096.

Previously, XdsEndpointGroup created sub-EndpointGroups which consume resources. (e.g. HealthCheckedEndpointGroup, DnsEndpointGroup, etc..) assuming that all xDS clients will share a common EndpointGroup.

However, XdsPreprocessor cannot be shared among all client types.
For instance, thrift clients will need to use RpcPreprocessor whereas web clients will need to use normal Preprocessors.

I propose that XdsBootstrap contains a single ClusterManager which caches all EndpointGroups derived from Clusters. This ClusterManager is aligned with the xDS API cluster_manager and is aligned with the upstream implementation as well.

One prerequisite for the introduction of ClusterManager is the ability to cache Clusters. This is difficult since the current implementation of ClusterSnapshot also contains a reference to the containing Route.

* The {@link Route} this {@link Cluster} belongs to.

I propose that VirtualHostSnapshot and RouteEntry is introduced, and snapshot mappings are unidirectional. This direction aligns with a possible future implementation of VHDS as well.

As this is a refactor of the previous implementation, there is no expected change in behavior except for subset load balancing (which actually matches upstream now)

Modifications:

  • Introduced VirtualHostSnapshot which is equivalent to VirtualHost, and RouteEntry which is equivalent to Route.
  • Refactored ClusterManager and SubsetLoadBalancer to not use ClusterSnapshot#route
    • SubsetLoadBalancer now is aligned with the upstream implementation (ref)
    • The previous test case for skipping failure on empty metadata is removed as it does not match the upstream implementation.
  • (Misc) Cleaned up the ConfigSourceMapper implementation by removing unused fields and the corresponding logic.

Result:

  • The ClusterSnapshot API is prepared for the introduction of a central ClusterManager maintained by XdsBootstrapImpl

@jrhee17 jrhee17 added this to the 1.33.0 milestone Feb 18, 2025
@jrhee17 jrhee17 marked this pull request as ready for review February 18, 2025 02:25
@github-actions github-actions bot added the Stale label Mar 22, 2025
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍


@Override
void doOnChanged(VirtualHostXdsResource resource) {
pending.clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In RouteResourceNode, we call virtualHostSnapshots.clear(); where as we call pending.clear(); here. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, updated both implementations to just clear both data structures

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nits. Looks good!

@github-actions github-actions bot removed the Stale label Mar 26, 2025
@jrhee17
Copy link
Contributor Author

jrhee17 commented Mar 27, 2025

Let me go ahead and merge this PR, thanks for the review all

@jrhee17 jrhee17 merged commit be16610 into line:main Mar 27, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants