diff options
Diffstat (limited to 'roles/openshift_health_checker')
24 files changed, 587 insertions, 304 deletions
diff --git a/roles/openshift_health_checker/action_plugins/openshift_health_check.py b/roles/openshift_health_checker/action_plugins/openshift_health_check.py index 05e53333d..8d35db6b5 100644 --- a/roles/openshift_health_checker/action_plugins/openshift_health_check.py +++ b/roles/openshift_health_checker/action_plugins/openshift_health_check.py @@ -1,76 +1,74 @@ """ Ansible action plugin to execute health checks in OpenShift clusters. """ -# pylint: disable=wrong-import-position,missing-docstring,invalid-name import sys import os +import traceback from collections import defaultdict +from ansible.plugins.action import ActionBase +from ansible.module_utils.six import string_types + try: from __main__ import display except ImportError: + # pylint: disable=ungrouped-imports; this is the standard way how to import + # the default display object in Ansible action plugins. from ansible.utils.display import Display display = Display() -from ansible.plugins.action import ActionBase -from ansible.module_utils.six import string_types - # Augment sys.path so that we can import checks from a directory relative to # this callback plugin. sys.path.insert(1, os.path.dirname(os.path.dirname(__file__))) +# pylint: disable=wrong-import-position; the import statement must come after +# the manipulation of sys.path. from openshift_checks import OpenShiftCheck, OpenShiftCheckException, load_checks # noqa: E402 class ActionModule(ActionBase): + """Action plugin to execute health checks.""" def run(self, tmp=None, task_vars=None): result = super(ActionModule, self).run(tmp, task_vars) task_vars = task_vars or {} - # vars are not supportably available in the callback plugin, - # so record any it will need in the result. + # callback plugins cannot read Ansible vars, but we would like + # zz_failure_summary to have access to certain values. We do so by + # storing the information we need in the result. result['playbook_context'] = task_vars.get('r_openshift_health_checker_playbook_context') - if "openshift" not in task_vars: - result["failed"] = True - result["msg"] = "'openshift' is undefined, did 'openshift_facts' run?" - return result - try: known_checks = self.load_known_checks(tmp, task_vars) args = self._task.args requested_checks = normalize(args.get('checks', [])) + + if not requested_checks: + result['failed'] = True + result['msg'] = list_known_checks(known_checks) + return result + resolved_checks = resolve_checks(requested_checks, known_checks.values()) - except OpenShiftCheckException as e: + except OpenShiftCheckException as exc: result["failed"] = True - result["msg"] = str(e) + result["msg"] = str(exc) + return result + + if "openshift" not in task_vars: + result["failed"] = True + result["msg"] = "'openshift' is undefined, did 'openshift_facts' run?" return result result["checks"] = check_results = {} user_disabled_checks = normalize(task_vars.get('openshift_disable_check', [])) - for check_name in resolved_checks: - display.banner("CHECK [{} : {}]".format(check_name, task_vars["ansible_host"])) - check = known_checks[check_name] - - if not check.is_active(): - r = dict(skipped=True, skipped_reason="Not active for this host") - elif check_name in user_disabled_checks: - r = dict(skipped=True, skipped_reason="Disabled by user request") - else: - try: - r = check.run() - except OpenShiftCheckException as e: - r = dict( - failed=True, - msg=str(e), - ) - + for name in resolved_checks: + display.banner("CHECK [{} : {}]".format(name, task_vars["ansible_host"])) + check = known_checks[name] + check_results[name] = run_check(name, check, user_disabled_checks) if check.changed: - r["changed"] = True - check_results[check_name] = r + check_results[name]["changed"] = True result["changed"] = any(r.get("changed") for r in check_results.values()) if any(r.get("failed") for r in check_results.values()): @@ -80,22 +78,55 @@ class ActionModule(ActionBase): return result def load_known_checks(self, tmp, task_vars): + """Find all existing checks and return a mapping of names to instances.""" load_checks() known_checks = {} for cls in OpenShiftCheck.subclasses(): - check_name = cls.name - if check_name in known_checks: - other_cls = known_checks[check_name].__class__ + name = cls.name + if name in known_checks: + other_cls = known_checks[name].__class__ raise OpenShiftCheckException( - "non-unique check name '{}' in: '{}.{}' and '{}.{}'".format( - check_name, - cls.__module__, cls.__name__, - other_cls.__module__, other_cls.__name__)) - known_checks[check_name] = cls(execute_module=self._execute_module, tmp=tmp, task_vars=task_vars) + "duplicate check name '{}' in: '{}' and '{}'" + "".format(name, full_class_name(cls), full_class_name(other_cls)) + ) + known_checks[name] = cls(execute_module=self._execute_module, tmp=tmp, task_vars=task_vars) return known_checks +def list_known_checks(known_checks): + """Return text listing the existing checks and tags.""" + # TODO: we could include a description of each check by taking it from a + # check class attribute (e.g., __doc__) when building the message below. + msg = ( + 'This playbook is meant to run health checks, but no checks were ' + 'requested. Set the `openshift_checks` variable to a comma-separated ' + 'list of check names or a YAML list. Available checks:\n {}' + ).format('\n '.join(sorted(known_checks))) + + tags = describe_tags(known_checks.values()) + + msg += ( + '\n\nTags can be used as a shortcut to select multiple ' + 'checks. Available tags and the checks they select:\n {}' + ).format('\n '.join(tags)) + + return msg + + +def describe_tags(check_classes): + """Return a sorted list of strings describing tags and the checks they include.""" + tag_checks = defaultdict(list) + for cls in check_classes: + for tag in cls.tags: + tag_checks[tag].append(cls.name) + tags = [ + '@{} = {}'.format(tag, ','.join(sorted(checks))) + for tag, checks in tag_checks.items() + ] + return sorted(tags) + + def resolve_checks(names, all_checks): """Returns a set of resolved check names. @@ -123,6 +154,12 @@ def resolve_checks(names, all_checks): if unknown_tag_names: msg.append('Unknown tag names: {}.'.format(', '.join(sorted(unknown_tag_names)))) msg.append('Make sure there is no typo in the playbook and no files are missing.') + # TODO: implement a "Did you mean ...?" when the input is similar to a + # valid check or tag. + msg.append('Known checks:') + msg.append(' {}'.format('\n '.join(sorted(known_check_names)))) + msg.append('Known tags:') + msg.append(' {}'.format('\n '.join(describe_tags(all_checks)))) raise OpenShiftCheckException('\n'.join(msg)) tag_to_checks = defaultdict(set) @@ -146,3 +183,32 @@ def normalize(checks): if isinstance(checks, string_types): checks = checks.split(',') return [name.strip() for name in checks if name.strip()] + + +def run_check(name, check, user_disabled_checks): + """Run a single check if enabled and return a result dict.""" + if name in user_disabled_checks: + return dict(skipped=True, skipped_reason="Disabled by user request") + + # pylint: disable=broad-except; capturing exceptions broadly is intentional, + # to isolate arbitrary failures in one check from others. + try: + is_active = check.is_active() + except Exception as exc: + reason = "Could not determine if check should be run, exception: {}".format(exc) + return dict(skipped=True, skipped_reason=reason, exception=traceback.format_exc()) + + if not is_active: + return dict(skipped=True, skipped_reason="Not active for this host") + + try: + return check.run() + except OpenShiftCheckException as exc: + return dict(failed=True, msg=str(exc)) + except Exception as exc: + return dict(failed=True, msg=str(exc), exception=traceback.format_exc()) + + +def full_class_name(cls): + """Return the name of a class prefixed with its module name.""" + return '{}.{}'.format(cls.__module__, cls.__name__) diff --git a/roles/openshift_health_checker/callback_plugins/zz_failure_summary.py b/roles/openshift_health_checker/callback_plugins/zz_failure_summary.py index d10200719..349655966 100644 --- a/roles/openshift_health_checker/callback_plugins/zz_failure_summary.py +++ b/roles/openshift_health_checker/callback_plugins/zz_failure_summary.py @@ -1,161 +1,223 @@ -""" -Ansible callback plugin to give a nicely formatted summary of failures. -""" +"""Ansible callback plugin to print a nicely formatted summary of failures. -# Reason: In several locations below we disable pylint protected-access -# for Ansible objects that do not give us any public way -# to access the full details we need to report check failures. -# Status: disabled permanently or until Ansible object has a public API. -# This does leave the code more likely to be broken by future Ansible changes. +The file / module name is prefixed with `zz_` to make this plugin be loaded last +by Ansible, thus making its output the last thing that users see. +""" -from pprint import pformat +from collections import defaultdict +import traceback from ansible.plugins.callback import CallbackBase from ansible import constants as C from ansible.utils.color import stringc +FAILED_NO_MSG = u'Failed without returning a message.' + + class CallbackModule(CallbackBase): - """ - This callback plugin stores task results and summarizes failures. - The file name is prefixed with `zz_` to make this plugin be loaded last by - Ansible, thus making its output the last thing that users see. - """ + """This callback plugin stores task results and summarizes failures.""" CALLBACK_VERSION = 2.0 CALLBACK_TYPE = 'aggregate' CALLBACK_NAME = 'failure_summary' CALLBACK_NEEDS_WHITELIST = False - _playbook_file = None def __init__(self): super(CallbackModule, self).__init__() self.__failures = [] + self.__playbook_file = '' def v2_playbook_on_start(self, playbook): super(CallbackModule, self).v2_playbook_on_start(playbook) - # re: playbook attrs see top comment # pylint: disable=protected-access - self._playbook_file = playbook._file_name + # pylint: disable=protected-access; Ansible gives us no public API to + # get the file name of the current playbook from a callback plugin. + self.__playbook_file = playbook._file_name def v2_runner_on_failed(self, result, ignore_errors=False): super(CallbackModule, self).v2_runner_on_failed(result, ignore_errors) if not ignore_errors: - self.__failures.append(dict(result=result, ignore_errors=ignore_errors)) + self.__failures.append(result) def v2_playbook_on_stats(self, stats): super(CallbackModule, self).v2_playbook_on_stats(stats) - if self.__failures: - self._print_failure_details(self.__failures) - - def _print_failure_details(self, failures): - """Print a summary of failed tasks or checks.""" - self._display.display(u'\nFailure summary:\n') - - width = len(str(len(failures))) - initial_indent_format = u' {{:>{width}}}. '.format(width=width) - initial_indent_len = len(initial_indent_format.format(0)) - subsequent_indent = u' ' * initial_indent_len - subsequent_extra_indent = u' ' * (initial_indent_len + 10) - - for i, failure in enumerate(failures, 1): - entries = _format_failure(failure) - self._display.display(u'\n{}{}'.format(initial_indent_format.format(i), entries[0])) - for entry in entries[1:]: - entry = entry.replace(u'\n', u'\n' + subsequent_extra_indent) - indented = u'{}{}'.format(subsequent_indent, entry) - self._display.display(indented) - - failed_checks = set() - playbook_context = None - # re: result attrs see top comment # pylint: disable=protected-access - for failure in failures: - # Get context from check task result since callback plugins cannot access task vars. - # NOTE: thus context is not known unless checks run. Failures prior to checks running - # don't have playbook_context in the results. But we only use it now when checks fail. - playbook_context = playbook_context or failure['result']._result.get('playbook_context') - failed_checks.update( - name - for name, result in failure['result']._result.get('checks', {}).items() - if result.get('failed') - ) - if failed_checks: - self._print_check_failure_summary(failed_checks, playbook_context) - - def _print_check_failure_summary(self, failed_checks, context): - checks = ','.join(sorted(failed_checks)) - # The purpose of specifying context is to vary the output depending on what the user was - # expecting to happen (based on which playbook they ran). The only use currently is to - # vary the message depending on whether the user was deliberately running checks or was - # trying to install/upgrade and checks are just included. Other use cases may arise. - summary = ( # default to explaining what checks are in the first place - '\n' - 'The execution of "{playbook}"\n' - 'includes checks designed to fail early if the requirements\n' - 'of the playbook are not met. One or more of these checks\n' - 'failed. To disregard these results, you may choose to\n' - 'disable failing checks by setting an Ansible variable:\n\n' - ' openshift_disable_check={checks}\n\n' - 'Failing check names are shown in the failure details above.\n' - 'Some checks may be configurable by variables if your requirements\n' - 'are different from the defaults; consult check documentation.\n' - 'Variables can be set in the inventory or passed on the\n' - 'command line using the -e flag to ansible-playbook.\n\n' - ).format(playbook=self._playbook_file, checks=checks) - if context in ['pre-install', 'health']: - summary = ( # user was expecting to run checks, less explanation needed - '\n' - 'You may choose to configure or disable failing checks by\n' - 'setting Ansible variables. To disable those above:\n\n' - ' openshift_disable_check={checks}\n\n' - 'Consult check documentation for configurable variables.\n' - 'Variables can be set in the inventory or passed on the\n' - 'command line using the -e flag to ansible-playbook.\n\n' - ).format(checks=checks) - self._display.display(summary) - - -# re: result attrs see top comment # pylint: disable=protected-access -def _format_failure(failure): + # pylint: disable=broad-except; capturing exceptions broadly is + # intentional, to isolate arbitrary failures in this callback plugin. + try: + if self.__failures: + self._display.display(failure_summary(self.__failures, self.__playbook_file)) + except Exception: + msg = stringc( + u'An error happened while generating a summary of failures:\n' + u'{}'.format(traceback.format_exc()), C.COLOR_WARN) + self._display.v(msg) + + +def failure_summary(failures, playbook): + """Return a summary of failed tasks, including details on health checks.""" + if not failures: + return u'' + + # NOTE: because we don't have access to task_vars from callback plugins, we + # store the playbook context in the task result when the + # openshift_health_check action plugin is used, and we use this context to + # customize the error message. + # pylint: disable=protected-access; Ansible gives us no sufficient public + # API on TaskResult objects. + context = next(( + context for context in + (failure._result.get('playbook_context') for failure in failures) + if context + ), None) + + failures = [failure_to_dict(failure) for failure in failures] + failures = deduplicate_failures(failures) + + summary = [u'', u'', u'Failure summary:', u''] + + width = len(str(len(failures))) + initial_indent_format = u' {{:>{width}}}. '.format(width=width) + initial_indent_len = len(initial_indent_format.format(0)) + subsequent_indent = u' ' * initial_indent_len + subsequent_extra_indent = u' ' * (initial_indent_len + 10) + + for i, failure in enumerate(failures, 1): + entries = format_failure(failure) + summary.append(u'\n{}{}'.format(initial_indent_format.format(i), entries[0])) + for entry in entries[1:]: + entry = entry.replace(u'\n', u'\n' + subsequent_extra_indent) + indented = u'{}{}'.format(subsequent_indent, entry) + summary.append(indented) + + failed_checks = set() + for failure in failures: + failed_checks.update(name for name, message in failure['checks']) + if failed_checks: + summary.append(check_failure_footer(failed_checks, context, playbook)) + + return u'\n'.join(summary) + + +def failure_to_dict(failed_task_result): + """Extract information out of a failed TaskResult into a dict. + + The intent is to transform a TaskResult object into something easier to + manipulate. TaskResult is ansible.executor.task_result.TaskResult. + """ + # pylint: disable=protected-access; Ansible gives us no sufficient public + # API on TaskResult objects. + _result = failed_task_result._result + return { + 'host': failed_task_result._host.get_name(), + 'play': play_name(failed_task_result._task), + 'task': failed_task_result.task_name, + 'msg': _result.get('msg', FAILED_NO_MSG), + 'checks': tuple( + (name, result.get('msg', FAILED_NO_MSG)) + for name, result in sorted(_result.get('checks', {}).items()) + if result.get('failed') + ), + } + + +def play_name(obj): + """Given a task or block, return the name of its parent play. + + This is loosely inspired by ansible.playbook.base.Base.dump_me. + """ + # pylint: disable=protected-access; Ansible gives us no sufficient public + # API to implement this. + if not obj: + return '' + if hasattr(obj, '_play'): + return obj._play.get_name() + return play_name(getattr(obj, '_parent')) + + +def deduplicate_failures(failures): + """Group together similar failures from different hosts. + + Returns a new list of failures such that identical failures from different + hosts are grouped together in a single entry. The relative order of failures + is preserved. + """ + groups = defaultdict(list) + for failure in failures: + group_key = tuple(sorted((key, value) for key, value in failure.items() if key != 'host')) + groups[group_key].append(failure) + result = [] + for failure in failures: + group_key = tuple(sorted((key, value) for key, value in failure.items() if key != 'host')) + if group_key not in groups: + continue + failure['host'] = tuple(sorted(g_failure['host'] for g_failure in groups.pop(group_key))) + result.append(failure) + return result + + +def format_failure(failure): """Return a list of pretty-formatted text entries describing a failure, including relevant information about it. Expect that the list of text entries will be joined by a newline separator when output to the user.""" - result = failure['result'] - host = result._host.get_name() - play = _get_play(result._task) - if play: - play = play.get_name() - task = result._task.get_name() - msg = result._result.get('msg', u'???') + host = u', '.join(failure['host']) + play = failure['play'] + task = failure['task'] + msg = failure['msg'] + checks = failure['checks'] fields = ( - (u'Host', host), + (u'Hosts', host), (u'Play', play), (u'Task', task), (u'Message', stringc(msg, C.COLOR_ERROR)), ) - if 'checks' in result._result: - fields += ((u'Details', _format_failed_checks(result._result['checks'])),) + if checks: + fields += ((u'Details', format_failed_checks(checks)),) row_format = '{:10}{}' return [row_format.format(header + u':', body) for header, body in fields] -def _format_failed_checks(checks): +def format_failed_checks(checks): """Return pretty-formatted text describing checks that failed.""" - failed_check_msgs = [] - for check, body in checks.items(): - if body.get('failed', False): # only show the failed checks - msg = body.get('msg', u"Failed without returning a message") - failed_check_msgs.append('check "%s":\n%s' % (check, msg)) - if failed_check_msgs: - return stringc("\n\n".join(failed_check_msgs), C.COLOR_ERROR) - else: # something failed but no checks will admit to it, so dump everything - return stringc(pformat(checks), C.COLOR_ERROR) - - -# This is inspired by ansible.playbook.base.Base.dump_me. -# re: play/task/block attrs see top comment # pylint: disable=protected-access -def _get_play(obj): - """Given a task or block, recursively try to find its parent play.""" - if hasattr(obj, '_play'): - return obj._play - if getattr(obj, '_parent'): - return _get_play(obj._parent) + messages = [] + for name, message in checks: + messages.append(u'check "{}":\n{}'.format(name, message)) + return stringc(u'\n\n'.join(messages), C.COLOR_ERROR) + + +def check_failure_footer(failed_checks, context, playbook): + """Return a textual explanation about checks depending on context. + + The purpose of specifying context is to vary the output depending on what + the user was expecting to happen (based on which playbook they ran). The + only use currently is to vary the message depending on whether the user was + deliberately running checks or was trying to install/upgrade and checks are + just included. Other use cases may arise. + """ + checks = ','.join(sorted(failed_checks)) + summary = [u''] + if context in ['pre-install', 'health', 'adhoc']: + # User was expecting to run checks, less explanation needed. + summary.extend([ + u'You may configure or disable checks by setting Ansible ' + u'variables. To disable those above, set:', + u' openshift_disable_check={checks}'.format(checks=checks), + u'Consult check documentation for configurable variables.', + ]) + else: + # User may not be familiar with the checks, explain what checks are in + # the first place. + summary.extend([ + u'The execution of "{playbook}" includes checks designed to fail ' + u'early if the requirements of the playbook are not met. One or ' + u'more of these checks failed. To disregard these results,' + u'explicitly disable checks by setting an Ansible variable:'.format(playbook=playbook), + u' openshift_disable_check={checks}'.format(checks=checks), + u'Failing check names are shown in the failure details above. ' + u'Some checks may be configurable by variables if your requirements ' + u'are different from the defaults; consult check documentation.', + ]) + summary.append( + u'Variables can be set in the inventory or passed on the command line ' + u'using the -e flag to ansible-playbook.' + ) + return u'\n'.join(summary) diff --git a/roles/openshift_health_checker/library/aos_version.py b/roles/openshift_health_checker/library/aos_version.py index f9babebb9..c8769b511 100644 --- a/roles/openshift_health_checker/library/aos_version.py +++ b/roles/openshift_health_checker/library/aos_version.py @@ -24,11 +24,19 @@ from ansible.module_utils.basic import AnsibleModule # Python 3, we use six for cross compatibility in this module alone: from ansible.module_utils.six import string_types -IMPORT_EXCEPTION = None +YUM_IMPORT_EXCEPTION = None +DNF_IMPORT_EXCEPTION = None +PKG_MGR = None try: import yum # pylint: disable=import-error + PKG_MGR = "yum" except ImportError as err: - IMPORT_EXCEPTION = err + YUM_IMPORT_EXCEPTION = err +try: + import dnf # pylint: disable=import-error + PKG_MGR = "dnf" +except ImportError as err: + DNF_IMPORT_EXCEPTION = err class AosVersionException(Exception): @@ -47,8 +55,11 @@ def main(): supports_check_mode=True ) - if IMPORT_EXCEPTION: - module.fail_json(msg="aos_version module could not import yum: %s" % IMPORT_EXCEPTION) + if YUM_IMPORT_EXCEPTION and DNF_IMPORT_EXCEPTION: + module.fail_json( + msg="aos_version module could not import yum or dnf: %s %s" % + (YUM_IMPORT_EXCEPTION, DNF_IMPORT_EXCEPTION) + ) # determine the packages we will look for package_list = module.params['package_list'] @@ -83,9 +94,6 @@ def _to_dict(pkg_list): def _retrieve_available_packages(expected_pkgs): - # search for package versions available for openshift pkgs - yb = yum.YumBase() # pylint: disable=invalid-name - # The openshift excluder prevents unintended updates to openshift # packages by setting yum excludes on those packages. See: # https://wiki.centos.org/SpecialInterestGroup/PaaS/OpenShift-Origin-Control-Updates @@ -94,17 +102,41 @@ def _retrieve_available_packages(expected_pkgs): # attempt to determine what packages are available via yum they may # be excluded. So, for our purposes here, disable excludes to see # what will really be available during an install or upgrade. - yb.conf.disable_excludes = ['all'] - try: - pkgs = yb.pkgSack.returnPackages(patterns=expected_pkgs) - except yum.Errors.PackageSackError as excinfo: - # you only hit this if *none* of the packages are available - raise AosVersionException('\n'.join([ - 'Unable to find any OpenShift packages.', - 'Check your subscription and repo settings.', - str(excinfo), - ])) + if PKG_MGR == "yum": + # search for package versions available for openshift pkgs + yb = yum.YumBase() # pylint: disable=invalid-name + + yb.conf.disable_excludes = ['all'] + + try: + pkgs = yb.pkgSack.returnPackages(patterns=expected_pkgs) + except yum.Errors.PackageSackError as excinfo: + # you only hit this if *none* of the packages are available + raise AosVersionException('\n'.join([ + 'Unable to find any OpenShift packages.', + 'Check your subscription and repo settings.', + str(excinfo), + ])) + elif PKG_MGR == "dnf": + dbase = dnf.Base() # pyling: disable=invalid-name + + dbase.conf.disable_excludes = ['all'] + dbase.read_all_repos() + dbase.fill_sack(load_system_repo=False, load_available_repos=True) + + dquery = dbase.sack.query() + aquery = dquery.available() + + pkgs = list(aquery.filter(name=expected_pkgs)) + + if not pkgs: + # pkgs list is empty, raise because no expected packages found + raise AosVersionException('\n'.join([ + 'Unable to find any OpenShift packages.', + 'Check your subscription and repo settings.', + ])) + return pkgs diff --git a/roles/openshift_health_checker/library/rpm_version.py b/roles/openshift_health_checker/library/rpm_version.py index 8ea223055..c24fbba3b 100644 --- a/roles/openshift_health_checker/library/rpm_version.py +++ b/roles/openshift_health_checker/library/rpm_version.py @@ -4,6 +4,7 @@ Ansible module for rpm-based systems determining existing package version inform """ from ansible.module_utils.basic import AnsibleModule +from ansible.module_utils.six import string_types IMPORT_EXCEPTION = None try: @@ -82,11 +83,16 @@ def _check_pkg_versions(found_pkgs_dict, expected_pkgs_dict): continue found_versions = [_parse_version(version) for version in found_pkgs_dict[pkg_name]] - expected_version = _parse_version(pkg["version"]) - if expected_version not in found_versions: + + if isinstance(pkg["version"], string_types): + expected_versions = [_parse_version(pkg["version"])] + else: + expected_versions = [_parse_version(version) for version in pkg["version"]] + + if not set(expected_versions) & set(found_versions): invalid_pkg_versions[pkg_name] = { "found_versions": found_versions, - "required_version": expected_version, + "required_versions": expected_versions, } if not_found_pkgs: @@ -106,7 +112,7 @@ def _check_pkg_versions(found_pkgs_dict, expected_pkgs_dict): "The following packages were found to be installed with an incorrect version: {}".format('\n'.join([ " \n{}\n Required version: {}\n Found versions: {}".format( pkg_name, - pkg["required_version"], + ', '.join(pkg["required_versions"]), ', '.join([version for version in pkg["found_versions"]])) for pkg_name, pkg in invalid_pkg_versions.items() ])) diff --git a/roles/openshift_health_checker/meta/main.yml b/roles/openshift_health_checker/meta/main.yml index ed97d539c..bc8e7bdcf 100644 --- a/roles/openshift_health_checker/meta/main.yml +++ b/roles/openshift_health_checker/meta/main.yml @@ -1 +1,3 @@ --- +dependencies: +- role: openshift_facts diff --git a/roles/openshift_health_checker/openshift_checks/__init__.py b/roles/openshift_health_checker/openshift_checks/__init__.py index 29faa9c5b..02ee1d0f9 100644 --- a/roles/openshift_health_checker/openshift_checks/__init__.py +++ b/roles/openshift_health_checker/openshift_checks/__init__.py @@ -10,6 +10,7 @@ from importlib import import_module from ansible.module_utils import six from ansible.module_utils.six.moves import reduce # pylint: disable=import-error,redefined-builtin +from ansible.plugins.filter.core import to_bool as ansible_to_bool class OpenShiftCheckException(Exception): @@ -119,16 +120,59 @@ class OpenShiftCheck(object): Ansible task_vars structures are Python dicts, often mapping strings to other dicts. This helper makes it easier to get a nested value, raising - OpenShiftCheckException when a key is not found or returning a default value - provided as a keyword argument. + OpenShiftCheckException when a key is not found. + + Keyword args: + default: + On missing key, return this as default value instead of raising exception. + convert: + Supply a function to apply to normalize the value before returning it. + None is the default (return as-is). + This function should raise ValueError if the user has provided a value + that cannot be converted, or OpenShiftCheckException if some other + problem needs to be described to the user. """ + if len(keys) == 1: + keys = keys[0].split(".") + try: value = reduce(operator.getitem, keys, self.task_vars) except (KeyError, TypeError): - if "default" in kwargs: - return kwargs["default"] - raise OpenShiftCheckException("'{}' is undefined".format(".".join(map(str, keys)))) - return value + if "default" not in kwargs: + raise OpenShiftCheckException( + "This check expects the '{}' inventory variable to be defined\n" + "in order to proceed, but it is undefined. There may be a bug\n" + "in Ansible, the checks, or their dependencies." + "".format(".".join(map(str, keys))) + ) + value = kwargs["default"] + + convert = kwargs.get("convert", None) + try: + if convert is None: + return value + elif convert is bool: # interpret bool as Ansible does, instead of python truthiness + return ansible_to_bool(value) + else: + return convert(value) + + except ValueError as error: # user error in specifying value + raise OpenShiftCheckException( + 'Cannot convert inventory variable to expected type:\n' + ' "{var}={value}"\n' + '{error}'.format(var=".".join(keys), value=value, error=error) + ) + + except OpenShiftCheckException: # some other check-specific problem + raise + + except Exception as error: # probably a bug in the function + raise OpenShiftCheckException( + 'There is a bug in this check. While trying to convert variable \n' + ' "{var}={value}"\n' + 'the given converter cannot be used or failed unexpectedly:\n' + '{error}'.format(var=".".join(keys), value=value, error=error) + ) @staticmethod def get_major_minor_version(openshift_image_tag): @@ -153,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..f302fd14b 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, {}) @@ -119,30 +115,22 @@ class DiskAvailability(OpenShiftCheck): return { 'failed': True, - 'msg': ( - 'Available disk space in "{}" ({:.1f} GB) ' - 'is below minimum recommended ({:.1f} GB)' - ).format(path, free_gb, recommended_gb) + 'msg': msg, } 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_image_availability.py b/roles/openshift_health_checker/openshift_checks/docker_image_availability.py index 85a922f86..857a80c74 100644 --- a/roles/openshift_health_checker/openshift_checks/docker_image_availability.py +++ b/roles/openshift_health_checker/openshift_checks/docker_image_availability.py @@ -168,7 +168,10 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck): registries = [registry] for registry in registries: - args = {"_raw_params": "skopeo inspect --tls-verify=false docker://{}/{}".format(registry, image)} + args = { + "_raw_params": "timeout 10 skopeo inspect --tls-verify=false " + "docker://{}/{}".format(registry, image) + } result = self.execute_module("command", args) if result.get("rc", 0) == 0 and not result.get("failed"): return True 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_traffic.py b/roles/openshift_health_checker/openshift_checks/etcd_traffic.py index cc1b14d8a..b4c8957e9 100644 --- a/roles/openshift_health_checker/openshift_checks/etcd_traffic.py +++ b/roles/openshift_health_checker/openshift_checks/etcd_traffic.py @@ -14,8 +14,8 @@ class EtcdTraffic(OpenShiftCheck): group_names = self.get_var("group_names", default=[]) valid_group_names = "etcd" in group_names - version = self.get_var("openshift", "common", "short_version") - valid_version = version in ("3.4", "3.5", "1.4", "1.5") + version = self.get_major_minor_version(self.get_var("openshift_image_tag")) + valid_version = version in ((3, 4), (3, 5)) return super(EtcdTraffic, self).is_active() and valid_group_names and valid_version 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/openshift_checks/logging/logging.py b/roles/openshift_health_checker/openshift_checks/logging/logging.py index 3b7c39760..ecd8adb64 100644 --- a/roles/openshift_health_checker/openshift_checks/logging/logging.py +++ b/roles/openshift_health_checker/openshift_checks/logging/logging.py @@ -27,7 +27,7 @@ class LoggingCheck(OpenShiftCheck): name = "logging" def is_active(self): - logging_deployed = self.get_var("openshift_hosted_logging_deploy", default=False) + logging_deployed = self.get_var("openshift_hosted_logging_deploy", convert=bool, default=False) return logging_deployed and super(LoggingCheck, self).is_active() and self.is_first_master() def is_first_master(self): diff --git a/roles/openshift_health_checker/openshift_checks/ovs_version.py b/roles/openshift_health_checker/openshift_checks/ovs_version.py index d5e55bc25..363c12def 100644 --- a/roles/openshift_health_checker/openshift_checks/ovs_version.py +++ b/roles/openshift_health_checker/openshift_checks/ovs_version.py @@ -16,8 +16,8 @@ class OvsVersion(NotContainerizedMixin, OpenShiftCheck): tags = ["health"] openshift_to_ovs_version = { - "3.6": "2.6", - "3.5": "2.6", + "3.6": ["2.6", "2.7"], + "3.5": ["2.6", "2.7"], "3.4": "2.4", } diff --git a/roles/openshift_health_checker/test/action_plugin_test.py b/roles/openshift_health_checker/test/action_plugin_test.py index f5161d6f5..c109ebd24 100644 --- a/roles/openshift_health_checker/test/action_plugin_test.py +++ b/roles/openshift_health_checker/test/action_plugin_test.py @@ -80,7 +80,8 @@ def skipped(result): None, {}, ]) -def test_action_plugin_missing_openshift_facts(plugin, task_vars): +def test_action_plugin_missing_openshift_facts(plugin, task_vars, monkeypatch): + monkeypatch.setattr('openshift_health_check.resolve_checks', lambda *args: ['fake_check']) result = plugin.run(tmp=None, task_vars=task_vars) assert failed(result, msg_has=['openshift_facts']) @@ -94,7 +95,7 @@ def test_action_plugin_cannot_load_checks_with_the_same_name(plugin, task_vars, result = plugin.run(tmp=None, task_vars=task_vars) - assert failed(result, msg_has=['unique', 'duplicate_name', 'FakeCheck']) + assert failed(result, msg_has=['duplicate', 'duplicate_name', 'FakeCheck']) def test_action_plugin_skip_non_active_checks(plugin, task_vars, monkeypatch): @@ -217,24 +218,21 @@ def test_resolve_checks_ok(names, all_checks, expected): assert resolve_checks(names, all_checks) == expected -@pytest.mark.parametrize('names,all_checks,words_in_exception,words_not_in_exception', [ +@pytest.mark.parametrize('names,all_checks,words_in_exception', [ ( ['testA', 'testB'], [], ['check', 'name', 'testA', 'testB'], - ['tag', 'group', '@'], ), ( ['@group'], [], ['tag', 'name', 'group'], - ['check', '@'], ), ( ['testA', 'testB', '@group'], [], ['check', 'name', 'testA', 'testB', 'tag', 'group'], - ['@'], ), ( ['testA', 'testB', '@group'], @@ -244,13 +242,10 @@ def test_resolve_checks_ok(names, all_checks, expected): fake_check('from_group_2', ['preflight', 'group']), ], ['check', 'name', 'testA', 'testB'], - ['tag', 'group', '@'], ), ]) -def test_resolve_checks_failure(names, all_checks, words_in_exception, words_not_in_exception): +def test_resolve_checks_failure(names, all_checks, words_in_exception): with pytest.raises(Exception) as excinfo: resolve_checks(names, all_checks) for word in words_in_exception: assert word in str(excinfo.value) - for word in words_not_in_exception: - assert word not in str(excinfo.value) diff --git a/roles/openshift_health_checker/test/conftest.py b/roles/openshift_health_checker/test/conftest.py index 3cbd65507..244a1f0fa 100644 --- a/roles/openshift_health_checker/test/conftest.py +++ b/roles/openshift_health_checker/test/conftest.py @@ -7,5 +7,6 @@ openshift_health_checker_path = os.path.dirname(os.path.dirname(__file__)) sys.path[1:1] = [ openshift_health_checker_path, os.path.join(openshift_health_checker_path, 'action_plugins'), + os.path.join(openshift_health_checker_path, 'callback_plugins'), os.path.join(openshift_health_checker_path, 'library'), ] 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_traffic_test.py b/roles/openshift_health_checker/test/etcd_traffic_test.py index f4316c423..fae3e578d 100644 --- a/roles/openshift_health_checker/test/etcd_traffic_test.py +++ b/roles/openshift_health_checker/test/etcd_traffic_test.py @@ -8,7 +8,7 @@ from openshift_checks.etcd_traffic import EtcdTraffic (['masters'], "3.6", False), (['nodes'], "3.4", False), (['etcd'], "3.4", True), - (['etcd'], "3.5", True), + (['etcd'], "1.5", True), (['etcd'], "3.1", False), (['masters', 'nodes'], "3.5", False), (['masters', 'etcd'], "3.5", True), @@ -17,9 +17,7 @@ from openshift_checks.etcd_traffic import EtcdTraffic def test_is_active(group_names, version, is_active): task_vars = dict( group_names=group_names, - openshift=dict( - common=dict(short_version=version), - ), + openshift_image_tag=version, ) assert EtcdTraffic(task_vars=task_vars).is_active() == is_active 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) diff --git a/roles/openshift_health_checker/test/openshift_check_test.py b/roles/openshift_health_checker/test/openshift_check_test.py index 43aa875f4..789784c77 100644 --- a/roles/openshift_health_checker/test/openshift_check_test.py +++ b/roles/openshift_health_checker/test/openshift_check_test.py @@ -81,6 +81,7 @@ def dummy_check(task_vars): @pytest.mark.parametrize("keys,expected", [ (("foo",), 42), (("bar", "baz"), "openshift"), + (("bar.baz",), "openshift"), ]) def test_get_var_ok(task_vars, keys, expected): assert dummy_check(task_vars).get_var(*keys) == expected @@ -94,3 +95,24 @@ def test_get_var_error(task_vars, missing_keys): def test_get_var_default(task_vars, missing_keys): default = object() assert dummy_check(task_vars).get_var(*missing_keys, default=default) == default + + +@pytest.mark.parametrize("keys, convert, expected", [ + (("foo",), str, "42"), + (("foo",), float, 42.0), + (("bar", "baz"), bool, False), +]) +def test_get_var_convert(task_vars, keys, convert, expected): + assert dummy_check(task_vars).get_var(*keys, convert=convert) == expected + + +@pytest.mark.parametrize("keys, convert", [ + (("bar", "baz"), int), + (("bar.baz"), float), + (("foo"), "bogus"), + (("foo"), lambda a, b: 1), + (("foo"), lambda a: 1 / 0), +]) +def test_get_var_convert_error(task_vars, keys, convert): + with pytest.raises(OpenShiftCheckException): + dummy_check(task_vars).get_var(*keys, convert=convert) diff --git a/roles/openshift_health_checker/test/ovs_version_test.py b/roles/openshift_health_checker/test/ovs_version_test.py index b6acef5a6..e1bf29d2a 100644 --- a/roles/openshift_health_checker/test/ovs_version_test.py +++ b/roles/openshift_health_checker/test/ovs_version_test.py @@ -38,8 +38,8 @@ def test_invalid_openshift_release_format(): @pytest.mark.parametrize('openshift_release,expected_ovs_version', [ - ("3.5", "2.6"), - ("3.6", "2.6"), + ("3.5", ["2.6", "2.7"]), + ("3.6", ["2.6", "2.7"]), ("3.4", "2.4"), ("3.3", "2.4"), ("1.0", "2.4"), diff --git a/roles/openshift_health_checker/test/rpm_version_test.py b/roles/openshift_health_checker/test/rpm_version_test.py index 2f09ef965..2c1bcf876 100644 --- a/roles/openshift_health_checker/test/rpm_version_test.py +++ b/roles/openshift_health_checker/test/rpm_version_test.py @@ -49,7 +49,7 @@ def test_check_pkg_found(pkgs, expect_not_found): }, { "eggs": { - "required_version": "3.2", + "required_versions": ["3.2"], "found_versions": ["3.3"], } }, # not the right version @@ -61,11 +61,11 @@ def test_check_pkg_found(pkgs, expect_not_found): }, { "eggs": { - "required_version": "3.2", + "required_versions": ["3.2"], "found_versions": ["3.3", "1.2"], }, "spam": { - "required_version": "3.2", + "required_versions": ["3.2"], "found_versions": ["3.1", "3.3"], } }, # not the right version diff --git a/roles/openshift_health_checker/test/zz_failure_summary_test.py b/roles/openshift_health_checker/test/zz_failure_summary_test.py new file mode 100644 index 000000000..0fc258133 --- /dev/null +++ b/roles/openshift_health_checker/test/zz_failure_summary_test.py @@ -0,0 +1,70 @@ +from zz_failure_summary import deduplicate_failures + +import pytest + + +@pytest.mark.parametrize('failures,deduplicated', [ + ( + [ + { + 'host': 'master1', + 'msg': 'One or more checks failed', + }, + ], + [ + { + 'host': ('master1',), + 'msg': 'One or more checks failed', + }, + ], + ), + ( + [ + { + 'host': 'master1', + 'msg': 'One or more checks failed', + }, + { + 'host': 'node1', + 'msg': 'One or more checks failed', + }, + ], + [ + { + 'host': ('master1', 'node1'), + 'msg': 'One or more checks failed', + }, + ], + ), + ( + [ + { + 'host': 'node1', + 'msg': 'One or more checks failed', + 'checks': (('test_check', 'error message'),), + }, + { + 'host': 'master2', + 'msg': 'Some error happened', + }, + { + 'host': 'master1', + 'msg': 'One or more checks failed', + 'checks': (('test_check', 'error message'),), + }, + ], + [ + { + 'host': ('master1', 'node1'), + 'msg': 'One or more checks failed', + 'checks': (('test_check', 'error message'),), + }, + { + 'host': ('master2',), + 'msg': 'Some error happened', + }, + ], + ), +]) +def test_deduplicate_failures(failures, deduplicated): + assert deduplicate_failures(failures) == deduplicated |