Merge pull request #18508 from vespa-engine/mpolden/fix-unstable-test

Avoid async image pulling in ContainerTester

6 files changed, 50 insertions, 58 deletions

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 4e2a5052ea6..1d077449ed6 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
@@ -9,7 +9,6 @@ import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 
 import java.time.Duration;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -19,19 +18,12 @@ import java.util.concurrent.CountDownLatch;
 import java.util.stream.Collectors;
 
 /**
- * A mock implementation of {@link ContainerEngine}.
- *
- * Container operations are multi-thread safe. Note that this is a requirement since tests may use this through a real
- * (multi-threaded) node-admin instance.
- *
  * @author mpolden
  */
 public class ContainerEngineMock implements ContainerEngine {
 
-    private final Map<ContainerName, Container> containers = new HashMap<>();
+    private final Map<ContainerName, Container> containers = new ConcurrentHashMap<>();
     private final Map<String, ImageDownload> images = new ConcurrentHashMap<>();
-    private final Object monitor = new Object();
-
     private boolean asyncImageDownload = false;
 
     public ContainerEngineMock asyncImageDownload(boolean enabled) {
@@ -58,72 +50,56 @@ public class ContainerEngineMock implements ContainerEngine {
     }
 
     public ContainerEngineMock addContainers(List<Container> containers) {
-        synchronized (monitor) {
-            for (var container : containers) {
-                if (this.containers.containsKey(container.name())) {
-                    throw new IllegalArgumentException("Container " + container.name() + " already exists");
-                }
-                this.containers.put(container.name(), container);
+        for (var container : containers) {
+            if (this.containers.containsKey(container.name())) {
+                throw new IllegalArgumentException("Container " + container.name() + " already exists");
             }
-            return this;
+            this.containers.put(container.name(), container);
         }
+        return this;
     }
 
     public ContainerEngineMock addContainer(Container container) {
-        synchronized (monitor) {
-            return addContainers(List.of(container));
-        }
+        return addContainers(List.of(container));
     }
 
     @Override
     public void createContainer(NodeAgentContext context, ContainerData containerData, ContainerResources containerResources) {
-        synchronized (monitor) {
-            addContainer(createContainer(context, PartialContainer.State.created, containerResources));
-        }
+        addContainer(createContainer(context, PartialContainer.State.created, containerResources));
     }
 
     @Override
     public void startContainer(NodeAgentContext context) {
-        synchronized (monitor) {
-            Container container = requireContainer(context.containerName(), PartialContainer.State.created);
-            Container newContainer = createContainer(context, PartialContainer.State.running, container.resources());
-            containers.put(newContainer.name(), newContainer);
-        }
+        Container container = requireContainer(context.containerName(), PartialContainer.State.created);
+        Container newContainer = createContainer(context, PartialContainer.State.running, container.resources());
+        containers.put(newContainer.name(), newContainer);
     }
 
     @Override
     public void removeContainer(TaskContext context, PartialContainer container) {
-        synchronized (monitor) {
-            requireContainer(container.name());
-            containers.remove(container.name());
-        }
+        requireContainer(container.name());
+        containers.remove(container.name());
     }
 
     @Override
     public void updateContainer(NodeAgentContext context, ContainerId containerId, ContainerResources containerResources) {
-        synchronized (monitor) {
-            Container container = requireContainer(context.containerName());
-            containers.put(container.name(), new Container(containerId, container.name(), container.state(),
-                                                           container.imageId(), container.image(),
-                                                           container.labels(), container.pid(),
-                                                           container.conmonPid(), container.hostname(),
-                                                           containerResources, container.networks(),
-                                                           container.managed()));
-        }
+        Container container = requireContainer(context.containerName());
+        containers.put(container.name(), new Container(containerId, container.name(), container.state(),
+                                                       container.imageId(), container.image(),
+                                                       container.labels(), container.pid(),
+                                                       container.conmonPid(), container.hostname(),
+                                                       containerResources, container.networks(),
+                                                       container.managed()));
     }
 
     @Override
     public Optional<Container> getContainer(NodeAgentContext context) {
-        synchronized (monitor) {
-            return Optional.ofNullable(containers.get(context.containerName()));
-        }
+        return Optional.ofNullable(containers.get(context.containerName()));
     }
 
     @Override
     public List<PartialContainer> listContainers(TaskContext context) {
-        synchronized (monitor) {
-            return List.copyOf(containers.values());
-        }
+        return List.copyOf(containers.values());
     }
 
     @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 04d86a69057..898d7ebe901 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
@@ -8,6 +8,8 @@ import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImpl;
 import org.junit.Test;
 
+import java.util.List;
+
 import static com.yahoo.vespa.hosted.node.admin.integration.ContainerTester.containerMatcher;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.never;
@@ -20,8 +22,8 @@ public class ContainerFailTest {
 
     @Test
     public void test() {
-        try (ContainerTester tester = new ContainerTester()) {
-            DockerImage dockerImage = DockerImage.fromString("registry.example.com/dockerImage");
+        DockerImage dockerImage = DockerImage.fromString("registry.example.com/dockerImage");
+        try (ContainerTester tester = new ContainerTester(List.of(dockerImage))) {
             ContainerName containerName = new ContainerName("host1");
             String hostname = "host1.test.yahoo.com";
             NodeSpec nodeSpec = NodeSpec.Builder
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/ContainerTester.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/ContainerTester.java
index 4179f53370b..3f2083638dc 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/ContainerTester.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integration/ContainerTester.java
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.node.admin.integration;
 
+import com.yahoo.config.provision.DockerImage;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
@@ -28,6 +29,7 @@ import java.nio.file.FileSystem;
 import java.time.Clock;
 import java.time.Duration;
 import java.util.Collections;
+import java.util.List;
 import java.util.Optional;
 import java.util.logging.Logger;
 
@@ -50,7 +52,8 @@ public class ContainerTester implements AutoCloseable {
 
     private final Thread loopThread;
 
-    final ContainerOperations containerOperations = spy(new ContainerOperations(new ContainerEngineMock(), TestFileSystem.create()));
+    private final ContainerEngineMock containerEngine = new ContainerEngineMock();
+    final ContainerOperations containerOperations = spy(new ContainerOperations(containerEngine, TestFileSystem.create()));
     final NodeRepoMock nodeRepository = spy(new NodeRepoMock());
     final Orchestrator orchestrator = mock(Orchestrator.class);
     final StorageMaintainer storageMaintainer = mock(StorageMaintainer.class);
@@ -64,7 +67,8 @@ public class ContainerTester implements AutoCloseable {
     private volatile NodeAdminStateUpdater.State wantedState = NodeAdminStateUpdater.State.RESUMED;
 
 
-    ContainerTester() {
+    ContainerTester(List<DockerImage> images) {
+        images.forEach(image -> containerEngine.pullImage(null, image, RegistryCredentials.none));
         when(storageMaintainer.diskUsageFor(any())).thenReturn(Optional.empty());
 
         IPAddressesMock ipAddresses = new IPAddressesMock();
@@ -110,6 +114,12 @@ public class ContainerTester implements AutoCloseable {
 
     /** Adds a node to node-repository mock that is running on this host */
     void addChildNodeRepositoryNode(NodeSpec nodeSpec) {
+        if (nodeSpec.wantedDockerImage().isPresent()) {
+            if (!containerEngine.hasImage(null, nodeSpec.wantedDockerImage().get())) {
+                throw new IllegalArgumentException("Want to use image " + nodeSpec.wantedDockerImage().get() +
+                                                   ", but that image does not exist in the container engine");
+            }
+        }
         nodeRepository.updateNodeRepositoryNode(new NodeSpec.Builder(nodeSpec)
                 .parentHostname(HOST_HOSTNAME.value())
                 .build());
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 86db3ae092e..fdfb3457330 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
@@ -2,12 +2,14 @@
 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.configserver.noderepository.NodeState;
+import com.yahoo.vespa.hosted.node.admin.container.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import org.junit.Test;
 
+import java.util.List;
+
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.ArgumentMatchers.eq;
@@ -19,11 +21,12 @@ public class MultiContainerTest {
 
     @Test
     public void test() {
-        try (ContainerTester tester = new ContainerTester()) {
-            DockerImage image1 = DockerImage.fromString("registry.example.com/image1");
+        DockerImage image1 = DockerImage.fromString("registry.example.com/image1");
+        DockerImage image2 = DockerImage.fromString("registry.example.com/image2");
+        try (ContainerTester tester = new ContainerTester(List.of(image1, image2))) {
             addAndWaitForNode(tester, "host1.test.yahoo.com", image1);
             NodeSpec nodeSpec2 = addAndWaitForNode(
-                    tester, "host2.test.yahoo.com", DockerImage.fromString("registry.example.com/image2"));
+                    tester, "host2.test.yahoo.com", image2);
 
             tester.addChildNodeRepositoryNode(NodeSpec.Builder.testSpec(nodeSpec2.hostname(), NodeState.dirty).build());
 
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 dad02f46d88..109fb61d0c9 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
@@ -27,7 +27,7 @@ public class RebootTest {
 
     @Test
     public void test() {
-        try (ContainerTester tester = new ContainerTester()) {
+        try (ContainerTester tester = new ContainerTester(List.of(dockerImage))) {
             tester.addChildNodeRepositoryNode(NodeSpec.Builder.testSpec(hostname).wantedDockerImage(dockerImage).build());
 
             ContainerName host1 = new ContainerName("host1");
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 160948d7996..b848a5a91d9 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
@@ -7,6 +7,8 @@ import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeAttribu
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec;
 import org.junit.Test;
 
+import java.util.List;
+
 import static com.yahoo.vespa.hosted.node.admin.integration.ContainerTester.containerMatcher;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
@@ -20,10 +22,9 @@ public class RestartTest {
 
     @Test
     public void test() {
-        try (ContainerTester tester = new ContainerTester()) {
+        DockerImage dockerImage = DockerImage.fromString("registry.example.com/dockerImage:1.2.3");
+        try (ContainerTester tester = new ContainerTester(List.of(dockerImage))) {
             String hostname = "host1.test.yahoo.com";
-            DockerImage dockerImage = DockerImage.fromString("registry.example.com/dockerImage:1.2.3");
-
             NodeSpec nodeSpec = NodeSpec.Builder.testSpec(hostname).wantedDockerImage(dockerImage).build();
             tester.addChildNodeRepositoryNode(nodeSpec);