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 <[email protected]>

Author: stephenxs

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='<size>', type=int, help="Set reserved size size")
 @click.option('--dynamic_th', metavar='<dynamic_th>', type=str, help="Set dynamic threshold")
 @click.option('--pool', metavar='<pool>', 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='<size>', type=int, help="Set reserved size size")
 @click.option('--dynamic_th', metavar='<dynamic_th>', type=str, help="Set dynamic threshold")
 @click.option('--pool', metavar='<pool>', 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='<profile>', 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:

doc/Command-Reference.md

@@ -2365,18 +2365,18 @@ This command is used to configure a lossless buffer profile.
 - Usage:
 
   ```
-  config buffer_profile add <profile_name> -xon <xon_threshold> -xoff <xoff_threshold> [-headroom <headroom_size>] [-dynamic_th <dynamic_th_value>] [-pool <ingress_lossless_pool_name>]
-  config buffer_profile set <profile_name> -xon <xon_threshold> -xoff <xoff_threshold> [-headroom <headroom_size>] [-dynamic_th <dynamic_th_value>] [-pool <ingress_lossless_pool_name>]
+  config buffer_profile add <profile_name> -xon <xon_threshold> -xoff <xoff_threshold> [-size <size>] [-dynamic_th <dynamic_th_value>] [-pool <ingress_lossless_pool_name>]
+  config buffer_profile set <profile_name> -xon <xon_threshold> -xoff <xoff_threshold> [-size <size>] [-dynamic_th <dynamic_th_value>] [-pool <ingress_lossless_pool_name>]
   config buffer_profile remove <profile_name>
   ```
 
   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

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:

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')

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)
 
 
 #

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
+                                      }
+                                    ]
+           }