System Index Migration Failure Results in a Non-Recoverable State #122326

JVerwolf · 2025-02-11T22:32:29Z

Jira: ES-10666

In #120566 I wrote a test that uncovered a pre-existing issue whereby the system indices could end up in a non-recoverable state if the migration failed.

Specifically, broken system indices from the previous migration attempts will not be cleaned-up (although there is code to do so) because persistent task state is missing on the subsequent runs. When the system index migration logic tries to make a new index, and an existing broken new index already exists, an exception will occur. This appears to be caused by the task state not being persisted correctly during the original failure.

This PR changes the code to no-longer rely on the persistent task state for the cleanup logic of exisiting indices.

elasticsearchmachine · 2025-02-11T22:32:52Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticsearchmachine · 2025-02-11T22:33:54Z

Hi @JVerwolf, I've created a changelog YAML for you.

JVerwolf · 2025-02-12T15:16:59Z

@elasticsearchmachine update branch

JVerwolf · 2025-02-12T15:25:09Z

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java

-        Consumer<ClusterState> listener
-    ) {
        logger.debug("cleaning up previous migration, task state: [{}]", taskState == null ? "null" : Strings.toString(taskState));
-        if (taskState != null && taskState.getCurrentIndex() != null) {


The taskState is always null, so the cleanup logic is never invoked here.

JVerwolf · 2025-02-12T15:25:48Z

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java

        updateTask.submit(clusterService);
    }

-    private void prepareNextIndex(ClusterState clusterState, Consumer<ClusterState> listener, String lastFeatureName) {


The clusterState was never used, so I removed this extraneous param.

JVerwolf · 2025-02-12T15:29:14Z

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java

-            logger.debug("no incomplete index to remove");
-            clearResults(clusterService, ActionListener.wrap(listener::accept, this::markAsFailed));
-        }
+        clearResults(


The purpose ofclearResults is to remove the Persistent Task State leftover from prior runs so that we begin the migration with fresh start. I'm not sure if we ever even store the persistent task state in practice? The call to super.markAsFailed(e); in markAsFailed ultimately removes the task state, though perhaps this wasn't the original intention.

It may be possible that there are other failure scenarios that leave the teask sate (e.g. a node getting abruptly killed). So I left this herere for now untill I can reasont through this with @gwbrown when she's back.

JVerwolf · 2025-02-12T15:35:29Z

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java

+    private <T> void deleteIndex(SystemIndexMigrationInfo migrationInfo, ActionListener<AcknowledgedResponse> listener) {
+        logger.info("removing index [{}] from feature [{}]", migrationInfo.getNextIndexName(), migrationInfo.getFeatureName());
+        String newIndexName = migrationInfo.getNextIndexName();
+        baseClient.admin().indices().prepareDelete(newIndexName).execute(ActionListener.wrap(ackedResponse -> {


This code is identical to the prior cleanUpPreviousMigration code except for that I'm using a baseClient here since I don't have access to the migrationInfo needed for migrationInfo.createClient(baseClient). This previous call produced an Origin Setting Client. I don't think we need that here, though I'm not 100% on this.

JVerwolf · 2025-02-12T15:36:37Z

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java

+        baseClient.admin().indices().prepareDelete(newIndexName).execute(ActionListener.wrap(ackedResponse -> {
+            if (ackedResponse.isAcknowledged()) {
+                logger.info("successfully removed index [{}]", newIndexName);
+                listener.onResponse(ackedResponse);


This code is identical to the prior cleanUpPreviousMigration code except that I don't call clearResults(clusterService, ActionListener.wrap(listener::accept, this::markAsFailed)); here, as I've already cleaned up at the beginning of the migration, and I don't want to remove in-progress state since this now happens once the migration is already in progress.

alexey-ivanov-es

So many callbacks!

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java

alexey-ivanov-es · 2025-02-12T16:08:15Z

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java

+        createIndex(migrationInfo, ActionListener.wrap(listener::onResponse, e -> {
+            logger.warn("createIndex failed, retrying after removing index [{}] from previous attempt", migrationInfo.getNextIndexName());
+            deleteIndex(migrationInfo, ActionListener.wrap(cleanupResponse -> createIndex(migrationInfo, listener), e2 -> {
+                logger.warn("createIndex failed after retrying, aborting", e2);


I lost in callbacks, but isn't this error consumer for deleteIndex?

You're right, I've fixed this now. Nice catch! Also, welcome to callback hell.

JVerwolf · 2025-02-12T22:19:17Z

@elasticsearchmachine update branch

alexey-ivanov-es

LGTM

alexey-ivanov-es · 2025-02-13T14:50:32Z

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java

+            logger.warn("createIndex failed, retrying after removing index [{}] from previous attempt", migrationInfo.getNextIndexName());
+            deleteIndex(migrationInfo, ActionListener.wrap(cleanupResponse -> createIndex(migrationInfo, l.delegateResponse((l3, e3) -> {
+                logger.error(
+                    "createIndex failed after retrying, aborting; index [{}] will be left in an inconsistent state",


left in an inconsistent state

I am not sure we can do anything about this, so this is more out of curiosity - would it be deleted if a user retries the migration?

Yes.

If there is an existing index:
Previously:

we would try to create a new index

the create call would fail due to an existing index with the same name

Now, instead:

We try to create and index

The create call will fail due to a preexisting index with the same name

We catch the error and then delete the preexisting index

we retry the create, leaving the index in a broken state if it fails

Why not delete after step 4? Well, the error could be a cluster problem that prevents deletes as well. We'd still need to have the delete in response to a failed create. This was the original intention for how the code was supposed to work (IIRC after talking to @gwbrown), so I left the overall high-level approach as-is.

JVerwolf · 2025-02-13T15:37:36Z

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java

+    private void createIndexRetryOnFailure(SystemIndexMigrationInfo migrationInfo, ActionListener<ShardsAcknowledgedResponse> listener) {
+        createIndex(migrationInfo, listener.delegateResponse((l, e) -> {
+            logger.warn("createIndex failed, retrying after removing index [{}] from previous attempt", migrationInfo.getNextIndexName());
+            deleteIndex(migrationInfo, ActionListener.wrap(cleanupResponse -> createIndex(migrationInfo, l.delegateResponse((l3, e3) -> {


Here, we try to delete the index and retry if there is any error with the prior create call. I could instead only catch the specific "resource already exists" exception. However, I'd rather be more broad here in case there are other valid exceptions that get thrown, where deleting the existing resource is the right thing to do. Otherwise, we may end up in a non-recoverable state as a result of some scenario we haven't predicted.

Agree that it is better to be more broad here

JVerwolf · 2025-02-13T15:39:09Z

@elasticsearchmachine update branch

…ix-migration-blocked-from-previous-failure

elasticsearchmachine · 2025-02-13T19:50:17Z

💔 Backport failed

The backport operation could not be completed due to the following error:

There are no branches to backport to. Aborting.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 122326

JVerwolf · 2025-02-18T16:27:01Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x
✅	9.0
✅	8.18

Questions ?

Please refer to the Backport tool documentation

…astic#122326) This PR changes the code to no-longer rely on the persistent task state for the cleanup logic of existing indices. (cherry picked from commit 9076ac4)

…22326) (#122872) This PR changes the code to no-longer rely on the persistent task state for the cleanup logic of existing indices. (cherry picked from commit 9076ac4)

…22326) (#122874) This PR changes the code to no-longer rely on the persistent task state for the cleanup logic of existing indices. (cherry picked from commit 9076ac4)

…22326) (#122873) This PR changes the code to no-longer rely on the persistent task state for the cleanup logic of existing indices. (cherry picked from commit 9076ac4)

JVerwolf added >bug :Core/Infra/Core Core issues without another label labels Feb 11, 2025

JVerwolf requested a review from a team February 11, 2025 22:32

JVerwolf requested a review from a team as a code owner February 11, 2025 22:32

elasticsearchmachine added Team:Core/Infra Meta label for core/infra team v9.1.0 labels Feb 11, 2025

JVerwolf marked this pull request as draft February 11, 2025 22:43

Retry on createIndex failure

6fa041c

JVerwolf force-pushed the bugfix/fix-migration-blocked-from-previous-failure branch from bf04c7b to 6fa041c Compare February 11, 2025 22:58

JVerwolf removed the request for review from a team February 11, 2025 23:41

JVerwolf marked this pull request as ready for review February 11, 2025 23:41

Remove comment

abf0f97

remove dead code

aa2b112

JVerwolf commented Feb 12, 2025

View reviewed changes

Add additional failure branch

98cabbd

alexey-ivanov-es reviewed Feb 12, 2025

View reviewed changes

PR feedback: Fix log line

ed44d0d

JVerwolf requested a review from alexey-ivanov-es February 12, 2025 21:58

alexey-ivanov-es approved these changes Feb 13, 2025

View reviewed changes

JVerwolf commented Feb 13, 2025

View reviewed changes

Merge branch 'main' of github.com:elastic/elasticsearch into bugfix/f…

b3faf74

…ix-migration-blocked-from-previous-failure

JVerwolf added the auto-backport Automatically create backport pull requests when merged label Feb 13, 2025

JVerwolf enabled auto-merge (squash) February 13, 2025 17:01

Fix logger usage

b1d8538

JVerwolf merged commit 9076ac4 into elastic:main Feb 13, 2025
16 of 17 checks passed

elasticsearchmachine added the backport pending label Feb 13, 2025

JVerwolf added v9.0.1 v8.19.0 v8.18.1 labels Feb 18, 2025

System Index Migration Failure Results in a Non-Recoverable State #122326

System Index Migration Failure Results in a Non-Recoverable State #122326

Uh oh!

Conversation

JVerwolf commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 11, 2025

Uh oh!

elasticsearchmachine commented Feb 11, 2025

Uh oh!

JVerwolf commented Feb 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JVerwolf Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JVerwolf Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-ivanov-es left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JVerwolf Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JVerwolf commented Feb 12, 2025

Uh oh!

alexey-ivanov-es left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JVerwolf commented Feb 13, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 13, 2025

💔 Backport failed

Uh oh!

JVerwolf commented Feb 18, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JVerwolf commented Feb 11, 2025 •

edited

Loading

JVerwolf Feb 12, 2025 •

edited

Loading

JVerwolf Feb 12, 2025 •

edited

Loading

JVerwolf Feb 12, 2025 •

edited

Loading