Re: [openstack-dev] oslo removal of use_tpool conf option

2014-04-18 Thread Roman Podoliaka
Hi all,

 I objected to this and asked (more demanded) for this to be added back into 
 oslo. It was not. What I did not realize when I was reviewing this nova 
 patch, was that nova had already synced oslo’s change. And now we’ve 
 released Icehouse with a conf option missing that existed in Havana. 
 Whatever projects were using oslo’s DB API code has had this option 
 disappear (unless an alternative was merged). Maybe it’s only nova.. I 
 don’t know.

First, I'm very sorry that Nova Icehouse release was cut with this
option missing. Whether it actually works or not, we should always
ensure we preserve backwards compatibility. I should have insisted on
making this sync from oslo-incubator 'atomic' in the first place, so
that tpool option was removed from openstack/common code and re-added
to Nova code in one commit, not two. So it's clearly my fault as a
reviewer who has made the original change to oslo-incubator.
Nevertheless, the patch re-adding this to Nova has been on review
since December the 3rd. Can we ensure it lands to master ASAP and will
be backported to icehouse?

On removing this option from oslo.db originally. As I've already
responded to your comment on review, I believe, oslo.db should neither
know, nor care if you use eventlet/gevent/OS threads/multiple
processes/callbacks/etc for handling concurrency. For the very same
reason SQLAlchemy doesn't do that. It just can't (and should not) make
such decisions for you. At the same time, eventlet provides a very
similar feature out-of-box. And
https://review.openstack.org/#/c/59760/  reuses it in Nova.

 unless you really want to live with DB calls blocking the whole process. I 
 know I don’t

Me neither. But the way we've been dealing with this in Nova and other
projects is having multiple workers processing those queries. I know,
it's not perfect, but it's what we default to (what folks use in
production mostly) and what we test. And, as we all know, something
that is untested, is broken. If eventlet tpool was a better option, I
believe, we would default to it. On the other hand, this seems to be a
fundamental issue of MySQLdb-python DB API driver. A pure python
driver (it would use more CPU time of course), as well as psycopg2
would work just fine. Probably, it's the MySQLdb-python we should fix,
rather than focusing on using of a work around provided by eventlet.

Once again, sorry for breaking things. Let's fix this and try not to
repeat the same mistakes in the future.

Thanks,
Roman

On Fri, Apr 18, 2014 at 4:42 AM, Joshua Harlow harlo...@yahoo-inc.com wrote:
 Thanks for the good explanation, was just a curiosity of mine.

 Any idea why it has taken so long for the eventlet folks to fix this (I know
 u proposed a patch/patches a while ago)? Is eventlet really that
 unmaintained? :(

 From: Chris Behrens cbehr...@codestud.com
 Date: Thursday, April 17, 2014 at 4:59 PM
 To: Joshua Harlow harlo...@yahoo-inc.com
 Cc: Chris Behrens cbehr...@codestud.com, OpenStack Development Mailing
 List (not for usage questions) openstack-dev@lists.openstack.org
 Subject: Re: [openstack-dev] oslo removal of use_tpool conf option


 On Apr 17, 2014, at 4:26 PM, Joshua Harlow harlo...@yahoo-inc.com wrote:

 Just an honest question (no negativity intended I swear!).

 If a configuration option exists and only works with a patched eventlet why
 is that option an option to begin with? (I understand the reason for the
 patch, don't get me wrong).


 Right, it’s a valid question. This feature has existed one way or another in
 nova for quite a while. Initially the implementation in nova was wrong. I
 did not know that eventlet was also broken at the time, although I
 discovered it in the process of fixing nova’s code. I chose to leave the
 feature because it’s something that we absolutely need long term, unless you
 really want to live with DB calls blocking the whole process. I know I
 don’t. Unfortunately the bug in eventlet is out of our control. (I made an
 attempt at fixing it, but it’s not 100%. Eventlet folks currently have an
 alternative up that may or may not work… but certainly is not in a release
 yet.)  We have an outstanding bug on our side to track this, also.

 The below is comparing apples/oranges for me.

 - Chris


 Most users would not be able to use such a configuration since they do not
 have this patched eventlet (I assume a newer version of eventlet someday in
 the future will have this patch integrated in it?) so although I understand
 the frustration around this I don't understand why it would be an option in
 the first place. An aside, if the only way to use this option is via a
 non-standard eventlet then how is this option tested in the community, aka
 outside of said company?

 An example:

 If yahoo has some patched kernel A that requires an XYZ config turned on in
 openstack and the only way to take advantage of kernel A is with XYZ config
 'on', then it seems like that’s a yahoo only patch that is not testable and
 useable

Re: [openstack-dev] oslo removal of use_tpool conf option

2014-04-17 Thread Michael Still
It looks to me like this was removed in oslo in commit
a33989e7a2737af757648099cc1af6c642b6e016, which was synced with nova
in 605749ca12af969ac122008b4fa14904df68caf7 (however, I can't see the
change being listed in the commit message for nova, which I assume is
a process failure). That change merged into nova on March 6.

I think the only option we're left with for icehouse is a backport fix for this.

Michael

On Fri, Apr 18, 2014 at 8:20 AM, Chris Behrens cbehr...@codestud.com wrote:

 I’m going to try to not lose my cool here, but I’m extremely upset by this.

 In December, oslo apparently removed the code for ‘use_tpool’ which allows
 you to run DB calls in Threads because it was ‘eventlet specific’. I noticed
 this when a review was posted to nova to add the option within nova itself:

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

 I objected to this and asked (more demanded) for this to be added back into
 oslo. It was not. What I did not realize when I was reviewing this nova
 patch, was that nova had already synced oslo’s change. And now we’ve
 released Icehouse with a conf option missing that existed in Havana.
 Whatever projects were using oslo’s DB API code has had this option
 disappear (unless an alternative was merged). Maybe it’s only nova.. I don’t
 know.

 Some sort of process broke down here.  nova uses oslo.  And oslo removed
 something nova uses without deprecating or merging an alternative into nova
 first. How I believe this should have worked:

 1) All projects using oslo’s DB API code should have merged an alternative
 first.
 2) Remove code from oslo.
 3) Then sync oslo.

 What do we do now? I guess we’ll have to back port the removed code into
 nova. I don’t know about other projects.

 NOTE: Very few people are probably using this, because it doesn’t work
 without a patched eventlet. However, Rackspace happens to be one that does.
 And anyone waiting on a new eventlet to be released such that they could use
 this with Icehouse is currently out of luck.

 - Chris



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




