Monday, 2018-02-19

*** AlexeyAbashkin has joined #openstack-powervm07:56
*** k0da has joined #openstack-powervm08:39
*** AlexeyAbashkin has quit IRC11:07
*** AlexeyAbashkin has joined #openstack-powervm11:32
*** edmondsw has joined #openstack-powervm13:12
*** svenkat has joined #openstack-powervm13:22
*** apearson has joined #openstack-powervm13:57
*** esberglu has joined #openstack-powervm14:20
*** tjakobs_ has joined #openstack-powervm14:31
-openstackstatus- NOTICE: Zuul has been restarted to pick up latest memory fixes. Queues were saved however patches uploaded after 14:40UTC may have been missed. Please recheck if needed.15:15
*** k0da has quit IRC16:26
*** AlexeyAbashkin has quit IRC16:50
esbergluedmondsw: Figured we can discuss your CI q's in the driver meeting tomorrow17:39
esbergluBut I can respond to the thread if you prefer17:39
edmondswesberglu I am going to be out Tues and Wed. I may be able to pop online for the mtg but not sure yet17:40
edmondswif you're free now, want to talk through it?17:40
esbergluedmondsw: Sure17:41
esbergluWe don't test live migration in CI17:41
edmondswyep because we only currently use one node per CI run17:42
esbergluI haven't really looked into multinode testing before17:42
edmondswplease put that on your TODO list17:42
edmondswsounds like they will push for that to be covered in CI if we're going to port that in-tree, and I kinda agree... we should be testing that17:43
edmondswdo you think we have the number of machines we'll need to make that happen17:43
edmondswI'm guessing we'd still have one aio and add one plain compute17:44
esbergluedmondsw: We might need a couple more. vCPUs will be the limiting factor17:44
esbergluI think that we have 180 right now and give 2 to each host17:44
edmondswesberglu the hosts themselves are virtual?17:45
esbergluNot sure what you mean. Each undercloud neo has either 10 or 20 vcpus, we allocate 2 of them to each AIO tempest instance17:48
edmondswesberglu AIO has to be installed on a NovaLink, right? Do we somehow put multiple NovaLinks on a single physical host for the CI testing?17:49
esbergluEach physical host has 1 novalink17:52
edmondswesberglu ok, that's what I thought... so aren't we going to be limited by the number of physical hosts?17:53
edmondswoh... we use remote pypowervm access, so we don't have to install on NovaLink18:01
esbergluedmondsw: Just 1 quick clarification. There is the full set of tempest tests.18:10
esbergluThen we filter out our blacklist results (which you can see in the console.txt). These are the tests that are skipped and aren't included on powervm_os_ci.html18:11
esbergluThen tempest runs and further filters out tests to be skipped based on the conf (these are the ones you will see skipped in powervm_os_ci.html)18:11
esbergluI think I said that in the other order on the call18:12
edmondswesberglu I just noticed that console.txt.gz isn't actually a gz file, it's html18:15
esbergluedmondsw: Also click all on the powervm_os_ci.html page to expand everything18:15
*** AlexeyAbashkin has joined #openstack-powervm18:15
edmondswesberglu can we fix the name to be html instead of gz then?18:16
edmondswand is that only for this file, or same problem for others?18:16
esbergluedmondsw: The logserver takes the .gz file and generates the html page18:18
esbergluIt is a txt.gz file on the logserver18:18
edmondswoh, but it decompresses the file when a browser requests it?18:19
esbergluedmondsw: yeah18:20
*** AlexeyAbashkin has quit IRC18:20
*** edmondsw has quit IRC18:55
*** edmondsw has joined #openstack-powervm18:56
*** edmondsw has quit IRC19:00
*** apearson has quit IRC19:01
*** apearson has joined #openstack-powervm19:05
*** esberglu has quit IRC19:11
*** esberglu has joined #openstack-powervm19:29
*** k0da has joined #openstack-powervm19:34
*** apearson has quit IRC19:56
*** apearson has joined #openstack-powervm20:00
*** k0da has quit IRC20:03
*** edmondsw has joined #openstack-powervm20:30
edmondswesberglu was there a reason we didn't add the supports_attach_interface capability to our driver impl when we added OVS/SEA support, or is that just a miss?20:53
edmondswlooks like it enables hotplug20:54
esbergluWe didn't add hotplug21:01
*** openstackgerrit has joined #openstack-powervm21:24
openstackgerritMatthew Edmonds proposed openstack/nova-powervm master: stop setting extra capabilities
*** AlexeyAbashkin has joined #openstack-powervm21:26
edmondswesberglu is there a reason we didn't want to support hotplug? We appear to support it OOT21:27
edmondswis there any more to it other than setting that capability?21:27
esbergluedmondsw: I think we just have to add the attach_interface and detach_interface driver methods21:29
*** k0da has joined #openstack-powervm21:29
*** AlexeyAbashkin has quit IRC21:31
edmondswesberglu oh, I thought we already had. ok then21:31
edmondswesberglu efried can one of you see what I'm doing wrong with the syntax in the commit link above?21:31
edmondswpep8 says L180 E901 SyntaxError: invalid syntax21:32
efriededmondsw: What, you mean the fact that you can't put the RHS of an assignment on a new line?21:32
edmondswhow did I not know that?21:33
edmondswbut I didn't21:33
edmondswguess I've never tried21:33
efriededmondsw: Agree.  If it's too long and you can't hack it, you have to put it in parens.  Which ain't prettier.21:33
edmondswor \21:34
edmondswI assume that will work21:34
efriededmondsw: But will get a -121:34
efriededmondsw: But I don't see the point of this change21:34
efriedoh, sorry, it wasn't what I thought...21:35
efriededmondsw: Why do we care?21:36
*** k0da has quit IRC21:37
edmondswand I don't want nova dinging us on reporting capabilities that they don't want us reporting...21:38
edmondswwhen we port this stuff IT21:38
openstackgerritMatthew Edmonds proposed openstack/nova-powervm master: stop setting extra capabilities
edmondswefried fixed, and included the test change I forgot21:43
*** k0da has joined #openstack-powervm21:46
edmondswesberglu I think I'll add attach/detach interface to the rocky bp unless you have any objections. I thought that was already done, and makes sense to go ahead and finish out the network stuff21:46
efriededmondsw: How was that test passing before?21:50
efriededmondsw: It actually should have been complaining that 'mock has no getattr'21:51
efried...and it still should be.21:51
*** svenkat has quit IRC21:52
edmondswefried I'm not seeing what you're seeing there22:15
efriedI don't understand why test_driver.capabilities['has_imagecache'] would have been False before but True with your change.22:16
edmondswoh, I meant the getattr thing22:16
efriedbecause your change shouldn't have changed anything for the has_imagecache capability.  It should have just removed the other capabilities (which you should add tests for, btw)22:16
edmondswshould add tests that the other capabilities aren't included?22:17
efriedOh, so I tried to track down how that could possibly be the case, and from what I can tell, both the .update() and the dict access should be operating on a Mock().22:17
edmondswdoesn't seem worth the effort22:17
efriededmondsw: ffs, the whole patch doesn't seem worth the effort.22:17
edmondswjust trying to do a little cleanup as I go through the code22:17
efriedAs for the other thing...22:18
edmondswlooking at capabilities especially, trying to see what we need to port IT22:18
*** openstackgerrit has quit IRC22:18
efriedSo if the disk driver is a mock, we should have been getting one of those errors before your change, the other one after.22:21
efriedBut if the disk driver isn't a mock - if it's really a LocalStorage - then the test should have been failing before, because LocalStorage.capabilities has has_imagecache = True.22:21
efriedDo you see my confusion?  What am I missing?22:21
edmondswefried it's not a mock22:22
edmondswtest_driver = driver.PowerVMDriver(fake.FakeVirtAPI())22:22
edmondswthat's a real driver instantiation22:22
edmondswjust with a fake API passed in, but the driver itself is real22:22
efriedThe test case setUp installs the PowerVMComputeDriver fixture.22:22
efriedHum.  Which means the driver setup in that test case isn't doing what we thought it should be doing.22:24
edmondswthis test isn't using the fixture, though, is it?22:25
efried...Sometimes a fixture mocks the constructor - but in this case, no, you're right, it's not doing that, so the constructor call in this test case ought to be creating a real instance.22:26
efriedWhich brings me back to the second question, then.22:26
efriedHow is it that has_imagecache was False before, OR how is it that it's True now?22:27
edmondswit was set to False in the constructor and the update must not have actually been doing anything22:27
edmondswpresumably because the disk driver is using a fixture or mock22:28
edmondswor isn't set at all?22:28
edmondswno, must be setup, but I22:29
edmondswm not sure how yet22:29
edmondswah, yep... the setup calling useFixture(fx.PowerVMComputeDriver()) does it because that in turn sets up fixture for the disk driver22:33
edmondswwhich is just a mock with no capabilities, so the update had no effect22:35
edmondswefried ^22:36
efriedSorry, got a call.22:36
edmondswnp... as you said, this isn't exactly urgent :)22:36
efriedSo no22:36
efriedif it was going through the fixture before, it would be going through the fixture now.22:36
efriedYou didn't change that.22:36
edmondswhm, true22:37
efriedAnd as my paste demonstrates above, if you call real_dict.update(a_mock.an_attr), you get one error; a_mock.an_attr['key'] you get a different error.22:37
efriedNeither one should have allowed the test to pass.22:37
* efried confused.22:37
edmondswefried your paste didn't actually test a_mock.an_attr['key']22:39
edmondswit tested a_mock['key']22:39
edmondswnot sure if that would make a difference though22:39
efriedIn [8]: m.capabilities['foo']22:40
efriedTypeError                                 Traceback (most recent call last)22:40
efried<ipython-input-8-46c6110d1ec0> in <module>()22:40
efried----> 1 m.capabilities['foo']22:40
efriedTypeError: 'Mock' object has no attribute '__getitem__'22:40
edmondswso no difference22:40
edmondswdidn't really think it would, but...22:40
efriedAny time you access an attr of a mock, you get a mock (unless autospecced, which I don't think this is)22:40
efriedbut even if autospecced, the boggle would be the same.  It's either a mock or it's not.  And .update should have been doing the same thing as the explicit assignment.22:41
edmondswso maybe that disk driver fixture isn't used here... maybe it's a real local disk driver22:42
edmondswi.e. only is relevant22:42
edmondswbut then why did it pass before22:42
efriedAre we both waiting each other out to see who will actually pdb the sucker?22:44
edmondswthis is what I get when AssertFalse:22:45
edmondswAssertionError: <MagicMock name='LocalStorage().capabilities.__getitem__()' id='140003996242064'> is not false22:46
edmondswefried honestly I don't know how to pdb UTs22:47
efriedokay.  So it *is* a mock.22:47
efriedoh, I can help you with that.22:47
edmondswbut is that saying the capabilities are the mock?22:47
*** tjakobs_ has quit IRC22:48
efriedWe should really move to stestr.  esberglu ^22:49
efriedor rather <22:49
efriededmondsw: Here, grab this script and make it executable:
efriedNow you should be able to run pdbtox with the same args as you would normally run tox, but it'll actually heed ``import pdb; pdb.set_trace()`` if you stuff it in the code.22:51
efriededmondsw: In any case, the output you show above is proving that you're totally *not* testing what you thought you were testing.22:52
efried(and we probably weren't before, either)22:53
edmondswdefinitely weren't before either22:53
edmondswI'll dig into it22:53
efriedYou're now proving that mock().mock.mock() returns a thing that evaluates to True.  Which, since that's a Mock, is the case.22:53
*** openstackgerrit has joined #openstack-powervm22:53
openstackgerritEric Berglund proposed openstack/nova-powervm master: DNM: ci check
openstackgerritEric Berglund proposed openstack/nova-powervm master: DNM: CI Check2
efriedProve that by changing the capability to False in the LocalStorage driver, and your test will still pass.22:53
edmondswyeah, I expect it would22:54
efriedIt's still a mystery why real_dict.update(mock().capabilities) didn't raise an exception before.22:54
efriedBecause .update iterates over the result of its first arg's .keys() - which in this case will also be a mock, which is not iterable.22:55
efriedIt would be crazy if the assertFalse we're using is somehow converting TypeError.22:56
edmondswverified that chaning the cap to False in LocalStorage still passed22:57
edmondswefried could it be the difference between Mock and MagicMock?23:02
edmondswbecause this is a MagicMock23:02
edmondswI think that's it23:02
efriedapparently so.23:03
efriedApparently MagicMock is iterable.23:03
efriedI would have lost money on that bet.23:04
efriedDefault implementations of most magic methods.23:04
efriedLike __iter__, I suppose.23:05
efriedWell I'll be a monkey's uncle.23:05
edmondswso now to figure out what to do about it23:05
efriedBut I'm glad we figured this out, cause we definitely found out this test is testing nothing.23:05
edmondswthat line, anyway23:05
edmondswthe other lines are testing fine23:05
edmondswbecause the virt driver itself isn't a mock, so those have real values23:06
*** AlexeyAbashkin has joined #openstack-powervm23:06
efriedMove the test into TestPowerVMDriverInit23:07
efriedwe're not mocking the disk adapter in that one.23:07
edmondswcool beans23:08
efriedWhile you're at it, add a test case that uses a different disk driver and asserts has_imagecache is False; and also add assertions that the other keys (the ones you're avoiding by not doing .update) are absent from the virt driver's capabilities.23:08
efriedThe former should pass before & after your change.  The latter should fail before and pass after.23:09
*** AlexeyAbashkin has quit IRC23:10
*** k0da has quit IRC23:17
*** apearson has quit IRC23:53

Generated by 2.15.3 by Marius Gedminas - find it at!