frr: T6746: additional improvements after 10.2 upgrade (#4259)

* smoketest: T6746: add substring search in getFRRconfig()

Some daemons (e.g. bgpd) have several nested substrings/sections like

router bgp 100
 address-family ipv4 unicast
 ..
 exit-address-family
exit

We can now use getFRRconfig() with the substring option to extract only
 address-family ipv4 unicast
 ..
 exit-address-family

Making config validation more granular

* frrender: T6746: only re-render FRR config if config_dict did change

* frrender: T6746: fix naming glitch isis/eigrp

* frrender: T6746: add --stdout option when running with debug flags

* smoketest: T6746: remove unneeded commit_guard time

It was an invalid workarround as the underlaying issue seems to be a race
condition in CStore.

The commit process is not finished until all pending files from
VYATTA_CHANGES_ONLY_DIR are copied to VYATTA_ACTIVE_CONFIGURATION_DIR. This is
done inside libvyatta-cfg1 and the FUSE UnionFS part. On large non-interactive
commits FUSE UnionFS might not replicate the real state in time, leading to
errors when querying the working and effective configuration.

TO BE DELETED AFTER SWITCH TO IN MEMORY CONFIG

python/vyos/frrender.py

@@ -218,11 +218,11 @@ def dict_helper_pim_defaults(pim, path):
     # values present on the CLI - that's why we have if conf.exists()
     eigrp_cli_path = ['protocols', 'eigrp']
     if conf.exists(eigrp_cli_path):
-        isis = conf.get_config_dict(eigrp_cli_path, key_mangling=('-', '_'),
-                                    get_first_key=True,
-                                    no_tag_node_value_mangle=True,
-                                    with_recursive_defaults=True)
-        dict.update({'eigrp' : isis})
+        eigrp = conf.get_config_dict(eigrp_cli_path, key_mangling=('-', '_'),
+                                     get_first_key=True,
+                                     no_tag_node_value_mangle=True,
+                                     with_recursive_defaults=True)
+        dict.update({'eigrp' : eigrp})
     elif conf.exists_effective(eigrp_cli_path):
         dict.update({'eigrp' : {'deleted' : ''}})
 
@@ -537,14 +537,25 @@ def dict_helper_pim_defaults(pim, path):
     return dict
 
 class FRRender:
+    cached_config_dict = {}
     def __init__(self):
         self._frr_conf = '/run/frr/config/vyos.frr.conf'
 
     def generate(self, config_dict) -> None:
+        """
+        Generate FRR configuration file
+        Returns False if no changes to configuration were made, otherwise True
+        """
         if not isinstance(config_dict, dict):
             tmp = type(config_dict)
             raise ValueError(f'Config must be of type "dict" and not "{tmp}"!')
 
+
+        if self.cached_config_dict == config_dict:
+            debug('FRR:        NO CHANGES DETECTED')
+            return False
+        self.cached_config_dict = config_dict
+
         def inline_helper(config_dict) -> str:
             output = '!\n'
             if 'babel' in config_dict and 'deleted' not in config_dict['babel']:
@@ -639,7 +650,7 @@ def inline_helper(config_dict) -> str:
         debug(output)
         write_file(self._frr_conf, output)
         debug('FRR:        RENDERING CONFIG COMPLETE')
-        return None
+        return True
 
     def apply(self, count_max=5):
         count = 0
@@ -649,7 +660,7 @@ def apply(self, count_max=5):
             debug(f'FRR: reloading configuration - tries: {count} | Python class ID: {id(self)}')
             cmdline = '/usr/lib/frr/frr-reload.py --reload'
             if os.path.exists(frr_debug_enable):
-                cmdline += ' --debug'
+                cmdline += ' --debug --stdout'
             rc, emsg = rc_cmd(f'{cmdline} {self._frr_conf}')
             if rc != 0:
                 sleep(2)

smoketest/scripts/cli/base_vyostest_shim.py

@@ -18,7 +18,6 @@
 import pprint
 
 from time import sleep
-from time import time
 from typing import Type
 
 from vyos.configsession import ConfigSession
@@ -27,10 +26,17 @@
 from vyos.defaults import commit_lock
 from vyos.utils.process import cmd
 from vyos.utils.process import run
-from vyos.utils.process import process_named_running
 
 save_config = '/tmp/vyos-smoketest-save'
 
+# The commit process is not finished until all pending files from
+# VYATTA_CHANGES_ONLY_DIR are copied to VYATTA_ACTIVE_CONFIGURATION_DIR. This
+# is done inside libvyatta-cfg1 and the FUSE UnionFS part. On large non-
+# interactive commits FUSE UnionFS might not replicate the real state in time,
+# leading to errors when querying the working and effective configuration.
+# TO BE DELETED AFTER SWITCH TO IN MEMORY CONFIG
+CSTORE_GUARD_TIME = 4
+
 # This class acts as shim between individual Smoketests developed for VyOS and
 # the Python UnitTest framework. Before every test is loaded, we dump the current
 # system configuration and reload it after the test - despite the test results.
@@ -45,7 +51,6 @@ class TestCase(unittest.TestCase):
         # trigger the certain failure condition.
         # Use "self.debug = True" in derived classes setUp() method
         debug = False
-        commit_guard = time()
         @classmethod
         def setUpClass(cls):
             cls._session = ConfigSession(os.getpid())
@@ -86,14 +91,12 @@ def cli_commit(self):
             if self.debug:
                 print('commit')
             self._session.commit()
-            # during a commit there is a process opening commit_lock, and run() returns 0
+            # During a commit there is a process opening commit_lock, and run()
+            # returns 0
             while run(f'sudo lsof -nP {commit_lock}') == 0:
                 sleep(0.250)
-            # wait for FRR reload to be complete
-            while process_named_running('frr-reload.py'):
-                sleep(0.250)
-            # reset getFRRconfig() guard timer
-            self.commit_guard = time()
+            # Wait for CStore completion for fast non-interactive commits
+            sleep(CSTORE_GUARD_TIME)
 
         def op_mode(self, path : list) -> None:
             """
@@ -108,20 +111,27 @@ def op_mode(self, path : list) -> None:
                 pprint.pprint(out)
             return out
 
-        def getFRRconfig(self, string=None, end='$', endsection='^!', daemon='', guard_time=10, empty_retry=0):
-            """ Retrieve current "running configuration" from FRR """
-            # Sometimes FRR needs some time after reloading the configuration to
-            # appear in vtysh. This is a workaround addiung a 10 second guard timer
-            # between the last cli_commit() and the first read of FRR config via vtysh
-            while (time() - self.commit_guard) < guard_time:
-                sleep(0.250) # wait 250 milliseconds
-            command = f'vtysh -c "show run {daemon} no-header"'
-            if string: command += f' | sed -n "/^{string}{end}/,/{endsection}/p"'
+        def getFRRconfig(self, string=None, end='$', endsection='^!',
+                         substring=None, endsubsection=None, empty_retry=0):
+            """
+            Retrieve current "running configuration" from FRR
+
+            string:        search for a specific start string in the configuration
+            end:           end of the section to search for (line ending)
+            endsection:    end of the configuration
+            substring:     search section under the result found by string
+            endsubsection: end of the subsection (usually something with "exit")
+            """
+            command = f'vtysh -c "show run no-header"'
+            if string:
+                command += f' | sed -n "/^{string}{end}/,/{endsection}/p"'
+                if substring and endsubsection:
+                    command += f' | sed -n "/^{substring}/,/{endsubsection}/p"'
             out = cmd(command)
             if self.debug:
                 print(f'\n\ncommand "{command}" returned:\n')
                 pprint.pprint(out)
-            if empty_retry:
+            if empty_retry > 0:
                 retry_count = 0
                 while not out and retry_count < empty_retry:
                     if self.debug and retry_count % 10 == 0:

smoketest/scripts/cli/test_interfaces_bonding.py

@@ -24,7 +24,6 @@
 from vyos.configsession import ConfigSessionError
 from vyos.utils.network import get_interface_config
 from vyos.utils.file import read_file
-from vyos.frrender import mgmt_daemon
 
 class BondingInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
@@ -287,7 +286,7 @@ def test_bonding_evpn_multihoming(self):
 
         id = '5'
         for interface in self._interfaces:
-            frrconfig = self.getFRRconfig(f'interface {interface}', daemon=mgmt_daemon)
+            frrconfig = self.getFRRconfig(f'interface {interface}', endsection='^exit')
 
             self.assertIn(f' evpn mh es-id {id}', frrconfig)
             self.assertIn(f' evpn mh es-df-pref {id}', frrconfig)
@@ -304,7 +303,7 @@ def test_bonding_evpn_multihoming(self):
 
         id = '5'
         for interface in self._interfaces:
-            frrconfig = self.getFRRconfig(f'interface {interface}', daemon=mgmt_daemon)
+            frrconfig = self.getFRRconfig(f'interface {interface}', endsection='^exit')
             self.assertIn(f' evpn mh es-sys-mac 00:12:34:56:78:0{id}', frrconfig)
             self.assertIn(f' evpn mh uplink', frrconfig)
 

smoketest/scripts/cli/test_interfaces_ethernet.py

@@ -27,14 +27,12 @@
 from base_interfaces_test import BasicInterfaceTest
 from vyos.configsession import ConfigSessionError
 from vyos.ifconfig import Section
-from vyos.frrender import mgmt_daemon
 from vyos.utils.file import read_file
 from vyos.utils.network import is_intf_addr_assigned
 from vyos.utils.network import is_ipv6_link_local
 from vyos.utils.process import cmd
 from vyos.utils.process import popen
 
-
 class EthernetInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
@@ -222,14 +220,14 @@ def test_ethtool_flow_control(self):
                 out = loads(out)
                 self.assertFalse(out[0]['autonegotiate'])
 
-    def test_ethtool_evpn_uplink_tarcking(self):
+    def test_ethtool_evpn_uplink_tracking(self):
         for interface in self._interfaces:
             self.cli_set(self._base_path + [interface, 'evpn', 'uplink'])
 
         self.cli_commit()
 
         for interface in self._interfaces:
-            frrconfig = self.getFRRconfig(f'interface {interface}', daemon=mgmt_daemon)
+            frrconfig = self.getFRRconfig(f'interface {interface}', endsection='^exit')
             self.assertIn(' evpn mh uplink', frrconfig)
 
     def test_switchdev(self):
@@ -241,6 +239,5 @@ def test_switchdev(self):
 
         self.cli_delete(self._base_path + [interface, 'switchdev'])
 
-
 if __name__ == '__main__':
     unittest.main(verbosity=2)

smoketest/scripts/cli/test_protocols_babel.py

@@ -62,7 +62,7 @@ def test_01_basic(self):
 
         self.cli_commit()
 
-        frrconfig = self.getFRRconfig('router babel', endsection='^exit', daemon=babel_daemon)
+        frrconfig = self.getFRRconfig('router babel', endsection='^exit')
         self.assertIn(f' babel diversity', frrconfig)
         self.assertIn(f' babel diversity-factor {diversity_factor}', frrconfig)
         self.assertIn(f' babel resend-delay {resend_delay}', frrconfig)
@@ -81,7 +81,7 @@ def test_02_redistribute(self):
 
         self.cli_commit()
 
-        frrconfig = self.getFRRconfig('router babel', endsection='^exit', daemon=babel_daemon, empty_retry=5)
+        frrconfig = self.getFRRconfig('router babel', endsection='^exit', empty_retry=5)
         for protocol in ipv4_protos:
             self.assertIn(f' redistribute ipv4 {protocol}', frrconfig)
         for protocol in ipv6_protos:
@@ -150,7 +150,7 @@ def test_03_distribute_list(self):
 
         self.cli_commit()
 
-        frrconfig = self.getFRRconfig('router babel', endsection='^exit', daemon=babel_daemon)
+        frrconfig = self.getFRRconfig('router babel', endsection='^exit')
         self.assertIn(f' distribute-list {access_list_in4} in', frrconfig)
         self.assertIn(f' distribute-list {access_list_out4} out', frrconfig)
         self.assertIn(f' ipv6 distribute-list {access_list_in6} in', frrconfig)
@@ -198,11 +198,11 @@ def test_04_interfaces(self):
 
         self.cli_commit()
 
-        frrconfig = self.getFRRconfig('router babel', endsection='^exit', daemon=babel_daemon)
+        frrconfig = self.getFRRconfig('router babel', endsection='^exit')
         for interface in self._interfaces:
             self.assertIn(f' network {interface}', frrconfig)
 
-            iface_config = self.getFRRconfig(f'interface {interface}', endsection='^exit', daemon=babel_daemon)
+            iface_config = self.getFRRconfig(f'interface {interface}', endsection='^exit')
             self.assertIn(f' babel channel {channel}', iface_config)
             self.assertIn(f' babel enable-timestamps', iface_config)
             self.assertIn(f' babel update-interval {def_update_interval}', iface_config)

smoketest/scripts/cli/test_protocols_bfd.py

@@ -22,7 +22,6 @@
 from vyos.utils.process import process_named_running
 
 base_path = ['protocols', 'bfd']
-frr_endsection = '^ exit'
 
 dum_if = 'dum1001'
 vrf_name = 'red'
@@ -131,7 +130,7 @@ def test_bfd_peer(self):
         self.cli_commit()
 
         # Verify FRR bgpd configuration
-        frrconfig = self.getFRRconfig('bfd', endsection='^exit', daemon=bfd_daemon)
+        frrconfig = self.getFRRconfig('bfd', endsection='^exit')
         for peer, peer_config in peers.items():
             tmp = f'peer {peer}'
             if 'multihop' in peer_config:
@@ -144,8 +143,8 @@ def test_bfd_peer(self):
                 tmp += f' vrf {peer_config["vrf"]}'
 
             self.assertIn(tmp, frrconfig)
-            peerconfig = self.getFRRconfig(f' peer {peer}', end='', endsection=frr_endsection,
-                                           daemon=bfd_daemon)
+            peerconfig = self.getFRRconfig('bfd', endsection='^exit', substring=f' peer {peer}',
+                                           endsubsection='^ exit')
             if 'echo_mode' in peer_config:
                 self.assertIn(f'echo-mode', peerconfig)
             if 'intv_echo' in peer_config:
@@ -207,7 +206,8 @@ def test_bfd_profile(self):
 
         # Verify FRR bgpd configuration
         for profile, profile_config in profiles.items():
-            config = self.getFRRconfig(f' profile {profile}', endsection=frr_endsection)
+            config = self.getFRRconfig('bfd', endsection='^exit',
+                                       substring=f' profile {profile}', endsubsection='^ exit',)
             if 'echo_mode' in profile_config:
                 self.assertIn(f' echo-mode', config)
             if 'intv_echo' in profile_config:
@@ -229,8 +229,8 @@ def test_bfd_profile(self):
                 self.assertNotIn(f'shutdown', config)
 
         for peer, peer_config in peers.items():
-            peerconfig = self.getFRRconfig(f' peer {peer}', end='',
-                                           endsection=frr_endsection, daemon=bfd_daemon)
+            peerconfig = self.getFRRconfig('bfd', endsection='^exit',
+                                           substring=f' peer {peer}', endsubsection='^ exit')
             if 'profile' in peer_config:
                 self.assertIn(f' profile {peer_config["profile"]}', peerconfig)