Skip to content

Commit 6921735

Browse files
yxiecayxieca
authored
[FRR] fix FRR mgmtd losing configuration issue (sonic-net#22183)
[202411][FRR] Fix FRR mgmtd losing configuration issue
1 parent f7cbdd9 commit 6921735

File tree

2 files changed

+168
-0
lines changed

2 files changed

+168
-0
lines changed

‎src/sonic-frr/patch/0064-mgmtd-clean-session-config-only-when-it-is-needed.patch

+167
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
From 210a082dea671accfc10a8dcce1431329f0153ae Mon Sep 17 00:00:00 2001
2+
From: Ying Xie <[email protected]>
3+
Date: Sat, 29 Mar 2025 15:30:37 +0000
4+
Subject: [PATCH 63/63] [mgmtd] clean session config only when it is needed
5+
6+
mgmtd configuration management has an issue where any session
7+
can clean up outstanding configuration upon destruction.
8+
9+
When a long-lived session is taking configuration changes, and
10+
another short-lived session which never took any configuration
11+
closes, the outstanding configuration would be lost because
12+
the configuration clearing doesn't have protection during session
13+
closing.
14+
15+
This change keeps track if a session has received any configuration,
16+
and if the configuration has been applied or cleared.
17+
18+
The outstanding configuration should be applied or cleared before
19+
session closure (assertion).
20+
21+
When clearing the outstanding session structure, only attempt to
22+
clear configuration when the closing session has outstanding
23+
configurations.
24+
25+
Signed-off-by: Ying Xie <[email protected]>
26+
---
27+
mgmtd/mgmt_fe_adapter.c | 57 +++++++++++++++++++++++++++++------------
28+
1 file changed, 41 insertions(+), 16 deletions(-)
29+
30+
diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c
31+
index ec8e77335..27171dfa2 100644
32+
--- a/mgmtd/mgmt_fe_adapter.c
33+
+++ b/mgmtd/mgmt_fe_adapter.c
34+
@@ -45,6 +45,8 @@ struct mgmt_fe_session_ctx {
35+
uint8_t ds_locked[MGMTD_DS_MAX_ID];
36+
struct event *proc_cfg_txn_clnp;
37+
struct event *proc_show_txn_clnp;
38+
+ uint32_t config_count;
39+
+ uint32_t committed_count;
40+
41+
struct mgmt_fe_sessions_item list_linkage;
42+
};
43+
@@ -118,7 +120,10 @@ mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session)
44+
* Ensure any uncommitted changes in Candidate DS
45+
* is discarded.
46+
*/
47+
- mgmt_ds_copy_dss(mm->running_ds, mm->candidate_ds, false);
48+
+ if (session->config_count > 0) {
49+
+ mgmt_ds_copy_dss(mm->running_ds, mm->candidate_ds, false);
50+
+ session->config_count = 0;
51+
+ }
52+
53+
/*
54+
* Destroy the actual transaction created earlier.
55+
@@ -278,6 +283,8 @@ mgmt_fe_create_session(struct mgmt_fe_client_adapter *adapter,
56+
session->adapter = adapter;
57+
session->txn_id = MGMTD_TXN_ID_NONE;
58+
session->cfg_txn_id = MGMTD_TXN_ID_NONE;
59+
+ session->config_count = 0;
60+
+ session->committed_count = 0;
61+
mgmt_fe_adapter_lock(adapter);
62+
mgmt_fe_sessions_add_tail(&adapter->fe_sessions, session);
63+
if (!mgmt_fe_next_session_id)
64+
@@ -373,9 +380,11 @@ static int fe_adapter_send_set_cfg_reply(struct mgmt_fe_session_ctx *session,
65+
66+
assert(session->adapter);
67+
68+
- if (implicit_commit && session->cfg_txn_id)
69+
+ if (implicit_commit && session->cfg_txn_id) {
70+
+ session->committed_count += session->config_count;
71+
mgmt_fe_session_register_event(
72+
session, MGMTD_FE_SESSION_CFG_TXN_CLNUP);
73+
+ }
74+
75+
mgmtd__fe_set_config_reply__init(&setcfg_reply);
76+
setcfg_reply.session_id = session->session_id;
77+
@@ -443,9 +452,11 @@ static int fe_adapter_send_commit_cfg_reply(
78+
*/
79+
if (session->cfg_txn_id
80+
&& ((result == MGMTD_SUCCESS && !validate_only)
81+
- || (result == MGMTD_NO_CFG_CHANGES)))
82+
+ || (result == MGMTD_NO_CFG_CHANGES))) {
83+
+ session->committed_count += session->config_count;
84+
mgmt_fe_session_register_event(
85+
session, MGMTD_FE_SESSION_CFG_TXN_CLNUP);
86+
+ }
87+
88+
if (mm->perf_stats_en)
89+
gettimeofday(&session->adapter->cmt_stats.last_end, NULL);
90+
@@ -919,6 +930,7 @@ mgmt_fe_adapter_handle_msg(struct mgmt_fe_client_adapter *adapter,
91+
Mgmtd__FeMessage *fe_msg)
92+
{
93+
struct mgmt_fe_session_ctx *session;
94+
+ static int session_cnt = 0;
95+
96+
/*
97+
* protobuf-c adds a max size enum with an internal, and changing by
98+
@@ -941,27 +953,37 @@ mgmt_fe_adapter_handle_msg(struct mgmt_fe_client_adapter *adapter,
99+
&& fe_msg->session_req->id_case
100+
== MGMTD__FE_SESSION_REQ__ID_CLIENT_CONN_ID) {
101+
__dbg("Got SESSION_REQ (create) for client-id %" PRIu64
102+
- " from '%s'",
103+
- fe_msg->session_req->client_conn_id,
104+
- adapter->name);
105+
+ " from '%s' session_cnt %d",
106+
+ fe_msg->session_req->client_conn_id,
107+
+ adapter->name, session_cnt);
108+
109+
session = mgmt_fe_create_session(
110+
adapter, fe_msg->session_req->client_conn_id);
111+
+ if (session) {
112+
+ session_cnt++;
113+
+ }
114+
fe_adapter_send_session_reply(adapter, session, true,
115+
session ? true : false);
116+
} else if (
117+
!fe_msg->session_req->create
118+
&& fe_msg->session_req->id_case
119+
== MGMTD__FE_SESSION_REQ__ID_SESSION_ID) {
120+
- __dbg("Got SESSION_REQ (destroy) for session-id %" PRIu64
121+
- "from '%s'",
122+
- fe_msg->session_req->session_id, adapter->name);
123+
-
124+
session = mgmt_session_id2ctx(
125+
fe_msg->session_req->session_id);
126+
- fe_adapter_send_session_reply(adapter, session, false,
127+
- true);
128+
- mgmt_fe_cleanup_session(&session);
129+
+
130+
+ __dbg("Got SESSION_REQ (destroy) for session-id %" PRIu64
131+
+ " from '%s' session_cnt %d session %p config_count %u committed_count %u",
132+
+ fe_msg->session_req->session_id, adapter->name,
133+
+ session_cnt, session, session ? session->config_count : -1,
134+
+ session ? session->committed_count : -1);
135+
+
136+
+ if (session) {
137+
+ assert(session->config_count == 0);
138+
+ fe_adapter_send_session_reply(adapter, session, false,
139+
+ true);
140+
+ mgmt_fe_cleanup_session(&session);
141+
+ session_cnt--;
142+
+ }
143+
}
144+
break;
145+
case MGMTD__FE_MESSAGE__MESSAGE_LOCKDS_REQ:
146+
@@ -979,12 +1001,15 @@ mgmt_fe_adapter_handle_msg(struct mgmt_fe_client_adapter *adapter,
147+
session = mgmt_session_id2ctx(
148+
fe_msg->setcfg_req->session_id);
149+
session->adapter->setcfg_stats.set_cfg_count++;
150+
+ session->config_count++;
151+
__dbg("Got SETCFG_REQ (%d Xpaths, Implicit:%c) on DS:%s for session-id %" PRIu64
152+
- " from '%s'",
153+
- (int)fe_msg->setcfg_req->n_data,
154+
+ " from '%s' session %p config_count %u committed_count %u",
155+
+ (int)fe_msg->setcfg_req->n_data,
156+
fe_msg->setcfg_req->implicit_commit ? 'T' : 'F',
157+
mgmt_ds_id2name(fe_msg->setcfg_req->ds_id),
158+
- fe_msg->setcfg_req->session_id, adapter->name);
159+
+ fe_msg->setcfg_req->session_id, adapter->name,
160+
+ session, session->config_count,
161+
+ session->committed_count);
162+
163+
mgmt_fe_session_handle_setcfg_req_msg(
164+
session, fe_msg->setcfg_req);
165+
--
166+
2.25.1
167+

‎src/sonic-frr/patch/series

+1
Original file line numberDiff line numberDiff line change
@@ -43,3 +43,4 @@
4343
0061-Set-multipath-to-514-and-disable-bgp-vnc-for-optimiz.patch
4444
0062-zebra-lib-use-internal-rbtree-per-ns.patch
4545
0063-lib-Return-duplicate-prefix-list-entry-test.patch
46+
0064-mgmtd-clean-session-config-only-when-it-is-needed.patch

0 commit comments

Comments
 (0)