openstack/oslo-specs
Code Issues Proposed changes

db: Make EngineFacade a Facade

Browse Source 
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
This commit is contained in:
Mike Bayer 2014-09-30 16:11:44 -04:00 committed by Ben Nemec
parent 740b16d661
commit ec52f4370b
1 changed files with 767 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

767
specs/kilo/make-enginefacade-a-facade.rst Normal file
View File

@ -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(<statement>)
def some_core_api_method(context):
with sql.using_writer_connection(context) as conn:
conn.execute(<statement>)
``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