Support dynamic node role

[ylwu-amzn]

Signed-off-by: Yaliang Wu [email protected]

Description

Currently OpenSearch only supports several built-in nodes like data node
role. If specify unknown node role, OpenSearch node will fail to start.
This limit how to extend OpenSearch to support specific tasks. For
example, user may prefer to run ML tasks on some dedicated node which
doesn't serve as any built-in node roles. So the ML tasks won't impact
OpenSearch core function. This PR removed the limitation. So user can
specify any node role and OpenSearch will start node correctly with that
unknown role. This opens the door for plugin developer to run specific
tasks on dedicated nodes.

Issues Resolved

#2877

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

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.

[opensearch-ci-bot]

❌   Gradle Check failure e0533988c409f2d8ade38a001088f53b4c202e2b
Log 5576

Reports 5576

[opensearch-ci-bot]

❌   Gradle Check failure a9366bbd3633b6657d90924be60d54b37b54c11f
Log 5592

Reports 5592

[opensearch-ci-bot]

❌   Gradle Check failure 72c85a6a674ccbe2483ff568fbf94f3df72ff287
Log 5596

Reports 5596

[opensearch-ci-bot]

❌   Gradle Check failure 4cb8dac
Log 5603

Reports 5603

[opensearch-ci-bot]

❌   Gradle Check failure 2f2df16
Log 5605

Reports 5605

[dblock]

Is Unknown the right name? With this feature there's nothing unknown about it, so can we rename it? I think NamedRole would be closer to what we're trying to do.

[dblock]

I think UnknownRole, aka NamedRole should have a constructor that takes the roleName, and we don't need a generator.

[ylwu-amzn]

If we remove this generator , we need to make the DynamicRole class public and expose the constructor as public. That seems open some door to hack node roles. For example, some plugin can create new DynamicRole in code directly rather than reply on the opensearch.yml, that breaks the design. For example, if user tweak the node roles by changing from image_processing to ml, then the ml task may be dispatched to a wrong node.

This generator is only for testing. How about we keep this?

[reta]

It seems we should not use UnknownRole here but introduce the concept of DynamicRole, complementing the builtin roles. Why: the UnknownRole is supposed to be used to deal with migrations when some of the builtin roles may be removed in new version but still present in the old one. The DynamicRole would help to introduce such distinction.

[ylwu-amzn]

Thanks. Good point. As we support dynamic/custom roles, it will be hard to differentiate UnknownRole (for example deprecated built-in role like master) and DynamicRole role (for example ml role) as both are not in built-in role list. So how about we just rename the UnknownRole as DynamicRole?

[ylwu-amzn]

Oh, I think we should keep UnknownRole for bwc. If we plan to release this change on OS 2.1, the logic of https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/cluster/node/DiscoveryNode.java#L331 will be

if (in.getVersion().onOrAfter(Version.V_2_1_0)) {
    roles.add(new DiscoveryNodeRole.DynamicRole(roleName, roleNameAbbreviation, canContainData));
} else {
    roles.add(new DiscoveryNodeRole.UnknownRole(roleName, roleNameAbbreviation, canContainData));
}

[reta]

Is Unknown the right name? With this feature there's nothing unknown about it, so can we rename it? I think NamedRole would be closer to what we're trying to do.

👍 , suggesting the DynamicRole (to complement builtin roles) but name is up to the debate (NamedRole seems to be general since all roles do have name, imho)

[ylwu-amzn]

Is Unknown the right name? With this feature there's nothing unknown about it, so can we rename it? I think NamedRole would be closer to what we're trying to do.

👍 , suggesting the DynamicRole (to complement builtin roles) but name is up to the debate (NamedRole seems to be general since all roles do have name, imho)

Thanks, I think DynamicRole or NamedRole is better than UnknownRole, like @dblock said, there will not be unknown role as we are going to support any custom role.

I'm ok for either. Considering the role is complementing builtin roles, maybe DynamicRole is better?

[opensearch-ci-bot]

❌   Gradle Check failure 4245543
Log 5697

Reports 5697

[opensearch-ci-bot]

❌   Gradle Check failure a2fea20
Log 5698

Reports 5698

[opensearch-ci-bot]

✅   Gradle Check success 836be1f
Log 5710

Reports 5710

[reta]

While experimenting, run into interesting issue: the role name are case-sensitive, for example:

node.roles: [ master, Master]

Are 2 different roles. In general, it was fine, OpenSearch would refuse to start because Master was not recognized as a valid role. You may already see where I am going: with the dynamic roles, such set of roles becomes totally valid and confusing:

curl http://localhost:9200/_cat/nodes
127.0.0.1 14 84 7 2.24 2.35 3.06 m Master,master * my-ThinkPad-P15-Gen-1

I think we should be a bit more strict now with the way roles are treated and checks should be case-insensitive: in the example above master and Master should be reduced to same role master. @ylwu-amzn wdyt?

[dtaivpp]

@ylwu-amzn can I have you look over this?

[dblock]

The whole case-sensitive thing can be dealt with as a separate issue IMO.

[ylwu-amzn]

Sure, thanks for reminding. Busy with something else and missed this comment.

@reta that's a great point! I will make the code change to make role name case-insensitive.

[dblock]

I am good with this! @reta any objections?

[reta]

I am good with this! @reta any objections?

LGTM, thanks @ylwu-amzn !

[dblock]

Since you added some lower case node name transform, please also add unit tests for the fact that these are now case-insensitive.

[opensearch-ci-bot]

❌   Gradle Check failure 5101c9a
Log 5990

Reports 5990

[ylwu-amzn]

Since you added some lower case node name transform, please also add unit tests for the fact that these are now case-insensitive.

Sure, working on testing now. Publish change earlier to check if Github workflow can pass. It runs very slow on my dev desktop.

[opensearch-ci-bot]

✅   Gradle Check success 6e286c1
Log 5991

Reports 5991

[opensearch-ci-bot]

✅   Gradle Check success 8e2f2d8
Log 5992

Reports 5992

[opensearch-ci-bot]

✅   Gradle Check success 81d0d60
Log 5995

Reports 5995

[ylwu-amzn]

@reta @dblock Added tests for role name case insensitive change and Github workflow passed. Can you help review?

[reta]

LGTM!

[ylwu-amzn]

If no more comments, can anyone help merge this PR? I have no permission to merge.

[saratvemulapalli]

Thanks @ylwu-amzn for this.
I've spent sometime understanding the issue.

[dblock]

Good work @ylwu-amzn! Thanks.