[ILM] Delete step deletes data stream with only one index (#105772) (#…

…105897)

We seem to have a couple of checks to make sure we delete the data
stream when the last index reaches the delete step however, these checks
seem a bit contradictory.

Namely, the first check makes use if `Index` equality (UUID included)
and the second just checks the index name. So if a data stream with just
one index (the write index) is restored from snapshot (different UUID)
we would've failed the first index equality check and go through the
second check `dataStream.getWriteIndex().getName().equals(indexName)`
and fail the delete step (in a non-retryable way :( ) because we don't
want to delete the write index of a data stream (but we really do if the
data stream has only one index)

This PR makes 2 changes: 1. use the index name equality everywhere in
the step (we already looked up the index abstraction and the parent data
stream, so we know for sure the managed index is part of the data
stream) 2. do not throw exception when we got here via a write index
that is NOT the last index in the data stream but report the exception
so we keep retrying this step (i.e. this enables our users to simply
execute a manual rollover and the index is deleted by ILM eventually on
retry)

docs/changelog/105772.yaml

@@ -0,0 +1,5 @@
+pr: 105772
+summary: "[ILM] Delete step deletes data stream with only one index"
+area: ILM+SLM
+type: bug
+issues: []

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java

@@ -41,7 +41,10 @@ public void performDuringNoSnapshot(IndexMetadata indexMetadata, ClusterState cu
 
         if (dataStream != null) {
             assert dataStream.getWriteIndex() != null : dataStream.getName() + " has no write index";
-            if (dataStream.getIndices().size() == 1 && dataStream.getIndices().get(0).equals(indexMetadata.getIndex())) {
+
+            // using index name equality across this if/else branch as the UUID of the index might change via restoring a data stream
+            // with one index from snapshot
+            if (dataStream.getIndices().size() == 1 && dataStream.getWriteIndex().getName().equals(indexName)) {
                 // This is the last index in the data stream, the entire stream
                 // needs to be deleted, because we can't have an empty data stream
                 DeleteDataStreamAction.Request deleteReq = new DeleteDataStreamAction.Request(new String[] { dataStream.getName() });
@@ -62,7 +65,8 @@ public void performDuringNoSnapshot(IndexMetadata indexMetadata, ClusterState cu
                     policyName
                 );
                 logger.debug(errorMessage);
-                throw new IllegalStateException(errorMessage);
+                listener.onFailure(new IllegalStateException(errorMessage));
+                return;
             }
         }
 

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/DeleteStepTests.java

@@ -21,7 +21,10 @@
 
 import java.util.List;
 
+import static org.elasticsearch.test.ActionListenerUtils.anyActionListener;
 import static org.hamcrest.Matchers.is;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
 
 public class DeleteStepTests extends AbstractStepTestCase<DeleteStep> {
 
@@ -76,7 +79,7 @@ public void testDeleted() throws Exception {
             assertEquals(indexMetadata.getIndex().getName(), request.indices()[0]);
             listener.onResponse(null);
             return null;
-        }).when(indicesClient).delete(Mockito.any(), Mockito.any());
+        }).when(indicesClient).delete(any(), any());
 
         DeleteStep step = createRandomInstance();
         ClusterState clusterState = ClusterState.builder(emptyClusterState())
@@ -86,7 +89,7 @@ public void testDeleted() throws Exception {
 
         Mockito.verify(client, Mockito.only()).admin();
         Mockito.verify(adminClient, Mockito.only()).indices();
-        Mockito.verify(indicesClient, Mockito.only()).delete(Mockito.any(), Mockito.any());
+        Mockito.verify(indicesClient, Mockito.only()).delete(any(), any());
     }
 
     public void testExceptionThrown() {
@@ -102,7 +105,7 @@ public void testExceptionThrown() {
             assertEquals(indexMetadata.getIndex().getName(), request.indices()[0]);
             listener.onFailure(exception);
             return null;
-        }).when(indicesClient).delete(Mockito.any(), Mockito.any());
+        }).when(indicesClient).delete(any(), any());
 
         DeleteStep step = createRandomInstance();
         ClusterState clusterState = ClusterState.builder(emptyClusterState())
@@ -117,7 +120,13 @@ public void testExceptionThrown() {
         );
     }
 
