diff --git a/neutron/plugins/nicira/NeutronPlugin.py b/neutron/plugins/nicira/NeutronPlugin.py index f3ef16b64f..30862c8175 100644 --- a/neutron/plugins/nicira/NeutronPlugin.py +++ b/neutron/plugins/nicira/NeutronPlugin.py @@ -1801,32 +1801,10 @@ class NvpPluginV2(db_base_plugin_v2.NeutronDbPluginV2, else: raise l3.RouterInterfaceNotFoundForSubnet(router_id=router_id, subnet_id=subnet_id) - results = nvplib.query_lswitch_lports( - self.cluster, '*', relations="LogicalPortAttachment", - filters={'tag': port_id, 'tag_scope': 'q_port_id'}) - lrouter_port_id = None - if results: - lport = results[0] - attachment_data = lport['_relations'].get('LogicalPortAttachment') - lrouter_port_id = (attachment_data and - attachment_data.get('peer_port_uuid')) - else: - LOG.warning(_("The port %(port_id)s, connected to the router " - "%(router_id)s was not found on the NVP backend"), - {'port_id': port_id, 'router_id': router_id}) # Finally remove the data from the Neutron DB # This will also destroy the port on the logical switch info = super(NvpPluginV2, self).remove_router_interface( context, router_id, interface_info) - # Destroy router port (no need to unplug the attachment) - # FIXME(salvatore-orlando): In case of failures in the Neutron plugin - # this migth leave a dangling port. We perform the operation here - # to leverage validation performed in the base class - if not lrouter_port_id: - LOG.warning(_("Unable to find NVP logical router port for " - "Neutron port id:%s. Was this port ever paired " - "with a logical router?"), port_id) - return info # Ensure the connection to the 'metadata access network' # is removed (with the network) if this the last subnet @@ -1850,8 +1828,8 @@ class NvpPluginV2(db_base_plugin_v2.NeutronDbPluginV2, destination_ip_addresses=subnet['cidr']) except NvpApiClient.ResourceNotFound: raise nvp_exc.NvpPluginException( - err_msg=(_("Logical router port resource %s not found " - "on NVP platform"), lrouter_port_id)) + err_msg=(_("Logical router resource %s not found " + "on NVP platform") % router_id)) except NvpApiClient.NvpApiException: raise nvp_exc.NvpPluginException( err_msg=(_("Unable to update logical router" diff --git a/neutron/plugins/nicira/common/metadata_access.py b/neutron/plugins/nicira/common/metadata_access.py index bd5a0191b6..8bcb9ed3a8 100644 --- a/neutron/plugins/nicira/common/metadata_access.py +++ b/neutron/plugins/nicira/common/metadata_access.py @@ -17,13 +17,15 @@ # # @author: Salvatore Orlando, VMware +from eventlet import greenthread import netaddr from oslo.config import cfg from neutron.api.rpc.agentnotifiers import dhcp_rpc_agent_api from neutron.api.v2 import attributes from neutron.common import constants -from neutron.common import exceptions as q_exc +from neutron.common import exceptions as ntn_exc +from neutron.db import db_base_plugin_v2 from neutron.db import l3_db from neutron.db import models_v2 from neutron.openstack.common import log as logging @@ -50,18 +52,21 @@ class NvpMetadataAccess(object): return port def _create_metadata_access_network(self, context, router_id): - # This will still ensure atomicity on Neutron DB - with context.session.begin(subtransactions=True): - # Add network - # Network name is likely to be truncated on NVP - net_data = {'name': 'meta-%s' % router_id, - 'tenant_id': '', # intentionally not set - 'admin_state_up': True, - 'port_security_enabled': False, - 'shared': False, - 'status': constants.NET_STATUS_ACTIVE} - meta_net = self.create_network(context, - {'network': net_data}) + # Add network + # Network name is likely to be truncated on NVP + net_data = {'name': 'meta-%s' % router_id, + 'tenant_id': '', # intentionally not set + 'admin_state_up': True, + 'port_security_enabled': False, + 'shared': False, + 'status': constants.NET_STATUS_ACTIVE} + meta_net = self.create_network(context, + {'network': net_data}) + greenthread.sleep(0) # yield + # From this point on there will be resources to garbage-collect + # in case of failures + meta_sub = None + try: # Add subnet subnet_data = {'network_id': meta_net['id'], 'tenant_id': '', # intentionally not set @@ -77,34 +82,52 @@ class NvpMetadataAccess(object): 'host_routes': []} meta_sub = self.create_subnet(context, {'subnet': subnet_data}) + greenthread.sleep(0) # yield self.add_router_interface(context, router_id, {'subnet_id': meta_sub['id']}) - if cfg.CONF.dhcp_agent_notification: - # We need to send a notification to the dhcp agent in - # order to start the metadata agent proxy - dhcp_notifier = dhcp_rpc_agent_api.DhcpAgentNotifyAPI() - dhcp_notifier.notify(context, {'network': meta_net}, - 'network.create.end') + greenthread.sleep(0) # yield + except (ntn_exc.NeutronException, + nvp_exc.NvpPluginException, + NvpApiClient.NvpApiException): + # It is not necessary to explicitly delete the subnet + # as it will be removed with the network + self.delete_network(context, meta_net['id']) + + if cfg.CONF.dhcp_agent_notification: + # We need to send a notification to the dhcp agent in + # order to start the metadata agent proxy + dhcp_notifier = dhcp_rpc_agent_api.DhcpAgentNotifyAPI() + dhcp_notifier.notify(context, {'network': meta_net}, + 'network.create.end') def _destroy_metadata_access_network(self, context, router_id, ports): - # This will still ensure atomicity on Neutron DB - with context.session.begin(subtransactions=True): - if ports: - meta_port = self._find_metadata_port(context, ports) - if not meta_port: - return - meta_net_id = meta_port['network_id'] - self.remove_router_interface( - context, router_id, {'port_id': meta_port['id']}) - # Remove network (this will remove the subnet too) - self.delete_network(context, meta_net_id) - if cfg.CONF.dhcp_agent_notification: - # We need to send a notification to the dhcp agent in - # order to stop the metadata agent proxy - dhcp_notifier = dhcp_rpc_agent_api.DhcpAgentNotifyAPI() - dhcp_notifier.notify(context, - {'network': {'id': meta_net_id}}, - 'network.delete.end') + if not ports: + return + meta_port = self._find_metadata_port(context, ports) + if not meta_port: + return + meta_net_id = meta_port['network_id'] + meta_sub_id = meta_port['fixed_ips'][0]['subnet_id'] + self.remove_router_interface( + context, router_id, {'port_id': meta_port['id']}) + greenthread.sleep(0) # yield + try: + # Remove network (this will remove the subnet too) + self.delete_network(context, meta_net_id) + greenthread.sleep(0) # yield + except (ntn_exc.NeutronException, nvp_exc.NvpPluginException, + NvpApiClient.NvpApiException): + # must re-add the router interface + self.add_router_interface(context, router_id, + {'subnet_id': meta_sub_id}) + + if cfg.CONF.dhcp_agent_notification: + # We need to send a notification to the dhcp agent in + # order to stop the metadata agent proxy + dhcp_notifier = dhcp_rpc_agent_api.DhcpAgentNotifyAPI() + dhcp_notifier.notify(context, + {'network': {'id': meta_net_id}}, + 'network.delete.end') def _handle_metadata_access_network(self, context, router_id, do_create=True): @@ -120,35 +143,32 @@ class NvpMetadataAccess(object): ctx_elevated = context.elevated() device_filter = {'device_id': [router_id], 'device_owner': [l3_db.DEVICE_OWNER_ROUTER_INTF]} - with ctx_elevated.session.begin(subtransactions=True): - # Retrieve ports without going to plugin - ports = [self._make_port_dict(port) - for port in self._get_ports_query( - ctx_elevated, filters=device_filter) - if port['fixed_ips']] - try: - if ports: - if (do_create and - not self._find_metadata_port(ctx_elevated, ports)): - self._create_metadata_access_network(ctx_elevated, - router_id) - elif len(ports) == 1: - # The only port left is the metadata port - self._destroy_metadata_access_network(ctx_elevated, - router_id, - ports) - else: - LOG.debug(_("No router interface found for router '%s'. " - "No metadata access network should be " - "created or destroyed"), router_id) - # TODO(salvatore-orlando): A better exception handling in the - # NVP plugin would allow us to improve error handling here - except (q_exc.NeutronException, nvp_exc.NvpPluginException, - NvpApiClient.NvpApiException): - # Any exception here should be regarded as non-fatal - LOG.exception(_("An error occurred while operating on the " - "metadata access network for router:'%s'"), - router_id) + # Retrieve ports calling database plugin + ports = db_base_plugin_v2.NeutronDbPluginV2.get_ports( + self, context, filters=device_filter) + try: + if ports: + if (do_create and + not self._find_metadata_port(ctx_elevated, ports)): + self._create_metadata_access_network(ctx_elevated, + router_id) + elif len(ports) == 1: + # The only port left might be the metadata port + self._destroy_metadata_access_network(ctx_elevated, + router_id, + ports) + else: + LOG.debug(_("No router interface found for router '%s'. " + "No metadata access network should be " + "created or destroyed"), router_id) + # TODO(salvatore-orlando): A better exception handling in the + # NVP plugin would allow us to improve error handling here + except (ntn_exc.NeutronException, nvp_exc.NvpPluginException, + NvpApiClient.NvpApiException): + # Any exception here should be regarded as non-fatal + LOG.exception(_("An error occurred while operating on the " + "metadata access network for router:'%s'"), + router_id) def _ensure_metadata_host_route(self, context, fixed_ip_data, is_delete=False): diff --git a/neutron/tests/unit/nicira/test_nicira_plugin.py b/neutron/tests/unit/nicira/test_nicira_plugin.py index 8eeb72bd26..81fae9fe4c 100644 --- a/neutron/tests/unit/nicira/test_nicira_plugin.py +++ b/neutron/tests/unit/nicira/test_nicira_plugin.py @@ -553,7 +553,59 @@ class TestNiciraL3NatTestCase(test_l3_plugin.L3NatDBTestCase, None) self._nvp_metadata_teardown() - def test_metadatata_network_removed_with_router_interface_remove(self): + def test_metadata_network_create_rollback_on_create_subnet_failure(self): + self._nvp_metadata_setup() + with self.router() as r: + with self.subnet() as s: + # Raise a NeutronException (eg: NotFound) + with mock.patch.object(NeutronPlugin.NvpPluginV2, + 'create_subnet', + side_effect=ntn_exc.NotFound): + self._router_interface_action( + 'add', r['router']['id'], s['subnet']['id'], None) + # Ensure metadata network was removed + nets = self._list('networks')['networks'] + self.assertEqual(len(nets), 1) + # Needed to avoid 409 + self._router_interface_action('remove', + r['router']['id'], + s['subnet']['id'], + None) + self._nvp_metadata_teardown() + + def test_metadata_network_create_rollback_on_add_rtr_iface_failure(self): + self._nvp_metadata_setup() + with self.router() as r: + with self.subnet() as s: + # Raise a NeutronException when adding metadata subnet + # to router + # save function being mocked + real_func = NeutronPlugin.NvpPluginV2.add_router_interface + plugin_instance = manager.NeutronManager.get_plugin() + + def side_effect(*args): + if args[-1]['subnet_id'] == s['subnet']['id']: + # do the real thing + return real_func(plugin_instance, *args) + # otherwise raise + raise NvpApiClient.NvpApiException() + + with mock.patch.object(NeutronPlugin.NvpPluginV2, + 'add_router_interface', + side_effect=side_effect): + self._router_interface_action( + 'add', r['router']['id'], s['subnet']['id'], None) + # Ensure metadata network was removed + nets = self._list('networks')['networks'] + self.assertEqual(len(nets), 1) + # Needed to avoid 409 + self._router_interface_action('remove', + r['router']['id'], + s['subnet']['id'], + None) + self._nvp_metadata_teardown() + + def test_metadata_network_removed_with_router_interface_remove(self): self._nvp_metadata_setup() with self.router() as r: with self.subnet() as s: @@ -582,6 +634,45 @@ class TestNiciraL3NatTestCase(test_l3_plugin.L3NatDBTestCase, webob.exc.HTTPNotFound.code) self._nvp_metadata_teardown() + def test_metadata_network_remove_rollback_on_failure(self): + self._nvp_metadata_setup() + with self.router() as r: + with self.subnet() as s: + self._router_interface_action('add', r['router']['id'], + s['subnet']['id'], None) + networks = self._list('networks')['networks'] + for network in networks: + if network['id'] != s['subnet']['network_id']: + meta_net_id = network['id'] + ports = self._list( + 'ports', + query_params='network_id=%s' % meta_net_id)['ports'] + meta_port_id = ports[0]['id'] + # Raise a NeutronException when removing + # metadata subnet from router + # save function being mocked + real_func = NeutronPlugin.NvpPluginV2.remove_router_interface + plugin_instance = manager.NeutronManager.get_plugin() + + def side_effect(*args): + if args[-1].get('subnet_id') == s['subnet']['id']: + # do the real thing + return real_func(plugin_instance, *args) + # otherwise raise + raise NvpApiClient.NvpApiException() + + with mock.patch.object(NeutronPlugin.NvpPluginV2, + 'remove_router_interface', + side_effect=side_effect): + self._router_interface_action('remove', r['router']['id'], + s['subnet']['id'], None) + # Metadata network and subnet should still be there + self._show('networks', meta_net_id, + webob.exc.HTTPOk.code) + self._show('ports', meta_port_id, + webob.exc.HTTPOk.code) + self._nvp_metadata_teardown() + def test_metadata_dhcp_host_route(self): cfg.CONF.set_override('metadata_mode', 'dhcp_host_route', 'NVP') subnets = self._list('subnets')['subnets']