Use listContainers in NodeAdminImpl

7 files changed, 48 insertions, 29 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java
index a012de36b62..9e618d42000 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java
@@ -45,5 +45,7 @@ public interface DockerOperations {
      */
     List<Container> getAllManagedContainers();
 
+    List<ContainerName> listAllManagedContainers();
+
     void deleteUnusedDockerImages();
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java
index c2855da85a0..ddd18dc88d0 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java
@@ -314,6 +314,11 @@ public class DockerOperationsImpl implements DockerOperations {
     }
 
     @Override
+    public List<ContainerName> listAllManagedContainers() {
+        return docker.listAllContainersManagedBy(MANAGER_NAME);
+    }
+
+    @Override
     public void deleteUnusedDockerImages() {
         docker.deleteUnusedDockerImages();
     }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java
index 00e0568100d..2a5942ccd53 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/acl/AclMaintainer.java
@@ -78,14 +78,14 @@ public class AclMaintainer implements Runnable {
     }
 
     private synchronized void configureAcls() {
-        final Map<ContainerName, List<ContainerAclSpec>> aclSpecsGroupedByHostname = nodeRepository
+        final Map<ContainerName, List<ContainerAclSpec>> aclSpecsGroupedByContainerName = nodeRepository
                 .getContainerAclSpecs(nodeAdminHostname).stream()
                 .collect(Collectors.groupingBy(ContainerAclSpec::trustedBy));
 
         dockerOperations
                 .getAllManagedContainers().stream()
                 .filter(container -> container.state.isRunning())
-                .map(container -> new Pair<>(container, aclSpecsGroupedByHostname.get(container.name)))
+                .map(container -> new Pair<>(container, aclSpecsGroupedByContainerName.get(container.name)))
                 .filter(pair -> pair.getSecond() != null)
                 .forEach(pair ->
                         applyAcl(pair.getFirst().name, new Acl(pair.getFirst().pid, pair.getSecond())));
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdmin.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdmin.java
index 77f5fe3bf38..ab299b90af9 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdmin.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdmin.java
@@ -1,6 +1,7 @@
 // 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.nodeadmin;
 
+import com.yahoo.vespa.hosted.dockerapi.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.ContainerNodeSpec;
 
 import java.util.List;
@@ -43,7 +44,7 @@ public interface NodeAdmin {
     /**
      * Returns list of hosts.
      */
-    Set<String> getListOfHosts();
+    Set<ContainerName> getListOfHosts();
 
     /**
      * Returns a map containing all relevant NodeAdmin variables and their current values.
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 a64e7e405d1..1171e81ab28 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
@@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.node.admin.nodeadmin;
 import com.yahoo.collections.Pair;
 import com.yahoo.concurrent.ThreadFactoryFactory;
 import com.yahoo.net.HostName;
+import com.yahoo.vespa.hosted.dockerapi.ContainerName;
 import com.yahoo.vespa.hosted.dockerapi.metrics.CounterWrapper;
 import com.yahoo.vespa.hosted.dockerapi.metrics.Dimensions;
 import com.yahoo.vespa.hosted.dockerapi.metrics.GaugeWrapper;
@@ -46,7 +47,7 @@ public class NodeAdminImpl implements NodeAdmin {
     private final Optional<StorageMaintainer> storageMaintainer;
     private boolean isFrozen = true;
 
-    private final Map<String, NodeAgent> nodeAgents = new ConcurrentHashMap<>();
+    private final Map<ContainerName, NodeAgent> nodeAgents = new ConcurrentHashMap<>();
 
     private final int nodeAgentScanIntervalMillis;
 
@@ -83,15 +84,13 @@ public class NodeAdminImpl implements NodeAdmin {
 
     @Override
     public void refreshContainersToRun(final List<ContainerNodeSpec> containersToRun) {
-        final List<String> existingContainerHostnames = dockerOperations.getAllManagedContainers().stream()
-                .map(container -> container.hostname)
-                .collect(Collectors.toList());
+        final List<ContainerName> existingContainerNames = dockerOperations.listAllManagedContainers();
         final List<String> containersToRunHostnames = containersToRun.stream()
                 .map(container -> container.hostname)
                 .collect(Collectors.toList());
 
         storageMaintainer.ifPresent(StorageMaintainer::cleanNodeAdmin);
-        synchronizeNodeSpecsToNodeAgents(containersToRunHostnames, existingContainerHostnames);
+        synchronizeNodeSpecsToNodeAgents(containersToRunHostnames, existingContainerNames);
         dockerOperations.deleteUnusedDockerImages();
 
         updateNodeAgentMetrics();
@@ -137,7 +136,7 @@ public class NodeAdminImpl implements NodeAdmin {
     }
 
     @Override
-    public Set<String> getListOfHosts() {
+    public Set<ContainerName> getListOfHosts() {
         return nodeAgents.keySet();
     }
 
@@ -196,37 +195,39 @@ public class NodeAdminImpl implements NodeAdmin {
 
     // The method streams the list of containers twice.
     void synchronizeNodeSpecsToNodeAgents(
-            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 List<String> hostnamesToRun,
+            final List<ContainerName> existingContainers) {
+        final Map<ContainerName, String> hostnameByContainerName = hostnamesToRun.stream()
+                .collect(Collectors.toMap(ContainerName::fromHostname, i -> i));
+        final Stream<Pair<Optional<ContainerName>, Optional<ContainerName>>> nodeSpecContainerPairs = fullOuterJoin(
+                hostnameByContainerName.keySet().stream(), containerName -> containerName,
+                existingContainers.stream(), containerName -> containerName);
 
-        final Set<String> obsoleteAgentHostNames = diff(nodeAgents.keySet(), new HashSet<>(containersToRun));
-        obsoleteAgentHostNames.forEach(hostName -> nodeAgents.remove(hostName).stop());
+        final Set<ContainerName> obsoleteAgentContainerNames = diff(nodeAgents.keySet(), new HashSet<>(hostnameByContainerName.keySet()));
+        obsoleteAgentContainerNames.forEach(containerName -> nodeAgents.remove(containerName).stop());
 
         nodeSpecContainerPairs.forEach(nodeSpecContainerPair -> {
-            final Optional<String> nodeSpec = nodeSpecContainerPair.getFirst();
-            final Optional<String> existingContainer = nodeSpecContainerPair.getSecond();
+            final Optional<ContainerName> nodeSpec = nodeSpecContainerPair.getFirst();
+            final Optional<ContainerName> existingContainer = nodeSpecContainerPair.getSecond();
 
             if (!nodeSpec.isPresent()) {
                 assert existingContainer.isPresent();
-                logger.warning("Container " + existingContainer.get() + " exists, but is not in node repository runlist");
+                logger.warning("Container " + existingContainer.get().asString() + " exists, but is not in node repository runlist");
                 return;
             }
 
-            ensureNodeAgentForNodeIsStarted(nodeSpec.get());
+            ensureNodeAgentForNodeIsStarted(nodeSpec.get(), hostnameByContainerName.get(nodeSpec.get()));
         });
     }
 
-    private void ensureNodeAgentForNodeIsStarted(final String hostname) {
-        if (nodeAgents.containsKey(hostname)) {
+    private void ensureNodeAgentForNodeIsStarted(ContainerName containerName, String hostname) {
+        if (nodeAgents.containsKey(containerName)) {
             return;
         }
 
         final NodeAgent agent = nodeAgentFactory.apply(hostname);
         agent.start(nodeAgentScanIntervalMillis);
-        nodeAgents.put(hostname, agent);
+        nodeAgents.put(containerName, agent);
         try {
             Thread.sleep(1000);
         } catch (InterruptedException e) {
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java
index 67c2de924b3..356bbe2ef58 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java
@@ -64,6 +64,13 @@ public class DockerMock implements Docker {
     }
 
     @Override
+    public List<ContainerName> listAllContainersManagedBy(String manager) {
+        synchronized (monitor) {
+            return containers.stream().map(container -> container.name).collect(Collectors.toList());
+        }
+    }
+
+    @Override
     public Optional<ContainerStats> getContainerStats(ContainerName containerName) {
         return Optional.empty();
     }
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 7dc3d0e8ac2..a539ba67e64 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
@@ -3,6 +3,7 @@ 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.ContainerName;
 import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper;
 import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgent;
@@ -52,6 +53,7 @@ public class NodeAdminImplTest {
 
         final String hostName1 = "host1.test.yahoo.com";
         final String hostName2 = "host2.test.yahoo.com";
+        final ContainerName containerName1 = ContainerName.fromHostname(hostName1);
         final NodeAgent nodeAgent1 = mock(NodeAgentImpl.class);
         final NodeAgent nodeAgent2 = mock(NodeAgentImpl.class);
         when(nodeAgentFactory.apply(eq(hostName1))).thenReturn(nodeAgent1);
@@ -59,24 +61,24 @@ public class NodeAdminImplTest {
 
 
         final InOrder inOrder = inOrder(nodeAgentFactory, nodeAgent1, nodeAgent2);
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.emptyList(), Collections.singletonList(hostName1));
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.emptyList(), Collections.singletonList(containerName1));
         verifyNoMoreInteractions(nodeAgentFactory);
 
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(hostName1), Collections.singletonList(hostName1));
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(hostName1), Collections.singletonList(containerName1));
         inOrder.verify(nodeAgentFactory).apply(hostName1);
         inOrder.verify(nodeAgent1).start(100);
         inOrder.verify(nodeAgent1, never()).stop();
 
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(hostName1), Collections.singletonList(hostName1));
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(hostName1), Collections.singletonList(containerName1));
         inOrder.verify(nodeAgentFactory, never()).apply(any(String.class));
         inOrder.verify(nodeAgent1, never()).start(anyInt());
         inOrder.verify(nodeAgent1, never()).stop();
 
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.emptyList(), Collections.singletonList(hostName1));
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.emptyList(), Collections.singletonList(containerName1));
         inOrder.verify(nodeAgentFactory, never()).apply(any(String.class));
         verify(nodeAgent1).stop();
 
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(hostName2), Collections.singletonList(hostName1));
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(Collections.singletonList(hostName2), Collections.singletonList(containerName1));
         inOrder.verify(nodeAgentFactory).apply(hostName2);
         inOrder.verify(nodeAgent2).start(100);
         inOrder.verify(nodeAgent2, never()).stop();
@@ -110,7 +112,8 @@ public class NodeAdminImplTest {
             existingContainerHostnames.add(hostName);
         }
 
-        nodeAdmin.synchronizeNodeSpecsToNodeAgents(existingContainerHostnames, existingContainerHostnames);
+        nodeAdmin.synchronizeNodeSpecsToNodeAgents(existingContainerHostnames,
+                existingContainerHostnames.stream().map(ContainerName::fromHostname).collect(Collectors.toList()));
 
         assertTrue(nodeAdmin.isFrozen()); // Initially everything is frozen to force convergence
         mockNodeAgentSetFrozenResponse(nodeAgents, true, true, true);