Reject creating ILM policies with phase timings are not >= previous phase by dakrone · Pull Request #70089 · elastic/elasticsearch

dakrone · 2021-03-08T15:02:05Z

It can be confusing to configure policies with phase timings that get smaller, because phase timings
are absolute. To make things a little clearer, this commit now rejects policies where a configured
min_age is less than a previous phase's min_age.

This validation is added only to the PutLifecycleAction.Request instead of the
TimeseriesLifecycleType class because we cannot do this validation every time a lifecycle is
created or else we will block cluster state from being recoverable for existing clusters that may
have invalid policies.

Resolves #70032

…hase It can be confusing to configure policies with phase timings that get smaller, because phase timings are absolute. To make things a little clearer, this commit now rejects policies where a configured min_age is less than a previous phase's min_age. This validation is added only to the `PutLifecycleAction.Request` instead of the `TimeseriesLifecycleType` class because we cannot do this validation every time a lifecycle is created or else we will block cluster state from being recoverable for existing clusters that may have invalid policies. Resolves elastic#70032

elasticmachine · 2021-03-08T15:02:08Z

Pinging @elastic/es-core-features (Team:Core/Features)

dakrone · 2021-03-08T15:10:20Z

@elasticmachine update branch

andreidan

Thanks for working on this Lee. I've left a few rather minor comments

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

andreidan · 2021-03-08T16:11:11Z

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

+                TimeValue phaseMinAge = phase.getMinimumAge();
+                Set<String> followingPhases = new HashSet<>(ORDERED_VALID_PHASES.subList(i + 1, ORDERED_VALID_PHASES.size()));
+                Set<Phase> phasesWithBadAges = phases.stream()
+                    .filter(p -> followingPhases.contains(p.getName()))


I believe that would always be true, but maybe I'm misunderstanding the intent here?

So here we are looping through the phases, if we're on the "warm" phase then the followingPhases will be cold, frozen, delete. Then, this takes the phases configured in the policy (let's say it has hot, warm, frozen, and delete) and filters them to return a stream with "frozen" and "delete" in it because there is no "cold" phase.

Hopefully that makes sense?

That makes sense, but I thought the sublist above doest that already?

ORDERED_VALID_PHASES.subList(i + 1, ORDERED_VALID_PHASES.size())

Which I'm just seeing is placed in a HashSet so the order might be lost? I think we should keep an ArrayList above?

List<String> followingPhases = new ArrayList<>(ORDERED_VALID_PHASES.subList(i + 1, ORDERED_VALID_PHASES.size()));

Ah, I finally got there. I mixed-up the collections.

So I think the order can be indeed lost because we don't expect the validateMonotonicallyIncreasingPhaseTimings to be called with the list of ordered phases (and it is not https://github.com/elastic/elasticsearch/pull/70089/files#diff-dbca592978c7f9e3f747487f76b2e7327bdcac2fa635a23949aa35fba7411bfbR65) but we then iterate over the phases collection we received as if it is ordered - https://github.com/elastic/elasticsearch/pull/70089/files#diff-5964cde83b3b053cf693079cb18a73f4a5743c59590f09fda7dd268613098ce9R390

This is where the mix was in my mind. I think if we keep the order in the followingPhases collection (by not converting it to a Set) and iterate over it we can overcome this:

eg.

followingPhases.stream().filter(p -> p.getMinimumAge() != null && p.getMinimumAge().equals(TimeValue.ZERO) == false).filter(p -> p.getMinimumAge().compareTo(phaseMinAge) < 0)

So I think the order can be indeed lost because we don't expect the validateMonotonicallyIncreasingPhaseTimings to be called with the list of ordered phases (and it is not #70089 (files)) but we then iterate over the phases collection we received as if it is ordered

We don't iterate over the phases collection received though, instead we iterate over ORDERED_VALID_PHASES, picking out the phases from the provided collection one-by-one.

followingPhases doesn't need to be ordered because it is a subset of the phases occurring after the maybePhase, so if phase were warm then followingPhases would be cold, frozen, delete, and the order for those doesn't matter because they are all compared independently to the timings for the "warm" phase.

Hopefully that was clear? I'm probably not explaining it in the clearest way.

