Merge pull request #4016 from vyos/mergify/bp/circinus/pr-4015

T6671: defer config dependency if scheduled in priority queue (backport #4015)

7 files changed, 180 insertions, 16 deletions

diff --git a/python/vyos/configdep.py b/python/vyos/configdep.py
index e0fe1ddac..cf7c9d543 100644
--- a/python/vyos/configdep.py
+++ b/python/vyos/configdep.py
@@ -95,7 +95,8 @@ def get_dependency_dict(config: 'Config') -> dict:
         setattr(config, 'cached_dependency_dict', d)
     return d
 
-def run_config_mode_script(script: str, config: 'Config'):
+def run_config_mode_script(target: str, config: 'Config'):
+    script = target + '.py'
     path = os.path.join(directories['conf_mode'], script)
     name = canon_name(script)
     mod = load_as_module(name, path)
@@ -109,15 +110,34 @@ def run_config_mode_script(script: str, config: 'Config'):
     except (VyOSError, ConfigError) as e:
         raise ConfigError(str(e)) from e
 
+def run_conditionally(target: str, tagnode: str, config: 'Config'):
+    tag_ext = f'_{tagnode}' if tagnode else ''
+    script_name = f'{target}{tag_ext}'
+
+    scripts_called = getattr(config, 'scripts_called', [])
+    commit_scripts = getattr(config, 'commit_scripts', [])
+
+    debug_print(f'scripts_called: {scripts_called}')
+    debug_print(f'commit_scripts: {commit_scripts}')
+
+    if script_name in commit_scripts and script_name not in scripts_called:
+        debug_print(f'dependency {script_name} deferred to priority')
+        return
+
+    run_config_mode_script(target, config)
+
 def def_closure(target: str, config: 'Config',
                 tagnode: typing.Optional[str] = None) -> typing.Callable:
-    script = target + '.py'
     def func_impl():
+        tag_value = ''
         if tagnode is not None:
             os.environ['VYOS_TAGNODE_VALUE'] = tagnode
-        run_config_mode_script(script, config)
+            tag_value = tagnode
+        run_conditionally(target, tag_value, config)
+
     tag_ext = f'_{tagnode}' if tagnode is not None else ''
     func_impl.__name__ = f'{target}{tag_ext}'
+
     return func_impl
 
 def set_dependents(case: str, config: 'Config',
diff --git a/python/vyos/configdiff.py b/python/vyos/configdiff.py
index f975df45d..b6d4a5558 100644
--- a/python/vyos/configdiff.py
+++ b/python/vyos/configdiff.py
@@ -15,6 +15,7 @@
 
 from enum import IntFlag
 from enum import auto
+from itertools import chain
 
 from vyos.config import Config
 from vyos.configtree import DiffTree
@@ -22,7 +23,10 @@ from vyos.configdict import dict_merge
 from vyos.utils.dict import get_sub_dict
 from vyos.utils.dict import mangle_dict_keys
 from vyos.utils.dict import dict_search_args
+from vyos.utils.dict import dict_to_key_paths
 from vyos.xml_ref import get_defaults
+from vyos.xml_ref import owner
+from vyos.xml_ref import priority
 
 class ConfigDiffError(Exception):
     """
@@ -94,6 +98,39 @@ def get_config_diff(config, key_mangling=None):
     return ConfigDiff(config, key_mangling, diff_tree=diff_t,
                                             diff_dict=diff_d)
 
+def get_commit_scripts(config) -> list:
+    """Return the list of config scripts to be executed by commit
+
+    Return a list of the scripts to be called by commit for the proposed
+    config. The list is ordered by priority for reference, however, the
+    actual order of execution by the commit algorithm is not reflected
+    (delete vs. add queue), nor needed for current use.
+    """
+    if not config or not isinstance(config, Config):
+        raise TypeError("argument must me a Config instance")
+
+    if hasattr(config, 'commit_scripts'):
+        return getattr(config, 'commit_scripts')
+
+    D = get_config_diff(config)
+    d = D._diff_dict
+    s = set()
+    for p in chain(dict_to_key_paths(d['sub']), dict_to_key_paths(d['add'])):
+        p_owner = owner(p, with_tag=True)
+        if not p_owner:
+            continue
+        p_priority = priority(p)
+        if not p_priority:
+            # default priority in legacy commit-algorithm
+            p_priority = 0
+        p_priority = int(p_priority)
+        s.add((p_priority, p_owner))
+
+    res = [x[1] for x in sorted(s, key=lambda x: x[0])]
+    setattr(config, 'commit_scripts', res)
+
+    return res
+
 class ConfigDiff(object):
     """
     The class of config changes as represented by comparison between the
diff --git a/python/vyos/utils/dict.py b/python/vyos/utils/dict.py
index 062ab9c81..9a4671c5f 100644
--- a/python/vyos/utils/dict.py
+++ b/python/vyos/utils/dict.py
@@ -267,6 +267,7 @@ def dict_to_paths_values(conf: dict) -> dict:
         dict_of_options[path] = dict_search(path,conf)
 
     return dict_of_options
+
 def dict_to_key_paths(d: dict) -> list:
     """ Generator to return list of key paths from dict of list[str]|str
     """
diff --git a/python/vyos/xml_ref/__init__.py b/python/vyos/xml_ref/__init__.py
index 2ba3da4e8..7e9bf2b43 100644
--- a/python/vyos/xml_ref/__init__.py
+++ b/python/vyos/xml_ref/__init__.py
@@ -1,4 +1,4 @@
-# Copyright 2023 VyOS maintainers and contributors <maintainers@vyos.io>
+# Copyright 2024 VyOS maintainers and contributors <maintainers@vyos.io>
 #
 # This library is free software; you can redistribute it and/or
 # modify it under the terms of the GNU Lesser General Public
@@ -53,8 +53,8 @@ def is_valueless(path: list) -> bool:
 def is_leaf(path: list) -> bool:
     return load_reference().is_leaf(path)
 
-def owner(path: list) -> str:
-    return load_reference().owner(path)
+def owner(path: list, with_tag=False) -> str:
+    return load_reference().owner(path, with_tag=with_tag)
 
 def priority(path: list) -> str:
     return load_reference().priority(path)
diff --git a/python/vyos/xml_ref/definition.py b/python/vyos/xml_ref/definition.py
index c85835ffd..5ff28daed 100644
--- a/python/vyos/xml_ref/definition.py
+++ b/python/vyos/xml_ref/definition.py
@@ -1,4 +1,4 @@
-# Copyright 2023 VyOS maintainers and contributors <maintainers@vyos.io>
+# Copyright 2024 VyOS maintainers and contributors <maintainers@vyos.io>
 #
 # This library is free software; you can redistribute it and/or
 # modify it under the terms of the GNU Lesser General Public
@@ -139,28 +139,38 @@ class Xml:
         ref_path = path.copy()
         d = self.ref
         data = ''
+        tag = ''
         while ref_path and d:
+            tag_val = ''
             d = d.get(ref_path[0], {})
             ref_path.pop(0)
             if self._is_tag_node(d) and ref_path:
+                tag_val = ref_path[0]
                 ref_path.pop(0)
             if self._is_leaf_node(d) and ref_path:
                 ref_path.pop(0)
             res = self._get_ref_node_data(d, name)
             if res is not None:
                 data = res
+                tag = tag_val
 
-        return data
+        return data, tag
 
-    def owner(self, path: list) -> str:
+    def owner(self, path: list, with_tag=False) -> str:
         from pathlib import Path
-        data = self._least_upper_data(path, 'owner')
+        data, tag = self._least_upper_data(path, 'owner')
+        tag_ext = f'_{tag}' if tag else ''
         if data:
-            data = Path(data.split()[0]).name
+            if with_tag:
+                data = Path(data.split()[0]).stem
+                data = f'{data}{tag_ext}'
+            else:
+                data = Path(data.split()[0]).name
         return data
 
     def priority(self, path: list) -> str:
-        return self._least_upper_data(path, 'priority')
+        data, _ = self._least_upper_data(path, 'priority')
+        return data
 
     @staticmethod
     def _dict_get(d: dict, path: list) -> dict:
diff --git a/smoketest/scripts/cli/test_config_dependency.py b/smoketest/scripts/cli/test_config_dependency.py
index 14e88321a..99e807ac5 100755
--- a/smoketest/scripts/cli/test_config_dependency.py
+++ b/smoketest/scripts/cli/test_config_dependency.py
@@ -16,13 +16,39 @@
 
 
 import unittest
+from time import sleep
 
-from base_vyostest_shim import VyOSUnitTestSHIM
-
+from vyos.utils.process import is_systemd_service_running
+from vyos.utils.process import cmd
 from vyos.configsession import ConfigSessionError
 
+from base_vyostest_shim import VyOSUnitTestSHIM
+
 
 class TestConfigDep(VyOSUnitTestSHIM.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        # smoketests are run without configd in 1.4; with configd in 1.5
+        # the tests below check behavior under configd:
+        # test_configdep_error checks for regression under configd (T6559)
+        # test_configdep_prio_queue checks resolution under configd (T6671)
+        cls.running_state = is_systemd_service_running('vyos-configd.service')
+
+        if not cls.running_state:
+            cmd('sudo systemctl start vyos-configd.service')
+            # allow time for init
+            sleep(1)
+
+        super(TestConfigDep, cls).setUpClass()
+
+    @classmethod
+    def tearDownClass(cls):
+        super(TestConfigDep, cls).tearDownClass()
+
+        # return to running_state
+        if not cls.running_state:
+            cmd('sudo systemctl stop vyos-configd.service')
+
     def test_configdep_error(self):
         address_group = 'AG'
         address = '192.168.137.5'
@@ -45,5 +71,60 @@ class TestConfigDep(VyOSUnitTestSHIM.TestCase):
         self.cli_delete(['nat'])
         self.cli_commit()
 
+    def test_configdep_prio_queue(self):
+        # confirm that that a dependency (in this case, conntrack ->
+        # conntrack-sync) is not immediately called if the target is
+        # scheduled in the priority queue, indicating that it may require an
+        # intermediate activitation (bond0)
+        bonding_base = ['interfaces', 'bonding']
+        bond_interface = 'bond0'
+        bond_address = '192.0.2.1/24'
+        vrrp_group_base = ['high-availability', 'vrrp', 'group']
+        vrrp_sync_group_base = ['high-availability', 'vrrp', 'sync-group']
+        vrrp_group = 'ETH2'
+        vrrp_sync_group = 'GROUP'
+        conntrack_sync_base = ['service', 'conntrack-sync']
+        conntrack_peer = '192.0.2.77'
+
+        # simple set to trigger in-session conntrack -> conntrack-sync
+        # dependency; note that this is triggered on boot in 1.4 due to
+        # default 'system conntrack modules'
+        self.cli_set(['system', 'conntrack', 'table-size', '524288'])
+
+        self.cli_set(['interfaces', 'ethernet', 'eth2', 'address',
+                      '198.51.100.2/24'])
+
+        self.cli_set(bonding_base + [bond_interface, 'address',
+                                     bond_address])
+        self.cli_set(bonding_base + [bond_interface, 'member', 'interface',
+                                     'eth3'])
+
+        self.cli_set(vrrp_group_base + [vrrp_group, 'address',
+                                        '198.51.100.200/24'])
+        self.cli_set(vrrp_group_base + [vrrp_group, 'hello-source-address',
+                                        '198.51.100.2'])
+        self.cli_set(vrrp_group_base + [vrrp_group, 'interface', 'eth2'])
+        self.cli_set(vrrp_group_base + [vrrp_group, 'priority', '200'])
+        self.cli_set(vrrp_group_base + [vrrp_group, 'vrid', '22'])
+        self.cli_set(vrrp_sync_group_base + [vrrp_sync_group, 'member',
+                                             vrrp_group])
+
+        self.cli_set(conntrack_sync_base + ['failover-mechanism', 'vrrp',
+                                            'sync-group', vrrp_sync_group])
+
+        self.cli_set(conntrack_sync_base + ['interface', bond_interface,
+                                            'peer', conntrack_peer])
+
+        self.cli_commit()
+
+        # clean up
+        self.cli_delete(bonding_base)
+        self.cli_delete(vrrp_group_base)
+        self.cli_delete(vrrp_sync_group_base)
+        self.cli_delete(conntrack_sync_base)
+        self.cli_delete(['interfaces', 'ethernet', 'eth2', 'address'])
+        self.cli_delete(['system', 'conntrack', 'table-size'])
+        self.cli_commit()
+
 if __name__ == '__main__':
     unittest.main(verbosity=2)
diff --git a/src/services/vyos-configd b/src/services/vyos-configd
index d797e90cf..3674d9627 100755
--- a/src/services/vyos-configd
+++ b/src/services/vyos-configd
@@ -30,6 +30,7 @@ from vyos.defaults import directories
 from vyos.utils.boot import boot_configuration_complete
 from vyos.configsource import ConfigSourceString
 from vyos.configsource import ConfigSourceError
+from vyos.configdiff import get_commit_scripts
 from vyos.config import Config
 from vyos import ConfigError
 
@@ -220,6 +221,12 @@ def initialization(socket):
     dependent_func: dict[str, list[typing.Callable]] = {}
     setattr(config, 'dependent_func', dependent_func)
 
+    commit_scripts = get_commit_scripts(config)
+    logger.debug(f'commit_scripts: {commit_scripts}')
+
+    scripts_called = []
+    setattr(config, 'scripts_called', scripts_called)
+
     return config
 
 def process_node_data(config, data, last: bool = False) -> int:
@@ -228,6 +235,7 @@ def process_node_data(config, data, last: bool = False) -> int:
         return R_ERROR_DAEMON
 
     script_name = None
+    os.environ['VYOS_TAGNODE_VALUE'] = ''
     args = []
     config.dependency_list.clear()
 
@@ -244,6 +252,12 @@ def process_node_data(config, data, last: bool = False) -> int:
         args = res.group(3).split()
     args.insert(0, f'{script_name}.py')
 
+    tag_value = os.getenv('VYOS_TAGNODE_VALUE', '')
+    tag_ext = f'_{tag_value}' if tag_value else ''
+    script_record = f'{script_name}{tag_ext}'
+    scripts_called = getattr(config, 'scripts_called', [])
+    scripts_called.append(script_record)
+
     if script_name not in include_set:
         return R_PASS
 
@@ -302,11 +316,12 @@ if __name__ == '__main__':
             socket.send(resp.encode())
             config = initialization(socket)
         elif message["type"] == "node":
-            if message["last"]:
-                logger.debug(f'final element of priority queue')
             res = process_node_data(config, message["data"], message["last"])
             response = res.to_bytes(1, byteorder=sys.byteorder)
             logger.debug(f"Sending response {res}")
             socket.send(response)
+            if message["last"] and config:
+                scripts_called = getattr(config, 'scripts_called', [])
+                logger.debug(f'scripts_called: {scripts_called}')
         else:
             logger.critical(f"Unexpected message: {message}")