[ILM] Version the order of the actions in a phase. #97745

andreidan · 2023-07-18T09:31:11Z

This adds versioning for the order of actions within a phase in ILM.

Once an index enters a phase it will execute all the actions in the phase
according to one order (the version it sets when entering the phase).
Note that, as this is a new field we store in the ILM execution state and
expose via the explain API, some clusters out there don't have a value
for the actions_order_version. We treat a lack of value as version 1.

On transition to the next phase, we execute the actions in the current
latest order.

The versioning of the actions order must obey the following invariants:

All versions must exist, from current all the way down to VERSION_ONE
All phases must contain all versions
Higher versions must contain all the actions in the previous version, irrespective of order (i.e. we don't support removing actions)

Aside from the versioning this also changes the order of the downsample
action in the warm and cold phase (fixes #96734)
in order to exemplify the versioning and use a real life example as part of
an integration test.

Fixes #97444

andreidan · 2023-07-18T16:37:42Z

...plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java

+    public void testTimeseriesRegistryContainsAllActionsVersions() {
+        for (int version = INSTANCE.getLatestActionsOrderVersion(); version >= VERSION_ONE; version--) {
+            assertThat(ORDERED_VALID_HOT_ACTIONS.get(version), notNullValue());
+            assertThat(ORDERED_VALID_WARM_ACTIONS.get(version), notNullValue());
+            assertThat(ORDERED_VALID_COLD_ACTIONS.get(version), notNullValue());
+            assertThat(ORDERED_VALID_FROZEN_ACTIONS.get(version), notNullValue());
+            assertThat(ORDERED_VALID_DELETE_ACTIONS.get(version), notNullValue());
+        }
+    }
+
+    public void testTimeseriesRegistryDoesntRemoveActionsInFutureVersions() {
+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_HOT_ACTIONS);
+
+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_WARM_ACTIONS);
+
+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_COLD_ACTIONS);
+
+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_FROZEN_ACTIONS);
+
+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_DELETE_ACTIONS);
+    }


These two tests encapsulate the versioning invariants:

All versions must exist, from current all the way down to VERSION_ONE

All phases must contain all versions

Higher versions must contain all the actions in the previous version, irrespective of order (i.e. we don't support removing actions)

Higher versions must contain all the actions in the previous version, irrespective of order (i.e. we don't support removing actions)

Okay, I left a comment about this above, not sure if we should also have an assert for it in the code (though having a test does make me feel better)

andreidan · 2023-07-19T17:25:55Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

@@ -60,9 +63,11 @@ public class PolicyStepsRegistry {
    // keeps track of existing policies in the cluster state
    private final SortedMap<String, LifecyclePolicyMetadata> lifecyclePolicyMap;
    // keeps track of what the first step in a policy is, the key is policy name
-    private final Map<String, Step> firstStepMap;
+    private final Map<VersionedPolicyKey, Step> firstStepMap;


We need the firstStep map versioned as if the first action in a phase is reordered in a subsequent version we'd get a different first step in the phase depending on which version we're executing

andreidan · 2023-07-19T17:26:07Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

    // keeps track of a mapping from policy/step-name to respective Step, the key is policy name
-    private final Map<String, Map<Step.StepKey, Step>> stepMap;
+    private final Map<VersionedPolicyKey, Map<Step.StepKey, Step>> stepMap;


So stepMap is a bit tricky.

Given we don't allow removing actions across versions, so a later version will always contain all the actions in a previous version, and the fact that the stepMap is solely used in boolean stepExists(final String policy, final Step.StepKey stepKey) (note we do NOT return the Step that's in the stepMap), one could argue we don't technically need it versioned - it could hold only the latest actions version of the policy.

And that's true. The latest actions order will have all the step KEYs that exist in the previous versions. The value, the Step associated with a step KEY, could be different (if actions are reordered, the last step KEY in an action will point to different STEPs across actions order) however we never expose the Step.

So we could get away with not versioning the stepMap however it felt error prone to me. If we decide to expose the Step associated with a step key to the outside world we'd only ever expose the latest version Step (it'd be a very subtle bug). Also it feels wrong to me to have a steps registry that's not aware of all the versions of the steps.

andreidan · 2023-07-19T19:26:30Z

@elasticmachine update branch

… version 2

elasticsearchmachine · 2023-07-20T19:15:13Z

Pinging @elastic/es-data-management (Team:Data Management)

andreidan · 2023-07-24T08:11:02Z

@elasticmachine update branch

gmarouli · 2023-07-24T09:00:53Z

...plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java

+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_DELETE_ACTIONS);
+    }
+
+    private static void assertHigerVersionsContainActionsInLowerVersions(Map<Integer, List<String>> versionedActions) {


Typo in assertHigerVersionsContainActionsInLowerVersions -> assertHigherVersionsContainActionsInLowerVersions

++ Thanks Mary

Is the rest ... LGTM ? :)

dakrone

Thanks for working on this Andrei, and I'm sorry it's taken me so long to get to reviewing it. I left some pretty minor comments but this looks solid otherwise.

dakrone · 2023-08-04T17:24:55Z

