Merge pull request #3649 from sosiouxme/20170313-fix-output-and-exclusions

Merged by openshift-bot

6 files changed, 295 insertions, 64 deletions

diff --git a/roles/openshift_health_checker/library/aos_version.py b/roles/openshift_health_checker/library/aos_version.py
index 191a4b107..a46589443 100755
--- a/roles/openshift_health_checker/library/aos_version.py
+++ b/roles/openshift_health_checker/library/aos_version.py
@@ -1,91 +1,199 @@
 #!/usr/bin/python
 # vim: expandtab:tabstop=4:shiftwidth=4
 '''
-Ansible module for determining if multiple versions of an OpenShift package are
-available, and if the version requested is available down to the given
-precision.
-
-Multiple versions available suggest that multiple repos are enabled for the
-different versions, which may cause installation problems.
+Ansible module for yum-based systems determining if multiple releases
+of an OpenShift package are available, and if the release requested
+(if any) is available down to the given precision.
+
+For Enterprise, multiple releases available suggest that multiple repos
+are enabled for the different releases, which may cause installation
+problems. With Origin, however, this is a normal state of affairs as
+all the releases are provided in a single repo with the expectation that
+only the latest can be installed.
+
+Code in the openshift_version role contains a lot of logic to pin down
+the exact package and image version to use and so does some validation
+of release availability already. Without duplicating all that, we would
+like the user to have a helpful error message if we detect things will
+not work out right. Note that if openshift_release is not specified in
+the inventory, the version comparison checks just pass.
+
+TODO: fail gracefully on non-yum systems (dnf in Fedora)
 '''
 
-import yum  # pylint: disable=import-error
-
 from ansible.module_utils.basic import AnsibleModule
 
+IMPORT_EXCEPTION = None
+try:
+    import yum  # pylint: disable=import-error
+except ImportError as err:
+    IMPORT_EXCEPTION = err  # in tox test env, yum import fails
+
 
-def main():  # pylint: disable=missing-docstring,too-many-branches
+class AosVersionException(Exception):
+    '''Base exception class for package version problems'''
+    def __init__(self, message, problem_pkgs=None):
+        Exception.__init__(self, message)
+        self.problem_pkgs = problem_pkgs
+
+
+def main():
+    '''Entrypoint for this Ansible module'''
     module = AnsibleModule(
         argument_spec=dict(
-            prefix=dict(required=True),  # atomic-openshift, origin, ...
-            version=dict(required=True),
+            requested_openshift_release=dict(type="str", default=''),
+            openshift_deployment_type=dict(required=True),
+            rpm_prefix=dict(required=True),  # atomic-openshift, origin, ...?
         ),
         supports_check_mode=True
     )
 
-    def bail(error):  # pylint: disable=missing-docstring
-        module.fail_json(msg=error)
-
-    rpm_prefix = module.params['prefix']
+    if IMPORT_EXCEPTION:
+        module.fail_json(msg="aos_version module could not import yum: %s" % IMPORT_EXCEPTION)
 
+    # determine the packages we will look for
+    rpm_prefix = module.params['rpm_prefix']
     if not rpm_prefix:
-        bail("prefix must not be empty")
-
-    yb = yum.YumBase()  # pylint: disable=invalid-name
-    yb.conf.disable_excludes = ["all"]  # assume the openshift excluder will be managed, ignore current state
-
-    # search for package versions available for aos pkgs
-    expected_pkgs = [
+        module.fail_json(msg="rpm_prefix must not be empty")
+    expected_pkgs = set([
         rpm_prefix,
         rpm_prefix + '-master',
         rpm_prefix + '-node',
-    ]
+    ])
+
+    # determine what level of precision the user specified for the openshift version.
+    # should look like a version string with possibly many segments e.g. "3.4.1":
+    requested_openshift_release = module.params['requested_openshift_release']
+
+    # get the list of packages available and complain if anything is wrong
+    try:
+        pkgs = _retrieve_available_packages(expected_pkgs)
+        if requested_openshift_release:
+            _check_precise_version_found(pkgs, expected_pkgs, requested_openshift_release)
+            _check_higher_version_found(pkgs, expected_pkgs, requested_openshift_release)
+        if module.params['openshift_deployment_type'] in ['openshift-enterprise']:
+            _check_multi_minor_release(pkgs, expected_pkgs)
+    except AosVersionException as excinfo:
+        module.fail_json(msg=str(excinfo))
+    module.exit_json(changed=False)
+
+
+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
+    # Excludes are then disabled during an install or upgrade, but
+    # this check will most likely be running outside either. When we
+    # 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 e:  # pylint: disable=invalid-name
+    except yum.Errors.PackageSackError as excinfo:
         # you only hit this if *none* of the packages are available
