From 5aa74a3e1dbccef131cc35a924eb2e7c042961ee Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 4 May 2022 18:41:03 +0300 Subject: [PATCH 1/7] [sonic-bootchart] add sonic-bootchart Signed-off-by: Stepan Blyschak --- scripts/sonic-bootchart | 120 ++++++++++++++++++++++++++++++++++ setup.py | 1 + tests/sonic_bootchart_test.py | 81 +++++++++++++++++++++++ 3 files changed, 202 insertions(+) create mode 100755 scripts/sonic-bootchart create mode 100755 tests/sonic_bootchart_test.py diff --git a/scripts/sonic-bootchart b/scripts/sonic-bootchart new file mode 100755 index 0000000000..ce367cb43c --- /dev/null +++ b/scripts/sonic-bootchart @@ -0,0 +1,120 @@ +#!/usr/bin/env python3 + +import click +import sys +import configparser +import subprocess +import functools +import os +import glob +from tabulate import tabulate +import utilities_common.cli as clicommon + +SYSTEMD_BOOTCHART = "/lib/systemd/systemd-bootchart" +BOOTCHART_CONF = "/etc/systemd/bootchart.conf" +BOOTCHART_DEFAULT_OUTPUT_DIR = "/run/log/" +BOOTCHART_DEFAULT_OUTPUT_GLOB = os.path.join(BOOTCHART_DEFAULT_OUTPUT_DIR, "bootchart-*.svg") + +class BootChartConfigParser(configparser.ConfigParser): + def optionxform(self, option): + return option + +def exit_cli(*args, **kwargs): + """ Print a message and exit with rc 1. """ + click.secho(*args, **kwargs) + sys.exit(1) + + +def root_privileges_required(func): + """ Decorates a function, so that the function is invoked + only if the user is root. """ + @functools.wraps(func) + def wrapped_function(*args, **kwargs): + """ Wrapper around func. """ + if os.geteuid() != 0: + exit_cli("Root privileges required for this operation", fg="red") + return func(*args, **kwargs) + + wrapped_function.__doc__ += "\n\n NOTE: This command requires elevated (root) privileges to run." + return wrapped_function + + +def check_bootchart_installed(): + """ Fails imidiatelly if bootchart is not installed """ + if not os.path.exists(SYSTEMD_BOOTCHART): + exit_cli("systemd-bootchart is not installed", fg="red") + + +def get_enabled_status(): + """ Get systemd-bootchart status """ + return clicommon.run_command("systemctl is-enabled systemd-bootchart", return_cmd=True) + +def get_active_status(): + """ Get systemd-bootchart status """ + return clicommon.run_command("systemctl is-active systemd-bootchart", return_cmd=True) + +def get_output_files(): + bootchart_output_files = [] + for bootchart_output_file in glob.glob(BOOTCHART_DEFAULT_OUTPUT_GLOB): + bootchart_output_files.append(bootchart_output_file) + return "\n".join(bootchart_output_files) + + +@click.group() +def cli(): + """ Main CLI group """ + check_bootchart_installed() + + +@cli.command() +@root_privileges_required +def enable(): + """ Enable bootchart """ + clicommon.run_command("systemctl enable systemd-bootchart", display_cmd=True) + + +@cli.command() +@root_privileges_required +def disable(): + """ Disable bootchart """ + clicommon.run_command("systemctl disable systemd-bootchart", display_cmd=True) + + +@cli.command() +@click.option('--samples', type=int) +@click.option('--frequency', type=int) +@root_privileges_required +def config(**kwargs): + """ Configure bootchart """ + config = {k.title(): str(v) for k, v in kwargs.items() if v is not None} + if not config: + exit_cli("Nothing to update") + bootchart_config = BootChartConfigParser() + bootchart_config.read(BOOTCHART_CONF) + bootchart_config['Bootchart'].update(config) + with open(BOOTCHART_CONF, 'w') as config_file: + bootchart_config.write(config_file, space_around_delimiters=False) + + +@cli.command() +def show(): + """ Display bootchart configuration """ + bootchart_config = BootChartConfigParser() + bootchart_config.read(BOOTCHART_CONF) + + field_values = { + "Status": get_enabled_status(), + "Operational Status": get_active_status(), + "Samples": bootchart_config["Bootchart"]["Samples"], + "Frequency": bootchart_config["Bootchart"]["Frequency"], + "Output": get_output_files(), + } + + click.echo(tabulate([field_values.values()], field_values.keys())) + + +def main(): + cli() + +if __name__ == "__main__": + main() diff --git a/setup.py b/setup.py index a8bf39170c..e545c9c6f1 100644 --- a/setup.py +++ b/setup.py @@ -142,6 +142,7 @@ 'scripts/watermarkstat', 'scripts/watermarkcfg', 'scripts/sonic-kdump-config', + 'scripts/sonic-bootchart', 'scripts/centralize_database', 'scripts/null_route_helper', 'scripts/coredump_gen_handler.py', diff --git a/tests/sonic_bootchart_test.py b/tests/sonic_bootchart_test.py new file mode 100755 index 0000000000..11ea58f49d --- /dev/null +++ b/tests/sonic_bootchart_test.py @@ -0,0 +1,81 @@ +import os +import subprocess +import pytest +from click.testing import CliRunner +from unittest.mock import patch, Mock +import utilities_common +import imp + +sonic_bootchart = imp.load_source('sonic-bootchart', 'scripts/sonic-bootchart') + +BOOTCHART_OUTPUT_FILES = [ + os.path.join(sonic_bootchart.BOOTCHART_DEFAULT_OUTPUT_DIR, "bootchart-20220504-1040.svg"), + os.path.join(sonic_bootchart.BOOTCHART_DEFAULT_OUTPUT_DIR, "bootchart-20220504-1045.svg"), +] + +@pytest.fixture(autouse=True) +def setup(fs): + # create required file for bootchart installation check + fs.create_file(sonic_bootchart.SYSTEMD_BOOTCHART) + fs.create_file(sonic_bootchart.BOOTCHART_CONF) + for bootchart_output_file in BOOTCHART_OUTPUT_FILES: + fs.create_file(bootchart_output_file) + + with open(sonic_bootchart.BOOTCHART_CONF, 'w') as config_file: + config_file.write(""" + [Bootchart] + Samples=500 + Frequency=25 + """) + + # pass the root user check + with patch("os.geteuid") as mock: + mock.return_value = 0 + yield + + +@patch("utilities_common.cli.run_command") +class TestSonicBootchart: + def test_enable(self, mock_run_command): + runner = CliRunner() + result = runner.invoke(sonic_bootchart.cli.commands['enable'], []) + assert not result.exit_code + mock_run_command.assert_called_with("systemctl enable systemd-bootchart", display_cmd=True) + + def test_disable(self, mock_run_command): + runner = CliRunner() + result = runner.invoke(sonic_bootchart.cli.commands['disable'], []) + assert not result.exit_code + mock_run_command.assert_called_with("systemctl disable systemd-bootchart", display_cmd=True) + + def test_config_show(self, mock_run_command): + def run_command_side_effect(command, **kwargs): + if "is-enabled": + return "enabled" + elif "is-active": + return "active" + else: + raise Exception("unknown command") + + mock_run_command.side_effect = run_command_side_effect + + runner = CliRunner() + result = runner.invoke(sonic_bootchart.cli.commands['show'], []) + assert not result.exit_code + assert result.output == \ + "Status Operational Status Samples Frequency Output\n" \ + "-------- -------------------- --------- ----------- ------------------------------------\n" \ + "enabled enabled 500 25 /run/log/bootchart-20220504-1040.svg\n" \ + " /run/log/bootchart-20220504-1045.svg\n" + + result = runner.invoke(sonic_bootchart.cli.commands["config"], ["--samples", "100", "--frequency", "50"]) + assert not result.exit_code + + result = runner.invoke(sonic_bootchart.cli.commands['show'], []) + assert not result.exit_code + assert result.output == \ + "Status Operational Status Samples Frequency Output\n" \ + "-------- -------------------- --------- ----------- ------------------------------------\n" \ + "enabled enabled 100 50 /run/log/bootchart-20220504-1040.svg\n" \ + " /run/log/bootchart-20220504-1045.svg\n" + From 87fec53e5f0e6c1d95e2dd9247473e3d2d41a184 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 11 May 2022 14:28:00 +0300 Subject: [PATCH 2/7] add time column Signed-off-by: Stepan Blyschak --- scripts/sonic-bootchart | 9 +++++++-- tests/sonic_bootchart_test.py | 17 ++++++++--------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/scripts/sonic-bootchart b/scripts/sonic-bootchart index ce367cb43c..93231872c3 100755 --- a/scripts/sonic-bootchart +++ b/scripts/sonic-bootchart @@ -102,11 +102,16 @@ def show(): bootchart_config = BootChartConfigParser() bootchart_config.read(BOOTCHART_CONF) + samples = int(bootchart_config["Bootchart"]["Samples"]) + frequency = int(bootchart_config["Bootchart"]["Frequency"]) + time = samples // frequency + field_values = { "Status": get_enabled_status(), "Operational Status": get_active_status(), - "Samples": bootchart_config["Bootchart"]["Samples"], - "Frequency": bootchart_config["Bootchart"]["Frequency"], + "Samples": samples, + "Frequency": frequency, + "Time (sec)": time, "Output": get_output_files(), } diff --git a/tests/sonic_bootchart_test.py b/tests/sonic_bootchart_test.py index 11ea58f49d..0c87b647b4 100755 --- a/tests/sonic_bootchart_test.py +++ b/tests/sonic_bootchart_test.py @@ -63,10 +63,10 @@ def run_command_side_effect(command, **kwargs): result = runner.invoke(sonic_bootchart.cli.commands['show'], []) assert not result.exit_code assert result.output == \ - "Status Operational Status Samples Frequency Output\n" \ - "-------- -------------------- --------- ----------- ------------------------------------\n" \ - "enabled enabled 500 25 /run/log/bootchart-20220504-1040.svg\n" \ - " /run/log/bootchart-20220504-1045.svg\n" + "Status Operational Status Samples Frequency Time (sec) Output\n" \ + "-------- -------------------- --------- ----------- ------------ ------------------------------------\n" \ + "enabled enabled 500 25 20 /run/log/bootchart-20220504-1040.svg\n" \ + " /run/log/bootchart-20220504-1045.svg\n" result = runner.invoke(sonic_bootchart.cli.commands["config"], ["--samples", "100", "--frequency", "50"]) assert not result.exit_code @@ -74,8 +74,7 @@ def run_command_side_effect(command, **kwargs): result = runner.invoke(sonic_bootchart.cli.commands['show'], []) assert not result.exit_code assert result.output == \ - "Status Operational Status Samples Frequency Output\n" \ - "-------- -------------------- --------- ----------- ------------------------------------\n" \ - "enabled enabled 100 50 /run/log/bootchart-20220504-1040.svg\n" \ - " /run/log/bootchart-20220504-1045.svg\n" - + "Status Operational Status Samples Frequency Time (sec) Output\n" \ + "-------- -------------------- --------- ----------- ------------ ------------------------------------\n" \ + "enabled enabled 100 50 2 /run/log/bootchart-20220504-1040.svg\n" \ + " /run/log/bootchart-20220504-1045.svg\n" From 03c972163e9ca6f9fc2898b904ba31caa0caf544 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 1 Jun 2022 16:35:29 +0300 Subject: [PATCH 3/7] review comments Signed-off-by: Stepan Blyschak --- scripts/sonic-bootchart | 20 +++++++++++++------- tests/sonic_bootchart_test.py | 18 +++++++++--------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/scripts/sonic-bootchart b/scripts/sonic-bootchart index 93231872c3..d68050d76e 100755 --- a/scripts/sonic-bootchart +++ b/scripts/sonic-bootchart @@ -16,9 +16,13 @@ BOOTCHART_DEFAULT_OUTPUT_DIR = "/run/log/" BOOTCHART_DEFAULT_OUTPUT_GLOB = os.path.join(BOOTCHART_DEFAULT_OUTPUT_DIR, "bootchart-*.svg") class BootChartConfigParser(configparser.ConfigParser): + """ Custom bootchart config parser. Changes the way ConfigParser passes options """ + def optionxform(self, option): + """ Pass options as is, without modifications """ return option + def exit_cli(*args, **kwargs): """ Print a message and exit with rc 1. """ click.secho(*args, **kwargs) @@ -81,14 +85,17 @@ def disable(): @cli.command() -@click.option('--samples', type=int) -@click.option('--frequency', type=int) +@click.option('--time', type=int, required=True) +@click.option('--frequency', type=int, required=True) @root_privileges_required -def config(**kwargs): +def config(time, frequency): """ Configure bootchart """ - config = {k.title(): str(v) for k, v in kwargs.items() if v is not None} - if not config: - exit_cli("Nothing to update") + samples = time * frequency + + config = { + 'Samples': samples, + 'Frequency': frequency + } bootchart_config = BootChartConfigParser() bootchart_config.read(BOOTCHART_CONF) bootchart_config['Bootchart'].update(config) @@ -109,7 +116,6 @@ def show(): field_values = { "Status": get_enabled_status(), "Operational Status": get_active_status(), - "Samples": samples, "Frequency": frequency, "Time (sec)": time, "Output": get_output_files(), diff --git a/tests/sonic_bootchart_test.py b/tests/sonic_bootchart_test.py index 0c87b647b4..5b359a2d62 100755 --- a/tests/sonic_bootchart_test.py +++ b/tests/sonic_bootchart_test.py @@ -63,18 +63,18 @@ def run_command_side_effect(command, **kwargs): result = runner.invoke(sonic_bootchart.cli.commands['show'], []) assert not result.exit_code assert result.output == \ - "Status Operational Status Samples Frequency Time (sec) Output\n" \ - "-------- -------------------- --------- ----------- ------------ ------------------------------------\n" \ - "enabled enabled 500 25 20 /run/log/bootchart-20220504-1040.svg\n" \ - " /run/log/bootchart-20220504-1045.svg\n" + "Status Operational Status Frequency Time (sec) Output\n" \ + "-------- -------------------- ----------- ------------ ------------------------------------\n" \ + "enabled enabled 25 20 /run/log/bootchart-20220504-1040.svg\n" \ + " /run/log/bootchart-20220504-1045.svg\n" - result = runner.invoke(sonic_bootchart.cli.commands["config"], ["--samples", "100", "--frequency", "50"]) + result = runner.invoke(sonic_bootchart.cli.commands["config"], ["--time", "2", "--frequency", "50"]) assert not result.exit_code result = runner.invoke(sonic_bootchart.cli.commands['show'], []) assert not result.exit_code assert result.output == \ - "Status Operational Status Samples Frequency Time (sec) Output\n" \ - "-------- -------------------- --------- ----------- ------------ ------------------------------------\n" \ - "enabled enabled 100 50 2 /run/log/bootchart-20220504-1040.svg\n" \ - " /run/log/bootchart-20220504-1045.svg\n" + "Status Operational Status Frequency Time (sec) Output\n" \ + "-------- -------------------- ----------- ------------ ------------------------------------\n" \ + "enabled enabled 50 2 /run/log/bootchart-20220504-1040.svg\n" \ + " /run/log/bootchart-20220504-1045.svg\n" From 29734bb0745f4c0b65423117db9cf9dfd0453a0c Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Mon, 6 Jun 2022 17:04:50 +0300 Subject: [PATCH 4/7] review comments Signed-off-by: Stepan Blyschak --- scripts/sonic-bootchart | 4 ++-- tests/sonic_bootchart_test.py | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/scripts/sonic-bootchart b/scripts/sonic-bootchart index d68050d76e..6a8b0e9cd7 100755 --- a/scripts/sonic-bootchart +++ b/scripts/sonic-bootchart @@ -85,8 +85,8 @@ def disable(): @cli.command() -@click.option('--time', type=int, required=True) -@click.option('--frequency', type=int, required=True) +@click.option('--time', type=click.IntRange(min=1), required=True) +@click.option('--frequency', type=click.IntRange(min=1), required=True) @root_privileges_required def config(time, frequency): """ Configure bootchart """ diff --git a/tests/sonic_bootchart_test.py b/tests/sonic_bootchart_test.py index 5b359a2d62..b07307f5c8 100755 --- a/tests/sonic_bootchart_test.py +++ b/tests/sonic_bootchart_test.py @@ -50,9 +50,9 @@ def test_disable(self, mock_run_command): def test_config_show(self, mock_run_command): def run_command_side_effect(command, **kwargs): - if "is-enabled": + if "is-enabled" in command: return "enabled" - elif "is-active": + elif "is-active" in command: return "active" else: raise Exception("unknown command") @@ -65,7 +65,7 @@ def run_command_side_effect(command, **kwargs): assert result.output == \ "Status Operational Status Frequency Time (sec) Output\n" \ "-------- -------------------- ----------- ------------ ------------------------------------\n" \ - "enabled enabled 25 20 /run/log/bootchart-20220504-1040.svg\n" \ + "enabled active 25 20 /run/log/bootchart-20220504-1040.svg\n" \ " /run/log/bootchart-20220504-1045.svg\n" result = runner.invoke(sonic_bootchart.cli.commands["config"], ["--time", "2", "--frequency", "50"]) @@ -76,5 +76,13 @@ def run_command_side_effect(command, **kwargs): assert result.output == \ "Status Operational Status Frequency Time (sec) Output\n" \ "-------- -------------------- ----------- ------------ ------------------------------------\n" \ - "enabled enabled 50 2 /run/log/bootchart-20220504-1040.svg\n" \ + "enabled active 50 2 /run/log/bootchart-20220504-1040.svg\n" \ " /run/log/bootchart-20220504-1045.svg\n" + + # Input validation tests + + result = runner.invoke(sonic_bootchart.cli.commands["config"], ["--time", "0", "--frequency", "50"]) + assert result.exit_code + + result = runner.invoke(sonic_bootchart.cli.commands["config"], ["--time", "2", "--frequency", "-5"]) + assert result.exit_code From 8b76cb0aeb3edf8c96da9dfd70f8182b07e14832 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Mon, 6 Jun 2022 18:00:15 +0300 Subject: [PATCH 5/7] fix Signed-off-by: Stepan Blyschak --- scripts/sonic-bootchart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/sonic-bootchart b/scripts/sonic-bootchart index 6a8b0e9cd7..8158d5ae2e 100755 --- a/scripts/sonic-bootchart +++ b/scripts/sonic-bootchart @@ -93,8 +93,8 @@ def config(time, frequency): samples = time * frequency config = { - 'Samples': samples, - 'Frequency': frequency + 'Samples': str(samples), + 'Frequency': str(frequency), } bootchart_config = BootChartConfigParser() bootchart_config.read(BOOTCHART_CONF) From ba49716ebd88dbe29659b0f440bbf1df868aa1cf Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak <38952541+stepanblyschak@users.noreply.github.com> Date: Fri, 10 Jun 2022 12:27:03 +0300 Subject: [PATCH 6/7] Fix issue reported by LGTM --- scripts/sonic-bootchart | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/sonic-bootchart b/scripts/sonic-bootchart index 8158d5ae2e..2d39334260 100755 --- a/scripts/sonic-bootchart +++ b/scripts/sonic-bootchart @@ -3,7 +3,6 @@ import click import sys import configparser -import subprocess import functools import os import glob From a16ee694576b480cceed896bb7c62c444d653707 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 6 Jul 2022 15:53:10 +0300 Subject: [PATCH 7/7] add config parse error handling and corresponding unit tests Signed-off-by: Stepan Blyschak --- scripts/sonic-bootchart | 15 ++++++++++++--- tests/sonic_bootchart_test.py | 36 +++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/scripts/sonic-bootchart b/scripts/sonic-bootchart index 2d39334260..86e993d395 100755 --- a/scripts/sonic-bootchart +++ b/scripts/sonic-bootchart @@ -108,9 +108,18 @@ def show(): bootchart_config = BootChartConfigParser() bootchart_config.read(BOOTCHART_CONF) - samples = int(bootchart_config["Bootchart"]["Samples"]) - frequency = int(bootchart_config["Bootchart"]["Frequency"]) - time = samples // frequency + try: + samples = int(bootchart_config["Bootchart"]["Samples"]) + frequency = int(bootchart_config["Bootchart"]["Frequency"]) + except KeyError as key: + raise click.ClickException(f"Failed to parse bootchart config: {key} not found") + except ValueError as err: + raise click.ClickException(f"Failed to parse bootchart config: {err}") + + try: + time = samples // frequency + except ZeroDivisionError: + raise click.ClickException(f"Invalid frequency value: {frequency}") field_values = { "Status": get_enabled_status(), diff --git a/tests/sonic_bootchart_test.py b/tests/sonic_bootchart_test.py index b07307f5c8..f9ecdab1dc 100755 --- a/tests/sonic_bootchart_test.py +++ b/tests/sonic_bootchart_test.py @@ -86,3 +86,39 @@ def run_command_side_effect(command, **kwargs): result = runner.invoke(sonic_bootchart.cli.commands["config"], ["--time", "2", "--frequency", "-5"]) assert result.exit_code + + def test_invalid_config_show(self, mock_run_command): + with open(sonic_bootchart.BOOTCHART_CONF, 'w') as config_file: + config_file.write(""" + [Bootchart] + Samples=100 + """) + + runner = CliRunner() + result = runner.invoke(sonic_bootchart.cli.commands['show'], []) + assert result.exit_code + assert result.output == "Error: Failed to parse bootchart config: 'Frequency' not found\n" + + with open(sonic_bootchart.BOOTCHART_CONF, 'w') as config_file: + config_file.write(""" + [Bootchart] + Samples=abc + Frequency=def + """) + + runner = CliRunner() + result = runner.invoke(sonic_bootchart.cli.commands['show'], []) + assert result.exit_code + assert result.output == "Error: Failed to parse bootchart config: invalid literal for int() with base 10: 'abc'\n" + + with open(sonic_bootchart.BOOTCHART_CONF, 'w') as config_file: + config_file.write(""" + [Bootchart] + Samples=100 + Frequency=0 + """) + + runner = CliRunner() + result = runner.invoke(sonic_bootchart.cli.commands['show'], []) + assert result.exit_code + assert result.output == "Error: Invalid frequency value: 0\n"