Merge ~pjdc/ubuntu-mirror-charm/+git/ubuntu-mirror-charm:disk-checks into ubuntu-mirror-charm:master

[Paul Collins]

deploy a disk check for each mount point from which we are serving

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

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

[Paul Collins (pjdc)]

test environment looks good: https://pastebin.canonical.com/p/8hmpqyVBzb/

[Thomas Cuthbert (tcuthbert)]

LGTM just one suggestion inline plus additional comment.

review: Approve

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

Change successfully merged at revision fa87f48dff21c141b7cf2b23fac6b9baefeb320e

diff --git a/hooks/hooks.py b/hooks/hooks.py
index ff99b06..b31d5f0 100755
--- a/hooks/hooks.py
+++ b/hooks/hooks.py
@@ -56,6 +56,9 @@ apache_tls_settings = {
     'ssl_protocol': 'ALL -SSLv2 -SSLv3 -TLSv1 -TLSv1.1',
 }
 
+nagios_etcdir = "/etc/nagios/nrpe.d"
+nagios_exportdir = "/var/lib/nagios/export"
+
 required_pkgs = [
     'apache2',
     'curl',         # for check-mirror.sh
@@ -153,14 +156,11 @@ def configure_scripts(conf, hostname):
 def file_from_template(tmpl, dest, searchList, add_header=True):
     template_file = os.path.join(charm_dir(), "templates", tmpl)
     t = Template(file=template_file, searchList=searchList)
-    if os.path.isfile(dest):
-        # File already exists - remove it
-        os.unlink(dest)
-    with open(dest, "w") as f:
-        if add_header:
-            f.write(juju_header())
-        f.write(str(t))
-    os.chmod(dest, 0o444)
+    contents = ''
+    if add_header:
+        contents += juju_header()
+    contents += str(t)
+    write_file(dest, contents, perms=0o444)
     log("CHARM: Installed template {} to {}".format(tmpl, dest))
 
 
@@ -641,14 +641,62 @@ def configure_triggers(conf, hostname):
     write_file(keyfile, '\n'.join(triggers.values()) + '\n')
 
 
+def mountpoint_of(path):
+    """Test path, and its parent directories, returning the first mount point encountered."""
+    path = os.path.realpath(path)
+
+    while True:
+        if os.path.ismount(path):
+            return path
+        if path == '/':
+            break
+        path = os.path.realpath(os.path.join(path, '..'))
+
+    # If we reach / and / isn't a mount point, something's wrong somewhere.
+    raise Exception("Terminated without finding a mount point")
+
+
+def collect_mountpoints(paths):
+    mountpoints = []
+    for path in paths:
+        mountpoints.append(mountpoint_of(path))
+    return set(mountpoints)
+
+
+def generate_disk_check(conf, path):
+    mount_name = path.replace('/', '_').strip('_')
+
+    file_from_template(
+        "nagios-check_disk.tmpl",
+        os.path.join(nagios_etcdir, "check_disk_{mount_name}.cfg".format(mount_name=mount_name)),
+        {
+            "mount_name": mount_name,
+            "space_warn": conf.disk_space_warn(),
+            "space_crit": conf.disk_space_crit(),
+            "inode_crit": conf.disk_inode_crit(),
+            "path": path,
+        }
+    )
+    file_from_template(
+        "nagios-config.tmpl",
+        os.path.join(nagios_exportdir, "service__{hostname}_disk_{mount_name}.cfg".format(
+            hostname=conf.nagios_hostname(), mount_name=mount_name)),
+        {
+            "use": "active-service",
+            "nagios_hostname": conf.nagios_hostname(),
+            "description": "Disk space on {}".format(path),
+            "check_command": "check_nrpe!check_disk_{}".format(mount_name),
+            "servicegroup": conf.nagios_servicegroup(),
+        }
+    )
+    log("CHARM: Adding disk check for {}".format(path))
+
+
 def configure_nrpe(conf, hostname):  # noqa: C901
     if not conf.nagios_relation_ok():
         log("CHARM: Nagios relation not established - not configuring nrpe")
         return
 
-    nagios_etcdir = "/etc/nagios/nrpe.d"
-    nagios_exportdir = "/var/lib/nagios/export"
-
     role_checks = {
         "check_virthost": {"desc": "Check apache virthost"},
         "check_mirror": {"desc": "GPG signature via HTTP"},
@@ -668,6 +716,7 @@ def configure_nrpe(conf, hostname):  # noqa: C901
     wanted_checks = []
     # Add Nagios role checks
     roles = conf.roles()
+    paths = []
     if hostname in roles:
         tmpl_data = {}
         for role in roles[hostname]:
@@ -677,6 +726,7 @@ def configure_nrpe(conf, hostname):  # noqa: C901
                 log("CHARM: Missing details - skipping {}".format(role))
                 continue
 
+            paths.append(mirror['path'])
             tmpl_data["name"] = mirror["name"]
             role_config = get_role_config(roles, hostname, role)
             addresses = role_config.get('addresses', [])
@@ -751,30 +801,10 @@ def configure_nrpe(conf, hostname):  # noqa: C901
             except OSError:
                 log("CHARM: Unable to delete nrpe check {}".format(checkpath))
 
-    # Add disk checks (if /srv is a separate partition)
-    with open("/proc/mounts", "r") as mounts:
-        for line in mounts:
-            if line.split(" ")[1] == "/srv":
-                checkfile = "check_disk_srv.cfg"
-                checkpath = os.path.join(nagios_etcdir, checkfile)
-                tmpl_data = {}
-                tmpl_data["space_warn"] = conf.disk_space_warn()
-                tmpl_data["space_crit"] = conf.disk_space_crit()
-                tmpl_data["inode_crit"] = conf.disk_inode_crit()
-                tmpl_data["partition"] = "srv"
-                file_from_template("nagios-check_disk.tmpl", checkpath, tmpl_data)
-                configpath = os.path.join(nagios_exportdir, "service__%s_disk_srv.cfg" % (conf.nagios_hostname()))
-                tmpl_data = {}
-                tmpl_data["use"] = "active-service"
-                tmpl_data["nagios_hostname"] = conf.nagios_hostname()
-                tmpl_data["description"] = "Disk space on /srv"
-                tmpl_data["check_command"] = "check_nrpe!check_disk_srv"
-                tmpl_data["servicegroup"] = conf.nagios_servicegroup()
-                tmplfile = "nagios-config.tmpl"
-                file_from_template(tmplfile, configpath, tmpl_data)
-
-                log("CHARM: Adding checkfile {}".format(checkfile))
-                break
+    # Add disk checks
+    paths_to_check = [path for path in collect_mountpoints(paths) if path != '/']
+    for path in paths_to_check:
+        generate_disk_check(conf, path)
 
     # RT#79518: Add prompt-critical check for all disks; taken from puppet.
     check_all_disks = juju_header()
diff --git a/templates/nagios-check_disk.tmpl b/templates/nagios-check_disk.tmpl
index 4983e4f..1ed2a3e 100644
--- a/templates/nagios-check_disk.tmpl
+++ b/templates/nagios-check_disk.tmpl
@@ -1 +1 @@
-command[check_disk_${partition}]=/usr/lib/nagios/plugins/check_disk -u GB -w ${space_warn} -c ${space_crit} -K ${inode_crit} -p /${partition}
+command[check_disk_${mount_name}]=/usr/lib/nagios/plugins/check_disk -u GB -w ${space_warn} -c ${space_crit} -K ${inode_crit} -p ${path}
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 02fb4f2..a8a2fb8 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -1,4 +1,6 @@
 import json
+import os
+import pytest
 import unittest
 
 from base64 import b64encode
@@ -10,10 +12,13 @@ import hooks   # noqa: F401; for mockery
 
 from hooks import (
     SSH_TRIGGER_TEMPLATE,
+    collect_mountpoints,
     configure_triggers,
     extract_ssh_public_key,
+    generate_disk_check,
     make_triggers,
     merge_shared_key_triggers,
+    mountpoint_of,
 )
 
 from utils import (
@@ -119,3 +124,61 @@ class TestUbuntuMirrorCharm(unittest.TestCase):
         _isdir.return_value = True
         configure_triggers(conf, 'localhost')
         _write_file.assert_called_with(_get_ssh_keyfile.return_value, MERGEABLE_TRIGGERS_SSH_AUTHORIZED_KEYS)
+
+    def test_mountpoint_of_root(self):
+        """If the mountpoint of / isn't /, we're running in a weird environment."""
+        self.assertEqual(mountpoint_of('/'), '/')
+
+    def test_mountpoint_of_bin(self):
+        """If the mountpoint of /bin isn't /, we're running in a weird environment."""
+        self.assertEqual(mountpoint_of('/bin'), '/')
+
+    def test_mountpoint_of_proc_self_fd(self):
+        """Linux-specific because it's hard to think of a more general but equally complex example."""
+        self.assertEqual(mountpoint_of('/proc/self/fd'), '/proc')
+
+    def test_collect_mountpoints(self):
+        """Given the tests for mountpoint_of, we can test collect_mountpoints like this."""
+        self.assertEqual(collect_mountpoints(['/', '/bin', '/proc/self/fd']), {'/', '/proc'})
+
+    # Cheetah warns about using the Python version of NameMapper, so
+    # we ignore it here.  Matching more closely by the message doesn't
+    # seem to work, probably because it begins with a newline and
+    # Python's warnings matching code anchors the message regexp.
+    @pytest.mark.filterwarnings("ignore::UserWarning:Cheetah")
+    @patch('hooks.charm_dir')
+    @patch('hooks.write_file')
+    @patch('Config.config')
+    def test_generate_disk_check(self, _config, _write_file, _charm_dir):
+        _charm_dir.return_value = os.getcwd()
+        config = CallableDict(self.default_config)
+        _config.side_effect = config  # i.e. we want our CallableDict to supply the charm config
+        conf = Config.Config()
+        conf._relation_data = {
+            'nagios_hostname': 'localhost',
+            'nagios_host_context': 'fake-archive-mirror',
+        }
+        generate_disk_check(conf, '/srv2')
+        self.assertEqual(_write_file.call_count, 2)
+        _write_file.assert_any_call(
+            '/etc/nagios/nrpe.d/check_disk_srv2.cfg',
+            '#-------------------------------------------------#\n'
+            '# This file is Juju managed - do not edit by hand #\n'
+            '#-------------------------------------------------#\n'
+            'command[check_disk_srv2]=/usr/lib/nagios/plugins/check_disk -u GB -w 25% -c 20% -K 5% -p /srv2\n',
+            perms=0o444,
+        )
+        _write_file.assert_any_call(
+            '/var/lib/nagios/export/service__localhost_disk_srv2.cfg',
+            '#-------------------------------------------------#\n'
+            '# This file is Juju managed - do not edit by hand #\n'
+            '#-------------------------------------------------#\n'
+            'define service {\n'
+            '    use                             active-service\n'
+            '    host_name                       localhost\n'
+            '    service_description             Disk space on /srv2\n'
+            '    check_command                   check_nrpe!check_disk_srv2\n'
+            '    servicegroups                   fake-archive-mirror-ubuntu-mirror\n'
+            '}\n',
+            perms=0o444,
+        )