Skip to content

Commit 1dd6181

Browse files
r12fsudhanshukumar22
r12f
and
sudhanshukumar22
authored
[202412][PR:22408] Ported the fix FRRouting/frr#18601 from FRR community (sonic-net#1675)
Sync PR sonic-net#22408 from sonic-net/sonic-buildimage to 202412. Original PR: sonic-net#22408 Co-authored-by: sudhanshukumar22 <[email protected]>
1 parent c8bd83a commit 1dd6181

3 files changed

Lines changed: 222 additions & 1 deletion

File tree

‎src/sonic-frr/patch/0144-mgmtd-remove-bogus-hedge-code-which-corrupted-active.patch‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
From b12b4c28b4c4a76cbc906b703ee5a694a082ab74 Mon Sep 17 00:00:00 2001
2+
From: Christian Hopps <[email protected]>
3+
Date: Tue, 8 Apr 2025 05:15:53 +0000
4+
Subject: [PATCH] mgmtd: remove bogus "hedge" code which corrupted active
5+
candidate DS
6+
7+
Say you have 2 mgmtd frontend sessions (2 vtysh's) the first one is long
8+
running and is actively changing the global candidate datastore (DS),
9+
the second one starts and exits, this code would then copy running
10+
back over the candidate, blowing away any changes made by the first
11+
session.
12+
13+
(the long running session could technically be any user)
14+
15+
Instead we need to trust the various cleanup code that already exits.
16+
For example in the commit_cfg_reply on success candidate is copied to
17+
running, and on failure *for implicit commit* running is copied back to
18+
candidate clearing the change. This leaves the non-implicit
19+
configuration changes in this case we actually want candidate to keep
20+
it's changes in transactional cases, in the other case of pending commit
21+
during a file read the code restores candidate (if needed) on exit from
22+
"config terminal", with this call stack:
23+
24+
vty_config_node_exit()
25+
nb_cli_pending_commit_check()
26+
nb_cli_classic_commit()
27+
nb_candidate_commit_prepare() [fail] -> copy running -> candidate
28+
nb_candidate_commit_apply() -> copy candidate -> running
29+
30+
fixes #18541
31+
32+
Signed-off-by: Christian Hopps <[email protected]>
33+
---
34+
mgmtd/mgmt_fe_adapter.c | 6 ------
35+
1 file changed, 6 deletions(-)
36+
37+
diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c
38+
index 8d59198803..d077e08679 100644
39+
--- a/mgmtd/mgmt_fe_adapter.c
40+
+++ b/mgmtd/mgmt_fe_adapter.c
41+
@@ -216,12 +216,6 @@ static void mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id,
42+
static void
43+
mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session)
44+
{
45+
- /*
46+
- * Ensure any uncommitted changes in Candidate DS
47+
- * is discarded.
48+
- */
49+
- mgmt_ds_copy_dss(mm->running_ds, mm->candidate_ds, false);
50+
-
51+
/*
52+
* Destroy the actual transaction created earlier.
53+
*/
54+
--
55+
2.39.4
56+

‎src/sonic-frr/patch/0145-mgmtd-normalize-argument-order-to-copy-dst-src.patch‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
From 59d2368b0f055f28aeda8f6080d686acfa35c20b Mon Sep 17 00:00:00 2001
2+
From: Christian Hopps <[email protected]>
3+
Date: Tue, 8 Apr 2025 05:55:03 +0000
4+
Subject: [PATCH] mgmtd: normalize argument order to copy(dst, src)
5+
6+
Having just completed a code audit during RCA, the fact that we have 2
7+
different argument orders for the related datastore copying functions
8+
was unnecessary and super confusing.
9+
10+
Fix this code-maintenance/comprehension mistake and move the newer mgmtd
11+
copy routines to use the same arg order as the pre-existing underlying
12+
northbound copy functions (i.e., use `copy(dst, src)`)
13+
14+
Signed-off-by: Christian Hopps <[email protected]>
15+
---
16+
mgmtd/mgmt_ds.c | 19 ++++++++-----------
17+
mgmtd/mgmt_ds.h | 12 +++++-------
18+
mgmtd/mgmt_txn.c | 26 ++++++++++----------------
19+
3 files changed, 23 insertions(+), 34 deletions(-)
20+
21+
diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c
22+
index dabae4afd1..e83980f97b 100644
23+
--- a/mgmtd/mgmt_ds.c
24+
+++ b/mgmtd/mgmt_ds.c
25+
@@ -74,8 +74,7 @@ static int mgmt_ds_dump_in_memory(struct mgmt_ds_ctx *ds_ctx,
26+
return 0;
27+
}
28+
29+
-static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src,
30+
- struct mgmt_ds_ctx *dst)
31+
+static int ds_copy(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src)
32+
{
33+
if (!src || !dst)
34+
return -1;
35+
@@ -95,8 +94,7 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src,
36+
return 0;
37+
}
38+
39+
-static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src,
40+
- struct mgmt_ds_ctx *dst)
41+
+static int ds_merge(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src)
42+
{
43+
int ret;
44+
45+
@@ -251,14 +249,13 @@ void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx)
46+
ds_ctx->locked = 0;
47+
}
48+
49+
-int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx,
50+
- struct mgmt_ds_ctx *dst_ds_ctx, bool updt_cmt_rec)
51+
+int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool updt_cmt_rec)
52+
{
53+
- if (mgmt_ds_replace_dst_with_src_ds(src_ds_ctx, dst_ds_ctx) != 0)
54+
+ if (ds_copy(dst, src) != 0)
55+
return -1;
56+
57+
- if (updt_cmt_rec && dst_ds_ctx->ds_id == MGMTD_DS_RUNNING)
58+
- mgmt_history_new_record(dst_ds_ctx);
59+
+ if (updt_cmt_rec && dst->ds_id == MGMTD_DS_RUNNING)
60+
+ mgmt_history_new_record(dst);
61+
62+
return 0;
63+
}
64+
@@ -416,9 +413,9 @@ int mgmt_ds_load_config_from_file(struct mgmt_ds_ctx *dst,
65+
parsed.ds_id = dst->ds_id;
66+
67+
if (merge)
68+
- mgmt_ds_merge_src_with_dst_ds(&parsed, dst);
69+
+ ds_merge(dst, &parsed);
70+
else
71+
- mgmt_ds_replace_dst_with_src_ds(&parsed, dst);
72+
+ ds_copy(dst, &parsed);
73+
74+
nb_config_free(parsed.root.cfg_root);
75+
76+
diff --git a/mgmtd/mgmt_ds.h b/mgmtd/mgmt_ds.h
77+
index b8e77e330a..f7e1d7c5ee 100644
78+
--- a/mgmtd/mgmt_ds.h
79+
+++ b/mgmtd/mgmt_ds.h
80+
@@ -196,21 +196,19 @@ extern void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx);
81+
/*
82+
* Copy from source to destination datastore.
83+
*
84+
- * src_ds
85+
- * Source datastore handle (ds to be copied from).
86+
- *
87+
- * dst_ds
88+
+ * dst
89+
* Destination datastore handle (ds to be copied to).
90+
*
91+
+ * src
92+
+ * Source datastore handle (ds to be copied from).
93+
+ *
94+
* update_cmd_rec
95+
* TRUE if need to update commit record, FALSE otherwise.
96+
*
97+
* Returns:
98+
* 0 on success, -1 on failure.
99+
*/
100+
-extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx,
101+
- struct mgmt_ds_ctx *dst_ds_ctx,
102+
- bool update_cmt_rec);
103+
+extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool update_cmt_rec);
104+
105+
/*
106+
* Fetch northbound configuration for a given datastore context.
107+
diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c
108+
index 483dfab8e8..2b4734e971 100644
109+
--- a/mgmtd/mgmt_txn.c
110+
+++ b/mgmtd/mgmt_txn.c
111+
@@ -764,17 +764,15 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
112+
!txn->commit_cfg_req->req.commit_cfg.rollback);
113+
114+
/*
115+
- * Successful commit: Merge Src DS into Dst DS if and only if
116+
+ * Successful commit: Copy Src DS to Dst DS if and only if
117+
* this was not a validate-only or abort request.
118+
*/
119+
if ((txn->session_id &&
120+
!txn->commit_cfg_req->req.commit_cfg.validate_only &&
121+
!txn->commit_cfg_req->req.commit_cfg.abort) ||
122+
txn->commit_cfg_req->req.commit_cfg.rollback) {
123+
- mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg
124+
- .src_ds_ctx,
125+
- txn->commit_cfg_req->req.commit_cfg
126+
- .dst_ds_ctx,
127+
+ mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx,
128+
+ txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
129+
create_cmt_info_rec);
130+
}
131+
132+
@@ -783,22 +781,18 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
133+
* request.
134+
*/
135+
if (txn->session_id && txn->commit_cfg_req->req.commit_cfg.abort)
136+
- mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg
137+
- .dst_ds_ctx,
138+
- txn->commit_cfg_req->req.commit_cfg
139+
- .src_ds_ctx,
140+
- false);
141+
+ mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
142+
+ txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false);
143+
} else {
144+
/*
145+
* The commit has failied. For implicit commit requests restore
146+
- * back the contents of the candidate DS.
147+
+ * back the contents of the candidate DS. For non-implicit
148+
+ * commit we want to allow the user to re-commit on the changes
149+
+ * (whether further modified or not).
150+
*/
151+
if (txn->commit_cfg_req->req.commit_cfg.implicit)
152+
- mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg
153+
- .dst_ds_ctx,
154+
- txn->commit_cfg_req->req.commit_cfg
155+
- .src_ds_ctx,
156+
- false);
157+
+ mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
158+
+ txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false);
159+
}
160+
161+
if (txn->commit_cfg_req->req.commit_cfg.rollback) {
162+
--
163+
2.39.4
164+

‎src/sonic-frr/patch/series‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,4 +122,5 @@
122122
0140-zebra-fix-show-route-vrf-all-memory-consumption.patch
123123
0141-zebra-fix-show-route-memory-consumption.patch
124124
0142-lib-Return-duplicate-prefix-list-entry-test.patch
125-
0143-mgmtd-clean-session-config-only-when-it-is-needed.patch
125+
0144-mgmtd-remove-bogus-hedge-code-which-corrupted-active.patch
126+
0145-mgmtd-normalize-argument-order-to-copy-dst-src.patch

0 commit comments

Comments
 (0)