Skip to content

Commit 2225efc

Browse files
dakronedakrone
committed
Restrict ILM frozen phase to searchable snapshot actions only (elastic#70158)
This commit changes the frozen phase within ILM in the following ways: - The `searchable_snapshot` action now no longer takes a `storage` parameter. The storage type is determined by the phase within which it is invoked (shared cache for frozen and full copy for everything else). - The frozen phase in ILM now no longer allows *any* actions other than `searchable_snapshot` - If a frozen phase is provided, it *must* include a `searchable_snapshot` action. These changes may seem breaking, but since they are intended to go back to 7.12 which has not been released yet, they are not truly breaking changes.
1 parent 10bb2ed commit 2225efc

File tree

9 files changed

+115
-208
lines changed

9 files changed

+115
-208
lines changed

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

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ To keep the snapshot, set `delete_searchable_snapshot` to `false` in the delete
3636
`snapshot_repository`::
3737
(Required, string)
3838
Specifies where to store the snapshot.
39-
See <<snapshots-register-repository>> for more information.
39+
See <<snapshots-register-repository>> for more information. In non-frozen phases the snapshot will
40+
be mounted as a `full_copy`, and in frozen phases mounted with the `shared_cache` storage type.
4041

4142
`force_merge_index`::
4243
(Optional, Boolean)
@@ -52,12 +53,6 @@ the shards are relocating, in which case they will not be merged.
5253
The `searchable_snapshot` action will continue executing even if not all shards
5354
are force merged.
5455

55-
`storage`::
56-
(Optional, string)
57-
Specifies the type of snapshot that should be mounted for a searchable snapshot. This corresponds to
58-
the <<searchable-snapshots-api-mount-query-params, `storage` option when mounting a snapshot>>.
59-
Defaults to `full_copy` in non-frozen phases, or `shared_cache` in the frozen phase.
60-
6156
[[ilm-searchable-snapshot-ex]]
6257
==== Examples
6358
[source,console]
@@ -69,8 +64,7 @@ PUT _ilm/policy/my_policy
6964
"cold": {
7065
"actions": {
7166
"searchable_snapshot" : {
72-
"snapshot_repository" : "backing_repo",
73-
"storage": "shared_cache"
67+
"snapshot_repository" : "backing_repo"
7468
}
7569
}
7670
}

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

Lines changed: 10 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
import org.elasticsearch.cluster.health.ClusterHealthStatus;
1414
import org.elasticsearch.cluster.metadata.IndexAbstraction;
1515
import org.elasticsearch.cluster.metadata.IndexMetadata;
16-
import org.elasticsearch.common.Nullable;
1716
import org.elasticsearch.common.ParseField;
1817
import org.elasticsearch.common.Strings;
1918
import org.elasticsearch.common.io.stream.StreamInput;
@@ -44,7 +43,6 @@ public class SearchableSnapshotAction implements LifecycleAction {
4443

4544
public static final ParseField SNAPSHOT_REPOSITORY = new ParseField("snapshot_repository");
4645
public static final ParseField FORCE_MERGE_INDEX = new ParseField("force_merge_index");
47-
public static final ParseField STORAGE = new ParseField("storage");
4846
public static final String CONDITIONAL_DATASTREAM_CHECK_KEY = BranchingStep.NAME + "-on-datastream-check";
4947
public static final String CONDITIONAL_SKIP_ACTION_STEP = BranchingStep.NAME + "-check-prerequisites";
5048
public static final String CONDITIONAL_SKIP_GENERATE_AND_CLEAN = BranchingStep.NAME + "-check-existing-snapshot";
@@ -53,21 +51,11 @@ public class SearchableSnapshotAction implements LifecycleAction {
5351
public static final String PARTIAL_RESTORED_INDEX_PREFIX = "partial-";
5452

5553
private static final ConstructingObjectParser<SearchableSnapshotAction, Void> PARSER = new ConstructingObjectParser<>(NAME,
56-
a -> {
57-
String storageName = (String) a[2];
58-
final MountSearchableSnapshotRequest.Storage storageType;
59-
if (storageName == null) {
60-
storageType = null;
61-
} else {
62-
storageType = MountSearchableSnapshotRequest.Storage.fromString(storageName);
63-
}
64-
return new SearchableSnapshotAction((String) a[0], a[1] == null || (boolean) a[1], storageType);
65-
});
54+
a -> new SearchableSnapshotAction((String) a[0], a[1] == null || (boolean) a[1]));
6655

6756
static {
6857
PARSER.declareString(ConstructingObjectParser.constructorArg(), SNAPSHOT_REPOSITORY);
6958
PARSER.declareBoolean(ConstructingObjectParser.optionalConstructorArg(), FORCE_MERGE_INDEX);
70-
PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), STORAGE);
7159
}
7260

