From c56f65ac7fde9c5da5c05ca5b765be1be0b25934 Mon Sep 17 00:00:00 2001 From: Morten Tokle Date: Sat, 24 Jun 2023 00:03:02 +0200 Subject: Make stop more robust --- .../container/jdisc/DataplaneProxyService.java | 43 ++++++++++++++-------- .../container/jdisc/DataplaneProxyServiceTest.java | 34 ++++++++++++++++- 2 files changed, 61 insertions(+), 16 deletions(-) (limited to 'container-disc') diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java b/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java index e6af65c0bc8..5fc2032bec2 100644 --- a/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java @@ -36,8 +36,8 @@ public class DataplaneProxyService extends AbstractComponent { private final Path root; enum NginxState {INITIALIZING, RUNNING, RELOAD_REQUIRED, STOPPED}; - private NginxState state; - private NginxState wantedState; + private volatile NginxState state; + private volatile NginxState wantedState; private DataplaneProxyConfig cfg; private Path proxyCredentialsCert; @@ -113,35 +113,46 @@ public class DataplaneProxyService extends AbstractComponent { throw new RuntimeException("Error reconfiguring data plane proxy", e); } } - if (wantedState == NginxState.RUNNING) { + NginxState convergeTo = wantedState; + if (convergeTo == NginxState.RUNNING) { boolean nginxRunning = proxyCommands.isRunning(); if (!nginxRunning) { try { proxyCommands.start(nginxConf); - changeState(wantedState); + changeState(convergeTo); } catch (Exception e) { logger.log(Level.INFO, "Failed to start nginx, will retry"); + logger.log(Level.FINE, "Exception from nginx start", e); } - } else if (nginxRunning && state == NginxState.RELOAD_REQUIRED) { - try { - proxyCommands.reload(); - changeState(wantedState); - } catch (Exception e) { - logger.log(Level.INFO, "Failed to reconfigure nginx, will retry."); + } else { + if (state == NginxState.RELOAD_REQUIRED) { + try { + proxyCommands.reload(); + changeState(convergeTo); + } catch (Exception e) { + logger.log(Level.INFO, "Failed to reconfigure nginx, will retry."); + logger.log(Level.FINE, "Exception from nginx reload", e); + } + } else if (state != convergeTo) { + // Already running, but state not updated + changeState(convergeTo); } } - } else if (wantedState == NginxState.STOPPED) { + } else if (convergeTo == NginxState.STOPPED) { if (proxyCommands.isRunning()) { try { proxyCommands.stop(); - changeState(wantedState); - executorService.shutdownNow(); } catch (Exception e) { logger.log(Level.INFO, "Failed to stop nginx, will retry"); + logger.log(Level.FINE, "Exception from nginx stop", e); } } + if (! proxyCommands.isRunning()) { + changeState(convergeTo); + executorService.shutdownNow(); + } } else { - logger.warning("Unknown state " + wantedState); + logger.warning("Unknown state " + convergeTo); } } @@ -152,7 +163,7 @@ public class DataplaneProxyService extends AbstractComponent { try { executorService.awaitTermination(5, TimeUnit.MINUTES); } catch (InterruptedException e) { - logger.log(Level.WARNING, "Error shutting down proxy reload thread"); + logger.log(Level.WARNING, "Error shutting down proxy reload thread", e); } } @@ -203,10 +214,12 @@ public class DataplaneProxyService extends AbstractComponent { return template.replaceAll("\\$\\{%s\\}".formatted(key), value); } + // Used for testing NginxState state() { return state; } + // Used for testing NginxState wantedState() { return wantedState; } diff --git a/container-disc/src/test/java/com/yahoo/container/jdisc/DataplaneProxyServiceTest.java b/container-disc/src/test/java/com/yahoo/container/jdisc/DataplaneProxyServiceTest.java index 947c99adf51..351890e2a3a 100644 --- a/container-disc/src/test/java/com/yahoo/container/jdisc/DataplaneProxyServiceTest.java +++ b/container-disc/src/test/java/com/yahoo/container/jdisc/DataplaneProxyServiceTest.java @@ -22,13 +22,16 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class DataplaneProxyServiceTest { private FileSystem fileSystem = Jimfs.newFileSystem(); - DataplaneProxyService.ProxyCommands proxyCommandsMock = Mockito.mock(DataplaneProxyService.ProxyCommands.class); + DataplaneProxyService.ProxyCommands proxyCommandsMock = mock(DataplaneProxyService.ProxyCommands.class); @Test public void starts_and_reloads_if_no_errors() throws IOException { @@ -122,6 +125,35 @@ public class DataplaneProxyServiceTest { assertFalse(proxyCommands.isRunning()); } + @Test + public void stops_executor_when_nginx_stop_throws() throws IOException, InterruptedException { + DataplaneProxyService.ProxyCommands mockProxyCommands = mock(DataplaneProxyService.ProxyCommands.class); + DataplaneProxyService service = dataplaneProxyService(mockProxyCommands); + service.converge(); + when (mockProxyCommands.isRunning()).thenReturn(true); + assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state()); + + reset(proxyCommandsMock); + + when(mockProxyCommands.isRunning()).thenReturn(true).thenReturn(false); + doThrow(new RuntimeException("Failed to stop proxy")).when(proxyCommandsMock).stop(); + Thread thread = new Thread(service::deconstruct);// deconstruct will block until nginx is stopped + thread.start(); + + // Wait for above thread to set the wanted state to STOPPED + while (service.wantedState() != DataplaneProxyService.NginxState.STOPPED) { + try { + Thread.sleep(10); + } catch (InterruptedException e) { + } + } + service.converge(); + assertEquals(service.state(), DataplaneProxyService.NginxState.STOPPED); + thread.join(); + + verify(mockProxyCommands, times(1)).stop(); + } + private DataplaneProxyService dataplaneProxyService(DataplaneProxyService.ProxyCommands proxyCommands) throws IOException { Path root = fileSystem.getPath("/opt/vespa"); -- cgit v1.2.3