| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-01-14 | |||
| 17:01:37 | tkajinam | ok so I'd ask MengyangZhang[m] to add quick description and probably propose actual change, and then we can make the decision in upcoming weeks | |
| 17:01:49 | tkajinam | based on these | |
| 17:03:45 | bauzas | yeah, that'd be nice | |
| 17:03:57 | bauzas | I don't want to rush them, but the sooner the better | |
| 17:04:05 | dansmith | sean-k-mooney: melwitt: kinda surprised to see this merge with not a line of test coverage: https://review.opendev.org/c/openstack/nova/+/932669 | |
| 17:04:25 | dansmith | especially since it looks like it's getting backported all the way | |
| 17:04:39 | tkajinam | bauzas, yeah. we don't have very long time left for this cycle so we may need timely update to get that in for E | |
| 17:04:47 | dansmith | especially given the backports, seems like coverage would be good no? | |
| 17:04:54 | tkajinam | bauzas sean-k-mooney, thank you, both ! | |
| 17:04:55 | sean-k-mooney | dansmith: i guess that a good point i was not really sure how to assert it was a proxy object | |
| 17:05:12 | sean-k-mooney | dansmith: i wasnt sure that an issubclass check woudl work but perhasp it could | |
| 17:05:26 | sean-k-mooney | dansmith: do you know if we have any test coverage to that effect? | |
| 17:05:39 | dansmith | sean-k-mooney: I think this is not special in that regard | |
| 17:06:19 | dansmith | I mean mocking the wrap method and asserting it was called would be something at least | |
| 17:06:41 | sean-k-mooney | oh mocking _wrap_libvirt_proxy | |
| 17:06:48 | sean-k-mooney | ya that could also work | |
| 17:06:53 | bauzas | sean-k-mooney: we have some example https://review.opendev.org/c/openstack/nova/+/677736/5/nova/tests/unit/virt/libvirt/test_host.py | |
| 17:06:59 | bauzas | or a Mock | |
| 17:07:11 | bauzas | anyway, sounds to me achievable | |
| 17:07:12 | sean-k-mooney | melwitt: ^ is that something you can do a follow up for | |
| 17:07:25 | bauzas | not testing the threads themselves | |
| 17:07:26 | dansmith | well, follow up won't get into the backport | |
| 17:07:34 | sean-k-mooney | dansmith: your right that it proably shoudl have been included in the initall patch | |
| 17:07:48 | dansmith | might be cleaner to revert and re-do it with tests... we could backport tests, but it's messy-er | |
| 17:08:07 | sean-k-mooney | the first backport does not look like it has merged yet | |
| 17:08:11 | sean-k-mooney | so that still an option | |
| 17:09:57 | sean-k-mooney | i guess the refence patch that mel refers to has some examples we can reuse https://review.opendev.org/c/openstack/nova/+/677736/5/nova/tests/unit/virt/libvirt/test_host.py | |
| 17:11:01 | dansmith | sean-k-mooney: so will you propose a revert on master? | |
| 17:11:06 | sean-k-mooney | its kind of a wackamole probelem, it would be nice if we had some way to poison the module in test so that it could not return non wapped obejcts | |
| 17:11:16 | sean-k-mooney | dansmith: sure i can | |
| 17:11:26 | dansmith | thanks | |
| 17:11:36 | dansmith | I'll stand by to fast approve :) | |
| 17:12:02 | opendevreview | sean mooney proposed openstack/nova master: Revert "libvirt: Wrap un-proxied listDevices() and listAllDevices()" https://review.opendev.org/c/openstack/nova/+/939262 | |
| 17:12:42 | opendevreview | sean mooney proposed openstack/nova master: Revert "libvirt: Wrap un-proxied listDevices() and listAllDevices()" https://review.opendev.org/c/openstack/nova/+/939262 | |
| 17:12:50 | sean-k-mooney | just fixed a typo in the commit | |
| 17:13:08 | opendevreview | ribaudr proposed openstack/os-traits master: Add 'HW_PCI_LIVE_MIGRATABLE' https://review.opendev.org/c/openstack/os-traits/+/939264 | |
| 17:17:53 | sean-k-mooney | dansmith: thinking about it, i guess the tpool.proxy is a "contains" or "has a" relationship rahter then an inheritance so self.assertIsInstance() works in that case. so that makes sense | |
| 17:18:38 | sean-k-mooney | we are not just replacing the metacass or simairl on teh orginl object, its fully wrapping it | |
| 19:52:37 | melwitt | sean-k-mooney: sorry about that. I'm not sure why I thought tpool proxy stuff in general wasn't covered in unit tests. I see now that there is, so I'll add to that area with the reproposal | |
| 19:56:16 | sean-k-mooney | melwitt: it was dansmith that noticed, i didnt think we had testign that the returned obejcts were proxies either | |
| 19:57:06 | dansmith | maybe it's just me, but any patch that touches production code and doesn't touch anything in the test tree at all should probably be a red flag, and at least have a note in the commit message about why it's not tested | |
| 19:58:50 | sean-k-mooney | ocationally its a pure refactor with no observable change, that how i was looking at this but that was not actully ture in this case | |
| 19:59:02 | sean-k-mooney | we can observe that the objects are wrapped | |
| 20:00:08 | melwitt | it was originally a test patch that I uploaded for someone to run experiments while I was debugging a downstream incident and then time passed and then for some reason I didn't find the existing test coverage. I thought it was something we don't test in general, but obviously I had missed it | |
| 20:01:50 | dansmith | sean-k-mooney: sure a refactor is a good reason to have no test coverage showing the refactor has no impact.. but this was not that :) | |
| 20:03:37 | sean-k-mooney | does anyone maintian storyboard or freezer anymore? | |
| 20:03:44 | sean-k-mooney | i assume no | |
| 20:07:44 | melwitt | dunno anything about freezer but afaik storyboard is no longer being developed | |
| 20:08:25 | sean-k-mooney | so there is an eventlet isseu with watcher im trying to debug | |
| 20:08:47 | sean-k-mooney | but very few projects use the apschduler lib | |
| 20:08:48 | JayF | sean-k-mooney: obligatory: have you ensured the new eventlet+oslo.log releases don't fix it? | |
| 20:08:57 | sean-k-mooney | and sofar i dont think they use eventlet | |
| 20:09:07 | sean-k-mooney | JayF: yep | |
| 20:09:18 | JayF | too bad :( | |
| 20:09:42 | sean-k-mooney | im tryign to confirm if wathcer is the only project that uses that lib and eventlet | |
| 20:10:09 | sean-k-mooney | zuul ueses apscheduler for example but i dotn thinik it uses eventlet | |
| 20:10:49 | sean-k-mooney | oh also the issue only happen on python 3.12 and the actual issue is happenign isn sqlachemy... | |
| 20:11:25 | sean-k-mooney | so you know tottally trivial to figure out what the hell is happening | |
| 20:12:13 | melwitt | good times | |
| 20:19:40 | sean-k-mooney | the fun part is they are working on a 4.0 release like sqlalchme 2.0 and have been in the process of that migration for a while | |
| 20:20:56 | sean-k-mooney | so even if i get this workign with 3.11 we weill have to eventually port to the new 4.0 interface, 3.x is still supproted at least | |
| 20:23:12 | sean-k-mooney | the 4.x brnach supprot sqlalchemy 2.0 im not sure that is true of the 3.x branch but that one of the paths im looking into | |
| 20:35:52 | opendevreview | Douglas Viroel proposed openstack/nova master: WIP - Add support for showing scheduler_hints in server details https://review.opendev.org/c/openstack/nova/+/938604 | |
| 20:36:10 | opendevreview | Douglas Viroel proposed openstack/nova master: Add support for showing scheduler_hints in server details https://review.opendev.org/c/openstack/nova/+/938604 | |
| #openstack-nova - 2025-01-15 | |||
| 02:53:59 | hemanth | Hi team, unfortunately I could not attend weekly meeting yesterday to ask about one of the item which I added in https://etherpad.opendev.org/p/nova-2025.1-status, appreciate if someone can review https://review.opendev.org/c/openstack/nova/+/939044 and provide feedback, thanks! | |
| 03:28:15 | opendevreview | Mengyang Zhang proposed openstack/nova master: Add Burst Length Support to Cinder QoS https://review.opendev.org/c/openstack/nova/+/939310 | |
| 06:46:17 | opendevreview | melanie witt proposed openstack/nova master: libvirt: Wrap un-proxied listDevices() and listAllDevices() https://review.opendev.org/c/openstack/nova/+/939317 | |
| 06:48:14 | melwitt | sean-k-mooney: stacked this on top of your revert fyi ^ | |
| 09:39:42 | opendevreview | Dmitriy Chubinidze proposed openstack/nova master: Using "dnf" instead of "yum" with uncommenting compute_driver option. https://review.opendev.org/c/openstack/nova/+/939325 | |
| 09:55:49 | opendevreview | Ivan Anfimov proposed openstack/nova master: doc: Adding link for RabbitMQ installation during nova deployment on controller node. https://review.opendev.org/c/openstack/nova/+/938702 | |
| 10:02:52 | opendevreview | Dmitriy Chubinidze proposed openstack/nova master: Enabling of using LibvirtDriver for compute node. https://review.opendev.org/c/openstack/nova/+/939325 | |
| 10:14:20 | opendevreview | Dmitriy Chubinidze proposed openstack/nova master: doc: Enabling of using LibvirtDriver for compute node. https://review.opendev.org/c/openstack/nova/+/939325 | |
| 10:21:32 | opendevreview | Dmitriy Chubinidze proposed openstack/nova master: doc: Enabling of using LibvirtDriver for compute node. https://review.opendev.org/c/openstack/nova/+/939325 | |
| 14:47:16 | opendevreview | Mengyang Zhang proposed openstack/nova-specs master: Add Burst Length Support to Cinder QoS https://review.opendev.org/c/openstack/nova-specs/+/932653 | |
| 15:00:24 | MengyangZhang[m] | ||
| 15:08:19 | opendevreview | Takashi Kajinami proposed openstack/nova master: Add Burst Length Support to Cinder QoS https://review.opendev.org/c/openstack/nova/+/939310 | |
| 15:43:55 | sean-k-mooney | JayF: im actully questioning if the oslo.log patch did actully fix the event let issue or not. | |
| 15:45:05 | sean-k-mooney | i swaped form my debuggin patch to the tip of master because i wanted to do a rebase and if i restart the service instead of getting the blockign call in the first 3-5 second like i was reliablly before it s been running for 5 mins witn no issue | |
| 15:46:22 | JayF | Given the nature of the eventlet change, I wonder more if it's "existing bug that's coming to the surface more easily" vs it being a direct eventlet issue | |
| 15:46:35 | JayF | I wonder if you have any pipemutex-shaped constructs in nova that might need some patching | |
| 15:47:07 | JayF | if you can isolate the case, you can see if you can get itamarst in #openstack-eventlet-removal to look if you get stuck :) | |
| 15:47:08 | sean-k-mooney | oh the thing im debuging is in watcher | |
| 15:47:10 | sean-k-mooney | not nova | |
| 15:47:12 | JayF | aha | |
| 15:47:39 | JayF | either way, same comment applies, even though TBH our shared downstream doesn't use watcher :D | |
| 15:48:29 | sean-k-mooney | well the thing im trying to debug is actuly related to oslo.db and sqlachemy, eventlet complains that you shoudl not make blocking calls on the main tread | |
| 15:48:55 | sean-k-mooney | the blockign call is time.sleep(0) form oslo.db _thread_yeild function | |
| 15:49:49 | sean-k-mooney | so this https://github.com/openstack/oslo.db/blob/21c484ffb26ed03cbb1ca9dd7fd27fb19a1d4b36/oslo_db/sqlalchemy/engines.py#L43-L52 | |
| 15:51:10 | JayF | I wonder if that should be something more like "if eventlet is imported; sleep(0)" | |
| 15:51:24 | JayF | time.sleep(0) unpatched technically is a blocking call, yeah? | |
| 15:51:25 | sean-k-mooney | https://paste.opendev.org/show/bGPgfURx1cZYOsgmtDyw/ | |
| 15:52:00 | JayF | I'm really, really curious if changing that to an eventlet.sleep(0) in your test fixes that | |
| 15:52:04 | sean-k-mooney | that is the error but it iss patched as far as i could tell | |
| 15:53:11 | JayF | sean-k-mooney: where does the patch happen? | |
| 15:53:14 | sean-k-mooney | JayF: this is where the runtim error is comign form https://github.com/eventlet/eventlet/blob/3450e6adb017f4cd47d1c4e06331c9fa26a986d1/eventlet/greenthread.py#L35 | |
| 15:53:23 | JayF | sean-k-mooney: I'm looking for it, I see no calls to https://github.com/openstack/watcher/blob/master/watcher/eventlet.py | |
| 15:53:58 | JayF | hmm, yeah, this is patching late | |
| 15:54:08 | JayF | I suspect this is like, half eventlet patched | |
| 15:54:09 | sean-k-mooney | https://github.com/openstack/watcher/blob/master/watcher/cmd/__init__.py#L29 | |
| 15:54:23 | sean-k-mooney | JayF: its not patching late | |