Fix multi-asic behaviour for ecnconfig (#3062)

* Fixes multi-asic behaviour for ecnconfig.

Previously, ecnconfig -l was not behaving correctly on multi-asic devices,
as the '-n' namespace option was not available, and correct namespaces were
not traversed on multi-asic devices to retrieve correct results for ecnconfig.
This change fixes multi-asic behaviour of CLI commands that rely on ecnconfig.

* tests cleanup

* Remove wred on lossy

* Addendum to previous commit regarding wred lossy

* Enhancements to multi-asic support in ecnconfig

- Removed unix socket and fixed failing unit test
- Replace argparse with click
- Add multi-asic support for get and set queue
- Add multi-asic support for set threshold and prob
- Modify test framework to support multi-asic
- Use multi_asic decorators
- Resolve precommit errors

---------

Co-authored-by: rdjeric <[email protected]>
Co-authored-by: arista-hpandya <[email protected]>

config/main.py

@@ -6407,7 +6407,8 @@ def remove_reasons(counter_name, reasons, verbose):
 @click.option('-ydrop', metavar='<yellow drop probability>', type=click.IntRange(0, 100), help="Set yellow drop probability")
 @click.option('-gdrop', metavar='<green drop probability>', type=click.IntRange(0, 100), help="Set green drop probability")
 @click.option('-v', '--verbose', is_flag=True, help="Enable verbose output")
-def ecn(profile, rmax, rmin, ymax, ymin, gmax, gmin, rdrop, ydrop, gdrop, verbose):
+@multi_asic_util.multi_asic_click_option_namespace
+def ecn(profile, rmax, rmin, ymax, ymin, gmax, gmin, rdrop, ydrop, gdrop, verbose, namespace):
     """ECN-related configuration tasks"""
     log.log_info("'ecn -profile {}' executing...".format(profile))
     command = ['ecnconfig', '-p', str(profile)]
@@ -6421,6 +6422,8 @@ def ecn(profile, rmax, rmin, ymax, ymin, gmax, gmin, rdrop, ydrop, gdrop, verbos
     if ydrop is not None: command += ['-ydrop', str(ydrop)]
     if gdrop is not None: command += ['-gdrop', str(gdrop)]
     if verbose: command += ["-vv"]
+    if namespace is not None:
+        command += ['-n', str(namespace)]
     clicommon.run_command(command, display_cmd=verbose)
 
 

show/main.py

@@ -2154,10 +2154,13 @@ def policer(policer_name, verbose):
 # 'ecn' command ("show ecn")
 #
 @cli.command('ecn')
+@multi_asic_util.multi_asic_click_option_namespace
 @click.option('--verbose', is_flag=True, help="Enable verbose output")
-def ecn(verbose):
+def ecn(namespace, verbose):
     """Show ECN configuration"""
     cmd = ['ecnconfig', '-l']
+    if namespace is not None:
+        cmd += ['-n', str(namespace)]
     run_command(cmd, display_cmd=verbose)
 
 

tests/ecn_test.py

@@ -6,25 +6,123 @@
 from click.testing import CliRunner
 
 import config.main as config
-from .ecn_input.ecn_test_vectors import *
+from .ecn_input.ecn_test_vectors import testData
 from .utils import get_result_and_return_code
 from utilities_common.db import Db
 import show.main as show
 
+# Constants
+ARGS_DELIMITER = ','
+NAMESPACE_DELIMITER = '-'
+
 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 TestEcnConfig(object):
+class TestEcnConfigBase(object):
     @classmethod
     def setup_class(cls):
+        print("SETUP")
         os.environ["PATH"] += os.pathsep + scripts_path
         os.environ['UTILITIES_UNIT_TESTING'] = "2"
-        print("SETUP")
 
+    def process_cmp_args(self, cmp_args):
+        """
+        The arguments are a string marked by delimiters
+        Arguments marked as 'None', are treated as None objects
+        First arg is always a collection of namespaces
+        """
+
+        args = cmp_args.split(ARGS_DELIMITER)
+        args = [None if arg == "None" else arg for arg in args]
+        args[0] = args[0].split(NAMESPACE_DELIMITER)
+        return args
+
+    def verify_profile(self, queue_db_entry, profile, value):
+        if profile is not None:
+            assert queue_db_entry[profile] == value
+        else:
+            assert profile not in queue_db_entry,\
+                   "Profile needs to be fully removed from table to propagate NULL OID to SAI"
+
+    def executor(self, input):
+        runner = CliRunner()
+
+        if 'db_table' in input:
+            db = Db()
+            data_list = list(db.cfgdb.get_table(input['db_table']))
+            input['rc_msg'] = input['rc_msg'].format(",".join(data_list))
+
+        if 'show' in input['cmd']:
+            exec_cmd = show.cli.commands["ecn"]
+            result = runner.invoke(exec_cmd, input['args'])
+            exit_code = result.exit_code
+            output = result.output
+        elif 'q_cmd' in input['cmd'] or 'show_masic' in input['cmd'] or 'config_masic' in input['cmd']:
+            exit_code, output = get_result_and_return_code(["ecnconfig"] + input['args'])
+        else:
+            exec_cmd = config.config.commands["ecn"]
+            result = runner.invoke(exec_cmd, input['args'])
+            exit_code = result.exit_code
+            output = result.output
+
+        print(exit_code)
+        print(output)
+
+        if input['rc'] == 0:
+            assert exit_code == 0
+        else:
+            assert exit_code != 0
+
+        if 'cmp_args' in input:
+            fd = open('/tmp/ecnconfig', 'r')
+            cmp_data = json.load(fd)
+
+            # Verify queue assignments
+            if 'cmp_q_args' in input:
+                namespaces, profile, value = self.process_cmp_args(input['cmp_args'][0])
+                for namespace in namespaces:
+                    for key in cmp_data[namespace]:
+                        queue_idx = ast.literal_eval(key)[-1]
+                        if queue_idx in input['cmp_q_args']:
+                            self.verify_profile(cmp_data[namespace][key], profile, value)
+
+                # other_q helps verify two different queue assignments
+                if 'other_q' in input:
+                    namespaces1, profile1, value1 = self.process_cmp_args(input['cmp_args'][-1])
+                    for namespace1 in namespaces1:
+                        for key in cmp_data[namespace1]:
+                            queue_idx = ast.literal_eval(key)[-1]
+                            if 'other_q' in input and queue_idx in input['other_q']:
+                                self.verify_profile(cmp_data[namespace1][key], profile1, value1)
+            # Verify non-queue related assignments
+            else:
+                for args in input['cmp_args']:
+                    namespaces, profile, name, value = self.process_cmp_args(args)
+                    for namespace in namespaces:
+                        assert(cmp_data[namespace][profile][name] == value)
+            fd.close()
+
+        if 'rc_msg' in input:
+            assert input['rc_msg'] in output
+
+        if 'rc_output' in input:
+            assert 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"
+
+        if os.path.isfile('/tmp/ecnconfig'):
+            os.remove('/tmp/ecnconfig')
+        print("TEARDOWN")
+
+
+class TestEcnConfig(TestEcnConfigBase):
     def test_ecn_show_config(self):
         self.executor(testData['ecn_show_config'])
 
@@ -123,77 +221,3 @@ def test_ecn_queue_set_all_on_verbose(self):
 
     def test_ecn_queue_set_lossy_q_on(self):
         self.executor(testData['ecn_cfg_lossy_q_on'])
-
-    def process_cmp_args(self, cmp_args):
-        if cmp_args is None:
-            return (None, None)
-        return cmp_args.split(',')
-
-    def verify_profile(self, queue_db_entry, profile, value):
-        if profile != None:
-            assert queue_db_entry[profile] == value
-        else:
-            assert profile not in queue_db_entry,\
-                   "Profile needs to be fully removed from table to propagate NULL OID to SAI"
-
-    def executor(self, input):
-        runner = CliRunner()
-
-        if 'db_table' in input:
-            db = Db()
-            data_list = list(db.cfgdb.get_table(input['db_table']))
-            input['rc_msg'] = input['rc_msg'].format(",".join(data_list))
-
-        if 'show' in input['cmd']:
-            exec_cmd = show.cli.commands["ecn"]
-            result = runner.invoke(exec_cmd, input['args'])
-            exit_code = result.exit_code
-            output = result.output
-        elif 'q_cmd' in input['cmd'] :
-            exit_code, output = get_result_and_return_code(["ecnconfig"] + input['args'])
-        else:
-            exec_cmd = config.config.commands["ecn"]
-            result = runner.invoke(exec_cmd, input['args'])
-            exit_code = result.exit_code
-            output = result.output
-
-        print(exit_code)
-        print(output)
-
-        if input['rc'] == 0:
-            assert exit_code == 0
-        else:
-            assert exit_code != 0
-
-        if 'cmp_args' in input:
-            fd = open('/tmp/ecnconfig', 'r')
-            cmp_data = json.load(fd)
-            if 'cmp_q_args' in input:
-                profile, value = self.process_cmp_args(input['cmp_args'][0])
-                if 'other_q' in input:
-                    profile1, value1 = self.process_cmp_args(input['cmp_args'][-1])
-                for key in cmp_data:
-                    queue_idx = ast.literal_eval(key)[-1]
-                    if queue_idx in input['cmp_q_args']:
-                        self.verify_profile(cmp_data[key], profile, value)
-                    if 'other_q' in input and queue_idx in input['other_q']:
-                        self.verify_profile(cmp_data[key], profile1, value1)
-            else:
-                for args in input['cmp_args']:
-                    profile, name, value = args.split(',')
-                    assert(cmp_data[profile][name] == value)
-            fd.close()
-
-        if 'rc_msg' in input:
-            assert input['rc_msg'] in output
-
-        if 'rc_output' in input:
-            assert 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"
-        if os.path.isfile('/tmp/ecnconfig'):
-            os.remove('/tmp/ecnconfig')
-        print("TEARDOWN")

tests/mock_tables/asic0/config_db.json

@@ -303,5 +303,28 @@
     "SYSLOG_CONFIG_FEATURE|database": {
         "rate_limit_interval": "222",
         "rate_limit_burst": "22222"
+    },
+    "WRED_PROFILE|AZURE_LOSSLESS": {
+        "red_max_threshold": "2097152",
+        "ecn": "ecn_all",
+        "green_min_threshold": "1048576",
+        "red_min_threshold": "1048576",
+        "yellow_min_threshold": "1048576",
+        "green_max_threshold": "2097152",
+        "green_drop_probability": "5",
+        "yellow_max_threshold": "2097152",
+        "yellow_drop_probability": "5",
+        "red_drop_probability": "5"
+    },
+    "DEVICE_NEIGHBOR|Ethernet4": {
+        "name": "Serverss0",
+        "port": "eth0"
+    },
+    "QUEUE|Ethernet4|0": {
+        "scheduler": "[SCHEDULAR|scheduler.0]"
+    },
+    "QUEUE|Ethernet4|1": {
+        "scheduler": "[SCHEDULAR|scheduler.0]",
+        "wred_profile": "AZURE_LOSSLESS"
     }
 }

tests/mock_tables/asic1/config_db.json

@@ -242,5 +242,25 @@
     "SYSLOG_CONFIG_FEATURE|database": {
         "rate_limit_interval": "555",
         "rate_limit_burst": "55555"
+    },
+    "WRED_PROFILE|AZURE_LOSSY": {
+            "red_max_threshold":"32760",
+            "red_min_threshold":"4095",
+            "yellow_max_threshold":"32760",
+            "yellow_min_threshold":"4095",
+            "green_max_threshold": "32760",
+            "green_min_threshold": "4095",
+            "yellow_drop_probability": "2"
+    },
+    "DEVICE_NEIGHBOR|Ethernet0": {
+        "name": "Servers",
+        "port": "eth0"
+    },
+    "QUEUE|Ethernet0|0": {
+        "scheduler": "[SCHEDULAR|scheduler.0]",
+        "wred_profile": "AZURE_LOSSLESS"
+    },
+    "QUEUE|Ethernet0|1": {
+        "scheduler": "[SCHEDULAR|scheduler.0]"
     }
 }

