Skip to content

stop throwing an IllegalStateException for unrecognized steps #32212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jul 31, 2018
Merged

stop throwing an IllegalStateException for unrecognized steps #32212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5e66362
stop throwing an IllegalStateException when current step not found in…
talevy Jul 19, 2018
3235a1f
Merge branch 'index-lifecycle' into ilm-no-illegal-state
talevy Jul 24, 2018
5a96ade
more nullity instead of throwing exceptions
talevy Jul 24, 2018
f19ba97
Merge branch 'index-lifecycle' into ilm-no-illegal-state
talevy Jul 25, 2018
24ee939
Merge branch 'index-lifecycle' into ilm-no-illegal-state
talevy Jul 25, 2018
81bf84c
fix rest test
talevy Jul 25, 2018
c82a463
Merge branch 'index-lifecycle' into ilm-no-illegal-state
talevy Jul 27, 2018
c009641
update to not throw exception when step not recognized in runner, but…
talevy Jul 27, 2018
460fb34
Revert "update to not throw exception when step not recognized in run…
talevy Jul 30, 2018
76f9245
Merge branch 'index-lifecycle' into ilm-no-illegal-state
talevy Jul 30, 2018
4bb6910
add javadoc for moveClusterStateToStep
talevy Jul 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 28 additions & 8 deletions ...-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -64,8 +65,11 @@ public void runPolicy(String policy, IndexMetaData indexMetaData, ClusterState c
}
Step currentStep = getCurrentStep(stepRegistry, policy, indexSettings);
Copy link
Contributor Author

@talevy talevy Jul 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked closer at this. I missed something here.

This method calls PolicyStepsRegistry#getStep, which chooses to throw IllegalStateExceptions as well. Further investigation to see what the repercussions of changing that is necessary.

and tests need to be added to walk these paths as well. Current tests do not catch this

Copy link
Contributor Author

@talevy talevy Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, should https://github.com/talevy/elasticsearch/blob/80ab7739ca8877e2036edfac724f6014fefb7e52/x-pack/plugin/index-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistry.java#L101-L114 continue to throw an IllegalStateException?

Copy link
Contributor

@colings86 colings86 Jul 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably need to make PolicyStepsRegistry.getStep() return null if the step is missing (in much the same way that a map returns null for a missing key. Then we'll have to check for null:

  • here in IndexLifecycleRunner.runPolicy() and if its null log a warning and return
  • In IndexLifecycleRunner.moveClusterStateToStep() where I think we should do the same as above
  • In ExecuteStepsUpdateTask.execute() where we should log the warning if its null but return the cluster state up to changing the current step so we don't lose all the progress made on the previous cluster state steps

wdyt?

Copy link
Contributor Author

@talevy talevy Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is what I was thinking as well. Just wanted to double check with you before I take that route

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()
Copy link
Contributor Author

@talevy talevy Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I love that we want to provide the step info here, I have mixed feelings about the assertions that exist inside of getCurrentStepKey. I think this should be changed to throwing IllegalStateException. what do you think @colings86?

Copy link
Contributor

@colings86 colings86 Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the assertions are fine because ILM is in full control of the phase, action and step settings (they are INTERNAL settings so can't be touched by a user and will soon be moved to a custom index metadata object further locking down access to them. Therefore, I don't think its necessary to check that if one is set all three are set in production code but its useful to have the check in testing to make sure we don't do something silly so assertions feel right to me.

+ "] with policy [" + policy + "] is not recognized");
return;
}
logger.debug("running policy with current-step[" + currentStep.getKey() + "]");
if (currentStep instanceof TerminalPolicyStep) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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");
Copy link
Contributor

@colings86 colings86 Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why this still needs to throw an exception rather than just returning an unmodified cluster state? Also I know we threw an IllegalArgumentException before but it feels wrong to me since the user won't have supplied anything invalid

Copy link
Contributor Author

@talevy talevy Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am cool with that.

I still feel like IllegalArgumentException should be thrown to users of TransportMoveToStepAction. So I will push up a basic exception for that, and it will know that nothing "right" happened since the clusterStates will be the same instance

}

return IndexLifecycleRunner.moveClusterStateToNextStep(idxMeta.getIndex(), currentState, currentStepKey, nextStepKey, nowSupplier);
Expand Down Expand Up @@ -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());
Expand All @@ -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
Expand Down
12 changes: 6 additions & 6 deletions ...x-lifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
28 changes: 28 additions & 0 deletions ...cle/src/test/java/org/elasticsearch/xpack/indexlifecycle/ExecuteStepsUpdateTaskTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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))
Expand Down Expand Up @@ -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);
Expand Down
21 changes: 6 additions & 15 deletions ...cycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunnerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 2 additions & 4 deletions ...ecycle/src/test/java/org/elasticsearch/xpack/indexlifecycle/PolicyStepsRegistryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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() {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/src/test/resources/rest-api-spec/test/index_lifecycle/20_move_to_step.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down