Add Bond Interface input plugin #3424
Conversation
…add-bond-input-plugin
plugins/inputs/bond/bond.go
Outdated
|
|
||
| // default bond directory path | ||
| const ( | ||
| BOND_PATH = "/proc/net/bonding" |
There was a problem hiding this comment.
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.
plugins/inputs/bond/bond.go
Outdated
| 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)) |
There was a problem hiding this comment.
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.
plugins/inputs/bond/bond.go
Outdated
| "bond": bondName, | ||
| } | ||
|
|
||
| lines := strings.Split(rawFile, "\n") |
There was a problem hiding this comment.
Use a bufio.Scanner to avoid EOL encoding issues.
plugins/inputs/bond/bond.go
Outdated
| if len(stats) < 2 { | ||
| continue | ||
| } | ||
| name := strings.ToLower(strings.Replace(strings.TrimSpace(stats[0]), " ", "_", -1)) |
There was a problem hiding this comment.
Why lowercase and replace spaces with underscores?
There was a problem hiding this comment.
It's too excessive, my bad. Removed.
plugins/inputs/bond/bond.go
Outdated
| var slave string | ||
| var status int | ||
|
|
||
| lines := strings.Split(rawFile, "\n") |
plugins/inputs/bond/bond.go
Outdated
| if bond.BondPath == "" { | ||
| bond.BondPath = BOND_PATH | ||
| } | ||
| if len(bond.BondInterfaces) == 0 { |
There was a problem hiding this comment.
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.
| ## By default, telegraf gather stats for all bond interfaces | ||
| ## Setting interfaces will restrict the stats to the specified | ||
| ## bond interfaces. | ||
| bond_interfaces = ["bond0", "bond1"] |
There was a problem hiding this comment.
Comment this line out with a single #, so the default will be used.
plugins/inputs/bond/README.md
Outdated
| [[inputs.bond]] | ||
| ## Sets bonding directory path | ||
| ## If not specified, then default is: | ||
| bond_path = "/proc/net/bonding" |
There was a problem hiding this comment.
Comment out since this is default.
| Down Delay (ms): 0 | ||
|
|
||
| Slave Interface: eth3 | ||
| MII Status: down |
There was a problem hiding this comment.
It's also possible for both of the interfaces to be up, right? Maybe we should gather the active slave on the bond master?
There was a problem hiding this comment.
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
|
Hello @danielnelson ! Thank you for review. I've made some changes and improvements. Please take a look when you have time. |
plugins/inputs/bond/bond.go
Outdated
| var sampleConfig = ` | ||
| ## Sets 'proc' directory path | ||
| ## If not specified, then default is /proc | ||
| # host_proc = "/proc" |
plugins/inputs/bond/README.md
Outdated
|
|
||
| ## Sets 'bonding' directory relative path | ||
| ## If not specified, then default is /net/bonding | ||
| # bond_path = "/net/bonding" |
There was a problem hiding this comment.
Would this ever not be "/net/bonding"?
There was a problem hiding this comment.
Seems no, would you like me to hardcode this parameter?
There was a problem hiding this comment.
Yeah, will make the interface simpler which is always a good thing.
|
Merged for 1.5, thanks! |
|
Thank you! |
Required for all PRs: