openstack
/
neutron
Code Issues Proposed changes

[ovn] Drop use of OVN_GW_PORT_EXT_ID_KEY 

At present Neutron maintains an external_id on the
Logical_Router (LR) representing the gw port (singluar).  This
is problematic when introducing multiple gateway ports.

Instead we can find Logical Router Port (LRP) that act as
gateways for the LR at runtime by looking for configuration
present on all GW ports.

Partial-Bug: #2002687
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
Needed-By: I95a0d5f1b7aef985df5625cd83222799db811f2b
Change-Id: I8a915dca1410c70bdfe7a2d72931921d2a1a265e
This commit is contained in:
Frode Nordahl 2023-03-17 18:42:53 +01:00 committed by Dmitrii Shcherbakov
parent 4ed33b55a7
commit d67d1c2736
7 changed files with 111 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py
View File

@ -836,19 +836,8 @@ class DeleteLRouterExtGwCommand(command.BaseCommand):
lrouter.delvalue('nat', nat)
nat.delete()
lrouter_ext_ids = getattr(lrouter, 'external_ids', {})
gw_port_id = lrouter_ext_ids.get(ovn_const.OVN_GW_PORT_EXT_ID_KEY)
if not gw_port_id:
return
try:
lrouter_port = idlutils.row_by_value(
self.api.idl, 'Logical_Router_Port', 'name',
utils.ovn_lrouter_port_name(gw_port_id))
except idlutils.RowNotFound:
return
lrouter.delvalue('ports', lrouter_port)
for gw_port in self.api.get_lrouter_gw_ports(lrouter.name):
lrouter.delvalue('ports', gw_port)
class SetLSwitchPortToVirtualTypeCommand(command.BaseCommand):

16
neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py
View File

@ -785,6 +785,22 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend):
result = lrp.execute(check_error=True)
return result[0] if result else None
def get_lrouter_gw_ports(self, lrouter_name):
lr = self.get_lrouter(lrouter_name)
gw_ports = []
for lrp in getattr(lr, 'ports', []):
lrp_ext_ids = getattr(lrp, 'external_ids', {})
if (ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY not in lrp_ext_ids or
utils.ovn_name(lrp_ext_ids[
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY]) != lr.name):
continue
lrp_ha_cfg = (getattr(lrp, 'gateway_chassis', None) or
getattr(lrp, 'options', {}).get(
ovn_const.OVN_GATEWAY_CHASSIS_KEY))
if lrp_ha_cfg:
gw_ports.append(lrp)
return gw_ports
def delete_lrouter_ext_gw(self, lrouter_name, if_exists=True):
return cmd.DeleteLRouterExtGwCommand(self, lrouter_name, if_exists)

2
neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
View File

