From d2bea3023fc9e7a2f43ed700f9b16f2de9f577f9 Mon Sep 17 00:00:00 2001 From: Larry Safran Date: Wed, 12 Feb 2025 15:16:14 -0800 Subject: [PATCH 1/2] don't process resourceDoesNotExist for watchers that have been cancelled. --- .../java/io/grpc/xds/XdsDependencyManager.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/xds/src/main/java/io/grpc/xds/XdsDependencyManager.java b/xds/src/main/java/io/grpc/xds/XdsDependencyManager.java index 4adfebfe74d..7eff0c549e2 100644 --- a/xds/src/main/java/io/grpc/xds/XdsDependencyManager.java +++ b/xds/src/main/java/io/grpc/xds/XdsDependencyManager.java @@ -149,6 +149,7 @@ private void cancelWatcher(XdsWatcherBase watcher) throwIfParentContextsNotEmpty(watcher); } + watcher.cancelled = true; XdsResourceType type = watcher.type; String resourceName = watcher.resourceName; @@ -597,6 +598,8 @@ private abstract static class XdsWatcherBase implements ResourceWatcher { private final XdsResourceType type; private final String resourceName; + boolean cancelled; + @Nullable private StatusOr data; @@ -693,6 +696,10 @@ public void onError(Status error) { @Override public void onResourceDoesNotExist(String resourceName) { + if (cancelled) { + return; + } + handleDoesNotExist(resourceName); xdsConfigWatcher.onResourceDoesNotExist(toContextString()); } @@ -752,6 +759,9 @@ public void onError(Status error) { @Override public void onResourceDoesNotExist(String resourceName) { + if (cancelled) { + return; + } handleDoesNotExist(checkNotNull(resourceName, "resourceName")); xdsConfigWatcher.onResourceDoesNotExist(toContextString()); } @@ -836,6 +846,9 @@ public void onChanged(XdsClusterResource.CdsUpdate update) { @Override public void onResourceDoesNotExist(String resourceName) { + if (cancelled) { + return; + } handleDoesNotExist(checkNotNull(resourceName, "resourceName")); maybePublishConfig(); } @@ -857,6 +870,9 @@ public void onChanged(XdsEndpointResource.EdsUpdate update) { @Override public void onResourceDoesNotExist(String resourceName) { + if (cancelled) { + return; + } handleDoesNotExist(checkNotNull(resourceName, "resourceName")); maybePublishConfig(); } From c871e58aa1ee8e0ee78fea184388fc7b6d867092 Mon Sep 17 00:00:00 2001 From: Larry Safran Date: Thu, 13 Feb 2025 15:35:39 -0800 Subject: [PATCH 2/2] Change test to use an ArgumentMatcher instead of expecting that only the final result will be sent since depending on timing there may be configs sent for clusters being removed with their entries as errors. --- .../io/grpc/xds/XdsDependencyManagerTest.java | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/XdsDependencyManagerTest.java b/xds/src/test/java/io/grpc/xds/XdsDependencyManagerTest.java index c4b7c6c8ace..f94a94a9446 100644 --- a/xds/src/test/java/io/grpc/xds/XdsDependencyManagerTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsDependencyManagerTest.java @@ -88,6 +88,7 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import org.mockito.ArgumentCaptor; +import org.mockito.ArgumentMatcher; import org.mockito.ArgumentMatchers; import org.mockito.Captor; import org.mockito.InOrder; @@ -696,9 +697,9 @@ public void testChangeAggCluster() { controlPlaneService.setXdsConfig(ADS_TYPE_URL_EDS, edsMap); // Verify that the config is updated as expected - inOrder.verify(xdsConfigWatcher, timeout(1000)).onUpdate(xdsConfigCaptor.capture()); - XdsConfig config = xdsConfigCaptor.getValue(); - assertThat(config.getClusters().keySet()).containsExactly("root", "clusterA21", "clusterA22"); + ClusterNameMatcher nameMatcher + = new ClusterNameMatcher(Arrays.asList("root", "clusterA21", "clusterA22")); + inOrder.verify(xdsConfigWatcher, timeout(1000)).onUpdate(argThat(nameMatcher)); } private Listener buildInlineClientListener(String rdsName, String clusterName) { @@ -789,4 +790,21 @@ void deliverLdsUpdate(long httpMaxStreamDurationNano, }); } } + + static class ClusterNameMatcher implements ArgumentMatcher { + private final List expectedNames; + + ClusterNameMatcher(List expectedNames) { + this.expectedNames = expectedNames; + } + + @Override + public boolean matches(XdsConfig xdsConfig) { + if (xdsConfig == null || xdsConfig.getClusters() == null) { + return false; + } + return xdsConfig.getClusters().size() == expectedNames.size() + && xdsConfig.getClusters().keySet().containsAll(expectedNames); + } + } }