From: Christian Hopps Date: Sun, 18 Jun 2023 20:19:54 +0000 (-0400) Subject: mgmtd: KISS the locking code X-Git-Tag: base_9.1~334^2~1 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=df0173ceeb93572329b04f1bfc5a8925e60513e3;p=mirror%2Ffrr.git mgmtd: KISS the locking code Move away from things like "lock if not locked" type code, require the user has locked prior to geting to that point. For now we warn if we are taking a lock we already had; however, this should really be a failure point. New requirements: SETCFG - not implicit commit - requires user has locked candidate DS and they must unlock after implicit commit - requires user has locked candidate and running DS both locks will be unlocked on reply to the SETCFG COMMITCFG - requires user has locked candidate and running DS and they must unlock after rollback - this code now get both locks and then does an unlock and early return thing on the adapter side. It needs to be un-special cased in follow up work that would also include tests for this functionality. Signed-off-by: Christian Hopps --- 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; + + /* 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; + } } - 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]; - } + for (indx = 0; indx < vty->num_cfg_changes; indx++) { + mgmt_yang_data_init(&cfg_data[indx]); - cfg_data[cnt].xpath = vty->cfg_changes[indx].xpath; + 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]; + } - 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; + cfg_data[indx].xpath = vty->cfg_changes[indx].xpath; - 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"); - } + 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; - cfgreq[cnt] = &cfg_req[cnt]; - cnt++; + 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(); } - 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"); - return -1; - } + cfgreq[indx] = &cfg_req[indx]; + } + if (!indx) + return 0; - vty->mgmt_req_pending_cmd = "MESSAGE_SETCFG_REQ"; + 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,20 +781,11 @@ 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; - } - - 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); + /* delete transaction if we just created it */ + if (txn_created) + mgmt_destroy_txn(&session->cfg_txn_id); + } return 0; } @@ -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. - */ - 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. - */ - dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, commcfg_req->dst_ds_id); - if (!dst_ds_ctx) { + /* 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, - "No such destination DS exists!"); + "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); + assert(src_ds_ctx); + dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, commcfg_req->dst_ds_id); + 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");