Skip to content

Commit 6935d3a

Browse files
shivaspeaksshivaspeaks
authored
Revert "xds: add "resource_timer_is_transient_failure" server feature (#12063)" (#12228)
1 parent d7d70c6 commit 6935d3a

File tree

8 files changed

+41
-176
lines changed

8 files changed

+41
-176
lines changed

‎xds/src/main/java/io/grpc/xds/client/Bootstrapper.java‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,20 +63,17 @@ public abstract static class ServerInfo {
6363

6464
public abstract boolean isTrustedXdsServer();
6565

66-
public abstract boolean resourceTimerIsTransientError();
67-
6866
@VisibleForTesting
6967
public static ServerInfo create(String target, @Nullable Object implSpecificConfig) {
70-
return new AutoValue_Bootstrapper_ServerInfo(target, implSpecificConfig,
71-
false, false, false);
68+
return new AutoValue_Bootstrapper_ServerInfo(target, implSpecificConfig, false, false);
7269
}
7370

7471
@VisibleForTesting
7572
public static ServerInfo create(
7673
String target, Object implSpecificConfig, boolean ignoreResourceDeletion,
77-
boolean isTrustedXdsServer, boolean resourceTimerIsTransientError) {
74+
boolean isTrustedXdsServer) {
7875
return new AutoValue_Bootstrapper_ServerInfo(target, implSpecificConfig,
79-
ignoreResourceDeletion, isTrustedXdsServer, resourceTimerIsTransientError);
76+
ignoreResourceDeletion, isTrustedXdsServer);
8077
}
8178
}
8279

‎xds/src/main/java/io/grpc/xds/client/BootstrapperImpl.java‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,6 @@ public abstract class BootstrapperImpl extends Bootstrapper {
4343

4444
public static final String GRPC_EXPERIMENTAL_XDS_FALLBACK =
4545
"GRPC_EXPERIMENTAL_XDS_FALLBACK";
46-
public static final String GRPC_EXPERIMENTAL_XDS_DATA_ERROR_HANDLING =
47-
"GRPC_EXPERIMENTAL_XDS_DATA_ERROR_HANDLING";
4846

4947
// Client features.
5048
@VisibleForTesting
@@ -56,16 +54,10 @@ public abstract class BootstrapperImpl extends Bootstrapper {
5654
// Server features.
5755
private static final String SERVER_FEATURE_IGNORE_RESOURCE_DELETION = "ignore_resource_deletion";
5856
private static final String SERVER_FEATURE_TRUSTED_XDS_SERVER = "trusted_xds_server";
59-
private static final String
60-
SERVER_FEATURE_RESOURCE_TIMER_IS_TRANSIENT_ERROR = "resource_timer_is_transient_error";
6157

6258
@VisibleForTesting
6359
static boolean enableXdsFallback = GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_FALLBACK, true);
6460

65-
@VisibleForTesting
66-
public static boolean xdsDataErrorHandlingEnabled
67-
= GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DATA_ERROR_HANDLING, false);
68-
6961
protected final XdsLogger logger;
7062

7163
protected FileReader reader = LocalFileReader.INSTANCE;
@@ -255,22 +247,18 @@ private List<ServerInfo> parseServerInfos(List<?> rawServerConfigs, XdsLogger lo
255247

256248
Object implSpecificConfig = getImplSpecificConfig(serverConfig, serverUri);
257249

258-
boolean resourceTimerIsTransientError = false;
259250
boolean ignoreResourceDeletion = false;
260251
// "For forward compatibility reasons, the client will ignore any entry in the list that it
261252
// does not understand, regardless of type."
262253
List<?> serverFeatures = JsonUtil.getList(serverConfig, "server_features");
263254
if (serverFeatures != null) {
264255
logger.log(XdsLogLevel.INFO, "Server features: {0}", serverFeatures);
265256
ignoreResourceDeletion = serverFeatures.contains(SERVER_FEATURE_IGNORE_RESOURCE_DELETION);
266-
resourceTimerIsTransientError = xdsDataErrorHandlingEnabled
267-
&& serverFeatures.contains(SERVER_FEATURE_RESOURCE_TIMER_IS_TRANSIENT_ERROR);
268257
}
269258
servers.add(
270259
ServerInfo.create(serverUri, implSpecificConfig, ignoreResourceDeletion,
271260
serverFeatures != null
272-
&& serverFeatures.contains(SERVER_FEATURE_TRUSTED_XDS_SERVER),
273-
resourceTimerIsTransientError));
261+
&& serverFeatures.contains(SERVER_FEATURE_TRUSTED_XDS_SERVER)));
274262
}
275263
return servers.build();
276264
}

