allow querying sensors via the open interface

closes #2244
closes #1547

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
 

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

plugins/inputs/ipmi_sensor/command.go

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

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
 }

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 {
+	if len(m.path) == 0 {
-		m.runner = CommandRunner{}
+		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 {
+func (m *Ipmi) parse(acc telegraf.Accumulator, server string) error {
-	conn := NewConnection(serv)
+	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,10 +92,14 @@ func (m *Ipmi) gatherServer(serv string, acc telegraf.Accumulator) error {
 		}
 
 		tags := map[string]string{
-			"server": conn.Hostname,
 			"name": transform(vals[0]),
 		}
 
+		// tag the server is we have one
+		if hostname != "" {
+			tags["server"] = hostname
+		}
+
 		fields := make(map[string]interface{})
 		if strings.EqualFold("ok", trim(vals[2])) {
 			fields["status"] = 1
@@ -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
 	})
 }

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 = `
+	err := i.Gather(&acc)
-Ambient Temp     | 20 degrees C      | ok
+
+	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 {
+	args := os.Args
-	out string
+
-	err error
+	// 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,
 	}
-}
+	os.Exit(0)
-
-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)
-
 }