openstack
/
neutron
Code Issues Proposed changes

[ovn] Drop use of LR OVN_GW_NETWORK_EXT_ID_KEY 

An update to the OVN QoS driver to support the `qos_gateway_ip`
QoS extension [0] introduced adding the GW network id as an
external_id on the Logical_Router (LR).  This is problematic
when introducing multiple gateway ports, because a single LR
can have gateways in multiple networks.

The external_id key was presumably added because at the point in
time when a LR is deleted, the code had no other source of this
information.  However, it turns out this step is redundant and
not neccessary.

To prove this I include a excerpt of a stack trace when deleting
a router in the commit message:

    File "services/ovn_l3/plugin.py", line 210, in delete_router
      super(OVNL3RouterPlugin, self).delete_router(context, id)
    File "db/l3_db.py", line 612, in delete_router
      self._delete_current_gw_port(context, id, router, None)
    File "db/l3_db.py", line 452, in _delete_current_gw_port
      self._core_plugin.delete_port(
    File "plugins/ml2/drivers/ovn/mech_driver/mech_driver.py",
        line 886, in delete_port_postcommit
      self._ovn_client.delete_port(context.plugin_context, port['id'],
    File "plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py",
        line 830, in delete_port
      self._delete_port(port_id, port_object=port_object)

Essentially, a routers GW port(s) will be removed prior to
deleting the router itself.

The `ovn_client.delete_port` method will call on the QoS extension
to remove rules matching the GW port, and that will be the same
rules as has previously been added for the router.

I also added a functional test that confirms this fact [1].

0: I46864b9234af64f190f6b6daebfd94d2e3bd0c17
1: Ic92a7b3bd73920d08dee41974bfe3aeb1c64b557

Partial-Bug: #2002687
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
Needed-By: I95a0d5f1b7aef985df5625cd83222799db811f2b
Change-Id: If7c22bc8a95fa13e746c86a1e9d4a6fa25496e1f
This commit is contained in:
Frode Nordahl 2023-03-16 09:48:02 +01:00 committed by Dmitrii Shcherbakov
parent d67d1c2736
commit e5d4499672
6 changed files with 25 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
neutron/common/ovn/constants.py
View File

@ -32,8 +32,8 @@ OVN_ROUTER_NAME_EXT_ID_KEY = 'neutron:router_name'
OVN_ROUTER_ID_EXT_ID_KEY = 'neutron:router_id'
OVN_AZ_HINTS_EXT_ID_KEY = 'neutron:availability_zone_hints'
OVN_ROUTER_IS_EXT_GW = 'neutron:is_ext_gw'
OVN_GW_PORT_EXT_ID_KEY = 'neutron:gw_port_id'
OVN_GW_NETWORK_EXT_ID_KEY = 'neutron:gw_network_id'
OVN_GW_PORT_EXT_ID_KEY = 'neutron:gw_port_id' # DEPRECATED, DON'T USE
OVN_GW_NETWORK_EXT_ID_KEY = 'neutron:gw_network_id' # DEPRECATED, DON'T USE
OVN_SUBNET_EXT_ID_KEY = 'neutron:subnet_id'
OVN_SUBNET_EXT_IDS_KEY = 'neutron:subnet_ids'
OVN_SUBNET_POOL_EXT_ADDR_SCOPE4_KEY = 'neutron:subnet_pool_addr_scope4'

11
neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/qos.py
View File

@ -425,13 +425,6 @@ class OVNClientQosExtension(object):
def disassociate_floatingip(self, txn, floatingip):
self.delete_floatingip(txn, floatingip)
def _delete_gateway_ip_qos_rules(self, txn, router_id, network_id):
if network_id:
lswitch_name = utils.ovn_name(network_id)
txn.add(self.nb_idl.qos_del_ext_ids(
lswitch_name,
{ovn_const.OVN_ROUTER_ID_EXT_ID_KEY: router_id}))
def create_router(self, txn, router):
self.update_router(txn, router)
@ -465,10 +458,6 @@ class OVNClientQosExtension(object):
# Delete, if exists, the QoS rule in this direction.
txn.add(self.nb_idl.qos_del(**ovn_rule, if_exists=True))
def delete_router(self, txn, router):
self._delete_gateway_ip_qos_rules(txn, router['id'],
router['gw_network_id'])
def update_policy(self, context, policy):
updated_port_ids = set([])
updated_fip_ids = set([])

29
neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py
View File

@ -775,30 +775,31 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
txn.add(cmd)
raise periodics.NeverAgain()
# TODO(ralonsoh): Remove this in the Z+3 cycle. This method adds the
# "external_ids:OVN_GW_NETWORK_EXT_ID_KEY" to each router that has
# a gateway (that means, that has "external_ids:OVN_GW_PORT_EXT_ID_KEY").
# TODO(fnordahl): Remove this in the A+3 cycle. This method removes the
# now redundant "external_ids:OVN_GW_NETWORK_EXT_ID_KEY" and
# "external_ids:OVN_GW_PORT_EXT_ID_KEY" from to each router.
# A static spacing value is used here, but this method will only run
# once per lock due to the use of periodics.NeverAgain().
@periodics.periodic(spacing=600, run_immediately=True)
def update_logical_router_with_gateway_network_id(self):
"""Update all OVN logical router registers with the GW network ID"""
def remove_gw_ext_ids_from_logical_router(self):
"""Remove `gw_port_id` and `gw_network_id` external_ids from LRs"""
if not self.has_lock:
return
cmds = []
context = n_context.get_admin_context()
for lr in self._nb_idl.lr_list().execute(check_error=True):
gw_port = lr.external_ids.get(ovn_const.OVN_GW_PORT_EXT_ID_KEY)
gw_net = lr.external_ids.get(ovn_const.OVN_GW_NETWORK_EXT_ID_KEY)
if not gw_port or (gw_port and gw_net):
# This router does not have a gateway network assigned yet or
# it has a gateway port and its corresponding network.
if (ovn_const.OVN_GW_PORT_EXT_ID_KEY not in lr.external_ids and
ovn_const.OVN_GW_NETWORK_EXT_ID_KEY not in
lr.external_ids):
# This router have none of the deprecated external_ids.
continue
port = self._ovn_client._plugin.get_port(context, gw_port)
external_ids = {
ovn_const.OVN_GW_NETWORK_EXT_ID_KEY: port['network_id']}
external_ids = lr.external_ids.copy()
for k in (ovn_const.OVN_GW_PORT_EXT_ID_KEY,
ovn_const.OVN_GW_NETWORK_EXT_ID_KEY):
if k in external_ids:
del(external_ids[k])
cmds.append(self._nb_idl.db_set(
'Logical_Router', lr.uuid, ('external_ids', external_ids)))

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

@ -1366,8 +1366,6 @@ class OVNClient(object):
return networks
def _gen_router_ext_ids(self, router):
gw_net_id = (router.get('external_gateway_info') or
{}).get('network_id') or ''
return {
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY:
router.get('name', 'no_router_name'),
@ -1375,7 +1373,6 @@ class OVNClient(object):
router, ovn_const.TYPE_ROUTERS)),
ovn_const.OVN_AZ_HINTS_EXT_ID_KEY:
','.join(common_utils.get_az_hints(router)),
ovn_const.OVN_GW_NETWORK_EXT_ID_KEY: gw_net_id,
}
def create_router(self, context, router, add_external_gateway=True):
@ -1496,13 +1493,8 @@ class OVNClient(object):
def delete_router(self, context, router_id):
"""Delete a logical router."""
lrouter_name = utils.ovn_name(router_id)
ovn_router = self._nb_idl.get_lrouter(lrouter_name)
gw_network_id = ovn_router.external_ids.get(
ovn_const.OVN_GW_NETWORK_EXT_ID_KEY) if ovn_router else None
router_dict = {'id': router_id, 'gw_network_id': gw_network_id}
with self._nb_idl.transaction(check_error=True) as txn:
txn.add(self._nb_idl.delete_lrouter(lrouter_name))
self._qos_driver.delete_router(txn, router_dict)
db_rev.delete_revision(context, router_id, ovn_const.TYPE_ROUTERS)
def get_candidates_for_scheduling(self, physnet, cms=None,

15
neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py
View File

@ -716,18 +716,18 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight,
self.fake_ovn_client._nb_idl.db_set.assert_has_calls(
expected_calls)
def test_update_logical_router_with_gateway_network_id(self):
def test_remove_gw_ext_ids_from_logical_router(self):
nb_idl = self.fake_ovn_client._nb_idl
# lr0: GW port ID, not GW network ID --> we need to add network ID.
# lr0: GW port ID, not GW network ID --> we need to remove port ID.
lr0 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={
'name': 'lr0',
'external_ids': {constants.OVN_GW_PORT_EXT_ID_KEY: 'port0'}})
# lr1: GW port ID and not GW network ID --> register already updated.
# lr1: GW port ID and GW network ID --> we need to remove both.
lr1 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={
'name': 'lr1',
'external_ids': {constants.OVN_GW_PORT_EXT_ID_KEY: 'port1',
constants.OVN_GW_NETWORK_EXT_ID_KEY: 'net1'}})
# lr2: no GW port ID (nor GW network ID) --> no QoS.
# lr2: no GW port ID (nor GW network ID) --> no action needed.
lr2 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={
'name': 'lr2', 'external_ids': {}})
nb_idl.lr_list.return_value.execute.return_value = (lr0, lr1, lr2)
@ -736,10 +736,11 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight,
self.assertRaises(
periodics.NeverAgain,
self.periodic.update_logical_router_with_gateway_network_id)
ext_ids = {constants.OVN_GW_NETWORK_EXT_ID_KEY: 'net0'}
self.periodic.remove_gw_ext_ids_from_logical_router)
expected_calls = [mock.call('Logical_Router', lr0.uuid,
('external_ids', ext_ids))]
('external_ids', {})),
mock.call('Logical_Router', lr1.uuid,
('external_ids', {}))]
nb_idl.db_set.assert_has_calls(expected_calls)
def _test_check_baremetal_ports_dhcp_options(self, dhcp_disabled=False):

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_NETWORK_EXT_ID_KEY: '',
ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router',
ovn_const.OVN_AZ_HINTS_EXT_ID_KEY: ''})
@ -455,7 +454,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_NETWORK_EXT_ID_KEY: '',
ovn_const.OVN_AZ_HINTS_EXT_ID_KEY: ''})
@mock.patch.object(utils, 'get_lrouter_non_gw_routes')
@ -549,7 +547,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_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(
'neutron-router-id', external_ids=external_ids,