From 3c5064fd61bdb3ff393f1784e60af3fb1693fc07 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Tue, 14 Dec 2021 17:50:17 -0700 Subject: [PATCH] Change HLRC's LifecyclePolicy to allow all valid ILM actions (#81483) This commit changes the High Level Rest Client's `LifecyclePolicy` implementation to allow all the same valid actions for each phase that ILM supports. It was previously missing actions in the hot and cold phases, as well as missing the frozen phase entirely. This also adds a test that uses the "real" `TimeseriesLifecycleType.ORDERED_*ACTIONS` lists so that if an action is added or removed in the future the test will fail. Resolves #81461 --- .../client/ilm/LifecyclePolicy.java | 17 +++- .../client/ilm/MigrateAction.java | 1 + .../ilm/GetLifecyclePolicyResponseTests.java | 3 +- .../ilm/LifecyclePolicyMetadataTests.java | 3 +- .../client/ilm/LifecyclePolicyTests.java | 79 +++++++++++++------ .../core/ilm/TimeseriesLifecycleType.java | 20 ++--- .../ilm/TimeseriesLifecycleTypeTests.java | 4 +- 7 files changed, 88 insertions(+), 39 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/ilm/LifecyclePolicy.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/ilm/LifecyclePolicy.java index d5da9a717ce68..1f09704267091 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/ilm/LifecyclePolicy.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/ilm/LifecyclePolicy.java @@ -52,7 +52,18 @@ public class LifecyclePolicy implements ToXContentObject { PHASES_FIELD ); - ALLOWED_ACTIONS.put("hot", Sets.newHashSet(UnfollowAction.NAME, SetPriorityAction.NAME, RolloverAction.NAME)); + ALLOWED_ACTIONS.put( + "hot", + Sets.newHashSet( + UnfollowAction.NAME, + SetPriorityAction.NAME, + RolloverAction.NAME, + ReadOnlyAction.NAME, + ShrinkAction.NAME, + ForceMergeAction.NAME, + SearchableSnapshotAction.NAME + ) + ); ALLOWED_ACTIONS.put( "warm", Sets.newHashSet( @@ -73,9 +84,11 @@ public class LifecyclePolicy implements ToXContentObject { MigrateAction.NAME, AllocateAction.NAME, FreezeAction.NAME, - SearchableSnapshotAction.NAME + SearchableSnapshotAction.NAME, + ReadOnlyAction.NAME ) ); + ALLOWED_ACTIONS.put("frozen", Sets.newHashSet(UnfollowAction.NAME, SearchableSnapshotAction.NAME)); ALLOWED_ACTIONS.put("delete", Sets.newHashSet(DeleteAction.NAME, WaitForSnapshotAction.NAME)); } diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/ilm/MigrateAction.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/ilm/MigrateAction.java index 4df87a1443481..af236b08793ab 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/ilm/MigrateAction.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/ilm/MigrateAction.java @@ -24,6 +24,7 @@ public class MigrateAction implements LifecycleAction, ToXContentObject { private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( NAME, + true, a -> new MigrateAction(a[0] == null ? true : (boolean) a[0]) ); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/GetLifecyclePolicyResponseTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/GetLifecyclePolicyResponseTests.java index 807bd2130177e..55b3da746330d 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/GetLifecyclePolicyResponseTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/GetLifecyclePolicyResponseTests.java @@ -86,7 +86,8 @@ protected NamedXContentRegistry xContentRegistry() { new ParseField(SearchableSnapshotAction.NAME), SearchableSnapshotAction::parse ), - new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(UnfollowAction.NAME), UnfollowAction::parse) + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(UnfollowAction.NAME), UnfollowAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(MigrateAction.NAME), MigrateAction::parse) ) ); return new NamedXContentRegistry(entries); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/LifecyclePolicyMetadataTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/LifecyclePolicyMetadataTests.java index 8377abbb51185..85df82b2a13a6 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/LifecyclePolicyMetadataTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/LifecyclePolicyMetadataTests.java @@ -80,7 +80,8 @@ protected NamedXContentRegistry xContentRegistry() { new ParseField(SearchableSnapshotAction.NAME), SearchableSnapshotAction::parse ), - new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(UnfollowAction.NAME), UnfollowAction::parse) + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(UnfollowAction.NAME), UnfollowAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(MigrateAction.NAME), MigrateAction::parse) ) ); return new NamedXContentRegistry(entries); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/LifecyclePolicyTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/LifecyclePolicyTests.java index 354a3d04c0a5f..8335a51a71b5f 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/LifecyclePolicyTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/LifecyclePolicyTests.java @@ -8,17 +8,19 @@ package org.elasticsearch.client.ilm; import org.elasticsearch.cluster.ClusterModule; -import org.elasticsearch.common.util.set.Sets; +import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.core.TimeValue; import org.elasticsearch.test.AbstractXContentTestCase; import org.elasticsearch.xcontent.NamedXContentRegistry; import org.elasticsearch.xcontent.ParseField; import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -29,23 +31,11 @@ import static org.hamcrest.Matchers.equalTo; public class LifecyclePolicyTests extends AbstractXContentTestCase { - private static final Set VALID_HOT_ACTIONS = Sets.newHashSet(UnfollowAction.NAME, SetPriorityAction.NAME, RolloverAction.NAME); - private static final Set VALID_WARM_ACTIONS = Sets.newHashSet( - UnfollowAction.NAME, - SetPriorityAction.NAME, - AllocateAction.NAME, - ForceMergeAction.NAME, - ReadOnlyAction.NAME, - ShrinkAction.NAME - ); - private static final Set VALID_COLD_ACTIONS = Sets.newHashSet( - UnfollowAction.NAME, - SetPriorityAction.NAME, - AllocateAction.NAME, - FreezeAction.NAME, - SearchableSnapshotAction.NAME - ); - private static final Set VALID_DELETE_ACTIONS = Sets.newHashSet(DeleteAction.NAME, WaitForSnapshotAction.NAME); + private static final Set VALID_HOT_ACTIONS = new HashSet<>(TimeseriesLifecycleType.ORDERED_VALID_HOT_ACTIONS); + private static final Set VALID_WARM_ACTIONS = new HashSet<>(TimeseriesLifecycleType.ORDERED_VALID_WARM_ACTIONS); + private static final Set VALID_COLD_ACTIONS = new HashSet<>(TimeseriesLifecycleType.ORDERED_VALID_COLD_ACTIONS); + private static final Set VALID_FROZEN_ACTIONS = new HashSet<>(TimeseriesLifecycleType.ORDERED_VALID_FROZEN_ACTIONS); + private static final Set VALID_DELETE_ACTIONS = new HashSet<>(TimeseriesLifecycleType.ORDERED_VALID_DELETE_ACTIONS); private String lifecycleName; @@ -88,7 +78,8 @@ protected NamedXContentRegistry xContentRegistry() { new ParseField(SearchableSnapshotAction.NAME), SearchableSnapshotAction::parse ), - new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(UnfollowAction.NAME), UnfollowAction::parse) + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(UnfollowAction.NAME), UnfollowAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(MigrateAction.NAME), MigrateAction::parse) ) ); return new NamedXContentRegistry(entries); @@ -128,13 +119,17 @@ public void testValidateHotPhase() { .map(this::getTestAction) .collect(Collectors.toMap(LifecycleAction::getName, Function.identity())); if (randomBoolean()) { - invalidAction = getTestAction(randomFrom("allocate", "forcemerge", "delete", "shrink")); + invalidAction = getTestAction(randomFrom("allocate", "migrate", "delete")); actions.put(invalidAction.getName(), invalidAction); } Map hotPhase = Collections.singletonMap("hot", new Phase("hot", TimeValue.ZERO, actions)); if (invalidAction != null) { - Exception e = expectThrows(IllegalArgumentException.class, () -> new LifecyclePolicy(lifecycleName, hotPhase)); + Exception e = expectThrows( + IllegalArgumentException.class, + "expected " + invalidAction + " to throw but it didn't", + () -> new LifecyclePolicy(lifecycleName, hotPhase) + ); assertThat(e.getMessage(), equalTo("invalid action [" + invalidAction.getName() + "] defined in phase [hot]")); } else { new LifecyclePolicy(lifecycleName, hotPhase); @@ -147,7 +142,7 @@ public void testValidateWarmPhase() { .map(this::getTestAction) .collect(Collectors.toMap(LifecycleAction::getName, Function.identity())); if (randomBoolean()) { - invalidAction = getTestAction(randomFrom("rollover", "delete")); + invalidAction = getTestAction(randomFrom("rollover", "delete", "searchable_snapshot")); actions.put(invalidAction.getName(), invalidAction); } Map warmPhase = Collections.singletonMap("warm", new Phase("warm", TimeValue.ZERO, actions)); @@ -179,6 +174,25 @@ public void testValidateColdPhase() { } } + public void testValidateFrozenPhase() { + LifecycleAction invalidAction = null; + Map actions = randomSubsetOf(VALID_FROZEN_ACTIONS).stream() + .map(this::getTestAction) + .collect(Collectors.toMap(LifecycleAction::getName, Function.identity())); + if (randomBoolean()) { + invalidAction = getTestAction(randomFrom("allocate", "rollover", "delete", "forcemerge", "shrink", "readonly")); + actions.put(invalidAction.getName(), invalidAction); + } + Map coldPhase = Collections.singletonMap("cold", new Phase("frozen", TimeValue.ZERO, actions)); + + if (invalidAction != null) { + Exception e = expectThrows(IllegalArgumentException.class, () -> new LifecyclePolicy(lifecycleName, coldPhase)); + assertThat(e.getMessage(), equalTo("invalid action [" + invalidAction.getName() + "] defined in phase [frozen]")); + } else { + new LifecyclePolicy(lifecycleName, coldPhase); + } + } + public void testValidateDeletePhase() { LifecycleAction invalidAction = null; Map actions = VALID_DELETE_ACTIONS.stream() @@ -260,12 +274,15 @@ public static LifecyclePolicy createRandomPolicy(String lifecycleName) { case UnfollowAction.NAME: return new UnfollowAction(); case SearchableSnapshotAction.NAME: - return SearchableSnapshotActionTests.randomInstance(); + return new SearchableSnapshotAction("repo", randomBoolean()); + case MigrateAction.NAME: + return new MigrateAction(randomBoolean()); default: throw new IllegalArgumentException("invalid action [" + action + "]"); } }; TimeValue prev = null; + boolean searchableSnapshotSeen = false; for (String phase : phaseNames) { TimeValue after = prev == null ? TimeValue.parseTimeValue(randomTimeValue(0, 10000, "s", "m", "h", "d"), "test_after") @@ -278,6 +295,20 @@ public static LifecyclePolicy createRandomPolicy(String lifecycleName) { } else { actionNames = randomSubsetOf(randomIntBetween(1, validActions.apply(phase).size()), validActions.apply(phase)); } + if ("hot".equals(phase)) { + actions.put( + RolloverAction.NAME, + new RolloverAction(null, new ByteSizeValue(randomNonNegativeLong()), null, randomNonNegativeLong()) + ); + } + if (searchableSnapshotSeen || actionNames.contains(SearchableSnapshotAction.NAME)) { + searchableSnapshotSeen = true; + // let's make sure phases don't configure actions that conflict with the `searchable_snapshot` action + actionNames.removeAll(TimeseriesLifecycleType.ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT); + } + if (actionNames.contains(MigrateAction.NAME)) { + actionNames.remove(AllocateAction.NAME); + } for (String action : actionNames) { actions.put(action, randomAction.apply(action)); } @@ -310,6 +341,8 @@ private LifecycleAction getTestAction(String actionName) { return SearchableSnapshotActionTests.randomInstance(); case UnfollowAction.NAME: return new UnfollowAction(); + case MigrateAction.NAME: + return new MigrateAction(randomBoolean()); default: throw new IllegalArgumentException("unsupported phase action [" + actionName + "]"); } 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 7b9052371e28d..06bb15e0d0a66 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 @@ -49,9 +49,9 @@ public class TimeseriesLifecycleType implements LifecycleType { static final String COLD_PHASE = "cold"; static final String FROZEN_PHASE = "frozen"; static final String DELETE_PHASE = "delete"; - static final List ORDERED_VALID_PHASES = Arrays.asList(HOT_PHASE, WARM_PHASE, COLD_PHASE, FROZEN_PHASE, DELETE_PHASE); + public static final List ORDERED_VALID_PHASES = List.of(HOT_PHASE, WARM_PHASE, COLD_PHASE, FROZEN_PHASE, DELETE_PHASE); - static final List ORDERED_VALID_HOT_ACTIONS = Stream.of( + public static final List ORDERED_VALID_HOT_ACTIONS = Stream.of( SetPriorityAction.NAME, UnfollowAction.NAME, RolloverAction.NAME, @@ -60,8 +60,8 @@ public class TimeseriesLifecycleType implements LifecycleType { ShrinkAction.NAME, ForceMergeAction.NAME, SearchableSnapshotAction.NAME - ).filter(Objects::nonNull).collect(toList()); - static final List ORDERED_VALID_WARM_ACTIONS = Arrays.asList( + ).filter(Objects::nonNull).toList(); + public static final List ORDERED_VALID_WARM_ACTIONS = Arrays.asList( SetPriorityAction.NAME, UnfollowAction.NAME, ReadOnlyAction.NAME, @@ -70,7 +70,7 @@ public class TimeseriesLifecycleType implements LifecycleType { ShrinkAction.NAME, ForceMergeAction.NAME ); - static final List ORDERED_VALID_COLD_ACTIONS = Stream.of( + public static final List ORDERED_VALID_COLD_ACTIONS = Stream.of( SetPriorityAction.NAME, UnfollowAction.NAME, ReadOnlyAction.NAME, @@ -79,9 +79,9 @@ public class TimeseriesLifecycleType implements LifecycleType { MigrateAction.NAME, FreezeAction.NAME, RollupV2.isEnabled() ? RollupILMAction.NAME : null - ).filter(Objects::nonNull).collect(toList()); - static final List ORDERED_VALID_FROZEN_ACTIONS = Arrays.asList(UnfollowAction.NAME, SearchableSnapshotAction.NAME); - static final List ORDERED_VALID_DELETE_ACTIONS = Arrays.asList(WaitForSnapshotAction.NAME, DeleteAction.NAME); + ).filter(Objects::nonNull).toList(); + public static final List ORDERED_VALID_FROZEN_ACTIONS = List.of(UnfollowAction.NAME, SearchableSnapshotAction.NAME); + public static final List ORDERED_VALID_DELETE_ACTIONS = List.of(WaitForSnapshotAction.NAME, DeleteAction.NAME); static final Set VALID_HOT_ACTIONS = Sets.newHashSet(ORDERED_VALID_HOT_ACTIONS); static final Set VALID_WARM_ACTIONS = Sets.newHashSet(ORDERED_VALID_WARM_ACTIONS); @@ -111,8 +111,8 @@ public class TimeseriesLifecycleType implements LifecycleType { ); // Set of actions that cannot be defined (executed) after the managed index has been mounted as searchable snapshot. // It's ordered to produce consistent error messages which can be unit tested. - static final Set ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT = new LinkedHashSet<>( - Arrays.asList(ForceMergeAction.NAME, FreezeAction.NAME, ShrinkAction.NAME, RollupILMAction.NAME) + public static final Set ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT = Collections.unmodifiableSet( + new LinkedHashSet<>(Arrays.asList(ForceMergeAction.NAME, FreezeAction.NAME, ShrinkAction.NAME, RollupILMAction.NAME)) ); private TimeseriesLifecycleType() {} 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 4f2d92b2875e9..3ce4bdd0132ef 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 @@ -180,8 +180,8 @@ public void testValidateFrozenPhase() { Map actions = randomSubsetOf(VALID_FROZEN_ACTIONS).stream() .map(this::getTestAction) .collect(Collectors.toMap(LifecycleAction::getWriteableName, Function.identity())); - // regardless of the randomised actions, we must have a SearchableSnapshotAction in frozen - actions.put(SearchableSnapshotAction.NAME, getTestAction(SearchableSnapshotAction.NAME)); + // Frozen requires the searchable snapshot action to be present + actions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo", randomBoolean())); if (randomBoolean()) { invalidAction = getTestAction(randomFrom("rollover", "delete", "forcemerge", "shrink")); actions.put(invalidAction.getWriteableName(), invalidAction);