From 7c4f1c2535326367b8277e0325311434d986a462 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Mon, 7 Jun 2021 09:03:52 -0600 Subject: [PATCH] Copy all execution state in CopyExecutionStateStep (#73792) This changes the CopyExecutionStateStep to copy all of the existing state from the original to the new index after a shrink or searchable snapshot ILM action. It also has two other changes First, it fixes issues in LifecycleExecutionState.equals and hashCode where parts were missing, and modifies the tests better so they would catch these issues better. Second, it moves the lifecycle policy validation out of the constructor to a new method. This is required because PolicyStepsRegistry constructs a new LifecyclePolicy out of the cached phase JSON combined with the policy from the cluster state. In the event that the cached JSON contains an action that prevents subsequent actions from being defined, we need to leniently allow this behavior, because those steps will be ignored (see #64883 for this original implementation). The policy is still validated when loaded elsewhere however. Resolves #73791 # Conflicts: # x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java --- .../core/ilm/CopyExecutionStateStep.java | 10 +- .../core/ilm/LifecycleExecutionState.java | 4 +- .../xpack/core/ilm/LifecyclePolicy.java | 3 + .../xpack/core/ilm/LifecyclePolicyUtils.java | 4 +- .../core/ilm/action/PutLifecycleAction.java | 1 + .../core/ilm/CopyExecutionStateStepTests.java | 38 +++++++ .../ilm/LifecycleExecutionStateTests.java | 107 ++++++++++-------- .../actions/SearchableSnapshotActionIT.java | 7 +- 8 files changed, 117 insertions(+), 57 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CopyExecutionStateStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CopyExecutionStateStep.java index a61b6eb51d0fb..ffaf02b1513d3 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CopyExecutionStateStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/CopyExecutionStateStep.java @@ -80,15 +80,11 @@ public ClusterState performAction(Index index, ClusterState clusterState) { String step = targetNextStepKey.getName(); long lifecycleDate = lifecycleState.getLifecycleDate(); - LifecycleExecutionState.Builder relevantTargetCustomData = LifecycleExecutionState.builder(); - relevantTargetCustomData.setIndexCreationDate(lifecycleDate); - relevantTargetCustomData.setAction(action); + LifecycleExecutionState.Builder relevantTargetCustomData = LifecycleExecutionState.builder(lifecycleState); + // Override the phase, action, and step for the target next StepKey relevantTargetCustomData.setPhase(phase); + relevantTargetCustomData.setAction(action); relevantTargetCustomData.setStep(step); - relevantTargetCustomData.setSnapshotRepository(lifecycleState.getSnapshotRepository()); - relevantTargetCustomData.setSnapshotName(lifecycleState.getSnapshotName()); - relevantTargetCustomData.setSnapshotIndexName(lifecycleState.getSnapshotIndexName()); - relevantTargetCustomData.setShrinkIndexName(lifecycleState.getShrinkIndexName()); Metadata.Builder newMetadata = Metadata.builder(clusterState.getMetadata()) .put(IndexMetadata.builder(targetIndexMetadata) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionState.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionState.java index f74f04b85230a..687420afc3439 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionState.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionState.java @@ -371,6 +371,8 @@ public boolean equals(Object o) { Objects.equals(getSnapshotRepository(), that.getSnapshotRepository()) && Objects.equals(getSnapshotName(), that.getSnapshotName()) && Objects.equals(getSnapshotIndexName(), that.getSnapshotIndexName()) && + Objects.equals(getShrinkIndexName(), that.getShrinkIndexName()) && + Objects.equals(getRollupIndexName(), that.getRollupIndexName()) && Objects.equals(getPhaseDefinition(), that.getPhaseDefinition()); } @@ -378,7 +380,7 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(getPhase(), getAction(), getStep(), getFailedStep(), isAutoRetryableError(), getFailedStepRetryCount(), getStepInfo(), getPhaseDefinition(), getLifecycleDate(), getPhaseTime(), getActionTime(), getStepTime(), - getSnapshotRepository(), getSnapshotName(), getSnapshotIndexName()); + getSnapshotRepository(), getSnapshotName(), getSnapshotIndexName(), getShrinkIndexName(), getRollupIndexName()); } @Override diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicy.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicy.java index b498719043a53..f313c44789076 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicy.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicy.java @@ -122,6 +122,9 @@ public LifecyclePolicy(LifecycleType type, String name, Map phase this.phases = phases; this.type = type; this.metadata = metadata; + } + + public void validate() { this.type.validate(phases.values()); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyUtils.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyUtils.java index 3c556788c4e86..5ddc1592f3248 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyUtils.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyUtils.java @@ -39,7 +39,9 @@ public static LifecyclePolicy loadPolicy(String name, String resource, NamedXCon try (XContentParser parser = XContentType.JSON.xContent() .createParser(xContentRegistry, LoggingDeprecationHandler.THROW_UNSUPPORTED_OPERATION, source.utf8ToString())) { - return LifecyclePolicy.parse(parser, name); + LifecyclePolicy policy = LifecyclePolicy.parse(parser, name); + policy.validate(); + return policy; } } catch (Exception e) { throw new IllegalArgumentException("unable to load policy [" + name + "] from [" + resource + "]", e); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/action/PutLifecycleAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/action/PutLifecycleAction.java index 38cd0c1f86de5..7d2de184971fa 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/action/PutLifecycleAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/action/PutLifecycleAction.java @@ -61,6 +61,7 @@ public LifecyclePolicy getPolicy() { @Override public ActionRequestValidationException validate() { + this.policy.validate(); ActionRequestValidationException err = null; String phaseTimingErr = TimeseriesLifecycleType.validateMonotonicallyIncreasingPhaseTimings(this.policy.getPhases().values()); if (Strings.hasText(phaseTimingErr)) { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/CopyExecutionStateStepTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/CopyExecutionStateStepTests.java index 36e13677337b4..c232029ec635a 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/CopyExecutionStateStepTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/CopyExecutionStateStepTests.java @@ -14,6 +14,7 @@ import org.elasticsearch.cluster.metadata.Metadata; import org.elasticsearch.xpack.core.ilm.Step.StepKey; +import java.util.HashMap; import java.util.Map; import java.util.function.BiFunction; @@ -102,6 +103,43 @@ public void testPerformAction() { assertEquals(newIndexData.getSnapshotName(), oldIndexData.getSnapshotName()); } + public void testAllStateCopied() { + CopyExecutionStateStep step = createRandomInstance(); + String indexName = randomAlphaOfLengthBetween(5, 20); + + IndexMetadata originalIndexMetadata = IndexMetadata.builder(indexName) + .settings(settings(Version.CURRENT)).numberOfShards(randomIntBetween(1,5)) + .numberOfReplicas(randomIntBetween(1,5)) + .putCustom(ILM_CUSTOM_METADATA_KEY, createCustomMetadata()) + .build(); + IndexMetadata shrunkIndexMetadata = + IndexMetadata.builder(step.getTargetIndexNameSupplier().apply(indexName, LifecycleExecutionState.builder().build())) + .settings(settings(Version.CURRENT)).numberOfShards(randomIntBetween(1,5)) + .numberOfReplicas(randomIntBetween(1,5)) + .build(); + + ClusterState originalClusterState = ClusterState.builder(ClusterName.DEFAULT) + .metadata(Metadata.builder() + .put(originalIndexMetadata, false) + .put(shrunkIndexMetadata, false)) + .build(); + + ClusterState newClusterState = step.performAction(originalIndexMetadata.getIndex(), originalClusterState); + + LifecycleExecutionState oldIndexData = LifecycleExecutionState.fromIndexMetadata(originalIndexMetadata); + LifecycleExecutionState newIndexData = LifecycleExecutionState + .fromIndexMetadata(newClusterState.metadata().index(step.getTargetIndexNameSupplier().apply(indexName, + LifecycleExecutionState.builder().build()))); + + Map beforeMap = new HashMap<>(oldIndexData.asMap()); + // The target step key's StepKey is used in the new metadata, so update the "before" map with the new info so it can be compared + beforeMap.put("phase", step.getTargetNextStepKey().getPhase()); + beforeMap.put("action", step.getTargetNextStepKey().getAction()); + beforeMap.put("step", step.getTargetNextStepKey().getName()); + Map newMap = newIndexData.asMap(); + assertThat(beforeMap, equalTo(newMap)); + } + public void testPerformActionWithNoTarget() { CopyExecutionStateStep step = createRandomInstance(); String indexName = randomAlphaOfLengthBetween(5, 20); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionStateTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionStateTests.java index 851f02ba70efb..ecf300124d166 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionStateTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionStateTests.java @@ -137,52 +137,65 @@ public void testGetCurrentStepKey() { private static LifecycleExecutionState mutate(LifecycleExecutionState toMutate) { LifecycleExecutionState.Builder newState = LifecycleExecutionState.builder(toMutate); - boolean changed = false; - if (randomBoolean()) { - newState.setPhase(randomValueOtherThan(toMutate.getPhase(), () -> randomAlphaOfLengthBetween(5, 20))); - changed = true; + switch (randomIntBetween(0, 17)) { + case 0: + newState.setPhase(randomValueOtherThan(toMutate.getPhase(), () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 1: + newState.setAction(randomValueOtherThan(toMutate.getAction(), () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 2: + newState.setStep(randomValueOtherThan(toMutate.getStep(), () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 3: + newState.setPhaseDefinition(randomValueOtherThan(toMutate.getPhaseDefinition(), () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 4: + newState.setFailedStep(randomValueOtherThan(toMutate.getFailedStep(), () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 5: + newState.setStepInfo(randomValueOtherThan(toMutate.getStepInfo(), () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 6: + newState.setPhaseTime(randomValueOtherThan(toMutate.getPhaseTime(), ESTestCase::randomLong)); + break; + case 7: + newState.setActionTime(randomValueOtherThan(toMutate.getActionTime(), ESTestCase::randomLong)); + break; + case 8: + newState.setStepTime(randomValueOtherThan(toMutate.getStepTime(), ESTestCase::randomLong)); + break; + case 9: + newState.setIndexCreationDate(randomValueOtherThan(toMutate.getLifecycleDate(), ESTestCase::randomLong)); + break; + case 10: + newState.setShrinkIndexName(randomValueOtherThan(toMutate.getShrinkIndexName(), () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 11: + newState.setSnapshotRepository(randomValueOtherThan(toMutate.getSnapshotRepository(), + () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 12: + newState.setSnapshotIndexName(randomValueOtherThan(toMutate.getSnapshotIndexName(), + () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 13: + newState.setSnapshotName(randomValueOtherThan(toMutate.getSnapshotName(), () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 14: + newState.setRollupIndexName(randomValueOtherThan(toMutate.getRollupIndexName(), () -> randomAlphaOfLengthBetween(5, 20))); + break; + case 15: + newState.setIsAutoRetryableError(randomValueOtherThan(toMutate.isAutoRetryableError(), ESTestCase::randomBoolean)); + break; + case 16: + newState.setFailedStepRetryCount(randomValueOtherThan(toMutate.getFailedStepRetryCount(), ESTestCase::randomInt)); + break; + case 17: + return LifecycleExecutionState.builder().build(); + default: + throw new IllegalStateException("unknown randomization branch"); } - if (randomBoolean()) { - newState.setAction(randomValueOtherThan(toMutate.getAction(), () -> randomAlphaOfLengthBetween(5, 20))); - changed = true; - } - if (randomBoolean()) { - newState.setStep(randomValueOtherThan(toMutate.getStep(), () -> randomAlphaOfLengthBetween(5, 20))); - changed = true; - } - if (randomBoolean()) { - newState.setPhaseDefinition(randomValueOtherThan(toMutate.getPhaseDefinition(), () -> randomAlphaOfLengthBetween(5, 20))); - changed = true; - } - if (randomBoolean()) { - newState.setFailedStep(randomValueOtherThan(toMutate.getFailedStep(), () -> randomAlphaOfLengthBetween(5, 20))); - changed = true; - } - if (randomBoolean()) { - newState.setStepInfo(randomValueOtherThan(toMutate.getStepInfo(), () -> randomAlphaOfLengthBetween(5, 20))); - changed = true; - } - if (randomBoolean()) { - newState.setPhaseTime(randomValueOtherThan(toMutate.getPhaseTime(), ESTestCase::randomLong)); - changed = true; - } - if (randomBoolean()) { - newState.setActionTime(randomValueOtherThan(toMutate.getActionTime(), ESTestCase::randomLong)); - changed = true; - } - if (randomBoolean()) { - newState.setStepTime(randomValueOtherThan(toMutate.getStepTime(), ESTestCase::randomLong)); - changed = true; - } - if (randomBoolean()) { - newState.setIndexCreationDate(randomValueOtherThan(toMutate.getLifecycleDate(), ESTestCase::randomLong)); - changed = true; - } - - if (changed == false) { - return LifecycleExecutionState.builder().build(); - } - return newState.build(); } @@ -215,6 +228,10 @@ static Map createCustomMetadata() { customMetadata.put("snapshot_repository", repositoryName); customMetadata.put("snapshot_name", snapshotName); customMetadata.put("snapshot_index_name", snapshotIndexName); + customMetadata.put("shrink_index_name", randomAlphaOfLengthBetween(5, 20)); + customMetadata.put("rollup_index_name", randomAlphaOfLengthBetween(5, 20)); + customMetadata.put("is_auto_retryable_error", String.valueOf(randomBoolean())); + customMetadata.put("failed_step_retry_count", String.valueOf(randomInt())); return customMetadata; } } diff --git a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java index dc121bb1d5cf4..6566a7f44ec08 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java @@ -11,6 +11,7 @@ import org.apache.http.entity.StringEntity; import org.elasticsearch.client.Request; import org.elasticsearch.client.Response; +import org.elasticsearch.client.ResponseException; import org.elasticsearch.cluster.metadata.DataStream; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.metadata.Template; @@ -207,7 +208,7 @@ public void testDeleteActionDeletesSearchableSnapshot() throws Exception { } public void testCreateInvalidPolicy() { - IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> createPolicy(client(), policy, + ResponseException exception = expectThrows(ResponseException.class, () -> createPolicy(client(), policy, new Phase("hot", TimeValue.ZERO, org.elasticsearch.common.collect.Map.of(RolloverAction.NAME, new RolloverAction(null, null, null, 1L), SearchableSnapshotAction.NAME, new SearchableSnapshotAction(randomAlphaOfLengthBetween(4, 10)))), @@ -218,7 +219,7 @@ public void testCreateInvalidPolicy() { ) ); - assertThat(exception.getMessage(), is("phases [warm,cold] define one or more of [forcemerge, freeze, shrink, rollup]" + + assertThat(exception.getMessage(), containsString("phases [warm,cold] define one or more of [forcemerge, freeze, shrink, rollup]" + " actions which are not allowed after a managed index is mounted as a searchable snapshot")); } @@ -458,7 +459,7 @@ public void testSecondSearchableSnapshotUsingDifferentRepoThrows() throws Except String secondRepo = randomAlphaOfLengthBetween(10, 20); createSnapshotRepo(client(), snapshotRepo, randomBoolean()); createSnapshotRepo(client(), secondRepo, randomBoolean()); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + ResponseException e = expectThrows(ResponseException.class, () -> createPolicy(client(), policy, null, null, new Phase("cold", TimeValue.ZERO, singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(snapshotRepo, randomBoolean()))),