Return ContainerData from container create

10 files changed, 60 insertions, 55 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngine.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngine.java
index cfa0452ebf9..2aa1d12c491 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngine.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngine.java
@@ -20,8 +20,11 @@ import java.util.Optional;
  */
 public interface ContainerEngine {
 
-    /** Create a new container */
-    void createContainer(NodeAgentContext context, ContainerData containerData, ContainerResources containerResources);
+    /**
+     * Create a new container
+     * @return ContainerData that can be used to write files inside container
+     */
+    ContainerData createContainer(NodeAgentContext context, ContainerResources containerResources);
 
     /** Start a created container */
     void startContainer(NodeAgentContext context);
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperations.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperations.java
index 8a66373c28b..9060261b806 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperations.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperations.java
@@ -41,8 +41,8 @@ public class ContainerOperations {
         this.containerStatsCollector = new ContainerStatsCollector(cgroup, fileSystem);
     }
 
-    public void createContainer(NodeAgentContext context, ContainerData containerData, ContainerResources containerResources) {
-        containerEngine.createContainer(context, containerData, containerResources);
+    public ContainerData createContainer(NodeAgentContext context, ContainerResources containerResources) {
+        return containerEngine.createContainer(context, containerResources);
     }
 
     public void startContainer(NodeAgentContext context) {
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/ContainerData.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/ContainerData.java
index 26c3d101c69..7e30ae8d649 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/ContainerData.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/ContainerData.java
@@ -23,8 +23,11 @@ public interface ContainerData {
      */
     void addFile(ContainerPath path, String data, String permissions);
 
-    /** Add directory in container at path. */
-    void addDirectory(ContainerPath path);
+    /**
+     * @param path Container path to create directory at
+     * @param permissions optional file permissions, see {@link UnixPath#setPermissions(String)} for format.
+     */
+    void addDirectory(ContainerPath path, String... permissions);
 
     /**
      * Symlink to a file in container at path.
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 9da7f1dbdb6..61e777a9576 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
@@ -27,9 +27,7 @@ import com.yahoo.vespa.hosted.node.admin.maintenance.acl.AclMaintainer;
 import com.yahoo.vespa.hosted.node.admin.maintenance.identity.CredentialsMaintainer;
 import com.yahoo.vespa.hosted.node.admin.maintenance.servicedump.VespaServiceDumper;
 import com.yahoo.vespa.hosted.node.admin.nodeadmin.ConvergenceException;
-import com.yahoo.vespa.hosted.node.admin.task.util.fs.ContainerPath;
 
-import java.nio.file.Path;
 import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
@@ -233,10 +231,10 @@ public class NodeAgentImpl implements NodeAgent {
     }
 
     private Container startContainer(NodeAgentContext context) {
-        ContainerData containerData = createContainerData(context);
         ContainerResources wantedResources = warmUpDuration(context).isNegative() ?
                 getContainerResources(context) : getContainerResources(context).withUnlimitedCpus();
-        containerOperations.createContainer(context, containerData, wantedResources);
+        ContainerData containerData = containerOperations.createContainer(context, wantedResources);
+        writeContainerData(context, containerData);
         containerOperations.startContainer(context);
 
         currentRebootGeneration = context.node().wantedRebootGeneration();
@@ -598,29 +596,7 @@ public class NodeAgentImpl implements NodeAgent {
         }
     }
 
-    protected ContainerData createContainerData(NodeAgentContext context) {
-        return new ContainerData() {
-            @Override
-            public void addFile(ContainerPath path, String data) {
-                throw new UnsupportedOperationException("addFile not implemented");
-            }
-
-            @Override
-            public void addFile(ContainerPath path, String data, String permissions) {
-                throw new UnsupportedOperationException("addFile not implemented");
-            }
-
-            @Override
-            public void addDirectory(ContainerPath path) {
-                throw new UnsupportedOperationException("addDirectory not implemented");
-            }
-
-            @Override
-            public void createSymlink(ContainerPath symlink, Path target) {
-                throw new UnsupportedOperationException("createSymlink not implemented");
-            }
-        };
-    }
+    protected void writeContainerData(NodeAgentContext context, ContainerData containerData) { }
 
     protected List<CredentialsMaintainer> credentialsMaintainers() {
         return credentialsMaintainers;
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java
index 25cdff4b726..de3ae4cff98 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java
@@ -7,8 +7,10 @@ import com.yahoo.vespa.hosted.node.admin.container.image.Image;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.ContainerData;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixUser;
+import com.yahoo.vespa.hosted.node.admin.task.util.fs.ContainerPath;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 
+import java.nio.file.Path;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.List;
@@ -66,8 +68,29 @@ public class ContainerEngineMock implements ContainerEngine {
     }
 
     @Override
-    public void createContainer(NodeAgentContext context, ContainerData containerData, ContainerResources containerResources) {
+    public ContainerData createContainer(NodeAgentContext context, ContainerResources containerResources) {
         addContainer(createContainer(context, PartialContainer.State.created, containerResources));
+        return new ContainerData() {
+            @Override
+            public void addFile(ContainerPath path, String data) {
+                throw new UnsupportedOperationException("addFile not implemented");
+            }
+
+            @Override
+            public void addFile(ContainerPath path, String data, String permissions) {
+                throw new UnsupportedOperationException("addFile not implemented");
+            }
+
+            @Override
+            public void addDirectory(ContainerPath path, String... permissions) {
+                throw new UnsupportedOperationException("addDirectory not implemented");
+            }
+
+            @Override
+            public void createSymlink(ContainerPath symlink, Path target) {
+                throw new UnsupportedOperationException("createSymlink not implemented");
+            }
+        };
     }
 
     @Override
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/ContainerFailTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/ContainerFailTest.java
index a2dbfe0db4b..5649da7c4ea 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/ContainerFailTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/ContainerFailTest.java
@@ -2,8 +2,8 @@
 package com.yahoo.vespa.hosted.node.admin.integration;
 
 import com.yahoo.config.provision.DockerImage;
-import com.yahoo.vespa.hosted.node.admin.container.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec;
+import com.yahoo.vespa.hosted.node.admin.container.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImpl;
 import com.yahoo.vespa.test.file.TestFileSystem;
@@ -36,13 +36,13 @@ public class ContainerFailTest {
 
             NodeAgentContext context = NodeAgentContextImpl.builder(nodeSpec).fileSystem(TestFileSystem.create()).build();
 
-            tester.inOrder(tester.containerOperations).createContainer(containerMatcher(containerName), any(), any());
+            tester.inOrder(tester.containerOperations).createContainer(containerMatcher(containerName), any());
             tester.inOrder(tester.containerOperations).resumeNode(containerMatcher(containerName));
 
             tester.containerOperations.removeContainer(context, tester.containerOperations.getContainer(context).get());
 
             tester.inOrder(tester.containerOperations).removeContainer(containerMatcher(containerName), any());
-            tester.inOrder(tester.containerOperations).createContainer(containerMatcher(containerName), any(), any());
+            tester.inOrder(tester.containerOperations).createContainer(containerMatcher(containerName), any());
             tester.inOrder(tester.containerOperations).resumeNode(containerMatcher(containerName));
 
             verify(tester.nodeRepository, never()).updateNodeAttributes(any(), any());
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/MultiContainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/MultiContainerTest.java
index 5bb279323e9..79546d8cf78 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/MultiContainerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/MultiContainerTest.java
@@ -48,7 +48,7 @@ public class MultiContainerTest {
         tester.addChildNodeRepositoryNode(nodeSpec);
 
         ContainerName containerName = ContainerName.fromHostname(hostName);
-        tester.inOrder(tester.containerOperations).createContainer(containerMatcher(containerName), any(), any());
+        tester.inOrder(tester.containerOperations).createContainer(containerMatcher(containerName), any());
         tester.inOrder(tester.containerOperations).resumeNode(containerMatcher(containerName));
         tester.inOrder(tester.nodeRepository).updateNodeAttributes(eq(hostName), any());
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/RebootTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/RebootTest.java
index 4d95991dd29..f3635be1b4b 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/RebootTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/RebootTest.java
@@ -2,8 +2,8 @@
 package com.yahoo.vespa.hosted.node.admin.integration;
 
 import com.yahoo.config.provision.DockerImage;
-import com.yahoo.vespa.hosted.node.admin.container.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec;
+import com.yahoo.vespa.hosted.node.admin.container.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.nodeadmin.NodeAdminStateUpdater;
 import org.junit.Test;
 
@@ -31,7 +31,7 @@ public class RebootTest {
             tester.addChildNodeRepositoryNode(NodeSpec.Builder.testSpec(hostname).wantedDockerImage(dockerImage).build());
 
             ContainerName host1 = new ContainerName("host1");
-            tester.inOrder(tester.containerOperations).createContainer(containerMatcher(host1), any(), any());
+            tester.inOrder(tester.containerOperations).createContainer(containerMatcher(host1), any());
 
             tester.setWantedState(NodeAdminStateUpdater.State.SUSPENDED);
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/RestartTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/RestartTest.java
index 13b3db1c55c..0675626c0e2 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/RestartTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/RestartTest.java
@@ -2,9 +2,9 @@
 package com.yahoo.vespa.hosted.node.admin.integration;
 
 import com.yahoo.config.provision.DockerImage;
-import com.yahoo.vespa.hosted.node.admin.container.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeAttributes;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec;
+import com.yahoo.vespa.hosted.node.admin.container.ContainerName;
 import org.junit.Test;
 
 import java.util.List;
@@ -29,7 +29,7 @@ public class RestartTest {
             tester.addChildNodeRepositoryNode(nodeSpec);
 
             ContainerName host1 = new ContainerName("host1");
-            tester.inOrder(tester.containerOperations).createContainer(containerMatcher(host1), any(), any());
+            tester.inOrder(tester.containerOperations).createContainer(containerMatcher(host1), any());
             tester.inOrder(tester.nodeRepository).updateNodeAttributes(
                     eq(hostname), eq(new NodeAttributes().withDockerImage(dockerImage).withVespaVersion(dockerImage.tagAsVersion())));
 
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 28ef19d4065..2eff2c64cec 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
@@ -154,7 +154,7 @@ public class NodeAgentImplTest {
 
         nodeAgent.stopForHostSuspension(context);
         nodeAgent.doConverge(context);
-        inOrder.verify(containerOperations, times(1)).createContainer(eq(context), any(), any());
+        inOrder.verify(containerOperations, times(1)).createContainer(eq(context), any());
         inOrder.verify(containerOperations, times(1)).startContainer(eq(context));
         inOrder.verify(containerOperations, times(0)).startServices(eq(context)); // done as part of startContainer
         inOrder.verify(containerOperations, times(1)).resumeNode(eq(context));
@@ -181,7 +181,7 @@ public class NodeAgentImplTest {
 
         final InOrder inOrder = inOrder(containerOperations, orchestrator, nodeRepository, aclMaintainer, healthChecker);
         inOrder.verify(containerOperations, times(1)).pullImageAsyncIfNeeded(any(), eq(dockerImage), any());
-        inOrder.verify(containerOperations, times(1)).createContainer(eq(context), any(), any());
+        inOrder.verify(containerOperations, times(1)).createContainer(eq(context), any());
         inOrder.verify(containerOperations, times(1)).startContainer(eq(context));
         inOrder.verify(aclMaintainer, times(1)).converge(eq(context));
         inOrder.verify(containerOperations, times(1)).resumeNode(eq(context));
@@ -310,7 +310,7 @@ public class NodeAgentImplTest {
             fail("Expected to throw an exception");
         } catch (OrchestratorException ignored) { }
 
-        verify(containerOperations, never()).createContainer(eq(context), any(), any());
+        verify(containerOperations, never()).createContainer(eq(context), any());
         verify(containerOperations, never()).startContainer(eq(context));
         verify(orchestrator, never()).resume(any(String.class));
         verify(nodeRepository, never()).updateNodeAttributes(any(String.class), any(NodeAttributes.class));
@@ -343,7 +343,7 @@ public class NodeAgentImplTest {
         // First time we fail to resume because health verification fails
         verify(orchestrator, times(1)).suspend(eq(hostName));
         verify(containerOperations, times(1)).removeContainer(eq(context), any());
-        verify(containerOperations, times(1)).createContainer(eq(context), any(), any());
+        verify(containerOperations, times(1)).createContainer(eq(context), any());
         verify(containerOperations, times(1)).startContainer(eq(context));
         verify(orchestrator, never()).resume(eq(hostName));
         verify(nodeRepository, never()).updateNodeAttributes(any(), any());
@@ -352,7 +352,7 @@ public class NodeAgentImplTest {
 
         // Do not reboot the container again
         verify(containerOperations, times(1)).removeContainer(eq(context), any());
-        verify(containerOperations, times(1)).createContainer(eq(context), any(), any());
+        verify(containerOperations, times(1)).createContainer(eq(context), any());
         verify(orchestrator, times(1)).resume(eq(hostName));
         verify(nodeRepository, times(1)).updateNodeAttributes(eq(hostName), eq(new NodeAttributes()
                 .withRebootGeneration(wantedRebootGeneration)));
@@ -393,7 +393,7 @@ public class NodeAgentImplTest {
         // Should only be called once, when we initialize
         verify(containerOperations, times(1)).getContainer(eq(context));
         verify(containerOperations, never()).removeContainer(eq(context), any());
-        verify(containerOperations, never()).createContainer(eq(context), any(), any());
+        verify(containerOperations, never()).createContainer(eq(context), any());
         verify(containerOperations, never()).startContainer(eq(context));
         verify(orchestrator, never()).resume(any(String.class));
         verify(nodeRepository, never()).updateNodeAttributes(eq(hostName), any());
@@ -458,7 +458,7 @@ public class NodeAgentImplTest {
         inOrder.verify(storageMaintainer, times(1)).archiveNodeStorage(eq(context));
         inOrder.verify(nodeRepository, times(1)).setNodeState(eq(hostName), eq(NodeState.ready));
 
-        verify(containerOperations, never()).createContainer(eq(context), any(), any());
+        verify(containerOperations, never()).createContainer(eq(context), any());
         verify(containerOperations, never()).startContainer(eq(context));
         verify(containerOperations, never()).suspendNode(eq(context));
         verify(containerOperations, times(1)).stopServices(eq(context));
@@ -508,7 +508,7 @@ public class NodeAgentImplTest {
         nodeAgent.doConverge(context);
 
         verify(containerOperations, times(1)).removeContainer(eq(context), any());
-        verify(containerOperations, times(1)).createContainer(eq(context), any(), any());
+        verify(containerOperations, times(1)).createContainer(eq(context), any());
         verify(containerOperations, times(1)).startContainer(eq(context));
     }
 
@@ -568,7 +568,7 @@ public class NodeAgentImplTest {
         } catch (RuntimeException ignored) { }
 
         verify(containerOperations, never()).removeContainer(eq(context), any());
-        verify(containerOperations, times(1)).createContainer(eq(context), any(), any());
+        verify(containerOperations, times(1)).createContainer(eq(context), any());
         verify(containerOperations, times(1)).startContainer(eq(context));
         verify(nodeAgent, never()).resumeNodeIfNeeded(any());
 
@@ -578,7 +578,7 @@ public class NodeAgentImplTest {
         nodeAgent.doConverge(context);
 
         verify(containerOperations, times(1)).removeContainer(eq(context), any());
-        verify(containerOperations, times(2)).createContainer(eq(context), any(), any());
+        verify(containerOperations, times(2)).createContainer(eq(context), any());
         verify(containerOperations, times(2)).startContainer(eq(context));
         verify(nodeAgent, times(1)).resumeNodeIfNeeded(any());
     }
@@ -605,7 +605,7 @@ public class NodeAgentImplTest {
 
         final InOrder inOrder = inOrder(containerOperations, orchestrator, nodeRepository, aclMaintainer);
         inOrder.verify(containerOperations, times(1)).pullImageAsyncIfNeeded(any(), eq(dockerImage), any());
-        inOrder.verify(containerOperations, times(1)).createContainer(eq(context), any(), any());
+        inOrder.verify(containerOperations, times(1)).createContainer(eq(context), any());
         inOrder.verify(containerOperations, times(1)).startContainer(eq(context));
         inOrder.verify(aclMaintainer, times(1)).converge(eq(context));
         inOrder.verify(containerOperations, times(1)).resumeNode(eq(context));
@@ -770,10 +770,10 @@ public class NodeAgentImplTest {
         mockGetContainer(dockerImage, isRunning);
         doAnswer(invoc -> {
             NodeAgentContext context = invoc.getArgument(0, NodeAgentContext.class);
-            ContainerResources resources = invoc.getArgument(2, ContainerResources.class);
+            ContainerResources resources = invoc.getArgument(1, ContainerResources.class);
             mockGetContainer(context.node().wantedDockerImage().get(), resources, true);
             return null;
-        }).when(containerOperations).createContainer(any(), any(), any());
+        }).when(containerOperations).createContainer(any(), any());
 
         doAnswer(invoc -> {
             NodeAgentContext context = invoc.getArgument(0, NodeAgentContext.class);