Verify proxy running + handle stop

2 files changed, 132 insertions, 42 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 78c89e3a7d4..e6af65c0bc8 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
@@ -35,8 +35,9 @@ public class DataplaneProxyService extends AbstractComponent {
     private final ScheduledThreadPoolExecutor executorService;
     private final Path root;
 
-    enum NginxState {INITIALIZING, RUNNING, RELOAD_REQUIRED};
+    enum NginxState {INITIALIZING, RUNNING, RELOAD_REQUIRED, STOPPED};
     private NginxState state;
+    private NginxState wantedState;
 
     private DataplaneProxyConfig cfg;
     private Path proxyCredentialsCert;
@@ -52,14 +53,14 @@ public class DataplaneProxyService extends AbstractComponent {
         this.root = root;
         this.proxyCommands = proxyCommands;
         changeState(NginxState.INITIALIZING);
+        wantedState = NginxState.RUNNING;
         configTemplate = root.resolve("conf/nginx/nginx.conf.template");
         serverCertificateFile = root.resolve("conf/nginx/server_cert.pem");
         serverKeyFile = root.resolve("conf/nginx/server_key.pem");
         nginxConf = root.resolve("conf/nginx/nginx.conf");
 
         executorService = new ScheduledThreadPoolExecutor(1);
-        executorService.scheduleAtFixedRate(this::startOrReloadNginx, reloadPeriodMinutes, reloadPeriodMinutes, TimeUnit.MINUTES);
-
+        executorService.scheduleAtFixedRate(this::converge, reloadPeriodMinutes, reloadPeriodMinutes, TimeUnit.MINUTES);
     }
 
     public void reconfigure(DataplaneProxyConfig config, DataplaneProxyCredentials credentialsProvider) {
@@ -70,11 +71,11 @@ public class DataplaneProxyService extends AbstractComponent {
         }
     }
 
-    private synchronized void changeState(NginxState newState) {
+    private void changeState(NginxState newState) {
         state = newState;
     }
 
-    void startOrReloadNginx() {
+    void converge() {
         DataplaneProxyConfig config;
         Path proxyCredentialsCert;
         Path proxyCredentialsKey;
@@ -86,14 +87,13 @@ public class DataplaneProxyService extends AbstractComponent {
             this.proxyCredentialsCert = null;
             this.proxyCredentialsKey = null;
         }
-
-        boolean configChanged = false;
         if (config != null) {
             try {
 
                 String serverCert = config.serverCertificate();
                 String serverKey = config.serverKey();
 
+                boolean configChanged = false;
                 configChanged |= writeFile(serverCertificateFile, serverCert);
                 configChanged |= writeFile(serverKeyFile, serverKey);
                 configChanged |= writeFile(nginxConf,
@@ -113,38 +113,47 @@ public class DataplaneProxyService extends AbstractComponent {
                 throw new RuntimeException("Error reconfiguring data plane proxy", e);
             }
         }
-
-        if (state == NginxState.INITIALIZING) {
-            try {
-                proxyCommands.start(nginxConf);
-                changeState(NginxState.RUNNING);
-            } catch (Exception e) {
-                logger.log(Level.INFO, "Failed to start nginx, will retry");
+        if (wantedState == NginxState.RUNNING) {
+            boolean nginxRunning = proxyCommands.isRunning();
+            if (!nginxRunning) {
+                try {
+                    proxyCommands.start(nginxConf);
+                    changeState(wantedState);
+                } catch (Exception e) {
+                    logger.log(Level.INFO, "Failed to start nginx, will retry");
+                }
+            } 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(NginxState.RUNNING);
-            } catch (Exception e) {
-                logger.log(Level.INFO, "Failed to reconfigure nginx, will retry.");
+        } else if (wantedState == 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");
+                }
             }
+        } else {
+            logger.warning("Unknown state " + wantedState);
         }
     }
 
     @Override
     public void deconstruct() {
         super.deconstruct();
+        wantedState = NginxState.STOPPED;
         try {
-            executorService.shutdownNow();
-            executorService.awaitTermination(1, TimeUnit.MINUTES);
+            executorService.awaitTermination(5, TimeUnit.MINUTES);
         } catch (InterruptedException e) {
             logger.log(Level.WARNING, "Error shutting down proxy reload thread");
         }
-        try {
-            proxyCommands.stop();
-        } catch (Exception e) {
-            logger.log(Level.WARNING, "Failed shutting down nginx");
-        }
     }
 
     /*
@@ -153,7 +162,6 @@ public class DataplaneProxyService extends AbstractComponent {
      */
     private boolean writeFile(Path file, String contents) throws IOException {
         Path tempPath = file.getParent().resolve(file.getFileName().toString() + ".new");
-//        Path tempPath = Paths.get(file.toFile().getAbsolutePath() + ".new");
         Files.createDirectories(tempPath.getParent());
         Files.writeString(tempPath, contents);
 
@@ -199,10 +207,15 @@ public class DataplaneProxyService extends AbstractComponent {
         return state;
     }
 
+    NginxState wantedState() {
+        return wantedState;
+    }
+
     public interface ProxyCommands {
         void start(Path configFile);
         void stop();
         void reload();
+        boolean isRunning();
     }
 
     public static class NginxProxyCommands implements ProxyCommands {
@@ -255,5 +268,12 @@ public class DataplaneProxyService extends AbstractComponent {
                 throw new RuntimeException("Could not start nginx", e);
             }
         }
+
+        @Override
+        public boolean isRunning() {
+            return ProcessHandle.allProcesses()
+                    .map(ProcessHandle::info)
+                    .anyMatch(info -> info.command().orElse("").endsWith("nginx"));
+        }
     }
 }
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 b71dfbba88a..947c99adf51 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
@@ -19,61 +19,107 @@ import java.time.Duration;
 
 import static com.yahoo.yolean.Exceptions.uncheck;
 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.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 public class DataplaneProxyServiceTest {
     private FileSystem fileSystem = Jimfs.newFileSystem();
-    DataplaneProxyService.ProxyCommands proxyCommands = Mockito.mock(DataplaneProxyService.ProxyCommands.class);
+    DataplaneProxyService.ProxyCommands proxyCommandsMock = Mockito.mock(DataplaneProxyService.ProxyCommands.class);
 
     @Test
     public void starts_and_reloads_if_no_errors() throws IOException {
-        DataplaneProxyService service = dataplaneProxyService(proxyCommands);
+        DataplaneProxyService service = dataplaneProxyService(proxyCommandsMock);
 
         assertEquals(DataplaneProxyService.NginxState.INITIALIZING, service.state());
         service.reconfigure(proxyConfig(), credentials(fileSystem));
 
         // Simulate executor next tick
-        service.startOrReloadNginx();
+        service.converge();
         assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
 
         // Trigger reload by recreating the proxy config (generates new server cert)
         service.reconfigure(proxyConfig(), credentials(fileSystem));
-        service.startOrReloadNginx();
+        service.converge();
         assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
     }
 
     @Test
     public void retries_startup_errors() throws IOException {
-        Mockito.doThrow(new RuntimeException("IO error")).doNothing().when(proxyCommands).start(any());
-        DataplaneProxyService service = dataplaneProxyService(proxyCommands);
+        Mockito.doThrow(new RuntimeException("IO error")).doNothing().when(proxyCommandsMock).start(any());
+        DataplaneProxyService service = dataplaneProxyService(proxyCommandsMock);
 
         assertEquals(DataplaneProxyService.NginxState.INITIALIZING, service.state());
         service.reconfigure(proxyConfig(), credentials(fileSystem));
 
-        // Start nginx,
-        service.startOrReloadNginx();
+        // Start nginx, starting will fail, so the service should be in INITIALIZING state
+        service.converge();
         assertEquals(DataplaneProxyService.NginxState.INITIALIZING, service.state());
-        service.startOrReloadNginx();
+        service.converge();
         assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
     }
 
     @Test
     public void retries_reload_errors() throws IOException {
-        Mockito.doThrow(new RuntimeException("IO error")).doNothing().when(proxyCommands).reload();
-        DataplaneProxyService service = dataplaneProxyService(proxyCommands);
+        Mockito.doThrow(new RuntimeException("IO error")).doNothing().when(proxyCommandsMock).reload();
+        when(proxyCommandsMock.isRunning()).thenReturn(false);
+        DataplaneProxyService service = dataplaneProxyService(proxyCommandsMock);
 
         // Make sure service in running state
         service.reconfigure(proxyConfig(), credentials(fileSystem));
-        service.startOrReloadNginx();
+        service.converge();
         assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+        when(proxyCommandsMock.isRunning()).thenReturn(true);
 
         // Trigger reload, verifies 2nd attempt succeeds
         service.reconfigure(proxyConfig(), credentials(fileSystem));
-        service.startOrReloadNginx();
+        service.converge();
         assertEquals(DataplaneProxyService.NginxState.RELOAD_REQUIRED, service.state());
-        service.startOrReloadNginx();
+        service.converge();
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+        verify(proxyCommandsMock, times(2)).reload();
+    }
+
+    @Test
+    public void converges_to_wanted_state_when_nginx_not_running() throws IOException {
+        DataplaneProxyService.ProxyCommands proxyCommands = new TestProxyCommands();
+        DataplaneProxyService service = dataplaneProxyService(proxyCommands);
+
+        assertFalse(proxyCommands.isRunning());
+        service.reconfigure(proxyConfig(), credentials(fileSystem));
+        service.converge();
         assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+        assertTrue(proxyCommands.isRunning());
 
+        // Simulate nginx process dying
+        proxyCommands.stop();
+        assertFalse(proxyCommands.isRunning());
+        service.converge();
+        assertTrue(proxyCommands.isRunning());
+    }
+
+    @Test
+    public void shuts_down() throws IOException {
+        DataplaneProxyService.ProxyCommands proxyCommands = new TestProxyCommands();
+        DataplaneProxyService service = dataplaneProxyService(proxyCommands);
+        service.converge();
+        assertTrue(proxyCommands.isRunning());
+        assertEquals(DataplaneProxyService.NginxState.RUNNING, service.state());
+
+        new Thread(service::deconstruct).start(); // deconstruct will block until nginx is stopped
+        // 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);
+        assertFalse(proxyCommands.isRunning());
     }
 
     private DataplaneProxyService dataplaneProxyService(DataplaneProxyService.ProxyCommands proxyCommands) throws IOException {
@@ -101,4 +147,28 @@ public class DataplaneProxyServiceTest {
         uncheck(() -> Files.createDirectories(path));
         return new DataplaneProxyCredentials(path.resolve("cert.pem"), path.resolve("key.pem"));
     }
+
+    private static class TestProxyCommands implements DataplaneProxyService.ProxyCommands {
+        private boolean running = false;
+
+        @Override
+        public void start(Path configFile) {
+            running = true;
+        }
+
+        @Override
+        public void stop() {
+            running = false;
+        }
+
+        @Override
+        public void reload() {
+
+        }
+
+        @Override
+        public boolean isRunning() {
+            return running;
+        }
+    }
 }