tests: apply KISS to retry fixture

This python fixture was way too complex for what is needed.

Eliminate gratuitous options/over-engineering:

- Change from non-deterministic `wait` and `attempts` to a single
`retry_timeout` value. This is both more deterministic, as well as
what the user should actually be thinking about.

- Use a fixed 2 second pause between executing the wrapped function
rather than a bunch of arbitrary choices of 2, 3 and 4 seconds
spread all over the test code.

- Get rid of the multiple variables for determining what "Positive" and
"Negative" results are. Instead just implement what all the user code
already wants, i.e., boolean False or a str (errormsg) means
"Negative" result otherwise it's a "Positive" result.

- As part of the above the inversion logic is much more comprehensible
in the fixture code (and more correct to boot).

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

12 files changed, 170 insertions, 138 deletions

diff --git a/tests/topotests/lib/bgp.py b/tests/topotests/lib/bgp.py
index db7b3586f1..a236a916b5 100644
--- a/tests/topotests/lib/bgp.py
+++ b/tests/topotests/lib/bgp.py
@@ -989,7 +989,7 @@ def modify_bgp_config_when_bgpd_down(tgen, topo, input_dict):
 #############################################
 # Verification APIs
 #############################################
-@retry(attempts=4, wait=2, return_is_str=True)
+@retry(retry_timeout=8)
 def verify_router_id(tgen, topo, input_dict, expected=True):
     """
     Running command "show ip bgp json" for DUT and reading router-id
@@ -1061,7 +1061,7 @@ def verify_router_id(tgen, topo, input_dict, expected=True):
     return True
 
 
-@retry(attempts=50, wait=3, return_is_str=True)
+@retry(retry_timeout=150)
 def verify_bgp_convergence(tgen, topo, dut=None, expected=True):
     """
     API will verify if BGP is converged with in the given time frame.
@@ -1266,7 +1266,7 @@ def verify_bgp_convergence(tgen, topo, dut=None, expected=True):
     return True
 
 
-@retry(attempts=4, wait=4, return_is_str=True)
+@retry(retry_timeout=16)
 def verify_bgp_community(
     tgen, addr_type, router, network, input_dict=None, vrf=None, bestpath=False, expected=True
 ):
@@ -1427,7 +1427,7 @@ def modify_as_number(tgen, topo, input_dict):
     return True
 
 
-@retry(attempts=4, wait=2, return_is_str=True)
+@retry(retry_timeout=8)
 def verify_as_numbers(tgen, topo, input_dict, expected=True):
     """
     This API is to verify AS numbers for given DUT by running
@@ -1527,7 +1527,7 @@ def verify_as_numbers(tgen, topo, input_dict, expected=True):
     return True
 
 
-@retry(attempts=50, wait=3, return_is_str=True)
+@retry(retry_timeout=150)
 def verify_bgp_convergence_from_running_config(tgen, dut=None, expected=True):
     """
     API to verify BGP convergence b/w loopback and physical interface.
@@ -2083,7 +2083,7 @@ def verify_bgp_timers_and_functionality(tgen, topo, input_dict):
     return True
 
 
