From: Salvatore Orlando Date: Tue, 23 Jul 2013 21:50:07 +0000 (+0200) Subject: Remove long db transaction for metadata access network X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=2d335360866b68586ed00a7b7a725ccdbb0f9c62;p=openstack-build%2Fneutron-build.git Remove long db transaction for metadata access network Bug 1204277 Removes nested transactions wrapping plugin ops, and adds rollback code where required. Also ensures NeutronPlugin.py does not attempt to remove router ports twice. Change-Id: I299d4ed688a70b6dff506c999355661cf783ae26 --- diff --git a/neutron/plugins/nicira/NeutronPlugin.py b/neutron/plugins/nicira/NeutronPlugin.py index b5f50c3d2..b434a0493 100644 --- a/neutron/plugins/nicira/NeutronPlugin.py +++ b/neutron/plugins/nicira/NeutronPlugin.py @@ -1751,32 +1751,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 @@ -1798,12 +1776,10 @@ class NvpPluginV2(db_base_plugin_v2.NeutronDbPluginV2, self.cluster, router_id, "NoSourceNatRule", max_num_expected=1, min_num_expected=0, destination_ip_addresses=subnet['cidr']) - nvplib.delete_router_lport(self.cluster, - router_id, lrouter_port_id) 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 e54434bdf..cd1ce2959 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 bf7c6cc92..8a4d166b7 100644 --- a/neutron/tests/unit/nicira/test_nicira_plugin.py +++ b/neutron/tests/unit/nicira/test_nicira_plugin.py @@ -22,6 +22,7 @@ from oslo.config import cfg import webob.exc from neutron.common import constants +from neutron.common import exceptions as ntn_exc import neutron.common.test_lib as test_lib from neutron import context from neutron.extensions import l3 @@ -529,7 +530,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: @@ -558,6 +611,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']