Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction

[Jay Pipes]

On 11/19/2014 01:25 PM, Mike Bayer wrote:

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 = 
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.


My impression after reading the EngineFacade spec (and the reason I 
supported it, and still support the idea behind it) was that the "API 
call" referred to in the EngineFacade spec was the *nova-conductor* API 
call, not the *nova-api* API call. We need a way to mark an RPC API call 
on the nova-conductor as involving a set of writer or reader DB calls, 
and that's what I thought we were referring to in that spec. I 
specifically did not think that we were leaving the domain of the 
nova-conductor, because clearly we would be leaving the domain of a 
single RPC call in that case, and in order to do transactional 
containers, we'd need to use two-phase commit, which is definitely not 
something I recommend...


So, in short, for the EngineFacade effort, I believe the @reader and 
@writer decorators should be on the conductor RPC API calls.


Best,
-jay

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction

[Dan Smith]

> * Flavor.save() makes an unbounded number of db calls in separate
> transactions.

This is actually part of the design of the original flavors public API.
Since we can add and remove projects/specs individually, we avoid ending
up with just one or the other group of values, for competing requests.

But as I said before, your point is entirely valid for the cases where
it's relevant.

> * Instance.save() cascades saves to security groups, each of which is
> saved in a separate transaction.
> 
> We can push these into the db layer, but it is my understanding that
> NovaObjects are supposed to manage their own mapping of internal state
> -> db representation. If we push this into the db api, we're violating
> the separation of concerns. If we're going to do this, we'd better
> understand, and be able to articulate, *why* we don't want to allow
> NovaObjects to manage a db transaction when required. The pattern I
> outlined below is very simple.

The DB API isn't a stable interface, and exists purely to do what we
need it to do. So, if pushing the above guarantees into the DB API works
for the moment, we can do that. If we needed to change something about
it in the future, we could. If you look at it right now, it has some
very (very) specific queries and updates that serve very directed
purposes. I don't see pushing atomicity expectations into the DB API as
a problem.

> I'm proposing a pattern which is always safe and is simple to reason
> about. I would implement it everywhere. I don't think there are any
> downsides.

Cool, sounds like a spec. I'd say propose it and we'll see where it goes
in review.

Thanks!

--Dan



signature.asc
Description: OpenPGP digital signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction

[Matthew Booth]

On 19/11/14 18:25, Mike Bayer wrote:
>> 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.

Take a look at this WIP: https://review.openstack.org/#/c/136040/

It's obviously not using oslo.db, but I think it's sufficiently similar
to the proposal to be a fairly simple mechanical fixup. It's incomplete
and there are still test failures to look at in there, but I have a good
feeling about the approach.

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction

[Matthew Booth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 19/11/14 18:39, Dan Smith wrote:
>> However, it presents a problem when we consider NovaObjects, and 
>> dependencies between them.
> 
> I disagree with this assertion, because:
> 
>> 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.
> 
> This has always been two DB calls, and for a while recently, it was
> two RPCs, each of which did one call.

Well, let's take the opportunity to fix it :)

I'll also point out that although your head may contain everywhere
that Nova touches both info_cache and instance:

1. Not many other peoples' do.
2. Reasoning about the safety of this robustly is still hard.

As I mentioned, this is also just 1 example among many. Others include:

* Flavor.save() makes an unbounded number of db calls in separate
transactions.

* Instance.save() cascades saves to security groups, each of which is
saved in a separate transaction.

We can push these into the db layer, but it is my understanding that
NovaObjects are supposed to manage their own mapping of internal state
- -> db representation. If we push this into the db api, we're violating
the separation of concerns. If we're going to do this, we'd better
understand, and be able to articulate, *why* we don't want to allow
NovaObjects to manage a db transaction when required. The pattern I
outlined below is very simple.

>> This has at least 2 undesirable effects:
>> 
>> 1. A failure can result in an inconsistent database. i.e.
>> info_cache having been persisted, but instance.vm_state not
>> having been persisted.
>> 
>> 2. Even in the absence of a failure, an external reader can see
>> the new info_cache but the old instance.
> 
> I think you might want to pick a different example. We update the 
> info_cache all the time asynchronously, due to "time has passed"
> and other non-user-visible reasons.

Ok, pick one of the others above.

>> New features continue to add to the problem, including numa
>> topology and pci requests.
> 
> NUMA and PCI information are now created atomically with the
> instance (or at least, passed to SQLA in a way I expect does the
> insert as a single transaction). We don't yet do that in save(), I
> think because we didn't actually change this information after
> creation until recently.
> 
> Definitely agree that we should not save the PCI part without the
> base instance part.

How are we going to achieve that?

>> 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. This will mean:
>> 
>> 1. A change will be persisted either entirely or not at all in
>> the event of a failure.
>> 
>> 2. A reader will see either the whole change or none of it.
> 
> This is definitely what we should strive for in cases where the
> updates are related, but as I said above, for things (like info
> cache) where it doesn't matter, we should be fine.

I'm proposing a pattern which is always safe and is simple to reason
about. I would implement it everywhere. I don't think there are any
downsides.

>> 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().
> 
> I don't agree that we need to be concerned about this at the 
> NovaObject.save() level. I do agree that Instance.save() needs to
> have a relationship to its sub-objects that facilitates atomicity
> (where appropriate), and that such a pattern can be used for other
> such hierarchies.
> 
>> An obvious precursor to that is removing N309 from hacking,
>> which specifically tests for db apis which accept a session
>> argument. We then need to consider how NovaObject.save() should
>> manage and propagate db transactions.
> 
> Right, so I believe that we had more consistent handling of
> transactions in the past. We had a mechanism for passing around the
> session between chained db/api methods to ensure they

Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction

[Dan Smith]

> However, it presents a problem when we consider NovaObjects, and
> dependencies between them.

I disagree with this assertion, because:

> 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.

This has always been two DB calls, and for a while recently, it was two
RPCs, each of which did one call.

> This has at least 2 undesirable effects:
> 
> 1. A failure can result in an inconsistent database. i.e. info_cache
> having been persisted, but instance.vm_state not having been persisted.
> 
> 2. Even in the absence of a failure, an external reader can see the new
> info_cache but the old instance.

I think you might want to pick a different example. We update the
info_cache all the time asynchronously, due to "time has passed" and
other non-user-visible reasons.

> New features continue to add to the problem,
> including numa topology and pci requests.

NUMA and PCI information are now created atomically with the instance
(or at least, passed to SQLA in a way I expect does the insert as a
single transaction). We don't yet do that in save(), I think because we
didn't actually change this information after creation until recently.

Definitely agree that we should not save the PCI part without the base
instance part.

> 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. This will mean:
> 
> 1. A change will be persisted either entirely or not at all in the event
> of a failure.
> 
> 2. A reader will see either the whole change or none of it.

This is definitely what we should strive for in cases where the updates
are related, but as I said above, for things (like info cache) where it
doesn't matter, we should be fine.

> 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().

I don't agree that we need to be concerned about this at the
NovaObject.save() level. I do agree that Instance.save() needs to have a
relationship to its sub-objects that facilitates atomicity (where
appropriate), and that such a pattern can be used for other such
hierarchies.

> An obvious precursor to that is removing N309 from hacking, which
> specifically tests for db apis which accept a session argument. We then
> need to consider how NovaObject.save() should manage and propagate db
> transactions.

Right, so I believe that we had more consistent handling of transactions
in the past. We had a mechanism for passing around the session between
chained db/api methods to ensure they happened atomically. I think Boris
led the charge to eliminate that, culminating with the hacking rule you
mentioned.

Maybe getting back to the justification for removing that facility would
help us understand the challenges we face going forward?

> [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.

We've had a few people ask about it, in terms of rewriting some or all
of our DB API to talk to a totally non-SQL backend. Further, AFAIK, RAX
rewrites a few of the DB API calls to use raw SQL queries for
performance (or did, at one point).

I'm quite happy to have the implementation of Instance.save() make use
of primitives to ensure atomicity where appropriate. I don't think
that's something that needs or deserves generalization at this point,
and I'm not convinced it needs to be in the save method itself. Right
now we update several things atomically by passing something to db/api
that gets turned into properly-related SQLA objects. I think we could do
the same for any that we're currently cascading separately, even if the
db/api update method uses a transaction to ensure safety.

--Dan



signature.asc
Description: OpenPGP digital signature
___

Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction

[Mike Bayer]

> On Nov 19, 2014, at 12:59 PM, Boris Pavlovic  wrote:
> 
> Matthew, 
> 
> 
> LOL ORM on top of another ORM 
> 
> https://img.neoseeker.com/screenshots/TW92aWVzL0RyYW1h/inception_image33.png 
> 

I know where you stand on this Boris, but I fail to see how this is a 
productive contribution to the discussion.  Leo Dicaprio isn’t going to solve 
our issue here and I look forward to iterating on what we have today.




> 
> 
> 
> Best regards,
> Boris Pavlovic 
> 
> On Wed, Nov 19, 2014 at 8:46 PM, Matthew Booth  > 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. This has at
> least 2 undesirable effects:
> 
> 1. A failure can result in an inconsistent database. i.e. info_cache
> having been persisted, but instance.vm_state not having been persisted.
> 
> 2. Even in the absence of a failure, an external reader can see the new
> info_cache but the old instance.
> 
> This is one example, but there are lots. We might convince ourselves
> that the impact of this particular case is limited, but there will be
> others where it isn't. Confidently assuring ourselves of a limited
> impact also requires a large amount of context which not many
> maintainers will have. New features continue to add to the problem,
> including numa topology and pci requests.
> 
> 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. This will mean:
> 
> 1. A change will be persisted either entirely or not at all in the event
> of a failure.
> 
> 2. A reader will see either the whole change or none of it.
> 
> We are not talking about crossing an RPC boundary. The single database
> transaction only makes sense within the context of a single RPC call.
> This will always be the case when NovaObject.save() cascades to other
> object saves.
> 
> 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().
> 
> An obvious precursor to that is removing N309 from hacking, which
> specifically tests for db apis which accept a session argument. We then
> need to consider how NovaObject.save() should manage and propagate db
> transactions.
> 
> I think the following pattern would solve it:
> 
> @remotable
> def save():
> session = 
> 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)
> 
> 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().
> 
> Thoughts?
> 
> Matt

Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction

[Mike Bayer]

> On Nov 19, 2014, at 11:46 AM, Matthew Booth  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 = 
>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 menti

Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction

[Boris Pavlovic]

Matthew,


LOL ORM on top of another ORM 

https://img.neoseeker.com/screenshots/TW92aWVzL0RyYW1h/inception_image33.png



Best regards,
Boris Pavlovic

On Wed, Nov 19, 2014 at 8:46 PM, Matthew Booth  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. This has at
> least 2 undesirable effects:
>
> 1. A failure can result in an inconsistent database. i.e. info_cache
> having been persisted, but instance.vm_state not having been persisted.
>
> 2. Even in the absence of a failure, an external reader can see the new
> info_cache but the old instance.
>
> This is one example, but there are lots. We might convince ourselves
> that the impact of this particular case is limited, but there will be
> others where it isn't. Confidently assuring ourselves of a limited
> impact also requires a large amount of context which not many
> maintainers will have. New features continue to add to the problem,
> including numa topology and pci requests.
>
> 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. This will mean:
>
> 1. A change will be persisted either entirely or not at all in the event
> of a failure.
>
> 2. A reader will see either the whole change or none of it.
>
> We are not talking about crossing an RPC boundary. The single database
> transaction only makes sense within the context of a single RPC call.
> This will always be the case when NovaObject.save() cascades to other
> object saves.
>
> 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().
>
> An obvious precursor to that is removing N309 from hacking, which
> specifically tests for db apis which accept a session argument. We then
> need to consider how NovaObject.save() should manage and propagate db
> transactions.
>
> I think the following pattern would solve it:
>
> @remotable
> def save():
> session = 
> 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)
>
> 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().
>
> 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.
> --
> Matthew Booth
> Red Hat Engineering, Virtualisation Team
>
> Phone: +442070094448 (UK)
> GPG ID:  D33C3490
> GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mail

[openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction

[Matthew Booth]

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. This has at
least 2 undesirable effects:

1. A failure can result in an inconsistent database. i.e. info_cache
having been persisted, but instance.vm_state not having been persisted.

2. Even in the absence of a failure, an external reader can see the new
info_cache but the old instance.

This is one example, but there are lots. We might convince ourselves
that the impact of this particular case is limited, but there will be
others where it isn't. Confidently assuring ourselves of a limited
impact also requires a large amount of context which not many
maintainers will have. New features continue to add to the problem,
including numa topology and pci requests.

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. This will mean:

1. A change will be persisted either entirely or not at all in the event
of a failure.

2. A reader will see either the whole change or none of it.

We are not talking about crossing an RPC boundary. The single database
transaction only makes sense within the context of a single RPC call.
This will always be the case when NovaObject.save() cascades to other
object saves.

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().

An obvious precursor to that is removing N309 from hacking, which
specifically tests for db apis which accept a session argument. We then
need to consider how NovaObject.save() should manage and propagate db
transactions.

I think the following pattern would solve it:

@remotable
def save():
session = 
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)

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().

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.
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev