Open Stack

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 <harlowja at 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 <cbehrens at codestud.com>
> Date: Thursday, April 17, 2014 at 4:59 PM
> To: Joshua Harlow <harlowja at yahoo-inc.com>
> Cc: Chris Behrens <cbehrens at codestud.com>, "OpenStack Development Mailing
> List (not for usage questions)" <openstack-dev at lists.openstack.org>
> Subject: Re: [openstack-dev] oslo removal of use_tpool conf option
>
>
> On Apr 17, 2014, at 4:26 PM, Joshua Harlow <harlowja at 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 <cbehrens at codestud.com>
> Reply-To: "OpenStack Development Mailing List (not for usage questions)"
> <openstack-dev at lists.openstack.org>
> Date: Thursday, April 17, 2014 at 3:20 PM
> To: OpenStack Development Mailing List <openstack-dev at 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 at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>