From 09fe057cd475fe9068339f9805fa855476e248bf Mon Sep 17 00:00:00 2001 From: Cedric Brandily Date: Sat, 10 Jan 2015 14:25:04 +0000 Subject: [PATCH] Use db constraint to ensure mac address uniqueness Currently port mac address uniqueness per network is checked before Port db object create but without locking. It implies 2 port create requests can allocate the same mac address on a network if each request performs mac address uniqueness check before the other creates the Port db object. This change replaces the check by a db unique constraint on Port (network_id, mac_address). Change-Id: Iad7460356ceb74d963cddf5ec33268d77792f1fe Closes-Bug: #1194565 --- neutron/db/db_base_plugin_v2.py | 114 ++++++++---------- ...b59e0_add_mac_address_unique_constraint.py | 48 ++++++++ .../alembic_migrations/versions/HEAD | 2 +- neutron/db/models_v2.py | 6 + .../tests/unit/metaplugin/test_metaplugin.py | 3 +- neutron/tests/unit/ml2/db/test_ml2_db.py | 12 +- neutron/tests/unit/ml2/db/test_ml2_dvr_db.py | 5 +- neutron/tests/unit/test_db_plugin.py | 22 ++-- 8 files changed, 127 insertions(+), 85 deletions(-) create mode 100644 neutron/db/migration/alembic_migrations/versions/2a1ee2fb59e0_add_mac_address_unique_constraint.py diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index 344ba58e69b..b399517738f 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -13,10 +13,9 @@ # License for the specific language governing permissions and limitations # under the License. -import random - import netaddr from oslo.config import cfg +from oslo.db import exception as db_exc from oslo.utils import excutils from sqlalchemy import and_ from sqlalchemy import event @@ -27,6 +26,7 @@ from neutron.api.v2 import attributes from neutron.common import constants from neutron.common import exceptions as n_exc from neutron.common import ipv6_utils +from neutron.common import utils from neutron import context as ctx from neutron.db import common_db_mixin from neutron.db import models_v2 @@ -127,41 +127,8 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, return context.session.query(models_v2.Subnet).all() @staticmethod - def _generate_mac(context, network_id): - base_mac = cfg.CONF.base_mac.split(':') - max_retries = cfg.CONF.mac_generation_retries - for i in range(max_retries): - mac = [int(base_mac[0], 16), int(base_mac[1], 16), - int(base_mac[2], 16), random.randint(0x00, 0xff), - random.randint(0x00, 0xff), random.randint(0x00, 0xff)] - if base_mac[3] != '00': - mac[3] = int(base_mac[3], 16) - mac_address = ':'.join(map(lambda x: "%02x" % x, mac)) - if NeutronDbPluginV2._check_unique_mac(context, network_id, - mac_address): - LOG.debug("Generated mac for network %(network_id)s " - "is %(mac_address)s", - {'network_id': network_id, - 'mac_address': mac_address}) - return mac_address - else: - LOG.debug("Generated mac %(mac_address)s exists. Remaining " - "attempts %(max_retries)s.", - {'mac_address': mac_address, - 'max_retries': max_retries - (i + 1)}) - LOG.error(_LE("Unable to generate mac address after %s attempts"), - max_retries) - raise n_exc.MacAddressGenerationFailure(net_id=network_id) - - @staticmethod - def _check_unique_mac(context, network_id, mac_address): - mac_qry = context.session.query(models_v2.Port) - try: - mac_qry.filter_by(network_id=network_id, - mac_address=mac_address).one() - except exc.NoResultFound: - return True - return False + def _generate_mac(): + return utils.get_random_mac(cfg.CONF.base_mac.split(':')) @staticmethod def _delete_ip_allocation(context, network_id, subnet_id, ip_address): @@ -1310,6 +1277,36 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, def create_port_bulk(self, context, ports): return self._create_bulk('port', context, ports) + def _create_port_with_mac(self, context, network_id, port_data, + mac_address, nested=False): + try: + with context.session.begin(subtransactions=True, nested=nested): + db_port = models_v2.Port(mac_address=mac_address, **port_data) + context.session.add(db_port) + return db_port + except db_exc.DBDuplicateEntry: + raise n_exc.MacAddressInUse(net_id=network_id, mac=mac_address) + + def _create_port(self, context, network_id, port_data): + max_retries = cfg.CONF.mac_generation_retries + for i in range(max_retries): + mac = self._generate_mac() + try: + # nested = True frames an operation that may potentially fail + # within a transaction, so that it can be rolled back to the + # point before its failure while maintaining the enclosing + # transaction + return self._create_port_with_mac( + context, network_id, port_data, mac, nested=True) + except n_exc.MacAddressInUse: + LOG.debug('Generated mac %(mac_address)s exists on ' + 'network %(network_id)s', + {'mac_address': mac, 'network_id': network_id}) + + LOG.error(_LE("Unable to generate mac address after %s attempts"), + max_retries) + raise n_exc.MacAddressGenerationFailure(net_id=network_id) + def create_port(self, context, port): p = port['port'] port_id = p.get('id') or uuidutils.generate_uuid() @@ -1321,41 +1318,26 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, self._enforce_device_owner_not_router_intf_or_device_id(context, p, tenant_id) + port_data = dict(tenant_id=tenant_id, + name=p['name'], + id=port_id, + network_id=network_id, + admin_state_up=p['admin_state_up'], + status=p.get('status', constants.PORT_STATUS_ACTIVE), + device_id=p['device_id'], + device_owner=p['device_owner']) + with context.session.begin(subtransactions=True): # Ensure that the network exists. self._get_network(context, network_id) - # Ensure that a MAC address is defined and it is unique on the - # network + # Create the port if p['mac_address'] is attributes.ATTR_NOT_SPECIFIED: - #Note(scollins) Add the generated mac_address to the port, - #since _allocate_ips_for_port will need the mac when - #calculating an EUI-64 address for a v6 subnet - p['mac_address'] = NeutronDbPluginV2._generate_mac(context, - network_id) + db_port = self._create_port(context, network_id, port_data) + p['mac_address'] = db_port['mac_address'] else: - # Ensure that the mac on the network is unique - if not NeutronDbPluginV2._check_unique_mac(context, - network_id, - p['mac_address']): - raise n_exc.MacAddressInUse(net_id=network_id, - mac=p['mac_address']) - - if 'status' not in p: - status = constants.PORT_STATUS_ACTIVE - else: - status = p['status'] - - db_port = models_v2.Port(tenant_id=tenant_id, - name=p['name'], - id=port_id, - network_id=network_id, - mac_address=p['mac_address'], - admin_state_up=p['admin_state_up'], - status=status, - device_id=p['device_id'], - device_owner=p['device_owner']) - context.session.add(db_port) + db_port = self._create_port_with_mac( + context, network_id, port_data, p['mac_address']) # Update the IP's for the port ips = self._allocate_ips_for_port(context, port) diff --git a/neutron/db/migration/alembic_migrations/versions/2a1ee2fb59e0_add_mac_address_unique_constraint.py b/neutron/db/migration/alembic_migrations/versions/2a1ee2fb59e0_add_mac_address_unique_constraint.py new file mode 100644 index 00000000000..c5af2d81b76 --- /dev/null +++ b/neutron/db/migration/alembic_migrations/versions/2a1ee2fb59e0_add_mac_address_unique_constraint.py @@ -0,0 +1,48 @@ +# Copyright (c) 2015 Thales Services SAS +# +# 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. +# + +"""Add mac_address unique constraint + +Revision ID: 2a1ee2fb59e0 +Revises: 41662e32bce2 +Create Date: 2015-01-10 11:44:27.550349 + +""" + +# revision identifiers, used by Alembic. +revision = '2a1ee2fb59e0' +down_revision = '41662e32bce2' + +from alembic import op + + +TABLE_NAME = 'ports' +CONSTRAINT_NAME = 'uniq_ports0network_id0mac_address' + + +def upgrade(): + op.create_unique_constraint( + name=CONSTRAINT_NAME, + source=TABLE_NAME, + local_cols=['network_id', 'mac_address'] + ) + + +def downgrade(): + op.drop_constraint( + name=CONSTRAINT_NAME, + source=TABLE_NAME, + local_cols=['network_id', 'mac_address'] + ) diff --git a/neutron/db/migration/alembic_migrations/versions/HEAD b/neutron/db/migration/alembic_migrations/versions/HEAD index 77b1edbe83e..19ae3849e05 100644 --- a/neutron/db/migration/alembic_migrations/versions/HEAD +++ b/neutron/db/migration/alembic_migrations/versions/HEAD @@ -1 +1 @@ -41662e32bce2 +2a1ee2fb59e0 diff --git a/neutron/db/models_v2.py b/neutron/db/models_v2.py index 8def401e7dd..f721aac83b5 100644 --- a/neutron/db/models_v2.py +++ b/neutron/db/models_v2.py @@ -135,6 +135,12 @@ class Port(model_base.BASEV2, HasId, HasTenant): status = sa.Column(sa.String(16), nullable=False) device_id = sa.Column(sa.String(255), nullable=False) device_owner = sa.Column(sa.String(255), nullable=False) + __table_args__ = ( + sa.UniqueConstraint( + network_id, mac_address, + name='uniq_ports0network_id0mac_address'), + model_base.BASEV2.__table_args__ + ) def __init__(self, id=None, tenant_id=None, name=None, network_id=None, mac_address=None, admin_state_up=None, status=None, diff --git a/neutron/tests/unit/metaplugin/test_metaplugin.py b/neutron/tests/unit/metaplugin/test_metaplugin.py index 4f94ed7ac4c..eefb4545c61 100644 --- a/neutron/tests/unit/metaplugin/test_metaplugin.py +++ b/neutron/tests/unit/metaplugin/test_metaplugin.py @@ -125,8 +125,7 @@ class MetaNeutronPluginV2Test(testlib_api.SqlTestCase, 'admin_state_up': True, 'host_routes': [], 'fixed_ips': [], - 'mac_address': - self.plugin._generate_mac(self.context, net_id), + 'mac_address': self.plugin._generate_mac(), 'tenant_id': self.fake_tenant_id}} def _fake_subnet(self, net_id): diff --git a/neutron/tests/unit/ml2/db/test_ml2_db.py b/neutron/tests/unit/ml2/db/test_ml2_db.py index 23236921667..a3890690f0d 100644 --- a/neutron/tests/unit/ml2/db/test_ml2_db.py +++ b/neutron/tests/unit/ml2/db/test_ml2_db.py @@ -14,6 +14,7 @@ # limitations under the License. from neutron import context +from neutron.db import db_base_plugin_v2 from neutron.db import models_v2 from neutron.extensions import portbindings from neutron.openstack.common import uuidutils @@ -34,15 +35,17 @@ class Ml2DBTestCase(testlib_api.SqlTestCase): self.ctx.session.add(models_v2.Network(id=network_id)) def _setup_neutron_port(self, network_id, port_id): + mac_address = db_base_plugin_v2.NeutronDbPluginV2._generate_mac() with self.ctx.session.begin(subtransactions=True): port = models_v2.Port(id=port_id, network_id=network_id, - mac_address='foo_mac_address', + mac_address=mac_address, admin_state_up=True, status='DOWN', device_id='', device_owner='') self.ctx.session.add(port) + return port def _setup_neutron_portbinding(self, port_id, vif_type, host): with self.ctx.session.begin(subtransactions=True): @@ -190,12 +193,11 @@ class Ml2DBTestCase(testlib_api.SqlTestCase): def test_get_port_from_device_mac(self): network_id = 'foo-network-id' port_id = 'foo-port-id' - mac_address = 'foo_mac_address' self._setup_neutron_network(network_id) - self._setup_neutron_port(network_id, port_id) + port = self._setup_neutron_port(network_id, port_id) - port = ml2_db.get_port_from_device_mac(mac_address) - self.assertEqual(port_id, port.id) + observed_port = ml2_db.get_port_from_device_mac(port['mac_address']) + self.assertEqual(port_id, observed_port.id) def test_get_locked_port_and_binding(self): network_id = 'foo-network-id' diff --git a/neutron/tests/unit/ml2/db/test_ml2_dvr_db.py b/neutron/tests/unit/ml2/db/test_ml2_dvr_db.py index c2f6a9e59cb..d4520cdc255 100644 --- a/neutron/tests/unit/ml2/db/test_ml2_dvr_db.py +++ b/neutron/tests/unit/ml2/db/test_ml2_dvr_db.py @@ -18,6 +18,7 @@ import mock from sqlalchemy.orm import query from neutron import context +from neutron.db import db_base_plugin_v2 from neutron.db import l3_db from neutron.db import models_v2 from neutron.extensions import portbindings @@ -37,9 +38,11 @@ class Ml2DvrDBTestCase(testlib_api.SqlTestCase): self.ctx.session.add(models_v2.Network(id=network_id)) ports = [] for port_id in port_ids: + mac_address = (db_base_plugin_v2.NeutronDbPluginV2. + _generate_mac()) port = models_v2.Port(id=port_id, network_id=network_id, - mac_address='foo_mac_address', + mac_address=mac_address, admin_state_up=True, status='ACTIVE', device_id='', diff --git a/neutron/tests/unit/test_db_plugin.py b/neutron/tests/unit/test_db_plugin.py index 576926f99c4..d56bf246600 100644 --- a/neutron/tests/unit/test_db_plugin.py +++ b/neutron/tests/unit/test_db_plugin.py @@ -1298,20 +1298,22 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s def test_mac_exhaustion(self): # rather than actually consuming all MAC (would take a LONG time) - # we just raise the exception that would result. - @staticmethod - def fake_gen_mac(context, net_id): - raise n_exc.MacAddressGenerationFailure(net_id=net_id) + # we try to allocate an already allocated mac address + cfg.CONF.set_override('mac_generation_retries', 3) - with mock.patch.object(neutron.db.db_base_plugin_v2.NeutronDbPluginV2, - '_generate_mac', new=fake_gen_mac): - res = self._create_network(fmt=self.fmt, name='net1', - admin_state_up=True) - network = self.deserialize(self.fmt, res) - net_id = network['network']['id'] + res = self._create_network(fmt=self.fmt, name='net1', + admin_state_up=True) + network = self.deserialize(self.fmt, res) + net_id = network['network']['id'] + + error = n_exc.MacAddressInUse(net_id=net_id, mac='00:11:22:33:44:55') + with mock.patch.object( + neutron.db.db_base_plugin_v2.NeutronDbPluginV2, + '_create_port_with_mac', side_effect=error) as create_mock: res = self._create_port(self.fmt, net_id=net_id) self.assertEqual(res.status_int, webob.exc.HTTPServiceUnavailable.code) + self.assertEqual(3, create_mock.call_count) def test_requested_duplicate_ip(self): with self.subnet() as subnet: