openstack/neutron
Code Issues Proposed changes

Ensure most of ML2's core methods not in transaction

Browse Source 
This adds a check to each of the core methods that other callers
(e.g. service plugins) may use to manipulate core resources. This
check prevents them from passing in a context that is already part
of an ongoing DB session because we do not want DB rollbacks to be
allowed after the ML2 plugin calls postcommit methods on drivers.

All of the core methods are protected except for create_port and
update_port. This was left out because of a few particularily deeply
nested calls to the port methods from the L3 code that will be
addressed in change I5aa099c2264636336ab0b76c0826b506e2dc44b6.

For more details, read the devref added by this patch.

Partial-Bug: #1540844
Change-Id: I9924600c57648f7eccaa5abb6979419d9547a2ff
This commit is contained in:
Kevin Benton 2016-05-12 09:17:56 -07:00
parent f2c481ee3c
commit e770c868f8
4 changed files with 64 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
doc/source/devref/calling_ml2_plugin.rst Normal file
View File

@ -0,0 +1,47 @@
..
Licensed under the Apache License, Version 2.0 (the "License"); you may
not use this file except in compliance with the License. You may obtain
a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
License for the specific language governing permissions and limitations
under the License.
Convention for heading levels in Neutron devref:
======= Heading 0 (reserved for the title in a document)
------- Heading 1
~~~~~~~ Heading 2
+++++++ Heading 3
''''''' Heading 4
(Avoid deeper levels because they do not render well.)
Calling the ML2 Plugin
======================
When writing code for an extension, service plugin, or any other part of
Neutron you must not call core plugin methods that mutate state while
you have a transaction open on the session that you pass into the core
plugin method.
The create and update methods for ports, networks, and subnets in ML2
all have a precommit phase and postcommit phase. During the postcommit
phase, the data is expected to be fully persisted to the database and
ML2 drivers will use this time to relay information to a backend outside
of Neutron. Calling the ML2 plugin within a transaction would violate
this semantic because the data would not be persisted to the DB; and,
were a failure to occur that caused the whole transaction to be rolled
back, the backend would become inconsistent with the state in Neutron's
DB.
To prevent this, these methods are protected with a decorator that will
raise a RuntimeError if they are called with context that has a session
in an active transaction. The decorator can be found at
neutron.common.utils.transaction_guard and may be used in other places
in Neutron to protect functions that are expected to be called outside
of a transaction.

1
doc/source/devref/index.rst
View File

@ -55,6 +55,7 @@ Neutron Internals
services_and_agents
api_layer
ml2_ext_manager
calling_ml2_plugin
quota
api_extensions
plugin-api

11
neutron/plugins/ml2/plugin.py
View File

@ -757,6 +757,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
self._apply_dict_extend_functions('networks', result, net_db)
return result, mech_context
@utils.transaction_guard
def create_network(self, context, network):
result, mech_context = self._create_network_db(context, network)
kwargs = {'context': context, 'network': result}
@ -771,10 +772,12 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
return result
@utils.transaction_guard
def create_network_bulk(self, context, networks):
objects = self._create_bulk_ml2(attributes.NETWORK, context, networks)
return [obj['result'] for obj in objects]
@utils.transaction_guard
def update_network(self, context, id, network):
net_data = network[attributes.NETWORK]
provider._raise_if_updates_provider_attributes(net_data)
@ -966,6 +969,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
return result, mech_context
@utils.transaction_guard
def create_subnet(self, context, subnet):
result, mech_context = self._create_subnet_db(context, subnet)
kwargs = {'context': context, 'subnet': result}
@ -979,10 +983,12 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
self.delete_subnet(context, result['id'])
return result
@utils.transaction_guard
def create_subnet_bulk(self, context, subnets):
objects = self._create_bulk_ml2(attributes.SUBNET, context, subnets)
return [obj['result'] for obj in objects]
@utils.transaction_guard
def update_subnet(self, context, id, subnet):
session = context.session
with session.begin(subtransactions=True):
@ -1256,6 +1262,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
return bound_context.current
@utils.transaction_guard
def create_port_bulk(self, context, ports):
objects = self._create_bulk_ml2(attributes.PORT, context, ports)
@ -1462,6 +1469,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
binding.host = attrs and attrs.get(portbindings.HOST_ID)
binding.router_id = attrs and attrs.get('device_id')
@utils.transaction_guard
def update_distributed_port_binding(self, context, id, port):
attrs = port[attributes.PORT]
@ -1518,6 +1526,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
raise e.errors[0].error
raise exc.ServicePortInUse(port_id=port_id, reason=e)
@utils.transaction_guard
def delete_port(self, context, id, l3_port_check=True):
self._pre_delete_port(context, id, l3_port_check)
# TODO(armax): get rid of the l3 dependency in the with block
@ -1587,6 +1596,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
self.notifier.port_delete(context, port['id'])
self.notify_security_groups_member_updated(context, port)
@utils.transaction_guard
def get_bound_port_context(self, plugin_context, port_id, host=None,
cached_networks=None):
session = plugin_context.session
@ -1643,6 +1653,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
exception_checker=lambda e: isinstance(e, (sa_exc.StaleDataError,
os_db_exception.DBDeadlock))
)
@utils.transaction_guard
def update_port_status(self, context, port_id, status, host=None,
network=None):
"""

13
neutron/tests/unit/plugins/ml2/test_plugin.py
View File

@ -1565,20 +1565,16 @@ class TestMl2PortBinding(Ml2PluginV2TestCase,
def test_update_distributed_port_binding_on_concurrent_port_delete(self):
plugin = manager.NeutronManager.get_plugin()
l3plugin = manager.NeutronManager.get_service_plugins().get(
p_const.L3_ROUTER_NAT)
with self.port() as port:
port = {
'id': port['port']['id'],
portbindings.HOST_ID: 'foo_host',
}
# NOTE(rtheis): Mock L3 service prevent_l3_port_deletion() to
# prevent recursive loop introduced by the plugin get_port mock.
with mock.patch.object(plugin, 'get_port',
new=plugin.delete_port), \
mock.patch.object(l3plugin, 'prevent_l3_port_deletion'):
exc = db_exc.DBReferenceError('', '', '', '')
with mock.patch.object(ml2_db, 'ensure_distributed_port_binding',
side_effect=exc):
res = plugin.update_distributed_port_binding(
self.context, 'foo_port_id', {'port': port})
self.context, port['id'], {'port': port})
self.assertIsNone(res)
def test_update_distributed_port_binding_on_non_existent_port(self):
@ -2316,6 +2312,7 @@ class TestMl2PluginCreateUpdateDeletePort(base.BaseTestCase):
# neutron.objects.db.api from core plugin instance
self.setup_coreplugin(PLUGIN_NAME)
self.context = mock.MagicMock()
self.context.session.is_active = False
self.notify_p = mock.patch('neutron.callbacks.registry.notify')
self.notify = self.notify_p.start()