Use custom index metadata for ILM state #33783

gwbrown · 2018-09-17T18:02:48Z

Using index settings for ILM state is fragile and exposes too much
information that doesn't need to be exposed. Using custom index metadata
is more resilient and allows more controlled access to internal
information.

Also adds a class (LifecycleExecutionState) to handle serializing/deserializing information to/from the Map<String, String> that must be used for custom index metadata.

This PR is pretty big, so some areas to focus on for reviewers:

The LifecycleExecutionState class and associated Tests
The new ShrinkCopyExecutionState, which copies over the custom metadata after the shrunken index has been created, and surrounding machinery/tests.
Changes to IndexLifecycleRunner, as GitHub collapses this and it's easy to miss.

Using index settings for ILM state is fragile and exposes too much information that doesn't need to be exposed. Using custom index metadata is more resilient and allows more controlled access to internal information.

elasticmachine · 2018-09-17T18:02:49Z

Pinging @elastic/es-core-infra

dakrone

I left some comments, I think we should try to avoid using "" and "-1" for empty values, instead, removing them from the map entirely

dakrone · 2018-09-17T18:24:47Z

.../core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecycleExecutionState.java

+            .setFailedStep(customData.getOrDefault(FAILED_STEP, ""))
+            .setStepInfo(customData.getOrDefault(STEP_INFO, ""))
+            .setPhaseDefinition(customData.getOrDefault(PHASE_DEFINITION, ""))
+            .setIndexCreationDate(Long.parseLong(customData.getOrDefault(INDEX_CREATION_DATE, "-1")))


Could you add more descriptive parsing error messages in the event that these are incorrectly formatted as a long (ie, including the name)

.../core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecycleExecutionState.java

dakrone · 2018-09-17T18:33:34Z

.../core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecycleExecutionState.java

