Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting #6331

sandeshkr419 · 2023-02-15T23:30:09Z

Description

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.

Reference in cdk where it uses legacy settings to bootstrap nodes: https://github.com/opensearch-project/opensearch-cluster-cdk/blob/main/lib/opensearch-config/multi-node-base-config.yml

Allowing only one of the both roles to be attached instead of both - 'master' which is deprecated as part of change of 'master' nomenclature: #472 - 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.

Issues Resolved

Resolves #6103

The change modifies the role attached in multi-node dedicated cluster-manager setup.

API response before the change:

Multi node with dedicated cluster-manager when bootstrapped with node.master: true:

..
"nodes" : {
    "count" : {
      "total" : 6,
      "cluster_manager" : 6,
      "coordinating_only" : 0,
      "data" : 3,
      "ingest" : 3,
      "master" : 6,
      "remote_cluster_client" : 6
    },
    "versions" : [
      "2.5.1"
    ],
..

curl "<cluster-endpoint>/_cat/nodes?v"
ip         heap.percent ram.percent cpu load_1m load_5m load_15m node.role node.roles                                   cluster_manager name
10.0.3.110           62          70   0    0.00    0.00     0.00 mmr       cluster_manager,master,remote_cluster_client -               manager-node
10.0.3.72            60          69   0    0.00    0.00     0.00 dir       data,ingest,remote_cluster_client            -               data-node
10.0.4.132           20          70   0    0.00    0.00     0.00 dir       data,ingest,remote_cluster_client            -               data-node
10.0.5.42            26          69   0    0.04    0.02     0.00 dir       data,ingest,remote_cluster_client            -               data-node
10.0.4.156           39          69   0    0.00    0.00     0.00 mmr       cluster_manager,master,remote_cluster_client -               manager-node
10.0.5.39            47          70   0    0.00    0.04     0.01 mmr       cluster_manager,master,remote_cluster_client *               seed

API response after the change:

Multi node with dedicated cluster-manager when bootstrapped with node.master: true:

..
"nodes" : {
    "count" : {
      "total" : 6,
      "cluster_manager" : 3,
      "coordinating_only" : 0,
      "data" : 3,
      "ingest" : 3,
      "master" : 3,
      "remote_cluster_client" : 6,
      "search" : 0
    },
..

curl   "<endpoint>/_cat/nodes?v"
ip         heap.percent ram.percent cpu load_1m load_5m load_15m node.role node.roles                            cluster_manager name
10.0.5.192           38          63   0    0.00    0.00     0.00 mr        master,remote_cluster_client -               manager-node
10.0.3.75            30          63   0    0.00    0.00     0.00 mr        master,remote_cluster_client -               manager-node
10.0.5.168           16          63   0    0.65    0.16     0.05 dir       data,ingest,remote_cluster_client     -               data-node
10.0.4.83            46          63   0    0.00    0.00     0.00 dir       data,ingest,remote_cluster_client     -               data-node
10.0.5.169           55          63   0    0.00    0.00     0.00 mr        master,remote_cluster_client *               seed
10.0.3.251           60          63   0    0.00    0.00     0.00 dir       data,ingest,remote_cluster_client     -               data-node

Check List

New functionality includes testing.
All tests pass
New functionality has been documented.
New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2023-02-15T23:54:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11205/
CommitID: 749f9f1
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-02-16T21:54:54Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11249/
CommitID: d3df6ee
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-01T23:58:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11879/
CommitID: 5bf7962
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

andrross · 2023-03-02T00:43:27Z

At first glance, adding the cluster manager role when node.master is true seems wrong. node.master is deprecated the same as adding master to node.roles, so it seems like that legacy setting should just add the master role and not the cluster_manager role. Can we just remove the linked code to fix this bug?

github-actions · 2023-03-02T01:54:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11891/
CommitID: e16a4f7
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-02T20:16:36Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11920/
CommitID: 74b94ca
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

sandeshkr419 · 2023-03-02T23:07:25Z

Thanks @andrross for providing feedback. I appreciate the ideation of keeping the deprecated legacy settings to be restricted to deprecated roles, in terms of selection of master and cluster_manager for the deprecated node.master setting. This makes sense with the understanding that in future OS 3.x release, master role and all legacy settings including node.master will be removed in entirely.
I don't think removing this is within the scope of OS 2.x and specially in this PR. I tried removing this legacy setting from here and it breaks a whole lot of different things - which requires a complete code refactoring exercise which IMO is worth with OS 3.x where we deprecate this entirely.

Here is the new response from _cat/nodes when the cluster is bootstrapped using legacy settings:

ip         heap.percent ram.percent cpu load_1m load_5m load_15m node.role node.roles                        cluster_manager name
10.0.5.117           21          61   0    0.00    0.01     0.03 dir       data,ingest,remote_cluster_client -               data-node
10.0.3.38            52          62   0    0.00    0.00     0.01 mr        master,remote_cluster_client      *               seed
10.0.4.97            26          61   0    0.00    0.00     0.00 mr        master,remote_cluster_client      -               manager-node
10.0.5.82            28          61   0    0.00    0.00     0.00 mr        master,remote_cluster_client      -               manager-node
10.0.4.191           27          62   0    0.00    0.00     0.00 dir       data,ingest,remote_cluster_client -               data-node

_cluster/stats:

..
...
 "nodes" : {
    "count" : {
      "total" : 5,
      "cluster_manager" : 3,
      "coordinating_only" : 0,
      "data" : 2,
      "ingest" : 2,
      "master" : 3,
      "remote_cluster_client" : 5,
      "search" : 0
    }
...
..

The tests added validate node roles and cluster stats response

I still want to point that the legacy settings: when you provide a combination of node.master, node.data, node.ingest, it sets up these 3 roles: cluster_manager/master, data, ingest alongwith the 4th remote_cluster_client role for a node. remote_cluster_client doesn't gets setup automatically unless provided explicitly when using node.roles - I have tested this out by spinning up a cluster using cdk and providing these 3 node roles (data, ingest, master) instead of legacy settings - you get only the roles you specify and not the remote_cluster_client.

When setting up explicit roles - master or cluster_manager roles via node.roles, the behavior would continue to exist as before: the role will be provided as specified by the user.

github-actions · 2023-03-02T23:08:09Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11924/
CommitID: ec76209
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

shwetathareja · 2023-03-03T12:01:32Z

...r/src/internalClusterTest/java/org/opensearch/action/admin/cluster/stats/ClusterStatsIT.java

@@ -322,4 +323,81 @@ public void testFieldTypes() {
            }
        }
    }
+
+    public void testNodeCountsWithLegacyMasterSetting() throws ExecutionException, InterruptedException {


please add test with cluster manager role as well

Thanks @shwetathareja for checking. Added some tests and refactored some.

github-actions · 2023-03-06T05:52:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11995/
CommitID: 8353014
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

andrross · 2023-03-28T00:16:49Z

server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java

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

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


Nitpick, the the prevailing style is Booleans.isBoolean(settings.get("node.master")) == false

Thanks for reviewing @andrross! Addressed the comment and re-based my changes against main to resolve conflicts in CHANGELOG.md

…ovided Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com> Remove 'node.master' legacy setting from 'cluster_manager' role Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com> Typo in test Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com> Add tests with node.roles for cluster manager Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com> minor refactoring with specifying node roles Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com> Revert "minor refactoring with specifying node roles" This reverts commit c9c123e. Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com> minor refactoring with specifying node roles Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

github-actions · 2023-03-28T01:44:06Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12971/
CommitID: b7cda75
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-28T02:33:07Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12973/
CommitID: b7cda75
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

github-actions · 2023-03-28T03:15:41Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12974/
CommitID: 6dffd09
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

github-actions · 2023-03-28T04:29:59Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12976/
CommitID: 6dffd09
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-28T04:46:27Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/12977/
CommitID: 35327e1

…recated 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 <sandeshkr419@gmail.com> (cherry picked from commit 5f89081) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…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>

…recated setting (opensearch-project#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 <sandeshkr419@gmail.com> Signed-off-by: Valentin Mitrofanov <mitrofmep@gmail.com>

sandeshkr419 changed the title ~~[DRAFT] Remove deprecated 'master' role~~ [DRAFT] Remove deprecated 'master' role attachment from 'cluster_manager` nodes Feb 16, 2023

sandeshkr419 mentioned this pull request Feb 16, 2023

[BUG] _cluster/stats API returning incorrect cluster_manager count #6103

Closed

sandeshkr419 force-pushed the node-roles-fix branch from 5bf7962 to e16a4f7 Compare March 2, 2023 01:38

sandeshkr419 force-pushed the node-roles-fix branch from e16a4f7 to 74b94ca Compare March 2, 2023 20:00

shwetathareja reviewed Mar 3, 2023

View reviewed changes

sandeshkr419 changed the title ~~[DRAFT] Remove deprecated 'master' role attachment from 'cluster_manager` nodes~~ [DRAFT] Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting Mar 6, 2023

sandeshkr419 marked this pull request as ready for review March 6, 2023 06:05

sandeshkr419 requested review from reta, anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19 and adnapibar as code owners March 6, 2023 06:05

tlfeng added the backport 2.x Backport to 2.x branch label Mar 27, 2023

andrross approved these changes Mar 28, 2023

View reviewed changes

sandeshkr419 added 10 commits March 27, 2023 18:19

test modificaiton

c8dff61

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

Add Locale.Root in String format()

7b93d8c

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

./gradlew :server:spotlessApply code formatting

e9bad81

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

modify settings filtering by using getRolesFromSettings()

ec35774

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

Edit Changelog

ca821e0

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

Add test with deprecated master and cluster manager role

a94f6e1

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

Fix role filtering from legacy settings

53d9d2e

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

Override isEnabledByDefault() for cluster manager role

a8fc151

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

minor refactoring with boolean condition

b7cda75

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

sandeshkr419 force-pushed the node-roles-fix branch from 10493ee to b7cda75 Compare March 28, 2023 01:20

Remove test flakiness

6dffd09

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

Test reduce flakiness

35327e1

Signed-off-by: Sandesh Kumar <sandeshkr419@gmail.com>

tlfeng merged commit 5f89081 into opensearch-project:main Mar 28, 2023

sandeshkr419 deleted the node-roles-fix branch March 28, 2023 05:19

opensearch-trigger-bot bot mentioned this pull request Mar 28, 2023

[Backport 2.x] Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting #6853

Merged

This was referenced Mar 30, 2023

[Backport 2.x] Xcontent fix opensearch-project/security#2613

Closed

Fix failing OpenSSL tests opensearch-project/security#2614

Closed

peternied mentioned this pull request Mar 30, 2023

[BUILD BREAK] SeedHostsResolver failed to resolve host | OpenSSLTest test failures opensearch-project/security#2612

Closed

kotwanikunal mentioned this pull request May 3, 2023

[BUG] Test org.opensearch.action.admin.cluster.stats.ClusterStatsIT.testNodeRolesWithDataNodeLegacySetting is flaky #7327

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting #6331

Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting #6331

sandeshkr419 commented Feb 15, 2023 •

edited

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Mar 1, 2023

andrross commented Mar 2, 2023

github-actions bot commented Mar 2, 2023

github-actions bot commented Mar 2, 2023

sandeshkr419 commented Mar 2, 2023

github-actions bot commented Mar 2, 2023

shwetathareja Mar 3, 2023

sandeshkr419 Mar 6, 2023 •

edited

github-actions bot commented Mar 6, 2023

andrross Mar 28, 2023

sandeshkr419 Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting #6331

Remove 'cluster_manager' role attachment when using 'node.master' deprecated setting #6331

Conversation

sandeshkr419 commented Feb 15, 2023 • edited

Description

Issues Resolved

Check List

github-actions bot commented Feb 15, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Feb 16, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 1, 2023

Gradle Check (Jenkins) Run Completed with:

andrross commented Mar 2, 2023

github-actions bot commented Mar 2, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 2, 2023

Gradle Check (Jenkins) Run Completed with:

sandeshkr419 commented Mar 2, 2023

github-actions bot commented Mar 2, 2023

Gradle Check (Jenkins) Run Completed with:

shwetathareja Mar 3, 2023

Choose a reason for hiding this comment

sandeshkr419 Mar 6, 2023 • edited

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2023

Gradle Check (Jenkins) Run Completed with:

andrross Mar 28, 2023

Choose a reason for hiding this comment

sandeshkr419 Mar 28, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 28, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 28, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 28, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 28, 2023

Gradle Check (Jenkins) Run Completed with:

sandeshkr419 commented Feb 15, 2023 •

edited

sandeshkr419 Mar 6, 2023 •

edited