Simplify disk availability check, review tests

- only support a fixed list of recommended values for now, no
overwriting via Ansible variables (keep it simple, add features as
needed).

- implement is_active: run this check only for hosts that have a
recommended disk space.

- test priority of mount paths / and /var.

roles/openshift_health_checker/openshift_checks/disk_availability.py

@@ -9,65 +9,57 @@ class DiskAvailability(NotContainerizedMixin, OpenShiftCheck):
     name = "disk_availability"
     tags = ["preflight"]
 
-    # all values are base-10 as they are taken, as is, from
-    # the latest requirements for an OpenShift installation
+    # Values taken from the official installation documentation:
     # https://docs.openshift.org/latest/install_config/install/prerequisites.html#system-requirements
-    recommended_diskspace = {
-        "nodes": 15 * 10 ** 9,
-        "masters": 40 * 10 ** 9,
-        "etcd": 20 * 10 ** 9,
+    recommended_disk_space_bytes = {
+        "masters": 40 * 10**9,
+        "nodes": 15 * 10**9,
+        "etcd": 20 * 10**9,
     }
 
-    def run(self, tmp, task_vars):
-        ansible_mounts = get_var(task_vars, "ansible_mounts")
-        self.recommended_diskspace["nodes"] = get_var(task_vars,
-                                                      "min_recommended_diskspace_node",
-                                                      default=self.recommended_diskspace["nodes"])
-        self.recommended_diskspace["masters"] = get_var(task_vars,
-                                                        "min_recommended_diskspace_master",
-                                                        default=self.recommended_diskspace["masters"])
-        self.recommended_diskspace["etcd"] = get_var(task_vars,
-                                                     "min_recommended_diskspace_etcd",
-                                                     default=self.recommended_diskspace["etcd"])
-
-        failed, msg = self.volume_check(ansible_mounts, task_vars)
-        return {"failed": failed, "msg": msg}
-
-    def volume_check(self, ansible_mounts, task_vars):
+    @classmethod
+    def is_active(cls, task_vars):
+        """Skip hosts that do not have recommended disk space requirements."""
         group_names = get_var(task_vars, "group_names", default=[])
+        has_disk_space_recommendation = bool(set(group_names).intersection(cls.recommended_disk_space_bytes))
+        return super(DiskAvailability, cls).is_active(task_vars) and has_disk_space_recommendation
 
-        if not set(self.recommended_diskspace).intersection(group_names):
-            msg = "Unable to determine recommended volume size for group_name {group_name}"
-            raise OpenShiftCheckException(msg.format(group_name=group_names))
+    def run(self, tmp, task_vars):
+        group_names = get_var(task_vars, "group_names")
+        ansible_mounts = get_var(task_vars, "ansible_mounts")
 
-        recommended_diskspace_bytes = max(self.recommended_diskspace.get(group, 0) for group in group_names)
-        openshift_diskfree_bytes = self.get_openshift_disk_availability(ansible_mounts)
+        min_free_bytes = max(self.recommended_disk_space_bytes.get(name, 0) for name in group_names)
+        free_bytes = self.openshift_available_disk(ansible_mounts)
 
-        if openshift_diskfree_bytes < recommended_diskspace_bytes:
-            msg = ("Available disk space ({diskfree} GB) for the volume containing \"/var\" is "
-                   "below recommended storage. Minimum required disk space: {recommended} GB")
-            return True, msg.format(diskfree=self.to_gigabytes(openshift_diskfree_bytes),
-                                    recommended=self.to_gigabytes(recommended_diskspace_bytes))
+        if free_bytes < min_free_bytes:
+            return {
+                'failed': True,
+                'msg': (
+                    'Available disk space ({:.1f} GB) for the volume containing '
+                    '"/var" is below minimum recommended space ({:.1f} GB)'
+                ).format(float(free_bytes) / 10**9, float(min_free_bytes) / 10**9)
+            }
 
-        return False, ""
+        return {}
 
     @staticmethod
-    def get_openshift_disk_availability(ansible_mounts):
-        """Iterates through a map of mounted volumes to determine space remaining on the OpenShift volume"""
-        if not ansible_mounts:
-            msg = "Unable to determine existing volume mounts from ansible_mounts"
-            raise OpenShiftCheckException(msg)
+    def openshift_available_disk(ansible_mounts):
+        """Determine the available disk space for an OpenShift installation.
 
+        ansible_mounts should be a list of dicts like the 'setup' Ansible module
+        returns.
+        """
         # priority list in descending order
         supported_mnt_paths = ["/var", "/"]
         available_mnts = {mnt.get("mount"): mnt for mnt in ansible_mounts}
 
-        for path in supported_mnt_paths:
-            if path in available_mnts:
-                return available_mnts[path].get("size_available")
+        try:
+            for path in supported_mnt_paths:
+                if path in available_mnts:
+                    return available_mnts[path]["size_available"]
+        except KeyError:
+            pass
 
-        return 0
-
-    @staticmethod
-    def to_gigabytes(total_bytes):
-        return total_bytes / 10**9
+        paths = ''.join(sorted(available_mnts)) or 'none'
+        msg = "Unable to determine available disk space. Paths mounted: {}.".format(paths)
+        raise OpenShiftCheckException(msg)

roles/openshift_health_checker/test/disk_availability_test.py

@@ -3,104 +3,153 @@ import pytest
 from openshift_checks.disk_availability import DiskAvailability, OpenShiftCheckException
 
 
-def test_exception_raised_on_empty_ansible_mounts():
+@pytest.mark.parametrize('group_names,is_containerized,is_active', [
+    (['masters'], False, True),
+    # ensure check is skipped on containerized installs
+    (['masters'], True, False),
+    (['nodes'], False, True),
+    (['etcd'], False, True),
+    (['masters', 'nodes'], False, True),
+    (['masters', 'etcd'], False, True),
+    ([], False, False),
+    (['lb'], False, False),
+    (['nfs'], False, False),
+])
+def test_is_active(group_names, is_containerized, is_active):
+    task_vars = dict(
+        group_names=group_names,
+        openshift=dict(common=dict(is_containerized=is_containerized)),
+    )
+    assert DiskAvailability.is_active(task_vars=task_vars) == is_active
+
+
+@pytest.mark.parametrize('ansible_mounts,extra_words', [
+    ([], ['none']), # empty ansible_mounts
+    ([{'mount': '/mnt'}], ['/mnt']), # missing relevant mount paths
+    ([{'mount': '/var'}], ['/var']), # missing size_available
+])
+def test_cannot_determine_available_disk(ansible_mounts, extra_words):
+    task_vars = dict(
+        group_names=['masters'],
+        ansible_mounts=ansible_mounts,
+    )
+    check = DiskAvailability(execute_module=fake_execute_module)
+
     with pytest.raises(OpenShiftCheckException) as excinfo:
-        DiskAvailability(execute_module=NotImplementedError).get_openshift_disk_availability([])
+        check.run(tmp=None, task_vars=task_vars)
 
-    assert "existing volume mounts from ansible_mounts" in str(excinfo.value)
+    for word in 'determine available disk'.split() + extra_words:
+        assert word in str(excinfo.value)
 
 
-@pytest.mark.parametrize("group_name,size_available", [
+@pytest.mark.parametrize('group_names,ansible_mounts', [
+    (
+        ['masters'],
+        [{
+            'mount': '/',
+            'size_available': 40 * 10**9 + 1,
+        }],
+    ),
     (
-        "masters",
-        41110980608,
+        ['nodes'],
+        [{
+            'mount': '/',
+            'size_available': 15 * 10**9 + 1,
+        }],
     ),
     (
-        "nodes",
-        21110980608,
+        ['etcd'],
+        [{
+            'mount': '/',
+            'size_available': 20 * 10**9 + 1,
+        }],
     ),
     (
-        "etcd",
-        21110980608,
+        ['etcd'],
+        [{
+            # not enough space on / ...
+            'mount': '/',
+            'size_available': 0,
+        }, {
+            # ... but enough on /var
+            'mount': '/var',
+            'size_available': 20 * 10**9 + 1,
+        }],
     ),
 ])
-def test_volume_check_with_recommended_diskspace(group_name, size_available):
-    result = DiskAvailability(execute_module=NotImplementedError).run(tmp=None, task_vars=dict(
-        openshift=dict(common=dict(
-            service_type='origin',
-            is_containerized=False,
-        )),
-        group_names=[group_name],
-        ansible_mounts=[{
-            "mount": "/",
-            "size_available": size_available,
-        }]
-    ))
-
-    assert not result['failed']
-    assert not result['msg']
-
-
-@pytest.mark.parametrize("group_name,size_available", [
+def test_succeeds_with_recommended_disk_space(group_names, ansible_mounts):
+    task_vars = dict(
+        group_names=group_names,
+        ansible_mounts=ansible_mounts,
+    )
+
+    check = DiskAvailability(execute_module=fake_execute_module)
+    result = check.run(tmp=None, task_vars=task_vars)
+
+    assert not result.get('failed', False)
+
+
+@pytest.mark.parametrize('group_names,ansible_mounts,extra_words', [
+    (
+        ['masters'],
+        [{
+            'mount': '/',
+            'size_available': 1,
+        }],
+        ['0.0 GB'],
+    ),
     (
-        "masters",
-        21110980608,
+        ['nodes'],
+        [{
+            'mount': '/',
+            'size_available': 1 * 10**9,
+        }],
+        ['1.0 GB'],
     ),
     (
-        "nodes",
-        1110980608,
+        ['etcd'],
+        [{
+            'mount': '/',
+            'size_available': 1,
+        }],
+        ['0.0 GB'],
     ),
     (
-        "etcd",
-        1110980608,
+        ['nodes', 'masters'],
+        [{
+            'mount': '/',
+            # enough space for a node, not enough for a master
+            'size_available': 15 * 10**9 + 1,
+        }],
+        ['15.0 GB'],
+    ),
+    (
+        ['etcd'],
+        [{
+            # enough space on / ...
+            'mount': '/',
+            'size_available': 20 * 10**9 + 1,
+        }, {
+            # .. but not enough on /var
+            'mount': '/var',
+            'size_available': 0,
+        }],
+        ['0.0 GB'],
     ),
 ])
-def test_volume_check_with_insufficient_diskspace(group_name, size_available):
-    result = DiskAvailability(execute_module=NotImplementedError).run(tmp=None, task_vars=dict(
-        openshift=dict(common=dict(
-            service_type='origin',
-            is_containerized=False,
-        )),
-        group_names=[group_name],
-        ansible_mounts=[{
-            "mount": "/",
-            "size_available": size_available,
-        }]
-    ))
+def test_fails_with_insufficient_disk_space(group_names, ansible_mounts, extra_words):
+    task_vars = dict(
+        group_names=group_names,
+        ansible_mounts=ansible_mounts,
+    )
 
-    assert result['failed']
-    assert "is below recommended storage" in result['msg']
-
-
-def test_volume_check_with_unsupported_mountpaths():
-    result = DiskAvailability(execute_module=NotImplementedError).run(tmp=None, task_vars=dict(
-        openshift=dict(common=dict(
-            service_type='origin',
-            is_containerized=False,
-        )),
-        group_names=["masters", "nodes"],
-        ansible_mounts=[{
-            "mount": "/unsupported",
-            "size_available": 12345,
-        }]
-    ))
+    check = DiskAvailability(execute_module=fake_execute_module)
+    result = check.run(tmp=None, task_vars=task_vars)
 
     assert result['failed']
-    assert "0 GB" in result['msg']
+    for word in 'below recommended'.split() + extra_words:
+        assert word in result['msg']
 
 
-def test_volume_check_with_invalid_groupname():
-    with pytest.raises(OpenShiftCheckException) as excinfo:
-        result = DiskAvailability(execute_module=NotImplementedError).run(tmp=None, task_vars=dict(
-            openshift=dict(common=dict(
-                service_type='origin',
-                is_containerized=False,
-            )),
-            group_names=["invalid"],
-            ansible_mounts=[{
-                "mount": "/unsupported",
-                "size_available": 12345,
-            }]
-        ))
-
-    assert "'invalid'" in str(excinfo.value)
+def fake_execute_module(*args):
+    raise AssertionError('this function should not be called')