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

Jump to bottom

Fix lustre2 input plugin config parse regression #6114

Merged
merged 4 commits into from
Jul 18, 2019
Merged

Fix lustre2 input plugin config parse regression #6114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a188a5
Add failing test for lustre2 input toml parsing regression
GeorgeMac Jul 12, 2019
50d5bad
Fix config parsing regression in lustre2 input
GeorgeMac Jul 12, 2019
01f2d55
Replace assert with require in lustre2 input plugin test
GeorgeMac Jul 17, 2019
539d768
Add explicit toml struct tags to lustre2 input plugin config
GeorgeMac Jul 17, 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
4 changes: 2 additions & 2 deletions plugins/inputs/lustre2/lustre2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ type tags struct {
// Lustre proc files can change between versions, so we want to future-proof
// by letting people choose what to look at.
type Lustre2 struct {
Ost_procfiles []string `toml:"ost_jobstat"`
Mds_procfiles []string `toml:"mds_jobstat"`
Ost_procfiles []string `toml:"ost_procfiles"`
Mds_procfiles []string `toml:"mds_procfiles"`

// allFields maps and OST name to the metric fields associated with that OST
allFields map[tags]map[string]interface{}
Expand Down
38 changes: 38 additions & 0 deletions plugins/inputs/lustre2/lustre2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import (
"testing"

"github.com/influxdata/telegraf/testutil"
"github.com/influxdata/toml"
"github.com/influxdata/toml/ast"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -330,3 +333,38 @@ func TestLustre2GeneratesJobstatsMetrics(t *testing.T) {
err = os.RemoveAll(os.TempDir() + "/telegraf")
require.NoError(t, err)
}

func TestLustre2CanParseConfiguration(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an observation. I have mixed feelings on this test, you could imagine having tests like this for all plugins and overall I think if we did this it would be too burdensome in it's current form. On the other hand obviously it does check for a valid bug.

Copy link
Contributor Author

@GeorgeMac GeorgeMac Jul 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that in this form it would be burdensome. I actually had mentioned about having a test like this everywhere with @goller.

Would a kind of test harness for all plugins be worthwhile?

e.g.

import "github.com/influxdata/telegraf/internal/harness"

func TestHarness(t *testing.T) {
    harness.TestInputPlugin(t, "some_plugin", &SomePlugin{})
}

Which would validate the plugin has been registered under some_plugin. Parse the result of plugin.SamplePlugin() and assert it against the provided telegraf.Input.

Could have one per plugin type and capture some high-level criteria like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'm ambivalent, it's not something that has historically been a huge source of bugs, so it might just not be worth the effort or a situation where we have bigger fish to fry. We could definitely provide a better interface to the config package to ease toml string -> plugin, but I'm not entirely sure how the harness would work. It seems like you need to provide at a minimum the input TOML with all options set, and a filled plugin struct to compare against.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah my illustration is lacking a little. For this PR shall I just leave it as is or remove the test?

The harness would ideally test all configurable fields, so the sample would need uncommenting in part if it were used. This could be done with a simple regexp, but that may be brittle and difficult to explain.

config := []byte(`
[[inputs.lustre2]]
ost_procfiles = [
"/proc/fs/lustre/obdfilter/*/stats",
"/proc/fs/lustre/osd-ldiskfs/*/stats",
]
mds_procfiles = [
"/proc/fs/lustre/mdt/*/md_stats",
]`)

table, err := toml.Parse([]byte(config))
require.NoError(t, err)

inputs, ok := table.Fields["inputs"]
require.True(t, ok)

lustre2, ok := inputs.(*ast.Table).Fields["lustre2"]
require.True(t, ok)

var plugin Lustre2

require.NoError(t, toml.UnmarshalTable(lustre2.([]*ast.Table)[0], &plugin))

assert.Equal(t, Lustre2{
Ost_procfiles: []string{
"/proc/fs/lustre/obdfilter/*/stats",
"/proc/fs/lustre/osd-ldiskfs/*/stats",
},
Mds_procfiles: []string{
"/proc/fs/lustre/mdt/*/md_stats",
},
}, plugin)
}