Skip to content

Commit 075280a

Browse files
yxiecayxieca
committed
[FRR] fix FRR mgmtd losing configuration issue
mgmtd configuration management has an issue where any session can clean up outstanding configuration upon destruction. When a long-lived session is taking configuration changes, and another short-lived session which never took any configuration closes, the outstanding configuration would be lost because the configuration clearing doesn't have protection during session closing. This change keeps track if a session has received any configuration, and if the configuration has been applied or cleared. The outstanding configuration should be applied or cleared before session closure (assertion). When clearing the outstanding session structure, only attempt to clear configuration when the closing session has outstanding configurations. Signed-off-by: Ying Xie <[email protected]>
1 parent e523d51 commit 075280a

File tree

2 files changed

+168
-0
lines changed

2 files changed

+168
-0
lines changed

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

Lines changed: 167 additions & 0 deletions
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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,4 @@
4242
0060-bgpd-Validate-both-nexthop-information-NEXTHOP-and-N.patch
4343
0061-Set-multipath-to-514-and-disable-bgp-vnc-for-optimiz.patch
4444
0062-zebra-lib-use-internal-rbtree-per-ns.patch
45+
0063-mgmtd-clean-session-config-only-when-it-is-needed.patch

0 commit comments

Comments
 (0)