Merge pull request #5090 from vespa-engine/hmusum/split-out-create-container

Split out createContainer() into its own method

7 files changed, 51 insertions, 9 deletions

diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java
index e006d5aca4c..04d4628d576 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java
@@ -50,7 +50,9 @@ public interface Docker {
     }
 
     Optional<ContainerStats> getContainerStats(ContainerName containerName);
-    
+
+    void createContainer(CreateContainerCommand createContainerCommand);
+
     void startContainer(ContainerName containerName);
 
     void stopContainer(ContainerName containerName);
diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java
index a72865e023a..e81c6325922 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java
@@ -340,6 +340,18 @@ public class DockerImpl implements Docker {
     }
 
     @Override
+    public void createContainer(CreateContainerCommand createContainerCommand) {
+        try {
+            dockerClient.execCreateCmd(createContainerCommand.toString());
+        } catch (NotModifiedException ignored) {
+            // If is already created, ignore
+        } catch (RuntimeException e) {
+            numberOfDockerDaemonFails.add();
+            throw new DockerException("Failed to create container '" + createContainerCommand.toString() + "'", e);
+        }
+    }
+
+    @Override
     public void startContainer(ContainerName containerName) {
         try {
             dockerClient.startContainerCmd(containerName.asString()).exec();
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 80978d30da5..eb86e4cb5db 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
@@ -13,6 +13,8 @@ import java.util.Optional;
 
 public interface DockerOperations {
 
+    void createContainer(ContainerName containerName, ContainerNodeSpec nodeSpec);
+
     void startContainer(ContainerName containerName, ContainerNodeSpec nodeSpec);
 
     void removeContainer(Container existingContainer, ContainerNodeSpec nodeSpec);
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 b30cac2476e..c0d939b0d97 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
@@ -33,7 +33,7 @@ import static com.yahoo.vespa.defaults.Defaults.getDefaults;
 /**
  * Class that wraps the Docker class and have some tools related to running programs in docker.
  *
- * @author dybis
+ * @author Haakon Dybdahl
  */
 public class DockerOperationsImpl implements DockerOperations {
     public static final String NODE_PROGRAM = getDefaults().underVespaHome("bin/vespa-nodectl");
@@ -98,13 +98,11 @@ public class DockerOperationsImpl implements DockerOperations {
     }
 
     @Override
-    public void startContainer(ContainerName containerName, final ContainerNodeSpec nodeSpec) {
+    public void createContainer(ContainerName containerName, final ContainerNodeSpec nodeSpec) {
         PrefixLogger logger = PrefixLogger.getNodeAgentLogger(DockerOperationsImpl.class, containerName);
-
-        logger.info("Starting container " + containerName);
+        logger.info("Creating container " + containerName);
         try {
             InetAddress nodeInetAddress = environment.getInetAddressForHost(nodeSpec.hostname);
-            final boolean isIPv6 = nodeInetAddress instanceof Inet6Address;
 
             String configServers = environment.getConfigServerUris().stream()
                     .map(URI::getHost)
@@ -126,7 +124,7 @@ public class DockerOperationsImpl implements DockerOperations {
             if (!docker.networkNPTed()) {
                 command.withIpAddress(nodeInetAddress);
                 command.withNetworkMode(DockerImpl.DOCKER_CUSTOM_MACVLAN_NETWORK_NAME);
-                command.withVolume("/etc/hosts", "/etc/hosts"); // TODO This is probably not nessesary - review later
+                command.withVolume("/etc/hosts", "/etc/hosts"); // TODO This is probably not necessary - review later
             } else {
                 command.withIpAddress(NetworkPrefixTranslator.translate(
                         nodeInetAddress,
@@ -148,9 +146,23 @@ public class DockerOperationsImpl implements DockerOperations {
                 command.withEnvironment("VESPA_TOTAL_MEMORY_MB", Long.toString(minMainMemoryAvailableMb));
             }
 
-            logger.info("Starting new container with args: " + command);
+            logger.info("Creating new container with args: " + command);
             command.create();
 
+            docker.createContainer(command);
+        } catch (IOException e) {
+            throw new RuntimeException("Failed to create container " + containerName.asString(), e);
+        }
+    }
+
+    @Override
+    public void startContainer(ContainerName containerName, final ContainerNodeSpec nodeSpec) {
+        PrefixLogger logger = PrefixLogger.getNodeAgentLogger(DockerOperationsImpl.class, containerName);
+        logger.info("Starting container " + containerName);
+        try {
+            InetAddress nodeInetAddress = environment.getInetAddressForHost(nodeSpec.hostname);
+            boolean isIPv6 = nodeInetAddress instanceof Inet6Address;
+
             if (isIPv6) {
                 if (!docker.networkNPTed()) {
                     docker.connectContainerToNetwork(containerName, "bridge");
@@ -165,7 +177,7 @@ public class DockerOperationsImpl implements DockerOperations {
             DIRECTORIES_TO_MOUNT.entrySet().stream().filter(Map.Entry::getValue).forEach(entry ->
                     docker.executeInContainerAsRoot(containerName, "chmod", "-R", "a+w", entry.getKey()));
         } catch (IOException e) {
-            throw new RuntimeException("Failed to create container " + containerName.asString(), e);
+            throw new RuntimeException("Failed to start container " + containerName.asString(), e);
         }
     }
 
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 893054e5ac0..05977b9ebce 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
@@ -255,6 +255,7 @@ public class NodeAgentImpl implements NodeAgent {
 
     private void startContainer(ContainerNodeSpec nodeSpec) {
         aclMaintainer.run();
+        dockerOperations.createContainer(containerName, nodeSpec);
         dockerOperations.startContainer(containerName, nodeSpec);
         lastCpuMetric = new CpuUsageReporter();
 
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 8a4c4fd8c88..6494037ec3e 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
@@ -83,6 +83,13 @@ public class DockerMock implements Docker {
     }
 
     @Override
+    public void createContainer(CreateContainerCommand createContainerCommand) {
+        synchronized (monitor) {
+            callOrderVerifier.add("createContainer with " + createContainerCommand.toString());
+        }
+    }
+
+    @Override
     public void startContainer(ContainerName containerName) {
         synchronized (monitor) {
             callOrderVerifier.add("startContainer with " + containerName);
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 fb9303ea382..aa69d68a812 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
@@ -187,6 +187,7 @@ public class NodeAgentImplTest {
         final InOrder inOrder = inOrder(dockerOperations, orchestrator, nodeRepository, aclMaintainer);
         inOrder.verify(dockerOperations, times(1)).pullImageAsyncIfNeeded(eq(dockerImage));
         inOrder.verify(aclMaintainer, times(1)).run();
+        inOrder.verify(dockerOperations, times(1)).createContainer(eq(containerName), eq(nodeSpec));
         inOrder.verify(dockerOperations, times(1)).startContainer(eq(containerName), eq(nodeSpec));
         inOrder.verify(dockerOperations, times(1)).resumeNode(eq(containerName));
         inOrder.verify(nodeRepository).updateNodeAttributes(
@@ -263,6 +264,7 @@ public class NodeAgentImplTest {
         inOrder.verify(orchestrator).resume(any(String.class));
         inOrder.verify(orchestrator).suspend(any(String.class));
         inOrder.verify(dockerOperations).removeContainer(any(), any());
+        inOrder.verify(dockerOperations, times(1)).createContainer(eq(containerName), eq(thirdSpec));
         inOrder.verify(dockerOperations).startContainer(eq(containerName), eq(thirdSpec));
         inOrder.verify(orchestrator).resume(any(String.class));
     }
@@ -288,6 +290,7 @@ public class NodeAgentImplTest {
             fail("Expected to throw an exception");
         } catch (Exception ignored) { }
 
+        verify(dockerOperations, never()).createContainer(eq(containerName), eq(nodeSpec));
         verify(dockerOperations, never()).startContainer(eq(containerName), eq(nodeSpec));
         verify(orchestrator, never()).resume(any(String.class));
         verify(nodeRepository, never()).updateNodeAttributes(any(String.class), any(NodeAttributes.class));
@@ -345,6 +348,7 @@ 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()).createContainer(eq(containerName), eq(nodeSpec));
         verify(dockerOperations, never()).startContainer(eq(containerName), eq(nodeSpec));
         verify(orchestrator, never()).resume(any(String.class));
         verify(nodeRepository).updateNodeAttributes(
@@ -439,6 +443,7 @@ public class NodeAgentImplTest {
         inOrder.verify(storageMaintainer, times(1)).cleanupNodeStorage(eq(containerName), eq(nodeSpec));
         inOrder.verify(nodeRepository, times(1)).markNodeAvailableForNewAllocation(eq(hostName));
 
+        verify(dockerOperations, never()).createContainer(eq(containerName), any());
         verify(dockerOperations, never()).startContainer(eq(containerName), any());
         verify(orchestrator, never()).resume(any(String.class));
         verify(orchestrator, never()).suspend(any(String.class));
@@ -493,6 +498,7 @@ public class NodeAgentImplTest {
         nodeAgent.tick();
 
         verify(dockerOperations, times(1)).removeContainer(any(), any());
+        verify(dockerOperations, times(1)).createContainer(eq(containerName), eq(nodeSpec));
         verify(dockerOperations, times(1)).startContainer(eq(containerName), eq(nodeSpec));
     }