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

Remove auto option from setup.ilm.enabled and set the default value to true #28671

Merged
merged 4 commits into from
Oct 29, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
rm auto option
  • Loading branch information
kvch committed Oct 27, 2021
commit 51fd6786da9c84703d7c11321878b03cef741949
52 changes: 16 additions & 36 deletions libbeat/idxmgmt/ilm/client_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (

// ClientHandler defines the interface between a remote service and the Manager.
type ClientHandler interface {
CheckILMEnabled(Mode) (bool, error)
CheckILMEnabled(bool) (bool, error)

HasAlias(name string) (bool, error)
CreateAlias(alias Alias) error
Expand Down Expand Up @@ -92,40 +92,29 @@ func NewFileClientHandler(c FileClient) *FileClientHandler {
}

// CheckILMEnabled indicates whether or not ILM is supported for the configured mode and ES instance.
func (h *ESClientHandler) CheckILMEnabled(mode Mode) (bool, error) {
if mode == ModeDisabled {
func (h *ESClientHandler) CheckILMEnabled(enabled bool) (bool, error) {
if !enabled {
return false, nil
}

avail, probe := checkILMVersion(mode, h.client.GetVersion())
if !avail {
if mode == ModeEnabled {
ver := h.client.GetVersion()
return false, errf(ErrESVersionNotSupported,
"Elasticsearch %v does not support ILM", ver.String())
}
return false, nil
}

if !probe {
// version potentially supports ILM, but mode + version indicates that we
// want to disable ILM support.
return false, nil
ver := h.client.GetVersion()
ruflin marked this conversation as resolved.
Show resolved Hide resolved
if !checkILMVersion(h.client.GetVersion()) {
return false, errf(ErrESVersionNotSupported, "Elasticsearch %v does not support ILM", ver.String())
}

avail, enabled, err := h.checkILMSupport()
avail, enabledILM, err := h.checkILMSupport()
if err != nil {
return false, err
}

if !avail {
if mode == ModeEnabled {
if enabledILM {
return false, errOf(ErrESVersionNotSupported)
}
return false, nil
}

if !enabled && mode == ModeEnabled {
if !enabledILM && enabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

if enabled == false we should never get to this line of code because of line 96?

return false, errOf(ErrESILMDisabled)
}
return enabled, nil
Expand Down Expand Up @@ -244,18 +233,14 @@ func (h *ESClientHandler) queryFeatures(to interface{}) (int, error) {
}

// CheckILMEnabled indicates whether or not ILM is supported for the configured mode and client version.
func (h *FileClientHandler) CheckILMEnabled(mode Mode) (bool, error) {
if mode == ModeDisabled {
return false, nil
}
avail, probe := checkILMVersion(mode, h.client.GetVersion())
if avail {
return probe, nil
}
if mode != ModeEnabled {
func (h *FileClientHandler) CheckILMEnabled(enabled bool) (bool, error) {
if !enabled {
return false, nil
}
version := h.client.GetVersion()
if checkILMVersion(version) {
return enabled, nil
}
return false, errf(ErrESVersionNotSupported,
"Elasticsearch %v does not support ILM", version.String())
}
Expand Down Expand Up @@ -287,11 +272,6 @@ func (h *FileClientHandler) HasAlias(name string) (bool, error) {
// avail: indicates whether version supports ILM
// probe: in case version potentially supports ILM, check the combination of mode + version
// to indicate whether or not ILM support should be enabled or disabled
func checkILMVersion(mode Mode, ver common.Version) (avail, probe bool) {
avail = !ver.LessThan(esMinILMVersion)
if avail {
probe = (mode == ModeEnabled) ||
(mode == ModeAuto && !ver.LessThan(esMinDefaultILMVersion))
}
return avail, probe
func checkILMVersion(ver common.Version) (avail bool) {
return !ver.LessThan(esMinILMVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked in the code what esMinILMVersion and it is 6.6.0, so we will also be able to get rid of this code 🥳

Copy link
Collaborator

Choose a reason for hiding this comment

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

So let's clean this up 👍🏼

}
42 changes: 13 additions & 29 deletions libbeat/idxmgmt/ilm/client_handler_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,14 @@ const (
func TestESClientHandler_CheckILMEnabled(t *testing.T) {
t.Run("no ilm if disabled", func(t *testing.T) {
h := newESClientHandler(t)
b, err := h.CheckILMEnabled(ilm.ModeDisabled)
b, err := h.CheckILMEnabled(false)
assert.NoError(t, err)
assert.False(t, b)
})

t.Run("with ilm if auto", func(t *testing.T) {
h := newESClientHandler(t)
b, err := h.CheckILMEnabled(ilm.ModeAuto)
assert.NoError(t, err)
assert.True(t, b)
})

t.Run("with ilm if enabled", func(t *testing.T) {
h := newESClientHandler(t)
b, err := h.CheckILMEnabled(ilm.ModeEnabled)
b, err := h.CheckILMEnabled(true)
assert.NoError(t, err)
assert.True(t, b)
})
Expand Down Expand Up @@ -268,38 +261,29 @@ func getEnv(name, def string) string {

func TestFileClientHandler_CheckILMEnabled(t *testing.T) {
for name, test := range map[string]struct {
m ilm.Mode
version string
enabled bool
err bool
enabled bool
version string
ilmEnabled bool
err bool
}{
"ilm enabled": {
m: ilm.ModeEnabled,
enabled: true,
},
"ilm auto": {
m: ilm.ModeAuto,
enabled: true,
enabled: true,
ilmEnabled: true,
},
"ilm disabled": {
m: ilm.ModeDisabled,
enabled: false,
enabled: false,
ilmEnabled: false,
},
"ilm enabled, version too old": {
m: ilm.ModeEnabled,
enabled: true,
version: "5.0.0",
err: true,
},
"ilm auto, version too old": {
m: ilm.ModeAuto,
version: "5.0.0",
enabled: false,
},
} {
t.Run(name, func(t *testing.T) {
h := ilm.NewFileClientHandler(newMockClient(test.version))
b, err := h.CheckILMEnabled(test.m)
assert.Equal(t, test.enabled, b)
b, err := h.CheckILMEnabled(test.enabled)
assert.Equal(t, test.ilmEnabled, b)
if test.err {
assert.Error(t, err)
} else {
Expand Down
44 changes: 3 additions & 41 deletions libbeat/idxmgmt/ilm/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ package ilm

import (
"fmt"
"strconv"
"strings"

"github.com/elastic/beats/v7/libbeat/beat"
"github.com/elastic/beats/v7/libbeat/common"
Expand All @@ -29,7 +27,7 @@ import (

// Config is used for unpacking a common.Config.
type Config struct {
Mode Mode `config:"enabled"`
Enabled bool `config:"enabled"`
PolicyName fmtstr.EventFormatString `config:"policy_name"`
PolicyFile string `config:"policy_file"`
RolloverAlias fmtstr.EventFormatString `config:"rollover_alias"`
Expand All @@ -44,20 +42,6 @@ type Config struct {
Overwrite bool `config:"overwrite"`
}

//Mode is used for enumerating the ilm mode.
type Mode uint8

const (
//ModeAuto enum 'auto'
ModeAuto Mode = iota

//ModeEnabled enum 'true'
ModeEnabled

//ModeDisabled enum 'false'
ModeDisabled
)

const ilmDefaultPattern = "{now/d}-000001"

// DefaultPolicy defines the default policy to be used if no custom policy is
Expand All @@ -79,31 +63,9 @@ var DefaultPolicy = common.MapStr{
},
}

//Unpack creates enumeration value true, false or auto
func (m *Mode) Unpack(in string) error {
in = strings.ToLower(in)

if in == "auto" {
*m = ModeAuto
return nil
}

b, err := strconv.ParseBool(in)
if err != nil {
return fmt.Errorf("ilm.enabled` mode '%v' is invalid (try auto, true, false)", in)
}

if b {
*m = ModeEnabled
} else {
*m = ModeDisabled
}
return nil
}

//Validate verifies that expected config options are given and valid
func (cfg *Config) Validate() error {
if cfg.RolloverAlias.IsEmpty() && cfg.Mode != ModeDisabled {
if cfg.RolloverAlias.IsEmpty() && cfg.Enabled {
return fmt.Errorf("rollover_alias must be set when ILM is not disabled")
}
return nil
Expand All @@ -115,7 +77,7 @@ func defaultConfig(info beat.Info) Config {
policyFmt := fmtstr.MustCompileEvent(info.Beat)

return Config{
Mode: ModeAuto,
Enabled: true,
PolicyName: *policyFmt,
RolloverAlias: *aliasFmt,
Pattern: ilmDefaultPattern,
Expand Down
6 changes: 3 additions & 3 deletions libbeat/idxmgmt/ilm/ilm.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type SupportFactory func(*logp.Logger, beat.Info, *common.Config) (Supporter, er
// write alias a manager instance must be generated.
type Supporter interface {
// Query settings
Mode() Mode
Enabled() bool
ruflin marked this conversation as resolved.
Show resolved Hide resolved
Alias() Alias
Policy() Policy
Overwrite() bool
Expand Down Expand Up @@ -82,7 +82,7 @@ func DefaultSupport(log *logp.Logger, info beat.Info, config *common.Config) (Su
}
}

if cfg.Mode == ModeDisabled {
if !cfg.Enabled {
return NewNoopSupport(info, config)
}

Expand Down Expand Up @@ -137,7 +137,7 @@ func StdSupport(log *logp.Logger, info beat.Info, config *common.Config) (Suppor
policy.Body = body
}

return NewStdSupport(log, cfg.Mode, alias, policy, cfg.Overwrite, cfg.CheckExists), nil
return NewStdSupport(log, cfg.Enabled, alias, policy, cfg.Overwrite, cfg.CheckExists), nil
}

// NoopSupport configures a new noop ILM support implementation,
Expand Down
38 changes: 9 additions & 29 deletions libbeat/idxmgmt/ilm/ilm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,27 +31,6 @@ import (
func TestDefaultSupport_Init(t *testing.T) {
info := beat.Info{Beat: "test", Version: "9.9.9"}

t.Run("mode from config", func(t *testing.T) {
cases := map[string]Mode{
"true": ModeEnabled,
"false": ModeDisabled,
"auto": ModeAuto,
}
for setting, expected := range cases {
expected := expected
t.Run(setting, func(t *testing.T) {
cfg := common.MustNewConfigFrom(map[string]interface{}{
"enabled": setting,
"rollover_alias": "test",
})

s, err := DefaultSupport(nil, info, cfg)
require.NoError(t, err)
assert.Equal(t, expected, s.Mode())
})
}
})

t.Run("with an empty rollover_alias", func(t *testing.T) {
_, err := DefaultSupport(nil, info, common.MustNewConfigFrom(
map[string]interface{}{
Expand Down Expand Up @@ -82,7 +61,7 @@ func TestDefaultSupport_Init(t *testing.T) {
assert := assert.New(t)
assert.Equal(true, s.overwrite)
assert.Equal(false, s.checkExists)
assert.Equal(ModeEnabled, s.Mode())
assert.Equal(true, s.Enabled())
assert.Equal(DefaultPolicy, common.MapStr(s.Policy().Body))
assert.Equal(Alias{Name: "alias", Pattern: "01"}, s.Alias())
})
Expand All @@ -103,7 +82,7 @@ func TestDefaultSupport_Init(t *testing.T) {
assert := assert.New(t)
assert.Equal(true, s.overwrite)
assert.Equal(false, s.checkExists)
assert.Equal(ModeEnabled, s.Mode())
assert.Equal(true, s.Enabled())
assert.Equal(DefaultPolicy, common.MapStr(s.Policy().Body))
assert.Equal(Alias{Name: "alias-9.9.9", Pattern: "01"}, s.Alias())
})
Expand All @@ -123,7 +102,7 @@ func TestDefaultSupport_Init(t *testing.T) {
assert := assert.New(t)
assert.Equal(true, s.overwrite)
assert.Equal(false, s.checkExists)
assert.Equal(ModeEnabled, s.Mode())
assert.Equal(true, s.Enabled())
assert.Equal(DefaultPolicy, common.MapStr(s.Policy().Body))
assert.Equal(Alias{Name: "test-9.9.9", Pattern: "01"}, s.Alias())
})
Expand All @@ -150,32 +129,33 @@ func TestDefaultSupport_Manager_Enabled(t *testing.T) {
},
"disabled via handler": {
calls: []onCall{
onCheckILMEnabled(ModeAuto).Return(false, nil),
onCheckILMEnabled(true).Return(false, ErrESILMDisabled),
},
err: true,
},
"enabled via handler": {
calls: []onCall{
onCheckILMEnabled(ModeAuto).Return(true, nil),
onCheckILMEnabled(true).Return(true, nil),
},
enabled: true,
},
"handler confirms enabled flag": {
calls: []onCall{
onCheckILMEnabled(ModeEnabled).Return(true, nil),
onCheckILMEnabled(true).Return(true, nil),
},
cfg: map[string]interface{}{"enabled": true},
enabled: true,
},
"fail enabled": {
calls: []onCall{
onCheckILMEnabled(ModeEnabled).Return(false, nil),
onCheckILMEnabled(true).Return(false, nil),
},
cfg: map[string]interface{}{"enabled": true},
fail: ErrESILMDisabled,
},
"io error": {
calls: []onCall{
onCheckILMEnabled(ModeAuto).Return(false, errors.New("ups")),
onCheckILMEnabled(true).Return(false, errors.New("ups")),
},
cfg: map[string]interface{}{},
err: true,
Expand Down
6 changes: 3 additions & 3 deletions libbeat/idxmgmt/ilm/mockapihandler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ func newMockHandler(calls ...onCall) *mockHandler {
return m
}

func onCheckILMEnabled(m Mode) onCall { return makeOnCall("CheckILMEnabled", m) }
func (h *mockHandler) CheckILMEnabled(mode Mode) (bool, error) {
args := h.Called(mode)
func onCheckILMEnabled(enabled bool) onCall { return makeOnCall("CheckILMEnabled", enabled) }
func (h *mockHandler) CheckILMEnabled(enabled bool) (bool, error) {
args := h.Called(enabled)
return args.Bool(0), args.Error(1)
}

Expand Down
2 changes: 1 addition & 1 deletion libbeat/idxmgmt/ilm/noop.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func NewNoopSupport(info beat.Info, config *common.Config) (Supporter, error) {
return (*noopSupport)(nil), nil
}

func (*noopSupport) Mode() Mode { return ModeDisabled }
func (*noopSupport) Enabled() bool { return false }
func (*noopSupport) Alias() Alias { return Alias{} }
func (*noopSupport) Policy() Policy { return Policy{} }
func (*noopSupport) Overwrite() bool { return false }
Expand Down
Loading