Skip to content

Commit

Permalink
Revert "Revert frr route check (#2761)" (#2762)
Browse files Browse the repository at this point in the history
This reverts commit 79a21ce.

DEPENDS ON: sonic-net/sonic-buildimage#12853

What I did
Reverted changes back

How I did it
Reverted changes back

How to verify it
UT
  • Loading branch information
stepanblyschak authored May 23, 2023
1 parent 3d89589 commit b4f4e63
Show file tree
Hide file tree
Showing 4 changed files with 248 additions and 16 deletions.
122 changes: 111 additions & 11 deletions scripts/route_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@
How:
NOTE: The flow from APPL-DB to ASIC-DB takes non zero milliseconds.
1) Initiate subscribe for ASIC-DB updates.
2) Read APPL-DB & ASIC-DB
2) Read APPL-DB & ASIC-DB
3) Get the diff.
4) If any diff,
4) If any diff,
4.1) Collect subscribe messages for a second
4.2) check diff against the subscribe messages
4.2) check diff against the subscribe messages
5) Rule out local interfaces & default routes
6) If still outstanding diffs, report failure.
Expand All @@ -29,7 +29,7 @@
down to ensure failure.
Analyze the reported failures to match expected.
You may use the exit code to verify the result as success or not.
"""
Expand All @@ -45,7 +45,9 @@
import time
import signal
import traceback
import subprocess

from ipaddress import ip_network
from swsscommon import swsscommon
from utilities_common import chassis

Expand All @@ -71,6 +73,9 @@

PRINT_MSG_LEN_MAX = 1000

FRR_CHECK_RETRIES = 3
FRR_WAIT_TIME = 15

class Level(Enum):
ERR = 'ERR'
INFO = 'INFO'
Expand Down Expand Up @@ -141,7 +146,7 @@ def add_prefix(ip):
ip = ip + PREFIX_SEPARATOR + "32"
else:
ip = ip + PREFIX_SEPARATOR + "128"
return ip
return str(ip_network(ip))


def add_prefix_ifnot(ip):
Expand All @@ -150,7 +155,7 @@ def add_prefix_ifnot(ip):
:param ip: IP to add prefix as string.
:return ip with prefix
"""
return ip if ip.find(PREFIX_SEPARATOR) != -1 else add_prefix(ip)
return str(ip_network(ip)) if ip.find(PREFIX_SEPARATOR) != -1 else add_prefix(ip)


def is_local(ip):
Expand Down Expand Up @@ -293,7 +298,7 @@ def get_routes():

def get_route_entries():
"""
helper to read present route entries from ASIC-DB and
helper to read present route entries from ASIC-DB and
as well initiate selector for ASIC-DB:ASIC-state updates.
:return (selector, subscriber, <list of sorted routes>)
"""
Expand All @@ -309,14 +314,39 @@ def get_route_entries():
res, e = checkout_rt_entry(k)
if res:
rt.append(e)

print_message(syslog.LOG_DEBUG, json.dumps({"ASIC_ROUTE_ENTRY": sorted(rt)}, indent=4))

selector = swsscommon.Select()
selector.addSelectable(subs)
return (selector, subs, sorted(rt))


def is_suppress_fib_pending_enabled():
"""
Returns True if FIB suppression is enabled, False otherwise
"""
cfg_db = swsscommon.ConfigDBConnector()
cfg_db.connect()

state = cfg_db.get_entry('DEVICE_METADATA', 'localhost').get('suppress-fib-pending')

return state == 'enabled'


def get_frr_routes():
"""
Read routes from zebra through CLI command
:return frr routes dictionary
"""

output = subprocess.check_output('show ip route json', shell=True)
routes = json.loads(output)
output = subprocess.check_output('show ipv6 route json', shell=True)
routes.update(json.loads(output))
return routes


