lib: introduce configuration back-off timer for YANG-modeled commands

When using the default CLI mode, the northbound layer needs to create
a separate transaction to process each YANG-modeled command since
they are supposed to be applied immediately (there's no candidate
configuration nor the "commit" command like in the transactional
CLI). The problem is that configuration transactions have an overhead
associated to them, in big part because of the use of some heavy
libyang functions like `lyd_validate()` and `lyd_diff()`. As of
now this overhead is substantial and doesn't scale well when large
numbers of transactions need to be performed in sequence.

As an example, loading 50k prefix-lists using a single transaction
takes about 2 seconds on a modern CPU. Loading the same 50k
prefix-lists using 50k transactions can take more than an hour
to complete (which is unacceptable by any standard). To fix this
problem, some heavy optimization work needs to be done on libyang and
on the FRR northbound itself too (e.g. perform partial configuration
diffs whenever possible).  This, however, should be a long term
effort since these optimizations shouldn't be trivial to implement
and we're far from having the performance numbers we need.

In the meanwhile, this commit introduces a simple but efficient
workaround to alleviate the issue. In short, a new back-off timer
was introduced in the CLI to monitor and detect when too many
YANG-modeled commands are being received at the same time. When
a certain threshold is reached (100 YANG-modeled commands within
one second), the northbound starts to group all subsequent commands
into a single large transaction, which allows them to be processed
much faster (e.g. seconds and not hours).  It's essentially a
protection mechanism that creates dynamically-sized transactions
when necessary to prevent performance issues from happening. This
mechanism is enabled both when parsing configuration files and when
reading commands from a terminal.

The downside of this optimization is that, if several YANG-modeled
commands are grouped into the same transaction and at least one of
them fails, the whole transaction is rejected. This is undesirable
since users don't expect transactional behavior when that's not
enabled explicitly. To minimize this issue, the CLI will log all
commands that were rejected whenever that happens, to make the
user aware of what happened and have enough information to fix
the problem. Commands that fail due to parsing errors or CLI-level
validations in general are rejected separately.

Again, this proposed workaround is intended to be temporary. The
goal is to provided a quick fix to issues like #6658 while we work
on better long-term solutions.

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

diff --git a/lib/command.c b/lib/command.c
index 80b75d9b2327567b880dde251ff1b4c91af2a6dd..159ed07b3822ff44150a5148667fb03cc8bae35b 100644 (file)
--- a/lib/command.c
+++ b/lib/command.c
@@ -904,6 +904,13 @@ static int cmd_execute_command_real(vector vline, enum cmd_filter_type filter,
                                       > vty->candidate_config->version)
                                nb_config_replace(vty->candidate_config,
                                                  running_config, true);
+
+                       /*
+                        * Perform pending commit (if any) before executing
+                        * non-YANG command.
+                        */
+                       if (matched_element->attr != CMD_ATTR_YANG)
+                               nb_cli_pending_commit_check(vty);
                }
 
                ret = matched_element->func(matched_element, vty, argc, argv);

diff --git a/lib/if.c b/lib/if.c
index 8d59e6bd6886ef6ed0bf79170cf18e0d949a4f8c..07e786c7086d714061f55d631ea71d139b80911b 100644 (file)
--- a/lib/if.c
+++ b/lib/if.c
@@ -1384,6 +1384,7 @@ DEFPY_YANG_NOSH (interface,
                 * all interface-level commands are converted to the new
                 * northbound model.
                 */
+               nb_cli_pending_commit_check(vty);
                ifp = if_lookup_by_name(ifname, vrf_id);
                if (ifp)
                        VTY_PUSH_CONTEXT(INTERFACE_NODE, ifp);

diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c
index 105fc83cefc1947fae274db65e81dbf451c5a075..534b5128ee52b100ac93ccf0ebb1491d007b3e53 100644 (file)
--- a/lib/northbound_cli.c
+++ b/lib/northbound_cli.c
@@ -53,6 +53,106 @@ static void vty_show_nb_errors(struct vty *vty, int error, const char *errmsg)
                vty_out(vty, "Error description: %s\n", errmsg);
 }
 
