| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-09-13 | |||
| 13:48:21 | supamatt | If you guys wouldn't mind adding that UT, that'd be awesome. This patch seems like a big regression. I've done a number of openstack deployments over the last decade and NFS being used as shared storage is in almost a quarter of those deployments. | |
| 13:49:16 | gibi | supamatt: sure, I can propose the UTs | |
| 13:49:46 | elodilles | bauzas: ACK, sounds good | |
| 13:50:20 | sean-k-mooney | frickler: sorry was gone for lunch with some local readhatter | |
| 13:50:21 | bauzas | elodilles: I'll +1 the placement RC1 | |
| 13:50:29 | elodilles | thanks o/ | |
| 13:50:46 | elodilles | we still have 2 weeks until the final RC deadline o:) | |
| 13:50:55 | sean-k-mooney | frickler: so the password shoudl be valid for the requested microversion but i think i agree that just not passing it when its not preseent on the command line will work around it | |
| 13:51:07 | sean-k-mooney | but there might also be bugs in the sdk about that | |
| 13:51:11 | elodilles | but let's hope RC1 will be enough :] fingers crossed :X | |
| 13:54:23 | frickler | sean-k-mooney: the real issue is that the parameter is named admin_password in the sdk, that change was missed while switching evacuate to use the sdk | |
| 13:55:09 | frickler | the sdk always sends none in the request anyway, so I reverted that part in PS2 | |
| 13:55:25 | sean-k-mooney | well the real issue is its named adminPassword in the api and the sdk and clinet were both calling it somethign else | |
| 13:55:51 | sean-k-mooney | frickler: the sdk has historically renamed the parmater form there real api names | |
| 13:56:06 | sean-k-mooney | which causes a lot of similear bug that will hopefully stop in the futrue | |
| 13:56:38 | sean-k-mooney | if we complete the openapi stuff and starge generaging parts of the sdk form the openapi definitions | |
| 13:57:08 | sean-k-mooney | actully its adminPass in the api | |
| 13:58:24 | artom | gibi, bauzas, shouldn't mdev and vpmem live migration have uncovered the problem before? They're also cases where we started calling cleanup, but the destroy_disks logic was incorrect? | |
| 13:59:04 | bauzas | maybe | |
| 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 | do_cleanup = (not migrate_data.is_shared_instance_path or | |
| 14:00:13 | artom | has_vpmem or has_mdevs) | |
| 14:00:24 | bauzas | while for mdev, it was only if you were migrating the mdevs | |
| 14:00:24 | sean-k-mooney | oh actully no becasue it need nfs too | |
| 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 | |