| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-12-19 | |||
| 14:59:45 | mriedem | https://github.com/openstack/nova/blob/master/nova/objects/pci_device.py#L244 | |
| 14:59:56 | sean-k-mooney | so this is suspicious https://github.com/openstack/nova/blob/master/nova/pci/manager.py#L166-L181 | |
| 15:01:08 | mriedem | idk, seems like a very easy solution to avoid screwing up the db state would just be raise an exception here https://github.com/openstack/nova/blob/master/nova/objects/pci_device.py#L244 if self.instance_uuid is not None | |
| 15:01:17 | mriedem | i.e. you can't remove/delete an allocated pci device | |
| 15:02:31 | sean-k-mooney | yes but it looks like https://github.com/openstack/nova/blob/master/nova/pci/manager.py#L166-L181 was maybe working around that | |
| 15:04:02 | mriedem | that was added 6 years ago https://github.com/openstack/nova/commit/4855239497050c9ee03fed627c5f41d6b59eddc6 | |
| 15:04:05 | mriedem | and clearly it sucks | |
| 15:04:26 | gibi | mriedem: FYI my wall of text as a review guide for the bandwidth series http://lists.openstack.org/pipermail/openstack-discuss/2018-December/001129.html | |
| 15:04:37 | sean-k-mooney | yes im reading the original commit curently | |
| 15:05:35 | mriedem | gibi: ack thanks | |
| 15:06:56 | sean-k-mooney | mriedem: if we simply dont do existed.status = 'removed' with "continue" in the excpet clause that may be enough. | |
| 15:07:24 | sean-k-mooney | mriedem: i lean more and more to its working how its was intended but what was intended is dumb and we shoudl fix it | |
| 15:07:45 | mriedem | mgariepy: did you see this warning in the nova-compute logs? https://github.com/openstack/nova/blob/master/nova/pci/manager.py#L169 | |
| 15:08:41 | mriedem | sean-k-mooney: yeah i don't understand that logic at all | |
| 15:08:55 | mriedem | i guess it's saying the hypervisor is no longer reporting that device so we need to forcefully remove it/ | |
| 15:08:56 | mriedem | ? | |
| 15:09:26 | sean-k-mooney | mriedem: the orginal code comment was lost at some point from the set_hvdevs fucntion | |
| 15:09:30 | sean-k-mooney | "Devices should not be hot-plugged when assigned to a guest, | |
| 15:09:32 | sean-k-mooney | but possibly the hypervisor has no such guarantee. The best | |
| 15:09:34 | sean-k-mooney | we can do is to give a warning if a device is changed | |
| 15:09:36 | sean-k-mooney | or removed while assigned. | |
| 15:09:38 | sean-k-mooney | " | |
| 15:09:44 | mriedem | oh but the new_devs are filtered through the whitelist | |
| 15:09:46 | mriedem | in device_assignable | |
| 15:09:53 | mriedem | https://github.com/openstack/nova/blob/master/nova/pci/manager.py#L120 | |
| 15:10:08 | mriedem | so it's not that the hypervisor is no longer reporting the devices, it's that the whitelist changed | |
| 15:10:14 | mriedem | which is exactly the bug | |
| 15:10:15 | sean-k-mooney | yes | |
| 15:10:59 | sean-k-mooney | so if we change the filtering to allow allocated deivce in addtion to the whitelist it will be fine | |
| 15:11:49 | sean-k-mooney | when the device is deallocated form the guest it will be removed from the aviable device on the next periodic sync | |
| 15:11:59 | sean-k-mooney | as it si nolonger in the whitelist or allocated | |
| 15:12:53 | stephenfin | mriedem: Not to distract you now, but did you make a mistake on https://github.com/openstack/nova/commit/cdf8ba5acb ? You've said it fixes https://bugs.launchpad.net/nova/+bug/1784579 but that bug is for live migration, not compute service restart which is what your commit addresses | |
| 15:12:53 | openstack | Launchpad bug 1784579 in OpenStack Compute (nova) queens "unable to live migrate instance after update to queens" [Medium,Confirmed] | |
| 15:13:19 | stephenfin | mriedem: I ask because I found a similar bug which does deal with the compute service restart https://bugs.launchpad.net/nova/+bug/1738373 | |
| 15:13:19 | openstack | Launchpad bug 1738373 in OpenStack Compute (nova) "nova-compute cannot restart if _init_host failed" [Undecided,In progress] - Assigned to Xiao Gong (gongxiao) | |
| 15:13:24 | mgariepy | mriedem, https://paste.ubuntu.com/p/GVJQqMSTrM/ | |
| 15:13:37 | mgariepy | yes i did saw the warning. | |
| 15:14:49 | mriedem | mgariepy: yup and that matches the instance uuid in https://paste.ubuntu.com/p/Pn76QVmwqr/ | |
| 15:14:53 | mriedem | for the deleted pci device | |
| 15:15:31 | sean-k-mooney | mriedem: mgariepy ill write this up in the bug and i might go fix it. that said im going on vaction today so i might not get to it untill january | |
| 15:15:46 | sean-k-mooney | if other want to work on it in the interim feel free. | |
| 15:16:46 | mriedem | sean-k-mooney: i just did https://bugs.launchpad.net/nova/+bug/1633120 | |
| 15:16:46 | openstack | Launchpad bug 1633120 in OpenStack Compute (nova) "Nova scheduler tries to assign an already-in-use SRIOV QAT VF to a new instance" [Undecided,Confirmed] | |
| 15:17:29 | sean-k-mooney | mriedem: ok in that case ill close https://bugs.launchpad.net/nova/+bug/1809040 as a duplicate of https://bugs.launchpad.net/nova/+bug/1633120 | |
| 15:17:29 | openstack | Launchpad bug 1633120 in OpenStack Compute (nova) "duplicate for #1809040 Nova scheduler tries to assign an already-in-use SRIOV QAT VF to a new instance" [Undecided,Confirmed] | |
| 15:17:57 | mriedem | sean-k-mooney: already did | |
| 15:18:07 | mriedem | stephenfin: yes bug 1784579 is about os-vif port binding failed errors right? | |
| 15:18:08 | openstack | bug 1784579 in OpenStack Compute (nova) queens "unable to live migrate instance after update to queens" [Medium,Confirmed] https://launchpad.net/bugs/1784579 | |
| 15:18:20 | jaypipes | mriedem: apologies. internet down for last 15 minutes in Sarasota... last thing I got from you was "is that from the db or the config" | |
| 15:18:29 | sean-k-mooney | mriedem: in that case ill pay sean-k-mooney 1 million dollars :) | |
| 15:19:00 | mriedem | jaypipes: oh just this terrible code https://github.com/openstack/nova/blob/master/nova/pci/manager.py#L177 | |
| 15:19:14 | stephenfin | mriedem: Yup, but it's to do with live migration and the fix is only for the service startup code path | |
| 15:19:18 | mriedem | sean-k-mooney: if you want, throw up a patch that changes ^ to a continue, fix whatever test hits that and i'll +2 | |
| 15:19:40 | stephenfin | At least, assuming I'm reading it right. I'll do some digging but just wanted to sanity check it before I dived down the rabbit hole :) | |
| 15:19:56 | mriedem | stephenfin: the live migratoin fails because of the port binding failures | |
| 15:20:06 | jaypipes | mriedem: I think you meant this code: https://github.com/openstack/nova/blob/a1dba961f0018a4995d208a290f4a859ce295840/nova/pci/manager.py#L1-L357 | |
| 15:20:22 | mriedem | i see what you did there | |
| 15:20:28 | jaypipes | u like that? | |
| 15:21:03 | sean-k-mooney | jaypipes: your not wronge | |
| 15:21:15 | mriedem | stephenfin: comment | |
| 15:21:16 | mriedem | 2 | |
| 15:21:17 | mriedem | "To summarize, it looks like the pre_live_migration method on the destination host fails to plug vifs and you end up with the "binding_failed" error, which is raised and makes the source live_migration method fail as expected. The failure is on the dest host. As a result, the info cache is updated with "binding_failed" which causes the source compute restart to fail here:" | |
| 15:21:23 | jaypipes | sean-k-mooney: but you are. | |
| 15:21:30 | jaypipes | wronge that is. | |
| 15:21:36 | jaypipes | sean-k-mooney: :P | |
| 15:22:13 | sean-k-mooney | jaypipes: it is currently more functional then cyborge's pci passthough support at least ... | |
| 15:22:19 | mriedem | stephenfin: so no i didn't fix the original reason for the port binding failure in pre_live_migration, because that could have been for any number of reasons (neutron agent was down on the dest host?) | |
| 15:22:38 | mriedem | i fixed a symptom of that failure, which was nova-compute failed to restart after that failure | |
| 15:22:54 | mriedem | as the commit message says, "Admittedly this isn't the smartest thing and doesn't attempt | |
| 15:22:54 | mriedem | to recover / fix the instance networking info" | |
| 15:22:59 | stephenfin | mriedem: I'm missing something. Why make changes to 'ComputeManager.init_host' (via '_init_instance') in that commit? The exception was being seen in the live migration flow | |
| 15:23:01 | stephenfin | ahhhhh | |
| 15:23:21 | mriedem | 1. live migratoin fails, port binding failed - that gets saved in the info cache | |
| 15:23:32 | mriedem | 2. restart source compute - that blows up because it wasn't handling binding_failed vif types in the os-vif conversion code | |
| 15:23:39 | mriedem | i handle #2 | |
| 15:23:46 | mriedem | #1 is sort of out of my control | |
| 15:23:51 | stephenfin | Your fix would inadvertently resolve https://bugs.launchpad.net/nova/+bug/1738373 so | |
| 15:23:51 | openstack | Launchpad bug 1738373 in OpenStack Compute (nova) "nova-compute cannot restart if _init_host failed" [Undecided,In progress] - Assigned to Xiao Gong (gongxiao) | |
| 15:24:12 | mriedem | i mean, we probably shouldn't be saving off busted port binding information when pre_live_migration fails, | |
| 15:24:27 | mriedem | since that overwrites the previously good port binding information from the source host | |
| 15:25:03 | mriedem | i would have to dig into where we save off the bad port binding information | |
| 15:25:08 | stephenfin | Yup, there's a related fix (also for live migration) that you worked on which looks more involved https://bugs.launchpad.net/nova/+bug/1783917 | |
| 15:25:08 | openstack | Launchpad bug 1783917 in OpenStack Compute (nova) "live migration fails with NovaException: Unsupported VIF type unbound convert '_nova_to_osvif_vif_unbound'" [High,Fix released] - Assigned to Matt Riedemann (mriedem) | |
| 15:25:18 | stephenfin | Wait, wrong link? | |
| 15:26:02 | mriedem | ^ was a regression in rocky | |
| 15:26:46 | mriedem | so i suppose my fix should have been related to bug 1784579 | |
| 15:26:46 | openstack | bug 1784579 in OpenStack Compute (nova) queens "unable to live migrate instance after update to queens" [Medium,Confirmed] https://launchpad.net/bugs/1784579 | |
| 15:26:49 | mriedem | not closes it | |
| 15:27:27 | stephenfin | Probably, yeah | |
| 15:27:32 | stephenfin | OK, there's just a lot of overlap on these and I'm just trying to unravel it. We also have https://bugzilla.redhat.com/show_bug.cgi?id=1578028 (which I'm filing upstream now) which seems related too | |
| 15:27:32 | openstack | bugzilla.redhat.com bug 1578028 in openstack-nova "ovaException: Unsupported VIF type unbound convert '_nova_to_osvif_vif_unbound'" [Urgent,Assigned] - Assigned to nova-maint | |
| 15:28:09 | stephenfin | That os_vif conversion code could probably do with some tweaks. I'll see what I can do | |
| 15:29:20 | mriedem | looks like i need to backport https://review.openstack.org/#/c/595317/ as well - i had marked the bug for queens but forgot to keep going i guess | |
| 15:29:43 | sean-k-mooney | stephenfin mriedem fixed that in rocky rc phase | |
| 15:30:30 | mriedem | stephenfin: sorting out where we save off bogus port binding failed information during pre_live_migration is probably worthwhile | |
| 15:30:40 | mriedem | i'm sure there is probably some update db decorator involved that automatically does it | |
| 15:31:12 | sean-k-mooney | mriedem: i thik its related to the network opdate events we get form neutron | |
| 15:31:22 | mriedem | oh yeah that might have been it | |
| 15:31:27 | stephenfin | mriedem: Ack. I just need to get a reproducer. Easier said than done | |
| 15:32:12 | mriedem | sean-k-mooney: b/c we'll get a network-changed event for that i believe | |