-        bail('Unable to find any OpenShift packages.\nCheck your subscription and repo settings.\n%s' % e)
+        raise AosVersionException('\n'.join([
+            'Unable to find any OpenShift packages.',
+            'Check your subscription and repo settings.',
+            str(excinfo),
+        ]))
+    return pkgs
 
-    # determine what level of precision we're expecting for the version
-    expected_version = module.params['version']
-    if expected_version.startswith('v'):  # v3.3 => 3.3
-        expected_version = expected_version[1:]
-    num_dots = expected_version.count('.')
 
-    pkgs_by_name_version = {}
+class PreciseVersionNotFound(AosVersionException):
+    '''Exception for reporting packages not available at given release'''
+    def __init__(self, requested_release, not_found):
+        msg = ['Not all of the required packages are available at requested version %s:' % requested_release]
+        msg += ['  ' + name for name in not_found]
+        msg += ['Please check your subscriptions and enabled repositories.']
+        AosVersionException.__init__(self, '\n'.join(msg), not_found)
+
+
+def _check_precise_version_found(pkgs, expected_pkgs, requested_openshift_release):
+    # see if any packages couldn't be found at requested release version
+    # we would like to verify that the latest available pkgs have however specific a version is given.
+    # so e.g. if there is a package version 3.4.1.5 the check passes; if only 3.4.0, it fails.
+
     pkgs_precise_version_found = {}
     for pkg in pkgs:
-        # get expected version precision
-        match_version = '.'.join(pkg.version.split('.')[:num_dots + 1])
-        if match_version == expected_version:
+        if pkg.name not in expected_pkgs:
+            continue
+        # does the version match, to the precision requested?
+        # and, is it strictly greater, at the precision requested?
+        match_version = '.'.join(pkg.version.split('.')[:requested_openshift_release.count('.') + 1])
+        if match_version == requested_openshift_release:
             pkgs_precise_version_found[pkg.name] = True
-        # get x.y version precision
-        minor_version = '.'.join(pkg.version.split('.')[:2])
-        if pkg.name not in pkgs_by_name_version:
-            pkgs_by_name_version[pkg.name] = {}
-        pkgs_by_name_version[pkg.name][minor_version] = True
 
-    # see if any packages couldn't be found at requested version
-    # see if any packages are available in more than one minor version
     not_found = []
-    multi_found = []
     for name in expected_pkgs:
         if name not in pkgs_precise_version_found:
             not_found.append(name)
+
+    if not_found:
+        raise PreciseVersionNotFound(requested_openshift_release, not_found)
+
+
+class FoundHigherVersion(AosVersionException):
+    '''Exception for reporting that a higher version than requested is available'''
+    def __init__(self, requested_release, higher_found):
+        msg = ['Some required package(s) are available at a version',
+               'that is higher than requested %s:' % requested_release]
+        msg += ['  ' + name for name in higher_found]
+        msg += ['This will prevent installing the version you requested.']
+        msg += ['Please check your enabled repositories or adjust openshift_release.']
+        AosVersionException.__init__(self, '\n'.join(msg), higher_found)
+
+
+def _check_higher_version_found(pkgs, expected_pkgs, requested_openshift_release):
+    req_release_arr = [int(segment) for segment in requested_openshift_release.split(".")]
+    # see if any packages are available in a version higher than requested
+    higher_version_for_pkg = {}
+    for pkg in pkgs:
+        if pkg.name not in expected_pkgs:
+            continue
+        version = [int(segment) for segment in pkg.version.split(".")]
+        too_high = version[:len(req_release_arr)] > req_release_arr
+        higher_than_seen = version > higher_version_for_pkg.get(pkg.name, [])
+        if too_high and higher_than_seen:
+            higher_version_for_pkg[pkg.name] = version
+
+    if higher_version_for_pkg:
+        higher_found = []
+        for name, version in higher_version_for_pkg.items():
+            higher_found.append(name + '-' + '.'.join(str(segment) for segment in version))
+        raise FoundHigherVersion(requested_openshift_release, higher_found)
+
+
+class FoundMultiRelease(AosVersionException):
+    '''Exception for reporting multiple minor releases found for same package'''
+    def __init__(self, multi_found):
+        msg = ['Multiple minor versions of these packages are available']
+        msg += ['  ' + name for name in multi_found]
+        msg += ["There should only be one OpenShift release repository enabled at a time."]
+        AosVersionException.__init__(self, '\n'.join(msg), multi_found)
+
+
+def _check_multi_minor_release(pkgs, expected_pkgs):
+    # see if any packages are available in more than one minor version
+    pkgs_by_name_version = {}
+    for pkg in pkgs:
+        # keep track of x.y (minor release) versions seen
+        minor_release = '.'.join(pkg.version.split('.')[:2])
+        if pkg.name not in pkgs_by_name_version:
+            pkgs_by_name_version[pkg.name] = {}
+        pkgs_by_name_version[pkg.name][minor_release] = True
+
+    multi_found = []
+    for name in expected_pkgs:
         if name in pkgs_by_name_version and len(pkgs_by_name_version[name]) > 1:
             multi_found.append(name)
