Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-01-19
16:53:29 mdbooth lyarwood: I've found the root cause
16:54:37 mdbooth lyarwood: It's a difference in the implementation of hasattr between python2 and python3: http://paste.openstack.org/show/647153/
16:55:37 mdbooth What the hell do you do about that? is question 2
16:57:59 lyarwood mdbooth: cool thanks, I was sure I hit something like this before but can't seem to find anything
16:58:33 lyarwood mdbooth: https://review.openstack.org/#/c/342111/13 for example, actually introduced hasattr to get py34 passing at the time
17:00:43 mdbooth It's a bug in OVO imho
17:01:05 mdbooth It seems python expects you to raise AttributeError in a __gettattr__
17:01:39 mdbooth But obj_load_attr is raising NotImplementedError
17:02:07 lyarwood mdbooth: right and 2.7 catches any exception
17:02:09 lyarwood mdbooth: https://docs.python.org/2/library/functions.html?highlight=hasattr#hasattr
17:02:26 lyarwood 99% sure I've hit this before and there was a workaround
17:02:50 mdbooth The Nova fix would be to override obj_load_attr() and raise AttributeError instead of NotImplementedError
17:03:19 mdbooth However, we should also submit a patch to ovo
17:03:23 openstackgerrit Merged openstack/os-traits master: Add CPU features for AArch64 https://review.openstack.org/523665
17:03:32 mdbooth https://docs.python.org/3/library/functions.html#hasattr
17:06:17 mdbooth lyarwood: Hmm, it's messier than that because we also call it in other contexts. Some subclasses raise ObjectActionError
17:06:42 mdbooth lyarwood: So I think this calls for ocean-boiling-avoidance
17:07:09 mdbooth Although it is a bug in OVO
17:07:36 claudiub edmondsw: sounds ok in that case, but in other cases, it can really drive up the execution time. I've tried it now for nova.tests.unit.virt.hyperv.test_vmops, only using mock.create_autospec for a few instance attributes, and the execution time jumped from 3.8 sec / 121 tests to 7.5 sec / 121 tests. Again, not everything was autospec'd :)
17:08:02 lyarwood mdbooth: using obj_load_attr you mean?
17:08:49 mdbooth lyarwood: ovo should handle the lack of contract for obj_load_attr
17:08:49 edmondsw claudiub is that comparing to master or to the oslotests fixture?
17:09:12 mdbooth If we're going to port OpenStack to a new language, there are better options than python3
17:09:25 claudiub mdbooth: mock.create_autospec autospecs literally everything in the given object / class, which is not really necessary for every single test
17:09:47 claudiub while mock.Mock(autospec=) only autospecs something when accessed.
17:09:59 edmondsw claudiub and I'm not sure that's a big deal... you can look at as twice as long (bad) or as 4 extra seconds (insignificant). Would need to see impact on a larger scale
17:10:21 mdbooth claudiub: nice
17:11:03 edmondsw claudiub that does sound nice though
17:11:39 claudiub edmondsw: 3.929s per 121 tests
17:13:22 claudiub ok, so, 3.8 sec with no change. 3.929 with mock.Mock(autospec) (oslotest), and 7.5 sec with mock.create_autospec
17:15:03 mdbooth lyarwood: The workaround is to use getattr instead, because that has consistent behaviour
17:15:11 mdbooth i.e. it raises an exception
17:16:39 mdbooth lyarwood: Assuming you've got local changes, I'll leave ^^^ in a review comment
17:16:52 lyarwood mdbooth: yeah thanks
17:18:34 mdbooth https://bugs.launchpad.net/oslo.versionedobjects/+bug/1552927
17:18:35 openstack Launchpad bug 1552927 in oslo.versionedobjects "hasattr() function behavior differences between Py27 and Py34" [Undecided,New]
17:18:43 mdbooth lyarwood: Already been reported ^^^^
17:19:34 mdbooth obj.attr_is_set()...
17:21:16 lyarwood mdbooth: ah!
17:21:42 mdbooth Just testing that
17:22:34 mdbooth lyarwood: Works, at least for that test
17:22:48 lyarwood mdbooth: yeah works for me
17:23:48 openstackgerrit Lee Yarwood proposed openstack/nova master: libvirt: QEMU native LUKS decryption for encrypted volumes https://review.openstack.org/523958
17:35:28 claudiub mdbooth: gonna send you a link soon. already 60 seconds passed while trying to execute unittests which shouldn't last more than 10 seconds. :)
17:42:40 claudiub mdbooth: https://github.com/bclau/nova/tree/autospec-issue
17:43:22 mdbooth lyarwood: I'd have left you a +1 if Jenkins had voted already. I'd appreciate if you consider the readability of the new test, though.
17:43:53 mdbooth My eyeballs aren't good at computing diffs.
17:43:54 claudiub execution time: vanilla: ~10 seconds. oslotest mock.MagicMock(autospec): ~14.5 sec. mock.create_autospec: I dunno, a few hundred seconds, still running.
17:44:54 mdbooth claudiub: That's great info, and definitely something I hadn't appreciated.
17:45:11 mdbooth I think I've used create_autospec in a few places in the past.
17:45:53 claudiub it's definetely great, but the fact that it autospecs literally everything can be problematic.
17:46:19 claudiub and I have sent pull requests to python 3 and the mock library, but no response for at least half a year
17:46:35 claudiub and there hasn't been any updates to the mock library since more than 12-13 months
17:47:03 claudiub Ran 364 tests in 434.626s
17:53:05 openstackgerrit David Rabel proposed openstack/nova stable/pike: Fix format in live-migration-usage.rst https://review.openstack.org/535871
18:00:27 mdbooth Incoming
18:01:03 openstackgerrit Matthew Booth proposed openstack/nova master: Remove redundant swap_volume tests https://review.openstack.org/531179
18:01:04 openstackgerrit Matthew Booth proposed openstack/nova master: Add DriverLocalImageBlockDevice https://review.openstack.org/526347
18:01:04 openstackgerrit Matthew Booth proposed openstack/nova master: Give volume DriverBlockDevice classes a common prefix https://review.openstack.org/526346
18:01:05 openstackgerrit Matthew Booth proposed openstack/nova master: Add local_root to block_device_info https://review.openstack.org/529029
18:01:05 openstackgerrit Matthew Booth proposed openstack/nova master: Expose driver_block_device fields as attributes https://review.openstack.org/528362
18:01:06 openstackgerrit Matthew Booth proposed openstack/nova master: Use real block_device_info data in libvirt tests https://review.openstack.org/527916
18:01:06 openstackgerrit Matthew Booth proposed openstack/nova master: Pass DriverBlockDevice to driver.attach_volume https://review.openstack.org/528363
18:01:07 openstackgerrit Matthew Booth proposed openstack/nova master: Pass disk_info dict to libvirt_info https://review.openstack.org/529329
18:01:07 openstackgerrit Matthew Booth proposed openstack/nova master: Fix libvirt volume tests passing invalid disk_info https://review.openstack.org/529328
18:01:08 openstackgerrit Matthew Booth proposed openstack/nova master: Expose volume host type and path independent of libvirt config https://review.openstack.org/530786
18:01:09 openstackgerrit Matthew Booth proposed openstack/nova master: Local disk serial numbers for the libvirt driver https://review.openstack.org/529380
18:01:09 openstackgerrit Matthew Booth proposed openstack/nova master: Don't generate fake disk_info in swap_volume https://review.openstack.org/530787
18:15:36 lyarwood mdbooth: ack, I might rewrite both tests using the conf objects and format_dom calls
18:38:02 openstackgerrit Jim Rollenhagen proposed openstack/nova master: Ironic: negotiate microversion to allow downgrade to Pike https://review.openstack.org/535786
19:13:34 cfriesen found an interesting glance/nova interworking issue. if doing a snapshot, and cinder/glance are both on ceph, nova tries to shortcut the snapshot then set the location in glance. But by default glance won't let you set the location of an image, so this fails.
19:16:09 cfriesen correction, not cinder, but when instance backend is ceph.
19:57:39 cburgess cfriesen Sounds like nova is doing the right thing but glance just isn't configured properly. Sounds like maybe a release note or something is needed?
19:57:54 cburgess cfriesen I suspect cinder will run into the same issue.
20:22:17 fried_rolls you gotta be effin kidding me. The bottom three patches, all +W'd, got zuul -1 with (three *different*) spurious failures. The fourth patch - the first one with a -1 - passed.
20:24:05 openstackgerrit OpenStack Proposal Bot proposed openstack/nova master: Updated from global requirements https://review.openstack.org/535919
20:25:32 ildikov fried_rice: mine got a time_out error on the gate, sooo close, but no...
20:26:46 fried_rice Boo. I feel your pain.
20:43:30 fried_rice figleaf regarding https://review.openstack.org/#/c/526540/17/nova/scheduler/client/report.py@390 -- the only possible reason I can see for it having been split out before was LOG.warning vs .error.
20:43:59 fried_rice figleaf But given that the error text is (at least) as descriptive as what was there before, are you cool with it?
20:55:35 openstackgerrit Ihar Hrachyshka proposed openstack/nova master: DNM testing whether lib/neutron switch breaks this repo https://review.openstack.org/535947
21:02:27 cfriesen cburgess: I've been diving down into the rathole of the nova/glance interaction. turns out glance's policy stuff is all mucked up, so that enabling nova's snapshot in this case requires glance to be configured insecurely. It might be possible to use service tokens to fix it, but nova doesn't send them by default.
21:02:39 cfriesen speaking of which...anyone know why send_service_user_token is False by default?
21:03:19 cfriesen cburgess: the glance folks suggested a workaround of a separate glance node that is configured less secure, which is only accessible by other openstack components. kind of icky if you ask me.
21:08:02 figleaf fried_rice: It just seemed like a bigger behavior change than it set out to be
21:08:13 figleaf fried_rice: it's not a showstopper by any stretch
21:08:20 fried_rice coolcool.
21:09:58 leakypipes fried_rice: k, your patches are now reviewed up to the WIPs.
21:10:11 fried_rice leakypipes Thanks dude. Starting to work through 'em.
21:10:17 leakypipes yup, np
21:11:52 openstackgerrit Matthew Edmonds proposed openstack/nova master: Fix bad autospec usage https://review.openstack.org/535806
21:15:21 edmondsw claudiub ^ fixed a pep8 error. Interested in how you feel about that patch
21:16:20 edmondsw I'm not against using the fixture you got in oslotests if the performance is better, but this might be quicker to merge and doesn't preclude your changes coming later
21:16:44 edmondsw doesn't have a significant performance impact at this small scale that I've detected
21:20:26 edmondsw cfriesen send_service_user_token is False by default simply because you have to configure credentials for that to work
21:21:06 edmondsw I'd like for send_service_user_token to go away, and just have the code detect whether it has credentials or not and use that as the determining factor, but that's a nit
21:22:37 openstackgerrit Hongbin Lu proposed openstack/nova master: Handle RPC timeout on get_console_output https://review.openstack.org/525335
21:28:03 cburgess cfriesen Thats very icky, especially considering that its worked this way for a while (the nova side I mean).
21:28:20 cburgess nova merged this stuff shortly after Tokyo
21:37:56 cfriesen cburgess: I think it might be related to the glance v1/v2 stuff

Earlier   Later