Merge pull request #4397 from sosiouxme/20170608-include-more-install-checks

Merged by openshift-bot

10 files changed, 189 insertions, 91 deletions

diff --git a/playbooks/byo/openshift-cluster/config.yml b/playbooks/byo/openshift-cluster/config.yml
index fd4a9eb26..2372a5322 100644
--- a/playbooks/byo/openshift-cluster/config.yml
+++ b/playbooks/byo/openshift-cluster/config.yml
@@ -15,6 +15,11 @@
       checks:
       - disk_availability
       - memory_availability
+      - package_availability
+      - package_update
+      - package_version
+      - docker_image_availability
+      - docker_storage
 
 - include: ../../common/openshift-cluster/std_include.yml
   tags:
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 27e6fe383..60aacf715 100644
--- a/roles/openshift_health_checker/openshift_checks/docker_image_availability.py
+++ b/roles/openshift_health_checker/openshift_checks/docker_image_availability.py
@@ -1,8 +1,24 @@
-# pylint: disable=missing-docstring
+"""Check that required Docker images are available."""
+
 from openshift_checks import OpenShiftCheck, get_var
 from openshift_checks.mixins import DockerHostMixin
 
 
+NODE_IMAGE_SUFFIXES = ["haproxy-router", "docker-registry", "deployer", "pod"]
+DEPLOYMENT_IMAGE_INFO = {
+    "origin": {
+        "namespace": "openshift",
+        "name": "origin",
+        "registry_console_image": "cockpit/kubernetes",
+    },
+    "openshift-enterprise": {
+        "namespace": "openshift3",
+        "name": "ose",
+        "registry_console_image": "registry.access.redhat.com/openshift3/registry-console",
+    },
+}
+
+
 class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
     """Check that required Docker images are available.
 
@@ -13,25 +29,13 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
 
     name = "docker_image_availability"
     tags = ["preflight"]
-
     dependencies = ["skopeo", "python-docker-py"]
 
-    deployment_image_info = {
-        "origin": {
-            "namespace": "openshift",
-            "name": "origin",
-        },
-        "openshift-enterprise": {
-            "namespace": "openshift3",
-            "name": "ose",
-        },
-    }
-
     @classmethod
     def is_active(cls, task_vars):
         """Skip hosts with unsupported deployment types."""
         deployment_type = get_var(task_vars, "openshift_deployment_type")
-        has_valid_deployment_type = deployment_type in cls.deployment_image_info
+        has_valid_deployment_type = deployment_type in DEPLOYMENT_IMAGE_INFO
 
         return super(DockerImageAvailability, cls).is_active(task_vars) and has_valid_deployment_type
 
@@ -70,51 +74,55 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
 
         return {"changed": changed}
 
-    def required_images(self, task_vars):
-        deployment_type = get_var(task_vars, "openshift_deployment_type")
-        image_info = self.deployment_image_info[deployment_type]
-
-        openshift_release = get_var(task_vars, "openshift_release", default="latest")
-        openshift_image_tag = get_var(task_vars, "openshift_image_tag")
-        is_containerized = get_var(task_vars, "openshift", "common", "is_containerized")
-
-        images = set(self.required_docker_images(
-            image_info["namespace"],
-            image_info["name"],
-            ["registry-console"] if "enterprise" in deployment_type else [],  # include enterprise-only image names
-            openshift_release,
-            is_containerized,
-        ))
-
-        # append images with qualified image tags to our list of required images.
-        # these are images with a (v0.0.0.0) tag, rather than a standard release
-        # format tag (v0.0). We want to check this set in both containerized and
-        # non-containerized installations.
-        images.update(
-            self.required_qualified_docker_images(
-                image_info["namespace"],
-                image_info["name"],
-                openshift_image_tag,
-            ),
-        )
-
-        return images
-
     @staticmethod
-    def required_docker_images(namespace, name, additional_image_names, version, is_containerized):
-        if is_containerized:
-            return ["{}/{}:{}".format(namespace, name, version)] if name else []
-
-        # include additional non-containerized images specific to the current deployment type
-        return ["{}/{}:{}".format(namespace, img_name, version) for img_name in additional_image_names]
-
-    @staticmethod
-    def required_qualified_docker_images(namespace, name, version):
-        # pylint: disable=invalid-name
-        return [
-            "{}/{}-{}:{}".format(namespace, name, suffix, version)
-            for suffix in ["haproxy-router", "docker-registry", "deployer", "pod"]
-        ]
+    def required_images(task_vars):
+        """
+        Determine which images we expect to need for this host.
+        Returns: a set of required images like 'openshift/origin:v3.6'
+
+        The thorny issue of determining the image names from the variables is under consideration
+        via https://github.com/openshift/openshift-ansible/issues/4415
+
+        For now we operate as follows:
+        * For containerized components (master, node, ...) we look at the deployment type and
+          use openshift/origin or openshift3/ose as the base for those component images. The
+          version is openshift_image_tag as determined by the openshift_version role.
+        * For OpenShift-managed infrastructure (router, registry...) we use oreg_url if
+          it is defined; otherwise we again use the base that depends on the deployment type.
+        Registry is not included in constructed images. It may be in oreg_url or etcd image.
+        """
+        required = set()
+        deployment_type = get_var(task_vars, "openshift_deployment_type")
+        host_groups = get_var(task_vars, "group_names")
+        image_tag = get_var(task_vars, "openshift_image_tag")
+        image_info = DEPLOYMENT_IMAGE_INFO[deployment_type]
+        if not image_info:
+            return required
+
+        # template for images that run on top of OpenShift
+        image_url = "{}/{}-{}:{}".format(image_info["namespace"], image_info["name"], "${component}", "${version}")
+        image_url = get_var(task_vars, "oreg_url", default="") or image_url
+        if 'nodes' in host_groups:
+            for suffix in NODE_IMAGE_SUFFIXES:
+                required.add(image_url.replace("${component}", suffix).replace("${version}", image_tag))
+            # The registry-console is for some reason not prefixed with ose- like the other components.
+            # Nor is it versioned the same, so just look for latest.
+            # Also a completely different name is used for Origin.
+            required.add(image_info["registry_console_image"])
+
+        # images for containerized components
+        if get_var(task_vars, "openshift", "common", "is_containerized"):
+            components = set()
+            if 'nodes' in host_groups:
+                components.update(["node", "openvswitch"])
+            if 'masters' in host_groups:  # name is "origin" or "ose"
+                components.add(image_info["name"])
+            for component in components:
+                required.add("{}/{}:{}".format(image_info["namespace"], component, image_tag))
+            if 'etcd' in host_groups:  # special case, note it is the same for origin/enterprise
+                required.add("registry.access.redhat.com/rhel7/etcd")  # and no image tag
+
+        return required
 
     def local_images(self, images, task_vars):
         """Filter a list of images and return those available locally."""
@@ -124,7 +132,8 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
         ]
 
     def is_image_local(self, image, task_vars):
-        result = self.module_executor("docker_image_facts", {"name": image}, task_vars)
+        """Check if image is already in local docker index."""
+        result = self.execute_module("docker_image_facts", {"name": image}, task_vars=task_vars)
         if result.get("failed", False):
             return False
 
@@ -132,6 +141,7 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
 
     @staticmethod
     def known_docker_registries(task_vars):
+        """Build a list of docker registries available according to inventory vars."""
         docker_facts = get_var(task_vars, "openshift", "docker")
         regs = set(docker_facts["additional_registries"])
 
@@ -147,17 +157,21 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
         """Inspect existing images using Skopeo and return all images successfully inspected."""
         return [
             image for image in images
-            if any(self.is_available_skopeo_image(image, registry, task_vars) for registry in registries)
+            if self.is_available_skopeo_image(image, registries, task_vars)
         ]
 
-    def is_available_skopeo_image(self, image, registry, task_vars):
-        """Uses Skopeo to determine if required image exists in a given registry."""
+    def is_available_skopeo_image(self, image, registries, task_vars):
+        """Use Skopeo to determine if required image exists in known registry(s)."""
+
+        # if image does already includes a registry, just use that
+        if image.count("/") > 1:
+            registry, image = image.split("/", 1)
+            registries = [registry]
 
-        cmd_str = "skopeo inspect docker://{registry}/{image}".format(
-            registry=registry,
-            image=image,
-        )
+        for registry in registries:
+            args = {"_raw_params": "skopeo inspect docker://{}/{}".format(registry, image)}
+            result = self.execute_module("command", args, task_vars=task_vars)
+            if result.get("rc", 0) == 0 and not result.get("failed"):
+                return True
 
-        args = {"_raw_params": cmd_str}
-        result = self.module_executor("command", args, task_vars)
-        return not result.get("failed", False) and result.get("rc", 0) == 0
+        return False
diff --git a/roles/openshift_health_checker/openshift_checks/docker_storage.py b/roles/openshift_health_checker/openshift_checks/docker_storage.py
index 7f1751b36..2bd615457 100644
--- a/roles/openshift_health_checker/openshift_checks/docker_storage.py
+++ b/roles/openshift_health_checker/openshift_checks/docker_storage.py
@@ -34,7 +34,7 @@ class DockerStorage(DockerHostMixin, OpenShiftCheck):
             }
 
         # attempt to get the docker info hash from the API
-        info = self.execute_module("docker_info", {}, task_vars)
+        info = self.execute_module("docker_info", {}, task_vars=task_vars)
         if info.get("failed"):
             return {"failed": True, "changed": changed,
                     "msg": "Failed to query Docker API. Is docker running on this host?"}
@@ -146,7 +146,7 @@ class DockerStorage(DockerHostMixin, OpenShiftCheck):
         vgs_cmd = "/sbin/vgs --noheadings -o vg_free --select vg_name=" + vg_name
         # should return free space like "  12.00g" if the VG exists; empty if it does not
 
-        ret = self.execute_module("command", {"_raw_params": vgs_cmd}, task_vars)
+        ret = self.execute_module("command", {"_raw_params": vgs_cmd}, task_vars=task_vars)
         if ret.get("failed") or ret.get("rc", 0) != 0:
             raise OpenShiftCheckException(
                 "Is LVM installed? Failed to run /sbin/vgs "
diff --git a/roles/openshift_health_checker/openshift_checks/mixins.py b/roles/openshift_health_checker/openshift_checks/mixins.py
index 7f3d78cc4..2cb2e21aa 100644
--- a/roles/openshift_health_checker/openshift_checks/mixins.py
+++ b/roles/openshift_health_checker/openshift_checks/mixins.py
@@ -40,8 +40,11 @@ class DockerHostMixin(object):
 
         # NOTE: we would use the "package" module but it's actually an action plugin
         # and it's not clear how to invoke one of those. This is about the same anyway:
-        pkg_manager = get_var(task_vars, "ansible_pkg_mgr", default="yum")
-        result = self.module_executor(pkg_manager, {"name": self.dependencies, "state": "present"}, task_vars)
+        result = self.execute_module(
+            get_var(task_vars, "ansible_pkg_mgr", default="yum"),
+            {"name": self.dependencies, "state": "present"},
+            task_vars=task_vars,
+        )
         msg = result.get("msg", "")
         if result.get("failed"):
             if "No package matching" in msg:
diff --git a/roles/openshift_health_checker/openshift_checks/ovs_version.py b/roles/openshift_health_checker/openshift_checks/ovs_version.py
index 1e45ae3af..2dd045f1f 100644
--- a/roles/openshift_health_checker/openshift_checks/ovs_version.py
+++ b/roles/openshift_health_checker/openshift_checks/ovs_version.py
@@ -43,7 +43,7 @@ class OvsVersion(NotContainerizedMixin, OpenShiftCheck):
                 },
             ],
         }
-        return self.execute_module("rpm_version", args, task_vars)
+        return self.execute_module("rpm_version", args, task_vars=task_vars)
 
     def get_required_ovs_version(self, task_vars):
         """Return the correct Open vSwitch version for the current OpenShift version"""
diff --git a/roles/openshift_health_checker/openshift_checks/package_availability.py b/roles/openshift_health_checker/openshift_checks/package_availability.py
index a7eb720fd..e87567fe6 100644
--- a/roles/openshift_health_checker/openshift_checks/package_availability.py
+++ b/roles/openshift_health_checker/openshift_checks/package_availability.py
@@ -25,7 +25,7 @@ class PackageAvailability(NotContainerizedMixin, OpenShiftCheck):
             packages.update(self.node_packages(rpm_prefix))
 
         args = {"packages": sorted(set(packages))}
-        return self.execute_module("check_yum_update", args, tmp, task_vars)
+        return self.execute_module("check_yum_update", args, tmp=tmp, task_vars=task_vars)
 
     @staticmethod
     def master_packages(rpm_prefix):
@@ -36,7 +36,6 @@ class PackageAvailability(NotContainerizedMixin, OpenShiftCheck):
             "bash-completion",
             "cockpit-bridge",
             "cockpit-docker",
-            "cockpit-kubernetes",
             "cockpit-shell",
             "cockpit-ws",
             "etcd",
diff --git a/roles/openshift_health_checker/openshift_checks/package_update.py b/roles/openshift_health_checker/openshift_checks/package_update.py
index fd0c0a755..f432380c6 100644
--- a/roles/openshift_health_checker/openshift_checks/package_update.py
+++ b/roles/openshift_health_checker/openshift_checks/package_update.py
@@ -11,4 +11,4 @@ class PackageUpdate(NotContainerizedMixin, OpenShiftCheck):
 
     def run(self, tmp, task_vars):
         args = {"packages": []}
-        return self.execute_module("check_yum_update", args, tmp, task_vars)
+        return self.execute_module("check_yum_update", args, tmp=tmp, task_vars=task_vars)
diff --git a/roles/openshift_health_checker/openshift_checks/package_version.py b/roles/openshift_health_checker/openshift_checks/package_version.py
index 2e737818b..6a76bb93d 100644
--- a/roles/openshift_health_checker/openshift_checks/package_version.py
+++ b/roles/openshift_health_checker/openshift_checks/package_version.py
@@ -71,7 +71,7 @@ class PackageVersion(NotContainerizedMixin, OpenShiftCheck):
             ],
         }
 
-        return self.execute_module("aos_version", args, tmp, task_vars)
+        return self.execute_module("aos_version", args, tmp=tmp, task_vars=task_vars)
 
     def get_required_ovs_version(self, task_vars):
         """Return the correct Open vSwitch version for the current OpenShift version.