-    if not_found:
-        msg = 'Not all of the required packages are available at requested version %s:\n' % expected_version
-        for name in not_found:
-            msg += '  %s\n' % name
-        bail(msg + 'Please check your subscriptions and enabled repositories.')
-    if multi_found:
-        msg = 'Multiple minor versions of these packages are available\n'
-        for name in multi_found:
-            msg += '  %s\n' % name
-        bail(msg + "There should only be one OpenShift version's repository enabled at a time.")
 
-    module.exit_json(changed=False)
+    if multi_found:
+        raise FoundMultiRelease(multi_found)
 
 
 if __name__ == '__main__':
diff --git a/roles/openshift_health_checker/meta/main.yml b/roles/openshift_health_checker/meta/main.yml
index 0bbeadd34..cd9b55902 100644
--- a/roles/openshift_health_checker/meta/main.yml
+++ b/roles/openshift_health_checker/meta/main.yml
@@ -1,3 +1,4 @@
 ---
 dependencies:
   - role: openshift_facts
+  - role: openshift_repos
diff --git a/roles/openshift_health_checker/openshift_checks/package_version.py b/roles/openshift_health_checker/openshift_checks/package_version.py
index 42193a1c6..cca2d8b75 100644
--- a/roles/openshift_health_checker/openshift_checks/package_version.py
+++ b/roles/openshift_health_checker/openshift_checks/package_version.py
@@ -10,11 +10,9 @@ class PackageVersion(NotContainerizedMixin, OpenShiftCheck):
     tags = ["preflight"]
 
     def run(self, tmp, task_vars):
-        rpm_prefix = get_var(task_vars, "openshift", "common", "service_type")
-        openshift_release = get_var(task_vars, "openshift_release")
-
         args = {
-            "prefix": rpm_prefix,
-            "version": openshift_release,
+            "requested_openshift_release": get_var(task_vars, "openshift_release", default=''),
+            "openshift_deployment_type": get_var(task_vars, "openshift_deployment_type"),
+            "rpm_prefix": get_var(task_vars, "openshift", "common", "service_type"),
         }
         return self.execute_module("aos_version", args, tmp, task_vars)
