Optimize usage calculation in ILM policies retrieval API

[nielsbauman]

Optimize calculating the usage of ILM policies in the GET _ilm/policy and GET _ilm/policy/<policy_id> endpoints. I loaded some dummy data into my single-node locally-running ES cluster using the following script (numbers are inspired by a customer who was facing timeouts in Kibana due to this endpoint being slow):

Bash snippet

#!/bin/bash ES_URL="${ES_URL:-http://localhost:9200}" NR_POLICIES=42 NR_COMPONENT_TEMPLATES=700 NR_INDEX_TEMPLATES=8000 NR_DATA_STREAMS=1000 NR_INDICES=1000 for (( i=0; i<NR_POLICIES; i++ )); do curl -s -X PUT "$ES_URL/_ilm/policy/policy-$i" -H 'Content-Type: application/json' -d '  {  "policy": {  "phases": {}  }  }  ' > /dev/null done for (( i=0; i<NR_COMPONENT_TEMPLATES; i++ )); do curl -s -X PUT "$ES_URL/_component_template/component-template-$i" -H 'Content-Type: application/json' -d '  {"template": {}}  ' > /dev/null done for (( i=0; i<NR_INDEX_TEMPLATES; i++ )); do component_template=$((RANDOM % NR_COMPONENT_TEMPLATES)) curl -s -X PUT "$ES_URL/_index_template/index-template-$i" -H 'Content-Type: application/json' -d "  {  \"index_patterns\": [\"index-pattern-$i\"],  \"template\": {},  \"composed_of\": [\"component-template-$component_template\"],  \"data_stream\": {}  }  " > /dev/null done for (( i=0; i<NR_DATA_STREAMS; i++ )); do curl -s -X PUT "$ES_URL/_data_stream/index-pattern-$i" > /dev/null done for (( i=0; i<NR_INDICES; i++ )); do curl -s -X PUT "$ES_URL/index-$i" > /dev/null done

I also played around with some aspects of the script such as making the templates use policies or not, as that represents different scenarios that users might face.

I generated a flamegraph on main, which showed that the MetadataIndexTemplateService.findV2Template(...) call was by far the most expensive.

Flamegraph

