From b855e95fd36f66d6644437afa248ad6afe6f4c44 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 2 Jul 2020 14:43:36 -0300 Subject: [PATCH] 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 --- lib/command.c | 7 +++ lib/if.c | 1 + lib/northbound_cli.c | 132 ++++++++++++++++++++++++++++++++++++------- lib/northbound_cli.h | 8 +++ lib/routemap_cli.c | 1 + lib/vrf.c | 1 + lib/vty.c | 3 + lib/vty.h | 8 +++ 8 files changed, 142 insertions(+), 19 deletions(-) diff --git a/lib/command.c b/lib/command.c index 80b75d9b2327..159ed07b3822 100644 --- 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 8d59e6bd6886..07e786c7086d 100644 --- 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 105fc83cefc1..534b5128ee52 100644 --- 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 b2d8c8f0353f..112d62efda7d 100644 --- 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 f92c16905079..014147c3f8a5 100644 --- 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 f9a1ac19faeb..20e08b03d848 100644 --- 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 0d0e54af6c73..184c7604b809 100644 --- 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 5d5676199b50..623f97ab0370 100644 --- 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;