Metricbeat: Remove duplicated filesystems in system module #6819

jsoriano · 2018-04-10T15:46:57Z

Mountpoints whose device name is the same absolute path are considered to be the same and are counted only once. The directory name used is the shorter between them. If the device of a filesystem is a directory, it is considered some kind of bind mount and is ignored. This avoids duplication of filesystems as can happen with bind mounts in Linux (#3384). This mimics the behaviour of df command.

There are still filesystems that can be counted twice if union filesystems are used (as aufs or overlay), but this can be controlled with the filesystem.ignore_types option. ~~Maybe we should blacklist them by default.~~ Update: For that, if the option is left empty, we fill the list of ignored types with all nodev devices in /proc/filesystems where this file exists.

jsoriano · 2018-04-10T15:55:39Z

metricbeat/module/system/filesystem/helper.go

+
+		// If a block device is mounted multiple times (e.g. with bind mounts),
+		// store it only once, and use the shorter mount point path.
+		if seen, found := devices[fs.DevName]; found {


This is not completely accurate, there can be different device paths that are really pointing to the same block device, but I don't believe this is common at all. To control this we should check duplicates by device identifier (Major:Minor) but this is not supported in gosigar yet. Does it worth to implement this?

I would leave it what you have for now but potentially follow up with a change in gosigar to make it possible if it is a case that can happen (more then once :-)). @andrewkroh WDYT?

If it will help make Metricbeat more accurate then I'm +1.

I will take a look to this in gosigar, meanwhile I think we are good enough with the checks based on device name.

jsoriano · 2018-04-10T17:10:47Z

Please don't merge this, I have just seen that gosigar obtains list of filesystem from mtab, what doesn't include the device name for bind mounts.

ruflin

I like the idea of having some defaults for filesystem.ignore_types. Someone could still set it to filesystem.ignore_types: [ ] if we wants all types.

ruflin · 2018-04-10T18:01:54Z

metricbeat/module/system/filesystem/helper.go

+
+		// If a block device is mounted multiple times (e.g. with bind mounts),
+		// store it only once, and use the shorter mount point path.
+		if seen, found := devices[fs.DevName]; found {


I would leave it what you have for now but potentially follow up with a change in gosigar to make it possible if it is a case that can happen (more then once :-)). @andrewkroh WDYT?

ruflin · 2018-04-10T18:02:55Z

metricbeat/module/system/filesystem/helper_test.go

+			},
+		},
+		{
+			description: "Don't repeat devices, sortest of dir names should be used",


nit: shortest, also in all the ones below

jsoriano · 2018-04-10T21:17:54Z

On linux we could maybe ignore by default all types marked as nodev in /proc/filesystems.

ruflin · 2018-04-11T06:08:43Z

@jsoriano I'm ok with excluding nodev type by default. Your comment reminded me that as part of this PR we should extend the docs for filesystem and explain in detail what we include and exclude. This question will pop up and it's good to have a place to link to and look it up. This should also make it easier to revisit the decision later again.

jsoriano · 2018-04-11T11:25:36Z

Two more changes added:

Filesystems whose device name is a directory are not collected, it is assumed to be some kind of bind mount.
If no filesystem.ignore_types is set, it is set to the list of known virtual filesystems in the system, it is by now the list of types marked as nodev in systems where /proc/filesystems is available. Documentation has been updated for this.

jsoriano · 2018-04-11T11:37:26Z

Testing it in a linux machine it looks quite better now with the default settings.

ruflin · 2018-04-11T11:52:45Z

metricbeat/module/system/filesystem/filesystem.go

@@ -32,6 +34,13 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
 		return nil, err
 	}

