Skip to content

Commit 8eba37a

Browse files
droberts195droberts195
committed
[ML] Fix annotations index maintenance after reindexing (elastic#82304)
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. Fixes elastic#82250
1 parent b4b58db commit 8eba37a

File tree

3 files changed

+148
-29
lines changed

3 files changed

+148
-29
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationIndex.java

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -107,15 +107,21 @@ public static void createAnnotationsIndexIfNecessary(
107107
finalListener::onFailure
108108
);
109109

110-
final ActionListener<Boolean> createAliasListener = ActionListener.wrap(success -> {
110+
final ActionListener<String> createAliasListener = ActionListener.wrap(currentIndexName -> {
111111
final IndicesAliasesRequestBuilder requestBuilder = client.admin()
112112
.indices()
113113
.prepareAliases()
114-
.addAliasAction(IndicesAliasesRequest.AliasActions.add().index(LATEST_INDEX_NAME).alias(READ_ALIAS_NAME).isHidden(true))
115-
.addAliasAction(IndicesAliasesRequest.AliasActions.add().index(LATEST_INDEX_NAME).alias(WRITE_ALIAS_NAME).isHidden(true));
114+
.addAliasAction(IndicesAliasesRequest.AliasActions.add().index(currentIndexName).alias(READ_ALIAS_NAME).isHidden(true))
115+
.addAliasAction(IndicesAliasesRequest.AliasActions.add().index(currentIndexName).alias(WRITE_ALIAS_NAME).isHidden(true));
116+
SortedMap<String, IndexAbstraction> lookup = state.getMetadata().getIndicesLookup();
116117
for (String oldIndexName : OLD_INDEX_NAMES) {
117-
if (state.getMetadata().hasIndexAbstraction(oldIndexName)) {
118-
requestBuilder.removeAlias(oldIndexName, WRITE_ALIAS_NAME);
118+
IndexAbstraction oldIndexAbstraction = lookup.get(oldIndexName);
119+
if (oldIndexAbstraction != null) {
120+
// The old index might no longer be an index - that index could have been reindexed
121+
// with the old index name now being an alias to that reindexed index.
122+
for (Index oldIndex : oldIndexAbstraction.getIndices()) {
123+
requestBuilder.removeAlias(oldIndex.getName(), WRITE_ALIAS_NAME);
124+
}
119125
}
120126
}
121127
executeAsyncWithOrigin(
@@ -140,7 +146,8 @@ public static void createAnnotationsIndexIfNecessary(
140146
&& mlLookup.firstKey().startsWith(".ml")) {
141147

142148
// Create the annotations index if it doesn't exist already.
143-
if (mlLookup.containsKey(LATEST_INDEX_NAME) == false) {
149+
IndexAbstraction currentIndexAbstraction = mlLookup.get(LATEST_INDEX_NAME);
150+
if (currentIndexAbstraction == null) {
144151
logger.debug(
145152
() -> new ParameterizedMessage(
146153
"Creating [{}] because [{}] exists; trace {}",
@@ -162,12 +169,12 @@ public static void createAnnotationsIndexIfNecessary(
162169
client.threadPool().getThreadContext(),
163170
ML_ORIGIN,
164171
createIndexRequest,
165-
ActionListener.<CreateIndexResponse>wrap(r -> createAliasListener.onResponse(r.isAcknowledged()), e -> {
172+
ActionListener.<CreateIndexResponse>wrap(r -> createAliasListener.onResponse(LATEST_INDEX_NAME), e -> {
166173
// Possible that the index was created while the request was executing,
167174
// so we need to handle that possibility
168175
if (ExceptionsHelper.unwrapCause(e) instanceof ResourceAlreadyExistsException) {
169176
// Create the alias
170-
createAliasListener.onResponse(true);
177+
createAliasListener.onResponse(LATEST_INDEX_NAME);
171178
} else {
172179
finalListener.onFailure(e);
173180
}
@@ -177,16 +184,20 @@ public static void createAnnotationsIndexIfNecessary(
177184
return;
178185
}
179186

187+
// Account for the possibility that the latest index has been reindexed
188+
// into a new index with the latest index name as an alias.
189+
String currentIndexName = currentIndexAbstraction.getIndices().get(0).getName();
190+
180191
// Recreate the aliases if they've gone even though the index still exists.
181-
IndexAbstraction writeAliasDefinition = mlLookup.get(WRITE_ALIAS_NAME);
182-
if (mlLookup.containsKey(READ_ALIAS_NAME) == false || writeAliasDefinition == null) {
183-
createAliasListener.onResponse(true);
192+
IndexAbstraction writeAliasAbstraction = mlLookup.get(WRITE_ALIAS_NAME);
193+
if (mlLookup.containsKey(READ_ALIAS_NAME) == false || writeAliasAbstraction == null) {
194+
createAliasListener.onResponse(currentIndexName);
184195
return;
185196
}
186197

187-
List<Index> writeAliasIndices = writeAliasDefinition.getIndices();
188-
if (writeAliasIndices.size() != 1 || LATEST_INDEX_NAME.equals(writeAliasIndices.get(0).getName()) == false) {
189-
createAliasListener.onResponse(true);
198+
List<Index> writeAliasIndices = writeAliasAbstraction.getIndices();
199+
if (writeAliasIndices.size() != 1 || currentIndexName.equals(writeAliasIndices.get(0).getName()) == false) {
200+
createAliasListener.onResponse(currentIndexName);
190201
return;
191202
}
192203

‎x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AnnotationIndexIT.java

Lines changed: 119 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,11 @@
99
import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
1010

1111
import org.elasticsearch.action.admin.indices.alias.Alias;
12+
import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest;
13+
import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequestBuilder;
1214
import org.elasticsearch.action.admin.indices.create.CreateIndexAction;
1315
import org.elasticsearch.action.admin.indices.create.CreateIndexRequest;
16+
import org.elasticsearch.action.admin.indices.get.GetIndexResponse;
1417
import org.elasticsearch.action.index.IndexRequest;
1518
import org.elasticsearch.action.index.IndexResponse;
1619
import org.elasticsearch.action.search.SearchPhaseExecutionException;
@@ -23,7 +26,6 @@
2326
import org.elasticsearch.common.collect.ImmutableOpenMap;
2427
import org.elasticsearch.common.settings.Settings;
2528
import org.elasticsearch.rest.RestStatus;
26-
import org.elasticsearch.test.ESIntegTestCase;
2729
import org.elasticsearch.xpack.core.XPackSettings;
2830
import org.elasticsearch.xpack.core.action.SetResetModeActionRequest;
2931
import org.elasticsearch.xpack.core.ml.action.SetResetModeAction;
@@ -33,6 +35,7 @@
3335
import org.elasticsearch.xpack.ml.notifications.AnomalyDetectionAuditor;
3436

3537
import java.util.ArrayList;
38+
import java.util.Arrays;
3639
import java.util.Collections;
3740
import java.util.List;
3841

@@ -54,22 +57,99 @@ public void testNotCreatedWhenNoOtherMlIndices() {
5457

5558
// Ask a few times to increase the chance of failure if the .ml-annotations index is created when no other ML index exists
5659
for (int i = 0; i < 10; ++i) {
57-
assertFalse(annotationsIndexExists());
60+
assertFalse(annotationsIndexExists(AnnotationIndex.LATEST_INDEX_NAME));
5861
assertEquals(0, numberOfAnnotationsAliases());
5962
}
6063
}
6164

6265
public void testCreatedWhenAfterOtherMlIndex() throws Exception {
63-
AnomalyDetectionAuditor auditor = new AnomalyDetectionAuditor(client(), getInstanceFromNode(ClusterService.class));
64-
auditor.info("whatever", "blah");
66+
// Creating a document in the .ml-notifications-000002 index should cause .ml-annotations
67+
// to be created, as it should get created as soon as any other ML index exists
68+
createAnnotation();
69+
70+
assertBusy(() -> {
71+
assertTrue(annotationsIndexExists(AnnotationIndex.LATEST_INDEX_NAME));
72+
assertEquals(2, numberOfAnnotationsAliases());
73+
});
74+
}
6575

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

6981
assertBusy(() -> {
70-
assertTrue(annotationsIndexExists());
82+
assertTrue(annotationsIndexExists(AnnotationIndex.LATEST_INDEX_NAME));
7183
assertEquals(2, numberOfAnnotationsAliases());
7284
});
85+
86+
client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(true)).actionGet();
87+
88+
String reindexedIndexName = ".reindexed-v7-ml-annotations-6";
89+
createReindexedIndex(reindexedIndexName);
90+
91+
IndicesAliasesRequestBuilder indicesAliasesRequestBuilder = client().admin()
92+
.indices()
93+
.prepareAliases()
94+
.addAliasAction(
95+
IndicesAliasesRequest.AliasActions.add().index(reindexedIndexName).alias(AnnotationIndex.READ_ALIAS_NAME).isHidden(true)
96+
)
97+
.addAliasAction(
98+
IndicesAliasesRequest.AliasActions.add().index(reindexedIndexName).alias(AnnotationIndex.WRITE_ALIAS_NAME).isHidden(true)
99+
)
100+
.addAliasAction(IndicesAliasesRequest.AliasActions.removeIndex().index(AnnotationIndex.LATEST_INDEX_NAME))
101+
.addAliasAction(
102+
IndicesAliasesRequest.AliasActions.add().index(reindexedIndexName).alias(AnnotationIndex.LATEST_INDEX_NAME).isHidden(true)
103+
);
104+
105+
client().admin().indices().aliases(indicesAliasesRequestBuilder.request()).actionGet();
106+
107+
client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(false)).actionGet();
108+
109+
// Ask a few times to increase the chance of failure if the .ml-annotations index is created when no other ML index exists
110+
for (int i = 0; i < 10; ++i) {
111+
assertFalse(annotationsIndexExists(AnnotationIndex.LATEST_INDEX_NAME));
112+
assertTrue(annotationsIndexExists(reindexedIndexName));
113+
// Aliases should be read, write and original name
114+
assertEquals(3, numberOfAnnotationsAliases());
115+
}
116+
}
117+
118+
public void testReindexingWithLostAliases() throws Exception {
119+
// Creating a document in the .ml-notifications-000002 index should cause .ml-annotations
120+
// to be created, as it should get created as soon as any other ML index exists
121+
createAnnotation();
122+
123+
assertBusy(() -> {
124+
assertTrue(annotationsIndexExists(AnnotationIndex.LATEST_INDEX_NAME));
125+
assertEquals(2, numberOfAnnotationsAliases());
126+
});
127+
128+
client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(true)).actionGet();
129+
130+
String reindexedIndexName = ".reindexed-v7-ml-annotations-6";
131+
createReindexedIndex(reindexedIndexName);
132+
133+
IndicesAliasesRequestBuilder indicesAliasesRequestBuilder = client().admin()
134+
.indices()
135+
.prepareAliases()
136+
// The difference compared to the standard reindexing test is that the read and write aliases are not correctly set up.
137+
// The annotations index maintenance code should add them back.
138+
.addAliasAction(IndicesAliasesRequest.AliasActions.removeIndex().index(AnnotationIndex.LATEST_INDEX_NAME))
139+
.addAliasAction(
140+
IndicesAliasesRequest.AliasActions.add().index(reindexedIndexName).alias(AnnotationIndex.LATEST_INDEX_NAME).isHidden(true)
141+
);
142+
143+
client().admin().indices().aliases(indicesAliasesRequestBuilder.request()).actionGet();
144+
145+
client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(false)).actionGet();
146+
147+
assertBusy(() -> {
148+
assertFalse(annotationsIndexExists(AnnotationIndex.LATEST_INDEX_NAME));
149+
assertTrue(annotationsIndexExists(reindexedIndexName));
150+
// Aliases should be read, write and original name
151+
assertEquals(3, numberOfAnnotationsAliases());
152+
});
73153
}
74154

75155
public void testAliasesMovedFromOldToNew() throws Exception {
@@ -91,7 +171,7 @@ public void testAliasesMovedFromOldToNew() throws Exception {
91171
// When this happens the read alias should be changed to cover both indices, and the write alias should be
92172
// switched to only point at the new index.
93173
assertBusy(() -> {
94-
assertTrue(annotationsIndexExists());
174+
assertTrue(annotationsIndexExists(AnnotationIndex.LATEST_INDEX_NAME));
95175
ImmutableOpenMap<String, List<AliasMetadata>> aliases = client().admin()
96176
.indices()
97177
.prepareGetAliases(AnnotationIndex.READ_ALIAS_NAME, AnnotationIndex.WRITE_ALIAS_NAME)
@@ -124,11 +204,9 @@ public void testNotCreatedWhenAfterOtherMlIndexAndUpgradeInProgress() throws Exc
124204
client().execute(SetUpgradeModeAction.INSTANCE, new SetUpgradeModeAction.Request(true)).actionGet();
125205

126206
try {
127-
AnomalyDetectionAuditor auditor = new AnomalyDetectionAuditor(client(), getInstanceFromNode(ClusterService.class));
128-
auditor.info("whatever", "blah");
129-
130207
// Creating a document in the .ml-notifications-000002 index would normally cause .ml-annotations
131208
// to be created, but in this case it shouldn't as we're doing an upgrade
209+
createAnnotation();
132210

133211
assertBusy(() -> {
134212
try {
@@ -137,7 +215,7 @@ public void testNotCreatedWhenAfterOtherMlIndexAndUpgradeInProgress() throws Exc
137215
} catch (SearchPhaseExecutionException e) {
138216
throw new AssertionError("Notifications index exists but shards not yet ready - continuing busy wait", e);
139217
}
140-
assertFalse(annotationsIndexExists());
218+
assertFalse(annotationsIndexExists(AnnotationIndex.LATEST_INDEX_NAME));
141219
assertEquals(0, numberOfAnnotationsAliases());
142220
});
143221
} finally {
@@ -162,23 +240,30 @@ public void testNotCreatedWhenAfterOtherMlIndexAndResetInProgress() throws Excep
162240
assertBusy(() -> {
163241
SearchResponse response = client().search(new SearchRequest(".ml-state")).actionGet();
164242
assertEquals(1, response.getHits().getHits().length);
165-
assertFalse(annotationsIndexExists());
243+
assertFalse(annotationsIndexExists(AnnotationIndex.LATEST_INDEX_NAME));
166244
assertEquals(0, numberOfAnnotationsAliases());
167245
});
168246
} finally {
169247
client().execute(SetResetModeAction.INSTANCE, SetResetModeActionRequest.disabled(true)).actionGet();
170248
}
171249
}
172250

173-
private boolean annotationsIndexExists() {
174-
return ESIntegTestCase.indexExists(AnnotationIndex.LATEST_INDEX_NAME, client());
251+
private boolean annotationsIndexExists(String expectedName) {
252+
GetIndexResponse getIndexResponse = client().admin()
253+
.indices()
254+
.prepareGetIndex()
255+
.setIndices(AnnotationIndex.LATEST_INDEX_NAME)
256+
.setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN)
257+
.execute()
258+
.actionGet();
259+
return Arrays.asList(getIndexResponse.getIndices()).contains(expectedName);
175260
}
176261

177262
private int numberOfAnnotationsAliases() {
178263
int count = 0;
179264
ImmutableOpenMap<String, List<AliasMetadata>> aliases = client().admin()
180265
.indices()
181-
.prepareGetAliases(AnnotationIndex.READ_ALIAS_NAME, AnnotationIndex.WRITE_ALIAS_NAME)
266+
.prepareGetAliases(AnnotationIndex.READ_ALIAS_NAME, AnnotationIndex.WRITE_ALIAS_NAME, AnnotationIndex.LATEST_INDEX_NAME)
182267
.setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN_CLOSED_HIDDEN)
183268
.get()
184269
.getAliases();
@@ -192,4 +277,24 @@ private int numberOfAnnotationsAliases() {
192277
}
193278
return count;
194279
}
280+
281+
private void createReindexedIndex(String reindexedIndexName) {
282+
CreateIndexRequest createIndexRequest = new CreateIndexRequest(reindexedIndexName).mapping(AnnotationIndex.annotationsMapping())
283+
.settings(
284+
Settings.builder()
285+
.put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1")
286+
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, "1")
287+
.put(IndexMetadata.SETTING_INDEX_HIDDEN, true)
288+
);
289+
290+
client().admin().indices().create(createIndexRequest).actionGet();
291+
292+
// At this point the upgrade assistant would reindex the old index into the new index but there's
293+
// no point in this test as there's nothing in the old index.
294+
}
295+
296+
private void createAnnotation() {
297+
AnomalyDetectionAuditor auditor = new AnomalyDetectionAuditor(client(), getInstanceFromNode(ClusterService.class));
298+
auditor.info("whatever", "blah");
299+
}
195300
}

‎x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/MlSingleNodeTestCase.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.elasticsearch.common.settings.Settings;
1414
import org.elasticsearch.common.unit.ByteSizeValue;
1515
import org.elasticsearch.core.TimeValue;
16+
import org.elasticsearch.index.mapper.extras.MapperExtrasPlugin;
1617
import org.elasticsearch.ingest.common.IngestCommonPlugin;
1718
import org.elasticsearch.license.LicenseService;
1819
import org.elasticsearch.plugins.Plugin;
@@ -96,7 +97,9 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
9697
IngestCommonPlugin.class,
9798
MockPainlessScriptEngine.TestPlugin.class,
9899
// ILM is required for .ml-state template index settings
99-
IndexLifecycle.class
100+
IndexLifecycle.class,
101+
// Needed for scaled_float
102+
MapperExtrasPlugin.class
100103
);
101104
}
102105

0 commit comments

Comments
 (0)