| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-09-13 | |||
| 13:59:44 | sean-k-mooney | either of those are tested in ci currently | |
| 13:59:49 | sean-k-mooney | *neitehr | |
| 13:59:52 | bauzas | but here, we now call cleanup everytime if you have dst_numa_info | |
| 14:00:08 | sean-k-mooney | so i have a ci job that will test this we just have not merged it yet | |
| 14:00:12 | artom | Right, but before we had this: | |
| 14:00:13 | artom | has_vpmem or has_mdevs) | |
| 14:00:13 | artom | do_cleanup = (not migrate_data.is_shared_instance_path or | |
| 14:00:24 | sean-k-mooney | oh actully no becasue it need nfs too | |
| 14:00:24 | bauzas | while for mdev, it was only if you were migrating the mdevs | |
| 14:00:32 | artom | So we called cleanup every time we had vpmem or mdevs | |
| 14:00:42 | sean-k-mooney | yes | |
| 14:00:45 | artom | So if any of those were on NFS, they would see their disk deleted as well? | |
| 14:00:49 | gibi | artom: I'm fairly certain that mdev + shared storage would trigger it too | |
| 14:00:56 | sean-k-mooney | because we do need to clean up the persitent memeory | |
| 14:00:57 | artom | And we just never caught it? | |
| 14:01:12 | sean-k-mooney | we did not supprot mdev live migation until caracal | |
| 14:01:16 | bauzas | we're calling it for mdevs because we do something in cleanup about mdevs | |
| 14:01:25 | artom | Right, I understand _why_ we need to call it | |
| 14:01:43 | gibi | artom: I think we don't have much coverage with NFS under nova instances path | |
| 14:01:45 | sean-k-mooney | my point is that we have no test coverage of any of the condtion in ci today | |
| 14:01:46 | artom | I'm saying it's suspect that power management live migration is the first thing to have caught the fault destroy_disks logic | |
| 14:01:54 | sean-k-mooney | we dont have coverage fo mdev or pmem in ci | |
| 14:02:07 | artom | OK, mdev is new in Caracal, but vpmem? That's been around for a while... | |
| 14:02:10 | gibi | artom: correct the power mgmt condition was vide enough to hit people | |
| 14:02:26 | bauzas | rather the problem is that we don't really test shared instances | |
| 14:02:30 | sean-k-mooney | artom: intel killed pmem remeber | |
| 14:02:31 | artom | gibi, OK, that makes sense I guess. | |
| 14:02:33 | gibi | as it hits all deployments with NFS + numa | |
| 14:02:55 | sean-k-mooney | dansmith yep frickler has a possibel fix | |
| 14:02:59 | dansmith | (stupid client auto /me'ing) | |
| 14:03:06 | sean-k-mooney | https://review.opendev.org/c/openstack/python-openstackclient/+/929236 | |
| 14:03:07 | bauzas | migrating instances with mdevs is not a problem if you don't have shared storage | |
| 14:03:13 | artom | I'm always terrified when I'm touching that code. I had to do it with vTPM live migration as well | |
| 14:03:16 | dansmith | sean-k-mooney: okay it seemed relevant to your discussion above but I wasn't sure | |
| 14:03:18 | gibi | artom: the fix propsed will work for not just NUMA but also for mdev and vpmem instances too | |
| 14:03:32 | bauzas | in order to see this problem, you would need to have the CI checking *both* mdevs *and* shared storage | |
| 14:03:36 | artom | gibi, right, because power management isn't the cause, it just happened to have exposed the bug | |
| 14:03:42 | gibi | yes | |
| 14:03:55 | bauzas | because we default to check *every* numa instance | |
| 14:04:06 | gibi | yepp | |
| 14:04:25 | artom | Maybe that check should have taken into account the power management config option :P | |
| 14:04:26 | sean-k-mooney | bauzas: we can test this in ci going forward | |
| 14:04:34 | bauzas | we could have another field in the migrate_data that would say "heh, my compute supports power management, so please do it too" | |
| 14:04:39 | sean-k-mooney | we will just do the numa live mgiration testing in a multi node nfs job | |
| 14:04:56 | sean-k-mooney | i was going to enable it in the ceph job | |
| 14:05:06 | sean-k-mooney | but i can just change which job i enable numa migration in | |
| 14:05:13 | artom | sean-k-mooney, so actually, would this bug have been visible in CI? The instance might remain ACTIVE even if its disk is gone... | |
| 14:05:35 | bauzas | right, we don't have any exceptions | |
| 14:05:38 | artom | We would need to do something like soft-reboot it after the live migration? | |
| 14:05:41 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/913842 | |
| 14:06:11 | sean-k-mooney | artom: it should have caused the instance to crash and caused the connectivy test to fail when we ssh in | |
| 14:06:25 | gibi | artom: in the volume booted case I think the instance will remain ACTIVE. For the shared storage case I don't know what happen when the local disk is deleted under it | |
| 14:06:35 | sean-k-mooney | we also have one broken the back and forth live migration | |
| 14:06:55 | artom | Ah, yeah, that would probably expose this | |
| 14:07:50 | artom | OK, I think I've saturated - I've literally had a feverish night tossing and turning because of some mystery illness one of the kids (presumably the baby) brought home. I'll do go dumb paperwork stuff :P | |
| 14:08:14 | bauzas | sean-k-mooney: does the ceph job check shared storage with cephfs ? | |
| 14:08:21 | sean-k-mooney | bauzas: no | |
| 14:08:24 | bauzas | or is it just rbd ? | |
| 14:08:37 | bauzas | okay, then we wouldn't see the bug | |
| 14:08:38 | sean-k-mooney | its images_type=rbd | |
| 14:08:41 | gibi | artom: take care | |
| 14:08:43 | bauzas | I see | |
| 14:08:48 | sean-k-mooney | right i said i need to create a new job for this | |
| 14:08:56 | sean-k-mooney | i tought we had an nfs job already | |
| 14:09:04 | bauzas | artom: joys of being parent of kids going back at school | |
| 14:09:05 | sean-k-mooney | we have - devstack-plugin-nfs-tempest-full: | |
| 14:09:19 | sean-k-mooney | in the experimatal line but not in check | |
| 14:09:51 | sean-k-mooney | bauzas: basically we have 2 chioices i can continue enabling numa testing in the ceph job | |
| 14:09:59 | bauzas | so you'd add numa live-migration tests in there ? | |
| 14:10:07 | sean-k-mooney | or i could pivort and create a shared storage migration job and put them in that | |
| 14:10:13 | bauzas | I'm cool but we're just adding more tests to the CI | |
| 14:10:23 | sean-k-mooney | we dont have a job that would catch this currently | |
| 14:10:32 | sean-k-mooney | we do not have any testing with nova on shared storage | |
| 14:11:34 | sean-k-mooney | bauzas: so im saying i can create one and also put the numa stuff in it, or i can proceed with putting the numa stuff in teh ceph jobs and we can create a seperate nova-shared-stroage-migration job | |
| 14:12:22 | sean-k-mooney | bauzas: do you have a prefernce? | |
| 14:12:37 | bauzas | kill shared storage with fire ? | |
| 14:12:44 | sean-k-mooney | :) | |
| 14:13:18 | bauzas | you can have a shared image backend, why would you mind using NFS which would create you don't know ? | |
| 14:13:41 | sean-k-mooney | well i cant argue agaisnt that but we coudl certenly make it less teribad in the future if we included this as part of the image backend refactor | |
| 14:13:53 | bauzas | another good case, yes | |
| 14:14:08 | sean-k-mooney | like if we had image_typs=nfs that would simplfy things a lot | |
| 14:15:21 | sean-k-mooney | bauzas: lets pause this dicussion for now and we can loop back to the tempest coverage after rc1 | |
| 14:17:03 | sean-k-mooney | bauzas: https://github.com/openstack/devstack-plugin-nfs currently just supprot seting up cinder on nfs so to test this i woudl eiter need to extend that or just put it in the nova devstack plugin which is honestly simpler | |
| 14:18:34 | sean-k-mooney | when we have a little more time ill try and figure out which job to extend to give us the most test coverage without needing extra josb | |
| 14:21:13 | sean-k-mooney | im thinkg of using nova-ovs-hybrid-plug to test shared storage and numa and then just leave the ceph job as is | |
| 14:25:33 | sean-k-mooney | summerised that here https://review.opendev.org/c/openstack/nova/+/913842/comments/ee7e862a_c1fd1b8c | |
| 14:26:25 | sean-k-mooney | frickler: is there a dnm that testing your openstack client patch | |
| 14:27:02 | sean-k-mooney | frickler: if not i need to rebase one of my patches to adress a nit anyway so i might add a depends on to it | |
| 14:56:47 | gibi | bauzas: which way you want to land the shared storage fix: a) I add the UT to supamatt's commit directly or b) I push the UT separately and then I can +2 the commit with the fix still | |
| 14:58:56 | bauzas | can we squash both ? | |
| 14:59:13 | bauzas | you can +2 the patch, I'm fine | |
| 14:59:25 | bauzas | I'll explain in my comment why it's fine | |
| 14:59:40 | bauzas | (and I want to stop by the top of the hour :) ) | |
| 15:01:28 | gibi | OK I will push the squash in 5 mins | |
| 15:08:12 | opendevreview | OpenStack Release Bot proposed openstack/placement stable/2024.2: Update .gitreview for stable/2024.2 https://review.opendev.org/c/openstack/placement/+/929290 | |
| 15:08:13 | opendevreview | OpenStack Release Bot proposed openstack/placement stable/2024.2: Update TOX_CONSTRAINTS_FILE for stable/2024.2 https://review.opendev.org/c/openstack/placement/+/929291 | |
| 15:08:15 | opendevreview | OpenStack Release Bot proposed openstack/placement master: Update master for stable/2024.2 https://review.opendev.org/c/openstack/placement/+/929292 | |
| 15:09:35 | opendevreview | Balazs Gibizer proposed openstack/nova master: Fix regression with live migration on shared storage https://review.opendev.org/c/openstack/nova/+/928970 | |
| 15:09:40 | gibi | bauzas: ^^ | |
| 15:09:45 | gibi | sorry it was 8 minutes | |