| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-10-29 | |||
| 17:54:29 | mriedem | yes | |
| 17:54:44 | mriedem | but if the xml on the source already had mtu in it, and the dest doesn't support that, then kaboom anyway right? | |
| 17:54:52 | cfriesen | mriedem: I think the issue is that the instance originally didn't have mtu in it | |
| 17:54:59 | cfriesen | then we upgraded nova, and now it would have mtu | |
| 17:55:10 | mnaser | the host_mtu was added really long time ago in qemu | |
| 17:55:20 | mnaser | so its not likely that qemu will crash for lack of that feature | |
| 17:55:38 | mnaser | but if the destination doesnt get a host_mtu.. it just .. doesnt set one and moves on | |
| 17:55:53 | cfriesen | nova on the dest doesn't need to have the mtu, but the xml that we pass the live migration needs to match the xml that was used to start the running instance. | |
| 17:56:10 | mnaser | cfriesen: doing a much better job explaining this than i am anyways :p | |
| 17:58:53 | mriedem | "but the xml that we pass the live migration needs to match the xml that was used to start the running instance." | |
| 17:58:57 | mriedem | ok i guess i didn't know that was a thing | |
| 18:00:44 | mriedem | i'm not sure why it cares about the xml from when the guest was last started if we're sending over the xml that we want | |
| 18:01:07 | mriedem | same is true for things like block devices | |
| 18:02:07 | mriedem | so the proposed fix is going to be checking within _update_vif_xml if the source guest xml had mtu set before we call get_config to get the new dest xml vif config? | |
| 18:02:21 | mriedem | and if not, omit it from the dest xml? | |
| 18:02:33 | mnaser | mriedem: i think that is where im going | |
| 18:02:47 | mnaser | i think i have a failing test too | |
| 18:04:18 | mnaser | another reason it was missed was because the tests were testing bridge => vhostuser | |
| 18:04:23 | mnaser | vhostuser doesnt set mtus | |
| 18:04:29 | mnaser | so the tests didnt show mtu appearing | |
| 18:07:39 | mriedem | i'm pretty sure the live migration tests for _update_vif_xml are just using fake xml aren't they? | |
| 18:08:31 | mriedem | well maybe not, but i'm not sure they would have cared much about the delta given part of the reason for that change was live migrating between vif types which is going to yield different xml | |
| 18:11:54 | mnaser | mriedem: kinda, also only bridge and ethernet net_type support mtu | |
| 18:12:12 | mnaser | and the tests was testing bridge=>vhostuser which meant mtu was never added anyways | |
| 18:19:09 | openstackgerrit | Jack Ding proposed openstack/nova-specs master: High Precision Event Timer (HPET) on x86 guests https://review.openstack.org/607989 | |
| 18:21:29 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Add tests for bug #1800511 https://review.openstack.org/614004 | |
| 18:21:30 | openstack | bug 1800511 in OpenStack Compute (nova) rocky "VMs started before Rocky upgrade cannot be live migrated" [High,Triaged] https://launchpad.net/bugs/1800511 | |
| 18:21:42 | mnaser | mriedem: cfriesen mdbooth_390 ^ round 1.. that's a failing test that's commented out, i'll work on the actual fix now | |
| 18:29:30 | mnaser | mriedem: sorry to bother ya, do you think _update_vif_xml() should make sure "mtu" is there (if it was before) and removed if it wasn't there before.. or should i pass an extra arg to get_vif_config() inside _update_vif_xml() like migration=True and handle it in the vif layer? | |
| 18:30:46 | mriedem | i think it would probably be simpler to isolate it within _update_vif_xml rather than plumb down through to the vif get_config stuff | |
| 18:31:01 | mriedem | i just wonder how whack-a-mole this is going to be going forward with any changes to that os-vif code | |
| 18:31:10 | mriedem | or if this mtu thing is an outlier | |
| 18:32:40 | mnaser | mriedem: it looks like the mtu is an outlier because $kvm things .. but i dunno | |
| 18:34:33 | mriedem | yeah i was just wondering based on that bz | |
| 18:34:35 | mriedem | which i skimmed | |
| 18:34:40 | mriedem | sounds like libvirt had to revert that | |
| 18:34:45 | mriedem | s/that/something/ | |
| 18:34:51 | mnaser | yep, they did back in 3.3 | |
| 18:34:56 | mnaser | so we really just repeated their bug a layer above | |
| 18:35:03 | mriedem | sweet | |
| 18:44:15 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Add tests for bug #1800511 https://review.openstack.org/614004 | |
| 18:44:15 | openstack | bug 1800511 in OpenStack Compute (nova) rocky "VMs started before Rocky upgrade cannot be live migrated" [High,Triaged] https://launchpad.net/bugs/1800511 | |
| 18:47:57 | cfriesen | mriedem: missed your comment earlier. the XML that we send as part of the migration is used to start up qemu on the dest, this creates data structures in memory, and those data structures need to match the data structures for the guest on the source node. | |
| 18:48:51 | mriedem | cfriesen: sure but this isn't the only thing we change on the dest xml when we send it over | |
| 18:49:01 | mriedem | which is why i'm hoping the mtu thing is an outlier | |
| 18:50:23 | cfriesen | yeah, unless it changes the in-memory layout we don't really care. | |
| 18:51:13 | cfriesen | so we preserve things like device types, virtual machine type, CPU/RAM topology, etc. | |
| 18:51:31 | cfriesen | I wonder if the MTU size affects the layout of the NIC ring buffers | |
| 18:52:26 | mnaser | i mean i guess this explains why/what | |
| 18:52:28 | mnaser | https://bugzilla.redhat.com/show_bug.cgi?id=1449346 | |
| 18:52:28 | openstack | bugzilla.redhat.com bug 1449346 in libvirt "Addition of host_mtu=XXXX changes PCI ioport size & addresses for virtio-net device" [Unspecified,Closed: errata] - Assigned to laine | |
| 18:52:32 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Avoid setting MTU if none was configured https://review.openstack.org/614008 | |
| 18:52:44 | mnaser | anyhow | |
| 18:53:00 | cfriesen | ah, that would do it | |
| 18:53:13 | mnaser | thats a fix with the test in the patch above it, i'd be more than happy to iterate if anyone has comments | |
| 18:53:28 | mnaser | considering this has put a big upgrade project to a halt :< | |
| 18:57:26 | mriedem | i'm on a call for the next hour or so but will try to take a look | |
| 18:58:13 | mnaser | cool, thank you, please highlight me if anything so i can iterate | |
| 19:08:50 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Cross-cell resize https://review.openstack.org/603930 | |
| 19:08:50 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add Migration.cross_cell_move and get_by_uuid https://review.openstack.org/614012 | |
| 19:10:21 | cfriesen | mnaser: got a possible problem scenario for you. boot instance on upgraded nova, live-migrate to non-upgraded nova (this will preserve the MTU), then live-migrate back. I suspect the running guest will have MTU, but the generated XML won't. | |
| 19:11:11 | mnaser | cfriesen: isn’t that covered by the _with_mtu test case? | |
| 19:11:28 | mnaser | Where we have a source mtu and it is maintained for the live migration | |
| 19:13:11 | mnaser | It only omits it if the domain XML doesn’t have it, and if the domain XML doesn’t have it then the host_mtu is not set and we’re okay | |
| 19:13:45 | cfriesen | no, in the scenario I just described the running guest would have the MTU, but it's on an older nova that doesn't even try to generate the mtu in the xml | |
| 19:14:05 | cfriesen | unless we actually dump the xml from the running guest, in which case it might be okay | |
| 19:14:25 | cfriesen | can't remember the exact details right now | |
| 19:15:56 | mnaser | cfriesen: I’m assuming in all of this that the XML is being dumped from the running guest | |
| 19:16:22 | mnaser | Let me check | |
| 19:17:06 | cfriesen | yeah, it does. get_xml_desc() in guest.py | |
| 19:19:29 | mnaser | Yup. And it specifies dump_migratable=True | |
| 19:19:55 | mnaser | Which I assume means libvirt will be taking care of not making us do bad things | |
| 19:32:20 | mriedem | mnaser: comments inline | |
| 19:32:21 | mriedem | mostly just nits | |
| 19:36:22 | mriedem | cfriesen: you're talking about this right? https://github.com/openstack/nova/blob/a0eacbf7fff60282007ddca705ef7331e8a4a6f8/nova/virt/libvirt/migration.py#L83 | |
| 19:36:41 | mriedem | yeah that's what i was referring to earlier - before _update_vif_xml we'd just always send the source vif xml unchanged | |
| 19:37:25 | cfriesen | mriedem: yes | |
| 19:44:35 | mnaser | mriedem: cfriesen thanks for the reviews both, i tried to address but in case it wasn't clear in terms of comments, please let me know | |
| 19:44:44 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Add tests for bug #1800511 https://review.openstack.org/614004 | |
| 19:44:44 | openstack | bug 1800511 in OpenStack Compute (nova) "VMs with vif_type bridge/tap started before Rocky upgrade cannot be live migrated" [High,In progress] https://launchpad.net/bugs/1800511 - Assigned to Mohammed Naser (mnaser) | |
| 19:44:44 | openstackgerrit | Mohammed Naser proposed openstack/nova master: libvirt: Avoid setting MTU during live migration if unset https://review.openstack.org/614008 | |
| 19:50:50 | cfriesen | mnaser: much better, one little comment | |
| 19:52:07 | mnaser | cfriesen: i guess i wanted to make sure that a normal reboot wouldn't do the trick (because the qemu-kvm process doesnt get restarted) | |
| 19:52:13 | mnaser | but i guess the follow up comment clears it up | |
| 19:52:47 | openstackgerrit | Mohammed Naser proposed openstack/nova master: libvirt: Avoid setting MTU during live migration if unset https://review.openstack.org/614008 | |
| 19:52:50 | mnaser | cfriesen: voila | |
| 19:52:51 | cfriesen | mnaser: a "nova reboot" without the "hard" option will reset things | |
| 19:52:59 | mnaser | really | |
| 19:53:04 | mnaser | i thought i tried to do an acpi reboot | |
| 19:54:20 | cfriesen | we try an ACPI shutdown | |
| 19:54:31 | cfriesen | then if that doesn't work we do a hard shutdown | |
| 19:54:39 | mnaser | aaaah | |
| 19:54:40 | mnaser | gotcha | |
| 19:54:44 | mnaser | so the reboot always restarts the process | |
| 19:54:54 | mnaser | cool, TIL! | |
| 19:56:11 | mriedem | pep8 is going to fail on https://review.openstack.org/#/c/614004/ | |
| 19:56:57 | mnaser | ah shit | |
| 19:57:42 | mnaser | ill run that locally | |
| 19:58:03 | mriedem | +2 on the top one | |
| 20:21:12 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Add tests for bug #1800511 https://review.openstack.org/614004 | |