From 368bc637c79b0070085c63ac88e186f9c1be4f8e Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 7 Mar 2023 11:24:43 +0000 Subject: [PATCH 1/5] Enhance the logic to wait for all buffer tables to be removed in _clear_qos On top of waiting for BUFFER_POOL_TABLE to be cleared, we need to wait for key sets as well Signed-off-by: Stephen Sun --- config/main.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/config/main.py b/config/main.py index f2576ac74d..4b0fd2994f 100644 --- a/config/main.py +++ b/config/main.py @@ -743,18 +743,20 @@ def storm_control_delete_entry(port_name, storm_type): return True -def _wait_until_clear(table, interval=0.5, timeout=30): +def _wait_until_clear(tables, interval=0.5, timeout=30): 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 + 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 @@ -797,7 +799,7 @@ 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) + _wait_until_clear(["BUFFER_POOL_TABLE:*", "BUFFER_*_SET"],interval=0.5, timeout=30) def _get_sonic_generated_services(num_asic): if not os.path.isfile(SONIC_GENERATED_SERVICE_PATH): From 9497ca262a6c98a8f31d7449ee2ef836fb015b12 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 7 Mar 2023 14:12:15 +0000 Subject: [PATCH 2/5] Extend timeout to 120 seconds Signed-off-by: Stephen Sun --- config/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/main.py b/config/main.py index 4b0fd2994f..9a1f3e46af 100644 --- a/config/main.py +++ b/config/main.py @@ -799,7 +799,7 @@ 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:*", "BUFFER_*_SET"],interval=0.5, timeout=30) + _wait_until_clear(["BUFFER_POOL_TABLE:*", "BUFFER_*_SET"], interval=0.5, timeout=120) def _get_sonic_generated_services(num_asic): if not os.path.isfile(SONIC_GENERATED_SERVICE_PATH): From 229d69aa84998e839b7ed7cbab616335a87e1061 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Wed, 8 Mar 2023 01:27:11 +0000 Subject: [PATCH 3/5] Enhance more 1. do not wait for traditional buffer manager 2. wait for all buffer tables Signed-off-by: Stephen Sun --- config/main.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/config/main.py b/config/main.py index 9a1f3e46af..e723972dbb 100644 --- a/config/main.py +++ b/config/main.py @@ -799,7 +799,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:*", "BUFFER_*_SET"], interval=0.5, timeout=120) + 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) def _get_sonic_generated_services(num_asic): if not os.path.isfile(SONIC_GENERATED_SERVICE_PATH): From dd5bfa032cb6541bbba0a845a3abecba59384ab5 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Wed, 8 Mar 2023 05:27:20 +0000 Subject: [PATCH 4/5] Add verbose output to avoid keep user waiting Signed-off-by: Stephen Sun --- config/main.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/config/main.py b/config/main.py index e723972dbb..940c2bf4aa 100644 --- a/config/main.py +++ b/config/main.py @@ -743,7 +743,7 @@ def storm_control_delete_entry(port_name, storm_type): return True -def _wait_until_clear(tables, 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') @@ -755,6 +755,8 @@ def _wait_until_clear(tables, interval=0.5, timeout=30): 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: @@ -762,7 +764,7 @@ def _wait_until_clear(tables, interval=0.5, timeout=30): return empty -def _clear_qos(delay = False): +def _clear_qos(delay=False, verbose=False): QOS_TABLE_NAMES = [ 'PORT_QOS_MAP', 'QUEUE', @@ -802,7 +804,7 @@ def _clear_qos(delay = 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) + _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): @@ -2623,10 +2625,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""" @@ -2643,6 +2646,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" @@ -2651,7 +2655,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)) @@ -2660,7 +2664,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() From 64b8619cc0ce216fdf6abeac4d85cd6826597c00 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Thu, 9 Mar 2023 00:20:25 +0000 Subject: [PATCH 5/5] Enhance the UT to satisfy the coverage requirement Signed-off-by: Stephen Sun --- tests/config_test.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/config_test.py b/tests/config_test.py index 5fa50abd00..fff66d47e6 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -693,7 +693,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): @@ -701,9 +701,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