From ef2cf37a6df0e6797417f476061e46cd6bfea760 Mon Sep 17 00:00:00 2001 From: Artem Prigoda Date: Tue, 29 Oct 2024 12:03:19 +0100 Subject: [PATCH] Revert "Don't return or accept `node_version` in the Desired Nodes API (#114580)" (#115829) This reverts commit c64226c3503b458c3285064d95528932d324177d. --- .../upgrades/DesiredNodesUpgradeIT.java | 13 ++- rest-api-spec/build.gradle | 2 - .../test/cluster.desired_nodes/10_basic.yml | 95 +++++++++++++++++++ .../cluster/metadata/DesiredNode.java | 77 ++++++++++++++- .../metadata/DesiredNodeWithStatus.java | 5 +- .../cluster/RestUpdateDesiredNodesAction.java | 12 +++ 6 files changed, 191 insertions(+), 13 deletions(-) diff --git a/qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/DesiredNodesUpgradeIT.java b/qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/DesiredNodesUpgradeIT.java index 17618d5439d4..e0d1e7aafa63 100644 --- a/qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/DesiredNodesUpgradeIT.java +++ b/qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/DesiredNodesUpgradeIT.java @@ -11,6 +11,7 @@ package org.elasticsearch.upgrades; import com.carrotsearch.randomizedtesting.annotations.Name; +import org.elasticsearch.Build; import org.elasticsearch.action.admin.cluster.desirednodes.UpdateDesiredNodesRequest; import org.elasticsearch.client.Request; import org.elasticsearch.client.ResponseException; @@ -81,7 +82,8 @@ public class DesiredNodesUpgradeIT extends AbstractRollingUpgradeTestCase { Settings.builder().put(NODE_NAME_SETTING.getKey(), nodeName).build(), 1238.49922909, ByteSizeValue.ofGb(32), - ByteSizeValue.ofGb(128) + ByteSizeValue.ofGb(128), + clusterHasFeature(DesiredNode.DESIRED_NODE_VERSION_DEPRECATED) ? null : Build.current().version() ) ) .toList(); @@ -151,7 +153,8 @@ public class DesiredNodesUpgradeIT extends AbstractRollingUpgradeTestCase { Settings.builder().put(NODE_NAME_SETTING.getKey(), nodeName).build(), processorsPrecision == ProcessorsPrecision.DOUBLE ? randomDoubleProcessorCount() : 0.5f, ByteSizeValue.ofGb(randomIntBetween(10, 24)), - ByteSizeValue.ofGb(randomIntBetween(128, 256)) + ByteSizeValue.ofGb(randomIntBetween(128, 256)), + clusterHasFeature(DesiredNode.DESIRED_NODE_VERSION_DEPRECATED) ? null : Build.current().version() ) ) .toList(); @@ -164,7 +167,8 @@ public class DesiredNodesUpgradeIT extends AbstractRollingUpgradeTestCase { Settings.builder().put(NODE_NAME_SETTING.getKey(), nodeName).build(), new DesiredNode.ProcessorsRange(minProcessors, minProcessors + randomIntBetween(10, 20)), ByteSizeValue.ofGb(randomIntBetween(10, 24)), - ByteSizeValue.ofGb(randomIntBetween(128, 256)) + ByteSizeValue.ofGb(randomIntBetween(128, 256)), + clusterHasFeature(DesiredNode.DESIRED_NODE_VERSION_DEPRECATED) ? null : Build.current().version() ); }).toList(); } @@ -178,7 +182,8 @@ public class DesiredNodesUpgradeIT extends AbstractRollingUpgradeTestCase { Settings.builder().put(NODE_NAME_SETTING.getKey(), nodeName).build(), randomIntBetween(1, 24), ByteSizeValue.ofGb(randomIntBetween(10, 24)), - ByteSizeValue.ofGb(randomIntBetween(128, 256)) + ByteSizeValue.ofGb(randomIntBetween(128, 256)), + clusterHasFeature(DesiredNode.DESIRED_NODE_VERSION_DEPRECATED) ? null : Build.current().version() ) ) .toList(); diff --git a/rest-api-spec/build.gradle b/rest-api-spec/build.gradle index 1a398f79085e..6cc2028bffa3 100644 --- a/rest-api-spec/build.gradle +++ b/rest-api-spec/build.gradle @@ -61,6 +61,4 @@ tasks.named("yamlRestCompatTestTransform").configure ({ task -> task.skipTest("search/330_fetch_fields/Test search rewrite", "warning does not exist for compatibility") task.skipTest("indices.create/21_synthetic_source_stored/object param - nested object with stored array", "temporary until backported") task.skipTest("cat.aliases/10_basic/Deprecated local parameter", "CAT APIs not covered by compatibility policy") - task.skipTest("cluster.desired_nodes/10_basic/Test delete desired nodes with node_version generates a warning", "node_version warning is removed in 9.0") - task.skipTest("cluster.desired_nodes/10_basic/Test update desired nodes with node_version generates a warning", "node_version warning is removed in 9.0") }) 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 a45146a4e147..1d1aa524ffb2 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 @@ -59,6 +59,61 @@ teardown: - contains: { nodes: { settings: { node: { name: "instance-000187" } }, processors: 8.5, memory: "64gb", storage: "128gb" } } - contains: { nodes: { settings: { node: { name: "instance-000188" } }, processors: 16.0, memory: "128gb", storage: "1tb" } } --- +"Test update desired nodes with node_version generates a warning": + - skip: + reason: "contains is a newly added assertion" + features: ["contains", "allowed_warnings"] + - do: + cluster.state: {} + + # Get master node id + - set: { master_node: master } + + - do: + nodes.info: {} + - set: { nodes.$master.version: es_version } + + - do: + _internal.update_desired_nodes: + history_id: "test" + version: 1 + body: + nodes: + - { settings: { "node.name": "instance-000187" }, processors: 8.5, memory: "64gb", storage: "128gb", node_version: $es_version } + allowed_warnings: + - "[version removal] Specifying node_version in desired nodes requests is deprecated." + - match: { replaced_existing_history_id: false } + + - do: + _internal.get_desired_nodes: {} + - match: + $body: + history_id: "test" + version: 1 + nodes: + - { settings: { node: { name: "instance-000187" } }, processors: 8.5, memory: "64gb", storage: "128gb", node_version: $es_version } + + - do: + _internal.update_desired_nodes: + history_id: "test" + version: 2 + body: + nodes: + - { settings: { "node.name": "instance-000187" }, processors: 8.5, memory: "64gb", storage: "128gb", node_version: $es_version } + - { settings: { "node.name": "instance-000188" }, processors: 16.0, memory: "128gb", storage: "1tb", node_version: $es_version } + allowed_warnings: + - "[version removal] Specifying node_version in desired nodes requests is deprecated." + - match: { replaced_existing_history_id: false } + + - do: + _internal.get_desired_nodes: {} + + - match: { history_id: "test" } + - match: { version: 2 } + - length: { nodes: 2 } + - contains: { nodes: { settings: { node: { name: "instance-000187" } }, processors: 8.5, memory: "64gb", storage: "128gb", node_version: $es_version } } + - contains: { nodes: { settings: { node: { name: "instance-000188" } }, processors: 16.0, memory: "128gb", storage: "1tb", node_version: $es_version } } +--- "Test update move to a new history id": - skip: reason: "contains is a newly added assertion" @@ -144,6 +199,46 @@ teardown: _internal.get_desired_nodes: {} - match: { status: 404 } --- +"Test delete desired nodes with node_version generates a warning": + - skip: + features: allowed_warnings + - do: + cluster.state: {} + + - set: { master_node: master } + + - do: + nodes.info: {} + - set: { nodes.$master.version: es_version } + + - do: + _internal.update_desired_nodes: + history_id: "test" + version: 1 + body: + nodes: + - { settings: { "node.external_id": "instance-000187" }, processors: 8.0, memory: "64gb", storage: "128gb", node_version: $es_version } + allowed_warnings: + - "[version removal] Specifying node_version in desired nodes requests is deprecated." + - match: { replaced_existing_history_id: false } + + - do: + _internal.get_desired_nodes: {} + - match: + $body: + history_id: "test" + version: 1 + nodes: + - { settings: { node: { external_id: "instance-000187" } }, processors: 8.0, memory: "64gb", storage: "128gb", node_version: $es_version } + + - do: + _internal.delete_desired_nodes: {} + + - do: + catch: missing + _internal.get_desired_nodes: {} + - match: { status: 404 } +--- "Test update desired nodes is idempotent": - skip: reason: "contains is a newly added assertion" 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 fe72a59565cf..fb8559b19d81 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java @@ -14,6 +14,7 @@ import org.elasticsearch.TransportVersions; import org.elasticsearch.Version; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodeRole; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; @@ -21,6 +22,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.unit.Processors; import org.elasticsearch.core.Nullable; +import org.elasticsearch.core.UpdateForV9; import org.elasticsearch.features.NodeFeature; import org.elasticsearch.xcontent.ConstructingObjectParser; import org.elasticsearch.xcontent.ObjectParser; @@ -36,6 +38,7 @@ import java.util.Objects; import java.util.Set; import java.util.TreeSet; import java.util.function.Predicate; +import java.util.regex.Pattern; import static java.lang.String.format; import static org.elasticsearch.node.Node.NODE_EXTERNAL_ID_SETTING; @@ -55,6 +58,8 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl private static final ParseField PROCESSORS_RANGE_FIELD = new ParseField("processors_range"); private static final ParseField MEMORY_FIELD = new ParseField("memory"); private static final ParseField STORAGE_FIELD = new ParseField("storage"); + @UpdateForV9(owner = UpdateForV9.Owner.DISTRIBUTED_COORDINATION) // Remove deprecated field + private static final ParseField VERSION_FIELD = new ParseField("node_version"); public static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( "desired_node", @@ -64,7 +69,8 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl (Processors) args[1], (ProcessorsRange) args[2], (ByteSizeValue) args[3], - (ByteSizeValue) args[4] + (ByteSizeValue) args[4], + (String) args[5] ) ); @@ -98,6 +104,12 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl STORAGE_FIELD, ObjectParser.ValueType.STRING ); + parser.declareField( + ConstructingObjectParser.optionalConstructorArg(), + (p, c) -> p.text(), + VERSION_FIELD, + ObjectParser.ValueType.STRING + ); } private final Settings settings; @@ -106,9 +118,21 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl private final ByteSizeValue memory; private final ByteSizeValue storage; + @UpdateForV9(owner = UpdateForV9.Owner.DISTRIBUTED_COORDINATION) // Remove deprecated version field + private final String version; private final String externalId; private final Set roles; + @Deprecated + public DesiredNode(Settings settings, ProcessorsRange processorsRange, ByteSizeValue memory, ByteSizeValue storage, String version) { + this(settings, null, processorsRange, memory, storage, version); + } + + @Deprecated + public DesiredNode(Settings settings, double processors, ByteSizeValue memory, ByteSizeValue storage, String version) { + this(settings, Processors.of(processors), null, memory, storage, version); + } + public DesiredNode(Settings settings, ProcessorsRange processorsRange, ByteSizeValue memory, ByteSizeValue storage) { this(settings, null, processorsRange, memory, storage); } @@ -118,6 +142,17 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl } DesiredNode(Settings settings, Processors processors, ProcessorsRange processorsRange, ByteSizeValue memory, ByteSizeValue storage) { + this(settings, processors, processorsRange, memory, storage, null); + } + + DesiredNode( + Settings settings, + Processors processors, + ProcessorsRange processorsRange, + ByteSizeValue memory, + ByteSizeValue storage, + @Deprecated String version + ) { assert settings != null; assert memory != null; assert storage != null; @@ -151,6 +186,7 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl this.processorsRange = processorsRange; 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))); } @@ -174,7 +210,19 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl } else { version = Version.readVersion(in).toString(); } - return new DesiredNode(settings, processors, processorsRange, memory, storage); + return new DesiredNode(settings, processors, processorsRange, memory, storage, version); + } + + private static final Pattern SEMANTIC_VERSION_PATTERN = Pattern.compile("^(\\d+\\.\\d+\\.\\d+)\\D?.*"); + + private static Version parseLegacyVersion(String version) { + if (version != null) { + var semanticVersionMatcher = SEMANTIC_VERSION_PATTERN.matcher(version); + if (semanticVersionMatcher.matches()) { + return Version.fromString(semanticVersionMatcher.group(1)); + } + } + return null; } @Override @@ -191,9 +239,15 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl memory.writeTo(out); storage.writeTo(out); if (out.getTransportVersion().onOrAfter(TransportVersions.V_8_13_0)) { - out.writeOptionalString(null); + out.writeOptionalString(version); } else { - Version.writeVersion(Version.CURRENT, out); + Version parsedVersion = parseLegacyVersion(version); + if (version == null) { + // Some node is from before we made the version field not required. If so, fill in with the current node version. + Version.writeVersion(Version.CURRENT, out); + } else { + Version.writeVersion(parsedVersion, out); + } } } @@ -221,6 +275,14 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl } builder.field(MEMORY_FIELD.getPreferredName(), memory); builder.field(STORAGE_FIELD.getPreferredName(), storage); + addDeprecatedVersionField(builder); + } + + @UpdateForV9(owner = UpdateForV9.Owner.DISTRIBUTED_COORDINATION) // Remove deprecated field from response + private void addDeprecatedVersionField(XContentBuilder builder) throws IOException { + if (version != null) { + builder.field(VERSION_FIELD.getPreferredName(), version); + } } public boolean hasMasterRole() { @@ -304,6 +366,7 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl return Objects.equals(settings, that.settings) && Objects.equals(memory, that.memory) && Objects.equals(storage, that.storage) + && Objects.equals(version, that.version) && Objects.equals(externalId, that.externalId) && Objects.equals(roles, that.roles); } @@ -316,7 +379,7 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl @Override public int hashCode() { - return Objects.hash(settings, processors, processorsRange, memory, storage, externalId, roles); + return Objects.hash(settings, processors, processorsRange, memory, storage, version, externalId, roles); } @Override @@ -345,6 +408,10 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl + '}'; } + public boolean hasVersion() { + return Strings.isNullOrBlank(version) == false; + } + public record ProcessorsRange(Processors min, @Nullable Processors max) implements Writeable, ToXContentObject { private static final ParseField MIN_FIELD = new ParseField("min"); diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodeWithStatus.java b/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodeWithStatus.java index 606309adf205..7b89406be9aa 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodeWithStatus.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodeWithStatus.java @@ -44,12 +44,13 @@ public record DesiredNodeWithStatus(DesiredNode desiredNode, Status status) (Processors) args[1], (DesiredNode.ProcessorsRange) args[2], (ByteSizeValue) args[3], - (ByteSizeValue) args[4] + (ByteSizeValue) args[4], + (String) args[5] ), // An unknown status is expected during upgrades to versions >= STATUS_TRACKING_SUPPORT_VERSION // the desired node status would be populated when a node in the newer version is elected as // master, the desired nodes status update happens in NodeJoinExecutor. - args[5] == null ? Status.PENDING : (Status) args[5] + args[6] == null ? Status.PENDING : (Status) args[6] ) ); diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestUpdateDesiredNodesAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestUpdateDesiredNodesAction.java index b8e1fa0c836a..ec8bb6285bdd 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestUpdateDesiredNodesAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestUpdateDesiredNodesAction.java @@ -12,11 +12,13 @@ package org.elasticsearch.rest.action.admin.cluster; import org.elasticsearch.action.admin.cluster.desirednodes.UpdateDesiredNodesAction; import org.elasticsearch.action.admin.cluster.desirednodes.UpdateDesiredNodesRequest; import org.elasticsearch.client.internal.node.NodeClient; +import org.elasticsearch.cluster.metadata.DesiredNode; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.features.NodeFeature; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.RestToXContentListener; +import org.elasticsearch.xcontent.XContentParseException; import org.elasticsearch.xcontent.XContentParser; import java.io.IOException; @@ -65,6 +67,16 @@ public class RestUpdateDesiredNodesAction extends BaseRestHandler { ); } + if (clusterSupportsFeature.test(DesiredNode.DESIRED_NODE_VERSION_DEPRECATED)) { + if (updateDesiredNodesRequest.getNodes().stream().anyMatch(DesiredNode::hasVersion)) { + deprecationLogger.compatibleCritical("desired_nodes_version", VERSION_DEPRECATION_MESSAGE); + } + } else { + if (updateDesiredNodesRequest.getNodes().stream().anyMatch(n -> n.hasVersion() == false)) { + throw new XContentParseException("[node_version] field is required and must have a valid value"); + } + } + return restChannel -> client.execute( UpdateDesiredNodesAction.INSTANCE, updateDesiredNodesRequest,