From ca0d19fcff778bac60dc6662b31d67de7080192c Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Sat, 3 Feb 2024 00:42:58 +0200 Subject: [PATCH] mgmtd, vtysh: fix possible conflict when reading the config When FRR starts, after mgmtd is initialized, backend clients connect to it and request their config. To supply the config, mgmtd creates a configuration transaction. At the same time, `vtysh -b` tries to read the startup config and configure mgmtd, which also creates a configuration transaction. If these two actions happen at the exact same time, there's a conflict between them, because only a single configuration translaction is allowed. Because of that, vtysh fails and the config is completely ignored. When starting the config reading, vtysh locks candidate and running datastores in mgmtd. This commit adds locking of running datastore when initializing the backend client. It allows to retry locking on the vtysh side and read the config only when the lock is aquired instead of failing. This change also prevents running datastore from being changed during initialization of backend clients. This could lead to a desynchronized state between mgmtd and backends. Signed-off-by: Igor Ryzhov --- mgmtd/mgmt_fe_adapter.c | 3 --- mgmtd/mgmt_txn.c | 29 +++++++++++++++++++++++++++-- vtysh/vtysh_config.c | 8 +++++++- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index 001da7680b8a..ec8e7733548b 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -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); diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index df2a1d852d09..8cc5c5602bca 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -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; @@ -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); @@ -2081,6 +2090,7 @@ 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) { @@ -2093,6 +2103,19 @@ int mgmt_txn_notify_be_adapter_conn(struct mgmt_be_client_adapter *adapter, return 0; } + 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) { + __log_err("Running datatore is locked"); + nb_config_diff_del_changes(adapter_cfgs); + return -1; + } + /* * Create a CONFIG transaction to push the config changes * provided to the backend client. @@ -2101,6 +2124,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; } @@ -2114,10 +2138,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; diff --git a/vtysh/vtysh_config.c b/vtysh/vtysh_config.c index 888f6a8c2162..15bcd343c979 100644 --- a/vtysh/vtysh_config.c +++ b/vtysh/vtysh_config.c @@ -616,7 +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"); + /* + * 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) + usleep(100000); vty->vtysh_file_locked = true; if (!dry_run)