From 86a3f484310fef31a09c7ab9157194141e807a54 Mon Sep 17 00:00:00 2001 From: Sonic Build Admin Date: Tue, 21 Jan 2025 19:56:07 +0000 Subject: [PATCH] [config] Exit with non-zero when qos reload fail Fix issue when 'config qos reload' operation unsuccessful, but returns exit code 0, when operation unsuccessful, now it will return code 1 after executing all the commands. #### What I did When config qos reload failed with error print "Operation not completed successfully", exit with code 1 finally, instead of exit with code 0. #### How I did it Check return value in function _wait_until_clear(), and after config qos reload command finish, check this flag, if it is none zero, then exit 1. #### How to verify it Test with command "config qos reload", will exit 1. Note that "config load_minigraph" is also verified, not impacted. #### Previous command output (if the output of a command-line utility has changed) ```bash # config qos reload Operation not completed successfully, please save and reload configuration. Running command: /usr/local/bin/sonic-cfggen -d -t /usr/share/sonic/device/x86_64-nvidia_sn5600-r0/Mellanox-SN5600-C256S1/buffers_dynamic.json.j2,/tmp/cfg_buffer.json -t /usr/share/sonic/device/x86_64-nvidia_sn5600-r0/Mellanox-SN5600-C256S1/qos.json.j2,/tmp/cfg_qos.json -y /etc/sonic/sonic_version.yml Running command: /usr/local/bin/sonic-cfggen -j /tmp/cfg_buffer.json -j /tmp/cfg_qos.json --write-to-db Buffer calculation model updated, restarting swss is required to take effect ``` #### New command output (if the output of a command-line utility has changed) ```bash # config qos reload Operation not completed successfully, please save and reload configuration. Running command: /usr/local/bin/sonic-cfggen -d -t /usr/share/sonic/device/x86_64-nvidia_sn5600-r0/Mellanox-SN5600-C256S1/buffers_dynamic.json.j2,/tmp/cfg_buffer.json -t /usr/share/sonic/device/x86_64-nvidia_sn5600-r0/Mellanox-SN5600-C256S1/qos.json.j2,/tmp/cfg_qos.json -y /etc/sonic/sonic_version.yml Running command: /usr/local/bin/sonic-cfggen -j /tmp/cfg_buffer.json -j /tmp/cfg_qos.json --write-to-db Buffer calculation model updated, restarting swss is required to take effect # echo $? 1 ``` --- config/main.py | 10 ++++++++-- doc/Command-Reference.md | 1 + tests/config_test.py | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/config/main.py b/config/main.py index 68d9e24c01..2c3b321b47 100644 --- a/config/main.py +++ b/config/main.py @@ -799,6 +799,7 @@ def _wait_until_clear(tables, interval=0.5, timeout=30, verbose=False): def _clear_qos(delay=False, verbose=False): + status = True QOS_TABLE_NAMES = [ 'PORT_QOS_MAP', 'QUEUE', @@ -838,7 +839,8 @@ def _clear_qos(delay=False, verbose=False): device_metadata = config_db.get_entry('DEVICE_METADATA', 'localhost') # Traditional buffer manager do not remove buffer tables in any case, no need to wait. timeout = 120 if device_metadata and device_metadata.get('buffer_model') == 'dynamic' else 0 - _wait_until_clear(["BUFFER_*_TABLE:*", "BUFFER_*_SET"], interval=0.5, timeout=timeout, verbose=verbose) + status = _wait_until_clear(["BUFFER_*_TABLE:*", "BUFFER_*_SET"], interval=0.5, timeout=timeout, verbose=verbose) + return status def _get_sonic_generated_services(num_asic): if not os.path.isfile(SONIC_GENERATED_SERVICE_PATH): @@ -3160,6 +3162,7 @@ def _update_buffer_calculation_model(config_db, model): help="Dry run, writes config to the given file" ) def reload(ctx, no_dynamic_buffer, no_delay, dry_run, json_data, ports, verbose): + status = True """Reload QoS configuration""" if ports: log.log_info("'qos reload --ports {}' executing...".format(ports)) @@ -3168,7 +3171,7 @@ def reload(ctx, no_dynamic_buffer, no_delay, dry_run, json_data, ports, verbose) log.log_info("'qos reload' executing...") if not dry_run: - _clear_qos(delay = not no_delay, verbose=verbose) + status = _clear_qos(delay=not no_delay, verbose=verbose) _, hwsku_path = device_info.get_paths_to_platform_and_hwsku_dirs() sonic_version_file = device_info.get_sonic_version_file() @@ -3251,6 +3254,9 @@ def reload(ctx, no_dynamic_buffer, no_delay, dry_run, json_data, ports, verbose) if buffer_model_updated: print("Buffer calculation model updated, restarting swss is required to take effect") + if not status: + sys.exit(1) + def _qos_update_ports(ctx, ports, dry_run, json_data): """Reload QoS configuration""" _, hwsku_path = device_info.get_paths_to_platform_and_hwsku_dirs() diff --git a/doc/Command-Reference.md b/doc/Command-Reference.md index fdff48d5c7..4dad474cb1 100644 --- a/doc/Command-Reference.md +++ b/doc/Command-Reference.md @@ -9605,6 +9605,7 @@ Some of the example QOS configurations that users can modify are given below. In this example, it uses the buffers.json.j2 file and qos.json.j2 file from platform specific folders. When there are no changes in the platform specific configutation files, they internally use the file "/usr/share/sonic/templates/buffers_config.j2" and "/usr/share/sonic/templates/qos_config.j2" to generate the configuration. + When an error occurs, such as "Operation not completed successfully, please save and reload configuration," the system will record the status, after executing all the latter commands, exit with code 1. ``` **config qos reload --ports port_list** diff --git a/tests/config_test.py b/tests/config_test.py index afce0a86ad..af39aac259 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -1604,6 +1604,46 @@ def test_qos_clear_no_wait(self, _wait_until_clear): _clear_qos(True, False) _wait_until_clear.assert_called_with(['BUFFER_*_TABLE:*', 'BUFFER_*_SET'], interval=0.5, timeout=0, verbose=False) + @mock.patch('config.main._wait_until_clear') + def test_clear_qos_without_delay(self, mock_wait_until_clear): + from config.main import _clear_qos + + status = _clear_qos(False, False) + mock_wait_until_clear.assert_not_called() + assert status is True + + @mock.patch('config.main._wait_until_clear') + def test_clear_qos_with_delay_returns_true(self, mock_wait_until_clear): + from config.main import _clear_qos + mock_wait_until_clear.return_value = True + + status = _clear_qos(True, False) + mock_wait_until_clear.assert_called_once() + assert status is True + + @mock.patch('config.main._wait_until_clear') + def test_clear_qos_with_delay_returns_false(self, mock_wait_until_clear): + from config.main import _clear_qos + mock_wait_until_clear.return_value = False + + status = _clear_qos(True, False) + mock_wait_until_clear.assert_called_once() + assert status is False + + @patch('config.main._wait_until_clear') + def test_qos_reload_not_empty_should_exit(self, mock_wait_until_clear): + mock_wait_until_clear.return_value = False + runner = CliRunner() + output_file = os.path.join(os.sep, "tmp", "qos_config_output.json") + print("Saving output in {}".format(output_file)) + result = runner.invoke( + config.config.commands["qos"], ["reload"] + ) + print(result.exit_code) + print(result.output) + # Expect sys.exit(1) when _wait_until_clear returns False + assert result.exit_code == 1 + def test_qos_reload_single( self, get_cmd_module, setup_qos_mock_apis, setup_single_broadcom_asic