Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-09-02
16:38:47 gibi https://bugs.launchpad.net/nova/+bug/1988311/comments/3
16:38:53 gibi yepp
16:39:36 sean-k-mooney are we context switch between the two coroutines inside the critical section under the lock?
16:39:58 sean-k-mooney and there for data racing?
16:40:36 gibi the bug is written as two pinned VM evacuated and ended up selecting overlapping cpus
16:41:13 sean-k-mooney yep so unlike pci devices we dont enforce that in the db
16:41:28 sean-k-mooney the only protection we have is the rt lock
16:41:41 sean-k-mooney to ensure we claim the cpus and update the host numa toplogy blob
16:42:05 sean-k-mooney then we regrenrate that over time based on the instance numa toplogy blob in the perodic
16:42:28 sean-k-mooney so if this lock is broken its very posible for that to break and not be able to fix itslef
16:43:00 sean-k-mooney it need to not only prorect against the concurrent evacuate btu also the preiodic running
16:43:44 gibi yes
16:43:56 gibi and we have the rt lock around many actions
16:47:30 sean-k-mooney have you treid https://review.opendev.org/c/openstack/nova/+/842359/5/nova/monkey_patch.py
16:47:57 sean-k-mooney eventlet.spawn_n = eventlet.spawn
16:47:59 sean-k-mooney try:
16:48:01 sean-k-mooney import eventlet.convenient
16:48:03 sean-k-mooney eventlet.convenient.spawn_n = eventlet.spawn
16:48:05 sean-k-mooney except ImportError:
16:48:07 sean-k-mooney pass
16:48:10 sean-k-mooney to see if that fixes the reproducer
16:49:18 gibi If I change eventlet.spawn_n to eventlet.spawn in https://gist.github.com/gibizer/9051369e67fd46a20d52963dac534852 the the locking works
16:49:39 gibi melwitt has a very good thread in https://github.com/eventlet/eventlet/issues/731 about the whole picture
16:49:40 sean-k-mooney right but does chanign it like that work
16:49:53 sean-k-mooney since that is how we tried to do that in nova
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

Earlier   Later