ILM: Add support for the searchable_snapshot action in the hot phase #64883

andreidan · 2020-11-10T17:21:07Z

This adds support for the searchable_snapshot ILM action in the hot phase.

We define a series of actions that cannot be executed after the index has been
mounted as a searchable snapshot. Namely: freeze, forcemerge, shrink,
and searchable_snapshot (also available in the cold phase).

If by virtue of snapshot/restoring a managed index or updating an ILM policy while it
is executing for an index, these actions could get to be executed on an index that was
mounted as searchable snapshot in the hot phase. If this happens the actions will
skip entirely. ILM will not move into the ERROR step.

Resolves #64656

andreidan · 2020-11-10T17:22:58Z

...de/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java

+            "allowed after the managed index was mounted as searchable snapshot"));
+    }
+
+    public void testUpdatePolicyToAddPhasesYieldsInvalidActionsToBeSkipped() throws Exception {


@dakrone do you think we should aim to prevent this case when updating the policy?

No I don't think so, I think we should allow a user to remove searchable snapshots from the policy as well as adding other actions (post searchable snapshot for instance) that should run just fine.

andreidan · 2020-11-10T17:26:52Z

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

        StepKey countKey = new StepKey(phase, NAME, SegmentCountStep.NAME);

+        BranchingStep conditionalSkipShrinkStep = new BranchingStep(preForceMergeBranchingKey, checkNotWriteIndex, nextStepKey,
+            (index, clusterState) -> clusterState.getMetadata().index(index).getSettings().get("index.store.snapshot.index_name") != null);


@dakrone there are several setting we configure for a searchable snapshot. I wonder if we should be more strict here and check the index name is configured for this setting? I chose not to as that's an invariant that should be guaranteed by the searchable-snapshots module. Is there a better setting to use here?

What do you think?

dakrone

Thanks for working on this Andrei, I think this is looking good. I left some comments, and I think as followup for this, we should open a separate issue to decide whether we want to add this check behavior for closed indices (I believe we can check the index state for the INDEX_CLOSED block) and skip those.

Another thing that is missing is that we need to add the check in the SearchableSnapshotAction itself, so if a user specifies searchable snapshot in the cold phase but the index is already a searchable snapshot, we skip past it instead of trying to snapshot our snapshot index :)

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

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

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

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

...de/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java

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

dakrone · 2020-11-10T23:11:07Z

...de/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java

+            "allowed after the managed index was mounted as searchable snapshot"));
+    }
+
+    public void testUpdatePolicyToAddPhasesYieldsInvalidActionsToBeSkipped() throws Exception {


No I don't think so, I think we should allow a user to remove searchable snapshots from the policy as well as adding other actions (post searchable snapshot for instance) that should run just fine.

Co-authored-by: Lee Hinman <[email protected]>

…anups

andreidan · 2020-11-11T16:33:24Z

@elasticmachine update branch

elasticmachine · 2020-11-11T17:04:32Z

Pinging @elastic/es-core-features (:Core/Features/ILM+SLM)

dakrone

LGTM, thanks for iterating on this Andrei! I left a few really tiny comments, but nothing big

dakrone · 2020-11-11T18:25:56Z

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

    public static final String SLM_RETENTION_DURATION = "slm.retention_duration";
    public static final String SLM_MINIMUM_INTERVAL = "slm.minimum_interval";

+    public static final String SNAPSHOT_INDEX_NAME = "index.store.snapshot.index_name";


Super minor, but can you add a comment about what this is for (since it's not actually an ILM setting, just one we check)

dakrone · 2020-11-11T18:27:11Z

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


+IMPORTANT: If the `searchable_snapshot` action is used in the `hot` phase the
+subsequent phases cannot define any of the `shrink`, `forcemerge`, `freeze` or
+`searchable_snapshot` (also available in the cold phase) actions.


Can you add a blurb similar to what we have for forcemerge related to requiring the "rollover" action:

"To use the XYZ action in the hot phase, the rollover action must be present. If no rollover action is configured, ILM will reject the policy."

dakrone · 2020-11-11T18:28:11Z

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

 * A {@link LifecycleAction} which shrinks the index.
 */
 public class ShrinkAction implements LifecycleAction {
+    private final Logger logger = LogManager.getLogger(ShrinkAction.class);


Super minor, but this can be static (it looks like it used to be but was dropped when it was moved)

…lastic#64883) This adds support for the searchable_snapshot ILM action in the hot phase. We define a series of actions that cannot be executed after the index has been mounted as a searchable snapshot. Namely: freeze, forcemerge, shrink, and searchable_snapshot (also available in the cold phase). If by virtue of snapshot/restoring a managed index or updating an ILM policy while it is executing for an index, these actions could get to be executed on an index that was mounted as searchable snapshot in the hot phase. If this happens the actions will skip entirely. ILM will not move into the ERROR step. (cherry picked from commit 7d45355) Signed-off-by: Andrei Dan <[email protected]>

…phase (#64883) (#64988) This adds support for the searchable_snapshot ILM action in the hot phase. We define a series of actions that cannot be executed after the index has been mounted as a searchable snapshot. Namely: freeze, forcemerge, shrink, and searchable_snapshot (also available in the cold phase). If by virtue of snapshot/restoring a managed index or updating an ILM policy while it is executing for an index, these actions could get to be executed on an index that was mounted as searchable snapshot in the hot phase. If this happens the actions will skip entirely. ILM will not move into the ERROR step. (cherry picked from commit 7d45355) Signed-off-by: Andrei Dan <[email protected]> * Use org.elasticsearch.common.collect collections (and Map)

This changes the `CopyExecutionStateStep` to copy all of the existing state from the original to the new index after a shrink or searchable snapshot ILM action. It also has two other changes First, it fixes issues in `LifecycleExecutionState.equals` and `hashCode` where parts were missing, and modifies the tests better so they would catch these issues better. Second, it moves the lifecycle policy validation out of the constructor to a new method. This is required because `PolicyStepsRegistry` constructs a new `LifecyclePolicy` out of the cached phase JSON combined with the policy from the cluster state. In the event that the cached JSON contains an action that prevents subsequent actions from being defined, we need to leniently allow this behavior, because those steps will be ignored (see elastic#64883 for this original implementation). The policy is still validated when loaded elsewhere however. Resolves elastic#73791

This changes the CopyExecutionStateStep to copy all of the existing state from the original to the new index after a shrink or searchable snapshot ILM action. It also has two other changes First, it fixes issues in LifecycleExecutionState.equals and hashCode where parts were missing, and modifies the tests better so they would catch these issues better. Second, it moves the lifecycle policy validation out of the constructor to a new method. This is required because PolicyStepsRegistry constructs a new LifecyclePolicy out of the cached phase JSON combined with the policy from the cluster state. In the event that the cached JSON contains an action that prevents subsequent actions from being defined, we need to leniently allow this behavior, because those steps will be ignored (see #64883 for this original implementation). The policy is still validated when loaded elsewhere however. Resolves #73791

This changes the CopyExecutionStateStep to copy all of the existing state from the original to the new index after a shrink or searchable snapshot ILM action. It also has two other changes First, it fixes issues in LifecycleExecutionState.equals and hashCode where parts were missing, and modifies the tests better so they would catch these issues better. Second, it moves the lifecycle policy validation out of the constructor to a new method. This is required because PolicyStepsRegistry constructs a new LifecyclePolicy out of the cached phase JSON combined with the policy from the cluster state. In the event that the cached JSON contains an action that prevents subsequent actions from being defined, we need to leniently allow this behavior, because those steps will be ignored (see elastic#64883 for this original implementation). The policy is still validated when loaded elsewhere however. Resolves elastic#73791 # Conflicts: # x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java

This changes the CopyExecutionStateStep to copy all of the existing state from the original to the new index after a shrink or searchable snapshot ILM action. It also has two other changes First, it fixes issues in LifecycleExecutionState.equals and hashCode where parts were missing, and modifies the tests better so they would catch these issues better. Second, it moves the lifecycle policy validation out of the constructor to a new method. This is required because PolicyStepsRegistry constructs a new LifecyclePolicy out of the cached phase JSON combined with the policy from the cluster state. In the event that the cached JSON contains an action that prevents subsequent actions from being defined, we need to leniently allow this behavior, because those steps will be ignored (see elastic#64883 for this original implementation). The policy is still validated when loaded elsewhere however. Resolves elastic#73791 # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicy.java # x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java

…73849) This changes the CopyExecutionStateStep to copy all of the existing state from the original to the new index after a shrink or searchable snapshot ILM action. It also has two other changes First, it fixes issues in LifecycleExecutionState.equals and hashCode where parts were missing, and modifies the tests better so they would catch these issues better. Second, it moves the lifecycle policy validation out of the constructor to a new method. This is required because PolicyStepsRegistry constructs a new LifecyclePolicy out of the cached phase JSON combined with the policy from the cluster state. In the event that the cached JSON contains an action that prevents subsequent actions from being defined, we need to leniently allow this behavior, because those steps will be ignored (see #64883 for this original implementation). The policy is still validated when loaded elsewhere however. Resolves #73791 # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicy.java # x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java

andreidan added 2 commits November 10, 2020 10:10

Enable the searchable_snapshot action in the hot phase

7b68166

Add IT for restoring index mounted as searchable snapshot

0e92f00

andreidan commented Nov 10, 2020

View reviewed changes

andreidan added 2 commits November 10, 2020 17:48

Add logging

b278e05

Fix test

97ec99f

andreidan requested a review from dakrone November 10, 2020 18:02

dakrone requested changes Nov 10, 2020

View reviewed changes

andreidan and others added 3 commits November 11, 2020 11:58

Update error message

2bd01c5

Co-authored-by: Lee Hinman <[email protected]>

Make ensure searchable_snapshot action cannot follow itself and cle…

b663f72

…anups

Fix tests

82c9e66

elasticmachine and others added 3 commits November 11, 2020 11:33

Merge branch 'master' into searchable-snapshot-hot-phase

b7936ee

Fix ForceMergeActionTests

3feb501

Fix ForceMergeActionTest

5e2366d

andreidan marked this pull request as ready for review November 11, 2020 17:03

andreidan added :Data Management/ILM+SLM Index and Snapshot lifecycle management v7.11.0 v8.0.0 labels Nov 11, 2020

elasticmachine added the Team:Data Management Meta label for data/management team label Nov 11, 2020

andreidan and others added 3 commits November 11, 2020 17:14

Fix random lifecycle generation

471d329

Fix phases validation when generating test policies

0c3b35e

Merge branch 'master' into searchable-snapshot-hot-phase

b914cfe

andreidan requested a review from dakrone November 11, 2020 18:23

Update doc

1128f07

dakrone approved these changes Nov 11, 2020

View reviewed changes

andreidan added 3 commits November 12, 2020 09:29

Duplicate sets as we remove actions to generate valid policies

a1326c3

Static loggers

54180d6

Docs: Mention rollover is needed in order to use action in the hot phase

a5e4354

Document setting name

56af507

andreidan merged commit 7d45355 into elastic:master Nov 12, 2020

andreidan added the backport pending label Nov 12, 2020

andreidan mentioned this pull request Nov 12, 2020

[7.x] ILM: Add support for the searchable_snapshot action in the hot phase (#64883) #64988

Merged

andreidan removed the backport pending label Nov 12, 2020

pugnascotia added the >enhancement label Dec 16, 2020

dakrone mentioned this pull request Jun 4, 2021

Copy all execution state in CopyExecutionStateStep #73792

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

joegallo mentioned this pull request Dec 11, 2024

Miscellaneous ILM cleanups #118488

Merged

ILM: Add support for the searchable_snapshot action in the hot phase #64883

ILM: Add support for the searchable_snapshot action in the hot phase #64883

Uh oh!

Conversation

andreidan commented Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andreidan Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

dakrone Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

andreidan Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dakrone Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

andreidan commented Nov 11, 2020

Uh oh!

elasticmachine commented Nov 11, 2020

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

dakrone Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

dakrone Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

dakrone Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andreidan commented Nov 10, 2020 •

edited

Loading

andreidan Nov 10, 2020 •

edited

Loading