Remove suppress-fib-pending CLI and make route_check.py check suppress-fib in BGP configuration (sonic-net#3331)

What I did

Revert suppress FIB pending feature

Why I did it

Some unresolved FRR issues in current version

How I verified it

Build and run [route_check] check if suppress fib is enabled in bgp

Signed-off-by: Stepan Blyschak <[email protected]>

Author: stepanblyschak

config/main.py

@@ -2336,18 +2336,6 @@ def synchronous_mode(sync_mode):
               config reload -y \n
     Option 2. systemctl restart swss""" % sync_mode)
 
-#
-# 'suppress-fib-pending' command ('config suppress-fib-pending ...')
-#
-@config.command('suppress-fib-pending')
-@click.argument('state', metavar='<enabled|disabled>', required=True, type=click.Choice(['enabled', 'disabled']))
-@clicommon.pass_db
-def suppress_pending_fib(db, state):
-    ''' Enable or disable pending FIB suppression. Once enabled, BGP will not advertise routes that are not yet installed in the hardware '''
-
-    config_db = db.cfgdb
-    config_db.mod_entry('DEVICE_METADATA' , 'localhost', {"suppress-fib-pending" : state})
-
 #
 # 'yang_config_validation' command ('config yang_config_validation ...')
 #

doc/Command-Reference.md

@@ -2610,26 +2610,6 @@ This command displays the routing policy that takes precedence over the other ro
       Exit routemap
   ```
 
-**show suppress-fib-pending**
-
-This command is used to show the status of suppress pending FIB feature.
-When enabled, BGP will not advertise routes which aren't yet offloaded.
-
-- Usage:
-  ```
-  show suppress-fib-pending
-  ```
-
-- Examples:
-  ```
-  admin@sonic:~$ show suppress-fib-pending
-  Enabled
-  ```
-  ```
-  admin@sonic:~$ show suppress-fib-pending
-  Disabled
-  ```
-
 **show bgp device-global**
 
 This command displays BGP device global configuration.
@@ -2742,24 +2722,6 @@ This command is used to remove particular IPv4 or IPv6 BGP neighbor configuratio
   admin@sonic:~$ sudo config bgp remove neighbor SONIC02SPINE
   ```
 
-**config suppress-fib-pending**
-
-This command is used to enable or disable announcements of routes not yet installed in the HW.
-Once enabled, BGP will not advertise routes which aren't yet offloaded.
-
-- Usage:
-  ```
-  config suppress-fib-pending <enabled|disabled>
-  ```
-
-- Examples:
-  ```
-  admin@sonic:~$ sudo config suppress-fib-pending enabled
-  ```
-  ```
-  admin@sonic:~$ sudo config suppress-fib-pending disabled 
-  ```
-
 **config bgp device-global tsa/w-ecmp**
 
 This command is used to manage BGP device global configuration.

scripts/route_check.py

@@ -328,6 +328,16 @@ def get_asicdb_routes(namespace):
     return (selector, subs, sorted(rt))
 
 
+def is_bgp_suppress_fib_pending_enabled(namespace):
+    """
+    Retruns True if FIB suppression is enabled in BGP config, False otherwise
+    """
+    show_run_cmd = ['show', 'runningconfiguration', 'bgp', '-n', namespace]
+
+    output = subprocess.check_output(show_run_cmd, text=True)
+    return 'bgp suppress-fib-pending' in output
+
+
 def is_suppress_fib_pending_enabled(namespace):
     """
     Returns True if FIB suppression is enabled, False otherwise
@@ -781,18 +791,20 @@ def check_routes(namespace):
                 results[namespace] = {}
             results[namespace]["Unaccounted_ROUTE_ENTRY_TABLE_entries"] = rt_asic_miss
 
-        rt_frr_miss = check_frr_pending_routes(namespace)
+        if is_bgp_suppress_fib_pending_enabled(namespace):
+            rt_frr_miss = check_frr_pending_routes(namespace)
 
-        if rt_frr_miss:
-            if namespace not in results:
-                results[namespace] = {}
-            results[namespace]["missed_FRR_routes"] = rt_frr_miss
+            if rt_frr_miss:
+                if namespace not in results:
+                    results[namespace] = {}
+                results[namespace]["missed_FRR_routes"] = rt_frr_miss
 
-        if results:
-            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".format(namespace))
-                if is_suppress_fib_pending_enabled(namespace):
-                    mitigate_installed_not_offloaded_frr_routes(namespace, rt_frr_miss, rt_appl)
+            if results:
+                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".format(namespace))
+                    if is_suppress_fib_pending_enabled(namespace):
+                        mitigate_installed_not_offloaded_frr_routes(namespace, rt_frr_miss, rt_appl)
 
     if results:
         print_message(syslog.LOG_WARNING, "Failure results: {",  json.dumps(results, indent=4), "}")

show/main.py

@@ -165,7 +165,7 @@ def get_config_json_by_namespace(namespace):
 iface_alias_converter = lazy_object_proxy.Proxy(lambda: clicommon.InterfaceAliasConverter())
 
 #
-# Display all storm-control data 
+# Display all storm-control data
 #
 def display_storm_all():
     """ Show storm-control """
@@ -465,7 +465,7 @@ def is_mgmt_vrf_enabled(ctx):
     return False
 
 #
-# 'storm-control' group 
+# 'storm-control' group
 # "show storm-control [interface <interface>]"
 #
 @cli.group('storm-control', invoke_without_command=True)
@@ -2111,7 +2111,7 @@ def summary(db):
             key_values = key.split('|')
             values = db.db.get_all(db.db.STATE_DB, key)
             if "local_discriminator" not in values.keys():
-                values["local_discriminator"] = "NA"            
+                values["local_discriminator"] = "NA"
             bfd_body.append([key_values[3], key_values[2], key_values[1], values["state"], values["type"], values["local_addr"],
                                 values["tx_interval"], values["rx_interval"], values["multiplier"], values["multihop"], values["local_discriminator"]])
 
@@ -2142,24 +2142,13 @@ def peer(db, peer_ip):
             key_values = key.split(delimiter)
             values = db.db.get_all(db.db.STATE_DB, key)
             if "local_discriminator" not in values.keys():
-                values["local_discriminator"] = "NA"            
+                values["local_discriminator"] = "NA"
             bfd_body.append([key_values[3], key_values[2], key_values[1], values.get("state"), values.get("type"), values.get("local_addr"),
                                 values.get("tx_interval"), values.get("rx_interval"), values.get("multiplier"), values.get("multihop"), values.get("local_discriminator")])
 
     click.echo(tabulate(bfd_body, bfd_headers))
 
 
-# 'suppress-fib-pending' subcommand ("show suppress-fib-pending")
-@cli.command('suppress-fib-pending')
-@clicommon.pass_db
-def suppress_pending_fib(db):
-    """ Show the status of suppress pending FIB feature """
-
-    field_values = db.cfgdb.get_entry('DEVICE_METADATA', 'localhost')
-    state = field_values.get('suppress-fib-pending', 'disabled').title()
-    click.echo(state)
-
-
 # asic-sdk-health-event subcommand ("show asic-sdk-health-event")
 @cli.group(cls=clicommon.AliasedGroup)
 def asic_sdk_health_event():

tests/route_check_test.py

@@ -252,8 +252,11 @@ def run_test(self, ct_data):
 
     def mock_check_output(self, ct_data, *args, **kwargs):
         ns = self.extract_namespace_from_args(args[0])
-        routes = ct_data.get(FRR_ROUTES, {}).get(ns, {})
-        return json.dumps(routes)
+        if 'show runningconfiguration bgp' in ' '.join(args[0]):
+            return 'bgp suppress-fib-pending'
+        else:
+            routes = ct_data.get(FRR_ROUTES, {}).get(ns, {})
+            return json.dumps(routes)
 
     def assert_results(self, ct_data, ret, res):
         expect_ret = ct_data.get(RET, 0)