Revert "Don't return or accept node_version in the Desired Nodes API (elastic#114580)" (elastic#115829)

arteam · jfreden · commit ed69b7b53491 · 2024-11-04T14:32:00.000+01:00
This reverts commit c64226c.
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
@@ -11,6 +11,7 @@
 
 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 @@ private void assertDesiredNodesUpdatedWithRoundedUpFloatsAreIdempotent() throws
                     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 @@ private void addClusterNodesToDesiredNodesWithProcessorsOrProcessorRanges(int ve
                         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 @@ private void addClusterNodesToDesiredNodesWithProcessorsOrProcessorRanges(int ve
                     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 @@ private void addClusterNodesToDesiredNodesWithIntegerProcessors(int version) thr
                     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
@@ -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
@@ -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
@@ -14,13 +14,15 @@
 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;
 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.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<DesiredNode, Void> 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 @@ static <T> void configureParser(ConstructingObjectParser<T, Void> parser) {
             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 @@ static <T> void configureParser(ConstructingObjectParser<T, Void> parser) {
     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<DiscoveryNodeRole> 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 DesiredNode(Settings settings, double processors, ByteSizeValue memory, B
     }
 
     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 DesiredNode(Settings settings, double processors, ByteSizeValue memory, B
         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 static DesiredNode readFrom(StreamInput in) throws IOException {
         } 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 void writeTo(StreamOutput out) throws IOException {
         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 void toInnerXContent(XContentBuilder builder, Params params) throws IOExc
         }
         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 @@ private boolean equalsWithoutProcessorsSpecification(DesiredNode that) {
         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 boolean equalsWithProcessorsCloseTo(DesiredNode that) {
 
     @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 String toString() {
             + '}';
     }
 
+    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
@@ -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
@@ -12,11 +12,13 @@
 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 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
             );
         }
 
+        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,
