Skip to content

Commit 166f05c

Browse files
dypetdypet
committedJan 14, 2025
Address review comments.
1 parent 4a02b5b commit 166f05c

File tree

2 files changed

+79
-45
lines changed

2 files changed

+79
-45
lines changed
 

‎src/sonic-bgpcfgd/bgpcfgd/managers_bfd.py

+13-13
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ def __init__(self, common_objs, db, table):
2828
table,
2929
)
3030

31-
self.check_and_start_bfdd()
32-
self.bfd_sessions = self.load_bfd_sessions()
31+
if(self.check_and_start_bfdd()):
32+
self.bfd_sessions = self.load_bfd_sessions()
3333

3434
def check_and_start_bfdd(self):
3535
"""
@@ -44,9 +44,9 @@ def check_and_start_bfdd(self):
4444
# Start bfdd process
4545
log_warn("bfdd process is not running, starting now...")
4646
command = ["/usr/lib/frr/bfdd", "-A", "127.0.0.1", "-d"]
47-
ret_code, out, err = run_command(command)
48-
if ret_code != 0:
49-
log_err("Can't start bfdd: %s" % err)
47+
proc_out = subprocess.run(command)
48+
if proc_out.returncode != 0:
49+
log_err("Can't start bfdd: %s" % proc_out.returncode)
5050
return False
5151

5252
return True
@@ -67,8 +67,8 @@ def get_def_res_fields(self):
6767
'multihop': False,
6868
'local': '',
6969
'detect-multiplier': self.MULTIPLIER,
70-
'receive-interval': self.RX_INTERVAL,
71-
'transmit-interval': self.TX_INTERVAL,
70+
'receive-interval_ms': self.RX_INTERVAL,
71+
'transmit-interval_ms': self.TX_INTERVAL,
7272
'passive-mode': True,
7373
}
7474

@@ -97,10 +97,10 @@ def redis_to_local_res(self, data):
9797
res_data["detect-multiplier"] = int(data["multiplier"])
9898

9999
if "rx_interval" in data:
100-
res_data["receive-interval"] = int(data["rx_interval"])
100+
res_data["receive-interval_ms"] = int(data["rx_interval"])
101101

102102
if "tx_interval" in data:
103-
res_data["transmit-interval"] = int(data["tx_interval"])
103+
res_data["transmit-interval_ms"] = int(data["tx_interval"])
104104

105105
if "type" in data:
106106
if data["type"] == "async_active":
@@ -142,9 +142,9 @@ def create_bfd_peer_cmd(self, session_key, res_data, is_delete):
142142
bfd_cmds.append("-c")
143143
bfd_cmds.append("detect-multiplier " + str(res_data["detect-multiplier"]))
144144
bfd_cmds.append("-c")
145-
bfd_cmds.append("receive-interval " + str(res_data["receive-interval"]))
145+
bfd_cmds.append("receive-interval " + str(res_data["receive-interval_ms"]))
146146
bfd_cmds.append("-c")
147-
bfd_cmds.append("transmit-interval " + str(res_data["transmit-interval"]))
147+
bfd_cmds.append("transmit-interval " + str(res_data["transmit-interval_ms"]))
148148

149149
bfd_cmds.append("-c")
150150
if (res_data["passive-mode"] == True):
@@ -168,7 +168,7 @@ def add_frr_session(self, session_key, data):
168168

169169
ret_code, out, err = run_command(command)
170170
if ret_code != 0:
171-
log_err("Can't add bfd session: %s" % err)
171+
log_err("Can't add bfd session: %s, err: %s" % (str(session_key), err))
172172
return False
173173

174174
# Add the new session's key to local session DB
@@ -207,7 +207,7 @@ def update_frr_session(self, session_key, data):
207207
if command != "":
208208
ret_code, out, err = run_command(command)
209209
if ret_code != 0:
210-
log_err("Can't update bfd session: %s" % err)
210+
log_err("Can't update bfd session: %s, err: %s" % (str(session_key), err))
211211
return False
212212

213213
# update session in local session DB

‎src/sonic-bgpcfgd/tests/test_bfdmgr.py

+66-32
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ def bfd_mgr(mocked_check_output, mocked_run_command):
3737
def test_constructor(bfd_mgr):
3838
assert len(bfd_mgr.bfd_sessions) == 0
3939

40-
@patch('bgpcfgd.managers_bfd.run_command', return_value=(0, '', ''))
40+
@patch('subprocess.run', return_value=subprocess.CompletedProcess(args=[], returncode=0, stdout=''))
4141
@patch('bgpcfgd.managers_bfd.subprocess.check_output', side_effect=subprocess.CalledProcessError(1, 'pgrep'))
4242
@patch('bgpcfgd.managers_bfd.log_warn')
4343
def test_check_and_start_bfdd_stopped(mocked_log_warn, mocked_check_output, mocked_run_command, bfd_mgr):
@@ -48,7 +48,7 @@ def test_check_and_start_bfdd_stopped(mocked_log_warn, mocked_check_output, mock
4848
mocked_run_command.assert_called_once_with(cmd)
4949
mocked_log_warn.assert_called_with("bfdd process is not running, starting now...")
5050

51-
@patch('bgpcfgd.managers_bfd.run_command', return_value=(1, '', 'Error'))
51+
@patch('subprocess.run', return_value=subprocess.CompletedProcess(args=[], returncode=1, stdout=''))
5252
@patch('bgpcfgd.managers_bfd.subprocess.check_output', side_effect=subprocess.CalledProcessError(1, 'pgrep'))
5353
@patch('bgpcfgd.managers_bfd.log_err')
5454
@patch('bgpcfgd.managers_bfd.log_warn')
@@ -59,16 +59,16 @@ def test_check_and_start_bfdd_error(mocked_log_warn, mocked_log_err, mocked_chec
5959
assert bfd_mgr.check_and_start_bfdd() == False
6060
mocked_run_command.assert_called_once_with(cmd)
6161
mocked_log_warn.assert_called_with("bfdd process is not running, starting now...")
62-
mocked_log_err.assert_called_with("Can't start bfdd: Error")
62+
mocked_log_err.assert_called_with("Can't start bfdd: 1")
6363

6464
def test_get_def_res_fields(bfd_mgr):
6565
result = bfd_mgr.get_def_res_fields()
6666
expected_result = {
6767
'multihop': False,
6868
'local': '',
6969
'detect-multiplier': 3,
70-
'receive-interval': 200,
71-
'transmit-interval': 200,
70+
'receive-interval_ms': 200,
71+
'transmit-interval_ms': 200,
7272
'passive-mode': True,
7373
}
7474
assert result == expected_result
@@ -87,8 +87,8 @@ def test_redis_to_local_res(bfd_mgr):
8787
'multihop': False,
8888
'local': '127.0.0.1',
8989
'detect-multiplier': 5,
90-
'receive-interval': 300,
91-
'transmit-interval': 300,
90+
'receive-interval_ms': 300,
91+
'transmit-interval_ms': 300,
9292
'passive-mode': False,
9393
}
9494
assert result == expected_result
@@ -106,8 +106,8 @@ def test_redis_to_local_res_no_mh(bfd_mgr):
106106
'multihop': False,
107107
'local': '127.0.0.1',
108108
'detect-multiplier': 5,
109-
'receive-interval': 300,
110-
'transmit-interval': 300,
109+
'receive-interval_ms': 300,
110+
'transmit-interval_ms': 300,
111111
'passive-mode': True,
112112
}
113113
assert result == expected_result
@@ -121,8 +121,8 @@ def test_redis_to_local_res_basic(bfd_mgr):
121121
'multihop': False,
122122
'local': '127.0.0.1',
123123
'detect-multiplier': 3,
124-
'receive-interval': 200,
125-
'transmit-interval': 200,
124+
'receive-interval_ms': 200,
125+
'transmit-interval_ms': 200,
126126
'passive-mode': True,
127127
}
128128
assert result == expected_result
@@ -156,7 +156,7 @@ def test_add_frr_session_failure(mocked_log_err, mocked_run_command, bfd_mgr):
156156
"type": "async_active",
157157
}
158158
assert bfd_mgr.add_frr_session(session_key, data) == False
159-
mocked_log_err.assert_called_with("Can't add bfd session: Error")
159+
mocked_log_err.assert_called_with("Can't add bfd session: {'vrf': 'default', 'interface': 'default', 'peer': '10.0.0.1'}, err: Error")
160160
assert len(bfd_mgr.bfd_sessions) == 0
161161

162162
@patch('bgpcfgd.managers_bfd.run_command', return_value=(0, '', ''))
@@ -166,8 +166,8 @@ def test_del_frr_session_success(mocked_run_command, bfd_mgr):
166166
'multihop': True,
167167
'local': '127.0.0.1',
168168
'detect-multiplier': 3,
169-
'receive-interval': 200,
170-
'transmit-interval': 200,
169+
'receive-interval_ms': 200,
170+
'transmit-interval_ms': 200,
171171
'passive-mode': False,
172172
}
173173
bfd_mgr.bfd_sessions[bfd_mgr.dict_to_fs(session_key)] = local_res_data
@@ -188,8 +188,8 @@ def test_del_frr_session_failure(mocked_log_err, mocked_run_command, bfd_mgr):
188188
'multihop': True,
189189
'local': '127.0.0.1',
190190
'detect-multiplier': 3,
191-
'receive-interval': 200,
192-
'transmit-interval': 200,
191+
'receive-interval_ms': 200,
192+
'transmit-interval_ms': 200,
193193
'passive-mode': False,
194194
}
195195
bfd_mgr.bfd_sessions[bfd_mgr.dict_to_fs(session_key)] = local_res_data
@@ -207,8 +207,8 @@ def test_load_bfd_sessions(mocked_log_err, mocked_run_command, bfd_mgr):
207207
"local": "180.1.1.1",
208208
"vrf": "default",
209209
"detect-multiplier": 5,
210-
"transmit-interval": 500,
211-
"receive-interval": 200,
210+
"transmit-interval_ms": 500,
211+
"receive-interval_ms": 200,
212212
"passive-mode": true
213213
},
214214
{
@@ -220,9 +220,27 @@ def test_load_bfd_sessions(mocked_log_err, mocked_run_command, bfd_mgr):
220220
"local": "180.1.1.2",
221221
"vrf": "default",
222222
"detect-multiplier": 6,
223-
"transmit-interval": 100,
224-
"receive-interval": 300,
223+
"transmit-interval_ms": 100,
224+
"receive-interval_ms": 300,
225+
"passive-mode": false
226+
},
227+
{
228+
"peer": "105::109",
229+
"local": "180::220",
230+
"vrf": "default",
231+
"detect-multiplier": 4,
232+
"transmit-interval_ms": 900,
233+
"receive-interval_ms": 700,
225234
"passive-mode": false
235+
},
236+
{
237+
"peer": "110::101",
238+
"local": "190::57",
239+
"vrf": "default",
240+
"detect-multiplier": 3,
241+
"transmit-interval_ms": 100,
242+
"receive-interval_ms": 100,
243+
"passive-mode": true
226244
}
227245
]
228246
'''
@@ -232,23 +250,39 @@ def test_load_bfd_sessions(mocked_log_err, mocked_run_command, bfd_mgr):
232250
'local': '180.1.1.1',
233251
'multihop': False,
234252
'passive-mode': True,
235-
'receive-interval': 200,
236-
'transmit-interval': 500
253+
'receive-interval_ms': 200,
254+
'transmit-interval_ms': 500
237255
},
238256
frozenset({('peer', '192.168.1.2'), ('vrf', 'default'), ('interface', 'default')}): {
239257
'detect-multiplier': 6,
240258
'local': '180.1.1.2',
241259
'multihop': False,
242260
'passive-mode': False,
243-
'receive-interval': 300,
244-
'transmit-interval': 100
261+
'receive-interval_ms': 300,
262+
'transmit-interval_ms': 100
263+
},
264+
frozenset({('peer', '105::109'), ('vrf', 'default'), ('interface', 'default')}): {
265+
'detect-multiplier': 4,
266+
'local': '180::220',
267+
'multihop': False,
268+
'passive-mode': False,
269+
'receive-interval_ms': 700,
270+
'transmit-interval_ms': 900
271+
},
272+
frozenset({('peer', '110::101'), ('vrf', 'default'), ('interface', 'default')}): {
273+
'detect-multiplier': 3,
274+
'local': '190::57',
275+
'multihop': False,
276+
'passive-mode': True,
277+
'receive-interval_ms': 100,
278+
'transmit-interval_ms': 100
245279
},
246280
}
247281
mocked_run_command.return_value = (0, bfd_frr_json, "")
248282
bfd_sessions = bfd_mgr.load_bfd_sessions()
249283
mocked_log_err.assert_called_with("Peer is not set in frr bfd session, skipping")
250284
mocked_run_command.assert_called_once_with(["vtysh", "-c", "show bfd peers json"])
251-
assert len(bfd_sessions) == 2
285+
assert len(bfd_sessions) == 4
252286
assert bfd_sessions == expected_sessions
253287

254288
@patch('bgpcfgd.managers_bfd.run_command', return_value=(1, '', 'Error'))
@@ -265,8 +299,8 @@ def test_update_frr_session_success(mocked_run_command, bfd_mgr):
265299
'multihop': True,
266300
'local': '127.0.0.1',
267301
'detect-multiplier': 3,
268-
'receive-interval': 200,
269-
'transmit-interval': 200,
302+
'receive-interval_ms': 200,
303+
'transmit-interval_ms': 200,
270304
'passive-mode': False,
271305
}
272306
data = {
@@ -316,7 +350,7 @@ def test_update_frr_session_failure(mocked_log_warn, mocked_log_err, mocked_run_
316350
#Error while running command
317351
data["local_addr"] = "127.0.0.2"
318352
assert bfd_mgr.update_frr_session(session_key, data) == False
319-
mocked_log_err.assert_called_with("Can't update bfd session: Error")
353+
mocked_log_err.assert_called_with("Can't update bfd session: {'vrf': 'default', 'interface': 'default', 'peer': '10.0.0.1'}, err: Error")
320354

321355
@patch('bgpcfgd.managers_bfd.run_command', return_value=(0, '', ''))
322356
@patch('bgpcfgd.managers_bfd.log_warn')
@@ -366,8 +400,8 @@ def test_set_handler_upd(mocked_log_warn, mocked_run_command, bfd_mgr):
366400
'multihop': True,
367401
'local': '127.0.0.1',
368402
'detect-multiplier': 3,
369-
'receive-interval': 200,
370-
'transmit-interval': 200,
403+
'receive-interval_ms': 200,
404+
'transmit-interval_ms': 200,
371405
'passive-mode': False,
372406
}
373407
data = {
@@ -399,8 +433,8 @@ def test_del_handler(mocked_log_warn, mocked_run_command, bfd_mgr):
399433
'multihop': True,
400434
'local': '127.0.0.1',
401435
'detect-multiplier': 3,
402-
'receive-interval': 200,
403-
'transmit-interval': 200,
436+
'receive-interval_ms': 200,
437+
'transmit-interval_ms': 200,
404438
'passive-mode': False,
405439
}
406440
data = {

0 commit comments

Comments
 (0)
Please sign in to comment.