Remove MAC duplicate detection for generated macs

The previous MAC duplicate detection code relied on catching a
DBDuplicateError the correct time which resulted in two problems.
First, the duplicate could have been caused by something else being
flushed to the database at the same time.
Second, the DBDuplicate may not occur at flush time if the conflict
happens due to two concurrent port creations and it will instead
happen at commit time.

This patch just dumps the DBDuplicate catch to allow the API layer
to retry the operation since MAC collisions on a network should be
extremely rare.

Closes-Bug: #1590602
Change-Id: Idf816c07198be3ce745252ac9aa9c4aad8f11910

neutron/db/db_base_plugin_common.py

@@ -80,6 +80,12 @@ class DbBasePluginCommon(common_db_mixin.CommonDbMixin):
     def _generate_mac():
         return utils.get_random_mac(cfg.CONF.base_mac.split(':'))
 
+    def _is_mac_in_use(self, context, network_id, mac_address):
+        return bool(context.session.query(models_v2.Port).
+                    filter(models_v2.Port.network_id == network_id).
+                    filter(models_v2.Port.mac_address == mac_address).
+                    count())
+
     @staticmethod
     def _delete_ip_allocation(context, network_id, subnet_id, ip_address):
 

neutron/db/db_base_plugin_v2.py

@@ -1175,33 +1175,18 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon,
 
         return dns_assignment
 
-    def _create_port_with_mac(self, context, network_id, port_data,
-                              mac_address):
-        try:
-            # since this method could either be used within or outside the
-            # transaction, use convenience method to avoid passing a flag
-            with db_api.autonested_transaction(context.session):
-                db_port = models_v2.Port(mac_address=mac_address, **port_data)
-                context.session.add(db_port)
-                return db_port
-        except db_exc.DBDuplicateEntry:
-            raise 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:
-                return self._create_port_with_mac(
-                    context, network_id, port_data, mac)
-            except 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_db_port_obj(self, context, port_data):
+        if port_data.get('mac_address'):
+            mac_address = port_data.pop('mac_address')
+            if self._is_mac_in_use(context, port_data['network_id'],
+                                   mac_address):
+                raise exc.MacAddressInUse(net_id=port_data['network_id'],
+                                          mac=mac_address)
+        else:
+            mac_address = self._generate_mac()
+        db_port = models_v2.Port(mac_address=mac_address, **port_data)
+        context.session.add(db_port)
+        return db_port
 
     def create_port(self, context, port):
         db_port = self.create_port_db(context, port)
@@ -1227,6 +1212,8 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon,
                          device_id=p['device_id'],
                          device_owner=p['device_owner'],
                          description=p.get('description'))
+        if p.get('mac_address') is not constants.ATTR_NOT_SPECIFIED:
+            port_data['mac_address'] = p.get('mac_address')
         if ('dns-integration' in self.supported_extension_aliases and
             'dns_name' in p):
             request_dns_name = self._get_request_dns_name(p)
@@ -1237,12 +1224,8 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon,
             self._get_network(context, network_id)
 
             # Create the port
-            if p['mac_address'] is constants.ATTR_NOT_SPECIFIED:
-                db_port = self._create_port(context, network_id, port_data)
-                p['mac_address'] = db_port['mac_address']
-            else:
-                db_port = self._create_port_with_mac(
-                    context, network_id, port_data, p['mac_address'])
+            db_port = self._create_db_port_obj(context, port_data)
+            p['mac_address'] = db_port['mac_address']
 
             ips = self.ipam.allocate_ips_for_port_and_store(context, port,
                                                             port_id)

neutron/db/ipam_backend_mixin.py

@@ -107,12 +107,11 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon):
     def _update_db_port(self, context, db_port, new_port, network_id, new_mac):
         # Remove all attributes in new_port which are not in the port DB model
         # and then update the port
-        try:
-            db_port.update(self._filter_non_model_columns(new_port,
-                                                          models_v2.Port))
-            context.session.flush()
-        except db_exc.DBDuplicateEntry:
+        if (new_mac and new_mac != db_port.mac_address and
+                self._is_mac_in_use(context, network_id, new_mac)):
             raise exc.MacAddressInUse(net_id=network_id, mac=new_mac)
+        db_port.update(self._filter_non_model_columns(new_port,
+                                                      models_v2.Port))
 
     def _update_subnet_host_routes(self, context, id, s):
 

neutron/tests/unit/db/test_db_base_plugin_v2.py

@@ -1691,24 +1691,16 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s
             return
         self.fail("No exception for illegal base_mac format")
 
-    def test_mac_exhaustion(self):
-        # rather than actually consuming all MAC (would take a LONG time)
-        # we try to allocate an already allocated mac address
-        cfg.CONF.set_override('mac_generation_retries', 3)
-
-        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 = lib_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(webob.exc.HTTPServiceUnavailable.code,
-                             res.status_int)
-            self.assertEqual(3, create_mock.call_count)
+    def test_is_mac_in_use(self):
+        ctx = context.get_admin_context()
+        with self.port() as port:
+            net_id = port['port']['network_id']
+            mac = port['port']['mac_address']
+            self.assertTrue(self.plugin._is_mac_in_use(ctx, net_id, mac))
+            mac2 = '00:22:00:44:00:66'  # other mac, same network
+            self.assertFalse(self.plugin._is_mac_in_use(ctx, net_id, mac2))
+            net_id2 = port['port']['id']  # other net uuid, same mac
+            self.assertFalse(self.plugin._is_mac_in_use(ctx, net_id2, mac))
 
     def test_requested_duplicate_ip(self):
         with self.subnet() as subnet: