Enforce `discovery.zen.minimum_master_nodes` is set when bound to a public ip #17288

discovery.zen.minimum_master_nodes is the single most important setting to set on a production cluster. We have no way of supplying a good default so it must be set by the user. Binding a node to a public IP (as opposed to the default local host) is a good enough indication that a node will be part of a production cluster cluster and thus it's a good tradeoff to enforce the settings. Note that nothing prevent users from setting it to 1 in a single node cluster.

Closes #17288

core/src/main/java/org/elasticsearch/bootstrap/BootstrapCheck.java

@@ -25,6 +25,7 @@ import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.network.NetworkService;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.discovery.zen.elect.ElectMasterService;
 import org.elasticsearch.monitor.process.ProcessProbe;
 import org.elasticsearch.transport.TransportSettings;
 
@@ -39,7 +40,6 @@ import java.util.Set;
 /**
  * We enforce limits once any network host is configured. In this case we assume the node is running in production
  * and all production limit checks must pass. This should be extended as we go to settings like:
- * - discovery.zen.minimum_master_nodes
  * - discovery.zen.ping.unicast.hosts is set if we use zen disco
  * - ensure we can write in all data directories
  * - fail if vm.max_map_count is under a certain limit (not sure if this works cross platform)
@@ -114,10 +114,10 @@ final class BootstrapCheck {
     }
 
     // the list of checks to execute
-    private static List<Check> checks(final Settings settings) {
+    static List<Check> checks(final Settings settings) {
         final List<Check> checks = new ArrayList<>();
         final FileDescriptorCheck fileDescriptorCheck
-                = Constants.MAC_OS_X ? new OsXFileDescriptorCheck() : new FileDescriptorCheck();
+            = Constants.MAC_OS_X ? new OsXFileDescriptorCheck() : new FileDescriptorCheck();
         checks.add(fileDescriptorCheck);
         checks.add(new MlockallCheck(BootstrapSettings.MLOCKALL_SETTING.get(settings)));
         if (Constants.LINUX) {
@@ -126,6 +126,7 @@ final class BootstrapCheck {
         if (Constants.LINUX || Constants.MAC_OS_X) {
             checks.add(new MaxSizeVirtualMemoryCheck());
         }
+        checks.add(new MinMasterNodesCheck(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.exists(settings)));
         return Collections.unmodifiableList(checks);
     }
 
@@ -186,10 +187,10 @@ final class BootstrapCheck {
         @Override
         public final String errorMessage() {
             return String.format(
-                    Locale.ROOT,
-                    "max file descriptors [%d] for elasticsearch process likely too low, increase to at least [%d]",
-                    getMaxFileDescriptorCount(),
-                    limit
+                Locale.ROOT,
+                "max file descriptors [%d] for elasticsearch process likely too low, increase to at least [%d]",
+                getMaxFileDescriptorCount(),
+                limit
             );
         }
 
@@ -226,6 +227,26 @@ final class BootstrapCheck {
 
     }
 
+    static class MinMasterNodesCheck implements Check {
+
+        final boolean minMasterNodesIsSet;
+
+        MinMasterNodesCheck(boolean minMasterNodesIsSet) {
+            this.minMasterNodesIsSet = minMasterNodesIsSet;
+        }
+
+        @Override
+        public boolean check() {
+            return minMasterNodesIsSet == false;
+        }
+
+        @Override
+        public String errorMessage() {
+            return "please set [" + ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey() +
+                "] to a majority of the number of master eligible nodes in your cluster.";
+        }
+    }
+
     static class MaxNumberOfThreadsCheck implements Check {
 
         private final long maxNumberOfThreadsThreshold = 1 << 11;

core/src/test/java/org/elasticsearch/bootstrap/BootstrapCheckTests.java

@@ -32,6 +32,8 @@ import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
 
 import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.Matchers.not;
 
 public class BootstrapCheckTests extends ESTestCase {
 
@@ -80,9 +82,9 @@ public class BootstrapCheckTests extends ESTestCase {
 
     public void testFileDescriptorLimitsThrowsOnInvalidLimit() {
         final IllegalArgumentException e =
-                expectThrows(
-                        IllegalArgumentException.class,
-                        () -> new BootstrapCheck.FileDescriptorCheck(-randomIntBetween(0, Integer.MAX_VALUE)));
+            expectThrows(
+                IllegalArgumentException.class,
+                () -> new BootstrapCheck.FileDescriptorCheck(-randomIntBetween(0, Integer.MAX_VALUE)));
         assertThat(e.getMessage(), containsString("limit must be positive but was"));
     }
 
@@ -121,8 +123,8 @@ public class BootstrapCheckTests extends ESTestCase {
                     fail("should have failed due to memory not being locked");
                 } catch (final RuntimeException e) {
                     assertThat(
-                            e.getMessage(),
-                            containsString("memory locking requested for elasticsearch process but memory is not locked"));
+                        e.getMessage(),
+                        containsString("memory locking requested for elasticsearch process but memory is not locked"));
                 }
             } else {
                 // nothing should happen
@@ -197,4 +199,12 @@ public class BootstrapCheckTests extends ESTestCase {
         assertTrue(BootstrapCheck.enforceLimits(settings));
     }
 
+    public void testMinMasterNodes() {
+        boolean isSet = randomBoolean();
+        BootstrapCheck.Check check = new BootstrapCheck.MinMasterNodesCheck(isSet);
+        assertThat(check.check(), not(equalTo(isSet)));
+        List<BootstrapCheck.Check> defaultChecks = BootstrapCheck.checks(Settings.EMPTY);
+
+        expectThrows(RuntimeException.class, () -> BootstrapCheck.check(true, defaultChecks));
+    }
 }

docs/reference/migration/migrate_5_0/settings.asciidoc

@@ -210,3 +210,11 @@ setting settings via `--name.of.setting value.of.setting`. This feature
 has been removed. Instead, use
 `-Ees.name.of.setting=value.of.setting`. Note that in all cases the
 name of the setting must be prefixed with `es.`.
+
+==== Discovery Settings
+
+The `discovery.zen.minimum_master_node` must bet set for nodes that are bound
+to a non-loopback network interface. We see those nodes as in "production" mode and
+thus require the setting.
+
+