Skip to content

Remove per-index metadata without assigned shards #49234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Remove per-index metadata without assigned shards #49234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 7 additions & 32 deletions server/src/main/java/org/elasticsearch/gateway/IncrementalClusterStateWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import org.elasticsearch.index.Index;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand All @@ -53,9 +52,7 @@ public class IncrementalClusterStateWriter {

private final MetaStateService metaStateService;

// On master-eligible nodes we call updateClusterState under the Coordinator's mutex; on master-ineligible data nodes we call
// updateClusterState on the (unique) cluster applier thread; on other nodes we never call updateClusterState. In all cases there's
// no need to synchronize access to these fields.
// We call updateClusterState on the (unique) cluster applier thread so there's no need to synchronize access to these fields.
private Manifest previousManifest;
private ClusterState previousClusterState;
private final LongSupplier relativeTimeMillisSupplier;
Expand Down Expand Up @@ -89,10 +86,6 @@ Manifest getPreviousManifest() {
return previousManifest;
}

ClusterState getPreviousClusterState() {
return previousClusterState;
}

void setIncrementalWrite(boolean incrementalWrite) {
this.incrementalWrite = incrementalWrite;
}
Expand Down Expand Up @@ -206,38 +199,20 @@ static List<IndexMetaDataAction> resolveIndexMetaDataActions(Map<Index, Long> pr
return actions;
}

