diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py index 974843366a..6fafeda814 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py @@ -16,12 +16,14 @@ class StaticRouteMgr(Manager): """ super(StaticRouteMgr, self).__init__( common_objs, - [("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], + [], db, table, ) - + + self.directory.subscribe([("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),], self.on_bgp_asn_change) self.static_routes = {} + self.vrf_pending_redistribution = set() OP_DELETE = 'DELETE' OP_ADD = 'ADD' @@ -47,15 +49,10 @@ class StaticRouteMgr(Manager): # Enable redistribution of static routes when it is the first one get set if not self.static_routes.get(vrf, {}): - log_debug("Enabling static route redistribution") - bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] - if vrf == 'default': - cmd_list.append("router bgp %s" % bgp_asn) + if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): + cmd_list.extend(self.enable_redistribution_command(vrf)) else: - cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) - for af in ["ipv4", "ipv6"]: - cmd_list.append(" address-family %s" % af) - cmd_list.append(" redistribute static") + self.vrf_pending_redistribution.add(vrf) if cmd_list: self.cfg_mgr.push_list(cmd_list) @@ -78,15 +75,9 @@ class StaticRouteMgr(Manager): # Disable redistribution of static routes when it is the last one to delete if self.static_routes.get(vrf, {}).keys() == {ip_prefix}: - log_debug("Disabling static route redistribution") - bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] - if vrf == 'default': - cmd_list.append("router bgp %s" % bgp_asn) - else: - cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) - for af in ["ipv4", "ipv6"]: - cmd_list.append(" address-family %s" % af) - cmd_list.append(" no redistribute static") + if self.directory.path_exist("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"): + cmd_list.extend(self.disable_redistribution_command(vrf)) + self.vrf_pending_redistribution.discard(vrf) if cmd_list: self.cfg_mgr.push_list(cmd_list) @@ -135,6 +126,38 @@ class StaticRouteMgr(Manager): ' vrf {}'.format(vrf) if vrf != 'default' else '' ) + def enable_redistribution_command(self, vrf): + log_debug("Enabling static route redistribution") + cmd_list = [] + bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] + if vrf == 'default': + cmd_list.append("router bgp %s" % bgp_asn) + else: + cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) + for af in ["ipv4", "ipv6"]: + cmd_list.append(" address-family %s" % af) + cmd_list.append(" redistribute static") + return cmd_list + + def disable_redistribution_command(self, vrf): + log_debug("Disabling static route redistribution") + cmd_list = [] + bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] + if vrf == 'default': + cmd_list.append("router bgp %s" % bgp_asn) + else: + cmd_list.append("router bgp %s vrf %s" % (bgp_asn, vrf)) + for af in ["ipv4", "ipv6"]: + cmd_list.append(" address-family %s" % af) + cmd_list.append(" no redistribute static") + return 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 in self.vrf_pending_redistribution: + self.cfg_mgr.push_list(self.enable_redistribution_command(vrf)) + self.vrf_pending_redistribution.clear() + class IpNextHop: def __init__(self, af_id, blackhole, dst_ip, if_name, dist, vrf): zero_ip = lambda af: '0.0.0.0' if af == socket.AF_INET else '::' diff --git a/src/sonic-bgpcfgd/tests/test_static_rt.py b/src/sonic-bgpcfgd/tests/test_static_rt.py index 0fa2e3df6c..e0b9b1b17e 100644 --- a/src/sonic-bgpcfgd/tests/test_static_rt.py +++ b/src/sonic-bgpcfgd/tests/test_static_rt.py @@ -6,7 +6,7 @@ from bgpcfgd.managers_static_rt import StaticRouteMgr from collections import Counter from swsscommon import swsscommon -def constructor(): +def constructor(skip_bgp_asn=False): cfg_mgr = MagicMock() common_objs = { @@ -17,7 +17,8 @@ def constructor(): } mgr = StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE") - mgr.directory.put("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost", {"bgp_asn": "65100"}) + if not skip_bgp_asn: + mgr.directory.put("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost", {"bgp_asn": "65100"}) assert len(mgr.static_routes) == 0 return mgr @@ -573,3 +574,80 @@ def test_set_invalid_ipaddr(): False, [] ) + +def test_set_del_no_bgp_asn(): + mgr = constructor(skip_bgp_asn=True) + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61", + "ifname": "PortChannel0001,PortChannel0002,PortChannel0003", + "distance": "10,20,30", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED", + ] + ) + set_del_test( + mgr, + "DEL", + ("vrfRED|10.1.3.0/24",), + True, + [ + "no ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "no ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED", + "no ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED", + ] + ) + +def test_set_del_bgp_asn_change(): + mgr = constructor(skip_bgp_asn=True) + set_del_test( + mgr, + "SET", + ("vrfRED|10.1.3.0/24", { + "nexthop": "10.0.0.57,10.0.0.59,10.0.0.61", + "ifname": "PortChannel0001,PortChannel0002,PortChannel0003", + "distance": "10,20,30", + "nexthop-vrf": "nh_vrf,,default", + "blackhole": "false,false,false", + }), + True, + [ + "ip route 10.1.3.0/24 10.0.0.57 PortChannel0001 10 nexthop-vrf nh_vrf vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.59 PortChannel0002 20 vrf vrfRED", + "ip route 10.1.3.0/24 10.0.0.61 PortChannel0003 30 nexthop-vrf default vrf vrfRED", + ] + ) + + assert mgr.vrf_pending_redistribution == {"vrfRED"} + + expected_cmds = [ + "router bgp 65100 vrf vrfRED", + " address-family ipv4", + " redistribute static", + " address-family ipv6", + " redistribute static" + ] + def push_list(cmds): + set_del_test.push_list_called = True + assert Counter(cmds) == Counter(expected_cmds) # check if commands are expected (regardless of the order) + max_del_idx = -1 + min_set_idx = len(cmds) + for idx in range(len(cmds)): + if cmds[idx].startswith('no ip') and idx > max_del_idx: + max_del_idx = idx + if cmds[idx].startswith('ip') and idx < min_set_idx: + min_set_idx = idx + assert max_del_idx < min_set_idx, "DEL command comes after SET command" # DEL commands should be done first + return True + mgr.cfg_mgr.push_list = push_list + mgr.directory.put("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost", {"bgp_asn": "65100"}) + + assert not mgr.vrf_pending_redistribution