Re: [openstack-dev] oslo removal of use_tpool conf option
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
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
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
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
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
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