From b70baa6f8af260529d1db1330da246f309039908 Mon Sep 17 00:00:00 2001 From: Ruben Herold Date: Tue, 20 Oct 2026 14:21:52 +0200 Subject: frrender: T9442: cache the FRR configuration only after it was applied The configuration was stored as the cached one before FRR had reloaded it. If the reload failed, or FRR was down, the next commit found the configuration unchanged, skipped the reload and reported success while FRR kept running the old configuration. Keep the rendered configuration aside and make it the cached one only after the reload succeeded. A render which does not reach the reload is not cached either. If the reload fails, FRR may run a part of the new configuration. The cache is dropped then, so even a return to the previous configuration reloads FRR. A reload that succeeds on the last try no longer raises an error. --- python/vyos/frrender.py | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) (limited to 'python') diff --git a/python/vyos/frrender.py b/python/vyos/frrender.py index 6a8124325..e4b8dc1ab 100644 --- a/python/vyos/frrender.py +++ b/python/vyos/frrender.py @@ -788,10 +788,14 @@ def get_dhcp_route_interfaces(config_dict) -> set: return interfaces class FRRender: + # Configuration of the last successful apply(), see generate() cached_config_dict = {} cached_dhcp_gateways = {} def __init__(self): self._frr_conf = frr_config_file + # Configuration rendered by generate(), which apply() has not + # confirmed yet + self._pending = None def generate(self, config_dict) -> None: """ @@ -812,6 +816,9 @@ class FRRender: for interface in get_dhcp_route_interfaces(config_dict) } + # A render which did not reach apply() must not count as applied + self._pending = None + if ( self.cached_config_dict == config_dict and self.cached_dhcp_gateways == dhcp_gateways @@ -819,9 +826,6 @@ class FRRender: debug('FRR: NO CHANGES DETECTED') return False - self.cached_config_dict = config_dict - self.cached_dhcp_gateways = dhcp_gateways - def inline_helper(config_dict) -> str: output = '!\n' if 'babel' in config_dict and 'deleted' not in config_dict['babel']: @@ -950,6 +954,11 @@ class FRRender: debug(output) write_file(self._frr_conf, output) debug('FRR: RENDERING CONFIG COMPLETE') + + # Only apply() makes this the cached configuration. If FRR does not + # take it, the next commit must render and apply it again instead of + # reporting no changes (T9442). + self._pending = (config_dict, dhcp_gateways) return True def apply(self, count_max=5): @@ -962,6 +971,7 @@ class FRRender: count = 0 emsg = '' + reloaded = False while count < count_max: count += 1 debug(f'FRR: reloading configuration - tries: {count} | Python class ID: {id(self)}') @@ -974,9 +984,16 @@ class FRRender: continue debug(emsg) debug('FRR: configuration reload complete') + reloaded = True break - if count >= count_max: + if not reloaded: + # frr-reload.py applies its changes through vtysh in several + # steps, FRR can be left with a part of the new configuration. + # Forget the cached one, so that the next commit reloads FRR even + # if it returns to the previously applied configuration. + self.cached_config_dict = None + self.cached_dhcp_gateways = {} raise ConfigError(emsg) write_file(frr_applied_config_file, read_file(self._frr_conf)) @@ -988,3 +1005,9 @@ class FRRender: # --daemon. Repeating it asks every daemon for its configuration once # more, and a bgpd busy with a policy walk then holds the commit until # the walk ends. + + # FRR runs the rendered configuration now, remember it for the change + # detection of generate() + if self._pending is not None: + self.cached_config_dict, self.cached_dhcp_gateways = self._pending + self._pending = None -- cgit v1.2.3 From 8338931cf435761c12e923d98b857d124e74ceab Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Sat, 31 Oct 2026 11:10:19 +0100 Subject: frrender: T9442: reload FRR when the rendered configuration was not applied A failed reload only dropped the in-memory cache. The record of the configuration FRR runs still held the previous one, so the DHCP render helper skipped a return to it while FRR kept a part of the failed configuration. After a rejected reload test, the rendered file also kept the rejected configuration. Change detection now also requires the rendered file to match the applied one, and a failed reload removes that record. The next commit then renders and reloads FRR again, even when it returns to the previous configuration. A smoketest stops FRR during a commit and verifies that committing again brings the configuration into FRR. --- python/vyos/frrender.py | 20 +++++++++++------ smoketest/scripts/cli/test_system_frr.py | 38 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-) (limited to 'python') diff --git a/python/vyos/frrender.py b/python/vyos/frrender.py index e4b8dc1ab..824342773 100644 --- a/python/vyos/frrender.py +++ b/python/vyos/frrender.py @@ -819,9 +819,14 @@ class FRRender: # A render which did not reach apply() must not count as applied self._pending = None + # The rendered file can differ from the one FRR runs although the + # configuration is unchanged: frr-reload.py rejected it, or a failed + # reload removed frr_applied_config_file (T9442) if ( self.cached_config_dict == config_dict and self.cached_dhcp_gateways == dhcp_gateways + and read_file(self._frr_conf, None) + == read_file(frr_applied_config_file, None) ): debug('FRR: NO CHANGES DETECTED') return False @@ -936,7 +941,7 @@ class FRRender: if 'protocols' not in vrf_config: continue - # Do not mutate config_dict in-place — it is also stored as + # Do not mutate config_dict in-place - apply() stores it as # cached_config_dict and in-place mutation breaks change # detection on the next commit (T7931). protocols = { @@ -962,6 +967,8 @@ class FRRender: return True def apply(self, count_max=5): + pending, self._pending = self._pending, None + # Do a config reload test cmdline = f'/usr/lib/frr/frr-reload.py --test' rc, emsg = rc_cmd(f'{cmdline} {self._frr_conf}') @@ -990,10 +997,10 @@ class FRRender: if not reloaded: # frr-reload.py applies its changes through vtysh in several # steps, FRR can be left with a part of the new configuration. - # Forget the cached one, so that the next commit reloads FRR even + # Forget the applied one, so that the next commit reloads FRR even # if it returns to the previously applied configuration. - self.cached_config_dict = None - self.cached_dhcp_gateways = {} + if os.path.exists(frr_applied_config_file): + os.unlink(frr_applied_config_file) raise ConfigError(emsg) write_file(frr_applied_config_file, read_file(self._frr_conf)) @@ -1008,6 +1015,5 @@ class FRRender: # FRR runs the rendered configuration now, remember it for the change # detection of generate() - if self._pending is not None: - self.cached_config_dict, self.cached_dhcp_gateways = self._pending - self._pending = None + if pending is not None: + self.cached_config_dict, self.cached_dhcp_gateways = pending diff --git a/smoketest/scripts/cli/test_system_frr.py b/smoketest/scripts/cli/test_system_frr.py index ea280dd27..be3de1989 100755 --- a/smoketest/scripts/cli/test_system_frr.py +++ b/smoketest/scripts/cli/test_system_frr.py @@ -18,7 +18,11 @@ import re import unittest from base_vyostest_shim import VyOSUnitTestSHIM +from vyos.configsession import ConfigSessionError +from vyos.frrender import mgmt_daemon from vyos.utils.file import read_file +from vyos.utils.process import cmdl +from vyos.utils.process import process_named_running from vyos.xml_ref import default_value config_file = '/etc/frr/daemons' @@ -219,6 +223,40 @@ class TestSystemFRR(VyOSUnitTestSHIM.TestCase): f'watchfrr_options="--timeout={default_watchfrr_timeout}"', daemons_config ) + def test_frr_commit_retried_after_failure(self): + # T9442: a configuration FRR did not take must not count as applied, + # committing it again has to reach FRR + static_path = ['protocols', 'static', 'route'] + route_applied = '192.0.2.0/24' + route_failed = '198.51.100.0/24' + + self.cli_set(static_path + [route_applied, 'blackhole']) + self.cli_commit() + + self.cli_set(static_path + [route_failed, 'blackhole']) + cmdl(['systemctl', 'stop', 'frr.service'], sudo=True) + try: + with self.assertRaises(ConfigSessionError): + self.cli_commit() + finally: + cmdl(['systemctl', 'start', 'frr.service'], sudo=True) + # restarted on purpose, see base class tearDown() + self.__class__.mgmt_daemon_pid = process_named_running(mgmt_daemon) + + # FRR starts with the previously applied configuration + frrconfig = self.getFRRconfig() + self.assertIn(f'ip route {route_applied} blackhole', frrconfig) + self.assertNotIn(f'ip route {route_failed} blackhole', frrconfig) + + self.cli_commit() + + frrconfig = self.getFRRconfig() + self.assertIn(f'ip route {route_applied} blackhole', frrconfig) + self.assertIn(f'ip route {route_failed} blackhole', frrconfig) + + self.cli_delete(['protocols', 'static']) + self.cli_commit() + if __name__ == '__main__': unittest.main(verbosity=2, failfast=VyOSUnitTestSHIM.TestCase.debug_on()) -- cgit v1.2.3