| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-08 | |||
| 15:09:36 | artom | Well now you're just picking on em :( | |
| 15:09:38 | artom | *me | |
| 15:09:42 | dansmith | but yeah, I'd like to hear from lyarwood I guess | |
| 15:09:52 | mriedem | dansmith: it was my change :) | |
| 15:09:55 | mriedem | lyarwood backported it | |
| 15:10:24 | mriedem | and https://review.openstack.org/#/c/179228/ was your change :) | |
| 15:10:27 | mriedem | it's great | |
| 15:10:33 | artom | In a meeting now, I think I'll have to play around with it afterwards | |
| 15:10:37 | artom | Until now I was relying on logs | |
| 15:11:02 | artom | Yo-yo patches... | |
| 15:11:05 | stephenfin | oh, that's interesting | |
| 15:15:37 | dansmith | oh | |
| 15:15:40 | mriedem | artom: dansmith: ok so i see the race in https://review.openstack.org/#/c/595069/ | |
| 15:15:43 | mriedem | the dest triggers the event, | |
| 15:15:46 | mriedem | the source is waiting for it, | |
| 15:15:53 | mriedem | but it might come before the source is registered for the callback | |
| 15:16:13 | dansmith | so wait, we've gone back and forth twice now? | |
| 15:16:16 | mriedem | dest unplugs vifs when it calls driver.destroy | |
| 15:16:17 | mriedem | yes | |
| 15:16:40 | mriedem | when the original change was made by dansmith we didn't have the code in the API which routes events to both the source and host if the instance had a migration context | |
| 15:16:49 | mriedem | so when i made my change, the source will get the event routed to it, | |
| 15:17:00 | mriedem | but, we might not be registered for the callback on the source by the time the event arrives | |
| 15:17:39 | mriedem | as you can see from my comments in https://review.openstack.org/#/c/179228/ it's all very confusing | |
| 15:18:09 | mriedem | the sequence of events is kind of tribal knowledge with only 1.5 people in the tribe nowadays | |
| 15:19:45 | mriedem | so we should probably (1) revert my change and (2) add a comment in the libvirt driver finish_revert_migration code about why we don't wait for the event (like there is a comment in finish_migration) | |
| 15:20:06 | mriedem | because the event does come to the source, but we are racing to catch it | |
| 15:21:16 | mriedem | actually...finish_revert_migration does plug vifs, | |
| 15:21:22 | mriedem | so why wouldn't we get an event for that on the source? | |
| 15:21:43 | mriedem | dest unplugs vifs on revert_resize because of driver.destroy, | |
| 15:21:58 | mriedem | source plugs vifs because of finish_revert_migration which re-spawns the guest | |
| 15:22:21 | mriedem | artom: are you seeing this downstream with OVS or linuxbridge? | |
| 15:22:57 | artom | mriedem, ovs | |
| 15:23:13 | artom | mriedem, we do get the event on the source, just before we wait for it | |
| 15:23:30 | artom | Because as soon as Neutron gets the API call, it's able to wire them and send us the events | |
| 15:23:49 | mriedem | the network-vif-plugged event? | |
| 15:23:52 | artom | Yeah | |
| 15:24:20 | mriedem | ok that's from something else then | |
| 15:24:39 | mriedem | because when the source calls plug_vifs during finish_revert_migration, it's doing it within the context to wait for the event | |
| 15:25:21 | artom | mriedem, do you have access to slaweq's notes on our downstream BZ? | |
| 15:25:47 | artom | mriedem, https://bugzilla.redhat.com/show_bug.cgi?id=1678681#c8 | |
| 15:25:48 | openstack | bugzilla.redhat.com bug 1678681 in openstack-nova "REVERT_RESIZE stuck for 300s: "VirtualInterfaceCreateException: Virtual Interface creation failed" [Medium,On_dev] - Assigned to alifshit | |
| 15:26:35 | mriedem | artom: then i'm pretty sure it's what we've talked about before, finish_revert_resize on the source host calls migrate_instance_finish which updates the port binding to point at the source host, which triggers an event | |
| 15:26:40 | mriedem | *before* the driver registers the callback | |
| 15:27:28 | artom | mriedem, right, but there's another thing at play here - Neutron will only wire the ports after we plug them | |
| 15:27:33 | artom | ... I think | |
| 15:28:35 | mriedem | yes i can read his comments and i think they align with what i just said, | |
| 15:28:56 | mriedem | "Nova then asks neutron to bind port on compute-0 again, at it happens:" - that's the migrate_instance_finish call on the source finish_revert_resize to update the port binding | |
| 15:29:15 | mriedem | "And after that, L2 agent on compute-0 wire port again:" - i assume that's the plug_vifs call from the driver's finish_revert_migration on the source host | |
| 15:30:35 | mriedem | artom: how recreateable is this? it would be a lot more clear if we just had some logging on the source that said, "updating port bindings to point at the source host" and then "finish revert migration in the driver which will plug vifs" | |
| 15:30:53 | mriedem | because if we saw the event come between those 2 messages we know it's the port binding change that is triggering the event before we're ready to wait for it | |
| 15:30:54 | artom | mriedem, like, 70% in our CI? | |
| 15:31:06 | mriedem | are you able to test with patches from upstream? | |
| 15:31:21 | artom | I'd need to check with the CI guys, but I could provide them test builds, yeah | |
| 15:31:55 | kashyap | aspiers: You about...? | |
| 15:32:43 | kashyap | aspiers: When you are -- at the risk of adding more work ... wonder if we should simply split out the addition of getDomainCapablities() method into its own patch. | |
| 15:33:06 | mriedem | artom: ew do you have to patch an rpm or something? | |
| 15:33:13 | kashyap | It is just my OCD of "one logical change per-patch thing". And it allows quicker merge, too. As it'll be easier on the reviewers eyes | |
| 15:33:14 | artom | mriedem, scratch build | |
| 15:33:35 | artom | Backport a patch, build RPMs with that | |
| 15:34:04 | mriedem | artom: actually you should be able to determine this with existing logs | |
| 15:34:11 | kashyap | (A "scratch build" is something that is short-lived, and will be "scratched" from the build system) | |
| 15:34:54 | artom | (Backport a patch, build a SRPM without pushing anything, scratch build, I should have said) | |
| 15:35:04 | mriedem | artom: you should see this on the source https://github.com/openstack/nova/blob/4f9bc724010f0c935bf83a6d19bdd805e86b7086/nova/network/neutronv2/api.py#L3355 | |
| 15:35:23 | mriedem | with binding:host_id changing to point at the source host | |
| 15:35:43 | mriedem | and then you should see this from the driver https://github.com/openstack/nova/blob/4f9bc724010f0c935bf83a6d19bdd805e86b7086/nova/virt/libvirt/driver.py#L8965 | |
| 15:35:47 | artom | mriedem, ack, looking | |
| 15:36:43 | mriedem | granted there is some other stuff that happens in the driver after that before unplug_vifs happens, so it's a tight window | |
| 15:42:25 | aspiers | kashyap: yeah could do, I kind of like there being an incentive to get SEV stuff merged though ;-) | |
| 15:42:54 | aspiers | when do we fork for stein? | |
| 15:43:27 | kashyap | aspiers: Yeah, I hear you. But as we both know ... it can be used for multiple features :-) | |
| 15:43:57 | kashyap | (I won't insist on it, though. But if you appetite...) | |
| 15:43:59 | aspiers | we already hit feature freeze, right? | |
| 15:44:03 | aspiers | https://wiki.openstack.org/wiki/Nova/Stein_Release_Schedule | |
| 15:44:04 | kashyap | aspiers: Yes, yesterday | |
| 15:44:10 | kashyap | mriedem: ^ Right? | |
| 15:45:04 | mriedem | yes | |
| 15:45:13 | mriedem | please don't be approving anything that's not already approved | |
| 15:45:14 | kashyap | aspiers: Also think of it this way: splitting it out allows it to be merged while SEV bits get reviewed :-) | |
| 15:45:39 | aspiers | I guess | |
| 15:45:40 | kashyap | mriedem: This (AMD SEV work) was already approved for Stein | |
| 15:45:55 | aspiers | I think mriedem means W+1 | |
| 15:45:58 | kashyap | aspiers: But sorry to be "that guy"; you're allowed to hate me for 5 minutes. | |
| 15:46:04 | aspiers | haha | |
| 15:46:21 | sean-k-mooney | aspiers: we will fork stien at RC1 so master is feature frozen for the next 2 ish weeks until that is done i think | |
| 15:46:42 | aspiers | sean-k-mooney: OK thanks | |
| 15:47:04 | kashyap | aspiers: This work will spill over into "Train", yes? | |
| 15:47:10 | sean-k-mooney | rc1 will be around march 21st | |
| 15:47:26 | aspiers | kashyap: https://review.openstack.org/#/c/641994/ | |
| 15:47:43 | aspiers | Oh, you already saw that :) | |
| 15:47:46 | aspiers | I forgot | |
| 15:47:53 | kashyap | Yeah, no worries. | |
| 15:48:33 | aspiers | Anyway, ultimately I need to do whatever you guys think is best :) | |
| 15:48:39 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Documentation for bandwidth support https://review.openstack.org/642064 | |
| 15:48:42 | aspiers | If splitting it out helps then let's do that | |
| 15:49:14 | kashyap | aspiers: Yeah, it also will be less "daunting" to those who don't normally dwell on this area of code | |
| 15:49:28 | aspiers | Sure | |
| 15:49:29 | mriedem | what are you talking about splitting out? | |
| 15:49:45 | aspiers | the call to getDomainCapabilities | |
| 15:50:13 | kashyap | mriedem: Yeah, the adding of getDomainCapablities() in libvirt/host.py | |
| 15:50:34 | aspiers | kashyap: I was also thinking about splitting this code move out https://review.openstack.org/#/c/633855/11/nova/virt/libvirt/utils.py | |