Improve robustness of test_bgp_allow_list

tests/bgp/bgp_helpers.py

@@ -122,19 +122,19 @@ def parse_rib(host, ip_ver):
 
     return routes
 
-def verify_all_routes_announce_to_bgpmon(duthost, ptfhost):
+def get_routes_not_announced_to_bgpmon(duthost, ptfhost):
     """
-    Verify that the routes are announced by checking neighbors route dump.
+    Get the routes that are not announced to bgpmon by checking dump of bgpmon on PTF.
     """
-    time.sleep(BGP_ANNOUNCE_TIME)
+    def _dump_fie_exists(host):
+        return host.stat(path=DUMP_FILE).get('stat', {}).get('exists', False)
+    pytest_assert(wait_until(120, 10, _dump_fie_exists, ptfhost))
+    time.sleep(20)  # Wait until all routes announced to bgpmon
     bgpmon_routes = parse_exabgp_dump(ptfhost)
     rib_v4 = parse_rib(duthost, 4)
     rib_v6 = parse_rib(duthost, 6)
     routes_dut = dict(rib_v4.items() + rib_v6.items())
-    for route in routes_dut.keys():
-        if route not in bgpmon_routes:
-            return False
-    return True
+    return [route for route in routes_dut.keys() if route not in bgpmon_routes]
 
 def remove_bgp_neighbors(duthost, asic_index):
     """

tests/bgp/conftest.py

@@ -155,6 +155,61 @@ def setup_interfaces(duthost, ptfhost, request, tbinfo):
     def _is_ipv4_address(ip_addr):
         return ipaddress.ip_address(ip_addr).version == 4
 
+    def _duthost_cleanup_ip(duthost, namespace, ip):
+        """
+        Search if "ip" is configured on any DUT interface. If yes, remove it.
+        """
+
+        for line in duthost.shell("ip addr show | grep 'inet '")['stdout_lines']:
+            # Example line: '''    inet 10.0.0.2/31 scope global Ethernet104'''
+            fields = line.split()
+            intf_ip = fields[1].split("/")[0]
+            if intf_ip == ip:
+                intf_name = fields[-1]
+                duthost.shell("config interface %s ip remove %s %s" % (namespace, intf_name, ip))
+
+        ip_intfs = duthost.show_and_parse('show ip {} interface'.format(namespace))
+
+        # For interface that has two IP configured, the output looks like:
+        #       admin@vlab-03:~$ show ip int
+        #       Interface        Master    IPv4 address/mask    Admin/Oper    BGP Neighbor    Neighbor IP
+        #       ---------------  --------  -------------------  ------------  --------------  -------------
+        #       Ethernet100                10.0.0.50/31         up/up         ARISTA10T0      10.0.0.51
+        #       Ethernet104                10.0.0.2/31          up/up         N/A             N/A
+        #                                  10.0.0.52/31                       ARISTA11T0      10.0.0.53
+        #       Ethernet108                10.0.0.54/31         up/up         ARISTA12T0      10.0.0.55
+        #       Ethernet112                10.0.0.56/31         up/up         ARISTA13T0      10.0.0.57
+        #
+        # For interface Ethernet104, it has two entries in the output list:
+        #   [{
+        #     "ipv4 address/mask": "10.0.0.2/31",
+        #     "neighbor ip": "N/A",
+        #     "master": "",
+        #     "admin/oper": "up/up",
+        #     "interface": "Ethernet104",
+        #     "bgp neighbor": "N/A"
+        #   },
+        #   {
+        #     "ipv4 address/mask": "10.0.0.52/31",
+        #     "neighbor ip": "10.0.0.53",
+        #     "master": "",
+        #     "admin/oper": "",
+        #     "interface": "",
+        #     "bgp neighbor": "ARISTA11T0"
+        #   },]
+        # The second item has empty value for key "interface". Below code is to fill "Ethernet104" for the second item.
+        last_interface = ""
+        for ip_intf in ip_intfs:
+            if ip_intf["interface"] == "":
+                ip_intf["interface"] = last_interface
+            else:
+                last_interface = ip_intf["interface"]
+
+        # Remove the specified IP from interfaces
+        for ip_intf in ip_intfs:
+            if ip_intf["ipv4 address/mask"].split("/")[0] == ip:
+                duthost.shell("config interface %s ip remove %s %s" % (namespace, ip_intf["interface"], ip))
+
     @contextlib.contextmanager
     def _setup_interfaces_t0(mg_facts, peer_count):
         try:
@@ -192,6 +247,8 @@ def _setup_interfaces_t0(mg_facts, peer_count):
                 conn["loopback_ip"] = loopback_ip
                 connections.append(conn)
 
+            ptfhost.remove_ip_addresses()  # In case other case did not cleanup IP address configured on PTF interface
+
             for conn in connections:
                 ptfhost.shell("ifconfig %s %s" % (conn["neighbor_intf"],
                                                   conn["neighbor_addr"]))
@@ -253,9 +310,16 @@ def _setup_interfaces_t1(mg_facts, peer_count):
                     conn["neighbor_intf"] = "eth%s" % mg_facts["minigraph_port_indices"][intf]
                 connections.append(conn)
 
+            ptfhost.remove_ip_addresses()  # In case other case did not cleanup IP address configured on PTF interface
+
             for conn in connections:
-                # bind the ip to the interface and notify bgpcfgd
                 namespace = '-n {}'.format(conn["namespace"]) if conn["namespace"] else ''
+
+                # Find out if any other interface has the same IP configured. If yes, remove it
+                # Otherwise, there may be conflicts and test would fail.
+                _duthost_cleanup_ip(duthost, namespace, conn["local_addr"])
+
+                # bind the ip to the interface and notify bgpcfgd
                 duthost.shell("config interface %s ip add %s %s" % (namespace, conn["local_intf"], conn["local_addr"]))
                 ptfhost.shell("ifconfig %s %s" % (conn["neighbor_intf"], conn["neighbor_addr"]))
 
@@ -361,6 +425,12 @@ def bgpmon_setup_teardown(ptfhost, duthost, localhost, setup_interfaces):
     duthost.command("sonic-cfggen -j {} -w".format(BGPMON_CONFIG_FILE))
 
     logger.info("Starting bgp monitor session on PTF")
+
+    # Clean up in case previous run failed to clean up.
+    ptfhost.exabgp(name=BGP_MONITOR_NAME, state="absent")
+    ptfhost.file(path=CUSTOM_DUMP_SCRIPT_DEST, state="absent")
+
+    # Start bgp monitor session on PTF
     ptfhost.file(path=DUMP_FILE, state="absent")
     ptfhost.copy(src=CUSTOM_DUMP_SCRIPT, dest=CUSTOM_DUMP_SCRIPT_DEST)
     ptfhost.exabgp(name=BGP_MONITOR_NAME,
@@ -372,18 +442,25 @@ def bgpmon_setup_teardown(ptfhost, duthost, localhost, setup_interfaces):
                    peer_asn=asn,
                    port=BGP_MONITOR_PORT,
                    dump_script=CUSTOM_DUMP_SCRIPT_DEST)
+
+    # Flush neighbor and route in advance to avoid possible "RTNETLINK answers: File exists"
+    ptfhost.shell("ip neigh flush to %s nud permanent" % dut_lo_addr)
+    ptfhost.shell("ip route flush match %s" % dut_lo_addr + "/32")
+
     # Add the route to DUT loopback IP  and the interface router mac
     ptfhost.shell("ip neigh add %s lladdr %s dev %s" % (dut_lo_addr, duthost.facts["router_mac"], connection["neighbor_intf"]))
     ptfhost.shell("ip route add %s dev %s" % (dut_lo_addr + "/32", connection["neighbor_intf"]))
 
     pt_assert(wait_tcp_connection(localhost, ptfhost.mgmt_ip, BGP_MONITOR_PORT),
                   "Failed to start bgp monitor session on PTF")
+    pt_assert(wait_until(20, 5, duthost.check_bgp_session_state, [peer_addr]), 'BGP session {} on duthost is not established'.format(BGP_MONITOR_NAME))
+
     yield
     # Cleanup bgp monitor
     duthost.shell("redis-cli -n 4 -c DEL 'BGP_MONITORS|{}'".format(peer_addr))
     ptfhost.exabgp(name=BGP_MONITOR_NAME, state="absent")
     ptfhost.file(path=CUSTOM_DUMP_SCRIPT_DEST, state="absent")
     ptfhost.file(path=DUMP_FILE, state="absent")
     # Remove the route to DUT loopback IP  and the interface router mac
-    ptfhost.shell("ip route del %s dev %s" % (dut_lo_addr + "/32", connection["neighbor_intf"]))
-    ptfhost.shell("ip neigh del %s lladdr %s dev %s" % (dut_lo_addr, duthost.facts["router_mac"], connection["neighbor_intf"]))
+    ptfhost.shell("ip route flush match %s" % dut_lo_addr + "/32")
+    ptfhost.shell("ip neigh flush to %s nud permanent" % dut_lo_addr)

tests/bgp/test_bgp_allow_list.py

@@ -15,7 +15,7 @@
 from tests.common.helpers.constants import DEFAULT_NAMESPACE
 from tests.common.helpers.parallel import reset_ansible_local_tmp
 from tests.common.helpers.parallel import parallel_run
-from bgp_helpers import verify_all_routes_announce_to_bgpmon 
+from bgp_helpers import get_routes_not_announced_to_bgpmon
 
 pytestmark = [
     pytest.mark.topology('t1'),
@@ -98,7 +98,7 @@ def setup(tbinfo, nbrhosts, duthosts, rand_one_dut_hostname):
     tor1_namespace = DEFAULT_NAMESPACE
     for dut_port, neigh in mg_facts['minigraph_neighbors'].items():
         if tor1 == neigh['name'] and neigh['namespace']:
-            tor1_namespace = neigh['namespace'] 
+            tor1_namespace = neigh['namespace']
             break
 
     setup_info = {
@@ -134,9 +134,9 @@ def update_routes(action, ptfip, port, route):
 @pytest.fixture
 def load_remove_allow_list(duthosts, setup, rand_one_dut_hostname, request):
     duthost = duthosts[rand_one_dut_hostname]
-    
+
     allowed_list_prefixes = ALLOW_LIST['BGP_ALLOWED_PREFIXES']
-    
+
     for k,v in allowed_list_prefixes.items():
         v['default_action'] = request.param
 
@@ -212,7 +212,7 @@ def prepare_eos_routes(setup, ptfhost, build_routes, nbrhosts, tbinfo):
 
 
 class TestBGPAllowListBase(object):
-    
+
     def check_routes_on_tor1(self, setup, nbrhosts):
         tor1 = setup['tor1']
         for prefixes in PREFIX_LISTS.values():
@@ -373,18 +373,20 @@ def test_default_allow_list_preconfig(self, duthosts, rand_one_dut_hostname, set
         self.check_routes_on_tor1(setup, nbrhosts)
         self.check_routes_on_dut(duthost, setup['tor1_namespace'])
         self.check_routes_on_neighbors_empty_allow_list(nbrhosts, setup, permit)
-        pytest_assert(verify_all_routes_announce_to_bgpmon(duthost, ptfhost),
-                      "Not all routes are announced to bgpmon")
-
+        routes_not_announced = get_routes_not_announced_to_bgpmon(duthost, ptfhost)
+        pytest_assert(routes_not_announced==[],
+                      "Not all routes are announced to bgpmon: %s" % str(routes_not_announced))
+
     @pytest.mark.parametrize('load_remove_allow_list', ["permit", "deny"], indirect=['load_remove_allow_list'])
     def test_allow_list(self, duthosts, rand_one_dut_hostname, setup, nbrhosts, load_remove_allow_list, ptfhost, bgpmon_setup_teardown):
         permit = True if load_remove_allow_list == "permit" else False
         duthost = duthosts[rand_one_dut_hostname]
         self.check_routes_on_tor1(setup, nbrhosts)
         self.check_routes_on_dut(duthost, setup['tor1_namespace'])
         self.check_routes_on_neighbors(nbrhosts, setup, permit)
-        pytest_assert(verify_all_routes_announce_to_bgpmon(duthost, ptfhost),
-                      "Not all routes are announced to bgpmon")
-
+        routes_not_announced = get_routes_not_announced_to_bgpmon(duthost, ptfhost)
+        pytest_assert(routes_not_announced==[],
+                      "Not all routes are announced to bgpmon: %s" % str(routes_not_announced))
+
     def test_default_allow_list_postconfig(self, duthosts, rand_one_dut_hostname, setup, nbrhosts, ptfhost, bgpmon_setup_teardown):
         self.test_default_allow_list_preconfig(duthosts, rand_one_dut_hostname, setup, nbrhosts, ptfhost, bgpmon_setup_teardown)

tests/bgp/test_traffic_shift.py

@@ -1,7 +1,7 @@
 import pytest
 import logging
 import ipaddr as ipaddress
-from bgp_helpers import parse_rib, verify_all_routes_announce_to_bgpmon,remove_bgp_neighbors,restore_bgp_neighbors
+from bgp_helpers import parse_rib, get_routes_not_announced_to_bgpmon,remove_bgp_neighbors,restore_bgp_neighbors
 from tests.common.helpers.constants import DEFAULT_ASIC_ID
 from tests.common.helpers.assertions import pytest_assert
 import re
@@ -164,7 +164,7 @@ def test_TSA(duthost, ptfhost, nbrhosts, bgpmon_setup_teardown, traffic_shift_co
         # Verify DUT is in maintenance state.
         pytest_assert(TS_MAINTENANCE == get_traffic_shift_state(duthost),
                       "DUT is not in maintenance state")
-        pytest_assert(verify_all_routes_announce_to_bgpmon(duthost, ptfhost),
+        pytest_assert(get_routes_not_announced_to_bgpmon(duthost, ptfhost)==[],
                       "Not all routes are announced to bgpmon")
         pytest_assert(verify_only_loopback_routes_are_announced_to_neighs(duthost, nbrhosts, traffic_shift_community),
                       "Failed to verify routes on eos in TSA")
@@ -183,7 +183,7 @@ def test_TSB(duthost, ptfhost, nbrhosts, bgpmon_setup_teardown):
     # Verify DUT is in normal state.
     pytest_assert(TS_NORMAL == get_traffic_shift_state(duthost),
                   "DUT is not in normal state")
-    pytest_assert(verify_all_routes_announce_to_bgpmon(duthost, ptfhost),
+    pytest_assert(get_routes_not_announced_to_bgpmon(duthost, ptfhost)==[],
                   "Not all routes are announced to bgpmon")
     pytest_assert(verify_all_routes_announce_to_neighs(duthost, nbrhosts, parse_rib(duthost, 4), 4),
                   "Not all ipv4 routes are announced to neighbors")

tests/common/devices/ptf.py

@@ -1,5 +1,9 @@
 from tests.common.devices.base import AnsibleHostBase
 
+CHANGE_MAC_ADDRESS_SCRIPT = "scripts/change_mac.sh"
+REMOVE_IP_ADDRESS_SCRIPT = "scripts/remove_ip.sh"
+
+
 
 class PTFHost(AnsibleHostBase):
     """
