Skip to content

Commit

Permalink
Do not close bad indices on startup (elastic#39500)
Browse files Browse the repository at this point in the history 
With elastic#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 (elastic#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 elastic#18467, where we limit the retries of failed allocations. The solution here
is therefore to just undo elastic#17187, as it's no longer necessary, and covered by elastic#18467, which will solve
the issue for Zen2 and replicated closed indices as well.
  • Loading branch information
@ywelsch
ywelsch committed Mar 1, 2019
1 parent b9b46fd commit 1a50af7
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 67 deletions.
22 changes: 0 additions & 22 deletions server/src/main/java/org/elasticsearch/gateway/ClusterStateUpdaters.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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());
Expand Down
1 change: 0 additions & 1 deletion server/src/main/java/org/elasticsearch/gateway/Gateway.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ public void performStateRecovery(final GatewayStateRecoveredListener listener) t
}
ClusterState recoveredState = Function.<ClusterState>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);
Expand Down
1 change: 0 additions & 1 deletion server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
23 changes: 19 additions & 4 deletions server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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 =
Expand Down
31 changes: 0 additions & 31 deletions server/src/test/java/org/elasticsearch/gateway/ClusterStateUpdatersTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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 {

Expand Down Expand Up @@ -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);

Expand Down
41 changes: 33 additions & 8 deletions server/src/test/java/org/elasticsearch/gateway/GatewayIndexStateIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)
Expand Down Expand Up @@ -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"));
Expand Down Expand Up @@ -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());
Expand Down

0 comments on commit 1a50af7

Please sign in to comment.