diff --git a/docs/changelog/84227.yaml b/docs/changelog/84227.yaml new file mode 100644 index 0000000000000..b0449211cb1b7 --- /dev/null +++ b/docs/changelog/84227.yaml @@ -0,0 +1,5 @@ +pr: 84227 +summary: Enforce external id uniqueness during `DesiredNode` construction +area: Distributed +type: bug +issues: [] diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.desired_nodes/10_basic.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.desired_nodes/10_basic.yml index 804a25bf67e63..c17c3268eff30 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.desired_nodes/10_basic.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.desired_nodes/10_basic.yml @@ -9,6 +9,9 @@ teardown: _internal.delete_desired_nodes: {} --- "Test update desired nodes": + - skip: + reason: "contains is a newly added assertion" + features: contains - do: cluster.state: {} @@ -49,15 +52,17 @@ teardown: - do: _internal.get_desired_nodes: {} - - match: - $body: - history_id: "test" - version: 2 - nodes: - - { settings: { node: { name: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } - - { settings: { node: { name: "instance-000188" } }, processors: 16, memory: "128gb", storage: "1tb", node_version: $es_version } + + - match: { history_id: "test" } + - match: { version: 2 } + - length: { nodes: 2 } + - contains: { nodes: { settings: { node: { name: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } } + - contains: { nodes: { settings: { node: { name: "instance-000188" } }, processors: 16, memory: "128gb", storage: "1tb", node_version: $es_version } } --- "Test update move to a new history id": + - skip: + reason: "contains is a newly added assertion" + features: contains - do: cluster.state: {} @@ -97,13 +102,11 @@ teardown: - do: _internal.get_desired_nodes: {} - - match: - $body: - history_id: "new_history" - version: 1 - nodes: - - { settings: { node: { external_id: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } - - { settings: { node: { external_id: "instance-000188" } }, processors: 16, memory: "128gb", storage: "1tb", node_version: $es_version } + - match: { history_id: "new_history" } + - match: { version: 1 } + - length: { nodes: 2 } + - contains: { nodes: { settings: { node: { external_id: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } } + - contains: { nodes: { settings: { node: { external_id: "instance-000188" } }, processors: 16, memory: "128gb", storage: "1tb", node_version: $es_version } } --- "Test delete desired nodes": - do: @@ -142,6 +145,9 @@ teardown: - match: { status: 404 } --- "Test update desired nodes is idempotent": + - skip: + reason: "contains is a newly added assertion" + features: contains - do: cluster.state: {} @@ -158,16 +164,51 @@ teardown: body: nodes: - { settings: { "node.external_id": "instance-000187" }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } + - { settings: { "node.external_id": "instance-000188" }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } - match: { replaced_existing_history_id: false } - do: _internal.get_desired_nodes: {} - - match: - $body: + + - match: { history_id: "test" } + - match: { version: 1 } + - length: { nodes: 2 } + - contains: { nodes: { settings: { node: { external_id: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } } + - contains: { nodes: { settings: { node: { external_id: "instance-000188" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } } + + - do: + _internal.update_desired_nodes: history_id: "test" version: 1 - nodes: - - { settings: { node: { external_id: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } + body: + nodes: + - { settings: { "node.external_id": "instance-000187" }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } + - { settings: { "node.external_id": "instance-000188" }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } + + - match: { replaced_existing_history_id: false } + + - do: + _internal.get_desired_nodes: {} + + - match: { history_id: "test" } + - match: { version: 1 } + - length: { nodes: 2 } + - contains: { nodes: { settings: { node: { external_id: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } } + - contains: { nodes: { settings: { node: { external_id: "instance-000188" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } } +--- +"Test update desired nodes is idempotent with different order": + - skip: + version: " - 8.2.99" + features: contains + reason: "Bug fixed in 8.3.0 and uses contains feature" + - do: + cluster.state: {} + + - set: { master_node: master } + + - do: + nodes.info: {} + - set: { nodes.$master.version: es_version } - do: _internal.update_desired_nodes: @@ -176,16 +217,37 @@ teardown: body: nodes: - { settings: { "node.external_id": "instance-000187" }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } + - { settings: { "node.external_id": "instance-000188" }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } - match: { replaced_existing_history_id: false } - do: _internal.get_desired_nodes: {} - - match: - $body: + + - match: { history_id: "test" } + - match: { version: 1 } + - length: { nodes: 2 } + - contains: { nodes: { settings: { node: { external_id: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } } + - contains: { nodes: { settings: { node: { external_id: "instance-000188" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } } + + - do: + _internal.update_desired_nodes: history_id: "test" version: 1 - nodes: - - { settings: { node: { external_id: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } + body: + nodes: + - { settings: { "node.external_id": "instance-000188" }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } + - { settings: { "node.external_id": "instance-000187" }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } + + - match: { replaced_existing_history_id: false } + + - do: + _internal.get_desired_nodes: {} + + - match: { history_id: "test" } + - match: { version: 1 } + - length: { nodes: 2 } + - contains: { nodes: { settings: { node: { external_id: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } } + - contains: { nodes: { settings: { node: { external_id: "instance-000188" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } } --- "Test going backwards within the same history is forbidden": - do: @@ -343,6 +405,9 @@ teardown: - match: { replaced_existing_history_id: false } --- "Test external_id or node.name is required": + - skip: + version: " - 8.2.99" + reason: "Change error code in 8.3" - do: cluster.state: {} @@ -361,11 +426,13 @@ teardown: nodes: - { settings: { }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } - match: { status: 400 } - - match: { error.type: illegal_argument_exception } - - match: { error.reason: "Nodes with ids [] in positions [0] contain invalid settings" } - - match: { error.suppressed.0.reason: "[node.name] or [node.external_id] is missing or empty" } + - match: { error.type: x_content_parse_exception } + - match: { error.caused_by.caused_by.caused_by.reason: "[node.name] or [node.external_id] is missing or empty" } --- "Test external_id must have content": + - skip: + version: " - 8.2.99" + reason: "Change error code in 8.3" - do: cluster.state: {} @@ -384,9 +451,8 @@ teardown: nodes: - { settings: { "node.external_id": " " }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } - match: { status: 400 } - - match: { error.type: illegal_argument_exception } - - match: { error.reason: "Nodes with ids [] in positions [0] contain invalid settings" } - - match: { error.suppressed.0.reason: "[node.name] or [node.external_id] is missing or empty" } + - match: { error.type: x_content_parse_exception } + - match: { error.caused_by.caused_by.caused_by.reason: "[node.name] or [node.external_id] is missing or empty" } --- "Test duplicated external ids are not allowed": - do: diff --git a/server/src/internalClusterTest/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportDesiredNodesActionsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportDesiredNodesActionsIT.java index 9e77a51eca694..347d946dc1b3f 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportDesiredNodesActionsIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportDesiredNodesActionsIT.java @@ -16,6 +16,7 @@ import org.elasticsearch.cluster.ClusterStateTaskExecutor; import org.elasticsearch.cluster.ClusterStateUpdateTask; import org.elasticsearch.cluster.desirednodes.VersionConflictException; +import org.elasticsearch.cluster.metadata.DesiredNode; import org.elasticsearch.cluster.metadata.DesiredNodes; import org.elasticsearch.cluster.metadata.DesiredNodesMetadata; import org.elasticsearch.cluster.metadata.DesiredNodesTestCase; @@ -27,6 +28,7 @@ import org.junit.After; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.concurrent.CountDownLatch; @@ -37,7 +39,6 @@ import static org.elasticsearch.cluster.metadata.DesiredNodesTestCase.randomDesiredNodes; import static org.elasticsearch.common.util.concurrent.EsExecutors.NODE_PROCESSORS_SETTING; import static org.elasticsearch.http.HttpTransportSettings.SETTING_HTTP_TCP_KEEP_IDLE; -import static org.elasticsearch.node.Node.NODE_EXTERNAL_ID_SETTING; import static org.elasticsearch.node.NodeRoleSettings.NODE_ROLES_SETTING; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -64,10 +65,16 @@ public void testUpdateDesiredNodes() { public void testUpdateDesiredNodesIsIdempotent() { final DesiredNodes desiredNodes = putRandomDesiredNodes(); - updateDesiredNodes(desiredNodes); + + final List desiredNodesList = new ArrayList<>(desiredNodes.nodes()); + if (randomBoolean()) { + Collections.shuffle(desiredNodesList, random()); + } + + updateDesiredNodes(new DesiredNodes(desiredNodes.historyID(), desiredNodes.version(), desiredNodesList)); final ClusterState state = client().admin().cluster().prepareState().get().getState(); - final DesiredNodesMetadata metadata = state.metadata().custom(DesiredNodesMetadata.TYPE); + final DesiredNodesMetadata metadata = DesiredNodesMetadata.fromClusterState(state); assertThat(metadata, is(notNullValue())); final DesiredNodes latestDesiredNodes = metadata.getLatestDesiredNodes(); assertThat(latestDesiredNodes, is(equalTo(desiredNodes))); @@ -247,10 +254,9 @@ public void testNodeProcessorsGetValidatedWithDesiredNodeProcessors() { final DesiredNodes latestDesiredNodes = metadata.getLatestDesiredNodes(); assertThat(latestDesiredNodes, is(equalTo(desiredNodes))); assertThat(latestDesiredNodes.nodes().isEmpty(), is(equalTo(false))); - assertThat( - latestDesiredNodes.nodes().get(0).settings().get(NODE_PROCESSORS_SETTING.getKey()), - is(equalTo(Integer.toString(numProcessors))) - ); + for (DesiredNode desiredNode : latestDesiredNodes.nodes()) { + assertThat(desiredNode.settings().get(NODE_PROCESSORS_SETTING.getKey()), is(equalTo(Integer.toString(numProcessors)))); + } } } @@ -263,7 +269,7 @@ public void testUpdateDesiredNodesTasksAreBatchedCorrectly() throws Exception { final UpdateDesiredNodesRequest request = new UpdateDesiredNodesRequest( desiredNodes.historyID(), desiredNodes.version(), - desiredNodes.nodes() + List.copyOf(desiredNodes.nodes()) ); // Use the master client to ensure the same updates ordering as in proposedDesiredNodesList updateDesiredNodesFutures.add(internalCluster().masterClient().execute(UpdateDesiredNodesAction.INSTANCE, request)); @@ -280,7 +286,7 @@ public void testUpdateDesiredNodesTasksAreBatchedCorrectly() throws Exception { } final ClusterState state = client().admin().cluster().prepareState().get().getState(); - final DesiredNodes latestDesiredNodes = DesiredNodesMetadata.latestFromClusterState(state); + final DesiredNodes latestDesiredNodes = DesiredNodes.latestFromClusterState(state); final DesiredNodes latestProposedDesiredNodes = proposedDesiredNodes.get(proposedDesiredNodes.size() - 1); assertThat(latestDesiredNodes, equalTo(latestProposedDesiredNodes)); } @@ -308,7 +314,7 @@ public void testDeleteDesiredNodesTasksAreBatchedCorrectly() throws Exception { } final ClusterState state = client().admin().cluster().prepareState().get().getState(); - final DesiredNodes latestDesiredNodes = DesiredNodesMetadata.latestFromClusterState(state); + final DesiredNodes latestDesiredNodes = DesiredNodes.latestFromClusterState(state); assertThat(latestDesiredNodes, is(nullValue())); } @@ -328,21 +334,6 @@ public void testDeleteDesiredNodes() { expectThrows(ResourceNotFoundException.class, this::getLatestDesiredNodes); } - public void testEmptyExternalIDIsInvalid() { - final Consumer settingsConsumer = (settings) -> settings.put(NODE_EXTERNAL_ID_SETTING.getKey(), " "); - final DesiredNodes desiredNodes = new DesiredNodes( - UUIDs.randomBase64UUID(), - randomIntBetween(1, 20), - randomList(1, 20, () -> randomDesiredNode(Version.CURRENT, settingsConsumer)) - ); - - final IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> updateDesiredNodes(desiredNodes)); - assertThat(exception.getMessage(), containsString("Nodes with ids")); - assertThat(exception.getMessage(), containsString("contain invalid settings")); - assertThat(exception.getSuppressed().length > 0, is(equalTo(true))); - assertThat(exception.getSuppressed()[0].getMessage(), containsString("[node.external_id] is missing or empty")); - } - private void deleteDesiredNodes() { final DeleteDesiredNodesAction.Request request = new DeleteDesiredNodesAction.Request(); client().execute(DeleteDesiredNodesAction.INSTANCE, request).actionGet(); @@ -364,7 +355,7 @@ private UpdateDesiredNodesResponse updateDesiredNodes(DesiredNodes desiredNodes) final UpdateDesiredNodesRequest request = new UpdateDesiredNodesRequest( desiredNodes.historyID(), desiredNodes.version(), - desiredNodes.nodes() + List.copyOf(desiredNodes.nodes()) ); return client().execute(UpdateDesiredNodesAction.INSTANCE, request).actionGet(); } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportGetDesiredNodesAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportGetDesiredNodesAction.java index def3fbf1113cd..a8c2bedee6bbf 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportGetDesiredNodesAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportGetDesiredNodesAction.java @@ -16,7 +16,6 @@ import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.block.ClusterBlockLevel; import org.elasticsearch.cluster.metadata.DesiredNodes; -import org.elasticsearch.cluster.metadata.DesiredNodesMetadata; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; @@ -55,7 +54,7 @@ protected void masterOperation( ClusterState state, ActionListener listener ) throws Exception { - final DesiredNodes latestDesiredNodes = DesiredNodesMetadata.latestFromClusterState(state); + final DesiredNodes latestDesiredNodes = DesiredNodes.latestFromClusterState(state); if (latestDesiredNodes == null) { listener.onFailure(new ResourceNotFoundException("Desired nodes not found")); } else { diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportUpdateDesiredNodesAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportUpdateDesiredNodesAction.java index 4c8b12bb7c755..2970d7e1348b0 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportUpdateDesiredNodesAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportUpdateDesiredNodesAction.java @@ -74,8 +74,7 @@ protected void masterOperation( ActionListener listener ) throws Exception { try { - DesiredNodes proposedDesiredNodes = new DesiredNodes(request.getHistoryID(), request.getVersion(), request.getNodes()); - settingsValidator.validate(proposedDesiredNodes); + settingsValidator.validate(request.getNodes()); clusterService.submitStateUpdateTask( "update-desired-nodes", @@ -85,8 +84,8 @@ protected void masterOperation( @Override public ClusterState execute(ClusterState currentState) { final ClusterState updatedState = updateDesiredNodes(currentState, request); - final DesiredNodes previousDesiredNodes = DesiredNodesMetadata.latestFromClusterState(currentState); - final DesiredNodes latestDesiredNodes = DesiredNodesMetadata.latestFromClusterState(updatedState); + final DesiredNodes previousDesiredNodes = DesiredNodes.latestFromClusterState(currentState); + final DesiredNodes latestDesiredNodes = DesiredNodes.latestFromClusterState(updatedState); replacedExistingHistoryId = previousDesiredNodes != null && previousDesiredNodes.hasSameHistoryId(latestDesiredNodes) == false; return updatedState; @@ -110,9 +109,9 @@ public void clusterStateProcessed(ClusterState oldState, ClusterState newState) } static ClusterState updateDesiredNodes(ClusterState currentState, UpdateDesiredNodesRequest request) { - DesiredNodesMetadata desiredNodesMetadata = currentState.metadata().custom(DesiredNodesMetadata.TYPE, DesiredNodesMetadata.EMPTY); - DesiredNodes latestDesiredNodes = desiredNodesMetadata.getLatestDesiredNodes(); - DesiredNodes proposedDesiredNodes = new DesiredNodes(request.getHistoryID(), request.getVersion(), request.getNodes()); + final DesiredNodesMetadata desiredNodesMetadata = DesiredNodesMetadata.fromClusterState(currentState); + final DesiredNodes latestDesiredNodes = desiredNodesMetadata.getLatestDesiredNodes(); + final DesiredNodes proposedDesiredNodes = new DesiredNodes(request.getHistoryID(), request.getVersion(), request.getNodes()); if (latestDesiredNodes != null) { if (latestDesiredNodes.equals(proposedDesiredNodes)) { diff --git a/server/src/main/java/org/elasticsearch/cluster/desirednodes/DesiredNodesSettingsValidator.java b/server/src/main/java/org/elasticsearch/cluster/desirednodes/DesiredNodesSettingsValidator.java index f71efee103239..0b25538ea4397 100644 --- a/server/src/main/java/org/elasticsearch/cluster/desirednodes/DesiredNodesSettingsValidator.java +++ b/server/src/main/java/org/elasticsearch/cluster/desirednodes/DesiredNodesSettingsValidator.java @@ -10,7 +10,6 @@ import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.DesiredNode; -import org.elasticsearch.cluster.metadata.DesiredNodes; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; @@ -23,15 +22,9 @@ import static java.lang.String.format; import static org.elasticsearch.common.util.concurrent.EsExecutors.NODE_PROCESSORS_SETTING; -import static org.elasticsearch.node.Node.NODE_EXTERNAL_ID_SETTING; -import static org.elasticsearch.node.Node.NODE_NAME_SETTING; public class DesiredNodesSettingsValidator { - private record DesiredNodeValidationError(int position, @Nullable String externalId, RuntimeException exception) { - public String externalId() { - return externalId == null ? "" : externalId; - } - } + private record DesiredNodeValidationError(int position, @Nullable String externalId, RuntimeException exception) {} private final ClusterSettings clusterSettings; @@ -39,9 +32,8 @@ public DesiredNodesSettingsValidator(ClusterSettings clusterSettings) { this.clusterSettings = clusterSettings; } - public void validate(DesiredNodes desiredNodes) { + public void validate(List nodes) { final List validationErrors = new ArrayList<>(); - final List nodes = desiredNodes.nodes(); for (int i = 0; i < nodes.size(); i++) { final DesiredNode node = nodes.get(i); try { @@ -82,12 +74,6 @@ private void validate(DesiredNode node) { ); } - if (node.externalId() == null) { - throw new IllegalArgumentException( - format(Locale.ROOT, "[%s] or [%s] is missing or empty", NODE_NAME_SETTING.getKey(), NODE_EXTERNAL_ID_SETTING.getKey()) - ); - } - // Validating settings for future versions can be unsafe: // - If the legal range is upgraded in the newer version // - If a new setting is used as the default value for a previous setting diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java b/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java index 7125ee21bd86d..3e5a247ea9026 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java @@ -9,6 +9,7 @@ package org.elasticsearch.cluster.metadata; import org.elasticsearch.Version; +import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodeRole; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -23,14 +24,18 @@ import org.elasticsearch.xcontent.XContentParser; import java.io.IOException; +import java.util.Collections; +import java.util.Locale; +import java.util.Objects; +import java.util.Set; +import java.util.TreeSet; +import static java.lang.String.format; import static org.elasticsearch.node.Node.NODE_EXTERNAL_ID_SETTING; +import static org.elasticsearch.node.Node.NODE_NAME_SETTING; import static org.elasticsearch.node.NodeRoleSettings.NODE_ROLES_SETTING; -public record DesiredNode(Settings settings, int processors, ByteSizeValue memory, ByteSizeValue storage, Version version) - implements - Writeable, - ToXContentObject { +public final class DesiredNode implements Writeable, ToXContentObject, Comparable { private static final ParseField SETTINGS_FIELD = new ParseField("settings"); private static final ParseField PROCESSORS_FIELD = new ParseField("processors"); @@ -80,13 +85,36 @@ private static Version parseVersion(String version) { return Version.fromString(version); } - public DesiredNode { + private final Settings settings; + private final int processors; + private final ByteSizeValue memory; + private final ByteSizeValue storage; + private final Version version; + private final String externalId; + private final Set roles; + + public DesiredNode(Settings settings, int processors, ByteSizeValue memory, ByteSizeValue storage, Version version) { + assert settings != null; assert memory != null; assert storage != null; assert version != null; if (processors <= 0) { throw new IllegalArgumentException("processors must be greater than 0, but got " + processors); } + + if (NODE_EXTERNAL_ID_SETTING.get(settings).isBlank()) { + throw new IllegalArgumentException( + format(Locale.ROOT, "[%s] or [%s] is missing or empty", NODE_NAME_SETTING.getKey(), NODE_EXTERNAL_ID_SETTING.getKey()) + ); + } + + this.settings = settings; + this.processors = processors; + this.memory = memory; + this.storage = storage; + this.version = version; + this.externalId = NODE_EXTERNAL_ID_SETTING.get(settings); + this.roles = Collections.unmodifiableSortedSet(new TreeSet<>(DiscoveryNode.getRolesFromSettings(settings))); } public DesiredNode(StreamInput in) throws IOException { @@ -120,12 +148,81 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder; } + public boolean hasMasterRole() { + return NODE_ROLES_SETTING.get(settings).contains(DiscoveryNodeRole.MASTER_ROLE); + } + + public Settings settings() { + return settings; + } + + public int processors() { + return processors; + } + + public ByteSizeValue memory() { + return memory; + } + + public ByteSizeValue storage() { + return storage; + } + + public Version version() { + return version; + } + public String externalId() { - String externalId = NODE_EXTERNAL_ID_SETTING.get(settings); - return externalId.isBlank() ? null : externalId; + return externalId; } - public boolean hasMasterRole() { - return NODE_ROLES_SETTING.get(settings).contains(DiscoveryNodeRole.MASTER_ROLE); + public Set getRoles() { + return roles; + } + + @Override + public boolean equals(Object obj) { + if (obj == this) return true; + if (obj == null || obj.getClass() != this.getClass()) return false; + var that = (DesiredNode) obj; + // Note that we might consider a DesiredNode different if the order + // in some settings is different, i.e. we convert node roles to a set in this class, + // so it can be confusing if we compare two DesiredNode instances that only differ + // in the node.roles setting order, but that's the semantics provided by the Settings class. + return Objects.equals(this.settings, that.settings) + && this.processors == that.processors + && Objects.equals(this.memory, that.memory) + && Objects.equals(this.storage, that.storage) + && Objects.equals(this.version, that.version); + } + + @Override + public int hashCode() { + return Objects.hash(settings, processors, memory, storage, version); + } + + @Override + public int compareTo(DesiredNode o) { + return externalId.compareTo(o.externalId); + } + + @Override + public String toString() { + return "DesiredNode[" + + "settings=" + + settings + + ", " + + "processors=" + + processors + + ", " + + "memory=" + + memory + + ", " + + "storage=" + + storage + + ", " + + "version=" + + version + + ']'; } } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodes.java b/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodes.java index 698f14df0ff22..803a2b4b1c961 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodes.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodes.java @@ -8,9 +8,11 @@ package org.elasticsearch.cluster.metadata; +import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.core.Nullable; import org.elasticsearch.xcontent.ConstructingObjectParser; import org.elasticsearch.xcontent.ParseField; import org.elasticsearch.xcontent.ToXContentObject; @@ -18,15 +20,21 @@ import org.elasticsearch.xcontent.XContentParser; import java.io.IOException; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Locale; +import java.util.Map; +import java.util.Objects; import java.util.Set; +import java.util.TreeMap; +import java.util.function.Function; +import java.util.stream.Collectors; import static java.lang.String.format; import static org.elasticsearch.node.Node.NODE_EXTERNAL_ID_SETTING; -public record DesiredNodes(String historyID, long version, List nodes) implements Writeable, ToXContentObject { +public class DesiredNodes implements Writeable, ToXContentObject { private static final ParseField HISTORY_ID_FIELD = new ParseField("history_id"); private static final ParseField VERSION_FIELD = new ParseField("version"); @@ -45,10 +53,18 @@ public record DesiredNodes(String historyID, long version, List nod PARSER.declareObjectArray(ConstructingObjectParser.constructorArg(), (p, c) -> DesiredNode.fromXContent(p), NODES_FIELD); } - public DesiredNodes { + private final String historyID; + private final long version; + private final Map nodes; + + public DesiredNodes(String historyID, long version, List nodes) { assert historyID != null && historyID.isBlank() == false; assert version != Long.MIN_VALUE; checkForDuplicatedExternalIDs(nodes); + + this.historyID = historyID; + this.version = version; + this.nodes = toMap(nodes); } public DesiredNodes(StreamInput in) throws IOException { @@ -59,7 +75,7 @@ public DesiredNodes(StreamInput in) throws IOException { public void writeTo(StreamOutput out) throws IOException { out.writeString(historyID); out.writeLong(version); - out.writeList(nodes); + out.writeCollection(nodes.values()); } static DesiredNodes fromXContent(XContentParser parser) throws IOException { @@ -71,11 +87,15 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.startObject(); builder.field(HISTORY_ID_FIELD.getPreferredName(), historyID); builder.field(VERSION_FIELD.getPreferredName(), version); - builder.xContentList(NODES_FIELD.getPreferredName(), nodes); + builder.xContentList(NODES_FIELD.getPreferredName(), nodes.values()); builder.endObject(); return builder; } + public static DesiredNodes latestFromClusterState(ClusterState clusterState) { + return DesiredNodesMetadata.fromClusterState(clusterState).getLatestDesiredNodes(); + } + public boolean isSupersededBy(DesiredNodes otherDesiredNodes) { return historyID.equals(otherDesiredNodes.historyID) == false || version < otherDesiredNodes.version; } @@ -93,10 +113,9 @@ private static void checkForDuplicatedExternalIDs(List nodes) { Set duplicatedIDs = new HashSet<>(); for (DesiredNode node : nodes) { String externalID = node.externalId(); - if (externalID != null) { - if (nodeIDs.add(externalID) == false) { - duplicatedIDs.add(externalID); - } + assert externalID != null; + if (nodeIDs.add(externalID) == false) { + duplicatedIDs.add(externalID); } } if (duplicatedIDs.isEmpty() == false) { @@ -110,4 +129,49 @@ private static void checkForDuplicatedExternalIDs(List nodes) { ); } } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + DesiredNodes that = (DesiredNodes) o; + return version == that.version && Objects.equals(historyID, that.historyID) && Objects.equals(nodes, that.nodes); + } + + @Override + public int hashCode() { + return Objects.hash(historyID, version, nodes); + } + + @Override + public String toString() { + return "DesiredNodes{" + "historyID='" + historyID + '\'' + ", version=" + version + ", nodes=" + nodes + '}'; + } + + public String historyID() { + return historyID; + } + + public long version() { + return version; + } + + public List nodes() { + return List.copyOf(nodes.values()); + } + + @Nullable + public DesiredNode find(String externalId) { + return nodes.get(externalId); + } + + private static Map toMap(final List desiredNodes) { + // use a linked hash map to preserve order + return Collections.unmodifiableMap( + desiredNodes.stream().collect(Collectors.toMap(DesiredNode::externalId, Function.identity(), (left, right) -> { + assert left.externalId().equals(right.externalId()) == false; + throw new IllegalStateException("duplicate desired node external id [" + left.externalId() + "]"); + }, TreeMap::new)) + ); + } } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMetadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMetadata.java index a93b48ff47265..9bdeedec88e48 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMetadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMetadata.java @@ -72,8 +72,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder; } - public static DesiredNodes latestFromClusterState(ClusterState clusterState) { - return clusterState.metadata().custom(TYPE, EMPTY).getLatestDesiredNodes(); + public static DesiredNodesMetadata fromClusterState(ClusterState clusterState) { + return clusterState.metadata().custom(TYPE, EMPTY); } @Nullable diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportUpdateDesiredNodesActionTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportUpdateDesiredNodesActionTests.java index 95180f1b7dc15..816d99eaa8ad1 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportUpdateDesiredNodesActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/desirednodes/TransportUpdateDesiredNodesActionTests.java @@ -28,10 +28,13 @@ import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; import static org.elasticsearch.action.admin.cluster.desirednodes.UpdateDesiredNodesRequestSerializationTests.randomUpdateDesiredNodesRequest; import static org.elasticsearch.cluster.metadata.DesiredNodesMetadataSerializationTests.randomDesiredNodesMetadata; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -47,7 +50,7 @@ public class TransportUpdateDesiredNodesActionTests extends DesiredNodesTestCase public static final DesiredNodesSettingsValidator NO_OP_SETTINGS_VALIDATOR = new DesiredNodesSettingsValidator(null) { @Override - public void validate(DesiredNodes desiredNodes) {} + public void validate(List desiredNodes) {} }; public void testWriteBlocks() { @@ -93,7 +96,7 @@ public void testNoBlocks() { public void testSettingsGetValidated() throws Exception { DesiredNodesSettingsValidator validator = new DesiredNodesSettingsValidator(null) { @Override - public void validate(DesiredNodes desiredNodes) { + public void validate(List desiredNodes) { throw new IllegalArgumentException("Invalid settings"); } }; @@ -150,7 +153,7 @@ public void testUpdateDesiredNodes() { assertThat(desiredNodes, is(notNullValue())); assertThat(desiredNodes.historyID(), is(equalTo(request.getHistoryID()))); assertThat(desiredNodes.version(), is(equalTo(request.getVersion()))); - assertThat(desiredNodes.nodes(), is(equalTo(request.getNodes()))); + assertThat(desiredNodes.nodes(), containsInAnyOrder(request.getNodes().toArray())); } public void testUpdatesAreIdempotent() { @@ -160,10 +163,15 @@ public void testUpdatesAreIdempotent() { .build(); final DesiredNodes latestDesiredNodes = desiredNodesMetadata.getLatestDesiredNodes(); + + final List equivalentDesiredNodesList = new ArrayList<>(latestDesiredNodes.nodes()); + if (randomBoolean()) { + Collections.shuffle(equivalentDesiredNodesList, random()); + } final UpdateDesiredNodesRequest request = new UpdateDesiredNodesRequest( latestDesiredNodes.historyID(), latestDesiredNodes.version(), - latestDesiredNodes.nodes() + equivalentDesiredNodesList ); final ClusterState updatedClusterState = TransportUpdateDesiredNodesAction.updateDesiredNodes(currentClusterState, request); @@ -203,7 +211,7 @@ public void testBackwardUpdatesFails() { final UpdateDesiredNodesRequest request = new UpdateDesiredNodesRequest( latestDesiredNodes.historyID(), latestDesiredNodes.version() - 1, - latestDesiredNodes.nodes() + List.copyOf(latestDesiredNodes.nodes()) ); VersionConflictException exception = expectThrows( diff --git a/server/src/test/java/org/elasticsearch/cluster/desirednodes/DesiredNodesSettingsValidatorTests.java b/server/src/test/java/org/elasticsearch/cluster/desirednodes/DesiredNodesSettingsValidatorTests.java index 8714904af51d4..1b06cacd51541 100644 --- a/server/src/test/java/org/elasticsearch/cluster/desirednodes/DesiredNodesSettingsValidatorTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/desirednodes/DesiredNodesSettingsValidatorTests.java @@ -10,7 +10,6 @@ import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.DesiredNode; -import org.elasticsearch.cluster.metadata.DesiredNodes; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; @@ -23,8 +22,8 @@ import java.util.function.Consumer; import static org.elasticsearch.cluster.metadata.DesiredNodesTestCase.randomDesiredNode; -import static org.elasticsearch.cluster.metadata.DesiredNodesTestCase.randomDesiredNodes; -import static org.elasticsearch.cluster.metadata.DesiredNodesTestCase.randomDesiredNodesWithRandomSettings; +import static org.elasticsearch.cluster.metadata.DesiredNodesTestCase.randomDesiredNodeList; +import static org.elasticsearch.cluster.metadata.DesiredNodesTestCase.randomDesiredNodeListWithRandomSettings; import static org.elasticsearch.common.util.concurrent.EsExecutors.NODE_PROCESSORS_SETTING; import static org.elasticsearch.node.Node.NODE_EXTERNAL_ID_SETTING; import static org.elasticsearch.node.Node.NODE_NAME_SETTING; @@ -52,7 +51,7 @@ public void testSettingsValidation() { final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, availableSettings); final DesiredNodesSettingsValidator validator = new DesiredNodesSettingsValidator(clusterSettings); - final DesiredNodes desiredNodes = randomDesiredNodes(Version.CURRENT, settingsProvider); + final List desiredNodes = randomDesiredNodeList(Version.CURRENT, settingsProvider); IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> validator.validate(desiredNodes)); assertThat(exception.getMessage(), containsString("Nodes with ids")); @@ -62,11 +61,7 @@ public void testSettingsValidation() { } public void testNodeVersionValidation() { - final DesiredNodes desiredNodes = new DesiredNodes( - randomAlphaOfLength(10), - randomIntBetween(1, 20), - List.of(randomDesiredNode(Version.CURRENT.previousMajor(), (settings) -> {})) - ); + final List desiredNodes = List.of(randomDesiredNode(Version.CURRENT.previousMajor(), (settings) -> {})); final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, Collections.emptySet()); final DesiredNodesSettingsValidator validator = new DesiredNodesSettingsValidator(clusterSettings); @@ -81,7 +76,7 @@ public void testNodeVersionValidation() { public void testUnknownSettingsInKnownVersionsAreInvalid() { final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, Collections.emptySet()); final DesiredNodesSettingsValidator validator = new DesiredNodesSettingsValidator(clusterSettings); - final DesiredNodes desiredNodes = randomDesiredNodes(); + final List desiredNodes = randomDesiredNodeList(Version.CURRENT, settings -> {}); IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> validator.validate(desiredNodes)); assertThat(exception.getMessage(), containsString("Nodes with ids")); @@ -94,34 +89,10 @@ public void testSettingsInFutureVersionsAreNotValidated() { final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, Collections.emptySet()); final DesiredNodesSettingsValidator validator = new DesiredNodesSettingsValidator(clusterSettings); - final DesiredNodes desiredNodes = randomDesiredNodesWithRandomSettings(Version.fromString("99.9.0")); + final List desiredNodes = randomDesiredNodeListWithRandomSettings(Version.fromString("99.9.0")); validator.validate(desiredNodes); } - public void testExternalIDIsRequired() { - final Set> availableSettings = Set.of( - Setting.intSetting("test.value", 1, Setting.Property.NodeScope), - NODE_EXTERNAL_ID_SETTING, - NODE_NAME_SETTING - ); - final Consumer settingsProvider = settings -> { - settings.put("test.value", randomInt()); - settings.remove(NODE_EXTERNAL_ID_SETTING.getKey()); - settings.remove(NODE_NAME_SETTING.getKey()); - }; - - final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, availableSettings); - final DesiredNodesSettingsValidator validator = new DesiredNodesSettingsValidator(clusterSettings); - - final DesiredNodes desiredNodes = randomDesiredNodes(Version.CURRENT, settingsProvider); - - IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> validator.validate(desiredNodes)); - assertThat(exception.getMessage(), containsString("Nodes with ids")); - assertThat(exception.getMessage(), containsString("contain invalid settings")); - assertThat(exception.getSuppressed().length > 0, is(equalTo(true))); - assertThat(exception.getSuppressed()[0].getMessage(), containsString("[node.name] or [node.external_id] is missing or empty")); - } - public void testNodeProcessorsValidation() { final Set> availableSettings = Set.of(NODE_PROCESSORS_SETTING, NODE_EXTERNAL_ID_SETTING, NODE_NAME_SETTING); @@ -134,10 +105,8 @@ public void testNodeProcessorsValidation() { .put(NODE_EXTERNAL_ID_SETTING.getKey(), randomAlphaOfLength(10)) .put(NODE_PROCESSORS_SETTING.getKey(), desiredNodeProcessors) .build(); - final DesiredNodes desiredNodes = new DesiredNodes( - randomAlphaOfLength(10), - 1, - List.of(new DesiredNode(nodeSettings, desiredNodeProcessors, ByteSizeValue.ofGb(1), ByteSizeValue.ofGb(1), Version.CURRENT)) + final List desiredNodes = List.of( + new DesiredNode(nodeSettings, desiredNodeProcessors, ByteSizeValue.ofGb(1), ByteSizeValue.ofGb(1), Version.CURRENT) ); validator.validate(desiredNodes); @@ -149,10 +118,8 @@ public void testNodeProcessorsValidation() { .put(NODE_EXTERNAL_ID_SETTING.getKey(), randomAlphaOfLength(10)) .put(NODE_PROCESSORS_SETTING.getKey(), desiredNodeProcessors + 1) .build(); - final DesiredNodes desiredNodes = new DesiredNodes( - randomAlphaOfLength(10), - 1, - List.of(new DesiredNode(nodeSettings, desiredNodeProcessors, ByteSizeValue.ofGb(1), ByteSizeValue.ofGb(1), Version.CURRENT)) + final List desiredNodes = List.of( + new DesiredNode(nodeSettings, desiredNodeProcessors, ByteSizeValue.ofGb(1), ByteSizeValue.ofGb(1), Version.CURRENT) ); final IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> validator.validate(desiredNodes)); diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodeTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodeTests.java index c1f680f0de05d..404154fec6316 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodeTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodeTests.java @@ -9,17 +9,40 @@ package org.elasticsearch.cluster.metadata; import org.elasticsearch.Version; +import org.elasticsearch.cluster.node.DiscoveryNodeRole; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.test.ESTestCase; +import static org.elasticsearch.node.Node.NODE_EXTERNAL_ID_SETTING; import static org.elasticsearch.node.Node.NODE_NAME_SETTING; import static org.elasticsearch.node.NodeRoleSettings.NODE_ROLES_SETTING; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; public class DesiredNodeTests extends ESTestCase { + + public void testExternalIdIsRequired() { + final Settings.Builder settings = Settings.builder(); + if (randomBoolean()) { + final String key = randomBoolean() ? NODE_NAME_SETTING.getKey() : NODE_EXTERNAL_ID_SETTING.getKey(); + if (randomBoolean()) { + settings.put(key, " "); + } else { + settings.putNull(key); + } + } + + final IllegalArgumentException exception = expectThrows( + IllegalArgumentException.class, + () -> new DesiredNode(settings.build(), 1, ByteSizeValue.ofGb(1), ByteSizeValue.ofGb(1), Version.CURRENT) + ); + assertThat(exception.getMessage(), is(equalTo("[node.name] or [node.external_id] is missing or empty"))); + } + public void testExternalIdFallbacksToNodeName() { final String nodeName = randomAlphaOfLength(10); final Settings settings = Settings.builder().put(NODE_NAME_SETTING.getKey(), nodeName).build(); @@ -66,4 +89,22 @@ public void testHasMasterRole() { assertFalse(desiredNode.hasMasterRole()); } } + + public void testGetRoles() { + final var settings = Settings.builder().put(NODE_NAME_SETTING.getKey(), randomAlphaOfLength(10)); + + final var role = randomBoolean() ? null : randomFrom(DiscoveryNodeRole.roles()); + if (role != null) { + settings.put(NODE_ROLES_SETTING.getKey(), role.roleName()); + } + + final var desiredNode = new DesiredNode(settings.build(), 1, ByteSizeValue.ofGb(1), ByteSizeValue.ofGb(1), Version.CURRENT); + + if (role != null) { + assertThat(desiredNode.getRoles(), hasSize(1)); + assertThat(desiredNode.getRoles(), contains(role)); + } else { + assertThat(desiredNode.getRoles(), contains(NODE_ROLES_SETTING.get(Settings.EMPTY).toArray())); + } + } } diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodesTestCase.java b/server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodesTestCase.java index 83ca91dd25735..1d572da90f2e4 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodesTestCase.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodesTestCase.java @@ -15,6 +15,7 @@ import org.elasticsearch.core.TimeValue; import org.elasticsearch.test.ESTestCase; +import java.util.List; import java.util.function.Consumer; import static org.elasticsearch.node.Node.NODE_EXTERNAL_ID_SETTING; @@ -38,11 +39,15 @@ public static DesiredNodes randomDesiredNodes(Consumer setting } public static DesiredNodes randomDesiredNodes(Version version, Consumer settingsConsumer) { - return new DesiredNodes( - UUIDs.randomBase64UUID(), - randomIntBetween(1, 20), - randomList(1, 10, () -> randomDesiredNode(version, settingsConsumer)) - ); + return new DesiredNodes(UUIDs.randomBase64UUID(), randomIntBetween(1, 20), randomDesiredNodeList(version, settingsConsumer)); + } + + public static List randomDesiredNodeListWithRandomSettings(Version version) { + return randomDesiredNodeList(version, DesiredNodesTestCase::putRandomSetting); + } + + public static List randomDesiredNodeList(Version version, Consumer settingsConsumer) { + return randomList(2, 10, () -> randomDesiredNode(version, settingsConsumer)); } public static DesiredNode randomDesiredNodeWithRandomSettings() {