From fe6054f779a485dc91212d4e1116a582119fd877 Mon Sep 17 00:00:00 2001 From: David Roberts Date: Thu, 14 Oct 2021 13:23:58 +0100 Subject: [PATCH] [ML] Use a new annotations index for future annotations 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. Adds back #79006, which was reverted because Kibana was not ready. --- .../core/ml/annotations/AnnotationIndex.java | 46 ++++++++++---- .../core/ml/annotations_index_mappings.json | 1 + .../authz/store/ReservedRolesStoreTests.java | 4 +- .../MlNativeAutodetectIntegTestCase.java | 2 +- .../ml/integration/AnnotationIndexIT.java | 61 ++++++++++++++++--- .../AutodetectResultProcessorIT.java | 2 +- .../AutodetectProcessManagerTests.java | 13 ++-- .../test/upgraded_cluster/30_ml_jobs_crud.yml | 6 +- 8 files changed, 104 insertions(+), 31 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationIndex.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationIndex.java index 80018208e8c6d..8f9f0eeb56127 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationIndex.java +++ b/x-pack/plugin/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 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 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.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 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/x-pack/plugin/core/src/main/resources/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json b/x-pack/plugin/core/src/main/resources/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json index 17ebf089de809..1292ce06e3044 100644 --- a/x-pack/plugin/core/src/main/resources/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json +++ b/x-pack/plugin/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/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStoreTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStoreTests.java index fc4033eadde74..83dc0e79c0785 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStoreTests.java +++ b/x-pack/plugin/core/src/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/x-pack/plugin/ml/qa/native-multi-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/ml/integration/MlNativeAutodetectIntegTestCase.java b/x-pack/plugin/ml/qa/native-multi-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/ml/integration/MlNativeAutodetectIntegTestCase.java index 73bab18e0458d..88f836d10f730 100644 --- a/x-pack/plugin/ml/qa/native-multi-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/ml/integration/MlNativeAutodetectIntegTestCase.java +++ b/x-pack/plugin/ml/qa/native-multi-node-tests/src/javaRestTest/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/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AnnotationIndexIT.java b/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AnnotationIndexIT.java index 437217d3657bc..6d1ddc1a10d08 100644 --- a/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AnnotationIndexIT.java +++ b/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AnnotationIndexIT.java @@ -7,6 +7,10 @@ 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; @@ -14,6 +18,7 @@ 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> 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 indicesWithReadAlias = new ArrayList<>(); + for (ObjectObjectCursor> 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/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AutodetectResultProcessorIT.java b/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AutodetectResultProcessorIT.java index 4b23426a2c675..be7568aa18499 100644 --- a/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AutodetectResultProcessorIT.java +++ b/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AutodetectResultProcessorIT.java @@ -731,7 +731,7 @@ private QueryPage getModelSnapshots() throws Exception { private List 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/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcessManagerTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcessManagerTests.java index 50ba3baa09e7f..9881563350c81 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectProcessManagerTests.java +++ b/x-pack/plugin/ml/src/test/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/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml index 1e7b848190d92..00121f369bbc0 100644 --- a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml +++ b/x-pack/qa/rolling-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" }