| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-05 | |||
| 17:17:43 | openstackgerrit | Stephen Finucane proposed openstack/nova master: api: Remove 'Debug' middleware https://review.opendev.org/662506 | |
| 17:19:32 | stephenfin | mriedem: Thanks for the tip. We'll see how this goes https://review.opendev.org/663386 | |
| 17:34:48 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Cache host to cell mapping in HostManager https://review.opendev.org/663388 | |
| 17:34:48 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Convert HostMapping.cells to a dict https://review.opendev.org/663387 | |
| 17:38:07 | artom | I feel like I've messed up my method if I need to mock like 42 things when unit testing it :( | |
| 17:38:25 | mriedem | your method is too big | |
| 17:38:57 | artom | That's never been a problem before | |
| 17:50:51 | edleafe | artom: generally a "unit" doesn't have 42 dependencies. | |
| 17:51:46 | artom | 42 is obviously an exaggeration, but yeah, this bit of code is heavily coupled to a lot of things | |
| 17:54:20 | edleafe | artom: yeah, that's more of an integration test then. | |
| 18:10:03 | artom | I probably can't, because external events | |
| 18:10:17 | artom | Pretty sure the NeutronFixture doesn't handle those ;) | |
| 18:10:46 | artom | I suppose I can mock that part out... | |
| 18:11:43 | dansmith | artom: you understand that the concern is just that you didn't test that those parts are at all connected, and since you don't have a gate test to make mriedem happy, there really needs to be confirmation that they work together, right? | |
| 18:11:57 | dansmith | I mean, I'm pretty sure I could have removed the actual change you made and no tests would fail | |
| 18:12:42 | artom | dansmith, right, IOW, we need to test that if _uses_hybrid_plug returns true, we actually wait for events in the compute manager | |
| 18:12:54 | dansmith | at a minimum, I want to see a list of vif network_info structure passed into that method, and have the method not wait for events because it calls the helper and it examines the vif structures properly (and one for the opposite case if not already covered elsewhere) | |
| 18:13:19 | dansmith | artom: that's one piece yes | |
| 18:14:11 | dansmith | I'm totally uninterested in discussing the meaning of "unit" and how it relates to this, and only really interested in seeing that it works | |
| 18:14:48 | artom | dansmith, what's the other piece? By mocking _uses_hybrid_plug to return False, it's testing that the existing code paths continue to work | |
| 18:15:54 | dansmith | artom: the other piece being that you actually called the helper in a compatible way.. if all you do is check that the thing called your mock with who-knows-what and the mock always returns true or false, that doesn't mean it actually works when the parent calls the actual child implementation | |
| 18:16:12 | dansmith | by testing only units against mocks, you don't actually confirm that they're compatible with each other | |
| 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 | 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: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: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 | sean-k-mooney | im trying to think how we would even fake that out in a functional test | |
| 18:44:35 | artom | sean-k-mooney, the evacuate case? Maybe in a different patch, but why put it in mine? | |
| 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 | dansmith | no, it's not paused on the source | |
| 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: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) | |