From de1416a95182d4bcc8ef8282f807f5b732d0de43 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Tue, 6 Aug 2024 11:03:34 +0100 Subject: [PATCH] Re-enable pylint too-many-instance-attributes (#965) --- hotsos/cli.py | 2 +- hotsos/core/host_helpers/cli.py | 99 ++++++++++--------- hotsos/core/plugins/openstack/common.py | 39 +++++--- hotsos/core/plugins/openstack/openstack.py | 47 ++++++--- .../openstack/agent/exceptions.py | 4 +- .../openstack/service_features.py | 2 +- .../openstack/service_network_checks.py | 40 ++++---- hotsos/plugin_extensions/openstack/vm_info.py | 4 +- pylintrc | 1 - 9 files changed, 136 insertions(+), 102 deletions(-) diff --git a/hotsos/cli.py b/hotsos/cli.py index 40ae17ae4..83c034f78 100755 --- a/hotsos/cli.py +++ b/hotsos/cli.py @@ -190,7 +190,7 @@ def progress_spinner(show_spinner, path): @dataclass(frozen=True) -class CLIArgs: +class CLIArgs: # pylint: disable=too-many-instance-attributes """Command line arguments.""" data_root: click.Path diff --git a/hotsos/core/host_helpers/cli.py b/hotsos/core/host_helpers/cli.py index 29476faed..b1cfc1c99 100644 --- a/hotsos/core/host_helpers/cli.py +++ b/hotsos/core/host_helpers/cli.py @@ -6,6 +6,7 @@ import re import subprocess import tempfile +from dataclasses import dataclass, field, fields from functools import cached_property from hotsos.core.config import HotSOSConfig @@ -28,20 +29,25 @@ from hotsos.core.log import log -class CmdBase(): - """ Base class for all command source types. """ - def __init__(self): +@dataclass +class CmdBase: + """ Base class for all command source types. + + Provides a way to save original state and restore to that state. + """ + def __post_init__(self): self.hooks = {} - self.reset() + # make a copy of field original values. + for f in fields(self): + setattr(self, 'original_' + f.name, f.type(getattr(self, f.name))) + + def get_original_attr_value(self, name): + return getattr(self, 'original_' + name) def reset(self): - """ - Used to reset an object after it has been called. In other words, each - time a command object is called it may alter its initial state e.g. via - hooks but this state should not persist to the next call so this is - used to restore state. - """ - raise NotImplementedError + """ Reset fields to original values. """ + for f in fields(self): + setattr(self, f.name, f.type(getattr(self, 'original_' + f.name))) @classmethod def safe_readlines(cls, path): @@ -61,25 +67,34 @@ def register_hook(self, name, f): self.hooks[name] = f -class BinCmd(CmdBase): - """ Implements binary command execution. """ - TYPE = "BIN" +@dataclass +class FileCmdBase(CmdBase): + """ + State used to execute a file-based command i.e. a command whose output is + already saved in a file. + """ + path: str + json_decode: bool = False + singleline: bool = False + decode_error_handling: str = None - def __init__(self, cmd, json_decode=False, singleline=False): - """ - @param cmd: command in string format (not list) - """ - self.cmd = self.original_cmd = cmd - self.original_cmd_extras = [] - self.original_json_decode = json_decode - self.original_singleline = singleline - super().__init__() + def __post_init__(self): + self.path = os.path.join(HotSOSConfig.data_root, self.path) + super().__post_init__() - def reset(self): - self.cmd = self.original_cmd - self.original_cmd_extras = [] - self.json_decode = self.original_json_decode - self.singleline = self.original_singleline + +@dataclass +class BinCmdBase(CmdBase): + """ State used to execute a binary command. """ + cmd: str + json_decode: bool = False + singleline: bool = False + cmd_extras: list = field(default_factory=lambda: []) + + +class BinCmd(BinCmdBase): + """ Implements binary command execution. """ + TYPE = "BIN" @catch_exceptions(*CLI_COMMON_EXCEPTIONS) @reset_command @@ -93,7 +108,7 @@ def __call__(self, *args, skip_json_decode=False, **kwargs): if kwargs: cmd = cmd.format(**kwargs) - _cmd = cmd.split() + self.original_cmd_extras + _cmd = cmd.split() + self.cmd_extras out = subprocess.run(_cmd, timeout=HotSOSConfig.command_timeout, capture_output=True, check=False) output = out.stdout @@ -119,7 +134,7 @@ def __call__(self, *args, skip_json_decode=False, **kwargs): return CmdOutput(output.splitlines(keepends=True)) -class FileCmd(CmdBase): +class FileCmd(FileCmdBase): """ Implements file-based command execution. This is used e.g. with sosreports where the output of a command is saved @@ -127,21 +142,6 @@ class FileCmd(CmdBase): """ TYPE = "FILE" - def __init__(self, path, json_decode=False, - singleline=False, decode_error_handling=None): - self.path = self.original_path = os.path.join(HotSOSConfig.data_root, - path) - self.original_json_decode = json_decode - self.original_singleline = singleline - self.original_decode_error_handling = decode_error_handling - super().__init__() - - def reset(self): - self.path = self.original_path - self.json_decode = self.original_json_decode - self.singleline = self.original_singleline - self.decode_error_handling = self.original_decode_error_handling - @catch_exceptions(*CLI_COMMON_EXCEPTIONS) @reset_command @run_post_exec_hooks @@ -205,8 +205,11 @@ class BinFileCmd(FileCmd): @run_post_exec_hooks @run_pre_exec_hooks def __call__(self, *args, **kwargs): - if not os.path.exists(self.original_path): - raise SourceNotFound(self.original_path) + # NOTE: we check the original path since by this point the 'path' + # attribute will have been modified to include a binary and other args + # required to execute it. + if not os.path.exists(self.get_original_attr_value('path')): + raise SourceNotFound(self.get_original_attr_value('path')) if args: self.path = self.path.format(*args) @@ -412,7 +415,7 @@ def format_date_cmd(self, **kwargs): self.cmd = f'{self.cmd} --utc' if fmt: # this can't get split() so add to the end of the command list - self.original_cmd_extras = [fmt] + self.cmd_extras = [fmt] class DateFileCmd(FileCmd): diff --git a/hotsos/core/plugins/openstack/common.py b/hotsos/core/plugins/openstack/common.py index e96ec2992..f89080842 100644 --- a/hotsos/core/plugins/openstack/common.py +++ b/hotsos/core/plugins/openstack/common.py @@ -2,6 +2,7 @@ from datetime import datetime import os import re +from dataclasses import dataclass from functools import cached_property from hotsos.core.config import HotSOSConfig @@ -28,6 +29,14 @@ from hotsos.core.ycheck.events import EventHandlerBase, EventCallbackBase +@dataclass +class OSTProjectHelpers: + """ OpenStack project helper objects. """ + nova: NovaBase + neutron: NeutronBase + octavia: OctaviaBase + + class OpenstackBase(): """ Base class for Openstack checks. @@ -36,20 +45,20 @@ class OpenstackBase(): """ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.nova = NovaBase() - self.neutron = NeutronBase() - self.octavia = OctaviaBase() self.certificate_expire_days = 60 - self.ost_projects = OSTProjectCatalog() - service_exprs = self.ost_projects.service_exprs + self.project_helpers = OSTProjectHelpers(NovaBase(), NeutronBase(), + OctaviaBase()) + self.project_catalog = OSTProjectCatalog() + + service_exprs = self.project_catalog.service_exprs self.pebble = PebbleHelper(service_exprs=service_exprs) self.systemd = SystemdHelper(service_exprs=service_exprs) - self.apt = APTPackageHelper( - core_pkgs=self.ost_projects.packages_core_exprs, - other_pkgs=self.ost_projects.packages_dep_exprs) - self.docker = DockerImageHelper( - core_pkgs=self.ost_projects.packages_core_exprs, - other_pkgs=self.ost_projects.packages_dep_exprs) + + core_pkgs = self.project_catalog.packages_core_exprs + other_pkgs = self.project_catalog.packages_dep_exprs + self.apt = APTPackageHelper(core_pkgs=core_pkgs, other_pkgs=other_pkgs) + self.docker = DockerImageHelper(core_pkgs=core_pkgs, + other_pkgs=other_pkgs) @cached_property def apt_source_path(self): @@ -148,15 +157,15 @@ def bind_interfaces(self): """ interfaces = {} - ifaces = self.nova.bind_interfaces + ifaces = self.project_helpers.nova.bind_interfaces if ifaces: interfaces.update(ifaces) - ifaces = self.neutron.bind_interfaces + ifaces = self.project_helpers.neutron.bind_interfaces if ifaces: interfaces.update(ifaces) - ifaces = self.octavia.bind_interfaces + ifaces = self.project_helpers.octavia.bind_interfaces if ifaces: interfaces.update(ifaces) @@ -172,7 +181,7 @@ def unexpected_masked_services(self): if not masked: return [] - expected_masked = self.ost_projects.default_masked_services + expected_masked = self.project_catalog.default_masked_services return sorted(list(masked.difference(expected_masked))) @cached_property diff --git a/hotsos/core/plugins/openstack/openstack.py b/hotsos/core/plugins/openstack/openstack.py index 2fde27f65..4ddaff89e 100644 --- a/hotsos/core/plugins/openstack/openstack.py +++ b/hotsos/core/plugins/openstack/openstack.py @@ -335,6 +335,21 @@ class OSTProjectParameters: log_path_overrides: dict = field(default_factory=lambda: {}) +@dataclass +class OSTProjectAptHelperParams: + """ APTPackageHelper package matching expression lists. """ + deps: list + core: list + + +@dataclass +class OSTProjectSystemdHelperParams: + """SystemdHelper service matching expression lists. """ + extra_services: list + masked_services: list + deprecated_services: list + + class OSTProject(): """ Representation of an Openstack project/service. @@ -368,12 +383,14 @@ def __init__(self, params: OSTProjectParameters): don't match the name of the project. """ self.name = params.name - self.packages_deps = [self.PY_CLIENT_PREFIX.format(params.name)] - self.packages_core = [params.name] + + self.apt_params = OSTProjectAptHelperParams([self.PY_CLIENT_PREFIX. + format(params.name)], + [params.name]) if params.apt_core_alt: - self.packages_core.extend(params.apt_core_alt) + self.apt_params.core.extend(params.apt_core_alt) for c in params.apt_core_alt: - self.packages_deps.append(self.PY_CLIENT_PREFIX.format(c)) + self.apt_params.deps.append(self.PY_CLIENT_PREFIX.format(c)) self.config = {} if params.config: @@ -386,9 +403,11 @@ def __init__(self, params: OSTProjectParameters): ) self.config[label] = OpenstackConfig(path) - self.systemd_extra_services = params.systemd_extra_services - self.systemd_masked_services = params.systemd_masked_services - self.systemd_deprecated_services = params.systemd_deprecated_services + self.systemd_params = OSTProjectSystemdHelperParams( + params.systemd_extra_services, + params.systemd_masked_services, + params.systemd_deprecated_services) + self.logs_path = os.path.join("var/log", params.name) self.log_path_overrides = params.log_path_overrides self.exceptions = EXCEPTIONS_COMMON + OST_EXCEPTIONS.get( @@ -400,13 +419,13 @@ def __init__(self, params: OSTProjectParameters): def installed(self): """ Return True if the openstack service is installed. """ return bool(host_helpers.APTPackageHelper( - core_pkgs=self.packages_core).core) + core_pkgs=self.apt_params.core).core) @cached_property def services_expr(self): exprs = [f'{self.name}{self.SVC_VALID_SUFFIX}'] - if self.systemd_extra_services: - exprs += self.systemd_extra_services + if self.systemd_params.extra_services: + exprs += self.systemd_params.extra_services return exprs @cached_property @@ -429,7 +448,7 @@ def log_paths(self, include_deprecated_services=True): yield proj_manage, [path] for svc in self.services: if (not include_deprecated_services and - svc in self.systemd_deprecated_services): + svc in self.systemd_params.deprecated_services): continue path = os.path.join('var/log', self.name, @@ -578,7 +597,7 @@ def default_masked_services(self): """ masked = [] for p in self.all.values(): - masked += p.systemd_masked_services + masked += p.systemd_params.masked_services return sorted(masked) @@ -590,7 +609,7 @@ def add(self, name, *args, **kwargs): def packages_core_exprs(self): core = set() for p in self.all.values(): - core.update(p.packages_core) + core.update(p.apt_params.core) return list(core) @@ -598,7 +617,7 @@ def packages_core_exprs(self): def packages_dep_exprs(self): deps = set(self.APT_DEPS_COMMON) for p in self.all.values(): - deps.update(p.packages_deps) + deps.update(p.apt_params.deps) return list(deps) diff --git a/hotsos/plugin_extensions/openstack/agent/exceptions.py b/hotsos/plugin_extensions/openstack/agent/exceptions.py index f6034b431..ee378f420 100644 --- a/hotsos/plugin_extensions/openstack/agent/exceptions.py +++ b/hotsos/plugin_extensions/openstack/agent/exceptions.py @@ -190,7 +190,7 @@ def _load(self): exceptions. """ log.debug("loading exception search defs") - for project in self.ost_projects.all.values(): + for project in self.project_catalog.all.values(): if not project.installed: log.debug("%s is not installed - excluding from exception " "checks", project.name) @@ -246,7 +246,7 @@ def _run(self, search_results): """ log.debug("processing exception search results") agent_exceptions = {} - for name, project in self.ost_projects.all.items(): + for name, project in self.project_catalog.all.items(): if not project.installed: continue diff --git a/hotsos/plugin_extensions/openstack/service_features.py b/hotsos/plugin_extensions/openstack/service_features.py index c25e3d387..82e289e0c 100644 --- a/hotsos/plugin_extensions/openstack/service_features.py +++ b/hotsos/plugin_extensions/openstack/service_features.py @@ -82,7 +82,7 @@ def summary_features(self): """ features = {} for service, modules in FEATURES.items(): - svc_cfg = self.ost_projects.all[service].config + svc_cfg = self.project_catalog.all[service].config if not svc_cfg['main'].exists: continue diff --git a/hotsos/plugin_extensions/openstack/service_network_checks.py b/hotsos/plugin_extensions/openstack/service_network_checks.py index 7cc7323a3..4c1a10e84 100644 --- a/hotsos/plugin_extensions/openstack/service_network_checks.py +++ b/hotsos/plugin_extensions/openstack/service_network_checks.py @@ -1,4 +1,5 @@ import re +from dataclasses import fields from hotsos.core.host_helpers import CLIHelper, HostNetworkingHelper from hotsos.core.plugins.openstack.common import OpenstackBase, OpenStackChecks @@ -56,9 +57,9 @@ def _get_port_stat_outliers(counters): def get_config_info(self): config_info = {} - for project in ['nova', 'neutron', 'octavia']: - _project = getattr(self, project) - if _project and _project.bind_interfaces: + for project in [f.name for f in fields(self.project_helpers)]: + _project = getattr(self.project_helpers, project) + if _project.bind_interfaces: for name, port in _project.bind_interfaces.items(): if project not in config_info: config_info[project] = {} @@ -73,16 +74,18 @@ def get_phy_port_health_info(self): etc) for any outliers detected. """ port_health_info = {} - for project in ['nova', 'neutron', 'octavia']: - _project = getattr(self, project) - if _project and _project.bind_interfaces: - for port in _project.bind_interfaces.values(): - if port.stats: - stats = self._get_port_stat_outliers(port.stats) - if not stats: - continue + for project in [f.name for f in fields(self.project_helpers)]: + _project = getattr(self.project_helpers, project) + if not (_project and _project.bind_interfaces): + continue + + for port in _project.bind_interfaces.values(): + if port.stats: + stats = self._get_port_stat_outliers(port.stats) + if not stats: + continue - port_health_info[port.name] = stats + port_health_info[port.name] = stats return port_health_info @@ -149,12 +152,12 @@ def _get_router_iface_mtus(self): @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') - if not (project and project.bind_interfaces): + bind_interfaces = self.project_helpers.neutron.bind_interfaces + if not bind_interfaces: return None phy_mtus = set() - for port in project.bind_interfaces.values(): + for port in bind_interfaces.values(): phy_mtus.add(port.mtu) tunnels = OpenvSwitchBase().tunnels @@ -188,11 +191,11 @@ def summary_router_port_mtus(self): def summary_vm_port_health(self): """ For each instance get its ports and check port health, reporting on any outliers. """ - if not self.nova.instances: + if not self.project_helpers.nova.instances: return None port_health_info = {} - for guest in self.nova.instances.values(): + for guest in self.project_helpers.nova.instances.values(): for port in guest.ports: stats = port.stats if stats: @@ -206,7 +209,8 @@ def summary_vm_port_health(self): port_health_info[guest.uuid][port.hwaddr] = outliers if port_health_info: - health = {'num-vms-checked': len(self.nova.instances), + health = {'num-vms-checked': + len(self.project_helpers.nova.instances), 'stats': port_health_info} return health diff --git a/hotsos/plugin_extensions/openstack/vm_info.py b/hotsos/plugin_extensions/openstack/vm_info.py index a1f126d4d..cda272403 100644 --- a/hotsos/plugin_extensions/openstack/vm_info.py +++ b/hotsos/plugin_extensions/openstack/vm_info.py @@ -23,7 +23,7 @@ class OpenstackInstanceChecks(OpenstackBase, OpenStackChecks): def summary_vm_info(self): _info = {} - instances = self.nova.instances.values() + instances = self.project_helpers.nova.instances.values() if instances: _info['running'] = {'count': len(instances), 'uuids': [i.uuid for i in instances]} @@ -135,7 +135,7 @@ def __call__(self, event): _end = datetime.strptime(end, "%Y-%m-%d %H:%M:%S") duration = round(float((_end - _start).total_seconds()), 2) info = {'start': start, 'end': end, 'duration': duration} - instance = self.nova.instances.get(vm_uuid) + instance = self.project_helpers.nova.instances.get(vm_uuid) if instance and instance.memory_mbytes is not None: info['resources'] = {'memory_mbytes': instance.memory_mbytes} diff --git a/pylintrc b/pylintrc index bb9f39e00..738fafc1b 100644 --- a/pylintrc +++ b/pylintrc @@ -23,7 +23,6 @@ score=yes disable= missing-function-docstring, missing-module-docstring, - too-many-instance-attributes, too-many-locals, [DESIGN]