Merge pull request #6503 from vespa-engine/freva/fixes

Node-admin fixes

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();