-- 
Rackspace Australia

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


Re: [openstack-dev] oslo removal of use_tpool conf option

2014-04-17 Thread Joshua Harlow
Just an honest question (no negativity intended I swear!).

If a configuration option exists and only works with a patched eventlet why is 
that option an option to begin with? (I understand the reason for the patch, 
don't get me wrong).

Most users would not be able to use such a configuration since they do not have 
this patched eventlet (I assume a newer version of eventlet someday in the 
future will have this patch integrated in it?) so although I understand the 
frustration around this I don't understand why it would be an option in the 
first place. An aside, if the only way to use this option is via a non-standard 
eventlet then how is this option tested in the community, aka outside of said 
company?

An example:

If yahoo has some patched kernel A that requires an XYZ config turned on in 
openstack and the only way to take advantage of kernel A is with XYZ config 
'on', then it seems like that’s a yahoo only patch that is not testable and 
useable for others, even if patched kernel A is somewhere on github it's still 
imho not something that should be a option in the community (anyone can throw 
stuff up on github and then say I need XYZ config to use it).

To me non-standard patches that require XYZ config in openstack shouldn't be 
part of the standard openstack, no matter the company. If patch A is in the 
mainline kernel (or other mainline library), then sure it's fair game.

-Josh

From: Chris Behrens cbehr...@codestud.commailto:cbehr...@codestud.com
Reply-To: OpenStack Development Mailing List (not for usage questions) 
openstack-dev@lists.openstack.orgmailto:openstack-dev@lists.openstack.org
Date: Thursday, April 17, 2014 at 3:20 PM
To: OpenStack Development Mailing List 
openstack-dev@lists.openstack.orgmailto:openstack-dev@lists.openstack.org
Subject: [openstack-dev] oslo removal of use_tpool conf option


I’m going to try to not lose my cool here, but I’m extremely upset by this.

In December, oslo apparently removed the code for ‘use_tpool’ which allows you 
to run DB calls in Threads because it was ‘eventlet specific’. I noticed this 
when a review was posted to nova to add the option within nova itself:

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

I objected to this and asked (more demanded) for this to be added back into 
oslo. It was not. What I did not realize when I was reviewing this nova patch, 
was that nova had already synced oslo’s change. And now we’ve released Icehouse 
with a conf option missing that existed in Havana. Whatever projects were using 
oslo’s DB API code has had this option disappear (unless an alternative was 
merged). Maybe it’s only nova.. I don’t know.

Some sort of process broke down here.  nova uses oslo.  And oslo removed 
something nova uses without deprecating or merging an alternative into nova 
first. How I believe this should have worked:

1) All projects using oslo’s DB API code should have merged an alternative 
first.
2) Remove code from oslo.
3) Then sync oslo.

