diff --git a/hotsos/core/ycheck/engine/properties/common.py b/hotsos/core/ycheck/engine/properties/common.py index aa2258145..5a43f78af 100644 --- a/hotsos/core/ycheck/engine/properties/common.py +++ b/hotsos/core/ycheck/engine/properties/common.py @@ -590,17 +590,23 @@ def eval_ungrouped_item(self, item): """ final_results = [] for entry in item: + # For the purposes of caching we treat a list with > 1 item as + # "grouped". result = self.get_item_result_callback(entry, - is_default_group=True) + grouped=len(item) > 1) final_results.append(result) return final_results @abc.abstractmethod - def get_item_result_callback(self, item, is_default_group=False): + def get_item_result_callback(self, item, grouped=False): """ Must be implemented to evaluate a single item and return its result. + + @param item: property we want to evaluate + @param grouped: whether or not the item is part of a logical group or + list. """ def _is_op_group(self, prop): @@ -635,7 +641,9 @@ def eval_op_group_items(self, logical_op_group): log.debug("op group member has %s item(s)", len(member)) if len(member) == 1: - group_results.append(self.get_item_result_callback(member)) + result = self.get_item_result_callback(member, + grouped=True) + group_results.append(result) num_single += 1 continue @@ -649,7 +657,7 @@ def eval_op_group_items(self, logical_op_group): result, logical_op) break - result = self.get_item_result_callback(entry) + result = self.get_item_result_callback(entry, grouped=True) group_results.append(result) prev = result num_list += 1 diff --git a/hotsos/core/ycheck/engine/properties/conclusions.py b/hotsos/core/ycheck/engine/properties/conclusions.py index 6fd40dca8..8d23990a4 100644 --- a/hotsos/core/ycheck/engine/properties/conclusions.py +++ b/hotsos/core/ycheck/engine/properties/conclusions.py @@ -121,7 +121,7 @@ def _override_mapped_member_types(cls): def add_checks_instances(self, checks): self.checks_instances = checks - def get_item_result_callback(self, item, is_default_group=False): + def get_item_result_callback(self, item, grouped=False): name = str(item) if name not in self.checks_instances: raise Exception("no check found with name {}".format(name)) diff --git a/hotsos/core/ycheck/engine/properties/requires/requires.py b/hotsos/core/ycheck/engine/properties/requires/requires.py index 29cd685dd..f788be35f 100644 --- a/hotsos/core/ycheck/engine/properties/requires/requires.py +++ b/hotsos/core/ycheck/engine/properties/requires/requires.py @@ -18,6 +18,7 @@ class YPropertyRequiresBase(YPropertyMappedOverrideBase, LogicalCollectionHandler): + CACHE_CHECK_KEY = '__PREVIOUSLY_CACHED_PROPERTY_TYPE' @classmethod def _override_mapped_member_types(cls): @@ -30,13 +31,42 @@ def _override_mapped_member_types(cls): path.YRequirementTypePath, varops.YPropertyVarOps] - def get_item_result_callback(self, item, is_default_group=False): + def get_item_result_callback(self, item, grouped=False): + """ + Evaluate item and return its result. + + Also copy the cache of the item into the local cache. If part of a + logical group whereby more than one item is being evaluated to get + a final results, we only copy the cache data if the result is True + and either (a) it is the first to be saved or (b) it has the same type + as the previous one to be saved. + + For example with the following group: + + or: + - apt: foo + - apt: bar + + If package foo exists and bar does not, the final contents of the group + will that from 'apt: foo'. If both exists then it will be the result of + 'apt: bar'. + + @param item: YRequirementType object + @param grouped: If True, item is part of a (logical) group of one or + more items or a list with more than one item to which + to default logical operator (AND) will be applied. + @return: boolean result i.e. True or False + """ result = item() - # if its a single group we can cache the results but otherwise it wont - # make sense since all will conflict. - if is_default_group: + previously_cached = getattr(self.cache, self.CACHE_CHECK_KEY) + req_prop_type = item.__class__.__name__ + if not grouped or (result and + previously_cached in (None, req_prop_type)): + if previously_cached is None: + self.cache.set(self.CACHE_CHECK_KEY, req_prop_type) + log.debug("merging item (type=%s) cache id=%s into cache id=%s", - item.__class__.__name__, item.cache.id, self.cache.id) + req_prop_type, item.cache.id, self.cache.id) self.cache.merge(item.cache) return result diff --git a/hotsos/core/ycheck/engine/properties/requires/types/config.py b/hotsos/core/ycheck/engine/properties/requires/types/config.py index b1ab3b9e4..75e7f1206 100644 --- a/hotsos/core/ycheck/engine/properties/requires/types/config.py +++ b/hotsos/core/ycheck/engine/properties/requires/types/config.py @@ -61,7 +61,7 @@ def and_group_stop_on_first_false(self): """ return False - def get_item_result_callback(self, item, is_default_group=False): + def get_item_result_callback(self, item, grouped=False): handlers = self.context.assertions_ctxt['cfg_handlers'] if not handlers: return diff --git a/tests/unit/test_ycheck_properties.py b/tests/unit/test_ycheck_properties.py index af326aab6..789ab7e4c 100644 --- a/tests/unit/test_ycheck_properties.py +++ b/tests/unit/test_ycheck_properties.py @@ -1,9 +1,14 @@ import datetime +import os +import shutil +import tempfile from unittest import mock import yaml from hotsos.core.config import HotSOSConfig from hotsos.core.host_helpers.config import SectionalConfigBase +from hotsos.core.issues.utils import IssuesStore +from hotsos.core.ycheck import scenarios from hotsos.core.ycheck.engine import ( YDefsSection, YDefsLoader, @@ -196,6 +201,209 @@ def __init__(self, name, state, has_instances, start_time): """ # noqa +class TempScenarioDefs(object): + + def __init__(self): + self.root = None + self.path = None + + def __enter__(self): + self.root = tempfile.mkdtemp() + HotSOSConfig.plugin_yaml_defs = self.root + self.path = os.path.join(self.root, 'scenarios', + HotSOSConfig.plugin_name, 'test.yaml') + os.makedirs(os.path.dirname(self.path)) + return self + + def __exit__(self, *args): + shutil.rmtree(self.root) + return False + + +class TestYamlRequiresTypeCache(utils.BaseTestCase): + + @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': 'ii foo 123 amd64'}) + def test_single_item(self): + mydef = YDefsSection('mydef', yaml.safe_load("chk1:\n apt: foo")) + for entry in mydef.leaf_sections: + self.assertTrue(entry.requires.passes) + expected = {'__PREVIOUSLY_CACHED_PROPERTY_TYPE': + 'YRequirementTypeAPT', + 'package': 'foo', + 'passes': True, + 'version': '123'} + self.assertEqual(entry.requires.cache.data, expected) + + @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': + 'ii foo 123 amd64'}) + def test_grouped_items_first_true(self): + """ If the first item evaluates to True we get that one. """ + scenario = """ + checks: + c1: + or: + - apt: foo + - apt: bar + conclusions: + c1: + decision: c1 + raises: + type: SystemWarning + message: '{pkg}' + format-dict: + pkg: '@checks.c1.requires.package' + """ # noqa + with TempScenarioDefs() as tmpscenarios: + with open(tmpscenarios.path, 'w') as fd: + fd.write(scenario) + + scenarios.YScenarioChecker().load_and_run() + issues = list(IssuesStore().load().values())[0] + self.assertEqual(issues[0]['message'], 'foo') + + @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': + 'ii bar 123 amd64'}) + def test_grouped_items_last_true(self): + """ If the last item evaluates to True we get that one. """ + scenario = """ + checks: + c1: + or: + - apt: foo + - apt: bar + conclusions: + c1: + decision: c1 + raises: + type: SystemWarning + message: '{pkg}' + format-dict: + pkg: '@checks.c1.requires.package' + """ # noqa + with TempScenarioDefs() as tmpscenarios: + with open(tmpscenarios.path, 'w') as fd: + fd.write(scenario) + + scenarios.YScenarioChecker().load_and_run() + issues = list(IssuesStore().load().values())[0] + self.assertEqual(issues[0]['message'], 'bar') + + @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': + 'ii foo 123 amd64\nii bar 123 amd64'}) + def test_grouped_items_all_true(self): + """ If all items evaluate to True we get the cache of the last one. """ + scenario = """ + checks: + c1: + or: + - apt: foo + - apt: bar + conclusions: + c1: + decision: c1 + raises: + type: SystemWarning + message: '{pkg}' + format-dict: + pkg: '@checks.c1.requires.package' + """ # noqa + with TempScenarioDefs() as tmpscenarios: + with open(tmpscenarios.path, 'w') as fd: + fd.write(scenario) + + scenarios.YScenarioChecker().load_and_run() + issues = list(IssuesStore().load().values())[0] + self.assertEqual(issues[0]['message'], 'bar') + + @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': ''}) + def test_grouped_items_all_false(self): + """ If the all items evaluates to False nothing is copied. """ + scenario = """ + checks: + c1: + or: + - apt: foo + - apt: bar + conclusions: + c1: + decision: c1 + raises: + type: SystemWarning + message: '{pkg}' + format-dict: + pkg: '@checks.c1.requires.package' + """ + with TempScenarioDefs() as tmpscenarios: + with open(tmpscenarios.path, 'w') as fd: + fd.write(scenario) + + scenarios.YScenarioChecker().load_and_run() + self.assertEqual(len(IssuesStore().load()), 0) + + @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': + 'ii foo 123 amd64\nii bar 123 amd64', + 'sos_commands/snap/snap_list_--all': + 'snapd 2.54.2 14549 latest/stable xxx'}) + def test_grouped_items_all_true_mixed_types_apt_first(self): + """ If the all items evaluates to False nothing is copied. """ + scenario = """ + checks: + c1: + or: + - apt: foo + - apt: bar + - snap: snapd + conclusions: + c1: + decision: c1 + raises: + type: SystemWarning + message: '{pkg}' + format-dict: + pkg: '@checks.c1.requires.package' + """ + with TempScenarioDefs() as tmpscenarios: + with open(tmpscenarios.path, 'w') as fd: + fd.write(scenario) + + scenarios.YScenarioChecker().load_and_run() + issues = list(IssuesStore().load().values())[0] + self.assertEqual(issues[0]['message'], 'bar') + + @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': + 'ii foo 123 amd64\nii bar 123 amd64', + 'sos_commands/snap/snap_list_--all': + 'snapd 2.54.2 14549 latest/stable xxx'}) + def test_grouped_items_all_true_mixed_types_snap_first(self): + """ If the all items evaluates to False nothing is copied. """ + scenario = """ + checks: + c1: + or: + - snap: snapd + - apt: foo + - apt: bar + conclusions: + c1: + decision: c1 + raises: + type: SystemWarning + message: '{pkg}' + format-dict: + pkg: '@checks.c1.requires.package' + """ + with TempScenarioDefs() as tmpscenarios: + with open(tmpscenarios.path, 'w') as fd: + fd.write(scenario) + + scenarios.YScenarioChecker().load_and_run() + issues = list(IssuesStore().load().values())[0] + # NOTE: dicts and lists are currently being evaluated in + # alphabetical order. Not clear why but that explains why + # grouped items give this (unexpected) result. + self.assertEqual(issues[0]['message'], 'bar') + + class TestYamlRequiresTypeAPT(utils.BaseTestCase): @utils.create_data_root({'sos_commands/dpkg/dpkg_-l': DPKG_L})