Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction
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 = 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. 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
-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 happened atomically. I think Boris led the charge to eliminate that, culminating with the hacking rule you mentioned. Maybe getting
Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction
* 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
[openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction
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 = 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) 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
Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction
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 mbo...@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. 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 = 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) 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 ___
Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction
On Nov 19, 2014, at 11:46 AM, Matthew Booth mbo...@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
Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction
On Nov 19, 2014, at 12:59 PM, Boris Pavlovic bpavlo...@mirantis.com wrote: Matthew, LOL ORM on top of another ORM https://img.neoseeker.com/screenshots/TW92aWVzL0RyYW1h/inception_image33.png 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 mbo...@redhat.com mailto:mbo...@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 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 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 = 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) 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
Re: [openstack-dev] [oslo.db][nova] NovaObject.save() needs its own DB transaction
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 ___ OpenStack-dev
