Re-create container if wanted create command changes

8 files changed, 52 insertions, 20 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/Container.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/Container.java
index fb789874acf..e0e11ad0a3a 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/Container.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/Container.java
@@ -19,15 +19,17 @@ public class Container extends PartialContainer {
     private final ContainerResources resources;
     private final int conmonPid;
     private final List<Network> networks;
+    private final List<String> createCommand;
 
     public Container(ContainerId id, ContainerName name, Instant createdAt, State state, String imageId, DockerImage image,
                      Map<String, String> labels, int pid, int conmonPid, String hostname,
-                     ContainerResources resources, List<Network> networks, boolean managed) {
+                     ContainerResources resources, List<Network> networks, boolean managed, List<String> createCommand) {
         super(id, name, createdAt, state, imageId, image, labels, pid, managed);
         this.hostname = Objects.requireNonNull(hostname);
         this.resources = Objects.requireNonNull(resources);
         this.conmonPid = conmonPid;
         this.networks = List.copyOf(Objects.requireNonNull(networks));
+        this.createCommand = List.copyOf(Objects.requireNonNull(createCommand));
     }
 
     /** The hostname of this, if any */
@@ -50,18 +52,23 @@ public class Container extends PartialContainer {
         return networks;
     }
 
+    /** The command used to create this */
+    public List<String> createCommand() {
+        return createCommand;
+    }
+
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;
         if (o == null || getClass() != o.getClass()) return false;
         if (!super.equals(o)) return false;
-        Container that = (Container) o;
-        return conmonPid == that.conmonPid && hostname.equals(that.hostname) && resources.equals(that.resources) && networks.equals(that.networks);
+        Container container = (Container) o;
+        return conmonPid == container.conmonPid && hostname.equals(container.hostname) && resources.equals(container.resources) && networks.equals(container.networks) && createCommand.equals(container.createCommand);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(super.hashCode(), hostname, resources, conmonPid, networks);
+        return Objects.hash(super.hashCode(), hostname, resources, conmonPid, networks, createCommand);
     }
 
     /** The network of a container */
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 2aa1d12c491..323a2223bd1 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
@@ -35,6 +35,9 @@ public interface ContainerEngine {
     /** Remove given container. The container will be stopped if necessary */
     void removeContainer(TaskContext context, PartialContainer container);
 
+    /** Returns whether the given container should be re-created to apply new configuration */
+    boolean shouldRecreate(NodeAgentContext context, Container container);
+
     /** Get container for given context */
     Optional<Container> getContainer(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 9060261b806..f144552343d 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
@@ -57,6 +57,10 @@ public class ContainerOperations {
         containerEngine.updateContainer(context, containerId, containerResources);
     }
 
+    public boolean shouldRecreate(NodeAgentContext context, Container container) {
+        return containerEngine.shouldRecreate(context, container);
+    }
+
     public Optional<Container> getContainer(NodeAgentContext context) {
         return containerEngine.getContainer(context);
     }
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 20ea29381f3..dc7e3587407 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
@@ -332,34 +332,34 @@ public class NodeAgentImpl implements NodeAgent {
     }
 
     private List<String> shouldRemoveContainer(NodeAgentContext context, Container existingContainer) {
-        final NodeState nodeState = context.node().state();
+        NodeState nodeState = context.node().state();
         List<String> reasons = new ArrayList<>();
-        if (nodeState == NodeState.dirty || nodeState == NodeState.provisioned)
+        if (nodeState == NodeState.dirty || nodeState == NodeState.provisioned) {
             reasons.add("Node in state " + nodeState + ", container should no longer be running");
-
+        }
         if (context.node().wantedDockerImage().isPresent() &&
                 !context.node().wantedDockerImage().get().equals(existingContainer.image())) {
-            reasons.add("The node is supposed to run a new Docker image: "
+            reasons.add("The node is supposed to run a new image: "
                         + existingContainer.image().asString() + " -> " + context.node().wantedDockerImage().get().asString());
         }
-
-        if (!existingContainer.state().isRunning())
+        if (!existingContainer.state().isRunning()) {
             reasons.add("Container no longer running");
-
+        }
         if (currentRebootGeneration < context.node().wantedRebootGeneration()) {
             reasons.add(String.format("Container reboot wanted. Current: %d, Wanted: %d",
                     currentRebootGeneration, context.node().wantedRebootGeneration()));
         }
-
         ContainerResources wantedContainerResources = getContainerResources(context);
         if (!wantedContainerResources.equalsMemory(existingContainer.resources())) {
             reasons.add("Container should be running with different memory allocation, wanted: " +
                         wantedContainerResources.toStringMemory() + ", actual: " + existingContainer.resources().toStringMemory());
         }
-
-        if (containerState == STARTING)
+        if (containerOperations.shouldRecreate(context, existingContainer)) {
+            reasons.add("Container should be re-created to apply new configuration");
+        }
+        if (containerState == STARTING) {
             reasons.add("Container failed to start");
-
+        }
         return reasons;
     }
 
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 2d3a4976fe5..d8888cb7267 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
@@ -28,6 +28,7 @@ public class ContainerEngineMock implements ContainerEngine {
 
     private final Map<ContainerName, Container> containers = new ConcurrentHashMap<>();
     private final Map<String, ImageDownload> images = new ConcurrentHashMap<>();
+
     private boolean asyncImageDownload = false;
 
     public ContainerEngineMock asyncImageDownload(boolean enabled) {
@@ -112,6 +113,11 @@ public class ContainerEngineMock implements ContainerEngine {
     }
 
     @Override
+    public boolean shouldRecreate(NodeAgentContext context, Container container) {
+        return false;
+    }
+
+    @Override
     public void updateContainer(NodeAgentContext context, ContainerId containerId, ContainerResources containerResources) {
         Container container = requireContainer(context.containerName());
         containers.put(container.name(), new Container(containerId, container.name(), container.createdAt(), container.state(),
@@ -119,7 +125,8 @@ public class ContainerEngineMock implements ContainerEngine {
                                                        container.labels(), container.pid(),
                                                        container.conmonPid(), container.hostname(),
                                                        containerResources, container.networks(),
-                                                       container.managed()));
+                                                       container.managed(),
+                                                       container.createCommand()));
     }
 
     @Override
@@ -200,7 +207,8 @@ public class ContainerEngineMock implements ContainerEngine {
                              context.hostname().value(),
                              containerResources,
                              List.of(),
-                             true);
+                             true,
+                             List.of());
     }
 
     private static class ImageDownload {
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperationsTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperationsTest.java
index 9a5ca8c805e..8fb65e4bd47 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperationsTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerOperationsTest.java
@@ -59,7 +59,7 @@ public class ContainerOperationsTest {
     private Container createContainer(String name, boolean managed) {
         return new Container(new ContainerId("id-of-" + name), new ContainerName(name), Instant.EPOCH, PartialContainer.State.running,
                              "image-id", DockerImage.EMPTY, Map.of(), 42, 43, name,
-                             ContainerResources.UNLIMITED, List.of(), managed);
+                             ContainerResources.UNLIMITED, List.of(), managed, List.of());
     }
 
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePrunerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePrunerTest.java
index 2ef6780dff6..0d83a397d33 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePrunerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePrunerTest.java
@@ -123,7 +123,7 @@ public class ContainerImagePrunerTest {
         return new Container(new ContainerId("id-of-" + name), new ContainerName(name), Instant.EPOCH,
                              Container.State.running, imageId, DockerImage.EMPTY, Map.of(),
                              42, 43, name + ".example.com", ContainerResources.UNLIMITED,
-                             List.of(), true);
+                             List.of(), true, List.of());
     }
 
     private static class Tester {
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 fb132c9b717..63a7242c96c 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
@@ -358,6 +358,15 @@ public class NodeAgentImplTest {
         verify(orchestrator, times(1)).resume(eq(hostName));
         verify(nodeRepository, times(1)).updateNodeAttributes(eq(hostName), eq(new NodeAttributes()
                 .withRebootGeneration(wantedRebootGeneration)));
+
+        // Re-create if new container config needs to be applied
+        when(containerOperations.shouldRecreate(eq(context), any())).thenReturn(true);
+        nodeAgent.doConverge(context);
+        verify(containerOperations, times(2)).removeContainer(eq(context), any());
+        verify(containerOperations, times(2)).createContainer(eq(context), any());
+        verify(orchestrator, times(2)).resume(eq(hostName));
+        verify(nodeRepository, times(2)).updateNodeAttributes(eq(hostName), eq(new NodeAttributes()
+                                                                                       .withRebootGeneration(wantedRebootGeneration)));
     }
 
     @Test
@@ -815,7 +824,8 @@ public class NodeAgentImplTest {
                             hostName,
                             containerResources,
                             List.of(),
-                            true)) :
+                            true,
+                            List.of())) :
                     Optional.empty();
         }).when(containerOperations).getContainer(any());
     }