| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-09-13 | |||
| 08:46:37 | gibi | as those would trigger calling clenaup | |
| 08:46:38 | bauzas | yup, but that's something we can doc | |
| 08:47:04 | bauzas | here the problem is that shared storage is broken *anyway* | |
| 08:47:20 | bauzas | and you can't opt out from it | |
| 08:48:28 | gibi | yepp it seem shared storage live migration only works we we don't call cleanup | |
| 08:48:48 | gibi | but I'm not 100% sure whw | |
| 08:48:50 | gibi | why | |
| 08:49:27 | gibi | do you have any comment on my las comment https://review.opendev.org/c/openstack/nova/+/928970/comment/c28b9ea3_30ed15a5/ | |
| 08:49:58 | gibi | especially around deleting the instance dir when the VM is volume backed? | |
| 08:51:50 | bauzas | even if the instance is volume backed, we still have an instance dir, right? | |
| 08:51:55 | bauzas | we just don't have disks | |
| 08:53:42 | bauzas | I could imagine that they have NFS on /var/lib/instances | |
| 08:53:57 | bauzas | so the BFV instances are present on that shared path | |
| 08:54:22 | bauzas | in that case, | |
| 08:54:25 | bauzas | cleanup_instance_dir = migrate_data.is_shared_block_storage | |
| 08:54:36 | bauzas | it will cleanup the dir, right ? | |
| 08:55:01 | bauzas | oh wait no | |
| 08:55:17 | bauzas | that's not shared storage, that's shared RBD image backend | |
| 08:55:44 | gibi | the instance dir for a volume booted instance (without config drive) only holds the console.log fiel nothing else | |
| 08:56:22 | bauzas | I don't disagree, but that's still an instance dir | |
| 08:56:23 | gibi | is_shared_block_storage will be true if the instance is booted from volume | |
| 08:56:44 | gibi | (and in other cases too) | |
| 08:56:54 | bauzas | okay, so that's why this is deleting the instance dir | |
| 08:56:59 | gibi | Is it a problem if we delete hte instance dir | |
| 08:57:03 | gibi | ? | |
| 08:57:08 | bauzas | good question | |
| 08:57:26 | bauzas | as you said, the instance dir is quite empty | |
| 08:57:38 | bauzas | where is stored the domain definition ? | |
| 09:01:51 | gibi | that is nova does not store the xml | |
| 09:02:21 | gibi | * nova does not store the xml | |
| 09:02:30 | gibi | I'm not sure libvirt stores it by default | |
| 09:03:21 | gibi | it is under /run/libvirt/qemu/instance-00000002.xml | |
| 09:03:31 | gibi | in devstack | |
| 09:04:36 | bauzas | okay | |
| 09:05:44 | bauzas | your question is actually good : does it harm if we delete an instance dir from a BFV instance that's shared ? | |
| 09:06:09 | bauzas | https://github.com/openstack/nova/blob/16d815990b53b9afa35fc4da38609f87820fa690/nova/virt/libvirt/driver.py#L1708-L1711 | |
| 09:06:24 | bauzas | I enjoy mdbooth's comment : "I'm pretty sure this is wrong" | |
| 09:07:24 | bauzas | from what I read from both comments is that this is EXPECTED that we delete the instance dir if the instance is BFV | |
| 09:07:44 | bauzas | not sure we do support shared storage with BFV | |
| 09:07:50 | bauzas | see ? | |
| 09:12:34 | gibi | I see | |
| 09:13:14 | gibi | I'm not sure wht mdbooth's comment really mean. I.e. does the comment really implies that the logic is wrong and we should change it? | |
| 09:16:19 | gibi | we need to figure out if live migration on shared storage with a volume backed VM actually fails if the instance dir is deleted | |
| 09:16:34 | gibi | we never seen the real failure from the reporter | |
| 09:16:45 | gibi | they just stated that the dir is deleted | |
| 09:17:06 | gibi | the only thing they said is | |
| 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 | |