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

Add support for ES index aliases / rollover to the dependency store (Resolves #2143) #2144

Merged
merged 3 commits into from
Feb 3, 2022

Conversation

frittentheke
Copy link
Contributor

@frittentheke frittentheke commented Mar 30, 2020

Which problem is this PR solving?

Currently the dependency store has no support for index aliases / rollover indices.
Resolves #2143

Short description of the changes

Use the already existing and used config variable UseReadWriteAliases to switch between the current behavior and using non-dated "-read" and "-write" index names. This aligns with the behavior that is already in place for spans and services.

@pavolloffay
Copy link
Member

pavolloffay commented Mar 30, 2020

You have cracked an issue that will require some more work :)

  • Remove dependency mapping from go code and move it to JSON in mappings directory. Then install index template at Jaeger startup like we do for main indices.
  • Change rollover script to work with dependency index - we probably want to handle dependency indices only when users want it - so we should introduce enable flag for it in the script.
  • Change index cleaner to remove dependency index when rollover is enabled. There are a couple of things to look for e.g. do not remove active writer index
    def filter_main_indices_rollover(ilo, prefix):
  • Add test for index cleaner.

I would recommend submitting a separate PR for the first step initially.

@frittentheke
Copy link
Contributor Author

You have cracked an issue that will require some more work :)

* [ ]  Change rollover script to work with dependency index - we probably want to handle dependency indices only when users want it - so we should introduce enable flag for it in the script.
* [ ]  Change index cleaner to remove dependency index when rollover is enabled. There are a couple of things to look for e.g. do not remove active writer index https://github.com/jaegertracing/jaeger/blob/47d20296982d2e01432be46daef1dfc2c365872b/plugin/storage/es/esCleaner.py#L62
* [ ]  Add test for index cleaner.  https://github.com/jaegertracing/jaeger/blob/4bf5e9b440b0d95025248182a4b2caff8b0036fc/plugin/storage/integration/es_index_cleaner_test.go#L17

If I already cracked it and you thankfully already looked a little deeper into the other dependencies / breaking changes I take the liberty to go even another step further:

As I suggested in #2143 (comment) how about simply switching to using / providing a CronJob using the ElasticSearch curator (including some basic config for it). It's also a Python based tool, but very flexible and advertised by Elastic themselves to actually be the one stop housekeeper - It can do rules with time and name filters and then apply all sorts of actions, including rollover and alias. It also has a dry-run mode for people to first check what will be done (https://github.com/elastic/curator/blob/master/docs/asciidoc/examples.asciidoc).

I mean this is Jaeger tracing and likely you do not want to maintain and provide housekeeping tooling for all sorts of storages.

I would recommend submitting a separate PR for the first step initially.

I shall get started on that shortly.

@pavolloffay
Copy link
Member

As I suggested in #2143 (comment) how about simply switching to using / providing a CronJob using the ElasticSearch curator (including some basic config for it). It's also a Python based tool, but very flexible and advertised by Elastic themselves to actually be the one stop housekeeper

See the response in #2143 (comment). If you manage to simplify those scripts we will be happy to accept the patch.

@frittentheke
Copy link
Contributor Author

@pavolloffay I pushed some more commits - thanks for your patience.
Please kindly TAL.

@frittentheke
Copy link
Contributor Author

@pavolloffay

As for the index templates - I just created another PR: #2149
If you feel this should just be one PR "adding ES rollover" to dependencies please let me know, I shall then add these commits this PR.

@pavolloffay
Copy link
Member

#2149 looks good just remove there any other changes that are not related to creating index templates.

@frittentheke
Copy link
Contributor Author

frittentheke commented Apr 3, 2020

#2149 looks good just remove there any other changes that are not related to creating index templates.

Could you elaborate a bit please @pavolloffay . The PR #2149 is based on this one that is why those commits are in there as well. Should I reorder the changes of the two PRs then? Otherwise after merging this one, the other one should be rebased and then only contain the index template bits.

@pavolloffay
Copy link
Member

#2149 should contain only refactoring to move the index mappings to json file and submitting those mappings as index templates at Jaeger startup.

@frittentheke
Copy link
Contributor Author

#2149 should contain only refactoring to move the index mappings to json file and submitting those mappings as index templates at Jaeger startup.

@pavolloffay I did rearrange the two PRs now. So first step is the removal of the mappings in #2149 and this PR here then follows with adding the support for index aliases and the Params struct to configure things.

I noticed that there is not writer for dependencies initialized, that is why I kept the creation of templates with the reader.

@frittentheke
Copy link
Contributor Author

@pavolloffay with the merge of #2285 I now did a rebase of the PR here to allow using an rollover alias for the dependency store.

FTAL

@frittentheke frittentheke force-pushed the depAliases branch 2 times, most recently from 1f314b1 to cc2a52d Compare June 11, 2020 20:11
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from this PR we will need changes to rollover and clean indices scripts.

plugin/storage/es/dependencystore/storage.go Outdated Show resolved Hide resolved
plugin/storage/es/dependencystore/storage.go Show resolved Hide resolved
plugin/storage/es/factory.go Outdated Show resolved Hide resolved
@Leinad0033
Copy link

Any chance of this getting updated to a state where is can be merged into master?

@frittentheke
Copy link
Contributor Author

Any chance of this getting updated to a state where is can be merged into master?

@pavolloffay does it make sense to pick up on this then?
Should I rebase the PR and work on your remarks?

@jpkrohling
Copy link
Contributor

Closing due to inactivity.

@jpkrohling jpkrohling closed this Aug 23, 2021
@frittentheke
Copy link
Contributor Author

Closing due to inactivity.

@jpkrohling it's unfortunate that this, being such a rather small change, was not pursued.
Was there anything I could have done different?

@jpkrohling
Copy link
Contributor

Thank you for asking! Sometimes, things fall off the cracks and this might have been one of them. Perhaps it would have helped to ping @pavolloffay more frequently (or even other maintainers).

If you are still interested in contributing this feature, let me know and I'll reopen this one!

@frittentheke
Copy link
Contributor Author

frittentheke commented Aug 23, 2021

Thank you for asking! Sometimes, things fall off the cracks and this might have been one of them. Perhaps it would have helped to ping @pavolloffay more frequently (or even other maintainers).

If you are still interested in contributing this feature, let me know and I'll reopen this one!

I certainly would still like to finish this PR.
Another reminder might have helped, but that honestly is a fine line. I neither want to get on @pavolloffay's nerves nor force something onto a project that does not want a certain contribution.

Maybe going through open PRs from time to time so see what is keeping them from moving would be sensible and a good sign to contributors. Kinda like what you did, just before hitting the close button :-)

As said, I gladly rebase the PR and would love to hear your feedback on what needs to be adjusted.

@jpkrohling
Copy link
Contributor

I neither want to get on @pavolloffay's nerves nor force something onto a project that does not want a certain contribution.

I think it's fine to ping people from time to time. If it's a feature we don't want for some reason, we'll say so :-)

Would you prefer to open a new PR, or should I reopen this one?

@pavolloffay, would you be able to take another look at this PR and share what you think it's missing?

@frittentheke
Copy link
Contributor Author

Would you prefer to open a new PR, or should I reopen this one?

if you don't mind please reopen this one.

@frittentheke
Copy link
Contributor Author

@pavolloffay @jpkrohling could you kindly take a look at this?

@wiardvanrij
Copy link

+1 let's move this forward please. :) @frittentheke thanks for your work!

@pavolloffay
Copy link
Member

@frittentheke / @wiardvanrij please add support for dependency index to rollover and index cleaner components. I see some changes in rollover but the init command does not create any dependency indices/aliases.

