Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-01-19
16:10:54 bauzas gotcha
16:10:59 claudiub edmondsw: sure, that works too, but i've had nightmares with mock.create_autospec
16:11:18 bauzas ildikov: sorry, I'm in general used to see things happening at the manager level once you pass a new attribute
16:11:27 ildikov bauzas: yeah, many pieces to put together
16:11:27 bauzas a new *RPC* attribute
16:11:34 claudiub I've had run times increase exponentially because of mock.create_autospec
16:11:43 bauzas ildikov: yeah, https://review.openstack.org/#/c/532928/4/nova/compute/manager.py does that
16:12:26 edmondsw claudiub hmm
16:12:33 ildikov bauzas: we tried to look into failing as quick as possible if either bit is not there
16:13:06 claudiub at one time, i've had an execution time of 4000 seconds for ~300 unit tests. :)
16:13:18 claudiub aprox.
16:13:21 ildikov bauzas: as we don't have all the info at the API we needed some tricks :)
16:14:40 bauzas ildikov: I'll +W it, but I think you should add a NOTE in the manager explaining why you don't really care doing things after the RPC call
16:15:08 bauzas ildikov: just because it took me some time to understand that, some other people wanting to contribute could be confused by that new attribute we don't really use
16:15:15 bauzas it's just a flag
16:15:53 ildikov bauzas: sure, I guess you added a comment already, so I will do it in a follow up patch
16:16:00 ildikov bauzas: thank you
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

Earlier   Later