Allow YamlTemplateRegistry to fallback to TemplateResources #112615

lahsivjar · 2024-09-06T19:38:02Z

The PR refactors YamlTemplateRegistry to load from TemplateResources for locating resource files and fallbacks to the plugin-module itself if it can't locate the file in the TemplateResources (more details are in the java doc). It also introduces a convention to use the named-module's name as the folder name in the TemplateResource to keep interfaces a bit simple.

Related to

#112137
elastic/apm-server#13898

lahsivjar · 2024-09-06T19:39:03Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/ResourceUtils.java

        InputStream is = clazz.getResourceAsStream(name);
+        if (is == null) {
+            is = TemplateResources.class.getResourceAsStream("/"+clazz.getPackageName()+name);


[For reviewers] I decided to use the package name as the folder in the template-resources so that we don't have to bloat the interface with fallback root path. Let me know if there is a better suggestion to do this.

Just to make sure I understand: this would mean that we'd have something like this on disk?

/path/to/override/template-resources/org.elasticsearch.xpack.apmdata/component-templates/whatever.yaml

Is that right? Do plugins live in their own Java module? If so would it make sense to use the module name, rather than the package name?

Is it possible to add a YAML REST test to apm-data showing the overrides working?

Should it be the other way around? i.e. look for an override in template-resources first, and in the plugin second?

Hmm, this does make sense w.r.t. what the template resource is meant to do. I will make the changes.

Is that right?

Yes, I have a sample for this in https://github.com/elastic/elasticsearch/pull/112614/files

Do plugins live in their own Java module?

Not sure if I understand "Java module" correctly and my Java9++ is not that good but from what I can see, we haven't modularized apm-data plugin. I think it should be as simple as adding the correct module-info. If we do that, then we can use the module name. I will give it a shot.

Is it possible to add a YAML REST test to apm-data showing the overrides working?

We are not overriding in this PR, the fallback files are required by the plugin, and failing to locate them would be fatal for starting ES -- so, they are already be covered by the tests. Are you thinking about adding explicit tests to check if all the files are installed?

Maybe I'm totally misunderstanding the approach. What I thought we were going to do is:

Add the "-fallback@lifecycle" component templates in the plugin, in a way that would work with "stateful" Elasticsearch OOTB

Allow these to be overridden by placing a component template of the same name in template-resources; if that exists, prefer using that

(I was not expecting to see any templates added to template-resources in #112614)

In terms of YAML REST tests, I was thinking we would:

Add test cases for the scenarios laid out in New indexes created for datastreams after update to 8.15.0 are without lifecycle policies apm-server#13898 (comment)

Add another test case that adds an override

... but thinking about it some more, I'm not sure if (2) is possible. I had in mind that the overrides were on disk, but I think they're also embedded in a jar? Just it's a different jar in serverless vs. stateful.

Allow these to be overridden by placing a component template of the same name in template-resources; if that exists, prefer using that

Ah, I see the misunderstanding. The overrides for serverless are performed with files in https://github.com/elastic/elasticsearch-serverless/tree/main/libs/serverless-xpack-template-resources. I am not sure if we can override for serverless using template-resources (not sure how we can trigger the if/else condition i.e. if we are on serverless load this...). The approach suggested to us is to provide the fallback to ILM with the default apm-data plugin and override the fallback files for serverless to be empty.

... but thinking about it some more, I'm not sure if (2) is possible. I had in mind that the overrides were on disk, but I think they're also embedded in a jar? Just it's a different jar in serverless vs. stateful.

Exactly. Also, we could have added test if some version of apm-data supported ILM (by upgrading from that version) but with the current state of things, it is not straightforward and will require creating datastreams with ILMs and then applying the apm-data plugin... BTW, the current PR is only about introducing the fallback mechanism for loading template resources in YamlTemplateRegistry, we can discuss more on the tests in the PR where we introduce the ILM files.

If so would it make sense to use the module name, rather than the package name?

I pushed a commit to use the module name (ref). Note that to make this work we will need to add module-info for apm-data (similar to #95057) -- I am not sure if this is better though.

I think this looks good. I think using the module name should be a bit less brittle than the package name. Thinking again, ideally we'd use the plugin name. I'm not aware of a way to get that dynamically.

axw

The PR refactors YamlTemplateRegistry to fallback to TemplateResources for locating resource files if the plugin-module itself doesn't have the resources.

Should it be the other way around? i.e. look for an override in template-resources first, and in the plugin second?

axw · 2024-09-09T01:53:02Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/ResourceUtils.java

        InputStream is = clazz.getResourceAsStream(name);
+        if (is == null) {
+            is = TemplateResources.class.getResourceAsStream("/"+clazz.getPackageName()+name);


Just to make sure I understand: this would mean that we'd have something like this on disk?

/path/to/override/template-resources/org.elasticsearch.xpack.apmdata/component-templates/whatever.yaml

Is that right? Do plugins live in their own Java module? If so would it make sense to use the module name, rather than the package name?

Is it possible to add a YAML REST test to apm-data showing the overrides working?

elasticsearchmachine · 2024-09-10T03:43:32Z

Pinging @elastic/es-data-management (Team:Data Management)

gmarouli

Hi @lahsivjar , thanks for picking this up. This is slightly different than I expected and I am curious about the following:

Have you considered to move all the APM resources to the template-resource so it can follow the same way of working like all the other resources, if yes, what stopped you from following that path?
I have concerns about the module name convention clazz.getModule().getName(). I am concerned that it introduces an implicit dependency. If you rename the module and you forget that this is used in a resource folder in a different place, it will break. Furthermore, the rest of the folders are not named like that, and it has the benefit that the names are shorter and easier to navigate.
I am curious about your thoughts.

lahsivjar · 2024-09-11T08:43:37Z

Have you considered to move all the APM resources to the template-resource so it can follow the same way of working like all the other resources, if yes, what stopped you from following that path?

I discussed this with @axw and we wanted to keep the resources in the plugin module itself as it is easier to maintain and manage. @axw do you have any other points to add here?

I have concerns about the module name convention clazz.getModule().getName(). I am concerned that it introduces an implicit dependency. If you rename the module and you forget that this is used in a resource folder in a different place, it will break

I think having the resource files in 2 places is an implicit dependency by itself. Also, since the folder structure inside template resources needs to follow what we would have in overrides, the same case kinda applies (if we rename a folder here we could end up breaking serverless). I think the better way would be to cover this via serverless tests, would that be possible?

gmarouli · 2024-09-11T11:57:58Z

@lahsivjar Quick question have you tested this with serverless? To see that it works, I am asking because #112614 has failed tests.

gmarouli · 2024-09-11T12:46:26Z

@lahsivjar I entertaining a new idea.

Since in this case we do not want to override the component templates, we want them to not be there at all. What if we override the

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/YamlTemplateRegistry.java

Line 150 in 079cc34

public Map<String, ComposableIndexTemplate> getComposableTemplateConfigs() {

to exclude the ILM policies? We could use the setting

elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/DataStreamLifecycle.java

Line 59 in bed6e18

    
           public static final String DATA_STREAMS_LIFECYCLE_ONLY_SETTING_NAME = "data_streams.lifecycle_only.mode";

to know when to exclude the ilm files.

It's a new way of doing this, but I think it fits because this is using a setting to detect if ILM is supported and if not exclude these component templates. It removes the need of duplication of templates and introducing a lot of empty ones.

Thoughts?

lahsivjar · 2024-09-11T13:21:01Z

@lahsivjar Quick question have you tested this with serverless? To see that it works, I am asking because #112614 has failed tests.

I haven't added the PR for overriding files yet so it will fail for now.

It's a new way of doing this, but I think it fits because this is using a setting to detect if ILM is supported and if not exclude these component templates. It removes the need of duplication of templates and introducing a lot of empty ones.

Thoughts?

I love the idea, let me see if I can make this work!

lahsivjar · 2024-09-11T17:52:30Z

@gmarouli Thanks for the suggestion, I have created an alternative PR: #112759 (created a separate PR since the idea was completely different).

lahsivjar · 2024-09-16T13:40:18Z

Closing this, superseded by #112759

Allow YamlTemplateRegistry to fallback to TemplateResources

89dc9c1

lahsivjar requested a review from axw September 6, 2024 19:38

elasticsearchmachine added v8.16.0 needs:triage Requires assignment of a team area label labels Sep 6, 2024

lahsivjar commented Sep 6, 2024

View reviewed changes

axw reviewed Sep 9, 2024

View reviewed changes

Use module name as folder location

9433ac6

axw approved these changes Sep 10, 2024

View reviewed changes

axw added the :Data Management/Data streams Data streams and their lifecycles label Sep 10, 2024

elasticsearchmachine added Team:Data Management Meta label for data/management team and removed needs:triage Requires assignment of a team area label labels Sep 10, 2024

lahsivjar added the >bug label Sep 10, 2024

apply spotlessJava

51c64c2

lahsivjar requested a review from a team September 10, 2024 11:32

gmarouli reviewed Sep 11, 2024

View reviewed changes

mark-vieira added v9.0.0 and removed v8.16.0 labels Sep 11, 2024

lahsivjar mentioned this pull request Sep 11, 2024

[plugin/apm-data]Fallback to ILM for non-dsl-only mode if DSL unavailable #112759

Merged

lahsivjar closed this Sep 16, 2024

lahsivjar deleted the yamltemplateregistry branch September 16, 2024 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow YamlTemplateRegistry to fallback to TemplateResources #112615

Allow YamlTemplateRegistry to fallback to TemplateResources #112615

lahsivjar commented Sep 6, 2024 •

edited

Loading

lahsivjar Sep 6, 2024

axw Sep 9, 2024

lahsivjar Sep 9, 2024

axw Sep 9, 2024

lahsivjar Sep 9, 2024

lahsivjar Sep 9, 2024 •

edited

Loading

axw Sep 10, 2024 •

edited

Loading

axw left a comment

axw Sep 9, 2024

elasticsearchmachine commented Sep 10, 2024

gmarouli left a comment

lahsivjar commented Sep 11, 2024

gmarouli commented Sep 11, 2024 •

edited

Loading

gmarouli commented Sep 11, 2024 •

edited

Loading

lahsivjar commented Sep 11, 2024

lahsivjar commented Sep 11, 2024

lahsivjar commented Sep 16, 2024

Allow YamlTemplateRegistry to fallback to TemplateResources #112615

Allow YamlTemplateRegistry to fallback to TemplateResources #112615

Conversation

lahsivjar commented Sep 6, 2024 • edited Loading

Related to

lahsivjar Sep 6, 2024

Choose a reason for hiding this comment

axw Sep 9, 2024

Choose a reason for hiding this comment

lahsivjar Sep 9, 2024

Choose a reason for hiding this comment

axw Sep 9, 2024

Choose a reason for hiding this comment

lahsivjar Sep 9, 2024

Choose a reason for hiding this comment

lahsivjar Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

axw Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

axw left a comment

Choose a reason for hiding this comment

axw Sep 9, 2024

Choose a reason for hiding this comment

elasticsearchmachine commented Sep 10, 2024

gmarouli left a comment

Choose a reason for hiding this comment

lahsivjar commented Sep 11, 2024

gmarouli commented Sep 11, 2024 • edited Loading

gmarouli commented Sep 11, 2024 • edited Loading

lahsivjar commented Sep 11, 2024

lahsivjar commented Sep 11, 2024

lahsivjar commented Sep 16, 2024

lahsivjar commented Sep 6, 2024 •

edited

Loading

lahsivjar Sep 9, 2024 •

edited

Loading

axw Sep 10, 2024 •

edited

Loading

gmarouli commented Sep 11, 2024 •

edited

Loading

gmarouli commented Sep 11, 2024 •

edited

Loading