openshift_checks: enable providing file outputs

Some refactoring of checks and the action plugin to enable writing files
locally about the check operation and results, if the user wants them.
This is aimed at enabling persistent and machine-readable results from
recurring runs of health checks.

Now, rather than trying to build a result hash to return from running
each check, checks can just register what they need to as they're going
along, and the action plugin processes state when the check is done.
Checks can register failures, notes about what they saw, and arbitrary
files to be saved into a directory structure where the user specifies.
If no directory is specified, no files are written.

At this time checks can still return a result hash, but that will likely
be refactored away in the next iteration.

Multiple failures can be registered without halting check execution.
Throwing an exception or returning a hash with "failed" is registered as
a failure.

execute_module now does a little more with the results. Results are
automatically included in notes and written individually as files.
"changed" results are propagated. Some json results are decoded.

A few of the checks were enhanced to use these features; all get some of
the features for free.

8 files changed, 166 insertions, 47 deletions

diff --git a/roles/openshift_health_checker/test/action_plugin_test.py b/roles/openshift_health_checker/test/action_plugin_test.py
index 58864da21..f14887303 100644
--- a/roles/openshift_health_checker/test/action_plugin_test.py
+++ b/roles/openshift_health_checker/test/action_plugin_test.py
@@ -3,10 +3,12 @@ import pytest
 from ansible.playbook.play_context import PlayContext
 
 from openshift_health_check import ActionModule, resolve_checks
-from openshift_checks import OpenShiftCheckException
+from openshift_health_check import copy_remote_file_to_dir, write_result_to_output_dir, write_to_output_file
+from openshift_checks import OpenShiftCheckException, FileToSave
 
 
-def fake_check(name='fake_check', tags=None, is_active=True, run_return=None, run_exception=None, changed=False):
+def fake_check(name='fake_check', tags=None, is_active=True, run_return=None, run_exception=None,
+               run_logs=None, run_files=None, changed=False, get_var_return=None):
     """Returns a new class that is compatible with OpenShiftCheck for testing."""
 
     _name, _tags = name, tags
