diff options
author | Martin Polden <mpolden@mpolden.no> | 2022-11-30 10:08:48 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-11-30 10:08:48 +0100 |
commit | 18d26144dfdf2b0e72350d0a748837c8d4ca60d1 (patch) | |
tree | a37311f5752390a5708c60fe80a69c9837fa8a4e /node-admin | |
parent | 711362f17d4bbece0dc2d0833a22063374ae3e04 (diff) |
Revert "Re-create container if wanted create command changes"
Diffstat (limited to 'node-admin')
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()); } |