Open Stack

> On Nov 19, 2014, at 11:46 AM, Matthew Booth <mbooth at redhat.com> wrote:
> 
> We currently have a pattern in Nova where all database code lives in
> db/sqla/api.py[1]. Database transactions are only ever created or used
> in this module. This was an explicit design decision:
> https://blueprints.launchpad.net/nova/+spec/db-session-cleanup .
> 
> However, it presents a problem when we consider NovaObjects, and
> dependencies between them. For example, take Instance.save(). An
> Instance has relationships with several other object types, one of which
> is InstanceInfoCache. Consider the following code, which is amongst what
> happens in spawn():
> 
> instance = Instance.get_by_uuid(uuid)
> instance.vm_state = vm_states.ACTIVE
> instance.info_cache.network_info = new_nw_info
> instance.save()
> 
> instance.save() does (simplified):
>  self.info_cache.save()
>  self._db_save()
> 
> Both of these saves happen in separate db transactions.
> 

> I don't think we can reasonably remove the cascading save() above due to
> the deliberate design of objects. Objects don't correspond directly to
> their datamodels, so save() does more work than just calling out to the
> DB. We need a way to allow cascading object saves to happen within a
> single DB transaction.

So this is actually part of what https://review.openstack.org/#/c/125181/ aims to solve.    If it isn’t going to achieve this (and I think I see what the problem is), we need to fix it.

> 
> Note that we also have a separate problem, which is that the DB api's
> internal use of transactions is wildly inconsistent. A single db api
> call can result in multiple concurrent db transactions from the same
> thread, and all the deadlocks that implies. This needs to be fixed, but
> it doesn't require changing our current assumption that DB transactions
> live only within the DB api.
> 
> Note that there is this recently approved oslo.db spec to make
> transactions more manageable:
> 
> https://review.openstack.org/#/c/125181/11/specs/kilo/make-enginefacade-a-facade.rst,cm
> 
> Again, while this will be a significant benefit to the DB api, it will
> not solve the problem of cascading object saves without allowing
> transaction management at the level of NovaObject.save(): we need to
> allow something to call a db api with an existing session, and we need
> to allow something to pass an existing db transaction to NovaObject.save().

OK so here is why EngineFacade as described so far doesn’t work, because if it is like this:

def some_api_operation ->

        novaobject1.save() ->

               @writer
               def do_some_db_thing()

        novaobject2.save() ->

               @writer
               def do_some_other_db_thing()

then yes, those two @writer calls aren’t coordinated.   So yes, I think something that ultimately communicates the same meaning as @writer needs to be at the top:

@something_that_invokes_writer_without_exposing_db_stuff
def some_api_operation ->

    # … etc

If my decorator is not clear enough, let me clarify that a decorator that is present at the API/ nova objects layer will interact with the SQL layer through some form of dependency injection, and not any kind of explicit import; that is, when the SQL layer is invoked, it registers some kind of state onto the @something_that_invokes_writer_without_exposing_db_stuff system that causes its “cleanup”, in this case the commit(), to occur at the end of that topmost decorator.


> I think the following pattern would solve it:
> 
> @remotable
> def save():
>    session = <insert magic here>
>    try:
>        r = self._save(session)
>        session.commit() (or reader/writer magic from oslo.db)
>        return r
>    except Exception:
>        session.rollback() (or reader/writer magic from oslo.db)
>        raise
> 
> @definitelynotremotable
> def _save(session):
>    previous contents of save() move here
>    session is explicitly passed to db api calls
>    cascading saves call object._save(session)

so again with EngineFacade rewrite, the @definitelynotremotable system should also interact such that if @writer is invoked internally, an error is raised, just the same as when @writer is invoked within @reader.


> 
> Whether we wait for the oslo.db updates or not, we need something like
> the above. We could implement this today by exposing
> db.sqla.api.get_session().

EngineFacade is hoped to be ready for Kilo and obviously Nova is very much hoped to be my first customer for integration.     It would be great if folks want to step up and help implement it, or at least take hold of a prototype I can build relatively quickly and integration test it and/or work on a real nova integration.

> 
> Thoughts?
> 
> Matt
> 
> [1] At a slight tangent, this looks like an artifact of some premature
> generalisation a few years ago. It seems unlikely that anybody is going
> to rewrite the db api using an ORM other than sqlalchemy, so we should
> probably ditch it and promote it to db/api.py.

funny you should mention that as this has already happened and it is already used in at least one production environment:  https://review.openstack.org/#/c/53019/.   So it has happened, though going forward this approach is not the plan.