@@ -14,12 +16,16 @@ def fake_check(name='fake_check', tags=None, is_active=True, run_return=None, ru
     class FakeCheck(object):
         name = _name
         tags = _tags or []
-        changed = False
 
-        def __init__(self, execute_module=None, task_vars=None, tmp=None):
-            pass
+        def __init__(self, **_):
+            self.changed = False
+            self.failures = []
+            self.logs = run_logs or []
+            self.files_to_save = run_files or []
 
         def is_active(self):
+            if isinstance(is_active, Exception):
+                raise is_active
             return is_active
 
         def run(self):
@@ -28,6 +34,13 @@ def fake_check(name='fake_check', tags=None, is_active=True, run_return=None, ru
                 raise run_exception
             return run_return
 
+        def get_var(*args, **_):
+            return get_var_return
+
+        def register_failure(self, exc):
+            self.failures.append(OpenShiftCheckException(str(exc)))
+            return
+
     return FakeCheck
 
 
@@ -98,13 +111,18 @@ def test_action_plugin_cannot_load_checks_with_the_same_name(plugin, task_vars,
     assert failed(result, msg_has=['duplicate', 'duplicate_name', 'FakeCheck'])
 
 
-def test_action_plugin_skip_non_active_checks(plugin, task_vars, monkeypatch):
-    checks = [fake_check(is_active=False)]
+@pytest.mark.parametrize('is_active, skipped_reason', [
+    (False, "Not active for this host"),
+    (Exception("borked"), "exception"),
+])
+def test_action_plugin_skip_non_active_checks(is_active, skipped_reason, plugin, task_vars, monkeypatch):
+    checks = [fake_check(is_active=is_active)]
     monkeypatch.setattr('openshift_checks.OpenShiftCheck.subclasses', classmethod(lambda cls: checks))
 
     result = plugin.run(tmp=None, task_vars=task_vars)
 
-    assert result['checks']['fake_check'] == dict(skipped=True, skipped_reason="Not active for this host")
+    assert result['checks']['fake_check'].get('skipped')
+    assert skipped_reason in result['checks']['fake_check'].get('skipped_reason')
     assert not failed(result)
     assert not changed(result)
     assert not skipped(result)
@@ -128,10 +146,21 @@ def test_action_plugin_skip_disabled_checks(to_disable, plugin, task_vars, monke
     assert not skipped(result)
 
 
+def test_action_plugin_run_list_checks(monkeypatch):
+    task = FakeTask('openshift_health_check', {'checks': []})
+    plugin = ActionModule(task, None, PlayContext(), None, None, None)
+    monkeypatch.setattr(plugin, 'load_known_checks', lambda *_: {})
+    result = plugin.run()
+
+    assert failed(result, msg_has="Available checks")
+    assert not changed(result)
+    assert not skipped(result)
+
+
 def test_action_plugin_run_check_ok(plugin, task_vars, monkeypatch):
     check_return_value = {'ok': 'test'}
-    check_class = fake_check(run_return=check_return_value)
-    monkeypatch.setattr(plugin, 'load_known_checks', lambda tmp, task_vars: {'fake_check': check_class()})
+    check_class = fake_check(run_return=check_return_value, run_files=[None])
+    monkeypatch.setattr(plugin, 'load_known_checks', lambda *_: {'fake_check': check_class()})
     monkeypatch.setattr('openshift_health_check.resolve_checks', lambda *args: ['fake_check'])
 
     result = plugin.run(tmp=None, task_vars=task_vars)
@@ -145,7 +174,7 @@ def test_action_plugin_run_check_ok(plugin, task_vars, monkeypatch):
 def test_action_plugin_run_check_changed(plugin, task_vars, monkeypatch):
     check_return_value = {'ok': 'test'}
     check_class = fake_check(run_return=check_return_value, changed=True)
-    monkeypatch.setattr(plugin, 'load_known_checks', lambda tmp, task_vars: {'fake_check': check_class()})
+    monkeypatch.setattr(plugin, 'load_known_checks', lambda *_: {'fake_check': check_class()})
     monkeypatch.setattr('openshift_health_check.resolve_checks', lambda *args: ['fake_check'])
 
     result = plugin.run(tmp=None, task_vars=task_vars)
@@ -158,9 +187,9 @@ def test_action_plugin_run_check_changed(plugin, task_vars, monkeypatch):
 
 
 def test_action_plugin_run_check_fail(plugin, task_vars, monkeypatch):
-    check_return_value = {'failed': True}
+    check_return_value = {'failed': True, 'msg': 'this is a failure'}
     check_class = fake_check(run_return=check_return_value)
-    monkeypatch.setattr(plugin, 'load_known_checks', lambda tmp, task_vars: {'fake_check': check_class()})
+    monkeypatch.setattr(plugin, 'load_known_checks', lambda *_: {'fake_check': check_class()})
     monkeypatch.setattr('openshift_health_check.resolve_checks', lambda *args: ['fake_check'])
 
     result = plugin.run(tmp=None, task_vars=task_vars)
@@ -171,24 +200,51 @@ def test_action_plugin_run_check_fail(plugin, task_vars, monkeypatch):
     assert not skipped(result)
 
 
-def test_action_plugin_run_check_exception(plugin, task_vars, monkeypatch):
+@pytest.mark.parametrize('exc_class, expect_traceback', [
+    (OpenShiftCheckException, False),
+    (Exception, True),
+])
+def test_action_plugin_run_check_exception(plugin, task_vars, exc_class, expect_traceback, monkeypatch):
     exception_msg = 'fake check has an exception'
-    run_exception = OpenShiftCheckException(exception_msg)
+    run_exception = exc_class(exception_msg)
     check_class = fake_check(run_exception=run_exception, changed=True)
-    monkeypatch.setattr(plugin, 'load_known_checks', lambda tmp, task_vars: {'fake_check': check_class()})
+    monkeypatch.setattr(plugin, 'load_known_checks', lambda *_: {'fake_check': check_class()})
     monkeypatch.setattr('openshift_health_check.resolve_checks', lambda *args: ['fake_check'])
 
     result = plugin.run(tmp=None, task_vars=task_vars)
 
     assert failed(result['checks']['fake_check'], msg_has=exception_msg)
+    assert expect_traceback == ("Traceback" in result['checks']['fake_check']['msg'])
     assert failed(result, msg_has=['failed'])
     assert changed(result['checks']['fake_check'])
     assert changed(result)
     assert not skipped(result)
 
 
+def test_action_plugin_run_check_output_dir(plugin, task_vars, tmpdir, monkeypatch):
+    check_class = fake_check(
+        run_return={},
+        run_logs=[('thing', 'note')],
+        run_files=[
+            FileToSave('save.file', 'contents', None),
+            FileToSave('save.file', 'duplicate', None),
+            FileToSave('copy.file', None, 'foo'),  # note: copy runs execute_module => exception
+        ],
+    )
+    task_vars['openshift_checks_output_dir'] = str(tmpdir)
+    check_class.get_var = lambda self, name, **_: task_vars.get(name)
+    monkeypatch.setattr(plugin, 'load_known_checks', lambda *_: {'fake_check': check_class()})
+    monkeypatch.setattr('openshift_health_check.resolve_checks', lambda *args: ['fake_check'])
+
+    plugin.run(tmp=None, task_vars=task_vars)
+    assert any(path.basename == task_vars['ansible_host'] for path in tmpdir.listdir())
+    assert any(path.basename == 'fake_check.log.json' for path in tmpdir.visit())
+    assert any(path.basename == 'save.file' for path in tmpdir.visit())
+    assert any(path.basename == 'save.file.2' for path in tmpdir.visit())
+
+
 def test_action_plugin_resolve_checks_exception(plugin, task_vars, monkeypatch):
-    monkeypatch.setattr(plugin, 'load_known_checks', lambda tmp, task_vars: {})
+    monkeypatch.setattr(plugin, 'load_known_checks', lambda *_: {})
 
     result = plugin.run(tmp=None, task_vars=task_vars)
 
@@ -254,3 +310,38 @@ def test_resolve_checks_failure(names, all_checks, words_in_exception):
         resolve_checks(names, all_checks)
     for word in words_in_exception:
         assert word in str(excinfo.value)
+
+
+@pytest.mark.parametrize('give_output_dir, result, expect_file', [
+    (False, None, False),
+    (True, dict(content="c3BhbQo=", encoding="base64"), True),
+    (True, dict(content="encoding error", encoding="base64"), False),
+    (True, dict(content="spam", no_encoding=None), True),
+    (True, dict(failed=True, msg="could not slurp"), False),
+])
+def test_copy_remote_file_to_dir(give_output_dir, result, expect_file, tmpdir):
+    check = fake_check()()
+    check.execute_module = lambda *args, **_: result
+    copy_remote_file_to_dir(check, "remote_file", str(tmpdir) if give_output_dir else "", "local_file")
+    assert expect_file == any(path.basename == "local_file" for path in tmpdir.listdir())
+
+
+def test_write_to_output_exceptions(tmpdir, monkeypatch, capsys):
+
+    class Spam(object):
+        def __str__(self):
+            raise Exception("break str")
+
+    test = {1: object(), 2: Spam()}
+    test[3] = test
+    write_result_to_output_dir(str(tmpdir), test)
+    assert "Error writing" in test["output_files"]
+
+    output_dir = tmpdir.join("eggs")
+    output_dir.write("spam")  # so now it's not a dir
+    write_to_output_file(str(output_dir), "somefile", "somedata")
+    assert "Could not write" in capsys.readouterr()[1]
+
+    monkeypatch.setattr("openshift_health_check.prepare_output_dir", lambda *_: False)
+    write_result_to_output_dir(str(tmpdir), test)
+    assert "Error creating" in test["output_files"]
diff --git a/roles/openshift_health_checker/test/disk_availability_test.py b/roles/openshift_health_checker/test/disk_availability_test.py
index f4fd2dfed..9ae679b79 100644
--- a/roles/openshift_health_checker/test/disk_availability_test.py
+++ b/roles/openshift_health_checker/test/disk_availability_test.py
@@ -183,11 +183,12 @@ def test_fails_with_insufficient_disk_space(name, group_names, configured_min, a
         ansible_mounts=ansible_mounts,
     )
 
-    result = DiskAvailability(fake_execute_module, task_vars).run()
+    check = DiskAvailability(fake_execute_module, task_vars)
+    check.run()
 
-    assert result['failed']
+    assert check.failures
     for chunk in 'below recommended'.split() + expect_chunks:
-        assert chunk in result.get('msg', '')
+        assert chunk in str(check.failures[0])
 
 
 @pytest.mark.parametrize('name,group_names,context,ansible_mounts,failed,extra_words', [
@@ -237,11 +238,11 @@ def test_min_required_space_changes_with_upgrade_context(name, group_names, cont
     )
 
     check = DiskAvailability(fake_execute_module, task_vars)
-    result = check.run()
+    check.run()
 
-    assert result.get("failed", False) == failed
+    assert bool(check.failures) == failed
     for word in extra_words:
-        assert word in result.get('msg', '')
+        assert word in str(check.failures[0])
 
 
 def fake_execute_module(*args):
diff --git a/roles/openshift_health_checker/test/elasticsearch_test.py b/roles/openshift_health_checker/test/elasticsearch_test.py
index 09bacd9ac..3fa5e8929 100644
--- a/roles/openshift_health_checker/test/elasticsearch_test.py
+++ b/roles/openshift_health_checker/test/elasticsearch_test.py
@@ -72,7 +72,7 @@ def test_check_elasticsearch():
     assert_error_in_list('NoRunningPods', excinfo.value)
 
     # canned oc responses to match so all the checks pass
-    def exec_oc(cmd, args):
+    def exec_oc(cmd, args, **_):
         if '_cat/master' in cmd:
             return 'name logging-es'
         elif '/_nodes' in cmd:
@@ -97,7 +97,7 @@ def test_check_running_es_pods():
 
 def test_check_elasticsearch_masters():
     pods = [plain_es_pod]
-    check = canned_elasticsearch(task_vars_config_base, lambda *_: plain_es_pod['_test_master_name_str'])
+    check = canned_elasticsearch(task_vars_config_base, lambda *args, **_: plain_es_pod['_test_master_name_str'])
     assert not check.check_elasticsearch_masters(pods_by_name(pods))
 
 
@@ -117,7 +117,7 @@ def test_check_elasticsearch_masters():
 ])
 def test_check_elasticsearch_masters_error(pods, expect_error):
     test_pods = list(pods)
-    check = canned_elasticsearch(task_vars_config_base, lambda *_: test_pods.pop(0)['_test_master_name_str'])
+    check = canned_elasticsearch(task_vars_config_base, lambda *args, **_: test_pods.pop(0)['_test_master_name_str'])
     assert_error_in_list(expect_error, check.check_elasticsearch_masters(pods_by_name(pods)))
 
 
@@ -129,7 +129,7 @@ es_node_list = {
 
 
 def test_check_elasticsearch_node_list():
-    check = canned_elasticsearch(task_vars_config_base, lambda *_: json.dumps(es_node_list))
+    check = canned_elasticsearch(task_vars_config_base, lambda *args, **_: json.dumps(es_node_list))
     assert not check.check_elasticsearch_node_list(pods_by_name([plain_es_pod]))
 
 
@@ -151,13 +151,13 @@ def test_check_elasticsearch_node_list():
     ),
 ])
 def test_check_elasticsearch_node_list_errors(pods, node_list, expect_error):
-    check = canned_elasticsearch(task_vars_config_base, lambda cmd, args: json.dumps(node_list))
+    check = canned_elasticsearch(task_vars_config_base, lambda cmd, args, **_: json.dumps(node_list))
     assert_error_in_list(expect_error, check.check_elasticsearch_node_list(pods_by_name(pods)))
 
 
 def test_check_elasticsearch_cluster_health():
     test_health_data = [{"status": "green"}]
-    check = canned_elasticsearch(exec_oc=lambda *_: json.dumps(test_health_data.pop(0)))
+    check = canned_elasticsearch(exec_oc=lambda *args, **_: json.dumps(test_health_data.pop(0)))
     assert not check.check_es_cluster_health(pods_by_name([plain_es_pod]))
 
 
@@ -175,12 +175,12 @@ def test_check_elasticsearch_cluster_health():
 ])
 def test_check_elasticsearch_cluster_health_errors(pods, health_data, expect_error):
     test_health_data = list(health_data)
