Merge pull request #5520 from c-po/T9278-frr-render-without-configd

configd: T9368: reconcile FRR after a DHCP lease without the commit socket

8 files changed, 142 insertions, 50 deletions

diff --git a/src/etc/dhcp/dhclient-enter-hooks.d/98-vyos-static-routes-dhclient-hook b/src/etc/dhcp/dhclient-enter-hooks.d/98-vyos-static-routes-dhclient-hook
index 439a8dd87..f3b1b35f1 100755
--- a/src/etc/dhcp/dhclient-enter-hooks.d/98-vyos-static-routes-dhclient-hook
+++ b/src/etc/dhcp/dhclient-enter-hooks.d/98-vyos-static-routes-dhclient-hook
@@ -28,6 +28,7 @@ fi
 # - RELEASE: lease released, remove routes
 # - STOP: dhclient stopped, remove routes
 if [ "$reason" == "PREINIT" ] || [ "$reason" == "EXPIRE" ] || [ "$reason" == "FAIL" ] || [ "$reason" == "RELEASE" ] || [ "$reason" == "STOP" ]; then
-    # Re-generate static routes config to remove routes that depend on this interface
-    sudo /usr/libexec/vyos/vyos-request-configd-update.py
+    # Reconcile the rendered FRR configuration - routes depending on this
+    # interface must be removed
+    sudo /usr/libexec/vyos/vyos-frr-render.py
 fi