tests/multi_asic_ecnconfig_test.py

@@ -0,0 +1,64 @@
+import os
+import sys
+from .ecn_test import TestEcnConfigBase
+from .ecn_input.ecn_test_vectors import testData
+
+root_path = os.path.dirname(os.path.abspath(__file__))
+modules_path = os.path.dirname(root_path)
+scripts_path = os.path.join(modules_path, "scripts")
+sys.path.insert(0, root_path)
+sys.path.insert(0, modules_path)
+
+
+class TestEcnConfigMultiAsic(TestEcnConfigBase):
+    @classmethod
+    def setup_class(cls):
+        super().setup_class()
+        os.environ["UTILITIES_UNIT_TESTING_TOPOLOGY"] = "multi_asic"
+
+    def test_ecn_show_config_all_masic(self):
+        self.executor(testData['ecn_show_config_masic'])
+
+    def test_ecn_show_config_all_verbose_masic(self):
+        self.executor(testData['test_ecn_show_config_verbose_masic'])
+
+    def test_ecn_show_config_one_masic(self):
+        self.executor(testData['test_ecn_show_config_namespace'])
+
+    def test_ecn_show_config_one_verbose_masic(self):
+        self.executor(testData['test_ecn_show_config_namespace_verbose'])
+
+    def test_ecn_config_change_other_threshold_masic(self):
+        self.executor(testData['ecn_cfg_threshold_masic'])
+
+    def test_ecn_config_change_other_prob_masic(self):
+        self.executor(testData['ecn_cfg_probability_masic'])
+
+    def test_ecn_config_change_gdrop_verbose_all_masic(self):
+        self.executor(testData['ecn_cfg_gdrop_verbose_all_masic'])
+
+    def test_ecn_config_multi_set_verbose_all_masic(self):
+        self.executor(testData['ecn_cfg_multi_set_verbose_all_masic'])
+
+    def test_ecn_queue_get_masic(self):
+        self.executor(testData['ecn_q_get_masic'])
+
+    def test_ecn_queue_get_verbose_masic(self):
+        self.executor(testData['ecn_q_get_verbose_masic'])
+
+    def test_ecn_queue_get_all_masic(self):
+        self.executor(testData['ecn_q_get_all_ns_masic'])
+
+    def test_ecn_queue_get_all_verbose_masic(self):
+        self.executor(testData['ecn_q_get_all_ns_verbose_masic'])
+
+    def test_ecn_q_set_off_all_masic(self):
+        self.executor(testData['ecn_cfg_q_all_ns_off_masic'])
+
+    def test_ecn_q_set_off_one_masic(self):
+        self.executor(testData['ecn_cfg_q_one_ns_off_verbose_masic'])
+
+    @classmethod
+    def teardown_class(cls):
+        super().teardown_class()
+        os.environ["UTILITIES_UNIT_TESTING_TOPOLOGY"] = ""