| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-04 | |||
| 16:49:19 | dansmith | artom: I understand | |
| 16:49:21 | artom | From the virt driver, I should say. It's a call to the compute manager | |
| 16:49:46 | artom | Hrmm, there's no RPC, mind you | |
| 16:49:56 | artom | Just... a whole bunch of intermediary steps | |
| 16:50:07 | bauzas | gibi: I'm on it | |
| 16:50:13 | gibi | bauzas: thanks a lot | |
| 16:50:29 | bauzas | I wish I could help better, that's it | |
| 16:51:45 | artom | dansmith, anyways, I'll try to come up with a least bad way of doing it, we'll have a strawman to pick apart | |
| 16:52:03 | artom | At least you're on board with the need for change ;) | |
| 16:54:06 | mriedem | artom: so we likely need 2 context managers to wait for separate events right? | |
| 16:54:15 | mriedem | one for the port binding host change, and one for the vif plug | |
| 16:54:29 | mriedem | that port binding host change happens in several move operations we'd care about (at least resize and evacuate) | |
| 16:54:37 | mriedem | and unshelve i think... | |
| 16:54:58 | artom | mriedem, hrmm, you lost me, how would 2 context managers solve this? | |
| 16:55:14 | mriedem | couldn't we create a helper method for that neutronv2.api method that updates the host in the port binding to register an event callback | |
| 16:55:22 | mriedem | artom: we have 2 events to wait for yeah? | |
| 16:55:40 | mriedem | so you wait for the first (port binding host change), then call the driver to spawn or whatever which does the vif plug | |
| 16:55:45 | mriedem | which triggers the event we've always waited for | |
| 16:56:14 | artom | mriedem, uhh, I thought it was Nova that PUT'ed the post binding host change? | |
| 16:56:28 | mriedem | it does | |
| 16:56:38 | artom | And that got us a vif-plugged as a "response"? | |
| 16:56:42 | mriedem | and then we get an event from neutron that we're not waiting for | |
| 16:57:08 | mriedem | and then we actually plug vifs which produces another event yeah? | |
| 16:57:11 | dansmith | artom: was it not updating instance_extra.migration_context, or was it not persisting the applied-from-context instance attributes? | |
| 16:57:13 | artom | We are, sort us, we just start waiting after it gets to us, but yeah, you're basically right | |
| 16:57:15 | dansmith | artom: I had assumed the former | |
| 16:57:29 | artom | dansmith, the latter | |
| 16:57:32 | dansmith | ah | |
| 16:57:42 | dansmith | artom: which attributes of those specifically? | |
| 16:58:00 | artom | dansmith, so I haven't looked into it in detail, but I'm told the new NUMA topology | |
| 16:58:02 | dansmith | numa_topology? | |
| 16:58:11 | dansmith | you probably need to do something to make those dirty, | |
| 16:58:16 | artom | ... | |
| 16:58:23 | dansmith | since they're nested objects, they probably only save if the object itself is dirty | |
| 16:58:35 | dansmith | artom: you need to give them "the talk" | |
| 16:59:09 | artom | Right, tell them they've been naughty and need to be saved from their sins? | |
| 17:00:05 | mdbooth | lyarwood: Ok, I can see in the tempest logs that tempest receives a 404 for GET volume *before* deleting the type, and I can see the same in the cinder logs. However, c-vol doesn't log the completion of the delete until some time later, which is *after* the attempted deletion of the type. | |
| 17:00:23 | dansmith | they must not be marked as changed in the migration context, so I'm thinking just applying them to the instance and then calling save doesn't have them included in obj_what_changed() | |
| 17:00:31 | openstackgerrit | Matt Riedemann proposed openstack/python-novaclient master: Microversion 2.71 - show server group https://review.openstack.org/640657 | |
| 17:00:36 | artom | mriedem, wait, plugging the vifs produces *another* event? | |
| 17:01:06 | artom | mriedem, I need to flow through the code again in that case, because it means I don't have a complete picture | |
| 17:01:08 | sean-k-mooney | artom: binding the vif produces a network-chaged event | |
| 17:01:13 | mriedem | artom: that was the original reason for adding the context manager in the virt driver | |
| 17:01:20 | artom | Oh, so binding -> network-changed | |
| 17:01:27 | artom | vig plugging -> vif-plugged | |
| 17:01:34 | mriedem | i don't think that's correct | |
| 17:01:36 | sean-k-mooney | when the neutron agent plugs the vif we get a network-vif-plugged event | |
| 17:01:57 | mriedem | if they were different events, we wouldn't be missing the 2nd event | |
| 17:02:31 | dansmith | the problem is the vif shows up on the destination, neutron notices and immediately sends the vif-plugged event right? | |
| 17:02:32 | mdbooth | mriedem: lyarwood was having trouble with the timestamp tests being flaky and removed them in https://review.openstack.org/#/c/639331/ . I vaguely recall you previously expressing an opinion on these tests. | |
| 17:02:33 | sean-k-mooney | i dont think we need to move the handeling of the network-chaged event | |
| 17:02:35 | dansmith | before nova is ready to start it | |
| 17:02:43 | mriedem | as far as i understand the problem, we change the port binding host and then plug vifs, which normally happens close enough that neutron sends a single network-vif-plugged event which we're waiting for by the time we plugged vifs and registered a callback in the virt driver | |
| 17:02:49 | sean-k-mooney | dansmith: yep basically | |
| 17:03:31 | dansmith | mriedem: and this is happening in the migrate case in a different place than it happens during spawn and that's the issue, right? | |
| 17:03:56 | sean-k-mooney | neutron sees the port on the sorce because we have not clean it up yet so when the rollback of the migration happens and we set the host back to the source neturon send the vif-plugged event right away | |
| 17:04:51 | dansmith | oh, it's the rollback case only? | |
| 17:05:28 | artom | dansmith, revert resize is the downstream bug | |
| 17:05:38 | artom | Hrmm, do we actually have 2 problems here? | |
| 17:05:46 | dansmith | okay, I'm even less concerned then :) | |
| 17:05:46 | sean-k-mooney | dansmith: artom is fixing the rollback case. there is another race with sdn controler that send vif-plugged when you bind the port that this will also fix | |
| 17:05:46 | bauzas | gibi: left yet another comment https://review.openstack.org/#/c/623543/39 | |
| 17:05:57 | bauzas | gibi: can't we do all of this on the libvirt driver anyway? | |
| 17:06:12 | dansmith | the rollback case could just not wait for events since it's already on the source, I would think.. that would be a band-aid worth doing I would think | |
| 17:06:39 | bauzas | gibi: but I guess it's not possible because you wanna claim() this | |
| 17:06:59 | gibi | bauzas: looking | |
| 17:07:39 | artom | dansmith, that'd be a weird bit of into to pass down to the virt driver, no? | |
| 17:07:47 | artom | ... cuz we just call spawn there | |
| 17:08:03 | dansmith | we already pass a similar flag in places don't we? | |
| 17:08:04 | mriedem | mdbooth: i don't really remember that, but i'm assuming that timestamp stuff was copied from other scenario tests, and the commit message doesn't really explain the problem with the timestamp stuff | |
| 17:08:12 | mriedem | just "it's bad" isn't very good | |
| 17:08:23 | artom | dansmith, I'd have to check | |
| 17:08:44 | dansmith | maybe I'm thinking about within the driver, but still | |
| 17:08:47 | mriedem | dansmith: upstream it's at least known to be happening in evacuate | |
| 17:08:58 | mriedem | http://status.openstack.org/elastic-recheck/#1813789 | |
| 17:09:01 | gibi | bauzas: yeah we need the info to drive the pci claim | |
| 17:09:08 | dansmith | mriedem: on evacuate, not evacuate and then restart-on-original-host right? | |
| 17:09:13 | sean-k-mooney | ya i was going to say the same. but dan is right we could skip waiting in this case but i kindo of like not have another flag to pass down | |
| 17:09:18 | gibi | bauzas: does compute/manager have access to the ProvideTree? | |
| 17:09:19 | bauzas | gibi: but you understand my concern, right? | |
| 17:09:32 | dansmith | mriedem: the evac case seems ripe for just moving where we wait, but I haven't gone digging | |
| 17:09:39 | bauzas | gibi: that's the problem, I don't think so | |
| 17:09:39 | sean-k-mooney | the flag would only fix the rollback case. artom change would fix the cisco aci/old odl case | |
| 17:09:41 | gibi | bauzas: I understand that we call placement a lot | |
| 17:09:47 | sean-k-mooney | in addtion to rollbacks | |
| 17:10:12 | dansmith | artom: thinking of this: vifs_already_plugged=False, | |
| 17:10:18 | bauzas | gibi: and say you have two pci requests for the same interface... | |
| 17:10:22 | mriedem | gibi: bauzas: pretty sure the compute manager has a handle to the provider tree, | |
| 17:10:26 | bauzas | it's a bit sad to query it twice | |
| 17:10:27 | mriedem | because it has to clear it on sighup | |
| 17:10:41 | gibi | mriedem: ooh | |
| 17:10:41 | mriedem | dansmith: i don't know the answer to that offhand | |
| 17:10:47 | artom | dansmith, ah, OK. It's still only a bandaid tho... | |
| 17:10:59 | mriedem | gibi: bauzas: https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L551 | |
| 17:11:02 | artom | dansmith, thanks, that'll allow me to eat :P | |
| 17:11:10 | gibi | bauzas: sure your concern is valid either I would like to use a ProviderTree or add a local cache | |
| 17:12:00 | gibi | mriedem: looking | |
| 17:12:13 | mdbooth | mriedem: The purpose of the timestamp stuff IIUC is to assert that the contents of the volume are preserved across the migration. I personally think it's an important test, but if it's problematic in practise we'd need to be pragmatic, e.g. removing or guarding it. NP if you don't have a strong opinion, lyarwood and I can fling fedoras at each other internally :) | |
| 17:13:26 | mriedem | well, give a justifiable reason in the commit message to remove it | |
| 17:13:26 | gibi | bauzas, mriedem: so here in the compute/manager we only need the name of the RP and we know the UUID of it. So I guess I should be able to use the ProvideTree cache for that | |