Skip to content

Adds a check to only fail policy update if unsafe action is changed #32002

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 1 commit into from
Jul 12, 2018
Merged

Adds a check to only fail policy update if unsafe action is changed #32002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 23 additions & 3 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 @@ -28,8 +28,10 @@
import org.elasticsearch.xpack.core.indexlifecycle.ClusterStateWaitStep;
import org.elasticsearch.xpack.core.indexlifecycle.ErrorStep;
import org.elasticsearch.xpack.core.indexlifecycle.IndexLifecycleMetadata;
import org.elasticsearch.xpack.core.indexlifecycle.LifecycleAction;
import org.elasticsearch.xpack.core.indexlifecycle.LifecyclePolicy;
import org.elasticsearch.xpack.core.indexlifecycle.LifecycleSettings;
import org.elasticsearch.xpack.core.indexlifecycle.Phase;
import org.elasticsearch.xpack.core.indexlifecycle.RolloverAction;
import org.elasticsearch.xpack.core.indexlifecycle.Step;
import org.elasticsearch.xpack.core.indexlifecycle.Step.StepKey;
Expand Down Expand Up @@ -340,15 +342,33 @@ private static boolean canSetPolicy(StepKey currentStepKey, LifecyclePolicy curr
if (currentPolicy.isActionSafe(currentStepKey)) {
return true;
} else {
// Index is in the shrink action so fail it
// NORELEASE also need to check if the shrink action has changed between oldPolicy and newPolicy
return false;
// Index is in an unsafe action so fail it if the action has changed between oldPolicy and newPolicy
return isActionChanged(currentStepKey, currentPolicy, newPolicy) == false;
}
} else {
// Index not previously managed by ILM so safe to change policy
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());
if (newAction == null) {
return true;
} else {
return currentAction.equals(newAction) == false;
Copy link
Contributor

Choose a reason for hiding this comment

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

since getActionFromPolicy can, theoretically, return null, would it be safer to switch these around to

newAction.equals(currentAction) == false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right but if currentAction is null then we have a bug because we should never be in a situation where the current step doesn't exist in the current policy. If this does happen I would rather not silently fail returning a value. If this does not seem very friendly to you I can add a null check for currentAction which throws and IllegalStateException instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that would make things super clear, but you're right, it would be a bug... so tossing a stacktrace vs. an illegalstateexception is almost the same thing 😄. all good

}
}

private static LifecycleAction getActionFromPolicy(LifecyclePolicy policy, String phaseName, String actionName) {
Phase phase = policy.getPhases().get(phaseName);
if (phase != null) {
return phase.getActions().get(actionName);
} else {
return null;
}
}

