Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-11-05
17:03:22 lbragstad oh, nice...
17:04:55 johnthetubaguy melwitt: not sure its too much of a problem really, but that is the only recheck that is relevant in the end. Isn't the issue that we need to write into the correct DB before we recheck?
17:06:05 lbragstad for historical context, what was the reason for moving the recheck logic to conductor? Just to have it closer to the database?
17:06:06 melwitt johnthetubaguy: it is, but related to behavior change we have where multi-create causes potentially a lot of instances to fall into ERROR state if recheck fails, I had been thinking we should instead count build requests + instances for the recheck and do that in nova-api instead of doing the conductor thing
17:06:27 melwitt lbragstad: because that's where the instance record is created, yes
17:06:33 lbragstad got it
17:07:16 johnthetubaguy lbragstad: would it not be OK just to make the context manager optional, because when verify=false the context manager is really strange?
17:07:28 melwitt I can propose a WIP patch today to show what I mean. I just hadn't gotten around to trying it out
17:07:29 johnthetubaguy melwitt: oh good thinking... build requests
17:07:35 lbragstad yeah - i guess you're right
17:07:50 lbragstad originally, we were thinking it would make adoption easier, but in this case it might not...
17:08:30 lbragstad we just didn't want to have to require developers to have to remember to put specific ordering in their API code (e.g., don't forget to recheck *here*)
17:09:23 lbragstad seemed like an interesting opportunity to encapsulate some of that behind the __exit__ of a context manager
17:10:13 johnthetubaguy lbragstad: if we make the recheck compulsory, I think it works (like oslo.limit has the configuration about if you recheck or not)
17:10:35 melwitt yeah, I suppose other people might run into a similar issue if they've got a resource create happening in a separate service/process as well. I'm sorry I missed this case at previous oslo.limit sessions :\
17:11:16 lbragstad well - i think this is just one of those things where you don't really see it fall apart until you're knee-deep in the code
17:11:36 johnthetubaguy yeah, I have similar regrets, that PoC patch has really made me think again on a few things
17:12:00 lbragstad which is good, who knows, maybe the context manager is just an over-optimization at this point
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:

Earlier   Later