Fix mount points being collected multiple times in filesystem_linux - #3376
Conversation
c347b31 to
83efcfe
Compare
|
@SuperQ do you have an opinion on this? Do you think this minimal fix is a good approach or would you like a more obvious/clearer implementation here? |
|
@SuperQ @discordianfish Just pinging for an update :) Are you interested in this change? I'm pretty sure this issue still exists. @aequitas are you still seeing these errors in your log messages? Is it possible for you to test if this change fixes the issue for you? |
|
@0xf09f95b4 I'll try to test this change this week and let you know if it fixes things |
|
This needs a rebase after #3387. Yea, this is tricky to fix cleanly as we're overloading the "labels" with non-label filesystem information. |
Signed-off-by: Markus Sütter <markus.suetter@secunet.com>
83efcfe to
de310e3
Compare
|
Thanks for your feedback! I just tested 1.10.0 and I can still see the issue happening. I rebased the commit. I had to modify the patch to now zero our both mount option strings. I also checked and as far as I can see, those options are still not used anywhere after the changed line. |
SuperQ
left a comment
SuperQ
left a comment
There was a problem hiding this comment.
Thanks. This seems like a reasonable workaround for now. Fixing this cleanly would probably require some serious thought / refactoring of the whole filesystem collector flow.
…rometheus#3376) Signed-off-by: Markus Sütter <markus.suetter@secunet.com> Signed-off-by: Shashwat Hiregoudar <shashwathiregoudar@gmail.com>
* [BUGFIX] filesystem: Fix mount points being collected multiple times prometheus#3376 * [BUGFIX] filesystem: Refactor mountinfo parsing prometheus#3452 * [BUGFIX] meminfo: Add Zswap/Zswapped metrics prometheus#3453 Signed-off-by: Ben Kochie <superq@gmail.com> Signed-off-by: Shashwat Hiregoudar <shashwathiregoudar@gmail.com>
The mount table deduplication keys on the whole filesystemLabels struct, but no metric carries all of those fields. Two entries that differ only in a field no metric exposes are treated as distinct and then emit byte-identical series, which client_golang rejects. That fails the whole scrape rather than just dropping the repeated filesystem. filesystemLabels is wider than either emitted label set: every metric except node_filesystem_mount_info carries device, mountpoint, fstype and device_error, while mount_info carries device, major, minor and mountpoint. Key each set on the labels it actually emits so an entry is skipped per metric set instead of as a whole. Blanking the mount options in prometheus#3376 removed two of the extra fields, but major and minor are still part of the key and still absent from every metric but mount_info, so the bug remains reachable. A multihomed NFS export reaches it: one mountinfo line per server address, same device, mount point and fstype, and a superblock of its own per address. That matches the reports, which all fail with exactly seven duplicated metrics and never mention mount_info. Had the entries differed only in their options the device numbers would match too and mount_info would collide as well, giving eight. Emission moves into collectStats because Update calls the platform-specific GetStats and offered no way to feed in a mount table. The filesystem collector is disabled in the end-to-end tests, so no golden output changes. Fixes prometheus#2514 Signed-off-by: 霏承 <huangleshu.hls@alibaba-inc.com>
3 participants
Hi,
this PR attempts to address #2805.
The issue we encountered is a case in which a path is bind-mounted to itself, the same issue you can see in this latest comment.
NixOS bind-mounts the
/nix/storepath to/nix/storeto remount the same mount read-only and (nosuid, nodev), as you can see here. If the/nix/storepath is already a mount from some device, this will lead to metrics being collected twice for the same (mountPoint, device) combination if the mount options differ.This issue only occurs, when the filesystem options of the two mounts are different. The core issue is, that the existing filter that wants to prevent this issue here takes the entire
labelsstruct into account, including mount options.The mount options themselves are not used or exposed by node_exporter. So this minimal fix only removes the filesystem options after they are used to extract the
rogauge. This way, theseen := map[filesystemLabels]bool{}filter works correctly (as the options are always"").This has a side-effect: Previously, the
rogauge was exported twice in this case, if the two mounts were read only and read-write (oncero, oncerw). Now, the gauge is exported only once and in the example, it would be exportedrw, even though the mount actually used by the kernel (the upper mount), isro.Additional options we came up with to fix this:
optionsfromfileSystemLabelsalltogether (only used in Linux anyway).rostate.rooption to gauges as a label.@discordianfish You were involved in the original issue. What do you think?