diff --git a/lib/lib_vty.c b/lib/lib_vty.c index 91318e0d95..3e4dc9d78c 100644 --- a/lib/lib_vty.c +++ b/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) diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index 523b383c62..281d9a4704 100644 --- a/lib/northbound_cli.c +++ b/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); diff --git a/lib/vty.c b/lib/vty.c index c6d5645443..c667ca4549 100644 --- a/lib/vty.c +++ b/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; diff --git a/lib/vty.h b/lib/vty.h index 4d3eb591df..3176a52863 100644 --- a/lib/vty.h +++ b/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 } diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index cc812ab150..d45630478b 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/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( diff --git a/mgmtd/mgmt_history.c b/mgmtd/mgmt_history.c index 4fecfa9835..75def3a05e 100644 --- a/mgmtd/mgmt_history.c +++ b/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; diff --git a/mgmtd/mgmt_history.h b/mgmtd/mgmt_history.h index 23ce4062ef..29a1d7738e 100644 --- a/mgmtd/mgmt_history.h +++ b/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. */ diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index 05b593798e..3a3043421b 100644 --- a/mgmtd/mgmt_txn.c +++ b/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; }