diff --git a/roles/openshift_health_checker/test/docker_image_availability_test.py b/roles/openshift_health_checker/test/docker_image_availability_test.py
index 197c65f51..0a7c0f8d3 100644
--- a/roles/openshift_health_checker/test/docker_image_availability_test.py
+++ b/roles/openshift_health_checker/test/docker_image_availability_test.py
@@ -31,15 +31,15 @@ def test_is_active(deployment_type, is_containerized, group_names, expect_active
     (False, True),
 ])
 def test_all_images_available_locally(is_containerized, is_atomic):
-    def execute_module(module_name, args, task_vars):
+    def execute_module(module_name, module_args, task_vars):
         if module_name == "yum":
             return {"changed": True}
 
         assert module_name == "docker_image_facts"
-        assert 'name' in args
-        assert args['name']
+        assert 'name' in module_args
+        assert module_args['name']
         return {
-            'images': [args['name']],
+            'images': [module_args['name']],
         }
 
     result = DockerImageAvailability(execute_module=execute_module).run(tmp=None, task_vars=dict(
@@ -52,8 +52,8 @@ def test_all_images_available_locally(is_containerized, is_atomic):
             docker=dict(additional_registries=["docker.io"]),
         ),
         openshift_deployment_type='origin',
-        openshift_release='v3.4',
         openshift_image_tag='3.4',
+        group_names=['nodes', 'masters'],
     ))
 
     assert not result.get('failed', False)
