Ensure there is always at most 1 dvr fip gw port per agent and network

In patch [1] there was introduced simple lock for creation of
DVR agent's floating IP gateway ports for network to avoid races
and creation of duplicated ports for one agent and one network.

This fix from [1] works in simple examples with only one neutron-server,
so it helped e.g. in CI but it wasn't proper fix for production
deployments which are much bigger and have more neutron server api
workers.

So this patch introduces constraint on database level so this works even
across cluster with multiple neutron-server api workers.

[1] https://review.opendev.org/#/c/673331/

Change-Id: Id55b8a21d6ecf5e029d1ca267b2cbd2ed91cca4c
Closes-Bug: #1830763

neutron/db/l3_dvr_db.py

@@ -26,6 +26,7 @@ from neutron_lib.db import api as db_api
 from neutron_lib import exceptions as n_exc
 from neutron_lib.exceptions import agent as agent_exc
 from neutron_lib.exceptions import l3 as l3_exc
+from neutron_lib.objects import exceptions as o_exc
 from neutron_lib.plugins import constants as plugin_constants
 from neutron_lib.plugins import directory
 from neutron_lib.plugins import utils as plugin_utils
@@ -1013,16 +1014,41 @@ class _DVRAgentInterfaceMixin(object):
                         {'ag': const.AGENT_TYPE_L3,
                          'host': host})
             return
+        if not l3_agent_db:
+            return
+
         l3_agent_mode = self._get_agent_mode(l3_agent_db)
         if l3_agent_mode == const.L3_AGENT_MODE_DVR_NO_EXTERNAL:
             return
