Inject migrate action regardless of allocate action

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/MigrateAction.java

@@ -36,7 +36,7 @@ public class MigrateAction implements LifecycleAction {
     public static final ParseField ENABLED_FIELD = new ParseField("enabled");
 
     private static final Logger logger = LogManager.getLogger(MigrateAction.class);
-    static final String CONDITIONAL_SKIP_MIGRATE_STEP = BranchingStep.NAME + "-check-skip-action";
+    public static final String CONDITIONAL_SKIP_MIGRATE_STEP = BranchingStep.NAME + "-check-skip-action";
 
     private static final ConstructingObjectParser<MigrateAction, Void> PARSER = new ConstructingObjectParser<>(NAME,
         a -> new MigrateAction(a[0] == null ? true : (boolean) a[0]));

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java

@@ -124,22 +124,17 @@ public List<Phase> getOrderedPhases(Map<String, Phase> phases) {
     }
 
     public static boolean shouldInjectMigrateStepForPhase(Phase phase) {
-        AllocateAction allocateAction = (AllocateAction) phase.getActions().get(AllocateAction.NAME);
-        if (allocateAction != null) {
-            if (definesAllocationRules(allocateAction)) {
-                // we won't automatically migrate the data if an allocate action that defines any allocation rule is present
-                return false;
-            }
-        }
-
+        // searchable snapshots automatically set their own allocation rules, no need to configure them with a migrate step.
         if (phase.getActions().get(SearchableSnapshotAction.NAME) != null) {
-            // Searchable snapshots automatically set their own allocation rules, no need to configure them with a migrate step.
             return false;
         }
 
-        MigrateAction migrateAction = (MigrateAction) phase.getActions().get(MigrateAction.NAME);
         // if the user configured the {@link MigrateAction} already we won't automatically configure it
-        return migrateAction == null;
+        if (phase.getActions().get(MigrateAction.NAME) != null) {
+            return false;
+        }
+
+        return true;
     }
 
     @Override

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java

@@ -623,51 +623,27 @@ public void testGetNextActionName() {
 
     public void testShouldMigrateDataToTiers() {
         {
-            // the allocate action contain allocation rules
+            // there's an allocate action
             Map<String, LifecycleAction> actions = new HashMap<>();
-            actions.put(TEST_MIGRATE_ACTION.getWriteableName(), new MigrateAction(false));
-            actions.put(TEST_ALLOCATE_ACTION.getWriteableName(), TEST_ALLOCATE_ACTION);
-            Phase phase = new Phase(WARM_PHASE, TimeValue.ZERO, actions);
-            assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false));
-        }
-
-        {
-            // the allocate action only specifies the number of replicas
-            Map<String, LifecycleAction> actions = new HashMap<>();
-            actions.put(TEST_ALLOCATE_ACTION.getWriteableName(), new AllocateAction(2, 20, null, null, null));
+            actions.put(TEST_ALLOCATE_ACTION.getWriteableName(),
+                randomFrom(TEST_ALLOCATE_ACTION, new AllocateAction(2, 20, null, null, null)));
             Phase phase = new Phase(WARM_PHASE, TimeValue.ZERO, actions);
             assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(true));
         }
 
         {
-            // there's an enabled migrate action specified
-            Map<String, LifecycleAction> actions = new HashMap<>();
-            actions.put(TEST_MIGRATE_ACTION.getWriteableName(), new MigrateAction(true));
-            Phase phase = new Phase(WARM_PHASE, TimeValue.ZERO, actions);
-            assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false));
-        }
-
-        {
-            // there's a disabled migrate action specified
+            // there's a migrate action
             Map<String, LifecycleAction> actions = new HashMap<>();
-            actions.put(TEST_MIGRATE_ACTION.getWriteableName(), new MigrateAction(false));
+            actions.put(TEST_MIGRATE_ACTION.getWriteableName(), new MigrateAction(randomBoolean()));
             Phase phase = new Phase(WARM_PHASE, TimeValue.ZERO, actions);
             assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false));
         }
 
         {
-            // test phase defines a `searchable_snapshot` action
-            Map<String, LifecycleAction> actions = new HashMap<>();
-            actions.put(TEST_SEARCHABLE_SNAPSHOT_ACTION.getWriteableName(), TEST_SEARCHABLE_SNAPSHOT_ACTION);
-            Phase phase = new Phase(COLD_PHASE, TimeValue.ZERO, actions);
-            assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false));
-        }
-
-        {
-            // test `frozen` phase defines a `searchable_snapshot` action
+            // there's a searchable_snapshot action
             Map<String, LifecycleAction> actions = new HashMap<>();
             actions.put(TEST_SEARCHABLE_SNAPSHOT_ACTION.getWriteableName(), TEST_SEARCHABLE_SNAPSHOT_ACTION);
-            Phase phase = new Phase(FROZEN_PHASE, TimeValue.ZERO, actions);
+            Phase phase = new Phase(randomFrom(COLD_PHASE, FROZEN_PHASE), TimeValue.ZERO, actions);
             assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false));
         }
 

x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/cluster/metadata/MetadataMigrateToDataTiersRoutingServiceTests.java

@@ -231,14 +231,16 @@ public void testMigrateIlmPolicyRefreshesCachedPhase() {
 
             Map<String, Object> migratedPhaseDefAsMap = getPhaseDefinitionAsMap(newLifecycleState);
 
-            // expecting the phase definition to be refreshed with the index being in the shrink action
+            // expecting the phase definition to be refreshed with the index being in the migrate action.
+            // even though the policy above doesn't mention migrate specifically, it has been injected.
             Map<String, Object> actions = (Map<String, Object>) migratedPhaseDefAsMap.get("actions");
             assertThat(actions.size(), is(2));
             Map<String, Object> allocateDef = (Map<String, Object>) actions.get(AllocateAction.NAME);
             assertThat(allocateDef, nullValue());
-            assertThat(newLifecycleState.getAction(), is(ShrinkAction.NAME));
-            assertThat(newLifecycleState.getStep(), is(ShrinkAction.CONDITIONAL_SKIP_SHRINK_STEP));
+            assertThat(newLifecycleState.getAction(), is(MigrateAction.NAME));
+            assertThat(newLifecycleState.getStep(), is(MigrateAction.CONDITIONAL_SKIP_MIGRATE_STEP));
 
+            // the shrink and set_priority actions are unchanged
             Map<String, Object> shrinkDef = (Map<String, Object>) actions.get(ShrinkAction.NAME);
             assertThat(shrinkDef.get("number_of_shards"), is(2));
             Map<String, Object> setPriorityDef = (Map<String, Object>) actions.get(SetPriorityAction.NAME);