Avoid making the index read-only in the Force merge action for ILM (C…

…loses elastic#43426) (elastic#81162)

    Added a new NoopStep that we can use as a placeholder for a step that we had in a previous version but now we want to remove it. If the just go and erase the step then the indices that were in that step will get into a stuck state. Instead, we're using the same old step key but with this NoopStep which does nothing but the transition to the next step.

    Modify the ForceMergeLifecycleAction in ILM so it doesn't make the index read-only. For older indices already in the "convert index to read-only" step before the upgrade, we'll use the NoopStep mentioned before in order to ignore the step and just transition to the next one.

Solves: elastic#43426

Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>

docs/changelog/81162.yaml

@@ -0,0 +1,6 @@
+pr: 81162
+summary: Stop making index read-only when executing force merge index lifecycle management action
+area: Infra/Core
+type: enhancement
+issues:
+ - 81162

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

@@ -35,8 +35,6 @@
 public class ForceMergeAction implements LifecycleAction {
     private static final Logger logger = LogManager.getLogger(ForceMergeAction.class);
 
-    private static final Settings READ_ONLY_SETTINGS = Settings.builder().put(IndexMetadata.SETTING_BLOCKS_WRITE, true).build();
-
     private static final Settings BEST_COMPRESSION_SETTINGS = Settings.builder()
         .put(EngineConfig.INDEX_CODEC_SETTING.getKey(), CodecService.BEST_COMPRESSION_CODEC)
         .build();
@@ -120,7 +118,7 @@ public List<Step> toSteps(Client client, String phase, Step.StepKey nextStepKey)
         final boolean codecChange = codec != null && codec.equals(CodecService.BEST_COMPRESSION_CODEC);
 
         StepKey preForceMergeBranchingKey = new StepKey(phase, NAME, CONDITIONAL_SKIP_FORCE_MERGE_STEP);
-        StepKey checkNotWriteIndex = new StepKey(phase, NAME, CheckNotDataStreamWriteIndexStep.NAME);
+        StepKey checkNotWriteIndexKey = new StepKey(phase, NAME, CheckNotDataStreamWriteIndexStep.NAME);
         StepKey readOnlyKey = new StepKey(phase, NAME, ReadOnlyAction.NAME);
 
         StepKey closeKey = new StepKey(phase, NAME, CloseIndexStep.NAME);
@@ -133,7 +131,7 @@ public List<Step> toSteps(Client client, String phase, Step.StepKey nextStepKey)
 
         BranchingStep conditionalSkipShrinkStep = new BranchingStep(
             preForceMergeBranchingKey,
-            checkNotWriteIndex,
+            checkNotWriteIndexKey,
             nextStepKey,
             (index, clusterState) -> {
                 IndexMetadata indexMetadata = clusterState.metadata().index(index);
@@ -152,14 +150,18 @@ public List<Step> toSteps(Client client, String phase, Step.StepKey nextStepKey)
                 return false;
             }
         );
-        CheckNotDataStreamWriteIndexStep checkNotWriteIndexStep = new CheckNotDataStreamWriteIndexStep(checkNotWriteIndex, readOnlyKey);
-        UpdateSettingsStep readOnlyStep = new UpdateSettingsStep(
-            readOnlyKey,
-            codecChange ? closeKey : forceMergeKey,
-            client,
-            READ_ONLY_SETTINGS
+
+        // Indices in this step key can skip the no-op step and jump directly to the step with closeKey/forcemergeKey key
+        CheckNotDataStreamWriteIndexStep checkNotWriteIndexStep = new CheckNotDataStreamWriteIndexStep(
+            checkNotWriteIndexKey,
+            codecChange ? closeKey : forceMergeKey
         );
 
+        // Indices already in this step key when upgrading need to know how to move forward but stop making the index
+        // read-only. In order to achieve this we introduce a no-op step with the same key as the read-only step so that
+        // the index can safely move to the next step without performing any read-only action nor getting stuck in this step
+        NoopStep noopStep = new NoopStep(readOnlyKey, codecChange ? closeKey : forceMergeKey);
+
         CloseIndexStep closeIndexStep = new CloseIndexStep(closeKey, updateCompressionKey, client);
         UpdateSettingsStep updateBestCompressionSettings = new UpdateSettingsStep(
             updateCompressionKey,
@@ -180,7 +182,7 @@ public List<Step> toSteps(Client client, String phase, Step.StepKey nextStepKey)
         List<Step> mergeSteps = new ArrayList<>();
         mergeSteps.add(conditionalSkipShrinkStep);
         mergeSteps.add(checkNotWriteIndexStep);
-        mergeSteps.add(readOnlyStep);
+        mergeSteps.add(noopStep);
 
         if (codecChange) {
             mergeSteps.add(closeIndexStep);

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

@@ -0,0 +1,34 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+package org.elasticsearch.xpack.core.ilm;
+
+import org.elasticsearch.cluster.ClusterState;
+import org.elasticsearch.index.Index;
+
+/**
+ * This is a Noop step that can be used for backwards compatibility when removing some step in newer versions.
+ * It literally does nothing so that we can safely proceed to the nextStepKey without getting stuck.
+ */
+public class NoopStep extends ClusterStateWaitStep {
+    public static final String NAME = "noop";
+
+    public NoopStep(StepKey key, StepKey nextStepKey) {
+        super(key, nextStepKey);
+    }
+
+    @Override
+    public boolean isRetryable() {
+        // As this is a noop step and we don't want to get stuck in, we want it to be retryable
+        return true;
+    }
+
+    @Override
+    public Result isConditionMet(Index index, ClusterState clusterState) {
+        // We always want to move forward with this step so this should always be true
+        return new Result(true, null);
+    }
+}

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

@@ -6,7 +6,6 @@
  */
 package org.elasticsearch.xpack.core.ilm;
 
-import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.io.stream.Writeable.Reader;
 import org.elasticsearch.common.xcontent.XContentHelper;
@@ -70,21 +69,24 @@ private void assertNonBestCompression(ForceMergeAction instance) {
         assertEquals(5, steps.size());
         BranchingStep firstStep = (BranchingStep) steps.get(0);
         CheckNotDataStreamWriteIndexStep secondStep = (CheckNotDataStreamWriteIndexStep) steps.get(1);
-        UpdateSettingsStep thirdStep = (UpdateSettingsStep) steps.get(2);
+        NoopStep thirdStep = (NoopStep) steps.get(2);
         ForceMergeStep fourthStep = (ForceMergeStep) steps.get(3);
         SegmentCountStep fifthStep = (SegmentCountStep) steps.get(4);
 
         assertThat(
             firstStep.getKey(),
             equalTo(new StepKey(phase, ForceMergeAction.NAME, ForceMergeAction.CONDITIONAL_SKIP_FORCE_MERGE_STEP))
         );
+
         assertThat(secondStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, CheckNotDataStreamWriteIndexStep.NAME)));
-        assertThat(secondStep.getNextStepKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ReadOnlyAction.NAME)));
+        assertThat(secondStep.getNextStepKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ForceMergeStep.NAME)));
+
         assertThat(thirdStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ReadOnlyAction.NAME)));
         assertThat(thirdStep.getNextStepKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ForceMergeStep.NAME)));
