Address review comments

tests/copp/conftest.py

@@ -1,10 +1,10 @@
 """
-Pytest configuration used by the COPP tests.
+    Pytest configuration used by the COPP tests.
 """
 
 def pytest_addoption(parser):
     """
-    Adds options to pytest that are used by the COPP tests.
+        Adds options to pytest that are used by the COPP tests.
     """
 
     parser.addoption(

tests/copp/copp_utils.py

@@ -1,8 +1,8 @@
 """
-Helpful utilities for writing tests for the COPP feature.
+    Helpful utilities for writing tests for the COPP feature.
 
-Todo:
-    Refactor ptfadapter so it can be leveraged in these test cases.
+    Todo:
+        Refactor ptfadapter so it can be leveraged in these test cases.
 """
 
 DEFAULT_NN_TARGET_PORT = 3
@@ -15,22 +15,22 @@
 _SWSS_COPP_CONFIG = "swss:/etc/swss/config.d/00-copp.config.json"
 _TEMP_COPP_CONFIG = "/tmp/copp_config.json"
 
-_PTF_NN_TEMPLATE = "copp/templates/ptf_nn_agent.conf.ptf.j2"
+_PTF_NN_TEMPLATE = "templates/ptf_nn_agent.conf.ptf.j2"
 _PTF_NN_DEST = "/etc/supervisor/conf.d/ptf_nn_agent.conf"
 
-_SYNCD_NN_TEMPLATE = "copp/templates/ptf_nn_agent.conf.dut.j2"
+_SYNCD_NN_TEMPLATE = "templates/ptf_nn_agent.conf.dut.j2"
 _SYNCD_NN_DEST = "/tmp/ptf_nn_agent.conf"
 
 def limit_policer(dut, pps_limit):
     """
-    Updates the COPP configuration in the SWSS container to respect a given rate limit.
+        Updates the COPP configuration in the SWSS container to respect a given rate limit.
 
-    Note:
-        The SWSS container must be restarted for the config change to take effect.
+        Note:
+            The SWSS container must be restarted for the config change to take effect.
 
-    Args:
-        dut (SonicHost): The target device.
-        pps_limit (int): The rate limit for COPP to enforce on ALL trap groups.
+        Args:
+            dut (SonicHost): The target device.
+            pps_limit (int): The rate limit for COPP to enforce on ALL trap groups.
     """
 
     dut.command("docker cp {} {}".format(_SWSS_COPP_CONFIG, _BASE_COPP_CONFIG))
@@ -40,22 +40,22 @@ def limit_policer(dut, pps_limit):
 
 def restore_policer(dut):
     """
-    Reloads the default COPP configuration in the SWSS container.
+        Reloads the default COPP configuration in the SWSS container.
 
-    Notes:
-        This method should only be used after limit_policer.
+        Notes:
+            This method should only be used after limit_policer.
 
-        The SWSS container must be restarted for the config change to take effect.
+            The SWSS container must be restarted for the config change to take effect.
     """
     dut.command("docker cp {} {}".format(_BASE_COPP_CONFIG, _SWSS_COPP_CONFIG))
 
 def configure_ptf(ptf, nn_target_port):
     """
-    Configures the PTF to run the NN agent on the specified port.
+        Configures the PTF to run the NN agent on the specified port.
 
-    Args:
-        ptf (PTFHost): The target PTF.
-        nn_target_port (int): The port to run NN agent on.
+        Args:
+            ptf (PTFHost): The target PTF.
+            nn_target_port (int): The port to run NN agent on.
     """
 
     ptf.script(cmd=_REMOVE_IP_SCRIPT)
@@ -71,10 +71,10 @@ def configure_ptf(ptf, nn_target_port):
 
 def restore_ptf(ptf):
     """
-    Restores the PTF and the NN agent to default settings.
+        Restores the PTF and the NN agent to default settings.
 
-    Args:
-        ptf (PTFHost): The target PTF.
+        Args:
+            ptf (PTFHost): The target PTF.
     """
 
     ptf.script(cmd=_REMOVE_IP_SCRIPT)
@@ -88,15 +88,15 @@ def restore_ptf(ptf):
 
 def configure_syncd(dut, nn_target_port):
     """
-    Configures syncd to run the NN agent on the specified port.
+        Configures syncd to run the NN agent on the specified port.
 
-    Note:
-        The DUT must be running an RPC syncd image in order for the
-        NN agent to be available.
+        Note:
+            The DUT must be running an RPC syncd image in order for the
+            NN agent to be available.
 
-    Args:
-        dut (SonicHost): The target device.
-        nn_target_port (int): The port to run NN agent on.
+        Args:
+            dut (SonicHost): The target device.
+            nn_target_port (int): The port to run NN agent on.
     """
 
     facts = {"nn_target_port": nn_target_port,
@@ -111,7 +111,7 @@ def configure_syncd(dut, nn_target_port):
 
 def _map_port_number_to_interface(dut, nn_target_port):
     """
-    Retrieves the correct interface for a given port number.
+        Retrieves the correct interface for a given port number.
     """
 
     interfaces = dut.command("portstat")["stdout_lines"][2:]

tests/copp/scripts/update_copp_config.py

@@ -32,6 +32,13 @@ def generate_limited_pps_config(pps_limit, input_config_file, output_config_file
 
     for trap_group in copp_config:
         for _, group_config in trap_group.items():
+            # Notes:
+            # CIR (committed information rate) - bandwidth limit set by the policer
+            # CBS (committed burst size) - largest burst of packets allowed by the policer
+            #
+            # Setting these two values to pps_limit restricts the policer to allowing exactly
+            # that number of packets per second, which is what we want for our tests.
+
             if "cir" in group_config:
                 group_config["cir"] = pps_limit
 

tests/copp/test_copp.py

@@ -1,29 +1,29 @@
 """
-Tests the COPP feature in SONiC.
+    Tests the COPP feature in SONiC.
 
-Notes:
-    These test cases require that a special RPC syncd image is installed on the
-    DUT. You can either pre-install this image and run the test normally, or
-    specify the `--swap-syncd` flag from the command line to have the test fetch
-    the RPC image and install it before the test runs.
+    Notes:
+        These test cases require that a special RPC syncd image is installed on the
+        DUT. You can either pre-install this image and run the test normally, or
+        specify the `--swap-syncd` flag from the command line to have the test fetch
+        the RPC image and install it before the test runs.
 
-    These test cases limit the PPS of all trap groups to 600. This is done to ensure
-    that the PTF can send traffic fast enough to trigger the policer. In order to validate
-    higher rate limits, a physical traffic generator is needed, which is beyond the scope
-    of these test cases.
+        These test cases limit the PPS of all trap groups to 600. This is done to ensure
+        that the PTF can send traffic fast enough to trigger the policer. In order to validate
+        higher rate limits, a physical traffic generator is needed, which is beyond the scope
+        of these test cases.
 
-Parameters:
-    --nn_target_port <port> (int): Which port you want the test to send traffic
-        to. Default is 3.
+    Parameters:
+        --nn_target_port <port> (int): Which port you want the test to send traffic
+            to. Default is 3.
 
-        Note that this is not the same as the interface name. For example, Ethernet12
-        may not be the 12th port in your system depending on the HWSKU under test.
+            Note that this is not the same as the interface name. For example, Ethernet12
+            may not be the 12th port in your system depending on the HWSKU under test.
 
-    --pkt_tx_count <n> (int): How many packets to send during each individual test case.
-        Default is 100000.
+        --pkt_tx_count <n> (int): How many packets to send during each individual test case.
+            Default is 100000.
 
-    --swap_syncd: Used to install the RPC syncd image before running the tests. Default
-        is disabled.
+        --swap_syncd: Used to install the RPC syncd image before running the tests. Default
+            is disabled.
 """
 
 import time
@@ -45,7 +45,7 @@
 
 class TestCOPP(object):
     """
-    Tests basic COPP functionality in SONiC.
+        Tests basic COPP functionality in SONiC.
     """
 
     @pytest.mark.parametrize("protocol", ["ARP",
@@ -54,10 +54,10 @@ class TestCOPP(object):
                                           "SSH"])
     def test_policer(self, protocol, duthost, ptfhost, _copp_testbed):
         """
-        Validates that rate-limited COPP groups work as expected.
+            Validates that rate-limited COPP groups work as expected.
 
-        Checks that the policer enforces the rate limit for protocols
-        that have a set rate limit.
+            Checks that the policer enforces the rate limit for protocols
+            that have a set rate limit.
         """
 
         if protocol == "ARP" and is_broadcom_device(duthost):
@@ -78,10 +78,10 @@ def test_policer(self, protocol, duthost, ptfhost, _copp_testbed):
                                           "UDLD"])
     def test_no_policer(self, protocol, duthost, ptfhost, _copp_testbed):
         """
-        Validates that non-rate-limited COPP groups work as expected.
+            Validates that non-rate-limited COPP groups work as expected.
 
-        Checks that the policer does not enforce a rate limit for protocols
-        that do not have any set rate limit.
+            Checks that the policer does not enforce a rate limit for protocols
+            that do not have any set rate limit.
         """
 
         if protocol == "BGP" and _copp_testbed.topo == "t1-lag":
@@ -95,7 +95,7 @@ def test_no_policer(self, protocol, duthost, ptfhost, _copp_testbed):
 @pytest.fixture(scope="class")
 def _copp_testbed(duthost, ptfhost, testbed, request):
     """
-    Pytest fixture to handle setup and cleanup for the COPP tests.
+        Pytest fixture to handle setup and cleanup for the COPP tests.
     """
 
     test_params = _gather_test_params(testbed, request)
@@ -109,7 +109,7 @@ def _copp_testbed(duthost, ptfhost, testbed, request):
 
 def _copp_runner(dut, ptf, protocol, test_params):
     """
-    Configures and runs the PTF test cases.
+        Configures and runs the PTF test cases.
     """
 
     params = {"verbose": False,
@@ -135,7 +135,7 @@ def _copp_runner(dut, ptf, protocol, test_params):
 
 def _gather_test_params(testbed, request):
     """
-    Fetches the test parameters from pytest.
+        Fetches the test parameters from pytest.
     """
 
     nn_target_port = request.config.getoption("--nn_target_port")
@@ -150,7 +150,7 @@ def _gather_test_params(testbed, request):
 
 def _setup_testbed(dut, ptf, test_params):
     """
-    Sets up the testbed to run the COPP tests.
+        Sets up the testbed to run the COPP tests.
     """
 
     # We don't want LLDP to throw off our test results, so we disable it first.
@@ -172,7 +172,7 @@ def _setup_testbed(dut, ptf, test_params):
 
 def _teardown_testbed(dut, ptf, test_params):
     """
-    Tears down the testbed, returning it to its initial state.
+        Tears down the testbed, returning it to its initial state.
     """
 
     copp_utils.restore_ptf(ptf)
@@ -189,7 +189,7 @@ def _teardown_testbed(dut, ptf, test_params):
 
 def _restart_swss(dut):
     """
-    Restarts SWSS and waits for the system to stabilize.
+        Restarts SWSS and waits for the system to stabilize.
     """
 
     # The failure counter may be incremented by other test cases, so we clear it