From 93d9d6bbba083ede727aa233903edc3456c5dbeb Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Mon, 13 Jan 2020 11:26:28 +0100 Subject: [PATCH] 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 | 45 ++++++++-- .../alembic_migrations/versions/EXPAND_HEAD | 2 +- ..._add_dvr_fip_gateway_port_network_table.py | 42 +++++++++ neutron/db/models/dvr.py | 17 ++++ neutron/objects/router.py | 16 ++++ neutron/tests/unit/db/test_l3_dvr_db.py | 86 +++++++++++++++++-- neutron/tests/unit/objects/test_objects.py | 1 + neutron/tests/unit/objects/test_router.py | 6 ++ 8 files changed, 200 insertions(+), 15 deletions(-) create mode 100644 neutron/db/migration/alembic_migrations/versions/ussuri/expand/2217c4222de6_add_dvr_fip_gateway_port_network_table.py diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index ce523b68c06..fc8e4ecaefa 100644 --- a/neutron/db/l3_dvr_db.py +++ b/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.""" diff --git a/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD b/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD index f0871630e7a..593cfb3be89 100644 --- a/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD +++ b/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD @@ -1 +1 @@ -Ibac91d24da2 +2217c4222de6 diff --git a/neutron/db/migration/alembic_migrations/versions/ussuri/expand/2217c4222de6_add_dvr_fip_gateway_port_network_table.py b/neutron/db/migration/alembic_migrations/versions/ussuri/expand/2217c4222de6_add_dvr_fip_gateway_port_network_table.py new file mode 100644 index 00000000000..ced08c14a59 --- /dev/null +++ b/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) + ) diff --git a/neutron/db/models/dvr.py b/neutron/db/models/dvr.py index 27551ccd48b..17488d5e426 100644 --- a/neutron/db/models/dvr.py +++ b/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) diff --git a/neutron/objects/router.py b/neutron/objects/router.py index d04967053e9..d7a908e019f 100644 --- a/neutron/objects/router.py +++ b/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(), + } diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index 6e993a465ee..40f4b8aa1dc 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/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): diff --git a/neutron/tests/unit/objects/test_objects.py b/neutron/tests/unit/objects/test_objects.py index 001b6adfdf1..e3b0e6c9cc5 100644 --- a/neutron/tests/unit/objects/test_objects.py +++ b/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', diff --git a/neutron/tests/unit/objects/test_router.py b/neutron/tests/unit/objects/test_router.py index 4c82eaacba0..ea98d352452 100644 --- a/neutron/tests/unit/objects/test_router.py +++ b/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