‎xds/src/main/java/io/grpc/xds/client/XdsClient.java‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -199,10 +199,6 @@ public static ResourceMetadata newResourceMetadataDoesNotExist() {
199199
return new ResourceMetadata(ResourceMetadataStatus.DOES_NOT_EXIST, "", 0, false, null, null);
200200
}
201201

202-
public static ResourceMetadata newResourceMetadataTimeout() {
203-
return new ResourceMetadata(ResourceMetadataStatus.TIMEOUT, "", 0, false, null, null);
204-
}
205-
206202
public static ResourceMetadata newResourceMetadataAcked(
207203
Any rawResource, String version, long updateTimeNanos) {
208204
checkNotNull(rawResource, "rawResource");
@@ -260,7 +256,7 @@ public UpdateFailureState getErrorState() {
260256
* config_dump.proto</a>
261257
*/
262258
public enum ResourceMetadataStatus {
263-
UNKNOWN, REQUESTED, DOES_NOT_EXIST, ACKED, NACKED, TIMEOUT
259+
UNKNOWN, REQUESTED, DOES_NOT_EXIST, ACKED, NACKED
264260
}
265261

266262
/**

‎xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java‎

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818

1919
import static com.google.common.base.Preconditions.checkArgument;
2020
import static com.google.common.base.Preconditions.checkNotNull;
21-
import static io.grpc.xds.client.BootstrapperImpl.xdsDataErrorHandlingEnabled;
2221
import static io.grpc.xds.client.XdsResourceType.ParsedResource;
2322
import static io.grpc.xds.client.XdsResourceType.ValidatedResourceUpdate;
2423

@@ -68,7 +67,6 @@ public final class XdsClientImpl extends XdsClient implements ResourceStore {
6867
// Longest time to wait, since the subscription to some resource, for concluding its absence.
6968
@VisibleForTesting
7069
public static final int INITIAL_RESOURCE_FETCH_TIMEOUT_SEC = 15;
71-
public static final int EXTENDED_RESOURCE_FETCH_TIMEOUT_SEC = 30;
7270

7371
private final SynchronizationContext syncContext = new SynchronizationContext(
7472
new Thread.UncaughtExceptionHandler() {
@@ -740,9 +738,6 @@ void restartTimer() {
740738
// When client becomes ready, it triggers a restartTimer for all relevant subscribers.
741739
return;
742740
}
743-
ServerInfo serverInfo = activeCpc.getServerInfo();
744-
int timeoutSec = xdsDataErrorHandlingEnabled && serverInfo.resourceTimerIsTransientError()
745-
? EXTENDED_RESOURCE_FETCH_TIMEOUT_SEC : INITIAL_RESOURCE_FETCH_TIMEOUT_SEC;
746741

747742
class ResourceNotFound implements Runnable {
748743
@Override
@@ -766,7 +761,8 @@ public String toString() {
766761
respTimer.cancel();
767762
}
768763
respTimer = syncContext.schedule(
769-
new ResourceNotFound(), timeoutSec, TimeUnit.SECONDS, timeService);
764+
new ResourceNotFound(), INITIAL_RESOURCE_FETCH_TIMEOUT_SEC, TimeUnit.SECONDS,
765+
timeService);
770766
}
771767

772768
void stopTimer() {
@@ -844,8 +840,6 @@ void onAbsent(@Nullable ProcessingTracker processingTracker, ServerInfo serverIn
844840
// Ignore deletion of State of the World resources when this feature is on,
845841
// and the resource is reusable.
846842
boolean ignoreResourceDeletionEnabled = serverInfo.ignoreResourceDeletion();
847-
boolean resourceTimerIsTransientError =
848-
xdsDataErrorHandlingEnabled && serverInfo.resourceTimerIsTransientError();
849843
if (ignoreResourceDeletionEnabled && type.isFullStateOfTheWorld() && data != null) {
850844
if (!resourceDeletionIgnored) {
851845
logger.log(XdsLogLevel.FORCE_WARNING,
@@ -860,20 +854,14 @@ void onAbsent(@Nullable ProcessingTracker processingTracker, ServerInfo serverIn
860854
if (!absent) {
861855
data = null;
862856
absent = true;
863-
metadata = resourceTimerIsTransientError ? ResourceMetadata.newResourceMetadataTimeout() :
864-
ResourceMetadata.newResourceMetadataDoesNotExist();
857+
metadata = ResourceMetadata.newResourceMetadataDoesNotExist();
865858
for (ResourceWatcher<T> watcher : watchers.keySet()) {
866859
if (processingTracker != null) {
867860
processingTracker.startTask();
868861
}
869862
watchers.get(watcher).execute(() -> {
870863
try {
871-
if (resourceTimerIsTransientError) {
872-
watcher.onError(Status.UNAVAILABLE.withDescription(
873-
"Timed out waiting for resource " + resource + " from xDS server"));
874-
} else {
875-
watcher.onResourceDoesNotExist(resource);
876-
}
864+
watcher.onResourceDoesNotExist(resource);
877865
} finally {
878866
if (processingTracker != null) {
879867
processingTracker.onComplete();

‎xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3549,7 +3549,7 @@ private static Filter buildHttpConnectionManagerFilter(HttpFilter... httpFilters
35493549

35503550
private XdsResourceType.Args getXdsResourceTypeArgs(boolean isTrustedServer) {
35513551
return new XdsResourceType.Args(
3552-
ServerInfo.create("http://td", "", false, isTrustedServer, false), "1.0", null, null, null, null
3552+
ServerInfo.create("http://td", "", false, isTrustedServer), "1.0", null, null, null, null
35533553
);
35543554
}
35553555
}

‎xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java‎

Lines changed: 28 additions & 132 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@
8585
import io.grpc.xds.client.Bootstrapper.BootstrapInfo;
8686
import io.grpc.xds.client.Bootstrapper.CertificateProviderInfo;
8787
import io.grpc.xds.client.Bootstrapper.ServerInfo;
88-
import io.grpc.xds.client.BootstrapperImpl;
8988
import io.grpc.xds.client.EnvoyProtoData.Node;
9089
import io.grpc.xds.client.LoadStatsManager2.ClusterDropStats;
9190
import io.grpc.xds.client.Locality;
@@ -146,7 +145,7 @@
146145
public abstract class GrpcXdsClientImplTestBase {
147146

148147
private static final String SERVER_URI = "trafficdirector.googleapis.com";
149-
private static final String SERVER_URI_CUSTOM_AUTHORITY = "trafficdirector2.googleapis.com";
148+
private static final String SERVER_URI_CUSTOME_AUTHORITY = "trafficdirector2.googleapis.com";
150149
private static final String SERVER_URI_EMPTY_AUTHORITY = "trafficdirector3.googleapis.com";
151150
private static final String LDS_RESOURCE = "listener.googleapis.com";
152151
private static final String RDS_RESOURCE = "route-configuration.googleapis.com";
@@ -305,30 +304,6 @@ public long currentTimeNanos() {
305304
private final BindableService adsService = createAdsService();
306305
private final BindableService lrsService = createLrsService();
307306

308-
private XdsTransportFactory xdsTransportFactory = new XdsTransportFactory() {
309-
@Override
310-
public XdsTransport create(ServerInfo serverInfo) {
311-
if (serverInfo.target().equals(SERVER_URI)) {
312-
return new GrpcXdsTransport(channel);
313-
}
314-
if (serverInfo.target().equals(SERVER_URI_CUSTOM_AUTHORITY)) {
315-
if (channelForCustomAuthority == null) {
316-
channelForCustomAuthority = cleanupRule.register(
317-
InProcessChannelBuilder.forName(serverName).directExecutor().build());
318-
}
319-
return new GrpcXdsTransport(channelForCustomAuthority);
320-
}
321-
if (serverInfo.target().equals(SERVER_URI_EMPTY_AUTHORITY)) {
322-
if (channelForEmptyAuthority == null) {
323-
channelForEmptyAuthority = cleanupRule.register(
324-
InProcessChannelBuilder.forName(serverName).directExecutor().build());
325-
}
326-
return new GrpcXdsTransport(channelForEmptyAuthority);
327-
}
328-
throw new IllegalArgumentException("Can not create channel for " + serverInfo);
329-
}
330-
};
331-
332307
@Before
333308
public void setUp() throws IOException {
334309
when(backoffPolicyProvider.get()).thenReturn(backoffPolicy1, backoffPolicy2);
@@ -347,9 +322,32 @@ public void setUp() throws IOException {
347322
.start());
348323
channel =
349324
cleanupRule.register(InProcessChannelBuilder.forName(serverName).directExecutor().build());
325+
XdsTransportFactory xdsTransportFactory = new XdsTransportFactory() {
326+
@Override
327+
public XdsTransport create(ServerInfo serverInfo) {
328+
if (serverInfo.target().equals(SERVER_URI)) {
329+
return new GrpcXdsTransport(channel);
330+
}
331+
if (serverInfo.target().equals(SERVER_URI_CUSTOME_AUTHORITY)) {
332+
if (channelForCustomAuthority == null) {
333+
channelForCustomAuthority = cleanupRule.register(
334+
InProcessChannelBuilder.forName(serverName).directExecutor().build());
335+
}
336+
return new GrpcXdsTransport(channelForCustomAuthority);
337+
}
338+
if (serverInfo.target().equals(SERVER_URI_EMPTY_AUTHORITY)) {
339+
if (channelForEmptyAuthority == null) {
340+
channelForEmptyAuthority = cleanupRule.register(
341+
InProcessChannelBuilder.forName(serverName).directExecutor().build());
342+
}
343+
return new GrpcXdsTransport(channelForEmptyAuthority);
344+
}
345+
throw new IllegalArgumentException("Can not create channel for " + serverInfo);
346+
}
347+
};
350348

351349
xdsServerInfo = ServerInfo.create(SERVER_URI, CHANNEL_CREDENTIALS, ignoreResourceDeletion(),
352-
true, false);
350+
true);
353351
BootstrapInfo bootstrapInfo =
354352
Bootstrapper.BootstrapInfo.builder()
355353
.servers(Collections.singletonList(xdsServerInfo))
@@ -359,7 +357,7 @@ public void setUp() throws IOException {
359357
AuthorityInfo.create(
360358
"xdstp://authority.xds.com/envoy.config.listener.v3.Listener/%s",
361359
ImmutableList.of(Bootstrapper.ServerInfo.create(
362-
SERVER_URI_CUSTOM_AUTHORITY, CHANNEL_CREDENTIALS))),
360+
SERVER_URI_CUSTOME_AUTHORITY, CHANNEL_CREDENTIALS))),
363361
"",
364362
AuthorityInfo.create(
365363
"xdstp:///envoy.config.listener.v3.Listener/%s",
@@ -3157,108 +3155,6 @@ public void flowControlAbsent() throws Exception {
31573155
verify(anotherWatcher).onError(any());
31583156
}
31593157

3160-
@Test
3161-
@SuppressWarnings("unchecked")
3162-
public void resourceTimerIsTransientError_schedulesExtendedTimeout() {
3163-
BootstrapperImpl.xdsDataErrorHandlingEnabled = true;
3164-
ServerInfo serverInfo = ServerInfo.create(SERVER_URI, CHANNEL_CREDENTIALS,
3165-
false, true, true);
3166-
BootstrapInfo bootstrapInfo =
3167-
Bootstrapper.BootstrapInfo.builder()
3168-
.servers(Collections.singletonList(serverInfo))
3169-
.node(NODE)
3170-
.authorities(ImmutableMap.of(
3171-
"",
3172-
AuthorityInfo.create(
3173-
"xdstp:///envoy.config.listener.v3.Listener/%s",
3174-
ImmutableList.of(Bootstrapper.ServerInfo.create(
3175-
SERVER_URI_EMPTY_AUTHORITY, CHANNEL_CREDENTIALS)))))
3176-
.certProviders(ImmutableMap.of())
3177-
.build();
3178-
xdsClient = new XdsClientImpl(
3179-
xdsTransportFactory,
3180-
bootstrapInfo,
3181-
fakeClock.getScheduledExecutorService(),
3182-
backoffPolicyProvider,
3183-
fakeClock.getStopwatchSupplier(),
3184-
timeProvider,
3185-
MessagePrinter.INSTANCE,
3186-
new TlsContextManagerImpl(bootstrapInfo),
3187-
xdsClientMetricReporter);
3188-
ResourceWatcher<CdsUpdate> watcher = mock(ResourceWatcher.class);
3189-
String resourceName = "cluster.googleapis.com";
3190-
3191-
xdsClient.watchXdsResource(
3192-
XdsClusterResource.getInstance(),
3193-
resourceName,
3194-
watcher,
3195-
fakeClock.getScheduledExecutorService());
3196-
3197-
ScheduledTask task = Iterables.getOnlyElement(
3198-
fakeClock.getPendingTasks(CDS_RESOURCE_FETCH_TIMEOUT_TASK_FILTER));
3199-
assertThat(task.getDelay(TimeUnit.SECONDS))
3200-
.isEqualTo(XdsClientImpl.EXTENDED_RESOURCE_FETCH_TIMEOUT_SEC);
3201-
fakeClock.runDueTasks();
3202-
BootstrapperImpl.xdsDataErrorHandlingEnabled = false;
3203-
}
3204-
3205-
@Test
3206-
@SuppressWarnings("unchecked")
3207-
public void resourceTimerIsTransientError_callsOnErrorUnavailable() {
3208-
BootstrapperImpl.xdsDataErrorHandlingEnabled = true;
3209-
xdsServerInfo = ServerInfo.create(SERVER_URI, CHANNEL_CREDENTIALS, ignoreResourceDeletion(),
3210-
true, true);
3211-
BootstrapInfo bootstrapInfo =
3212-
Bootstrapper.BootstrapInfo.builder()
3213-
.servers(Collections.singletonList(xdsServerInfo))
3214-
.node(NODE)
3215-
.authorities(ImmutableMap.of(
3216-
"authority.xds.com",
3217-
AuthorityInfo.create(
3218-
"xdstp://authority.xds.com/envoy.config.listener.v3.Listener/%s",
3219-
ImmutableList.of(Bootstrapper.ServerInfo.create(
3220-
SERVER_URI_CUSTOM_AUTHORITY, CHANNEL_CREDENTIALS))),
3221-
"",
3222-
AuthorityInfo.create(
3223-
"xdstp:///envoy.config.listener.v3.Listener/%s",
3224-
ImmutableList.of(Bootstrapper.ServerInfo.create(
3225-
SERVER_URI_EMPTY_AUTHORITY, CHANNEL_CREDENTIALS)))))
3226-
.certProviders(ImmutableMap.of("cert-instance-name",
3227-
CertificateProviderInfo.create("file-watcher", ImmutableMap.of())))
3228-
.build();
3229-
xdsClient = new XdsClientImpl(
3230-
xdsTransportFactory,
3231-
bootstrapInfo,
3232-
fakeClock.getScheduledExecutorService(),
3233-
backoffPolicyProvider,
3234-
fakeClock.getStopwatchSupplier(),
3235-
timeProvider,
3236-
MessagePrinter.INSTANCE,
3237-
new TlsContextManagerImpl(bootstrapInfo),
3238-
xdsClientMetricReporter);
3239-
String timeoutResource = CDS_RESOURCE + "_timeout";
3240-
ResourceWatcher<CdsUpdate> timeoutWatcher = mock(ResourceWatcher.class);
3241-
3242-
xdsClient.watchXdsResource(
3243-
XdsClusterResource.getInstance(),
3244-
timeoutResource,
3245-
timeoutWatcher,
3246-
fakeClock.getScheduledExecutorService());
3247-
3248-
assertThat(resourceDiscoveryCalls).hasSize(1);
3249-
DiscoveryRpcCall call = resourceDiscoveryCalls.poll();
3250-
call.verifyRequest(CDS, ImmutableList.of(timeoutResource), "", "", NODE);
3251-
fakeClock.forwardTime(XdsClientImpl.EXTENDED_RESOURCE_FETCH_TIMEOUT_SEC, TimeUnit.SECONDS);
3252-
fakeClock.runDueTasks();
3253-
ArgumentCaptor<Status> errorCaptor = ArgumentCaptor.forClass(Status.class);
3254-
verify(timeoutWatcher).onError(errorCaptor.capture());
3255-
Status error = errorCaptor.getValue();
3256-
assertThat(error.getCode()).isEqualTo(Status.Code.UNAVAILABLE);
3257-
assertThat(error.getDescription()).isEqualTo(
3258-
"Timed out waiting for resource " + timeoutResource + " from xDS server");
3259-
BootstrapperImpl.xdsDataErrorHandlingEnabled = false;
3260-
}
3261-
32623158
private Answer<Void> blockUpdate(CyclicBarrier barrier) {
32633159
return new Answer<Void>() {
32643160
@Override
@@ -4324,7 +4220,7 @@ private XdsClientImpl createXdsClient(String serverUri) {
43244220
private BootstrapInfo buildBootStrap(String serverUri) {
43254221

43264222
ServerInfo xdsServerInfo = ServerInfo.create(serverUri, CHANNEL_CREDENTIALS,
4327-
ignoreResourceDeletion(), true, false);
4223+
ignoreResourceDeletion(), true);
43284224

43294225
return Bootstrapper.BootstrapInfo.builder()
43304226
.servers(Collections.singletonList(xdsServerInfo))
@@ -4334,7 +4230,7 @@ private BootstrapInfo buildBootStrap(String serverUri) {
43344230
AuthorityInfo.create(
43354231
"xdstp://authority.xds.com/envoy.config.listener.v3.Listener/%s",
43364232
ImmutableList.of(Bootstrapper.ServerInfo.create(
4337-
SERVER_URI_CUSTOM_AUTHORITY, CHANNEL_CREDENTIALS))),
4233+
SERVER_URI_CUSTOME_AUTHORITY, CHANNEL_CREDENTIALS))),
43384234
"",
43394235
AuthorityInfo.create(
43404236
"xdstp:///envoy.config.listener.v3.Listener/%s",

0 commit comments

Comments
 (0)