Code review based improvements.

10 files changed, 31 insertions, 32 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java
index 147f7cdab9f..863a8f47965 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java
@@ -11,7 +11,7 @@ import java.util.Set;
  */
 public interface NodeAdmin {
 
-    void setState(final List<ContainerNodeSpec> containersToRun);
+    void refreshContainersToRun(final List<ContainerNodeSpec> containersToRun);
 
     boolean freezeAndCheckIfAllFrozen();
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java
index ce59c19ec5a..f9e131e3d6c 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java
@@ -48,7 +48,7 @@ public class NodeAdminImpl implements NodeAdmin {
         this.nodeAgentFactory = nodeAgentFactory;
     }
 
-    public void setState(final List<ContainerNodeSpec> containersToRun) {
+    public void refreshContainersToRun(final List<ContainerNodeSpec> containersToRun) {
         final List<Container> existingContainers = docker.getAllManagedContainers();
 
         synchronizeLocalContainerState(containersToRun, existingContainers);
@@ -153,7 +153,7 @@ public class NodeAdminImpl implements NodeAdmin {
                 .map(spec -> spec.hostname)
                 .collect(Collectors.toSet());
         final Set<HostName> obsoleteAgentHostNames = diff(nodeAgents.keySet(), nodeHostNames);
-        obsoleteAgentHostNames.forEach(hostName -> nodeAgents.remove(hostName).terminate());
+        obsoleteAgentHostNames.forEach(hostName -> nodeAgents.remove(hostName).stop());
 
         nodeSpecContainerPairs.forEach(nodeSpecContainerPair -> {
             final Optional<ContainerNodeSpec> nodeSpec = nodeSpecContainerPair.getFirst();
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java
index 97bcdfa724a..191da411f02 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java
@@ -1,31 +1,22 @@
 // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.node.admin;
 
-import com.google.inject.Inject;
 import com.yahoo.component.AbstractComponent;
-import com.yahoo.vespa.applicationmodel.HostName;
-import com.yahoo.vespa.hosted.node.admin.docker.Docker;
 import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepository;
-import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepositoryImpl;
 import com.yahoo.vespa.hosted.node.admin.orchestrator.Orchestrator;
-import com.yahoo.vespa.hosted.node.admin.orchestrator.OrchestratorImpl;
-import com.yahoo.vespa.hosted.node.admin.util.Environment;
 
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
-import java.util.Set;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
-import java.util.function.Function;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import static com.yahoo.vespa.hosted.node.admin.NodeAdminStateUpdater.State.RESUMED;
 import static com.yahoo.vespa.hosted.node.admin.NodeAdminStateUpdater.State.SUSPENDED;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
-import static java.util.concurrent.TimeUnit.SECONDS;
 
 /**
  * Pulls information from node repository and forwards containers to run to node admin.
@@ -113,7 +104,7 @@ public class NodeAdminStateUpdater extends AbstractComponent {
                 return;
             }
             try {
-                nodeAdmin.setState(containersToRun);
+                nodeAdmin.refreshContainersToRun(containersToRun);
             } catch (Throwable t) {
                 log.log(Level.WARNING, "Failed updating node admin: ", t);
                 return;
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgent.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgent.java
index eac1b228378..4d5e19e4bce 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgent.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgent.java
@@ -11,7 +11,7 @@ package com.yahoo.vespa.hosted.node.admin;
 public interface NodeAgent {
 
     enum Command {UPDATE_FROM_NODE_REPO, FREEZE, UNFREEZE}
-    enum State {WAITING, WORKING, DIRTY, FROZEN, TERMINATED}
+    enum State {WAITING, WORKING, FROZEN, TERMINATED}
 
     /**
      * Signals to the agent that it should update the node specification and container state and maintain wanted state.
@@ -40,5 +40,5 @@ public interface NodeAgent {
      * Cleans up any resources the agent owns, such as threads, connections etc. Cleanup is synchronous; when this
      * method returns, no more actions will be taken by the agent.
      */
-    void terminate();
+    void stop();
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java
index d4af21900c3..aa7e76780ab 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java
@@ -105,18 +105,18 @@ public class NodeAgentImpl implements NodeAgent {
         logger.log(LogLevel.INFO, logPrefix + "Scheduling start of NodeAgent");
         synchronized (monitor) {
             if (state == State.TERMINATED) {
-                throw new IllegalStateException("Cannot re-start a stopped node agent");
+                throw new IllegalStateException("Cannot re-start a stopped (terminated) node agent");
             }
         }
         thread.start();
     }
 
     @Override
-    public void terminate() {
+    public void stop() {
         logger.log(LogLevel.INFO, logPrefix + "Scheduling stop of NodeAgent");
         synchronized (monitor) {
             if (state == State.TERMINATED) {
-                throw new IllegalStateException("Cannot stop an already stopped node agent");
+                throw new IllegalStateException("Cannot stop an already stopped (terminated) node agent");
             }
             wantedState = State.TERMINATED;
         }
@@ -400,7 +400,7 @@ public class NodeAgentImpl implements NodeAgent {
                     }
                 }
             } catch (InterruptedException e) {
-                throw new RuntimeException(e);
+                logger.severe("NodeAgent thread interrupted. Ignoring this: " + e.getMessage());
             }
     }
 
@@ -429,7 +429,8 @@ public class NodeAgentImpl implements NodeAgent {
                                 new IllegalStateException(String.format("Node '%s' missing from node repository.", hostname)));
                 final Optional<Container> existingContainer = docker.getContainer(hostname);
                 synchronizeLocalContainerState(nodeSpec, existingContainer);
-            } catch (Exception e) {
+            } catch (Throwable e) {
+                // TODO: We should probably halt the VM at this stage so the containers survive and NodeAdmin get into a fresh state.
                 logger.log(LogLevel.ERROR, logPrefix + "Unhandled exception.", e);
             }
         }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java
index 06fb9f63148..aea9905ca5c 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java
@@ -1,5 +1,7 @@
 package com.yahoo.vespa.hosted.node.admin.restapi;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.container.jdisc.HttpResponse;
 import com.yahoo.container.jdisc.LoggingRequestHandler;
@@ -29,6 +31,7 @@ import static com.yahoo.jdisc.http.HttpRequest.Method.PUT;
 public class RestApiHandler extends LoggingRequestHandler{
 
     private final NodeAdminStateUpdater refresher;
+    private final static ObjectMapper objectMapper = new ObjectMapper();
 
     public RestApiHandler(Executor executor, AccessLog accessLog, ComponentsProvider componentsProvider) {
         super(executor, accessLog);
@@ -48,7 +51,11 @@ public class RestApiHandler extends LoggingRequestHandler{
     }
 
     private HttpResponse handleGet(HttpRequest request) {
-        return new SimpleResponse(200, refresher.getDebugPage());
+        String path = request.getUri().getPath();
+        if (path.endsWith("info")) {
+            return new SimpleResponse(200, refresher.getDebugPage());
+        }
+        return new SimpleResponse(400, "unknown path" + path);
     }
 
     private HttpResponse handlePut(HttpRequest request) {
@@ -77,8 +84,9 @@ public class RestApiHandler extends LoggingRequestHandler{
 
         public SimpleResponse(int code, String message) {
             super(code);
-            // TODO: Use some library to build json as this easily fails
-            this.jsonMessage = "{ \"jsonMessage\":\"" + message + "\"}";
+            ObjectNode objectNode = objectMapper.createObjectNode();
+            objectNode.put("jsonMessage", message);
+            this.jsonMessage = objectNode.toString();
         }
 
         @Override
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImplTest.java
index f3bfeb47e9f..737dd25092a 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImplTest.java
@@ -77,30 +77,30 @@ public class NodeAdminImplTest {
         inOrder.verify(nodeAgentFactory).apply(hostName);
         inOrder.verify(nodeAgent1).start();
         inOrder.verify(nodeAgent1).execute(NodeAgent.Command.UPDATE_FROM_NODE_REPO);
-        inOrder.verify(nodeAgent1, never()).terminate();
+        inOrder.verify(nodeAgent1, never()).stop();
 
         nodeAdmin.synchronizeLocalContainerState(asList(nodeSpec), asList(existingContainer));
         inOrder.verify(nodeAgentFactory, never()).apply(any(HostName.class));
         inOrder.verify(nodeAgent1, never()).start();
         inOrder.verify(nodeAgent1).execute(NodeAgent.Command.UPDATE_FROM_NODE_REPO);
-        inOrder.verify(nodeAgent1, never()).terminate();
+        inOrder.verify(nodeAgent1, never()).stop();
 
         nodeAdmin.synchronizeLocalContainerState(Collections.emptyList(), asList(existingContainer));
         inOrder.verify(nodeAgentFactory, never()).apply(any(HostName.class));
         inOrder.verify(nodeAgent1, never()).execute(NodeAgent.Command.UPDATE_FROM_NODE_REPO);
-        verify(nodeAgent1).terminate();
+        verify(nodeAgent1).stop();
 
         nodeAdmin.synchronizeLocalContainerState(asList(nodeSpec), asList(existingContainer));
         inOrder.verify(nodeAgentFactory).apply(hostName);
         inOrder.verify(nodeAgent2).start();
         inOrder.verify(nodeAgent2).execute(NodeAgent.Command.UPDATE_FROM_NODE_REPO);
-        inOrder.verify(nodeAgent2, never()).terminate();
+        inOrder.verify(nodeAgent2, never()).stop();
 
         nodeAdmin.synchronizeLocalContainerState(Collections.emptyList(), Collections.emptyList());
         inOrder.verify(nodeAgentFactory, never()).apply(any(HostName.class));
         inOrder.verify(nodeAgent2, never()).start();
         inOrder.verify(nodeAgent2, never()).execute(NodeAgent.Command.UPDATE_FROM_NODE_REPO);
-        inOrder.verify(nodeAgent2).terminate();
+        inOrder.verify(nodeAgent2).stop();
 
         verifyNoMoreInteractions(nodeAgent1);
         verifyNoMoreInteractions(nodeAgent2);
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java
index f9d9ffdf0be..2b8517ab33b 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java
@@ -6,7 +6,6 @@ import com.yahoo.vespa.hosted.node.admin.docker.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.integrationTests.OrchestratorMock;
 import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepository;
 import com.yahoo.vespa.hosted.node.admin.noderepository.NodeState;
-import com.yahoo.vespa.hosted.node.admin.orchestrator.Orchestrator;
 import org.junit.Test;
 import org.mockito.stubbing.Answer;
 
@@ -45,7 +44,7 @@ public class NodeAdminStateUpdaterTest {
                 throw new RuleBaseException("This exception is expected, and should show up in the log.");
             }
             return null;
-        }).when(nodeAdmin).setState(anyList());
+        }).when(nodeAdmin).refreshContainersToRun(anyList());
 
         final List<ContainerNodeSpec> containersToRun = new ArrayList<>();
         containersToRun.add(createSample());
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java
index 4043195ea5e..0f042162de3 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java
@@ -27,7 +27,7 @@ public class NodeAdminMock implements NodeAdmin {
     private Object monitor = new Object();
 
     @Override
-    public void setState(List<ContainerNodeSpec> containersToRun) {
+    public void refreshContainersToRun(List<ContainerNodeSpec> containersToRun) {
         synchronized (monitor) {
             hostnames.clear();
             containersToRun.forEach(container -> hostnames.add(container.hostname));
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java
index b704e4624b7..21862f4598f 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java
@@ -107,7 +107,7 @@ public class RunInContainerTest {
         waitForJdiscContainerToServe();
         assertThat(doPutCall("resume"), is(true));
         assertThat(doPutCall("suspend"), is(false));
-        assertThat(doGetInfoCall(), is("{ \"jsonMessage\":\"isRunningUpdates is false. NodeAdmin:  " +
+        assertThat(doGetInfoCall(), is("{\"jsonMessage\":\"isRunningUpdates is false. NodeAdmin:  " +
                 "Unfreeze called while in state false " +
                 "Freeze called while in state false\"}"));
     }