Merge pull request #9865 from vespa-engine/freva/split-http-exception

Fix orchestrator exception bug

6 files changed, 63 insertions, 28 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java
index 73c86fc8de1..59873d7956e 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImpl.java
@@ -116,16 +116,18 @@ public class ConfigServerApiImpl implements ConfigServerApi {
                 if (configServers.size() == 1) break;
 
                 // Failure to communicate with a config server is not abnormal during upgrades
-                if (e.getMessage().contains("(Connection refused)")) {
-                    logger.info("Connection refused to " + configServer + " (upgrading?), will try next");
+                if (ConnectionException.isKnownConnectionException(e)) {
+                    logger.info("Failed to connect to " + configServer + " (upgrading?), will try next: " + e.getMessage());
                 } else {
                     logger.warning("Failed to communicate with " + configServer + ", will try next: " + e.getMessage());
                 }
             }
         }
 
-        throw HttpException.handleException(
-                "All requests against the config servers (" + configServers + ") failed, last as follows:", lastException);
+        String prefix = configServers.size() == 1 ?
+                "Request against " + configServers.get(0) + " failed: " :
+                "All requests against the config servers (" + configServers + ") failed, last as follows: ";
+        throw ConnectionException.handleException(prefix, lastException);
     }
 
     @Override
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConnectionException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConnectionException.java
new file mode 100644
index 00000000000..7e860bfb66b
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/ConnectionException.java
@@ -0,0 +1,43 @@
+// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.node.admin.configserver;
+
+import com.yahoo.vespa.hosted.node.admin.nodeadmin.ConvergenceException;
+import org.apache.http.NoHttpResponseException;
+
+import java.io.EOFException;
+import java.net.SocketException;
+import java.net.SocketTimeoutException;
+
+/**
+ * @author freva
+ */
+@SuppressWarnings("serial")
+public class ConnectionException extends ConvergenceException {
+
+    private ConnectionException(String message) {
+        super(message);
+    }
+
+    /**
+     * Returns {@link ConnectionException} if the given Throwable is of a known and well understood error or
+     * a RuntimeException with the given exception as cause otherwise.
+     */
+    public static RuntimeException handleException(String prefix, Throwable t) {
+        if (isKnownConnectionException(t))
+            return new ConnectionException(prefix + t.getMessage());
+
+        return new RuntimeException(prefix, t);
+    }
+
+    static boolean isKnownConnectionException(Throwable t) {
+        for (; t != null; t = t.getCause()) {
+            if (t instanceof SocketException ||
+                    t instanceof SocketTimeoutException ||
+                    t instanceof NoHttpResponseException ||
+                    t instanceof EOFException)
+                return true;
+        }
+
+        return false;
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java
index 3825107bfa6..a9493d4606e 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java
@@ -2,12 +2,8 @@
 package com.yahoo.vespa.hosted.node.admin.configserver;
 
 import com.yahoo.vespa.hosted.node.admin.nodeadmin.ConvergenceException;
-import org.apache.http.NoHttpResponseException;
 
 import javax.ws.rs.core.Response;
-import java.io.EOFException;
-import java.net.SocketException;
-import java.net.SocketTimeoutException;
 
 /**
  * @author hakonhall
@@ -66,22 +62,6 @@ public class HttpException extends ConvergenceException {
         throw new HttpException(status, message, true);
     }
 
-    /**
-     * Returns {@link HttpException} if the given Throwable is of a known and well understood error or
-     * a RuntimeException with the given exception as cause otherwise.
-     */
-    public static RuntimeException handleException(String prefix, Throwable t) {
-        for (; t != null; t = t.getCause()) {
-            if (t instanceof SocketException ||
-                t instanceof SocketTimeoutException ||
-                t instanceof NoHttpResponseException ||
-                t instanceof EOFException)
-                return new HttpException(prefix + t.getMessage());
-        }
-
-        return new RuntimeException(prefix, t);
-    }
-
     public static class NotFoundException extends HttpException {
         public NotFoundException(String message) {
             super(Response.Status.NOT_FOUND, message, false);
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImpl.java
index 6124e1bdc0e..353abd64778 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/orchestrator/OrchestratorImpl.java
@@ -2,7 +2,9 @@
 package com.yahoo.vespa.hosted.node.admin.configserver.orchestrator;
 
 import com.yahoo.vespa.hosted.node.admin.configserver.ConfigServerApi;
+import com.yahoo.vespa.hosted.node.admin.configserver.ConnectionException;
 import com.yahoo.vespa.hosted.node.admin.configserver.HttpException;
+import com.yahoo.vespa.hosted.node.admin.nodeadmin.ConvergenceException;
 import com.yahoo.vespa.orchestrator.restapi.HostApi;
 import com.yahoo.vespa.orchestrator.restapi.HostSuspensionApi;
 import com.yahoo.vespa.orchestrator.restapi.wire.BatchOperationResult;
@@ -41,6 +43,8 @@ public class OrchestratorImpl implements Orchestrator {
             throw new OrchestratorNotFoundException("Failed to suspend " + hostName + ", host not found");
         } catch (HttpException e) {
             throw new OrchestratorException("Failed to suspend " + hostName + ": " + e.toString());
+        } catch (ConnectionException e) {
+            throw new ConvergenceException("Failed to suspend " + hostName + ": " + e.getMessage());
         } catch (RuntimeException e) {
             throw new RuntimeException("Got error on suspend", e);
         }
@@ -60,6 +64,8 @@ public class OrchestratorImpl implements Orchestrator {
             batchOperationResult = configServerApi.put(url, Optional.empty(), BatchOperationResult.class);
         } catch (HttpException e) {
             throw new OrchestratorException("Failed to batch suspend for " + parentHostName + ": " + e.toString());
+        } catch (ConnectionException e) {
+            throw new ConvergenceException("Failed to batch suspend for " + parentHostName + ": " + e.getMessage());
         } catch (RuntimeException e) {
             throw new RuntimeException("Got error on batch suspend for " + parentHostName + ", with nodes " + hostNames, e);
         }
@@ -78,7 +84,9 @@ public class OrchestratorImpl implements Orchestrator {
         } catch (HttpException.NotFoundException n) {
             throw new OrchestratorNotFoundException("Failed to resume " + hostName + ", host not found");
         } catch (HttpException e) {
-            throw new OrchestratorException("Failed to suspend " + hostName + ": " + e.toString());
+            throw new OrchestratorException("Failed to resume " + hostName + ": " + e.toString());
+        } catch (ConnectionException e) {
+            throw new ConvergenceException("Failed to resume " + hostName + ": " + e.getMessage());
         } catch (RuntimeException e) {
             throw new RuntimeException("Got error on resume", e);
         }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImpl.java
index 2fe8d4b4792..e99a107cfe1 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImpl.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.node.admin.configserver.state;
 
 import com.yahoo.vespa.hosted.node.admin.configserver.ConfigServerApi;
+import com.yahoo.vespa.hosted.node.admin.configserver.ConnectionException;
 import com.yahoo.vespa.hosted.node.admin.configserver.HttpException;
 import com.yahoo.vespa.hosted.node.admin.configserver.state.bindings.HealthResponse;
 
@@ -20,7 +21,7 @@ public class StateImpl implements State {
         try {
             HealthResponse response = configServerApi.get("/state/v1/health", HealthResponse.class);
             return HealthCode.fromString(response.status.code);
-        } catch (HttpException e) {
+        } catch (ConnectionException | HttpException e) {
             return HealthCode.DOWN;
         }
     }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImplTest.java
index 14755ebf9cc..a3256b6955b 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/state/StateImplTest.java
@@ -2,7 +2,7 @@
 package com.yahoo.vespa.hosted.node.admin.configserver.state;
 
 import com.yahoo.vespa.hosted.node.admin.configserver.ConfigServerApi;
-import com.yahoo.vespa.hosted.node.admin.configserver.HttpException;
+import com.yahoo.vespa.hosted.node.admin.configserver.ConnectionException;
 import com.yahoo.vespa.hosted.node.admin.configserver.state.bindings.HealthResponse;
 import org.junit.Test;
 
@@ -29,7 +29,8 @@ public class StateImplTest {
 
     @Test
     public void connectException() {
-        RuntimeException exception = HttpException.handleException("Error: ", new ConnectException("connection refused"));
+        RuntimeException exception =
+                ConnectionException.handleException("Error: ", new ConnectException("connection refused"));
         when(api.get(any(), any())).thenThrow(exception);
 
         HealthCode code = state.getHealth();