private static Set<Index> getRelevantIndicesOnDataOnlyNode(ClusterState state) {
RoutingNode newRoutingNode = state.getRoutingNodes().node(state.nodes().getLocalNodeId());
// exposed for tests
static Set<Index> getRelevantIndices(ClusterState state) {
assert state.nodes().getLocalNode().isDataNode();
final RoutingNode newRoutingNode = state.getRoutingNodes().node(state.nodes().getLocalNodeId());
if (newRoutingNode == null) {
throw new IllegalStateException("cluster state does not contain this node - cannot write index meta state");
}
Set<Index> indices = new HashSet<>();
for (ShardRouting routing : newRoutingNode) {
final Set<Index> indices = new HashSet<>();
for (final ShardRouting routing : newRoutingNode) {
indices.add(routing.index());
}
return indices;
}

private static Set<Index> getRelevantIndicesForMasterEligibleNode(ClusterState state) {
Set<Index> relevantIndices = new HashSet<>();
// we have to iterate over the metadata to make sure we also capture closed indices
for (IndexMetaData indexMetaData : state.metaData()) {
relevantIndices.add(indexMetaData.getIndex());
}
return relevantIndices;
}

// exposed for tests
static Set<Index> getRelevantIndices(ClusterState state) {
if (state.nodes().getLocalNode().isMasterNode()) {
return getRelevantIndicesForMasterEligibleNode(state);
} else if (state.nodes().getLocalNode().isDataNode()) {
return getRelevantIndicesOnDataOnlyNode(state);
} else {
return Collections.emptySet();
}
}

/**
* Action to perform with index metadata.
*/
Expand Down
19 changes: 4 additions & 15 deletions server/src/main/java/org/elasticsearch/indices/IndicesService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,7 @@ public void deleteUnassignedIndex(String reason, IndexMetaData metaData, Cluster
throw new IllegalStateException("Can't delete unassigned index store for [" + indexName + "] - it's still part of " +
"the cluster state [" + index.getIndexUUID() + "] [" + metaData.getIndexUUID() + "]");
}
deleteIndexStore(reason, metaData, clusterState);
deleteIndexStore(reason, metaData);
} catch (Exception e) {
logger.warn(() -> new ParameterizedMessage("[{}] failed to delete unassigned index (reason [{}])",
metaData.getIndex(), reason), e);
Expand All @@ -767,7 +767,7 @@ public void deleteUnassignedIndex(String reason, IndexMetaData metaData, Cluster
*
* Package private for testing
*/
void deleteIndexStore(String reason, IndexMetaData metaData, ClusterState clusterState) throws IOException {
void deleteIndexStore(String reason, IndexMetaData metaData) throws IOException {
if (nodeEnv.hasNodeFile()) {
synchronized (this) {
Index index = metaData.getIndex();
Expand All @@ -776,15 +776,6 @@ void deleteIndexStore(String reason, IndexMetaData metaData, ClusterState cluste
throw new IllegalStateException("Can't delete index store for [" + index.getName() +
"] - it's still part of the indices service [" + localUUid + "] [" + metaData.getIndexUUID() + "]");
}

if (clusterState.metaData().hasIndex(index.getName()) && (clusterState.nodes().getLocalNode().isMasterNode() == true)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks old and somewhat bogus -- it seems that it would avoid deleting index metadata for indices that still exist in the cluster state by name (not by UUID) on master nodes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jup seems there was no deeper reasoning behind this back then in #9985 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great this code can finally go and the code that operates on the data directories is separate from the one that writes out the cluster state.

// we do not delete the store if it is a master eligible node and the index is still in the cluster state
// because we want to keep the meta data for indices around even if no shards are left here
final IndexMetaData idxMeta = clusterState.metaData().index(index.getName());
throw new IllegalStateException("Can't delete index store for [" + index.getName() + "] - it's still part of the " +
"cluster state [" + idxMeta.getIndexUUID() + "] [" + metaData.getIndexUUID() + "], " +
"we are master eligible, so will keep the index metadata even if no shards are left.");
}
}
final IndexSettings indexSettings = buildIndexSettings(metaData);
deleteIndexStore(reason, indexSettings.getIndex(), indexSettings);
Expand Down Expand Up @@ -862,13 +853,11 @@ public void deleteShardStore(String reason, ShardId shardId, ClusterState cluste
nodeEnv.deleteShardDirectorySafe(shardId, indexSettings);
logger.debug("{} deleted shard reason [{}]", shardId, reason);

// master nodes keep the index meta data, even if having no shards..
if (clusterState.nodes().getLocalNode().isMasterNode() == false &&
canDeleteIndexContents(shardId.getIndex(), indexSettings)) {
if (canDeleteIndexContents(shardId.getIndex(), indexSettings)) {
if (nodeEnv.findAllShardIds(shardId.getIndex()).isEmpty()) {
try {
// note that deleteIndexStore have more safety checks and may throw an exception if index was concurrently created.
deleteIndexStore("no longer used", metaData, clusterState);
deleteIndexStore("no longer used", metaData);
} catch (Exception e) {
// wrap the exception to indicate we already deleted the shard
throw new ElasticsearchException("failed to delete unused index after deleting its last shard (" + shardId + ")", e);
Expand Down
87 changes: 86 additions & 1 deletion server/src/test/java/org/elasticsearch/gateway/GatewayIndexStateIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import org.elasticsearch.test.InternalTestCluster.RestartCallback;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -533,7 +534,7 @@ public void testHalfDeletedIndexImport() throws Exception {
// // term in the coordination metadata
// .coordinationMetaData(CoordinationMetaData.builder(metaData.coordinationMetaData()).term(0L).build())
// // add a tombstone but do not delete the index metadata from disk
// .putCustom(IndexGraveyard.TYPE, IndexGraveyard.builder().addTombstone(metaData.index("test").getIndex()).build()).build());
// .putCustom(IndexGraveyard.TYPE, IndexGraveyard.builder().addTombstone(metaData.index("test").getIndex()).build()).build());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line length, oops :(

// for (final Path path : paths) {
// try (Stream<Path> stateFiles = Files.list(path.resolve(MetaDataStateFormat.STATE_DIR_NAME))) {
// for (final Path manifestPath : stateFiles
Expand All @@ -549,6 +550,90 @@ public void testHalfDeletedIndexImport() throws Exception {
assertBusy(() -> assertThat(internalCluster().getInstance(NodeEnvironment.class).availableIndexFolders(), empty()));
}

public void testOnlyWritesIndexMetaDataFilesOnDataNodes() throws Exception {
final String masterNode = internalCluster().startMasterOnlyNode();
final String dataNode = internalCluster().startDataOnlyNode();
final String mixedNode = internalCluster().startNode();

createIndex("test", Settings.builder()
.put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, between(1, 3))
.put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 1)
.build());
ensureGreen("test");

final String indexUUID = client().admin().indices().prepareStats("test").get().getIndex("test").getUuid();

final Path[] masterPaths = internalCluster().getInstance(NodeEnvironment.class, masterNode).nodeDataPaths();
final Path[] dataPaths = internalCluster().getInstance(NodeEnvironment.class, dataNode).nodeDataPaths();
final Path[] mixedPaths = internalCluster().getInstance(NodeEnvironment.class, mixedNode).nodeDataPaths();

for (final Path path : masterPaths) {
assertFalse("master: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER)));
}
for (final Path path : dataPaths) {
assertTrue("data: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER).resolve(indexUUID)));
}
for (final Path path : mixedPaths) {
assertTrue("mixed: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER).resolve(indexUUID)));
}

logger.info("--> remove shards from data node, to check the index folder is cleaned up");
assertAcked(client().admin().indices().prepareUpdateSettings("test").setSettings(Settings.builder()
.put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0)
.put(IndexMetaData.INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + "._name", dataNode)));
assertFalse(client().admin().cluster().prepareHealth("test").setWaitForGreenStatus()
.setWaitForNoInitializingShards(true).setWaitForEvents(Priority.LANGUID).get().isTimedOut());

for (final Path path : masterPaths) {
assertFalse("master: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER)));
}
for (final Path path : mixedPaths) {
assertTrue("mixed: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER).resolve(indexUUID)));
}
assertBusy(() -> {
for (final Path path : dataPaths) {
assertFalse("data: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER).resolve(indexUUID)));
}
});

logger.info("--> remove shards from mixed master/data node, to check the index folder is cleaned up");
assertAcked(client().admin().indices().prepareUpdateSettings("test").setSettings(Settings.builder()
.put(IndexMetaData.INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + "._name", mixedNode)));
assertFalse(client().admin().cluster().prepareHealth("test").setWaitForGreenStatus()
.setWaitForNoInitializingShards(true).setWaitForEvents(Priority.LANGUID).get().isTimedOut());

for (final Path path : masterPaths) {
assertFalse("master: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER)));
}
for (final Path path : dataPaths) {
assertTrue("data: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER).resolve(indexUUID)));
}
assertBusy(() -> {
for (final Path path : mixedPaths) {
assertFalse("mixed: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER).resolve(indexUUID)));
}
});

logger.info("--> delete index and check the index folder is cleaned up on all nodes");
assertAcked(client().admin().indices().prepareUpdateSettings("test").setSettings(Settings.builder()
.put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 1)
.putNull(IndexMetaData.INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + "._name")));
ensureGreen("test");
assertAcked(client().admin().indices().prepareDelete("test"));

for (final Path path : masterPaths) {
assertFalse("master: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER)));
}
assertBusy(() -> {
for (final Path path : dataPaths) {
assertFalse("data: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER).resolve(indexUUID)));
}
for (final Path path : mixedPaths) {
assertFalse("mixed: " + path, Files.exists(path.resolve(NodeEnvironment.INDICES_FOLDER).resolve(indexUUID)));
}
});
}

private void restartNodesOnBrokenClusterState(ClusterState.Builder clusterStateBuilder) throws Exception {
Map<String, LucenePersistedStateFactory> lucenePersistedStateFactories = Stream.of(internalCluster().getNodeNames())
.collect(Collectors.toMap(Function.identity(),
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/gateway/IncrementalClusterStateWriterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ private IndexMetaData createIndexMetaData(String name) {
public void testGetRelevantIndicesWithUnassignedShardsOnMasterEligibleNode() {
IndexMetaData indexMetaData = createIndexMetaData("test");
Set<Index> indices = IncrementalClusterStateWriter.getRelevantIndices(clusterStateWithUnassignedIndex(indexMetaData, true));
assertThat(indices.size(), equalTo(1));
assertThat(indices.size(), equalTo(0));
}

public void testGetRelevantIndicesWithUnassignedShardsOnDataOnlyNode() {
Expand Down
33 changes: 9 additions & 24 deletions server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,50 +223,35 @@ public void testDeleteIndexStore() throws Exception {
ClusterService clusterService = getInstanceFromNode(ClusterService.class);
IndexMetaData firstMetaData = clusterService.state().metaData().index("test");
assertTrue(test.hasShard(0));
ShardPath firstPath = ShardPath.loadShardPath(logger, getNodeEnvironment(), new ShardId(test.index(), 0), test.getIndexSettings());

try {
indicesService.deleteIndexStore("boom", firstMetaData, clusterService.state());
fail();
} catch (IllegalStateException ex) {
// all good
}
expectThrows(IllegalStateException.class, () -> indicesService.deleteIndexStore("boom", firstMetaData));
assertTrue(firstPath.exists());

GatewayMetaState gwMetaState = getInstanceFromNode(GatewayMetaState.class);
MetaData meta = gwMetaState.getMetaData();
assertNotNull(meta);
assertNotNull(meta.index("test"));
assertAcked(client().admin().indices().prepareDelete("test"));

assertFalse(firstPath.exists());

meta = gwMetaState.getMetaData();
assertNotNull(meta);
assertNull(meta.index("test"));


test = createIndex("test");
client().prepareIndex("test").setId("1").setSource("field", "value").setRefreshPolicy(IMMEDIATE).get();
client().admin().indices().prepareFlush("test").get();
assertHitCount(client().prepareSearch("test").get(), 1);
IndexMetaData secondMetaData = clusterService.state().metaData().index("test");
assertAcked(client().admin().indices().prepareClose("test"));
ShardPath path = ShardPath.loadShardPath(logger, getNodeEnvironment(), new ShardId(test.index(), 0), test.getIndexSettings());
assertTrue(path.exists());
ShardPath secondPath = ShardPath.loadShardPath(logger, getNodeEnvironment(), new ShardId(test.index(), 0), test.getIndexSettings());
assertTrue(secondPath.exists());

try {
indicesService.deleteIndexStore("boom", secondMetaData, clusterService.state());
fail();
} catch (IllegalStateException ex) {
// all good
}

assertTrue(path.exists());
expectThrows(IllegalStateException.class, () -> indicesService.deleteIndexStore("boom", secondMetaData));
assertTrue(secondPath.exists());

// now delete the old one and make sure we resolve against the name
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of this test relates to the old-and-somewhat-bogus code mentioned above -- it was checking that we didn't delete the index metadata for a different index with the same name as one in the cluster state.

try {
indicesService.deleteIndexStore("boom", firstMetaData, clusterService.state());
fail();
} catch (IllegalStateException ex) {
// all good
}
assertAcked(client().admin().indices().prepareOpen("test"));
ensureGreen("test");
}
Expand Down