Default max local storage nodes to one

This commit defaults the max local storage nodes to one. The motivation
for this change is that a default value greather than one is dangerous
as users sometimes end up unknowingly starting a second node and start
thinking that they have encountered data loss.

Relates #19964

Author: jasontedor

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy

@@ -261,6 +261,7 @@ class ClusterFormationTasks {
                 'node.attr.testattr'                : 'test',
                 'repositories.url.allowed_urls': 'http://snapshot.test*'
         ]
+        esConfig['node.max_local_storage_nodes'] = node.config.numNodes
         esConfig['http.port'] = node.config.httpPort
         esConfig['transport.tcp.port'] =  node.config.transportPort
         esConfig.putAll(node.config.settings)

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

@@ -68,6 +68,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Random;
 import java.util.Set;
@@ -151,7 +152,7 @@ public String toString() {
     /**
      * 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", 50, 1,
+    public static final Setting<Integer> MAX_LOCAL_STORAGE_NODES_SETTING = Setting.intSetting("node.max_local_storage_nodes", 1, 1,
         Property.NodeScope);
 
     /**
@@ -244,8 +245,15 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce
             }
 
             if (locks[0] == null) {
-                throw new IllegalStateException("Failed to obtain node lock, is the following location writable?: "
-                    + Arrays.toString(environment.dataWithClusterFiles()), lastException);
+                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.dataWithClusterFiles()),
+                    maxLocalStorageNodes == 1 ? " [0]" : "s [0--" + (maxLocalStorageNodes - 1) + "]",
+                    MAX_LOCAL_STORAGE_NODES_SETTING.getKey(),
+                    maxLocalStorageNodes);
+                throw new IllegalStateException(message, lastException);
             }
             this.nodeMetaData = loadOrCreateNodeMetaData(settings, startupTraceLogger, nodePaths);
             this.logger = Loggers.getLogger(getClass(), Node.addNodeNameIfNeeded(settings, this.nodeMetaData.nodeId()));

core/src/main/java/org/elasticsearch/tribe/TribeService.java

@@ -117,6 +117,9 @@ public static Settings processSettings(Settings settings) {
         sb.put(Node.NODE_MASTER_SETTING.getKey(), false);
         sb.put(Node.NODE_DATA_SETTING.getKey(), false);
         sb.put(Node.NODE_INGEST_SETTING.getKey(), false);
+        if (!NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING.exists(settings)) {
+            sb.put(NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING.getKey(), nodesSettings.size());
+        }
         sb.put(DiscoveryModule.DISCOVERY_TYPE_SETTING.getKey(), "local"); // a tribe node should not use zen discovery
         // nothing is going to be discovered, since no master will be elected
         sb.put(DiscoverySettings.INITIAL_STATE_TIMEOUT_SETTING.getKey(), 0);

core/src/test/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java

@@ -38,6 +38,7 @@
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
+import org.elasticsearch.env.NodeEnvironment;
 import org.elasticsearch.index.IndexService;
 import org.elasticsearch.index.shard.IndexShard;
 import org.elasticsearch.index.store.Store;
@@ -118,6 +119,7 @@ public void blockActions(String... actions) {
     protected Settings nodeSettings(int nodeOrdinal) {
         return Settings.builder()
                 // manual collection or upon cluster forming.
+                .put(NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING.getKey(), 2)
                 .put(InternalClusterInfoService.INTERNAL_CLUSTER_INFO_TIMEOUT_SETTING.getKey(), "1s")
                 .build();
     }

core/src/test/java/org/elasticsearch/discovery/DiscoveryWithServiceDisruptionsIT.java

@@ -55,6 +55,7 @@
 import org.elasticsearch.discovery.zen.ping.ZenPingService;
 import org.elasticsearch.discovery.zen.ping.unicast.UnicastZenPing;
 import org.elasticsearch.discovery.zen.publish.PublishClusterStateAction;
+import org.elasticsearch.env.NodeEnvironment;
 import org.elasticsearch.indices.store.IndicesStoreIntegrationIT;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.test.ESIntegTestCase;
@@ -207,6 +208,7 @@ private void configureUnicastCluster(
         // TODO: Rarely use default settings form some of these
         Settings nodeSettings = Settings.builder()
                 .put(settings)
+                .put(NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING.getKey(), 4)
                 .put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey(), minimumMasterNode)
                 .build();
 

core/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java

@@ -74,18 +74,14 @@ public void testNodeLockSillySettings() {
     }
 
     public void testNodeLockSingleEnvironment() throws IOException {
-        final Settings settings = buildEnvSettings(Settings.builder()
-            .put(NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING.getKey(), 1).build());
+        final Settings settings = buildEnvSettings(Settings.builder().put("node.max_local_storage_nodes", 1).build());
         NodeEnvironment env = newNodeEnvironment(settings);
         List<String> dataPaths = Environment.PATH_DATA_SETTING.get(settings);
 
-        try {
-            // Reuse the same location and attempt to lock again
-            new NodeEnvironment(settings, new Environment(settings));
-            fail("env has already locked all the data directories it is allowed");
-        } catch (IllegalStateException ex) {
-            assertThat(ex.getMessage(), containsString("Failed to obtain node lock"));
-        }
+        // Reuse the same location and attempt to lock again
+        IllegalStateException ex =
+            expectThrows(IllegalStateException.class, () -> new NodeEnvironment(settings, new Environment(settings)));
+        assertThat(ex.getMessage(), containsString("failed to obtain node lock"));
 
         // Close the environment that holds the lock and make sure we can get the lock after release
         env.close();
@@ -121,7 +117,7 @@ public void testSegmentInfosTracing() {
     }
 
     public void testNodeLockMultipleEnvironment() throws IOException {
-        final Settings settings = buildEnvSettings(Settings.EMPTY);
+        final Settings settings = buildEnvSettings(Settings.builder().put("node.max_local_storage_nodes", 2).build());
         final NodeEnvironment first = newNodeEnvironment(settings);
         List<String> dataPaths = Environment.PATH_DATA_SETTING.get(settings);
         NodeEnvironment second = new NodeEnvironment(settings, new Environment(settings));

core/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerNoopIT.java

@@ -22,6 +22,7 @@
 import org.elasticsearch.action.index.IndexRequestBuilder;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.env.NodeEnvironment;
 import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService;
 import org.elasticsearch.search.sort.SortOrder;
 import org.elasticsearch.test.ESIntegTestCase;
@@ -40,6 +41,7 @@ public class CircuitBreakerNoopIT extends ESIntegTestCase {
     @Override
     protected Settings nodeSettings(int nodeOrdinal) {
         return Settings.builder()
+                .put(NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING.getKey(), 2)
                 .put(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_TYPE_SETTING.getKey(), "noop")
                 // This is set low, because if the "noop" is not a noop, it will break
                 .put(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_LIMIT_SETTING.getKey(), "10b")

core/src/test/java/org/elasticsearch/tribe/TribeIT.java

@@ -39,6 +39,7 @@
 import org.elasticsearch.discovery.DiscoveryModule;
 import org.elasticsearch.discovery.MasterNotDiscoveredException;
 import org.elasticsearch.discovery.zen.ping.unicast.UnicastZenPing;
+import org.elasticsearch.env.NodeEnvironment;
 import org.elasticsearch.node.Node;
 import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.test.InternalTestCluster;
@@ -128,6 +129,7 @@ private void setupTribeNode(Settings settings) {
         tribe1Defaults.putArray("tribe.t2." + UnicastZenPing.DISCOVERY_ZEN_PING_UNICAST_HOSTS_SETTING.getKey(), getUnicastHosts(cluster2.client()));
 
         Settings merged = Settings.builder()
+                .put(internalCluster().getDefaultSettings())
                 .put("tribe.t1.cluster.name", internalCluster().getClusterName())
                 .put("tribe.t2.cluster.name", cluster2.getClusterName())
                 .put("tribe.t1.transport.type", "local")
@@ -142,7 +144,6 @@ private void setupTribeNode(Settings settings) {
 
                 .put(tribe1Defaults.build())
                 .put(tribe2Defaults.build())
-                .put(internalCluster().getDefaultSettings())
                 .put("node.name", "tribe_node") // make sure we can identify threads from this node
                 .build();
 

docs/reference/migration/migrate_5_0/settings.asciidoc

@@ -310,3 +310,14 @@ The unit 'w' representing weeks is no longer supported.
 
 Fractional time values (e.g., 0.5s) are no longer supported. For example, this means when setting
 timeouts "0.5s" will be rejected and should instead be input as "500ms".
+
+==== Node max local storage nodes
+
+Previous versions of Elasticsearch defaulted to allowing multiple nodes to share the same data
+directory (up to 50). This can be confusing where users accidentally startup multiple nodes and end
+up thinking that they've lost data because the second node will start with an empty data directory.
+While the default of allowing multiple nodes is friendly to playing with forming a small cluster on
+a laptop, and end-users do sometimes run multiple nodes on the same host, this tends to be the
+exception. Keeping with Elasticsearch's continual movement towards safer out-of-the-box defaults,
+and optimizing for the norm instead of the exception, the default for
+`node.max_local_storage_nodes` is now one.

qa/evil-tests/src/test/java/org/elasticsearch/tribe/TribeUnitTests.java

@@ -60,7 +60,9 @@ public static void createTribes() {
             .put(NetworkModule.HTTP_ENABLED.getKey(), false)
             .put("transport.type", "local")
             .put(DiscoveryModule.DISCOVERY_TYPE_SETTING.getKey(), "local")
-            .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()).build();
+            .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir())
+            .put(NodeEnvironment.MAX_LOCAL_STORAGE_NODES_SETTING.getKey(), 2)
+            .build();
 
         tribe1 = new TribeClientNode(
             Settings.builder()