TSDB: Implement Downsampling ILM Action for time-series indices #87269

csoulios · 2022-05-31T19:21:07Z

This PR adds support for an ILM action that downsamples a time-series index by invoking the _rollup endpoint (#85708)

A policy that includes the rollup action will look like the following

PUT _ilm/policy/my_policy
{
  "policy": {
    "phases": {
      "warm": {
        "actions": {
          "rollup": {
  	    "fixed_interval": "24h"
  	  }
  	}
      }
    }
  }
}

Relates to #74660
Fixes #68609

Finally, this PR is based on the code written for #68184

elasticmachine · 2022-05-31T19:21:11Z

Pinging @elastic/es-data-management (Team:Data Management)

elasticmachine · 2022-05-31T19:21:11Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticmachine · 2022-05-31T19:21:11Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticsearchmachine · 2022-05-31T19:21:32Z

Hi @csoulios, I've created a changelog YAML for you.

Fix tests

Removed the `config` object and added `fixed_interval` param right in the `rollup` action

dakrone

Thanks for the changes Christos! I think this generally looks good to me, I left a few more comments and I think once we handle the rollup failure this will be ready.

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

dakrone · 2022-07-13T15:24:34Z

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

+        WaitForIndexColorStep waitForGreenIndexHealthStep = new WaitForIndexColorStep(
+            waitForGreenRollupIndexKey,
+            copyMetadataKey,
+            ClusterHealthStatus.GREEN,
+            (indexName, lifecycleState) -> lifecycleState.rollupIndexName()
+        );


I think this needs to be a bit more complicated, for example, in shrink we use ClusterStateWaitUntilThresholdStep to make the shrink "retryable", and to match that, I think this should be something like:

ClusterStateWaitUntilThresholdStep rollupAllocated = new ClusterStateWaitUntilThresholdStep( new WaitForIndexColorStep( waitForGreenRollupIndexKey, copyMetadataKey, ClusterHealthStatus.YELLOW, (indexName, lifecycleState) -> lifecycleState.rollupIndexName() ), cleanupRollupIndexKey );

I also changed this to "yellow" because I don't think it's necessary for us to wait the rollup to be green for 12 hours (in a single-node cluster for instance) before retrying.

I'm not 100% sure of all the failure scenarios of the rollup action, so what do you think—does this make sense?

It makes sense. I only wonder what would happen if rollup index is recovered (status turns yellow), we delete the source index and then we lose one of the nodes before the rollup index is replicated?
Is this scenario possible? Will it lead to data loss?

we delete the source index and then we lose one of the nodes before the rollup index is replicated? Is this scenario possible? Will it lead to data loss?

That is possible, but waiting for green would mean we are subject to waiting forever for single-node clusters, which is also out I think.

dakrone · 2022-07-13T15:30:16Z

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

-        }
+
+        // Here is where the actual rollup action takes place
+        RollupStep rollupStep = new RollupStep(rollupKey, waitForGreenRollupIndexKey, client, fixedInterval);


The RollupStep will fail when the rollup target index exists but doesn't have a "SUCCESS" status, but what we really need to do in this step is to go back to the cleanupRollupIndexKey so that the index can be deleted and we can try the rollup again. I think this should be similar to a branching step then, so that the failure scenario can provide a custom StepKey for going back to the cleanup step.

That's good catch. I preferred to delete the index at the same step, rather than redirecting to another step. At that point, I have all the information I need to remove the rollup index. I think this would be cleaner than redirecting to a previous step and adding a cleanup-step there.

Let me know if you think otherwise

dakrone · 2022-07-13T18:06:47Z

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

