Discovery: don't accept a dynamic update to min_master_nodes which is larger then current master node count

The discovery.zen.minimum_master_nodes setting can be updated dynamically. Settings it to a value higher then the current number of master nodes will cause the current master to step down. This is dangerous because if done by mistake (typo) there is no way to restore the settings (this requires an active master).

Closes #8321

src/main/java/org/elasticsearch/cluster/settings/ClusterDynamicSettingsModule.java

@@ -58,7 +58,6 @@ public class ClusterDynamicSettingsModule extends AbstractModule {
         clusterDynamicSettings.addDynamicSetting(DisableAllocationDecider.CLUSTER_ROUTING_ALLOCATION_DISABLE_NEW_ALLOCATION);
         clusterDynamicSettings.addDynamicSetting(DisableAllocationDecider.CLUSTER_ROUTING_ALLOCATION_DISABLE_ALLOCATION);
         clusterDynamicSettings.addDynamicSetting(DisableAllocationDecider.CLUSTER_ROUTING_ALLOCATION_DISABLE_REPLICA_ALLOCATION);
-        clusterDynamicSettings.addDynamicSetting(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES, Validator.INTEGER);
         clusterDynamicSettings.addDynamicSetting(ZenDiscovery.SETTING_REJOIN_ON_MASTER_GONE, Validator.BOOLEAN);
         clusterDynamicSettings.addDynamicSetting(DiscoverySettings.NO_MASTER_BLOCK);
         clusterDynamicSettings.addDynamicSetting(FilterAllocationDecider.CLUSTER_ROUTING_INCLUDE_GROUP + "*");

src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java

@@ -36,6 +36,9 @@ import org.elasticsearch.cluster.node.DiscoveryNodes;
 import org.elasticsearch.cluster.routing.allocation.AllocationService;
 import org.elasticsearch.cluster.routing.allocation.RoutingAllocation;
 import org.elasticsearch.cluster.service.InternalClusterService;
+import org.elasticsearch.cluster.settings.ClusterDynamicSettings;
+import org.elasticsearch.cluster.settings.DynamicSettings;
+import org.elasticsearch.cluster.settings.Validator;
 import org.elasticsearch.common.Priority;
 import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.component.AbstractLifecycleComponent;
@@ -144,9 +147,9 @@ public class ZenDiscovery extends AbstractLifecycleComponent<Discovery> implemen
 
     @Inject
     public ZenDiscovery(Settings settings, ClusterName clusterName, ThreadPool threadPool,
-                        TransportService transportService, ClusterService clusterService, NodeSettingsService nodeSettingsService,
+                        TransportService transportService, final ClusterService clusterService, NodeSettingsService nodeSettingsService,
                         DiscoveryNodeService discoveryNodeService, ZenPingService pingService, ElectMasterService electMasterService,
-                        DiscoverySettings discoverySettings) {
+                        DiscoverySettings discoverySettings, @ClusterDynamicSettings DynamicSettings dynamicSettings) {
         super(settings);
         this.clusterName = clusterName;
         this.clusterService = clusterService;
@@ -196,6 +199,24 @@ public class ZenDiscovery extends AbstractLifecycleComponent<Discovery> implemen
         this.joinThreadControl = new JoinThreadControl(threadPool);
 
         transportService.registerHandler(DISCOVERY_REJOIN_ACTION_NAME, new RejoinClusterRequestHandler());
+
+        dynamicSettings.addDynamicSetting(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES, new Validator() {
+            @Override
+            public String validate(String setting, String value) {
+                int intValue;
+                try {
+                    intValue = Integer.parseInt(value);
+                } catch (NumberFormatException ex) {
+                    return "cannot parse value [" + value + "] as an integer";
+                }
+                int masterNodes = clusterService.state().nodes().masterNodes().size();
+                if (intValue > masterNodes) {
+                    return "cannot set " + ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES + " to more than the current master nodes count [" + masterNodes + "]";
+                }
+                return null;
+            }
+        });
+
     }
 
     @Override

src/test/java/org/elasticsearch/cluster/ClusterServiceTests.java

@@ -608,7 +608,7 @@ public class ClusterServiceTests extends ElasticsearchIntegrationTest {
                 .put("plugin.types", TestPlugin.class.getName())
                 .build();
 
-        internalCluster().startNode(settings);
+        String node_0 = internalCluster().startNode(settings);
         ClusterService clusterService = internalCluster().getInstance(ClusterService.class);
         MasterAwareService testService = internalCluster().getInstance(MasterAwareService.class);
 
@@ -635,15 +635,22 @@ public class ClusterServiceTests extends ElasticsearchIntegrationTest {
         clusterHealth = client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForNodes("1").get();
         assertThat(clusterHealth.isTimedOut(), equalTo(false));
 
-        // now that node1 is closed, node2 should be elected as master
+        // now that node0 is closed, node1 should be elected as master
         assertThat(clusterService1.state().nodes().localNodeMaster(), is(true));
         assertThat(testService1.master(), is(true));
 
+        // start another node and set min_master_node
+        internalCluster().startNode(ImmutableSettings.builder().put(settings));
+        assertFalse(client().admin().cluster().prepareHealth().setWaitForNodes("2").get().isTimedOut());
+
         Settings transientSettings = settingsBuilder()
                 .put("discovery.zen.minimum_master_nodes", 2)
                 .build();
         client().admin().cluster().prepareUpdateSettings().setTransientSettings(transientSettings).get();
 
+        // and shutdown the second node
+        internalCluster().stopRandomNonMasterNode();
+
         // there should not be any master as the minimum number of required eligible masters is not met
         awaitBusy(new Predicate<Object>() {
             public boolean apply(Object obj) {
@@ -652,7 +659,7 @@ public class ClusterServiceTests extends ElasticsearchIntegrationTest {
         });
         assertThat(testService1.master(), is(false));
 
-
+        // bring the node back up
         String node_2 = internalCluster().startNode(ImmutableSettings.builder().put(settings).put(transientSettings));
         ClusterService clusterService2 = internalCluster().getInstance(ClusterService.class, node_2);
         MasterAwareService testService2 = internalCluster().getInstance(MasterAwareService.class, node_2);

src/test/java/org/elasticsearch/cluster/MinimumMasterNodesTests.java

@@ -25,6 +25,7 @@ import org.elasticsearch.action.admin.cluster.health.ClusterHealthStatus;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.common.Priority;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.settings.ImmutableSettings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.discovery.DiscoverySettings;
 import org.elasticsearch.discovery.zen.elect.ElectMasterService;
@@ -35,14 +36,17 @@ import org.elasticsearch.test.junit.annotations.TestLogging;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 
 import static org.elasticsearch.client.Requests.clusterHealthRequest;
 import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder;
 import static org.elasticsearch.test.ElasticsearchIntegrationTest.Scope;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows;
 import static org.hamcrest.Matchers.*;
 
-@ClusterScope(scope = Scope.TEST, numDataNodes =0)
+@ClusterScope(scope = Scope.TEST, numDataNodes = 0)
 public class MinimumMasterNodesTests extends ElasticsearchIntegrationTest {
 
     @Test
@@ -98,7 +102,7 @@ public class MinimumMasterNodesTests extends ElasticsearchIntegrationTest {
         internalCluster().stopCurrentMasterNode();
         awaitBusy(new Predicate<Object>() {
             public boolean apply(Object obj) {
-                ClusterState  state = client().admin().cluster().prepareState().setLocal(true).execute().actionGet().getState();
+                ClusterState state = client().admin().cluster().prepareState().setLocal(true).execute().actionGet().getState();
                 return state.blocks().hasGlobalBlock(DiscoverySettings.NO_MASTER_BLOCK_ID);
             }
         });
@@ -187,7 +191,7 @@ public class MinimumMasterNodesTests extends ElasticsearchIntegrationTest {
                 return state.blocks().hasGlobalBlock(DiscoverySettings.NO_MASTER_BLOCK_ID);
             }
         });
-        
+
         awaitBusy(new Predicate<Object>() {
             public boolean apply(Object obj) {
                 ClusterState state = client().admin().cluster().prepareState().setLocal(true).execute().actionGet().getState();
@@ -308,4 +312,40 @@ public class MinimumMasterNodesTests extends ElasticsearchIntegrationTest {
             }
         }, 20, TimeUnit.SECONDS), equalTo(true));
     }
+
+    @Test
+    public void testCanNotBringClusterDown() throws ExecutionException, InterruptedException {
+        int nodeCount = scaledRandomIntBetween(1, 10);
+        ImmutableSettings.Builder settings = settingsBuilder()
+                .put("discovery.type", "zen")
+                .put("discovery.zen.ping_timeout", "200ms")
+                .put("discovery.initial_state_timeout", "500ms")
+                .put("gateway.type", "local");
+
+        if (randomBoolean()) {
+            // sometime set an initial value
+            settings.put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES, randomIntBetween(1, nodeCount));
+        }
+
+        internalCluster().startNodesAsync(nodeCount, settings.build()).get();
+
+        client().admin().cluster().prepareHealth().setWaitForNodes("=" + nodeCount);
+
+        int updateCount = randomIntBetween(1, nodeCount);
+
+        logger.info("--> updating [{}] to [{}]", ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES, updateCount);
+        assertAcked(client().admin().cluster().prepareUpdateSettings()
+                .setPersistentSettings(settingsBuilder().put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES, updateCount)));
+
+        client().admin().cluster().prepareHealth().setWaitForNodes("=" + nodeCount);
+
+        updateCount = nodeCount + randomIntBetween(1, 2000);
+        logger.info("--> trying to updating [{}] to [{}]", ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES, updateCount);
+        assertThat(client().admin().cluster().prepareUpdateSettings()
+                        .setPersistentSettings(settingsBuilder().put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES, updateCount))
+                        .get().getPersistentSettings().getAsMap().keySet(),
+                empty());
+
+        client().admin().cluster().prepareHealth().setWaitForNodes("=" + nodeCount);
+    }
 }