From 9b84294ffc3f3257e718da546e597e6d8826faea Mon Sep 17 00:00:00 2001 From: Guohan Lu Date: Thu, 26 May 2022 06:20:40 -0700 Subject: [PATCH] Revert "[bgpcfgd] ECMP overlay VxLan with BGP support (#10716)" This reverts commit 35c9becc3ca670c33c6e5fa691489ec5df2e3c7f. --- src/sonic-bgpcfgd/bgpcfgd/main.py | 2 - .../bgpcfgd/managers_advertise_rt.py | 83 +++++++------------ src/sonic-bgpcfgd/bgpcfgd/managers_rm.py | 78 ----------------- src/sonic-bgpcfgd/tests/test_advertise_rt.py | 72 ++-------------- src/sonic-bgpcfgd/tests/test_rm.py | 62 -------------- 5 files changed, 34 insertions(+), 263 deletions(-) delete mode 100644 src/sonic-bgpcfgd/bgpcfgd/managers_rm.py delete mode 100644 src/sonic-bgpcfgd/tests/test_rm.py diff --git a/src/sonic-bgpcfgd/bgpcfgd/main.py b/src/sonic-bgpcfgd/bgpcfgd/main.py index 698e1756e654..52d591d1fcb0 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/main.py +++ b/src/sonic-bgpcfgd/bgpcfgd/main.py @@ -17,7 +17,6 @@ from .managers_intf import InterfaceMgr from .managers_setsrc import ZebraSetSrc from .managers_static_rt import StaticRouteMgr -from .managers_rm import RouteMapMgr from .runner import Runner, signal_handler from .template import TemplateFabric from .utils import read_constants @@ -61,7 +60,6 @@ def do_work(): StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE"), # Route Advertisement Managers AdvertiseRouteMgr(common_objs, "STATE_DB", swsscommon.STATE_ADVERTISE_NETWORK_TABLE_NAME), - RouteMapMgr(common_objs, "APPL_DB", swsscommon.APP_BGP_PROFILE_TABLE_NAME), ] runner = Runner(common_objs['cfg_mgr']) for mgr in managers: diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py index 68c48b044f61..352b89f7286c 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_advertise_rt.py @@ -1,14 +1,10 @@ from .manager import Manager from .template import TemplateFabric from swsscommon import swsscommon -from .managers_rm import ROUTE_MAPS -import ipaddress -from .log import log_info, log_err, log_debug class AdvertiseRouteMgr(Manager): """ This class Advertises routes when ADVERTISE_NETWORK_TABLE in STATE_DB is updated """ - def __init__(self, common_objs, db, table): """ Initialize the object @@ -22,105 +18,82 @@ def __init__(self, common_objs, db, table): db, table, ) - + self.directory.subscribe([("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], self.on_bgp_asn_change) self.advertised_routes = dict() - OP_DELETE = "DELETE" - OP_ADD = "ADD" + OP_DELETE = 'DELETE' + OP_ADD = 'ADD' + def set_handler(self, key, data): - log_debug("AdvertiseRouteMgr:: set handler") - if not self.__set_handler_validate(key, data): - return True vrf, ip_prefix = self.split_key(key) - self.add_route_advertisement(vrf, ip_prefix, data) + self.add_route_advertisement(vrf, ip_prefix) return True + def del_handler(self, key): - log_debug("AdvertiseRouteMgr:: del handler") vrf, ip_prefix = self.split_key(key) self.remove_route_advertisement(vrf, ip_prefix) - def __set_handler_validate(self, key, data): - if data: - if ("profile" in data and data["profile"] in ROUTE_MAPS) or data == {"":""}: - """ - APP which config the data should be responsible to pass a valid IP prefix - """ - return True - - log_err("BGPAdvertiseRouteMgr:: Invalid data %s for advertised route %s" % (data, key)) - return False - - def add_route_advertisement(self, vrf, ip_prefix, data): + + def add_route_advertisement(self, vrf, ip_prefix): if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): - if not self.advertised_routes.get(vrf, dict()): + if not self.advertised_routes.get(vrf, set()): self.bgp_network_import_check_commands(vrf, self.OP_ADD) - self.advertise_route_commands(ip_prefix, vrf, self.OP_ADD, data) + self.advertise_route_commands(ip_prefix, vrf, self.OP_ADD) + + self.advertised_routes.setdefault(vrf, set()).add(ip_prefix) - self.advertised_routes.setdefault(vrf, dict()).update({ip_prefix: data}) def remove_route_advertisement(self, vrf, ip_prefix): - if ip_prefix not in self.advertised_routes.get(vrf, dict()): - log_info("BGPAdvertiseRouteMgr:: %s|%s does not exist" % (vrf, ip_prefix)) - return - self.advertised_routes.get(vrf, dict()).pop(ip_prefix) - if not self.advertised_routes.get(vrf, dict()): + self.advertised_routes.setdefault(vrf, set()).discard(ip_prefix) + if not self.advertised_routes.get(vrf, set()): self.advertised_routes.pop(vrf, None) if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): - if not self.advertised_routes.get(vrf, dict()): + if not self.advertised_routes.get(vrf, set()): self.bgp_network_import_check_commands(vrf, self.OP_DELETE) self.advertise_route_commands(ip_prefix, vrf, self.OP_DELETE) - def advertise_route_commands(self, ip_prefix, vrf, op, data=None): + + def advertise_route_commands(self, ip_prefix, vrf, op): is_ipv6 = TemplateFabric.is_ipv6(ip_prefix) bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] cmd_list = [] - if vrf == "default": + if vrf == 'default': cmd_list.append("router bgp %s" % bgp_asn) else: cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) cmd_list.append(" address-family %s unicast" % ("ipv6" if is_ipv6 else "ipv4")) - - if data and "profile" in data: - cmd_list.append(" network %s route-map %s" % (ip_prefix, "%s_RM" % data["profile"])) - log_debug( - "BGPAdvertiseRouteMgr:: Update bgp %s network %s with route-map %s" - % (bgp_asn, vrf + "|" + ip_prefix, "%s_RM" % data["profile"]) - ) - else: - cmd_list.append(" %snetwork %s" % ("no " if op == self.OP_DELETE else "", ip_prefix)) - log_debug( - "BGPAdvertiseRouteMgr:: %sbgp %s network %s" - % ("Remove " if op == self.OP_DELETE else "Update ", bgp_asn, vrf + "|" + ip_prefix) - ) + cmd_list.append(" %snetwork %s" % ('no ' if op == self.OP_DELETE else '', ip_prefix)) self.cfg_mgr.push_list(cmd_list) - log_debug("BGPAdvertiseRouteMgr::Done") + def bgp_network_import_check_commands(self, vrf, op): bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] cmd_list = [] - if vrf == "default": + if vrf == 'default': cmd_list.append("router bgp %s" % bgp_asn) else: cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) - cmd_list.append(" %sbgp network import-check" % ("" if op == self.OP_DELETE else "no ")) + cmd_list.append(" %sbgp network import-check" % ('' if op == self.OP_DELETE else 'no ')) self.cfg_mgr.push_list(cmd_list) + def on_bgp_asn_change(self): if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): for vrf, ip_prefixes in self.advertised_routes.items(): self.bgp_network_import_check_commands(vrf, self.OP_ADD) for ip_prefix in ip_prefixes: - self.add_route_advertisement(vrf, ip_prefix, ip_prefixes[ip_prefix]) + self.add_route_advertisement(vrf, ip_prefix) + @staticmethod def split_key(key): @@ -129,7 +102,7 @@ def split_key(key): :param key: key to split :return: vrf name extracted from the key, ip prefix extracted from the key """ - if "|" not in key: - return "default", key + if '|' not in key: + return 'default', key else: - return tuple(key.split("|", 1)) + return tuple(key.split('|', 1)) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py b/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py deleted file mode 100644 index 08609c68f9a6..000000000000 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_rm.py +++ /dev/null @@ -1,78 +0,0 @@ -from .manager import Manager -from .log import log_err, log_debug - -ROUTE_MAPS = ["FROM_SDN_SLB_ROUTES"] - - -class RouteMapMgr(Manager): - """This class add route-map when BGP_PROFILE_TABLE in APPL_DB is updated""" - - def __init__(self, common_objs, db, table): - """ - Initialize the object - :param common_objs: common object dictionary - :param db: name of the db - :param table: name of the table in the db - """ - super(RouteMapMgr, self).__init__( - common_objs, - [], - db, - table, - ) - - def set_handler(self, key, data): - log_debug("BGPRouteMapMgr:: set handler") - """Only need a name as the key, and community id as the data""" - if not self.__set_handler_validate(key, data): - return True - - self.__update_rm(key, data) - return True - - def del_handler(self, key): - log_debug("BGPRouteMapMgr:: del handler") - if not self.__del_handler_validate(key): - return - self.__remove_rm(key) - - def __remove_rm(self, rm): - cmds = ["no route-map %s permit 100" % ("%s_RM" % rm)] - log_debug("BGPRouteMapMgr:: remove route-map %s" % ("%s_RM" % rm)) - self.cfg_mgr.push_list(cmds) - log_debug("BGPRouteMapMgr::Done") - - def __set_handler_validate(self, key, data): - if key not in ROUTE_MAPS: - log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key) - return False - - if not data: - log_err("BGPRouteMapMgr:: data is None") - return False - community_ids = data["community_id"].split(":") - try: - if ( - len(community_ids) != 2 - or int(community_ids[0]) not in range(0, 65536) - or int(community_ids[1]) not in range(0, 65536) - ): - log_err("BGPRouteMapMgr:: data %s doesn't include valid community id %s" % (data, community_ids)) - return False - except ValueError: - log_err("BGPRouteMapMgr:: data %s includes illegal input" % (data)) - return False - - return True - - def __del_handler_validate(self, key): - if key not in ROUTE_MAPS: - log_err("BGPRouteMapMgr:: Invalid key for route-map %s" % key) - return False - return True - - def __update_rm(self, rm, data): - cmds = ["route-map %s permit 100" % ("%s_RM" % rm), " set community %s" % data["community_id"]] - log_debug("BGPRouteMapMgr:: update route-map %s community %s" % ("%s_RM" % rm, data["community_id"])) - self.cfg_mgr.push_list(cmds) - log_debug("BGPRouteMapMgr::Done") diff --git a/src/sonic-bgpcfgd/tests/test_advertise_rt.py b/src/sonic-bgpcfgd/tests/test_advertise_rt.py index 751540600006..26f7b6617650 100644 --- a/src/sonic-bgpcfgd/tests/test_advertise_rt.py +++ b/src/sonic-bgpcfgd/tests/test_advertise_rt.py @@ -48,7 +48,7 @@ def test_set_del(): set_del_test( mgr, "SET", - ("10.1.0.0/24", {"":""}), + ("10.1.0.0/24", {}), True, [ ["router bgp 65100", @@ -62,7 +62,7 @@ def test_set_del(): set_del_test( mgr, "SET", - ("fc00:10::/64", {"":""}), + ("fc00:10::/64", {}), True, [ ["router bgp 65100", @@ -103,7 +103,7 @@ def test_set_del_vrf(): set_del_test( mgr, "SET", - ("vrfRED|10.2.0.0/24", {"":""}), + ("vrfRED|10.2.0.0/24", {}), True, [ ["router bgp 65100 vrf vrfRED", @@ -117,7 +117,7 @@ def test_set_del_vrf(): set_del_test( mgr, "SET", - ("vrfRED|fc00:20::/64", {"":""}), + ("vrfRED|fc00:20::/64", {}), True, [ ["router bgp 65100 vrf vrfRED", @@ -158,9 +158,7 @@ def test_set_del_bgp_asn_change(): set_del_test( mgr, "SET", - ("vrfRED|10.3.0.0/24", { - "profile": "FROM_SDN_SLB_ROUTES" - }), + ("vrfRED|10.3.0.0/24", {}), True, [] ) @@ -172,7 +170,7 @@ def test_set_del_bgp_asn_change(): " no bgp network import-check"], ["router bgp 65100 vrf vrfRED", " address-family ipv4 unicast", - " network 10.3.0.0/24 route-map FROM_SDN_SLB_ROUTES_RM"] + " network 10.3.0.0/24"] ] def push_list(cmds): test_set_del_bgp_asn_change.push_list_called = True @@ -185,61 +183,3 @@ def push_list(cmds): mgr.directory.put("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost", {"bgp_asn": "65100"}) assert test_set_del_bgp_asn_change.push_list_called - -def test_set_del_with_community(): - mgr = constructor() - set_del_test( - mgr, - "SET", - ("10.1.0.0/24", { - "profile": "FROM_SDN_SLB_ROUTES" - }), - True, - [ - ["router bgp 65100", - " no bgp network import-check"], - ["router bgp 65100", - " address-family ipv4 unicast", - " network 10.1.0.0/24 route-map FROM_SDN_SLB_ROUTES_RM"] - ] - ) - - set_del_test( - mgr, - "SET", - ("fc00:10::/64", { - "profile": "FROM_SDN_SLB_ROUTES" - }), - True, - [ - ["router bgp 65100", - " address-family ipv6 unicast", - " network fc00:10::/64 route-map FROM_SDN_SLB_ROUTES_RM"] - ] - ) - - set_del_test( - mgr, - "DEL", - ("10.1.0.0/24",), - True, - [ - ["router bgp 65100", - " address-family ipv4 unicast", - " no network 10.1.0.0/24"] - ] - ) - - set_del_test( - mgr, - "DEL", - ("fc00:10::/64",), - True, - [ - ["router bgp 65100", - " bgp network import-check"], - ["router bgp 65100", - " address-family ipv6 unicast", - " no network fc00:10::/64"] - ] - ) \ No newline at end of file diff --git a/src/sonic-bgpcfgd/tests/test_rm.py b/src/sonic-bgpcfgd/tests/test_rm.py deleted file mode 100644 index fe89055d27f4..000000000000 --- a/src/sonic-bgpcfgd/tests/test_rm.py +++ /dev/null @@ -1,62 +0,0 @@ -from unittest.mock import MagicMock -from bgpcfgd.directory import Directory -from bgpcfgd.managers_rm import RouteMapMgr -from swsscommon import swsscommon - -def constructor(): - cfg_mgr = MagicMock() - - common_objs = { - 'directory': Directory(), - 'cfg_mgr': cfg_mgr, - 'constants': {}, - } - - mgr = RouteMapMgr(common_objs, "APPL_DB", "BGP_PROFILE_TABLE") - return mgr - -def set_del_test(mgr, op, args, expected_ret, expected_cmds): - set_del_test.push_list_called = False - def push_list(cmds): - set_del_test.push_list_called = True - assert cmds in expected_cmds - return True - mgr.cfg_mgr.push_list = push_list - - if op == "SET": - ret = mgr.set_handler(*args) - assert ret == expected_ret - elif op == "DEL": - mgr.del_handler(*args) - else: - assert False, "Wrong operation" - - if expected_cmds: - assert set_del_test.push_list_called, "cfg_mgr.push_list wasn't called" - else: - assert not set_del_test.push_list_called, "cfg_mgr.push_list was called" - -def test_set_del(): - mgr = constructor() - set_del_test( - mgr, - "SET", - ("FROM_SDN_SLB_ROUTES", { - "community_id": "1234:1234" - }), - True, - [ - ["route-map FROM_SDN_SLB_ROUTES_RM permit 100", - " set community 1234:1234"] - ] - ) - - set_del_test( - mgr, - "DEL", - ("FROM_SDN_SLB_ROUTES",), - True, - [ - ["no route-map FROM_SDN_SLB_ROUTES_RM permit 100"] - ] - )