From 5ed8ce482842e601e18c9280b169ee622db80d25 Mon Sep 17 00:00:00 2001 From: Chris Harris Date: Tue, 23 Jun 2026 09:31:41 +0100 Subject: [PATCH 1/3] Post-Processing: Add plotting for collectl resources When generating a report CBT can pull CPU and memory usage from the fio output files. This is not very useful, however as it only reports the resource usage for the fio process itself. CBT also supports running other resource monitoring tools such as top and collectl. This PR deals with specifically plotting CPU and memory usage for collectl. The basic infrastructure already existed, so all that was needed was the pieces to parse the collectl output files and store the resource usage data in the intermediate format. The code changes were: - a factory method to instantiate the correct resource_result class to process resouce usage data - the collectl resource_result class - updates to the run_result to use the factory method - plot multiple CPU lines - unit tests fpr the new code Signed-off-by: Chris Harris Assisted-by: IBM Bob 1.03 and 2.0 --- .gitignore | 2 + benchmark/librbdfio.py | 295 +++++++-------- docs/CollectlIntegration_Phase1_Complete.md | 0 .../formatter/common_output_formatter.py | 41 +- .../plotter/common_format_plotter.py | 109 ++++-- post_processing/plotter/cpu_plotter.py | 89 ++++- post_processing/plotter/memory_plotter.py | 96 ++++- .../run_results/resource_result_factory.py | 60 +++ .../resources/collectl_resource.py | 209 +++++++++++ post_processing/run_results/run_result.py | 46 ++- tests/test_collectl_resource.py | 325 ++++++++++++++++ tests/test_common_format_plotter.py | 115 +++++- tests/test_common_output_formatter.py | 3 +- tests/test_cpu_plotter.py | 63 +++- tests/test_memory_plotter.py | 83 +++-- tests/test_resource_result_factory.py | 350 ++++++++++++++++++ 16 files changed, 1634 insertions(+), 252 deletions(-) create mode 100644 docs/CollectlIntegration_Phase1_Complete.md create mode 100644 post_processing/run_results/resource_result_factory.py create mode 100644 post_processing/run_results/resources/collectl_resource.py create mode 100644 tests/test_collectl_resource.py create mode 100644 tests/test_resource_result_factory.py diff --git a/.gitignore b/.gitignore index 5f8e2366..940ab171 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,5 @@ *.toml .coverage .bob +.vscode + diff --git a/benchmark/librbdfio.py b/benchmark/librbdfio.py index f7f1d49f..6960d817 100644 --- a/benchmark/librbdfio.py +++ b/benchmark/librbdfio.py @@ -1,22 +1,25 @@ """ - lirbdfio.py -- module to support the FIO benchmark exercising RBD. +lirbdfio.py -- module to support the FIO benchmark exercising RBD. """ -import os -import time + import logging -import common -import settings -import monitoring +import os import re +import time from pathlib import Path from typing import Union +import common +import monitoring +import settings +from post_processing.post_processing_types import ReportType from post_processing.report import Report, ReportOptions from .benchmark import Benchmark logger = logging.getLogger("cbt") + class LibrbdFio(Benchmark): """ Class LibrbdFio @@ -26,32 +29,32 @@ def __init__(self, archive_dir, cluster, config): super(LibrbdFio, self).__init__(archive_dir, cluster, config) # FIXME there are too many permutations, need to put results in SQLITE3 - self.cmd_path = config.get('cmd_path', '/usr/bin/fio') - self.pool_profile = config.get('pool_profile', 'default') - self.recov_pool_profile = config.get('recov_pool_profile', 'default') - self.recov_test_type = config.get('recov_test_type', 'blocking') - self.data_pool_profile = config.get('data_pool_profile', None) - self.time = config.get('time', None) + self.cmd_path = config.get("cmd_path", "/usr/bin/fio") + self.pool_profile = config.get("pool_profile", "default") + self.recov_pool_profile = config.get("recov_pool_profile", "default") + self.recov_test_type = config.get("recov_test_type", "blocking") + self.data_pool_profile = config.get("data_pool_profile", None) + self.time = config.get("time", None) # Global FIO options can be overwritten for specific workload options # would be nice to have them as a separate class -- future PR - self.time_based = bool(config.get('time_based', False)) - self.ramp = config.get('ramp', None) - self.numjobs = config.get('numjobs', 1) - self.end_fsync = config.get('end_fsync', 0) - self.mode = config.get('mode', 'write') - self.rwmixread = config.get('rwmixread', 50) + self.time_based = bool(config.get("time_based", False)) + self.ramp = config.get("ramp", None) + self.numjobs = config.get("numjobs", 1) + self.end_fsync = config.get("end_fsync", 0) + self.mode = config.get("mode", "write") + self.rwmixread = config.get("rwmixread", 50) self.rwmixwrite = 100 - self.rwmixread - self.log_avg_msec = config.get('log_avg_msec', None) - self.op_size = config.get('op_size', 4194304) - - self.pgs = config.get('pgs', 2048) - self.vol_size = config.get('vol_size', 65536) - self.vol_object_size = config.get('vol_object_size', 22) - self.volumes_per_client : int = int(config.get('volumes_per_client', 1)) - self.procs_per_volume = config.get('procs_per_volume', 1) - self.random_distribution = config.get('random_distribution', None) - self.rate_iops = config.get('rate_iops', None) - self.fio_out_format = config.get('fio_out_format', 'json,normal') + self.log_avg_msec = config.get("log_avg_msec", None) + self.op_size = config.get("op_size", 4194304) + + self.pgs = config.get("pgs", 2048) + self.vol_size = config.get("vol_size", 65536) + self.vol_object_size = config.get("vol_object_size", 22) + self.volumes_per_client: int = int(config.get("volumes_per_client", 1)) + self.procs_per_volume = config.get("procs_per_volume", 1) + self.random_distribution = config.get("random_distribution", None) + self.rate_iops = config.get("rate_iops", None) + self.fio_out_format = config.get("fio_out_format", "json,normal") self.data_pool = None iodepth_key: str = self._get_iodepth_key(config.keys()) # type: ignore[arg-type] @@ -61,62 +64,62 @@ def __init__(self, archive_dir, cluster, config): ) # use_existing_volumes needs to be true to set the pool and rbd names - self.use_existing_volumes = bool(config.get('use_existing_volumes', False)) - self.no_sudo = bool(config.get('no_sudo', False)) - self.idle_monitor_sleep = config.get('idle_monitor_sleep', 60) + self.use_existing_volumes = bool(config.get("use_existing_volumes", False)) + self.no_sudo = bool(config.get("no_sudo", False)) + self.idle_monitor_sleep = config.get("idle_monitor_sleep", 60) self.pool_name = config.get("poolname", "cbt-librbdfio") self.recov_pool_name = config.get("recov_pool_name", "cbt-rbdfio-recov") - self.rbdname = config.get('rbdname', '') - self.prefill_vols = config.get('prefill', {'blocksize': '4M', - 'numjobs': '1'}) - self.total_procs = (self.procs_per_volume * self.volumes_per_client * - len(settings.getnodes('clients').split(','))) + self.rbdname = config.get("rbdname", "") + self.prefill_vols = config.get("prefill", {"blocksize": "4M", "numjobs": "1"}) + self.total_procs = ( + self.procs_per_volume * self.volumes_per_client * len(settings.getnodes("clients").split(",")) + ) if not self._workloads.exist(): - self.run_dir += ( f'op_size-{int(self.op_size):08d}/' - f'concurrent_procs-{int(self.total_procs):03d}/' - f'iodepth-{int(self.iodepth):03d}/{self.mode}' ) + self.run_dir += ( + f"op_size-{int(self.op_size):08d}/" + f"concurrent_procs-{int(self.total_procs):03d}/" + f"iodepth-{int(self.iodepth):03d}/{self.mode}" + ) self.out_dir = self.archive_dir self.norandommap = config.get("norandommap", False) self.wait_pgautoscaler_timeout = config.get("wait_pgautoscaler_timeout", -1) # Make the file names string (repeated across volumes) - self.names = '' + self.names = "" for proc_num in range(self.procs_per_volume): - rbd_name = f'cbt-rbdfio-`{common.get_fqdn_cmd()}`-file-{proc_num:d}' - self.names += f'--name={rbd_name} ' + rbd_name = f"cbt-rbdfio-`{common.get_fqdn_cmd()}`-file-{proc_num:d}" + self.names += f"--name={rbd_name} " def exists(self): """ Verify whether the out_dir exists """ if os.path.exists(self.out_dir): - logger.info('Skipping existing test in %s.', self.out_dir) + logger.info("Skipping existing test in %s.", self.out_dir) return True return False - def initialize(self): super(LibrbdFio, self).initialize() # Clean and Create the run directory common.clean_remote_dir(self.run_dir) common.make_remote_dir(self.run_dir) - logger.info('Pausing for %ds for idle monitoring.', self.idle_monitor_sleep) - monitoring.start( f"{self.run_dir}idle_monitoring" ) + logger.info("Pausing for %ds for idle monitoring.", self.idle_monitor_sleep) + monitoring.start(f"{self.run_dir}idle_monitoring") time.sleep(self.idle_monitor_sleep) monitoring.stop() - common.sync_files( f'{self.run_dir}/', self.out_dir) + common.sync_files(f"{self.run_dir}/", self.out_dir) # Create the recovery image based on test type requested - if 'recovery_test' in self.cluster.config and self.recov_test_type == 'background': + if "recovery_test" in self.cluster.config and self.recov_test_type == "background": self.mkrecovimage() if self._workloads.exist(): logger.info("Workloads:\n %s", self._workloads.get_names().replace(" ", "\n")) - logger.info('Creating fio images...') + logger.info("Creating fio images...") self.mkimages() - logger.info('Attempting to prefill fio images...') + logger.info("Attempting to prefill fio images...") self.prefill() - def run(self): super(LibrbdFio, self).run() # We'll always drop caches for rados bench @@ -128,20 +131,18 @@ def run(self): time.sleep(5) # If the pg autoscaler kicks in before starting the test, # wait for it to complete. Otherwise, results may be skewed. - ret = self.cluster.check_pg_autoscaler(self.wait_pgautoscaler_timeout, - f"{self.run_dir}pgautoscaler.log") + ret = self.cluster.check_pg_autoscaler(self.wait_pgautoscaler_timeout, f"{self.run_dir}pgautoscaler.log") if ret == 1: - logger.warn("PG autoscaler taking longer to complete." - "Continuing anyway...results may be skewed.") + logger.warn("PG autoscaler taking longer to complete.Continuing anyway...results may be skewed.") # Start the recovery thread if requested - if 'recovery_test' in self.cluster.config: - if self.recov_test_type == 'blocking': + if "recovery_test" in self.cluster.config: + if self.recov_test_type == "blocking": recovery_callback = self.recovery_callback_blocking - elif self.recov_test_type == 'background': + elif self.recov_test_type == "background": recovery_callback = self.recovery_callback_background self.cluster.create_recovery_test(self.run_dir, recovery_callback, self.recov_test_type) - if 'recovery_test' in self.cluster.config and self.recov_test_type == 'background': + if "recovery_test" in self.cluster.config and self.recov_test_type == "background": # Wait for a signal from the recovery thread to initiate client IO self.cluster.wait_start_io() @@ -152,25 +153,25 @@ def run(self): else: # Original style monitoring.start(self.run_dir) - logger.info('Running rbd fio %s test.', self.mode) + logger.info("Running rbd fio %s test.", self.mode) ps = [] number_of_volumes: int = len(self._iodepth_per_volume.keys()) for i in range(number_of_volumes): fio_cmd = self.mkfiocmd(i) - p = common.pdsh(settings.getnodes('clients'), fio_cmd) + p = common.pdsh(settings.getnodes("clients"), fio_cmd) ps.append(p) for p in ps: p.wait() - + # If we were doing recovery, wait until it's done. - if 'recovery_test' in self.cluster.config: + if "recovery_test" in self.cluster.config: self.cluster.wait_recovery_done() monitoring.stop(self.run_dir) # Finally, get the historic ops self.cluster.dump_historic_ops(self.run_dir) - source_directory: str = f'{self.run_dir}/*' + source_directory: str = f"{self.run_dir}/*" if self._workloads.exist(): source_directory = f"{self._workloads.get_base_run_directory()}/*" common.sync_files(source_directory, self.out_dir) @@ -178,21 +179,20 @@ def run(self): if self._create_report: report_config: dict[str, Union[str, bool]] = settings.report - output_directory: str = report_config.get('output_directory', f"{self.out_dir}/report") + output_directory: str = report_config.get("output_directory", f"{self.out_dir}/report") report_options: ReportOptions = ReportOptions( - archives = [f"{self.archive_dir}"], - output_directory = output_directory, - results_file_root = "json_output", - create_pdf = report_config.get("create_pdf", False), - force_refresh = report_config.get("force_refresh", False), - no_error_bars = report_config.get("no_error_bars", False), - comparison = False, - plot_resources = report_config.get("plot_resource", False) + archives=[f"{self.archive_dir}"], + output_directory=output_directory, + results_file_root="json_output", + create_pdf=report_config.get("create_pdf", False), + force_refresh=report_config.get("force_refresh", False), + no_error_bars=report_config.get("no_error_bars", False), + report_type=ReportType.SIMPLE, + plot_resources=report_config.get("plot_resource", False), ) report: Report = Report(report_options) report.generate() - def mkfiocmd(self, volnum: int) -> str: """ Construct a FIO cmd (note the shell interpolation for the host @@ -201,96 +201,101 @@ def mkfiocmd(self, volnum: int) -> str: if self.use_existing_volumes and len(self.rbdname): rbdname = self.rbdname else: - rbdname = f'cbt-rbdfio-`{common.get_fqdn_cmd()}`-{volnum:d}' + rbdname = f"cbt-rbdfio-`{common.get_fqdn_cmd()}`-{volnum:d}" - logger.debug('Using rbdname %s', rbdname) - out_file = f'{self.run_dir}/output.{volnum:d}' + logger.debug("Using rbdname %s", rbdname) + out_file = f"{self.run_dir}/output.{volnum:d}" - fio_cmd: str = '' + fio_cmd: str = "" if not self.no_sudo: - fio_cmd = 'sudo ' - fio_cmd += '%s --ioengine=rbd --clientname=admin --pool=%s --rbdname=%s --invalidate=0' % (self.cmd_path, self.pool_name, rbdname) - fio_cmd += ' --rw=%s' % self.mode - fio_cmd += ' --output-format=%s' % self.fio_out_format - if (self.mode == 'readwrite' or self.mode == 'randrw'): - fio_cmd += ' --rwmixread=%s --rwmixwrite=%s' % (self.rwmixread, self.rwmixwrite) + fio_cmd = "sudo " + fio_cmd += "%s --ioengine=rbd --clientname=admin --pool=%s --rbdname=%s --invalidate=0" % ( + self.cmd_path, + self.pool_name, + rbdname, + ) + fio_cmd += " --rw=%s" % self.mode + fio_cmd += " --output-format=%s" % self.fio_out_format + if self.mode == "readwrite" or self.mode == "randrw": + fio_cmd += " --rwmixread=%s --rwmixwrite=%s" % (self.rwmixread, self.rwmixwrite) if self.time is not None: - fio_cmd += ' --runtime=%d' % self.time + fio_cmd += " --runtime=%d" % self.time if self.time_based is True: - fio_cmd += ' --time_based' + fio_cmd += " --time_based" if self.ramp is not None: - fio_cmd += ' --ramp_time=%d' % self.ramp - fio_cmd += ' --numjobs=%s' % self.numjobs - fio_cmd += ' --direct=1' - fio_cmd += ' --bs=%dB' % self.op_size + fio_cmd += " --ramp_time=%d" % self.ramp + fio_cmd += " --numjobs=%s" % self.numjobs + fio_cmd += " --direct=1" + fio_cmd += " --bs=%dB" % self.op_size iodepth: str = f"{self._iodepth_per_volume[volnum]}" - - fio_cmd += ' --iodepth=%s' % iodepth - fio_cmd += ' --end_fsync=%d' % self.end_fsync -# if self.vol_size: -# fio_cmd += ' -- size=%dM' % self.vol_size + + fio_cmd += " --iodepth=%s" % iodepth + fio_cmd += " --end_fsync=%d" % self.end_fsync + # if self.vol_size: + # fio_cmd += ' -- size=%dM' % self.vol_size if self.norandommap: - fio_cmd += ' --norandommap' + fio_cmd += " --norandommap" if self.log_iops: - fio_cmd += ' --write_iops_log=%s' % out_file + fio_cmd += " --write_iops_log=%s" % out_file if self.log_bw: - fio_cmd += ' --write_bw_log=%s' % out_file + fio_cmd += " --write_bw_log=%s" % out_file if self.log_lat: - fio_cmd += ' --write_lat_log=%s' % out_file - if 'recovery_test' in self.cluster.config: - fio_cmd += ' --time_based' + fio_cmd += " --write_lat_log=%s" % out_file + if "recovery_test" in self.cluster.config: + fio_cmd += " --time_based" if self.random_distribution is not None: - fio_cmd += ' --random_distribution=%s' % self.random_distribution + fio_cmd += " --random_distribution=%s" % self.random_distribution if self.log_avg_msec is not None: - fio_cmd += ' --log_avg_msec=%s' % self.log_avg_msec + fio_cmd += " --log_avg_msec=%s" % self.log_avg_msec if self.rate_iops is not None: - fio_cmd += ' --rate_iops=%s' % self.rate_iops + fio_cmd += " --rate_iops=%s" % self.rate_iops # End the fio_cmd - fio_cmd += ' %s > %s' % (self.names, out_file) + fio_cmd += " %s > %s" % (self.names, out_file) return fio_cmd - def mkrecovimage(self): """ Create a reecovery image """ - logger.info('Creating recovery image...') - monitoring.start( f"{self.run_dir}/recovery_pool_monitoring" ) + logger.info("Creating recovery image...") + monitoring.start(f"{self.run_dir}/recovery_pool_monitoring") if self.use_existing_volumes is False: self.cluster.rmpool(self.recov_pool_name, self.recov_pool_profile) - self.cluster.mkpool(self.recov_pool_name, self.recov_pool_profile, 'rbd') - for node in common.get_fqdn_list('clients'): + self.cluster.mkpool(self.recov_pool_name, self.recov_pool_profile, "rbd") + for node in common.get_fqdn_list("clients"): for volnum in range(0, self.volumes_per_client): node = node.rpartition("@")[2] - self.cluster.mkimage( f'cbt-rbdfio-recov-{node}-{volnum:d}', - self.vol_size, self.recov_pool_name, self.data_pool, - self.vol_object_size ) + self.cluster.mkimage( + f"cbt-rbdfio-recov-{node}-{volnum:d}", + self.vol_size, + self.recov_pool_name, + self.data_pool, + self.vol_object_size, + ) monitoring.stop() - def mkimages(self): """ Create an RBD pool and a number of volumes per client """ - monitoring.start( f"{self.run_dir}/pool_monitoring" ) + monitoring.start(f"{self.run_dir}/pool_monitoring") if self.use_existing_volumes is False: self.cluster.rmpool(self.pool_name, self.pool_profile) - self.cluster.mkpool(self.pool_name, self.pool_profile, 'rbd') + self.cluster.mkpool(self.pool_name, self.pool_profile, "rbd") if self.data_pool_profile: self.data_pool = self.pool_name + "-data" self.cluster.rmpool(self.data_pool, self.data_pool_profile) - self.cluster.mkpool(self.data_pool, self.data_pool_profile, 'rbd') - for node in common.get_fqdn_list('clients'): + self.cluster.mkpool(self.data_pool, self.data_pool_profile, "rbd") + for node in common.get_fqdn_list("clients"): for volnum in range(0, self.volumes_per_client): node = node.rpartition("@")[2] - self.cluster.mkimage( f'cbt-rbdfio-{node}-{volnum:d}', - self.vol_size, self.pool_name, self.data_pool, - self.vol_object_size) + self.cluster.mkimage( + f"cbt-rbdfio-{node}-{volnum:d}", self.vol_size, self.pool_name, self.data_pool, self.vol_object_size + ) monitoring.stop() - def prefill(self): """ Execute a FIO cmd to prefill the volumes @@ -299,32 +304,31 @@ def prefill(self): if not self.use_existing_volumes: rbd_base_name: str = self.config.get("rbdname", "cbt-rbdfio") for volnum in range(self.volumes_per_client): - rbd_name = f'{rbd_base_name}-`{common.get_fqdn_cmd()}`-{volnum:d}' - pre_cmd = '' + rbd_name = f"{rbd_base_name}-`{common.get_fqdn_cmd()}`-{volnum:d}" + pre_cmd = "" if not self.no_sudo: - pre_cmd += 'sudo ' - numjobs = self.prefill_vols['numjobs'] - bs = self.prefill_vols['blocksize'] - pre_cmd += ( f'{self.cmd_path} --ioengine=rbd --clientname=admin' - f' --pool={self.pool_name}' - f' --rbdname={rbd_name} --invalidate=0 --rw=write' - f' --numjobs={numjobs}' - f' --bs={bs}' - f' --size {self.vol_size:d}M {self.names}' - f' --output-format={self.fio_out_format} > /dev/null' ) - p = common.pdsh(settings.getnodes('clients'), pre_cmd) + pre_cmd += "sudo " + numjobs = self.prefill_vols["numjobs"] + bs = self.prefill_vols["blocksize"] + pre_cmd += ( + f"{self.cmd_path} --ioengine=rbd --clientname=admin" + f" --pool={self.pool_name}" + f" --rbdname={rbd_name} --invalidate=0 --rw=write" + f" --numjobs={numjobs}" + f" --bs={bs}" + f" --size {self.vol_size:d}M {self.names}" + f" --output-format={self.fio_out_format} > /dev/null" + ) + p = common.pdsh(settings.getnodes("clients"), pre_cmd) ps.append(p) for p in ps: p.wait() - def recovery_callback_blocking(self): - common.pdsh(settings.getnodes('clients'), 'sudo killall -2 fio').communicate() - + common.pdsh(settings.getnodes("clients"), "sudo killall -2 fio").communicate() def recovery_callback_background(self): - logger.info('Recovery thread completed!') - + logger.info("Recovery thread completed!") def parse(self, out_dir): """ @@ -337,7 +341,7 @@ def parse(self, out_dir): ] for file in files_to_process: with file.open("r", encoding="utf-8") as input_file: - output_file_name: str = f"{file.parent}/json_output{file.name[file.name.find('.'):]}" + output_file_name: str = f"{file.parent}/json_output{file.name[file.name.find('.') :]}" output_path = Path(output_file_name) found: bool = False with output_path.open("w", encoding="utf-8") as output_file: @@ -349,13 +353,12 @@ def parse(self, out_dir): output_file.write(line) found = False break - + if found: output_file.write(line) - def analyze(self, out_dir): - logger.info('Convert results to json format.') + logger.info("Convert results to json format.") self.parse(out_dir) def _get_iodepth_key(self, configuration_keys: list[str]) -> str: @@ -429,6 +432,6 @@ def _set_iodepth_for_every_volume(self, number_of_volumes: int, iodepth: int) -> queue_depths[volume_id] = iodepth return queue_depths - + def __str__(self): return "%s\n%s\n%s" % (self.run_dir, self.out_dir, super(LibrbdFio, self).__str__()) diff --git a/docs/CollectlIntegration_Phase1_Complete.md b/docs/CollectlIntegration_Phase1_Complete.md new file mode 100644 index 00000000..e69de29b diff --git a/post_processing/formatter/common_output_formatter.py b/post_processing/formatter/common_output_formatter.py index bee8b642..961e163b 100644 --- a/post_processing/formatter/common_output_formatter.py +++ b/post_processing/formatter/common_output_formatter.py @@ -45,7 +45,9 @@ import json import re from pathlib import Path -from typing import Optional +from typing import Optional, Union + +from typing_extensions import override from post_processing.formatter.base_formatter import BaseFormatter from post_processing.post_processing_types import CommonFormatDataType, InternalFormattedOutputType @@ -259,6 +261,7 @@ def _write_operation_results( # pylint: disable=too-many-locals except OSError as e: self.log.error("Failed to write hockey-stick file %s: %s", filename, e) + @override def process(self) -> None: """ Process input data and convert to intermediate format. @@ -290,13 +293,13 @@ def process(self) -> None: self.log.debug( "We have more than one directory for test run %s so using the compatibility method", testrun_id ) - self._process_compatibility_mode() + _ = self._process_compatibility_mode() # For compatibility mode, still need to add metadata and write self._add_common_metadata() self._add_peak_metrics() else: # Memory-efficient mode: results written during _process_single_testrun - self._process_single_testrun(testrun_directories[0]) + _ = self._process_single_testrun(testrun_directories[0]) def _find_maximum_bandwidth_and_iops_with_latency( self, test_run_data: CommonFormatDataType @@ -344,7 +347,35 @@ def _find_max_resource_usage(self, test_run_data: CommonFormatDataType) -> tuple for _, data in test_run_data.items(): if isinstance(data, dict): - max_cpu = max(max_cpu, float(data["cpu"])) - # max memory here, when we start recording it + # Handle multi-source resource data structure + # data["cpu"] is a dict like {"fio": "10.5", "collectl": "12.3"} + cpu_data: Union[dict[str, str], str, int, float] = data.get("cpu", {}) + if isinstance(cpu_data, dict): + # Find max across all sources + for source_cpu in cpu_data.values(): + try: + max_cpu = max(max_cpu, float(source_cpu)) + except (ValueError, TypeError): + self.log.warning("Invalid CPU value: %s", source_cpu) + else: + # Backward compatibility: handle old single-value format + try: + max_cpu = max(max_cpu, float(cpu_data)) + except (ValueError, TypeError): + self.log.warning("Invalid CPU value: %s", cpu_data) + + # Handle memory similarly (when implemented) + memory_data: Union[dict[str, str], str, int, float] = data.get("memory", {}) + if isinstance(memory_data, dict): + for source_memory in memory_data.values(): + try: + max_memory = max(max_memory, float(source_memory)) + except (ValueError, TypeError): + self.log.warning("Invalid memory value: %s", source_memory) + else: + try: + max_memory = max(max_memory, float(memory_data)) + except (ValueError, TypeError): + self.log.warning("Invalid memory value: %s", memory_data) return f"{max_cpu}", f"{max_memory}" diff --git a/post_processing/plotter/common_format_plotter.py b/post_processing/plotter/common_format_plotter.py index ddbf8f4b..73c81a9b 100644 --- a/post_processing/plotter/common_format_plotter.py +++ b/post_processing/plotter/common_format_plotter.py @@ -10,7 +10,7 @@ # the ModuleType does exists in the types module, so no idea why pylint is # flagging this from types import ModuleType -from typing import Optional +from typing import Any, Optional, Union, cast from matplotlib.axes import Axes @@ -41,8 +41,8 @@ class CommonFormatPlotter(ABC): The base class for plotting results curves """ - def __init__(self, plotter: ModuleType): - self._plotter = plotter + def __init__(self, plotter: ModuleType) -> None: + self._plotter: ModuleType = plotter @abstractmethod def draw_and_save(self) -> None: @@ -70,7 +70,7 @@ def _add_title(self, source_files: list[Path]) -> None: else: title = self._construct_title_from_list_of_file_names(source_files) - self._plotter.title(title) + _: Any = self._plotter.title(title) def _construct_title_from_list_of_file_names(self, file_paths: list[Path]) -> str: """ @@ -142,8 +142,8 @@ def _set_axis(self, maximum_values: Optional[tuple[int, int]] = None) -> None: maximum_x = maximum_values[0] maximum_y = maximum_values[1] - self._plotter.xlim(0, maximum_x) - self._plotter.ylim(0, maximum_y) + _: Any = self._plotter.xlim(0, maximum_x) + _: Any = self._plotter.ylim(0, maximum_y) def _sort_plot_data(self, unsorted_data: CommonFormatDataType) -> PlotDataType: """ @@ -255,6 +255,74 @@ def _validate_input_data(self, sorted_plot_data: PlotDataType) -> None: if not sorted_plot_data: raise ValueError("Cannot extract plot data from empty dataset") + def _process_cpu_data( + self, + data: dict[str, str], + queue_depth: str, + cpu_plotter: CPUPlotter, + resource_plotting_enabled: bool, + ) -> bool: + """ + Process CPU data from a data point, handling both single-value and multi-source formats. + + Args: + data: Data dictionary containing CPU information + queue_depth: Queue depth identifier for error messages + cpu_plotter: CPU plotter to receive CPU data + resource_plotting_enabled: Whether resource plotting is requested + + Returns: + True if CPU data was successfully processed, False otherwise + """ + if not resource_plotting_enabled: + return False + + cpu_value: Union[dict[str, str], str, None] = data.get("cpu") + if cpu_value is None: + return False + + # Pass CPU data directly to plotter - it handles both dict and single value formats + try: + if isinstance(cpu_value, dict): + # Validate that dict contains at least one valid numeric value + valid_sources = 0 + cpu_dict = cast(dict[str, str], cpu_value) + for source, value in cpu_dict.items(): + try: + _ = float(value) # Validate it's convertible to float + valid_sources += 1 + except (ValueError, TypeError): + log.warning( + "Invalid CPU value from source '%s' for queue depth %s: %s. Skipping this source.", + source, + queue_depth, + value, + ) + + if valid_sources == 0: + log.warning( + "No valid CPU values found for queue depth %s: %s. Skipping CPU data.", + queue_depth, + cpu_value, + ) + return False + + # Pass the entire dict to plotter so it can plot separate lines + cpu_plotter.add_y_data(cpu_value) + return True + + # Single value format (backward compatibility) + _ = float(cpu_value) # Validate it's convertible to float + cpu_plotter.add_y_data(cpu_value) + return True + except (ValueError, TypeError): + log.warning( + "Invalid CPU value for queue depth %s: %s. Skipping CPU data.", + queue_depth, + cpu_value, + ) + return False + def _process_data_point( # pylint: disable=too-many-arguments,too-many-positional-arguments self, data: dict[str, str], @@ -291,7 +359,7 @@ def _process_data_point( # pylint: disable=too-many-arguments,too-many-position # Validate latency is numeric latency_value = data["latency"] try: - float(latency_value) # Validate it's convertible to float + _ = float(latency_value) # Validate it's convertible to float except (ValueError, TypeError) as e: raise ValueError(f"Invalid latency value for queue depth {queue_depth}: {latency_value}") from e @@ -302,23 +370,7 @@ def _process_data_point( # pylint: disable=too-many-arguments,too-many-position io_plotter.add_y_data(latency_value) # Handle CPU data with proper validation - resource_available = resource_plotting_enabled - if resource_plotting_enabled: - cpu_value = data.get("cpu") - if cpu_value is None: - resource_available = False - else: - # Validate CPU value is numeric - try: - float(cpu_value) - cpu_plotter.add_y_data(cpu_value) - except (ValueError, TypeError): - log.warning( - "Invalid CPU value for queue depth %s: %s. Skipping CPU data.", - queue_depth, - cpu_value, - ) - resource_available = False + resource_available = self._process_cpu_data(data, queue_depth, cpu_plotter, resource_plotting_enabled) # Calculate error bars error_bar = self._calculate_error_bar(data, plot_error_bars, resource_available) @@ -380,14 +432,13 @@ def _extract_plot_data( # pylint: disable=too-many-arguments,too-many-positiona # Set x-axis label once (from first valid data point) if x_label is None: x_label = point_result.x_label - main_axes.set_xlabel(x_label) # pyright: ignore[reportUnknownMemberType] + _: Any = main_axes.set_xlabel(x_label) # pyright: ignore[reportUnknownMemberType] # Disable resource plotting if CPU data unavailable (only check once) if resource_plotting_enabled and not point_result.resource_available: if not cpu_warning_logged: log.warning( - "Unable to plot CPU usage: CPU data not found in intermediate files. " - "Disabling resource usage plotting." + "Unable to plot CPU usage: CPU data not found in intermediate files. Disabling resource usage plotting." ) cpu_warning_logged = True resource_plotting_enabled = False @@ -474,11 +525,11 @@ def _save_plot(self, file_path: str) -> None: The bbox_inches="tight" option makes sure that the legend is included in the plot and not cut off """ - self._plotter.savefig(file_path, format=f"{PLOT_FILE_EXTENSION}", bbox_inches="tight") + _: Any = self._plotter.savefig(file_path, format=f"{PLOT_FILE_EXTENSION}", bbox_inches="tight") def _clear_plot(self) -> None: """ Clear the plot data """ - self._plotter.close() + _: Any = self._plotter.close() # self._plotter.clf() diff --git a/post_processing/plotter/cpu_plotter.py b/post_processing/plotter/cpu_plotter.py index 56b3fcfe..9f4bc95c 100644 --- a/post_processing/plotter/cpu_plotter.py +++ b/post_processing/plotter/cpu_plotter.py @@ -4,32 +4,101 @@ """ from logging import Logger, getLogger +from typing import Union + +from matplotlib.axes import Axes from post_processing.plotter.axis_plotter import AxisPlotter log: Logger = getLogger("plotter") -CPU_PLOT_DEFAULT_COLOUR: str = "xkcd:leaf green" # Leaf green from xkcd color survey CPU_Y_LABEL: str = "System CPU use (%)" CPU_PLOT_LABEL: str = "CPU use" +# Color mapping for different resource sources +CPU_SOURCE_COLOURS: dict[str, str] = { + "fio": "xkcd:leaf green", + "collectl": "xkcd:sky blue", + "default": "xkcd:orange", +} + class CPUPlotter(AxisPlotter): """ - A class to add the resource use measurements to a plot as separate axes + A class to add CPU usage measurements to a plot as separate axes. + + Supports both single-source (legacy) and multi-source formats: + - Legacy: data_value is a string "45.2" + - Multi-source: data_value is a dict {"fio": "45.2", "collectl": "47.8"} + + When multiple sources are present, plots separate lines for each source. """ - def add_y_data(self, data_value: str) -> None: + def __init__(self, main_axis: "Axes") -> None: + """Initialize CPUPlotter with support for multiple data sources.""" + super().__init__(main_axis) + # Store data per source: {"fio": [val1, val2, ...], "collectl": [...]} + self._y_data_by_source: dict[str, list[float]] = {} + + def add_y_data(self, data_value: Union[str, dict[str, str]]) -> None: """ - Add a point of CPU data for this plot + Add a point of CPU data for this plot. + + Supports both legacy single-value format and new multi-source format. - :param cpu_value: A single value for CPU usage - :type cpu_value: str + Args: + data_value: Either a single string value (legacy) or dict of {source: value} """ - self._y_data.append(float(data_value)) + if isinstance(data_value, dict): + # Multi-source format: {"fio": "45.2", "collectl": "47.8"} + for source, value in data_value.items(): + if source not in self._y_data_by_source: + self._y_data_by_source[source] = [] + try: + self._y_data_by_source[source].append(float(value)) + except (ValueError, TypeError) as e: + log.warning("Invalid CPU value for source %s: %s (%s)", source, value, e) + self._y_data_by_source[source].append(0.0) + else: + # Legacy single-value format: "45.2" + if "default" not in self._y_data_by_source: + self._y_data_by_source["default"] = [] + try: + self._y_data_by_source["default"].append(float(data_value)) + except (ValueError, TypeError) as e: + log.warning("Invalid CPU value: %s (%s)", data_value, e) + self._y_data_by_source["default"].append(0.0) def plot(self, x_data: list[float], colour: str = "") -> None: + """ + Plot CPU data, creating separate lines for each source. + + Args: + x_data: X-axis data points (typically queue depths or time) + colour: Ignored - colors are determined per source + """ + if not self._y_data_by_source: + log.debug("No CPU data to plot") + return + cpu_axis = self._main_axes.twinx() - self._label = CPU_PLOT_LABEL - self._y_label = CPU_Y_LABEL - self._plot(x_data=x_data, axis=cpu_axis, colour=CPU_PLOT_DEFAULT_COLOUR) + cpu_axis.set_ylabel(CPU_Y_LABEL) + + # Plot a line for each source + for source in sorted(self._y_data_by_source.keys()): + y_data = self._y_data_by_source[source] + + # Determine label and color for this source + if source == "default": + label = CPU_PLOT_LABEL + else: + label = f"{CPU_PLOT_LABEL} ({source})" + + source_colour = CPU_SOURCE_COLOURS.get(source, CPU_SOURCE_COLOURS["default"]) + + # Plot this source's data + cpu_axis.plot(x_data, y_data, label=label, color=source_colour, linestyle="-", linewidth=1.5, marker="o") + + # Add legend if multiple sources + if len(self._y_data_by_source) > 1: + cpu_axis.legend(loc="upper right") diff --git a/post_processing/plotter/memory_plotter.py b/post_processing/plotter/memory_plotter.py index db884d63..aae02e2c 100644 --- a/post_processing/plotter/memory_plotter.py +++ b/post_processing/plotter/memory_plotter.py @@ -4,33 +4,105 @@ """ from logging import Logger, getLogger -from typing import Union +from typing import Any, Union + +from matplotlib.axes import Axes +from matplotlib.axes._axes import Axes +from typing_extensions import override from post_processing.plotter.axis_plotter import AxisPlotter log: Logger = getLogger("plotter") -MEMORY_PLOT_DEFAULT_COLOUR: str = "#4b006e" MEMORY_Y_LABEL: str = "Memory use (Mb)" MEMORY_PLOT_LABEL: str = "Memory use" +# Color mapping for different resource sources +MEMORY_SOURCE_COLOURS: dict[str, str] = { + "fio": "xkcd:purple", + "collectl": "xkcd:red", + "default": "xkcd:orange", +} + class MemoryPlotter(AxisPlotter): """ - A class to add the resource use measurements to a plot as separate axes + A class to add memory usage measurements to a plot as separate axes. + + Supports both single-source (legacy) and multi-source formats: + - Legacy: data_value is a string "1024.5" + - Multi-source: data_value is a dict {"fio": "1024.5", "collectl": "2048.0"} + + When multiple sources are present, plots separate lines for each source. """ - def add_y_data(self, data_value: str) -> None: + def __init__(self, main_axis: Axes) -> None: + """Initialize MemoryPlotter with support for multiple data sources.""" + super().__init__(main_axis) + # Store data per source: {"fio": [val1, val2, ...], "collectl": [...]} + self._y_data_by_source: dict[str, list[float]] = {} + + @override + def add_y_data(self, data_value: Union[str, dict[str, str]]) -> None: """ - Add a point of memory usage data for this plot + Add a point of memory data for this plot. + + Supports both legacy single-value format and new multi-source format. - :param memory_value: A single value for memory usage - :type memory_value: str + Args: + data_value: Either a single string value (legacy) or dict of {source: value} """ - self._y_data.append(float(data_value)) + if isinstance(data_value, dict): + # Multi-source format: {"fio": "1024.5", "collectl": "2048.0"} + for source, value in data_value.items(): + if source not in self._y_data_by_source: + self._y_data_by_source[source] = [] + try: + self._y_data_by_source[source].append(float(value)) + except (ValueError, TypeError) as e: + log.warning("Invalid memory value for source %s: %s (%s)", source, value, e) + self._y_data_by_source[source].append(0.0) + else: + # Legacy single-value format: "1024.5" + if "default" not in self._y_data_by_source: + self._y_data_by_source["default"] = [] + try: + self._y_data_by_source["default"].append(float(data_value)) + except (ValueError, TypeError) as e: + log.warning("Invalid memory value: %s (%s)", data_value, e) + self._y_data_by_source["default"].append(0.0) + @override def plot(self, x_data: list[Union[int, float]], colour: str = "") -> None: - memory_axis = self._main_axes.twinx() - self._label = MEMORY_PLOT_LABEL - self._y_label = MEMORY_Y_LABEL - self._plot(x_data=x_data, axis=memory_axis, colour=MEMORY_PLOT_DEFAULT_COLOUR) + """ + Plot memory data, creating separate lines for each source. + + Args: + x_data: X-axis data points (typically queue depths or time) + colour: Ignored - colors are determined per source + """ + if not self._y_data_by_source: + log.debug("No memory data to plot") + return + + memory_axis: Axes = self._main_axes.twinx() + memory_axis.set_ylabel(MEMORY_Y_LABEL) + + # Plot a line for each source + for source in sorted(self._y_data_by_source.keys()): + y_data = self._y_data_by_source[source] + + # Determine label and color for this source + if source == "default": + label = MEMORY_PLOT_LABEL + else: + label = f"{MEMORY_PLOT_LABEL} ({source})" + + source_colour = MEMORY_SOURCE_COLOURS.get(source, MEMORY_SOURCE_COLOURS["default"]) + + # Plot this source's data + memory_axis.plot(x_data, y_data, label=label, color=source_colour, linestyle="-", linewidth=1.5, marker="s") + + # Add legend if multiple sources + if len(self._y_data_by_source) > 1: + memory_axis.legend(loc="upper right") diff --git a/post_processing/run_results/resource_result_factory.py b/post_processing/run_results/resource_result_factory.py new file mode 100644 index 00000000..e008a223 --- /dev/null +++ b/post_processing/run_results/resource_result_factory.py @@ -0,0 +1,60 @@ +""" +Factory for discovering and creating all available resource result parsers. + +This module provides functionality to automatically discover and instantiate +all available resource monitoring parsers (FIO, Collectl, etc.) for a given +benchmark output file. +""" + +from logging import Logger, getLogger +from pathlib import Path + +from post_processing.run_results.resource_result import ResourceResult +from post_processing.run_results.resources.collectl_resource import CollectlResource +from post_processing.run_results.resources.fio_resource import FIOResource + +log: Logger = getLogger("formatter") + + +def get_all_resources(file_path: Path) -> list[ResourceResult]: + """ + Discover and instantiate all available resource parsers for a benchmark file. + + This function attempts to create resource parsers for all available monitoring + sources. It will try FIO (embedded in benchmark output) and Collectl (separate + monitoring files) if available. + + Args: + file_path: Path to the benchmark output file (e.g., json_output.0) + + Returns: + List of ResourceResult instances for all available sources. + Returns empty list if no sources are available. + """ + resources: list[ResourceResult] = [] + + # Always try FIO resource (embedded in benchmark output) + try: + fio_resource = FIOResource(file_path) + resources.append(fio_resource) + log.debug("Added FIO resource for %s", file_path) + except Exception as e: + log.warning("Could not create FIO resource for %s: %s", file_path, e) + + # Check for collectl data + collectl_dir = file_path.parent / "collectl" + if collectl_dir.exists() and collectl_dir.is_dir(): + try: + collectl_resource = CollectlResource(file_path) + resources.append(collectl_resource) + log.debug("Added Collectl resource for %s", file_path) + except Exception as e: + log.warning("Could not create Collectl resource for %s: %s", file_path, e) + + if not resources: + log.error("No resource parsers available for %s", file_path) + + return resources + + +# Made with Bob diff --git a/post_processing/run_results/resources/collectl_resource.py b/post_processing/run_results/resources/collectl_resource.py new file mode 100644 index 00000000..0a0130c8 --- /dev/null +++ b/post_processing/run_results/resources/collectl_resource.py @@ -0,0 +1,209 @@ +""" +Process CPU statistics from Collectl monitoring output. + +Collectl is a system monitoring tool that captures detailed performance metrics. +This module parses collectl's semicolon-separated CPU output files to extract +CPU usage statistics. +""" + +from logging import Logger, getLogger +from pathlib import Path +from typing import Any + +from post_processing.run_results.resource_result import ResourceResult + +log: Logger = getLogger("formatter") + + +class CollectlResource(ResourceResult): + """ + Processes resource usage statistics from Collectl monitoring output. + + Collectl produces detailed system monitoring data in semicolon-separated + format. This class extracts CPU usage by aggregating across all cores + and time samples. + + The collectl output files are expected to be in a 'collectl' subdirectory + relative to the benchmark output file, with filenames matching the pattern: + -.cpu + """ + + @property + def source(self) -> str: + """Return the source identifier for this resource parser.""" + return "collectl" + + def _get_resource_output_file_from_file_path(self, file_path: Path) -> Path: + """ + Locate the collectl CPU file from the benchmark output file path. + + Args: + file_path: Path to benchmark output (e.g., .../json_output.0) + + Returns: + Path to collectl .cpu file + + Raises: + FileNotFoundError: If collectl directory or CPU file not found + """ + collectl_dir = file_path.parent / "collectl" + + if not collectl_dir.exists(): + raise FileNotFoundError(f"Collectl directory not found: {collectl_dir}") + + # Find .cpu files (format: hostname-YYYYMMDD.cpu) + cpu_files = list(collectl_dir.glob("*.cpu")) + + if not cpu_files: + raise FileNotFoundError(f"No .cpu files found in {collectl_dir}") + + if len(cpu_files) > 1: + log.warning("Multiple .cpu files found in %s, using first: %s", collectl_dir, cpu_files[0]) + + return cpu_files[0] + + def _parse(self, data: dict[str, Any]) -> None: + """ + Parse collectl CPU data and calculate average CPU usage. + + Reads the semicolon-separated CPU file, extracts per-core metrics, + and calculates the average total CPU usage across all cores and time samples. + + Args: + data: Not used for collectl (reads directly from file) + """ + try: + cpu_usage = self._parse_cpu_file() + self._cpu = f"{cpu_usage:.2f}" + self._memory = "0.00" # Memory not implemented yet + self._has_been_parsed = True + except Exception as e: + log.error("Failed to parse collectl data from %s: %s", self._resource_file_path, e) + self._cpu = "0.00" + self._memory = "0.00" + self._has_been_parsed = True + + def _parse_cpu_file(self) -> float: + """ + Parse collectl .cpu file and calculate average CPU usage. + + Returns: + Average CPU usage percentage across all cores and time samples + """ + with open(self._resource_file_path, encoding="utf-8") as f: + lines = f.readlines() + + # Find header line (starts with #Date;Time;) + header_line = None + data_start_idx = 0 + + for idx, line in enumerate(lines): + if line.startswith("#Date;Time;"): + header_line = line + data_start_idx = idx + 1 + break + + if not header_line: + raise ValueError("Could not find header line in collectl CPU file") + + # Parse header to find CPU column indices + headers = header_line.strip().split(";") + cpu_indices = self._find_cpu_columns(headers) + + if not cpu_indices["user"] and not cpu_indices["totl"]: + raise ValueError("No CPU columns found in collectl header") + + # Parse data lines and calculate average + total_cpu_samples: list[float] = [] + + for line in lines[data_start_idx:]: + line = line.strip() + if not line or line.startswith("#"): + continue + + values = line.split(";") + if len(values) < len(headers): + log.warning("Skipping malformed line: %s", line) + continue + + # Calculate total CPU for this sample (sum across all cores) + sample_cpu = self._calculate_sample_cpu(values, cpu_indices) + total_cpu_samples.append(sample_cpu) + + if not total_cpu_samples: + log.warning("No valid CPU samples found in %s", self._resource_file_path) + return 0.0 + + # Return average CPU across all samples + avg_cpu = sum(total_cpu_samples) / len(total_cpu_samples) + log.debug("Parsed %d CPU samples, average: %.2f%%", len(total_cpu_samples), avg_cpu) + + return avg_cpu + + def _find_cpu_columns(self, headers: list[str]) -> dict[str, list[int]]: + """ + Find column indices for CPU metrics (User%, Sys%, etc.) for each core. + + Args: + headers: List of column headers from collectl file + + Returns: + Dict mapping metric names to lists of column indices + """ + cpu_columns: dict[str, list[int]] = {"user": [], "sys": [], "totl": []} + + for idx, header in enumerate(headers): + # Match patterns like [CPU:0]User%, [CPU:15]Sys%, etc. + if "User%" in header: + cpu_columns["user"].append(idx) + elif "Sys%" in header: + cpu_columns["sys"].append(idx) + elif "Totl%" in header: + cpu_columns["totl"].append(idx) + + return cpu_columns + + def _calculate_sample_cpu(self, values: list[str], cpu_indices: dict[str, list[int]]) -> float: + """ + Calculate total CPU usage for a single time sample. + + Args: + values: List of values from a data line + cpu_indices: Dict of CPU column indices + + Returns: + Average CPU usage across all cores for this sample + """ + # If Totl% is available, use it directly + if cpu_indices["totl"]: + totals = [float(values[idx]) for idx in cpu_indices["totl"] if idx < len(values)] + return sum(totals) / len(totals) if totals else 0.0 + + # Otherwise calculate from User% + Sys% + num_cores = len(cpu_indices["user"]) + if num_cores == 0: + return 0.0 + + core_totals: list[float] = [] + for core_idx in range(num_cores): + user_idx = cpu_indices["user"][core_idx] + sys_idx = cpu_indices["sys"][core_idx] + + if user_idx < len(values) and sys_idx < len(values): + user = float(values[user_idx]) + sys = float(values[sys_idx]) + core_totals.append(user + sys) + + return sum(core_totals) / len(core_totals) if core_totals else 0.0 + + def _read_results_from_file(self) -> dict[str, Any]: + """ + Override parent method - collectl doesn't use JSON format. + + Returns: + Empty dict (parsing happens in _parse_cpu_file) + """ + return {} + + +# Made with Bob diff --git a/post_processing/run_results/run_result.py b/post_processing/run_results/run_result.py index cabb4cc8..f03c6034 100644 --- a/post_processing/run_results/run_result.py +++ b/post_processing/run_results/run_result.py @@ -19,8 +19,7 @@ ) from post_processing.run_results.benchmark_result import BenchmarkResult from post_processing.run_results.resource_result import ResourceResult - -# from post_processing.run_results.resources.resource_result import ResourceResult +from post_processing.run_results.resource_result_factory import get_all_resources log: Logger = getLogger("formatter") @@ -218,7 +217,7 @@ def _build_test_result_data( test_config: tuple[str, str, str, str], io_details: IodepthDataType, global_details: dict[str, str], - resource_data: dict[str, str], + resource_data: dict[str, dict[str, str]], ) -> InternalNumJobsDataType: """ Build the complete nested data structure for a test result. @@ -227,7 +226,8 @@ def _build_test_result_data( test_config: Tuple of (operation, blocksize, iodepth, number_of_jobs) io_details: Merged IO performance details global_details: Global benchmark options - resource_data: Resource usage statistics + resource_data: Resource usage statistics with structure: + {"cpu": {"source1": "value1", ...}, "memory": {...}} Returns: Nested dictionary structure: {numjobs: {blocksize: {iodepth: data}}} @@ -235,10 +235,11 @@ def _build_test_result_data( _, blocksize, iodepth, number_of_jobs = test_config # Build from innermost to outermost level + # Merge all data including the nested resource data iodepth_data = {**global_details, **io_details, **resource_data} iodepth_details = {iodepth: iodepth_data} blocksize_details = cast(InternalBlocksizeDataType, {blocksize: iodepth_details}) - numjobs_details = cast(InternalNumJobsDataType, {number_of_jobs: blocksize_details}) + numjobs_details: InternalNumJobsDataType = {number_of_jobs: blocksize_details} return numjobs_details @@ -384,6 +385,28 @@ def _write_and_clear_timeseries_by_directory(self) -> None: self._timeseries_by_directory.clear() log.debug("Cleared timeseries data from memory") + def _collect_multi_source_resources(self, resources: list[ResourceResult]) -> dict[str, dict[str, str]]: + """ + Collect resource data from multiple sources into nested dict format. + + Args: + resources: List of ResourceResult instances + + Returns: + Dict with structure: {"cpu": {"source1": "value1", ...}, "memory": {...}} + """ + cpu_data: dict[str, str] = {} + memory_data: dict[str, str] = {} + + for resource in resources: + source = resource.source + resource_dict = resource.get() + + cpu_data[source] = resource_dict.get("cpu", "0.00") + memory_data[source] = resource_dict.get("memory", "0.00") + + return {"cpu": cpu_data, "memory": memory_data} + def _convert_file(self, file_path: Path) -> None: """ Convert an individual benchmark result file to the common intermediate format. @@ -392,6 +415,8 @@ def _convert_file(self, file_path: Path) -> None: statistics, and stores them in the internal data structure organized by operation type, blocksize, and IO depth. + Now supports multiple resource sources (FIO, Collectl, etc.) simultaneously. + If include_timeseries is True, also extracts time-series data from log files. Args: @@ -402,17 +427,22 @@ def _convert_file(self, file_path: Path) -> None: KeyError: If required data fields are missing from results """ try: - # Use factory methods to get the correct classes + # Use factory method for benchmark result io: BenchmarkResult = self._create_benchmark_result(file_path) - resource: ResourceResult = self._create_resource_result(file_path) + + # Get ALL available resource sources using factory + resources: list[ResourceResult] = get_all_resources(file_path) test_config = self._extract_test_configuration(io) # Merge IO details with existing data if present io_details = self._merge_io_details(test_config, io.io_details) + # Collect resource data from all sources + resource_data = self._collect_multi_source_resources(resources) + # Build complete test result data structure - numjobs_details = self._build_test_result_data(test_config, io_details, io.global_options, resource.get()) + numjobs_details = self._build_test_result_data(test_config, io_details, io.global_options, resource_data) # Update internal processed data self._update_processed_data(test_config, numjobs_details) diff --git a/tests/test_collectl_resource.py b/tests/test_collectl_resource.py new file mode 100644 index 00000000..bc3f025e --- /dev/null +++ b/tests/test_collectl_resource.py @@ -0,0 +1,325 @@ +""" +Unit tests for CollectlResource class +""" + +# pyright: strict, reportPrivateUsage=false + +import tempfile +from pathlib import Path + +import pytest + +from post_processing.run_results.resources.collectl_resource import CollectlResource + + +class TestCollectlResourceInitialization: + """Test CollectlResource initialization""" + + def test_initialization_with_valid_setup(self) -> None: + """Test initialization with proper collectl directory structure""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test_file.json" + file_path.touch() + + # Create collectl directory with a CPU file + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text("#Date;Time;[CPU:0]Totl%\n20260619;17:06:30;50\n") + + resource = CollectlResource(file_path) + + assert resource.source == "collectl" + # Accessing cpu property triggers parsing + assert resource.cpu == "50.00" + assert resource.memory == "0.00" + + def test_initialization_without_collectl_directory(self) -> None: + """Test initialization fails when collectl directory doesn't exist""" + with tempfile.TemporaryDirectory() as tmpdir: + file_path = Path(tmpdir) / "test_file.json" + file_path.touch() + + with pytest.raises(FileNotFoundError, match="Collectl directory not found"): + CollectlResource(file_path) + + def test_initialization_without_cpu_files(self) -> None: + """Test initialization fails when no CPU files exist""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + + with pytest.raises(FileNotFoundError, match="No .cpu files found"): + CollectlResource(file_path) + + +class TestCollectlResourceParsing: + """Test CPU data parsing with various formats""" + + def test_parse_with_totl_column(self) -> None: + """Test parsing CPU data with Totl% column""" + cpu_data = """#Date;Time;[CPU:0]User%;[CPU:0]Sys%;[CPU:0]Totl%;[CPU:1]User%;[CPU:1]Sys%;[CPU:1]Totl% +20260619;17:06:30;30;20;50;32;18;50 +20260619;17:06:40;28;22;50;30;20;50 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text(cpu_data) + + resource = CollectlResource(file_path) + + assert resource.cpu == "50.00" + assert resource.memory == "0.00" + + def test_parse_without_totl_column(self) -> None: + """Test parsing CPU data without Totl% (calculates from User+Sys)""" + cpu_data = """#Date;Time;[CPU:0]User%;[CPU:0]Sys%;[CPU:1]User%;[CPU:1]Sys% +20260619;17:06:30;30;20;32;18 +20260619;17:06:40;28;22;30;20 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text(cpu_data) + + resource = CollectlResource(file_path) + + # Sample 1: (30+20 + 32+18)/2 = 50 + # Sample 2: (28+22 + 30+20)/2 = 50 + assert resource.cpu == "50.00" + + def test_parse_with_comments_and_malformed_lines(self) -> None: + """Test parsing CPU data with comments and malformed lines""" + cpu_data = """# This is a comment +#Date;Time;[CPU:0]Totl% +# Another comment +20260619;17:06:30;50 +malformed line without enough fields +20260619;17:06:40;50 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text(cpu_data) + + resource = CollectlResource(file_path) + + # Should skip malformed lines and average the good ones + assert resource.cpu == "50.00" + + def test_parse_single_core(self) -> None: + """Test parsing CPU data with single core""" + cpu_data = """#Date;Time;[CPU:0]Totl% +20260619;17:06:30;50 +20260619;17:06:40;50 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text(cpu_data) + + resource = CollectlResource(file_path) + + # Accessing cpu property triggers parsing + assert resource.cpu == "50.00" + + def test_parse_multiple_cores(self) -> None: + """Test parsing CPU data with multiple cores""" + cpu_data = """#Date;Time;[CPU:0]Totl%;[CPU:1]Totl%;[CPU:2]Totl%;[CPU:3]Totl% +20260619;17:06:30;40;50;60;70 +20260619;17:06:40;50;60;70;80 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text(cpu_data) + + resource = CollectlResource(file_path) + + # Sample 1: (40+50+60+70)/4 = 55 + # Sample 2: (50+60+70+80)/4 = 65 + # Average: 60.0 + assert resource.cpu == "60.00" + + def test_parse_empty_file(self) -> None: + """Test parsing empty CPU file returns 0""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text("") + + resource = CollectlResource(file_path) + + assert resource.cpu == "0.00" + + def test_parse_no_data_rows(self) -> None: + """Test parsing CPU file with header but no data""" + cpu_data = """#Date;Time;[CPU:0]Totl% +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text(cpu_data) + + resource = CollectlResource(file_path) + + assert resource.cpu == "0.00" + + +class TestCollectlResourceGet: + """Test get() method""" + + def test_get_returns_correct_format(self) -> None: + """Test that get() returns correct dictionary format""" + cpu_data = """#Date;Time;[CPU:0]Totl% +20260619;17:06:30;45 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text(cpu_data) + + resource = CollectlResource(file_path) + result = resource.get() + + assert result == {"cpu": "45.00", "memory": "0.00", "source": "collectl"} + + def test_get_triggers_parsing(self) -> None: + """Test get() automatically triggers parsing""" + cpu_data = """#Date;Time;[CPU:0]Totl% +20260619;17:06:30;50 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text(cpu_data) + + resource = CollectlResource(file_path) + result = resource.get() + + # get() triggers parsing automatically + assert result == {"cpu": "50.00", "memory": "0.00", "source": "collectl"} + + +class TestCollectlResourceEdgeCases: + """Test edge cases and error conditions""" + + def test_zero_cpu_values(self) -> None: + """Test handling of zero CPU values""" + cpu_data = """#Date;Time;[CPU:0]Totl% +20260619;17:06:30;0 +20260619;17:06:40;0 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text(cpu_data) + + resource = CollectlResource(file_path) + + assert resource.cpu == "0.00" + + def test_decimal_cpu_values(self) -> None: + """Test handling of decimal CPU values""" + cpu_data = """#Date;Time;[CPU:0]Totl% +20260619;17:06:30;45.5 +20260619;17:06:40;54.5 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text(cpu_data) + + resource = CollectlResource(file_path) + + assert resource.cpu == "50.00" + + def test_multiple_cpu_files_uses_first(self) -> None: + """Test that first CPU file is used when multiple exist""" + cpu_data1 = """#Date;Time;[CPU:0]Totl% +20260619;17:06:30;30 +""" + cpu_data2 = """#Date;Time;[CPU:0]Totl% +20260619;17:06:30;70 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "test.json" + file_path.touch() + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + + # Create two CPU files + (collectl_dir / "host1-20260619.cpu").write_text(cpu_data1) + (collectl_dir / "host2-20260619.cpu").write_text(cpu_data2) + + resource = CollectlResource(file_path) + + # Should use one of them (order may vary, but should be valid) + cpu_value = float(resource.cpu) + assert cpu_value in [30.0, 70.0] + + +# Made with Bob diff --git a/tests/test_common_format_plotter.py b/tests/test_common_format_plotter.py index 99cd8835..d7624288 100644 --- a/tests/test_common_format_plotter.py +++ b/tests/test_common_format_plotter.py @@ -30,7 +30,6 @@ class ConcreteCommonFormatPlotter(CommonFormatPlotter): def draw_and_save(self) -> None: """Dummy implementation""" - pass def _generate_output_file_name(self, files: list) -> str: # type: ignore[type-arg] """Dummy implementation""" @@ -511,9 +510,7 @@ def test_extract_plot_data_invalid_cpu_value(self, mock_log: MagicMock) -> None: mock_log.warning.assert_called() # Check for either the specific invalid CPU warning or the general CPU data not found warning warning_message = str(mock_log.warning.call_args) - self.assertTrue( - "Invalid CPU value" in warning_message or "CPU data not found" in warning_message - ) + self.assertTrue("Invalid CPU value" in warning_message or "CPU data not found" in warning_message) # Should still process valid data self.assertEqual(len(result.x_data), 2) @@ -548,6 +545,116 @@ def test_extract_plot_data_missing_cpu_disables_resource_plotting(self, mock_log # Resource plotting should be disabled self.assertFalse(result.plot_resource_usage) + @patch("post_processing.plotter.common_format_plotter.log") + def test_extract_plot_data_multi_source_cpu(self, mock_log: MagicMock) -> None: + """Test _extract_plot_data handles multi-source CPU data (dict format)""" + mock_axes = MagicMock(spec=Axes) + mock_io_plotter = MagicMock(spec=IOPlotter) + mock_cpu_plotter = MagicMock(spec=CPUPlotter) + + sorted_plot_data = { # type: ignore[arg-type] + "1": { + "blocksize": "4096", + "bandwidth_bytes": "1000000", + "iops": "250", + "latency": "5000000", + "cpu": {"collectl": "39.75", "fio": "0.288828"}, # Multi-source CPU data + }, + "2": { + "blocksize": "4096", + "bandwidth_bytes": "2000000", + "iops": "500", + "latency": "4000000", + "cpu": {"collectl": "45.5", "fio": "0.5"}, + }, + } + + result = self.plotter._extract_plot_data( + sorted_plot_data, mock_axes, mock_io_plotter, mock_cpu_plotter, False, True + ) + + # Should successfully extract data from both entries + self.assertEqual(len(result.x_data), 2) + # Resource plotting should remain enabled + self.assertTrue(result.plot_resource_usage) + # CPU plotter should have received the dict for each data point + self.assertEqual(mock_cpu_plotter.add_y_data.call_count, 2) + # First call should receive the entire dict + first_call_arg = mock_cpu_plotter.add_y_data.call_args_list[0][0][0] + self.assertIsInstance(first_call_arg, dict) + self.assertEqual(first_call_arg, {"collectl": "39.75", "fio": "0.288828"}) + # Second call should receive the entire dict + second_call_arg = mock_cpu_plotter.add_y_data.call_args_list[1][0][0] + self.assertIsInstance(second_call_arg, dict) + self.assertEqual(second_call_arg, {"collectl": "45.5", "fio": "0.5"}) + + @patch("post_processing.plotter.common_format_plotter.log") + def test_extract_plot_data_multi_source_cpu_with_invalid_values(self, mock_log: MagicMock) -> None: + """Test _extract_plot_data handles multi-source CPU data with some invalid values""" + mock_axes = MagicMock(spec=Axes) + mock_io_plotter = MagicMock(spec=IOPlotter) + mock_cpu_plotter = MagicMock(spec=CPUPlotter) + + sorted_plot_data = { # type: ignore[arg-type] + "1": { + "blocksize": "4096", + "bandwidth_bytes": "1000000", + "iops": "250", + "latency": "5000000", + "cpu": {"collectl": "39.75", "fio": "invalid"}, # One invalid source + }, + } + + result = self.plotter._extract_plot_data( + sorted_plot_data, mock_axes, mock_io_plotter, mock_cpu_plotter, False, True + ) + + # Should log warning about invalid CPU value from specific source + mock_log.warning.assert_called() + warning_calls = [str(call) for call in mock_log.warning.call_args_list] + self.assertTrue(any("Invalid CPU value from source" in call for call in warning_calls)) + + # Should still pass the dict (with both valid and invalid values) to the plotter + # The plotter itself will handle filtering invalid values + self.assertEqual(len(result.x_data), 1) + self.assertTrue(result.plot_resource_usage) + mock_cpu_plotter.add_y_data.assert_called_once() + call_arg = mock_cpu_plotter.add_y_data.call_args[0][0] + self.assertIsInstance(call_arg, dict) + self.assertEqual(call_arg, {"collectl": "39.75", "fio": "invalid"}) + + @patch("post_processing.plotter.common_format_plotter.log") + def test_extract_plot_data_multi_source_cpu_all_invalid(self, mock_log: MagicMock) -> None: + """Test _extract_plot_data handles multi-source CPU data with all invalid values""" + mock_axes = MagicMock(spec=Axes) + mock_io_plotter = MagicMock(spec=IOPlotter) + mock_cpu_plotter = MagicMock(spec=CPUPlotter) + + sorted_plot_data = { # type: ignore[arg-type] + "1": { + "blocksize": "4096", + "bandwidth_bytes": "1000000", + "iops": "250", + "latency": "5000000", + "cpu": {"collectl": "invalid1", "fio": "invalid2"}, # All invalid + }, + } + + result = self.plotter._extract_plot_data( + sorted_plot_data, mock_axes, mock_io_plotter, mock_cpu_plotter, False, True + ) + + # Should log warnings about invalid CPU values + self.assertGreaterEqual(mock_log.warning.call_count, 2) + warning_calls = [str(call) for call in mock_log.warning.call_args_list] + # Should warn about no valid CPU values found + self.assertTrue(any("No valid CPU values found" in call for call in warning_calls)) + + # Should still process the data point but disable resource plotting + self.assertEqual(len(result.x_data), 1) + self.assertFalse(result.plot_resource_usage) + mock_cpu_plotter.add_y_data.assert_not_called() + class TestCommonFormatPlotterTitleGeneration(unittest.TestCase): """Test cases for title generation methods""" diff --git a/tests/test_common_output_formatter.py b/tests/test_common_output_formatter.py index 336448a0..5df675d4 100644 --- a/tests/test_common_output_formatter.py +++ b/tests/test_common_output_formatter.py @@ -106,8 +106,7 @@ def test_find_max_resource_usage(self) -> None: max_cpu, max_memory = self.formatter._find_max_resource_usage(test_data) self.assertEqual(max_cpu, "45.8") - # Note: max_memory is not currently implemented in the code - self.assertEqual(max_memory, "0") + self.assertEqual(max_memory, "2048.0") def test_find_max_resource_usage_with_empty_data(self) -> None: """Test finding maximum resource usage with empty data""" diff --git a/tests/test_cpu_plotter.py b/tests/test_cpu_plotter.py index 93602038..d396d631 100644 --- a/tests/test_cpu_plotter.py +++ b/tests/test_cpu_plotter.py @@ -13,8 +13,8 @@ from matplotlib.axes import Axes from post_processing.plotter.cpu_plotter import ( - CPU_PLOT_DEFAULT_COLOUR, CPU_PLOT_LABEL, + CPU_SOURCE_COLOURS, CPU_Y_LABEL, CPUPlotter, ) @@ -33,19 +33,32 @@ def setUp(self) -> None: def test_initialization(self) -> None: """Test CPUPlotter initialization""" self.assertEqual(self.plotter._main_axes, self.mock_axes) - self.assertEqual(self.plotter._y_data, []) + self.assertEqual(self.plotter._y_data_by_source, {}) - def test_add_y_data(self) -> None: - """Test adding CPU data""" + def test_add_y_data_legacy_format(self) -> None: + """Test adding CPU data in legacy string format""" self.plotter.add_y_data("45.5") self.plotter.add_y_data("67.8") - self.assertEqual(len(self.plotter._y_data), 2) - self.assertAlmostEqual(self.plotter._y_data[0], 45.5) - self.assertAlmostEqual(self.plotter._y_data[1], 67.8) - - def test_plot(self) -> None: - """Test plotting CPU data""" + self.assertIn("default", self.plotter._y_data_by_source) + self.assertEqual(len(self.plotter._y_data_by_source["default"]), 2) + self.assertAlmostEqual(self.plotter._y_data_by_source["default"][0], 45.5) + self.assertAlmostEqual(self.plotter._y_data_by_source["default"][1], 67.8) + + def test_add_y_data_multi_source_format(self) -> None: + """Test adding CPU data in multi-source dict format""" + self.plotter.add_y_data({"fio": "45.5", "collectl": "47.8"}) + self.plotter.add_y_data({"fio": "50.0", "collectl": "52.3"}) + + self.assertIn("fio", self.plotter._y_data_by_source) + self.assertIn("collectl", self.plotter._y_data_by_source) + self.assertEqual(len(self.plotter._y_data_by_source["fio"]), 2) + self.assertEqual(len(self.plotter._y_data_by_source["collectl"]), 2) + self.assertAlmostEqual(self.plotter._y_data_by_source["fio"][0], 45.5) + self.assertAlmostEqual(self.plotter._y_data_by_source["collectl"][0], 47.8) + + def test_plot_legacy_single_source(self) -> None: + """Test plotting CPU data with legacy single source""" self.plotter.add_y_data("50.0") self.plotter.add_y_data("60.0") @@ -55,19 +68,39 @@ def test_plot(self) -> None: # Should create twin axes self.mock_axes.twinx.assert_called_once() - # Should set label and y_label - self.assertEqual(self.plotter._label, CPU_PLOT_LABEL) - self.assertEqual(self.plotter._y_label, CPU_Y_LABEL) + # Should set y_label + self.mock_twin_axes.set_ylabel.assert_called_once_with(CPU_Y_LABEL) # Should call plot on twin axes - self.mock_twin_axes.set_ylabel.assert_called_once_with(CPU_Y_LABEL) self.mock_twin_axes.plot.assert_called_once() + def test_plot_multi_source(self) -> None: + """Test plotting CPU data with multiple sources""" + self.plotter.add_y_data({"fio": "50.0", "collectl": "52.0"}) + self.plotter.add_y_data({"fio": "60.0", "collectl": "62.0"}) + + x_data = [100.0, 200.0] + self.plotter.plot(x_data) + + # Should create twin axes + self.mock_axes.twinx.assert_called_once() + + # Should set y_label + self.mock_twin_axes.set_ylabel.assert_called_once_with(CPU_Y_LABEL) + + # Should call plot twice (once per source) + self.assertEqual(self.mock_twin_axes.plot.call_count, 2) + + # Should add legend for multiple sources + self.mock_twin_axes.legend.assert_called_once() + def test_cpu_constants(self) -> None: """Test CPU plotter constants""" - self.assertEqual(CPU_PLOT_DEFAULT_COLOUR, "xkcd:leaf green") self.assertEqual(CPU_Y_LABEL, "System CPU use (%)") self.assertEqual(CPU_PLOT_LABEL, "CPU use") + self.assertIn("fio", CPU_SOURCE_COLOURS) + self.assertIn("collectl", CPU_SOURCE_COLOURS) + self.assertIn("default", CPU_SOURCE_COLOURS) # Made with Bob diff --git a/tests/test_memory_plotter.py b/tests/test_memory_plotter.py index 2dd71542..284c9a72 100644 --- a/tests/test_memory_plotter.py +++ b/tests/test_memory_plotter.py @@ -2,13 +2,16 @@ Unit tests for the MemoryPlotter class """ -from unittest.mock import MagicMock, patch +# pyright: strict, reportPrivateUsage=false +# +# We are OK to ignore private use in unit tests as the whole point of the tests +# is to validate the functions contained in the module -import pytest +from unittest.mock import MagicMock from post_processing.plotter.memory_plotter import ( - MEMORY_PLOT_DEFAULT_COLOUR, MEMORY_PLOT_LABEL, + MEMORY_SOURCE_COLOURS, MEMORY_Y_LABEL, MemoryPlotter, ) @@ -23,18 +26,18 @@ def test_initialization(self) -> None: plotter = MemoryPlotter(main_axis=mock_axes) assert plotter._main_axes == mock_axes - assert plotter._y_data == [] - assert plotter._label == "" - assert plotter._y_label == "" + assert not plotter._y_data_by_source def test_memory_constants(self) -> None: """Test that memory constants are defined correctly""" - assert MEMORY_PLOT_DEFAULT_COLOUR == "#4b006e" assert MEMORY_Y_LABEL == "Memory use (Mb)" assert MEMORY_PLOT_LABEL == "Memory use" + assert "fio" in MEMORY_SOURCE_COLOURS + assert "collectl" in MEMORY_SOURCE_COLOURS + assert "default" in MEMORY_SOURCE_COLOURS - def test_add_y_data(self) -> None: - """Test adding memory data points""" + def test_add_y_data_legacy_format(self) -> None: + """Test adding memory data points in legacy string format""" mock_axes = MagicMock() plotter = MemoryPlotter(main_axis=mock_axes) @@ -42,7 +45,21 @@ def test_add_y_data(self) -> None: plotter.add_y_data("200.75") plotter.add_y_data("150.25") - assert plotter._y_data == [100.5, 200.75, 150.25] + assert "default" in plotter._y_data_by_source + assert plotter._y_data_by_source["default"] == [100.5, 200.75, 150.25] + + def test_add_y_data_multi_source_format(self) -> None: + """Test adding memory data in multi-source dict format""" + mock_axes = MagicMock() + plotter = MemoryPlotter(main_axis=mock_axes) + + plotter.add_y_data({"fio": "100.5", "collectl": "105.0"}) + plotter.add_y_data({"fio": "200.75", "collectl": "210.0"}) + + assert "fio" in plotter._y_data_by_source + assert "collectl" in plotter._y_data_by_source + assert plotter._y_data_by_source["fio"] == [100.5, 200.75] + assert plotter._y_data_by_source["collectl"] == [105.0, 210.0] def test_add_y_data_converts_string_to_float(self) -> None: """Test that add_y_data converts string values to float""" @@ -50,11 +67,11 @@ def test_add_y_data_converts_string_to_float(self) -> None: plotter = MemoryPlotter(main_axis=mock_axes) plotter.add_y_data("42") - assert plotter._y_data == [42.0] - assert isinstance(plotter._y_data[0], float) + assert plotter._y_data_by_source["default"] == [42.0] + assert isinstance(plotter._y_data_by_source["default"][0], float) - def test_plot(self) -> None: - """Test plotting memory data""" + def test_plot_legacy_single_source(self) -> None: + """Test plotting memory data with legacy single source""" mock_main_axes = MagicMock() mock_memory_axes = MagicMock() mock_main_axes.twinx.return_value = mock_memory_axes @@ -69,15 +86,39 @@ def test_plot(self) -> None: # Verify twinx was called to create secondary axis mock_main_axes.twinx.assert_called_once() - # Verify labels were set - assert plotter._label == MEMORY_PLOT_LABEL - assert plotter._y_label == MEMORY_Y_LABEL + # Verify set_ylabel was called + mock_memory_axes.set_ylabel.assert_called_once_with(MEMORY_Y_LABEL) # Verify plot was called on the memory axis mock_memory_axes.plot.assert_called_once() + def test_plot_multi_source(self) -> None: + """Test plotting memory data with multiple sources""" + mock_main_axes = MagicMock() + mock_memory_axes = MagicMock() + mock_main_axes.twinx.return_value = mock_memory_axes + + plotter = MemoryPlotter(main_axis=mock_main_axes) + plotter.add_y_data({"fio": "100", "collectl": "105"}) + plotter.add_y_data({"fio": "200", "collectl": "210"}) + + x_data: list[float] = [1.0, 2.0] + plotter.plot(x_data=x_data) + + # Verify twinx was called to create secondary axis + mock_main_axes.twinx.assert_called_once() + + # Verify set_ylabel was called + mock_memory_axes.set_ylabel.assert_called_once_with(MEMORY_Y_LABEL) + + # Verify plot was called twice (once per source) + assert mock_memory_axes.plot.call_count == 2 + + # Verify legend was added for multiple sources + mock_memory_axes.legend.assert_called_once() + def test_plot_with_custom_colour_ignored(self) -> None: - """Test that custom colour parameter is ignored and default is used""" + """Test that custom colour parameter is ignored""" mock_main_axes = MagicMock() mock_memory_axes = MagicMock() mock_main_axes.twinx.return_value = mock_memory_axes @@ -89,9 +130,8 @@ def test_plot_with_custom_colour_ignored(self) -> None: # Pass a custom colour, but it should be ignored plotter.plot(x_data=x_data, colour="#FF0000") - # Verify the default colour is used - call_args = mock_memory_axes.plot.call_args - assert MEMORY_PLOT_DEFAULT_COLOUR in str(call_args) + # Verify plot was called (colour is determined internally) + mock_memory_axes.plot.assert_called_once() def test_plot_sets_y_label_on_axis(self) -> None: """Test that plot sets the y-axis label""" @@ -108,4 +148,5 @@ def test_plot_sets_y_label_on_axis(self) -> None: # Verify set_ylabel was called with correct label mock_memory_axes.set_ylabel.assert_called_once_with(MEMORY_Y_LABEL) + # Made with Bob diff --git a/tests/test_resource_result_factory.py b/tests/test_resource_result_factory.py new file mode 100644 index 00000000..7635b1b8 --- /dev/null +++ b/tests/test_resource_result_factory.py @@ -0,0 +1,350 @@ +""" +Unit tests for resource_result_factory module +""" + +# pyright: strict, reportPrivateUsage=false + +import json +import tempfile +from pathlib import Path + +from post_processing.run_results.resource_result_factory import get_all_resources +from post_processing.run_results.resources.collectl_resource import CollectlResource +from post_processing.run_results.resources.fio_resource import FIOResource + + +class TestGetAllResourcesFIOOnly: + """Test get_all_resources with only FIO data available""" + + def test_fio_only_returns_single_resource(self) -> None: + """Test that only FIO resource is returned when collectl dir missing""" + fio_data = { + "jobs": [ + { + "job_name": "test", + "usr_cpu": 45.5, + "sys_cpu": 10.2, + "ctx": 1000, + "majf": 0, + "minf": 100, + } + ] + } + + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + resources = get_all_resources(file_path) + + assert len(resources) == 1 + assert isinstance(resources[0], FIOResource) + assert resources[0].source == "fio" + + def test_fio_only_with_empty_directory(self) -> None: + """Test FIO-only scenario with empty parent directory""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 30.0, "sys_cpu": 20.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + resources = get_all_resources(file_path) + + assert len(resources) == 1 + assert resources[0].source == "fio" + + +class TestGetAllResourcesWithInvalidData: + """Test get_all_resources with invalid data (both sources still created)""" + + def test_both_created_even_with_invalid_fio_json(self) -> None: + """Test that both resources are created even if FIO JSON is invalid""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text("invalid json data") + + # Create collectl directory with CPU file + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text("#Date;Time;[CPU:0]Totl%\n20260619;17:06:30;50\n") + + resources = get_all_resources(file_path) + + # Both resources are created (FIO handles invalid JSON gracefully) + assert len(resources) == 2 + sources = {r.source for r in resources} + assert sources == {"fio", "collectl"} + + def test_both_created_with_empty_fio_file(self) -> None: + """Test both resources created when FIO file is empty""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text("") + + # Create collectl directory + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text("#Date;Time;[CPU:0]Totl%\n20260619;17:06:30;45\n") + + resources = get_all_resources(file_path) + + # Both created (empty file is handled gracefully) + assert len(resources) == 2 + sources = {r.source for r in resources} + assert sources == {"fio", "collectl"} + + +class TestGetAllResourcesBothSources: + """Test get_all_resources with both FIO and Collectl available""" + + def test_both_sources_returns_two_resources(self) -> None: + """Test that both FIO and Collectl resources are returned""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 30.0, "sys_cpu": 15.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + # Create collectl directory + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text("#Date;Time;[CPU:0]Totl%\n20260619;17:06:30;50\n") + + resources = get_all_resources(file_path) + + assert len(resources) == 2 + + # Check that we have both sources + sources = {r.source for r in resources} + assert sources == {"fio", "collectl"} + + # Verify types + fio_resources = [r for r in resources if isinstance(r, FIOResource)] + collectl_resources = [r for r in resources if isinstance(r, CollectlResource)] + + assert len(fio_resources) == 1 + assert len(collectl_resources) == 1 + + def test_both_sources_order(self) -> None: + """Test that FIO is returned before Collectl (order matters for consistency)""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 25.0, "sys_cpu": 10.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text("#Date;Time;[CPU:0]Totl%\n20260619;17:06:30;60\n") + + resources = get_all_resources(file_path) + + # FIO should be first + assert resources[0].source == "fio" + assert resources[1].source == "collectl" + + +class TestGetAllResourcesMinimalScenarios: + """Test get_all_resources in minimal scenarios""" + + def test_fio_always_created_if_file_exists(self) -> None: + """Test that FIO resource is always created if file exists""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text("invalid json") + + # No collectl directory + resources = get_all_resources(file_path) + + # FIO is always created (handles invalid data gracefully) + assert len(resources) == 1 + assert resources[0].source == "fio" + + def test_fio_only_with_empty_collectl_dir(self) -> None: + """Test FIO-only when collectl dir exists but has no CPU files""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text("not valid json") + + # Create empty collectl directory (no .cpu files) + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + + resources = get_all_resources(file_path) + + # Only FIO (collectl has no CPU files) + assert len(resources) == 1 + assert resources[0].source == "fio" + + +class TestGetAllResourcesErrorHandling: + """Test error handling in get_all_resources""" + + def test_handles_collectl_exception_gracefully(self) -> None: + """Test that Collectl exceptions don't prevent FIO from being added""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 40.0, "sys_cpu": 20.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + # Create collectl dir but with no CPU files (will raise exception) + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + # Don't create any .cpu files + + resources = get_all_resources(file_path) + + # Should still get FIO even though Collectl failed + assert len(resources) == 1 + assert resources[0].source == "fio" + + def test_handles_missing_fio_file(self) -> None: + """Test that missing FIO file is handled gracefully""" + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "nonexistent.json" + # Don't create the file + + # Create valid collectl data + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text("#Date;Time;[CPU:0]Totl%\n20260619;17:06:30;55\n") + + resources = get_all_resources(file_path) + + # Both are created (FIO handles missing file gracefully) + assert len(resources) == 2 + sources = {r.source for r in resources} + assert sources == {"fio", "collectl"} + + +class TestGetAllResourcesCollectlDirectoryChecks: + """Test collectl directory existence checks""" + + def test_collectl_dir_is_file_not_directory(self) -> None: + """Test that collectl is skipped if it's a file, not a directory""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 35.0, "sys_cpu": 15.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + # Create 'collectl' as a file, not directory + collectl_file = base_path / "collectl" + collectl_file.write_text("this is a file") + + resources = get_all_resources(file_path) + + # Should only get FIO (collectl is not a directory) + assert len(resources) == 1 + assert resources[0].source == "fio" + + def test_collectl_dir_does_not_exist(self) -> None: + """Test that missing collectl directory is handled correctly""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 50.0, "sys_cpu": 10.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + # Don't create collectl directory + resources = get_all_resources(file_path) + + assert len(resources) == 1 + assert resources[0].source == "fio" + + +class TestGetAllResourcesIntegration: + """Integration tests for get_all_resources""" + + def test_realistic_scenario_both_sources(self) -> None: + """Test realistic scenario with both FIO and Collectl data""" + fio_data = { + "jobs": [ + { + "job_name": "seq_read", + "usr_cpu": 25.5, + "sys_cpu": 15.3, + "ctx": 5000, + "majf": 0, + "minf": 250, + } + ] + } + + cpu_data = """#Date;Time;[CPU:0]User%;[CPU:0]Sys%;[CPU:0]Totl%;[CPU:1]User%;[CPU:1]Sys%;[CPU:1]Totl% +20260619;17:06:30;30;20;50;32;18;50 +20260619;17:06:40;28;22;50;30;20;50 +20260619;17:06:50;29;21;50;31;19;50 +""" + + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "cephalasquad3-20260619.cpu" + cpu_file.write_text(cpu_data) + + resources = get_all_resources(file_path) + + assert len(resources) == 2 + + # Verify both resources can be used + for resource in resources: + result = resource.get() + assert "cpu" in result + assert "memory" in result + assert "source" in result + assert result["source"] in ["fio", "collectl"] + + def test_can_retrieve_data_from_all_resources(self) -> None: + """Test that data can be retrieved from all returned resources""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 20.0, "sys_cpu": 10.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base_path = Path(tmpdir) + file_path = base_path / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + collectl_dir = base_path / "collectl" + collectl_dir.mkdir() + cpu_file = collectl_dir / "host-20260619.cpu" + cpu_file.write_text("#Date;Time;[CPU:0]Totl%\n20260619;17:06:30;45\n") + + resources = get_all_resources(file_path) + + # Get data from all resources + results = [r.get() for r in resources] + + assert len(results) == 2 + assert all("cpu" in r for r in results) + assert all("memory" in r for r in results) + assert all("source" in r for r in results) + + # Verify sources are different + sources = [r["source"] for r in results] + assert set(sources) == {"fio", "collectl"} + + +# Made with Bob From 3e2ca41d2bb69262ccf5f350e2ed0c2431979cc8 Mon Sep 17 00:00:00 2001 From: Chris Harris Date: Mon, 29 Jun 2026 16:02:30 +0100 Subject: [PATCH 2/3] Monitoring: monitoring classes refactor Resource monitoring in CBT is contained within the monitoring class in CBT. This file contains multiple classes, one for each potential tool to use. To make this easier to maintain in the future and to meet current Python coding guidelines this should be split up into once class per file. These should have a common abstract base class which they all ingerit from, and a factory method to make sure the expected monitoring classes are run Signed-off-by: Chris Harris Assisted-by: IBM Bob 2.0 --- benchmark/cephtestrados.py | 10 +- benchmark/cosbench.py | 10 +- benchmark/fio.py | 6 +- benchmark/getput.py | 10 +- benchmark/hsbench.py | 6 +- benchmark/kvmrbdfio.py | 6 +- benchmark/librbdfio.py | 18 +-- benchmark/radosbench.py | 10 +- benchmark/rawfio.py | 6 +- benchmark/rbdfio.py | 14 +- cluster/ceph.py | 16 +- monitoring.py | 231 ----------------------------- monitoring/__init__.py | 0 monitoring/base.py | 26 ++++ monitoring/blktrace_monitoring.py | 50 +++++++ monitoring/collectl_monitoring.py | 36 +++++ monitoring/monitoring_factory.py | 79 ++++++++++ monitoring/perf_monitoring.py | 87 +++++++++++ monitoring/top_monitoring.py | 67 +++++++++ tests/test_monitoring_base.py | 73 +++++++++ tests/test_monitoring_blktrace.py | 160 ++++++++++++++++++++ tests/test_monitoring_collectl.py | 69 +++++++++ tests/test_monitoring_factory.py | 236 ++++++++++++++++++++++++++++++ tests/test_monitoring_perf.py | 179 ++++++++++++++++++++++ tests/test_monitoring_top.py | 159 ++++++++++++++++++++ tests/test_workloads.py | 6 +- workloads/workloads.py | 6 +- 27 files changed, 1283 insertions(+), 293 deletions(-) delete mode 100644 monitoring.py create mode 100644 monitoring/__init__.py create mode 100644 monitoring/base.py create mode 100644 monitoring/blktrace_monitoring.py create mode 100644 monitoring/collectl_monitoring.py create mode 100644 monitoring/monitoring_factory.py create mode 100644 monitoring/perf_monitoring.py create mode 100644 monitoring/top_monitoring.py create mode 100644 tests/test_monitoring_base.py create mode 100644 tests/test_monitoring_blktrace.py create mode 100644 tests/test_monitoring_collectl.py create mode 100644 tests/test_monitoring_factory.py create mode 100644 tests/test_monitoring_perf.py create mode 100644 tests/test_monitoring_top.py diff --git a/benchmark/cephtestrados.py b/benchmark/cephtestrados.py index cf110557..942acc45 100644 --- a/benchmark/cephtestrados.py +++ b/benchmark/cephtestrados.py @@ -1,7 +1,7 @@ from .benchmark import Benchmark import common import settings -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import os import time import logging @@ -74,7 +74,7 @@ def run(self): self.mkpool() self.dropcaches() self.cluster.dump_config(self.run_dir) - monitoring.start(self.run_dir) + MonitoringFactory.start(self.run_dir) time.sleep(5) # Run the backfill testing thread if requested if 'recovery_test' in self.cluster.config: @@ -92,7 +92,7 @@ def run(self): if 'recovery_test' in self.cluster.config: self.cluster.wait_recovery_done() - monitoring.stop(self.run_dir) + MonitoringFactory.stop(self.run_dir) # Finally, get the historic ops self.cluster.dump_historic_ops(self.run_dir) @@ -117,10 +117,10 @@ def mkcmd(self): return ' '.join(cmd) def mkpool(self): - monitoring.start("%s/pool_monitoring" % self.run_dir) + MonitoringFactory.start("%s/pool_monitoring" % self.run_dir) self.cluster.rmpool('ceph_test_rados', self.pool_profile) self.cluster.mkpool('ceph_test_rados', self.pool_profile, 'ceph_test_rados') - monitoring.stop() + MonitoringFactory.stop() def recovery_callback(self): common.pdsh(settings.getnodes('clients'), 'sudo pkill -f ceph_test_rados').communicate() diff --git a/benchmark/cosbench.py b/benchmark/cosbench.py index 46dda0db..a6c623fb 100644 --- a/benchmark/cosbench.py +++ b/benchmark/cosbench.py @@ -1,6 +1,6 @@ import common import settings -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import os import sys import time @@ -185,9 +185,9 @@ def initialize(self): self.prerun_check() logger.debug('Pausing for 60s for idle monitoring.') - monitoring.start("%s/idle_monitoring" % self.run_dir) + MonitoringFactory.start("%s/idle_monitoring" % self.run_dir) time.sleep(60) - monitoring.stop() + MonitoringFactory.stop() common.sync_files('%s' % self.run_dir, self.out_dir) @@ -248,7 +248,7 @@ def run(self): super(Cosbench, self).run() self.dropcaches() self.cluster.dump_config(self.run_dir) - monitoring.start(self.run_dir) + MonitoringFactory.start(self.run_dir) # Run cosbench test try: @@ -262,7 +262,7 @@ def run(self): self.check_workload_status() self.check_cosbench_res_dir() - monitoring.stop(self.run_dir) + MonitoringFactory.stop(self.run_dir) self.cluster.dump_historic_ops(self.run_dir) common.sync_files('%s/*' % self.run_dir, self.out_dir) diff --git a/benchmark/fio.py b/benchmark/fio.py index 9407dbb1..8ae388f3 100644 --- a/benchmark/fio.py +++ b/benchmark/fio.py @@ -1,6 +1,6 @@ import common import settings -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import os import time import logging @@ -228,7 +228,7 @@ def run(self): # Wait for signal to start client IO self.cluster.wait_start_io() - monitoring.start(self.run_dir) + MonitoringFactory.start(self.run_dir) logger.info('Running fio %s test.', self.mode) ps = [] @@ -241,7 +241,7 @@ def run(self): if 'recovery_test' in self.cluster.config: self.cluster.wait_recovery_done() - monitoring.stop(self.run_dir) + MonitoringFactory.stop(self.run_dir) # Finally, get the historic ops self.cluster.dump_historic_ops(self.run_dir) diff --git a/benchmark/getput.py b/benchmark/getput.py index f6559e6b..aed81800 100644 --- a/benchmark/getput.py +++ b/benchmark/getput.py @@ -1,6 +1,6 @@ import common import settings -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import os import time import logging @@ -58,9 +58,9 @@ def initialize(self): common.make_remote_dir(self.run_dir) logger.info('Pausing for 60s for idle monitoring.') - monitoring.start("%s/idle_monitoring" % self.run_dir) + MonitoringFactory.start("%s/idle_monitoring" % self.run_dir) time.sleep(60) - monitoring.stop() + MonitoringFactory.stop() common.sync_files('%s/*' % self.run_dir, self.out_dir) @@ -125,7 +125,7 @@ def run(self): self.cluster.create_recovery_test(self.run_dir, recovery_callback) # Run getput - monitoring.start(self.run_dir) + MonitoringFactory.start(self.run_dir) logger.info('Running getput %s test.' % self.test) ps = [] @@ -135,7 +135,7 @@ def run(self): ps.append(p) for p in ps: p.wait() - monitoring.stop(self.run_dir) + MonitoringFactory.stop(self.run_dir) # If we were doing recovery, wait until it's done. if 'recovery_test' in self.cluster.config: diff --git a/benchmark/hsbench.py b/benchmark/hsbench.py index cdc6ad50..efa9bb3b 100644 --- a/benchmark/hsbench.py +++ b/benchmark/hsbench.py @@ -1,6 +1,6 @@ import common import settings -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import os import logging import pathlib @@ -141,7 +141,7 @@ def run(self): recovery_callback = self.recovery_callback self.cluster.create_recovery_test(self.run_dir, recovery_callback) - monitoring.start(self.run_dir) + MonitoringFactory.start(self.run_dir) logger.info('Running hsbench %s test.' % self.modes) ps = [] for i in range(self.endpoints_per_client): @@ -153,7 +153,7 @@ def run(self): if 'recovery_test' in self.cluster.config: self.cluster.wait_recovery_done() - monitoring.stop(self.run_dir) + MonitoringFactory.stop(self.run_dir) # If we were doing recovery, wait until it's done. if 'recovery_test' in self.cluster.config: diff --git a/benchmark/kvmrbdfio.py b/benchmark/kvmrbdfio.py index 9c98af3b..225b3770 100644 --- a/benchmark/kvmrbdfio.py +++ b/benchmark/kvmrbdfio.py @@ -1,6 +1,6 @@ import common import settings -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import os import time import logging @@ -94,7 +94,7 @@ def run(self): # We'll always drop caches for rados bench self.dropcaches() - monitoring.start(self.run_dir) + MonitoringFactory.start(self.run_dir) time.sleep(5) # Run the backfill testing thread if requested @@ -139,7 +139,7 @@ def run(self): fio_process_list.append(common.pdsh(clnts, fio_cmd, continue_if_error=False)) for p in fio_process_list: p.communicate() - monitoring.stop(self.run_dir) + MonitoringFactory.stop(self.run_dir) logger.info('Finished rbd fio test') common.sync_files('%s/*' % self.run_dir, self.out_dir) diff --git a/benchmark/librbdfio.py b/benchmark/librbdfio.py index 6960d817..0b2b564e 100644 --- a/benchmark/librbdfio.py +++ b/benchmark/librbdfio.py @@ -10,7 +10,7 @@ from typing import Union import common -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import settings from post_processing.post_processing_types import ReportType from post_processing.report import Report, ReportOptions @@ -106,9 +106,9 @@ def initialize(self): common.clean_remote_dir(self.run_dir) common.make_remote_dir(self.run_dir) logger.info("Pausing for %ds for idle monitoring.", self.idle_monitor_sleep) - monitoring.start(f"{self.run_dir}idle_monitoring") + MonitoringFactory.start(f"{self.run_dir}idle_monitoring") time.sleep(self.idle_monitor_sleep) - monitoring.stop() + MonitoringFactory.stop() common.sync_files(f"{self.run_dir}/", self.out_dir) # Create the recovery image based on test type requested if "recovery_test" in self.cluster.config and self.recov_test_type == "background": @@ -152,7 +152,7 @@ def run(self): self._workloads.run() else: # Original style - monitoring.start(self.run_dir) + MonitoringFactory.start(self.run_dir) logger.info("Running rbd fio %s test.", self.mode) ps = [] number_of_volumes: int = len(self._iodepth_per_volume.keys()) @@ -167,7 +167,7 @@ def run(self): if "recovery_test" in self.cluster.config: self.cluster.wait_recovery_done() - monitoring.stop(self.run_dir) + MonitoringFactory.stop(self.run_dir) # Finally, get the historic ops self.cluster.dump_historic_ops(self.run_dir) @@ -260,7 +260,7 @@ def mkrecovimage(self): Create a reecovery image """ logger.info("Creating recovery image...") - monitoring.start(f"{self.run_dir}/recovery_pool_monitoring") + MonitoringFactory.start(f"{self.run_dir}/recovery_pool_monitoring") if self.use_existing_volumes is False: self.cluster.rmpool(self.recov_pool_name, self.recov_pool_profile) self.cluster.mkpool(self.recov_pool_name, self.recov_pool_profile, "rbd") @@ -274,13 +274,13 @@ def mkrecovimage(self): self.data_pool, self.vol_object_size, ) - monitoring.stop() + MonitoringFactory.stop() def mkimages(self): """ Create an RBD pool and a number of volumes per client """ - monitoring.start(f"{self.run_dir}/pool_monitoring") + MonitoringFactory.start(f"{self.run_dir}/pool_monitoring") if self.use_existing_volumes is False: self.cluster.rmpool(self.pool_name, self.pool_profile) self.cluster.mkpool(self.pool_name, self.pool_profile, "rbd") @@ -294,7 +294,7 @@ def mkimages(self): self.cluster.mkimage( f"cbt-rbdfio-{node}-{volnum:d}", self.vol_size, self.pool_name, self.data_pool, self.vol_object_size ) - monitoring.stop() + MonitoringFactory.stop() def prefill(self): """ diff --git a/benchmark/radosbench.py b/benchmark/radosbench.py index eb425077..26f1582f 100644 --- a/benchmark/radosbench.py +++ b/benchmark/radosbench.py @@ -1,6 +1,6 @@ import common import settings -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import os import time import logging @@ -60,7 +60,7 @@ def initialize(self): super(Radosbench, self).initialize() logger.info('Pausing for 60s for idle monitoring.') - with monitoring.monitor("%s/idle_monitoring" % self.run_dir): + with MonitoringFactory.monitor("%s/idle_monitoring" % self.run_dir): time.sleep(60) common.sync_files('%s/*' % self.run_dir, self.out_dir) @@ -150,7 +150,7 @@ def _run(self, mode, run_dir, out_dir, max_objects, runtime): self.cluster.create_recovery_test(run_dir, recovery_callback) # Run rados bench - with monitoring.monitor(run_dir) as monitor: + with MonitoringFactory.monitor(run_dir) as monitor: logger.info('Running radosbench %s test.' % mode) ps = [] for i in range(self.concurrent_procs): @@ -204,7 +204,7 @@ def _run(self, mode, run_dir, out_dir, max_objects, runtime): self.analyze(out_dir) def mkpools(self): - with monitoring.monitor("%s/pool_monitoring" % self.run_dir): + with MonitoringFactory.monitor("%s/pool_monitoring" % self.run_dir): if self.pool_per_proc: # allow use of a separate storage pool per process for i in range(self.concurrent_procs): for node in settings.getnodes('clients').split(','): @@ -264,7 +264,7 @@ def get_total_ops(self): return res[0] def get_cpu_cycles(self): - return monitoring.get_cpu_cycles(self.out_dir) + return MonitoringFactory.get_cpu_cycles(self.out_dir) def get_cpu_cycles_per_op(self): num_cpu_cycles = self.get_cpu_cycles() diff --git a/benchmark/rawfio.py b/benchmark/rawfio.py index fad27550..85b818cb 100644 --- a/benchmark/rawfio.py +++ b/benchmark/rawfio.py @@ -1,6 +1,6 @@ import common import settings -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import time import logging @@ -78,7 +78,7 @@ def run(self): # We'll always drop caches for rados bench self.dropcaches() - monitoring.start(self.run_dir) + MonitoringFactory.start(self.run_dir) time.sleep(5) @@ -119,7 +119,7 @@ def run(self): fio_process_list.append(common.pdsh(clnts, fio_cmd, continue_if_error=False)) for p in fio_process_list: p.communicate() - monitoring.stop(self.run_dir) + MonitoringFactory.stop(self.run_dir) logger.info('Finished raw fio test') common.sync_files('%s/*' % self.run_dir, self.out_dir) diff --git a/benchmark/rbdfio.py b/benchmark/rbdfio.py index 80cc2605..8321c641 100644 --- a/benchmark/rbdfio.py +++ b/benchmark/rbdfio.py @@ -1,6 +1,6 @@ import common import settings -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import os import time import logging @@ -59,9 +59,9 @@ def initialize(self): super(RbdFio, self).initialize() logger.info('Pausing for 60s for idle monitoring.') - monitoring.start("%s/idle_monitoring" % self.run_dir) + MonitoringFactory.start("%s/idle_monitoring" % self.run_dir) time.sleep(60) - monitoring.stop() + MonitoringFactory.stop() common.sync_files('%s/*' % self.run_dir, self.out_dir) @@ -85,7 +85,7 @@ def run(self): # We'll always drop caches for rados bench self.dropcaches() - monitoring.start(self.run_dir) + MonitoringFactory.start(self.run_dir) # Run the backfill testing thread if requested if 'recovery_test' in self.cluster.config: @@ -129,7 +129,7 @@ def run(self): if 'recovery_test' in self.cluster.config: self.cluster.wait_recovery_done() - monitoring.stop(self.run_dir) + MonitoringFactory.stop(self.run_dir) # Finally, get the historic ops self.cluster.dump_historic_ops(self.run_dir) @@ -145,7 +145,7 @@ def __str__(self): return "%s\n%s\n%s" % (self.run_dir, self.out_dir, super(RbdFio, self).__str__()) def mkimages(self): - monitoring.start("%s/pool_monitoring" % self.run_dir) + MonitoringFactory.start("%s/pool_monitoring" % self.run_dir) self.cluster.rmpool(self.poolname, self.pool_profile) self.cluster.mkpool(self.poolname, self.pool_profile, 'rbd') common.pdsh(settings.getnodes('clients'), '/usr/bin/rbd create cbt-kernelrbdfio-`hostname -s` --size %s --pool %s' % (self.vol_size, self.poolname)).communicate() @@ -153,7 +153,7 @@ def mkimages(self): common.pdsh(settings.getnodes('clients'), 'sudo mkfs.xfs /dev/rbd/cbt-kernelrbdfio/cbt-kernelrbdfio-`hostname -s`').communicate() common.pdsh(settings.getnodes('clients'), 'sudo mkdir -p -m0755 -- %s/cbt-kernelrbdfio-`hostname -s`' % self.cluster.mnt_dir).communicate() common.pdsh(settings.getnodes('clients'), 'sudo mount -t xfs -o noatime,inode64 /dev/rbd/cbt-kernelrbdfio/cbt-kernelrbdfio-`hostname -s` %s/cbt-kernelrbdfio-`hostname -s`' % self.cluster.mnt_dir).communicate() - monitoring.stop() + MonitoringFactory.stop() def recovery_callback(self): common.pdsh(settings.getnodes('clients'), 'sudo killall -9 fio').communicate() diff --git a/cluster/ceph.py b/cluster/ceph.py index 0003d1b0..d631c6f0 100644 --- a/cluster/ceph.py +++ b/cluster/ceph.py @@ -1,6 +1,6 @@ import common import settings -import monitoring +from monitoring.monitoring_factory import MonitoringFactory import os import time import uuid @@ -199,16 +199,16 @@ def initialize(self): self.setup_fs() # Build the cluster - monitoring.start('%s/creation' % self.monitoring_dir) + MonitoringFactory.start('%s/creation' % self.monitoring_dir) self.make_mons() self.start_mgrs() self.make_osds() - monitoring.stop() + MonitoringFactory.stop() # Check Health - monitoring.start('%s/initial_health_check' % self.monitoring_dir) + MonitoringFactory.start('%s/initial_health_check' % self.monitoring_dir) self.check_health() - monitoring.stop() + MonitoringFactory.stop() # Disable scrub and wait for any scrubbing to complete self.disable_scrub() @@ -227,9 +227,9 @@ def initialize(self): self.start_mds() # Peform Idle Monitoring if self.idle_duration > 0: - monitoring.start("%s/idle_monitoring" % self.monitoring_dir) + MonitoringFactory.start("%s/idle_monitoring" % self.monitoring_dir) time.sleep(self.idle_duration) - monitoring.stop() + MonitoringFactory.stop() return True @@ -248,7 +248,7 @@ def shutdown(self): common.pdsh(nodes, 'sudo killall -9 radosgw-admin').communicate() common.pdsh(nodes, 'sudo /etc/init.d/apache2 stop').communicate() common.pdsh(nodes, 'sudo killall -9 pdsh').communicate() - monitoring.stop() + MonitoringFactory.stop() def cleanup(self): nodes = settings.getnodes('clients', 'osds', 'mons', 'rgws', 'mdss', 'mgrs') diff --git a/monitoring.py b/monitoring.py deleted file mode 100644 index 025033e7..00000000 --- a/monitoring.py +++ /dev/null @@ -1,231 +0,0 @@ -from contextlib import contextmanager -import glob -import os.path -import common -import settings -import logging - -logger = logging.getLogger("cbt") - -class Monitoring(object): - def __init__(self, mconfig): - # the initializers should be the very single places interrogating - # settings for the sake of explicitness - nodes_list = mconfig.get('nodes', self._get_default_nodes()) - self.nodes = settings.getnodes(*nodes_list) - - @staticmethod - def _get_all(): - for monitoring, mconfig in sorted(settings.monitoring_profiles.items()): - yield Monitoring._get_object(monitoring, mconfig) - - @staticmethod - def _get_object(monitoring, mconfig): - if monitoring == 'collectl': - return CollectlMonitoring(mconfig) - if monitoring == 'perf': - return PerfMonitoring(mconfig) - if monitoring == 'blktrace': - return BlktraceMonitoring(mconfig) - if monitoring == 'top': - return TopMonitoring(mconfig) - - -class CollectlMonitoring(Monitoring): - def __init__(self, mconfig): - super(CollectlMonitoring, self).__init__(mconfig) - - self.args = mconfig.get('args', '-s+mYZ -i 1:10 -F0 -f {collectl_dir} ' - r'--rawdskfilt \"+cciss/c\d+d\d+ |hd[ab] | sd[a-z]+ |dm-\d+ |xvd[a-z] |fio[a-z]+ | vd[a-z]+ |emcpower[a-z]+ |psv\d+ |nvme[0-9]n[0-9]+p[0-9]+ \"') - - def start(self, directory): - collectl_dir = '%s/collectl' % directory - common.pdsh(self.nodes, 'mkdir -p -m0755 -- %s' % collectl_dir).communicate() - common.pdsh(self.nodes, ['collectl', self.args.format(collectl_dir=collectl_dir)]) - - def stop(self, directory): - common.pdsh(self.nodes, 'pkill -SIGINT -f collectl').communicate() - - @staticmethod - def _get_default_nodes(): - return ['clients', 'osds', 'mons', 'rgws'] - - -class PerfMonitoring(Monitoring): - def __init__(self, mconfig): - super(PerfMonitoring, self).__init__(mconfig) - self.pid_dir = settings.cluster.get('pid_dir') - self.pid_glob = mconfig.get('pid_glob', 'osd.*.pid') - self.user = settings.cluster.get('user') - self.perf_cmd = mconfig.get('perf_cmd', 'sudo perf') - self.args_template = mconfig.get('args') - self.perf_runners = [] - self.perf_dir = '' # we need the output file to extract data - - def start(self, directory): - perf_dir = '%s/perf' % directory - self.perf_dir = perf_dir - common.pdsh(self.nodes, 'mkdir -p -m0755 -- %s' % perf_dir).communicate() - - perf_template = '{} {} &'.format(self.perf_cmd, self.args_template) - local_node = common.get_localnode(self.nodes) - if local_node: - logger.debug("PerfMonitoring: in local_node"); - logger.debug("pid_dir: %s" % self.pid_dir); - for pid_path in glob.glob(os.path.join(self.pid_dir, self.pid_glob)): - logger.debug("PerfMonitoring pid_path: %s" % pid_path); - with open(pid_path) as pidfile: - pid = pidfile.read().strip() - perf_cmd = perf_template.format(perf_dir=perf_dir, pid=pid) - runner = common.sh(local_node, perf_cmd) - self.perf_runners.append(runner) - else: - logger.debug("PerfMonitoring: remote_node"); - # ${pid} will be handled by remote's sh - perf_cmd = perf_template.format(perf_dir=perf_dir, pid='${pid}') - common.pdsh(self.nodes, ['for pid in `cat %s/%s`;' % (self.pid_dir, self.pid_glob), - 'do', perf_cmd, - 'done']) - - def stop(self, directory): - if self.perf_runners: - for runner in self.perf_runners: - runner.kill() - else: - common.pdsh(self.nodes, 'sudo pkill -SIGINT -f perf\ ').communicate() - if directory: - common.pdsh(self.nodes, 'sudo chown {user}.{user} {dir}/perf/perf.data'.format( - user=self.user, dir=directory)) - common.pdsh(self.nodes, 'sudo chown {user}.{user} {dir}/perf/perf_stat.*'.format( - user=self.user, dir=directory)) - - def get_cpu_cycles(self, out_dir): - import re - total_cpu_cycles = 0 - perf_dir_name = str(glob.glob(out_dir + "/perf*")[0]) - perf_stat_fnames = os.listdir(perf_dir_name) - for perf_out_fname in perf_stat_fnames: - perf_output_file = open(perf_dir_name + "/" + perf_out_fname, "rt") - match = re.search(r'(.*) cycles(.*?) .*', perf_output_file.read(), re.M | re.I) - if match: - cpu_cycles = match.group(1).strip() - else: - return None - total_cpu_cycles = total_cpu_cycles + int(cpu_cycles.replace(',', '')) - return total_cpu_cycles - - @staticmethod - def _get_default_nodes(): - return ['osds'] - - -class BlktraceMonitoring(Monitoring): - def __init__(self, mconfig): - super(BlktraceMonitoring, self).__init__(mconfig) - self.osds_per_node = settings.cluster.get('osds_per_node') - self.use_existing = settings.cluster.get('use_existing', True) - self.user = settings.cluster.get('user') - - def start(self, directory): - blktrace_dir = '%s/blktrace' % directory - common.pdsh(self.nodes, 'mkdir -p -m0755 -- %s' % blktrace_dir).communicate() - for device in range(0, self.osds_per_node): - common.pdsh(self.nodes, 'cd %s;sudo blktrace -o device%s -d /dev/disk/by-partlabel/osd-device-%s-data' - % (blktrace_dir, device, device)) - - def stop(self, directory): - common.pdsh(self.nodes, 'sudo pkill -SIGINT -f blktrace').communicate() - if directory and not self.use_existing: - self._make_movies(directory) - - def _make_movies(self, directory): - seekwatcher = '/home/%s/bin/seekwatcher' % self.user - blktrace_dir = '%s/blktrace' % directory - - for device in range(self.osds_per_node): - common.pdsh(self.nodes, 'cd %s;%s -t device%s -o device%s.mpg --movie' % - (blktrace_dir, seekwatcher, device, device)).communicate() - - @staticmethod - def _get_default_nodes(): - return ['osds'] - - -class TopMonitoring(Monitoring): - def __init__(self, mconfig): - super(TopMonitoring, self).__init__(mconfig) - self.pid_dir = settings.cluster.get('pid_dir') - self.pid_glob = mconfig.get('pid_glob', 'osd.*.pid') - self.user = settings.cluster.get('user') - self.top_cmd = mconfig.get('top_cmd', 'top') - self.args = mconfig.get('args', '-b -H -1 -p {pid} -n 30 > {top_dir}/{pid}_osd_top.out') - self.top_runners = [] - - def start(self, directory): - top_dir = '%s/top' % directory - common.pdsh(self.nodes, 'mkdir -p -m0755 -- %s' % top_dir).communicate() - - top_template = '{} {}'.format(self.top_cmd, self.args) - local_node = common.get_localnode(self.nodes) - if local_node: - logger.debug("TopMonitoring: in local_node"); - logger.debug("pid_dir: %s" % self.pid_dir); - for pid_path in glob.glob(os.path.join(self.pid_dir, self.pid_glob)): - logger.debug("TopMonitoring pid_path: %s" % pid_path); - with open(pid_path) as pidfile: - pid = pidfile.read().strip() - top_cmd = top_template.format(top_dir=top_dir, pid=pid) - runner = common.sh(local_node, top_cmd) - self.top_runners.append(runner) - else: - logger.debug("TopMonitoring: remote_node"); - # ${pid} will be handled by remote's sh - top_cmd = top_template.format(top_dir=top_dir, pid='${pid}') - common.pdsh(self.nodes, ['for pid in `cat %s/%s`;' % (self.pid_dir, self.pid_glob), - 'do', top_cmd, - 'done']) - - def stop(self, directory): - #common.pdsh(self.nodes, 'pkill -SIGINT -f top').communicate() - if self.top_runners: - for runner in self.top_runners: - runner.kill() - else: - #ToDO: find the pid of the correct top instance process - common.pdsh(self.nodes, 'sudo pkill -SIGINT -f top\ ').communicate() - if directory: - common.pdsh(self.nodes, 'sudo chown {user}.{user} {dir}/top/*top.out'.format( - user=self.user, dir=directory)) - - @staticmethod - def _get_default_nodes(): - return ['osds'] - - -def start(directory): - for m in Monitoring._get_all(): - m.start(directory) - - -def stop(directory=None): - for m in Monitoring._get_all(): - m.stop(directory) - - -@contextmanager -def monitor(directory): - monitors = [] - for m in Monitoring._get_all(): - m.start(directory) - monitors.append(m) - yield - for m in monitors: - m.stop(directory) - - -def get_cpu_cycles(out_dir): - # check if perf stat is configured - for monitoring_profile in Monitoring._get_all(): - if(isinstance(monitoring_profile, PerfMonitoring)): - return monitoring_profile.get_cpu_cycles(out_dir) # if it is, then return the number of cycle - return None diff --git a/monitoring/__init__.py b/monitoring/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/monitoring/base.py b/monitoring/base.py new file mode 100644 index 00000000..33e5814e --- /dev/null +++ b/monitoring/base.py @@ -0,0 +1,26 @@ +"""Base abstractions for monitoring backends.""" + +from abc import ABC, abstractmethod +from typing import Any, ClassVar, Optional, cast + +import settings + + +class Monitoring(ABC): + """Abstract base class for monitoring backends.""" + + DEFAULT_NODES: ClassVar[list[str]] + + def __init__(self, mconfig: dict[str, Any]) -> None: + """Resolve monitoring nodes from configuration or subclass defaults.""" + nodes_list = mconfig.get("nodes", self.DEFAULT_NODES) + # Remove this cast and ignore once settings.getnodes() is fully typed. + self._nodes = cast(str, settings.getnodes(*nodes_list)) # type: ignore[no-untyped-call] + + @abstractmethod + def start(self, directory: str) -> None: + """Start monitoring and write output beneath the given directory.""" + + @abstractmethod + def stop(self, directory: Optional[str]) -> None: + """Stop monitoring and finalize any output files.""" diff --git a/monitoring/blktrace_monitoring.py b/monitoring/blktrace_monitoring.py new file mode 100644 index 00000000..49df00cc --- /dev/null +++ b/monitoring/blktrace_monitoring.py @@ -0,0 +1,50 @@ +"""Blktrace monitoring backend.""" + +import logging +from typing import Any, ClassVar, Optional, cast + +import common +import settings +from monitoring.base import Monitoring + +logger = logging.getLogger("cbt") + + +class BlktraceMonitoring(Monitoring): + """Monitoring backend that captures blktrace output and optionally renders seekwatcher movies.""" + + DEFAULT_NODES: ClassVar[list[str]] = ["osds"] + + def __init__(self, mconfig: dict[str, Any]) -> None: + """Initialize blktrace monitoring configuration.""" + super().__init__(mconfig) + # Remove these casts once settings.cluster.get() is fully typed. + self._osds_per_node = cast(int, settings.cluster.get("osds_per_node")) + self._use_existing = cast(bool, settings.cluster.get("use_existing", True)) + self._user = cast(str, settings.cluster.get("user")) + + def start(self, directory: str) -> None: + """Create the blktrace output directory and start tracing on each OSD device.""" + blktrace_dir = f"{directory}/blktrace" + common.pdsh(self._nodes, f"mkdir -p -m0755 -- {blktrace_dir}").communicate() # type: ignore[no-untyped-call] + for device in range(self._osds_per_node): + common.pdsh( # type: ignore[no-untyped-call] + self._nodes, + f"cd {blktrace_dir};sudo blktrace -o device{device} -d /dev/disk/by-partlabel/osd-device-{device}-data", + ) + + def stop(self, directory: Optional[str]) -> None: + """Stop blktrace and optionally generate seekwatcher movies.""" + common.pdsh(self._nodes, "sudo pkill -SIGINT -f blktrace").communicate() # type: ignore[no-untyped-call] + if directory and not self._use_existing: + self._make_movies(directory) + + def _make_movies(self, directory: str) -> None: + """Generate an mpg movie for each OSD device using seekwatcher.""" + seekwatcher = f"/home/{self._user}/bin/seekwatcher" + blktrace_dir = f"{directory}/blktrace" + for device in range(self._osds_per_node): + common.pdsh( # type: ignore[no-untyped-call] + self._nodes, + f"cd {blktrace_dir};{seekwatcher} -t device{device} -o device{device}.mpg --movie", + ).communicate() diff --git a/monitoring/collectl_monitoring.py b/monitoring/collectl_monitoring.py new file mode 100644 index 00000000..32a52ce4 --- /dev/null +++ b/monitoring/collectl_monitoring.py @@ -0,0 +1,36 @@ +"""Collectl monitoring backend.""" + +from typing import Any, ClassVar, Optional + +import common +from monitoring.base import Monitoring + + +class CollectlMonitoring(Monitoring): + """Monitoring backend that captures collectl output.""" + + DEFAULT_NODES: ClassVar[list[str]] = ["clients", "osds", "mons", "rgws"] + DEFAULT_ARGS: ClassVar[str] = ( + "-s+mYZ -i 1:10 -F0 -f {collectl_dir} " + r"--rawdskfilt \"+cciss/c\d+d\d+ |hd[ab] | sd[a-z]+ |dm-\d+ |" + r"xvd[a-z]+ |fio[a-z]+ | vd[a-z]+ |emcpower[a-z]+ |psv\d+ |" + r"nvme[0-9]n[0-9]+p[0-9]+ \"" + ) + + def __init__(self, mconfig: dict[str, Any]) -> None: + """Initialize collectl monitoring configuration.""" + super().__init__(mconfig) + self._args = mconfig.get("args", self.DEFAULT_ARGS) + + def start(self, directory: str) -> None: + """Create the output directory and start collectl.""" + collectl_dir = f"{directory}/collectl" + common.pdsh(self._nodes, f"mkdir -p -m0755 -- {collectl_dir}").communicate() # type: ignore[no-untyped-call] + common.pdsh( + self._nodes, ["collectl", self._args.format(collectl_dir=collectl_dir)] + ) # type: ignore[no-untyped-call] + + def stop(self, directory: Optional[str]) -> None: + """Stop running collectl processes.""" + del directory + common.pdsh(self._nodes, "pkill -SIGINT -f collectl").communicate() # type: ignore[no-untyped-call] diff --git a/monitoring/monitoring_factory.py b/monitoring/monitoring_factory.py new file mode 100644 index 00000000..4adc2b78 --- /dev/null +++ b/monitoring/monitoring_factory.py @@ -0,0 +1,79 @@ +"""Factory class for creating and managing monitoring backends.""" + +from collections.abc import Iterator +from contextlib import contextmanager +from typing import Any, ClassVar, Optional + +import settings +from monitoring.base import Monitoring +from monitoring.blktrace_monitoring import BlktraceMonitoring +from monitoring.collectl_monitoring import CollectlMonitoring +from monitoring.perf_monitoring import PerfMonitoring +from monitoring.top_monitoring import TopMonitoring + + +class MonitoringFactory: + """Instantiates monitoring backends and owns the lifecycle API.""" + + _REGISTRY: ClassVar[dict[str, type[Monitoring]]] = { + "collectl": CollectlMonitoring, + "perf": PerfMonitoring, + "blktrace": BlktraceMonitoring, + "top": TopMonitoring, + } + + @classmethod + def get_object(cls, name: str, mconfig: dict[str, Any]) -> Monitoring: + """Return a new monitoring instance for the given profile name. + + Raises: + ValueError: If *name* is not a known monitoring backend key. + """ + try: + return cls._REGISTRY[name](mconfig) + except KeyError as exc: + raise ValueError(f"Unknown monitoring backend: {name!r}") from exc + + @classmethod + def get_all(cls) -> Iterator[Monitoring]: + """Yield one instance for every entry in ``settings.monitoring_profiles``.""" + for name, mconfig in sorted(settings.monitoring_profiles.items()): + yield cls.get_object(name, mconfig) + + @classmethod + def start(cls, directory: str) -> None: + """Start all configured monitoring backends.""" + for monitor in cls.get_all(): + monitor.start(directory) + + @classmethod + def stop(cls, directory: Optional[str] = None) -> None: + """Stop all configured monitoring backends.""" + for monitor in cls.get_all(): + monitor.stop(directory) + + @classmethod + @contextmanager + def monitor(cls, directory: str) -> Iterator[None]: + """Context manager: start all monitors, yield, then stop all.""" + monitors = list(cls.get_all()) + for monitor in monitors: + monitor.start(directory) + try: + yield + finally: + for monitor in monitors: + monitor.stop(directory) + + @classmethod + def get_cpu_cycles(cls, out_dir: str) -> Optional[int]: + """Return total CPU cycles from perf stat output, if perf is configured. + + Iterates monitoring profiles and delegates to the first + ``PerfMonitoring`` instance found. Returns ``None`` when no perf + profile is configured. + """ + for monitor in cls.get_all(): + if isinstance(monitor, PerfMonitoring): + return monitor.get_cpu_cycles(out_dir) + return None diff --git a/monitoring/perf_monitoring.py b/monitoring/perf_monitoring.py new file mode 100644 index 00000000..8f488544 --- /dev/null +++ b/monitoring/perf_monitoring.py @@ -0,0 +1,87 @@ +"""Perf monitoring backend.""" + +import glob +import logging +import os +import re +from typing import Any, ClassVar, Optional, cast + +import common +import settings +from monitoring.base import Monitoring + +logger = logging.getLogger("cbt") + + +class PerfMonitoring(Monitoring): + """Monitoring backend that captures perf output.""" + + DEFAULT_NODES: ClassVar[list[str]] = ["osds"] + + def __init__(self, mconfig: dict[str, Any]) -> None: + """Initialize perf monitoring configuration.""" + super().__init__(mconfig) + # Remove these casts once settings.cluster.get() is fully typed. + self._pid_dir = cast(str, settings.cluster.get("pid_dir")) + self._pid_glob = mconfig.get("pid_glob", "osd.*.pid") + self._user = cast(str, settings.cluster.get("user")) + self._perf_cmd = mconfig.get("perf_cmd", "sudo perf") + self._args_template = mconfig.get("args") + self._perf_runners: list[Any] = [] + self._perf_dir = "" + + def start(self, directory: str) -> None: + """Create the perf output directory and start perf collection.""" + perf_dir = f"{directory}/perf" + self._perf_dir = perf_dir + common.pdsh(self._nodes, f"mkdir -p -m0755 -- {perf_dir}").communicate() # type: ignore[no-untyped-call] + + perf_template = f"{self._perf_cmd} {self._args_template} &" + local_node = common.get_localnode(self._nodes) # type: ignore[no-untyped-call] + if local_node: + logger.debug("PerfMonitoring: in local_node") + logger.debug("pid_dir: %s", self._pid_dir) + for pid_path in glob.glob(os.path.join(self._pid_dir, self._pid_glob)): + logger.debug("PerfMonitoring pid_path: %s", pid_path) + with open(pid_path, encoding="utf-8") as pidfile: + pid = pidfile.read().strip() + perf_cmd = perf_template.format(perf_dir=perf_dir, pid=pid) + runner = common.sh(local_node, perf_cmd) # type: ignore[no-untyped-call] + self._perf_runners.append(runner) + else: + logger.debug("PerfMonitoring: remote_node") + perf_cmd = perf_template.format(perf_dir=perf_dir, pid="${pid}") + common.pdsh( # type: ignore[no-untyped-call] + self._nodes, + [f"for pid in `cat {self._pid_dir}/{self._pid_glob}`;", "do", perf_cmd, ";", "done"], + ) + + def stop(self, directory: Optional[str]) -> None: + """Stop perf collection and adjust file ownership when needed.""" + if self._perf_runners: + for runner in self._perf_runners: + runner.kill() + else: + common.pdsh(self._nodes, r"sudo pkill -SIGINT -f perf\ ").communicate() # type: ignore[no-untyped-call] + if directory: + common.pdsh( # type: ignore[no-untyped-call] + self._nodes, f"sudo chown {self._user}.{self._user} {directory}/perf/perf.data" + ) + common.pdsh( # type: ignore[no-untyped-call] + self._nodes, f"sudo chown {self._user}.{self._user} {directory}/perf/perf_stat.*" + ) + + def get_cpu_cycles(self, out_dir: str) -> Optional[int]: + """Return total CPU cycles from perf stat output, if available.""" + total_cpu_cycles = 0 + perf_dir_name = str(glob.glob(out_dir + "/perf*")[0]) + perf_stat_fnames = os.listdir(perf_dir_name) + for perf_out_fname in perf_stat_fnames: + with open(f"{perf_dir_name}/{perf_out_fname}", encoding="utf-8") as perf_output_file: + match = re.search(r"(.*) cycles(.*?) .*", perf_output_file.read(), re.M | re.I) + if match: + cpu_cycles = match.group(1).strip() + else: + return None + total_cpu_cycles = total_cpu_cycles + int(cpu_cycles.replace(",", "")) + return cast(Optional[int], total_cpu_cycles) diff --git a/monitoring/top_monitoring.py b/monitoring/top_monitoring.py new file mode 100644 index 00000000..783bdf2b --- /dev/null +++ b/monitoring/top_monitoring.py @@ -0,0 +1,67 @@ +"""Top monitoring backend.""" + +import glob +import logging +import os +from typing import Any, ClassVar, Optional, cast + +import common +import settings +from monitoring.base import Monitoring + +logger = logging.getLogger("cbt") + + +class TopMonitoring(Monitoring): + """Monitoring backend that captures top output for OSD processes.""" + + DEFAULT_NODES: ClassVar[list[str]] = ["osds"] + + def __init__(self, mconfig: dict[str, Any]) -> None: + """Initialize top monitoring configuration.""" + super().__init__(mconfig) + # Remove these casts once settings.cluster.get() is fully typed. + self._pid_dir = cast(str, settings.cluster.get("pid_dir")) + self._pid_glob = mconfig.get("pid_glob", "osd.*.pid") + self._user = cast(str, settings.cluster.get("user")) + self._top_cmd = mconfig.get("top_cmd", "top") + self._args = mconfig.get("args", "-b -H -1 -p {pid} -n 30 > {top_dir}/{pid}_osd_top.out") + self._top_runners: list[Any] = [] + + def start(self, directory: str) -> None: + """Create the top output directory and start top collection.""" + top_dir = f"{directory}/top" + common.pdsh(self._nodes, f"mkdir -p -m0755 -- {top_dir}").communicate() # type: ignore[no-untyped-call] + + top_template = f"{self._top_cmd} {self._args}" + local_node = common.get_localnode(self._nodes) # type: ignore[no-untyped-call] + if local_node: + logger.debug("TopMonitoring: in local_node") + logger.debug("pid_dir: %s", self._pid_dir) + for pid_path in glob.glob(os.path.join(self._pid_dir, self._pid_glob)): + logger.debug("TopMonitoring pid_path: %s", pid_path) + with open(pid_path, encoding="utf-8") as pidfile: + pid = pidfile.read().strip() + top_cmd = top_template.format(top_dir=top_dir, pid=pid) + runner = common.sh(local_node, top_cmd) # type: ignore[no-untyped-call] + self._top_runners.append(runner) + else: + logger.debug("TopMonitoring: remote_node") + top_cmd = top_template.format(top_dir=top_dir, pid="${pid}") + common.pdsh( # type: ignore[no-untyped-call] + self._nodes, + [f"for pid in `cat {self._pid_dir}/{self._pid_glob}`;", "do", top_cmd, ";", "done"], + ) + + def stop(self, directory: Optional[str]) -> None: + """Stop top collection and adjust file ownership when needed.""" + if self._top_runners: + for runner in self._top_runners: + runner.kill() + else: + common.pdsh(self._nodes, r"sudo pkill -SIGINT -f top\ ").communicate() # type: ignore[no-untyped-call] + if directory: + common.pdsh( # type: ignore[no-untyped-call] + self._nodes, + f"sudo chown {self._user}.{self._user} {directory}/top/*top.out", + ) diff --git a/tests/test_monitoring_base.py b/tests/test_monitoring_base.py new file mode 100644 index 00000000..269f8e95 --- /dev/null +++ b/tests/test_monitoring_base.py @@ -0,0 +1,73 @@ +"""Tests for the monitoring base class.""" + +# pylint: disable=protected-access + +from typing import ClassVar, Optional +from unittest.mock import patch + +import pytest + +from monitoring.base import Monitoring + + +class MonitoringSubclass(Monitoring): + """Concrete monitoring subclass used for base class tests.""" + + DEFAULT_NODES: ClassVar[list[str]] = ["osds"] + + def start(self, directory: str) -> None: + pass + + def stop(self, directory: Optional[str]) -> None: + pass + + +class MissingDefaultNodesMonitoring(Monitoring): + """Concrete monitoring subclass without default nodes for error tests.""" + + def start(self, directory: str) -> None: + pass + + def stop(self, directory: Optional[str]) -> None: + pass + + +def test_init_uses_explicit_nodes() -> None: + """Use configured nodes when they are provided in monitoring config.""" + with patch("monitoring.base.settings") as mock_settings: + mock_settings.getnodes.return_value = "node1,node2" + + monitor = MonitoringSubclass({"nodes": ["clients", "mons"]}) + + mock_settings.getnodes.assert_called_once_with("clients", "mons") + assert monitor._nodes == "node1,node2" + + +def test_init_falls_back_to_default_nodes() -> None: + """Use subclass default nodes when config does not provide nodes.""" + with patch("monitoring.base.settings") as mock_settings: + mock_settings.getnodes.return_value = "osd-node" + + monitor = MonitoringSubclass({}) + + mock_settings.getnodes.assert_called_once_with("osds") + assert monitor._nodes == "osd-node" + + +def test_init_calls_settings_getnodes_with_resolved_nodes() -> None: + """Pass the resolved node groups to settings.getnodes.""" + with patch("monitoring.base.settings") as mock_settings: + mock_settings.getnodes.return_value = "resolved-nodes" + + MonitoringSubclass({"nodes": ["rgws"]}) + + mock_settings.getnodes.assert_called_once_with("rgws") + + +def test_missing_default_nodes_raises_attribute_error() -> None: + """Raise an attribute error when a subclass omits default nodes.""" + with patch("monitoring.base.settings") as mock_settings: + mock_settings.getnodes.return_value = "unused" + + with pytest.raises(AttributeError): + MissingDefaultNodesMonitoring({}) diff --git a/tests/test_monitoring_blktrace.py b/tests/test_monitoring_blktrace.py new file mode 100644 index 00000000..7098a1f0 --- /dev/null +++ b/tests/test_monitoring_blktrace.py @@ -0,0 +1,160 @@ +"""Tests for the blktrace monitoring backend.""" + +# pylint: disable=protected-access + +from typing import Any, Optional +from unittest.mock import MagicMock, call, patch + +from monitoring.blktrace_monitoring import BlktraceMonitoring + + +def _make_monitor( + osds_per_node: int = 2, + use_existing: bool = True, + user: str = "ceph", + mconfig: Optional[dict[str, Any]] = None, +) -> BlktraceMonitoring: + """Construct a BlktraceMonitoring instance with mocked settings.""" + if mconfig is None: + mconfig = {} + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.blktrace_monitoring.settings") as mock_settings, + patch("monitoring.blktrace_monitoring.common.pdsh"), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "osds_per_node": osds_per_node, + "use_existing": use_existing, + "user": user, + }.get(key, default) + return BlktraceMonitoring(mconfig) + + +def test_default_nodes_is_osds() -> None: + """DEFAULT_NODES must be ['osds'].""" + assert BlktraceMonitoring.DEFAULT_NODES == ["osds"] + + +def test_init_stores_cluster_settings() -> None: + """__init__ reads osds_per_node, use_existing and user from settings.cluster.""" + monitor = _make_monitor(osds_per_node=4, use_existing=False, user="admin") + assert monitor._osds_per_node == 4 + assert monitor._use_existing is False + assert monitor._user == "admin" + + +def test_start_creates_directory_and_starts_traces() -> None: + """start() calls pdsh for mkdir and once per device.""" + mkdir_runner = MagicMock() + trace_runner = MagicMock() + + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.blktrace_monitoring.settings") as mock_settings, + patch("monitoring.blktrace_monitoring.common.pdsh") as mock_pdsh, + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "osds_per_node": 2, + "use_existing": True, + "user": "ceph", + }.get(key, default) + mock_pdsh.side_effect = [mkdir_runner, trace_runner, trace_runner] + + monitor = BlktraceMonitoring({}) + monitor.start("/tmp/output") + + mock_pdsh.assert_any_call("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/blktrace") + mkdir_runner.communicate.assert_called_once_with() + + mock_pdsh.assert_any_call( + "resolved-nodes", + "cd /tmp/output/blktrace;sudo blktrace -o device0 -d /dev/disk/by-partlabel/osd-device-0-data", + ) + mock_pdsh.assert_any_call( + "resolved-nodes", + "cd /tmp/output/blktrace;sudo blktrace -o device1 -d /dev/disk/by-partlabel/osd-device-1-data", + ) + assert mock_pdsh.call_count == 3 # mkdir + 2 devices + + +def test_stop_issues_pkill() -> None: + """stop() always calls pdsh pkill blktrace.""" + pkill_runner = MagicMock() + monitor = _make_monitor() + + with ( + patch("monitoring.blktrace_monitoring.common.pdsh", return_value=pkill_runner) as mock_pdsh, + patch.object(monitor, "_make_movies") as mock_movies, + ): + monitor.stop(None) + + mock_pdsh.assert_called_once_with("resolved-nodes", "sudo pkill -SIGINT -f blktrace") + pkill_runner.communicate.assert_called_once_with() + mock_movies.assert_not_called() + + +def test_stop_calls_make_movies_when_not_use_existing() -> None: + """stop() calls _make_movies when use_existing is False and directory is provided.""" + pkill_runner = MagicMock() + monitor = _make_monitor(use_existing=False) + + with ( + patch("monitoring.blktrace_monitoring.common.pdsh", return_value=pkill_runner), + patch.object(monitor, "_make_movies") as mock_movies, + ): + monitor.stop("/tmp/output") + + mock_movies.assert_called_once_with("/tmp/output") + + +def test_stop_does_not_call_make_movies_when_use_existing() -> None: + """stop() skips _make_movies when use_existing is True.""" + pkill_runner = MagicMock() + monitor = _make_monitor(use_existing=True) + + with ( + patch("monitoring.blktrace_monitoring.common.pdsh", return_value=pkill_runner), + patch.object(monitor, "_make_movies") as mock_movies, + ): + monitor.stop("/tmp/output") + + mock_movies.assert_not_called() + + +def test_stop_does_not_call_make_movies_when_directory_is_none() -> None: + """stop() skips _make_movies when directory is None even if use_existing is False.""" + pkill_runner = MagicMock() + monitor = _make_monitor(use_existing=False) + + with ( + patch("monitoring.blktrace_monitoring.common.pdsh", return_value=pkill_runner), + patch.object(monitor, "_make_movies") as mock_movies, + ): + monitor.stop(None) + + mock_movies.assert_not_called() + + +def test_make_movies_issues_seekwatcher_per_device() -> None: + """_make_movies() calls pdsh with seekwatcher command for each device.""" + movie_runner = MagicMock() + monitor = _make_monitor(osds_per_node=2, user="ceph") + + with patch("monitoring.blktrace_monitoring.common.pdsh", return_value=movie_runner) as mock_pdsh: + monitor._make_movies("/tmp/output") + + expected_calls = [ + call( + "resolved-nodes", + "cd /tmp/output/blktrace;/home/ceph/bin/seekwatcher -t device0 -o device0.mpg --movie", + ), + call( + "resolved-nodes", + "cd /tmp/output/blktrace;/home/ceph/bin/seekwatcher -t device1 -o device1.mpg --movie", + ), + ] + mock_pdsh.assert_has_calls(expected_calls) + assert mock_pdsh.call_count == 2 + assert movie_runner.communicate.call_count == 2 diff --git a/tests/test_monitoring_collectl.py b/tests/test_monitoring_collectl.py new file mode 100644 index 00000000..24508051 --- /dev/null +++ b/tests/test_monitoring_collectl.py @@ -0,0 +1,69 @@ +"""Tests for the collectl monitoring backend.""" + +# pylint: disable=protected-access + +from unittest.mock import MagicMock, patch + +from monitoring.collectl_monitoring import CollectlMonitoring + + +def test_init_sets_default_args() -> None: + """Use the default collectl argument string when args are not configured.""" + with patch("monitoring.base.settings") as mock_settings: + mock_settings.getnodes.return_value = "resolved-nodes" + + monitor = CollectlMonitoring({}) + + assert monitor._args == CollectlMonitoring.DEFAULT_ARGS + + +def test_init_uses_custom_args() -> None: + """Use custom collectl args from monitoring config when provided.""" + with patch("monitoring.base.settings") as mock_settings: + mock_settings.getnodes.return_value = "resolved-nodes" + + monitor = CollectlMonitoring({"args": "--custom {collectl_dir}"}) + + assert monitor._args == "--custom {collectl_dir}" + + +def test_start_creates_directory_and_starts_collectl() -> None: + """Create the collectl directory and invoke collectl through pdsh.""" + mkdir_runner = MagicMock() + with ( + patch("monitoring.base.settings") as mock_settings, + patch("monitoring.collectl_monitoring.common.pdsh") as mock_pdsh, + ): + mock_settings.getnodes.return_value = "resolved-nodes" + mock_pdsh.side_effect = [mkdir_runner, MagicMock()] + monitor = CollectlMonitoring({}) + + monitor.start("/tmp/output") + + mock_pdsh.assert_any_call("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/collectl") + mkdir_runner.communicate.assert_called_once_with() + mock_pdsh.assert_any_call( + "resolved-nodes", + ["collectl", monitor._args.format(collectl_dir="/tmp/output/collectl")], + ) + + +def test_stop_calls_pdsh_with_collectl_pkill() -> None: + """Stop collectl processes through pdsh.""" + stop_runner = MagicMock() + with ( + patch("monitoring.base.settings") as mock_settings, + patch("monitoring.collectl_monitoring.common.pdsh", return_value=stop_runner) as mock_pdsh, + ): + mock_settings.getnodes.return_value = "resolved-nodes" + monitor = CollectlMonitoring({}) + + monitor.stop(None) + + mock_pdsh.assert_called_once_with("resolved-nodes", "pkill -SIGINT -f collectl") + stop_runner.communicate.assert_called_once_with() + + +def test_default_nodes_matches_collectl_configuration() -> None: + """Expose the expected default node groups for collectl monitoring.""" + assert CollectlMonitoring.DEFAULT_NODES == ["clients", "osds", "mons", "rgws"] diff --git a/tests/test_monitoring_factory.py b/tests/test_monitoring_factory.py new file mode 100644 index 00000000..e3bf1fea --- /dev/null +++ b/tests/test_monitoring_factory.py @@ -0,0 +1,236 @@ +"""Tests for MonitoringFactory.""" + +# pylint: disable=protected-access + +from typing import Any +from unittest.mock import MagicMock, patch + +import pytest + +from monitoring.blktrace_monitoring import BlktraceMonitoring +from monitoring.collectl_monitoring import CollectlMonitoring +from monitoring.monitoring_factory import MonitoringFactory +from monitoring.perf_monitoring import PerfMonitoring +from monitoring.top_monitoring import TopMonitoring + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _patch_settings(profiles: dict[str, Any]) -> Any: + """Return a context manager that patches settings.monitoring_profiles.""" + return patch("monitoring.monitoring_factory.settings.monitoring_profiles", profiles) + + +def _stub_monitor() -> MagicMock: + """Return a MagicMock that behaves like a Monitoring instance.""" + m = MagicMock() + m.start = MagicMock() + m.stop = MagicMock() + return m + + +# --------------------------------------------------------------------------- +# get_object +# --------------------------------------------------------------------------- + + +def test_get_object_returns_collectl_monitoring() -> None: + """get_object('collectl') returns a CollectlMonitoring instance.""" + with patch("monitoring.base.settings") as mock_base_settings: + mock_base_settings.getnodes.return_value = "node1" + instance = MonitoringFactory.get_object("collectl", {}) + assert isinstance(instance, CollectlMonitoring) + + +def test_get_object_returns_perf_monitoring() -> None: + """get_object('perf') returns a PerfMonitoring instance.""" + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + ): + mock_base_settings.getnodes.return_value = "node1" + mock_settings.cluster.get.return_value = "dummy" + instance = MonitoringFactory.get_object("perf", {}) + assert isinstance(instance, PerfMonitoring) + + +def test_get_object_returns_blktrace_monitoring() -> None: + """get_object('blktrace') returns a BlktraceMonitoring instance.""" + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.blktrace_monitoring.settings") as mock_settings, + ): + mock_base_settings.getnodes.return_value = "node1" + mock_settings.cluster.get.return_value = "dummy" + instance = MonitoringFactory.get_object("blktrace", {}) + assert isinstance(instance, BlktraceMonitoring) + + +def test_get_object_returns_top_monitoring() -> None: + """get_object('top') returns a TopMonitoring instance.""" + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.top_monitoring.settings") as mock_settings, + ): + mock_base_settings.getnodes.return_value = "node1" + mock_settings.cluster.get.return_value = "dummy" + instance = MonitoringFactory.get_object("top", {}) + assert isinstance(instance, TopMonitoring) + + +def test_get_object_raises_for_unknown_key() -> None: + """get_object() raises ValueError for an unrecognised backend name.""" + with pytest.raises(ValueError, match="Unknown monitoring backend: 'bogus'"): + MonitoringFactory.get_object("bogus", {}) + + +# --------------------------------------------------------------------------- +# get_all +# --------------------------------------------------------------------------- + + +def test_get_all_yields_one_instance_per_profile() -> None: + """get_all() yields one Monitoring instance for each configured profile.""" + profiles: dict[str, Any] = {"collectl": {}, "perf": {}} + collectl_inst = _stub_monitor() + perf_inst = _stub_monitor() + + with ( + _patch_settings(profiles), + patch.object(MonitoringFactory, "get_object") as mock_get_object, + ): + mock_get_object.side_effect = [collectl_inst, perf_inst] + result = list(MonitoringFactory.get_all()) + + assert result == [collectl_inst, perf_inst] + mock_get_object.assert_any_call("collectl", {}) + mock_get_object.assert_any_call("perf", {}) + + +def test_get_all_yields_nothing_when_no_profiles() -> None: + """get_all() yields nothing when monitoring_profiles is empty.""" + with _patch_settings({}): + result = list(MonitoringFactory.get_all()) + assert not result + + +# --------------------------------------------------------------------------- +# start +# --------------------------------------------------------------------------- + + +def test_start_calls_start_on_every_monitor() -> None: + """start() calls m.start(directory) for every monitor returned by get_all.""" + m1, m2 = _stub_monitor(), _stub_monitor() + with patch.object(MonitoringFactory, "get_all", return_value=iter([m1, m2])): + MonitoringFactory.start("/tmp/out") + + m1.start.assert_called_once_with("/tmp/out") + m2.start.assert_called_once_with("/tmp/out") + + +# --------------------------------------------------------------------------- +# stop +# --------------------------------------------------------------------------- + + +def test_stop_calls_stop_on_every_monitor() -> None: + """stop() calls m.stop(directory) for every monitor returned by get_all.""" + m1, m2 = _stub_monitor(), _stub_monitor() + with patch.object(MonitoringFactory, "get_all", return_value=iter([m1, m2])): + MonitoringFactory.stop("/tmp/out") + + m1.stop.assert_called_once_with("/tmp/out") + m2.stop.assert_called_once_with("/tmp/out") + + +def test_stop_passes_none_by_default() -> None: + """stop() passes None as directory when called with no argument.""" + m = _stub_monitor() + with patch.object(MonitoringFactory, "get_all", return_value=iter([m])): + MonitoringFactory.stop() + + m.stop.assert_called_once_with(None) + + +# --------------------------------------------------------------------------- +# monitor (context manager) +# --------------------------------------------------------------------------- + + +def test_monitor_starts_then_stops_all() -> None: + """monitor() starts all monitors before yield and stops all after.""" + m1, m2 = _stub_monitor(), _stub_monitor() + call_order: list[str] = [] + + m1.start.side_effect = lambda d: call_order.append("m1.start") + m2.start.side_effect = lambda d: call_order.append("m2.start") + m1.stop.side_effect = lambda d: call_order.append("m1.stop") + m2.stop.side_effect = lambda d: call_order.append("m2.stop") + + with (patch.object(MonitoringFactory, "get_all", return_value=iter([m1, m2])),): + with MonitoringFactory.monitor("/tmp/out"): + call_order.append("body") + + assert call_order == ["m1.start", "m2.start", "body", "m1.stop", "m2.stop"] + + +def test_monitor_stops_all_even_if_body_raises() -> None: + """monitor() runs stop for all monitors even when the body raises.""" + m = _stub_monitor() + with patch.object(MonitoringFactory, "get_all", return_value=iter([m])): + with pytest.raises(RuntimeError): + with MonitoringFactory.monitor("/tmp/out"): + raise RuntimeError("boom") + + m.stop.assert_called_once_with("/tmp/out") + + +# --------------------------------------------------------------------------- +# get_cpu_cycles +# --------------------------------------------------------------------------- + + +def test_get_cpu_cycles_delegates_to_perf_monitor() -> None: + """get_cpu_cycles() returns the value from the first PerfMonitoring instance.""" + perf_inst = MagicMock(spec=PerfMonitoring) + perf_inst.get_cpu_cycles.return_value = 42000 + + with patch.object(MonitoringFactory, "get_all", return_value=iter([perf_inst])): + result = MonitoringFactory.get_cpu_cycles("/tmp/out") + + assert result == 42000 + perf_inst.get_cpu_cycles.assert_called_once_with("/tmp/out") + + +def test_get_cpu_cycles_skips_non_perf_monitors() -> None: + """get_cpu_cycles() skips non-PerfMonitoring instances.""" + collectl_inst = MagicMock(spec=CollectlMonitoring) + perf_inst = MagicMock(spec=PerfMonitoring) + perf_inst.get_cpu_cycles.return_value = 99 + + with patch.object(MonitoringFactory, "get_all", return_value=iter([collectl_inst, perf_inst])): + result = MonitoringFactory.get_cpu_cycles("/tmp/out") + + assert result == 99 + assert not hasattr(collectl_inst, "get_cpu_cycles") or not collectl_inst.get_cpu_cycles.called + + +def test_get_cpu_cycles_returns_none_when_no_perf_configured() -> None: + """get_cpu_cycles() returns None when no PerfMonitoring is in the profiles.""" + collectl_inst = MagicMock(spec=CollectlMonitoring) + + with patch.object(MonitoringFactory, "get_all", return_value=iter([collectl_inst])): + result = MonitoringFactory.get_cpu_cycles("/tmp/out") + + assert result is None + + +def test_get_cpu_cycles_returns_none_when_profiles_empty() -> None: + """get_cpu_cycles() returns None when no profiles are configured at all.""" + with patch.object(MonitoringFactory, "get_all", return_value=iter([])): + result = MonitoringFactory.get_cpu_cycles("/tmp/out") + + assert result is None diff --git a/tests/test_monitoring_perf.py b/tests/test_monitoring_perf.py new file mode 100644 index 00000000..aece298d --- /dev/null +++ b/tests/test_monitoring_perf.py @@ -0,0 +1,179 @@ +"""Tests for the perf monitoring backend.""" + +# pylint: disable=protected-access + +from unittest.mock import MagicMock, mock_open, patch + +from monitoring.perf_monitoring import PerfMonitoring + + +def test_start_local_node_runs_perf_for_each_pid() -> None: + """Start perf locally for each matching pid file.""" + mkdir_runner = MagicMock() + local_runner = MagicMock() + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + patch("monitoring.perf_monitoring.common.pdsh", return_value=mkdir_runner) as mock_pdsh, + patch("monitoring.perf_monitoring.common.get_localnode", return_value="node1"), + patch("monitoring.perf_monitoring.common.sh", return_value=local_runner) as mock_sh, + patch("monitoring.perf_monitoring.glob.glob", return_value=["/var/run/ceph/osd.1.pid"]), + patch("builtins.open", mock_open(read_data="123\n")), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + + monitor.start("/tmp/output") + + mock_pdsh.assert_called_once_with("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/perf") + mkdir_runner.communicate.assert_called_once_with() + mock_sh.assert_called_once_with("node1", "sudo perf stat -p 123 -o /tmp/output/perf/perf_stat.123 &") + assert monitor._perf_runners == [local_runner] + assert monitor._perf_dir == "/tmp/output/perf" + + +def test_start_remote_node_uses_pdsh_loop() -> None: + """Start perf remotely through pdsh when no local node is available.""" + mkdir_runner = MagicMock() + remote_runner = MagicMock() + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + patch("monitoring.perf_monitoring.common.pdsh") as mock_pdsh, + patch("monitoring.perf_monitoring.common.get_localnode", return_value=None), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + mock_pdsh.side_effect = [mkdir_runner, remote_runner] + monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + + monitor.start("/tmp/output") + + mock_pdsh.assert_any_call("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/perf") + mock_pdsh.assert_any_call( + "resolved-nodes", + [ + "for pid in `cat /var/run/ceph/osd.*.pid`;", + "do", + "sudo perf stat -p ${pid} -o /tmp/output/perf/perf_stat.${pid} &", + ";", + "done", + ], + ) + + +def test_stop_kills_local_perf_runners() -> None: + """Kill locally started perf runners when present.""" + runner = MagicMock() + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + monitor._perf_runners = [runner] + + monitor.stop(None) + + runner.kill.assert_called_once_with() + + +def test_stop_uses_pdsh_when_no_local_runners_exist() -> None: + """Stop perf remotely through pdsh when no local runners are tracked.""" + stop_runner = MagicMock() + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + patch("monitoring.perf_monitoring.common.pdsh", return_value=stop_runner) as mock_pdsh, + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + + monitor.stop(None) + + mock_pdsh.assert_called_once_with("resolved-nodes", r"sudo pkill -SIGINT -f perf\ ") + stop_runner.communicate.assert_called_once_with() + + +def test_stop_chowns_output_files_when_directory_is_provided() -> None: + """Adjust ownership of generated perf files when an output directory is given.""" + stop_runner = MagicMock() + chown_data_runner = MagicMock() + chown_stat_runner = MagicMock() + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + patch("monitoring.perf_monitoring.common.pdsh") as mock_pdsh, + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + mock_pdsh.side_effect = [stop_runner, chown_data_runner, chown_stat_runner] + monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + + monitor.stop("/tmp/output") + + mock_pdsh.assert_any_call("resolved-nodes", r"sudo pkill -SIGINT -f perf\ ") + mock_pdsh.assert_any_call("resolved-nodes", "sudo chown ceph.ceph /tmp/output/perf/perf.data") + mock_pdsh.assert_any_call("resolved-nodes", "sudo chown ceph.ceph /tmp/output/perf/perf_stat.*") + + +def test_get_cpu_cycles_returns_total_cycles() -> None: + """Sum cycle counts from all perf stat output files.""" + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + patch("monitoring.perf_monitoring.glob.glob", return_value=["/tmp/output/perf"]), + patch("monitoring.perf_monitoring.os.listdir", return_value=["perf_stat.1", "perf_stat.2"]), + patch( + "builtins.open", + side_effect=[ + mock_open(read_data="1,000 cycles user\n").return_value, + mock_open(read_data="2,500 cycles user\n").return_value, + ], + ), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + + assert monitor.get_cpu_cycles("/tmp/output") == 3500 + + +def test_get_cpu_cycles_returns_none_when_cycles_are_missing() -> None: + """Return None when perf output does not contain a cycles line.""" + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + patch("monitoring.perf_monitoring.glob.glob", return_value=["/tmp/output/perf"]), + patch("monitoring.perf_monitoring.os.listdir", return_value=["perf_stat.1"]), + patch("builtins.open", mock_open(read_data="nothing to match\n")), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + + assert monitor.get_cpu_cycles("/tmp/output") is None diff --git a/tests/test_monitoring_top.py b/tests/test_monitoring_top.py new file mode 100644 index 00000000..2aae8f78 --- /dev/null +++ b/tests/test_monitoring_top.py @@ -0,0 +1,159 @@ +"""Tests for the top monitoring backend.""" + +# pylint: disable=protected-access + +from typing import Any, Optional +from unittest.mock import MagicMock, mock_open, patch + +from monitoring.top_monitoring import TopMonitoring + + +def _make_monitor( + pid_dir: str = "/var/run/ceph", + user: str = "ceph", + mconfig: Optional[dict[str, Any]] = None, +) -> TopMonitoring: + """Construct a TopMonitoring instance with mocked settings.""" + if mconfig is None: + mconfig = {} + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.top_monitoring.settings") as mock_settings, + patch("monitoring.top_monitoring.common.pdsh"), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": pid_dir, + "user": user, + }.get(key, default) + return TopMonitoring(mconfig) + + +def test_default_nodes_is_osds() -> None: + """DEFAULT_NODES must be ['osds'].""" + assert TopMonitoring.DEFAULT_NODES == ["osds"] + + +def test_init_stores_cluster_settings() -> None: + """__init__ reads pid_dir and user from settings.cluster.""" + monitor = _make_monitor(pid_dir="/run/ceph", user="admin") + assert monitor._pid_dir == "/run/ceph" + assert monitor._user == "admin" + + +def test_init_stores_default_args() -> None: + """__init__ uses the default top argument string when mconfig has no 'args'.""" + monitor = _make_monitor() + assert monitor._top_cmd == "top" + assert monitor._pid_glob == "osd.*.pid" + assert "{pid}" in monitor._args + assert "{top_dir}" in monitor._args + + +def test_init_accepts_custom_args() -> None: + """__init__ accepts custom top_cmd, args, and pid_glob from mconfig.""" + monitor = _make_monitor(mconfig={"top_cmd": "htop", "args": "-d 1", "pid_glob": "*.pid"}) + assert monitor._top_cmd == "htop" + assert monitor._args == "-d 1" + assert monitor._pid_glob == "*.pid" + + +def test_start_local_node_runs_top_for_each_pid() -> None: + """start() runs top locally for each matching pid file.""" + mkdir_runner = MagicMock() + local_runner = MagicMock() + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.top_monitoring.settings") as mock_settings, + patch("monitoring.top_monitoring.common.pdsh", return_value=mkdir_runner) as mock_pdsh, + patch("monitoring.top_monitoring.common.get_localnode", return_value="node1"), + patch("monitoring.top_monitoring.common.sh", return_value=local_runner) as mock_sh, + patch("monitoring.top_monitoring.glob.glob", return_value=["/var/run/ceph/osd.1.pid"]), + patch("builtins.open", mock_open(read_data="42\n")), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + monitor = TopMonitoring({}) + + monitor.start("/tmp/output") + + mock_pdsh.assert_called_once_with("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/top") + mkdir_runner.communicate.assert_called_once_with() + expected_cmd = "top -b -H -1 -p 42 -n 30 > /tmp/output/top/42_osd_top.out" + mock_sh.assert_called_once_with("node1", expected_cmd) + assert monitor._top_runners == [local_runner] + + +def test_start_remote_node_uses_pdsh_loop() -> None: + """start() dispatches via pdsh for-loop when no local node is available.""" + mkdir_runner = MagicMock() + remote_runner = MagicMock() + with ( + patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.top_monitoring.settings") as mock_settings, + patch("monitoring.top_monitoring.common.pdsh") as mock_pdsh, + patch("monitoring.top_monitoring.common.get_localnode", return_value=None), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + mock_pdsh.side_effect = [mkdir_runner, remote_runner] + monitor = TopMonitoring({}) + + monitor.start("/tmp/output") + + mock_pdsh.assert_any_call("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/top") + mock_pdsh.assert_any_call( + "resolved-nodes", + [ + "for pid in `cat /var/run/ceph/osd.*.pid`;", + "do", + "top -b -H -1 -p ${pid} -n 30 > /tmp/output/top/${pid}_osd_top.out", + ";", + "done", + ], + ) + + +def test_stop_kills_local_top_runners() -> None: + """stop() kills locally started runners when present.""" + runner = MagicMock() + monitor = _make_monitor() + monitor._top_runners = [runner] + + with patch("monitoring.top_monitoring.common.pdsh") as mock_pdsh: + monitor.stop(None) + + runner.kill.assert_called_once_with() + mock_pdsh.assert_not_called() + + +def test_stop_uses_pdsh_when_no_local_runners_exist() -> None: + """stop() issues pkill via pdsh when no local runners are tracked.""" + stop_runner = MagicMock() + monitor = _make_monitor() + + with patch("monitoring.top_monitoring.common.pdsh", return_value=stop_runner) as mock_pdsh: + monitor.stop(None) + + mock_pdsh.assert_called_once_with("resolved-nodes", r"sudo pkill -SIGINT -f top\ ") + stop_runner.communicate.assert_called_once_with() + + +def test_stop_chowns_output_files_when_directory_is_provided() -> None: + """stop() adjusts ownership of top output files when a directory is given.""" + stop_runner = MagicMock() + chown_runner = MagicMock() + monitor = _make_monitor(user="ceph") + + with patch("monitoring.top_monitoring.common.pdsh") as mock_pdsh: + mock_pdsh.side_effect = [stop_runner, chown_runner] + monitor.stop("/tmp/output") + + mock_pdsh.assert_any_call("resolved-nodes", r"sudo pkill -SIGINT -f top\ ") + mock_pdsh.assert_any_call("resolved-nodes", "sudo chown ceph.ceph /tmp/output/top/*top.out") diff --git a/tests/test_workloads.py b/tests/test_workloads.py index 7ff58c56..484eaa07 100644 --- a/tests/test_workloads.py +++ b/tests/test_workloads.py @@ -136,7 +136,7 @@ def test_get_global_options_excludes_prefill(self) -> None: @patch("workloads.workloads.pdsh") @patch("workloads.workloads.make_remote_dir") - @patch("workloads.workloads.monitoring") + @patch("workloads.workloads.MonitoringFactory") @patch("workloads.workloads.getnodes") @patch("workloads.workloads.sleep") def test_run_with_workloads( @@ -164,7 +164,7 @@ def test_run_with_workloads( @patch("workloads.workloads.pdsh") @patch("workloads.workloads.make_remote_dir") - @patch("workloads.workloads.monitoring") + @patch("workloads.workloads.MonitoringFactory") @patch("workloads.workloads.getnodes") def test_run_with_script( self, @@ -226,7 +226,7 @@ def test_run_without_workloads(self) -> None: @patch("workloads.workloads.pdsh") @patch("workloads.workloads.make_remote_dir") - @patch("workloads.workloads.monitoring") + @patch("workloads.workloads.MonitoringFactory") @patch("workloads.workloads.getnodes") @patch("workloads.workloads.sleep") def test_run_with_ramp_time( diff --git a/workloads/workloads.py b/workloads/workloads.py index 877285c5..cb89de48 100644 --- a/workloads/workloads.py +++ b/workloads/workloads.py @@ -6,8 +6,8 @@ from time import sleep from typing import Optional, Union -import monitoring from common import CheckedPopen, CheckedPopenLocal, make_remote_dir, pdsh # pyright: ignore[reportUnknownVariableType] +from monitoring.monitoring_factory import MonitoringFactory from settings import getnodes # pyright: ignore[reportUnknownVariableType] from workloads.workload import Workload from workloads.workload_types import BenchmarkConfigurationType, WorkloadType, WorkloadYamlType @@ -92,12 +92,12 @@ def run(self) -> None: if ramp_time: sleep(int(ramp_time)) - monitoring.start(output_directory) # type: ignore[no-untyped-call] + MonitoringFactory.start(output_directory) for process in processes: process.wait() # type: ignore[no-untyped-call] - monitoring.stop() # type: ignore[no-untyped-call] + MonitoringFactory.stop() log.info("== Workloads completed ==") From c2716ee4a97e3d7198ad129fdf594a3926882739 Mon Sep 17 00:00:00 2001 From: Chris Harris Date: Thu, 2 Jul 2026 13:36:20 +0100 Subject: [PATCH 3/3] Post Processing: Add top resource usage to the plots Add plotting of top and collectl resource monitoring to CBT. This required a slight re-factoring of both top and perf monitoring as they were specifically targetted twards OSDs, requiring an OSD PID file and only monitoring OSD PIDs, no matter what arguments were passed. There is now a generic TopMonitoring and PerfMonitoring class, which will run the arguments passed in the YAML, an an Osd*Monitoring class that will require the OSD PID file as the current behaviour. Signed-off-by: Chris Harris: Assisted-by: IBM Bob v 2.0 and 2.0.1 --- benchmark/librbdfio.py | 2 +- example/rbd_fio_test.yml | 5 +- monitoring/blktrace_monitoring.py | 2 +- monitoring/collectl_monitoring.py | 2 +- monitoring/{base.py => monitoring.py} | 0 monitoring/monitoring_factory.py | 12 +- monitoring/perf_monitoring.py | 69 ++- monitoring/top_monitoring.py | 98 +++- post_processing/plotter/cpu_plotter.py | 17 +- post_processing/plotter/io_plotter.py | 11 +- post_processing/plotter/memory_plotter.py | 15 +- .../plotter/time_series_latency_plotter.py | 4 +- .../run_results/resource_result_factory.py | 11 + .../run_results/resources/fio_resource.py | 8 +- .../run_results/resources/top_resource.py | 257 +++++++++ post_processing/run_results/run_result.py | 129 ++++- tests/test_cpu_plotter.py | 11 +- tests/test_fio_resource_result.py | 21 +- tests/test_io_plotter.py | 5 +- tests/test_monitoring_base.py | 10 +- tests/test_monitoring_blktrace.py | 4 +- tests/test_monitoring_collectl.py | 8 +- tests/test_monitoring_factory.py | 36 +- tests/test_monitoring_perf.py | 279 +++++++--- tests/test_monitoring_top.py | 280 +++++++--- tests/test_resource_result_factory.py | 133 +++++ tests/test_run_result.py | 489 ++++++++++++----- tests/test_time_series_latency_plotter.py | 4 +- tests/test_top_resource.py | 500 ++++++++++++++++++ 29 files changed, 2019 insertions(+), 403 deletions(-) rename monitoring/{base.py => monitoring.py} (100%) create mode 100644 post_processing/run_results/resources/top_resource.py create mode 100644 tests/test_top_resource.py diff --git a/benchmark/librbdfio.py b/benchmark/librbdfio.py index 0b2b564e..e5166ead 100644 --- a/benchmark/librbdfio.py +++ b/benchmark/librbdfio.py @@ -188,7 +188,7 @@ def run(self): force_refresh=report_config.get("force_refresh", False), no_error_bars=report_config.get("no_error_bars", False), report_type=ReportType.SIMPLE, - plot_resources=report_config.get("plot_resource", False), + plot_resources=report_config.get("plot_resources", False), ) report: Report = Report(report_options) report.generate() diff --git a/example/rbd_fio_test.yml b/example/rbd_fio_test.yml index cdaf9385..76bfde31 100644 --- a/example/rbd_fio_test.yml +++ b/example/rbd_fio_test.yml @@ -44,11 +44,11 @@ monitoring_profiles: # produce gnuplot file data format (aka csv) in output file (gziped) # args: '-c 30 -sZC -i 5:10 --procopts t --cpufilt 0-3 --procfilt cosd -P -f {collectl_dir}' - perf: + osd_perf: perf_cmd: 'perf' # This collects 10 secs of data to produce flame graphs args: 'record -e cycles:u --call-graph dwarf -i -p {pid} -o {perf_dir}/{pid}_osd_perf.out sleep 10' - top: + osd_top: top_cmd: 'top' # This collects 30 samples, Core and thread CPU utilisation args: '-b -H -1 -p {pid} -n 30 > {top_dir}/{pid}_osd_top.out' @@ -77,7 +77,6 @@ benchmarks: poolname: 'rbd' mode: 'randwrite' iodepth: [1, 4 ,16] - numjobs: [1, 4, 8] op_size: [4096] # block IO size in bytes procs_per_client: [1] volumes_per_client: [1] # volumes per ceph node diff --git a/monitoring/blktrace_monitoring.py b/monitoring/blktrace_monitoring.py index 49df00cc..0abb7de4 100644 --- a/monitoring/blktrace_monitoring.py +++ b/monitoring/blktrace_monitoring.py @@ -5,7 +5,7 @@ import common import settings -from monitoring.base import Monitoring +from monitoring.monitoring import Monitoring logger = logging.getLogger("cbt") diff --git a/monitoring/collectl_monitoring.py b/monitoring/collectl_monitoring.py index 32a52ce4..a97a32b2 100644 --- a/monitoring/collectl_monitoring.py +++ b/monitoring/collectl_monitoring.py @@ -3,7 +3,7 @@ from typing import Any, ClassVar, Optional import common -from monitoring.base import Monitoring +from monitoring.monitoring import Monitoring class CollectlMonitoring(Monitoring): diff --git a/monitoring/base.py b/monitoring/monitoring.py similarity index 100% rename from monitoring/base.py rename to monitoring/monitoring.py diff --git a/monitoring/monitoring_factory.py b/monitoring/monitoring_factory.py index 4adc2b78..e321739c 100644 --- a/monitoring/monitoring_factory.py +++ b/monitoring/monitoring_factory.py @@ -1,15 +1,15 @@ """Factory class for creating and managing monitoring backends.""" -from collections.abc import Iterator +from collections.abc import Generator, Iterator from contextlib import contextmanager from typing import Any, ClassVar, Optional import settings -from monitoring.base import Monitoring from monitoring.blktrace_monitoring import BlktraceMonitoring from monitoring.collectl_monitoring import CollectlMonitoring -from monitoring.perf_monitoring import PerfMonitoring -from monitoring.top_monitoring import TopMonitoring +from monitoring.monitoring import Monitoring +from monitoring.perf_monitoring import OsdPerfMonitoring, PerfMonitoring +from monitoring.top_monitoring import OsdTopMonitoring, TopMonitoring class MonitoringFactory: @@ -18,8 +18,10 @@ class MonitoringFactory: _REGISTRY: ClassVar[dict[str, type[Monitoring]]] = { "collectl": CollectlMonitoring, "perf": PerfMonitoring, + "osd_perf": OsdPerfMonitoring, "blktrace": BlktraceMonitoring, "top": TopMonitoring, + "osd_top": OsdTopMonitoring, } @classmethod @@ -54,7 +56,7 @@ def stop(cls, directory: Optional[str] = None) -> None: @classmethod @contextmanager - def monitor(cls, directory: str) -> Iterator[None]: + def monitor(cls, directory: str) -> Generator[None, None, None]: """Context manager: start all monitors, yield, then stop all.""" monitors = list(cls.get_all()) for monitor in monitors: diff --git a/monitoring/perf_monitoring.py b/monitoring/perf_monitoring.py index 8f488544..bad76ae0 100644 --- a/monitoring/perf_monitoring.py +++ b/monitoring/perf_monitoring.py @@ -8,13 +8,17 @@ import common import settings -from monitoring.base import Monitoring +from monitoring.monitoring import Monitoring logger = logging.getLogger("cbt") class PerfMonitoring(Monitoring): - """Monitoring backend that captures perf output.""" + """Monitoring backend that captures perf output. + + Runs ``perf`` with a caller-supplied ``args`` template. For OSD-specific + PID discovery use :class:`OsdPerfMonitoring` instead. + """ DEFAULT_NODES: ClassVar[list[str]] = ["osds"] @@ -22,8 +26,6 @@ def __init__(self, mconfig: dict[str, Any]) -> None: """Initialize perf monitoring configuration.""" super().__init__(mconfig) # Remove these casts once settings.cluster.get() is fully typed. - self._pid_dir = cast(str, settings.cluster.get("pid_dir")) - self._pid_glob = mconfig.get("pid_glob", "osd.*.pid") self._user = cast(str, settings.cluster.get("user")) self._perf_cmd = mconfig.get("perf_cmd", "sudo perf") self._args_template = mconfig.get("args") @@ -36,25 +38,13 @@ def start(self, directory: str) -> None: self._perf_dir = perf_dir common.pdsh(self._nodes, f"mkdir -p -m0755 -- {perf_dir}").communicate() # type: ignore[no-untyped-call] - perf_template = f"{self._perf_cmd} {self._args_template} &" + perf_cmd = f"{self._perf_cmd} {self._args_template} &".format(perf_dir=perf_dir) local_node = common.get_localnode(self._nodes) # type: ignore[no-untyped-call] if local_node: - logger.debug("PerfMonitoring: in local_node") - logger.debug("pid_dir: %s", self._pid_dir) - for pid_path in glob.glob(os.path.join(self._pid_dir, self._pid_glob)): - logger.debug("PerfMonitoring pid_path: %s", pid_path) - with open(pid_path, encoding="utf-8") as pidfile: - pid = pidfile.read().strip() - perf_cmd = perf_template.format(perf_dir=perf_dir, pid=pid) - runner = common.sh(local_node, perf_cmd) # type: ignore[no-untyped-call] - self._perf_runners.append(runner) + runner = common.sh(local_node, perf_cmd) # type: ignore[no-untyped-call] + self._perf_runners.append(runner) else: - logger.debug("PerfMonitoring: remote_node") - perf_cmd = perf_template.format(perf_dir=perf_dir, pid="${pid}") - common.pdsh( # type: ignore[no-untyped-call] - self._nodes, - [f"for pid in `cat {self._pid_dir}/{self._pid_glob}`;", "do", perf_cmd, ";", "done"], - ) + common.pdsh(self._nodes, perf_cmd) # type: ignore[no-untyped-call] def stop(self, directory: Optional[str]) -> None: """Stop perf collection and adjust file ownership when needed.""" @@ -85,3 +75,42 @@ def get_cpu_cycles(self, out_dir: str) -> Optional[int]: return None total_cpu_cycles = total_cpu_cycles + int(cpu_cycles.replace(",", "")) return cast(Optional[int], total_cpu_cycles) + + +class OsdPerfMonitoring(PerfMonitoring): + """PerfMonitoring specialised for Ceph OSD processes. + + Discovers the target PIDs by scanning PID files matching ``pid_glob`` + inside ``pid_dir`` (read from ``settings.cluster``), then launches a + separate ``perf`` invocation per OSD PID. + """ + + def __init__(self, mconfig: dict[str, Any]) -> None: + """Initialize OSD perf monitoring configuration.""" + super().__init__(mconfig) + self._pid_dir = cast(str, settings.cluster.get("pid_dir")) + self._pid_glob = mconfig.get("pid_glob", "osd.*.pid") + + def start(self, directory: str) -> None: + """Create the perf output directory and start a perf instance per OSD PID.""" + perf_dir = f"{directory}/perf" + self._perf_dir = perf_dir + common.pdsh(self._nodes, f"mkdir -p -m0755 -- {perf_dir}").communicate() # type: ignore[no-untyped-call] + + perf_template = f"{self._perf_cmd} {self._args_template} &" + local_node = common.get_localnode(self._nodes) # type: ignore[no-untyped-call] + if local_node: + logger.debug("OsdPerfMonitoring: local_node pid_dir=%s", self._pid_dir) + for pid_path in glob.glob(os.path.join(self._pid_dir, self._pid_glob)): + with open(pid_path, encoding="utf-8") as pidfile: + pid = pidfile.read().strip() + perf_cmd = perf_template.format(perf_dir=perf_dir, pid=pid) + runner = common.sh(local_node, perf_cmd) # type: ignore[no-untyped-call] + self._perf_runners.append(runner) + else: + logger.debug("OsdPerfMonitoring: remote_node") + perf_cmd = perf_template.format(perf_dir=perf_dir, pid="${pid}") + common.pdsh( # type: ignore[no-untyped-call] + self._nodes, + [f"for pid in `cat {self._pid_dir}/{self._pid_glob}`;", "do", perf_cmd, ";", "done"], + ) diff --git a/monitoring/top_monitoring.py b/monitoring/top_monitoring.py index 783bdf2b..7eb30d6b 100644 --- a/monitoring/top_monitoring.py +++ b/monitoring/top_monitoring.py @@ -7,25 +7,32 @@ import common import settings -from monitoring.base import Monitoring +from monitoring.monitoring import Monitoring logger = logging.getLogger("cbt") class TopMonitoring(Monitoring): - """Monitoring backend that captures top output for OSD processes.""" + """Monitoring backend that runs top against an explicit PID list or system-wide. + + When ``args`` contains ``{pid}``, the caller is responsible for supplying + the PID list via ``start()``. For OSD-specific PID discovery use + :class:`OsdTopMonitoring` instead. + """ DEFAULT_NODES: ClassVar[list[str]] = ["osds"] def __init__(self, mconfig: dict[str, Any]) -> None: """Initialize top monitoring configuration.""" super().__init__(mconfig) - # Remove these casts once settings.cluster.get() is fully typed. - self._pid_dir = cast(str, settings.cluster.get("pid_dir")) - self._pid_glob = mconfig.get("pid_glob", "osd.*.pid") self._user = cast(str, settings.cluster.get("user")) self._top_cmd = mconfig.get("top_cmd", "top") - self._args = mconfig.get("args", "-b -H -1 -p {pid} -n 30 > {top_dir}/{pid}_osd_top.out") + # NOTE: top's %CPU column behaviour depends on the Irix/Solaris mode + # toggle ('I' key / Mode_irixps in ~/.toprc). The procps-ng build + # default is Irix mode ON (%CPU is per-core). TopResource assumes + # this. There is no CLI flag to enforce it; if a user has toggled it + # off in their ~/.toprc the resulting CPU figures will be incorrect. + self._args = mconfig.get("args", "-b -H -1 -n 30 > {top_dir}/top.out") self._top_runners: list[Any] = [] def start(self, directory: str) -> None: @@ -33,25 +40,13 @@ def start(self, directory: str) -> None: top_dir = f"{directory}/top" common.pdsh(self._nodes, f"mkdir -p -m0755 -- {top_dir}").communicate() # type: ignore[no-untyped-call] - top_template = f"{self._top_cmd} {self._args}" + top_cmd = f"{self._top_cmd} {self._args}".format(top_dir=top_dir) local_node = common.get_localnode(self._nodes) # type: ignore[no-untyped-call] if local_node: - logger.debug("TopMonitoring: in local_node") - logger.debug("pid_dir: %s", self._pid_dir) - for pid_path in glob.glob(os.path.join(self._pid_dir, self._pid_glob)): - logger.debug("TopMonitoring pid_path: %s", pid_path) - with open(pid_path, encoding="utf-8") as pidfile: - pid = pidfile.read().strip() - top_cmd = top_template.format(top_dir=top_dir, pid=pid) - runner = common.sh(local_node, top_cmd) # type: ignore[no-untyped-call] - self._top_runners.append(runner) + runner = common.sh(local_node, top_cmd) # type: ignore[no-untyped-call] + self._top_runners.append(runner) else: - logger.debug("TopMonitoring: remote_node") - top_cmd = top_template.format(top_dir=top_dir, pid="${pid}") - common.pdsh( # type: ignore[no-untyped-call] - self._nodes, - [f"for pid in `cat {self._pid_dir}/{self._pid_glob}`;", "do", top_cmd, ";", "done"], - ) + common.pdsh(self._nodes, top_cmd).communicate() # type: ignore[no-untyped-call] def stop(self, directory: Optional[str]) -> None: """Stop top collection and adjust file ownership when needed.""" @@ -59,9 +54,66 @@ def stop(self, directory: Optional[str]) -> None: for runner in self._top_runners: runner.kill() else: - common.pdsh(self._nodes, r"sudo pkill -SIGINT -f top\ ").communicate() # type: ignore[no-untyped-call] + pkill_cmd = f"sudo pkill -SIGINT -f '{self._top_cmd} {self._args}'" + common.pdsh(self._nodes, pkill_cmd).communicate() # type: ignore[no-untyped-call] if directory: common.pdsh( # type: ignore[no-untyped-call] self._nodes, f"sudo chown {self._user}.{self._user} {directory}/top/*top.out", ) + + +class OsdTopMonitoring(TopMonitoring): + """TopMonitoring specialised for Ceph OSD processes. + + Discovers the target PIDs by scanning PID files matching ``pid_glob`` + inside ``pid_dir`` (read from ``settings.cluster``), then launches a + separate ``top`` invocation per OSD PID. + """ + + def __init__(self, mconfig: dict[str, Any]) -> None: + """Initialize OSD top monitoring configuration.""" + super().__init__(mconfig) + self._pid_dir = cast(str, settings.cluster.get("pid_dir")) + self._pid_glob = mconfig.get("pid_glob", "osd.*.pid") + # Override default args to include per-pid placeholders. + self._args = mconfig.get("args", "-b -H -1 -p {pid} -n 30 > {top_dir}/{pid}_osd_top.out") + # NOTE: see TopMonitoring.__init__ comment regarding Irix/Solaris mode. + + def start(self, directory: str) -> None: + """Create the top output directory and start a top instance per OSD PID.""" + top_dir = f"{directory}/top" + common.pdsh(self._nodes, f"mkdir -p -m0755 -- {top_dir}").communicate() # type: ignore[no-untyped-call] + + top_template = f"{self._top_cmd} {self._args}" + local_node = common.get_localnode(self._nodes) # type: ignore[no-untyped-call] + if local_node: + logger.debug("OsdTopMonitoring: local_node pid_dir=%s", self._pid_dir) + pid_paths = glob.glob(os.path.join(self._pid_dir, self._pid_glob)) + if not pid_paths: + logger.warning( + "OsdTopMonitoring: no PID files matched %s in %s — no top processes started", + self._pid_glob, + self._pid_dir, + ) + for pid_path in pid_paths: + with open(pid_path, encoding="utf-8") as pidfile: + pid = pidfile.read().strip() + top_cmd = top_template.format(top_dir=top_dir, pid=pid) + runner = common.sh(local_node, top_cmd) # type: ignore[no-untyped-call] + self._top_runners.append(runner) + else: + logger.debug("OsdTopMonitoring: remote_node") + pid_glob_path = f"{self._pid_dir}/{self._pid_glob}" + ls_runner = common.pdsh(self._nodes, f"ls {pid_glob_path} 2>/dev/null") # type: ignore[no-untyped-call] + stdout, _ = ls_runner.communicate() + if not stdout.strip(): + logger.warning( + "OsdTopMonitoring: no PID files matched %s on remote nodes — no top processes started", + pid_glob_path, + ) + top_cmd = top_template.format(top_dir=top_dir, pid="${pid}") + common.pdsh( # type: ignore[no-untyped-call] + self._nodes, + [f"for pid in `cat {pid_glob_path}`;", "do", top_cmd, ";", "done"], + ).communicate() diff --git a/post_processing/plotter/cpu_plotter.py b/post_processing/plotter/cpu_plotter.py index 9f4bc95c..3164f3a7 100644 --- a/post_processing/plotter/cpu_plotter.py +++ b/post_processing/plotter/cpu_plotter.py @@ -15,11 +15,15 @@ CPU_Y_LABEL: str = "System CPU use (%)" CPU_PLOT_LABEL: str = "CPU use" -# Color mapping for different resource sources +# Color mapping for different resource sources. +# Colours are verified to be perceptually distinct under normal vision and the three +# main forms of colour-blindness (deuteranopia, protanopia, tritanopia) using ΔE≥15 +# for every pair. Also distinct from IO lines (xkcd:cerulean) and memory lines. CPU_SOURCE_COLOURS: dict[str, str] = { - "fio": "xkcd:leaf green", - "collectl": "xkcd:sky blue", - "default": "xkcd:orange", + "fio": "xkcd:grass green", # vivid green + "collectl": "xkcd:burnt orange", # deep reddish-orange + "top": "xkcd:coral", # warm pink-red + "default": "xkcd:goldenrod", # bright yellow-gold (legacy single-source fallback) } @@ -83,6 +87,7 @@ def plot(self, x_data: list[float], colour: str = "") -> None: cpu_axis = self._main_axes.twinx() cpu_axis.set_ylabel(CPU_Y_LABEL) + cpu_axis.set_ylim(0, 100) # Plot a line for each source for source in sorted(self._y_data_by_source.keys()): @@ -98,7 +103,3 @@ def plot(self, x_data: list[float], colour: str = "") -> None: # Plot this source's data cpu_axis.plot(x_data, y_data, label=label, color=source_colour, linestyle="-", linewidth=1.5, marker="o") - - # Add legend if multiple sources - if len(self._y_data_by_source) > 1: - cpu_axis.legend(loc="upper right") diff --git a/post_processing/plotter/io_plotter.py b/post_processing/plotter/io_plotter.py index f3b976fa..777e37d9 100644 --- a/post_processing/plotter/io_plotter.py +++ b/post_processing/plotter/io_plotter.py @@ -9,7 +9,7 @@ log: Logger = getLogger("plotter") -IO_PLOT_DEFAULT_COLOUR: str = "xkcd:leaf green" # Leaf green from xkcd color survey +IO_PLOT_DEFAULT_COLOUR: str = "xkcd:cerulean" # Cerulean from xkcd color survey; distinct from CPU/memory lines IO_Y_LABEL: str = "Latency (ms)" IO_PLOT_LABEL: str = "IO Details" @@ -48,5 +48,12 @@ def plot_with_error_bars(self, x_data: list[float], error_data: list[float], cap io_axis.set_ylabel(self.y_label) # io_axis.tick_params(axis="y") # pyright: ignore[reportUnknownMemberType] io_axis.errorbar( # pyright: ignore[reportUnknownMemberType] - x_data, self._y_data, yerr=error_data, fmt="+-", capsize=cap_size, ecolor="red", label=self._label + x_data, + self._y_data, + yerr=error_data, + fmt="+-", + capsize=cap_size, + color=IO_PLOT_DEFAULT_COLOUR, + ecolor="xkcd:red", + label=self._label, ) diff --git a/post_processing/plotter/memory_plotter.py b/post_processing/plotter/memory_plotter.py index aae02e2c..86b049f2 100644 --- a/post_processing/plotter/memory_plotter.py +++ b/post_processing/plotter/memory_plotter.py @@ -4,10 +4,9 @@ """ from logging import Logger, getLogger -from typing import Any, Union +from typing import Union from matplotlib.axes import Axes -from matplotlib.axes._axes import Axes from typing_extensions import override from post_processing.plotter.axis_plotter import AxisPlotter @@ -17,11 +16,15 @@ MEMORY_Y_LABEL: str = "Memory use (Mb)" MEMORY_PLOT_LABEL: str = "Memory use" -# Color mapping for different resource sources +# Color mapping for different resource sources. +# Colours are verified to be perceptually distinct under normal vision and the three +# main forms of colour-blindness (deuteranopia, protanopia, tritanopia) using ΔE≥15 +# for every pair. Also distinct from IO lines (xkcd:cerulean) and CPU lines. MEMORY_SOURCE_COLOURS: dict[str, str] = { - "fio": "xkcd:purple", - "collectl": "xkcd:red", - "default": "xkcd:orange", + "fio": "xkcd:periwinkle", # blue-purple + "collectl": "xkcd:magenta", # vivid pink-purple + "top": "xkcd:slate blue", # cool blue-grey + "default": "xkcd:moss green", # muted earthy green (legacy single-source fallback) } diff --git a/post_processing/plotter/time_series_latency_plotter.py b/post_processing/plotter/time_series_latency_plotter.py index e3a0773d..e83de039 100644 --- a/post_processing/plotter/time_series_latency_plotter.py +++ b/post_processing/plotter/time_series_latency_plotter.py @@ -15,9 +15,9 @@ log: Logger = getLogger("plotter") LATENCY_MEAN_COLOR: str = "xkcd:orange" # Orange from xkcd color survey -LATENCY_P50_COLOR: str = "xkcd:green" # Green from xkcd color survey +LATENCY_P50_COLOR: str = "xkcd:blue" # Blue from xkcd color survey LATENCY_P95_COLOR: str = "xkcd:red" # Red from xkcd color survey -LATENCY_P99_COLOR: str = "xkcd:dark red" # Dark red from xkcd color survey +LATENCY_P99_COLOR: str = "xkcd:purple" # Purple from xkcd color survey LATENCY_MAX_COLOR: str = "xkcd:dark grey" # Dark grey from xkcd color survey LATENCY_Y_LABEL: str = "Latency (ms)" LATENCY_PLOT_LABEL: str = "Mean Latency" diff --git a/post_processing/run_results/resource_result_factory.py b/post_processing/run_results/resource_result_factory.py index e008a223..abd34ed3 100644 --- a/post_processing/run_results/resource_result_factory.py +++ b/post_processing/run_results/resource_result_factory.py @@ -12,6 +12,7 @@ from post_processing.run_results.resource_result import ResourceResult from post_processing.run_results.resources.collectl_resource import CollectlResource from post_processing.run_results.resources.fio_resource import FIOResource +from post_processing.run_results.resources.top_resource import TopResource log: Logger = getLogger("formatter") @@ -51,6 +52,16 @@ def get_all_resources(file_path: Path) -> list[ResourceResult]: except Exception as e: log.warning("Could not create Collectl resource for %s: %s", file_path, e) + # Check for top monitoring data + top_dir = file_path.parent / "top" + if top_dir.exists() and top_dir.is_dir(): + try: + top_resource = TopResource(file_path) + resources.append(top_resource) + log.debug("Added Top resource for %s", file_path) + except Exception as e: + log.warning("Could not create Top resource for %s: %s", file_path, e) + if not resources: log.error("No resource parsers available for %s", file_path) diff --git a/post_processing/run_results/resources/fio_resource.py b/post_processing/run_results/resources/fio_resource.py index a5a4cfbf..f165a587 100644 --- a/post_processing/run_results/resources/fio_resource.py +++ b/post_processing/run_results/resources/fio_resource.py @@ -2,6 +2,7 @@ Process the CPU statistics as provided by FIO """ +import os from logging import Logger, getLogger from pathlib import Path from typing import Any @@ -43,17 +44,20 @@ def _parse(self, data: dict[str, Any]) -> None: Extract CPU and memory usage from FIO output data. Combines system CPU and user CPU percentages to get total CPU usage. + FIO reports usr_cpu/sys_cpu as a percentage of one CPU core + (100% = one core fully busy), so the sum is divided by os.cpu_count() + to normalise to system capacity (0-100% = all cores fully busy). Memory usage is currently not extracted from FIO output. Args: data: Dictionary containing parsed FIO JSON output """ memory_usage: float = 0.0 - cpu_usage: float = 0.0 sys_cpu: float = float(f"{data['jobs'][0]['sys_cpu']}") user_cpu: float = float(f"{data['jobs'][0]['usr_cpu']}") - cpu_usage = sys_cpu + user_cpu + cpu_count: int = os.cpu_count() or 1 + cpu_usage: float = (sys_cpu + user_cpu) / cpu_count self._cpu = f"{cpu_usage:02f}" self._memory = f"{memory_usage:02f}" diff --git a/post_processing/run_results/resources/top_resource.py b/post_processing/run_results/resources/top_resource.py new file mode 100644 index 00000000..5bc44ad7 --- /dev/null +++ b/post_processing/run_results/resources/top_resource.py @@ -0,0 +1,257 @@ +""" +Process CPU and memory statistics from Linux top batch-mode output files. + +CBT runs ``top -b -H -1 -p {pid} -n {count}`` for each monitored PID and +writes output to a ``top/`` subdirectory of the benchmark run directory +(e.g. ``results/.../iodepth-000128/top/``). + +The output filename is controlled by the ``args`` template in +``TopMonitoring`` and can be anything the user configures. ``TopResource`` +therefore reads **every file** in the ``top/`` directory rather than +filtering by a fixed glob, keeping the two components fully decoupled. + +The -H flag shows individual threads, so each snapshot may contain many lines. + +CPU aggregation (Irix mode assumption) +-------------------------------------- +The ``%CPU`` column in top's task area is controlled by the Irix/Solaris mode +toggle (``I`` key, or ``Mode_irixps`` in ``~/.toprc``). + +* **Irix mode ON** (procps-ng build default, ``Mode_irixps=1``): + ``%CPU`` is per-core — 100 % means one core fully busy. + A 32-thread OSD that saturates 8 cores will show eight rows near 100 %. + +* **Solaris mode** (``Mode_irixps=0``, user must have explicitly toggled and + saved their ``~/.toprc``): + ``%CPU`` is already divided by the total CPU count — 100 % means all cores + fully busy. Applying the normalisation below would double-correct. + +There is no procps-ng command-line flag that forces Irix mode when combined +with batch mode (``-A`` requires being the sole argument). CBT therefore +assumes Irix mode is active — the factory default on all mainstream +distributions. + +Aggregation steps (Irix mode): + +1. For each snapshot in a PID file: **sum** ``%CPU`` across all thread rows + to get the total core-utilisation at that instant. +2. **Average** the per-snapshot totals across all snapshots in that file to + get a representative core-utilisation for that PID. +3. **Sum** the per-file averages across all PID files (each file is one OSD + process) to get the total OSD-fleet core-utilisation. +4. **Divide by** ``os.cpu_count()`` to normalise to the range 0-100 % + (100 % = all cores on the host fully busy). + +Memory: ``%MEM`` is a system-relative percentage regardless of Irix/Solaris +mode, so it is simply averaged across all threads, snapshots, and files. +""" + +import os +from logging import Logger, getLogger +from pathlib import Path +from typing import Any + +from post_processing.run_results.resource_result import ResourceResult + +log: Logger = getLogger("formatter") + + +class TopResource(ResourceResult): + """ + Processes resource usage from Linux top batch-mode output files. + + CBT stores one file per monitored PID in a ``top/`` subdirectory alongside + the benchmark output file. Each file contains multiple top snapshots + (because top is run with ``-n ``). + + All files present in the ``top/`` directory are parsed — the output + filename is determined by the ``args`` template in ``TopMonitoring`` and + may be anything the user configures. + + CPU is normalised to 0-100 % of total system capacity. See the module + docstring for the full aggregation algorithm and the Irix mode assumption. + + Memory is averaged across all threads, snapshots, and files. + + Args: + file_path: Path to the benchmark output file (e.g., ``json_output.0``) + """ + + @property + def source(self) -> str: + """Return the source identifier for this resource parser.""" + return "top" + + def _get_resource_output_file_from_file_path(self, file_path: Path) -> Path: + """ + Locate the top output directory from the benchmark result file path. + + The ``top/`` directory must exist next to the benchmark output file + and contain at least one file. All files present are parsed — the + naming convention is not enforced here. + + Args: + file_path: Path to the benchmark output file (e.g., ``json_output.0``) + + Returns: + Path to the top output directory + + Raises: + FileNotFoundError: If the top directory does not exist or is empty + """ + top_dir = file_path.parent / "top" + + if not top_dir.exists(): + raise FileNotFoundError(f"top directory not found: {top_dir}") + + if not any(top_dir.iterdir()): + raise FileNotFoundError(f"top directory is empty: {top_dir}") + + # Return the directory so _parse() can iterate all files inside it + return top_dir + + def _read_results_from_file(self) -> dict[str, Any]: + """ + Override parent: top output is plain text, not JSON. + + Returns: + Empty dict; parsing happens directly in _parse_top_directory() + """ + return {} + + def _parse(self, data: dict[str, Any]) -> None: + """ + Parse all *_osd_top.out files and average CPU and memory across all + threads, snapshots, and PID files. + + Args: + data: Not used (top output is plain text, not JSON) + """ + try: + cpu_usage, memory_usage = self._parse_top_directory() + self._cpu = f"{cpu_usage:.2f}" + self._memory = f"{memory_usage:.2f}" + self._has_been_parsed = True + except Exception as e: # pylint: disable=broad-except + log.error("Failed to parse top data from %s: %s", self._resource_file_path, e) + self._cpu = "0.00" + self._memory = "0.00" + self._has_been_parsed = True + + def _parse_top_directory(self) -> tuple[float, float]: + """ + Aggregate CPU and memory from all files in the top directory. + + CPU aggregation (assumes Irix mode — see module docstring): + - Per file: sum threads per snapshot, then average across snapshots. + - Across files: sum the per-file averages (one file = one OSD process). + - Final: divide by os.cpu_count() to normalise to system capacity. + + Memory: averaged across all thread rows, all snapshots, all files. + + Returns: + Tuple of (normalised_cpu_percent, average_memory_percent) + """ + top_files = [f for f in self._resource_file_path.iterdir() if f.is_file()] + + all_mem: list[float] = [] + total_cpu: float = 0.0 + file_count: int = 0 + + for top_file in top_files: + snapshot_cpu_totals, mem_samples = self._parse_top_file(top_file) + if snapshot_cpu_totals: + total_cpu += sum(snapshot_cpu_totals) / len(snapshot_cpu_totals) + file_count += 1 + all_mem.extend(mem_samples) + + if not file_count: + log.warning("No valid CPU samples found in %s", self._resource_file_path) + return 0.0, 0.0 + + cpu_count: int = os.cpu_count() or 1 + normalised_cpu = total_cpu / cpu_count + avg_mem = sum(all_mem) / len(all_mem) if all_mem else 0.0 + log.debug("Parsed %d PID files from top, normalised CPU: %.2f%%", file_count, normalised_cpu) + + return normalised_cpu, avg_mem + + def _parse_top_file(self, file_path: Path) -> tuple[list[float], list[float]]: + """ + Parse a single top batch-mode output file. + + top -b produces multiple snapshots concatenated in one file. Each + snapshot has a column-header line starting with "PID", followed by one + data row per thread. We locate the %CPU and %MEM columns from the + header and extract values from every subsequent data row until the next + snapshot boundary (a line beginning with 'top'). + + Returns a list of **per-snapshot CPU totals** (sum of all thread %CPU + values within that snapshot) and a flat list of all %MEM values seen. + + Args: + file_path: Path to a single top output file + + Returns: + Tuple of (per_snapshot_cpu_total_list, flat_mem_sample_list) + """ + snapshot_cpu_totals: list[float] = [] + all_mem: list[float] = [] + + current_snapshot_cpu: float = 0.0 + in_process_block: bool = False + has_rows_in_snapshot: bool = False + cpu_col: int = -1 + mem_col: int = -1 + + with open(file_path, encoding="utf-8") as f: + lines = f.readlines() + + for line in lines: + stripped = line.strip() + + # Column-header line anchors the column positions for this snapshot. + # Commit any in-progress snapshot total first. + if stripped.startswith("PID"): + if has_rows_in_snapshot: + snapshot_cpu_totals.append(current_snapshot_cpu) + current_snapshot_cpu = 0.0 + has_rows_in_snapshot = False + headers = stripped.split() + try: + cpu_col = headers.index("%CPU") + mem_col = headers.index("%MEM") + except ValueError: + log.warning("Could not find %%CPU/%%MEM columns in header: %s", stripped) + in_process_block = False + continue + in_process_block = True + continue + + if not in_process_block: + continue + + # Non-process lines mark a snapshot boundary or summary section + if any(stripped.startswith(kw) for kw in ("top", "%Cpu", "Tasks", "MiB", "KiB", "Cpu")): + in_process_block = False + continue + + if not stripped: + continue + + parts = stripped.split() + if len(parts) <= max(cpu_col, mem_col): + continue + + try: + current_snapshot_cpu += float(parts[cpu_col]) + all_mem.append(float(parts[mem_col])) + has_rows_in_snapshot = True + except ValueError: + log.debug("Skipping unparseable line in %s: %s", file_path.name, stripped) + + # Commit the final snapshot if the file didn't end with a boundary line + if has_rows_in_snapshot: + snapshot_cpu_totals.append(current_snapshot_cpu) + + return snapshot_cpu_totals, all_mem diff --git a/post_processing/run_results/run_result.py b/post_processing/run_results/run_result.py index f03c6034..5bc9ed35 100644 --- a/post_processing/run_results/run_result.py +++ b/post_processing/run_results/run_result.py @@ -24,7 +24,7 @@ log: Logger = getLogger("formatter") -class RunResult(ABC): +class RunResult(ABC): # pylint: disable=too-many-instance-attributes """ A result run file that needs processing """ @@ -38,6 +38,15 @@ def __init__(self, directory: Path, file_name_root: str, include_timeseries: boo self._processed_data: InternalFormattedOutputType = {} self._timeseries_data: dict[str, TimeSeriesFormatType] = {} self._timeseries_by_directory: dict[Path, dict[str, TimeSeriesFormatType]] = {} + # Tracks how many volume files have been merged per test configuration key + # (operation, blocksize, iodepth, numjobs) — used by _merge_resource_data. + self._resource_volume_counts: dict[tuple[str, str, str, str], int] = {} + # Tracks how many non-zero contributions have been seen per + # (test_config, source) pair for shared-directory sources (collectl, top). + # Only incremented when the source returned a non-zero value, so that + # volumes whose monitoring directory is absent (returning 0.00) are not + # counted in the denominator and do not dilute the running average. + self._resource_source_counts: dict[tuple[tuple[str, str, str, str], str], int] = {} @abstractmethod def _find_files_for_testrun(self, file_name_root: str) -> list[Path]: @@ -385,6 +394,12 @@ def _write_and_clear_timeseries_by_directory(self) -> None: self._timeseries_by_directory.clear() log.debug("Cleared timeseries data from memory") + # Sources where each volume file yields an independent CPU measurement that + # should be summed to get the total load across all volumes. All other + # sources (collectl, top) capture a shared system view from one directory + # that is read once per volume file, so they are averaged instead. + _SUMMED_RESOURCE_SOURCES: frozenset[str] = frozenset({"fio"}) + def _collect_multi_source_resources(self, resources: list[ResourceResult]) -> dict[str, dict[str, str]]: """ Collect resource data from multiple sources into nested dict format. @@ -407,6 +422,100 @@ def _collect_multi_source_resources(self, resources: list[ResourceResult]) -> di return {"cpu": cpu_data, "memory": memory_data} + def _aggregate_metric(self, source: str, prev: float, new: float, source_count: int) -> str: + """ + Aggregate a single CPU or memory metric value for one source across volumes. + + Args: + source: Resource source identifier (e.g. "fio", "collectl", "top") + prev: Previously accumulated value + new: New value from the current volume + source_count: Number of non-zero contributions already accumulated + for this source (used as denominator for shared-directory + sources; ignored for summed sources). + + Returns: + Aggregated value as a formatted string + """ + if source in self._SUMMED_RESOURCE_SOURCES: + return f"{prev + new:.2f}" + # Shared-directory source: running_avg = (prev * n + new) / (n + 1) + return f"{(prev * source_count + new) / (source_count + 1):.2f}" + + def _get_existing_iodepth(self, test_config: tuple[str, str, str, str]) -> Optional[Union[str, IodepthDataType]]: + """ + Look up the already-stored iodepth entry for a test configuration. + + Args: + test_config: Tuple of (operation, blocksize, iodepth, number_of_jobs) + + Returns: + The stored iodepth data dict, a bare str, or None if not yet stored + """ + operation, blocksize, iodepth, number_of_jobs = test_config + return self._processed_data.get(operation, {}).get(number_of_jobs, {}).get(blocksize, {}).get(iodepth) + + def _merge_resource_data( + self, + test_config: tuple[str, str, str, str], + new_resource_data: dict[str, dict[str, str]], + ) -> dict[str, dict[str, str]]: + """ + Merge new resource data with any previously stored data for the same test + configuration, aggregating correctly across multiple volumes. + + Aggregation strategy per source: + - FIO: each volume file contains an independent per-job CPU measurement, + so values are **summed** across volumes. + - collectl / top: both point at a shared directory alongside the benchmark + files, so every volume file in the same directory reads the same data. + Values are **averaged** (divide running sum by volume count) so the + final figure is not artificially inflated. + + Args: + test_config: Tuple of (operation, blocksize, iodepth, number_of_jobs) + new_resource_data: Resource data from the current volume file + + Returns: + Merged resource data dict with the same structure as the input + """ + if test_config not in self._resource_volume_counts: + # First volume — nothing to merge yet + return new_resource_data + + existing_iodepth = self._get_existing_iodepth(test_config) + if not isinstance(existing_iodepth, dict): + return new_resource_data + + raw_cpu = existing_iodepth.get("cpu", "") + raw_mem = existing_iodepth.get("memory", "") + existing_cpu: dict[str, str] = raw_cpu if isinstance(raw_cpu, dict) else {} + existing_mem: dict[str, str] = raw_mem if isinstance(raw_mem, dict) else {} + new_cpu_map = new_resource_data.get("cpu", {}) + new_mem_map = new_resource_data.get("memory", {}) + merged_cpu: dict[str, str] = {} + merged_mem: dict[str, str] = {} + + for source in set(existing_cpu) | set(new_cpu_map): + new_cpu_val = float(new_cpu_map.get(source, "0.00")) + new_mem_val = float(new_mem_map.get(source, "0.00")) + source_count: int = self._resource_source_counts.get((test_config, source), 0) + + if source not in self._SUMMED_RESOURCE_SOURCES and new_cpu_val == 0.0 and new_mem_val == 0.0: + # Missing monitoring directory — keep the accumulated value unchanged + # and do not increment _resource_source_counts for this source. + merged_cpu[source] = existing_cpu.get(source, "0.00") + merged_mem[source] = existing_mem.get(source, "0.00") + else: + merged_cpu[source] = self._aggregate_metric( + source, float(existing_cpu.get(source, "0.00")), new_cpu_val, source_count + ) + merged_mem[source] = self._aggregate_metric( + source, float(existing_mem.get(source, "0.00")), new_mem_val, source_count + ) + + return {"cpu": merged_cpu, "memory": merged_mem} + def _convert_file(self, file_path: Path) -> None: """ Convert an individual benchmark result file to the common intermediate format. @@ -416,6 +525,9 @@ def _convert_file(self, file_path: Path) -> None: operation type, blocksize, and IO depth. Now supports multiple resource sources (FIO, Collectl, etc.) simultaneously. + Resource values are aggregated across multiple volumes: sources that report + independent per-volume measurements (e.g. FIO) are summed; sources that + capture a shared system view (e.g. collectl, top) are averaged. If include_timeseries is True, also extracts time-series data from log files. @@ -438,8 +550,10 @@ def _convert_file(self, file_path: Path) -> None: # Merge IO details with existing data if present io_details = self._merge_io_details(test_config, io.io_details) - # Collect resource data from all sources + # Collect resource data from all sources then merge with any + # previously accumulated data for the same test configuration resource_data = self._collect_multi_source_resources(resources) + resource_data = self._merge_resource_data(test_config, resource_data) # Build complete test result data structure numjobs_details = self._build_test_result_data(test_config, io_details, io.global_options, resource_data) @@ -447,6 +561,17 @@ def _convert_file(self, file_path: Path) -> None: # Update internal processed data self._update_processed_data(test_config, numjobs_details) + # Increment the per-configuration volume counter (all sources). + self._resource_volume_counts[test_config] = self._resource_volume_counts.get(test_config, 0) + 1 + + # Increment the per-(config, source) counter only for non-zero + # shared-directory sources, so that volumes whose monitoring + # directory is absent are not counted in the averaging denominator. + for source, cpu_val in resource_data.get("cpu", {}).items(): + if source not in self._SUMMED_RESOURCE_SOURCES and float(cpu_val) != 0.0: + key = (test_config, source) + self._resource_source_counts[key] = self._resource_source_counts.get(key, 0) + 1 + # Process time-series data if requested if self._include_timeseries: self._process_timeseries_data(test_config, io) diff --git a/tests/test_cpu_plotter.py b/tests/test_cpu_plotter.py index d396d631..b98c6e18 100644 --- a/tests/test_cpu_plotter.py +++ b/tests/test_cpu_plotter.py @@ -71,6 +71,9 @@ def test_plot_legacy_single_source(self) -> None: # Should set y_label self.mock_twin_axes.set_ylabel.assert_called_once_with(CPU_Y_LABEL) + # Y-axis should always be fixed 0-100% + self.mock_twin_axes.set_ylim.assert_called_once_with(0, 100) + # Should call plot on twin axes self.mock_twin_axes.plot.assert_called_once() @@ -88,11 +91,13 @@ def test_plot_multi_source(self) -> None: # Should set y_label self.mock_twin_axes.set_ylabel.assert_called_once_with(CPU_Y_LABEL) + # Y-axis should always be fixed 0-100% + self.mock_twin_axes.set_ylim.assert_called_once_with(0, 100) + # Should call plot twice (once per source) self.assertEqual(self.mock_twin_axes.plot.call_count, 2) - - # Should add legend for multiple sources - self.mock_twin_axes.legend.assert_called_once() + # Should not add inline legend (legend appears below the plot) + self.mock_twin_axes.legend.assert_not_called() def test_cpu_constants(self) -> None: """Test CPU plotter constants""" diff --git a/tests/test_fio_resource_result.py b/tests/test_fio_resource_result.py index 4740cfda..8d5a41d0 100644 --- a/tests/test_fio_resource_result.py +++ b/tests/test_fio_resource_result.py @@ -12,6 +12,7 @@ import tempfile import unittest from pathlib import Path +from unittest import mock from post_processing.run_results.resources.fio_resource import FIOResource @@ -46,13 +47,15 @@ def test_get_resource_output_file_from_file_path(self) -> None: self.assertEqual(resource._resource_file_path, self.test_file) def test_parse_cpu_usage(self) -> None: - """Test parsing CPU usage from FIO output""" - resource = FIOResource(self.test_file) + """Test CPU usage is normalised by cpu_count. - cpu = resource.cpu + sys_cpu (25.5) + usr_cpu (30.2) = 55.7 per-core; divided by 4 = 13.925 + """ + with mock.patch("post_processing.run_results.resources.fio_resource.os.cpu_count", return_value=4): + resource = FIOResource(self.test_file) + cpu = resource.cpu - # Should be sum of sys_cpu (25.5) and usr_cpu (30.2) = 55.7 - self.assertAlmostEqual(float(cpu), 55.7, places=1) + self.assertAlmostEqual(float(cpu), 55.7 / 4, places=3) def test_parse_memory_usage(self) -> None: """Test parsing memory usage (currently returns 0)""" @@ -65,14 +68,14 @@ def test_parse_memory_usage(self) -> None: def test_get_method(self) -> None: """Test get method returns formatted resource data""" - resource = FIOResource(self.test_file) - - data = resource.get() + with mock.patch("post_processing.run_results.resources.fio_resource.os.cpu_count", return_value=4): + resource = FIOResource(self.test_file) + data = resource.get() self.assertEqual(data["source"], "fio") self.assertIn("cpu", data) self.assertIn("memory", data) - self.assertAlmostEqual(float(data["cpu"]), 55.7, places=1) + self.assertAlmostEqual(float(data["cpu"]), 55.7 / 4, places=3) # Made with Bob diff --git a/tests/test_io_plotter.py b/tests/test_io_plotter.py index 289f5ca5..42d627f7 100644 --- a/tests/test_io_plotter.py +++ b/tests/test_io_plotter.py @@ -71,7 +71,8 @@ def test_plot_with_error_bars(self) -> None: # Verify error bars self.assertEqual(list(call_args[1]["yerr"]), error_data) self.assertEqual(call_args[1]["capsize"], cap_size) - self.assertEqual(call_args[1]["ecolor"], "red") + self.assertEqual(call_args[1]["color"], IO_PLOT_DEFAULT_COLOUR) + self.assertEqual(call_args[1]["ecolor"], "xkcd:red") def test_plot_with_error_bars_no_caps(self) -> None: """Test plotting with cap_size=0 (no error bar caps)""" @@ -87,7 +88,7 @@ def test_plot_with_error_bars_no_caps(self) -> None: def test_io_constants(self) -> None: """Test IO plotter constants""" - self.assertEqual(IO_PLOT_DEFAULT_COLOUR, "xkcd:leaf green") + self.assertEqual(IO_PLOT_DEFAULT_COLOUR, "xkcd:cerulean") self.assertEqual(IO_Y_LABEL, "Latency (ms)") self.assertEqual(IO_PLOT_LABEL, "IO Details") diff --git a/tests/test_monitoring_base.py b/tests/test_monitoring_base.py index 269f8e95..cca439b6 100644 --- a/tests/test_monitoring_base.py +++ b/tests/test_monitoring_base.py @@ -7,7 +7,7 @@ import pytest -from monitoring.base import Monitoring +from monitoring.monitoring import Monitoring class MonitoringSubclass(Monitoring): @@ -34,7 +34,7 @@ def stop(self, directory: Optional[str]) -> None: def test_init_uses_explicit_nodes() -> None: """Use configured nodes when they are provided in monitoring config.""" - with patch("monitoring.base.settings") as mock_settings: + with patch("monitoring.monitoring.settings") as mock_settings: mock_settings.getnodes.return_value = "node1,node2" monitor = MonitoringSubclass({"nodes": ["clients", "mons"]}) @@ -45,7 +45,7 @@ def test_init_uses_explicit_nodes() -> None: def test_init_falls_back_to_default_nodes() -> None: """Use subclass default nodes when config does not provide nodes.""" - with patch("monitoring.base.settings") as mock_settings: + with patch("monitoring.monitoring.settings") as mock_settings: mock_settings.getnodes.return_value = "osd-node" monitor = MonitoringSubclass({}) @@ -56,7 +56,7 @@ def test_init_falls_back_to_default_nodes() -> None: def test_init_calls_settings_getnodes_with_resolved_nodes() -> None: """Pass the resolved node groups to settings.getnodes.""" - with patch("monitoring.base.settings") as mock_settings: + with patch("monitoring.monitoring.settings") as mock_settings: mock_settings.getnodes.return_value = "resolved-nodes" MonitoringSubclass({"nodes": ["rgws"]}) @@ -66,7 +66,7 @@ def test_init_calls_settings_getnodes_with_resolved_nodes() -> None: def test_missing_default_nodes_raises_attribute_error() -> None: """Raise an attribute error when a subclass omits default nodes.""" - with patch("monitoring.base.settings") as mock_settings: + with patch("monitoring.monitoring.settings") as mock_settings: mock_settings.getnodes.return_value = "unused" with pytest.raises(AttributeError): diff --git a/tests/test_monitoring_blktrace.py b/tests/test_monitoring_blktrace.py index 7098a1f0..5a53c8d9 100644 --- a/tests/test_monitoring_blktrace.py +++ b/tests/test_monitoring_blktrace.py @@ -18,7 +18,7 @@ def _make_monitor( if mconfig is None: mconfig = {} with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.blktrace_monitoring.settings") as mock_settings, patch("monitoring.blktrace_monitoring.common.pdsh"), ): @@ -50,7 +50,7 @@ def test_start_creates_directory_and_starts_traces() -> None: trace_runner = MagicMock() with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.blktrace_monitoring.settings") as mock_settings, patch("monitoring.blktrace_monitoring.common.pdsh") as mock_pdsh, ): diff --git a/tests/test_monitoring_collectl.py b/tests/test_monitoring_collectl.py index 24508051..e5aa1d81 100644 --- a/tests/test_monitoring_collectl.py +++ b/tests/test_monitoring_collectl.py @@ -9,7 +9,7 @@ def test_init_sets_default_args() -> None: """Use the default collectl argument string when args are not configured.""" - with patch("monitoring.base.settings") as mock_settings: + with patch("monitoring.monitoring.settings") as mock_settings: mock_settings.getnodes.return_value = "resolved-nodes" monitor = CollectlMonitoring({}) @@ -19,7 +19,7 @@ def test_init_sets_default_args() -> None: def test_init_uses_custom_args() -> None: """Use custom collectl args from monitoring config when provided.""" - with patch("monitoring.base.settings") as mock_settings: + with patch("monitoring.monitoring.settings") as mock_settings: mock_settings.getnodes.return_value = "resolved-nodes" monitor = CollectlMonitoring({"args": "--custom {collectl_dir}"}) @@ -31,7 +31,7 @@ def test_start_creates_directory_and_starts_collectl() -> None: """Create the collectl directory and invoke collectl through pdsh.""" mkdir_runner = MagicMock() with ( - patch("monitoring.base.settings") as mock_settings, + patch("monitoring.monitoring.settings") as mock_settings, patch("monitoring.collectl_monitoring.common.pdsh") as mock_pdsh, ): mock_settings.getnodes.return_value = "resolved-nodes" @@ -52,7 +52,7 @@ def test_stop_calls_pdsh_with_collectl_pkill() -> None: """Stop collectl processes through pdsh.""" stop_runner = MagicMock() with ( - patch("monitoring.base.settings") as mock_settings, + patch("monitoring.monitoring.settings") as mock_settings, patch("monitoring.collectl_monitoring.common.pdsh", return_value=stop_runner) as mock_pdsh, ): mock_settings.getnodes.return_value = "resolved-nodes" diff --git a/tests/test_monitoring_factory.py b/tests/test_monitoring_factory.py index e3bf1fea..4746bf86 100644 --- a/tests/test_monitoring_factory.py +++ b/tests/test_monitoring_factory.py @@ -10,8 +10,8 @@ from monitoring.blktrace_monitoring import BlktraceMonitoring from monitoring.collectl_monitoring import CollectlMonitoring from monitoring.monitoring_factory import MonitoringFactory -from monitoring.perf_monitoring import PerfMonitoring -from monitoring.top_monitoring import TopMonitoring +from monitoring.perf_monitoring import OsdPerfMonitoring, PerfMonitoring +from monitoring.top_monitoring import OsdTopMonitoring, TopMonitoring # --------------------------------------------------------------------------- # Helpers @@ -38,7 +38,7 @@ def _stub_monitor() -> MagicMock: def test_get_object_returns_collectl_monitoring() -> None: """get_object('collectl') returns a CollectlMonitoring instance.""" - with patch("monitoring.base.settings") as mock_base_settings: + with patch("monitoring.monitoring.settings") as mock_base_settings: mock_base_settings.getnodes.return_value = "node1" instance = MonitoringFactory.get_object("collectl", {}) assert isinstance(instance, CollectlMonitoring) @@ -47,7 +47,7 @@ def test_get_object_returns_collectl_monitoring() -> None: def test_get_object_returns_perf_monitoring() -> None: """get_object('perf') returns a PerfMonitoring instance.""" with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.perf_monitoring.settings") as mock_settings, ): mock_base_settings.getnodes.return_value = "node1" @@ -59,7 +59,7 @@ def test_get_object_returns_perf_monitoring() -> None: def test_get_object_returns_blktrace_monitoring() -> None: """get_object('blktrace') returns a BlktraceMonitoring instance.""" with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.blktrace_monitoring.settings") as mock_settings, ): mock_base_settings.getnodes.return_value = "node1" @@ -71,7 +71,7 @@ def test_get_object_returns_blktrace_monitoring() -> None: def test_get_object_returns_top_monitoring() -> None: """get_object('top') returns a TopMonitoring instance.""" with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.top_monitoring.settings") as mock_settings, ): mock_base_settings.getnodes.return_value = "node1" @@ -80,6 +80,30 @@ def test_get_object_returns_top_monitoring() -> None: assert isinstance(instance, TopMonitoring) +def test_get_object_returns_osd_top_monitoring() -> None: + """get_object('osd_top') returns an OsdTopMonitoring instance.""" + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.top_monitoring.settings") as mock_settings, + ): + mock_base_settings.getnodes.return_value = "node1" + mock_settings.cluster.get.return_value = "dummy" + instance = MonitoringFactory.get_object("osd_top", {}) + assert isinstance(instance, OsdTopMonitoring) + + +def test_get_object_returns_osd_perf_monitoring() -> None: + """get_object('osd_perf') returns an OsdPerfMonitoring instance.""" + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + ): + mock_base_settings.getnodes.return_value = "node1" + mock_settings.cluster.get.return_value = "dummy" + instance = MonitoringFactory.get_object("osd_perf", {}) + assert isinstance(instance, OsdPerfMonitoring) + + def test_get_object_raises_for_unknown_key() -> None: """get_object() raises ValueError for an unrecognised backend name.""" with pytest.raises(ValueError, match="Unknown monitoring backend: 'bogus'"): diff --git a/tests/test_monitoring_perf.py b/tests/test_monitoring_perf.py index aece298d..4df43d60 100644 --- a/tests/test_monitoring_perf.py +++ b/tests/test_monitoring_perf.py @@ -2,132 +2,154 @@ # pylint: disable=protected-access +from typing import Any, Optional from unittest.mock import MagicMock, mock_open, patch -from monitoring.perf_monitoring import PerfMonitoring +from monitoring.perf_monitoring import OsdPerfMonitoring, PerfMonitoring +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- -def test_start_local_node_runs_perf_for_each_pid() -> None: - """Start perf locally for each matching pid file.""" +_ARGS = "stat -p {pid} -o {perf_dir}/perf_stat.{pid}" + + +def _make_perf_monitor( + user: str = "ceph", + mconfig: Optional[dict[str, Any]] = None, +) -> PerfMonitoring: + """Construct a PerfMonitoring instance with mocked settings.""" + if mconfig is None: + mconfig = {"args": _ARGS} + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: {"user": user}.get(key, default) + return PerfMonitoring(mconfig) + + +def _make_osd_perf_monitor( + pid_dir: str = "/var/run/ceph", + user: str = "ceph", + mconfig: Optional[dict[str, Any]] = None, +) -> OsdPerfMonitoring: + """Construct an OsdPerfMonitoring instance with mocked settings.""" + if mconfig is None: + mconfig = {"args": _ARGS} + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": pid_dir, + "user": user, + }.get(key, default) + return OsdPerfMonitoring(mconfig) + + +# --------------------------------------------------------------------------- +# PerfMonitoring +# --------------------------------------------------------------------------- + + +def test_perf_default_nodes_is_osds() -> None: + """PerfMonitoring.DEFAULT_NODES must be ['osds'].""" + assert PerfMonitoring.DEFAULT_NODES == ["osds"] + + +def test_perf_init_stores_user_and_defaults() -> None: + """PerfMonitoring.__init__ reads user from settings and stores perf_cmd.""" + monitor = _make_perf_monitor(user="admin") + assert monitor._user == "admin" + assert monitor._perf_cmd == "sudo perf" + + +def test_perf_has_no_pid_dir_or_pid_glob() -> None: + """PerfMonitoring must not have _pid_dir or _pid_glob attributes.""" + monitor = _make_perf_monitor() + assert not hasattr(monitor, "_pid_dir") + assert not hasattr(monitor, "_pid_glob") + + +def test_perf_start_local_node_runs_perf() -> None: + """PerfMonitoring.start() runs a single perf command locally.""" mkdir_runner = MagicMock() local_runner = MagicMock() with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.perf_monitoring.settings") as mock_settings, patch("monitoring.perf_monitoring.common.pdsh", return_value=mkdir_runner) as mock_pdsh, patch("monitoring.perf_monitoring.common.get_localnode", return_value="node1"), patch("monitoring.perf_monitoring.common.sh", return_value=local_runner) as mock_sh, - patch("monitoring.perf_monitoring.glob.glob", return_value=["/var/run/ceph/osd.1.pid"]), - patch("builtins.open", mock_open(read_data="123\n")), ): mock_base_settings.getnodes.return_value = "resolved-nodes" - mock_settings.cluster.get.side_effect = lambda key, default=None: { - "pid_dir": "/var/run/ceph", - "user": "ceph", - }.get(key, default) - monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + mock_settings.cluster.get.side_effect = lambda key, default=None: {"user": "ceph"}.get(key, default) + monitor = PerfMonitoring({"args": "stat -o {perf_dir}/perf_stat.out"}) monitor.start("/tmp/output") mock_pdsh.assert_called_once_with("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/perf") mkdir_runner.communicate.assert_called_once_with() - mock_sh.assert_called_once_with("node1", "sudo perf stat -p 123 -o /tmp/output/perf/perf_stat.123 &") + mock_sh.assert_called_once_with("node1", "sudo perf stat -o /tmp/output/perf/perf_stat.out &") assert monitor._perf_runners == [local_runner] assert monitor._perf_dir == "/tmp/output/perf" -def test_start_remote_node_uses_pdsh_loop() -> None: - """Start perf remotely through pdsh when no local node is available.""" +def test_perf_start_remote_node_uses_pdsh() -> None: + """PerfMonitoring.start() dispatches a single pdsh command for remote nodes.""" mkdir_runner = MagicMock() remote_runner = MagicMock() with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.perf_monitoring.settings") as mock_settings, patch("monitoring.perf_monitoring.common.pdsh") as mock_pdsh, patch("monitoring.perf_monitoring.common.get_localnode", return_value=None), ): mock_base_settings.getnodes.return_value = "resolved-nodes" - mock_settings.cluster.get.side_effect = lambda key, default=None: { - "pid_dir": "/var/run/ceph", - "user": "ceph", - }.get(key, default) + mock_settings.cluster.get.side_effect = lambda key, default=None: {"user": "ceph"}.get(key, default) mock_pdsh.side_effect = [mkdir_runner, remote_runner] - monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) - - monitor.start("/tmp/output") + PerfMonitoring({"args": "stat -o {perf_dir}/perf_stat.out"}).start("/tmp/output") mock_pdsh.assert_any_call("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/perf") - mock_pdsh.assert_any_call( - "resolved-nodes", - [ - "for pid in `cat /var/run/ceph/osd.*.pid`;", - "do", - "sudo perf stat -p ${pid} -o /tmp/output/perf/perf_stat.${pid} &", - ";", - "done", - ], - ) + mock_pdsh.assert_any_call("resolved-nodes", "sudo perf stat -o /tmp/output/perf/perf_stat.out &") -def test_stop_kills_local_perf_runners() -> None: - """Kill locally started perf runners when present.""" +def test_perf_stop_kills_local_runners() -> None: + """PerfMonitoring.stop() kills locally started runners when present.""" runner = MagicMock() - with ( - patch("monitoring.base.settings") as mock_base_settings, - patch("monitoring.perf_monitoring.settings") as mock_settings, - ): - mock_base_settings.getnodes.return_value = "resolved-nodes" - mock_settings.cluster.get.side_effect = lambda key, default=None: { - "pid_dir": "/var/run/ceph", - "user": "ceph", - }.get(key, default) - monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) - monitor._perf_runners = [runner] + monitor = _make_perf_monitor() + monitor._perf_runners = [runner] + with patch("monitoring.perf_monitoring.common.pdsh"): monitor.stop(None) runner.kill.assert_called_once_with() -def test_stop_uses_pdsh_when_no_local_runners_exist() -> None: - """Stop perf remotely through pdsh when no local runners are tracked.""" +def test_perf_stop_uses_pdsh_when_no_local_runners() -> None: + """PerfMonitoring.stop() issues pkill via pdsh when no local runners are tracked.""" stop_runner = MagicMock() - with ( - patch("monitoring.base.settings") as mock_base_settings, - patch("monitoring.perf_monitoring.settings") as mock_settings, - patch("monitoring.perf_monitoring.common.pdsh", return_value=stop_runner) as mock_pdsh, - ): - mock_base_settings.getnodes.return_value = "resolved-nodes" - mock_settings.cluster.get.side_effect = lambda key, default=None: { - "pid_dir": "/var/run/ceph", - "user": "ceph", - }.get(key, default) - monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + monitor = _make_perf_monitor() + with patch("monitoring.perf_monitoring.common.pdsh", return_value=stop_runner) as mock_pdsh: monitor.stop(None) mock_pdsh.assert_called_once_with("resolved-nodes", r"sudo pkill -SIGINT -f perf\ ") stop_runner.communicate.assert_called_once_with() -def test_stop_chowns_output_files_when_directory_is_provided() -> None: - """Adjust ownership of generated perf files when an output directory is given.""" +def test_perf_stop_chowns_output_files_when_directory_provided() -> None: + """PerfMonitoring.stop() adjusts ownership of generated perf files when a directory is given.""" stop_runner = MagicMock() chown_data_runner = MagicMock() chown_stat_runner = MagicMock() - with ( - patch("monitoring.base.settings") as mock_base_settings, - patch("monitoring.perf_monitoring.settings") as mock_settings, - patch("monitoring.perf_monitoring.common.pdsh") as mock_pdsh, - ): - mock_base_settings.getnodes.return_value = "resolved-nodes" - mock_settings.cluster.get.side_effect = lambda key, default=None: { - "pid_dir": "/var/run/ceph", - "user": "ceph", - }.get(key, default) - mock_pdsh.side_effect = [stop_runner, chown_data_runner, chown_stat_runner] - monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + monitor = _make_perf_monitor() + with patch("monitoring.perf_monitoring.common.pdsh") as mock_pdsh: + mock_pdsh.side_effect = [stop_runner, chown_data_runner, chown_stat_runner] monitor.stop("/tmp/output") mock_pdsh.assert_any_call("resolved-nodes", r"sudo pkill -SIGINT -f perf\ ") @@ -135,10 +157,10 @@ def test_stop_chowns_output_files_when_directory_is_provided() -> None: mock_pdsh.assert_any_call("resolved-nodes", "sudo chown ceph.ceph /tmp/output/perf/perf_stat.*") -def test_get_cpu_cycles_returns_total_cycles() -> None: - """Sum cycle counts from all perf stat output files.""" +def test_perf_get_cpu_cycles_returns_total_cycles() -> None: + """PerfMonitoring.get_cpu_cycles() sums cycle counts from all perf stat output files.""" with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.perf_monitoring.settings") as mock_settings, patch("monitoring.perf_monitoring.glob.glob", return_value=["/tmp/output/perf"]), patch("monitoring.perf_monitoring.os.listdir", return_value=["perf_stat.1", "perf_stat.2"]), @@ -151,29 +173,116 @@ def test_get_cpu_cycles_returns_total_cycles() -> None: ), ): mock_base_settings.getnodes.return_value = "resolved-nodes" - mock_settings.cluster.get.side_effect = lambda key, default=None: { - "pid_dir": "/var/run/ceph", - "user": "ceph", - }.get(key, default) - monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + mock_settings.cluster.get.side_effect = lambda key, default=None: {"user": "ceph"}.get(key, default) + monitor = PerfMonitoring({"args": _ARGS}) assert monitor.get_cpu_cycles("/tmp/output") == 3500 -def test_get_cpu_cycles_returns_none_when_cycles_are_missing() -> None: - """Return None when perf output does not contain a cycles line.""" +def test_perf_get_cpu_cycles_returns_none_when_cycles_missing() -> None: + """PerfMonitoring.get_cpu_cycles() returns None when perf output has no cycles line.""" with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.perf_monitoring.settings") as mock_settings, patch("monitoring.perf_monitoring.glob.glob", return_value=["/tmp/output/perf"]), patch("monitoring.perf_monitoring.os.listdir", return_value=["perf_stat.1"]), patch("builtins.open", mock_open(read_data="nothing to match\n")), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: {"user": "ceph"}.get(key, default) + monitor = PerfMonitoring({"args": _ARGS}) + + assert monitor.get_cpu_cycles("/tmp/output") is None + + +# --------------------------------------------------------------------------- +# OsdPerfMonitoring +# --------------------------------------------------------------------------- + + +def test_osd_perf_default_nodes_is_osds() -> None: + """OsdPerfMonitoring.DEFAULT_NODES must be ['osds'] (inherited).""" + assert OsdPerfMonitoring.DEFAULT_NODES == ["osds"] + + +def test_osd_perf_is_subclass_of_perf_monitoring() -> None: + """OsdPerfMonitoring must be a subclass of PerfMonitoring.""" + assert issubclass(OsdPerfMonitoring, PerfMonitoring) + + +def test_osd_perf_init_stores_pid_dir_and_glob() -> None: + """OsdPerfMonitoring.__init__ reads pid_dir from cluster settings and sets pid_glob.""" + monitor = _make_osd_perf_monitor(pid_dir="/run/ceph") + assert monitor._pid_dir == "/run/ceph" + assert monitor._pid_glob == "osd.*.pid" + + +def test_osd_perf_init_accepts_custom_pid_glob() -> None: + """OsdPerfMonitoring accepts a custom pid_glob from mconfig.""" + monitor = _make_osd_perf_monitor(mconfig={"args": _ARGS, "pid_glob": "*.pid"}) + assert monitor._pid_glob == "*.pid" + + +def test_osd_perf_start_local_node_runs_perf_per_pid() -> None: + """OsdPerfMonitoring.start() runs perf locally for each matching pid file.""" + mkdir_runner = MagicMock() + local_runner = MagicMock() + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + patch("monitoring.perf_monitoring.common.pdsh", return_value=mkdir_runner) as mock_pdsh, + patch("monitoring.perf_monitoring.common.get_localnode", return_value="node1"), + patch("monitoring.perf_monitoring.common.sh", return_value=local_runner) as mock_sh, + patch("monitoring.perf_monitoring.glob.glob", return_value=["/var/run/ceph/osd.1.pid"]), + patch("builtins.open", mock_open(read_data="123\n")), ): mock_base_settings.getnodes.return_value = "resolved-nodes" mock_settings.cluster.get.side_effect = lambda key, default=None: { "pid_dir": "/var/run/ceph", "user": "ceph", }.get(key, default) - monitor = PerfMonitoring({"args": "stat -p {pid} -o {perf_dir}/perf_stat.{pid}"}) + monitor = OsdPerfMonitoring({"args": _ARGS}) - assert monitor.get_cpu_cycles("/tmp/output") is None + monitor.start("/tmp/output") + + mock_pdsh.assert_called_once_with("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/perf") + mkdir_runner.communicate.assert_called_once_with() + mock_sh.assert_called_once_with("node1", "sudo perf stat -p 123 -o /tmp/output/perf/perf_stat.123 &") + assert monitor._perf_runners == [local_runner] + assert monitor._perf_dir == "/tmp/output/perf" + + +def test_osd_perf_start_remote_node_uses_pdsh_loop() -> None: + """OsdPerfMonitoring.start() dispatches via pdsh for-loop when no local node is available.""" + mkdir_runner = MagicMock() + remote_runner = MagicMock() + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.perf_monitoring.settings") as mock_settings, + patch("monitoring.perf_monitoring.common.pdsh") as mock_pdsh, + patch("monitoring.perf_monitoring.common.get_localnode", return_value=None), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + mock_pdsh.side_effect = [mkdir_runner, remote_runner] + OsdPerfMonitoring({"args": _ARGS}).start("/tmp/output") + + mock_pdsh.assert_any_call("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/perf") + mock_pdsh.assert_any_call( + "resolved-nodes", + [ + "for pid in `cat /var/run/ceph/osd.*.pid`;", + "do", + "sudo perf stat -p ${pid} -o /tmp/output/perf/perf_stat.${pid} &", + ";", + "done", + ], + ) + + +def test_osd_perf_stop_inherited_from_perf_monitoring() -> None: + """OsdPerfMonitoring inherits stop() from PerfMonitoring without override.""" + assert OsdPerfMonitoring.stop is PerfMonitoring.stop diff --git a/tests/test_monitoring_top.py b/tests/test_monitoring_top.py index 2aae8f78..f6e198b9 100644 --- a/tests/test_monitoring_top.py +++ b/tests/test_monitoring_top.py @@ -5,11 +5,14 @@ from typing import Any, Optional from unittest.mock import MagicMock, mock_open, patch -from monitoring.top_monitoring import TopMonitoring +from monitoring.top_monitoring import OsdTopMonitoring, TopMonitoring +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- -def _make_monitor( - pid_dir: str = "/var/run/ceph", + +def _make_top_monitor( user: str = "ceph", mconfig: Optional[dict[str, Any]] = None, ) -> TopMonitoring: @@ -17,113 +20,116 @@ def _make_monitor( if mconfig is None: mconfig = {} with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.top_monitoring.settings") as mock_settings, + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: {"user": user}.get(key, default) + return TopMonitoring(mconfig) + + +def _make_osd_top_monitor( + pid_dir: str = "/var/run/ceph", + user: str = "ceph", + mconfig: Optional[dict[str, Any]] = None, +) -> OsdTopMonitoring: + """Construct an OsdTopMonitoring instance with mocked settings.""" + if mconfig is None: + mconfig = {} + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.top_monitoring.settings") as mock_settings, - patch("monitoring.top_monitoring.common.pdsh"), ): mock_base_settings.getnodes.return_value = "resolved-nodes" mock_settings.cluster.get.side_effect = lambda key, default=None: { "pid_dir": pid_dir, "user": user, }.get(key, default) - return TopMonitoring(mconfig) + return OsdTopMonitoring(mconfig) -def test_default_nodes_is_osds() -> None: - """DEFAULT_NODES must be ['osds'].""" - assert TopMonitoring.DEFAULT_NODES == ["osds"] +# --------------------------------------------------------------------------- +# TopMonitoring +# --------------------------------------------------------------------------- -def test_init_stores_cluster_settings() -> None: - """__init__ reads pid_dir and user from settings.cluster.""" - monitor = _make_monitor(pid_dir="/run/ceph", user="admin") - assert monitor._pid_dir == "/run/ceph" - assert monitor._user == "admin" +def test_top_default_nodes_is_osds() -> None: + """TopMonitoring.DEFAULT_NODES must be ['osds'].""" + assert TopMonitoring.DEFAULT_NODES == ["osds"] -def test_init_stores_default_args() -> None: - """__init__ uses the default top argument string when mconfig has no 'args'.""" - monitor = _make_monitor() +def test_top_init_stores_user_and_defaults() -> None: + """TopMonitoring.__init__ reads user from settings and applies default args.""" + monitor = _make_top_monitor(user="admin") + assert monitor._user == "admin" assert monitor._top_cmd == "top" - assert monitor._pid_glob == "osd.*.pid" - assert "{pid}" in monitor._args assert "{top_dir}" in monitor._args + assert "{pid}" not in monitor._args -def test_init_accepts_custom_args() -> None: - """__init__ accepts custom top_cmd, args, and pid_glob from mconfig.""" - monitor = _make_monitor(mconfig={"top_cmd": "htop", "args": "-d 1", "pid_glob": "*.pid"}) +def test_top_init_accepts_custom_args() -> None: + """TopMonitoring accepts custom top_cmd and args from mconfig.""" + monitor = _make_top_monitor(mconfig={"top_cmd": "htop", "args": "-d 1 > {top_dir}/out.txt"}) assert monitor._top_cmd == "htop" - assert monitor._args == "-d 1" - assert monitor._pid_glob == "*.pid" + assert monitor._args == "-d 1 > {top_dir}/out.txt" -def test_start_local_node_runs_top_for_each_pid() -> None: - """start() runs top locally for each matching pid file.""" +def test_top_has_no_pid_dir_or_pid_glob() -> None: + """TopMonitoring must not have _pid_dir or _pid_glob attributes.""" + monitor = _make_top_monitor() + assert not hasattr(monitor, "_pid_dir") + assert not hasattr(monitor, "_pid_glob") + + +def test_top_start_local_node_runs_top() -> None: + """TopMonitoring.start() runs top locally without per-pid iteration.""" mkdir_runner = MagicMock() local_runner = MagicMock() with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.top_monitoring.settings") as mock_settings, patch("monitoring.top_monitoring.common.pdsh", return_value=mkdir_runner) as mock_pdsh, patch("monitoring.top_monitoring.common.get_localnode", return_value="node1"), patch("monitoring.top_monitoring.common.sh", return_value=local_runner) as mock_sh, - patch("monitoring.top_monitoring.glob.glob", return_value=["/var/run/ceph/osd.1.pid"]), - patch("builtins.open", mock_open(read_data="42\n")), ): mock_base_settings.getnodes.return_value = "resolved-nodes" - mock_settings.cluster.get.side_effect = lambda key, default=None: { - "pid_dir": "/var/run/ceph", - "user": "ceph", - }.get(key, default) + mock_settings.cluster.get.side_effect = lambda key, default=None: {"user": "ceph"}.get(key, default) monitor = TopMonitoring({}) monitor.start("/tmp/output") mock_pdsh.assert_called_once_with("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/top") mkdir_runner.communicate.assert_called_once_with() - expected_cmd = "top -b -H -1 -p 42 -n 30 > /tmp/output/top/42_osd_top.out" + expected_cmd = "top -b -H -1 -n 30 > /tmp/output/top/top.out" mock_sh.assert_called_once_with("node1", expected_cmd) assert monitor._top_runners == [local_runner] -def test_start_remote_node_uses_pdsh_loop() -> None: - """start() dispatches via pdsh for-loop when no local node is available.""" +def test_top_start_remote_node_uses_pdsh() -> None: + """TopMonitoring.start() dispatches a single pdsh command for remote nodes.""" mkdir_runner = MagicMock() remote_runner = MagicMock() with ( - patch("monitoring.base.settings") as mock_base_settings, + patch("monitoring.monitoring.settings") as mock_base_settings, patch("monitoring.top_monitoring.settings") as mock_settings, patch("monitoring.top_monitoring.common.pdsh") as mock_pdsh, patch("monitoring.top_monitoring.common.get_localnode", return_value=None), ): mock_base_settings.getnodes.return_value = "resolved-nodes" - mock_settings.cluster.get.side_effect = lambda key, default=None: { - "pid_dir": "/var/run/ceph", - "user": "ceph", - }.get(key, default) + mock_settings.cluster.get.side_effect = lambda key, default=None: {"user": "ceph"}.get(key, default) mock_pdsh.side_effect = [mkdir_runner, remote_runner] monitor = TopMonitoring({}) monitor.start("/tmp/output") mock_pdsh.assert_any_call("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/top") - mock_pdsh.assert_any_call( - "resolved-nodes", - [ - "for pid in `cat /var/run/ceph/osd.*.pid`;", - "do", - "top -b -H -1 -p ${pid} -n 30 > /tmp/output/top/${pid}_osd_top.out", - ";", - "done", - ], - ) + mock_pdsh.assert_any_call("resolved-nodes", "top -b -H -1 -n 30 > /tmp/output/top/top.out") -def test_stop_kills_local_top_runners() -> None: - """stop() kills locally started runners when present.""" +def test_top_stop_kills_local_runners() -> None: + """TopMonitoring.stop() kills locally started runners when present.""" runner = MagicMock() - monitor = _make_monitor() + monitor = _make_top_monitor() monitor._top_runners = [runner] with patch("monitoring.top_monitoring.common.pdsh") as mock_pdsh: @@ -133,27 +139,177 @@ def test_stop_kills_local_top_runners() -> None: mock_pdsh.assert_not_called() -def test_stop_uses_pdsh_when_no_local_runners_exist() -> None: - """stop() issues pkill via pdsh when no local runners are tracked.""" +def test_top_stop_uses_pdsh_when_no_local_runners() -> None: + """TopMonitoring.stop() issues pkill via pdsh when no local runners are tracked.""" stop_runner = MagicMock() - monitor = _make_monitor() + monitor = _make_top_monitor() + expected_pkill = f"sudo pkill -SIGINT -f '{monitor._top_cmd} {monitor._args}'" with patch("monitoring.top_monitoring.common.pdsh", return_value=stop_runner) as mock_pdsh: monitor.stop(None) - mock_pdsh.assert_called_once_with("resolved-nodes", r"sudo pkill -SIGINT -f top\ ") + mock_pdsh.assert_called_once_with("resolved-nodes", expected_pkill) stop_runner.communicate.assert_called_once_with() -def test_stop_chowns_output_files_when_directory_is_provided() -> None: - """stop() adjusts ownership of top output files when a directory is given.""" +def test_top_stop_chowns_output_files_when_directory_provided() -> None: + """TopMonitoring.stop() adjusts ownership of top output files when a directory is given.""" stop_runner = MagicMock() chown_runner = MagicMock() - monitor = _make_monitor(user="ceph") + monitor = _make_top_monitor(user="ceph") with patch("monitoring.top_monitoring.common.pdsh") as mock_pdsh: mock_pdsh.side_effect = [stop_runner, chown_runner] monitor.stop("/tmp/output") - mock_pdsh.assert_any_call("resolved-nodes", r"sudo pkill -SIGINT -f top\ ") mock_pdsh.assert_any_call("resolved-nodes", "sudo chown ceph.ceph /tmp/output/top/*top.out") + + +# --------------------------------------------------------------------------- +# OsdTopMonitoring +# --------------------------------------------------------------------------- + + +def test_osd_top_default_nodes_is_osds() -> None: + """OsdTopMonitoring.DEFAULT_NODES must be ['osds'] (inherited).""" + assert OsdTopMonitoring.DEFAULT_NODES == ["osds"] + + +def test_osd_top_is_subclass_of_top_monitoring() -> None: + """OsdTopMonitoring must be a subclass of TopMonitoring.""" + assert issubclass(OsdTopMonitoring, TopMonitoring) + + +def test_osd_top_init_stores_pid_dir_and_glob() -> None: + """OsdTopMonitoring.__init__ reads pid_dir from cluster settings and sets pid_glob.""" + monitor = _make_osd_top_monitor(pid_dir="/run/ceph") + assert monitor._pid_dir == "/run/ceph" + assert monitor._pid_glob == "osd.*.pid" + + +def test_osd_top_init_args_include_pid_placeholder() -> None: + """OsdTopMonitoring default args template must contain {pid}.""" + monitor = _make_osd_top_monitor() + assert "{pid}" in monitor._args + assert "{top_dir}" in monitor._args + + +def test_osd_top_init_accepts_custom_pid_glob() -> None: + """OsdTopMonitoring accepts a custom pid_glob from mconfig.""" + monitor = _make_osd_top_monitor(mconfig={"pid_glob": "*.pid"}) + assert monitor._pid_glob == "*.pid" + + +def test_osd_top_start_local_node_runs_top_per_pid() -> None: + """OsdTopMonitoring.start() runs top locally for each matching pid file.""" + mkdir_runner = MagicMock() + local_runner = MagicMock() + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.top_monitoring.settings") as mock_settings, + patch("monitoring.top_monitoring.common.pdsh", return_value=mkdir_runner) as mock_pdsh, + patch("monitoring.top_monitoring.common.get_localnode", return_value="node1"), + patch("monitoring.top_monitoring.common.sh", return_value=local_runner) as mock_sh, + patch("monitoring.top_monitoring.glob.glob", return_value=["/var/run/ceph/osd.1.pid"]), + patch("builtins.open", mock_open(read_data="42\n")), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + monitor = OsdTopMonitoring({}) + + monitor.start("/tmp/output") + + mock_pdsh.assert_called_once_with("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/top") + mkdir_runner.communicate.assert_called_once_with() + expected_cmd = "top -b -H -1 -p 42 -n 30 > /tmp/output/top/42_osd_top.out" + mock_sh.assert_called_once_with("node1", expected_cmd) + assert monitor._top_runners == [local_runner] + + +def test_osd_top_start_local_node_warns_when_no_pid_files() -> None: + """OsdTopMonitoring.start() logs a warning when no local PID files are found.""" + mkdir_runner = MagicMock() + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.top_monitoring.settings") as mock_settings, + patch("monitoring.top_monitoring.common.pdsh", return_value=mkdir_runner), + patch("monitoring.top_monitoring.common.get_localnode", return_value="node1"), + patch("monitoring.top_monitoring.glob.glob", return_value=[]), + patch("monitoring.top_monitoring.logger") as mock_logger, + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + OsdTopMonitoring({}).start("/tmp/output") + + warning_calls = [str(c) for c in mock_logger.warning.call_args_list] + assert any("no PID files" in c for c in warning_calls) + + +def test_osd_top_start_remote_node_uses_pdsh_loop() -> None: + """OsdTopMonitoring.start() dispatches via pdsh for-loop when no local node is available.""" + mkdir_runner = MagicMock() + ls_runner = MagicMock() + ls_runner.communicate.return_value = ("/var/run/ceph/osd.1.pid\n", "") + remote_runner = MagicMock() + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.top_monitoring.settings") as mock_settings, + patch("monitoring.top_monitoring.common.pdsh") as mock_pdsh, + patch("monitoring.top_monitoring.common.get_localnode", return_value=None), + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + mock_pdsh.side_effect = [mkdir_runner, ls_runner, remote_runner] + OsdTopMonitoring({}).start("/tmp/output") + + mock_pdsh.assert_any_call("resolved-nodes", "mkdir -p -m0755 -- /tmp/output/top") + mock_pdsh.assert_any_call("resolved-nodes", "ls /var/run/ceph/osd.*.pid 2>/dev/null") + mock_pdsh.assert_any_call( + "resolved-nodes", + [ + "for pid in `cat /var/run/ceph/osd.*.pid`;", + "do", + "top -b -H -1 -p ${pid} -n 30 > /tmp/output/top/${pid}_osd_top.out", + ";", + "done", + ], + ) + + +def test_osd_top_start_remote_node_warns_when_no_pid_files() -> None: + """OsdTopMonitoring.start() logs a warning when the remote ls finds no PID files.""" + mkdir_runner = MagicMock() + ls_runner = MagicMock() + ls_runner.communicate.return_value = ("", "") + remote_runner = MagicMock() + with ( + patch("monitoring.monitoring.settings") as mock_base_settings, + patch("monitoring.top_monitoring.settings") as mock_settings, + patch("monitoring.top_monitoring.common.pdsh") as mock_pdsh, + patch("monitoring.top_monitoring.common.get_localnode", return_value=None), + patch("monitoring.top_monitoring.logger") as mock_logger, + ): + mock_base_settings.getnodes.return_value = "resolved-nodes" + mock_settings.cluster.get.side_effect = lambda key, default=None: { + "pid_dir": "/var/run/ceph", + "user": "ceph", + }.get(key, default) + mock_pdsh.side_effect = [mkdir_runner, ls_runner, remote_runner] + OsdTopMonitoring({}).start("/tmp/output") + + warning_calls = [str(c) for c in mock_logger.warning.call_args_list] + assert any("no PID files" in c for c in warning_calls) + + +def test_osd_top_stop_inherited_from_top_monitoring() -> None: + """OsdTopMonitoring inherits stop() from TopMonitoring without override.""" + assert OsdTopMonitoring.stop is TopMonitoring.stop diff --git a/tests/test_resource_result_factory.py b/tests/test_resource_result_factory.py index 7635b1b8..4602cca2 100644 --- a/tests/test_resource_result_factory.py +++ b/tests/test_resource_result_factory.py @@ -7,10 +7,14 @@ import json import tempfile from pathlib import Path +from unittest import mock + +import pytest from post_processing.run_results.resource_result_factory import get_all_resources from post_processing.run_results.resources.collectl_resource import CollectlResource from post_processing.run_results.resources.fio_resource import FIOResource +from post_processing.run_results.resources.top_resource import TopResource class TestGetAllResourcesFIOOnly: @@ -347,4 +351,133 @@ def test_can_retrieve_data_from_all_resources(self) -> None: assert set(sources) == {"fio", "collectl"} +class TestGetAllResourcesWithTop: + """Test get_all_resources when top monitoring data is present.""" + + def _make_top_dir(self, base: Path, content: str) -> None: + top_dir = base / "top" + top_dir.mkdir() + (top_dir / "12345_osd_top.out").write_text(content) + + _TOP_CONTENT = ( + "top - 14:32:01 up 1 day\n%Cpu(s): 5.0 us\n\n" + " PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND\n" + "12345 ceph 20 0 1.0g 100m 10m S 30.0 0.5 0:01.00 crimson-osd\n" + ) + + def test_top_only_returns_fio_and_top(self) -> None: + """FIO and Top resources are returned when only a top dir is present.""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 20.0, "sys_cpu": 10.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + self._make_top_dir(base, self._TOP_CONTENT) + + resources = get_all_resources(file_path) + + assert len(resources) == 2 + sources = {r.source for r in resources} + assert sources == {"fio", "top"} + + def test_top_resource_is_top_resource_instance(self) -> None: + """The top entry in the list is a TopResource instance.""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 20.0, "sys_cpu": 10.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + self._make_top_dir(base, self._TOP_CONTENT) + + resources = get_all_resources(file_path) + + top_resources = [r for r in resources if isinstance(r, TopResource)] + assert len(top_resources) == 1 + + def test_all_three_sources_when_collectl_and_top_present(self) -> None: + """FIO, Collectl, and Top are all returned when both dirs exist.""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 20.0, "sys_cpu": 10.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + # Collectl dir + collectl_dir = base / "collectl" + collectl_dir.mkdir() + (collectl_dir / "host-20260619.cpu").write_text("#Date;Time;[CPU:0]Totl%\n20260619;17:06:30;50\n") + + # Top dir + self._make_top_dir(base, self._TOP_CONTENT) + + resources = get_all_resources(file_path) + + assert len(resources) == 3 + sources = {r.source for r in resources} + assert sources == {"fio", "collectl", "top"} + + def test_source_order_fio_collectl_top(self) -> None: + """Resources are returned in fio → collectl → top order.""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 20.0, "sys_cpu": 10.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + + collectl_dir = base / "collectl" + collectl_dir.mkdir() + (collectl_dir / "host-20260619.cpu").write_text("#Date;Time;[CPU:0]Totl%\n20260619;17:06:30;50\n") + + self._make_top_dir(base, self._TOP_CONTENT) + + resources = get_all_resources(file_path) + + assert resources[0].source == "fio" + assert resources[1].source == "collectl" + assert resources[2].source == "top" + + def test_top_dir_without_out_files_skipped(self) -> None: + """An empty top dir does not add a TopResource.""" + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 20.0, "sys_cpu": 10.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + (base / "top").mkdir() # exists but empty + + resources = get_all_resources(file_path) + + sources = {r.source for r in resources} + assert "top" not in sources + + def test_top_resource_data_retrievable(self) -> None: + """get() on the TopResource returns the expected dict structure. + + _TOP_CONTENT has one snapshot, one thread at 30.0 core-units. + Normalised by cpu_count=4: 30.0 / 4 = 7.5. + """ + fio_data = {"jobs": [{"job_name": "test", "usr_cpu": 20.0, "sys_cpu": 10.0}]} + + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.write_text(json.dumps(fio_data)) + self._make_top_dir(base, self._TOP_CONTENT) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=4): + resources = get_all_resources(file_path) + top_resource = next(r for r in resources if r.source == "top") + result = top_resource.get() + + assert result["source"] == "top" + assert "cpu" in result + assert "memory" in result + assert float(result["cpu"]) == pytest.approx(7.5) + + # Made with Bob diff --git a/tests/test_run_result.py b/tests/test_run_result.py index 7292780d..acab2e15 100644 --- a/tests/test_run_result.py +++ b/tests/test_run_result.py @@ -8,7 +8,7 @@ import json import tempfile from pathlib import Path -from unittest.mock import Mock +from unittest.mock import Mock, patch import pytest @@ -25,7 +25,7 @@ def test_initialization_basic(self): with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) result = RBDFIO(path, "json_output") - + assert result._path == path assert result._has_been_processed is False assert result._include_timeseries is False @@ -38,7 +38,7 @@ def test_initialization_with_timeseries(self): with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) result = RBDFIO(path, "json_output", include_timeseries=True) - + assert result._include_timeseries is True @@ -50,10 +50,10 @@ def test_process_with_no_files(self): with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) result = RBDFIO(path, "json_output") - + # No files created, so _files should be empty result.process() - + assert result._has_been_processed is True assert len(result._processed_data) == 0 @@ -61,7 +61,7 @@ def test_process_with_files(self): """Test process() with valid files.""" with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) - + # Create test data test_data = { "global options": { @@ -71,34 +71,36 @@ def test_process_with_files(self): "numjobs": "1", "runtime": "60", }, - "jobs": [{ - "read": { - "io_bytes": 1000000000, - "bw_bytes": 16666666, - "iops": 4000.0, - "total_ios": 244140, - "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, - }, - "write": { - "io_bytes": 0, - "bw_bytes": 0, - "iops": 0.0, - "total_ios": 0, - "clat_ns": {"mean": 0.0, "stddev": 0.0}, - }, - "sys_cpu": 5.5, - "usr_cpu": 10.2, - }], + "jobs": [ + { + "read": { + "io_bytes": 1000000000, + "bw_bytes": 16666666, + "iops": 4000.0, + "total_ios": 244140, + "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, + }, + "write": { + "io_bytes": 0, + "bw_bytes": 0, + "iops": 0.0, + "total_ios": 0, + "clat_ns": {"mean": 0.0, "stddev": 0.0}, + }, + "sys_cpu": 5.5, + "usr_cpu": 10.2, + } + ], } - + # Create test file test_file = path / "json_output.0" with open(test_file, "w") as f: json.dump(test_data, f) - + result = RBDFIO(path, "json_output") result.process() - + assert result._has_been_processed is True assert len(result._processed_data) > 0 @@ -111,11 +113,11 @@ def test_get_without_processing(self): with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) result = RBDFIO(path, "json_output") - + assert result._has_been_processed is False - + data = result.get() - + assert result._has_been_processed is True assert isinstance(data, dict) @@ -124,10 +126,10 @@ def test_get_after_processing(self): with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) result = RBDFIO(path, "json_output") - + result.process() data = result.get() - + assert isinstance(data, dict) # Tests for get_timeseries() removed - with memory-efficient approach, @@ -141,14 +143,14 @@ def test_process_empty_file(self): """Test processing skips empty files.""" with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) - + # Create empty file empty_file = path / "json_output.0" empty_file.touch() - + result = RBDFIO(path, "json_output") result.process() - + # Should complete without error, but no data processed assert result._has_been_processed is True @@ -156,7 +158,7 @@ def test_process_precondition_file(self): """Test processing skips precondition files.""" with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) - + # Create test data test_data = { "global options": { @@ -166,35 +168,37 @@ def test_process_precondition_file(self): "numjobs": "1", "runtime": "60", }, - "jobs": [{ - "jobname": "precondition", - "read": { - "io_bytes": 1000000000, - "bw_bytes": 16666666, - "iops": 4000.0, - "total_ios": 244140, - "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, - }, - "write": { - "io_bytes": 0, - "bw_bytes": 0, - "iops": 0.0, - "total_ios": 0, - "clat_ns": {"mean": 0.0, "stddev": 0.0}, - }, - "sys_cpu": 5.5, - "usr_cpu": 10.2, - }], + "jobs": [ + { + "jobname": "precondition", + "read": { + "io_bytes": 1000000000, + "bw_bytes": 16666666, + "iops": 4000.0, + "total_ios": 244140, + "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, + }, + "write": { + "io_bytes": 0, + "bw_bytes": 0, + "iops": 0.0, + "total_ios": 0, + "clat_ns": {"mean": 0.0, "stddev": 0.0}, + }, + "sys_cpu": 5.5, + "usr_cpu": 10.2, + } + ], } - + # Create precondition file precond_file = path / "json_output.0" with open(precond_file, "w") as f: json.dump(test_data, f) - + result = RBDFIO(path, "json_output") result.process() - + # Should complete without error assert result._has_been_processed is True @@ -207,16 +211,16 @@ def test_extract_configuration(self): with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) result = RBDFIO(path, "json_output") - + # Create mock benchmark result mock_benchmark = Mock(spec=BenchmarkResult) mock_benchmark.operation = "randread" mock_benchmark.blocksize = "4096" mock_benchmark.iodepth = "32" mock_benchmark.number_of_jobs = "1" - + config = result._extract_test_configuration(mock_benchmark) - + assert config == ("randread", "4096", "32", "1") @@ -228,7 +232,7 @@ def test_merge_with_no_existing_data(self): with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) result = RBDFIO(path, "json_output") - + test_config = ("randread", "4096", "32", "1") new_io_details: IodepthDataType = { "io_bytes": "1000000000", @@ -238,9 +242,9 @@ def test_merge_with_no_existing_data(self): "latency": "8.0", "std_deviation": "0.5", } - + merged = result._merge_io_details(test_config, new_io_details) - + # Should return new_io_details unchanged assert merged == new_io_details @@ -249,7 +253,7 @@ def test_merge_with_existing_data(self): with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) result = RBDFIO(path, "json_output") - + # Set up existing data test_config = ("randread", "4096", "32", "1") existing_io: IodepthDataType = { @@ -260,17 +264,9 @@ def test_merge_with_existing_data(self): "latency": "8.0", "std_deviation": "0.5", } - - result._processed_data = { - "randread": { - "1": { - "4096": { - "32": existing_io - } - } - } - } - + + result._processed_data = {"randread": {"1": {"4096": {"32": existing_io}}}} + new_io_details: IodepthDataType = { "io_bytes": "1000000000", "iops": "4000.0", @@ -279,9 +275,9 @@ def test_merge_with_existing_data(self): "latency": "8.0", "std_deviation": "0.5", } - + merged = result._merge_io_details(test_config, new_io_details) - + # Should sum the values assert float(merged["io_bytes"]) == 2000000000.0 assert float(merged["iops"]) == 8000.0 @@ -294,7 +290,7 @@ def test_convert_file_success(self): """Test successful file conversion.""" with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) - + # Create test data test_data = { "global options": { @@ -304,33 +300,35 @@ def test_convert_file_success(self): "numjobs": "1", "runtime": "60", }, - "jobs": [{ - "read": { - "io_bytes": 1000000000, - "bw_bytes": 16666666, - "iops": 4000.0, - "total_ios": 244140, - "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, - }, - "write": { - "io_bytes": 0, - "bw_bytes": 0, - "iops": 0.0, - "total_ios": 0, - "clat_ns": {"mean": 0.0, "stddev": 0.0}, - }, - "sys_cpu": 5.5, - "usr_cpu": 10.2, - }], + "jobs": [ + { + "read": { + "io_bytes": 1000000000, + "bw_bytes": 16666666, + "iops": 4000.0, + "total_ios": 244140, + "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, + }, + "write": { + "io_bytes": 0, + "bw_bytes": 0, + "iops": 0.0, + "total_ios": 0, + "clat_ns": {"mean": 0.0, "stddev": 0.0}, + }, + "sys_cpu": 5.5, + "usr_cpu": 10.2, + } + ], } - + test_file = path / "json_output.0" with open(test_file, "w") as f: json.dump(test_data, f) - + result = RBDFIO(path, "json_output") result._convert_file(test_file) - + # Should have processed data assert len(result._processed_data) > 0 @@ -338,14 +336,14 @@ def test_convert_file_with_error(self): """Test file conversion with error handling.""" with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) - + # Create invalid JSON file test_file = path / "json_output.0" with open(test_file, "w") as f: f.write("invalid json") - + result = RBDFIO(path, "json_output") - + # Should raise an exception with pytest.raises(Exception): result._convert_file(test_file) @@ -358,7 +356,7 @@ def test_full_workflow_single_volume(self): """Test complete workflow with single volume.""" with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) - + # Create test data test_data = { "global options": { @@ -368,33 +366,35 @@ def test_full_workflow_single_volume(self): "numjobs": "1", "runtime": "60", }, - "jobs": [{ - "read": { - "io_bytes": 1000000000, - "bw_bytes": 16666666, - "iops": 4000.0, - "total_ios": 244140, - "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, - }, - "write": { - "io_bytes": 0, - "bw_bytes": 0, - "iops": 0.0, - "total_ios": 0, - "clat_ns": {"mean": 0.0, "stddev": 0.0}, - }, - "sys_cpu": 5.5, - "usr_cpu": 10.2, - }], + "jobs": [ + { + "read": { + "io_bytes": 1000000000, + "bw_bytes": 16666666, + "iops": 4000.0, + "total_ios": 244140, + "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, + }, + "write": { + "io_bytes": 0, + "bw_bytes": 0, + "iops": 0.0, + "total_ios": 0, + "clat_ns": {"mean": 0.0, "stddev": 0.0}, + }, + "sys_cpu": 5.5, + "usr_cpu": 10.2, + } + ], } - + test_file = path / "json_output.0" with open(test_file, "w") as f: json.dump(test_data, f) - + result = RBDFIO(path, "json_output") data = result.get() - + assert "randread" in data assert "1" in data["randread"] assert "4096" in data["randread"]["1"] @@ -404,7 +404,7 @@ def test_full_workflow_multiple_volumes(self): """Test complete workflow with multiple volumes.""" with tempfile.TemporaryDirectory() as tmpdir: path = Path(tmpdir) - + # Create test data for multiple volumes test_data = { "global options": { @@ -414,35 +414,37 @@ def test_full_workflow_multiple_volumes(self): "numjobs": "1", "runtime": "60", }, - "jobs": [{ - "read": { - "io_bytes": 1000000000, - "bw_bytes": 16666666, - "iops": 4000.0, - "total_ios": 244140, - "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, - }, - "write": { - "io_bytes": 0, - "bw_bytes": 0, - "iops": 0.0, - "total_ios": 0, - "clat_ns": {"mean": 0.0, "stddev": 0.0}, - }, - "sys_cpu": 5.5, - "usr_cpu": 10.2, - }], + "jobs": [ + { + "read": { + "io_bytes": 1000000000, + "bw_bytes": 16666666, + "iops": 4000.0, + "total_ios": 244140, + "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, + }, + "write": { + "io_bytes": 0, + "bw_bytes": 0, + "iops": 0.0, + "total_ios": 0, + "clat_ns": {"mean": 0.0, "stddev": 0.0}, + }, + "sys_cpu": 5.5, + "usr_cpu": 10.2, + } + ], } - + # Create multiple volume files for i in range(3): test_file = path / f"json_output.{i}" with open(test_file, "w") as f: json.dump(test_data, f) - + result = RBDFIO(path, "json_output") data = result.get() - + # Data should be aggregated from all volumes assert "randread" in data # Navigate through nested structure @@ -459,4 +461,197 @@ def test_full_workflow_multiple_volumes(self): assert iops_value == pytest.approx(12000.0, rel=0.01) +class TestMergeResourceData: + """Test _merge_resource_data() aggregates CPU/memory correctly across volumes.""" + + def _make_rbdfio(self, tmpdir: str) -> RBDFIO: + return RBDFIO(Path(tmpdir), "json_output") + + def test_first_volume_returns_unchanged(self): + """First volume has no existing data — resource data is returned as-is.""" + with tempfile.TemporaryDirectory() as tmpdir: + result = self._make_rbdfio(tmpdir) + test_config = ("randread", "4096", "32", "1") + resource_data = {"cpu": {"fio": "15.70"}, "memory": {"fio": "0.00"}} + + merged = result._merge_resource_data(test_config, resource_data) + + assert merged == resource_data + + def test_fio_cpu_is_summed_across_volumes(self): + """FIO CPU values should be summed because each volume file is independent.""" + with tempfile.TemporaryDirectory() as tmpdir: + result = self._make_rbdfio(tmpdir) + test_config = ("randread", "4096", "32", "1") + + # Simulate first volume already stored + result._processed_data = { + "randread": {"1": {"4096": {"32": {"cpu": {"fio": "10.00"}, "memory": {"fio": "0.00"}}}}} + } + result._resource_volume_counts[test_config] = 1 + + resource_data = {"cpu": {"fio": "15.00"}, "memory": {"fio": "0.00"}} + merged = result._merge_resource_data(test_config, resource_data) + + assert float(merged["cpu"]["fio"]) == pytest.approx(25.00) + + def test_shared_source_cpu_is_averaged_across_volumes(self): + """collectl/top CPU should be averaged because they share one directory.""" + with tempfile.TemporaryDirectory() as tmpdir: + result = self._make_rbdfio(tmpdir) + test_config = ("randread", "4096", "32", "1") + + # One volume already stored, count == 1 + result._processed_data = { + "randread": {"1": {"4096": {"32": {"cpu": {"collectl": "40.00"}, "memory": {"collectl": "0.00"}}}}} + } + result._resource_volume_counts[test_config] = 1 + result._resource_source_counts[(test_config, "collectl")] = 1 + + resource_data = {"cpu": {"collectl": "60.00"}, "memory": {"collectl": "0.00"}} + merged = result._merge_resource_data(test_config, resource_data) + + # (40 * 1 + 60) / 2 = 50 + assert float(merged["cpu"]["collectl"]) == pytest.approx(50.00) + + def test_shared_source_average_uses_volume_count(self): + """Running average uses the stored volume count for a third+ volume.""" + with tempfile.TemporaryDirectory() as tmpdir: + result = self._make_rbdfio(tmpdir) + test_config = ("randread", "4096", "32", "1") + + # Two volumes already processed, running average is 50, source_count == 2 + result._processed_data = { + "randread": {"1": {"4096": {"32": {"cpu": {"top": "50.00"}, "memory": {"top": "0.00"}}}}} + } + result._resource_volume_counts[test_config] = 2 + result._resource_source_counts[(test_config, "top")] = 2 + + resource_data = {"cpu": {"top": "62.00"}, "memory": {"top": "0.00"}} + merged = result._merge_resource_data(test_config, resource_data) + + # (50 * 2 + 62) / 3 = 162 / 3 = 54 + assert float(merged["cpu"]["top"]) == pytest.approx(54.00) + + def test_multiple_sources_merged_independently(self): + """FIO is summed while collectl is averaged in the same merge call.""" + with tempfile.TemporaryDirectory() as tmpdir: + result = self._make_rbdfio(tmpdir) + test_config = ("randread", "4096", "32", "1") + + result._processed_data = { + "randread": { + "1": { + "4096": { + "32": { + "cpu": {"fio": "10.00", "collectl": "40.00"}, + "memory": {"fio": "0.00", "collectl": "0.00"}, + } + } + } + } + } + result._resource_volume_counts[test_config] = 1 + result._resource_source_counts[(test_config, "collectl")] = 1 + + resource_data = { + "cpu": {"fio": "15.00", "collectl": "60.00"}, + "memory": {"fio": "0.00", "collectl": "0.00"}, + } + merged = result._merge_resource_data(test_config, resource_data) + + assert float(merged["cpu"]["fio"]) == pytest.approx(25.00) # summed + assert float(merged["cpu"]["collectl"]) == pytest.approx(50.00) # averaged + + +class TestResourceAggregationIntegration: + """Integration tests confirming CPU is correctly aggregated across multiple volumes.""" + + def _make_volume_data(self, sys_cpu: float = 5.0, usr_cpu: float = 10.0) -> dict: + return { + "global options": { + "bs": "4096", + "rw": "randread", + "iodepth": "32", + "numjobs": "1", + "runtime": "60", + }, + "jobs": [ + { + "read": { + "io_bytes": 1000000000, + "bw_bytes": 16666666, + "iops": 4000.0, + "total_ios": 244140, + "clat_ns": {"mean": 8000000.0, "stddev": 500000.0}, + }, + "write": { + "io_bytes": 0, + "bw_bytes": 0, + "iops": 0.0, + "total_ios": 0, + "clat_ns": {"mean": 0.0, "stddev": 0.0}, + }, + "sys_cpu": sys_cpu, + "usr_cpu": usr_cpu, + } + ], + } + + def test_fio_cpu_summed_across_three_volumes(self): + """FIO CPU from three identical volumes should appear as 3x the per-volume normalised value.""" + with tempfile.TemporaryDirectory() as tmpdir: + path = Path(tmpdir) + sys_cpu, usr_cpu = 5.5, 10.2 + cpu_count = 4 + per_volume_cpu = (sys_cpu + usr_cpu) / cpu_count # normalised per-volume value + + for i in range(3): + (path / f"json_output.{i}").write_text( + json.dumps(self._make_volume_data(sys_cpu=sys_cpu, usr_cpu=usr_cpu)) + ) + + with patch("post_processing.run_results.resources.fio_resource.os.cpu_count", return_value=cpu_count): + result = RBDFIO(path, "json_output") + data = result.get() + iodepth_entry = data["randread"]["1"]["4096"]["32"] + + fio_cpu = float(iodepth_entry["cpu"]["fio"]) + assert fio_cpu == pytest.approx(per_volume_cpu * 3, rel=0.01) + + def test_single_volume_fio_cpu_unchanged(self): + """Single-volume result should have exactly the per-volume normalised FIO CPU value.""" + with tempfile.TemporaryDirectory() as tmpdir: + path = Path(tmpdir) + sys_cpu, usr_cpu = 5.5, 10.2 + cpu_count = 4 + per_volume_cpu = (sys_cpu + usr_cpu) / cpu_count + + (path / "json_output.0").write_text(json.dumps(self._make_volume_data(sys_cpu=sys_cpu, usr_cpu=usr_cpu))) + + with patch("post_processing.run_results.resources.fio_resource.os.cpu_count", return_value=cpu_count): + result = RBDFIO(path, "json_output") + data = result.get() + iodepth_entry = data["randread"]["1"]["4096"]["32"] + + fio_cpu = float(iodepth_entry["cpu"]["fio"]) + assert fio_cpu == pytest.approx(per_volume_cpu, rel=0.01) + + def test_volume_counter_not_stored_in_processed_data(self): + """The internal volume counter must not appear anywhere in the output data.""" + with tempfile.TemporaryDirectory() as tmpdir: + path = Path(tmpdir) + + for i in range(2): + (path / f"json_output.{i}").write_text(json.dumps(self._make_volume_data())) + + result = RBDFIO(path, "json_output") + data = result.get() + iodepth_entry = data["randread"]["1"]["4096"]["32"] + + # Counter lives in _resource_volume_counts, not inside _processed_data + assert "_resource_volume_count" not in iodepth_entry + assert "_resource_volume_count" not in iodepth_entry.get("cpu", {}) + + # Made with Bob diff --git a/tests/test_time_series_latency_plotter.py b/tests/test_time_series_latency_plotter.py index 4966694f..c18b4503 100644 --- a/tests/test_time_series_latency_plotter.py +++ b/tests/test_time_series_latency_plotter.py @@ -148,9 +148,9 @@ def test_plot_skips_max_when_zero(self) -> None: def test_latency_constants(self) -> None: """Test latency plotter constants""" self.assertEqual(LATENCY_MEAN_COLOR, "xkcd:orange") - self.assertEqual(LATENCY_P50_COLOR, "xkcd:green") + self.assertEqual(LATENCY_P50_COLOR, "xkcd:blue") self.assertEqual(LATENCY_P95_COLOR, "xkcd:red") - self.assertEqual(LATENCY_P99_COLOR, "xkcd:dark red") + self.assertEqual(LATENCY_P99_COLOR, "xkcd:purple") self.assertEqual(LATENCY_Y_LABEL, "Latency (ms)") self.assertEqual(LATENCY_PLOT_LABEL, "Mean Latency") diff --git a/tests/test_top_resource.py b/tests/test_top_resource.py new file mode 100644 index 00000000..f6785311 --- /dev/null +++ b/tests/test_top_resource.py @@ -0,0 +1,500 @@ +""" +Unit tests for TopResource class. +""" + +# pyright: strict, reportPrivateUsage=false + +import tempfile +from pathlib import Path +from unittest import mock + +import pytest + +from post_processing.run_results.resources.top_resource import TopResource + +# --------------------------------------------------------------------------- +# Shared fixture helpers +# --------------------------------------------------------------------------- + +# A minimal two-snapshot top -b output for a single PID. +# Snapshot 1: one thread at 25.0 % CPU, 0.6 % MEM → snapshot total = 25.0 +# Snapshot 2: one thread at 35.0 % CPU, 0.8 % MEM → snapshot total = 35.0 +# Per-file average = (25.0 + 35.0) / 2 = 30.0 core-units +# With cpu_count=4: normalised = 30.0 / 4 = 7.50 +# Mem: avg(0.6, 0.8) = 0.70 +SINGLE_PID_TWO_SNAPSHOTS = """\ +top - 14:32:01 up 2 days, 3:10, 0 users, load average: 6.06, 7.09, 8.13 +Tasks: 1 total, 0 running, 1 sleeping, 0 stopped, 0 zombie +%Cpu(s): 12.3 us, 4.1 sy, 0.0 ni, 83.2 id, 0.3 wa +MiB Mem : 376023.2 total, 333174.8 free, 38078.1 used + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND +12345 ceph 20 0 1.234g 234.0m 12.0m S 25.0 0.6 1:23.45 crimson-osd + +top - 14:32:02 up 2 days, 3:10, 0 users, load average: 6.06, 7.09, 8.13 +Tasks: 1 total, 0 running, 1 sleeping, 0 stopped, 0 zombie +%Cpu(s): 13.1 us, 3.9 sy, 0.0 ni, 82.6 id, 0.4 wa +MiB Mem : 376023.2 total, 333174.8 free, 38078.1 used + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND +12345 ceph 20 0 1.234g 234.0m 12.0m S 35.0 0.8 1:24.50 crimson-osd +""" + +# Two threads per snapshot (as produced by top -H). +# Snapshot 1: thread A 20.0 + thread B 10.0 → snapshot total = 30.0 +# Snapshot 2: thread A 30.0 + thread B 10.0 → snapshot total = 40.0 +# Per-file average = (30.0 + 40.0) / 2 = 35.0 core-units +# With cpu_count=4: normalised = 35.0 / 4 = 8.75 +# Mem: avg(0.4, 0.2, 0.5, 0.2) = 0.325 → "0.33" +MULTI_THREAD_TWO_SNAPSHOTS = """\ +top - 14:32:01 up 1 day +Tasks: 2 total +%Cpu(s): 5.0 us +MiB Mem : 376023.2 total + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND + 1001 ceph 20 0 1.0g 100m 10m S 20.0 0.4 0:10.00 reactor-0 + 1002 ceph 20 0 1.0g 100m 10m S 10.0 0.2 0:05.00 reactor-1 + +top - 14:32:02 up 1 day +Tasks: 2 total +%Cpu(s): 5.0 us +MiB Mem : 376023.2 total + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND + 1001 ceph 20 0 1.0g 100m 10m S 30.0 0.5 0:11.00 reactor-0 + 1002 ceph 20 0 1.0g 100m 10m S 10.0 0.2 0:06.00 reactor-1 +""" + +_CPU_COUNT = 4 + + +def _make_top_dir(base: Path, files: dict[str, str]) -> None: + """Create a 'top' subdirectory containing the given {filename: content} files.""" + top_dir = base / "top" + top_dir.mkdir() + for name, content in files.items(): + (top_dir / name).write_text(content) + + +# --------------------------------------------------------------------------- +# Initialisation +# --------------------------------------------------------------------------- + + +class TestTopResourceInitialization: + """Test TopResource initialisation.""" + + def test_valid_setup(self) -> None: + """Initialisation succeeds when top dir exists with an output file.""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": SINGLE_PID_TWO_SNAPSHOTS}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + + assert resource.source == "top" + + def test_missing_top_directory_raises(self) -> None: + """FileNotFoundError raised when the top directory does not exist.""" + with tempfile.TemporaryDirectory() as tmpdir: + file_path = Path(tmpdir) / "json_output.0" + file_path.touch() + + with pytest.raises(FileNotFoundError, match="top directory not found"): + TopResource(file_path) + + def test_empty_top_directory_raises(self) -> None: + """FileNotFoundError raised when the top directory is empty.""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + (base / "top").mkdir() + + with pytest.raises(FileNotFoundError, match="top directory is empty"): + TopResource(file_path) + + +# --------------------------------------------------------------------------- +# Parsing — single file +# --------------------------------------------------------------------------- + + +class TestTopResourceParsingSingleFile: + """Test CPU/memory extraction from a single top output file.""" + + def test_single_snapshot_single_thread(self) -> None: + """Parse one snapshot with one process line. + + Snapshot total = 40.0 core-units. + Avg across snapshots = 40.0. + Normalised (÷4) = 10.00. + """ + content = """\ +top - 14:32:01 up 1 day +Tasks: 1 total +%Cpu(s): 5.0 us +MiB Mem : 376023.2 total + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND +12345 ceph 20 0 1.0g 100m 10m S 40.0 1.2 0:10.00 crimson-osd +""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": content}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + assert resource.cpu == "10.00" + assert resource.memory == "1.20" + + def test_two_snapshots_single_thread(self) -> None: + """CPU is summed per snapshot then averaged, then normalised. + + Snapshot 1 total = 25.0, snapshot 2 total = 35.0. + Per-file avg = 30.0 core-units. + Normalised (÷4) = 7.50. + """ + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": SINGLE_PID_TWO_SNAPSHOTS}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + assert resource.cpu == "7.50" + assert resource.memory == "0.70" + + def test_multi_thread_two_snapshots(self) -> None: + """Threads are summed per snapshot before averaging (not averaged individually). + + Snapshot 1: 20.0 + 10.0 = 30.0; snapshot 2: 30.0 + 10.0 = 40.0. + Per-file avg = 35.0 core-units. + Normalised (÷4) = 8.75. + """ + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"1001_osd_top.out": MULTI_THREAD_TWO_SNAPSHOTS}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + assert resource.cpu == "8.75" + assert resource.memory == "0.33" + + def test_zero_cpu_values(self) -> None: + """Handles zero CPU/memory without errors.""" + content = """\ +top - 14:32:01 up 1 day +%Cpu(s): 0.0 us +MiB Mem : 100.0 total + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND +99999 root 20 0 100m 10m 1m S 0.0 0.0 0:00.00 idle +""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"99999_osd_top.out": content}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + assert resource.cpu == "0.00" + assert resource.memory == "0.00" + + def test_header_only_no_data_rows(self) -> None: + """A file with a PID header but no data rows returns 0.""" + content = """\ +top - 14:32:01 up 1 day +%Cpu(s): 5.0 us + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND +""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": content}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + assert resource.cpu == "0.00" + assert resource.memory == "0.00" + + def test_empty_file_returns_zero(self) -> None: + """An empty .out file does not crash and returns 0.""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": ""}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + assert resource.cpu == "0.00" + assert resource.memory == "0.00" + + +# --------------------------------------------------------------------------- +# Parsing — multiple files +# --------------------------------------------------------------------------- + + +class TestTopResourceParsingMultipleFiles: + """Test aggregation across multiple PID output files.""" + + def test_two_pid_files_summed(self) -> None: + """CPU per-file averages are summed across PID files (not averaged). + + File A: one snapshot, one thread at 20.0 → per-file avg = 20.0 core-units. + File B: one snapshot, one thread at 60.0 → per-file avg = 60.0 core-units. + Total = 80.0. Normalised (÷4) = 20.00. + """ + content_a = """\ +top - 14:32:01 up 1 day +%Cpu(s): 5.0 us + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND + 1001 ceph 20 0 1.0g 100m 10m S 20.0 0.4 0:10.00 reactor-0 +""" + content_b = """\ +top - 14:32:01 up 1 day +%Cpu(s): 20.0 us + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND + 2001 ceph 20 0 1.0g 100m 10m S 60.0 0.8 0:10.00 reactor-1 +""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"1001_osd_top.out": content_a, "2001_osd_top.out": content_b}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + # total = 20.0 + 60.0 = 80.0; normalised = 80.0 / 4 = 20.00 + assert resource.cpu == "20.00" + + def test_three_pid_files(self) -> None: + """Aggregation works for three files. + + Files at 10, 20, 30 → total = 60.0 core-units → normalised (÷4) = 15.00. + """ + + def _make_content(cpu_val: float, mem_val: float) -> str: + return ( + "top - 14:32:01\n%Cpu(s): 5.0 us\n\n" + " PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND\n" + f" 1000 ceph 20 0 1.0g 100m 10m S {cpu_val} {mem_val} 0:01.00 osd\n" + ) + + files = { + "1000_osd_top.out": _make_content(10.0, 0.2), + "2000_osd_top.out": _make_content(20.0, 0.4), + "3000_osd_top.out": _make_content(30.0, 0.6), + } + + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, files) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + # total = 10 + 20 + 30 = 60; normalised = 60 / 4 = 15.0 + assert resource.cpu == "15.00" + # mem avg = (0.2 + 0.4 + 0.6) / 3 = 0.4 (unchanged by new algorithm) + assert resource.memory == "0.40" + + +# --------------------------------------------------------------------------- +# get() method +# --------------------------------------------------------------------------- + + +class TestTopResourceGet: + """Test the get() method output contract.""" + + def test_get_returns_correct_keys(self) -> None: + """get() returns a dict with source, cpu, and memory keys.""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": SINGLE_PID_TWO_SNAPSHOTS}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + result = TopResource(file_path).get() + + # avg snapshots = 30.0; normalised = 30.0 / 4 = 7.50 + assert result == {"source": "top", "cpu": "7.50", "memory": "0.70"} + + def test_get_triggers_parsing_once(self) -> None: + """Calling get() twice does not re-parse.""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": SINGLE_PID_TWO_SNAPSHOTS}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + first = resource.get() + second = resource.get() + + assert first == second + assert resource._has_been_parsed is True # pylint: disable=protected-access + + def test_source_property(self) -> None: + """source property returns 'top' before any parsing.""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": SINGLE_PID_TWO_SNAPSHOTS}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + assert resource.source == "top" + + +# --------------------------------------------------------------------------- +# Edge cases — error paths not covered by the happy-path tests +# --------------------------------------------------------------------------- + + +class TestTopResourceEdgeCases: + """Test uncommon but reachable branches.""" + + def test_pid_header_missing_cpu_column_is_skipped(self) -> None: + """A PID header without a %%CPU column logs a warning and is skipped.""" + content = """\ +top - 14:32:01 up 1 day +%Cpu(s): 5.0 us + + PID USER PR NI VIRT RES SHR S NOCPU NOMEM TIME+ COMMAND +12345 ceph 20 0 1.0g 100m 10m S 40.0 1.2 0:10.00 crimson-osd +""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": content}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + assert resource.cpu == "0.00" + assert resource.memory == "0.00" + + def test_data_row_too_short_is_skipped(self) -> None: + """A process row that has fewer columns than the %CPU/%MEM indices is skipped.""" + content = """\ +top - 14:32:01 up 1 day +%Cpu(s): 5.0 us + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND +12345 ceph too_short +""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": content}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + assert resource.cpu == "0.00" + assert resource.memory == "0.00" + + def test_unparseable_cpu_value_is_skipped(self) -> None: + """A data row with a non-numeric CPU value is skipped without crashing.""" + content = """\ +top - 14:32:01 up 1 day +%Cpu(s): 5.0 us + + PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND +12345 ceph 20 0 1.0g 100m 10m S N/A N/A 0:10.00 crimson-osd +""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": content}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + assert resource.cpu == "0.00" + assert resource.memory == "0.00" + + def test_parse_exception_returns_zero(self) -> None: + """If _parse_top_directory raises unexpectedly, cpu/memory fall back to 0.""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"12345_osd_top.out": SINGLE_PID_TWO_SNAPSHOTS}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + with mock.patch.object(resource, "_parse_top_directory", side_effect=RuntimeError("boom")): + resource._parse({}) # pylint: disable=protected-access + + assert resource.cpu == "0.00" + assert resource.memory == "0.00" + assert resource._has_been_parsed is True # pylint: disable=protected-access + + +# --------------------------------------------------------------------------- +# All-files behaviour +# --------------------------------------------------------------------------- + + +class TestTopResourceAllFiles: + """TopResource reads every file in the top/ directory regardless of name.""" + + def test_arbitrarily_named_file_is_parsed(self) -> None: + """A file with any name in top/ is parsed.""" + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir(base, {"my_custom_output.txt": SINGLE_PID_TWO_SNAPSHOTS}) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + # avg snapshots = 30.0; normalised = 30.0 / 4 = 7.50 + assert resource.cpu == "7.50" + assert resource.memory == "0.70" + + def test_mixed_naming_all_files_summed(self) -> None: + """Files with different naming conventions are all included and summed. + + Two identical files, each with per-file avg = 30.0 core-units. + Total = 60.0. Normalised (÷4) = 15.00. + """ + with tempfile.TemporaryDirectory() as tmpdir: + base = Path(tmpdir) + file_path = base / "json_output.0" + file_path.touch() + _make_top_dir( + base, + { + "12345_osd_top.out": SINGLE_PID_TWO_SNAPSHOTS, + "99999_mon_top.out": SINGLE_PID_TWO_SNAPSHOTS, + }, + ) + + with mock.patch("post_processing.run_results.resources.top_resource.os.cpu_count", return_value=_CPU_COUNT): + resource = TopResource(file_path) + # Two files, each contributing 30.0 core-units → total 60.0 → /4 = 15.00 + assert resource.cpu == "15.00"