Voting config exclusions should work with absent nodes #50836

zacharymorn · 2020-01-10T06:46:49Z

Voting config exclusions should work with absent nodes. For details, please see #47990.

elasticmachine · 2020-01-10T06:50:21Z

Pinging @elastic/es-distributed (:Distributed/Cluster Coordination)

DaveCTurner

Thanks for looking at this.

We would like to entirely drop support for the more complex node resolution process from DiscoveryNodes#resolveNodes and only support node names and node IDs.

I suggest doing this by deprecating support for describing the nodes using POST /_cluster/voting_config_exclusions/{node_name} and instead adding query parameters ?node_names={...} and ?node_ids={...}. We could use these to add VotingConfigExclusions for the corresponding nodes, using a placeholder for nodeId or nodeName if it is not known at resolution time.

zacharymorn · 2020-01-13T05:16:52Z

I see.

I tried to add the suggested query params as follow and this gave me an error. It seems as query param might be optional, they don't get taken into account when checking for url uniqueness

public RestAddVotingConfigExclusionAction(RestController controller) {
    // This API is existing and being deprecated.
    controller.registerHandler(RestRequest.Method.POST, “/_cluster/voting_config_exclusions/{node_name}”, this);

    // These are new
    controller.registerHandler(RestRequest.Method.POST, "/_cluster/voting_config_exclusions?node_names={node_names}", this);
    controller.registerHandler(RestRequest.Method.POST, "/_cluster/voting_config_exclusions?node_ids={node_ids}", this);
}

org.elasticsearch.bootstrap.StartupException: java.lang.IllegalArgumentException: Trying to use conflicting wildcard names for same path: node_names and node_ids

In addition, when I just keep one of those two, it still conflicts with RestClearVotingConfigExclusionsAction as it's registered with the url /_cluster/voting_config_exclusions

What do you think we register with the following urls instead?

/_cluster/voting_config_exclusions/node_names/{node_names}
/_cluster/voting_config_exclusions/node_ids/{node_ids}

…name

zacharymorn · 2020-01-20T04:56:01Z

@DaveCTurner I just pushed a new commit to use a new API that excludes node just based on id or name. Could you please let me know if this is in the right direction?

DaveCTurner

Thanks for the update @zacharymorn and apologies for taking longer than normal to get back to you. I have left some more comments on the approach.

...ain/java/org/elasticsearch/rest/action/admin/cluster/RestAddVotingConfigExclusionAction.java

...a/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java

zacharymorn · 2020-02-28T06:19:55Z

@DaveCTurner I’ve taken your suggestions and made a new commit. The changes could use more unit tests but I would like to get your feedback early. Could you please take a look and let me know if it is in the right direction?

DaveCTurner

Thanks @zacharymorn, this looks neater. I left a few more comments.

...a/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java

DaveCTurner · 2020-03-02T08:37:27Z

...a/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java

