docker_image_availability: probe registry connectivity

Probe whether the host has connectivity to the registry before trying to
inspect it for images, and remember the result. Also if later inspection
fails due to timeout, mark registry as unreachable. Note in failure
output if any registries were unreachable.

Registry order should match what is configured into docker now as well.

Fixes bug 1480195
https://bugzilla.redhat.com/show_bug.cgi?id=1480195

2 files changed, 132 insertions, 122 deletions

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 458e51d87..4418fd7d5 100644
--- a/roles/openshift_health_checker/openshift_checks/docker_image_availability.py
+++ b/roles/openshift_health_checker/openshift_checks/docker_image_availability.py
@@ -32,7 +32,12 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
     # we use python-docker-py to check local docker for images, and skopeo
     # to look for images available remotely without waiting to pull them.
     dependencies = ["python-docker-py", "skopeo"]
-    skopeo_img_check_command = "timeout 10 skopeo inspect --tls-verify=false"
+    skopeo_img_check_command = "timeout 10 skopeo inspect --tls-verify=false docker://{registry}/{image}"
+
+    def __init__(self, *args, **kwargs):
+        super(DockerImageAvailability, self).__init__(*args, **kwargs)
+        # record whether we could reach a registry or not (and remember results)
+        self.reachable_registries = {}
 
     def is_active(self):
         """Skip hosts with unsupported deployment types."""
@@ -64,15 +69,21 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
         unavailable_images = set(missing_images) - set(available_images)
 
         if unavailable_images:
-            return {
-                "failed": True,
-                "msg": (
-                    "One or more required Docker images are not available:\n    {}\n"
-                    "Configured registries: {}\n"
-                    "Checked by: {}"
-                ).format(",\n    ".join(sorted(unavailable_images)), ", ".join(registries),
-                         self.skopeo_img_check_command),
-            }
+            registries = [
+                reg if self.reachable_registries.get(reg, True) else reg + " (unreachable)"
+                for reg in registries
+            ]
+            msg = (
+                "One or more required Docker images are not available:\n    {}\n"
+                "Configured registries: {}\n"
+                "Checked by: {}"
+            ).format(
+                ",\n    ".join(sorted(unavailable_images)),
+                ", ".join(registries),
+                self.skopeo_img_check_command
+            )
+
+            return dict(failed=True, msg=msg)
 
         return {}
 
@@ -136,23 +147,19 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
     def is_image_local(self, image):
         """Check if image is already in local docker index."""
         result = self.execute_module("docker_image_facts", {"name": image})
-        if result.get("failed", False):
-            return False
-
-        return bool(result.get("images", []))
+        return bool(result.get("images")) and not result.get("failed")
 
     def known_docker_registries(self):
         """Build a list of docker registries available according to inventory vars."""
-        docker_facts = self.get_var("openshift", "docker")
-        regs = set(docker_facts["additional_registries"])
+        regs = list(self.get_var("openshift.docker.additional_registries"))
 
         deployment_type = self.get_var("openshift_deployment_type")
-        if deployment_type == "origin":
-            regs.update(["docker.io"])
-        elif "enterprise" in deployment_type:
-            regs.update(["registry.access.redhat.com"])
+        if deployment_type == "origin" and "docker.io" not in regs:
+            regs.append("docker.io")
+        elif "enterprise" in deployment_type and "registry.access.redhat.com" not in regs:
+            regs.append("registry.access.redhat.com")
 
-        return list(regs)
+        return regs
 
     def available_images(self, images, default_registries):
         """Search remotely for images. Returns: list of images found."""
@@ -165,15 +172,35 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
         """Use Skopeo to determine if required image exists in known registry(s)."""
         registries = default_registries
 
-        # if image already includes a registry, only use that
+        # If image already includes a registry, only use that.
+        # NOTE: This logic would incorrectly identify images that do not use a namespace, e.g.
+        # registry.access.redhat.com/rhel7 as if the registry were a namespace.
+        # It's not clear that there's any way to distinguish them, but fortunately
+        # the current set of images all look like [registry/]namespace/name[:version].
         if image.count("/") > 1:
             registry, image = image.split("/", 1)
             registries = [registry]
 
         for registry in registries:
-            args = {"_raw_params": self.skopeo_img_check_command + " docker://{}/{}".format(registry, image)}
+            if registry not in self.reachable_registries:
+                self.reachable_registries[registry] = self.connect_to_registry(registry)
+            if not self.reachable_registries[registry]:
+                continue
+
+            args = {"_raw_params": self.skopeo_img_check_command.format(registry=registry, image=image)}
             result = self.execute_module_with_retries("command", args)
             if result.get("rc", 0) == 0 and not result.get("failed"):
                 return True
+            if result.get("rc") == 124:  # RC 124 == timed out; mark unreachable
+                self.reachable_registries[registry] = False
 
         return False
