Skip to content

[ML] Fix annotations index maintenance after reindexing #82304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 6, 2022
Merged

[ML] Fix annotations index maintenance after reindexing #82304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 25 additions & 14 deletions ...lugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationIndex.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,21 @@ public static void createAnnotationsIndexIfNecessary(
finalListener::onFailure
);

final ActionListener<Boolean> createAliasListener = ActionListener.wrap(success -> {
final ActionListener<String> createAliasListener = ActionListener.wrap(currentIndexName -> {
final IndicesAliasesRequestBuilder requestBuilder = client.admin()
.indices()
.prepareAliases()
.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));
.addAliasAction(IndicesAliasesRequest.AliasActions.add().index(currentIndexName).alias(READ_ALIAS_NAME).isHidden(true))
.addAliasAction(IndicesAliasesRequest.AliasActions.add().index(currentIndexName).alias(WRITE_ALIAS_NAME).isHidden(true));
SortedMap<String, IndexAbstraction> lookup = state.getMetadata().getIndicesLookup();
for (String oldIndexName : OLD_INDEX_NAMES) {
if (state.getMetadata().hasIndexAbstraction(oldIndexName)) {
requestBuilder.removeAlias(oldIndexName, WRITE_ALIAS_NAME);
IndexAbstraction oldIndexAbstraction = lookup.get(oldIndexName);
if (oldIndexAbstraction != null) {
// The old index might no longer be an index - that index could have been reindexed
// with the old index name now being an alias to that reindexed index.
for (Index oldIndex : oldIndexAbstraction.getIndices()) {
requestBuilder.removeAlias(oldIndex.getName(), WRITE_ALIAS_NAME);
}
}
}
executeAsyncWithOrigin(
Expand All @@ -140,7 +146,8 @@ public static void createAnnotationsIndexIfNecessary(
&& mlLookup.firstKey().startsWith(".ml")) {

// Create the annotations index if it doesn't exist already.
if (mlLookup.containsKey(LATEST_INDEX_NAME) == false) {
IndexAbstraction currentIndexAbstraction = mlLookup.get(LATEST_INDEX_NAME);
if (currentIndexAbstraction == null) {
logger.debug(
() -> new ParameterizedMessage(
"Creating [{}] because [{}] exists; trace {}",
Expand All @@ -162,12 +169,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(LATEST_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(LATEST_INDEX_NAME);
} else {
finalListener.onFailure(e);
}
Expand All @@ -177,16 +184,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.
IndexAbstraction writeAliasDefinition = mlLookup.get(WRITE_ALIAS_NAME);
if (mlLookup.containsKey(READ_ALIAS_NAME) == false || writeAliasDefinition == null) {
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 = writeAliasDefinition.getIndices();
if (writeAliasIndices.size() != 1 || LATEST_INDEX_NAME.equals(writeAliasIndices.get(0).getName()) == false) {
createAliasListener.onResponse(true);
List<Index> writeAliasIndices = writeAliasAbstraction.getIndices();
if (writeAliasIndices.size() != 1 || currentIndexName.equals(writeAliasIndices.get(0).getName()) == false) {
createAliasListener.onResponse(currentIndexName);
return;
}

Expand Down
133 changes: 119 additions & 14 deletions ...rc/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AnnotationIndexIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@
import com.carrotsearch.hppc.cursors.ObjectObjectCursor;

import org.elasticsearch.action.admin.indices.alias.Alias;
import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest;
import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequestBuilder;
import org.elasticsearch.action.admin.indices.create.CreateIndexAction;
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;
Expand All @@ -23,7 +26,6 @@
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.xpack.core.XPackSettings;
import org.elasticsearch.xpack.core.action.SetResetModeActionRequest;
import org.elasticsearch.xpack.core.ml.action.SetResetModeAction;
Expand All @@ -33,6 +35,7 @@
import org.elasticsearch.xpack.ml.notifications.AnomalyDetectionAuditor;

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

Expand All @@ -54,22 +57,99 @@ 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.LATEST_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.LATEST_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());
assertTrue(annotationsIndexExists(AnnotationIndex.LATEST_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.LATEST_INDEX_NAME))
.addAliasAction(
IndicesAliasesRequest.AliasActions.add().index(reindexedIndexName).alias(AnnotationIndex.LATEST_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.LATEST_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(AnnotationIndex.LATEST_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.LATEST_INDEX_NAME))
.addAliasAction(
IndicesAliasesRequest.AliasActions.add().index(reindexedIndexName).alias(AnnotationIndex.LATEST_INDEX_NAME).isHidden(true)
);

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

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

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

public void testAliasesMovedFromOldToNew() throws Exception {
Expand All @@ -91,7 +171,7 @@ public void testAliasesMovedFromOldToNew() throws Exception {
// 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());
assertTrue(annotationsIndexExists(AnnotationIndex.LATEST_INDEX_NAME));
ImmutableOpenMap<String, List<AliasMetadata>> aliases = client().admin()
.indices()
.prepareGetAliases(AnnotationIndex.READ_ALIAS_NAME, AnnotationIndex.WRITE_ALIAS_NAME)
Expand Down Expand Up @@ -124,11 +204,9 @@ public void testNotCreatedWhenAfterOtherMlIndexAndUpgradeInProgress() throws Exc
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 @@ -137,7 +215,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.LATEST_INDEX_NAME));
assertEquals(0, numberOfAnnotationsAliases());
});
} finally {
Expand All @@ -162,23 +240,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.LATEST_INDEX_NAME));
assertEquals(0, numberOfAnnotationsAliases());
});
} finally {
client().execute(SetResetModeAction.INSTANCE, SetResetModeActionRequest.disabled(true)).actionGet();
}
}

private boolean annotationsIndexExists() {
return ESIntegTestCase.indexExists(AnnotationIndex.LATEST_INDEX_NAME, client());
private boolean annotationsIndexExists(String expectedName) {
GetIndexResponse getIndexResponse = client().admin()
.indices()
.prepareGetIndex()
.setIndices(AnnotationIndex.LATEST_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.LATEST_INDEX_NAME)
.setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN_CLOSED_HIDDEN)
.get()
.getAliases();
Expand All @@ -192,4 +277,24 @@ private int numberOfAnnotationsAliases() {
}
return count;
}

private void createReindexedIndex(String reindexedIndexName) {
CreateIndexRequest createIndexRequest = new CreateIndexRequest(reindexedIndexName).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)
);

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");
}
}
5 changes: 4 additions & 1 deletion x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/MlSingleNodeTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.index.mapper.extras.MapperExtrasPlugin;
import org.elasticsearch.ingest.common.IngestCommonPlugin;
import org.elasticsearch.license.LicenseService;
import org.elasticsearch.plugins.Plugin;
Expand Down Expand Up @@ -96,7 +97,9 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
IngestCommonPlugin.class,
MockPainlessScriptEngine.TestPlugin.class,
// ILM is required for .ml-state template index settings
IndexLifecycle.class
IndexLifecycle.class,
// Needed for scaled_float
MapperExtrasPlugin.class
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit just avoids masses of log spam while the tests run - like #82055

);
}

Expand Down