Merge pull request #15286 from idryzhov/fix-mgmt-startup

Fix conflict in mgmtd on startup

lib/vty.h

@@ -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)

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);

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,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;
 		}
 
@@ -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;
 		}
@@ -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;
 

vtysh/vtysh.c

@@ -1669,7 +1669,6 @@ static int vtysh_end(void)
 		/* Nothing to do. */
 		break;
 	default:
-		vty->vtysh_file_locked = false;
 		vty->node = ENABLE_NODE;
 		break;
 	}
@@ -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]]",
 	"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;
 }
 
@@ -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");
-		/* 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;
 }
 
@@ -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. */

vtysh/vtysh_config.c

@@ -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)
+		usleep(100000);
 
 	if (!dry_run)
 		vtysh_execute_no_pager("XFRR_start_configuration");
@@ -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);