Skip to content

Commit

Permalink
Add a new node role 'cluster_manager' as the alternative for 'master'…
Browse files Browse the repository at this point in the history
… role and deprecate 'master' role (#2424)

Add a new node role `cluster_manager`, as the replacement for `master` role, that used in setting `node.roles: [ master ]`. 
They have got the same functionality, but can NOT be assigned to `node.roles` together.
* Add `CLUSTER_MANAGER_ROLE` in `DiscoveryNodeRole` class, and deprecate `MASTER_ROLE`
* Remove `MASTER_ROLE` from the `DiscoveryNodeRole.BUILT_IN_ROLES`, and temporarily load the role by the existing method `DiscoveryNode.setAdditionalRoles`
* Add a method `validateRole()` in `DiscoveryNodeRole` class, it's used to validate a specific role is compatible with the other roles to be assigned to a node together.
* Add deprecation message when assigning `master` role in setting `node.roles`
* Replace most `MASTER_ROLE` with `CLUSTER_MANAGER_ROLE` in current unit and integration tests
* Add new unit and integration tests to validate `CLUSTER_MANAGER_ROLE` and `MASTER_ROLE` can be treated as the same.

More explanation:
* New node will have "cluster_manager", "data", and "ingest" roles by default. Which means the default value of setting "node.roles" will be `["cluster_manager","data","ingest"]`, instead of `["master","data","ingest"]`
* "cluster_manager" role will be treated as "master" role in the OpenSearch node of previous versions.
* "cluster_manager” role and "master" role have got the same abbreviation name: "m"

Signed-off-by: Tianli Feng <ftianli@amazon.com>
  • Loading branch information
Tianli Feng authored Mar 18, 2022
1 parent 641350b commit ea31483
Show file tree
Hide file tree
Showing 52 changed files with 356 additions and 105 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public static DiscoveryNode newNode(String nodeId, Map<String, String> attribute
nodeId,
new TransportAddress(TransportAddress.META_ADDRESS, portGenerator.incrementAndGet()),
attributes,
Sets.newHashSet(DiscoveryNodeRole.MASTER_ROLE, DiscoveryNodeRole.DATA_ROLE),
Sets.newHashSet(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE, DiscoveryNodeRole.DATA_ROLE),
Version.CURRENT
);
}
Expand Down
8 changes: 4 additions & 4 deletions client/rest/src/main/java/org/opensearch/client/Node.java
Original file line number Diff line number Diff line change
Expand Up @@ -210,21 +210,21 @@ public Roles(final Set<String> roles) {
}

/**
* Teturns whether or not the node <strong>could</strong> be elected master.
* Returns whether or not the node <strong>could</strong> be elected master.
*/
public boolean isMasterEligible() {
return roles.contains("master");
return roles.contains("master") || roles.contains("cluster_manager");
}

/**
* Teturns whether or not the node stores data.
* Returns whether or not the node stores data.
*/
public boolean isData() {
return roles.contains("data");
}

/**
* Teturns whether or not the node runs ingest pipelines.
* Returns whether or not the node runs ingest pipelines.
*/
public boolean isIngest() {
return roles.contains("ingest");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,22 @@
cluster.stats: {}

- is_true: nodes.packaging_types

---
"get cluster stats nodes count with both master and cluster_manager":
- skip:
version: " - 1.4.99"
reason: "node role cluster_manager is added in 2.0.0"

- do:
cluster.stats: {}

- set:
nodes.count.cluster_manager: cluster_manager_count

- gte: { nodes.count.total: 1}
- match: { nodes.count.cluster_manager: $cluster_manager_count }
- match: { nodes.count.master: $cluster_manager_count }
- gte: { nodes.count.data: 1}
- gte: { nodes.count.ingest: 0}
- gte: { nodes.count.coordinating_only: 0}
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,24 @@ setup:
- is_true: cluster_name

---
"node_info role test":
"node_info role test - before 2.0.0":
- skip:
version: " - 7.7.99"
version: " - 7.7.99 , 2.0.0 - "
reason: "node roles were not sorted before 7.8.0"
features: node_selector

- do:
nodes.info: {}
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,
# see OpenSearchClientYamlSuiteTestCase#initAndResetContext() for detail.
# 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"

- set:
# Note: It will only stash the first node_id in the api response.
nodes._arbitrary_key_: node_id

- is_true: nodes.$node_id.roles
Expand All @@ -29,3 +39,21 @@ setup:
- match: { nodes.$node_id.roles.2: "master" }
- match: { nodes.$node_id.roles.3: "remote_cluster_client" }

---
"node_info role test":
- skip:
version: " - 1.4.99"
reason: "node role cluster_manager is added in 2.0.0"

- do:
nodes.info: {}

- set:
nodes._arbitrary_key_: node_id

- is_true: nodes.$node_id.roles
# the roles output is sorted
- match: { nodes.$node_id.roles.0: "cluster_manager" }
- match: { nodes.$node_id.roles.1: "data" }
- match: { nodes.$node_id.roles.2: "ingest" }
- match: { nodes.$node_id.roles.3: "remote_cluster_client" }
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ public void testNodeCounts() {
Map<String, Integer> expectedCounts = new HashMap<>();
expectedCounts.put(DiscoveryNodeRole.DATA_ROLE.roleName(), 1);
expectedCounts.put(DiscoveryNodeRole.MASTER_ROLE.roleName(), 1);
expectedCounts.put(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(), 1);
expectedCounts.put(DiscoveryNodeRole.INGEST_ROLE.roleName(), 1);
expectedCounts.put(DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName(), 1);
expectedCounts.put(ClusterStatsNodes.Counts.COORDINATING_ONLY, 0);
Expand All @@ -106,7 +107,7 @@ public void testNodeCounts() {
roles.add(DiscoveryNodeRole.INGEST_ROLE);
}
if (isMasterNode) {
roles.add(DiscoveryNodeRole.MASTER_ROLE);
roles.add(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE);
}
if (isRemoteClusterClientNode) {
roles.add(DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE);
Expand All @@ -129,6 +130,7 @@ public void testNodeCounts() {
}
if (isMasterNode) {
incrementCountForRole(DiscoveryNodeRole.MASTER_ROLE.roleName(), expectedCounts);
incrementCountForRole(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(), expectedCounts);
}
if (isRemoteClusterClientNode) {
incrementCountForRole(DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName(), expectedCounts);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public Settings onNodeStopped(String nodeName) {
})
);
if (writeDanglingIndices) {
assertThat(ex.getMessage(), startsWith("node does not have the data and master roles but has index metadata"));
assertThat(ex.getMessage(), startsWith("node does not have the data and cluster_manager roles but has index metadata"));
} else {
assertThat(ex.getMessage(), startsWith("node does not have the data role but has shard data"));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,14 @@ private Counts(final List<NodeInfo> nodeInfos) {
roles.merge(COORDINATING_ONLY, 1, Integer::sum);
} else {
for (DiscoveryNodeRole role : nodeInfo.getNode().getRoles()) {
roles.merge(role.roleName(), 1, Integer::sum);
// TODO: Remove the 'if' condition and only keep the statement in 'else' after removing MASTER_ROLE.
// As of 2.0, CLUSTER_MANAGER_ROLE is added, and it should be taken as MASTER_ROLE
if (role.isClusterManager()) {
roles.merge(DiscoveryNodeRole.MASTER_ROLE.roleName(), 1, Integer::sum);
roles.merge(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(), 1, Integer::sum);
} else {
roles.merge(role.roleName(), 1, Integer::sum);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public static boolean hasRole(final Settings settings, final DiscoveryNodeRole r
}

public static boolean isMasterNode(Settings settings) {
return hasRole(settings, DiscoveryNodeRole.MASTER_ROLE);
return hasRole(settings, DiscoveryNodeRole.MASTER_ROLE) || hasRole(settings, DiscoveryNodeRole.CLUSTER_MANAGER_ROLE);
}

/**
Expand Down Expand Up @@ -343,7 +343,7 @@ public DiscoveryNode(StreamInput in) throws IOException {
final LegacyRole legacyRole = in.readEnum(LegacyRole.class);
switch (legacyRole) {
case MASTER:
roles.add(DiscoveryNodeRole.MASTER_ROLE);
roles.add(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE);
break;
case DATA:
roles.add(DiscoveryNodeRole.DATA_ROLE);
Expand Down Expand Up @@ -390,11 +390,11 @@ public void writeTo(StreamOutput out) throws IOException {
.collect(Collectors.toList());
out.writeVInt(rolesToWrite.size());
for (final DiscoveryNodeRole role : rolesToWrite) {
if (role == DiscoveryNodeRole.MASTER_ROLE) {
if (role.isClusterManager()) {
out.writeEnum(LegacyRole.MASTER);
} else if (role == DiscoveryNodeRole.DATA_ROLE) {
} else if (role.equals(DiscoveryNodeRole.DATA_ROLE)) {
out.writeEnum(LegacyRole.DATA);
} else if (role == DiscoveryNodeRole.INGEST_ROLE) {
} else if (role.equals(DiscoveryNodeRole.INGEST_ROLE)) {
out.writeEnum(LegacyRole.INGEST);
}
}
Expand Down Expand Up @@ -456,7 +456,7 @@ public boolean isDataNode() {
* Can this node become master or not.
*/
public boolean isMasterNode() {
return roles.contains(DiscoveryNodeRole.MASTER_ROLE);
return roles.contains(DiscoveryNodeRole.MASTER_ROLE) || roles.contains(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE);
}

/**
Expand Down Expand Up @@ -591,15 +591,19 @@ public static void setAdditionalRoles(final Set<DiscoveryNodeRole> additionalRol
+ "], roles by name abbreviation ["
+ roleNameAbbreviationToPossibleRoles
+ "]";
roleMap = roleNameToPossibleRoles;
// 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);
}

public static Set<String> getPossibleRoleNames() {
return roleMap.keySet();
}

/**
* Enum that holds all the possible roles that that a node can fulfill in a cluster.
* Enum that holds all the possible roles that a node can fulfill in a cluster.
* Each role has its name and a corresponding abbreviation used by cat apis.
*/
private enum LegacyRole {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,16 @@

import org.opensearch.LegacyESVersion;
import org.opensearch.Version;
import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.common.settings.Setting;
import org.opensearch.common.settings.Setting.Property;
import org.opensearch.common.settings.Settings;
import org.opensearch.transport.RemoteClusterService;

import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.Objects;
import java.util.SortedSet;
import java.util.TreeSet;
Expand All @@ -50,6 +53,10 @@
*/
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 @@ -129,6 +136,13 @@ public DiscoveryNodeRole getCompatibilityRole(Version nodeVersion) {
return this;
}

/**
* Validate the role is compatible with the other roles in the list, when assigning the list of roles to a node.
* An {@link IllegalArgumentException} is expected to be thrown, if the role can't coexist with the other roles.
* @param roles a {@link List} of {@link DiscoveryNodeRole} that a node is going to have
*/
public void validateRole(List<DiscoveryNodeRole> roles) {};

@Override
public final boolean equals(Object o) {
if (this == o) return true;
Expand Down Expand Up @@ -193,15 +207,60 @@ public Setting<Boolean> legacySetting() {

/**
* Represents the role for a master-eligible node.
* @deprecated As of 2.0, because promoting inclusive language, replaced by {@link #CLUSTER_MANAGER_ROLE}
*/
@Deprecated
public static final DiscoveryNodeRole MASTER_ROLE = new DiscoveryNodeRole("master", "m") {

@Override
public Setting<Boolean> legacySetting() {
// copy the setting here so we can mark it private in org.opensearch.node.Node
// As of 2.0, set the default value to 'false', so that MASTER_ROLE isn't added as a default value of NODE_ROLES_SETTING
return Setting.boolSetting("node.master", false, Property.Deprecated, Property.NodeScope);
}

@Override
public void validateRole(List<DiscoveryNodeRole> roles) {
deprecationLogger.deprecate("node_role_master", MASTER_ROLE_DEPRECATION_MESSAGE);
}

};

/**
* Represents the role for a cluster-manager-eligible node.
*/
public static final DiscoveryNodeRole CLUSTER_MANAGER_ROLE = new DiscoveryNodeRole("cluster_manager", "m") {

@Override
public Setting<Boolean> legacySetting() {
// copy the setting here so we can mark it private in org.opensearch.node.Node
return Setting.boolSetting("node.master", true, Property.Deprecated, Property.NodeScope);
}

@Override
public DiscoveryNodeRole getCompatibilityRole(Version nodeVersion) {
if (nodeVersion.onOrAfter(Version.V_2_0_0)) {
return this;
} else {
return DiscoveryNodeRole.MASTER_ROLE;
}
}

@Override
public void validateRole(List<DiscoveryNodeRole> roles) {
if (roles.contains(DiscoveryNodeRole.MASTER_ROLE)) {
throw new IllegalArgumentException(
String.format(
Locale.ROOT,
"The two roles [%s, %s] can not be assigned together to a node. %s",
DiscoveryNodeRole.MASTER_ROLE.roleName(),
DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(),
MASTER_ROLE_DEPRECATION_MESSAGE
)
);
}
}

};

public static final DiscoveryNodeRole REMOTE_CLUSTER_CLIENT_ROLE = new DiscoveryNodeRole("remote_cluster_client", "r") {
Expand All @@ -223,7 +282,7 @@ public Setting<Boolean> legacySetting() {
* The built-in node roles.
*/
public static SortedSet<DiscoveryNodeRole> BUILT_IN_ROLES = Collections.unmodifiableSortedSet(
new TreeSet<>(Arrays.asList(DATA_ROLE, INGEST_ROLE, MASTER_ROLE, REMOTE_CLUSTER_CLIENT_ROLE))
new TreeSet<>(Arrays.asList(DATA_ROLE, INGEST_ROLE, CLUSTER_MANAGER_ROLE, REMOTE_CLUSTER_CLIENT_ROLE))
);

/**
Expand Down Expand Up @@ -262,4 +321,13 @@ public Setting<Boolean> legacySetting() {

}

/**
* 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.
* @return true if the node role is{@link #CLUSTER_MANAGER_ROLE} or {@link #MASTER_ROLE}
*/
@Deprecated
public boolean isClusterManager() {
return this.equals(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE) || this.equals(DiscoveryNodeRole.MASTER_ROLE);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ public DiscoveryNode resolveNode(String node) {
* Works by tracking the current set of nodes and applying each node specification in sequence. The set starts out empty and each node
* specification may either add or remove nodes. For instance:
*
* - _local, _master and _all respectively add to the subset the local node, the currently-elected master, and all the nodes
* - _local, _cluster_manager (_master) and _all respectively add to the subset the local node, the currently-elected cluster_manager, and all the nodes
* - node IDs, names, hostnames and IP addresses all add to the subset any nodes which match
* - a wildcard-based pattern of the form "attr*:value*" adds to the subset all nodes with a matching attribute with a matching value
* - role:true adds to the subset all nodes with a matching role
Expand All @@ -393,7 +393,7 @@ public String[] resolveNodes(String... nodes) {
if (localNodeId != null) {
resolvedNodesIds.add(localNodeId);
}
} else if (nodeId.equals("_master")) {
} else if (nodeId.equals("_master") || nodeId.equals("_cluster_manager")) {
String masterNodeId = getMasterNodeId();
if (masterNodeId != null) {
resolvedNodesIds.add(masterNodeId);
Expand All @@ -419,7 +419,7 @@ public String[] resolveNodes(String... nodes) {
} else {
resolvedNodesIds.removeAll(dataNodes.keys());
}
} else if (DiscoveryNodeRole.MASTER_ROLE.roleName().equals(matchAttrName)) {
} else if (roleNameIsClusterManager(matchAttrName)) {
if (Booleans.parseBoolean(matchAttrValue, true)) {
resolvedNodesIds.addAll(masterNodes.keys());
} else {
Expand Down Expand Up @@ -797,4 +797,17 @@ public boolean isLocalNodeElectedMaster() {
return masterNodeId != null && masterNodeId.equals(localNodeId);
}
}

/**
* Check if the given name of the node role is 'cluster_manger' or 'master'.
* The method is added for {@link #resolveNodes} to keep the code clear, when support the both above roles.
* @deprecated As of 2.0, because promoting inclusive language. MASTER_ROLE is deprecated.
* @param matchAttrName a given String for a name of the node role.
* @return true if the given roleName is 'cluster_manger' or 'master'
*/
@Deprecated
private boolean roleNameIsClusterManager(String matchAttrName) {
return DiscoveryNodeRole.MASTER_ROLE.roleName().equals(matchAttrName)
|| DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName().equals(matchAttrName);
}
}
Loading

0 comments on commit ea31483

Please sign in to comment.