@@ -64,7 +64,7 @@ def test_all_images_available_locally(is_containerized, is_atomic):
     True,
 ])
 def test_all_images_available_remotely(available_locally):
-    def execute_module(module_name, args, task_vars):
+    def execute_module(module_name, module_args, task_vars):
         if module_name == 'docker_image_facts':
             return {'images': [], 'failed': available_locally}
         return {'changed': False}
@@ -79,8 +79,8 @@ def test_all_images_available_remotely(available_locally):
             docker=dict(additional_registries=["docker.io", "registry.access.redhat.com"]),
         ),
         openshift_deployment_type='origin',
-        openshift_release='3.4',
         openshift_image_tag='v3.4',
+        group_names=['nodes', 'masters'],
     ))
 
     assert not result.get('failed', False)
@@ -108,8 +108,8 @@ def test_all_images_unavailable():
             docker=dict(additional_registries=["docker.io"]),
         ),
         openshift_deployment_type="openshift-enterprise",
-        openshift_release=None,
-        openshift_image_tag='latest'
+        openshift_image_tag='latest',
+        group_names=['nodes', 'masters'],
     ))
 
     assert actual['failed']
@@ -147,8 +147,8 @@ def test_skopeo_update_failure(message, extra_words):
             docker=dict(additional_registries=["unknown.io"]),
         ),
         openshift_deployment_type="openshift-enterprise",