server/src/main/java/org/elasticsearch/cluster/metadata/LifecycleExecutionState.java

 ) {

+    public LifecycleExecutionState {
+        if (actionsOrderVersion == null) {


I think we should also enforce that it's > 0, to avoid weird -1 issues

dakrone · 2023-08-04T17:25:36Z

server/src/main/java/org/elasticsearch/cluster/metadata/LifecycleExecutionState.java

@@ -58,6 +65,7 @@ public record LifecycleExecutionState(
    private static final String SNAPSHOT_INDEX_NAME = "snapshot_index_name";
    private static final String SHRINK_INDEX_NAME = "shrink_index_name";
    private static final String DOWNSAMPLE_INDEX_NAME = "rollup_index_name";
+    private static final String ACTIONS_ORDER_VERSION = "actions_order_version";


I think we should go with action_order_version and avoid the awkward plural-action-but-singular-version noun

dakrone · 2023-08-04T17:28:04Z

...lugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/IndexLifecycleExplainResponse.java

@@ -221,7 +230,8 @@ private IndexLifecycleExplainResponse(
        String snapshotName,
        String shrinkIndexName,
        BytesReference stepInfo,
-        PhaseExecutionInfo phaseExecutionInfo
+        PhaseExecutionInfo phaseExecutionInfo,
+        Integer actionsOrderVersion


We treat this as non-nullable elsewhere with the check in the state against null and setting it to 1. Should we remove the nullification here and make it a regular int, enforcing it to be 1 if it's unset?

dakrone · 2023-08-04T17:28:42Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecycleType.java

@@ -31,4 +31,6 @@ public interface LifecycleType extends NamedWriteable {
     *             if a specific phase or lack of a specific phase is invalid.
     */
    void validate(Collection<Phase> phases);
+
+    int getLatestActionsOrderVersion();


Can you add javadocs for this?

dakrone · 2023-08-04T17:30:15Z

.../core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleActionsRegistry.java

+        VERSION_ONE,
+        Stream.of(
+            SetPriorityAction.NAME,
+            UnfollowAction.NAME,
+            RolloverAction.NAME,
+            ReadOnlyAction.NAME,
+            DownsampleAction.NAME,
+            ShrinkAction.NAME,
+            ForceMergeAction.NAME,
+            SearchableSnapshotAction.NAME
+        ).filter(Objects::nonNull).toList(),
+        VERSION_TWO,
+        Stream.of(
+            SetPriorityAction.NAME,
+            UnfollowAction.NAME,
+            RolloverAction.NAME,
+            ReadOnlyAction.NAME,
+            DownsampleAction.NAME,
+            ShrinkAction.NAME,
+            ForceMergeAction.NAME,
+            SearchableSnapshotAction.NAME
+        ).filter(Objects::nonNull).toList()


Since these are the same, can we put them in a variable so it's clear here that the order isn't changing between versions?

dakrone · 2023-08-04T17:36:49Z

.../core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleActionsRegistry.java

+    public static final int VERSION_ONE = 1;
+    // moves the downsample action after the migrate action in the warm and cold phases
+    public static final int VERSION_TWO = 2;
+    public static final int CURRENT_VERSION = VERSION_TWO;


Suggestion, maybe we should call this LATEST_VERSION, what do you think?

dakrone · 2023-08-04T17:37:52Z

...plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java

+    public void testTimeseriesRegistryContainsAllActionsVersions() {
+        for (int version = INSTANCE.getLatestActionsOrderVersion(); version >= VERSION_ONE; version--) {
+            assertThat(ORDERED_VALID_HOT_ACTIONS.get(version), notNullValue());
+            assertThat(ORDERED_VALID_WARM_ACTIONS.get(version), notNullValue());
+            assertThat(ORDERED_VALID_COLD_ACTIONS.get(version), notNullValue());
+            assertThat(ORDERED_VALID_FROZEN_ACTIONS.get(version), notNullValue());
+            assertThat(ORDERED_VALID_DELETE_ACTIONS.get(version), notNullValue());
+        }
+    }
+
+    public void testTimeseriesRegistryDoesntRemoveActionsInFutureVersions() {
+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_HOT_ACTIONS);
+
+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_WARM_ACTIONS);
+
+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_COLD_ACTIONS);
+
+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_FROZEN_ACTIONS);
+
+        assertHigerVersionsContainActionsInLowerVersions(ORDERED_VALID_DELETE_ACTIONS);
+    }


Higher versions must contain all the actions in the previous version, irrespective of order (i.e. we don't support removing actions)

Okay, I left a comment about this above, not sure if we should also have an assert for it in the code (though having a test does make me feel better)

dakrone · 2023-08-04T17:41:33Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

+            if (policyMetadataToRemove != null) {
+                for (int actionsOrderVersion = policyMetadataToRemove.getPolicy()
+                    .getType()
+                    .getLatestActionsOrderVersion(); actionsOrderVersion >= VERSION_ONE; actionsOrderVersion--) {
+                    // remove all versions for the deleted policy
+                    VersionedPolicyKey key = new VersionedPolicyKey(actionsOrderVersion, policyMetadataToRemove.getName());
+                    firstStepMap.remove(key);
+                    stepMap.remove(key);
+                }
+            }


I think a comment here might help when we want to understand why we need to loop through all versions.

dakrone · 2023-08-04T17:41:55Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

+                for (int actionsOrderVersion = policy.getType()
+                    .getLatestActionsOrderVersion(); actionsOrderVersion >= VERSION_ONE; actionsOrderVersion--) {


Same here about a comment explaining why we loop through the versions

dakrone · 2023-08-04T17:43:32Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java

@@ -214,7 +232,8 @@ private List<Step> getAllStepsForIndex(ClusterState state, Index index) {
            ClientHelper.INDEX_LIFECYCLE_ORIGIN,
            policyMetadata.getHeaders()
        );
-        return policyMetadata.getPolicy().toSteps(policyClient, licenseState);
+        return policyMetadata.getPolicy()
+            .toSteps(policyClient, indexMetadata.getLifecycleExecutionState().actionsOrderVersion(), licenseState);


I know it's extremely unlikely, but could we possibly reach this and have indexMetadata.getLifecycleExecutionState() return null and hit an NPE?

andreidan added >non-issue :Data Management/ILM+SLM Index and Snapshot lifecycle management WIP labels Jul 18, 2023

elasticsearchmachine added the v8.10.0 label Jul 18, 2023

andreidan commented Jul 18, 2023

View reviewed changes

andreidan force-pushed the ilm-actions-order branch from a22fadf to 6b99be5 Compare July 19, 2023 13:07

andreidan added 3 commits July 19, 2023 17:24

[ILM] Version the order of the actions in a phase.

05a4cd1

Fix tests

788741b

Use CURRENT_VERSION in tests

8775cfb

andreidan force-pushed the ilm-actions-order branch 2 times, most recently from 823dafb to 8775cfb Compare July 19, 2023 16:33

Execute downsample before allocate and migrate in warm/cold

1bb3676

dakrone self-requested a review July 19, 2023 16:52

Remove unversioned toSteps method

2b106fe

andreidan commented Jul 19, 2023

View reviewed changes

elasticmachine and others added 5 commits July 19, 2023 14:26

Merge branch 'main' into ilm-actions-order

a35195b

Expose the actions order version in the explain API response

72ed49b

ILM explain output docs

6549e95

spotless

1573fb6

Add integration test with the warm phase executing version 1 and cold…

3111380

… version 2

andreidan removed the WIP label Jul 20, 2023

andreidan marked this pull request as ready for review July 20, 2023 19:14

elasticsearchmachine added the Team:Data Management Meta label for data/management team label Jul 20, 2023

Use String.format with Locale instead of formatted

9cf38a7

Merge branch 'main' into ilm-actions-order

f475798

gmarouli reviewed Jul 24, 2023

View reviewed changes

dakrone reviewed Aug 4, 2023

View reviewed changes

quux00 added v8.11.0 and removed v8.10.0 labels Aug 16, 2023

mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023

brianseeders added v8.13.0 and removed v8.12.0 labels Dec 6, 2023

andreidan mentioned this pull request Feb 12, 2024

[ILM] The downsample action runs before the allocate and migrate actions #96734

Open

elasticsearchmachine added v8.14.0 and removed v8.13.0 labels Feb 14, 2024

elasticsearchmachine added v8.15.0 and removed v8.14.0 labels Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ILM] Version the order of the actions in a phase. #97745

[ILM] Version the order of the actions in a phase. #97745

andreidan commented Jul 18, 2023 •

edited

andreidan Jul 18, 2023 •

edited

dakrone Aug 4, 2023

andreidan Jul 19, 2023 •

edited

andreidan Jul 19, 2023 •

edited

andreidan commented Jul 19, 2023

elasticsearchmachine commented Jul 20, 2023

andreidan commented Jul 24, 2023

gmarouli Jul 24, 2023

andreidan Jul 27, 2023

dakrone left a comment

dakrone Aug 4, 2023

dakrone Aug 4, 2023

dakrone Aug 4, 2023

dakrone Aug 4, 2023

dakrone Aug 4, 2023

dakrone Aug 4, 2023

dakrone Aug 4, 2023

dakrone Aug 4, 2023

dakrone Aug 4, 2023

dakrone Aug 4, 2023

		for (int actionsOrderVersion = policy.getType()
		.getLatestActionsOrderVersion(); actionsOrderVersion >= VERSION_ONE; actionsOrderVersion--) {

[ILM] Version the order of the actions in a phase. #97745

Are you sure you want to change the base?

[ILM] Version the order of the actions in a phase. #97745

Conversation

andreidan commented Jul 18, 2023 • edited

andreidan Jul 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreidan Jul 19, 2023 • edited

Choose a reason for hiding this comment

andreidan Jul 19, 2023 • edited

Choose a reason for hiding this comment

andreidan commented Jul 19, 2023

elasticsearchmachine commented Jul 20, 2023

andreidan commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakrone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreidan commented Jul 18, 2023 •

edited

andreidan Jul 18, 2023 •

edited

andreidan Jul 19, 2023 •

edited

andreidan Jul 19, 2023 •

edited