@ -1371,8 +1371,6 @@ class OVNClient(object):
return {
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY:
router.get('name', 'no_router_name'),
ovn_const.OVN_GW_PORT_EXT_ID_KEY:
router.get('gw_port_id') or '',
ovn_const.OVN_REV_NUM_EXT_ID_KEY: str(utils.get_revision_number(
router, ovn_const.TYPE_ROUTERS)),
ovn_const.OVN_AZ_HINTS_EXT_ID_KEY:

1
neutron/tests/unit/fake_resources.py
View File

@ -163,6 +163,7 @@ class FakeOvsdbNbOvnIdl(object):
self.ha_chassis_group_del = mock.Mock()
self.ha_chassis_group_add_chassis = mock.Mock()
self.ha_chassis_group_del_chassis = mock.Mock()
self.get_lrouter_gw_ports = mock.Mock()
class FakeOvsdbSbOvnIdl(object):

22
neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py
View File

@ -1214,6 +1214,7 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand):
fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row(
attrs={'static_routes': [fake_route_1, fake_route_2],
'nat': []})
self.ovn_api.get_lrouter_gw_ports.return_value = []
with mock.patch.object(self.ovn_api, "is_col_present",
return_value=True):
with mock.patch.object(idlutils, 'row_by_value',
@ -1234,6 +1235,7 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand):
fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row(
attrs={'nat': [fake_nat_1, fake_nat_2],
'static_routes': []})
self.ovn_api.get_lrouter_gw_ports.return_value = []
with mock.patch.object(self.ovn_api, "is_col_present",
return_value=True):
with mock.patch.object(idlutils, 'row_by_value',
@ -1246,10 +1248,11 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand):
def test_delete_lrouter_extgw_ports(self):
port_id = 'fake-port-id'
fake_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row(
attrs={'gateway_chassis': ['fake_gwc']})
fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row(
attrs={'external_ids':
{ovn_const.OVN_GW_PORT_EXT_ID_KEY: port_id},
'static_routes': [], 'nat': []})
attrs={'ports': [fake_lrp], 'static_routes': [], 'nat': []})
self.ovn_api.get_lrouter_gw_ports.return_value = [fake_lrp]
with mock.patch.object(self.ovn_api, "is_col_present",
return_value=True):
with mock.patch.object(idlutils, 'row_by_value',
@ -1258,22 +1261,21 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand):
self.ovn_api, fake_lrouter.name, False)
cmd.run_idl(self.transaction)
fake_lrouter.delvalue.assert_called_once_with(
'ports', port_id)
'ports', fake_lrp)
def test_delete_lrouter_extgw_ports_not_found(self):
port_id = 'fake-port-id'
fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row(
attrs={'external_ids':
{ovn_const.OVN_GW_PORT_EXT_ID_KEY: port_id},
'static_routes': [], 'nat': []})
attrs={'static_routes': [], 'nat': []})
self.ovn_api.get_lrouter_gw_ports.return_value = []
with mock.patch.object(self.ovn_api, "is_col_present",
return_value=True):
with mock.patch.object(idlutils, 'row_by_value',
side_effect=[fake_lrouter,
idlutils.RowNotFound]):
side_effect=[fake_lrouter]):
cmd = commands.DeleteLRouterExtGwCommand(
self.ovn_api, fake_lrouter.name, False)
cmd.run_idl(self.transaction)
self.ovn_api.get_lrouter_gw_ports.assert_called_once_with(
fake_lrouter.name)
fake_lrouter.delvalue.assert_not_called()
def _test_delete_lrouter_no_lrouter_exist(self, if_exists=True):

85
neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py
View File

