Merge pull request #4944 from sosiouxme/20170728-refactor-ansible-mounts

openshift_checks: refactor find_ansible_mount

8 files changed, 73 insertions, 88 deletions

diff --git a/roles/openshift_health_checker/openshift_checks/__init__.py b/roles/openshift_health_checker/openshift_checks/__init__.py
index 09139408c..07ec6f7ef 100644
--- a/roles/openshift_health_checker/openshift_checks/__init__.py
+++ b/roles/openshift_health_checker/openshift_checks/__init__.py
@@ -197,6 +197,31 @@ class OpenShiftCheck(object):
         components = tuple(int(x) for x in components[:2])
         return components
 
+    def find_ansible_mount(self, path):
+        """Return the mount point for path from ansible_mounts."""
+
+        # reorganize list of mounts into dict by path
+        mount_for_path = {
+            mount['mount']: mount
+            for mount
+            in self.get_var('ansible_mounts')
+        }
+
+        # NOTE: including base cases '/' and '' to ensure the loop ends
+        mount_targets = set(mount_for_path.keys()) | {'/', ''}
+        mount_point = path
+        while mount_point not in mount_targets:
+            mount_point = os.path.dirname(mount_point)
+
+        try:
+            return mount_for_path[mount_point]
+        except KeyError:
+            known_mounts = ', '.join('"{}"'.format(mount) for mount in sorted(mount_for_path))
+            raise OpenShiftCheckException(
+                'Unable to determine mount point for path "{}".\n'
+                'Known mount points: {}.'.format(path, known_mounts or 'none')
+            )
+
 
 LOADER_EXCLUDES = (
     "__init__.py",
diff --git a/roles/openshift_health_checker/openshift_checks/disk_availability.py b/roles/openshift_health_checker/openshift_checks/disk_availability.py
index 39ac0e4ec..6d1dea9ce 100644
--- a/roles/openshift_health_checker/openshift_checks/disk_availability.py
+++ b/roles/openshift_health_checker/openshift_checks/disk_availability.py
@@ -1,6 +1,5 @@
 """Check that there is enough disk space in predefined paths."""
 
-import os.path
 import tempfile
 
 from openshift_checks import OpenShiftCheck, OpenShiftCheckException
@@ -55,9 +54,6 @@ class DiskAvailability(OpenShiftCheck):
 
     def run(self):
         group_names = self.get_var("group_names")
-        ansible_mounts = self.get_var("ansible_mounts")
-        ansible_mounts = {mount['mount']: mount for mount in ansible_mounts}
-
         user_config = self.get_var("openshift_check_min_host_disk_gb", default={})
         try:
             # For backwards-compatibility, if openshift_check_min_host_disk_gb
@@ -80,7 +76,7 @@ class DiskAvailability(OpenShiftCheck):
         # not part of the official recommendation but present in the user
         # configuration.
         for path, recommendation in self.recommended_disk_space_bytes.items():
-            free_bytes = self.free_bytes(path, ansible_mounts)
+            free_bytes = self.free_bytes(path)
             recommended_bytes = max(recommendation.get(name, 0) for name in group_names)
 
             config = user_config.get(path, {})
@@ -127,22 +123,17 @@ class DiskAvailability(OpenShiftCheck):
 
         return {}
 
-    @staticmethod
-    def free_bytes(path, ansible_mounts):
+    def free_bytes(self, path):
         """Return the size available in path based on ansible_mounts."""
-        mount_point = path
-        # arbitry value to prevent an infinite loop, in the unlike case that '/'
-        # is not in ansible_mounts.
-        max_depth = 32
-        while mount_point not in ansible_mounts and max_depth > 0:
-            mount_point = os.path.dirname(mount_point)
-            max_depth -= 1
-
+        mount = self.find_ansible_mount(path)
         try:
-            free_bytes = ansible_mounts[mount_point]['size_available']
+            return mount['size_available']
         except KeyError:
-            known_mounts = ', '.join('"{}"'.format(mount) for mount in sorted(ansible_mounts)) or 'none'
-            msg = 'Unable to determine disk availability for "{}". Known mount points: {}.'
-            raise OpenShiftCheckException(msg.format(path, known_mounts))
-
-        return free_bytes
+            raise OpenShiftCheckException(
+                'Unable to retrieve disk availability for "{path}".\n'
+                'Ansible facts included a matching mount point for this path:\n'
+                '  {mount}\n'
+                'however it is missing the size_available field.\n'
+                'To investigate, you can inspect the output of `ansible -m setup <host>`'
+                ''.format(path=path, mount=mount)
+            )
diff --git a/roles/openshift_health_checker/openshift_checks/docker_storage.py b/roles/openshift_health_checker/openshift_checks/docker_storage.py
index 7ae384bd7..0558ddf14 100644
--- a/roles/openshift_health_checker/openshift_checks/docker_storage.py
+++ b/roles/openshift_health_checker/openshift_checks/docker_storage.py
@@ -1,6 +1,5 @@
 """Check Docker storage driver and usage."""
 import json
-import os.path
 import re
 from openshift_checks import OpenShiftCheck, OpenShiftCheckException
 from openshift_checks.mixins import DockerHostMixin
@@ -252,7 +251,7 @@ class DockerStorage(DockerHostMixin, OpenShiftCheck):
                 "msg": "Specified 'max_overlay_usage_percent' is not a percentage: {}".format(threshold),
             }
 
