ILM: Skip rolling indexes that are already rolled #47324

andreidan · 2019-09-30T21:27:35Z

An index with an ILM policy that has a rollover action in one of the
phases was rolled over when the ILM conditions dictated regardless if
it was already rolled over (eg. manually after modifying an index
template in order to force the creation of a new index that uses the new
mappings).
This changes this behaviour and has ILM check if the index it's about to
roll has not been rolled over in the meantime.

Closes #44175

An index with an ILM policy that has a `rollover` action in one of the phases was rolled over when the ILM conditions dictated regardless if it was already rolled over (eg. manually after modifying an index template in order to force the creation of a new index that uses the new mappings). This commit adds the `index.lifecycle.rollover.skip_rolled` setting, which defaults to true, to change this behaviour and have ILM check if the index it's about to roll has not been rolled over in the meantime.

elasticmachine · 2019-09-30T21:27:36Z

Pinging @elastic/es-core-features

andreidan · 2019-09-30T21:28:50Z

docs/reference/settings/ilm-settings.asciidoc

@@ -23,6 +23,17 @@ policy that contains a rollover action. When the index rolls over, the alias is
 updated to reflect that the index is no longer the write index. For more
 information about rollover, see <<using-policies-rollover>>.

+`index.lifecycle.rollover.skip_rolled`:: (Expert Setting)


Is there a better suggestion for naming this setting? I also went ahead and made it true by default. What do you think?

allow_multiple_rollovers maybe? I don't love that either.
skip_rollover_if_already_rolled? More verbose, but that might be okay.

I'm honestly not sure why someone would want to change this setting in the first place. It's probably good to have a setting for it just in case, but I can't think of any actual use case for it.

I think this setting is a little strange, if a user were to set it to false they would essentially be rolling over an index that was no longer the write index, and in the case that ILM is configured like it usually is (the policy is applied to every index), it will lead to unintended behavior.

I think we should remove this setting, since to me this feels a little like having a setting to re-introduce buggy behavior.

andreidan · 2019-09-30T21:32:48Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

