Add Bond Interface input plugin by ildarsv · Pull Request #3424 · influxdata/telegraf

ildarsv · 2017-11-03T11:08:50Z

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

…add-bond-input-plugin

danielnelson · 2017-11-08T00:03:25Z

plugins/inputs/bond/bond.go

+
+// default bond directory path
+const (
+	BOND_PATH = "/proc/net/bonding"


I would say call this defaultBondPath because Go naming guidelines, but to match existing style we should only specify the proc location. We can add it to the config as host_proc = "/proc", but we should also load it from the HOST_PROC environment variable.

danielnelson · 2017-11-08T00:06:20Z

plugins/inputs/bond/bond.go

+	for _, bondName := range bond.BondInterfaces {
+		file, err := ioutil.ReadFile(bond.BondPath + "/" + bondName)
+		if err != nil {
+			acc.AddError(fmt.Errorf("E! error due inspecting '%s' interface: %v", bondName, err))


The log level E! will be added by the accumulator, so just "error inspecting '%s' interface: %v", only add a log level if directly using the logger. Check for this throughout pull request.

danielnelson · 2017-11-08T00:17:19Z

plugins/inputs/bond/bond.go

+		"bond": bondName,
+	}
+
+	lines := strings.Split(rawFile, "\n")


Use a bufio.Scanner to avoid EOL encoding issues.

danielnelson · 2017-11-08T00:20:47Z

plugins/inputs/bond/bond.go

+		if len(stats) < 2 {
+			continue
+		}
+		name := strings.ToLower(strings.Replace(strings.TrimSpace(stats[0]), " ", "_", -1))


Why lowercase and replace spaces with underscores?

It's too excessive, my bad. Removed.

danielnelson · 2017-11-08T00:21:21Z

plugins/inputs/bond/bond.go

+	var slave string
+	var status int
+
+	lines := strings.Split(rawFile, "\n")


bufio.Scanner

danielnelson · 2017-11-08T00:30:25Z

plugins/inputs/bond/bond.go

+	if bond.BondPath == "" {
+		bond.BondPath = BOND_PATH
+	}
+	if len(bond.BondInterfaces) == 0 {


Don't cache the bond interfaces, or we won't be able to see new bonds without restarting Telegraf. Return the paths instead of adding them to the struct.

danielnelson · 2017-11-08T00:37:07Z

plugins/inputs/bond/README.md

+  ## By default, telegraf gather stats for all bond interfaces
+  ## Setting interfaces will restrict the stats to the specified
+  ## bond interfaces.
+  bond_interfaces = ["bond0", "bond1"]


Comment this line out with a single #, so the default will be used.

danielnelson · 2017-11-08T00:37:36Z

plugins/inputs/bond/README.md

+[[inputs.bond]]
+  ## Sets bonding directory path
+  ## If not specified, then default is:
+  bond_path = "/proc/net/bonding"


Comment out since this is default.

danielnelson · 2017-11-08T00:39:28Z

plugins/inputs/bond/bond_test.go

+Down Delay (ms): 0
+
+Slave Interface: eth3
+MII Status: down


It's also possible for both of the interfaces to be up, right? Maybe we should gather the active slave on the bond master?

Sure, I've added active_slave field. Thanks for the note.

- align README.md - made host_proc configurable, add loading from env variables - removed log levels from error messages - used bufio.Scanner instead of strings.Split - removed cache of bond interfaces - gathered active slave for active-backuo mode

ildarsv · 2017-11-19T19:50:48Z

Hello @danielnelson ! Thank you for review. I've made some changes and improvements. Please take a look when you have time.

danielnelson · 2017-11-28T19:36:50Z

plugins/inputs/bond/bond.go

+var sampleConfig = `
+	## Sets 'proc' directory path
+	## If not specified, then default is /proc
+	# host_proc = "/proc"


Fix indention

danielnelson · 2017-11-28T19:36:55Z

plugins/inputs/bond/README.md

+
+  ## Sets 'bonding' directory relative path
+  ## If not specified, then default is /net/bonding
+  # bond_path = "/net/bonding"


Would this ever not be "/net/bonding"?

Seems no, would you like me to hardcode this parameter?

Yeah, will make the interface simpler which is always a good thing.

danielnelson · 2017-11-28T23:22:11Z

Merged for 1.5, thanks!

ildarsv · 2017-11-29T05:24:12Z

Thank you!
#3390 can be closed now

Add Bond Interface input plugin

18bc685

ildarsv mentioned this pull request Nov 3, 2017

Bond Interface monitoring #3390

Closed

Merge branch 'master' of https://github.com/influxdata/telegraf into …

d3d7222

…add-bond-input-plugin

danielnelson reviewed Nov 8, 2017

View reviewed changes

Made some fixes and improvements

a943169

- align README.md - made host_proc configurable, add loading from env variables - removed log levels from error messages - used bufio.Scanner instead of strings.Split - removed cache of bond interfaces - gathered active slave for active-backuo mode

danielnelson reviewed Nov 28, 2017

View reviewed changes

ildarsv added 2 commits November 29, 2017 01:06

Hardcode 'bonding' directory relative path

435d1f3

Align indentions

925f2d3

danielnelson added this to the 1.5.0 milestone Nov 28, 2017

danielnelson merged commit 132fb50 into influxdata:master Nov 28, 2017

ildarsv deleted the add-bond-input-plugin branch November 29, 2017 05:21

aromeyer pushed a commit to aromeyer/telegraf that referenced this pull request May 19, 2018

Add bond input plugin (influxdata#3424)

a83ed29

MrMaxBuilds pushed a commit that referenced this pull request Jun 26, 2018

Add bond input plugin (#3424)

2ccebf2

Conversation

ildarsv commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ildarsv commented Nov 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielnelson commented Nov 28, 2017

Uh oh!

ildarsv commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ildarsv commented Nov 3, 2017 •

edited

Loading