-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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 ignore_missing_component_templates
config option
#92436
Add ignore_missing_component_templates
config option
#92436
Conversation
As described in elastic#92426 a config option to not ignore missing component templates is needed. This introduces the config option `ignore_missing_component_templates`. If set to true, missing component templates are ignored. If set to false or not set at all, the existing behaviour applies. This is currently a draft PR as it only contains the functionality. It still needs tests and docs. Goal is to get an initial set of opinions on it.
server/src/main/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplate.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplate.java
Outdated
Show resolved
Hide resolved
ignore_missing_component_tempaltes
config optionignore_missing_component_templates
config option
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
Outdated
Show resolved
Hide resolved
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
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.
Left some comments and suggestions that should help the code snippets CI tests pass.
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
@dakrone Who would be a good person to have a first read of this PR. I would like to know if the approach I'm following is the expected path as this is has been my first ES PR in quite some time. |
What exactly is the |
.../src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml
Outdated
Show resolved
Hide resolved
Update: Found the issue. The bot put two areas into the list but seems only 1 is allowed. The
But it is not clear to me why the changelog is invalid? |
@dakrone Thanks for the review. I did a first round of changes and by now also CI is happy. Could you take another look? |
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.
Okay, I left a few more minor comments, but this is pretty close.
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
.../src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml
Show resolved
Hide resolved
.../src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml
Outdated
Show resolved
Hide resolved
.../src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml
Outdated
Show resolved
Hide resolved
+ name | ||
+ "] specifies a missing component templates [" | ||
+ missingComponentTemplate | ||
+ "] that does not exist and is not part of 'ignore_missing_component_templates'" |
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.
If it is used with a non empty list but template is not found, we leave a not in the logs.
I'm not sure what you mean by this, if it has a non-empty list and the template is not found, we should throw an exception like this currently does (rather than logging anything).
server/src/test/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplateTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplateTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java
Outdated
Show resolved
Hide resolved
…dices.put_index_template/15_composition.yml Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>
…ableIndexTemplateTests.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>
docs/reference/indices/ignore-missing-component-templates.asciidoc
Outdated
Show resolved
Hide resolved
.../src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml
Outdated
Show resolved
Hide resolved
.../src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java
Outdated
Show resolved
Hide resolved
docs/changelog/92436.yaml
Outdated
@@ -0,0 +1,7 @@ | |||
pr: 92436 | |||
summary: Add `ignore_missing_component_templates` config option | |||
# The bot was creating `area: "Data streams, Indices APIs"` but this seems to be invalid |
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.
@dakrone I'll remove this line but maybe worth someone checks on why this is happening. It seems the bot can't handle 2 labels well (or the other way around).
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.
That's because PRs only support a single green label, so the entry does not get duplicated in the changelog. This change would be indices APIs more than data streams (you don't have to change the labels, since you fixed this.)
@@ -505,17 +505,51 @@ public static void validateV2TemplateRequest(Metadata metadata, String name, Com | |||
} | |||
|
|||
final Map<String, ComponentTemplate> componentTemplates = metadata.componentTemplates(); | |||
final List<String> ignoreMissingComponentTemplates = ( | |||
template.getIgnoreMissingComponentTemplates() == null? List.of():template.getIgnoreMissingComponentTemplates() |
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 was going forth and back on this one. Either I check for null in getIgnoreMissingComponentTemplates
and always return an empty list or do it here. It seems for non requirement params null
is in many places and excepted value. Also it put a bit a challenge in many other places that expect the entry to not show up by default if it is entry. So I do the check here.
@elasticmachine run elasticsearch-ci/part-1 |
This comment was marked as outdated.
This comment was marked as outdated.
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.
LGTM, thanks for iterating on this @ruflin, I'll merge it in.
@dakrone Thanks for the reviews and feedback! Looking forward to my future contributions ;-) |
This change introduces the configuration option `ignore_missing_component_templates` as discussed in elastic#92426 The implementation [option 6](elastic#92426 (comment)) was picked with a slight adjustment meaning no patterns are allowed. ## Implementation During the creation of an index template, the list of component templates is checked if all component templates exist. This check is extended to skip any component templates which are listed under `ignore_missing_component_templates`. An index template that skips the check for the component template `logs-foo@custom` looks as following: ``` PUT _index_template/logs-foo { "index_patterns": ["logs-foo-*"], "data_stream": { }, "composed_of": ["logs-foo@package", "logs-foo@custom"], "ignore_missing_component_templates": ["logs-foo@custom"], "priority": 500 } ``` The component template `logs-foo@package` has to exist before creation. It can be created with: ``` PUT _component_template/logs-foo@custom { "template": { "mappings": { "properties": { "host.ip": { "type": "ip" } } } } } ``` ## Testing For manual testing, different scenarios can be tested. To simplify testing, the commands from `.http` file are added. Before each test run, a clean cluster is expected. ### New behaviour, missing component template With the new config option, it must be possible to create an index template with a missing component templates without getting an error: ``` ### Add logs-foo@package component template PUT http://localhost:9200/ _component_template/logs-foo@package Authorization: Basic elastic password Content-Type: application/json { "template": { "mappings": { "properties": { "host.name": { "type": "keyword" } } } } } ### Add logs-foo index template PUT http://localhost:9200/ _index_template/logs-foo Authorization: Basic elastic password Content-Type: application/json { "index_patterns": ["logs-foo-*"], "data_stream": { }, "composed_of": ["logs-foo@package", "logs-foo@custom"], "ignore_missing_component_templates": ["logs-foo@custom"], "priority": 500 } ### Create data stream PUT http://localhost:9200/ _data_stream/logs-foo-bar Authorization: Basic elastic password Content-Type: application/json ### Check if mappings exist GET http://localhost:9200/ logs-foo-bar Authorization: Basic elastic password Content-Type: application/json ``` It is checked if all templates could be created and data stream mappings are correct. ### Old behaviour, with all component templates In the following, a component template is made optional but it already exists. It is checked, that it will show up in the mappings: ``` ### Add logs-foo@package component template PUT http://localhost:9200/ _component_template/logs-foo@package Authorization: Basic elastic password Content-Type: application/json { "template": { "mappings": { "properties": { "host.name": { "type": "keyword" } } } } } ### Add logs-foo@custom component template PUT http://localhost:9200/ _component_template/logs-foo@custom Authorization: Basic elastic password Content-Type: application/json { "template": { "mappings": { "properties": { "host.ip": { "type": "ip" } } } } } ### Add logs-foo index template PUT http://localhost:9200/ _index_template/logs-foo Authorization: Basic elastic password Content-Type: application/json { "index_patterns": ["logs-foo-*"], "data_stream": { }, "composed_of": ["logs-foo@package", "logs-foo@custom"], "ignore_missing_component_templates": ["logs-foo@custom"], "priority": 500 } ### Create data stream PUT http://localhost:9200/ _data_stream/logs-foo-bar Authorization: Basic elastic password Content-Type: application/json ### Check if mappings exist GET http://localhost:9200/ logs-foo-bar Authorization: Basic elastic password Content-Type: application/json ``` ### Check old behaviour Ensure, that the old behaviour still exists when a component template is used that is not part of `ignore_missing_component_templates`: ``` ### Add logs-foo index template PUT http://localhost:9200/ _index_template/logs-foo Authorization: Basic elastic password Content-Type: application/json { "index_patterns": ["logs-foo-*"], "data_stream": { }, "composed_of": ["logs-foo@package", "logs-foo@custom"], "ignore_missing_component_templates": ["logs-foo@custom"], "priority": 500 } ``` Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>
This change introduces the configuration option
ignore_missing_component_templates
as discussed in #92426 The implementation option 6 was picked with a slight adjustment meaning no patterns are allowed.Implementation
During the creation of an index template, the list of component templates is checked if all component templates exist. This check is extended to skip any component templates which are listed under
ignore_missing_component_templates
. An index template that skips the check for the component templatelogs-foo@custom
looks as following:The component template
logs-foo@package
has to exist before creation. It can be created with:Testing
For manual testing, different scenarios can be tested. To simplify testing, the commands from
.http
file are added. Before each test run, a clean cluster is expected.New behaviour, missing component template
With the new config option, it must be possible to create an index template with a missing component templates without getting an error:
It is checked if all templates could be created and data stream mappings are correct.
Old behaviour, with all component templates
In the following, a component template is made optional but it already exists. It is checked, that it will show up in the mappings:
Check old behaviour
Ensure, that the old behaviour still exists when a component template is used that is not part of
ignore_missing_component_templates
: