From 36ce0e27cf43d6ee0884ad03d28dbe4364674423 Mon Sep 17 00:00:00 2001 From: sanjaypujare Date: Tue, 14 Jun 2022 20:45:25 -0700 Subject: [PATCH] xds: delete the permanent error logic in processing LDS updates in XdsServerWrapper (#9268) (#9276) --- .../java/io/grpc/xds/XdsServerBuilder.java | 9 ++- .../java/io/grpc/xds/XdsServerWrapper.java | 19 +----- .../XdsClientWrapperForServerSdsTestMisc.java | 67 ------------------- .../io/grpc/xds/XdsServerWrapperTest.java | 9 ++- 4 files changed, 10 insertions(+), 94 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java b/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java index d4df317a7e9..e6dac5d25af 100644 --- a/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java +++ b/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java @@ -176,19 +176,18 @@ public interface XdsServingStatusListener { private static class DefaultListener implements XdsServingStatusListener { private final Logger logger; private final String prefix; - boolean notServing; + boolean notServingDueToError; DefaultListener(String prefix) { logger = Logger.getLogger(DefaultListener.class.getName()); this.prefix = prefix; - notServing = true; } /** Log calls to onServing() following a call to onNotServing() at WARNING level. */ @Override public void onServing() { - if (notServing) { - notServing = false; + if (notServingDueToError) { + notServingDueToError = false; logger.warning("[" + prefix + "] Entering serving state."); } } @@ -196,7 +195,7 @@ public void onServing() { @Override public void onNotServing(Throwable throwable) { logger.warning("[" + prefix + "] " + throwable.getMessage()); - notServing = true; + notServingDueToError = true; } } } diff --git a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java index 7b6d05e0738..366f01e2daa 100644 --- a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java +++ b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java @@ -61,7 +61,6 @@ import java.net.SocketAddress; import java.util.ArrayList; import java.util.Collections; -import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -445,12 +444,8 @@ public void run() { if (stopped) { return; } - boolean isPermanentError = isPermanentError(error); - logger.log(Level.FINE, "{0} error from XdsClient: {1}", - new Object[]{isPermanentError ? "Permanent" : "Transient", error}); - if (isPermanentError) { - handleConfigNotFound(error.asException()); - } else if (!isServing) { + logger.log(Level.FINE, "Error from XdsClient", error); + if (!isServing) { listener.onNotServing(error.asException()); } } @@ -719,16 +714,6 @@ private void maybeUpdateSelector() { } } } - - private boolean isPermanentError(Status error) { - return EnumSet.of( - Status.Code.INTERNAL, - Status.Code.INVALID_ARGUMENT, - Status.Code.FAILED_PRECONDITION, - Status.Code.PERMISSION_DENIED, - Status.Code.UNAUTHENTICATED) - .contains(error.getCode()); - } } @VisibleForTesting diff --git a/xds/src/test/java/io/grpc/xds/XdsClientWrapperForServerSdsTestMisc.java b/xds/src/test/java/io/grpc/xds/XdsClientWrapperForServerSdsTestMisc.java index e698397b2fe..442bfef653c 100644 --- a/xds/src/test/java/io/grpc/xds/XdsClientWrapperForServerSdsTestMisc.java +++ b/xds/src/test/java/io/grpc/xds/XdsClientWrapperForServerSdsTestMisc.java @@ -199,56 +199,6 @@ public void run() { assertThat(selectorManager.getSelectorToUpdateSelector()).isSameInstanceAs(NO_FILTER_CHAIN); } - @Test - public void registerServerWatcher_notifyInternalError() throws Exception { - final SettableFuture start = SettableFuture.create(); - Executors.newSingleThreadExecutor().execute(new Runnable() { - @Override - public void run() { - try { - start.set(xdsServerWrapper.start()); - } catch (Exception ex) { - start.setException(ex); - } - } - }); - xdsClient.ldsResource.get(5, TimeUnit.SECONDS); - xdsClient.ldsWatcher.onError(Status.INTERNAL); - verify(listener, timeout(5000)).onNotServing(any()); - try { - start.get(START_WAIT_AFTER_LISTENER_MILLIS, TimeUnit.MILLISECONDS); - fail("Start should throw exception"); - } catch (TimeoutException ex) { - assertThat(start.isDone()).isFalse(); - } - assertThat(selectorManager.getSelectorToUpdateSelector()).isSameInstanceAs(NO_FILTER_CHAIN); - } - - @Test - public void registerServerWatcher_notifyPermDeniedError() throws Exception { - final SettableFuture start = SettableFuture.create(); - Executors.newSingleThreadExecutor().execute(new Runnable() { - @Override - public void run() { - try { - start.set(xdsServerWrapper.start()); - } catch (Exception ex) { - start.setException(ex); - } - } - }); - xdsClient.ldsResource.get(5, TimeUnit.SECONDS); - xdsClient.ldsWatcher.onError(Status.PERMISSION_DENIED); - verify(listener, timeout(5000)).onNotServing(any()); - try { - start.get(START_WAIT_AFTER_LISTENER_MILLIS, TimeUnit.MILLISECONDS); - fail("Start should throw exception"); - } catch (TimeoutException ex) { - assertThat(start.isDone()).isFalse(); - } - assertThat(selectorManager.getSelectorToUpdateSelector()).isSameInstanceAs(NO_FILTER_CHAIN); - } - @Test public void releaseOldSupplierOnChanged_noCloseDueToLazyLoading() throws Exception { InetAddress ipLocalAddress = InetAddress.getByName("10.1.2.3"); @@ -327,23 +277,6 @@ public void releaseOldSupplierOnNotFound_verifyClose() throws Exception { verify(tlsContextManager, times(1)).releaseServerSslContextProvider(eq(sslContextProvider1)); } - @Test - public void releaseOldSupplierOnPermDeniedError_verifyClose() throws Exception { - SslContextProvider sslContextProvider1 = mock(SslContextProvider.class); - when(tlsContextManager.findOrCreateServerSslContextProvider(eq(tlsContext1))) - .thenReturn(sslContextProvider1); - InetAddress ipLocalAddress = InetAddress.getByName("10.1.2.3"); - localAddress = new InetSocketAddress(ipLocalAddress, PORT); - sendListenerUpdate(localAddress, tlsContext1, null, - tlsContextManager); - SslContextProviderSupplier returnedSupplier = - getSslContextProviderSupplier(selectorManager.getSelectorToUpdateSelector()); - assertThat(returnedSupplier.getTlsContext()).isSameInstanceAs(tlsContext1); - callUpdateSslContext(returnedSupplier); - xdsClient.ldsWatcher.onError(Status.PERMISSION_DENIED); - verify(tlsContextManager, times(1)).releaseServerSslContextProvider(eq(sslContextProvider1)); - } - @Test public void releaseOldSupplierOnTemporaryError_noClose() throws Exception { SslContextProvider sslContextProvider1 = mock(SslContextProvider.class); diff --git a/xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java b/xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java index c86d34a45c7..a546a95459a 100644 --- a/xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java @@ -685,7 +685,7 @@ public void run() { xdsClient.ldsWatcher.onError(Status.INTERNAL); assertThat(selectorManager.getSelectorToUpdateSelector()) .isSameInstanceAs(FilterChainSelector.NO_FILTER_CHAIN); - assertThat(xdsClient.rdsWatchers).isEmpty(); + RdsResourceWatcher saveRdsWatcher = xdsClient.rdsWatchers.get("rds"); verify(mockBuilder, times(1)).build(); verify(listener, times(2)).onNotServing(any(StatusException.class)); assertThat(sslSupplier0.isShutdown()).isFalse(); @@ -705,7 +705,6 @@ public void run() { assertThat(ex.getCause()).isInstanceOf(IOException.class); assertThat(ex.getCause().getMessage()).isEqualTo("error!"); } - RdsResourceWatcher saveRdsWatcher = xdsClient.rdsWatchers.get("rds"); assertThat(executor.forwardNanos(RETRY_DELAY_NANOS)).isEqualTo(1); verify(mockBuilder, times(1)).build(); verify(mockServer, times(2)).start(); @@ -739,7 +738,7 @@ public void run() { // not serving after serving xdsClient.ldsWatcher.onResourceDoesNotExist(ldsResource); assertThat(xdsClient.rdsWatchers).isEmpty(); - verify(mockServer, times(3)).shutdown(); + verify(mockServer, times(2)).shutdown(); when(mockServer.isShutdown()).thenReturn(true); assertThat(selectorManager.getSelectorToUpdateSelector()) .isSameInstanceAs(FilterChainSelector.NO_FILTER_CHAIN); @@ -777,7 +776,7 @@ public void run() { assertThat(executor.numPendingTasks()).isEqualTo(1); xdsClient.ldsWatcher.onResourceDoesNotExist(ldsResource); - verify(mockServer, times(4)).shutdown(); + verify(mockServer, times(3)).shutdown(); verify(listener, times(4)).onNotServing(any(StatusException.class)); when(mockServer.isShutdown()).thenReturn(true); assertThat(executor.numPendingTasks()).isEqualTo(0); @@ -804,7 +803,7 @@ public void run() { assertThat(realConfig.interceptors()).isEqualTo(ImmutableMap.of()); xdsServerWrapper.shutdown(); - verify(mockServer, times(5)).shutdown(); + verify(mockServer, times(4)).shutdown(); assertThat(sslSupplier3.isShutdown()).isTrue(); when(mockServer.awaitTermination(anyLong(), any(TimeUnit.class))).thenReturn(true); assertThat(xdsServerWrapper.awaitTermination(5, TimeUnit.SECONDS)).isTrue();