+
+    def connect_to_registry(self, registry):
+        """Use ansible wait_for module to test connectivity from host to registry. Returns bool."""
+        # test a simple TCP connection
+        host, _, port = registry.partition(":")
+        port = port or 443
+        args = dict(host=host, port=port, state="started", timeout=30)
+        result = self.execute_module("wait_for", args)
+        return result.get("rc", 0) == 0 and not result.get("failed")
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 b51fdeac4..6a7c16c7e 100644
--- a/roles/openshift_health_checker/test/docker_image_availability_test.py
+++ b/roles/openshift_health_checker/test/docker_image_availability_test.py
@@ -3,6 +3,23 @@ import pytest
 from openshift_checks.docker_image_availability import DockerImageAvailability
 
 
+@pytest.fixture()
+def task_vars():
+    return dict(
+        openshift=dict(
+            common=dict(
+                service_type='origin',
+                is_containerized=False,
+                is_atomic=False,
+            ),
+            docker=dict(),
+        ),
+        openshift_deployment_type='origin',
+        openshift_image_tag='',
+        group_names=['nodes', 'masters'],
+    )
+
+
 @pytest.mark.parametrize('deployment_type, is_containerized, group_names, expect_active', [
     ("origin", True, [], True),
     ("openshift-enterprise", True, [], True),
@@ -15,12 +32,10 @@ from openshift_checks.docker_image_availability import DockerImageAvailability
     ("origin", False, ["nodes", "masters"], True),
     ("openshift-enterprise", False, ["etcd"], False),
 ])
-def test_is_active(deployment_type, is_containerized, group_names, expect_active):
-    task_vars = dict(
-        openshift=dict(common=dict(is_containerized=is_containerized)),
-        openshift_deployment_type=deployment_type,
-        group_names=group_names,
-    )
+def test_is_active(task_vars, deployment_type, is_containerized, group_names, expect_active):
+    task_vars['openshift_deployment_type'] = deployment_type
+    task_vars['openshift']['common']['is_containerized'] = is_containerized
+    task_vars['group_names'] = group_names
     assert DockerImageAvailability(None, task_vars).is_active() == expect_active
 
 
@@ -30,10 +45,10 @@ def test_is_active(deployment_type, is_containerized, group_names, expect_active
     (True, False),
     (False, True),
 ])
-def test_all_images_available_locally(is_containerized, is_atomic):
+def test_all_images_available_locally(task_vars, is_containerized, is_atomic):
     def execute_module(module_name, module_args, *_):
         if module_name == "yum":
-            return {"changed": True}
+            return {}
 
         assert module_name == "docker_image_facts"
         assert 'name' in module_args
@@ -42,19 +57,9 @@ def test_all_images_available_locally(is_containerized, is_atomic):
             'images': [module_args['name']],
         }
 
-    result = DockerImageAvailability(execute_module, task_vars=dict(
-        openshift=dict(
-            common=dict(
-                service_type='origin',
-                is_containerized=is_containerized,
-                is_atomic=is_atomic,
-            ),
-            docker=dict(additional_registries=["docker.io"]),
-        ),
-        openshift_deployment_type='origin',
-        openshift_image_tag='3.4',
-        group_names=['nodes', 'masters'],
-    )).run()
+    task_vars['openshift']['common']['is_containerized'] = is_containerized
+    task_vars['openshift']['common']['is_atomic'] = is_atomic
+    result = DockerImageAvailability(execute_module, task_vars).run()
 
     assert not result.get('failed', False)
 
@@ -63,55 +68,34 @@ def test_all_images_available_locally(is_containerized, is_atomic):
     False,
     True,
 ])
-def test_all_images_available_remotely(available_locally):
+def test_all_images_available_remotely(task_vars, available_locally):
     def execute_module(module_name, *_):
         if module_name == 'docker_image_facts':
             return {'images': [], 'failed': available_locally}
-        return {'changed': False}
+        return {}
 
-    check = DockerImageAvailability(execute_module, task_vars=dict(
-        openshift=dict(
-            common=dict(
-                service_type='origin',
-                is_containerized=False,
-                is_atomic=False,
-            ),
-            docker=dict(additional_registries=["docker.io", "registry.access.redhat.com"]),
-        ),
-        openshift_deployment_type='origin',
-        openshift_image_tag='v3.4',
-        group_names=['nodes', 'masters'],
-    ))
+    task_vars['openshift']['docker']['additional_registries'] = ["docker.io", "registry.access.redhat.com"]
+    task_vars['openshift_image_tag'] = 'v3.4'
+    check = DockerImageAvailability(execute_module, task_vars)
     check._module_retry_interval = 0
     result = check.run()
 
     assert not result.get('failed', False)
 
 
-def test_all_images_unavailable():
-    def execute_module(module_name=None, *_):
-        if module_name == "command":
-            return {
-                'failed': True,
-            }
+def test_all_images_unavailable(task_vars):
+    def execute_module(module_name=None, *args):
+        if module_name == "wait_for":
+            return {}
+        elif module_name == "command":
+            return {'failed': True}
 
