Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ILM: Skip rolling indexes that are already rolled #47324

Merged
merged 10 commits into from
Oct 4, 2019
Merged

ILM: Skip rolling indexes that are already rolled #47324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
51b631f
ILM: Skip rolling indexes that are already rolled
andreidan Sep 30, 2019
3ae12a7
Fix test that expects ILM to fail when rolling over a rolled index
andreidan Sep 30, 2019
3a92289
Merge branch 'master' into ilm-skip-rolled-index
elasticmachine Oct 1, 2019
617a2f4
Drop the index.lifecycle.rollover.skip_rolled setting
andreidan Oct 4, 2019
2a70f55
Complete the WaitForRolloverReadyStep
andreidan Oct 4, 2019
837f98a
RolloverStep checks for already rolled index
andreidan Oct 4, 2019
ffaa20d
Adapt readd policy test to not expect error
andreidan Oct 4, 2019
fabc7bf
ILM w rollover on already rolled index integ test
andreidan Oct 4, 2019
5c71c38
Merge branch 'master' into ilm-skip-rolled-index
elasticmachine Oct 4, 2019
742f6c2
Remove docs for index.lifecycle.rollover.skip_rolled
andreidan Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ public void performAction(IndexMetaData indexMetaData, ClusterState currentClust
return;
}

if (indexMetaData.getRolloverInfos().get(rolloverAlias) != null) {
logger.info("index [{}] was already rolled over for alias [{}], not attempting to roll over again",
indexMetaData.getIndex().getName(), rolloverAlias);
listener.onResponse(true);
return;
}

