| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-09-13 | |||
| 08:37:31 | bauzas | ++ | |
| 08:37:43 | bauzas | (oops, wrong window) | |
| 08:40:15 | gibi | I will try to set up multinode devstack with NFS but I cannot promise it will work today | |
| 08:41:18 | bauzas | I have two nodes, I can also try to modify them | |
| 08:41:42 | bauzas | either way, we'll probably need to come to a decision today or monday | |
| 08:41:58 | bauzas | we can't delay RC1 more | |
| 08:42:24 | bauzas | but I'd like some core quorum | |
| 08:43:18 | gibi | an alternative is to revert the power mgmt live migration fix. Declare that that power mgmt does not work with live migration and fix the whole thing later | |
| 08:44:07 | bauzas | that's what I had in mind | |
| 08:44:26 | bauzas | or a third alternative is ship power management but let the call be optional | |
| 08:44:46 | bauzas | if you don't setup power management, you shouldn't be impacted | |
| 08:44:49 | gibi | we could hack a fix together where shared storage live migration works if power mgmt is not used, and power mgmt live migration works if no shared storage is used (basically the original proposal from the reported) but that is uggly | |
| 08:45:14 | bauzas | that's the concern I have with the current master, which inconditionnally says you're power manageable | |
| 08:45:14 | gibi | and power mgmt with shared storage will still not work | |
| 08:45:44 | bauzas | I wish we had a flag on the migrate_data object that would signal that info | |
| 08:45:53 | bauzas | like we did for mdevs or vpmems | |
| 08:46:27 | gibi | that would not remove the problem that shared storage live migration does not work if you have power mgmt, mdevs, or vpmems. | |
| 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. | |