+                );
+                listener.onResponse(null);
+            } else {
+                listener.onFailure(


For example, here we'd want to jump back to the cleanup step (as I mentioned in my other comment)

…ilm/RollupILMAction.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>

…into tsdb-rollup-ilm

After merging PR elastic#88565, that delegates this task to the actual rollup step

csoulios · 2022-07-25T17:27:53Z

@dakrone After merging #88565, I have removed the copy-settings step.
Also, I have addressed all the other comments you have left.

Can you please have another look?

csoulios · 2022-07-25T17:33:43Z

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


-    public CopySettingsStep(StepKey key, StepKey nextStepKey, String indexPrefix, String... settingsKeys) {
+    private final BiFunction<String, LifecycleExecutionState, String> targetIndexNameSupplier;


Now that I have removed the copy-settings step from rollup ILM action, I can revert the CopySettingsStep so that it uses indexPrefix.

I only thought that making it more generic using an index name supplier function may be needed in the future. I don't have a strong opinion about keeping it this way or reverting it.

I think it's fine to keep for now

dakrone · 2022-07-25T19:13:25Z

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


-    public CopySettingsStep(StepKey key, StepKey nextStepKey, String indexPrefix, String... settingsKeys) {
+    private final BiFunction<String, LifecycleExecutionState, String> targetIndexNameSupplier;


I think it's fine to keep for now

dakrone · 2022-07-25T19:16:46Z

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

+        if (lifecycleState.lifecycleDate() == null) {
+            throw new IllegalStateException("source index [" + indexMetadata.getIndex().getName() + "] is missing lifecycle date");
+        }


Out of curiosity, why do we need to ensure that the lifecycle date is set here?

I don't have a good answer for this. I only copied it from the ShrinkStep thinking this may be needed later sinceShrinkStep and RollupStep are pretty similar in their operation.

Do you think I should remove this check?

No I don't think it's necessarily a bad check, I was just curious where it came from.

dakrone · 2022-07-25T19:21:55Z

.../qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/RollupActionIT.java

+        createIndexWithSettings(client(), index, alias, settings, mapping);
+    }
+
+    public void testRollupIndex() throws Exception {


Should these tests also perform a simple query to ensure that they can query the data stream without any problems after the rollup?

I added an assertion that the IndexMetadata.INDEX_ROLLUP_STATUS index setting is RollupTaskStatus.SUCCESS at the end of the process: 7345a57

For queries on rollup indexes there is the RollupActionSingleNodeTests class that validates the result of the rollup action.

dakrone · 2022-07-25T19:23:38Z

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

+        WaitForIndexColorStep waitForGreenIndexHealthStep = new WaitForIndexColorStep(
+            waitForGreenRollupIndexKey,
+            copyMetadataKey,
+            ClusterHealthStatus.GREEN,
+            (indexName, lifecycleState) -> lifecycleState.rollupIndexName()
+        );


we delete the source index and then we lose one of the nodes before the rollup index is replicated? Is this scenario possible? Will it lead to data loss?

That is possible, but waiting for green would mean we are subject to waiting forever for single-node clusters, which is also out I think.

dakrone

LGTM, I left a few really trivial comments/questions

Initial commit for Downsampling ILM Action

32519c0

csoulios added >feature :Data Management/ILM+SLM Index and Snapshot lifecycle management :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data :StorageEngine/TSDB You know, for Metrics v8.4.0 labels May 31, 2022

elasticmachine added Team:Data Management Meta label for data/management team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels May 31, 2022

csoulios marked this pull request as draft May 31, 2022 19:21

Update docs/changelog/87269.yaml

a34aa1c

csoulios mentioned this pull request May 31, 2022

Add better support for metric data types (TSDB) #74660

Closed

Fixed changelog

d003985

csoulios mentioned this pull request May 31, 2022

Enable TSDB downsampling ILM configuration elastic/kibana#130437

Closed

csoulios added 11 commits June 1, 2022 16:09

Minor change to test

f2c5471

Merge branch 'master' into tsdb-rollup-ilm

a9845fc

Remove ilm policy for rollup index

45bc157

Fix tests

Fix test

15650da

Merge branch 'master' into tsdb-rollup-ilm

d6e99d8

Merge branch 'master' into tsdb-rollup-ilm

b225e3f

Merge branch 'master' into tsdb-rollup-ilm

a625c8d

Added more tests

1f47891

Added tests for datastreams

c82bd6e

Checkstyle

9c0d422

Fix broken test

6bc6b15

csoulios requested a review from martijnvg June 16, 2022 10:25

csoulios added 2 commits July 13, 2022 16:35

Changed parameters for RollupILMAction

6d91aaa

Removed the `config` object and added `fixed_interval` param right in the `rollup` action

checkstyle

8bb250c

dakrone requested changes Jul 13, 2022

View reviewed changes

csoulios added 2 commits July 14, 2022 12:56

Merge branch 'master' into tsdb-rollup-ilm

4e28be7

Fix tests

78baf97

csoulios mentioned this pull request Jul 19, 2022

Remove TSDB feature flag #88585

Closed

csoulios and others added 4 commits July 19, 2022 15:41

Merge branch 'master' into tsdb-rollup-ilm

5b1c2dc

Update x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/…

5893ce0

…ilm/RollupILMAction.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>

Merge remote-tracking branch 'refs/remotes/csoulios/tsdb-rollup-ilm' …

40964b2

…into tsdb-rollup-ilm

Addressing reviewer comments

5bdea59

csoulios requested a review from dakrone July 19, 2022 16:38

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:07

csoulios added 5 commits July 25, 2022 12:40

Merge branch 'master' into tsdb-rollup-ilm

d121009

Merge branch 'main' into tsdb-rollup-ilm

66685c3

Merge branch 'main' into tsdb-rollup-ilm

b860625

Merge branch 'main' into tsdb-rollup-ilm

48dc8ae

Removed the copy-settings step

704e9a9

After merging PR elastic#88565, that delegates this task to the actual rollup step

csoulios commented Jul 25, 2022

View reviewed changes

dakrone reviewed Jul 25, 2022

View reviewed changes

dakrone approved these changes Jul 25, 2022

View reviewed changes

csoulios added 3 commits July 26, 2022 11:27

Merge branch 'main' into tsdb-rollup-ilm

5f38df3

Added assertion for rollup status

7345a57

Merge branch 'main' into tsdb-rollup-ilm

2d6d139

csoulios merged commit 55c0d1a into elastic:main Jul 26, 2022

csoulios deleted the tsdb-rollup-ilm branch July 26, 2022 15:27

joegallo added a commit that referenced this pull request Jul 26, 2022

Fix conflict between #83345 and #87269

a622864

csoulios added release highlight and removed release highlight labels Sep 26, 2022

csoulios mentioned this pull request Jan 31, 2023

Randomize Rollup Index name when one is not provided to Rollup Action #68225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TSDB: Implement Downsampling ILM Action for time-series indices #87269

TSDB: Implement Downsampling ILM Action for time-series indices #87269

csoulios commented May 31, 2022 •

edited

Loading

elasticmachine commented May 31, 2022

elasticmachine commented May 31, 2022

elasticmachine commented May 31, 2022

elasticsearchmachine commented May 31, 2022

dakrone left a comment

dakrone Jul 13, 2022

csoulios Jul 19, 2022

dakrone Jul 25, 2022 •

edited

Loading

dakrone Jul 13, 2022

csoulios Jul 19, 2022

dakrone Jul 13, 2022

csoulios commented Jul 25, 2022

csoulios Jul 25, 2022 •

edited

Loading

dakrone Jul 25, 2022

dakrone Jul 25, 2022

dakrone Jul 25, 2022

csoulios Jul 26, 2022

dakrone Jul 26, 2022

dakrone Jul 25, 2022

csoulios Jul 26, 2022 •

edited

Loading

dakrone Jul 25, 2022 •

edited

Loading

dakrone left a comment


		public CopySettingsStep(StepKey key, StepKey nextStepKey, String indexPrefix, String... settingsKeys) {
		private final BiFunction<String, LifecycleExecutionState, String> targetIndexNameSupplier;

TSDB: Implement Downsampling ILM Action for time-series indices #87269

TSDB: Implement Downsampling ILM Action for time-series indices #87269

Conversation

csoulios commented May 31, 2022 • edited Loading

elasticmachine commented May 31, 2022

elasticmachine commented May 31, 2022

elasticmachine commented May 31, 2022

elasticsearchmachine commented May 31, 2022

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

dakrone Jul 25, 2022 • 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

csoulios commented Jul 25, 2022

csoulios Jul 25, 2022 • 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

csoulios Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

dakrone Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

dakrone left a comment

Choose a reason for hiding this comment

csoulios commented May 31, 2022 •

edited

Loading

dakrone Jul 25, 2022 •

edited

Loading

csoulios Jul 25, 2022 •

edited

Loading

csoulios Jul 26, 2022 •

edited

Loading

dakrone Jul 25, 2022 •

edited

Loading