| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-11-05 | |||
| 17:28:33 | melwitt | because enforce == first check and verify == second check with +/- 0 | |
| 17:28:41 | lbragstad | the context manager could use the same exact public APIs | |
| 17:28:51 | johnthetubaguy | totally agreed with that | |
| 17:28:55 | melwitt | that actually sounds like it fits nicely | |
| 17:29:02 | johnthetubaguy | +1 | |
| 17:29:16 | lbragstad | ok - sounds like we have some things we can improve in oslo.limit then | |
| 17:29:31 | johnthetubaguy | I wondered if you could just have a single check API that is used in both sides of the context manager, but possibly not | |
| 17:29:52 | johnthetubaguy | i.e. use check in enter and exit | |
| 17:30:12 | lbragstad | you mean make enforce() generic enough to handle all cases? | |
| 17:30:16 | johnthetubaguy | but I think I prefer oslo.limit deciding if a recheck is done or not | |
| 17:30:17 | johnthetubaguy | yeah | |
| 17:30:39 | lbragstad | yeah - i think wxy-xiyuan tried to do something like that initially | |
| 17:31:32 | jaypipes | johnthetubaguy: did you want a review on the patch above? | |
| 17:31:52 | jaypipes | johnthetubaguy: I'm not a fan of mixing usage and limit information in the same function/object | |
| 17:32:33 | jaypipes | johnthetubaguy: lemme push up what I've been working on. perhaps it will make a bit more sense. | |
| 17:32:43 | johnthetubaguy | jaypipes: from a direction point of view, that would be great | |
| 17:32:55 | johnthetubaguy | jaypipes: yeah, that would be cool | |
| 17:33:14 | johnthetubaguy | I have to run now, but will catch up later | |
| 17:33:59 | johnthetubaguy | lbragstad: yeah, the clear separation is almost certainly a win over a smaller API | |
| 17:37:41 | melwitt | not to complicate things but thinking about the APIs, I do wonder how the they'll look in the future when other backends are added, like the etcd locking idea as another way to deal with races | |
| 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 | just return the attribute, not a functools.partial | |
| 18:02:55 | efried | else | |
| 18:02:55 | efried | do the thing it does now | |
| 18:02:55 | efried | if callable: | |
| 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 | return getattr(self.instance, name) | |
| 18:08:44 | efried | self.instance = self.klass(*self.args, **self.kwargs) | |
| 18:08:44 | efried | if self.instance is None: | |
| 18:08:44 | efried | def __getattr__(self, name): | |
| 18:08:44 | efried | bauzas: butbutbut, why wouldn't it work to just do this: | |
| 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 | 'nova.scheduler.client.report.SchedulerReportClient')) | |
| 18:20:54 | efried | self.reportclient = LazyLoader(importutils.import_class( | |
| 18:20:54 | efried | 'nova.scheduler.client.query.SchedulerQueryClient')) | |
| 18:20:54 | efried | self.queryclient = LazyLoader(importutils.import_class( | |
| 18:20:54 | efried | def __init__(self): | |
| 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 | |