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 elastic#48701

Author: DaveCTurner

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<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.
      */

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);

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<Path> 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<String, LucenePersistedStateFactory> lucenePersistedStateFactories = Stream.of(internalCluster().getNodeNames())
             .collect(Collectors.toMap(Function.identity(),

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<Index> indices = IncrementalClusterStateWriter.getRelevantIndices(clusterStateWithUnassignedIndex(indexMetaData, true));
-        assertThat(indices.size(), equalTo(1));
+        assertThat(indices.size(), equalTo(0));
     }
 
     public void testGetRelevantIndicesWithUnassignedShardsOnDataOnlyNode() {

server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java

@@ -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
-        try {
-            indicesService.deleteIndexStore("boom", firstMetaData, clusterService.state());
-            fail();
-        } catch (IllegalStateException ex) {
-            // all good
-        }
         assertAcked(client().admin().indices().prepareOpen("test"));
         ensureGreen("test");
     }