From 25ab14b2544d49621dbf8b5e83f3457e653d3540 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 13 Oct 2021 12:42:21 -0400 Subject: [PATCH 01/10] Remove this extraneous MigrateAction --- .../xpack/core/ilm/TimeseriesLifecycleTypeTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java index 3f1eece8cfbe5..e9101c4f542b3 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java @@ -625,7 +625,6 @@ public void testShouldMigrateDataToTiers() { { // the allocate action contain allocation rules Map 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)); From 23ef06fb3eca91877b7c0f6124aa52b2c1a81ce8 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 13 Oct 2021 12:45:41 -0400 Subject: [PATCH 02/10] Combine these tests IMHO the details don't matter much, and randomness captures the "doesn't matter / either way" components just fine. --- .../ilm/TimeseriesLifecycleTypeTests.java | 22 +++---------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java index e9101c4f542b3..255c5ed547f00 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java @@ -639,17 +639,9 @@ public void testShouldMigrateDataToTiers() { } { - // there's an enabled migrate action specified + // there's a migrate action specified Map 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 - Map 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)); } @@ -658,15 +650,7 @@ public void testShouldMigrateDataToTiers() { // test phase defines a `searchable_snapshot` action Map 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 - Map 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)); } From 68080422ea71318c04eb60036f93d37b78a42597 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 13 Oct 2021 12:47:40 -0400 Subject: [PATCH 03/10] Reorganize these just a little bit It reads more clearly to me this way --- .../xpack/core/ilm/TimeseriesLifecycleType.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java index a38784fa3fdc2..037962c8f3e9d 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java @@ -132,14 +132,17 @@ public static boolean shouldInjectMigrateStepForPhase(Phase phase) { } } + // 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 From c126224fae9e71e952fe4456e4edeacf6df37e02 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 13 Oct 2021 12:48:45 -0400 Subject: [PATCH 04/10] Ignore the allocate action when injecting migrate --- .../xpack/core/ilm/TimeseriesLifecycleType.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java index 037962c8f3e9d..bb4b5f5fb3033 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java @@ -124,14 +124,6 @@ public List getOrderedPhases(Map 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) { return false; From 619dd893380190e43291d643208c9c55179dd77d Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 13 Oct 2021 12:51:34 -0400 Subject: [PATCH 05/10] Get these tests passing --- .../org/elasticsearch/xpack/core/ilm/MigrateAction.java | 2 +- .../xpack/core/ilm/TimeseriesLifecycleTypeTests.java | 4 ++-- .../MetadataMigrateToDataTiersRoutingServiceTests.java | 8 +++++--- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/MigrateAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/MigrateAction.java index 2210f03db46e5..041feca58a8e2 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/MigrateAction.java +++ b/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 PARSER = new ConstructingObjectParser<>(NAME, a -> new MigrateAction(a[0] == null ? true : (boolean) a[0])); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java index 255c5ed547f00..c1a3609b77880 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java @@ -623,11 +623,11 @@ public void testGetNextActionName() { public void testShouldMigrateDataToTiers() { { - // the allocate action contain allocation rules + // there's an allocate action that contains allocation rules Map actions = new HashMap<>(); actions.put(TEST_ALLOCATE_ACTION.getWriteableName(), TEST_ALLOCATE_ACTION); Phase phase = new Phase(WARM_PHASE, TimeValue.ZERO, actions); - assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(false)); + assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(true)); } { diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/cluster/metadata/MetadataMigrateToDataTiersRoutingServiceTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/cluster/metadata/MetadataMigrateToDataTiersRoutingServiceTests.java index f97bc1d376910..86e30fe5bc85a 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/cluster/metadata/MetadataMigrateToDataTiersRoutingServiceTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/cluster/metadata/MetadataMigrateToDataTiersRoutingServiceTests.java @@ -231,14 +231,16 @@ public void testMigrateIlmPolicyRefreshesCachedPhase() { Map 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 actions = (Map) migratedPhaseDefAsMap.get("actions"); assertThat(actions.size(), is(2)); Map allocateDef = (Map) 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 shrinkDef = (Map) actions.get(ShrinkAction.NAME); assertThat(shrinkDef.get("number_of_shards"), is(2)); Map setPriorityDef = (Map) actions.get(SetPriorityAction.NAME); From 93453b4460cd0765849b8116ebd7cbeb4bdc227d Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 13 Oct 2021 12:53:27 -0400 Subject: [PATCH 06/10] One last round of test cleanup --- .../core/ilm/TimeseriesLifecycleTypeTests.java | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java index c1a3609b77880..27cd87854d6f9 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java @@ -623,23 +623,16 @@ public void testGetNextActionName() { public void testShouldMigrateDataToTiers() { { - // there's an allocate action that contains allocation rules + // there's an allocate action Map actions = new HashMap<>(); - actions.put(TEST_ALLOCATE_ACTION.getWriteableName(), TEST_ALLOCATE_ACTION); + 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)); } { - // the allocate action only specifies the number of replicas - Map actions = new HashMap<>(); - actions.put(TEST_ALLOCATE_ACTION.getWriteableName(), new AllocateAction(2, 20, null, null, null)); - Phase phase = new Phase(WARM_PHASE, TimeValue.ZERO, actions); - assertThat(TimeseriesLifecycleType.shouldInjectMigrateStepForPhase(phase), is(true)); - } - - { - // there's a migrate action specified + // there's a migrate action Map actions = new HashMap<>(); actions.put(TEST_MIGRATE_ACTION.getWriteableName(), new MigrateAction(randomBoolean())); Phase phase = new Phase(WARM_PHASE, TimeValue.ZERO, actions); @@ -647,7 +640,7 @@ public void testShouldMigrateDataToTiers() { } { - // test phase defines a `searchable_snapshot` action + // there's a searchable_snapshot Map actions = new HashMap<>(); actions.put(TEST_SEARCHABLE_SNAPSHOT_ACTION.getWriteableName(), TEST_SEARCHABLE_SNAPSHOT_ACTION); Phase phase = new Phase(randomFrom(COLD_PHASE, FROZEN_PHASE), TimeValue.ZERO, actions); From 97d5b32fbf234a4cb18701e24229242c8decb1c7 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 13 Oct 2021 13:10:09 -0400 Subject: [PATCH 07/10] Oops, typo --- .../xpack/core/ilm/TimeseriesLifecycleTypeTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java index 27cd87854d6f9..5abfdc3914fff 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java @@ -640,7 +640,7 @@ public void testShouldMigrateDataToTiers() { } { - // there's a searchable_snapshot + // there's a searchable_snapshot action Map actions = new HashMap<>(); actions.put(TEST_SEARCHABLE_SNAPSHOT_ACTION.getWriteableName(), TEST_SEARCHABLE_SNAPSHOT_ACTION); Phase phase = new Phase(randomFrom(COLD_PHASE, FROZEN_PHASE), TimeValue.ZERO, actions); From ec8c6befd450585f3a57150f86c07b45fa363d83 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Fri, 15 Oct 2021 15:57:11 -0400 Subject: [PATCH 08/10] Update the ilm-migrate docs Automatic migration is no longer conditional on whether or not there's an applicable allocate action. --- docs/reference/ilm/actions/ilm-migrate.asciidoc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/reference/ilm/actions/ilm-migrate.asciidoc b/docs/reference/ilm/actions/ilm-migrate.asciidoc index f096fd98815c2..8244e495073cb 100644 --- a/docs/reference/ilm/actions/ilm-migrate.asciidoc +++ b/docs/reference/ilm/actions/ilm-migrate.asciidoc @@ -8,11 +8,11 @@ Moves the index to the <> that corresponds to the current phase by updating the <> index setting. {ilm-init} automatically injects the migrate action in the warm and cold -phases if no allocation options are specified with the <> 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. If you specify an <> action that only +modifies the number of index replicas, {ilm-init} reduces the number of +replicas before migrating the index. To prevent automatic migration, you +can explicitly include the migrate action and set the enabled option to +`false`. If the `cold` phase defines a <> the `migrate` action will not be injected automatically in the `cold` phase because the managed index will be From a4f045c17f781cb720e2bf557f56e4fec054cc12 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Fri, 15 Oct 2021 16:04:13 -0400 Subject: [PATCH 09/10] Better updates, and fixes a few missed spots --- docs/reference/ilm/actions/ilm-migrate.asciidoc | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/docs/reference/ilm/actions/ilm-migrate.asciidoc b/docs/reference/ilm/actions/ilm-migrate.asciidoc index 8244e495073cb..61ce2e1a4064d 100644 --- a/docs/reference/ilm/actions/ilm-migrate.asciidoc +++ b/docs/reference/ilm/actions/ilm-migrate.asciidoc @@ -8,9 +8,7 @@ Moves the index to the <> that corresponds to the current phase by updating the <> index setting. {ilm-init} automatically injects the migrate action in the warm and cold -phases. If you specify an <> action that only -modifies the number of index replicas, {ilm-init} reduces the number of -replicas before migrating the index. To prevent automatic migration, you +phases. To prevent automatic migration, you can explicitly include the migrate action and set the enabled option to `false`. @@ -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 <> 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 From d305312b95afe250eb5f92cde667ff666fa17be9 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Fri, 15 Oct 2021 16:15:10 -0400 Subject: [PATCH 10/10] Just one more spot --- .../data-management/migrate-index-allocation-filters.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/data-management/migrate-index-allocation-filters.asciidoc b/docs/reference/data-management/migrate-index-allocation-filters.asciidoc index be6b0d2c06067..2cd24fd8c4cc5 100644 --- a/docs/reference/data-management/migrate-index-allocation-filters.asciidoc +++ b/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 <> action into each phase to automatically transition the indices through the data tiers.