Thanks for the clarification. I was thrown by the phases.stream() line which is iterating over the possibly unordered parameter list, however the .filter(p -> followingPhases.contains(p.getName())) filtering makes sure we do only process the following phases, and you're right the order at that stage does not matter anymore.

Thanks for bearing with me!

andreidan · 2021-03-08T16:16:42Z

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

+            Phase warmPhase = new Phase(WARM_PHASE, TimeValue.timeValueDays(2), Collections.emptyMap());
+            Phase coldPhase = new Phase(COLD_PHASE, null, Collections.emptyMap());
+            Phase frozenPhase = new Phase(FROZEN_PHASE, TimeValue.timeValueDays(1), Collections.emptyMap());
+            Phase deletePhase = new Phase(DELETE_PHASE, TimeValue.timeValueHours(36), Collections.emptyMap());


I suspect we might get more errors than we want (or maybe we do want them?) in the following case:

hot 1d, warm 3d, cold 0, frozen 2d, delete 1d

Can we add a test for this please? (I think we either want to support to multiple errors in this case, or otherwise we need to add a break after this line https://github.com/elastic/elasticsearch/pull/70089/files#diff-5964cde83b3b053cf693079cb18a73f4a5743c59590f09fda7dd268613098ce9R400

Yep, I've added a test for this and some logic to avoid it in 77dea57

dakrone · 2021-03-08T18:16:07Z

@elasticmachine update branch

andreidan

Thanks for iterating on this and for adding this validation Lee.

LGTM

…hase (elastic#70089) It can be confusing to configure policies with phase timings that get smaller, because phase timings are absolute. To make things a little clearer, this commit now rejects policies where a configured min_age is less than a previous phase's min_age. This validation is added only to the PutLifecycleAction.Request instead of the TimeseriesLifecycleType class because we cannot do this validation every time a lifecycle is created or else we will block cluster state from being recoverable for existing clusters that may have invalid policies. Resolves elastic#70032

…ious phase (#70089) (#70110) It can be confusing to configure policies with phase timings that get smaller, because phase timings are absolute. To make things a little clearer, this commit now rejects policies where a configured min_age is less than a previous phase's min_age. This validation is added only to the PutLifecycleAction.Request instead of the TimeseriesLifecycleType class because we cannot do this validation every time a lifecycle is created or else we will block cluster state from being recoverable for existing clusters that may have invalid policies. Resolves #70032

dakrone added :Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. v8.0.0 v7.13.0 labels Mar 8, 2021

dakrone requested a review from andreidan March 8, 2021 15:02

elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Mar 8, 2021

Merge branch 'master' into ilm-validate-increasing-phase-timings

b78aa70

andreidan reviewed Mar 8, 2021

View reviewed changes

dakrone added 2 commits March 8, 2021 11:03

Add assertions, use days for the time unit in test

a9e3593

Handle duplicate errors (only report the first one)

77dea57

Merge branch 'master' into ilm-validate-increasing-phase-timings

387fcb5

andreidan approved these changes Mar 8, 2021

View reviewed changes

dakrone merged commit 5df763f into elastic:master Mar 8, 2021

dakrone deleted the ilm-validate-increasing-phase-timings branch March 8, 2021 19:40

dakrone mentioned this pull request Mar 8, 2021

[7.x] Reject creating ILM policies with phase timings are not >= previous phase (#70089) #70110

Merged

cjcenizal mentioned this pull request Mar 11, 2021

Improve human-readability of ILM timing error message #70336

Closed

pugnascotia added the >enhancement label Apr 20, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

stefnestor mentioned this pull request Feb 24, 2022

ILM API should disallow a policy with conflicting phase timing #63141

Closed

Conversation

dakrone commented Mar 8, 2021

Uh oh!

elasticmachine commented Mar 8, 2021

Uh oh!

dakrone commented Mar 8, 2021

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreidan Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

dakrone Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

andreidan Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

andreidan Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreidan Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

andreidan Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

dakrone Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone commented Mar 8, 2021

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andreidan Mar 8, 2021 •

edited

Loading

dakrone Mar 8, 2021 •

edited

Loading

dakrone Mar 8, 2021 •

edited

Loading