| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-19 | |||
| 19:10:51 | artom | cfriesen, there was https://review.openstack.org/#/c/246910/14 that wanted to send events to both source and dest during live-migration | |
| 19:11:05 | artom | That ended up being re-worked when we merged the other patch | |
| 19:11:12 | artom | But it shows the need was apparently there | |
| 19:11:31 | artom | I suspect a lot of deployments ran with vif_plugging_fatal = false, which masks the whole thing | |
| 19:16:57 | mriedem | replied in the ML | |
| 19:17:08 | mriedem | i'm not sure how sending events to the source compute during an evacuation makes any sense, | |
| 19:17:11 | mriedem | isn't the source compute down? | |
| 19:18:44 | dansmith | it might be, but the point is, any move/migrate should be sending them to both sides | |
| 19:19:43 | mriedem | looking at what artom linked it looked like what was really needed there was the vif plugged/unplugged events, which makes sense | |
| 19:20:01 | mriedem | network-changed is triggered anytime a port is updated - which for us is when it's bound/unbound to a host | |
| 19:20:20 | mriedem | which makes sense, but as noted we shotgun our way to refresh the cache b/c we don't know which specific port was updated in that case | |
| 19:20:24 | mriedem | which is my series of changes | |
| 19:20:45 | mriedem | also, | |
| 19:21:04 | mriedem | i'm loathe to decide in the API based on migration type and event type which we should send and which we shouldn't based on how we expect the computes to behave | |
| 19:21:51 | artom | Also, I thought events the compute didn't specifically set up to listen for got ignored? | |
| 19:22:04 | mriedem | no | |
| 19:22:07 | artom | Is network-changes always processed? | |
| 19:22:10 | cfriesen | sure...but can we in nova selectively ignore the events if we know we don't need to care about them? | |
| 19:22:16 | mriedem | artom: yes | |
| 19:22:40 | mriedem | https://review.openstack.org/#/c/465792/ | |
| 19:22:43 | mriedem | oops | |
| 19:22:46 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6854 | |
| 19:22:54 | mriedem | is a flat out, rebuild the entire nw info cache | |
| 19:23:20 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6862 for network-vif-deleted is targeted to at least just deallocate the port and remove it from the nw info cache w/o a full rebuild | |
| 19:23:25 | prometheanfire | looks like the oslo-cache test may be overly specific http://logs.openstack.org/87/465887/4/check/gate-cross-nova-python27-ubuntu-xenial/19a9161/testr_results.html.gz | |
| 19:23:35 | mriedem | artom: else we fall into https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6800 | |
| 19:23:47 | mriedem | _event = self.instance_events.pop_instance_event(instance, event) are the things the specific compute is listening for, | |
| 19:23:48 | mriedem | like vif plugged | |
| 19:23:49 | artom | mriedem, OK, I was thinking of https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L302 | |
| 19:23:50 | dansmith | mriedem: yes I think the API making that call is bad news.. it's bad news for even compute manager to do it because the virt driver is running the migration | |
| 19:24:21 | mriedem | ^ triggers the callback into the virt driver to finish spawn() | |
| 19:24:33 | artom | mriedem, oh ok, yeah, your first link shows that network-changed is always processed | |
| 19:24:41 | mriedem | but that's the network-vif-plugged and network-vif-unplugged events | |
| 19:24:47 | artom | Regardless of what the compute prepared for | |
| 19:24:55 | mriedem | right | |
| 19:24:55 | dansmith | also, when we support cross-cell migrations we'll want network-changed processed on both sides since they're separate databases | |
| 19:25:22 | dansmith | if we don't we'll end up with stupid bugs like info cache is as provided by the source compute until the next periodic refresh | |
| 19:25:31 | mriedem | dansmith: during a cross cell migration, wouldn't we get an InstanceNotFound on the source cell when trying to update it's nw info cache? | |
| 19:25:32 | cfriesen | okay, so can we add more info from neutron to reduce the number of queries nova has to do? | |
| 19:25:59 | dansmith | mriedem: no, if we do cross-cell migrations we'll have to create the instance in the target database before any of the migration stuff starts | |
| 19:26:11 | mriedem | ew yeah | |
| 19:26:12 | artom | Also, am I the only one who finds it completely mental that https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6856 is on the *compute*? | |
| 19:26:22 | artom | I'd put that somewhere more central, conductor I guess | |
| 19:26:34 | dansmith | artom: why? | |
| 19:26:35 | prometheanfire | mriedem: need a bug for http://logs.openstack.org/87/465887/4/check/gate-cross-nova-python27-ubuntu-xenial/19a9161/testr_results.html.gz ? | |
| 19:26:59 | dansmith | artom: you have more computes than conductors, and instance events don't come to conductor, they come to the compute(s) that are associated with the instances | |
| 19:27:05 | cfriesen | dansmith: it's a bit silly that both computes are simultaneously querying the same data from neutron | |
| 19:27:08 | dansmith | anything we can obviously do at the edges (computes) is better | |
| 19:27:24 | mriedem | prometheanfire: probably - without me looking into the actual tests | |
| 19:27:25 | dansmith | cfriesen: not in a cross-cell migration | |
| 19:27:30 | artom | dansmith, I guess I was thinking in the context of this overloading Neutron discussion | |
| 19:27:42 | cfriesen | dansmith: wouldn't you have separate conductors there too? | |
| 19:27:50 | artom | dansmith, but yeah, I'll shut up now ;) | |
| 19:27:53 | prometheanfire | mriedem: k, fyi, I think this will be blocking reqs updates on dogpile.cache | |
| 19:28:01 | dansmith | cfriesen: sure? but it's still twice the calls | |
| 19:28:17 | cfriesen | dansmith: for migrations in a single cell you could have the conductor get the data from neutron and then pass just the changed bits to both compute nodes | |
| 19:29:06 | mriedem | network-changed doesn't really have anything to do with the compute | |
| 19:29:12 | mriedem | so that one could be centralized | |
| 19:29:22 | dansmith | cfriesen: yes, you could have the api guess on all that and make those events different | |
| 19:29:32 | mriedem | network-vif-deleted and the vif plug/unplug ones call the driver so that's why they are on the compute | |
| 19:30:43 | mriedem | handling network-changed is an optimization that could be made at the top probably | |
| 19:30:48 | mriedem | but baby steps probably | |
| 19:31:38 | dansmith | mriedem: all events are handled by the computes now, so if you change that, you need to know which conductor (super or cell) to send events to, and then adding new things becomes a little less clear about where each is handled | |
| 19:32:10 | dansmith | making the operations that are happening less insanely heavy will yield much more benefit than cutting a big number in half in some situations | |
| 19:32:34 | mriedem | the former is what i'm trying to start with now yeah | |
| 19:32:40 | mriedem | by targeting the network-changed event to a single port | |
| 19:32:45 | mriedem | so we don't do the full nw info cache rebuild | |
| 19:32:48 | dansmith | yeah | |
| 19:33:01 | dansmith | I thought at one point we had a partial update thing like that | |
| 19:33:09 | dansmith | did we lose that or was it different? | |
| 19:33:20 | mriedem | not sure what you're referring to | |
| 19:33:31 | cfriesen | would it be reasonable for neutron to add more info about what exactly changed? (subnet, fixedip, floatingip, etc.) | |
| 19:33:47 | mriedem | cfriesen: maybe | |
| 19:33:54 | dansmith | we used to rebuild the whole cache every time and arosen did something to try to merge our networkinfo with just one change | |
| 19:34:27 | dansmith | cfriesen: you only get a tag in the event, but you could use that to signal what to refresh, yes, or have a different event like "floatingip-changed" | |
| 19:35:03 | cfriesen | even something like that could cut the number of neutron GETs down dramatically | |
| 19:35:03 | mriedem | https://review.openstack.org/#/q/owner:aaronorosen%2540gmail.com+status:abandoned+project:openstack/nova | |
| 19:35:10 | melwitt | dansmith: I put a lock around the cache access in https://review.openstack.org/465042 to address your comment | |
| 19:35:33 | dansmith | mriedem: no, this thing merged and caused a bunch of bugs, but I thought they all got squashed out | |
| 19:35:56 | dansmith | mriedem: anyway, not worth going to find it, it was 2013ish so just move on optimizing what we have now | |
| 19:35:57 | mriedem | dansmith: oh idk then, that would be like 3+ years ago now | |
| 19:36:00 | dansmith | yes | |
| 19:36:16 | dansmith | melwitt: I know, sorry | |
| 19:36:55 | melwitt | k, just in case you hadn't seen | |
| 19:37:04 | mriedem | anywho, i've got some patches to review which i promised (well didn't promise, but said maybe) i'd review, so going to do those first and then can cleanup this patch | |
| 19:39:01 | cfriesen | mriedem: dansmith: oh, and it turns out that once you have mriedem's "update the specific port" change, we don't really have any duplicate events for the same instance anymore | |
| 19:40:08 | mriedem | because they never were duplicates, | |
| 19:40:10 | mriedem | but nova just didn't know | |
| 19:40:12 | cfriesen | yeah | |
| 19:40:13 | mriedem | right? | |
| 19:42:25 | cfriesen | which actually means that the "vanilla" numbers for the neutron utilization would be much worse, since I had duplicate event removal in there. | |
| 19:42:46 | mriedem | do i get a large beer in denver for this? | |
| 19:42:52 | mriedem | s/beer/ginger ale/ | |
| 19:42:54 | cfriesen | oh, definitely | |
| 19:43:38 | melwitt | I don't understand why I would get "This change depends on a change that failed to merge." on a set where when I rebase it locally, I don't see any merge conflicts or auto-merging | |
| 19:43:50 | dansmith | melwitt: depends-on? | |
| 19:44:07 | melwitt | I don't think I have any but I'm checking again | |
| 19:44:42 | mriedem | melwitt: there used to be a thing with zuul where if you rebased a large series, it couldn't keep up with the stream of events, | |
| 19:44:46 | melwitt | yeah, I don't have any Depends-On. ugh, wtf | |
| 19:44:50 | mriedem | and you'd get a merge conflict, even though there wasn't one | |