Avoid race condition in ILMHistorySotre (elastic#53039)

* 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 elastic#50353 and elastic#52853

* Review comment

Co-authored-by: Elastic Machine <[email protected]>

Author: probakowski

x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java

@@ -1367,10 +1367,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("{" +
@@ -1384,10 +1381,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");
@@ -1405,69 +1399,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");

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/history/ILMHistoryStore.java

@@ -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;
@@ -45,6 +48,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
@@ -163,6 +167,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);
@@ -171,11 +176,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);