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

Remove `indexing_complete` when removing policy #36620

Merged

Conversation

Projects
None yet
3 participants
@gwbrown
Copy link
Contributor

commented Dec 13, 2018

Leaving index.lifecycle.indexing_complete in place when removing the
lifecycle policy from an index can cause confusion, as if a new policy
is associated with the policy, rollover will be silently skipped.
Removing that setting when removing the policy from an index makes
associating a new policy with the index more involved, but allows ILM to
fail loudly, rather than silently skipping operations which the user may
assume are being performed.


Context: After synchronous discussion earlier today, @talevy, @eskibars, and I decided that failing loudly when we might possibly be able to succeed is better than being silently surprising, especially when the fix is relatively straightforward.

Remove `indexing_complete` when removing policy
Leaving `index.lifecycle.indexing_complete` in place when removing the
lifecycle policy from an index can cause confusion, as if a new policy
is associated with the policy, rollover will be silently skipped.
Removing that setting when removing the policy from an index makes
associating a new policy with the index more involved, but allows ILM to
fail loudly, rather than silently skipping operations which the user may
assume are being performed.
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Dec 13, 2018

@gwbrown

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

@martijnvg I requested your review here partially to double-check my thinking here: This setting will be used (in combination with other signals) to indicate to ILM that it is safe to de-follow a following index.

I believe the only case when removing this setting when removing a policy would have an impact is if the policy is removed from an index after it rolls over (and so is actually finished indexing), but a follower has not yet caught up (and therefore has not yet de-followed the index) before the policy is removed.

In this case, the following index will not have this setting set, and so be stuck waiting for this setting to be set. The fix is simply to set this setting manually. Correct?

gwbrown added some commits Dec 17, 2018

Adjust order of checks in WaitForRolloverReadyStep
This allows ILM to error out properly for indices that have a valid
alias, but are not the write index, while still handling
`indexing_complete` on old-style aliases and rollover (that is, those
which only point to a single index at a time with no explicit write
index)
@gwbrown

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

@colings86 I've added the test you asked for. In the process of writing the test, I ran across a bug: Instead of moving to the error step at https://github.com/elastic/elasticsearch/pull/36620/files#diff-5de660b10fd8979ff41a9ff8d850aaecR569, execution of the policy for this index would start checking for rollover, because we didn't check that the index was actually the write index for the alias.

I've added that check in WaitForRolloverReadyStep, and adjusted the order of the checks in that step, which is necessary to accommodate both new-style aliases with write indices set and legacy-style aliases which only point to one index at a time.

Because there's now a slight difference in how those are handled, I've tweaked createIndexWithSettings to randomize between new-style and legacy-style aliases - I've run all tests both ways and they pass.

It might be worth pulling those changes out into a separate PR, but this PR won't pass without those changes.

This is likely worth discussing when you have time. Also, big thanks to @talevy for helping me figure out the alias gymnastics here!

@gwbrown

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

@elasticmachine run the gradle build tests 2 please

@gwbrown gwbrown merged commit d39956c into elastic:master Dec 19, 2018

8 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci-1 Build finished.
Details
elasticsearch-ci-2 Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/docs-check Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Dec 19, 2018

Remove `indexing_complete` when removing policy (elastic#36620)
Leaving `index.lifecycle.indexing_complete` in place when removing the
lifecycle policy from an index can cause confusion, as if a new policy
is associated with the policy, rollover will be silently skipped.
Removing that setting when removing the policy from an index makes
associating a new policy with the index more involved, but allows ILM to
fail loudly, rather than silently skipping operations which the user may
assume are being performed.

* Adjust order of checks in WaitForRolloverReadyStep

This allows ILM to error out properly for indices that have a valid
alias, but are not the write index, while still handling
`indexing_complete` on old-style aliases and rollover (that is, those
which only point to a single index at a time with no explicit write
index)

gwbrown added a commit that referenced this pull request Dec 20, 2018

Remove `indexing_complete` when removing policy (#36620)
Leaving `index.lifecycle.indexing_complete` in place when removing the
lifecycle policy from an index can cause confusion, as if a new policy
is associated with the policy, rollover will be silently skipped.
Removing that setting when removing the policy from an index makes
associating a new policy with the index more involved, but allows ILM to
fail loudly, rather than silently skipping operations which the user may
assume are being performed.

* Adjust order of checks in WaitForRolloverReadyStep

This allows ILM to error out properly for indices that have a valid
alias, but are not the write index, while still handling
`indexing_complete` on old-style aliases and rollover (that is, those
which only point to a single index at a time with no explicit write
index)

gwbrown added a commit that referenced this pull request Dec 20, 2018

Remove `indexing_complete` when removing policy (#36620) (#36860)
Leaving `index.lifecycle.indexing_complete` in place when removing the
lifecycle policy from an index can cause confusion, as if a new policy
is associated with the policy, rollover will be silently skipped.
Removing that setting when removing the policy from an index makes
associating a new policy with the index more involved, but allows ILM to
fail loudly, rather than silently skipping operations which the user may
assume are being performed.

* Adjust order of checks in WaitForRolloverReadyStep

This allows ILM to error out properly for indices that have a valid
alias, but are not the write index, while still handling
`indexing_complete` on old-style aliases and rollover (that is, those
which only point to a single index at a time with no explicit write
index)

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.