Merge pull request #24066 from vespa-engine/hmusum/cleanup-32

Cluster controller cleanup, part 14 [run-systemtest]

5 files changed, 38 insertions, 96 deletions

diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterFeedBlockTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterFeedBlockTest.java
index 409e8932867..0485bd80aa0 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterFeedBlockTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterFeedBlockTest.java
@@ -1,8 +1,6 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.clustercontroller.core;
 
-import com.yahoo.jrt.Supervisor;
-import com.yahoo.jrt.Transport;
 import com.yahoo.vdslib.state.Node;
 import com.yahoo.vdslib.state.NodeState;
 import com.yahoo.vdslib.state.NodeType;
@@ -11,21 +9,19 @@ import com.yahoo.vespa.clustercontroller.core.database.DatabaseHandler;
 import com.yahoo.vespa.clustercontroller.core.database.ZooKeeperDatabaseFactory;
 import com.yahoo.vespa.clustercontroller.core.status.StatusHandler;
 import com.yahoo.vespa.clustercontroller.utils.util.NoMetricReporter;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
-
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import static com.yahoo.vespa.clustercontroller.core.FeedBlockUtil.createResourceUsageJson;
 import static com.yahoo.vespa.clustercontroller.core.FeedBlockUtil.mapOf;
 import static com.yahoo.vespa.clustercontroller.core.FeedBlockUtil.setOf;
 import static com.yahoo.vespa.clustercontroller.core.FeedBlockUtil.usage;
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static com.yahoo.vespa.clustercontroller.core.FeedBlockUtil.createResourceUsageJson;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -35,15 +31,9 @@ public class ClusterFeedBlockTest extends FleetControllerTest {
     private static final int NODE_COUNT = 3;
 
     // TODO dedupe fixture and setup stuff with other tests
-    private Supervisor supervisor;
     private FleetController ctrl;
     private DummyCommunicator communicator;
 
-    @BeforeEach
-    public void setUp() {
-        supervisor = new Supervisor(new Transport());
-    }
-
     private void initialize(FleetControllerOptions options) throws Exception {
         List<Node> nodes = new ArrayList<>();
         for (int i = 0; i < options.nodes().size(); ++i) {
@@ -78,14 +68,6 @@ public class ClusterFeedBlockTest extends FleetControllerTest {
         ctrl.tick();
     }
 
-    public void tearDown() throws Exception {
-        if (supervisor != null) {
-            supervisor.transport().shutdown().join();
-            supervisor = null;
-        }
-        super.tearDown();
-    }
-
     private static FleetControllerOptions createOptions(Map<String, Double> feedBlockLimits, double clusterFeedBlockNoiseLevel) {
         return defaultOptions("mycluster")
                 .setStorageDistribution(DistributionBuilder.forFlatCluster(NODE_COUNT))
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java
index cd36bf1eb37..49d04e20621 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java
@@ -104,6 +104,7 @@ public abstract class FleetControllerTest implements Waiter {
         slobrok = new Slobrok();
         if (builder.zooKeeperServerAddress() != null) {
             zooKeeperServer = new ZooKeeperTestServer();
+            // Need to set zookeeper address again, as port number is not known until ZooKeeperTestServer has been created
             builder.setZooKeeperServerAddress(zooKeeperServer.getAddress());
             log.log(Level.FINE, "Set up new zookeeper server at " + zooKeeperServer.getAddress());
         }
@@ -150,15 +151,11 @@ public abstract class FleetControllerTest implements Waiter {
     protected FleetControllerOptions setUpFleetController(boolean useFakeTimer, FleetControllerOptions.Builder builder) throws Exception {
         if (slobrok == null) setUpSystem(builder);
         options = builder.build();
-        if (fleetControllers.isEmpty()) {
-            fleetControllers.add(createFleetController(useFakeTimer, options));
-        } else {
-            throw new Exception("called setUpFleetcontroller but it was already setup");
-        }
+        startFleetController(useFakeTimer);
         return options;
     }
 
-    void stopFleetController() throws Exception {
+    void stopFleetController() {
         fleetControllers.forEach(f -> {
             try {
                 f.shutdown();
@@ -170,25 +167,26 @@ public abstract class FleetControllerTest implements Waiter {
     }
 
     void startFleetController(boolean useFakeTimer) throws Exception {
-        if (fleetControllers.isEmpty()) {
-            fleetControllers.add(createFleetController(useFakeTimer, options));
-        } else {
-            log.log(Level.WARNING, "already started fleetcontroller, not starting another");
-        }
+        if ( ! fleetControllers.isEmpty()) throw new IllegalStateException("already started fleetcontroller, not starting another");
+
+        fleetControllers.add(createFleetController(useFakeTimer, options));
     }
 
     protected void setUpVdsNodes(boolean useFakeTimer) throws Exception {
         setUpVdsNodes(useFakeTimer, false);
     }
+
     protected void setUpVdsNodes(boolean useFakeTimer, boolean startDisconnected) throws Exception {
         setUpVdsNodes(useFakeTimer, startDisconnected, DEFAULT_NODE_COUNT);
     }
+
     protected void setUpVdsNodes(boolean useFakeTimer, boolean startDisconnected, int nodeCount) throws Exception {
         TreeSet<Integer> nodeIndexes = new TreeSet<>();
         for (int i = 0; i < nodeCount; ++i)
             nodeIndexes.add(this.nodes.size()/2 + i); // divide by 2 because there are 2 nodes (storage and distributor) per index
         setUpVdsNodes(useFakeTimer, startDisconnected, nodeIndexes);
     }
+
     protected void setUpVdsNodes(boolean useFakeTimer, boolean startDisconnected, Set<Integer> nodeIndexes) throws Exception {
         for (int nodeIndex : nodeIndexes) {
             nodes.add(createNode(useFakeTimer, startDisconnected, DISTRIBUTOR, nodeIndex));
@@ -254,7 +252,8 @@ public abstract class FleetControllerTest implements Waiter {
         subsetWaiter.waitForState(expectedState);
     }
 
-    protected void tearDownSystem() {
+    @AfterEach
+    public void tearDown() {
         if (testName != null) {
             //log.log(Level.INFO, "STOPPING TEST " + testName);
             System.err.println("STOPPING TEST " + testName);
@@ -281,11 +280,6 @@ public abstract class FleetControllerTest implements Waiter {
         }
     }
 
-    @AfterEach
-    public void tearDown() throws Exception {
-        tearDownSystem();
-    }
-
     public ClusterState waitForStableSystem() throws Exception { return waiter.waitForStableSystem(); }
     public ClusterState waitForStableSystem(int nodeCount) throws Exception { return waiter.waitForStableSystem(nodeCount); }
     public ClusterState waitForState(String state) throws Exception { return waiter.waitForState(state); }
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java
index 0631485bba8..4b566bd2aab 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java
@@ -35,8 +35,6 @@ public class MasterElectionTest extends FleetControllerTest {
     private static final Logger log = Logger.getLogger(MasterElectionTest.class.getName());
     private static int defaultZkSessionTimeoutInMillis() { return 30_000; }
 
-    private Supervisor supervisor;
-
     protected void setUpFleetControllers(int count, boolean useFakeTimer, FleetControllerOptions.Builder builder) throws Exception {
         if (zooKeeperServer == null) {
             zooKeeperServer = new ZooKeeperTestServer();
@@ -86,13 +84,6 @@ public class MasterElectionTest extends FleetControllerTest {
         }
     }
 
-    public void tearDown() throws Exception {
-        if (supervisor != null) {
-            supervisor.transport().shutdown().join();
-        }
-        super.tearDown();
-    }
-
     @Test
     void testMasterElection() throws Exception {
         startingTest("MasterElectionTest::testMasterElection");
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java
index f9eda9040d2..e61bdd566c3 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java
@@ -44,15 +44,6 @@ public class RpcServerTest extends FleetControllerTest {
 
     public static Logger log = Logger.getLogger(RpcServerTest.class.getName());
 
-    private Supervisor supervisor;
-
-    public void tearDown() throws Exception {
-        if (supervisor != null) {
-            supervisor.transport().shutdown().join();
-        }
-        super.tearDown();
-    }
-
     @Test
     void testRebinding() throws Exception {
         startingTest("RpcServerTest::testRebinding");
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateChangeTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateChangeTest.java
index af6e2d2b104..5868ad723a4 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateChangeTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateChangeTest.java
@@ -1,8 +1,6 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.clustercontroller.core;
 
-import com.yahoo.jrt.Supervisor;
-import com.yahoo.jrt.Transport;
 import com.yahoo.vdslib.distribution.ConfiguredNode;
 import com.yahoo.vdslib.state.ClusterState;
 import com.yahoo.vdslib.state.Node;
@@ -14,10 +12,8 @@ import com.yahoo.vespa.clustercontroller.core.database.ZooKeeperDatabaseFactory;
 import com.yahoo.vespa.clustercontroller.core.status.StatusHandler;
 import com.yahoo.vespa.clustercontroller.core.testutils.StateWaiter;
 import com.yahoo.vespa.clustercontroller.utils.util.NoMetricReporter;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
-
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -27,22 +23,18 @@ import java.util.logging.Logger;
 
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 @ExtendWith(CleanupZookeeperLogsOnSuccess.class)
 public class StateChangeTest extends FleetControllerTest {
 
     public static Logger log = Logger.getLogger(StateChangeTest.class.getName());
-    private Supervisor supervisor;
     private FleetController ctrl;
     private DummyCommunicator communicator;
     private EventLog eventLog;
 
-    @BeforeEach
-    public void setUp() {
-        supervisor = new Supervisor(new Transport());
-    }
-
     private void initialize(FleetControllerOptions options) throws Exception {
         List<Node> nodes = new ArrayList<>();
         for (int i = 0; i < options.nodes().size(); ++i) {
@@ -79,14 +71,6 @@ public class StateChangeTest extends FleetControllerTest {
         ctrl.tick();
     }
 
-    public void tearDown() throws Exception {
-        if (supervisor != null) {
-            supervisor.transport().shutdown().join();
-            supervisor = null;
-        }
-        super.tearDown();
-    }
-
     private void verifyNodeEvents(Node n, String correct) {
         String actual = "";
         for (NodeEvent e : eventLog.getNodeEvents(n)) {
@@ -105,7 +89,7 @@ public class StateChangeTest extends FleetControllerTest {
 
     @Test
     void testNormalStartup() throws Exception {
-        FleetControllerOptions.Builder options = defaultOptions("mycluster", createNodes(10));
+        FleetControllerOptions.Builder options = defaultOptions();
         options.setMaxInitProgressTime(50000);
 
         initialize(options.build());
@@ -177,7 +161,7 @@ public class StateChangeTest extends FleetControllerTest {
 
     @Test
     void testNodeGoingDownAndUp() throws Exception {
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10))
+        FleetControllerOptions.Builder builder = defaultOptions()
                 .setNodeStateRequestTimeoutMS(60 * 60 * 1000)
                 .setMinTimeBetweenNewSystemStates(0)
                 .setMaxInitProgressTime(50000)
@@ -267,7 +251,7 @@ public class StateChangeTest extends FleetControllerTest {
     @Test
     void testNodeGoingDownAndUpNotifying() throws Exception {
         // Same test as above, but node manages to notify why it is going down first.
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10))
+        FleetControllerOptions.Builder builder = defaultOptions()
                 .setNodeStateRequestTimeoutMS(60 * 60 * 1000)
                 .setMaxSlobrokDisconnectGracePeriod(100000);
 
@@ -338,7 +322,7 @@ public class StateChangeTest extends FleetControllerTest {
 
     @Test
     void testNodeGoingDownAndUpFast() throws Exception {
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10))
+        FleetControllerOptions.Builder builder = defaultOptions()
                 .setMaxSlobrokDisconnectGracePeriod(60 * 1000);
 
         initialize(builder.build());
@@ -379,7 +363,7 @@ public class StateChangeTest extends FleetControllerTest {
 
     @Test
     void testMaintenanceWhileNormalStorageNodeRestart() throws Exception {
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10))
+        FleetControllerOptions.Builder builder = defaultOptions()
                 .setMaxSlobrokDisconnectGracePeriod(60 * 1000);
 
         initialize(builder.build());
@@ -520,7 +504,7 @@ public class StateChangeTest extends FleetControllerTest {
     @Test
     void testDownNodeInitializing() throws Exception {
         // Actually report initializing state if node has been down steadily for a while
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10))
+        FleetControllerOptions.Builder builder = defaultOptions()
                 .setMaxTransitionTime(NodeType.STORAGE, 5000)
                 .setMaxInitProgressTime(5000)
                 .setStableStateTimePeriod(20000)
@@ -583,7 +567,7 @@ public class StateChangeTest extends FleetControllerTest {
     @Test
     void testNodeInitializationStalled() throws Exception {
         // Node should eventually be marked down, and not become initializing next time, but stay down until up
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10))
+        FleetControllerOptions.Builder builder = defaultOptions()
                 .setMaxTransitionTime(NodeType.STORAGE, 5000)
                 .setMaxInitProgressTime(5000)
                 .setStableStateTimePeriod(1000000)
@@ -669,7 +653,7 @@ public class StateChangeTest extends FleetControllerTest {
     @Test
     void testBackwardsInitializationProgress() throws Exception {
         // Same as stalled. Mark down, keep down until up
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10));
+        FleetControllerOptions.Builder builder = defaultOptions();
         builder.setMaxTransitionTime(NodeType.STORAGE, 5000);
         builder.setMaxInitProgressTime(5000);
         builder.setStableStateTimePeriod(1000000);
@@ -712,7 +696,7 @@ public class StateChangeTest extends FleetControllerTest {
     @Test
     void testNodeGoingDownWhileInitializing() throws Exception {
         // Same as stalled. Mark down, keep down until up
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10))
+        FleetControllerOptions.Builder builder = defaultOptions()
                 .setMaxTransitionTime(NodeType.STORAGE, 5000)
                 .setMaxInitProgressTime(5000)
                 .setStableStateTimePeriod(1000000)
@@ -772,7 +756,7 @@ public class StateChangeTest extends FleetControllerTest {
     void testContinuousCrashRightAfterInit() throws Exception {
         startingTest("StateChangeTest::testContinuousCrashRightAfterInit");
         // If node does this too many times, take it out of service
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10))
+        FleetControllerOptions.Builder builder = defaultOptions()
                 .setMaxTransitionTime(NodeType.STORAGE, 5000)
                 .setMaxInitProgressTime(5000)
                 .setMaxPrematureCrashes(2)
@@ -826,7 +810,7 @@ public class StateChangeTest extends FleetControllerTest {
     void testClusterStateMinNodes() throws Exception {
         startingTest("StateChangeTest::testClusterStateMinNodes");
         // If node does this too many times, take it out of service
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10))
+        FleetControllerOptions.Builder builder = defaultOptions()
                 .setMaxTransitionTime(NodeType.STORAGE, 0)
                 .setMaxInitProgressTime(0)
                 .setMinDistributorNodesUp(6)
@@ -881,7 +865,7 @@ public class StateChangeTest extends FleetControllerTest {
     void testClusterStateMinFactor() throws Exception {
         startingTest("StateChangeTest::testClusterStateMinFactor");
         // If node does this too many times, take it out of service
-        FleetControllerOptions.Builder options = defaultOptions("mycluster", createNodes(10));
+        FleetControllerOptions.Builder options = defaultOptions();
         options.setMaxTransitionTime(NodeType.STORAGE, 0);
         options.setMaxInitProgressTime(0);
         options.setMinDistributorNodesUp(0);
@@ -954,8 +938,8 @@ public class StateChangeTest extends FleetControllerTest {
     @Test
     void testNoSystemStateBeforeInitialTimePeriod() throws Exception {
         startingTest("StateChangeTest::testNoSystemStateBeforeInitialTimePeriod()");
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10));
-        builder.setMinTimeBeforeFirstSystemStateBroadcast(3 * 60 * 1000);
+        FleetControllerOptions.Builder builder = defaultOptions()
+                .setMinTimeBeforeFirstSystemStateBroadcast(3 * 60 * 1000);
         setUpSystem(builder);
         boolean useFakeTimer = true;
         setUpVdsNodes(useFakeTimer, true);
@@ -1000,8 +984,8 @@ public class StateChangeTest extends FleetControllerTest {
     @Test
     void testSystemStateSentWhenNodesReplied() throws Exception {
         startingTest("StateChangeTest::testSystemStateSentWhenNodesReplied()");
-        FleetControllerOptions.Builder builder = defaultOptions("mycluster", createNodes(10));
-        builder.setMinTimeBeforeFirstSystemStateBroadcast(300 * 60 * 1000);
+        FleetControllerOptions.Builder builder = defaultOptions()
+                .setMinTimeBeforeFirstSystemStateBroadcast(300 * 60 * 1000);
 
         boolean useFakeTimer = true;
         setUpSystem(builder);
@@ -1038,7 +1022,7 @@ public class StateChangeTest extends FleetControllerTest {
     @Test
     void testDontTagFailingSetSystemStateOk() throws Exception {
         startingTest("StateChangeTest::testDontTagFailingSetSystemStateOk()");
-        FleetControllerOptions.Builder options = defaultOptions("mycluster", createNodes(10));
+        FleetControllerOptions.Builder options = defaultOptions();
         setUpFleetController(true, options);
         setUpVdsNodes(true);
         waitForStableSystem();
@@ -1069,7 +1053,7 @@ public class StateChangeTest extends FleetControllerTest {
     @Test
     void testAlteringDistributionSplitCount() throws Exception {
         startingTest("StateChangeTest::testAlteringDistributionSplitCount");
-        FleetControllerOptions.Builder options = defaultOptions("mycluster", createNodes(10));
+        FleetControllerOptions.Builder options = defaultOptions();
         options.setDistributionBits(17);
 
         initialize(options.build());
@@ -1116,7 +1100,7 @@ public class StateChangeTest extends FleetControllerTest {
     @Test
     void testSetAllTimestampsAfterDowntime() throws Exception {
         startingTest("StateChangeTest::testSetAllTimestampsAfterDowntime");
-        FleetControllerOptions.Builder options = defaultOptions("mycluster", createNodes(10));
+        FleetControllerOptions.Builder options = defaultOptions();
         setUpFleetController(true, options);
         setUpVdsNodes(true);
         waitForStableSystem();
@@ -1165,7 +1149,7 @@ public class StateChangeTest extends FleetControllerTest {
 
     @Test
     void consolidated_cluster_state_reflects_node_changes_when_cluster_is_down() throws Exception {
-        FleetControllerOptions.Builder options = defaultOptions("mycluster", createNodes(10));
+        FleetControllerOptions.Builder options = defaultOptions();
         options.setMaxTransitionTime(NodeType.STORAGE, 0);
         options.setMinStorageNodesUp(10);
         options.setMinDistributorNodesUp(10);
@@ -1199,7 +1183,7 @@ public class StateChangeTest extends FleetControllerTest {
     // of previous timer invocations (with subsequent state generation) would not be visible.
     @Test
     void timer_events_during_cluster_down_observe_most_recent_node_changes() throws Exception {
-        FleetControllerOptions.Builder options = defaultOptions("mycluster", createNodes(10));
+        FleetControllerOptions.Builder options = defaultOptions();
         options.setMaxTransitionTime(NodeType.STORAGE, 1000);
         options.setMinStorageNodesUp(10);
         options.setMinDistributorNodesUp(10);
@@ -1234,7 +1218,7 @@ public class StateChangeTest extends FleetControllerTest {
 
     @Test
     void do_not_emit_multiple_events_when_node_state_does_not_match_versioned_state() throws Exception {
-        FleetControllerOptions.Builder options = defaultOptions("mycluster", createNodes(10));
+        FleetControllerOptions.Builder options = defaultOptions();
         initialize(options.build());
 
         ctrl.tick();
@@ -1409,7 +1393,7 @@ public class StateChangeTest extends FleetControllerTest {
     }
 
     private static FleetControllerOptions.Builder optionsWithZeroTransitionTime() {
-        FleetControllerOptions.Builder options = defaultOptions("mycluster", createNodes(10));
+        FleetControllerOptions.Builder options = defaultOptions();
         options.setMaxTransitionTime(NodeType.STORAGE, 0);
         return options;
     }