if (indexMetaData.getAliases().containsKey(rolloverAlias) == false) {
listener.onFailure(new IllegalArgumentException(String.format(Locale.ROOT,
"%s [%s] does not point to index [%s]", RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, rolloverAlias,
Expand Down
7 changes: 7 additions & 0 deletions .../plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ public void evaluateCondition(IndexMetaData indexMetaData, Listener listener) {
return;
}

if (indexMetaData.getRolloverInfos().get(rolloverAlias) != null) {
logger.info("index [{}] was already rolled over for alias [{}], not attempting to roll over again",
indexMetaData.getIndex().getName(), rolloverAlias);
listener.onResponse(true, new WaitForRolloverReadyStep.EmptyInfo());
return;
dakrone marked this conversation as resolved.
Show resolved Hide resolved
}

// The order of the following checks is important in ways which may not be obvious.

// First, figure out if 1) The configured alias points to this index, and if so,
Expand Down
37 changes: 37 additions & 0 deletions x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/RolloverStepTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.admin.indices.rollover.MaxSizeCondition;
import org.elasticsearch.action.admin.indices.rollover.RolloverInfo;
import org.elasticsearch.action.admin.indices.rollover.RolloverRequest;
import org.elasticsearch.action.admin.indices.rollover.RolloverResponse;
import org.elasticsearch.client.AdminClient;
import org.elasticsearch.client.Client;
import org.elasticsearch.client.IndicesAdminClient;
import org.elasticsearch.cluster.metadata.AliasMetaData;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.xpack.core.ilm.Step.StepKey;
import org.junit.Before;
import org.mockito.Mockito;
Expand All @@ -25,6 +28,7 @@
import java.util.Locale;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.core.Is.is;

public class RolloverStepTests extends AbstractStepTestCase<RolloverStep> {

Expand Down Expand Up @@ -154,6 +158,39 @@ public void onFailure(Exception e) {
assertEquals(true, actionCompleted.get());
}

public void testPerformActionSkipsRolloverForAlreadyRolledIndex() {
String rolloverAlias = randomAlphaOfLength(5);
IndexMetaData indexMetaData = IndexMetaData.builder(randomAlphaOfLength(10))
.putAlias(AliasMetaData.builder(rolloverAlias))
.settings(settings(Version.CURRENT).put(RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, rolloverAlias))
.putRolloverInfo(new RolloverInfo(rolloverAlias,
Collections.singletonList(new MaxSizeCondition(new ByteSizeValue(2L))),
System.currentTimeMillis())
)
.numberOfShards(randomIntBetween(1, 5)).numberOfReplicas(randomIntBetween(0, 5)).build();

RolloverStep step = createRandomInstance();
AdminClient adminClient = Mockito.mock(AdminClient.class);
IndicesAdminClient indicesClient = Mockito.mock(IndicesAdminClient.class);
Mockito.when(client.admin()).thenReturn(adminClient);
Mockito.when(adminClient.indices()).thenReturn(indicesClient);

step.performAction(indexMetaData, null, null, new AsyncActionStep.Listener() {

@Override
public void onResponse(boolean complete) {
assertThat(complete, is(true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes! Good shout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@Override
public void onFailure(Exception e) {
throw new AssertionError("Unexpected method call", e);
}
});

Mockito.verify(indicesClient, Mockito.never()).rolloverIndex(Mockito.any(), Mockito.any());
}

public void testPerformActionFailure() {
String alias = randomAlphaOfLength(5);
IndexMetaData indexMetaData = IndexMetaData.builder(randomAlphaOfLength(10))
Expand Down
69 changes: 69 additions & 0 deletions ...in/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.elasticsearch.action.admin.indices.rollover.MaxAgeCondition;
import org.elasticsearch.action.admin.indices.rollover.MaxDocsCondition;
import org.elasticsearch.action.admin.indices.rollover.MaxSizeCondition;
import org.elasticsearch.action.admin.indices.rollover.RolloverInfo;
import org.elasticsearch.action.admin.indices.rollover.RolloverRequest;
import org.elasticsearch.action.admin.indices.rollover.RolloverResponse;
import org.elasticsearch.client.AdminClient;
Expand All @@ -29,13 +30,15 @@
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;

import java.util.Collections;
import java.util.HashSet;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;

public class WaitForRolloverReadyStepTests extends AbstractStepTestCase<WaitForRolloverReadyStep> {

Expand Down Expand Up @@ -173,6 +176,64 @@ public void onFailure(Exception e) {
Mockito.verify(indicesClient, Mockito.only()).rolloverIndex(Mockito.any(), Mockito.any());
}

public void testEvaluateDoesntTriggerRolloverForIndexManuallyRolledOnLifecycleRolloverAlias() {
String rolloverAlias = randomAlphaOfLength(5);
IndexMetaData indexMetaData = IndexMetaData.builder(randomAlphaOfLength(10))
.putAlias(AliasMetaData.builder(rolloverAlias))
.settings(settings(Version.CURRENT).put(RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, rolloverAlias))
.putRolloverInfo(new RolloverInfo(rolloverAlias, Collections.singletonList(new MaxSizeCondition(new ByteSizeValue(2L))),
System.currentTimeMillis()))
.numberOfShards(randomIntBetween(1, 5)).numberOfReplicas(randomIntBetween(0, 5)).build();

WaitForRolloverReadyStep step = createRandomInstance();
IndicesAdminClient indicesClient = indicesAdminClientMock();

step.evaluateCondition(indexMetaData, new AsyncWaitStep.Listener() {

@Override
public void onResponse(boolean complete, ToXContentObject informationContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably check that complete is true.

assertThat(complete, is(true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@Override
public void onFailure(Exception e) {
throw new AssertionError("Unexpected method call", e);
}
});

Mockito.verify(indicesClient, Mockito.never()).rolloverIndex(Mockito.any(), Mockito.any());
}

public void testEvaluateTriggersRolloverForIndexManuallyRolledOnDifferentAlias() {
String rolloverAlias = randomAlphaOfLength(5);
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),
Copy link
Contributor Author

@andreidan andreidan Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Collections.singletonList(new MaxSizeCondition(new ByteSizeValue(2L))),
System.currentTimeMillis())
)
.numberOfShards(randomIntBetween(1, 5)).numberOfReplicas(randomIntBetween(0, 5)).build();

WaitForRolloverReadyStep step = createRandomInstance();
IndicesAdminClient indicesClient = indicesAdminClientMock();

step.evaluateCondition(indexMetaData, new AsyncWaitStep.Listener() {

@Override
public void onResponse(boolean complete, ToXContentObject informationContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably check that complete is true.

assertThat(complete, is(true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here about the latch :)

}

@Override
public void onFailure(Exception e) {
throw new AssertionError("Unexpected method call", e);
}
});

Mockito.verify(indicesClient, Mockito.only()).rolloverIndex(Mockito.any(), Mockito.any());
}

public void testPerformActionWithIndexingComplete() {
String alias = randomAlphaOfLength(5);
IndexMetaData indexMetaData = IndexMetaData.builder(randomAlphaOfLength(10))
Expand Down Expand Up @@ -399,4 +460,12 @@ public void onFailure(Exception e) {
"%s [%s] does not point to index [%s]", RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, alias,
indexMetaData.getIndex().getName())));
}

private IndicesAdminClient indicesAdminClientMock() {
AdminClient adminClient = Mockito.mock(AdminClient.class);
IndicesAdminClient indicesClient = Mockito.mock(IndicesAdminClient.class);
Mockito.when(client.admin()).thenReturn(adminClient);
Mockito.when(adminClient.indices()).thenReturn(indicesClient);
return indicesClient;
}
}
87 changes: 73 additions & 14 deletions ...qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,20 +760,6 @@ public void testRemoveAndReaddPolicy() throws Exception {
client().performRequest(addPolicyRequest);
assertBusy(() -> assertTrue((boolean) explainIndex(originalIndex).getOrDefault("managed", false)));

// Wait for rollover to error
assertBusy(() -> assertThat(getStepKeyForIndex(originalIndex), equalTo(new StepKey("hot", RolloverAction.NAME, ErrorStep.NAME))));

// Set indexing complete
Request setIndexingCompleteRequest = new Request("PUT", "/" + originalIndex + "/_settings");
setIndexingCompleteRequest.setJsonEntity("{\n" +
" \"index.lifecycle.indexing_complete\": true\n" +
"}");
client().performRequest(setIndexingCompleteRequest);

// Retry policy
Request retryRequest = new Request("POST", "/" + originalIndex + "/_ilm/retry");
client().performRequest(retryRequest);

// Wait for everything to be copacetic
assertBusy(() -> assertThat(getStepKeyForIndex(originalIndex), equalTo(TerminalPolicyStep.KEY)));
}
Expand Down Expand Up @@ -875,6 +861,79 @@ public void testExplainFilters() throws Exception {
});
}

public void testILMRolloverOnManuallyRolledIndex() throws Exception {
String originalIndex = index + "-000001";
String secondIndex = index + "-000002";
String thirdIndex = index + "-000003";

// Configure ILM to run every second
Request updateLifecylePollSetting = new Request("PUT", "_cluster/settings");
updateLifecylePollSetting.setJsonEntity("{" +
" \"transient\": {\n" +
"\"indices.lifecycle.poll_interval\" : \"1s\" \n" +
" }\n" +
"}");
client().performRequest(updateLifecylePollSetting);

// Set up a policy with rollover
createNewSingletonPolicy("hot", new RolloverAction(null, null, 2L));
Request createIndexTemplate = new Request("PUT", "_template/rolling_indexes");
createIndexTemplate.setJsonEntity("{" +
"\"index_patterns\": [\""+ index + "-*\"], \n" +
" \"settings\": {\n" +
" \"number_of_shards\": 1,\n" +
" \"number_of_replicas\": 0,\n" +
" \"index.lifecycle.name\": \"" + policy+ "\", \n" +
" \"index.lifecycle.rollover_alias\": \"alias\"\n" +
" }\n" +
"}");
client().performRequest(createIndexTemplate);

createIndexWithSettings(
originalIndex,
Settings.builder().put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1)
.put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0),
true
);

// Index a document
index(client(), originalIndex, "1", "foo", "bar");
Request refreshOriginalIndex = new Request("POST", "/" + originalIndex + "/_refresh");
client().performRequest(refreshOriginalIndex);

// Manual rollover
Request rolloverRequest = new Request("POST", "/alias/_rollover");
rolloverRequest.setJsonEntity("{\n" +
" \"conditions\": {\n" +
" \"max_docs\": \"1\"\n" +
" }\n" +
"}"
);
client().performRequest(rolloverRequest);
assertBusy(() -> assertTrue(indexExists(secondIndex)));

// Index another document into the original index so the ILM rollover policy condition is met
index(client(), originalIndex, "2", "foo", "bar");
client().performRequest(refreshOriginalIndex);

// Wait for the rollover policy to execute
assertBusy(() -> assertThat(getStepKeyForIndex(originalIndex), equalTo(TerminalPolicyStep.KEY)));

// ILM should manage the second index after attempting (and skipping) rolling the original index
assertBusy(() -> assertTrue((boolean) explainIndex(secondIndex).getOrDefault("managed", true)));

// index some documents to trigger an ILM rollover
index(client(), "alias", "1", "foo", "bar");
index(client(), "alias", "2", "foo", "bar");
index(client(), "alias", "3", "foo", "bar");
Request refreshSecondIndex = new Request("POST", "/" + secondIndex + "/_refresh");
client().performRequest(refreshSecondIndex).getStatusLine();

// ILM should rollover the second index even though it skipped the first one
assertBusy(() -> assertThat(getStepKeyForIndex(secondIndex), equalTo(TerminalPolicyStep.KEY)));
assertBusy(() -> assertTrue(indexExists(thirdIndex)));
}

private void createFullPolicy(TimeValue hotTime) throws IOException {
Map<String, LifecycleAction> hotActions = new HashMap<>();
hotActions.put(SetPriorityAction.NAME, new SetPriorityAction(100));
Expand Down