From 2996f9ff53f78ff47378d8d7ed0cd1c3991ae002 Mon Sep 17 00:00:00 2001 From: Maor Blaustein Date: Thu, 30 Jan 2025 14:34:46 +0200 Subject: [PATCH] Fix LRP.external_ids router name prefix Fix create/update/delete LRP.external_ids router name to use "neutron-" prefix. Adjusts ovn unit tests accordingly. Added maintenance method to fix all existing LRPs, a unit test, and a functional test for it. Related-Bug: #2055045 Change-Id: I40ef017f2645076b7ca2a72c7e4b8680268bf4c9 --- .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 7 ++-- .../ovn/mech_driver/ovsdb/maintenance.py | 40 +++++++++++++++++++ .../ovn/mech_driver/ovsdb/ovn_client.py | 9 +++-- .../ovn/mech_driver/ovsdb/test_maintenance.py | 19 +++++++++ .../functional/services/ovn_l3/test_plugin.py | 5 ++- .../mech_driver/ovsdb/test_impl_idl_ovn.py | 24 +++++++---- .../ovn/mech_driver/ovsdb/test_maintenance.py | 27 +++++++++++++ .../tests/unit/services/ovn_l3/test_plugin.py | 25 +++++++----- 8 files changed, 130 insertions(+), 26 deletions(-) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py index c749c95916b..ed1c6e6aed6 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py @@ -560,10 +560,9 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend): default=None) if not lrp: return [] - router_id = lrp.external_ids.get( + router_name = lrp.external_ids.get( ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY, "") - lrouter = self.lookup('Logical_Router', utils.ovn_name(router_id), - default=None) + lrouter = self.lookup('Logical_Router', router_name, default=None) if not lrouter: return [] az_string = lrouter.external_ids.get( @@ -819,7 +818,7 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend): for lrp in getattr(lr, 'ports', []): lrp_ext_ids = getattr(lrp, 'external_ids', {}) if (r_name not in lrp_ext_ids or - utils.ovn_name(lrp_ext_ids[r_name]) != lr.name or + lrp_ext_ids[r_name] != lr.name or not strutils.bool_from_string(lrp_ext_ids.get(is_gw))): continue diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py index a7a3df8dd4e..0c003e3ebea 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py @@ -485,6 +485,46 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase): context, fip_id) self._ovn_client.delete_floatingip(context, fip_id) + # A static spacing value is used here, but this method will only run + # once per lock due to the use of periodics.NeverAgain(). + @has_lock_periodic( + periodic_run_limit=ovn_const.MAINTENANCE_TASK_RETRY_LIMIT, + spacing=ovn_const.MAINTENANCE_ONE_RUN_TASK_SPACING, + run_immediately=True) + def update_lrouter_ports_ext_ids_name_prefix(self): + """Update OVN logical router ports if missing external ids + "neutron-" prefix for router name. + """ + LOG.debug( + 'Maintenance task: Check missing prefix router_name in ' + 'external_ids of LRPs.') + self._sync_timer.restart() + ports_to_prefix = [] + rports = self._nb_idl.db_find( + 'Logical_Router_Port').execute(check_error=True) + for port in rports: + ext_id_rname = port['external_ids'].get( + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY) + if ext_id_rname and not ext_id_rname.startswith( + ovn_const.OVN_NAME_PREFIX): + ports_to_prefix.append(port) + if ports_to_prefix: + LOG.debug('Update missing prefix for router_name in %d LRPs:\n%s', + len(ports_to_prefix), + [str(p['_uuid']) for p in ports_to_prefix]) + with self._nb_idl.transaction(check_error=True) as txn: + for port in ports_to_prefix: + router_id = port['external_ids'].get( + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY) + txn.add(self._nb_idl.update_lrouter_port( + name=port.name, + external_ids=self._ovn_client._gen_router_port_ext_ids( + port, router_id))) + self._sync_timer.stop() + LOG.info('Maintenance task: Check missing prefix router_name in LRPs ' + '(took %.2f seconds)', self._sync_timer.elapsed()) + raise periodics.NeverAgain() + # A static spacing value is used here, but this method will only run # once per lock due to the use of periodics.NeverAgain(). @has_lock_periodic( diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py index 07f46c38167..f7d75767792 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py @@ -1672,7 +1672,8 @@ class OVNClient: utils.ovn_name(port['network_id']), ovn_const.OVN_ROUTER_IS_EXT_GW: str(const.DEVICE_OWNER_ROUTER_GW == port.get('device_owner')), - ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: router_id, + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name(router_id), } def _get_reside_redir_for_gateway_port(self, device_id): @@ -1877,7 +1878,8 @@ class OVNClient: external_ids = self._nb_idl.db_get( 'Logical_Router_Port', lrp_name, 'external_ids').execute(check_error=True) - router_id = external_ids[ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY] + router_id = external_ids[ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY].replace( + ovn_const.OVN_NAME_PREFIX, '') commands = [ self._nb_idl.update_lrouter_port( name=lrp_name, @@ -1956,7 +1958,8 @@ class OVNClient: port_removed = True router_id = ovn_port.external_ids[ - ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY] + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY].replace( + ovn_const.OVN_NAME_PREFIX, '') router = None gw_ports = [] try: diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py index 162894d7cd5..0a4528dba1a 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py @@ -197,6 +197,10 @@ class _TestMaintenanceHelper(testlib_api.MySQLTestCaseMixin, ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY) == name): return row + def _get_lrp_ext_ids_router_name(self, port_id): + row = self._find_router_port_row_by_port_id(port_id) + return row.external_ids.get(ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY) + def _create_security_group(self): data = {'security_group': {'name': 'sgtest', 'description': 'SpongeBob Rocks!'}} @@ -730,6 +734,21 @@ class TestMaintenance(_TestMaintenanceHelper): self.assertIsNotNone( self._find_router_port_row_by_port_id(neutron_obj['port_id'])) + # Assert router port has "neutron-" prefix and correct router uuid + # for external_ids neutron:router_name value, when created and after + # maintenance/updated (LP#2055045) + + self.assertEqual( + '%s%s' % (ovn_const.OVN_NAME_PREFIX, neutron_router['id']), + self._get_lrp_ext_ids_router_name(neutron_obj['port_id'])) + + self.assertRaises(periodics.NeverAgain, + self.maint.update_lrouter_ports_ext_ids_name_prefix) + + self.assertEqual( + '%s%s' % (ovn_const.OVN_NAME_PREFIX, neutron_router['id']), + self._get_lrp_ext_ids_router_name(neutron_obj['port_id'])) + # > Delete with mock.patch.object(self._l3_ovn_client, 'delete_router_port'): diff --git a/neutron/tests/functional/services/ovn_l3/test_plugin.py b/neutron/tests/functional/services/ovn_l3/test_plugin.py index 7f4429797ca..f9db69c59a2 100644 --- a/neutron/tests/functional/services/ovn_l3/test_plugin.py +++ b/neutron/tests/functional/services/ovn_l3/test_plugin.py @@ -346,8 +346,9 @@ class TestRouter(base.TestOVNFunctionalBase): if (ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY not in row.external_ids): continue - if row.external_ids[ - ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY] == router['id']: + ext_ids_rtr_name = row.external_ids[ + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY] + if ext_ids_rtr_name == ovn_utils.ovn_name(router['id']): chassis = {} for gwc in row.gateway_chassis: chassis[gwc.priority] = gwc.chassis_name diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py index 130f1ffc453..0e3f6a360ef 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py @@ -168,18 +168,24 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): 'lr-name-f'}}], 'lrouter_ports': [ {'name': utils.ovn_lrouter_port_name('orp-id-a1'), - 'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'lr-id-a', - ovn_const.OVN_ROUTER_IS_EXT_GW: str(True)}, + 'external_ids': { + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('lr-id-a'), + ovn_const.OVN_ROUTER_IS_EXT_GW: str(True)}, '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': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'lr-id-a', - ovn_const.OVN_ROUTER_IS_EXT_GW: str(True)}, + 'external_ids': { + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('lr-id-a'), + ovn_const.OVN_ROUTER_IS_EXT_GW: str(True)}, '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': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'lr-id-a', - ovn_const.OVN_ROUTER_IS_EXT_GW: str(True)}, + 'external_ids': { + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('lr-id-a'), + ovn_const.OVN_ROUTER_IS_EXT_GW: str(True)}, 'networks': ['10.0.3.0/24'], 'options': {ovn_const.OVN_GATEWAY_CHASSIS_KEY: None}}, {'name': 'xrp-id-b1', @@ -191,8 +197,10 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): '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', - ovn_const.OVN_ROUTER_IS_EXT_GW: str(True)}, + 'external_ids': { + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('lr-id-f'), + ovn_const.OVN_ROUTER_IS_EXT_GW: str(True)}, 'networks': ['10.0.4.0/24'], 'options': {}}], 'gateway_chassis': [ diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py index 613c6563a2d..aaaf6176179 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py @@ -394,6 +394,33 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight, self.periodic._log_maintenance_inconsistencies(incst, []) self.assertFalse(mock_log.called) + def test_update_lrouter_ports_ext_ids_name_prefix(self): + lrp1_no_prefix = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={ + '_uuid': 'port-id1', + 'name': 'lrp-id1', + 'external_ids': { + constants.OVN_ROUTER_NAME_EXT_ID_KEY: 'rtr-id1'}}) + lrp2_with_prefix = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={ + '_uuid': 'port-id2', + 'name': 'lrp-id2', + 'external_ids': { + constants.OVN_ROUTER_NAME_EXT_ID_KEY: + '%srtr-id2' % constants.OVN_NAME_PREFIX}}) + + nb_idl = self.fake_ovn_client._nb_idl + nb_idl.db_find.return_value.execute.return_value = [ + lrp1_no_prefix, + lrp2_with_prefix] + + self.assertRaises( + periodics.NeverAgain, + self.periodic.update_lrouter_ports_ext_ids_name_prefix) + + nb_idl.update_lrouter_port.assert_called_once_with( + name='lrp-id1', external_ids=mock.ANY) + def test_check_for_igmp_snoop_support(self): cfg.CONF.set_override('igmp_snooping_enable', True, group='OVS') nb_idl = self.fake_ovn_client._nb_idl diff --git a/neutron/tests/unit/services/ovn_l3/test_plugin.py b/neutron/tests/unit/services/ovn_l3/test_plugin.py index 9e60090fa08..8a8db2d3c00 100644 --- a/neutron/tests/unit/services/ovn_l3/test_plugin.py +++ b/neutron/tests/unit/services/ovn_l3/test_plugin.py @@ -106,7 +106,8 @@ class BaseTestOVNL3RouterPluginMixin(): ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name(self.fake_network['id']), ovn_const.OVN_ROUTER_IS_EXT_GW: 'False', - ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router-id', + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('router-id'), } } self.fake_router_ports = [self.fake_router_port] @@ -172,7 +173,8 @@ class BaseTestOVNL3RouterPluginMixin(): ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name('ext-network-id'), ovn_const.OVN_ROUTER_IS_EXT_GW: 'True', - ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router-id', + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('router-id'), }, 'options': { ovn_const.OVN_ROUTER_PORT_GW_MTU_OPTION: str(default_mtu) @@ -429,7 +431,8 @@ class BaseTestOVNL3RouterPluginMixin(): fake_rtr_intf_networks = ['2001:db8::1/24', '2001:dba::1/24'] payload = self._create_payload_for_router_interface(router_id) self.l3_inst._nb_ovn.db_get.return_value.execute.return_value = { - ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: router_id} + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('router-id')} self.ovn_drv._process_add_router_interface(resources.ROUTER_INTERFACE, events.AFTER_CREATE, self, payload) @@ -448,7 +451,8 @@ class BaseTestOVNL3RouterPluginMixin(): def test_remove_router_interface(self): router_id = 'router-id' self.l3_inst._nb_ovn.lookup.return_value = mock.Mock( - external_ids={ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: router_id}) + external_ids={ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name(router_id)}) self.get_port.side_effect = n_exc.PortNotFound( port_id='router-port-id') @@ -465,7 +469,7 @@ class BaseTestOVNL3RouterPluginMixin(): def test_remove_router_interface_update_lrouter_port(self): router_id = 'router-id' self.l3_inst._nb_ovn.db_get.return_value.execute.return_value = { - ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: router_id} + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name(router_id)} payload = self._create_payload_for_router_interface(router_id, pass_subnet=False) self.ovn_drv._process_remove_router_interface( @@ -482,17 +486,19 @@ class BaseTestOVNL3RouterPluginMixin(): ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name(self.fake_network['id']), ovn_const.OVN_ROUTER_IS_EXT_GW: 'False', - ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: router_id, + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name(router_id) }) def test_remove_router_interface_router_not_found(self): router_id = 'router-id' self.l3_inst._nb_ovn.lookup.return_value = mock.Mock( - external_ids={ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: router_id}) + external_ids={ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name(router_id)}) self.get_port.side_effect = n_exc.PortNotFound( port_id='router-port-id') self.get_router.side_effect = l3_exc.RouterNotFound( - router_id='router-id') + router_id='neutron-router-id') payload = self._create_payload_for_router_interface(router_id, pass_subnet=False) @@ -2177,7 +2183,8 @@ class OVNL3ExtrarouteTests(test_l3_gw.ExtGwModeIntTestCase, 'OVNClient.delete_mac_binding_entries_by_mac', return_value=1) self.setup_notification_driver() - ext_ids = {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router-id'} + ext_ids = {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('router-id')} self.l3_inst._nb_ovn.db_get.return_value.execute.return_value = ext_ids self.l3_inst._nb_ovn.lookup.return_value = mock.Mock( external_ids=ext_ids)