| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-17 | |||
| 14:43:57 | openstackgerrit | Artem Tiumentcev proposed openstack/python-novaclient master: Add 'local_gb' disk quota support https://review.openstack.org/484391 | |
| 15:02:50 | openstackgerrit | Steve Noyes proposed openstack/nova master: update live migration to use v3 cinder api https://review.openstack.org/463987 | |
| 15:09:00 | mriedem | sdague: can you go through this new style cinder attachments swap volume change? https://review.openstack.org/#/c/456971/ - i'm good with it and jay is +2, | |
| 15:09:34 | mriedem | sdague: we really need another core to help review the new style cinder attachments stuff, so if you could help here this week it'd be really good | |
| 15:10:24 | mriedem | keep in mind, everything is keyed off the bdm.attachment_id field being set - which doesn't happen until the very last patch where we turn this all on - which only happens when (1) cinder is new enough to provide 3.27 and (2) all computes are upgraded | |
| 15:11:44 | gibi | mriedem: hi! I have a fix up for the volume_swap notification test instability https://review.openstack.org/#/c/484288/ | |
| 15:13:15 | mriedem | gibi: thanks, +2 | |
| 15:21:51 | gibi | mriedem: thanks | |
| 15:23:34 | openstackgerrit | Artem Tiumentcev proposed openstack/python-novaclient master: Add 'local_gb' disk quota support https://review.openstack.org/484391 | |
| 15:25:38 | mriedem | sfinucan: i'm debugging a fun issue with NUMATopologyLimits not getting serialized in the scheduler with a new oslo.serialization change | |
| 15:25:47 | mriedem | http://logs.openstack.org/87/482287/6/check/gate-cross-nova-func-ubuntu-xenial/a645ce8/console.html#_2017-07-16_14_02_02_649384 | |
| 15:25:59 | mriedem | that's due to this change in oslo.serialization https://review.openstack.org/#/c/327564/ | |
| 15:26:16 | mriedem | which is failing here: https://review.openstack.org/#/c/482287/ | |
| 15:27:36 | mriedem | i'm trying to sort out where this is getting serializaed | |
| 15:27:38 | mriedem | *serialized | |
| 15:27:48 | mriedem | obviously the select_destinations call | |
| 15:28:45 | mriedem | oh i think i see, | |
| 15:28:59 | mriedem | the request spec is serialized, and contains numa_topology which comes from the HostState object: | |
| 15:29:00 | mriedem | host_state.limits['numa_topology'] = limits | |
| 15:29:12 | mriedem | set via the NUMATopologyFilter | |
| 15:31:22 | mriedem | still not sure why that would fail to serialize | |
| 15:32:49 | jangutter | newbie question about the gate: is there a way to inform it that the unit tests require an updated os-vif? | |
| 15:33:06 | mriedem | jangutter: depends-on | |
| 15:33:08 | mriedem | buth, | |
| 15:33:11 | mriedem | *but, | |
| 15:33:22 | mriedem | you have to get the changes merged and released in os-vif first | |
| 15:33:46 | jangutter | mriedem: yah, thought as much. | |
| 15:33:46 | mriedem | you can't depends-on a source change in a library w/o it being released first in most jobs | |
| 15:36:22 | jangutter | mriedem: funnily enough, the non-test code seem to load the patched os-vif. | |
| 15:36:45 | jangutter | mriedem: I presume it's because tox creates a sandbox. | |
| 15:40:17 | mriedem | i'm not really understanding what you're saying | |
| 15:40:41 | mriedem | it's all going to run in a tox venv with dependencies using the versions from upper-constraints | |
| 15:40:50 | mriedem | https://github.com/openstack/requirements/blob/master/upper-constraints.txt | |
| 15:41:04 | mriedem | so until the change in os-vif is released and in upper-constraints, then the tox venv isn't using it | |
| 15:41:07 | mriedem | at least in the CI system | |
| 15:41:24 | mriedem | so either your local venv is hacked or it's not working the way you think | |
| 15:42:12 | jangutter | mriedem: Yeah, I hacked my local venv (by running install on os-vif), but only for the unit tests. | |
| 15:43:45 | mriedem | sfinucan: this is what we get right before select_destinations blows up http://paste.openstack.org/show/615631/ | |
| 15:44:10 | sdague | mriedem: will look | |
| 15:44:22 | mriedem | sdague: thanks, poke me with questions | |
| 15:46:08 | bauzas | jaypipes: dansmith: mriedem: sorry, was off last friday, but just saw https://review.openstack.org/#/c/483564/2 | |
| 15:47:10 | bauzas | jaypipes: dansmith: mriedem: so we're now passing a full list of weighed hosts to _schedule() even if don't really use those | |
| 15:47:43 | bauzas | jaypipes: dansmith: mriedem: I get of course why we need to do such in later changes but I do wonder of the performance penalty | |
| 15:48:10 | mriedem | huh? | |
| 15:48:15 | bauzas | jaypipes: dansmith: mriedem: the performance penalty being for example a list of 1000 nodes | |
| 15:48:19 | mriedem | what's getting passed to _schedule that you are worried about? | |
| 15:48:25 | bauzas | given we don't pass an iterator but a full ist | |
| 15:48:26 | bauzas | list | |
| 15:48:40 | bauzas | mriedem: https://review.openstack.org/#/c/483564/2/nova/scheduler/filter_scheduler.py@150 | |
| 15:48:59 | bauzas | that's a list, not an iterator | |
| 15:49:17 | mriedem | you mean not what's passed to _schedule, you mean what's passed from _get_sorted_hosts and used in _schedule | |
| 15:49:27 | bauzas | so we literally build in memory an object that could be large just for the purpose of selecting only the first item | |
| 15:49:34 | bauzas | yup, that | |
| 15:49:51 | bauzas | I'd frankly be more happy if that was an iterator | |
| 15:50:00 | bauzas | but I could be picky | |
| 15:50:07 | bauzas | just raising my concern out loud | |
| 15:50:16 | bauzas | not worth a revert of course | |
| 15:50:24 | mriedem | so push a change to do that | |
| 15:50:38 | mriedem | or leave a comment in the patch | |
| 15:50:51 | bauzas | mriedem: I can workout a patch | |
| 15:54:14 | bauzas | jaypipes: mriedem: oh looks like https://review.openstack.org/#/c/483564/2 will not land because of a functional test issue | |
| 15:54:17 | mriedem | i see the "# Note: remember, we are using an iterator here." on L93 but which variable is the iterator? is it 'hosts' returned from _get_all_host_states ? | |
| 15:54:30 | mriedem | because i think that returns a tuple | |
| 15:55:04 | mriedem | HostManager.get_host_states_by_uuids calls _get_host_states which returns a tuple | |
| 15:56:35 | mriedem | I think get_filtered_hosts is what converts to an iterator | |
| 15:56:41 | mriedem | hosts = six.itervalues(name_to_cls_map) | |
| 15:56:43 | bauzas | mriedem: the whole filters/weighers accepts an iterator | |
| 15:56:45 | bauzas | AFAIR | |
| 15:56:48 | mriedem | and passes that to filter_handler.get_filtered_objects | |
| 15:56:53 | bauzas | mriedem: yup | |
| 15:57:01 | mriedem | and ^ calls the filters | |
| 15:57:12 | bauzas | mriedem: but the thing is that we build a list once we get the weighed hosts | |
| 15:58:07 | mriedem | doesn't get_weighed_hosts return a sorted list? | |
| 15:59:36 | mriedem | ew now i see that WeighedObject is a wrapper around a HostState - wonder why someone used a wrapper rather than subclass HostState with a weight field? | |
| 15:59:45 | mriedem | WeighedHostState(HostState) | |
| 15:59:54 | mriedem | that's why we have that ugly host.obj.foo all over | |
| 16:00:37 | mriedem | anyway, i'm a bit lost in here wrt the iterator | |
| 16:01:13 | bauzas | yeah, seems like it's uglier than I thought | |
| 16:01:26 | mriedem | bauzas: the new _get_sorted_hosts method is still calling host_manager.get_filtered_hosts which still returns an iterator | |
| 16:01:44 | bauzas | mriedem: I was remembering we were using iterators, but looks like our filtering/weighing helpers work with lists... | |
| 16:01:46 | mriedem | and that iterator is still passed to host_manager.get_weighed_hosts | |
| 16:01:59 | mriedem | and weighed_hosts comes back as a sorted list | |
| 16:02:01 | mriedem | not an iterator | |
| 16:02:03 | mriedem | from what i can tell | |
| 16:02:07 | bauzas | yup | |
| 16:02:33 | mriedem | and by the time you get to chosen_host = hosts[0] it's really no different from before | |
| 16:02:38 | mriedem | so seems like something that doesn't matter to this change | |
| 16:02:42 | bauzas | I was just concerned by the new code taking more memory than previously, but it's just honestly not the case | |
| 16:02:48 | bauzas | yup | |
| 16:03:13 | bauzas | because we did work with iterators but once you enter filtering/weighting you're back to full lists of hosts anyway | |
| 16:03:42 | bauzas | anyway, I'm now looking at why that patch failed | |
| 16:05:23 | bauzas | mmmmpf http://logs.openstack.org/64/483564/2/gate/gate-nova-tox-functional-ubuntu-xenial/0f4534d/console.html#_2017-07-17_15_20_35_769898 | |
| 16:09:25 | mriedem | shouldn't that also be running through other filters? | |
| 16:09:35 | jaypipes | mriedem, bauzas: that whole comment about it being an iterator is nonsense. As soon as it enters the filter host manager, it converts it all to a list: https://github.com/openstack/nova/blob/master/nova/filters.py#L69 | |
| 16:09:39 | mriedem | like ServerGroupAntiAffinityFilter | |
| 16:09:58 | mordred | mriedem, sdague: if a client sends the legacy microversion header to a new nova, that's not an error, right? and if it sends both, that's also acceptable? | |
| 16:09:59 | jaypipes | mriedem: it was always trying to be too clever for its own good, IMHO. | |
| 16:10:07 | bauzas | jaypipes: yup, just saw that, see above the IRC discussion with matt | |
| 16:10:28 | bauzas | jaypipes: honestly, I'm not surprised | |
| 16:10:30 | sdague | mordred: correct, either are accepted | |