tests: fix check for daemon startup, remove sleep hack

- Remove the .pid and .vty files and then wait for them to show back up.
- Fix broken BGP GR test to not fail now that it's bug is exposed. It
only worked b/c when starting a daemon the pid file still existed and
blocked the bogus second BGP launch from happening.

Signed-off-by: Christian Hopps <chopps@labn.net>

tests/topotests/bgp_gr_functionality_topo1/test_bgp_gr_functionality_topo1-3.py

@@ -1159,7 +1159,7 @@ def test_BGP_GR_TC_31_2_p1(request):
     reset_config_on_routers(tgen)
 
     logger.info(
-        "[Phase 1] : Test Setup " "[Disable Mode]R1-----R2[Restart Mode] initialized  "
+        "[Phase 1] : Test Setup " "[Disable Mode]R1-----R2[Helper Mode] initialized  "
     )
 
     # Configure graceful-restart
@@ -1251,7 +1251,7 @@ def test_BGP_GR_TC_31_2_p1(request):
             tc_name, result
         )
 
-    logger.info("[Phase 2] : R2 Goes from Disable to Restart Mode  ")
+    logger.info("[Phase 2] : R1 Goes from Disable to Restart Mode  ")
 
     # Configure graceful-restart
     input_dict = {
@@ -1356,31 +1356,7 @@ def test_BGP_GR_TC_31_2_p1(request):
         },
     }
 
-    # here the verify_graceful_restart fro the neighbor would be
+    logger.info("[Phase 4] : R1 is UP and GR state is correct ")
-    # "NotReceived" as the latest GR config is not yet applied.
-    for addr_type in ADDR_TYPES:
-        result = verify_graceful_restart(
-            tgen, topo, addr_type, input_dict, dut="r1", peer="r2"
-        )
-        assert result is True, "Testcase {} : Failed \n Error {}".format(
-            tc_name, result
-        )
-
-    for addr_type in ADDR_TYPES:
-        # Verifying RIB routes
-        next_hop = next_hop_per_address_family(
-            tgen, dut, peer, addr_type, NEXT_HOP_IP_2
-        )
-        input_topo = {key: topo["routers"][key] for key in ["r2"]}
-        result = verify_rib(tgen, addr_type, dut, input_topo, next_hop, protocol)
-        assert result is True, "Testcase {} : Failed \n Error {}".format(
-            tc_name, result
-        )
-
-    logger.info("[Phase 6] : R1 is about to come up now  ")
-    start_router_daemons(tgen, "r1", ["bgpd"])
-
-    logger.info("[Phase 4] : R1 is UP now, so time to collect GR stats ")
 
     for addr_type in ADDR_TYPES:
         result = verify_graceful_restart(

tests/topotests/lib/topotest.py

@@ -30,6 +30,7 @@ from copy import deepcopy
 import lib.topolog as topolog
 from lib.micronet_compat import Node
 from lib.topolog import logger
+from munet.base import Timeout
 
 from lib import micronet
 
@@ -1769,18 +1770,30 @@ class Router(Node):
                     daemons_list.append(daemon)
 
         tail_log_files = []
+        check_daemon_files = []
 
         def start_daemon(daemon, extra_opts=None):
             daemon_opts = self.daemons_options.get(daemon, "")
+
+            # get pid and vty filenames and remove the files
+            m = re.match(r"(.* |^)-n (\d+)( ?.*|$)", daemon_opts)
+            dfname = daemon if not m else "{}-{}".format(daemon, m.group(2))
+            runbase = "/var/run/{}/{}".format(self.routertype, dfname)
+            # If this is a new system bring-up remove the pid/vty files, otherwise
+            # do not since apparently presence of the pidfile impacts BGP GR
+            self.cmd_status("rm -f {0}.pid {0}.vty".format(runbase))
+
             rediropt = " > {0}.out 2> {0}.err".format(daemon)
             if daemon == "snmpd":
                 binary = "/usr/sbin/snmpd"
                 cmdenv = ""
                 cmdopt = "{} -C -c /etc/frr/snmpd.conf -p ".format(
                     daemon_opts
-                ) + "/var/run/{}/snmpd.pid -x /etc/frr/agentx".format(self.routertype)
+                ) + "{}.pid -x /etc/frr/agentx".format(runbase)
+                # check_daemon_files.append(runbase + ".pid")
             else:
                 binary = os.path.join(self.daemondir, daemon)
+                check_daemon_files.extend([runbase + ".pid", runbase + ".vty"])
 
                 cmdenv = "ASAN_OPTIONS="
                 if asan_abort:
@@ -1844,8 +1857,6 @@ class Router(Node):
                 logger.info(
                     "%s: %s %s launched in gdb window", self, self.routertype, daemon
                 )
-                # Need better check for daemons running.
-                time.sleep(5)
             else:
                 if daemon != "snmpd":
                     cmdopt += " -d "
@@ -1904,9 +1915,22 @@ class Router(Node):
             start_daemon(daemon)
 
         # Check if daemons are running.
-        rundaemons = self.cmd("ls -1 /var/run/%s/*.pid" % self.routertype)
+        wait_time = 30 if (gdb_routers or gdb_daemons) else 10
-        if re.search(r"No such file or directory", rundaemons):
+        timeout = Timeout(wait_time)
-            return "Daemons are not running"
+        for remaining in timeout:
+            if not check_daemon_files:
+                break
+            check = check_daemon_files[0]
+            if self.path_exists(check):
+                check_daemon_files.pop(0)
+                continue
+            self.logger.debug("Waiting {}s for {} to appear".format(remaining, check))
+            time.sleep(0.5)
+
+        if check_daemon_files:
+            assert False, "Timeout({}) waiting for {} to appear on {}".format(
+                wait_time, check_daemon_files[0], self.name
+            )
 
         # Update the permissions on the log files
         self.cmd("chown frr:frr -R {}/{}".format(self.logdir, self.name))