Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-11-05
17:12:15 melwitt I think it's definitely a nice option where it fits
17:12:32 lbragstad what if we (oslo.limit) give nova a public API for enforce and recheck (which you can call optionall)?
17:12:32 johnthetubaguy yeah, I like the option of having it
17:12:58 jaypipes stephenfin: yeah, they're on good drive... one sec.
17:13:20 lbragstad and forget the context manager for now
17:13:56 lbragstad in the future, we could expose a context manager that consumes enforce() on __enter__ and recheck() or reverify() on __exit__() for compatibility
17:14:42 johnthetubaguy lbragstad: I think that works, although it makes "claim" seem like the wrong word, its more a resource request
17:14:55 jaypipes stephenfin: https://bit.ly/scheduler-wars-a-new-hope and https://bit.ly/scheduler-wars-revenge-of-the-split
17:15:19 cfriesen The vTPM spec is up at https://review.openstack.org/#/c/571111/ if any cores feel like taking a look. I believe all comments have been addressed.
17:15:31 lbragstad johnthetubaguy yeah - i'm not tied to the terminology, at the time it seemed easy to think of it as "I'm attempting to claim 4 cores on this project"
17:15:35 stephenfin jaypipes: Awesome. Thanks very much
17:15:45 jaypipes np
17:17:14 johnthetubaguy lbragstad: I would be tempted to say include the context manager from the beginning, but its hard to justify if we have no one to use it in v1
17:18:11 lbragstad do you think it's going to be hard to design enforce() and verify() with the hopes that there will be a context manager in the future?
17:19:03 johnthetubaguy lbragstad: that sounds good though, some kind of limit.check call sounds good. Not sure you need a recheck one, depending on the arguments you go for.
17:19:49 lbragstad doesn't nova need a verify() function that isn't associated to the context manager?
17:20:29 lbragstad that way nova-conductor can call it?
17:20:44 johnthetubaguy sorry, I just confused things there
17:22:10 johnthetubaguy if we have the recheck configuration move into olso.limit we need a verify call that takes a list of resource_names to check, only if configured to
17:22:27 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove QuotaEngine.register_resources() https://review.openstack.org/615613
17:22:28 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove get_quota_classes() driver method https://review.openstack.org/615615
17:22:28 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove defaults kwarg in get_project_quotas https://review.openstack.org/615614
17:22:29 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove FakeContext from quota unit tests https://review.openstack.org/615617
17:22:29 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove Context.quota_class https://review.openstack.org/615616
17:22:30 openstackgerrit Jay Pipes proposed openstack/nova master: quota: clean up DbQuotaDriver unit tests https://review.openstack.org/615619
17:22:30 openstackgerrit Jay Pipes proposed openstack/nova master: quota: remove _no_class tests https://review.openstack.org/615618
17:22:45 johnthetubaguy so that is what I waiting to see ^
17:23:13 jaypipes johnthetubaguy: that's mostly just cleanups and test refactoring. still have the new LimitsDriver stuff to push.
17:23:35 lbragstad yeah - i was thinking the main reason for not using a context manager was because nova-conductor does the verification step if configured to do so
17:23:35 johnthetubaguy jaypipes: did you see the thing I did here, for the direction I was thinking: https://review.openstack.org/#/c/615180
17:24:02 jaypipes johnthetubaguy: the series there is just slowly chipping away at the bloated quota driver interface, attempting to simplify it as much as possible before introducing further changes.
17:24:06 johnthetubaguy lbragstad: yeah, its just right now we use a single call, we pass +0 extra resources in the conductor
17:24:54 johnthetubaguy jaypipes: yeah, my thinking was to put a new stack next to the old one, and remove the old stack later, but open to ideas
17:25:16 johnthetubaguy (took me all friday to work out that is what I was actually thinking)
17:27:12 lbragstad johnthetubaguy right - and that should still be possible if oslo.limit gives you enforce() and verify() functions, right?
17:27:34 lbragstad you'd just be calling them from two different places
17:27:58 johnthetubaguy lbragstad: yeah totally, I just was wondering if you needed the separation, I think you probably do
17:28:12 melwitt yeah, you're just thinking allow another option where the APIs are decoupled right? and then if they are co-located in some part of code, you could use the context manager
17:28:27 lbragstad ^ yeah
17:28:33 johnthetubaguy yeah, +1
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

Earlier   Later