-    check = canned_elasticsearch(exec_oc=lambda *_: json.dumps(test_health_data.pop(0)))
+    check = canned_elasticsearch(exec_oc=lambda *args, **_: json.dumps(test_health_data.pop(0)))
     assert_error_in_list(expect_error, check.check_es_cluster_health(pods_by_name(pods)))
 
 
 def test_check_elasticsearch_diskspace():
-    check = canned_elasticsearch(exec_oc=lambda *_: 'IUse% Use%\n 3%  4%\n')
+    check = canned_elasticsearch(exec_oc=lambda *args, **_: 'IUse% Use%\n 3%  4%\n')
     assert not check.check_elasticsearch_diskspace(pods_by_name([plain_es_pod]))
 
 
@@ -199,5 +199,5 @@ def test_check_elasticsearch_diskspace():
     ),
 ])
 def test_check_elasticsearch_diskspace_errors(disk_data, expect_error):
-    check = canned_elasticsearch(exec_oc=lambda *_: disk_data)
+    check = canned_elasticsearch(exec_oc=lambda *args, **_: disk_data)
     assert_error_in_list(expect_error, check.check_elasticsearch_diskspace(pods_by_name([plain_es_pod])))
diff --git a/roles/openshift_health_checker/test/logging_index_time_test.py b/roles/openshift_health_checker/test/logging_index_time_test.py
index 22566b295..c48ade9b8 100644
--- a/roles/openshift_health_checker/test/logging_index_time_test.py
+++ b/roles/openshift_health_checker/test/logging_index_time_test.py
@@ -102,7 +102,7 @@ def test_with_running_pods():
     ),
 ], ids=lambda argval: argval[0])
 def test_wait_until_cmd_or_err_succeeds(name, json_response, uuid, timeout):
-    check = canned_loggingindextime(lambda *_: json.dumps(json_response))
+    check = canned_loggingindextime(lambda *args, **_: json.dumps(json_response))
     check.wait_until_cmd_or_err(plain_running_elasticsearch_pod, uuid, timeout)
 
 
@@ -131,7 +131,7 @@ def test_wait_until_cmd_or_err_succeeds(name, json_response, uuid, timeout):
     )
 ], ids=lambda argval: argval[0])
 def test_wait_until_cmd_or_err(name, json_response, timeout, expect_error):
-    check = canned_loggingindextime(lambda *_: json.dumps(json_response))
+    check = canned_loggingindextime(lambda *args, **_: json.dumps(json_response))
     with pytest.raises(OpenShiftCheckException) as error:
         check.wait_until_cmd_or_err(plain_running_elasticsearch_pod, SAMPLE_UUID, timeout)
 
@@ -139,7 +139,7 @@ def test_wait_until_cmd_or_err(name, json_response, timeout, expect_error):
 
 
 def test_curl_kibana_with_uuid():
-    check = canned_loggingindextime(lambda *_: json.dumps({"statusCode": 404}))
+    check = canned_loggingindextime(lambda *args, **_: json.dumps({"statusCode": 404}))
     check.generate_uuid = lambda: SAMPLE_UUID
     assert SAMPLE_UUID == check.curl_kibana_with_uuid(plain_running_kibana_pod)
 