What do we do now? I guess we’ll have to back port the removed code into nova. 
I don’t know about other projects.

NOTE: Very few people are probably using this, because it doesn’t work without 
a patched eventlet. However, Rackspace happens to be one that does. And anyone 
waiting on a new eventlet to be released such that they could use this with 
Icehouse is currently out of luck.

- Chris


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


Re: [openstack-dev] oslo removal of use_tpool conf option

2014-04-17 Thread Chris Behrens

On Apr 17, 2014, at 4:26 PM, Joshua Harlow harlo...@yahoo-inc.com wrote:

 Just an honest question (no negativity intended I swear!).
 
 If a configuration option exists and only works with a patched eventlet why 
 is that option an option to begin with? (I understand the reason for the 
 patch, don't get me wrong).
 

Right, it’s a valid question. This feature has existed one way or another in 
nova for quite a while. Initially the implementation in nova was wrong. I did 
not know that eventlet was also broken at the time, although I discovered it in 
the process of fixing nova’s code. I chose to leave the feature because it’s 
something that we absolutely need long term, unless you really want to live 
with DB calls blocking the whole process. I know I don’t. Unfortunately the bug 
in eventlet is out of our control. (I made an attempt at fixing it, but it’s 
not 100%. Eventlet folks currently have an alternative up that may or may not 
work… but certainly is not in a release yet.)  We have an outstanding bug on 
our side to track this, also.

The below is comparing apples/oranges for me.

- Chris


 Most users would not be able to use such a configuration since they do not 
 have this patched eventlet (I assume a newer version of eventlet someday in 
 the future will have this patch integrated in it?) so although I understand 
 the frustration around this I don't understand why it would be an option in 
 the first place. An aside, if the only way to use this option is via a 
 non-standard eventlet then how is this option tested in the community, aka 
 outside of said company?
 
 An example:
 
 If yahoo has some patched kernel A that requires an XYZ config turned on in 
 openstack and the only way to take advantage of kernel A is with XYZ config 
 'on', then it seems like that’s a yahoo only patch that is not testable and 
 useable for others, even if patched kernel A is somewhere on github it's 
 still imho not something that should be a option in the community (anyone can 
 throw stuff up on github and then say I need XYZ config to use it).
 
 To me non-standard patches that require XYZ config in openstack shouldn't be 
 part of the standard openstack, no matter the company. If patch A is in the 
 mainline kernel (or other mainline library), then sure it's fair game.
 
 -Josh
 
 From: Chris Behrens cbehr...@codestud.com
 Reply-To: OpenStack Development Mailing List (not for usage questions) 
 openstack-dev@lists.openstack.org
 Date: Thursday, April 17, 2014 at 3:20 PM
 To: OpenStack Development Mailing List openstack-dev@lists.openstack.org
 Subject: [openstack-dev] oslo removal of use_tpool conf option
 
 
 I’m going to try to not lose my cool here, but I’m extremely upset by this.
 
 In December, oslo apparently removed the code for ‘use_tpool’ which allows 
 you to run DB calls in Threads because it was ‘eventlet specific’. I noticed 
 this when a review was posted to nova to add the option within nova itself:
 
 https://review.openstack.org/#/c/59760/
 
 I objected to this and asked (more demanded) for this to be added back into 
 oslo. It was not. What I did not realize when I was reviewing this nova 
 patch, was that nova had already synced oslo’s change. And now we’ve 
 released Icehouse with a conf option missing that existed in Havana. 
 Whatever projects were using oslo’s DB API code has had this option 
 disappear (unless an alternative was merged). Maybe it’s only nova.. I don’t 
 know.
 
 Some sort of process broke down here.  nova uses oslo.  And oslo removed 
 something nova uses without deprecating or merging an alternative into nova 
 first. How I believe this should have worked:
 
 1) All projects using oslo’s DB API code should have merged an alternative 
 first.
 2) Remove code from oslo.
 3) Then sync oslo.
 
 What do we do now? I guess we’ll have to back port the removed code into 
 nova. I don’t know about other projects.
 
 NOTE: Very few people are probably using this, because it doesn’t work 
 without a patched eventlet. However, Rackspace happens to be one that does. 
 And anyone waiting on a new eventlet to be released such that they could use 
 this with Icehouse is currently out of luck.
 
 - Chris
 
 

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


