lib: add support for confirmed commits

Confirmed commits allow the user to request an automatic rollback to
the previous configuration if the commit operation is not confirmed
within a number of minutes. This is particularly useful when the user
is accessing the CLI through the network (e.g. using SSH) and any
configuration change might cause an unexpected loss of connectivity
between the user and the managed device (e.g. misconfiguration of a
routing protocol). By using a confirmed commit, the user can rest
assured the connectivity will be restored after the given timeout
expires, avoiding the need to access the router physically to fix
the problem.

When "commit confirmed TIMEOUT" is used, a new "commit" command is
expected to confirm the previous commit before the given timeout
expires. If "commit confirmed TIMEOUT" is used while there's already
a confirmed-commit in progress, the confirmed-commit timeout is
reset to the new value.

In the current implementation, if other users perform commits while
there's a confirmed-commit in progress, all commits are rolled back
when the confirmed-commit timeout expires. It's recommended to use
the "configure exclusive" configuration mode to prevent unexpected
outcomes when using confirmed commits.

When an user exits from the configuration mode while there's a
confirmed-commit in progress, the commit is automatically rolled
back and the user is notified about it. In the future we might
want to prompt the user if he or she really wants to exit from the
configuration mode when there's a pending confirmed commit.

Needless to say, confirmed commit only work for configuration
commands converted to the new northbound model. vtysh support will
be implemented at a later time.

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>

diff --git a/lib/libfrr.c b/lib/libfrr.c
index 6ebe24eef7940b550f653141f6f66da27a629e47..9119b04992aabb62f5c96121686c6576da61d212 100644 (file)
--- a/lib/libfrr.c
+++ b/lib/libfrr.c
@@ -653,7 +653,7 @@ struct thread_master *frr_init(void)
        lib_error_init();
 
        yang_init();
-       nb_init(di->yang_modules, di->n_yang_modules);
+       nb_init(master, di->yang_modules, di->n_yang_modules);
 
        return master;
 }

diff --git a/lib/northbound.c b/lib/northbound.c
index 490b3abe576ea00467914d667c9944c49675d8c5..8503f87d608c5a32783c419e8d9fe8f0aa9a0f42 100644 (file)
--- a/lib/northbound.c
+++ b/lib/northbound.c
@@ -1539,7 +1539,8 @@ static void nb_load_callbacks(const struct frr_yang_module_info *module)
        }
 }
 
-void nb_init(const struct frr_yang_module_info *modules[], size_t nmodules)
+void nb_init(struct thread_master *tm,
+            const struct frr_yang_module_info *modules[], size_t nmodules)
 {
        unsigned int errors = 0;
 
@@ -1574,7 +1575,7 @@ void nb_init(const struct frr_yang_module_info *modules[], size_t nmodules)
        running_config = nb_config_new(NULL);
 
        /* Initialize the northbound CLI. */
-       nb_cli_init();
+       nb_cli_init(tm);
 }
 
 void nb_terminate(void)

diff --git a/lib/northbound.h b/lib/northbound.h
index e26a2f861723e1a3b6d9899d508678012136866f..c8e8d7570171e90babe151df258def7c568061a4 100644 (file)
--- a/lib/northbound.h
+++ b/lib/northbound.h
@@ -20,6 +20,7 @@
 #ifndef _FRR_NORTHBOUND_H_
 #define _FRR_NORTHBOUND_H_
 
+#include "thread.h"
 #include "hook.h"
 #include "linklist.h"
 #include "openbsd-tree.h"
@@ -825,7 +826,7 @@ extern const char *nb_client_name(enum nb_client client);
  * nmodules
  *    Size of the modules array.
  */
-extern void nb_init(const struct frr_yang_module_info *modules[],
+extern void nb_init(struct thread_master *tm, const struct frr_yang_module_info *modules[],
                    size_t nmodules);
 
 /*

diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c
index 2cacc6b1dc79178d82fba3b883c6ad09d2658820..4a4adb423f1e4098c6b884c80fa105f11f411eba 100644 (file)
--- a/lib/northbound_cli.c
+++ b/lib/northbound_cli.c
@@ -36,6 +36,7 @@
 
 int debug_northbound;
 struct nb_config *vty_shared_candidate_config;
+static struct thread_master *master;
 
 static void vty_show_libyang_errors(struct vty *vty, struct ly_ctx *ly_ctx)
 {
@@ -213,16 +214,80 @@ int nb_cli_rpc(const char *xpath, struct list *input, struct list *output)
        }
 }
 
-static int nb_cli_commit(struct vty *vty, bool force, char *comment)
+void nb_cli_confirmed_commit_clean(struct vty *vty)
+{
+       THREAD_TIMER_OFF(vty->t_confirmed_commit_timeout);
+       nb_config_free(vty->confirmed_commit_rollback);
+       vty->confirmed_commit_rollback = NULL;
+}
+
+int nb_cli_confirmed_commit_rollback(struct vty *vty)
 {
        uint32_t transaction_id;
        int ret;
 
+       /* Perform the rollback. */
+       ret = nb_candidate_commit(
+               vty->confirmed_commit_rollback, NB_CLIENT_CLI, true,
+               "Rollback to previous configuration - confirmed commit has timed out",
+               &transaction_id);
+       if (ret == NB_OK)
+               vty_out(vty,
+                       "Rollback performed successfully (Transaction ID #%u).\n",
+                       transaction_id);
+       else
+               vty_out(vty, "Failed to rollback to previous configuration.\n");
+
+       return ret;
+}
+
+static int nb_cli_confirmed_commit_timeout(struct thread *thread)
+{
+       struct vty *vty = THREAD_ARG(thread);
+
+       /* XXX: broadcast this message to all logged-in users? */
+       vty_out(vty,
+               "\nConfirmed commit has timed out, rolling back to previous configuration.\n\n");
+
+       nb_cli_confirmed_commit_rollback(vty);
+       nb_cli_confirmed_commit_clean(vty);
+
+       return 0;
+}
+
+static int nb_cli_commit(struct vty *vty, bool force,
+                        unsigned int confirmed_timeout, char *comment)
+{
+       uint32_t transaction_id;
+       int ret;
+
+       /* Check if there's a pending confirmed commit. */
+       if (vty->t_confirmed_commit_timeout) {
+               if (confirmed_timeout) {
+                       /* Reset timeout if "commit confirmed" is used again. */
+                       vty_out(vty,
+                               "%% Resetting confirmed-commit timeout to %u minute(s)\n\n",
+                               confirmed_timeout);
+
+                       THREAD_TIMER_OFF(vty->t_confirmed_commit_timeout);
+                       thread_add_timer(master,
+                                        nb_cli_confirmed_commit_timeout, vty,
+                                        confirmed_timeout * 60,
+                                        &vty->t_confirmed_commit_timeout);
+               } else {
+                       /* Accept commit confirmation. */
+                       vty_out(vty, "%% Commit complete.\n\n");
+                       nb_cli_confirmed_commit_clean(vty);
+               }
+               return CMD_SUCCESS;
+       }
+
        if (vty_exclusive_lock != NULL && vty_exclusive_lock != vty) {
                vty_out(vty, "%% Configuration is locked by another VTY.\n\n");
                return CMD_WARNING;
        }
 
+       /* "force" parameter. */
        if (!force && nb_candidate_needs_update(vty->candidate_config)) {
                vty_out(vty,
                        "%% Candidate configuration needs to be updated before commit.\n\n");
@@ -231,6 +296,16 @@ static int nb_cli_commit(struct vty *vty, bool force, char *comment)
                return CMD_WARNING;
        }
 
+       /* "confirm" parameter. */
+       if (confirmed_timeout) {
+               vty->confirmed_commit_rollback = nb_config_dup(running_config);
+
+               vty->t_confirmed_commit_timeout = NULL;
+               thread_add_timer(master, nb_cli_confirmed_commit_timeout, vty,
+                                confirmed_timeout * 60,
+                                &vty->t_confirmed_commit_timeout);
+       }
+
        ret = nb_candidate_commit(vty->candidate_config, NB_CLIENT_CLI, true,
                                  comment, &transaction_id);
 
@@ -534,18 +609,22 @@ DEFUN (config_private,
 
 DEFPY (config_commit,
        config_commit_cmd,
-       "commit [force$force]",
+       "commit [{force$force|confirmed (1-60)}]",
        "Commit changes into the running configuration\n"
-       "Force commit even if the candidate is outdated\n")
+       "Force commit even if the candidate is outdated\n"
+       "Rollback this commit unless there is a confirming commit\n"
+       "Timeout in minutes for the commit to be confirmed\n")
 {
-       return nb_cli_commit(vty, !!force, NULL);
+       return nb_cli_commit(vty, !!force, confirmed, NULL);
 }
 
 DEFPY (config_commit_comment,
        config_commit_comment_cmd,
-       "commit [force$force] comment LINE...",
+       "commit [{force$force|confirmed (1-60)}] comment LINE...",
        "Commit changes into the running configuration\n"
        "Force commit even if the candidate is outdated\n"
+       "Rollback this commit unless there is a confirming commit\n"
+       "Timeout in minutes for the commit to be confirmed\n"
        "Assign a comment to this commit\n"
        "Comment for this commit (Max 80 characters)\n")
 {
@@ -555,7 +634,7 @@ DEFPY (config_commit_comment,
 
        argv_find(argv, argc, "LINE", &idx);
        comment = argv_concat(argv, argc, idx);
-       ret = nb_cli_commit(vty, !!force, comment);
+       ret = nb_cli_commit(vty, !!force, confirmed, comment);
        XFREE(MTYPE_TMP, comment);
 
        return ret;
@@ -1517,8 +1596,10 @@ static const struct cmd_variable_handler yang_var_handlers[] = {
         .completions = yang_translator_autocomplete},
        {.completions = NULL}};
 
-void nb_cli_init(void)
+void nb_cli_init(struct thread_master *tm)
 {
+       master = tm;
+
        /* Initialize the shared candidate configuration. */
        vty_shared_candidate_config = nb_config_new(NULL);
 

diff --git a/lib/northbound_cli.h b/lib/northbound_cli.h
index febcbd86f17baa40a9e711910b99764ffa0a46c3..362a4bc32528c262d01aaaa8c0a60b74c9cc11b4 100644 (file)
--- a/lib/northbound_cli.h
+++ b/lib/northbound_cli.h
@@ -105,8 +105,10 @@ extern void nb_cli_show_dnode_cmds(struct vty *vty, struct lyd_node *dnode,
                                   bool show_defaults);
 
 /* Prototypes of internal functions. */
+extern void nb_cli_confirmed_commit_clean(struct vty *vty);
+extern int nb_cli_confirmed_commit_rollback(struct vty *vty);
 extern void nb_cli_install_default(int node);
-extern void nb_cli_init(void);
+extern void nb_cli_init(struct thread_master *tm);
 extern void nb_cli_terminate(void);
 
 #endif /* _FRR_NORTHBOUND_CLI_H_ */

diff --git a/lib/vty.c b/lib/vty.c
index 9908ada7f04607eab8cf576f64222fafcda976e1..085cbac742a004082a1594870d828994c671e815 100644 (file)
--- a/lib/vty.c
+++ b/lib/vty.c
@@ -2714,6 +2714,14 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive)
 
 void vty_config_exit(struct vty *vty)
 {
+       /* Check if there's a pending confirmed commit. */
+       if (vty->t_confirmed_commit_timeout) {
+               vty_out(vty,
+                       "WARNING: exiting with a pending confirmed commit. Rolling back to previous configuration.\n\n");
+               nb_cli_confirmed_commit_rollback(vty);
+               nb_cli_confirmed_commit_clean(vty);
+       }
+
        vty_config_exclusive_unlock(vty);
 
        if (vty->candidate_config) {

diff --git a/lib/vty.h b/lib/vty.h
index ae6c4bae965390db8fd20af8287c32648fcc6cc4..ad4dc273b971ca31d0bed739c1df7584f9043dac 100644 (file)
--- a/lib/vty.h
+++ b/lib/vty.h
@@ -126,6 +126,10 @@ struct vty {
        /* Base candidate configuration. */
        struct nb_config *candidate_config_base;
 
+       /* Confirmed-commit timeout and rollback configuration. */
+       struct thread *t_confirmed_commit_timeout;
+       struct nb_config *confirmed_commit_rollback;
+
        /* qobj object ID (replacement for "index") */
        uint64_t qobj_index;
 

diff --git a/tests/bgpd/test_peer_attr.c b/tests/bgpd/test_peer_attr.c
index 2fbc686e1e81dc5135e5ac8b6b59e391e4edd076..78016dc9cefe544dfde53ab6bdb3ac386e95e640 100644 (file)
--- a/tests/bgpd/test_peer_attr.c
+++ b/tests/bgpd/test_peer_attr.c
@@ -1384,10 +1384,10 @@ static void bgp_startup(void)
                 LOG_DAEMON);
        zprivs_preinit(&bgpd_privs);
        zprivs_init(&bgpd_privs);
-       yang_init();
-       nb_init(NULL, 0);
 
        master = thread_master_create(NULL);
+       yang_init();
+       nb_init(master, NULL, 0);
        bgp_master_init(master);
        bgp_option_set(BGP_OPT_NO_LISTEN);
        vrf_init(NULL, NULL, NULL, NULL, NULL);

diff --git a/tests/helpers/c/main.c b/tests/helpers/c/main.c
index 9e34a7c255ba45f9345b3b2978190941bbe343ee..768cf296ad407c1e50ca71c7c41645922cdff883 100644 (file)
--- a/tests/helpers/c/main.c
+++ b/tests/helpers/c/main.c
@@ -156,7 +156,7 @@ int main(int argc, char **argv)
        vty_init(master);
        memory_init();
        yang_init();
-       nb_init(NULL, 0);
+       nb_init(master, NULL, 0);
 
        /* OSPF vty inits. */
        test_vty_init();

diff --git a/tests/lib/cli/common_cli.c b/tests/lib/cli/common_cli.c
index 04f1e3253d0b625e5eec58647054561dcdb33da0..3cf30f00c378e889ead2e0f25532792f8051a88a 100644 (file)
--- a/tests/lib/cli/common_cli.c
+++ b/tests/lib/cli/common_cli.c
@@ -84,7 +84,7 @@ int main(int argc, char **argv)
        vty_init(master);
        memory_init();
        yang_init();
-       nb_init(NULL, 0);
+       nb_init(master, NULL, 0);
 
        test_init(argc, argv);
 

diff --git a/tests/lib/cli/test_commands.c b/tests/lib/cli/test_commands.c
index 74816ece8c54c32a62610cd6c5411de4c76cf6bf..ba46bdcea9dbd8336d6a0fdf8e4d1c19013dde4c 100644 (file)
--- a/tests/lib/cli/test_commands.c
+++ b/tests/lib/cli/test_commands.c
@@ -143,7 +143,7 @@ static void test_init(void)
 
        cmd_init(1);
        yang_init();
-       nb_init(NULL, 0);
+       nb_init(master, NULL, 0);
 
        install_node(&bgp_node, NULL);
        install_node(&rip_node, NULL);

diff --git a/tests/lib/northbound/test_oper_data.c b/tests/lib/northbound/test_oper_data.c
index a9a89ee491a40759ff314e02f0c29142323219a3..7c5713d8f99ce5c50a0a8e0d9782026ca34c6879 100644 (file)
--- a/tests/lib/northbound/test_oper_data.c
+++ b/tests/lib/northbound/test_oper_data.c
@@ -449,7 +449,7 @@ int main(int argc, char **argv)
        vty_init(master);
        memory_init();
        yang_init();
-       nb_init(modules, array_size(modules));
+       nb_init(master, modules, array_size(modules));
 
        /* Create artificial data. */
        create_data(num_vrfs, num_interfaces, num_routes);