From 1a50af7dd4a8696a26e218f4ca4433dcdc5da541 Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Fri, 1 Mar 2019 09:16:53 +0100 Subject: [PATCH] Do not close bad indices on startup (#39500) With #17187, we verified IndexService creation during initial state recovery on the master and if the recovery failed the index was imported as closed, not allocating any shards. This was mainly done to prevent endless allocation loops and full log files on data-nodes when the indexmetadata contained broken settings / analyzers. Zen2 loads the cluster state eagerly, and this check currently runs on all nodes (not only the elected master), which can significantly slow down startup on data nodes. Furthermore, with replicated closed indices (#33888) on the horizon, importing the index as closed will no longer not allocate any shards. Fortunately, the original issue for endless allocation loops is no longer a problem due to #18467, where we limit the retries of failed allocations. The solution here is therefore to just undo #17187, as it's no longer necessary, and covered by #18467, which will solve the issue for Zen2 and replicated closed indices as well. --- .../gateway/ClusterStateUpdaters.java | 22 ---------- .../org/elasticsearch/gateway/Gateway.java | 1 - .../gateway/GatewayMetaState.java | 1 - .../admin/indices/create/CreateIndexIT.java | 23 +++++++++-- .../gateway/ClusterStateUpdatersTests.java | 31 -------------- .../gateway/GatewayIndexStateIT.java | 41 +++++++++++++++---- 6 files changed, 52 insertions(+), 67 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/ClusterStateUpdaters.java b/server/src/main/java/org/elasticsearch/gateway/ClusterStateUpdaters.java index 056919ab1c4be..74eb3e08f002f 100644 --- a/server/src/main/java/org/elasticsearch/gateway/ClusterStateUpdaters.java +++ b/server/src/main/java/org/elasticsearch/gateway/ClusterStateUpdaters.java @@ -31,8 +31,6 @@ import org.elasticsearch.cluster.node.DiscoveryNodes; import org.elasticsearch.cluster.routing.RoutingTable; import org.elasticsearch.common.settings.ClusterSettings; -import org.elasticsearch.index.Index; -import org.elasticsearch.indices.IndicesService; import java.util.Map; @@ -92,26 +90,6 @@ static ClusterState recoverClusterBlocks(final ClusterState state) { return ClusterState.builder(state).blocks(blocks).build(); } - static ClusterState closeBadIndices(final ClusterState clusterState, final IndicesService indicesService) { - final MetaData.Builder builder = MetaData.builder(clusterState.metaData()).removeAllIndices(); - - for (IndexMetaData metaData : clusterState.metaData()) { - try { - if (metaData.getState() == IndexMetaData.State.OPEN) { - // verify that we can actually create this index - if not we recover it as closed with lots of warn logs - indicesService.verifyIndexMetadata(metaData, metaData); - } - } catch (final Exception e) { - final Index electedIndex = metaData.getIndex(); - logger.warn(() -> new ParameterizedMessage("recovering index {} failed - recovering as closed", electedIndex), e); - metaData = IndexMetaData.builder(metaData).state(IndexMetaData.State.CLOSE).build(); - } - builder.put(metaData, false); - } - - return ClusterState.builder(clusterState).metaData(builder).build(); - } - static ClusterState updateRoutingTable(final ClusterState state) { // initialize all index routing tables as empty final RoutingTable.Builder routingTableBuilder = RoutingTable.builder(state.routingTable()); diff --git a/server/src/main/java/org/elasticsearch/gateway/Gateway.java b/server/src/main/java/org/elasticsearch/gateway/Gateway.java index cffb672f0cfda..317bf63a4a651 100644 --- a/server/src/main/java/org/elasticsearch/gateway/Gateway.java +++ b/server/src/main/java/org/elasticsearch/gateway/Gateway.java @@ -126,7 +126,6 @@ public void performStateRecovery(final GatewayStateRecoveredListener listener) t } ClusterState recoveredState = Function.identity() .andThen(state -> ClusterStateUpdaters.upgradeAndArchiveUnknownOrInvalidSettings(state, clusterService.getClusterSettings())) - .andThen(state -> ClusterStateUpdaters.closeBadIndices(state, indicesService)) .apply(ClusterState.builder(clusterService.getClusterName()).metaData(metaDataBuilder).build()); listener.onSuccess(recoveredState); diff --git a/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java b/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java index f14d86c7602bf..bd6fd908d49ab 100644 --- a/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java +++ b/server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java @@ -137,7 +137,6 @@ public void applyClusterStateUpdaters() { .andThen(ClusterStateUpdaters::addStateNotRecoveredBlock) .andThen(state -> ClusterStateUpdaters.setLocalNode(state, transportService.getLocalNode())) .andThen(state -> ClusterStateUpdaters.upgradeAndArchiveUnknownOrInvalidSettings(state, clusterService.getClusterSettings())) - .andThen(state -> ClusterStateUpdaters.closeBadIndices(state, indicesService)) .andThen(ClusterStateUpdaters::recoverClusterBlocks) .apply(previousClusterState); } diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java b/server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java index 05da57cc5da45..f23dbaa8ea413 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java @@ -34,6 +34,10 @@ import org.elasticsearch.cluster.metadata.MappingMetaData; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.cluster.routing.IndexRoutingTable; +import org.elasticsearch.cluster.routing.IndexShardRoutingTable; +import org.elasticsearch.cluster.routing.RoutingTable; +import org.elasticsearch.cluster.routing.UnassignedInfo; import org.elasticsearch.common.collect.ImmutableOpenMap; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; @@ -50,6 +54,7 @@ import java.util.HashSet; import java.util.Set; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.BiFunction; @@ -60,6 +65,7 @@ import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.instanceOf; @@ -419,11 +425,20 @@ public Settings onNodeStopped(String nodeName) throws Exception { return super.onNodeStopped(nodeName); } }); - ensureGreen(metaData.getIndex().getName()); // we have to wait for the index to show up in the metadata or we will fail in a race - final ClusterState stateAfterRestart = client().admin().cluster().prepareState().get().getState(); - // the index should not be open after we restart and recover the broken index metadata - assertThat(stateAfterRestart.getMetaData().index(metaData.getIndex()).getState(), equalTo(IndexMetaData.State.CLOSE)); + // check that the cluster does not keep reallocating shards + assertBusy(() -> { + final RoutingTable routingTable = client().admin().cluster().prepareState().get().getState().routingTable(); + final IndexRoutingTable indexRoutingTable = routingTable.index("test"); + assertNotNull(indexRoutingTable); + for (IndexShardRoutingTable shardRoutingTable : indexRoutingTable) { + assertTrue(shardRoutingTable.primaryShard().unassigned()); + assertEquals(UnassignedInfo.AllocationStatus.DECIDERS_NO, + shardRoutingTable.primaryShard().unassignedInfo().getLastAllocationStatus()); + assertThat(shardRoutingTable.primaryShard().unassignedInfo().getNumFailedAllocations(), greaterThan(0)); + } + }, 60, TimeUnit.SECONDS); + client().admin().indices().prepareClose("test").get(); // try to open the index final ElasticsearchException e = diff --git a/server/src/test/java/org/elasticsearch/gateway/ClusterStateUpdatersTests.java b/server/src/test/java/org/elasticsearch/gateway/ClusterStateUpdatersTests.java index cae33db90a6bc..999d80586fea4 100644 --- a/server/src/test/java/org/elasticsearch/gateway/ClusterStateUpdatersTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/ClusterStateUpdatersTests.java @@ -33,10 +33,8 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.index.Index; -import org.elasticsearch.indices.IndicesService; import org.elasticsearch.test.ESTestCase; -import java.io.IOException; import java.util.Arrays; import java.util.Collections; import java.util.Set; @@ -47,7 +45,6 @@ import static org.elasticsearch.cluster.metadata.MetaData.CLUSTER_READ_ONLY_BLOCK; import static org.elasticsearch.gateway.ClusterStateUpdaters.addStateNotRecoveredBlock; -import static org.elasticsearch.gateway.ClusterStateUpdaters.closeBadIndices; import static org.elasticsearch.gateway.ClusterStateUpdaters.hideStateIfNotRecovered; import static org.elasticsearch.gateway.ClusterStateUpdaters.mixCurrentStateAndRecoveredState; import static org.elasticsearch.gateway.ClusterStateUpdaters.recoverClusterBlocks; @@ -59,8 +56,6 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; -import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.mock; public class ClusterStateUpdatersTests extends ESTestCase { @@ -201,32 +196,6 @@ public void testAddStateNotRecoveredBlock() { assertTrue(newState.blocks().hasGlobalBlock(STATE_NOT_RECOVERED_BLOCK)); } - public void testCloseBadIndices() throws IOException { - final IndicesService indicesService = mock(IndicesService.class); - final IndexMetaData good = createIndexMetaData("good", Settings.EMPTY); - final IndexMetaData bad = createIndexMetaData("bad", Settings.EMPTY); - final IndexMetaData ugly = IndexMetaData.builder(createIndexMetaData("ugly", Settings.EMPTY)) - .state(IndexMetaData.State.CLOSE) - .build(); - - final ClusterState initialState = ClusterState - .builder(ClusterState.EMPTY_STATE) - .metaData(MetaData.builder() - .put(good, false) - .put(bad, false) - .put(ugly, false) - .build()) - .build(); - - doThrow(new RuntimeException("test")).when(indicesService).verifyIndexMetadata(bad, bad); - doThrow(new AssertionError("ugly index is already closed")).when(indicesService).verifyIndexMetadata(ugly, ugly); - - final ClusterState newState = closeBadIndices(initialState, indicesService); - assertThat(newState.metaData().index(good.getIndex()).getState(), equalTo(IndexMetaData.State.OPEN)); - assertThat(newState.metaData().index(bad.getIndex()).getState(), equalTo(IndexMetaData.State.CLOSE)); - assertThat(newState.metaData().index(ugly.getIndex()).getState(), equalTo(IndexMetaData.State.CLOSE)); - } - public void testUpdateRoutingTable() { final int numOfShards = randomIntBetween(1, 10); diff --git a/server/src/test/java/org/elasticsearch/gateway/GatewayIndexStateIT.java b/server/src/test/java/org/elasticsearch/gateway/GatewayIndexStateIT.java index ebdae985a39c7..272ac31658229 100644 --- a/server/src/test/java/org/elasticsearch/gateway/GatewayIndexStateIT.java +++ b/server/src/test/java/org/elasticsearch/gateway/GatewayIndexStateIT.java @@ -34,7 +34,11 @@ import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MappingMetaData; import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.cluster.routing.IndexRoutingTable; +import org.elasticsearch.cluster.routing.IndexShardRoutingTable; +import org.elasticsearch.cluster.routing.RoutingTable; import org.elasticsearch.cluster.routing.ShardRoutingState; +import org.elasticsearch.cluster.routing.UnassignedInfo; import org.elasticsearch.common.Priority; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentFactory; @@ -51,12 +55,14 @@ import java.io.IOException; import java.util.List; +import java.util.concurrent.TimeUnit; import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.nullValue; @ClusterScope(scope = Scope.TEST, numDataNodes = 0) @@ -369,9 +375,20 @@ public Settings onNodeStopped(String nodeName) throws Exception { } }); - // ensureGreen(closedIndex) waits for the index to show up in the metadata - // this is crucial otherwise the state call below might not contain the index yet - ensureGreen(metaData.getIndex().getName()); + // check that the cluster does not keep reallocating shards + assertBusy(() -> { + final RoutingTable routingTable = client().admin().cluster().prepareState().get().getState().routingTable(); + final IndexRoutingTable indexRoutingTable = routingTable.index("test"); + assertNotNull(indexRoutingTable); + for (IndexShardRoutingTable shardRoutingTable : indexRoutingTable) { + assertTrue(shardRoutingTable.primaryShard().unassigned()); + assertEquals(UnassignedInfo.AllocationStatus.DECIDERS_NO, + shardRoutingTable.primaryShard().unassignedInfo().getLastAllocationStatus()); + assertThat(shardRoutingTable.primaryShard().unassignedInfo().getNumFailedAllocations(), greaterThan(0)); + } + }, 60, TimeUnit.SECONDS); + client().admin().indices().prepareClose("test").get(); + state = client().admin().cluster().prepareState().get().getState(); assertEquals(IndexMetaData.State.CLOSE, state.getMetaData().index(metaData.getIndex()).getState()); assertEquals("classic", state.getMetaData().index(metaData.getIndex()).getSettings().get("archived.index.similarity.BM25.type")); @@ -432,11 +449,19 @@ public Settings onNodeStopped(String nodeName) throws Exception { } }); - // ensureGreen(closedIndex) waits for the index to show up in the metadata - // this is crucial otherwise the state call below might not contain the index yet - ensureGreen(metaData.getIndex().getName()); - state = client().admin().cluster().prepareState().get().getState(); - assertEquals(IndexMetaData.State.CLOSE, state.getMetaData().index(metaData.getIndex()).getState()); + // check that the cluster does not keep reallocating shards + assertBusy(() -> { + final RoutingTable routingTable = client().admin().cluster().prepareState().get().getState().routingTable(); + final IndexRoutingTable indexRoutingTable = routingTable.index("test"); + assertNotNull(indexRoutingTable); + for (IndexShardRoutingTable shardRoutingTable : indexRoutingTable) { + assertTrue(shardRoutingTable.primaryShard().unassigned()); + assertEquals(UnassignedInfo.AllocationStatus.DECIDERS_NO, + shardRoutingTable.primaryShard().unassignedInfo().getLastAllocationStatus()); + assertThat(shardRoutingTable.primaryShard().unassignedInfo().getNumFailedAllocations(), greaterThan(0)); + } + }, 60, TimeUnit.SECONDS); + client().admin().indices().prepareClose("test").get(); // try to open it with the broken setting - fail again! ElasticsearchException ex = expectThrows(ElasticsearchException.class, () -> client().admin().indices().prepareOpen("test").get());