diff --git a/lib/mgmt_be_client.c b/lib/mgmt_be_client.c index 7bd9980357d5..0b69e04e4c1d 100644 --- a/lib/mgmt_be_client.c +++ b/lib/mgmt_be_client.c @@ -433,7 +433,7 @@ static int mgmt_be_txn_cfg_prepare(struct mgmt_be_txn_ctx *txn) client_ctx->candidate_config, txn_req->req.set_cfg.cfg_changes, (size_t)txn_req->req.set_cfg.num_cfg_changes, - NULL, NULL, 0, err_buf, sizeof(err_buf), + NULL, NULL, 0, false, err_buf, sizeof(err_buf), &error); if (error) { err_buf[sizeof(err_buf) - 1] = 0; diff --git a/lib/northbound.c b/lib/northbound.c index 69b96d365693..f4fa88a007e7 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -500,6 +500,27 @@ static void nb_config_diff_deleted(const struct lyd_node *dnode, uint32_t *seq, } } +/* + * Check if a data node already exists in one of the config change + * entry. If so return the exact config change entry. + */ +static struct nb_config_change *nb_config_find_change(struct nb_config_cbs *changes, + struct lyd_node *dnode) +{ + struct nb_config_cb *cb; + struct nb_config_change *chg = NULL; + + RB_FOREACH (cb, nb_config_cbs, changes) { + chg = (struct nb_config_change *)cb; + + zlog_debug("%s: Dnode: %p", __func__, chg->cb.dnode); + if (chg->cb.dnode == dnode) + return chg; + } + + return NULL; +} + static int nb_lyd_diff_get_op(const struct lyd_node *dnode) { const struct lyd_meta *meta; @@ -753,6 +774,10 @@ int nb_candidate_edit(struct nb_config *candidate, return NB_OK; } +/* + * Update changes to the scratch buffer associated with the specifc + * candidate data tree. + */ static void nb_update_candidate_changes(struct nb_config *candidate, struct nb_cfg_change *change, uint32_t *seq) @@ -763,14 +788,49 @@ static void nb_update_candidate_changes(struct nb_config *candidate, struct lyd_node *dnode; struct nb_config_cbs *cfg_chgs = &candidate->cfg_chgs; int op; + bool new = false, delete = false; + struct nb_config_change *chg = NULL; + char dn_xpath[XPATH_MAXLEN]; switch (oper) { case NB_OP_CREATE: + root = yang_dnode_get(candidate->dnode, xpath); + new = true; + zlog_debug("%s: Dnode: %p, XPath: %s, Oper: CREATE", + __func__, root, xpath); + break; case NB_OP_MODIFY: root = yang_dnode_get(candidate->dnode, xpath); + zlog_debug("%s: Dnode: %p, XPath: %s, Oper: MODIFY", + __func__, root, xpath); break; case NB_OP_DESTROY: + /* + * Let's first check if the config item being requested + * to delete has been recently added to the scratch + * buffer or not. If so we need to remove from the + * scratch buffer first. + */ + root = yang_dnode_get(candidate->dnode, xpath); + zlog_debug("%s: Dnode: %p, XPath: %s, Oper: DESTROY", + __func__, root, xpath); + if (root) + chg = nb_config_find_change(cfg_chgs, root); + if (chg) { + zlog_debug("Delete dnode %p, xpath: %s from Scratch Buffer!", + root, xpath); + RB_REMOVE(nb_config_cbs, cfg_chgs, &chg->cb); + XFREE(MTYPE_TMP, chg); + } + + /* + * Next check if the config item being requested + * to delete has been previously committed to the + * running datastore or not. If so we need to add + * a delete request in the scratch buffer. + */ root = yang_dnode_get(running_config->dnode, xpath); + delete = true; /* code */ break; case NB_OP_MOVE: @@ -790,24 +850,44 @@ static void nb_update_candidate_changes(struct nb_config *candidate, return; LYD_TREE_DFS_BEGIN (root, dnode) { + lyd_path(dnode, LYD_PATH_STD, dn_xpath, + sizeof(dn_xpath)-1); op = nb_lyd_diff_get_op(dnode); switch (op) { case 'c': /* create */ - nb_config_diff_created(dnode, seq, cfg_chgs); LYD_TREE_DFS_continue = 1; break; case 'd': /* delete */ - nb_config_diff_deleted(dnode, seq, cfg_chgs); LYD_TREE_DFS_continue = 1; break; case 'r': /* replace */ - nb_config_diff_add_change(cfg_chgs, NB_OP_MODIFY, seq, - dnode); break; case 'n': /* none */ + /* + * There are no operational metadata associated. Just + * follow the original operation. + */ + LYD_TREE_DFS_continue = 1; default: break; } + + if (delete) { + zlog_debug("%s: Add dnode %p, xpath: %s to Scratch Buffer for deletion", + __func__, dnode, dn_xpath); + nb_config_diff_deleted(dnode, seq, cfg_chgs); + } + else if (new) { + zlog_debug("%s: Add dnode %p, xpath: %s to Scratch Buffer for creation", + __func__, dnode, dn_xpath); + nb_config_diff_created(dnode, seq, cfg_chgs); + } else { + zlog_debug("%s: Add dnode %p, xpath: %s to Scratch Buffer for modification", + __func__, dnode, dn_xpath); + nb_config_diff_add_change(cfg_chgs, + NB_OP_MODIFY, seq, dnode); + } + LYD_TREE_DFS_END(root, dnode); } } @@ -827,7 +907,8 @@ static bool nb_is_operation_allowed(struct nb_node *nb_node, void nb_candidate_edit_config_changes( struct nb_config *candidate_config, struct nb_cfg_change cfg_changes[], size_t num_cfg_changes, const char *xpath_base, const char *curr_xpath, - int xpath_index, char *err_buf, int err_bufsize, bool *error) + int xpath_index, bool update_scratchbuf, char *err_buf, + int err_bufsize, bool *error) { uint32_t seq = 0; @@ -884,6 +965,28 @@ void nb_candidate_edit_config_changes( data = yang_data_new(xpath, change->value); /* + * NOTE: + * It is time to update the change in Candidate datastore. But + * we also need to consider adding the change to the scratch + * buffer associated with the Candidate datastore. Adding to + * scratch buffer helps in scenarios where we have few config + * changes and a big running and candidate tree already. In + * such cases a regular diff between candidate and running + * tree takes unusually very long. + */ + + /* + * If this is a delete operation we need to remove it from + * the scratch buffer before updating the candidate node + * (if it has been added previously as part of the same + * commit). + */ + if (update_scratchbuf && change->operation == NB_OP_DESTROY) + nb_update_candidate_changes(candidate_config, change, + &seq); + + /* + * And then add the changes to the actual candidate data tree. * Ignore "not found" errors when editing the candidate * configuration. */ @@ -900,7 +1003,14 @@ void nb_candidate_edit_config_changes( *error = true; continue; } - nb_update_candidate_changes(candidate_config, change, &seq); + + /* + * And finally add all create/modify requests to the scratch + * buffer associated with the candidate data tree. + */ + if (update_scratchbuf && change->operation != NB_OP_DESTROY) + nb_update_candidate_changes(candidate_config, change, + &seq); } if (error && *error) { diff --git a/lib/northbound.h b/lib/northbound.h index 1723a87e4e75..5218453ad2dd 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -923,6 +923,10 @@ extern bool nb_candidate_needs_update(const struct nb_config *candidate); * xpath_index * Index of xpath being processed. * + * update_scratchbuf + * Update the scratch buffer associated with candidate data tree + * as well. + * * err_buf * Buffer to store human-readable error message in case of error. * @@ -935,7 +939,8 @@ extern bool nb_candidate_needs_update(const struct nb_config *candidate); extern void nb_candidate_edit_config_changes( struct nb_config *candidate_config, struct nb_cfg_change cfg_changes[], size_t num_cfg_changes, const char *xpath_base, const char *curr_xpath, - int xpath_index, char *err_buf, int err_bufsize, bool *error); + int xpath_index, bool update_scratchbuf, char *err_buf, + int err_bufsize, bool *error); /* * Delete candidate configuration changes. diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index 8003679ed57b..96df96829970 100644 --- a/lib/northbound_cli.c +++ b/lib/northbound_cli.c @@ -147,8 +147,8 @@ static int nb_cli_apply_changes_internal(struct vty *vty, nb_candidate_edit_config_changes( vty->candidate_config, vty->cfg_changes, vty->num_cfg_changes, - xpath_base, VTY_CURR_XPATH, vty->xpath_index, buf, sizeof(buf), - &error); + xpath_base, VTY_CURR_XPATH, vty->xpath_index, false, buf, + sizeof(buf), &error); if (error) { /* * Failure to edit the candidate configuration should never diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c index a0e610c7c745..701a9c75065d 100644 --- a/mgmtd/mgmt_ds.c +++ b/mgmtd/mgmt_ds.c @@ -101,6 +101,14 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src, nb_config_diff_del_changes(&src->root.cfg_root->cfg_chgs); } + if (dst->ds_id == MGMTD_DS_CANDIDATE) { + /* + * Drop the changes in scratch-buffer. + */ + MGMTD_DS_DBG("Emptying Candidate Scratch buffer!"); + nb_config_diff_del_changes(&dst->root.cfg_root->cfg_chgs); + } + return 0; } @@ -134,6 +142,14 @@ static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src, nb_config_diff_del_changes(&src->root.cfg_root->cfg_chgs); } + if (dst->ds_id == MGMTD_DS_CANDIDATE) { + /* + * Drop the changes in scratch-buffer. + */ + MGMTD_DS_DBG("Emptying Candidate Scratch buffer!"); + nb_config_diff_del_changes(&dst->root.cfg_root->cfg_chgs); + } + return 0; } diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index 53b9b7df46c2..dabbbbe80e38 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -666,7 +666,7 @@ static void mgmt_txn_process_set_cfg(struct event *thread) txn_req->req.set_cfg->cfg_changes, (size_t)txn_req->req.set_cfg ->num_cfg_changes, - NULL, NULL, 0, err_buf, + NULL, NULL, 0, true, err_buf, sizeof(err_buf), &error); if (error) { mgmt_fe_send_set_cfg_reply(txn->session_id, txn->txn_id, @@ -1199,6 +1199,7 @@ static int mgmt_txn_prepare_config(struct mgmt_txn_ctx *txn) */ cfg_chgs = &nb_config->cfg_chgs; if (RB_EMPTY(nb_config_cbs, cfg_chgs)) { + MGMTD_TXN_DBG("Candidate Scratch buffer Empty! Computing FULL Diff!"); /* * This could be the case when the config is directly * loaded onto the candidate DS from a file. Get the @@ -1211,6 +1212,8 @@ static int mgmt_txn_prepare_config(struct mgmt_txn_ctx *txn) nb_config, &changes); cfg_chgs = &changes; del_cfg_chgs = true; + } else { + MGMTD_TXN_DBG("Found changes in Candidate Scratch buffer!"); } if (RB_EMPTY(nb_config_cbs, cfg_chgs)) { @@ -2228,12 +2231,16 @@ int mgmt_txn_send_set_config_req(uint64_t txn_id, uint64_t req_id, else continue; - MGMTD_TXN_DBG("XPath: '%s', Value: '%s'", + MGMTD_TXN_DBG("XPath: '%s', Value: '%s', OPER: %s", cfg_req[indx]->data->xpath, (cfg_req[indx]->data->value && cfg_req[indx]->data->value->encoded_str_val ? cfg_req[indx]->data->value->encoded_str_val - : "NULL")); + : "NULL"), + (cfg_chg->operation == NB_OP_CREATE ? "Create" + : cfg_chg->operation == NB_OP_MODIFY ? "Modify" + : cfg_chg->operation == NB_OP_DESTROY + ? "Destroy" : "Other")); strlcpy(cfg_chg->xpath, cfg_req[indx]->data->xpath, sizeof(cfg_chg->xpath)); cfg_chg->value =