Skip to content

Refactor and add tests for template and ilm handling. #12065

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

Merged
simitt merged 9 commits into from
May 8, 2019
Merged

Refactor and add tests for template and ilm handling. #12065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e6bb393
Refactor and add tests for template and ilm handling.
simitt May 6, 2019
bc98f7a
Only set setup.ILM if nil
simitt May 6, 2019
0efa03e
Rework according to PR comments.
simitt May 6, 2019
530f6e7
Add changelog entry
simitt May 6, 2019
b0e027f
change log settings for tests.
simitt May 6, 2019
679d402
update tests
simitt May 7, 2019
293210a
fix tests
simitt May 7, 2019
e746f1c
fix last test
simitt May 7, 2019
331aebd
Update delete in tests.
simitt May 8, 2019
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
2 changes: 1 addition & 1 deletion CHANGELOG-developer.next.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,5 @@ The list below covers the major changes between 7.0.0-rc2 and master only.
by `make` and `mage`. Example: `export PYTHON_EXE=python2.7`. {pull}11212[11212]
- Prometheus helper for metricbeat contains now `Namespace` field for `prometheus.MetricsMappings` {pull}11424[11424]
- Update Jinja2 version to 2.10.1. {pull}11817[11817]
- Reduce idxmgmt.Supporter interface and rework export commands to reuse logic. {pull}11777[11777]
- Reduce idxmgmt.Supporter interface and rework export commands to reuse logic. {pull}11777[11777], {pull}12065[12065]
- Update urllib3 version to 1.24.2 {pull}11930[11930]
6 changes: 5 additions & 1 deletion libbeat/cmd/export/ilm_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/elastic/beats/libbeat/cmd/instance"
"github.com/elastic/beats/libbeat/idxmgmt"
"github.com/elastic/beats/libbeat/idxmgmt/ilm"
)

