Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A node with roles [ml, remote_cluster_client] is still dedicated ML #66533

Conversation

droberts195
Copy link
Contributor

Where CCS is being used it makes sense for ML nodes to have the
remote_cluster_client role. This single extra role is not
significant enough to stop an ML node being considered a
"dedicated ML node".

Where CCS is being used it makes sense for ML nodes to have the
remote_cluster_client role.  This single extra role is not
significant enough to stop an ML node being considered a
"dedicated ML node".
@droberts195 droberts195 added >non-issue :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts :ml Machine learning v8.0.0 :Core/Infra/Packaging labels Dec 17, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@elasticmachine elasticmachine added Team:Delivery Meta label for Delivery team Team:Core/Infra Meta label for core/infra team labels Dec 17, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@droberts195
Copy link
Contributor Author

>non-issue as it's a minor tweak to #65905 and that hasn't been released yet.

@@ -142,7 +142,7 @@ public static MachineNodeRole parse(InputStream config) {
return MachineNodeRole.DATA;
} else if (containsOnly(roles, "master")) {
return MachineNodeRole.MASTER_ONLY;
} else if (containsOnly(roles, "ml")) {
} else if (containsOnly(roles, "ml") || containsOnly(roles, "ml", "remote_cluster_client")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to else if (containsOnly(roles, "ml", "remote_cluster_client"). That simply asserts that only the listed roles area llowd to be in the given collection, but a subset is fine as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking now, this would also be true in a case where a node had only the remote_cluster_client role, so maybe we might want to assert roles.contains("ml") && containsOnly(roles, "ml", "remote_cluster_client")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I did that and also added a test to prove remote_cluster_client on its own isn't an ML node.

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/packaging-windows

1 similar comment
@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/packaging-windows

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@droberts195 droberts195 merged commit bf78062 into elastic:master Dec 18, 2020
@droberts195 droberts195 deleted the allow_remote_cluster_client_for_dedicated_ml_node branch December 18, 2020 13:00
droberts195 added a commit that referenced this pull request Dec 18, 2020
…66533)

Where CCS is being used it makes sense for ML nodes to have the
remote_cluster_client role.  This single extra role is not
significant enough to stop an ML node being considered a
"dedicated ML node".
droberts195 added a commit that referenced this pull request Dec 18, 2020
…66533)

Where CCS is being used it makes sense for ML nodes to have the
remote_cluster_client role.  This single extra role is not
significant enough to stop an ML node being considered a
"dedicated ML node".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts :ml Machine learning >non-issue Team:Core/Infra Meta label for core/infra team Team:Delivery Meta label for Delivery team v7.11.0 v7.12.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants