Merge pull request #5591 from ruben-herold/T9442-frrender-cache

frrender: T9442: cache the FRR configuration only after it was applied

1 files changed, 34 insertions, 5 deletions

diff --git a/python/vyos/frrender.py b/python/vyos/frrender.py
index 6a8124325..824342773 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,16 +816,21 @@ 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
+
+        # 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
 
-        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']:
@@ -932,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 = {
@@ -950,9 +959,16 @@ 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):
+        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}')
@@ -962,6 +978,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 +991,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 applied one, so that the next commit reloads FRR even
+            # if it returns to the previously applied configuration.
+            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))
@@ -988,3 +1012,8 @@ 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 pending is not None:
+            self.cached_config_dict, self.cached_dhcp_gateways = pending