From 612d02e428be57f8e91cf4b3aec5c7560a389266 Mon Sep 17 00:00:00 2001 From: Valerij Fredriksen Date: Fri, 21 Jun 2019 10:25:25 +0200 Subject: Fix orchestrator exception bug --- .../admin/configserver/ConfigServerApiImpl.java | 10 +++-- .../configserver/HttpConnectionException.java | 43 ++++++++++++++++++++++ .../node/admin/configserver/HttpException.java | 20 ---------- .../orchestrator/OrchestratorImpl.java | 10 ++++- .../node/admin/configserver/state/StateImpl.java | 3 +- .../admin/configserver/state/StateImplTest.java | 5 ++- 6 files changed, 63 insertions(+), 28 deletions(-) create mode 100644 node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpConnectionException.java (limited to 'node-admin') 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..3a0fffcd534 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 (HttpConnectionException.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 HttpConnectionException.handleException(prefix, lastException); } @Override diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpConnectionException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpConnectionException.java new file mode 100644 index 00000000000..cb4af3b4edc --- /dev/null +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpConnectionException.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 HttpConnectionException extends ConvergenceException { + + private HttpConnectionException(String message) { + super(message); + } + + /** + * Returns {@link HttpConnectionException} 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 HttpConnectionException(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..ea0e4271c16 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.HttpConnectionException; 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 (HttpConnectionException 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 (HttpConnectionException 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 (HttpConnectionException 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..2b8db6fea10 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.HttpConnectionException; 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 (HttpConnectionException | 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..e58e9a9a814 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.HttpConnectionException; 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 = + HttpConnectionException.handleException("Error: ", new ConnectException("connection refused")); when(api.get(any(), any())).thenThrow(exception); HealthCode code = state.getHealth(); -- cgit v1.2.3