diff options
author | Valerij Fredriksen <freva@users.noreply.github.com> | 2018-08-06 14:36:31 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-08-06 14:36:31 +0200 |
commit | 9bb8dcdb0429997c7fa634c5f0c5ba109fefc97d (patch) | |
tree | 4e9c0b5241fa0e683da9b26b7b712318c1662878 /node-admin | |
parent | e1544823688b4576b15ab6322ac0be082669b090 (diff) | |
parent | b86361e730c92678df28cd8c565fb518420b5212 (diff) |
Merge pull request #6503 from vespa-engine/freva/fixes
Node-admin fixes
Diffstat (limited to 'node-admin')
9 files changed, 46 insertions, 69 deletions
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/ConfigServerInfo.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/ConfigServerInfo.java index 93243f8b8ed..9e94f6ed7e4 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/ConfigServerInfo.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/ConfigServerInfo.java @@ -1,20 +1,14 @@ // Copyright 2018 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.component; -import com.google.common.base.Strings; -import com.yahoo.vespa.athenz.api.AthenzIdentity; import com.yahoo.vespa.athenz.api.AthenzService; import com.yahoo.vespa.athenz.utils.AthenzIdentities; import com.yahoo.vespa.hosted.node.admin.config.ConfigServerConfig; -import com.yahoo.vespa.hosted.node.admin.util.KeyStoreOptions; import java.net.URI; -import java.nio.file.Paths; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.function.Function; import static java.util.stream.Collectors.toMap; 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 12ba777f018..aea44e728ad 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 @@ -110,41 +110,26 @@ public class ConfigServerApiImpl implements ConfigServerApi { private <T> T tryAllConfigServers(CreateRequest requestFactory, Class<T> wantedReturnType) { Exception lastException = null; for (URI configServer : configServers) { - final CloseableHttpResponse response; - try { - response = client.execute(requestFactory.createRequest(configServer)); - } catch (Exception e) { - // Failure to communicate with a config server is not abnormal, as they are - // upgraded at the same time as Docker hosts. - if (e.getMessage().indexOf("(Connection refused)") > 0) { - NODE_ADMIN_LOGGER.info("Connection refused to " + configServer + " (upgrading?), will try next"); - } else { - NODE_ADMIN_LOGGER.warning("Failed to communicate with " + configServer + ", will try next: " + e.getMessage()); - } - lastException = e; - continue; - } - - try { - Optional<HttpException> retryableException = HttpException.handleStatusCode( - response.getStatusLine().getStatusCode(), - "Config server " + configServer); - if (retryableException.isPresent()) { - lastException = retryableException.get(); - continue; - } + try (CloseableHttpResponse response = client.execute(requestFactory.createRequest(configServer))) { + HttpException.handleStatusCode( + response.getStatusLine().getStatusCode(), "Config server " + configServer); try { return mapper.readValue(response.getEntity().getContent(), wantedReturnType); } catch (IOException e) { - throw new RuntimeException("Response didn't contain nodes element, failed parsing?", e); + throw new RuntimeException("Failed parse response from config server", e); } - } finally { - try { - response.close(); - } catch (IOException e) { - NODE_ADMIN_LOGGER.warning("Ignoring exception from closing response", e); + } catch (HttpException e) { + if (!e.isRetryable()) throw e; + lastException = e; + } catch (Exception e) { + // Failure to communicate with a config server is not abnormal during upgrades + if (e.getMessage().contains("(Connection refused)")) { + NODE_ADMIN_LOGGER.info("Connection refused to " + configServer + " (upgrading?), will try next"); + } else { + NODE_ADMIN_LOGGER.warning("Failed to communicate with " + configServer + ", will try next: " + e.getMessage()); } + lastException = e; } } 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 d0f436d16b6..256fe38ec68 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,7 +2,6 @@ package com.yahoo.vespa.hosted.node.admin.configserver; import javax.ws.rs.core.Response; -import java.util.Optional; /** * @author hakonhall @@ -10,35 +9,34 @@ import java.util.Optional; @SuppressWarnings("serial") public class HttpException extends RuntimeException { - public static class NotFoundException extends HttpException { - - public NotFoundException(String message) { - super(Response.Status.NOT_FOUND, message); - } + private final boolean isRetryable; + private HttpException(int statusCode, String message, boolean isRetryable) { + super("HTTP status code " + statusCode + ": " + message); + this.isRetryable = isRetryable; } - public static class ForbiddenException extends HttpException { - - public ForbiddenException(String message) { - super(Response.Status.FORBIDDEN, message); - } + private HttpException(Response.Status status, String message, boolean isRetryable) { + super(status.toString() + " (" + status.getStatusCode() + "): " + message); + this.isRetryable = isRetryable; + } + public boolean isRetryable() { + return isRetryable; } /** - * Returns empty on success. - * Returns an exception if the error is retriable. - * Throws an exception on a non-retriable error, like 404 Not Found. + * Returns on success. + * @throws HttpException for all non-expected status codes. */ - static Optional<HttpException> handleStatusCode(int statusCode, String message) { + static void handleStatusCode(int statusCode, String message) { Response.Status status = Response.Status.fromStatusCode(statusCode); if (status == null) { - return Optional.of(new HttpException(statusCode, message)); + throw new HttpException(statusCode, message, true); } switch (status.getFamily()) { - case SUCCESSFUL: return Optional.empty(); + case SUCCESSFUL: return; case CLIENT_ERROR: switch (status) { case FORBIDDEN: @@ -48,20 +46,24 @@ public class HttpException extends RuntimeException { case CONFLICT: // A response body is assumed to be present, and // will later be interpreted as an error. - return Optional.empty(); + return; } - throw new HttpException(statusCode, message); + throw new HttpException(status, message, false); } // Other errors like server-side errors are assumed to be retryable. - return Optional.of(new HttpException(status, message)); + throw new HttpException(status, message, true); } - private HttpException(int statusCode, String message) { - super("HTTP status code " + statusCode + ": " + message); + public static class NotFoundException extends HttpException { + public NotFoundException(String message) { + super(Response.Status.NOT_FOUND, message, false); + } } - private HttpException(Response.Status status, String message) { - super(status.toString() + " (" + status.getStatusCode() + "): " + message); + public static class ForbiddenException extends HttpException { + public ForbiddenException(String message) { + super(Response.Status.FORBIDDEN, message, false); + } } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/StoredInteger.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/StoredInteger.java index 2c15912ae58..a815515ac83 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/StoredInteger.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/StoredInteger.java @@ -19,7 +19,7 @@ import java.util.logging.Logger; */ public class StoredInteger implements Supplier<OptionalInt> { - private static Logger logger = Logger.getLogger(StoredInteger.class.getName()); + private static final Logger logger = Logger.getLogger(StoredInteger.class.getName()); private final Path path; private OptionalInt value; diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessException.java index 9f7aaab2060..cbc8ffbf1b7 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessException.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessException.java @@ -65,7 +65,7 @@ public abstract class ChildProcessException extends RuntimeException { if (possiblyHugeOutput.length() <= maxOutputPrefix + maxOutputSuffix + maxOutputSlack) { stringBuilder.append(possiblyHugeOutput); } else { - stringBuilder.append(possiblyHugeOutput.substring(0, maxOutputPrefix)) + stringBuilder.append(possiblyHugeOutput, 0, maxOutputPrefix) .append("... [") .append(possiblyHugeOutput.length() - maxOutputPrefix - maxOutputSuffix) .append(" chars omitted] ...") diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RebootTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RebootTest.java index db14efdd5d2..a1af36f9c21 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RebootTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RebootTest.java @@ -11,10 +11,6 @@ import com.yahoo.vespa.hosted.provision.Node; import org.junit.Ignore; import org.junit.Test; -import java.util.Optional; - -import static org.hamcrest.core.Is.is; -import static org.hamcrest.junit.MatcherAssert.assertThat; import static org.junit.Assert.assertTrue; /** diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java index 91c61623ee7..c348dc4c8b5 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java @@ -25,6 +25,7 @@ import java.util.stream.Collectors; import static java.util.Arrays.asList; import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -152,7 +153,7 @@ public class NodeAdminImplTest { assertTrue(nodeAdmin.isFrozen()); assertTrue(nodeAdmin.subsystemFreezeDuration().isZero()); clock.advance(Duration.ofSeconds(1)); - assertTrue(nodeAdmin.subsystemFreezeDuration().equals(Duration.ofSeconds(1))); + assertEquals(Duration.ofSeconds(1), nodeAdmin.subsystemFreezeDuration()); // Unfreezing floors freeze duration assertTrue(nodeAdmin.setFrozen(false)); // Unfreeze everything @@ -164,7 +165,7 @@ public class NodeAdminImplTest { assertTrue(nodeAdmin.setFrozen(true)); assertTrue(nodeAdmin.subsystemFreezeDuration().isZero()); clock.advance(Duration.ofSeconds(1)); - assertTrue(nodeAdmin.subsystemFreezeDuration().equals(Duration.ofSeconds(1))); + assertEquals(Duration.ofSeconds(1), nodeAdmin.subsystemFreezeDuration()); } @Test diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectoryTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectoryTest.java index a83f3bbe7d4..b714ab539f6 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectoryTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectoryTest.java @@ -25,7 +25,7 @@ public class MakeDirectoryTest { private final FileSystem fileSystem = TestFileSystem.create(); private final TestTaskContext context = new TestTaskContext(); - private String path = "/parent/dir"; + private final String path = "/parent/dir"; private String permissions = "rwxr----x"; private String owner = "test-owner"; private String group = "test-group"; diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLineTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLineTest.java index 5bc45d7540e..a5eb0ab059b 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLineTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLineTest.java @@ -104,7 +104,6 @@ public class CommandLineTest { @Test public void programFails() { - TestChildProcess2 child = new TestChildProcess2(0, ""); terminal.expectCommand("foo 2>&1", 1, ""); try { commandLine.add("foo").execute(); |