[7.x] Refactor IndexLifecycleRunner to split state modificatio… (elastic#49936)

This commit refactors the `IndexLifecycleRunner` to split out and
consolidate the number of methods that change state from within ILM. It
adds a new class `IndexLifecycleTransition` that contains a number of
static methods used to modify ILM's state. These methods all return new
cluster states rather than making changes themselves (they can be
thought of as helpers for modifying ILM state).

Rather than having multiple ways to move an index to a particular step
(like `moveClusterStateToStep`, `moveClusterStateToNextStep`,
`moveClusterStateToPreviouslyFailedStep`, etc (there are others)) this
now consolidates those into three with (hopefully) useful names:

- `moveClusterStateToStep`
- `moveClusterStateToErrorStep`
- `moveClusterStateToPreviouslyFailedStep`

In the move, I was also able to consolidate duplicate or redundant
arguments to these functions. Prior to this commit there were many calls
that provided duplicate information (both `IndexMetaData` and
`LifecycleExecutionState` for example) where the duplicate argument
could be derived from a previous argument with no problems.

With this split, `IndexLifecycleRunner` now contains the methods used to
actually run steps as well as the methods that kick off cluster state
updates for state transitions. `IndexLifecycleTransition` contains only
the helpers for constructing new states from given scenarios.

This also adds Javadocs to all methods in both `IndexLifecycleRunner`
and `IndexLifecycleTransition` (this accounts for almost all of the
increase in code lines for this commit). It also makes all methods be as
restrictive in visibility, to limit the scope of where they are used.

This refactoring is part of work towards capturing actions and
transitions that ILM makes, by consolidating and simplifying the places
we make state changes, it will make adding operation auditing easier.

Author: dakrone

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

@@ -8,6 +8,8 @@
 
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
+import org.elasticsearch.common.Nullable;
+import org.elasticsearch.common.Strings;
 
 import java.util.Collections;
 import java.util.HashMap;
@@ -78,6 +80,31 @@ public static LifecycleExecutionState fromIndexMetadata(IndexMetaData indexMetaD
         return fromCustomMetadata(customData);
     }
 
+    /**
+     * Retrieves the current {@link Step.StepKey} from the lifecycle state. Note that
+     * it is illegal for the step to be set with the phase and/or action unset,
+     * or for the step to be unset with the phase and/or action set. All three
+     * settings must be either present or missing.
+     *
+     * @param lifecycleState the index custom data to extract the {@link Step.StepKey} from.
+     */
+    @Nullable
+    public static Step.StepKey getCurrentStepKey(LifecycleExecutionState lifecycleState) {
+        Objects.requireNonNull(lifecycleState, "cannot determine current step key as lifecycle state is null");
+        String currentPhase = lifecycleState.getPhase();
+        String currentAction = lifecycleState.getAction();
+        String currentStep = lifecycleState.getStep();
+        if (Strings.isNullOrEmpty(currentStep)) {
+            assert Strings.isNullOrEmpty(currentPhase) : "Current phase is not empty: " + currentPhase;
+            assert Strings.isNullOrEmpty(currentAction) : "Current action is not empty: " + currentAction;
+            return null;
+        } else {
+            assert Strings.isNullOrEmpty(currentPhase) == false;
+            assert Strings.isNullOrEmpty(currentAction) == false;
+            return new Step.StepKey(currentPhase, currentAction, currentStep);
+        }
+    }
+
     public static Builder builder() {
         return new Builder();
     }
@@ -278,6 +305,11 @@ public int hashCode() {
             getStepInfo(), getPhaseDefinition(), getLifecycleDate(), getPhaseTime(), getActionTime(), getStepTime());
     }
 
+    @Override
+    public String toString() {
+        return asMap().toString();
+    }
+
     public static class Builder {
         private String phase;
         private String action;

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

@@ -71,6 +71,69 @@ public void testEqualsAndHashcode() {
             LifecycleExecutionStateTests::mutate);
     }
 
