stop throwing an IllegalStateException for unrecognized steps (#32212)

Since the reason for a step not being found in a registry may be due to staleness of the
registry between it and the cluster state, we do not want to throw an IllegalStateException.

Staleness is something that will be self-healing after follow-up applications of the cluster state
updates, so this is a recoverable issue that should log a warning instead of throwing an exception

Closes #32181.

Author: talevy

x-pack/plugin/index-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java

@@ -9,6 +9,7 @@
 
 import org.apache.logging.log4j.Logger;
 import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.action.support.TransportAction;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.cluster.metadata.MetaData;
@@ -64,8 +65,11 @@ public void runPolicy(String policy, IndexMetaData indexMetaData, ClusterState c
         }
         Step currentStep = getCurrentStep(stepRegistry, policy, indexSettings);
         if (currentStep == null) {
-            throw new IllegalStateException(
-                "current step for index [" + indexMetaData.getIndex().getName() + "] with policy [" + policy + "] is not recognized");
+            // This may happen in the case that there is invalid ilm-step index settings or the stepRegistry is out of
+            // sync with the current cluster state
+            logger.warn("current step [" + getCurrentStepKey(indexSettings) + "] for index [" + indexMetaData.getIndex().getName()
+                + "] with policy [" + policy + "] is not recognized");
+            return;
         }
         logger.debug("running policy with current-step[" + currentStep.getKey() + "]");
         if (currentStep instanceof TerminalPolicyStep) {
@@ -164,6 +168,23 @@ static Step getCurrentStep(PolicyStepsRegistry stepRegistry, String policy, Sett
         }
     }
 
+    /**
+     * This method is intended for handling moving to different steps from {@link TransportAction} executions.
+     * For this reason, it is reasonable to throw {@link IllegalArgumentException} when state is not as expected.
+     * @param indexName
+     *          The index whose step is to change
+     * @param currentState
+     *          The current {@link ClusterState}
+     * @param currentStepKey
+     *          The current {@link StepKey} found for the index in the current cluster state
+     * @param nextStepKey
+     *          The next step to move the index into
+     * @param nowSupplier
+     *          The current-time supplier for updating when steps changed
+     * @param stepRegistry
+     *          The steps registry to check a step-key's existence in the index's current policy
+     * @return The updated cluster state where the index moved to <code>nextStepKey</code>
+     */
     static ClusterState moveClusterStateToStep(String indexName, ClusterState currentState, StepKey currentStepKey,
                                                StepKey nextStepKey, LongSupplier nowSupplier,
                                                PolicyStepsRegistry stepRegistry) {
@@ -180,10 +201,9 @@ static ClusterState moveClusterStateToStep(String indexName, ClusterState curren
             throw new IllegalArgumentException("index [" + indexName + "] is not on current step [" + currentStepKey + "]");
         }
 
-        try {
-            stepRegistry.getStep(indexPolicySetting, nextStepKey);
-        } catch (IllegalStateException e) {
-            throw new IllegalArgumentException(e.getMessage());
+        Step nextStep = stepRegistry.getStep(indexPolicySetting, nextStepKey);
+        if (nextStep == null) {
+            throw new IllegalArgumentException("step [" + nextStepKey + "] with policy [" + indexPolicySetting + "] does not exist");
         }
 
         return IndexLifecycleRunner.moveClusterStateToNextStep(idxMeta.getIndex(), currentState, currentStepKey, nextStepKey, nowSupplier);
@@ -358,7 +378,7 @@ private static boolean canSetPolicy(StepKey currentStepKey, LifecyclePolicy curr
             return true;
         }
     }
-    
+
     private static boolean isActionChanged(StepKey stepKey, LifecyclePolicy currentPolicy, LifecyclePolicy newPolicy) {
         LifecycleAction currentAction = getActionFromPolicy(currentPolicy, stepKey.getPhase(), stepKey.getAction());
         LifecycleAction newAction = getActionFromPolicy(newPolicy, stepKey.getPhase(), stepKey.getAction());
@@ -385,7 +405,7 @@ private static LifecycleAction getActionFromPolicy(LifecyclePolicy policy, Strin
      * state where it is able to deal with the policy being updated to
      * <code>newPolicy</code>. If any of these indexes is not in a state wheree
      * it can deal with the update the method will return <code>false</code>.
-     * 
+     *
      * @param policyName
      *            the name of the policy being updated
      * @param newPolicy

x-pack/plugin/index-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistry.java

@@ -5,10 +5,12 @@
  */
 package org.elasticsearch.xpack.indexlifecycle;
 
+import org.apache.logging.log4j.Logger;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.Diff;
 import org.elasticsearch.cluster.DiffableUtils;
+import org.elasticsearch.common.logging.ESLoggerFactory;
 import org.elasticsearch.xpack.core.ClientHelper;
 import org.elasticsearch.xpack.core.indexlifecycle.ErrorStep;
 import org.elasticsearch.xpack.core.indexlifecycle.IndexLifecycleMetadata;
@@ -23,6 +25,8 @@
 import java.util.function.LongSupplier;
 
 public class PolicyStepsRegistry {
+    private static final Logger logger = ESLoggerFactory.getLogger(PolicyStepsRegistry.class);
+
     // keeps track of existing policies in the cluster state
     private SortedMap<String, LifecyclePolicyMetadata> lifecyclePolicyMap;
     // keeps track of what the first step in a policy is
@@ -104,13 +108,9 @@ public Step getStep(String policy, Step.StepKey stepKey) {
         }
         Map<Step.StepKey, Step> steps = stepMap.get(policy);
         if (steps == null) {
-            throw new IllegalStateException("policy [" + policy + "] does not exist");
-        }
-        Step step = steps.get(stepKey);
-        if (step == null) {
-            throw new IllegalStateException("step [" + stepKey + "] does not exist");
+            return null;
         }
-        return step;
+        return steps.get(stepKey);
     }
 
     public Step getFirstStep(String policy) {

x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/ExecuteStepsUpdateTaskTests.java

@@ -52,10 +52,12 @@ public class ExecuteStepsUpdateTaskTests extends ESTestCase {
     private static final StepKey firstStepKey = new StepKey("phase_1", "action_1", "step_1");
     private static final StepKey secondStepKey = new StepKey("phase_1", "action_1", "step_2");
     private static final StepKey thirdStepKey = new StepKey("phase_1", "action_1", "step_3");
+    private static final StepKey invalidStepKey = new StepKey("invalid", "invalid", "invalid");
     private ClusterState clusterState;
     private PolicyStepsRegistry policyStepsRegistry;
     private String mixedPolicyName;
     private String allClusterPolicyName;
+    private String invalidPolicyName;
     private Index index;
     private MockClusterStateActionStep firstStep;
     private MockClusterStateWaitStep secondStep;
@@ -75,17 +77,23 @@ public void prepareState() {
         thirdStep = new MockStep(thirdStepKey, null);
         mixedPolicyName = randomAlphaOfLengthBetween(5, 10);
         allClusterPolicyName = randomAlphaOfLengthBetween(1, 4);
+        invalidPolicyName = randomAlphaOfLength(11);
         Phase mixedPhase = new Phase("first_phase", TimeValue.ZERO, Collections.singletonMap(MockAction.NAME,
             new MockAction(Arrays.asList(firstStep, secondStep, thirdStep))));
         Phase allClusterPhase = new Phase("first_phase", TimeValue.ZERO, Collections.singletonMap(MockAction.NAME,
             new MockAction(Arrays.asList(firstStep, allClusterSecondStep))));
+        Phase invalidPhase = new Phase("invalid_phase", TimeValue.ZERO, Collections.singletonMap(MockAction.NAME,
+            new MockAction(Arrays.asList(new MockClusterStateActionStep(firstStepKey, invalidStepKey)))));
         LifecyclePolicy mixedPolicy = new LifecyclePolicy(TestLifecycleType.INSTANCE, mixedPolicyName,
             Collections.singletonMap(mixedPhase.getName(), mixedPhase));
         LifecyclePolicy allClusterPolicy = new LifecyclePolicy(TestLifecycleType.INSTANCE, allClusterPolicyName,
             Collections.singletonMap(allClusterPhase.getName(), allClusterPhase));
+        LifecyclePolicy invalidPolicy = new LifecyclePolicy(TestLifecycleType.INSTANCE, invalidPolicyName,
+            Collections.singletonMap(invalidPhase.getName(), invalidPhase));
         Map<String, LifecyclePolicyMetadata> policyMap = new HashMap<>();
         policyMap.put(mixedPolicyName, new LifecyclePolicyMetadata(mixedPolicy, Collections.emptyMap()));
         policyMap.put(allClusterPolicyName, new LifecyclePolicyMetadata(allClusterPolicy, Collections.emptyMap()));
+        policyMap.put(invalidPolicyName, new LifecyclePolicyMetadata(invalidPolicy, Collections.emptyMap()));
         policyStepsRegistry = new PolicyStepsRegistry();
 
         IndexMetaData indexMetadata = IndexMetaData.builder(randomAlphaOfLength(5))
@@ -165,6 +173,26 @@ public void testExecuteUntilFirstNonClusterStateStep() throws IOException {
         assertThat(LifecycleSettings.LIFECYCLE_STEP_INFO_SETTING.get(newState.metaData().index(index).getSettings()), equalTo(""));
     }
 
+    public void testExecuteInvalidStartStep() throws IOException {
+        setStateToKey(firstStepKey);
+        Step startStep = policyStepsRegistry.getStep(mixedPolicyName, firstStepKey);
+        long now = randomNonNegativeLong();
+        ExecuteStepsUpdateTask task = new ExecuteStepsUpdateTask(invalidPolicyName, index, startStep, policyStepsRegistry, () -> now);
+        ClusterState newState = task.execute(clusterState);
+        assertSame(newState, clusterState);
+
+    }
+
+    public void testExecuteUntilNullStep() throws IOException {
+        setStateToKey(firstStepKey);
+        Step startStep = policyStepsRegistry.getStep(invalidPolicyName, firstStepKey);
+        long now = randomNonNegativeLong();
+        ExecuteStepsUpdateTask task = new ExecuteStepsUpdateTask(invalidPolicyName, index, startStep, policyStepsRegistry, () -> now);
+        ClusterState newState = task.execute(clusterState);
+        StepKey currentStepKey = IndexLifecycleRunner.getCurrentStepKey(newState.metaData().index(index).getSettings());
+        assertThat(currentStepKey, equalTo(invalidStepKey));
+    }
+
     public void testExecuteIncompleteWaitStepNoInfo() throws IOException {
         secondStep.setWillComplete(false);
         setStateToKey(secondStepKey);

x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunnerTests.java

@@ -332,18 +332,13 @@ public void testRunPolicyAsyncWaitStepClusterStateChangeIgnored() {
 
     public void testRunPolicyWithNoStepsInRegistry() {
         String policyName = "cluster_state_action_policy";
-        StepKey stepKey = new StepKey("phase", "action", "cluster_state_action_step");
         ClusterService clusterService = mock(ClusterService.class);
         IndexLifecycleRunner runner = new IndexLifecycleRunner(new PolicyStepsRegistry(), clusterService, () -> 0L);
         IndexMetaData indexMetaData = IndexMetaData.builder("my_index").settings(settings(Version.CURRENT))
             .numberOfShards(randomIntBetween(1, 5)).numberOfReplicas(randomIntBetween(0, 5)).build();
-
-        IllegalStateException exception = expectThrows(IllegalStateException.class,
-            () -> runner.runPolicy(policyName, indexMetaData, null, randomBoolean()));
-        assertEquals("current step for index [my_index] with policy [cluster_state_action_policy] is not recognized",
-            exception.getMessage());
+        // verify that no exception is thrown
+        runner.runPolicy(policyName, indexMetaData, null, randomBoolean());
         Mockito.verifyZeroInteractions(clusterService);
-
     }
 
     public void testRunPolicyUnknownStepType() {
@@ -524,13 +519,8 @@ public void testGetCurrentStep() {
                 .put(LifecycleSettings.LIFECYCLE_ACTION, "action_1")
                 .put(LifecycleSettings.LIFECYCLE_STEP, "step_3")
                 .build();
-        IllegalStateException exception = expectThrows(IllegalStateException.class,
-                () -> IndexLifecycleRunner.getCurrentStep(registry, policyName, invalidIndexSettings));
-        assertEquals("step [{\"phase\":\"phase_1\",\"action\":\"action_1\",\"name\":\"step_3\"}] does not exist", exception.getMessage());
-
-        exception = expectThrows(IllegalStateException.class,
-                () -> IndexLifecycleRunner.getCurrentStep(registry, "policy_does_not_exist", invalidIndexSettings));
-        assertEquals("policy [policy_does_not_exist] does not exist", exception.getMessage());
+        assertNull(IndexLifecycleRunner.getCurrentStep(registry, policyName, invalidIndexSettings));
+        assertNull(IndexLifecycleRunner.getCurrentStep(registry, "policy_does_not_exist", invalidIndexSettings));
     }
 
     public void testMoveClusterStateToNextStep() {
@@ -688,7 +678,8 @@ public void testValidatedMoveClusterStateToNextStepInvalidNextStep() {
             () -> IndexLifecycleRunner.moveClusterStateToStep(indexName, clusterState, currentStepKey,
                 nextStepKey, () -> now, stepRegistry));
         assertThat(exception.getMessage(),
-            equalTo("step [{\"phase\":\"next_phase\",\"action\":\"next_action\",\"name\":\"next_step\"}] does not exist"));
+            equalTo("step [{\"phase\":\"next_phase\",\"action\":\"next_action\",\"name\":\"next_step\"}] " +
+                "with policy [my_policy] does not exist"));
     }
 
     public void testMoveClusterStateToErrorStep() throws IOException {

x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistryTests.java

@@ -79,8 +79,7 @@ public void testGetStepErrorStep() {
     public void testGetStepUnknownPolicy() {
         String policyName = randomAlphaOfLengthBetween(2, 10);
         PolicyStepsRegistry registry = new PolicyStepsRegistry(null, null, Collections.emptyMap());
-        IllegalStateException exception = expectThrows(IllegalStateException.class, () -> registry.getStep(policyName, MOCK_STEP_KEY));
-        assertThat(exception.getMessage(), equalTo("policy [" + policyName +"] does not exist"));
+        assertNull(registry.getStep(policyName, MOCK_STEP_KEY));
     }
 
     public void testGetStepUnknownStepKey() {
@@ -91,8 +90,7 @@ public void testGetStepUnknownStepKey() {
         PolicyStepsRegistry registry = new PolicyStepsRegistry(null, null, stepMap);
         Step.StepKey unknownStepKey = new Step.StepKey(MOCK_STEP_KEY.getPhase(),
             MOCK_STEP_KEY.getAction(),MOCK_STEP_KEY.getName() + "not");
-        IllegalStateException exception = expectThrows(IllegalStateException.class, () -> registry.getStep(policyName, unknownStepKey));
-        assertThat(exception.getMessage(), equalTo("step [" + unknownStepKey +"] does not exist"));
+        assertNull(registry.getStep(policyName, unknownStepKey));
     }
 
     public void testUpdateFromNothingToSomethingToNothing() {

x-pack/plugin/src/test/resources/rest-api-spec/test/index_lifecycle/20_move_to_step.yml

@@ -138,7 +138,7 @@ teardown:
             action: "invalid"
             name: "invalid"
   - match: { error.root_cause.0.type: "illegal_argument_exception" }
-  - match: { error.root_cause.0.reason: "step [{\"phase\":\"invalid\",\"action\":\"invalid\",\"name\":\"invalid\"}] does not exist" }
+  - match: { error.root_cause.0.reason: "step [{\"phase\":\"invalid\",\"action\":\"invalid\",\"name\":\"invalid\"}] with policy [my_moveable_timeseries_lifecycle] does not exist" }
 
   - do:
       acknowledge: true