openstack/ovn-bgp-agent
Code Issues Proposed changes

Ensure OVN LB on provider are only exposed in the right node

Browse Source 
We need to wait to expose the ovn lb with VIP on the provider
network for them to have members (and therefore have an entry
on the Load_Balancer table at the SB DB) so that it gets
associated to one specific cr-lrp (the one that will reply to
ARPs)

This patch partly reverts/adapt what was done in [1] to fix the
problem of exposing the VIPs in all the nodes with cr-lrp connected
to the same provider network

[1] https://review.opendev.org/c/x/ovn-bgp-agent/+/873073

Change-Id: Idc1022bc0593c21e272d138757391800f12415f1
This commit is contained in:
Luis Tomas Bolivar 2023-02-17 16:43:34 +01:00
parent 05de18942d
commit 5d760e41bb
8 changed files with 306 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
ovn_bgp_agent/constants.py
View File

@ -21,6 +21,8 @@ OVN_CHASSISREDIRECT_VIF_PORT_TYPE = "chassisredirect"
OVN_LOCALNET_VIF_PORT_TYPE = "localnet"
OVN_CIDRS_EXT_ID_KEY = 'neutron:cidrs'
OVN_PORT_NAME_EXT_ID_KEY = 'neutron:port_name'
LB_VIP_PORT_PREFIX = "ovn-lb-vip-"
OVS_RULE_COOKIE = "999"
OVS_VRF_RULE_COOKIE = "998"

52
ovn_bgp_agent/drivers/openstack/ovn_bgp_driver.py
View File

