| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-09-13 | |||
| 09:17:11 | gibi | "making the VM unusable for future operations." | |
| 09:17:36 | gibi | I need to drop for an hour but will continue from here | |
| 09:17:48 | bauzas | 🤷 | |
| 09:18:38 | bauzas | sure, let's continue to discuss this later once more people are here | |
| 09:19:21 | gibi | one thing we can suggest is to only delete the instance dir if the instance is volume backed but the instance path is not shared | |
| 09:19:45 | gibi | I can amend the my suggested patch with that logic | |
| 09:19:49 | gibi | and they can test | |
| 09:31:17 | bauzas | yeah, seems a good point | |
| 09:31:41 | bauzas | but this would revert the previous logic :) | |
| 10:01:44 | elinux | any one available here ? | |
| 10:08:30 | elinux | Anyone online | |
| 10:08:36 | supamatt | o/ | |
| 10:41:58 | frickler | stephenfin: sean-k-mooney: https://review.opendev.org/c/openstack/python-openstackclient/+/929236 should fix the second part of the evacuate command, not sure about the microversion thing yet | |
| 10:45:38 | sean-k-mooney | frickler so i think this is an sdk regression? | |
| 10:46:53 | sean-k-mooney | i guess it was intorduced with https://github.com/openstack/python-openstackclient/commit/e6dc0f39c0891ea551867b77663a463b5f76987c perhaps | |
| 10:48:03 | sean-k-mooney | frickler: the actul password handeling code has not changed in over a year | |
| 10:48:57 | sean-k-mooney | but the args are now being passed to an sdk client instead of nova client | |
| 10:50:00 | opendevreview | Matthew Heler proposed openstack/nova master: Fix regression with live migration on shared storage https://review.opendev.org/c/openstack/nova/+/928970 | |
| 10:50:37 | sean-k-mooney | frickler: the microversion we used is https://docs.openstack.org/nova/latest/reference/api-microversion-history.html#id62 | |
| 10:50:47 | sean-k-mooney | which removed foraced evacuates | |
| 10:51:44 | supamatt | bauzas the latest patch I posted for 928970 works with shared storage now | |
| 10:52:53 | sean-k-mooney | frickler: the password is adminPass | |
| 10:53:00 | sean-k-mooney | in the rest api | |
| 10:54:31 | opendevreview | Matthew Heler proposed openstack/nova master: Fix regression with live migration on shared storage https://review.opendev.org/c/openstack/nova/+/928970 | |
| 10:59:02 | gibi | supamatt: nice, I think that was what I imagined above by "one thing we can suggest is to only delete the instance dir if the instance is volume backed but the instance path is not shared" | |
| 10:59:57 | gibi | supamatt: so the patch you posted proves my theory. | |
| 11:00:38 | gibi | lets wait for bauzas as well but I think we can make your patch mergeable by adding some unti tests around it and a release notes | |
| 11:05:39 | supamatt | +1 | |
| 11:13:06 | frickler | sean-k-mooney: yes, I think that that is a regression that was missed in the commit you mention | |
| 11:13:41 | gibi | supamatt: if you need pointers / help to add unit tests and a release notes then let me know | |
| 11:35:20 | supamatt | gibi: for the unit test, I'm not sure how best to do that. the release notes I can do | |
| 11:51:43 | gibi | supamatt: for he compute manager change you can extend the unit test coverage here based on the existing examples https://github.com/openstack/nova/blob/16d815990b53b9afa35fc4da38609f87820fa690/nova/tests/unit/compute/test_compute_mgr.py#L11385-L11448 | |
| 11:54:03 | opendevreview | Matthew Heler proposed openstack/nova master: Fix regression with live migration on shared storage https://review.opendev.org/c/openstack/nova/+/928970 | |
| 11:54:15 | gibi | supamatt: for the driver change here is an example test case you can use https://github.com/openstack/nova/blob/16d815990b53b9afa35fc4da38609f87820fa690/nova/tests/unit/virt/libvirt/test_driver.py#L20993-L21013 | |
| 12:59:37 | bauzas | sorry, was offline due to some network change | |
| 13:00:14 | bauzas | supamatt: gibi : can you please give me a summary? | |
| 13:02:37 | gibi | bauzas: supamatt: basically implemented what I described before I dropped this morning. | |
| 13:03:10 | gibi | So the patch now only deletes the instance dir if the instance is booted from volume but the instance directory is not on shared storage | |
| 13:03:21 | gibi | supamatt: stated that this solves their issue | |
| 13:03:29 | gibi | and it is aligned with my understanding | |
| 13:03:42 | gibi | now supamatt works on unit test coverage for the change | |
| 13:08:15 | bauzas | excellent then | |
| 13:08:31 | bauzas | I tried to work on a functest but wasn't simple | |
| 13:09:00 | bauzas | but we can try to write a specific shared storage test if we want in another patch | |
| 13:09:21 | gibi | given the time pressure and the fact that we have first hand info from the reporter that it solves the regression I'm OK not to have functional reproduce for this | |
| 13:09:46 | gibi | if we can add functional test later that is fine too | |
| 13:10:01 | bauzas | yup | |
| 13:10:18 | bauzas | but yeah, I think we need some way to check shared storage by a functest | |
| 13:15:04 | elinux_ | I have observed somethign while testing the nova api calls. after the request comes in , the request spec object is built. after that I have placed a debugger at the scheduler level. after the pdb prompt appears, by that time when I inspect the spec obj, it is loosing the metadata key,value if passed by the user. but when I dont use the debugger and use only a simple print statement , it is printing the metadata value | |
| 13:15:31 | elinux_ | os my question is , is there a time span after which the data within that object is removed ? | |
| 13:15:39 | elinux_ | *so my | |
| 13:36:08 | gibi | elinux: probably the rpc all to the scheduler is timed out while you are debugged the scheduler | |
| 13:45:00 | elinux | gibi: but all other details r intact for the request. Only metadata is missing | |
| 13:45:51 | supamatt | bauzas gibi: yea the unit test doesn't look simple to me either ;S | |
| 13:46:22 | gibi | supamatt: If you are stuck I can try to create a follow up patch top of yours with the unit tests | |
| 13:46:33 | bauzas | supamatt: I was talking of functional tests which are in another directory | |
| 13:46:44 | bauzas | yes, we can help on the UTs | |
| 13:46:58 | bauzas | have you found the test methods to update ? | |
| 13:47:47 | bauzas | we need another core to chime in https://review.opendev.org/c/openstack/nova/+/928995 | |
| 13:48:20 | bauzas | elodilles: I think we will reasonable release nova RC1 by monday | |
| 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 | |