@@ -10,4 +14,10 @@ class PTFHost(AnsibleHostBase):
     def __init__(self, ansible_adhoc, hostname):
         AnsibleHostBase.__init__(self, ansible_adhoc, hostname)
 
+    def change_mac_addresses(self):
+        self.script(CHANGE_MAC_ADDRESS_SCRIPT)
+
+    def remove_ip_addresses(self):
+        self.script(REMOVE_IP_ADDRESS_SCRIPT)
+
     # TODO: Add a method for running PTF script

tests/common/fixtures/ptfhost_utils.py

@@ -21,8 +21,6 @@
 ARP_RESPONDER_PY = "arp_responder.py"
 ICMP_RESPONDER_PY = "icmp_responder.py"
 ICMP_RESPONDER_CONF_TEMPL = "icmp_responder.conf.j2"
-CHANGE_MAC_ADDRESS_SCRIPT = "scripts/change_mac.sh"
-REMOVE_IP_ADDRESS_SCRIPT = "scripts/remove_ip.sh"
 GARP_SERVICE_PY = 'garp_service.py'
 GARP_SERVICE_CONF_TEMPL = 'garp_service.conf.j2'
 
@@ -99,7 +97,7 @@ def change_mac_addresses(ptfhost):
             None
     """
     logger.info("Change interface MAC addresses on ptfhost '{0}'".format(ptfhost.hostname))
-    ptfhost.script(CHANGE_MAC_ADDRESS_SCRIPT)
+    ptfhost.change_mac_addresses()
 
 
 @pytest.fixture(scope="session", autouse=True)
@@ -113,12 +111,12 @@ def remove_ip_addresses(ptfhost):
             None
     """
     logger.info("Remove existing IPs on ptfhost '{0}'".format(ptfhost.hostname))
-    ptfhost.script(REMOVE_IP_ADDRESS_SCRIPT)
+    ptfhost.remove_ip_addresses()
 
     yield
 
     logger.info("Remove IPs to restore ptfhost '{0}'".format(ptfhost.hostname))
-    ptfhost.script(REMOVE_IP_ADDRESS_SCRIPT)
+    ptfhost.remove_ip_addresses()
 
 
 @pytest.fixture(scope="session", autouse=True)