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

[ILM] Fix downsample to skip already downsampled indices #102250

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

andreidan
Copy link
Contributor

This fixes a bug in ILM where if an ILM policy is updated such that the next phase (e.g. warm) contains the downsample action from the previous phase (e.g. hot), whilst the managed index is waiting to enter the next phase (e.g. warm), if the managed index was already downsampled in hot it would get deleted in the warm phase.

Fixes #102249

This fixes a bug in ILM where if an ILM policy is updated such that the next
phase (e.g. warm) contains the downsample action from the previous phase (e.g.
hot), whilst the managed index is waiting to enter the next phase (e.g.
warm), if the managed index was already downsampled in hot it would get
deleted in the warm phase.
@andreidan andreidan requested a review from martijnvg November 15, 2023 18:28
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Nov 15, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@andreidan andreidan requested a review from dakrone November 15, 2023 18:28
Comment on lines +169 to +181
if (downsampleStatus == IndexMetadata.DownsampleTaskStatus.UNKNOWN) {
// This isn't a downsample index, but it has the name of our target downsample index - very bad, we'll skip the
// downsample action to avoid blocking the lifecycle of this index - if there
// is another downsample action configured in the next phase, it'll be able to proceed successfully
logger.warn(
"index [{}] as part of policy [{}] cannot be downsampled at interval [{}] in phase [{}] because it has"
+ " the name of the target downsample index and is itself not a downsampled index. Skipping the downsample "
+ "action.",
index.getName(),
indexMetadata.getLifecyclePolicyName(),
fixedInterval,
phase
);
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand, why do we only log this warning if the status is unknown, if the status is successful we still skip this downsample, but we don't log about it. Shouldn't we log in all cases then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the status is successful we don't log anything because this action (even as a no-op) has achieve its goal - the index is downsampled at the desired /configured interval.

If the index has the shape of a downsample index, but it is not (i.e. status is UNKNOWN) we're skipping this action without achieving its goal - i.e. the data is not downsampled. We're logging the warning that we haven't executed downsampling.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, thanks for the explanation

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this Andrei

Comment on lines +169 to +181
if (downsampleStatus == IndexMetadata.DownsampleTaskStatus.UNKNOWN) {
// This isn't a downsample index, but it has the name of our target downsample index - very bad, we'll skip the
// downsample action to avoid blocking the lifecycle of this index - if there
// is another downsample action configured in the next phase, it'll be able to proceed successfully
logger.warn(
"index [{}] as part of policy [{}] cannot be downsampled at interval [{}] in phase [{}] because it has"
+ " the name of the target downsample index and is itself not a downsampled index. Skipping the downsample "
+ "action.",
index.getName(),
indexMetadata.getLifecyclePolicyName(),
fixedInterval,
phase
);
Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, thanks for the explanation

@andreidan andreidan added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 15, 2023
@elasticsearchmachine elasticsearchmachine merged commit ffed132 into elastic:main Nov 15, 2023
@andreidan andreidan deleted the fix-downsample-twice-ilm branch November 15, 2023 19:12
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.11

andreidan added a commit to andreidan/elasticsearch that referenced this pull request Nov 15, 2023
)

This fixes a bug in ILM where if an ILM policy is updated such that the
next phase (e.g. warm) contains the downsample action from the previous
phase (e.g. hot), whilst the managed index is waiting to enter the next
phase (e.g. warm), if the managed index was already downsampled in hot
it would get deleted in the warm phase.

Fixes elastic#102249
elasticsearchmachine pushed a commit that referenced this pull request Nov 15, 2023
…102251)

This fixes a bug in ILM where if an ILM policy is updated such that the
next phase (e.g. warm) contains the downsample action from the previous
phase (e.g. hot), whilst the managed index is waiting to enter the next
phase (e.g. warm), if the managed index was already downsampled in hot
it would get deleted in the warm phase.

Fixes #102249
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks Andrei for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >bug :Data Management/ILM+SLM Index and Snapshot lifecycle management Team:Data Management Meta label for data/management team v8.11.2 v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Downsample action can attempt to downsample on the same interval and delete data
4 participants