@ -37,7 +37,7 @@ LOG = logging.getLogger(__name__)
# LOG.setLevel(logging.DEBUG)
# logging.basicConfig(level=logging.DEBUG)
OVN_TABLES = ["Port_Binding", "Chassis", "Datapath_Binding"]
OVN_TABLES = ["Port_Binding", "Chassis", "Datapath_Binding", "Load_Balancer"]
class OVNBGPDriver(driver_api.AgentDriverBase):
@ -134,13 +134,14 @@ class OVNBGPDriver(driver_api.AgentDriverBase):
"PortBindingChassisDeletedEvent",
"FIPSetEvent",
"FIPUnsetEvent",
"OVNLBVIPPortEvent",
"OVNLBMemberCreateDeleteEvent",
"ChassisCreateEvent"])
if self._expose_tenant_networks:
events.update(["SubnetRouterAttachedEvent",
"SubnetRouterDetachedEvent",
"TenantPortCreatedEvent",
"TenantPortDeletedEvent"])
"TenantPortDeletedEvent",
"OVNLBVIPPortEvent"])
return events
@lockutils.synchronized('bgp')
@ -228,8 +229,9 @@ class OVNBGPDriver(driver_api.AgentDriverBase):
# add missing routes/ips related to ovn-octavia loadbalancers
# on the provider networks
ovn_lb_vips = self.sb_idl.get_ovn_lb_vips_on_provider_datapath(
cr_lrp_info['provider_datapath'])
cr_lrp_subnets_dp = set(cr_lrp_info['subnets_datapath'].values())
ovn_lb_vips = self.sb_idl.get_ovn_lb_vips_on_cr_lrp(
cr_lrp_info['provider_datapath'], cr_lrp_subnets_dp)
for ovn_lb_port, ovn_lb_ip in ovn_lb_vips.items():
self._expose_ovn_lb_on_provider(ovn_lb_ip,
ovn_lb_port,
@ -429,15 +431,15 @@ class OVNBGPDriver(driver_api.AgentDriverBase):
def _process_ovn_lb(self, ip, row, action):
try:
provider_network = self.sb_idl.is_provider_network(row.datapath)
if (not self._expose_tenant_networks or
self.sb_idl.is_provider_network(row.datapath)):
return
except agent_exc.DatapathNotFound:
# There is no need to expose anything related to a removed
# datapath
LOG.debug("LoadBalancer with VIP %s not being exposed as its "
"associated datapath %s was removed", ip, row.datapath)
return
if not provider_network:
if not self._expose_tenant_networks:
LOG.debug("LoadBalancer with VIP %s not being exposed/withdraw as"
" its associated datapath %s was removed", ip,
row.datapath)
return
if action == constants.EXPOSE:
return self._expose_remote_ip([ip], row)
@ -445,18 +447,14 @@ class OVNBGPDriver(driver_api.AgentDriverBase):
return self._withdraw_remote_ip([ip], row)
# if unknown action return
return
local_lb_cr_lrp = None
for cr_lrp_port, cr_lrp_info in self.ovn_local_cr_lrps.items():
if cr_lrp_info['provider_datapath'] == row.datapath:
local_lb_cr_lrp = cr_lrp_port
break
if not local_lb_cr_lrp:
return
vip_port = row.logical_port
if action == constants.EXPOSE:
self._expose_ovn_lb_on_provider(ip, vip_port, local_lb_cr_lrp)
if action == constants.WITHDRAW:
self._withdraw_ovn_lb_on_provider(vip_port, local_lb_cr_lrp)
@lockutils.synchronized('bgp')
def expose_ovn_lb_on_provider(self, ip, vip_port, cr_lrp_port):
self._expose_ovn_lb_on_provider(ip, vip_port, cr_lrp_port)
@lockutils.synchronized('bgp')
def withdraw_ovn_lb_on_provider(self, vip_port, cr_lrp_port):
self._withdraw_ovn_lb_on_provider(vip_port, cr_lrp_port)
def _expose_ovn_lb_on_provider(self, ip, ovn_lb_vip_port, cr_lrp,
exposed_ips=None, ovn_ip_rules=None):
@ -829,8 +827,12 @@ class OVNBGPDriver(driver_api.AgentDriverBase):
for lrp in lrp_ports:
self._process_lrp_port(lrp, cr_lrp_port)
ovn_lb_vips = self.sb_idl.get_ovn_lb_vips_on_provider_datapath(
provider_datapath)
cr_lrp_provider_dp = self.ovn_local_cr_lrps[cr_lrp_port][
'provider_datapath']
cr_lrp_subnets_dp = set(self.ovn_local_cr_lrps[cr_lrp_port][
'subnets_datapath'].values())
ovn_lb_vips = self.sb_idl.get_ovn_lb_vips_on_cr_lrp(
cr_lrp_provider_dp, cr_lrp_subnets_dp)
for ovn_lb_port, ovn_lb_ip in ovn_lb_vips.items():
self._expose_ovn_lb_on_provider(ovn_lb_ip,
ovn_lb_port,

46
ovn_bgp_agent/drivers/openstack/utils/ovn.py
View File

@ -312,11 +312,16 @@ class OvsdbSbOvnIdl(sb_impl_idl.OvnSbApiIdlImpl, Backend):
datapath, port_type=constants.OVN_VIRTUAL_VIF_PORT_TYPE)
return [r for r in rows if r.chassis and r.chassis[0].name == chassis]
def get_ovn_lb_vips_on_provider_datapath(self, datapath):
def get_ovn_lb(self, name):
cmd = self.db_find_rows('Load_Balancer', ('name', '=', name))
lb_info = cmd.execute(check_error=True)
return lb_info[0] if lb_info else []
def get_ovn_lb_vips_on_cr_lrp(self, provider_dp, subnets_dp):
# return {vip_port: vip_ip, vip_port2: vip_ip2, ...}
# ovn-sbctl find port_binding type=\"\" chassis=[] mac=[] up=false
cmd = self.db_find_rows('Port_Binding',
('datapath', '=', datapath),
('datapath', '=', provider_dp),
('type', '=', constants.OVN_VM_VIF_PORT_TYPE),
('chassis', '=', []),
('mac', '=', []),
@ -325,10 +330,37 @@ class OvsdbSbOvnIdl(sb_impl_idl.OvnSbApiIdlImpl, Backend):
for row in cmd.execute(check_error=True):
# This is depending on the external-id information added by
# neutron, regarding the neutron:cidrs
ext_n_cidr = row.external_ids.get(constants.OVN_CIDRS_EXT_ID_KEY)
if not ext_n_cidr:
port_name = row.external_ids.get(
constants.OVN_PORT_NAME_EXT_ID_KEY)
if (not port_name or
len(port_name) <= len(constants.LB_VIP_PORT_PREFIX)):
continue
ovn_lb_ip = ext_n_cidr.split(" ")[0].split("/")[0]
lbs[row.logical_port] = ovn_lb_ip
lb_name = port_name[len(constants.LB_VIP_PORT_PREFIX):]
lb = self.get_ovn_lb(lb_name)
if not lb:
continue
if hasattr(lb, 'datapath_group'):
lb_dp = lb.datapath_group[0].datapaths
else:
lb_dp = lb.datapaths
if set(lb_dp).intersection(subnets_dp):
ip_info = row.external_ids.get(constants.OVN_CIDRS_EXT_ID_KEY)
if not ip_info:
continue
lb_ip = ip_info.split(" ")[0].split("/")[0]
lbs[row.logical_port] = lb_ip
return lbs
def get_ovn_vip_port(self, name):
# ovn-sbctl find port_binding type=\"\" chassis=[] mac=[] up=false
cmd = self.db_find_rows('Port_Binding',
('type', '=', constants.OVN_VM_VIF_PORT_TYPE),
('chassis', '=', []),
('mac', '=', []),
('up', '=', False))
for row in cmd.execute(check_error=True):
port_name = row.external_ids.get(
constants.OVN_PORT_NAME_EXT_ID_KEY)
if port_name[len(constants.LB_VIP_PORT_PREFIX):] == name:
return row

9
ovn_bgp_agent/drivers/openstack/watchers/base_watcher.py
View File

@ -25,3 +25,12 @@ class PortBindingChassisEvent(row_event.RowEvent):
def _check_ip_associated(self, mac):
return len(mac.split(' ')) > 1
class OVNLBMemberEvent(row_event.RowEvent):
def __init__(self, bgp_agent, events):
self.agent = bgp_agent
table = 'Load_Balancer'
super(OVNLBMemberEvent, self).__init__(
events, table, None)
self.event_name = self.__class__.__name__

55
ovn_bgp_agent/drivers/openstack/watchers/bgp_watcher.py
View File

@ -350,6 +350,61 @@ class OVNLBVIPPortEvent(base_watcher.PortBindingChassisEvent):
self.agent.expose_ovn_lb(ovn_lb_ip, row)
class OVNLBMemberCreateDeleteEvent(base_watcher.OVNLBMemberEvent):
def __init__(self, bgp_agent):
events = (self.ROW_CREATE, self.ROW_DELETE,)
super(OVNLBMemberCreateDeleteEvent, self).__init__(
bgp_agent, events)
def match_fn(self, event, row, old):
# Only process event if the local node has a cr-lrp ports associated
return bool(self.agent.ovn_local_cr_lrps)
def run(self, event, row, old):
# Only process event if the local node has a cr-lrp port whose provider
# datapath is included into the loadbalancer. This means the
# loadbalancer has the VIP on a provider network
# Also, the cr-lrp port needs to have subnet datapaths (LS) associated
# to it that include the load balancer
try:
row_dp = row.datapaths
except AttributeError:
row_dp = []
if hasattr(row, 'datapath_group'):
if row.datapath_group:
dp_datapaths = row.datapath_group[0].datapaths
if dp_datapaths:
row_dp = dp_datapaths
vip_port = self.agent.sb_idl.get_ovn_vip_port(row.name)
if not vip_port:
return
associated_cr_lrp_port = None
for cr_lrp_port, cr_lrp_info in self.agent.ovn_local_cr_lrps.items():
if vip_port.datapath != cr_lrp_info.get('provider_datapath'):
continue
if set(row_dp).intersection(set(
cr_lrp_info.get('subnets_datapath').values())):
associated_cr_lrp_port = cr_lrp_port
break
else:
return
with _SYNC_STATE_LOCK.read_lock():
if event == self.ROW_DELETE:
# loadbalancer deleted. Withdraw the VIP through the cr-lrp
return self.agent.withdraw_ovn_lb_on_provider(
vip_port.logical_port, associated_cr_lrp_port)
if event == self.ROW_CREATE:
vip_ip = vip_port.external_ids.get(
constants.OVN_CIDRS_EXT_ID_KEY)
if not vip_ip:
return
vip_ip = vip_ip.split(" ")[0].split("/")[0]
return self.agent.expose_ovn_lb_on_provider(
vip_ip, vip_port.logical_port, associated_cr_lrp_port)
class ChassisCreateEventBase(row_event.RowEvent):
table = None

18
ovn_bgp_agent/tests/unit/drivers/openstack/test_ovn_bgp_driver.py
View File

@ -835,10 +835,6 @@ class TestOVNBGPDriver(test_base.TestCase):
self.bgp_driver, '_expose_remote_ip').start()
mock_withdraw_remote_ip = mock.patch.object(
self.bgp_driver, '_withdraw_remote_ip').start()
mock_expose_ovn_lb = mock.patch.object(
self.bgp_driver, '_expose_ovn_lb_on_provider').start()
mock_withdraw_ovn_lb = mock.patch.object(
self.bgp_driver, '_withdraw_ovn_lb_on_provider').start()
self.sb_idl.is_provider_network.return_value = provider
ip = 'fake-vip-ip'
@ -853,12 +849,7 @@ class TestOVNBGPDriver(test_base.TestCase):
if provider:
mock_expose_remote_ip.assert_not_called()
mock_withdraw_remote_ip.assert_not_called()
mock_expose_ovn_lb.assert_called_once_with(
ip, row.logical_port, self.cr_lrp0)
mock_withdraw_ovn_lb.assert_not_called()
else:
mock_expose_ovn_lb.assert_not_called()
mock_withdraw_ovn_lb.assert_not_called()
mock_expose_remote_ip.assert_called_once_with([ip], row)
mock_withdraw_remote_ip.assert_not_called()
@ -866,12 +857,7 @@ class TestOVNBGPDriver(test_base.TestCase):
if provider:
mock_expose_remote_ip.assert_not_called()
mock_withdraw_remote_ip.assert_not_called()
mock_expose_ovn_lb.assert_not_called()
mock_withdraw_ovn_lb.assert_called_once_with(
row.logical_port, self.cr_lrp0)
else:
mock_expose_ovn_lb.assert_not_called()
mock_withdraw_ovn_lb.assert_not_called()
mock_expose_remote_ip.assert_not_called()
mock_withdraw_remote_ip.assert_called_once_with([ip], row)
@ -1114,7 +1100,7 @@ class TestOVNBGPDriver(test_base.TestCase):
ovn_lb_vip = '172.24.4.5'
ovn_lb_vips = {'fake-vip-port': ovn_lb_vip}
self.sb_idl.get_ovn_lb_vips_on_provider_datapath.return_value = (
self.sb_idl.get_ovn_lb_vips_on_cr_lrp.return_value = (
ovn_lb_vips)
mock_expose_ovn_lb = mock.patch.object(
self.bgp_driver, '_expose_ovn_lb_on_provider').start()
@ -1556,7 +1542,7 @@ class TestOVNBGPDriver(test_base.TestCase):
dp_port0 = mock.Mock()
self.sb_idl.get_lrp_ports_for_router.return_value = [dp_port0]
ovn_lb_vips = {'fake-vip-port': 'fake-vip-ip'}
self.sb_idl.get_ovn_lb_vips_on_provider_datapath.return_value = (
self.sb_idl.get_ovn_lb_vips_on_cr_lrp.return_value = (
ovn_lb_vips)
self.bgp_driver._expose_cr_lrp_port(

62
ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_ovn.py
View File

@ -488,20 +488,70 @@ class TestOvsdbSbOvnIdl(test_base.TestCase):
self.assertEqual([port1], ret)
def test_get_ovn_lb_vips_on_provider_datapath(self):
def test_get_ovn_lb(self):
fake_lb_info = 'fake-lbinfo'
lb = 'fake-lb'
self.sb_idl.db_find_rows.return_value.execute.return_value = [
fake_lb_info]
ret = self.sb_idl.get_ovn_lb(lb)
self.assertEqual(fake_lb_info, ret)
self.sb_idl.db_find_rows.assert_called_once_with(
'Load_Balancer', ('name', '=', lb))
def test_get_ovn_lb_empty(self):
lb = 'fake-port'
self.sb_idl.db_find_rows.return_value.execute.return_value = []
ret = self.sb_idl.get_ovn_lb(lb)
self.assertEqual([], ret)
self.sb_idl.db_find_rows.assert_called_once_with(
'Load_Balancer', ('name', '=', lb))
def test_get_ovn_lb_vips_on_cr_lrp(self):
lb1_name = 'ovn-lb-vip-fake-lb1'
lb2_name = 'ovn-lb-vip-fake-lb2'
provider_dp = 'fake-provider-dp'
subnets_dp = ['fake-subnet-dp']
dp1 = fakes.create_object({'datapaths': ['fake-subnet-dp']})
lb1 = fakes.create_object({'datapath_group': [dp1]})
port0 = fakes.create_object({
'logical_port': 'fake-port-0',
'external_ids': {constants.OVN_CIDRS_EXT_ID_KEY: '10.0.0.15/24'}})
port1 = fakes.create_object({'logical_port': 'fake-port-1',
'external_ids': {}})
'external_ids': {constants.OVN_CIDRS_EXT_ID_KEY: '10.0.0.15/24',
constants.OVN_PORT_NAME_EXT_ID_KEY: lb1_name}})
port1 = fakes.create_object({
'logical_port': 'fake-port-1',
'external_ids': {constants.OVN_CIDRS_EXT_ID_KEY: '10.0.0.16/24'}})
port2 = fakes.create_object({
'logical_port': 'fake-port-0',
'external_ids': {constants.OVN_CIDRS_EXT_ID_KEY: '10.0.0.17/24',
constants.OVN_PORT_NAME_EXT_ID_KEY: lb2_name}})
self.sb_idl.db_find_rows.return_value.execute.return_value = [
port0, port1]
port0, port1, port2]
ret = self.sb_idl.get_ovn_lb_vips_on_provider_datapath('fake-datapath')
with mock.patch.object(self.sb_idl, 'get_ovn_lb') as mock_p:
mock_p.side_effect = (lb1, [])
ret = self.sb_idl.get_ovn_lb_vips_on_cr_lrp(provider_dp,
subnets_dp)
expected_return = {'fake-port-0': '10.0.0.15'}
self.assertEqual(expected_return, ret)
def test_get_ovn_vip_port(self):
lb_name = 'ovn-lb-vip-fake-lb'
lb1 = fakes.create_object(
{'external_ids': {
constants.OVN_PORT_NAME_EXT_ID_KEY: 'different-name'}})
lb2 = fakes.create_object(
{'external_ids': {constants.OVN_PORT_NAME_EXT_ID_KEY: lb_name}})
self.sb_idl.db_find_rows.return_value.execute.return_value = [
lb1, lb2]
ret = self.sb_idl.get_ovn_vip_port('fake-lb')
self.assertEqual(lb2, ret)
class TestOvnSbIdl(test_base.TestCase):

110
ovn_bgp_agent/tests/unit/drivers/openstack/watchers/test_bgp_watcher.py
View File

@ -810,6 +810,116 @@ class OVNLBVIPPortEvent(test_base.TestCase):
self.agent.withdraw_ovn_lb.assert_not_called()
class TestOVNLBMemberCreateDeleteEvent(test_base.TestCase):
def setUp(self):
super(TestOVNLBMemberCreateDeleteEvent, self).setUp()
self.chassis = '935f91fa-b8f8-47b9-8b1b-3a7a90ef7c26'
self.agent = mock.Mock(chassis=self.chassis)
self.agent.ovn_local_cr_lrps = {
'cr-lrp1': {'provider_datapath': 'dp1',
'subnets_datapath': {'lrp1': 's_dp1'},
'ovn_lbs': 'ovn-lb1'}}
self.event = bgp_watcher.OVNLBMemberCreateDeleteEvent(self.agent)
def test_match_fn(self):
self.assertTrue(self.event.match_fn(mock.Mock(), mock.Mock(),
mock.Mock()))
def test_match_fn_no_cr_lrp(self):
self.agent.ovn_local_cr_lrps = {}
self.assertFalse(self.event.match_fn(mock.Mock(), mock.Mock(),
mock.Mock()))
def test_run(self):
dpg1 = utils.create_row(_uuid='fake_dp_group',
datapaths=['s_dp1'])
row = utils.create_row(name='ovn-lb1',
datapath_group=[dpg1],
vips={'172.24.100.66:80': '10.0.0.5:8080'})
vip_port = utils.create_row(
datapath='dp1',
logical_port='ovn-lb-port-1',
external_ids={constants.OVN_CIDRS_EXT_ID_KEY: '172.24.100.66/26'})
self.agent.sb_idl.get_ovn_vip_port.return_value = vip_port
self.event.run(self.event.ROW_CREATE, row, mock.Mock())
self.agent.expose_ovn_lb_on_provider.assert_called_once_with(
'172.24.100.66', 'ovn-lb-port-1', 'cr-lrp1')
self.agent.withdraw_ovn_lb_on_provider.assert_not_called()
def test_run_no_vip_port(self):
dpg1 = utils.create_row(_uuid='fake_dp_group',
datapaths=['s_dp1'])
row = utils.create_row(name='ovn-lb1',
datapath_group=[dpg1],
vips={'172.24.100.66:80': '10.0.0.5:8080'})
self.agent.sb_idl.get_ovn_vip_port.return_value = []
self.event.run(self.event.ROW_CREATE, row, mock.Mock())
self.agent.expose_ovn_lb_on_provider.assert_not_called()
self.agent.withdraw_ovn_lb_on_provider.assert_not_called()
def test_run_different_provider(self):
dpg1 = utils.create_row(_uuid='fake_dp_group',
datapaths=['s_dp1'])
row = utils.create_row(name='ovn-lb1',
datapath_group=[dpg1],
vips={'172.24.100.66:80': '10.0.0.5:8080'})
vip_port = utils.create_row(
datapath='dp2',
logical_port='ovn-lb-port-1',
external_ids={constants.OVN_CIDRS_EXT_ID_KEY: '172.24.100.66/26'})
self.agent.sb_idl.get_ovn_vip_port.return_value = vip_port
self.event.run(self.event.ROW_CREATE, row, mock.Mock())
self.agent.expose_ovn_lb_on_provider.assert_not_called()
self.agent.withdraw_ovn_lb_on_provider.assert_not_called()
def test_run_no_cr_lrp_match(self):
dpg1 = utils.create_row(_uuid='fake_dp_group',
datapaths=['s_dp2'])
row = utils.create_row(name='ovn-lb1',
datapath_group=[dpg1],
vips={'172.24.100.66:80': '10.0.0.5:8080'})
vip_port = utils.create_row(
datapath='dp1',
logical_port='ovn-lb-port-1',
external_ids={constants.OVN_CIDRS_EXT_ID_KEY: '172.24.100.66/26'})
self.agent.sb_idl.get_ovn_vip_port.return_value = vip_port
self.event.run(self.event.ROW_CREATE, row, mock.Mock())
self.agent.expose_ovn_lb_on_provider.assert_not_called()
self.agent.withdraw_ovn_lb_on_provider.assert_not_called()
def test_run_no_vip(self):
dpg1 = utils.create_row(_uuid='fake_dp_group',
datapaths=['s_dp1'])
row = utils.create_row(name='ovn-lb1',
datapath_group=[dpg1],
vips={'172.24.100.66:80': '10.0.0.5:8080'})
vip_port = utils.create_row(
datapath='dp1',
logical_port='port-1',
external_ids={})
self.agent.sb_idl.get_ovn_vip_port.return_value = vip_port
self.event.run(self.event.ROW_CREATE, row, mock.Mock())
self.agent.expose_ovn_lb_on_provider.assert_not_called()
self.agent.withdraw_ovn_lb_on_provider.assert_not_called()
def test_run_delete(self):
dpg1 = utils.create_row(_uuid='fake_dp_group',
datapaths=['s_dp1'])
row = utils.create_row(name='ovn-lb1',
datapath_group=[dpg1],
vips={'172.24.100.66:80': '10.0.0.5:8080'})
vip_port = utils.create_row(
datapath='dp1',
logical_port='ovn-lb-port-1',
external_ids={constants.OVN_CIDRS_EXT_ID_KEY: '172.24.100.66/26'})
self.agent.sb_idl.get_ovn_vip_port.return_value = vip_port
self.event.run(self.event.ROW_DELETE, row, mock.Mock())
self.agent.withdraw_ovn_lb_on_provider.assert_called_once_with(
'ovn-lb-port-1', 'cr-lrp1')
self.agent.expose_ovn_lb_on_provider.assert_not_called()
class TestChassisCreateEvent(test_base.TestCase):
_event = bgp_watcher.ChassisCreateEvent