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

Add migration support to the event log #58010

Merged
merged 8 commits into from
Feb 25, 2020

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Feb 19, 2020

Resolves #55639.

In this PR, I'm namespacing the event log by version similar to APM, Beats, etc.

Example: in 8.0.0, the event log index template will be named .kibana-event-log-8.0.0-template that will create indicies like .kibana-event-log-8.0.0-000001.

Whenever the version changes, Kibana will create a new template index and alias (ex: .kibana-event-log-8.1.0-000001). We can still query on the index pattern .kibana-event-log* for data but write to a specific version / alias that is pulled from package.json.

It was also recommended to not define the alias in the template but rather in the index (https://discuss.elastic.co/t/index-lifecycle-management-does-not-point-to-index-error/211513/2).

The ILM policy will only be created once and will stop changing after being created. This is by design to keep user changes intact if ever they modify the ILM policy and want it to apply to future releases.

@mikecote mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Feb 19, 2020
@mikecote mikecote self-assigned this Feb 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@mikecote mikecote marked this pull request as ready for review February 21, 2020 17:42
@mikecote mikecote requested a review from a team as a code owner February 21, 2020 17:42
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mikecote mikecote added review and removed review labels Feb 24, 2020
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM, added some esoteric questions :-)

@@ -173,6 +173,7 @@ describe('createIndex', () => {
await clusterClientAdapter.createIndex('foo');
expect(clusterClient.callAsInternalUser).toHaveBeenCalledWith('indices.create', {
index: 'foo',
body: {},
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why we added the extra body here. I was frankly curious I could create an index WITHOUT a document to begin with, but I'm wondering if this change was deliberate, and if so, why was it done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to set additional settings when creating the initial index. I need this to setup the alias when creating the initial index now that it's not done with the index template anymore.

Reference: https://github.com/elastic/kibana/pull/58010/files#diff-88aee8214a0ea2006f946be367b1d715R66

@@ -10,10 +10,7 @@ import mappings from '../../generated/mappings.json';
// returns the body of an index template used in an ES indices.putTemplate call
export function getIndexTemplate(esNames: EsNames) {
const indexTemplateBody: any = {
index_patterns: [esNames.indexPattern],
aliases: {
Copy link
Member

Choose a reason for hiding this comment

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

I guess the aliases aren't really needed here, since it's already set up in the index.lifecycle.rollover_alias property below this?

I guess my only worry is that if it rolls over for some other reason, that didn't take the that ilm property into account, the aliases could get into some funky state. Not sure if it even CAN roll over for some other reason. :-).

One of the nice things about this version, is that if a rando user creates an index matching our template, it WON'T get aliased in for free, which is likely what we want to happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I recall, with the alias definition here, there would be ambiguity finding the write index after a second one is created. After doing some research, it was recommended to only setup is_write_index on the initial index and ILM would then handle the changeover automatically on rollover.

Reference: https://discuss.elastic.co/t/index-lifecycle-management-does-not-point-to-index-error/211513/2

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

@mikecote mikecote merged commit 2943978 into elastic:master Feb 25, 2020
mikecote added a commit to mikecote/kibana that referenced this pull request Feb 25, 2020
* Initial work

* Add missing file

* Add tests where missing

* Add kibana version to esNames

* Share ILM policy across versions
mikecote added a commit that referenced this pull request Feb 26, 2020
* Initial work

* Add missing file

* Add tests where missing

* Add kibana version to esNames

* Share ILM policy across versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes review Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[alerting event log] deal with migrating event log mappings over releases
5 participants