Node-Admin: Added few node state transition tests

6 files changed, 373 insertions, 73 deletions

diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java
index d1669a847ea..81de3783bb2 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java
@@ -9,6 +9,7 @@ import com.yahoo.vespa.hosted.node.admin.docker.ProcessResult;
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
@@ -18,58 +19,80 @@ import java.util.stream.Collectors;
 
 /**
  * Mock with some simple logic
+ *
  * @author valerijf
  */
 public class DockerMock implements Docker {
-    private List<Container> containers = new ArrayList<>();
-    public static StringBuilder requests = new StringBuilder();
+    private List<Container> containers;
+    private static StringBuilder requests;
+
+    private static final Object monitor = new Object();
+
+    static {
+        reset();
+    }
 
     public DockerMock() {
-        if(OrchestratorMock.semaphore.tryAcquire()) {
+        if (OrchestratorMock.semaphore.tryAcquire()) {
             throw new RuntimeException("OrchestratorMock.semaphore must be acquired before using DockerMock");
         }
+
+        containers = new ArrayList<>();
     }
 
     @Override
     public void startContainer(DockerImage dockerImage, HostName hostName, ContainerName containerName,
                                double minCpuCores, double minDiskAvailableGb, double minMainMemoryAvailableGb) {
-        requests.append("startContainer with DockerImage: ").append(dockerImage).append(", HostName: ").append(hostName)
-                .append(", ContainerName: ").append(containerName).append(", minCpuCores: ").append(minCpuCores)
-                .append(", minDiskAvailableGb: ").append(minDiskAvailableGb).append(", minMainMemoryAvailableGb: ")
-                .append(minMainMemoryAvailableGb).append("\n");
-        containers.add(new Container(hostName, dockerImage, containerName, true));
+        synchronized (monitor) {
+            requests.append("startContainer with DockerImage: ").append(dockerImage).append(", HostName: ").append(hostName)
+                    .append(", ContainerName: ").append(containerName).append(", minCpuCores: ").append(minCpuCores)
+                    .append(", minDiskAvailableGb: ").append(minDiskAvailableGb).append(", minMainMemoryAvailableGb: ")
+                    .append(minMainMemoryAvailableGb).append("\n");
+            containers.add(new Container(hostName, dockerImage, containerName, true));
+        }
     }
 
     @Override
     public void stopContainer(ContainerName containerName) {
-        containers = containers.stream()
-                .map(container -> container.name.equals(containerName) ?
-                        new Container(container.hostname, container.image, container.name, false) : container)
-                .collect(Collectors.toList());
+        synchronized (monitor) {
+            requests.append("stopContainer with ContainerName: ").append(containerName).append("\n");
+            containers = containers.stream()
+                    .map(container -> container.name.equals(containerName) ?
+                            new Container(container.hostname, container.image, container.name, false) : container)
+                    .collect(Collectors.toList());
+        }
     }
 
     @Override
     public void deleteContainer(ContainerName containerName) {
-        requests.append("deleteContainer with ContainerName: ").append(containerName);
-        containers = containers.stream()
-                .filter(container -> !container.name.equals(containerName))
-                .collect(Collectors.toList());
+        synchronized (monitor) {
+            requests.append("deleteContainer with ContainerName: ").append(containerName).append("\n");
+            containers = containers.stream()
+                    .filter(container -> !container.name.equals(containerName))
+                    .collect(Collectors.toList());
+        }
     }
 
     @Override
     public List<Container> getAllManagedContainers() {
-        return new ArrayList<>(containers);
+        synchronized (monitor) {
+            return new ArrayList<>(containers);
+        }
     }
 
     @Override
     public Optional<Container> getContainer(HostName hostname) {
-        return containers.stream().filter(container -> container.hostname.equals(hostname)).findFirst();
+        synchronized (monitor) {
+            return containers.stream().filter(container -> container.hostname.equals(hostname)).findFirst();
+        }
     }
 
     @Override
     public CompletableFuture<DockerImage> pullImageAsync(DockerImage image) {
-        requests.append("pullImageAsync with DockerImage: ").append(image);
-        return null;
+        synchronized (monitor) {
+            requests.append("pullImageAsync with DockerImage: ").append(image);
+            return null;
+        }
     }
 
     @Override
@@ -99,7 +122,23 @@ public class DockerMock implements Docker {
 
     @Override
     public ProcessResult executeInContainer(ContainerName containerName, String... args) {
+        synchronized (monitor) {
+            requests.append("executeInContainer with ContainerName: ").append(containerName)
+                    .append(", args: ").append(Arrays.toString(args)).append("\n");
+        }
         return new ProcessResult(0, "OK");
     }
+
+    public static String getRequests() {
+        synchronized (monitor) {
+            return requests.toString();
+        }
+    }
+
+    public static void reset() {
+        synchronized (monitor) {
+            requests = new StringBuilder();
+        }
+    }
 }
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeRepoMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeRepoMock.java
index ef8a041048a..d79079261cb 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeRepoMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeRepoMock.java
@@ -2,51 +2,121 @@ package com.yahoo.vespa.hosted.node.admin.integrationTests;
 
 import com.yahoo.vespa.applicationmodel.HostName;
 import com.yahoo.vespa.hosted.node.admin.ContainerNodeSpec;
+import com.yahoo.vespa.hosted.node.admin.docker.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.docker.DockerImage;
 import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepository;
+import com.yahoo.vespa.hosted.node.admin.noderepository.NodeState;
 
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Collectors;
 
 /**
  * Mock with some simple logic
+ *
  * @author dybis
  */
 public class NodeRepoMock implements NodeRepository {
 
-    public static final List<ContainerNodeSpec> containerNodeSpecs = new ArrayList<>();
-    public static StringBuilder requests = new StringBuilder();
+    private static List<ContainerNodeSpec> containerNodeSpecs;
+    private static StringBuilder requests;
+
+    private static final Object monitor = new Object();
+
+    static {
+        reset();
+    }
 
     public NodeRepoMock() {
-        if(OrchestratorMock.semaphore.tryAcquire()) {
+        if (OrchestratorMock.semaphore.tryAcquire()) {
             throw new RuntimeException("OrchestratorMock.semaphore must be acquired before using NodeRepoMock");
         }
     }
 
     @Override
     public List<ContainerNodeSpec> getContainersToRun() throws IOException {
-        return containerNodeSpecs;
+        synchronized (monitor) {
+            return containerNodeSpecs;
+        }
     }
 
     @Override
     public Optional<ContainerNodeSpec> getContainerNodeSpec(HostName hostName) throws IOException {
-        return containerNodeSpecs.stream()
-                .filter(containerNodeSpec -> containerNodeSpec.hostname.equals(hostName))
-                .findFirst();
+        synchronized (monitor) {
+            return containerNodeSpecs.stream()
+                    .filter(containerNodeSpec -> containerNodeSpec.hostname.equals(hostName))
+                    .findFirst();
+        }
     }
 
     @Override
     public void updateNodeAttributes(HostName hostName, long restartGeneration, DockerImage dockerImage, String containerVespaVersion) throws IOException {
-        requests.append("updateNodeAttributes with HostName: ").append(hostName)
-                .append(", restartGeneration: ").append(restartGeneration)
-                .append(", DockerImage: ").append(dockerImage)
-                .append(", containerVespaVersion: ").append(containerVespaVersion).append("\n");
+        synchronized (monitor) {
+            requests.append("updateNodeAttributes with HostName: ").append(hostName)
+                    .append(", restartGeneration: ").append(restartGeneration)
+                    .append(", DockerImage: ").append(dockerImage)
+                    .append(", containerVespaVersion: ").append(containerVespaVersion).append("\n");
+        }
     }
 
     @Override
     public void markAsReady(HostName hostName) throws IOException {
+        Optional<ContainerNodeSpec> cns = getContainerNodeSpec(hostName);
+
+        synchronized (monitor) {
+            if (cns.isPresent()) {
+                updateContainerNodeSpec(cns.get().hostname,
+                        cns.get().wantedDockerImage, cns.get().containerName, NodeState.READY,
+                        cns.get().wantedRestartGeneration, cns.get().currentRestartGeneration,
+                        cns.get().minCpuCores, cns.get().minMainMemoryAvailableGb, cns.get().minDiskAvailableGb);
+            }
+            requests.append("markAsReady with HostName: ").append(hostName).append("\n");
+        }
+    }
+
+    public static void updateContainerNodeSpec(HostName hostName,
+                                               Optional<DockerImage> wantedDockerImage,
+                                               ContainerName containerName,
+                                               NodeState nodeState,
+                                               Optional<Long> wantedRestartGeneration,
+                                               Optional<Long> currentRestartGeneration,
+                                               Optional<Double> minCpuCores,
+                                               Optional<Double> minMainMemoryAvailableGb,
+                                               Optional<Double> minDiskAvailableGb) {
+        addContainerNodeSpec(new ContainerNodeSpec(hostName,
+                wantedDockerImage, containerName, nodeState,
+                wantedRestartGeneration, currentRestartGeneration,
+                minCpuCores, minMainMemoryAvailableGb, minDiskAvailableGb));
+    }
 
+    public static void addContainerNodeSpec(ContainerNodeSpec containerNodeSpec) {
+        synchronized (monitor) {
+            containerNodeSpecs = containerNodeSpecs.stream()
+                    .filter(c -> !c.hostname.equals(containerNodeSpec.hostname))
+                    .collect(Collectors.toList());
+
+            containerNodeSpecs.add(containerNodeSpec);
+        }
+    }
+
+    public static void clearContainerNodeSpecs() {
+        synchronized (monitor) {
+            containerNodeSpecs.clear();
+        }
+    }
+
+    public static String getRequests() {
+        synchronized (monitor) {
+            return requests.toString();
+        }
+    }
+
+    public static void reset() {
+        synchronized (monitor) {
+            containerNodeSpecs = new ArrayList<>();
+            requests = new StringBuilder();
+        }
     }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeStateTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeStateTest.java
new file mode 100644
index 00000000000..dba84f81cd3
--- /dev/null
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeStateTest.java
@@ -0,0 +1,168 @@
+package com.yahoo.vespa.hosted.node.admin.integrationTests;
+
+import com.yahoo.vespa.applicationmodel.HostName;
+import com.yahoo.vespa.hosted.node.admin.ContainerNodeSpec;
+import com.yahoo.vespa.hosted.node.admin.NodeAdmin;
+import com.yahoo.vespa.hosted.node.admin.NodeAdminImpl;
+import com.yahoo.vespa.hosted.node.admin.NodeAdminStateUpdater;
+import com.yahoo.vespa.hosted.node.admin.NodeAgent;
+import com.yahoo.vespa.hosted.node.admin.NodeAgentImpl;
+import com.yahoo.vespa.hosted.node.admin.docker.ContainerName;
+import com.yahoo.vespa.hosted.node.admin.docker.DockerImage;
+import com.yahoo.vespa.hosted.node.admin.noderepository.NodeState;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.Optional;
+import java.util.function.Function;
+
+import static org.hamcrest.core.Is.is;
+import static org.hamcrest.core.IsNot.not;
+import static org.hamcrest.core.StringEndsWith.endsWith;
+import static org.junit.Assert.assertThat;
+
+/**
+ * Test NodeState transitions in NodeRepository
+ *
+ * @author valerijf
+ */
+
+public class NodeStateTest {
+    private NodeRepoMock nodeRepositoryMock;
+    private DockerMock dockerMock;
+    private HostName hostName;
+    private ContainerNodeSpec initialContainerNodeSpec;
+    private NodeAdminStateUpdater updater;
+
+    @Before
+    public void before() throws InterruptedException {
+        try {
+            OrchestratorMock.semaphore.acquire();
+        } catch (InterruptedException e) {
+            e.printStackTrace();
+        }
+
+        OrchestratorMock.reset();
+        NodeRepoMock.reset();
+        DockerMock.reset();
+
+        OrchestratorMock orchestratorMock = new OrchestratorMock();
+        nodeRepositoryMock = new NodeRepoMock();
+        dockerMock = new DockerMock();
+
+        Function<HostName, NodeAgent> nodeAgentFactory = (hostName) ->
+                new NodeAgentImpl(hostName, dockerMock, nodeRepositoryMock, orchestratorMock);
+        NodeAdmin nodeAdmin = new NodeAdminImpl(dockerMock, nodeAgentFactory);
+
+        hostName = new HostName("hostName");
+        initialContainerNodeSpec = new ContainerNodeSpec(
+                hostName,
+                Optional.of(new DockerImage("dockerImage")),
+                new ContainerName("container"),
+                NodeState.ACTIVE,
+                Optional.of(1L),
+                Optional.of(1L),
+                Optional.of(1d),
+                Optional.of(1d),
+                Optional.of(1d));
+        NodeRepoMock.addContainerNodeSpec(initialContainerNodeSpec);
+
+        updater = new NodeAdminStateUpdater(nodeRepositoryMock, nodeAdmin, 1, 1, orchestratorMock, "basehostname");
+
+        // Wait for node admin to be notified with node repo state and the docker container has been started
+        while (nodeAdmin.getListOfHosts().size() == 0) {
+            Thread.sleep(10);
+        }
+
+        while (!DockerMock.getRequests().startsWith("startContainer with DockerImage: DockerImage { imageId=dockerImage }, " +
+                "HostName: hostName, ContainerName: ContainerName { name=container }, minCpuCores: 1.0, minDiskAvailableGb: 1.0, " +
+                "minMainMemoryAvailableGb: 1.0\nexecuteInContainer with ContainerName: ContainerName { name=container }, " +
+                "args: [/usr/bin/env, test, -x, /opt/vespa/bin/vespa-nodectl]\nexecuteInContainer with ContainerName: " +
+                "ContainerName { name=container }, args: [/opt/vespa/bin/vespa-nodectl, resume]\n")) {
+            Thread.sleep(10);
+        }
+    }
+
+    @After
+    public void after() {
+        updater.deconstruct();
+        OrchestratorMock.semaphore.release();
+    }
+
+
+    @Test
+    public void activeToDirty() throws InterruptedException, IOException {
+        // Change node state to dirty
+        NodeRepoMock.updateContainerNodeSpec(
+                initialContainerNodeSpec.hostname,
+                initialContainerNodeSpec.wantedDockerImage,
+                initialContainerNodeSpec.containerName,
+                NodeState.DIRTY,
+                initialContainerNodeSpec.wantedRestartGeneration,
+                initialContainerNodeSpec.currentRestartGeneration,
+                initialContainerNodeSpec.minCpuCores,
+                initialContainerNodeSpec.minMainMemoryAvailableGb,
+                initialContainerNodeSpec.minDiskAvailableGb);
+
+        // Wait until it is marked ready
+        Optional<ContainerNodeSpec> containerNodeSpec;
+        while ((containerNodeSpec = nodeRepositoryMock.getContainerNodeSpec(hostName)).isPresent()
+                && containerNodeSpec.get().nodeState != NodeState.READY) {
+            Thread.sleep(10);
+        }
+
+        assertThat(nodeRepositoryMock.getContainerNodeSpec(hostName).get().nodeState, is(NodeState.READY));
+
+
+        // Wait until docker receives deleteContainer request
+        while (!DockerMock.getRequests().endsWith("deleteContainer with ContainerName: ContainerName { name=container }\n")) {
+            Thread.sleep(10);
+        }
+
+        assertThat(DockerMock.getRequests(), endsWith("deleteContainer with ContainerName: ContainerName { name=container }\n"));
+    }
+
+
+    @Test
+    public void activeToInactiveToActive() throws InterruptedException, IOException {
+        String initialDockerRequests = DockerMock.getRequests() +
+                "stopContainer with ContainerName: ContainerName { name=container }\n" +
+                "deleteContainer with ContainerName: ContainerName { name=container }\n";
+        Optional<DockerImage> newDockerImage = Optional.of(new DockerImage("newDockerImage"));
+
+        // Change node state to inactive and change the wanted docker image
+        NodeRepoMock.updateContainerNodeSpec(
+                initialContainerNodeSpec.hostname,
+                newDockerImage,
+                initialContainerNodeSpec.containerName,
+                NodeState.INACTIVE,
+                initialContainerNodeSpec.wantedRestartGeneration,
+                initialContainerNodeSpec.currentRestartGeneration,
+                initialContainerNodeSpec.minCpuCores,
+                initialContainerNodeSpec.minMainMemoryAvailableGb,
+                initialContainerNodeSpec.minDiskAvailableGb);
+
+        Thread.sleep(1000);
+        assertThat(initialDockerRequests, is(DockerMock.getRequests()));
+
+
+        // Change node state to active
+        NodeRepoMock.updateContainerNodeSpec(
+                initialContainerNodeSpec.hostname,
+                newDockerImage,
+                initialContainerNodeSpec.containerName,
+                NodeState.ACTIVE,
+                initialContainerNodeSpec.wantedRestartGeneration,
+                initialContainerNodeSpec.currentRestartGeneration,
+                initialContainerNodeSpec.minCpuCores,
+                initialContainerNodeSpec.minMainMemoryAvailableGb,
+                initialContainerNodeSpec.minDiskAvailableGb);
+
+        while (DockerMock.getRequests().equals(initialDockerRequests)) {
+            Thread.sleep(10);
+        }
+        assertThat(initialDockerRequests, not(DockerMock.getRequests()));
+    }
+}
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/OrchestratorMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/OrchestratorMock.java
index 9c740bcdf44..c77c6fd64b6 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/OrchestratorMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/OrchestratorMock.java
@@ -3,28 +3,33 @@ package com.yahoo.vespa.hosted.node.admin.integrationTests;
 import com.yahoo.vespa.applicationmodel.HostName;
 import com.yahoo.vespa.hosted.node.admin.orchestrator.Orchestrator;
 
-import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
-import java.util.Set;
 import java.util.concurrent.Semaphore;
 
 /**
  * Mock with some simple logic
+ *
  * @author dybis
  */
 public class OrchestratorMock implements Orchestrator {
 
-    public static Set<HostName> running = new HashSet<>();
-    public static StringBuilder requests = new StringBuilder();
-    public static Optional<String> forceMultipleRequestsResponse = null;
+    private static StringBuilder requests = new StringBuilder();
+
+    private static boolean forceSingleSuspendResponse;
+    private static boolean forceSingleResumeResponse;
+    private static Optional<String> forceGroupSuspendResponse;
 
     private static final Object monitor = new Object();
 
     public static final Semaphore semaphore = new Semaphore(1);
 
+    static {
+        reset();
+    }
+
     public OrchestratorMock() {
-        if(semaphore.tryAcquire()) {
+        if (semaphore.tryAcquire()) {
             throw new RuntimeException("OrchestratorMock.semaphore must be acquired before using OrchestratorMock");
         }
     }
@@ -32,43 +37,58 @@ public class OrchestratorMock implements Orchestrator {
     @Override
     public boolean suspend(HostName hostName) {
         synchronized (monitor) {
-            requests.append("Suspend for ").append(hostName.toString()).append("\n");
-            return running.remove(hostName);
+            return forceSingleSuspendResponse;
         }
     }
 
     @Override
     public boolean resume(HostName hostName) {
         synchronized (monitor) {
-            requests.append("Resume for ").append(hostName.toString()).append("\n");
-            return running.add(hostName);
+            requests.append("Resume for ").append(hostName).append("\n");
+            return forceSingleResumeResponse;
         }
     }
 
     @Override
     public Optional<String> suspend(String parentHostName, List<String> hostNames) {
         synchronized (monitor) {
-            requests.append("Suspend with parent: ").append(parentHostName).append(" and hostnames: ").append(hostNames);
-            if (forceMultipleRequestsResponse != null) {
-                requests.append(" - Forced response: ").append(forceMultipleRequestsResponse).append("\n");
-                return forceMultipleRequestsResponse;
-            }
-
-            for (String hostName : hostNames) {
-                if (! suspend(new HostName(hostName))) {
-                    requests.append(" - Normal response: fail").append("\n");
-                    return Optional.of("Could not suspend " + hostName);
-                }
-            }
+            requests.append("Suspend with parent: ").append(parentHostName)
+                    .append(" and hostnames: ").append(hostNames)
+                    .append(" - Forced response: ").append(forceGroupSuspendResponse).append("\n");
+            return forceGroupSuspendResponse;
+        }
+    }
+
+    public static String getRequests() {
+        synchronized (monitor) {
+            return requests.toString();
+        }
+    }
+
+    public static void setForceSingleSuspendResponse(boolean forceSingleSuspendResponse) {
+        synchronized (monitor) {
+            OrchestratorMock.forceSingleSuspendResponse = forceSingleSuspendResponse;
+        }
+    }
+
+    public static void setForceSingleResumeResponse(boolean forceSingleResumeResponse) {
+        synchronized (monitor) {
+            OrchestratorMock.forceSingleResumeResponse = forceSingleResumeResponse;
         }
+    }
 
-        requests.append(" - Normal response: success").append("\n");
-        return Optional.empty();
+    public static void setForceGroupSuspendResponse(Optional<String> forceGroupSuspendResponse) {
+        synchronized (monitor) {
+            OrchestratorMock.forceGroupSuspendResponse = forceGroupSuspendResponse;
+        }
     }
 
     public static void reset() {
-        running = new HashSet<>();
-        requests = new StringBuilder();
-        forceMultipleRequestsResponse = null;
+        synchronized (monitor) {
+            requests = new StringBuilder();
+            forceSingleResumeResponse = true;
+            forceSingleSuspendResponse = true;
+            forceGroupSuspendResponse = Optional.empty();
+        }
     }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/ResumeTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/ResumeTest.java
index 5b633cf45ac..768f5964a60 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/ResumeTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/ResumeTest.java
@@ -21,10 +21,12 @@ import java.util.Optional;
 import java.util.function.Function;
 
 import static org.hamcrest.core.Is.is;
+import static org.hamcrest.core.StringStartsWith.startsWith;
 import static org.junit.Assert.assertThat;
 
 /**
  * Scenario test for NodeAdminStateUpdater.
+ *
  * @author dybis
  */
 public class ResumeTest {
@@ -36,6 +38,8 @@ public class ResumeTest {
             throw new RuntimeException(e);
         }
         OrchestratorMock.reset();
+        NodeRepoMock.reset();
+        DockerMock.reset();
     }
 
     @After
@@ -53,7 +57,7 @@ public class ResumeTest {
                 new NodeAgentImpl(hostName, dockerMock, nodeRepositoryMock, orchestratorMock);
         NodeAdmin nodeAdmin = new NodeAdminImpl(dockerMock, nodeAgentFactory);
 
-        NodeRepoMock.containerNodeSpecs.add(new ContainerNodeSpec(
+        NodeRepoMock.addContainerNodeSpec(new ContainerNodeSpec(
                 new HostName("hostName"),
                 Optional.of(new DockerImage("dockerImage")),
                 new ContainerName("container"),
@@ -71,28 +75,28 @@ public class ResumeTest {
             Thread.sleep(10);
         }
 
-        while (!DockerMock.requests.toString().startsWith("startContainer with DockerImage: DockerImage { imageId=dockerImage }, " +
+        while (!DockerMock.getRequests().startsWith("startContainer with DockerImage: DockerImage { imageId=dockerImage }, " +
                 "HostName: hostName, ContainerName: ContainerName { name=container }, minCpuCores: 1.0, " +
                 "minDiskAvailableGb: 1.0, minMainMemoryAvailableGb: 1.0\n")) {
             Thread.sleep(10);
         }
 
-        assertThat(DockerMock.requests.toString(), is("startContainer with DockerImage: DockerImage { imageId=dockerImage }, " +
+        assertThat(DockerMock.getRequests(), startsWith("startContainer with DockerImage: DockerImage { imageId=dockerImage }, " +
                 "HostName: hostName, ContainerName: ContainerName { name=container }, minCpuCores: 1.0, " +
                 "minDiskAvailableGb: 1.0, minMainMemoryAvailableGb: 1.0\n"));
 
 
         // Check that NodeRepo has received the PATCH update
-        while (!NodeRepoMock.requests.toString().startsWith("updateNodeAttributes with HostName: hostName, " +
+        while (!NodeRepoMock.getRequests().startsWith("updateNodeAttributes with HostName: hostName, " +
                 "restartGeneration: 1, DockerImage: DockerImage { imageId=dockerImage }, containerVespaVersion: null\n")) {
             Thread.sleep(10);
         }
 
-        assertThat(NodeRepoMock.requests.toString(), is("updateNodeAttributes with HostName: hostName, restartGeneration: 1," +
+        assertThat(NodeRepoMock.getRequests(), startsWith("updateNodeAttributes with HostName: hostName, restartGeneration: 1," +
                 " DockerImage: DockerImage { imageId=dockerImage }, containerVespaVersion: null\n"));
 
         // Force orchestrator to reject the suspend
-        OrchestratorMock.forceMultipleRequestsResponse = Optional.of("Orchestrator reject suspend");
+        OrchestratorMock.setForceGroupSuspendResponse(Optional.of("Orchestrator reject suspend"));
 
         // At this point NodeAdmin should be fine with the suspend and it is up to Orchestrator
         while (!updater.setResumeStateAndCheckIfResumed(NodeAdminStateUpdater.State.SUSPENDED)
@@ -102,11 +106,11 @@ public class ResumeTest {
         assertThat(updater.setResumeStateAndCheckIfResumed(NodeAdminStateUpdater.State.SUSPENDED), is(Optional.of("Orchestrator reject suspend")));
 
         //Make orchestrator allow suspend requests
-        OrchestratorMock.forceMultipleRequestsResponse = Optional.empty();
+        OrchestratorMock.setForceGroupSuspendResponse(Optional.empty());
         assertThat(updater.setResumeStateAndCheckIfResumed(NodeAdminStateUpdater.State.SUSPENDED), is(Optional.empty()));
 
         // Now, change data in node repo, should not propagate.
-        NodeRepoMock.containerNodeSpecs.clear();
+        NodeRepoMock.clearContainerNodeSpecs();
 
         // New node repo state should have not propagated to node admin
         Thread.sleep(2);
@@ -120,12 +124,11 @@ public class ResumeTest {
             Thread.sleep(1);
         }
 
-        final String[] allRequests = OrchestratorMock.requests.toString().split("\n");
+        final String[] allRequests = OrchestratorMock.getRequests().split("\n");
         final List<String> noRepeatingRequests = new ArrayList<>();
-        noRepeatingRequests.add(allRequests[0]);
-        for (int i = 1; i < allRequests.length; i++) {
-            if (!allRequests[i].equals(allRequests[i - 1])) {
-                noRepeatingRequests.add(allRequests[i]);
+        for (String request : allRequests) {
+            if (!noRepeatingRequests.contains(request)) {
+                noRepeatingRequests.add(request);
             }
         }
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java
index aa1f71c769b..e3f15e061ab 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java
@@ -107,9 +107,9 @@ public class RunInContainerTest {
     public void testGetContainersToRunAPi() throws IOException, InterruptedException {
         waitForJdiscContainerToServe();
         assertThat(doPutCall("resume"), is(true));
-        OrchestratorMock.forceMultipleRequestsResponse = Optional.of("Denied");
+        OrchestratorMock.setForceGroupSuspendResponse(Optional.of("Denied"));
         assertThat(doPutCall("suspend"), is(false));
-        assertThat(OrchestratorMock.requests.toString(), is("Suspend with parent: localhost and hostnames: [] - Forced response: Optional[Denied]\n"));
+        assertThat(OrchestratorMock.getRequests(), is("Suspend with parent: localhost and hostnames: [] - Forced response: Optional[Denied]\n"));
     }