Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-09-02
16:34:25 gibi here is a minimal reproductionhttps://gist.github.com/gibizer/9051369e67fd46a20d52963dac534852
16:34:28 gibi https://gist.github.com/gibizer/9051369e67fd46a20d52963dac534852
16:35:32 gibi the realization came when I looked at the logs in https://bugs.launchpad.net/nova/+bug/1988311//
16:35:56 gibi those logs shows that two rebuild_claim can take the same lock twice
16:36:37 sean-k-mooney isnt it an instantace lock
16:36:42 sean-k-mooney or is it the rt lock
16:37:05 sean-k-mooney it must be the rt lock actully
16:37:18 gibi it is the rt lock
16:37:25 gibi https://github.com/openstack/nova/blob/8b55b44cc605533f2a12189a2b5899c0f58c91a7/nova/compute/resource_tracker.py#L201-L202
16:38:14 sean-k-mooney i havent looked in deail but the lock name would be the same
16:38:29 sean-k-mooney i assuem you are loking at someting in the outpu specificaly
16:38:31 gibi yes it is compute_resources
16:38:47 sean-k-mooney that shows they are taking the same lock wtich
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

Earlier   Later