Remove node.max_local_storage_nodes (#42428)

This setting, which prior to Elasticsearch 5 was enabled by default and caused all kinds of
confusion, has since been disabled by default and is not recommended for production use. The
preferred way going forward is for users to explicitly specify separate data folders for each started
node to ensure that each node is consistently assigned to the same data path.

Relates to #42426

Author: ywelsch

docs/reference/commands/node-tool.asciidoc

@@ -13,7 +13,7 @@ with the data on disk.
 [source,shell]
 --------------------------------------------------
 bin/elasticsearch-node repurpose|unsafe-bootstrap|detach-cluster|override-version
-  [--ordinal <Integer>] [-E <KeyValuePair>]
+  [-E <KeyValuePair>]
   [-h, --help] ([-s, --silent] | [-v, --verbose])
 --------------------------------------------------
 
@@ -290,10 +290,6 @@ it can join a different cluster.
 `override-version`:: Overwrites the version number stored in the data path so
 that a node can start despite being incompatible with the on-disk data.
 
-`--ordinal <Integer>`:: If there is <<max-local-storage-nodes,more than one
-node sharing a data path>> then this specifies which node to target. Defaults
-to `0`, meaning to use the first node in the data path.
-
 `-E <KeyValuePair>`:: Configures a setting.
 
 `-h, --help`:: Returns all of the command parameters.

docs/reference/migration/migrate_8_0.asciidoc

@@ -20,6 +20,7 @@ coming[8.0.0]
 * <<breaking_80_ilm_changes>>
 * <<breaking_80_java_changes>>
 * <<breaking_80_network_changes>>
+* <<breaking_80_node_changes>>
 * <<breaking_80_transport_changes>>
 * <<breaking_80_http_changes>>
 * <<breaking_80_reindex_changes>>
@@ -54,6 +55,7 @@ include::migrate_8_0/security.asciidoc[]
 include::migrate_8_0/ilm.asciidoc[]
 include::migrate_8_0/java.asciidoc[]
 include::migrate_8_0/network.asciidoc[]
+include::migrate_8_0/node.asciidoc[]
 include::migrate_8_0/transport.asciidoc[]
 include::migrate_8_0/http.asciidoc[]
 include::migrate_8_0/reindex.asciidoc[]

docs/reference/migration/migrate_8_0/node.asciidoc

@@ -0,0 +1,16 @@
+[float]
+[[breaking_80_node_changes]]
+=== Node changes
+
+//NOTE: The notable-breaking-changes tagged regions are re-used in the
+//Installation and Upgrade Guide
+//tag::notable-breaking-changes[]
+
+// end::notable-breaking-changes[]
+
+[float]
+==== Removal of `node.max_local_storage_nodes` setting
+
+The `node.max_local_storage_nodes` setting was deprecated in 7.x and
+has been removed in 8.0. Nodes should be run on separate data paths
+to ensure that each node is consistently assigned to the same data path.

docs/reference/modules/node.asciidoc

@@ -277,21 +277,6 @@ home directory, so that the home directory can be deleted without deleting
 your data! The RPM and Debian distributions do this for you already.
 
 
-[float]
-[[max-local-storage-nodes]]
-=== `node.max_local_storage_nodes`
-
-The <<data-path,data path>> can be shared by multiple nodes, even by nodes from different
-clusters. This is very useful for testing failover and different configurations on your development
-machine. In production, however, it is recommended to run only one node of Elasticsearch per server.
-
-By default, Elasticsearch is configured to prevent more than one node from sharing the same data
-path. To allow for more than one node (e.g., on your development machine), use the setting
-`node.max_local_storage_nodes` and set this to a positive integer larger than one.
-
-WARNING: Never run different node types (i.e. master, data) from the same data directory. This can
-lead to unexpected data loss.
-
 [float]
 == Other node settings
 

qa/evil-tests/src/test/java/org/elasticsearch/env/NodeEnvironmentEvilTests.java

@@ -51,10 +51,10 @@ public void testMissingWritePermission() throws IOException {
             Settings build = Settings.builder()
                     .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath().toString())
                     .putList(Environment.PATH_DATA_SETTING.getKey(), tempPaths).build();
-            IOException ioException = expectThrows(IOException.class, () -> {
+            IOException exception = expectThrows(IOException.class, () -> {
                 new NodeEnvironment(build, TestEnvironment.newEnvironment(build));
             });
-            assertTrue(ioException.getMessage(), ioException.getMessage().startsWith(path.toString()));
+            assertTrue(exception.getMessage(), exception.getMessage().startsWith(path.toString()));
         }
     }
 

server/src/main/java/org/elasticsearch/cluster/coordination/ElasticsearchNodeCommand.java

@@ -20,7 +20,6 @@
 
 import joptsimple.OptionParser;
 import joptsimple.OptionSet;
-import joptsimple.OptionSpec;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.lucene.store.LockObtainFailedException;
@@ -59,22 +58,15 @@ public abstract class ElasticsearchNodeCommand extends EnvironmentAwareCommand {
     static final String NO_GLOBAL_METADATA_MSG = "failed to find global metadata, metadata corrupted?";
     static final String WRITE_METADATA_EXCEPTION_MSG = "exception occurred when writing new metadata to disk";
     protected static final String ABORTED_BY_USER_MSG = "aborted by user";
-    final OptionSpec<Integer> nodeOrdinalOption;
 
     public ElasticsearchNodeCommand(String description) {
         super(description);
-        nodeOrdinalOption = parser.accepts("ordinal", "Optional node ordinal, 0 if not specified")
-                .withRequiredArg().ofType(Integer.class);
         namedXContentRegistry = new NamedXContentRegistry(ClusterModule.getNamedXWriteables());
     }
 
-    protected void processNodePathsWithLock(Terminal terminal, OptionSet options, Environment env) throws IOException {
+    protected void processNodePaths(Terminal terminal, OptionSet options, Environment env) throws IOException {
         terminal.println(Terminal.Verbosity.VERBOSE, "Obtaining lock for node");
-        Integer nodeOrdinal = nodeOrdinalOption.value(options);
-        if (nodeOrdinal == null) {
-            nodeOrdinal = 0;
-        }
-        try (NodeEnvironment.NodeLock lock = new NodeEnvironment.NodeLock(nodeOrdinal, logger, env, Files::exists)) {
+        try (NodeEnvironment.NodeLock lock = new NodeEnvironment.NodeLock(logger, env, Files::exists)) {
             final Path[] dataPaths =
                     Arrays.stream(lock.getNodePaths()).filter(Objects::nonNull).map(p -> p.path).toArray(Path[]::new);
             if (dataPaths.length == 0) {
@@ -118,7 +110,7 @@ protected void confirm(Terminal terminal, String msg) {
     protected final void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
         terminal.println(STOP_WARNING_MSG);
         if (validateBeforeLock(terminal, env)) {
-            processNodePathsWithLock(terminal, options, env);
+            processNodePaths(terminal, options, env);
         }
     }
 

server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java

@@ -390,7 +390,6 @@ public void apply(Settings value, Settings current, Settings previous) {
             ThreadContext.DEFAULT_HEADERS_SETTING,
             Loggers.LOG_DEFAULT_LEVEL_SETTING,
             Loggers.LOG_LEVEL_SETTING,
-            NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING,
             NodeEnvironment.ENABLE_LUCENE_SEGMENT_INFOS_TRACE_SETTING,
             OsService.REFRESH_INTERVAL_SETTING,
             ProcessService.REFRESH_INTERVAL_SETTING,

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java

@@ -81,7 +81,6 @@
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -91,9 +90,9 @@
  */
 public final class NodeEnvironment  implements Closeable {
     public static class NodePath {
-        /* ${data.paths}/nodes/{node.id} */
+        /* ${data.paths}/nodes/0 */
         public final Path path;
-        /* ${data.paths}/nodes/{node.id}/indices */
+        /* ${data.paths}/nodes/0/indices */
         public final Path indicesPath;
         /** Cached FileStore from path */
         public final FileStore fileStore;
@@ -152,18 +151,11 @@ public String toString() {
     private final Path sharedDataPath;
     private final Lock[] locks;
 
-    private final int nodeLockId;
     private final AtomicBoolean closed = new AtomicBoolean(false);
     private final Map<ShardId, InternalShardLock> shardLocks = new HashMap<>();
 
     private final NodeMetaData nodeMetaData;
 
-    /**
-     * Maximum number of data nodes that should run in an environment.
-     */
-    public static final Setting<Integer> MAX_LOCAL_STORAGE_NODES_SETTING = Setting.intSetting("node.max_local_storage_nodes", 1, 1,
-        Property.NodeScope);
-
     /**
      * Seed for determining a persisted unique uuid of this node. If the node has already a persisted uuid on disk,
      * this seed will be ignored and the uuid from disk will be reused.
@@ -184,25 +176,23 @@ public String toString() {
 
     public static class NodeLock implements Releasable {
 
-        private final int nodeId;
         private final Lock[] locks;
         private final NodePath[] nodePaths;
 
         /**
          * Tries to acquire a node lock for a node id, throws {@code IOException} if it is unable to acquire it
          * @param pathFunction function to check node path before attempt of acquiring a node lock
          */
-        public NodeLock(final int nodeId, final Logger logger,
+        public NodeLock(final Logger logger,
                         final Environment environment,
                         final CheckedFunction<Path, Boolean, IOException> pathFunction) throws IOException {
-            this.nodeId = nodeId;
             nodePaths = new NodePath[environment.dataFiles().length];
             locks = new Lock[nodePaths.length];
             try {
                 final Path[] dataPaths = environment.dataFiles();
                 for (int dirIndex = 0; dirIndex < dataPaths.length; dirIndex++) {
                     Path dataDir = dataPaths[dirIndex];
-                    Path dir = resolveNodePath(dataDir, nodeId);
+                    Path dir = resolveNodePath(dataDir);
                     if (pathFunction.apply(dir) == false) {
                         continue;
                     }
@@ -248,61 +238,35 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce
             nodePaths = null;
             sharedDataPath = null;
             locks = null;
-            nodeLockId = -1;
             nodeMetaData = new NodeMetaData(generateNodeId(settings), Version.CURRENT);
             return;
         }
         boolean success = false;
-        NodeLock nodeLock = null;
 
         try {
             sharedDataPath = environment.sharedDataFile();
-            IOException lastException = null;
-            int maxLocalStorageNodes = MAX_LOCAL_STORAGE_NODES_SETTING.get(settings);
 
-            final AtomicReference<IOException> onCreateDirectoriesException = new AtomicReference<>();
-            for (int possibleLockId = 0; possibleLockId < maxLocalStorageNodes; possibleLockId++) {
-                try {
-                    nodeLock = new NodeLock(possibleLockId, logger, environment,
-                        dir -> {
-                            try {
-                                Files.createDirectories(dir);
-                            } catch (IOException e) {
-                                onCreateDirectoriesException.set(e);
-                                throw e;
-                            }
-                            return true;
-                        });
-                    break;
-                } catch (LockObtainFailedException e) {
-                    // ignore any LockObtainFailedException
-                } catch (IOException e) {
-                    if (onCreateDirectoriesException.get() != null) {
-                        throw onCreateDirectoriesException.get();
-                    }
-                    lastException = e;
-                }
+            for (Path path : environment.dataFiles()) {
+                Files.createDirectories(resolveNodePath(path));
             }
 
-            if (nodeLock == null) {
+            final NodeLock nodeLock;
+            try {
+                nodeLock = new NodeLock(logger, environment, dir -> true);
+            } catch (IOException e) {
                 final String message = String.format(
                     Locale.ROOT,
-                    "failed to obtain node locks, tried [%s] with lock id%s;" +
-                        " maybe these locations are not writable or multiple nodes were started without increasing [%s] (was [%d])?",
-                    Arrays.toString(environment.dataFiles()),
-                    maxLocalStorageNodes == 1 ? " [0]" : "s [0--" + (maxLocalStorageNodes - 1) + "]",
-                    MAX_LOCAL_STORAGE_NODES_SETTING.getKey(),
-                    maxLocalStorageNodes);
-                throw new IllegalStateException(message, lastException);
+                    "failed to obtain node locks, tried %s;" +
+                        " maybe these locations are not writable or multiple nodes were started on the same data path?",
+                    Arrays.toString(environment.dataFiles()));
+                throw new IllegalStateException(message, e);
             }
+
             this.locks = nodeLock.locks;
             this.nodePaths = nodeLock.nodePaths;
-            this.nodeLockId = nodeLock.nodeId;
             this.nodeMetaData = loadOrCreateNodeMetaData(settings, logger, nodePaths);
 
-            if (logger.isDebugEnabled()) {
-                logger.debug("using node location [{}], local_lock_id [{}]", nodePaths, nodeLockId);
-            }
+            logger.debug("using node location {}", Arrays.toString(nodePaths));
 
             maybeLogPathDetails();
             maybeLogHeapDetails();
@@ -334,11 +298,10 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce
      * Resolve a specific nodes/{node.id} path for the specified path and node lock id.
      *
      * @param path       the path
-     * @param nodeLockId the node lock id
      * @return the resolved path
      */
-    public static Path resolveNodePath(final Path path, final int nodeLockId) {
-        return path.resolve(NODES_FOLDER).resolve(Integer.toString(nodeLockId));
+    public static Path resolveNodePath(final Path path) {
+        return path.resolve(NODES_FOLDER).resolve("0");
     }
 
     private void maybeLogPathDetails() throws IOException {
@@ -805,14 +768,6 @@ public NodePath[] nodePaths() {
         return nodePaths;
     }
 
-    public int getNodeLockId() {
-        assertEnvIsLocked();
-        if (nodePaths == null || locks == null) {
-            throw new IllegalStateException("node is not configured to store local location");
-        }
-        return nodeLockId;
-    }
-
     /**
      * Returns all index paths.
      */
@@ -1137,12 +1092,12 @@ private static boolean isIndexMetaDataPath(Path path) {
      *
      * @param indexSettings settings for the index
      */
-    public static Path resolveBaseCustomLocation(IndexSettings indexSettings, Path sharedDataPath, int nodeLockId) {
+    public static Path resolveBaseCustomLocation(IndexSettings indexSettings, Path sharedDataPath) {
         String customDataDir = indexSettings.customDataPath();
         if (customDataDir != null) {
             // This assert is because this should be caught by MetaDataCreateIndexService
             assert sharedDataPath != null;
-            return sharedDataPath.resolve(customDataDir).resolve(Integer.toString(nodeLockId));
+            return sharedDataPath.resolve(customDataDir).resolve("0");
         } else {
             throw new IllegalArgumentException("no custom " + IndexMetaData.SETTING_DATA_PATH + " setting available");
         }
@@ -1156,11 +1111,11 @@ public static Path resolveBaseCustomLocation(IndexSettings indexSettings, Path s
      * @param indexSettings settings for the index
      */
     private Path resolveIndexCustomLocation(IndexSettings indexSettings) {
-        return resolveIndexCustomLocation(indexSettings, sharedDataPath, nodeLockId);
+        return resolveIndexCustomLocation(indexSettings, sharedDataPath);
     }
 
-    private static Path resolveIndexCustomLocation(IndexSettings indexSettings, Path sharedDataPath, int nodeLockId) {
-        return resolveBaseCustomLocation(indexSettings, sharedDataPath, nodeLockId).resolve(indexSettings.getUUID());
+    private static Path resolveIndexCustomLocation(IndexSettings indexSettings, Path sharedDataPath) {
+        return resolveBaseCustomLocation(indexSettings, sharedDataPath).resolve(indexSettings.getUUID());
     }
 
     /**
@@ -1172,11 +1127,11 @@ private static Path resolveIndexCustomLocation(IndexSettings indexSettings, Path
      * @param shardId shard to resolve the path to
      */
     public Path resolveCustomLocation(IndexSettings indexSettings, final ShardId shardId) {
-        return resolveCustomLocation(indexSettings, shardId, sharedDataPath, nodeLockId);
+        return resolveCustomLocation(indexSettings, shardId, sharedDataPath);
     }
 
-    public static Path resolveCustomLocation(IndexSettings indexSettings, final ShardId shardId, Path sharedDataPath, int nodeLockId) {
-        return resolveIndexCustomLocation(indexSettings, sharedDataPath, nodeLockId).resolve(Integer.toString(shardId.id()));
+    public static Path resolveCustomLocation(IndexSettings indexSettings, final ShardId shardId, Path sharedDataPath) {
+        return resolveIndexCustomLocation(indexSettings, sharedDataPath).resolve(Integer.toString(shardId.id()));
     }
 
     /**