[7.x] Restrict ILM frozen phase to searchable snapshot actions only (#70158) (#70170)

Backports the following commits to 7.x:

    Restrict ILM frozen phase to searchable snapshot actions only (#70158)
    Remove "frozen" from read only tests (#70173)

Author: dakrone

build.gradle

@@ -189,8 +189,8 @@ tasks.register("verifyVersions") {
  * after the backport of the backcompat code is complete.
  */
 
-boolean bwc_tests_enabled = true
-String bwc_tests_disabled_issue = "" /* place a PR link here when committing bwc changes */
+boolean bwc_tests_enabled = false
+String bwc_tests_disabled_issue = "https://github.com/elastic/elasticsearch/pull/70158" /* place a PR link here when committing bwc changes */
 /*
  * FIPS 140-2 behavior was fixed in 7.11.0. Before that there is no way to run elasticsearch in a
  * JVM that is properly configured to be in fips mode with BCFIPS. For now we need to disable

docs/reference/ilm/actions/ilm-searchable-snapshot.asciidoc

@@ -36,7 +36,8 @@ To keep the snapshot, set `delete_searchable_snapshot` to `false` in the delete
 `snapshot_repository`::
 (Required, string)
 Specifies where to store the snapshot. 
-See <<snapshots-register-repository>> for more information.
+See <<snapshots-register-repository>> for more information. In non-frozen phases the snapshot will
+be mounted as a `full_copy`, and in frozen phases mounted with the `shared_cache` storage type.
 
 `force_merge_index`::
 (Optional, Boolean)
@@ -52,12 +53,6 @@ the shards are relocating, in which case they will not be merged.
 The `searchable_snapshot` action will continue executing even if not all shards
 are force merged.
 
-`storage`::
-(Optional, string)
-Specifies the type of snapshot that should be mounted for a searchable snapshot. This corresponds to
-the <<searchable-snapshots-api-mount-query-params, `storage` option when mounting a snapshot>>.
-Defaults to `full_copy` in non-frozen phases, or `shared_cache` in the frozen phase.
-
 [[ilm-searchable-snapshot-ex]]
 ==== Examples
 [source,console]
@@ -69,8 +64,7 @@ PUT _ilm/policy/my_policy
       "cold": {
         "actions": {
           "searchable_snapshot" : {
-            "snapshot_repository" : "backing_repo",
-            "storage": "shared_cache"
+            "snapshot_repository" : "backing_repo"
           }
         }
       }

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

@@ -13,7 +13,6 @@
 import org.elasticsearch.cluster.health.ClusterHealthStatus;
 import org.elasticsearch.cluster.metadata.IndexAbstraction;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
-import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
@@ -44,7 +43,6 @@ public class SearchableSnapshotAction implements LifecycleAction {
 
     public static final ParseField SNAPSHOT_REPOSITORY = new ParseField("snapshot_repository");
     public static final ParseField FORCE_MERGE_INDEX = new ParseField("force_merge_index");
-    public static final ParseField STORAGE = new ParseField("storage");
     public static final String CONDITIONAL_DATASTREAM_CHECK_KEY = BranchingStep.NAME + "-on-datastream-check";
     public static final String CONDITIONAL_SKIP_ACTION_STEP = BranchingStep.NAME + "-check-prerequisites";
     public static final String CONDITIONAL_SKIP_GENERATE_AND_CLEAN = BranchingStep.NAME + "-check-existing-snapshot";
@@ -53,21 +51,11 @@ public class SearchableSnapshotAction implements LifecycleAction {
     public static final String PARTIAL_RESTORED_INDEX_PREFIX = "partial-";
 
     private static final ConstructingObjectParser<SearchableSnapshotAction, Void> PARSER = new ConstructingObjectParser<>(NAME,
-        a -> {
-            String storageName = (String) a[2];
-            final MountSearchableSnapshotRequest.Storage storageType;
-            if (storageName == null) {
-                storageType = null;
-            } else {
-                storageType = MountSearchableSnapshotRequest.Storage.fromString(storageName);
-            }
-            return new SearchableSnapshotAction((String) a[0], a[1] == null || (boolean) a[1], storageType);
-        });
+        a -> new SearchableSnapshotAction((String) a[0], a[1] == null || (boolean) a[1]));
 
     static {
         PARSER.declareString(ConstructingObjectParser.constructorArg(), SNAPSHOT_REPOSITORY);
         PARSER.declareBoolean(ConstructingObjectParser.optionalConstructorArg(), FORCE_MERGE_INDEX);
-        PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), STORAGE);
     }
 
 
@@ -77,21 +65,17 @@ public static SearchableSnapshotAction parse(XContentParser parser) {
 
     private final String snapshotRepository;
     private final boolean forceMergeIndex;
-    @Nullable
-    private final MountSearchableSnapshotRequest.Storage storageType;
 
-    public SearchableSnapshotAction(String snapshotRepository, boolean forceMergeIndex,
-                                    @Nullable MountSearchableSnapshotRequest.Storage type) {
+    public SearchableSnapshotAction(String snapshotRepository, boolean forceMergeIndex) {
         if (Strings.hasText(snapshotRepository) == false) {
             throw new IllegalArgumentException("the snapshot repository must be specified");
         }
         this.snapshotRepository = snapshotRepository;
         this.forceMergeIndex = forceMergeIndex;
-        this.storageType = type;
     }
 
     public SearchableSnapshotAction(String snapshotRepository) {
-        this(snapshotRepository, true, null);
+        this(snapshotRepository, true);
     }
 
     public SearchableSnapshotAction(StreamInput in) throws IOException {
@@ -101,22 +85,12 @@ public SearchableSnapshotAction(StreamInput in) throws IOException {
         } else {
             this.forceMergeIndex = true;
         }
-        if (in.getVersion().onOrAfter(Version.V_7_12_0)) {
-            this.storageType = in.readOptionalEnum(MountSearchableSnapshotRequest.Storage.class);
-        } else {
-            this.storageType = null;
-        }
     }
 
     boolean isForceMergeIndex() {
         return forceMergeIndex;
     }
 
-    @Nullable
-    public MountSearchableSnapshotRequest.Storage getStorageType() {
-        return storageType;
-    }
-
     public String getSnapshotRepository() {
         return snapshotRepository;
     }
@@ -290,28 +264,15 @@ public List<Step> toSteps(Client client, String phase, StepKey nextStepKey) {
      * Resolves the prefix to be used for the mounted index depending on the provided key
      */
     String getRestoredIndexPrefix(StepKey currentKey) {
-        if (storageType == null) {
-            if (currentKey.getPhase().equals(TimeseriesLifecycleType.FROZEN_PHASE)) {
-                return PARTIAL_RESTORED_INDEX_PREFIX;
-            } else {
-                return FULL_RESTORED_INDEX_PREFIX;
-            }
-        }
-        switch (storageType) {
-            case FULL_COPY:
-                return FULL_RESTORED_INDEX_PREFIX;
-            case SHARED_CACHE:
-                return PARTIAL_RESTORED_INDEX_PREFIX;
-            default:
-                throw new IllegalArgumentException("unexpected storage type: " + storageType);
+        if (currentKey.getPhase().equals(TimeseriesLifecycleType.FROZEN_PHASE)) {
+            return PARTIAL_RESTORED_INDEX_PREFIX;
+        } else {
+            return FULL_RESTORED_INDEX_PREFIX;
         }
     }
 
-    // Resolves the storage type from a Nullable to non-Nullable type
+    // Resolves the storage type depending on which phase the index is in
     MountSearchableSnapshotRequest.Storage getConcreteStorageType(StepKey currentKey) {
-        if (storageType != null) {
-            return storageType;
-        }
         if (currentKey.getPhase().equals(TimeseriesLifecycleType.FROZEN_PHASE)) {
             return MountSearchableSnapshotRequest.Storage.SHARED_CACHE;
         } else {
@@ -335,19 +296,13 @@ public void writeTo(StreamOutput out) throws IOException {
         if (out.getVersion().onOrAfter(Version.V_7_10_0)) {
             out.writeBoolean(forceMergeIndex);
         }
-        if (out.getVersion().onOrAfter(Version.V_7_12_0)) {
-            out.writeOptionalEnum(storageType);
-        }
     }
 
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject();
         builder.field(SNAPSHOT_REPOSITORY.getPreferredName(), snapshotRepository);
         builder.field(FORCE_MERGE_INDEX.getPreferredName(), forceMergeIndex);
-        if (storageType != null) {
-            builder.field(STORAGE.getPreferredName(), storageType);
-        }
         builder.endObject();
         return builder;
     }
@@ -361,12 +316,11 @@ public boolean equals(Object o) {
             return false;
         }
         SearchableSnapshotAction that = (SearchableSnapshotAction) o;
-        return Objects.equals(snapshotRepository, that.snapshotRepository) &&
-            Objects.equals(storageType, that.storageType);
+        return Objects.equals(snapshotRepository, that.snapshotRepository);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(snapshotRepository, storageType);
+        return Objects.hash(snapshotRepository);
     }
 }

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

@@ -50,8 +50,7 @@ public class TimeseriesLifecycleType implements LifecycleType {
     static final List<String> ORDERED_VALID_WARM_ACTIONS = Arrays.asList(SetPriorityAction.NAME, UnfollowAction.NAME, ReadOnlyAction.NAME,
         AllocateAction.NAME, MigrateAction.NAME, ShrinkAction.NAME, ForceMergeAction.NAME);
     static final List<String> ORDERED_VALID_COLD_ACTIONS;
-    static final List<String> ORDERED_VALID_FROZEN_ACTIONS = Arrays.asList(SetPriorityAction.NAME, UnfollowAction.NAME,
-        ReadOnlyAction.NAME, SearchableSnapshotAction.NAME, AllocateAction.NAME, MigrateAction.NAME, FreezeAction.NAME);
+    static final List<String> ORDERED_VALID_FROZEN_ACTIONS = Collections.singletonList(SearchableSnapshotAction.NAME);
     static final List<String> ORDERED_VALID_DELETE_ACTIONS = Arrays.asList(WaitForSnapshotAction.NAME, DeleteAction.NAME);
     static final Set<String> VALID_HOT_ACTIONS;
     static final Set<String> VALID_WARM_ACTIONS = Sets.newHashSet(ORDERED_VALID_WARM_ACTIONS);
@@ -136,10 +135,8 @@ static boolean shouldInjectMigrateStepForPhase(Phase phase) {
             }
         }
 
-        if (phase.getActions().get(SearchableSnapshotAction.NAME) != null && phase.getName().equals(FROZEN_PHASE) == false) {
-            // the `searchable_snapshot` action defines migration rules itself, so no need to inject a migrate action, unless we're in the
-            // frozen phase (as the migrate action would also include the `data_frozen` role which is not guaranteed to be included by all
-            // types of searchable snapshots)
+        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;
         }
 
@@ -301,6 +298,7 @@ public void validate(Collection<Phase> phases) {
 
         validateActionsFollowingSearchableSnapshot(phases);
         validateAllSearchableSnapshotActionsUseSameRepository(phases);
+        validateFrozenPhaseHasSearchableSnapshotAction(phases);
     }
 
     static void validateActionsFollowingSearchableSnapshot(Collection<Phase> phases) {
@@ -413,6 +411,22 @@ public static String validateMonotonicallyIncreasingPhaseTimings(Collection<Phas
         return Strings.collectionToCommaDelimitedString(errors);
     }
 
+    /**
+     * Require that the "frozen" phase configured in a policy has a searchable snapshot action.
+     */
+    static void validateFrozenPhaseHasSearchableSnapshotAction(Collection<Phase> phases) {
+        Optional<Phase> maybeFrozenPhase = phases.stream()
+            .filter(p -> FROZEN_PHASE.equals(p.getName()))
+            .findFirst();
+
+        maybeFrozenPhase.ifPresent(p -> {
+            if (p.getActions().containsKey(SearchableSnapshotAction.NAME) == false) {
+                throw new IllegalArgumentException("policy specifies the [" + FROZEN_PHASE + "] phase without a corresponding [" +
+                    SearchableSnapshotAction.NAME + "] action, but a searchable snapshot action is required in the frozen phase");
+            }
+        });
+    }
+
     private static boolean definesAllocationRules(AllocateAction action) {
         return action.getRequire().isEmpty() == false || action.getInclude().isEmpty() == false || action.getExclude().isEmpty() == false;
     }

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

@@ -29,8 +29,8 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 
-import static org.elasticsearch.xpack.core.ilm.SearchableSnapshotActionTests.randomStorageType;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.mockito.Mockito.mock;
@@ -128,7 +128,10 @@ public static LifecyclePolicy randomTimeseriesLifecyclePolicyWithAllPhases(@Null
 
     public static LifecyclePolicy randomTimeseriesLifecyclePolicy(@Nullable String lifecycleName) {
         List<String> phaseNames = randomSubsetOf(
-            between(0, TimeseriesLifecycleType.ORDERED_VALID_PHASES.size() - 1), TimeseriesLifecycleType.ORDERED_VALID_PHASES);
+            between(0, TimeseriesLifecycleType.ORDERED_VALID_PHASES.size() - 1), TimeseriesLifecycleType.ORDERED_VALID_PHASES).stream()
+            // Remove the frozen phase, we'll randomly re-add it later
+            .filter(pn -> TimeseriesLifecycleType.FROZEN_PHASE.equals(pn) == false)
+            .collect(Collectors.toList());
         Map<String, Phase> phases = new HashMap<>(phaseNames.size());
         Function<String, Set<String>> validActions = getPhaseToValidActions();
         Function<String, LifecycleAction> randomAction = getNameToActionFunction();
@@ -189,6 +192,17 @@ public static LifecyclePolicy randomTimeseriesLifecyclePolicy(@Nullable String l
             }
             phases.put(phase, new Phase(phase, after, actions));
         }
+        // Add a frozen phase if neither the hot nor cold phase contains a searchable snapshot action
+        if (hotPhaseContainsSearchableSnap == false && coldPhaseContainsSearchableSnap == false && randomBoolean()) {
+            TimeValue frozenTime = prev == null ? TimeValue.parseTimeValue(randomTimeValue(0, 100000, "s", "m", "h", "d"), "test") :
+                TimeValue.timeValueSeconds(prev.seconds() + randomIntBetween(60, 600));
+            phases.put(TimeseriesLifecycleType.FROZEN_PHASE,
+                new Phase(TimeseriesLifecycleType.FROZEN_PHASE, frozenTime,
+                    Collections.singletonMap(SearchableSnapshotAction.NAME,
+                        new SearchableSnapshotAction(randomAlphaOfLength(10), randomBoolean()))));
+        } else {
+            phases.remove(TimeseriesLifecycleType.FROZEN_PHASE);
+        }
         return new LifecyclePolicy(TimeseriesLifecycleType.INSTANCE, lifecycleName, phases);
     }
 
@@ -234,7 +248,7 @@ private static Function<String, LifecycleAction> getNameToActionFunction() {
                     case UnfollowAction.NAME:
                         return new UnfollowAction();
                     case SearchableSnapshotAction.NAME:
-                        return new SearchableSnapshotAction("repo", randomBoolean(), randomStorageType());
+                        return new SearchableSnapshotAction("repo", randomBoolean());
                     case MigrateAction.NAME:
                         return new MigrateAction(false);
                     case RollupILMAction.NAME:
@@ -269,8 +283,16 @@ protected LifecyclePolicy mutateInstance(LifecyclePolicy instance) throws IOExce
                 name = name + randomAlphaOfLengthBetween(1, 5);
                 break;
             case 1:
+                // Remove the frozen phase, because it makes a lot of invalid phases when randomly mutating an existing policy
+                phases.remove(TimeseriesLifecycleType.FROZEN_PHASE);
+                // Remove a random phase
+                if (phases.size() > 0) {
+                    phases.remove(new ArrayList<>(phases.keySet()).remove(randomIntBetween(0, phases.size() - 1)));
+                }
                 String phaseName = randomValueOtherThanMany(phases::containsKey,
-                    () -> randomFrom(TimeseriesLifecycleType.ORDERED_VALID_PHASES));
+                        () -> randomFrom(TimeseriesLifecycleType.ORDERED_VALID_PHASES.stream()
+                            .filter(pn -> TimeseriesLifecycleType.FROZEN_PHASE.equals(pn) == false)
+                            .collect(Collectors.toList())));
                 phases = new LinkedHashMap<>(phases);
                 phases.put(phaseName, new Phase(phaseName, null, Collections.emptyMap()));
                 break;