Re: [openstack-dev] oslo removal of use_tpool conf option

2014-04-17 Thread Davanum Srinivas
I do agree with Chris that process-wise, there was a failure and we
need a way to ensure that it does not happen again. (There was a
config option lost in the shuffle w/o a deprecation period)

On Thu, Apr 17, 2014 at 7:59 PM, Chris Behrens cbehr...@codestud.com wrote:

 On Apr 17, 2014, at 4:26 PM, Joshua Harlow harlo...@yahoo-inc.com wrote:

 Just an honest question (no negativity intended I swear!).

 If a configuration option exists and only works with a patched eventlet why
 is that option an option to begin with? (I understand the reason for the
 patch, don't get me wrong).


 Right, it’s a valid question. This feature has existed one way or another in
 nova for quite a while. Initially the implementation in nova was wrong. I
 did not know that eventlet was also broken at the time, although I
 discovered it in the process of fixing nova’s code. I chose to leave the
 feature because it’s something that we absolutely need long term, unless you
 really want to live with DB calls blocking the whole process. I know I
 don’t. Unfortunately the bug in eventlet is out of our control. (I made an
 attempt at fixing it, but it’s not 100%. Eventlet folks currently have an
 alternative up that may or may not work… but certainly is not in a release
 yet.)  We have an outstanding bug on our side to track this, also.

 The below is comparing apples/oranges for me.

 - Chris


 Most users would not be able to use such a configuration since they do not
 have this patched eventlet (I assume a newer version of eventlet someday in
 the future will have this patch integrated in it?) so although I understand
 the frustration around this I don't understand why it would be an option in
 the first place. An aside, if the only way to use this option is via a
 non-standard eventlet then how is this option tested in the community, aka
 outside of said company?

 An example:

 If yahoo has some patched kernel A that requires an XYZ config turned on in
 openstack and the only way to take advantage of kernel A is with XYZ config
 'on', then it seems like that’s a yahoo only patch that is not testable and
 useable for others, even if patched kernel A is somewhere on github it's
 still imho not something that should be a option in the community (anyone
 can throw stuff up on github and then say I need XYZ config to use it).

 To me non-standard patches that require XYZ config in openstack shouldn't be
 part of the standard openstack, no matter the company. If patch A is in the
 mainline kernel (or other mainline library), then sure it's fair game.

 -Josh

 From: Chris Behrens cbehr...@codestud.com
 Reply-To: OpenStack Development Mailing List (not for usage questions)
 openstack-dev@lists.openstack.org
 Date: Thursday, April 17, 2014 at 3:20 PM
 To: OpenStack Development Mailing List openstack-dev@lists.openstack.org
 Subject: [openstack-dev] oslo removal of use_tpool conf option


 I’m going to try to not lose my cool here, but I’m extremely upset by this.

 In December, oslo apparently removed the code for ‘use_tpool’ which allows
 you to run DB calls in Threads because it was ‘eventlet specific’. I noticed
 this when a review was posted to nova to add the option within nova itself:

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

 I objected to this and asked (more demanded) for this to be added back into
 oslo. It was not. What I did not realize when I was reviewing this nova
 patch, was that nova had already synced oslo’s change. And now we’ve
 released Icehouse with a conf option missing that existed in Havana.
 Whatever projects were using oslo’s DB API code has had this option
 disappear (unless an alternative was merged). Maybe it’s only nova.. I don’t
 know.

 Some sort of process broke down here.  nova uses oslo.  And oslo removed
 something nova uses without deprecating or merging an alternative into nova
 first. How I believe this should have worked:

 1) All projects using oslo’s DB API code should have merged an alternative
 first.
 2) Remove code from oslo.
 3) Then sync oslo.

 What do we do now? I guess we’ll have to back port the removed code into
 nova. I don’t know about other projects.

 NOTE: Very few people are probably using this, because it doesn’t work
 without a patched eventlet. However, Rackspace happens to be one that does.
 And anyone waiting on a new eventlet to be released such that they could use
 this with Icehouse is currently out of luck.

 - Chris




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




-- 
Davanum Srinivas :: http://davanum.wordpress.com

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


Re: [openstack-dev] oslo removal of use_tpool conf option

2014-04-17 Thread Joshua Harlow
Thanks for the good explanation, was just a curiosity of mine.

