lib, mgmtd: Fix the Scratch Buffer usage in Northbound code

The changes to the candidate data trees were not getting added to
the scratch-buffer since lyd_diff_get_op was returning nothing ('n')
as no 'operational' metadata associated with the data items in
the YANG models. Fixed the logic to add them to the scratch buffer
anyways.

Also, the scratch-buffer updations were getting invoked everywhere
(mgmtd, backend clients). Limited the usage to mgmtd daemon only.

Signed-off-by: Pushpasis Sarkar <[email protected]>

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;

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;
+			break;
 		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) {

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.

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

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;
 }
 

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 =