Revert "Re-create container if wanted create command changes"

8 files changed, 20 insertions, 52 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 e0e11ad0a3a..fb789874acf 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,17 +19,15 @@ 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, List<String> createCommand) {
+                     ContainerResources resources, List<Network> networks, boolean managed) {
         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 */
@@ -52,23 +50,18 @@ 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 container = (Container) o;
-        return conmonPid == container.conmonPid && hostname.equals(container.hostname) && resources.equals(container.resources) && networks.equals(container.networks) && createCommand.equals(container.createCommand);
+        Container that = (Container) o;
+        return conmonPid == that.conmonPid && hostname.equals(that.hostname) && resources.equals(that.resources) && networks.equals(that.networks);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(super.hashCode(), hostname, resources, conmonPid, networks, createCommand);
+        return Objects.hash(super.hashCode(), hostname, resources, conmonPid, networks);
     }
 
     /** 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 630509cf482..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
@@ -35,9 +35,6 @@ 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, ContainerResources wantedResources);
-
     /** 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 e632acd5223..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
@@ -57,10 +57,6 @@ public class ContainerOperations {
         containerEngine.updateContainer(context, containerId, containerResources);
     }
 
-    public boolean shouldRecreate(NodeAgentContext context, Container container, ContainerResources wantedResources) {
-        return containerEngine.shouldRecreate(context, container, wantedResources);
-    }
-
     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 d46325e4bca..20ea29381f3 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) {
-        NodeState nodeState = context.node().state();
+        final 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 image: "
+            reasons.add("The node is supposed to run a new Docker 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 (containerOperations.shouldRecreate(context, existingContainer, wantedContainerResources)) {
-            reasons.add("Container should be re-created to apply new configuration");
-        }
-        if (containerState == STARTING) {
+
+        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 744d78d83da..2d3a4976fe5 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,7 +28,6 @@ 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) {
@@ -113,11 +112,6 @@ public class ContainerEngineMock implements ContainerEngine {
     }
 
     @Override
-    public boolean shouldRecreate(NodeAgentContext context, Container container, ContainerResources wantedResources) {
-        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(),
@@ -125,8 +119,7 @@ public class ContainerEngineMock implements ContainerEngine {
                                                        container.labels(), container.pid(),
                                                        container.conmonPid(), container.hostname(),
                                                        containerResources, container.networks(),
-                                                       container.managed(),
-                                                       container.createCommand()));
+                                                       container.managed()));
     }
 
     @Override
@@ -207,8 +200,7 @@ public class ContainerEngineMock implements ContainerEngine {
                              context.hostname().value(),
                              containerResources,
                              List.of(),
-                             true,
-                             List.of());
+                             true);
     }
 
     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 8fb65e4bd47..9a5ca8c805e 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, List.of());
+                             ContainerResources.UNLIMITED, List.of(), managed);
     }
 
 }
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 0d83a397d33..2ef6780dff6 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());
+                             List.of(), true);
     }
 
     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 6e980b26cdf..fb132c9b717 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,15 +358,6 @@ 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(), 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
@@ -824,8 +815,7 @@ public class NodeAgentImplTest {
                             hostName,
                             containerResources,
                             List.of(),
-                            true,
-                            List.of())) :
+                            true)) :
                     Optional.empty();
         }).when(containerOperations).getContainer(any());
     }