Skip to content
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

Support for Elasticsearch ILM policies #2048

Closed
markosiira-arm opened this issue Jan 29, 2020 · 17 comments · Fixed by #2796
Closed

Support for Elasticsearch ILM policies #2048

markosiira-arm opened this issue Jan 29, 2020 · 17 comments · Fixed by #2796

Comments

@markosiira-arm
Copy link

Requirement - what kind of business use case are you trying to solve?

I want to define index lifecycle policy to manage jaeger indicies in Elasticsearch. Purpose for me is to set policy that will delete outdated indices. There may be other use-cases as well.

Problem - what in Jaeger blocks you from solving the requirement?

Index templates used by jaeger are not easily modifiable. We would like to define the policy in the index template to work with daily indices.

Proposal - what do you suggest to solve the problem or improve the existing situation?

Allow defining the policy somehow to all indices used by jaeger.

Any open questions to address

@pavolloffay
Copy link
Member

pavolloffay commented Jan 29, 2020

@markosiira-arm could you please point us to ES docs regarding what features you want to use? Is it this one https://www.elastic.co/guide/en/elasticsearch/reference/master/index-lifecycle-management.html? Note that we cannot use x-pack or any other non OSS Elasticsearch extensions.

Index templates used by jaeger are not easily modifiable. We would like to define the policy in the index template to work with daily indices.

Templates are here https://github.com/jaegertracing/jaeger/tree/master/plugin/storage/es/mappings. You can modify them and install before Jaeger is deployed and then disable overriding by --es.create-index-templates

EDIT:

the license is BASIC https://www.elastic.co/subscriptions

@markosiira-arm
Copy link
Author

Thank you for responding quickly to my issue.

ILM docs are at: https://www.elastic.co/guide/en/elasticsearch/reference/current/index-lifecycle-management.html and It seems that feature is marked as x-pack and AFAIK it's available with basic (no-cost) license level.

I have also seen jaeger-dependencies indices, I suppose those are orignated from spark ? That template is not available in the https://github.com/jaegertracing/jaeger/tree/master/plugin/storage/es/mappings . Any suggestion how to manage those indices with policies ?

@frittentheke
Copy link
Contributor

@markosiira-arm good spotting about those dependency index templates being missing.
I'd also rather have a single component "prepare" the storage at once (or just have all template JSONs in a single place for me to apply to my ElasticSearch)

@bhiravabhatla
Copy link
Contributor

bhiravabhatla commented Aug 31, 2020

I tried implementing ILM to manage jaeger indices - my implementation in comment below.

#1242 (comment)

@pavolloffay
Copy link
Member

@bhiravabhatla let's move the discusion about ILM here from the link ^^^.

I got a notification about bhiravabhatla/jaeger-index-rollover-with-ilm@a53c4d7. I glanced on the PR and it seems that there are these changes:

  • define ILM policy name in the templates
  • suppy ILM policy name in the rollover script
  • check if ILM policy exists in the rollover script - not sure why we need it

Is it feasible to hardcode the ILM policy name in the templates? This way users could use the feature with a minimal changes in Jaeger.

@bhiravabhatla
Copy link
Contributor

bhiravabhatla commented Sep 2, 2020

@pavolloffay - Yes we can. Actually it was hardcoded before, I got a feedback on my blog on this, and it made sense to make ILM policy name customizable for now -as user is responsible to create ILM policy manually now.

When we actually build this capability in jaeger - it could be hardcoded like the read and write alias names. Thoughts?

Or Are you suggesting to move the creation on ILM policy to the script? - Which would be tricky as the ILM policy could be different for different users.

If you ask me, I would rather leave ILM creation to the user and have him/her pass the name - that way we can be flexible. We already have a check in init script to see if ILM policy passed is already created or not, so we are covered in case init is run before creating ILM.

@pavolloffay
Copy link
Member

I am suggesting it to hardcode the name. That way we don't have to make changes in rollover scripts and add flag to jaeger.

@bhiravabhatla
Copy link
Contributor

@pavolloffay - Makes sense, Agreed. But I guess we still need to have a check in place if the ILM with that name is already created or not. May be we can have this check in jaeger in future.

One more change I did in the scripts was to add is_write_index true while adding write alias to initial/first index. This is still needed.

