Skip to content

Fix issue with ILM that write index was created before template was loaded#9617

Merged
ruflin merged 3 commits intoelastic:masterfrom
ruflin:add-ilm-test
Dec 18, 2018
Merged

Fix issue with ILM that write index was created before template was loaded#9617
ruflin merged 3 commits intoelastic:masterfrom
ruflin:add-ilm-test

Conversation

@ruflin
Copy link
Copy Markdown
Contributor

@ruflin ruflin commented Dec 18, 2018

The write index for ILM was created before the template was loaded. This meant the first template did not contain all the mappings and policy for ILM so was never rolled over. This change makes sure the template is loaded for ILM is setup up and adds a test to confirm it.

Further ILM was not respected when running metricbeat setup --template. This is now changed too.

mockbeat was modified to send multiple events instead of just one to allow the new tests.

@ruflin ruflin added review libbeat needs_backport PR is waiting to be backported to other branches. labels Dec 18, 2018
@ruflin ruflin requested a review from a team as a code owner December 18, 2018 11:22
@ruflin ruflin changed the title Fix issue with ILM that write index was created before template was l… Fix issue with ILM that write index was created before template was loaded Dec 18, 2018
@ruflin ruflin force-pushed the add-ilm-test branch 2 times, most recently from 778acb5 to 2400d39 Compare December 18, 2018 13:13
…oaded

The write index for ILM was created before the template was loaded. This meant the first template did not contain all the mappings and policy for ILM so was never rolled over. This change makes sure the template is loaded for ILM is setup up and adds a test to confirm it.

Further ILM was not respected when running `metricbeat setup --template`. This is now changed too.
Copy link
Copy Markdown
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

Test LGTM to me but I would prefer not having a sleep if possible. :(

logp.Info("Set settings.index.lifecycle.name in template to %s as ILM is enabled.", ILMPolicyName)
err = b.Config.Template.SetString("settings.index.lifecycle.name", -1, ILMPolicyName)
if err != nil {
return errw.Wrap(err, "error setting settings.index.lifecycle.name")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for making the function smaller.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're welcome, but I think we need further refactoring to make it really nice ;-)

proc.check_kill_and_wait()

# Give time to do the rollovers
time.sleep(2)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not a fan of sleep(x) and then assert a condition, probably because I have always ran into issue involving this pattern. I would prefer a fails / sleep / retry mechanism if possible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, I think I found a nicer solution. See 869637b

@ph
Copy link
Copy Markdown
Contributor

ph commented Dec 18, 2018

This is better.

@ruflin
Copy link
Copy Markdown
Contributor Author

ruflin commented Dec 18, 2018

jenkins, test this

@ruflin ruflin merged commit 36cee51 into elastic:master Dec 18, 2018
@ruflin ruflin deleted the add-ilm-test branch December 18, 2018 17:01
ruflin added a commit to ruflin/beats that referenced this pull request Dec 18, 2018
…oaded (elastic#9617)

The write index for ILM was created before the template was loaded. This meant the first template did not contain all the mappings and policy for ILM so was never rolled over. This change makes sure the template is loaded for ILM is setup up and adds a test to confirm it.

Further ILM was not respected when running `metricbeat setup --template`. This is now changed too.



(cherry picked from commit 36cee51)
@ruflin ruflin added v6.6.0 and removed needs_backport PR is waiting to be backported to other branches. labels Dec 18, 2018
ruflin added a commit that referenced this pull request Dec 18, 2018
…oaded (#9617) (#9628)

The write index for ILM was created before the template was loaded. This meant the first template did not contain all the mappings and policy for ILM so was never rolled over. This change makes sure the template is loaded for ILM is setup up and adds a test to confirm it.

Further ILM was not respected when running `metricbeat setup --template`. This is now changed too.



(cherry picked from commit 36cee51)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants