Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-12-20
14:20:18 ameeda Hello, Can you please review my code here ? https://review.openstack.org/#/c/528069/
14:23:47 stephenfin ameeda: If you could add a little more context to that, it would make it much easier to review :)
14:29:11 stephenfin mriedem: Two questions left on https://review.openstack.org/#/c/529185/. I can ignore them if you really want to get it in now (or I've misunderstood something)
14:29:18 stephenfin The first one is a-ok
14:31:41 ameeda stephenfin: thanks for comment, I change the commit and I will submit it now
14:32:25 mriedem stephenfin: replied
14:34:41 stephenfin Cool. +2 to that too, in that case
14:39:45 mriedem stephenfin: thanks
14:39:51 openstack Launchpad bug 1739323 in OpenStack Compute (nova) pike "KeyError in host_manager for _get_host_states" [High,Confirmed]
14:39:51 mriedem mnaser: i'm at a loss for how you're hittin this https://bugs.launchpad.net/nova/+bug/1739323
14:40:19 mriedem mnaser: dug through it again and the entries in seen_nodes should all also be in host_state_map because of https://github.com/openstack/nova/blob/3ebc358002d3f62a9ff0a3b0f50fea4f2a5c2c91/nova/scheduler/host_manager.py#L698
14:40:50 mriedem and i don't see anything else in the HostStateManager that modifies that dict, so no locking issues that i can see, unless you have a multiple workers patch or something
14:41:57 mriedem mdbooth: maybe your keen eye can spot an obvious issue in here ^
14:50:04 openstackgerrit Ameed Ashour proposed openstack/nova master: detach instance volumes when VM creation fails https://review.openstack.org/528385
14:50:04 openstackgerrit Ameed Ashour proposed openstack/nova master: detaching volumes when deleted VM is in error state https://review.openstack.org/528069
14:55:41 ameeda stephenfin: please check the comment and let me know, Thanks
14:57:30 openstackgerrit Matt Riedemann proposed openstack/nova master: Cleanups for the scheduler code https://review.openstack.org/529343
15:00:52 mdbooth mriedem_away: I'm a little lacking in scheduler context. It seems to me that we would hit this if the scheduler was capable of evaluating filters 'concurrently'.
15:01:11 mdbooth For some pythonic/eventlet definition of concurrently, obviously.
15:01:36 stephenfin lyarwood, mdbooth: Think you could take a look at this at some point? BDM'y things https://review.openstack.org/#/c/528069
15:01:39 mdbooth Does the scheduler serialize all scheduling requests?
15:04:51 jaypipes mdbooth: no
15:05:34 mdbooth jaypipes: Thanks. That looks like a bug, then.
15:05:53 jaypipes mdbooth: what's that?
15:06:14 mdbooth https://bugs.launchpad.net/nova/+bug/1739323/
15:06:15 openstack Launchpad bug 1739323 in OpenStack Compute (nova) "KeyError in host_manager for _get_host_states" [High,Incomplete]
15:06:21 jaypipes ah
15:06:25 jaypipes lemme looksie
15:06:34 mdbooth _get_host_states is returning an iterator over global state
15:06:52 mdbooth So if any 2 'threads' can be calling it simultaneously, it's potentially corrupt
15:07:01 openstackgerrit wes hayutin proposed openstack/nova master: DNM, testing only https://review.openstack.org/529349
15:07:18 mdbooth Especially as it's returning an iterator, so the evaluation period is extended
15:08:12 mdbooth I guess you'd expect to see this if you delete a compute node on a busy system?
15:08:17 mdbooth mnaser: ^^^ ?
15:09:05 jaypipes mdbooth: _get_host_states is returning a tuple, not an iterator.
15:09:22 mdbooth jaypipes: This line:
15:09:34 mdbooth return (self.host_state_map[host] for host in seen_nodes)
15:09:41 jaypipes is a tuple.
15:09:42 mdbooth That's a generator expression, no?
15:09:59 jaypipes I don't believe so...
15:10:14 edleafe jaypipes: it's a generator expression
15:10:17 jaypipes a generator would yield or return a function that yield'd, no?
15:10:58 mdbooth The expression itself is a generator
15:11:13 openstackgerrit Stephen Finucane proposed openstack/nova master: objects: Add PCI NUMA policy fields https://review.openstack.org/527470
15:11:14 openstackgerrit Stephen Finucane proposed openstack/nova master: trivial: Modify signature of _filter_non_requested_pfs https://review.openstack.org/527473
15:11:14 openstackgerrit Stephen Finucane proposed openstack/nova master: Add PCI NUMA policies https://review.openstack.org/527472
15:11:17 edleafe jaypipes: https://github.com/openstack/nova/blob/master/nova/scheduler/filter_scheduler.py#L139-L143
15:11:47 edleafe it calls it an iterator, which is technically correct, but it's a generator
15:11:55 mdbooth jaypipes: So [foo for foo in foos] is a list comprehension
15:11:56 stephenfin cfriesen, bauzas: Fancy taking a look at ^^^ again? Has been changed quite a lot (hopefully for the better)
15:12:11 mdbooth And (foo for foo in foos) is a generator expression which iterates over the same thing
15:13:37 jaypipes hmm, interesting.
15:14:34 mdbooth So if that last line is returning a generator, it's essentially a closure over seen_nodes, which is local, and self.host_state_map, which is global.
15:15:08 jaypipes yeah, I understand that now. Why don't we just list() that instead of returning a generator?
15:15:26 mdbooth Well that would still be a race, just a shorter one
15:15:52 openstack Launchpad bug 1739440 in OpenStack Compute (nova) "Ironic virt driver is hardcoding Ironic API microversion" [Undecided,New]
15:15:52 dtantsur hey folks! I've filed https://bugs.launchpad.net/nova/+bug/1739440 to track ironic API version negotiation in the virt driver.
15:16:02 dtantsur do you think it makes sense as a bug or a blueprint or a spec or ...?
15:16:03 jaypipes cool, thanks dtantsur
15:16:13 edleafe jaypipes: in a large deployment, it was feared that the number of hosts returned would be huge
15:16:15 jaypipes dtantsur: up to mriedem_away
15:16:35 jaypipes edleafe: so premature optimization, then. just like 90% of the code in the scheduler.'
15:16:44 edleafe yeah, pretty much
15:16:50 dtantsur jaypipes: how away is mriedem_away? :)
15:16:57 jaypipes dtantsur: no idea :)
15:17:08 mdbooth dtantsur: He wasn't away within the last 30 minutes.
15:17:17 edleafe I don't know if that was a result of real-world problems, or just premature, TBH
15:17:34 dtantsur okay, so not like away-for-the-reminder-of-the-year state of being away :)
15:17:50 mdbooth dtantsur: mriedem_away isn't allowed PTO
15:18:04 dtantsur I see, makes sense :D
15:22:43 jaypipes mdbooth: bauzas changed the return of _get_host_states() from an iterator over host_state_map's values to the generator expression that included the local seen_nodes variable here: https://github.com/openstack/nova/commit/4660333d0d97d8e00cf290ea1d4ed932f5edc1dc#diff-978b9f8734365934eaf8fbb01f11a7d7L624
15:23:37 jaypipes mdbooth: might be worth trying to reproduce this race before and after that patch to see if it makes a diff
15:24:32 openstackgerrit Matthew Booth proposed openstack/nova master: Fix an error in _get_host_states when deleting a compute node https://review.openstack.org/529352
15:24:38 jaypipes note that I'm not saying anything bad about bauzas' patch! :) I'm just pointing out that's the patch that changed from returning an iterator over the global state to a generator expression over local state
15:25:03 mdbooth jaypipes: Looking
15:25:24 openstackgerrit Chris Dent proposed openstack/nova master: Provide example for placement last-modified header of now https://review.openstack.org/529354
15:25:32 jaypipes mdbooth: of course, trying to reproduce this reliably is virtually impossible...
15:25:57 mdbooth jaypipes: I've written a couple of unit tests now which play with execution order
15:26:03 mdbooth They're always really hard to read, though
15:26:59 jaypipes yta
15:27:01 jaypipes ya
15:28:51 mdbooth Although as this is a generator it might be pretty easy
15:37:18 mdbooth jaypipes: mriedem_away Reproduced it
15:37:37 mdbooth Not sure if the unit test is worth anything, though, as it's irrelevant with my patch in place
15:38:55 mdbooth You have to create an iterator from the returned list in order to trigger a bug evaluating the iterator, which really doesn't make sense if it doesn't return an iterator
15:40:44 mdbooth http://paste.openstack.org/show/629469/
15:41:09 mdbooth ^^^ There's the test and its execution, anyway. But I don't think it makes sense to include it if we're not returning an iterator any more.
15:53:59 openstackgerrit Jay Pipes proposed openstack/nova master: WIP Support aggregate affinity filters https://review.openstack.org/529201
15:53:59 openstackgerrit Jay Pipes proposed openstack/nova master: Remove server group sched filter support caching https://review.openstack.org/529200
16:12:48 mriedem just read the scrollback
16:12:59 mriedem https://review.openstack.org/529343
16:13:05 mriedem i thought it was a tuple of HostState objects too
16:13:18 mriedem because of the change jaypipes pointed out where we stopped using six.itervalues
16:14:38 mriedem mdbooth: so with your change, we lose any yield behavior on the generator for the 10s of thousands of nodes optimization?
16:19:34 mnaser ok reading scrollback
16:19:43 mriedem ah v
16:19:44 mriedem http://intermediatepythonista.com/python-generators#generator-expressions_1
16:19:50 mriedem ok i'm sorely lacking in this area then
16:20:20 mnaser ok but forgive me if i'm being unclear but i think the problem is a lot simpler than that
16:20:35 mnaser seen_nodes contains a tuple list of (host, node) of all hosts in the system

Earlier   Later