Handle exceptions on create_dhcp_port

If a network/subnet is deleted while creating the dhcp
port, the agent will detect a conflict on state of the
network and deal with it accordingly.

A concurrent delete may manifest itself via a number
of exceptions, IPAddressGenerationFailure amongst others,
hence the refactoring of the error handling logic into its
own utility method.

Partial-bug: #1253344
Related-bug: #1243726

Change-Id: I442beb5f82f3db8786eea53926903ef0ba0efbf1

neutron/agent/dhcp_agent.py

@@ -127,7 +127,13 @@ class DhcpAgent(manager.Manager):
 
             getattr(driver, action)(**action_kwargs)
             return True
-
+        except exceptions.Conflict:
+            # No need to resync here, the agent will receive the event related
+            # to a status update for the network
+            LOG.warning(_('Unable to %(action)s dhcp for %(net_id)s: there is '
+                          'a conflict with its current state; please check '
+                          'that the network and/or its subnet(s) still exist.')
+                        % {'net_id': network.id, 'action': action})
         except Exception as e:
             self.needs_resync = True
             if (isinstance(e, common.RemoteError)
@@ -432,11 +438,13 @@ class DhcpPluginApi(proxy.RpcProxy):
 
     def create_dhcp_port(self, port):
         """Make a remote process call to create the dhcp port."""
-        return dhcp.DictModel(self.call(self.context,
+        port = self.call(self.context,
-                                        self.make_msg('create_dhcp_port',
+                         self.make_msg('create_dhcp_port',
-                                                      port=port,
+                                       port=port,
-                                                      host=self.host),
+                                       host=self.host),
-                                        topic=self.topic))
+                         topic=self.topic)
+        if port:
+            return dhcp.DictModel(port)
 
     def update_dhcp_port(self, port_id, port):
         """Make a remote process call to update the dhcp port."""

neutron/agent/linux/dhcp.py

@@ -691,6 +691,9 @@ class DeviceManager(object):
                 fixed_ips=[dict(subnet_id=s) for s in dhcp_enabled_subnet_ids])
             dhcp_port = self.plugin.create_dhcp_port({'port': port_dict})
 
