diff --git a/x-pack/plugin/index-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java b/x-pack/plugin/index-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java index c5de1454f500a..3a1489b974b6f 100644 --- a/x-pack/plugin/index-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java +++ b/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 nextStepKey + */ 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 * newPolicy. If any of these indexes is not in a state wheree * it can deal with the update the method will return false. - * + * * @param policyName * the name of the policy being updated * @param newPolicy diff --git a/x-pack/plugin/index-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistry.java b/x-pack/plugin/index-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistry.java index 502a9a0783e7e..0db59c9fc5ffe 100644 --- a/x-pack/plugin/index-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistry.java +++ b/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 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 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) { diff --git a/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/ExecuteStepsUpdateTaskTests.java b/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/ExecuteStepsUpdateTaskTests.java index 8102d99250d87..233356e743e84 100644 --- a/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/ExecuteStepsUpdateTaskTests.java +++ b/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 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); diff --git a/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunnerTests.java b/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunnerTests.java index e0a4c93e74f97..bb3dbc6d79873 100644 --- a/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunnerTests.java +++ b/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 { diff --git a/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistryTests.java b/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistryTests.java index 22a38702fb173..89d447e737259 100644 --- a/x-pack/plugin/index-lifecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistryTests.java +++ b/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() { diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/index_lifecycle/20_move_to_step.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/index_lifecycle/20_move_to_step.yml index ed04a029bae2a..579df475067d7 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/index_lifecycle/20_move_to_step.yml +++ b/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