| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-02-08 | |||
| 08:43:03 | sean-k-mooney | i was more concenred with teh test fallout then any impact it woudl nova on nova in production | |
| 08:43:41 | sean-k-mooney | the code change is pretty low risk | |
| 08:44:11 | sean-k-mooney | fixign all the broken test could be a lot of work. | |
| 08:45:03 | sean-k-mooney | bauzas: the libvirt event dispatch thread is curently while True | |
| 08:45:05 | sean-k-mooney | https://github.com/openstack/nova/blob/ea0526d959f7246c7d741ea24c207b52417d224a/nova/virt/libvirt/host.py#L209-L218 | |
| 08:45:33 | sean-k-mooney | to fix that we need a way to stop that thread for the functionl tests at least | |
| 08:46:32 | sean-k-mooney | well or we need to make sure its mocked out properly | |
| 08:48:09 | sean-k-mooney | https://github.com/openstack/nova/blob/ea0526d959f7246c7d741ea24c207b52417d224a/nova/virt/libvirt/host.py#L607 is always creating 2 greenthreads that never exit if its called today | |
| 08:49:36 | sean-k-mooney | so the libvirt fixture need to be enhanced to stub that out | |
| 08:49:38 | bauzas | sean-k-mooney: the problem is that given we don't know which test is causing trouble, we won't be able to make sure your change can fix it | |
| 08:49:54 | sean-k-mooney | my patch show which tests are causing the issue | |
| 08:50:39 | bauzas | I see | |
| 08:50:45 | sean-k-mooney | bauzas: i added a fixture that detects when tests leak green threads https://review.opendev.org/c/openstack/nova/+/873061/4/nova/tests/fixtures/nova.py#1138 | |
| 08:51:00 | sean-k-mooney | and prints there name although that part currenlty has a race | |
| 08:51:25 | sean-k-mooney | fortunetly when we hit the races it still raise an exception and fails the test | |
| 08:52:36 | bauzas | ok, so then we could use your change for telling which functests leak out | |
| 08:52:54 | sean-k-mooney | yep | |
| 08:53:00 | bauzas | but for the moment, we should only merge your change at the beginning of Bobcat | |
| 08:53:14 | sean-k-mooney | i suspect that much of the cases it in the common code | |
| 08:53:21 | bauzas | just because I want to have time to make sure that if we find some issues, it shouldn't be a time problem | |
| 08:53:38 | bauzas | (I just want to be careful here) | |
| 08:54:35 | sean-k-mooney | so as i noted above. the host.py initalize currently spanw 2 greenthreads and that is call by the libvirt driver | |
| 08:55:06 | sean-k-mooney | i dont think that is stubbed by the the libvirt fixture | |
| 08:55:32 | sean-k-mooney | so today i think all libvirt functional tests are leaking at least 2 greenthreds. | |
| 08:55:55 | sean-k-mooney | the libvit event dispatcher thread and connection event thread | |
| 08:57:04 | bauzas | lemme verify | |
| 08:57:44 | sean-k-mooney | we dont currently save the returned GT and they have a while true so currenlty there is no way to stop those but it would no be hard to add one | |
| 08:58:02 | bauzas | at least we know the leaked thread is calling self.live_migration_abort() | |
| 08:58:23 | bauzas | but I think the leaked thread comes from a RPC call | |
| 08:58:38 | bauzas | not from a libvirt thread | |
| 08:58:45 | gibi | I have some concerns of the pooling in general and I left comments there | |
| 08:58:57 | sean-k-mooney | ack | |
| 08:59:04 | bauzas | honestly, my concern is more about the time here | |
| 08:59:13 | gibi | bauzas, sean-k-mooney: I think the current pooling won't catch the RPC threads as that is created in oslo.messaging | |
| 08:59:37 | bauzas | I see three cores at least looking at this bug while we only have 7 days for merging features | |
| 08:59:39 | sean-k-mooney | gibi: yes it likely wont but those are not created directly by nova | |
| 09:00:05 | sean-k-mooney | gibi: with that said i might be able to make it do that | |
| 09:00:12 | gibi | I will keep rechecking bauzas' | |
| 09:00:22 | gibi | I will keep rechecking bauzas's patch to catch a failure to see where it is coming from | |
| 09:00:29 | bauzas | so, while I think it's important to have a better way to have green threads pooling, I'm just saying that we maybe should try to just find the issue and at least do other stuff | |
| 09:00:44 | bauzas | sean-k-mooney: see,that's a problem then | |
| 09:00:55 | sean-k-mooney | bauzas: no its not | |
| 09:01:12 | bauzas | sean-k-mooney: as I said, I'm pretty sure that the threads that are leaked and create this libvirt exceptioin are RPC calls | |
| 09:01:40 | bauzas | when I say a problem, I mean I'm not sure this change would help then | |
| 09:01:46 | sean-k-mooney | right but ye did not have a repoducer so i tried to create one and found a bunch of issue | |
| 09:01:51 | bauzas | https://4dca9d38a541907e85e1-0253beca39d73a6e7192d5b32ed5edc2.ssl.cf2.rackcdn.com/860282/2/check/nova-tox-functional-py310/466e0d7/testr_results.html | |
| 09:02:09 | sean-k-mooney | it may not fix the current issue but all the other tests if found may be flaky | |
| 09:02:17 | gibi | sean-k-mooney, bauzas: cool, we have two set of issues to solve then :) | |
| 09:02:19 | bauzas | agreed | |
| 09:02:26 | bauzas | and agreed with gibi | |
| 09:02:29 | sean-k-mooney | and if im wirte we are also constantly builiding up greenthreas as the test run | |
| 09:02:44 | sean-k-mooney | yes | |
| 09:02:44 | bauzas | sean-k-mooneyI'm not saying "NO" to your change and thanks for having worked on it | |
| 09:02:57 | sean-k-mooney | two sets of issues | |
| 09:03:12 | bauzas | sean-k-mooney:I'm just saying that I'm afraid this won't help the functest CI failure we have atm | |
| 09:04:02 | bauzas | anyway, I said loudly yesterday that I'll stop looking at the CI failures today and I'll rather review some changes | |
| 09:04:15 | bauzas | os-vif and os-traits first, and then nova features | |
| 09:04:34 | sean-k-mooney | there arnt any we need for os-vif in this release | |
| 09:04:35 | bauzas | and I'll continue to look at https://review.opendev.org/c/openstack/nova/+/872975 and recheck until we get a -1 | |
| 09:04:42 | sean-k-mooney | not sure about os-traits | |
| 09:04:54 | bauzas | that's what I'll be doing | |
| 09:05:07 | bauzas | people are free to do anything | |
| 09:05:14 | bauzas | they prefer | |
| 09:05:35 | bauzas | I also need to look at the releases we have for os-vif, os-traits and os-rc | |
| 09:06:03 | sean-k-mooney | i approved the os-vif one yesterday | |
| 09:06:09 | sean-k-mooney | i did not look at the others | |
| 09:09:28 | sean-k-mooney | gibi: so we do stub out the events thread https://github.com/openstack/nova/blob/ea0526d959f7246c7d741ea24c207b52417d224a/nova/tests/fixtures/libvirt.py#L919-L941 | |
| 09:09:35 | sean-k-mooney | but not the other one | |
| 09:10:31 | sean-k-mooney | https://github.com/openstack/nova/blob/ea0526d959f7246c7d741ea24c207b52417d224a/nova/virt/libvirt/host.py#L616-L619 | |
| 09:11:06 | sean-k-mooney | i wonder if we can just more utils.spawn(self._conn_event_thread) into self._init_events() | |
| 09:12:23 | gibi | sean-k-mooney: I won't mix the events part with the connection thread. the events part uses a native thread | |
| 09:12:50 | bauzas | elodilles: 2023-02-07 14:02:46.446989 | compute1 | neutron-openvswitch-agent: no process found on https://review.opendev.org/c/openstack/nova/+/871702 | |
| 09:13:04 | bauzas | elodilles: that's the second recheck having the same problem | |
| 09:13:05 | gibi | but sure we can wrap the connection thread spawning and mock it | |
| 09:13:17 | bauzas | elodilles: so, OK, when you say "broken broken", I understand it :) | |
| 09:13:36 | sean-k-mooney | well these are both related to event handeling | |
| 09:14:03 | sean-k-mooney | so it feell like it should be also in _init_events | |
| 09:14:16 | gibi | bauzas: how do you feel about https://review.opendev.org/c/openstack/nova/+/872975/2/nova/virt/libvirt/driver.py#10064 our current trials haven't hit the true positive case yet, but already hit the half false positives a lot | |
| 09:14:56 | gibi | sean-k-mooney: connection thread is where nova initiate calls to libvirt the event thread is where libvirt initiate call back to nova | |
| 09:15:49 | sean-k-mooney | ok but both of them are related to the libvirt events transmit vs recive | |
| 09:15:55 | sean-k-mooney | https://github.com/openstack/nova/blob/ea0526d959f7246c7d741ea24c207b52417d224a/nova/virt/libvirt/host.py#L480 | |
| 09:16:16 | sean-k-mooney | and the doc string makes it sound like a good fit | |
| 09:16:30 | sean-k-mooney | it woudl jsut be adding the spawn to the end fo that function | |
| 09:16:42 | sean-k-mooney | so we spawn the dispatche and reciver threads form the same place | |
| 09:16:54 | sean-k-mooney | and the native thread | |
| 09:18:57 | sean-k-mooney | anyway ill be back in an hour or two i was woken up at around 6 and started looking at this. so im going to see if i ca rest for a bit although at this point i may have missed that window | |
| 09:20:36 | gibi | sean-k-mooney: ack | |
| 09:23:19 | elodilles | bauzas: yepp, it's broken broken this time :] | |
| 09:25:19 | elodilles | bauzas: but this should do the work when merged: https://review.opendev.org/c/openstack/grenade/+/872969 | |
| 09:26:50 | opendevreview | Pierre Libeau proposed openstack/nova master: Add mechanism to manage snapshot during nc init https://review.opendev.org/c/openstack/nova/+/873062 | |
| 09:41:25 | zigo | sean-k-mooney[m]: Would you know how to easily just forbid *ALL* .vmdk on train and before? | |
| 09:43:26 | bauzas | gibi: sure, let's do it | |
| 09:48:46 | opendevreview | Sylvain Bauza proposed openstack/nova master: DNM: Add logging for leaking out the non-poisoned libvirt testcase https://review.opendev.org/c/openstack/nova/+/872975 | |
| 09:49:05 | bauzas | gibi: ^ | |
| 10:10:47 | gibi | bauzas: thanks | |
| 10:17:43 | bauzas | folks, on os-traits, given we only merged https://opendev.org/openstack/os-traits/commit/feb3e28a00eeab0d4dbf097dd801aff3adeb10d6 we don't need a release | |
| 10:18:21 | bauzas | that being said, there are 2 open changes that are related to some accepted nova blueprint https://review.opendev.org/q/status:open+project:openstack/os-traits | |
| 10:18:57 | bauzas | elodilles: I just approved https://review.opendev.org/c/openstack/os-traits/+/871226 on os-traits | |
| 10:19:06 | bauzas | elodilles: we'll need a release | |
| 10:21:10 | bauzas | cores, need a second +2/+W on https://review.opendev.org/c/openstack/os-traits/+/872185 to let Uggla not blocked by this os-trait change | |