pre-install checks: add more during byo install

Add the docker and RPM checks to the list that run at install time.
They can be disabled the same as the existing ones.

Removed cockpit-kubernetes RPM requirement as it no longer is.
Fixed up docker_image_availability to handle oreg_url and other nuances.
Switched to using the openshift_image_tag that's set by openshift_version for
both component and infrastructure images.
Fixed a bug where execute_module was being called with incorrect
positional arg "tmp" as a dict which caused errors down the call stack.

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