| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-11-05 | |||
| 17:38:33 | lbragstad | ^ a couple people also had ideas about using etcd as a way to implement more than two levels of hierarchical checking | |
| 17:40:02 | melwitt | to be clear, just thinking to keep those in mind to avoid painting ourselves into a corner for the future. but decoupling APIs shouldn't affect that really | |
| 17:41:59 | lbragstad | ideally - the usage of etcd would be an implementation detail for oslo.limit to handle | |
| 17:42:28 | lbragstad | i would think - i can't really think of a reason why the data coming from nova would change if etcd were being used | |
| 17:46:47 | melwitt | I think it would too, just thinking where it fits. it would be in enforce right? and then verify wouldn't be used in that case? | |
| 17:47:40 | melwitt | like, is verify() valid depending on backend? | |
| 17:58:43 | efried | bauzas: LazyLoader? | |
| 18:00:10 | bauzas | efried: wat? | |
| 18:00:26 | efried | bauzas: I'm wondering if there's a reason functools.partial was necessary there. | |
| 18:00:41 | bauzas | oh about my concern? | |
| 18:00:53 | bauzas | you mean functools.wraps ? | |
| 18:01:08 | bauzas | .partial is different | |
| 18:01:31 | efried | bauzas: This is unrelated to anything | |
| 18:01:46 | bauzas | not sure I understand you | |
| 18:01:59 | efried | bauzas: I'm running into a unit test snafu due to LazyLoader because I'm trying to access self.compute.reportclient._provider_tree | |
| 18:02:09 | efried | LazyLoader is returning _provider_tree as a functools.partial | |
| 18:02:23 | efried | because it's only set up to return *callable* attributes from the thing it's shimming. | |
| 18:02:54 | efried | So I'm trying to figure out if there's a way for me to do | |
| 18:02:55 | efried | if callable: | |
| 18:02:55 | efried | do the thing it does now | |
| 18:02:55 | efried | else | |
| 18:02:55 | efried | just return the attribute, not a functools.partial | |
| 18:03:13 | efried | but in order to do that, I'm going to have to collapse it | |
| 18:05:03 | efried | bauzas: https://review.openstack.org/#/c/104556/9..14/nova/scheduler/client/__init__.py | |
| 18:05:48 | bauzas | ah this | |
| 18:06:36 | efried | I fully expect you to 100% remember all the reasoning that went on behind this change from four years ago. | |
| 18:07:40 | bauzas | efried: .partial() is because we don't want to pass __name | |
| 18:08:02 | bauzas | example https://stackoverflow.com/questions/15331726/how-does-the-functools-partial-work-in-python | |
| 18:08:44 | efried | bauzas: butbutbut, why wouldn't it work to just do this: | |
| 18:08:44 | efried | def __getattr__(self, name): | |
| 18:08:44 | efried | if self.instance is None: | |
| 18:08:44 | efried | self.instance = self.klass(*self.args, **self.kwargs) | |
| 18:08:44 | efried | return getattr(self.instance, name) | |
| 18:10:47 | efried | (FWIW, it seems to do what I need) | |
| 18:12:07 | bauzas | yup, but then that's not a lazy loader ;) | |
| 18:12:23 | efried | bauzas: Say wha? | |
| 18:13:23 | efried | bauzas: The only difference in laziness is if a caller pulled a method but didn't call it. | |
| 18:13:32 | bauzas | efried: calling __getattr__ in your case will run getattr() of the instance, right? | |
| 18:13:47 | efried | yes | |
| 18:14:07 | bauzas | so, it's executing synchronously | |
| 18:14:28 | bauzas | the problem is when importing the module | |
| 18:14:46 | bauzas | if you don't lazy load it, then you're loopîng | |
| 18:15:09 | bauzas | see why I lazy-load it below | |
| 18:15:14 | efried | You mean getattr looping on itself? | |
| 18:15:25 | bauzas | nope | |
| 18:15:52 | bauzas | we will only import the modules when we call select_destinations() | |
| 18:16:14 | bauzas | *not when we import nova.scheduler.client* :) | |
| 18:16:40 | efried | that... still happens? | |
| 18:17:15 | efried | Never mind about why it matters that we wait to import the report client... | |
| 18:17:19 | bauzas | efried: see this blogpost https://snarky.ca/lazy-importing-in-python-3-7/ | |
| 18:19:08 | bauzas | efried: anyway, test it | |
| 18:19:22 | bauzas | efried: when I wrote this, we were having an import loop | |
| 18:19:29 | bauzas | hence the lazyload | |
| 18:19:32 | efried | bauzas: Test it how? Is there test code somewhere that verifies that the importing is being done lazily? | |
| 18:19:43 | bauzas | but now we changed a lot of the client, so maybe it's not needed | |
| 18:20:01 | bauzas | efried: you can pdb it, right? | |
| 18:20:11 | efried | I would think so. | |
| 18:20:19 | bauzas | or you can just remove the lazyloader and test whether we still have the import loop | |
| 18:20:30 | bauzas | if you have concerns by this class | |
| 18:20:39 | bauzas | anyway, 7:20pm here | |
| 18:20:40 | bauzas | ++ | |
| 18:20:54 | efried | def __init__(self): | |
| 18:20:54 | efried | self.queryclient = LazyLoader(importutils.import_class( | |
| 18:20:54 | efried | 'nova.scheduler.client.query.SchedulerQueryClient')) | |
| 18:20:54 | efried | self.reportclient = LazyLoader(importutils.import_class( | |
| 18:20:54 | efried | 'nova.scheduler.client.report.SchedulerReportClient')) | |
| 18:21:07 | efried | Pretty sure that -^ is actually doing the import right away. | |
| 18:21:47 | efried | You're calling LazyLoader with the *result* of running importutils.import_class(), which is the imported class. | |
| 18:24:06 | lbragstad | melwitt that's a good question, verify is going to need the current usage and limit information, which means it might need to use etcd | |
| 18:24:10 | lbragstad | to get that information | |
| 18:24:18 | cdent | efried: sure looks that way to me too | |
| 18:24:27 | cdent | the reason it avod the import loop is simply because it is called "later" | |
| 18:24:34 | cdent | but it isn't lazy | |
| 18:24:43 | efried | yuh. I guess I need to pull this part out into a separate patch, sigh. | |
| 18:26:11 | melwitt | lbragstad: I was wondering, is verify() even useful though if we are distributed locking on enforce + create? I dunno, just unhelpfully thinking aloud :P | |
| 18:26:56 | lbragstad | oh... i missed the distributed lock part | |
| 18:27:17 | lbragstad | using etcd as a distributed lock would prevent the need for verify(), then? | |
| 18:30:15 | melwitt | that's what I was thinking. this came up at... was it the forum in vancouver? someone asked why do the recheck thing to avoid races, why not use distributed locking instead, and people had some ideas about using etcd as part of distributed locking. and then we were thinking that could be just one of many potential approaches that oslo.limit could provide underneath | |
| 18:30:30 | melwitt | *to handle races | |
| 18:31:08 | lbragstad | oh - sure | |
| 18:31:19 | lbragstad | i haven't thought about that specifically, yet | |
| 18:31:22 | melwitt | and I was just thinking whether that affects the API we're thinking. maybe it would just make verify() optional in the case of that backend | |
| 18:31:26 | efried | cdent, bauzas: oic now. The lazy loader is deferring the *instantiation* of the class until some method is called on it. The import of the module is still happening as soon as you instantiate SchedulerClient. But the arg being passed to the LazyLoader is the class object, not an instance of that class. | |
| 18:31:26 | efried | And IMO that doesn't really save you anything over just instantiating the class right away in the SchedulerClient init. | |
| 18:31:40 | cdent | right | |
| 18:32:02 | cdent | It's not clear why you'd want to wait | |
| 18:32:08 | melwitt | lbragstad: yeah. too early to think too much about it but at the same time, throwing it out there in case it would cause a big problem with what we're thinking about for now | |
| 18:32:08 | lbragstad | in that same session i want to say we were talking about using etcd as a way to communicate events between nova (oslo.limit) and keystone | |
| 18:32:20 | melwitt | ah, gotcha | |
| 18:32:55 | lbragstad | because - once you go past the 2nd layer of projects, it becomes harder to make assumptions about the resources in the tree, i think | |
| 18:33:29 | dansmith | and if you take the locking approach, | |
| 18:33:51 | dansmith | you have to figure out how high to lock | |
| 18:34:13 | lbragstad | http://specs.openstack.org/openstack/keystone-specs/specs/keystone/rocky/strict-two-level-enforcement-model.html#limit-and-usage-awareness-across-endpoints | |
| 18:34:25 | lbragstad | dansmith yeah - that's another important bit | |
| 18:35:45 | melwitt | lbragstad: thanks for the link, I had missed that | |
| 18:36:32 | lbragstad | yeah, turns out that was an important bit to include as justification for why we decided to keep things at 2 levels | |
| 19:30:24 | openstackgerrit | Jay Pipes proposed openstack/nova master: quota: rename arguments to clarify they are limits https://review.openstack.org/615633 | |
| 19:31:21 | lbragstad | does nova allow users to create multiple instances in multiple projects in one request? | |
| 19:31:39 | melwitt | no | |
| 19:31:51 | lbragstad | ok - cool, just wanted to double check | |