Open Stack

Hey all -

Concerning the enginefacade system, approved blueprint:

https://review.openstack.org/#/c/125181/

which will replace the use of oslo_db.sqlalchemy.EngineFacade ultimately across all projects that use it (which is, all of them that use a database).

We are struggling to find a solution for the issue of application-defined contexts that might do things that the facade needs to know about, namely 1. that the object might be copied using deepcopy() or 2. that the object might be sent into a new set of worker threads, where its attributes are accessed concurrently.

While the above blueprint and the implementations so far have assumed that we need to receive this context object and use simple assignment, e.g. “context.session = the_session” in order to provide its attributes, in order to accommodate 1. and 2. I’ve had to add a significant amount of complexity in order to accommodate those needs (see patch set 28 at https://review.openstack.org/#/c/138215/).   It all works fine, but predictably, people are not comfortable with the extra few yards into the weeds it has to go to make all that happen.  In particular, in order to accommodate a RequestContext that is running in a different thread, it has to be copied first, because we have no ability to make the “.session” or “.connection” attributes dynamic without access to the RequestContext class up front.

So, what’s the alternative.   It’s that enginefacade is given just a tiny bit of visibility into the *class* used to create your context, such as in Nova, the nova.context.RequestContext class, so that we can place dynamic descriptors on it before instantiation (or I suppose we could monkeypatch the class on the first RequestContext object we see, but that seems even less desirable).   The blueprint went out of its way to avoid this.   But with contexts being copied and thrown into threads, I didn’t consider these use cases and I’d have probably needed to do the BP differently.

So what does the change look like?    If you’re not Nova, imagine you’re cinder.context.RequestContext, heat.common.context.RequestContext, glance.context.RequestContext, etc.    We throw a class decorator onto the class so that enginefacade can add some descriptors:

diff --git a/nova/context.py b/nova/context.py
index e78636c..205f926 100644
--- a/nova/context.py
+++ b/nova/context.py
@@ -22,6 +22,7 @@ import copy
from keystoneclient import auth
from keystoneclient import service_catalog
from oslo.utils import timeutils
+from oslo_db.sqlalchemy import enginefacade
import six

from nova import exception
@@ -61,6 +62,7 @@ class _ContextAuthPlugin(auth.BaseAuthPlugin):
                                            region_name=region_name)


+ at enginefacade.transaction_context_provider
class RequestContext(object):
    """Security context and request information.


the implementation of this one can be seen here: https://review.openstack.org/#/c/149289/.   In particular we can see all the lines of code removed from oslo’s approach, and in fact there’s a lot more nasties I can take out once I get to work on that some more.

so what’s controversial about this?   It’s that there’s an “oslo_db.sqlalchemy” import up front in the XYZ/context.py module of every participating project, outside of where anything else “sqlalchemy” happens.  

There’s potentially other ways to do this - subclasses of RequestContext that are generated by abstract factories, for one.   As I left my Java gigs years ago I’m hesitant to go there either :).   Perhaps projects can opt to run their RequestContext class through this decorator conditionally, wherever it is that it gets decided they are about to use their db/sqlalchemy/api.py module.

So can I please get +1 / -1 from the list on, “oslo_db.sqlalchemy wants an up-front patch on everyone’s RequestContext class”  ?  thanks!

- mike