diff --git a/roles/openshift_health_checker/test/aos_version_test.py b/roles/openshift_health_checker/test/aos_version_test.py
new file mode 100644
index 000000000..39c86067a
--- /dev/null
+++ b/roles/openshift_health_checker/test/aos_version_test.py
@@ -0,0 +1,120 @@
+import pytest
+import aos_version
+
+from collections import namedtuple
+Package = namedtuple('Package', ['name', 'version'])
+
+expected_pkgs = set(['spam', 'eggs'])
+
+
+@pytest.mark.parametrize('pkgs, requested_release, expect_not_found', [
+    (
+        [],
+        '3.2.1',
+        expected_pkgs,  # none found
+    ),
+    (
+        [Package('spam', '3.2.1')],
+        '3.2',
+        ['eggs'],  # completely missing
+    ),
+    (
+        [Package('spam', '3.2.1'), Package('eggs', '3.3.2')],
+        '3.2',
+        ['eggs'],  # not the right version
+    ),
+    (
+        [Package('spam', '3.2.1'), Package('eggs', '3.2.1')],
+        '3.2',
+        [],  # all found
+    ),
+    (
+        [Package('spam', '3.2.1'), Package('eggs', '3.2.1.5')],
+        '3.2.1',
+        [],  # found with more specific version
+    ),
+    (
+        [Package('eggs', '1.2.3'), Package('eggs', '3.2.1.5')],
+        '3.2.1',
+        ['spam'],  # eggs found with multiple versions
+    ),
+])
+def test_check_pkgs_for_precise_version(pkgs, requested_release, expect_not_found):
+    if expect_not_found:
+        with pytest.raises(aos_version.PreciseVersionNotFound) as e:
+            aos_version._check_precise_version_found(pkgs, expected_pkgs, requested_release)
+        assert set(expect_not_found) == set(e.value.problem_pkgs)
+    else:
+        aos_version._check_precise_version_found(pkgs, expected_pkgs, requested_release)
+
+
+@pytest.mark.parametrize('pkgs, requested_release, expect_higher', [
+    (
+        [],
+        '3.2.1',
+        [],
+    ),
+    (
+        [Package('spam', '3.2.1')],
+        '3.2',
+        [],  # more precise but not strictly higher
+    ),
+    (
+        [Package('spam', '3.3')],
+        '3.2.1',
+        ['spam-3.3'],  # lower precision, but higher
+    ),
+    (
+        [Package('spam', '3.2.1'), Package('eggs', '3.3.2')],
+        '3.2',
+        ['eggs-3.3.2'],  # one too high
+    ),
+    (
+        [Package('eggs', '1.2.3'), Package('eggs', '3.2.1.5'), Package('eggs', '3.4')],
+        '3.2.1',
+        ['eggs-3.4'],  # multiple versions, one is higher
+    ),
+    (
+        [Package('eggs', '3.2.1'), Package('eggs', '3.4'), Package('eggs', '3.3')],
+        '3.2.1',
+        ['eggs-3.4'],  # multiple versions, two are higher
+    ),
+])
+def test_check_pkgs_for_greater_version(pkgs, requested_release, expect_higher):
+    if expect_higher:
+        with pytest.raises(aos_version.FoundHigherVersion) as e:
+            aos_version._check_higher_version_found(pkgs, expected_pkgs, requested_release)
+        assert set(expect_higher) == set(e.value.problem_pkgs)
+    else:
+        aos_version._check_higher_version_found(pkgs, expected_pkgs, requested_release)
+
+
+@pytest.mark.parametrize('pkgs, expect_to_flag_pkgs', [
+    (
+        [],
+        [],
+    ),
+    (
+        [Package('spam', '3.2.1')],
+        [],
+    ),
+    (
+        [Package('spam', '3.2.1'), Package('eggs', '3.2.2')],
+        [],
+    ),
+    (
+        [Package('spam', '3.2.1'), Package('spam', '3.3.2')],
+        ['spam'],
+    ),
+    (
+        [Package('eggs', '1.2.3'), Package('eggs', '3.2.1.5'), Package('eggs', '3.4')],
+        ['eggs'],
+    ),
+])
+def test_check_pkgs_for_multi_release(pkgs, expect_to_flag_pkgs):
+    if expect_to_flag_pkgs:
+        with pytest.raises(aos_version.FoundMultiRelease) as e:
+            aos_version._check_multi_minor_release(pkgs, expected_pkgs)
+        assert set(expect_to_flag_pkgs) == set(e.value.problem_pkgs)
+    else:
+        aos_version._check_multi_minor_release(pkgs, expected_pkgs)
diff --git a/roles/openshift_health_checker/test/conftest.py b/roles/openshift_health_checker/test/conftest.py
index d16401260..3cbd65507 100644
--- a/roles/openshift_health_checker/test/conftest.py
+++ b/roles/openshift_health_checker/test/conftest.py
@@ -6,5 +6,6 @@ import sys
 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, 'action_plugins'),
+    os.path.join(openshift_health_checker_path, 'library'),
 ]
diff --git a/roles/openshift_health_checker/test/package_version_test.py b/roles/openshift_health_checker/test/package_version_test.py
index cc1d263bc..c6889ee9b 100644
--- a/roles/openshift_health_checker/test/package_version_test.py
+++ b/roles/openshift_health_checker/test/package_version_test.py
@@ -4,16 +4,19 @@ from openshift_checks.package_version import PackageVersion
 def test_package_version():
     task_vars = dict(
         openshift=dict(common=dict(service_type='origin')),
-        openshift_release='v3.5',
+        openshift_release='3.5',
+        openshift_deployment_type='origin',
     )
     return_value = object()
 
     def execute_module(module_name=None, module_args=None, tmp=None, task_vars=None):
         assert module_name == 'aos_version'
-        assert 'prefix' in module_args
-        assert 'version' in module_args
-        assert module_args['prefix'] == task_vars['openshift']['common']['service_type']
-        assert module_args['version'] == task_vars['openshift_release']
+        assert 'requested_openshift_release' in module_args
+        assert 'openshift_deployment_type' in module_args
+        assert 'rpm_prefix' in module_args
+        assert module_args['requested_openshift_release'] == task_vars['openshift_release']
+        assert module_args['openshift_deployment_type'] == task_vars['openshift_deployment_type']
+        assert module_args['rpm_prefix'] == task_vars['openshift']['common']['service_type']
         return return_value
 
     check = PackageVersion(execute_module=execute_module)