From ec52f4370bffab5bb93bf90312e2227f415e79ed Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Tue, 30 Sep 2014 16:11:44 -0400 Subject: [PATCH] db: Make EngineFacade a Facade This blueprint proposes a new declarative system of connection management and session and transaction scoping for all projects that currently make use of oslo.db.session.EngineFacade. Change-Id: I8c0efa3b859c2fe5f06f4c0416c5323362d7ef72 --- specs/kilo/make-enginefacade-a-facade.rst | 767 ++++++++++++++++++++++ 1 file changed, 767 insertions(+) create mode 100644 specs/kilo/make-enginefacade-a-facade.rst diff --git a/specs/kilo/make-enginefacade-a-facade.rst b/specs/kilo/make-enginefacade-a-facade.rst new file mode 100644 index 0000000..089f26b --- /dev/null +++ b/specs/kilo/make-enginefacade-a-facade.rst @@ -0,0 +1,767 @@ +================================ +Make EngineFacade into a Facade +================================ + +https://blueprints.launchpad.net/oslo.db/+spec/make-enginefacade-a-facade + +Problem description +=================== + +The oslo.db.sqlalchemy.session.EngineFacade class serves as the gateway +to the SQLAlchemy Engine and Session objects within many OpenStack projects, +including Ceilometer, Glance, Heat, Ironic, Keystone, Neutron, Nova, and +Sahara. However, the object is severely under-functional; while it provides +a function call that ultimately calls ``create_engine()`` and +``sessionmaker()``, consuming projects receive no other utility from this +object, and in order to solve closely related problems that all of them +share, each invent their own systems, all of which are different, +verbose, and error prone, with various performance, stability, +and scalability issues. + +Registry Functionality +---------------------- + +In the first case, EngineFacade as used by projects needs to act as a +thread-safe registry, a feature which it does not provide and for +which each consuming project has had to invent directly. These +inventions are verbose and inconsistent. + +For example, in Keystone, the EngineFacade is created thusly in +keystone/common/sql/core.py:: + + _engine_facade = None + + def _get_engine_facade(): + global _engine_facade + + if not _engine_facade: + _engine_facade = db_session.EngineFacade.from_config(CONF) + + return _engine_facade + +In Ironic we have this; Sahara contains something similar:: + + _FACADE = None + + def _create_facade_lazily(): + global _FACADE + if _FACADE is None: + _FACADE = db_session.EngineFacade( + CONF.database.connection, + **dict(CONF.database.iteritems()) + ) + return _FACADE + +However in Nova, we get a similar pattern but with one very critical twist:: + + _ENGINE_FACADE = None + _LOCK = threading.Lock() + + def _create_facade_lazily(): + global _LOCK, _ENGINE_FACADE + if _ENGINE_FACADE is None: + with _LOCK: + if _ENGINE_FACADE is None: + _ENGINE_FACADE = db_session.EngineFacade.from_config(CONF) + return _ENGINE_FACADE + +Each library invents their own system of establishing EngineFacade as a +singleton, and providing CONF into it; none of which are alike. +Nova happened to discover that this singleton pattern isn't threadsafe, +and added a mutex, however the lack of this critical improvement remains a +bug in all the other systems. + +Transactional Resource Functionality +------------------------------------- + +Adding a fully functional creational pattern is an easy win, +but the problem goes beyond that. EngineFacade ends its work at ``get_engine()`` +or ``get_session()``; the former returns a SQLAlchemy Engine object, which +itself is only a factory for connections, and the latter returns a +SQLAlchemy Session object, ready for use but otherwise unassociated with +any specific connection or transactional context. + +The definition of "facade" is a layer that conceals the use of fine- +grained APIs behind a layer that is coarse-grained and tailored to the +use case at hand. By this definition, the EngineFacade currently is +only a factory, and not a facade. + +The harm caused by this lack of guidance on EngineFacade's part is widespread. +While the failure to provide an adequate creational pattern leads +each Openstack project to invent its own workaround, +the failure to provide any guidance on connectivity or transactional +scope gives rise to a much more significant pattern of poor implementations +on the part of all Openstack projects. Each project observed illustrates +mis-use of engines, sessions and transactions to a greater or lesser degree, +more often than not having direct consequences for performance, stability, +and maintainability. + +The general theme among all projects is that while they are all +presented as web services, there is no structure in place which +establishes connectivity and transactional scope for a service method +as a whole. Individual methods include explicit boilerplate which +establishes some kind of connectivity, either within a transaction or +not. The format of this boilerplate alone is not only inconsistent +between projects, it's inconsistent within a single project and even +in a single module, sometimes intentionally and sometimes not. +Equally if not more seriously, individual API methods very frequently +proceed across a span of multiple connections and non-connected +transactions within the scope of a single operation, and in some cases +the multiple transactions are even nested. The use of multiple +connections and transactions is in the first place a major performance +impediment, and in the second place dilutes the usefulness of +transactional logic in the first place as an API method is not +actually atomic. When transactions are actually nested, the risk surface +for deadlocks increases significantly. + +Transactional Scoping Examples +------------------------------- + +This section will detail some specific examples of the issues just described, +for those who are curious. + +We first show Neutron's system, which is the most +organized and probably has the fewest issues of this nature. +Neutron has a system where all database operations proceed +where a neutron.context.Context object is passed; the Context object serves +as home base to a SQLAlchemy Session that was ultimately retrieved +from EngineFacade. A method excerpt looks like this:: + + def add_resource_association(self, context, service_type, provider_name, + resource_id): + + # ... + + with context.session.begin(subtransactions=True): + assoc = ProviderResourceAssociation(provider_name=provider_name, + resource_id=resource_id) + context.session.add(assoc) + +We see that while the Context object at least allows that all operations +are given access to the same Session, the method still has to state +that it wishes to begin a transaction, and that it needs to support the +fact that the Session may already be within a transaction. Neutron's system +is a little verbose, and suffers from the issue that individual methods called +in series may invoke their work within distinct transactions on new +connections each time, but at least ensures that just one Session is in +play for a given API method from start to finish; this prevents the issue +of inadvertent multiple transaction nesting, as the Session's ``begin()`` +method will disallow a nested call from opening a new connection. + +Next we look at Keystone. Keystone has some database-related helper functions +but they don't serve any functional purpose other than some naming abstraction. +Keystone has a lot of short "lookup" methods, so many of them +look like this:: + + @sql.handle_conflicts(conflict_type='trust') + def list_trusts(self): + session = sql.get_session() + trusts = session.query(TrustModel).filter_by(deleted_at=None) + return [trust_ref.to_dict() for trust_ref in trusts] + +Above, the ``sql.get_session()`` call is just another call to +EngineFacade.get_session(), and that's where the connectivity is set up. +The ``sql.handle_conflicts()`` call doesn't have any role in establishing +this session. + +The above call uses the SQLAlchemy Session in "autocommit" mode; in +this mode, SQLAlchemy essentially creates connection/transaction +context on a per-query basis, and discards it when the query is complete; +using the Python Database API (DBAPI), there is no cross-platform option to +prevent a transaction from ultimately being present; hence "autocommit" +doesn't mean, "no transaction". + +In all but the most minimal cases, using the Session in "autocommit" +mode is not a good approach to take, and is discouraged in SQLAlchemy's +own documentation (see +http://docs.sqlalchemy.org/en/rel_0_9/orm/session.html#autocommit-mode), +as it means a series of queries will each proceed upon a brand new connection +and transaction per query, wasting database resources with expensive rollbacks +and even creating a new database connection per query under slight +load, where the connection pool is in overflow mode. oslo.db +itself also emits a "pessimistic ping" on each connection, where a +"SELECT 1" is emitted in order to ensure the connection is alive, so emitting +three queries in "autocommit" mode means you're actually emitting *six* +queries. + +It's true that for a method like the above where exactly one SELECT is +emitted and definitely nothing else, there is a little less Python +overhead in that the Session does not build up an internal state +object for the transaction, but this is only a tiny optimization; if +optimization at that scale is needed, there are other ways to make the +above system vastly more performant (e.g. use baked queries, column- +based queries, or Core queries). + +While both Keystone and Neutron have the issue of implicit use of +"autocommit" mode, Nova has more significant issues, both because +it is more complex at the database level and is also more performance +critical regarding persistence. + +Within Nova, the connectivity system is more or less equivalent to +that of Keystone; many explicit calls to get_session() and heavy use of +the session in "autocommit" mode, most commonly through the model_query() +function. But more critical is that the complexity of Nova's API without a +foolproof system of maintaining transaction scope leads to +a widespread use of multiple transactions per API call, in some cases +concurrently, which has definite stability and performance implications. + +A typical Nova method looks like:: + + @require_admin_context + def cell_update(context, cell_name, values): + session = get_session() + with session.begin(): + cell_query = _cell_get_by_name_query(context, cell_name, + session=session) + if not cell_query.update(values): + raise exception.CellNotFound(cell_name=cell_name) + cell = cell_query.first() + return cell + +In the above call, the ``get_session()`` call returns a brand new session +upon which a transaction is begun; the method then calls into +``_cell_get_by_name_query``, passing in the Session in an effort to +ensure this sub-method uses the same transaction. The intent here is +good, that the ``cell_update()`` method knows it should share its +transactional context with a sub-method. + +However, this is a burdensome and verbose coding pattern which is +inconsistently applied. In those areas where it fails to be applied, +the end result is that a single operation invokes several new +connections and transactions, sometimes within a nested set of calls; +this is wasteful and slow and is a key risk factor for deadlocks. +Examples of non-nested, multiple connection/session use within a +single call are easy to find. Truly nested transactions are less +frequent; one is nova/db/api.py -> floating_ip_bulk_destroy. In this +method, we see:: + + @require_context + def floating_ip_bulk_destroy(context, ips): + session = get_session() + with session.begin(): + project_id_to_quota_count = collections.defaultdict(int) + for ip_block in _ip_range_splitter(ips): + query = model_query(context, models.FloatingIp).\ + filter(models.FloatingIp.address.in_(ip_block)).\ + filter_by(auto_assigned=False) + rows = query.all() + for row in rows: + project_id_to_quota_count[row['project_id']] -= 1 + model_query(context, models.FloatingIp).\ + filter(models.FloatingIp.address.in_(ip_block)).\ + soft_delete(synchronize_session='fetch') + for project_id, count in project_id_to_quota_count.iteritems(): + try: + reservations = quota.QUOTAS.reserve(context, + project_id=project_id, + floating_ips=count) + quota.QUOTAS.commit(context, + reservations, + project_id=project_id) + except Exception: + with excutils.save_and_reraise_exception(): + LOG.exception(_("Failed to update usages bulk " + "deallocating floating IP")) + + +The entire method is within ``session.begin()``. But within that, we +first see a two calls to ``model_query()``, each of which forget to pass the +session along, so ``model_query()`` makes it's own session and transaction +for each. But more seriously, ``get_session()`` is called many times again, without +any way of passing a session through, down below when ``quota.QUOTAS.commit`` +is called within a loop. The interface for this starts outside of the +database API, in nova/quota.py, where no ``session`` argument is available:: + + def commit(self, context, reservations, project_id=None, user_id=None): + """Commit reservations.""" + if project_id is None: + project_id = context.project_id + # If user_id is None, then we use the user_id in context + if user_id is None: + user_id = context.user_id + + db.reservation_commit(context, reservations, project_id=project_id, + user_id=user_id) + +``db.reservation_commit`` is back in nova/db/api.py, where we +see a whole new call to ``get_session()``, ``begin()``, calling a second +time through the ``@_retry_on_deadlock`` decorator which also would best +know how to manage its scope at the topmost-level:: + + @require_context + @_retry_on_deadlock + def reservation_commit(context, reservations, project_id=None, user_id=None): + session = get_session() + with session.begin(): + _project_usages, user_usages = _get_project_user_quota_usages( + context, session, project_id, user_id) + reservation_query = _quota_reservations_query(session, context, + reservations) + for reservation in reservation_query.all(): + usage = user_usages[reservation.resource] + if reservation.delta >= 0: + usage.reserved -= reservation.delta + usage.in_use += reservation.delta + reservation_query.soft_delete(synchronize_session=False) + +In the above example, we first see that the "ad-hoc-session" system and +"more than one way to do it" approach of ``model_query()`` leads +to coding errors that are silently masked, but in the case of +``reservation_commit()``, the architecture itself disallows this coding +error to even be corrected. + +Examples of non-nested multiple sessions and transactions in one API call can +be found by using an assertion within the test suite. The two main +areas this occurs in the current code are: + +* instance_create() calls get_session(), + then ec2_instance_create() -> models.save() -> get_session() + +* aggregate_create() calls get_session(), + then aggregate_get() -> model_query() -> get_session() + +The above examples can be fixed manually, but rather than adding +more boilerplate, decorators, and imperative arguments to solve the problem +as individual cases are identified, the solution should instead be to replace +all imperative database code involving transaction scope with a purely +declarative facade that handles connectivity, transaction scoping and +related features like method retrying in a consistent and context-aware +fashion across all projects. + + +Proposed change +=============== + +The change is to replace the use of get_session(), get_engine(), and +special context managers with a new set of decorators and +context managers, which themselves are invoked from a +simple import that replaces the usual EngineFacade logic. + +The import will essentially allow a single symbol that handles the work +of ``EngineFacade`` and ``CONF`` behind the scenes:: + + from oslo.db import enginefacade as sql + +This symbol will provide two key decorators, ``reader()`` and +``writer()``, as well as context managers which +mirror their behavior, ``using_reader()`` and ``using_writer()``. +The decorators deliver a SQLAlchemy Session object to +the existing ``context`` argument of API methods:: + + @sql.reader + def some_api_method(context): + # work with context.session + + @sql.writer + def some_other_api_method(context): + # work with context.session + +Whereas the context managers receive this ``context`` argument locally:: + + def some_api_method(context): + with sql.using_reader(context) as session: + # work with session + + def some_other_api_method(context): + with sql.using_writer(context) as session: + # work with session + + +Transaction Scope +----------------- + +These decorators and context managers will acquire a new Session using +methods similar to that of the current ``get_session()`` function if +one is not already scoped, or if one is already scoped, will return +that existing Session. The Session will then unconditionally be +within a transaction using ``begin()``, or we may better yet switch to +the default mode of ``Session`` which is that of "autocommit=False". +The state of this transaction will be to remain open until the method +ends, either by raising an exception (unconditional rollback) or by +completing (either a commit() or a close(), depending on reader/writer +semantics). + +The goal is that any level of nested calls can all call upon +``reader()`` or ``writer()`` and participate in an already ongoing +transaction. Only the outermost call within the scope actually ends +the transaction except in the case of an exception; the ``writer()`` +method will emit a ``commit()`` and the ``reader()`` method will +``close()`` the session, ensuring that the underlying connection is +rolled back in a lightweight way. + +Context and Thread Locals +------------------------- + +The proposal at the moment expects a "context" object, which can be +any Python object, to be present in order to provide some object that +bridges all elements of a call stack together. Most APIs with the notable +exception of Keystone appear to already include a context argument. + +To support a pattern that does not include a "context" argument, the only +alternative is to use thread locals. In discussions with the community, +the use of thread locals has the two concerns of: 1. it requires early patching +at the eventlet level and 2. thread locals are seen as "action at a distance", +more "implicit" than "explicit". + +The proposal as stated here can be made to work with thread locals using +this recipe:: + + # at the top of the api module + GLOBAL_CONTEXT = threading.local() + + + def some_api_method(): + with sql.using_writer(GLOBAL_CONTEXT) as session: + # work with session + +Whether or not we build in the above pattern, or we get Keystone to use +an explicit context object, is not yet decided. See "Alternatives" for +a listing of various options. + +Reader vs. Writer +----------------- + +At the outset, ``reader()`` vs. ``writer()`` only intend to allow a block +of functionality to mark itself as only requiring read-only access, or +involving write access. At the very least, it can indicate if the outermost +block need to be concerned about committing a transaction. Beyond that, +this declaration can be used to determine if a particular method or block +is suitable for "retry on deadlock", and also allows systems that attempt +to split logic between "reader" and "writer" database links to know upfront +which blocks should be routed where. + +While a fully specified description for open-ended support of multiple +databases is out of scope for this spec, as part of the +implementation here we will necessarily implement at least what is already +present. The existing EngineFacade features a "slave_engine" attribute +as well as a "use_slave" flag on ``get_session()`` and ``get_engine()``; +at least the Nova project and possibly others currently make use of this +flag. So we will carry over an equivalent level of functionality into +``reader()`` and ``writer()`` to start. + +Beyond maintaining existing functionality, more comprehensive and +potentially elaborate systems of multiple database support will be +made easier to specify and implement subsequent to the rollout +of this specification. This is because consuming projects will greatly reduce +their verbosity down to a simple declarative level, leaving oslo.db +free to expand upon the underlying machinery without incurring additional +across-the-board changes in projects (hence one of the main reasons "facades" +are used). + +The behavior for nesting of readers and writers is as follows: + +1. A ``reader()`` block that ultimately calls upon methods that then invoke + ``writer()`` should raise an exception; it means this ``reader()`` is not + really a ``reader()`` at all. + +2. A ``writer()`` block that ultimately + calls upon methods that invoke ``reader()`` should pass successfully; + those ``reader()`` blocks will in fact be made to act as a ``writer()`` + if they are called within the context of a ``writer()`` block. + +Core Connection Methods +----------------------- + +For those methods that use Core only, corresponding methods +``reader_connection()`` and ``writer_connection()`` are supplied, +which instead of returning a ``sqlalchemy.orm.Session``, return a +``sqlalchemy.engine.Connection``:: + + @sql.writer_connection + def some_core_api_method(context): + context.connection.execute() + + def some_core_api_method(context): + with sql.using_writer_connection(context) as conn: + conn.execute() + +``reader_connection()`` and ``writer_connection()`` will integrate with +``reader()`` and ``writer()``, such that the outermost context will establish +the ``sqlalchemy.engine.Connection`` that is to be used for the full +context, whether or not it is associated with a ``Session``. This means +the following: + +1. If a ``reader_connection()`` or ``writer_connection()`` manager is invoked + first, a ``sqlalchemy.engine.Connection`` + is associated with the context, and not a ``Session``. + +2. If a ``reader()`` or ``writer()`` manager is invoked first, a ``Session`` + is associated with the context, which will contain within it a + ``sqlalchemy.engine.Connection``. + +3. If a ``reader_connection()`` or ``writer_connection()`` manager is invoked + and there is already a ``Session`` present, the ``Session.connection()`` + method of that ``Session`` is used to get at the ``Connection``. + +4. If a ``reader()`` or ``writer()`` manager is invoked and there is already + a ``Connection`` present, the new ``Session`` is created, and it is + bound directly to this existing ``Connection``. + +Integration with Configuration / Startup +----------------------------------------- + +The ``reader()``, ``writer()`` and other methods will be calling upon +functional equivalents of the current ``get_session()`` and +``get_engine()`` methods within oslo.db, as well as handling the logic +that currently consists of invoking an ``EngineFacade`` and combining +it with ``CONF``. That is, the consuming application does not refer +to ``EngineFacade`` or ``CONF`` at all; the interaction with ``CONF`` +is performed similarly as it is now within oslo.db only, and is done +under a mutex so that it is thread safe, in the way that Nova performs +this task. + +For applications that currently have special logic to add keys to ``CONF`` +or ``EngineFacade``, additional API methods will be provided. For example, +Sahara wants to ensure the ``sqlite_fk`` flag is set to ``True``. The +pattern will look like:: + + from oslo.db import enginefacade as sql + + sql.configure(sqlite_fk=True) + + def some_api_method(): + with sql.reader() as session: + # work with session + +Retry on Deadlock / Other failures +----------------------------------- + +Oslo.db provides the ``@wrap_db_retry()`` decorator, which allows an API +method to replay itself on failure. Per +https://review.openstack.org/#/c/109549/, we will be adding specificity +to this decorator, which allows it to explicitly indicate that a method +should be retried when a deadlock condition occurs. We can look into +integrating this feature into the ``reader()`` and ``writer()`` decorators +as well. + + +Alternatives +------------ + +A key decision here is that of the decorator vs. the context manager, +as well as the use of thread locals. + +Example forms: + +1. Decorator, using context:: + + @sql.reader + def some_api_method(context): + # work with context.session + + @sql.writer + def some_other_api_method(context): + # work with context.session + + +2. Decorator, using thread local; here, the ``session`` argument is injected + into the argument list of the API method within the scope of the decorator, + it is *not* present in the outer call to the API method:: + + @sql.reader + def some_api_method(session): + # work with session + + @sql.writer + def some_other_api_method(session): + # work with session + +3. Context manager, using context:: + + def some_api_method(context): + with sql.using_reader(context) as session: + # work with session + + def some_other_api_method(context): + with sql.using_writer(context) as session: + # work with session + +4. Context manager, using implicit thread local:: + + def some_api_method(): + with sql.using_reader() as session: + # work with session + + def some_other_api_method(): + with sql.using_writer() as session: + # work with session + +5. Context manager, using explicit thread local:: + + def some_api_method(): + with sql.using_reader(GLOBAL_CONTEXT) as session: + # work with session + + def some_other_api_method(): + with sql.using_writer(GLOBAL_CONTEXT) as session: + # work with session + +The author favors approach #1. It should be noted that *all* the above +approaches can be supported at the same time, if projects cannot agree +on an approach. + +Advantages to using a decorator only with an explicit context are: + +1. The need for thread locals or any issues with eventlet is removed. + +2. The "Retry on deadlock" and other "retry" features could be + integrated into the ``reader()`` / ``writer()`` decorators, such that + all API methods automatically gain this feature. As it stands, + applications need to constantly push out new changes each time an + unavoidable deadlock situation is detected in the wild, adding their + ``@_retry_on_deadlock()`` decorators to ever more API methods. + +3. The decorator reduces nesting depth compared to context managers, and + is ultimately less verbose, save for the need to have a "context" + argument. + +4. Decorators eliminate the possibility of this already-present + antipattern:: + + def some_api_method(): + with sql.writer() as session: + # do something with session + + # transaction completes here + + for element in stuff: + # new transaction per element + some_other_api_method_with_db(element) + +Above, we are inadvertently performing any number of distinct +transactions, first with the ``sql.writer()``, then with each call to +some_other_api_method_with_db(). This antipattern can already be seen +in methods like Nova's ``instance_create()`` method, paraphrased +below:: + + @require_context + def instance_create(context, values): + + # ... about halfway through + + session = get_session() + + # session / connection / transaction #1 + with session.begin(): + # does some things with instnace_ref + + # session / connection / transaction #2 + ec2_instance_create(context, instance_ref['uuid']) + + # session / connection / transaction #3 + _instance_extra_create(context, {'instance_uuid': instance_ref['uuid']}) + + return instance_ref + +Because the context manager allows unnecessary choices about when a +transaction can begin and end within a method, we open ourselves up to make +the wrong choice, as is already occurring in current code. Using a decorator, +this antipattern is impossible:: + + @sql.writer() + def some_api_method(context): + # do something with context.session + + for element in stuff: + # uses same session / transaction guaranteed + some_other_api_method_with_db(context, element) + + # transaction completes here + +One advantage to using an implicit "thread local" context is that it is impossible +to inadvertently switch contexts in the middle of a call-chain, which would +again lead to the nested-transaction issue. + +An advantage of using context managers with implicit threadlocals is that +it would be easier for Keystone to migrate to this system. + + +Impact on Existing APIs +----------------------- + +Existing projects would need to integrate into some form of the +patterns given. + + +Security impact +--------------- + +none + +Performance Impact +------------------ + +Performance will be dramatically improved as the current use of many +redundant and disconnected sessions and transactions will be joined together. + + +Configuration Impact +-------------------- + +none. + + +Developer Impact +---------------- + +new patterns for developers to be aware of. + +Testing Impact +-------------- + +As most test suites currently make the simple decision of working with +SQLite and allowing API methods to make use of their usual get_session() / +get_engine() logic without any change or injection, little to no changes +should be needed at first. Within oslo.db, the "opportunistic" fixtures +as well as the DbTestCase system will be made to integrate with the +new context manager/decorator system. + + + +Implementation +============== + +Assignee(s) +----------- + +Mike Bayer + +Milestones +---------- + +Target Milestone for completion: + +Work Items +---------- + + +Incubation +========== + +Adoption +-------- + +Library +------- + +Anticipated API Stabilization +----------------------------- + + +Documentation Impact +==================== + +Dependencies +============ + + +References +========== + + +.. note:: + + This work is licensed under a Creative Commons Attribution 3.0 + Unported License. + http://creativecommons.org/licenses/by/3.0/legalcode +