+        IndexMetaData indexMetaData = IndexMetaData.builder(randomAlphaOfLength(10))
+            .putAlias(AliasMetaData.builder(rolloverAlias))
+            .settings(settings(Version.CURRENT).put(RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, rolloverAlias))
+            .putRolloverInfo(new RolloverInfo(randomAlphaOfLength(5),


do you think this scenario makes sense? (ie. allowing ILM to attempt a rollover if the index was manually rolled under a different alias or should it skip rolling over a rolled index regardless of the alias?)

This is like, the corneriest of corner cases. I suspect the behavior in this case will not ever matter. That said, I think we should only pay attention to rollovers using the same alias, and if we're doing that intentionally, might as well test it.

I think we should check to see if rolling the index using a different alias prompts the same error that #44175 sees (manually is fine), and if so, we should fix that too. I think Gordon is right that it won't really matter.

gwbrown

Looks like a good start, thanks! Left some comments, I also think an integration test in TimeSeriesLifecycleActionsIT would probably be good here to make sure we get through all the steps in rollover without erroring.

gwbrown · 2019-09-30T23:35:07Z

docs/reference/settings/ilm-settings.asciidoc

@@ -23,6 +23,17 @@ policy that contains a rollover action. When the index rolls over, the alias is
 updated to reflect that the index is no longer the write index. For more
 information about rollover, see <<using-policies-rollover>>.

+`index.lifecycle.rollover.skip_rolled`:: (Expert Setting)


allow_multiple_rollovers maybe? I don't love that either.
skip_rollover_if_already_rolled? More verbose, but that might be okay.

I'm honestly not sure why someone would want to change this setting in the first place. It's probably good to have a setting for it just in case, but I can't think of any actual use case for it.

gwbrown · 2019-09-30T23:37:51Z

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

@@ -53,6 +55,12 @@ public void evaluateCondition(IndexMetaData indexMetaData, Listener listener) {
            return;
        }

+        if (indexMetaData.getSettings().getAsBoolean(LIFECYCLE_ROLLOVER_SKIP_ROLLED, true) &&
+            indexMetaData.getRolloverInfos().get(rolloverAlias) != null) {
+            logger.trace("Index {} was already rolled, skipping", indexMetaData.getIndex().getName());


I think this should be info (rather than `trace)

Use brackets around index name, and don't capitalize, to fit better with the typical log format

Nitpicky, but I'd use a message more like index [{}] was already rolled over for alias [{}], not attempting to roll over again

gwbrown · 2019-09-30T23:41:59Z

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

@@ -53,6 +55,12 @@ public void evaluateCondition(IndexMetaData indexMetaData, Listener listener) {
            return;
        }

+        if (indexMetaData.getSettings().getAsBoolean(LIFECYCLE_ROLLOVER_SKIP_ROLLED, true) &&


I think we'll also need this check in RolloverStep, or it'll just try to issue the rollover request there and fail a step later. I think UpdateRolloverLifecycleDateStepis fine not checking it, because we should probably update the reference date anyway, and the step to set indexing complete is probably good to do anyway.

+1, otherwise we pass the check step but fail on the "actual" rollover

Ah, I wasn't seeing this in the manual tests as we need to complete this step here (call the listener callback). I'll add the check to the RolloverStep too with the listener callback as well. Will capture this in an integration test too as you guys suggested

gwbrown · 2019-09-30T23:51:20Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

+        IndexMetaData indexMetaData = IndexMetaData.builder(randomAlphaOfLength(10))
+            .putAlias(AliasMetaData.builder(rolloverAlias))
+            .settings(settings(Version.CURRENT).put(RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, rolloverAlias))
+            .putRolloverInfo(new RolloverInfo(randomAlphaOfLength(5),


This is like, the corneriest of corner cases. I suspect the behavior in this case will not ever matter. That said, I think we should only pay attention to rollovers using the same alias, and if we're doing that intentionally, might as well test it.

gwbrown · 2019-09-30T23:52:55Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

+        step.evaluateCondition(indexMetaData, new AsyncWaitStep.Listener() {
+
+            @Override
+            public void onResponse(boolean complete, ToXContentObject informationContext) {


This should probably check that complete is true.

gwbrown · 2019-09-30T23:52:58Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

+        step.evaluateCondition(indexMetaData, new AsyncWaitStep.Listener() {
+
+            @Override
+            public void onResponse(boolean complete, ToXContentObject informationContext) {


This should probably check that complete is true.

gwbrown · 2019-09-30T23:56:10Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

+        Mockito.verify(indicesClient, Mockito.only()).rolloverIndex(Mockito.any(), Mockito.any());
+    }
+
+    private IndicesAdminClient indicesAdminClientMock() {


Nitpick: Helper methods should come after all of the actual test methods.

dakrone

Thanks for working on this Andrei! I left some comments, in particular there is a bug where we don't actually advance the step. I think it's not caught because we don't have any integration tests for this (only unit tests). I think we should add some REST tests (should be something we can exercise from a .yaml test) for manually rolling over an index.

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

dakrone · 2019-10-01T02:42:21Z

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

@@ -53,6 +55,12 @@ public void evaluateCondition(IndexMetaData indexMetaData, Listener listener) {
            return;
        }

+        if (indexMetaData.getSettings().getAsBoolean(LIFECYCLE_ROLLOVER_SKIP_ROLLED, true) &&


+1, otherwise we pass the check step but fail on the "actual" rollover

dakrone · 2019-10-01T02:45:19Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

+        IndexMetaData indexMetaData = IndexMetaData.builder(randomAlphaOfLength(10))
+            .putAlias(AliasMetaData.builder(rolloverAlias))
+            .settings(settings(Version.CURRENT).put(RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, rolloverAlias))
+            .putRolloverInfo(new RolloverInfo(randomAlphaOfLength(5),


I think we should check to see if rolling the index using a different alias prompts the same error that #44175 sees (manually is fine), and if so, we should fix that too. I think Gordon is right that it won't really matter.

dakrone · 2019-10-01T02:47:30Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

+    }
+
+    private IndicesAdminClient indicesAdminClientMock() {
+        AdminClient adminClient = Mockito.mock(AdminClient.class);


I would vastly prefer that we subclass NoOpClient to test this rather than using mocks, but since this test already has precedence for using them, it's okay.

dakrone · 2019-10-01T02:52:45Z

docs/reference/settings/ilm-settings.asciidoc

@@ -23,6 +23,17 @@ policy that contains a rollover action. When the index rolls over, the alias is
 updated to reflect that the index is no longer the write index. For more
 information about rollover, see <<using-policies-rollover>>.

+`index.lifecycle.rollover.skip_rolled`:: (Expert Setting)


I think this setting is a little strange, if a user were to set it to false they would essentially be rolling over an index that was no longer the write index, and in the case that ILM is configured like it usually is (the policy is applied to every index), it will lead to unintended behavior.

I think we should remove this setting, since to me this feels a little like having a setting to re-introduce buggy behavior.

andreidan · 2019-10-01T14:26:28Z

Thanks for your comments @dakrone and @gwbrown. I agree on the setting being odd (and offers little value). I'll drop it.

#47324 (comment)

@dakrone a manual rollover on a different alias between ilm iterations will cause the ilm rollover to fail because the target index will already have been created (eg. a manual roll of logs-001 will create logs-002. when ilm will attempt to rollover the pre-conditions for rolling logs-001 will be valid but the rollover will fail because logs-002 already exists - unless of course the user archived/deleted/renamed it but I can't really think of an use case where this makes sense)

andreidan · 2019-10-01T15:03:53Z

@elasticmachine update branch

andreidan · 2019-10-04T12:29:59Z

@elasticmachine update branch

dakrone

LGTM, I did leave a comment about a minor change to the test, but it shouldn't need another review. Thanks for iterating on this Andrei!

dakrone · 2019-10-04T15:51:45Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/RolloverStepTests.java

+
+            @Override
+            public void onResponse(boolean complete) {
+                assertThat(complete, is(true));


Since this is asynchronous, it's possible that the onResponse never actually gets called (we don't verify that it was called), can you add a CountDownLatch that in onResponse is counted down, we can then .await() on the latch and ensure that onResponse did check the complete boolean?

Ah yes! Good shout

Actually, this would be true in an integration test but in this case we call evaluateStep from the main test thread, making this effectively synchronous.

dakrone · 2019-10-04T15:52:24Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

+
+            @Override
+            public void onResponse(boolean complete, ToXContentObject informationContext) {
+                assertThat(complete, is(true));


Same comment here about the latch, we need to check that onResponse is actually called in the test.

dakrone · 2019-10-04T15:56:13Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

+
+            @Override
+            public void onResponse(boolean complete, ToXContentObject informationContext) {
+                assertThat(complete, is(true));


Same comment here about the latch :)

An index with an ILM policy that has a rollover action in one of the phases was rolled over when the ILM conditions dictated regardless if it was already rolled over (eg. manually after modifying an index template in order to force the creation of a new index that uses the new mappings). This changes this behaviour and has ILM check if the index it's about to roll has not been rolled over in the meantime. (cherry picked from commit 37d6106) Signed-off-by: Andrei Dan <andrei.dan@elastic.co>

andreidan added >enhancement :Data Management/ILM+SLM Index and Snapshot lifecycle management v8.0.0 v7.5.0 labels Sep 30, 2019

andreidan requested review from gwbrown and dakrone September 30, 2019 21:27

andreidan commented Sep 30, 2019

View reviewed changes

Fix test that expects ILM to fail when rolling over a rolled index

3ae12a7

gwbrown requested changes Sep 30, 2019

View reviewed changes

dakrone requested changes Oct 1, 2019

View reviewed changes

elasticmachine and others added 6 commits October 1, 2019 08:04

Merge branch 'master' into ilm-skip-rolled-index

3a92289

Drop the index.lifecycle.rollover.skip_rolled setting

617a2f4

Complete the WaitForRolloverReadyStep

2a70f55

RolloverStep checks for already rolled index

837f98a

Adapt readd policy test to not expect error

ffaa20d

ILM w rollover on already rolled index integ test

fabc7bf

elasticmachine and others added 2 commits October 4, 2019 05:30

Merge branch 'master' into ilm-skip-rolled-index

5c71c38

Remove docs for index.lifecycle.rollover.skip_rolled

742f6c2

andreidan requested review from dakrone and gwbrown October 4, 2019 13:20

dakrone approved these changes Oct 4, 2019

View reviewed changes

andreidan merged commit 37d6106 into elastic:master Oct 4, 2019

andreidan added the backport pending label Oct 4, 2019

andreidan mentioned this pull request Oct 4, 2019

ILM: Skip rolling indexes that are already rolled (#47324) #47592

Merged

andreidan removed the backport pending label Oct 7, 2019

dakrone mentioned this pull request Oct 24, 2019

ILM: Changing a policy does not have desired effect. #48431

Closed

gwbrown mentioned this pull request Nov 27, 2019

enhancement: provide an API to manually rollover an index that currently has an ILM policy #49650

Closed

bhiravabhatla mentioned this pull request Dec 7, 2020

Add support for Elasticsearch ILM Polices jaegertracing/jaeger#2454

Closed

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: Skip rolling indexes that are already rolled #47324

ILM: Skip rolling indexes that are already rolled #47324

andreidan commented Sep 30, 2019 •

edited

Loading

elasticmachine commented Sep 30, 2019

andreidan Sep 30, 2019

gwbrown Sep 30, 2019

dakrone Oct 1, 2019

andreidan Sep 30, 2019 •

edited

Loading

gwbrown Sep 30, 2019

dakrone Oct 1, 2019

gwbrown left a comment

gwbrown Sep 30, 2019

gwbrown Sep 30, 2019

gwbrown Sep 30, 2019

dakrone Oct 1, 2019

andreidan Oct 1, 2019

gwbrown Sep 30, 2019

gwbrown Sep 30, 2019

gwbrown Sep 30, 2019

gwbrown Sep 30, 2019

dakrone left a comment

dakrone Oct 1, 2019

dakrone Oct 1, 2019

dakrone Oct 1, 2019

dakrone Oct 1, 2019

andreidan commented Oct 1, 2019

andreidan commented Oct 1, 2019

andreidan commented Oct 4, 2019

dakrone left a comment

dakrone Oct 4, 2019

andreidan Oct 4, 2019

andreidan Oct 4, 2019

dakrone Oct 4, 2019

dakrone Oct 4, 2019

ILM: Skip rolling indexes that are already rolled #47324

ILM: Skip rolling indexes that are already rolled #47324

Conversation

andreidan commented Sep 30, 2019 • edited Loading

elasticmachine commented Sep 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreidan Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwbrown 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

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 Oct 1, 2019

andreidan commented Oct 1, 2019

andreidan commented Oct 4, 2019

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

andreidan commented Sep 30, 2019 •

edited

Loading

andreidan Sep 30, 2019 •

edited

Loading