| *** svenkat has joined #openstack-powervm | 01:34 | |
| *** esberglu has quit IRC | 01:35 | |
| *** svenkat has quit IRC | 02:31 | |
| *** apearson has joined #openstack-powervm | 03:53 | |
| *** apearson has quit IRC | 04:08 | |
| *** apearson has joined #openstack-powervm | 04:08 | |
| *** k0da has joined #openstack-powervm | 05:13 | |
| *** apearson has quit IRC | 05:42 | |
| *** k0da has quit IRC | 06:06 | |
| *** k0da has joined #openstack-powervm | 06:47 | |
| *** k0da has quit IRC | 08:01 | |
| *** AlexeyAbashkin has joined #openstack-powervm | 08:07 | |
| *** k0da has joined #openstack-powervm | 09:34 | |
| *** k0da has quit IRC | 10:15 | |
| *** efried is now known as efried_rollin | 11:32 | |
| *** apearson has joined #openstack-powervm | 13:29 | |
| *** svenkat has joined #openstack-powervm | 13:29 | |
| *** svenkat_ has joined #openstack-powervm | 13:32 | |
| *** svenkat has quit IRC | 13:33 | |
| *** svenkat_ is now known as svenkat | 13:33 | |
| *** efried_rollin is now known as efried | 13:35 | |
| *** k0da has joined #openstack-powervm | 14:09 | |
| *** svenkat has quit IRC | 14:23 | |
| *** esberglu has joined #openstack-powervm | 14:31 | |
| *** tjakobs has joined #openstack-powervm | 14:58 | |
| esberglu | edmondsw: Regarding https://review.openstack.org/#/c/546813/ | 15:27 |
|---|---|---|
| esberglu | I added the Get() call to the plug_vifs flow | 15:27 |
| esberglu | But I'm thinking it might be better to add Get() in a separate patch which will also change the destroy flow to use it | 15:28 |
| esberglu | And then build the hotplug on top | 15:28 |
| esberglu | Or we could wait until something else in the destroy flow needs it | 15:34 |
| esberglu | edmondsw: efried: I'm cutting queens RC2 today. Only minor changes since RC1 | 15:41 |
| esberglu | - Adding .gitreview for queens | 15:41 |
| esberglu | - Updating UPPER_CONSTRAINTS_FILE for queens | 15:41 |
| esberglu | - Adding iSCSI to the support matrix | 15:42 |
| esberglu | And thats it | 15:42 |
| edmondsw | esberglu ack for rc2 | 16:01 |
| edmondsw | as for the review, isn't Get() already IT, so we just need to use it here? | 16:02 |
| edmondsw | oh... no it's not... | 16:03 |
| edmondsw | efried, what do you think of that? See comments in the review esberglu linked above | 16:03 |
| efried | stand by | 16:03 |
| esberglu | edmondsw: We didn't bring it IT because unplug vifs in the destroy flow was the only place that used it and we were trying to keep LOC down for reviewers | 16:04 |
| edmondsw | we're inconsistent between plug and unplug whether we get the lpar_wrap or expect it to already be provided earlier in the flow | 16:04 |
| efried | Right, I remember ripping that out to reduce the footprint of an IT patch. | 16:04 |
| edmondsw | I mean we're inconsistent OOT | 16:04 |
| efried | Need to track down why we need the LPAR wrapper. | 16:04 |
| efried | I want to say it has something to do with the mgmt CNA | 16:04 |
| esberglu | edmondsw: Where are we inconsistent OOT? | 16:05 |
| esberglu | Create provides the lpar_wrap for spawn | 16:05 |
| esberglu | Get for plug_vifs | 16:05 |
| edmondsw | PlugVifs requires lpar_wrap, but UnplugVifs doesn't, it just gets it itself | 16:05 |
| efried | Does the lpar_wrap get used by more than one task? | 16:05 |
| efried | in that flow? | 16:06 |
| edmondsw | esberglu oh, nm... both have requires lpar_wrap OOT... not sure what I was looking at then | 16:06 |
| efried | in either flow, actually. | 16:06 |
| edmondsw | e.g. in spawn, the Create provides lpar_wrap and then the PlugVifs requires it | 16:06 |
| esberglu | efried: In spawn it gets used by PlugVifs and CreateAndConnectCfgDrive | 16:06 |
| esberglu | Only required by UnplugVifs in destroy (OOT only) | 16:07 |
| edmondsw | esberglu oh, it was inconsistent IT, not OOT | 16:07 |
| efried | Right, so in spawn since we need it in two places it makes sense for us to use a task so we can carry that same wrapper through. | 16:07 |
| efried | I think IT it was because we only needed it for cfg drive, before we had networking. So no need to have a whole task for it. | 16:08 |
| efried | or something. | 16:08 |
| edmondsw | there's also the question of whether some of our other tasks both IT and OOT should require lpar_wrap but don't today | 16:08 |
| edmondsw | e.g. in the destroy flow it's needed for PowerOff but that's not required there, it just gets it | 16:08 |
| efried | You mean if there are places we're grabbing the LPAR wrapper manually? | 16:08 |
| efried | Again, it's a matter of whether it's needed elsewhere in the flow. | 16:09 |
| edmondsw | sure | 16:09 |
| efried | I mean, we could argue that once we have the task we should use it for any flow where we ever need it. | 16:09 |
| efried | just for consistency. | 16:09 |
| edmondsw | it's not needed for a flow that only does power off, but it is needed for the destroy flow that does power off and other things like unplug vifs | 16:09 |
| efried | is it needed for unplug? Then sure. | 16:10 |
| edmondsw | changing PowerOff to require lpar_wrap would be a bigger change, though. I don't think we bite that off in this commit | 16:12 |
| edmondsw | I'm not really all that concerned about it | 16:12 |
| esberglu | edmondsw: Changing UnplugVifs to require lpar_wrap small enough to pile in with the hotplug change? Or should I do separate | 16:12 |
| edmondsw | I was more concerned about not breaking something with spawn | 16:12 |
| edmondsw | esberglu you could probably bundle in for unplug. | 16:13 |
| edmondsw | well... I guess you'd have to add tf_vm.Get for that since it's not already there... maybe do that separate | 16:14 |
| edmondsw | I'm not so concerned about unplug, and power off... more about plug | 16:14 |
| esberglu | edmondsw: What do you mean? | 16:15 |
| edmondsw | esberglu my first comment in the review | 16:15 |
| edmondsw | right, sorry I'm not being clear, otph | 16:16 |
| esberglu | edmondsw: By power off you mean destroy? | 16:16 |
| edmondsw | esberglu I think I'm happy with the change you made in PS3, but let me review it when my call is over | 16:18 |
| edmondsw | power off is called in destroy, which is how that came up | 16:18 |
| esberglu | edmondsw: Sounds good. The only other thing we could add into that is having the UnplugVifs task require lpar_wrap | 16:19 |
| esberglu | And call Get() in both the destroy and unplug_vifs flows | 16:19 |
| edmondsw | power off needs lpar_wrap, but gets it itself. If/when we add a Get() call in the destroy flow and use that for unplug, we could also use that for poweroff | 16:19 |
| esberglu | edmondsw: efried: CI is broken by https://github.com/openstack/tempest/commit/24a753dcfb | 16:40 |
| esberglu | It's causing all runs to pass by running 0 tests | 16:40 |
| efried | neat. | 16:41 |
| efried | esberglu: I don't know if you saw this comment the other day, but we should move our OOT projects over to stestr | 16:41 |
| efried | You know, once you pick the CI up off the floor. | 16:41 |
| esberglu | efried: Must have missed it, I'll add it to the list | 16:45 |
| efried | I think it was while you were out. | 16:47 |
| efried | btw esberglu edmondsw if y'all haven't voted for vancouver talks: https://www.openstack.org/summit/vancouver-2018/vote-for-speakers | 16:59 |
| efried | esberglu: there's some nice juicy looking CI stuff in there. | 16:59 |
| *** AlexeyAbashkin has quit IRC | 17:09 | |
| *** k0da has quit IRC | 17:14 | |
| edmondsw | efried is yours in there somewhere? | 17:22 |
| edmondsw | found it... https://www.openstack.org/summit/vancouver-2018/vote-for-speakers/#/20813 | 17:32 |
| efried | No vote padding | 18:05 |
| esberglu | efried: edmondsw: This stestr stuff is nice, we can rip out all the logic for generating the test lists | 18:28 |
| esberglu | And just pass it our blacklists directly | 18:28 |
| efried | It also makes it easier to run pdb, and to run tests without all the tox gorp | 18:29 |
| edmondsw | efried course not | 18:29 |
| *** AlexeyAbashkin has joined #openstack-powervm | 18:36 | |
| *** AlexeyAbashkin has quit IRC | 18:40 | |
| efried | edmondsw: I think I figgered it out. | 18:58 |
| efried | I don't like what it portends, though. | 18:58 |
| edmondsw | figured what out? | 18:58 |
| efried | https://review.openstack.org/#/c/545984/3/nova_powervm/tests/virt/powervm/test_driver.py | 18:58 |
| efried | PowerVMDriver.capabilities is a class property. | 18:59 |
| efried | Which is fine if it's a primitive | 18:59 |
| efried | But it's a dict. | 18:59 |
| efried | So setting | 18:59 |
| efried | self.capabilities['something'] = whatever | 18:59 |
| efried | actually sets the dict key on the *class* member. | 18:59 |
| efried | Which means any subsequent access of the class member will get that value. | 19:00 |
| efried | This is fine IRL because PowerVMDriver is a singleton. | 19:00 |
| efried | But in the test suite, you've got several flying around. | 19:00 |
| edmondsw | yeah... ugh | 19:00 |
| edmondsw | efried maybe | 19:06 |
| efried | maybe what? | 19:07 |
| edmondsw | maybe it's what you said, but I'm not sure | 19:07 |
| edmondsw | to your comment on the review... | 19:07 |
| efried | the faah - yeah, I tested it out and it doesn't seem to have fixed it. | 19:07 |
| edmondsw | I have a mock there, yes, but isn't that only supposed to create the mock_disk var, which hasn't been used yet? | 19:07 |
| edmondsw | "faah"? | 19:08 |
| efried | oh, that comment was unrelated, but may now be relevant. | 19:08 |
| edmondsw | efried and I think I tried splitting this into two methods and was still seeing LocalStorage mocked on the one that didn't have that decorator | 19:08 |
| efried | No, if you create a mock with a decorator, that thing gets mocked before you start running your test code. You don't have to access the var for the mock to go into effect. | 19:09 |
| edmondsw | boo... but my second statement stands | 19:11 |
| edmondsw | efried just confirmed... removing that decorator doesn't solve the issue | 19:13 |
| edmondsw | still mocked | 19:13 |
| efried | That's crazyballs. | 19:13 |
| edmondsw | yep | 19:15 |
| *** openstackgerrit has joined #openstack-powervm | 19:16 | |
| openstackgerrit | Matthew Edmonds proposed openstack/nova-powervm master: stop setting extra capabilities https://review.openstack.org/545984 | 19:16 |
| edmondsw | efried with that new PS, tox -e py27 TestPowerVMDriverInit works but tox -e py27 -- nova_powervm.tests.virt.powervm.test_driver does not | 19:17 |
| edmondsw | fails on the method without the decorator | 19:17 |
| efried | yeah, that's the mystery I'm trying to nail down right now. | 19:18 |
| edmondsw | because the driver has has_imagecache: 'has_imagecache': <MagicMock name='LocalStorage().capabilities.__getitem__()' | 19:18 |
| efried | where is that freakin mock coming from? | 19:19 |
| edmondsw | exactly | 19:19 |
| *** k0da has joined #openstack-powervm | 19:21 | |
| efried | oh | 19:25 |
| efried | Still the same problem. | 19:25 |
| efried | The second test class's setup is installing the fixture, which itself creates a new PowerVMDriver instance. And thereby mucks with the class vars or whatever. | 19:26 |
| efried | Though I haven't fully figured out how the disk driver is getting overwritten. | 19:27 |
| efried | But I think we ought to maybe fix this at the class level, rather than trying to do it in the tests. | 19:27 |
| efried | Which is kind of suck, because again IRL the driver should be a singleton. | 19:27 |
| efried | edmondsw: Okay, yeah. This delta has both ways of invoking the tests behaving the same way: | 19:30 |
| efried | edmondsw: http://paste.openstack.org/show/682375/ | 19:31 |
| efried | And it's not a *super* unreasonable change. | 19:31 |
| efried | BTW, we just might be getting rid of that dict and making capabilities be traits on the compute RP. | 19:32 |
| edmondsw | k, tx for figuring that out | 19:32 |
| openstackgerrit | Matthew Edmonds proposed openstack/nova-powervm master: stop setting extra capabilities https://review.openstack.org/545984 | 19:37 |
| efried | One more test case please edmondsw | 19:39 |
| edmondsw | that would be pointless... replied | 19:40 |
| edmondsw | efried ^ | 19:41 |
| *** k0da has quit IRC | 19:48 | |
| openstackgerrit | Eric Fried proposed openstack/nova-powervm master: stop setting extra capabilities https://review.openstack.org/545984 | 20:26 |
| efried | edmondsw: ^ worked up those tests without mocks. | 20:26 |
| efried | well, without mocking the whole disk drivers. | 20:27 |
| edmondsw | :( | 20:27 |
| edmondsw | I don't agree | 20:27 |
| efried | So the capabilities are being set by the disk drivers natively, not by mocks. | 20:27 |
| edmondsw | just beat me to hitting send | 20:27 |
| edmondsw | will look at what you did | 20:27 |
| efried | The less mocking the better. | 20:28 |
| efried | by which I mean less mocking in the path we're actually trying to test | 20:28 |
| efried | Shuffled stuff around so there's no setUp and no unnecessary mocking hopefully anywhere. | 20:28 |
| efried | tbh, I suspect we have the same problem with disk adapters here | 20:29 |
| efried | those guys are setting capabilities on the class attr's dict. | 20:29 |
| efried | the tests pass as written. But I'd be tempted to shunt those dict assignments into the inits for the disk drivers like for the compute driver. | 20:30 |
| edmondsw | efried replied | 20:35 |
| efried | edmondsw: I think I replied to your reply before you replied :) | 20:35 |
| edmondsw | efried we're talking a net increase in mocks here with your change | 20:36 |
| efried | But zero mocks in the path we're testing. As opposed to not zero. | 20:37 |
| edmondsw | ? | 20:37 |
| efried | I.e. the only things we're mocking are incidentals. Side-branches that we don't care about. | 20:37 |
| edmondsw | I don't like it, but whatever | 20:38 |
| efried | Vs. mocking the whole disk driver and mimicking what (we hope) it's doing with the capabilities dict. | 20:38 |
| efried | We do a lot of the latter in our unit tests, being very granular. | 20:38 |
| efried | Which I used to think was very nice and very unit-y. | 20:38 |
| efried | But since I've been working upstream and... osmosing the wisdom of their experienced folks... | 20:39 |
| efried | I've become convinced of the value of reducing mockage as much as feasible in the active path. | 20:39 |
| efried | edmondsw: Okay, so now what do we do for code reviews? If we both +2 it, esberglu can +2+W? | 20:40 |
| edmondsw | efried I don't see any logical possibility for a bug that PS5 wouldn't catch but PS6 would, so it just seems like an extra test for no reason, but that's ok | 20:40 |
| edmondsw | as for reviews, we're kinda stuck... I guess just one +2 from esberglu | 20:41 |
| efried | With PS5, we could make the disk driver inits raise exceptions and the tests would still pass. | 20:41 |
| esberglu | edmondsw: efried: Have been loosely following along, will catch up and review by EOD | 20:42 |
| efried | esberglu: Thanks. | 20:42 |
| efried | edmondsw and I will continue to circle each other with hands hovering over holsters | 20:42 |
| edmondsw | efried why would we care here if a disk driver init raised an exception? In that case, capabilities are moot, the compute service is hosed | 20:43 |
| efried | That's kind of my point, though not exactly what I meant. | 20:44 |
| edmondsw | if your point is that we should be doing _some_ testing that actually instantiates disk drivers, I'll agree with that | 20:44 |
| edmondsw | totally unrelated to capabilities | 20:45 |
| efried | So what if that test you linked is actually passing because something is mocked. And then someone goes in and refactors, moving the has_imagecache assignment somewhere, but accidentally misspells it in the new location. Nobody notices because all the tests pass and it's not an obvious typo. | 20:46 |
| efried | It's all hypothetical, of course. | 20:46 |
| *** k0da has joined #openstack-powervm | 20:46 | |
| efried | But instead of having one test that says "Assuming disk driver does what it oughtta, compute driver does the right thing" and another test that says "Disk driver does the right thing" makes more assumptions and is more brittle than "Compute driver does the right thing for each disk driver". | 20:47 |
| edmondsw | -1 for a different reason | 20:48 |
| efried | oh now you're just being spiteful :P | 20:49 |
| efried | This has passed the point of diminishing returns. Let's put it to bed. | 20:50 |
| *** k0da has quit IRC | 20:51 | |
| edmondsw | yeah, I just don't want to argue anymore | 20:52 |
| edmondsw | esberglu please update your nova commit that adds capabilities IT to match | 20:54 |
| efried | We ended up with better code and more robust tests here. This is a good thing. | 20:55 |
| efried | strange and winding though the road may have been. | 20:55 |
| edmondsw | yep | 20:56 |
| edmondsw | esberglu we might actually need to propose that in 2 separate commits for IT... 1) capabilities, 2) attach interface | 20:58 |
| edmondsw | I can propose the first if you like, and then you can rebase the second on that | 20:58 |
| edmondsw | or is it easier for you to do both? | 20:58 |
| esberglu | edmondsw: Either way. I probably won't get to either until tomorrow, still sorting out this stestr CI stuff | 20:59 |
| edmondsw | the reason I think it needs to be split into 2 commits is that along with adding capabilities to the driver, we need to add the tests, and that just starts to get too far away from being about attach interface | 20:59 |
| edmondsw | esberglu k, I'll throw something up | 20:59 |
| esberglu | edmondsw: Yep I agree 2 is the way to go | 20:59 |
| edmondsw | esberglu but I'll wait for your +2 on the OOT change... in case we need more rounds on that :) | 21:00 |
| * edmondsw crosses fingers hoping we're done there | 21:00 | |
| esberglu | edmondsw: You cool with the hotplug patch otherwise? And then a separate patch using Get() everywhere that it isn't being used currently | 21:00 |
| edmondsw | esberglu yes, and I'm not even sure how much I care about using Get() everywhere... seems like a "we should probably do this but isn't super important" thing to throw on a TODO list | 21:01 |
| esberglu | sounds good | 21:02 |
| edmondsw | haven't studied the tests yet, but the code itself is fine | 21:02 |
| *** k0da has joined #openstack-powervm | 21:03 | |
| esberglu | efried: edmondsw: 6356 for stestr CI | 21:07 |
| efried | ack | 21:07 |
| edmondsw | ack | 21:07 |
| esberglu | Still need to run it through front to back on a fresh machine, but it appears to be working | 21:08 |
| edmondsw | esberglu is pike_it_blacklist.txt used when testing pike, and in_tree_blacklist.txt used for everything else? | 21:15 |
| esberglu | edmondsw: pikt_it_blacklist gets appended to in_tree_blacklist for pike runs | 21:16 |
| edmondsw | k | 21:17 |
| edmondsw | esberglu does that same sed expression still work? L31 in os_ci_tempest_intree.conf | 21:18 |
| esberglu | edmondsw: Yep | 21:18 |
| edmondsw | cool | 21:18 |
| edmondsw | esberglu and --parallel is not needed for stestr because... ? | 21:21 |
| esberglu | edmondsw: --concurrency is enough to know you want parallel | 21:22 |
| edmondsw | LGTM | 21:23 |
| esberglu | efried: edmondsw: I'm good with 545984, gonna +2, W+1 | 21:26 |
| efried | esberglu: Cool man, thanks. | 21:26 |
| * efried blows smoke from pistol | 21:26 | |
| efried | Sorry guys, we've been doing a western theme thing in -nova. | 21:26 |
| esberglu | efried: edmondsw: FYI I will be out tomorrow afternoon | 21:29 |
| efried | I'll be out all day tomorrow (most of it sitting at JFK) | 21:29 |
| efried | And PTG next week. And vacation the week after. | 21:29 |
| efried | And probably knocking off pretty soon here, as I got on around 4:30am today. | 21:29 |
| esberglu | efried: You have time to look at 6356 before you go? If not I'll just submit with the edmondsw+2 so we don't have to wait until next week for CI to work again | 21:32 |
| esberglu | I'll be pretty confident once this manual run get through | 21:32 |
| efried | Sorry, looking... | 21:35 |
| efried | esberglu: +2, nice one guvnor. | 21:38 |
| esberglu | efried: tx | 21:38 |
| esberglu | efried: edmondsw: One more iteration on 6356 | 21:50 |
| esberglu | Missed the stestr init | 21:50 |
| edmondsw | ack | 21:51 |
| efried | esberglu: Done. | 21:51 |
| efried | I did notice that deletion, but assumed it was yet another thing that wasn't needed. | 21:51 |
| edmondsw | same | 21:52 |
| esberglu | I didn't think it was needed, but I forgot that I ran that manually when I first started goofing around | 21:53 |
| openstackgerrit | Merged openstack/nova-powervm master: stop setting extra capabilities https://review.openstack.org/545984 | 22:18 |
| openstackgerrit | Eric Berglund proposed openstack/nova-powervm master: DNM: ci check https://review.openstack.org/328315 | 22:26 |
| openstackgerrit | Eric Berglund proposed openstack/nova-powervm master: DNM: CI Check2 https://review.openstack.org/328317 | 22:26 |
| *** esberglu has quit IRC | 22:32 | |
| *** tonyb has quit IRC | 22:35 | |
| *** tonyb has joined #openstack-powervm | 22:36 | |
| *** tjakobs has quit IRC | 22:46 | |
| *** k0da has quit IRC | 23:09 | |
| *** apearson has quit IRC | 23:19 | |
| *** k0da has joined #openstack-powervm | 23:25 | |
| *** efried has quit IRC | 23:32 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!