Add support for Elasticsearch ILM Polices

[bhiravabhatla]

Signed-off-by: santosh bsantosh@thoughtworks.com

Which problem is this PR solving?

• Resolves Support for Elasticsearch ILM policies #2048

Short description of the changes

• Adds support for ILM policies by creating overriding index templates - which assign the ILM policy and rollover alias and read-alias to the index upon creation. Change is made esRollover script to enable ILM using an environment variable USE_ILM to be backwards compatible.

[bhiravabhatla]

My bad, dint run integration tests locally. Looks like tests are failing because of ES version. is_write_index is supported in ES 6.4+. Would need some help in fixing tests, as I am new to golang

[pavolloffay]

I didn't want to add a new set of index templates.

Could we instead add the lifecycle configuration to our existing index templates?

[bhiravabhatla]

@pavolloffay - I tried that initially, When jaeger comes up, it is also trying to create the index templates ( even with --es.use-alias enabled) and if the template name is same - it is overriding it. Hence had to use new set of templates with higher priority (order = 1).

Ideally, if --es-use-alias is enabled - I believe template creation should also be left to external component (esRollover) along with alias creation. There might be a valid reason why it is not - I am not sure.

Right now, both jaeger and esRollover try to create the templates. As Jaeger starts after esRollover is run, its overriding the templates created by esRollover.

[pavolloffay]

Even if the templates are overridden it should not be an issue since the definition does not change it just references the name of ILM.

--es.create-index-templates can be used to disable template creation

[bhiravabhatla]

lifecycle or ILM is inside settings of index template -

