ILM: add support for rolling over data streams #57295

andreidan · 2020-05-28T13:58:36Z

As the datastream information is stored in the ClusterState.Metadata we exposed
the Metadata to the AsyncWaitStep#evaluateCondition method in order for
the steps to be able to identify when a managed index is part of a DataStream.

If a managed index is part of a DataStream the rollover target is the DataStream
name and the highest generation index is the write index (ie. the rolled index).

Relates to #53488
Relates to #53100

Relates to elastic#53488

As the datastream information is stored in the `ClusterState.Metadata` we exposed the `Metadata` to the `AsyncWaitStep#evaluateCondition` method in order for the steps to be able to identify when a managed index is part of a DataStream. If a managed index is part of a DataStream the rollover target is the DataStream name and the highest generation index is the write index (ie. the rolled index).

elasticmachine · 2020-05-28T13:58:37Z

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

danhermann · 2020-05-28T15:40:57Z

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

+            DataStream dataStream = indexAbstraction.getParentDataStream().getDataStream();
+            rolledIndexName = DataStream.getBackingIndexName(dataStream.getName(), dataStream.getGeneration());


Minor: this correctly identifies the data stream's write index under the current implementation of data streams. The IndexAbstraction::getWriteIndex method will always return the correct write index in the event that we change the logic around backing index names, generations, etc., and would slightly simplify the code above.

Thanks for the suggestion, Dan. Pushed a fix to change this.

dakrone

I left some comments on this, thanks Andrei!

dakrone · 2020-05-28T17:05:08Z

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

@@ -29,7 +30,7 @@ protected Client getClient() {
        return client;
    }

-    public abstract void evaluateCondition(IndexMetadata indexMetadata, Listener listener, TimeValue masterTimeout);
+    public abstract void evaluateCondition(Metadata metadata, IndexMetadata indexMetadata, Listener listener, TimeValue masterTimeout);


It does seem strange to pass both the Metadata and the IndexMetadata, should we instead pass Metadata and Index so it's easy to look up the index metadata?

Yeah, fair enough, it felt a bit odd to me too (it equally felt wasteful, in terms of CPU cycles, to re-do the lookup every time though, but given ILM is not so much about low latency I agree it makes sense to change it)

dakrone · 2020-05-28T17:07:31Z

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

-            listener.onResponse(true);
-            return;
-        }
+        IndexAbstraction indexAbstraction = currentClusterState.metadata().getIndicesLookup().get(indexMetadata.getIndex().getName());


Super minor, but can you add an

assert indexAbstraction != null : "expected the index " + indexName + " to exist in the lookup but it didn't";

after this line? I don't think it's going to happen, but we should check it regardless. Optionally, we could make it a real error also (throw an IllegalStateException)

dakrone · 2020-05-28T17:11:22Z

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

