ILM: add searchable snapshot action #52585

andreidan · 2020-02-20T17:46:10Z

Add ILM support for searchable snapshots in the cold phase. Searchable snapshots are part of the lazy snapshot restores effort that's being tracked here

The API for configuring creating a searchable snapshot is:

"cold": {
  "searchable_snapshot" : { 
    "snapshot_repository" : "snapshotRepositoryName"
  }
}

elasticmachine · 2020-02-20T17:46:13Z

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

andreidan · 2020-02-21T14:38:16Z

@elasticmachine update branch

…hot-action

dakrone

Thanks for working on this @andreidan, I know it's still a WIP, but I left a bunch of comments!

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

dakrone · 2020-02-20T22:46:06Z

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

+        Objects.nonNull(indexPrefix);
+        Objects.nonNull(settingsKeys);


These need to be Objects.requireNonNull, currently it just returns true or false but doesn't throw an exception.

Good catch (I hadn't even acknowledged the existence of an API that just does obj != null - TIL :) )

dakrone · 2020-02-20T22:48:30Z

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

+            settings.put(key, value);
+        }
+
+        UpdateSettingsRequest updateSettingsRequest = new UpdateSettingsRequest(indexName)


Currently we do this as an AsyncActionStep, but should we rather do this as a ClusterStateActionStep to avoid having to actually issue the request and wait for it? I believe it would be a bit better then, and it would also allow us to copy settings that may not be settable (if we need that functionality in the future)

Oh interesting. I agree it'd be a better avenue

dakrone · 2020-02-20T23:02:53Z

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

+            "the ilm policy but has " + lifecycleState.getSnapshotName();
+        LifecycleExecutionState.Builder newCustomData = LifecycleExecutionState.builder(lifecycleState);
+        String policy = indexMetaData.getSettings().get(LifecycleSettings.LIFECYCLE_NAME);
+        String snapshotName = generateSnapshotName(generateSnapshotName("<{now/M}-" + index.getName() + "-" + policy + ">"));


Why are we generating the snapshot name twice here?

Also, do we think {now/M} is enough granularity? We could probably go with /d to be a bit more granular?

