From c23761909bdf49c2641e621e57b93639463a4327 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 16 Nov 2020 18:18:04 +0800 Subject: [PATCH] Fix all review comments and support python 3 Review comments: - Add testcases for show and config command - Address review comments in db_migrator - Use "size" to represent the size of the PG and "headroom" for xoff - Fix typo Signed-off-by: Stephen Sun --- config/main.py | 34 +++---- doc/Command-Reference.md | 14 +-- scripts/db_migrator.py | 16 ++-- scripts/mmuconfig | 13 ++- show/main.py | 9 +- tests/buffer_input/buffer_test_vectors.py | 108 ++++++++++++++++++++++ tests/buffer_test.py | 46 ++++++++- tests/mock_tables/config_db.json | 4 +- tests/mock_tables/state_db.json | 32 +++++++ 9 files changed, 231 insertions(+), 45 deletions(-) create mode 100644 tests/buffer_input/buffer_test_vectors.py diff --git a/config/main.py b/config/main.py index 90967efd00..122ca960f0 100644 --- a/config/main.py +++ b/config/main.py @@ -1704,7 +1704,7 @@ def reload(ctx, no_dynamic_buffer): ), fg="yellow") if buffer_model_updated: - print "Buffer calculation model updated, restarting swss is required to take effect" + print("Buffer calculation model updated, restarting swss is required to take effect") def is_dynamic_buffer_enabled(config_db): """Return whether the current system supports dynamic buffer calculation""" @@ -2584,7 +2584,7 @@ def pgmaps_check_legality(ctx, interface_name, input_pg, is_new_pg): ctx.fail("PG {} doesn't exist".format(input_pg)) return - for k, v in existing_pgs.iteritems(): + for k, v in existing_pgs.items(): port, existing_pg = k if port == interface_name: existing_lower = int(existing_pg[0]) @@ -2633,7 +2633,7 @@ def remove_pg_on_port(ctx, interface_name, pg_map): # Remvoe all dynamic lossless PGs on the port existing_pgs = config_db.get_table("BUFFER_PG") removed = False - for k, v in existing_pgs.iteritems(): + for k, v in existing_pgs.items(): port, existing_pg = k if port == interface_name and (not pg_map or pg_map == existing_pg): need_to_remove = False @@ -3372,7 +3372,7 @@ def priority(ctx, interface_name, priority, status): clicommon.run_command("pfc config priority {0} {1} {2}".format(status, interface_name, priority)) # -# 'buffer_profile' group ('config buffer_profile ...') +# 'buffer' group ('config buffer ...') # @config.group(cls=clicommon.AbbreviationGroup) @@ -3400,11 +3400,11 @@ def profile(ctx): @click.option('--size', metavar='', type=int, help="Set reserved size size") @click.option('--dynamic_th', metavar='', type=str, help="Set dynamic threshold") @click.option('--pool', metavar='', type=str, help="Buffer pool") -@click.pass_context -def add_profile(ctx, profile, xon, xoff, size, dynamic_th, pool): +@clicommon.pass_db +def add_profile(db, profile, xon, xoff, size, dynamic_th, pool): """Add or modify a buffer profile""" - config_db = ConfigDBConnector() - config_db.connect() + config_db = db.cfgdb + ctx = click.get_current_context() profile_entry = config_db.get_entry('BUFFER_PROFILE', profile) if profile_entry: @@ -3420,11 +3420,11 @@ def add_profile(ctx, profile, xon, xoff, size, dynamic_th, pool): @click.option('--size', metavar='', type=int, help="Set reserved size size") @click.option('--dynamic_th', metavar='', type=str, help="Set dynamic threshold") @click.option('--pool', metavar='', type=str, help="Buffer pool") -@click.pass_context -def set_profile(ctx, profile, xon, xoff, size, dynamic_th, pool): +@clicommon.pass_db +def set_profile(db, profile, xon, xoff, size, dynamic_th, pool): """Add or modify a buffer profile""" - config_db = ConfigDBConnector() - config_db.connect() + config_db = db.cfgdb + ctx = click.get_current_context() profile_entry = config_db.get_entry('BUFFER_PROFILE', profile) if not profile_entry: @@ -3497,15 +3497,15 @@ def update_profile(ctx, config_db, profile_name, xon, xoff, size, dynamic_th, po @profile.command('remove') @click.argument('profile', metavar='', required=True) -@click.pass_context -def remove_profile(ctx, profile): +@clicommon.pass_db +def remove_profile(db, profile): """Delete a buffer profile""" - config_db = ConfigDBConnector() - config_db.connect() + config_db = db.cfgdb + ctx = click.get_current_context() full_profile_name = '[BUFFER_PROFILE|{}]'.format(profile) existing_pgs = config_db.get_table("BUFFER_PG") - for k, v in existing_pgs.iteritems(): + for k, v in existing_pgs.items(): port, pg = k referenced_profile = v.get('profile') if referenced_profile and referenced_profile == full_profile_name: diff --git a/doc/Command-Reference.md b/doc/Command-Reference.md index bf6366fc2a..8f8c8c83d5 100644 --- a/doc/Command-Reference.md +++ b/doc/Command-Reference.md @@ -2365,18 +2365,18 @@ This command is used to configure a lossless buffer profile. - Usage: ``` - config buffer_profile add -xon -xoff [-headroom ] [-dynamic_th ] [-pool ] - config buffer_profile set -xon -xoff [-headroom ] [-dynamic_th ] [-pool ] + config buffer_profile add -xon -xoff [-size ] [-dynamic_th ] [-pool ] + config buffer_profile set -xon -xoff [-size ] [-dynamic_th ] [-pool ] config buffer_profile remove ``` All the parameters are devided to two groups, one for headroom and one for dynamic_th. For any command at lease one group of parameters should be provided. For headroom parameters: - - At lease one of `xoff` and `headroom` should be provided and the other will be optional and conducted via the formula `xon + xoff = headroom`. + - At lease one of `xoff` and `size` should be provided and the other will be optional and conducted via the formula `xon + xoff = size`. All other parameters are optional. - `xon` is madantory. - - `xon` + `xoff` <= `headroom`; For Mellanox platform xon + xoff == headroom + - `xon` + `xoff` <= `size`; For Mellanox platform xon + xoff == size If only headroom parameters are provided, the `dynamic_th` will be taken from `CONFIG_DB.DEFAULT_LOSSLESS_BUFFER_PARAMETER.default_dynamic_th`. @@ -2578,20 +2578,20 @@ This command is used to display the status of buffer pools and profiles currentl ---------- -------------------------------- ``` -**show buffer configure** +**show buffer configuration** This command is used to display the status of buffer pools and profiles currently configured. - Usage: ``` - show buffer configure + show buffer configuration ``` - Example: ``` - admin@sonic:~$ show buffer configure + admin@sonic:~$ show buffer configuration Pool: ingress_lossless_pool ---- -------- type ingress diff --git a/scripts/db_migrator.py b/scripts/db_migrator.py index e3fd033591..5b571ce758 100755 --- a/scripts/db_migrator.py +++ b/scripts/db_migrator.py @@ -188,8 +188,8 @@ def migrate_config_db_buffer_tables_for_dynamic_calculation(self, speed_list, ca ''' # Migrate BUFFER_PROFILEs, removing dynamically generated profiles dynamic_profile = self.configDB.get_table('BUFFER_PROFILE') - profile_pattern = 'pg_lossless_([0-9]*000)_([0-9]*m)_profile' - for name, info in dynamic_profile.iteritems(): + profile_pattern = 'pg_lossless_([1-9][0-9]*000)_([1-9][0-9]*m)_profile' + for name, info in dynamic_profile.items(): m = re.search(profile_pattern, name) if not m: continue @@ -212,12 +212,12 @@ def migrate_config_db_buffer_tables_for_dynamic_calculation(self, speed_list, ca ports = self.configDB.get_table('PORT') all_cable_lengths = self.configDB.get_table('CABLE_LENGTH') if not buffer_pgs or not ports or not all_cable_lengths: - log.log_notice("At lease one of tables BUFFER_PG, PORT and CABLE_LENGTH hasn't been defined, skip following mitration") + log.log_notice("At lease one of tables BUFFER_PG, PORT and CABLE_LENGTH hasn't been defined, skip following migration") abandon_method() return True - cable_lengths = all_cable_lengths[all_cable_lengths.keys()[0]] - for name, profile in buffer_pgs.iteritems(): + cable_lengths = all_cable_lengths[list(all_cable_lengths.keys())[0]] + for name, profile in buffer_pgs.items(): # do the db migration port, pg = name if pg != '3-4': @@ -278,7 +278,7 @@ def prepare_dynamic_buffer_for_warm_reboot(self, buffer_pools = None, buffer_pro warmreboot_state = self.stateDB.get(self.stateDB.STATE_DB, 'WARM_RESTART_ENABLE_TABLE|system', 'enable') mmu_size = self.stateDB.get(self.stateDB.STATE_DB, 'BUFFER_MAX_PARAM_TABLE|global', 'mmu_size') if warmreboot_state == 'true' and not mmu_size: - log.log_notice("This is the very first run of buffermgrd (dynamc), prepare info requred from warm reboot") + log.log_notice("This is the very first run of buffermgrd (dynamic), prepare info required from warm reboot") else: return True @@ -298,7 +298,7 @@ def prepare_dynamic_buffer_for_warm_reboot(self, buffer_pools = None, buffer_pro app_table_name = table_name + "_TABLE" if not entries: entries = self.configDB.get_table(table_name) - for key, items in entries.iteritems(): + for key, items in entries.items(): # copy items to appl db if reference_field_name: confdb_ref = items.get(reference_field_name) @@ -328,7 +328,7 @@ def prepare_dynamic_buffer_for_warm_reboot(self, buffer_pools = None, buffer_pro appl_db_key = app_table_name + ':' + ':'.join(key) else: appl_db_key = app_table_name + ':' + key - for field, data in items.iteritems(): + for field, data in items.items(): self.appDB.set(self.appDB.APPL_DB, appl_db_key, field, data) if keys_copied: diff --git a/scripts/mmuconfig b/scripts/mmuconfig index 3ccae34a60..e44fcf0e4d 100755 --- a/scripts/mmuconfig +++ b/scripts/mmuconfig @@ -29,6 +29,17 @@ BUFFER_PROFILE_FIELDS = { "alpha": DYNAMIC_THRESHOLD } +# mock the redis for unit test purposes # +try: + if os.environ["UTILITIES_UNIT_TESTING"] == "2": + modules_path = os.path.join(os.path.dirname(__file__), "..") + tests_path = os.path.join(modules_path, "tests") + sys.path.insert(0, modules_path) + sys.path.insert(0, tests_path) + import mock_tables.dbconnector + +except KeyError: + pass class MmuConfig(object): def __init__(self, verbose, config): @@ -111,7 +122,6 @@ class MmuConfig(object): def main(config): if config: parser = argparse.ArgumentParser(description='Show and change: mmu configuration', - version='1.0.0', formatter_class=argparse.RawTextHelpFormatter) parser.add_argument('-l', '--list', action='store_true', help='show mmu configuration') @@ -120,7 +130,6 @@ def main(config): parser.add_argument('-v', '--version', action='version', version='%(prog)s 1.0') else: parser = argparse.ArgumentParser(description='Show buffer state', - version='1.0.0', formatter_class=argparse.RawTextHelpFormatter) parser.add_argument('-l', '--list', action='store_true', help='show buffer state') diff --git a/show/main.py b/show/main.py index 5dc30698b0..f86b936505 100644 --- a/show/main.py +++ b/show/main.py @@ -1437,8 +1437,7 @@ def boot(): def mmu(): """Show mmu configuration""" cmd = "mmuconfig -l" - proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, shell=True, text=True) - click.echo(proc.stdout.read()) + run_command(cmd) # # 'buffer' command ("show buffer") @@ -1455,8 +1454,7 @@ def buffer(): def configuration(): """show buffer configuration""" cmd = "mmuconfig -l" - proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, shell=True) - click.echo(proc.stdout.read()) + run_command(cmd) # # 'information' command ("show buffer state") @@ -1465,8 +1463,7 @@ def configuration(): def information(): """show buffer information""" cmd = "buffershow -l" - proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, shell=True) - click.echo(proc.stdout.read()) + run_command(cmd) # diff --git a/tests/buffer_input/buffer_test_vectors.py b/tests/buffer_input/buffer_test_vectors.py new file mode 100644 index 0000000000..82f0a09a35 --- /dev/null +++ b/tests/buffer_input/buffer_test_vectors.py @@ -0,0 +1,108 @@ +show_buffer_configuration="""\ +Pool: egress_lossless_pool +---- -------- +mode dynamic +size 13945824 +type egress +---- -------- + +Pool: egress_lossy_pool +---- ------- +mode dynamic +type egress +---- ------- + +Pool: ingress_lossless_pool +---- ------- +mode dynamic +type ingress +---- ------- + +Pool: ingress_lossy_pool +---- ------- +mode dynamic +type ingress +---- ------- + +Profile: ingress_lossy_profile +---------- -------------------------------- +dynamic_th 3 +pool [BUFFER_POOL|ingress_lossy_pool] +size 0 +---------- -------------------------------- + +Profile: headroom_profile +---------- ----------------------------------- +dynamic_th 0 +pool [BUFFER_POOL|ingress_lossless_pool] +xon 18432 +xoff 32768 +size 51200 +---------- ----------------------------------- + +Profile: alpha_profile +------------- ----------------------------------- +dynamic_th 0 +pool [BUFFER_POOL|ingress_lossless_pool] +headroom_type dynamic +------------- ----------------------------------- + +""" + +show_buffer_information_output="""\ +Pool: egress_lossless_pool +---- -------- +mode dynamic +size 13945824 +type egress +---- -------- + +Pool: egress_lossy_pool +---- ------- +mode dynamic +size 4580864 +type egress +---- ------- + +Pool: ingress_lossless_pool +---- ------- +mode dynamic +size 4580864 +type ingress +---- ------- + +Pool: ingress_lossy_pool +---- ------- +mode dynamic +size 4580864 +type ingress +---- ------- + +Profile: ingress_lossy_profile +---------- -------------------------------------- +dynamic_th 3 +pool [BUFFER_POOL_TABLE|ingress_lossy_pool] +size 0 +---------- -------------------------------------- + +Profile: headroom_profile +---------- ----------------------------------------- +dynamic_th 0 +pool [BUFFER_POOL_TABLE|ingress_lossless_pool] +xon 18432 +xoff 32768 +size 51200 +---------- ----------------------------------------- + +""" + +testData = { + 'show_buffer_configuration' : [ {'cmd' : ['buffer', 'configuration'], + 'rc_output': show_buffer_configuration + } + ], + 'show_buffer_information' : [ {'cmd' : ['buffer', 'information'], + 'rc_output': show_buffer_information_output + } + ] + } diff --git a/tests/buffer_test.py b/tests/buffer_test.py index 2c315d608d..cf32f39d55 100644 --- a/tests/buffer_test.py +++ b/tests/buffer_test.py @@ -1,18 +1,29 @@ +import imp import os import sys from click.testing import CliRunner from unittest import TestCase from swsssdk import ConfigDBConnector -import mock_tables.dbconnector +from .mock_tables import dbconnector +import show.main as show import config.main as config from utilities_common.db import Db +from .buffer_input.buffer_test_vectors import * + +test_path = os.path.dirname(os.path.abspath(__file__)) +modules_path = os.path.dirname(test_path) +scripts_path = os.path.join(modules_path, "scripts") +sys.path.insert(0, test_path) +sys.path.insert(0, modules_path) + class TestBuffer(object): @classmethod def setup_class(cls): - os.environ['UTILITIES_UNIT_TESTING'] = "1" + os.environ["PATH"] += os.pathsep + scripts_path + os.environ['UTILITIES_UNIT_TESTING'] = "2" print("SETUP") def setUp(self): @@ -23,19 +34,25 @@ def setUp(self): def test_config_buffer_profile_headroom(self): runner = CliRunner() + db = Db() result = runner.invoke(config.config.commands["buffer"].commands["profile"].commands["add"], - ["testprofile", "--dynamic_th", "3", "--xon", "18432", "--xoff", "32768"]) + ["testprofile", "--dynamic_th", "3", "--xon", "18432", "--xoff", "32768"], obj=db) print(result.exit_code) print(result.output) assert result.exit_code == 0 + profile = db.cfgdb.get_entry('BUFFER_PROFILE', 'testprofile') + assert profile == {'dynamic_th': '3', 'pool': '[BUFFER_POOL|ingress_lossless_pool]', 'xon': '18432', 'xoff': '32768', 'size': '51200'} def test_config_buffer_profile_dynamic_th(self): runner = CliRunner() + db = Db() result = runner.invoke(config.config.commands["buffer"].commands["profile"].commands["add"], - ["testprofile", "--dynamic_th", "3"]) + ["testprofile", "--dynamic_th", "3"], obj=db) print(result.exit_code) print(result.output) assert result.exit_code == 0 + profile = db.cfgdb.get_entry('BUFFER_PROFILE', 'testprofile') + assert profile == {'dynamic_th': '3', 'pool': '[BUFFER_POOL|ingress_lossless_pool]', 'headroom_type': 'dynamic'} def test_config_buffer_profile_add_existing(self): runner = CliRunner() @@ -64,7 +81,28 @@ def test_config_buffer_profile_add_headroom_to_dynamic_profile(self): assert result.exit_code != 0 assert "Can't change profile alpha_profile from dynamically calculating headroom to non-dynamically one" in result.output + def test_show_buffer_configuration(self): + self.executor(testData['show_buffer_configuration']) + + def test_show_buffer_information(self): + self.executor(testData['show_buffer_information']) + + def executor(self, testcase): + runner = CliRunner() + + for input in testcase: + exec_cmd = show.cli.commands[input['cmd'][0]].commands[input['cmd'][1]] + + result = runner.invoke(exec_cmd, []) + + print(result.exit_code) + print(result.output) + + assert result.exit_code == 0 + assert result.output == input['rc_output'] + @classmethod def teardown_class(cls): + os.environ["PATH"] = os.pathsep.join(os.environ["PATH"].split(os.pathsep)[:-1]) os.environ['UTILITIES_UNIT_TESTING'] = "0" print("TEARDOWN") diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index 982a512b6a..c80187dec9 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -1363,7 +1363,9 @@ "size": "51200" }, "BUFFER_PROFILE|alpha_profile": { - "dynamic_th": "0" + "dynamic_th": "0", + "pool": "[BUFFER_POOL|ingress_lossless_pool]", + "headroom_type": "dynamic" }, "BUFFER_PG|Ethernet0|3-4": { "profile": "NULL" diff --git a/tests/mock_tables/state_db.json b/tests/mock_tables/state_db.json index b8cd11315e..e489470bc7 100644 --- a/tests/mock_tables/state_db.json +++ b/tests/mock_tables/state_db.json @@ -327,5 +327,37 @@ }, "MUX_CABLE_TABLE|Ethernet12": { "state": "unknown" + }, + "BUFFER_POOL_TABLE|egress_lossless_pool": { + "mode": "dynamic", + "size": "13945824", + "type": "egress" + }, + "BUFFER_POOL_TABLE|egress_lossy_pool": { + "mode": "dynamic", + "size": "4580864", + "type": "egress" + }, + "BUFFER_POOL_TABLE|ingress_lossless_pool": { + "mode": "dynamic", + "size": "4580864", + "type": "ingress" + }, + "BUFFER_POOL_TABLE|ingress_lossy_pool": { + "mode": "dynamic", + "size": "4580864", + "type": "ingress" + }, + "BUFFER_PROFILE_TABLE|ingress_lossy_profile": { + "dynamic_th": "3", + "pool": "[BUFFER_POOL_TABLE|ingress_lossy_pool]", + "size": "0" + }, + "BUFFER_PROFILE_TABLE|headroom_profile": { + "dynamic_th": "0", + "pool": "[BUFFER_POOL_TABLE|ingress_lossless_pool]", + "xon": "18432", + "xoff": "32768", + "size": "51200" } }