Merge pull request #8856 from LabNConsulting/chopps/topo-valgrind

doc/developer/topotests.rst

@@ -312,6 +312,20 @@ Here's an example of launching ``zebra`` and ``bgpd`` inside ``gdb`` on router
           --gdb-breakpoints=nb_config_diff \
           all-protocol-startup
 
+Detecting Memleaks with Valgrind
+""""""""""""""""""""""""""""""""
+
+Topotest can automatically launch all daemons with ``valgrind`` to check for
+memleaks. This is enabled by specifying 1 or 2 CLI arguments.
+``--valgrind-memleaks`` will enable general memleak detection, and
+``--valgrind-extra`` enables extra functionality including generating a
+suppression file. The suppression file ``tools/valgrind.supp`` is used when
+memleak detection is enabled.
+
+.. code:: shell
+
+   pytest --valgrind-memleaks all-protocol-startup
+
 .. _topotests_docker:
 
 Running Tests with Docker

tests/topotests/conftest.py

@@ -2,8 +2,10 @@
 Topotest conftest.py file.
 """
 
+import glob
 import os
 import pdb
+import re
 import pytest
 
 from lib.topogen import get_topogen, diagnose_env
@@ -11,6 +13,12 @@ from lib.topotest import json_cmp_result
 from lib.topotest import g_extra_config as topotest_extra_config
 from lib.topolog import logger
 
+try:
+    from _pytest._code.code import ExceptionInfo
+    leak_check_ok = True
+except ImportError:
+    leak_check_ok = False
+
 
 def pytest_addoption(parser):
     """
@@ -66,6 +74,18 @@ def pytest_addoption(parser):
         help="Only set up this topology, don't run tests",
     )
 
+    parser.addoption(
+        "--valgrind-extra",
+        action="store_true",
+        help="Generate suppression file, and enable more precise (slower) valgrind checks",
+    )
+
+    parser.addoption(
+        "--valgrind-memleaks",
+        action="store_true",
+        help="Run all daemons under valgrind for memleak detection",
+    )
+
     parser.addoption(
         "--vtysh",
         metavar="ROUTER[,ROUTER...]",
@@ -79,6 +99,37 @@ def pytest_addoption(parser):
     )
 
 
+def check_for_memleaks():
+    if not topotest_extra_config["valgrind_memleaks"]:
+        return
+
+    leaks = []
+    tgen = get_topogen()
+    latest = []
+    existing = []
+    if tgen is not None:
+        logdir = "/tmp/topotests/{}".format(tgen.modname)
+        if hasattr(tgen, "valgrind_existing_files"):
+            existing = tgen.valgrind_existing_files
+        latest = glob.glob(os.path.join(logdir, "*.valgrind.*"))
+
+    for vfile in latest:
+        if vfile in existing:
+            continue
+        with open(vfile) as vf:
+            vfcontent = vf.read()
+            match = re.search(r"ERROR SUMMARY: (\d+) errors", vfcontent)
+            if match and match.group(1) != "0":
+                emsg = '{} in {}'.format(match.group(1), vfile)
+                leaks.append(emsg)
+
+    if leaks:
+        if leak_check_ok:
+            pytest.fail("Memleaks found:\n\t" + "\n\t".join(leaks))
+        else:
+            logger.error("Memleaks found:\n\t" + "\n\t".join(leaks))
+
+
 def pytest_runtest_call():
     """
     This function must be run after setup_module(), it does standarized post
@@ -139,6 +190,9 @@ def pytest_configure(config):
     shell_on_error = config.getoption("--shell-on-error")
     topotest_extra_config["shell_on_error"] = shell_on_error
 
+    topotest_extra_config["valgrind_extra"] = config.getoption("--valgrind-extra")
+    topotest_extra_config["valgrind_memleaks"] = config.getoption("--valgrind-memleaks")
+
     vtysh = config.getoption("--vtysh")
     topotest_extra_config["vtysh"] = vtysh.split(",") if vtysh else []
 
@@ -159,6 +213,12 @@ def pytest_runtest_makereport(item, call):
     else:
         pause = False
 
+    if call.excinfo is None and call.when == "call":
+        try:
+            check_for_memleaks()
+        except:
+            call.excinfo = ExceptionInfo()
+
     if call.excinfo is None:
         error = False
     else:

tests/topotests/lib/topogen.py

@@ -657,6 +657,8 @@ class TopoRouter(TopoGear):
 
         # Try to find relevant old logfiles in /tmp and delete them
         map(os.remove, glob.glob("{}/{}/*.log".format(self.logdir, self.name)))
+        # Remove old valgrind files
+        map(os.remove, glob.glob("{}/{}.valgrind.*".format(self.logdir, self.name)))
         # Remove old core files
         map(os.remove, glob.glob("{}/{}/*.dmp".format(self.logdir, self.name)))
 

tests/topotests/lib/topotest.py

@@ -1454,6 +1454,8 @@ class Router(Node):
         gdb_breakpoints = g_extra_config["gdb_breakpoints"]
         gdb_daemons = g_extra_config["gdb_daemons"]
         gdb_routers = g_extra_config["gdb_routers"]
+        valgrind_extra = g_extra_config["valgrind_extra"]
+        valgrind_memleaks = g_extra_config["valgrind_memleaks"]
 
         bundle_data = ""
 
@@ -1503,7 +1505,14 @@ class Router(Node):
                 ) + "/var/run/{}/snmpd.pid -x /etc/frr/agentx".format(self.routertype)
             else:
                 binary = os.path.join(self.daemondir, daemon)
+
                 cmdenv = "ASAN_OPTIONS=log_path={0}.asan".format(daemon)
+                if valgrind_memleaks:
+                    this_dir = os.path.dirname(os.path.abspath(os.path.realpath(__file__)))
+                    supp_file = os.path.abspath(os.path.join(this_dir, "../../../tools/valgrind.supp"))
+                    cmdenv += " /usr/bin/valgrind --num-callers=50 --log-file={1}/{2}.valgrind.{0}.%p --leak-check=full --suppressions={3}".format(daemon, self.logdir, self.name, supp_file)
+                    if valgrind_extra:
+                        cmdenv += "--gen-suppressions=all --expensive-definedness-checks=yes"
                 cmdopt = "{} --log file:{}.log --log-level debug".format(
                     daemon_opts, daemon
                 )