[bgpcfgd] Remove unnecessary dependency for StaticRouteMgr (#8037)
Why I did it Static route configuration should not depend on BGP_ASN. Remove the dependency on BGP_ASN for StaticRouteMgr. Fix #8027 How I did it Check if BGP_ASN field before configuring static route redistribution and wait until BGP_ASN is available to enable static route redistribution. How to verify it Add unit test to cover the scenario and verify the functionality on a virtual switch.
This commit is contained in:
Shi Su
Judy Joseph
parent
027b6d1dc5
commit
ed95ec76fa
@ -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 '::'
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
Reference in New Issue
Block a user