Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix conflict in mgmtd on startup #15286

Merged
merged 2 commits into from
Feb 7, 2024
Merged

Fix conflict in mgmtd on startup #15286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0db4d55
mgmtd, vtysh: fix possible conflict when reading the config
idryzhov Feb 2, 2024
2574f03
vtysh: remove resync workaround when exiting to config node
idryzhov Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions lib/vty.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,6 @@ struct vty {
uintptr_t mgmt_req_pending_data;
bool mgmt_locked_candidate_ds;
bool mgmt_locked_running_ds;
/* Need to track when we file-lock in vtysh to re-lock on end/conf t
* workaround
*/
bool vtysh_file_locked;
};

static inline void vty_push_context(struct vty *vty, int node, uint64_t id)
Expand Down
3 changes: 0 additions & 3 deletions mgmtd/mgmt_fe_adapter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,9 +711,6 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session,
}

if (session->cfg_txn_id == MGMTD_TXN_ID_NONE) {
/* as we have the lock no-one else should have a config txn */
assert(!mgmt_config_txn_in_progress());

/* Start a CONFIG Transaction (if not started already) */
session->cfg_txn_id = mgmt_create_txn(session->session_id,
MGMTD_TXN_TYPE_CONFIG);
Expand Down
28 changes: 25 additions & 3 deletions mgmtd/mgmt_txn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ struct mgmt_commit_cfg_req {
uint8_t abort : 1;
uint8_t implicit : 1;
uint8_t rollback : 1;
uint8_t init : 1;

/* Track commit phases */
enum mgmt_commit_phase phase;
Expand Down Expand Up @@ -750,6 +751,14 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
mgmt_history_rollback_complete(success);
}

if (txn->commit_cfg_req->req.commit_cfg.init) {
/*
* This is the backend init request.
* We need to unlock the running datastore.
*/
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);

Expand Down Expand Up @@ -2081,15 +2090,26 @@ int mgmt_txn_notify_be_adapter_conn(struct mgmt_be_client_adapter *adapter,
struct mgmt_commit_cfg_req *cmtcfg_req;
static struct mgmt_commit_stats dummy_stats;
struct nb_config_cbs *adapter_cfgs = NULL;
struct mgmt_ds_ctx *ds_ctx;

memset(&dummy_stats, 0, sizeof(dummy_stats));
if (connect) {
/* Get config for this single backend client */
ds_ctx = mgmt_ds_get_ctx_by_id(mm, MGMTD_DS_RUNNING);
assert(ds_ctx);

/*
* Lock the running datastore to prevent any changes while we
* are initializing the backend.
*/
if (mgmt_ds_lock(ds_ctx, 0) != 0)
return -1;

/* Get config for this single backend client */
mgmt_be_get_adapter_config(adapter, &adapter_cfgs);
if (!adapter_cfgs || RB_EMPTY(nb_config_cbs, adapter_cfgs)) {
SET_FLAG(adapter->flags,
MGMTD_BE_ADAPTER_FLAGS_CFG_SYNCED);
mgmt_ds_unlock(ds_ctx);
return 0;
}

Expand All @@ -2101,6 +2121,7 @@ int mgmt_txn_notify_be_adapter_conn(struct mgmt_be_client_adapter *adapter,
if (!txn) {
__log_err("Failed to create CONFIG Transaction for downloading CONFIGs for client '%s'",
adapter->name);
mgmt_ds_unlock(ds_ctx);
nb_config_diff_del_changes(adapter_cfgs);
return -1;
}
Expand All @@ -2114,10 +2135,11 @@ int mgmt_txn_notify_be_adapter_conn(struct mgmt_be_client_adapter *adapter,
txn_req = mgmt_txn_req_alloc(txn, 0, MGMTD_TXN_PROC_COMMITCFG);
txn_req->req.commit_cfg.src_ds_id = MGMTD_DS_NONE;
txn_req->req.commit_cfg.src_ds_ctx = 0;
txn_req->req.commit_cfg.dst_ds_id = MGMTD_DS_NONE;
txn_req->req.commit_cfg.dst_ds_ctx = 0;
txn_req->req.commit_cfg.dst_ds_id = MGMTD_DS_RUNNING;
txn_req->req.commit_cfg.dst_ds_ctx = ds_ctx;
txn_req->req.commit_cfg.validate_only = false;
txn_req->req.commit_cfg.abort = false;
txn_req->req.commit_cfg.init = true;
txn_req->req.commit_cfg.cmt_stats = &dummy_stats;
txn_req->req.commit_cfg.cfg_chgs = adapter_cfgs;

Expand Down
30 changes: 1 addition & 29 deletions vtysh/vtysh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1669,7 +1669,6 @@ static int vtysh_end(void)
/* Nothing to do. */
break;
default:
vty->vtysh_file_locked = false;
vty->node = ENABLE_NODE;
break;
}
Expand Down Expand Up @@ -2393,23 +2392,12 @@ DEFUNSH(VTYSH_REALLYALL, vtysh_disable, vtysh_disable_cmd, "disable",
}

DEFUNSH(VTYSH_REALLYALL, vtysh_config_terminal, vtysh_config_terminal_cmd,
"configure [terminal]",
"Configuration from vty interface\n"
"Configuration terminal\n")
{
vty->node = CONFIG_NODE;
return CMD_SUCCESS;
}

DEFUNSH(VTYSH_REALLYALL, vtysh_config_terminal_file_lock,
vtysh_config_terminal_file_lock_cmd,
"configure terminal file-lock",
"configure [terminal [file-lock]]",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make file-lock a hidden command, and just leave the configure [terminal] public.

Copy link
Contributor Author

@idryzhov idryzhov Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using file-lock is not only needed for config reading, but it's also the way to work with FRR in explicit-commit mode instead of implicit-commit-after-every-command mode. However, there are two problems:

  • this explicit-commit mode works only for mgmtd-converted daemons
  • file-lock naming is confusing and doesn't explain that it enables explicit-commit mode

So maybe you're right and we should hide it until all daemons are converted to mgmtd. But also there's an option to rename file-lock to something like locked or with-commit and leave it not hidden.

Anyway, it's not really related to this fix, so can be changed in a separate PR.

"Configuration from vty interface\n"
"Configuration terminal\n"
"Configuration with locked datastores\n")
{
vty->node = CONFIG_NODE;
vty->vtysh_file_locked = true;
return CMD_SUCCESS;
}

Expand All @@ -2424,21 +2412,6 @@ static int vtysh_exit(struct vty *vty)
if (cnode->parent_node)
vty->node = cnode->parent_node;

if (vty->node == CONFIG_NODE) {
bool locked = vty->vtysh_file_locked;

/* resync in case one of the daemons is somewhere else */
vtysh_execute("end");
choppsv1 marked this conversation as resolved.
Show resolved Hide resolved
/* NOTE: a rather expensive thing to do, can we avoid it? */

if (locked)
vtysh_execute("configure terminal file-lock");
else
vtysh_execute("configure terminal");
} else if (vty->node == ENABLE_NODE) {
vty->vtysh_file_locked = false;
}

return CMD_SUCCESS;
}

Expand Down Expand Up @@ -5125,7 +5098,6 @@ void vtysh_init_vty(void)
if (!user_mode)
install_element(VIEW_NODE, &vtysh_enable_cmd);
install_element(ENABLE_NODE, &vtysh_config_terminal_cmd);
install_element(ENABLE_NODE, &vtysh_config_terminal_file_lock_cmd);
install_element(ENABLE_NODE, &vtysh_disable_cmd);

/* "exit" command. */
Expand Down
10 changes: 7 additions & 3 deletions vtysh/vtysh_config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,8 +616,13 @@ static int vtysh_read_file(FILE *confp, bool dry_run)
vty->node = CONFIG_NODE;

vtysh_execute_no_pager("enable");
vtysh_execute_no_pager("conf term file-lock");
vty->vtysh_file_locked = true;
/*
* When reading the config, we need to wait until the lock is acquired.
* If we ignore the failure and continue without the lock, the config
* will be fully ignored.
*/
while (vtysh_execute_no_pager("conf term file-lock") == CMD_WARNING_CONFIG_FAILED)
choppsv1 marked this conversation as resolved.
Show resolved Hide resolved
usleep(100000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that vtysh will never get the lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not during the startup. After the startup, it is possible if someone uses conf t file-lock in one vtysh instance and trying to use vtysh -b in another terminal at the same time. But I don't think we should cover this - it's up to the user to either exit from the first vtysh instance or kill the second one. I can also add some retry limit if you feel it's necessary.


if (!dry_run)
vtysh_execute_no_pager("XFRR_start_configuration");
Expand All @@ -629,7 +634,6 @@ static int vtysh_read_file(FILE *confp, bool dry_run)
vtysh_execute_no_pager("XFRR_end_configuration");

vtysh_execute_no_pager("end");
vty->vtysh_file_locked = false;
vtysh_execute_no_pager("disable");

vty_close(vty);
Expand Down
Loading