From c10ba9ee4cd1fe479a416f8b3aea448bb137c21c Mon Sep 17 00:00:00 2001 From: Andrei Dan Date: Mon, 15 Feb 2021 09:41:53 +0000 Subject: [PATCH] ILM: Allow `searchable_snapshot` to follow `searchable_snapshot` (#68864) We added this validation for the hot phase as that was the only place a searchable_snapshot action could precede another `searchable_snapshot` action. This is not the case anymore and we now support multiple actions in the same policy. This expands the validation to include the cases where the frozen action defines illegal actions after a searchable_snapshot defined in the cold phase. --- .../core/ilm/TimeseriesLifecycleType.java | 60 ++++++++++----- .../xpack/core/ilm/LifecyclePolicyTests.java | 24 +++++- .../ilm/TimeseriesLifecycleTypeTests.java | 76 ++++++++++++++++--- .../actions/SearchableSnapshotActionIT.java | 30 +++++--- 4 files changed, 150 insertions(+), 40 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 29a0d477452c9..f4e4385e5104c 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 @@ -20,6 +20,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -61,7 +62,7 @@ public class TimeseriesLifecycleType implements LifecycleType { ForceMergeAction.NAME, RollupILMAction.NAME, SearchableSnapshotAction.NAME); // a set of actions that cannot be defined (executed) after the managed index has been mounted as searchable snapshot static final Set ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT = Sets.newHashSet(ShrinkAction.NAME, ForceMergeAction.NAME, - FreezeAction.NAME, SearchableSnapshotAction.NAME, RollupILMAction.NAME); + FreezeAction.NAME, RollupILMAction.NAME); static { if (RollupV2.isEnabled()) { @@ -301,23 +302,48 @@ public void validate(Collection phases) { } static void validateActionsFollowingSearchableSnapshot(Collection phases) { - boolean hotPhaseContainsSearchableSnapshot = phases.stream() - .filter(phase -> HOT_PHASE.equals(phase.getName())) - .anyMatch(phase -> phase.getActions().containsKey(SearchableSnapshotAction.NAME)); - if (hotPhaseContainsSearchableSnapshot) { - String phasesDefiningIllegalActions = phases.stream() - // we're looking for prohibited actions in phases other than hot - .filter(phase -> HOT_PHASE.equals(phase.getName()) == false) - // filter the phases that define illegal actions - .filter(phase -> - Collections.disjoint(ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT, phase.getActions().keySet()) == false) - .map(Phase::getName) - .collect(Collectors.joining(",")); - if (Strings.hasText(phasesDefiningIllegalActions)) { - throw new IllegalArgumentException("phases [" + phasesDefiningIllegalActions + "] define one or more of " + - ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT + " actions which are not allowed after a " + - "managed index is mounted as a searchable snapshot"); + // invalid configurations can occur if searchable_snapshot is defined in the `hot` phase, with subsequent invalid actions + // being defined in the warm/cold/frozen phases, or if it is defined in the `cold` phase with subsequent invalid actions + // being defined in the frozen phase + + Optional hotPhaseWithSearchableSnapshot = phases.stream() + .filter(phase -> phase.getName().equals(HOT_PHASE)) + .filter(phase -> phase.getActions().containsKey(SearchableSnapshotAction.NAME)) + .findAny(); + + final List phasesFollowingSearchableSnapshot = new ArrayList<>(phases.size()); + if (hotPhaseWithSearchableSnapshot.isPresent()) { + for (Phase phase : phases) { + if (phase.getName().equals(HOT_PHASE) == false) { + phasesFollowingSearchableSnapshot.add(phase); + } } + } else { + // let's see if the cold phase defines `searchable_snapshot` + Optional coldPhaseWithSearchableSnapshot = phases.stream() + .filter(phase -> phase.getName().equals(COLD_PHASE)) + .filter(phase -> phase.getActions().containsKey(SearchableSnapshotAction.NAME)) + .findAny(); + if (coldPhaseWithSearchableSnapshot.isPresent()) { + for (Phase phase : phases) { + if (phase.getName().equals(FROZEN_PHASE)) { + phasesFollowingSearchableSnapshot.add(phase); + break; + } + } + } + } + + final String phasesDefiningIllegalActions = phasesFollowingSearchableSnapshot.stream() + // filter the phases that define illegal actions + .filter(phase -> + Collections.disjoint(ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT, phase.getActions().keySet()) == false) + .map(Phase::getName) + .collect(Collectors.joining(",")); + if (Strings.hasText(phasesDefiningIllegalActions)) { + throw new IllegalArgumentException("phases [" + phasesDefiningIllegalActions + "] define one or more of " + + ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT + " actions which are not allowed after a " + + "managed index is mounted as a searchable snapshot"); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java index 5774fd4ff9439..cd585cf1fdf74 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java @@ -136,7 +136,16 @@ public static LifecyclePolicy randomTimeseriesLifecyclePolicy(@Nullable String l phaseNames.add(0, TimeseriesLifecycleType.HOT_PHASE); } boolean hotPhaseContainsSearchableSnap = false; - for (String phase : phaseNames) { + boolean coldPhaseContainsSearchableSnap = false; + // let's order the phases so we can reason about actions in a previous phase in order to generate a random *valid* policy + List orderedPhases = new ArrayList<>(phaseNames.size()); + for (String validPhase : TimeseriesLifecycleType.VALID_PHASES) { + if (phaseNames.contains(validPhase)) { + orderedPhases.add(validPhase); + } + } + + for (String phase : orderedPhases) { TimeValue after = TimeValue.parseTimeValue(randomTimeValue(0, 1000000000, "s", "m", "h", "d"), "test_after"); Map actions = new HashMap<>(); List actionNames = randomSubsetOf(validActions.apply(phase)); @@ -150,12 +159,23 @@ public static LifecyclePolicy randomTimeseriesLifecyclePolicy(@Nullable String l if (actionNames.contains(SearchableSnapshotAction.NAME)) { hotPhaseContainsSearchableSnap = true; } - } else { + } + if (phase.equals(TimeseriesLifecycleType.COLD_PHASE)) { if (hotPhaseContainsSearchableSnap) { // let's make sure the other phases don't configure actions that conflict with a possible `searchable_snapshot` action // configured in the hot phase actionNames.removeAll(TimeseriesLifecycleType.ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT); } + + if (actionNames.contains(SearchableSnapshotAction.NAME)) { + coldPhaseContainsSearchableSnap = true; + } + } else { + if (hotPhaseContainsSearchableSnap || coldPhaseContainsSearchableSnap) { + // let's make sure the other phases don't configure actions that conflict with a possible `searchable_snapshot` action + // configured in a previous phase (hot/cold) + actionNames.removeAll(TimeseriesLifecycleType.ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT); + } } for (String action : actionNames) { 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 0193b96586096..a6a9b552cd2df 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 @@ -205,21 +205,75 @@ public void testValidateConflictingDataMigrationConfigurations() { } public void testActionsThatCannotFollowSearchableSnapshot() { - assertThat(ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT.size(), is(5)); + assertThat(ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT.size(), is(4)); assertThat(ACTIONS_CANNOT_FOLLOW_SEARCHABLE_SNAPSHOT, containsInAnyOrder(ShrinkAction.NAME, FreezeAction.NAME, - ForceMergeAction.NAME, RollupILMAction.NAME, SearchableSnapshotAction.NAME)); + ForceMergeAction.NAME, RollupILMAction.NAME)); } public void testValidateActionsFollowingSearchableSnapshot() { - Phase hotPhase = new Phase("hot", TimeValue.ZERO, Map.of(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo"))); - Phase warmPhase = new Phase("warm", TimeValue.ZERO, Map.of(ShrinkAction.NAME, new ShrinkAction(1, null))); - Phase coldPhase = new Phase("cold", TimeValue.ZERO, Map.of(FreezeAction.NAME, new FreezeAction())); - - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> TimeseriesLifecycleType.validateActionsFollowingSearchableSnapshot(List.of(hotPhase, warmPhase, coldPhase))); - assertThat(e.getMessage(), is( - "phases [warm,cold] define one or more of [searchable_snapshot, forcemerge, freeze, shrink, rollup] actions" + - " which are not allowed after a managed index is mounted as a searchable snapshot")); + { + Phase hotPhase = new Phase("hot", TimeValue.ZERO, Map.of(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo"))); + Phase warmPhase = new Phase("warm", TimeValue.ZERO, Map.of(ShrinkAction.NAME, new ShrinkAction(1, null))); + Phase coldPhase = new Phase("cold", TimeValue.ZERO, Map.of(FreezeAction.NAME, new FreezeAction())); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> TimeseriesLifecycleType.validateActionsFollowingSearchableSnapshot(List.of(hotPhase, warmPhase, coldPhase))); + assertThat(e.getMessage(), is( + "phases [warm,cold] define one or more of [forcemerge, freeze, shrink, rollup] actions" + + " which are not allowed after a managed index is mounted as a searchable snapshot")); + } + + { + Phase warmPhase = new Phase("warm", TimeValue.ZERO, + Map.of(ShrinkAction.NAME, new ShrinkAction(1, null))); + Phase coldPhase = new Phase("cold", TimeValue.ZERO, + Map.of(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo"))); + Phase frozenPhase = new Phase("frozen", TimeValue.ZERO, + Map.of(FreezeAction.NAME, new FreezeAction())); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> TimeseriesLifecycleType.validateActionsFollowingSearchableSnapshot(List.of(warmPhase, coldPhase, frozenPhase))); + assertThat(e.getMessage(), is( + "phases [frozen] define one or more of [forcemerge, freeze, shrink, rollup] actions" + + " which are not allowed after a managed index is mounted as a searchable snapshot")); + } + + { + Phase hotPhase = new Phase("hot", TimeValue.ZERO, + Map.of(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo"))); + Phase warmPhase = new Phase("warm", TimeValue.ZERO, + Map.of(ShrinkAction.NAME, new ShrinkAction(1, null))); + Phase coldPhase = new Phase("cold", TimeValue.ZERO, + Map.of(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo"))); + Phase frozenPhase = new Phase("frozen", TimeValue.ZERO, + Map.of(FreezeAction.NAME, new FreezeAction())); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> TimeseriesLifecycleType.validateActionsFollowingSearchableSnapshot(List.of(hotPhase, warmPhase, coldPhase, + frozenPhase))); + assertThat(e.getMessage(), is( + "phases [warm,frozen] define one or more of [forcemerge, freeze, shrink, rollup] actions" + + " which are not allowed after a managed index is mounted as a searchable snapshot")); + } + + { + Phase hot = new Phase("hot", TimeValue.ZERO, Map.of(RolloverAction.NAME, new RolloverAction(null, null, 1L), + SearchableSnapshotAction.NAME, new SearchableSnapshotAction(randomAlphaOfLengthBetween(4, 10)))); + Phase warm = new Phase("warm", TimeValue.ZERO, Map.of(ForceMergeAction.NAME, new ForceMergeAction(1, null))); + Phase cold = new Phase("cold", TimeValue.ZERO, Map.of(FreezeAction.NAME, new FreezeAction())); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> TimeseriesLifecycleType.validateActionsFollowingSearchableSnapshot(List.of(warm, hot, cold))); + assertThat(e.getMessage(), is( + "phases [warm,cold] define one or more of [forcemerge, freeze, shrink, rollup] actions" + + " which are not allowed after a managed index is mounted as a searchable snapshot")); + } + + { + Phase frozenPhase = new Phase("frozen", TimeValue.ZERO, Map.of(FreezeAction.NAME, new FreezeAction(), + SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo"))); + try { + TimeseriesLifecycleType.validateActionsFollowingSearchableSnapshot(List.of(frozenPhase)); + } catch (Exception e) { + fail("unexpected exception while validating phase [ "+ frozenPhase +" ] but got [" + e.getMessage()+ "]"); + } + } } public void testGetOrderedPhases() { diff --git a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java index 6c7a4d30e6a7f..75c636cb17415 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java @@ -51,6 +51,7 @@ import static java.util.Collections.singletonMap; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.xpack.TimeSeriesRestDriver.createComposableTemplate; +import static org.elasticsearch.xpack.TimeSeriesRestDriver.createIndexWithSettings; import static org.elasticsearch.xpack.TimeSeriesRestDriver.createNewSingletonPolicy; import static org.elasticsearch.xpack.TimeSeriesRestDriver.createPolicy; import static org.elasticsearch.xpack.TimeSeriesRestDriver.createSnapshotRepo; @@ -227,7 +228,7 @@ public void testCreateInvalidPolicy() { ) ); - assertThat(exception.getMessage(), is("phases [warm,cold] define one or more of [searchable_snapshot, forcemerge, freeze, shrink, rollup]" + + assertThat(exception.getMessage(), is("phases [warm,cold] define one or more of [forcemerge, freeze, shrink, rollup]" + " actions which are not allowed after a managed index is mounted as a searchable snapshot")); } @@ -462,23 +463,32 @@ public void testConvertingSearchableSnapshotFromFullToPartial() throws Exception @SuppressWarnings("unchecked") public void testConvertingPartialSearchableSnapshotIntoFull() throws Exception { - String index = "myindex-" + randomAlphaOfLength(4).toLowerCase(Locale.ROOT); + String index = "myindex-" + randomAlphaOfLength(4).toLowerCase(Locale.ROOT) +"-000001"; createSnapshotRepo(client(), snapshotRepo, randomBoolean()); - createPolicy(client(), policy, null, null, - new Phase("cold", TimeValue.ZERO, - singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(snapshotRepo, randomBoolean(), - MountSearchableSnapshotRequest.Storage.SHARED_CACHE))), + createPolicy(client(), policy, + new Phase("hot", TimeValue.ZERO, + Map.of( + SearchableSnapshotAction.NAME, new SearchableSnapshotAction(snapshotRepo, randomBoolean(), + MountSearchableSnapshotRequest.Storage.SHARED_CACHE), + RolloverAction.NAME, new RolloverAction(null, null, 1L))), + null, null, new Phase("frozen", TimeValue.ZERO, singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(snapshotRepo, randomBoolean(), MountSearchableSnapshotRequest.Storage.FULL_COPY))), null ); - createIndex(index, Settings.builder() - .put(LifecycleSettings.LIFECYCLE_NAME, policy) - .build()); + String alias = "alias-" + randomAlphaOfLengthBetween(5, 10); + createIndexWithSettings(client(), index, alias, Settings.builder() + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) + .put(LifecycleSettings.LIFECYCLE_NAME, policy) + .put(RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, alias), + true); + ensureGreen(index); - indexDocument(client(), index, true); + indexDocument(client(), alias, true); + rolloverMaxOneDocCondition(client(), alias); final String searchableSnapMountedIndexName = SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX + SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX + index;