@ -161,7 +161,10 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
'lr-name-d'}},
{'name': utils.ovn_name('lr-id-e'),
'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY:
'lr-name-e'}}],
'lr-name-e'}},
{'name': utils.ovn_name('lr-id-f'),
'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY:
'lr-name-f'}}],
'lrouter_ports': [
{'name': utils.ovn_lrouter_port_name('orp-id-a1'),
'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY:
@ -169,10 +172,14 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
'networks': ['10.0.1.0/24'],
'options': {ovn_const.OVN_GATEWAY_CHASSIS_KEY: 'host-1'}},
{'name': utils.ovn_lrouter_port_name('orp-id-a2'),
'external_ids': {}, 'networks': ['10.0.2.0/24'],
'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY:
'lr-id-a'},
'networks': ['10.0.2.0/24'],
'options': {ovn_const.OVN_GATEWAY_CHASSIS_KEY: 'host-1'}},
{'name': utils.ovn_lrouter_port_name('orp-id-a3'),
'external_ids': {}, 'networks': ['10.0.3.0/24'],
'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY:
'lr-id-a'},
'networks': ['10.0.3.0/24'],
'options': {ovn_const.OVN_GATEWAY_CHASSIS_KEY:
ovn_const.OVN_GATEWAY_INVALID_CHASSIS}},
{'name': 'xrp-id-b1',
@ -182,7 +189,15 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
'options': {ovn_const.OVN_GATEWAY_CHASSIS_KEY: 'host-2'}},
{'name': utils.ovn_lrouter_port_name('orp-id-b3'),
'external_ids': {}, 'networks': ['20.0.3.0/24'],
'options': {}},
{'name': utils.ovn_lrouter_port_name('gwc'),
'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY:
'lr-id-f'},
'networks': ['10.0.4.0/24'],
'options': {}}],
'gateway_chassis': [
{'chassis_name': 'fake-chassis',
'name': utils.ovn_lrouter_port_name('gwc') + '_fake-chassis'}],
'static_routes': [{'ip_prefix': '20.0.0.0/16',
'nexthop': '10.0.3.253'},
{'ip_prefix': '10.0.0.0/16',
@ -317,7 +332,12 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
utils.ovn_lrouter_port_name('orp-id-a3')],
utils.ovn_name('lr-id-b'): [
'xrp-id-b1',
utils.ovn_lrouter_port_name('orp-id-b2')]},
utils.ovn_lrouter_port_name('orp-id-b2')],
utils.ovn_name('lr-id-f'): [
utils.ovn_lrouter_port_name('gwc')]},
'lrptogwc': {
utils.ovn_lrouter_port_name('gwc'): [
utils.ovn_lrouter_port_name('gwc') + '_fake-chassis']},
'lrtosroute': {
utils.ovn_name('lr-id-a'): ['20.0.0.0/16'],
utils.ovn_name('lr-id-b'): ['10.0.0.0/16']
@ -346,6 +366,7 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
self.dhcp_table = fakes.FakeOvsdbTable.create_one_ovsdb_table()
self.address_set_table = fakes.FakeOvsdbTable.create_one_ovsdb_table()
self.lb_table = fakes.FakeOvsdbTable.create_one_ovsdb_table()
self.gwc_table = fakes.FakeOvsdbTable.create_one_ovsdb_table()
self._tables = {}
self._tables['Logical_Switch'] = self.lswitch_table
@ -358,6 +379,7 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
self._tables['Address_Set'] = self.address_set_table
self._tables['Load_Balancer'] = self.lb_table
self._tables['NAT'] = self.nat_table
self._tables['Gateway_Chassis'] = self.gwc_table
with mock.patch.object(impl_idl_ovn.OvsdbNbOvnIdl, 'from_worker',
return_value=mock.Mock()):
@ -379,16 +401,23 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
TestNBImplIdlOvn.fake_associations['lstolsp'],
self.lswitch_table, self.lsp_table,
'name', 'name', 'ports')
# Load Routers and Router Ports
# Load Routers, Router Ports and Gateway Chassis
fake_lrouters = TestNBImplIdlOvn.fake_set['lrouters']
self._load_ovsdb_fake_rows(self.lrouter_table, fake_lrouters)
fake_lrps = TestNBImplIdlOvn.fake_set['lrouter_ports']
self._load_ovsdb_fake_rows(self.lrp_table, fake_lrps)
fake_gwc = TestNBImplIdlOvn.fake_set['gateway_chassis']
self._load_ovsdb_fake_rows(self.gwc_table, fake_gwc)
# Associate routers and router ports
self._construct_ovsdb_references(
TestNBImplIdlOvn.fake_associations['lrtolrp'],
self.lrouter_table, self.lrp_table,
'name', 'name', 'ports')
# Associate router ports and gateway chassis
self._construct_ovsdb_references(
TestNBImplIdlOvn.fake_associations['lrptogwc'],
self.lrp_table, self.gwc_table,
'name', 'name', 'gateway_chassis')
# Load static routes
fake_sroutes = TestNBImplIdlOvn.fake_set['static_routes']
self._load_ovsdb_fake_rows(self.sroute_table, fake_sroutes)
@ -484,6 +513,9 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
{'name': 'lr-id-d', 'ports': {}, 'static_routes': [],
'snats': [], 'dnat_and_snats': []},
{'name': 'lr-id-e', 'ports': {}, 'static_routes': [],
'snats': [], 'dnat_and_snats': []},
{'name': 'lr-id-f', 'static_routes': [],
'ports': {'gwc': ['10.0.4.0/24']},
'snats': [], 'dnat_and_snats': []}]
self.assertCountEqual(mapping, expected)
@ -556,6 +588,11 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
def test_get_all_chassis_gateway_bindings(self):
self._load_nb_db()
# NOTE(fnordahl): The `Gateway_Chassis` table being present without
# proper associations fools the test, remove for now.
del(self._tables['Gateway_Chassis'])
bindings = self.nb_ovn_idl.get_all_chassis_gateway_bindings()
expected = {'host-1': [utils.ovn_lrouter_port_name('orp-id-a1'),
utils.ovn_lrouter_port_name('orp-id-a2')],
@ -574,6 +611,11 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
def test_get_gateway_chassis_binding(self):
self._load_nb_db()
# NOTE(fnordahl): The `Gateway_Chassis` table being present without
# proper associations fools the test, remove for now.
del(self._tables['Gateway_Chassis'])
chassis = self.nb_ovn_idl.get_gateway_chassis_binding(
utils.ovn_lrouter_port_name('orp-id-a1'))
self.assertEqual(chassis, ['host-1'])
@ -591,6 +633,11 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
def test_get_unhosted_gateways(self):
self._load_nb_db()
# NOTE(fnordahl): The `Gateway_Chassis` table being present without
# proper associations fools the test, remove for now.
del(self._tables['Gateway_Chassis'])
# Port physnet-dict
port_physnet_dict = {
'orp-id-a1': 'physnet1', # scheduled
@ -626,6 +673,11 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
def test_get_unhosted_gateways_deleted_physnet(self):
self._load_nb_db()
# NOTE(fnordahl): The `Gateway_Chassis` table being present without
# proper associations fools the test, remove for now.
del(self._tables['Gateway_Chassis'])
# The LRP is on host-2 now
router_row = self._find_ovsdb_fake_row(self.lrp_table,
'name', 'lrp-orp-id-a1')
@ -813,6 +865,29 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn):
lb = self.nb_ovn_idl.get_floatingip_in_nat_or_lb(fip_id)
self.assertEqual(lb['_uuid'], lb_row.uuid)
def test_get_lrouter_gw_ports_legacy_option(self):
self._load_nb_db()
gw1_row = self._find_ovsdb_fake_row(
self.lrp_table, 'name', utils.ovn_lrouter_port_name('orp-id-a1'))
gw2_row = self._find_ovsdb_fake_row(
self.lrp_table, 'name', utils.ovn_lrouter_port_name('orp-id-a2'))
gw3_row = self._find_ovsdb_fake_row(
self.lrp_table, 'name', utils.ovn_lrouter_port_name('orp-id-a3'))
gw_ports = self.nb_ovn_idl.get_lrouter_gw_ports(
utils.ovn_name('lr-id-a'))
self.assertEqual([gw1_row, gw2_row, gw3_row], gw_ports)
def test_get_lrouter_gw_ports_gwc(self):
self._load_nb_db()
gw1_row = self._find_ovsdb_fake_row(
self.lrp_table, 'name', utils.ovn_lrouter_port_name('gwc'))
gw_ports = self.nb_ovn_idl.get_lrouter_gw_ports(
utils.ovn_name('lr-id-f'))
self.assertEqual([gw1_row], gw_ports)
class TestSBImplIdlOvnBase(TestDBImplIdlOvn):

3
neutron/tests/unit/services/ovn_l3/test_plugin.py
View File

@ -434,7 +434,6 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
{'router': updated_data})
self.l3_inst._nb_ovn.update_lrouter.assert_called_once_with(
'neutron-router-id', enabled=True, external_ids={
ovn_const.OVN_GW_PORT_EXT_ID_KEY: '',
ovn_const.OVN_GW_NETWORK_EXT_ID_KEY: '',
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router',
@ -456,7 +455,6 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
'neutron-router-id', enabled=False,
external_ids={ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'test',
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_GW_PORT_EXT_ID_KEY: '',
ovn_const.OVN_GW_NETWORK_EXT_ID_KEY: '',
ovn_const.OVN_AZ_HINTS_EXT_ID_KEY: ''})
@ -551,7 +549,6 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
external_ids = {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router',
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_GW_PORT_EXT_ID_KEY: 'gw-port-id',
ovn_const.OVN_GW_NETWORK_EXT_ID_KEY: 'ext-network-id',
ovn_const.OVN_AZ_HINTS_EXT_ID_KEY: ''}
self.l3_inst._nb_ovn.create_lrouter.assert_called_once_with(