| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-02-14 | |||
| 13:28:37 | sean-k-mooney | we added https://docs.openstack.org/nova/latest/configuration/config.html#libvirt.migration_inbound_addr | |
| 13:29:15 | ganso | sean-k-mooney: yep, that is the setting my customer is using to determine the migration network. The issue is that the setting is being ignored by the pre-live-migration step | |
| 13:29:39 | sean-k-mooney | ganso: migration_inbound_addr is somethign we are adding this cycle | |
| 13:29:57 | sean-k-mooney | so unless they are using master they are suign live_migration_inbound_addr | |
| 13:30:01 | ganso | sean-k-mooney: hmmmmmm let me confirm the naming, maybe it is similar to the other one I'm referring to | |
| 13:30:02 | sean-k-mooney | which is currently only for the libvirt part | |
| 13:31:00 | ganso | sean-k-mooney: yes! live_migration_inbound_addr | |
| 13:31:00 | ganso | sean-k-mooney: yes they are using libvirt | |
| 13:31:00 | sean-k-mooney | right so for cold migration there was no way to use a diffent netwrok before now | |
| 13:31:00 | ganso | sean-k-mooney: so the pre-live-migration code is using the wrong address, derived from the same way cold migration determine its address | |
| 13:31:00 | sean-k-mooney | yep | |
| 13:31:20 | sean-k-mooney | so on master you could "fix this" by settin gboth option to use the migration network | |
| 13:31:48 | sean-k-mooney | for older release it may be valid to change but it not valid to make cold migratoin use the live migration network | |
| 13:32:07 | sean-k-mooney | based on only live_migration_inbound_addr | |
| 13:32:14 | ganso | sean-k-mooney: I am not familiar with the patch code, I just found the fix proposal that I need, but I am a bit surprised that the fix needs an object versioning bump to make it use the correct network config instead of the wrong one | |
| 13:32:45 | ganso | sean-k-mooney: certainly, wouldn't make sense the change the cold-migration behavior, that indeed would be a new feature | |
| 13:32:48 | sean-k-mooney | so its not clear its using the wrong netwrok | |
| 13:33:02 | sean-k-mooney | techncially the exisitng option is only for the libvirt traffic | |
| 13:33:25 | ganso | sean-k-mooney: but fixing something that is a broken behavior in a previously accepted feature is kinda not really a new feature IMO | |
| 13:33:45 | sean-k-mooney | ganso: based on the docs for live_migration_inbound_addr | |
| 13:33:47 | sean-k-mooney | tis not broken | |
| 13:33:57 | sean-k-mooney | there expecation is incorrect based on teh docuemtned behavior | |
| 13:34:07 | sean-k-mooney | ganso: im not saying what you ask for is not an imporvement | |
| 13:34:24 | sean-k-mooney | im just sayting that is not what the docs say the config option does | |
| 13:34:34 | sean-k-mooney | """This option indicates the IP address which should be used as the target for live migration traffic when migrating to this hypervisor. This metadata is then used by the source of the live migration traffic to construct a migration URI.""" | |
| 13:34:46 | sean-k-mooney | copying the disk for block migration is not part of the live migration traffic | |
| 13:35:00 | ganso | well, pre-live-migration is part of live-migration, and if live-migration is not using live-migraiton-inbound-addr then it is somewhat diverging from expected behavior | |
| 13:35:08 | ganso | sean-k-mooney: oh copying the disk is not part of live migration traffic | |
| 13:35:20 | ganso | sean-k-mooney: I see, the devil is in the details xD | |
| 13:35:32 | sean-k-mooney | right this option is litrrally jsut for the data copied by libvirt | |
| 13:35:47 | sean-k-mooney | not the addtional actoins taken by nova | |
| 13:36:13 | sean-k-mooney | but i agree the enhacment that you are asking for makes sense btu that is why its kind of a feature not a bug | |
| 13:37:09 | ganso | sean-k-mooney: yea I understand the reasoning now. The fact that it changes the object versioning already makes it non-backportable If I understand correctly, so the fix would need to be coded differently to be backportable anyway | |
| 13:37:35 | sean-k-mooney | yes | |
| 13:38:57 | ganso | I need to analyze the code to see if that would be possible, but then also whether it would be acceptable as an backportable enhancement to an existing feature (pending more discussion) | |
| 13:39:00 | sean-k-mooney | ganso: so the new feature we added https://github.com/openstack/nova/commit/6bca37e904e9e56b250b123abde1901e951c8c9a | |
| 13:39:02 | ganso | I will talk to my customer first | |
| 13:39:05 | sean-k-mooney | i think would solve this usecase | |
| 13:39:20 | sean-k-mooney | and that is coded in a way that is backportable | |
| 13:39:31 | sean-k-mooney | so we coudl disucss if we wanted to backport this upstream to adress that bug | |
| 13:39:39 | sean-k-mooney | gibi: ^ | |
| 13:40:29 | ganso | sean-k-mooney: it is? but it is introducing a new config option (and it is indeed a new feature) so I assumed even if the code is backportable, it wouldn't be accepted to be | |
| 13:40:30 | sean-k-mooney | do you think https://bugs.launchpad.net/nova/+bug/1939869 is a vaild reason to backport the new migration_inboud_address config option | |
| 13:41:24 | sean-k-mooney | ganso: we normally would not backport it unless there is a large pain point that affects operators | |
| 13:42:10 | sean-k-mooney | we do sometime back port new config options the impoarnt thing with a backport of this kind is it must not change behaivor by default | |
| 13:42:57 | sean-k-mooney | which is true in this case if you want to move all traffic to the migratoin network with that feature you have to set the config option to use the dedicated networks ip/fqdn/hostname | |
| 13:43:10 | ganso | sean-k-mooney: if the default value of the new config options retains the existing behavior, that is less concerning impact for existing deployments while backporting | |
| 13:43:36 | sean-k-mooney | yep it does which is and we also did that for upgrades | |
| 13:43:50 | sean-k-mooney | we did not want to change the behavior on upgade for the same config file | |
| 13:44:33 | sean-k-mooney | elodilles: ^ i know this is borderline but i think gibis feature is a more reasoable approch then https://review.opendev.org/c/openstack/nova/+/906053 | |
| 13:45:15 | ganso | sean-k-mooney: yea if it solves the issue I am happy with backporting that instead. I need to test to be 100% sure, but it probably does as you say | |
| 13:45:52 | ganso | that other patch may even not be that much necessary anymore | |
| 13:46:09 | sean-k-mooney | ganso: i bleivle https://github.com/openstack/nova/commit/6bca37e904e9e56b250b123abde1901e951c8c9a#diff-486ddc37ce8e7c105a51566dd2cffbda034f28fae7eaad2e74abf470ab296352L4514 is the important part | |
| 13:47:27 | sean-k-mooney | i belive get_host_ip_addr is the common fucntion that is causing hte current behavior so yes https://review.opendev.org/c/openstack/nova/+/906053 shoudl not be requried anymore | |
| 13:49:04 | ganso | yep | |
| 14:55:38 | opendevreview | Rodrigo Barbieri proposed openstack/nova stable/2023.2: [libvirt]Add migration_inbound_addr https://review.opendev.org/c/openstack/nova/+/908995 | |
| 14:56:27 | ganso | sean-k-mooney: I proposed the backport just to get the discussion started ^. Waiting to hear gibi's thoughts on this | |
| 14:56:42 | sean-k-mooney | ack | |
| 15:02:19 | dougszu | Thanks for looking at https://review.opendev.org/c/openstack/nova/+/906053. It comes from a time before the other patch. I will take a look at it shortly. | |
| 15:02:50 | elodilles | sean-k-mooney: well, if it's more like a feature than a bugfix, then best to avoid the backport, though, there can be exceptions, i mean, if the stable cores agrees that it worth the risk and most probably won't cause any regression, then it can be backported. (the only thing is NOT to change the current behaviour for existing deployments, i'd say) | |
| 15:03:41 | sean-k-mooney | elodilles: ya for upgrade reasons when we added this it preserved the existing behavior | |
| 15:04:07 | sean-k-mooney | elodilles: so if we were to backport it as with master you would have to modify the config option to use it | |
| 15:04:14 | elodilles | +1 | |
| 15:04:35 | sean-k-mooney | elodilles: tl;dr we supprot movign the libvirt traffic to a dedicate network but only for live migraton | |
| 15:04:43 | sean-k-mooney | the feature enables that for cold migration | |
| 15:05:15 | sean-k-mooney | and as a sideffect also move the block copy done by nova wich is common for live migration to the new network if you opt in with the config option | |
| 15:05:45 | sean-k-mooney | not all users were aware that the live_migration_inbound_adr is just for the libvirt traffic | |
| 15:05:54 | sean-k-mooney | not also for novas data transfers | |
| 15:06:52 | sean-k-mooney | https://bugs.launchpad.net/nova/+bug/1939869 is partly a misundertaing of how thing work but i dont think its an unresobale expectation and other operator may have fallen faul of that too | |
| 15:08:34 | elodilles | i see, thanks for the details | |
| 15:19:51 | elodilles | sean-k-mooney: added my (long) comment to the backport o:) | |
| 15:21:52 | sean-k-mooney | ack we proably should chat about this either in the channel or in the team meetign to get import form stable folks in general | |
| 15:22:02 | elodilles | +1 | |
| 15:30:07 | opendevreview | Merged openstack/nova master: HyperV: Add todo to remove HyperVLiveMigrateData object https://review.opendev.org/c/openstack/nova/+/906636 | |
| 16:01:57 | dougszu | sean-k-mooney: Perhaps I missed something, but for https://bugs.launchpad.net/nova/+bug/1939869, i'm not sure how https://review.opendev.org/c/openstack/nova/+/908995 is going to 'fix' it. Since the source HV is calling the target HV by RPC, which then needs to get the IP of the source HV (not the hostname) | |
| 16:04:58 | sean-k-mooney | dougszu: it wont because it should not | |
| 16:05:20 | sean-k-mooney | howeve if you set the other config option then those calls shoudl use that for the data transfer | |
| 16:06:28 | sean-k-mooney | dougszu: defiening live_migration_inbound_addr should not cause the config driver or other data to be copied via that api | |
| 16:06:43 | sean-k-mooney | its a miss understanding fo what the config option currently is for | |
| 16:07:35 | sean-k-mooney | the new config option https://docs.openstack.org/nova/latest/configuration/config.html#libvirt.migration_inbound_addr | |
| 16:07:51 | sean-k-mooney | states """ metadata is then used by the source of the migration traffic to construct the commands used to copy data (e.g. disk image) to the destination.""" | |
| 16:08:28 | sean-k-mooney | so migration_inbound_addr should modify the adress used for the ssh/rsync | |
| 16:08:39 | dougszu | Yeah, no problem with rejecting the bug report. | |
| 16:08:50 | sean-k-mooney | but live_migration_inbound_addr should only modify the libvirt traffic | |
| 16:09:06 | dougszu | This bit, from i've seen happens the other way though: "metadata is then used by the source of the migration traffic to construct the commands used to copy data (e.g. disk image) to the destination" | |
| 16:09:17 | dougszu | I.e. the copy is initiated from the destination to the source | |
| 16:12:18 | dougszu | Eg. this runs on the dest: https://review.opendev.org/c/openstack/nova/+/906053/4/nova/virt/libvirt/driver.py#10917 | |
| 16:12:49 | dougszu | So there has to be some way of transferring the source IP/FQDN etc to the dest | |
| 16:13:07 | dougszu | Apologies if I've missed something :D | |
| 16:19:11 | ganso | dougszu: I see your point, the custom IP config set in the source would need to be propagated to the destination somehow, as it wouldn't matter that the destination has a custom migration IP too because that's not the relevant one, it is the source one | |
| 16:23:53 | dougszu | ganso: yeah, that's it. Wondering if it's worth converting https://review.opendev.org/c/openstack/nova/+/906053 to a feature request | |
| 16:25:57 | sean-k-mooney | thats not how this work | |
| 16:26:19 | sean-k-mooney | the info is passed form the dest back to the souce | |
| 16:26:35 | sean-k-mooney | as the migration is started form the soucrce | |
| 16:26:43 | sean-k-mooney | and we copy the data form the source to test | |
| 16:26:45 | sean-k-mooney | *dest | |
| 16:28:13 | ganso | sean-k-mooney: the migration of VM data is, the disk copy no. In the logs that I have I see the dest ssh'ing to the src to scp the disk | |
| 16:29:43 | dougszu | +1, I definitely see the scp initiated from the dest. Eg. for fetching config drive during live migrate. | |
| 16:30:05 | ganso | dougszu: to be honest, if the patch already merged doesn't address the issue, then it becomes even more important that the new one does, and being a non-backportable feature prevents customers from accessing the fix unless they upgrade to caracal, which for some is a long time in the future | |
| 16:30:07 | sean-k-mooney | hum really | |