Thursday, 2018-02-22

*** svenkat has joined #openstack-powervm01:34
*** esberglu has quit IRC01:35
*** svenkat has quit IRC02:31
*** apearson has joined #openstack-powervm03:53
*** apearson has quit IRC04:08
*** apearson has joined #openstack-powervm04:08
*** k0da has joined #openstack-powervm05:13
*** apearson has quit IRC05:42
*** k0da has quit IRC06:06
*** k0da has joined #openstack-powervm06:47
*** k0da has quit IRC08:01
*** AlexeyAbashkin has joined #openstack-powervm08:07
*** k0da has joined #openstack-powervm09:34
*** k0da has quit IRC10:15
*** efried is now known as efried_rollin11:32
*** apearson has joined #openstack-powervm13:29
*** svenkat has joined #openstack-powervm13:29
*** svenkat_ has joined #openstack-powervm13:32
*** svenkat has quit IRC13:33
*** svenkat_ is now known as svenkat13:33
*** efried_rollin is now known as efried13:35
*** k0da has joined #openstack-powervm14:09
*** svenkat has quit IRC14:23
*** esberglu has joined #openstack-powervm14:31
*** tjakobs has joined #openstack-powervm14:58
esbergluedmondsw: Regarding
esbergluI added the Get() call to the plug_vifs flow15:27
esbergluBut I'm thinking it might be better to add Get() in a separate patch which will also change the destroy flow to use it15:28
esbergluAnd then build the hotplug on top15:28
esbergluOr we could wait until something else in the destroy flow needs it15:34
esbergluedmondsw: efried: I'm cutting queens RC2 today. Only minor changes since RC115:41
esberglu- Adding .gitreview for queens15:41
esberglu- Updating UPPER_CONSTRAINTS_FILE for queens15:41
esberglu- Adding iSCSI to the support matrix15:42
esbergluAnd thats it15:42
edmondswesberglu ack for rc216:01
edmondswas for the review, isn't Get() already IT, so we just need to use it here?16:02
edmondswoh... no it's not...16:03
edmondswefried, what do you think of that? See comments in the review esberglu linked above16:03
efriedstand by16:03
esbergluedmondsw: 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 reviewers16:04
edmondswwe're inconsistent between plug and unplug whether we get the lpar_wrap or expect it to already be provided earlier in the flow16:04
efriedRight, I remember ripping that out to reduce the footprint of an IT patch.16:04
edmondswI mean we're inconsistent OOT16:04
efriedNeed to track down why we need the LPAR wrapper.16:04
efriedI want to say it has something to do with the mgmt CNA16:04
esbergluedmondsw: Where are we inconsistent OOT?16:05
esbergluCreate provides the lpar_wrap for spawn16:05
esbergluGet for plug_vifs16:05
edmondswPlugVifs requires lpar_wrap, but UnplugVifs doesn't, it just gets it itself16:05
efriedDoes the lpar_wrap get used by more than one task?16:05
efriedin that flow?16:06
edmondswesberglu oh, nm... both have requires lpar_wrap OOT... not sure what I was looking at then16:06
efriedin either flow, actually.16:06
edmondswe.g. in spawn, the Create provides lpar_wrap and then the PlugVifs requires it16:06
esbergluefried: In spawn it gets used by PlugVifs and CreateAndConnectCfgDrive16:06
esbergluOnly required by UnplugVifs in destroy (OOT only)16:07
edmondswesberglu oh, it was inconsistent IT, not OOT16:07
efriedRight, 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
efriedI 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
efriedor something.16:08
edmondswthere's also the question of whether some of our other tasks both IT and OOT should require lpar_wrap but don't today16:08
edmondswe.g. in the destroy flow it's needed for PowerOff but that's not required there, it just gets it16:08
efriedYou mean if there are places we're grabbing the LPAR wrapper manually?16:08
efriedAgain, it's a matter of whether it's needed elsewhere in the flow.16:09
efriedI mean, we could argue that once we have the task we should use it for any flow where we ever need it.16:09
efriedjust for consistency.16:09
edmondswit'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 vifs16:09
efriedis it needed for unplug?  Then sure.16:10
edmondswchanging PowerOff to require lpar_wrap would be a bigger change, though. I don't think we bite that off in this commit16:12
edmondswI'm not really all that concerned about it16:12
esbergluedmondsw: Changing UnplugVifs to require lpar_wrap small enough to pile in with the hotplug change? Or should I do separate16:12
edmondswI was more concerned about not breaking something with spawn16:12
edmondswesberglu you could probably bundle in for unplug.16:13
edmondswwell... I guess you'd have to add tf_vm.Get for that since it's not already there... maybe do that separate16:14
edmondswI'm not so concerned about unplug, and power off... more about plug16:14
esbergluedmondsw: What do you mean?16:15
edmondswesberglu my first comment in the review16:15
edmondswright, sorry I'm not being clear, otph16:16
esbergluedmondsw: By power off you mean destroy?16:16
edmondswesberglu I think I'm happy with the change you made in PS3, but let me review it when my call is over16:18
edmondswpower off is called in destroy, which is how that came up16:18
esbergluedmondsw: Sounds good. The only other thing we could add into that is having the UnplugVifs task require lpar_wrap16:19
esbergluAnd call Get() in both the destroy and unplug_vifs flows16:19
edmondswpower 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 poweroff16:19
esbergluedmondsw: efried: CI is broken by
esbergluIt's causing all runs to pass by running 0 tests16:40
efriedesberglu: I don't know if you saw this comment the other day, but we should move our OOT projects over to stestr16:41
efriedYou know, once you pick the CI up off the floor.16:41
esbergluefried: Must have missed it, I'll add it to the list16:45
efriedI think it was while you were out.16:47
efriedbtw esberglu edmondsw if y'all haven't voted for vancouver talks:
efriedesberglu: there's some nice juicy looking CI stuff in there.16:59
*** AlexeyAbashkin has quit IRC17:09
*** k0da has quit IRC17:14
edmondswefried is yours in there somewhere?17:22
edmondswfound it...
efriedNo vote padding18:05
esbergluefried: edmondsw: This stestr stuff is nice, we can rip out all the logic for generating the test lists18:28
esbergluAnd just pass it our blacklists directly18:28
efriedIt also makes it easier to run pdb, and to run tests without all the tox gorp18:29
edmondswefried course not18:29
*** AlexeyAbashkin has joined #openstack-powervm18:36
*** AlexeyAbashkin has quit IRC18:40
efriededmondsw: I think I figgered it out.18:58
efriedI don't like what it portends, though.18:58
edmondswfigured what out?18:58
efriedPowerVMDriver.capabilities is a class property.18:59
efriedWhich is fine if it's a primitive18:59
efriedBut it's a dict.18:59
efriedSo setting18:59
efriedself.capabilities['something'] = whatever18:59
efriedactually sets the dict key on the *class* member.18:59
efriedWhich means any subsequent access of the class member will get that value.19:00
efriedThis is fine IRL because PowerVMDriver is a singleton.19:00
efriedBut in the test suite, you've got several flying around.19:00
edmondswyeah... ugh19:00
edmondswefried maybe19:06
efriedmaybe what?19:07
edmondswmaybe it's what you said, but I'm not sure19:07
edmondswto your comment on the review...19:07
efriedthe faah - yeah, I tested it out and it doesn't seem to have fixed it.19:07
edmondswI 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
efriedoh, that comment was unrelated, but may now be relevant.19:08
edmondswefried and I think I tried splitting this into two methods and was still seeing LocalStorage mocked on the one that didn't have that decorator19:08
efriedNo, 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
edmondswboo... but my second statement stands19:11
edmondswefried just confirmed... removing that decorator doesn't solve the issue19:13
edmondswstill mocked19:13
efriedThat's crazyballs.19:13
*** openstackgerrit has joined #openstack-powervm19:16
openstackgerritMatthew Edmonds proposed openstack/nova-powervm master: stop setting extra capabilities
edmondswefried with that new PS, tox -e py27 TestPowerVMDriverInit works but tox -e py27 -- nova_powervm.tests.virt.powervm.test_driver does not19:17
edmondswfails on the method without the decorator19:17
efriedyeah, that's the mystery I'm trying to nail down right now.19:18
edmondswbecause the driver has has_imagecache:    'has_imagecache': <MagicMock name='LocalStorage().capabilities.__getitem__()'19:18
efriedwhere is that freakin mock coming from?19:19
*** k0da has joined #openstack-powervm19:21
efriedStill the same problem.19:25
efriedThe 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
efriedThough I haven't fully figured out how the disk driver is getting overwritten.19:27
efriedBut I think we ought to maybe fix this at the class level, rather than trying to do it in the tests.19:27
efriedWhich is kind of suck, because again IRL the driver should be a singleton.19:27
efriededmondsw: Okay, yeah.  This delta has both ways of invoking the tests behaving the same way:19:30
efriedAnd it's not a *super* unreasonable change.19:31
efriedBTW, we just might be getting rid of that dict and making capabilities be traits on the compute RP.19:32
edmondswk, tx for figuring that out19:32
openstackgerritMatthew Edmonds proposed openstack/nova-powervm master: stop setting extra capabilities
efriedOne more test case please edmondsw19:39
edmondswthat would be pointless... replied19:40
edmondswefried ^19:41
*** k0da has quit IRC19:48
openstackgerritEric Fried proposed openstack/nova-powervm master: stop setting extra capabilities
efriededmondsw: ^ worked up those tests without mocks.20:26
efriedwell, without mocking the whole disk drivers.20:27
edmondswI don't agree20:27
efriedSo the capabilities are being set by the disk drivers natively, not by mocks.20:27
edmondswjust beat me to hitting send20:27
edmondswwill look at what you did20:27
efriedThe less mocking the better.20:28
efriedby which I mean less mocking in the path we're actually trying to test20:28
efriedShuffled stuff around so there's no setUp and no unnecessary mocking hopefully anywhere.20:28
efriedtbh, I suspect we have the same problem with disk adapters here20:29
efriedthose guys are setting capabilities on the class attr's dict.20:29
efriedthe 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
edmondswefried replied20:35
efriededmondsw: I think I replied to your reply before you replied :)20:35
edmondswefried we're talking a net increase in mocks here with your change20:36
efriedBut zero mocks in the path we're testing.  As opposed to not zero.20:37
efriedI.e. the only things we're mocking are incidentals.  Side-branches that we don't care about.20:37
edmondswI don't like it, but whatever20:38
efriedVs. mocking the whole disk driver and mimicking what (we hope) it's doing with the capabilities dict.20:38
efriedWe do a lot of the latter in our unit tests, being very granular.20:38
efriedWhich I used to think was very nice and very unit-y.20:38
efriedBut since I've been working upstream and... osmosing the wisdom of their experienced folks...20:39
efriedI've become convinced of the value of reducing mockage as much as feasible in the active path.20:39
efriededmondsw: Okay, so now what do we do for code reviews?  If we both +2 it, esberglu can +2+W?20:40
edmondswefried 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 ok20:40
edmondswas for reviews, we're kinda stuck... I guess just one +2 from esberglu20:41
efriedWith PS5, we could make the disk driver inits raise exceptions and the tests would still pass.20:41
esbergluedmondsw: efried: Have been loosely following along, will catch up and review by EOD20:42
efriedesberglu: Thanks.20:42
efriededmondsw and I will continue to circle each other with hands hovering over holsters20:42
edmondswefried why would we care here if a disk driver init raised an exception? In that case, capabilities are moot, the compute service is hosed20:43
efriedThat's kind of my point, though not exactly what I meant.20:44
edmondswif your point is that we should be doing _some_ testing that actually instantiates disk drivers, I'll agree with that20:44
edmondswtotally unrelated to capabilities20:45
efriedSo 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
efriedIt's all hypothetical, of course.20:46
*** k0da has joined #openstack-powervm20:46
efriedBut 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 reason20:48
efriedoh now you're just being spiteful :P20:49
efriedThis has passed the point of diminishing returns.  Let's put it to bed.20:50
*** k0da has quit IRC20:51
edmondswyeah, I just don't want to argue anymore20:52
edmondswesberglu please update your nova commit that adds capabilities IT to match20:54
efriedWe ended up with better code and more robust tests here.  This is a good thing.20:55
efriedstrange and winding though the road may have been.20:55
edmondswesberglu we might actually need to propose that in 2 separate commits for IT... 1) capabilities, 2) attach interface20:58
edmondswI can propose the first if you like, and then you can rebase the second on that20:58
edmondswor is it easier for you to do both?20:58
esbergluedmondsw: Either way. I probably won't get to either until tomorrow, still sorting out this stestr CI stuff20:59
edmondswthe 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 interface20:59
edmondswesberglu k, I'll throw something up20:59
esbergluedmondsw: Yep I agree 2 is the way to go20:59
edmondswesberglu 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 there21:00
esbergluedmondsw: You cool with the hotplug patch otherwise? And then a separate patch using Get() everywhere that it isn't being used currently21:00
edmondswesberglu 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 list21:01
esberglusounds good21:02
edmondswhaven't studied the tests yet, but the code itself is fine21:02
*** k0da has joined #openstack-powervm21:03
esbergluefried: edmondsw: 6356 for stestr CI21:07
esbergluStill need to run it through front to back on a fresh machine, but it appears to be working21:08
edmondswesberglu is pike_it_blacklist.txt used when testing pike, and in_tree_blacklist.txt used for everything else?21:15
esbergluedmondsw: pikt_it_blacklist gets appended to in_tree_blacklist for pike runs21:16
edmondswesberglu does that same sed expression still work? L31 in os_ci_tempest_intree.conf21:18
esbergluedmondsw: Yep21:18
edmondswesberglu and --parallel is not needed for stestr because... ?21:21
esbergluedmondsw: --concurrency is enough to know you want parallel21:22
esbergluefried: edmondsw: I'm good with 545984, gonna +2, W+121:26
efriedesberglu: Cool man, thanks.21:26
* efried blows smoke from pistol21:26
efriedSorry guys, we've been doing a western theme thing in -nova.21:26
esbergluefried: edmondsw: FYI I will be out tomorrow afternoon21:29
efriedI'll be out all day tomorrow (most of it sitting at JFK)21:29
efriedAnd PTG next week.  And vacation the week after.21:29
efriedAnd probably knocking off pretty soon here, as I got on around 4:30am today.21:29
esbergluefried: 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 again21:32
esbergluI'll be pretty confident once this manual run get through21:32
efriedSorry, looking...21:35
efriedesberglu: +2, nice one guvnor.21:38
esbergluefried: tx21:38
esbergluefried: edmondsw: One more iteration on 635621:50
esbergluMissed the stestr init21:50
efriedesberglu: Done.21:51
efriedI did notice that deletion, but assumed it was yet another thing that wasn't needed.21:51
esbergluI didn't think it was needed, but I forgot that I ran that manually when I first started goofing around21:53
openstackgerritMerged openstack/nova-powervm master: stop setting extra capabilities
openstackgerritEric Berglund proposed openstack/nova-powervm master: DNM: ci check
openstackgerritEric Berglund proposed openstack/nova-powervm master: DNM: CI Check2
*** esberglu has quit IRC22:32
*** tonyb has quit IRC22:35
*** tonyb has joined #openstack-powervm22:36
*** tjakobs has quit IRC22:46
*** k0da has quit IRC23:09
*** apearson has quit IRC23:19
*** k0da has joined #openstack-powervm23:25
*** efried has quit IRC23:32

Generated by 2.15.3 by Marius Gedminas - find it at!