From 6947c6b14eebf7f95a29360f157cecf76ea13dc1 Mon Sep 17 00:00:00 2001 From: Håkon Hallingstad Date: Tue, 3 Oct 2017 17:07:35 +0200 Subject: Handle unsuccessful HTTP status codes in node admin --- .../admin/nodeadmin/NodeAdminStateUpdater.java | 3 +- .../admin/noderepository/NodeRepositoryImpl.java | 5 ++-- .../node/admin/orchestrator/OrchestratorImpl.java | 18 ++++++++--- .../util/ConfigServerHttpRequestExecutor.java | 14 ++------- .../hosted/node/admin/util/HttpException.java | 35 ++++++++++++++++++++++ .../admin/orchestrator/OrchestratorImplTest.java | 9 ++++-- .../util/ConfigServerHttpRequestExecutorTest.java | 20 +++++++++++-- 7 files changed, 80 insertions(+), 24 deletions(-) create mode 100644 node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/HttpException.java 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 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 tryAllConfigServers(CreateRequest requestFactory, Class 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 configServers = new ArraySet<>(2); configServers.add("host1"); configServers.add("host2"); @@ -92,6 +92,20 @@ public class ConfigServerHttpRequestExecutorTest { assertLogStringContainsGETForAHost(); } + @Test + public void testBasicSuccessWithNoRetries() throws Exception { + Set 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 configServers = new ArraySet<>(2); @@ -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(); -- cgit v1.2.3