+        if not dhcp_port:
+            raise exceptions.Conflict()
+
         # Convert subnet_id to subnet dict
         fixed_ips = [dict(subnet_id=fixed_ip.subnet_id,
                           ip_address=fixed_ip.ip_address,

neutron/db/dhcp_rpc_base.py

@@ -46,6 +46,31 @@ class DhcpRpcCallbackMixin(object):
             nets = plugin.get_networks(context, filters=filters)
         return nets
 
+    def _port_action(self, plugin, context, port, action):
+        """Perform port operations taking care of concurrency issues."""
+        try:
+            if action == 'create_port':
+                return plugin.create_port(context, port)
+            else:
+                msg = _('Unrecognized action')
+                raise n_exc.Invalid(message=msg)
+        except (db_exc.DBError, n_exc.NetworkNotFound,
+                n_exc.SubnetNotFound, n_exc.IpAddressGenerationFailure) as e:
+            if isinstance(e, n_exc.IpAddressGenerationFailure):
+                # Check if the subnet still exists and if it does not, this is
+                # the reason why the ip address generation failed. In any other
+                # unlikely event re-raise
+                try:
+                    subnet_id = port['port']['fixed_ips'][0]['subnet_id']
+                    plugin.get_subnet(context, subnet_id)
+                except n_exc.SubnetNotFound:
+                    pass
+                else:
+                    raise
+            network_id = port['port']['network_id']
+            LOG.warn(_("Port for network %(net_id)s could not be created: "
+                       "%(reason)s") % {"net_id": network_id, 'reason': e})
+
     def get_active_networks(self, context, **kwargs):
         """Retrieve and return a list of the active network ids."""
         # NOTE(arosen): This method is no longer used by the DHCP agent but is
@@ -99,7 +124,7 @@ class DhcpRpcCallbackMixin(object):
 
         This method will re-use an existing port if one already exists.  When a
         port is re-used, the fixed_ip allocation will be updated to the current
-        network state.
+        network state. If an expected failure occurs, a None port is returned.
 
         """
         host = kwargs.get('host')
@@ -164,14 +189,9 @@ class DhcpRpcCallbackMixin(object):
                 device_owner='network:dhcp',
                 fixed_ips=[dict(subnet_id=s) for s in dhcp_enabled_subnet_ids])
 
-            try:
+            retval = self._port_action(plugin, context, {'port': port_dict},
-                retval = plugin.create_port(context, dict(port=port_dict))
+                                       'create_port')
-            except (db_exc.DBError,
+            if not retval:
-                    n_exc.NetworkNotFound,
-                    n_exc.SubnetNotFound,
-                    n_exc.IpAddressGenerationFailure) as e:
-                LOG.warn(_("Port for network %(net_id)s could not be created: "
-                           "%(reason)s") % {"net_id": network_id, 'reason': e})
                 return
 
         # Convert subnet_id to subnet dict
@@ -229,7 +249,11 @@ class DhcpRpcCallbackMixin(object):
                       'from host %s.'), host)
 
     def create_dhcp_port(self, context, **kwargs):
-        """Create the dhcp port."""
+        """Create and return dhcp port information.
+
+        If an expected failure occurs, a None port is returned.
+
+        """
         host = kwargs.get('host')
         port = kwargs.get('port')
         LOG.debug(_('Create dhcp port %(port)s '
@@ -242,7 +266,7 @@ class DhcpRpcCallbackMixin(object):
         if 'mac_address' not in port['port']:
             port['port']['mac_address'] = attributes.ATTR_NOT_SPECIFIED
         plugin = manager.NeutronManager.get_plugin()
-        return plugin.create_port(context, port)
+        return self._port_action(plugin, context, port, 'create_port')
 
     def update_dhcp_port(self, context, **kwargs):
         """Update the dhcp port."""

neutron/tests/unit/test_db_rpc_base.py

@@ -17,6 +17,7 @@ import mock
 
 from neutron.common import exceptions as n_exc
 from neutron.db import dhcp_rpc_base
+from neutron.openstack.common.db import exception as db_exc
 from neutron.tests import base
 
 
@@ -50,6 +51,53 @@ class TestDhcpRpcCallackMixin(base.BaseTestCase):
 
         self.assertEqual(len(self.log.mock_calls), 1)
 
+    def _test__port_action_with_failures(self, exc=None, action=None):
+        port = {
+            'network_id': 'foo_network_id',
+            'device_owner': 'network:dhcp',
+            'fixed_ips': [{'subnet_id': 'foo_subnet_id'}]
+        }
+        self.plugin.create_port.side_effect = exc
+        self.assertIsNone(self.callbacks._port_action(self.plugin,
+                                                      mock.Mock(),
+                                                      {'port': port},
+                                                      action))
+
+    def test__port_action_bad_action(self):
+        self.assertRaises(
+            n_exc.Invalid,
+            self._test__port_action_with_failures,
+            exc=None,
+            action='foo_action')
+
+    def test_create_port_catch_network_not_found(self):
+        self._test__port_action_with_failures(
+            exc=n_exc.NetworkNotFound(net_id='foo_network_id'),
+            action='create_port')
+
+    def test_create_port_catch_subnet_not_found(self):
+        self._test__port_action_with_failures(
+            exc=n_exc.SubnetNotFound(subnet_id='foo_subnet_id'),
+            action='create_port')
+
+    def test_create_port_catch_db_error(self):
+        self._test__port_action_with_failures(exc=db_exc.DBError(),
+                                              action='create_port')
+
+    def test_create_port_catch_ip_generation_failure_reraise(self):
+        self.assertRaises(
+            n_exc.IpAddressGenerationFailure,
+            self._test__port_action_with_failures,
+            exc=n_exc.IpAddressGenerationFailure(net_id='foo_network_id'),
+            action='create_port')
+
+    def test_create_port_catch_and_handle_ip_generation_failure(self):
+        self.plugin.get_subnet.side_effect = (
+            n_exc.SubnetNotFound(subnet_id='foo_subnet_id'))
+        self._test__port_action_with_failures(
+            exc=n_exc.IpAddressGenerationFailure(net_id='foo_network_id'),
+            action='create_port')
+
     def test_get_network_info_return_none_on_not_found(self):
         self.plugin.get_network.side_effect = n_exc.NetworkNotFound(net_id='a')
         retval = self.callbacks.get_network_info(mock.Mock(), network_id='a')
@@ -110,38 +158,6 @@ class TestDhcpRpcCallackMixin(base.BaseTestCase):
                                         update_port=port_retval)
         self.assertEqual(len(self.log.mock_calls), 1)
 
-    def _test_get_dhcp_port_with_failures(self,
-                                          raise_get_network=None,
-                                          raise_create_port=None):
-        self.plugin.update_port.return_value = None
-        if raise_get_network:
-            self.plugin.get_network.side_effect = raise_get_network
-        else:
-            self.plugin.get_network.return_value = {'tenant_id': 'foo_tenant'}
-        if raise_create_port:
-            self.plugin.create_port.side_effect = raise_create_port
-        retval = self.callbacks.get_dhcp_port(mock.Mock(),
-                                              network_id='netid',
-                                              device_id='devid',
-                                              host='host')
-        self.assertIsNone(retval)
-
-    def test_get_dhcp_port_catch_not_found_on_get_network(self):
-        self._test_get_dhcp_port_with_failures(
-            raise_get_network=n_exc.NetworkNotFound(net_id='a'))
-
-    def test_get_dhcp_port_catch_network_not_found_on_create_port(self):
-        self._test_get_dhcp_port_with_failures(
-            raise_create_port=n_exc.NetworkNotFound(net_id='a'))
-
-    def test_get_dhcp_port_catch_subnet_not_found_on_create_port(self):
-        self._test_get_dhcp_port_with_failures(
-            raise_create_port=n_exc.SubnetNotFound(subnet_id='b'))
-
-    def test_get_dhcp_port_catch_ip_generation_failure_on_create_port(self):
-        self._test_get_dhcp_port_with_failures(
-            raise_create_port=n_exc.IpAddressGenerationFailure(net_id='a'))
-
     def _test_get_dhcp_port_create_new(self, update_port=None):
         self.plugin.get_network.return_value = dict(tenant_id='tenantid')
         create_spec = dict(tenant_id='tenantid', device_id='devid',

neutron/tests/unit/test_dhcp_agent.py

@@ -206,7 +206,8 @@ class TestDhcpAgent(base.BaseTestCase):
                                             mock.ANY,
                                             mock.ANY)
 
-    def _test_call_driver_failure(self, exc=None, trace_level='exception'):
+    def _test_call_driver_failure(self, exc=None,
+                                  trace_level='exception', expected_sync=True):
         network = mock.Mock()
         network.id = '1'
         self.driver.return_value.foo.side_effect = exc or Exception
@@ -219,7 +220,7 @@ class TestDhcpAgent(base.BaseTestCase):
                                                 mock.ANY,
                                                 mock.ANY)
             self.assertEqual(log.call_count, 1)
-            self.assertTrue(dhcp.needs_resync)
+            self.assertEqual(expected_sync, dhcp.needs_resync)
 
     def test_call_driver_failure(self):
         self._test_call_driver_failure()
@@ -234,6 +235,12 @@ class TestDhcpAgent(base.BaseTestCase):
             exc=exceptions.NetworkNotFound(net_id='1'),
             trace_level='warning')
 
+    def test_call_driver_conflict(self):
+        self._test_call_driver_failure(
+            exc=exceptions.Conflict(),
+            trace_level='warning',
+            expected_sync=False)
+
     def _test_sync_state_helper(self, known_networks, active_networks):
         with mock.patch(DHCP_PLUGIN) as plug:
             mock_plugin = mock.Mock()
@@ -859,6 +866,10 @@ class TestDhcpPluginApiProxy(base.BaseTestCase):
                                               device_id='devid',
                                               host='foo')
 
+    def test_get_dhcp_port_none(self):
+        self.call.return_value = None
+        self.assertIsNone(self.proxy.get_dhcp_port('netid', 'devid'))
+
     def test_get_active_networks_info(self):
         self.proxy.get_active_networks_info()
         self.make_msg.assert_called_once_with('get_active_networks_info',
@@ -878,6 +889,18 @@ class TestDhcpPluginApiProxy(base.BaseTestCase):
                                               port=port_body,
                                               host='foo')
 
+    def test_create_dhcp_port_none(self):
+        self.call.return_value = None
+        port_body = (
+            {'port':
+                {'name': '', 'admin_state_up': True,
+                 'network_id': fake_network.id,
+                 'tenant_id': fake_network.tenant_id,
+                 'fixed_ips': [{'subnet_id': fake_fixed_ip1.subnet_id}],
+                 'device_id': mock.ANY}})
+        self.assertIsNone(self.proxy.create_dhcp_port(port_body))
+        self.proxy.create_dhcp_port(port_body)
+
     def test_update_dhcp_port(self):
         port_body = {'port': {'fixed_ips':
                               [{'subnet_id': fake_fixed_ip1.subnet_id}]}}