Also, this should use the same validation that SnapshotLifecyclePolicy.validate() does for validating things (for example, no # in the name). We should factor that validation out into a method and use it here.

dakrone · 2020-02-21T15:52:51Z

...core/src/main/java/org/elasticsearch/xpack/core/ilm/SwapAliasesAndDeleteSourceIndexStep.java

+        getClient().admin().indices().aliases(aliasesRequest, new ActionListener<>() {
+
+            @Override
+            public void onResponse(AcknowledgedResponse response) {


We should check that the request was actually acknowledged and throw an exception if it wasn't so that it can be retried

dakrone · 2020-02-21T15:53:28Z

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

+
+import static org.elasticsearch.xpack.core.ilm.LifecycleExecutionState.fromIndexMetadata;
+
+public class TakeSnapshotStep extends AsyncActionStep {


I think a better name might be CreateSnapshotStep

dakrone · 2020-02-21T15:54:32Z

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

+        // TODO should we expose this?
+        request.includeGlobalState(false);


I don't think we should expose this (meaning to hardcode it as false like you have), these are specific snapshots used only for searchable snapshots

dakrone · 2020-02-21T15:56:16Z

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

+            public void onResponse(CreateSnapshotResponse createSnapshotResponse) {
+                listener.onResponse(true);


We should probably check the status() of the response, it's possible it could be a failure in which case I think we could retry again immediately?

dakrone · 2020-02-21T15:57:53Z

.../plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForGreenIndexHealthStep.java

+/**
+ * Optionally derives the index name using the provided prefix (if any) and waits for the status of the index to be GREEN.
+ */
+public class WaitForGreenIndexHealthStep extends ClusterStateWaitStep {


I think we could refactor the users of this class to use WaitForIndexColorStep and then we wouldn't need to add this class

Totally, thanks for the tip. I didn't know of its existence

elasticmachine · 2020-02-21T23:40:59Z

Pinging @elastic/es-ui (:ES-UI)

cjcenizal · 2020-02-25T15:29:24Z

@andreidan Would you mind adding some details to the PR description? The ES UI team would be particularly interested in changes to the API and an explanation of the use cases for new features.

- renamed TakeSnapshotStep to CreateSnapshotStep - made the Create and RestoreSnapshotStep retry-able using the AsyncRetryDuringSnapshotActionStep infrastructure - CreateSnapshotStep checks the response status for internal server error and fails the step in this case so we retry before we go to the next steps that’ll discover the snapshot was not created successfully - CleanupSnapshotStep reports missing repository in a more detailed way - CopySettingsStep copies the settings using the cluster state directly - GenerateSnapshotNameStep validates the snapshot name

This redesign is meant to allow the client to specify that it wants to stop waiting and move to the “condition unfulfilled” side of the branch.

andreidan · 2020-03-02T16:18:46Z

@elasticmachine update branch

…hot-action

This speeds up the test a bit and also avoids a race condition where the explain API will not get a response as the ILM is busy executing cluster action steps for the restored index.

andreidan · 2020-03-27T12:32:04Z

@elasticmachine update branch

…hot-action

andreidan · 2020-03-27T12:47:19Z

@elasticmachine update branch

…hot-action

andreidan · 2020-03-27T12:58:40Z

@elasticmachine update branch

…hot-action

dakrone

Okay whew, sorry about the delay on this Andrei, I left a few more really minor comments, I think the only thing that's major is the null check boolean flipping in WaitForSnapshotInProgressStep.java

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

dakrone · 2020-03-27T17:08:51Z

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

+        if (settingsKeys == null || settingsKeys.length == 0) {
+            return clusterState;
+        }


Super minor, but we can probably stick this before the targetIndexMetadata == null check to avoid an error where no settings are copied?

dakrone · 2020-03-27T17:11:37Z

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

    public DeleteAction() {
+        this(true);


It might be worth having a separate discussion (I can't remember if we discussed it) with David & Co about whether we want the default for this to be true or false (can be had after this PR)

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

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

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

dakrone · 2020-03-27T17:27:15Z

...lugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForSnapshotInProgressStep.java

+            return new Result(false, new Info(String.format(Locale.ROOT,
+                "snapshot [%s] generated by policy [%s] for index [%s] is still in progress", snapshotName, policyName, indexName)));


I actually think we need to return true here, it's possible that the snapshot was taken and completed (prior to the step being updated), and then the cluster was restarted, so the SnapshotsInProgress is null and won't ever exist because the snapshot was successful, best to err on the side of moving forward and doing a check than be wedged I think

Deleted the WaitForSnapshotInProgressStep altogether as we'll design a step that waits for a generation change in the repo metadata in a subsequent PR.

andreidan · 2020-03-30T15:53:07Z

@elasticmachine update branch

…hot-action

Co-Authored-By: Lee Hinman <dakrone@users.noreply.github.com>

We'll add a new way to save snapshot status api calls based on the repository data generation in a future PR.

andreidan · 2020-03-31T16:11:15Z

@elasticmachine update branch

…hot-action

dakrone

LGTM! I left two really minor typo change comments, thanks for iterating so much on this @andreidan!

dakrone · 2020-03-31T17:28:13Z

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

+ * newly created searchable snapshot backed index.
+ */
+public class SearchableSnapshotAction implements LifecycleAction {
+    public static final String NAME = "searchable_snapshot";


I think this one may have been lost in some of the other reviews, but I still think it should be changed (to searchable-snapshot)

dakrone · 2020-03-31T17:29:16Z

...ugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/SearchableSnaposhotActionTests.java

+import static org.elasticsearch.xpack.core.ilm.SearchableSnapshotAction.getCheckSnapshotStatusAsyncAction;
+import static org.hamcrest.Matchers.is;
+
+public class SearchableSnaposhotActionTests extends AbstractActionTestCase<SearchableSnapshotAction> {


Super minor, but there's a typo here (and in the name of the file):
SearchableSnaposhotActionTests vs
SearchableSnapshotActionTests

Ah, it was hard to spot the typo, great eyes, thanks @dakrone !

Regarding the action name, I renamed it to "searchable_snapshot" (lowercase) on purpose when we renamed "snapshot-repository" to "snapshot_repository" as I thought it was confusing to have a configuration that mixes scores and underscores eg.:

"searchable-snapshot" : { "snapshot_repository" : "snapshotRepositoryName" }

I using underscores everywhere employs less cognitive load:

"searchable_snapshot" : { "snapshot_repository" : "snapshotRepositoryName" }

The other "composed" action we have is set_priority so it would be inconsistent to use scores. If you don't have a strong opinion on this, I think I'd prefer we use underscores everywhere.

I read the other one as the step name rather than the action name, I agree that the action name should use underscores (like our other ones), sorry about that!

andreidan · 2020-04-01T08:17:37Z

Thanks for reviewing this rather huge chunk of functionality Lee

ILM: add searchable snapshot action

25cea2b

andreidan added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Feb 20, 2020

elasticmachine and others added 3 commits February 21, 2020 09:38

Merge branch 'feature/searchable-snapshots' into ilm-searchable-snaps…

3c32490

…hot-action

Add license header

7dbc46e

Document why we don’t restore the lifecycle setting

9881c44

dakrone reviewed Feb 21, 2020

View reviewed changes

andreidan added 2 commits February 21, 2020 16:03

Checkstyle

a601dbf

Remove unused import

f0a9da1

dakrone added :ES-UI >feature labels Feb 21, 2020

andreidan added 2 commits February 24, 2020 16:04

Use WaitForIndexColorStep

dfec7c9

Use requireNonNull

a6bc720

andreidan added 11 commits February 25, 2020 17:48

Make performAction final in AsyncRetryDuringSnapshotActionStep

f6f7d29

Add docs and redesign OnAsyncWaitBranchingStep

7075752

This redesign is meant to allow the client to specify that it wants to stop waiting and move to the “condition unfulfilled” side of the branch.

Rename BranchingStepListener onStopWaitingForCondition

501dbaf

Extract and reuse the alias swap request

1222902

Rename restoredIndexPrefix to targetIndexPrefix

faa2834

Fix tests

21d67cb

Unused imports

7e0d5f7

Handle null next step key

eafe3de

Try with resources in test

bc91ae3

Rename originalIndex to sourceIndexName

8520c7b

elasticmachine and others added 2 commits March 2, 2020 11:18

Merge branch 'feature/searchable-snapshots' into ilm-searchable-snaps…

9735649

…hot-action

More tests

d20cf52

andreidan and others added 6 commits March 25, 2020 21:20

Fix test

80c9a45

Merge branch 'feature/searchable-snapshots' into ilm-searchable-snaps…

8ed70c8

…hot-action

Delete phase min_age after 5 seconds

c38e081

Checkstyle

c032d3a

Use underscore in action name too

7107b1a

Get the generated snapshot name from the original index in test

5bc6df2

This speeds up the test a bit and also avoids a race condition where the explain API will not get a response as the ILM is busy executing cluster action steps for the restored index.

Merge branch 'feature/searchable-snapshots' into ilm-searchable-snaps…

5b7f599

…hot-action

Merge branch 'feature/searchable-snapshots' into ilm-searchable-snaps…

7f1940d

…hot-action

Merge branch 'feature/searchable-snapshots' into ilm-searchable-snaps…

879e7d6

…hot-action

dakrone requested changes Mar 27, 2020

View reviewed changes

elasticmachine and others added 7 commits March 30, 2020 11:53

Merge branch 'feature/searchable-snapshots' into ilm-searchable-snaps…

01222af

…hot-action

Update exception message

64a903d

Co-Authored-By: Lee Hinman <dakrone@users.noreply.github.com>

Update javadoc

6e4ede4

Co-Authored-By: Lee Hinman <dakrone@users.noreply.github.com>

Add example of generated snapshot name

864f4c3

Fail CleanupSnapshotStep if the request is not acked

9bbb9df

Move empty settings check before target index check

2c61f00

Delete WaitForSnapshotInProgressStepTests

ac574b2

We'll add a new way to save snapshot status api calls based on the repository data generation in a future PR.

Merge branch 'feature/searchable-snapshots' into ilm-searchable-snaps…

567d067

…hot-action

andreidan requested a review from dakrone March 31, 2020 16:53

dakrone approved these changes Mar 31, 2020

View reviewed changes

Fix typo

b766806

andreidan merged commit a5c7bec into elastic:feature/searchable-snapshots Apr 1, 2020

tlrx mentioned this pull request Apr 6, 2020

Merge feature/searchable-snapshots branch into master #54803

Merged

cjcenizal added the Team:Deployment Management Meta label for Management Experience - Deployment Management team label Jun 9, 2020


		import static org.elasticsearch.xpack.core.ilm.LifecycleExecutionState.fromIndexMetadata;

		public class TakeSnapshotStep extends AsyncActionStep {

		// TODO should we expose this?
		request.includeGlobalState(false);

		public void onResponse(CreateSnapshotResponse createSnapshotResponse) {
		listener.onResponse(true);

		return new Result(false, new Info(String.format(Locale.ROOT,
		"snapshot [%s] generated by policy [%s] for index [%s] is still in progress", snapshotName, policyName, indexName)));

ILM: add searchable snapshot action #52585

ILM: add searchable snapshot action #52585

Conversation

andreidan commented Feb 20, 2020 • edited Loading

elasticmachine commented Feb 20, 2020

andreidan commented Feb 21, 2020

dakrone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 21, 2020

cjcenizal commented Feb 25, 2020

andreidan commented Mar 2, 2020

andreidan commented Mar 27, 2020

andreidan commented Mar 27, 2020

andreidan commented Mar 27, 2020

dakrone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreidan commented Mar 30, 2020

andreidan commented Mar 31, 2020

dakrone left a comment

Choose a reason for hiding this comment

dakrone Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreidan commented Apr 1, 2020

andreidan commented Feb 20, 2020 •

edited

Loading

dakrone Mar 31, 2020 •

edited

Loading