/**
* Returns <code>true</code> if the provided policy is allowed to be updated
Expand Down
111 changes: 111 additions & 0 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 @@ -938,6 +938,67 @@ public void testSetPolicyForIndexIndexInUnsafe() {
assertSame(clusterState, newClusterState);
}

public void testSetPolicyForIndexIndexInUnsafeActionUnchanged() {
long now = randomNonNegativeLong();
String indexName = randomAlphaOfLength(10);
String oldPolicyName = "old_policy";
String newPolicyName = "new_policy";
StepKey currentStep = new StepKey(randomAlphaOfLength(10), MockAction.NAME, randomAlphaOfLength(10));
LifecyclePolicy oldPolicy = createPolicy(oldPolicyName, null, currentStep);
LifecyclePolicy newPolicy = createPolicy(newPolicyName, null, currentStep);
Settings.Builder indexSettingsBuilder = Settings.builder().put(LifecycleSettings.LIFECYCLE_NAME, oldPolicyName)
.put(LifecycleSettings.LIFECYCLE_PHASE, currentStep.getPhase())
.put(LifecycleSettings.LIFECYCLE_ACTION, currentStep.getAction())
.put(LifecycleSettings.LIFECYCLE_STEP, currentStep.getName()).put(LifecycleSettings.LIFECYCLE_SKIP, true);
List<LifecyclePolicyMetadata> policyMetadatas = new ArrayList<>();
policyMetadatas.add(new LifecyclePolicyMetadata(oldPolicy, Collections.emptyMap()));
ClusterState clusterState = buildClusterState(indexName, indexSettingsBuilder, policyMetadatas);
Index index = clusterState.metaData().index(indexName).getIndex();
Index[] indices = new Index[] { index };
List<String> failedIndexes = new ArrayList<>();

ClusterState newClusterState = IndexLifecycleRunner.setPolicyForIndexes(newPolicyName, indices, clusterState, newPolicy,
failedIndexes);

assertTrue(failedIndexes.isEmpty());
assertClusterStateOnPolicy(clusterState, index, newPolicyName, currentStep, currentStep, newClusterState, now);
}

public void testSetPolicyForIndexIndexInUnsafeActionChanged() {
String indexName = randomAlphaOfLength(10);
String oldPolicyName = "old_policy";
String newPolicyName = "new_policy";
StepKey currentStep = new StepKey(randomAlphaOfLength(10), MockAction.NAME, randomAlphaOfLength(10));
LifecyclePolicy oldPolicy = createPolicy(oldPolicyName, null, currentStep);

// change the current action so its not equal to the old one by adding a step
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also add a change here to remove the action from the policy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean as a separate test? The existing Unsafe tests actually already do this as the new policy we create is empty

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I missed this. thanks

Map<String, Phase> phases = new HashMap<>();
Map<String, LifecycleAction> actions = new HashMap<>();
MockAction unsafeAction = new MockAction(Collections.singletonList(new MockStep(currentStep, null)), false);
actions.put(unsafeAction.getWriteableName(), unsafeAction);
Phase phase = new Phase(currentStep.getPhase(), TimeValue.timeValueMillis(0), actions);
phases.put(phase.getName(), phase);
LifecyclePolicy newPolicy = new LifecyclePolicy(TestLifecycleType.INSTANCE, newPolicyName, phases);

Settings.Builder indexSettingsBuilder = Settings.builder().put(LifecycleSettings.LIFECYCLE_NAME, oldPolicyName)
.put(LifecycleSettings.LIFECYCLE_PHASE, currentStep.getPhase())
.put(LifecycleSettings.LIFECYCLE_ACTION, currentStep.getAction())
.put(LifecycleSettings.LIFECYCLE_STEP, currentStep.getName()).put(LifecycleSettings.LIFECYCLE_SKIP, true);
List<LifecyclePolicyMetadata> policyMetadatas = new ArrayList<>();
policyMetadatas.add(new LifecyclePolicyMetadata(oldPolicy, Collections.emptyMap()));
ClusterState clusterState = buildClusterState(indexName, indexSettingsBuilder, policyMetadatas);
Index index = clusterState.metaData().index(indexName).getIndex();
Index[] indices = new Index[] { index };
List<String> failedIndexes = new ArrayList<>();

ClusterState newClusterState = IndexLifecycleRunner.setPolicyForIndexes(newPolicyName, indices, clusterState, newPolicy,
failedIndexes);

assertEquals(1, failedIndexes.size());
assertEquals(index.getName(), failedIndexes.get(0));
assertSame(clusterState, newClusterState);
}

private static LifecyclePolicy createPolicy(String policyName, StepKey safeStep, StepKey unsafeStep) {
Map<String, Phase> phases = new HashMap<>();
if (safeStep != null) {
Expand Down Expand Up @@ -1002,6 +1063,56 @@ public void testCanUpdatePolicyIndexInUnsafe() {
assertFalse(canUpdatePolicy);
}

public void testCanUpdatePolicyIndexInUnsafeActionUnchanged() {
String indexName = randomAlphaOfLength(10);
String oldPolicyName = "old_policy";
String newPolicyName = "new_policy";
StepKey currentStep = new StepKey(randomAlphaOfLength(10), MockAction.NAME, randomAlphaOfLength(10));
LifecyclePolicy oldPolicy = createPolicy(oldPolicyName, null, currentStep);
LifecyclePolicy newPolicy = createPolicy(newPolicyName,
new StepKey(randomAlphaOfLength(10), MockAction.NAME, randomAlphaOfLength(10)), currentStep);
Settings.Builder indexSettingsBuilder = Settings.builder().put(LifecycleSettings.LIFECYCLE_NAME, oldPolicyName)
.put(LifecycleSettings.LIFECYCLE_PHASE, currentStep.getPhase())
.put(LifecycleSettings.LIFECYCLE_ACTION, currentStep.getAction())
.put(LifecycleSettings.LIFECYCLE_STEP, currentStep.getName()).put(LifecycleSettings.LIFECYCLE_SKIP, true);
List<LifecyclePolicyMetadata> policyMetadatas = new ArrayList<>();
policyMetadatas.add(new LifecyclePolicyMetadata(oldPolicy, Collections.emptyMap()));
ClusterState clusterState = buildClusterState(indexName, indexSettingsBuilder, policyMetadatas);

boolean canUpdatePolicy = IndexLifecycleRunner.canUpdatePolicy(oldPolicyName, newPolicy, clusterState);

assertTrue(canUpdatePolicy);
}

public void testCanUpdatePolicyIndexInUnsafeActionChanged() {
String indexName = randomAlphaOfLength(10);
String oldPolicyName = "old_policy";
String newPolicyName = "new_policy";
StepKey currentStep = new StepKey(randomAlphaOfLength(10), MockAction.NAME, randomAlphaOfLength(10));
LifecyclePolicy oldPolicy = createPolicy(oldPolicyName, null, currentStep);

// change the current action so its not equal to the old one by adding a step
Map<String, Phase> phases = new HashMap<>();
Map<String, LifecycleAction> actions = new HashMap<>();
MockAction unsafeAction = new MockAction(Collections.singletonList(new MockStep(currentStep, null)), false);
actions.put(unsafeAction.getWriteableName(), unsafeAction);
Phase phase = new Phase(currentStep.getPhase(), TimeValue.timeValueMillis(0), actions);
phases.put(phase.getName(), phase);
LifecyclePolicy newPolicy = new LifecyclePolicy(TestLifecycleType.INSTANCE, newPolicyName, phases);

Settings.Builder indexSettingsBuilder = Settings.builder().put(LifecycleSettings.LIFECYCLE_NAME, oldPolicyName)
.put(LifecycleSettings.LIFECYCLE_PHASE, currentStep.getPhase())
.put(LifecycleSettings.LIFECYCLE_ACTION, currentStep.getAction())
.put(LifecycleSettings.LIFECYCLE_STEP, currentStep.getName()).put(LifecycleSettings.LIFECYCLE_SKIP, true);
List<LifecyclePolicyMetadata> policyMetadatas = new ArrayList<>();
policyMetadatas.add(new LifecyclePolicyMetadata(oldPolicy, Collections.emptyMap()));
ClusterState clusterState = buildClusterState(indexName, indexSettingsBuilder, policyMetadatas);

boolean canUpdatePolicy = IndexLifecycleRunner.canUpdatePolicy(oldPolicyName, newPolicy, clusterState);

assertFalse(canUpdatePolicy);
}

public void testCanUpdatePolicyIndexNotManaged() {
String indexName = randomAlphaOfLength(10);
String oldPolicyName = "old_policy";
Expand Down