| amorin | Hey, I'l like some review on this: https://review.opendev.org/q/topic:%22eventlet-removal%22+project:openstack/mistral | 06:53 |
|---|---|---|
| *** ralonsoh_ is now known as ralonsoh | 09:21 | |
| hberaud[m] | ack | 13:14 |
| amorin | yes, this is not big changes, but still something that need double check and mistral team is kind of empty :) | 13:15 |
| hberaud[m] | although I'm not a mistral core. | 13:15 |
| hberaud[m] | in all the case thanks for your effort | 13:15 |
| amorin | I was wondering about the switch from eventlet to threading in oslo rpc server | 13:15 |
| amorin | is there any side effect of this change that you may know? | 13:16 |
| hberaud[m] | I guess it might have some performance impacts in some ways, as eventlet is based on cooperative coroutine, when, threading is threading,with preemptive behaviour etc... | 13:19 |
| hberaud[m] | but I do not expect much than that | 13:20 |
| amorin | ok, that's something we can accept | 13:20 |
| amorin | another question is about this one: https://review.opendev.org/c/openstack/mistral/+/937418 | 13:20 |
| amorin | we do rely on eventletutils | 13:20 |
| amorin | from oslo.utils | 13:21 |
| hberaud[m] | and in any case, this frees us from the eventlet so... | 13:21 |
| amorin | is it something we should get rid of? | 13:21 |
| hberaud[m] | lemme check | 13:21 |
| hberaud[m] | so | 13:22 |
| hberaud[m] | At first glance if there is no monkey patching on the mistral side, oslo.utils should not be an issue | 13:23 |
| hberaud[m] | if mistral is not monkey patched eventletutils.Event will just return threading.Event | 13:24 |
| hberaud[m] | else it will return Eventlet based Event | 13:24 |
| hberaud[m] | so coroutines | 13:24 |
| amorin | ack, it will depends then on fact that we monkey_patch first | 13:25 |
| hberaud[m] | we can keep this usage of oslo.utils while Eventlet is not fully removed from mistral | 13:25 |
| amorin | we do monkey_patch at some point, I'll have to make sure this is not the case here | 13:25 |
| amorin | is there any "new" way of writing this without eventletutils? | 13:26 |
| hberaud[m] | yes it fully depends on if the environment is patched or not | 13:26 |
| amorin | something that other project may already do? | 13:26 |
| hberaud[m] | writing this? What? | 13:26 |
| amorin | self._running = eventletutils.Event() | 13:27 |
| amorin | self._running = threading.Event() | 13:27 |
| amorin | maybe? | 13:27 |
| hberaud[m] | yes | 13:27 |
| hberaud[m] | but you have to drop the monkey patchs first | 13:28 |
| amorin | yes make sense | 13:28 |
| hberaud[m] | Else you will fall into something like https://greenlet.readthedocs.io/en/stable/python_threads.html | 13:28 |
| hberaud[m] | there is plenty of eventletutils here and where in our deliverables | 13:29 |
| hberaud[m] | oslo.messaging own its own eventletutils | 13:29 |
| hberaud[m] | it seems to me that nova did the same | 13:29 |
| hberaud[m] | and surely lot of other deliverables | 13:29 |
| hberaud[m] | and each implementation of this eventletutils is slightly different | 13:30 |
| hberaud[m] | s/here and where/here and there/ | 13:30 |
| amorin | another thing, don't know if frickler is there, about this one: https://review.opendev.org/c/openstack/requirements/+/933257 | 13:32 |
| amorin | even with last master branch of eventlet, mistral is still failing | 13:33 |
| hberaud[m] | yes frickler is into this channel | 13:33 |
| amorin | I have no idea how to achieve the monkey_patch earlier | 13:33 |
| amorin | unit test are executed by stestr, could it be an issue for monkey_patch? | 13:34 |
| hberaud[m] | maybe with something like https://github.com/openstack/oslo.messaging/blob/0deb6828683db276e7e3fadff0994b4fb04a347d/oslo_messaging/tests/__init__.py#L17 | 13:36 |
| hberaud[m] | init files are loaded first | 13:36 |
| hberaud[m] | I don't know how are monkey patched the mistral tests | 13:37 |
| amorin | https://opendev.org/openstack/mistral/src/branch/master/mistral/tests/unit/__init__.py | 13:37 |
| amorin | I do have this | 13:37 |
| amorin | maybe I should add it in the previous folder | 13:37 |
| hberaud[m] | yeah, I'd suggest to put it the sooner as possible during the tests loading | 13:38 |
| hberaud[m] | in oslo.messaging, you can observe that oslotest are even loaded after the monkey patching is applied https://github.com/openstack/oslo.messaging/blob/0deb6828683db276e7e3fadff0994b4fb04a347d/oslo_messaging/tests/__init__.py#L23C1-L23C16 | 13:39 |
| amorin | I can see that, thanks for this tip, I will try | 13:41 |
| hberaud[m] | no problem :) | 13:42 |
| hberaud[m] | The new version is out https://pypi.org/project/eventlet/0.38.1/ | 14:16 |
| itamarst | so I need to change oslo.log to run tests on newly released eventlet 0.38.1 (thanks hberaud[m]) but https://releases.openstack.org/constraints/upper/master is constrained to 0.36.1 | 14:59 |
| itamarst | how does the latter get upped? | 15:00 |
| hberaud[m] | itamarst: https://review.opendev.org/c/openstack/requirements/+/933257 | 15:00 |
| itamarst | cool | 15:00 |
| hberaud[m] | the problem is that the bump is for now not possible | 15:01 |
| hberaud[m] | because some cross-testing jobs are brokens | 15:01 |
| hberaud[m] | some deliverables needs adjustments first | 15:01 |
| itamarst | broken because of new eventlet? or unrelatedly | 15:01 |
| hberaud[m] | because of new eventelt | 15:01 |
| hberaud[m] | at least since 0.37.0 | 15:02 |
| itamarst | fun | 15:02 |
| itamarst | are there issues filed for that? | 15:02 |
| hberaud[m] | but people are working on it | 15:02 |
| itamarst | ah ok | 15:02 |
| itamarst | so just wait? | 15:02 |
| itamarst | I guess I can just submit the oslo.log patch and it will fail tests, but at least it will be in the queue? | 15:02 |
| itamarst | and there was a way to mark it as blocked on another, wasn't there... | 15:03 |
| hberaud[m] | there are links into the threads in the gerrit patch | 15:03 |
| hberaud[m] | if you want you can submit the oslo.log patch and put it at the top of the requirements bump | 15:03 |
| hberaud[m] | by using the Depends-on: https://review.opendev.org/c/openstack/requirements/+/933257 directive on the commit message | 15:04 |
| itamarst | thank you! | 15:04 |
| hberaud[m] | https://docs.openstack.org/project-team-guide/dependency-management.html#update-processes | 15:05 |
| hberaud[m] | you are welcome | 15:05 |
| hberaud[m] | itamarst: here is a living example https://review.opendev.org/c/openstack/oslo.db/+/922976 | 15:06 |
| hberaud[m] | although the requirements addition is now merged https://review.opendev.org/c/openstack/requirements/+/930947 | 15:07 |
| itamarst | now I just need to figure out why the tests started failing in the past 3 weeks | 15:08 |
| hberaud[m] | but you can do the same with the eventlet bump and acting like if the upper-constraints were already aligned with your needs | 15:08 |
| hberaud[m] | ah :) | 15:08 |
| hberaud[m] | the joys of randomness | 15:09 |
| itamarst | this is a "make pipemutex work with asyncio hub" patch | 15:09 |
| hberaud[m] | cool | 15:09 |
| hberaud[m] | that was one of our main pain point | 15:10 |
| itamarst | starting to get the impression the eventlet fix wasn't enough :/ this is the same error I was getting before that | 15:34 |
| itamarst | ah | 15:42 |
| itamarst | it's one of those "monkey patching too late" issues | 15:42 |
| frickler | oh, more failures with 0.38.1, including segfaults? | 15:54 |
| itamarst | frickler: are you asking me if that's what I'm seeing, or reporting something? | 15:54 |
| frickler | I was referring to the results on https://review.opendev.org/c/openstack/requirements/+/933257, but I'm also curious what you are seeing with oslo.log | 15:55 |
| itamarst | oh, I'm just seeing issues where in order to test monkey patching you ahve to monkey patch before asyncio ever gets imported, which is difficult | 15:56 |
| itamarst | so you get recursive failures on _threadlocal.hub lookups and the like | 15:56 |
| frickler | general question: is it safe to call eventlet.monkey_patch() multiple times? in neutron-server, it seems to get called again late during startup doing some module imports and it reports ungreened rlocks then, even though the initial monkey_patch call was run before any other import | 16:06 |
| itamarst | it _mostly_ doesn't duplicate logic | 16:08 |
| itamarst | er | 16:08 |
| itamarst | it _mostly_ notices duplicate runs and doesn't run logic twice | 16:08 |
| itamarst | except a few edge cases that are probably fine | 16:08 |
| itamarst | hberaud[m]: I have come up with a new mechanism to trigger monkey patching :/ | 17:45 |
| amorin | itamarst you want to propose a new wait to enable monkey patching? | 18:38 |
| hberaud[m] | itamarst: LGTM, I'll release tomorrow | 18:43 |
| hberaud[m] | it* | 18:43 |
| hberaud[m] | I need to dash | 18:43 |
| frickler | nice | 18:49 |
| itamarst | hberaud[m]: gonna open second PR next | 18:51 |
| itamarst | and it's up | 19:15 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!