Fix a TODO in NodeAdmin to simplify tests

3 files changed, 44 insertions, 68 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java
index cc953ae729b..e165b2476f4 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.node.admin.nodeadmin;
 
 import com.yahoo.collections.Pair;
 import com.yahoo.net.HostName;
-import com.yahoo.vespa.hosted.dockerapi.Container;
 import com.yahoo.vespa.hosted.dockerapi.metrics.CounterWrapper;
 import com.yahoo.vespa.hosted.dockerapi.metrics.Dimensions;
 import com.yahoo.vespa.hosted.dockerapi.metrics.GaugeWrapper;
@@ -81,10 +80,15 @@ public class NodeAdminImpl implements NodeAdmin {
 
     @Override
     public void refreshContainersToRun(final List<ContainerNodeSpec> containersToRun) {
-        final List<Container> existingContainers = dockerOperations.getAllManagedContainers();
+        final List<String> existingContainerHostnames = dockerOperations.getAllManagedContainers().stream()
+                .map(container -> container.hostname)
+                .collect(Collectors.toList());
+        final List<String> containersToRunHostnames = containersToRun.stream()
+                .map(container -> container.hostname)
+                .collect(Collectors.toList());
 
         storageMaintainer.ifPresent(StorageMaintainer::cleanNodeAdmin);
-        synchronizeNodeSpecsToNodeAgents(containersToRun, existingContainers);
+        synchronizeNodeSpecsToNodeAgents(containersToRunHostnames, existingContainerHostnames);
         dockerOperations.deleteUnusedDockerImages();
 
         updateNodeAgentMetrics();
@@ -153,7 +157,7 @@ public class NodeAdminImpl implements NodeAdmin {
             boolean metricsSchedulerShutdown = metricsScheduler.awaitTermination(30, TimeUnit.SECONDS);
             boolean aclSchedulerShutdown = aclScheduler.awaitTermination(30, TimeUnit.SECONDS);
             if (! (metricsSchedulerShutdown && aclSchedulerShutdown)) {
-                throw new RuntimeException("Failed shuttingdown all scheduler(s), shutdown status:\n" +
+                throw new RuntimeException("Failed shutting down all scheduler(s), shutdown status:\n" +
                         "\tMetrics Scheduler: " + metricsSchedulerShutdown + "\n" +
                         "\tACL Scheduler: " + aclSchedulerShutdown);
             }
@@ -187,25 +191,20 @@ public class NodeAdminImpl implements NodeAdmin {
                 .map(key -> new Pair<>(Optional.ofNullable(tMap.get(key)), Optional.ofNullable(uMap.get(key))));
     }
 
-    // TODO This method should rather take a list of Hostname instead of Container. However, it triggers
-    // a refactoring of the logic. Which is hard due to the style of programming.
     // The method streams the list of containers twice.
     void synchronizeNodeSpecsToNodeAgents(
-            final List<ContainerNodeSpec> containersToRun,
-            final List<Container> existingContainers) {
-        final Stream<Pair<Optional<ContainerNodeSpec>, Optional<Container>>> nodeSpecContainerPairs = fullOuterJoin(
-                containersToRun.stream(), nodeSpec -> nodeSpec.hostname,
-                existingContainers.stream(), container -> container.hostname);
-
-        final Set<String> nodeHostNames = containersToRun.stream()
-                .map(spec -> spec.hostname)
-                .collect(Collectors.toSet());
-        final Set<String> obsoleteAgentHostNames = diff(nodeAgents.keySet(), nodeHostNames);
+            final List<String> containersToRun,
+            final List<String> existingContainers) {
+        final Stream<Pair<Optional<String>, Optional<String>>> nodeSpecContainerPairs = fullOuterJoin(
+                containersToRun.stream(), hostname -> hostname,
+                existingContainers.stream(), hostname -> hostname);
+
+        final Set<String> obsoleteAgentHostNames = diff(nodeAgents.keySet(), new HashSet<>(containersToRun));
         obsoleteAgentHostNames.forEach(hostName -> nodeAgents.remove(hostName).stop());
 
         nodeSpecContainerPairs.forEach(nodeSpecContainerPair -> {
-            final Optional<ContainerNodeSpec> nodeSpec = nodeSpecContainerPair.getFirst();
-            final Optional<Container> existingContainer = nodeSpecContainerPair.getSecond();
+            final Optional<String> nodeSpec = nodeSpecContainerPair.getFirst();
+            final Optional<String> existingContainer = nodeSpecContainerPair.getSecond();
 
             if (!nodeSpec.isPresent()) {
                 assert existingContainer.isPresent();
@@ -217,14 +216,14 @@ public class NodeAdminImpl implements NodeAdmin {
         });
     }
 
-    private void ensureNodeAgentForNodeIsStarted(final ContainerNodeSpec nodeSpec) {
-        if (nodeAgents.containsKey(nodeSpec.hostname)) {
+    private void ensureNodeAgentForNodeIsStarted(final String hostname) {
+        if (nodeAgents.containsKey(hostname)) {
             return;
         }
 
-        final NodeAgent agent = nodeAgentFactory.apply(nodeSpec.hostname);
+        final NodeAgent agent = nodeAgentFactory.apply(hostname);
         agent.start(nodeAgentScanIntervalMillis);
-        nodeAgents.put(nodeSpec.hostname, agent);
+        nodeAgents.put(hostname, agent);
         try {
             Thread.sleep(1000);
         } catch (InterruptedException 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 ba57bd65470..8832d493a13 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
@@ -76,7 +76,7 @@ public class RestApiHandler extends LoggingRequestHandler{
                 }
             };
         }
-        return new SimpleResponse(400, "unknown path" + path);
+        return new SimpleResponse(400, "unknown path " + path);
     }
 
     private HttpResponse handlePut(HttpRequest request) {
@@ -96,7 +96,7 @@ public class RestApiHandler extends LoggingRequestHandler{
                     new SimpleResponse(200, "ok") :
                     new SimpleResponse(409, "fail");
         }
-        return new SimpleResponse(400, "unknown path" + path);
+        return new SimpleResponse(400, "unknown path " + path);
     }
 
     private static class SimpleResponse extends HttpResponse {
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 3594a16b182..8bd43024556 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
@@ -4,14 +4,9 @@ package com.yahoo.vespa.hosted.node.admin.nodeadmin;
 import com.yahoo.collections.Pair;
 import com.yahoo.metrics.simple.MetricReceiver;
 import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper;
-import com.yahoo.vespa.hosted.node.admin.ContainerNodeSpec;
-import com.yahoo.vespa.hosted.dockerapi.Container;
-import com.yahoo.vespa.hosted.dockerapi.ContainerName;
-import com.yahoo.vespa.hosted.dockerapi.DockerImage;
 import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgent;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentImpl;
-import com.yahoo.vespa.hosted.provision.Node;
 import org.junit.Test;
 import org.mockito.InOrder;
 
@@ -31,6 +26,7 @@ import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyBoolean;
+import static org.mockito.Matchers.anyInt;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
@@ -54,47 +50,41 @@ public class NodeAdminImplTest {
         final NodeAdminImpl nodeAdmin = new NodeAdminImpl(dockerOperations, nodeAgentFactory, Optional.empty(), 100,
                 new MetricReceiverWrapper(MetricReceiver.nullImplementation), Optional.empty());
 
+        final String hostName1 = "host1.test.yahoo.com";
+        final String hostName2 = "host2.test.yahoo.com";
         final NodeAgent nodeAgent1 = mock(NodeAgentImpl.class);
         final NodeAgent nodeAgent2 = mock(NodeAgentImpl.class);
-        when(nodeAgentFactory.apply(any(String.class))).thenReturn(nodeAgent1).thenReturn(nodeAgent2);
-
-        final String hostName = "host1.test.yahoo.com";
-        final DockerImage dockerImage = new DockerImage("image");
-        final ContainerName containerName = new ContainerName("host1");
-        final Container existingContainer = new Container(hostName, dockerImage, containerName, Container.State.RUNNING, 5);
-        final ContainerNodeSpec nodeSpec = new ContainerNodeSpec.Builder()
-                .hostname(hostName)
-                .wantedDockerImage(dockerImage)
-                .nodeState(Node.State.active)
-                .nodeType("tenant")
-                .nodeFlavor("docker")
-                .build();
+        when(nodeAgentFactory.apply(eq(hostName1))).thenReturn(nodeAgent1);
+        when(nodeAgentFactory.apply(eq(hostName2))).thenReturn(nodeAgent2);
+
 
         final InOrder inOrder = inOrder(nodeAgentFactory, nodeAgent1, nodeAgent2);
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.emptyList(), Collections.singletonList(existingContainer));
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.emptyList(), Collections.singletonList(hostName1));
         verifyNoMoreInteractions(nodeAgentFactory);
 
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(nodeSpec), Collections.singletonList(existingContainer));
-        inOrder.verify(nodeAgentFactory).apply(hostName);
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(hostName1), Collections.singletonList(hostName1));
+        inOrder.verify(nodeAgentFactory).apply(hostName1);
         inOrder.verify(nodeAgent1).start(100);
         inOrder.verify(nodeAgent1, never()).stop();
 
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(nodeSpec), Collections.singletonList(existingContainer));
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(hostName1), Collections.singletonList(hostName1));
         inOrder.verify(nodeAgentFactory, never()).apply(any(String.class));
-        inOrder.verify(nodeAgent1, never()).start(1);
+        inOrder.verify(nodeAgent1, never()).start(anyInt());
         inOrder.verify(nodeAgent1, never()).stop();
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.emptyList(), Collections.singletonList(existingContainer));
+
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.emptyList(), Collections.singletonList(hostName1));
         inOrder.verify(nodeAgentFactory, never()).apply(any(String.class));
         verify(nodeAgent1).stop();
 
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(nodeSpec), Collections.singletonList(existingContainer));
-        inOrder.verify(nodeAgentFactory).apply(hostName);
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(hostName2), Collections.singletonList(hostName1));
+        inOrder.verify(nodeAgentFactory).apply(hostName2);
         inOrder.verify(nodeAgent2).start(100);
         inOrder.verify(nodeAgent2, never()).stop();
