| opendevreview | Ghanshyam Maan proposed openstack/nova master: Reduce the unnecessary executors shutdown logs in test jobs https://review.opendev.org/c/openstack/nova/+/1004160 | 03:07 |
|---|---|---|
| *** ykarel__ is now known as ykarel | 06:23 | |
| gibi | gmaan: re https://bugs.launchpad.net/nova/+bug/2166009 I think my question is about a different scenario. https://github.com/openstack/nova/blob/94de0576ca02e0668233bedfbe7e18b87316c708/nova/compute/manager.py#L6418 runs on the source, and if the virt driver raises an exception then we wont hit this cleanup code so we potentially leak a task on the *source* node of the resize | 07:39 |
| gibi | dansmith: yeah, this wait forever on the executor is intentional (and debatable) in the functional test. I want to avoid leaking running threads across test cases as that leads to very hard to debug unexpected behaviors in later test cases. native threads are not killable so I cannot force cleanup between tests | 07:40 |
| gibi | you (we) can try to add some task tracking in the functional test and log and fail instead of wait forever if the executor is not empty to help debugging. | 07:42 |
| gibi | I would love to work on this but swamped with important bugfixes at the moment | 07:42 |
| gibi | you can try to locally flip the wait=true to wait=false in the executor shutdown and try to find a way to print the content of the excutor to help narroving which task is not finished in the test case | 07:43 |
| gibi | OK, maybe a way out, mock the shutdown in the func test to wait a bit but not forever for the executor and if the executor is not empty after the wait then fail the test case cleanly, that should be easy to implement and would avoid the hard to debug hang and clearly show at least which test case is effected | 07:47 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Add missing resize.error notification https://review.opendev.org/c/openstack/nova/+/1006163 | 08:13 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Reproduce bug 2166786 https://review.opendev.org/c/openstack/nova/+/1004658 | 08:30 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Fix PCI allocation leak on failed cold migration https://review.opendev.org/c/openstack/nova/+/1006003 | 08:30 |
| opendevreview | Sylvain Bauza proposed openstack/nova master: Reject stale reserve_block_device_name RPC on compute https://review.opendev.org/c/openstack/nova/+/1006011 | 08:35 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Reproduce bug 2166786 https://review.opendev.org/c/openstack/nova/+/1004658 | 08:59 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Fix PCI allocation leak on failed cold migration https://review.opendev.org/c/openstack/nova/+/1006003 | 08:59 |
| * zigo is running tempest on his packaged-based CI already, and VMs are already spawned. \o/ | 10:23 | |
| zigo | (Hibiscus, of course...) | 10:24 |
| zigo | I very much love that I was able to see: | 10:24 |
| zigo | setUpClass (tempest.api.compute.admin.test_live_migration.LiveMigrationWithVTPMTest) ... SKIPPED: LiveMigrationWithVTPMTest skipped as vTPM live migration is not enabled | 10:24 |
| zigo | :) | 10:24 |
| zigo | Super nice feature, thanks guys !!! | 10:25 |
| nicolairuckel | Is anyone currently working on/thinking about this? https://bugs.launchpad.net/nova/+bug/1785123 | 12:07 |
| gibi | nicolairuckel: isn't it fixed in https://review.opendev.org/c/openstack/nova/+/959682 ? | 12:09 |
| gibi | or is that a partial fix? | 12:10 |
| nicolairuckel | That was only a partial fix. That patch doesn't cover cold migration and shelve. | 12:10 |
| nicolairuckel | We decided to deal with those in a separate patch (see https://review.opendev.org/c/openstack/nova/+/959682/comments/ae045d85_9e58a5a5?tab=comments) | 12:10 |
| nicolairuckel | Unfortunately, we can't just use libvirt for that like I did in the patch you linked. | 12:11 |
| gibi | nicolairuckel: I'm not aware of anybody working on the cold migration part | 12:12 |
| gibi | sean-k-mooney: ^^ maybe you have more context | 12:12 |
| sean-k-mooney | am | 12:12 |
| sean-k-mooney | so i tought we had started on it but i think we mainly just fixed the reboot case so far | 12:13 |
| sean-k-mooney | i dont recall if there was a draft for cold migragte | 12:13 |
| sean-k-mooney | but we did say we likely need an api change for rezie eventually | 12:13 |
| sean-k-mooney | so sate if the nvram shold be cleared or not. sorry that was for rebuild | 12:14 |
| sean-k-mooney | for rebuild we need ot add an option to presserve/cleare the nvram | 12:14 |
| sean-k-mooney | to cather for the rebuild form snapshot vs unrelated image case | 12:14 |
| sean-k-mooney | there was https://review.opendev.org/c/openstack/nova/+/621646 | 12:14 |
| sean-k-mooney | but no https://review.opendev.org/c/openstack/nova/+/959682 did not fix cold migrate and shleve | 12:15 |
| sean-k-mooney | gibi: nicolairuckel i remember there was a reason we split it that way | 12:16 |
| sean-k-mooney | but i dont recall what that was | 12:16 |
| gibi | thanks sean-k-mooney | 12:17 |
| nicolairuckel | Because of the API change we probably won't be able to backport the changes needed for cold migration. | 12:17 |
| nicolairuckel | But for the other patch we were able to backport them | 12:17 |
| sean-k-mooney | for cold migration we do not need an api change | 12:17 |
| sean-k-mooney | that only for rebuild | 12:17 |
| nicolairuckel | ah | 12:17 |
| sean-k-mooney | i misspoke before | 12:17 |
| sean-k-mooney | for cold migrate we whosul always preseve the nvram | 12:18 |
| nicolairuckel | The other reason was complexity. My patch turned out to be quite simple since we were able to use libvirt. | 12:18 |
| nicolairuckel | Is this the problem for rebuild? https://bugs.launchpad.net/nova/+bug/2139077 | 12:18 |
| sean-k-mooney | no for rebuild we have 2 usecases | 12:18 |
| sean-k-mooney | some people use rebuild for backup and restore or to upgrade the software in teh vm | 12:19 |
| sean-k-mooney | so for that cohort we want to preserve the nvram | 12:19 |
| sean-k-mooney | but you can also use rebuild to change the image entrily and even disable secure boot | 12:19 |
| sean-k-mooney | so for that second group clearing the nvram is more desireable | 12:20 |
| sean-k-mooney | my prefernce woudl be to preserve by default and have an new api option to ask for it to be cleared | 12:20 |
| nicolairuckel | So I guess we should split that up even further: one patch for the cold migration and one for rebuild? | 12:20 |
| sean-k-mooney | https://bugs.launchpad.net/nova/+bug/2139077 is a third edgecase but its a less common one | 12:21 |
| sean-k-mooney | yes i would keep it split | 12:21 |
| sean-k-mooney | im not sure what the curret rebuild behivor is | 12:21 |
| sean-k-mooney | shelve is also the final edgbecase we did not adress | 12:22 |
| sean-k-mooney | which si it would be nice to save the nvram somewhere on shelve | 12:22 |
| sean-k-mooney | we just didnt agree where | 12:22 |
| sean-k-mooney | all of the edgecase for nvram also applies ot the tpm data too | 12:22 |
| sean-k-mooney | for windows in partical preserving the nvram and loosing the tpm data on cold migrate will require bitlocker recovery | 12:23 |
| sean-k-mooney | so if i was to work on this i would do cold-migrate/resize next for both nvram and tpm | 12:24 |
| nicolairuckel | In one patch or separate patches? | 13:13 |
| bauzas | gibi: hope your laptop is back, can I start to look at your patches ? | 13:13 |
| nicolairuckel | I'm not sure if we need TPM | 13:14 |
| gibi | bauzas: give me 5 to push what I have (some missing tests) | 13:15 |
| bauzas | ack, I'll stop in ~1.5hour | 13:16 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Fix PCI allocation leak on failed cold migration https://review.opendev.org/c/openstack/nova/+/1006003 | 13:21 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Clean PCI claim of failed resize in periodic task https://review.opendev.org/c/openstack/nova/+/1006209 | 13:21 |
| gibi | bauzas: so ^^ I have 3 patches | 13:22 |
| gibi | 1. reproducer https://review.opendev.org/c/openstack/nova/+/1004658 | 13:22 |
| gibi | 2. clean old leaks from periodics https://review.opendev.org/c/openstack/nova/+/1006209 | 13:22 |
| gibi | 3. prevent new leaks https://review.opendev.org/c/openstack/nova/+/1006003 | 13:23 |
| bauzas | ok | 13:23 |
| gibi | the 3rd is RPC sensitve :) | 13:23 |
| gibi | and I have to add unit test to the 2nd patch | 13:23 |
| dansmith | gibi: I found a deadlock in my own stuff that was definitely related (but haven't solved yet) so that may have been my only issue, but other test workers are also hanging, so not sure | 13:36 |
| dansmith | I guess it feels like maybe we should not wait there but fail if any tasks are left in some way instead of wait | 13:37 |
| gibi | yeah that is a good idea to implement | 13:40 |
| gmaan | yeah failing is also good, better than hanging. | 15:55 |
| gmaan | gibi: RE:resize_instance: on source it will not hang the task as task on source resize_instance is tracked via RPC common wrapper and it will always end the task as soon RPC requsted is ended either by success or fail | 16:00 |
| gmaan | gibi: but yes, event cleanup will be left in failure case | 16:00 |
| gmaan | gibi: dansmith but on dest side we have case of task leakage same as live migration so what you think of it and we can backport it to stable/2026.2 https://review.opendev.org/c/openstack/nova/+/1006095 | 16:01 |
| *** ralonsoh is now known as ralonsoh_ooo | 16:02 | |
| gmaan | dansmith: I think I am seeing it in my patch (https://review.opendev.org/c/openstack/nova/+/1004160) also where test are hanging due to wait on executors https://zuul.opendev.org/t/openstack/build/9263ab2ac8c244f3a81f94efd50ad1c7/log/job-output.txt | 20:09 |
| gmaan | let me convert the wait to failure and see | 20:10 |
| dansmith | gmaan: I need to go EOD soon but are you saying we can't make the functional test fail instead of wait because of the task leakage on live migration? | 21:22 |
| gmaan | dansmith: we can, I am working on something to propose soon | 22:53 |
| gmaan | dansmith: live migration task leakage is when we will bump the manager_shutdown_timeout which is 0 currently | 22:54 |
Generated by irclog2html.py 4.1.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!