Synchronize with node agent ticks, and shutdown, in ContainerTester

1 files changed, 43 insertions, 30 deletions

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 04b03afa7aa..fd93252f3b5 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
@@ -33,13 +33,15 @@ import java.time.Duration;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.Phaser;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.logging.Logger;
 
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.timeout;
 import static org.mockito.Mockito.when;
 
 /**
@@ -49,10 +51,10 @@ import static org.mockito.Mockito.when;
 public class ContainerTester implements AutoCloseable {
 
     private static final Logger log = Logger.getLogger(ContainerTester.class.getName());
-    private static final Duration INTERVAL = Duration.ofMillis(10);
     static final HostName HOST_HOSTNAME = HostName.from("host.test.yahoo.com");
 
     private final Thread loopThread;
+    private final Phaser phaser = new Phaser(1);
 
     private final ContainerEngineMock containerEngine = new ContainerEngineMock();
     private final FileSystem fileSystem = TestFileSystem.create();
@@ -66,7 +68,6 @@ public class ContainerTester implements AutoCloseable {
     final NodeAdminStateUpdater nodeAdminStateUpdater;
     final NodeAdminImpl nodeAdmin;
 
-    private boolean terminated = false;
     private volatile NodeAdminStateUpdater.State wantedState = NodeAdminStateUpdater.State.RESUMED;
 
 
@@ -86,11 +87,20 @@ public class ContainerTester implements AutoCloseable {
         Metrics metrics = new Metrics();
         FileSystem fileSystem = TestFileSystem.create();
 
-        NodeAgentFactory nodeAgentFactory = (contextSupplier, nodeContext) -> new NodeAgentImpl(
-                contextSupplier, nodeRepository, orchestrator, containerOperations, () -> RegistryCredentials.none,
-                storageMaintainer, flagSource,
-                Collections.emptyList(), Optional.empty(), Optional.empty(), clock, Duration.ofSeconds(-1),
-                VespaServiceDumper.DUMMY_INSTANCE);
+        NodeAgentFactory nodeAgentFactory = (contextSupplier, nodeContext) ->
+                new NodeAgentImpl(contextSupplier, nodeRepository, orchestrator, containerOperations, () -> RegistryCredentials.none,
+                                  storageMaintainer, flagSource,
+                                  Collections.emptyList(), Optional.empty(), Optional.empty(), clock, Duration.ofSeconds(-1),
+                                  VespaServiceDumper.DUMMY_INSTANCE) {
+                    @Override public void converge(NodeAgentContext context) {
+                        super.converge(context);
+                        phaser.arriveAndAwaitAdvance();
+                    }
+                    @Override public void stopForHostSuspension(NodeAgentContext context) {
+                        super.stopForHostSuspension(context);
+                        phaser.arriveAndDeregister();
+                    }
+             };
         nodeAdmin = new NodeAdminImpl(nodeAgentFactory, metrics, clock, Duration.ofMillis(10), Duration.ZERO);
         NodeAgentContextFactory nodeAgentContextFactory = (nodeSpec, acl) ->
                 NodeAgentContextImpl.builder(nodeSpec).acl(acl).fileSystem(fileSystem).build();
@@ -99,19 +109,14 @@ public class ContainerTester implements AutoCloseable {
 
         loopThread = new Thread(() -> {
             nodeAdminStateUpdater.start();
-
-            while (! terminated) {
+            while ( !phaser.isTerminated()) {
                 try {
                     nodeAdminStateUpdater.converge(wantedState);
                 } catch (RuntimeException e) {
                     log.info(e.getMessage());
                 }
-                try {
-                    Thread.sleep(INTERVAL.toMillis());
-                } catch (InterruptedException e) {
-                    e.printStackTrace();
-                }
             }
+            nodeAdminStateUpdater.stop();
         });
         loopThread.start();
     }
@@ -124,6 +129,10 @@ public class ContainerTester implements AutoCloseable {
                                                    ", but that image does not exist in the container engine");
             }
         }
+
+        if (nodeRepository.getOptionalNode(nodeSpec.hostname()).isEmpty())
+            phaser.register();
+
         nodeRepository.updateNodeSpec(new NodeSpec.Builder(nodeSpec)
                 .parentHostname(HOST_HOSTNAME.value())
                 .build());
@@ -134,10 +143,19 @@ public class ContainerTester implements AutoCloseable {
     }
 
     <T> T inOrder(T t) {
-        // Should probably wait for one NodeAdminStateUpdater tick & one node agent tick
-        // instead of waiting up to a timeout, but there are currently no guarantees on the node
-        // agent ticks.
-        return inOrder.verify(t, timeout(60000));
+        waitSomeTicks();
+        return inOrder.verify(t);
+    }
+
+    void waitSomeTicks() {
+        try {
+            // 3 is enough for everyone! (Well, maybe not for all eternity ...)
+            for (int i = 0; i < 3; i++)
+                phaser.awaitAdvanceInterruptibly(phaser.arrive(), 1000, TimeUnit.MILLISECONDS);
+        }
+        catch (InterruptedException | TimeoutException e) {
+            throw new RuntimeException(e);
+        }
     }
 
     public static NodeAgentContext containerMatcher(ContainerName containerName) {
@@ -146,17 +164,12 @@ public class ContainerTester implements AutoCloseable {
 
     @Override
     public void close() {
-        // First, stop NodeAdmin and all the NodeAgents
-        nodeAdminStateUpdater.stop();
-
-        terminated = true;
-        do {
-            try {
-                loopThread.join();
-            } catch (InterruptedException e) {
-                e.printStackTrace();
-            }
-        } while (loopThread.isAlive());
+        phaser.forceTermination();
+        try {
+            loopThread.join();
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+        }
     }
 
 }