Merge ~npochet/charm-nagios:exclude-tracefs into ~nagios-charmers/charm-nagios:master

[Nicolas Pochet]

Exclude virtual fs from local disk checks

Closes-Bug: 1836122

Exclude virtual fs from local disk checks

Closes-Bug: 1836122

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[James Hebden (ec0)]

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.

review: Needs Fixing

[Nicolas Pochet (npochet)]

> - 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).

It is now harmonized.

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

That now includes those FS.

[Tom Haddon (mthaddon)]

Hi Nicholas, if you're ready for another review, please add a comment selecting the "Review" type of "Resubmit" - this makes it easier for reviewers to know if you've addressed all of the comments and are ready for another review, or if you're still working on changes. Thanks.

[Nicolas Pochet (npochet)]

It is indeed ready for another review.
Thanks!

review: Needs Resubmitting

[Jeremy Lounder (jldev)]

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 00b10a5ba0a95f88e7871e2b0d7b1ab62ce7c932

diff --git a/hooks/templates/localhost_nagios2.cfg.tmpl b/hooks/templates/localhost_nagios2.cfg.tmpl
index 9fc4f32..e778357 100644
--- a/hooks/templates/localhost_nagios2.cfg.tmpl
+++ b/hooks/templates/localhost_nagios2.cfg.tmpl
@@ -22,10 +22,10 @@ define host{
 
 
 
-# 'check_all_disks_no_squashfs' command definition
+# 'check_all_disks_no_virtual_fs' command definition
 define command{
-        command_name    check_all_disks_no_squashfs
-        command_line    /usr/lib/nagios/plugins/check_disk -w '$ARG1$' -c '$ARG2$' -e -X squashfs
+        command_name    check_all_disks_no_virtual_fs
+        command_line    /usr/lib/nagios/plugins/check_disk -w '$ARG1$' -c '$ARG2$' -e -X squashfs -X tracefs -X debugfs -X procfs -X sysfs -X cgroup -X cgroup2 -X nsfs -X hugetlbfs -X bpf -X devpts
         }
 
 # Define a service to check the disk space of the root partition