diff --git a/docs/changelog/140199.yaml b/docs/changelog/140199.yaml new file mode 100644 index 0000000000000..7d36022b9b48a --- /dev/null +++ b/docs/changelog/140199.yaml @@ -0,0 +1,5 @@ +pr: 140199 +summary: Fix Decision.Type serialization BWC +area: Allocation +type: bug +issues: [] diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationDecision.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationDecision.java index 1e6123ea6a9e2..2de9ae108085c 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationDecision.java +++ b/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" ); diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/Decision.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/Decision.java index e0682b507cbcb..406e5b122cf46 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/Decision.java +++ b/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; diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/DecisionTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/DecisionTests.java index 95db962b3b289..b7ec9e2b81709 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/decider/DecisionTests.java +++ b/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 The remote enum type + */ + private > void testReadWriteEnum( + Decision.Type value, + Class 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 The remote enum type + */ + private > void testReadWriteEnum( + Decision.Type value, + Class 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)); + } }