diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java index c5ca2dffe6c57..c7e9227c766ac 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequest.java @@ -18,70 +18,142 @@ */ package org.elasticsearch.action.admin.cluster.configuration; +import org.apache.logging.log4j.LogManager; +import org.elasticsearch.Version; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.support.master.MasterNodeRequest; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.coordination.CoordinationMetadata.VotingConfigExclusion; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodes; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.logging.DeprecationLogger; import java.io.IOException; import java.util.Arrays; +import java.util.HashSet; +import java.util.Map; import java.util.Set; +import java.util.function.Function; import java.util.stream.Collectors; +import java.util.stream.StreamSupport; /** * A request to add voting config exclusions for certain master-eligible nodes, and wait for these nodes to be removed from the voting * configuration. */ public class AddVotingConfigExclusionsRequest extends MasterNodeRequest { + public static final String DEPRECATION_MESSAGE = "nodeDescription is deprecated and will be removed, use nodeIds or nodeNames instead"; + private static final DeprecationLogger deprecationLogger = new DeprecationLogger( + LogManager.getLogger(AddVotingConfigExclusionsRequest.class)); private final String[] nodeDescriptions; + private final String[] nodeIds; + private final String[] nodeNames; private final TimeValue timeout; /** - * Construct a request to add voting config exclusions for master-eligible nodes matching the given descriptions, and wait for a + * Construct a request to add voting config exclusions for master-eligible nodes matching the given node names, and wait for a * default 30 seconds for these exclusions to take effect, removing the nodes from the voting configuration. - * @param nodeDescriptions Descriptions of the nodes to add - see {@link DiscoveryNodes#resolveNodes(String...)} + * @param nodeNames Names of the nodes to add - see {@link AddVotingConfigExclusionsRequest#resolveVotingConfigExclusions(ClusterState)} */ - public AddVotingConfigExclusionsRequest(String[] nodeDescriptions) { - this(nodeDescriptions, TimeValue.timeValueSeconds(30)); + public AddVotingConfigExclusionsRequest(String... nodeNames) { + this(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, nodeNames, TimeValue.timeValueSeconds(30)); } /** * Construct a request to add voting config exclusions for master-eligible nodes matching the given descriptions, and wait for these * nodes to be removed from the voting configuration. * @param nodeDescriptions Descriptions of the nodes whose exclusions to add - see {@link DiscoveryNodes#resolveNodes(String...)}. + * @param nodeIds Ids of the nodes whose exclusions to add - see + * {@link AddVotingConfigExclusionsRequest#resolveVotingConfigExclusions(ClusterState)}. + * @param nodeNames Names of the nodes whose exclusions to add - see + * {@link AddVotingConfigExclusionsRequest#resolveVotingConfigExclusions(ClusterState)}. * @param timeout How long to wait for the added exclusions to take effect and be removed from the voting configuration. */ - public AddVotingConfigExclusionsRequest(String[] nodeDescriptions, TimeValue timeout) { + public AddVotingConfigExclusionsRequest(String[] nodeDescriptions, String[] nodeIds, String[] nodeNames, TimeValue timeout) { if (timeout.compareTo(TimeValue.ZERO) < 0) { throw new IllegalArgumentException("timeout [" + timeout + "] must be non-negative"); } + + if (noneOrMoreThanOneIsSet(nodeDescriptions, nodeIds, nodeNames)) { + throw new IllegalArgumentException("Please set node identifiers correctly. " + + "One and only one of [node_name], [node_names] and [node_ids] has to be set"); + } + + if (nodeDescriptions.length > 0) { + deprecationLogger.deprecatedAndMaybeLog("voting_config_exclusion", DEPRECATION_MESSAGE); + } + this.nodeDescriptions = nodeDescriptions; + this.nodeIds = nodeIds; + this.nodeNames = nodeNames; this.timeout = timeout; } public AddVotingConfigExclusionsRequest(StreamInput in) throws IOException { super(in); nodeDescriptions = in.readStringArray(); + if (in.getVersion().onOrAfter(Version.V_8_0_0)) { + nodeIds = in.readStringArray(); + nodeNames = in.readStringArray(); + } else { + nodeIds = Strings.EMPTY_ARRAY; + nodeNames = Strings.EMPTY_ARRAY; + } timeout = in.readTimeValue(); + + if (nodeDescriptions.length > 0) { + deprecationLogger.deprecatedAndMaybeLog("voting_config_exclusion", + "nodeDescription is deprecated and will be removed, use nodeIds or nodeNames instead"); + } + } Set resolveVotingConfigExclusions(ClusterState currentState) { final DiscoveryNodes allNodes = currentState.nodes(); - final Set resolvedNodes = Arrays.stream(allNodes.resolveNodes(nodeDescriptions)) - .map(allNodes::get).filter(DiscoveryNode::isMasterNode).map(VotingConfigExclusion::new).collect(Collectors.toSet()); - - if (resolvedNodes.isEmpty()) { - throw new IllegalArgumentException("add voting config exclusions request for " + Arrays.asList(nodeDescriptions) - + " matched no master-eligible nodes"); + Set newVotingConfigExclusions = new HashSet<>(); + + if (nodeDescriptions.length >= 1) { + newVotingConfigExclusions = Arrays.stream(allNodes.resolveNodes(nodeDescriptions)).map(allNodes::get) + .filter(DiscoveryNode::isMasterNode).map(VotingConfigExclusion::new).collect(Collectors.toSet()); + + if (newVotingConfigExclusions.isEmpty()) { + throw new IllegalArgumentException("add voting config exclusions request for " + Arrays.asList(nodeDescriptions) + + " matched no master-eligible nodes"); + } + } else if (nodeIds.length >= 1) { + for (String nodeId : nodeIds) { + if (allNodes.nodeExists(nodeId)) { + DiscoveryNode discoveryNode = allNodes.get(nodeId); + if (discoveryNode.isMasterNode()) { + newVotingConfigExclusions.add(new VotingConfigExclusion(discoveryNode)); + } + } else { + newVotingConfigExclusions.add(new VotingConfigExclusion(nodeId, VotingConfigExclusion.MISSING_VALUE_MARKER)); + } + } + } else { + assert nodeNames.length >= 1; + Map existingNodes = StreamSupport.stream(allNodes.spliterator(), false) + .collect(Collectors.toMap(DiscoveryNode::getName, Function.identity())); + + for (String nodeName : nodeNames) { + if (existingNodes.containsKey(nodeName)){ + DiscoveryNode discoveryNode = existingNodes.get(nodeName); + if (discoveryNode.isMasterNode()) { + newVotingConfigExclusions.add(new VotingConfigExclusion(discoveryNode)); + } + } else { + newVotingConfigExclusions.add(new VotingConfigExclusion(VotingConfigExclusion.MISSING_VALUE_MARKER, nodeName)); + } + } } - resolvedNodes.removeIf(n -> currentState.getVotingConfigExclusions().contains(n)); - return resolvedNodes; + newVotingConfigExclusions.removeIf(n -> currentState.getVotingConfigExclusions().contains(n)); + return newVotingConfigExclusions; } Set resolveVotingConfigExclusionsAndCheckMaximum(ClusterState currentState, int maxExclusionsCount, @@ -99,6 +171,16 @@ Set resolveVotingConfigExclusionsAndCheckMaximum(ClusterS return resolvedExclusions; } + private boolean noneOrMoreThanOneIsSet(String[] deprecatedNodeDescription, String[] nodeIds, String[] nodeNames) { + if (deprecatedNodeDescription.length > 0) { + return nodeIds.length > 0 || nodeNames.length > 0; + } else if (nodeIds.length > 0) { + return nodeNames.length > 0; + } else { + return nodeNames.length > 0 == false; + } + } + /** * @return descriptions of the nodes for whom to add voting config exclusions. */ @@ -106,6 +188,20 @@ public String[] getNodeDescriptions() { return nodeDescriptions; } + /** + * @return ids of the nodes for whom to add voting config exclusions. + */ + public String[] getNodeIds() { + return nodeIds; + } + + /** + * @return names of the nodes for whom to add voting config exclusions. + */ + public String[] getNodeNames() { + return nodeNames; + } + /** * @return how long to wait after adding the exclusions for the nodes to be removed from the voting configuration. */ @@ -122,14 +218,20 @@ public ActionRequestValidationException validate() { public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); out.writeStringArray(nodeDescriptions); + if (out.getVersion().onOrAfter(Version.V_8_0_0)) { + out.writeStringArray(nodeIds); + out.writeStringArray(nodeNames); + } out.writeTimeValue(timeout); } @Override public String toString() { return "AddVotingConfigExclusionsRequest{" + - "nodeDescriptions=" + Arrays.asList(nodeDescriptions) + - ", timeout=" + timeout + + "nodeDescriptions=" + Arrays.asList(nodeDescriptions) + ", " + + "nodeIds=" + Arrays.asList(nodeIds) + ", " + + "nodeNames=" + Arrays.asList(nodeNames) + ", " + + "timeout=" + timeout + '}'; } } diff --git a/server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationMetadata.java b/server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationMetadata.java index d542cab12b9ba..ba4baafb05a4b 100644 --- a/server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationMetadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationMetadata.java @@ -229,6 +229,7 @@ public CoordinationMetadata build() { } public static class VotingConfigExclusion implements Writeable, ToXContentFragment { + public static final String MISSING_VALUE_MARKER = "_absent_"; private final String nodeId; private final String nodeName; diff --git a/server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java b/server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java index 959b38e61517a..d33bed506482f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java +++ b/server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java @@ -868,6 +868,7 @@ assert localNodeMayWinElection(getLastAcceptedState()) : // Package-private for testing ClusterState improveConfiguration(ClusterState clusterState) { assert Thread.holdsLock(mutex) : "Coordinator mutex not held"; + assert validVotingConfigExclusionState(clusterState) : clusterState; // exclude any nodes whose ID is in the voting config exclusions list ... final Stream excludedNodeIds = clusterState.getVotingConfigExclusions().stream().map(VotingConfigExclusion::getNodeId); @@ -895,6 +896,31 @@ ClusterState improveConfiguration(ClusterState clusterState) { return clusterState; } + /* + * Valid Voting Configuration Exclusion state criteria: + * 1. Every voting config exclusion with an ID of _absent_ should not match any nodes currently in the cluster by name + * 2. Every voting config exclusion with a name of _absent_ should not match any nodes currently in the cluster by ID + */ + static boolean validVotingConfigExclusionState(ClusterState clusterState) { + Set votingConfigExclusions = clusterState.getVotingConfigExclusions(); + Set nodeNamesWithAbsentId = votingConfigExclusions.stream() + .filter(e -> e.getNodeId().equals(VotingConfigExclusion.MISSING_VALUE_MARKER)) + .map(VotingConfigExclusion::getNodeName) + .collect(Collectors.toSet()); + Set nodeIdsWithAbsentName = votingConfigExclusions.stream() + .filter(e -> e.getNodeName().equals(VotingConfigExclusion.MISSING_VALUE_MARKER)) + .map(VotingConfigExclusion::getNodeId) + .collect(Collectors.toSet()); + for (DiscoveryNode node : clusterState.getNodes()) { + if (node.isMasterNode() && + (nodeIdsWithAbsentName.contains(node.getId()) || nodeNamesWithAbsentId.contains(node.getName()))) { + return false; + } + } + + return true; + } + private AtomicBoolean reconfigurationTaskScheduled = new AtomicBoolean(); private void scheduleReconfigurationIfNeeded() { diff --git a/server/src/main/java/org/elasticsearch/cluster/coordination/JoinTaskExecutor.java b/server/src/main/java/org/elasticsearch/cluster/coordination/JoinTaskExecutor.java index ee4db3214892d..5612be90a851d 100644 --- a/server/src/main/java/org/elasticsearch/cluster/coordination/JoinTaskExecutor.java +++ b/server/src/main/java/org/elasticsearch/cluster/coordination/JoinTaskExecutor.java @@ -37,8 +37,12 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.Set; import java.util.function.BiConsumer; +import java.util.stream.Collectors; import static org.elasticsearch.gateway.GatewayService.STATE_NOT_RECOVERED_BLOCK; @@ -124,6 +128,7 @@ public ClusterTasksResult execute(ClusterState currentState, List jo // we only enforce major version transitions on a fully formed clusters final boolean enforceMajorVersion = currentState.getBlocks().hasGlobalBlock(STATE_NOT_RECOVERED_BLOCK) == false; // processing any joins + Map joiniedNodeNameIds = new HashMap<>(); for (final Task joinTask : joiningNodes) { if (joinTask.isBecomeMasterTask() || joinTask.isFinishElectionTask()) { // noop @@ -143,6 +148,9 @@ public ClusterTasksResult execute(ClusterState currentState, List jo nodesChanged = true; minClusterNodeVersion = Version.min(minClusterNodeVersion, node.getVersion()); maxClusterNodeVersion = Version.max(maxClusterNodeVersion, node.getVersion()); + if (node.isMasterNode()) { + joiniedNodeNameIds.put(node.getName(), node.getId()); + } } catch (IllegalArgumentException | IllegalStateException e) { results.failure(joinTask, e); continue; @@ -150,11 +158,37 @@ public ClusterTasksResult execute(ClusterState currentState, List jo } results.success(joinTask); } + if (nodesChanged) { rerouteService.reroute("post-join reroute", Priority.HIGH, ActionListener.wrap( r -> logger.trace("post-join reroute completed"), e -> logger.debug("post-join reroute failed", e))); + if (joiniedNodeNameIds.isEmpty() == false) { + Set currentVotingConfigExclusions = currentState.getVotingConfigExclusions(); + Set newVotingConfigExclusions = currentVotingConfigExclusions.stream() + .map(e -> { + // Update nodeId in VotingConfigExclusion when a new node with excluded node name joins + if (CoordinationMetadata.VotingConfigExclusion.MISSING_VALUE_MARKER.equals(e.getNodeId()) && + joiniedNodeNameIds.containsKey(e.getNodeName())) { + return new CoordinationMetadata.VotingConfigExclusion(joiniedNodeNameIds.get(e.getNodeName()), e.getNodeName()); + } else { + return e; + } + }).collect(Collectors.toSet()); + + // if VotingConfigExclusions did get updated + if (newVotingConfigExclusions.equals(currentVotingConfigExclusions) == false) { + CoordinationMetadata.Builder coordMetadataBuilder = CoordinationMetadata.builder(currentState.coordinationMetadata()) + .clearVotingConfigExclusions(); + newVotingConfigExclusions.forEach(coordMetadataBuilder::addVotingConfigExclusion); + Metadata newMetadata = Metadata.builder(currentState.metadata()) + .coordinationMetadata(coordMetadataBuilder.build()).build(); + return results.build(allocationService.adaptAutoExpandReplicas(newState.nodes(nodesBuilder) + .metadata(newMetadata).build())); + } + } + return results.build(allocationService.adaptAutoExpandReplicas(newState.nodes(nodesBuilder).build())); } else { // we must return a new cluster state instance to force publishing. This is important diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestAddVotingConfigExclusionAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestAddVotingConfigExclusionAction.java index f158898bbee65..01355c5e50e71 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestAddVotingConfigExclusionAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestAddVotingConfigExclusionAction.java @@ -28,14 +28,21 @@ import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.RestToXContentListener; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import java.io.IOException; import java.util.List; import static org.elasticsearch.rest.RestRequest.Method.POST; public class RestAddVotingConfigExclusionAction extends BaseRestHandler { - private static final TimeValue DEFAULT_TIMEOUT = TimeValue.timeValueSeconds(30L); + private static final Logger logger = LogManager.getLogger(RestAddVotingConfigExclusionAction.class); + + private static final String DEPRECATION_MESSAGE = "POST /_cluster/voting_config_exclusions/{node_name} " + + "will be removed in a future version. " + + "Please use POST /_cluster/voting_config_exclusions?node_ids=... " + + "or POST /_cluster/voting_config_exclusions?node_names=... instead."; @Override public String getName() { @@ -44,7 +51,8 @@ public String getName() { @Override public List routes() { - return List.of(new Route(POST, "/_cluster/voting_config_exclusions/{node_name}")); + return List.of(new DeprecatedRoute(POST, "/_cluster/voting_config_exclusions/{node_name}", DEPRECATION_MESSAGE), + new Route(POST, "/_cluster/voting_config_exclusions")); } @Override @@ -58,10 +66,29 @@ protected RestChannelConsumer prepareRequest(final RestRequest request, final No } AddVotingConfigExclusionsRequest resolveVotingConfigExclusionsRequest(final RestRequest request) { - String nodeName = request.param("node_name"); + String deprecatedNodeDescription = null; + String nodeIds = null; + String nodeNames = null; + + if (request.hasParam("node_name")) { + deprecatedNodeDescription = request.param("node_name"); + } + + if (request.hasParam("node_ids")){ + nodeIds = request.param("node_ids"); + } + + if (request.hasParam("node_names")){ + nodeNames = request.param("node_names"); + } + return new AddVotingConfigExclusionsRequest( - Strings.splitStringByCommaToArray(nodeName), + Strings.splitStringByCommaToArray(deprecatedNodeDescription), + Strings.splitStringByCommaToArray(nodeIds), + Strings.splitStringByCommaToArray(nodeNames), TimeValue.parseTimeValue(request.param("timeout"), DEFAULT_TIMEOUT, getClass().getSimpleName() + ".timeout") ); } + + } diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequestTests.java index dc6d0569ce287..ead56d7de7fcd 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/configuration/AddVotingConfigExclusionsRequestTests.java @@ -27,6 +27,7 @@ import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodeRole; import org.elasticsearch.cluster.node.DiscoveryNodes.Builder; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.test.ESTestCase; @@ -35,23 +36,48 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.emptySet; -import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; public class AddVotingConfigExclusionsRequestTests extends ESTestCase { + private static final String NODE_IDENTIFIERS_INCORRECTLY_SET_MSG = "Please set node identifiers correctly. " + + "One and only one of [node_name], [node_names] and [node_ids] has to be set"; + public void testSerialization() throws IOException { - int descriptionCount = between(0, 5); + int descriptionCount = between(1, 5); String[] descriptions = new String[descriptionCount]; for (int i = 0; i < descriptionCount; i++) { descriptions[i] = randomAlphaOfLength(10); } TimeValue timeout = TimeValue.timeValueMillis(between(0, 30000)); - final AddVotingConfigExclusionsRequest originalRequest = new AddVotingConfigExclusionsRequest(descriptions, timeout); + final AddVotingConfigExclusionsRequest originalRequest = new AddVotingConfigExclusionsRequest(descriptions, Strings.EMPTY_ARRAY, + Strings.EMPTY_ARRAY, timeout); final AddVotingConfigExclusionsRequest deserialized = copyWriteable(originalRequest, writableRegistry(), AddVotingConfigExclusionsRequest::new); assertThat(deserialized.getNodeDescriptions(), equalTo(originalRequest.getNodeDescriptions())); assertThat(deserialized.getTimeout(), equalTo(originalRequest.getTimeout())); + assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); + } + + public void testSerializationForNodeIdOrNodeName() throws IOException { + AddVotingConfigExclusionsRequest originalRequest = new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, + new String[]{"nodeId1", "nodeId2"}, Strings.EMPTY_ARRAY, TimeValue.ZERO); + AddVotingConfigExclusionsRequest deserialized = copyWriteable(originalRequest, writableRegistry(), + AddVotingConfigExclusionsRequest::new); + + assertThat(deserialized.getNodeDescriptions(), equalTo(originalRequest.getNodeDescriptions())); + assertThat(deserialized.getNodeIds(), equalTo(originalRequest.getNodeIds())); + assertThat(deserialized.getNodeNames(), equalTo(originalRequest.getNodeNames())); + assertThat(deserialized.getTimeout(), equalTo(originalRequest.getTimeout())); + + originalRequest = new AddVotingConfigExclusionsRequest("nodeName1", "nodeName2"); + deserialized = copyWriteable(originalRequest, writableRegistry(), AddVotingConfigExclusionsRequest::new); + + assertThat(deserialized.getNodeDescriptions(), equalTo(originalRequest.getNodeDescriptions())); + assertThat(deserialized.getNodeIds(), equalTo(originalRequest.getNodeIds())); + assertThat(deserialized.getNodeNames(), equalTo(originalRequest.getNodeNames())); + assertThat(deserialized.getTimeout(), equalTo(originalRequest.getTimeout())); } public void testResolve() { @@ -85,18 +111,163 @@ public void testResolve() { final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")).nodes(new Builder() .add(localNode).add(otherNode1).add(otherNode2).add(otherDataNode).localNodeId(localNode.getId())).build(); - assertThat(makeRequest().resolveVotingConfigExclusions(clusterState), - containsInAnyOrder(localNodeExclusion, otherNode1Exclusion, otherNode2Exclusion)); - assertThat(makeRequest("_all").resolveVotingConfigExclusions(clusterState), + assertThat(makeRequestWithNodeDescriptions("_all").resolveVotingConfigExclusions(clusterState), containsInAnyOrder(localNodeExclusion, otherNode1Exclusion, otherNode2Exclusion)); - assertThat(makeRequest("_local").resolveVotingConfigExclusions(clusterState), + assertThat(makeRequestWithNodeDescriptions("_local").resolveVotingConfigExclusions(clusterState), contains(localNodeExclusion)); - assertThat(makeRequest("other*").resolveVotingConfigExclusions(clusterState), + assertThat(makeRequestWithNodeDescriptions("other*").resolveVotingConfigExclusions(clusterState), containsInAnyOrder(otherNode1Exclusion, otherNode2Exclusion)); assertThat(expectThrows(IllegalArgumentException.class, - () -> makeRequest("not-a-node").resolveVotingConfigExclusions(clusterState)).getMessage(), - equalTo("add voting config exclusions request for [not-a-node] matched no master-eligible nodes")); + () -> makeRequestWithNodeDescriptions("not-a-node").resolveVotingConfigExclusions(clusterState)).getMessage(), + equalTo("add voting config exclusions request for [not-a-node] matched no master-eligible nodes")); + assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); + } + + public void testResolveAllNodeIdentifiersNullOrEmpty() { + assertThat(expectThrows(IllegalArgumentException.class, + () -> new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, + Strings.EMPTY_ARRAY, TimeValue.ZERO)).getMessage(), + equalTo(NODE_IDENTIFIERS_INCORRECTLY_SET_MSG)); + } + + public void testResolveMoreThanOneNodeIdentifiersSet() { + assertThat(expectThrows(IllegalArgumentException.class, + () -> new AddVotingConfigExclusionsRequest(new String[]{"local"}, new String[]{"nodeId"}, + Strings.EMPTY_ARRAY, TimeValue.ZERO)).getMessage(), + equalTo(NODE_IDENTIFIERS_INCORRECTLY_SET_MSG)); + + assertThat(expectThrows(IllegalArgumentException.class, + () -> new AddVotingConfigExclusionsRequest(new String[]{"local"}, Strings.EMPTY_ARRAY, + new String[]{"nodeName"}, TimeValue.ZERO)).getMessage(), + equalTo(NODE_IDENTIFIERS_INCORRECTLY_SET_MSG)); + + assertThat(expectThrows(IllegalArgumentException.class, + () -> new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, new String[]{"nodeId"}, + new String[]{"nodeName"}, TimeValue.ZERO)).getMessage(), + equalTo(NODE_IDENTIFIERS_INCORRECTLY_SET_MSG)); + + assertThat(expectThrows(IllegalArgumentException.class, + () -> new AddVotingConfigExclusionsRequest(new String[]{"local"}, new String[]{"nodeId"}, + new String[]{"nodeName"}, TimeValue.ZERO)).getMessage(), + equalTo(NODE_IDENTIFIERS_INCORRECTLY_SET_MSG)); + } + + public void testResolveByNodeIds() { + final DiscoveryNode node1 = new DiscoveryNode( + "nodeName1", + "nodeId1", + buildNewFakeTransportAddress(), + emptyMap(), + Set.of(DiscoveryNodeRole.MASTER_ROLE), + Version.CURRENT); + final VotingConfigExclusion node1Exclusion = new VotingConfigExclusion(node1); + + final DiscoveryNode node2 = new DiscoveryNode( + "nodeName2", + "nodeId2", + buildNewFakeTransportAddress(), + emptyMap(), + Set.of(DiscoveryNodeRole.MASTER_ROLE), + Version.CURRENT); + final VotingConfigExclusion node2Exclusion = new VotingConfigExclusion(node2); + + final DiscoveryNode node3 = new DiscoveryNode( + "nodeName3", + "nodeId3", + buildNewFakeTransportAddress(), + emptyMap(), + Set.of(DiscoveryNodeRole.MASTER_ROLE), + Version.CURRENT); + + final VotingConfigExclusion unresolvableVotingConfigExclusion = new VotingConfigExclusion("unresolvableNodeId", + VotingConfigExclusion.MISSING_VALUE_MARKER); + + final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")) + .nodes(new Builder().add(node1).add(node2).add(node3).localNodeId(node1.getId())).build(); + + assertThat(new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, new String[]{"nodeId1", "nodeId2"}, + Strings.EMPTY_ARRAY, TimeValue.ZERO).resolveVotingConfigExclusions(clusterState), + containsInAnyOrder(node1Exclusion, node2Exclusion)); + + assertThat(new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, new String[]{"nodeId1", "unresolvableNodeId"}, + Strings.EMPTY_ARRAY, TimeValue.ZERO).resolveVotingConfigExclusions(clusterState), + containsInAnyOrder(node1Exclusion, unresolvableVotingConfigExclusion)); + } + + public void testResolveByNodeNames() { + final DiscoveryNode node1 = new DiscoveryNode("nodeName1", + "nodeId1", + buildNewFakeTransportAddress(), + emptyMap(), + Set.of(DiscoveryNodeRole.MASTER_ROLE), + Version.CURRENT); + final VotingConfigExclusion node1Exclusion = new VotingConfigExclusion(node1); + + final DiscoveryNode node2 = new DiscoveryNode("nodeName2", + "nodeId2", + buildNewFakeTransportAddress(), + emptyMap(), + Set.of(DiscoveryNodeRole.MASTER_ROLE), + Version.CURRENT); + final VotingConfigExclusion node2Exclusion = new VotingConfigExclusion(node2); + + final DiscoveryNode node3 = new DiscoveryNode("nodeName3", + "nodeId3", + buildNewFakeTransportAddress(), + emptyMap(), + Set.of(DiscoveryNodeRole.MASTER_ROLE), + Version.CURRENT); + + final VotingConfigExclusion unresolvableVotingConfigExclusion = new VotingConfigExclusion( + VotingConfigExclusion.MISSING_VALUE_MARKER, "unresolvableNodeName"); + + final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")) + .nodes(new Builder().add(node1).add(node2).add(node3).localNodeId(node1.getId())).build(); + + assertThat(new AddVotingConfigExclusionsRequest("nodeName1", "nodeName2").resolveVotingConfigExclusions(clusterState), + containsInAnyOrder(node1Exclusion, node2Exclusion)); + + assertThat(new AddVotingConfigExclusionsRequest("nodeName1", "unresolvableNodeName").resolveVotingConfigExclusions(clusterState), + containsInAnyOrder(node1Exclusion, unresolvableVotingConfigExclusion)); + } + + public void testResolveRemoveExistingVotingConfigExclusions() { + final DiscoveryNode node1 = new DiscoveryNode("nodeName1", + "nodeId1", + buildNewFakeTransportAddress(), + emptyMap(), + Set.of(DiscoveryNodeRole.MASTER_ROLE), + Version.CURRENT); + + final DiscoveryNode node2 = new DiscoveryNode("nodeName2", + "nodeId2", + buildNewFakeTransportAddress(), + emptyMap(), + Set.of(DiscoveryNodeRole.MASTER_ROLE), + Version.CURRENT); + final VotingConfigExclusion node2Exclusion = new VotingConfigExclusion(node2); + + final DiscoveryNode node3 = new DiscoveryNode("nodeName3", + "nodeId3", + buildNewFakeTransportAddress(), + emptyMap(), + Set.of(DiscoveryNodeRole.MASTER_ROLE), + Version.CURRENT); + + final VotingConfigExclusion existingVotingConfigExclusion = new VotingConfigExclusion(node1); + + Metadata metadata = Metadata.builder() + .coordinationMetadata(CoordinationMetadata.builder() + .addVotingConfigExclusion(existingVotingConfigExclusion).build()) + .build(); + + final ClusterState clusterState = ClusterState.builder(new ClusterName("cluster")).metadata(metadata) + .nodes(new Builder().add(node1).add(node2).add(node3).localNodeId(node1.getId())).build(); + + assertThat(new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, new String[]{"nodeId1", "nodeId2"}, + Strings.EMPTY_ARRAY, TimeValue.ZERO).resolveVotingConfigExclusions(clusterState), + contains(node2Exclusion)); } public void testResolveAndCheckMaximum() { @@ -131,22 +302,18 @@ public void testResolveAndCheckMaximum() { .coordinationMetadata(CoordinationMetadata.builder().addVotingConfigExclusion(otherNode1Exclusion).build())); final ClusterState clusterState = builder.build(); - assertThat(makeRequest().resolveVotingConfigExclusionsAndCheckMaximum(clusterState, 3, "setting.name"), - containsInAnyOrder(localNodeExclusion, otherNode2Exclusion)); - assertThat(makeRequest("_local").resolveVotingConfigExclusionsAndCheckMaximum(clusterState, 2, "setting.name"), + assertThat(makeRequestWithNodeDescriptions("_local").resolveVotingConfigExclusionsAndCheckMaximum(clusterState, 2, "setting.name"), contains(localNodeExclusion)); - - assertThat(expectThrows(IllegalArgumentException.class, - () -> makeRequest().resolveVotingConfigExclusionsAndCheckMaximum(clusterState, 2, "setting.name")).getMessage(), - equalTo("add voting config exclusions request for [] would add [2] exclusions to the existing [1] which would exceed " + - "the maximum of [2] set by [setting.name]")); assertThat(expectThrows(IllegalArgumentException.class, - () -> makeRequest("_local").resolveVotingConfigExclusionsAndCheckMaximum(clusterState, 1, "setting.name")).getMessage(), + () -> makeRequestWithNodeDescriptions("_local").resolveVotingConfigExclusionsAndCheckMaximum(clusterState, 1, "setting.name")) + .getMessage(), equalTo("add voting config exclusions request for [_local] would add [1] exclusions to the existing [1] which would " + "exceed the maximum of [1] set by [setting.name]")); + assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); } - private static AddVotingConfigExclusionsRequest makeRequest(String... descriptions) { - return new AddVotingConfigExclusionsRequest(descriptions); + private static AddVotingConfigExclusionsRequest makeRequestWithNodeDescriptions(String... nodeDescriptions) { + return new AddVotingConfigExclusionsRequest(nodeDescriptions, Strings.EMPTY_ARRAY, + Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30)); } } diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/configuration/TransportAddVotingConfigExclusionsActionTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/configuration/TransportAddVotingConfigExclusionsActionTests.java index 2f1f74fe6b369..29bebc75b7796 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/configuration/TransportAddVotingConfigExclusionsActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/configuration/TransportAddVotingConfigExclusionsActionTests.java @@ -36,6 +36,7 @@ import org.elasticsearch.cluster.node.DiscoveryNodeRole; import org.elasticsearch.cluster.node.DiscoveryNodes.Builder; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; @@ -151,8 +152,7 @@ public void testWithdrawsVoteFromANode() throws InterruptedException { final CountDownLatch countDownLatch = new CountDownLatch(1); clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions()); - transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"other1"}), + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, new AddVotingConfigExclusionsRequest("other1"), expectSuccess(r -> { assertNotNull(r); countDownLatch.countDown(); @@ -168,7 +168,7 @@ public void testWithdrawsVotesFromMultipleNodes() throws InterruptedException { clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions()); transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"other1", "other2"}), + new AddVotingConfigExclusionsRequest("other1", "other2"), expectSuccess(r -> { assertNotNull(r); countDownLatch.countDown(); @@ -184,8 +184,7 @@ public void testWithdrawsVotesFromNodesMatchingWildcard() throws InterruptedExce final CountDownLatch countDownLatch = new CountDownLatch(1); clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions()); - transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"other*"}), + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, makeRequestWithNodeDescriptions("other*"), expectSuccess(r -> { assertNotNull(r); countDownLatch.countDown(); @@ -195,14 +194,14 @@ public void testWithdrawsVotesFromNodesMatchingWildcard() throws InterruptedExce assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(), containsInAnyOrder(otherNode1Exclusion, otherNode2Exclusion)); + assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); } public void testWithdrawsVotesFromAllMasterEligibleNodes() throws InterruptedException { final CountDownLatch countDownLatch = new CountDownLatch(1); clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions()); - transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"_all"}), + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, makeRequestWithNodeDescriptions("_all"), expectSuccess(r -> { assertNotNull(r); countDownLatch.countDown(); @@ -212,14 +211,14 @@ public void testWithdrawsVotesFromAllMasterEligibleNodes() throws InterruptedExc assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(), containsInAnyOrder(localNodeExclusion, otherNode1Exclusion, otherNode2Exclusion)); + assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); } public void testWithdrawsVoteFromLocalNode() throws InterruptedException { final CountDownLatch countDownLatch = new CountDownLatch(1); clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions()); - transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"_local"}), + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, makeRequestWithNodeDescriptions("_local"), expectSuccess(r -> { assertNotNull(r); countDownLatch.countDown(); @@ -229,6 +228,7 @@ public void testWithdrawsVoteFromLocalNode() throws InterruptedException { assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(), contains(localNodeExclusion)); + assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); } public void testReturnsImmediatelyIfVoteAlreadyWithdrawn() throws InterruptedException { @@ -243,8 +243,7 @@ public void testReturnsImmediatelyIfVoteAlreadyWithdrawn() throws InterruptedExc final CountDownLatch countDownLatch = new CountDownLatch(1); // no observer to reconfigure - transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"other1"}, TimeValue.ZERO), + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, new AddVotingConfigExclusionsRequest("other1"), expectSuccess(r -> { assertNotNull(r); countDownLatch.countDown(); @@ -256,12 +255,11 @@ public void testReturnsImmediatelyIfVoteAlreadyWithdrawn() throws InterruptedExc contains(otherNode1Exclusion)); } - public void testReturnsErrorIfNoMatchingNodes() throws InterruptedException { + public void testReturnsErrorIfNoMatchingNodeDescriptions() throws InterruptedException { final CountDownLatch countDownLatch = new CountDownLatch(1); final SetOnce exceptionHolder = new SetOnce<>(); - transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"not-a-node"}), + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, makeRequestWithNodeDescriptions("not-a-node"), expectError(e -> { exceptionHolder.set(e); countDownLatch.countDown(); @@ -273,6 +271,7 @@ public void testReturnsErrorIfNoMatchingNodes() throws InterruptedException { assertThat(rootCause, instanceOf(IllegalArgumentException.class)); assertThat(rootCause.getMessage(), equalTo("add voting config exclusions request for [not-a-node] matched no master-eligible nodes")); + assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); } public void testOnlyMatchesMasterEligibleNodes() throws InterruptedException { @@ -280,7 +279,7 @@ public void testOnlyMatchesMasterEligibleNodes() throws InterruptedException { final SetOnce exceptionHolder = new SetOnce<>(); transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"_all", "master:false"}), + makeRequestWithNodeDescriptions("_all", "master:false"), expectError(e -> { exceptionHolder.set(e); countDownLatch.countDown(); @@ -292,6 +291,74 @@ public void testOnlyMatchesMasterEligibleNodes() throws InterruptedException { assertThat(rootCause, instanceOf(IllegalArgumentException.class)); assertThat(rootCause.getMessage(), equalTo("add voting config exclusions request for [_all, master:false] matched no master-eligible nodes")); + assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); + } + + public void testExcludeAbsentNodesByNodeIds() throws InterruptedException { + final CountDownLatch countDownLatch = new CountDownLatch(1); + + clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions()); + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, + new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, new String[]{"absent_id"}, + Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30)), + expectSuccess(e -> { + countDownLatch.countDown(); + }) + ); + + assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); + assertEquals(Set.of(new VotingConfigExclusion("absent_id", VotingConfigExclusion.MISSING_VALUE_MARKER)), + clusterService.getClusterApplierService().state().getVotingConfigExclusions()); + } + + public void testExcludeExistingNodesByNodeIds() throws InterruptedException { + final CountDownLatch countDownLatch = new CountDownLatch(1); + + clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions()); + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, + new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, new String[]{"other1", "other2"}, + Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30)), + expectSuccess(r -> { + assertNotNull(r); + countDownLatch.countDown(); + }) + ); + + assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); + assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(), + containsInAnyOrder(otherNode1Exclusion, otherNode2Exclusion)); + } + + public void testExcludeAbsentNodesByNodeNames() throws InterruptedException { + final CountDownLatch countDownLatch = new CountDownLatch(1); + + clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions()); + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, new AddVotingConfigExclusionsRequest("absent_node"), + expectSuccess(e -> { + countDownLatch.countDown(); + }) + ); + + assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); + assertEquals(Set.of(new VotingConfigExclusion(VotingConfigExclusion.MISSING_VALUE_MARKER, "absent_node")), + clusterService.getClusterApplierService().state().getVotingConfigExclusions()); + } + + public void testExcludeExistingNodesByNodeNames() throws InterruptedException { + final CountDownLatch countDownLatch = new CountDownLatch(1); + + clusterStateObserver.waitForNextChange(new AdjustConfigurationForExclusions()); + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, + new AddVotingConfigExclusionsRequest("other1", "other2"), + expectSuccess(r -> { + assertNotNull(r); + countDownLatch.countDown(); + }) + ); + + assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); + assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(), + containsInAnyOrder(otherNode1Exclusion, otherNode2Exclusion)); } public void testSucceedsEvenIfAllExclusionsAlreadyAdded() throws InterruptedException { @@ -306,8 +373,7 @@ public void testSucceedsEvenIfAllExclusionsAlreadyAdded() throws InterruptedExce final CountDownLatch countDownLatch = new CountDownLatch(1); - transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"other1"}), + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, new AddVotingConfigExclusionsRequest("other1"), expectSuccess(r -> { assertNotNull(r); countDownLatch.countDown(); @@ -319,6 +385,56 @@ public void testSucceedsEvenIfAllExclusionsAlreadyAdded() throws InterruptedExce contains(otherNode1Exclusion)); } + public void testExcludeByNodeIdSucceedsEvenIfAllExclusionsAlreadyAdded() throws InterruptedException { + final ClusterState state = clusterService.state(); + final ClusterState.Builder builder = builder(state); + builder.metadata(Metadata.builder(state.metadata()). + coordinationMetadata( + CoordinationMetadata.builder(state.coordinationMetadata()) + .addVotingConfigExclusion(otherNode1Exclusion). + build())); + setState(clusterService, builder); + + final CountDownLatch countDownLatch = new CountDownLatch(1); + + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, + new AddVotingConfigExclusionsRequest(Strings.EMPTY_ARRAY, new String[]{"other1"}, + Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30)), + expectSuccess(r -> { + assertNotNull(r); + countDownLatch.countDown(); + }) + ); + + assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); + assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(), + contains(otherNode1Exclusion)); + } + + public void testExcludeByNodeNameSucceedsEvenIfAllExclusionsAlreadyAdded() throws InterruptedException { + final ClusterState state = clusterService.state(); + final ClusterState.Builder builder = builder(state); + builder.metadata(Metadata.builder(state.metadata()). + coordinationMetadata( + CoordinationMetadata.builder(state.coordinationMetadata()) + .addVotingConfigExclusion(otherNode1Exclusion). + build())); + setState(clusterService, builder); + + final CountDownLatch countDownLatch = new CountDownLatch(1); + + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, new AddVotingConfigExclusionsRequest("other1"), + expectSuccess(r -> { + assertNotNull(r); + countDownLatch.countDown(); + }) + ); + + assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); + assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(), + contains(otherNode1Exclusion)); + } + public void testReturnsErrorIfMaximumExclusionCountExceeded() throws InterruptedException { final Metadata.Builder metadataBuilder = Metadata.builder(clusterService.state().metadata()); CoordinationMetadata.Builder coordinationMetadataBuilder = @@ -357,8 +473,7 @@ public void testReturnsErrorIfMaximumExclusionCountExceeded() throws Interrupted final CountDownLatch countDownLatch = new CountDownLatch(1); final SetOnce exceptionHolder = new SetOnce<>(); - transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"other*"}), + transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, makeRequestWithNodeDescriptions("other*"), expectError(e -> { exceptionHolder.set(e); countDownLatch.countDown(); @@ -371,6 +486,7 @@ public void testReturnsErrorIfMaximumExclusionCountExceeded() throws Interrupted assertThat(rootCause.getMessage(), equalTo("add voting config exclusions request for [other*] would add [" + newCount + "] exclusions to the existing [" + existingCount + "] which would exceed the maximum of [" + actualMaximum + "] set by [cluster.max_voting_config_exclusions]")); + assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); } public void testTimesOut() throws InterruptedException { @@ -378,7 +494,8 @@ public void testTimesOut() throws InterruptedException { final SetOnce exceptionHolder = new SetOnce<>(); transportService.sendRequest(localNode, AddVotingConfigExclusionsAction.NAME, - new AddVotingConfigExclusionsRequest(new String[]{"other1"}, TimeValue.timeValueMillis(100)), + new AddVotingConfigExclusionsRequest(new String[]{"other1"}, Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, + TimeValue.timeValueMillis(100)), expectError(e -> { exceptionHolder.set(e); countDownLatch.countDown(); @@ -389,6 +506,8 @@ public void testTimesOut() throws InterruptedException { final Throwable rootCause = exceptionHolder.get().getRootCause(); assertThat(rootCause,instanceOf(ElasticsearchTimeoutException.class)); assertThat(rootCause.getMessage(), startsWith("timed out waiting for voting config exclusions [{other1}")); + assertWarnings(AddVotingConfigExclusionsRequest.DEPRECATION_MESSAGE); + } private TransportResponseHandler expectSuccess( @@ -466,4 +585,10 @@ public void onTimeout(TimeValue timeout) { throw new AssertionError("unexpected timeout"); } } + + private AddVotingConfigExclusionsRequest makeRequestWithNodeDescriptions(String... nodeDescriptions) { + return new AddVotingConfigExclusionsRequest(nodeDescriptions, Strings.EMPTY_ARRAY, + Strings.EMPTY_ARRAY, TimeValue.timeValueSeconds(30)); + } + } diff --git a/server/src/test/java/org/elasticsearch/cluster/MinimumMasterNodesIT.java b/server/src/test/java/org/elasticsearch/cluster/MinimumMasterNodesIT.java index 9ba979f4447c2..006ac159884ba 100644 --- a/server/src/test/java/org/elasticsearch/cluster/MinimumMasterNodesIT.java +++ b/server/src/test/java/org/elasticsearch/cluster/MinimumMasterNodesIT.java @@ -123,7 +123,7 @@ public void testTwoNodesNoMasterBlock() throws Exception { String masterNode = internalCluster().getMasterName(); String otherNode = node1Name.equals(masterNode) ? node2Name : node1Name; logger.info("--> add voting config exclusion for non-master node, to be sure it's not elected"); - client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(new String[]{otherNode})).get(); + client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(otherNode)).get(); logger.info("--> stop master node, no master block should appear"); Settings masterDataPathSettings = internalCluster().dataPathSettings(masterNode); internalCluster().stopRandomNode(InternalTestCluster.nameFilter(masterNode)); @@ -170,7 +170,7 @@ public void testTwoNodesNoMasterBlock() throws Exception { masterNode = internalCluster().getMasterName(); otherNode = node1Name.equals(masterNode) ? node2Name : node1Name; logger.info("--> add voting config exclusion for master node, to be sure it's not elected"); - client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(new String[]{masterNode})).get(); + client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(masterNode)).get(); logger.info("--> stop non-master node, no master block should appear"); Settings otherNodeDataPathSettings = internalCluster().dataPathSettings(otherNode); internalCluster().stopRandomNode(InternalTestCluster.nameFilter(otherNode)); diff --git a/server/src/test/java/org/elasticsearch/cluster/SpecificMasterNodesIT.java b/server/src/test/java/org/elasticsearch/cluster/SpecificMasterNodesIT.java index 0e8f0f3f7e193..fc457c890b19e 100644 --- a/server/src/test/java/org/elasticsearch/cluster/SpecificMasterNodesIT.java +++ b/server/src/test/java/org/elasticsearch/cluster/SpecificMasterNodesIT.java @@ -114,8 +114,7 @@ public void testElectOnlyBetweenMasterNodes() throws Exception { .execute().actionGet().getState().nodes().getMasterNode().getName(), equalTo(masterNodeName)); logger.info("--> closing master node (1)"); - client().execute(AddVotingConfigExclusionsAction.INSTANCE, - new AddVotingConfigExclusionsRequest(new String[]{masterNodeName})).get(); + client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(masterNodeName)).get(); // removing the master from the voting configuration immediately triggers the master to step down assertBusy(() -> { assertThat(internalCluster().nonMasterClient().admin().cluster().prepareState() diff --git a/server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java b/server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java index 1a9efe189f4eb..673d087c9507a 100644 --- a/server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java @@ -32,6 +32,8 @@ import org.elasticsearch.cluster.coordination.Coordinator.Mode; import org.elasticsearch.cluster.metadata.Metadata; import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.cluster.node.DiscoveryNodeRole; +import org.elasticsearch.cluster.node.DiscoveryNodes; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.regex.Regex; @@ -55,6 +57,7 @@ import java.util.function.Function; import java.util.stream.Collectors; +import static java.util.Collections.emptyMap; import static org.elasticsearch.cluster.coordination.AbstractCoordinatorTestCase.Cluster.DEFAULT_DELAY_VARIABILITY; import static org.elasticsearch.cluster.coordination.AbstractCoordinatorTestCase.Cluster.EXTREME_DELAY_VARIABILITY; import static org.elasticsearch.cluster.coordination.Coordinator.Mode.CANDIDATE; @@ -1433,4 +1436,46 @@ public void testDoesNotPerformElectionWhenRestartingFollower() { } } + public void testImproveConfigurationPerformsVotingConfigExclusionStateCheck() { + try (Cluster cluster = new Cluster(1)) { + cluster.runRandomly(); + cluster.stabilise(); + + final Coordinator coordinator = cluster.getAnyLeader().coordinator; + final ClusterState currentState = coordinator.getLastAcceptedState(); + + Set newVotingConfigExclusion1 = new HashSet<>(){{ + add(new CoordinationMetadata.VotingConfigExclusion("resolvableNodeId", + CoordinationMetadata.VotingConfigExclusion.MISSING_VALUE_MARKER)); + }}; + + ClusterState newState1 = buildNewClusterStateWithVotingConfigExclusion(currentState, newVotingConfigExclusion1); + + assertFalse(Coordinator.validVotingConfigExclusionState(newState1)); + + Set newVotingConfigExclusion2 = new HashSet<>(){{ + add(new CoordinationMetadata.VotingConfigExclusion(CoordinationMetadata.VotingConfigExclusion.MISSING_VALUE_MARKER, + "resolvableNodeName")); + }}; + + ClusterState newState2 = buildNewClusterStateWithVotingConfigExclusion(currentState, newVotingConfigExclusion2); + + assertFalse(Coordinator.validVotingConfigExclusionState(newState2)); + } + } + + private ClusterState buildNewClusterStateWithVotingConfigExclusion(ClusterState currentState, + Set newVotingConfigExclusion) { + DiscoveryNodes newNodes = DiscoveryNodes.builder(currentState.nodes()) + .add(new DiscoveryNode("resolvableNodeName", "resolvableNodeId", buildNewFakeTransportAddress(), + emptyMap(), Set.of(DiscoveryNodeRole.MASTER_ROLE), Version.CURRENT)) + .build(); + + CoordinationMetadata.Builder coordMetadataBuilder = CoordinationMetadata.builder(currentState.coordinationMetadata()); + newVotingConfigExclusion.forEach(coordMetadataBuilder::addVotingConfigExclusion); + Metadata newMetadata = Metadata.builder(currentState.metadata()).coordinationMetadata(coordMetadataBuilder.build()).build(); + + return ClusterState.builder(currentState).nodes(newNodes).metadata(newMetadata).build(); + } + } diff --git a/server/src/test/java/org/elasticsearch/cluster/coordination/NodeJoinTests.java b/server/src/test/java/org/elasticsearch/cluster/coordination/NodeJoinTests.java index 6fbe238389957..ed3266a00826a 100644 --- a/server/src/test/java/org/elasticsearch/cluster/coordination/NodeJoinTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/coordination/NodeJoinTests.java @@ -393,6 +393,45 @@ public void testJoinFollowerWithHigherTerm() throws Exception { assertTrue(isLocalNodeElectedMaster()); } + public void testJoinUpdateVotingConfigExclusion() throws Exception { + DiscoveryNode initialNode = newNode(0, true); + long initialTerm = randomLongBetween(1, 10); + long initialVersion = randomLongBetween(1, 10); + + CoordinationMetadata.VotingConfigExclusion votingConfigExclusion = new CoordinationMetadata.VotingConfigExclusion( + CoordinationMetadata.VotingConfigExclusion.MISSING_VALUE_MARKER, "knownNodeName"); + + setupFakeMasterServiceAndCoordinator(initialTerm, buildStateWithVotingConfigExclusion(initialNode, initialTerm, + initialVersion, votingConfigExclusion)); + + DiscoveryNode knownJoiningNode = new DiscoveryNode("knownNodeName", "newNodeId", buildNewFakeTransportAddress(), + emptyMap(), Set.of(DiscoveryNodeRole.MASTER_ROLE), Version.CURRENT); + long newTerm = initialTerm + randomLongBetween(1, 10); + long newerTerm = newTerm + randomLongBetween(1, 10); + + joinNodeAndRun(new JoinRequest(knownJoiningNode, initialTerm, + Optional.of(new Join(knownJoiningNode, initialNode, newerTerm, initialTerm, initialVersion)))); + + assertTrue(MasterServiceTests.discoveryState(masterService).getVotingConfigExclusions().stream().anyMatch(exclusion -> { + return "knownNodeName".equals(exclusion.getNodeName()) && "newNodeId".equals(exclusion.getNodeId()); + })); + } + + private ClusterState buildStateWithVotingConfigExclusion(DiscoveryNode initialNode, + long initialTerm, + long initialVersion, + CoordinationMetadata.VotingConfigExclusion votingConfigExclusion) { + ClusterState initialState = initialState(initialNode, initialTerm, initialVersion, + new VotingConfiguration(Collections.singleton(initialNode.getId()))); + Metadata newMetadata = Metadata.builder(initialState.metadata()) + .coordinationMetadata(CoordinationMetadata.builder(initialState.coordinationMetadata()) + .addVotingConfigExclusion(votingConfigExclusion) + .build()) + .build(); + + return ClusterState.builder(initialState).metadata(newMetadata).build(); + } + private void handleStartJoinFrom(DiscoveryNode node, long term) throws Exception { final RequestHandlerRegistry startJoinHandler = (RequestHandlerRegistry) transport.getRequestHandler(JoinHelper.START_JOIN_ACTION_NAME); diff --git a/server/src/test/java/org/elasticsearch/cluster/coordination/VotingConfigurationIT.java b/server/src/test/java/org/elasticsearch/cluster/coordination/VotingConfigurationIT.java index 21c6ae00c37b9..d65cc671f8ff4 100644 --- a/server/src/test/java/org/elasticsearch/cluster/coordination/VotingConfigurationIT.java +++ b/server/src/test/java/org/elasticsearch/cluster/coordination/VotingConfigurationIT.java @@ -54,8 +54,7 @@ public void testAbdicateAfterVotingConfigExclusionAdded() throws ExecutionExcept final String originalMaster = internalCluster().getMasterName(); logger.info("--> excluding master node {}", originalMaster); - client().execute(AddVotingConfigExclusionsAction.INSTANCE, - new AddVotingConfigExclusionsRequest(new String[]{originalMaster})).get(); + client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(originalMaster)).get(); client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).get(); assertNotEquals(originalMaster, internalCluster().getMasterName()); } diff --git a/server/src/test/java/org/elasticsearch/gateway/RecoveryFromGatewayIT.java b/server/src/test/java/org/elasticsearch/gateway/RecoveryFromGatewayIT.java index d20611681f5d9..2918dc1fe931b 100644 --- a/server/src/test/java/org/elasticsearch/gateway/RecoveryFromGatewayIT.java +++ b/server/src/test/java/org/elasticsearch/gateway/RecoveryFromGatewayIT.java @@ -309,7 +309,7 @@ public void testTwoNodeFirstNodeCleared() throws Exception { Map primaryTerms = assertAndCapturePrimaryTerms(null); - client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(new String[]{firstNode})).get(); + client().execute(AddVotingConfigExclusionsAction.INSTANCE, new AddVotingConfigExclusionsRequest(firstNode)).get(); internalCluster().fullRestart(new RestartCallback() { @Override diff --git a/server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestAddVotingConfigExclusionActionTests.java b/server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestAddVotingConfigExclusionActionTests.java index 538c57b035e01..2713f062b4573 100644 --- a/server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestAddVotingConfigExclusionActionTests.java +++ b/server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestAddVotingConfigExclusionActionTests.java @@ -20,6 +20,7 @@ package org.elasticsearch.rest.action.admin.cluster; import org.elasticsearch.action.admin.cluster.configuration.AddVotingConfigExclusionsRequest; +import org.elasticsearch.common.Strings; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.test.rest.FakeRestRequest; import org.elasticsearch.test.rest.RestActionTestCase; @@ -50,5 +51,41 @@ public void testResolveVotingConfigExclusionsRequest() { AddVotingConfigExclusionsRequest addVotingConfigExclusionsRequest = action.resolveVotingConfigExclusionsRequest(deprecatedRequest); String[] expected = {"node-1","node-2", "node-3"}; assertArrayEquals(expected, addVotingConfigExclusionsRequest.getNodeDescriptions()); + assertArrayEquals(Strings.EMPTY_ARRAY, addVotingConfigExclusionsRequest.getNodeIds()); + assertArrayEquals(Strings.EMPTY_ARRAY, addVotingConfigExclusionsRequest.getNodeNames()); + assertWarnings("nodeDescription is deprecated and will be removed, use nodeIds or nodeNames instead"); } + + public void testResolveVotingConfigExclusionsRequestNodeIds() { + Map params = new HashMap<>(); + params.put("node_ids", "node-1,node-2,node-3"); + RestRequest request = new FakeRestRequest.Builder(xContentRegistry()) + .withMethod(RestRequest.Method.PUT) + .withPath("/_cluster/voting_config_exclusions") + .withParams(params) + .build(); + + AddVotingConfigExclusionsRequest addVotingConfigExclusionsRequest = action.resolveVotingConfigExclusionsRequest(request); + String[] expected = {"node-1","node-2", "node-3"}; + assertArrayEquals(Strings.EMPTY_ARRAY, addVotingConfigExclusionsRequest.getNodeDescriptions()); + assertArrayEquals(expected, addVotingConfigExclusionsRequest.getNodeIds()); + assertArrayEquals(Strings.EMPTY_ARRAY, addVotingConfigExclusionsRequest.getNodeNames()); + } + + public void testResolveVotingConfigExclusionsRequestNodeNames() { + Map params = new HashMap<>(); + params.put("node_names", "node-1,node-2,node-3"); + RestRequest request = new FakeRestRequest.Builder(xContentRegistry()) + .withMethod(RestRequest.Method.PUT) + .withPath("/_cluster/voting_config_exclusions") + .withParams(params) + .build(); + + AddVotingConfigExclusionsRequest addVotingConfigExclusionsRequest = action.resolveVotingConfigExclusionsRequest(request); + String[] expected = {"node-1","node-2", "node-3"}; + assertArrayEquals(Strings.EMPTY_ARRAY, addVotingConfigExclusionsRequest.getNodeDescriptions()); + assertArrayEquals(Strings.EMPTY_ARRAY, addVotingConfigExclusionsRequest.getNodeIds()); + assertArrayEquals(expected, addVotingConfigExclusionsRequest.getNodeNames()); + } + } diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java index 19ea9ffdd3112..1109c6e16ed34 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -1655,7 +1655,7 @@ private NodeAndClient removeNode(NodeAndClient nodeAndClient) { private Set excludeMasters(Collection nodeAndClients) { assert Thread.holdsLock(this); - final Set excludedNodeIds = new HashSet<>(); + final Set excludedNodeNames = new HashSet<>(); if (autoManageMasterNodes && nodeAndClients.size() > 0) { final long currentMasters = nodes.values().stream().filter(NodeAndClient::isMasterEligible).count(); @@ -1667,19 +1667,19 @@ private Set excludeMasters(Collection nodeAndClients) { // However, we do not yet have a way to be sure there's a majority left, because the voting configuration may not yet have // been updated when the previous nodes shut down, so we must always explicitly withdraw votes. // TODO add cluster health API to check that voting configuration is optimal so this isn't always needed - nodeAndClients.stream().filter(NodeAndClient::isMasterEligible).map(NodeAndClient::getName).forEach(excludedNodeIds::add); - assert excludedNodeIds.size() == stoppingMasters; + nodeAndClients.stream().filter(NodeAndClient::isMasterEligible).map(NodeAndClient::getName).forEach(excludedNodeNames::add); + assert excludedNodeNames.size() == stoppingMasters; - logger.info("adding voting config exclusions {} prior to restart/shutdown", excludedNodeIds); + logger.info("adding voting config exclusions {} prior to restart/shutdown", excludedNodeNames); try { client().execute(AddVotingConfigExclusionsAction.INSTANCE, - new AddVotingConfigExclusionsRequest(excludedNodeIds.toArray(Strings.EMPTY_ARRAY))).get(); + new AddVotingConfigExclusionsRequest(excludedNodeNames.toArray(Strings.EMPTY_ARRAY))).get(); } catch (InterruptedException | ExecutionException e) { throw new AssertionError("unexpected", e); } } } - return excludedNodeIds; + return excludedNodeNames; } private void removeExclusions(Set excludedNodeIds) {