Merge pull request #12050 from LabNConsulting/working/lb/topotest-220909

6 files changed, 337 insertions, 62 deletions

diff --git a/doc/developer/building-frr-for-ubuntu2204.rst b/doc/developer/building-frr-for-ubuntu2204.rst
new file mode 100644
index 000000000..6b941b367
--- /dev/null
+++ b/doc/developer/building-frr-for-ubuntu2204.rst
@@ -0,0 +1,171 @@
+Ubuntu 22.04 LTS
+================
+
+This document describes installation from source. If you want to build a
+``deb``, see :ref:`packaging-debian`.
+
+Installing Dependencies
+-----------------------
+
+.. code-block:: console
+
+   sudo apt update
+   sudo apt-get install \
+      git autoconf automake libtool make libreadline-dev texinfo \
+      pkg-config libpam0g-dev libjson-c-dev bison flex \
+      libc-ares-dev python3-dev python3-sphinx \
+      install-info build-essential libsnmp-dev perl \
+      libcap-dev python2 libelf-dev libunwind-dev \
+      libyang2 libyang2-dev
+
+.. include:: building-libunwind-note.rst
+
+Note that Ubuntu >= 20 no longer installs python 2.x, so it must be
+installed explicitly. Ensure that your system has a symlink named
+``/usr/bin/python`` pointing at ``/usr/bin/python3``.
+
+.. code-block:: shell
+
+   sudo ln -s /usr/bin/python3 /usr/bin/python
+   python --version
+
+In addition, ``pip`` for python2 must be installed if you wish to run
+the FRR topotests. That version of ``pip`` is not available from the
+ubuntu apt repositories; in order to install it:
+
+.. code-block:: shell
+
+   curl https://bootstrap.pypa.io/pip/2.7/get-pip.py --output get-pip.py
+   sudo python2 ./get-pip.py
+
+   # And verify the installation
+   pip2 --version
+
+
+Protobuf
+^^^^^^^^
+This is optional
+
+.. code-block:: console
+
+   sudo apt-get install protobuf-c-compiler libprotobuf-c-dev
+
+ZeroMQ
+^^^^^^
+This is optional
+
+.. code-block:: console
+
+   sudo apt-get install libzmq5 libzmq3-dev
+
+Building & Installing FRR
+-------------------------
+
+Add FRR user and groups
+^^^^^^^^^^^^^^^^^^^^^^^
+
+.. code-block:: console
+
+   sudo groupadd -r -g 92 frr
+   sudo groupadd -r -g 85 frrvty
+   sudo adduser --system --ingroup frr --home /var/run/frr/ \
+      --gecos "FRR suite" --shell /sbin/nologin frr
+   sudo usermod -a -G frrvty frr
+
+Compile
+^^^^^^^
+
+.. include:: include-compile.rst
+
+Install FRR configuration files
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+.. code-block:: console
+
+   sudo install -m 775 -o frr -g frr -d /var/log/frr
+   sudo install -m 775 -o frr -g frrvty -d /etc/frr
+   sudo install -m 640 -o frr -g frrvty tools/etc/frr/vtysh.conf /etc/frr/vtysh.conf
+   sudo install -m 640 -o frr -g frr tools/etc/frr/frr.conf /etc/frr/frr.conf
+   sudo install -m 640 -o frr -g frr tools/etc/frr/daemons.conf /etc/frr/daemons.conf
+   sudo install -m 640 -o frr -g frr tools/etc/frr/daemons /etc/frr/daemons
+
+Tweak sysctls
+^^^^^^^^^^^^^
+
+Some sysctls need to be changed in order to enable IPv4/IPv6 forwarding and
+MPLS (if supported by your platform). If your platform does not support MPLS,
+skip the MPLS related configuration in this section.
+
+Edit :file:`/etc/sysctl.conf` and uncomment the following values (ignore the
+other settings):
+
+::
+
+   # Uncomment the next line to enable packet forwarding for IPv4
+   net.ipv4.ip_forward=1
+
+   # Uncomment the next line to enable packet forwarding for IPv6
+   #  Enabling this option disables Stateless Address Autoconfiguration
+   #  based on Router Advertisements for this host
+   net.ipv6.conf.all.forwarding=1
+
+Reboot or use ``sysctl -p`` to apply the same config to the running system.
+
+Add MPLS kernel modules
+"""""""""""""""""""""""
+
+Ubuntu 20.04 ships with kernel 5.4; MPLS modules are present by default.  To
+enable, add the following lines to :file:`/etc/modules-load.d/modules.conf`:
+
+::
+
+   # Load MPLS Kernel Modules
+   mpls_router
+   mpls_iptunnel
+
+
+And load the kernel modules on the running system:
+
+.. code-block:: console
+
+   sudo modprobe mpls-router mpls-iptunnel
+
+If the above command returns an error, you may need to install the appropriate
+or latest linux-modules-extra-<kernel-version>-generic package. For example
+``apt-get install linux-modules-extra-`uname -r`-generic``
+
+Enable MPLS Forwarding
+""""""""""""""""""""""
+
+Edit :file:`/etc/sysctl.conf` and the following lines. Make sure to add a line
+equal to :file:`net.mpls.conf.eth0.input` for each interface used with MPLS.
+
+::
+
+   # Enable MPLS Label processing on all interfaces
+   net.mpls.conf.eth0.input=1
+   net.mpls.conf.eth1.input=1
+   net.mpls.conf.eth2.input=1
+   net.mpls.platform_labels=100000
+
+Install service files
+^^^^^^^^^^^^^^^^^^^^^
+
+.. code-block:: console
+
+   sudo install -m 644 tools/frr.service /etc/systemd/system/frr.service
+   sudo systemctl enable frr
+
+Enable daemons
+^^^^^^^^^^^^^^
+
+Open :file:`/etc/frr/daemons` with your text editor of choice. Look for the
+section with ``watchfrr_enable=...`` and ``zebra=...`` etc.  Enable the daemons
+as required by changing the value to ``yes``.
+
+Start FRR
+^^^^^^^^^
+
+.. code-block:: shell
+
+   systemctl start frr
diff --git a/doc/developer/topotests.rst b/doc/developer/topotests.rst
index 13936e18e..7b1fde7b5 100644
--- a/doc/developer/topotests.rst
+++ b/doc/developer/topotests.rst
@@ -22,10 +22,8 @@ Installing Topotest Requirements
 
 .. code:: shell
 