yellow open jaeger-span-000001            Cz0EQoQ1R4SXYY-Yv1fqZQ 5 1   0   0    810b    810b
yellow open jaeger-service-000001         S5vE45HfRkq_zXuELooc_g 5 1   0   0    810b    810b
yellow open .triggered_watches            EV2sv3m7SkegBcCXdvkJuQ 1 1   0   0    162b    162b
yellow open .monitoring-alerts-6          qOhRKYxbRe6KY4JumeoERw 1 1   1   0   6.2kb   6.2kb
yellow open .watches                      DW-akf7oQcKORu6TLJTztQ 1 1   4   0  19.7kb  19.7kb
yellow open .watcher-history-6-2021.12.13 -xI_8_EWTkaOf4Krk24G_g 1 1  25   0  67.2kb  67.2kb
yellow open .monitoring-es-6-2021.12.13   GyB2V6haRSCK1d0LstBTyg 1 1 356 170 614.2kb 614.2kb

@vvdaal
Copy link

vvdaal commented Jan 12, 2022

@frittentheke possible any update on this or require help?

@frittentheke
Copy link
Contributor Author

@frittentheke possible any update on this or require help?

Sorry for the delay. I shall look into this soon and push an update.

@vvdaal
Copy link

vvdaal commented Jan 14, 2022

Sorry for the delay. I shall look into this soon and push an update.

No problem at all! It's all on a volunteer after all :) Really appreciate your contributions as this feature will help us in a use case we have!

@frittentheke
Copy link
Contributor Author

@vvdaal @wiardvanrij sorry about the huge delay ⌛

@frittentheke / @wiardvanrij please add support for dependency index to rollover and index cleaner components. I see some changes in rollover but the init command does not create any dependency indices/aliases.

yellow open jaeger-span-000001            Cz0EQoQ1R4SXYY-Yv1fqZQ 5 1   0   0    810b    810b
yellow open jaeger-service-000001         S5vE45HfRkq_zXuELooc_g 5 1   0   0    810b    810b
yellow open .triggered_watches            EV2sv3m7SkegBcCXdvkJuQ 1 1   0   0    162b    162b
yellow open .monitoring-alerts-6          qOhRKYxbRe6KY4JumeoERw 1 1   1   0   6.2kb   6.2kb
yellow open .watches                      DW-akf7oQcKORu6TLJTztQ 1 1   4   0  19.7kb  19.7kb
yellow open .watcher-history-6-2021.12.13 -xI_8_EWTkaOf4Krk24G_g 1 1  25   0  67.2kb  67.2kb
yellow open .monitoring-es-6-2021.12.13   GyB2V6haRSCK1d0LstBTyg 1 1 356 170 614.2kb 614.2kb

@pavolloffay @jpkrohling Could you kindly check out my PR again, as I don't actually see what is not working correctly.
To test I just started a single node ES (via Docker/podman) and ran the following commands:

  1. cmd/es-rollover/es-rollover-linux-amd64 init http://127.0.0.1:9200 which resulted in:
$ curl localhost:9200/_cat/indices
green  open .geoip_databases           vhPaxwXNTaiiuDCvAwtfbQ 1 0 42 0 40.4mb 40.4mb
yellow open jaeger-dependencies-000001 BxPTInxIQEe7dpyld_x0DA 5 1  0 0  1.1kb  1.1kb
yellow open jaeger-span-000001         684B2KtOQwSMxEgHzcF6UQ 5 1  0 0  1.1kb  1.1kb
yellow open jaeger-service-000001      rh0Ut4T6Tb-_2r3FqJQUjA 5 1  0 0  1.1kb  1.1kb

$ curl localhost:9200/_cat/aliases
jaeger-span-read          jaeger-span-000001         - - - -
jaeger-span-write         jaeger-span-000001         - - - -
jaeger-dependencies-read  jaeger-dependencies-000001 - - - -
jaeger-dependencies-write jaeger-dependencies-000001 - - - -
jaeger-service-read       jaeger-service-000001      - - - -
jaeger-service-write      jaeger-service-000001      - - - -
  1. I then tried the rollover via cmd/es-rollover/es-rollover-linux-amd64 rollover http://127.0.0.1:9200 --conditions "{\"max_age\": \"1m\"}" (the condition is just to force a rollover, did not see any other option to achieve this):
$ curl localhost:9200/_cat/indices                                                                           
green  open .geoip_databases           vhPaxwXNTaiiuDCvAwtfbQ 1 0 42 0 40.4mb 40.4mb
yellow open jaeger-dependencies-000001 BxPTInxIQEe7dpyld_x0DA 5 1  0 0  1.1kb  1.1kb
yellow open jaeger-dependencies-000002 VwCy5gf4QvSxfHK6XHmcFQ 5 1  0 0   641b   641b
yellow open jaeger-span-000001         684B2KtOQwSMxEgHzcF6UQ 5 1  0 0  1.1kb  1.1kb
yellow open jaeger-span-000002         hohYZJ4aQiCbhXwjzhSAbA 5 1  0 0   641b   641b
yellow open jaeger-service-000002      SFWvaMULSkyHeeoMoISZog 5 1  0 0   641b   641b
yellow open jaeger-service-000001      rh0Ut4T6Tb-_2r3FqJQUjA 5 1  0 0  1.1kb  1.1kb

$ curl localhost:9200/_cat/aliases                                                                           
jaeger-span-read          jaeger-span-000001         - - - -
jaeger-dependencies-read  jaeger-dependencies-000001 - - - -
jaeger-service-read       jaeger-service-000002      - - - -
jaeger-service-write      jaeger-service-000002      - - - -
jaeger-dependencies-read  jaeger-dependencies-000002 - - - -
jaeger-dependencies-write jaeger-dependencies-000002 - - - -
jaeger-span-read          jaeger-span-000002         - - - -
jaeger-span-write         jaeger-span-000002         - - - -
jaeger-service-read       jaeger-service-000001      - - - -

is that not exactly what should happen?

@albertteoh
Copy link
Contributor

@frittentheke, the integration tests will need updating to include the newly added jaeger-dependencies indices. For example: https://github.com/jaegertracing/jaeger/runs/4935280899?check_suite_focus=true#step:7:633

@albertteoh
Copy link
Contributor

is that not exactly what should happen?

@frittentheke Thanks for the test details, the rollover results look good to me.

I think we might need to support dependencies for the index cleaner as well.

I've basically continued on from your test above. After executing the index cleaner, I expect all indices (span, service and dependencies) except the latest to be deleted. I can see span and service indices being deleted but not dependencies.

$ ./cmd/es-index-cleaner/es-index-cleaner-linux-amd64 0 http://127.0.0.1:9200  --rollover
{"level":"info","ts":1643109670.3972557,"caller":"./main.go:86","msg":"Indices before this date will be deleted","date":"2022-01-26T00:00:00Z"}
{"level":"info","ts":1643109670.397428,"caller":"./main.go:95","msg":"Queried indices","indices":[{"Index":"jaeger-dependencies-000001","CreationTime":"2022-01-25T11:18:28.229Z","Aliases":{"jaeger-dependencies-read":true}},{"Index":"jaeger-dependencies-000002","CreationTime":"2022-01-25T11:20:51.639Z","Aliases":{"jaeger-dependencies-read":true,"jaeger-dependencies-write":true}},{"Index":"jaeger-service-000001","CreationTime":"2022-01-25T11:18:28.039Z","Aliases":{"jaeger-service-read":true}},{"Index":"jaeger-service-000002","CreationTime":"2022-01-25T11:20:51.477Z","Aliases":{"jaeger-service-read":true,"jaeger-service-write":true}},{"Index":"jaeger-span-000001","CreationTime":"2022-01-25T11:18:27.834Z","Aliases":{"jaeger-span-read":true}},{"Index":"jaeger-span-000002","CreationTime":"2022-01-25T11:20:51.281Z","Aliases":{"jaeger-span-read":true,"jaeger-span-write":true}}]}
{"level":"info","ts":1643109670.3976908,"caller":"./main.go:102","msg":"Deleting indices","indices":[{"Index":"jaeger-service-000001","CreationTime":"2022-01-25T11:18:28.039Z","Aliases":{"jaeger-service-read":true}},{"Index":"jaeger-span-000001","CreationTime":"2022-01-25T11:18:27.834Z","Aliases":{"jaeger-span-read":true}}]}

... and this is evident with:

