Handle unsuccessful HTTP status codes in node admin

7 files changed, 80 insertions, 24 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java
index 6bfa34388ef..e5e19ff69e4 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java
@@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAttributes;
 import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepository;
 import com.yahoo.vespa.hosted.node.admin.orchestrator.Orchestrator;
 import com.yahoo.vespa.hosted.node.admin.orchestrator.OrchestratorException;
+import com.yahoo.vespa.hosted.node.admin.util.HttpException;
 import com.yahoo.vespa.hosted.provision.Node;
 
 import java.io.IOException;
@@ -178,7 +179,7 @@ public class NodeAdminStateUpdater {
 
         try {
             convergeState(wantedStateCopy);
-        } catch (OrchestratorException | ConvergenceException e) {
+        } catch (OrchestratorException | ConvergenceException | HttpException e) {
             log.info("Unable to converge to " + wantedStateCopy + ": " + e.getMessage());
         } catch (Exception e) {
             log.log(LogLevel.ERROR, "Error while trying to converge to " + wantedStateCopy, e);
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java
index 08957a489b6..8283c90e43d 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java
@@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.node.admin.noderepository.bindings.NodeMessageResp
 import com.yahoo.vespa.hosted.node.admin.noderepository.bindings.UpdateNodeAttributesRequestBody;
 import com.yahoo.vespa.hosted.node.admin.noderepository.bindings.UpdateNodeAttributesResponse;
 import com.yahoo.vespa.hosted.node.admin.util.ConfigServerHttpRequestExecutor;
+import com.yahoo.vespa.hosted.node.admin.util.HttpException;
 import com.yahoo.vespa.hosted.node.admin.util.PrefixLogger;
 import com.yahoo.vespa.hosted.provision.Node;
 
@@ -76,7 +77,7 @@ public class NodeRepositoryImpl implements NodeRepository {
                 return Optional.empty();
             }
             return Optional.of(createContainerNodeSpec(nodeResponse));
-        } catch (ConfigServerHttpRequestExecutor.NotFoundException e) {
+        } catch (HttpException.NotFoundException e) {
             return Optional.empty();
         }
     }
@@ -90,7 +91,7 @@ public class NodeRepositoryImpl implements NodeRepository {
                     .map(node -> new ContainerAclSpec(
                             node.hostname, node.ipAddress, ContainerName.fromHostname(node.trustedBy)))
                     .collect(Collectors.toList());
-        } catch (ConfigServerHttpRequestExecutor.NotFoundException e) {
+        } catch (HttpException.NotFoundException e) {
             return Collections.emptyList();
         }
     }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImpl.java
index bd9df486e7b..d1e996f8e93 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImpl.java
@@ -3,6 +3,7 @@ package com.yahoo.vespa.hosted.node.admin.orchestrator;
 
 import com.yahoo.vespa.hosted.node.admin.util.ConfigServerHttpRequestExecutor;
 
+import com.yahoo.vespa.hosted.node.admin.util.HttpException;
 import com.yahoo.vespa.orchestrator.restapi.HostApi;
 import com.yahoo.vespa.orchestrator.restapi.HostSuspensionApi;
 import com.yahoo.vespa.orchestrator.restapi.wire.BatchHostSuspendRequest;
@@ -40,8 +41,11 @@ public class OrchestratorImpl implements Orchestrator {
                     port,
                     Optional.empty(), /* body */
                     UpdateHostResponse.class);
-        } catch (ConfigServerHttpRequestExecutor.NotFoundException n) {
+        } catch (HttpException.NotFoundException n) {
             throw new OrchestratorNotFoundException("Failed to suspend " + hostName + ", host not found");
+        } catch (HttpException e) {
+            throw new OrchestratorException("Failed to suspend " + hostName + ": " +
+                    e.toString());
         } catch (Exception e) {
             throw new RuntimeException("Got error on suspend", e);
         }
