diff options
author | Bjørn Christian Seime <bjorncs@yahooinc.com> | 2023-06-26 09:26:53 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-06-26 09:26:53 +0200 |
commit | 47c69aac8c4209c552f08ff8a1a7f01b5cb647d4 (patch) | |
tree | c2a15607bd700550a89758ff813ab065acdae718 /container-disc | |
parent | 7e8aaa1201ab6fe229547a75b8e1b06de6c96e1a (diff) | |
parent | 0cfcb18a58f9023182349845a215d3119fce43aa (diff) |
Merge pull request #27541 from vespa-engine/mortent/dp-more-robust-proxy
Make stopping data plane proxy more robust
Diffstat (limited to 'container-disc')
-rw-r--r-- | container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java | 45 | ||||
-rw-r--r-- | container-disc/src/test/java/com/yahoo/container/jdisc/DataplaneProxyServiceTest.java | 34 |
2 files changed, 62 insertions, 17 deletions
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..47050168b80 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); } } @@ -150,9 +161,9 @@ public class DataplaneProxyService extends AbstractComponent { super.deconstruct(); wantedState = NginxState.STOPPED; try { - executorService.awaitTermination(5, TimeUnit.MINUTES); + executorService.awaitTermination(30, TimeUnit.SECONDS); } 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"); |