diff --git a/lib/command.c b/lib/command.c index 0995637219..8025ab534f 100644 --- a/lib/command.c +++ b/lib/command.c @@ -1333,11 +1333,12 @@ int config_from_file(struct vty *vty, FILE *fp, unsigned int *line_num) /* Configuration from terminal */ DEFUN (config_terminal, config_terminal_cmd, - "configure [terminal]", + "configure [terminal [file-lock]]", "Configuration from vty interface\n" + "Configuration with locked datastores\n" "Configuration terminal\n") { - return vty_config_enter(vty, false, false); + return vty_config_enter(vty, false, false, argc == 3); } /* Enable command */ diff --git a/lib/mgmt.proto b/lib/mgmt.proto index 8a11ff0fa5..ac44eefd9e 100644 --- a/lib/mgmt.proto +++ b/lib/mgmt.proto @@ -243,7 +243,8 @@ message FeSetConfigReply { required DatastoreId ds_id = 2; required uint64 req_id = 3; required bool success = 4; - optional string error_if_any = 5; + required bool implicit_commit = 5; + optional string error_if_any = 6; } message FeCommitConfigReq { diff --git a/lib/mgmt_fe_client.c b/lib/mgmt_fe_client.c index 22d085cbce..45d57175d6 100644 --- a/lib/mgmt_fe_client.c +++ b/lib/mgmt_fe_client.c @@ -411,6 +411,7 @@ static int mgmt_fe_client_handle_msg(struct mgmt_fe_client *client, session->user_ctx, fe_msg->setcfg_reply->req_id, fe_msg->setcfg_reply->success, fe_msg->setcfg_reply->ds_id, + fe_msg->setcfg_reply->implicit_commit, fe_msg->setcfg_reply->error_if_any); break; case MGMTD__FE_MESSAGE__MESSAGE_COMMCFG_REPLY: diff --git a/lib/mgmt_fe_client.h b/lib/mgmt_fe_client.h index e3c016f9b6..532fee4397 100644 --- a/lib/mgmt_fe_client.h +++ b/lib/mgmt_fe_client.h @@ -91,7 +91,7 @@ struct mgmt_fe_client_cbs { uintptr_t session_id, uintptr_t user_session_client, uint64_t req_id, bool success, - Mgmtd__DatastoreId ds_id, + Mgmtd__DatastoreId ds_id, bool implcit_commit, char *errmsg_if_any); void (*commit_config_notify)(struct mgmt_fe_client *client, diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index 9d6ec66689..8003679ed5 100644 --- a/lib/northbound_cli.c +++ b/lib/northbound_cli.c @@ -763,7 +763,7 @@ DEFUN (config_exclusive, "Configuration from vty interface\n" "Configure exclusively from this terminal\n") { - return vty_config_enter(vty, true, true); + return vty_config_enter(vty, true, true, false); } /* Configure using a private candidate configuration. */ @@ -773,7 +773,7 @@ DEFUN (config_private, "Configuration from vty interface\n" "Configure using a private candidate configuration\n") { - return vty_config_enter(vty, true, false); + return vty_config_enter(vty, true, false, false); } DEFPY (config_commit, diff --git a/lib/vty.c b/lib/vty.c index c1f4d7f47d..fc6bed6a0a 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -1676,12 +1676,12 @@ struct vty *vty_new(void) if (!mgmt_client_id_next) mgmt_client_id_next++; new->mgmt_client_id = mgmt_client_id_next++; - if (mgmt_fe_create_client_session( - mgmt_fe_client, new->mgmt_client_id, - (uintptr_t) new) != MGMTD_SUCCESS) - zlog_err( - "Failed to open a MGMTD Frontend session for VTY session %p!!", - new); + new->mgmt_session_id = 0; + mgmt_fe_create_client_session( + mgmt_fe_client, new->mgmt_client_id, (uintptr_t) new); + /* we short-circuit create the session so it must be set now */ + assertf(new->mgmt_session_id != 0, + "Failed to create client session for VTY"); } return new; @@ -2233,6 +2233,9 @@ bool mgmt_vty_read_configs(void) vty->candidate_config = vty_shared_candidate_config; + vty_mgmt_lock_candidate_inline(vty); + vty_mgmt_lock_running_inline(vty); + for (index = 0; index < array_size(mgmt_daemons); index++) { snprintf(path, sizeof(path), "%s/%s.conf", frr_sysconfdir, mgmt_daemons[index]); @@ -2276,6 +2279,14 @@ bool mgmt_vty_read_configs(void) fclose(confp); } + /* Conditionally unlock as the config file may have "exit"d early which + * would then have unlocked things. + */ + if (vty->mgmt_locked_running_ds) + vty_mgmt_unlock_running_inline(vty); + if (vty->mgmt_locked_candidate_ds) + vty_mgmt_unlock_candidate_inline(vty); + vty->pending_allowed = false; if (!count) @@ -2844,9 +2855,11 @@ bool vty_read_config(struct nb_config *config, const char *config_file, return true; } -int vty_config_enter(struct vty *vty, bool private_config, bool exclusive) +int vty_config_enter(struct vty *vty, bool private_config, bool exclusive, + bool file_lock) { - if (exclusive && nb_running_lock(NB_CLIENT_CLI, vty)) { + if (exclusive && !vty_mgmt_fe_enabled() && + nb_running_lock(NB_CLIENT_CLI, vty)) { vty_out(vty, "%% Configuration is locked by other client\n"); return CMD_WARNING; } @@ -2857,20 +2870,20 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive) * message. For user interactive mode we are doing implicit commits * those will obtain the lock (or not) when they try and commit. */ - if (vty_mgmt_fe_enabled() && vty->pending_allowed && !private_config) { - /* - * lock using short-circuit, we set the locked boolean to true - * here so that it can be flipped to false by our locked_notify - * handler during the synchronous call. - */ - vty->mgmt_locked_candidate_ds = true; - if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, true, - true) || - !vty->mgmt_locked_candidate_ds) { + if (file_lock && vty_mgmt_fe_enabled() && !private_config) { + if (vty_mgmt_lock_candidate_inline(vty)) { vty_out(vty, "%% Can't enter config; candidate datastore locked by another session\n"); return CMD_WARNING_CONFIG_FAILED; } + if (vty_mgmt_lock_running_inline(vty)) { + vty_out(vty, + "%% Can't enter config; running datastore locked by another session\n"); + vty_mgmt_unlock_candidate_inline(vty); + return CMD_WARNING_CONFIG_FAILED; + } + assert(vty->mgmt_locked_candidate_ds); + assert(vty->mgmt_locked_running_ds); } vty->node = CONFIG_NODE; @@ -2923,18 +2936,15 @@ void vty_config_exit(struct vty *vty) int vty_config_node_exit(struct vty *vty) { - int ret; - vty->xpath_index = 0; - if (vty->mgmt_locked_candidate_ds) { - assert(vty->type != VTY_FILE); - /* use short-circuit call to immediately unlock */ - ret = vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, false, - true); - assert(!ret); - vty->mgmt_locked_candidate_ds = false; - } + /* TODO: could we check for un-commited changes here? */ + + if (vty->mgmt_locked_running_ds) + vty_mgmt_unlock_running_inline(vty); + + if (vty->mgmt_locked_candidate_ds) + vty_mgmt_unlock_candidate_inline(vty); /* Perform any pending commits. */ (void)nb_cli_pending_commit_check(vty); @@ -3537,7 +3547,8 @@ static void vty_mgmt_ds_lock_notified(struct mgmt_fe_client *client, static void vty_mgmt_set_config_result_notified( struct mgmt_fe_client *client, uintptr_t usr_data, uint64_t client_id, uintptr_t session_id, uintptr_t session_ctx, uint64_t req_id, - bool success, Mgmtd__DatastoreId ds_id, char *errmsg_if_any) + bool success, Mgmtd__DatastoreId ds_id, bool implicit_commit, + char *errmsg_if_any) { struct vty *vty; @@ -3555,6 +3566,12 @@ static void vty_mgmt_set_config_result_notified( client_id, req_id); } + if (implicit_commit) { + /* In this case the changes have been applied, we are done */ + vty_mgmt_unlock_candidate_inline(vty); + vty_mgmt_unlock_running_inline(vty); + } + vty_mgmt_resume_response(vty, success); } @@ -3688,7 +3705,6 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit) Mgmtd__YangCfgDataReq cfg_req[VTY_MAXCFGCHANGES]; Mgmtd__YangCfgDataReq *cfgreq[VTY_MAXCFGCHANGES] = {0}; size_t indx; - int cnt; if (vty->type == VTY_FILE) { /* @@ -3701,80 +3717,87 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit) return 0; } + /* If we are FE client and we have a vty then we have a session */ + assert(mgmt_fe_client && vty->mgmt_client_id && vty->mgmt_session_id); - if (mgmt_fe_client && vty->mgmt_client_id && !vty->mgmt_session_id) { - /* - * We are connected to mgmtd but we do not yet have an - * established session. this means we need to send any changes - * made during this "down-time" to all backend clients when this - * FE client finishes coming up. - */ - MGMTD_FE_CLIENT_DBG("skipping as no session exists"); + if (!vty->num_cfg_changes) return 0; - } - if (mgmt_fe_client && vty->mgmt_session_id) { - cnt = 0; - for (indx = 0; indx < vty->num_cfg_changes; indx++) { - mgmt_yang_data_init(&cfg_data[cnt]); - - if (vty->cfg_changes[indx].value) { - mgmt_yang_data_value_init(&value[cnt]); - value[cnt].encoded_str_val = - (char *)vty->cfg_changes[indx].value; - value[cnt].value_case = - MGMTD__YANG_DATA_VALUE__VALUE_ENCODED_STR_VAL; - cfg_data[cnt].value = &value[cnt]; - } - - cfg_data[cnt].xpath = vty->cfg_changes[indx].xpath; - - mgmt_yang_cfg_data_req_init(&cfg_req[cnt]); - cfg_req[cnt].data = &cfg_data[cnt]; - switch (vty->cfg_changes[indx].operation) { - case NB_OP_DESTROY: - cfg_req[cnt].req_type = - MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA; - break; - - case NB_OP_CREATE: - case NB_OP_MODIFY: - case NB_OP_MOVE: - case NB_OP_PRE_VALIDATE: - case NB_OP_APPLY_FINISH: - cfg_req[cnt].req_type = - MGMTD__CFG_DATA_REQ_TYPE__SET_DATA; - break; - case NB_OP_GET_ELEM: - case NB_OP_GET_NEXT: - case NB_OP_GET_KEYS: - case NB_OP_LOOKUP_ENTRY: - case NB_OP_RPC: - assert(!"Invalid type of operation"); - break; - default: - assert(!"non-enum value, invalid"); - } - - cfgreq[cnt] = &cfg_req[cnt]; - cnt++; - } - - vty->mgmt_req_id++; - if (cnt && mgmt_fe_send_setcfg_req( - mgmt_fe_client, 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); - vty_out(vty, "Failed to send SETCFG-REQ to MGMTD!\n"); + /* grab the candidate and running lock prior to sending implicit commit + * command + */ + if (implicit_commit) { + if (vty_mgmt_lock_candidate_inline(vty)) { + vty_out(vty, + "%% command failed, could not lock candidate DS\n"); + return -1; + } else if (vty_mgmt_lock_running_inline(vty)) { + vty_out(vty, + "%% command failed, could not lock running DS\n"); return -1; } - - vty->mgmt_req_pending_cmd = "MESSAGE_SETCFG_REQ"; } + for (indx = 0; indx < vty->num_cfg_changes; indx++) { + mgmt_yang_data_init(&cfg_data[indx]); + + if (vty->cfg_changes[indx].value) { + mgmt_yang_data_value_init(&value[indx]); + value[indx].encoded_str_val = + (char *)vty->cfg_changes[indx].value; + value[indx].value_case = + MGMTD__YANG_DATA_VALUE__VALUE_ENCODED_STR_VAL; + cfg_data[indx].value = &value[indx]; + } + + cfg_data[indx].xpath = vty->cfg_changes[indx].xpath; + + mgmt_yang_cfg_data_req_init(&cfg_req[indx]); + cfg_req[indx].data = &cfg_data[indx]; + switch (vty->cfg_changes[indx].operation) { + case NB_OP_DESTROY: + cfg_req[indx].req_type = + MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA; + break; + + case NB_OP_CREATE: + case NB_OP_MODIFY: + case NB_OP_MOVE: + case NB_OP_PRE_VALIDATE: + case NB_OP_APPLY_FINISH: + cfg_req[indx].req_type = + MGMTD__CFG_DATA_REQ_TYPE__SET_DATA; + break; + case NB_OP_GET_ELEM: + case NB_OP_GET_NEXT: + case NB_OP_GET_KEYS: + case NB_OP_LOOKUP_ENTRY: + case NB_OP_RPC: + default: + assertf(false, + "Invalid operation type for send config: %d", + vty->cfg_changes[indx].operation); + /*NOTREACHED*/ + abort(); + } + + cfgreq[indx] = &cfg_req[indx]; + } + if (!indx) + return 0; + + vty->mgmt_req_id++; + if (mgmt_fe_send_setcfg_req(mgmt_fe_client, vty->mgmt_session_id, + vty->mgmt_req_id, MGMTD_DS_CANDIDATE, + cfgreq, indx, implicit_commit, + MGMTD_DS_RUNNING)) { + zlog_err("Failed to send %zu config xpaths to mgmtd", indx); + vty_out(vty, "%% Failed to send commands to mgmtd\n"); + return -1; + } + + vty->mgmt_req_pending_cmd = "MESSAGE_SETCFG_REQ"; + return 0; } diff --git a/lib/vty.h b/lib/vty.h index 7e27b52fe1..8fb1483e5b 100644 --- a/lib/vty.h +++ b/lib/vty.h @@ -391,7 +391,7 @@ extern void vty_close(struct vty *); extern char *vty_get_cwd(void); extern void vty_update_xpath(const char *oldpath, const char *newpath); extern int vty_config_enter(struct vty *vty, bool private_config, - bool exclusive); + bool exclusive, bool file_lock); extern void vty_config_exit(struct vty *); extern int vty_config_node_exit(struct vty *); extern int vty_shell(struct vty *); diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index 1b6f45a05d..70c08d5cb4 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -41,7 +41,6 @@ struct mgmt_fe_session_ctx { uint64_t txn_id; uint64_t cfg_txn_id; uint8_t ds_locked[MGMTD_DS_MAX_ID]; - uint8_t ds_locked_implict[MGMTD_DS_MAX_ID]; struct event *proc_cfg_txn_clnp; struct event *proc_show_txn_clnp; @@ -105,7 +104,6 @@ static void mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id, session->session_id, mgmt_ds_id2name(ds_id)); session->ds_locked[ds_id] = false; - session->ds_locked_implict[ds_id] = false; mgmt_ds_unlock(ds_ctx); MGMTD_FE_ADAPTER_DBG( "Unlocked DS:%s write-locked earlier by session-id: %" PRIu64 @@ -117,21 +115,12 @@ static void mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id, static void mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session) { - Mgmtd__DatastoreId ds_id; - struct mgmt_ds_ctx *ds_ctx; - /* * Ensure any uncommitted changes in Candidate DS * is discarded. */ mgmt_ds_copy_dss(mm->running_ds, mm->candidate_ds, false); - for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) { - ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id); - if (ds_ctx && session->ds_locked_implict[ds_id]) - mgmt_fe_session_unlock_ds(ds_id, ds_ctx, session); - } - /* * Destroy the actual transaction created earlier. */ @@ -374,6 +363,7 @@ static int mgmt_fe_send_setcfg_reply(struct mgmt_fe_session_ctx *session, setcfg_reply.ds_id = ds_id; setcfg_reply.req_id = req_id; setcfg_reply.success = success; + setcfg_reply.implicit_commit = implicit_commit; if (error_if_any) setcfg_reply.error_if_any = (char *)error_if_any; @@ -612,8 +602,7 @@ static int mgmt_fe_adapter_notify_disconnect(struct msg_conn *conn) } /* - * XXX chopps: get rid of this, we should have deleted sessions when there was a - * disconnect + * Purge any old connections that share the same client name with `adapter` */ static void mgmt_fe_adapter_cleanup_old_conn(struct mgmt_fe_client_adapter *adapter) @@ -621,17 +610,16 @@ mgmt_fe_adapter_cleanup_old_conn(struct mgmt_fe_client_adapter *adapter) struct mgmt_fe_client_adapter *old; FOREACH_ADAPTER_IN_LIST (old) { - if (old != adapter && - !strncmp(adapter->name, old->name, sizeof(adapter->name))) { - /* - * We have a Zombie lingering around - */ - MGMTD_FE_ADAPTER_DBG( - "Client '%s' (FD:%d) seems to have reconnected. Removing old connection (FD:%d)!", - adapter->name, adapter->conn->fd, - old->conn->fd); - msg_conn_disconnect(old->conn, false); - } + if (old == adapter) + continue; + if (strncmp(adapter->name, old->name, sizeof(adapter->name))) + continue; + + MGMTD_FE_ADAPTER_DBG( + "Client '%s' (FD:%d) seems to have reconnected. Removing old connection (FD:%d)", + adapter->name, adapter->conn->fd, + old->conn->fd); + msg_conn_disconnect(old->conn, false); } } @@ -641,16 +629,12 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session, { struct mgmt_ds_ctx *ds_ctx; - /* - * Next check first if the SETCFG_REQ is for Candidate DS - * or not. Report failure if its not. MGMTD currently only - * supports editing the Candidate DS. - */ - if (lockds_req->ds_id != MGMTD_DS_CANDIDATE) { + if (lockds_req->ds_id != MGMTD_DS_CANDIDATE && + lockds_req->ds_id != MGMTD_DS_RUNNING) { mgmt_fe_send_lockds_reply( session, lockds_req->ds_id, lockds_req->req_id, lockds_req->lock, false, - "Lock/Unlock on datastores other than Candidate DS not permitted!"); + "Lock/Unlock on DS other than candidate or running DS not supported"); return -1; } @@ -673,8 +657,6 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session, "Lock already taken on DS by another session!"); return -1; } - - session->ds_locked_implict[lockds_req->ds_id] = false; } else { if (!session->ds_locked[lockds_req->ds_id]) { mgmt_fe_send_lockds_reply( @@ -710,79 +692,49 @@ static int mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, Mgmtd__FeSetConfigReq *setcfg_req) { - uint64_t cfg_session_id; - struct mgmt_ds_ctx *ds_ctx, *dst_ds_ctx; + struct mgmt_ds_ctx *ds_ctx, *dst_ds_ctx = NULL; + bool txn_created = false; if (mm->perf_stats_en) gettimeofday(&session->adapter->setcfg_stats.last_start, NULL); - /* - * Next check first if the SETCFG_REQ is for Candidate DS - * or not. Report failure if its not. MGMTD currently only - * supports editing the Candidate DS. - */ + /* MGMTD currently only supports editing the candidate DS. */ if (setcfg_req->ds_id != MGMTD_DS_CANDIDATE) { mgmt_fe_send_setcfg_reply( session, setcfg_req->ds_id, setcfg_req->req_id, false, - "Set-Config on datastores other than Candidate DS not permitted!", + "Set-Config on datastores other than Candidate DS not supported", setcfg_req->implicit_commit); return 0; } - - /* - * Get the DS handle. - */ ds_ctx = mgmt_ds_get_ctx_by_id(mm, setcfg_req->ds_id); - if (!ds_ctx) { + assert(ds_ctx); + + /* MGMTD currently only supports targetting the running DS. */ + if (setcfg_req->implicit_commit && + setcfg_req->commit_ds_id != MGMTD_DS_RUNNING) { mgmt_fe_send_setcfg_reply( session, setcfg_req->ds_id, setcfg_req->req_id, false, - "No such DS exists!", setcfg_req->implicit_commit); + "Implicit commit on datastores other than running DS not supported", + setcfg_req->implicit_commit); + return 0; + } + dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, setcfg_req->commit_ds_id); + assert(dst_ds_ctx); + + /* User should have write lock to change the DS */ + if (!session->ds_locked[setcfg_req->ds_id]) { + mgmt_fe_send_setcfg_reply(session, setcfg_req->ds_id, + setcfg_req->req_id, false, + "Candidate DS is not locked", + setcfg_req->implicit_commit); return 0; } if (session->cfg_txn_id == MGMTD_TXN_ID_NONE) { - /* - * Check first if the current session can run a CONFIG - * transaction or not. Report failure if a CONFIG transaction - * from another session is already in progress. - */ - cfg_session_id = mgmt_config_txn_in_progress(); - if (cfg_session_id != MGMTD_SESSION_ID_NONE) { - assert(cfg_session_id != session->session_id); - mgmt_fe_send_setcfg_reply( - session, setcfg_req->ds_id, setcfg_req->req_id, - false, - "Configuration already in-progress through a different user session!", - setcfg_req->implicit_commit); - goto mgmt_fe_sess_handle_setcfg_req_failed; - } + /* as we have the lock no-one else should have a config txn */ + assert(mgmt_config_txn_in_progress() == MGMTD_SESSION_ID_NONE); - - /* - * Try taking write-lock on the requested DS (if not already). - */ - if (!session->ds_locked[setcfg_req->ds_id]) { - MGMTD_FE_ADAPTER_ERR( - "SETCFG_REQ on session-id: %" PRIu64 - " without obtaining lock", - session->session_id); - if (mgmt_fe_session_write_lock_ds(setcfg_req->ds_id, - ds_ctx, session) - != 0) { - mgmt_fe_send_setcfg_reply( - session, setcfg_req->ds_id, - setcfg_req->req_id, false, - "Failed to lock the target DS", - setcfg_req->implicit_commit); - goto mgmt_fe_sess_handle_setcfg_req_failed; - } - - session->ds_locked_implict[setcfg_req->ds_id] = true; - } - - /* - * Start a CONFIG Transaction (if not started already) - */ + /* Start a CONFIG Transaction (if not started already) */ session->cfg_txn_id = mgmt_create_txn(session->session_id, MGMTD_TXN_TYPE_CONFIG); if (session->cfg_txn_id == MGMTD_SESSION_ID_NONE) { @@ -791,14 +743,15 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, false, "Failed to create a Configuration session!", setcfg_req->implicit_commit); - goto mgmt_fe_sess_handle_setcfg_req_failed; + return 0; } + txn_created = true; MGMTD_FE_ADAPTER_DBG("Created new Config txn-id: %" PRIu64 " for session-id %" PRIu64, session->cfg_txn_id, session->session_id); } else { - MGMTD_FE_ADAPTER_ERR("Config txn-id: %" PRIu64 + MGMTD_FE_ADAPTER_DBG("Config txn-id: %" PRIu64 " for session-id: %" PRIu64 " already created", session->cfg_txn_id, session->session_id); @@ -817,22 +770,7 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, } } - dst_ds_ctx = 0; - if (setcfg_req->implicit_commit) { - dst_ds_ctx = - mgmt_ds_get_ctx_by_id(mm, setcfg_req->commit_ds_id); - if (!dst_ds_ctx) { - mgmt_fe_send_setcfg_reply( - session, setcfg_req->ds_id, setcfg_req->req_id, - false, "No such commit DS exists!", - setcfg_req->implicit_commit); - return 0; - } - } - - /* - * Create the SETConfig request under the transaction. - */ + /* Create the SETConfig request under the transaction. */ if (mgmt_txn_send_set_config_req( session->cfg_txn_id, setcfg_req->req_id, setcfg_req->ds_id, ds_ctx, setcfg_req->data, setcfg_req->n_data, @@ -843,22 +781,13 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, session, setcfg_req->ds_id, setcfg_req->req_id, false, "Request processing for SET-CONFIG failed!", setcfg_req->implicit_commit); - goto mgmt_fe_sess_handle_setcfg_req_failed; + + /* delete transaction if we just created it */ + if (txn_created) + mgmt_destroy_txn(&session->cfg_txn_id); } return 0; - -mgmt_fe_sess_handle_setcfg_req_failed: - - /* - * Delete transaction created recently. - */ - if (session->cfg_txn_id != MGMTD_TXN_ID_NONE) - mgmt_destroy_txn(&session->cfg_txn_id); - if (ds_ctx && session->ds_locked[setcfg_req->ds_id]) - mgmt_fe_session_unlock_ds(setcfg_req->ds_id, ds_ctx, session); - - return 0; } static int @@ -907,7 +836,6 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, " for session-id: %" PRIu64, session->txn_id, session->session_id); } else { - /* XXX chopps: Why would we already have a TXN here? */ MGMTD_FE_ADAPTER_DBG("Show txn-id: %" PRIu64 " for session-id: %" PRIu64 " already created", @@ -1029,43 +957,30 @@ static int mgmt_fe_session_handle_commit_config_req_msg( if (mm->perf_stats_en) gettimeofday(&session->adapter->cmt_stats.last_start, NULL); session->adapter->cmt_stats.commit_cnt++; - /* - * Get the source DS handle. - */ + + /* Validate source and dest DS */ + if (commcfg_req->src_ds_id != MGMTD_DS_CANDIDATE || + commcfg_req->dst_ds_id != MGMTD_DS_RUNNING) { + mgmt_fe_send_commitcfg_reply( + session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id, + commcfg_req->req_id, MGMTD_INTERNAL_ERROR, + commcfg_req->validate_only, + "Source/Dest for commit must be candidate/running DS"); + return 0; + } src_ds_ctx = mgmt_ds_get_ctx_by_id(mm, commcfg_req->src_ds_id); - if (!src_ds_ctx) { - mgmt_fe_send_commitcfg_reply( - session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id, - commcfg_req->req_id, MGMTD_INTERNAL_ERROR, - commcfg_req->validate_only, - "No such source DS exists!"); - return 0; - } - - /* - * Get the destination DS handle. - */ + assert(src_ds_ctx); dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, commcfg_req->dst_ds_id); - if (!dst_ds_ctx) { - mgmt_fe_send_commitcfg_reply( - session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id, - commcfg_req->req_id, MGMTD_INTERNAL_ERROR, - commcfg_req->validate_only, - "No such destination DS exists!"); - return 0; - } + assert(dst_ds_ctx); - /* - * Next check first if the COMMCFG_REQ is for running DS - * or not. Report failure if its not. MGMTD currently only - * supports editing the Candidate DS. - */ - if (commcfg_req->dst_ds_id != MGMTD_DS_RUNNING) { + /* User should have lock on both source and dest DS */ + if (!session->ds_locked[commcfg_req->dst_ds_id] || + !session->ds_locked[commcfg_req->src_ds_id]) { mgmt_fe_send_commitcfg_reply( session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id, - commcfg_req->req_id, MGMTD_INTERNAL_ERROR, + commcfg_req->req_id, MGMTD_DS_LOCK_FAILED, commcfg_req->validate_only, - "Set-Config on datastores other than Running DS not permitted!"); + "Commit requires lock on candidate and/or running DS"); return 0; } @@ -1090,26 +1005,6 @@ static int mgmt_fe_session_handle_commit_config_req_msg( session->cfg_txn_id, session->session_id); } - - /* - * Try taking write-lock on the destination DS (if not already). - */ - if (!session->ds_locked[commcfg_req->dst_ds_id]) { - if (mgmt_fe_session_write_lock_ds(commcfg_req->dst_ds_id, - dst_ds_ctx, session) - != 0) { - mgmt_fe_send_commitcfg_reply( - session, commcfg_req->src_ds_id, - commcfg_req->dst_ds_id, commcfg_req->req_id, - MGMTD_DS_LOCK_FAILED, - commcfg_req->validate_only, - "Failed to lock the destination DS!"); - return 0; - } - - session->ds_locked_implict[commcfg_req->dst_ds_id] = true; - } - /* * Create COMMITConfig request under the transaction */ @@ -1647,12 +1542,8 @@ void mgmt_fe_adapter_status_write(struct vty *vty, bool detail) FOREACH_MGMTD_DS_ID (ds_id) { if (session->ds_locked[ds_id]) { locked = true; - vty_out(vty, " %s\t\t\t%s\n", - mgmt_ds_id2name(ds_id), - session->ds_locked_implict - [ds_id] - ? "Implicit" - : "Explicit"); + vty_out(vty, " %s\n", + mgmt_ds_id2name(ds_id)); } } if (!locked) diff --git a/mgmtd/mgmt_history.c b/mgmtd/mgmt_history.c index 89a7a60166..d4069325ca 100644 --- a/mgmtd/mgmt_history.c +++ b/mgmtd/mgmt_history.c @@ -196,23 +196,21 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty, } 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"); - return -1; - } - - /* - * Note: Write lock on src_ds is not required. This is already - * taken in 'conf te'. - */ dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, MGMTD_DS_RUNNING); - if (!dst_ds_ctx) { - vty_out(vty, "ERROR: Couldnot access Running datastore!\n"); + assert(src_ds_ctx); + assert(dst_ds_ctx); + + ret = mgmt_ds_lock(src_ds_ctx, vty->mgmt_session_id); + if (ret != 0) { + vty_out(vty, + "Failed to lock the DS %u for rollback Reason: %s!\n", + MGMTD_DS_RUNNING, strerror(ret)); return -1; } ret = mgmt_ds_lock(dst_ds_ctx, vty->mgmt_session_id); if (ret != 0) { + mgmt_ds_unlock(src_ds_ctx); vty_out(vty, "Failed to lock the DS %u for rollback Reason: %s!\n", MGMTD_DS_RUNNING, strerror(ret)); @@ -223,27 +221,28 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty, ret = mgmt_ds_load_config_from_file( src_ds_ctx, cmt_info->cmt_json_file, false); if (ret != 0) { - mgmt_ds_unlock(dst_ds_ctx); vty_out(vty, "Error with parsing the file with error code %d\n", ret); - return ret; + goto failed_unlock; } } /* Internally trigger a commit-request. */ ret = mgmt_txn_rollback_trigger_cfg_apply(src_ds_ctx, dst_ds_ctx); if (ret != 0) { - mgmt_ds_unlock(dst_ds_ctx); vty_out(vty, "Error with creating commit apply txn with error code %d\n", ret); - return ret; + goto failed_unlock; } mgmt_history_dump_cmt_record_index(); - /* XXX chopps when does this get unlocked? */ + /* + * TODO: Cleanup: the generic TXN code currently checks for rollback + * and does the unlock when it completes. + */ /* * Block the rollback command from returning till the rollback @@ -253,6 +252,11 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty, vty->mgmt_req_pending_cmd = "ROLLBACK"; rollback_vty = vty; return 0; + +failed_unlock: + mgmt_ds_unlock(src_ds_ctx); + mgmt_ds_unlock(dst_ds_ctx); + return ret; } void mgmt_history_rollback_complete(bool success) diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index c1f674556a..de1ffa1a1f 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -687,18 +687,18 @@ static void mgmt_txn_process_set_cfg(struct event *thread) assert(mgmt_txn_reqs_count(&txn->set_cfg_reqs) == 1); assert(txn_req->req.set_cfg->dst_ds_ctx); - ret = mgmt_ds_lock(txn_req->req.set_cfg->dst_ds_ctx, - txn->session_id); - if (ret != 0) { + /* We expect the user to have locked the DST DS */ + if (!mgmt_ds_is_locked(txn_req->req.set_cfg->dst_ds_ctx, + txn->session_id)) { MGMTD_TXN_ERR( - "Failed to lock DS %u txn-id: %" PRIu64 + "DS %u not locked for implicit commit txn-id: %" PRIu64 " session-id: %" PRIu64 " err: %s", txn_req->req.set_cfg->dst_ds_id, txn->txn_id, txn->session_id, strerror(ret)); mgmt_txn_send_commit_cfg_reply( txn, MGMTD_DS_LOCK_FAILED, - "Lock running DS before implicit commit failed!"); + "running DS not locked for implicit commit"); goto mgmt_txn_process_set_cfg_done; } @@ -757,7 +757,12 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, success = (result == MGMTD_SUCCESS || result == MGMTD_NO_CFG_CHANGES); + /* TODO: these replies should not be send if it's a rollback + * b/c right now that is special cased.. that special casing should be + * removed; however... + */ if (!txn->commit_cfg_req->req.commit_cfg.implicit && txn->session_id + && !txn->commit_cfg_req->req.commit_cfg.rollback && mgmt_fe_send_commit_cfg_reply( txn->session_id, txn->txn_id, txn->commit_cfg_req->req.commit_cfg.src_ds_id, @@ -773,6 +778,7 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, } if (txn->commit_cfg_req->req.commit_cfg.implicit && txn->session_id + && !txn->commit_cfg_req->req.commit_cfg.rollback && mgmt_fe_send_set_cfg_reply( txn->session_id, txn->txn_id, txn->commit_cfg_req->req.commit_cfg.src_ds_id, @@ -787,6 +793,7 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, if (success) { /* Stop the commit-timeout timer */ + /* XXX why only on success? */ EVENT_OFF(txn->comm_cfg_timeout); create_cmt_info_rec = @@ -833,17 +840,18 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, } if (txn->commit_cfg_req->req.commit_cfg.rollback) { + mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx); mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx); - /* * Resume processing the rollback command. + * + * TODO: there's no good reason to special case rollback, the + * rollback boolean should be passed back to the FE client and it + * can do the right thing. */ mgmt_history_rollback_complete(success); } - if (txn->commit_cfg_req->req.commit_cfg.implicit) - mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx); - txn->commit_cfg_req->req.commit_cfg.cmt_stats = NULL; mgmt_txn_req_free(&txn->commit_cfg_req); @@ -2651,10 +2659,11 @@ int mgmt_txn_rollback_trigger_cfg_apply(struct mgmt_ds_ctx *src_ds_ctx, struct mgmt_ds_ctx *dst_ds_ctx) { static struct nb_config_cbs changes; + static struct mgmt_commit_stats dummy_stats; + struct nb_config_cbs *cfg_chgs = NULL; struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; - static struct mgmt_commit_stats dummy_stats; memset(&changes, 0, sizeof(changes)); memset(&dummy_stats, 0, sizeof(dummy_stats)); diff --git a/tests/lib/test_grpc.cpp b/tests/lib/test_grpc.cpp index fd30f04346..87530d41d8 100644 --- a/tests/lib/test_grpc.cpp +++ b/tests/lib/test_grpc.cpp @@ -114,7 +114,7 @@ static void static_startup(void) // Add a route vty = vty_new(); vty->type = vty::VTY_TERM; - vty_config_enter(vty, true, false); + vty_config_enter(vty, true, false, false); auto ret = cmd_execute(vty, "ip route 11.0.0.0/8 Null0", NULL, 0); assert(!ret); diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c index c94b47fef5..ee52a98adb 100644 --- a/vtysh/vtysh.c +++ b/vtysh/vtysh.c @@ -2333,8 +2333,9 @@ DEFUNSH(VTYSH_REALLYALL, vtysh_disable, vtysh_disable_cmd, "disable", } DEFUNSH(VTYSH_REALLYALL, vtysh_config_terminal, vtysh_config_terminal_cmd, - "configure [terminal]", + "configure [terminal [file-lock]]", "Configuration from vty interface\n" + "Configuration with locked datastores\n" "Configuration terminal\n") { vty->node = CONFIG_NODE; @@ -2355,7 +2356,7 @@ static int vtysh_exit(struct vty *vty) if (vty->node == CONFIG_NODE) { /* resync in case one of the daemons is somewhere else */ vtysh_execute("end"); - vtysh_execute("configure"); + vtysh_execute("configure terminal file-lock"); } return CMD_SUCCESS; } diff --git a/vtysh/vtysh_config.c b/vtysh/vtysh_config.c index 2949faa427..a5f790bbc6 100644 --- a/vtysh/vtysh_config.c +++ b/vtysh/vtysh_config.c @@ -607,7 +607,7 @@ static int vtysh_read_file(FILE *confp, bool dry_run) vty->node = CONFIG_NODE; vtysh_execute_no_pager("enable"); - vtysh_execute_no_pager("configure terminal"); + vtysh_execute_no_pager("configure terminal file-lock"); if (!dry_run) vtysh_execute_no_pager("XFRR_start_configuration");