$ curl localhost:9200/_cat/aliases
jaeger-span-read          jaeger-span-000002         - - - -
jaeger-span-write         jaeger-span-000002         - - - -
jaeger-dependencies-read  jaeger-dependencies-000001 - - - -
jaeger-dependencies-read  jaeger-dependencies-000002 - - - -
jaeger-dependencies-write jaeger-dependencies-000002 - - - -
jaeger-service-read       jaeger-service-000002      - - - -
jaeger-service-write      jaeger-service-000002      - - - -

@frittentheke frittentheke force-pushed the depAliases branch 2 times, most recently from 4a51446 to d11cddd Compare January 26, 2022 19:41
@frittentheke
Copy link
Contributor Author

@albertteoh @pavolloffay @jpkrohling I pushed a new revision adding rollover to the es-index-cleaner and the missing tests.

PTAL

@albertteoh
Copy link
Contributor

Thanks @frittentheke. It looks like some ES integration tests are failing.

You can run these tests locally:

  • make index-rollover-integration-test
  • make index-cleaner-integration-test

@frittentheke
Copy link
Contributor Author

Thanks @frittentheke. It looks like some ES integration tests are failing.

You can run these tests locally:

* `make index-rollover-integration-test`

* `make index-cleaner-integration-test`

@albertteoh sorry for the delay. Tool be a minute more to figure that the optional ilm parts where missing in plugin/storage/es/mappings/jaeger-dependencies-7.json.

It's all green on my machine now - let's see what CI says then.

@albertteoh
Copy link
Contributor

albertteoh commented Jan 29, 2022

The TestMappingBuilder_GetMapping/jaeger-dependencies unit test is failing.

I think it may be related to the optional ILM part in jaeger-dependencies-7.json you discovered earlier.

By the way, the tests can be executed locally as well with: make test.

@frittentheke
Copy link
Contributor Author

The TestMappingBuilder_GetMapping/jaeger-dependencies unit test is failing.
I think it may be related to the optional ILM part in jaeger-dependencies-7.json you discovered earlier.

Yep, the fixtures needed fixing (additions). make test shows a PASS now.
@albertteoh PTAL

albertteoh
albertteoh previously approved these changes Feb 1, 2022
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a couple of nits.

cmd/es-index-cleaner/app/index_filter.go Outdated Show resolved Hide resolved
reader := esDepStore.NewDependencyStore(f.primaryClient, f.logger, f.primaryConfig.GetIndexPrefix(),
f.primaryConfig.GetIndexDateLayoutDependencies(), f.primaryConfig.GetMaxDocCount())
return reader, nil
return createDependencyReader(f.logger, f.primaryClient, f.primaryConfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we can just return esDepStore.NewDependencyStore(...).

I can see the intention here is to maintain consistency, though I think CreateSpanReader/Writer extract the function for reuse with its Archive equivalent. The DependencyStore doesn't support archiving AFAIK, so there doesn't seem to be much benefit of extracting the function for the cost of the additional indirection, unless you had other reasons?

What do you think?

Copy link
Contributor Author

@frittentheke frittentheke Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand your point and while I could argue there might arise the need to add archiving, I actually just would favor the consistency. As for performance, the compiler will optimize the additional call away and it's not like this is a hot path by any means.

In short, if you insist I will certainly change this, but I believe the code is just more approachable if similar things, as in creating readers for three types of stores, are not handled any differently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave this as is, I'm not strongly attached to my suggestion. 😄

 * Give DependencyStore a params struct like the SpanStore to carry its configuration parameters
 * Adapt and extend the tests accordingly

Signed-off-by: Christian Rohmann <[email protected]>
reader := esDepStore.NewDependencyStore(f.primaryClient, f.logger, f.primaryConfig.GetIndexPrefix(),
f.primaryConfig.GetIndexDateLayoutDependencies(), f.primaryConfig.GetMaxDocCount())
return reader, nil
return createDependencyReader(f.logger, f.primaryClient, f.primaryConfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave this as is, I'm not strongly attached to my suggestion. 😄

@albertteoh albertteoh enabled auto-merge (squash) February 2, 2022 10:21
@albertteoh albertteoh merged commit 18e9d5b into jaegertracing:main Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use single ElasticSearch index to store dependencies
7 participants