From a064b72433233ab3bcb1f53112a8502519628611 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Sun, 28 Jul 2024 14:58:46 +0100 Subject: [PATCH] Re-enable pylint duplicate-code check The bulk of the changes in this patch are to the plugin_extensions that implement summary output. Most plugins have some entries in common and those are now moved to a base class so that they don't need to be redefined by each plugin. Also optimises some of the hosthelpers and checks implementations to satisfy the duplicate-code check. Some disables have been used where it was deemed that fixing the code would introduce too much instability and this can be addressed in future patches. --- examples/hotsos-example-juju.summary.yaml | 2 +- hotsos/core/host_helpers/common.py | 74 +++++++- hotsos/core/host_helpers/pebble.py | 63 +------ hotsos/core/host_helpers/systemd.py | 65 +------ hotsos/core/plugins/juju/common.py | 7 + hotsos/core/plugins/mysql.py | 4 +- hotsos/core/plugins/storage/bcache.py | 5 + hotsos/core/plugins/storage/ceph/common.py | 4 + hotsos/core/plugintools.py | 175 +++++++++++++++--- .../core/ycheck/engine/properties/checks.py | 1 + .../ycheck/engine/properties/conclusions.py | 1 + .../engine/properties/requires/requires.py | 1 + .../engine/properties/requires/types/apt.py | 1 + .../properties/requires/types/pebble.py | 85 +++------ .../requires/types/service_manager_common.py | 56 ++++++ .../engine/properties/requires/types/snap.py | 1 + .../properties/requires/types/systemd.py | 96 ++++------ hotsos/plugin_extensions/juju/summary.py | 25 +-- hotsos/plugin_extensions/kernel/summary.py | 6 + .../plugin_extensions/kubernetes/summary.py | 34 ++-- hotsos/plugin_extensions/lxd/summary.py | 32 +--- hotsos/plugin_extensions/maas/summary.py | 26 +-- hotsos/plugin_extensions/mysql/summary.py | 21 +-- .../openstack/agent/events.py | 19 +- .../openstack/agent/exceptions.py | 9 +- .../openstack/nova_external_events.py | 8 +- .../openstack/service_features.py | 7 +- .../openstack/service_network_checks.py | 15 +- hotsos/plugin_extensions/openstack/summary.py | 44 +---- hotsos/plugin_extensions/openstack/vm_info.py | 9 +- .../plugin_extensions/openvswitch/summary.py | 33 ++-- hotsos/plugin_extensions/pacemaker/summary.py | 22 +-- hotsos/plugin_extensions/rabbitmq/summary.py | 26 ++- hotsos/plugin_extensions/sosreport/summary.py | 25 +-- .../storage/bcache_summary.py | 13 +- .../plugin_extensions/storage/ceph_summary.py | 59 ++---- hotsos/plugin_extensions/vault/summary.py | 21 +-- pylintrc | 1 - tests/unit/storage/test_ceph_mon.py | 3 +- tests/unit/storage/test_ceph_osd.py | 10 +- tests/unit/test_juju.py | 4 +- tests/unit/test_openstack.py | 54 +++--- tests/unit/test_openvswitch.py | 20 +- tests/unit/test_system.py | 2 + tests/unit/ycheck/test_properties.py | 2 +- tests/unit/ycheck/test_scenarios.py | 2 + 46 files changed, 613 insertions(+), 580 deletions(-) create mode 100644 hotsos/core/ycheck/engine/properties/requires/types/service_manager_common.py diff --git a/examples/hotsos-example-juju.summary.yaml b/examples/hotsos-example-juju.summary.yaml index fd372104e..edfdd546a 100644 --- a/examples/hotsos-example-juju.summary.yaml +++ b/examples/hotsos-example-juju.summary.yaml @@ -24,13 +24,13 @@ system: this environment. If maintenance windows are required please consider disabling unattended upgrades. juju: + version: 2.9.22 services: systemd: enabled: - jujud-machine-1 ps: - jujud (1) - version: 2.9.22 machine: '1' units: ceph-osd/0: diff --git a/hotsos/core/host_helpers/common.py b/hotsos/core/host_helpers/common.py index 0e36b4a9b..bed91ece9 100644 --- a/hotsos/core/host_helpers/common.py +++ b/hotsos/core/host_helpers/common.py @@ -4,6 +4,7 @@ import os import pickle import re +from functools import cached_property from searchkit.utils import MPCache from hotsos.core.config import HotSOSConfig @@ -12,6 +13,7 @@ SourceNotFound, ) from hotsos.core.log import log +from hotsos.core.utils import sorted_dict class NullCache(): @@ -88,6 +90,12 @@ def __init__(self, service_exprs, ps_allow_relative=True): self._ps_allow_relative = ps_allow_relative self._service_exprs = set(service_exprs) + @property + @abc.abstractmethod + def _service_manager_type(self): + """ A string name representing the type of service manager e.g. + 'systemd' """ + def get_cmd_from_ps_line(self, line, expr): """ Match a command in ps output line. @@ -111,23 +119,73 @@ def get_cmd_from_ps_line(self, line, expr): @property @abc.abstractmethod - def services(self): - """ Return a dictionary of identified services and their state. """ + def _service_filtered_ps(self): + """ Return a list ps entries corresponding to services. """ - @property - @abc.abstractmethod + @cached_property def processes(self): """ - Return a dictionary of processes associated with identified - services. + Identify running processes from ps that are associated with resolved + services. The search pattern used to identify a service is also + used to match the process binaryc/cmd name. + + Accounts for different types of process cmd path e.g. + + /snap//1830/ + /usr/bin/ + + and filter e.g. + + /var/lib/ and /var/log/ + + Returns a dictionary of process names along with the number of each. """ + _proc_info = {} + for line in self._service_filtered_ps: + for expr in self._service_exprs: + cmd = self.get_cmd_from_ps_line(line, expr) + if not cmd: + continue + + if cmd in _proc_info: + _proc_info[cmd] += 1 + else: + _proc_info[cmd] = 1 + + return _proc_info @property @abc.abstractmethod + def services(self): + """ Return a dictionary of identified services and their state. """ + + @property + def _service_info(self): + """Return a dictionary of services grouped by state. """ + info = {} + for svc, obj in sorted_dict(self.services).items(): + state = obj.state + if state not in info: + info[state] = [] + + info[state].append(svc) + + return info + + @property + def _process_info(self): + """Return a list of processes associated with services. """ + return [f"{name} ({count})" + for name, count in sorted_dict(self.processes).items()] + + @property def summary(self): - """ Return a dictionary summary of this class i.e. services, - their state and associated processes. """ + Output a dict summary of this class i.e. services, their state and any + processes run by them. + """ + return {self._service_manager_type: self._service_info, + 'ps': self._process_info} class NullSource(): diff --git a/hotsos/core/host_helpers/pebble.py b/hotsos/core/host_helpers/pebble.py index ef333406d..dc08f5d92 100644 --- a/hotsos/core/host_helpers/pebble.py +++ b/hotsos/core/host_helpers/pebble.py @@ -5,7 +5,6 @@ from hotsos.core.factory import FactoryBase from hotsos.core.host_helpers import CLIHelper from hotsos.core.host_helpers.common import ServiceManagerBase -from hotsos.core.utils import sorted_dict class PebbleService(): @@ -21,6 +20,10 @@ def __repr__(self): class PebbleHelper(ServiceManagerBase): """ Helper class used to query pebble services. """ + @property + def _service_manager_type(self): + return 'pebble' + @cached_property def services(self): """ Return a dict of identified pebble services and their state. """ @@ -38,62 +41,8 @@ def services(self): return _services @cached_property - def processes(self): - """ - Identify running processes from ps that are associated with resolved - pebble services. The search pattern used to identify a service is also - used to match the process binary/cmd name. - - Accounts for different types of process cmd path e.g. - - /snap//1830/ - /usr/bin/ - - and filter e.g. - - /var/lib/ and /var/log/ - - Returns a dictionary of process names along with the number of each. - """ - _proc_info = {} - for line in CLIHelper().ps(): - for expr in self._service_exprs: - cmd = self.get_cmd_from_ps_line(line, expr) - if cmd: - if cmd in _proc_info: - _proc_info[cmd] += 1 - else: - _proc_info[cmd] = 1 - - return _proc_info - - @property - def _service_info(self): - """Return a dictionary of pebble services grouped by state. """ - info = {} - for svc, obj in sorted_dict(self.services).items(): - state = obj.state - if state not in info: - info[state] = [] - - info[state].append(svc) - - return info - - @property - def _process_info(self): - """Return a list of processes associated with services. """ - return [f"{name} ({count})" - for name, count in sorted_dict(self.processes).items()] - - @property - def summary(self): - """ - Output a dict summary of this class i.e. services, their state and any - processes run by them. - """ - return {'pebble': self._service_info, - 'ps': self._process_info} + def _service_filtered_ps(self): + return CLIHelper().ps() class ServiceFactory(FactoryBase): diff --git a/hotsos/core/host_helpers/systemd.py b/hotsos/core/host_helpers/systemd.py index 783340193..f7575c57f 100644 --- a/hotsos/core/host_helpers/systemd.py +++ b/hotsos/core/host_helpers/systemd.py @@ -19,7 +19,6 @@ from hotsos.core.host_helpers import CLIHelper, CLIHelperFile from hotsos.core.host_helpers.common import ServiceManagerBase from hotsos.core.log import log -from hotsos.core.utils import sorted_dict class SystemdService(): @@ -187,6 +186,10 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self._cached_unit_files_exprs = {} + @property + def _service_manager_type(self): + return 'systemd' + @cached_property def _systemctl_list_units(self): return CLIHelper().systemctl_list_units() @@ -350,66 +353,6 @@ def _service_filtered_ps(self): return ps_filtered - @cached_property - def processes(self): - """ - Identify running processes from ps that are associated with resolved - systemd services. The search pattern used to identify a service is also - used to match the process binaryc/cmd name. - - Accounts for different types of process cmd path e.g. - - /snap//1830/ - /usr/bin/ - - and filter e.g. - - /var/lib/ and /var/log/ - - Returns a dictionary of process names along with the number of each. - """ - _proc_info = {} - for line in self._service_filtered_ps: - for expr in self._service_exprs: - cmd = self.get_cmd_from_ps_line(line, expr) - if not cmd: - continue - - if cmd in _proc_info: - _proc_info[cmd] += 1 - else: - _proc_info[cmd] = 1 - - return _proc_info - - @property - def _service_info(self): - """Return a dictionary of systemd services grouped by state. """ - info = {} - for svc, obj in sorted_dict(self.services).items(): - state = obj.state - if state not in info: - info[state] = [] - - info[state].append(svc) - - return info - - @property - def _process_info(self): - """Return a list of processes associated with services. """ - return [f"{name} ({count})" - for name, count in sorted_dict(self.processes).items()] - - @property - def summary(self): - """ - Output a dict summary of this class i.e. services, their state and any - processes run by them. - """ - return {'systemd': self._service_info, - 'ps': self._process_info} - class ServiceFactory(FactoryBase): """ diff --git a/hotsos/core/plugins/juju/common.py b/hotsos/core/plugins/juju/common.py index 53273480e..b4ecf67ab 100644 --- a/hotsos/core/plugins/juju/common.py +++ b/hotsos/core/plugins/juju/common.py @@ -24,6 +24,13 @@ def __init__(self, *args, **kwargs): # this is needed for juju scenarios self.systemd_processes = self.systemd.processes + @property + def version(self): + if self.machine: + return self.machine.version + + return "unknown" + @property def plugin_runnable(self): return os.path.exists(self.juju_lib_path) diff --git a/hotsos/core/plugins/mysql.py b/hotsos/core/plugins/mysql.py index f61073bc7..b00e5166e 100644 --- a/hotsos/core/plugins/mysql.py +++ b/hotsos/core/plugins/mysql.py @@ -24,13 +24,13 @@ class MySQLChecks(plugintools.PluginPartBase): def __init__(self, *args, **kwargs): super().__init__() - self.apt_info = APTPackageHelper(core_pkgs=CORE_APT) + self.apt = APTPackageHelper(core_pkgs=CORE_APT) self.pebble = PebbleHelper(service_exprs=MYSQL_SVC_EXPRS) self.systemd = SystemdHelper(service_exprs=MYSQL_SVC_EXPRS) @property def plugin_runnable(self): - return self.apt_info.core is not None + return self.apt.core is not None class MySQLConfig(host_helpers.IniConfigBase): diff --git a/hotsos/core/plugins/storage/bcache.py b/hotsos/core/plugins/storage/bcache.py index d4cc3b322..657fd7f8f 100644 --- a/hotsos/core/plugins/storage/bcache.py +++ b/hotsos/core/plugins/storage/bcache.py @@ -272,6 +272,11 @@ def cache_available_percent(self): class BcacheChecks(BcacheBase): """ Bcache checks. """ + + @property + def summary_subkey_include_default_entries(self): + return True + @property def summary_subkey(self): return 'bcache' diff --git a/hotsos/core/plugins/storage/ceph/common.py b/hotsos/core/plugins/storage/ceph/common.py index f4efff051..350cc03f7 100644 --- a/hotsos/core/plugins/storage/ceph/common.py +++ b/hotsos/core/plugins/storage/ceph/common.py @@ -148,6 +148,10 @@ def __init__(self, *args, **kwargs): self.systemd = SystemdHelper(service_exprs=CEPH_SERVICES_EXPRS) self.cluster = CephCluster() + @property + def summary_subkey_include_default_entries(self): + return True + @property def summary_subkey(self): return 'ceph' diff --git a/hotsos/core/plugintools.py b/hotsos/core/plugintools.py index 7e01767e2..bae95e640 100644 --- a/hotsos/core/plugintools.py +++ b/hotsos/core/plugintools.py @@ -1,4 +1,5 @@ import os +from enum import IntEnum, auto import yaml from jinja2 import FileSystemLoader, Environment @@ -215,6 +216,28 @@ class ApplicationBase(metaclass=PluginRegistryMeta): """ Base class for all plugins representing an application. """ + def __init__(self, *args, **kwargs): + self.apt = None # APTPackageHelper + self.snaps = None # SnapPackageHelper + self.docker = None # DockerImageHelper + self.pebble = None # PebbleHelper + self.systemd = None # SystemdHelper + super().__init__(*args, **kwargs) + + @property + def version(self): + """ Optional application version. """ + return None + + @property + def release_name(self): + """ Optional application release_name. """ + return None + + @property + def days_to_eol(self): + """ Optional application days_to_eol. """ + return None @property def bind_interfaces(self): @@ -224,6 +247,101 @@ def bind_interfaces(self): """ +def summary_entry(name, index=0): + """Decorate a class member function to indicate that it is a summary + function. Summary functions are automatically discovered by the + PluginPartBase class and will be part of the summary output.""" + + def real_decorator(func): + # Store the summary metadata information on the function itself. + func.__summary_elem_name = name # pylint: disable=protected-access + func.__summary_elem_index = index # pylint: disable=protected-access + return func + return real_decorator + + +def get_min_available_entry_index(): + """ Return first index available after default entries. """ + return DefaultSummaryEntryIndexes.AVAILABLE + + +class DefaultSummaryEntryIndexes(IntEnum): + """ Indexes used for default summary entries. """ + VERSION = 0 + RELEASE = auto() + SERVICES = auto() + SNAPS = auto() + DPKG = auto() + DOCKER_IMAGES = auto() + # The following index represents the first one that is available for + # implementations to use and they implement their indexes as an offset from + # this value so that if it is increased they are automatically increased. + AVAILABLE = auto() + + +class SummaryBase(ApplicationBase): + """ Common structure for application summary output. + + Individual application plugins should implement this class and extend to + include information specific to their application. + """ + @classmethod + def default_summary_entries(cls): + return [e for e in dir(SummaryBase) if str(e).startswith('summary_')] + + @summary_entry('version', DefaultSummaryEntryIndexes.VERSION) + def summary_version(self): + return self.version + + @summary_entry('release', DefaultSummaryEntryIndexes.RELEASE) + def summary_release(self): + if not all([self.release_name is not None, + self.days_to_eol is not None]): + return None + + return {'name': self.release_name, + 'days-to-eol': self.days_to_eol} + + @summary_entry('services', DefaultSummaryEntryIndexes.SERVICES) + def summary_services(self): + """Get string info for running services.""" + if self.systemd is not None and self.systemd.services: + return self.systemd.summary + + if self.pebble is not None and self.pebble.services: + return self.pebble.summary + + return None + + @summary_entry('snaps', DefaultSummaryEntryIndexes.SNAPS) + def summary_snaps(self): + if self.snaps is None: + return None + + return self.snaps.all_formatted or None + + @summary_entry('dpkg', DefaultSummaryEntryIndexes.DPKG) + def summary_dpkg(self): + # require at least one core package to be installed to include + # this in the report. + if self.apt is not None and self.apt.core: + return self.apt.all_formatted + + return None + + @summary_entry('docker-images', DefaultSummaryEntryIndexes.DOCKER_IMAGES) + def summary_docker_images(self): + if self.docker is None: + return None + + # require at least one core image to be in-use to include + # this in the report. + if self.docker.core: + return self.docker.all_formatted + + return None + + class SummaryEntry(): """ Formatter to convert output to Markdown. """ @@ -324,20 +442,7 @@ def all(self): return {HotSOSConfig.plugin_name: parts} -def summary_entry(name, index=0): - """Decorate a class member function to indicate that it is a summary - function. Summary functions are automatically discovered by the - PluginPartBase class and will be part of the summary output.""" - - def real_decorator(func): - # Store the summary metadata information on the function itself. - func.__summary_elem_name = name # pylint: disable=protected-access - func.__summary_elem_index = index # pylint: disable=protected-access - return func - return real_decorator - - -class PluginPartBase(ApplicationBase): +class PluginPartBase(SummaryBase): """ This is the base class used for all plugins. Provides a standard set of methods that plugins will need as well as the @@ -375,12 +480,25 @@ def plugin_runnable(self): """ raise NotImplementedError + @property + def summary_subkey_include_default_entries(self): + """ + By default the default/base class summary entries will NOT be included + in the output of a summary subkey. To override this behaviour you must + override this property to return True. + """ + return False + @property def summary_subkey(self): """ This can be optionally implemented in order to have all output of the current part placed under the keyname provided by this property i.e. : : ... + + By default the default/base class summary entries will NOT be included + in the output of a summary subkey. To override this behaviour you must + override summary_subkey_include_default_entries to return True. """ return None @@ -395,8 +513,7 @@ def summary(self): """ return None - @classmethod - def get_summary_entries(cls): + def get_summary_entries(self): """Return a list of summary functions defined in the current class and all the derived classes. @@ -406,16 +523,24 @@ def get_summary_entries(cls): result = {} # Iterate through all attributes of BaseClass - for name in dir(cls): - # Get the attribute from the current class (cls) - attr = getattr(cls, name) + for name in dir(self.__class__): + # Get the attribute from the current class (self.__class__) + attr = getattr(self.__class__, name) # Check if it's a function and has the specified attribute - if callable(attr) and hasattr(attr, '__summary_elem_name'): - name = getattr(attr, '__summary_elem_name') - index = getattr(attr, '__summary_elem_index') - log.debug("Found summary entry %s with idx %d", - name, index) - result[name] = [attr.__name__, index] + if not (callable(attr) and hasattr(attr, '__summary_elem_name')): + continue + + if not self.summary_subkey_include_default_entries: + # Exclude base class (default) entries from subkey parts + if (self.summary_subkey is not None and + attr.__name__ in self.default_summary_entries()): + continue + + name = getattr(attr, '__summary_elem_name') + index = getattr(attr, '__summary_elem_index') + log.debug("Found summary entry %s with idx %d", + name, index) + result[name] = [attr.__name__, index] return result @property diff --git a/hotsos/core/ycheck/engine/properties/checks.py b/hotsos/core/ycheck/engine/properties/checks.py index 2ffd691d3..560920c1a 100644 --- a/hotsos/core/ycheck/engine/properties/checks.py +++ b/hotsos/core/ycheck/engine/properties/checks.py @@ -151,6 +151,7 @@ def set_search_cache_info(self, results): @cached_property def result(self): + # pylint: disable=duplicate-code try: # Pass this object down to descendants so that they have access to # its cache etc. Note this is modifying global context but since diff --git a/hotsos/core/ycheck/engine/properties/conclusions.py b/hotsos/core/ycheck/engine/properties/conclusions.py index b15147e3a..ea73f6e02 100644 --- a/hotsos/core/ycheck/engine/properties/conclusions.py +++ b/hotsos/core/ycheck/engine/properties/conclusions.py @@ -160,6 +160,7 @@ class YPropertyDecision(DecisionBase, YPropertyMappedOverrideBase): @property def result(self): + # pylint: disable=duplicate-code results = [] try: stop_executon = False diff --git a/hotsos/core/ycheck/engine/properties/requires/requires.py b/hotsos/core/ycheck/engine/properties/requires/requires.py index 30763a6e2..9e06313aa 100644 --- a/hotsos/core/ycheck/engine/properties/requires/requires.py +++ b/hotsos/core/ycheck/engine/properties/requires/requires.py @@ -108,6 +108,7 @@ def result(self): or list of requirements. List may contain individual requirements or groups. """ + # pylint: disable=duplicate-code log.debug("running requirement") try: results = [] diff --git a/hotsos/core/ycheck/engine/properties/requires/types/apt.py b/hotsos/core/ycheck/engine/properties/requires/types/apt.py index 64a47e7b4..64e9a1793 100644 --- a/hotsos/core/ycheck/engine/properties/requires/types/apt.py +++ b/hotsos/core/ycheck/engine/properties/requires/types/apt.py @@ -36,6 +36,7 @@ class YRequirementTypeAPT(YRequirementTypeBase): @property @intercept_exception def _result(self): + # pylint: disable=duplicate-code _result = True items = APTCheckItems(self.content) diff --git a/hotsos/core/ycheck/engine/properties/requires/types/pebble.py b/hotsos/core/ycheck/engine/properties/requires/types/pebble.py index 8f509b4ed..b3ee4418a 100644 --- a/hotsos/core/ycheck/engine/properties/requires/types/pebble.py +++ b/hotsos/core/ycheck/engine/properties/requires/types/pebble.py @@ -1,9 +1,8 @@ from hotsos.core.log import log from hotsos.core.host_helpers import PebbleHelper -from hotsos.core.ycheck.engine.properties.requires import ( - intercept_exception, - ServiceCheckItemsBase, - YRequirementTypeBase, +from hotsos.core.ycheck.engine.properties.requires import ServiceCheckItemsBase +from hotsos.core.ycheck.engine.properties.requires.types import ( + service_manager_common ) @@ -16,64 +15,40 @@ def _svcs_info(self): return PebbleHelper(self._svcs_all) -class YRequirementTypePebble(YRequirementTypeBase): +class YRequirementTypePebble(service_manager_common.ServiceManagerTypeBase): """ Pebble requires type property. Provides support for defining checks on pebble service manager resources. """ override_keys = ['pebble'] override_autoregister = True + default_op = 'eq' + + def _check_item_settings(self, svc, svc_obj, settings, cache_info, + all_items): + processes = None + if isinstance(settings, str): + state = settings + ops = [[self.default_op, state]] + else: + processes = settings.get('processes') + if processes: + log.debug("checking systemd service processes: %s", processes) - @property - @intercept_exception - def _result(self): - cache_info = {} - default_op = 'eq' - _result = True - - items = PebbleServiceCheckItems(self.content) - if not items.not_installed: - for svc, settings in items: - svc_obj = items.installed[svc] - cache_info[svc] = {'actual': svc_obj.state} - if settings is None: - continue - - processes = None - if isinstance(settings, str): - state = settings - ops = [[default_op, state]] - else: - processes = settings.get('processes') - if processes: - log.debug("checking service processes: %s", processes) - - op = settings.get('op', default_op) - if 'state' in settings: - ops = [[op, settings.get('state')]] - else: - ops = [] + op = settings.get('op', self.default_op) + if 'state' in settings: + ops = [[op, settings.get('state')]] + else: + ops = [] - if processes and not items.processes_running(processes): - log.debug("one or more processes not running " - "- %s", ', '.join(processes)) - _result = False - else: - cache_info[svc]['ops'] = self.ops_to_str(ops) - _result = self.self.apply_ops(ops, input=svc_obj.state) + if processes and not all_items.processes_running(processes): + log.debug("one or more pebble service processes not running " + "- %s", ', '.join(processes)) + return False - if not _result: - # bail on first fail - break - else: - log.debug("one or more services not installed so returning False " - "- %s", ', '.join(items.not_installed)) - # bail on first fail i.e. if any not installed - _result = False + cache_info[svc]['ops'] = self.ops_to_str(ops) + return self.self.apply_ops(ops, input=svc_obj.state) - # this will represent what we have actually checked - self.cache.set('services', ', '.join(cache_info)) - svcs = [f"{svc}={settings}" for svc, settings in items] - log.debug('requirement check: %s (result=%s)', - ', '.join(svcs), _result) - return _result + @property + def items_to_check(self): + return PebbleServiceCheckItems(self.content) diff --git a/hotsos/core/ycheck/engine/properties/requires/types/service_manager_common.py b/hotsos/core/ycheck/engine/properties/requires/types/service_manager_common.py new file mode 100644 index 000000000..400091678 --- /dev/null +++ b/hotsos/core/ycheck/engine/properties/requires/types/service_manager_common.py @@ -0,0 +1,56 @@ +import abc + +from hotsos.core.log import log +from hotsos.core.ycheck.engine.properties.requires import ( + intercept_exception, + YRequirementTypeBase, +) + + +class ServiceManagerTypeBase(YRequirementTypeBase): + """ + Base class for implementations for service manager requirement types such + as systemd, pebble etc. + """ + default_op = 'eq' + + @property + @abc.abstractmethod + def items_to_check(self): + """ Return check items we want to iterate over until we find an error. + + The implementing class should have an accompanying implementation of + ServiceCheckItemsBase and return that as an object here. + """ + + @property + @intercept_exception + def _result(self): + cache_info = {} + _result = True + + items = self.items_to_check + if not items.not_installed: + for svc, settings in items: + svc_obj = items.installed[svc] + cache_info[svc] = {'actual': svc_obj.state} + if settings is None: + continue + + _result = self._check_item_settings(svc, svc_obj, settings, + cache_info, items) + if not _result: + # bail on first fail + break + else: + log.debug("one or more services not installed so returning False " + "- %s", ', '.join(items.not_installed)) + # bail on first fail i.e. if any not installed + _result = False + + # this will represent what we have actually checked + self.cache.set('services', ', '.join(cache_info)) + svcs = [f"{svc}={settings}" for svc, settings in items] + log.debug('requirement check: %s (result=%s)', + ', '.join(svcs), _result) + return _result diff --git a/hotsos/core/ycheck/engine/properties/requires/types/snap.py b/hotsos/core/ycheck/engine/properties/requires/types/snap.py index 46ec4aaaa..1ca18967c 100644 --- a/hotsos/core/ycheck/engine/properties/requires/types/snap.py +++ b/hotsos/core/ycheck/engine/properties/requires/types/snap.py @@ -97,6 +97,7 @@ def channel(self): @property @intercept_exception def _result(self): + # pylint: disable=duplicate-code _result = True items = SnapCheckItems(self.content) # bail on first fail i.e. if any not installed diff --git a/hotsos/core/ycheck/engine/properties/requires/types/systemd.py b/hotsos/core/ycheck/engine/properties/requires/types/systemd.py index 87115344f..8dabd1a2c 100644 --- a/hotsos/core/ycheck/engine/properties/requires/types/systemd.py +++ b/hotsos/core/ycheck/engine/properties/requires/types/systemd.py @@ -2,10 +2,9 @@ from hotsos.core.host_helpers import SystemdHelper from hotsos.core.log import log -from hotsos.core.ycheck.engine.properties.requires import ( - intercept_exception, - ServiceCheckItemsBase, - YRequirementTypeBase, +from hotsos.core.ycheck.engine.properties.requires import ServiceCheckItemsBase +from hotsos.core.ycheck.engine.properties.requires.types import ( + service_manager_common ) @@ -18,13 +17,14 @@ def _svcs_info(self): return SystemdHelper(self._svcs_all) -class YRequirementTypeSystemd(YRequirementTypeBase): +class YRequirementTypeSystemd(service_manager_common.ServiceManagerTypeBase): """ Systemd requires type property. Provides support for defining checks on systemd resources. """ override_keys = ['systemd'] override_autoregister = True + default_op = 'eq' def _check_service(self, svc, ops, started_after=None): """ @@ -73,61 +73,37 @@ def _check_service(self, svc, ops, started_after=None): return self.apply_ops(ops, opinput=svc.state) - @property - @intercept_exception - def _result(self): - cache_info = {} - default_op = 'eq' - _result = True - - items = SystemdServiceCheckItems(self.content) - if not items.not_installed: - for svc, settings in items: - svc_obj = items.installed[svc] - cache_info[svc] = {'actual': svc_obj.state} - if settings is None: - continue - - processes = None - started_after_obj = None - if isinstance(settings, str): - state = settings - ops = [[default_op, state]] - else: - processes = settings.get('processes') - if processes: - log.debug("checking service processes: %s", processes) - - op = settings.get('op', default_op) - started_after = settings.get('started-after') - started_after_obj = items.installed.get(started_after) - if 'state' in settings: - ops = [[op, settings.get('state')]] - else: - ops = [] - - if processes and not items.processes_running(processes): - log.debug("one or more processes not running " - "- %s", ', '.join(processes)) - _result = False - else: - cache_info[svc]['ops'] = self.ops_to_str(ops) - _result = self._check_service( - svc_obj, ops, - started_after=started_after_obj) - if not _result: - # bail on first fail - break + def _check_item_settings(self, svc, svc_obj, settings, cache_info, + all_items): + # pylint: disable=duplicate-code + processes = None + started_after_obj = None + if isinstance(settings, str): + state = settings + ops = [[self.default_op, state]] else: - log.debug("one or more services not installed so returning False " - "- %s", ', '.join(items.not_installed)) - # bail on first fail i.e. if any not installed - _result = False + processes = settings.get('processes') + if processes: + log.debug("checking systemd service processes: %s", processes) + processes = settings.get('processes') + + op = settings.get('op', self.default_op) + started_after = settings.get('started-after') + started_after_obj = all_items.installed.get(started_after) + if 'state' in settings: + ops = [[op, settings.get('state')]] + else: + ops = [] - # this will represent what we have actually checked - self.cache.set('services', ', '.join(cache_info)) + if processes and not all_items.processes_running(processes): + log.debug("one or more systemd service processes not running " + "- %s", ', '.join(processes)) + return False - svcs = [f"{svc}={settings}" for svc, settings in items] - log.debug('requirement check: %s (result=%s)', - ', '.join(svcs), _result) - return _result + cache_info[svc]['ops'] = self.ops_to_str(ops) + return self._check_service(svc_obj, ops, + started_after=started_after_obj) + + @property + def items_to_check(self): + return SystemdServiceCheckItems(self.content) diff --git a/hotsos/plugin_extensions/juju/summary.py b/hotsos/plugin_extensions/juju/summary.py index 7a33a69ce..640ee01bc 100644 --- a/hotsos/plugin_extensions/juju/summary.py +++ b/hotsos/plugin_extensions/juju/summary.py @@ -13,7 +13,10 @@ ) from hotsos.core.utils import sorted_dict from hotsos.core.search import CommonTimestampMatcher -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class UnitLogInfo(): @@ -101,30 +104,14 @@ class JujuSummary(JujuChecks): """ Implementation of Juju summary. """ summary_part_index = 0 - @summary_entry('services', 0) - def summary_services(self): - if self.systemd.services: - return self.systemd.summary - if self.pebble.services: - return self.pebble.summary - - return None - - @summary_entry('version', 1) - def summary_version(self): - if self.machine: - return self.machine.version - - return "unknown" - - @summary_entry('machine', 2) + @summary_entry('machine', get_min_available_entry_index()) def summary_machine(self): if self.machine: return self.machine.id return "unknown" - @summary_entry('units', 3) + @summary_entry('units', get_min_available_entry_index() + 1) def summary_units(self): if not self.units: return None diff --git a/hotsos/plugin_extensions/kernel/summary.py b/hotsos/plugin_extensions/kernel/summary.py index 2f7e9eedc..c1f07eb6b 100644 --- a/hotsos/plugin_extensions/kernel/summary.py +++ b/hotsos/plugin_extensions/kernel/summary.py @@ -11,6 +11,12 @@ class KernelSummary(KernelChecks): """ Implementation of Kernel summary. """ summary_part_index = 0 + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. + @property def cpu_info(self): cpu = CPU() diff --git a/hotsos/plugin_extensions/kubernetes/summary.py b/hotsos/plugin_extensions/kubernetes/summary.py index ca4b10304..de685b5e8 100644 --- a/hotsos/plugin_extensions/kubernetes/summary.py +++ b/hotsos/plugin_extensions/kubernetes/summary.py @@ -1,37 +1,37 @@ from hotsos.core.plugins.kubernetes import KubernetesChecks -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, + DefaultSummaryEntryIndexes, +) class KubernetesSummary(KubernetesChecks): """ Implementation of Kubernetes summary. """ summary_part_index = 0 - @summary_entry('services', 1) - def summary_services(self): - if self.systemd.services: - return self.systemd.summary - if self.pebble.services: - return self.pebble.summary + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. - return None - - @summary_entry('snaps', 2) - def summary_snaps(self): - return self.snaps.all_formatted or None - - @summary_entry('dpkg', 3) + @summary_entry('dpkg', DefaultSummaryEntryIndexes.DPKG) def summary_dpkg(self): + """ + Override the default entry to include all discovered packages. + """ return self.apt.all_formatted or None - @summary_entry('pods', 4) + @summary_entry('pods', get_min_available_entry_index()) def summary_pods(self): return self.pods or None - @summary_entry('containers', 5) + @summary_entry('containers', get_min_available_entry_index() + 1) def summary_containers(self): return self.containers or None - @summary_entry('flannel', 6) + @summary_entry('flannel', get_min_available_entry_index() + 2) def summary_flannel(self): info = {} for port in self.flannel_ports: diff --git a/hotsos/plugin_extensions/lxd/summary.py b/hotsos/plugin_extensions/lxd/summary.py index 75bd24189..ec6856937 100644 --- a/hotsos/plugin_extensions/lxd/summary.py +++ b/hotsos/plugin_extensions/lxd/summary.py @@ -1,33 +1,21 @@ from hotsos.core.plugins.lxd import LXD, LXDChecks -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class LXDSummary(LXDChecks): """ Implementation of LXD summary. """ summary_part_index = 0 - @summary_entry('services', 0) - def summary_services(self): - if self.systemd.services: - return self.systemd.summary - - return None - - @summary_entry('snaps', 1) - def summary_snaps(self): - if self.snaps: - return self.snaps.all_formatted - - return None - - @summary_entry('dpkg', 2) - def summary_dpkg(self): - if self.apt: - return self.apt.all_formatted - - return None + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. @staticmethod - @summary_entry('instances', 3) + @summary_entry('instances', get_min_available_entry_index()) def summary_instances(): return LXD().instances or None diff --git a/hotsos/plugin_extensions/maas/summary.py b/hotsos/plugin_extensions/maas/summary.py index f6e680959..d52bffa72 100644 --- a/hotsos/plugin_extensions/maas/summary.py +++ b/hotsos/plugin_extensions/maas/summary.py @@ -1,28 +1,14 @@ from hotsos.core.plugins.maas import MAASChecks -from hotsos.core.plugintools import summary_entry class MAASSummary(MAASChecks): """ Implementation of MAAS summary. """ summary_part_index = 0 - @summary_entry('services', 0) - def summary_services(self): - if self.systemd.services: - return self.systemd.summary + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. - return None - - @summary_entry('dpkg', 1) - def summary_dpkg(self): - if self.apt.core: - return self.apt.all_formatted - - return None - - @summary_entry('snaps', 2) - def summary_snaps(self): - if self.snaps.core: - return self.snaps.all_formatted - - return None + # No custom entries defined here yet so currently relying on the defaults. diff --git a/hotsos/plugin_extensions/mysql/summary.py b/hotsos/plugin_extensions/mysql/summary.py index 6ec9de25b..ada7b2b74 100644 --- a/hotsos/plugin_extensions/mysql/summary.py +++ b/hotsos/plugin_extensions/mysql/summary.py @@ -1,23 +1,14 @@ from hotsos.core.plugins.mysql import MySQLChecks -from hotsos.core.plugintools import summary_entry class MySQLSummary(MySQLChecks): """ Implementation of MySQL summary. """ summary_part_index = 0 - @summary_entry('services', 0) - def summary_services(self): - if self.systemd.services: - return self.systemd.summary - if self.pebble.services: - return self.pebble.summary + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. - return None - - @summary_entry('dpkg', 1) - def summary_dpkg(self): - if self.apt_info.core: - return self.apt_info.all_formatted - - return None + # No custom entries defined here yet so currently relying on the defaults. diff --git a/hotsos/plugin_extensions/openstack/agent/events.py b/hotsos/plugin_extensions/openstack/agent/events.py index 5553b0763..abea58571 100644 --- a/hotsos/plugin_extensions/openstack/agent/events.py +++ b/hotsos/plugin_extensions/openstack/agent/events.py @@ -18,7 +18,10 @@ from hotsos.core.plugins.openstack.neutron import NeutronHAInfo from hotsos.core import utils from hotsos.core.utils import sorted_dict -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) VRRP_TRANSITION_WARN_THRESHOLD = 8 @@ -78,7 +81,7 @@ class ApacheEventChecks(OpenstackEventHandlerBase): summary_part_index = 8 event_group = 'apache' - @summary_entry('agent-checks', 101) + @summary_entry('agent-checks', get_min_available_entry_index() + 101) def summary_agent_checks(self): out = self.run() if out: @@ -107,7 +110,7 @@ class APIEvents(OpenstackEventHandlerBase): summary_part_index = 9 event_group = 'http-requests' - @summary_entry('api-info', 100) + @summary_entry('api-info', get_min_available_entry_index() + 100) def summary_api_info(self): out = self.run() if out: @@ -215,7 +218,7 @@ class NeutronAgentEventChecks(OpenstackEventHandlerBase): summary_part_index = 7 event_group = 'neutron.agents' - @summary_entry('agent-checks', 101) + @summary_entry('agent-checks', get_min_available_entry_index() + 101) def summary_agent_checks(self): # NOTE: order is important here agents = ['neutron-server', 'neutron-l3-agent', 'neutron-ovs-agent'] @@ -269,7 +272,7 @@ class OctaviaAgentEventChecks(OpenstackEventHandlerBase): summary_part_index = 10 event_group = 'octavia' - @summary_entry('agent-checks', 101) + @summary_entry('agent-checks', get_min_available_entry_index() + 101) def summary_agent_checks(self): out = self.run() if out: @@ -296,7 +299,7 @@ class NovaComputeEventChecks(OpenstackEventHandlerBase): summary_part_index = 11 event_group = 'nova.nova-compute' - @summary_entry('agent-checks', 101) + @summary_entry('agent-checks', get_min_available_entry_index() + 101) def summary_agent_checks(self): out = self.run() if out: @@ -329,7 +332,7 @@ class AgentApparmorChecks(OpenstackEventHandlerBase): summary_part_index = 12 event_group = 'apparmor' - @summary_entry('agent-checks', 101) + @summary_entry('agent-checks', get_min_available_entry_index() + 101) def summary_agent_checks(self): out = self.run() if out: @@ -408,7 +411,7 @@ class NeutronL3HAEventChecks(OpenstackEventHandlerBase): summary_part_index = 13 event_group = 'neutron.ml2-routers' - @summary_entry('agent-checks', 101) + @summary_entry('agent-checks', get_min_available_entry_index() + 101) def summary_agent_checks(self): out = self.run() if out: diff --git a/hotsos/plugin_extensions/openstack/agent/exceptions.py b/hotsos/plugin_extensions/openstack/agent/exceptions.py index 8be3c4a75..d44523942 100644 --- a/hotsos/plugin_extensions/openstack/agent/exceptions.py +++ b/hotsos/plugin_extensions/openstack/agent/exceptions.py @@ -11,7 +11,10 @@ SearchConstraintSearchSince, ) from hotsos.core.search import CommonTimestampMatcher -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class AgentExceptionCheckResults(UserDict): @@ -276,7 +279,7 @@ def agent_results(self): self._agent_results = self._run(self.searchobj.run()) return self._agent_results - @summary_entry('agent-exceptions', 200) + @summary_entry('agent-exceptions', get_min_available_entry_index() + 200) def summary_agent_exceptions(self): """ Only ERROR level exceptions @@ -290,7 +293,7 @@ def summary_agent_exceptions(self): return None - @summary_entry('agent-warnings', 201) + @summary_entry('agent-warnings', get_min_available_entry_index() + 201) def summary_agent_warnings(self): """ Only WARNING level exceptions diff --git a/hotsos/plugin_extensions/openstack/nova_external_events.py b/hotsos/plugin_extensions/openstack/nova_external_events.py index aba7e132d..2e0b4774f 100644 --- a/hotsos/plugin_extensions/openstack/nova_external_events.py +++ b/hotsos/plugin_extensions/openstack/nova_external_events.py @@ -8,7 +8,10 @@ SearchConstraintSearchSince, ) from hotsos.core.search import CommonTimestampMatcher -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) EXT_EVENT_META = {'network-vif-plugged': {'stages_keys': ['Preparing', 'Received', @@ -87,6 +90,7 @@ class NovaExternalEventChecks(OpenstackEventHandlerBase): event_group = 'nova.external-events' summary_part_index = 1 - @summary_entry('os-server-external-events', 8) + @summary_entry('os-server-external-events', + get_min_available_entry_index() + 4) def summary_os_server_external_events(self): return self.run() diff --git a/hotsos/plugin_extensions/openstack/service_features.py b/hotsos/plugin_extensions/openstack/service_features.py index 9762dfe2a..cb5e97ce3 100644 --- a/hotsos/plugin_extensions/openstack/service_features.py +++ b/hotsos/plugin_extensions/openstack/service_features.py @@ -1,6 +1,9 @@ from hotsos.core.log import log from hotsos.core.plugins.openstack.common import OpenStackChecks -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) FEATURES = {'neutron': { 'main': { @@ -72,7 +75,7 @@ def _get_module_features(service, module, sections, cfg): return module_features - @summary_entry('features', 7) + @summary_entry('features', get_min_available_entry_index() + 3) def summary_features(self): """ This is used to display whether or not specific features are enabled. diff --git a/hotsos/plugin_extensions/openstack/service_network_checks.py b/hotsos/plugin_extensions/openstack/service_network_checks.py index 054991212..992be73f2 100644 --- a/hotsos/plugin_extensions/openstack/service_network_checks.py +++ b/hotsos/plugin_extensions/openstack/service_network_checks.py @@ -12,7 +12,10 @@ IssuesManager, OpenstackWarning, ) -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class OpenstackNetworkChecks(OpenStackChecks): @@ -83,7 +86,7 @@ def get_phy_port_health_info(self): return port_health_info - @summary_entry('config', 9) + @summary_entry('config', get_min_available_entry_index() + 5) def summary_config(self): config_info = self.get_config_info() if config_info: @@ -91,7 +94,7 @@ def summary_config(self): return None - @summary_entry('phy-port-health', 10) + @summary_entry('phy-port-health', get_min_available_entry_index() + 6) def summary_phy_port_health(self): port_health_info = self.get_phy_port_health_info() if port_health_info: @@ -99,7 +102,7 @@ def summary_phy_port_health(self): return None - @summary_entry('namespaces', 11) + @summary_entry('namespaces', get_min_available_entry_index() + 7) def summary_namespaces(self): """Populate namespace information dict.""" ns_info = {} @@ -143,7 +146,7 @@ def _get_router_iface_mtus(self): return {prefix: list(mtus) for prefix, mtus in router_mtus.items()} - @summary_entry('router-port-mtus', 12) + @summary_entry('router-port-mtus', get_min_available_entry_index() + 8) def summary_router_port_mtus(self): """ Provide a summary of ml2-ovs router port mtus. """ project = getattr(self, 'neutron') @@ -181,7 +184,7 @@ def summary_router_port_mtus(self): return router_mtus - @summary_entry('vm-port-health', 13) + @summary_entry('vm-port-health', get_min_available_entry_index() + 9) def summary_vm_port_health(self): """ For each instance get its ports and check port health, reporting on any outliers. """ diff --git a/hotsos/plugin_extensions/openstack/summary.py b/hotsos/plugin_extensions/openstack/summary.py index 14421eea6..883b5ddc9 100644 --- a/hotsos/plugin_extensions/openstack/summary.py +++ b/hotsos/plugin_extensions/openstack/summary.py @@ -1,47 +1,23 @@ from hotsos.core.plugins.openstack.common import OpenStackChecks from hotsos.core.plugins.openstack.neutron import NeutronHAInfo -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class OpenStackSummary(OpenStackChecks): """ Implementation of OpenStack summary. """ summary_part_index = 0 - @summary_entry('release', 0) - def summary_release(self): - return {'name': self.release_name, - 'days-to-eol': self.days_to_eol} - - @summary_entry('services', 1) - def summary_services(self): - """Get string info for running services.""" - if self.systemd.services: - return self.systemd.summary - if self.pebble.services: - return self.pebble.summary - - return None - - @summary_entry('dpkg', 2) - def summary_dpkg(self): - # require at least one core package to be installed to include - # this in the report. - if self.apt.core: - return self.apt.all_formatted - - return None - - @summary_entry('docker-images', 3) - def summary_docker_images(self): - # require at least one core image to be in-use to include - # this in the report. - if self.docker.core: - return self.docker.all_formatted - - return None + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. @staticmethod - @summary_entry('neutron-l3ha', 4) + @summary_entry('neutron-l3ha', get_min_available_entry_index()) def summary_neutron_l3ha(): routers = {} ha_info = NeutronHAInfo() diff --git a/hotsos/plugin_extensions/openstack/vm_info.py b/hotsos/plugin_extensions/openstack/vm_info.py index 70a1fdefa..0cda32a4a 100644 --- a/hotsos/plugin_extensions/openstack/vm_info.py +++ b/hotsos/plugin_extensions/openstack/vm_info.py @@ -8,14 +8,17 @@ ) from hotsos.core.plugins.openstack.nova import NovaLibvirt from hotsos.core import utils -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class OpenstackInstanceChecks(OpenStackChecks): """ Implements Openstack Nova instance checks. """ summary_part_index = 2 - @summary_entry('vm-info', 5) + @summary_entry('vm-info', get_min_available_entry_index() + 1) def summary_vm_info(self): _info = {} @@ -194,6 +197,6 @@ class NovaServerMigrationAnalysis(OpenstackEventHandlerBase): event_group = 'nova.migrations' summary_part_index = 3 - @summary_entry('nova-migrations', 6) + @summary_entry('nova-migrations', get_min_available_entry_index() + 2) def summary_nova_migrations(self): return self.run() diff --git a/hotsos/plugin_extensions/openvswitch/summary.py b/hotsos/plugin_extensions/openvswitch/summary.py index 7f62893f6..6cac15706 100644 --- a/hotsos/plugin_extensions/openvswitch/summary.py +++ b/hotsos/plugin_extensions/openvswitch/summary.py @@ -2,33 +2,28 @@ from hotsos.core.plugins.openvswitch import OpenvSwitchChecks from hotsos.core.plugins.openvswitch.ovn import OVNBase from hotsos.core.plugins.openvswitch.ovs import OpenvSwitchBase -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class OpenvSwitchSummary(OpenvSwitchChecks): """ Implementation of OpenvSwitch summary. """ summary_part_index = 0 + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.ovs = OpenvSwitchBase() self.ovn = OVNBase() - @summary_entry('services', 0) - def summary_services(self): - """Get string info for running daemons.""" - if self.systemd.services: - return self.systemd.summary - if self.pebble.services: - return self.pebble.summary - - return None - - @summary_entry('dpkg', 1) - def summary_dpkg(self): - return self.apt.all_formatted - - @summary_entry('config', 2) + @summary_entry('config', get_min_available_entry_index()) def summary_config(self): _config = {} if self.ovs.offload_enabled: @@ -44,7 +39,7 @@ def summary_config(self): return _config or None - @summary_entry('bridges', 3) + @summary_entry('bridges', get_min_available_entry_index() + 1) def summary_bridges(self): bridges = {} for bridge in self.ovs.bridges: @@ -75,11 +70,11 @@ def summary_bridges(self): return bridges or None - @summary_entry('tunnels', 4) + @summary_entry('tunnels', get_min_available_entry_index() + 2) def summary_tunnels(self): return self.ovs.tunnels or None - @summary_entry('ovn', 5) + @summary_entry('ovn', get_min_available_entry_index() + 3) def summary_ovn(self): info = {} if self.ovn.nbdb: diff --git a/hotsos/plugin_extensions/pacemaker/summary.py b/hotsos/plugin_extensions/pacemaker/summary.py index 8ddb9c794..267b4eee6 100644 --- a/hotsos/plugin_extensions/pacemaker/summary.py +++ b/hotsos/plugin_extensions/pacemaker/summary.py @@ -1,23 +1,21 @@ from hotsos.core.plugins.pacemaker import PacemakerChecks -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class PacemakerSummary(PacemakerChecks): """ Implementation of Pacemaker summary. """ summary_part_index = 0 - @summary_entry('services', 0) - def summary_services(self): - if self.systemd.services: - return self.systemd.summary + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. - return None - - @summary_entry('dpkg', 1) - def summary_dpkg(self): - return self.apt.all_formatted or None - - @summary_entry('nodes', 2) + @summary_entry('nodes', get_min_available_entry_index()) def summary_nodes(self): nodes = {} if self.online_nodes: diff --git a/hotsos/plugin_extensions/rabbitmq/summary.py b/hotsos/plugin_extensions/rabbitmq/summary.py index 4592ef942..7bd57ab9a 100644 --- a/hotsos/plugin_extensions/rabbitmq/summary.py +++ b/hotsos/plugin_extensions/rabbitmq/summary.py @@ -1,23 +1,19 @@ from hotsos.core.plugins.rabbitmq import RabbitMQChecks -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class RabbitMQSummary(RabbitMQChecks): """ Implementation of RabbitMQ summary. """ summary_part_index = 0 - @summary_entry('services', 0) - def summary_services(self): - if self.systemd.services: - return self.systemd.summary - if self.pebble.services: - return self.pebble.summary - - return None - - @summary_entry('dpkg', 1) - def summary_dpkg(self): - return self.apt.all_formatted or None + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. @property def queue_info(self): @@ -49,12 +45,12 @@ def queue_info(self): return None - @summary_entry('config', 2) + @summary_entry('config', get_min_available_entry_index()) def summary_config(self): setting = self.report.partition_handling or 'unknown' return {'cluster-partition-handling': setting} - @summary_entry('resources', 3) + @summary_entry('resources', get_min_available_entry_index() + 1) def summary_resources(self): resources = {} _queue_info = self.queue_info diff --git a/hotsos/plugin_extensions/sosreport/summary.py b/hotsos/plugin_extensions/sosreport/summary.py index ea0e92663..0b02b746d 100644 --- a/hotsos/plugin_extensions/sosreport/summary.py +++ b/hotsos/plugin_extensions/sosreport/summary.py @@ -1,26 +1,21 @@ from hotsos.core.plugins.sosreport import SOSReportChecks -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class SOSReportSummary(SOSReportChecks): """ Implementation of SOSReport summary. """ summary_part_index = 0 - @summary_entry('version', 0) - def summary_version(self): - if self.version is not None: - return self.version + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. - return None - - @summary_entry('dpkg', 1) - def summary_dpkg(self): - if self.apt.core: - return self.apt.all_formatted - - return None - - @summary_entry('plugin-timeouts', 2) + @summary_entry('plugin-timeouts', get_min_available_entry_index()) def summary_plugin_timeouts(self): if self.timed_out_plugins: return self.timed_out_plugins diff --git a/hotsos/plugin_extensions/storage/bcache_summary.py b/hotsos/plugin_extensions/storage/bcache_summary.py index 7e2a41501..def764046 100644 --- a/hotsos/plugin_extensions/storage/bcache_summary.py +++ b/hotsos/plugin_extensions/storage/bcache_summary.py @@ -1,12 +1,21 @@ from hotsos.core.plugins.storage.bcache import BcacheChecks -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class BcacheSummary(BcacheChecks): """ Implementation of Bcache summary. """ summary_part_index = 2 - @summary_entry('cachesets', 0) + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. + + @summary_entry('cachesets', get_min_available_entry_index()) def summary_cachesets(self): _state = {} for cset in self.cachesets: diff --git a/hotsos/plugin_extensions/storage/ceph_summary.py b/hotsos/plugin_extensions/storage/ceph_summary.py index 377df8970..9a57bafad 100644 --- a/hotsos/plugin_extensions/storage/ceph_summary.py +++ b/hotsos/plugin_extensions/storage/ceph_summary.py @@ -1,49 +1,26 @@ from hotsos.core.plugins.storage.ceph.common import CephChecks from hotsos.core.utils import sorted_dict -from hotsos.core.plugintools import summary_entry +from hotsos.core.plugintools import ( + summary_entry, + get_min_available_entry_index, +) class CephSummary(CephChecks): """ Implementation of Ceph summary. """ summary_part_index = 0 - @summary_entry('release', 0) - def summary_release(self): - return {'name': self.release_name, - 'days-to-eol': self.days_to_eol} + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. - @summary_entry('services', 1) - def summary_services(self): - """Get string info for running services.""" - if self.systemd.services: - return self.systemd.summary - - if self.pebble.services: - return self.pebble.summary - - return None - - @summary_entry('dpkg', 2) - def summary_dpkg(self): - # require at least one core package to be installed to include - # this in the report. - if self.apt.core: - return self.apt.all_formatted - - return None - - @summary_entry('snaps', 2) - def summary_snaps(self): - if self.snaps.core: - return self.snaps.all_formatted - - return None - - @summary_entry('status', 3) + @summary_entry('status', get_min_available_entry_index()) def summary_status(self): return self.cluster.health_status or None - @summary_entry('network', 4) + @summary_entry('network', get_min_available_entry_index() + 1) def summary_network(self): """ Identify ports used by Ceph daemons, include them in output for informational purposes. @@ -57,30 +34,30 @@ def summary_network(self): return None - @summary_entry('osd-pgs-near-limit', 5) + @summary_entry('osd-pgs-near-limit', get_min_available_entry_index() + 2) def summary_osd_pgs_near_limit(self): if self.cluster.osds_pgs_above_max: return self.cluster.osds_pgs_above_max return None - @summary_entry('osd-pgs-suboptimal', 6) + @summary_entry('osd-pgs-suboptimal', get_min_available_entry_index() + 3) def summary_osd_pgs_suboptimal(self): if self.cluster.osds_pgs_suboptimal: return self.cluster.osds_pgs_suboptimal return None - @summary_entry('versions', 7) + @summary_entry('versions', get_min_available_entry_index() + 4) def summary_versions(self): versions = self.cluster.ceph_daemon_versions_unique() return versions or None - @summary_entry('mgr-modules', 8) + @summary_entry('mgr-modules', get_min_available_entry_index() + 5) def summary_mgr_modules(self): return self.cluster.mgr_modules or None - @summary_entry('local-osds', 9) + @summary_entry('local-osds', get_min_available_entry_index() + 6) def summary_local_osds(self): if self.local_osds: osds = {} @@ -91,10 +68,10 @@ def summary_local_osds(self): return None - @summary_entry('crush-rules', 10) + @summary_entry('crush-rules', get_min_available_entry_index() + 7) def summary_crush_rules(self): return self.cluster.crush_map.rules or None - @summary_entry('large-omap-pgs', 11) + @summary_entry('large-omap-pgs', get_min_available_entry_index() + 8) def summary_large_omap_pgs(self): return self.cluster.large_omap_pgs or None diff --git a/hotsos/plugin_extensions/vault/summary.py b/hotsos/plugin_extensions/vault/summary.py index 3158b0f80..b87b50388 100644 --- a/hotsos/plugin_extensions/vault/summary.py +++ b/hotsos/plugin_extensions/vault/summary.py @@ -1,23 +1,12 @@ from hotsos.core.plugins.vault import VaultChecks -from hotsos.core.plugintools import summary_entry class VaultSummary(VaultChecks): """ Implementation of Vault summary. """ summary_part_index = 0 - @summary_entry('services', 0) - def summary_services(self): - if self.systemd.services: - return self.systemd.summary - if self.pebble.services: - return self.pebble.summary - - return None - - @summary_entry('snaps', 1) - def summary_snaps(self): - if self.snaps.core: - return self.snaps.all_formatted - - return None + # REMINDER: common entries are implemented in the SummaryBase base class + # and only application plugin specific customisations are + # implemented here. We use the get_min_available_entry_index() to + # ensure that additional entries don't clobber existing ones but + # conversely can also replace them by re-using their indices. diff --git a/pylintrc b/pylintrc index 1f09e14ad..96345d84a 100644 --- a/pylintrc +++ b/pylintrc @@ -21,7 +21,6 @@ score=yes [MESSAGES CONTROL] disable= - duplicate-code, missing-function-docstring, missing-module-docstring, too-few-public-methods, diff --git a/tests/unit/storage/test_ceph_mon.py b/tests/unit/storage/test_ceph_mon.py index 5ba4d0683..a856f94dc 100644 --- a/tests/unit/storage/test_ceph_mon.py +++ b/tests/unit/storage/test_ceph_mon.py @@ -410,7 +410,8 @@ def test_ceph_daemon_log_checker(self, mock_cli): with GlobalSearcher() as global_searcher: inst = ceph_event_checks.CephEventHandler(global_searcher) actual = self.part_output_to_actual(inst.output) - self.assertEqual(actual, result) + for key, value in result.items(): + self.assertEqual(actual[key], value) @utils.load_templated_tests('scenarios/storage/ceph/ceph-mon') diff --git a/tests/unit/storage/test_ceph_osd.py b/tests/unit/storage/test_ceph_osd.py index 25d3218b8..b855e0abc 100644 --- a/tests/unit/storage/test_ceph_osd.py +++ b/tests/unit/storage/test_ceph_osd.py @@ -11,6 +11,9 @@ from .. import utils +# pylint: disable=duplicate-code + + CEPH_CONF_NO_BLUESTORE = """ [global] [osd] @@ -152,14 +155,12 @@ def test_network_info(self): @mock.patch.object(host_helpers.packaging, 'CLIHelper') def test_service_info_unavailable(self, mock_helper): - release_info = {'name': 'unknown', 'days-to-eol': None} - mock_helper.return_value = mock.MagicMock() mock_helper.return_value.ps.return_value = [] mock_helper.return_value.dpkg_l.return_value = [] inst = ceph_summary.CephSummary() actual = self.part_output_to_actual(inst.output) - self.assertEqual(actual['release'], release_info) + self.assertFalse('release' in actual) def test_package_info(self): inst = ceph_summary.CephSummary() @@ -215,7 +216,8 @@ def test_ceph_daemon_log_checker(self, mock_cli): with GlobalSearcher() as global_searcher: inst = ceph_event_checks.CephEventHandler(global_searcher) actual = self.part_output_to_actual(inst.output) - self.assertEqual(actual, result) + for key, value in result.items(): + self.assertEqual(actual[key], value) @utils.load_templated_tests('scenarios/storage/ceph/ceph-osd') diff --git a/tests/unit/test_juju.py b/tests/unit/test_juju.py index 108e78ebc..6d9470756 100644 --- a/tests/unit/test_juju.py +++ b/tests/unit/test_juju.py @@ -77,8 +77,8 @@ class TestJujuSummary(JujuTestsBase): def test_summary_keys(self): inst = summary.JujuSummary() self.assertEqual(list(inst.output.keys()), - ['services', - 'version', + ['version', + 'services', 'machine', 'units']) diff --git a/tests/unit/test_openstack.py b/tests/unit/test_openstack.py index 3d9e1632e..1490f21c8 100644 --- a/tests/unit/test_openstack.py +++ b/tests/unit/test_openstack.py @@ -17,6 +17,9 @@ from hotsos.core.issues import IssuesManager +# pylint: disable=duplicate-code + + from . import utils OCTAVIA_UNIT_FILES = """ @@ -600,39 +603,36 @@ def test_get_neutronl3ha_info(self): class TestOpenstackVmInfo(TestOpenstackBase): """ Unit tests for OpenStack vm info. """ def test_get_vm_checks(self): - expected = {"vm-info": { - "running": { - 'count': 1, - 'uuids': ['d1d75e2f-ada4-49bc-a963-528d89dfda25']}, - "cpu-models": {'Skylake-Client-IBRS': 1}, - "vcpu-info": { - "available-cores": 2, - "system-cores": 2, - "smt": False, - "used": 1, - "overcommit-factor": 0.5}} - } + expected = {"running": { + 'count': 1, + 'uuids': ['d1d75e2f-ada4-49bc-a963-528d89dfda25']}, + "cpu-models": {'Skylake-Client-IBRS': 1}, + "vcpu-info": { + "available-cores": 2, + "system-cores": 2, + "smt": False, + "used": 1, + "overcommit-factor": 0.5}} inst = vm_info.OpenstackInstanceChecks() actual = self.part_output_to_actual(inst.output) - self.assertEqual(actual, expected) + self.assertEqual(actual['vm-info'], expected) def test_vm_migration_analysis(self): - expected = {'nova-migrations': { - 'live-migration': { - '359150c9-6f40-416e-b381-185bff09e974': [ - {'start': '2022-02-10 16:18:28', - 'end': '2022-02-10 16:18:28', - 'duration': 0.0, - 'regressions': { - 'memory': 0, - 'disk': 0}, - 'iterations': 1}] - }}} + expected = {'live-migration': { + '359150c9-6f40-416e-b381-185bff09e974': [ + {'start': '2022-02-10 16:18:28', + 'end': '2022-02-10 16:18:28', + 'duration': 0.0, + 'regressions': { + 'memory': 0, + 'disk': 0}, + 'iterations': 1}] + }} with GlobalSearcher() as searcher: inst = vm_info.NovaServerMigrationAnalysis(searcher) actual = self.part_output_to_actual(inst.output) - self.assertEqual(actual, expected) + self.assertEqual(actual['nova-migrations'], expected) class TestOpenstackNovaExternalEvents(TestOpenstackBase): @@ -703,7 +703,9 @@ def test_get_network_checker(self): } inst = service_network_checks.OpenstackNetworkChecks() actual = self.part_output_to_actual(inst.output) - self.assertEqual(actual, expected) + for key, value in expected.items(): + self.assertEqual(actual[key], value) + self.assertEqual(IssuesManager().load_issues(), {}) @mock.patch.object(service_network_checks, 'VXLAN_HEADER_BYTES', 31) diff --git a/tests/unit/test_openvswitch.py b/tests/unit/test_openvswitch.py index d845a3590..15fc9fb4d 100644 --- a/tests/unit/test_openvswitch.py +++ b/tests/unit/test_openvswitch.py @@ -272,7 +272,9 @@ def test_ovs_vswitchd_checks(self): '2022-02-10': {'tap6a0486f9-82': 1}}}} with GlobalSearcher() as searcher: inst = event_checks.OVSEventChecks(searcher) - self.assertEqual(self.part_output_to_actual(inst.output), expected) + actual = self.part_output_to_actual(inst.output) + for key, value in expected.items(): + self.assertEqual(actual[key], value) @mock.patch('hotsos.core.ycheck.engine.YDefsLoader._is_def', new=utils.is_def_filter('ovs-vswitchd.yaml', @@ -308,7 +310,9 @@ def test_ovs_common_log_checks(self): '2022-02-10': 4}}}} with GlobalSearcher() as searcher: inst = event_checks.OVSEventChecks(searcher) - self.assertEqual(self.part_output_to_actual(inst.output), expected) + actual = self.part_output_to_actual(inst.output) + for key, value in expected.items(): + self.assertEqual(actual[key], value) @mock.patch('hotsos.core.ycheck.engine.YDefsLoader._is_def', new=utils.is_def_filter('errors-and-warnings.yaml', @@ -371,7 +375,9 @@ def test_ovn_central_checks(self): }} with GlobalSearcher() as searcher: inst = event_checks.OVNEventChecks(searcher) - self.assertEqual(self.part_output_to_actual(inst.output), expected) + actual = self.part_output_to_actual(inst.output) + for key, value in expected.items(): + self.assertEqual(actual[key], value) @mock.patch('hotsos.core.ycheck.engine.YDefsLoader._is_def', new=utils.is_def_filter('ovn-controller.yaml', @@ -390,7 +396,9 @@ def test_ovn_controller_checks(self): 1}}}} with GlobalSearcher() as searcher: inst = event_checks.OVNEventChecks(searcher) - self.assertEqual(self.part_output_to_actual(inst.output), expected) + actual = self.part_output_to_actual(inst.output) + for key, value in expected.items(): + self.assertEqual(actual[key], value) @mock.patch('hotsos.core.ycheck.engine.YDefsLoader._is_def', new=utils.is_def_filter('errors-and-warnings.yaml', @@ -411,7 +419,9 @@ def test_ovn_common_log_checks(self): 'WARN': {'2022-02-16': 23, '2022-02-17': 23}}}} with GlobalSearcher() as searcher: inst = event_checks.OVNEventChecks(searcher) - self.assertEqual(self.part_output_to_actual(inst.output), expected) + actual = self.part_output_to_actual(inst.output) + for key, value in expected.items(): + self.assertEqual(actual[key], value) @mock.patch('hotsos.core.ycheck.engine.YDefsLoader._is_def', new=utils.is_def_filter('bfd.yaml', diff --git a/tests/unit/test_system.py b/tests/unit/test_system.py index 31ab352ce..f145d30ca 100644 --- a/tests/unit/test_system.py +++ b/tests/unit/test_system.py @@ -13,6 +13,8 @@ from . import utils +# pylint: disable=duplicate-code + NUMACTL = """ available: 2 nodes (0-1) diff --git a/tests/unit/ycheck/test_properties.py b/tests/unit/ycheck/test_properties.py index cf475458b..3d243ed4b 100644 --- a/tests/unit/ycheck/test_properties.py +++ b/tests/unit/ycheck/test_properties.py @@ -24,7 +24,7 @@ from .. import utils # It is fine for a test to access a protected member so allow it for all tests -# pylint: disable=protected-access +# pylint: disable=protected-access,disable=duplicate-code class TestProperty(YPropertyBase): diff --git a/tests/unit/ycheck/test_scenarios.py b/tests/unit/ycheck/test_scenarios.py index 8f63c206b..501a4c9d3 100644 --- a/tests/unit/ycheck/test_scenarios.py +++ b/tests/unit/ycheck/test_scenarios.py @@ -16,6 +16,8 @@ from .. import utils from . import test_scenarios_data as test_data +# pylint: disable=duplicate-code + class TestProperty(): """ Test Property """