Fix Decision.Type serialization BWC

docs/changelog/140199.yaml

@@ -0,0 +1,5 @@
+pr: 140199
+summary: Fix Decision.Type serialization BWC
+area: Allocation
+type: bug
+issues: []

server/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationDecision.java

@@ -72,7 +72,7 @@ public enum AllocationDecision implements Writeable {
 
     final byte id;
 
-    private static final TransportVersion ADD_NOT_PREFERRED_ALLOCATION_DECISION = TransportVersion.fromName(
+    public static final TransportVersion ADD_NOT_PREFERRED_ALLOCATION_DECISION = TransportVersion.fromName(
         "add_not_preferred_allocation_decision"
     );
 

server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/Decision.java

@@ -10,6 +10,7 @@
 package org.elasticsearch.cluster.routing.allocation.decider;
 
 import org.elasticsearch.TransportVersion;
+import org.elasticsearch.cluster.routing.allocation.AllocationDecision;
 import org.elasticsearch.cluster.routing.allocation.RoutingAllocation;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
@@ -124,13 +125,22 @@ enum Type implements Writeable {
         THROTTLE,
         YES;
 
-        private static final TransportVersion ALLOCATION_DECISION_NOT_PREFERRED = TransportVersion.fromName(
-            "allocation_decision_not_preferred"
-        );
+        // visible for testing
+        static final TransportVersion ALLOCATION_DECISION_NOT_PREFERRED = TransportVersion.fromName("allocation_decision_not_preferred");
 
         public static Type readFrom(StreamInput in) throws IOException {
-            if (in.getTransportVersion().supports(ALLOCATION_DECISION_NOT_PREFERRED)) {
+            if (in.getTransportVersion().supports(AllocationDecision.ADD_NOT_PREFERRED_ALLOCATION_DECISION)) {
                 return in.readEnum(Type.class);
+            } else if (in.getTransportVersion().supports(ALLOCATION_DECISION_NOT_PREFERRED)) {
+                int i = in.readVInt();
+                // the order of THROTTLE and NOT_PREFERRED was swapped
+                return switch (i) {
+                    case 0 -> NO;
+                    case 1 -> THROTTLE;
+                    case 2 -> NOT_PREFERRED;
+                    case 3 -> YES;
+                    default -> throw new IllegalArgumentException("No type for integer [" + i + "]");
+                };
             } else {
                 int i = in.readVInt();
                 return switch (i) {
@@ -151,8 +161,15 @@ public static Type min(Type a, Type b) {
 
         @Override
         public void writeTo(StreamOutput out) throws IOException {
-            if (out.getTransportVersion().supports(ALLOCATION_DECISION_NOT_PREFERRED)) {
+            if (out.getTransportVersion().supports(AllocationDecision.ADD_NOT_PREFERRED_ALLOCATION_DECISION)) {
                 out.writeEnum(this);
+            } else if (out.getTransportVersion().supports(ALLOCATION_DECISION_NOT_PREFERRED)) {
+                // the order of THROTTLE and NOT_PREFERRED was swapped
+                out.writeVInt(switch (this) {
+                    case NOT_PREFERRED -> 2;
+                    case THROTTLE -> 1;
+                    default -> this.ordinal();
+                });
             } else {
                 out.writeVInt(switch (this) {
                     case NO -> 0;

server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/DecisionTests.java

@@ -9,11 +9,17 @@
 
 package org.elasticsearch.cluster.routing.allocation.decider;
 
+import org.elasticsearch.TransportVersion;
+import org.elasticsearch.cluster.routing.allocation.decider.Decision.Type;
+import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.io.stream.BytesStreamOutput;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.EnumSerializationTestUtils;
 
+import java.io.IOException;
 import java.util.List;
 
+import static org.elasticsearch.cluster.routing.allocation.decider.Decision.Type.ALLOCATION_DECISION_NOT_PREFERRED;
 import static org.elasticsearch.cluster.routing.allocation.decider.Decision.Type.NO;
 import static org.elasticsearch.cluster.routing.allocation.decider.Decision.Type.NOT_PREFERRED;
 import static org.elasticsearch.cluster.routing.allocation.decider.Decision.Type.THROTTLE;
@@ -24,8 +30,27 @@
  */
 public class DecisionTests extends ESTestCase {
 
+    /**
+     * The {@link Decision.Type} enum at {@link Decision.Type#ALLOCATION_DECISION_NOT_PREFERRED}
+     */
+    private enum AllocationDecisionNotPreferredType {
+        NO,
+        THROTTLE,
+        NOT_PREFERRED,
+        YES
+    }
+
+    /**
+     * The {@link Decision.Type} enum before {@link Decision.Type#ALLOCATION_DECISION_NOT_PREFERRED}
+     */
+    private enum OriginalType {
+        NO,
+        YES,
+        THROTTLE
+    }
+
     public void testTypeEnumOrder() {
-        EnumSerializationTestUtils.assertEnumSerialization(Decision.Type.class, NO, NOT_PREFERRED, THROTTLE, YES);
+        EnumSerializationTestUtils.assertEnumSerialization(Type.class, NO, NOT_PREFERRED, THROTTLE, YES);
     }
 
     public void testTypeHigherThan() {
@@ -37,4 +62,94 @@ public void testTypeAllowed() {
         List.of(NO, THROTTLE).forEach(d -> assertFalse(d.assignmentAllowed()));
     }
 
+    public void testSerializationBackwardCompatibility() throws IOException {
+        testReadWriteEnum(
+            YES,
+            AllocationDecisionNotPreferredType.class,
+            AllocationDecisionNotPreferredType.YES,
+            ALLOCATION_DECISION_NOT_PREFERRED
+        );
+        testReadWriteEnum(
+            NOT_PREFERRED,
+            AllocationDecisionNotPreferredType.class,
+            AllocationDecisionNotPreferredType.NOT_PREFERRED,
+            ALLOCATION_DECISION_NOT_PREFERRED
+        );
+        testReadWriteEnum(
+            THROTTLE,
+            AllocationDecisionNotPreferredType.class,
+            AllocationDecisionNotPreferredType.THROTTLE,
+            ALLOCATION_DECISION_NOT_PREFERRED
+        );
+        testReadWriteEnum(
+            NO,
+            AllocationDecisionNotPreferredType.class,
+            AllocationDecisionNotPreferredType.NO,
+            ALLOCATION_DECISION_NOT_PREFERRED
+        );
+
+        // Older versions, lossy serialization - remove when no longer supported
+        assertFalse(TransportVersion.minimumCompatible().supports(ALLOCATION_DECISION_NOT_PREFERRED));
+        // YES/NOT_PREFERRED turn into YES in those versions, both round-trip to YES
+        testReadWriteEnum(YES, OriginalType.class, OriginalType.YES, TransportVersion.minimumCompatible());
+        testReadWriteEnum(NOT_PREFERRED, OriginalType.class, OriginalType.YES, YES, TransportVersion.minimumCompatible());
+        // THROTTLE and NO are unchanged
+        testReadWriteEnum(THROTTLE, OriginalType.class, OriginalType.THROTTLE, TransportVersion.minimumCompatible());
+        testReadWriteEnum(NO, OriginalType.class, OriginalType.NO, TransportVersion.minimumCompatible());
+    }
+
+    /**
+     * Test the reading and writing of an enum to a specific transport version (assuming lossless roundtrip)
+     *
+     * @param value The value to write
+     * @param remoteEnum The enum to use for deserialization
+     * @param expectedSerialisedValue The expected deserialized value
+     * @param remoteTransportVersion The transport version to use for serialization
+     * @param <E> The remote enum type
+     */
+    private <E extends Enum<E>> void testReadWriteEnum(
+        Decision.Type value,
+        Class<E> remoteEnum,
+        E expectedSerialisedValue,
+        TransportVersion remoteTransportVersion
+    ) throws IOException {
+        testReadWriteEnum(value, remoteEnum, expectedSerialisedValue, value, remoteTransportVersion);
+    }
+
+    /**
+     * Test the reading and writing of an enum to a specific transport version
+     *
+     * @param value The value to write
+     * @param remoteEnum The enum to use for deserialization
+     * @param expectedSerialisedValue The expected deserialized value
+     * @param roundTripValue The expected deserialized value after round-tripping
+     * @param remoteTransportVersion The transport version to use for serialization
+     * @param <E> The remote enum type
+     */
+    private <E extends Enum<E>> void testReadWriteEnum(
+        Decision.Type value,
+        Class<E> remoteEnum,
+        E expectedSerialisedValue,
+        Decision.Type roundTripValue,
+        TransportVersion remoteTransportVersion
+    ) throws IOException {
+        final var output = new BytesStreamOutput();
+        output.setTransportVersion(remoteTransportVersion);
+        value.writeTo(output);
+        assertEquals(expectedSerialisedValue, output.bytes().streamInput().readEnum(remoteEnum));
+        expectValue(roundTripValue, remoteTransportVersion, output.bytes());
+    }
+
+    /**
+     * Expect a value to be deserialized when read as a specific transport version
+     *
+     * @param expected The {@link Decision.Type} we expect to read
+     * @param readAsTransportVersion The TransportVersion to interpret the bytes as coming from
+     * @param bytes The bytes to read
+     */
+    private void expectValue(Decision.Type expected, TransportVersion readAsTransportVersion, BytesReference bytes) throws IOException {
+        final var currentValueInput = bytes.streamInput();
+        currentValueInput.setTransportVersion(readAsTransportVersion);
+        assertEquals(expected, Type.readFrom(currentValueInput));
+    }
 }