[9.0] Deprecate ILM Freeze Action & remove min_age check (#119456)

Author: gmarouli

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

@@ -6,12 +6,10 @@
  */
 package org.elasticsearch.xpack.core.ilm;
 
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.elasticsearch.client.internal.Client;
-import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.core.UpdateForV10;
 import org.elasticsearch.xcontent.ObjectParser;
 import org.elasticsearch.xcontent.XContentBuilder;
 import org.elasticsearch.xcontent.XContentParser;
@@ -21,10 +19,14 @@
 import java.util.List;
 
 /**
- * A {@link LifecycleAction} which freezes the index.
+ * A noop {@link LifecycleAction} that replaces the removed freeze action. We keep it for backwards compatibility purposes in case we
+ * encounter a policy or an index that refers to this action and its steps in the lifecycle state.
+ * At 10.x we would like to sanitize the freeze action from input and expunge from the lifecycle execution
+ * state of indices.
  */
+@Deprecated
+@UpdateForV10(owner = UpdateForV10.Owner.DATA_MANAGEMENT)
 public class FreezeAction implements LifecycleAction {
-    private static final Logger logger = LogManager.getLogger(FreezeAction.class);
 
     public static final String NAME = "freeze";
     public static final String CONDITIONAL_SKIP_FREEZE_STEP = BranchingStep.NAME + "-freeze-check-prerequisites";
@@ -34,6 +36,7 @@ public class FreezeAction implements LifecycleAction {
     private static final ObjectParser<FreezeAction, Void> PARSER = new ObjectParser<>(NAME, () -> INSTANCE);
 
     public static FreezeAction parse(XContentParser parser) {
+
         return PARSER.apply(parser, null);
     }
 
@@ -63,40 +66,10 @@ public boolean isSafeAction() {
     public List<Step> toSteps(Client client, String phase, StepKey nextStepKey) {
         StepKey preFreezeMergeBranchingKey = new StepKey(phase, NAME, CONDITIONAL_SKIP_FREEZE_STEP);
         StepKey checkNotWriteIndex = new StepKey(phase, NAME, CheckNotDataStreamWriteIndexStep.NAME);
-        StepKey freezeStepKey = new StepKey(phase, NAME, FreezeStep.NAME);
-
-        BranchingStep conditionalSkipFreezeStep = new BranchingStep(
-            preFreezeMergeBranchingKey,
-            checkNotWriteIndex,
-            nextStepKey,
-            (index, clusterState) -> {
-                IndexMetadata indexMetadata = clusterState.getMetadata().index(index);
-                assert indexMetadata != null : "index " + index.getName() + " must exist in the cluster state";
-                String policyName = indexMetadata.getLifecyclePolicyName();
-                if (indexMetadata.getSettings().get(LifecycleSettings.SNAPSHOT_INDEX_NAME) != null) {
-                    logger.warn(
-                        "[{}] action is configured for index [{}] in policy [{}] which is mounted as searchable snapshot. "
-                            + "Skipping this action",
-                        FreezeAction.NAME,
-                        index.getName(),
-                        policyName
-                    );
-                    return true;
-                }
-                if (indexMetadata.getSettings().getAsBoolean("index.frozen", false)) {
-                    logger.debug(
-                        "skipping [{}] action for index [{}] in policy [{}] as the index is already frozen",
-                        FreezeAction.NAME,
-                        index.getName(),
-                        policyName
-                    );
-                    return true;
-                }
-                return false;
-            }
-        );
-        CheckNotDataStreamWriteIndexStep checkNoWriteIndexStep = new CheckNotDataStreamWriteIndexStep(checkNotWriteIndex, freezeStepKey);
-        FreezeStep freezeStep = new FreezeStep(freezeStepKey, nextStepKey, client);
+        StepKey freezeStepKey = new StepKey(phase, NAME, NAME);
+        NoopStep conditionalSkipFreezeStep = new NoopStep(preFreezeMergeBranchingKey, nextStepKey);
+        NoopStep checkNoWriteIndexStep = new NoopStep(checkNotWriteIndex, nextStepKey);
+        NoopStep freezeStep = new NoopStep(freezeStepKey, nextStepKey);
         return List.of(conditionalSkipFreezeStep, checkNoWriteIndexStep, freezeStep);
     }
 

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

@@ -13,6 +13,7 @@
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.core.Nullable;
+import org.elasticsearch.core.UpdateForV10;
 import org.elasticsearch.license.XPackLicenseState;
 import org.elasticsearch.xcontent.ConstructingObjectParser;
 import org.elasticsearch.xcontent.ParseField;
@@ -33,6 +34,8 @@
 import java.util.function.Function;
 import java.util.stream.Collectors;
 
+import static org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType.COLD_PHASE;
+
 /**
  * Represents the lifecycle of an index from creation to deletion. A
  * {@link LifecyclePolicy} is made up of a set of {@link Phase}s which it will
@@ -365,4 +368,13 @@ public boolean equals(Object obj) {
     public String toString() {
         return Strings.toString(this, true, true);
     }
+
+    @UpdateForV10(owner = UpdateForV10.Owner.DATA_MANAGEMENT)
+    public boolean maybeAddDeprecationWarningForFreezeAction(String policyName) {
+        Phase coldPhase = phases.get(COLD_PHASE);
+        if (coldPhase != null) {
+            return coldPhase.maybeAddDeprecationWarningForFreezeAction(policyName);
+        }
+        return false;
+    }
 }

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

@@ -6,15 +6,15 @@
  */
 package org.elasticsearch.xpack.core.ilm;
 
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
+import org.elasticsearch.common.logging.DeprecationCategory;
+import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.util.Maps;
 import org.elasticsearch.core.TimeValue;
-import org.elasticsearch.core.UpdateForV9;
+import org.elasticsearch.core.UpdateForV10;
 import org.elasticsearch.xcontent.ConstructingObjectParser;
 import org.elasticsearch.xcontent.ContextParser;
 import org.elasticsearch.xcontent.ObjectParser.ValueType;
@@ -34,7 +34,7 @@
  * particular point in the lifecycle of an index.
  */
 public class Phase implements ToXContentObject, Writeable {
-    private static final Logger logger = LogManager.getLogger(Phase.class);
+    private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(Phase.class);
 
     public static final ParseField MIN_AGE = new ParseField("min_age");
     public static final ParseField ACTIONS_FIELD = new ParseField("actions");
@@ -51,19 +51,12 @@ public class Phase implements ToXContentObject, Writeable {
         return new Phase(name, (TimeValue) a[0], map);
     });
     static {
-        PARSER.declareField(ConstructingObjectParser.optionalConstructorArg(), (ContextParser<String, Object>) (p, c) -> {
-            // In earlier versions it was possible to create a Phase with a negative `min_age` which would then cause errors
-            // when the phase is read from the cluster state during startup (even before negative timevalues were strictly
-            // disallowed) so this is a hack to treat negative `min_age`s as 0 to prevent those errors.
-            // They will be saved as `0` so this hack can be removed once we no longer have to read cluster states from 7.x.
-            @UpdateForV9(owner = UpdateForV9.Owner.DATA_MANAGEMENT) // remove this hack now that we don't have to read 7.x cluster states
-            final String timeValueString = p.text();
-            if (timeValueString.startsWith("-")) {
-                logger.warn("phase has negative min_age value of [{}] - this will be treated as a min_age of 0", timeValueString);
-                return TimeValue.ZERO;
-            }
-            return TimeValue.parseTimeValue(timeValueString, MIN_AGE.getPreferredName());
-        }, MIN_AGE, ValueType.VALUE);
+        PARSER.declareField(
+            ConstructingObjectParser.optionalConstructorArg(),
+            (ContextParser<String, Object>) (p, c) -> TimeValue.parseTimeValue(p.text(), MIN_AGE.getPreferredName()),
+            MIN_AGE,
+            ValueType.VALUE
+        );
         PARSER.declareNamedObjects(
             ConstructingObjectParser.constructorArg(),
             (p, c, n) -> p.namedObject(LifecycleAction.class, n, null),
@@ -183,4 +176,20 @@ public String toString() {
         return Strings.toString(this, true, true);
     }
 
+    @UpdateForV10(owner = UpdateForV10.Owner.DATA_MANAGEMENT)
+    public boolean maybeAddDeprecationWarningForFreezeAction(String policyName) {
+        if (getActions().containsKey(FreezeAction.NAME)) {
+            deprecationLogger.warn(
+                DeprecationCategory.OTHER,
+                "ilm_freeze_action_deprecation",
+                "The freeze action in ILM is deprecated and will be removed in a future version;"
+                    + " this action is already a noop so it can be safely removed. Please remove the freeze action from the '"
+                    + policyName
+                    + "' policy."
+            );
+            return true;
+        }
+        return false;
+    }
+
 }

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

@@ -29,7 +29,8 @@ protected FreezeAction createTestInstance() {
 
     @Override
     protected FreezeAction mutateInstance(FreezeAction instance) {
-        return null;// TODO implement https://github.com/elastic/elasticsearch/issues/25929
+        // This class is a singleton
+        return null;
     }
 
     @Override
@@ -50,16 +51,15 @@ public void testToSteps() {
         assertEquals(3, steps.size());
         StepKey expectedFirstStepKey = new StepKey(phase, FreezeAction.NAME, FreezeAction.CONDITIONAL_SKIP_FREEZE_STEP);
         StepKey expectedSecondStepKey = new StepKey(phase, FreezeAction.NAME, CheckNotDataStreamWriteIndexStep.NAME);
-        StepKey expectedThirdStepKey = new StepKey(phase, FreezeAction.NAME, FreezeStep.NAME);
-
-        BranchingStep firstStep = (BranchingStep) steps.get(0);
-        CheckNotDataStreamWriteIndexStep secondStep = (CheckNotDataStreamWriteIndexStep) steps.get(1);
-        FreezeStep thirdStep = (FreezeStep) steps.get(2);
+        StepKey expectedThirdStepKey = new StepKey(phase, FreezeAction.NAME, FreezeAction.NAME);
 
+        NoopStep firstStep = (NoopStep) steps.get(0);
         assertThat(firstStep.getKey(), equalTo(expectedFirstStepKey));
-
+        assertThat(firstStep.getNextStepKey(), equalTo(nextStepKey));
+        NoopStep secondStep = (NoopStep) steps.get(1);
         assertEquals(expectedSecondStepKey, secondStep.getKey());
-        assertEquals(expectedThirdStepKey, secondStep.getNextStepKey());
+        assertEquals(nextStepKey, secondStep.getNextStepKey());
+        NoopStep thirdStep = (NoopStep) steps.get(2);
         assertEquals(expectedThirdStepKey, thirdStep.getKey());
         assertEquals(nextStepKey, thirdStep.getNextStepKey());
     }

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

@@ -35,6 +35,7 @@
 
 public class LifecyclePolicyTests extends AbstractXContentSerializingTestCase<LifecyclePolicy> {
 
+    // Excluding the deprecated freeze action and test it separately
     private String lifecycleName;
 
     @Override

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

@@ -9,6 +9,7 @@
 
 import org.elasticsearch.client.Request;
 import org.elasticsearch.client.Response;
+import org.elasticsearch.client.WarningFailureException;
 import org.elasticsearch.cluster.metadata.DataStream;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.metadata.Template;
@@ -216,29 +217,27 @@ public void testReadOnlyAction() throws Exception {
     }
 
     public void testFreezeAction() throws Exception {
-        createNewSingletonPolicy(client(), policyName, "cold", FreezeAction.INSTANCE);
+        try {
+            createNewSingletonPolicy(client(), policyName, "cold", FreezeAction.INSTANCE);
+        } catch (WarningFailureException e) {
+            assertThat(e.getMessage(), containsString("The freeze action in ILM is deprecated and will be removed in a future version"));
+        }
         createComposableTemplate(client(), template, dataStream + "*", getTemplate(policyName));
         indexDocument(client(), dataStream, true);
 
+        // The freeze action is a noop action with only noop steps and should pass through to complete the phase asap.
         String backingIndexName = DataStream.getDefaultBackingIndexName(dataStream, 1);
-        assertBusy(
-            () -> assertThat(
-                "index must wait in the " + CheckNotDataStreamWriteIndexStep.NAME + " until it is not the write index anymore",
-                explainIndex(client(), backingIndexName).get("step"),
-                is(CheckNotDataStreamWriteIndexStep.NAME)
-            ),
-            30,
-            TimeUnit.SECONDS
-        );
-
-        // 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(() -> {
+            try {
+                assertThat(explainIndex(client(), backingIndexName).get("step"), is(PhaseCompleteStep.NAME));
+                fail("expected a deprecation warning");
+            } catch (WarningFailureException e) {
+                assertThat(
+                    e.getMessage(),
+                    containsString("The freeze action in ILM is deprecated and will be removed in a future version")
+                );
+            }
+        }, 30, TimeUnit.SECONDS);
 
         Map<String, Object> settings = getOnlyIndexSettings(client(), backingIndexName);
         assertNull(settings.get("index.frozen"));