-@retry(attempts=4, wait=4, return_is_str=True)
+@retry(retry_timeout=16)
 def verify_bgp_attributes(
     tgen,
     addr_type,
@@ -2223,7 +2223,7 @@ def verify_bgp_attributes(
     return True
 
 
-@retry(attempts=4, wait=2, return_is_str=True)
+@retry(retry_timeout=8)
 def verify_best_path_as_per_bgp_attribute(
     tgen, addr_type, router, input_dict, attribute, expected=True
 ):
@@ -2429,7 +2429,7 @@ def verify_best_path_as_per_bgp_attribute(
     return True
 
 
-@retry(attempts=5, wait=2, return_is_str=True)
+@retry(retry_timeout=10)
 def verify_best_path_as_per_admin_distance(
     tgen, addr_type, router, input_dict, attribute, expected=True
 ):
@@ -2543,7 +2543,7 @@ def verify_best_path_as_per_admin_distance(
     return True
 
 
-@retry(attempts=5, wait=2, return_is_str=True, initial_wait=2)
+@retry(retry_timeout=10, initial_wait=2)
 def verify_bgp_rib(
     tgen, addr_type, dut, input_dict, next_hop=None, aspath=None, multi_nh=None, expected=True
 ):
@@ -2846,7 +2846,7 @@ def verify_bgp_rib(
     return True
 
 
-@retry(attempts=5, wait=2, return_is_str=True)
+@retry(retry_timeout=10)
 def verify_graceful_restart(tgen, topo, addr_type, input_dict, dut, peer, expected=True):
     """
     This API is to verify verify_graceful_restart configuration of DUT and
@@ -3096,7 +3096,7 @@ def verify_graceful_restart(tgen, topo, addr_type, input_dict, dut, peer, expect
     return True
 
 
-@retry(attempts=5, wait=2, return_is_str=True)
+@retry(retry_timeout=10)
 def verify_r_bit(tgen, topo, addr_type, input_dict, dut, peer, expected=True):
     """
     This API is to verify r_bit in the BGP gr capability advertised
@@ -3216,7 +3216,7 @@ def verify_r_bit(tgen, topo, addr_type, input_dict, dut, peer, expected=True):
     return True
 
 
-@retry(attempts=5, wait=2, return_is_str=True)
+@retry(retry_timeout=10)
 def verify_eor(tgen, topo, addr_type, input_dict, dut, peer, expected=True):
     """
     This API is to verify EOR
@@ -3379,7 +3379,7 @@ def verify_eor(tgen, topo, addr_type, input_dict, dut, peer, expected=True):
     return True
 
 
-@retry(attempts=4, wait=2, return_is_str=True)
+@retry(retry_timeout=8)
 def verify_f_bit(tgen, topo, addr_type, input_dict, dut, peer, expected=True):
     """
     This API is to verify f_bit in the BGP gr capability advertised
@@ -3520,7 +3520,7 @@ def verify_f_bit(tgen, topo, addr_type, input_dict, dut, peer, expected=True):
     return True
 
 
-@retry(attempts=5, wait=2, return_is_str=True)
+@retry(retry_timeout=10)
 def verify_graceful_restart_timers(tgen, topo, addr_type, input_dict, dut, peer):
     """
     This API is to verify graceful restart timers, configured and recieved
@@ -3648,7 +3648,7 @@ def verify_graceful_restart_timers(tgen, topo, addr_type, input_dict, dut, peer)
     return True
 
 
-@retry(attempts=4, wait=2, return_is_str=True)
+@retry(retry_timeout=8)
 def verify_gr_address_family(tgen, topo, addr_type, addr_family, dut, expected=True):
     """
     This API is to verify gr_address_family in the BGP gr capability advertised
@@ -3739,7 +3739,7 @@ def verify_gr_address_family(tgen, topo, addr_type, addr_family, dut, expected=T
     logger.debug("Exiting lib API: {}".format(sys._getframe().f_code.co_name))
 
 
-@retry(attempts=6, wait=2, return_is_str=True)
+@retry(retry_timeout=12)
 def verify_attributes_for_evpn_routes(
     tgen,
     topo,
@@ -4139,7 +4139,7 @@ def verify_attributes_for_evpn_routes(
     return False
 
 
-@retry(attempts=5, wait=2, return_is_str=True)
+@retry(retry_timeout=10)
 def verify_evpn_routes(
     tgen, topo, dut, input_dict, routeType=5, EthTag=0, next_hop=None, expected=True
 ):
diff --git a/tests/topotests/lib/common_config.py b/tests/topotests/lib/common_config.py
index 3f78f020bc..9e38608631 100644
--- a/tests/topotests/lib/common_config.py
+++ b/tests/topotests/lib/common_config.py
@@ -19,7 +19,7 @@
 #
 
 from collections import OrderedDict
-from datetime import datetime
+from datetime import datetime, timedelta
 from time import sleep
 from copy import deepcopy
 from subprocess import call
@@ -37,12 +37,14 @@ import socket
 import ipaddress
 import platform
 
-if sys.version_info[0] > 2:
-    import io
-    import configparser
-else:
-    import StringIO
+try:
+    # Imports from python2
+    from StringIO import StringIO
     import ConfigParser as configparser
+except ImportError:
+    # Imports from python3
+    from io import StringIO
+    import configparser
 
 from lib.topolog import logger, logger_config
 from lib.topogen import TopoRouter, get_topogen
@@ -136,6 +138,12 @@ DEBUG_LOGS = {
     ],
 }
 
+def is_string(value):
+    try:
+        return isinstance(value, basestring)
+    except NameError:
+        return isinstance(value, str)
+
 if config.has_option("topogen", "verbosity"):
     loglevel = config.get("topogen", "verbosity")
     loglevel = loglevel.upper()
@@ -448,16 +456,6 @@ def check_router_status(tgen):
     return True
 
 
-def getStrIO():
-    """
-    Return a StringIO object appropriate for the current python version.
-    """
-    if sys.version_info[0] > 2:
-        return io.StringIO()
-    else:
-        return StringIO.StringIO()
-
-
 def reset_config_on_routers(tgen, routerName=None):
     """
     Resets configuration on routers to the snapshot created using input JSON
@@ -529,7 +527,7 @@ def reset_config_on_routers(tgen, routerName=None):
             raise InvalidCLIError("Unknown error in %s", output)
 
         f = open(dname, "r")
-        delta = getStrIO()
+        delta = StringIO()
         delta.write("configure terminal\n")
         t_delta = f.read()
 
@@ -563,7 +561,7 @@ def reset_config_on_routers(tgen, routerName=None):
         output = router.vtysh_multicmd(delta.getvalue(), pretty_output=False)
 
         delta.close()
-        delta = getStrIO()
+        delta = StringIO()
         cfg = router.run("vtysh -c 'show running'")
         for line in cfg.split("\n"):
             line = line.strip()
@@ -1619,60 +1617,99 @@ def interface_status(tgen, topo, input_dict):
     return True
 
 
-def retry(attempts=3, wait=2, return_is_str=True, initial_wait=0, return_is_dict=False):
+def retry(retry_timeout, initial_wait=0, expected=True, diag_pct=0.75):
     """
-    Retries function execution, if return is an errormsg or exception
-
-    * `attempts`: Number of attempts to make
-    * `wait`: Number of seconds to wait between each attempt
-    * `return_is_str`: Return val is an errormsg in case of failure
-    * `initial_wait`: Sleeps for this much seconds before executing function
-
+    Fixture: Retries function while it's return value is an errormsg (str), False, or it raises an exception.
+
+    * `retry_timeout`: Retry for at least this many seconds; after waiting initial_wait seconds
+    * `initial_wait`: Sleeps for this many seconds before first executing function
+    * `expected`: if False then the return logic is inverted, except for exceptions,
+                      (i.e., a False or errmsg (str) function return ends the retry loop,
+                      and returns that False or str value)
+    * `diag_pct`: Percentage of `retry_timeout` to keep testing after negative result would have
+                  been returned in order to see if a positive result comes after. This is an
+                  important diagnostic tool, and normally should not be disabled. Calls to wrapped
+                  functions though, can override the `diag_pct` value to make it larger in case more
+                  diagnostic retrying is appropriate.
     """
 
     def _retry(func):
         @wraps(func)
         def func_retry(*args, **kwargs):
-            _wait = kwargs.pop("wait", wait)
-            _attempts = kwargs.pop("attempts", attempts)
-            _attempts = int(_attempts)
-            if _attempts < 0:
-                raise ValueError("attempts must be 0 or greater")
+            # We will continue to retry diag_pct of the timeout value to see if test would have passed with a
+            # longer retry timeout value.
+            saved_failure = None
+
+            retry_sleep = 2
+
+            # Allow the wrapped function's args to override the fixtures
+            _retry_timeout = kwargs.pop("retry_timeout", retry_timeout)
+            _expected = kwargs.pop("expected", expected)
+            _initial_wait = kwargs.pop("initial_wait", initial_wait)
+            _diag_pct = kwargs.pop("diag_pct", diag_pct)
+
+            start_time = datetime.now()
+            retry_until = datetime.now() + timedelta(seconds=_retry_timeout + _initial_wait)
 
             if initial_wait > 0:
                 logger.info("Waiting for [%s]s as initial delay", initial_wait)
                 sleep(initial_wait)
 
-            _return_is_str = kwargs.pop("return_is_str", return_is_str)
-            _return_is_dict = kwargs.pop("return_is_str", return_is_dict)
-            _expected = kwargs.setdefault("expected", True)
-            kwargs.pop("expected")
-            for i in range(1, _attempts + 1):
+            invert_logic = not _expected
+            while True:
+                seconds_left = (retry_until - datetime.now()).total_seconds()
                 try:
                     ret = func(*args, **kwargs)
                     logger.debug("Function returned %s", ret)
-                    if _return_is_str and isinstance(ret, bool) and _expected:
-                        return ret
-                    if (
-                        isinstance(ret, str) or isinstance(ret, unicode)
-                    ) and _expected is False:
-                        return ret
-                    if _return_is_dict and isinstance(ret, dict):
-                        return ret
-
-                    if _attempts == i:
-                        generate_support_bundle()
-                        return ret
-                except Exception as err:
-                    if _attempts == i:
-                        generate_support_bundle()
-                        logger.info("Max number of attempts (%r) reached", _attempts)
-                        raise
-                    else:
-                        logger.info("Function returned %s", err)
-                if i < _attempts:
-                    logger.info("Retry [#%r] after sleeping for %ss" % (i, _wait))
-                    sleep(_wait)
+
+                    negative_result = ret is False or is_string(ret)
+                    if negative_result == invert_logic:
+                        # Simple case, successful result in time
+                        if not saved_failure:
+                            return ret
+
+                        # Positive result, but happened after timeout failure, very important to
+                        # note for fixing tests.
+                        logger.warning("RETRY DIAGNOSTIC: SUCCEED after FAILED with requested timeout of %.1fs; however, succeeded in %.1fs, investigate timeout timing",
+                                       _retry_timeout, (datetime.now() - start_time).total_seconds())
+                        if isinstance(saved_failure, Exception):
+                            raise saved_failure             # pylint: disable=E0702
+                        return saved_failure
+
+                except Exception as error:
+                    logger.info("Function raised exception: %s", str(error))
+                    ret = error
+
+                if seconds_left < 0 and saved_failure:
+                    logger.info("RETRY DIAGNOSTIC: Retry timeout reached, still failing")
+                    if isinstance(saved_failure, Exception):
+                        raise saved_failure                 # pylint: disable=E0702
+                    return saved_failure
+
+                if seconds_left < 0:
+                    logger.info("Retry timeout of %ds reached", _retry_timeout)
+
+                    saved_failure = ret
+                    retry_extra_delta = timedelta(seconds=seconds_left + _retry_timeout * _diag_pct)
+                    retry_until = datetime.now() + retry_extra_delta
+                    seconds_left = retry_extra_delta.total_seconds()
+
+                    # Generate bundle after setting remaining diagnostic retry time
+                    generate_support_bundle()
+
+                    # If user has disabled diagnostic retries return now
+                    if not _diag_pct:
+                        if isinstance(saved_failure, Exception):
+                            raise saved_failure
+                        return saved_failure
+
+                if saved_failure:
+                    logger.info("RETRY DIAG: [failure] Sleeping %ds until next retry with %.1f retry time left - too see if timeout was too short",
+                                retry_sleep, seconds_left)
+                else:
+                    logger.info("Sleeping %ds until next retry with %.1f retry time left",
+                                retry_sleep, seconds_left)
+                sleep(retry_sleep)
 
         func_retry._original = func
         return func_retry
@@ -2881,7 +2918,7 @@ def configure_interface_mac(tgen, input_dict):
 #############################################
 # Verification APIs
 #############################################
-@retry(attempts=6, wait=2, return_is_str=True)
+@retry(retry_timeout=12)
 def verify_rib(
     tgen,
     addr_type,
@@ -3290,7 +3327,7 @@ def verify_rib(
     return True
 
 
-@retry(attempts=6, wait=2, return_is_str=True)
+@retry(retry_timeout=12)
 def verify_fib_routes(tgen, addr_type, dut, input_dict, next_hop=None):
     """
     Data will be read from input_dict or input JSON file, API will generate
@@ -3694,7 +3731,7 @@ def verify_prefix_lists(tgen, input_dict):
     return True
 
 
-@retry(attempts=3, wait=4, return_is_str=True)
+@retry(retry_timeout=12)
 def verify_route_maps(tgen, input_dict):
     """
     Running "show route-map" command and verifying given route-map
@@ -3746,7 +3783,7 @@ def verify_route_maps(tgen, input_dict):
     return True
 
 
-@retry(attempts=4, wait=4, return_is_str=True)
+@retry(retry_timeout=16)
 def verify_bgp_community(tgen, addr_type, router, network, input_dict=None):
     """
     API to veiryf BGP large community is attached in route for any given
@@ -3982,7 +4019,7 @@ def verify_cli_json(tgen, input_dict):
     return True
 
 
-@retry(attempts=3, wait=4, return_is_str=True)
+@retry(retry_timeout=12)
 def verify_evpn_vni(tgen, input_dict):
     """
     API to verify evpn vni details using "show evpn vni detail json"
@@ -4100,7 +4137,7 @@ def verify_evpn_vni(tgen, input_dict):
     return False
 
 
-@retry(attempts=3, wait=4, return_is_str=True)
+@retry(retry_timeout=12)
 def verify_vrf_vni(tgen, input_dict):
     """
     API to verify vrf vni details using "show vrf vni json"
diff --git a/tests/topotests/lib/ospf.py b/tests/topotests/lib/ospf.py
index 3f39b93d8c..dc9fe0fcca 100644
--- a/tests/topotests/lib/ospf.py
+++ b/tests/topotests/lib/ospf.py
@@ -579,7 +579,7 @@ def redistribute_ospf(tgen, topo, dut, route_type, **kwargs):
 ################################
 # Verification procs
 ################################
-@retry(attempts=40, wait=2, return_is_str=True)
+@retry(retry_timeout=80)
 def verify_ospf_neighbor(tgen, topo, dut=None, input_dict=None, lan=False, expected=True):
     """
     This API is to verify ospf neighborship by running
@@ -774,7 +774,7 @@ def verify_ospf_neighbor(tgen, topo, dut=None, input_dict=None, lan=False, expec
 ################################
 # Verification procs
 ################################
-@retry(attempts=10, wait=2, return_is_str=True)
+@retry(retry_timeout=20)
 def verify_ospf6_neighbor(tgen, topo, dut=None, input_dict=None, lan=False):
     """
     This API is to verify ospf neighborship by running
@@ -959,7 +959,7 @@ def verify_ospf6_neighbor(tgen, topo, dut=None, input_dict=None, lan=False):
     return result
 
 
-@retry(attempts=21, wait=2, return_is_str=True)
+@retry(retry_timeout=40)
 def verify_ospf_rib(
     tgen, dut, input_dict, next_hop=None, tag=None, metric=None, fib=None, expected=True
 ):
@@ -1236,7 +1236,7 @@ def verify_ospf_rib(
     return result
 
 
-@retry(attempts=10, wait=2, return_is_str=True)
+@retry(retry_timeout=20)
 def verify_ospf_interface(tgen, topo, dut=None, lan=False, input_dict=None, expected=True):
     """
     This API is to verify ospf routes by running
@@ -1326,7 +1326,7 @@ def verify_ospf_interface(tgen, topo, dut=None, lan=False, input_dict=None, expe
     return result
 
 
-@retry(attempts=11, wait=2, return_is_str=True)
+@retry(retry_timeout=20)
 def verify_ospf_database(tgen, topo, dut, input_dict, expected=True):
     """
     This API is to verify ospf lsa's by running
@@ -1490,7 +1490,7 @@ def verify_ospf_database(tgen, topo, dut, input_dict, expected=True):
     return result
 
 
-@retry(attempts=10, wait=2, return_is_str=True)
+@retry(retry_timeout=20)
 def verify_ospf_summary(tgen, topo, dut, input_dict, expected=True):
     """
     This API is to verify ospf routes by running
@@ -1571,7 +1571,7 @@ def verify_ospf_summary(tgen, topo, dut, input_dict, expected=True):
 
 
 
-@retry(attempts=10, wait=3, return_is_str=True)
+@retry(retry_timeout=30)
 def verify_ospf6_rib(tgen, dut, input_dict, next_hop=None,
             tag=None, metric=None, fib=None):
     """
@@ -1811,7 +1811,7 @@ def verify_ospf6_rib(tgen, dut, input_dict, next_hop=None,
     return result
 
 
-@retry(attempts=3, wait=2, return_is_str=True)
+@retry(retry_timeout=6)
 def verify_ospf6_interface(tgen, topo, dut=None,lan=False, input_dict=None):
     """
     This API is to verify ospf routes by running
@@ -1905,7 +1905,7 @@ def verify_ospf6_interface(tgen, topo, dut=None,lan=False, input_dict=None):
     return result
 
 
-@retry(attempts=11, wait=2, return_is_str=True)
+@retry(retry_timeout=20)
 def verify_ospf6_database(tgen, topo, dut, input_dict):
     """
     This API is to verify ospf lsa's by running
@@ -2176,9 +2176,9 @@ def config_ospf6_interface (tgen, topo, input_dict=None, build=False,
         config_data = []
         for lnk in input_dict[router]['links'].keys():
             if "ospf6" not in input_dict[router]['links'][lnk]:
-                logger.debug("Router %s: ospf6 configs is not present in"
-                             "input_dict, passed input_dict", router,
-                             input_dict)
+                logger.debug("Router %s: ospf6 config is not present in"
+                             "input_dict, passed input_dict %s", router,
+                             str(input_dict))
                 continue
             ospf_data = input_dict[router]['links'][lnk]['ospf6']
             data_ospf_area = ospf_data.setdefault("area", None)
diff --git a/tests/topotests/lib/pim.py b/tests/topotests/lib/pim.py
index ce90717fa4..7de1c7a2f9 100644
--- a/tests/topotests/lib/pim.py
+++ b/tests/topotests/lib/pim.py
@@ -495,7 +495,7 @@ def configure_pim_force_expire(tgen, topo, input_dict, build=False):
 #############################################
 # Verification APIs
 #############################################
-@retry(attempts=6, wait=2, return_is_str=True)
+@retry(retry_timeout=12)
 def verify_pim_neighbors(tgen, topo, dut=None, iface=None, nbr_ip=None, expected=True):
     """
     Verify all PIM neighbors are up and running, config is verified
@@ -619,7 +619,7 @@ def verify_pim_neighbors(tgen, topo, dut=None, iface=None, nbr_ip=None, expected
     return True
 
 
-@retry(attempts=21, wait=2, return_is_str=True)
+@retry(retry_timeout=40)
 def verify_igmp_groups(tgen, dut, interface, group_addresses, expected=True):
     """
     Verify IGMP groups are received from an intended interface
@@ -693,7 +693,7 @@ def verify_igmp_groups(tgen, dut, interface, group_addresses, expected=True):
     return True
 
 
-@retry(attempts=31, wait=2, return_is_str=True)
+@retry(retry_timeout=60)
 def verify_upstream_iif(
     tgen, dut, iif, src_address, group_addresses, joinState=None, refCount=1, expected=True
 ):
@@ -847,7 +847,7 @@ def verify_upstream_iif(
         return True
 
 
-@retry(attempts=6, wait=2, return_is_str=True)
+@retry(retry_timeout=12)
 def verify_join_state_and_timer(tgen, dut, iif, src_address, group_addresses, expected=True):
     """
     Verify  join state is updated correctly and join timer is
@@ -966,7 +966,7 @@ def verify_join_state_and_timer(tgen, dut, iif, src_address, group_addresses, ex
     return True
 
 
-@retry(attempts=41, wait=2, return_is_dict=True)
+@retry(retry_timeout=80)
 def verify_ip_mroutes(
     tgen, dut, src_address, group_addresses, iif, oil, return_uptime=False, mwait=0, expected=True
 ):
@@ -1163,7 +1163,7 @@ def verify_ip_mroutes(
     return True if return_uptime == False else uptime_dict
 
 
-@retry(attempts=31, wait=2, return_is_str=True)
+@retry(retry_timeout=60)
 def verify_pim_rp_info(
     tgen, topo, dut, group_addresses, oif=None, rp=None, source=None, iamrp=None, expected=True
 ):
@@ -1320,7 +1320,7 @@ def verify_pim_rp_info(
     return True
 
 
-@retry(attempts=31, wait=2, return_is_str=True)
+@retry(retry_timeout=60)
 def verify_pim_state(
     tgen, dut, iif, oil, group_addresses, src_address=None, installed_fl=None, expected=True
 ):
@@ -1490,7 +1490,7 @@ def verify_pim_interface_traffic(tgen, input_dict):
     return output_dict
 
 
-@retry(attempts=21, wait=2, return_is_str=True)
+@retry(retry_timeout=40)
 def verify_pim_interface(tgen, topo, dut, interface=None, interface_ip=None, expected=True):
     """
     Verify all PIM interface are up and running, config is verified
@@ -1797,7 +1797,7 @@ def clear_ip_igmp_interfaces(tgen, dut):
     return True
 
 
-@retry(attempts=10, wait=2, return_is_str=True)
+@retry(retry_timeout=20)
 def clear_ip_mroute_verify(tgen, dut, expected=True):
     """
     Clear ip mroute by running "clear ip mroute" cli and verify
@@ -2173,7 +2173,7 @@ def find_rp_from_bsrp_info(tgen, dut, bsr, grp=None):
     return rp_details
 
 
-@retry(attempts=6, wait=2, return_is_str=True)
+@retry(retry_timeout=12)
 def verify_pim_grp_rp_source(tgen, topo, dut, grp_addr, rp_source, rpadd=None, expected=True):
     """
     Verify pim rp info by running "show ip pim rp-info" cli
@@ -2276,7 +2276,7 @@ def verify_pim_grp_rp_source(tgen, topo, dut, grp_addr, rp_source, rpadd=None, e
     return errormsg
 
 
-@retry(attempts=31, wait=2, return_is_str=True)
+@retry(retry_timeout=60)
 def verify_pim_bsr(tgen, topo, dut, bsr_ip, expected=True):
     """
     Verify all PIM interface are up and running, config is verified
@@ -2332,7 +2332,7 @@ def verify_pim_bsr(tgen, topo, dut, bsr_ip, expected=True):
     return True
 
 
-@retry(attempts=31, wait=2, return_is_str=True)
+@retry(retry_timeout=60)
 def verify_ip_pim_upstream_rpf(tgen, topo, dut, interface, group_addresses, rp=None, expected=True):
     """
     Verify IP PIM upstream rpf, config is verified
@@ -2530,7 +2530,7 @@ def enable_disable_pim_bsm(tgen, router, intf, enable=True):
     return result
 
 
-@retry(attempts=31, wait=2, return_is_str=True)
+@retry(retry_timeout=60)
 def verify_ip_pim_join(tgen, topo, dut, interface, group_addresses, src_address=None, expected=True):
     """
     Verify ip pim join by running "show ip pim join" cli
@@ -2621,7 +2621,7 @@ def verify_ip_pim_join(tgen, topo, dut, interface, group_addresses, src_address=
     return True
 
 
-@retry(attempts=31, wait=2, return_is_dict=True)
+@retry(retry_timeout=60)
 def verify_igmp_config(tgen, input_dict, stats_return=False, expected=True):
     """
     Verify igmp interface details, verifying following configs:
@@ -2911,7 +2911,7 @@ def verify_igmp_config(tgen, input_dict, stats_return=False, expected=True):
     return True if stats_return == False else igmp_stats
 
 
-@retry(attempts=31, wait=2, return_is_str=True)
+@retry(retry_timeout=60)
 def verify_pim_config(tgen, input_dict, expected=True):
     """
     Verify pim interface details, verifying following configs:
@@ -3037,7 +3037,7 @@ def verify_pim_config(tgen, input_dict, expected=True):
     return True
 
 
-@retry(attempts=21, wait=2, return_is_dict=True)
+@retry(retry_timeout=40)
 def verify_multicast_traffic(tgen, input_dict, return_traffic=False, expected=True):
     """
     Verify multicast traffic by running
@@ -3280,7 +3280,7 @@ def get_refCount_for_mroute(tgen, dut, iif, src_address, group_addresses):
     return refCount
 
 
-@retry(attempts=21, wait=2, return_is_str=True)
+@retry(retry_timeout=40)
 def verify_multicast_flag_state(tgen, dut, src_address, group_addresses, flag, expected=True):
     """
     Verify flag state for mroutes and make sure (*, G)/(S, G) are having
@@ -3375,7 +3375,7 @@ def verify_multicast_flag_state(tgen, dut, src_address, group_addresses, flag, e
     return True
 
 
-@retry(attempts=21, wait=2, return_is_str=True)
+@retry(retry_timeout=40)
 def verify_igmp_interface(tgen, topo, dut, igmp_iface, interface_ip, expected=True):
     """
     Verify all IGMP interface are up and running, config is verified
diff --git a/tests/topotests/multicast_pim_bsm_topo2/test_mcast_pim_bsmp_02.py b/tests/topotests/multicast_pim_bsm_topo2/test_mcast_pim_bsmp_02.py
index 199746d5f6..60bd6de35d 100644
--- a/tests/topotests/multicast_pim_bsm_topo2/test_mcast_pim_bsmp_02.py
+++ b/tests/topotests/multicast_pim_bsm_topo2/test_mcast_pim_bsmp_02.py
@@ -456,7 +456,7 @@ def test_starg_mroute_p0(request):
     # Verify mroute not installed
     step("Verify mroute not installed in l1")
     result = verify_ip_mroutes(
-        tgen, dut, src_addr, GROUP_ADDRESS, iif, oil, wait=20, expected=False
+        tgen, dut, src_addr, GROUP_ADDRESS, iif, oil, retry_timeout=20, expected=False
     )
     assert (
         result is not True
@@ -705,6 +705,7 @@ def test_RP_priority_p0(request):
     ), "Testcase {} :Failed \n Error : rp expected {} rp received {}".format(
         tc_name,
         rp_add1,
+        rp2[group] if group in rp2 else None
     )
 
     # Verify if that rp is installed
diff --git a/tests/topotests/ospf_basic_functionality/test_ospf_authentication.py b/tests/topotests/ospf_basic_functionality/test_ospf_authentication.py
index 9c3be58937..a7f2893eab 100644
--- a/tests/topotests/ospf_basic_functionality/test_ospf_authentication.py
+++ b/tests/topotests/ospf_basic_functionality/test_ospf_authentication.py
@@ -257,7 +257,7 @@ def test_ospf_authentication_simple_pass_tc28_p1(request):
     sleep(6)
     dut = "r2"
     ospf_covergence = verify_ospf_neighbor(
-        tgen, topo, dut=dut, expected=False, attempts=5
+        tgen, topo, dut=dut, expected=False, retry_timeout=10
     )
     assert ospf_covergence is not True, "setup_module :Failed \n Error:" " {}".format(
         ospf_covergence
@@ -395,7 +395,7 @@ def test_ospf_authentication_md5_tc29_p1(request):
     sleep(6)
     dut = "r1"
     ospf_covergence = verify_ospf_neighbor(
-        tgen, topo, dut=dut, expected=False, attempts=3
+        tgen, topo, dut=dut, expected=False, retry_timeout=6
     )
     assert ospf_covergence is not True, "setup_module :Failed \n Error:" " {}".format(
         ospf_covergence
@@ -460,7 +460,7 @@ def test_ospf_authentication_md5_tc29_p1(request):
     sleep(6)
     dut = "r2"
     ospf_covergence = verify_ospf_neighbor(
-        tgen, topo, dut=dut, expected=False, attempts=5
+        tgen, topo, dut=dut, expected=False, retry_timeout=10
     )
     assert ospf_covergence is not True, "setup_module :Failed \n Error:" " {}".format(
         ospf_covergence
@@ -610,7 +610,7 @@ def test_ospf_authentication_different_auths_tc30_p1(request):
     step("Verify that the neighbour is not FULL between R1 and R2.")
     dut = "r1"
     ospf_covergence = verify_ospf_neighbor(
-        tgen, topo, dut=dut, expected=False, attempts=5
+        tgen, topo, dut=dut, expected=False, retry_timeout=10
     )
     assert ospf_covergence is not True, "setup_module :Failed \n Error:" " {}".format(
         ospf_covergence
diff --git a/tests/topotests/ospf_basic_functionality/test_ospf_ecmp.py b/tests/topotests/ospf_basic_functionality/test_ospf_ecmp.py
index 1aabc06db0..49ecaac9f7 100644
--- a/tests/topotests/ospf_basic_functionality/test_ospf_ecmp.py
+++ b/tests/topotests/ospf_basic_functionality/test_ospf_ecmp.py
@@ -326,7 +326,7 @@ def test_ospf_ecmp_tc16_p0(request):
     step("Verify that route is withdrawn from R2.")
     dut = "r1"
     result = verify_ospf_rib(
-        tgen, dut, input_dict, next_hop=nh, attempts=5, expected=False
+        tgen, dut, input_dict, next_hop=nh, retry_timeout=10, expected=False
     )
     assert (
         result is not True
@@ -342,7 +342,7 @@ def test_ospf_ecmp_tc16_p0(request):
         input_dict,
         protocol=protocol,
         next_hop=nh,
-        attempts=5,
+        retry_timeout=10,
         expected=False,
     )
     assert (
@@ -434,7 +434,7 @@ def test_ospf_ecmp_tc17_p0(request):
     step("Verify that route is withdrawn from R2.")
     dut = "r1"
     result = verify_ospf_rib(
-        tgen, dut, input_dict, next_hop=nh, attempts=5, expected=False
+        tgen, dut, input_dict, next_hop=nh, retry_timeout=10, expected=False
     )
     assert (
         result is not True
@@ -450,7 +450,7 @@ def test_ospf_ecmp_tc17_p0(request):
         input_dict,
         protocol=protocol,
         next_hop=nh,
-        attempts=5,
+        retry_timeout=10,
         expected=False,
     )
     assert (
diff --git a/tests/topotests/ospf_basic_functionality/test_ospf_ecmp_lan.py b/tests/topotests/ospf_basic_functionality/test_ospf_ecmp_lan.py
index e6dc18a434..47c6c45e39 100644
--- a/tests/topotests/ospf_basic_functionality/test_ospf_ecmp_lan.py
+++ b/tests/topotests/ospf_basic_functionality/test_ospf_ecmp_lan.py
@@ -305,7 +305,7 @@ def test_ospf_lan_ecmp_tc18_p0(request):
     step("Verify that all the routes are withdrawn from R0")
     dut = "r1"
     result = verify_ospf_rib(
-        tgen, dut, input_dict, next_hop=nh, attempts=5, expected=False
+        tgen, dut, input_dict, next_hop=nh, retry_timeout=10, expected=False
     )
     assert (
         result is not True
@@ -321,7 +321,7 @@ def test_ospf_lan_ecmp_tc18_p0(request):
         input_dict,
         protocol=protocol,
         next_hop=nh,
-        attempts=5,
+        retry_timeout=10,
         expected=False,
     )
     assert (
diff --git a/tests/topotests/ospf_basic_functionality/test_ospf_routemaps.py b/tests/topotests/ospf_basic_functionality/test_ospf_routemaps.py
index 7864d0307a..0848f6c94a 100644
--- a/tests/topotests/ospf_basic_functionality/test_ospf_routemaps.py
+++ b/tests/topotests/ospf_basic_functionality/test_ospf_routemaps.py
@@ -501,7 +501,7 @@ def test_ospf_routemaps_functionality_tc20_p0(request):
 
     dut = "r1"
     protocol = "ospf"
-    result = verify_ospf_rib(tgen, dut, input_dict, attempts=2, expected=False)
+    result = verify_ospf_rib(tgen, dut, input_dict, retry_timeout=4, expected=False)
     assert (
         result is not True
     ), "Testcase {} : Failed \n " "r1: OSPF routes are present \n Error: {}".format(
@@ -509,7 +509,7 @@ def test_ospf_routemaps_functionality_tc20_p0(request):
     )
 
     result = verify_rib(
-        tgen, "ipv4", dut, input_dict, protocol=protocol, attempts=2, expected=False
+        tgen, "ipv4", dut, input_dict, protocol=protocol, retry_timeout=4, expected=False
     )
     assert (
         result is not True
diff --git a/tests/topotests/ospf_basic_functionality/test_ospf_rte_calc.py b/tests/topotests/ospf_basic_functionality/test_ospf_rte_calc.py
index 9dfde325f6..f17346d5b1 100644
--- a/tests/topotests/ospf_basic_functionality/test_ospf_rte_calc.py
+++ b/tests/topotests/ospf_basic_functionality/test_ospf_rte_calc.py
@@ -263,7 +263,7 @@ def test_ospf_redistribution_tc5_p0(request):
         input_dict,
         protocol=protocol,
         next_hop=nh,
-        attempts=5,
+        retry_timeout=10,
         expected=False,
     )
     assert result is not True, (
diff --git a/tests/topotests/static_routing_with_ebgp/test_static_routes_topo2_ebgp.py b/tests/topotests/static_routing_with_ebgp/test_static_routes_topo2_ebgp.py
index bbb4370a93..2c44ec2351 100644
--- a/tests/topotests/static_routing_with_ebgp/test_static_routes_topo2_ebgp.py
+++ b/tests/topotests/static_routing_with_ebgp/test_static_routes_topo2_ebgp.py
@@ -690,9 +690,8 @@ def test_static_route_8nh_diff_AD_bgp_ecmp_p1_tc6_ebgp(request):
             next_hop=nh,
             protocol=protocol,
             fib=True,
+            retry_timeout=6,
             expected=False,
-            wait=2,
-            attempts=3,
         )
         assert (
             result is not True
@@ -804,8 +803,7 @@ def test_static_route_8nh_diff_AD_bgp_ecmp_p1_tc6_ebgp(request):
             protocol=protocol,
             fib=True,
             expected=False,
-            wait=2,
-            attempts=3,
+            retry_timeout=6,
         )
         assert (
             result is not True
@@ -1283,8 +1281,7 @@ def test_static_route_8nh_diff_AD_bgp_ecmp_p1_tc10_ebgp(request):
             protocol=protocol,
             fib=True,
             expected=False,
-            wait=2,
-            attempts=3,
+            retry_timeout=6,
         )
         assert (
             result is not True
diff --git a/tests/topotests/static_routing_with_ibgp/test_static_routes_topo2_ibgp.py b/tests/topotests/static_routing_with_ibgp/test_static_routes_topo2_ibgp.py
index ee0e01b411..85b9e8b543 100644
--- a/tests/topotests/static_routing_with_ibgp/test_static_routes_topo2_ibgp.py
+++ b/tests/topotests/static_routing_with_ibgp/test_static_routes_topo2_ibgp.py
@@ -695,12 +695,11 @@ def test_static_route_8nh_diff_AD_bgp_ecmp_p1_tc6_ibgp(request):
             protocol=protocol,
             fib=True,
             expected=False,
-            wait=2,
-            attempts=3,
+            retry_timeout=6,
         )
         assert (
             result is not True
-        ), "Testcase {} : Failed \nError: Routes " " are missing in RIB".format(tc_name)
+        ), "Testcase {} : Failed \nError: Routes " " are present in RIB".format(tc_name)
 
     step(
         "Remove the static route configured with nexthop N1 to N8, one"
@@ -808,8 +807,7 @@ def test_static_route_8nh_diff_AD_bgp_ecmp_p1_tc6_ibgp(request):
             protocol=protocol,
             fib=True,
             expected=False,
-            wait=2,
-            attempts=3,
+            retry_timeout=6,
         )
         assert (
             result is not True
@@ -1512,8 +1510,7 @@ def test_static_route_8nh_diff_AD_bgp_ecmp_p1_tc10_ibgp(request):
             protocol=protocol,
             fib=True,
             expected=False,
-            wait=2,
-            attempts=3,
+            retry_timeout=6,
         )
         assert (
             result is not True