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

introduce new indices key with multi ilm and template handling #10002

Closed
wants to merge 9 commits into from

Conversation

simitt
Copy link
Contributor

@simitt simitt commented Jan 10, 2019

As discussed offline, this WIP PR is a start to address configuration changes related to index, template, ilm handling. Since this is a larger topic I created a feature branch feature-multi-ilm against which we can create PRs.

related to ##9919

Points to cover:

  • Introduce new config options with root key indices. Deprecate config options setup.template, output.elasticsearch.index, output.elasticsearch.indices. Remove beta config option ilm. See new config options example.
  • Allow multiple Templates for one beat, by defining a template per index.
  • Allow multiple rollover policies, one per index/rollover_alias.
  • Ensure output config in pipeline uses the new indices configuration if available. Currently an index is always set, even when indices are configured, as a fallback when no condition applies. In the future one index that doesn't have a condition must be set in indices, which is used as the default index. If ILM is enabled, the rollover_alias must be used instead of the name.
  • Check for dependencies: Can/should the template.name and template.pattern be derived from the rollover_alias when ILM is enabled? Which fields are required to be set per indices entry?
  • Should ILM policy be overwritten when trying to load multiple times, or should it be configurable?
  • Add tests and fix system tests

@urso, @ph, @elastic/apm-server this PR is not ready for a detailed review, but I'd appreciate high-level feedback.

UPDATE:

  • cherry-pick policy handling from @urso 's implementation for 6.x
  • fix test for default yml config without indices, output.elasticsearch.index and setup.template section
  • @urso I introduced an index.DefaultConfig setting so every beat can set its default index from outside. This is specifically helpful for apm-server (see Add indices defaults in code apm-server#1480). I am not exactly clear on what the default configurations for all the beats should be (especially with ECS changes). I'd appreciate if someone else could update the configs and overwrite the index.DefaultConfig if necessary. I tried to update the config for metricbeat myself.

@simitt simitt requested a review from a team as a code owner January 10, 2019 17:21
libbeat/template/config.go Outdated Show resolved Hide resolved
libbeat/template/config.go Outdated Show resolved Hide resolved
libbeat/template/config.go Outdated Show resolved Hide resolved
libbeat/index/config.go Outdated Show resolved Hide resolved
libbeat/index/config.go Show resolved Hide resolved
libbeat/ilm/load.go Outdated Show resolved Hide resolved
libbeat/ilm/config.go Outdated Show resolved Hide resolved
libbeat/ilm/config.go Outdated Show resolved Hide resolved
libbeat/ilm/config.go Outdated Show resolved Hide resolved
libbeat/ilm/config.go Outdated Show resolved Hide resolved
@simitt simitt force-pushed the multi-ilm-multi-templates branch from 02b3c3d to e4a76b1 Compare January 16, 2019 15:53
@simitt simitt requested review from a team as code owners January 16, 2019 15:53
@simitt simitt changed the base branch from feature-multi-ilm to master January 16, 2019 15:55
libbeat/_meta/config.reference.yml Outdated Show resolved Hide resolved
libbeat/_meta/config.reference.yml Outdated Show resolved Hide resolved
libbeat/_meta/config.reference.yml Show resolved Hide resolved
libbeat/cmd/instance/beat.go Outdated Show resolved Hide resolved
libbeat/ilm/config.go Outdated Show resolved Hide resolved
libbeat/index/config.go Outdated Show resolved Hide resolved
libbeat/cmd/export/ilm_policy.go Show resolved Hide resolved
os.Exit(1)
}
logp.Info("Loaded Elasticsearch templates.")
},
Copy link

Choose a reason for hiding this comment

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

We will have to clean this up as well, so to not print all templates to stdout, right?

We print the templates, we do not load anything. Maybe we shouldn't print Loaded Elasticsearch templates. once we are 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.

We will have to clean this up as well, so to not print all templates to stdout, right?

We print exactly the templates that would be loaded - I think this behaves as expected.

We print the templates, we do not load anything. Maybe we shouldn't print Loaded Elasticsearch templates. once we are done.

agreed

libbeat/cmd/instance/beat.go Outdated Show resolved Hide resolved
return false
}

code, body, err := client.Request("GET", "/_xpack", "", nil, nil)
Copy link

Choose a reason for hiding this comment

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

hm... how do we get the info once _xpack is removed from the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, but this check is not introduced newly here. Can you link to the according issue where _xpack should be removed, so I can follow up.

@simitt
Copy link
Contributor Author

simitt commented Jan 18, 2019

@urso I am closing this for now to avoid unnecessary noise, will reopen when it is final.

@simitt simitt closed this Jan 18, 2019
*m = ModeAuto
case "true", "True", "yes", "y", "Yes":
*m = ModeEnabled
case "false", "False", "no", "n", "No":
Copy link
Contributor

Choose a reason for hiding this comment

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

This is inconstant with the rest of beats we do not support all the boolean possibilities.

lets use an explicit "auto", "false", "true"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only lowercase or also uppercase?

libbeat/ilm/config_test.go Outdated Show resolved Hide resolved
libbeat/ilm/load.go Outdated Show resolved Hide resolved
libbeat/ilm/policy.go Outdated Show resolved Hide resolved
libbeat/ilm/policy.go Outdated Show resolved Hide resolved
libbeat/index/config.go Outdated Show resolved Hide resolved
libbeat/index/loader.go Outdated Show resolved Hide resolved
libbeat/index/loader.go Outdated Show resolved Hide resolved
libbeat/index/loader.go Outdated Show resolved Hide resolved
@ph
Copy link
Contributor

ph commented Jan 18, 2019

@simitt I have still added a few notes to this PR, I will do a final review when its reopen.

@simitt
Copy link
Contributor Author

simitt commented Jan 18, 2019

great, thanks for the feedback @ph !

@simitt simitt reopened this Jan 22, 2019
@simitt simitt force-pushed the multi-ilm-multi-templates branch from 662d763 to f157a98 Compare January 22, 2019 10:56
@simitt simitt changed the title [WIP] [ILM] introduce new indices key with multi ilm and template handling [ILM] introduce new indices key with multi ilm and template handling Jan 22, 2019
@simitt simitt changed the title [ILM] introduce new indices key with multi ilm and template handling introduce new indices key with multi ilm and template handling Jan 22, 2019
@simitt simitt added the review label Jan 22, 2019
@simitt
Copy link
Contributor Author

simitt commented Jan 22, 2019

@ph, @urso this is ready for review. I updated the description above with some open questions/points.

cc @elastic/apm-server

@simitt simitt force-pushed the multi-ilm-multi-templates branch from f157a98 to 81ebf66 Compare January 22, 2019 11:11
@simitt
Copy link
Contributor Author

simitt commented Jan 22, 2019

Closing this PR as discussions around how to organise configuration settings properly in the future are still going on.

@cassandracomar
Copy link

Hi @simitt , is this work likely to be merged at any point? Not having this prevents us from being able to use ILM altogether as sending all of our cluster logs to the same index is introducing too many differing fields depending on the source of the log, leaving us with a Kibana that can't determine what fields are in our index patterns. We're disabling ILM for now in favor of using curator to do the same work for us but it looks like curator is being obsoleted over time in favor of ILM.

@simitt
Copy link
Contributor Author

simitt commented Aug 19, 2019

Handing off this question to @urso, as the ILM related index handling for APM and beats developed differently.

@simitt simitt deleted the multi-ilm-multi-templates branch August 4, 2020 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants