[ML] Use a new annotations index for future annotations (elastic#79006)

Due to historical bugs a non-negligible proportion of ML users have an annotations index with incorrect mappings. We have published instructions on how to correct the mappings, but the procedure is complicated, and some users prefer to tolerate the lack of annotations functionality rather than attempt the operations necessary to fix the mappings of the existing index. The best way to solve the problem is to create a new annotations index with the correct mappings, and use this for all annotations created from that moment on. This PR implements that solution. The annotations read alias will span the old and new indices in the case where both exist. The annotations write index is adjusted to point only at the new index. Fixes elastic#78439
vistaarjuneja · Oct 14, 2021 · 0af4fe2 · 0af4fe2
1 parent f6ccdbd
commit 0af4fe2
Show file tree

Hide file tree

Showing 8 changed files with 104 additions and 31 deletions.
diff --git a/...lugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationIndex.java b/...lugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationIndex.java
@@ -15,6 +15,7 @@
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthRequest;
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
 import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest;
+import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequestBuilder;
 import org.elasticsearch.action.admin.indices.create.CreateIndexRequest;
 import org.elasticsearch.action.admin.indices.create.CreateIndexResponse;
 import org.elasticsearch.action.support.master.AcknowledgedResponse;
@@ -30,6 +31,7 @@
 import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper;
 import org.elasticsearch.xpack.core.template.TemplateUtils;
 
+import java.util.List;
 import java.util.SortedMap;
 
 import static org.elasticsearch.xpack.core.ClientHelper.ML_ORIGIN;
@@ -41,8 +43,15 @@ public class AnnotationIndex {
 
     public static final String READ_ALIAS_NAME = ".ml-annotations-read";
     public static final String WRITE_ALIAS_NAME = ".ml-annotations-write";
-    // Exposed for testing, but always use the aliases in non-test code
-    public static final String INDEX_NAME = ".ml-annotations-6";
+
+    // Exposed for testing, but always use the aliases in non-test code.
+    public static final String LATEST_INDEX_NAME = ".ml-annotations-000001";
+    // Due to historical bugs this index may not have the correct mappings
+    // in some production clusters. Therefore new annotations should be
+    // written to the latest index. If we ever switch to another new annotations
+    // index then this list should be adjusted to include the previous latest
+    // index.
+    public static final List<String> OLD_INDEX_NAMES = List.of(".ml-annotations-6");
 
     private static final String MAPPINGS_VERSION_VARIABLE = "xpack.ml.version";
 
@@ -85,12 +94,18 @@ public static void createAnnotationsIndexIfNecessary(Client client, ClusterState
             finalListener::onFailure);
 
         final ActionListener<Boolean> createAliasListener = ActionListener.wrap(success -> {
-            final IndicesAliasesRequest request =
+            final IndicesAliasesRequestBuilder requestBuilder =
                 client.admin().indices().prepareAliases()
-                    .addAliasAction(IndicesAliasesRequest.AliasActions.add().index(INDEX_NAME).alias(READ_ALIAS_NAME).isHidden(true))
-                    .addAliasAction(IndicesAliasesRequest.AliasActions.add().index(INDEX_NAME).alias(WRITE_ALIAS_NAME).isHidden(true))
-                    .request();
-            executeAsyncWithOrigin(client.threadPool().getThreadContext(), ML_ORIGIN, request,
+                    .addAliasAction(IndicesAliasesRequest.AliasActions.add()
+                        .index(LATEST_INDEX_NAME).alias(READ_ALIAS_NAME).isHidden(true))
+                    .addAliasAction(IndicesAliasesRequest.AliasActions.add()
+                        .index(LATEST_INDEX_NAME).alias(WRITE_ALIAS_NAME).isHidden(true));
+            for (String oldIndexName : OLD_INDEX_NAMES) {
+                if (state.getMetadata().getIndicesLookup().containsKey(oldIndexName)) {
+                    requestBuilder.removeAlias(oldIndexName, WRITE_ALIAS_NAME);
+                }
+            }
+            executeAsyncWithOrigin(client.threadPool().getThreadContext(), ML_ORIGIN, requestBuilder.request(),
                 ActionListener.<AcknowledgedResponse>wrap(
                     r -> checkMappingsListener.onResponse(r.isAcknowledged()), finalListener::onFailure),
                 client.admin().indices()::aliases);
@@ -104,18 +119,18 @@ public static void createAnnotationsIndexIfNecessary(Client client, ClusterState
             mlLookup.isEmpty() == false && mlLookup.firstKey().startsWith(".ml")) {
 
             // Create the annotations index if it doesn't exist already.
-            if (mlLookup.containsKey(INDEX_NAME) == false) {
+            if (mlLookup.containsKey(LATEST_INDEX_NAME) == false) {
                 logger.debug(
                     () -> new ParameterizedMessage(
                         "Creating [{}] because [{}] exists; trace {}",
-                        INDEX_NAME,
+                        LATEST_INDEX_NAME,
                         mlLookup.firstKey(),
                         org.elasticsearch.ExceptionsHelper.formatStackTrace(Thread.currentThread().getStackTrace())
                     )
                 );
 
                 CreateIndexRequest createIndexRequest =
-                    new CreateIndexRequest(INDEX_NAME)
+                    new CreateIndexRequest(LATEST_INDEX_NAME)
                         .mapping(annotationsMapping())
                         .settings(Settings.builder()
                             .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1")
@@ -140,7 +155,14 @@ public static void createAnnotationsIndexIfNecessary(Client client, ClusterState
             }
 
             // Recreate the aliases if they've gone even though the index still exists.
-            if (mlLookup.containsKey(READ_ALIAS_NAME) == false || mlLookup.containsKey(WRITE_ALIAS_NAME) == false) {
+            IndexAbstraction writeAliasDefinition = mlLookup.get(WRITE_ALIAS_NAME);
+            if (mlLookup.containsKey(READ_ALIAS_NAME) == false || writeAliasDefinition == null) {
+                createAliasListener.onResponse(true);
+                return;
+            }
+
+            List<IndexMetadata> writeAliasMetadata = writeAliasDefinition.getIndices();
+            if (writeAliasMetadata.size() != 1 || LATEST_INDEX_NAME.equals(writeAliasMetadata.get(0).getIndex().getName()) == false) {
                 createAliasListener.onResponse(true);
                 return;
             }
@@ -154,7 +176,7 @@ public static void createAnnotationsIndexIfNecessary(Client client, ClusterState
         finalListener.onResponse(false);
     }
 
-    private static String annotationsMapping() {
+    public static String annotationsMapping() {
         return TemplateUtils.loadTemplate(
             "/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json", Version.CURRENT.toString(), MAPPINGS_VERSION_VARIABLE);
     }

diff --git a/...n/core/src/main/resources/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json b/...n/core/src/main/resources/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json
@@ -3,6 +3,7 @@
     "_meta" : {
       "version" : "${xpack.ml.version}"
     },
+    "dynamic" : false,
     "properties" : {
       "annotation" : {
         "type" : "text"

diff --git a/.../test/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStoreTests.java b/.../test/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStoreTests.java
@@ -1479,7 +1479,7 @@ public void testMachineLearningAdminRole() {
         assertOnlyReadAllowed(role, AnomalyDetectorsIndexFields.STATE_INDEX_PREFIX);
         assertOnlyReadAllowed(role, AnomalyDetectorsIndexFields.RESULTS_INDEX_PREFIX + AnomalyDetectorsIndexFields.RESULTS_INDEX_DEFAULT);
         assertOnlyReadAllowed(role, NotificationsIndex.NOTIFICATIONS_INDEX);
-        assertReadWriteDocsButNotDeleteIndexAllowed(role, AnnotationIndex.INDEX_NAME);
+        assertReadWriteDocsButNotDeleteIndexAllowed(role, AnnotationIndex.LATEST_INDEX_NAME);
 
         assertNoAccessAllowed(role, RestrictedIndicesNames.RESTRICTED_NAMES);
         assertNoAccessAllowed(role, XPackPlugin.ASYNC_RESULTS_INDEX + randomAlphaOfLengthBetween(0, 2));
@@ -1633,7 +1633,7 @@ public void testMachineLearningUserRole() {
         assertNoAccessAllowed(role, AnomalyDetectorsIndexFields.STATE_INDEX_PREFIX);
         assertOnlyReadAllowed(role, AnomalyDetectorsIndexFields.RESULTS_INDEX_PREFIX + AnomalyDetectorsIndexFields.RESULTS_INDEX_DEFAULT);
         assertOnlyReadAllowed(role, NotificationsIndex.NOTIFICATIONS_INDEX);
-        assertReadWriteDocsButNotDeleteIndexAllowed(role, AnnotationIndex.INDEX_NAME);
+        assertReadWriteDocsButNotDeleteIndexAllowed(role, AnnotationIndex.LATEST_INDEX_NAME);
 
         assertNoAccessAllowed(role, RestrictedIndicesNames.RESTRICTED_NAMES);
         assertNoAccessAllowed(role, XPackPlugin.ASYNC_RESULTS_INDEX + randomAlphaOfLengthBetween(0, 2));

diff --git a/...RestTest/java/org/elasticsearch/xpack/ml/integration/MlNativeAutodetectIntegTestCase.java b/...RestTest/java/org/elasticsearch/xpack/ml/integration/MlNativeAutodetectIntegTestCase.java
@@ -264,7 +264,7 @@ protected void waitForecastStatus(int maxWaitTimeSeconds,
 
     protected void assertThatNumberOfAnnotationsIsEqualTo(int expectedNumberOfAnnotations) throws IOException {
         // Refresh the annotations index so that recently indexed annotation docs are visible.
-        client().admin().indices().prepareRefresh(AnnotationIndex.INDEX_NAME)
+        client().admin().indices().prepareRefresh(AnnotationIndex.LATEST_INDEX_NAME)
             .setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN_CLOSED_HIDDEN)
             .execute()
             .actionGet();

diff --git a/...rc/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AnnotationIndexIT.java b/...rc/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AnnotationIndexIT.java
@@ -7,13 +7,18 @@
 package org.elasticsearch.xpack.ml.integration;
 
 import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
+
+import org.elasticsearch.action.admin.indices.alias.Alias;
+import org.elasticsearch.action.admin.indices.create.CreateIndexAction;
+import org.elasticsearch.action.admin.indices.create.CreateIndexRequest;
 import org.elasticsearch.action.index.IndexRequest;
 import org.elasticsearch.action.index.IndexResponse;
 import org.elasticsearch.action.search.SearchPhaseExecutionException;
 import org.elasticsearch.action.search.SearchRequest;
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.cluster.metadata.AliasMetadata;
+import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.collect.ImmutableOpenMap;
 import org.elasticsearch.common.settings.Settings;
@@ -26,11 +31,12 @@
 import org.elasticsearch.xpack.core.ml.annotations.AnnotationIndex;
 import org.elasticsearch.xpack.ml.MlSingleNodeTestCase;
 import org.elasticsearch.xpack.ml.notifications.AnomalyDetectionAuditor;
-import org.junit.Before;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
+import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.is;
 
 public class AnnotationIndexIT extends MlSingleNodeTestCase {
@@ -44,11 +50,6 @@ protected Settings nodeSettings()  {
         return newSettings.build();
     }
 
-    @Before
-    public void createComponents() throws Exception {
-        waitForMlTemplates();
-    }
-
     public void testNotCreatedWhenNoOtherMlIndices() {
 
         // Ask a few times to increase the chance of failure if the .ml-annotations index is created when no other ML index exists
@@ -71,6 +72,52 @@ public void testCreatedWhenAfterOtherMlIndex() throws Exception {
         });
     }
 
+    public void testAliasesMovedFromOldToNew() throws Exception {
+
+        // Create an old annotations index with both read and write aliases pointing at it.
+        String oldIndex = randomFrom(AnnotationIndex.OLD_INDEX_NAMES);
+        CreateIndexRequest createIndexRequest =
+            new CreateIndexRequest(oldIndex)
+                .mapping(AnnotationIndex.annotationsMapping())
+                .settings(Settings.builder()
+                    .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1")
+                    .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, "1")
+                    .put(IndexMetadata.SETTING_INDEX_HIDDEN, true))
+                .alias(new Alias(AnnotationIndex.READ_ALIAS_NAME).isHidden(true))
+                .alias(new Alias(AnnotationIndex.WRITE_ALIAS_NAME).isHidden(true));
+        client().execute(CreateIndexAction.INSTANCE, createIndexRequest).actionGet();
+
+        // Because the old annotations index name began with .ml, it will trigger the new annotations index to be created.
+        // When this happens the read alias should be changed to cover both indices, and the write alias should be
+        // switched to only point at the new index.
+        assertBusy(() -> {
+            assertTrue(annotationsIndexExists());
+            ImmutableOpenMap<String, List<AliasMetadata>> aliases = client().admin().indices()
+                .prepareGetAliases(AnnotationIndex.READ_ALIAS_NAME, AnnotationIndex.WRITE_ALIAS_NAME)
+                .setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN_CLOSED_HIDDEN)
+                .get()
+                .getAliases();
+            assertNotNull(aliases);
+            List<String> indicesWithReadAlias = new ArrayList<>();
+            for (ObjectObjectCursor<String, List<AliasMetadata>> entry : aliases) {
+                for (AliasMetadata aliasMetadata : entry.value) {
+                    switch (aliasMetadata.getAlias()) {
+                        case AnnotationIndex.WRITE_ALIAS_NAME:
+                            assertThat(entry.key, is(AnnotationIndex.LATEST_INDEX_NAME));
+                            break;
+                        case AnnotationIndex.READ_ALIAS_NAME:
+                            indicesWithReadAlias.add(entry.key);
+                            break;
+                        default:
+                            fail("Found unexpected alias " + aliasMetadata.getAlias() + " on index " + entry.key);
+                            break;
+                    }
+                }
+            }
+            assertThat(indicesWithReadAlias, containsInAnyOrder(oldIndex, AnnotationIndex.LATEST_INDEX_NAME));
+        });
+    }
+
     public void testNotCreatedWhenAfterOtherMlIndexAndUpgradeInProgress() throws Exception {
 
         client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(true)).actionGet();
@@ -123,7 +170,7 @@ public void testNotCreatedWhenAfterOtherMlIndexAndResetInProgress() throws Excep
     }
 
     private boolean annotationsIndexExists() {
-        return ESIntegTestCase.indexExists(AnnotationIndex.INDEX_NAME, client());
+        return ESIntegTestCase.indexExists(AnnotationIndex.LATEST_INDEX_NAME, client());
     }
 
     private int numberOfAnnotationsAliases() {

diff --git a/...lClusterTest/java/org/elasticsearch/xpack/ml/integration/AutodetectResultProcessorIT.java b/...lClusterTest/java/org/elasticsearch/xpack/ml/integration/AutodetectResultProcessorIT.java
@@ -731,7 +731,7 @@ private QueryPage<ModelSnapshot> getModelSnapshots() throws Exception {
 
     private List<Annotation> getAnnotations() throws Exception {
         // Refresh the annotations index so that recently indexed annotation docs are visible.
-        client().admin().indices().prepareRefresh(AnnotationIndex.INDEX_NAME)
+        client().admin().indices().prepareRefresh(AnnotationIndex.LATEST_INDEX_NAME)
             .setIndicesOptions(IndicesOptions.STRICT_EXPAND_OPEN_HIDDEN_FORBID_CLOSED)
             .execute()
             .actionGet();

diff --git a/...java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcessManagerTests.java b/...java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcessManagerTests.java
@@ -92,6 +92,7 @@
 import java.util.function.Consumer;
 
 import static org.elasticsearch.action.support.master.MasterNodeRequest.DEFAULT_MASTER_NODE_TIMEOUT;
+import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_INDEX_HIDDEN;
 import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS;
 import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS;
 import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_VERSION_CREATED;
@@ -184,21 +185,23 @@ public void setup() throws Exception {
                             Settings.builder()
                                 .put(SETTING_NUMBER_OF_SHARDS, 1)
                                 .put(SETTING_NUMBER_OF_REPLICAS, 0)
+                                .put(SETTING_INDEX_HIDDEN, true)
                                 .put(SETTING_VERSION_CREATED, Version.CURRENT)
                                 .build())
-                        .putAlias(AliasMetadata.builder(AnomalyDetectorsIndex.jobStateIndexWriteAlias()).build())
+                        .putAlias(AliasMetadata.builder(AnomalyDetectorsIndex.jobStateIndexWriteAlias()).isHidden(true).build())
                         .build())
                 .fPut(
-                    AnnotationIndex.INDEX_NAME,
-                    IndexMetadata.builder(AnnotationIndex.INDEX_NAME)
+                    AnnotationIndex.LATEST_INDEX_NAME,
+                    IndexMetadata.builder(AnnotationIndex.LATEST_INDEX_NAME)
                         .settings(
                             Settings.builder()
                                 .put(SETTING_NUMBER_OF_SHARDS, 1)
                                 .put(SETTING_NUMBER_OF_REPLICAS, 0)
+                                .put(SETTING_INDEX_HIDDEN, true)
                                 .put(SETTING_VERSION_CREATED, Version.CURRENT)
                                 .build())
-                        .putAlias(AliasMetadata.builder(AnnotationIndex.READ_ALIAS_NAME).build())
-                        .putAlias(AliasMetadata.builder(AnnotationIndex.WRITE_ALIAS_NAME).build())
+                        .putAlias(AliasMetadata.builder(AnnotationIndex.READ_ALIAS_NAME).isHidden(true).build())
+                        .putAlias(AliasMetadata.builder(AnnotationIndex.WRITE_ALIAS_NAME).isHidden(true).build())
                         .build())
                 .build())
             .build();

diff --git a/...olling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml b/...olling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml
@@ -157,6 +157,6 @@ setup:
       indices.get_mapping:
         index: .ml-annotations-write
 
-  - match: { \.ml-annotations-6.mappings.properties.type.type: "keyword" }
-  - match: { \.ml-annotations-6.mappings.properties.event.type: "keyword" }
-  - match: { \.ml-annotations-6.mappings.properties.detector_index.type: "integer" }
+  - match: { \.ml-annotations-000001.mappings.properties.type.type: "keyword" }
+  - match: { \.ml-annotations-000001.mappings.properties.event.type: "keyword" }
+  - match: { \.ml-annotations-000001.mappings.properties.detector_index.type: "integer" }