@@ -55,11 +59,14 @@ public class OrchestratorImpl implements Orchestrator {
     public void suspend(String parentHostName, List<String> hostNames) {
         final BatchOperationResult batchOperationResult;
         try {
-             batchOperationResult = requestExecutor.put(
+            batchOperationResult = requestExecutor.put(
                     ORCHESTRATOR_PATH_PREFIX_HOST_SUSPENSION_API,
-                     port,
+                    port,
                     Optional.of(new BatchHostSuspendRequest(parentHostName, hostNames)),
                     BatchOperationResult.class);
+        } catch (HttpException e) {
+            throw new OrchestratorException("Failed to batch suspend for " +
+                    parentHostName + ": " + e.toString());
         } catch (Exception e) {
             throw new RuntimeException("Got error on batch suspend for " + parentHostName + ", with nodes " + hostNames, e);
         }
@@ -75,8 +82,11 @@ public class OrchestratorImpl implements Orchestrator {
         try {
             String path = getSuspendPath(hostName);
             response = requestExecutor.delete(path, port, UpdateHostResponse.class);
-        } catch (ConfigServerHttpRequestExecutor.NotFoundException n) {
+        } 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());
         } catch (Exception e) {
             throw new RuntimeException("Got error on resume", e);
         }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java
index 9c8dc198388..f7d6b86ce69 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java
@@ -17,7 +17,6 @@ import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
 
-import javax.ws.rs.core.Response;
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.util.ArrayList;
@@ -72,13 +71,6 @@ public class ConfigServerHttpRequestExecutor {
         HttpUriRequest createRequest(String configserver) throws JsonProcessingException, UnsupportedEncodingException;
     }
 
-    public class NotFoundException extends RuntimeException {
-        private static final long serialVersionUID = 4791511887L;
-        public NotFoundException(String message) {
-            super(message);
-        }
-    }
-
     private <T> T tryAllConfigServers(CreateRequest requestFactory, Class<T> wantedReturnType) {
         Exception lastException = null;
         for (int loopRetry = 0; loopRetry < MAX_LOOPS; loopRetry++) {
@@ -99,9 +91,9 @@ public class ConfigServerHttpRequestExecutor {
                 }
 
                 try {
-                    if (response.getStatusLine().getStatusCode() == Response.Status.NOT_FOUND.getStatusCode()) {
-                        throw new NotFoundException("Not found returned from " + configServer);
-                    }
+                    HttpException.throwOnFailure(
+                            response.getStatusLine().getStatusCode(),
+                            "Config server " + configServer);
 
                     try {
                         return mapper.readValue(response.getEntity().getContent(), wantedReturnType);
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/HttpException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/HttpException.java
new file mode 100644
index 00000000000..fd7f6308593
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/HttpException.java
@@ -0,0 +1,35 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.node.admin.util;
+
+import javax.ws.rs.core.Response;
+
+@SuppressWarnings("serial")
+public class HttpException extends RuntimeException {
+    public static class NotFoundException extends HttpException {
+        private static final long serialVersionUID = 4791511887L;
+        public NotFoundException(String message) {
+            super(Response.Status.NOT_FOUND, message);
+        }
+    }
+
+    static void throwOnFailure(int statusCode, String message) {
+        Response.Status status = Response.Status.fromStatusCode(statusCode);
+        if (status == null) {
+            throw new HttpException(statusCode, message);
+        }
+
+        if (status == Response.Status.NOT_FOUND) {
+            throw new NotFoundException(message);
+        } else if (status.getFamily() != Response.Status.Family.SUCCESSFUL) {
+            throw new HttpException(status, message);
+        }
+    }
+
+    private HttpException(int statusCode, String message) {
+        super("HTTP status code " + statusCode + ": " + message);
+    }
+
+    private HttpException(Response.Status status, String message) {
+        super(status.toString() + " (" + status.getStatusCode() + "): " + message);
+    }
+}
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImplTest.java
index cef43a058c0..0a11ddd1e62 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImplTest.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.node.admin.orchestrator;
 
 import com.yahoo.vespa.hosted.node.admin.util.ConfigServerHttpRequestExecutor;
+import com.yahoo.vespa.hosted.node.admin.util.HttpException;
 import com.yahoo.vespa.orchestrator.restapi.wire.BatchHostSuspendRequest;
 import com.yahoo.vespa.orchestrator.restapi.wire.BatchOperationResult;
 import com.yahoo.vespa.orchestrator.restapi.wire.HostStateChangeDenialReason;
@@ -12,7 +13,9 @@ import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
 
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 /**
  * @author freva
@@ -56,7 +59,7 @@ public class OrchestratorImplTest {
                 any(Integer.class),
                 any(),
                 any()
-        )).thenThrow(requestExecutor.new NotFoundException("Not Found"));
+        )).thenThrow(new HttpException.NotFoundException("Not Found"));
 
         orchestrator.suspend(hostName);
     }
@@ -102,7 +105,7 @@ public class OrchestratorImplTest {
                 any(String.class),
                 any(Integer.class),
                 any()
-        )).thenThrow(requestExecutor.new NotFoundException("Not Found"));
+        )).thenThrow(new HttpException.NotFoundException("Not Found"));
 
         orchestrator.resume(hostName);
     }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java
index f0d81f2aaf7..e197dd8bc54 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutorTest.java
@@ -78,8 +78,8 @@ public class ConfigServerHttpRequestExecutorTest {
         assertLogStringContainsGETForAHost();
     }
 
-    @Test
-    public void testBasicFailureWithNoRetries() throws Exception {
+    @Test(expected = HttpException.class)
+    public void testBasicFailure() throws Exception {
         Set<String> configServers = new ArraySet<>(2);
         configServers.add("host1");
         configServers.add("host2");
@@ -93,6 +93,20 @@ public class ConfigServerHttpRequestExecutorTest {
     }
 
     @Test
+    public void testBasicSuccessWithNoRetries() throws Exception {
+        Set<String> configServers = new ArraySet<>(2);
+        configServers.add("host1");
+        configServers.add("host2");
+        // Server is returning 201, no retries.
+        mockReturnCode = 201;
+        ConfigServerHttpRequestExecutor executor = new ConfigServerHttpRequestExecutor(configServers, createClientMock());
+
+        TestPojo testPojo = executor.get("/path", 666, TestPojo.class);
+        assertEquals(testPojo.errorCode.intValue(), mockReturnCode);
+        assertLogStringContainsGETForAHost();
+    }
+
+    @Test
     public void testRetries() throws Exception {
         Set<String> configServers = new ArraySet<>(2);
         configServers.add("host1");
@@ -123,7 +137,7 @@ public class ConfigServerHttpRequestExecutorTest {
         try {
             executor.get("/path", 666, TestPojo.class);
             fail("Expected exception");
-        } catch (ConfigServerHttpRequestExecutor.NotFoundException e) {
+        } catch (HttpException.NotFoundException e) {
             // ignore
         }
         assertLogStringContainsGETForAHost();