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.

Sign up for GitHub

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

Do not try to load ILM if check_exists if false #27508

Merged
merged 3 commits into from
Aug 24, 2021
Merged

Do not try to load ILM if check_exists if false #27508

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d4bc4f
Do try to load ILM if check_exists if false
kvch Aug 19, 2021
351af3c
update docs with correct behaviour
kvch Aug 23, 2021
3834524
Add changelog entry
kvch Aug 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions libbeat/idxmgmt/ilm/ilm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,21 +243,21 @@ func TestDefaultSupport_Manager_EnsureAlias(t *testing.T) {
onCreateAlias(alias).Return(nil),
},
fail: nil,
cfg: map[string]interface{}{"check_exists": false, "overwrite": true},
cfg: map[string]interface{}{"check_exists": true, "overwrite": true},
},
"try overwrite existing": {
calls: []onCall{
onCreateAlias(alias).Return(errOf(ErrAliasAlreadyExists)),
},
fail: nil, // we detect that that the alias exists, and call it a day.
cfg: map[string]interface{}{"check_exists": false, "overwrite": true},
cfg: map[string]interface{}{"check_exists": true, "overwrite": true},
},
"fail to overwrite": {
calls: []onCall{
onCreateAlias(alias).Return(errOf(ErrAliasCreateFailed)),
},
fail: ErrAliasCreateFailed,
cfg: map[string]interface{}{"check_exists": false, "overwrite": true},
cfg: map[string]interface{}{"check_exists": true, "overwrite": true},
},
}

Expand Down
14 changes: 12 additions & 2 deletions libbeat/idxmgmt/ilm/std.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,16 @@ func (m *stdManager) CheckEnabled() (bool, error) {

func (m *stdManager) EnsureAlias() error {
log := m.log
if !m.checkExists {
Copy link
Member

Choose a reason for hiding this comment

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

Should this also check !overwrite? The docs say:

beats/metricbeat/metricbeat.reference.yml

Lines 2100 to 2103 in bbf7a58

# Disable the check for an existing lifecycle policy. The default is true. If
# you disable this check, set setup.ilm.overwrite: true so the lifecycle policy
# can be installed.
#setup.ilm.check_exists: true

So I think it's fair to say that if both are false, then no ILM policy is created. Still weird config, but at least consistent with the docs.

I think the alternative is to tell users to set setup.ilm.enabled:false, and set output.elasticsearch.index to the write index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As agreed in #27508 (comment) I will rather correct the behaviour of the options, not work around existing bad practice.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine by me either way, but for the record:

The config is nominally about ILM setup, but it's being overloaded to also configure the output index name. Seems to me this is the source of the problem. The user in #26322 wants to be able to configure the output index to work with ILM without setting up ILM.

Any fix other than introducing new config is going to be less than ideal, so I guess this one is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. I have responded to the original issue with your suggestions. However, I think my code still makes sense.

log.Infof("Index alias is not checked as setup.ilm.check_exists is disabled")
return nil
}

overwrite := m.Overwrite()
name := m.alias.Name

var exists bool
if m.checkExists && !overwrite {
if !overwrite {
var err error
exists, err = m.client.HasAlias(name)
if err != nil {
Expand Down Expand Up @@ -143,11 +148,16 @@ func (m *stdManager) EnsureAlias() error {

func (m *stdManager) EnsurePolicy(overwrite bool) (bool, error) {
log := m.log
if !m.checkExists {
log.Infof("ILM policy is not checked as setup.ilm.check_exists is disabled")
return false, nil
}

overwrite = overwrite || m.Overwrite()
name := m.policy.Name

var exists bool
if m.checkExists && !overwrite {
if !overwrite {
var err error
exists, err = m.client.HasILMPolicy(name)
if err != nil {
Expand Down