+    public void testGetCurrentStepKey() {
+        LifecycleExecutionState.Builder lifecycleState = LifecycleExecutionState.builder();
+        Step.StepKey stepKey = LifecycleExecutionState.getCurrentStepKey(lifecycleState.build());
+        assertNull(stepKey);
+
+        String phase = randomAlphaOfLength(20);
+        String action = randomAlphaOfLength(20);
+        String step = randomAlphaOfLength(20);
+        LifecycleExecutionState.Builder lifecycleState2 = LifecycleExecutionState.builder();
+        lifecycleState2.setPhase(phase);
+        lifecycleState2.setAction(action);
+        lifecycleState2.setStep(step);
+        stepKey = LifecycleExecutionState.getCurrentStepKey(lifecycleState2.build());
+        assertNotNull(stepKey);
+        assertEquals(phase, stepKey.getPhase());
+        assertEquals(action, stepKey.getAction());
+        assertEquals(step, stepKey.getName());
+
+        phase = randomAlphaOfLength(20);
+        action = randomAlphaOfLength(20);
+        step = null;
+        LifecycleExecutionState.Builder lifecycleState3 = LifecycleExecutionState.builder();
+        lifecycleState3.setPhase(phase);
+        lifecycleState3.setAction(action);
+        lifecycleState3.setStep(step);
+        AssertionError error3 = expectThrows(AssertionError.class,
+            () -> LifecycleExecutionState.getCurrentStepKey(lifecycleState3.build()));
+        assertEquals("Current phase is not empty: " + phase, error3.getMessage());
+
+        phase = null;
+        action = randomAlphaOfLength(20);
+        step = null;
+        LifecycleExecutionState.Builder lifecycleState4 = LifecycleExecutionState.builder();
+        lifecycleState4.setPhase(phase);
+        lifecycleState4.setAction(action);
+        lifecycleState4.setStep(step);
+        AssertionError error4 = expectThrows(AssertionError.class,
+            () -> LifecycleExecutionState.getCurrentStepKey(lifecycleState4.build()));
+        assertEquals("Current action is not empty: " + action, error4.getMessage());
+
+        phase = null;
+        action = randomAlphaOfLength(20);
+        step = randomAlphaOfLength(20);
+        LifecycleExecutionState.Builder lifecycleState5 = LifecycleExecutionState.builder();
+        lifecycleState5.setPhase(phase);
+        lifecycleState5.setAction(action);
+        lifecycleState5.setStep(step);
+        AssertionError error5 = expectThrows(AssertionError.class,
+            () -> LifecycleExecutionState.getCurrentStepKey(lifecycleState5.build()));
+        assertNull(error5.getMessage());
+
+        phase = null;
+        action = null;
+        step = randomAlphaOfLength(20);
+        LifecycleExecutionState.Builder lifecycleState6 = LifecycleExecutionState.builder();
+        lifecycleState6.setPhase(phase);
+        lifecycleState6.setAction(action);
+        lifecycleState6.setStep(step);
+        AssertionError error6 = expectThrows(AssertionError.class,
+            () -> LifecycleExecutionState.getCurrentStepKey(lifecycleState6.build()));
+        assertNull(error6.getMessage());
+    }
+
     private static LifecycleExecutionState mutate(LifecycleExecutionState toMutate) {
         LifecycleExecutionState.Builder newState = LifecycleExecutionState.builder(toMutate);
         boolean changed = false;

x-pack/plugin/ilm/qa/rest/src/test/resources/rest-api-spec/test/ilm/40_explain_lifecycle.yml

@@ -115,9 +115,9 @@ teardown:
   - match: { indices.my_index.step: "complete" }
   - is_true: indices.my_index.phase_time_millis
   - is_true: indices.my_index.age
+  - is_true: indices.my_index.phase_execution
   - is_false: indices.my_index.failed_step
   - is_false: indices.my_index.step_info
-  - is_false: indices.my_index.phase_execution
 
   - is_false: indices.my_index2
   - is_false: indices.another_index
@@ -139,9 +139,9 @@ teardown:
   - match: { indices.my_index.step: "complete" }
   - is_true: indices.my_index.phase_time_millis
   - is_true: indices.my_index.age
+  - is_true: indices.my_index.phase_execution
   - is_false: indices.my_index.failed_step
   - is_false: indices.my_index.step_info
-  - is_false: indices.my_index.phase_execution
 
   - is_true: indices.my_index2.managed
   - match: { indices.my_index2.index: "my_index2" }
@@ -151,9 +151,9 @@ teardown:
   - match: { indices.my_index2.step: "complete" }
   - is_true: indices.my_index2.phase_time_millis
   - is_true: indices.my_index2.age
+  - is_true: indices.my_index2.phase_execution
   - is_false: indices.my_index2.failed_step
   - is_false: indices.my_index2.step_info
-  - is_false: indices.my_index2.phase_execution
 
   - is_false: indices.another_index
   - is_false: indices.unmanaged_index
@@ -178,9 +178,9 @@ teardown:
   - match: { indices.my_index.step: "complete" }
   - is_true: indices.my_index.phase_time_millis
   - is_true: indices.my_index.age
+  - is_true: indices.my_index.phase_execution
   - is_false: indices.my_index.failed_step
   - is_false: indices.my_index.step_info
-  - is_false: indices.my_index.phase_execution
 
   - is_true: indices.my_index2.managed
   - match: { indices.my_index2.index: "my_index2" }
@@ -190,9 +190,9 @@ teardown:
   - match: { indices.my_index2.step: "complete" }
   - is_true: indices.my_index2.phase_time_millis
   - is_true: indices.my_index2.age
+  - is_true: indices.my_index2.phase_execution
   - is_false: indices.my_index2.failed_step
   - is_false: indices.my_index2.step_info
-  - is_false: indices.my_index2.phase_execution
 
   - is_true: indices.another_index.managed
   - match: { indices.another_index.index: "another_index" }
@@ -202,9 +202,9 @@ teardown:
   - match: { indices.another_index.step: "complete" }
   - is_true: indices.another_index.phase_time_millis
   - is_true: indices.another_index.age
+  - is_true: indices.another_index.phase_execution
   - is_false: indices.another_index.failed_step
   - is_false: indices.another_index.step_info
-  - is_false: indices.another_index.phase_execution
 
   - match: { indices.unmanaged_index.index: "unmanaged_index" }
   - is_false: indices.unmanaged_index.managed

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

@@ -16,7 +16,6 @@
 import org.elasticsearch.index.Index;
 import org.elasticsearch.xpack.core.ilm.ClusterStateActionStep;
 import org.elasticsearch.xpack.core.ilm.ClusterStateWaitStep;
-import org.elasticsearch.xpack.core.ilm.LifecycleExecutionState;
 import org.elasticsearch.xpack.core.ilm.Step;
 import org.elasticsearch.xpack.core.ilm.TerminalPolicyStep;
 
@@ -78,8 +77,7 @@ public ClusterState execute(final ClusterState currentState) throws IOException
             // This index doesn't exist any more, there's nothing to execute currently
             return currentState;
         }
