Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-06-27
14:35:26 edleafe jaypipes: we had this discussion weeks ago
14:35:29 jaypipes edleafe: alternately, let's change the select_destinations() *driver* method to return the HostState list instead of a list of dicts.
14:35:40 openstackgerrit Artom Lifshitz proposed openstack/nova master: Delete disk metadata when detaching volume https://review.openstack.org/435082
14:35:41 openstackgerrit Artom Lifshitz proposed openstack/nova master: API support for tagged device attachment https://review.openstack.org/376021
14:35:56 edleafe dansmith was arguing for the conductor doing claiming, and you and I were arguing for the scheduler doing the claiming, using the exact thing I'm talking about now
14:36:25 jaypipes edleafe: I'm not doing claiming in the conductor. I'm simply doing it in the scheduler manager not the scheduler driver.
14:36:35 edleafe jaypipes: yes, I know that
14:36:52 edleafe jaypipes: which is why I wrote "functionally" equivalent
14:36:58 jaypipes but it isn't.
14:37:10 edleafe jaypipes: because you no longer have access to the weighed host list
14:37:27 jaypipes edleafe: so have the select_destinations() return a weighed host list, then.
14:37:35 edleafe jaypipes: and if you want to return the full list, well, there goes your separation goal
14:37:36 dansmith edleafe: claiming in scheduler manager is functionally equivalent to claiming in conductor?
14:37:56 edleafe dansmith: as far as having access to the weighed list of hosts, yes
14:38:09 dansmith o.O
14:38:11 jaypipes edleafe: if we return the weighted list to the manager, how does that mean "there goes my separation goal"?
14:38:58 edleafe jaypipes: because HostState is a driver concept
14:39:07 edleafe jaypipes: they don't exist in the manager
14:39:24 jaypipes I'm not saying return the list of host state objects. I'm saying return a weighted list of hosts.
14:39:45 jaypipes in other words, what my patch is doing -- minus the returning alternatives part.
14:40:24 dansmith even still, HostState is not a driver concept, IMHO
14:40:28 dansmith it's defined in host_manager even
14:40:32 edleafe jaypipes: http://p.anticdent.org/3900
14:40:46 dansmith I see no reason to keep that hidden from scheduler manager if it makes things more convenient
14:40:51 jaypipes or even if we did return a list of host state objects, I actually wouldn't care, as long as the manager isn't modifying those objects.
14:41:09 jaypipes or even if we did return a list of host state objects, I actually wouldn't care, as long as the manager isn't modifying those objects.
14:41:20 jaypipes edleafe: ^
14:43:36 edleafe jaypipes: one of your stated concerns was the chance scheduler. Chance scheduler doesn't use HostState. It's specific to filter scheduler
14:44:05 edleafe jaypipes: so I'm not sure how this is separating things more cleanly
14:44:08 jaypipes edleafe: right, which is why I changed the returned value from select_destinations() to be a list of dicts that included the UUID.
14:45:05 edleafe 09:40 < jaypipes>| or even if we did return a list of host state objects, I actually wouldn't care, as long as the manager isn't modifying those objects.
14:45:10 jaypipes edleafe: because the manager will loop through the allocation requests and attempt to claim resources on the destinations returned by select_destinations(). If select_destinations() doesn't return something with that UUID, the manager doesn't have the ability to match an allocation request to a host.
14:46:19 edleafe jaypipes: well, if keeping claiming out of the filter scheduler is worth it, then go for it
14:46:21 jaypipes edleafe: the alternative is to have the _schedule() method of the filter scheduler driver do everything under the sun: filter, weigh, get allocation requests, and claim.
14:47:10 edleafe jaypipes: and IYO it's *so* much cleaner to have the _schedule method do filter, weigh, and get allocation requests
14:47:34 jaypipes edleafe: no, I have the _schedule() method only do filter and weigh.
14:47:44 jaypipes edleafe: the manager does the claiming.
14:47:47 edleafe jaypipes: In my mind, the claim is a final filter. _schedule() isn't doing its job if it returns an invalid host
14:47:52 jaypipes after receiving the set of destinations from the driver.
14:48:20 jaypipes edleafe: what do you mean by "returns an invalid host"? I lost you on that one.
14:48:29 jaypipes and I don't see the claim as a final filter at all.
14:48:34 jaypipes it's a completely different operation.
14:48:37 dansmith definitely not
14:48:42 dansmith if you mean a filterscheduler filter
14:48:46 edleafe jaypipes: in the case of horizontal scaling of the scheduler, when there could be races for the same host
14:49:11 edleafe jaypipes: a filter is logic that determines if a host is acceptable.
14:49:12 dansmith how does claiming in a filter make that better?
14:49:23 edleafe dansmith: not "in a filter".
14:49:28 jaypipes edleafe: there definitely *will* be races for the same host. And the manager will be the thing responsible for retrying those claim attempts on the same or an alternate matching host.
14:49:37 dansmith jaypipes: right
14:49:39 edleafe dansmith: it's the final step in the process
14:49:47 dansmith edleafe: in the manager.
14:50:00 edleafe ok, fine, fine
14:50:26 edleafe it just looks to me like you're trading one entanglement for another..
14:50:32 edleafe But don't let that slow you down
14:50:44 jaypipes ..
14:50:51 dansmith I don't even know how to respond to that
14:50:58 jaypipes I'm not going to.
14:51:02 edleafe you don't have to
14:51:08 edleafe I've stated my concerns
14:51:20 jaypipes I'll go back to trying to clean up the proposed code and add commentary to it.
14:51:30 dansmith jaypipes: before you do, we need to have a serious talk
14:51:37 edleafe if you still want to go down that route, I'll accept that
14:51:53 jaypipes dansmith: yes?
14:52:09 dansmith jaypipes: a couple months ago you started doing this trailing paren-on-a-line-by-itself thing, which I argued about, but let you continue for some large function calls in tests
14:52:22 dansmith jaypipes: but this patch has them all over the place and for calls with only a few parameters that could be a couple lines
14:52:35 dansmith jaypipes: _none_ of our other code looks like that and it's an abrupt change recently
14:52:50 dansmith it looks very different to everything else and it really (really) bugs me
14:53:28 jaypipes dansmith: k, I'll restyle
14:53:43 dansmith jaypipes: I'd *really* appreciate it :)
14:54:58 jaypipes dansmith: just for the parens, right? not the dict notation?
14:55:21 dansmith jaypipes: the parens are what make my eyes bleed, but give me a dict notation example
14:55:45 dansmith if you mean just ending on a line with } that's fine
14:55:54 jaypipes yeah
14:56:10 jaypipes k, will change.
14:56:17 dansmith yeah, we do that everywhere and that's cool
14:56:39 jaypipes dansmith: no prob, will fix.
14:56:45 jaypipes dansmith: just for the record, I find this:
14:56:47 jaypipes resource_query = ",".join(
14:56:47 jaypipes sorted("%s:%s" % (rc, amount)
14:56:47 jaypipes for (rc, amount) in resources.items())
14:56:47 jaypipes )
14:56:55 dansmith le barf
14:56:59 jaypipes easier to read than this:
14:56:59 jaypipes resource_query = ",".join(sorted("%s:%s" % (rc, amount)
14:57:00 jaypipes for (rc, amount) in resources.items()))
14:57:10 dansmith you can make it three lines,
14:57:19 dansmith just don't put the trailing paren on a line by itself
14:57:30 jaypipes loud and clear sir.
14:57:34 dansmith but even that is slightly less smelly than:
14:57:37 dansmith myfunc(
14:57:40 dansmith foo, bar,
14:57:41 dansmith )
14:57:45 dansmith le double barf ^
14:57:49 jaypipes heh
15:03:56 openstackgerrit Andreas Scheuring proposed openstack/nova master: DO NOT MERGE! Dummy patch https://review.openstack.org/478186
15:08:31 efried dansmith +1, thanks for bringing a hammer to that party.
15:09:09 dansmith efried: interventions are sometimes necessary to correct misbehavior
15:09:48 dansmith I learned that from TLC
15:09:54 efried The more you know

Earlier   Later