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>

diff --git a/lib/lib_vty.c b/lib/lib_vty.c
index 91318e0d95edb49630e7847ea322669f79091ea5..3e4dc9d78ca8194b51908c031fb7bd65025fff85 100644 (file)
--- 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 523b383c629bfb5102304ff02319a47d4995e423..281d9a4704eb959328be8c970d03ad3be95ba594 100644 (file)
--- 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 c6d564544363d8f13620a075b6bb67c0169c2e89..c667ca4549f802737b558821e0ea1b50ca33d437 100644 (file)
--- 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 4d3eb591dfb769c5aef79f63c3c58aace1fe0acb..3176a52863f13d123f2dfa08e02ce8016011a0e1 100644 (file)
--- 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 cc812ab150ed931e1d53768210eb5e4e37aafcd8..d45630478b058f1733711d18832f0c22a460cbfc 100644 (file)
--- 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 4fecfa9835c7845a3eef8fef64c6b16189de2a20..75def3a05e0e02a824b2c964af40a6b50b18c2bd 100644 (file)
--- 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 23ce4062ef3afc813b0165b4aa8800df2edc371a..29a1d7738ea9a6357b2166123a82d1c398618338 100644 (file)
--- 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 05b593798e43bfc9d34d478a2fd2090a51fa93bc..3a3043421bef8e368c8bcd3e8e892801a78959c4 100644 (file)
--- 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;
        }