Revert "Revert frr route check (sonic-net#2761)"

This reverts commit 79a21ce.

scripts/route_check.py

@@ -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.
 
@@ -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.
-    
+
 
 
 """
@@ -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
 
@@ -71,6 +73,9 @@
 
 PRINT_MSG_LEN_MAX = 1000
 
+FRR_CHECK_RETRIES = 3
+FRR_WAIT_TIME = 15
+
 class Level(Enum):
     ERR = 'ERR'
     INFO = 'INFO'
@@ -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):
@@ -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):
@@ -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>)
     """
@@ -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.
@@ -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')
 
@@ -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 = []
@@ -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
@@ -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 = []
@@ -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!")
@@ -649,7 +749,7 @@ def main():
                 return ret, res
         else:
             return ret, res
-            
+
 
 
 if __name__ == "__main__":

tests/mock_tables/config_db.json

@@ -842,7 +842,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"

tests/route_check_test.py

@@ -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
 
@@ -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):
@@ -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
@@ -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