From 3e4bfd2f3e11c2f793633a72144d1108ca8bee52 Mon Sep 17 00:00:00 2001 From: Sagar Sagar Date: Fri, 21 Aug 2026 09:50:56 +0000 Subject: [PATCH] pcp-tools: correct None handling and CPU utilization Handle None values safely in mpstat printers instead of calling replace() on None. Fix per-CPU utilization scaling so aggregate CPU values are divided by the total CPU count only when reporting the aggregate. Update interrupt usage tests to use the new computation context and explicit interrupt report type.fix(mpstat): correct None handling and CPU utilization Handle None values safely in mpstat printers instead of calling replace() on None. Fix per-CPU utilization scaling so aggregate CPU values are divided by the total CPU count only when reporting the aggregate. Update interrupt usage tests to use the new computation context and explicit interrupt report type. --- src/pcp/mpstat/pcp-mpstat.py | 6 +-- .../test/interrupt_usage_reporter_test.py | 4 +- src/pcp/mpstat/test/interrupt_usage_test.py | 13 +++--- .../none_handler_printer_decorator_test.py | 18 +++++++- .../mpstat/test/soft_interrupt_usage_test.py | 13 +++--- src/pcp/pidstat/pcp-pidstat.py | 2 +- .../none_handler_printer_decorator_test.py | 11 ++++- src/pcp/ps/pcp-ps.py | 2 +- .../none_handler_printer_decorator_test.py | 20 +++++++++ .../test/process_state_util_reporter_test.py | 11 +++-- src/pcp/ps/test/process_statusutil_test.py | 41 ++++++++++--------- 11 files changed, 94 insertions(+), 47 deletions(-) create mode 100644 src/pcp/ps/test/none_handler_printer_decorator_test.py diff --git a/src/pcp/mpstat/pcp-mpstat.py b/src/pcp/mpstat/pcp-mpstat.py index 0bcf96b0b7c..b888a6f9f57 100755 --- a/src/pcp/mpstat/pcp-mpstat.py +++ b/src/pcp/mpstat/pcp-mpstat.py @@ -36,7 +36,7 @@ class StdoutPrinter: def Print(self, args): if args is None: - args = args.replace('None', '?') + args = '?' print(args) class NamedInterrupts: @@ -179,7 +179,7 @@ def _compute_metric(self, metric_suffix): return None try: value = (100 * (c_time - p_time)) / (1000 * self.delta_time) - if self.instance is not None: + if self.instance is None: total = self.total_cpus() if total: value /= total @@ -544,7 +544,7 @@ def __init__(self, printer): self.printer = printer def Print(self, args): - new_args = args.replace('None','?') + new_args = '?' if args is None else args.replace('None','?') self.printer(new_args) class MpstatOptions(pmapi.pmOptions): diff --git a/src/pcp/mpstat/test/interrupt_usage_reporter_test.py b/src/pcp/mpstat/test/interrupt_usage_reporter_test.py index 4b06bd1d804..f6a28c35d80 100644 --- a/src/pcp/mpstat/test/interrupt_usage_reporter_test.py +++ b/src/pcp/mpstat/test/interrupt_usage_reporter_test.py @@ -44,7 +44,7 @@ def test_print_report(self): interrupt_usage.get_percpu_interrupts = Mock(return_value = cpu_interrupts) cpu_filter = Mock() cpu_filter.filter_cpus = Mock(return_value = cpu_interrupts) - report = InterruptUsageReporter(cpu_filter, printer, options) + report = InterruptUsageReporter(cpu_filter, printer, options, 'interrupt') timestamp = '2016-7-18 IST' calls = [call('\nTimestamp\tCPU \tSOME_INTERRUPT/s\tANOTHER_INTERRUPT/s\t'), call('2016-7-18 IST\t0 \t1.23 \t2.34 \t')] @@ -61,7 +61,7 @@ def test_print_report_with_cpu_filter_on(self): interrupt_usage.get_percpu_interrupts = Mock(return_value = cpu_interrupts) cpu_filter = Mock() cpu_filter.filter_cpus = Mock(return_value = [self.cpu_interrupt_zero]) - report = InterruptUsageReporter(cpu_filter, printer, options) + report = InterruptUsageReporter(cpu_filter, printer, options, 'interrupt') timestamp = '2016-7-18 IST' calls = [call('\nTimestamp\tCPU \tSOME_INTERRUPT/s\tANOTHER_INTERRUPT/s\t'), call('2016-7-18 IST\t0 \t1.23 \t2.34 \t')] diff --git a/src/pcp/mpstat/test/interrupt_usage_test.py b/src/pcp/mpstat/test/interrupt_usage_test.py index 0c513cee01a..f689ece866b 100644 --- a/src/pcp/mpstat/test/interrupt_usage_test.py +++ b/src/pcp/mpstat/test/interrupt_usage_test.py @@ -16,13 +16,14 @@ import sys import unittest from unittest.mock import Mock -from pcp_mpstat import InterruptUsage +from pcp_mpstat import InterruptComputationContext, InterruptUsage class TestInterruptUsage(unittest.TestCase): def setUp(self): self.metric_repository = Mock() self.metric_repository.current_value = Mock(side_effect = self.current_value_side_effect) self.metric_repository.previous_value = Mock(side_effect = self.previous_value_side_effect) + self.context = InterruptComputationContext(1.34, self.metric_repository) def current_value_side_effect(self, metric, instance): if metric == 'kernel.percpu.interrupts.line12' and instance == 0: @@ -43,35 +44,35 @@ def previous_value_side_effect(self, metric, instance): return None def test_if_name_has_line_in_it(self): - interrupt_usage = InterruptUsage(1.34, self.metric_repository, 'kernel.percpu.interrupts.line12', 0) + interrupt_usage = InterruptUsage(self.context, 'kernel.percpu.interrupts.line12', 0) name = interrupt_usage.name() self.assertEqual(name, '12') def test_if_name_does_not_have_line_in_it(self): - interrupt_usage = InterruptUsage(1.34, self.metric_repository, 'kernel.percpu.interrupts.PIW', 0) + interrupt_usage = InterruptUsage(self.context, 'kernel.percpu.interrupts.PIW', 0) name = interrupt_usage.name() self.assertEqual(name, 'PIW') def test_value_if_not_none(self): - interrupt_usage = InterruptUsage(1.34, self.metric_repository, 'kernel.percpu.interrupts.line0', 2) + interrupt_usage = InterruptUsage(self.context, 'kernel.percpu.interrupts.line0', 2) value = interrupt_usage.value() self.assertEqual(value, 1.49) def test_value_if_current_value_is_none(self): - interrupt_usage = InterruptUsage(1.34, self.metric_repository, 'kernel.percpu.interrupts.line12', 1) + interrupt_usage = InterruptUsage(self.context, 'kernel.percpu.interrupts.line12', 1) value = interrupt_usage.value() self.assertIsNone(value) def test_value_if_previous_value_is_none(self): - interrupt_usage = InterruptUsage(1.34, self.metric_repository, 'kernel.percpu.interrupts.line12', 0) + interrupt_usage = InterruptUsage(self.context, 'kernel.percpu.interrupts.line12', 0) value = interrupt_usage.value() diff --git a/src/pcp/mpstat/test/none_handler_printer_decorator_test.py b/src/pcp/mpstat/test/none_handler_printer_decorator_test.py index 37e417bf761..fb4102152d0 100644 --- a/src/pcp/mpstat/test/none_handler_printer_decorator_test.py +++ b/src/pcp/mpstat/test/none_handler_printer_decorator_test.py @@ -15,8 +15,8 @@ import sys import unittest -from unittest.mock import Mock -from pcp_mpstat import NoneHandlingPrinterDecorator +from unittest.mock import Mock, patch +from pcp_mpstat import NoneHandlingPrinterDecorator, StdoutPrinter class TestNoneHandlingPrinterDecorator(unittest.TestCase): @@ -37,5 +37,19 @@ def test_print_report_with_none_values(self): printer.Print.assert_called_with("2016-07-20 IST\tALL\t 1.23\t ?\t 3.45\t 4.56\t ?\t 6.78\t 7.89\t ?\t 1.34\t 2.45") + def test_print_report_with_none(self): + printer = Mock() + printer_decorator = NoneHandlingPrinterDecorator(printer.Print) + + printer_decorator.Print(None) + + printer.Print.assert_called_with("?") + + def test_stdout_printer_with_none(self): + with patch('builtins.print') as printer: + StdoutPrinter().Print(None) + + printer.assert_called_once_with("?") + if __name__ == "__main__": unittest.main() diff --git a/src/pcp/mpstat/test/soft_interrupt_usage_test.py b/src/pcp/mpstat/test/soft_interrupt_usage_test.py index 68ebda9cf57..9fb3c908c34 100644 --- a/src/pcp/mpstat/test/soft_interrupt_usage_test.py +++ b/src/pcp/mpstat/test/soft_interrupt_usage_test.py @@ -21,16 +21,13 @@ class TestSoftInterruptUsage(unittest.TestCase): def setUp(self): self.metric_repository = Mock() - self.metric_repository.current_values = Mock(side_effect = self.current_value_side_effect) - self.interrupt_metric = ['kernel.percpu.softirqs.RCU','kernel.percpu.softirqs.HRTIMER','kernel.percpu.softirqs.SCHED'] - - def current_value_side_effect(self, metric): - if metric == 'hinv.map.cpu_num': - return {'0':0,'1':1,'2':2,'3':3} - return None + self.metric_repository.group.get.return_value = Mock(desc=Mock(indom=1)) + self.metric_repository.group.contextCache.pmGetInDom.return_value = ( + [0, 1, 2, 3], + ['RCU::cpu0', 'RCU::cpu1', 'RCU::cpu2', 'RCU::cpu3']) def test_get_percpu_interrupts(self): - soft_interrupt_usage = SoftInterruptUsage(1.34, self.metric_repository, self.interrupt_metric) + soft_interrupt_usage = SoftInterruptUsage(1.34, self.metric_repository) percpu_interrupts = soft_interrupt_usage.get_percpu_interrupts() diff --git a/src/pcp/pidstat/pcp-pidstat.py b/src/pcp/pidstat/pcp-pidstat.py index 32bf9256211..337b36dd9b4 100755 --- a/src/pcp/pidstat/pcp-pidstat.py +++ b/src/pcp/pidstat/pcp-pidstat.py @@ -773,7 +773,7 @@ def __init__(self, printer): self.printer = printer def Print(self, args): - new_args = args.replace('None','?') + new_args = '?' if args is None else args.replace('None','?') self.printer.Print(new_args) diff --git a/src/pcp/pidstat/test/none_handler_printer_decorator_test.py b/src/pcp/pidstat/test/none_handler_printer_decorator_test.py index 4b341a43fec..4199335ca7b 100644 --- a/src/pcp/pidstat/test/none_handler_printer_decorator_test.py +++ b/src/pcp/pidstat/test/none_handler_printer_decorator_test.py @@ -14,7 +14,7 @@ # import unittest -from mock import Mock +from unittest.mock import Mock from pcp_pidstat import NoneHandlingPrinterDecorator class TestNoneHandlingPrinterDecorator(unittest.TestCase): @@ -36,5 +36,14 @@ def test_print_report_with_none_values(self): printer.Print.assert_called_with("123\t1000\t1\t?\t1.24\t0.0\t?\t1\tprocess_1") + def test_print_report_with_none(self): + printer = Mock() + printer.Print = Mock() + printer_decorator = NoneHandlingPrinterDecorator(printer) + + printer_decorator.Print(None) + + printer.Print.assert_called_with("?") + if __name__ == "__main__": unittest.main() diff --git a/src/pcp/ps/pcp-ps.py b/src/pcp/ps/pcp-ps.py index 2764c0e2c1a..29345303798 100755 --- a/src/pcp/ps/pcp-ps.py +++ b/src/pcp/ps/pcp-ps.py @@ -49,7 +49,7 @@ def __init__(self, printer): self.printer = printer def Print(self, args): - new_args = args.replace('None', '?') + new_args = '?' if args is None else args.replace('None', '?') self.printer.Print(new_args) diff --git a/src/pcp/ps/test/none_handler_printer_decorator_test.py b/src/pcp/ps/test/none_handler_printer_decorator_test.py new file mode 100644 index 00000000000..c502aab145c --- /dev/null +++ b/src/pcp/ps/test/none_handler_printer_decorator_test.py @@ -0,0 +1,20 @@ +#!/usr/bin/env pmpython + +import unittest +from unittest.mock import Mock + +from pcp_ps import NoneHandlingPrinterDecorator + + +class TestNoneHandlingPrinterDecorator(unittest.TestCase): + def test_print_report_with_none(self): + printer = Mock() + printer_decorator = NoneHandlingPrinterDecorator(printer) + + printer_decorator.Print(None) + + printer.Print.assert_called_once_with('?') + + +if __name__ == '__main__': + unittest.main() diff --git a/src/pcp/ps/test/process_state_util_reporter_test.py b/src/pcp/ps/test/process_state_util_reporter_test.py index e7a30364d44..a4661d5581e 100755 --- a/src/pcp/ps/test/process_state_util_reporter_test.py +++ b/src/pcp/ps/test/process_state_util_reporter_test.py @@ -13,15 +13,17 @@ # or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License # for more details. # -from mock import Mock import unittest +from unittest.mock import Mock from pcp_ps import ProcessStatusReporter class TestProcessStateReporter(unittest.TestCase): def setUp(self): - self.options = Mock( - show_process_user=None) + self.options = Mock(show_process_user=None) + self.options.debug_mode = False + self.options.sorting_flag = False + self.options.universal_flag = 'all' process_1 = Mock(pid=Mock(return_value=1), process_name=Mock(return_value="process_1"), @@ -29,7 +31,8 @@ def setUp(self): user_id=Mock(return_value=1000), stack_size=Mock(return_value=136), tty_name=Mock(return_value="tty"), - total_time=Mock(return_value=100)) + total_time=Mock(return_value=100), + process_name_with_args=Mock(return_value="process_1")) self.processes = [process_1] diff --git a/src/pcp/ps/test/process_statusutil_test.py b/src/pcp/ps/test/process_statusutil_test.py index 7d2c9797e85..5f1af1b9b34 100755 --- a/src/pcp/ps/test/process_statusutil_test.py +++ b/src/pcp/ps/test/process_statusutil_test.py @@ -14,8 +14,8 @@ # for more details. # -import mock import unittest +from unittest import mock from pcp_ps import ProcessStatusUtil @@ -24,6 +24,9 @@ def setUp(self): self.__metric_repository = mock.Mock() self.__metric_repository.current_value = mock.Mock(side_effect=self.metric_repo_current_value_side_effect) + def process_status_util(self): + return ProcessStatusUtil(1, None, 1.34, self.__metric_repository) + def metric_repo_current_value_side_effect(self, metric_name, instance): if metric_name == 'proc.psinfo.cmd' and instance == 1: return "test" @@ -74,87 +77,87 @@ def test_stack_referenced_size(self): #These are blank spaces in assert case been addded #to match the format of function ouput.please don't remove def test_username(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.user_name() self.assertEqual(name, "test ") def test_Processname(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.process_name() self.assertEqual(name, "test ") def test_process_name_with_args(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.process_name_with_args() self.assertEqual(name, "test ") def test_vszie(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() vsize = process_status_usage.vsize() self.assertEqual(vsize, 1) def test_rss(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() vsize = process_status_usage.rss() self.assertEqual(vsize, 1) def test_mem(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() vsize = process_status_usage.mem() self.assertEqual(vsize, 100) def test_pid(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() pid = process_status_usage.pid() self.assertEqual(pid,'1 ') def test_process_name(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.process_name() self.assertEqual(name, 'test ') def test_user_id(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() user_id = process_status_usage.user_id() self.assertEqual(user_id, 1) def test_s_name(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.s_name() self.assertEqual(name, 'R') def test_cpu_number(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.cpu_number() self.assertEqual(name, 1) def test_wchan_s(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.wchan_s() self.assertEqual(name, 'test ') def test_priority(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.priority() self.assertEqual(name, 1) def test_tty_name(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.tty_name() self.assertEqual(name, 'tty') def test_start_time(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.start_time() self.assertEqual(name, 1) def test_func_state(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.func_state() - self.assertEqual(name, 'N/A') + self.assertEqual(name, '-') def test_policy(self): - process_status_usage = ProcessStatusUtil(1, 1.34, self.__metric_repository) + process_status_usage = self.process_status_util() name = process_status_usage.policy() self.assertEqual(name, 'FIFO')