| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-06-27 | |||
| 14:32:31 | edleafe | jaypipes: you're making a claim after the HostState objects are gone | |
| 14:33:11 | jaypipes | edleafe: what do you mean by "after the HostState objects are gone"? | |
| 14:33:48 | edleafe | jaypipes: in _schedule() we have a list of filtered, weighed HostState objects. | |
| 14:34:05 | edleafe | When you return they are no longer accessible | |
| 14:34:27 | jaypipes | edleafe: what do we need the host state objects for after the scheduler driver has filtered and weighed them? | |
| 14:34:38 | edleafe | So whether you're in the scheduler manager or the conductor manager, you're limited to the objects returned from _schedule() | |
| 14:34:39 | jaypipes | edleafe: we just need the resource provider UUIDs. | |
| 14:34:58 | jaypipes | edleafe: exactly! have the scheduler driver do one thing and do it well. | |
| 14:34:59 | edleafe | jaypipes: in a race-free world, sure | |
| 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 | ) | |
| 14:56:47 | jaypipes | for (rc, amount) in resources.items()) | |
| 14:56:47 | jaypipes | sorted("%s:%s" % (rc, amount) | |
| 14:56:47 | jaypipes | resource_query = ",".join( | |
| 14:56:55 | dansmith | le barf | |
| 14:56:59 | jaypipes | resource_query = ",".join(sorted("%s:%s" % (rc, amount) | |
| 14:56:59 | jaypipes | easier to read than this: | |
| 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( | |