-        if l3_agent_db:
-            LOG.debug("Agent ID exists: %s", l3_agent_db['id'])
-            agent_port = self._get_agent_gw_ports_exist_for_network(
-                context, network_id, host, l3_agent_db['id'])
-            if not agent_port:
-                LOG.info("Floating IP Agent Gateway port does not exist, "
-                         "creating one")
+
+        LOG.debug("Agent ID exists: %s", l3_agent_db['id'])
+        agent_port = self._get_agent_gw_ports_exist_for_network(
+            context, network_id, host, l3_agent_db['id'])
+        if not agent_port:
+            LOG.info("Floating IP Agent Gateway port for network %(network)s "
+                     "does not exist on host %(host)s. Creating one.",
+                     {'network': network_id,
+                      'host': host})
+            fip_agent_port_obj = l3_obj.DvrFipGatewayPortAgentBinding(
+                context,
+                network_id=network_id,
+                agent_id=l3_agent_db['id']
+            )
+            try:
+                fip_agent_port_obj.create()
+            except o_exc.NeutronDbObjectDuplicateEntry:
+                LOG.debug("Floating IP Agent Gateway port for network "
+                          "%(network)s already exists on host %(host)s. "
+                          "Probably it was just created by other worker.",
+                          {'network': network_id,
+                           'host': host})
+                agent_port = self._get_agent_gw_ports_exist_for_network(
+                    context, network_id, host, l3_agent_db['id'])
+                LOG.debug("Floating IP Agent Gateway port %(gw)s found "
+                          "for the destination host: %(dest_host)s",
+                          {'gw': agent_port,
+                           'dest_host': host})
+            else:
                 port_data = {'tenant_id': '',
                              'network_id': network_id,
                              'device_id': l3_agent_db['id'],
@@ -1033,6 +1059,7 @@ class _DVRAgentInterfaceMixin(object):
                 agent_port = plugin_utils.create_port(
                     self._core_plugin, context, {'port': port_data})
                 if not agent_port:
+                    fip_agent_port_obj.delete()
                     msg = _("Unable to create Floating IP Agent Gateway port")
                     raise n_exc.BadRequest(resource='router', msg=msg)
                 LOG.debug("Floating IP Agent Gateway port %(gw)s created "
@@ -1040,8 +1067,8 @@ class _DVRAgentInterfaceMixin(object):
                           {'gw': agent_port,
                            'dest_host': host})
 
-            self._populate_mtu_and_subnets_for_ports(context, [agent_port])
-            return agent_port
+        self._populate_mtu_and_subnets_for_ports(context, [agent_port])
+        return agent_port
 
     def _get_subnet_id_for_given_fixed_ip(self, context, fixed_ip, port_dict):
         """Returns the subnet_id that matches the fixedip on a network."""

neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD

@@ -1 +1 @@
-Ibac91d24da2
+2217c4222de6

neutron/db/migration/alembic_migrations/versions/ussuri/expand/2217c4222de6_add_dvr_fip_gateway_port_network_table.py

@@ -0,0 +1,42 @@
+# Copyright 2020 OpenStack Foundation
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+#
+
+from alembic import op
+import sqlalchemy as sa
+
+
+"""add dvr FIP gateway port network table
+
+Revision ID: 2217c4222de6
+Revises: Ibac91d24da2
+Create Date: 2020-01-13 01:47:11.649472
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '2217c4222de6'
+down_revision = 'Ibac91d24da2'
+
+
+def upgrade():
+    op.create_table(
+        'dvr_fip_gateway_port_network',
+        sa.Column('network_id', sa.String(length=36),
+                  sa.ForeignKey('networks.id', ondelete='CASCADE'),
+                  primary_key=True),
+        sa.Column('agent_id', sa.String(length=36),
+                  sa.ForeignKey('agents.id', ondelete='CASCADE'),
+                  primary_key=True)
+    )

neutron/db/models/dvr.py

@@ -24,3 +24,20 @@ class DistributedVirtualRouterMacAddress(model_base.BASEV2):
 
     host = sa.Column(sa.String(255), primary_key=True, nullable=False)
     mac_address = sa.Column(sa.String(32), nullable=False, unique=True)
+
+
+class DvrFipGatewayPortAgentBinding(model_base.BASEV2):
+    """Represents a binding of agent's FIP gateway port and L3 agent.
+
+    Each L3 DVR agent can only have one FIP gateway port per network.
+    This table represents this constraint.
+    """
+
+    __tablename__ = 'dvr_fip_gateway_port_network'
+
+    network_id = sa.Column(sa.String(36), sa.ForeignKey("networks.id",
+                                                        ondelete="CASCADE"),
+                           primary_key=True)
+    agent_id = sa.Column(sa.String(36), sa.ForeignKey("agents.id",
+                                                      ondelete="CASCADE"),
+                         primary_key=True)

neutron/objects/router.py

@@ -309,3 +309,19 @@ class FloatingIP(base.NeutronDbObject):
         for key, value in group_iterator:
             row = [r for r in six.next(value)]
             yield (cls._load_object(context, row[0]), row[1])
+
+
+@base.NeutronObjectRegistry.register
+class DvrFipGatewayPortAgentBinding(base.NeutronDbObject):
+    # Version 1.0: Initial version
+    VERSION = '1.0'
+
+    db_model = dvr_models.DvrFipGatewayPortAgentBinding
+    new_facade = True
+
+    primary_keys = ['network_id', 'agent_id']
+
+    fields = {
+        'network_id': common_types.UUIDField(),
+        'agent_id': common_types.UUIDField(),
+    }

neutron/tests/unit/db/test_l3_dvr_db.py

@@ -22,6 +22,7 @@ from neutron_lib import constants as const
 from neutron_lib import context
 from neutron_lib import exceptions
 from neutron_lib.exceptions import l3 as l3_exc
+from neutron_lib.objects import exceptions as o_exc
 from neutron_lib.plugins import constants as plugin_constants
 from neutron_lib.plugins import directory
 from neutron_lib.plugins import utils as plugin_utils
@@ -625,6 +626,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
         self.assertTrue(create_fip.called)
 
     def test_create_fip_agent_gw_port_if_not_exists_with_l3_agent(self):
+        network_id = _uuid()
         fport_db = {'id': _uuid()}
         self.mixin._get_agent_gw_ports_exist_for_network = mock.Mock(
             return_value=fport_db)
@@ -641,7 +643,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
             return_value=fipagent)
         fport = self.mixin.create_fip_agent_gw_port_if_not_exists(
                                                 self.ctx,
-                                                'network_id',
+                                                network_id,
                                                 'host')
         self.assertIsNone(fport)
 
@@ -655,10 +657,84 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
                 configurations={"agent_mode": "dvr"})
         self.mixin._get_agent_by_type_and_host = mock.Mock(
             return_value=fipagent)
-        fport = self.mixin.create_fip_agent_gw_port_if_not_exists(
-                                                self.ctx,
-                                                'network_id',
-                                                'host')
+        with mock.patch.object(
+            router_obj, "DvrFipGatewayPortAgentBinding",
+        ) as dvr_fip_agent_port_obj:
+            dvr_fip_agent_port_obj_instance = (
+                dvr_fip_agent_port_obj.return_value)
+            fport = self.mixin.create_fip_agent_gw_port_if_not_exists(
+                                                    self.ctx,
+                                                    network_id,
+                                                    'host')
+
+        dvr_fip_agent_port_obj_instance.create.assert_not_called()
+        self.assertIsNotNone(fport)
+        dvr_fip_agent_port_obj_instance.delete.assert_not_called()
+
+    def test_create_fip_agent_gw_port_agent_port_not_created(self):
+        network_id = _uuid()
+        self.mixin._get_agent_gw_ports_exist_for_network = mock.Mock(
+            return_value=None)
+        fipagent = agent_obj.Agent(
+                self.ctx,
+                id=_uuid(),
+                binary='foo-agent',
+                host='host',
+                agent_type='L3 agent',
+                topic='foo_topic',
+                configurations={"agent_mode": "dvr"})
+        self.mixin._get_agent_by_type_and_host = mock.Mock(
+            return_value=fipagent)
+
+        with mock.patch.object(
+            router_obj, "DvrFipGatewayPortAgentBinding",
+        ) as dvr_fip_agent_port_obj,\
+            mock.patch.object(
+                plugin_utils, "create_port", return_value=None):
+
+            dvr_fip_agent_port_obj_instance = (
+                dvr_fip_agent_port_obj.return_value)
+
+            self.assertRaises(
+                exceptions.BadRequest,
+                self.mixin.create_fip_agent_gw_port_if_not_exists,
+                self.ctx, network_id, 'host')
+
+        dvr_fip_agent_port_obj_instance.create.assert_called_once_with()
+        self.mixin._get_agent_gw_ports_exist_for_network.\
+            assert_called_once_with(
+                self.ctx, network_id, 'host', fipagent['id'])
+        dvr_fip_agent_port_obj_instance.delete.assert_called_once_with()
+
+    def test_create_fip_agent_gw_port_if_not_exists_duplicate_port(self):
+        network_id = _uuid()
+        fport_db = {'id': _uuid()}
+        self.mixin._get_agent_gw_ports_exist_for_network = mock.Mock(
+            side_effect=[None, fport_db])
+        fipagent = agent_obj.Agent(
+                self.ctx,
+                id=_uuid(),
+                binary='foo-agent',
+                host='host',
+                agent_type='L3 agent',
+                topic='foo_topic',
+                configurations={"agent_mode": "dvr"})
+        self.mixin._get_agent_by_type_and_host = mock.Mock(
+            return_value=fipagent)
+
+        with mock.patch.object(
+            router_obj.DvrFipGatewayPortAgentBinding, 'create',
+            side_effect=o_exc.NeutronDbObjectDuplicateEntry(
+                mock.Mock(), mock.Mock())
+        ) as dvr_fip_gateway_port_agent_binding_create:
+            fport = self.mixin.create_fip_agent_gw_port_if_not_exists(
+                                                    self.ctx,
+                                                    network_id,
+                                                    'host')
+        dvr_fip_gateway_port_agent_binding_create.assert_called_once_with()
+        self.mixin._get_agent_gw_ports_exist_for_network.assert_has_calls([
+            mock.call(self.ctx, network_id, 'host', fipagent['id']),
+            mock.call(self.ctx, network_id, 'host', fipagent['id'])])
         self.assertIsNotNone(fport)
 
     def test_create_floatingip_agent_gw_port_with_non_dvr_router(self):

neutron/tests/unit/objects/test_objects.py

@@ -35,6 +35,7 @@ object_data = {
     'DistributedPortBinding': '1.0-39c0d17b281991dcb66716fee5a8bef2',
     'DNSNameServer': '1.0-bf87a85327e2d812d1666ede99d9918b',
     'ExternalNetwork': '1.0-53d885e033cb931f9bb3bdd6bbe3f0ce',
+    'DvrFipGatewayPortAgentBinding': '1.0-ee2af3296265a5463de0bc3695b35b51',
     'DVRMacAddress': '1.0-d3c61a8338d20da74db2364d4d6554f2',
     'ExtraDhcpOpt': '1.0-632f689cbeb36328995a7aed1d0a78d3',
     'FlatAllocation': '1.0-bf666f24f4642b047eeca62311fbcb41',

neutron/tests/unit/objects/test_router.py

@@ -168,3 +168,9 @@ class FloatingIPDbObjectTestCase(obj_test_base.BaseDbObjectTestCase,
             {'floating_port_id': lambda: self._create_test_port_id(),
              'fixed_port_id': lambda: self._create_test_port_id(),
              'router_id': lambda: self._create_test_router_id()})
+
+
+class DvrFipGatewayPortAgentBindingTestCase(
+        obj_test_base.BaseObjectIfaceTestCase):
+
+    _test_class = router.DvrFipGatewayPortAgentBinding