nagios-charmers team mailing list archive

[James Hebden <james.hebden@xxxxxxxxxxxxx>]

Review: Needs Fixing

In taking a look at this merge proposal, I have noticed a couple of things which should be corrected prior to this being merged:

- this change excludes tracefs, rather than debugfs in the initial bug. I don't believe excluding tracefs addressed the failure mode described in the bug.

- related, the filed bug highlights that we are monitoring virtual filesystems, not specifically tracefs or debugfs, while this merge only excludes tracefs. While making this change, we really should include as many sensible virtual filesystems as we can.

- nitpick, the additional filesystem uses the --exclude-type argument, whilst the squashfs exclusion uses -X. We should pick on (-X is a lot shorter and tidier in my opinion) and stick to it, because otherwise it looks to someone who is spinning up on this code as if they do different things. It hurts the readability of the code.

My suggestion is to convert to flags to all be '-X' instead of having one or more -X and one or more --exclude-type flags - and to include as many virtual filesystems as we see on a typically deployed Ubuntu system in supported releases (Xenial, Bionic).

At a minimum, I would expect us to exclude tracefs, debugfs, procfs, sysfs, squashfs, cgroup, cgroup2, nsfs, hugetlbfs, bpf, devpts - there are others, though.

There are alternate approaches to this as well, we could instead whitelist filesystems, only monitoring filesystems with actual block devices backing them, but a variation on the above blacklist is probably the biggest improvement for the smallest change in charm functionality.
-- 
https://code.launchpad.net/~npochet/nagios-charm/+git/nagios-charm/+merge/371645
Your team Nagios Charm developers is subscribed to branch nagios-charm:master.