Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2025-12-04
09:40:16 zigo Should we add a sleep() in these unit tests?
10:57:48 opendevreview Thomas Goirand proposed openstack/nova master: Fix race condition in DB updates https://review.opendev.org/c/openstack/nova/+/969745
11:12:47 opendevreview Dominik proposed openstack/nova master: Regression test for Placement allocations remaining during failed schedule (bug #2132020) https://review.opendev.org/c/openstack/nova/+/969251
11:19:23 opendevreview Dominik proposed openstack/nova master: Regression test for Placement allocations remaining during failed schedule https://review.opendev.org/c/openstack/nova/+/969251
11:39:54 sean-k-mooney zigo: we try not ot add sleeps in gerneal so ideally no
11:40:38 sean-k-mooney zigo: we normally contol time in these test
11:41:24 sean-k-mooney so it looks like the test has a race condtion. if we add a sleep it woudl be somethign liks sleep(0.001)
11:42:09 sean-k-mooney zigo: i think gibi fixed a few cases likt this before when they started workign on the eventlet removal
12:02:34 stephenfin ratailor: bauzas: you need to define a new schema for tempest before the nova patch can merge. gmaan is a better person again to ask https://github.com/openstack/tempest/tree/8e2e854051b247ce914982ab0c3c0ed5e2f01b09/tempest/lib/api_schema/response/compute
12:02:58 stephenfin hopefully the duplication can be removed once the final patches merge against nova (hint hint)
12:03:07 stephenfin *final OpenAPI patches
12:06:12 opendevreview Thomas Goirand proposed openstack/nova master: Fix race condition in DB updates https://review.opendev.org/c/openstack/nova/+/969745
12:12:06 ratailor stephenfin, ack.
12:19:00 zigo sean-k-mooney: You're wrong, IMO.
12:19:18 zigo sean-k-mooney: The issue is getting 2 timestamps in less than 1 second.
12:19:33 zigo Then they are equal, and later on, the test checks if they are NOT equal.
12:23:16 sean-k-mooney zigo: what im saying is we normlaly try to decouple the unit test entirly form teh infra
12:23:52 sean-k-mooney for example by mockign time.now() so that each call woudl increment by 1 second or some other way
12:24:44 sean-k-mooney zigo: for functional test we might uses sleeps as we try to avoid mockign if we cna btu for unit test we usually mock otu the tiem functions
12:37:24 opendevreview Merged openstack/nova stable/2025.1: [ironic] Ensure unprovision happens for new states https://review.opendev.org/c/openstack/nova/+/969547
12:40:55 zigo sean-k-mooney: Oh ok, I get it now. Then I can just do that (ie: do timestamp2 = timestamp+1)
12:44:27 zigo Though it may be harder than just mocking the timestamp function, no? I don't know how the updated_at is generated.
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

Earlier   Later