-        assertTrue(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.get(thirdStep.getSettings()));
+
         assertThat(fourthStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ForceMergeStep.NAME)));
-        assertThat(fourthStep.getNextStepKey(), equalTo(fifthStep.getKey()));
+        assertThat(fourthStep.getNextStepKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, SegmentCountStep.NAME)));
+
         assertThat(fifthStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, SegmentCountStep.NAME)));
         assertThat(fifthStep.getNextStepKey(), equalTo(nextStepKey));
     }
@@ -101,8 +103,9 @@ private void assertBestCompression(ForceMergeAction instance) {
             .skip(1)
             .map(s -> new Tuple<>(s.getKey(), s.getNextStepKey()))
             .collect(Collectors.toList());
+
         StepKey checkNotWriteIndex = new StepKey(phase, ForceMergeAction.NAME, CheckNotDataStreamWriteIndexStep.NAME);
-        StepKey readOnly = new StepKey(phase, ForceMergeAction.NAME, ReadOnlyAction.NAME);
+        StepKey noop = new StepKey(phase, ForceMergeAction.NAME, ReadOnlyAction.NAME);
         StepKey closeIndex = new StepKey(phase, ForceMergeAction.NAME, CloseIndexStep.NAME);
         StepKey updateCodec = new StepKey(phase, ForceMergeAction.NAME, UpdateSettingsStep.NAME);
         StepKey openIndex = new StepKey(phase, ForceMergeAction.NAME, OpenIndexStep.NAME);
@@ -116,8 +119,8 @@ private void assertBestCompression(ForceMergeAction instance) {
         assertThat(
             stepKeys,
             contains(
-                new Tuple<>(checkNotWriteIndex, readOnly),
-                new Tuple<>(readOnly, closeIndex),
+                new Tuple<>(checkNotWriteIndex, closeIndex),
+                new Tuple<>(noop, closeIndex),
                 new Tuple<>(closeIndex, updateCodec),
                 new Tuple<>(updateCodec, openIndex),
                 new Tuple<>(openIndex, waitForGreen),
@@ -127,11 +130,11 @@ private void assertBestCompression(ForceMergeAction instance) {
             )
         );
 
-        UpdateSettingsStep thirdStep = (UpdateSettingsStep) steps.get(2);
-        UpdateSettingsStep fifthStep = (UpdateSettingsStep) steps.get(4);
-
-        assertTrue(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.get(thirdStep.getSettings()));
-        assertThat(fifthStep.getSettings().get(EngineConfig.INDEX_CODEC_SETTING.getKey()), equalTo(CodecService.BEST_COMPRESSION_CODEC));
+        UpdateSettingsStep updateCodecStep = (UpdateSettingsStep) steps.get(4);
+        assertThat(
+            updateCodecStep.getSettings().get(EngineConfig.INDEX_CODEC_SETTING.getKey()),
+            equalTo(CodecService.BEST_COMPRESSION_CODEC)
+        );
     }
 
     public void testMissingMaxNumSegments() throws IOException {

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

@@ -279,6 +279,7 @@ public void testReadStepKeys() {
                 new Step.StepKey("phase", "allocate", AllocationRoutedStep.NAME),
                 new Step.StepKey("phase", "forcemerge", ForceMergeAction.CONDITIONAL_SKIP_FORCE_MERGE_STEP),
                 new Step.StepKey("phase", "forcemerge", CheckNotDataStreamWriteIndexStep.NAME),
+                // This read-only key is now a noop step but we preserved it for backwards compatibility
                 new Step.StepKey("phase", "forcemerge", ReadOnlyAction.NAME),
                 new Step.StepKey("phase", "forcemerge", ForceMergeAction.NAME),
                 new Step.StepKey("phase", "forcemerge", SegmentCountStep.NAME)

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

@@ -15,6 +15,7 @@
 import org.elasticsearch.cluster.metadata.Template;
 import org.elasticsearch.common.settings.Settings;
 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;
@@ -241,8 +242,8 @@ public void testFreezeAction() throws Exception {
         assertNull(settings.get("index.frozen"));
     }
 
-    public void testForceMergeAction() throws Exception {
-        createNewSingletonPolicy(client(), policyName, "warm", new ForceMergeAction(1, null));
+    public void checkForceMergeAction(String codec) throws Exception {
+        createNewSingletonPolicy(client(), policyName, "warm", new ForceMergeAction(1, codec));
         createComposableTemplate(client(), template, dataStream + "*", getTemplate(policyName));
         indexDocument(client(), dataStream, true);
 
@@ -260,11 +261,20 @@ public void testForceMergeAction() throws Exception {
         // Manual rollover the original index such that it's not the write index in the data stream anymore
         rolloverMaxOneDocCondition(client(), dataStream);
 
-        assertBusy(
-            () -> assertThat(explainIndex(client(), backingIndexName).get("step"), is(PhaseCompleteStep.NAME)),
-            30,
-            TimeUnit.SECONDS
-        );
+        assertBusy(() -> {
+            assertThat(explainIndex(client(), backingIndexName).get("step"), is(PhaseCompleteStep.NAME));
+            Map<String, Object> settings = getOnlyIndexSettings(client(), backingIndexName);
+            assertThat(settings.get(EngineConfig.INDEX_CODEC_SETTING.getKey()), equalTo(codec));
+            assertThat(settings.containsKey(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.getKey()), equalTo(false));
+        }, 30, TimeUnit.SECONDS);
+    }
+
+    public void testForceMergeAction() throws Exception {
+        checkForceMergeAction(null);
+    }
+
+    public void testForceMergeActionWithCompressionCodec() throws Exception {
+        checkForceMergeAction("best_compression");
     }
 
     @SuppressWarnings("unchecked")

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

@@ -473,7 +473,7 @@ public void testDeleteDuringSnapshot() throws Exception {
         assertOK(client().performRequest(new Request("DELETE", "/_snapshot/repo/" + snapName)));
     }
 
-    public void forceMergeActionWithCodec(String codec) throws Exception {
+    public void checkForceMergeAction(String codec) throws Exception {
         createIndexWithSettings(
             client(),
             index,
@@ -495,17 +495,19 @@ public void forceMergeActionWithCodec(String codec) throws Exception {
             assertThat(getStepKeyForIndex(client(), index), equalTo(PhaseCompleteStep.finalStep("warm").getKey()));
             Map<String, Object> settings = getOnlyIndexSettings(client(), index);
             assertThat(settings.get(EngineConfig.INDEX_CODEC_SETTING.getKey()), equalTo(codec));
-            assertThat(settings.get(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.getKey()), equalTo("true"));
+            assertThat(settings.containsKey(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.getKey()), equalTo(false));
         }, 30, TimeUnit.SECONDS);
-        expectThrows(ResponseException.class, () -> indexDocument(client(), index));
+
+        // No exception should be thrown here as writes were not blocked
+        indexDocument(client(), index);
     }
 
     public void testForceMergeAction() throws Exception {
-        forceMergeActionWithCodec(null);
+        checkForceMergeAction(null);
     }
 
     public void testForceMergeActionWithCompressionCodec() throws Exception {
-        forceMergeActionWithCodec("best_compression");
+        checkForceMergeAction("best_compression");
     }
 
     public void testSetPriority() throws Exception {