Collectd: df plugin: Devices mounted more than once with "ReportByDevice true" sends duplicate data

[katzj]

When a device is mounted more than once, it's sent multiple times but all of the keys are the same so you can't really distinguish between them. So it's probably worth de-duping before sending.

OpenSuSE has had a patch for this for a whlie (https://build.opensuse.org/package/view_file/openSUSE:Factory/collectd/collectd-df-remove-duplicates.patch?rev=4)

[octo]

Thanks for reporting this @katzj! I don't think the OpenSUSE patch actually _does_ anything, though: the continue statements that are supposed to skip duplicates will only advance mnt_dup_ptr, which would have happened anyway. It should be easy enough to fix the fix, though :)

—octo

[katzj]

Yeah, that patch looked janky to me too ;)

[octo]

How's this? I even tracked down the author of that patch :-D

[ciomaire]

I no longer see metrics for / on systems running systemd, ie with /etc/mtab as a symlink to /proc/self/mounts

with /etc/mtab containing 2 entries for /, the first (rootfs) is ignored on line 275 of df.c, but the second which should be processed is then detected as a duplicate on line 242.

Note: Editing to clarify ambiguous wording...
As a workaround allow for duplicate entries when the mount point is / as the entry with rootfs will be ignored on line 275 - ie something like this...

diff --git a/src/df.c b/src/df.c
index ef9e413..385d52e 100644
--- a/src/df.c
+++ b/src/df.c
@@ -239,7 +239,8 @@ static int df_read (void)

                /* ignore duplicates */
                if (dup_ptr != NULL)
-                       continue;
+                       if (strcmp (mnt_ptr->dir, "/") != 0)
+                               continue;

                if (STATANYFS (mnt_ptr->dir, &statbuf) < 0)
                {

[mfournier]

Thanks for digging into this regression @ciomaire ! We definetely need to fix that ASAP.

I think this is the same issue @davewongillies reported a few hours ago in #739.

[mfournier]

@ciomaire would you mind taking a look at #1511 ? I chose a different approach than what you suggested above. See commit message for the reasoning.

[ciomaire]

@mfournier that patch doesn't when the FSType option is used. Eg when set to ext4 (with IgnoreSelected false).which is the filesystem type for / on my test system. The rootfs entry is skipped because of the ignorelist_match test on line 220 and the '/' is ignored as a duplicate on 242.

[mfournier]

@ciomaire thanks for testing !

I'd argue this is a different problem (eg: "duplicate detection code doesn't play well with ignorelist feature"). But an issue, for sure !

Without looking at the code in details, it seems we should do the duplicate detection on the list of elements after getting filtered through the ignorelist functions. Or move the duplicate detection logic to utils_ignorelist.c.

[mfournier]

For the record, an example of problematic mtab we're talking about here looks like this:

rootfs / rootfs rw 0 0
sysfs /sys sysfs rw,nosuid,nodev,noexec,relatime 0 0
proc /proc proc rw,nosuid,nodev,noexec,relatime 0 0
[...]
/dev/dm-0 / ext4 rw,relatime,errors=remount-ro,data=ordered 0 0

@ciomaire regarding your last comment, at least there now is an easy workaround: add FSType rootfs to the config. Without 377fd55, monitoring / is unconditionnally skipped and there's nothing the user can do about it.