From 4c9bfcdc5216a6c037812f941edc9fd5885fa029 Mon Sep 17 00:00:00 2001 From: javanna Date: Tue, 20 Dec 2016 15:53:35 +0100 Subject: [PATCH 1/2] [TEST] Improve ESSingleNodeTestCase repeatability If we conditionally do random things, e.g. initialize a node only after the first test, we have to make sure that we fork the Random instance by calling random.nextLong() unconditionally, otherwise tests with same seed may not be repeatable. There is one problem left out of this PR, in IngestActionForwarder, relates to #22282 . That is the only reason left why subclasses of ESSingleNodeTestCase are not repeatable yet. --- .../test/ESSingleNodeTestCase.java | 78 ++++++++----------- 1 file changed, 32 insertions(+), 46 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java index 0b2adfa52e1bf..8e1cc205a992a 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java @@ -18,13 +18,6 @@ */ package org.elasticsearch.test; -import java.io.IOException; -import java.nio.file.Path; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; - import org.apache.lucene.util.IOUtils; import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder; @@ -43,6 +36,7 @@ import org.elasticsearch.common.util.concurrent.EsExecutors; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.env.Environment; +import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexService; import org.elasticsearch.indices.IndicesService; @@ -55,11 +49,17 @@ import org.elasticsearch.test.discovery.TestZenDiscovery; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.MockTcpTransportPlugin; -import org.junit.After; import org.junit.AfterClass; -import org.junit.Before; import org.junit.BeforeClass; +import java.io.IOException; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Random; + import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.lessThanOrEqualTo; @@ -72,62 +72,55 @@ public abstract class ESSingleNodeTestCase extends ESTestCase { private static Node NODE = null; - private void reset() throws IOException { - assert NODE != null; - stopNode(); - startNode(); - } - - protected void startNode() { + protected void startNode(Random random) { assert NODE == null; - NODE = newNode(); + NODE = newNode(random); // we must wait for the node to actually be up and running. otherwise the node might have started, elected itself master but might not yet have removed the // SERVICE_UNAVAILABLE/1/state not recovered / initialized block ClusterHealthResponse clusterHealthResponse = client().admin().cluster().prepareHealth().setWaitForGreenStatus().get(); assertFalse(clusterHealthResponse.isTimedOut()); client().admin().indices() - .preparePutTemplate("random_index_template") + .preparePutTemplate("one_shard_index_template") .setPatterns(Collections.singletonList("*")) .setOrder(0) .setSettings(Settings.builder().put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0)).get(); } - protected static void stopNode() throws IOException { + private static void stopNode() throws IOException { Node node = NODE; NODE = null; IOUtils.close(node); } - private void cleanup(boolean resetNode) throws IOException { - assertAcked(client().admin().indices().prepareDelete("*").get()); - if (resetNode) { - reset(); - } - MetaData metaData = client().admin().cluster().prepareState().get().getState().getMetaData(); - assertThat("test leaves persistent cluster metadata behind: " + metaData.persistentSettings().getAsMap(), - metaData.persistentSettings().getAsMap().size(), equalTo(0)); - assertThat("test leaves transient cluster metadata behind: " + metaData.transientSettings().getAsMap(), - metaData.transientSettings().getAsMap().size(), equalTo(0)); - } - - @Before @Override public void setUp() throws Exception { super.setUp(); + //this random instance has to be created here to ensure tests repeatability + Random random = new Random(random().nextLong()); // Create the node lazily, on the first test. This is ok because we do not randomize any settings, // only the cluster name. This allows us to have overridden properties for plugins and the version to use. if (NODE == null) { - startNode(); + startNode(random); } } - @After @Override public void tearDown() throws Exception { logger.info("[{}#{}]: cleaning up after test", getTestClass().getSimpleName(), getTestName()); super.tearDown(); - cleanup(resetNodeAfterTest()); + assertAcked(client().admin().indices().prepareDelete("*").get()); + MetaData metaData = client().admin().cluster().prepareState().get().getState().getMetaData(); + assertThat("test leaves persistent cluster metadata behind: " + metaData.persistentSettings().getAsMap(), + metaData.persistentSettings().getAsMap().size(), equalTo(0)); + assertThat("test leaves transient cluster metadata behind: " + metaData.transientSettings().getAsMap(), + metaData.transientSettings().getAsMap().size(), equalTo(0)); + if (resetNodeAfterTest()) { + //this random instance has to be created here to ensure tests repeatability + assert NODE != null; + stopNode(); + startNode(new Random(random().nextLong())); + } } @BeforeClass @@ -149,7 +142,6 @@ protected boolean resetNodeAfterTest() { return false; } - /** The plugin classes that should be added to the node. */ protected Collection> getPlugins() { return Collections.emptyList(); @@ -167,16 +159,16 @@ protected Settings nodeSettings() { return Settings.EMPTY; } - private Node newNode() { + private Node newNode(Random random) { final Path tempDir = createTempDir(); Settings settings = Settings.builder() - .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", randomLong())) + .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", random.nextLong())) .put(Environment.PATH_HOME_SETTING.getKey(), tempDir) .put(Environment.PATH_REPO_SETTING.getKey(), tempDir.resolve("repo")) // TODO: use a consistent data path for custom paths // This needs to tie into the ESIntegTestCase#indexSettings() method .put(Environment.PATH_SHARED_DATA_SETTING.getKey(), createTempDir().getParent()) - .put("node.name", nodeName()) + .put("node.name", "node_s_0") .put("script.inline", "true") .put("script.stored", "true") .put(ScriptService.SCRIPT_MAX_COMPILATIONS_PER_MINUTE.getKey(), 1000) @@ -184,6 +176,7 @@ private Node newNode() { .put(NetworkModule.HTTP_ENABLED.getKey(), false) .put("transport.type", MockTcpTransportPlugin.MOCK_TCP_TRANSPORT_NAME) .put(Node.NODE_DATA_SETTING.getKey(), true) + .put(NodeEnvironment.NODE_ID_SEED_SETTING.getKey(), random.nextLong()) .put(nodeSettings()) // allow test cases to provide their own settings or override these .build(); Collection> plugins = getPlugins(); @@ -211,13 +204,6 @@ public Client client() { return NODE.client(); } - /** - * Returns the single test nodes name. - */ - public String nodeName() { - return "node_s_0"; - } - /** * Return a reference to the singleton node. */ From cfd9969be3a3caecbd3d368d50dc68108a95455f Mon Sep 17 00:00:00 2001 From: javanna Date: Tue, 20 Dec 2016 21:03:20 +0100 Subject: [PATCH 2/2] move to runWithPrivateRandomness --- .../resources/checkstyle_suppressions.xml | 4 --- .../test/ESSingleNodeTestCase.java | 32 ++++++++++--------- .../org/elasticsearch/test/ESTestCase.java | 3 +- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/buildSrc/src/main/resources/checkstyle_suppressions.xml b/buildSrc/src/main/resources/checkstyle_suppressions.xml index dba0529923f61..02097315eaab1 100644 --- a/buildSrc/src/main/resources/checkstyle_suppressions.xml +++ b/buildSrc/src/main/resources/checkstyle_suppressions.xml @@ -993,13 +993,9 @@ - - - - diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java index 8e1cc205a992a..aa4dff995e586 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.test; +import com.carrotsearch.randomizedtesting.RandomizedContext; import org.apache.lucene.util.IOUtils; import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder; @@ -58,7 +59,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.Random; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; @@ -72,10 +72,11 @@ public abstract class ESSingleNodeTestCase extends ESTestCase { private static Node NODE = null; - protected void startNode(Random random) { + protected void startNode(long seed) throws Exception { assert NODE == null; - NODE = newNode(random); - // we must wait for the node to actually be up and running. otherwise the node might have started, elected itself master but might not yet have removed the + NODE = RandomizedContext.current().runWithPrivateRandomness(seed, this::newNode); + // we must wait for the node to actually be up and running. otherwise the node might have started, + // elected itself master but might not yet have removed the // SERVICE_UNAVAILABLE/1/state not recovered / initialized block ClusterHealthResponse clusterHealthResponse = client().admin().cluster().prepareHealth().setWaitForGreenStatus().get(); assertFalse(clusterHealthResponse.isTimedOut()); @@ -96,12 +97,12 @@ private static void stopNode() throws IOException { @Override public void setUp() throws Exception { super.setUp(); - //this random instance has to be created here to ensure tests repeatability - Random random = new Random(random().nextLong()); + //the seed has to be created regardless of whether it will be used or not, for repeatability + long seed = random().nextLong(); // Create the node lazily, on the first test. This is ok because we do not randomize any settings, // only the cluster name. This allows us to have overridden properties for plugins and the version to use. if (NODE == null) { - startNode(random); + startNode(seed); } } @@ -116,10 +117,10 @@ public void tearDown() throws Exception { assertThat("test leaves transient cluster metadata behind: " + metaData.transientSettings().getAsMap(), metaData.transientSettings().getAsMap().size(), equalTo(0)); if (resetNodeAfterTest()) { - //this random instance has to be created here to ensure tests repeatability assert NODE != null; stopNode(); - startNode(new Random(random().nextLong())); + //the seed can be created within this if as it will either be executed before every test method or will never be. + startNode(random().nextLong()); } } @@ -159,10 +160,10 @@ protected Settings nodeSettings() { return Settings.EMPTY; } - private Node newNode(Random random) { + private Node newNode() { final Path tempDir = createTempDir(); Settings settings = Settings.builder() - .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", random.nextLong())) + .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", random().nextLong())) .put(Environment.PATH_HOME_SETTING.getKey(), tempDir) .put(Environment.PATH_REPO_SETTING.getKey(), tempDir.resolve("repo")) // TODO: use a consistent data path for custom paths @@ -176,7 +177,7 @@ private Node newNode(Random random) { .put(NetworkModule.HTTP_ENABLED.getKey(), false) .put("transport.type", MockTcpTransportPlugin.MOCK_TCP_TRANSPORT_NAME) .put(Node.NODE_DATA_SETTING.getKey(), true) - .put(NodeEnvironment.NODE_ID_SEED_SETTING.getKey(), random.nextLong()) + .put(NodeEnvironment.NODE_ID_SEED_SETTING.getKey(), random().nextLong()) .put(nodeSettings()) // allow test cases to provide their own settings or override these .build(); Collection> plugins = getPlugins(); @@ -259,7 +260,8 @@ protected IndexService createIndex(String index, CreateIndexRequestBuilder creat // Wait for the index to be allocated so that cluster state updates don't override // changes that would have been done locally ClusterHealthResponse health = client().admin().cluster() - .health(Requests.clusterHealthRequest(index).waitForYellowStatus().waitForEvents(Priority.LANGUID).waitForNoRelocatingShards(true)).actionGet(); + .health(Requests.clusterHealthRequest(index).waitForYellowStatus().waitForEvents(Priority.LANGUID) + .waitForNoRelocatingShards(true)).actionGet(); assertThat(health.getStatus(), lessThanOrEqualTo(ClusterHealthStatus.YELLOW)); assertThat("Cluster must be a single node cluster", health.getNumberOfDataNodes(), equalTo(1)); IndicesService instanceFromNode = getInstanceFromNode(IndicesService.class); @@ -279,7 +281,6 @@ public Index resolveIndex(String index) { protected SearchContext createSearchContext(IndexService indexService) { BigArrays bigArrays = indexService.getBigArrays(); ThreadPool threadPool = indexService.getThreadPool(); - ScriptService scriptService = node().injector().getInstance(ScriptService.class); return new TestSearchContext(threadPool, bigArrays, indexService); } @@ -302,7 +303,8 @@ public ClusterHealthStatus ensureGreen(String... indices) { */ public ClusterHealthStatus ensureGreen(TimeValue timeout, String... indices) { ClusterHealthResponse actionGet = client().admin().cluster() - .health(Requests.clusterHealthRequest(indices).timeout(timeout).waitForGreenStatus().waitForEvents(Priority.LANGUID).waitForNoRelocatingShards(true)).actionGet(); + .health(Requests.clusterHealthRequest(indices).timeout(timeout).waitForGreenStatus().waitForEvents(Priority.LANGUID) + .waitForNoRelocatingShards(true)).actionGet(); if (actionGet.isTimedOut()) { logger.info("ensureGreen timed out, cluster state:\n{}\n{}", client().admin().cluster().prepareState().get().getState(), client().admin().cluster().preparePendingClusterTasks().get()); diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java index 1002d66c5d0cc..0a1b509f5b03c 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java @@ -794,7 +794,8 @@ public static List randomSubsetOf(Collection collection) { */ public static List randomSubsetOf(int size, Collection collection) { if (size > collection.size()) { - throw new IllegalArgumentException("Can\'t pick " + size + " random objects from a collection of " + collection.size() + " objects"); + throw new IllegalArgumentException("Can\'t pick " + size + " random objects from a collection of " + + collection.size() + " objects"); } List tempList = new ArrayList<>(collection); Collections.shuffle(tempList, random());