lib, mgmtd: Add few fixes for commit-check and rollback

This commit contains fixes for the following issues found
- 'mgmt commit check' issued through 'vtysh -f' was actually commtting the changeset.
- On config validation failure backend, mgmtd was not passing the correct error-reason
  to frontend.
- 'mgmt rollback ...' was reverting the change on backend, but config on mgmtd daemon
  remains intact

Signed-off-by: Pushpasis Sarkar <pushpasis@gmail.com>

lib/lib_vty.c

@@ -242,8 +242,16 @@ DEFUN_NOSH(end_config, end_config_cmd, "XFRR_end_configuration",
 	ret = nb_cli_pending_commit_check(vty);
 
 	zlog_info("Configuration Read in Took: %s", readin_time_str);
+	zlog_debug("%s: VTY:%p, pending SET-CFG: %u",
+		   __func__, vty, (uint32_t)vty->mgmt_num_pending_setcfg);
 
-	if (vty_mgmt_fe_enabled())
+	/*
+	 * If (and only if) we have sent any CLI config commands to MGMTd
+	 * FE interface using vty_mgmt_send_config_data() without implicit
+	 * commit before, should we need to send an explicit COMMIT-REQ now
+	 * to apply all those commands at once.
+	 */
+	if (vty->mgmt_num_pending_setcfg && vty_mgmt_fe_enabled())
 		vty_mgmt_send_commit_config(vty, false, false);
 
 	if (callback.end_config)

lib/northbound_cli.c

@@ -183,6 +183,8 @@ static int nb_cli_apply_changes_internal(struct vty *vty,
 int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
 {
 	char xpath_base[XPATH_MAXLEN] = {};
+	bool implicit_commit;
+	int ret;
 
 	/* Parse the base XPath format string. */
 	if (xpath_base_fmt) {
@@ -195,7 +197,12 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
 
 	if (vty_mgmt_fe_enabled()) {
 		VTY_CHECK_XPATH;
-		return vty_mgmt_send_config_data(vty);
+
+		implicit_commit = vty_needs_implicit_commit(vty);
+		ret = vty_mgmt_send_config_data(vty);
+		if (ret >= 0 && !implicit_commit)
+			vty->mgmt_num_pending_setcfg++;
+		return ret;
 	}
 
 	return nb_cli_apply_changes_internal(vty, xpath_base, false);
@@ -205,6 +212,8 @@ int nb_cli_apply_changes_clear_pending(struct vty *vty,
 				       const char *xpath_base_fmt, ...)
 {
 	char xpath_base[XPATH_MAXLEN] = {};
+	bool implicit_commit;
+	int ret;
 
 	/* Parse the base XPath format string. */
 	if (xpath_base_fmt) {
@@ -217,7 +226,12 @@ int nb_cli_apply_changes_clear_pending(struct vty *vty,
 
 	if (vty_mgmt_fe_enabled()) {
 		VTY_CHECK_XPATH;
-		return vty_mgmt_send_config_data(vty);
+
+		implicit_commit = vty_needs_implicit_commit(vty);
+		ret = vty_mgmt_send_config_data(vty);
+		if (ret >= 0 && !implicit_commit)
+			vty->mgmt_num_pending_setcfg++;
+		return ret;
 	}
 
 	return nb_cli_apply_changes_internal(vty, xpath_base, true);

lib/vty.c

@@ -121,7 +121,7 @@ static char integrate_default[] = SYSCONFDIR INTEGRATE_DEFAULT_CONFIG;
 static bool do_log_commands;
 static bool do_log_commands_perm;
 
-static void vty_mgmt_resume_response(struct vty *vty, bool success)
+void vty_mgmt_resume_response(struct vty *vty, bool success)
 {
 	uint8_t header[4] = {0, 0, 0, 0};
 	int ret = CMD_SUCCESS;
@@ -3496,6 +3496,7 @@ int vty_mgmt_send_config_data(struct vty *vty)
 	Mgmtd__YangCfgDataReq * cfgreq[VTY_MAXCFGCHANGES] = {0};
 	size_t indx;
 	int cnt;
+	bool implicit_commit = false;
 
 	if (mgmt_lib_hndl && vty->mgmt_session_id) {
 		cnt = 0;
@@ -3545,19 +3546,13 @@ int vty_mgmt_send_config_data(struct vty *vty)
 		}
 
 		vty->mgmt_req_id++;
+		implicit_commit = vty_needs_implicit_commit(vty);
 		if (cnt
 		    && mgmt_fe_set_config_data(
-			       mgmt_lib_hndl, vty->mgmt_session_id,
-			       vty->mgmt_req_id, MGMTD_DS_CANDIDATE, cfgreq,
-			       cnt,
-			       frr_get_cli_mode() == FRR_CLI_CLASSIC
-				       ? ((vty->pending_allowed
-					   || vty->no_implicit_commit)
-						  ? false
-						  : true)
-				       : false,
-			       MGMTD_DS_RUNNING)
-			       != MGMTD_SUCCESS) {
+				mgmt_lib_hndl, vty->mgmt_session_id,
+				vty->mgmt_req_id, MGMTD_DS_CANDIDATE, cfgreq,
+				cnt, implicit_commit, MGMTD_DS_RUNNING)
+		    != MGMTD_SUCCESS) {
 			zlog_err("Failed to send %d Config Xpaths to MGMTD!!",
 				 (int)indx);
 			return -1;
@@ -3588,6 +3583,7 @@ int vty_mgmt_send_commit_config(struct vty *vty, bool validate_only, bool abort)
 		}
 
 		vty->mgmt_req_pending = true;
+		vty->mgmt_num_pending_setcfg = 0;
 	}
 
 	return 0;

lib/vty.h

@@ -25,6 +25,7 @@
 #include "compiler.h"
 #include "northbound.h"
 #include "zlog_live.h"
+#include "libfrr.h"
 #include "mgmt_fe_client.h"
 
 #ifdef __cplusplus
@@ -120,6 +121,12 @@ struct vty {
 	int xpath_index;
 	char xpath[VTY_MAXDEPTH][XPATH_MAXLEN];
 
+	/*
+	 * Keep track of how many SET_CFG requests has been sent so far that
+	 * has not been committed yet.
+	 */
+	size_t mgmt_num_pending_setcfg;
+
 	/* In configure mode. */
 	bool config;
 
@@ -391,6 +398,17 @@ extern int vty_mgmt_send_get_data(struct vty *vty, Mgmtd__DatastoreId datastore,
 				  const char **xpath_list, int num_req);
 extern int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id,
 				    bool lock);
+extern void vty_mgmt_resume_response(struct vty *vty, bool success);
+
+static inline bool vty_needs_implicit_commit(struct vty *vty)
+{
+	return (frr_get_cli_mode() == FRR_CLI_CLASSIC
+		? ((vty->pending_allowed
+			|| vty->no_implicit_commit)
+				? false
+				: true)
+		: false);
+}
 
 #ifdef __cplusplus
 }

mgmtd/mgmt_fe_adapter.c

@@ -1333,8 +1333,9 @@ mgmt_fe_adapter_handle_msg(struct mgmt_fe_client_adapter *adapter,
 				fe_msg->setcfg_req->session_id);
 		session->adapter->setcfg_stats.set_cfg_count++;
 		MGMTD_FE_ADAPTER_DBG(
-			"Got Set Config Req Msg (%d Xpaths) on DS:%d for session-id %llu from '%s'",
+			"Got Set Config Req Msg (%d Xpaths, Implicit:%c) on DS:%d for session-id %llu from '%s'",
 			(int)fe_msg->setcfg_req->n_data,
+			fe_msg->setcfg_req->implicit_commit ? 'T':'F',
 			fe_msg->setcfg_req->ds_id,
 			(unsigned long long)fe_msg->setcfg_req->session_id,
 			adapter->name);
@@ -1346,9 +1347,10 @@ mgmt_fe_adapter_handle_msg(struct mgmt_fe_client_adapter *adapter,
 		session = mgmt_session_id2ctx(
 				fe_msg->commcfg_req->session_id);
 		MGMTD_FE_ADAPTER_DBG(
-			"Got Commit Config Req Msg for src-DS:%d dst-DS:%d on session-id %llu from '%s'",
+			"Got Commit Config Req Msg for src-DS:%d dst-DS:%d (Abort:%c) on session-id %llu from '%s'",
 			fe_msg->commcfg_req->src_ds_id,
 			fe_msg->commcfg_req->dst_ds_id,
+			fe_msg->commcfg_req->abort ? 'T':'F',
 			(unsigned long long)fe_msg->commcfg_req->session_id,
 			adapter->name);
 		mgmt_fe_session_handle_commit_config_req_msg(

mgmtd/mgmt_history.c

@@ -29,7 +29,11 @@ DECLARE_DLIST(mgmt_cmt_infos, struct mgmt_cmt_info_t, cmts);
 #define FOREACH_CMT_REC(mm, cmt_info)                                          \
 	frr_each_safe (mgmt_cmt_infos, &mm->cmts, cmt_info)
 
-
+/*
+ * The only instance of VTY session that has triggered an ongoing
+ * config rollback operation.
+ */
+static struct vty *rollback_vty = NULL;
 
 static bool mgmt_history_record_exists(char *file_path)
 {
@@ -194,6 +198,11 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty,
 	struct mgmt_ds_ctx *dst_ds_ctx;
 	int ret = 0;
 
+	if (rollback_vty) {
+		vty_out(vty, "ERROR: Rollback already in progress!\n");
+		return -1;
+	}
+
 	src_ds_ctx = mgmt_ds_get_ctx_by_id(mm, MGMTD_DS_CANDIDATE);
 	if (!src_ds_ctx) {
 		vty_out(vty, "ERROR: Couldnot access Candidate datastore!\n");
@@ -241,9 +250,23 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty,
 	}
 
 	mgmt_history_dump_cmt_record_index();
+
+	/*
+	 * Block the rollback command from returning till the rollback
+	 * is completed. On rollback completion mgmt_history_rollback_complete()
+	 * shall be called to resume the rollback command return to VTYSH.
+	 */
+	vty->mgmt_req_pending = true;
+	rollback_vty = vty;
 	return 0;
 }
 
+void mgmt_history_rollback_complete(bool success)
+{
+	vty_mgmt_resume_response(rollback_vty, success);
+	rollback_vty = NULL;
+}
+
 int mgmt_history_rollback_by_id(struct vty *vty, const char *cmtid_str)
 {
 	int ret = 0;

mgmtd/mgmt_history.h

@@ -42,6 +42,8 @@ extern int mgmt_history_rollback_by_id(struct vty *vty, const char *cmtid_str);
  */
 extern int mgmt_history_rollback_n(struct vty *vty, int num_cmts);
 
+extern void mgmt_history_rollback_complete(bool success);
+
 /*
  * Show mgmt commit history.
  */

mgmtd/mgmt_txn.c

@@ -832,6 +832,11 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
 			MGMTD_TXN_ERR(
 				"Failed to unlock the dst DS during rollback : %s",
 				strerror(ret));
+
+		/*
+		 * Resume processing the rollback command.
+		 */
+		mgmt_history_rollback_complete(success);
 	}
 
 	if (txn->commit_cfg_req->req.commit_cfg.implicit)
@@ -2592,6 +2597,7 @@ int mgmt_txn_notify_be_cfgdata_reply(
 			error_if_any ? error_if_any : "None");
 		mgmt_txn_send_commit_cfg_reply(
 			txn, MGMTD_INTERNAL_ERROR,
+			error_if_any ? error_if_any :
 			"Internal error! Failed to download config data to backend!");
 		return 0;
 	}
@@ -2635,6 +2641,7 @@ int mgmt_txn_notify_be_cfg_apply_reply(uint64_t txn_id, bool success,
 			error_if_any ? error_if_any : "None");
 		mgmt_txn_send_commit_cfg_reply(
 			txn, MGMTD_INTERNAL_ERROR,
+			error_if_any ? error_if_any :
 			"Internal error! Failed to apply config data on backend!");
 		return 0;
 	}