Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-09-02
16:50:29 sean-k-mooney im wondering if my patch woudl fix the issue basically
16:50:51 gibi based on the last comment in the eventlet issue we cannot simply replace spawn_n with spawn as eventlet calls spawn_n internall too
16:51:15 gibi https://github.com/eventlet/eventlet/issues/731#issuecomment-969891721
16:52:36 gibi https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/thread.py#L72
16:53:24 sean-k-mooney https://github.com/eventlet/eventlet/issues/731#issuecomment-968135262 said we shoudl be able too
16:53:33 sean-k-mooney and when i did it it did not break anything
16:54:05 sean-k-mooney i know that melwitt noted a delta in some fo the behaivor
16:54:29 sean-k-mooney but i dont think that caused any issues for our usage
16:56:01 gibi we dont have test coverage to detect the break, we have this broken locking for the last 7 month I think
16:56:34 gibi so we actually don't know if replaceing spawn_n with spawn in nova and keeping spawn_n internally is enough
16:56:41 sean-k-mooney well it passed tempest and our func/unit test with the replacment
16:56:47 gibi we are passing tempest today
16:56:50 gibi with the broken lock
16:56:53 sean-k-mooney yep
16:57:09 sean-k-mooney but what im saying is that if that fixes your repoducer
16:57:10 gibi so we have no information if the replacement actually fixed the problem or not
16:57:14 sean-k-mooney then it likely will fix the issue
16:57:46 sean-k-mooney doing the repacement the same way in yoru standalone repoducer does not help?
16:58:05 gibi it does but my reproducer does not use the Threading.thread way the last comment in the issue mentions
16:58:20 sean-k-mooney right but does the lock
16:58:21 gibi and we know from melwitt that someting under nova uses Threading.thread
16:58:34 sean-k-mooney that should not matter by the way
16:58:55 sean-k-mooney even if it deoes the global replacment should mean that provided that call did not happen before we monkey patched
16:59:05 sean-k-mooney it should get our replaced version
16:59:09 gibi nope
16:59:19 gibi the internall call does greenlet.spawn_n
16:59:39 gibi I think that is not even replaceble as it is a c extension
16:59:41 sean-k-mooney oh well we can patch that too
16:59:51 sean-k-mooney oh
16:59:56 sean-k-mooney if its c then no
17:00:15 sean-k-mooney but we dont uses threading.thread normally
17:00:35 sean-k-mooney so while it might not fix every case it might fix the case we care about
17:00:42 gibi melwitt found someting that uses
17:00:49 sean-k-mooney teh libvirt thread
17:01:01 gibi what the rpc worker use?
17:01:23 gibi how we spawn the rpc workers listening on rabbit?
17:01:28 sean-k-mooney whell there are only two thread i can think of that might use it
17:01:44 sean-k-mooney the libvirt one and the heathbeat
17:01:51 sean-k-mooney if we use a pthred
17:02:06 sean-k-mooney im not sure we are using one for rpc explictly but we might be
17:02:18 gibi note that threading.thread issue path is problematic when it is monkey patched
17:02:43 gibi so when threading.thread actually patched to create an eventlet
17:03:07 gibi I can try tracing our rpc eventlet to see if it is created from spawn_n or spawn
17:03:25 gibi I guess it is coming from oslo.messaging somehow
17:03:59 sean-k-mooney well we use it in a few places
17:04:31 sean-k-mooney https://github.com/openstack/nova/blob/18d9c85aa4cbdbc471c6c7916ca6f1367c7ab4e5/nova/virt/libvirt/host.py#L491
17:04:49 sean-k-mooney https://github.com/openstack/nova/blob/18d9c85aa4cbdbc471c6c7916ca6f1367c7ab4e5/nova/virt/hyperv/serialproxy.py#L101
17:05:08 sean-k-mooney i was going to use it for the healthchecks
17:05:20 gibi native threading is OK that is real python Thread
17:05:49 gibi https://github.com/openstack/nova/blob/18d9c85aa4cbdbc471c6c7916ca6f1367c7ab4e5/nova/virt/hyperv/serialproxy.py#L101 <-- this can be a problem though
17:06:07 gibi hm it is native too https://github.com/openstack/nova/blob/18d9c85aa4cbdbc471c6c7916ca6f1367c7ab4e5/nova/virt/hyperv/serialproxy.py#L32
17:06:31 gibi so this two places creates a real python thread that is probably OK from this issue perspective
17:07:32 sean-k-mooney so all other usages today are in libs
17:08:05 sean-k-mooney oslo.messaging, ovsdbapp (via os-vif), os-brick, and proably oslo.db
17:08:08 sean-k-mooney if i was to guess
17:08:37 sean-k-mooney https://github.com/openstack/os-brick/blob/4acfd6bc7e7e816ce8e9d5ac59cfc0f6e5e816f4/os_brick/executor.py#L71
17:08:48 gibi the rabbit driver has threading but I'm not sure if that is always native https://github.com/openstack/oslo.messaging/blob/e44f286ebca0fbde5eae2f7eb9a21ba55ba2a549/oslo_messaging/_drivers/impl_rabbit.py
17:09:13 sean-k-mooney gibi: its not we monkey patch before we import it
17:09:32 gibi OK so it might or might not native
17:09:33 gibi https://github.com/openstack/oslo.messaging/blob/e44f286ebca0fbde5eae2f7eb9a21ba55ba2a549/oslo_messaging/_drivers/impl_rabbit.py#L607
17:09:59 sean-k-mooney its sill be a GreenThread unless we disable patching threads
17:10:23 sean-k-mooney we currently mokeypatch every nova service
17:10:55 sean-k-mooney and that will happen very early
17:11:15 gibi OK, so if rabbit driver uses threading and it is monkeypatched then our rpc worker spawned by spawn_n and effect by the bug
17:11:22 gibi even if we replace eventlet.spawn_n
17:11:46 gibi I cannot do a reproduction in the functional as it does not use the rabbit driver
17:11:55 gibi so it won't be indicative
17:12:34 gibi doing a tempest reproducer would be a pita as the reproducer needs timing
17:12:53 sean-k-mooney we will need to re MonkeyPatch threading.Thread then
17:13:16 sean-k-mooney to make it not call greenlet.spwan_n
17:13:32 sean-k-mooney or change how ew do locking
17:13:46 sean-k-mooney do you knwo how/why it migh not be locking correctly
17:13:50 gibi or we need to do this globally https://review.opendev.org/c/openstack/nova/+/813114/4/nova/tests/fixtures/nova.py#425
17:14:07 gibi fastener changed how it get the current thread
17:14:34 gibi and getting it from a thread created by spawn_n actaully returns the native thread name not the eventlet id
17:14:54 gibi therefore fasteners sees to different eventelt as same
17:15:00 gibi and allow reentry to the lock
17:15:02 sean-k-mooney well that we can do
17:15:37 sean-k-mooney you should be able to test that in your repoducer
17:15:38 gibi the downside of the globlack monekeypatch that it might effect the native threads
17:16:05 sean-k-mooney that can un monky patch it
17:16:28 sean-k-mooney we can save the reference to the ortinal fucniton globally
17:16:48 sean-k-mooney adn restore it with a context manager when launching the native thread
17:17:01 gibi basically all the native thread user in our under nova needs to be aware of it
17:17:12 gibi *in or
17:17:28 gibi we can sure fix nova native threads
17:17:46 gibi I'm not sure we can fix native threading in all libs used by nova
17:17:53 sean-k-mooney well ok
17:17:58 sean-k-mooney we dont need to do it globally
17:18:02 sean-k-mooney we just need the lock to use it
17:18:18 gibi so we wrap the lock with a local mocking
17:18:18 sean-k-mooney so we only need to do that for oslo
17:18:26 sean-k-mooney basically
17:18:29 sean-k-mooney yes
17:18:33 gibi if we not pass the lock to libs then that could work
17:18:42 sean-k-mooney i dont think we do
17:18:47 gibi yeah
17:18:53 gibi so backtrack a bit
17:19:10 sean-k-mooney we also alrady have nova util fucntion for locks already
17:20:00 gibi how can we test it in a way that i) uses the real things like rabbit driver ii)

Earlier   Later