-    public void testPerformActionThrowsExceptionIfIndexIsTheDataStreamWriteIndex() {
+    public void testPerformActionCallsFailureListenerIfIndexIsTheDataStreamWriteIndex() {
+        doThrow(
+            new IllegalStateException(
+                "the client must not be called in this test as we should fail in the step validation phase before we call the delete API"
+            )
+        ).when(indicesClient).delete(any(DeleteIndexRequest.class), anyActionListener());
+
         String policyName = "test-ilm-policy";
         String dataStreamName = randomAlphaOfLength(10);
 
@@ -149,31 +158,27 @@ public void testPerformActionThrowsExceptionIfIndexIsTheDataStreamWriteIndex() {
             .metadata(Metadata.builder().put(index1, false).put(sourceIndexMetadata, false).put(dataStream).build())
             .build();
 
-        IllegalStateException illegalStateException = expectThrows(
-            IllegalStateException.class,
-            () -> createRandomInstance().performDuringNoSnapshot(sourceIndexMetadata, clusterState, new ActionListener<>() {
-                @Override
-                public void onResponse(Void complete) {
-                    fail("unexpected listener callback");
-                }
-
-                @Override
-                public void onFailure(Exception e) {
-                    fail("unexpected listener callback");
-                }
-            })
-        );
-        assertThat(
-            illegalStateException.getMessage(),
-            is(
-                "index ["
-                    + sourceIndexMetadata.getIndex().getName()
-                    + "] is the write index for data stream ["
-                    + dataStreamName
-                    + "]. stopping execution of lifecycle [test-ilm-policy] as a data stream's write index cannot be deleted. "
-                    + "manually rolling over the index will resume the execution of the policy as the index will not be the "
-                    + "data stream's write index anymore"
-            )
-        );
+        createRandomInstance().performDuringNoSnapshot(sourceIndexMetadata, clusterState, new ActionListener<>() {
+            @Override
+            public void onResponse(Void complete) {
+                fail("unexpected listener callback");
+            }
+
+            @Override
+            public void onFailure(Exception e) {
+                assertThat(
+                    e.getMessage(),
+                    is(
+                        "index ["
+                            + sourceIndexMetadata.getIndex().getName()
+                            + "] is the write index for data stream ["
+                            + dataStreamName
+                            + "]. stopping execution of lifecycle [test-ilm-policy] as a data stream's write index cannot be deleted. "
+                            + "manually rolling over the index will resume the execution of the policy as the index will not be the "
+                            + "data stream's write index anymore"
+                    )
+                );
+            }
+        });
     }
 }

x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesDataStreamsIT.java

@@ -11,12 +11,14 @@
 import org.elasticsearch.client.Response;
 import org.elasticsearch.cluster.metadata.DataStream;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
+import org.elasticsearch.cluster.metadata.Template;
 import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.index.engine.EngineConfig;
 import org.elasticsearch.test.rest.ESRestTestCase;
 import org.elasticsearch.xcontent.XContentType;
 import org.elasticsearch.xpack.core.ilm.CheckNotDataStreamWriteIndexStep;
 import org.elasticsearch.xpack.core.ilm.DeleteAction;
+import org.elasticsearch.xpack.core.ilm.DeleteStep;
 import org.elasticsearch.xpack.core.ilm.ForceMergeAction;
 import org.elasticsearch.xpack.core.ilm.FreezeAction;
 import org.elasticsearch.xpack.core.ilm.PhaseCompleteStep;
@@ -37,6 +39,7 @@
 import static org.elasticsearch.xpack.TimeSeriesRestDriver.createNewSingletonPolicy;
 import static org.elasticsearch.xpack.TimeSeriesRestDriver.createSnapshotRepo;
 import static org.elasticsearch.xpack.TimeSeriesRestDriver.explainIndex;
+import static org.elasticsearch.xpack.TimeSeriesRestDriver.getBackingIndices;
 import static org.elasticsearch.xpack.TimeSeriesRestDriver.getOnlyIndexSettings;
 import static org.elasticsearch.xpack.TimeSeriesRestDriver.getStepKeyForIndex;
 import static org.elasticsearch.xpack.TimeSeriesRestDriver.getTemplate;
@@ -45,6 +48,7 @@
 import static org.elasticsearch.xpack.TimeSeriesRestDriver.waitAndGetShrinkIndexName;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.is;
 
 public class TimeSeriesDataStreamsIT extends ESRestTestCase {
@@ -303,4 +307,46 @@ public void testDeleteOnlyIndexInDataStreamDeletesDataStream() throws Exception
         });
     }
 
+    @SuppressWarnings("unchecked")
+    public void testDataStreamWithMultipleIndicesAndWriteIndexInDeletePhase() throws Exception {
+        createComposableTemplate(client(), template, dataStream + "*", new Template(null, null, null, null));
+        indexDocument(client(), dataStream, true);
+
+        createNewSingletonPolicy(client(), policyName, "delete", DeleteAction.NO_SNAPSHOT_DELETE);
+        // let's update the index template so the new write index (after rollover) is managed by an ILM policy that sents it to the
+        // delete step - note that we'll have here a data stream with generation 000001 not managed and the write index 000002 in the
+        // delete phase (the write index in this case, being not the only backing index must NOT be deleted).
+        createComposableTemplate(client(), template, dataStream + "*", getTemplate(policyName));
+
+        client().performRequest(new Request("POST", dataStream + "/_rollover"));
+        indexDocument(client(), dataStream, true);
+
+        String secondGenerationIndex = getBackingIndices(client(), dataStream).get(1);
+        assertBusy(() -> {
+            Request explainRequest = new Request("GET", "/_data_stream/" + dataStream);
+            Response response = client().performRequest(explainRequest);
+            Map<String, Object> responseMap;
+            try (InputStream is = response.getEntity().getContent()) {
+                responseMap = XContentHelper.convertToMap(XContentType.JSON.xContent(), is, true);
+            }
+
+            List<Object> dataStreams = (List<Object>) responseMap.get("data_streams");
+            assertThat(dataStreams.size(), is(1));
+            Map<String, Object> dataStream = (Map<String, Object>) dataStreams.get(0);
+
+            List<Object> indices = (List<Object>) dataStream.get("indices");
+            // no index should be deleted
+            assertThat(indices.size(), is(2));
+
+            Map<String, Object> explainIndex = explainIndex(client(), secondGenerationIndex);
+            assertThat(explainIndex.get("failed_step"), is(DeleteStep.NAME));
+            assertThat((Integer) explainIndex.get("failed_step_retry_count"), is(greaterThan(1)));
+        });
+
+        // rolling the data stream again would see 000002 not be the write index anymore and should be deleted automatically
+        client().performRequest(new Request("POST", dataStream + "/_rollover"));
+
+        assertBusy(() -> assertThat(indexExists(secondGenerationIndex), is(false)));
+    }
+
 }