Skip to content

Commit

Permalink
Support dynamic node role (#3436)
Browse files Browse the repository at this point in the history
* 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: #2877

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* fix cat nodes rest API spec

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* fix mixed cluster IT failure

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* add DynamicRole

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* change generator method name

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* fix failed docker test

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* transform role name to lower case to avoid confusion

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* transform the node role abbreviation to lower case

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* fix checkstyle

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

* add test for case-insensitive role name change

Signed-off-by: Yaliang Wu <ylwu@amazon.com>
  • Loading branch information
ylwu-amzn authored Jun 14, 2022
1 parent c764d63 commit e9c5ce3
Show file tree
Hide file tree
Showing 10 changed files with 210 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,17 @@

- 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:
v: true

- 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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -32,17 +32,17 @@

- 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:
v: true

- 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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -328,7 +329,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 ["
Expand Down Expand Up @@ -567,10 +572,12 @@ 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 + "]");
// 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 roleMap.get(roleName);
return new DiscoveryNodeRole.DynamicRole(lowerCasedRoleName, lowerCasedRoleName, false);
}

public static Set<DiscoveryNodeRole> getPossibleRoles() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -95,13 +94,19 @@ 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}.
*/
public final boolean isKnownRole() {
return isKnownRole;
}

public final boolean isDynamicRole() {
return isDynamicRole;
}

public boolean isEnabledByDefault(final Settings settings) {
return legacySetting() != null && legacySetting().get(settings);
}
Expand All @@ -111,18 +116,21 @@ 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.roleName = Objects.requireNonNull(roleName);
this.roleNameAbbreviation = Objects.requireNonNull(roleNameAbbreviation);
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).toLowerCase(Locale.ROOT);
this.canContainData = canContainData;
}

Expand Down Expand Up @@ -153,12 +161,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
Expand All @@ -178,6 +187,7 @@ public final String toString() {
+ ", canContainData="
+ canContainData
+ (isKnownRole ? "" : ", isKnownRole=false")
+ (isDynamicRole ? "" : ", isDynamicRole=false")
+ '}';
}

Expand Down Expand Up @@ -311,7 +321,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
Expand All @@ -323,6 +333,32 @@ public Setting<Boolean> 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
*/
DynamicRole(final String roleName, final String roleNameAbbreviation, final boolean canContainData) {
super(false, true, roleName, roleNameAbbreviation, canContainData);
}

@Override
public Setting<Boolean> 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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());

Expand Down
Original file line number Diff line number Diff line change
@@ -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 createDynamicRole(String roleName) {
return new DiscoveryNodeRole.DynamicRole(roleName, roleName, false);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -117,7 +118,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(),
Expand All @@ -126,6 +127,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());
}
}

Expand All @@ -138,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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import java.util.Arrays;
import java.util.Collections;
import java.util.List;

import static org.hamcrest.Matchers.containsString;

Expand Down Expand Up @@ -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());
}
}
Loading

0 comments on commit e9c5ce3

Please sign in to comment.