From b12b4c28b4c4a76cbc906b703ee5a694a082ab74 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Tue, 8 Apr 2025 05:15:53 +0000 Subject: [PATCH 1/2] mgmtd: remove bogus "hedge" code which corrupted active candidate DS Say you have 2 mgmtd frontend sessions (2 vtysh's) the first one is long running and is actively changing the global candidate datastore (DS), the second one starts and exits, this code would then copy running back over the candidate, blowing away any changes made by the first session. (the long running session could technically be any user) Instead we need to trust the various cleanup code that already exits. For example in the commit_cfg_reply on success candidate is copied to running, and on failure *for implicit commit* running is copied back to candidate clearing the change. This leaves the non-implicit configuration changes in this case we actually want candidate to keep it's changes in transactional cases, in the other case of pending commit during a file read the code restores candidate (if needed) on exit from "config terminal", with this call stack: vty_config_node_exit() nb_cli_pending_commit_check() nb_cli_classic_commit() nb_candidate_commit_prepare() [fail] -> copy running -> candidate nb_candidate_commit_apply() -> copy candidate -> running fixes #18541 Signed-off-by: Christian Hopps --- mgmtd/mgmt_fe_adapter.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index 8d59198803..d077e08679 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -216,12 +216,6 @@ 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) { - /* - * Ensure any uncommitted changes in Candidate DS - * is discarded. - */ - mgmt_ds_copy_dss(mm->running_ds, mm->candidate_ds, false); - /* * Destroy the actual transaction created earlier. */ From 59d2368b0f055f28aeda8f6080d686acfa35c20b Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Tue, 8 Apr 2025 05:55:03 +0000 Subject: [PATCH 2/2] mgmtd: normalize argument order to copy(dst, src) Having just completed a code audit during RCA, the fact that we have 2 different argument orders for the related datastore copying functions was unnecessary and super confusing. Fix this code-maintenance/comprehension mistake and move the newer mgmtd copy routines to use the same arg order as the pre-existing underlying northbound copy functions (i.e., use `copy(dst, src)`) Signed-off-by: Christian Hopps --- mgmtd/mgmt_ds.c | 19 ++++++++----------- mgmtd/mgmt_ds.h | 12 +++++------- mgmtd/mgmt_txn.c | 26 ++++++++++---------------- 3 files changed, 23 insertions(+), 34 deletions(-) diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c index dabae4afd1..e83980f97b 100644 --- a/mgmtd/mgmt_ds.c +++ b/mgmtd/mgmt_ds.c @@ -74,8 +74,7 @@ static int mgmt_ds_dump_in_memory(struct mgmt_ds_ctx *ds_ctx, return 0; } -static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src, - struct mgmt_ds_ctx *dst) +static int ds_copy(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src) { if (!src || !dst) return -1; @@ -95,8 +94,7 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src, return 0; } -static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src, - struct mgmt_ds_ctx *dst) +static int ds_merge(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src) { int ret; @@ -251,14 +249,13 @@ void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx) ds_ctx->locked = 0; } -int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx, - struct mgmt_ds_ctx *dst_ds_ctx, bool updt_cmt_rec) +int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool updt_cmt_rec) { - if (mgmt_ds_replace_dst_with_src_ds(src_ds_ctx, dst_ds_ctx) != 0) + if (ds_copy(dst, src) != 0) return -1; - if (updt_cmt_rec && dst_ds_ctx->ds_id == MGMTD_DS_RUNNING) - mgmt_history_new_record(dst_ds_ctx); + if (updt_cmt_rec && dst->ds_id == MGMTD_DS_RUNNING) + mgmt_history_new_record(dst); return 0; } @@ -416,9 +413,9 @@ int mgmt_ds_load_config_from_file(struct mgmt_ds_ctx *dst, parsed.ds_id = dst->ds_id; if (merge) - mgmt_ds_merge_src_with_dst_ds(&parsed, dst); + ds_merge(dst, &parsed); else - mgmt_ds_replace_dst_with_src_ds(&parsed, dst); + ds_copy(dst, &parsed); nb_config_free(parsed.root.cfg_root); diff --git a/mgmtd/mgmt_ds.h b/mgmtd/mgmt_ds.h index b8e77e330a..f7e1d7c5ee 100644 --- a/mgmtd/mgmt_ds.h +++ b/mgmtd/mgmt_ds.h @@ -196,21 +196,19 @@ extern void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx); /* * Copy from source to destination datastore. * - * src_ds - * Source datastore handle (ds to be copied from). - * - * dst_ds + * dst * Destination datastore handle (ds to be copied to). * + * src + * Source datastore handle (ds to be copied from). + * * update_cmd_rec * TRUE if need to update commit record, FALSE otherwise. * * Returns: * 0 on success, -1 on failure. */ -extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx, - struct mgmt_ds_ctx *dst_ds_ctx, - bool update_cmt_rec); +extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool update_cmt_rec); /* * Fetch northbound configuration for a given datastore context. diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index 483dfab8e8..2b4734e971 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -764,17 +764,15 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, !txn->commit_cfg_req->req.commit_cfg.rollback); /* - * Successful commit: Merge Src DS into Dst DS if and only if + * Successful commit: Copy Src DS to Dst DS if and only if * this was not a validate-only or abort request. */ if ((txn->session_id && !txn->commit_cfg_req->req.commit_cfg.validate_only && !txn->commit_cfg_req->req.commit_cfg.abort) || txn->commit_cfg_req->req.commit_cfg.rollback) { - mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg - .src_ds_ctx, - txn->commit_cfg_req->req.commit_cfg - .dst_ds_ctx, + mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, + txn->commit_cfg_req->req.commit_cfg.src_ds_ctx, create_cmt_info_rec); } @@ -783,22 +781,18 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, * request. */ if (txn->session_id && txn->commit_cfg_req->req.commit_cfg.abort) - mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg - .dst_ds_ctx, - txn->commit_cfg_req->req.commit_cfg - .src_ds_ctx, - false); + mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx, + txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false); } else { /* * The commit has failied. For implicit commit requests restore - * back the contents of the candidate DS. + * back the contents of the candidate DS. For non-implicit + * commit we want to allow the user to re-commit on the changes + * (whether further modified or not). */ if (txn->commit_cfg_req->req.commit_cfg.implicit) - mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg - .dst_ds_ctx, - txn->commit_cfg_req->req.commit_cfg - .src_ds_ctx, - false); + mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx, + txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false); } if (txn->commit_cfg_req->req.commit_cfg.rollback) {