-        mount = self.find_ansible_mount(path, self.get_var("ansible_mounts"))
+        mount = self.find_ansible_mount(path)
         try:
             free_bytes = mount['size_available']
             total_bytes = mount['size_total']
@@ -275,22 +274,3 @@ class DockerStorage(DockerHostMixin, OpenShiftCheck):
             }
 
         return {}
-
-    # TODO(lmeyer): migrate to base class
-    @staticmethod
-    def find_ansible_mount(path, ansible_mounts):
-        """Return the mount point for path from ansible_mounts."""
-
-        mount_for_path = {mount['mount']: mount for mount in ansible_mounts}
-        mount_point = path
-        while mount_point not in mount_for_path:
-            if mount_point in ["/", ""]:  # "/" not in ansible_mounts???
-                break
-            mount_point = os.path.dirname(mount_point)
-
-        try:
-            return mount_for_path[mount_point]
-        except KeyError:
-            known_mounts = ', '.join('"{}"'.format(mount) for mount in sorted(mount_for_path)) or 'none'
-            msg = 'Unable to determine mount point for path "{}". Known mount points: {}.'
-            raise OpenShiftCheckException(msg.format(path, known_mounts))
diff --git a/roles/openshift_health_checker/openshift_checks/etcd_imagedata_size.py b/roles/openshift_health_checker/openshift_checks/etcd_imagedata_size.py
index ae8460b7e..f4296753a 100644
--- a/roles/openshift_health_checker/openshift_checks/etcd_imagedata_size.py
+++ b/roles/openshift_health_checker/openshift_checks/etcd_imagedata_size.py
@@ -2,7 +2,7 @@
 Ansible module for determining if the size of OpenShift image data exceeds a specified limit in an etcd cluster.
 """
 
-from openshift_checks import OpenShiftCheck, OpenShiftCheckException
+from openshift_checks import OpenShiftCheck
 
 
 class EtcdImageDataSize(OpenShiftCheck):
@@ -12,7 +12,7 @@ class EtcdImageDataSize(OpenShiftCheck):
     tags = ["etcd"]
 
     def run(self):
-        etcd_mountpath = self._get_etcd_mountpath(self.get_var("ansible_mounts"))
+        etcd_mountpath = self.find_ansible_mount("/var/lib/etcd")
         etcd_avail_diskspace = etcd_mountpath["size_available"]
         etcd_total_diskspace = etcd_mountpath["size_total"]
 
@@ -68,18 +68,5 @@ class EtcdImageDataSize(OpenShiftCheck):
         return {}
 
     @staticmethod
-    def _get_etcd_mountpath(ansible_mounts):
-        valid_etcd_mount_paths = ["/var/lib/etcd", "/var/lib", "/var", "/"]
-
-        mount_for_path = {mnt.get("mount"): mnt for mnt in ansible_mounts}
-        for path in valid_etcd_mount_paths:
-            if path in mount_for_path:
-                return mount_for_path[path]
-
-        paths = ', '.join(sorted(mount_for_path)) or 'none'
-        msg = "Unable to determine a valid etcd mountpath. Paths mounted: {}.".format(paths)
-        raise OpenShiftCheckException(msg)
-
-    @staticmethod
     def _to_gigabytes(byte_size):
         return float(byte_size) / 10.0**9
diff --git a/roles/openshift_health_checker/openshift_checks/etcd_volume.py b/roles/openshift_health_checker/openshift_checks/etcd_volume.py
index e55d55e91..e5d93ff3f 100644
--- a/roles/openshift_health_checker/openshift_checks/etcd_volume.py
+++ b/roles/openshift_health_checker/openshift_checks/etcd_volume.py
@@ -1,6 +1,6 @@
 """A health check for OpenShift clusters."""
 
-from openshift_checks import OpenShiftCheck, OpenShiftCheckException
+from openshift_checks import OpenShiftCheck
 
 
 class EtcdVolume(OpenShiftCheck):
@@ -11,8 +11,8 @@ class EtcdVolume(OpenShiftCheck):
 
     # Default device usage threshold. Value should be in the range [0, 100].
     default_threshold_percent = 90
-    # Where to find ectd data, higher priority first.
-    supported_mount_paths = ["/var/lib/etcd", "/var/lib", "/var", "/"]
+    # Where to find etcd data
+    etcd_mount_path = "/var/lib/etcd"
 
     def is_active(self):
         etcd_hosts = self.get_var("groups", "etcd", default=[]) or self.get_var("groups", "masters", default=[]) or []
@@ -20,7 +20,7 @@ class EtcdVolume(OpenShiftCheck):
         return super(EtcdVolume, self).is_active() and is_etcd_host
 
     def run(self):
-        mount_info = self._etcd_mount_info()
+        mount_info = self.find_ansible_mount(self.etcd_mount_path)
         available = mount_info["size_available"]
         total = mount_info["size_total"]
         used = total - available
@@ -41,15 +41,3 @@ class EtcdVolume(OpenShiftCheck):
             return {"failed": True, "msg": msg}
 
         return {}
-
-    def _etcd_mount_info(self):
-        ansible_mounts = self.get_var("ansible_mounts")
-        mounts = {mnt.get("mount"): mnt for mnt in ansible_mounts}
-
-        for path in self.supported_mount_paths:
-            if path in mounts:
-                return mounts[path]
-
-        paths = ', '.join(sorted(mounts)) or 'none'
-        msg = "Unable to find etcd storage mount point. Paths mounted: {}.".format(paths)
-        raise OpenShiftCheckException(msg)
diff --git a/roles/openshift_health_checker/test/disk_availability_test.py b/roles/openshift_health_checker/test/disk_availability_test.py
index 5720eeacf..f4fd2dfed 100644
--- a/roles/openshift_health_checker/test/disk_availability_test.py
+++ b/roles/openshift_health_checker/test/disk_availability_test.py
@@ -20,12 +20,24 @@ def test_is_active(group_names, is_active):
     assert DiskAvailability(None, task_vars).is_active() == 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
+@pytest.mark.parametrize('desc, ansible_mounts, expect_chunks', [
+    (
+        'empty ansible_mounts',
+        [],
+        ['determine mount point', 'none'],
+    ),
+    (
+        'missing relevant mount paths',
+        [{'mount': '/mnt'}],
+        ['determine mount point', '/mnt'],
+    ),
+    (
+        'missing size_available',
+        [{'mount': '/var'}, {'mount': '/usr'}, {'mount': '/tmp'}],
+        ['missing', 'size_available'],
+    ),
 ])
-def test_cannot_determine_available_disk(ansible_mounts, extra_words):
+def test_cannot_determine_available_disk(desc, ansible_mounts, expect_chunks):
     task_vars = dict(
         group_names=['masters'],
         ansible_mounts=ansible_mounts,
@@ -34,8 +46,8 @@ def test_cannot_determine_available_disk(ansible_mounts, extra_words):
     with pytest.raises(OpenShiftCheckException) as excinfo:
         DiskAvailability(fake_execute_module, task_vars).run()
 
-    for word in 'determine disk availability'.split() + extra_words:
-        assert word in str(excinfo.value)
+    for chunk in expect_chunks:
+        assert chunk in str(excinfo.value)
 
 
 @pytest.mark.parametrize('group_names,configured_min,ansible_mounts', [
@@ -97,7 +109,7 @@ def test_succeeds_with_recommended_disk_space(group_names, configured_min, ansib
     assert not result.get('failed', False)
 
 
-@pytest.mark.parametrize('name,group_names,configured_min,ansible_mounts,extra_words', [
+@pytest.mark.parametrize('name,group_names,configured_min,ansible_mounts,expect_chunks', [
     (
         'test with no space available',
         ['masters'],
@@ -164,7 +176,7 @@ def test_succeeds_with_recommended_disk_space(group_names, configured_min, ansib
         ['0.0 GB'],
     ),
 ], ids=lambda argval: argval[0])
-def test_fails_with_insufficient_disk_space(name, group_names, configured_min, ansible_mounts, extra_words):
+def test_fails_with_insufficient_disk_space(name, group_names, configured_min, ansible_mounts, expect_chunks):
     task_vars = dict(
         group_names=group_names,
         openshift_check_min_host_disk_gb=configured_min,
@@ -174,8 +186,8 @@ def test_fails_with_insufficient_disk_space(name, group_names, configured_min, a
     result = DiskAvailability(fake_execute_module, task_vars).run()
 
     assert result['failed']
-    for word in 'below recommended'.split() + extra_words:
-        assert word in result.get('msg', '')
+    for chunk in 'below recommended'.split() + expect_chunks:
+        assert chunk in result.get('msg', '')
 
 
 @pytest.mark.parametrize('name,group_names,context,ansible_mounts,failed,extra_words', [
diff --git a/roles/openshift_health_checker/test/etcd_imagedata_size_test.py b/roles/openshift_health_checker/test/etcd_imagedata_size_test.py
index e3d6706fa..d3aae98f2 100644
--- a/roles/openshift_health_checker/test/etcd_imagedata_size_test.py
+++ b/roles/openshift_health_checker/test/etcd_imagedata_size_test.py
@@ -1,7 +1,8 @@
 import pytest
 
 from collections import namedtuple
-from openshift_checks.etcd_imagedata_size import EtcdImageDataSize, OpenShiftCheckException
+from openshift_checks.etcd_imagedata_size import EtcdImageDataSize
+from openshift_checks import OpenShiftCheckException
 from etcdkeysize import check_etcd_key_size
 
 
@@ -56,7 +57,7 @@ def test_cannot_determine_available_mountpath(ansible_mounts, extra_words):
     with pytest.raises(OpenShiftCheckException) as excinfo:
         check.run()
 
-    for word in 'determine valid etcd mountpath'.split() + extra_words:
+    for word in ['Unable to determine mount point'] + extra_words:
         assert word in str(excinfo.value)
 
 
diff --git a/roles/openshift_health_checker/test/etcd_volume_test.py b/roles/openshift_health_checker/test/etcd_volume_test.py
index 0b255136e..077cea3ea 100644
--- a/roles/openshift_health_checker/test/etcd_volume_test.py
+++ b/roles/openshift_health_checker/test/etcd_volume_test.py
@@ -1,6 +1,7 @@
 import pytest
 
-from openshift_checks.etcd_volume import EtcdVolume, OpenShiftCheckException
+from openshift_checks.etcd_volume import EtcdVolume
+from openshift_checks import OpenShiftCheckException
 
 
 @pytest.mark.parametrize('ansible_mounts,extra_words', [
@@ -15,7 +16,7 @@ def test_cannot_determine_available_disk(ansible_mounts, extra_words):
     with pytest.raises(OpenShiftCheckException) as excinfo:
         EtcdVolume(fake_execute_module, task_vars).run()
 
-    for word in 'Unable to find etcd storage mount point'.split() + extra_words:
+    for word in ['Unable to determine mount point'] + extra_words:
         assert word in str(excinfo.value)