| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-05 | |||
| 18:16:46 | artom | dansmith, you just put words on why I'm not liking my current mock-42-things approach | |
| 18:17:56 | dansmith | the world can't be reduced to tiny methods with only two external interactions each, so sometimes things get messy | |
| 18:18:15 | dansmith | I'm sure that method is too complex and refactoring won't help for backportability, so.. reality strikes | |
| 18:18:44 | artom | Stupid reality | |
| 18:23:39 | edleafe | artom: it does sound like you will be better off with a functional test that shows those parts working together as intended. Otherwise, you're just testing the python mock library :) | |
| 18:31:06 | sean-k-mooney | dansmith: i just replied to your question on the other move operations https://review.opendev.org/#/c/644881/19/nova/compute/manager.py@4192 i belive they are safe but it would be good for someone else to double check my logic | |
| 18:31:29 | dansmith | sean-k-mooney: I bet mriedem has thoughts on that | |
| 18:32:39 | sean-k-mooney | do we allow evacuate to be reverted by the way? | |
| 18:32:58 | sean-k-mooney | i did not think so be honestly i have never looked | |
| 18:33:07 | dansmith | unfortunately we do, within a window | |
| 18:33:24 | sean-k-mooney | ok so that is one case that porably needs to be handeled | |
| 18:36:07 | artom | Wait, evacuate can be reverted? | |
| 18:36:45 | artom | Also I just realized that the bug my patch links to is probably irrelevant :/ | |
| 18:37:27 | sean-k-mooney | thats the bug the waiting behavior was added to fix | |
| 18:37:40 | sean-k-mooney | i think | |
| 18:37:42 | dansmith | artom: not revert in the conventional sense, | |
| 18:37:49 | artom | sean-k-mooney, I mean https://bugs.launchpad.net/nova/+bug/1813789 | |
| 18:37:50 | openstack | Launchpad bug 1813789 in OpenStack Compute (nova) "Evacuate test intermittently fails with network-vif-plugged timeout exception" [Medium,In progress] - Assigned to Artom Lifshitz (notartom) | |
| 18:38:01 | sean-k-mooney | artom: yes i know | |
| 18:38:08 | dansmith | but if you start an evacuation, it fails, and then the original source comes back alive, you can keep it there, barring bugs and timing and all kinds of crazy | |
| 18:38:08 | artom | Even if the symptoms are the same, it has to be a different root cause, because upstream doens't use hybrid plug | |
| 18:38:13 | sean-k-mooney | artom: that was the bug we intoduce the wait on revert to fix | |
| 18:38:14 | dansmith | which I wish wasn't the case | |
| 18:38:48 | artom | dansmith, that couldn't have been on purpose... | |
| 18:39:20 | sean-k-mooney | dansmith: im assuming that only works if we have not updated the instance host yet | |
| 18:39:21 | dansmith | artom: nothing about evacuate was done with a great sense of purpose, but it was decided not to make it explicitly disallowed (to stay put) | |
| 18:39:26 | sean-k-mooney | e.g. we failed before that point | |
| 18:39:36 | dansmith | personally I'd prefer that the instant you start an evac, the instance _has_ to move somewhere | |
| 18:39:40 | sean-k-mooney | and it jsut happend to work if the host became alive again? | |
| 18:39:44 | dansmith | sean-k-mooney: yeah | |
| 18:40:42 | sean-k-mooney | ok so its just happening because we dont keep retrying to move the instance after the failure | |
| 18:40:55 | sean-k-mooney | and when it fails we dont update the db | |
| 18:41:39 | dansmith | it depends on when we fail | |
| 18:41:41 | dansmith | .of course | |
| 18:41:48 | dansmith | which is why it seems broken to me, | |
| 18:42:13 | sean-k-mooney | so i proably know the answer to this but do we really care about that edgecase? | |
| 18:42:14 | dansmith | especially because HA stuff that has made a decision to rebuild the instance is easier knowing it has fenced the original node, etc | |
| 18:42:36 | sean-k-mooney | i mean we proably should but other stuff is proably broken at that point | |
| 18:42:43 | dansmith | sean-k-mooney: IIRC, mriedem (and maybe others) really want to keep the "can keep it on the source if it's not too late" behavior | |
| 18:43:06 | sean-k-mooney | dansmith: no im kind of ok with that | |
| 18:43:12 | sean-k-mooney | what i ment was for artom patch | |
| 18:43:27 | dansmith | oh, yeah, I think that's worth investigating and fixing if it's related | |
| 18:43:35 | sean-k-mooney | do we need to add in extra code to handel that case | |
| 18:44:35 | artom | sean-k-mooney, the evacuate case? Maybe in a different patch, but why put it in mine? | |
| 18:44:35 | sean-k-mooney | im trying to think how we would even fake that out in a functional test | |
| 18:45:00 | artom | dansmith, so, turns out I was being too paranoid too soon about mocking things, and it actully works out pretty clean | |
| 18:45:01 | sean-k-mooney | artom: just because dansmith asked about the other move operation in yours | |
| 18:45:21 | sean-k-mooney | but it could be in another patch | |
| 18:45:31 | dansmith | sean-k-mooney: and when mriedem reviews it, that's going to be one of the first things he says :) | |
| 18:46:16 | sean-k-mooney | well i dont think artoms patch makes that case any worse but it also might not fix things in that case | |
| 18:46:32 | sean-k-mooney | assuming its currently broken in that case | |
| 18:47:15 | dansmith | it's a matter of making sure we don't re-have this conversation in a year when someone reports it for evacuate, and maybe fix it a different way | |
| 18:47:36 | sean-k-mooney | ya thats fair | |
| 18:47:41 | dansmith | also, I think hard reboot used to wait, now doesn't, potentially because we had this convo already and didn't go deep enough | |
| 18:47:54 | dansmith | https://review.opendev.org/#/c/553035/ | |
| 18:48:01 | artom | But... I thought the whole thing here is that hybrid plug is special in how it wires vifs | |
| 18:48:03 | dansmith | heh, about a year ago | |
| 18:48:06 | sean-k-mooney | well not quite | |
| 18:48:24 | artom | And resize is special in that it leaves in the instance paused on the source until confirm or revert | |
| 18:48:39 | sean-k-mooney | some network backeds only send events on port bind and we dont bind the ports on hard reboot or rebuild | |
| 18:48:39 | dansmith | no, it's not paused on the source | |
| 18:48:42 | dansmith | it's off | |
| 18:48:43 | artom | So.. how would that be applicable for evacuate, when the instance was never on the dest to begin with? | |
| 18:48:51 | artom | dansmith, sorry, off (aka destoryed) | |
| 18:49:28 | artom | sean-k-mooney, ah, so some backends might send the event as soon as we change the port binding to a host, regardless if it's wired or not? | |
| 18:49:44 | sean-k-mooney | artom: yes | |
| 18:49:51 | artom | I'd say that's a bug in those backends | |
| 18:50:00 | sean-k-mooney | it was by design | |
| 18:50:06 | artom | It shouldn't be sending the event if the thing isn't actually plugged/wired | |
| 18:50:39 | sean-k-mooney | not all networking backend can support that or at least could back when this was firt intoduced | |
| 18:50:58 | artom | That doesn't invalidate my argument :) | |
| 18:51:08 | sean-k-mooney | odl took 2 years to catch up and intoduce a way for odl to send events to neutron which neutron could send to nova | |
| 18:51:14 | artom | The idea was for Nova to be sure we have networking for instance by the time it spawns | |
| 18:51:17 | artom | So things like DHCP work | |
| 18:51:27 | artom | If we get the event before that's happened, that's not for Nova to fix | |
| 18:51:49 | sean-k-mooney | yep i had this argument with the odl folk back in 2014 | |
| 18:51:53 | artom | Because we'd be acting like networking is ready, when in reality it isn't | |
| 18:52:01 | artom | (yey, 5 years late) | |
| 18:52:39 | sean-k-mooney | for what its worth im pretty sure netorking-aci still does not send it when the port is wireded up an send it on bind | |
| 18:52:57 | sean-k-mooney | and ovn is not doing the right thing either in all cases | |
| 18:52:58 | artom | The Cisco thing? | |
| 18:53:02 | sean-k-mooney | yes | |
| 18:53:14 | artom | That's just one more argument for people not to buy Cisco ;) | |
| 18:53:36 | mriedem | "IIRC, mriedem (and maybe others) really want to keep the "can keep it on the source if it's not too late" behavior" | |
| 18:53:40 | mriedem | i have no opinion on that | |
| 18:53:42 | mriedem | that i'm aware of | |
| 18:54:14 | mriedem | i'm also assuming i don't want to cruft up an already complicated change to cover other cases in the same patch if we're doing backports | |
| 18:54:23 | mriedem | b/c there is a >0% chance this will introduce a regression somehow | |
| 18:54:27 | mriedem | b/c this is all very brittle sounding | |
| 18:54:30 | mriedem | and super duper latent | |
| 18:55:12 | mriedem | how would one even recreate this hybrid ovs resize thing in the gate? configure devstack for that backend (is that easy?) and then put a sleep in the nova code between migrate_instance_start(finish?) and the call to the virt driver? | |
| 18:55:16 | artom | Like osteoporosis | |
| 18:56:14 | sean-k-mooney | mriedem: we have patches to recreate. you configure ovs of iptables and then we can race on teh revert tests | |
| 18:56:39 | sean-k-mooney | *ovs to use the iptables firewall driver | |
| 18:56:53 | artom | https://review.opendev.org/#/c/660782/5 specifically | |
| 18:57:59 | sean-k-mooney | our downstream ci default to iptables firewall still on older releases so that is why we are seeing it downstream. upstream use the openvswtich contrack firewall driver by default | |
| 18:58:53 | mriedem | why does https://review.opendev.org/#/c/653498/ come after the fix in the series? | |
| 18:59:27 | artom | mriedem, because without hybrid plug it's actually irrelevant to the fix :/ | |
| 18:59:32 | artom | Well | |
| 18:59:39 | artom | I suppose it tests the non-hybrid-plug case | |