| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-09-13 | |||
| 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 | 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... | |