[OVN] Backport duplicate lswitch and NAT gateway_port

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

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(

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():

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):

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:

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):

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):