From c8de4833e3c524bfe932d0ba600f6d8868f50b98 Mon Sep 17 00:00:00 2001 From: Matteo Cerutti Date: Mon, 9 Jan 2017 10:45:31 +0000 Subject: [PATCH] allow querying sensors via the open interface closes #2244 closes #1547 --- CHANGELOG.md | 1 + plugins/inputs/ipmi_sensor/README.md | 38 ++- plugins/inputs/ipmi_sensor/command.go | 35 --- plugins/inputs/ipmi_sensor/connection.go | 1 - plugins/inputs/ipmi_sensor/ipmi.go | 80 +++-- plugins/inputs/ipmi_sensor/ipmi_test.go | 354 +++++++++++++++-------- 6 files changed, 315 insertions(+), 194 deletions(-) delete mode 100644 plugins/inputs/ipmi_sensor/command.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 87748d2bc..3696cdb8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ It is highly recommended that all users migrate to the new riemann output plugin - [#1453](https://github.com/influxdata/telegraf/pull/1453): diskio: add support for name templates and udev tags. - [#2277](https://github.com/influxdata/telegraf/pull/2277): add integer metrics for Consul check health state. - [#2201](https://github.com/influxdata/telegraf/pull/2201): Add lock option to the IPtables input plugin. +- [#2244](https://github.com/influxdata/telegraf/pull/2244): Support ipmi_sensor plugin querying local ipmi sensors. ### Bugfixes diff --git a/plugins/inputs/ipmi_sensor/README.md b/plugins/inputs/ipmi_sensor/README.md index 4a248fdc9..3a75d0c65 100644 --- a/plugins/inputs/ipmi_sensor/README.md +++ b/plugins/inputs/ipmi_sensor/README.md @@ -4,33 +4,50 @@ Get bare metal metrics using the command line utility `ipmitool` see ipmitool(https://sourceforge.net/projects/ipmitool/files/ipmitool/) -The plugin will use the following command to collect remote host sensor stats: +If no servers are specified, the plugin will query the local machine sensor stats via the following command: -ipmitool -I lan -H 192.168.1.1 -U USERID -P PASSW0RD sdr +``` +ipmitool sdr +``` + +When one or more servers are specified, the plugin will use the following command to collect remote host sensor stats: + +``` +ipmitool -I lan -H SERVER -U USERID -P PASSW0RD sdr +``` ## Measurements - ipmi_sensor: - * Tags: `name`, `server`, `unit` + * Tags: `name`, `unit` * Fields: - status - value +The `server` tag will be made available when retrieving stats from remote server(s). + ## Configuration ```toml +# Read metrics from the bare metal servers via IPMI [[inputs.ipmi_sensor]] - ## specify servers via a url matching: + ## optionally specify the path to the ipmitool executable + # path = "/usr/bin/ipmitool" + # + ## optionally specify one or more servers via a url matching ## [username[:password]@][protocol[(address)]] ## e.g. ## root:passwd@lan(127.0.0.1) ## - servers = ["USERID:PASSW0RD@lan(10.20.2.203)"] + ## if no servers are specified, local machine sensor stats will be queried + ## + # servers = ["USERID:PASSW0RD@lan(192.168.1.1)"] ``` ## Output +When retrieving stats from a remote server: ``` > ipmi_sensor,server=10.20.2.203,unit=degrees_c,name=ambient_temp status=1i,value=20 1458488465012559455 > ipmi_sensor,server=10.20.2.203,unit=feet,name=altitude status=1i,value=80 1458488465012688613 @@ -40,3 +57,14 @@ ipmitool -I lan -H 192.168.1.1 -U USERID -P PASSW0RD sdr > ipmi_sensor,server=10.20.2.203,unit=rpm,name=fan_1a_tach status=1i,value=2610 1458488465013137932 > ipmi_sensor,server=10.20.2.203,unit=rpm,name=fan_1b_tach status=1i,value=1775 1458488465013279896 ``` + +When retrieving stats from the local machine (no server specified): +``` +> ipmi_sensor,unit=degrees_c,name=ambient_temp status=1i,value=20 1458488465012559455 +> ipmi_sensor,unit=feet,name=altitude status=1i,value=80 1458488465012688613 +> ipmi_sensor,unit=watts,name=avg_power status=1i,value=220 1458488465012776511 +> ipmi_sensor,unit=volts,name=planar_3.3v status=1i,value=3.28 1458488465012861875 +> ipmi_sensor,unit=volts,name=planar_vbat status=1i,value=3.04 1458488465013072508 +> ipmi_sensor,unit=rpm,name=fan_1a_tach status=1i,value=2610 1458488465013137932 +> ipmi_sensor,unit=rpm,name=fan_1b_tach status=1i,value=1775 1458488465013279896 +``` diff --git a/plugins/inputs/ipmi_sensor/command.go b/plugins/inputs/ipmi_sensor/command.go deleted file mode 100644 index 76374c494..000000000 --- a/plugins/inputs/ipmi_sensor/command.go +++ /dev/null @@ -1,35 +0,0 @@ -package ipmi_sensor - -import ( - "fmt" - "os/exec" - "strings" - "time" - - "github.com/influxdata/telegraf/internal" -) - -type CommandRunner struct{} - -func (t CommandRunner) cmd(conn *Connection, args ...string) *exec.Cmd { - path := conn.Path - opts := append(conn.options(), args...) - - if path == "" { - path = "ipmitool" - } - - return exec.Command(path, opts...) -} - -func (t CommandRunner) Run(conn *Connection, args ...string) (string, error) { - cmd := t.cmd(conn, args...) - - output, err := internal.CombinedOutputTimeout(cmd, time.Second*5) - if err != nil { - return "", fmt.Errorf("run %s %s: %s (%s)", - cmd.Path, strings.Join(cmd.Args, " "), string(output), err) - } - - return string(output), err -} diff --git a/plugins/inputs/ipmi_sensor/connection.go b/plugins/inputs/ipmi_sensor/connection.go index 1e9bfbdcb..432b4aa02 100644 --- a/plugins/inputs/ipmi_sensor/connection.go +++ b/plugins/inputs/ipmi_sensor/connection.go @@ -12,7 +12,6 @@ type Connection struct { Hostname string Username string Password string - Path string Port int Interface string } diff --git a/plugins/inputs/ipmi_sensor/ipmi.go b/plugins/inputs/ipmi_sensor/ipmi.go index aec56a0e4..b2389a675 100644 --- a/plugins/inputs/ipmi_sensor/ipmi.go +++ b/plugins/inputs/ipmi_sensor/ipmi.go @@ -1,48 +1,62 @@ package ipmi_sensor import ( + "fmt" + "os/exec" "strconv" "strings" "time" "github.com/influxdata/telegraf" + "github.com/influxdata/telegraf/internal" "github.com/influxdata/telegraf/plugins/inputs" ) +var ( + execCommand = exec.Command // execCommand is used to mock commands in tests. +) + type Ipmi struct { + path string Servers []string - runner Runner } var sampleConfig = ` - ## specify servers via a url matching: + ## optionally specify the path to the ipmitool executable + # path = "/usr/bin/ipmitool" + # + ## optionally specify one or more servers via a url matching ## [username[:password]@][protocol[(address)]] ## e.g. ## root:passwd@lan(127.0.0.1) ## - servers = ["USERID:PASSW0RD@lan(192.168.1.1)"] + ## if no servers are specified, local machine sensor stats will be queried + ## + # servers = ["USERID:PASSW0RD@lan(192.168.1.1)"] ` -func NewIpmi() *Ipmi { - return &Ipmi{ - runner: CommandRunner{}, - } -} - func (m *Ipmi) SampleConfig() string { return sampleConfig } func (m *Ipmi) Description() string { - return "Read metrics from one or many bare metal servers" + return "Read metrics from the bare metal servers via IPMI" } func (m *Ipmi) Gather(acc telegraf.Accumulator) error { - if m.runner == nil { - m.runner = CommandRunner{} + if len(m.path) == 0 { + return fmt.Errorf("ipmitool not found: verify that ipmitool is installed and that ipmitool is in your PATH") } - for _, serv := range m.Servers { - err := m.gatherServer(serv, acc) + + if len(m.Servers) > 0 { + for _, server := range m.Servers { + err := m.parse(acc, server) + if err != nil { + return err + } + } + } else { + err := m.parse(acc, "") if err != nil { return err } @@ -51,17 +65,26 @@ func (m *Ipmi) Gather(acc telegraf.Accumulator) error { return nil } -func (m *Ipmi) gatherServer(serv string, acc telegraf.Accumulator) error { - conn := NewConnection(serv) +func (m *Ipmi) parse(acc telegraf.Accumulator, server string) error { + opts := make([]string, 0) + hostname := "" - res, err := m.runner.Run(conn, "sdr") + if server != "" { + conn := NewConnection(server) + hostname = conn.Hostname + opts = conn.options() + } + + opts = append(opts, "sdr") + cmd := execCommand(m.path, opts...) + out, err := internal.CombinedOutputTimeout(cmd, time.Second*5) if err != nil { - return err + return fmt.Errorf("failed to run command %s: %s - %s", strings.Join(cmd.Args, " "), err, string(out)) } // each line will look something like // Planar VBAT | 3.05 Volts | ok - lines := strings.Split(res, "\n") + lines := strings.Split(string(out), "\n") for i := 0; i < len(lines); i++ { vals := strings.Split(lines[i], "|") if len(vals) != 3 { @@ -69,8 +92,12 @@ func (m *Ipmi) gatherServer(serv string, acc telegraf.Accumulator) error { } tags := map[string]string{ - "server": conn.Hostname, - "name": transform(vals[0]), + "name": transform(vals[0]), + } + + // tag the server is we have one + if hostname != "" { + tags["server"] = hostname } fields := make(map[string]interface{}) @@ -99,10 +126,6 @@ func (m *Ipmi) gatherServer(serv string, acc telegraf.Accumulator) error { return nil } -type Runner interface { - Run(conn *Connection, args ...string) (string, error) -} - func Atofloat(val string) float64 { f, err := strconv.ParseFloat(val, 64) if err != nil { @@ -123,7 +146,12 @@ func transform(s string) string { } func init() { + m := Ipmi{} + path, _ := exec.LookPath("ipmitool") + if len(path) > 0 { + m.path = path + } inputs.Add("ipmi_sensor", func() telegraf.Input { - return &Ipmi{} + return &m }) } diff --git a/plugins/inputs/ipmi_sensor/ipmi_test.go b/plugins/inputs/ipmi_sensor/ipmi_test.go index c62447e39..94dc066c8 100644 --- a/plugins/inputs/ipmi_sensor/ipmi_test.go +++ b/plugins/inputs/ipmi_sensor/ipmi_test.go @@ -1,6 +1,9 @@ package ipmi_sensor import ( + "fmt" + "os" + "os/exec" "testing" "github.com/influxdata/telegraf/testutil" @@ -8,10 +11,219 @@ import ( "github.com/stretchr/testify/require" ) -const serv = "USERID:PASSW0RD@lan(192.168.1.1)" +func TestGather(t *testing.T) { + i := &Ipmi{ + Servers: []string{"USERID:PASSW0RD@lan(192.168.1.1)"}, + path: "ipmitool", + } + // overwriting exec commands with mock commands + execCommand = fakeExecCommand + var acc testutil.Accumulator -const cmdReturn = ` -Ambient Temp | 20 degrees C | ok + err := i.Gather(&acc) + + require.NoError(t, err) + + assert.Equal(t, acc.NFields(), 266, "non-numeric measurements should be ignored") + + conn := NewConnection(i.Servers[0]) + assert.Equal(t, "USERID", conn.Username) + assert.Equal(t, "lan", conn.Interface) + + var testsWithServer = []struct { + fields map[string]interface{} + tags map[string]string + }{ + { + map[string]interface{}{ + "value": float64(20), + "status": int(1), + }, + map[string]string{ + "name": "ambient_temp", + "server": "192.168.1.1", + "unit": "degrees_c", + }, + }, + { + map[string]interface{}{ + "value": float64(80), + "status": int(1), + }, + map[string]string{ + "name": "altitude", + "server": "192.168.1.1", + "unit": "feet", + }, + }, + { + map[string]interface{}{ + "value": float64(210), + "status": int(1), + }, + map[string]string{ + "name": "avg_power", + "server": "192.168.1.1", + "unit": "watts", + }, + }, + { + map[string]interface{}{ + "value": float64(4.9), + "status": int(1), + }, + map[string]string{ + "name": "planar_5v", + "server": "192.168.1.1", + "unit": "volts", + }, + }, + { + map[string]interface{}{ + "value": float64(3.05), + "status": int(1), + }, + map[string]string{ + "name": "planar_vbat", + "server": "192.168.1.1", + "unit": "volts", + }, + }, + { + map[string]interface{}{ + "value": float64(2610), + "status": int(1), + }, + map[string]string{ + "name": "fan_1a_tach", + "server": "192.168.1.1", + "unit": "rpm", + }, + }, + { + map[string]interface{}{ + "value": float64(1775), + "status": int(1), + }, + map[string]string{ + "name": "fan_1b_tach", + "server": "192.168.1.1", + "unit": "rpm", + }, + }, + } + + for _, test := range testsWithServer { + acc.AssertContainsTaggedFields(t, "ipmi_sensor", test.fields, test.tags) + } + + i = &Ipmi{ + path: "ipmitool", + } + + err = i.Gather(&acc) + + var testsWithoutServer = []struct { + fields map[string]interface{} + tags map[string]string + }{ + { + map[string]interface{}{ + "value": float64(20), + "status": int(1), + }, + map[string]string{ + "name": "ambient_temp", + "unit": "degrees_c", + }, + }, + { + map[string]interface{}{ + "value": float64(80), + "status": int(1), + }, + map[string]string{ + "name": "altitude", + "unit": "feet", + }, + }, + { + map[string]interface{}{ + "value": float64(210), + "status": int(1), + }, + map[string]string{ + "name": "avg_power", + "unit": "watts", + }, + }, + { + map[string]interface{}{ + "value": float64(4.9), + "status": int(1), + }, + map[string]string{ + "name": "planar_5v", + "unit": "volts", + }, + }, + { + map[string]interface{}{ + "value": float64(3.05), + "status": int(1), + }, + map[string]string{ + "name": "planar_vbat", + "unit": "volts", + }, + }, + { + map[string]interface{}{ + "value": float64(2610), + "status": int(1), + }, + map[string]string{ + "name": "fan_1a_tach", + "unit": "rpm", + }, + }, + { + map[string]interface{}{ + "value": float64(1775), + "status": int(1), + }, + map[string]string{ + "name": "fan_1b_tach", + "unit": "rpm", + }, + }, + } + + for _, test := range testsWithoutServer { + acc.AssertContainsTaggedFields(t, "ipmi_sensor", test.fields, test.tags) + } +} + +// fackeExecCommand is a helper function that mock +// the exec.Command call (and call the test binary) +func fakeExecCommand(command string, args ...string) *exec.Cmd { + cs := []string{"-test.run=TestHelperProcess", "--", command} + cs = append(cs, args...) + cmd := exec.Command(os.Args[0], cs...) + cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1"} + return cmd +} + +// TestHelperProcess isn't a real test. It's used to mock exec.Command +// For example, if you run: +// GO_WANT_HELPER_PROCESS=1 go test -test.run=TestHelperProcess -- chrony tracking +// it returns below mockData. +func TestHelperProcess(t *testing.T) { + if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { + return + } + + mockData := `Ambient Temp | 20 degrees C | ok Altitude | 80 feet | ok Avg Power | 210 Watts | ok Planar 3.3V | 3.29 Volts | ok @@ -146,130 +358,18 @@ PCI 5 | 0x00 | ok OS RealTime Mod | 0x00 | ok ` -type runnerMock struct { - out string - err error -} + args := os.Args + + // Previous arguments are tests stuff, that looks like : + // /tmp/go-build970079519/…/_test/integration.test -test.run=TestHelperProcess -- + cmd, args := args[3], args[4:] + + if cmd == "ipmitool" { + fmt.Fprint(os.Stdout, mockData) + } else { + fmt.Fprint(os.Stdout, "command not found") + os.Exit(1) -func newRunnerMock(out string, err error) Runner { - return &runnerMock{ - out: out, - err: err, } -} - -func (r runnerMock) Run(conn *Connection, args ...string) (out string, err error) { - if r.err != nil { - return out, r.err - } - return r.out, nil -} - -func TestIpmi(t *testing.T) { - i := &Ipmi{ - Servers: []string{"USERID:PASSW0RD@lan(192.168.1.1)"}, - runner: newRunnerMock(cmdReturn, nil), - } - - var acc testutil.Accumulator - - err := i.Gather(&acc) - - require.NoError(t, err) - - assert.Equal(t, acc.NFields(), 266, "non-numeric measurements should be ignored") - - var tests = []struct { - fields map[string]interface{} - tags map[string]string - }{ - { - map[string]interface{}{ - "value": float64(20), - "status": int(1), - }, - map[string]string{ - "name": "ambient_temp", - "server": "192.168.1.1", - "unit": "degrees_c", - }, - }, - { - map[string]interface{}{ - "value": float64(80), - "status": int(1), - }, - map[string]string{ - "name": "altitude", - "server": "192.168.1.1", - "unit": "feet", - }, - }, - { - map[string]interface{}{ - "value": float64(210), - "status": int(1), - }, - map[string]string{ - "name": "avg_power", - "server": "192.168.1.1", - "unit": "watts", - }, - }, - { - map[string]interface{}{ - "value": float64(4.9), - "status": int(1), - }, - map[string]string{ - "name": "planar_5v", - "server": "192.168.1.1", - "unit": "volts", - }, - }, - { - map[string]interface{}{ - "value": float64(3.05), - "status": int(1), - }, - map[string]string{ - "name": "planar_vbat", - "server": "192.168.1.1", - "unit": "volts", - }, - }, - { - map[string]interface{}{ - "value": float64(2610), - "status": int(1), - }, - map[string]string{ - "name": "fan_1a_tach", - "server": "192.168.1.1", - "unit": "rpm", - }, - }, - { - map[string]interface{}{ - "value": float64(1775), - "status": int(1), - }, - map[string]string{ - "name": "fan_1b_tach", - "server": "192.168.1.1", - "unit": "rpm", - }, - }, - } - - for _, test := range tests { - acc.AssertContainsTaggedFields(t, "ipmi_sensor", test.fields, test.tags) - } -} - -func TestIpmiConnection(t *testing.T) { - conn := NewConnection(serv) - assert.Equal(t, "USERID", conn.Username) - assert.Equal(t, "lan", conn.Interface) - + os.Exit(0) }