-        openshift_release='',
         openshift_image_tag='',
+        group_names=['nodes', 'masters'],
     ))
 
     assert actual["failed"]
@@ -177,8 +177,85 @@ def test_registry_availability(deployment_type, registries):
             docker=dict(additional_registries=registries),
         ),
         openshift_deployment_type=deployment_type,
-        openshift_release='',
         openshift_image_tag='',
+        group_names=['nodes', 'masters'],
     ))
 
     assert not actual.get("failed", False)
+
+
+@pytest.mark.parametrize("deployment_type, is_containerized, groups, oreg_url, expected", [
+    (  # standard set of stuff required on nodes
+        "origin", False, ['nodes'], None,
+        set([
+            'openshift/origin-pod:vtest',
+            'openshift/origin-deployer:vtest',
+            'openshift/origin-docker-registry:vtest',
+            'openshift/origin-haproxy-router:vtest',
+            'cockpit/kubernetes',  # origin version of registry-console
+        ])
+    ),
+    (  # set a different URL for images
+        "origin", False, ['nodes'], 'foo.io/openshift/origin-${component}:${version}',
+        set([
+            'foo.io/openshift/origin-pod:vtest',
+            'foo.io/openshift/origin-deployer:vtest',
+            'foo.io/openshift/origin-docker-registry:vtest',
+            'foo.io/openshift/origin-haproxy-router:vtest',
+            'cockpit/kubernetes',  # AFAICS this is not built from the URL
+        ])
+    ),
+    (
+        "origin", True, ['nodes', 'masters', 'etcd'], None,
+        set([
+            # images running on top of openshift
+            'openshift/origin-pod:vtest',
+            'openshift/origin-deployer:vtest',
+            'openshift/origin-docker-registry:vtest',
+            'openshift/origin-haproxy-router:vtest',
+            'cockpit/kubernetes',
+            # containerized component images
+            'openshift/origin:vtest',
+            'openshift/node:vtest',
+            'openshift/openvswitch:vtest',
+            'registry.access.redhat.com/rhel7/etcd',
+        ])
+    ),
+    (  # enterprise images
+        "openshift-enterprise", True, ['nodes'], 'foo.io/openshift3/ose-${component}:f13ac45',
+        set([
+            'foo.io/openshift3/ose-pod:f13ac45',
+            'foo.io/openshift3/ose-deployer:f13ac45',
+            'foo.io/openshift3/ose-docker-registry:f13ac45',
+            'foo.io/openshift3/ose-haproxy-router:f13ac45',
+            # registry-console is not constructed/versioned the same as the others.
+            'registry.access.redhat.com/openshift3/registry-console',
+            # containerized images aren't built from oreg_url
+            'openshift3/node:vtest',
+            'openshift3/openvswitch:vtest',
+        ])
+    ),
+    (
+        "openshift-enterprise", True, ['etcd', 'lb'], 'foo.io/openshift3/ose-${component}:f13ac45',
+        set([
+            'registry.access.redhat.com/rhel7/etcd',
+            # lb does not yet come in a containerized version
+        ])
+    ),
+
+])
+def test_required_images(deployment_type, is_containerized, groups, oreg_url, expected):
+    task_vars = dict(
+        openshift=dict(
+            common=dict(
+                is_containerized=is_containerized,
+                is_atomic=False,
+            ),
+        ),
+        openshift_deployment_type=deployment_type,
+        group_names=groups,
+        oreg_url=oreg_url,
+        openshift_image_tag='vtest',
+    )
+
+    assert expected == DockerImageAvailability("DUMMY").required_images(task_vars)
diff --git a/roles/openshift_health_checker/test/docker_storage_test.py b/roles/openshift_health_checker/test/docker_storage_test.py
index 292a323db..876614b1d 100644
--- a/roles/openshift_health_checker/test/docker_storage_test.py
+++ b/roles/openshift_health_checker/test/docker_storage_test.py
@@ -77,7 +77,7 @@ non_atomic_task_vars = {"openshift": {"common": {"is_atomic": False}}}
     ),
 ])
 def test_check_storage_driver(docker_info, failed, expect_msg):
-    def execute_module(module_name, args, tmp=None, task_vars=None):
+    def execute_module(module_name, module_args, tmp=None, task_vars=None):
         if module_name == "yum":
             return {}
         if module_name != "docker_info":
@@ -187,7 +187,7 @@ def test_dm_usage(task_vars, driver_status, vg_free, success, expect_msg):
     )
 ])
 def test_vg_free(pool, command_returns, raises, returns):
-    def execute_module(module_name, args, tmp=None, task_vars=None):
+    def execute_module(module_name, module_args, tmp=None, task_vars=None):
         if module_name != "command":
             raise ValueError("not expecting module " + module_name)
         return command_returns