From 7a6f4633f1c224e485eae1d186407e8a321e9f18 Mon Sep 17 00:00:00 2001 From: David Turner Date: Mon, 18 Nov 2019 10:44:17 +0000 Subject: [PATCH] Remove per-index metadata without assigned shards Today on master-eligible nodes we maintain per-index metadata files for every index. However, we also keep this metadata in the `LucenePersistedState`, and only use the per-index metadata files for importing dangling indices. However there is no point in importing a dangling index without any shard data, so we do not need to maintain these extra files any more. This commit removes per-index metadata files from nodes which do not hold any shards of those indices. Relates #48701 --- .../IncrementalClusterStateWriter.java | 39 ++------- .../elasticsearch/indices/IndicesService.java | 19 +--- .../gateway/GatewayIndexStateIT.java | 87 ++++++++++++++++++- .../IncrementalClusterStateWriterTests.java | 2 +- .../indices/IndicesServiceTests.java | 33 ++----- 5 files changed, 107 insertions(+), 73 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/IncrementalClusterStateWriter.java b/server/src/main/java/org/elasticsearch/gateway/IncrementalClusterStateWriter.java index eb0d243d74fab..6ff97b6a9d2a5 100644 --- a/server/src/main/java/org/elasticsearch/gateway/IncrementalClusterStateWriter.java +++ b/server/src/main/java/org/elasticsearch/gateway/IncrementalClusterStateWriter.java @@ -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; @@ -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; @@ -89,10 +86,6 @@ Manifest getPreviousManifest() { return previousManifest; } - ClusterState getPreviousClusterState() { - return previousClusterState; - } - void setIncrementalWrite(boolean incrementalWrite) { this.incrementalWrite = incrementalWrite; } @@ -206,38 +199,20 @@ static List resolveIndexMetaDataActions(Map pr return actions; } - private static Set getRelevantIndicesOnDataOnlyNode(ClusterState state) { - RoutingNode newRoutingNode = state.getRoutingNodes().node(state.nodes().getLocalNodeId()); + // exposed for tests + static Set 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 indices = new HashSet<>(); - for (ShardRouting routing : newRoutingNode) { + final Set indices = new HashSet<>(); + for (final ShardRouting routing : newRoutingNode) { indices.add(routing.index()); } return indices; } - private static Set getRelevantIndicesForMasterEligibleNode(ClusterState state) { - Set 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 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. */ diff --git a/server/src/main/java/org/elasticsearch/indices/IndicesService.java b/server/src/main/java/org/elasticsearch/indices/IndicesService.java index 3e2c4d45055c1..d978013756c66 100644 --- a/server/src/main/java/org/elasticsearch/indices/IndicesService.java +++ b/server/src/main/java/org/elasticsearch/indices/IndicesService.java @@ -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); @@ -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(); @@ -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)) { - // 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); @@ -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); diff --git a/server/src/test/java/org/elasticsearch/gateway/GatewayIndexStateIT.java b/server/src/test/java/org/elasticsearch/gateway/GatewayIndexStateIT.java index 209e660cc42fb..f78a9c874267d 100644 --- a/server/src/test/java/org/elasticsearch/gateway/GatewayIndexStateIT.java +++ b/server/src/test/java/org/elasticsearch/gateway/GatewayIndexStateIT.java @@ -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; @@ -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()); // for (final Path path : paths) { // try (Stream stateFiles = Files.list(path.resolve(MetaDataStateFormat.STATE_DIR_NAME))) { // for (final Path manifestPath : stateFiles @@ -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 lucenePersistedStateFactories = Stream.of(internalCluster().getNodeNames()) .collect(Collectors.toMap(Function.identity(), diff --git a/server/src/test/java/org/elasticsearch/gateway/IncrementalClusterStateWriterTests.java b/server/src/test/java/org/elasticsearch/gateway/IncrementalClusterStateWriterTests.java index 65432466c61a1..86fe94587093a 100644 --- a/server/src/test/java/org/elasticsearch/gateway/IncrementalClusterStateWriterTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/IncrementalClusterStateWriterTests.java @@ -173,7 +173,7 @@ private IndexMetaData createIndexMetaData(String name) { public void testGetRelevantIndicesWithUnassignedShardsOnMasterEligibleNode() { IndexMetaData indexMetaData = createIndexMetaData("test"); Set indices = IncrementalClusterStateWriter.getRelevantIndices(clusterStateWithUnassignedIndex(indexMetaData, true)); - assertThat(indices.size(), equalTo(1)); + assertThat(indices.size(), equalTo(0)); } public void testGetRelevantIndicesWithUnassignedShardsOnDataOnlyNode() { diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java index 1ed0538288d6b..b82a6a63b0af4 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java @@ -223,13 +223,10 @@ 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(); @@ -237,36 +234,24 @@ public void testDeleteIndexStore() throws Exception { 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 - try { - indicesService.deleteIndexStore("boom", firstMetaData, clusterService.state()); - fail(); - } catch (IllegalStateException ex) { - // all good - } assertAcked(client().admin().indices().prepareOpen("test")); ensureGreen("test"); }