| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-09-12 | |||
| 07:15:40 | gibi | supamatt: I have doubts about the problem the code pointed out by the report and the fix does not effect the setting of the destroy_disks variable that is reponsible to signal deletion of files from the disk at cleanup | |
| 07:23:13 | gibi | it seems nobody is around so I will update the bug report with my doubt/questions | |
| 07:43:43 | gibi | done | |
| 07:47:21 | bauzas | gibi: ack, sorry, I had trouble with my home network | |
| 07:48:21 | gibi | bauzas: no worries | |
| 07:48:49 | bauzas | gibi: this is on 2024.1 | |
| 07:48:55 | bauzas | Caracal so | |
| 07:49:04 | gibi | bauzas: it was a backport from master | |
| 07:49:07 | gibi | merged this cycle | |
| 07:49:12 | bauzas | technically, we could deliver RC1 and then backport that one in a .z release | |
| 07:49:28 | gibi | https://review.opendev.org/c/openstack/nova/+/909806 | |
| 07:49:29 | bauzas | okay, lemme look at the commit SHA1 | |
| 07:49:40 | gibi | that is the offending patch according to the reporter | |
| 07:50:29 | gibi | bauzas: I lean towards that the reporter do see a numa live migration regression on shared storage but I'm not sure that the root cause is the power mgmt flag added. But more like that flag now trigger a driver cleanup call that wasn't triggered before | |
| 07:50:49 | gibi | and that cleanup call might have issues | |
| 07:50:58 | bauzas | agreed | |
| 07:51:02 | bauzas | that's the cleanup | |
| 07:53:01 | bauzas | gibi: oh, that's the conditional | |
| 07:53:16 | bauzas | we only cleanup if we don't have shared path | |
| 07:53:28 | bauzas | but here, the 'or' makes it now optional | |
| 07:53:47 | bauzas | we just need to change the conditional | |
| 07:56:04 | gibi | calling the cleanup with destroy_disks should be OK | |
| 07:56:17 | gibi | destroy_disk= false | |
| 07:57:23 | gibi | appreantly it isn't OK but changing the logic at when to trigger do_cleanup seems to be the wrong fix to me | |
| 07:57:45 | gibi | as destroy_disk=false should prevent the disk deletion | |
| 07:57:54 | bauzas | true | |
| 07:58:19 | bauzas | but we check two different fields for shared storage | |
| 07:58:34 | bauzas | the conditional checks whether both computes use the same path | |
| 07:58:36 | gibi | yes I spotted that but did not dig deeper tehre | |
| 07:59:05 | bauzas | while the destroy_disks arg checks whether we use block storage | |
| 07:59:38 | bauzas | I assume the reporter gets something like True for the shared path but False for shared block storage | |
| 08:01:36 | gibi | what is a shared block storage? | |
| 08:01:41 | bauzas | for mdevs and vpmems, there was a reason to call cleanup | |
| 08:01:45 | gibi | just cinder? or something more special? | |
| 08:01:55 | bauzas | we need to delete the local residues | |
| 08:02:24 | bauzas | honestly, that's the first time I'm hearing this | |
| 08:02:45 | bauzas | I'm used to the regular FS share between instance pathes | |
| 08:03:01 | bauzas | that's the regular shared storage I know | |
| 08:03:28 | gibi | yeah I think is_shared_instance_path is the case when the instance dir is on e.g. NFS | |
| 08:03:41 | bauzas | yup | |
| 08:04:24 | gibi | we need to call cleanup if power mgmt is enabled so the current fix proposed would regress that | |
| 08:04:38 | bauzas | I need to see what makes migrate_data.is_shared_block_storage | |
| 08:04:54 | gibi | yep | |
| 08:05:02 | bauzas | and it would also regress the mdev live-migration support | |
| 08:05:14 | bauzas | we need to call cleanup to delete the mdevs we reserved | |
| 08:05:32 | bauzas | hence the 'or' | |
| 08:06:00 | bauzas | I need a coffee | |
| 08:06:14 | bauzas | it's early and I'm already fried | |
| 08:06:45 | bauzas | trying to improve my local LTE connection took me too much of time | |
| 08:08:39 | gibi | https://github.com/openstack/nova/blob/16d815990b53b9afa35fc4da38609f87820fa690/nova/virt/libvirt/driver.py#L10367-L10369 | |
| 08:09:35 | bauzas | ooooh I see | |
| 08:09:42 | gibi | https://github.com/openstack/nova/blob/16d815990b53b9afa35fc4da38609f87820fa690/nova/virt/libvirt/driver.py#L10470 | |
| 08:10:25 | bauzas | yeah, some this is when you have BFV or you share the same libvirt image backend between both | |
| 08:10:33 | bauzas | that's not NFS | |
| 08:11:13 | gibi | yeah it is the RBD image backend | |
| 08:11:20 | bauzas | so | |
| 08:11:27 | bauzas | I think of something quick | |
| 08:11:50 | bauzas | previously, we were NOT calling cleanup on shared storage | |
| 08:12:02 | bauzas | right? | |
| 08:12:25 | bauzas | now we WANT to call it too, but we don't want to destroy the disks | |
| 08:12:35 | bauzas | I see a simple solution | |
| 08:13:06 | bauzas | which is to add another clause to destroy_disks | |
| 08:13:25 | bauzas | gibi: thoughts ? | |
| 08:14:09 | gibi | thinking... | |
| 08:14:49 | gibi | we did not call cleanup if is_shared_instance_path was true. And now we need it due to power mgmt. | |
| 08:14:58 | bauzas | yup | |
| 08:15:06 | bauzas | but we need to call it smartly | |
| 08:15:09 | gibi | yes | |
| 08:16:27 | gibi | would the change destroy_disks = not (migrate_data.is_shared_block_storage or migrate_data.is_shared_instance_path) work? | |
| 08:16:42 | bauzas | that's my hope | |
| 08:16:54 | bauzas | but we need to check the cleanup method | |
| 08:20:24 | gibi | https://github.com/openstack/nova/blob/16d815990b53b9afa35fc4da38609f87820fa690/nova/virt/libvirt/driver.py#L1707-L1724 this is the relevant path. I think it would work | |
| 08:20:33 | gibi | but we don't have reproduction | |
| 08:21:35 | bauzas | yeah | |
| 08:21:42 | bauzas | we need a functional test | |
| 08:22:14 | gibi | In 20 mins I need to step away for more than an hour | |
| 08:22:23 | bauzas | I can try to work on the reproducer | |
| 08:22:30 | gibi | I will summarize the above discussion in the bug | |
| 08:22:37 | bauzas | I already did in the report | |
| 08:23:13 | gibi | cool | |
| 08:23:37 | gibi | I will add a request to the reporter to try,if they can, our fix proposal | |
| 08:23:59 | bauzas | please, but I assume he's on West-US timezones | |
| 08:36:12 | gibi | done | |
| 10:28:25 | opendevreview | Sylvain Bauza proposed openstack/nova master: Add Dalmatian prelude section https://review.opendev.org/c/openstack/nova/+/928995 | |
| 10:28:34 | opendevreview | Merged openstack/osc-placement master: Update master for stable/2024.2 https://review.opendev.org/c/openstack/osc-placement/+/928360 | |
| 11:32:56 | yusufgungor_ | Also debug logs shows that mtu is null for one of the interfaces for the failed scenarios. _update_vif_xml function prints log as below on debug: | |
| 11:32:56 | yusufgungor_ | "Target network card MTU 0 does not match source 1450: libvirt.libvirtError: unsupported configuration: Target network card MTU 0 does not match source 1450" | |
| 11:32:56 | yusufgungor_ | hi everyone, we think hit a strange bug. We got the mtu error as below on the source compute node, when live migrating an instance with the specified scenarios. | |
| 11:32:58 | yusufgungor_ | Updating guest XML with vif config: <interface type="ethernet"> | |
| 11:33:00 | yusufgungor_ | <model type="virtio"/> | |
| 11:33:00 | yusufgungor_ | <mac address="fa:16:3e:19:a8:ba"/> | |
| 11:33:02 | yusufgungor_ | </interface> | |
| 11:33:02 | yusufgungor_ | <target dev="tapfb1b3816-ed"/> | |
| 11:33:04 | yusufgungor_ | Updating guest XML with vif config: <interface type="ethernet"> | |
| 11:33:06 | yusufgungor_ | <model type="virtio"/> | |
| 11:33:06 | yusufgungor_ | <mac address="fa:16:3e:8e:e7:b3"/> | |
| 11:33:08 | yusufgungor_ | <target dev="tapd001bd30-9c"/> | |
| 11:33:08 | yusufgungor_ | <mtu size="1500"/> | |
| 11:33:10 | yusufgungor_ | </interface> | |
| 11:33:12 | yusufgungor_ | Should we create a bug report? This mtu error causes revert the migration and triggers another strange bug on the os-brick side which unmaps the incorrect iscsi device for random instance :/ It is another story. | |