After some experimentation, I ended up extracting a separate class that pre-computes some parts on initialization (i.e. only once per request) and then uses those pre-computed parts when calculating the usage for an individual policy. This is of course a trade-off between memory usage and run-time performance. But, I think the added memory usage (which shouldn't be crazy much) is worth the significant speed improvement here.

Fixes #105773

[elasticsearchmachine]

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

[elasticsearchmachine]

Hi @nielsbauman, I've created a changelog YAML for you.

[gmarouli]

I really like the idea of reducing the number of templates we are going through to speed up things. However, I am afraid this optimisation is not correct, I created a test that demonstrates the bug:

 { // Test when a data stream does not use the policy anymore because of a higher template Metadata.Builder mBuilder = Metadata.builder() .put(IndexMetadata.builder("myindex").settings(indexSettings(IndexVersion.current(), 1, 0).put(LifecycleSettings.LIFECYCLE_NAME, "mypolicy"))) .putCustom( IndexLifecycleMetadata.TYPE, new IndexLifecycleMetadata( Map.of("mypolicy", LifecyclePolicyMetadataTests.createRandomPolicyMetadata("mypolicy")), OperationMode.RUNNING ) ) .putCustom( ComposableIndexTemplateMetadata.TYPE, new ComposableIndexTemplateMetadata( Map.of( "mytemplate", ComposableIndexTemplate.builder() .indexPatterns(Collections.singletonList("myds*")) .template( new Template(Settings.builder().put(LifecycleSettings.LIFECYCLE_NAME, "mypolicy").build(), null, null) ) .dataStreamTemplate(new ComposableIndexTemplate.DataStreamTemplate(false, false)) .build(), "myhighertemplate", ComposableIndexTemplate.builder() .indexPatterns(Collections.singletonList("myds")) .dataStreamTemplate(new ComposableIndexTemplate.DataStreamTemplate(false, false)) .priority(1_000L) .build() ) ) ); mBuilder.put(DataStreamTestHelper.newInstance("myds", Collections.singletonList(mBuilder.get("myindex").getIndex()))); // Test where policy exists and is used by an index, datastream, and template ClusterState state = ClusterState.builder(new ClusterName("mycluster")).metadata(mBuilder.build()).build(); assertThat( LifecyclePolicyUtils.calculateUsage(iner, state, "mypolicy"), equalTo( new ItemUsage(List.of("myindex"),List.of(), List.of("mytemplate")) ) ); } } 

In this test we see a data stream whose winner template does not have a policy but there is another candidate template with lower priority that has our policy. The optimisation wrongly reports that this policy is used by the data stream.

We can explore more ideas like this, for example:

• What if we filter and sort only data stream templates? In the worst case scenario we make something slow even slower, but it might speed up some things in many other cases.

[nielsbauman]

The templateToPolicy map is basically only there to save some MetadataIndexTemplateService.resolveSettings calls (as we have to loop over all templates in calculateUsage anyway), but resolving the settings seemed to be relatively expensive, so templateToPolicy basically serves as a cache for that.

[nielsbauman]

I could pre-compute a map of policy to indices as well, but I'm not sure the memory vs. speed trade-off is worth it there.

[nielsbauman]

On second thought, since we have to build this list for every policy anyway, pre-computing these lists and putting them in a map shouldn't be too much additional memory overhead. I'll wait till someone has done a first review before making more changes (in case my whole approach is off).

[nielsbauman]

And the same goes for the composable templates of course.

[gmarouli]

Let's think about this. You only need to keep a cache of what is necessary. Right?

For example:
Composable templates:
Cache the resolved templates that match our policies and their index patterns: template -> policy, probably you can also store policy -> templates.

Data streams
Go over the data streams like you already do, find the template for each data stream and check the cache template -> policy. You do not need to keep track of nulls anymore, since you have collected the relevant templates, if the template is not in your cache then the data stream shouldn't be either. So you create the policy -> data stream.

Indices
I do not see an issue with going over the indices during initialisation because from what I saw the information is pre-calculated and then serialised. So, here you create policy -> indices.

Then retrieving the data is just picking them up from the cache. Thoughts?

[gmarouli]

So this confused me a little bit, both the name and the comment. It wasn't clear to me what is filtered and when we use the word templates, which templates do we mean. So, what if we rename:

• templateNames to requestedTemplateNames, signalling that these was provided by the caller.
• templates to restTemplateCandidates or otherTemplateCandidates, signalling that these are the rest, so not the ones provided by the user and candidates because isGlobalAndHasIndexHiddenSetting is filtering out the non candidates.

Names are up for debate but I hope I made clear the direction I am considering.

[nielsbauman]

Good point, I've renamed the templates variable and updated the comments. Let me know if this is more clear.

[gmarouli]

Hm..... I am not going to lie, seeing this does not give me a good feeling. Is this really the best alternative to structure the code?

[nielsbauman]

I think I've found a much better alternative, see 293ff9d. Let me know whether you agree that this approach is better.

[gmarouli]

Apologies for the late review! Really happy to see this work @nielsbauman, this is awesome! Let me know what you think on the comments.

I like the direction that you have taken :)

[gmarouli]

Let's think about this. You only need to keep a cache of what is necessary. Right?

For example:
Composable templates:
Cache the resolved templates that match our policies and their index patterns: template -> policy, probably you can also store policy -> templates.

Data streams
Go over the data streams like you already do, find the template for each data stream and check the cache template -> policy. You do not need to keep track of nulls anymore, since you have collected the relevant templates, if the template is not in your cache then the data stream shouldn't be either. So you create the policy -> data stream.

Indices
I do not see an issue with going over the indices during initialisation because from what I saw the information is pre-calculated and then serialised. So, here you create policy -> indices.

Then retrieving the data is just picking them up from the cache. Thoughts?

[nielsbauman]

@gmarouli thanks for your feedback! I've adjusted the code a bit more to just cache everything (as you proposed in one of the discussions above). I processed your other feedback as well and made some small adjustments. Let me know what you think!

[nielsbauman]

I still want to run some benchmarks again, to validate that my changes are still as effective as I think they are. That'll take some time because I have to configure the cluster in the right way again, so I'll get to that later.

[nielsbauman]

I'm worried whether this expected size is too large. We're looping over all templates in the cluster, but I don't think we'll often actually put all templates in that map -- especially when there is only one requested policy. At the same time, I don't really have any suggestions for a better expected size, so I'd be either this or just an empty map. What do you think?

[gmarouli]

Again I would vote for simplicity. So this has an extra statement and a comment explaining that statement, it's not super difficult but it is an extra thing to process. So if the benefit is not significant, then I would prefer to have just a map with the default size that reads effortlessly.

What do you think?

[nielsbauman]

I changed this line to just use an empty map. I'm not sure what "statement" and "comment" you're referring to exactly. The comment in this diff justifies the map itself, which we need anyway.

[nielsbauman]

I just ran another benchmark and with the setup I created, a call to GET /_ilm/policy on main takes roughly 2 minutes, whereas on my branch it takes less than 0.5 seconds. I'm good to go on this performance-wise. What do you think?

[gmarouli]

LGTM, great improvement @nielsbauman . I added some minor comments but they are not blocking.

[gmarouli]

I am thinking out loud, shouldn't we pass here the ds.isHidden() flag instead of just setting it to false? I understand that for now this doesn't make any difference, but it does seem misleading. What do you think?

Disclaimer, this is not blocking considering it was already working like that.

[nielsbauman]

Hm, that's a good point. I think passing ds.isHidden() is not the right way to go, as it indeed wouldn't make a difference (because we include data stream indices anyway, regardless of whether the data stream is hidden or not), so I'd argue that passing that is confusing as well. Instead, I'd probably be more inclined to extract/separate some methods (in MetadataIndexTemplateService). We could either 1. overload the existing findV2Template method(s) to not require the isHidden parameter and always pass false - with a comment explaining why we do that, or 2. we extract/refactor te findV2CandidateTemplates method to have a version that truly doesn't use the isHidden parameter - although that might get a bit ugly. What do you think?

[gmarouli]

Good point. I do not think we need to change the code. Maybe a comment here and a mention in the javadoc of the method could be enough. What do you think?

[nielsbauman]

I added a comment to findV2CandidateTemplates. There are several usages of that method that pass false instead of ds.isHidden(), so adding comment on the caller side didn't make much sense IMO. I think the comment on findV2CandidateTemplates is a good addition, thanks for the suggestion.