x/vmware-nsx
Code Issues Proposed changes

Remove long db transaction for metadata access network

Browse Source 
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
This commit is contained in:
Salvatore Orlando 2013-07-23 23:50:07 +02:00
parent 5658b599d0
commit 2d33536086
3 changed files with 181 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
neutron/plugins/nicira/NeutronPlugin.py
View File

@ -1751,32 +1751,10 @@ class NvpPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
else: else:
raise l3.RouterInterfaceNotFoundForSubnet(router_id=router_id, raise l3.RouterInterfaceNotFoundForSubnet(router_id=router_id,
subnet_id=subnet_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 # Finally remove the data from the Neutron DB
# This will also destroy the port on the logical switch # This will also destroy the port on the logical switch
info = super(NvpPluginV2, self).remove_router_interface( info = super(NvpPluginV2, self).remove_router_interface(
context, router_id, interface_info) 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' # Ensure the connection to the 'metadata access network'
# is removed (with the network) if this the last subnet # 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", self.cluster, router_id, "NoSourceNatRule",
max_num_expected=1, min_num_expected=0, max_num_expected=1, min_num_expected=0,
destination_ip_addresses=subnet['cidr']) destination_ip_addresses=subnet['cidr'])
nvplib.delete_router_lport(self.cluster,
router_id, lrouter_port_id)
except NvpApiClient.ResourceNotFound: except NvpApiClient.ResourceNotFound:
raise nvp_exc.NvpPluginException( raise nvp_exc.NvpPluginException(
err_msg=(_("Logical router port resource %s not found " err_msg=(_("Logical router resource %s not found "
"on NVP platform"), lrouter_port_id)) "on NVP platform") % router_id))
except NvpApiClient.NvpApiException: except NvpApiClient.NvpApiException:
raise nvp_exc.NvpPluginException( raise nvp_exc.NvpPluginException(
err_msg=(_("Unable to update logical router" err_msg=(_("Unable to update logical router"

152
neutron/plugins/nicira/common/metadata_access.py
View File

@ -17,13 +17,15 @@
# #
# @author: Salvatore Orlando, VMware # @author: Salvatore Orlando, VMware
from eventlet import greenthread
import netaddr import netaddr
from oslo.config import cfg from oslo.config import cfg
from neutron.api.rpc.agentnotifiers import dhcp_rpc_agent_api from neutron.api.rpc.agentnotifiers import dhcp_rpc_agent_api
from neutron.api.v2 import attributes from neutron.api.v2 import attributes
from neutron.common import constants 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 l3_db
from neutron.db import models_v2 from neutron.db import models_v2
from neutron.openstack.common import log as logging from neutron.openstack.common import log as logging
@ -50,18 +52,21 @@ class NvpMetadataAccess(object):
return port return port
def _create_metadata_access_network(self, context, router_id): def _create_metadata_access_network(self, context, router_id):
# This will still ensure atomicity on Neutron DB # Add network
with context.session.begin(subtransactions=True): # Network name is likely to be truncated on NVP
# Add network net_data = {'name': 'meta-%s' % router_id,
# Network name is likely to be truncated on NVP 'tenant_id': '', # intentionally not set
net_data = {'name': 'meta-%s' % router_id, 'admin_state_up': True,
'tenant_id': '', # intentionally not set 'port_security_enabled': False,
'admin_state_up': True, 'shared': False,
'port_security_enabled': False, 'status': constants.NET_STATUS_ACTIVE}
'shared': False, meta_net = self.create_network(context,
'status': constants.NET_STATUS_ACTIVE} {'network': net_data})
meta_net = self.create_network(context, greenthread.sleep(0) # yield
{'network': net_data}) # From this point on there will be resources to garbage-collect
# in case of failures
meta_sub = None
try:
# Add subnet # Add subnet
subnet_data = {'network_id': meta_net['id'], subnet_data = {'network_id': meta_net['id'],
'tenant_id': '', # intentionally not set 'tenant_id': '', # intentionally not set
@ -77,34 +82,52 @@ class NvpMetadataAccess(object):
'host_routes': []} 'host_routes': []}
meta_sub = self.create_subnet(context, meta_sub = self.create_subnet(context,
{'subnet': subnet_data}) {'subnet': subnet_data})
greenthread.sleep(0) # yield
self.add_router_interface(context, router_id, self.add_router_interface(context, router_id,
{'subnet_id': meta_sub['id']}) {'subnet_id': meta_sub['id']})
if cfg.CONF.dhcp_agent_notification: greenthread.sleep(0) # yield
# We need to send a notification to the dhcp agent in except (ntn_exc.NeutronException,
# order to start the metadata agent proxy nvp_exc.NvpPluginException,
dhcp_notifier = dhcp_rpc_agent_api.DhcpAgentNotifyAPI() NvpApiClient.NvpApiException):
dhcp_notifier.notify(context, {'network': meta_net}, # It is not necessary to explicitly delete the subnet
'network.create.end') # 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): def _destroy_metadata_access_network(self, context, router_id, ports):
# This will still ensure atomicity on Neutron DB if not ports:
with context.session.begin(subtransactions=True): return
if ports: meta_port = self._find_metadata_port(context, ports)
meta_port = self._find_metadata_port(context, ports) if not meta_port:
if not meta_port: return
return meta_net_id = meta_port['network_id']
meta_net_id = meta_port['network_id'] meta_sub_id = meta_port['fixed_ips'][0]['subnet_id']
self.remove_router_interface( self.remove_router_interface(
context, router_id, {'port_id': meta_port['id']}) context, router_id, {'port_id': meta_port['id']})
# Remove network (this will remove the subnet too) greenthread.sleep(0) # yield
self.delete_network(context, meta_net_id) try:
if cfg.CONF.dhcp_agent_notification: # Remove network (this will remove the subnet too)
# We need to send a notification to the dhcp agent in self.delete_network(context, meta_net_id)
# order to stop the metadata agent proxy greenthread.sleep(0) # yield
dhcp_notifier = dhcp_rpc_agent_api.DhcpAgentNotifyAPI() except (ntn_exc.NeutronException, nvp_exc.NvpPluginException,
dhcp_notifier.notify(context, NvpApiClient.NvpApiException):
{'network': {'id': meta_net_id}}, # must re-add the router interface
'network.delete.end') 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, def _handle_metadata_access_network(self, context, router_id,
do_create=True): do_create=True):
@ -120,35 +143,32 @@ class NvpMetadataAccess(object):
ctx_elevated = context.elevated() ctx_elevated = context.elevated()
device_filter = {'device_id': [router_id], device_filter = {'device_id': [router_id],
'device_owner': [l3_db.DEVICE_OWNER_ROUTER_INTF]} 'device_owner': [l3_db.DEVICE_OWNER_ROUTER_INTF]}
with ctx_elevated.session.begin(subtransactions=True): # Retrieve ports calling database plugin
# Retrieve ports without going to plugin ports = db_base_plugin_v2.NeutronDbPluginV2.get_ports(
ports = [self._make_port_dict(port) self, context, filters=device_filter)
for port in self._get_ports_query( try:
ctx_elevated, filters=device_filter) if ports:
if port['fixed_ips']] if (do_create and
try: not self._find_metadata_port(ctx_elevated, ports)):
if ports: self._create_metadata_access_network(ctx_elevated,
if (do_create and router_id)
not self._find_metadata_port(ctx_elevated, ports)): elif len(ports) == 1:
self._create_metadata_access_network(ctx_elevated, # The only port left might be the metadata port
router_id) self._destroy_metadata_access_network(ctx_elevated,
elif len(ports) == 1: router_id,
# The only port left is the metadata port ports)
self._destroy_metadata_access_network(ctx_elevated, else:
router_id, LOG.debug(_("No router interface found for router '%s'. "
ports) "No metadata access network should be "
else: "created or destroyed"), router_id)
LOG.debug(_("No router interface found for router '%s'. " # TODO(salvatore-orlando): A better exception handling in the
"No metadata access network should be " # NVP plugin would allow us to improve error handling here
"created or destroyed"), router_id) except (ntn_exc.NeutronException, nvp_exc.NvpPluginException,
# TODO(salvatore-orlando): A better exception handling in the NvpApiClient.NvpApiException):
# NVP plugin would allow us to improve error handling here # Any exception here should be regarded as non-fatal
except (q_exc.NeutronException, nvp_exc.NvpPluginException, LOG.exception(_("An error occurred while operating on the "
NvpApiClient.NvpApiException): "metadata access network for router:'%s'"),
# Any exception here should be regarded as non-fatal router_id)
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, def _ensure_metadata_host_route(self, context, fixed_ip_data,
is_delete=False): is_delete=False):

94
neutron/tests/unit/nicira/test_nicira_plugin.py
View File

@ -22,6 +22,7 @@ from oslo.config import cfg
import webob.exc import webob.exc
from neutron.common import constants from neutron.common import constants
from neutron.common import exceptions as ntn_exc
import neutron.common.test_lib as test_lib import neutron.common.test_lib as test_lib
from neutron import context from neutron import context
from neutron.extensions import l3 from neutron.extensions import l3
@ -529,7 +530,59 @@ class TestNiciraL3NatTestCase(test_l3_plugin.L3NatDBTestCase,
None) None)
self._nvp_metadata_teardown() 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() self._nvp_metadata_setup()
with self.router() as r: with self.router() as r:
with self.subnet() as s: with self.subnet() as s:
@ -558,6 +611,45 @@ class TestNiciraL3NatTestCase(test_l3_plugin.L3NatDBTestCase,
webob.exc.HTTPNotFound.code) webob.exc.HTTPNotFound.code)
self._nvp_metadata_teardown() 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): def test_metadata_dhcp_host_route(self):
cfg.CONF.set_override('metadata_mode', 'dhcp_host_route', 'NVP') cfg.CONF.set_override('metadata_mode', 'dhcp_host_route', 'NVP')
subnets = self._list('subnets')['subnets'] subnets = self._list('subnets')['subnets']