Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Restrict ILM frozen phase to searchable snapshot actions only #70158

Merged
merged 3 commits into from
Mar 9, 2021
Merged

Restrict ILM frozen phase to searchable snapshot actions only #70158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b344b7d
Restrict ILM frozen phase to searchable snapshot actions only
dakrone Mar 8, 2021
2a21ea1
Update issue for BWC disabled
dakrone Mar 9, 2021
28c4bc2
Fixup documentation not to imply multiple frozen phases
dakrone Mar 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Comment on lines +192 to +193
Copy link
Member Author

@dakrone dakrone Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required because I am removing the serialization for 7.12 in a weird way (we don't usually remove things like this), once this has been backported all the way back to 7.12 I will re-enable BWC

/*
* 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
Expand Down
12 changes: 3 additions & 9 deletions docs/reference/ilm/actions/ilm-searchable-snapshot.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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]
Expand All @@ -69,8 +64,7 @@ PUT _ilm/policy/my_policy
"cold": {
"actions": {
"searchable_snapshot" : {
"snapshot_repository" : "backing_repo",
"storage": "shared_cache"
"snapshot_repository" : "backing_repo"
}
}
}
Expand Down
66 changes: 10 additions & 56 deletions .../plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/SearchableSnapshotAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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";
Expand All @@ -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);
}


Expand All @@ -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 {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
}
Expand All @@ -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);
}
}
26 changes: 20 additions & 6 deletions ...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -301,6 +298,7 @@ public void validate(Collection<Phase> phases) {

validateActionsFollowingSearchableSnapshot(phases);
validateAllSearchableSnapshotActionsUseSameRepository(phases);
validateFrozenPhaseHasSearchableSnapshotAction(phases);
}

static void validateActionsFollowingSearchableSnapshot(Collection<Phase> phases) {
Expand Down Expand Up @@ -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;
}
Expand Down
30 changes: 26 additions & 4 deletions x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the connection between the hot/cold actions and the frozen phase? We're still able to have searchable_snapshot actions in all phases as far as I understand

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mostly because we'd have to use a consistent repository name (or else validation would fail), we have enough validation that this gets more and more complex, and this doesn't have to generate all possible valid lifecycles, just a random one.

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);
}

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;
Expand Down
Loading