Skip to content

Commit

Permalink
[7.17] [ML] Fix annotations index maintenance after reindexing (#82312)
Browse files Browse the repository at this point in the history 
The code that manages the ML annotations index was not taking
into account the possibility that the usual index name would
be an alias pointing to a reindexed copy of the index. This is
exactly what happens when the upgrade assistant reindexes
indices into the latest Lucene format.

Backport of #82304
  • Loading branch information
@droberts195
droberts195 committed Jan 6, 2022
1 parent aa59ba0 commit eb453f2
Show file tree
Hide file tree
Showing 2 changed files with 149 additions and 21 deletions.
32 changes: 23 additions & 9 deletions ...lugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationIndex.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.index.Index;
import org.elasticsearch.xcontent.XContentType;
import org.elasticsearch.xpack.core.ml.MlMetadata;
import org.elasticsearch.xpack.core.ml.job.persistence.ElasticsearchMappings;
import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper;
import org.elasticsearch.xpack.core.template.TemplateUtils;

import java.util.Collections;
import java.util.List;
import java.util.SortedMap;

import static org.elasticsearch.index.mapper.MapperService.SINGLE_MAPPING_NAME;
Expand Down Expand Up @@ -99,12 +101,12 @@ public static void createAnnotationsIndexIfNecessary(
finalListener::onFailure
);

final ActionListener<Boolean> createAliasListener = ActionListener.wrap(success -> {
final ActionListener<String> createAliasListener = ActionListener.wrap(currentIndexName -> {
IndicesAliasesRequest.AliasActions addReadAliasAction = IndicesAliasesRequest.AliasActions.add()
.index(INDEX_NAME)
.index(currentIndexName)
.alias(READ_ALIAS_NAME);
IndicesAliasesRequest.AliasActions addWriteAliasAction = IndicesAliasesRequest.AliasActions.add()
.index(INDEX_NAME)
.index(currentIndexName)
.alias(WRITE_ALIAS_NAME);
if (isHiddenAttributeAvailable) {
addReadAliasAction.isHidden(true);
Expand Down Expand Up @@ -138,7 +140,8 @@ public static void createAnnotationsIndexIfNecessary(
&& mlLookup.firstKey().startsWith(".ml")) {

// Create the annotations index if it doesn't exist already.
if (mlLookup.containsKey(INDEX_NAME) == false) {
IndexAbstraction currentIndexAbstraction = mlLookup.get(INDEX_NAME);
if (currentIndexAbstraction == null) {

Settings.Builder settingsBuilder = Settings.builder()
.put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1")
Expand All @@ -157,12 +160,12 @@ public static void createAnnotationsIndexIfNecessary(
client.threadPool().getThreadContext(),
ML_ORIGIN,
createIndexRequest,
ActionListener.<CreateIndexResponse>wrap(r -> createAliasListener.onResponse(r.isAcknowledged()), e -> {
ActionListener.<CreateIndexResponse>wrap(r -> createAliasListener.onResponse(INDEX_NAME), e -> {
// Possible that the index was created while the request was executing,
// so we need to handle that possibility
if (ExceptionsHelper.unwrapCause(e) instanceof ResourceAlreadyExistsException) {
// Create the alias
createAliasListener.onResponse(true);
createAliasListener.onResponse(INDEX_NAME);
} else {
finalListener.onFailure(e);
}
Expand All @@ -172,9 +175,20 @@ public static void createAnnotationsIndexIfNecessary(
return;
}

// Account for the possibility that the latest index has been reindexed
// into a new index with the latest index name as an alias.
String currentIndexName = currentIndexAbstraction.getIndices().get(0).getName();

// 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) {
createAliasListener.onResponse(true);
IndexAbstraction writeAliasAbstraction = mlLookup.get(WRITE_ALIAS_NAME);
if (mlLookup.containsKey(READ_ALIAS_NAME) == false || writeAliasAbstraction == null) {
createAliasListener.onResponse(currentIndexName);
return;
}

List<Index> writeAliasIndices = writeAliasAbstraction.getIndices();
if (writeAliasIndices.size() != 1 || currentIndexName.equals(writeAliasIndices.get(0).getName()) == false) {
createAliasListener.onResponse(currentIndexName);
return;
}

Expand All @@ -187,7 +201,7 @@ public static void createAnnotationsIndexIfNecessary(
finalListener.onResponse(false);
}

private static String annotationsMapping() {
public static String annotationsMapping() {
return annotationsMapping(SINGLE_MAPPING_NAME);
}

Expand Down
138 changes: 126 additions & 12 deletions ...rc/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AnnotationIndexIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,23 @@

import com.carrotsearch.hppc.cursors.ObjectObjectCursor;

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.get.GetIndexResponse;
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;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xcontent.XContentType;
import org.elasticsearch.xpack.core.XPackSettings;
import org.elasticsearch.xpack.core.action.SetResetModeActionRequest;
import org.elasticsearch.xpack.core.ml.action.SetResetModeAction;
Expand All @@ -28,9 +34,11 @@
import org.elasticsearch.xpack.ml.notifications.AnomalyDetectionAuditor;
import org.junit.Before;

import java.util.Arrays;
import java.util.Collections;
import java.util.List;

import static org.elasticsearch.index.mapper.MapperService.SINGLE_MAPPING_NAME;
import static org.hamcrest.Matchers.is;

public class AnnotationIndexIT extends MlSingleNodeTestCase {
Expand All @@ -53,34 +61,109 @@ 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
for (int i = 0; i < 10; ++i) {
assertFalse(annotationsIndexExists());
assertFalse(annotationsIndexExists(AnnotationIndex.INDEX_NAME));
assertEquals(0, numberOfAnnotationsAliases());
}
}

public void testCreatedWhenAfterOtherMlIndex() throws Exception {
AnomalyDetectionAuditor auditor = new AnomalyDetectionAuditor(client(), getInstanceFromNode(ClusterService.class));
auditor.info("whatever", "blah");
// Creating a document in the .ml-notifications-000002 index should cause .ml-annotations
// to be created, as it should get created as soon as any other ML index exists
createAnnotation();

assertBusy(() -> {
assertTrue(annotationsIndexExists(AnnotationIndex.INDEX_NAME));
assertEquals(2, numberOfAnnotationsAliases());
});
}

public void testReindexing() throws Exception {
// Creating a document in the .ml-notifications-000002 index should cause .ml-annotations
// to be created, as it should get created as soon as any other ML index exists
createAnnotation();

assertBusy(() -> {
assertTrue(annotationsIndexExists(AnnotationIndex.INDEX_NAME));
assertEquals(2, numberOfAnnotationsAliases());
});

client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(true)).actionGet();

String reindexedIndexName = ".reindexed-v7-ml-annotations-6";
createReindexedIndex(reindexedIndexName);

IndicesAliasesRequestBuilder indicesAliasesRequestBuilder = client().admin()
.indices()
.prepareAliases()
.addAliasAction(
IndicesAliasesRequest.AliasActions.add().index(reindexedIndexName).alias(AnnotationIndex.READ_ALIAS_NAME).isHidden(true)
)
.addAliasAction(
IndicesAliasesRequest.AliasActions.add().index(reindexedIndexName).alias(AnnotationIndex.WRITE_ALIAS_NAME).isHidden(true)
)
.addAliasAction(IndicesAliasesRequest.AliasActions.removeIndex().index(AnnotationIndex.INDEX_NAME))
.addAliasAction(
IndicesAliasesRequest.AliasActions.add().index(reindexedIndexName).alias(AnnotationIndex.INDEX_NAME).isHidden(true)
);

client().admin().indices().aliases(indicesAliasesRequestBuilder.request()).actionGet();

client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(false)).actionGet();

// Ask a few times to increase the chance of failure if the .ml-annotations index is created when no other ML index exists
for (int i = 0; i < 10; ++i) {
assertFalse(annotationsIndexExists(AnnotationIndex.INDEX_NAME));
assertTrue(annotationsIndexExists(reindexedIndexName));
// Aliases should be read, write and original name
assertEquals(3, numberOfAnnotationsAliases());
}
}

public void testReindexingWithLostAliases() throws Exception {
// Creating a document in the .ml-notifications-000002 index should cause .ml-annotations
// to be created, as it should get created as soon as any other ML index exists
createAnnotation();

assertBusy(() -> {
assertTrue(annotationsIndexExists());
assertTrue(annotationsIndexExists(AnnotationIndex.INDEX_NAME));
assertEquals(2, numberOfAnnotationsAliases());
});

client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(true)).actionGet();

String reindexedIndexName = ".reindexed-v7-ml-annotations-6";
createReindexedIndex(reindexedIndexName);

IndicesAliasesRequestBuilder indicesAliasesRequestBuilder = client().admin()
.indices()
.prepareAliases()
// The difference compared to the standard reindexing test is that the read and write aliases are not correctly set up.
// The annotations index maintenance code should add them back.
.addAliasAction(IndicesAliasesRequest.AliasActions.removeIndex().index(AnnotationIndex.INDEX_NAME))
.addAliasAction(
IndicesAliasesRequest.AliasActions.add().index(reindexedIndexName).alias(AnnotationIndex.INDEX_NAME).isHidden(true)
);

client().admin().indices().aliases(indicesAliasesRequestBuilder.request()).actionGet();

client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(false)).actionGet();

assertBusy(() -> {
assertFalse(annotationsIndexExists(AnnotationIndex.INDEX_NAME));
assertTrue(annotationsIndexExists(reindexedIndexName));
// Aliases should be read, write and original name
assertEquals(3, numberOfAnnotationsAliases());
});
}

public void testNotCreatedWhenAfterOtherMlIndexAndUpgradeInProgress() throws Exception {

client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(true)).actionGet();

try {
AnomalyDetectionAuditor auditor = new AnomalyDetectionAuditor(client(), getInstanceFromNode(ClusterService.class));
auditor.info("whatever", "blah");

// Creating a document in the .ml-notifications-000002 index would normally cause .ml-annotations
// to be created, but in this case it shouldn't as we're doing an upgrade
createAnnotation();

assertBusy(() -> {
try {
Expand All @@ -89,7 +172,7 @@ public void testNotCreatedWhenAfterOtherMlIndexAndUpgradeInProgress() throws Exc
} catch (SearchPhaseExecutionException e) {
throw new AssertionError("Notifications index exists but shards not yet ready - continuing busy wait", e);
}
assertFalse(annotationsIndexExists());
assertFalse(annotationsIndexExists(AnnotationIndex.INDEX_NAME));
assertEquals(0, numberOfAnnotationsAliases());
});
} finally {
Expand All @@ -114,23 +197,30 @@ public void testNotCreatedWhenAfterOtherMlIndexAndResetInProgress() throws Excep
assertBusy(() -> {
SearchResponse response = client().search(new SearchRequest(".ml-state")).actionGet();
assertEquals(1, response.getHits().getHits().length);
assertFalse(annotationsIndexExists());
assertFalse(annotationsIndexExists(AnnotationIndex.INDEX_NAME));
assertEquals(0, numberOfAnnotationsAliases());
});
} finally {
client().execute(SetResetModeAction.INSTANCE, SetResetModeActionRequest.disabled(true)).actionGet();
}
}

private boolean annotationsIndexExists() {
return client().admin().indices().prepareExists(AnnotationIndex.INDEX_NAME).get().isExists();
private boolean annotationsIndexExists(String expectedName) {
GetIndexResponse getIndexResponse = client().admin()
.indices()
.prepareGetIndex()
.setIndices(AnnotationIndex.INDEX_NAME)
.setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN)
.execute()
.actionGet();
return Arrays.asList(getIndexResponse.getIndices()).contains(expectedName);
}

private int numberOfAnnotationsAliases() {
int count = 0;
ImmutableOpenMap<String, List<AliasMetadata>> aliases = client().admin()
.indices()
.prepareGetAliases(AnnotationIndex.READ_ALIAS_NAME, AnnotationIndex.WRITE_ALIAS_NAME)
.prepareGetAliases(AnnotationIndex.READ_ALIAS_NAME, AnnotationIndex.WRITE_ALIAS_NAME, AnnotationIndex.INDEX_NAME)
.setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN_CLOSED_HIDDEN)
.get()
.getAliases();
Expand All @@ -144,4 +234,28 @@ private int numberOfAnnotationsAliases() {
}
return count;
}

private void createReindexedIndex(String reindexedIndexName) {
CreateIndexRequest createIndexRequest = new CreateIndexRequest(reindexedIndexName).mapping(
SINGLE_MAPPING_NAME,
AnnotationIndex.annotationsMapping(),
XContentType.JSON
)
.settings(
Settings.builder()
.put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1")
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, "1")
.put(IndexMetadata.SETTING_INDEX_HIDDEN, true)
);

client().admin().indices().create(createIndexRequest).actionGet();

// At this point the upgrade assistant would reindex the old index into the new index but there's
// no point in this test as there's nothing in the old index.
}

private void createAnnotation() {
AnomalyDetectionAuditor auditor = new AnomalyDetectionAuditor(client(), getInstanceFromNode(ClusterService.class));
auditor.info("whatever", "blah");
}
}

0 comments on commit eb453f2

Please sign in to comment.