From 738406b7519f29605cf79ee500aafcbc3b97f1b1 Mon Sep 17 00:00:00 2001 From: Stephen Sun <5379172+stephenxs@users.noreply.github.com> Date: Tue, 14 Mar 2023 21:01:52 +0800 Subject: [PATCH] Enhance the logic to wait for all buffer tables to be removed in _clear_qos (#2720) - What I did This is an enhancement of PR #2503 - How I did it On top of waiting for BUFFER_POOL_TABLE to be cleared from APPL_DB, we need to wait for KEY_SET and DEL_SET as well. KEY_SET and DEL_SET are designed to accommodate the APPL_DB entries that were updated by manager daemons but have not yet been handled by the orchagent. In this case, even if the buffer tables are empty, entries in KEY_SET or DEL_SET will be in the buffer tables later on. So, we need to wait for key set tables as well. Do not delay for traditional buffer manager because it does not remove any buffer table. Provide a CLI option to print the detailed message if there is any table item which still exists - How to verify it Manually test and unit test - Previous command output (if the output of a command-line utility has changed) Running command: /usr/local/bin/sonic-cfggen -d --write-to-db -t /usr/share/sonic/device/x86_64-mlnx_msn2410-r0/ACS-MSN2410/buffers_dynamic.json.j2,config-db -t /usr/share/sonic/device/x86_64-mlnx_msn2410-r0/ACS-MSN2410/qos.json.j2,config-db -y /etc/sonic/sonic_version.yml - New command output (if the output of a command-line utility has changed) Only with option --verbose there are new output. Without the option, the output is the same as it is. admin@mtbc-sonic-01-2410:~$ sudo config qos reload --verbose Some entries matching BUFFER_*_TABLE:* still exist: BUFFER_QUEUE_TABLE:Ethernet108:0-2 Some entries matching BUFFER_*_SET still exist: BUFFER_PG_TABLE_KEY_SET Some entries matching BUFFER_*_TABLE:* still exist: BUFFER_QUEUE_TABLE:Ethernet108:0-2 Some entries matching BUFFER_*_SET still exist: BUFFER_PG_TABLE_KEY_SET Some entries matching BUFFER_*_TABLE:* still exist: BUFFER_QUEUE_TABLE:Ethernet108:0-2 Running command: /usr/local/bin/sonic-cfggen -d --write-to-db -t /usr/share/sonic/device/x86_64-mlnx_msn2410-r0/ACS-MSN2410/buffers_dynamic.json.j2,config-db -t /usr/share/sonic/device/x86_64-mlnx_msn2410-r0/ACS-MSN2410/qos.json.j2,config-db -y /etc/sonic/sonic_version.yml --- config/main.py | 33 +++++++++++++++++++++------------ tests/config_test.py | 10 ++++++++-- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/config/main.py b/config/main.py index c167bbc3fe..a6df174d1b 100644 --- a/config/main.py +++ b/config/main.py @@ -652,24 +652,28 @@ def _clear_cbf(): config_db.delete_table(cbf_table) -def _wait_until_clear(table, interval=0.5, timeout=30): +def _wait_until_clear(tables, interval=0.5, timeout=30, verbose=False): start = time.time() empty = False app_db = SonicV2Connector(host='127.0.0.1') app_db.connect(app_db.APPL_DB) while not empty and time.time() - start < timeout: - current_profiles = app_db.keys(app_db.APPL_DB, table) - if not current_profiles: - empty = True - else: - time.sleep(interval) + non_empty_table_count = 0 + for table in tables: + keys = app_db.keys(app_db.APPL_DB, table) + if keys: + non_empty_table_count += 1 + if verbose: + click.echo("Some entries matching {} still exist: {}".format(table, keys[0])) + time.sleep(interval) + empty = (non_empty_table_count == 0) if not empty: click.echo("Operation not completed successfully, please save and reload configuration.") return empty -def _clear_qos(delay = False): +def _clear_qos(delay=False, verbose=False): QOS_TABLE_NAMES = [ 'PORT_QOS_MAP', 'QUEUE', @@ -706,7 +710,10 @@ def _clear_qos(delay = False): for qos_table in QOS_TABLE_NAMES: config_db.delete_table(qos_table) if delay: - _wait_until_clear("BUFFER_POOL_TABLE:*",interval=0.5, timeout=30) + 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) def _get_sonic_generated_services(num_asic): if not os.path.isfile(SONIC_GENERATED_SERVICE_PATH): @@ -2449,10 +2456,11 @@ def qos(ctx): pass @qos.command('clear') -def clear(): +@click.option('--verbose', is_flag=True, help="Enable verbose output") +def clear(verbose): """Clear QoS configuration""" log.log_info("'qos clear' executing...") - _clear_qos() + _clear_qos(verbose=verbose) def _update_buffer_calculation_model(config_db, model): """Update the buffer calculation model into CONFIG_DB""" @@ -2469,6 +2477,7 @@ def _update_buffer_calculation_model(config_db, model): @click.option('--ports', is_flag=False, required=False, help="List of ports that needs to be updated") @click.option('--no-dynamic-buffer', is_flag=True, help="Disable dynamic buffer calculation") @click.option('--no-delay', is_flag=True, hidden=True) +@click.option('--verbose', is_flag=True, help="Enable verbose output") @click.option( '--json-data', type=click.STRING, help="json string with additional data, valid with --dry-run option" @@ -2477,7 +2486,7 @@ def _update_buffer_calculation_model(config_db, model): '--dry_run', type=click.STRING, help="Dry run, writes config to the given file" ) -def reload(ctx, no_dynamic_buffer, no_delay, dry_run, json_data, ports): +def reload(ctx, no_dynamic_buffer, no_delay, dry_run, json_data, ports, verbose): """Reload QoS configuration""" if ports: log.log_info("'qos reload --ports {}' executing...".format(ports)) @@ -2486,7 +2495,7 @@ def reload(ctx, no_dynamic_buffer, no_delay, dry_run, json_data, ports): log.log_info("'qos reload' executing...") if not dry_run: - _clear_qos(delay = not no_delay) + _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() diff --git a/tests/config_test.py b/tests/config_test.py index a19d16d6f9..692e125a77 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -658,7 +658,7 @@ def test_qos_wait_until_clear_empty(self): with mock.patch('swsscommon.swsscommon.SonicV2Connector.keys', side_effect=TestConfigQos._keys): TestConfigQos._keys_counter = 1 - empty = _wait_until_clear("BUFFER_POOL_TABLE:*", 0.5,2) + empty = _wait_until_clear(["BUFFER_POOL_TABLE:*"], 0.5,2) assert empty def test_qos_wait_until_clear_not_empty(self): @@ -666,9 +666,15 @@ def test_qos_wait_until_clear_not_empty(self): with mock.patch('swsscommon.swsscommon.SonicV2Connector.keys', side_effect=TestConfigQos._keys): TestConfigQos._keys_counter = 10 - empty = _wait_until_clear("BUFFER_POOL_TABLE:*", 0.5,2) + empty = _wait_until_clear(["BUFFER_POOL_TABLE:*"], 0.5,2) assert not empty + @mock.patch('config.main._wait_until_clear') + def test_qos_clear_no_wait(self, _wait_until_clear): + from config.main import _clear_qos + _clear_qos(True, False) + _wait_until_clear.assert_called_with(['BUFFER_*_TABLE:*', 'BUFFER_*_SET'], interval=0.5, timeout=0, verbose=False) + def test_qos_reload_single( self, get_cmd_module, setup_qos_mock_apis, setup_single_broadcom_asic