-   apt-get install gdb
-   apt-get install iproute2
-   apt-get install net-tools
-   apt-get install python3-pip
+   apt-get install gdb iproute2 net-tools python3-pip \
+      iputils-ping valgrind
    python3 -m pip install wheel
    python3 -m pip install 'pytest>=6.2.4'
    python3 -m pip install 'pytest-xdist>=2.3.0'
@@ -119,6 +117,7 @@ If you prefer to manually build FRR, then use the following suggested config:
        --sysconfdir=/etc/frr \
        --enable-vtysh \
        --enable-pimd \
+       --enable-pim6d \
        --enable-sharpd \
        --enable-multipath=64 \
        --enable-user=frr \
@@ -311,32 +310,6 @@ Test will set exit code which can be used with ``git bisect``.
 
 For the simulated topology, see the description in the python file.
 
-StdErr log from daemos after exit
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-To enable the reporting of any messages seen on StdErr after the daemons exit,
-the following env variable can be set::
-
-   export TOPOTESTS_CHECK_STDERR=Yes
-
-(The value doesn't matter at this time. The check is whether the env
-variable exists or not.) There is no pass/fail on this reporting; the
-Output will be reported to the console.
-
-Collect Memory Leak Information
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-FRR processes can report unfreed memory allocations upon exit. To
-enable the reporting of memory leaks, define an environment variable
-``TOPOTESTS_CHECK_MEMLEAK`` with the file prefix, i.e.::
-
-   export TOPOTESTS_CHECK_MEMLEAK="/home/mydir/memleak_"
-
-This will enable the check and output to console and the writing of
-the information to files with the given prefix (followed by testname),
-ie :file:`/home/mydir/memcheck_test_bgp_multiview_topo1.txt` in case
-of a memory leak.
-
 Running Topotests with AddressSanitizer
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
@@ -562,6 +535,48 @@ Here's an example of launching ``zebra`` and ``bgpd`` inside ``gdb`` on router
           --gdb-breakpoints=nb_config_diff \
           all-protocol-startup
 
