Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support dynamic node role #3436

Merged
merged 10 commits into from
Jun 14, 2022
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 @@ -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 ["
Expand Down Expand Up @@ -567,10 +571,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)) {
Copy link
Collaborator

@reta reta Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While experimenting, run into interesting issue: the role name are case-sensitive, for example:

node.roles: [ master, Master]

Are 2 different roles. In general, it was fine, OpenSearch would refuse to start because Master was not recognized as a valid role. You may already see where I am going: with the dynamic roles, such set of roles becomes totally valid and confusing:

curl http://localhost:9200/_cat/nodes
127.0.0.1 14 84 7 2.24 2.35 3.06 m Master,master * my-ThinkPad-P15-Gen-1

I think we should be a bit more strict now with the way roles are treated and checks should be case-insensitive: in the example above master and Master should be reduced to same role master. @ylwu-amzn wdyt?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ylwu-amzn can I have you look over this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole case-sensitive thing can be dealt with as a separate issue IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks for reminding. Busy with something else and missed this comment.

@reta that's a great point! I will make the code change to make role name case-insensitive.

return roleMap.get(roleName);
}
return roleMap.get(roleName);
return new DiscoveryNodeRole.DynamicRole(roleName, roleName, 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,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;
Expand Down Expand Up @@ -153,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
Expand All @@ -178,6 +186,7 @@ public final String toString() {
+ ", canContainData="
+ canContainData
+ (isKnownRole ? "" : ", isKnownRole=false")
+ (isDynamicRole ? "" : ", isDynamicRole=false")
+ '}';
}

Expand Down Expand Up @@ -311,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
Expand All @@ -323,6 +332,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 @@ -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(),
Expand All @@ -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());
}
}

Expand Down
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