| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-05 | |||
| 16:43:42 | sean-k-mooney | but its more complicated | |
| 16:44:33 | sean-k-mooney | kashyap: it would be nice if we just had a propatable set defiend by qemu or libvirt that all distos used | |
| 16:44:55 | kashyap | Sorry, what is a "propatable"? | |
| 16:45:01 | sean-k-mooney | portable | |
| 16:45:30 | sean-k-mooney | or interoperable | |
| 16:45:44 | kashyap | Aaah, I don't know how feasible that is; need to think with a fresh brain :-) | |
| 16:45:54 | kashyap | Alright, need to go cook make food. See ya | |
| 16:46:35 | sean-k-mooney | i think recent version of libvirt or qemu have got better of checking compatibliy if you use the alais e.g. "q35" | |
| 16:47:26 | sean-k-mooney | but i dont know if it will allow q35-centos... to migrate to q35-rhel... even if they are the same | |
| 16:48:00 | sean-k-mooney | i know it did not work betweeen centos and ubunut in the past but if sepcifically set them to the same machine type it did | |
| 16:50:26 | aram1s | Hi there! Does anyone know what the 'state' represents for hypervisors? I thought it reflected the state of nova compute service but when I disable it I still see it as up | |
| 16:50:52 | aram1s | how can I make it go down? | |
| 16:51:30 | aram1s | without explicitly setting it as down. Will it chance based on any other status? | |
| 16:51:37 | aram1s | change* | |
| 17:00:16 | melwitt | aram1s: status will also change if you for example, stop the nova compute service | |
| 17:00:43 | melwitt | other than that, there's the forced_down API https://developer.openstack.org/api-ref/compute/?expanded=update-forced-down-detail#update-forced-down | |
| 17:01:48 | stephenfin | mriedem: Could you drop your -2 from these given my comments inline? https://review.opendev.org/#/c/662501/ https://review.opendev.org/#/c/662502/ | |
| 17:03:04 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Unplug VIFs as part of cleanup of networks https://review.opendev.org/663382 | |
| 17:08:08 | mriedem | stephenfin: of course the ec2 objects aren't used outside of nova https://review.opendev.org/#/c/662502/ | |
| 17:08:22 | mriedem | the ec2 API shim within nova is what's going to be used by the ec2api code | |
| 17:08:28 | mriedem | which uses those objects | |
| 17:08:38 | stephenfin | It doesn't though | |
| 17:08:42 | mriedem | the ec2utils stuff might be different | |
| 17:09:14 | stephenfin | Not used anywhere outside of nova and not used inside nova either | |
| 17:09:55 | stephenfin | At least not once I've removed the unused functions from ec2utils | |
| 17:10:00 | mriedem | i'd feel a lot more comfortable with this if we could get a tempest run on the ec2api with a dependency on this series | |
| 17:10:23 | stephenfin | I can try figure out how to do that | |
| 17:11:03 | mriedem | it should be as easy as adding openstack/nova to https://github.com/openstack/ec2-api/blob/master/.zuul.yaml#L7 and adding a depends-on to your series | |
| 17:16:58 | aram1s | thanks melwitt! | |
| 17:17:40 | openstackgerrit | Stephen Finucane proposed openstack/nova master: ec2: Remove unused functions from 'ec2utils' https://review.opendev.org/662501 | |
| 17:17:41 | openstackgerrit | Stephen Finucane proposed openstack/nova master: objects: Remove unused ec2 objects https://review.opendev.org/662502 | |
| 17:17:41 | openstackgerrit | Stephen Finucane proposed openstack/nova master: ec2: Remove ec2.CloudController https://review.opendev.org/662503 | |
| 17:17:42 | openstackgerrit | Stephen Finucane proposed openstack/nova master: ec2: Pre-move cleanup of utils https://review.opendev.org/662504 | |
| 17:17:42 | openstackgerrit | Stephen Finucane proposed openstack/nova master: ec2: Move ec2utils functions to their callers https://review.opendev.org/662505 | |
| 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: Convert HostMapping.cells to a dict https://review.opendev.org/663387 | |
| 17:34:48 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Cache host to cell mapping in HostManager https://review.opendev.org/663388 | |
| 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 | 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? | |