-
Notifications
You must be signed in to change notification settings - Fork 306
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 injection vs template_api setting #1115
base: main
Are you sure you want to change the base?
ILM injection vs template_api setting #1115
Conversation
d06ce4f
to
0db4015
Compare
Setting this flag to `composable` will use index template API to create index template. | ||
|
||
NOTE: The format of template provided to <<plugins-{type}s-{plugin}-template>> needs to match the template API being used. | ||
* Value can be any of: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review note: I intentionally don't list auto
here, because it is not particularly meaningful since it is already the default. Instead, I reveal that the default behavior is the same as either composable
or legacy
depending on which version of Elasticsearch we are connected to.
@@ -279,6 +279,8 @@ class LogStash::Outputs::ElasticSearch < LogStash::Outputs::Base | |||
def initialize(*params) | |||
super | |||
setup_ecs_compatibility_related_defaults | |||
|
|||
raise LogStash::ConfigurationError("`template_api` is not allowed unless a `template` is also provided") if original_params.include?('template_api') && !original_params.include?('template') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very very small breaking risk. The intent of template_api
was always as a hint of the template that was being provided, and never successfully controlled which API was used for the default template. It is possible that a user explicitly added a meaningless template_api
hint without a template
that happened to align with the default behaviour that did work, but even these cases we should reject the config helpfully.
if plugin.template | ||
if plugin.maximum_seen_major_version < 8 && plugin.template_api == 'auto' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review note: since template_api
is a hint about template
, only check-and-emit this warning if the user has provided a template
@@ -42,19 +42,29 @@ def self.install(client, template_endpoint, template_name, template, template_ov | |||
end | |||
|
|||
def self.add_ilm_settings_to_template(plugin, template) | |||
if plugin.template | |||
plugin.deprecation_logger.deprecated("Injecting Index Lifecycle Management configuration into a provided `template` is deprecated, and support will be removed in a future version. Please add the configuration directly to your template.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with most of changes but this message brings one concern to me. We are asking user to add the configuration directly to their template
. However, we are overwriting their template index.lifecycle.name
and index.lifecycle.rollover_alias
ILM settings (template_manager.rb
L53~L56) with plugin ILM settings, that means user custom template is updated even they add configurations to their template. Both index.lifecycle.name
and index.lifecycle.rollover_alias
have default values which may differ from user custom template ILM settings.
I was thinking following two options, they may be applied together but choosing one would simplify:
- Clear message which should highlight that we will opt out automatic setting but for now, to set same ILM configs int both plugin and template. I would not consider this option as it is manual effort and still under the risk that plugin still automatically applies
index.lifecycle.name
andindex.lifecycle.rollover_alias
to the custom template.
plugin.deprecation_logger.deprecated("Injecting Index Lifecycle Management configuration into a provided `template` is deprecated, and support will be removed in a future version. Please add the configuration directly to your template.") | |
plugin.deprecation_logger.deprecated("Injecting Index Lifecycle Management configuration into a provided `template` is deprecated, and support will be removed in a future version. Please add `index.lifecycle.name` and `index.lifecycle.rollover_alias` ILM configurations directly to your template as well as `ilm_policy` and `ilm_rollover_alias` in plugin configs.") |
- We automatically set the ILM
index.lifecycle.name
andindex.lifecycle.rollover_alias
only IFF user custom template doesn't contain these settings. We also _info_rm user that we are using custom template ILM configs as it is and ignoring plugin level config. With this approach, we achieve backward compatibility. I do believe that the users who touch these settings in the custom template, mostly are knowledgable about ILM policy, rollover, etc..
In my opinion, option-2 looks better approach.
Let me please know if I am missing (or misunderstanding) anything.
"but will resolve to `composable` the first time it connects to Elasticsearch 8+. " + | ||
"We recommend either setting `template_api => legacy` to continue providing legacy-style templates, " + | ||
"or migrating your template to the composable style and setting `template_api => composable`. " + | ||
"The legacy template API is slated for removal in Elasticsearch 9.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that Elasticsearch 9
is not newly added with change but I would prefer to change it to upcoming Elasticsearch versions
(?!) as I couldn't find Elasticsearch 9
relative context in the public docs.
This is a spike that attempts to convey meaningully what I suggested in an issue comment of #1102 to solve #1108.