enhancement - inputs/system/diskio_linux - configurable udev data path

[ryanbreed]

from the nightly commit 76ce71f, the linux diskio plugin collects its info cache from a hard-coded path:

telegraf/plugins/inputs/system/diskio_linux.go

Line 17 in 76ce71f

var udevPath = "/run/udev/data"

this seems to vary by distribution - on amzn-linux 1 at least, this is available in /dev/.udev/data, and cannot be configured via udevd. Here's the base AMI I'm using:

image_name="amzn-ami-hvm"
image_version="2017.09"
image_arch="x86_64"
image_file="amzn-ami-hvm-2017.09.1.20171120-x86_64.ext4.gpt"
image_stamp="03dc-49fe"
image_date="20171120221247"
recipe_name="amzn ami"
recipe_id="c9c871d2-7d71-8baa-e873-c70f-cbbe-ddb2-79ee86e7"

[ryanbreed]

@danielnelson - although i'm new to go, I think I could throw a pr your way if you'd let me know what the expectations would be on the testing side. I think that the current implementation fails gracefully enough since ENOENT/EACCESS/etc get propagated already (obvs), and don't cause any uncontained instability as far as I can tell.

I think it's a one-line addition to the DiskIO Type to relocate the module-scoped var into the config if I'm reading things right. The rest would be updating the info cache population to pull from the config type instead of the module-scoped var.

I'd be pleased as punch to squish this one unless other plans are in the mix.

[danielnelson]

I don't think we need to worry that much about testing right now as that would require a fair bit of refactoring to do right and this is a pretty low risk change. What you are proposing will work fine, just make sure that if the new variable udev_data_path is unspecified that it defaults correctly and also update the docs and sample config. Thanks!

[red15]

Could we build a hardcoded fallback in that uses the /dev/.udev/db/block:sda1 equivalent on older systems that don't have the /run/udev/* path mounted ?

Or is the overhead of running twice the amount of stat/read calls for those older systems prohibitive for implementing it this way?

[danielnelson]

I think the fallback idea would work too, it won't cost anything for those on platforms where the initial path works and it wouldn't be very expensive in the alternative case.

[red15]

I've had a better idea, can't we just instead rely on the udevadm tool to query the data for us ? It does require that this tool is present and (hopefully) didn't change it's syntax our output style across versions but it would prevent us from having to learn all the different ways udev has changed over the years and allow us to re-use their "public api" which can do all the difficulty of dealing with different versions in the back.

Reference:

udevadm info -q property -n sda
udevadm info -q property -p /sys/block/sda

[danielnelson]

The performance is quite a bit worse running udevadm so it would preferred to continue to use the file.

[ryanbreed]

i've obviously lost my emotional stake in this bug (wound up switching OS for unrelated reasons)

[vmj]

Quite a few Telegraf input plugins are based on gopsutil, which has this pattern of using HOST_PROC, HOST_RUN, HOST_VAR environment variables for defining where directories are located.

Would it make sense to copy that logic for this, too?

[danielnelson]

In this case its not a problem of adding a prefix path, the file has just completely changed locations, so I don't think we want to use the environment variables here. The #5180 pr is on the right track just needs a few tweaks to be ready.

[red15]

@danielnelson I've got this new PR that should be a lot cleaner. I'm leaving the older one open for now so you can have your pick basically.