Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2025-12-04
12:51:08 sean-k-mooney we can likely mock time.now() and that shoudl be enough i think but i woudl have to look at the specific tests
12:55:34 gibi zigo: sean-k-mooney: we should have a fixture for the time.now() already in our tests maybe it is not used in those specific tests
12:55:49 gibi but overall I agree to make the test independent of the value of time.now
12:56:16 zigo I'd say: please, feel free to takeover my patch and make it better.
12:56:23 zigo I don't have enough available time to do so.
12:59:11 gibi I don't have time for that at the moment too. sorry
14:14:14 sean-k-mooney gibi: we do have a fixture for that yes but i didnt check if it was used either
14:14:40 sean-k-mooney https://github.com/openstack/nova/blob/master/nova/test.py#L828
14:15:29 sean-k-mooney that works a littel defently then i descrbed making all calsl return a fixed time
14:16:01 sean-k-mooney in this case the test logic is either incorect or we need one to monotmicly march forward to fit this test
14:16:34 sean-k-mooney https://github.com/openstack/nova/blob/master/nova/tests/functional/test_nova_manage.py#L2172-L2175
14:24:43 opendevreview Dominik proposed openstack/nova master: Regression test for Placement allocations remaining during failed schedule https://review.opendev.org/c/openstack/nova/+/969251
14:39:43 opendevreview Dominik proposed openstack/nova master: Remove Placement allocations in the broken build cleanup https://review.opendev.org/c/openstack/nova/+/968446
14:45:01 opendevreview Merged openstack/nova-specs master: Propose Spec 1 of Graceful shutodwn for 2026.1 https://review.opendev.org/c/openstack/nova-specs/+/969299
15:08:05 gibi dansmith: sorry no progress yet with the utils.spawn issue. I was distracted with other things
15:08:44 dansmith gibi: no worries, I found it while doing other things, so once I found the issue I went back to those things
15:18:39 gibi dansmith: but now I found the change I had a hunch about https://review.opendev.org/c/openstack/nova/+/953121/2/nova/tests/fixtures/nova.py#b1248 so before this change the utils.spawn in untit test executed the passed in function and *raised back* any exception. But that is semantically wrong as only utils.spawn(f).wait() should raise it. So I changed the unit test fixture semantics to not raise from
15:18:45 gibi __init__
15:19:12 gibi so that is way in the past the test got the exception without the code under test ever waiting for the result of the spawned thread
15:19:46 gibi a/way/why/
15:19:51 gibi s/way/why/
15:20:45 dansmith gibi: okay but we don't wait() on the result of the spawn there, that's the point right?
15:21:19 gibi right. the live migration code never wait for that spawned thread to finish hence it cannot get the exception from it
15:21:20 sean-k-mooney dansmith: gibi thanks for the review. ill remove the storage stuff and try and adress the other comments
15:21:20 dansmith we should have wait()ed on the result of the greenthread before, so the exception showed up at that point, so it was probably not semantically right before, but we still need a change
15:22:06 gibi dansmith: I don't know yet if the livemigration code needs to wait or not. In the past it did not waited.
15:23:13 dansmith gibi: but if it doesn't we would never notice if we failed to call guest.migrate() or raised any exception within
15:25:15 dansmith this example was a bad mock, but it stands for any failure within the thread we spin up to do the actual migration
15:27:03 gibi yeah I got you. The errors in the thread are either handled within that thread or lost and the migration_monitor just detects the failed migration independetly
15:27:44 dansmith will it? it will eventually time out or something, but I'm not sure that's really the same
15:27:54 dansmith but yeah, either way.. I think we need to collect the result regardless
15:28:17 dansmith and certainly by not doing it here, any unit testing we do will always pass no matter what happens inside that "thread"
15:28:18 gibi yeah I'm not against collecting the result
15:28:49 gibi I will do some trials locally what happens in devstack if I break the call to guest.migrate()
15:29:45 dansmith okay
15:31:05 gibi do I understand correctly that such result collection is anyhow after the monitor already finished the monitoring so the migration either happened or failed and we just want to get better logs by knowing any exceptions from the thread?
15:32:02 dansmith sean-k-mooney: quick before you revise, I think storage_fsid is fine.. we could even make that support(able) on other things by saying "put a .fsid file with a uuid in the root and we'll report it".. it's the local|shared I think is a lie :)
15:32:38 dansmith gibi: I'll have to go back and look and the monitor behavior if nothing ever starts
15:33:43 gibi OK. No need to jump back yet. I will verify that locally. I just wanted to see that my general understading is not totally wrong
15:33:50 dansmith kinda looks like it will wait forever if DOMAIN_JOB_NONE right?
15:33:57 opendevreview Merged openstack/nova master: pre-commit: Bump versions https://review.opendev.org/c/openstack/nova/+/966089
15:33:59 gibi yeah there is a loop
15:34:13 dansmith we can probe the future in that loop if we pass it right? and stop waiting if the thread has ended for any reason?
15:35:49 gibi future.done() is non blocking so yes we can probe
15:36:17 dansmith right
15:36:42 dansmith if future.done(): future.result()
15:36:55 dansmith ^ will raise where we're already try..excepting for migration failure
15:36:59 dansmith if done inside the loop
15:38:24 gibi yeah I think so
15:39:28 sean-k-mooney dansmith: ok so if i removed storage_type and kept stroage_fsid you woudl be fine with that
15:39:55 sean-k-mooney and sure we culd make that declarbale in some way vai a file or config option where we cant dicusver it easily.
15:40:36 dansmith sean-k-mooney: yes, I called out storage_type specifically because we can't know.. we _can_ know storage_fsid
15:40:45 sean-k-mooney ack
15:41:09 dansmith man, I do not get it..
15:41:13 sean-k-mooney oh you mean each compute cant know that its shared even if its nfs
15:41:18 sean-k-mooney oh ya your right
15:41:23 opendevreview Takashi Kajinami proposed openstack/nova master: libvirt: Use firmware auto-selection by libvirt https://review.opendev.org/c/openstack/nova/+/969132
15:41:24 dansmith correct
15:41:33 dansmith sometimes I load the spec and gerrit shows me tons of unresolved comments and other times, not.
15:41:43 sean-k-mooney ya...
15:41:52 sean-k-mooney i noticed that too for the last few days
15:42:04 sean-k-mooney i dont know if its a recent regression or browswer caching
15:45:47 gibi dansmith: heh we have a convoluted way already in place to detect early end of the thread. The code adds a callback to the future to execute when it finishes. That callback sets finish_event (which is an Event) that is passed into the monitoring loop and checked there https://github.com/openstack/nova/blob/5d3d0c870a1c37dcd6ce9cd0d542282c7c7994f3/nova/virt/libvirt/driver.py#L11405
15:46:24 gibi so the monitoring loop detects the early end of the thread via that event
15:46:26 dansmith gibi: that detects when the future ends yes, but doesn't care why
15:46:30 gibi (still it does not collect the resutl)
15:46:32 dansmith right
15:46:34 dansmith yep
15:47:09 dansmith gibi: fwiw, I wasn't actually seeing that callback be called when I was pdb'ing the unit test
15:47:20 dansmith but that might be specific to the unit test environment or something
15:47:35 gibi the unit test fixture calles the callback right when it is registered :/
15:47:57 dansmith ah
15:48:16 gibi that fixture is suboptimal to serve the unit test needs
15:48:44 gibi OK. I'm convinced enough now that collecting the result after the monitor make sense
15:49:33 dansmith do we actually need the convoluted done event? is that maybe to give the same behavior in both models or something, instead of just checking future.done?
15:50:35 gibi as far as I think we don't need the convoluted way, we can just use future.done instead within the monitor
15:51:01 dansmith okay that might be better anyway then
15:51:03 gibi using the future directly removes the callback which is a nice symplification
15:51:14 dansmith yup
15:51:48 gibi I have context now should I just pick you patch up and update it? or you would like to do it?
15:53:12 dansmith you're welcome to, but if you need to do other stuff I can do it also
15:53:48 gibi I take it. I need to write some code to keep my sanity :)
15:54:13 dansmith yeah, I've been just reviewing for weeks now.. sanity gone :(
16:21:27 gmaan dansmith: gibi: bauzas: I did one ammended in graceful shutdown backlog spec as spec3, please check. it is for backlog only and to keep it out of scope from the scope of this cycle spec https://review.opendev.org/c/openstack/nova-specs/+/969543
16:22:43 gibi gmaan: make sense to me thanks.
16:30:03 opendevreview sean mooney proposed openstack/nova-specs master: add spec for resource tracker notifications https://review.opendev.org/c/openstack/nova-specs/+/967712
16:48:00 opendevreview Ghanshyam proposed openstack/nova-specs master: Follow up fixes for graceful shutdown spec https://review.opendev.org/c/openstack/nova-specs/+/969813
16:50:08 gmaan gibi: bauzas ^^ quick follow up for fixing some comments
16:51:04 gmaan and thanks dansmith sean-k-mooney gibi bauzas for review and help on the shaping graceful shutdown specs.
16:52:47 gmaan ratailor: most probably it is missing schema but let me review todaty
17:07:07 opendevreview Merged openstack/nova-specs master: Follow up fixes for graceful shutdown spec https://review.opendev.org/c/openstack/nova-specs/+/969813
17:18:46 opendevreview sean mooney proposed openstack/nova-specs master: Repropose and update cyborg vGPU (mdev) support https://review.opendev.org/c/openstack/nova-specs/+/967515
17:22:58 sean-k-mooney bauzas: gibi i adresss bogdans feedback and the other open comments if you have time to review that too.
17:51:09 opendevreview Balazs Gibizer proposed openstack/nova master: Collect result of _live_migration_operation https://review.opendev.org/c/openstack/nova/+/969501
18:08:13 opendevreview sean mooney proposed openstack/nova-specs master: Repropose and update cyborg vGPU (mdev) support https://review.opendev.org/c/openstack/nova-specs/+/967515
18:17:21 opendevreview Jay Faulkner proposed openstack/nova stable/2024.2: [ironic] Ensure unprovision happens for new states https://review.opendev.org/c/openstack/nova/+/969833
18:17:39 JayF The next step backport for the ironic bugfix, for your consideration ^^^^ :) thank you!
19:06:05 melwitt dansmith: EZ io=native config patch that you have expressed some interest before 😇 https://review.opendev.org/c/openstack/nova/+/964848
19:12:20 dansmith oh yeah I literally have it open already
19:12:39 dansmith I shall hit for sure eventually

Earlier   Later