@pavolloffay
Copy link
Member

Also can a template reference non existing ILM policy?

But I guess we still need to have a check in place if the ILM with that name is already created or not. May be we can have this check in jaeger in future.

When is the policy created? If it is created once before the first deployment the check might not be needed users would be responsible for creating it.

@bhiravabhatla
Copy link
Contributor

bhiravabhatla commented Sep 2, 2020

Also can a template reference non existing ILM policy?

Yes, it can. But if the first index is created before creating the Policy (i.e. if init is run before Policy exists). Index would end up referencing an non-existent policy. User could go back create the ILM later - until ILM is created - no rollover happens obviously.

When is the policy created? If it is created once before the first deployment the check might not be needed users would be responsible for creating it.

Ideally it should be created before jaeger is brought up. And yes we can document the same, and leave it to the user to make sure the ILM is created.

@pavolloffay Thoughts?

@pavolloffay
Copy link
Member

If the template can reference non-existing ILM policy then we should add it to our templates to simplify the configuration. If my understanding is correct users would just then create the policy before app startup and run the rollover (with the change (is_write_index).

@pavolloffay
Copy link
Member

@bhiravabhatla would you be able to create a PR to add ILM policy name to jaeger templates? We could also set the is_write_index to true. It might work bc jaeger rollover does not use it.

@bhiravabhatla
Copy link
Contributor

bhiravabhatla commented Sep 3, 2020

We could also set the is_write_index to true

This has to be done while we add the first indices span/service (*-000001) to write alias.

https://github.com/bhiravabhatla/jaeger-index-rollover-with-ilm/blob/869c8c257a35d99f18e529c7f9669e7208305d4c/esRollover.py#L125

Dint get your point of adding is_write_index in template @pavolloffay

@bhiravabhatla
Copy link
Contributor

If the template can reference non-existing ILM policy then we should add it to our templates to simplify the configuration. If my understanding is correct users would just then create the policy before app startup and run the rollover (with the change (is_write_index).

Makes sense. We should document the name of the ILM somewhere. And we decided it should be "jaeger"? @pavolloffay

@bhiravabhatla
Copy link
Contributor

@bhiravabhatla would you be able to create a PR to add ILM policy name to jaeger templates?

Sure, would raise, once we conclude on above questions, I guess would need to raise a PR for init script as well where we need to add is-write-index setting.

@pavolloffay
Copy link
Member

jaeger name for the ILM policy sounds fine to me. We can also include the is_write_index option in the script I think that would not cause any issues.

@bhiravabhatla
Copy link
Contributor

@pavolloffay - Let me make those changes and raise a PR.

One more thing is once we have the ILM - dont think we would be needing rollover & cleanup scripts. You want to add a deprecation warning there?

We can also have the initializer as an optional pre-hook in jaeger helm charts - if it isnt already there.

bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Sep 3, 2020
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Sep 3, 2020
…aintain backwards compatibility & to override the templates created by jaeger
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Sep 3, 2020
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Sep 4, 2020
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Sep 26, 2020
…id creating new templates in repo.

Signed-off-by: santosh <[email protected]>
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Sep 26, 2020
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Dec 4, 2020
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Dec 4, 2020
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Dec 5, 2020
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 22, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 22, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 22, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 22, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
…test for es versions less than 7

Signed-off-by: santosh <[email protected]>
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
…emplates with text/template and golang util & Implement feedback on tests

Signed-off-by: santosh <[email protected]>
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 25, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 27, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Jan 29, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Feb 6, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Feb 6, 2021
@yurishkuro yurishkuro mentioned this issue Feb 6, 2021
5 tasks
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Feb 13, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Feb 13, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Feb 13, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Feb 13, 2021
bhiravabhatla added a commit to bhiravabhatla/jaeger that referenced this issue Feb 14, 2021
yurishkuro pushed a commit that referenced this issue Feb 15, 2021
* #2048 - Support Elasticsearch ILM for managing jaeger indices

Signed-off-by: santosh <[email protected]>

* #2048 - Implement @yurishkuro's feedback on PR

Signed-off-by: santosh <[email protected]>

* #2048 - Fix fmt issue

Signed-off-by: santosh <[email protected]>

* #2048 - Fix estemplate package alias

Signed-off-by: santosh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment