Skip to content

useILM will prevent index template creation#3599

Closed
rbizos wants to merge 3 commits intojaegertracing:mainfrom
rbizos:main
Closed

useILM will prevent index template creation#3599
rbizos wants to merge 3 commits intojaegertracing:mainfrom
rbizos:main

Conversation

@rbizos
Copy link
Copy Markdown
Contributor

@rbizos rbizos commented Mar 24, 2022

Problem

With ILM, index patter overridden without lifecycle policy set by collector if not explicitly disabled with --es.create-index-templates=false

Proposal

Making useILM disable index creation to prevent this issue to happen

Alternative

Make the actual cmd flags mutually exclusive. This is more explicit but is breaking
rbizos@12bff25

@rbizos rbizos requested a review from a team as a code owner March 24, 2022 16:19
@rbizos rbizos requested a review from vprithvi March 24, 2022 16:19
@rbizos rbizos mentioned this pull request Mar 24, 2022
Merged
@yurishkuro yurishkuro requested review from albertteoh and pavolloffay and removed request for vprithvi March 24, 2022 17:09
@albertteoh
Copy link
Copy Markdown
Contributor

albertteoh commented Mar 29, 2022

I just tested this locally with the following steps:

  1. Following instructions from: https://www.jaegertracing.io/docs/1.32/deployment/#enabling-ilm-support

  2. Starting collector with the following:

SPAN_STORAGE_TYPE=$SPAN_STORAGE_TYPE .../github.com/albertteoh/jaeger/cmd/collector/collector-linux-amd64  --es.use-aliases --es.use-ilm
  1. Checking ES indices and they seem to be rolling over with ILM without manual intervention:
green  open .apm-agent-configuration        bg-CPnErQ9iXiciW8af-TQ 1 0   0   0    226b    226b
yellow open jaeger-span-000001              nRh5LgdySyyLdqhjfXUjkQ 5 1   0   0   1.1kb   1.1kb
yellow open jaeger-span-000003              2C9rELz2QyG8Y0CdKPd-Gw 5 1 863   0 127.8kb 127.8kb
yellow open jaeger-span-000002              kqN_yFEuSfehrJDbOTy7Aw 5 1 720   0  84.4kb  84.4kb
green  open .geoip_databases                sAJYDvSmQZmkxiFXQvKgsQ 1 0  44   0  41.5mb  41.5mb
yellow open jaeger-dependencies-000001      Yi6EICzYTHOf7EqVrVHkPA 5 1   0   0   1.1kb   1.1kb
yellow open jaeger-dependencies-000002      Hs60pwBtQXKDYONqJ2wdOA 5 1   0   0   1.1kb   1.1kb
green  open .apm-custom-link                IjgfAFKjRPy0brMCdhiJUQ 1 0   0   0    226b    226b
green  open .kibana_7.16.1_001              UKRDgHJ9TOC00gnk5E_q6Q 1 0  25  25   2.3mb   2.3mb
green  open .kibana_task_manager_7.16.1_001 vnetekw6Sm-6lSyTowheYw 1 0  17 999 251.9kb 251.9kb
yellow open jaeger-service-000003           kl9iZj3lRJy-q3MDnUgG4Q 5 1   5   0  12.1kb  12.1kb
yellow open jaeger-service-000002           1rpklUbzQhiM5QCbEUqQnQ 5 1   4   0    16kb    16kb
yellow open jaeger-service-000001           Gux5tTk1THyK3B4fsWxPqw 5 1   0   0   1.1kb   1.1kb

There's a good chance I've misunderstood the problem you're describing.

Are you able to list the steps to reproduce this problem locally?

@rbizos
Copy link
Copy Markdown
Contributor Author

rbizos commented Mar 29, 2022 •
edited
Loading

Hello,
Those are the exact steps I used to reproduce the issue locally. After starting the collector, there is a single rollover for jaeger-span and jaeger-service. After a few rotation, I only have a single index for those 2 but the jaeger-dependencies continue to rollover as the template is not overridden when starting the collector:

