| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-19 | |||
| 16:05:28 | leakypipes | :) | |
| 16:05:33 | fried_rice | Then lobbest thou the holy hand grenade... | |
| 16:05:37 | fried_rice | okay okay. | |
| 16:05:40 | leakypipes | hehe | |
| 16:05:54 | fried_rice | Back to Airplane! quotes. | |
| 16:06:20 | openstackgerrit | Matthew Edmonds proposed openstack/nova master: Fix bad autospec usage https://review.openstack.org/535806 | |
| 16:07:17 | edmondsw | claudiub I would prefer ^ to the oslotest fixture | |
| 16:07:33 | edmondsw | but it's at least a first step even if you want to continue to push use of that fixture | |
| 16:07:42 | edmondsw | (which could be done subsequently) | |
| 16:08:58 | bauzas | ildikov: I don't see any change in the series that does things at the driver level with https://review.openstack.org/#/c/531244/10/nova/compute/manager.py@5190 | |
| 16:09:16 | bauzas | ildikov: I mean, you and matt made the RPC changes for asking for multiattach, sure | |
| 16:09:31 | bauzas | but then, how do we pass that down to libvirt? I'm confused | |
| 16:10:03 | bauzas | oh f***, nevermind | |
| 16:10:05 | ildikov | bauzas: it's in the capabilities | |
| 16:10:26 | ildikov | bauzas: I guess you found it :) | |
| 16:10:30 | bauzas | ildikov: yeah, so basically the RPC change is just for asking whether the compute driver supports it | |
| 16:10:37 | ildikov | bauzas: yes | |
| 16:10:40 | bauzas | if not, raise | |
| 16:10:45 | ildikov | exactly | |
| 16:10:51 | bauzas | if yes, let's the volume say whether it's multiattached | |
| 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 | |