-
Notifications
You must be signed in to change notification settings - Fork 77
Fix regex in benchmarks cpu/disk_on_idle (BugFix) #2487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c28b30c
10d6750
a52bb97
8f6f398
b38d4da
766b01a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| node_modules | ||
|
|
||
| venv | ||
| venv | ||
|
|
||
| CLAUDE.md | ||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,74 @@ | ||||||||||||
| #!/usr/bin/env python3 | ||||||||||||
| # This file is part of Checkbox. | ||||||||||||
| # | ||||||||||||
| # Copyright 2026 Canonical Ltd. | ||||||||||||
| # Written by: | ||||||||||||
| # Jeff Lane <jeff@ubuntu.com> | ||||||||||||
| # | ||||||||||||
| # Checkbox is free software: you can redistribute it and/or modify | ||||||||||||
| # it under the terms of the GNU General Public License version 3, | ||||||||||||
| # as published by the Free Software Foundation. | ||||||||||||
| # | ||||||||||||
| # Checkbox is distributed in the hope that it will be useful, | ||||||||||||
| # but WITHOUT ANY WARRANTY; without even the implied warranty of | ||||||||||||
| # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||||||||||||
| # GNU General Public License for more details. | ||||||||||||
| # | ||||||||||||
| # You should have received a copy of the GNU General Public License | ||||||||||||
| # along with Checkbox. If not, see <http://www.gnu.org/licenses/>. | ||||||||||||
|
|
||||||||||||
| import argparse | ||||||||||||
| import re | ||||||||||||
| import subprocess | ||||||||||||
| import sys | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def parse_iostat_column(output, column): | ||||||||||||
| values = [float(n) for n in re.findall(column + r"\n.*?(\S+)\n", output)] | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this regex was here before, but it's only working because the value you are checking is the last one of the table: Also, what happens if there is more than one disk? Is that the intended behavior? |
||||||||||||
| if not values: | ||||||||||||
| print( | ||||||||||||
| "ERROR: No '{}' values found in iostat output".format(column), | ||||||||||||
| file=sys.stderr, | ||||||||||||
| ) | ||||||||||||
| return 1 | ||||||||||||
|
Comment on lines
+29
to
+33
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just raise a system exit. Don't print and return 1. |
||||||||||||
| print("{:.2f}%".format(sum(values) / len(values))) | ||||||||||||
| return 0 | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to return 0 |
||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def main(): | ||||||||||||
| parser = argparse.ArgumentParser( | ||||||||||||
| description="Measure average CPU or disk utilization from iostat." | ||||||||||||
| ) | ||||||||||||
| parser.add_argument( | ||||||||||||
| "metric", | ||||||||||||
| choices=["cpu", "disk"], | ||||||||||||
| help="Which metric to report: 'cpu' (idle %%) or 'disk' (util %%)", | ||||||||||||
| ) | ||||||||||||
| parser.add_argument( | ||||||||||||
| "-t", | ||||||||||||
| "--time", | ||||||||||||
| action="store", | ||||||||||||
| default=10, | ||||||||||||
| help="Time in seconds to run iostat. (default: %(default)s)", | ||||||||||||
| ) | ||||||||||||
| args = parser.parse_args() | ||||||||||||
|
|
||||||||||||
| column = "idle" if args.metric == "cpu" else "util" | ||||||||||||
|
|
||||||||||||
| try: | ||||||||||||
| result = subprocess.run( | ||||||||||||
| ["iostat", "-x", "-m", "1", str(args.time)], | ||||||||||||
| stdout=subprocess.PIPE, | ||||||||||||
| stderr=subprocess.PIPE, | ||||||||||||
| universal_newlines=True, | ||||||||||||
| check=True, | ||||||||||||
| ) | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you are analyzing the output, it's simpler to use
Suggested change
|
||||||||||||
| except subprocess.CalledProcessError as e: | ||||||||||||
| print("ERROR: iostat failed: {}".format(e), file=sys.stderr) | ||||||||||||
| return 1 | ||||||||||||
|
Comment on lines
+66
to
+68
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to try/except if you are going to raise the error anyway. |
||||||||||||
|
|
||||||||||||
| return parse_iostat_column(result.stdout, column) | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| if __name__ == "__main__": | ||||||||||||
| sys.exit(main()) | ||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| import subprocess | ||
| import unittest | ||
| from io import StringIO | ||
| from unittest.mock import patch | ||
|
|
||
| import iostat_benchmark | ||
|
|
||
| IOSTAT_OUTPUT = """\ | ||
| Linux 6.8.0-57-generic (hostname) 04/27/2026 _x86_64_ (8 CPU) | ||
|
|
||
| avg-cpu: %user %nice %system %iowait %steal %idle | ||
| 0.50 0.00 0.25 0.10 0.00 99.15 | ||
|
|
||
| Device r/s rMB/s rrqm/s %rrqm r_await rareq-sz w/s wMB/s wrqm/s %wrqm w_await wareq-sz d/s dMB/s drqm/s %drqm d_await dareq-sz f/s f_await aqu-sz %util | ||
| sda 0.10 0.00 0.00 0.00 0.50 10.00 1.00 0.01 0.50 33.33 5.00 10.24 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.01 0.10 | ||
|
|
||
| avg-cpu: %user %nice %system %iowait %steal %idle | ||
| 0.60 0.00 0.30 0.05 0.00 99.05 | ||
|
|
||
| Device r/s rMB/s rrqm/s %rrqm r_await rareq-sz w/s wMB/s wrqm/s %wrqm w_await wareq-sz d/s dMB/s drqm/s %drqm d_await dareq-sz f/s f_await aqu-sz %util | ||
| sda 0.00 0.00 0.00 0.00 0.00 0.00 0.50 0.00 0.25 33.33 4.00 8.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.20 | ||
|
|
||
| """ | ||
|
|
||
|
|
||
| class TestParseIostatColumn(unittest.TestCase): | ||
| def test_cpu_idle_average(self): | ||
| out = StringIO() | ||
| with patch("sys.stdout", out): | ||
| ret = iostat_benchmark.parse_iostat_column(IOSTAT_OUTPUT, "idle") | ||
| self.assertEqual(ret, 0) | ||
| # (99.15 + 99.05) / 2 = 99.1 | ||
| self.assertIn("99.1", out.getvalue()) | ||
|
|
||
| def test_disk_util_average(self): | ||
| out = StringIO() | ||
| with patch("sys.stdout", out): | ||
| ret = iostat_benchmark.parse_iostat_column(IOSTAT_OUTPUT, "util") | ||
| self.assertEqual(ret, 0) | ||
| # (0.10 + 0.20) / 2 = 0.15 | ||
| self.assertIn("0.15", out.getvalue()) | ||
|
|
||
| def test_missing_column_returns_error(self): | ||
| err = StringIO() | ||
| with patch("sys.stderr", err): | ||
| ret = iostat_benchmark.parse_iostat_column( | ||
| "no output here", "idle" | ||
| ) | ||
| self.assertEqual(ret, 1) | ||
| self.assertIn("idle", err.getvalue()) | ||
|
|
||
|
|
||
| class TestMain(unittest.TestCase): | ||
| @patch( | ||
| "iostat_benchmark.subprocess.run", | ||
| return_value=subprocess.CompletedProcess( | ||
| args=[], returncode=0, stdout=IOSTAT_OUTPUT | ||
| ), | ||
| ) | ||
| def test_main_cpu(self, mock_run): | ||
| out = StringIO() | ||
| with patch("sys.stdout", out), patch( | ||
| "sys.argv", ["iostat_benchmark.py", "cpu"] | ||
| ): | ||
| ret = iostat_benchmark.main() | ||
| self.assertEqual(ret, 0) | ||
| self.assertEqual(mock_run.call_count, 1) | ||
| args, kwargs = mock_run.call_args | ||
| self.assertEqual(args[0], ["iostat", "-x", "-m", "1", "10"]) | ||
| self.assertTrue(kwargs.get("check")) | ||
| self.assertTrue( | ||
| ( | ||
| kwargs.get("capture_output") is True | ||
| and kwargs.get("text") is True | ||
| ) | ||
| or ( | ||
| kwargs.get("stdout") == subprocess.PIPE | ||
| and kwargs.get("stderr") == subprocess.PIPE | ||
| and kwargs.get("universal_newlines") is True | ||
| ) | ||
| ) | ||
|
|
||
| @patch( | ||
| "iostat_benchmark.subprocess.run", | ||
| return_value=subprocess.CompletedProcess( | ||
| args=[], returncode=0, stdout=IOSTAT_OUTPUT | ||
| ), | ||
| ) | ||
| def test_main_disk(self, mock_run): | ||
| out = StringIO() | ||
| with patch("sys.stdout", out), patch( | ||
| "sys.argv", ["iostat_benchmark.py", "disk"] | ||
| ): | ||
| ret = iostat_benchmark.main() | ||
| self.assertEqual(ret, 0) | ||
|
|
||
| @patch( | ||
| "iostat_benchmark.subprocess.run", | ||
| side_effect=subprocess.CalledProcessError(1, "iostat"), | ||
| ) | ||
| def test_main_iostat_failure(self, _mock_run): | ||
| err = StringIO() | ||
| with patch("sys.stderr", err), patch( | ||
| "sys.argv", ["iostat_benchmark.py", "cpu"] | ||
| ): | ||
| ret = iostat_benchmark.main() | ||
| self.assertEqual(ret, 1) | ||
| self.assertIn("iostat failed", err.getvalue()) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a CLAUDE.md file in checkbox