Load the deprecated master role in a dedicated method instead of in s…

…etAdditionalRoles()

Signed-off-by: Tianli Feng <ftianli@amazon.com>

CHANGELOG.md

@@ -45,6 +45,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [CCR] Add getHistoryOperationsFromTranslog method to fetch the history snapshot from translogs ([#3948](https://github.com/opensearch-project/OpenSearch/pull/3948))
 - [Remote Store] Change behaviour in replica recovery for remote translog enabled indices ([#4318](https://github.com/opensearch-project/OpenSearch/pull/4318))
 - Unmute test RelocationIT.testRelocationWhileIndexingRandom ([#4580](https://github.com/opensearch-project/OpenSearch/pull/4580))
+- Load the deprecated master role in a dedicated method instead of in setAdditionalRoles() ([#4582](https://github.com/opensearch-project/OpenSearch/pull/4582))
 
 ### Deprecated
 

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

@@ -616,11 +616,18 @@ public static void setAdditionalRoles(final Set<DiscoveryNodeRole> additionalRol
             + "], roles by name abbreviation ["
             + roleNameAbbreviationToPossibleRoles
             + "]";
-        // TODO: Remove the Map 'roleNameToPossibleRolesWithMaster' and let 'roleMap = roleNameToPossibleRoles', after removing MASTER_ROLE.
-        // It's used to allow CLUSTER_MANAGER_ROLE that introduced in 2.0, having the same abbreviation name with MASTER_ROLE.
-        final Map<String, DiscoveryNodeRole> roleNameToPossibleRolesWithMaster = new HashMap<>(roleNameToPossibleRoles);
-        roleNameToPossibleRolesWithMaster.put(DiscoveryNodeRole.MASTER_ROLE.roleName(), DiscoveryNodeRole.MASTER_ROLE);
-        roleMap = Collections.unmodifiableMap(roleNameToPossibleRolesWithMaster);
+        roleMap = roleNameToPossibleRoles;
+    }
+
+    /**
+     * Load the deprecated {@link DiscoveryNodeRole#MASTER_ROLE}.
+     * Master role is not added into BUILT_IN_ROLES, because {@link #setAdditionalRoles(Set)} check role name abbreviation duplication,
+     * and CLUSTER_MANAGER_ROLE has the same abbreviation name with MASTER_ROLE.
+     */
+    public static void setDeprecatedMasterRole() {
+        final Map<String, DiscoveryNodeRole> modifiableRoleMap = new HashMap<>(roleMap);
+        modifiableRoleMap.put(DiscoveryNodeRole.MASTER_ROLE.roleName(), DiscoveryNodeRole.MASTER_ROLE);
+        roleMap = Collections.unmodifiableMap(modifiableRoleMap);
     }
 
     public static Set<String> getPossibleRoleNames() {

server/src/main/java/org/opensearch/node/Node.java

@@ -429,6 +429,8 @@ protected Node(
                 .collect(Collectors.toSet());
             DiscoveryNode.setAdditionalRoles(additionalRoles);
 
+            DiscoveryNode.setDeprecatedMasterRole();
+
             /*
              * Create the environment based on the finalized view of the settings. This is to ensure that components get the same setting
              * values, no matter they ask for them from.

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

@@ -56,8 +56,7 @@ public void testIsIngestNode() {
     }
 
     public void testIsMasterNode() {
-        // It's used to add MASTER_ROLE into 'roleMap', because MASTER_ROLE is removed from DiscoveryNodeRole.BUILT_IN_ROLES in 2.0.
-        DiscoveryNode.setAdditionalRoles(Collections.emptySet());
+        DiscoveryNode.setDeprecatedMasterRole();
         runRoleTest(DiscoveryNode::isClusterManagerNode, DiscoveryNodeRole.MASTER_ROLE);
     }
 

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

@@ -176,11 +176,11 @@ public void testDiscoveryNodeIsRemoteClusterClientUnset() {
     }
 
     // Added in 2.0 temporarily, validate the MASTER_ROLE is in the list of known roles.
-    // MASTER_ROLE was removed from BUILT_IN_ROLES and is imported by setAdditionalRoles(),
+    // MASTER_ROLE was removed from BUILT_IN_ROLES and is imported by setDeprecatedMasterRole(),
     // as a workaround for making the new CLUSTER_MANAGER_ROLE has got the same abbreviation 'm'.
     // The test validate this behavior.
-    public void testSetAdditionalRolesCanAddDeprecatedMasterRole() {
-        DiscoveryNode.setAdditionalRoles(Collections.emptySet());
+    public void testSetDeprecatedMasterRoleCanAddMasterRole() {
+        DiscoveryNode.setDeprecatedMasterRole();
         assertTrue(DiscoveryNode.getPossibleRoleNames().contains(DiscoveryNodeRole.MASTER_ROLE.roleName()));
     }
 

server/src/test/java/org/opensearch/node/NodeRoleSettingsTests.java

@@ -26,8 +26,7 @@ public class NodeRoleSettingsTests extends OpenSearchTestCase {
      * Remove the test after removing MASTER_ROLE.
      */
     public void testClusterManagerAndMasterRoleCanNotCoexist() {
-        // It's used to add MASTER_ROLE into 'roleMap', because MASTER_ROLE is removed from DiscoveryNodeRole.BUILT_IN_ROLES in 2.0.
-        DiscoveryNode.setAdditionalRoles(Collections.emptySet());
+        DiscoveryNode.setDeprecatedMasterRole();
         Settings roleSettings = Settings.builder().put(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), "cluster_manager, master").build();
         Exception exception = expectThrows(IllegalArgumentException.class, () -> NodeRoleSettings.NODE_ROLES_SETTING.get(roleSettings));
         assertThat(exception.getMessage(), containsString("[master, cluster_manager] can not be assigned together to a node"));
@@ -49,8 +48,7 @@ public void testClusterManagerAndDataNodeRoles() {
      * Remove the test after removing MASTER_ROLE.
      */
     public void testMasterRoleDeprecationMessage() {
-        // It's used to add MASTER_ROLE into 'roleMap', because MASTER_ROLE is removed from DiscoveryNodeRole.BUILT_IN_ROLES in 2.0.
-        DiscoveryNode.setAdditionalRoles(Collections.emptySet());
+        DiscoveryNode.setDeprecatedMasterRole();
         Settings roleSettings = Settings.builder().put(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), "master").build();
         assertEquals(Collections.singletonList(DiscoveryNodeRole.MASTER_ROLE), NodeRoleSettings.NODE_ROLES_SETTING.get(roleSettings));
         assertWarnings(DiscoveryNodeRole.MASTER_ROLE_DEPRECATION_MESSAGE);