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.

Sign up for GitHub

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

Jump to bottom

Add new classes for eliminating xds config tears #11740

Open

larry-safran wants to merge 24 commits into grpc:master from larry-safran:XdsDependencyManager

Contributor

larry-safran commented Dec 11, 2024 •

edited

Loading

This is just defining the XdsDependencyManager and associated classes; no movement of any functionality.

larry-safran requested a review from ejona86

December 11, 2024 02:56

larry-safran marked this pull request as draft

December 11, 2024 02:56

ejona86 reviewed

View reviewed changes

Member

ejona86 left a comment

The shape looks fine.

xds/src/main/java/io/grpc/xds/XdsClusterSubscriptionRegistry.java Outdated Show resolved Hide resolved

larry-safran force-pushed the XdsDependencyManager branch from 32c80c0 to f995211 Compare

December 14, 2024 00:50

larry-safran force-pushed the XdsDependencyManager branch 2 times, most recently from f5090ab to 9fcbe4c Compare

January 6, 2025 22:13

linux-foundation-easycla bot commented Jan 7, 2025 •

edited

Loading

The committers listed above are authorized under a signed CLA.

✅ login: larry-safran / name: Larry Safran (4a53fce, fd64f20, 06466fc, 954ced3, 5a75b10, b898e34, 5f8d479, 28d29fb, d3b713f, d5dea83, 8e668b5, b2cb05b, 5b14a3e, b2e924e, 5dceeaf, f99fc56, fd069e3, ec50490, 68071e6, 6089730, c97118f, ef13712, 518cef1, 482cd9d)

larry-safran marked this pull request as ready for review

January 7, 2025 05:35

larry-safran requested a review from ejona86

January 7, 2025 05:35

Contributor Author

larry-safran commented Jan 7, 2025

Ready for review @ejona86

ejona86 reviewed

View reviewed changes

Member

ejona86 left a comment

I still have a good amount to review. Sending what I have.

xds/src/main/java/io/grpc/xds/XdsConfig.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsConfig.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsConfig.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsConfig.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

ejona86 reviewed

View reviewed changes

Member

ejona86 left a comment

Nothing really new here; just a quick drive-by. Still need to look through the other parts of the PR.

xds/src/main/java/io/grpc/xds/XdsConfig.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsConfig.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsConfig.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

larry-safran force-pushed the XdsDependencyManager branch from 1fac607 to 3f59a16 Compare

January 8, 2025 19:41

larry-safran requested a review from ejona86

January 14, 2025 00:47

larry-safran added 13 commits

January 13, 2025 17:35


          Framework definition to support A74

fd069e3


          Test that update works (with associated fixes)

b2cb05b


          Cleanup

8e668b5


          Cleanup

5b14a3e


          Add verification of data changing

5f8d479


          Support aggregate clusters correctly

ec50490


          Fix class name referenced in javadoc

5dceeaf


          Address a number of code review comments and add a test for missing C…

fd64f20

…ds aggregate and eds entries.


          Remove syncContext from watchers. Add checkNotNull, private and final…

b2e924e

… as recommended by code review.


          Add a test for corrupt LDS

5a75b10


          Change aggregate cluster handling to correctly handle cluster names a…

d3b713f

…ppearing in multiple trees or overlapping with the RDS.virtualHosts.


          Errorprone


          Add max recursion limit for clusters to match c++.

d5dea83

larry-safran force-pushed the XdsDependencyManager branch from 32a2975 to d5dea83 Compare

January 14, 2025 02:04

ejona86 reviewed

View reviewed changes

Member

ejona86 left a comment

Found these comments pending. Sending

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated



		private XdsWatcherBase(XdsResourceType<T> type, String resourceName) {
		this.type = type;

Member

ejona86 Jan 9, 2025

checkNotNull? When we store a reference for later we do ourselves a service by checking for null. Otherwise by the time the code fails with null we have lost context where the value came from.

Contributor Author

larry-safran Jan 17, 2025

Done

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

ejona86 reviewed

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated Show resolved Hide resolved

larry-safran added 2 commits

January 15, 2025 13:16


          Fix handling of route and cluster updates.

28d29fb


          Make data private and XdsWatcherBase static

4a53fce

ejona86 reviewed

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated

+                public static final XdsClusterResource CLUSTER_RESOURCE = XdsClusterResource.getInstance();
+                public static final String TOP_CDS_CONTEXT = toContextStr(CLUSTER_RESOURCE.typeName(), "");
+                public static final XdsEndpointResource ENDPOINT_RESOURCE = XdsEndpointResource.getInstance();
+                private static final int MAX_CLUSTER_RECURSION_DEPTH = 16; // Matches core

Member

ejona86 Jan 16, 2025

s/core/c core/ (or "C++")? Core here would typically mean grpc-core, the Java code.

Contributor Author

larry-safran Jan 17, 2025

Went with C++

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated

+                  if (watcher instanceof CdsWatcher) {
+                    CdsWatcher cdsWatcher = (CdsWatcher) watcher;
+                    if (!cdsWatcher.parentContexts.isEmpty()) {
+                      return;

Member

ejona86 Jan 16, 2025

throw IllegalStateException? It seems if this case is ever hit there is a bug (e.g., the wrong cancelWatcher() overload was called).

Contributor Author

larry-safran Jan 17, 2025

Done

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java Outdated

+                    switch (update.clusterType()) {
+                      case EDS:
+                        setData(update);
+                        if (!hasWatcher(ENDPOINT_RESOURCE, update.edsServiceName())) {

Member

ejona86 Jan 16, 2025

The hasWatcher() method was suspicious; it shouldn't be significant whether there was a watcher already. So I looked here, and it seems to not handle multiple parents. I agree it is unlikely that two different clusters use the same endpoint child, but that's still totally permitted and would be working today.

Contributor Author

larry-safran Jan 17, 2025

Fixed it so that EdsWatcher keeps a list of its parents.

larry-safran added 9 commits

January 16, 2025 13:22


          In LDS onChanged(), get old activeVirtualHost before possibly doing c…

c97118f

…leanUpRdsWatcher().

Let cancelWatcher remove the old RDS watcher instead of doing it explicitly in cleanUpRdsWatcher.


          Change comment for clarity

06466fc


          Allow EdsWatcher to have multiple CdsWatcher parents

b898e34


          Allow EdsWatcher to have multiple CdsWatcher parents

954ced3


          Fully support inlined RouteConfig

ef13712


          Add lots of checkNotNull()

518cef1


          Add test case testMultipleParentsInCdsTree, make a couple of cluster …

68071e6

…related fixes and add some test utility methods for generating xds configuration.


          Add test case testMultipleParentsInCdsTree, make a couple of cluster …

f99fc56

…related fixes and add some test utility methods for generating xds configuration.


          Add virtual host to XdsConfig as per spec

482cd9d

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

Labels

None yet