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

arteam · web-flow · commit c64226c3503b · 2024-10-24T18:19:14.000+02:00
It was deprecated in elastic#104209 (8.13) and shouldn't be set or returned in 9.0 The Desired Nodes API is an internal API, and users shouldn't depend on its backward compatibility.
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,7 +11,6 @@
 
 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;
@@ -82,8 +81,7 @@ private void assertDesiredNodesUpdatedWithRoundedUpFloatsAreIdempotent() throws
                     Settings.builder().put(NODE_NAME_SETTING.getKey(), nodeName).build(),
                     1238.49922909,
                     ByteSizeValue.ofGb(32),
-                    ByteSizeValue.ofGb(128),
-                    clusterHasFeature(DesiredNode.DESIRED_NODE_VERSION_DEPRECATED) ? null : Build.current().version()
+                    ByteSizeValue.ofGb(128)
                 )
             )
             .toList();
@@ -153,8 +151,7 @@ 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)),
-                        clusterHasFeature(DesiredNode.DESIRED_NODE_VERSION_DEPRECATED) ? null : Build.current().version()
+                        ByteSizeValue.ofGb(randomIntBetween(128, 256))
                     )
                 )
                 .toList();
@@ -167,8 +164,7 @@ 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)),
-                    clusterHasFeature(DesiredNode.DESIRED_NODE_VERSION_DEPRECATED) ? null : Build.current().version()
+                    ByteSizeValue.ofGb(randomIntBetween(128, 256))
                 );
             }).toList();
         }
@@ -182,8 +178,7 @@ 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)),
-                    clusterHasFeature(DesiredNode.DESIRED_NODE_VERSION_DEPRECATED) ? null : Build.current().version()
+                    ByteSizeValue.ofGb(randomIntBetween(128, 256))
                 )
             )
             .toList();
diff --git a/rest-api-spec/build.gradle b/rest-api-spec/build.gradle
@@ -61,4 +61,6 @@ 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,61 +59,6 @@ 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"
@@ -199,46 +144,6 @@ 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,15 +14,13 @@
 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;
@@ -38,7 +36,6 @@
 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;
@@ -58,8 +55,6 @@ 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",
@@ -69,8 +64,7 @@ public final class DesiredNode implements Writeable, ToXContentObject, Comparabl
             (Processors) args[1],
             (ProcessorsRange) args[2],
             (ByteSizeValue) args[3],
-            (ByteSizeValue) args[4],
-            (String) args[5]
+            (ByteSizeValue) args[4]
         )
     );
 
@@ -104,12 +98,6 @@ 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;
@@ -118,21 +106,9 @@ 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);
     }
@@ -142,17 +118,6 @@ 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;
@@ -186,7 +151,6 @@ 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)));
     }
@@ -210,19 +174,7 @@ public static DesiredNode readFrom(StreamInput in) throws IOException {
         } else {
             version = Version.readVersion(in).toString();
         }
-        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;
+        return new DesiredNode(settings, processors, processorsRange, memory, storage);
     }
 
     @Override
@@ -239,15 +191,9 @@ public void writeTo(StreamOutput out) throws IOException {
         memory.writeTo(out);
         storage.writeTo(out);
         if (out.getTransportVersion().onOrAfter(TransportVersions.V_8_13_0)) {
-            out.writeOptionalString(version);
+            out.writeOptionalString(null);
         } else {
-            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);
-            }
+            Version.writeVersion(Version.CURRENT, out);
         }
     }
 
@@ -275,14 +221,6 @@ 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() {
@@ -366,7 +304,6 @@ 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);
     }
@@ -379,7 +316,7 @@ public boolean equalsWithProcessorsCloseTo(DesiredNode that) {
 
     @Override
     public int hashCode() {
-        return Objects.hash(settings, processors, processorsRange, memory, storage, version, externalId, roles);
+        return Objects.hash(settings, processors, processorsRange, memory, storage, externalId, roles);
     }
 
     @Override
@@ -408,10 +345,6 @@ 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,13 +44,12 @@ public record DesiredNodeWithStatus(DesiredNode desiredNode, Status status)
                 (Processors) args[1],
                 (DesiredNode.ProcessorsRange) args[2],
                 (ByteSizeValue) args[3],
-                (ByteSizeValue) args[4],
-                (String) args[5]
+                (ByteSizeValue) args[4]
             ),
             // 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[6] == null ? Status.PENDING : (Status) args[6]
+            args[5] == null ? Status.PENDING : (Status) args[5]
         )
     );
 
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,13 +12,11 @@
 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;
@@ -67,16 +65,6 @@ 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,
