Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2025-12-04
08:00:48 ratailor In my local devstack env, I could get the instance action finish_time for create and attach_volume actions here https://paste.openstack.org/show/bWzmPfwfOZ9FQpfCpVPm/
08:01:17 ratailor bauzas, ack.
08:01:31 ratailor gmaan, ^^
08:06:19 bauzas ratailor: I'm not really a tempest expert about schema validations
08:06:50 bauzas I saw the patch that got -1 review.opendev.org/c/openstack/tempest/+/953331 but honestly I don't know why
08:07:15 ratailor bauzas, ack. I think stephenfin can help here. ^^
08:07:23 bauzas I see you're depending on the nova patches
08:07:52 bauzas so everything looks correct to me at a first glance
08:08:02 ratailor bauzas, could you please review nova-spec and implementation and suggest if anything is missing.
08:08:35 bauzas as I said, I'll do another round of open specs today
08:09:02 gibi Details: HTTP response body is invalid (Additional properties are not allowed ('finish_time' was unexpected)
08:09:18 gibi so I assume your nova patch added a new field to the response
08:09:22 gibi that was not there before
08:09:26 ratailor gibi, yes
08:09:43 gibi and the tempest schema wasn't updated to accept that field in the new microvresion
08:21:24 ratailor gibi, this is what I have done to update tempest schema
08:21:26 ratailor https://review.opendev.org/c/openstack/tempest/+/953331/7/tempest/lib/api_schema/response/compute/v2_101/servers.py
09:00:23 jkulik ratailor: looks like you're overwriting your changes to show_instance_action in line 19 again in line 40
09:13:47 ratailor jkulik, nice catch, thanks!
09:36:53 zigo https://ci.debian.net/packages/n/nova/testing/ppc64el/66782181/
09:36:53 zigo Looks like I have 2 unit tests failing in Nova's autopkgtest because our ppc64el servers are too fast, no ?
09:36:53 zigo Hi there!
09:39:44 zigo gibi: ^ Your thoughts?
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

Earlier   Later