From ee906811f33f213b4d5358c2cca30a4b0e54f2c6 Mon Sep 17 00:00:00 2001 From: Sudharsan Dhamal Gopalarathnam Date: Mon, 2 Sep 2024 06:44:18 -0700 Subject: [PATCH] Revert "Remove suppress-fib-pending CLI and make route_check.py check suppress-fib in BGP configuration" (#3477) Reverts #3331 BGP zebra enhancements is merged to master branch sonic-net/sonic-buildimage#19717 Reverting the revert of bgp suppress pending feature to enable it in master branch --- config/main.py | 14 +++++++++++ doc/Command-Reference.md | 38 ++++++++++++++++++++++++++++++ scripts/route_check.py | 35 ++++++++++----------------- show/main.py | 11 +++++++++ tests/route_check_test.py | 7 ++---- tests/suppress_pending_fib_test.py | 34 ++++++++++++++++++++++++++ 6 files changed, 111 insertions(+), 28 deletions(-) create mode 100644 tests/suppress_pending_fib_test.py diff --git a/config/main.py b/config/main.py index 7509628a67..0399639d97 100644 --- a/config/main.py +++ b/config/main.py @@ -2376,6 +2376,20 @@ 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='', 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 ...') # diff --git a/doc/Command-Reference.md b/doc/Command-Reference.md index 689ca23b73..cdc3f5644d 100644 --- a/doc/Command-Reference.md +++ b/doc/Command-Reference.md @@ -2612,6 +2612,26 @@ 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. @@ -2724,6 +2744,24 @@ 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 + ``` + +- 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. diff --git a/scripts/route_check.py b/scripts/route_check.py index 2fbe041547..a1abd3c352 100755 --- a/scripts/route_check.py +++ b/scripts/route_check.py @@ -328,16 +328,6 @@ 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 @@ -791,20 +781,19 @@ def check_routes(namespace): results[namespace] = {} results[namespace]["Unaccounted_ROUTE_ENTRY_TABLE_entries"] = rt_asic_miss - 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 + rt_frr_miss = check_frr_pending_routes(namespace) - 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 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: print_message(syslog.LOG_WARNING, "Failure results: {", json.dumps(results, indent=4), "}") diff --git a/show/main.py b/show/main.py index c9e5e2086c..8d3f117b2f 100755 --- a/show/main.py +++ b/show/main.py @@ -2159,6 +2159,17 @@ def peer(db, peer_ip): 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(): diff --git a/tests/route_check_test.py b/tests/route_check_test.py index 26c632d742..1f92b3d19a 100644 --- a/tests/route_check_test.py +++ b/tests/route_check_test.py @@ -252,11 +252,8 @@ def run_test(self, ct_data): def mock_check_output(self, ct_data, *args, **kwargs): ns = self.extract_namespace_from_args(args[0]) - 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) + 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) diff --git a/tests/suppress_pending_fib_test.py b/tests/suppress_pending_fib_test.py new file mode 100644 index 0000000000..b4dcc7d4bc --- /dev/null +++ b/tests/suppress_pending_fib_test.py @@ -0,0 +1,34 @@ +from click.testing import CliRunner + +import config.main as config +import show.main as show +from utilities_common.db import Db + + +class TestSuppressFibPending: + def test_synchronous_mode(self): + runner = CliRunner() + + db = Db() + + result = runner.invoke(config.config.commands['suppress-fib-pending'], ['enabled'], obj=db) + print(result.output) + assert result.exit_code == 0 + assert db.cfgdb.get_entry('DEVICE_METADATA', 'localhost')['suppress-fib-pending'] == 'enabled' + + result = runner.invoke(show.cli.commands['suppress-fib-pending'], obj=db) + assert result.exit_code == 0 + assert result.output == 'Enabled\n' + + result = runner.invoke(config.config.commands['suppress-fib-pending'], ['disabled'], obj=db) + print(result.output) + assert result.exit_code == 0 + assert db.cfgdb.get_entry('DEVICE_METADATA', 'localhost')['suppress-fib-pending'] == 'disabled' + + result = runner.invoke(show.cli.commands['suppress-fib-pending'], obj=db) + assert result.exit_code == 0 + assert result.output == 'Disabled\n' + + result = runner.invoke(config.config.commands['suppress-fib-pending'], ['invalid-input'], obj=db) + print(result.output) + assert result.exit_code != 0