diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py index a7faf1b212c..45bd7da7fba 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py @@ -145,18 +145,28 @@ def run_idl(self, txn): table = self.api.tables[self.table_name] try: ls = table.rows[self.network_uuid] + except KeyError: + # NOTE: a Logical_Switch created before persist_uuid was used has + # a random register UUID and is only found by its name. The + # Logical_Switch table has no index in the OVN_Northbound schema, + # thus ovsdb-server accepts a second register with the same name. + ls = idlutils.row_by_value(self.api.idl, self.table_name, 'name', + utils.ovn_name(self.network_uuid), + None) + + if ls is not None: if self.may_exist: self.result = rowview.RowView(ls) return msg = _("Switch %s already exists") % self.network_uuid raise RuntimeError(msg) - except KeyError: - # Adding a new LS - if utils.ovs_persist_uuid_supported(txn.idl): - ls = txn.insert(table, new_uuid=self.network_uuid, - persist_uuid=True) - else: - ls = txn.insert(table) + + # Adding a new LS + if utils.ovs_persist_uuid_supported(txn.idl): + ls = txn.insert(table, new_uuid=self.network_uuid, + persist_uuid=True) + else: + ls = txn.insert(table) self.set_columns(ls, **self.columns) ls.name = utils.ovn_name(self.network_uuid) self.result = ls.uuid 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 379f84bb3ee..ee14e81a79b 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py @@ -238,7 +238,7 @@ def __init__(self, ovn_client): self._resources_func_map = { ovn_const.TYPE_NETWORKS: { 'neutron_get': self._ovn_client._plugin.get_network, - 'ovn_get': self._nb_idl.get_lswitch, + 'ovn_get': self._get_lswitch, 'ovn_create': self._ovn_client.create_network, 'ovn_update': self._ovn_client.update_network, 'ovn_delete': self._ovn_client.delete_network, @@ -489,6 +489,13 @@ def check_for_inconsistencies(self): {'res_uuid': row.resource_uuid, 'res_type': row.resource_type}) + def _get_lswitch(self, net_id): + # NOTE: ``get_lswitch`` requires the Logical_Switch name, not the + # Neutron network ID. A Logical_Switch created before persist_uuid + # was used has a random register UUID that does not match the + # network ID; such register is only found by its name. + return self._nb_idl.get_lswitch(utils.ovn_name(net_id)) + def _create_lrouter_port(self, context, port): router_id = port['device_id'] iface_info = self._ovn_client._l3_plugin._add_neutron_router_interface( 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 5ba1539a1ce..0ad4f59011d 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 @@ -932,17 +932,18 @@ def _create_or_update_floatingip(self, context, floatingip, txn=None): 'options': options, } - # If OVN supports gateway_port column for NAT rules set gateway port - # uuid to floating IP without gw port reference - LP#2035281. - router_db = self._l3_plugin.get_router(admin_context, router_id) - gw_port_id = router_db.get('gw_port_id') - lrp = self._nb_idl.get_lrouter_port(gw_port_id) - # If LRP is not bound to a chassis, it means that router can be - # bound instead. In this case we do not want to define - # gateway_port LP#2083527. - if lrp.options.get( - ovn_const.LRP_OPTIONS_RESIDE_REDIR_CH) == 'true': - columns['gateway_port'] = lrp.uuid + # Set gateway_port on NAT rules when distributed floating IPs are + # enabled and the LRP is scheduled on a chassis. History: LP#2035281 + # added gateway_port support, LP#2083527 added a guard for gateway + # routers, and LP#2150866 fixed the guard to check ha_chassis_group. + if ovn_conf.is_ovn_distributed_floating_ip(): + router_db = self._l3_plugin.get_router(admin_context, router_id) + gw_port_id = router_db.get('gw_port_id') + lrp = self._nb_idl.get_lrouter_port(gw_port_id) + # If the gateway LRP is scheduled on a chassis (it has + # ha_chassis_group), then assign the gateway_port reference. + if lrp and lrp.ha_chassis_group: + columns['gateway_port'] = lrp.uuid if ovn_conf.is_ovn_distributed_floating_ip(): if self._nb_idl.lsp_get_up(floatingip['port_id']).execute(): diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index 284b47eb4bb..10b7bb5611c 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -129,6 +129,28 @@ def test_old_network_new_port(self): port_lsp = self.nb_api.lsp_get(port).execute(check_error=True) self.assertIn(port_lsp, n1_ls.ports) + def test_old_network_no_duplicated_lswitch(self): + if not utils.ovs_persist_uuid_supported(self.nb_api): + self.skipTest("OVS persist_uuid not supported") + mock_supported = mock.patch.object(utils, 'ovs_persist_uuid_supported', + return_value=False).start() + network = self._make_network(self.fmt, 'n1', True) + network_id = network['network']['id'] + ls_name = utils.ovn_name(network_id) + n1_ls = self.nb_api.ls_get(ls_name).execute(check_error=True) + self.assertNotEqual(uuid.UUID(network_id), n1_ls.uuid) + mock_supported.return_value = True + + # The Logical_Switch register UUID does not match the network ID; + # adding the same network again must not create a second register + # with the same name. + self.nb_api.ls_add(network_id=network_id, + may_exist=True).execute(check_error=True) + switches = [ls for ls in + self.nb_api.ls_list().execute(check_error=True) + if ls.name == ls_name] + self.assertEqual([n1_ls.uuid], [ls.uuid for ls in switches]) + class TestPortBinding(base.TestOVNFunctionalBase): @@ -1612,7 +1634,9 @@ def test_create_floatingip(self): rules = self.nb_api.get_all_logical_routers_with_rports()[0] fip_rule = rules['dnat_and_snats'][0] - self.assertNotEqual([], fip_rule['gateway_port']) + # gateway_port is only set when distributed FIPs are enabled + # LP#2150866 + self.assertEqual([], fip_rule['gateway_port']) class TestRouterGWPort(_TestRouter): diff --git a/neutron/tests/unit/fake_resources.py b/neutron/tests/unit/fake_resources.py index eee726c6c40..4b585d012b4 100644 --- a/neutron/tests/unit/fake_resources.py +++ b/neutron/tests/unit/fake_resources.py @@ -212,6 +212,7 @@ def __init__(self, **kwargs): class FakeOvsdbTransaction: def __init__(self, **kwargs): self.insert = mock.Mock() + self.idl = mock.Mock() class FakePlugin: diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py index 7fb5ed19d66..c6f892d71f4 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py @@ -13,12 +13,15 @@ # from unittest import mock +import uuid from neutron_lib import constants as n_const +from oslo_utils import uuidutils from ovsdbapp.backend.ovs_idl import idlutils from neutron.common.ovn import constants as ovn_const from neutron.common.ovn import exceptions as ovn_exc +from neutron.common.ovn import utils from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import commands from neutron.tests import base from neutron.tests.unit import fake_resources as fakes @@ -94,6 +97,80 @@ def test_check_liveness(self): self.assertNotEqual(cmd.result, old_ng_cfg) +class TestAddNetworkCommand(TestBaseCommand): + + def setUp(self): + super().setUp() + self.net_id = uuidutils.generate_uuid() + # The OVSDB backend exposes the tables both as "tables" and + # "_tables"; the fake NB IDL only defines the latter. + self.ovn_api.tables = self.ovn_api._tables + self.ls_table = self.ovn_api.tables['Logical_Switch'] + + def _test_network_add(self, persist_uuid): + fake_ls = fakes.FakeOvsdbRow.create_one_ovsdb_row() + self.transaction.insert.return_value = fake_ls + with mock.patch.object(idlutils, 'row_by_value', return_value=None), \ + mock.patch.object(utils, 'ovs_persist_uuid_supported', + return_value=persist_uuid): + cmd = commands.AddNetworkCommand(self.ovn_api, self.net_id) + cmd.run_idl(self.transaction) + if persist_uuid: + self.transaction.insert.assert_called_once_with( + self.ls_table, new_uuid=uuid.UUID(self.net_id), + persist_uuid=True) + else: + self.transaction.insert.assert_called_once_with(self.ls_table) + self.assertEqual(utils.ovn_name(self.net_id), fake_ls.name) + + def test_network_add(self): + self._test_network_add(True) + + def test_network_add_no_persist_uuid_support(self): + self._test_network_add(False) + + def test_network_add_exists_may_exist(self): + fake_ls = fakes.FakeOvsdbRow.create_one_ovsdb_row() + self.ls_table.rows[uuid.UUID(self.net_id)] = fake_ls + cmd = commands.AddNetworkCommand(self.ovn_api, self.net_id, + may_exist=True) + cmd.run_idl(self.transaction) + self.assertEqual(fake_ls.uuid, cmd.result.uuid) + self.transaction.insert.assert_not_called() + + def test_network_add_exists(self): + fake_ls = fakes.FakeOvsdbRow.create_one_ovsdb_row() + self.ls_table.rows[uuid.UUID(self.net_id)] = fake_ls + cmd = commands.AddNetworkCommand(self.ovn_api, self.net_id) + self.assertRaises(RuntimeError, cmd.run_idl, self.transaction) + self.transaction.insert.assert_not_called() + + def test_network_add_legacy_lswitch_may_exist(self): + # A Logical_Switch created before persist_uuid was used has a random + # register UUID and is only found by its name. + fake_ls = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={'name': utils.ovn_name(self.net_id)}) + with mock.patch.object(idlutils, 'row_by_value', + return_value=fake_ls) as mock_row_by_value: + cmd = commands.AddNetworkCommand(self.ovn_api, self.net_id, + may_exist=True) + cmd.run_idl(self.transaction) + mock_row_by_value.assert_called_once_with( + self.ovn_api.idl, 'Logical_Switch', 'name', + utils.ovn_name(self.net_id), None) + self.assertEqual(fake_ls.uuid, cmd.result.uuid) + self.transaction.insert.assert_not_called() + + def test_network_add_legacy_lswitch(self): + fake_ls = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={'name': utils.ovn_name(self.net_id)}) + with mock.patch.object(idlutils, 'row_by_value', + return_value=fake_ls): + cmd = commands.AddNetworkCommand(self.ovn_api, self.net_id) + self.assertRaises(RuntimeError, cmd.run_idl, self.transaction) + self.transaction.insert.assert_not_called() + + class TestAddLSwitchPortCommand(TestBaseCommand): def test_lswitch_not_found(self): 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 f7a0b9e3a40..bf3c7f08868 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 @@ -234,6 +234,12 @@ def _test_fix_create_update_network(self, ovn_rev, neutron_rev): self.fake_ovn_client._plugin.get_network.return_value = self.net self.periodic._fix_create_update(self.ctx, row) + # The Logical_Switch must be retrieved by name; a register + # created before persist_uuid was used does not have the + # network ID as register UUID. + self.fake_ovn_client._nb_idl.get_lswitch.assert_called_once_with( + utils.ovn_name(self.net['id'])) + # Since the revision number was < 0, make sure create_network() # is invoked with the latest version of the object in the neutron # database @@ -253,6 +259,31 @@ def test_fix_network_create(self): def test_fix_network_update(self): self._test_fix_create_update_network(ovn_rev=5, neutron_rev=7) + def test_fix_network_legacy_lswitch(self): + # A Logical_Switch created before persist_uuid was used is only + # found by its name; the maintenance task must not create a second + # register for it. + _nb_idl = self.fake_ovn_client._nb_idl + with db_api.CONTEXT_WRITER.using(self.ctx): + self.net['revision_number'] = 7 + ovn_revision_numbers_db.create_initial_revision( + self.ctx, self.net['id'], constants.TYPE_NETWORKS, + revision_number=5) + row = ovn_revision_numbers_db.get_revision_row(self.ctx, + self.net['id']) + fake_ls = mock.Mock(external_ids={ + constants.OVN_REV_NUM_EXT_ID_KEY: 5}) + _nb_idl.get_lswitch.side_effect = ( + lambda name: fake_ls + if name == utils.ovn_name(self.net['id']) else None) + + self.fake_ovn_client._plugin.get_network.return_value = self.net + self.periodic._fix_create_update(self.ctx, row) + + self.fake_ovn_client.create_network.assert_not_called() + self.fake_ovn_client.update_network.assert_called_once_with( + self.ctx, self.net) + def _test_fix_create_update_port(self, ovn_rev, neutron_rev): _nb_idl = self.fake_ovn_client._nb_idl with db_api.CONTEXT_WRITER.using(self.ctx): diff --git a/neutron/tests/unit/services/ovn_l3/test_plugin.py b/neutron/tests/unit/services/ovn_l3/test_plugin.py index 814dba209c7..ddd2cf7d8d4 100644 --- a/neutron/tests/unit/services/ovn_l3/test_plugin.py +++ b/neutron/tests/unit/services/ovn_l3/test_plugin.py @@ -1191,6 +1191,7 @@ def test_create_floatingip_distributed(self): external_ip='192.168.0.10', external_mac='00:01:02:03:04:05', logical_port='port_id', external_ids=expected_ext_ids, + gateway_port=mock.ANY, options={'stateless': 'false'}, ) @@ -1227,6 +1228,7 @@ def test_create_floatingip_distributed_logical_port_down(self): external_ip='192.168.0.10', logical_port='port_id', external_ids=expected_ext_ids, + gateway_port=mock.ANY, options={'stateless': 'false'}, ) @@ -1347,6 +1349,7 @@ def test_create_floatingip_lb_member_fip(self): logical_ip='10.0.0.10', type='dnat_and_snat', external_ids=expected_ext_ids, + gateway_port=mock.ANY, options={'stateless': 'false'}, ) @@ -1386,6 +1389,7 @@ def test_create_floatingip_lb_vip_fip(self): logical_port='port_id', type='dnat_and_snat', external_ids=expected_ext_ids, + gateway_port=mock.ANY, options={'stateless': 'false'}, ) self.l3_inst._nb_ovn.db_find_rows.assert_called_with( @@ -1396,18 +1400,18 @@ def test_create_floatingip_lb_vip_fip(self): mock.call('NAT', self.fake_ovn_nat_rule.uuid, 'external_mac'), mock.call('NAT', self.fake_ovn_nat_rule.uuid, 'logical_port')]) - def _test_create_floatingip_gateway_port_option(self, is_gw_port): + def _test_create_floatingip_gateway_port_option( + self, distributed_fip, has_hcg=False): _nb_ovn = self.l3_inst._nb_ovn _nb_ovn.is_col_present.return_value = True self._get_floatingip.return_value = {'floating_port_id': 'fip-port-id'} _nb_ovn.get_lrouter_nat_rules.return_value = [ {'external_ip': '192.168.0.10', 'logical_ip': '10.0.0.0/24', 'type': 'snat', 'uuid': 'uuid1'}] - lrp_options = {} - if is_gw_port: - lrp_options[ovn_const.LRP_OPTIONS_RESIDE_REDIR_CH] = 'true' + ha_chassis_group = ['fake-hcg-uuid'] if has_hcg else [] lrp = fake_resources.FakeOvsdbRow.create_one_ovsdb_row( - attrs={'options': lrp_options}) + attrs={'options': {}, + 'ha_chassis_group': ha_chassis_group}) _nb_ovn.get_lrouter_port.return_value = lrp self.l3_inst.get_router.return_value = self.fake_router_with_ext_gw @@ -1415,9 +1419,13 @@ def _test_create_floatingip_gateway_port_option(self, is_gw_port): self.context, states=(self.fake_floating_ip,), resource_id=self.fake_floating_ip['id'], request_body={'floatingip': self.fake_floating_ip}) - self.ovn_drv._process_floatingip_create(resources.FLOATING_IP, - events.AFTER_CREATE, - self, payload) + with mock.patch( + 'neutron.conf.plugins.ml2.drivers.ovn.ovn_conf.' + 'is_ovn_distributed_floating_ip', + return_value=distributed_fip): + self.ovn_drv._process_floatingip_create(resources.FLOATING_IP, + events.AFTER_CREATE, + self, payload) _nb_ovn.set_nat_rule_in_lrouter.assert_not_called() expected_ext_ids = { @@ -1431,33 +1439,32 @@ def _test_create_floatingip_gateway_port_option(self, is_gw_port): ovn_const.OVN_FIP_NET_ID: self.fake_floating_ip['floating_network_id']} - if is_gw_port: - _nb_ovn.add_nat_rule_in_lrouter.assert_called_once_with( - 'neutron-router-id', - type='dnat_and_snat', - logical_ip='10.0.0.10', - external_ip='192.168.0.10', - logical_port='port_id', - external_ids=expected_ext_ids, - gateway_port=lrp.uuid, - options={'stateless': 'false'}, - ) - else: - _nb_ovn.add_nat_rule_in_lrouter.assert_called_once_with( - 'neutron-router-id', - type='dnat_and_snat', - logical_ip='10.0.0.10', - external_ip='192.168.0.10', - logical_port='port_id', - external_ids=expected_ext_ids, - options={'stateless': 'false'}, - ) + expected_kwargs = { + 'type': 'dnat_and_snat', + 'logical_ip': '10.0.0.10', + 'external_ip': '192.168.0.10', + 'logical_port': 'port_id', + 'external_ids': expected_ext_ids, + 'options': {'stateless': 'false'}, + } + if distributed_fip and has_hcg: + expected_kwargs['gateway_port'] = lrp.uuid + if distributed_fip: + expected_kwargs['external_mac'] = 'aa:aa:aa:aa:aa:aa' + _nb_ovn.add_nat_rule_in_lrouter.assert_called_once_with( + 'neutron-router-id', **expected_kwargs) def test_create_floatingip_with_gateway_port(self): - self._test_create_floatingip_gateway_port_option(True) + self._test_create_floatingip_gateway_port_option( + distributed_fip=True, has_hcg=True) def test_create_floatingip_without_gateway_port(self): - self._test_create_floatingip_gateway_port_option(False) + self._test_create_floatingip_gateway_port_option( + distributed_fip=False) + + def test_create_floatingip_no_gateway_port_dfip_without_hcg(self): + self._test_create_floatingip_gateway_port_option( + distributed_fip=True, has_hcg=False) @mock.patch('neutron.db.l3_db.L3_NAT_dbonly_mixin.delete_floatingip') def test_delete_floatingip(self, df): @@ -1673,6 +1680,7 @@ def test_update_floatingip_associate_distributed(self, uf, gn): logical_ip='10.10.10.10', external_ip='192.168.0.10', external_mac='00:01:02:03:04:05', logical_port='new-port_id', external_ids=expected_ext_ids, + gateway_port=mock.ANY, options={'stateless': 'false'}, ) diff --git a/releasenotes/notes/bug-2162974-ovn-duplicated-lswitch-3f2c85b1a7d40e69.yaml b/releasenotes/notes/bug-2162974-ovn-duplicated-lswitch-3f2c85b1a7d40e69.yaml new file mode 100644 index 00000000000..2d6c259e47f --- /dev/null +++ b/releasenotes/notes/bug-2162974-ovn-duplicated-lswitch-3f2c85b1a7d40e69.yaml @@ -0,0 +1,24 @@ +--- +fixes: + - | + [``ML2/OVN``] A ``Logical_Switch`` created before ``persist_uuid`` was + used has a random register UUID and is only identifiable by its + ``neutron-`` name. Two code paths still resolved it by + register UUID: the maintenance task, that passed the Neutron network ID + to ``get_lswitch``, and ``AddNetworkCommand``. Both missed such a + register and created a second one with the same name. Since the + ``Logical_Switch`` table has no index in the ``OVN_Northbound`` schema, + ``ovsdb-server`` accepts the duplicated register; the ports created + afterwards are resolved by name and land on any of them, and those + landing on the duplicated one have neither router port nor metadata + port. Both code paths now retrieve the ``Logical_Switch`` by name. For + more information, see bug + `2162974 `_. +upgrade: + - | + [``ML2/OVN``] Deployments affected by bug + `2162974 `_ can already + have duplicated ``Logical_Switch`` registers. This patch prevents the + creation of new duplicates but does not remove the existing ones. Check + for registers sharing the same ``neutron-`` name and delete + the one that has no router port and no ``ovn-metadata`` port.