+static int nb_cli_classic_commit(struct vty *vty)
+{
+       struct nb_context context = {};
+       char errmsg[BUFSIZ] = {0};
+       int ret;
+
+       context.client = NB_CLIENT_CLI;
+       context.user = vty;
+       ret = nb_candidate_commit(&context, vty->candidate_config, true, NULL,
+                                 NULL, errmsg, sizeof(errmsg));
+       if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) {
+               vty_out(vty, "%% Configuration failed.\n\n");
+               vty_show_nb_errors(vty, ret, errmsg);
+               if (vty->t_pending_commit)
+                       vty_out(vty,
+                               "The following commands were dynamically grouped into the same transaction and rejected:\n%s",
+                               vty->pending_cmds_buf);
+
+               /* Regenerate candidate for consistency. */
+               nb_config_replace(vty->candidate_config, running_config, true);
+               return CMD_WARNING_CONFIG_FAILED;
+       }
+
+       return CMD_SUCCESS;
+}
+
+static void nb_cli_pending_commit_clear(struct vty *vty)
+{
+       THREAD_TIMER_OFF(vty->t_pending_commit);
+       vty->backoff_cmd_count = 0;
+       XFREE(MTYPE_TMP, vty->pending_cmds_buf);
+       vty->pending_cmds_buflen = 0;
+       vty->pending_cmds_bufpos = 0;
+}
+
+static int nb_cli_pending_commit_cb(struct thread *thread)
+{
+       struct vty *vty = THREAD_ARG(thread);
+
+       (void)nb_cli_classic_commit(vty);
+       nb_cli_pending_commit_clear(vty);
+
+       return 0;
+}
+
+void nb_cli_pending_commit_check(struct vty *vty)
+{
+       if (vty->t_pending_commit) {
+               (void)nb_cli_classic_commit(vty);
+               nb_cli_pending_commit_clear(vty);
+       }
+}
+
+static bool nb_cli_backoff_start(struct vty *vty)
+{
+       struct timeval now, delta;
+
+       /*
+        * Start the configuration backoff timer only if 100 YANG-modeled
+        * commands or more were entered within the last second.
+        */
+       monotime(&now);
+       if (monotime_since(&vty->backoff_start, &delta) >= 1000000) {
+               vty->backoff_start = now;
+               vty->backoff_cmd_count = 1;
+               return false;
+       }
+       if (++vty->backoff_cmd_count < 100)
+               return false;
+
+       return true;
+}
+
+static int nb_cli_schedule_command(struct vty *vty)
+{
+       /* Append command to dynamically sized buffer of scheduled commands. */
+       if (!vty->pending_cmds_buf) {
+               vty->pending_cmds_buflen = 4096;
+               vty->pending_cmds_buf =
+                       XCALLOC(MTYPE_TMP, vty->pending_cmds_buflen);
+       }
+       if ((strlen(vty->buf) + 3)
+           > (vty->pending_cmds_buflen - vty->pending_cmds_bufpos)) {
+               vty->pending_cmds_buflen *= 2;
+               vty->pending_cmds_buf =
+                       XREALLOC(MTYPE_TMP, vty->pending_cmds_buf,
+                                vty->pending_cmds_buflen);
+       }
+       strlcat(vty->pending_cmds_buf, "- ", vty->pending_cmds_buflen);
+       vty->pending_cmds_bufpos = strlcat(vty->pending_cmds_buf, vty->buf,
+                                          vty->pending_cmds_buflen);
+
+       /* Schedule the commit operation. */
+       THREAD_TIMER_OFF(vty->t_pending_commit);
+       thread_add_timer_msec(master, nb_cli_pending_commit_cb, vty, 100,
+                             &vty->t_pending_commit);
+
+       return CMD_SUCCESS;
+}
+
 void nb_cli_enqueue_change(struct vty *vty, const char *xpath,
                           enum nb_operation operation, const char *value)
 {
@@ -76,7 +176,6 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
 {
        char xpath_base[XPATH_MAXLEN] = {};
        bool error = false;
-       int ret;
 
        VTY_CHECK_XPATH;
 
@@ -95,6 +194,7 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
                struct nb_node *nb_node;
                char xpath[XPATH_MAXLEN];
                struct yang_data *data;
+               int ret;
 
                /* Handle relative XPaths. */
                memset(xpath, 0, sizeof(xpath));
@@ -158,25 +258,19 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
                        yang_print_errors(ly_native_ctx, buf, sizeof(buf)));
        }
 
-       /* Do an implicit "commit" when using the classic CLI mode. */
+       /*
+        * Do an implicit commit when using the classic CLI mode.
+        *
+        * NOTE: the implicit commit might be scheduled to run later when
+        * too many commands are being sent at the same time. This is a
+        * protection mechanism where multiple commands are grouped into the
+        * same configuration transaction, allowing them to be processed much
+        * faster.
+        */
        if (frr_get_cli_mode() == FRR_CLI_CLASSIC) {
-               struct nb_context context = {};
-               char errmsg[BUFSIZ] = {0};
-
-               context.client = NB_CLIENT_CLI;
-               context.user = vty;
-               ret = nb_candidate_commit(&context, vty->candidate_config,
-                                         false, NULL, NULL, errmsg,
-                                         sizeof(errmsg));
-               if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) {
-                       vty_out(vty, "%% Configuration failed.\n\n");
-                       vty_show_nb_errors(vty, ret, errmsg);
-
-                       /* Regenerate candidate for consistency. */
-                       nb_config_replace(vty->candidate_config, running_config,
-                                         true);
-                       return CMD_WARNING_CONFIG_FAILED;
-               }
+               if (vty->t_pending_commit || nb_cli_backoff_start(vty))
+                       return nb_cli_schedule_command(vty);
+               return nb_cli_classic_commit(vty);
        }
 
        return CMD_SUCCESS;

