Re: [openstack-dev] [oslo] [nova] [all] potential enginefacade adjustment - can everyone live with this?
On Mon, Jan 26, 2015, at 08:55 AM, Matthew Booth wrote: > On 23/01/15 19:47, Mike Bayer wrote: > > > > > > Doug Hellmann wrote: > > > >> > >> > >> On Fri, Jan 23, 2015, at 12:49 PM, Mike Bayer wrote: > >>> Mike Bayer wrote: > >>> > Ihar Hrachyshka wrote: > > > On 01/23/2015 05:38 PM, Mike Bayer wrote: > >> Doug Hellmann wrote: > >> > >>> We put the new base class for RequestContext in its own library > >>> because > >>> both the logging and messaging code wanted to influence it's API. > >>> Would > >>> it make sense to do this database setup there, too? > >> whoa, where’s that? is this an oslo-wide RequestContext class ? that > >> would > >> solve everything b.c. right now every project seems to implement > >> RequestContext themselves. > >>> > >>> > >>> so Doug - > >>> > >>> How does this “influence of API” occur, would oslo.db import > >>> oslo_context.context and patch onto RequestContext at that point? Or the > >>> other way around? Or… ? > >> > >> No, it's a social thing. I didn't want dependencies between > >> oslo.messaging and oslo.log, but the API of the context needs to support > >> use cases in both places. > >> > >> Your case might be different, in that we might need to actually have > >> oslo.context depend on oslo.db in order to call some setup code. We'll > >> have to think about whether that makes sense and what other dependencies > >> it might introduce between the existing users of oslo.context. > > > > hey Doug - > > > > for the moment, I have oslo_db.sqlalchemy.enginefacade applying its > > descriptors at import time onto oslo_context: OK. I'll have to think about that. We've been trying to move away from import-time side-effects elsewhere (especially with configuration options) so we may want to think about doing the decoration at runtime, either through a hook discovered by oslo.context or by placing the call right in oslo.context. Let's talk about options this week. > > > > https://review.openstack.org/#/c/138215/30/oslo_db/sqlalchemy/enginefacade.py > > > > https://review.openstack.org/gitweb?p=openstack/oslo.db.git;a=blob;f=oslo_db/sqlalchemy/enginefacade.py;h=3f76678a6c9788f62288c8fa5ef520db8dff2c0a;hb=bc33d20dc6db2f8e5f8cb02b4eb5f97d24dafb7a#l692 > > > > https://review.openstack.org/gitweb?p=openstack/oslo.db.git;a=blob;f=oslo_db/sqlalchemy/enginefacade.py;h=3f76678a6c9788f62288c8fa5ef520db8dff2c0a;hb=bc33d20dc6db2f8e5f8cb02b4eb5f97d24dafb7a#l498 > > May I suggest that we decouple these changes by doing both? Oslo's > RequestContext object can have the enginefacade decorator applied to it, > so any project which uses it doesn't have to apply it themselves. > Meanwhile, the decorator remains part of the public api for projects not > using the oslo RequestContext. We can leave the function in the public API, but oslo.log assumes the application is using a context class subclassed from the base class provided in oslo.context. As we evolve that integration, we'll make appropriate changes to the base class so they will usually be transparent to the application code, so it's going to be safer to just use the base class we provide. Doug > > I suggest that we'll probably stay with a decorator within oslo, anyway. > It doesn't lend itself well to a Mixin, as we need a reference to a > specific _TransactionContextManager, and moving code directly into > RequestContext would be a very invasive coupling. > > 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 Development Mailing List (not for usage questions) > Unsubscribe: > openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo] [nova] [all] potential enginefacade adjustment - can everyone live with this?
On 23/01/15 19:47, Mike Bayer wrote: > > > Doug Hellmann wrote: > >> >> >> On Fri, Jan 23, 2015, at 12:49 PM, Mike Bayer wrote: >>> Mike Bayer wrote: >>> Ihar Hrachyshka wrote: > On 01/23/2015 05:38 PM, Mike Bayer wrote: >> Doug Hellmann wrote: >> >>> We put the new base class for RequestContext in its own library because >>> both the logging and messaging code wanted to influence it's API. Would >>> it make sense to do this database setup there, too? >> whoa, where’s that? is this an oslo-wide RequestContext class ? that >> would >> solve everything b.c. right now every project seems to implement >> RequestContext themselves. >>> >>> >>> so Doug - >>> >>> How does this “influence of API” occur, would oslo.db import >>> oslo_context.context and patch onto RequestContext at that point? Or the >>> other way around? Or… ? >> >> No, it's a social thing. I didn't want dependencies between >> oslo.messaging and oslo.log, but the API of the context needs to support >> use cases in both places. >> >> Your case might be different, in that we might need to actually have >> oslo.context depend on oslo.db in order to call some setup code. We'll >> have to think about whether that makes sense and what other dependencies >> it might introduce between the existing users of oslo.context. > > hey Doug - > > for the moment, I have oslo_db.sqlalchemy.enginefacade applying its > descriptors at import time onto oslo_context: > > https://review.openstack.org/#/c/138215/30/oslo_db/sqlalchemy/enginefacade.py > > https://review.openstack.org/gitweb?p=openstack/oslo.db.git;a=blob;f=oslo_db/sqlalchemy/enginefacade.py;h=3f76678a6c9788f62288c8fa5ef520db8dff2c0a;hb=bc33d20dc6db2f8e5f8cb02b4eb5f97d24dafb7a#l692 > > https://review.openstack.org/gitweb?p=openstack/oslo.db.git;a=blob;f=oslo_db/sqlalchemy/enginefacade.py;h=3f76678a6c9788f62288c8fa5ef520db8dff2c0a;hb=bc33d20dc6db2f8e5f8cb02b4eb5f97d24dafb7a#l498 May I suggest that we decouple these changes by doing both? Oslo's RequestContext object can have the enginefacade decorator applied to it, so any project which uses it doesn't have to apply it themselves. Meanwhile, the decorator remains part of the public api for projects not using the oslo RequestContext. I suggest that we'll probably stay with a decorator within oslo, anyway. It doesn't lend itself well to a Mixin, as we need a reference to a specific _TransactionContextManager, and moving code directly into RequestContext would be a very invasive coupling. 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 Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo] [nova] [all] potential enginefacade adjustment - can everyone live with this?
Doug Hellmann wrote: > > > On Fri, Jan 23, 2015, at 12:49 PM, Mike Bayer wrote: >> Mike Bayer wrote: >> >>> Ihar Hrachyshka wrote: >>> On 01/23/2015 05:38 PM, Mike Bayer wrote: > Doug Hellmann wrote: > >> We put the new base class for RequestContext in its own library because >> both the logging and messaging code wanted to influence it's API. Would >> it make sense to do this database setup there, too? > whoa, where’s that? is this an oslo-wide RequestContext class ? that would > solve everything b.c. right now every project seems to implement > RequestContext themselves. >> >> >> so Doug - >> >> How does this “influence of API” occur, would oslo.db import >> oslo_context.context and patch onto RequestContext at that point? Or the >> other way around? Or… ? > > No, it's a social thing. I didn't want dependencies between > oslo.messaging and oslo.log, but the API of the context needs to support > use cases in both places. > > Your case might be different, in that we might need to actually have > oslo.context depend on oslo.db in order to call some setup code. We'll > have to think about whether that makes sense and what other dependencies > it might introduce between the existing users of oslo.context. hey Doug - for the moment, I have oslo_db.sqlalchemy.enginefacade applying its descriptors at import time onto oslo_context: https://review.openstack.org/#/c/138215/30/oslo_db/sqlalchemy/enginefacade.py https://review.openstack.org/gitweb?p=openstack/oslo.db.git;a=blob;f=oslo_db/sqlalchemy/enginefacade.py;h=3f76678a6c9788f62288c8fa5ef520db8dff2c0a;hb=bc33d20dc6db2f8e5f8cb02b4eb5f97d24dafb7a#l692 https://review.openstack.org/gitweb?p=openstack/oslo.db.git;a=blob;f=oslo_db/sqlalchemy/enginefacade.py;h=3f76678a6c9788f62288c8fa5ef520db8dff2c0a;hb=bc33d20dc6db2f8e5f8cb02b4eb5f97d24dafb7a#l498 > > Doug > >> I’m almost joyful that this is here. Assuming we can get everyone to >> use it, should be straightforward for that right? >> >> >> >> __ >> OpenStack Development Mailing List (not for usage questions) >> Unsubscribe: >> openstack-dev-requ...@lists.openstack.org?subject:unsubscribe >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > > __ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo] [nova] [all] potential enginefacade adjustment - can everyone live with this?
On Fri, Jan 23, 2015, at 12:49 PM, Mike Bayer wrote: > > > Mike Bayer wrote: > > > > > > > Ihar Hrachyshka wrote: > > > >> On 01/23/2015 05:38 PM, Mike Bayer wrote: > >>> Doug Hellmann wrote: > >>> > We put the new base class for RequestContext in its own library because > both the logging and messaging code wanted to influence it's API. Would > it make sense to do this database setup there, too? > >>> whoa, where’s that? is this an oslo-wide RequestContext class ? that would > >>> solve everything b.c. right now every project seems to implement > >>> RequestContext themselves. > > > so Doug - > > How does this “influence of API” occur, would oslo.db import > oslo_context.context and patch onto RequestContext at that point? Or the > other way around? Or… ? No, it's a social thing. I didn't want dependencies between oslo.messaging and oslo.log, but the API of the context needs to support use cases in both places. Your case might be different, in that we might need to actually have oslo.context depend on oslo.db in order to call some setup code. We'll have to think about whether that makes sense and what other dependencies it might introduce between the existing users of oslo.context. Doug > > > I’m almost joyful that this is here. Assuming we can get everyone to > use it, should be straightforward for that right? > > > > __ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: > openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo] [nova] [all] potential enginefacade adjustment - can everyone live with this?
Mike Bayer wrote: > > > Ihar Hrachyshka wrote: > >> On 01/23/2015 05:38 PM, Mike Bayer wrote: >>> Doug Hellmann wrote: >>> We put the new base class for RequestContext in its own library because both the logging and messaging code wanted to influence it's API. Would it make sense to do this database setup there, too? >>> whoa, where’s that? is this an oslo-wide RequestContext class ? that would >>> solve everything b.c. right now every project seems to implement >>> RequestContext themselves. so Doug - How does this “influence of API” occur, would oslo.db import oslo_context.context and patch onto RequestContext at that point? Or the other way around? Or… ? I’m almost joyful that this is here. Assuming we can get everyone to use it, should be straightforward for that right? __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo] [nova] [all] potential enginefacade adjustment - can everyone live with this?
Ihar Hrachyshka wrote: > On 01/23/2015 05:38 PM, Mike Bayer wrote: >> Doug Hellmann wrote: >> >>> We put the new base class for RequestContext in its own library because >>> both the logging and messaging code wanted to influence it's API. Would >>> it make sense to do this database setup there, too? >> whoa, where’s that? is this an oslo-wide RequestContext class ? that would >> solve everything b.c. right now every project seems to implement >> RequestContext themselves. > > https://github.com/openstack/oslo.context/blob/master/oslo_context/context.py#L35 > > Though not every project migrated to it yet. WOW !! OK! Dear Openstack: Can you all start using oslo_context/context.py for your RequestContext base, as a condition of migrating off of legacy EngineFacade? __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo] [nova] [all] potential enginefacade adjustment - can everyone live with this?
On 01/23/2015 05:38 PM, Mike Bayer wrote: Doug Hellmann wrote: We put the new base class for RequestContext in its own library because both the logging and messaging code wanted to influence it's API. Would it make sense to do this database setup there, too? whoa, where’s that? is this an oslo-wide RequestContext class ? that would solve everything b.c. right now every project seems to implement RequestContext themselves. https://github.com/openstack/oslo.context/blob/master/oslo_context/context.py#L35 Though not every project migrated to it yet. /Ihar __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo] [nova] [all] potential enginefacade adjustment - can everyone live with this?
Doug Hellmann wrote: > We put the new base class for RequestContext in its own library because > both the logging and messaging code wanted to influence it's API. Would > it make sense to do this database setup there, too? whoa, where’s that? is this an oslo-wide RequestContext class ? that would solve everything b.c. right now every project seems to implement RequestContext themselves. __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo] [nova] [all] potential enginefacade adjustment - can everyone live with this?
On Thu, Jan 22, 2015, at 10:36 AM, Mike Bayer wrote: > 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) > > > +@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! We put the new base class for RequestContext in its own library because both the logging and messaging code wanted to influence it's API. Would it make sense to do this database setup there, too? Doug > > - mike > > > > > > > > > __ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: > openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
[openstack-dev] [oslo] [nova] [all] potential enginefacade adjustment - can everyone live with this?
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) +@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 __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
