From 2b9bd122b2569591f1b2a14d0e0a21a15b3283e3 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 28 Mar 2023 05:20:08 +0000 Subject: [PATCH] Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting (#6331) `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. Signed-off-by: Sandesh Kumar (cherry picked from commit 5f89081d5256aadd1d90e8618deecb63fceb473a) Signed-off-by: github-actions[bot] --- CHANGELOG.md | 1 + .../admin/cluster/stats/ClusterStatsIT.java | 157 ++++++++++++++++-- .../cluster/node/DiscoveryNodeRoleIT.java | 16 ++ .../cluster/node/DiscoveryNode.java | 5 +- .../cluster/node/DiscoveryNodeRole.java | 9 +- .../node/DiscoveryNodeRoleSettingTests.java | 5 - 6 files changed, 165 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3af9b355be95b..41fae4c4da711 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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)) diff --git a/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/stats/ClusterStatsIT.java b/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/stats/ClusterStatsIT.java index 11d1af608fbee..1fa9ef2f91712 100644 --- a/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/stats/ClusterStatsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/stats/ClusterStatsIT.java @@ -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; @@ -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; @@ -83,14 +85,7 @@ private void waitForNodes(int numNodes) { public void testNodeCounts() { int total = 1; internalCluster().startNode(); - Map 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 expectedCounts = getExpectedCounts(1, 1, 1, 1, 1, 0, 0); int numNodes = randomIntBetween(1, 5); ClusterStatsResponse response = client().admin().cluster().prepareClusterStats().get(); @@ -147,7 +142,7 @@ 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())) @@ -155,17 +150,13 @@ public void testNodeCountsWithDeprecatedMasterRole() { internalCluster().startNode(settings); waitForNodes(total); - Map 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 expectedCounts = getExpectedCounts(0, 1, 1, 0, 0, 0, 0); ClusterStatsResponse response = client().admin().cluster().prepareClusterStats().get(); assertCounts(response.getNodesStats().getCounts(), total, expectedCounts); + + Set expectedRoles = Set.of(DiscoveryNodeRole.MASTER_ROLE.roleName()); + assertEquals(expectedRoles, getNodeRoles(0)); } private static void incrementCountForRole(String role, Map counts) { @@ -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 expectedCounts = getExpectedCounts(0, 1, 1, 0, 1, 0, 0); + + ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get(); + assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedCounts); + + Set 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 expectedCounts = getExpectedCounts(0, 1, 1, 0, 1, 0, 0); + + ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get(); + assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedCounts); + + Set 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 expectedRoleCounts = getExpectedCounts(1, 1, 1, 0, 1, 0, 0); + + ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get(); + assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedRoleCounts); + + Set 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 expectedRoleCounts = getExpectedCounts(1, 1, 1, 0, 1, 0, 0); + + ClusterStatsResponse clusterStatsResponse = client().admin().cluster().prepareClusterStats().get(); + assertCounts(clusterStatsResponse.getNodesStats().getCounts(), total, expectedRoleCounts); + + Set> 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 getExpectedCounts( + int dataRoleCount, + int masterRoleCount, + int clusterManagerRoleCount, + int ingestRoleCount, + int remoteClusterClientRoleCount, + int searchRoleCount, + int coordinatingOnlyCount + ) { + Map 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 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()); + } } diff --git a/server/src/internalClusterTest/java/org/opensearch/cluster/node/DiscoveryNodeRoleIT.java b/server/src/internalClusterTest/java/org/opensearch/cluster/node/DiscoveryNodeRoleIT.java index 4e30a7f6130fb..29c2f6e970144 100644 --- a/server/src/internalClusterTest/java/org/opensearch/cluster/node/DiscoveryNodeRoleIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/cluster/node/DiscoveryNodeRoleIT.java @@ -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; @@ -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")); + } } 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 b934984c20375..c6dbc0fd56293 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java @@ -287,10 +287,7 @@ public static Set 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()); } } 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 bfc44378632d8..07d70b2c6c1b2 100644 --- a/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java +++ b/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java @@ -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; @@ -245,8 +246,8 @@ public void validateRole(List roles) { @Override public Setting 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 @@ -273,6 +274,10 @@ public void validateRole(List 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") { diff --git a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleSettingTests.java b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleSettingTests.java index efcb80f8e3429..f5935af47fef5 100644 --- a/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleSettingTests.java +++ b/server/src/test/java/org/opensearch/cluster/node/DiscoveryNodeRoleSettingTests.java @@ -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); } @@ -96,5 +92,4 @@ private void runRoleTest(final Predicate predicate, final DiscoveryNod assertThat(e.getMessage(), startsWith("can not explicitly configure node roles and use legacy role setting")); assertNoDeprecationWarnings(); } - }