Any idea why it has taken so long for the eventlet folks to fix this (I know u 
proposed a patch/patches a while ago)? Is eventlet really that unmaintained? :(

From: Chris Behrens cbehr...@codestud.commailto:cbehr...@codestud.com
Date: Thursday, April 17, 2014 at 4:59 PM
To: Joshua Harlow harlo...@yahoo-inc.commailto:harlo...@yahoo-inc.com
Cc: Chris Behrens cbehr...@codestud.commailto:cbehr...@codestud.com, 
OpenStack Development Mailing List (not for usage questions) 
openstack-dev@lists.openstack.orgmailto:openstack-dev@lists.openstack.org
Subject: Re: [openstack-dev] oslo removal of use_tpool conf option


On Apr 17, 2014, at 4:26 PM, Joshua Harlow 
harlo...@yahoo-inc.commailto:harlo...@yahoo-inc.com wrote:

Just an honest question (no negativity intended I swear!).

If a configuration option exists and only works with a patched eventlet why is 
that option an option to begin with? (I understand the reason for the patch, 
don't get me wrong).


Right, it’s a valid question. This feature has existed one way or another in 
nova for quite a while. Initially the implementation in nova was wrong. I did 
not know that eventlet was also broken at the time, although I discovered it in 
the process of fixing nova’s code. I chose to leave the feature because it’s 
something that we absolutely need long term, unless you really want to live 
with DB calls blocking the whole process. I know I don’t. Unfortunately the bug 
in eventlet is out of our control. (I made an attempt at fixing it, but it’s 
not 100%. Eventlet folks currently have an alternative up that may or may not 
work… but certainly is not in a release yet.)  We have an outstanding bug on 
our side to track this, also.

The below is comparing apples/oranges for me.

- Chris


Most users would not be able to use such a configuration since they do not have 
this patched eventlet (I assume a newer version of eventlet someday in the 
future will have this patch integrated in it?) so although I understand the 
frustration around this I don't understand why it would be an option in the 
first place. An aside, if the only way to use this option is via a non-standard 
eventlet then how is this option tested in the community, aka outside of said 
company?

An example:

If yahoo has some patched kernel A that requires an XYZ config turned on in 
openstack and the only way to take advantage of kernel A is with XYZ config 
'on', then it seems like that’s a yahoo only patch that is not testable and 
useable for others, even if patched kernel A is somewhere on github it's still 
imho not something that should be a option in the community (anyone can throw 
stuff up on github and then say I need XYZ config to use it).

To me non-standard patches that require XYZ config in openstack shouldn't be 
part of the standard openstack, no matter the company. If patch A is in the 
mainline kernel (or other mainline library), then sure it's fair game.

-Josh

From: Chris Behrens cbehr...@codestud.commailto:cbehr...@codestud.com
Reply-To: OpenStack Development Mailing List (not for usage questions) 
openstack-dev@lists.openstack.orgmailto:openstack-dev@lists.openstack.org
Date: Thursday, April 17, 2014 at 3:20 PM
To: OpenStack Development Mailing List 
openstack-dev@lists.openstack.orgmailto:openstack-dev@lists.openstack.org
Subject: [openstack-dev] oslo removal of use_tpool conf option


I’m going to try to not lose my cool here, but I’m extremely upset by this.

In December, oslo apparently removed the code for ‘use_tpool’ which allows you 
to run DB calls in Threads because it was ‘eventlet specific’. I noticed this 
when a review was posted to nova to add the option within nova itself:

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

I objected to this and asked (more demanded) for this to be added back into 
oslo. It was not. What I did not realize when I was reviewing this nova patch, 
was that nova had already synced oslo’s change. And now we’ve released Icehouse 
with a conf option missing that existed in Havana. Whatever projects were using 
oslo’s DB API code has had this option disappear (unless an alternative was 
merged). Maybe it’s only nova.. I don’t know.

Some sort of process broke down here.  nova uses oslo.  And oslo removed 
something nova uses without deprecating or merging an alternative into nova 
first. How I believe this should have worked:

1) All projects using oslo’s DB API code should have merged an alternative 
first.
2) Remove code from oslo.
3) Then sync oslo.

What do we do now? I guess we’ll have to back port the removed code into nova. 
I don’t know about other projects.

NOTE: Very few people are probably using this, because it doesn’t work without 
a patched eventlet. However, Rackspace happens to be one that does. And anyone 
waiting on a new eventlet to be released such that they could use this with 
Icehouse is currently out of luck.

- Chris