fix(inputs.diskio): Add missing udev properties

plugins/inputs/diskio/README.md

@@ -16,24 +16,20 @@ See the [CONFIGURATION.md][CONFIGURATION.md] for more details.
 ```toml @sample.conf
 # Read metrics about disk IO by device
 [[inputs.diskio]]
-  ## By default, telegraf will gather stats for all devices including
-  ## disk partitions.
-  ## Setting devices will restrict the stats to the specified devices.
-  ## NOTE: Globbing expressions (e.g. asterix) are not supported for
-  ##       disk synonyms like '/dev/disk/by-id'.
-  # devices = ["sda", "sdb", "vd*", "/dev/disk/by-id/nvme-eui.00123deadc0de123"]
-  ## Uncomment the following line if you need disk serial numbers.
-  # skip_serial_number = false
-  #
-  ## On systems which support it, device metadata can be added in the form of
-  ## tags.
-  ## Currently only Linux is supported via udev properties. You can view
-  ## available properties for a device by running:
-  ## 'udevadm info -q property -n /dev/sda'
+  ## Devices to collect stats for
+  ## Wildcards are supported except for disk synonyms like '/dev/disk/by-id'.
+  ## ex. devices = ["sda", "sdb", "vd*", "/dev/disk/by-id/nvme-eui.00123deadc0de123"]
+  # devices = ["*"]
+
+  ## Skip gathering of the disk's serial numbers.
+  # skip_serial_number = true
+
+  ## Device metadata tags to add on systems supporting it (Linux only)
+  ## Use 'udevadm info -q property -n <device>' to get a list of properties.
   ## Note: Most, but not all, udev properties can be accessed this way. Properties
   ## that are currently inaccessible include DEVTYPE, DEVNAME, and DEVPATH.
   # device_tags = ["ID_FS_TYPE", "ID_FS_USAGE"]
-  #
+
   ## Using the same metadata source as device_tags, you can also customize the
   ## name of the device via templates.
   ## The 'name_templates' parameter is a list of templates to try and apply to

plugins/inputs/diskio/diskio.go

@@ -25,6 +25,18 @@ func hasMeta(s string) bool {
 	return strings.ContainsAny(s, "*?[")
 }
 
+type DiskIO struct {
+	Devices          []string        `toml:"devices"`
+	DeviceTags       []string        `toml:"device_tags"`
+	NameTemplates    []string        `toml:"name_templates"`
+	SkipSerialNumber bool            `toml:"skip_serial_number"`
+	Log              telegraf.Logger `toml:"-"`
+
+	ps           system.PS
+	infoCache    map[string]diskInfoCache
+	deviceFilter filter.Filter
+}
+
 func (*DiskIO) SampleConfig() string {
 	return sampleConfig
 }
@@ -39,6 +51,9 @@ func (d *DiskIO) Init() error {
 			d.deviceFilter = deviceFilter
 		}
 	}
+
+	d.infoCache = make(map[string]diskInfoCache)
+
 	return nil
 }
 

plugins/inputs/diskio/diskio_linux.go

@@ -11,51 +11,30 @@ import (
 	"strings"
 
 	"golang.org/x/sys/unix"
-
-	"github.com/influxdata/telegraf"
-	"github.com/influxdata/telegraf/filter"
-	"github.com/influxdata/telegraf/plugins/inputs/system"
 )
 
-type DiskIO struct {
-	ps system.PS
-
-	Devices          []string
-	DeviceTags       []string
-	NameTemplates    []string
-	SkipSerialNumber bool
-
-	Log telegraf.Logger
-
-	infoCache    map[string]diskInfoCache
-	deviceFilter filter.Filter
-}
-
 type diskInfoCache struct {
 	modifiedAt   int64 // Unix Nano timestamp of the last modification of the device. This value is used to invalidate the cache
 	udevDataPath string
+	sysBlockPath string
 	values       map[string]string
 }
 
 func (d *DiskIO) diskInfo(devName string) (map[string]string, error) {
-	var err error
-	var stat unix.Stat_t
-
+	// Check if the device exists
 	path := "/dev/" + devName
-	err = unix.Stat(path, &stat)
-	if err != nil {
+	var stat unix.Stat_t
+	if err := unix.Stat(path, &stat); err != nil {
 		return nil, err
 	}
 
-	if d.infoCache == nil {
-		d.infoCache = map[string]diskInfoCache{}
-	}
+	// Check if we already got a cached and valid entry
 	ic, ok := d.infoCache[devName]
-
 	if ok && stat.Mtim.Nano() == ic.modifiedAt {
 		return ic.values, nil
 	}
 
+	// Determine udev properties
 	var udevDataPath string
 	if ok && len(ic.udevDataPath) > 0 {
 		// We can reuse the udev data path from a "previous" entry.
@@ -65,33 +44,60 @@ func (d *DiskIO) diskInfo(devName string) (map[string]string, error) {
 		major := unix.Major(uint64(stat.Rdev)) //nolint:unconvert // Conversion needed for some architectures
 		minor := unix.Minor(uint64(stat.Rdev)) //nolint:unconvert // Conversion needed for some architectures
 		udevDataPath = fmt.Sprintf("/run/udev/data/b%d:%d", major, minor)
-
-		_, err := os.Stat(udevDataPath)
-		if err != nil {
+		if _, err := os.Stat(udevDataPath); err != nil {
 			// This path failed, try the fallback .udev style (non-systemd)
 			udevDataPath = "/dev/.udev/db/block:" + devName
-			_, err := os.Stat(udevDataPath)
-			if err != nil {
+			if _, err := os.Stat(udevDataPath); err != nil {
 				// Giving up, cannot retrieve disk info
 				return nil, err
 			}
 		}
 	}
-	// Final open of the confirmed (or the previously detected/used) udev file
-	f, err := os.Open(udevDataPath)
+
+	info, err := readUdevData(udevDataPath)
 	if err != nil {
 		return nil, err
 	}
-	defer f.Close()
 
-	di := map[string]string{}
+	// Read additional device properties
+	var sysBlockPath string
+	if ok && len(ic.sysBlockPath) > 0 {
+		// We can reuse the /sys block path from a "previous" entry.
+		// This allows us to also "poison" it during test scenarios
+		sysBlockPath = ic.sysBlockPath
+	} else {
+		sysBlockPath = "/sys/block/" + devName
+		if _, err := os.Stat(sysBlockPath); err != nil {
+			// Giving up, cannot retrieve additional info
+			return nil, err
+		}
+	}
+	devInfo, err := readDevData(sysBlockPath)
+	if err != nil {
+		return nil, err
+	}
+	for k, v := range devInfo {
+		info[k] = v
+	}
 
 	d.infoCache[devName] = diskInfoCache{
 		modifiedAt:   stat.Mtim.Nano(),
 		udevDataPath: udevDataPath,
-		values:       di,
+		values:       info,
 	}
 
+	return info, nil
+}
+
+func readUdevData(path string) (map[string]string, error) {
+	// Final open of the confirmed (or the previously detected/used) udev file
+	f, err := os.Open(path)
+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()
+
+	info := make(map[string]string)
 	scnr := bufio.NewScanner(f)
 	var devlinks bytes.Buffer
 	for scnr.Scan() {
@@ -114,22 +120,59 @@ func (d *DiskIO) diskInfo(devName string) (map[string]string, error) {
 		if len(kv) < 2 {
 			continue
 		}
-		di[kv[0]] = kv[1]
+		info[kv[0]] = kv[1]
 	}
 
 	if devlinks.Len() > 0 {
-		di["DEVLINKS"] = devlinks.String()
+		info["DEVLINKS"] = devlinks.String()
+	}
+
+	return info, nil
+}
+
+func readDevData(path string) (map[string]string, error) {
+	// Open the file and read line-wise
+	f, err := os.Open(filepath.Join(path, "uevent"))
+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()
+
+	// Read DEVNAME and DEVTYPE
+	info := make(map[string]string)
+	scanner := bufio.NewScanner(f)
+	for scanner.Scan() {
+		line := scanner.Text()
+		if !strings.HasPrefix(line, "DEV") {
+			continue
+		}
+
+		k, v, found := strings.Cut(line, "=")
+		if !found {
+			continue
+		}
+		info[strings.TrimSpace(k)] = strings.TrimSpace(v)
+	}
+	if d, found := info["DEVNAME"]; found && !strings.HasPrefix(d, "/dev") {
+		info["DEVNAME"] = "/dev/" + d
+	}
+
+	// Find the DEVPATH property
+	if devlnk, err := filepath.EvalSymlinks(filepath.Join(path, "device")); err == nil {
+		devlnk = filepath.Join(devlnk, filepath.Base(path))
+		devlnk = strings.TrimPrefix(devlnk, "/sys")
+		info["DEVPATH"] = devlnk
 	}
 
-	return di, nil
+	return info, nil
 }
 
 func resolveName(name string) string {
 	resolved, err := filepath.EvalSymlinks(name)
 	if err == nil {
 		return resolved
 	}
-	if err != nil && !errors.Is(err, fs.ErrNotExist) {
+	if !errors.Is(err, fs.ErrNotExist) {
 		return name
 	}
 	// Try to prepend "/dev"

plugins/inputs/diskio/diskio_linux_test.go

@@ -3,72 +3,29 @@
 package diskio
 
 import (
-	"os"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 )
 
-var nullDiskInfo = []byte(`
-E:MY_PARAM_1=myval1
-E:MY_PARAM_2=myval2
-S:foo/bar/devlink
-S:foo/bar/devlink1
-`)
-
-// setupNullDisk sets up fake udev info as if /dev/null were a disk.
-func setupNullDisk(t *testing.T, s *DiskIO, devName string) func() {
-	td, err := os.CreateTemp("", ".telegraf.DiskInfoTest")
-	require.NoError(t, err)
-
-	if s.infoCache == nil {
-		s.infoCache = make(map[string]diskInfoCache)
-	}
-	ic, ok := s.infoCache[devName]
-	if !ok {
-		// No previous calls for the device were done, easy to poison the cache
-		s.infoCache[devName] = diskInfoCache{
-			modifiedAt:   0,
-			udevDataPath: td.Name(),
-			values:       map[string]string{},
-		}
-	}
-	origUdevPath := ic.udevDataPath
-
-	cleanFunc := func() {
-		ic.udevDataPath = origUdevPath
-		os.Remove(td.Name())
-	}
-
-	ic.udevDataPath = td.Name()
-	_, err = td.Write(nullDiskInfo)
-	if err != nil {
-		cleanFunc()
-		t.Fatal(err)
-	}
-
-	return cleanFunc
-}
-
 func TestDiskInfo(t *testing.T) {
-	s := &DiskIO{}
-	clean := setupNullDisk(t, s, "null")
-	defer clean()
-	di, err := s.diskInfo("null")
-	require.NoError(t, err)
-	require.Equal(t, "myval1", di["MY_PARAM_1"])
-	require.Equal(t, "myval2", di["MY_PARAM_2"])
-	require.Equal(t, "/dev/foo/bar/devlink /dev/foo/bar/devlink1", di["DEVLINKS"])
-
-	// test that data is cached
-	clean()
+	plugin := &DiskIO{
+		infoCache: map[string]diskInfoCache{
+			"null": {
+				modifiedAt:   0,
+				udevDataPath: "testdata/udev.txt",
+				sysBlockPath: "testdata",
+				values:       map[string]string{},
+			},
+		},
+	}
 
-	di, err = s.diskInfo("null")
+	di, err := plugin.diskInfo("null")
 	require.NoError(t, err)
 	require.Equal(t, "myval1", di["MY_PARAM_1"])
 	require.Equal(t, "myval2", di["MY_PARAM_2"])
 	require.Equal(t, "/dev/foo/bar/devlink /dev/foo/bar/devlink1", di["DEVLINKS"])
-	// unfortunately we can't adjust mtime on /dev/null to test cache invalidation
 }
 
 // DiskIOStats.diskName isn't a linux specific function, but dependent
@@ -89,25 +46,39 @@ func TestDiskIOStats_diskName(t *testing.T) {
 		{[]string{"$MY_PARAM_2/$MISSING"}, "null"},
 	}
 
-	for _, tc := range tests {
-		func() {
-			s := DiskIO{
+	for i, tc := range tests {
+		t.Run(fmt.Sprintf("template %d", i), func(t *testing.T) {
+			plugin := DiskIO{
 				NameTemplates: tc.templates,
+				infoCache: map[string]diskInfoCache{
+					"null": {
+						modifiedAt:   0,
+						udevDataPath: "testdata/udev.txt",
+						sysBlockPath: "testdata",
+						values:       map[string]string{},
+					},
+				},
 			}
-			defer setupNullDisk(t, &s, "null")() //nolint:revive // done on purpose, cleaning will be executed properly
-			name, _ := s.diskName("null")
+			name, _ := plugin.diskName("null")
 			require.Equal(t, tc.expected, name, "Templates: %#v", tc.templates)
-		}()
+		})
 	}
 }
 
 // DiskIOStats.diskTags isn't a linux specific function, but dependent
 // functions are a no-op on non-Linux.
 func TestDiskIOStats_diskTags(t *testing.T) {
-	s := &DiskIO{
+	plugin := &DiskIO{
 		DeviceTags: []string{"MY_PARAM_2"},
+		infoCache: map[string]diskInfoCache{
+			"null": {
+				modifiedAt:   0,
+				udevDataPath: "testdata/udev.txt",
+				sysBlockPath: "testdata",
+				values:       map[string]string{},
+			},
+		},
 	}
-	defer setupNullDisk(t, s, "null")() //nolint:revive // done on purpose, cleaning will be executed properly
-	dt := s.diskTags("null")
+	dt := plugin.diskTags("null")
 	require.Equal(t, map[string]string{"MY_PARAM_2": "myval2"}, dt)
 }