diff --git a/lib/northbound_cli.h b/lib/northbound_cli.h
index b2d8c8f0353f4f2d38046fd0e2d6423315cd3b42..112d62efda7d670b778138b51e1ecede18d8dc13 100644 (file)
--- a/lib/northbound_cli.h
+++ b/lib/northbound_cli.h
@@ -108,6 +108,14 @@ extern int nb_cli_rpc(const char *xpath, struct list *input,
 extern void nb_cli_show_dnode_cmds(struct vty *vty, struct lyd_node *dnode,
                                   bool show_defaults);
 
+/*
+ * Perform pending commit, if any.
+ *
+ * vty
+ *    The vty context.
+ */
+extern void nb_cli_pending_commit_check(struct vty *vty);
+
 /* Prototypes of internal functions. */
 extern void nb_cli_show_config_prepare(struct nb_config *config,
                                       bool with_defaults);

diff --git a/lib/routemap_cli.c b/lib/routemap_cli.c
index f92c16905079200568fe1df422a49c4d9064661a..014147c3f8a520cc0d24928812abfd08e614db6a 100644 (file)
--- a/lib/routemap_cli.c
+++ b/lib/routemap_cli.c
@@ -70,6 +70,7 @@ DEFPY_YANG_NOSH(
                VTY_PUSH_XPATH(RMAP_NODE, xpath_index);
 
                /* Add support for non-migrated route map users. */
+               nb_cli_pending_commit_check(vty);
                rm = route_map_get(name);
                action_type = (action[0] == 'p') ? RMAP_PERMIT : RMAP_DENY;
                rmi = route_map_index_get(rm, action_type, sequence);

diff --git a/lib/vrf.c b/lib/vrf.c
index f9a1ac19faebd6dd20fd7fc95bf7355ebf1b9d9a..20e08b03d848f32a796fd15cc821fc9db0b65700 100644 (file)
--- a/lib/vrf.c
+++ b/lib/vrf.c
@@ -638,6 +638,7 @@ int vrf_handler_create(struct vty *vty, const char *vrfname,
                ret = nb_cli_apply_changes(vty, xpath_list);
                if (ret == CMD_SUCCESS) {
                        VTY_PUSH_XPATH(VRF_NODE, xpath_list);
+                       nb_cli_pending_commit_check(vty);
                        vrfp = vrf_lookup_by_name(vrfname);
                        if (vrfp)
                                VTY_PUSH_CONTEXT(VRF_NODE, vrfp);

diff --git a/lib/vty.c b/lib/vty.c
index 0d0e54af6c73d029c4988c0cdc2478ef33cf411a..184c7604b8090379f5a408b73a6f274d981652bb 100644 (file)
--- a/lib/vty.c
+++ b/lib/vty.c
@@ -2631,6 +2631,9 @@ int vty_config_node_exit(struct vty *vty)
 {
        vty->xpath_index = 0;
 
+       /* Perform pending commit if any. */
+       nb_cli_pending_commit_check(vty);
+
        /* Check if there's a pending confirmed commit. */
        if (vty->t_confirmed_commit_timeout) {
                vty_out(vty,

diff --git a/lib/vty.h b/lib/vty.h
index 5d5676199b50da03fd9963f630c709c14a83f0c6..623f97ab03704e34c9a77ce9faea9518ba8aa203 100644 (file)
--- a/lib/vty.h
+++ b/lib/vty.h
@@ -134,6 +134,14 @@ struct vty {
        /* Base candidate configuration. */
        struct nb_config *candidate_config_base;
 
+       /* Dynamic transaction information. */
+       struct timeval backoff_start;
+       size_t backoff_cmd_count;
+       struct thread *t_pending_commit;
+       char *pending_cmds_buf;
+       size_t pending_cmds_buflen;
+       size_t pending_cmds_bufpos;
+
        /* Confirmed-commit timeout and rollback configuration. */
        struct thread *t_confirmed_commit_timeout;
        struct nb_config *confirmed_commit_rollback;