+        return Collections.unmodifiableMap(result);
+    }
+
+    public String getPhase() {


This may be my (semi controversial) opinion, but I think we could ditch the getters and make the vars public since this class does nothing with the values; what do you think?

dakrone · 2018-09-17T18:34:10Z

.../core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecycleExecutionState.java

+        private String step = "";
+        private String failedStep = "";
+        private String stepInfo = "";
+        private String phaseDefinition = "";


See my above comment, I think these should default to null and not be written if they aren't present

dakrone · 2018-09-17T18:35:41Z

...core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/ShrinkCopyExecutionState.java

+
+import static org.elasticsearch.xpack.core.indexlifecycle.LifecycleExecutionState.ILM_CUSTOM_METADATA_KEY;
+
+public class ShrinkCopyExecutionState extends ClusterStateActionStep {


Can you add javadocs for what this class is for please?

I also think we could genericize the name, since we may want this for rollups in the future (so just call it CopyExecutionStateStep)

dakrone · 2018-09-17T18:38:49Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/ShrinkStep.java

-
-        Long lifecycleDate = LifecycleSettings.LIFECYCLE_INDEX_CREATION_DATE_SETTING.get(indexMetaData.getSettings());
-        if (lifecycleDate == null) {
-            throw new IllegalStateException("source index[" + indexMetaData.getIndex().getName() +


You removed this validation but didn't re-add it in the copy state step that was added, do we want to remove this entirely? I don't think we want to

.../src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecycleExecutionStateTests.java

dakrone · 2018-09-17T18:44:35Z

...ck/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java

+
+        // clear any step info or error-related settings from the current step
+        updatedState.setFailedStep("");
+        updatedState.setStepInfo("");


I think we should set these to null so they are subsequently removed from the map rather than use an empty string

dakrone · 2018-09-17T18:44:58Z

...ck/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java

        if (currentStep.getPhase().equals(nextStep.getPhase()) == false) {
            final String newPhaseDefinition;
            if ("new".equals(nextStep.getPhase()) || TerminalPolicyStep.KEY.equals(nextStep)) {
                newPhaseDefinition = nextStep.getPhase();
            } else {
                Phase nextPhase = policy.getPhases().get(nextStep.getPhase());
                if (nextPhase == null) {
-                    newPhaseDefinition = null;
+                    newPhaseDefinition = "";


Same here, I think we should use null and remove it from the map

dakrone · 2018-09-17T18:45:59Z

...ck/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/IndexLifecycleRunner.java

+        Map<String, String> newCustomData = new HashMap<>();
+
+        return IndexMetaData.builder(indexMetadata)
+            .putCustom(ILM_CUSTOM_METADATA_KEY, newCustomData)


Can we do .putCustom("ilm", null) here? Or that does break anything?

We might rather want to add a .removeCustom("ilm") method on the builder

gwbrown · 2018-09-17T23:19:00Z

...main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportExplainLifecycleAction.java

+                    lifecycleState.getPhaseTime() == null ? -1 : lifecycleState.getPhaseTime(),
+                    lifecycleState.getActionTime() == null ? -1 : lifecycleState.getActionTime(),
+                    lifecycleState.getStepTime() == null ? -1 : lifecycleState.getStepTime(),
+                    new BytesArray(lifecycleState.getStepInfo() == null ? "" : lifecycleState.getStepInfo()));


I need to go back and do something else here because this is really ugly, but otherwise it throws NPEs and I wanted to get eyes on the rest of the code.

gwbrown · 2018-09-18T22:30:51Z

@dakrone I believe I've made all the changes you requested, mind giving this another look?

dakrone

This LGTM thanks @gwbrown, I left a few more comments (no need for another review after addressing them)

dakrone · 2018-09-18T22:48:46Z

.../core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecycleExecutionState.java

+        return phaseDefinition;
+    }
+
+    public Long getIndexCreationDate() {


I think should should be getLifecycleDate() because the creation date isn't always used (sometimes it's rollover). We should also change the var name from indexCreationDate -> indexLifecycleDate

dakrone · 2018-09-18T22:49:16Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/ShrinkStep.java

-            throw new IllegalStateException("source index[" + indexMetaData.getIndex().getName() +
-                "] is missing setting[" + LifecycleSettings.LIFECYCLE_INDEX_CREATION_DATE);
+        LifecycleExecutionState lifecycleState = LifecycleExecutionState.fromIndexMetadata(indexMetaData);
+        if (lifecycleState == null) {


I think this should be if (lifecycleState.getLifecycleDate() == null) { rather than just checking if the state exists at all

dakrone · 2018-09-18T22:49:28Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/ShrinkStep.java

+        LifecycleExecutionState lifecycleState = LifecycleExecutionState.fromIndexMetadata(indexMetaData);
+        if (lifecycleState == null) {
+            throw new IllegalStateException("source index [" + indexMetaData.getIndex().getName() +
+                "] is missing creation date");


"creation date" -> "lifecycle date"

dakrone · 2018-09-18T22:51:47Z

...k/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/ShrinkStepTests.java

@@ -84,13 +84,14 @@ public void testPerformAction() throws Exception {
        String lifecycleName = randomAlphaOfLength(5);
        long creationDate = randomNonNegativeLong();
        ShrinkStep step = createRandomInstance();
+        LifecycleExecutionState.Builder lifecycleState = LifecycleExecutionState.builder();


You create this lifecycleState but never use it in the test?

dakrone · 2018-09-18T22:57:32Z

...main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportExplainLifecycleAction.java

            PhaseExecutionInfo phaseExecutionInfo = null;
            if (Strings.isNullOrEmpty(phaseDef) == false) {
                try (XContentParser parser = JsonXContent.jsonXContent.createParser(xContentRegistry,
                        DeprecationHandler.THROW_UNSUPPORTED_OPERATION, phaseDef)) {
                    phaseExecutionInfo = PhaseExecutionInfo.parse(parser, currentPhase);
                } catch (IOException e) {
                    listener.onFailure(new ElasticsearchParseException(
-                        "failed to parse [" + LifecycleSettings.LIFECYCLE_PHASE_DEFINITION + "] for index [" + index + "]", e));
+                        "failed to parse [phase_definition] for index [" + index + "]", e));


we can probable just make this

failed to parse phase definition for index [foo]

rather than making "phase_definition" seem like a special thing (since it's not a setting any more)

Using index settings for ILM state is fragile and exposes too much information that doesn't need to be exposed. Using custom index metadata is more resilient and allows more controlled access to internal information. As part of these changes, moves away from using defaults for ILM-related values, in favor of using null values to clearly indicate that the value is not present.

Use custom index metadata for ILM state

47a4c0e

Using index settings for ILM state is fragile and exposes too much information that doesn't need to be exposed. Using custom index metadata is more resilient and allows more controlled access to internal information.

gwbrown added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Sep 17, 2018

gwbrown requested review from dakrone and talevy September 17, 2018 18:02

elasticmachine mentioned this pull request Sep 17, 2018

[meta] Index Lifecycle Management Plan #29823

Closed

gwbrown mentioned this pull request Sep 17, 2018

Rebuild step on PolicyStepsRegistry.getStep #33780

Merged

dakrone requested changes Sep 17, 2018

View reviewed changes

Changes per review in elastic#33783

327165f

gwbrown commented Sep 17, 2018

View reviewed changes

gwbrown added 3 commits September 17, 2018 17:33

More review changes

e4d0bb4

Merge branch 'index-lifecycle' into ilm-settings2metadata

6449d67

Finish moving to nullable data instead of defaults

4330513

dakrone approved these changes Sep 18, 2018

View reviewed changes

gwbrown added 2 commits September 19, 2018 10:01

More review changes

382d59e

Merge branch 'index-lifecycle' into ilm-settings2metadata

b36fdfb

gwbrown merged commit 90de436 into elastic:index-lifecycle Sep 19, 2018

gwbrown deleted the ilm-settings2metadata branch September 24, 2018 20:59


		import static org.elasticsearch.xpack.core.indexlifecycle.LifecycleExecutionState.ILM_CUSTOM_METADATA_KEY;

		public class ShrinkCopyExecutionState extends ClusterStateActionStep {

Use custom index metadata for ILM state #33783

Use custom index metadata for ILM state #33783

Uh oh!

Conversation

gwbrown commented Sep 17, 2018

Uh oh!

elasticmachine commented Sep 17, 2018

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwbrown commented Sep 18, 2018

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!