diff --git a/src/etc/dhcp/dhclient-exit-hooks.d/98-vyos-static-routes-dhclient-hook b/src/etc/dhcp/dhclient-exit-hooks.d/98-vyos-static-routes-dhclient-hook
index 4f207411e..2c895ffd6 100755
--- a/src/etc/dhcp/dhclient-exit-hooks.d/98-vyos-static-routes-dhclient-hook
+++ b/src/etc/dhcp/dhclient-exit-hooks.d/98-vyos-static-routes-dhclient-hook
@@ -34,5 +34,5 @@ elif [ "$reason" != "BOUND" ] && [ "$reason" != "EXPIRE" ] && [ "$reason" != "RE
     return 0
 fi
 
-# Re-generate the static routes config
-sudo /usr/libexec/vyos/vyos-request-configd-update.py
+# Reconcile the rendered FRR configuration with the current lease
+sudo /usr/libexec/vyos/vyos-frr-render.py
diff --git a/src/etc/ppp/ip-up.d/99-vyos-pppoe-callback b/src/etc/ppp/ip-up.d/99-vyos-pppoe-callback
index f3efb9ac0..9decfcbdd 100755
--- a/src/etc/ppp/ip-up.d/99-vyos-pppoe-callback
+++ b/src/etc/ppp/ip-up.d/99-vyos-pppoe-callback
@@ -26,6 +26,7 @@ from sys import exit
 from vyos.configquery import ConfigTreeQuery
 from vyos.configdict import get_interface_dict
 from vyos.ifconfig import PPPoEIf
+from vyos.utils.commit import wait_for_commit_lock
 
 # When the ppp link comes up, this script is called with the following
 # parameters
@@ -41,6 +42,12 @@ if (len(argv) < 7):
 
 interface = argv[6]
 
+# The session is re-established from within a commit and can come back up
+# before that commit finished. The configuration read below is the running
+# one, which until then still is the configuration from before the commit -
+# applying it would undo what is being committed right now
+wait_for_commit_lock()
+
 conf = ConfigTreeQuery()
 _, pppoe = get_interface_dict(conf.config, ['interfaces', 'pppoe'], interface)
 
diff --git a/src/helpers/vyos-frr-render.py b/src/helpers/vyos-frr-render.py
new file mode 100755
index 000000000..739b56410
--- /dev/null
+++ b/src/helpers/vyos-frr-render.py
@@ -0,0 +1,58 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) VyOS Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 2 or later as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Render the FRR configuration and reload FRR without vyos-configd.
+
+Only vyos-configd renders FRR, and only at the end of a commit it handled
+itself. Two paths need a render without it: a DHCP lease event, which changes
+the rendered configuration without any CLI change, and a commit which fell back
+to running the conf-mode scripts directly."""
+
+import sys
+
+from vyos.config import Config
+from vyos.frrender import FRRender
+from vyos.frrender import frr_applied_config_file
+from vyos.frrender import frr_config_file
+from vyos.frrender import frr_render_lock
+from vyos.frrender import get_frrender_dict
+from vyos.utils.file import read_file
+from vyos import ConfigError
+
+
+def render() -> None:
+    """Reload FRR if the rendered configuration differs from the one FRR was
+    last reloaded with.
+
+    A fresh instance has no cached configuration, thus its own change detection
+    can not be used here. The rendered file is no substitute for it - a render
+    which failed before the reload leaves it unapplied."""
+    frr = FRRender()
+    frr.generate(get_frrender_dict(Config()))
+
+    if read_file(frr_config_file) == read_file(frr_applied_config_file, None):
+        return
+
+    frr.apply()
+
+
+if __name__ == '__main__':
+    try:
+        with frr_render_lock():
+            render()
+    except ConfigError as e:
+        print(e)
+        sys.exit(1)
diff --git a/src/helpers/vyos-request-configd-update.py b/src/helpers/vyos-request-configd-update.py
deleted file mode 100755
index 55ed0eef9..000000000
--- a/src/helpers/vyos-request-configd-update.py
+++ /dev/null
@@ -1,31 +0,0 @@
-#!/usr/bin/env python3
-
-import json
-import zmq
-
-from vyos.utils.commit import wait_for_commit_lock
-from vyos.defaults import vyos_configd_socket_path
-
-context = zmq.Context()
-
-request = {
-    'type': 'node',
-    'last': True,
-    'data': '/usr/libexec/vyos/conf_mode/protocols_static.py',
-}
-request = json.dumps(request)
-
-print("Waiting for commit lock...")
-wait_for_commit_lock()
-
-print("Connecting to vyos-configd server...")
-socket = context.socket(zmq.REQ)
-socket.connect(vyos_configd_socket_path)
-
-print(f"Sending request {request}...")
-socket.send_string(request)
-
-message = socket.recv()
-print(f"Received reply {request} [ {message} ]")
-
-print("All done")
diff --git a/src/services/vyos-commitd b/src/services/vyos-commitd
index b8e430b93..8a515446b 100755
--- a/src/services/vyos-commitd
+++ b/src/services/vyos-commitd
@@ -45,6 +45,7 @@ from vyos.configsource import ConfigSourceError
 from vyos.configdiff import get_commit_scripts
 from vyos.config import Config
 from vyos.frrender import FRRender
+from vyos.frrender import frr_render_lock
 from vyos.frrender import get_frrender_dict
 from vyos import ConfigError
 
@@ -293,11 +294,12 @@ def call_frr_render(frr, config):
     def _call_frr_render(frr, config):
         # pylint: disable=broad-exception-caught
         try:
-            tmp = get_frrender_dict(config)
-            if frr.generate(tmp):
-                # only apply a new FRR configuration if anything changed
-                # in comparison to the previous applied configuration
-                frr.apply()
+            with frr_render_lock():
+                tmp = get_frrender_dict(config)
+                if frr.generate(tmp):
+                    # only apply a new FRR configuration if anything changed
+                    # in comparison to the previous applied configuration
+                    frr.apply()
 
         except ConfigError as e:
             logger.error(e)
diff --git a/src/services/vyos-configd b/src/services/vyos-configd
index 32d773f78..8e0b1adec 100755
--- a/src/services/vyos-configd
+++ b/src/services/vyos-configd
@@ -39,6 +39,7 @@ from vyos.configsource import ConfigSourceError
 from vyos.configdiff import get_commit_scripts
 from vyos.config import Config
 from vyos.frrender import FRRender
+from vyos.frrender import frr_render_lock
 from vyos.frrender import get_frrender_dict
 from vyos import ConfigError
 from vyos.configmanager import ConfigManager
@@ -250,11 +251,12 @@ def call_frr_render(frr, config):
     def _call_frr_render(frr, config):
         # pylint: disable=broad-exception-caught
         try:
-            tmp = get_frrender_dict(config)
-            if frr.generate(tmp):
-                # only apply a new FRR configuration if anything changed
-                # in comparison to the previous applied configuration
-                frr.apply()
+            with frr_render_lock():
+                tmp = get_frrender_dict(config)
+                if frr.generate(tmp):
+                    # only apply a new FRR configuration if anything changed
+                    # in comparison to the previous applied configuration
+                    frr.apply()
 
         except ConfigError as e:
             logger.error(e)
diff --git a/src/shim/vyshim.c b/src/shim/vyshim.c
index 7b516cd46..81a1f8b49 100644
--- a/src/shim/vyshim.c
+++ b/src/shim/vyshim.c
@@ -53,6 +53,8 @@
 #define COMMIT_MARKER "/var/tmp/initial_in_commit"
 #define QUEUE_MARKER "/var/tmp/last_in_queue"
 
+#define FRR_RENDER "/usr/libexec/vyos/vyos-frr-render.py"
+
 enum {
     SUCCESS =      1 << 0,
     ERROR_COMMIT = 1 << 1,
@@ -66,6 +68,7 @@ volatile int timeout = 0;
 
 int initialization(void *, char *);
 int pass_through(char **, int);
+int frr_render(char *);
 void timer_handler(int);
 void leave_hint(char *);
 
@@ -107,6 +110,13 @@ int main(int argc, char* argv[])
     strsep(&pid_str, "_");
     debug_print("config session pid: %s\n", pid_str);
 
+    // Consume the marker before talking to the daemon, else it is left behind
+    // for the next commit when the daemon can not be used here
+    if (access(QUEUE_MARKER, F_OK) != -1) {
+        last = 1;
+        remove(QUEUE_MARKER);
+    }
+
     if (access(COMMIT_MARKER, F_OK) != -1) {
         init_timeout = initialization(requester, pid_str);
         if (!init_timeout) remove(COMMIT_MARKER);
@@ -115,14 +125,12 @@ int main(int argc, char* argv[])
     // if initial communication failed, pass through execution of script
     if (init_timeout) {
         int ret = pass_through(argv, ex_index);
+        // The conf-mode scripts do not render FRR, only the daemon does -
+        // without this the FRR reload of this commit is silently skipped
+        if (last && frr_render(pid_str) != 0) ret = -1;
         return ret;
     }
 
-    if (access(QUEUE_MARKER, F_OK) != -1) {
-        last = 1;
-        remove(QUEUE_MARKER);
-    }
-
     char error_code[1];
     debug_print("Sending node data ...\n");
     char *string_node_data_msg = mkjson(MKJSON_OBJ, 3,
@@ -159,6 +167,11 @@ int main(int argc, char* argv[])
         ret = pass_through(argv, ex_index);
     }
 
+    // The daemon renders FRR only for a last script it handled itself
+    if (last && (err & (PASS | ERROR_DAEMON))) {
+        if (frr_render(pid_str) != 0) ret = -1;
+    }
+
     if (err & ERROR_COMMIT) {
         debug_print("Received ERROR_COMMIT\n");
         ret = -1;
@@ -344,6 +357,46 @@ int pass_through(char **argv, int ex_index)
     return 0;
 }
 
+int frr_render(char *pid_val)
+{
+    pid_t child_pid;
+    int status;
+
+    // Nothing to fall back to, e.g. during a partial package upgrade
+    if (access(FRR_RENDER, X_OK) == -1) {
+        debug_print("%s is not available\n", FRR_RENDER);
+        return 0;
+    }
+
+    debug_print("rendering FRR configuration without the daemon\n");
+
+    if ((child_pid=fork()) < 0) {
+        debug_print("frr_render fork() failed\n");
+        return -1;
+    } else if (child_pid == 0) {
+        char *newargv[] = { FRR_RENDER, NULL };
+        if (-1 == execv(FRR_RENDER, newargv)) {
+            fprintf(stderr, "frr_render execv failed %s: %s\n",
+                    FRR_RENDER, strerror(errno));
+            exit(EXIT_FAILURE);
+        }
+    }
+
+    if (waitpid(child_pid, &status, 0) != child_pid) {
+        debug_print("frr_render waitpid() failed\n");
+        return -1;
+    }
+
+    if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
+        return 0;
+    }
+
+    // Same treatment as an apply error reported by the daemon
+    leave_hint(pid_val);
+
+    return -1;
+}
+
 void timer_handler(int signum)
 {
     debug_print("timer_handler invoked\n");