From 55e62a0d2a6133aa437c30fdeccefb2aeb403bcd Mon Sep 17 00:00:00 2001 From: Rohini Chandra <61837065+r14chandra@users.noreply.github.com> Date: Thu, 30 Jan 2025 16:49:55 +0530 Subject: [PATCH] code refactoring --- ros/processor/suggestions_engine.py | 94 ++++++++++++++--------------- tests/test_suggestions_engine.py | 42 +------------ 2 files changed, 47 insertions(+), 89 deletions(-) diff --git a/ros/processor/suggestions_engine.py b/ros/processor/suggestions_engine.py index 6d04c2c9..15c87936 100644 --- a/ros/processor/suggestions_engine.py +++ b/ros/processor/suggestions_engine.py @@ -21,19 +21,6 @@ logging = get_logger(__name__) -PCP_METRICS = [ - 'disk.dev.total', - 'hinv.ncpu', - 'kernel.all.cpu.idle', - 'kernel.all.pressure.cpu.some.avg', - 'kernel.all.pressure.io.full.avg', - 'kernel.all.pressure.io.some.avg', - 'kernel.all.pressure.memory.full.avg', - 'kernel.all.pressure.memory.some.avg', - 'mem.physmem', - 'mem.util.available', -] - class SuggestionsEngine: def __init__(self): @@ -133,45 +120,56 @@ def create_output_dir(request_id, host): return output_dir +def run_pmlogextract(service, event, host, index_file_path, output_dir): + """Run the pmlogextract command.""" + + pmlogextract_command = [ + "pmlogextract", + "-c", + "ros/lib/pcp_extract_config", + index_file_path, + output_dir + ] + + logging.debug(f"{service} - {event} - Running pmlogextract command for system {host.get('id')}.") + try: + subprocess.run(pmlogextract_command, check=True, ) + logging.debug(f"{service} - {event} - Successfully ran pmlogextract command for system {host.get('id')}.") + except subprocess.CalledProcessError as error: + logging.error( + f"{service} - {event} - Error running pmlogextract command for system {host.get('id')}: {error.stdout}" + ) + raise + + +def run_pmlogsummary(service, event, host, output_dir): + """Run the pmlogsummary command.""" + + pmlogsummary_command = [ + "pmlogsummary", + "-F", + os.path.join(output_dir, ".index") + ] + + logging.debug(f"{service} - {event} - Running pmlogsummary command for system {host.get('id')}.") + try: + subprocess.run(pmlogsummary_command, check=True, text=True, capture_output=True) + logging.debug(f"{service} - {event} - Successfully ran pmlogsummary command for system {host.get('id')}.") + except subprocess.CalledProcessError as error: + logging.error( + f"{service} - {event} - Error running pmlogsummary command for system {host.get('id')}: {error.stdout}" + ) + raise + + def run_pcp_commands(service, event, host, index_file_path, request_id): try: sanitized_request_id = request_id.replace("/", "_") output_dir = create_output_dir(sanitized_request_id, host) - pmlogextract_command = [ - "pmlogextract", - "-c", - "ros/lib/pcp_extract_config", - index_file_path, - output_dir - ] + run_pmlogextract(service, event, host, index_file_path, output_dir) - logging.info(f"{service} - {event} - Running pmlogextract command for system {host.get('id')}.") - try: - subprocess.run(pmlogextract_command, check=True) - logging.info(f"{service} - {event} - Successfully ran pmlogextract command for system {host.get('id')}.") - except subprocess.CalledProcessError as error: - logging.error( - f"{service} - {event} - Error running pmlogextract command for system {host.get('id')}: {error.stdout}" - ) - return - - pmlogsummary_command = [ - "pmlogsummary", - "-f", - os.path.join(output_dir, ".index"), - *PCP_METRICS - ] - - logging.info(f"{service} - {event} - Running pmlogsummary command for system {host.get('id')}.") - try: - subprocess.run(pmlogsummary_command, check=True, capture_output=True, text=True) - logging.info(f"{service} - {event} - Successfully ran pmlogsummary command for system {host.get('id')}.") - except subprocess.CalledProcessError as error: - logging.error( - f"{service} - {event} - Error running pmlogsummary command for system {host.get('id')}: {error.stdout}" - ) - return + run_pmlogsummary(service, event, host, output_dir) except Exception as error: logging.error( @@ -181,7 +179,7 @@ def run_pcp_commands(service, event, host, index_file_path, request_id): try: if os.path.exists(output_dir): shutil.rmtree(output_dir) - logging.info(f"{service} - {event} - Cleaned up output directory for system {host.get('id')}.") + logging.debug(f"{service} - {event} - Cleaned up output directory for system {host.get('id')}.") except Exception as error: logging.error( f"{service} - {event} - Error cleaning up the output directory for system {host.get('id')}: {error}" @@ -190,7 +188,7 @@ def run_pcp_commands(service, event, host, index_file_path, request_id): @contextmanager def download_and_extract(service, event, archive_URL, host, org_id): - logging.info(f"{service} - {event} - Downloading the report for system {host.get('id')}.") + logging.debug(f"{service} - {event} - Downloading the report for system {host.get('id')}.") try: response = requests.get(archive_URL, timeout=10) diff --git a/tests/test_suggestions_engine.py b/tests/test_suggestions_engine.py index f6a7eeec..28123e44 100644 --- a/tests/test_suggestions_engine.py +++ b/tests/test_suggestions_engine.py @@ -8,8 +8,7 @@ download_and_extract, get_index_file_path, create_output_dir, - find_root_directory, - PCP_METRICS + find_root_directory ) @@ -159,44 +158,5 @@ def test_create_output_dir(self, mock_path_exists, mock_makedirs): self.assertEqual(output_dir, "/tmp/pmlogextract-output-12345/") -class TestPCPMetrics(unittest.TestCase): - def test_pcp_metrics_check(self): - expected_metrics = [ - "disk.dev.total", - "hinv.ncpu", - "kernel.all.cpu.idle", - "kernel.all.pressure.cpu.some.avg", - "kernel.all.pressure.io.full.avg", - "kernel.all.pressure.io.some.avg", - "kernel.all.pressure.memory.full.avg", - "kernel.all.pressure.memory.some.avg", - "mem.physmem", - "mem.util.available", - ] - self.assertEqual(PCP_METRICS, expected_metrics) - - def test_pcp_metrics_in_command(self): - output_dir = "/tmp/pmlogextract-output-12345" - command = ["pmlogsummary", "-f", output_dir, *PCP_METRICS] - - expected_command = [ - "pmlogsummary", - "-f", - output_dir, - "disk.dev.total", - "hinv.ncpu", - "kernel.all.cpu.idle", - "kernel.all.pressure.cpu.some.avg", - "kernel.all.pressure.io.full.avg", - "kernel.all.pressure.io.some.avg", - "kernel.all.pressure.memory.full.avg", - "kernel.all.pressure.memory.some.avg", - "mem.physmem", - "mem.util.available", - ] - - self.assertEqual(command, expected_command) - - if __name__ == '__main__': unittest.main()