Support unknown node role

Currently OpenSearch only supports several built-in nodes like data node role. If specify unknown node role, OpenSearch node will fail to start. This limit how to extend OpenSearch to support some extension function. For example, user may prefer to run ML tasks on some dedicated node which doesn't serve as any built-in node roles. So the ML tasks won't impact OpenSearch core function. This PR removed the limitation and user can specify any node role and OpenSearch will start node correctly with that unknown role. This opens the door for plugin developer to run specific tasks on dedicated nodes. Issue: opensearch-project#2877 Signed-off-by: Yaliang Wu <ylwu@amazon.com>
ylwu-amzn · May 26, 2022 · 433a196 · 433a196
1 parent 296fa09
commit 433a196
Show file tree

Hide file tree

Showing 6 changed files with 117 additions and 18 deletions.
diff --git a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java
@@ -567,10 +567,10 @@ private static Map<String, DiscoveryNodeRole> rolesToMap(final Stream<DiscoveryN
     private static Map<String, DiscoveryNodeRole> roleMap = rolesToMap(DiscoveryNodeRole.BUILT_IN_ROLES.stream());
 
     public static DiscoveryNodeRole getRoleFromRoleName(final String roleName) {
-        if (roleMap.containsKey(roleName) == false) {
-            throw new IllegalArgumentException("unknown role [" + roleName + "]");
+        if (roleMap.containsKey(roleName)) {
+            return roleMap.get(roleName);
         }
-        return roleMap.get(roleName);
+        return new DiscoveryNodeRole.UnknownRole(roleName, roleName, false);
     }
 
     public static Set<DiscoveryNodeRole> getPossibleRoles() {

diff --git a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java
@@ -58,7 +58,6 @@ public abstract class DiscoveryNodeRole implements Comparable<DiscoveryNodeRole>
     private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(DiscoveryNodeRole.class);
     public static final String MASTER_ROLE_DEPRECATION_MESSAGE =
         "Assigning [master] role in setting [node.roles] is deprecated. To promote inclusive language, please use [cluster_manager] role instead.";
-
     private final String roleName;
 
     /**
@@ -299,7 +298,7 @@ public Setting<Boolean> legacySetting() {
 
     /**
      * Represents an unknown role. This can occur if a newer version adds a role that an older version does not know about, or a newer
-     * version removes a role that an older version knows about.
+     * version removes a role that an older version knows about, or some custom role for extension function provided by plugin.
      */
     static class UnknownRole extends DiscoveryNodeRole {
 

diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java
@@ -195,10 +195,12 @@ protected Table getTableWithHeader(final RestRequest request) {
         table.addCell("load_5m", "alias:l;text-align:right;desc:5m load avg");
         table.addCell("load_15m", "alias:l;text-align:right;desc:15m load avg");
         table.addCell("uptime", "default:false;alias:u;text-align:right;desc:node uptime");
+        // TODO: Deprecate "node.role", use "node.roles" which shows full node role names
         table.addCell(
             "node.role",
             "alias:r,role,nodeRole;desc:m:master eligible node, d:data node, i:ingest node, -:coordinating node only"
         );
+        table.addCell("node.roles", "alias:rs,all roles;desc: -:coordinating node only");
         // TODO: Remove the header alias 'master', after removing MASTER_ROLE. It's added for compatibility when using parameter 'h=master'.
         table.addCell("cluster_manager", "alias:cm,m,master;desc:*:current cluster manager");
         table.addCell("name", "alias:n;desc:node name");
@@ -423,12 +425,22 @@ Table buildTable(
             table.addCell(jvmStats == null ? null : jvmStats.getUptime());
 
             final String roles;
+            final String allRoles;
             if (node.getRoles().isEmpty()) {
                 roles = "-";
+                allRoles = "-";
             } else {
-                roles = node.getRoles().stream().map(DiscoveryNodeRole::roleNameAbbreviation).sorted().collect(Collectors.joining());
+                List<DiscoveryNodeRole> knownNodeRoles = node.getRoles()
+                    .stream()
+                    .filter(DiscoveryNodeRole::isKnownRole)
+                    .collect(Collectors.toList());
+                roles = knownNodeRoles.size() > 0
+                    ? knownNodeRoles.stream().map(DiscoveryNodeRole::roleNameAbbreviation).sorted().collect(Collectors.joining())
+                    : "-";
+                allRoles = node.getRoles().stream().map(DiscoveryNodeRole::roleName).sorted().collect(Collectors.joining(","));
             }
             table.addCell(roles);
+            table.addCell(allRoles);
             table.addCell(clusterManagerId == null ? "x" : clusterManagerId.equals(node.getId()) ? "*" : "-");
             table.addCell(node.getName());
 

diff --git a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleGenerator.java b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleGenerator.java
@@ -0,0 +1,16 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.cluster.node;
+
+public class DiscoveryNodeRoleGenerator {
+
+    public static DiscoveryNodeRole createUnknownRole(String roleName) {
+        return new DiscoveryNodeRole.UnknownRole(roleName, roleName, false);
+    }
+}
diff --git a/server/src/test/java/org/opensearch/node/NodeRoleSettingsTests.java b/server/src/test/java/org/opensearch/node/NodeRoleSettingsTests.java
@@ -15,6 +15,7 @@
 
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.List;
 
 import static org.hamcrest.Matchers.containsString;
 
@@ -54,4 +55,23 @@ public void testMasterRoleDeprecationMessage() {
         assertEquals(Collections.singletonList(DiscoveryNodeRole.MASTER_ROLE), NodeRoleSettings.NODE_ROLES_SETTING.get(roleSettings));
         assertWarnings(DiscoveryNodeRole.MASTER_ROLE_DEPRECATION_MESSAGE);
     }
+
+    public void testUnknownNodeRoleAndBuiltInRoleCanCoexist() {
+        String testRole = "test_role";
+        Settings roleSettings = Settings.builder().put(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), "data, " + testRole).build();
+        List<DiscoveryNodeRole> nodeRoles = NodeRoleSettings.NODE_ROLES_SETTING.get(roleSettings);
+        assertEquals(2, nodeRoles.size());
+        assertEquals(DiscoveryNodeRole.DATA_ROLE, nodeRoles.get(0));
+        assertEquals(testRole, nodeRoles.get(1).roleName());
+        assertEquals(testRole, nodeRoles.get(1).roleNameAbbreviation());
+    }
+
+    public void testUnknownNodeRoleOnly() {
+        String testRole = "test_role";
+        Settings roleSettings = Settings.builder().put(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), testRole).build();
+        List<DiscoveryNodeRole> nodeRoles = NodeRoleSettings.NODE_ROLES_SETTING.get(roleSettings);
+        assertEquals(1, nodeRoles.size());
+        assertEquals(testRole, nodeRoles.get(0).roleName());
+        assertEquals(testRole, nodeRoles.get(0).roleNameAbbreviation());
+    }
 }
diff --git a/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java b/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java
@@ -40,14 +40,22 @@
 import org.opensearch.cluster.ClusterName;
 import org.opensearch.cluster.ClusterState;
 import org.opensearch.cluster.node.DiscoveryNode;
+import org.opensearch.cluster.node.DiscoveryNodeRole;
+import org.opensearch.cluster.node.DiscoveryNodeRoleGenerator;
 import org.opensearch.cluster.node.DiscoveryNodes;
+import org.opensearch.common.Table;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.test.OpenSearchTestCase;
 import org.opensearch.test.rest.FakeRestRequest;
 import org.opensearch.threadpool.TestThreadPool;
 import org.junit.Before;
 
 import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Consumer;
 
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.emptySet;
@@ -64,18 +72,15 @@ public void setUpAction() {
     }
 
     public void testBuildTableDoesNotThrowGivenNullNodeInfoAndStats() {
-        ClusterName clusterName = new ClusterName("cluster-1");
-        DiscoveryNodes.Builder builder = DiscoveryNodes.builder();
-        builder.add(new DiscoveryNode("node-1", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT));
-        DiscoveryNodes discoveryNodes = builder.build();
-        ClusterState clusterState = mock(ClusterState.class);
-        when(clusterState.nodes()).thenReturn(discoveryNodes);
-
-        ClusterStateResponse clusterStateResponse = new ClusterStateResponse(clusterName, clusterState, false);
-        NodesInfoResponse nodesInfoResponse = new NodesInfoResponse(clusterName, Collections.emptyList(), Collections.emptyList());
-        NodesStatsResponse nodesStatsResponse = new NodesStatsResponse(clusterName, Collections.emptyList(), Collections.emptyList());
-
-        action.buildTable(false, new FakeRestRequest(), clusterStateResponse, nodesInfoResponse, nodesStatsResponse);
+        testBuildTableWithRoles(emptySet(), (table) -> {
+            Map<String, List<Table.Cell>> nodeInfoMap = table.getAsMap();
+            List<Table.Cell> cells = nodeInfoMap.get("node.role");
+            assertEquals(1, cells.size());
+            assertEquals("-", cells.get(0).value);
+            cells = nodeInfoMap.get("node.roles");
+            assertEquals(1, cells.size());
+            assertEquals("-", cells.get(0).value);
+        });
     }
 
     public void testCatNodesWithLocalDeprecationWarning() {
@@ -89,4 +94,51 @@ public void testCatNodesWithLocalDeprecationWarning() {
 
         terminate(threadPool);
     }
+
+    public void testBuildTableWithUnknownRoleOnly() {
+        Set<DiscoveryNodeRole> roles = new HashSet<>();
+        String roleName = "test_role";
+        DiscoveryNodeRole testRole = DiscoveryNodeRoleGenerator.createUnknownRole(roleName);
+        roles.add(testRole);
+
+        testBuildTableWithRoles(roles, (table) -> {
+            Map<String, List<Table.Cell>> nodeInfoMap = table.getAsMap();
+            List<Table.Cell> cells = nodeInfoMap.get("node.roles");
+            assertEquals(1, cells.size());
+            assertEquals(roleName, cells.get(0).value);
+        });
+    }
+
+    public void testBuildTableWithBothBuiltInAndUnknownRoles() {
+        Set<DiscoveryNodeRole> roles = new HashSet<>();
+        roles.add(DiscoveryNodeRole.DATA_ROLE);
+        String roleName = "test_role";
+        DiscoveryNodeRole testRole = DiscoveryNodeRoleGenerator.createUnknownRole(roleName);
+        roles.add(testRole);
+
+        testBuildTableWithRoles(roles, (table) -> {
+            Map<String, List<Table.Cell>> nodeInfoMap = table.getAsMap();
+            List<Table.Cell> cells = nodeInfoMap.get("node.roles");
+            assertEquals(1, cells.size());
+            assertEquals("data,test_role", cells.get(0).value);
+        });
+    }
+
+    private void testBuildTableWithRoles(Set<DiscoveryNodeRole> roles, Consumer<Table> verificationFunction) {
+        ClusterName clusterName = new ClusterName("cluster-1");
+        DiscoveryNodes.Builder builder = DiscoveryNodes.builder();
+
+        builder.add(new DiscoveryNode("node-1", buildNewFakeTransportAddress(), emptyMap(), roles, Version.CURRENT));
+        DiscoveryNodes discoveryNodes = builder.build();
+        ClusterState clusterState = mock(ClusterState.class);
+        when(clusterState.nodes()).thenReturn(discoveryNodes);
+
+        ClusterStateResponse clusterStateResponse = new ClusterStateResponse(clusterName, clusterState, false);
+        NodesInfoResponse nodesInfoResponse = new NodesInfoResponse(clusterName, Collections.emptyList(), Collections.emptyList());
+        NodesStatsResponse nodesStatsResponse = new NodesStatsResponse(clusterName, Collections.emptyList(), Collections.emptyList());
+
+        Table table = action.buildTable(false, new FakeRestRequest(), clusterStateResponse, nodesInfoResponse, nodesStatsResponse);
+
+        verificationFunction.accept(table);
+    }
 }