+	if len(config.IgnoreTypes) == 0 {


The behaviour I would have expected that if a user sets filesystem.ignore_types: [] that then no file systems would be ignore. But checking for 0 indicates that it also applies in this case. Could we instead check if the config is set?

If we go with the above suggestion, the reference config file should be adjusted to contain the two values.

Oh, right, checking nil instead of zero lenght. I have added a comment also in reference config.

andrewkroh · 2018-04-11T13:01:34Z

metricbeat/module/system/filesystem/helper.go

+
+		// If the device name is a directory, this is a bind mount or nullfs,
+		// don't count it as it'd be counting again its parent filesystem.
+		devFileInfo, err := os.Stat(fs.DevName)


One of the original reasons we added ignore_types was to avoid doing a statfs on autofs filesystems. Metricbeat was causing autofs mounts to never be unmounted because its continuous metric gathering was making autofs think the mount was being accessed.

Will this cause the same problem?

Good point.
Here the stat is done against the device name, on #4823 the mentioned issued seemed to appear when doing statfs on the mount point (DirName). So I don't think it can cause the same problem.

andrewkroh

LGTM

andrewkroh · 2018-04-12T14:33:44Z

jenkins, test it

ruflin · 2018-04-13T07:03:39Z

@jsoriano I think the failing windows tests are related to this change.

jsoriano · 2018-04-13T08:00:57Z

Removing filtering on Windows

ruflin · 2018-04-13T14:57:08Z

Something went wrong in the helper:

darwin/beat/metricbeat/label/macosx/src/github.com/elastic/beats/metricbeat/module/system/filesystem/helper.go:41:3: not enough arguments to return
08:29:51 	have ([]gosigar.FileSystem)
08:29:51 	want ([]gosigar.FileSystem, error)

jsoriano · 2018-04-13T15:48:04Z

jenkins, test again please

Mountpoints whose device name is the same absolute path are considered to be the same and are counted only once. The directory name used is the shorter between them. This avoids duplication of filesystems as can happen with bind mounts in Linux (elastic#3384).

In system module, when no filesystem type is set to be ignored, it tries to make a sane guess. In systems with /proc/filesystems file it ignores all devices marked as `nodev`.

jsoriano added module review Metricbeat Metricbeat labels Apr 10, 2018

jsoriano requested review from ruflin and andrewkroh April 10, 2018 15:46

jsoriano commented Apr 10, 2018

View reviewed changes

jsoriano force-pushed the fsstat-filter-bind-mounts branch 3 times, most recently from b304e19 to 9c8173b Compare April 10, 2018 16:22

jsoriano added the in progress Pull request is currently in progress. label Apr 10, 2018

ruflin reviewed Apr 10, 2018

View reviewed changes

jsoriano force-pushed the fsstat-filter-bind-mounts branch from 9c8173b to cf2673f Compare April 11, 2018 11:13

jsoriano removed the in progress Pull request is currently in progress. label Apr 11, 2018

jsoriano added the enhancement label Apr 11, 2018

ruflin reviewed Apr 11, 2018

View reviewed changes

jsoriano force-pushed the fsstat-filter-bind-mounts branch from cf2673f to fe6c17c Compare April 11, 2018 12:14

andrewkroh reviewed Apr 11, 2018

View reviewed changes

jsoriano force-pushed the fsstat-filter-bind-mounts branch from fe6c17c to 5427e7a Compare April 11, 2018 16:30

andrewkroh approved these changes Apr 12, 2018

View reviewed changes

jsoriano force-pushed the fsstat-filter-bind-mounts branch from 5427e7a to a0acdd5 Compare April 12, 2018 15:32

jsoriano force-pushed the fsstat-filter-bind-mounts branch from a0acdd5 to e608ad9 Compare April 13, 2018 07:59

jsoriano force-pushed the fsstat-filter-bind-mounts branch from e608ad9 to fb6a7bd Compare April 13, 2018 15:16

jsoriano added 6 commits April 16, 2018 11:14

Metricbeat: Don't filter out filesystems with the same mount point

0b2b72b

Fix spelling of description in tests

0de8263

Metricbeat: Don't collect metrics of bind-mounted directories

aed4277

Metricbeat: Ignore nodev filesystems by default

b9a9aa1

In system module, when no filesystem type is set to be ignored, it tries to make a sane guess. In systems with /proc/filesystems file it ignores all devices marked as `nodev`.

Metricbeat: Don't filter filesystems in Windows

0222403

jsoriano force-pushed the fsstat-filter-bind-mounts branch from fb6a7bd to 0222403 Compare April 16, 2018 09:15

exekias merged commit 272cad5 into elastic:master Apr 16, 2018

jsoriano mentioned this pull request Apr 16, 2018

[Metricbeat] Excluding bind mounts #3384

Closed

andrewkroh mentioned this pull request Sep 17, 2018

metricbeat: provide means to exclude non-user file systems for fsstat. #3226

Closed

jsoriano mentioned this pull request Jun 11, 2019

List all known mountpoints in filesystem events #12497

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metricbeat: Remove duplicated filesystems in system module #6819

Metricbeat: Remove duplicated filesystems in system module #6819

jsoriano commented Apr 10, 2018 •

edited

Loading

jsoriano Apr 10, 2018

ruflin Apr 10, 2018

andrewkroh Apr 11, 2018

jsoriano Apr 11, 2018

jsoriano commented Apr 10, 2018

ruflin left a comment

ruflin Apr 10, 2018

ruflin Apr 10, 2018

jsoriano Apr 10, 2018

jsoriano commented Apr 10, 2018

ruflin commented Apr 11, 2018

jsoriano commented Apr 11, 2018 •

edited

Loading

jsoriano commented Apr 11, 2018

ruflin Apr 11, 2018

jsoriano Apr 11, 2018

andrewkroh Apr 11, 2018

jsoriano Apr 11, 2018

andrewkroh left a comment

andrewkroh commented Apr 12, 2018

ruflin commented Apr 13, 2018

jsoriano commented Apr 13, 2018

ruflin commented Apr 13, 2018

jsoriano commented Apr 13, 2018

Metricbeat: Remove duplicated filesystems in system module #6819

Metricbeat: Remove duplicated filesystems in system module #6819

Conversation

jsoriano commented Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Apr 10, 2018

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Apr 10, 2018

ruflin commented Apr 11, 2018

jsoriano commented Apr 11, 2018 • edited Loading

jsoriano commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

andrewkroh commented Apr 12, 2018

ruflin commented Apr 13, 2018

jsoriano commented Apr 13, 2018

ruflin commented Apr 13, 2018

jsoriano commented Apr 13, 2018

jsoriano commented Apr 10, 2018 •

edited

Loading

jsoriano commented Apr 11, 2018 •

edited

Loading