From 437ce63475baef208bcec51c9783c4d65e5df532 Mon Sep 17 00:00:00 2001 From: juanvallejo Date: Fri, 28 Apr 2017 15:58:15 -0400 Subject: [PATCH] update error handling --- .../openshift_checks/docker_storage.py | 92 +++++++++---------- .../openshift_checks/docker_storage_driver.py | 4 +- .../test/docker_storage_test.py | 76 ++++++++++++--- 3 files changed, 108 insertions(+), 64 deletions(-) diff --git a/roles/openshift_health_checker/openshift_checks/docker_storage.py b/roles/openshift_health_checker/openshift_checks/docker_storage.py index 0a157743d9f..1ac831f0310 100644 --- a/roles/openshift_health_checker/openshift_checks/docker_storage.py +++ b/roles/openshift_health_checker/openshift_checks/docker_storage.py @@ -1,14 +1,13 @@ # pylint: disable=missing-docstring import json -from openshift_checks import OpenShiftCheck, get_var +from openshift_checks import OpenShiftCheck, OpenShiftCheckException, get_var class DockerStorage(OpenShiftCheck): """Check Docker storage sanity. - This check ensures that thinpool usage is not - close to 100% for a containerized installation. + Check for thinpool usage during a containerized installation """ name = "docker_storage" @@ -24,86 +23,83 @@ def is_active(cls, task_vars): return super(DockerStorage, cls).is_active(task_vars) and is_containerized def run(self, tmp, task_vars): - self.max_thinpool_data_usage_percent = get_var(task_vars, - "max_thinpool_data_usage_percent", - default=self.max_thinpool_data_usage_percent) - self.max_thinpool_metadata_usage_percent = get_var(task_vars, "max_thinpool_metadata_usage_percent", - default=self.max_thinpool_metadata_usage_percent) + try: + self.max_thinpool_data_usage_percent = float(get_var(task_vars, + "max_thinpool_data_usage_percent", + default=self.max_thinpool_data_usage_percent)) + self.max_thinpool_metadata_usage_percent = float(get_var(task_vars, "max_thinpool_metadata_usage_percent", + default=self.max_thinpool_metadata_usage_percent)) + except ValueError as err: + return { + "failed": True, + "msg": "Unable to convert thinpool data usage limit to float: {}".format(str(err)) + } - failed, msg = self.check_thinpool_usage(task_vars) - if failed: - return {"failed": True, "msg": msg} + err_msg = self.check_thinpool_usage(task_vars) + if err_msg: + return {"failed": True, "msg": err_msg} - return {"changed": False} + return {} def check_thinpool_usage(self, task_vars): - no_data_msg = "no thinpool usage data returned by the host." - - failed, data, msg = self.get_lvs_data(task_vars) - if failed: - if not msg: - msg = no_data_msg - return failed, msg - - lv = self.extract_thinpool_obj(data) - if not lv: - return True, no_data_msg - - failed, data_percent = self.get_thinpool_data_usage(lv) - if failed: - return failed, no_data_msg + lv_data = self.get_lvs_data(task_vars) + lv = self.extract_thinpool_obj(lv_data) - failed, metadata_percent = self.get_thinpool_metadata_usage(lv) - if failed: - return failed, no_data_msg + data_percent = self.get_thinpool_data_usage(lv) + metadata_percent = self.get_thinpool_metadata_usage(lv) - if data_percent > float(self.max_thinpool_data_usage_percent): + if data_percent > self.max_thinpool_data_usage_percent: msg = "thinpool data usage above maximum threshold of {threshold}%" - return True, msg.format(threshold=self.max_thinpool_data_usage_percent) + return msg.format(threshold=self.max_thinpool_data_usage_percent) - if metadata_percent > float(self.max_thinpool_metadata_usage_percent): + if metadata_percent > self.max_thinpool_metadata_usage_percent: msg = "thinpool metadata usage above maximum threshold of {threshold}%" - return True, msg.format(threshold=self.max_thinpool_metadata_usage_percent) + return msg.format(threshold=self.max_thinpool_metadata_usage_percent) - return False, "" + return "" def get_lvs_data(self, task_vars): lvs_cmd = "/sbin/lvs --select vg_name=docker --select lv_name=docker-pool --report-format json" result = self.exec_cmd(lvs_cmd, task_vars) - failed = False if result.get("failed", False): - return True, {}, result.get("msg", "") + raise OpenShiftCheckException("no thinpool usage data returned by the host: {}".format(result.get("msg", ""))) try: - data_json = json.loads(result.get("stdout")) + data_json = json.loads(result.get("stdout", "")) except ValueError as err: - return True, {}, str(err) + raise OpenShiftCheckException("Invalid value returned by lvs command: {}".format(str(err))) - return failed, data_json.get("report", {}), "" + data = data_json.get("report") + if not data: + raise OpenShiftCheckException("no thinpool usage data returned by the host.") + + return data - def get_thinpool_data_usage(self, thinpool_lv_data): + @staticmethod + def get_thinpool_data_usage(thinpool_lv_data): data = thinpool_lv_data.get("data_percent") if not data: - return True, 0 + raise OpenShiftCheckException("no thinpool usage data returned by the host.") - return False, float(data) + return float(data) - def get_thinpool_metadata_usage(self, thinpool_lv_data): + @staticmethod + def get_thinpool_metadata_usage(thinpool_lv_data): data = thinpool_lv_data.get("metadata_percent") if not data: - return True, 0 + raise OpenShiftCheckException("no thinpool usage data returned by the host.") - return False, float(data) + return float(data) @staticmethod def extract_thinpool_obj(thinpool_data): if not thinpool_data or not thinpool_data[0]: - return None + raise OpenShiftCheckException("no thinpool usage data returned by the host.") lv = thinpool_data[0].get("lv") if not lv or not lv[0]: - return None + raise OpenShiftCheckException("no thinpool usage data returned by the host.") return lv[0] diff --git a/roles/openshift_health_checker/openshift_checks/docker_storage_driver.py b/roles/openshift_health_checker/openshift_checks/docker_storage_driver.py index 3db9e1bbbbc..94ea7ba9c0f 100644 --- a/roles/openshift_health_checker/openshift_checks/docker_storage_driver.py +++ b/roles/openshift_health_checker/openshift_checks/docker_storage_driver.py @@ -25,13 +25,13 @@ def run(self, tmp, task_vars): if not self.is_supported_storage_driver(info): msg = "Unsupported Docker storage driver detected. Supported storage drivers: {drivers}" - return {"failed": True, "msg": msg.format(drivers=self.storage_drivers)} + return {"failed": True, "msg": msg.format(drivers=', '.join(self.storage_drivers))} if self.is_using_loopback_device(info): msg = "Use of loopback devices is discouraged. Try running Docker with `--storage-opt dm.thinpooldev`" return {"failed": True, "msg": msg} - return {"changed": False} + return {} def is_supported_storage_driver(self, docker_info): return docker_info.get("Driver", "") in self.storage_drivers diff --git a/roles/openshift_health_checker/test/docker_storage_test.py b/roles/openshift_health_checker/test/docker_storage_test.py index 12417de1b67..d82d750bcb8 100644 --- a/roles/openshift_health_checker/test/docker_storage_test.py +++ b/roles/openshift_health_checker/test/docker_storage_test.py @@ -2,7 +2,7 @@ import json -from openshift_checks.docker_storage import DockerStorage +from openshift_checks.docker_storage import DockerStorage, OpenShiftCheckException @pytest.mark.parametrize('is_containerized,is_active', [ @@ -18,9 +18,9 @@ def test_is_active(is_containerized, is_active): @pytest.mark.parametrize('stdout,message,failed,extra_words', [ (None, "", True, ["no thinpool usage data"]), - ("", "", False, ["No JSON object could be decoded"]), + ("", "", False, ["Invalid value returned by lvs command"]), (None, "invalid response", True, ["invalid response"]), - ("", "invalid response", False, ["No JSON object could be decoded"]), + ("invalid", "invalid response", False, ["No JSON object could be decoded"]), ]) def test_get_lvs_data_with_failed_response(stdout, message, failed, extra_words): def execute_module(module_name, args, tmp=None, task_vars=None): @@ -29,20 +29,70 @@ def execute_module(module_name, args, tmp=None, task_vars=None): "changed": False, } - return { + response = { "stdout": stdout, "msg": message, "failed": failed, } + if stdout is None: + response.pop("stdout") + + return response + task_vars = dict( - max_thinpool_data_usage_percent="90" + max_thinpool_data_usage_percent=90.0 ) - check = DockerStorage(execute_module=execute_module).run(tmp=None, task_vars=task_vars) + check = DockerStorage(execute_module=execute_module) + with pytest.raises(OpenShiftCheckException) as excinfo: + check.run(tmp=None, task_vars=task_vars) for word in extra_words: - assert word in check["msg"] + assert word in str(excinfo.value) + + +@pytest.mark.parametrize('limit_percent,failed,extra_words', [ + ("90.0", False, []), + (80.0, False, []), + ("invalid percent", True, ["Unable to convert", "to float", "could not convert string to float: invalid percent"]), + ("90%", True, ["Unable to convert", "to float", "invalid literal for float(): 90%"]), +]) +def test_invalid_value_for_thinpool_usage_limit(limit_percent, failed, extra_words): + def execute_module(module_name, args, tmp=None, task_vars=None): + if module_name != "command": + return { + "changed": False, + } + + return { + "stdout": json.dumps({ + "report": [ + { + "lv": [ + {"lv_name": "docker-pool", "vg_name": "docker", "lv_attr": "twi-aot---", "lv_size": "6.95g", + "pool_lv": "", "origin": "", "data_percent": "58.96", "metadata_percent": "4.77", + "move_pv": "", "mirror_log": "", "copy_percent": "", "convert_lv": ""}, + ] + } + ] + }), + "failed": False, + } + + task_vars = dict( + max_thinpool_data_usage_percent=limit_percent + ) + + check = DockerStorage(execute_module=execute_module).run(tmp=None, task_vars=task_vars) + + if failed: + assert check["failed"] + + for word in extra_words: + assert word in check["msg"] + else: + assert not check.get("failed", False) def test_get_lvs_data_with_valid_response(): @@ -71,7 +121,7 @@ def execute_module(module_name, args, tmp=None, task_vars=None): ) check = DockerStorage(execute_module=execute_module).run(tmp=None, task_vars=task_vars) - assert check + assert not check.get("failed", False) @pytest.mark.parametrize('response,extra_words', [ @@ -134,13 +184,11 @@ def execute_module(module_name, args, tmp=None, task_vars=None): max_thinpool_data_usage_percent=90.0 ) - check = DockerStorage(execute_module=execute_module).run(tmp=None, task_vars=task_vars) - assert "no thinpool usage data" in check["msg"] - - - - + check = DockerStorage(execute_module=execute_module) + with pytest.raises(OpenShiftCheckException) as excinfo: + check.run(tmp=None, task_vars=task_vars) + assert "no thinpool usage data" in str(excinfo.value) @pytest.mark.parametrize('response,extra_words', [