@@ -47,7 +53,7 @@
     * @param nodeDescriptions Descriptions of the nodes to add - see {@link DiscoveryNodes#resolveNodes(String...)}
     */
    public AddVotingConfigExclusionsRequest(String[] nodeDescriptions) {


This constructor is only used in tests, and it looks like we could migrate all of those tests over to using node names instead of node descriptions. Some of them would also be neater if we used a varargs:

Suggested change

public AddVotingConfigExclusionsRequest(String[] nodeDescriptions) {

public AddVotingConfigExclusionsRequest(String... nodeNames) {

If we migrate these tests now to use node names instead of descriptions, I'm a bit concerned that we may not have tests to prove that the changes are still backward compatible and don't have bugs that may break logic based on nodeDescriptions, before it is fully migrated to nodeIds / nodeNames. What do you think?

Sorry, you're right, I didn't quite mean "all" these tests. We should comprehensively test the different kinds of node resolution by strengthening AddVotingConfigExclusionsRequestTests. The other tests can move over to node names without loss of coverage IMO.

Make sense.

This isn't resolved yet?

Sorry missed this earlier. Done in commit 02a3533

You removed this useful constructor in 02a3533 and added a lot of noise to the tests as a result. Could you follow my suggestion above instead?

Sorry when I re-read this thread I somehow got the wrong idea that this constructor need to be removed. I reverted that commit and tried again in commit 5c7a226. Could you let me know if this looks good?

server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java

DaveCTurner · 2020-03-02T08:57:02Z

server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java

+            }
+            else {
+                Map<String, String> existingNodesNameId = new HashMap<>();
+                for (DiscoveryNode node : this) {


Maybe just the master-eligible nodes?

Sorry a question just came up when I looked at this again. When we resolve by nodeId, we use ALL existing nodes to check if it exists, not just the master-eligible ones. Shall we keep this behavior the same for resolving by node name as well?

I'll open a new comment thread on the newly-moved code.

...a/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java

…joins

zacharymorn · 2020-03-10T05:21:08Z

@DaveCTurner I've added more tests to cover most of my changes I believe, could you please take a look and let me know if more is needed?

DaveCTurner

Thanks @zacharymorn, I have done another pass and left more comments.

DaveCTurner · 2020-03-25T09:42:17Z

...a/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java

@@ -47,7 +53,7 @@
     * @param nodeDescriptions Descriptions of the nodes to add - see {@link DiscoveryNodes#resolveNodes(String...)}
     */
    public AddVotingConfigExclusionsRequest(String[] nodeDescriptions) {


This isn't resolved yet?

...a/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java

server/src/main/java/org/elasticsearch/cluster/coordination/JoinTaskExecutor.java

.../elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequestTests.java

server/src/test/java/org/elasticsearch/cluster/coordination/NodeJoinTests.java

server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java

server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java

…estability

DaveCTurner · 2020-04-09T10:13:39Z

@elasticmachine ok to test

In elastic#50836 we deprecated the existing voting config exclusions API and added a new one. This commit adjust the docs to match.

…inTaskExecutor.java Co-Authored-By: David Turner <[email protected]>

…nfiguration/AddVotingConfigExclusionsRequest.java Co-Authored-By: David Turner <[email protected]>

…ordinatorTests.java Co-Authored-By: David Turner <[email protected]>

DaveCTurner

LGTM

Today the voting config exclusions API accepts node filters and resolves them to a collection of node IDs against the current cluster membership. This is problematic since we may want to exclude nodes that are not currently members of the cluster. For instance: - if attempting to remove a flaky node from the cluster you cannot reliably exclude it from the voting configuration since it may not reliably be a member of the cluster - if `cluster.auto_shrink_voting_configuration: false` then naively shrinking the cluster will remove some nodes but will leaving their node IDs in the voting configuration. The only way to clean up the voting configuration is to grow the cluster back to its original size (potentially replacing some of the voting configuration) and then use the exclusions API. This commit adds an alternative API that accepts node names and node IDs but not node filters in general, and deprecates the current node-filters-based API. Relates elastic#47990.

Today the voting config exclusions API accepts node filters and resolves them to a collection of node IDs against the current cluster membership. This is problematic since we may want to exclude nodes that are not currently members of the cluster. For instance: - if attempting to remove a flaky node from the cluster you cannot reliably exclude it from the voting configuration since it may not reliably be a member of the cluster - if `cluster.auto_shrink_voting_configuration: false` then naively shrinking the cluster will remove some nodes but will leaving their node IDs in the voting configuration. The only way to clean up the voting configuration is to grow the cluster back to its original size (potentially replacing some of the voting configuration) and then use the exclusions API. This commit adds an alternative API that accepts node names and node IDs but not node filters in general, and deprecates the current node-filters-based API. Relates #47990. Backport of #50836 to 7.x. Co-authored-by: zacharymorn <[email protected]>

In #50836 we deprecated the existing voting config exclusions API and added a new one. This commit adjust the docs to match.

DaveCTurner · 2020-04-22T12:35:14Z

I will follow up with a PR to remove the deprecated API in the next few days.

The use of node filters for excluding nodes from the voting configuration was deprecated in #50836; this commit removes support for node filters in this API. Closes #47990

Voting config exclusions should work with absent nodes

c3d4615

cbuescher added the :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. label Jan 10, 2020

DaveCTurner self-requested a review January 10, 2020 10:29

DaveCTurner requested changes Jan 10, 2020

View reviewed changes

zacharymorn added 2 commits January 17, 2020 11:24

Merge branch 'master' into issue-47990

7881e7b

Add new APIs to add voting config exclusion just based on node id or …

39eb2a1

…name

DaveCTurner requested changes Jan 28, 2020

View reviewed changes

Address feedback comment

c3eb5a1

DaveCTurner requested changes Mar 2, 2020

View reviewed changes

zacharymorn added 6 commits March 3, 2020 23:23

Address feedback

ad41573

Address comment

b085c2c

Update nodeId for VotingConfigExclusion when node with matching name …

57cbc47

…joins

Add test cases to AddVotingConfigExclusionsRequestTests

e08a8a1

Add assertion for voting config exclusion in cluster state

d325123

Add test cases to TransportAddVotingConfigExclusionsActionTests

bede9a0

DaveCTurner mentioned this pull request Mar 9, 2020

TransportTasksAction does not respect an empty set of nodes #53281

Closed

zacharymorn added 2 commits March 9, 2020 10:54

Add test to NodeJoinTests

fb337f2

Add test to CooridnatorTests

5e28294

zacharymorn added 2 commits March 9, 2020 22:35

Inline deprecation message

49f749c

Merge branch 'master' into issue-47990

a174c7c

zacharymorn requested a review from DaveCTurner March 19, 2020 01:18

DaveCTurner mentioned this pull request Mar 24, 2020

Elasticsearch rest-api-spec is missing the Voting Config Exclusions API #48131

Closed

DaveCTurner reviewed Mar 25, 2020

View reviewed changes

zacharymorn added 2 commits March 25, 2020 21:40

Address some comments that can be fixed quickly

2d37e5c

Make Coordinator#validVotingConfigExclusionState package static for t…

0fb29c3

…estability

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Apr 9, 2020

Adjust docs for voting config exclusions API

8fda24d

In elastic#50836 we deprecated the existing voting config exclusions API and added a new one. This commit adjust the docs to match.

DaveCTurner mentioned this pull request Apr 9, 2020

Adjust docs for voting config exclusions API #55006

Merged

zacharymorn and others added 5 commits April 9, 2020 18:47

Update server/src/main/java/org/elasticsearch/cluster/coordination/Jo…

6d6353e

…inTaskExecutor.java Co-Authored-By: David Turner <[email protected]>

Update server/src/main/java/org/elasticsearch/action/admin/cluster/co…

99a584d

…nfiguration/AddVotingConfigExclusionsRequest.java Co-Authored-By: David Turner <[email protected]>

Update server/src/test/java/org/elasticsearch/cluster/coordination/Co…

89ab437

…ordinatorTests.java Co-Authored-By: David Turner <[email protected]>

Address feedback to use Map<String, DiscoveryNode> for existing nodes

fd64f4b

Fix checkstyle

51f74bf

DaveCTurner approved these changes Apr 16, 2020

View reviewed changes

DaveCTurner merged commit 6b299d4 into elastic:master Apr 16, 2020

DaveCTurner added >deprecation >enhancement v7.8.0 v8.0.0 backport pending labels Apr 16, 2020

DaveCTurner mentioned this pull request Apr 16, 2020

Voting config exclusions should work with absent nodes #55291

Merged

DaveCTurner removed the backport pending label Apr 16, 2020

DaveCTurner added a commit that referenced this pull request Apr 20, 2020

Adjust docs for voting config exclusions API (#55006)

10ab397

In #50836 we deprecated the existing voting config exclusions API and added a new one. This commit adjust the docs to match.

DaveCTurner added a commit that referenced this pull request Apr 20, 2020

Adjust docs for voting config exclusions API (#55006)

8e618fd

In #50836 we deprecated the existing voting config exclusions API and added a new one. This commit adjust the docs to match.

DaveCTurner mentioned this pull request Apr 23, 2020

Remove node filters for voting config exclusions #55673

Merged

russcam mentioned this pull request May 29, 2020

7.8.0 Meta ticket elastic/elasticsearch-net#4718

Closed

17 tasks

sebgl mentioned this pull request Nov 16, 2020

Do not try to exclude a master node that never existed elastic/cloud-on-k8s#1986

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Voting config exclusions should work with absent nodes #50836

Voting config exclusions should work with absent nodes #50836

zacharymorn commented Jan 10, 2020

elasticmachine commented Jan 10, 2020

DaveCTurner left a comment

zacharymorn commented Jan 13, 2020

zacharymorn commented Jan 20, 2020

DaveCTurner left a comment

zacharymorn commented Feb 28, 2020

DaveCTurner left a comment

DaveCTurner Mar 2, 2020

zacharymorn Mar 4, 2020

DaveCTurner Mar 4, 2020

zacharymorn Mar 5, 2020

DaveCTurner Mar 25, 2020

zacharymorn Mar 27, 2020

DaveCTurner Apr 8, 2020

zacharymorn Apr 9, 2020

DaveCTurner Mar 2, 2020

zacharymorn Mar 4, 2020

zacharymorn Mar 7, 2020

DaveCTurner Apr 8, 2020

zacharymorn commented Mar 10, 2020

DaveCTurner left a comment

DaveCTurner Mar 25, 2020

DaveCTurner commented Apr 9, 2020

DaveCTurner left a comment

DaveCTurner commented Apr 22, 2020

	public AddVotingConfigExclusionsRequest(String[] nodeDescriptions) {
	public AddVotingConfigExclusionsRequest(String... nodeNames) {

Voting config exclusions should work with absent nodes #50836

Voting config exclusions should work with absent nodes #50836

Conversation

zacharymorn commented Jan 10, 2020

elasticmachine commented Jan 10, 2020

DaveCTurner left a comment

Choose a reason for hiding this comment

zacharymorn commented Jan 13, 2020

zacharymorn commented Jan 20, 2020

DaveCTurner left a comment

Choose a reason for hiding this comment

zacharymorn commented Feb 28, 2020

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharymorn commented Mar 10, 2020

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Apr 9, 2020

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner commented Apr 22, 2020