Skip to content

Commit

Permalink
Remove 'cluster_manager' role attachment when using 'node.master' dep…
Browse files Browse the repository at this point in the history 
…recated setting (#6331) (#6853)

`cluster_manager` role no longer configures the legacy `node.master` setting.

Using `node.master: true` configures both cluster_manager and master role to a node which is not correct, only one of them should be assigned

Allowing only one of the both roles to be attached instead of both - 'master' which is deprecated - since both the legacy setting `node.master` and `master` role are deprecated - so we need to keep the initial behavior only with deprecated roles and settings.

(cherry picked from commit 5f89081)

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>
  • Loading branch information
@opensearch-trigger-bot
opensearch-trigger-bot[bot] authored Mar 28, 2023
1 parent 8356d2b commit 8b47610
Show file tree
Hide file tree
Showing 6 changed files with 165 additions and 28 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- [Remote Store] Integrate remote segment store in peer recovery flow ([#6664](https://github.com/opensearch-project/OpenSearch/pull/6664))
- [Segment Replication] Add new cluster setting to set replication strategy by default for all indices in cluster. ([#6791](https://github.com/opensearch-project/OpenSearch/pull/6791))
- Enable sort optimization for all NumericTypes ([#6464](https://github.com/opensearch-project/OpenSearch/pull/6464)
- Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting ([#6331](https://github.com/opensearch-project/OpenSearch/pull/6331))

### Dependencies
- Bump `org.apache.logging.log4j:log4j-core` from 2.18.0 to 2.20.0 ([#6490](https://github.com/opensearch-project/OpenSearch/pull/6490))
Expand Down
157 changes: 140 additions & 17 deletions ...rc/internalClusterTest/java/org/opensearch/action/admin/cluster/stats/ClusterStatsIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.opensearch.Version;
import org.opensearch.action.admin.cluster.health.ClusterHealthResponse;
import org.opensearch.action.admin.cluster.node.stats.NodeStats;
import org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest;
import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse;
import org.opensearch.client.Requests;
import org.opensearch.cluster.health.ClusterHealthStatus;
Expand All @@ -54,6 +55,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
Expand Down Expand Up @@ -83,14 +85,7 @@ private void waitForNodes(int numNodes) {
public void testNodeCounts() {
int total = 1;
internalCluster().startNode();
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(DiscoveryNodeRole.SEARCH_ROLE.roleName(), 0);
expectedCounts.put(ClusterStatsNodes.Counts.COORDINATING_ONLY, 0);
Map<String, Integer> expectedCounts = getExpectedCounts(1, 1, 1, 1, 1, 0, 0);
int numNodes = randomIntBetween(1, 5);

ClusterStatsResponse response = client().admin().cluster().prepareClusterStats().get();
Expand Down Expand Up @@ -147,25 +142,21 @@ public void testNodeCounts() {
}

// Validate assigning value "master" to setting "node.roles" can get correct count in Node Stats response after MASTER_ROLE deprecated.
public void testNodeCountsWithDeprecatedMasterRole() {
public void testNodeCountsWithDeprecatedMasterRole() throws ExecutionException, InterruptedException {
int total = 1;
Settings settings = Settings.builder()
.putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), Collections.singletonList(DiscoveryNodeRole.MASTER_ROLE.roleName()))
.build();
internalCluster().startNode(settings);
waitForNodes(total);

Map<String, Integer> expectedCounts = new HashMap<>();
expectedCounts.put(DiscoveryNodeRole.DATA_ROLE.roleName(), 0);
expectedCounts.put(DiscoveryNodeRole.MASTER_ROLE.roleName(), 1);
expectedCounts.put(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(), 1);
expectedCounts.put(DiscoveryNodeRole.INGEST_ROLE.roleName(), 0);
expectedCounts.put(DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName(), 0);
expectedCounts.put(DiscoveryNodeRole.SEARCH_ROLE.roleName(), 0);
expectedCounts.put(ClusterStatsNodes.Counts.COORDINATING_ONLY, 0);
Map<String, Integer> expectedCounts = getExpectedCounts(0, 1, 1, 0, 0, 0, 0);

ClusterStatsResponse response = client().admin().cluster().prepareClusterStats().get();
assertCounts(response.getNodesStats().getCounts(), total, expectedCounts);

Set<String> expectedRoles = Set.of(DiscoveryNodeRole.MASTER_ROLE.roleName());
assertEquals(expectedRoles, getNodeRoles(0));
}

private static void incrementCountForRole(String role, Map<String, Integer> counts) {
Expand Down Expand Up @@ -322,4 +313,136 @@ public void testFieldTypes() {
}
}
}

public void testNodeRolesWithMasterLegacySettings() throws ExecutionException, InterruptedException {
int total = 1;
Settings legacyMasterSettings = Settings.builder()
.put("node.master", true)
.put("node.data", false)
.put("node.ingest", false)
.build();

internalCluster().startNodes(legacyMasterSettings);
waitForNodes(total);

Map<String, Integer> expectedCounts = getExpectedCounts(0, 1, 1, 0, 1, 0, 0);

ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get();
assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedCounts);

Set<String> expectedRoles = Set.of(
DiscoveryNodeRole.MASTER_ROLE.roleName(),
DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName()
);
assertEquals(expectedRoles, getNodeRoles(0));
}

public void testNodeRolesWithClusterManagerRole() throws ExecutionException, InterruptedException {
int total = 1;
Settings clusterManagerNodeRoleSettings = Settings.builder()
.put(
"node.roles",
String.format(
Locale.ROOT,
"%s, %s",
DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(),
DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName()
)
)
.build();

internalCluster().startNodes(clusterManagerNodeRoleSettings);
waitForNodes(total);

Map<String, Integer> expectedCounts = getExpectedCounts(0, 1, 1, 0, 1, 0, 0);

ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get();
assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedCounts);

Set<String> expectedRoles = Set.of(
DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(),
DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName()
);
assertEquals(expectedRoles, getNodeRoles(0));
}

public void testNodeRolesWithSeedDataNodeLegacySettings() throws ExecutionException, InterruptedException {
int total = 1;
Settings legacySeedDataNodeSettings = Settings.builder()
.put("node.master", true)
.put("node.data", true)
.put("node.ingest", false)
.build();

internalCluster().startNodes(legacySeedDataNodeSettings);
waitForNodes(total);

Map<String, Integer> expectedRoleCounts = getExpectedCounts(1, 1, 1, 0, 1, 0, 0);

ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get();
assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedRoleCounts);

Set<String> expectedRoles = Set.of(
DiscoveryNodeRole.MASTER_ROLE.roleName(),
DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName(),
DiscoveryNodeRole.DATA_ROLE.roleName()
);
assertEquals(expectedRoles, getNodeRoles(0));
}

public void testNodeRolesWithDataNodeLegacySettings() throws ExecutionException, InterruptedException {
int total = 2;
Settings legacyDataNodeSettings = Settings.builder()
.put("node.master", false)
.put("node.data", true)
.put("node.ingest", false)
.build();

// can't start data-only node without assigning cluster-manager
internalCluster().startClusterManagerOnlyNodes(1);
internalCluster().startNodes(legacyDataNodeSettings);
waitForNodes(total);

Map<String, Integer> expectedRoleCounts = getExpectedCounts(1, 1, 1, 0, 1, 0, 0);

ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get();
assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedRoleCounts);

Set<Set<String>> expectedNodesRoles = Set.of(
Set.of(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName()),
Set.of(DiscoveryNodeRole.DATA_ROLE.roleName(), DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName())
);
assertEquals(expectedNodesRoles, Set.of(getNodeRoles(0), getNodeRoles(1)));
}

private Map<String, Integer> getExpectedCounts(
int dataRoleCount,
int masterRoleCount,
int clusterManagerRoleCount,
int ingestRoleCount,
int remoteClusterClientRoleCount,
int searchRoleCount,
int coordinatingOnlyCount
) {
Map<String, Integer> expectedCounts = new HashMap<>();
expectedCounts.put(DiscoveryNodeRole.DATA_ROLE.roleName(), dataRoleCount);
expectedCounts.put(DiscoveryNodeRole.MASTER_ROLE.roleName(), masterRoleCount);
expectedCounts.put(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE.roleName(), clusterManagerRoleCount);
expectedCounts.put(DiscoveryNodeRole.INGEST_ROLE.roleName(), ingestRoleCount);
expectedCounts.put(DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE.roleName(), remoteClusterClientRoleCount);
expectedCounts.put(DiscoveryNodeRole.SEARCH_ROLE.roleName(), searchRoleCount);
expectedCounts.put(ClusterStatsNodes.Counts.COORDINATING_ONLY, coordinatingOnlyCount);
return expectedCounts;
}

private Set<String> getNodeRoles(int nodeNumber) throws ExecutionException, InterruptedException {
NodesStatsResponse nodesStatsResponse = client().admin().cluster().nodesStats(new NodesStatsRequest()).get();
return nodesStatsResponse.getNodes()
.get(nodeNumber)
.getNode()
.getRoles()
.stream()
.map(DiscoveryNodeRole::roleName)
.collect(Collectors.toSet());
}
}
16 changes: 16 additions & 0 deletions server/src/internalClusterTest/java/org/opensearch/cluster/node/DiscoveryNodeRoleIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@
import java.util.List;
import java.util.Set;

import static org.hamcrest.Matchers.startsWith;
import static org.opensearch.test.NodeRoles.addRoles;
import static org.opensearch.test.NodeRoles.onlyRole;
import static org.opensearch.test.NodeRoles.removeRoles;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.hasSize;
Expand Down Expand Up @@ -126,4 +128,18 @@ private void runTestNodeHasAdditionalRole(final Settings settings) {
assertThat(response.getNodes().get(0).getNode().getRoles(), matcher);
}

public void testStartNodeWithClusterManagerRoleAndMasterSetting() {
final Settings settings = Settings.builder()
.put("node.master", randomBoolean())
.put(onlyRole(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE))
.build();

final IllegalArgumentException e1 = expectThrows(
IllegalArgumentException.class,
() -> DiscoveryNode.getRolesFromSettings(settings)
);
assertThat(e1.getMessage(), startsWith("can not explicitly configure node roles and use legacy role setting"));
final IllegalArgumentException e2 = expectThrows(IllegalArgumentException.class, () -> internalCluster().startNodes(settings));
assertThat(e2.getMessage(), startsWith("can not explicitly configure node roles and use legacy role setting"));
}
}
5 changes: 1 addition & 4 deletions server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,7 @@ public static Set<DiscoveryNodeRole> getRolesFromSettings(final Settings setting
validateLegacySettings(settings, roleMap);
return Collections.unmodifiableSet(new HashSet<>(NODE_ROLES_SETTING.get(settings)));
} else {
return roleMap.values()
.stream()
.filter(s -> s.legacySetting() != null && s.legacySetting().get(settings))
.collect(Collectors.toSet());
return roleMap.values().stream().filter(s -> s.isEnabledByDefault(settings)).collect(Collectors.toSet());
}
}

Expand Down
9 changes: 7 additions & 2 deletions server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

import org.opensearch.LegacyESVersion;
import org.opensearch.Version;
import org.opensearch.common.Booleans;
import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.common.settings.Setting;
import org.opensearch.common.settings.Setting.Property;
Expand Down Expand Up @@ -245,8 +246,8 @@ public void validateRole(List<DiscoveryNodeRole> roles) {

@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);
// 'cluster_manager' role should not configure legacy setting since deprecated 'master' role is supported till OS 2.x
return null;
}

@Override
Expand All @@ -273,6 +274,10 @@ public void validateRole(List<DiscoveryNodeRole> roles) {
}
}

@Override
public boolean isEnabledByDefault(final Settings settings) {
return Booleans.isBoolean(settings.get("node.master")) == false;
}
};

public static final DiscoveryNodeRole REMOTE_CLUSTER_CLIENT_ROLE = new DiscoveryNodeRole("remote_cluster_client", "r") {
Expand Down
5 changes: 0 additions & 5 deletions server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleSettingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,6 @@ public void testIsMasterNode() {
runRoleTest(DiscoveryNode::isClusterManagerNode, DiscoveryNodeRole.MASTER_ROLE);
}

public void testIsClusterManagerNode() {
runRoleTest(DiscoveryNode::isClusterManagerNode, DiscoveryNodeRole.CLUSTER_MANAGER_ROLE);
}

public void testIsRemoteClusterClient() {
runRoleTest(DiscoveryNode::isRemoteClusterClient, DiscoveryNodeRole.REMOTE_CLUSTER_CLIENT_ROLE);
}
Expand Down Expand Up @@ -96,5 +92,4 @@ private void runRoleTest(final Predicate<Settings> predicate, final DiscoveryNod
assertThat(e.getMessage(), startsWith("can not explicitly configure node roles and use legacy role setting"));
assertNoDeprecationWarnings();
}

}

0 comments on commit 8b47610

Please sign in to comment.