Skip to content

Commit

Permalink
Avoid race condition in ILMHistorySotre (#53039)
Browse files Browse the repository at this point in the history 
* Avoid race condition in ILMHistorySotre

This change modifies ILMHistoryStore to always apply correct settings and mappings,
even if template is deleted and not yet recreated. This ensures that ILM history index
is correctly managed by ILM and also fixes flaky history tests that were prone to
triggenring this race.

This commit also refactors and simplifies ILM history tests.

Closes #50353 and #52853

* Review comment

Co-authored-by: Elastic Machine <[email protected]>
  • Loading branch information
@probakowski @elasticmachine
probakowski and elasticmachine authored Mar 4, 2020
1 parent 1278960 commit 2a9c1f8
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 53 deletions.
58 changes: 8 additions & 50 deletions ...qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1358,10 +1358,7 @@ public void testWaitForActiveShardsStep() throws Exception {
assertBusy(() -> assertThat(getStepKeyForIndex(originalIndex), equalTo(PhaseCompleteStep.finalStep("hot").getKey())));
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/50353")
public void testHistoryIsWrittenWithSuccess() throws Exception {
String index = "success-index";

createNewSingletonPolicy("hot", new RolloverAction(null, null, 1L));
Request createIndexTemplate = new Request("PUT", "_template/rolling_indexes");
createIndexTemplate.setJsonEntity("{" +
Expand All @@ -1375,10 +1372,7 @@ public void testHistoryIsWrittenWithSuccess() throws Exception {
"}");
client().performRequest(createIndexTemplate);

createIndexWithSettings(index + "-1",
Settings.builder().put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1)
.put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0),
true);
createIndexWithSettings(index + "-1", Settings.builder(), true);

// Index a document
index(client(), index + "-1", "1", "foo", "bar");
Expand All @@ -1396,69 +1390,34 @@ public void testHistoryIsWrittenWithSuccess() throws Exception {
assertBusy(() -> assertHistoryIsPresent(policy, index + "-1", true, "wait-for-yellow-step"), 30, TimeUnit.SECONDS);
assertBusy(() -> assertHistoryIsPresent(policy, index + "-1", true, "check-rollover-ready"), 30, TimeUnit.SECONDS);
assertBusy(() -> assertHistoryIsPresent(policy, index + "-1", true, "attempt-rollover"), 30, TimeUnit.SECONDS);
assertBusy(() -> assertHistoryIsPresent(policy, index + "-1", true, "update-rollover-lifecycle-date"), 30, TimeUnit.SECONDS);
assertBusy(() -> assertHistoryIsPresent(policy, index + "-1", true, "set-indexing-complete"), 30, TimeUnit.SECONDS);
assertBusy(() -> assertHistoryIsPresent(policy, index + "-1", true, "completed"), 30, TimeUnit.SECONDS);
assertBusy(() -> assertHistoryIsPresent(policy, index + "-1", true, "complete"), 30, TimeUnit.SECONDS);

assertBusy(() -> assertHistoryIsPresent(policy, index + "-000002", true, "check-rollover-ready"), 30, TimeUnit.SECONDS);
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/50353")
public void testHistoryIsWrittenWithFailure() throws Exception {
String index = "failure-index";

createIndexWithSettings(index + "-1", Settings.builder(), false);
createNewSingletonPolicy("hot", new RolloverAction(null, null, 1L));
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" +
" }\n" +
"}");
client().performRequest(createIndexTemplate);

createIndexWithSettings(index + "-1",
Settings.builder().put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1)
.put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0),
false);
updatePolicy(index + "-1", policy);

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

assertBusy(() -> assertThat(getStepKeyForIndex(index + "-1").getName(), equalTo(ErrorStep.NAME)));
assertBusy(() -> assertThat(getStepKeyForIndex(index + "-1").getName(), equalTo(ErrorStep.NAME)), 30, TimeUnit.SECONDS);

assertBusy(() -> assertHistoryIsPresent(policy, index + "-1", false, "ERROR"), 30, TimeUnit.SECONDS);
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/50353")
public void testHistoryIsWrittenWithDeletion() throws Exception {
String index = "delete-index";

createNewSingletonPolicy("delete", new DeleteAction());
Request createIndexTemplate = new Request("PUT", "_template/delete_indexes");
createIndexTemplate.setJsonEntity("{" +
"\"index_patterns\": [\""+ index + "\"], \n" +
" \"settings\": {\n" +
" \"number_of_shards\": 1,\n" +
" \"number_of_replicas\": 0,\n" +
" \"index.lifecycle.name\": \"" + policy+ "\"\n" +
" }\n" +
"}");
client().performRequest(createIndexTemplate);

// Index should be created and then deleted by ILM
createIndexWithSettings(index, Settings.builder(), false);
createNewSingletonPolicy("delete", new DeleteAction());
updatePolicy(index, policy);

assertBusy(() -> {
logger.info("--> checking for index deletion...");
Request existCheck = new Request("HEAD", "/" + index);
Response resp = client().performRequest(existCheck);
assertThat(resp.getStatusLine().getStatusCode(), equalTo(404));
});
assertBusy(() -> assertFalse(indexExists(index)));

assertBusy(() -> {
assertHistoryIsPresent(policy, index, true, "delete", "delete", "wait-for-shard-history-leases");
Expand Down Expand Up @@ -1593,7 +1552,6 @@ private void assertHistoryIsPresent(String policyName, String indexName, boolean
// This method should be called inside an assertBusy, it has no retry logic of its own
private void assertHistoryIsPresent(String policyName, String indexName, boolean success,
@Nullable String phase, @Nullable String action, String stepName) throws IOException {
assertOK(client().performRequest(new Request("POST", indexName + "/_refresh")));
logger.info("--> checking for history item [{}], [{}], success: [{}], phase: [{}], action: [{}], step: [{}]",
policyName, indexName, success, phase, action, stepName);
final Request historySearchRequest = new Request("GET", "ilm-history*/_search?expand_wildcards=all");
Expand Down
19 changes: 16 additions & 3 deletions x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/history/ILMHistoryStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.AliasOrIndex;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.threadpool.ThreadPool;

import java.io.Closeable;
Expand All @@ -46,6 +49,7 @@
import static org.elasticsearch.xpack.core.ClientHelper.INDEX_LIFECYCLE_ORIGIN;
import static org.elasticsearch.xpack.core.ilm.LifecycleSettings.LIFECYCLE_HISTORY_INDEX_ENABLED_SETTING;
import static org.elasticsearch.xpack.ilm.history.ILMHistoryTemplateRegistry.INDEX_TEMPLATE_VERSION;
import static org.elasticsearch.xpack.ilm.history.ILMHistoryTemplateRegistry.TEMPLATE_ILM_HISTORY;

/**
* The {@link ILMHistoryStore} handles indexing {@link ILMHistoryItem} documents into the
Expand Down Expand Up @@ -175,6 +179,7 @@ public void putAsync(ILMHistoryItem item) {
* @param listener Called after the index has been created. `onResponse` called with `true` if the index was created,
* `false` if it already existed.
*/
@SuppressWarnings("unchecked")
static void ensureHistoryIndex(Client client, ClusterState state, ActionListener<Boolean> listener) {
final String initialHistoryIndexName = ILM_HISTORY_INDEX_PREFIX + "000001";
final AliasOrIndex ilmHistory = state.metaData().getAliasAndIndexLookup().get(ILM_HISTORY_ALIAS);
Expand All @@ -183,11 +188,19 @@ static void ensureHistoryIndex(Client client, ClusterState state, ActionListener
if (ilmHistory == null && initialHistoryIndex == null) {
// No alias or index exists with the expected names, so create the index with appropriate alias
logger.debug("creating ILM history index [{}]", initialHistoryIndexName);

// Template below should be already defined as real index template but it can be deleted. To avoid race condition with its
// recreation we apply settings and mappings ourselves
byte[] templateBytes = TEMPLATE_ILM_HISTORY.loadBytes();
Map<String, Object> templateAsMap = XContentHelper.convertToMap(new BytesArray(templateBytes, 0, templateBytes.length),
false, XContentType.JSON).v2();

client.admin().indices().prepareCreate(initialHistoryIndexName)
.setSettings((Map<String, ?>) templateAsMap.get("settings"))
.setMapping((Map<String, Object>) templateAsMap.get("mappings"))
.setWaitForActiveShards(1)
.addAlias(new Alias(ILM_HISTORY_ALIAS)
.writeIndex(true))
.execute(new ActionListener<CreateIndexResponse>() {
.addAlias(new Alias(ILM_HISTORY_ALIAS).writeIndex(true))
.execute(new ActionListener<>() {
@Override
public void onResponse(CreateIndexResponse response) {
listener.onResponse(true);
Expand Down

0 comments on commit 2a9c1f8

Please sign in to comment.