7361

@@ -77,21 +65,17 @@ public static SearchableSnapshotAction parse(XContentParser parser) {
7765

7866
private final String snapshotRepository;
7967
private final boolean forceMergeIndex;
80-
@Nullable
81-
private final MountSearchableSnapshotRequest.Storage storageType;
8268

83-
public SearchableSnapshotAction(String snapshotRepository, boolean forceMergeIndex,
84-
@Nullable MountSearchableSnapshotRequest.Storage type) {
69+
public SearchableSnapshotAction(String snapshotRepository, boolean forceMergeIndex) {
8570
if (Strings.hasText(snapshotRepository) == false) {
8671
throw new IllegalArgumentException("the snapshot repository must be specified");
8772
}
8873
this.snapshotRepository = snapshotRepository;
8974
this.forceMergeIndex = forceMergeIndex;
90-
this.storageType = type;
9175
}
9276

9377
public SearchableSnapshotAction(String snapshotRepository) {
94-
this(snapshotRepository, true, null);
78+
this(snapshotRepository, true);
9579
}
9680

9781
public SearchableSnapshotAction(StreamInput in) throws IOException {
@@ -101,22 +85,12 @@ public SearchableSnapshotAction(StreamInput in) throws IOException {
10185
} else {
10286
this.forceMergeIndex = true;
10387
}
104-
if (in.getVersion().onOrAfter(Version.V_7_12_0)) {
105-
this.storageType = in.readOptionalEnum(MountSearchableSnapshotRequest.Storage.class);
106-
} else {
107-
this.storageType = null;
108-
}
10988
}
11089

11190
boolean isForceMergeIndex() {
11291
return forceMergeIndex;
11392
}
11493

115-
@Nullable
116-
public MountSearchableSnapshotRequest.Storage getStorageType() {
117-
return storageType;
118-
}
119-
12094
public String getSnapshotRepository() {
12195
return snapshotRepository;
12296
}
@@ -290,28 +264,15 @@ public List<Step> toSteps(Client client, String phase, StepKey nextStepKey) {
290264
* Resolves the prefix to be used for the mounted index depending on the provided key
291265
*/
292266
String getRestoredIndexPrefix(StepKey currentKey) {
293-
if (storageType == null) {
294-
if (currentKey.getPhase().equals(TimeseriesLifecycleType.FROZEN_PHASE)) {
295-
return PARTIAL_RESTORED_INDEX_PREFIX;
296-
} else {
297-
return FULL_RESTORED_INDEX_PREFIX;
298-
}
299-
}
300-
switch (storageType) {
301-
case FULL_COPY:
302-
return FULL_RESTORED_INDEX_PREFIX;
303-
case SHARED_CACHE:
304-
return PARTIAL_RESTORED_INDEX_PREFIX;
305-
default:
306-
throw new IllegalArgumentException("unexpected storage type: " + storageType);
267+
if (currentKey.getPhase().equals(TimeseriesLifecycleType.FROZEN_PHASE)) {
268+
return PARTIAL_RESTORED_INDEX_PREFIX;
269+
} else {
270+
return FULL_RESTORED_INDEX_PREFIX;
307271
}
308272
}
309273

310-
// Resolves the storage type from a Nullable to non-Nullable type
274+
// Resolves the storage type depending on which phase the index is in
311275
MountSearchableSnapshotRequest.Storage getConcreteStorageType(StepKey currentKey) {
312-
if (storageType != null) {
313-
return storageType;
314-
}
315276
if (currentKey.getPhase().equals(TimeseriesLifecycleType.FROZEN_PHASE)) {
316277
return MountSearchableSnapshotRequest.Storage.SHARED_CACHE;
317278
} else {
@@ -335,19 +296,13 @@ public void writeTo(StreamOutput out) throws IOException {
335296
if (out.getVersion().onOrAfter(Version.V_7_10_0)) {
336297
out.writeBoolean(forceMergeIndex);
337298
}
338-
if (out.getVersion().onOrAfter(Version.V_7_12_0)) {
339-
out.writeOptionalEnum(storageType);
340-
}
341299
}
342300

343301
@Override
344302
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
345303
builder.startObject();
346304
builder.field(SNAPSHOT_REPOSITORY.getPreferredName(), snapshotRepository);
347305
builder.field(FORCE_MERGE_INDEX.getPreferredName(), forceMergeIndex);
348-
if (storageType != null) {
349-
builder.field(STORAGE.getPreferredName(), storageType);
350-
}
351306
builder.endObject();
352307
return builder;
353308
}
@@ -361,12 +316,11 @@ public boolean equals(Object o) {
361316
return false;
362317
}
363318
SearchableSnapshotAction that = (SearchableSnapshotAction) o;
364-
return Objects.equals(snapshotRepository, that.snapshotRepository) &&
365-
Objects.equals(storageType, that.storageType);
319+
return Objects.equals(snapshotRepository, that.snapshotRepository);
366320
}
367321

368322
@Override
369323
public int hashCode() {
370-
return Objects.hash(snapshotRepository, storageType);
324+
return Objects.hash(snapshotRepository);
371325
}
372326
}

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

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,7 @@ public class TimeseriesLifecycleType implements LifecycleType {
4848
static final List<String> ORDERED_VALID_WARM_ACTIONS = Arrays.asList(SetPriorityAction.NAME, UnfollowAction.NAME, ReadOnlyAction.NAME,
4949
AllocateAction.NAME, MigrateAction.NAME, ShrinkAction.NAME, ForceMergeAction.NAME);
5050
static final List<String> ORDERED_VALID_COLD_ACTIONS;
51-
static final List<String> ORDERED_VALID_FROZEN_ACTIONS = Arrays.asList(SetPriorityAction.NAME, UnfollowAction.NAME,
52-
ReadOnlyAction.NAME, SearchableSnapshotAction.NAME, AllocateAction.NAME, MigrateAction.NAME, FreezeAction.NAME);
51+
static final List<String> ORDERED_VALID_FROZEN_ACTIONS = Collections.singletonList(SearchableSnapshotAction.NAME);
5352
static final List<String> ORDERED_VALID_DELETE_ACTIONS = Arrays.asList(WaitForSnapshotAction.NAME, DeleteAction.NAME);
5453
static final Set<String> VALID_HOT_ACTIONS;
5554
static final Set<String> VALID_WARM_ACTIONS = Sets.newHashSet(ORDERED_VALID_WARM_ACTIONS);
@@ -134,10 +133,8 @@ static boolean shouldInjectMigrateStepForPhase(Phase phase) {
134133
}
135134
}
136135

137-
if (phase.getActions().get(SearchableSnapshotAction.NAME) != null && phase.getName().equals(FROZEN_PHASE) == false) {
138-
// the `searchable_snapshot` action defines migration rules itself, so no need to inject a migrate action, unless we're in the
139-
// frozen phase (as the migrate action would also include the `data_frozen` role which is not guaranteed to be included by all
140-
// types of searchable snapshots)
136+
if (phase.getActions().get(SearchableSnapshotAction.NAME) != null) {
137+
// Searchable snapshots automatically set their own allocation rules, no need to configure them with a migrate step.
141138
return false;
142139
}
143140

@@ -299,6 +296,7 @@ public void validate(Collection<Phase> phases) {
299296

300297
validateActionsFollowingSearchableSnapshot(phases);
301298
validateAllSearchableSnapshotActionsUseSameRepository(phases);
299+
validateFrozenPhaseHasSearchableSnapshotAction(phases);
302300
}
303301

304302
static void validateActionsFollowingSearchableSnapshot(Collection<Phase> phases) {
@@ -363,6 +361,22 @@ static void validateAllSearchableSnapshotActionsUseSameRepository(Collection<Pha
363361
}
364362
}
365363

364+
/**
365+
* Require that the "frozen" phase configured in a policy has a searchable snapshot action.
366+
*/
367+
static void validateFrozenPhaseHasSearchableSnapshotAction(Collection<Phase> phases) {
368+
Optional<Phase> maybeFrozenPhase = phases.stream()
369+
.filter(p -> FROZEN_PHASE.equals(p.getName()))
370+
.findFirst();
371+
372+
maybeFrozenPhase.ifPresent(p -> {
373+
if (p.getActions().containsKey(SearchableSnapshotAction.NAME) == false) {
374+
throw new IllegalArgumentException("policy specifies the [" + FROZEN_PHASE + "] phase without a corresponding [" +
375+
SearchableSnapshotAction.NAME + "] action, but a searchable snapshot action is required in the frozen phase");
376+
}
377+
});
378+
}
379+
366380
private static boolean definesAllocationRules(AllocateAction action) {
367381
return action.getRequire().isEmpty() == false || action.getInclude().isEmpty() == false || action.getExclude().isEmpty() == false;
368382
}

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

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@
2929
import java.util.Map;
3030
import java.util.Set;
3131
import java.util.function.Function;
32+
import java.util.stream.Collectors;
3233

33-
import static org.elasticsearch.xpack.core.ilm.SearchableSnapshotActionTests.randomStorageType;
3434
import static org.hamcrest.Matchers.equalTo;
3535
import static org.hamcrest.Matchers.instanceOf;
3636
import static org.mockito.Mockito.mock;
@@ -125,7 +125,10 @@ public static LifecyclePolicy randomTimeseriesLifecyclePolicyWithAllPhases(@Null
125125

126126
public static LifecyclePolicy randomTimeseriesLifecyclePolicy(@Nullable String lifecycleName) {
127127
List<String> phaseNames = randomSubsetOf(
128-
between(0, TimeseriesLifecycleType.VALID_PHASES.size() - 1), TimeseriesLifecycleType.VALID_PHASES);
128+
between(0, TimeseriesLifecycleType.VALID_PHASES.size() - 1), TimeseriesLifecycleType.VALID_PHASES).stream()
129+
// Remove the frozen phase, we'll randomly re-add it later
130+
.filter(pn -> TimeseriesLifecycleType.FROZEN_PHASE.equals(pn) == false)
131+
.collect(Collectors.toList());
129132
Map<String, Phase> phases = new HashMap<>(phaseNames.size());
130133
Function<String, Set<String>> validActions = getPhaseToValidActions();
131134
Function<String, LifecycleAction> randomAction = getNameToActionFunction();
@@ -183,6 +186,16 @@ public static LifecyclePolicy randomTimeseriesLifecyclePolicy(@Nullable String l
183186
}
184187
phases.put(phase, new Phase(phase, after, actions));
185188
}
189+
// Add a frozen phase if neither the hot nor cold phase contains a searchable snapshot action
190+
if (hotPhaseContainsSearchableSnap == false && coldPhaseContainsSearchableSnap == false && randomBoolean()) {
191+
TimeValue frozenTime = TimeValue.parseTimeValue(randomTimeValue(0, 100000, "s", "m", "h", "d"), "test");
192+
phases.put(TimeseriesLifecycleType.FROZEN_PHASE,
193+
new Phase(TimeseriesLifecycleType.FROZEN_PHASE, frozenTime,
194+
Collections.singletonMap(SearchableSnapshotAction.NAME,
195+
new SearchableSnapshotAction(randomAlphaOfLength(10), randomBoolean()))));
196+
} else {
197+
phases.remove(TimeseriesLifecycleType.FROZEN_PHASE);
198+
}
186199
return new LifecyclePolicy(TimeseriesLifecycleType.INSTANCE, lifecycleName, phases);
187200
}
188201

@@ -228,7 +241,7 @@ private static Function<String, LifecycleAction> getNameToActionFunction() {
228241
case UnfollowAction.NAME:
229242
return new UnfollowAction();
230243
case SearchableSnapshotAction.NAME:
231-
return new SearchableSnapshotAction("repo", randomBoolean(), randomStorageType());
244+
return new SearchableSnapshotAction("repo", randomBoolean());
232245
case MigrateAction.NAME:
233246
return new MigrateAction(false);
234247
case RollupILMAction.NAME:
@@ -263,7 +276,16 @@ protected LifecyclePolicy mutateInstance(LifecyclePolicy instance) throws IOExce
263276
name = name + randomAlphaOfLengthBetween(1, 5);
264277
break;
265278
case 1:
266-
String phaseName = randomValueOtherThanMany(phases::containsKey, () -> randomFrom(TimeseriesLifecycleType.VALID_PHASES));
279+
// Remove the frozen phase, because it makes a lot of invalid phases when randomly mutating an existing policy
280+
phases.remove(TimeseriesLifecycleType.FROZEN_PHASE);
281+
// Remove a random phase
282+
if (phases.size() > 0) {
283+
phases.remove(new ArrayList<>(phases.keySet()).remove(randomIntBetween(0, phases.size() - 1)));
284+
}
285+
String phaseName = randomValueOtherThanMany(phases::containsKey,
286+
() -> randomFrom(TimeseriesLifecycleType.VALID_PHASES.stream()
287+
.filter(pn -> TimeseriesLifecycleType.FROZEN_PHASE.equals(pn) == false)
288+
.collect(Collectors.toList())));
267289
phases = new LinkedHashMap<>(phases);
268290
phases.put(phaseName, new Phase(phaseName, TimeValue.timeValueSeconds(randomIntBetween(1, 1000)), Collections.emptyMap()));
269291
break;

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

Lines changed: 2 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
*/
77
package org.elasticsearch.xpack.core.ilm;
88

9-
import org.elasticsearch.common.Nullable;
109
import org.elasticsearch.common.io.stream.Writeable;
1110
import org.elasticsearch.common.xcontent.XContentParser;
1211
import org.elasticsearch.xpack.core.ilm.Step.StepKey;
@@ -61,28 +60,15 @@ public void testToSteps() {
6160
}
6261

6362
public void testPrefixAndStorageTypeDefaults() {
64-
SearchableSnapshotAction action = new SearchableSnapshotAction("repo", randomBoolean(), null);
63+
SearchableSnapshotAction action = new SearchableSnapshotAction("repo", randomBoolean());
6564
StepKey nonFrozenKey = new StepKey(randomFrom("hot", "warm", "cold", "delete"), randomAlphaOfLength(5), randomAlphaOfLength(5));
6665
StepKey frozenKey = new StepKey("frozen", randomAlphaOfLength(5), randomAlphaOfLength(5));
6766

68-
assertThat(action.getStorageType(), equalTo(null));
6967
assertThat(action.getConcreteStorageType(nonFrozenKey), equalTo(MountSearchableSnapshotRequest.Storage.FULL_COPY));
7068
assertThat(action.getRestoredIndexPrefix(nonFrozenKey), equalTo(SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX));
7169

7270
assertThat(action.getConcreteStorageType(frozenKey), equalTo(MountSearchableSnapshotRequest.Storage.SHARED_CACHE));
7371
assertThat(action.getRestoredIndexPrefix(frozenKey), equalTo(SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX));
74-
75-
action = new SearchableSnapshotAction("repo", randomBoolean(), MountSearchableSnapshotRequest.Storage.FULL_COPY);
76-
assertThat(action.getConcreteStorageType(nonFrozenKey), equalTo(MountSearchableSnapshotRequest.Storage.FULL_COPY));
77-
assertThat(action.getRestoredIndexPrefix(nonFrozenKey), equalTo(SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX));
78-
assertThat(action.getConcreteStorageType(frozenKey), equalTo(MountSearchableSnapshotRequest.Storage.FULL_COPY));
79-
assertThat(action.getRestoredIndexPrefix(frozenKey), equalTo(SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX));
80-
81-
action = new SearchableSnapshotAction("repo", randomBoolean(), MountSearchableSnapshotRequest.Storage.SHARED_CACHE);
82-
assertThat(action.getConcreteStorageType(nonFrozenKey), equalTo(MountSearchableSnapshotRequest.Storage.SHARED_CACHE));
83-
assertThat(action.getRestoredIndexPrefix(nonFrozenKey), equalTo(SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX));
84-
assertThat(action.getConcreteStorageType(frozenKey), equalTo(MountSearchableSnapshotRequest.Storage.SHARED_CACHE));
85-
assertThat(action.getRestoredIndexPrefix(frozenKey), equalTo(SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX));
8672
}
8773

8874
private List<StepKey> expectedStepKeysWithForceMerge(String phase) {
@@ -125,20 +111,6 @@ private List<StepKey> expectedStepKeysNoForceMerge(String phase) {
125111
new StepKey(phase, NAME, SwapAliasesAndDeleteSourceIndexStep.NAME));
126112
}
127113

128-
@Nullable
129-
public static MountSearchableSnapshotRequest.Storage randomStorageType() {
130-
if (randomBoolean()) {
131-
// null is the same as a full copy, it just means it was not specified
132-
if (randomBoolean()) {
133-
return null;
134-
} else {
135-
return MountSearchableSnapshotRequest.Storage.FULL_COPY;
136-
}
137-
} else {
138-
return MountSearchableSnapshotRequest.Storage.SHARED_CACHE;
139-
}
140-
}
141-
142114
@Override
143115
protected SearchableSnapshotAction doParseInstance(XContentParser parser) throws IOException {
144116
return SearchableSnapshotAction.parse(parser);
@@ -160,6 +132,6 @@ protected SearchableSnapshotAction mutateInstance(SearchableSnapshotAction insta
160132
}
161133

162134
static SearchableSnapshotAction randomInstance() {
163-
return new SearchableSnapshotAction(randomAlphaOfLengthBetween(5, 10), randomBoolean(), randomStorageType());
135+
return new SearchableSnapshotAction(randomAlphaOfLengthBetween(5, 10), randomBoolean());
164136
}
165137
}

0 commit comments

Comments
 (0)