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>

8 files changed, 142 insertions, 19 deletions

diff --git a/lib/command.c b/lib/command.c
index 80b75d9b2..159ed07b3 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 8d59e6bd6..07e786c70 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 105fc83ce..534b5128e 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 b2d8c8f03..112d62efd 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 f92c16905..014147c3f 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 f9a1ac19f..20e08b03d 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 0d0e54af6..184c7604b 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 5d5676199..623f97ab0 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;