"settings": {
    "index.number_of_shards": ${__NUMBER_OF_SHARDS__},
    "index.number_of_replicas": ${__NUMBER_OF_REPLICAS__},
    "index.mapping.nested_fields.limit": 50,
    "index.requests.cache.enable": true,
    "lifecycle": {
      "name": "jaeger",
      "rollover_alias": "${__INDEX_WRITE_ALIAS__}"
    }

Default template has settings without lifecycle. Hence when we start jaeger - it overrides the settings of the template ( unless --es.create-index-templates is used to disable it). That means we are saying user has to use --es.use-alias with --es.create-index-templates always - if ILM is needed.

[pavolloffay]

I get it now, I want to avoid having multiple variations of the index templates.

Can be the lifecycle be configured but a PUT (change) request?

[bhiravabhatla]

I get it now, I want to avoid having multiple variations of the index templates.

Yes - completely agree. Ideal thing would be to disable creation of templates from jaeger end whenever we have --es.use-aliases.

Can be the lifecycle be configured but a PUT (change) request?

Unfortunately we have to run init before starting jaeger. And index-templates must be created before 1st index is created. So, right now, I see no way but to have another set of templates for ILM with different name and higher order - so that when index templates are applied - ILM template takes precedence.

Either this or we somehow mandate to always use --es.use-alias with --es.create-index-templates set to false.

[bhiravabhatla]

Ideal thing would be to disable creation of templates from jaeger end whenever we have --es.use-aliases

@pavolloffay, I think I know why this was not done. As of now, If we enable use-aliases and start jaeger with out running esRollover init first --> Jaeger is creating the default index templates and two initial indices with names jaeger-span-write & jaeger-span-read, so that it would continue to work.

[bhiravabhatla]

@pavolloffay - As suggested - changes made to code to avoid using multiple variations of index templates. Please go through the latest commit

[bhiravabhatla]

@pavolloffay Any feedback on this?

[bhiravabhatla]

@pavolloffay / @jaegertracing/elasticsearch - Let me know, if you have any thoughts on the approach

[prestonvanloon]

Any update on this?

[bhiravabhatla]

Yes. I updated the code to use single variation of index template. I am waiting for feedback on the same.

[prestonvanloon]

Any update? Can this be merged? Does jaeger-operator support ILM?

[yurishkuro]

@albertteoh I think you folks run Jaeger with ES, would you like to take a look?

[albertteoh]

Sure thing @yurishkuro, I can take a look at it tomorrow.

[albertteoh]

@bhiravabhatla, sorry I took some time to get back to you; all of this is quite new to me, so I had to invest some time learning and trying stuff out.

Thanks for providing your repo, it helped me understand what you're trying to achieve and allowed me to see it working locally; and it works well by the way. 👍

I also tested the original non-ILM behaviour with rollover and that seems to be working as expected too.

One thing I noticed though is that, before this PR, there would be a single write alias per index alias like:

jaeger-service-read  jaeger-service-000001 - - - -
jaeger-service-read  jaeger-service-000002 - - - -
jaeger-service-write jaeger-service-000002 - - - -
jaeger-span-read     jaeger-span-000002    - - - -
jaeger-span-write    jaeger-span-000002    - - - -
jaeger-span-read     jaeger-span-000001    - - - -

When I query on jaeger-span-write, it only returns data from jaeger-span-000002.

With this change, I noticed every index has a write alias, but only one has the is_write_index set to true, which makes sense:

jaeger-span-write   jaeger-span-000002    - - - false
jaeger-span-write   jaeger-span-000001    - - - false
jaeger-span-write   jaeger-span-000004    - - - true
jaeger-span-write   jaeger-span-000003    - - - false

When I query on jaeger-span-write, it returns data from all indices, which also makes sense; however, this means behaviour has changed because previously, it would only return data from a single index, the one which is being written to. Is this expected behaviour when using the is_write_index setting? I don't expect users to be querying from the write index anyway, as they should be using the read index, but was just curious.

Either this or we somehow mandate to always use --es.use-alias with --es.create-index-templates set to false.

As you noted, without --es.create-index-templates, the collector will create an additional template:

jaeger-span-with-ilm    [jaeger-span-*]     1  
jaeger-service-with-ilm [jaeger-service-*]  1  
jaeger-span             [*jaeger-span-*]    0  
jaeger-service          [*jaeger-service-*] 0  

However, those with higher order (larger number) will be merged and overwrites overlapping attributes from lower order templates.
Given that, why is there a need to mandate the use of --es.create-index-templates along with --es.use-alias?

[bhiravabhatla]

@albertteoh Thanks you for taking time out to go through the PR. Let me try and answer your questions

However, those with higher order (larger number) will be merged and overwrites overlapping attributes from lower order templates.
Given that, why is there a need to mandate the use of --es.create-index-templates along with --es.use-alias?

Yes you are correct, it not mandatory. As I mentioned in one of the comments above - #2454 (comment) , I made changes to create templates with different name to not clash with default templates created by default by jaeger on startup.

But to be clean, I would suggest to add it in documentation to preferably disable creating templates when using aliases.

Is this expected behaviour when using the is_write_index setting?

As we use ILM rollover, ILM assigns this alias(rollover alias) to subsequent indexes - so all the indexes would end up having this alias. I dont foresee any issue with this from Jaeger point of view, as at a single point of time, we would have only one index having "is_write_index" set to true.

[codecov]

Codecov Report

Merging #2454 (a3b465b) into master (2ff0a3d) will decrease coverage by 0.02%.
The diff coverage is 96.03%.

@@            Coverage Diff             @@
##           master    #2454      +/-   ##
==========================================
- Coverage   95.83%   95.81%   -0.03%     
==========================================
  Files         217      220       +3     
  Lines        9656     9744      +88     
==========================================
+ Hits         9254     9336      +82     
- Misses        331      334       +3     
- Partials       71       74       +3     

Impacted Files	Coverage Δ
cmd/templatizer/app/renderer/render.go	87.50% <87.50%> (ø)
plugin/storage/es/factory.go	98.16% <94.44%> (-1.84%)	⬇️
cmd/templatizer/app/flags.go	100.00% <100.00%> (ø)
pkg/es/textTemplate.go	100.00% <100.00%> (ø)
plugin/storage/es/options.go	100.00% <100.00%> (ø)
plugin/storage/es/spanstore/writer.go	100.00% <100.00%> (ø)
cmd/query/app/server.go	95.68% <0.00%> (-1.44%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ff0a3d...a3b465b. Read the comment docs.

[albertteoh]

I think we'll need to add a .use-ilm config in plugin/storage/es/options.go and pass it into "UseILM" instead of the hard-coded false.

Please also add ILM-enabled usage instructions to https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/es/README.md.

[bhiravabhatla]

@albertteoh - I did add the flag (useILM) to collector in initial commits and then reverted it. My thought process behind it below:

• If we do have a flag to enable UseILM in jaeger collector - we have to add a check if its being used along with --use-aliases or not. Because without aliases - there is no use of enabling ILM and it might need lead to confusion.
• If you think about it, the actual support for ILM (creation of initial indices, aliases ) is done by rollover.py. It makes no difference if have the flag at collector or not.

[albertteoh]

we have to add a check if its being used along with --use-aliases or not

I thought this should be a fairly easy and cheap check, though maybe I'm wrong?

If you think about it, the actual support for ILM (creation of initial indices, aliases ) is done by rollover.py. It makes no difference if have the flag at collector or not.

Consider these two scenarios where I think supporting use-ilm in jaeger-collector would be useful:

• On-boarding to ILM from manual rollovers (init with ILM disabled, but start jaeger-collector with ILM enabled), which simply involves a single manual rollover to pickup the ILM-enabled template.
• Off-boarding from ILM to manual rollovers (see Add support for Elasticsearch ILM Polices #2454 (comment)).

Further, I think it would reduce confusion for developers, who see that UseILM = false in jaeger-collector, yet wonder why ILM is working; as what happened to me when reviewing this PR :).

[bhiravabhatla]

Yep - I see your point 👍 . Let me revert my commits which actually added support for this flag along with tests and push. Would work on this during weekend.

[bhiravabhatla]

Added UseILM flag to es storage config and updated corresponding tests. Kindly check @albertteoh

[albertteoh]

Why is the -override-template necessary here? It seems to add another template matching the same index pattern; but the only difference is the order; which seems unnecessary to me.

[bhiravabhatla]

@albertteoh - Kept this because, as of now if some one wants to override replicas/shards - the only way they could do is with esRollover script (passing different Shard and Replica counts). By default, I guess counts are hard coded.

jaeger/plugin/storage/es/factory.go

Line 216 in 0b43839

mapping = strings.Replace(mapping, "${__NUMBER_OF_SHARDS__}", strconv.FormatInt(shards, 10), 1)

jaeger/plugin/storage/es/factory.go

Line 217 in 0b43839

mapping = strings.Replace(mapping, "${__NUMBER_OF_REPLICAS__}", strconv.FormatInt(replicas, 10), 1)

[bhiravabhatla]

If we dont have -override-template suffixed to the index template name. When jaeger comes up, if --es.create-index-templates is not set to false, it would overwrite the template created by rollover script (which is always run before jaeger is brought up) - as the name of index template would be same.

[albertteoh]

@bhiravabhatla

Kept this because, as of now if some one wants to override replicas/shards - the only way they could do is with esRollover script (passing different Shard and Replica counts).

Yup, this is a use case which I hadn't considered. However, if we do want to give users ability to override the shard and replica counts (presumably via the jaeger-collector), shouldn't Order be set to 0? From my own tests, because order for -override-template is 2, my jaeger-collector shard and replica settings (which have order = 1) don't take effect because they are overridden by the esRollover script's -override-template. Recall the templates are merged/applied in the order: 0 -> 1 -> 2 -> .... They are additively applied and the larger order is preferred on conflict/overlapping configuration.

If we dont have -override-template suffixed to the index template name. When jaeger comes up, if --es.create-index-templates is not set to false, it would overwrite the template created by rollover script

Yes, you're right. Is this necessarily a bad thing though? From what I understand, the trade-offs of overwriting the template created by esRollover script are:
Pros:

• Reduces the number of overlapping templates:
  • Simplifies the development & debugging process; there isn't a need to mentally merge two similar templates nor a need to understand how ordering works with ES templates.
  • Avoids potential bugs from overlapping json elements that were not considered.
• Removes the need to manage ordering, which in turn, can avoid potential bugs if users decide to create their own templates matching the same jaeger indices, but with the wrong order. This also simplifies the code, removing the need to manage template ordering.
• Provides the ability to easily "off-board" from ILM, if users wish to customise management of index rollovers because templates are additively applied when overlapping with another template.
  • Consider the scenario where a user creates indices against an ILM-enabled template. They then decide to revert to manual rollovers (perhaps to have finer-grain control on index management). However, they cannot disable ILM, even when setting ES_USE_ILM=false in jaeger-collector, because the base template contains the lifecycle setting and can't be removed through overriding templates.

Cons:

• Overwriting means we will lose history of the original template and when it was replaced.
• The base template may have index configurations that users may want and overwriting will remove these.
  • Addressable by installing the custom template and running jaeger-collector with --es.create-index-templates, as suggested in: Revert "Fix failing kafka integration test" #1077.

To me, the benefits of simplicity, maintainability and the ability to transition from ILM to manual rollovers outweigh the costs, though perhaps my understanding is incorrect. Let me know what you think.

[bhiravabhatla]

@albertteoh

However, if we do want to give users ability to override the shard and replica counts (presumably via the jaeger-collector)

As of now there is no way to override the shard and replica counts from collector. Its always hardcoded to 10. Only way to achieve it is via override template in esRollover. And the ordering works as you explained. So, only way to customize shards/replicas as of now is to give higher order to templates getting created from esRollover or have additional flags in collector to customize the counts. Not sure if I am missing something

[bhiravabhatla]

And I agree with your points on ease of maintenance/on-boarding and off-boarding. Also I see esRollover script is trying to solve multiple problems. If you agree, we can have a separate issue created to add support for customizing replicas and shards from collector flags and then we can jus use esRollover for manual rollover and creation of initial aliases/indices

[albertteoh]

As of now there is no way to override the shard and replica counts from collector.

I can see it's possible and was able to override shard counts in my template via the collector:

$ SPAN_STORAGE_TYPE=elasticsearch ES_NUM_SHARDS=3 go run ./cmd/collector/main.go

Help docs:

$ SPAN_STORAGE_TYPE=elasticsearch go run ./cmd/collector/main.go --help
...
      --es.num-replicas int                                The number of replicas per index in Elasticsearch (default 1)
      --es.num-shards int                                  The number of shards per index in Elasticsearch (default 5)
...

[bhiravabhatla]

Oh, I missed this then. My bad.

I see it now -

spanMapping, serviceMapping := GetSpanServiceMappings(cfg.GetNumShards(), cfg.GetNumReplicas(), client.GetVersion(), cfg.GetIndexPrefix())

[bhiravabhatla]

Will push the changes removing the suffix in template name and order in the template.

[bhiravabhatla]

Implemented above feedback @albertteoh

[albertteoh]

The "elasticsearch 7.x" tests are failing because of this line. "*{{ ESPrefix }}jaeger-service-*" should fix it.

[bhiravabhatla]

Fixed the tests

[albertteoh]

Same as above: "*{{ ESPrefix }}jaeger-span-*" should fix it.

[bhiravabhatla]

@albertteoh Forgot to mention one more usecase -

Two Jaeger Installations ( mostly belonging to two environments - e.g. uat dev ) using same elasticsearch backend.
For rollover/ILM to work correctly would need two different index templates for both of them ( because both should use different index prefix for aliases and indices). In esRollover - I made changes to add index prefix to index template id (name).

create_index_template(fix_mapping(mapping, shards, replicas, prefix, use_ilm),
                              prefix + template_name)

Now that we decided not to have multiple templates and not to use order - we should do the same thing while creating index templates in writer.go -

Here -

jaeger/plugin/storage/es/spanstore/writer.go

Line 92 in 1b20947

_, err := s.client.CreateTemplate("jaeger-span").Body(spanTemplate).Do(context.Background())

and here -

jaeger/plugin/storage/es/spanstore/writer.go

Line 96 in 1b20947

_, err = s.client.CreateTemplate("jaeger-service").Body(serviceTemplate).Do(context.Background())

I propose we change it to -

func (s *SpanWriter) CreateTemplates(spanTemplate, serviceTemplate, indexPrefix string) error {
	if indexPrefix != "" {
		indexPrefix += "-"
	}
	_, err := s.client.CreateTemplate(indexPrefix+"jaeger-span").Body(spanTemplate).Do(context.Background())
	if err != nil {
		return err
	}
	_, err = s.client.CreateTemplate(indexPrefix+"jaeger-service").Body(serviceTemplate).Do(context.Background())
	if err != nil {
		return err
	}
	return nil
}

[bhiravabhatla]

The last force push messed up my git history. Created a new branch and raising another PR. Sorry for the confusion caused. Closing this PR.

New PR link
#2739
@albertteoh @yurishkuro