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

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

Merged
merged 9 commits into from
May 8, 2019
Merged
Show file tree
Hide file tree
Changes from 8 commits
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
2 changes: 1 addition & 1 deletion CHANGELOG-developer.next.asciidoc
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
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
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
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
simitt marked this conversation as resolved.
Show resolved Hide resolved
}

err = m.Setup(tmplLoadMode, ilmLoadMode)
Expand Down
8 changes: 5 additions & 3 deletions libbeat/idxmgmt/idxmgmt.go
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
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
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

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

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

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

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

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
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
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
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

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
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
}
simitt marked this conversation as resolved.
Show resolved Hide resolved

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