[Backport 2.x] Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting

CHANGELOG.md

@@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - [Remote Store] Integrate remote segment store in peer recovery flow ([#6664](https://github.com/opensearch-project/OpenSearch/pull/6664))
 - [Segment Replication] Add new cluster setting to set replication strategy by default for all indices in cluster. ([#6791](https://github.com/opensearch-project/OpenSearch/pull/6791))
 - Enable sort optimization for all NumericTypes ([#6464](https://github.com/opensearch-project/OpenSearch/pull/6464)
+- Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting ([#6331](https://github.com/opensearch-project/OpenSearch/pull/6331))
 
 ### Dependencies
 - Bump `org.apache.logging.log4j:log4j-core` from 2.18.0 to 2.20.0 ([#6490](https://github.com/opensearch-project/OpenSearch/pull/6490))

server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/stats/ClusterStatsIT.java

@@ -35,6 +35,7 @@
 import org.opensearch.Version;
 import org.opensearch.action.admin.cluster.health.ClusterHealthResponse;
 import org.opensearch.action.admin.cluster.node.stats.NodeStats;
+import org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest;
 import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse;
 import org.opensearch.client.Requests;
 import org.opensearch.cluster.health.ClusterHealthStatus;
@@ -54,6 +55,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
@@ -83,14 +85,7 @@ private void waitForNodes(int numNodes) {
     public void testNodeCounts() {
         int total = 1;
         internalCluster().startNode();
-        Map<String, Integer> expectedCounts = new HashMap<>();
-        expectedCounts.put(DiscoveryNodeRole.DATA_ROLE.roleName(), 1);
-        expectedCounts.put(DiscoveryNodeRole.MASTER_ROLE.roleName(), 1);
-        expectedCounts.put(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(), 1);
-        expectedCounts.put(DiscoveryNodeRole.INGEST_ROLE.roleName(), 1);
-        expectedCounts.put(DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName(), 1);
-        expectedCounts.put(DiscoveryNodeRole.SEARCH_ROLE.roleName(), 0);
-        expectedCounts.put(ClusterStatsNodes.Counts.COORDINATING_ONLY, 0);
+        Map<String, Integer> expectedCounts = getExpectedCounts(1, 1, 1, 1, 1, 0, 0);
         int numNodes = randomIntBetween(1, 5);
 
         ClusterStatsResponse response = client().admin().cluster().prepareClusterStats().get();
@@ -147,25 +142,21 @@ public void testNodeCounts() {
     }
 
     // Validate assigning value "master" to setting "node.roles" can get correct count in Node Stats response after MASTER_ROLE deprecated.
-    public void testNodeCountsWithDeprecatedMasterRole() {
+    public void testNodeCountsWithDeprecatedMasterRole() throws ExecutionException, InterruptedException {
         int total = 1;
         Settings settings = Settings.builder()
             .putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), Collections.singletonList(DiscoveryNodeRole.MASTER_ROLE.roleName()))
             .build();
         internalCluster().startNode(settings);
         waitForNodes(total);
 
-        Map<String, Integer> expectedCounts = new HashMap<>();
-        expectedCounts.put(DiscoveryNodeRole.DATA_ROLE.roleName(), 0);
-        expectedCounts.put(DiscoveryNodeRole.MASTER_ROLE.roleName(), 1);
-        expectedCounts.put(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(), 1);
-        expectedCounts.put(DiscoveryNodeRole.INGEST_ROLE.roleName(), 0);
-        expectedCounts.put(DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName(), 0);
-        expectedCounts.put(DiscoveryNodeRole.SEARCH_ROLE.roleName(), 0);
-        expectedCounts.put(ClusterStatsNodes.Counts.COORDINATING_ONLY, 0);
+        Map<String, Integer> expectedCounts = getExpectedCounts(0, 1, 1, 0, 0, 0, 0);
 
         ClusterStatsResponse response = client().admin().cluster().prepareClusterStats().get();
         assertCounts(response.getNodesStats().getCounts(), total, expectedCounts);
+
+        Set<String> expectedRoles = Set.of(DiscoveryNodeRole.MASTER_ROLE.roleName());
+        assertEquals(expectedRoles, getNodeRoles(0));
     }
 
     private static void incrementCountForRole(String role, Map<String, Integer> counts) {
@@ -322,4 +313,136 @@ public void testFieldTypes() {
             }
         }
     }
+
+    public void testNodeRolesWithMasterLegacySettings() throws ExecutionException, InterruptedException {
+        int total = 1;
+        Settings legacyMasterSettings = Settings.builder()
+            .put("node.master", true)
+            .put("node.data", false)
+            .put("node.ingest", false)
+            .build();
+
+        internalCluster().startNodes(legacyMasterSettings);
+        waitForNodes(total);
+
+        Map<String, Integer> expectedCounts = getExpectedCounts(0, 1, 1, 0, 1, 0, 0);
+
+        ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get();
+        assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedCounts);
+
+        Set<String> expectedRoles = Set.of(
+            DiscoveryNodeRole.MASTER_ROLE.roleName(),
+            DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName()
+        );
+        assertEquals(expectedRoles, getNodeRoles(0));
+    }
+
+    public void testNodeRolesWithClusterManagerRole() throws ExecutionException, InterruptedException {
+        int total = 1;
+        Settings clusterManagerNodeRoleSettings = Settings.builder()
+            .put(
+                "node.roles",
+                String.format(
+                    Locale.ROOT,
+                    "%s, %s",
+                    DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(),
+                    DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName()
+                )
+            )
+            .build();
+
+        internalCluster().startNodes(clusterManagerNodeRoleSettings);
+        waitForNodes(total);
+
+        Map<String, Integer> expectedCounts = getExpectedCounts(0, 1, 1, 0, 1, 0, 0);
+
+        ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get();
+        assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedCounts);
+
+        Set<String> expectedRoles = Set.of(
+            DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(),
+            DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName()
+        );
+        assertEquals(expectedRoles, getNodeRoles(0));
+    }
+
+    public void testNodeRolesWithSeedDataNodeLegacySettings() throws ExecutionException, InterruptedException {
+        int total = 1;
+        Settings legacySeedDataNodeSettings = Settings.builder()
+            .put("node.master", true)
+            .put("node.data", true)
+            .put("node.ingest", false)
+            .build();
+
+        internalCluster().startNodes(legacySeedDataNodeSettings);
+        waitForNodes(total);
+
+        Map<String, Integer> expectedRoleCounts = getExpectedCounts(1, 1, 1, 0, 1, 0, 0);
+
+        ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get();
+        assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedRoleCounts);
+
+        Set<String> expectedRoles = Set.of(
+            DiscoveryNodeRole.MASTER_ROLE.roleName(),
+            DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName(),
+            DiscoveryNodeRole.DATA_ROLE.roleName()
+        );
+        assertEquals(expectedRoles, getNodeRoles(0));
+    }
+
+    public void testNodeRolesWithDataNodeLegacySettings() throws ExecutionException, InterruptedException {
+        int total = 2;
+        Settings legacyDataNodeSettings = Settings.builder()
+            .put("node.master", false)
+            .put("node.data", true)
+            .put("node.ingest", false)
+            .build();
+
+        // can't start data-only node without assigning cluster-manager
+        internalCluster().startClusterManagerOnlyNodes(1);
+        internalCluster().startNodes(legacyDataNodeSettings);
+        waitForNodes(total);
+
+        Map<String, Integer> expectedRoleCounts = getExpectedCounts(1, 1, 1, 0, 1, 0, 0);
+
+        ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get();
+        assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedRoleCounts);
+
+        Set<Set<String>> expectedNodesRoles = Set.of(
+            Set.of(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName()),
+            Set.of(DiscoveryNodeRole.DATA_ROLE.roleName(), DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName())
+        );
+        assertEquals(expectedNodesRoles, Set.of(getNodeRoles(0), getNodeRoles(1)));
+    }
+
+    private Map<String, Integer> getExpectedCounts(
+        int dataRoleCount,
+        int masterRoleCount,
+        int clusterManagerRoleCount,
+        int ingestRoleCount,
+        int remoteClusterClientRoleCount,
+        int searchRoleCount,
+        int coordinatingOnlyCount
+    ) {
+        Map<String, Integer> expectedCounts = new HashMap<>();
+        expectedCounts.put(DiscoveryNodeRole.DATA_ROLE.roleName(), dataRoleCount);
+        expectedCounts.put(DiscoveryNodeRole.MASTER_ROLE.roleName(), masterRoleCount);
+        expectedCounts.put(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(), clusterManagerRoleCount);
+        expectedCounts.put(DiscoveryNodeRole.INGEST_ROLE.roleName(), ingestRoleCount);
+        expectedCounts.put(DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName(), remoteClusterClientRoleCount);
+        expectedCounts.put(DiscoveryNodeRole.SEARCH_ROLE.roleName(), searchRoleCount);
+        expectedCounts.put(ClusterStatsNodes.Counts.COORDINATING_ONLY, coordinatingOnlyCount);
+        return expectedCounts;
+    }
+
+    private Set<String> getNodeRoles(int nodeNumber) throws ExecutionException, InterruptedException {
+        NodesStatsResponse nodesStatsResponse = client().admin().cluster().nodesStats(new NodesStatsRequest()).get();
+        return nodesStatsResponse.getNodes()
+            .get(nodeNumber)
+            .getNode()
+            .getRoles()
+            .stream()
+            .map(DiscoveryNodeRole::roleName)
+            .collect(Collectors.toSet());
+    }
 }

server/src/internalClusterTest/java/org/opensearch/cluster/node/DiscoveryNodeRoleIT.java

@@ -46,7 +46,9 @@
 import java.util.List;
 import java.util.Set;
 
+import static org.hamcrest.Matchers.startsWith;
 import static org.opensearch.test.NodeRoles.addRoles;
+import static org.opensearch.test.NodeRoles.onlyRole;
 import static org.opensearch.test.NodeRoles.removeRoles;
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.hasSize;
@@ -126,4 +128,18 @@ private void runTestNodeHasAdditionalRole(final Settings settings) {
         assertThat(response.getNodes().get(0).getNode().getRoles(), matcher);
     }
 
+    public void testStartNodeWithClusterManagerRoleAndMasterSetting() {
+        final Settings settings = Settings.builder()
+            .put("node.master", randomBoolean())
+            .put(onlyRole(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE))
+            .build();
+
+        final IllegalArgumentException e1 = expectThrows(
+            IllegalArgumentException.class,
+            () -> DiscoveryNode.getRolesFromSettings(settings)
+        );
+        assertThat(e1.getMessage(), startsWith("can not explicitly configure node roles and use legacy role setting"));
+        final IllegalArgumentException e2 = expectThrows(IllegalArgumentException.class, () -> internalCluster().startNodes(settings));
+        assertThat(e2.getMessage(), startsWith("can not explicitly configure node roles and use legacy role setting"));
+    }
 }

server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java

@@ -287,10 +287,7 @@ public static Set<DiscoveryNodeRole> getRolesFromSettings(final Settings setting
             validateLegacySettings(settings, roleMap);
             return Collections.unmodifiableSet(new HashSet<>(NODE_ROLES_SETTING.get(settings)));
         } else {
-            return roleMap.values()
-                .stream()
-                .filter(s -> s.legacySetting() != null && s.legacySetting().get(settings))
-                .collect(Collectors.toSet());
+            return roleMap.values().stream().filter(s -> s.isEnabledByDefault(settings)).collect(Collectors.toSet());
         }
     }
 

server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java

@@ -34,6 +34,7 @@
 
 import org.opensearch.LegacyESVersion;
 import org.opensearch.Version;
+import org.opensearch.common.Booleans;
 import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.Setting.Property;
@@ -245,8 +246,8 @@ public void validateRole(List<DiscoveryNodeRole> roles) {
 
         @Override
         public Setting<Boolean> legacySetting() {
-            // copy the setting here so we can mark it private in org.opensearch.node.Node
-            return Setting.boolSetting("node.master", true, Property.Deprecated, Property.NodeScope);
+            // 'cluster_manager' role should not configure legacy setting since deprecated 'master' role is supported till OS 2.x
+            return null;
         }
 
         @Override
@@ -273,6 +274,10 @@ public void validateRole(List<DiscoveryNodeRole> roles) {
             }
         }
 
+        @Override
+        public boolean isEnabledByDefault(final Settings settings) {
+            return Booleans.isBoolean(settings.get("node.master")) == false;
+        }
     };
 
     public static final DiscoveryNodeRole REMOTE_CLUSTER_CLIENT_ROLE = new DiscoveryNodeRole("remote_cluster_client", "r") {

server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleSettingTests.java

@@ -60,10 +60,6 @@ public void testIsMasterNode() {
         runRoleTest(DiscoveryNode::isClusterManagerNode, DiscoveryNodeRole.MASTER_ROLE);
     }
 
-    public void testIsClusterManagerNode() {
-        runRoleTest(DiscoveryNode::isClusterManagerNode, DiscoveryNodeRole.CLUSTER_MANAGER_ROLE);
-    }
-
     public void testIsRemoteClusterClient() {
         runRoleTest(DiscoveryNode::isRemoteClusterClient, DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE);
     }
@@ -96,5 +92,4 @@ private void runRoleTest(final Predicate<Settings> predicate, final DiscoveryNod
         assertThat(e.getMessage(), startsWith("can not explicitly configure node roles and use legacy role setting"));
         assertNoDeprecationWarnings();
     }
-
 }