Do not NodeSpec to start/remove container

4 files changed, 46 insertions, 59 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 baf1be41c36..37ab6a08ffb 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
@@ -16,9 +16,9 @@ public interface DockerOperations {
 
     void createContainer(ContainerName containerName, NodeSpec node, ContainerData containerData);
 
-    void startContainer(ContainerName containerName, NodeSpec node);
+    void startContainer(ContainerName containerName);
 
-    void removeContainer(Container existingContainer, NodeSpec node);
+    void removeContainer(Container existingContainer);
 
     Optional<Container> getContainer(ContainerName containerName);
 
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 03ff1760158..7a51b7243f3 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
@@ -180,36 +180,29 @@ public class DockerOperationsImpl implements DockerOperations {
     }
 
     @Override
-    public void startContainer(ContainerName containerName, final NodeSpec node) {
+    public void startContainer(ContainerName containerName) {
         PrefixLogger logger = PrefixLogger.getNodeAgentLogger(DockerOperationsImpl.class, containerName);
         logger.info("Starting container " + containerName);
-        try {
-            InetAddress nodeInetAddress = environment.getInetAddressForHost(node.getHostname());
-            boolean isIPv6 = nodeInetAddress instanceof Inet6Address;
 
-            if (isIPv6) {
-                if (!docker.networkNATed()) {
-                    docker.connectContainerToNetwork(containerName, "bridge");
-                }
+        if (!docker.networkNATed()) {
+            docker.connectContainerToNetwork(containerName, "bridge");
+        }
 
-                docker.startContainer(containerName);
-                setupContainerNetworkConnectivity(containerName);
-            } else {
-                docker.startContainer(containerName);
-            }
+        docker.startContainer(containerName);
 
-            directoriesToMount.entrySet().stream()
-                    .filter(Map.Entry::getValue)
-                    .map(Map.Entry::getKey)
-                    .forEach(path ->
-                            docker.executeInContainerAsRoot(containerName, "chmod", "-R", "a+w", path.toString()));
-        } catch (IOException e) {
-            throw new RuntimeException("Failed to start container " + containerName.asString(), e);
+        if (!docker.networkNATed()) {
+            setupContainerNetworkConnectivity(containerName);
         }
+
+        directoriesToMount.entrySet().stream()
+                .filter(Map.Entry::getValue)
+                .map(Map.Entry::getKey)
+                .forEach(path ->
+                        docker.executeInContainerAsRoot(containerName, "chmod", "-R", "a+w", path.toString()));
     }
 
     @Override
-    public void removeContainer(final Container existingContainer, NodeSpec node) {
+    public void removeContainer(Container existingContainer) {
         final ContainerName containerName = existingContainer.name;
         PrefixLogger logger = PrefixLogger.getNodeAgentLogger(DockerOperationsImpl.class, containerName);
         if (existingContainer.state.isRunning()) {
@@ -252,12 +245,10 @@ public class DockerOperationsImpl implements DockerOperations {
      * Due to a bug in docker (https://github.com/docker/libnetwork/issues/1443), we need to manually set
      * IPv6 gateway in containers connected to more than one docker network
      */
-    private void setupContainerNetworkConnectivity(ContainerName containerName) throws IOException {
-        if (!docker.networkNATed()) {
-            InetAddress hostDefaultGateway = DockerNetworkCreator.getDefaultGatewayLinux(true);
-            executeCommandInNetworkNamespace(containerName,
-                    "route", "-A", "inet6", "add", "default", "gw", hostDefaultGateway.getHostAddress(), "dev", "eth1");
-        }
+    private void setupContainerNetworkConnectivity(ContainerName containerName) {
+        InetAddress hostDefaultGateway = uncheck(() -> DockerNetworkCreator.getDefaultGatewayLinux(true));
+        executeCommandInNetworkNamespace(containerName,
+                "route", "-A", "inet6", "add", "default", "gw", hostDefaultGateway.getHostAddress(), "dev", "eth1");
     }
 
     @Override
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
index 9d43e0b8b4f..5f2093c4719 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
@@ -26,7 +26,6 @@ import com.yahoo.vespa.hosted.node.admin.maintenance.identity.AthenzCredentialsM
 import com.yahoo.vespa.hosted.node.admin.util.PrefixLogger;
 import com.yahoo.vespa.hosted.provision.Node;
 
-import java.nio.file.Path;
 import java.text.SimpleDateFormat;
 import java.time.Clock;
 import java.time.Duration;
@@ -276,7 +275,7 @@ public class NodeAgentImpl implements NodeAgent {
     private void startContainer(NodeSpec node) {
         ContainerData containerData = createContainerData(environment, node);
         dockerOperations.createContainer(containerName, node, containerData);
-        dockerOperations.startContainer(containerName, node);
+        dockerOperations.startContainer(containerName);
         lastCpuMetric = new CpuUsageReporter();
 
         resumeScriptRun = false;
@@ -364,7 +363,7 @@ public class NodeAgentImpl implements NodeAgent {
                 }
             }
             stopFilebeatSchedulerIfNeeded();
-            dockerOperations.removeContainer(existingContainer, node);
+            dockerOperations.removeContainer(existingContainer);
             containerState = ABSENT;
             logger.info("Container successfully removed, new containerState is " + containerState);
             return Optional.empty();
@@ -738,11 +737,8 @@ public class NodeAgentImpl implements NodeAgent {
     }
 
     protected ContainerData createContainerData(Environment environment, NodeSpec node) {
-        return new ContainerData() {
-            @Override
-            public void addFile(Path pathInContainer, String data) {
-                throw new UnsupportedOperationException("addFile not implemented");
-            }
+        return (pathInContainer, data) -> {
+            throw new UnsupportedOperationException("addFile not implemented");
         };
     }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
index 782917ab7c0..f5d4dcf4e5e 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
@@ -123,7 +123,7 @@ public class NodeAgentImplTest {
 
         nodeAgent.converge();
 
-        verify(dockerOperations, never()).removeContainer(any(), any());
+        verify(dockerOperations, never()).removeContainer(any());
         verify(orchestrator, never()).suspend(any(String.class));
         verify(dockerOperations, never()).pullImageAsyncIfNeeded(any());
         verify(storageMaintainer, never()).removeOldFilesFromNode(eq(containerName));
@@ -182,13 +182,13 @@ public class NodeAgentImplTest {
 
         nodeAgent.converge();
 
-        verify(dockerOperations, never()).removeContainer(any(), any());
+        verify(dockerOperations, never()).removeContainer(any());
         verify(orchestrator, never()).suspend(any(String.class));
 
         final InOrder inOrder = inOrder(dockerOperations, orchestrator, nodeRepository, aclMaintainer);
         inOrder.verify(dockerOperations, times(1)).pullImageAsyncIfNeeded(eq(dockerImage));
         inOrder.verify(dockerOperations, times(1)).createContainer(eq(containerName), eq(node), any());
-        inOrder.verify(dockerOperations, times(1)).startContainer(eq(containerName), eq(node));
+        inOrder.verify(dockerOperations, times(1)).startContainer(eq(containerName));
         inOrder.verify(aclMaintainer, times(1)).run();
         inOrder.verify(dockerOperations, times(1)).resumeNode(eq(containerName));
         inOrder.verify(nodeRepository).updateNodeAttributes(
@@ -224,7 +224,7 @@ public class NodeAgentImplTest {
 
         verify(orchestrator, never()).suspend(any(String.class));
         verify(orchestrator, never()).resume(any(String.class));
-        verify(dockerOperations, never()).removeContainer(any(), any());
+        verify(dockerOperations, never()).removeContainer(any());
 
         final InOrder inOrder = inOrder(dockerOperations);
         inOrder.verify(dockerOperations, times(1)).pullImageAsyncIfNeeded(eq(newDockerImage));
@@ -264,9 +264,9 @@ public class NodeAgentImplTest {
         inOrder.verify(orchestrator).resume(any(String.class));
         inOrder.verify(orchestrator).resume(any(String.class));
         inOrder.verify(orchestrator).suspend(any(String.class));
-        inOrder.verify(dockerOperations).removeContainer(any(), any());
+        inOrder.verify(dockerOperations).removeContainer(any());
         inOrder.verify(dockerOperations, times(1)).createContainer(eq(containerName), eq(thirdSpec), any());
-        inOrder.verify(dockerOperations).startContainer(eq(containerName), eq(thirdSpec));
+        inOrder.verify(dockerOperations).startContainer(eq(containerName));
         inOrder.verify(orchestrator).resume(any(String.class));
     }
 
@@ -292,7 +292,7 @@ public class NodeAgentImplTest {
         } catch (Exception ignored) { }
 
         verify(dockerOperations, never()).createContainer(eq(containerName), eq(node), any());
-        verify(dockerOperations, never()).startContainer(eq(containerName), eq(node));
+        verify(dockerOperations, never()).startContainer(eq(containerName));
         verify(orchestrator, never()).resume(any(String.class));
         verify(nodeRepository, never()).updateNodeAttributes(any(String.class), any(NodeAttributes.class));
     }
@@ -316,7 +316,7 @@ public class NodeAgentImplTest {
 
         nodeAgent.converge();
 
-        verify(dockerOperations, never()).removeContainer(any(), any());
+        verify(dockerOperations, never()).removeContainer(any());
         verify(orchestrator, never()).resume(any(String.class));
         verify(nodeRepository, never()).updateNodeAttributes(eq(hostName), any());
     }
@@ -342,9 +342,9 @@ public class NodeAgentImplTest {
 
         // Should only be called once, when we initialize
         verify(dockerOperations, times(1)).getContainer(eq(containerName));
-        verify(dockerOperations, never()).removeContainer(any(), any());
+        verify(dockerOperations, never()).removeContainer(any());
         verify(dockerOperations, never()).createContainer(eq(containerName), eq(node), any());
-        verify(dockerOperations, never()).startContainer(eq(containerName), eq(node));
+        verify(dockerOperations, never()).startContainer(eq(containerName));
         verify(orchestrator, never()).resume(any(String.class));
         verify(nodeRepository, never()).updateNodeAttributes(eq(hostName), any());
     }
@@ -372,7 +372,7 @@ public class NodeAgentImplTest {
         nodeAgent.converge();
 
         final InOrder inOrder = inOrder(storageMaintainer, dockerOperations);
-        inOrder.verify(dockerOperations, never()).removeContainer(any(), any());
+        inOrder.verify(dockerOperations, never()).removeContainer(any());
 
         verify(orchestrator, never()).resume(any(String.class));
         verify(nodeRepository, never()).updateNodeAttributes(eq(hostName), any());
@@ -419,12 +419,12 @@ public class NodeAgentImplTest {
         nodeAgent.converge();
 
         final InOrder inOrder = inOrder(storageMaintainer, dockerOperations, nodeRepository);
-        inOrder.verify(dockerOperations, times(1)).removeContainer(any(), any());
+        inOrder.verify(dockerOperations, times(1)).removeContainer(any());
         inOrder.verify(storageMaintainer, times(1)).cleanupNodeStorage(eq(containerName), eq(node));
         inOrder.verify(nodeRepository, times(1)).setNodeState(eq(hostName), eq(Node.State.ready));
 
         verify(dockerOperations, never()).createContainer(eq(containerName), any(), any());
-        verify(dockerOperations, never()).startContainer(eq(containerName), any());
+        verify(dockerOperations, never()).startContainer(eq(containerName));
         verify(orchestrator, never()).resume(any(String.class));
         verify(orchestrator, never()).suspend(any(String.class));
         // current Docker image and vespa version should be cleared
@@ -477,9 +477,9 @@ public class NodeAgentImplTest {
 
         nodeAgent.tick();
 
-        verify(dockerOperations, times(1)).removeContainer(any(), any());
+        verify(dockerOperations, times(1)).removeContainer(any());
         verify(dockerOperations, times(1)).createContainer(eq(containerName), eq(node), any());
-        verify(dockerOperations, times(1)).startContainer(eq(containerName), eq(node));
+        verify(dockerOperations, times(1)).startContainer(eq(containerName));
     }
 
     @Test
@@ -568,16 +568,16 @@ public class NodeAgentImplTest {
         when(pathResolver.getApplicationStoragePathForHost()).thenReturn(Files.createTempDirectory("bar"));
         when(dockerOperations.pullImageAsyncIfNeeded(eq(dockerImage))).thenReturn(false);
         when(storageMaintainer.getDiskUsageFor(eq(containerName))).thenReturn(Optional.of(201326592000L));
-        doThrow(new DockerException("Failed to set up network")).doNothing().when(dockerOperations).startContainer(eq(containerName), eq(node));
+        doThrow(new DockerException("Failed to set up network")).doNothing().when(dockerOperations).startContainer(eq(containerName));
 
         try {
             nodeAgent.converge();
             fail("Expected to get DockerException");
         } catch (DockerException ignored) { }
 
-        verify(dockerOperations, never()).removeContainer(any(), any());
+        verify(dockerOperations, never()).removeContainer(any());
         verify(dockerOperations, times(1)).createContainer(eq(containerName), eq(node), any());
-        verify(dockerOperations, times(1)).startContainer(eq(containerName), eq(node));
+        verify(dockerOperations, times(1)).startContainer(eq(containerName));
         verify(nodeAgent, never()).runLocalResumeScriptIfNeeded(any());
 
         // The docker container was actually started and is running, but subsequent exec calls to set up
@@ -585,9 +585,9 @@ public class NodeAgentImplTest {
         mockGetContainer(dockerImage, true);
         nodeAgent.converge();
 
-        verify(dockerOperations, times(1)).removeContainer(any(), eq(node));
+        verify(dockerOperations, times(1)).removeContainer(any());
         verify(dockerOperations, times(2)).createContainer(eq(containerName), eq(node), any());
-        verify(dockerOperations, times(2)).startContainer(eq(containerName), eq(node));
+        verify(dockerOperations, times(2)).startContainer(eq(containerName));
         verify(nodeAgent, times(1)).runLocalResumeScriptIfNeeded(any());
     }
 
@@ -695,13 +695,13 @@ public class NodeAgentImplTest {
 
         nodeAgent.converge();
 
-        verify(dockerOperations, never()).removeContainer(any(), any());
+        verify(dockerOperations, never()).removeContainer(any());
         verify(orchestrator, never()).suspend(any(String.class));
 
         final InOrder inOrder = inOrder(dockerOperations, orchestrator, nodeRepository, aclMaintainer);
         inOrder.verify(dockerOperations, times(1)).pullImageAsyncIfNeeded(eq(dockerImage));
         inOrder.verify(dockerOperations, times(1)).createContainer(eq(containerName), eq(node), any());
-        inOrder.verify(dockerOperations, times(1)).startContainer(eq(containerName), eq(node));
+        inOrder.verify(dockerOperations, times(1)).startContainer(eq(containerName));
         inOrder.verify(aclMaintainer, times(1)).run();
         inOrder.verify(dockerOperations, times(1)).resumeNode(eq(containerName));
         inOrder.verify(nodeRepository).updateNodeAttributes(