def get_interfaces():
"""
helper to read interface table from APPL-DB.
Expand Down Expand Up @@ -354,7 +384,7 @@ def filter_out_local_interfaces(keys):

chassis_local_intfs = chassis.get_chassis_local_interfaces()
local_if_lst.update(set(chassis_local_intfs))

db = swsscommon.DBConnector(APPL_DB_NAME, 0)
tbl = swsscommon.Table(db, 'ROUTE_TABLE')

Expand Down Expand Up @@ -493,6 +523,61 @@ def filter_out_standalone_tunnel_routes(routes):
return updated_routes


def check_frr_pending_routes():
"""
Check FRR routes for offload flag presence by executing "show ip route json"
Returns a list of routes that have no offload flag.
"""

missed_rt = []

retries = FRR_CHECK_RETRIES
for i in range(retries):
missed_rt = []
frr_routes = get_frr_routes()

for _, entries in frr_routes.items():
for entry in entries:
if entry['protocol'] != 'bgp':
continue

# TODO: Also handle VRF routes. Currently this script does not check for VRF routes so it would be incorrect for us
# to assume they are installed in ASIC_DB, so we don't handle them.
if entry['vrfName'] != 'default':
continue

if not entry.get('offloaded', False):
missed_rt.append(entry)

if not missed_rt:
break

time.sleep(FRR_WAIT_TIME)

return missed_rt


def mitigate_installed_not_offloaded_frr_routes(missed_frr_rt, rt_appl):
"""
Mitigate installed but not offloaded FRR routes.
In case route exists in APPL_DB, this function will manually send a notification to fpmsyncd
to trigger the flow that sends offload flag to zebra.
It is designed to mitigate a problem when orchagent fails to send notification about installed route to fpmsyncd
or fpmsyncd not being able to read the notification or in case zebra fails to receive offload update due to variety of reasons.
All of the above mentioned cases must be considered as a bug, but even in that case we will report an error in the log but
given that this script ensures the route is installed in the hardware it will automitigate such a bug.
"""
db = swsscommon.DBConnector('APPL_STATE_DB', 0)
response_producer = swsscommon.NotificationProducer(db, f'{APPL_DB_NAME}_{swsscommon.APP_ROUTE_TABLE_NAME}_RESPONSE_CHANNEL')
for entry in [entry for entry in missed_frr_rt if entry['prefix'] in rt_appl]:
fvs = swsscommon.FieldValuePairs([('err_str', 'SWSS_RC_SUCCESS'), ('protocol', entry['protocol'])])
response_producer.send('SWSS_RC_SUCCESS', entry['prefix'], fvs)

print_message(syslog.LOG_ERR, f'Mitigated route {entry["prefix"]}')


def get_soc_ips(config_db):
mux_table = config_db.get_table('MUX_CABLE')
soc_ips = []
Expand Down Expand Up @@ -536,7 +621,7 @@ def check_routes():
"""
The heart of this script which runs the checks.
Read APPL-DB & ASIC-DB, the relevant tables for route checking.
Checkout routes in ASIC-DB to match APPL-DB, discounting local &
Checkout routes in ASIC-DB to match APPL-DB, discounting local &
default routes. In case of missed / unexpected entries in ASIC,
it might be due to update latency between APPL & ASIC DBs. So collect
ASIC-DB subscribe updates for a second, and checkout if you see SET
Expand All @@ -545,12 +630,16 @@ def check_routes():
If there are still some unjustifiable diffs, between APPL & ASIC DB,
related to routes report failure, else all good.
If there are FRR routes that aren't marked offloaded but all APPL & ASIC DB
routes are in sync report failure and perform a mitigation action.
:return (0, None) on sucess, else (-1, results) where results holds
the unjustifiable entries.
"""
intf_appl_miss = []
rt_appl_miss = []
rt_asic_miss = []
rt_frr_miss = []

results = {}
adds = []
Expand Down Expand Up @@ -599,11 +688,22 @@ def check_routes():
if rt_asic_miss:
results["Unaccounted_ROUTE_ENTRY_TABLE_entries"] = rt_asic_miss

rt_frr_miss = check_frr_pending_routes()

if rt_frr_miss:
results["missed_FRR_routes"] = rt_frr_miss

if results:
print_message(syslog.LOG_WARNING, "Failure results: {", json.dumps(results, indent=4), "}")
print_message(syslog.LOG_WARNING, "Failed. Look at reported mismatches above")
print_message(syslog.LOG_WARNING, "add: ", json.dumps(adds, indent=4))
print_message(syslog.LOG_WARNING, "del: ", json.dumps(deletes, indent=4))

if rt_frr_miss and not rt_appl_miss and not rt_asic_miss:
print_message(syslog.LOG_ERR, "Some routes are not set offloaded in FRR but all routes in APPL_DB and ASIC_DB are in sync")
if is_suppress_fib_pending_enabled():
mitigate_installed_not_offloaded_frr_routes(rt_frr_miss, rt_appl)

return -1, results
else:
print_message(syslog.LOG_INFO, "All good!")
Expand Down Expand Up @@ -649,7 +749,7 @@ def main():
return ret, res
else:
return ret, res



if __name__ == "__main__":
Expand Down
3 changes: 2 additions & 1 deletion tests/mock_tables/config_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,8 @@
"mac": "1d:34:db:16:a6:00",
"platform": "x86_64-mlnx_msn3800-r0",
"peer_switch": "sonic-switch",
"type": "ToRRouter"
"type": "ToRRouter",
"suppress-fib-pending": "enabled"
},
"SNMP_COMMUNITY|msft": {
"TYPE": "RO"
Expand Down
17 changes: 14 additions & 3 deletions tests/route_check_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import time
from sonic_py_common import device_info
from unittest.mock import MagicMock, patch
from tests.route_check_test_data import APPL_DB, ARGS, ASIC_DB, CONFIG_DB, DEFAULT_CONFIG_DB, DESCR, OP_DEL, OP_SET, PRE, RESULT, RET, TEST_DATA, UPD
from tests.route_check_test_data import APPL_DB, ARGS, ASIC_DB, CONFIG_DB, DEFAULT_CONFIG_DB, DESCR, OP_DEL, OP_SET, PRE, RESULT, RET, TEST_DATA, UPD, FRR_ROUTES

import pytest

Expand Down Expand Up @@ -239,6 +239,7 @@ def setup(self):

def init(self):
route_check.UNIT_TESTING = 1
route_check.FRR_WAIT_TIME = 0

@pytest.fixture
def force_hang(self):
Expand All @@ -258,7 +259,8 @@ def mock_dbs(self):
patch("route_check.swsscommon.Table") as mock_table, \
patch("route_check.swsscommon.Select") as mock_sel, \
patch("route_check.swsscommon.SubscriberStateTable") as mock_subs, \
patch("route_check.swsscommon.ConfigDBConnector", return_value=mock_config_db):
patch("route_check.swsscommon.ConfigDBConnector", return_value=mock_config_db), \
patch("route_check.swsscommon.NotificationProducer"):
device_info.get_platform = MagicMock(return_value='unittest')
set_mock(mock_table, mock_conn, mock_sel, mock_subs, mock_config_db)
yield
Expand All @@ -272,7 +274,16 @@ def test_route_check(self, mock_dbs, test_num):
set_test_case_data(ct_data)
logger.info("Running test case {}: {}".format(test_num, ct_data[DESCR]))

with patch('sys.argv', ct_data[ARGS].split()):
with patch('sys.argv', ct_data[ARGS].split()), \
patch('route_check.subprocess.check_output') as mock_check_output:

routes = ct_data.get(FRR_ROUTES, {})

def side_effect(*args, **kwargs):
return json.dumps(routes)

mock_check_output.side_effect = side_effect

ret, res = route_check.main()
expect_ret = ct_data[RET] if RET in ct_data else 0
expect_res = ct_data[RESULT] if RESULT in ct_data else None
Expand Down
Loading

0 comments on commit b4f4e63

Please sign in to comment.