Skip to content
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.

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

Allow ILM step transition to the phase terminal step #91754

Merged
merged 8 commits into from
Mar 14, 2023

Conversation

andreidan
Copy link
Contributor

ILM tries to honour the cached phase however, some steps are implicit (e.g. injected actions or the terminal policy/phase step) Currently ILM would throw an exception if the currently cached phase was removed:

step [{"phase":"warm","action":"complete","name":"complete"}] for index [index] with policy [my-policy] does not exist

This fixes this scenario allowing ILM to transition to the phase complete step even if the phase has been removed from the actual policy.

Part of #91749

ILM tries to honour the cached phase however, some steps are implicit
(e.g. injected actions or the terminal policy/phase step)
Currently ILM would throw an exception if the currently cached phase
was removed:
```
step [{"phase":"warm","action":"complete","name":"complete"}] for index [index] with policy [my-policy] does not exist
```

This fixes this scenario allowing ILM to transition to the phase complete
step even if the phase has been removed from the actual policy.
@andreidan andreidan added >bug :Data Management/ILM+SLM Index and Snapshot lifecycle management v7.17.8 v8.6.1 labels Nov 21, 2022
@elasticsearchmachine elasticsearchmachine added v8.7.0 Team:Data Management Meta label for data/management team labels Nov 21, 2022
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @andreidan, I've created a changelog YAML for you.

Comment on lines -593 to -602
Map<String, LifecycleAction> actions = new HashMap<>();
actions.put(SetPriorityAction.NAME, new SetPriorityAction(100));
Phase hotPhase = new Phase("hot", TimeValue.ZERO, actions);
Map<String, Phase> phases = Collections.singletonMap("hot", hotPhase);
LifecyclePolicy policyWithoutRollover = new LifecyclePolicy("my-policy", phases);
LifecyclePolicyMetadata policyMetadata = new LifecyclePolicyMetadata(policyWithoutRollover, Collections.emptyMap(), 2L, 2L);

ClusterState existingState = ClusterState.builder(ClusterState.EMPTY_STATE)
.metadata(Metadata.builder(Metadata.EMPTY_METADATA).put(meta, false).build())
.build();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were unused

Copy link
Contributor

Choose a reason for hiding this comment

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

<3

&& (stepRegistry.stepExists(policyName, newStepKey) == false && newStepKey.equals(TerminalPolicyStep.KEY) == false)) {
&& (stepRegistry.stepExists(policyName, newStepKey) == false
&& newStepKey.equals(TerminalPolicyStep.KEY) == false
&& newStepKey.equals(new Step.StepKey(lifecycleState.phase(), PhaseCompleteStep.NAME, PhaseCompleteStep.NAME)) == false)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: The new here threw me off when I read it the first time. What do you think about adding a helper method in org.elasticsearch.xpack.core.ilm.PhaseCompleteStep:

  • Either adding boolean isFinalStep(StepKey stepKey) ,
  • or to add a final step key constructor:
    public static StepKey stepKey(String phase) {
        return new StepKey(phase, NAME, NAME);
    }

Copy link
Contributor

@gmarouli gmarouli left a comment

Choose a reason for hiding this comment

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

LGTM, thank for improving this, I left a minor comment.

@andreidan
Copy link
Contributor Author

@elasticmachine update branch

@andreidan
Copy link
Contributor Author

@elasticmachine update branch

@andreidan
Copy link
Contributor Author

@elasticmachine update branch

@andreidan andreidan added v8.7.1 auto-backport-and-merge Automatically create backport pull requests and merge when ready and removed v8.6.3 labels Mar 14, 2023
@andreidan andreidan merged commit 3962556 into elastic:main Mar 14, 2023
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
7.17 Commit could not be cherrypicked due to conflicts
8.7

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 91754

andreidan added a commit to andreidan/elasticsearch that referenced this pull request Mar 14, 2023
ILM tries to honour the cached phase however, some steps are implicit
(e.g. injected actions or the terminal policy/phase step)
Currently ILM would throw an exception if the currently cached phase
was removed:
```
step [{"phase":"warm","action":"complete","name":"complete"}] for index [index] with policy [my-policy] does not exist
```

This fixes this scenario allowing ILM to transition to the phase complete
step even if the phase has been removed from the actual policy.
elasticsearchmachine pushed a commit that referenced this pull request Mar 14, 2023
ILM tries to honour the cached phase however, some steps are implicit
(e.g. injected actions or the terminal policy/phase step)
Currently ILM would throw an exception if the currently cached phase
was removed:
```
step [{"phase":"warm","action":"complete","name":"complete"}] for index [index] with policy [my-policy] does not exist
```

This fixes this scenario allowing ILM to transition to the phase complete
step even if the phase has been removed from the actual policy.
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Mar 20, 2023
ILM tries to honour the cached phase however, some steps are implicit
(e.g. injected actions or the terminal policy/phase step)
Currently ILM would throw an exception if the currently cached phase
was removed:
```
step [{"phase":"warm","action":"complete","name":"complete"}] for index [index] with policy [my-policy] does not exist
```

This fixes this scenario allowing ILM to transition to the phase complete
step even if the phase has been removed from the actual policy.

(cherry picked from commit 3962556)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
andreidan added a commit that referenced this pull request Mar 20, 2023
ILM tries to honour the cached phase however, some steps are implicit
(e.g. injected actions or the terminal policy/phase step)
Currently ILM would throw an exception if the currently cached phase
was removed:
```
step [{"phase":"warm","action":"complete","name":"complete"}] for index [index] with policy [my-policy] does not exist
```

This fixes this scenario allowing ILM to transition to the phase complete
step even if the phase has been removed from the actual policy.

(cherry picked from commit 3962556)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
saarikabhasi pushed a commit to saarikabhasi/elasticsearch that referenced this pull request Apr 10, 2023
ILM tries to honour the cached phase however, some steps are implicit
(e.g. injected actions or the terminal policy/phase step)
Currently ILM would throw an exception if the currently cached phase
was removed:
```
step [{"phase":"warm","action":"complete","name":"complete"}] for index [index] with policy [my-policy] does not exist
```

This fixes this scenario allowing ILM to transition to the phase complete
step even if the phase has been removed from the actual policy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport-and-merge Automatically create backport pull requests and merge when ready >bug :Data Management/ILM+SLM Index and Snapshot lifecycle management Team:Data Management Meta label for data/management team v7.17.10 v8.7.1 v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants