From 637c6d1606f8a63a1c296c9fa0952b52c97c4f26 Mon Sep 17 00:00:00 2001 From: Michael McCandless Date: Thu, 25 Sep 2014 16:50:48 -0400 Subject: [PATCH] Tests: always run Lucene's CheckIndex when shards are closed in tests and fail the test if corruption is detected Today we only run 10% of the time, and the test doesn't fail when corruption is detected. I think it's better to always run and fail the test, so we can catch any possible resiliency bugs in Lucene/Elasticsearch causing corruption. For known tests that create corrupted indices, it's easy to set MockFSDirectoryService.CHECK_INDEX_ON_CLOSE to false... Closes #7730 --- .../index/service/InternalIndexService.java | 4 ++-- .../indices/IndicesLifecycle.java | 2 +- .../indices/InternalIndicesLifecycle.java | 4 ++-- .../lucene/util/AbstractRandomizedTest.java | 5 +++++ .../elasticsearch/cluster/ack/AckTests.java | 13 ++++++++---- .../index/store/CorruptedFileTest.java | 2 ++ .../org/elasticsearch/indices/FlushTest.java | 5 +---- .../SearchWithRandomExceptionsTests.java | 2 -- .../test/store/MockFSDirectoryService.java | 21 ++++++++++++------- .../versioning/SimpleVersioningTests.java | 5 +---- 10 files changed, 37 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/elasticsearch/index/service/InternalIndexService.java b/src/main/java/org/elasticsearch/index/service/InternalIndexService.java index f92b87f1fa2..e29e56fb84a 100644 --- a/src/main/java/org/elasticsearch/index/service/InternalIndexService.java +++ b/src/main/java/org/elasticsearch/index/service/InternalIndexService.java @@ -453,7 +453,7 @@ public class InternalIndexService extends AbstractIndexComponent implements Inde } // call this before we close the store, so we can release resources for it - indicesLifecycle.afterIndexShardClosed(sId); + indicesLifecycle.afterIndexShardClosed(sId, indexShard); // if we delete or have no gateway or the store is not persistent, clean the store... Store store = shardInjector.getInstance(Store.class); // and close it @@ -464,4 +464,4 @@ public class InternalIndexService extends AbstractIndexComponent implements Inde } Injectors.close(injector); } -} \ No newline at end of file +} diff --git a/src/main/java/org/elasticsearch/indices/IndicesLifecycle.java b/src/main/java/org/elasticsearch/indices/IndicesLifecycle.java index 5a2bbacb221..7f7a990915c 100644 --- a/src/main/java/org/elasticsearch/indices/IndicesLifecycle.java +++ b/src/main/java/org/elasticsearch/indices/IndicesLifecycle.java @@ -130,7 +130,7 @@ public interface IndicesLifecycle { * * @param shardId The shard id */ - public void afterIndexShardClosed(ShardId shardId) { + public void afterIndexShardClosed(ShardId shardId, @Nullable IndexShard indexShard) { } diff --git a/src/main/java/org/elasticsearch/indices/InternalIndicesLifecycle.java b/src/main/java/org/elasticsearch/indices/InternalIndicesLifecycle.java index 967f97e0782..1711810a67a 100644 --- a/src/main/java/org/elasticsearch/indices/InternalIndicesLifecycle.java +++ b/src/main/java/org/elasticsearch/indices/InternalIndicesLifecycle.java @@ -152,10 +152,10 @@ public class InternalIndicesLifecycle extends AbstractComponent implements Indic } } - public void afterIndexShardClosed(ShardId shardId) { + public void afterIndexShardClosed(ShardId shardId, @Nullable IndexShard indexShard) { for (Listener listener : listeners) { try { - listener.afterIndexShardClosed(shardId); + listener.afterIndexShardClosed(shardId, indexShard); } catch (Throwable t) { logger.warn("{} failed to invoke after shard closed callback", t, shardId); } diff --git a/src/test/java/org/apache/lucene/util/AbstractRandomizedTest.java b/src/test/java/org/apache/lucene/util/AbstractRandomizedTest.java index 71e1476f966..af934d38b7c 100644 --- a/src/test/java/org/apache/lucene/util/AbstractRandomizedTest.java +++ b/src/test/java/org/apache/lucene/util/AbstractRandomizedTest.java @@ -371,12 +371,16 @@ public abstract class AbstractRandomizedTest extends RandomizedTest { // Suite and test case setup/ cleanup. // ----------------------------------------------------------------- + /** MockFSDirectoryService sets this: */ + public static boolean checkIndexFailed; + /** * For subclasses to override. Overrides must call {@code super.setUp()}. */ @Before public void setUp() throws Exception { parentChainCallRule.setupCalled = true; + checkIndexFailed = false; } /** @@ -385,6 +389,7 @@ public abstract class AbstractRandomizedTest extends RandomizedTest { @After public void tearDown() throws Exception { parentChainCallRule.teardownCalled = true; + assertFalse("at least one shard failed CheckIndex", checkIndexFailed); } diff --git a/src/test/java/org/elasticsearch/cluster/ack/AckTests.java b/src/test/java/org/elasticsearch/cluster/ack/AckTests.java index e6101b8a71f..d7e38a0b43c 100644 --- a/src/test/java/org/elasticsearch/cluster/ack/AckTests.java +++ b/src/test/java/org/elasticsearch/cluster/ack/AckTests.java @@ -19,9 +19,6 @@ package org.elasticsearch.cluster.ack; -import com.carrotsearch.hppc.cursors.ObjectObjectCursor; -import com.google.common.base.Predicate; -import com.google.common.collect.ImmutableList; import org.elasticsearch.action.admin.cluster.reroute.ClusterRerouteResponse; import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse; import org.elasticsearch.action.admin.indices.alias.IndicesAliasesResponse; @@ -49,7 +46,11 @@ import org.elasticsearch.discovery.DiscoverySettings; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.search.warmer.IndexWarmersMetaData; import org.elasticsearch.test.ElasticsearchIntegrationTest; +import org.elasticsearch.test.store.MockFSDirectoryService; import org.junit.Test; +import com.carrotsearch.hppc.cursors.ObjectObjectCursor; +import com.google.common.base.Predicate; +import com.google.common.collect.ImmutableList; import static org.elasticsearch.cluster.metadata.IndexMetaData.*; import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder; @@ -395,7 +396,11 @@ public class AckTests extends ElasticsearchIntegrationTest { @Test public void testOpenIndexNoAcknowledgement() { - createIndex("test"); + // TODO: this test fails CheckIndex test for some reason ... seems like the index is being deleted while we run CheckIndex?? + assertAcked(client().admin().indices().prepareCreate("test").setSettings( + ImmutableSettings.settingsBuilder() + // Never run CheckIndex in the end: + .put(MockFSDirectoryService.CHECK_INDEX_ON_CLOSE, false).build())); ensureGreen(); CloseIndexResponse closeIndexResponse = client().admin().indices().prepareClose("test").execute().actionGet(); diff --git a/src/test/java/org/elasticsearch/index/store/CorruptedFileTest.java b/src/test/java/org/elasticsearch/index/store/CorruptedFileTest.java index aa83c53344e..e0ae43a440c 100644 --- a/src/test/java/org/elasticsearch/index/store/CorruptedFileTest.java +++ b/src/test/java/org/elasticsearch/index/store/CorruptedFileTest.java @@ -317,6 +317,8 @@ public class CorruptedFileTest extends ElasticsearchIntegrationTest { assertAcked(prepareCreate("test").setSettings(ImmutableSettings.builder() .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, "0") .put(InternalEngine.INDEX_FAIL_ON_CORRUPTION, true) + // This does corrupt files on the replica, so we can't check: + .put(MockFSDirectoryService.CHECK_INDEX_ON_CLOSE, false) .put("index.routing.allocation.include._name", primariesNode.getNode().name()) .put("indices.recovery.concurrent_streams", 10) )); diff --git a/src/test/java/org/elasticsearch/indices/FlushTest.java b/src/test/java/org/elasticsearch/indices/FlushTest.java index 96efbc9c29b..161ad6c0553 100644 --- a/src/test/java/org/elasticsearch/indices/FlushTest.java +++ b/src/test/java/org/elasticsearch/indices/FlushTest.java @@ -38,10 +38,7 @@ public class FlushTest extends ElasticsearchIntegrationTest { @Test public void testWaitIfOngoing() throws InterruptedException { - assertAcked(client().admin().indices().prepareCreate("test").setSettings( - ImmutableSettings.settingsBuilder() - // Always run CheckIndex in the end: - .put(MockFSDirectoryService.CHECK_INDEX_ON_CLOSE, true).build())); + createIndex("test"); ensureGreen("test"); final int numIters = scaledRandomIntBetween(10, 30); for (int i = 0; i < numIters; i++) { diff --git a/src/test/java/org/elasticsearch/search/basic/SearchWithRandomExceptionsTests.java b/src/test/java/org/elasticsearch/search/basic/SearchWithRandomExceptionsTests.java index feff675ccef..ba06895b812 100644 --- a/src/test/java/org/elasticsearch/search/basic/SearchWithRandomExceptionsTests.java +++ b/src/test/java/org/elasticsearch/search/basic/SearchWithRandomExceptionsTests.java @@ -92,7 +92,6 @@ public class SearchWithRandomExceptionsTests extends ElasticsearchIntegrationTes if (createIndexWithoutErrors) { Builder settings = settingsBuilder() .put("index.number_of_replicas", randomIntBetween(0, 1)) - .put(MockFSDirectoryService.CHECK_INDEX_ON_CLOSE, true) .put("gateway.type", "local"); logger.info("creating index: [test] using settings: [{}]", settings.build().getAsMap()); client().admin().indices().prepareCreate("test") @@ -107,7 +106,6 @@ public class SearchWithRandomExceptionsTests extends ElasticsearchIntegrationTes client().admin().indices().prepareFlush("test").setWaitIfOngoing(true).execute().get(); client().admin().indices().prepareClose("test").execute().get(); client().admin().indices().prepareUpdateSettings("test").setSettings(settingsBuilder() - .put(MockFSDirectoryService.CHECK_INDEX_ON_CLOSE, true) .put(MockDirectoryHelper.RANDOM_IO_EXCEPTION_RATE, exceptionRate) .put(MockDirectoryHelper.RANDOM_IO_EXCEPTION_RATE_ON_OPEN, exceptionOnOpenRate)); client().admin().indices().prepareOpen("test").execute().get(); diff --git a/src/test/java/org/elasticsearch/test/store/MockFSDirectoryService.java b/src/test/java/org/elasticsearch/test/store/MockFSDirectoryService.java index 78dcc47f756..2666cb6b85e 100644 --- a/src/test/java/org/elasticsearch/test/store/MockFSDirectoryService.java +++ b/src/test/java/org/elasticsearch/test/store/MockFSDirectoryService.java @@ -21,9 +21,11 @@ package org.elasticsearch.test.store; import com.google.common.base.Charsets; import org.apache.lucene.index.CheckIndex; +import org.apache.lucene.index.IndexWriter; import org.apache.lucene.store.Directory; import org.apache.lucene.store.LockFactory; import org.apache.lucene.store.StoreRateLimiting; +import org.apache.lucene.util.AbstractRandomizedTest; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.BytesStreamOutput; @@ -60,15 +62,15 @@ public class MockFSDirectoryService extends FsDirectoryService { final long seed = indexSettings.getAsLong(ElasticsearchIntegrationTest.SETTING_INDEX_SEED, 0l); Random random = new Random(seed); helper = new MockDirectoryHelper(shardId, indexSettings, logger, random, seed); - checkIndexOnClose = indexSettings.getAsBoolean(CHECK_INDEX_ON_CLOSE, random.nextDouble() < 0.1); + checkIndexOnClose = indexSettings.getAsBoolean(CHECK_INDEX_ON_CLOSE, true); delegateService = helper.randomDirectorService(indexStore); if (checkIndexOnClose) { final IndicesLifecycle.Listener listener = new IndicesLifecycle.Listener() { @Override - public void beforeIndexShardClosed(ShardId sid, @Nullable IndexShard indexShard) { + public void afterIndexShardClosed(ShardId sid, @Nullable IndexShard indexShard) { if (shardId.equals(sid) && indexShard != null) { - checkIndex(((InternalIndexShard) indexShard).store()); + checkIndex(((InternalIndexShard) indexShard).store(), sid); } service.indicesLifecycle().removeListener(this); } @@ -87,19 +89,24 @@ public class MockFSDirectoryService extends FsDirectoryService { throw new UnsupportedOperationException(); } - - public void checkIndex(Store store) throws IndexShardException { + public void checkIndex(Store store, ShardId shardId) throws IndexShardException { try { - if (!Lucene.indexExists(store.directory())) { + Directory dir = store.directory(); + if (!Lucene.indexExists(dir)) { return; } - CheckIndex checkIndex = new CheckIndex(store.directory()); + if (IndexWriter.isLocked(dir)) { + AbstractRandomizedTest.checkIndexFailed = true; + throw new IllegalStateException("IndexWriter is still open on shard " + shardId); + } + CheckIndex checkIndex = new CheckIndex(dir); BytesStreamOutput os = new BytesStreamOutput(); PrintStream out = new PrintStream(os, false, Charsets.UTF_8.name()); checkIndex.setInfoStream(out); out.flush(); CheckIndex.Status status = checkIndex.checkIndex(); if (!status.clean) { + AbstractRandomizedTest.checkIndexFailed = true; logger.warn("check index [failure]\n{}", new String(os.bytes().toBytes(), Charsets.UTF_8)); throw new IndexShardException(shardId, "index check failure"); } else { diff --git a/src/test/java/org/elasticsearch/versioning/SimpleVersioningTests.java b/src/test/java/org/elasticsearch/versioning/SimpleVersioningTests.java index 62fddde8931..d47b9bab562 100644 --- a/src/test/java/org/elasticsearch/versioning/SimpleVersioningTests.java +++ b/src/test/java/org/elasticsearch/versioning/SimpleVersioningTests.java @@ -526,10 +526,7 @@ public class SimpleVersioningTests extends ElasticsearchIntegrationTest { @Test @Slow public void testRandomIDsAndVersions() throws Exception { - assertAcked(client().admin().indices().prepareCreate("test").setSettings( - ImmutableSettings.settingsBuilder() - // Always run CheckIndex in the end: - .put(MockFSDirectoryService.CHECK_INDEX_ON_CLOSE, true).build())); + createIndex("test"); ensureGreen(); // TODO: sometimes use _bulk API