+Reporting Memleaks with FRR Memory Statistics
+"""""""""""""""""""""""""""""""""""""""""""""
+
+FRR reports all allocated FRR memory objects on exit to standard error.
+Topotest can be run to report such output as errors in order to check for
+memleaks in FRR memory allocations. Specifying the CLI argument
+``--memleaks`` will enable reporting FRR-based memory allocations at exit as errors.
+
+.. code:: shell
+
+   sudo -E pytest --memleaks all-protocol-startup
+
+
+StdErr log from daemos after exit
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+When running with ``--memleaks``, to enable the reporting of other,
+non-memory related, messages seen on StdErr after the daemons exit,
+the following env variable can be set::
+
+   export TOPOTESTS_CHECK_STDERR=Yes
+
+(The value doesn't matter at this time. The check is whether the env
+variable exists or not.) There is no pass/fail on this reporting; the
+Output will be reported to the console.
+
+Collect Memory Leak Information
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+When running with ``--memleaks``, FRR processes report unfreed memory
+allocations upon exit. To enable also reporting of memory leaks to a specific
+location, define an environment variable ``TOPOTESTS_CHECK_MEMLEAK`` with the
+file prefix, i.e.:
+
+   export TOPOTESTS_CHECK_MEMLEAK="/home/mydir/memleak_"
+
+For tests that support the TOPOTESTS_CHECK_MEMLEAK environment variable, this
+will enable output to the information to files with the given prefix (followed
+by testname), e.g.,:
+file:`/home/mydir/memcheck_test_bgp_multiview_topo1.txt` in case
+of a memory leak.
+
 Detecting Memleaks with Valgrind
 """"""""""""""""""""""""""""""""
 
diff --git a/lib/libfrr.c b/lib/libfrr.c
index 07e2eafec..e89005726 100644
--- a/lib/libfrr.c
+++ b/lib/libfrr.c
@@ -130,6 +130,7 @@ static const struct optspec os_always = {
 	"      --limit-fds    Limit number of fds supported\n",
 	lo_always};
 
+static bool logging_to_stdout = false; /* set when --log stdout specified */
 
 static const struct option lo_cfg[] = {
 	{"config_file", required_argument, NULL, 'f'},
@@ -738,6 +739,11 @@ struct event_loop *frr_init(void)
 	while ((log_arg = log_args_pop(di->early_logging))) {
 		command_setup_early_logging(log_arg->target,
 					    di->early_loglevel);
+		/* this is a bit of a hack,
+		   but need to notice when
+		   the target is stdout */
+		if (strcmp(log_arg->target, "stdout") == 0)
+			logging_to_stdout = true;
 		XFREE(MTYPE_TMP, log_arg);
 	}
 
@@ -1088,9 +1094,15 @@ static void frr_terminal_close(int isexit)
 			     "%s: failed to open /dev/null: %s", __func__,
 			     safe_strerror(errno));
 	} else {
-		dup2(nullfd, 0);
-		dup2(nullfd, 1);
-		dup2(nullfd, 2);
+		int fd;
+		/*
+		 * only redirect stdin, stdout, stderr to null when a tty also
+		 * don't redirect when stdout is set with --log stdout
+		 */
+		for (fd = 2; fd >= 0; fd--)
+			if (isatty(fd) &&
+			    (fd != STDOUT_FILENO || !logging_to_stdout))
+				dup2(nullfd, fd);
 		close(nullfd);
 	}
 }
@@ -1168,9 +1180,16 @@ void frr_run(struct event_loop *master)
 				     "%s: failed to open /dev/null: %s",
 				     __func__, safe_strerror(errno));
 		} else {
-			dup2(nullfd, 0);
-			dup2(nullfd, 1);
-			dup2(nullfd, 2);
+			int fd;
+			/*
+			 * only redirect stdin, stdout, stderr to null when a
+			 * tty also don't redirect when stdout is set with --log
+			 * stdout
+			 */
+			for (fd = 2; fd >= 0; fd--)
+				if (isatty(fd) &&
+				    (fd != STDOUT_FILENO || !logging_to_stdout))
+					dup2(nullfd, fd);
 			close(nullfd);
 		}
 
@@ -1194,7 +1213,7 @@ void frr_fini(void)
 {
 	FILE *fp;
 	char filename[128];
-	int have_leftovers;
+	int have_leftovers = 0;
 
 	hook_call(frr_fini);
 
@@ -1220,16 +1239,15 @@ void frr_fini(void)
 	/* frrmod_init -> nothing needed / hooks */
 	rcu_shutdown();
 
-	if (!debug_memstats_at_exit)
-		return;
-
-	have_leftovers = log_memstats(stderr, di->name);
+	/* also log memstats to stderr when stderr goes to a file*/
+	if (debug_memstats_at_exit || !isatty(STDERR_FILENO))
+		have_leftovers = log_memstats(stderr, di->name);
 
 	/* in case we decide at runtime that we want exit-memstats for
-	 * a daemon, but it has no stderr because it's daemonized
+	 * a daemon
 	 * (only do this if we actually have something to print though)
 	 */
-	if (!have_leftovers)
+	if (!debug_memstats_at_exit || !have_leftovers)
 		return;
 
 	snprintf(filename, sizeof(filename), "/tmp/frr-memstats-%s-%llu-%llu",
diff --git a/tests/topotests/all_protocol_startup/test_all_protocol_startup.py b/tests/topotests/all_protocol_startup/test_all_protocol_startup.py
index 92bb99c8f..4b7c4de80 100644
--- a/tests/topotests/all_protocol_startup/test_all_protocol_startup.py
+++ b/tests/topotests/all_protocol_startup/test_all_protocol_startup.py
@@ -198,6 +198,12 @@ def test_error_messages_daemons():
     if fatal_error != "":
         pytest.skip(fatal_error)
 
+    if os.environ.get("TOPOTESTS_CHECK_STDERR") is None:
+        print(
+            "SKIPPED final check on StdErr output: Disabled (TOPOTESTS_CHECK_STDERR undefined)\n"
+        )
+        pytest.skip("Skipping test for Stderr output")
+
     print("\n\n** Check for error messages in daemons")
     print("******************************************\n")
 
diff --git a/tests/topotests/conftest.py b/tests/topotests/conftest.py
index 74e308dbc..b78a2f105 100755
--- a/tests/topotests/conftest.py
+++ b/tests/topotests/conftest.py
@@ -87,6 +87,12 @@ def pytest_addoption(parser):
     )
 
     parser.addoption(
+        "--memleaks",
+        action="store_true",
+        help="Report memstat results as errors",
+    )
+
+    parser.addoption(
         "--pause",
         action="store_true",
         help="Pause after each test",
@@ -189,7 +195,7 @@ def pytest_addoption(parser):
     )
 
 
-def check_for_memleaks():
+def check_for_valgrind_memleaks():
     assert topotest.g_pytest_config.option.valgrind_memleaks
 
     leaks = []
@@ -232,11 +238,47 @@ def check_for_memleaks():
         pytest.fail("valgrind memleaks found for daemons: " + " ".join(daemons))
 
 
+def check_for_memleaks():
+    leaks = []
+    tgen = get_topogen()  # pylint: disable=redefined-outer-name
+    latest = []
+    existing = []
+    if tgen is not None:
+        logdir = tgen.logdir
+        if hasattr(tgen, "memstat_existing_files"):
+            existing = tgen.memstat_existing_files
+        latest = glob.glob(os.path.join(logdir, "*/*.err"))
+
+    daemons = []
+    for vfile in latest:
+        if vfile in existing:
+            continue
+        with open(vfile, encoding="ascii") as vf:
+            vfcontent = vf.read()
+            num = vfcontent.count("memstats:")
+            if num:
+                existing.append(vfile)  # have summary don't check again
+                emsg = "{} types in {}".format(num, vfile)
+                leaks.append(emsg)
+                daemon = re.match(r".*test[a-z_A-Z0-9\+]*/(.*)\.err", vfile).group(1)
+                daemons.append("{}({})".format(daemon, num))
+
+    if tgen is not None:
+        tgen.memstat_existing_files = existing
+
+    if leaks:
+        logger.error("memleaks found:\n\t%s", "\n\t".join(leaks))
+        pytest.fail("memleaks found for daemons: " + " ".join(daemons))
+
+
 @pytest.fixture(autouse=True, scope="module")
 def module_check_memtest(request):
     yield
     if request.config.option.valgrind_memleaks:
         if get_topogen() is not None:
+            check_for_valgrind_memleaks()
+    if request.config.option.memleaks:
+        if get_topogen() is not None:
             check_for_memleaks()
 
 
@@ -264,6 +306,8 @@ def pytest_runtest_call(item: pytest.Item) -> None:
 
     # Check for leaks if requested
     if item.config.option.valgrind_memleaks:
+        check_for_valgrind_memleaks()
+    if item.config.option.memleaks:
         check_for_memleaks()
 
 
@@ -370,10 +414,22 @@ def pytest_configure(config):
     if config.option.topology_only and is_xdist:
         pytest.exit("Cannot use --topology-only with distributed test mode")
 
+        pytest.exit("Cannot use --topology-only with distributed test mode")
+
     # Check environment now that we have config
     if not diagnose_env(rundir):
         pytest.exit("environment has errors, please read the logs in %s" % rundir)
 
+    # slave TOPOTESTS_CHECK_MEMLEAK to memleaks flag
+    if config.option.memleaks:
+        if "TOPOTESTS_CHECK_MEMLEAK" not in os.environ:
+            os.environ["TOPOTESTS_CHECK_MEMLEAK"] = "/dev/null"
+    else:
+        if "TOPOTESTS_CHECK_MEMLEAK" in os.environ:
+            del os.environ["TOPOTESTS_CHECK_MEMLEAK"]
+        if "TOPOTESTS_CHECK_STDERR" in os.environ:
+            del os.environ["TOPOTESTS_CHECK_STDERR"]
+
 
 @pytest.fixture(autouse=True, scope="session")
 def setup_session_auto():
diff --git a/tests/topotests/lib/topotest.py b/tests/topotests/lib/topotest.py
index 967f09ecb..9c594d3c8 100644
--- a/tests/topotests/lib/topotest.py
+++ b/tests/topotests/lib/topotest.py
@@ -1469,21 +1469,22 @@ class Router(Node):
                 if not running:
                     break
 
-        if not running:
-            return ""
-
-        logger.warning(
-            "%s: sending SIGBUS to: %s", self.name, ", ".join([x[0] for x in running])
-        )
-        for name, pid in running:
-            pidfile = "/var/run/{}/{}.pid".format(self.routertype, name)
-            logger.info("%s: killing %s", self.name, name)
-            self.cmd("kill -SIGBUS %d" % pid)
-            self.cmd("rm -- " + pidfile)
-
-        sleep(
-            0.5, "%s: waiting for daemons to exit/core after initial SIGBUS" % self.name
-        )
+        if running:
+            logger.warning(
+                "%s: sending SIGBUS to: %s",
+                self.name,
+                ", ".join([x[0] for x in running]),
+            )
+            for name, pid in running:
+                pidfile = "/var/run/{}/{}.pid".format(self.routertype, name)
+                logger.info("%s: killing %s", self.name, name)
+                self.cmd("kill -SIGBUS %d" % pid)
+                self.cmd("rm -- " + pidfile)
+
+            sleep(
+                0.5,
+                "%s: waiting for daemons to exit/core after initial SIGBUS" % self.name,
+            )
 
         errors = self.checkRouterCores(reportOnce=True)
         if self.checkRouterVersion("<", minErrorVersion):
@@ -1683,7 +1684,11 @@ class Router(Node):
         return self.getLog("out", daemon)
 
     def getLog(self, log, daemon):
-        return self.cmd("cat {}/{}/{}.{}".format(self.logdir, self.name, daemon, log))
+        filename = "{}/{}/{}.{}".format(self.logdir, self.name, daemon, log)
+        log = ""
+        with open(filename) as file:
+            log = file.read()
+        return log
 
     def startRouterDaemons(self, daemons=None, tgen=None):
         "Starts FRR daemons for this router."
@@ -1841,9 +1846,13 @@ class Router(Node):
                 logger.info(
                     "%s: %s %s launched in gdb window", self, self.routertype, daemon
                 )
-            elif daemon in perfds and (self.name in perfds[daemon] or "all" in perfds[daemon]):
+            elif daemon in perfds and (
+                self.name in perfds[daemon] or "all" in perfds[daemon]
+            ):
                 cmdopt += rediropt
-                cmd = " ".join(["perf record {} --".format(perf_options), binary, cmdopt])
+                cmd = " ".join(
+                    ["perf record {} --".format(perf_options), binary, cmdopt]
+                )
                 p = self.popen(cmd)
                 self.perf_daemons[daemon] = p
                 if p.poll() and p.returncode: