Skip to content

Commit

Permalink
Enhance the logic to wait for all buffer tables to be removed in _cle…
Browse files Browse the repository at this point in the history
…ar_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
  • Loading branch information
stephenxs authored Mar 14, 2023
1 parent e6179af commit f7f783b
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 14 deletions.
33 changes: 21 additions & 12 deletions config/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -743,24 +743,28 @@ 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, 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',
Expand Down Expand Up @@ -797,7 +801,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):
Expand Down Expand Up @@ -2644,10 +2651,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"""
Expand All @@ -2664,6 +2672,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"
Expand All @@ -2672,7 +2681,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))
Expand All @@ -2681,7 +2690,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()
Expand Down
10 changes: 8 additions & 2 deletions tests/config_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -693,17 +693,23 @@ 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):
from config.main import _wait_until_clear

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
Expand Down

0 comments on commit f7f783b

Please sign in to comment.