-        return {
-            'changed': False,
-        }
+        return {}  # docker_image_facts failure
 
-    check = DockerImageAvailability(execute_module, task_vars=dict(
-        openshift=dict(
-            common=dict(
-                service_type='origin',
-                is_containerized=False,
-                is_atomic=False,
-            ),
-            docker=dict(additional_registries=["docker.io"]),
-        ),
-        openshift_deployment_type="openshift-enterprise",
-        openshift_image_tag='latest',
-        group_names=['nodes', 'masters'],
-    ))
+    task_vars['openshift']['docker']['additional_registries'] = ["docker.io"]
+    task_vars['openshift_deployment_type'] = "openshift-enterprise"
+    task_vars['openshift_image_tag'] = 'latest'
+    check = DockerImageAvailability(execute_module, task_vars)
     check._module_retry_interval = 0
     actual = check.run()
 
@@ -129,30 +113,19 @@ def test_all_images_unavailable():
         ["dependencies can be installed via `yum`"]
     ),
 ])
-def test_skopeo_update_failure(message, extra_words):
+def test_skopeo_update_failure(task_vars, message, extra_words):
     def execute_module(module_name=None, *_):
         if module_name == "yum":
             return {
                 "failed": True,
                 "msg": message,
-                "changed": False,
             }
 
-        return {'changed': False}
+        return {}
 
-    check = DockerImageAvailability(execute_module, task_vars=dict(
-        openshift=dict(
-            common=dict(
-                service_type='origin',
-                is_containerized=False,
-                is_atomic=False,
-            ),
-            docker=dict(additional_registries=["unknown.io"]),
-        ),
-        openshift_deployment_type="openshift-enterprise",
-        openshift_image_tag='',
-        group_names=['nodes', 'masters'],
-    ))
+    task_vars['openshift']['docker']['additional_registries'] = ["unknown.io"]
+    task_vars['openshift_deployment_type'] = "openshift-enterprise"
+    check = DockerImageAvailability(execute_module, task_vars)
     check._module_retry_interval = 0
     actual = check.run()
 
@@ -161,32 +134,42 @@ def test_skopeo_update_failure(message, extra_words):
         assert word in actual["msg"]
 
 
-@pytest.mark.parametrize("deployment_type,registries", [
-    ("origin", ["unknown.io"]),
-    ("openshift-enterprise", ["registry.access.redhat.com"]),
-    ("openshift-enterprise", []),
-])
-def test_registry_availability(deployment_type, registries):
+@pytest.mark.parametrize(
+    "image, registries, connection_test_failed, skopeo_failed, "
+    "expect_success, expect_registries_reached", [
+        (
+            "spam/eggs:v1", ["test.reg"],
+            True, True,
+            False,
+            {"test.reg": False},
+        ),
+        (
+            "spam/eggs:v1", ["test.reg"],
+            False, True,
+            False,
+            {"test.reg": True},
+        ),
+        (
+            "eggs.reg/spam/eggs:v1", ["test.reg"],
+            False, False,
+            True,
+            {"eggs.reg": True},
+        ),
+    ])
+def test_registry_availability(image, registries, connection_test_failed, skopeo_failed,
+                               expect_success, expect_registries_reached):
     def execute_module(module_name=None, *_):
-        return {
-            'changed': False,
-        }
+        if module_name == "wait_for":
+            return dict(msg="msg", failed=connection_test_failed)
+        elif module_name == "command":
+            return dict(msg="msg", failed=skopeo_failed)
 
-    actual = DockerImageAvailability(execute_module, task_vars=dict(
-        openshift=dict(
-            common=dict(
-                service_type='origin',
-                is_containerized=False,
-                is_atomic=False,
-            ),
-            docker=dict(additional_registries=registries),
-        ),
-        openshift_deployment_type=deployment_type,
-        openshift_image_tag='',
-        group_names=['nodes', 'masters'],
-    )).run()
+    check = DockerImageAvailability(execute_module, task_vars())
+    check._module_retry_interval = 0
 
-    assert not actual.get("failed", False)
+    available = check.is_available_skopeo_image(image, registries)
+    assert available == expect_success
+    assert expect_registries_reached == check.reachable_registries
 
 
 @pytest.mark.parametrize("deployment_type, is_containerized, groups, oreg_url, expected", [
@@ -263,7 +246,7 @@ def test_required_images(deployment_type, is_containerized, groups, oreg_url, ex
         openshift_image_tag='vtest',
     )
 
-    assert expected == DockerImageAvailability("DUMMY", task_vars).required_images()
+    assert expected == DockerImageAvailability(task_vars=task_vars).required_images()
 
 
 def test_containerized_etcd():
@@ -277,4 +260,4 @@ def test_containerized_etcd():
         group_names=['etcd'],
     )
     expected = set(['registry.access.redhat.com/rhel7/etcd'])
-    assert expected == DockerImageAvailability("DUMMY", task_vars).required_images()
+    assert expected == DockerImageAvailability(task_vars=task_vars).required_images()