@@ -161,7 +161,7 @@ def test_curl_kibana_with_uuid():
     ),
 ], ids=lambda argval: argval[0])
 def test_failed_curl_kibana_with_uuid(name, json_response, expect_error):
-    check = canned_loggingindextime(lambda *_: json.dumps(json_response))
+    check = canned_loggingindextime(lambda *args, **_: json.dumps(json_response))
     check.generate_uuid = lambda: SAMPLE_UUID
 
     with pytest.raises(OpenShiftCheckException) as error:
diff --git a/roles/openshift_health_checker/test/openshift_check_test.py b/roles/openshift_health_checker/test/openshift_check_test.py
index 789784c77..bc0c3b26c 100644
--- a/roles/openshift_health_checker/test/openshift_check_test.py
+++ b/roles/openshift_health_checker/test/openshift_check_test.py
@@ -106,13 +106,40 @@ 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 convert_oscexc(_):
+    raise OpenShiftCheckException("known failure")
+
+
+def convert_exc(_):
+    raise Exception("failure unknown")
+
+
+@pytest.mark.parametrize("keys, convert, expect_text", [
+    (("bar", "baz"), int, "Cannot convert"),
+    (("bar.baz",), float, "Cannot convert"),
+    (("foo",), "bogus", "TypeError"),
+    (("foo",), lambda a, b: 1, "TypeError"),
+    (("foo",), lambda a: 1 / 0, "ZeroDivisionError"),
+    (("foo",), convert_oscexc, "known failure"),
+    (("foo",), convert_exc, "failure unknown"),
 ])