-        Step registeredCurrentStep = IndexLifecycleRunner.getCurrentStep(policyStepsRegistry, policy, indexMetaData,
-            LifecycleExecutionState.fromIndexMetadata(indexMetaData));
+        Step registeredCurrentStep = IndexLifecycleRunner.getCurrentStep(policyStepsRegistry, policy, indexMetaData);
         if (currentStep.equals(registeredCurrentStep)) {
             ClusterState state = currentState;
             // We can do cluster state steps all together until we
@@ -103,8 +101,8 @@ public ClusterState execute(final ClusterState currentState) throws IOException
                         return state;
                     } else {
                         logger.trace("[{}] moving cluster state to next step [{}]", index.getName(), nextStepKey);
-                        state = IndexLifecycleRunner.moveClusterStateToNextStep(index, state, currentStep.getKey(),
-                            nextStepKey, nowSupplier, false);
+                        state = IndexLifecycleTransition.moveClusterStateToStep(index, state, nextStepKey, nowSupplier,
+                            policyStepsRegistry, false);
                     }
                 } else {
                     // set here to make sure that the clusterProcessed knows to execute the
@@ -130,8 +128,8 @@ public ClusterState execute(final ClusterState currentState) throws IOException
                         if (currentStep.getNextStepKey() == null) {
                             return state;
                         } else {
-                            state = IndexLifecycleRunner.moveClusterStateToNextStep(index, state, currentStep.getKey(),
-                                currentStep.getNextStepKey(), nowSupplier, false);
+                            state = IndexLifecycleTransition.moveClusterStateToStep(index, state,
+                                currentStep.getNextStepKey(), nowSupplier, policyStepsRegistry,false);
                         }
                     } else {
                         logger.trace("[{}] condition not met ({}) [{}], returning existing state",
@@ -145,7 +143,7 @@ public ClusterState execute(final ClusterState currentState) throws IOException
                         if (stepInfo == null) {
                             return state;
                         } else {
-                            return IndexLifecycleRunner.addStepInfoToClusterState(index, state, stepInfo);
+                            return IndexLifecycleTransition.addStepInfoToClusterState(index, state, stepInfo);
                         }
                     }
                 }