From e770c868f87f7053b8e282bcdbd2ce1a00a34b7f Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Thu, 12 May 2016 09:17:56 -0700 Subject: [PATCH] Ensure most of ML2's core methods not in transaction 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 --- doc/source/devref/calling_ml2_plugin.rst | 47 +++++++++++++++++++ doc/source/devref/index.rst | 1 + neutron/plugins/ml2/plugin.py | 11 +++++ neutron/tests/unit/plugins/ml2/test_plugin.py | 13 ++--- 4 files changed, 64 insertions(+), 8 deletions(-) create mode 100644 doc/source/devref/calling_ml2_plugin.rst diff --git a/doc/source/devref/calling_ml2_plugin.rst b/doc/source/devref/calling_ml2_plugin.rst new file mode 100644 index 00000000000..6cae36362bf --- /dev/null +++ b/doc/source/devref/calling_ml2_plugin.rst @@ -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. diff --git a/doc/source/devref/index.rst b/doc/source/devref/index.rst index 72581ce3215..bbb84d5ab73 100644 --- a/doc/source/devref/index.rst +++ b/doc/source/devref/index.rst @@ -55,6 +55,7 @@ Neutron Internals services_and_agents api_layer ml2_ext_manager + calling_ml2_plugin quota api_extensions plugin-api diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index 01348e24a75..2c616b62a5f 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -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): """ diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index 5012218f478..f833cbd618d 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -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()