Open Stack

On 21/11/18 5:35 PM, Sean Mooney wrote:
> On Wed, 2018-11-21 at 17:03 -0500, Zane Bitter wrote:
>> On 21/11/18 2:50 PM, Sean Mooney wrote:
>>> On Wed, 2018-11-21 at 14:38 -0500, Zane Bitter wrote:
>>>> On 21/11/18 9:47 AM, Herve Beraud wrote:
>>>>> # Questions and proposed solutions
>>>>>
>>>>> This thread try to summarize the current situation.
>>>>>
>>>>> We need to find how to be able to proceed, so this thread aim to allow
>>>>> to discuss between team to find the best way to fix.
>>>>>
>>>>> 1. Do we need to continue to try to backport fixture on oslo.service to
>>>>> fix the CI problem (https://review.openstack.org/#/c/617989/)?
>>
>> This is the worst option, because you won't be able to use either an
>> older nova with a newer oslo.service, nor an older oslo.service with a
>> newer nova.
>>
>> In fact, if I'm interpreting Matt's comment on
>> https://review.openstack.org/619246 correctly, then this may be a
>> non-starter because increasing lower-constraints is not allowed on
>> stable branches.
> you also cannot use a newer versions then are in upper-constaints
> this thread arose from the fact that performacne feature/bug fix form 1.32.x is being
> backported to 1.31.6. upper-constainst set a limit on oslo.service of 1.31.x so you are
> not expect to be able to use newer relases of oslo.service with the rocky release of nova.

Right, but the whole point of this discussion is that once we fix this 
we can bump the upper-constraints to 1.31.6 or 1.31.7.

Whereas apparently we *cannot* bump the lower-constraints.

>>>>> 2. Do we need to find an another approach like mocking
>>>>> oslo.service.loopingcall._Event.wait in nova instead of mocking
>>>>> oslo_service.loopingcall._ThreadingEvent.wait (example:
>>>>> https://review.openstack.org/#/c/616697/2/nova/tests/unit/compute/test_compute_mgr.py)?
>>
>> This is marginally better, provided that it's done in a
>> backwards-compatible way (i.e. that doesn't require bumping the
>> lower-constraints like https://review.openstack.org/619246 and
>> https://review.openstack.org/619022 do). Here's an example that should
>> do the trick:
>>
>> https://review.openstack.org/619360
>>
>> However, this does still mean that you can't use an older Nova with a
>> newer oslo.service. Which is bound to cause trouble for somebody.
>>
>> It also relies on a different private interface, although we're less
>> likely to need to change this one in stable/rocky.
>>
>>>> 3. Doesn't this get solved if we add a line like:
>>>>
>>>>      _ThreadingEvent = _Event
>>>>
>>>> in oslo.service on stable/rocky? That seems harmless and the easiest way
>>>> to maintain the same sort-of-public interface so nothing else ought to
>>>> break either. And with no change in Nova people won't need to worry
>>>> about needing to update oslo.service at the same time they update Nova
>>>> to avoid breakage.
>>>>
>>>> Here's a patch: https://review.openstack.org/619342
>>>
>>> a stable only patch is not really any better in my view then 2
>>
>> Surely being able to update oslo.service and nova independently is
>> objectively better than having to upgrade them in lock-step.
>>
>> Would it make you feel better if this patch were also on master? Why?
>>
>>> you are also chaning the loopingcall modules semantics
>>> as it is a different type even if you are allowing previously syntaxly
>>> vaild code to execute it does not maintain backwards compatiblity.
>>
>> This is a fair point; only the clear()/wait()/stop() methods are the
>> same. is_running() changes to is_set() and done() changes to set(). So
>> it is a bit hacky. But it still solves the instance of the problem we
>> know about (and FWIW any instance of the problem that *could*, in
>> principle, be solved by the fixture).
> if we go with this appoch and are doing a stable only change then
> instead of just doing _ThreadingEvent =_Event
> could we either use the debtcoltor moduel to make it as deprecated

Sadly, this won't work because if they're not referring to the same 
object then stubbing out the method won't have the desired effect.

> or stub out the signigure of the old interface and have it log a warning if invoked

Happy to do this.

> nova is only using this in test but if we are consusred about people upgrading then
> when we remove this workaourd at some point e.g. we shoudl at least warn other that might
> have done the same and depended on the ThredingEvents.

Well, we already removed it because it's long gone on master. So I doubt 
that adding deprecation warnings in a stable branch will catch any 
interesting cases that haven't been found already.

cheers,
Zane.