From caf4bd2c829d8f2752d8a66c31c3d2ef074db8ad Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 12 Sep 2016 10:52:52 -0400 Subject: [PATCH] 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@2fd81b3eaf05256ebb94ca997ef5563b8590d093 --- ...IndicesBackwardsCompatibilityTestCase.java | 26 ++++++++++++++++++- .../test/SecurityIntegTestCase.java | 15 +++++++++++ ...atcherIndicesBackwardsCompatibilityIT.java | 25 ++++++++---------- 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/AbstractOldXPackIndicesBackwardsCompatibilityTestCase.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/AbstractOldXPackIndicesBackwardsCompatibilityTestCase.java index 979d79cb82c..a6dce66a7aa 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/AbstractOldXPackIndicesBackwardsCompatibilityTestCase.java +++ b/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 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 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); } diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java index 1a60747cd93..20f1015fcec 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java +++ b/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 + * and 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)) diff --git a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/OldWatcherIndicesBackwardsCompatibilityIT.java b/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/OldWatcherIndicesBackwardsCompatibilityIT.java index 953db56e9ce..aa5e6846f0e 100644 --- a/elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/xpack/watcher/OldWatcherIndicesBackwardsCompatibilityIT.java +++ b/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 {