From caaa8b9c834dcf26292af67ebaaf154cac7cb8fe Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Sat, 5 Oct 2019 20:52:36 -0300 Subject: [PATCH 1/6] lib: optimize VTY_CHECK_XPATH There's no need to check for removed configuration objects in the following two cases: * A private candidate configuration is being edited; * The startup configuration is being read. Signed-off-by: Renato Westphal --- lib/vty.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/vty.h b/lib/vty.h index c7352efbd3..a0b8e8afa1 100644 --- a/lib/vty.h +++ b/lib/vty.h @@ -254,7 +254,8 @@ static inline void vty_push_context(struct vty *vty, int node, uint64_t id) #define VTY_CHECK_XPATH \ do { \ - if (vty->xpath_index > 0 \ + if (vty->type != VTY_FILE && !vty->private_config \ + && vty->xpath_index > 0 \ && !yang_dnode_exists(vty->candidate_config->dnode, \ VTY_CURR_XPATH)) { \ vty_out(vty, \ From c7c9103b01899d316372cc0272c37df9a7e59426 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Sat, 5 Oct 2019 21:20:28 -0300 Subject: [PATCH 2/6] lib: remove expensive error handling in the northbound CLI client The nb_cli_apply_changes() function was creating a full copy of the candidate configuration before editing it. This excerpt from the northbond documentation explains why this was being done: "NOTE: the nb_cli_cfg_change() function clones the candidate configuration before actually editing it. This way, if any error happens during the editing, the original candidate is restored to avoid inconsistencies. Either all changes from the configuration command are performed successfully or none are. It's like a mini-transaction but happening on the candidate configuration (thus the northbound callbacks are not involved)". The problem is that this kind of error handling is just too expensive. A command should never fail to edit the candidate configuration unless there's a bug in the code (e.g. when the CLI wrapper command passes an integer value that YANG rejects due to a "range" statement). In such cases, a command might fail to be applied or applied only partially if it edits multiple YANG nodes. When that happens, just log an error to make the operator aware of the problem, but otherwise ignore it instead of rejecting the command and restoring the candidate to its previous state. We shouldn't add an extreme overhead to the northbound CLI client only to handle errors that should never happen in practice. Signed-off-by: Renato Westphal --- lib/northbound_cli.c | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index a15fe3d1c9..a215d8b759 100644 --- a/lib/northbound_cli.c +++ b/lib/northbound_cli.c @@ -84,20 +84,12 @@ void nb_cli_enqueue_change(struct vty *vty, const char *xpath, int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) { - struct nb_config *candidate_transitory; char xpath_base[XPATH_MAXLEN] = {}; bool error = false; int ret; VTY_CHECK_XPATH; - /* - * Create a copy of the candidate configuration. For consistency, we - * need to ensure that either all changes made by the command are - * accepted or none are. - */ - candidate_transitory = nb_config_dup(vty->candidate_config); - /* Parse the base XPath format string. */ if (xpath_base_fmt) { va_list ap; @@ -137,7 +129,7 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) flog_warn(EC_LIB_YANG_UNKNOWN_DATA_PATH, "%s: unknown data path: %s", __func__, xpath); error = true; - break; + continue; } /* If the value is not set, get the default if it exists. */ @@ -149,7 +141,7 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) * Ignore "not found" errors when editing the candidate * configuration. */ - ret = nb_candidate_edit(candidate_transitory, nb_node, + ret = nb_candidate_edit(vty->candidate_config, nb_node, change->operation, xpath, NULL, data); yang_data_free(data); if (ret != NB_OK && ret != NB_ERR_NOT_FOUND) { @@ -159,29 +151,20 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) __func__, nb_operation_name(change->operation), xpath); error = true; - break; + continue; } } if (error) { - nb_config_free(candidate_transitory); - - switch (frr_get_cli_mode()) { - case FRR_CLI_CLASSIC: - vty_out(vty, "%% Configuration failed.\n\n"); - break; - case FRR_CLI_TRANSACTIONAL: - vty_out(vty, - "%% Failed to edit candidate configuration.\n\n"); - break; - } + /* + * Failure to edit the candidate configuration should never + * happen in practice, unless there's a bug in the code. When + * that happens, log the error but otherwise ignore it. + */ + vty_out(vty, "%% Failed to edit configuration.\n\n"); vty_show_libyang_errors(vty, ly_native_ctx); - - return CMD_WARNING_CONFIG_FAILED; } - nb_config_replace(vty->candidate_config, candidate_transitory, false); - /* Do an implicit "commit" when using the classic CLI mode. */ if (frr_get_cli_mode() == FRR_CLI_CLASSIC) { ret = nb_candidate_commit(vty->candidate_config, NB_CLIENT_CLI, From 5e6a9350c16f54113eeedb497c98fe45b8ce6222 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Sun, 6 Oct 2019 01:16:47 -0300 Subject: [PATCH 3/6] lib: avoid expensive operations when editing a candidate config nb_candidate_edit() was calling both the lyd_schema_sort() and lyd_validate() functions whenever a new node was added to the candidate configuration. This was done to ensure the candidate is always ready to be displayed correctly (libyang only creates default child nodes during the validation process, and data nodes aren't guaranteed to be ordered by default). The problem is that the two aforementioned functions are too expensive to be called in the northbound hot path. Instead, it makes more sense to call them only before displaying the configuration (in which case a recursive sort needs to be done). Introduce the nb_cli_show_config_prepare() to achieve that purpose. Signed-off-by: Renato Westphal --- lib/command.c | 2 ++ lib/northbound.c | 11 ----------- lib/northbound_cli.c | 19 +++++++++++++++++++ lib/northbound_cli.h | 2 ++ 4 files changed, 23 insertions(+), 11 deletions(-) diff --git a/lib/command.c b/lib/command.c index 04f2bd95a0..32e877280d 100644 --- a/lib/command.c +++ b/lib/command.c @@ -1709,6 +1709,8 @@ static int vty_write_config(struct vty *vty) if (host.noconfig) return CMD_SUCCESS; + nb_cli_show_config_prepare(running_config, false); + if (vty->type == VTY_TERM) { vty_out(vty, "\nCurrent configuration:\n"); vty_out(vty, "!\n"); diff --git a/lib/northbound.c b/lib/northbound.c index 1b332fb1e8..2f14b287d9 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -516,17 +516,6 @@ int nb_candidate_edit(struct nb_config *candidate, __func__); return NB_ERR; } - - /* - * If a new node was created, call lyd_validate() only to create - * default child nodes. - */ - if (dnode) { - lyd_schema_sort(dnode, 0); - lyd_validate(&dnode, - LYD_OPT_CONFIG | LYD_OPT_WHENAUTODEL, - ly_native_ctx); - } break; case NB_OP_DESTROY: dnode = yang_dnode_get(candidate->dnode, xpath_edit); diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index a215d8b759..07e8c9dff9 100644 --- a/lib/northbound_cli.c +++ b/lib/northbound_cli.c @@ -421,6 +421,23 @@ static struct lyd_node *ly_iter_next_up(const struct lyd_node *elem) return elem->parent; } +/* Prepare the configuration for display. */ +void nb_cli_show_config_prepare(struct nb_config *config, bool with_defaults) +{ + lyd_schema_sort(config->dnode, 1); + + /* + * When "with-defaults" is used, call lyd_validate() only to create + * default child nodes, ignoring any possible validation error. This + * doesn't need to be done when displaying the running configuration + * since it's always fully validated. + */ + if (with_defaults && config != running_config) + (void)lyd_validate(&config->dnode, + LYD_OPT_CONFIG | LYD_OPT_WHENAUTODEL, + ly_native_ctx); +} + void nb_cli_show_dnode_cmds(struct vty *vty, struct lyd_node *root, bool with_defaults) { @@ -513,6 +530,8 @@ static int nb_cli_show_config(struct vty *vty, struct nb_config *config, struct yang_translator *translator, bool with_defaults) { + nb_cli_show_config_prepare(config, with_defaults); + switch (format) { case NB_CFG_FMT_CMDS: nb_cli_show_config_cmds(vty, config, with_defaults); diff --git a/lib/northbound_cli.h b/lib/northbound_cli.h index 209239ca63..b2d8c8f035 100644 --- a/lib/northbound_cli.h +++ b/lib/northbound_cli.h @@ -109,6 +109,8 @@ 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_show_config_prepare(struct nb_config *config, + bool with_defaults); 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); From 91f9fd78cb32396e95567fc59ec9741b620c22be Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Wed, 9 Oct 2019 22:17:08 -0300 Subject: [PATCH 4/6] lib: optimize loading of the startup configuration Load the startup configuration directly into the CLI shared candidate configuration instead of loading it into a private candidate configuration. This way we don't need to initialize the shared candidate separately later as a copy of the running configuration, which is a potentially expensive operation. Also, make the northbound process SIGHUP correctly even when --tcli is not used. Signed-off-by: Renato Westphal --- lib/libfrr.c | 22 +++++++++++++++++----- lib/vty.c | 3 +-- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/lib/libfrr.c b/lib/libfrr.c index d4aa1f899a..8ef32eaa8a 100644 --- a/lib/libfrr.c +++ b/lib/libfrr.c @@ -853,22 +853,34 @@ static void frr_daemonize(void) */ static int frr_config_read_in(struct thread *t) { - if (!vty_read_config(NULL, di->config_file, config_default) && - di->backup_config_file) { + if (!vty_read_config(vty_shared_candidate_config, di->config_file, + config_default) + && di->backup_config_file) { char *orig = XSTRDUP(MTYPE_TMP, host_config_get()); zlog_info("Attempting to read backup config file: %s specified", di->backup_config_file); - vty_read_config(NULL, di->backup_config_file, config_default); + vty_read_config(vty_shared_candidate_config, + di->backup_config_file, config_default); host_config_set(orig); XFREE(MTYPE_TMP, orig); } /* - * Update the shared candidate after reading the startup configuration. + * Automatically commit the candidate configuration after + * reading the configuration file. */ - nb_config_replace(vty_shared_candidate_config, running_config, true); + if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL) { + int ret; + + ret = nb_candidate_commit(vty_shared_candidate_config, + NB_CLIENT_CLI, NULL, true, + "Read configuration file", NULL); + if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) + zlog_err("%s: failed to read configuration file.", + __func__); + } return 0; } diff --git a/lib/vty.c b/lib/vty.c index 502d2c9d04..c08e5e151a 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -2341,8 +2341,7 @@ static void vty_read_file(struct nb_config *config, FILE *confp) * Automatically commit the candidate configuration after * reading the configuration file. */ - if (config == NULL && vty->candidate_config - && frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL) { + if (config == NULL) { ret = nb_candidate_commit(vty->candidate_config, NB_CLIENT_CLI, vty, true, "Read configuration file", NULL); From fe3f2c61939a9dda27bf84feb4312f92bf1e1bb5 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Wed, 9 Oct 2019 23:24:04 -0300 Subject: [PATCH 5/6] lib: fix processing of the 'apply_finish' callbacks Commit 6b5d6e2dbc88 changed how we order configuration callbacks and introduced a regression in the processing of the 'apply_finish' callbacks. Fix this. Signed-off-by: Renato Westphal --- lib/northbound.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index 2f14b287d9..a3b75d5c89 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -350,7 +350,22 @@ static inline int nb_config_cb_compare(const struct nb_config_cb *a, /* * Preserve the order of the configuration changes as told by libyang. */ - return a->seq - b->seq; + if (a->seq < b->seq) + return -1; + if (a->seq > b->seq) + return 1; + + /* + * All 'apply_finish' callbacks have their sequence number set to zero. + * In this case, compare them using their dnode pointers (the order + * doesn't matter for callbacks that have the same priority). + */ + if (a->dnode < b->dnode) + return -1; + if (a->dnode > b->dnode) + return 1; + + return 0; } RB_GENERATE(nb_config_cbs, nb_config_cb, entry, nb_config_cb_compare); @@ -1025,13 +1040,15 @@ nb_apply_finish_cb_new(struct nb_config_cbs *cbs, const char *xpath, } static struct nb_config_cb * -nb_apply_finish_cb_find(struct nb_config_cbs *cbs, const char *xpath, - const struct nb_node *nb_node) +nb_apply_finish_cb_find(struct nb_config_cbs *cbs, + const struct nb_node *nb_node, + const struct lyd_node *dnode) { struct nb_config_cb s; - strlcpy(s.xpath, xpath, sizeof(s.xpath)); + s.seq = 0; s.nb_node = nb_node; + s.dnode = dnode; return RB_FIND(nb_config_cbs, cbs, &s); } @@ -1085,7 +1102,7 @@ static void nb_transaction_apply_finish(struct nb_transaction *transaction) * data node. */ yang_dnode_get_path(dnode, xpath, sizeof(xpath)); - if (nb_apply_finish_cb_find(&cbs, xpath, nb_node)) + if (nb_apply_finish_cb_find(&cbs, nb_node, dnode)) goto next; nb_apply_finish_cb_new(&cbs, xpath, nb_node, dnode); From 0de19c0e0a8e4f8de4f3e27220f53fae33a6f817 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Wed, 9 Oct 2019 22:20:26 -0300 Subject: [PATCH 6/6] lib: reduce memory allocation when processing large config transactions Remove the xpath field from the nb_config_cb structure in order to reduce its size. This allows the northbound to spend less time allocating memory during the processing of large configuration transactions. To make this work, use yang_dnode_get_path() to obtain the xpath from the dnode field whenever necessary. Signed-off-by: Renato Westphal --- lib/northbound.c | 26 +++++++++++++------------- lib/northbound.h | 1 - 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index a3b75d5c89..debd463624 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -381,7 +381,6 @@ static void nb_config_diff_add_change(struct nb_config_cbs *changes, change->cb.seq = *seq; *seq = *seq + 1; change->cb.nb_node = dnode->schema->priv; - yang_dnode_get_path(dnode, change->cb.xpath, sizeof(change->cb.xpath)); change->cb.dnode = dnode; RB_INSERT(nb_config_cbs, changes, &change->cb); @@ -810,7 +809,7 @@ static int nb_callback_configuration(const enum nb_event event, struct nb_config_change *change) { enum nb_operation operation = change->cb.operation; - const char *xpath = change->cb.xpath; + char xpath[XPATH_MAXLEN]; const struct nb_node *nb_node = change->cb.nb_node; const struct lyd_node *dnode = change->cb.dnode; union nb_resource *resource; @@ -822,6 +821,7 @@ static int nb_callback_configuration(const enum nb_event event, if (dnode && !yang_snode_is_typeless_data(dnode->schema)) value = yang_dnode_get_string(dnode, NULL); + yang_dnode_get_path(dnode, xpath, sizeof(xpath)); nb_log_callback(event, operation, xpath, value); } @@ -844,6 +844,7 @@ static int nb_callback_configuration(const enum nb_event event, ret = (*nb_node->cbs.move)(event, dnode); break; default: + yang_dnode_get_path(dnode, xpath, sizeof(xpath)); flog_err(EC_LIB_DEVELOPMENT, "%s: unknown operation (%u) [xpath %s]", __func__, operation, xpath); @@ -854,6 +855,8 @@ static int nb_callback_configuration(const enum nb_event event, int priority; enum lib_log_refs ref; + yang_dnode_get_path(dnode, xpath, sizeof(xpath)); + switch (event) { case NB_EV_VALIDATE: priority = LOG_WARNING; @@ -1024,14 +1027,12 @@ static int nb_transaction_process(enum nb_event event, } static struct nb_config_cb * -nb_apply_finish_cb_new(struct nb_config_cbs *cbs, const char *xpath, - const struct nb_node *nb_node, +nb_apply_finish_cb_new(struct nb_config_cbs *cbs, const struct nb_node *nb_node, const struct lyd_node *dnode) { struct nb_config_cb *cb; cb = XCALLOC(MTYPE_TMP, sizeof(*cb)); - strlcpy(cb->xpath, xpath, sizeof(cb->xpath)); cb->nb_node = nb_node; cb->dnode = dnode; RB_INSERT(nb_config_cbs, cbs, cb); @@ -1057,6 +1058,7 @@ static void nb_transaction_apply_finish(struct nb_transaction *transaction) { struct nb_config_cbs cbs; struct nb_config_cb *cb; + char xpath[XPATH_MAXLEN]; /* Initialize tree of 'apply_finish' callbacks. */ RB_INIT(nb_config_cbs, &cbs); @@ -1073,8 +1075,6 @@ static void nb_transaction_apply_finish(struct nb_transaction *transaction) * be called though). */ if (change->cb.operation == NB_OP_DESTROY) { - char xpath[XPATH_MAXLEN]; - dnode = dnode->parent; if (!dnode) break; @@ -1090,7 +1090,6 @@ static void nb_transaction_apply_finish(struct nb_transaction *transaction) xpath); } while (dnode) { - char xpath[XPATH_MAXLEN]; struct nb_node *nb_node; nb_node = dnode->schema->priv; @@ -1101,11 +1100,10 @@ static void nb_transaction_apply_finish(struct nb_transaction *transaction) * Don't call the callback more than once for the same * data node. */ - yang_dnode_get_path(dnode, xpath, sizeof(xpath)); if (nb_apply_finish_cb_find(&cbs, nb_node, dnode)) goto next; - nb_apply_finish_cb_new(&cbs, xpath, nb_node, dnode); + nb_apply_finish_cb_new(&cbs, nb_node, dnode); next: dnode = dnode->parent; @@ -1114,9 +1112,11 @@ static void nb_transaction_apply_finish(struct nb_transaction *transaction) /* Call the 'apply_finish' callbacks, sorted by their priorities. */ RB_FOREACH (cb, nb_config_cbs, &cbs) { - if (DEBUG_MODE_CHECK(&nb_dbg_cbs_config, DEBUG_MODE_ALL)) - nb_log_callback(NB_EV_APPLY, NB_OP_APPLY_FINISH, - cb->xpath, NULL); + if (DEBUG_MODE_CHECK(&nb_dbg_cbs_config, DEBUG_MODE_ALL)) { + yang_dnode_get_path(cb->dnode, xpath, sizeof(xpath)); + nb_log_callback(NB_EV_APPLY, NB_OP_APPLY_FINISH, xpath, + NULL); + } (*cb->nb_node->cbs.apply_finish)(cb->dnode); } diff --git a/lib/northbound.h b/lib/northbound.h index fbd7771db7..f52fcc90cf 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -458,7 +458,6 @@ struct nb_config_cb { RB_ENTRY(nb_config_cb) entry; enum nb_operation operation; uint32_t seq; - char xpath[XPATH_MAXLEN]; const struct nb_node *nb_node; const struct lyd_node *dnode; };