From 92b95815318806f90eca987145ad453782dd7041 Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Thu, 15 Mar 2018 14:25:04 -0700 Subject: [PATCH] objects: automatically detect whether engine facade is used While we added new_facade object attribute to framework and it has its niche and used in some stadium subprojects, it's not ideal because it's global to an object. Meaning that if you mark an object for new_facade = True, *all* business logic using the object must also switch to new facade in the same step, which is a pain and sometimes close to impossible to do without changing thousands loosely related lines of code in multiple modules. It would be nice to instead use objects as usual in different contexts - some using engine facade and some still using session.begin(...) - and allow the OVO framework to pick the right way to nest subtransactions. This patch does exactly that. We call an internal function from oslo.db and check whether it raises an exception. If it does, it means that the engine facade is not used; otherwise, we use the new style of nested transactions management. By default, if session is not active when OVO action is called, we stick to the old facade. Once we are done with switching the rest of the plugin code / OVO objects to the new facade, we will rip off the transitionary logic. Change-Id: Ia05df925eccf3c9d397748f282e995203a058de9 Partially-Implements: blueprint enginefacade-switch Partially-Implements: blueprint adopt-oslo-versioned-objects-for-db --- .../contributor/internals/objects_usage.rst | 4 +++ neutron/objects/base.py | 14 ++++++-- neutron/tests/unit/objects/test_base.py | 33 +++++++++++++++---- neutron/tests/unit/plugins/ml2/test_plugin.py | 2 +- 4 files changed, 43 insertions(+), 10 deletions(-) diff --git a/doc/source/contributor/internals/objects_usage.rst b/doc/source/contributor/internals/objects_usage.rst index f55357739f1..62798d517bd 100644 --- a/doc/source/contributor/internals/objects_usage.rst +++ b/doc/source/contributor/internals/objects_usage.rst @@ -360,6 +360,10 @@ following database session decorators: ``db_context_reader`` and ``db_context_writer`` decorators abstract the choice of engine facade used for particular object from action implementation. +Alternatively, you can call all OVO actions under an active ``reader.using`` / +``writer.using`` context manager (or ``session.begin``). In this case, OVO will +pick the appropriate method to open a subtransaction. + Synthetic fields ---------------- :code:`synthetic_fields` is a list of fields, that are not directly backed by diff --git a/neutron/objects/base.py b/neutron/objects/base.py index b9c6ae2223c..4a4239211a5 100644 --- a/neutron/objects/base.py +++ b/neutron/objects/base.py @@ -19,6 +19,7 @@ import itertools from neutron_lib import exceptions as n_exc from neutron_lib.objects import exceptions as o_exc from oslo_db import exception as obj_exc +from oslo_db.sqlalchemy import enginefacade from oslo_db.sqlalchemy import utils as db_utils from oslo_log import log as logging from oslo_serialization import jsonutils @@ -505,17 +506,26 @@ class NeutronDbObject(NeutronObject): if is_attr_nullable: self[attrname] = None + # TODO(ihrachys) remove once we switch plugin code to enginefacade + @staticmethod + def _use_db_facade(context): + try: + enginefacade._transaction_ctx_for_context(context) + except obj_exc.NoEngineContextEstablished: + return False + return True + @classmethod def db_context_writer(cls, context): """Return read-write session activation decorator.""" - if cls.new_facade: + if cls.new_facade or cls._use_db_facade(context): return db_api.context_manager.writer.using(context) return db_api.autonested_transaction(context.session) @classmethod def db_context_reader(cls, context): """Return read-only session activation decorator.""" - if cls.new_facade: + if cls.new_facade or cls._use_db_facade(context): return db_api.context_manager.reader.using(context) return db_api.autonested_transaction(context.session) diff --git a/neutron/tests/unit/objects/test_base.py b/neutron/tests/unit/objects/test_base.py index f76f9bcb6a4..cbd06939ae7 100644 --- a/neutron/tests/unit/objects/test_base.py +++ b/neutron/tests/unit/objects/test_base.py @@ -32,6 +32,7 @@ from oslo_versionedobjects import fields as obj_fields import testtools from neutron.db import _model_query as model_query +from neutron.db import api as db_api from neutron import objects from neutron.objects import agent from neutron.objects import base @@ -1699,16 +1700,23 @@ class BaseDbObjectTestCase(_BaseObjectTestCase, self.assertEqual(1, mock_commit.call_count) def _get_ro_txn_exit_func_name(self): - # for old engine facade, we didn't have distinction between r/o and r/w - # transactions and so we always call commit even for getters when the - # old facade is used + # with no engine facade, we didn't have distinction between r/o and + # r/w transactions and so we always call commit even for getters when + # no facade is used return ( SQLALCHEMY_CLOSE - if self._test_class.new_facade else SQLALCHEMY_COMMIT) + if self._test_class._use_db_facade else SQLALCHEMY_COMMIT) def test_get_objects_single_transaction(self): with mock.patch(self._get_ro_txn_exit_func_name()) as mock_exit: - self._test_class.get_objects(self.context) + with db_api.autonested_transaction(self.context.session): + self._test_class.get_objects(self.context) + self.assertEqual(1, mock_exit.call_count) + + def test_get_objects_single_transaction_enginefacade(self): + with mock.patch(self._get_ro_txn_exit_func_name()) as mock_exit: + with db_api.context_manager.reader.using(self.context): + self._test_class.get_objects(self.context) self.assertEqual(1, mock_exit.call_count) def test_get_object_single_transaction(self): @@ -1716,8 +1724,19 @@ class BaseDbObjectTestCase(_BaseObjectTestCase, obj.create() with mock.patch(self._get_ro_txn_exit_func_name()) as mock_exit: - obj = self._test_class.get_object(self.context, - **obj._get_composite_keys()) + with db_api.autonested_transaction(self.context.session): + obj = self._test_class.get_object(self.context, + **obj._get_composite_keys()) + self.assertEqual(1, mock_exit.call_count) + + def test_get_object_single_transaction_enginefacade(self): + obj = self._make_object(self.obj_fields[0]) + obj.create() + + with mock.patch(self._get_ro_txn_exit_func_name()) as mock_exit: + with db_api.context_manager.reader.using(self.context): + obj = self._test_class.get_object(self.context, + **obj._get_composite_keys()) self.assertEqual(1, mock_exit.call_count) def test_get_objects_supports_extra_filtername(self): diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index 5bd47f9339e..f67fe589ee3 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -1685,7 +1685,7 @@ class TestMl2DvrPortsV2(TestMl2PortsV2): {'subnet_id': s['subnet']['id']}) # lie to turn the port into an SNAT interface - with db_api.context_manager.reader.using(self.context): + with db_api.context_manager.writer.using(self.context): pager = base_obj.Pager(limit=1) rp = l3_obj.RouterPort.get_objects( self.context, _pager=pager, port_id=p['port_id'])