+        verify(nodeAgent1).stop();
 
         nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.emptyList(), Collections.emptyList());
         inOrder.verify(nodeAgentFactory, never()).apply(any(String.class));
-        inOrder.verify(nodeAgent2, never()).start(1);
+        inOrder.verify(nodeAgent2, never()).start(anyInt());
         inOrder.verify(nodeAgent2).stop();
 
         verifyNoMoreInteractions(nodeAgent1);
@@ -110,30 +100,17 @@ public class NodeAdminImplTest {
                 new MetricReceiverWrapper(MetricReceiver.nullImplementation), Optional.empty());
 
         List<NodeAgent> nodeAgents = new ArrayList<>();
-        List<Container> existingContainers = new ArrayList<>();
-        List<ContainerNodeSpec> nodeSpecs = new ArrayList<>();
-        final DockerImage dockerImage = new DockerImage("image");
+        List<String> existingContainerHostnames = new ArrayList<>();
         for (int i = 0; i < 3; i++) {
             final String hostName = "host" + i + ".test.yahoo.com";
             NodeAgent nodeAgent = mock(NodeAgent.class);
             nodeAgents.add(nodeAgent);
             when(nodeAgentFactory.apply(eq(hostName))).thenReturn(nodeAgent);
 
-            final ContainerName containerName = new ContainerName("host" + i);
-            existingContainers.add(
-                    new Container(hostName, dockerImage, containerName, Container.State.RUNNING, 5));
-
-            nodeSpecs.add(
-                    new ContainerNodeSpec.Builder()
-                            .hostname(hostName)
-                            .wantedDockerImage(dockerImage)
-                            .nodeState(Node.State.active)
-                            .nodeType("tenant")
-                            .nodeFlavor("docker")
-                            .build());
-
+            existingContainerHostnames.add(hostName);
         }
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(nodeSpecs, existingContainers);
+
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(existingContainerHostnames, existingContainerHostnames);
 
         mockNodeAgentSetFrozenResponse(nodeAgents, false, false, false);
         assertFalse(nodeAdmin.setFrozen(true)); // NodeAdmin freezes only when all the NodeAgents are frozen