From e70b054604062683f766a28dc295c7443b63c4a1 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak <38952541+stepanblyschak@users.noreply.github.com> Date: Wed, 24 Jan 2024 20:23:30 +0200 Subject: [PATCH] [202311] Revert bgp suppress fib pending (#3109) * Revert "[config/show] Add command to control pending FIB suppression (#2495)" This reverts commit 9126e7f8ab66427096b16c6e305d075767be49eb. * Revert "Revert "Revert frr route check (#2761)" (#2762)" This reverts commit b4f4e63ed7310a1e8684a68eb4a33cc4a559bd6d. --- config/main.py | 16 +--- doc/Command-Reference.md | 38 --------- scripts/route_check.py | 105 +------------------------ show/main.py | 11 --- tests/mock_tables/config_db.json | 3 +- tests/route_check_test.py | 17 +--- tests/route_check_test_data.py | 120 ----------------------------- tests/suppress_pending_fib_test.py | 34 -------- 8 files changed, 9 insertions(+), 335 deletions(-) delete mode 100644 tests/suppress_pending_fib_test.py diff --git a/config/main.py b/config/main.py index 630facb453..3bdb31a10b 100644 --- a/config/main.py +++ b/config/main.py @@ -1771,7 +1771,7 @@ def load_minigraph(db, no_service_restart, traffic_shift_away, override_config, cfggen_namespace_option = ['-n', str(namespace)] clicommon.run_command([db_migrator, '-o', 'set_version'] + cfggen_namespace_option) - # Keep device isolated with TSA + # Keep device isolated with TSA if traffic_shift_away: clicommon.run_command(["TSA"], display_cmd=True) if override_config: @@ -2056,21 +2056,9 @@ 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 ...') -# +# @config.command('yang_config_validation') @click.argument('yang_config_validation', metavar='', required=True) def yang_config_validation(yang_config_validation): diff --git a/doc/Command-Reference.md b/doc/Command-Reference.md index 3e64103a4e..13489eb7d3 100644 --- a/doc/Command-Reference.md +++ b/doc/Command-Reference.md @@ -2447,26 +2447,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 - ``` - Go Back To [Beginning of the document](#) or [Beginning of this section](#bgp) ### BGP config commands @@ -2559,24 +2539,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 - ``` - -- Examples: - ``` - admin@sonic:~$ sudo config suppress-fib-pending enabled - ``` - ``` - admin@sonic:~$ sudo config suppress-fib-pending disabled - ``` - Go Back To [Beginning of the document](#) or [Beginning of this section](#bgp) ## Console diff --git a/scripts/route_check.py b/scripts/route_check.py index 85d0539c63..8af8393f46 100755 --- a/scripts/route_check.py +++ b/scripts/route_check.py @@ -45,9 +45,8 @@ import time import signal import traceback -import subprocess - from ipaddress import ip_network + from swsscommon import swsscommon from utilities_common import chassis @@ -73,9 +72,6 @@ PRINT_MSG_LEN_MAX = 1000 -FRR_CHECK_RETRIES = 3 -FRR_WAIT_TIME = 15 - class Level(Enum): ERR = 'ERR' INFO = 'INFO' @@ -146,7 +142,7 @@ def add_prefix(ip): ip = ip + PREFIX_SEPARATOR + "32" else: ip = ip + PREFIX_SEPARATOR + "128" - return str(ip_network(ip)) + return ip def add_prefix_ifnot(ip): @@ -322,31 +318,6 @@ def get_route_entries(): 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. @@ -523,61 +494,6 @@ 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 = [] @@ -612,7 +528,7 @@ def filter_out_soc_ip_routes(routes): if not soc_ips: return routes - + updated_routes = [] for route in routes: if route not in soc_ips: @@ -679,16 +595,12 @@ 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 = [] @@ -742,22 +654,11 @@ 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!") diff --git a/show/main.py b/show/main.py index 725556e6e8..096e4e4aac 100755 --- a/show/main.py +++ b/show/main.py @@ -2128,17 +2128,6 @@ 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) - - # Load plugins and register them helper = util_base.UtilHelper() helper.load_and_register_plugins(plugins, cli) diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index 2a81f96bfa..8b4b93459d 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -883,8 +883,7 @@ "mac": "1d:34:db:16:a6:00", "platform": "x86_64-mlnx_msn3800-r0", "peer_switch": "sonic-switch", - "type": "ToRRouter", - "suppress-fib-pending": "enabled" + "type": "ToRRouter" }, "SNMP_COMMUNITY|msft": { "TYPE": "RO" diff --git a/tests/route_check_test.py b/tests/route_check_test.py index 118e9eab56..85e6a64a95 100644 --- a/tests/route_check_test.py +++ b/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, FRR_ROUTES +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 import pytest @@ -239,7 +239,6 @@ def setup(self): def init(self): route_check.UNIT_TESTING = 1 - route_check.FRR_WAIT_TIME = 0 @pytest.fixture def force_hang(self): @@ -259,8 +258,7 @@ 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.NotificationProducer"): + patch("route_check.swsscommon.ConfigDBConnector", return_value=mock_config_db): device_info.get_platform = MagicMock(return_value='unittest') set_mock(mock_table, mock_conn, mock_sel, mock_subs, mock_config_db) yield @@ -274,16 +272,7 @@ 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()), \ - 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 - + with patch('sys.argv', ct_data[ARGS].split()): 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 diff --git a/tests/route_check_test_data.py b/tests/route_check_test_data.py index b0f3e9975a..0f0f74bfe0 100644 --- a/tests/route_check_test_data.py +++ b/tests/route_check_test_data.py @@ -6,7 +6,6 @@ CONFIG_DB = 4 PRE = "pre-value" UPD = "update" -FRR_ROUTES = "frr-routes" RESULT = "res" OP_SET = "SET" @@ -363,125 +362,6 @@ } } }, - "10": { - DESCR: "basic good one, check FRR routes", - ARGS: "route_check -m INFO -i 1000", - PRE: { - APPL_DB: { - ROUTE_TABLE: { - "0.0.0.0/0" : { "ifname": "portchannel0" }, - "10.10.196.12/31" : { "ifname": "portchannel0" }, - }, - INTF_TABLE: { - "PortChannel1013:10.10.196.24/31": {}, - "PortChannel1023:2603:10b0:503:df4::5d/126": {}, - "PortChannel1024": {} - } - }, - ASIC_DB: { - RT_ENTRY_TABLE: { - RT_ENTRY_KEY_PREFIX + "10.10.196.12/31" + RT_ENTRY_KEY_SUFFIX: {}, - RT_ENTRY_KEY_PREFIX + "10.10.196.24/32" + RT_ENTRY_KEY_SUFFIX: {}, - RT_ENTRY_KEY_PREFIX + "2603:10b0:503:df4::5d/128" + RT_ENTRY_KEY_SUFFIX: {}, - RT_ENTRY_KEY_PREFIX + "0.0.0.0/0" + RT_ENTRY_KEY_SUFFIX: {} - } - }, - }, - FRR_ROUTES: { - "0.0.0.0/0": [ - { - "prefix": "0.0.0.0/0", - "vrfName": "default", - "protocol": "bgp", - "offloaded": "true", - }, - ], - "10.10.196.12/31": [ - { - "prefix": "10.10.196.12/31", - "vrfName": "default", - "protocol": "bgp", - "offloaded": "true", - }, - ], - "10.10.196.24/31": [ - { - "protocol": "connected", - }, - ], - }, - }, - "11": { - DESCR: "failure test case, missing FRR routes", - ARGS: "route_check -m INFO -i 1000", - PRE: { - APPL_DB: { - ROUTE_TABLE: { - "0.0.0.0/0" : { "ifname": "portchannel0" }, - "10.10.196.12/31" : { "ifname": "portchannel0" }, - }, - INTF_TABLE: { - "PortChannel1013:10.10.196.24/31": {}, - "PortChannel1023:2603:10b0:503:df4::5d/126": {}, - "PortChannel1024": {} - } - }, - ASIC_DB: { - RT_ENTRY_TABLE: { - RT_ENTRY_KEY_PREFIX + "10.10.196.12/31" + RT_ENTRY_KEY_SUFFIX: {}, - RT_ENTRY_KEY_PREFIX + "10.10.196.24/32" + RT_ENTRY_KEY_SUFFIX: {}, - RT_ENTRY_KEY_PREFIX + "2603:10b0:503:df4::5d/128" + RT_ENTRY_KEY_SUFFIX: {}, - RT_ENTRY_KEY_PREFIX + "0.0.0.0/0" + RT_ENTRY_KEY_SUFFIX: {} - } - }, - }, - FRR_ROUTES: { - "0.0.0.0/0": [ - { - "prefix": "0.0.0.0/0", - "vrfName": "default", - "protocol": "bgp", - "offloaded": "true", - }, - ], - "10.10.196.12/31": [ - { - "prefix": "10.10.196.12/31", - "vrfName": "default", - "protocol": "bgp", - }, - ], - "10.10.196.24/31": [ - { - "protocol": "connected", - }, - ], - }, - RESULT: { - "missed_FRR_routes": [ - {"prefix": "10.10.196.12/31", "vrfName": "default", "protocol": "bgp"} - ], - }, - RET: -1, - }, - "10": { - DESCR: "basic good one with IPv6 address", - ARGS: "route_check -m INFO -i 1000", - PRE: { - APPL_DB: { - ROUTE_TABLE: { - }, - INTF_TABLE: { - "PortChannel1013:2000:31:0:0::1/64": {}, - } - }, - ASIC_DB: { - RT_ENTRY_TABLE: { - RT_ENTRY_KEY_PREFIX + "2000:31::1/128" + RT_ENTRY_KEY_SUFFIX: {}, - } - } - } - }, "11": { DESCR: "dualtor ignore vlan neighbor route miss case", ARGS: "route_check -i 15", diff --git a/tests/suppress_pending_fib_test.py b/tests/suppress_pending_fib_test.py deleted file mode 100644 index 04064d306e..0000000000 --- a/tests/suppress_pending_fib_test.py +++ /dev/null @@ -1,34 +0,0 @@ -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