From 433a196cb80fc347dd3640212e65b980074bc72b Mon Sep 17 00:00:00 2001 From: Yaliang Wu Date: Tue, 24 May 2022 23:16:05 +0000 Subject: [PATCH 01/10] 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: https://github.com/opensearch-project/OpenSearch/issues/2877 Signed-off-by: Yaliang Wu --- .../cluster/node/DiscoveryNode.java | 6 +- .../cluster/node/DiscoveryNodeRole.java | 3 +- .../rest/action/cat/RestNodesAction.java | 14 +++- .../node/DiscoveryNodeRoleGenerator.java | 16 ++++ .../node/NodeRoleSettingsTests.java | 20 +++++ .../rest/action/cat/RestNodesActionTests.java | 76 ++++++++++++++++--- 6 files changed, 117 insertions(+), 18 deletions(-) create mode 100644 server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleGenerator.java diff --git a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java index 38e4cb6d8791a..60581e578744c 100644 --- 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 rolesToMap(final Stream 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 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 index 26ace4b9d80c1..2ad5edb79279b 100644 --- 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 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 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 index 661a53e3d37b8..aaa0413dc4c5f 100644 --- 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 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 new file mode 100644 index 0000000000000..b3646e3b79a7b --- /dev/null +++ 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 index c875fec1979d1..3248b97b8b71f 100644 --- 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 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 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 index 593ad2907797e..dfeeb6d6b5452 100644 --- 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,7 +40,10 @@ 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; @@ -48,6 +51,11 @@ 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> nodeInfoMap = table.getAsMap(); + List 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 roles = new HashSet<>(); + String roleName = "test_role"; + DiscoveryNodeRole testRole = DiscoveryNodeRoleGenerator.createUnknownRole(roleName); + roles.add(testRole); + + testBuildTableWithRoles(roles, (table) -> { + Map> nodeInfoMap = table.getAsMap(); + List cells = nodeInfoMap.get("node.roles"); + assertEquals(1, cells.size()); + assertEquals(roleName, cells.get(0).value); + }); + } + + public void testBuildTableWithBothBuiltInAndUnknownRoles() { + Set roles = new HashSet<>(); + roles.add(DiscoveryNodeRole.DATA_ROLE); + String roleName = "test_role"; + DiscoveryNodeRole testRole = DiscoveryNodeRoleGenerator.createUnknownRole(roleName); + roles.add(testRole); + + testBuildTableWithRoles(roles, (table) -> { + Map> nodeInfoMap = table.getAsMap(); + List cells = nodeInfoMap.get("node.roles"); + assertEquals(1, cells.size()); + assertEquals("data,test_role", cells.get(0).value); + }); + } + + private void testBuildTableWithRoles(Set roles, Consumer 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); + } } From 4cb8dac9361b7753b4a6508a768a7eff11bf61bb Mon Sep 17 00:00:00 2001 From: Yaliang Wu Date: Wed, 25 May 2022 17:15:52 +0000 Subject: [PATCH 02/10] fix cat nodes rest API spec Signed-off-by: Yaliang Wu --- .../rest-api-spec/test/cat.nodes/10_basic.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.nodes/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.nodes/10_basic.yml index f04c674d420ee..1b5b96d62b726 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.nodes/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.nodes/10_basic.yml @@ -10,9 +10,9 @@ v: true node_selector: # Only send request to nodes in <2.0 versions, especially during ':qa:mixed-cluster:v1.x.x#mixedClusterTest'. - # Because YAML REST test takes the minimum OpenSearch version in the cluster to apply the filter in 'skip' section, + # Because YAML REST test takes the minimum OpenSearch version in the cluster to apply the filter in 'skip' section, # see OpenSearchClientYamlSuiteTestCase#initAndResetContext() for detail. - # During 'mixedClusterTest', the cluster can be mixed with nodes in 1.x and 2.x versions, + # During 'mixedClusterTest', the cluster can be mixed with nodes in 1.x and 2.x versions, # so node_selector is required, and only filtering version in 'skip' is not enough. version: "1.0.0 - 1.4.99" @@ -32,8 +32,8 @@ - match: $body: | - / #ip heap.percent ram.percent cpu load_1m load_5m load_15m node.role cluster_manager name - ^ ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)?\s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ + / #ip heap.percent ram.percent cpu load_1m load_5m load_15m node.role node.roles cluster_manager name + ^ ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)?\s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) \s+ (-|\w+(,\w+)*+) \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ - do: cat.nodes: @@ -41,8 +41,8 @@ - match: $body: | - /^ ip \s+ heap\.percent \s+ ram\.percent \s+ cpu \s+ load_1m \s+ load_5m \s+ load_15m \s+ node\.role \s+ cluster_manager \s+ name \n - ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ + /^ ip \s+ heap\.percent \s+ ram\.percent \s+ cpu \s+ load_1m \s+ load_5m \s+ load_15m \s+ node\.role \s+ node\.roles \s+ cluster_manager \s+ name \n + ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) \s+ (-|\w+(,\w+)*+ ) \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ - do: cat.nodes: From 2f2df1678ae3bb8f1d423a4188fd568e46b87676 Mon Sep 17 00:00:00 2001 From: Yaliang Wu Date: Thu, 26 May 2022 05:25:18 +0000 Subject: [PATCH 03/10] fix mixed cluster IT failure Signed-off-by: Yaliang Wu --- .../resources/rest-api-spec/test/cat.nodes/10_basic.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.nodes/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.nodes/10_basic.yml index 1b5b96d62b726..525d705de88f1 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.nodes/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.nodes/10_basic.yml @@ -33,7 +33,7 @@ - match: $body: | / #ip heap.percent ram.percent cpu load_1m load_5m load_15m node.role node.roles cluster_manager name - ^ ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)?\s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) \s+ (-|\w+(,\w+)*+) \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ + ^ ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)?\s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) (\s+ (-|\w+(,\w+)*+))? \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ - do: cat.nodes: @@ -41,8 +41,8 @@ - match: $body: | - /^ ip \s+ heap\.percent \s+ ram\.percent \s+ cpu \s+ load_1m \s+ load_5m \s+ load_15m \s+ node\.role \s+ node\.roles \s+ cluster_manager \s+ name \n - ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) \s+ (-|\w+(,\w+)*+ ) \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ + /^ ip \s+ heap\.percent \s+ ram\.percent \s+ cpu \s+ load_1m \s+ load_5m \s+ load_15m \s+ node\.role (\s+ node\.roles)? \s+ cluster_manager \s+ name \n + ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) (\s+ (-|\w+(,\w+)*+ ))? \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ - do: cat.nodes: From 42455431d2dafdd24d0e0feb0769dd38ea8e16d3 Mon Sep 17 00:00:00 2001 From: Yaliang Wu Date: Wed, 1 Jun 2022 01:57:21 +0000 Subject: [PATCH 04/10] add DynamicRole Signed-off-by: Yaliang Wu --- .../cluster/node/DiscoveryNode.java | 8 +++- .../cluster/node/DiscoveryNodeRole.java | 46 +++++++++++++++++-- .../cluster/node/DiscoveryNodeRoleTests.java | 11 ++++- 3 files changed, 57 insertions(+), 8 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 index 60581e578744c..9fe96caf268a5 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java @@ -328,7 +328,11 @@ public DiscoveryNode(StreamInput in) throws IOException { } final DiscoveryNodeRole role = roleMap.get(roleName); if (role == null) { - roles.add(new DiscoveryNodeRole.UnknownRole(roleName, roleNameAbbreviation, canContainData)); + if (in.getVersion().onOrAfter(Version.V_2_1_0)) { + roles.add(new DiscoveryNodeRole.DynamicRole(roleName, roleNameAbbreviation, canContainData)); + } else { + roles.add(new DiscoveryNodeRole.UnknownRole(roleName, roleNameAbbreviation, canContainData)); + } } else { assert roleName.equals(role.roleName()) : "role name [" + roleName + "] does not match role [" + role.roleName() + "]"; assert roleNameAbbreviation.equals(role.roleNameAbbreviation()) : "role name abbreviation [" @@ -570,7 +574,7 @@ public static DiscoveryNodeRole getRoleFromRoleName(final String roleName) { if (roleMap.containsKey(roleName)) { return roleMap.get(roleName); } - return new DiscoveryNodeRole.UnknownRole(roleName, roleName, false); + return new DiscoveryNodeRole.DynamicRole(roleName, roleName, false); } public static Set 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 index 2ad5edb79279b..1c460591e5066 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java @@ -94,6 +94,8 @@ public final boolean canContainData() { private final boolean isKnownRole; + private final boolean isDynamicRole; + /** * Whether this role is known by this node, or is an {@link DiscoveryNodeRole.UnknownRole}. */ @@ -101,6 +103,10 @@ public final boolean isKnownRole() { return isKnownRole; } + public final boolean isDynamicRole() { + return isDynamicRole; + } + public boolean isEnabledByDefault(final Settings settings) { return legacySetting() != null && legacySetting().get(settings); } @@ -110,16 +116,18 @@ protected DiscoveryNodeRole(final String roleName, final String roleNameAbbrevia } protected DiscoveryNodeRole(final String roleName, final String roleNameAbbreviation, final boolean canContainData) { - this(true, roleName, roleNameAbbreviation, canContainData); + this(true, false, roleName, roleNameAbbreviation, canContainData); } private DiscoveryNodeRole( final boolean isKnownRole, + final boolean isDynamicRole, final String roleName, final String roleNameAbbreviation, final boolean canContainData ) { this.isKnownRole = isKnownRole; + this.isDynamicRole = isDynamicRole; this.roleName = Objects.requireNonNull(roleName); this.roleNameAbbreviation = Objects.requireNonNull(roleNameAbbreviation); this.canContainData = canContainData; @@ -152,12 +160,13 @@ public final boolean equals(Object o) { return roleName.equals(that.roleName) && roleNameAbbreviation.equals(that.roleNameAbbreviation) && canContainData == that.canContainData - && isKnownRole == that.isKnownRole; + && isKnownRole == that.isKnownRole + && isDynamicRole == that.isDynamicRole; } @Override public final int hashCode() { - return Objects.hash(isKnownRole, roleName(), roleNameAbbreviation(), canContainData()); + return Objects.hash(isKnownRole, isDynamicRole, roleName(), roleNameAbbreviation(), canContainData()); } @Override @@ -177,6 +186,7 @@ public final String toString() { + ", canContainData=" + canContainData + (isKnownRole ? "" : ", isKnownRole=false") + + (isDynamicRole ? "" : ", isDynamicRole=false") + '}'; } @@ -298,7 +308,7 @@ public Setting 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, or some custom role for extension function provided by plugin. + * version removes a role that an older version knows about. */ static class UnknownRole extends DiscoveryNodeRole { @@ -310,7 +320,7 @@ static class UnknownRole extends DiscoveryNodeRole { * @param canContainData whether or not nodes with the role can contain data */ UnknownRole(final String roleName, final String roleNameAbbreviation, final boolean canContainData) { - super(false, roleName, roleNameAbbreviation, canContainData); + super(false, false, roleName, roleNameAbbreviation, canContainData); } @Override @@ -322,6 +332,32 @@ public Setting legacySetting() { } + /** + * Represents a dynamic role. This can occur if a custom role that not in {@link DiscoveryNodeRole#BUILT_IN_ROLES} added for a node. + * Some plugin can support extension function with dynamic roles. For example, ML plugin may run machine learning tasks on nodes + * with "ml" dynamic role. + */ + static class DynamicRole extends DiscoveryNodeRole { + + /** + * Construct a dynamic role with the specified role name and role name abbreviation. + * + * @param roleName the role name + * @param roleNameAbbreviation the role name abbreviation + * @param canContainData whether or not nodes with the role can contain data + */ + public DynamicRole(final String roleName, final String roleNameAbbreviation, final boolean canContainData) { + super(false, true, roleName, roleNameAbbreviation, canContainData); + } + + @Override + public Setting legacySetting() { + // return null as dynamic role has no legacy setting + return null; + } + + } + /** * Check if the role is {@link #CLUSTER_MANAGER_ROLE} or {@link #MASTER_ROLE}. * @deprecated As of 2.0, because promoting inclusive language. MASTER_ROLE is deprecated. diff --git a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleTests.java b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleTests.java index d1acec2832b7c..5a4a68fddfdb8 100644 --- a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleTests.java +++ b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleTests.java @@ -117,7 +117,7 @@ public void testDiscoveryNodeRoleEqualsHashCode() { } - public void testUnknownRoleIsDistinctFromKnownRoles() { + public void testUnknownRoleIsDistinctFromKnownOrDynamicRoles() { for (DiscoveryNodeRole buildInRole : DiscoveryNodeRole.BUILT_IN_ROLES) { final DiscoveryNodeRole.UnknownRole unknownDataRole = new DiscoveryNodeRole.UnknownRole( buildInRole.roleName(), @@ -126,6 +126,15 @@ public void testUnknownRoleIsDistinctFromKnownRoles() { ); assertNotEquals(buildInRole, unknownDataRole); assertNotEquals(buildInRole.toString(), unknownDataRole.toString()); + final DiscoveryNodeRole.DynamicRole dynamicRole = new DiscoveryNodeRole.DynamicRole( + buildInRole.roleName(), + buildInRole.roleNameAbbreviation(), + buildInRole.canContainData() + ); + assertNotEquals(buildInRole, dynamicRole); + assertNotEquals(buildInRole.toString(), dynamicRole.toString()); + assertNotEquals(unknownDataRole, dynamicRole); + assertNotEquals(unknownDataRole.toString(), dynamicRole.toString()); } } From a2fea20a3f7e86beedd0d38e74d2d940c195ae14 Mon Sep 17 00:00:00 2001 From: Yaliang Wu Date: Wed, 1 Jun 2022 02:36:24 +0000 Subject: [PATCH 05/10] change generator method name Signed-off-by: Yaliang Wu --- .../org/opensearch/cluster/node/DiscoveryNodeRole.java | 2 +- .../cluster/node/DiscoveryNodeRoleGenerator.java | 4 ++-- .../opensearch/rest/action/cat/RestNodesActionTests.java | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java index 1c460591e5066..03cadf20170b1 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java @@ -346,7 +346,7 @@ static class DynamicRole extends DiscoveryNodeRole { * @param roleNameAbbreviation the role name abbreviation * @param canContainData whether or not nodes with the role can contain data */ - public DynamicRole(final String roleName, final String roleNameAbbreviation, final boolean canContainData) { + DynamicRole(final String roleName, final String roleNameAbbreviation, final boolean canContainData) { super(false, true, roleName, roleNameAbbreviation, canContainData); } diff --git a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleGenerator.java b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleGenerator.java index b3646e3b79a7b..c1aa9390fec94 100644 --- a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleGenerator.java +++ b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleGenerator.java @@ -10,7 +10,7 @@ public class DiscoveryNodeRoleGenerator { - public static DiscoveryNodeRole createUnknownRole(String roleName) { - return new DiscoveryNodeRole.UnknownRole(roleName, roleName, false); + public static DiscoveryNodeRole createDynamicRole(String roleName) { + return new DiscoveryNodeRole.DynamicRole(roleName, roleName, false); } } 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 index dfeeb6d6b5452..6485ddd3bbc94 100644 --- a/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java +++ b/server/src/test/java/org/opensearch/rest/action/cat/RestNodesActionTests.java @@ -95,10 +95,10 @@ public void testCatNodesWithLocalDeprecationWarning() { terminate(threadPool); } - public void testBuildTableWithUnknownRoleOnly() { + public void testBuildTableWithDynamicRoleOnly() { Set roles = new HashSet<>(); String roleName = "test_role"; - DiscoveryNodeRole testRole = DiscoveryNodeRoleGenerator.createUnknownRole(roleName); + DiscoveryNodeRole testRole = DiscoveryNodeRoleGenerator.createDynamicRole(roleName); roles.add(testRole); testBuildTableWithRoles(roles, (table) -> { @@ -109,11 +109,11 @@ public void testBuildTableWithUnknownRoleOnly() { }); } - public void testBuildTableWithBothBuiltInAndUnknownRoles() { + public void testBuildTableWithBothBuiltInAndDynamicRoles() { Set roles = new HashSet<>(); roles.add(DiscoveryNodeRole.DATA_ROLE); String roleName = "test_role"; - DiscoveryNodeRole testRole = DiscoveryNodeRoleGenerator.createUnknownRole(roleName); + DiscoveryNodeRole testRole = DiscoveryNodeRoleGenerator.createDynamicRole(roleName); roles.add(testRole); testBuildTableWithRoles(roles, (table) -> { From 836be1f0a45de967a02015c55ff5dda9d44fcabe Mon Sep 17 00:00:00 2001 From: Yaliang Wu Date: Wed, 1 Jun 2022 23:12:25 +0000 Subject: [PATCH 06/10] fix failed docker test Signed-off-by: Yaliang Wu --- .../src/test/resources/rest-api-spec/test/11_nodes.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/distribution/docker/src/test/resources/rest-api-spec/test/11_nodes.yml b/distribution/docker/src/test/resources/rest-api-spec/test/11_nodes.yml index a6b78645087f4..1c10166e96eeb 100644 --- a/distribution/docker/src/test/resources/rest-api-spec/test/11_nodes.yml +++ b/distribution/docker/src/test/resources/rest-api-spec/test/11_nodes.yml @@ -24,8 +24,8 @@ - match: $body: | - / #ip heap.percent ram.percent cpu load_1m load_5m load_15m node.role cluster_manager name - ^ ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)?\s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ + / #ip heap.percent ram.percent cpu load_1m load_5m load_15m node.role node.roles cluster_manager name + ^ ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)?\s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) (\s+ (-|\w+(,\w+)*+))? \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ - do: cat.nodes: @@ -33,8 +33,8 @@ - match: $body: | - /^ ip \s+ heap\.percent \s+ ram\.percent \s+ cpu \s+ load_1m \s+ load_5m \s+ load_15m \s+ node\.role \s+ cluster_manager \s+ name \n - ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ + /^ ip \s+ heap\.percent \s+ ram\.percent \s+ cpu \s+ load_1m \s+ load_5m \s+ load_15m \s+ node\.role \s+ node\.roles \s+ cluster_manager \s+ name \n + ((\d{1,3}\.){3}\d{1,3} \s+ \d+ \s+ \d* \s+ (-)?\d* \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ ((-)?\d*(\.\d+)?)? \s+ (-|[cdhilmrstvw]{1,11}) (\s+ (-|\w+(,\w+)*+ ))? \s+ [-*x] \s+ (\S+\s?)+ \n)+ $/ - do: cat.nodes: From 5101c9a21849af2547e9c4ec421715ae3d7ca48c Mon Sep 17 00:00:00 2001 From: Yaliang Wu Date: Tue, 14 Jun 2022 18:28:19 +0000 Subject: [PATCH 07/10] transform role name to lower case to avoid confusion Signed-off-by: Yaliang Wu --- .../java/org/opensearch/cluster/node/DiscoveryNode.java | 9 ++++++--- .../org/opensearch/cluster/node/DiscoveryNodeRole.java | 3 ++- 2 files changed, 8 insertions(+), 4 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 index 9fe96caf268a5..5e8f796300812 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java @@ -52,6 +52,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.SortedSet; import java.util.TreeSet; @@ -571,10 +572,12 @@ private static Map rolesToMap(final Stream roleMap = rolesToMap(DiscoveryNodeRole.BUILT_IN_ROLES.stream()); public static DiscoveryNodeRole getRoleFromRoleName(final String roleName) { - if (roleMap.containsKey(roleName)) { - return roleMap.get(roleName); + //As we are supporting dynamic role, should make role name case-insensitive to avoid confusion of role name like "Data"/"DATA" + String lowerCasedRoleName = Objects.requireNonNull(roleName).toLowerCase(Locale.ROOT); + if (roleMap.containsKey(lowerCasedRoleName)) { + return roleMap.get(lowerCasedRoleName); } - return new DiscoveryNodeRole.DynamicRole(roleName, roleName, false); + return new DiscoveryNodeRole.DynamicRole(lowerCasedRoleName, lowerCasedRoleName, false); } public static Set 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 index 03cadf20170b1..f5a9170beb4d4 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java @@ -128,7 +128,8 @@ private DiscoveryNodeRole( ) { this.isKnownRole = isKnownRole; this.isDynamicRole = isDynamicRole; - this.roleName = Objects.requireNonNull(roleName); + //As we are supporting dynamic role, should make role name case-insensitive to avoid confusion of role name like "Data"/"DATA" + this.roleName = Objects.requireNonNull(roleName).toLowerCase(Locale.ROOT); this.roleNameAbbreviation = Objects.requireNonNull(roleNameAbbreviation); this.canContainData = canContainData; } From 6e286c153f22e50dcaa6dffde9f869f966791648 Mon Sep 17 00:00:00 2001 From: Yaliang Wu Date: Tue, 14 Jun 2022 18:33:47 +0000 Subject: [PATCH 08/10] transform the node role abbreviation to lower case Signed-off-by: Yaliang Wu --- .../java/org/opensearch/cluster/node/DiscoveryNodeRole.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java index f5a9170beb4d4..21cefb34957aa 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java @@ -130,7 +130,7 @@ private DiscoveryNodeRole( this.isDynamicRole = isDynamicRole; //As we are supporting dynamic role, should make role name case-insensitive to avoid confusion of role name like "Data"/"DATA" this.roleName = Objects.requireNonNull(roleName).toLowerCase(Locale.ROOT); - this.roleNameAbbreviation = Objects.requireNonNull(roleNameAbbreviation); + this.roleNameAbbreviation = Objects.requireNonNull(roleNameAbbreviation).toLowerCase(Locale.ROOT); this.canContainData = canContainData; } From 8e2f2d83d387d01a9fc0f4929b21366ae90cfbc5 Mon Sep 17 00:00:00 2001 From: Yaliang Wu Date: Tue, 14 Jun 2022 18:36:11 +0000 Subject: [PATCH 09/10] fix checkstyle Signed-off-by: Yaliang Wu --- .../main/java/org/opensearch/cluster/node/DiscoveryNode.java | 2 +- .../java/org/opensearch/cluster/node/DiscoveryNodeRole.java | 2 +- 2 files changed, 2 insertions(+), 2 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 index 5e8f796300812..0d55624a35998 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java @@ -572,7 +572,7 @@ private static Map rolesToMap(final Stream roleMap = rolesToMap(DiscoveryNodeRole.BUILT_IN_ROLES.stream()); public static DiscoveryNodeRole getRoleFromRoleName(final String roleName) { - //As we are supporting dynamic role, should make role name case-insensitive to avoid confusion of role name like "Data"/"DATA" + // As we are supporting dynamic role, should make role name case-insensitive to avoid confusion of role name like "Data"/"DATA" String lowerCasedRoleName = Objects.requireNonNull(roleName).toLowerCase(Locale.ROOT); if (roleMap.containsKey(lowerCasedRoleName)) { return roleMap.get(lowerCasedRoleName); diff --git a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java index 21cefb34957aa..5685667c05b1a 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java @@ -128,7 +128,7 @@ private DiscoveryNodeRole( ) { this.isKnownRole = isKnownRole; this.isDynamicRole = isDynamicRole; - //As we are supporting dynamic role, should make role name case-insensitive to avoid confusion of role name like "Data"/"DATA" + // As we are supporting dynamic role, should make role name case-insensitive to avoid confusion of role name like "Data"/"DATA" this.roleName = Objects.requireNonNull(roleName).toLowerCase(Locale.ROOT); this.roleNameAbbreviation = Objects.requireNonNull(roleNameAbbreviation).toLowerCase(Locale.ROOT); this.canContainData = canContainData; From 81d0d601d8f98988e2d423558dd8d0eabb7084bc Mon Sep 17 00:00:00 2001 From: Yaliang Wu Date: Tue, 14 Jun 2022 19:16:21 +0000 Subject: [PATCH 10/10] add test for case-insensitive role name change Signed-off-by: Yaliang Wu --- .../cluster/node/DiscoveryNodeRoleTests.java | 12 ++++++++++++ .../opensearch/cluster/node/DiscoveryNodeTests.java | 11 +++++++++++ 2 files changed, 23 insertions(+) diff --git a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleTests.java b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleTests.java index 5a4a68fddfdb8..f906a0f937d28 100644 --- a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleTests.java +++ b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleTests.java @@ -38,6 +38,7 @@ import java.util.Arrays; import java.util.HashSet; +import java.util.Locale; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasToString; @@ -147,4 +148,15 @@ public void testIsClusterManager() { assertTrue(DiscoveryNodeRole.MASTER_ROLE.isClusterManager()); assertFalse(randomFrom(DiscoveryNodeRole.DATA_ROLE.isClusterManager(), DiscoveryNodeRole.INGEST_ROLE.isClusterManager())); } + + public void testRoleNameIsCaseInsensitive() { + String roleName = "TestRole"; + String roleNameAbbreviation = "T"; + DiscoveryNodeRole unknownRole = new DiscoveryNodeRole.UnknownRole(roleName, roleNameAbbreviation, false); + assertEquals(roleName.toLowerCase(Locale.ROOT), unknownRole.roleName()); + assertEquals(roleNameAbbreviation.toLowerCase(Locale.ROOT), unknownRole.roleNameAbbreviation()); + DiscoveryNodeRole dynamicRole = new DiscoveryNodeRole.DynamicRole(roleName, roleNameAbbreviation, false); + assertEquals(roleName.toLowerCase(Locale.ROOT), dynamicRole.roleName()); + assertEquals(roleNameAbbreviation.toLowerCase(Locale.ROOT), dynamicRole.roleNameAbbreviation()); + } } diff --git a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeTests.java b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeTests.java index 1b7f698ae1f5c..abd1cae1ed97d 100644 --- a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeTests.java +++ b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeTests.java @@ -44,6 +44,7 @@ import java.net.InetAddress; import java.util.Collections; import java.util.HashSet; +import java.util.Locale; import java.util.Set; import java.util.stream.Collectors; @@ -193,4 +194,14 @@ private void runTestDiscoveryNodeIsRemoteClusterClient(final Settings settings, } } + public void testGetRoleFromRoleNameIsCaseInsensitive() { + String dataRoleName = "DATA"; + DiscoveryNodeRole dataNodeRole = DiscoveryNode.getRoleFromRoleName(dataRoleName); + assertEquals(DiscoveryNodeRole.DATA_ROLE, dataNodeRole); + + String dynamicRoleName = "TestRole"; + DiscoveryNodeRole dynamicNodeRole = DiscoveryNode.getRoleFromRoleName(dynamicRoleName); + assertEquals(dynamicRoleName.toLowerCase(Locale.ROOT), dynamicNodeRole.roleName()); + assertEquals(dynamicRoleName.toLowerCase(Locale.ROOT), dynamicNodeRole.roleNameAbbreviation()); + } }