Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-09-02
13:34:49 sean-k-mooney i think we can simplfy the code and basicaly alway have a count of 1
13:35:10 gibi yeah , if we can split, then the count filed become unused
13:35:37 sean-k-mooney i think thats ok
13:35:39 gibi and some logic where we loop on request and the loop on count can be refactored to a single loop
13:35:54 sean-k-mooney we can drop it in a future version if we rev the object major version
13:36:52 gibi the only thing where we have to be careful is code that might did some affinity or block based decision based on a single request with count > 1
13:37:11 gibi instead of doing it device by device
13:37:21 gibi the filter_pools code need to be checked
13:37:51 sean-k-mooney gibi: well it was ment to be per device today
13:38:00 sean-k-mooney so if it was per alias that would be a bug
13:38:16 sean-k-mooney i think you noted that the request could be fullfiled form differnt pools already
13:38:30 sean-k-mooney so hopefuly that all works
13:38:42 sean-k-mooney but soudn like more functional test can prove that one way or another
13:40:11 gibi yeah, I'm not afraid of the actual consumption part as that already needs to work per device as we can have multiple RP uuid per PCIreq in the current series
13:40:26 gibi this was my last fix btw ^^
13:41:02 sean-k-mooney ah ok
13:41:10 sean-k-mooney that was the bug
13:41:12 frickler gibi: sean-k-mooney: prettytable 3.4.1 was just releases which reverts the broken change. nova tests work fine for me with that locally. does one of you want to propose the exclude in reqs?
13:41:42 gibi frickler: if you already at it then feel free to propose the exclude
13:41:43 sean-k-mooney frickler: should we proceed with hardeing the nova code anyway
13:42:31 sean-k-mooney my patch works with 3.4.1 too it seams
13:43:05 frickler can't hurt to be on the safe side, then, I'd say
13:43:56 sean-k-mooney ok ill keep it open and backport it to yoga so
13:44:26 sean-k-mooney at least that way if a disto hits this issue or they make the chagne in 4.0 we will be fine
13:48:18 frickler now I only need to find out the path it get's pulled in, since it doesn't seem to be a direct dependency
13:52:58 sean-k-mooney frickler: PrettyTable ?
13:53:07 sean-k-mooney nova depend on it directly but i guess we might not list it
13:53:34 sean-k-mooney https://github.com/openstack/nova/blob/master/requirements.txt#L18
13:53:38 sean-k-mooney its there
13:53:57 bauzas sean-k-mooney: I checked all the libs and we don't need a new release
13:54:01 bauzas did I miss one ?
13:54:35 sean-k-mooney we may want anotther release of python-novaclint
13:54:37 sean-k-mooney brb
13:54:45 sean-k-mooney if we merge some pending patches
14:02:20 opendevreview Slawek Kaplonski proposed openstack/nova master: WIP Don't provide MTU value in metadata service if DHCP is enabled https://review.opendev.org/c/openstack/nova/+/855664
14:04:39 frickler sean-k-mooney: ah, CaseMismatch in my search, thx
14:08:38 sean-k-mooney gibi: i may hae missed something but https://review.opendev.org/c/openstack/nova-specs/+/855218 looks good over all a cople of nits inline
14:08:49 sean-k-mooney im gong to read it again quickly
14:09:32 gibi thanks I will fix the nits a bit later todayt
14:10:49 bauzas fwiw, I'm not using the -2 hammer yet
14:10:56 bauzas for all the open changes
14:11:13 bauzas I'll do it only on Tuesday
14:13:50 sean-k-mooney unless there is anything urgnet im going to take a break form lookign at upstream reviews and work on some automation
14:15:41 bauzas => goes getting his kid from school
16:34:08 gibi I think lockutils.synchronized(...fair=True) + eventlet.spawn_n() + fastener > 0.15 actually breaks nova proper. Not just the test code that we fixed in https://review.opendev.org/c/openstack/nova/+/813114
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 gibi https://bugs.launchpad.net/nova/+bug/1988311/comments/3
16:38:47 sean-k-mooney that shows they are taking the same lock wtich
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

Earlier   Later