Be careful when old index tests start nodes

We were starting nodes at weird times and then shutting them down again,
slowing down the tests and causing the watcher tests to fail because
watcher wasn't being shut down with its traditional kid gloves.

Original commit: elastic/x-pack-elasticsearch@2fd81b3eaf

elasticsearch/x-pack/security/src/test/java/org/elasticsearch/AbstractOldXPackIndicesBackwardsCompatibilityTestCase.java

@@ -35,13 +35,32 @@ import static org.elasticsearch.test.OldIndexUtils.loadDataFilesList;
  */
 @ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0, numClientNodes = 0) // We'll start the nodes manually
 public abstract class AbstractOldXPackIndicesBackwardsCompatibilityTestCase extends SecurityIntegTestCase {
-    protected List<String> dataFiles;
+    /**
+     * Set to true when it is ok to start a node. We don't want to start nodes at unexpected times.
+     */
+    private boolean okToStartNode = false;
+    private List<String> dataFiles;
 
     @Override
     protected final boolean ignoreExternalCluster() {
         return true;
     }
 
+    @Override
+    protected boolean shouldAssertXPackIsInstalled() {
+        return false; // Skip asserting that the xpack is installed because it tries to start the cluter.
+    }
+
+    @Override
+    protected void ensureClusterSizeConsistency() {
+        // We manage our nodes ourselves. At this point no node should be running anyway and this would start a new one!
+    }
+
+    @Override
+    protected void ensureClusterStateConsistency() throws IOException {
+        // We manage our nodes ourselves. At this point no node should be running anyway and this would start a new one!
+    }
+
     @Before
     public final void initIndexesList() throws Exception {
         dataFiles = loadDataFilesList("x-pack", getBwcIndicesPath());
@@ -49,6 +68,9 @@ public abstract class AbstractOldXPackIndicesBackwardsCompatibilityTestCase exte
 
     @Override
     public Settings nodeSettings(int ord) {
+        if (false == okToStartNode) {
+            throw new IllegalStateException("Starting nodes must only happen in setupCluster");
+        }
         // speed up recoveries
         return Settings.builder()
                 .put(super.nodeSettings(ord))
@@ -152,7 +174,9 @@ public abstract class AbstractOldXPackIndicesBackwardsCompatibilityTestCase exte
 
         // start the node
         logger.info("--> Data path for importing node: {}", dataPath);
+        okToStartNode = true;
         String importingNodeName = internalCluster().startNode(nodeSettings.build());
+        okToStartNode = false;
         Path[] nodePaths = internalCluster().getInstance(NodeEnvironment.class, importingNodeName).nodeDataPaths();
         assertEquals(1, nodePaths.length);
     }

elasticsearch/x-pack/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java

@@ -5,6 +5,7 @@
  */
 package org.elasticsearch.test;
 
+import org.elasticsearch.AbstractOldXPackIndicesBackwardsCompatibilityTestCase;
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
 import org.elasticsearch.action.admin.cluster.node.info.NodeInfo;
 import org.elasticsearch.action.admin.cluster.node.info.NodesInfoResponse;
@@ -159,6 +160,9 @@ public abstract class SecurityIntegTestCase extends ESIntegTestCase {
     @Before
     //before methods from the superclass are run before this, which means that the current cluster is ready to go
     public void assertXPackIsInstalled() {
+        if (false == shouldAssertXPackIsInstalled()) {
+            return;
+        }
         NodesInfoResponse nodeInfos = client().admin().cluster().prepareNodesInfo().clear().setPlugins(true).get();
         for (NodeInfo nodeInfo : nodeInfos.getNodes()) {
             // TODO: disable this assertion for now, due to random runs with mock plugins. perhaps run without mock plugins?
@@ -170,6 +174,17 @@ public abstract class SecurityIntegTestCase extends ESIntegTestCase {
         }
     }
 
+    /**
+     * Should this test assert that x-pack is installed? You might want to skip this assertion if the test itself validates the installation
+     * <strong>and</strong> running the assertion would significantly affect the performance or function of the test. For example
+     * {@link AbstractOldXPackIndicesBackwardsCompatibilityTestCase} disables the assertion because the assertion would force starting a
+     * node which it will then just shut down. That would slow the test down significantly and causes spurious failures because watcher
+     * needs to be shut down with kid gloves.
+     */
+    protected boolean shouldAssertXPackIsInstalled() {
+        return true;
+    }
+
     @Override
     protected Settings nodeSettings(int nodeOrdinal) {
         return Settings.builder().put(super.nodeSettings(nodeOrdinal))

elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/OldWatcherIndicesBackwardsCompatibilityIT.java

@@ -21,7 +21,6 @@ import org.elasticsearch.xpack.watcher.transport.actions.put.PutWatchResponse;
 import org.elasticsearch.xpack.watcher.trigger.schedule.IntervalSchedule;
 import org.elasticsearch.xpack.watcher.trigger.schedule.IntervalSchedule.Interval;
 import org.elasticsearch.xpack.watcher.trigger.schedule.ScheduleTrigger;
-import org.junit.After;
 
 import java.util.Map;
 
@@ -33,7 +32,6 @@ import static org.hamcrest.Matchers.not;
 /**
  * Tests for watcher indexes created before 5.0.
  */
-@LuceneTestCase.AwaitsFix(bugUrl = "leaks trigger engine threads")
 public class OldWatcherIndicesBackwardsCompatibilityIT extends AbstractOldXPackIndicesBackwardsCompatibilityTestCase {
     @Override
     public Settings nodeSettings(int ord) {
@@ -43,24 +41,23 @@ public class OldWatcherIndicesBackwardsCompatibilityIT extends AbstractOldXPackI
                 .build();
     }
 
-    @After
-    public void shutDownWatcher() throws Exception {
-        // Wait for watcher to fully start before shutting down
-        assertBusy(() -> {
-            assertEquals(WatcherState.STARTED, internalCluster().getInstance(WatcherService.class).state());
-        });
-        // Shutdown watcher on the last node so that the test can shutdown cleanly
-        internalCluster().getInstance(WatcherLifeCycleService.class).stop();
-    }
-
     @Override
     protected void checkVersion(Version version) throws Exception {
         // Wait for watcher to actually start....
         assertBusy(() -> {
             assertEquals(WatcherState.STARTED, internalCluster().getInstance(WatcherService.class).state());
         });
-        assertWatchIndexContentsWork(version);
-        assertBasicWatchInteractions();
+        try {
+            assertWatchIndexContentsWork(version);
+            assertBasicWatchInteractions();
+        } finally {
+            /* Shut down watcher after every test because watcher can be a bit finicky about shutting down when the node shuts down. This
+             * makes super sure it shuts down *and* causes the test to fail in a sensible spot if it doesn't shut down. */
+            internalCluster().getInstance(WatcherLifeCycleService.class).stop();
+            assertBusy(() -> {
+                assertEquals(WatcherState.STOPPED, internalCluster().getInstance(WatcherService.class).state());
+            });
+        }
     }
 
     void assertWatchIndexContentsWork(Version version) throws Exception {