[BUG] _cluster/stats API returning incorrect cluster_manager count

[sandeshkr419]

Describe the bug
_cluster/stats API returns wrong count of nodes with cluster_manager role.

To Reproduce
Steps to reproduce the behavior:

1. Create a multi-node cluster on OS 2.3 (I tried it on 2.3), lets say with 3 nodes with cluster_manager role.
2. Check response of _cat/nodes - which should show correct roles of each nodes.

ip         heap.percent ram.percent cpu load_1m load_5m load_15m node.role node.roles                                   cluster_manager name
10.0.3.37            40          77   0    0.00    0.00     0.00 dir       data,ingest,remote_cluster_client            -               data-node
10.0.5.179           45          77   0    0.00    0.00     0.00 dir       data,ingest,remote_cluster_client            -               data-node
10.0.4.180           12          76   0    0.00    0.02     0.01 -         ml                                           -               ml-node
10.0.4.224           41          76   0    0.00    0.00     0.00 mmr       cluster_manager,master,remote_cluster_client -               manager-node
10.0.4.16            37          78   0    0.00    0.00     0.00 dir       data,ingest,remote_cluster_client            -               data-node
10.0.3.181           13          76   0    0.00    0.00     0.00 mmr       cluster_manager,master,remote_cluster_client -               manager-node
10.0.5.122           17          76   0    0.01    0.01     0.00 mmr       cluster_manager,master,remote_cluster_client *               seed

3. Check response of _cluster/stats

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

Expected behavior
Count of cluster_manager and master should be 3 in above case.

Plugins
None

Screenshots
None

Host/Environment (please complete the following information):

• Version: 2.3

Additional context
The above response was correct till OS 1.3.x

[shwetathareja]

Bug is due to the fact it is iterating over all the roles (Master & Cluster Manager are 2 roles associated with nodes) and it will end up counting twice.

                for (DiscoveryNodeRole role : nodeInfo.getNode().getRoles()) {
                    // 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);

@sandeshkr419 do you want to pick up the fix?

[andrross]

I assigned the issue to you @sandeshkr419. Thanks in advance for the help! If for some reason you can't get to this then please provide an update here.

[dblock]

Ouch. If someone wants to start with a failing unit test that'd be awesome.

[sandeshkr419]

I started working on this and noticed this difference in 'master' role in single node vs dedicated cluster-manager setup:

Single node / localhost:

curl "localhost:9200/_cat/nodes?v"
ip        heap.percent ram.percent cpu load_1m load_5m load_15m node.role node.roles                                        cluster_manager name
127.0.0.1           33         100  12    8.40                  dimr      cluster_manager,data,ingest,remote_cluster_client *               runTask-0

Dedicated cluster-manager cluster:

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

Quick question: Any reason, why is the deprecated master role associated in a dedicated cluster-manager setup but not in single node setup? Should this behavior be consistent in both cases?

[sandeshkr419]

Making this change actually counts correctly based on roles. So for the above output of _cat/nodes the _cluster/stats output looks like (after the above change):

Single node setup (master count is 0 since role is not attached):

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

Multi-node setup (master count is 3 because master role is attached):

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

So making change to _cluster/stats API now depends on what behavior with roles do we want with dedicated & non-dedicated setup.

[dblock]

It sounds reasonable. Consider whether there's a breaking change here or just a bug fix. Write a bunch of specs that describe the various scenarios, and let's see if any existing ones break.

[andrross]

I started working on this and noticed this difference in 'master' role in single node vs dedicated cluster-manager setup:

@tlfeng Can you provide any insight into why the code behaves this way?

[tlfeng]

I will carefully look into the problem and provide insight. I'm surprised to see the REST API response is different for single node and multi-node cluster. The current test may only cover the situation of single-node cluster.

[sandeshkr419]

Looking into @tlfeng changes: #2424, it seems like the original idea was to replace the "master" role to "cluster_manager" role just and not have both the roles assigned to node.
It seems like a gap in testing where we have not validated/tested the same for a dedicated cluster-manager setup.

Present output for _nodes/_all for cluster-manager nodes:

....
"<node-id>" : {
      "name" : "manager-node",
      "transport_address" : "<>",
      "host" : "<>",
      "ip" : "<>",
      "version" : "2.5.1",
      "build_type" : "tar",
      "build_hash" : "415b06c6ea7d69735a392e7d69963f4a22547af7",
      "total_indexing_buffer" : 107374182,
      "roles" : [
        "cluster_manager",
        "master",
        "remote_cluster_client"
      ],
...

Looks like bug is with the roles attached to cluster-manager nodes in dedicated setup. I'll find out the code fixes for this - how to have only cluster-manager role and not both. The count should correct automatically after that. Does this sounds okay?