Skip to content

Commit 41dd0c6

Browse files
authored
xds:Cleanup to reduce test flakiness (#11895)
* don't process resourceDoesNotExist for watchers that have been cancelled. * 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.
1 parent 5a7f350 commit 41dd0c6

File tree

2 files changed

+37
-3
lines changed

2 files changed

+37
-3
lines changed

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

+16
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ private <T extends ResourceUpdate> void cancelWatcher(XdsWatcherBase<T> watcher)
149149
throwIfParentContextsNotEmpty(watcher);
150150
}
151151

152+
watcher.cancelled = true;
152153
XdsResourceType<T> type = watcher.type;
153154
String resourceName = watcher.resourceName;
154155

@@ -597,6 +598,8 @@ private abstract static class XdsWatcherBase<T extends ResourceUpdate>
597598
implements ResourceWatcher<T> {
598599
private final XdsResourceType<T> type;
599600
private final String resourceName;
601+
boolean cancelled;
602+
600603
@Nullable
601604
private StatusOr<T> data;
602605

@@ -693,6 +696,10 @@ public void onError(Status error) {
693696

694697
@Override
695698
public void onResourceDoesNotExist(String resourceName) {
699+
if (cancelled) {
700+
return;
701+
}
702+
696703
handleDoesNotExist(resourceName);
697704
xdsConfigWatcher.onResourceDoesNotExist(toContextString());
698705
}
@@ -752,6 +759,9 @@ public void onError(Status error) {
752759

753760
@Override
754761
public void onResourceDoesNotExist(String resourceName) {
762+
if (cancelled) {
763+
return;
764+
}
755765
handleDoesNotExist(checkNotNull(resourceName, "resourceName"));
756766
xdsConfigWatcher.onResourceDoesNotExist(toContextString());
757767
}
@@ -836,6 +846,9 @@ public void onChanged(XdsClusterResource.CdsUpdate update) {
836846

837847
@Override
838848
public void onResourceDoesNotExist(String resourceName) {
849+
if (cancelled) {
850+
return;
851+
}
839852
handleDoesNotExist(checkNotNull(resourceName, "resourceName"));
840853
maybePublishConfig();
841854
}
@@ -857,6 +870,9 @@ public void onChanged(XdsEndpointResource.EdsUpdate update) {
857870

858871
@Override
859872
public void onResourceDoesNotExist(String resourceName) {
873+
if (cancelled) {
874+
return;
875+
}
860876
handleDoesNotExist(checkNotNull(resourceName, "resourceName"));
861877
maybePublishConfig();
862878
}

xds/src/test/java/io/grpc/xds/XdsDependencyManagerTest.java

+21-3
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
import org.junit.runner.RunWith;
8989
import org.junit.runners.JUnit4;
9090
import org.mockito.ArgumentCaptor;
91+
import org.mockito.ArgumentMatcher;
9192
import org.mockito.ArgumentMatchers;
9293
import org.mockito.Captor;
9394
import org.mockito.InOrder;
@@ -696,9 +697,9 @@ public void testChangeAggCluster() {
696697
controlPlaneService.setXdsConfig(ADS_TYPE_URL_EDS, edsMap);
697698

698699
// Verify that the config is updated as expected
699-
inOrder.verify(xdsConfigWatcher, timeout(1000)).onUpdate(xdsConfigCaptor.capture());
700-
XdsConfig config = xdsConfigCaptor.getValue();
701-
assertThat(config.getClusters().keySet()).containsExactly("root", "clusterA21", "clusterA22");
700+
ClusterNameMatcher nameMatcher
701+
= new ClusterNameMatcher(Arrays.asList("root", "clusterA21", "clusterA22"));
702+
inOrder.verify(xdsConfigWatcher, timeout(1000)).onUpdate(argThat(nameMatcher));
702703
}
703704

704705
private Listener buildInlineClientListener(String rdsName, String clusterName) {
@@ -789,4 +790,21 @@ void deliverLdsUpdate(long httpMaxStreamDurationNano,
789790
});
790791
}
791792
}
793+
794+
static class ClusterNameMatcher implements ArgumentMatcher<XdsConfig> {
795+
private final List<String> expectedNames;
796+
797+
ClusterNameMatcher(List<String> expectedNames) {
798+
this.expectedNames = expectedNames;
799+
}
800+
801+
@Override
802+
public boolean matches(XdsConfig xdsConfig) {
803+
if (xdsConfig == null || xdsConfig.getClusters() == null) {
804+
return false;
805+
}
806+
return xdsConfig.getClusters().size() == expectedNames.size()
807+
&& xdsConfig.getClusters().keySet().containsAll(expectedNames);
808+
}
809+
}
792810
}

0 commit comments

Comments
 (0)