Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-01-19
16:16:04 bauzas ildikov: yup, no rush
16:16:12 bauzas ildikov: that can totally be a follow-up
16:16:23 ildikov bauzas: cool :)
16:16:26 bauzas but I'd be happy if we could keep sort of the logic in the code
16:16:41 bauzas matt did a good job doing that in the conductor
16:16:58 ildikov yeah, it always starts simple and then we give it more and more thoughts...
16:17:19 bauzas ildikov: for example https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L896-L903
16:17:26 ildikov the tricks are all on him here too and I mean it the best way possible
16:17:45 bauzas and https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L913-L921
16:18:00 bauzas ildikov: sure, I don't deny it
16:18:02 ildikov nice, good examples
16:18:13 bauzas ildikov: I'm just saying, we somehow need to remember what we did
16:18:28 ildikov bauzas: I completely agree
16:18:44 bauzas anyway, I'm calling it a day
16:19:02 bauzas the API change is hairy and not my expertise, so it'll require a bit more thoughts
16:19:11 bauzas not for a friday thing
16:19:15 bauzas ++ folks
16:19:34 ildikov bauzas: makes sense, thanks for looking into the series
16:20:24 ildikov bauzas: have a great weekend!
16:21:33 lyarwood mdbooth: http://logs.openstack.org/58/523958/16/check/openstack-tox-py35/db3ef07/testr_results.html.gz - if you have time, any thoughts on these failures? hasattr is failing on o.vo objects on py35, works on py27.
16:22:41 mdbooth lyarwood: Yeah, will look now.
16:27:04 mdbooth lyarwood: I'm hacking on it right now. Ping me if you fix it before I do.
16:27:15 lyarwood mdbooth: the race is on
16:27:21 mdbooth Hehe
16:27:30 mdbooth If you've got other stuff to do, feel free :)
16:27:31 lyarwood mdbooth: but given I pinged as I was stuck I think you might win
16:42:49 edmondsw claudiub I ran the UTs for test_compute (which take the longest of the files changed) for both master and my proposal with create_autospec. No significant perf impact
16:43:09 edmondsw not saying there couldn't be an impact somewhere else, but not substantial in that file anyway
16:44:27 edmondsw ran 1934 tests in 336s without and 360s with create_autospec (on my slow laptop env)
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 edmondsw claudiub is that comparing to master or to the oslotests fixture?
17:08:49 mdbooth lyarwood: ovo should handle the lack of contract for obj_load_attr
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: Give volume DriverBlockDevice classes a common prefix https://review.openstack.org/526346
18:01:04 openstackgerrit Matthew Booth proposed openstack/nova master: Add DriverLocalImageBlockDevice https://review.openstack.org/526347
18:01:05 openstackgerrit Matthew Booth proposed openstack/nova master: Expose driver_block_device fields as attributes https://review.openstack.org/528362
18:01:05 openstackgerrit Matthew Booth proposed openstack/nova master: Add local_root to block_device_info https://review.openstack.org/529029
18:01:06 openstackgerrit Matthew Booth proposed openstack/nova master: Pass DriverBlockDevice to driver.attach_volume https://review.openstack.org/528363
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:07 openstackgerrit Matthew Booth proposed openstack/nova master: Fix libvirt volume tests passing invalid disk_info https://review.openstack.org/529328
18:01:07 openstackgerrit Matthew Booth proposed openstack/nova master: Pass disk_info dict to libvirt_info https://review.openstack.org/529329
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: Don't generate fake disk_info in swap_volume https://review.openstack.org/530787
18:01:09 openstackgerrit Matthew Booth proposed openstack/nova master: Local disk serial numbers for the libvirt driver https://review.openstack.org/529380
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.

Earlier   Later