Merge pull request #27541 from vespa-engine/mortent/dp-more-robust-proxy

Make stopping data plane proxy more robust

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");