Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2024-03-19
19:52:13 sean-k-mooney in theory rene's code should be runnign even if that is not devied but we should be able to test this in the gate anyway
19:59:42 opendevreview Stephen Finucane proposed openstack/nova master: Make overcommit check for pinned instance pagesize aware https://review.opendev.org/c/openstack/nova/+/631053
20:01:56 dansmith sean-k-mooney: yeah, I'm unsure either, but I'm positive that means we're just not running the code we think we're running
20:02:02 opendevreview ribaudr proposed openstack/nova master: Fix: Live migrating to a host with cpu_shared_set configured will now update the VM's configuration accordingly. https://review.opendev.org/c/openstack/nova/+/903706
20:02:02 opendevreview ribaudr proposed openstack/nova master: Fix [backport]: Live migrating to a host with cpu_shared_set configured will now update the VM's configuration accordingly. https://review.opendev.org/c/openstack/nova/+/877773
20:07:51 sean-k-mooney dansmith: my guess right now is we are never populating the numa toplogy info in the migrate_data
20:08:18 dansmith sean-k-mooney: you mean not running a test that causes it to be populated right?
20:08:46 dansmith I mean...not running a test _case_ where it would be populated and that code path exercised
20:10:40 sean-k-mooney no i ment not seting the object that had the new data at all but we appear to be
20:10:54 artom sean-k-mooney, dansmith, hrmm, I wonder if we're fine because... let my type this out
20:11:08 artom The NUMA live migration code kicks in only if the instance has a NUMA topology
20:11:39 artom Uggla's patch _always_ adds that hidden item in the dst_numa_info
20:12:19 sean-k-mooney so it alwasy uses the numa migration code?
20:12:42 artom Hold on
20:13:50 artom No, I thought I had it...
20:16:11 sean-k-mooney the fact we have to have this converstaion and its not obvious how its working after a few of us lokking at it is a code smell to me in either case
20:16:31 sean-k-mooney the code might be correct but its to complex to debug
20:18:31 artom So to trigger it, all that should be required in live migration a NUMA VM to a dest with cpu_shared_set, right?
20:19:10 artom That would cause Uggla's patch to stash the cpu_shared_set in the "hidden" key in dst_numa_info, and *should* cause the update XML code to trip over and explode?
20:19:26 artom We've _definitely_ done that ^^ in whitebox, and nothing has exploded
20:19:42 sean-k-mooney artom: no
20:19:56 sean-k-mooney to trigger the bug yuou need to migate a non numa vm
20:20:06 sean-k-mooney oh
20:20:17 sean-k-mooney you mena the the driver issue
20:20:37 artom Yeah, the thing that we're trying to understand how it even works
20:20:39 sean-k-mooney yes if the loop over the keys was broken then you woudl have to migrate a pinned instance
20:22:06 sean-k-mooney right but since this is alwasy set https://review.opendev.org/c/openstack/nova/+/877773/10/nova/virt/libvirt/driver.py#10028
20:22:32 sean-k-mooney any numa instance should triger this code path
20:22:38 artom Exactly
20:23:05 artom I'm expecting migrate_data.dst_numa_info.cpu_pins["dst_cpu_shared_set_info"] to always be there
20:25:05 artom Ohhhhh
20:25:57 artom We just clobber the entire object for NUMA live migration: https://opendev.org/openstack/nova/src/branch/master/nova/virt/libvirt/driver.py#L10106
20:26:22 sean-k-mooney but again numa instance
20:26:24 artom And that ^^ happens later in the flow
20:26:32 sean-k-mooney sorry that was in my buffer
20:26:48 sean-k-mooney ya that would make sense
20:27:08 sean-k-mooney as that is where it woudl normally be created
20:27:25 artom That's after we make the claim on the dest, and Uggla's code was adding its thing earlier in check_can_live_migrate_destination
20:27:58 artom So it just gets overwritten
20:28:33 sean-k-mooney is it in check_can_live_migrate_destination i tought it was in pre_live_migration (at dest)
20:28:44 dansmith but as sean-k-mooney said now and I said earlier, this is hiding demons in the details on so many levels, especially since this transforms to a real object property in the next patch
20:29:14 dansmith artom: just a thought - are you running the tests you expect to pass now on the lower patch or only the stack? if the latter, then this code is "gone" by then right?
20:29:36 artom sean-k-mooney, https://review.opendev.org/c/openstack/nova/+/877773/10/nova/virt/libvirt/driver.py#9958
20:29:45 sean-k-mooney oh its in check_can_live_migrate_destination in the driver i was thinking of the conductor method with a similer name
20:29:58 artom dansmith, definitely the backportable one
20:30:06 dansmith okay just checking
20:30:12 artom dansmith, but I think we've figured it out :)
20:31:39 dansmith yeah, but I haven't figured out yet if you're thinking that we're not covering the case where we would trip over that code, or if they're mutually exclusive somehow (not that it makes it okay)
20:32:01 sean-k-mooney well at a minium this show we are missign a funtional test to demonstrate that the xml is updated properly between the srouce and destinatnio hosts
20:33:26 artom dansmith, they're not mutually exclusive, they definitely both run, and the NUMA live migration code runs _after_ the hack, so it clobbers the entire dst_numa_info object that was used as the hiding place for the "secret" cpu shared set info
20:34:18 dansmith I don't understand how that can be
20:34:26 artom dansmith lulz another call ^_^ ?
20:34:31 sean-k-mooney its because of the 3 conditions in the if
20:34:34 dansmith we have to put the dst info into the blob so it can be used to generate the xml right?
20:34:36 sean-k-mooney if ('cpu_pins' in info and
20:34:38 sean-k-mooney 'cell_pins' in info and
20:34:40 sean-k-mooney 'emulator_pins' in info):
20:34:52 sean-k-mooney in the non numa case only one of those 3 in checks is true
20:35:00 sean-k-mooney so we never enter the loop
20:35:08 sean-k-mooney https://review.opendev.org/c/openstack/nova/+/877773/9/nova/virt/libvirt/migration.py#186
20:35:28 sean-k-mooney in the numa case the numa live migration code clobers the object that rene modifed
20:36:05 sean-k-mooney so it just happens to work becasue we get lucky and the case where its not clobpered we dont enter the loop
20:36:29 dansmith and to be clear, that's always the case or because we're missing a test scenario where we could get unlucky?
20:36:31 sean-k-mooney cell_pins and emulator_pins are not defiend in the non numa case
20:36:36 artom The case where it's not clobbered is when a VM doesn't have a NUMA topoogy
20:36:41 dansmith I guess the point is. yeah that ^
20:36:53 sean-k-mooney thats alwasy the case
20:37:07 artom So by pure luck we've made them functionally mutually exclusive
20:37:14 sean-k-mooney when we dont have a numa guest cell_pins and emulator_pins are not set
20:38:09 sean-k-mooney and in the numa case the dst_numa_info is regenerated by the numa live migration code
20:39:12 sean-k-mooney so we never execute _update_numa_xml with cpu_pins that contains the stashed value
20:39:24 dansmith usually when I say "this will be a nightmare of complexity to debug in the future" I have to wait years to collect on that debt.. glad this time it was under 24 hours :D
20:39:58 sean-k-mooney me too because i proably would have been the one trying to fiugre this out in a few years :)
20:40:05 dansmith yep
20:41:06 artom In a hypothetical few years wherein we've chosen to merge the haxx ;)
20:41:30 dansmith s/we've/we'd/ yeah
20:41:31 sean-k-mooney Uggla: have you had time to sync with artom on the change of direction
20:45:15 sean-k-mooney i need to go eat so ill call it a day there
20:45:27 Uggla sean-k-mooney, more or less. But I guess Artom and I will sync tomorrow....
20:45:31 sean-k-mooney im glad we know why it was working im not happy with how it was working
20:51:48 spatel_ sean-k-mooney I believe I talked about this in past. Just re-checking again. Does snapshot support include of memory footprint?
#openstack-nova - 2024-03-20
02:44:17 opendevreview Merged openstack/osc-placement master: Update master for stable/2024.1 https://review.opendev.org/c/openstack/osc-placement/+/912623
02:45:37 opendevreview Merged openstack/os-vif master: Update master for stable/2024.1 https://review.opendev.org/c/openstack/os-vif/+/912620
02:48:32 opendevreview Merged openstack/python-novaclient master: Update master for stable/2024.1 https://review.opendev.org/c/openstack/python-novaclient/+/912627
03:58:06 opendevreview Merged openstack/placement master: Update master for stable/2024.1 https://review.opendev.org/c/openstack/placement/+/913350
04:00:00 opendevreview Merged openstack/nova master: Update master for stable/2024.1 https://review.opendev.org/c/openstack/nova/+/913633
09:55:31 opendevreview sean mooney proposed openstack/nova master: Test live migration between hosts with differnet cpu_shared_sets https://review.opendev.org/c/openstack/nova/+/913744
10:03:50 sean-k-mooney[m] spatel_: no snapshot is just of the root disk.
10:04:09 sean-k-mooney[m] not memory
13:21:47 sean-k-mooney dansmith: just an fyi i added you to this anyway https://review.opendev.org/c/openstack/nova/+/913744 but that is proply executing Uggla migration change and testing it in a way we i think can merge and keep with little to no downside
13:23:16 sean-k-mooney its proably a converstaion for the ptg but i think there are a limited subset of whitebox test we may want to conser adding to exsitng jobs. for example we have a non voting barbican-tempest-plugin-simple-crypto job today.
13:24:44 sean-k-mooney i would like to either make that voting or have a diffent voting barbican job to test the ephmeral encyption series when taht is complete but i would alos like to enable the whitebox vtpm test in that job too. we dont need to enabel all of whitebox but it hink we can enable select tests in specific jobs if we agree that is desireable
13:26:00 sean-k-mooney i would like to add a whitebox tempest test to validate Uggla change at some point (i.e. checking the xml is updated correctly) and enable that in the live migration job if peopel were ok with that.
13:27:07 sean-k-mooney given whitebox is an offical deliverable of the qa project i dont think that is out of place but if we were to take that step we would certenly need to ensure that whitebox does not break teh nova gate going forward.
13:27:47 sean-k-mooney to that end if have also been explofing using the rdo zuul instance to instead execute the whitebox tests as a third party ci for nova
13:28:15 sean-k-mooney i proably wont have time to create a poc of that between now and the ptg
13:29:16 sean-k-mooney but im going to explore that option regardless of of we do decied to integrate a subset of whitebox in the first party ci or not
13:30:21 dansmith I'm not sure what you're trying to say... this tests mix of shared/pinned that you said we don't want to support anyway, or some other scenario?
13:30:38 sean-k-mooney dansmith: another senairo
13:31:03 sean-k-mooney specificly i want a test for two host with diffen cpu_shared_set and numa or cpu pinning

Earlier   Later