| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-10-29 | |||
| 16:59:18 | mdbooth_390 | mriedem: Yeah, come on. It's so obvious! | |
| 17:04:31 | cfriesen | I updated the vTPM spec, I think it should address all the issues that were raised in the pervious version. | |
| 17:05:00 | mnaser | mriedem: not even mixed version hypervisor | |
| 17:05:12 | mnaser | i think this breaks live migrations of any vm created before pre-rocky to post-rocky | |
| 17:05:13 | mdbooth_390 | mnaser: Sorta. Mixed on the same host. | |
| 17:05:20 | mnaser | yeah i guess | |
| 17:05:29 | cfriesen | mixed version nova, not hypervisor | |
| 17:06:02 | mriedem | so live migrating a queens vm to a stein? | |
| 17:06:36 | cfriesen | I think queens to rocky, potentially. | |
| 17:07:44 | cfriesen | for vifs of type ethernet and bridge, on libvirt 3.3.0+ | |
| 17:07:59 | cfriesen | (assuming it's https://github.com/openstack/nova/commit/f02b3800051234ecc14f3117d5987b1a8ef75877 that is the culprit) | |
| 17:08:34 | openstack | Launchpad bug 1713783 in OpenStack Compute (nova) pike "duplicate for #1583504 After failed evacuation the recovered source compute tries to delete the instance" [High,Fix committed] - Assigned to Matt Riedemann (mriedem) | |
| 17:08:34 | tomtom001 | https://bugs.launchpad.net/nova/+bug/1583504 This is still an issue when: You shutoff CPU-A - then evacuate VM-B, then if it fails you decide to delete VM-B. When you start CPU-A, it cannot find VM-B and fails to start nova-compute | |
| 17:08:38 | mdbooth_390 | mriedem: You create an instance on Queens, then upgrade nova on that compute to Rocky, then try to migrate it somewhere. | |
| 17:09:05 | mdbooth_390 | mnaser: ^^^^ ? | |
| 17:13:58 | mnaser | mdbooth_390: correct | |
| 17:14:25 | mnaser | mriedem: cfriesen migrating a vm that was running before nova was upgraded to rocky would be affected | |
| 17:33:46 | mnaser | oh wait | |
| 17:33:49 | mnaser | that might not even be the commit that broke it | |
| 17:34:08 | mnaser | https://github.com/openstack/nova/blob/f02b3800051234ecc14f3117d5987b1a8ef75877/nova/virt/libvirt/driver.py#L6900-L6904 | |
| 17:34:47 | mnaser | because at the time we didnt regen network | |
| 17:36:15 | mnaser | https://github.com/openstack/nova/commit/2b52cde565d542c03f004b48ee9c1a6a25f5b7cd | |
| 17:36:18 | mnaser | this one specifically broke it | |
| 17:39:36 | openstack | Launchpad bug 1800511 in OpenStack Compute (nova) "VMs started before Rocky upgrade cannot be live migrated" [Undecided,New] | |
| 17:39:36 | mnaser | https://bugs.launchpad.net/nova/+bug/1800511 | |
| 17:39:39 | mnaser | i think this might be an easy fix actaully | |
| 17:40:17 | mnaser | looks like _update_vif_xml is only used in migrations | |
| 17:44:49 | mriedem | it is | |
| 17:44:53 | mriedem | live migrations | |
| 17:45:44 | mnaser | ok, so i will try to get a failing test case there | |
| 17:46:15 | mnaser | where no mtu => adds an mtu, seems straightforward | |
| 17:46:56 | mriedem | is the libvirt version the same on both hosts? | |
| 17:47:02 | mriedem | >=3.3? | |
| 17:47:12 | mnaser | mriedem: nope its 3.9 | |
| 17:47:25 | mriedem | which is >3.3 where mtu is supported for those vif types | |
| 17:47:55 | mnaser | yeah but the problem is if you provide an mtu the internals of qemu change and the instance becomes non-livemigrateable | |
| 17:48:16 | mnaser | and actually if you do virsh dumpxml --migratable <instance> it doesnt even include mtu | |
| 17:48:23 | mnaser | so libvirt kinda conciously doesnt put it, but we do | |
| 17:51:14 | mriedem | ok so https://github.com/openstack/nova/commit/2b52cde565d542c03f004b48ee9c1a6a25f5b7cd exposes the issue because it calls the get_config code for the vif which returns the mtu now | |
| 17:51:19 | mriedem | and we try sending that to the dest | |
| 17:51:48 | mnaser | mriedem: correct | |
| 17:51:51 | cdent | jaypipes: if you loving on the gabbi integration tests, I made this one for nova (the required tempest and gabbi-tempest changes have merged): https://review.openstack.org/#/c/613386/ | |
| 17:51:54 | mriedem | i think this would have been an issue regardless of https://github.com/openstack/nova/commit/2b52cde565d542c03f004b48ee9c1a6a25f5b7cd if the guest xml on the source had the mtru in the vif xml | |
| 17:51:59 | mriedem | *mtu | |
| 17:52:08 | mriedem | since we'd just send that over unchanged | |
| 17:52:31 | mnaser | mriedem: if mtu exists on both sides, it's fine | |
| 17:52:37 | mnaser | if mtu doesn't in one, it blows up | |
| 17:52:53 | mriedem | sure but i don't think https://github.com/openstack/nova/commit/2b52cde565d542c03f004b48ee9c1a6a25f5b7cd changes that | |
| 17:52:54 | mnaser | so if source has mtu, then dest will get it .. if source doesnt, dest won't.. and we're good | |
| 17:53:15 | mnaser | it calls self.vif_driver.get_config | |
| 17:53:18 | mnaser | which gets mtu configs | |
| 17:53:23 | mriedem | *on the source host* | |
| 17:53:31 | mriedem | we don't know what is configured for mtu on the dest | |
| 17:53:37 | mriedem | since that's not in the LiveMigrateData object | |
| 17:53:39 | mriedem | as far as i know | |
| 17:53:54 | mriedem | _update_vif_xml is called from the source | |
| 17:53:59 | mriedem | using migrate data from the dest | |
| 17:54:18 | mnaser | yes, to "modify" the xml which will be shipped to dest to start the instance to recieve data.. right? | |
| 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 | 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:44:15 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Add tests for bug #1800511 https://review.openstack.org/614004 | |