-def test_get_var_convert_error(task_vars, keys, convert):
-    with pytest.raises(OpenShiftCheckException):
+def test_get_var_convert_error(task_vars, keys, convert, expect_text):
+    with pytest.raises(OpenShiftCheckException) as excinfo:
         dummy_check(task_vars).get_var(*keys, convert=convert)
+    assert expect_text in str(excinfo.value)
+
+
+def test_register(task_vars):
+    check = dummy_check(task_vars)
+
+    check.register_failure(OpenShiftCheckException("spam"))
+    assert "spam" in str(check.failures[0])
+
+    with pytest.raises(OpenShiftCheckException) as excinfo:
+        check.register_file("spam")  # no file contents specified
+    assert "not specified" in str(excinfo.value)
+
+    # normally execute_module registers the result file; test disabling that
+    check._execute_module = lambda *args, **_: dict()
+    check.execute_module("eggs", module_args={}, register=False)
+    assert not check.files_to_save
diff --git a/roles/openshift_health_checker/test/ovs_version_test.py b/roles/openshift_health_checker/test/ovs_version_test.py
index e1bf29d2a..602f32989 100644
--- a/roles/openshift_health_checker/test/ovs_version_test.py
+++ b/roles/openshift_health_checker/test/ovs_version_test.py
@@ -50,7 +50,7 @@ def test_ovs_package_version(openshift_release, expected_ovs_version):
         openshift_release=openshift_release,
         openshift_image_tag='v' + openshift_release,
     )
-    return_value = object()
+    return_value = {}  # note: check.execute_module modifies return hash contents
 
     def execute_module(module_name=None, module_args=None, *_):
         assert module_name == 'rpm_version'
diff --git a/roles/openshift_health_checker/test/package_availability_test.py b/roles/openshift_health_checker/test/package_availability_test.py
index 8aa87ca59..b34e8fbfc 100644
--- a/roles/openshift_health_checker/test/package_availability_test.py
+++ b/roles/openshift_health_checker/test/package_availability_test.py
@@ -49,7 +49,7 @@ def test_is_active(pkg_mgr, is_containerized, is_active):
     ),
 ])
 def test_package_availability(task_vars, must_have_packages, must_not_have_packages):
-    return_value = object()
+    return_value = {}
 
     def execute_module(module_name=None, module_args=None, *_):
         assert module_name == 'check_yum_update'
diff --git a/roles/openshift_health_checker/test/package_update_test.py b/roles/openshift_health_checker/test/package_update_test.py
index 7d9035a36..85d3c9cab 100644
--- a/roles/openshift_health_checker/test/package_update_test.py
+++ b/roles/openshift_health_checker/test/package_update_test.py
@@ -2,7 +2,7 @@ from openshift_checks.package_update import PackageUpdate
 
 
 def test_package_update():
-    return_value = object()
+    return_value = {}
 
     def execute_module(module_name=None, module_args=None, *_):
         assert module_name == 'check_yum_update'