// GenGetILMPolicyCmd is the command used to export the ilm policy.
Expand All @@ -33,14 +34,17 @@ func GenGetILMPolicyCmd(settings instance.Settings) *cobra.Command {
version, _ := cmd.Flags().GetString("es.version")
dir, _ := cmd.Flags().GetString("dir")

if settings.ILM == nil {
settings.ILM = ilm.StdSupport
}
b, err := instance.NewInitializedBeat(settings)
if err != nil {
fatalfInitCmd(err)
}

clientHandler := idxmgmt.NewFileClientHandler(newIdxmgmtClient(dir, version))
idxManager := b.IdxSupporter.Manager(clientHandler, idxmgmt.BeatsAssets(b.Fields))
if err := idxManager.Setup(idxmgmt.LoadModeDisabled, idxmgmt.LoadModeEnabled); err != nil {
if err := idxManager.Setup(idxmgmt.LoadModeDisabled, idxmgmt.LoadModeForce); err != nil {
fatalf("Error exporting ilm-policy: %+v.", err)
}
},
Expand Down
14 changes: 5 additions & 9 deletions libbeat/cmd/export/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,9 @@ package export
import (
"github.com/spf13/cobra"

"github.com/elastic/beats/libbeat/beat"
"github.com/elastic/beats/libbeat/cmd/instance"
"github.com/elastic/beats/libbeat/common"
"github.com/elastic/beats/libbeat/idxmgmt"
"github.com/elastic/beats/libbeat/idxmgmt/ilm"
"github.com/elastic/beats/libbeat/logp"
)

// GenTemplateConfigCmd is the command used to export the elasticsearch template.
Expand All @@ -39,7 +36,10 @@ func GenTemplateConfigCmd(settings instance.Settings) *cobra.Command {
noILM, _ := cmd.Flags().GetBool("noilm")

if noILM {
settings.ILM = ilmNoopSupport
settings.ILM = ilm.NoopSupport
}
if settings.ILM == nil {
settings.ILM = ilm.StdSupport
}

b, err := instance.NewInitializedBeat(settings)
Expand All @@ -49,7 +49,7 @@ func GenTemplateConfigCmd(settings instance.Settings) *cobra.Command {

clientHandler := idxmgmt.NewFileClientHandler(newIdxmgmtClient(dir, version))
idxManager := b.IdxSupporter.Manager(clientHandler, idxmgmt.BeatsAssets(b.Fields))
if err := idxManager.Setup(idxmgmt.LoadModeEnabled, idxmgmt.LoadModeDisabled); err != nil {
if err := idxManager.Setup(idxmgmt.LoadModeForce, idxmgmt.LoadModeDisabled); err != nil {
fatalf("Error exporting template: %+v.", err)
}
},
Expand All @@ -61,7 +61,3 @@ func GenTemplateConfigCmd(settings instance.Settings) *cobra.Command {

return genTemplateConfigCmd
}

func ilmNoopSupport(_ *logp.Logger, info beat.Info, config *common.Config) (ilm.Supporter, error) {
return ilm.NoopSupport(info, config)
}
4 changes: 2 additions & 2 deletions libbeat/cmd/instance/beat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,10 +490,10 @@ func (b *Beat) Setup(settings Settings, bt beat.Creator, setup SetupSettings) er
m := b.IdxSupporter.Manager(idxmgmt.NewESClientHandler(esClient), idxmgmt.BeatsAssets(b.Fields))
var tmplLoadMode, ilmLoadMode = idxmgmt.LoadModeUnset, idxmgmt.LoadModeUnset
if setup.Template {
tmplLoadMode = idxmgmt.LoadModeForce
tmplLoadMode = idxmgmt.LoadModeOverwrite
}
if setup.ILMPolicy {
ilmLoadMode = idxmgmt.LoadModeForce
ilmLoadMode = idxmgmt.LoadModeOverwrite
}

err = m.Setup(tmplLoadMode, ilmLoadMode)
Expand Down
8 changes: 5 additions & 3 deletions libbeat/idxmgmt/idxmgmt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,14 @@ const (
// LoadModeUnset indicates that no specific mode is set.
// Instead the decision about loading data will be derived from the config or their respective default values.
LoadModeUnset LoadMode = iota //unset
// LoadModeDisabled indicates no loading
LoadModeDisabled //disabled
// LoadModeEnabled indicates loading if not already available
LoadModeEnabled //enabled
// LoadModeForce indicates loading in any case.
// LoadModeOverwrite indicates overwriting existing components, if loading is not generally disabled.
LoadModeOverwrite //overwrite
// LoadModeForce indicates forcing to load components in any case, independent of general loading configurations.
LoadModeForce //force
// LoadModeDisabled indicates no loading
LoadModeDisabled //disabled
)

// Enabled returns whether or not the LoadMode should be considered enabled
Expand Down
36 changes: 35 additions & 1 deletion libbeat/idxmgmt/idxmgmt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,24 @@ func TestDefaultSupport_TemplateHandling(t *testing.T) {
loadTemplate: LoadModeEnabled,
tmplCfg: &defaultCfg,
},
"template default loadMode Overwrite, ilm disabled": {
cfg: common.MapStr{
"setup.ilm.enabled": false,
},
loadTemplate: LoadModeOverwrite,
tmplCfg: cfgWith(template.DefaultConfig(), map[string]interface{}{
"overwrite": "true",
}),
},
"template default loadMode Force, ilm disabled": {
cfg: common.MapStr{
"setup.ilm.enabled": false,
},
loadTemplate: LoadModeForce,
tmplCfg: cfgWith(template.DefaultConfig(), map[string]interface{}{
"overwrite": "true",
}),
},
"template loadMode disabled, ilm disabled": {
cfg: common.MapStr{
"setup.ilm.enabled": false,
Expand All @@ -281,6 +299,22 @@ func TestDefaultSupport_TemplateHandling(t *testing.T) {
alias: "test-9.9.9",
policy: "test-9.9.9",
},
"template disabled, ilm disabled, loadMode Overwrite": {
cfg: common.MapStr{
"setup.template.enabled": false,
"setup.ilm.enabled": false,
},
loadILM: LoadModeOverwrite,
},
"template disabled, ilm disabled, loadMode Force": {
cfg: common.MapStr{
"setup.template.enabled": false,
"setup.ilm.enabled": false,
},
loadILM: LoadModeForce,
alias: "test-9.9.9",
policy: "test-9.9.9",
},
"template loadmode disabled, ilm loadMode enabled": {
loadTemplate: LoadModeDisabled,
loadILM: LoadModeEnabled,
Expand All @@ -304,7 +338,7 @@ func TestDefaultSupport_TemplateHandling(t *testing.T) {
for name, test := range cases {
t.Run(name, func(t *testing.T) {
info := beat.Info{Beat: "test", Version: "9.9.9"}
factory := MakeDefaultSupport(nil)
factory := MakeDefaultSupport(ilm.StdSupport)
im, err := factory(nil, info, common.MustNewConfigFrom(test.cfg))
require.NoError(t, err)

Expand Down
29 changes: 23 additions & 6 deletions libbeat/idxmgmt/ilm/ilm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,22 @@ type Alias struct {

// DefaultSupport configures a new default ILM support implementation.
func DefaultSupport(log *logp.Logger, info beat.Info, config *common.Config) (Supporter, error) {
cfg := defaultConfig(info)
if config != nil {
if err := config.Unpack(&cfg); err != nil {
return nil, err
}
}

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

return StdSupport(log, info, config)
}

// StdSupport configures a new std ILM support implementation.
func StdSupport(log *logp.Logger, info beat.Info, config *common.Config) (Supporter, error) {
if log == nil {
log = logp.NewLogger("ilm")
} else {
Expand All @@ -83,10 +99,6 @@ func DefaultSupport(log *logp.Logger, info beat.Info, config *common.Config) (Su
}
}

if cfg.Mode == ModeDisabled {
return NoopSupport(info, config)
}
Copy link

@urso urso May 6, 2019

Choose a reason for hiding this comment

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

Semantic change, bug fix or noop-change? The cfg.Mode is unpacked from the config at the code block right before this check. Does stdSupport act like noopSupport if cfg.Mode == ModeDisabled? Or do we want to ensure that Alias/Policy names are set?

Copy link
Contributor Author

@simitt simitt May 6, 2019

Choose a reason for hiding this comment

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

Noop, the diff is a bit confusing. Basically I only introduce two new public fcts StdSupport and NewNoopSupport as they can be reused in the export cmds.

Copy link

@urso urso May 8, 2019

Choose a reason for hiding this comment

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

Yes, this I noticed. But StdSupport can still be disabled via settings. Is it guaranteed to behave like NoopSupport in this case?

Copy link
Contributor Author

@simitt simitt May 8, 2019

Choose a reason for hiding this comment

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

No StdSupport does not behave like NoopSupport, but it cfg.Mode == ModeDisabled it still returns NoopSupport, see https://github.com/elastic/beats/pull/12065/files/bc98f7a142d53f74138e3e349efc0a7f42f7643f#diff-1cf0f286dd8a8b51745db0683d527a62R80.

I simply extracted creating the StdSupport into its own method so it is reusable.

Copy link

@urso urso May 8, 2019

Choose a reason for hiding this comment

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

Yeah, I noticed that DefaultSupport semantics didn't change. I'm just curious if StdSupport behaves correctly if cfg.Mode == ModeDisabled on construction. As the stdManager.Enabled returns false if ModeDisabled, it should behave correcly, though. Yet the other methods do not check the mode and still happily try to execute the commands.

This turns the Manager interface more into a protocol in a sense of: The other methods MUST NOT be used if Enabled did return false. This is true in the current code base, but only some internal assumption.


name, err := applyStaticFmtstr(info, &cfg.PolicyName)
if err != nil {
return nil, errors.Wrap(err, "failed to read ilm policy name")
Expand Down Expand Up @@ -115,8 +127,13 @@ func DefaultSupport(log *logp.Logger, info beat.Info, config *common.Config) (Su
policy.Body = body
}

log.Infof("Policy name: %v", name)
return NewDefaultSupport(log, cfg.Mode, alias, policy, cfg.Overwrite, cfg.CheckExists), nil
return NewStdSupport(log, cfg.Mode, alias, policy, cfg.Overwrite, cfg.CheckExists), nil
}

// NoopSupport configures a new noop ILM support implementation,
// should be used when ILM is disabled
func NoopSupport(_ *logp.Logger, info beat.Info, config *common.Config) (Supporter, error) {
return NewNoopSupport(info, config)
}

func applyStaticFmtstr(info beat.Info, fmt *fmtstr.EventFormatString) (string, error) {
Expand Down
2 changes: 1 addition & 1 deletion libbeat/idxmgmt/ilm/ilm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func TestDefaultSupport_Init(t *testing.T) {
))
require.NoError(t, err)

s := tmp.(*ilmSupport)
s := tmp.(*stdSupport)
assert := assert.New(t)
assert.Equal(true, s.overwrite)
assert.Equal(false, s.checkExists)
Expand Down
4 changes: 2 additions & 2 deletions libbeat/idxmgmt/ilm/noop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ import (
type noopSupport struct{}
type noopManager struct{}

// NoopSupport creates a noop ILM implementation with ILM support being always
// NewNoopSupport creates a noop ILM implementation with ILM support being always
// disabled. Attempts to install a policy or create a write alias will fail.
func NoopSupport(info beat.Info, config *common.Config) (Supporter, error) {
func NewNoopSupport(info beat.Info, config *common.Config) (Supporter, error) {
return (*noopSupport)(nil), nil
}

Expand Down
30 changes: 15 additions & 15 deletions libbeat/idxmgmt/ilm/std.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
"github.com/elastic/beats/libbeat/logp"
)

type ilmSupport struct {
type stdSupport struct {
log *logp.Logger

mode Mode
Expand All @@ -34,8 +34,8 @@ type ilmSupport struct {
policy Policy
}

type singlePolicyManager struct {
*ilmSupport
type stdManager struct {
*stdSupport
client ClientHandler

// cached info
Expand All @@ -49,15 +49,15 @@ type infoCache struct {

var defaultCacheDuration = 5 * time.Minute

// NewDefaultSupport creates an instance of default ILM support implementation.
func NewDefaultSupport(
// NewStdSupport creates an instance of default ILM support implementation.
func NewStdSupport(
log *logp.Logger,
mode Mode,
alias Alias,
policy Policy,
overwrite, checkExists bool,
) Supporter {
return &ilmSupport{
return &stdSupport{
log: log,
mode: mode,
overwrite: overwrite,
Expand All @@ -67,18 +67,18 @@ func NewDefaultSupport(
}
}

func (s *ilmSupport) Mode() Mode { return s.mode }
func (s *ilmSupport) Alias() Alias { return s.alias }
func (s *ilmSupport) Policy() Policy { return s.policy }
func (s *stdSupport) Mode() Mode { return s.mode }
func (s *stdSupport) Alias() Alias { return s.alias }
func (s *stdSupport) Policy() Policy { return s.policy }

func (s *ilmSupport) Manager(h ClientHandler) Manager {
return &singlePolicyManager{
func (s *stdSupport) Manager(h ClientHandler) Manager {
return &stdManager{
client: h,
ilmSupport: s,
stdSupport: s,
}
}

func (m *singlePolicyManager) Enabled() (bool, error) {
func (m *stdManager) Enabled() (bool, error) {
if m.mode == ModeDisabled {
return false, nil
}
Expand All @@ -101,7 +101,7 @@ func (m *singlePolicyManager) Enabled() (bool, error) {
return enabled, nil
}

func (m *singlePolicyManager) EnsureAlias() error {
func (m *stdManager) EnsureAlias() error {
b, err := m.client.HasAlias(m.alias.Name)
if err != nil {
return err
Expand All @@ -114,7 +114,7 @@ func (m *singlePolicyManager) EnsureAlias() error {
return m.client.CreateAlias(m.alias)
}

func (m *singlePolicyManager) EnsurePolicy(overwrite bool) (bool, error) {
func (m *stdManager) EnsurePolicy(overwrite bool) (bool, error) {
log := m.log
overwrite = overwrite || m.overwrite

Expand Down
11 changes: 6 additions & 5 deletions libbeat/idxmgmt/loadmode_string.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 7 additions & 6 deletions libbeat/idxmgmt/std.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,20 +197,20 @@ func (m *indexManager) Setup(loadTemplate, loadILM LoadMode) error {
}
}

if withILM && loadILM.Enabled() {
if loadILM == LoadModeForce || withILM && loadILM.Enabled() {
// mark ILM as enabled in indexState if withILM is true
m.support.st.withILM.CAS(false, true)

// install ilm policy
policyCreated, err := m.ilm.EnsurePolicy(loadILM == LoadModeForce)
policyCreated, err := m.ilm.EnsurePolicy(loadILM >= LoadModeOverwrite)
if err != nil {
return err
}
log.Info("ILM policy successfully loaded.")

// The template should be updated if a new policy is created.
if policyCreated && loadTemplate.Enabled() {
loadTemplate = LoadModeForce
loadTemplate = LoadModeOverwrite
}

// create alias
Expand All @@ -225,7 +225,7 @@ func (m *indexManager) Setup(loadTemplate, loadILM LoadMode) error {
}

// create and install template
if m.support.templateCfg.Enabled && loadTemplate.Enabled() {
if loadTemplate == LoadModeForce || m.support.templateCfg.Enabled && loadTemplate.Enabled() {
tmplCfg := m.support.templateCfg

if withILM {
Expand All @@ -237,11 +237,12 @@ func (m *indexManager) Setup(loadTemplate, loadILM LoadMode) error {
}

if loadTemplate == LoadModeForce {
tmplCfg.Enabled = true
}
if loadTemplate >= LoadModeOverwrite {
tmplCfg.Overwrite = true
}

fields := m.assets.Fields(m.support.info.Beat)

err = m.clientHandler.Load(tmplCfg, m.support.info, fields, m.support.migration)
if err != nil {
return fmt.Errorf("error loading template: %v", err)
Expand Down
Loading