From: armando-migliaccio Date: Fri, 22 Nov 2013 02:42:08 +0000 (-0800) Subject: Handle exceptions on create_dhcp_port X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=bff120a4775a1b1f3846a27c38d8eff4a678fd24;p=openstack-build%2Fneutron-build.git 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 --- diff --git a/neutron/agent/dhcp_agent.py b/neutron/agent/dhcp_agent.py index 2cbd14949..db744ba21 100644 --- a/neutron/agent/dhcp_agent.py +++ b/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, - self.make_msg('create_dhcp_port', - port=port, - host=self.host), - topic=self.topic)) + port = self.call(self.context, + self.make_msg('create_dhcp_port', + port=port, + host=self.host), + 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.""" diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index 6c4f09477..de426ffc8 100644 --- a/neutron/agent/linux/dhcp.py +++ b/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, diff --git a/neutron/db/dhcp_rpc_base.py b/neutron/db/dhcp_rpc_base.py index 93fe58a5a..dced81a29 100644 --- a/neutron/db/dhcp_rpc_base.py +++ b/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 = plugin.create_port(context, dict(port=port_dict)) - except (db_exc.DBError, - 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}) + retval = self._port_action(plugin, context, {'port': port_dict}, + 'create_port') + if not retval: 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.""" diff --git a/neutron/tests/unit/test_db_rpc_base.py b/neutron/tests/unit/test_db_rpc_base.py index 617169c56..970cb4c54 100644 --- a/neutron/tests/unit/test_db_rpc_base.py +++ b/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', diff --git a/neutron/tests/unit/test_dhcp_agent.py b/neutron/tests/unit/test_dhcp_agent.py index cd7d0a2aa..ae4d5a3b7 100644 --- a/neutron/tests/unit/test_dhcp_agent.py +++ b/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}]}}