Inject migrate action regardless of allocate action (#79090)

docs/reference/data-management/migrate-index-allocation-filters.asciidoc

@@ -69,7 +69,7 @@ node.roles [ data_hot, data_content ]
 === Remove custom allocation settings from existing {ilm-init} policies
 
 Update the allocate action for each lifecycle phase to remove the attribute-based
-allocation settings. This enables {ilm-init} to inject the
+allocation settings. {ilm-init} will inject a
 <<ilm-migrate,migrate>> action into each phase
 to automatically transition the indices through the data tiers.
 

docs/reference/ilm/actions/ilm-migrate.asciidoc

@@ -8,11 +8,9 @@ Moves the index to the <<data-tiers, data tier>> that corresponds
 to the current phase by updating the <<tier-preference-allocation-filter, `index.routing.allocation.include._tier_preference`>>
 index setting.
 {ilm-init} automatically injects the migrate action in the warm and cold
-phases if no allocation options are specified with the <<ilm-allocate, allocate>> action.
-If you specify an allocate action that only modifies the number of index
-replicas, {ilm-init} reduces the number of replicas before migrating the index.
-To prevent automatic migration without specifying allocation options,
-you can explicitly include the migrate action and set the enabled option to `false`.
+phases. To prevent automatic migration, you
+can explicitly include the migrate action and set the enabled option to
+`false`.
 
 If the `cold` phase defines a <<ilm-searchable-snapshot, searchable snapshot action>> the `migrate`
 action will not be injected automatically in the `cold` phase because the managed index will be
@@ -53,9 +51,9 @@ Defaults to `true`.
 [[ilm-enabled-migrate-ex]]
 ==== Example
 
-In the following policy, the allocate action is specified to reduce the number of replicas before {ilm-init} migrates the index to warm nodes.
+In the following policy, the <<ilm-allocate, allocate>> action is specified to reduce the number of replicas before {ilm-init} migrates the index to warm nodes.
 
-NOTE: Explicitly specifying the migrate action is not required--{ilm-init} automatically performs the migrate action unless you specify allocation options or disable migration.
+NOTE: Explicitly specifying the migrate action is not required--{ilm-init} automatically performs the migrate action unless you disable migration.
 
 [source,console]
 --------------------------------------------------
@@ -84,8 +82,6 @@ The migrate action in the following policy is disabled and
 the allocate action assigns the index to nodes that have a
 `rack_id` of _one_ or _two_.
 
-NOTE: Explicitly disabling the migrate action is not required--{ilm-init} does not inject the migrate action if you specify allocation options.
-
 [source,console]
 --------------------------------------------------
 PUT _ilm/policy/my_policy

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

@@ -234,14 +234,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);