curl localhost:9200/_cat/indices
green  open .geoip_databases           kyujjJkRSn63jEu-JZU9CA 1 0 44 0 41.4mb 41.4mb
yellow open jaeger-span-000005         U0PMk3G_R1SvEyKASC4FFw 5 1  0 0  1.1kb  1.1kb
yellow open jaeger-dependencies-000007 UVAfyP2TR8iub2_4J6ijOw 5 1  0 0  1.1kb  1.1kb
yellow open jaeger-service-000005      0eILVdibRLauPSRDH6azew 5 1  0 0  1.1kb  1.1kb
yellow open jaeger-dependencies-000005 AKZHip6vSXCwBLpfXZosUA 5 1  0 0  1.1kb  1.1kb
yellow open jaeger-dependencies-000006 12qlFEt5SM642811DuC5TA 5 1  0 0  1.1kb  1.1kb

@albertteoh
Copy link
Copy Markdown
Contributor

albertteoh commented Mar 30, 2022

The unit test appears to be failing:

    factory_test.go:196: 
        	Error Trace:	factory_test.go:196
        	Error:      	Expected nil, but got: &errors.errorString{s:"template-error"}
        	Test:       	TestILMDisableTemplateCreation

After a few rotation, I only have a single index for those 2 but the jaeger-dependencies continue to rollover as the template is not overridden when starting the collector

Ah yes, you're right. I observed this later on; the rolled over service and span indices are no longer managed by ILM.

@rbizos rbizos force-pushed the main branch 2 times, most recently from 85b7355 to e3a49fe Compare March 30, 2022 14:32
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2022 •
edited
Loading

Codecov Report

Merging #3599 (b63574a) into main (2a66711) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3599      +/-   ##
==========================================
- Coverage   96.54%   96.53%   -0.02%     
==========================================
  Files         264      264              
  Lines       15447    15449       +2     
==========================================
  Hits        14914    14914              
- Misses        449      451       +2     
  Partials       84       84
Impacted Files Coverage Δ
plugin/storage/es/factory.go 97.61% <100.00%> (+0.03%) ⬆️
pkg/config/tlscfg/cert_watcher.go 92.63% <0.00%> (-2.11%) ⬇️

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 2a66711...b63574a. Read the comment docs.

albertteoh
albertteoh previously approved these changes Mar 30, 2022
View reviewed changes
Copy link
Copy Markdown
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, thanks! Just a suggestion around adding a comment to explain why we shouldn't create index templates when ILM is enabled, to avoid confusion.

plugin/storage/es/factory.go
UseReadWriteAliases: cfg.GetUseReadWriteAliases(),
})
if cfg.IsCreateIndexTemplates() {
if cfg.IsCreateIndexTemplates() && !cfg.GetUseILM() {
Copy link
Copy Markdown
Contributor

@albertteoh albertteoh Mar 30, 2022

Choose a reason for hiding this comment

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

I suggest adding a comment to explain why we shouldn't create index templates when ILM is enabled.

Copy link
Copy Markdown
Contributor Author

@rbizos rbizos Apr 1, 2022

Choose a reason for hiding this comment

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

Of course

rbizos added 3 commits April 1, 2022 11:25
@rbizos
useILM will prevent index template creation
3b49a20 
The index template should be created by es-rollover. Without this, it
was mandatory to use --es.create-index-templates=false with
--es.use-ilm, otherwise it would override the index template with an
empty lifecycle.name thus preventing the rollover to work

Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
@rbizos
fixing ILM test
77c32ff 
Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
@rbizos
Adding a comment for ILM and index creation
b63574a 
Signed-off-by: Raphael Bizos <r.bizos@criteo.com>
@albertteoh
Copy link
Copy Markdown
Contributor

albertteoh commented Apr 1, 2022

@rbizos Ah, I think the builds are failing with Error: Cannot perform an interactive login from a non TTY device Error: Process completed with exit code 1. because you need to create a PR off a branch on your fork, not main. Please create a new PR from a feature branch.

@rbizos rbizos mentioned this pull request Apr 1, 2022
Merged
@rbizos
Copy link
Copy Markdown
Contributor Author

rbizos commented Apr 1, 2022 •
edited
Loading

@albertteoh done here #3610

@albertteoh albertteoh closed this Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albertteoh albertteoh albertteoh left review comments

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rbizos @albertteoh