| *** AlexeyAbashkin has joined #openstack-powervm | 07:56 | |
| *** k0da has joined #openstack-powervm | 08:39 | |
| *** AlexeyAbashkin has quit IRC | 11:07 | |
| *** AlexeyAbashkin has joined #openstack-powervm | 11:32 | |
| *** edmondsw has joined #openstack-powervm | 13:12 | |
| *** svenkat has joined #openstack-powervm | 13:22 | |
| *** apearson has joined #openstack-powervm | 13:57 | |
| *** esberglu has joined #openstack-powervm | 14:20 | |
| *** tjakobs_ has joined #openstack-powervm | 14: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 IRC | 16:26 | |
| *** AlexeyAbashkin has quit IRC | 16:50 | |
| esberglu | edmondsw: Figured we can discuss your CI q's in the driver meeting tomorrow | 17:39 |
|---|---|---|
| esberglu | But I can respond to the thread if you prefer | 17:39 |
| edmondsw | esberglu I am going to be out Tues and Wed. I may be able to pop online for the mtg but not sure yet | 17:40 |
| edmondsw | if you're free now, want to talk through it? | 17:40 |
| esberglu | edmondsw: Sure | 17:41 |
| esberglu | We don't test live migration in CI | 17:41 |
| edmondsw | yep because we only currently use one node per CI run | 17:42 |
| esberglu | I haven't really looked into multinode testing before | 17:42 |
| edmondsw | please put that on your TODO list | 17:42 |
| esberglu | Sure | 17:42 |
| edmondsw | sounds 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 that | 17:43 |
| edmondsw | do you think we have the number of machines we'll need to make that happen | 17:43 |
| edmondsw | ? | 17:43 |
| edmondsw | I'm guessing we'd still have one aio and add one plain compute | 17:44 |
| esberglu | edmondsw: We might need a couple more. vCPUs will be the limiting factor | 17:44 |
| esberglu | I think that we have 180 right now and give 2 to each host | 17:44 |
| edmondsw | esberglu the hosts themselves are virtual? | 17:45 |
| esberglu | Not sure what you mean. Each undercloud neo has either 10 or 20 vcpus, we allocate 2 of them to each AIO tempest instance | 17:48 |
| edmondsw | esberglu 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 |
| esberglu | Each physical host has 1 novalink | 17:52 |
| edmondsw | esberglu ok, that's what I thought... so aren't we going to be limited by the number of physical hosts? | 17:53 |
| edmondsw | oh... we use remote pypowervm access, so we don't have to install on NovaLink | 18:01 |
| esberglu | edmondsw: Just 1 quick clarification. There is the full set of tempest tests. | 18:10 |
| esberglu | Then 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.html | 18:11 |
| esberglu | Then 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 |
| esberglu | I think I said that in the other order on the call | 18:12 |
| edmondsw | k | 18:14 |
| edmondsw | esberglu I just noticed that console.txt.gz isn't actually a gz file, it's html | 18:15 |
| esberglu | edmondsw: Also click all on the powervm_os_ci.html page to expand everything | 18:15 |
| *** AlexeyAbashkin has joined #openstack-powervm | 18:15 | |
| edmondsw | esberglu can we fix the name to be html instead of gz then? | 18:16 |
| edmondsw | and is that only for this file, or same problem for others? | 18:16 |
| esberglu | edmondsw: The logserver takes the .gz file and generates the html page | 18:18 |
| esberglu | It is a txt.gz file on the logserver | 18:18 |
| edmondsw | oh, but it decompresses the file when a browser requests it? | 18:19 |
| esberglu | edmondsw: yeah | 18:20 |
| *** AlexeyAbashkin has quit IRC | 18:20 | |
| edmondsw | k | 18:20 |
| *** edmondsw has quit IRC | 18:55 | |
| *** edmondsw has joined #openstack-powervm | 18:56 | |
| *** edmondsw has quit IRC | 19:00 | |
| *** apearson has quit IRC | 19:01 | |
| *** apearson has joined #openstack-powervm | 19:05 | |
| *** esberglu has quit IRC | 19:11 | |
| *** esberglu has joined #openstack-powervm | 19:29 | |
| *** k0da has joined #openstack-powervm | 19:34 | |
| *** apearson has quit IRC | 19:56 | |
| *** apearson has joined #openstack-powervm | 20:00 | |
| *** k0da has quit IRC | 20:03 | |
| *** edmondsw has joined #openstack-powervm | 20:30 | |
| edmondsw | esberglu 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 |
| edmondsw | looks like it enables hotplug | 20:54 |
| esberglu | We didn't add hotplug | 21:01 |
| *** openstackgerrit has joined #openstack-powervm | 21:24 | |
| openstackgerrit | Matthew Edmonds proposed openstack/nova-powervm master: stop setting extra capabilities https://review.openstack.org/545984 | 21:24 |
| *** AlexeyAbashkin has joined #openstack-powervm | 21:26 | |
| edmondsw | esberglu is there a reason we didn't want to support hotplug? We appear to support it OOT | 21:27 |
| edmondsw | is there any more to it other than setting that capability? | 21:27 |
| esberglu | edmondsw: I think we just have to add the attach_interface and detach_interface driver methods | 21:29 |
| *** k0da has joined #openstack-powervm | 21:29 | |
| *** AlexeyAbashkin has quit IRC | 21:31 | |
| edmondsw | esberglu oh, I thought we already had. ok then | 21:31 |
| edmondsw | esberglu efried can one of you see what I'm doing wrong with the syntax in the commit link above? | 21:31 |
| edmondsw | pep8 says L180 E901 SyntaxError: invalid syntax | 21:32 |
| efried | edmondsw: What, you mean the fact that you can't put the RHS of an assignment on a new line? | 21:32 |
| edmondsw | how did I not know that? | 21:33 |
| edmondsw | but I didn't | 21:33 |
| edmondsw | guess I've never tried | 21:33 |
| edmondsw | annoying | 21:33 |
| efried | edmondsw: 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 |
| edmondsw | or \ | 21:34 |
| edmondsw | I assume that will work | 21:34 |
| efried | edmondsw: But will get a -1 | 21:34 |
| edmondsw | tx | 21:34 |
| efried | edmondsw: But I don't see the point of this change | 21:34 |
| efried | oh, sorry, it wasn't what I thought... | 21:35 |
| efried | edmondsw: Why do we care? | 21:36 |
| edmondsw | readability | 21:37 |
| *** k0da has quit IRC | 21:37 | |
| edmondsw | and I don't want nova dinging us on reporting capabilities that they don't want us reporting... | 21:38 |
| edmondsw | when we port this stuff IT | 21:38 |
| openstackgerrit | Matthew Edmonds proposed openstack/nova-powervm master: stop setting extra capabilities https://review.openstack.org/545984 | 21:42 |
| edmondsw | efried fixed, and included the test change I forgot | 21:43 |
| *** k0da has joined #openstack-powervm | 21:46 | |
| edmondsw | esberglu 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 stuff | 21:46 |
| esberglu | Yep | 21:47 |
| efried | edmondsw: How was that test passing before? | 21:50 |
| efried | edmondsw: It actually should have been complaining that 'mock has no getattr' | 21:51 |
| efried | ...and it still should be. | 21:51 |
| *** svenkat has quit IRC | 21:52 | |
| edmondsw | efried I'm not seeing what you're seeing there | 22:15 |
| efried | I don't understand why test_driver.capabilities['has_imagecache'] would have been False before but True with your change. | 22:16 |
| edmondsw | oh, I meant the getattr thing | 22:16 |
| efried | because 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 |
| edmondsw | should add tests that the other capabilities aren't included? | 22:17 |
| efried | Oh, 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 |
| edmondsw | doesn't seem worth the effort | 22:17 |
| efried | edmondsw: ffs, the whole patch doesn't seem worth the effort. | 22:17 |
| edmondsw | just trying to do a little cleanup as I go through the code | 22:17 |
| efried | As for the other thing... | 22:18 |
| edmondsw | looking at capabilities especially, trying to see what we need to port IT | 22:18 |
| efried | http://paste.openstack.org/show/677884/ | 22:18 |
| *** openstackgerrit has quit IRC | 22:18 | |
| efried | So 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 |
| efried | But 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 |
| efried | Do you see my confusion? What am I missing? | 22:21 |
| edmondsw | efried it's not a mock | 22:22 |
| edmondsw | test_driver = driver.PowerVMDriver(fake.FakeVirtAPI()) | 22:22 |
| edmondsw | that's a real driver instantiation | 22:22 |
| edmondsw | just with a fake API passed in, but the driver itself is real | 22:22 |
| efried | The test case setUp installs the PowerVMComputeDriver fixture. | 22:22 |
| efried | Hum. Which means the driver setup in that test case isn't doing what we thought it should be doing. | 22:24 |
| edmondsw | this 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 |
| efried | Which brings me back to the second question, then. | 22:26 |
| efried | How is it that has_imagecache was False before, OR how is it that it's True now? | 22:27 |
| edmondsw | it was set to False in the constructor and the update must not have actually been doing anything | 22:27 |
| edmondsw | presumably because the disk driver is using a fixture or mock | 22:28 |
| edmondsw | or isn't set at all? | 22:28 |
| edmondsw | no, must be setup, but I | 22:29 |
| edmondsw | m not sure how yet | 22:29 |
| edmondsw | ah, yep... the setup calling useFixture(fx.PowerVMComputeDriver()) does it because that in turn sets up fixture for the disk driver | 22:33 |
| edmondsw | https://github.com/openstack/nova-powervm/blob/master/nova_powervm/tests/virt/powervm/fixtures.py#L134 | 22:34 |
| edmondsw | which is just a mock with no capabilities, so the update had no effect | 22:35 |
| edmondsw | efried ^ | 22:36 |
| efried | Sorry, got a call. | 22:36 |
| edmondsw | np... as you said, this isn't exactly urgent :) | 22:36 |
| efried | So no | 22:36 |
| efried | if it was going through the fixture before, it would be going through the fixture now. | 22:36 |
| efried | You didn't change that. | 22:36 |
| edmondsw | hm, true | 22:37 |
| efried | And 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 |
| efried | Neither one should have allowed the test to pass. | 22:37 |
| efried | So. | 22:37 |
| * efried confused. | 22:37 | |
| edmondsw | efried your paste didn't actually test a_mock.an_attr['key'] | 22:39 |
| edmondsw | it tested a_mock['key'] | 22:39 |
| edmondsw | not sure if that would make a difference though | 22:39 |
| efried | In [8]: m.capabilities['foo'] | 22:40 |
| efried | --------------------------------------------------------------------------- | 22:40 |
| efried | TypeError Traceback (most recent call last) | 22:40 |
| efried | <ipython-input-8-46c6110d1ec0> in <module>() | 22:40 |
| efried | ----> 1 m.capabilities['foo'] | 22:40 |
| efried | TypeError: 'Mock' object has no attribute '__getitem__' | 22:40 |
| edmondsw | so no difference | 22:40 |
| edmondsw | didn't really think it would, but... | 22:40 |
| edmondsw | hmm | 22:40 |
| efried | Any time you access an attr of a mock, you get a mock (unless autospecced, which I don't think this is) | 22:40 |
| efried | but 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 |
| edmondsw | so maybe that disk driver fixture isn't used here... maybe it's a real local disk driver | 22:42 |
| edmondsw | i.e. only https://github.com/openstack/nova-powervm/blob/master/nova_powervm/tests/virt/powervm/test_driver.py#L93 is relevant | 22:42 |
| edmondsw | but then why did it pass before | 22:42 |
| efried | exaciticaly | 22:44 |
| efried | Are we both waiting each other out to see who will actually pdb the sucker? | 22:44 |
| edmondsw | this is what I get when AssertFalse: | 22:45 |
| edmondsw | AssertionError: <MagicMock name='LocalStorage().capabilities.__getitem__()' id='140003996242064'> is not false | 22:46 |
| edmondsw | efried honestly I don't know how to pdb UTs | 22:47 |
| efried | okay. So it *is* a mock. | 22:47 |
| efried | oh, I can help you with that. | 22:47 |
| edmondsw | but is that saying the capabilities are the mock? | 22:47 |
| *** tjakobs_ has quit IRC | 22:48 | |
| efried | We should really move to stestr. esberglu ^ | 22:49 |
| efried | or rather < | 22:49 |
| efried | edmondsw: Here, grab this script and make it executable: http://paste.openstack.org/show/677904/ | 22:50 |
| efried | Now 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 |
| efried | edmondsw: 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 |
| edmondsw | definitely weren't before either | 22:53 |
| edmondsw | I'll dig into it | 22:53 |
| efried | You'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-powervm | 22:53 | |
| openstackgerrit | Eric Berglund proposed openstack/nova-powervm master: DNM: ci check https://review.openstack.org/328315 | 22:53 |
| openstackgerrit | Eric Berglund proposed openstack/nova-powervm master: DNM: CI Check2 https://review.openstack.org/328317 | 22:53 |
| efried | Prove that by changing the capability to False in the LocalStorage driver, and your test will still pass. | 22:53 |
| edmondsw | yeah, I expect it would | 22:54 |
| efried | It's still a mystery why real_dict.update(mock().capabilities) didn't raise an exception before. | 22:54 |
| efried | Because .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 |
| efried | It would be crazy if the assertFalse we're using is somehow converting TypeError. | 22:56 |
| edmondsw | verified that chaning the cap to False in LocalStorage still passed | 22:57 |
| edmondsw | efried could it be the difference between Mock and MagicMock? | 23:02 |
| edmondsw | because this is a MagicMock | 23:02 |
| edmondsw | I think that's it | 23:02 |
| efried | wow | 23:03 |
| efried | apparently so. | 23:03 |
| efried | Apparently MagicMock is iterable. | 23:03 |
| efried | I would have lost money on that bet. | 23:04 |
| efried | Default implementations of most magic methods. | 23:04 |
| edmondsw | yep | 23:05 |
| efried | Like __iter__, I suppose. | 23:05 |
| efried | Well I'll be a monkey's uncle. | 23:05 |
| edmondsw | so now to figure out what to do about it | 23:05 |
| efried | But I'm glad we figured this out, cause we definitely found out this test is testing nothing. | 23:05 |
| edmondsw | yep | 23:05 |
| edmondsw | that line, anyway | 23:05 |
| edmondsw | the other lines are testing fine | 23:05 |
| edmondsw | because the virt driver itself isn't a mock, so those have real values | 23:06 |
| *** AlexeyAbashkin has joined #openstack-powervm | 23:06 | |
| efried | Move the test into TestPowerVMDriverInit | 23:07 |
| efried | we're not mocking the disk adapter in that one. | 23:07 |
| edmondsw | cool beans | 23:08 |
| efried | While 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 |
| efried | The former should pass before & after your change. The latter should fail before and pass after. | 23:09 |
| *** AlexeyAbashkin has quit IRC | 23:10 | |
| *** k0da has quit IRC | 23:17 | |
| *** apearson has quit IRC | 23:53 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!