+        IndexAbstraction indexAbstraction = currentClusterState.metadata().getIndicesLookup().get(indexMetadata.getIndex().getName());
+        final String rolloverTarget;
+        if (indexAbstraction.getParentDataStream() != null) {
+            rolloverTarget = indexAbstraction.getParentDataStream().getDataStream().getName();


Just to be super paranoid, I think we should handle the case where getDataStream() returns null

I think that can be changed to:

rolloverTarget = indexAbstraction.getParentDataStream().getName();

to eliminate the need for another null check.

dakrone · 2020-05-28T17:13:55Z

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

+            boolean indexingComplete = LifecycleSettings.LIFECYCLE_INDEXING_COMPLETE_SETTING.get(indexMetadata.getSettings());
+            if (indexingComplete) {
+                logger.trace(indexMetadata.getIndex() + " has lifecycle complete set, skipping " + RolloverStep.NAME);
+                listener.onResponse(true);
+                return;
+            }


Should we allow this indexing complete setting regardless of whether the parent data stream exists or not? (in otherwords, moving it before the if (indexAbstraction.getParentDataStream() != null) { check)

dakrone · 2020-05-28T17:17:28Z

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

-        }
+            if (Strings.isNullOrEmpty(rolloverAlias)) {
+                listener.onFailure(new IllegalArgumentException(String.format(Locale.ROOT,
+                    "setting [%s] for index [%s] is empty or not defined", RolloverAction.LIFECYCLE_ROLLOVER_ALIAS,


I think this is a chance we can improve this error message (if you agree), maybe something like:

setting [index.lifecycle.rollover_alias] for index [foo-1] is not defined, it must be set to the name of the alias pointing to the group of indices being rolled over

I'm not stuck on the wording, maybe you have a better idea?

Great point, I think the wording sounds good

dakrone · 2020-05-28T17:21:32Z

...gin/core/src/main/java/org/elasticsearch/xpack/core/ilm/UpdateRolloverLifecycleDateStep.java

+        IndexAbstraction indexAbstraction = currentState.metadata().getIndicesLookup().get(index.getName());
+        final String rolloverTarget;
+        if (indexAbstraction.getParentDataStream() != null) {
+            rolloverTarget = indexAbstraction.getParentDataStream().getDataStream().getName();


Same comment here about null check for the getDataStream(), I also wonder if maybe we should make this a nice static helper like IndexAbstraction.streamNameOrNull(indexAbstraction)?

The underlying dataStream is non-nullable though. The getName method that Dan recommended emphasis this so replaced it here as well. Happy to discuss if there are any situations where this could be null though (in which case I believe more null checks are needed in IndexAbstraction.DataStream)

dakrone · 2020-05-28T17:25:40Z

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

+            IndexMetadata aliasWriteIndex = aliasAbstraction.getWriteIndex();
+            if (aliasWriteIndex != null) {
+                rolledIndexName = aliasWriteIndex.getIndex().getName();
+                waitForActiveShardsSettingValue = aliasWriteIndex.getSettings().get("index.write.wait_for_active_shards");


Super minor, but can we use IndexMetadata.SETTING_WAIT_FOR_ACTIVE_SHARDS instead of hardcoding here?

dakrone · 2020-05-28T17:26:03Z

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

+                    return getErrorResultOnNullMetadata(index);
+                }
+                rolledIndexName = rolledIndexMeta.getIndex().getName();
+                waitForActiveShardsSettingValue = rolledIndexMeta.getSettings().get("index.write.wait_for_active_shards");


Same here about using IndexMetadata.SETTING_WAIT_FOR_ACTIVE_SHARDS

dakrone · 2020-05-28T17:26:58Z

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

@@ -114,6 +124,16 @@ public Result isConditionMet(Index index, ClusterState clusterState) {
        return new Result(enoughShardsActive, new ActiveShardsInfo(currentActiveShards, activeShardCount.toString(), enoughShardsActive));
    }

+    private Result getErrorResultOnNullMetadata(Index originalIndex) {


This can be static I think

dakrone · 2020-05-28T17:33:24Z

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

+        assert indexAbstraction != null : "invalid cluster metadata. index [" + indexMetadata.getIndex().getName() + "] was not found";
+        final String rolloverTarget;
+        if (indexAbstraction.getParentDataStream() != null) {
+            rolloverTarget = indexAbstraction.getParentDataStream().getDataStream().getName();


Same here about null check or a helper for retrieving the name :)

andreidan · 2020-05-29T12:45:34Z

Dan, Lee, thanks for the review. I believe I've addressed all your suggestions

danhermann

LGTM. Thanks, @andreidan!

cjcenizal · 2020-05-29T19:57:41Z

Just wanted to note that the docs also need to be updated: https://www.elastic.co/guide/en/elasticsearch/reference/current/getting-started-index-lifecycle-management.html.

dakrone

LGTM, thanks Andrei!

andreidan · 2020-06-02T09:24:17Z

@cjcenizal thanks for the heads up, we'll update the docs in an upcoming PR

As the datastream information is stored in the `ClusterState.Metadata` we exposed the `Metadata` to the `AsyncWaitStep#evaluateCondition` method in order for the steps to be able to identify when a managed index is part of a DataStream. If a managed index is part of a DataStream the rollover target is the DataStream name and the highest generation index is the write index (ie. the rolled index). (cherry picked from commit 6b410df) Signed-off-by: Andrei Dan <andrei.dan@elastic.co>

andreidan added 3 commits May 28, 2020 14:53

Lift getIndexSettingsAsMap in EsRestTestCase

a692663

Extract rest ILM testing utilities in TimeSeriesRestDriver

aae8ce3

Relates to elastic#53488

andreidan added :Data Management/ILM+SLM Index and Snapshot lifecycle management v8.0.0 v7.9.0 labels May 28, 2020

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

andreidan added 2 commits May 28, 2020 15:28

Fix tests

b890e04

Fix line length

6fc0933

danhermann reviewed May 28, 2020

View reviewed changes

andreidan requested a review from dakrone May 28, 2020 15:53

Get rolled index using the DataStream writeIndex

1aeedfd

dakrone requested changes May 28, 2020

View reviewed changes

andreidan added 4 commits May 29, 2020 11:14

Change AsyncWaitStep#evaluateCondition signature

2feb945

Static methods, drop hardcoded strings, cosmetics

308d1b6

Remove unused imports

91a86cd

Fix test

9a62405

andreidan requested review from dakrone and danhermann May 29, 2020 12:45

danhermann approved these changes May 29, 2020

View reviewed changes

dakrone approved these changes Jun 1, 2020

View reviewed changes

andreidan merged commit 6b410df into elastic:master Jun 2, 2020

andreidan mentioned this pull request Jun 2, 2020

[7.x] ILM: add support for rolling over data streams (#57295) #57515

Merged

pugnascotia added the >enhancement label Jul 16, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ILM: add support for rolling over data streams #57295

ILM: add support for rolling over data streams #57295

andreidan commented May 28, 2020

elasticmachine commented May 28, 2020

danhermann May 28, 2020 •

edited

Loading

andreidan May 28, 2020

dakrone left a comment

dakrone May 28, 2020

andreidan May 29, 2020

dakrone May 28, 2020

dakrone May 28, 2020

danhermann May 28, 2020

dakrone May 28, 2020

dakrone May 28, 2020

andreidan May 29, 2020 •

edited

Loading

dakrone May 28, 2020

andreidan May 29, 2020

dakrone May 28, 2020

dakrone May 28, 2020

dakrone May 28, 2020

dakrone May 28, 2020

andreidan commented May 29, 2020

danhermann left a comment

cjcenizal commented May 29, 2020

dakrone left a comment

andreidan commented Jun 2, 2020

		DataStream dataStream = indexAbstraction.getParentDataStream().getDataStream();
		rolledIndexName = DataStream.getBackingIndexName(dataStream.getName(), dataStream.getGeneration());

ILM: add support for rolling over data streams #57295

ILM: add support for rolling over data streams #57295

Conversation

andreidan commented May 28, 2020

elasticmachine commented May 28, 2020

danhermann May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

andreidan May 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreidan commented May 29, 2020

danhermann left a comment

Choose a reason for hiding this comment

cjcenizal commented May 29, 2020

dakrone left a comment

Choose a reason for hiding this comment

andreidan commented Jun 2, 2020

danhermann May 28, 2020 •

edited

Loading

andreidan May 29, 2020 •

edited

Loading